Re: [PATCH v10 10/11] iio: adc: hx711: pass iio_chan_spec to hx711_reset_read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `iio_device_claim_direct_mode()` allows concurrent sysfs reads to disrupt triggered buffer continuous sampling.
- [High] Out-of-bounds Array Write in `hx711_trigger()` when the timestamp channel is enabled.
--

commit b8324538619dcb536bed461da5b5bd5e89ca409b
Author: Piyush Patle <piyushpatle228@gmail.com>

iio: adc: hx711: pass iio_chan_spec to hx711_reset_read

Change hx711_reset_read() to accept the channel descriptor directly and
update its callers accordingly. Split the existing HX711 channel-selection
work into a small helper so a later variant-specific change can add a
matching helper without growing hx711_reset_read() further.

> diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
> --- a/drivers/iio/adc/hx711.c
> +++ b/drivers/iio/adc/hx711.c
[ ... ]
> @@ -300,7 +315,7 @@ static int hx711_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		mutex_lock(&hx711_data->lock);
>  
> -		*val = hx711_reset_read(hx711_data, chan->channel);
> +		*val = hx711_reset_read(hx711_data, chan);
>  
>  		mutex_unlock(&hx711_data->lock);
>  

[Severity: High]
This is a pre-existing issue, but does this function need to call
iio_device_claim_direct_mode() before accessing the hardware?

If a triggered buffer is active, couldn't a concurrent sysfs read to
in_voltage0_raw take the lock, wait for the device to be ready, and perform
a hardware read?

This might steal a sample from the continuous buffer stream, introduce
massive timing jitter due to gain changes, and disrupt the functional
behavior of the triggered buffer.

[ ... ]
> @@ -390,8 +405,8 @@ static irqreturn_t hx711_trigger(int irq, void *p)
>  	memset(&hx711_data->buffer, 0, sizeof(hx711_data->buffer));
>  
>  	iio_for_each_active_channel(indio_dev, i) {
> -		hx711_data->buffer.channel[j] = hx711_reset_read(hx711_data,
> -					indio_dev->channels[i].channel);
> +		hx711_data->buffer.channel[j] =
> +			hx711_reset_read(hx711_data, &indio_dev->channels[i]);
>  		j++;
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but does this loop correctly
handle the timestamp channel?

Since iio_for_each_active_channel() iterates over all bits set in the
active_scan_mask, wouldn't it include the soft timestamp channel if enabled
by userspace?

If the loop doesn't explicitly skip the timestamp channel, it appears it
would treat it as an ADC channel and call hx711_reset_read() on it, wasting
time on a hardware conversion.

Furthermore, when j reaches 2, writing to channel[j] would be an
out-of-bounds array access on the 2-element array, potentially triggering a
kernel panic when compiled with UBSAN bounds checking.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525105127.180156-1-piyushpatle228@gmail.com?part=10