| Message ID | 20210319042923.1584593-1-elder@linaro.org |
|---|---|
| Headers | show |
| Series | net: ipa: fix validation | expand |
On 3/18/21 11:29 PM, Alex Elder wrote: > There is sanity checking code in the IPA driver that's meant to be > enabled only during development. This allows the driver to make > certain assumptions, but not have to verify those assumptions are > true at (operational) runtime. This code is built conditional on > IPA_VALIDATION, set (if desired) inside the IPA makefile. Given the pushback on the ipa_assert() patch I will send out version 2 of this series, omitting the two patches that involve assertions. I still think there's a case for my proposal, but I'm going to move on for now and try to find other ways to do what I want. In some cases BUILD_BUG_ON() or WARN_ON_DEV() could be used. In other spots, I might be able to use dev_dbg() for checking things only while developing. But there remain a few cases where none of these options is quite right. If I ever want to suggest an assertion again I'll do it as an RFC and will copy Leon and Andrew, to make sure they can provide input. Thanks. -Alex > Unfortunately, this validation code has some errors. First, there > are some mismatched arguments supplied to some dev_err() calls in > ipa_cmd_table_valid() and ipa_cmd_header_valid(), and these are > exposed if validation is enabled. Second, the tag that enables > this conditional code isn't used consistently (it's IPA_VALIDATE > in some spots and IPA_VALIDATION in others). > > This series fixes those two problems with the conditional validation > code. > > In addition, this series introduces some new assertion macros. I > have been meaning to add this for a long time. There are comments > indicating places where assertions could be checked throughout the > code. > > The macros are designed so that any asserted condition will be > checked at compile time if possible. Otherwise, the condition > will be checked at runtime *only* if IPA_VALIDATION is enabled, > and ignored otherwise. > > NOTE: The third patch produces two bogus (but understandable) > warnings from checkpatch.pl. It does not recognize that the "expr" > argument passed to those macros aren't actually evaluated more than > once. In both cases, all but one reference is consumed by the > preprocessor or compiler. > > A final patch converts a handful of commented assertions into > "real" ones. Some existing validation code can done more simply > with assertions, so over time such cases will be converted. For > now though, this series adds this assertion capability. > > -Alex > > Alex Elder (4): > net: ipa: fix init header command validation > net: ipa: fix IPA validation > net: ipa: introduce ipa_assert() > net: ipa: activate some commented assertions > > drivers/net/ipa/Makefile | 2 +- > drivers/net/ipa/gsi_trans.c | 8 ++--- > drivers/net/ipa/ipa_assert.h | 50 ++++++++++++++++++++++++++++++++ > drivers/net/ipa/ipa_cmd.c | 53 ++++++++++++++++++++++------------ > drivers/net/ipa/ipa_cmd.h | 6 ++-- > drivers/net/ipa/ipa_endpoint.c | 6 ++-- > drivers/net/ipa/ipa_main.c | 6 ++-- > drivers/net/ipa/ipa_mem.c | 6 ++-- > drivers/net/ipa/ipa_reg.h | 7 +++-- > drivers/net/ipa/ipa_table.c | 11 ++++--- > drivers/net/ipa/ipa_table.h | 6 ++-- > 11 files changed, 115 insertions(+), 46 deletions(-) > create mode 100644 drivers/net/ipa/ipa_assert.h >
On 3/20/21 8:24 AM, Alex Elder wrote: > On 3/18/21 11:29 PM, Alex Elder wrote: >> There is sanity checking code in the IPA driver that's meant to be >> enabled only during development. This allows the driver to make >> certain assumptions, but not have to verify those assumptions are >> true at (operational) runtime. This code is built conditional on >> IPA_VALIDATION, set (if desired) inside the IPA makefile. > > Given the pushback on the ipa_assert() patch I will send > out version 2 of this series, omitting the two patches > that involve assertions. I posted version 2, but I think dropping two patches without changing the subject might have messed up the robots. I don't know how to fix that and don't want to make any more trouble by trying. If there's something I can do, someone please tell me. -Alex > I still think there's a case for my proposal, but I'm > going to move on for now and try to find other ways > to do what I want. In some cases BUILD_BUG_ON() or > WARN_ON_DEV() could be used. In other spots, I might > be able to use dev_dbg() for checking things only > while developing. But there remain a few cases where > none of these options is quite right. > > If I ever want to suggest an assertion again I'll do > it as an RFC and will copy Leon and Andrew, to make > sure they can provide input. > > Thanks. > > -Alex > >> Unfortunately, this validation code has some errors. First, there >> are some mismatched arguments supplied to some dev_err() calls in >> ipa_cmd_table_valid() and ipa_cmd_header_valid(), and these are >> exposed if validation is enabled. Second, the tag that enables >> this conditional code isn't used consistently (it's IPA_VALIDATE >> in some spots and IPA_VALIDATION in others). >> >> This series fixes those two problems with the conditional validation >> code. >> >> In addition, this series introduces some new assertion macros. I >> have been meaning to add this for a long time. There are comments >> indicating places where assertions could be checked throughout the >> code. >> >> The macros are designed so that any asserted condition will be >> checked at compile time if possible. Otherwise, the condition >> will be checked at runtime *only* if IPA_VALIDATION is enabled, >> and ignored otherwise. >> >> NOTE: The third patch produces two bogus (but understandable) >> warnings from checkpatch.pl. It does not recognize that the "expr" >> argument passed to those macros aren't actually evaluated more than >> once. In both cases, all but one reference is consumed by the >> preprocessor or compiler. >> >> A final patch converts a handful of commented assertions into >> "real" ones. Some existing validation code can done more simply >> with assertions, so over time such cases will be converted. For >> now though, this series adds this assertion capability. >> >> -Alex >> >> Alex Elder (4): >> net: ipa: fix init header command validation >> net: ipa: fix IPA validation >> net: ipa: introduce ipa_assert() >> net: ipa: activate some commented assertions >> >> drivers/net/ipa/Makefile | 2 +- >> drivers/net/ipa/gsi_trans.c | 8 ++--- >> drivers/net/ipa/ipa_assert.h | 50 ++++++++++++++++++++++++++++++++ >> drivers/net/ipa/ipa_cmd.c | 53 ++++++++++++++++++++++------------ >> drivers/net/ipa/ipa_cmd.h | 6 ++-- >> drivers/net/ipa/ipa_endpoint.c | 6 ++-- >> drivers/net/ipa/ipa_main.c | 6 ++-- >> drivers/net/ipa/ipa_mem.c | 6 ++-- >> drivers/net/ipa/ipa_reg.h | 7 +++-- >> drivers/net/ipa/ipa_table.c | 11 ++++--- >> drivers/net/ipa/ipa_table.h | 6 ++-- >> 11 files changed, 115 insertions(+), 46 deletions(-) >> create mode 100644 drivers/net/ipa/ipa_assert.h >> >