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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider:
- [High] Passing `__be16` to `i2c_smbus_write_word_data` sends bytes in the wrong order on big-endian architectures.
- [High] The power channel's scale calculation can overflow a 32-bit signed integer for valid shunt resistor values, leading to a negative scale factor.
- [High] Missing initialization of device characteristics on devicetree fallback path causes probe failure or incorrect measurements.
- [High] Concurrent sysfs reads and periodic background refreshes cause data races and torn reads on 64-bit cached register values.
- [High] Concurrent accesses to `pac1711_retrieve_data` check the refresh timestamp outside of a lock, leading to multiple back-to-back hardware refreshes that permanently discard fractional accumulated energy.
- [High] Accumulator overflow persistently returns `-EINVAL` and breaks all instantaneous measurement channels.
- [Medium] Integer division truncation in wait time calculation results in no delay for conversion completion at higher sample rates.
- [Medium] Timestamp cache invalidation fails when `jiffies` has not advanced.
- [Medium] Missing validation of I2C block read length allows parsing uninitialized data.
- [Medium] Custom sysfs attributes are used instead of standard IIO channels for Energy and Charge.
- [Medium] The in_energy_raw attribute outputs a fractional value, violating the IIO ABI which requires _raw attributes to be unscaled integers.
- [Medium] The in_coulomb_counter_scale attribute outputs a scale factor that evaluates to milliCoulombs, despite the custom ABI documenting it as Coulombs.
- [Medium] Inconsistent read-modify-write for the sample frequency configuration writes a read-only mirror register's contents back to the main control register.
- [Low] Passing an s32 array to device_property_read_u32_array causes an incompatible pointer type warning.
- [Low] `info->chip_reg_data.jiffies_tstamp` is modified outside the mutex lock, causing a data race.
--

--- Patch [2]: [PATCH v2 2/2] iio: adc: add support for PAC1711 ---
commit 3e5ab329ae21d2e946511636289fcfb6dbf555cd
Author: Ariana Lazar <ariana.lazar@microchip.com>
Subject: iio: adc: add support for PAC1711

This is 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..ee6adee31524e
> --- /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)
> +{
[ ... ]
> +	/* Read the data registers */
> +	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_ACC_COUNT_REG_ADDR,
> +					    PAC1711_MEAS_REG_SNAPSHOT_LEN,
> +					    (u8 *)info->chip_reg_data.meas_regs);
> +	if (ret < 0) {

[Severity: Medium]
Could this parse uninitialized memory if the I2C block read returns fewer
bytes than expected?

i2c_smbus_read_i2c_block_data() returns the number of bytes read on success.
Should it check if ret equals the requested length instead of just checking
for negative error codes?

> +		dev_err(dev, "%s - cannot read regs from 0x%02X\n",
> +			__func__, PAC1711_ACC_COUNT_REG_ADDR);
> +		return ret;
> +	}
[ ... ]
> +		if (check_add_overflow(info->chip_reg_data.acc_val, inc,
> +				       &info->chip_reg_data.acc_val)) {
> +			if (inc < 0)
> +				info->chip_reg_data.acc_val = S64_MIN;
> +			else
> +				info->chip_reg_data.acc_val = S64_MAX;
> +
> +			dev_err(dev, "Accumulator Overflow detected!\n");
> +			return -EINVAL;
> +		}

[Severity: High]
Does returning -EINVAL here permanently break all instantaneous measurement
channels once the accumulator overflows?

Because it aborts early, subsequent reads will see a positive increment,
trigger overflow again, and return -EINVAL, permanently disabling VBUS,
VSENSE, and VPOWER updates.

[ ... ]
> +static int pac1711_retrieve_data(struct pac1711_chip_info *info, u32 wait_time)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * Check if the minimal elapsed time has passed and if so,
> +	 * read again the chip, otherwise use the cached info.
> +	 */
> +	if (time_after(jiffies, info->chip_reg_data.jiffies_tstamp +
> +			msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS))) {

[Severity: High]
Can concurrent sysfs reads trigger multiple back-to-back hardware refreshes
here?

Since the time_after check and the jiffies_tstamp read occur without any
locking, two threads might both enter the refresh path, causing the second
thread to discard the fractional accumulated energy before it can accumulate.

[Severity: Medium]
If the system jiffies has not incremented since the timestamp was recorded,
will this time_after() check evaluate to false?

If the cache invalidation is triggered (e.g. by modifying the jiffies_tstamp
when changing sample rates), it might be ignored here, causing the driver
to return stale data.

> +		ret = pac1711_reg_snapshot(info, true, PAC1711_REFRESH_REG_ADDR,
> +					   wait_time);
[ ... ]
> +static ssize_t pac1711_in_power_acc_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);
> +	s64 curr_energy, int_part;
> +	int ret, rem;
> +
> +	ret = pac1711_retrieve_data(info, PAC1711_MIN_UPDATE_WAIT_TIME_US);
> +	if (ret)
> +		return ret;
> +
> +	/* Expresses the 64 bit energy value as a 64 bit integer and a 32 bit nano value */
> +	curr_energy = info->chip_reg_data.acc_val;

