From: Dan Carpenter <dan.carpenter@linaro.org>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: "GONG, Ruiqi" <gongruiqi@huaweicloud.com>,
Linus Walleij <linus.walleij@linaro.org>,
linux-renesas-soc@vger.kernel.org, linux-gpio@vger.kernel.org,
linux-kernel@vger.kernel.org, gongruiqi1@huawei.com,
linux-sparse@vger.kernel.org
Subject: Re: [PATCH] pinctrl: renesas: remove checker warnings: x | !y
Date: Tue, 13 Jun 2023 11:37:00 +0300 [thread overview]
Message-ID: <869dd27f-7046-486a-83a0-acd489565083@kadam.mountain> (raw)
In-Reply-To: <CAMuHMdXGG2xu+nXJt6CSTfV6aM=U=hMW+DiDgP3RhOw8+O8y=A@mail.gmail.com>
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
next prev parent reply other threads:[~2023-06-13 8:37 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20230613021643.3330661-1-gongruiqi@huaweicloud.com>
2023-06-13 7:38 ` [PATCH] pinctrl: renesas: remove checker warnings: x | !y Geert Uytterhoeven
2023-06-13 8:37 ` Dan Carpenter [this message]
2023-06-13 9:45 ` GONG, Ruiqi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=869dd27f-7046-486a-83a0-acd489565083@kadam.mountain \
--to=dan.carpenter@linaro.org \
--cc=geert@linux-m68k.org \
--cc=gongruiqi1@huawei.com \
--cc=gongruiqi@huaweicloud.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux-sparse@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).