* [bug report] rtc: Introduce ti-k3-rtc
@ 2022-06-28 6:56 Dan Carpenter
2022-07-01 1:48 ` Nishanth Menon
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-06-28 6:56 UTC (permalink / raw)
To: nm; +Cc: linux-rtc
Hello Nishanth Menon,
The patch b09d633575e5: "rtc: Introduce ti-k3-rtc" from Jun 23, 2022,
leads to the following Smatch static checker warning:
drivers/rtc/rtc-ti-k3.c:186 k3rtc_unlock_rtc()
info: return a literal instead of 'ret'
drivers/rtc/rtc-ti-k3.c
180 static int k3rtc_unlock_rtc(struct ti_k3_rtc *priv)
181 {
182 int ret;
183
184 ret = k3rtc_check_unlocked(priv);
185 if (!ret)
--> 186 return ret;
It look more intentional when code uses literals:
if (!ret)
return 0;
The k3rtc_check_unlocked() function can also return error codes so maybe
this should be:
if (ret <= 0)
return 0;
187
188 k3rtc_field_write(priv, K3RTC_KICK0, K3RTC_KICK0_UNLOCK_VALUE);
189 k3rtc_field_write(priv, K3RTC_KICK1, K3RTC_KICK1_UNLOCK_VALUE);
190
191 /* Skip fence since we are going to check the unlock bit as fence */
192 ret = regmap_field_read_poll_timeout(priv->r_fields[K3RTC_UNLOCK], ret,
193 !ret, 2, priv->sync_timeout_us);
194
195 return ret;
196 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] rtc: Introduce ti-k3-rtc
2022-06-28 6:56 [bug report] rtc: Introduce ti-k3-rtc Dan Carpenter
@ 2022-07-01 1:48 ` Nishanth Menon
2022-07-01 4:43 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Nishanth Menon @ 2022-07-01 1:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-rtc
Hi Dan,
On 09:56-20220628, Dan Carpenter wrote:
> Hello Nishanth Menon,
>
> The patch b09d633575e5: "rtc: Introduce ti-k3-rtc" from Jun 23, 2022,
> leads to the following Smatch static checker warning:
>
> drivers/rtc/rtc-ti-k3.c:186 k3rtc_unlock_rtc()
> info: return a literal instead of 'ret'
>
> drivers/rtc/rtc-ti-k3.c
> 180 static int k3rtc_unlock_rtc(struct ti_k3_rtc *priv)
> 181 {
> 182 int ret;
> 183
> 184 ret = k3rtc_check_unlocked(priv);
> 185 if (!ret)
> --> 186 return ret;
>
> It look more intentional when code uses literals:
>
> if (!ret)
> return 0;
>
> The k3rtc_check_unlocked() function can also return error codes so maybe
> this should be:
>
> if (ret <= 0)
> return 0;
Interesting for a couple of reasons:
a) yep - if ret was < 0, driver does'nt anticipate that from regmap
op(unless something really bad happened), so it warn_once
the information and does continue as if nothing happened.. So, yep, I
could potentially do <=0 and document the rationale - might be better
to return ret instead of 0 though. I can send a patch if you agree.
b) How do i reproduce the warning that you reported?
I have been trying to reproduce the check (so that I don't get more
reports) and I am probably missing something obvious (using
https://repo.or.cz/smatch.git and hints from
Documentation/driver-api/media/maintainer-entry-profile.rst):
$ git describe
next-20220630
$ rm drivers/rtc/rtc-ti-k3.o; make CROSS_COMPILE=aarch64-none-linux-gnu- ARCH=arm64 -j1 CHECK=/tmp/x C=2 drivers/rtc/rtc-ti-k3.o
CHECK scripts/mod/empty.c
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
CHECK arch/arm64/kernel/vdso/vgettimeofday.c
CC drivers/rtc/rtc-ti-k3.o
CHECK drivers/rtc/rtc-ti-k3.c
$ cat /tmp/x
#!/bin/bash
/tmp/sm/bin/smatch -p=kernel $@
$ /tmp/sm/bin/smatch --version
v0.5.0-8047-g2aeb55346b81
$ cd /tmp/smatch
/tmp/smatch$ git describe
v0.5.0-8047-g2aeb55346b81
$ CROSS_COMPILE=aarch64-none-linux-gnu- ARCH=arm64 /tmp/smatch/smatch_scripts/kchecker drivers/rtc/rtc-ti-k3.c
SYNC include/generated/autoconf.h
CHECK scripts/mod/empty.c
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
CHECK arch/arm64/kernel/vdso/vgettimeofday.c
CC drivers/rtc/rtc-ti-k3.o
CHECK drivers/rtc/rtc-ti-k3.c
$ aarch64-none-linux-gnu-gcc --version
aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 10.3-2021.07 (arm-10.29)) 10.3.1 20210621
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] rtc: Introduce ti-k3-rtc
2022-07-01 1:48 ` Nishanth Menon
@ 2022-07-01 4:43 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2022-07-01 4:43 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-rtc
On Thu, Jun 30, 2022 at 08:48:04PM -0500, Nishanth Menon wrote:
> Hi Dan,
> On 09:56-20220628, Dan Carpenter wrote:
> > Hello Nishanth Menon,
> >
> > The patch b09d633575e5: "rtc: Introduce ti-k3-rtc" from Jun 23, 2022,
> > leads to the following Smatch static checker warning:
> >
> > drivers/rtc/rtc-ti-k3.c:186 k3rtc_unlock_rtc()
> > info: return a literal instead of 'ret'
> >
> > drivers/rtc/rtc-ti-k3.c
> > 180 static int k3rtc_unlock_rtc(struct ti_k3_rtc *priv)
> > 181 {
> > 182 int ret;
> > 183
> > 184 ret = k3rtc_check_unlocked(priv);
> > 185 if (!ret)
> > --> 186 return ret;
> >
> > It look more intentional when code uses literals:
> >
> > if (!ret)
> > return 0;
> >
> > The k3rtc_check_unlocked() function can also return error codes so maybe
> > this should be:
> >
> > if (ret <= 0)
> > return 0;
>
> Interesting for a couple of reasons:
> a) yep - if ret was < 0, driver does'nt anticipate that from regmap
> op(unless something really bad happened), so it warn_once
> the information and does continue as if nothing happened.. So, yep, I
> could potentially do <=0 and document the rationale - might be better
> to return ret instead of 0 though. I can send a patch if you agree.
> b) How do i reproduce the warning that you reported?
Oh sorry. I never published this warning. :/
I generally don't like style checks because life is too short for style
debates. But the real reason for this check is to find inverted
if (ret) checks. So maybe I should publish it. Inverted checks aren't
super common.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-01 4:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-28 6:56 [bug report] rtc: Introduce ti-k3-rtc Dan Carpenter
2022-07-01 1:48 ` Nishanth Menon
2022-07-01 4:43 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.