Re: [PATCH v3 2/2] hwmon: Add support for TI INA4230 power monitor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Kernel panic due to division by zero when calculating conversion time with all channels disabled.
- [High] The driver modifies its cached `CONFIG1` register during probe but never writes this configuration to the hardware.
- [High] Resetting the device in `ina4230_resume()` corrupts the regmap cache for `CONFIG2` and prevents it from being restored.
- [High] Silent failure when modifying shunt channel enable attributes due to missing channel index modulo.
- [High] `ina4230_read_energy` corrupts energy readings by reading across 16-bit register boundaries.
- [High] Sysfs attributes remain active after `ina4230_remove()` completes, causing a lifecycle race that can trigger PM counter underflows or bus errors.
- [High] Infinite polling loop in `ina4230_wait_for_data` when no channels are active.
- [Medium] Concurrent sysfs writes to extra attributes (`shuntX_resistor`) cause a data race on shared driver state.
- [Medium] Unbalanced PM runtime usage counter decrements in the probe error path and teardown path.
- [Medium] `ina4230_is_enabled()` incorrectly reports a channel as enabled based on the global device PM state.
- [Medium] Missing averaging multiplier in conversion time calculation causes premature timeouts.
- [Low] Missing `HWMON_I_ENABLE` flag prevents the exposure of channel enable/disable sysfs attributes.
--

--- Patch [2]: [PATCH v3 2/2] hwmon: Add support for TI INA4230 power monitor ---
commit fc5e3f0118ad0f82f69443f939a77d438a653011
Author: Alexey Charkov <alchark@flipper.net>
Subject: hwmon: Add support for TI INA4230 power monitor

This commit adds a driver for the TI INA4230 Quad Current/Voltage Monitor.
It supports voltage, current, power and energy measurements over I2C, but
skips the alert functionality in this initial implementation.

> diff --git a/drivers/hwmon/ina4230.c b/drivers/hwmon/ina4230.c
> new file mode 100644
> index 0000000000000..7e5c7fe2274b3
> --- /dev/null
> +++ b/drivers/hwmon/ina4230.c
[ ... ]
> +static inline bool ina4230_is_enabled(struct ina4230_data *ina, int channel)
> +{
> +	return pm_runtime_active(ina->pm_dev) &&
> +		!ina->inputs[channel].disconnected;
> +}

[Severity: Medium]
Could this be a regression where the function no longer reflects individual
channel state?

It checks pm_runtime_active(), which represents the global power state of
the chip. If channel 1 is disabled via sysfs but channel 2 remains enabled,
the device remains active and this would incorrectly return true for channel 1.
This might allow sysfs reads to fetch stale data instead of returning -ENODATA
as intended by the API contract.

[ ... ]
> +static inline u32 ina4230_interval_ms_to_conv_time(u16 config, int interval)
> +{
> +	u32 channels = hweight16(config & INA4230_CONFIG1_ACTIVE_CHANNEL_MASK);
> +	u32 samples_idx = FIELD_GET(INA4230_CONFIG1_AVG_MASK, config);
> +	u32 samples = ina4230_avg_samples[samples_idx];
> +
> +	/* Bisect the result to Bus and Shunt conversion times */
> +	return DIV_ROUND_CLOSEST(interval * 1000 / 2, channels * samples);
> +}

[Severity: High]
What happens here if all channels are disabled?

If the channel bitmask is zero (either disconnected in DT or disabled via
sysfs), the channels variable evaluates to 0. The subsequent calculation
would then result in a division by zero panic, which seems like a regression
during sysfs writes.

[ ... ]
> +static inline u32 ina4230_reg_to_interval_us(u16 config)
> +{
> +	u32 channels = hweight16(config & INA4230_CONFIG1_ACTIVE_CHANNEL_MASK);
> +	u32 vbus_ct_idx = FIELD_GET(INA4230_CONFIG1_VBUSCT_MASK, config);
> +	u32 vsh_ct_idx = FIELD_GET(INA4230_CONFIG1_VSHCT_MASK, config);
> +	u32 vbus_ct = ina4230_conv_time[vbus_ct_idx];
> +	u32 vsh_ct = ina4230_conv_time[vsh_ct_idx];
> +
> +	/* Calculate total conversion time */
> +	return channels * (vbus_ct + vsh_ct);
> +}

