[v1] iio: adc: ad7124: Implement input validation

[PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

Posted by Uwe Kleine-König 2 weeks, 5 days ago

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;
+	}
+
 	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
 			    sizeof(*chan), GFP_KERNEL);
 	if (!chan)
-- 
2.45.2

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

Posted by Nuno Sá 2 weeks, 2 days ago

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á

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

Posted by Uwe Kleine-König 2 weeks, 2 days ago

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

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

Posted by Nuno Sá 2 weeks, 2 days ago

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á

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

Posted by David Lechner 2 weeks, 5 days ago

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.

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

Posted by Uwe Kleine-König 2 weeks, 2 days ago

[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

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

Posted by David Lechner 2 weeks, 2 days ago

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.

Re: [PATCH 1/2] iio: adc: ad7124: Don't create more channels than the hardware is capable of

Posted by Jonathan Cameron 4 days, 4 hours ago

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