[bug report] rtc: Introduce ti-k3-rtc

[bug report] rtc: Introduce ti-k3-rtc

[Dan Carpenter]

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

Re: [bug report] rtc: Introduce ti-k3-rtc

[Nishanth Menon]

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

Re: [bug report] rtc: Introduce ti-k3-rtc

[Dan Carpenter]

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