| Message ID | 20241108181813.272593-5-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7124: Implement input validation | expand |
On 11/8/24 12:18 PM, Uwe Kleine-König wrote: > The ad7124-4 and ad7124-8 both support 16 channel registers. Don't > accept more (logical) channels from dt than that. Why should the devicetree be limited by the number of channel registers? Channel registers are a resource than can be dynamically assigned, so it doesn't seem like the devicetree should be specifying that assignment. It's true we can't do a buffered read of more than 8 or 16 channels at the same time because it is limited by the number of channel registers available on the chip. But it seems reasonable that if there are more logical channels than that, we could read 8 logical channels, then disable those and enable a different 8 logical channels and read those by reconfiguring the channel registers.
On Fri, 2024-11-08 at 19:18 +0100, Uwe Kleine-König wrote: > The ad7124-4 and ad7124-8 both support 16 channel registers. Don't > accept more (logical) channels from dt than that. > > Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > --- > drivers/iio/adc/ad7124.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c > index a5d91933f505..7473bcef7bc6 100644 > --- a/drivers/iio/adc/ad7124.c > +++ b/drivers/iio/adc/ad7124.c > @@ -18,6 +18,7 @@ > #include <linux/property.h> > #include <linux/regulator/consumer.h> > #include <linux/spi/spi.h> > +#include <linux/stringify.h> > > #include <linux/iio/iio.h> > #include <linux/iio/adc/ad_sigma_delta.h> > @@ -821,6 +822,11 @@ static int ad7124_parse_channel_config(struct iio_dev > *indio_dev, > if (!st->num_channels) > return dev_err_probe(dev, -ENODEV, "no channel children\n"); > > + if (st->num_channels > AD7124_MAX_CHANNELS) { > + dev_warn(dev, "Limit number of channels to " > __stringify(AD7124_MAX_CHANNELS) "\n"); > + st->num_channels = AD7124_MAX_CHANNELS; > + } Hmmm, I would treat it as an error... - Nuno Sá
Hello Nuno, On Mon, Nov 11, 2024 at 11:37:46AM +0100, Nuno Sá wrote: > On Fri, 2024-11-08 at 19:18 +0100, Uwe Kleine-König wrote: > > @@ -821,6 +822,11 @@ static int ad7124_parse_channel_config(struct iio_dev > > *indio_dev, > > if (!st->num_channels) > > return dev_err_probe(dev, -ENODEV, "no channel children\n"); > > > > + if (st->num_channels > AD7124_MAX_CHANNELS) { > > + dev_warn(dev, "Limit number of channels to " > > __stringify(AD7124_MAX_CHANNELS) "\n"); > > + st->num_channels = AD7124_MAX_CHANNELS; > > + } > > Hmmm, I would treat it as an error... Well, it probably results in an error further below when the first child is hit that uses a too high reg property. I considered erroring out here, but thought this might not be justified if some children are not logical channels. I'm not sure either way, but can rework accordingly if all other concerns are resolved. Best regards Uwe
[dropped Mircea Caprioru from Cc: as their address bounces.] Hello David, On Fri, Nov 08, 2024 at 12:52:35PM -0600, David Lechner wrote: > On 11/8/24 12:18 PM, Uwe Kleine-König wrote: > > The ad7124-4 and ad7124-8 both support 16 channel registers. Don't > > accept more (logical) channels from dt than that. > > Why should the devicetree be limited by the number of channel > registers? Channel registers are a resource than can be > dynamically assigned, so it doesn't seem like the devicetree > should be specifying that assignment. Note the device tree isn't limited as I didn't adapt the binding. It's just that the driver doesn't bind if too many channels are specified. And while your statement about the channels being a dynamic resource is right, currently the driver doesn't cope and allocates resources statically, and happily assumes there is a CHANNEL_16 register if the device tree specifies 17 (or more) logical channels and writes to CONFIG_0 then which very likely results in strange effects. So as long as the driver doesn't implement this (possible) dynamic mapping to the CHANNEL registers, it's IMHO right to refuse to bind (or alternatively only use the 16 first logical channels). Best regards Uwe
On 11/11/24 6:08 AM, Uwe Kleine-König wrote: > [dropped Mircea Caprioru from Cc: as their address bounces.] > > Hello David, > > On Fri, Nov 08, 2024 at 12:52:35PM -0600, David Lechner wrote: >> On 11/8/24 12:18 PM, Uwe Kleine-König wrote: >>> The ad7124-4 and ad7124-8 both support 16 channel registers. Don't >>> accept more (logical) channels from dt than that. >> >> Why should the devicetree be limited by the number of channel >> registers? Channel registers are a resource than can be >> dynamically assigned, so it doesn't seem like the devicetree >> should be specifying that assignment. > > Note the device tree isn't limited as I didn't adapt the binding. It's > just that the driver doesn't bind if too many channels are specified. > And while your statement about the channels being a dynamic resource is > right, currently the driver doesn't cope and allocates resources > statically, and happily assumes there is a CHANNEL_16 register if the > device tree specifies 17 (or more) logical channels and writes to > CONFIG_0 then which very likely results in strange effects. > > So as long as the driver doesn't implement this (possible) dynamic > mapping to the CHANNEL registers, it's IMHO right to refuse to bind (or > alternatively only use the 16 first logical channels). > > Best regards > Uwe Understood. It would be nice to implement such dynamic allocation in the future but as a fix to backport to stable kernels, this makes sense.
On Mon, 2024-11-11 at 12:53 +0100, Uwe Kleine-König wrote: > Hello Nuno, > > On Mon, Nov 11, 2024 at 11:37:46AM +0100, Nuno Sá wrote: > > On Fri, 2024-11-08 at 19:18 +0100, Uwe Kleine-König wrote: > > > @@ -821,6 +822,11 @@ static int ad7124_parse_channel_config(struct iio_dev > > > *indio_dev, > > > if (!st->num_channels) > > > return dev_err_probe(dev, -ENODEV, "no channel > > > children\n"); > > > > > > + if (st->num_channels > AD7124_MAX_CHANNELS) { > > > + dev_warn(dev, "Limit number of channels to " > > > __stringify(AD7124_MAX_CHANNELS) "\n"); > > > + st->num_channels = AD7124_MAX_CHANNELS; > > > + } > > > > Hmmm, I would treat it as an error... > > Well, it probably results in an error further below when the first child > is hit that uses a too high reg property. I considered erroring out Assuming no one points two different nodes to the same reg :) > here, but thought this might not be justified if some children are not > logical channels. I'm not sure either way, but can rework accordingly if > all other concerns are resolved. > Anyways, I'm not totally sure about the logical channels you and David are discussing (did not went through it), but FWIW, I agree that we should stop probe if something is not supported rather than proceed just to get non obvious, subtle bugs. - Nuno Sá
On Mon, 11 Nov 2024 08:21:45 -0600 David Lechner <dlechner@baylibre.com> wrote: > On 11/11/24 6:08 AM, Uwe Kleine-König wrote: > > [dropped Mircea Caprioru from Cc: as their address bounces.] > > > > Hello David, > > > > On Fri, Nov 08, 2024 at 12:52:35PM -0600, David Lechner wrote: > >> On 11/8/24 12:18 PM, Uwe Kleine-König wrote: > >>> The ad7124-4 and ad7124-8 both support 16 channel registers. Don't > >>> accept more (logical) channels from dt than that. > >> > >> Why should the devicetree be limited by the number of channel > >> registers? Channel registers are a resource than can be > >> dynamically assigned, so it doesn't seem like the devicetree > >> should be specifying that assignment. > > > > Note the device tree isn't limited as I didn't adapt the binding. It's > > just that the driver doesn't bind if too many channels are specified. > > And while your statement about the channels being a dynamic resource is > > right, currently the driver doesn't cope and allocates resources > > statically, and happily assumes there is a CHANNEL_16 register if the > > device tree specifies 17 (or more) logical channels and writes to > > CONFIG_0 then which very likely results in strange effects. > > > > So as long as the driver doesn't implement this (possible) dynamic > > mapping to the CHANNEL registers, it's IMHO right to refuse to bind (or > > alternatively only use the 16 first logical channels). > > > > Best regards > > Uwe > > Understood. It would be nice to implement such dynamic allocation > in the future but as a fix to backport to stable kernels, this makes > sense. Agreed. We do have other drivers that have internal allocators for constrained sequencer resources, but they are complex so not fix material! Jonathan
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c index a5d91933f505..7473bcef7bc6 100644 --- a/drivers/iio/adc/ad7124.c +++ b/drivers/iio/adc/ad7124.c @@ -18,6 +18,7 @@ #include <linux/property.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> +#include <linux/stringify.h> #include <linux/iio/iio.h> #include <linux/iio/adc/ad_sigma_delta.h> @@ -821,6 +822,11 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev, if (!st->num_channels) return dev_err_probe(dev, -ENODEV, "no channel children\n"); + if (st->num_channels > AD7124_MAX_CHANNELS) { + dev_warn(dev, "Limit number of channels to " __stringify(AD7124_MAX_CHANNELS) "\n"); + st->num_channels = AD7124_MAX_CHANNELS; + } + chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels, sizeof(*chan), GFP_KERNEL); if (!chan)
The ad7124-4 and ad7124-8 both support 16 channel registers. Don't accept more (logical) channels from dt than that. Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad7124.c | 6 ++++++ 1 file changed, 6 insertions(+)