| Message ID | 20220322111619.54808-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] iio: core: Print error in case sample bits do not fit storage bits | expand |
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` > --- > 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.realbits, > + channels[i].scan_type.shift); > + continue; > + } > + > ret = iio_buffer_add_channel_sysfs(indio_dev, buffer, > &channels[i]); > if (ret < 0)
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... - 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) >
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) > > >
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.realbits, + channels[i].scan_type.shift); + continue; + } + ret = iio_buffer_add_channel_sysfs(indio_dev, buffer, &channels[i]); if (ret < 0)
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> --- 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(+)