Re: [PATCH v2 1/3] hwmon: ina238: add support for samples and update_interval

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Integer truncation from u64 to u32 in `ina238_interval_us_to_conv_time` causes large user inputs to wrap around, resulting in incorrect hardware configuration on 64-bit platforms.
--

commit 4bfb82fb9bb164125fe0facd52cce87c1d8b248e
Author: Ferdinand Schwenk <ferdinand.schwenk@advastore.com>

hwmon: ina238: add support for samples and update_interval

This commit exposes the INA238 ADC averaging count and conversion timing
through samples and update_interval hwmon attributes. It caches ADC_CONFIG
to optimize read-modify-write operations. The conversion times are derived
using per-chip lookup tables.

> diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c
> index ff67b03189f73..667f78d43a6a8 100644
> --- a/drivers/hwmon/ina238.c
> +++ b/drivers/hwmon/ina238.c

[ ... ]

> @@ -261,6 +296,97 @@ static int ina228_read_voltage(struct ina238_data *data, int channel, long *val)

[ ... ]

> +static inline u32 ina238_interval_ms_to_conv_time(long interval, u32 samples)
> +{
> +	u64 interval_us;
> +
> +	interval = clamp_val(interval, 0, LONG_MAX / 1000);
> +	interval_us = (u64)interval * 1000;
> +
> +	/*
> +	 * update_interval reports the ADC cycle time including averaging.
> +	 * The target per-field conversion time is interval_us / (samples * 3).
> +	 */
> +	return DIV_ROUND_CLOSEST_ULL(interval_us, samples * 3);
> +}

[Severity: Medium]
Does returning a u32 here cause integer truncation for large user inputs on
64-bit platforms? Since interval is clamped to LONG_MAX / 1000, interval_us
can be extremely large. The DIV_ROUND_CLOSEST_ULL macro evaluates to an
unsigned long long, but returning it as u32 will silently truncate results
exceeding 4294967295.

[ ... ]

> +static int ina238_write_chip(struct device *dev, u32 attr, long val)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	u16 adc_config;
> +	int idx, ret;
> +
> +	switch (attr) {

[ ... ]

> +	case hwmon_chip_update_interval:
> +		val = ina238_interval_ms_to_conv_time(val, ina238_samples(data));
> +		idx = find_closest(val, data->config->conv_time,
> +				   ARRAY_SIZE(ina238_conv_time));

[Severity: Medium]
Could the truncation in the interval helper above cause this find_closest()
lookup to select the wrong hardware configuration? For example, if a user
input results in a target conversion time of 4294967333 us, it wraps to
37 us, which would incorrectly select the minimum conversion time (50 us)
instead of clamping to the maximum supported hardware interval (8230 us).

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-hwmon-ina238-update-interval-us-v2-v2-0-2d939fbb2ea1@advastore.com?part=1