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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing validation for the `silergy,rimon-micro-ohms` device tree property allows a division by zero kernel panic and integer truncation.
- [High] Unchecked subtraction in the `PMBUS_IIN_OC_FAULT_LIMIT` calculation leads to an integer underflow that defeats hardware over-current protection.
- [Low] Using a `GENMASK` macro as the exponent for a bitwise shift operation is conceptually incorrect and brittle.
- [Low] A comment regarding `PSC_CURRENT_IN` and `PSC_POWER` scaling parameters describes software scaling logic that does not exist in the code.
--

commit 837bf8e7761b5fce46f4967982468008be6c4700
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..1c3b2bf2a026f
> --- /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;
> +	u16 word = temp / ((u64)rimon * gimon);

[Severity: High]
If the silergy,rimon-micro-ohms property is read as 0 from the device tree
during sq24860_probe(), will this division by zero cause a kernel panic
during boot?

Additionally, if a user provides a very small value (e.g., < 5394 micro-ohms),
the quotient will exceed U16_MAX. Since the result is directly assigned to
a u16, could this cause silent truncation and result in garbage calibration
values being written to the hardware?

> +	int ret = i2c_smbus_write_word_data(client, SQ24860_IIN_CAL_GAIN,
> +						word);
> +	return ret;
> +}

[ ... ]

> +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;

[Severity: High]
Does this unchecked subtraction risk an integer underflow if a user
requests a limit smaller than SQ24860_IIN_OCF_OFF (165)?

If value -= 165 underflows and wraps around to a large number, the
subsequent clamp_val() would hit its upper bound of 63. Could this
cause a safely low requested limit to configure the hardware's maximum
possible over-current limit, bypassing the safety threshold?

> +		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;
> +
> +	case PMBUS_VIRT_SAMPLES:
> +		value = clamp_val(value, 1, 1 << PK_MIN_AVG_AVG_CNT);

[Severity: Low]
Is it safe to use PK_MIN_AVG_AVG_CNT as a shift exponent here?

Since PK_MIN_AVG_AVG_CNT is defined as GENMASK(2, 0), it evaluates
to 7. While 1 << 7 coincidentally produces the correct limit of 128
samples, using a bitmask macro as a shift exponent is brittle. If the field
were to shift (for example, GENMASK(3, 1) evaluating to 14), would this logic
break catastrophically while still passing compilation?

> +		value = ilog2(value);
> +		ret = pmbus_update_byte_data(client, page, SQ24860_PK_MIN_AVG,
> +					     PK_MIN_AVG_AVG_CNT,
> +					     FIELD_PREP(PK_MIN_AVG_AVG_CNT, value));
> +		break;

[ ... ]

> +static const struct pmbus_driver_info sq24860_base_info = {
> +	.pages = 1,
> +	.format[PSC_VOLTAGE_IN] = direct,

[ ... ]

> +	.format[PSC_TEMPERATURE] = direct,
> +	.m[PSC_TEMPERATURE] = 1,
> +	.b[PSC_TEMPERATURE] = 0,
> +	.R[PSC_TEMPERATURE] = 0,
> +	/*
> +	 * Current and Power measurement depends on the ohm value
> +	 * of Rimon. m is multiplied by 1000 below to have an integer
> +	 * and -3 is added to R to compensate.
> +	 */

[Severity: Low]
Is this comment accurate for the current implementation?

The driver info struct definitions below merely set .m = 16 and .R = 0,
with no multiplication by 1000 or -3 added to R. Is this an orphaned
copy-paste comment from another PMBus driver?

> +	.format[PSC_CURRENT_IN] = direct,
> +	.m[PSC_CURRENT_IN] = 16,
> +	.b[PSC_CURRENT_IN] = 0,
> +	.R[PSC_CURRENT_IN] = 0,

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