[bug report] platform/x86: ISST: Add IOCTL default callback

[bug report] platform/x86: ISST: Add IOCTL default callback

[Dan Carpenter]

Hello Srinivas Pandruvada,

The patch 33c16dc1a2d1: "platform/x86: ISST: Add IOCTL default
callback" from Feb 10, 2023, leads to the following Smatch static
checker warning:

	drivers/platform/x86/intel/speed_select_if/isst_if_common.c:629 isst_if_def_ioctl()
	info: return a literal instead of 'ret'

drivers/platform/x86/intel/speed_select_if/isst_if_common.c
    615         case ISST_IF_MSR_COMMAND:
    616                 cmd_cb.cmd_size = sizeof(struct isst_if_msr_cmd);
    617                 cmd_cb.offset = offsetof(struct isst_if_msr_cmds, msr_cmd);
    618                 cmd_cb.cmd_callback = isst_if_msr_cmd_req;
    619                 ret = isst_if_exec_multi_cmd(argp, &cmd_cb);
    620                 break;
    621         default:
    622                 for (i = 0; i < ISST_IF_DEV_MAX; ++i) {
    623                         struct isst_if_cmd_cb *cb = &punit_callbacks[i];
    624                         int ret;
    625 
    626                         if (cb->def_ioctl) {
    627                                 ret = cb->def_ioctl(file, cmd, arg);
    628                                 if (!ret)
--> 629                                         return ret;

This returns the first time something succeeds.  Normally it would be
the other way around, where we return the first time something fails.
If this is really intentional it would be better to do an explicit
"return 0;"

    630                         }
    631                 }
    632                 break;
    633         }
    634 
    635         return ret;
    636 }

regards,
dan carpenter

Re: [bug report] platform/x86: ISST: Add IOCTL default callback

[srinivas pandruvada]

Hi Dan,

On Fri, 2023-03-10 at 14:57 +0300, Dan Carpenter wrote:
> Hello Srinivas Pandruvada,
> 
> The patch 33c16dc1a2d1: "platform/x86: ISST: Add IOCTL default
> callback" from Feb 10, 2023, leads to the following Smatch static
> checker warning:
> 
>         drivers/platform/x86/intel/speed_select_if/isst_if_common.c:6
> 29 isst_if_def_ioctl()
>         info: return a literal instead of 'ret'
> 
I use your blog
https://blogs.oracle.com/linux/post/smatch-static-analysis-tool-overview-by-dan-carpenter

smatch/smatch_scripts/kchecker --spammy
drivers/platform/x86/intel/speed_select_if/isst_if_common.c
  CHECK   scripts/mod/empty.c
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/platform/x86/intel/speed_select_if/isst_if_common.o
  CHECK   drivers/platform/x86/intel/speed_select_if/isst_if_common.c

Also tried with 
https://smatch.sourceforge.net/

make CHECK="~/path/to/smatch/smatch -p=kernel" C=1 \
                bzImage module

What is the correct way to run this to get this error?

Thanks,
Srinivas

> drivers/platform/x86/intel/speed_select_if/isst_if_common.c
>     615         case ISST_IF_MSR_COMMAND:
>     616                 cmd_cb.cmd_size = sizeof(struct
> isst_if_msr_cmd);
>     617                 cmd_cb.offset = offsetof(struct
> isst_if_msr_cmds, msr_cmd);
>     618                 cmd_cb.cmd_callback = isst_if_msr_cmd_req;
>     619                 ret = isst_if_exec_multi_cmd(argp, &cmd_cb);
>     620                 break;
>     621         default:
>     622                 for (i = 0; i < ISST_IF_DEV_MAX; ++i) {
>     623                         struct isst_if_cmd_cb *cb =
> &punit_callbacks[i];
>     624                         int ret;
>     625 
>     626                         if (cb->def_ioctl) {
>     627                                 ret = cb->def_ioctl(file,
> cmd, arg);
>     628                                 if (!ret)
> --> 629                                         return ret;
> 
> This returns the first time something succeeds.  Normally it would be
> the other way around, where we return the first time something fails.
> If this is really intentional it would be better to do an explicit
> "return 0;"
> 
>     630                         }
>     631                 }
>     632                 break;
>     633         }
>     634 
>     635         return ret;
>     636 }
> 
> regards,
> dan carpenter

Re: [bug report] platform/x86: ISST: Add IOCTL default callback

[Dan Carpenter]

On Thu, Mar 16, 2023 at 12:02:04PM -0700, srinivas pandruvada wrote:
> Hi Dan,
> 
> On Fri, 2023-03-10 at 14:57 +0300, Dan Carpenter wrote:
> > Hello Srinivas Pandruvada,
> > 
> > The patch 33c16dc1a2d1: "platform/x86: ISST: Add IOCTL default
> > callback" from Feb 10, 2023, leads to the following Smatch static
> > checker warning:
> > 
> >         drivers/platform/x86/intel/speed_select_if/isst_if_common.c:6
> > 29 isst_if_def_ioctl()
> >         info: return a literal instead of 'ret'
> > 
> I use your blog
> https://blogs.oracle.com/linux/post/smatch-static-analysis-tool-overview-by-dan-carpenter
> 
> smatch/smatch_scripts/kchecker --spammy
> drivers/platform/x86/intel/speed_select_if/isst_if_common.c
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   DESCEND objtool
>   INSTALL libsubcmd_headers
>   CC [M]  drivers/platform/x86/intel/speed_select_if/isst_if_common.o
>   CHECK   drivers/platform/x86/intel/speed_select_if/isst_if_common.c
> 
> Also tried with 
> https://smatch.sourceforge.net/
> 
> make CHECK="~/path/to/smatch/smatch -p=kernel" C=1 \
>                 bzImage module
> 
> What is the correct way to run this to get this error?
> 

Sorry, I haven't published this check, yet.  I'm still working through
some bugs in it.  You would think that a check like this would be
really simple, but Smatch warns about stuff like:

	ret = sunxi_nfc_hw_ecc_read_chunks_dma(nand, buf, oob_required, page,
					       nand->ecc.steps);
	if (ret >= 0)
		return ret;

For some reason in my system Smatch thinks
sunxi_nfc_hw_ecc_read_chunks_dma() only returns zero.  Which is bogus
and I need to fix it before I can publish the check.

(That might be a bug from something I did yesterday.  My build is very
messed up right now.  But there are other weirdnesses...)

regards,
dan carpenter