| Message ID | 20230613021643.3330661-1-gongruiqi@huaweicloud.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pinctrl: renesas: remove checker warnings: x | !y | expand |
Hi Gong, On Tue, Jun 13, 2023 at 4:13 AM GONG, Ruiqi <gongruiqi@huaweicloud.com> wrote: > Eliminate the following Sparse reports when building with C=1: > > drivers/pinctrl/renesas/pinctrl-rzn1.c:187:52: warning: dubious: x | !y > drivers/pinctrl/renesas/pinctrl-rzn1.c:193:52: warning: dubious: x | !y > > Signed-off-by: GONG, Ruiqi <gongruiqi@huaweicloud.com> Thanks for your patch! Looks like sparse needs to be taught the "|" is not used in a boolean context here? > --- a/drivers/pinctrl/renesas/pinctrl-rzn1.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c > @@ -184,13 +184,15 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value) > * address | 1. > */ > if (lock & LOCK_LEVEL1) { > - u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1); > + u32 val = ipctl->lev1_protect_phys | > + (value & LOCK_LEVEL1 ? 0 : 1); > > writel(val, &ipctl->lev1->status_protect); > } > > if (lock & LOCK_LEVEL2) { > - u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2); > + u32 val = ipctl->lev2_protect_phys | > + (value & LOCK_LEVEL2 ? 0 : 1); > > writel(val, &ipctl->lev2->status_protect); > } Gr{oetje,eeting}s, Geert
On Tue, Jun 13, 2023 at 09:38:20AM +0200, Geert Uytterhoeven wrote: > Hi Gong, > > On Tue, Jun 13, 2023 at 4:13 AM GONG, Ruiqi <gongruiqi@huaweicloud.com> wrote: > > Eliminate the following Sparse reports when building with C=1: > > > > drivers/pinctrl/renesas/pinctrl-rzn1.c:187:52: warning: dubious: x | !y > > drivers/pinctrl/renesas/pinctrl-rzn1.c:193:52: warning: dubious: x | !y > > > > Signed-off-by: GONG, Ruiqi <gongruiqi@huaweicloud.com> > > Thanks for your patch! > > Looks like sparse needs to be taught the "|" is not used in a boolean > context here? > I've spent some time exploring how these bugs look like but it was years ago so I have forgotten the details. I think the main issue is when the ! is on the left. Bug: if (!x & 0xf) { Fixed: if (!(x & 0xf)) { Or less commonly: Bug: if (!x > y) { Fixed: if (x <= y) { Originally Sparse used to only warn about !x & y... I feel like Josh maybe got a bit over enthusiastic in changing it to warn about everything. But that was in 2008 and we're only noticing now so maybe it's fine. The other bug that we see is mixing up logical and bitwise negation but those bugs are harder to separate from good code. > > --- a/drivers/pinctrl/renesas/pinctrl-rzn1.c > > +++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c > > @@ -184,13 +184,15 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value) > > * address | 1. > > */ > > if (lock & LOCK_LEVEL1) { > > - u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1); > > + u32 val = ipctl->lev1_protect_phys | > > + (value & LOCK_LEVEL1 ? 0 : 1); To me this code is more confusing than the original code because I struggle to remember if & has higher precedence that ?: (It does. The code is fine). But Cppcheck also thinks the code is confusing and will print a warning: style: Clarify calculation precedence for '&' and '?'. [clarifyCalculation] You would think adding another set of parentheses would silence the sparse warning but it doesn't: u32 val = ipctl->lev1_protect_phys | (!(value & LOCK_LEVEL1)); regards, dan carpenter
Hi Geert, On 2023/06/13 15:38, Geert Uytterhoeven wrote: > Hi Gong, > > On Tue, Jun 13, 2023 at 4:13 AM GONG, Ruiqi <gongruiqi@huaweicloud.com> wrote: >> Eliminate the following Sparse reports when building with C=1: >> >> drivers/pinctrl/renesas/pinctrl-rzn1.c:187:52: warning: dubious: x | !y >> drivers/pinctrl/renesas/pinctrl-rzn1.c:193:52: warning: dubious: x | !y >> >> Signed-off-by: GONG, Ruiqi <gongruiqi@huaweicloud.com> > > Thanks for your patch! > > Looks like sparse needs to be taught the "|" is not used in a boolean > context here? Okay after reading the source code of Sparse I think what this kind of warnings actually means is to hint us a possible misuse of "|" instead of "||" (i.e. misusing a binary operator in a conditional context). Here the code is doing binary operation (i.e. to flip a bit or two), so in this sense the warnings should be just false alarms. However, the original code is a bit weird for me because of the sudden appearance of a boolean operator (i.e. "!") in the middle of a binary calculation. And I think it looks better after this change, since it makes the expression look more "binary". So maybe we can still consider apply this change ;) Greetings, Ruiqi > >> --- a/drivers/pinctrl/renesas/pinctrl-rzn1.c >> +++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c >> @@ -184,13 +184,15 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value) >> * address | 1. >> */ >> if (lock & LOCK_LEVEL1) { >> - u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1); >> + u32 val = ipctl->lev1_protect_phys | >> + (value & LOCK_LEVEL1 ? 0 : 1); >> >> writel(val, &ipctl->lev1->status_protect); >> } >> >> if (lock & LOCK_LEVEL2) { >> - u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2); >> + u32 val = ipctl->lev2_protect_phys | >> + (value & LOCK_LEVEL2 ? 0 : 1); >> >> writel(val, &ipctl->lev2->status_protect); >> } > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/pinctrl/renesas/pinctrl-rzn1.c b/drivers/pinctrl/renesas/pinctrl-rzn1.c index 374b9f281324..2391a316d5c5 100644 --- a/drivers/pinctrl/renesas/pinctrl-rzn1.c +++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c @@ -184,13 +184,15 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value) * address | 1. */ if (lock & LOCK_LEVEL1) { - u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1); + u32 val = ipctl->lev1_protect_phys | + (value & LOCK_LEVEL1 ? 0 : 1); writel(val, &ipctl->lev1->status_protect); } if (lock & LOCK_LEVEL2) { - u32 val = ipctl->lev2_protect_phys | !(value & LOCK_LEVEL2); + u32 val = ipctl->lev2_protect_phys | + (value & LOCK_LEVEL2 ? 0 : 1); writel(val, &ipctl->lev2->status_protect); }
Eliminate the following Sparse reports when building with C=1: drivers/pinctrl/renesas/pinctrl-rzn1.c:187:52: warning: dubious: x | !y drivers/pinctrl/renesas/pinctrl-rzn1.c:193:52: warning: dubious: x | !y Signed-off-by: GONG, Ruiqi <gongruiqi@huaweicloud.com> --- drivers/pinctrl/renesas/pinctrl-rzn1.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)