Re: [PATCH v2] iio: core: Print error in case sample bits do not fit storage bits

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 23 Mar 2022 13:05:01 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-03-22 at 21:42 +0000, Jonathan Cameron wrote:
> > On Tue, 22 Mar 2022 12:16:19 +0100
> > Marek Vasut <marex@denx.de> wrote:
> >   
> > > Add runtime check to verify whether storagebits are at least as big
> > > as shifted realbits. This should help spot broken drivers which may
> > > set realbits + shift above storagebits.
> > > 
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > Cc: Andy Shevchenko <andy@kernel.org>
> > > Cc: Daniel Baluta <daniel.baluta@nxp.com>
> > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> > 
> > Hmm. I was thinking we'd fail the probe if this happens,
> > though I guess there might be cases where we get away 
> > (in kernel anyway) with a driver setting this wrong as
> > many drivers don't use realbits internally in an explicit
> > fashion, so maybe a message and skipping the channel is
> > the right choice...
> > 
> > Userspace running against such a description is likely
> > to generate garbage though unless it's very lucky and
> > the spill past storage bits is into padding space and
> > the driver doesn't put anything in there (padding might
> > contain old data or similar).
> > 
> > Either way it's a definite improvement so I'm probably fine
> > with the message and not failing the probe, (though will
> > think a bit more about it before picking this up.)
> > 
> > 
> > Jonathan`
> >   
> 
> FWIW, if we assume we are ok with potentially some drivers starting to
> fail probe, I'm +1 on this should fail probe...

I think we are.  If anyone has messed this up the results are
pretty ugly and so good to know asap and get those drivers fixed.

I'm probably falsely hopeful that no one has messed this up because
it would normally be very easy to spot in a driver assuming there
isn't a bunch of macro magic hiding the actual value assignment.

So unless you feel strongly that it should be a warning, Marek
would you mind spinning a v3 that fails the probe by
returning an error.

Thanks,

Jonathan


> 
> - Nuno Sá
> >   
> > > ---
> > > V2: Use dev_err() instead as WARN_ON() may panic() the kernel on
> > > existing machines
> > > ---
> > >  drivers/iio/industrialio-buffer.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/iio/industrialio-buffer.c
> > > b/drivers/iio/industrialio-buffer.c
> > > index b078eb2f3c9de..b5670398b06d7 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1629,6 +1629,18 @@ static int
> > > __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> > >                         if (channels[i].scan_index < 0)
> > >                                 continue;
> > >  
> > > +                       /* Verify that sample bits fit into storage
> > > */
> > > +                       if (channels[i].scan_type.storagebits <
> > > +                           channels[i].scan_type.realbits +
> > > +                           channels[i].scan_type.shift) {
> > > +                               dev_err(&indio_dev->dev,
> > > +                                       "Channel %d storagebits
> > > (%d) < shifted realbits (%d + %d)\n",
> > > +                                       i,
> > > channels[i].scan_type.storagebits,
> > > +                                       channels[i].scan_type.realb
> > > its,
> > > +                                       channels[i].scan_type.shift
> > > );
> > > +                               continue;
> > > +                       }
> > > +
> > >                         ret =
> > > iio_buffer_add_channel_sysfs(indio_dev, buffer,
> > >                                                         
> > > &channels[i]);
> > >                         if (ret < 0)  
> >   
>