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

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

[GONG, Ruiqi]

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(-)

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);
 	}
-- 
2.25.1

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

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

[GONG, Ruiqi]

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
>