[Severity: Medium]
Should this calculation include the number of samples being averaged?

ina4230_interval_ms_to_conv_time() correctly factors in the sample count,
but it is omitted here. This could cause the returned timeout duration to be
significantly shorter than the actual hardware integration time, potentially
leading to premature timeouts during polling, which could be a regression.

[ ... ]
> +static inline int ina4230_wait_for_data(struct ina4230_data *ina)
> +{
> +	u32 wait, cvrf;
> +
> +	wait = ina4230_reg_to_interval_us(ina->reg_config1);
> +
> +	/* Polling the CVRF bit to make sure read data is ready */
> +	return regmap_field_read_poll_timeout(ina->fields[F_CVRF],
> +					      cvrf, cvrf, wait, wait * 2);
> +}

[Severity: High]
Is it possible for this regression to cause an infinite spin?

If no channels are active, ina4230_reg_to_interval_us() returns 0. Passing
0 as the timeout to regmap_field_read_poll_timeout() usually means to poll
forever without timing out. Since no hardware conversions are occurring, the
CVRF bit will never be asserted, potentially causing a soft lockup.

[ ... ]
> +static int ina4230_set_calibration(struct ina4230_data *ina, int channel)
> +{
[ ... ]
> +	input->shunt_gain = shunt_range_uV > 20480 ? 1 : 4;
> +	ina->reg_config2 &= ~INA4230_CONFIG2_RANGE_CH(channel);
> +	if (input->shunt_gain == 4)
> +		ina->reg_config2 |= INA4230_CONFIG2_RANGE_CH(channel);
> +
> +	ret = regmap_write(ina->regmap, INA4230_CONFIG2, ina->reg_config2);

[Severity: Medium]
Is a lock needed here to protect the read-modify-write of ina->reg_config2?

Extra sysfs attributes backed by ina4230_shunt_store() do not take the hwmon
core lock automatically and can be executed concurrently. Modifying the
cached config value without a driver-level lock could lead to corrupted
internal state and hardware state, which might be a regression.

[ ... ]
> +static int ina4230_read_energy(struct device *dev, u32 attr, int channel, long *val)
> +{
[ ... ]
> +	case hwmon_energy_input:
> +		if (!ina4230_is_enabled(ina, channel))
> +			return -ENODATA;
> +
> +		ret = regmap_noinc_read(ina->regmap, reg, &regval, sizeof(regval));

[Severity: High]
Does reading 4 bytes here pull in data from adjacent registers?

The registers on this chip are 16 bits wide (val_bits = 16), and the energy
registers are only 16 bits. Reading sizeof(regval), which is 4 bytes for a
__be32, would read 2 bytes from the energy register and auto-increment to read
the next 2 bytes from the adjacent calibration register, resulting in a
corrupted value. Could this be a data corruption regression?

[ ... ]
> +static int ina4230_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_chip:
> +		ret = ina4230_write_chip(dev, attr, val);
> +		break;
> +	case hwmon_in:
> +		/* 0-align channel ID */
> +		ret = ina4230_write_enable(dev, channel - 1, val);

[Severity: High]
Does this handle shunt channel indices correctly?

Sysfs paths provide channel numbers up to 8 (voltage channels 1-4 and shunt
channels 5-8). For a shunt channel like channel 5, passing channel - 1 results
in index 4. The channel enablement mask only covers bits 12-15, so shifting
by 4 would push the mask into bits 16-19, leading to an empty mask. The driver
would then silently fail to disable the shunt channel, causing a regression in
expected behavior.

[ ... ]
> +static const struct hwmon_channel_info * const ina4230_info[] = {
[ ... ]
> +	HWMON_CHANNEL_INFO(in,
> +			   /* 0: dummy, skipped in is_visible */
> +			   HWMON_I_INPUT,
> +			   /* 1-4: input voltage Channels */
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,

[Severity: Low]
Should HWMON_I_ENABLE be included in these flags?

The driver implements logic in ina4230_write_enable() and hwmon_in_enable to
support dynamically enabling and disabling channels, but without the
HWMON_I_ENABLE flag here, the hwmon core will never create the corresponding
sysfs attributes, masking this feature regression.

[ ... ]
> +static int ina4230_probe(struct i2c_client *client)
> +{
[ ... ]
> +	/* The driver will be reset, so use reset value */
> +	ina->reg_config1 = INA4230_CONFIG_DEFAULT;
> +	ina->reg_config2 = 0;
> +
> +	if (ina->single_shot)
> +		FIELD_MODIFY(INA4230_CONFIG1_MODE_MASK, &ina->reg_config1,
> +			     INA4230_MODE_BUS_SHUNT_SINGLE);
> +
> +	/* Disable channels if their inputs are disconnected */
> +	for (i = 0; i < INA4230_NUM_CHANNELS; i++) {
> +		if (ina->inputs[i].disconnected)
> +			ina->reg_config1 &= ~INA4230_CONFIG_CHx_EN(i);
> +	}
> +
> +	ina->pm_dev = dev;

[Severity: High]
Is the cached reg_config1 value ever written to the hardware during
initialization?

The driver modifies the cache for defaults, single-shot mode, and disconnected
channels, but there doesn't appear to be a regmap_write() or regcache_sync()
to push these changes to the physical device. The device might remain in its
default continuous mode with all channels active, leading to a state regression.

[ ... ]
> +static void ina4230_remove(struct i2c_client *client)
> +{
> +	struct ina4230_data *ina = dev_get_drvdata(&client->dev);
> +	int i;
> +
> +	pm_runtime_disable(ina->pm_dev);
> +	pm_runtime_set_suspended(ina->pm_dev);

[Severity: High]
Can sysfs attributes be accessed after pm_runtime_disable() is called here?

The sysfs nodes are managed by devres and will persist until after the remove
function completes. A concurrent sysfs read or write could attempt I2C traffic
or trigger PM counter underflows on the logically removed device. Would it be
safer to use devm_pm_runtime_enable() to serialize the teardown and avoid this
regression?

> +
> +	/* pm_runtime_put_noidle() for connected channels to balance get_sync */
> +	for (i = 0; i < INA4230_NUM_CHANNELS; i++) {
> +		if (!ina->inputs[i].disconnected)
> +			pm_runtime_put_noidle(ina->pm_dev);
> +	}
> +}

[Severity: Medium]
Does this teardown sequence handle disabled channels correctly?

The loop blindly calls pm_runtime_put_noidle() for all physically connected
channels. If a channel was previously disabled via sysfs (which already called
pm_runtime_put_sync()), this could drop the reference count below zero and
cause PM usage counter underflows. This regression also appears to exist in
the probe error path.

[ ... ]
> +static int ina4230_resume(struct device *dev)
> +{
> +	struct ina4230_data *ina = dev_get_drvdata(dev);
> +	int ret;
> +
> +	regcache_cache_only(ina->regmap, false);
> +
> +	/* Software reset the chip */
> +	ret = regmap_field_write(ina->fields[F_RST], true);
> +	if (ret) {
> +		dev_err(dev, "Unable to reset device\n");
> +		return ret;
> +	}
> +
> +	/* Restore cached register values to hardware */
> +	ret = regcache_sync(ina->regmap);

[Severity: High]
Does writing true to F_RST bypass the regcache and prevent CONFIG2 from
being restored?

Sending the reset command through the active regmap alters the regcache entry
for the register containing F_RST and clears its dirty flag. When
regcache_sync() runs immediately after, it may consider that register clean
and skip restoring the calibration settings to the hardware, resulting in a
configuration regression.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260310-ina4230-v3-0-06ab3a77c570@flipper.net?part=2