On Tue, 2022-03-22 at 21:42 +0000, Jonathan Cameron wrote:
> On Tue, 22 Mar 2022 12:16:19 +0100
> Marek Vasut <marex@xxxxxxx> 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@xxxxxxx>
> > Cc: Andy Shevchenko <andy@xxxxxxxxxx>
> > Cc: Daniel Baluta <daniel.baluta@xxxxxxx>
> > Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> 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...
- 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)
>
