Re: [PATCH v4 3/3] iio: adc: add support for PAC1711

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] iio: integer overflow in scale calculation yields negative scale
- [High] iio: skipped conversion wait during sample rate change causes corrupted reads
- [High] iio: fixed 50ms polling rate limits abort slow hardware conversions
- [Medium] iio: data race exposing un-sign-extended vbus/vsense and shunt to userspace
- [Medium] iio: missing devicetree fallback name assignment exposes (null) to sysfs
- [Medium] iio: TOCTOU race in pac1711_set_acc_enable due to split mutex scopes
- [Low] iio: incompatible pointer type assignment to vals triggers build failure
--

commit 8dfe13196844f805df3c10995b75092fcc0ad04a
Author: Ariana Lazar <ariana.lazar@microchip.com>

iio: adc: add support for PAC1711

This commit adds the IIO driver for Microchip PAC1711, PAC1721, PAC1811 and
PAC1821 single-channel power monitors with accumulator.

> diff --git a/drivers/iio/adc/pac1711.c b/drivers/iio/adc/pac1711.c
> new file mode 100644
> index 0000000000000..c6795d86e3866
> --- /dev/null
> +++ b/drivers/iio/adc/pac1711.c
[ ... ]
> +static int pac1711_reg_snapshot(struct pac1711_chip_info *info, bool do_refresh,
> +				u8 refresh_cmd, u32 wait_time, bool *refreshed)
> +{
> +	guard(mutex)(&info->lock);
> +
> +	/* No hardware refresh is needed */
> +	if (refreshed)
> +		*refreshed = false;
> +
> +	/* Return early if the minimum polling time hasn't elapsed */
> +	if (!time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
> +			msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS)))
> +		return 0;

[Severity: High]
Does this hardcoded 50ms check prematurely abort slow hardware conversions?

If the sample rate is configured to be slower than 50ms per conversion (for
example, 8 samples per second takes 125ms), it seems polling just past 50ms
will continuously trigger new refresh commands and reset the device before
conversions complete.

[ ... ]
> +static int pac1711_set_acc_enable(struct pac1711_chip_info *info, int val)
> +{
[ ... ]
> +	scoped_guard(mutex, &info->lock) {
> +		if (val == info->enable_acc)
> +			return 0;
> +	}
> +
> +	cancel_delayed_work_sync(&info->work_chip_refresh);
> +
> +	scoped_guard(mutex, &info->lock) {

[Severity: Medium]
Could this lock drop around cancel_delayed_work_sync() open a race condition
where concurrent calls modify the device configuration?

It appears the device could be left in the opposite state of the requested
configuration while still reporting success.

[ ... ]
> +static int pac1711_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
[ ... ]
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
> +		if (ret)
> +			return ret;
> +
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = info->chip_reg_data.vbus;
> +			return IIO_VAL_INT;

[Severity: Medium]
Unlike the IIO_POWER case below which uses a scoped_guard, does reading
info->chip_reg_data.vbus without taking info->lock expose readers to an
intermediate un-sign-extended state?

In pac1711_reg_snapshot_locked(), vbus is updated in two steps:

	info->chip_reg_data.vbus = get_unaligned_be16(offset_reg_data_p);
	if (info->vbus_mode != PAC1711_FULL_RANGE_UNIPOLAR)
		info->chip_reg_data.vbus = sign_extend32(info->chip_reg_data.vbus, 15);

[ ... ]
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->address) {
[ ... ]
> +		case PAC1711_VPOWER_REG_ADDR:
> +		case PAC1711_VACC_REG_ADDR:
[ ... ]
> +			/*
> +			 * Power - uW - it will use the combined scale
> +			 * for current and voltage
> +			 * current(mA) * voltage(mV) = power (uW)
> +			 */
> +			tmp = info->features->voltage_pv_fsr;
> +
> +			do_div(tmp, info->shunt);
> +			*val = (int)tmp;

[Severity: High]
Does casting the 64-bit unsigned tmp to a 32-bit signed int overflow if
info->shunt is small?

For a 1 mOhm shunt, tmp could be 4,200,000,000, which would wrap around the
maximum signed 32-bit integer and produce a negative scale factor.

[ ... ]
> +static int pac1711_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
[ ... ]
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		scoped_guard(mutex, &info->lock) {
[ ... ]
> +			info->sample_rate_idx = new_idx;
> +			info->chip_reg_data.ctrl_act_reg = val_u16;
> +
> +			/* Force register snapshot and timestamp update with a refresh. */
> +			refresh_time = msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS) + 1;
> +			info->chip_reg_data.jiffies_tstamp = jiffies - refresh_time;
> +		}
> +
> +		/* We must wait for cycle completion before updating the sample rate value. */
> +		ret = pac1711_retrieve_data(info, ((1024 * 1000) / old_samp_rate));

[Severity: High]
Since info->lock is dropped here, can a concurrent reader intercept the
artificially aged jiffies_tstamp and trigger a hardware refresh?

The original thread's call to pac1711_retrieve_data() could then see a fresh
timestamp, skip the wait entirely, and return prematurely, leading to
corrupted reads.

Additionally, since fsleep() occurs in pac1711_send_refresh() after the
command is sent, the hardware has already snapshotted the state before the
wait happens.

[ ... ]
> +static int pac1711_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *channel,
> +			      const int **vals, int *type, int *length, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*type = IIO_VAL_INT;
> +		*vals = pac1711_samp_rate_map_tbl;

[Severity: Low]
Does this incompatible pointer type assignment trigger a build failure with
the compiler flag to treat warnings as errors?

pac1711_samp_rate_map_tbl is defined as an array of unsigned int, but vals
expects a const int **.

[ ... ]
> +static int pac1711_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ret = pac1711_chip_identify(indio_dev, info);
> +	if (ret == -ENODEV) {
> +		/*
> +		 * If it fails to identify the hardware based on internal
> +		 * registers, use compatible from devicetree.
> +		 */
> +		info->features = i2c_get_match_data(client);
> +		if (!info->features)
> +			return -EINVAL;
> +	} else if (ret) {

[Severity: Medium]
If the devicetree fallback path is taken, it appears indio_dev->name is never
initialized before calling devm_iio_device_register().

Will this expose a (null) sysfs attribute and break userspace tools?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-pac1711-v4-0-6ef34d7a7947@microchip.com?part=3