iio: accel: sca3000: dead code issue

iio: accel: sca3000: dead code issue

[Colin King (gmail)]

[-- Attachment #1.1.1: Type: text/plain, Size: 1884 bytes --]

Hi,

Static analysis on drivers/iio/accel/sca3000.c on linux-next has 
detected an issue in the function sca3000_ring_int_process. The
issue is described as follows (prefixed by >>>)

static void sca3000_ring_int_process(u8 val, struct iio_dev *indio_dev)
{
         struct sca3000_state *st = iio_priv(indio_dev);
         int ret, i, num_available;

         mutex_lock(&st->lock);

         if (val & SCA3000_REG_INT_STATUS_HALF) {
                 ret = spi_w8r8(st->us, 
SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));


 >>>
 >>>  the call to spi_w8r8 returns 0 on success or -ve on an error
 >>>

                 if (ret)
                         goto error_ret;

 >>>
 >>> ret is always zero, at this point, so num_available is zero too
 >>>

                 num_available = ret;
                 /*
                  * num_available is the total number of samples available
                  * i.e. number of time points * number of channels.
                  */
                 ret = sca3000_read_data(st, SCA3000_REG_RING_OUT_ADDR, 
num_available * 2);
                 if (ret)
                         goto error_ret;

 >>>
 >>>  num_available is zero, so for-loop is never executed.
 >>>

                 for (i = 0; i < num_available / 3; i++) {
                         /*
                          * Dirty hack to cover for 11 bit in fifo, 13 bit
                          * direct reading.
                          *
                          * In theory the bottom two bits are undefined.
                          * In reality they appear to always be 0.
                          */
                         iio_push_to_buffers(indio_dev, st->rx + i * 3 * 2);
                 }
         }


I'm not sure what the logic should be to fix this, so I'm reporting this 
as an issue.

Regards,

Colin

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 4901 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: iio: accel: sca3000: dead code issue

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 10:00:55AM +0100, Colin King (gmail) wrote:

>                 ret = spi_w8r8(st->us,
> SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
> 
> >>>  the call to spi_w8r8 returns 0 on success or -ve on an error

Where did you get this from, please?  Any link to elixir or Git repo?

> 
>                 if (ret)
>                         goto error_ret;

-- 
With Best Regards,
Andy Shevchenko

Re: iio: accel: sca3000: dead code issue

[Jonathan Cameron]

On Wed, 2 Jul 2025 15:11:26 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Jul 02, 2025 at 10:00:55AM +0100, Colin King (gmail) wrote:
> 
> >                 ret = spi_w8r8(st->us,
> > SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
> >   
> > >>>  the call to spi_w8r8 returns 0 on success or -ve on an error  
> 
> Where did you get this from, please?  Any link to elixir or Git repo?
> 

Hmm.  Just for reference the docs of spi_w8r8 are:

* Return: the (unsigned) eight bit number returned by the
* device, or else a negative error code.

Not 0 on success (well not unless it is zero.

So the check indeed looks wrong as should be if (ret < 0)


https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=ca66d8208762492df8442a503db568d9aab65f2f
It's in my tree.

I'll drop the patch when I'm on the right machine.  Andrew, could
you do a new version fixing this up?  If not can make the changes
but will be at least the weekend before I get a chance.

Looks like there are a couple of instances of this.

Jonathan



> > 
> >                 if (ret)
> >                         goto error_ret;  
> 

Re: iio: accel: sca3000: dead code issue

[Andrew Ijano]

On Wed, Jul 2, 2025 at 12:40 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 2 Jul 2025 15:11:26 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>
> > On Wed, Jul 02, 2025 at 10:00:55AM +0100, Colin King (gmail) wrote:
> >
> > >                 ret = spi_w8r8(st->us,
> > > SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
> > >
> > > >>>  the call to spi_w8r8 returns 0 on success or -ve on an error
> >
> > Where did you get this from, please?  Any link to elixir or Git repo?
> >
>
> Hmm.  Just for reference the docs of spi_w8r8 are:
>
> * Return: the (unsigned) eight bit number returned by the
> * device, or else a negative error code.
>
> Not 0 on success (well not unless it is zero.
>
> So the check indeed looks wrong as should be if (ret < 0)
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=ca66d8208762492df8442a503db568d9aab65f2f
> It's in my tree.
>
> I'll drop the patch when I'm on the right machine.  Andrew, could
> you do a new version fixing this up?  If not can make the changes
> but will be at least the weekend before I get a chance.
>

Hey, guys! Thanks for pointing this out, I totally missed this problem
before. I'll try to fix this tomorrow.

In this case, should I send a new version of the patchset fixing the
problem or a single patch following this commit with the fix?

Thanks,
Andrew

Re: iio: accel: sca3000: dead code issue

[Jonathan Cameron]

On Wed, 2 Jul 2025 23:52:02 -0300
Andrew Ijano <andrew.ijano@gmail.com> wrote:

> On Wed, Jul 2, 2025 at 12:40 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Wed, 2 Jul 2025 15:11:26 +0300
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >  
> > > On Wed, Jul 02, 2025 at 10:00:55AM +0100, Colin King (gmail) wrote:
> > >  
> > > >                 ret = spi_w8r8(st->us,
> > > > SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
> > > >  
> > > > >>>  the call to spi_w8r8 returns 0 on success or -ve on an error  
> > >
> > > Where did you get this from, please?  Any link to elixir or Git repo?
> > >  
> >
> > Hmm.  Just for reference the docs of spi_w8r8 are:
> >
> > * Return: the (unsigned) eight bit number returned by the
> > * device, or else a negative error code.
> >
> > Not 0 on success (well not unless it is zero.
> >
> > So the check indeed looks wrong as should be if (ret < 0)
> >
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=ca66d8208762492df8442a503db568d9aab65f2f
> > It's in my tree.
> >
> > I'll drop the patch when I'm on the right machine.  Andrew, could
> > you do a new version fixing this up?  If not can make the changes
> > but will be at least the weekend before I get a chance.
> >  
> 
> Hey, guys! Thanks for pointing this out, I totally missed this problem
> before. I'll try to fix this tomorrow.
> 
> In this case, should I send a new version of the patchset fixing the
> problem or a single patch following this commit with the fix?

Given there will probably be some fuzz for the later patches, send
a new version of the whole series.

Thanks

Jonathan

> 
> Thanks,
> Andrew

Re: iio: accel: sca3000: dead code issue

[Andy Shevchenko]

Wed, Jul 02, 2025 at 04:40:22PM +0100, Jonathan Cameron kirjoitti:
> On Wed, 2 Jul 2025 15:11:26 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Wed, Jul 02, 2025 at 10:00:55AM +0100, Colin King (gmail) wrote:
> > 
> > >                 ret = spi_w8r8(st->us,
> > > SCA3000_READ_REG(SCA3000_REG_BUF_COUNT_ADDR));
> > >   
> > > >>>  the call to spi_w8r8 returns 0 on success or -ve on an error  
> > 
> > Where did you get this from, please?  Any link to elixir or Git repo?
> 
> Hmm.  Just for reference the docs of spi_w8r8 are:
> 
> * Return: the (unsigned) eight bit number returned by the
> * device, or else a negative error code.
> 
> Not 0 on success (well not unless it is zero.

Right. My point was that the comment is misleading. With the adjusted comment
the rest becomes immediately clear.

> So the check indeed looks wrong as should be if (ret < 0)

Exactly. Report is valid, the comment in the analysis is not.

> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=ca66d8208762492df8442a503db568d9aab65f2f
> It's in my tree.

I was asking for the link to the spi_w8r8() where it's written like '0 on
success and -ve on error'. Not needed anymore as you cited the documentation.

> I'll drop the patch when I'm on the right machine.  Andrew, could
> you do a new version fixing this up?  If not can make the changes
> but will be at least the weekend before I get a chance.
> 
> Looks like there are a couple of instances of this.
> 
> > >                 if (ret)
> > >                         goto error_ret;  

-- 
With Best Regards,
Andy Shevchenko