Re: [PATCH 05/16] iio: adc: Use iio_push_to_buffers_with_ts() to provide length for runtime checks.

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 10 Mar 2025 16:49:18 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 3/9/25 1:20 PM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > This new function allows us to perform debug checks in the helper to ensure
> > that the overrun does not occur.  Use it in all the simple cases where
> > either a static buffer or a structure is used in the drivers.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---  
> 
> ...
> 
> > diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> > index 9a020680885d..77c5dd414d36 100644
> > --- a/drivers/iio/adc/ad4030.c
> > +++ b/drivers/iio/adc/ad4030.c
> > @@ -706,8 +706,9 @@ static irqreturn_t ad4030_trigger_handler(int irq, void *p)
> >  	if (ret)
> >  		goto out;
> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_data.raw,
> > -					   pf->timestamp);
> > +	iio_push_to_buffers_with_ts(indio_dev, st->rx_data.raw,  
> 
> Would &st->rx_data be more correct? I guess it doesn't really matter in this case.
Feels slightly more logical to use rx_data so changed.

> 
> > +				    sizeof(st->rx_data.raw),
> > +				    pf->timestamp);
> >  
> >  out:
> >  	iio_trigger_notify_done(indio_dev->trig);  
> 
> ...
> 
> > diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> > index 18559757f908..4217d3963455 100644
> > --- a/drivers/iio/adc/ad7266.c
> > +++ b/drivers/iio/adc/ad7266.c
> > @@ -87,8 +87,9 @@ static irqreturn_t ad7266_trigger_handler(int irq, void *p)
> >  
> >  	ret = spi_read(st->spi, st->data.sample, 4);
> >  	if (ret == 0) {
> > -		iio_push_to_buffers_with_timestamp(indio_dev, &st->data,
> > -			    pf->timestamp);
> > +		iio_push_to_buffers_with_ts(indio_dev, &st->data,
> > +					    sizeof(st->data),  
> 
> This one has `s64 timestamp;` which should be aligned_s64.
Ah. That's an old bug!  Guess no one rand this on x86_32 :)

I'll add a precursor fix.
Note I didn't include some other drivers in this series because they are
also broken, but I missed this one. Good spot.
> 
> > +					    pf->timestamp);
> >  	}
> >  
> >  	iio_trigger_notify_done(indio_dev->trig);
> > diff --git a/drivers/iio/adc/ad7298.c b/drivers/iio/adc/ad7298.c
> > index 28b88092b4aa..def5f91dc343 100644
> > --- a/drivers/iio/adc/ad7298.c
> > +++ b/drivers/iio/adc/ad7298.c
> > @@ -155,8 +155,9 @@ static irqreturn_t ad7298_trigger_handler(int irq, void *p)
> >  	if (b_sent)
> >  		goto done;
> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
> > -		iio_get_time_ns(indio_dev));
> > +	iio_push_to_buffers_with_ts(indio_dev, st->rx_buf,  
> 
> Does this one actually have enough room in rx_buf for 9 16-bit channels and a
> an aligned timestamp?

Only 8 channels have scan_index != -1 so this is fine I think.
-1 is magic for don't allow this one to be used in buffered mode.

> 
> __be16	rx_buf[12] __aligned(IIO_DMA_MINALIGN);
> 
> 
> > +				    sizeof(st->rx_buf),
> > +				    iio_get_time_ns(indio_dev));
> >  
> >  done:
> >  	iio_trigger_notify_done(indio_dev->trig);  
> 
> ...
> 
> > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > index 631e83717167..3ffa11b2e060 100644
> > --- a/drivers/iio/adc/ad7606.c
> > +++ b/drivers/iio/adc/ad7606.c
> > @@ -679,8 +679,8 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
> >  	if (ret)
> >  		goto error_ret;
> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, &st->data,
> > -					   iio_get_time_ns(indio_dev));
> > +	iio_push_to_buffers_with_ts(indio_dev, &st->data, sizeof(st->data),  
> 
> This one might need ALIGNED in the buffer declarations. I'm working on this
> driver though, so can fix it up.
Corner case.  It's actually fine because IIO_DMA_MINALIGN is >= 8 but
that's not obvious and may not remain true for all future architectures so
indeed good to tidy that up at somepoint with the additional markings.

> 
> > +				    iio_get_time_ns(indio_dev));
> >  error_ret:
> >  	iio_trigger_notify_done(indio_dev->trig);
> >  	/* The rising edge of the CONVST signal starts a new conversion. */
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index 5a863005aca6..4eb586d473ce 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -474,8 +474,9 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
> >  	if (ret < 0)
> >  		goto out;
> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, &st->data.scan,
> > -					   iio_get_time_ns(indio_dev));
> > +	iio_push_to_buffers_with_ts(indio_dev, &st->data.scan,
> > +				    sizeof(st->data.scan),  
> 
> This one could use aligned_s64 on the timestamp.
Definitely. Another bug I'll put a separate fix in for this.

> 
> > +				    iio_get_time_ns(indio_dev));
> >  
> >  out:
> >  	iio_trigger_notify_done(indio_dev->trig);  
> 
> ...
> 
> > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> > index 87945efb940b..0369151c7db1 100644
> > --- a/drivers/iio/adc/ad7923.c
> > +++ b/drivers/iio/adc/ad7923.c
> > @@ -207,8 +207,8 @@ static irqreturn_t ad7923_trigger_handler(int irq, void *p)
> >  	if (b_sent)
> >  		goto done;
> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
> > -					   iio_get_time_ns(indio_dev));
> > +	iio_push_to_buffers_with_ts(indio_dev, st->rx_buf, sizeof(st->rx_buf),  
> 
> This one is similar to ad7298 but doesn't have a temperature channel, so
> probably OK. Still could use ALIGNED in the buffer declaration to make it more
> obvious that we have a timestamp.

Also relies on IIO_DMA_MINALIGN >= 8
It's a bit messy to force alignment twice so I'll leave this for now.

> 
> > +				    iio_get_time_ns(indio_dev));
> >  
> >  done:
> >  	iio_trigger_notify_done(indio_dev->trig);
> > diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-adc.c
> > index a1e48a756a7b..5ffb4b5f5c99 100644
> > --- a/drivers/iio/adc/dln2-adc.c
> > +++ b/drivers/iio/adc/dln2-adc.c
> > @@ -488,8 +488,8 @@ static irqreturn_t dln2_adc_trigger_h(int irq, void *p)
> >  		       (void *)dev_data.values + t->from, t->length);
> >  	}
> >  
> > -	iio_push_to_buffers_with_timestamp(indio_dev, &data,
> > -					   iio_get_time_ns(indio_dev));
> > +	iio_push_to_buffers_with_ts(indio_dev, &data, sizeof(data),  
> 
> Could use aligned_s64 on timestamp_space.

This one is more subtle as the structure is big enough but the overall
structure might be misaligned. I'll add a patch fixing it but not sure
it is suitable for a backport.

> 
> > +				    iio_get_time_ns(indio_dev));
> >  
> >  done:
> >  	iio_trigger_notify_done(indio_dev->trig);  
> 
> ...
> 
> > diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
> > index 8f1e6acea53b..92baf3f5f560 100644
> > --- a/drivers/iio/adc/mxs-lradc-adc.c
> > +++ b/drivers/iio/adc/mxs-lradc-adc.c
> > @@ -425,7 +425,8 @@ static irqreturn_t mxs_lradc_adc_trigger_handler(int irq, void *p)
> >  		j++;
> >  	}
> >  
> > -	iio_push_to_buffers_with_timestamp(iio, adc->buffer, pf->timestamp);
> > +	iio_push_to_buffers_with_ts(iio, adc->buffer, sizeof(adc->buffer),  
> 
> u32			buffer[10] __aligned(8);
> 
> Technically OK, but could use ALIGN. There are some other drivers like this
> but I won't call all of them out since they should be OK.

You mean to calculate the number of entrees in the array?

Jonathan

> 
> > +				    pf->timestamp);
> >  
> >  	iio_trigger_notify_done(iio->trig);
> >    
> 
> ...
> 
> > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> > index 5dbf5f136768..32e7938b3892 100644
> > --- a/drivers/iio/adc/stm32-adc.c
> > +++ b/drivers/iio/adc/stm32-adc.c
> > @@ -1858,8 +1858,8 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
> >  
> >  	/* reset buffer index */
> >  	adc->bufi = 0;
> > -	iio_push_to_buffers_with_timestamp(indio_dev, adc->buffer,
> > -					   pf->timestamp);
> > +	iio_push_to_buffers_with_ts(indio_dev, adc->buffer, sizeof(adc->buffer),  
> 
> u16			buffer[STM32_ADC_MAX_SQ + 4] __aligned(8);
> 
> Could use ALIGN for timestamp instead of assuming STM32_ADC_MAX_SQ * 2 is the
> correct alignment. There are also a few more drivers like this that depend on
> a macro being even or multiple of 4 to get the correct alignment, which seems
> like it could be fragile, e.g. if a temperature channel was added.
> 
> > +				    pf->timestamp);
> >  	iio_trigger_notify_done(indio_dev->trig);
> >  
> >  	/* re-enable eoc irq */  
> 
> ...
> 
>