Re: [PATCH v2 2/3] hwmon: pmbus: Add support for Silergy SQ24860

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Passing a 64-bit divisor to `div_u64()` silently truncates the divisor to 32 bits, causing an incorrect calculation that unconditionally fails probe, and potentially triggering a kernel panic due to division by zero.
- [High] Unsigned underflow when writing a low `PMBUS_IIN_OC_FAULT_LIMIT` value inadvertently configures the maximum hardware over-current limit, defeating hardware protection.
--

commit ba80340cc247f7eb04cd41303e3f6374716421eb
Author: Ziming Zhu <ziming.zhu@silergycorp.com>

hwmon: pmbus: Add support for Silergy SQ24860

Add PMBus hwmon support for the Silergy SQ24860 eFuse.
The driver reports input voltage, output voltage, auxiliary voltage,
input current, input power, and temperature.

> diff --git a/drivers/hwmon/pmbus/sq24860.c b/drivers/hwmon/pmbus/sq24860.c
> new file mode 100644
> index 0000000000000..43a2cb542169c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/sq24860.c

[ ... ]

> +static int sq24860_write_iin_cal_gain(struct i2c_client *client, u32 rimon,
> +					u32 gimon)
> +{
> +	u64 temp = (u64)6400 * 1000000000 * 1000;
> +	u64 denom;
> +	u64 word;
> +
> +	if (!rimon || !gimon)
> +		return -EINVAL;
> +
> +	denom = (u64)rimon * gimon;
> +	word = div_u64(temp, denom);
> +	if (word > U16_MAX)
> +		return -ERANGE;

[Severity: High]
Does this code inadvertently truncate the 64-bit divisor?

Because denom is a 64-bit unsigned integer, passing it to div_u64() which
expects a 32-bit divisor will silently truncate denom to its lower 32 bits.

With the default values (rimon = 1600000000, gimon = 18180), denom evaluates
to 29,088,000,000,000. Truncating this to 32 bits yields 1,044,881,408, causing
the division to incorrectly yield 6,125,096 instead of 220.

Since this exceeds U16_MAX, won't this unconditionally return -ERANGE and
fail the probe? Also, if denom happens to be an exact multiple of 2^32,
could the truncated divisor become 0 and trigger a kernel panic?

Should this use div64_u64() instead?

[ ... ]

> +static int sq24860_write_word_data(struct i2c_client *client,
> +				    int page, int reg, u16 value)
> +{
> +	int ret;
> +
> +	switch (reg) {

[ ... ]

> +	case PMBUS_IIN_OC_FAULT_LIMIT:
> +		value -= SQ24860_IIN_OCF_OFF;
> +		value = DIV_ROUND_CLOSEST(((unsigned int)value) * SQ24860_IIN_OCF_DIV,
> +					  SQ24860_IIN_OCF_NUM);
> +		value = clamp_val(value, 0, 0x3f);
> +		ret = pmbus_write_byte_data(client, page, SQ24860_VIREF, value);
> +		break;

[Severity: High]
Will subtracting from value underflow if it is less than 165?

Since value is an unsigned 16-bit integer, if user space requests a
restrictive over-current limit corresponding to a value less than
SQ24860_IIN_OCF_OFF (165), value will underflow and wrap around to a
large positive number.

Because this large value is subsequently clamped to 0x3f (63), won't
attempting to set a restrictive over-current limit inadvertently configure
the hardware to its absolute maximum limit instead?

Could value be checked against SQ24860_IIN_OCF_OFF before the subtraction
to prevent silently defeating the hardware protection?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605063042.91776-1-zmzhu0630@163.com?part=2