[Severity: High]
Can this result in torn reads on 32-bit architectures? 

The 64-bit acc_val variable is read here without holding info->lock, while
a background worker might be updating it concurrently.

> +	int_part = div_s64_rem(curr_energy, 1000000000, &rem);
> +
> +	if (rem < 0)
> +		return sysfs_emit(buf, "-%lld.%09u\n", abs(int_part), -rem);
> +	else
> +		return sysfs_emit(buf, "%lld.%09u\n", int_part, abs(rem));
> +}

[Severity: Medium]
Is it correct to output a formatted fractional string for a _raw attribute?

The IIO ABI specifies that _raw attributes should expose unscaled integers
from the hardware, so standard userspace parsers might fail to parse this.

[ ... ]
> +static ssize_t pac1711_in_coulomb_counter_scale_show(struct device *dev,
> +						     struct device_attribute *attr, char *buf)
> +{
[ ... ]
> +	/*
> +	 * Increasing precision
> +	 * (100mV * 1000000 * 1000000000) / 2^(12 or 16))
> +	 */
> +	tmp_u64 = div_u64(ref, info->shunt);
> +	rem = do_div(tmp_u64, 1000000000UL);
> +
> +	return sysfs_emit(buf, "%lld.%09u\n", tmp_u64, rem);
> +}

[Severity: Medium]
Does this scale factor evaluate to milliCoulombs rather than Coulombs?

The custom ABI documentation specifies the output unit is Coulombs, but the
calculated scale factor appears to represent milliCoulombs (mA * s), meaning
the userspace charge measurement would be off by a factor of 1000.

> +static IIO_DEVICE_ATTR(in_energy_raw, 0444,
> +		       pac1711_in_power_acc_raw_show, NULL, 0);

[Severity: Medium]
Should this use the standard IIO_ENERGY and IIO_CHARGE channel types instead
of custom sysfs attributes?

Using custom attributes prevents generic IIO userspace tools (like libiio)
from properly discovering, scaling, or buffering the energy and charge
measurements.

[ ... ]
> +static int pac1711_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
[ ... ]
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = info->chip_reg_data.vbus;
> +			return IIO_VAL_INT;
> +		case IIO_CURRENT:
> +			*val = info->chip_reg_data.vsense;
> +			return IIO_VAL_INT;
> +		case IIO_POWER:
> +			*val = (u32)info->chip_reg_data.vpower;
> +			*val2 = (u32)(info->chip_reg_data.vpower >> 32);

[Severity: High]
Can this result in torn reads on 32-bit architectures? 

The 64-bit vpower variable is read here in two halves without holding
info->lock, while a background worker might be updating it concurrently.

> +			return IIO_VAL_INT_64;
[ ... ]
> +		case PAC1711_VPOWER_REG_ADDR:
> +			/*
> +			 * Power - uW - it will use the combined scale
> +			 * for current and voltage
> +			 * current(mA) * voltage(mV) = power (uW)
> +			 */
> +			switch (info->chip_variant) {
> +			case PAC1711_PRODUCT_ID_1711:
> +			case PAC1711_PRODUCT_ID_1811:
> +				tmp = PAC1711_PRODUCT_VOLTAGE_PV_FSR;
> +				break;
> +			case PAC1711_PRODUCT_ID_1721:
> +			case PAC1711_PRODUCT_ID_1821:
> +				tmp = PAC1721_PRODUCT_VOLTAGE_PV_FSR;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +
> +			do_div(tmp, info->shunt);
> +			*val = (int)tmp;

[Severity: High]
Will this overflow a 32-bit signed integer when using a standard 1 mOhm
(1000 uOhm) shunt resistor?

Dividing 4,200,000,000,000 by 1000 results in 4.2 billion, which exceeds
INT_MAX and wraps around to a negative number when cast to int, leading to
incorrect power scale readings in userspace.

[ ... ]
> +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(tmp_u16), (u8 *)&tmp_be16);

[Severity: Medium]
Does this read-modify-write sequence read from the active status mirror
register (0x17) but write back to the main control register (0x01)?

This might inadvertently write back transient active status bits into the
configuration. In initialization, the driver correctly reads from 0x01.

> +			if (ret < 0) {
> +				dev_err(&client->dev, "cannot read regs from 0x%02X\n",
> +					PAC1711_CTRL_ACT_REG_ADDR);
> +				return ret;
> +			}
> +
> +			tmp_u16 = be16_to_cpu(tmp_be16);
> +			tmp_u16 &= ~PAC1711_CTRL_SAMPLE_MODE_MASK;
> +			tmp_u16 |= FIELD_PREP(PAC1711_CTRL_SAMPLE_MODE_MASK, new_idx);
> +			tmp_be16 = cpu_to_be16(tmp_u16);
> +
> +			ret = i2c_smbus_write_word_data(client, PAC1711_CTRL_REG_ADDR, tmp_be16);

