Re: [PATCH v3 2/2] iio: adc: add support for PAC1711

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Sample rate configuration silently fails because `val_be16` is not updated before being written back to the device via I2C.
- [High] Concurrent calls to `pac1711_set_acc_enable()` can permanently cancel the background refresh workqueue due to an unsafe early return.
- [Medium] Torn reads and data races of 64-bit values (`acc_val` and `vpower`) because they are read outside of `info->lock`.
- [Medium] Timestamp manipulation intended to force a hardware refresh fails to guarantee execution if the previous snapshot was taken very recently.
- [Medium] Incorrect wait time calculated using the old sampling rate instead of the newly applied rate.
- [Low] Assigning an `unsigned int` array to a `const int *` pointer triggers a compiler warning.
--

commit 2bd1bfeeb99a3cf2d3d02a346520acaae1f7f3ea
Author: Ariana Lazar <ariana.lazar@microchip.com>
Subject: iio: adc: add support for PAC1711

This adds the IIO driver for Microchip PAC1711, PAC1721, PAC1811, and
PAC1821 single-channel power monitors. It supports voltage, current, and
power measurements, as well as an accumulator for energy or charge.

> diff --git a/drivers/iio/adc/pac1711.c b/drivers/iio/adc/pac1711.c
> new file mode 100644
> index 0000000000000..f23d08205952f
> --- /dev/null
> +++ b/drivers/iio/adc/pac1711.c
[ ... ]
> +static int pac1711_set_acc_enable(struct pac1711_chip_info *info, int val)
> +{
[ ... ]
> +	cancel_delayed_work_sync(&info->work_chip_refresh);
> +
> +	scoped_guard(mutex, &info->lock) {
> +		if (val == info->enable_acc)
> +			return 0;

[Severity: High]
Could this early return permanently halt the background refresh task? If
another thread updates enable_acc concurrently and this thread cancels the
just-scheduled work, returning 0 here skips the schedule_delayed_work() call
below. Does this cause the hardware accumulator to silently overflow since it
is no longer refreshed?

> +
> +		/*
> +		 * Force a hardware refresh to zero out the internal accumulators
[ ... ]
> +static ssize_t pac1711_in_coulomb_counter_raw_show(struct device *dev,
> +						   struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lld\n", info->chip_reg_data.acc_val);

[Severity: Medium]
Can this sysfs read race with concurrent background updates? Since acc_val is
a 64-bit value accessed outside of info->lock, could this result in torn reads
on 32-bit platforms when the workqueue updates it concurrently?

> +}
[ ... ]
> +static int pac1711_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct pac1711_chip_info *info = iio_priv(indio_dev);
> +	int ret;
> +	u64 tmp = 0;
> +
> +	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_POWER:
> +			*val = (u32)info->chip_reg_data.vpower;
> +			*val2 = (u32)(info->chip_reg_data.vpower >> 32);
> +			return IIO_VAL_INT_64;

[Severity: Medium]
Are these 64-bit accesses safe against concurrent modification? Reading the
upper and lower 32 bits independently outside of info->lock might expose an
intermediate state if the background workqueue modifies vpower concurrently.

> +		case IIO_ENERGY:
> +			*val = (u32)info->chip_reg_data.acc_val;
> +			*val2 = (u32)(info->chip_reg_data.acc_val >> 32);
> +			return IIO_VAL_INT_64;
[ ... ]
> +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) {
> +			old_samp_rate = pac1711_samp_rate_map_tbl[info->sample_rate_idx];
> +			new_idx = pac1711_get_samp_rate_idx(val);
> +			if (new_idx < 0)
> +				return new_idx;
> +
> +			ret = i2c_smbus_read_i2c_block_data(client, PAC1711_CTRL_ACT_REG_ADDR,
> +							    sizeof(val_u16), (u8 *)&val_be16);
> +			if (ret != sizeof(val_u16)) {
> +				dev_err(&client->dev, "cannot read regs from 0x%02X\n",
> +					PAC1711_CTRL_ACT_REG_ADDR);
> +				return ret < 0 ? ret : -EIO;
> +			}
> +
> +			val_u16 = be16_to_cpu(val_be16);
> +			FIELD_MODIFY(PAC1711_CTRL_SAMPLE_MODE_MASK, &val_u16, new_idx);
> +			ret = i2c_smbus_write_i2c_block_data(client, PAC1711_CTRL_REG_ADDR,
> +							     sizeof(val_be16), (u8 *)&val_be16);

[Severity: High]
Does this code write the unmodified register back to the hardware? It looks like
val_u16 is updated, but is not converted and stored back into val_be16 before
calling i2c_smbus_write_i2c_block_data(). Should there be an assignment
updating val_be16 here?

> +			if (ret) {
> +				dev_err(dev, "Failed to configure sampling mode in 0x%02X\n",
> +					PAC1711_CTRL_ACT_REG_ADDR);
> +				return ret;
> +			}
> +
> +			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 -= refresh_time;

[Severity: Medium]
Will this reliably force a hardware refresh? Subtracting 49ms from a freshly
captured timestamp leaves it within the 50ms polling threshold. Doesn't the
time_after() check in pac1711_reg_snapshot() evaluate to false if the last
snapshot was taken very recently, causing the refresh command to be skipped?

> +		}
> +
> +		ret = pac1711_retrieve_data(info, ((1024 * 1000) / old_samp_rate));

[Severity: Medium]
Is the wait time calculation using the correct sampling rate? The chip was just
commanded to use the new rate, but old_samp_rate is used to calculate the
delay. If the rate was decreased, wouldn't this wait time be too short, causing
stale or incomplete data to be read?

> +		if (ret) {
> +			dev_err(dev, "%s - cannot snapshot ctrl and measurement regs\n", __func__);
> +			return ret;
> +		}
[ ... ]
> +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 pointer assignment generate a compiler warning? The global table
pac1711_samp_rate_map_tbl is declared as an unsigned int array, but the IIO
core passes a const int ** for vals. Should the mapping table be declared
as const int to match the expected signature?

> +		*length = ARRAY_SIZE(pac1711_samp_rate_map_tbl);
> +		return IIO_AVAIL_LIST;
> +	}
> +
> +	return -EINVAL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-pac1711-v3-0-dff81003b82f@microchip.com?part=2