Re: [PATCH] pinctrl: renesas: remove checker warnings: x | !y

[Dan Carpenter]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> 
> 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