[Severity: High]
Can this corrupt the device's control register on big-endian architectures?

i2c_smbus_write_word_data() expects a CPU-endian u16 and handles the byte
order for the SMBus transaction internally. Passing a __be16 will silently
reverse the byte order on the I2C wire on big-endian platforms.

> +			if (ret < 0) {
> +				dev_err(&client->dev, "Failed to configure sampling mode\n");
> +				return ret;
> +			}
> +
> +			info->sample_rate_idx = new_idx;
> +			info->chip_reg_data.ctrl_act_reg = tmp_u16;
> +		}
> +
> +		/* Force register snapshot and timestamp update with a refresh. */
> +		info->chip_reg_data.jiffies_tstamp -= msecs_to_jiffies(PAC1711_MIN_POLLING_TIME_MS);

[Severity: Low]
Is it safe to modify info->chip_reg_data.jiffies_tstamp outside the mutex
lock? The periodic worker thread could be accessing it concurrently, creating
a data race.

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

[Severity: Medium]
When the sample rate is 4096 or 8192 SPS, does this integer division evaluate
to zero before the multiplication?

This evaluates to 0 delay, potentially causing the driver to bypass the
required hardware conversion delay. Should the multiplication happen before
the division?

[ ... ]
> +static int pac1711_chip_identify(struct iio_dev *indio_dev, struct pac1711_chip_info *info)
> +{
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &client->dev;
> +	u8 chip_rev_info[3];
> +	int ret;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, PAC1711_PID_REG_ADDR,
> +					    sizeof(chip_rev_info), chip_rev_info);
> +	if (ret < 0) {

[Severity: Medium]
Could this parse uninitialized memory if the I2C block read returns fewer
bytes than expected?

i2c_smbus_read_i2c_block_data() returns the number of bytes read on success.
Should it check if ret equals the requested length instead of just checking
for negative error codes?

[ ... ]
> +static int pac1711_init_vbus_vsense_ranges(struct pac1711_chip_info *info, bool is_vbus)
> +{
> +	struct i2c_client *client = info->client;
> +	struct device *dev = &client->dev;
> +	const char *prop_name;
> +	s32 vals[2];
> +	int ret;
> +
> +	if (is_vbus)
> +		prop_name = "microchip,vbus-input-range-microvolt";
> +	else
> +		prop_name = "microchip,vsense-input-range-microvolt";
> +
> +	ret = device_property_read_u32_array(dev, prop_name, vals, 2);

[Severity: Low]
Does passing an s32 array to device_property_read_u32_array() cause an
incompatible pointer type warning? This might break builds compiling with
-Werror.

[ ... ]
> +static int pac1711_chip_configure(struct pac1711_chip_info *info)
> +{
[ ... ]
> +	/* Configure the accumulation mode */
> +	tmp_u16 &= ~PAC1711_CTRL_ACC_MODE_MASK;
> +	tmp_u16 |= FIELD_PREP(PAC1711_CTRL_ACC_MODE_MASK, info->accumulation_mode);
> +
> +	tmp_be16 = cpu_to_be16(tmp_u16);
> +	ret = i2c_smbus_write_word_data(client, PAC1711_CTRL_REG_ADDR, tmp_be16);

[Severity: High]
Can this corrupt the device's control register on big-endian architectures?

i2c_smbus_write_word_data() expects a CPU-endian u16 and handles the byte
order for the SMBus transaction internally. Passing a __be16 will silently
reverse the byte order on the I2C wire on big-endian platforms.

[ ... ]
> +	/*
> +	 * Get the current (in the chip) sampling speed and compute the
> +	 * required timeout based on its value the timeout is 1/sampling_speed
> +	 * wait the maximum amount of time to be on the safe side - the
> +	 * maximum wait time is for 8sps
> +	 */
> +	wait_time = (1024 / pac1711_samp_rate_map_tbl[info->sample_rate_idx]) * 1000;
> +	fsleep(wait_time);

[Severity: Medium]
When the sample rate is 4096 or 8192 SPS, does this integer division evaluate
to zero before the multiplication?

This evaluates to 0 delay, potentially causing the driver to bypass the
required hardware conversion delay. Should the multiplication happen before
the division?

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

[Severity: High]
Does this fallback path leave info->voltage_range_idx and
info->is_pac18x1_family uninitialized?

If I2C identification fails, it appears these remain 0 and false, defaulting
to PAC1711 behavior and silently corrupting sensor data for other chip
variants like PAC1721 or PAC1821.

> +	/* Always start with accumulation channels enabled. */
> +	info->enable_acc = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-pac1711-v2-0-609bc026093c@microchip.com?part=2