Re: [PATCH 1/3] regulator: mp886x: fix vsel_mask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware I2C read/write errors during initialization are silently ignored, resulting in negative error codes stored in unsigned integers and corrupted state logic.
- [Medium] Potential division by zero in `mp8869_scale` due to unvalidated device tree properties.
--

commit e23dcb351cfbd15403625e41999e01844e07121f
Author: Jisheng Zhang <jszhang@kernel.org>

    regulator: mp886x: fix vsel_mask
    
    The MP886X vsel is 7bits, fix the vsel_mask.
    
    Fixes: 97be82880b61 ("regulator: add support for MP8869 regulator")

> diff --git a/drivers/regulator/mp886x.c b/drivers/regulator/mp886x.c
> index e0b62bc02a1e1..a10f3a20d413f 100644
> --- a/drivers/regulator/mp886x.c
> +++ b/drivers/regulator/mp886x.c
> @@ -267,14 +267,14 @@ static int mp886x_regulator_register(struct mp886x_device_info *di,
>  	rdesc->min_uV = 600000;
>  	rdesc->uV_step = 10000;
>  	rdesc->vsel_reg = MP886X_VSEL;
> -	rdesc->vsel_mask = 0x3f;
> +	rdesc->vsel_mask = 0x7f;
>  	rdesc->ramp_reg = MP886X_SYSCNTLREG1;
>  	rdesc->ramp_mask = MP886X_SLEW_MASK;
>  	rdesc->ramp_delay_table = di->ci->slew_rates;
>  	rdesc->n_ramp_values = ARRAY_SIZE(di->ci->slew_rates);
>  	rdesc->owner = THIS_MODULE;
>  
>  	rdev = devm_regulator_register(di->dev, &di->desc, config);
>  	if (IS_ERR(rdev))
>  		return PTR_ERR(rdev);
>  	di->sel = rdesc->ops->get_voltage_sel(rdev);

[Severity: High]
This is a pre-existing issue, but does this code properly handle negative error
codes if I2C communication fails?

If get_voltage_sel() fails and returns -EIO or another negative error code,
it gets implicitly cast and stored in the unsigned int di->sel. The error
is never returned to the caller.

This negative value could later cause issues in mp8867_set_voltage_sel():

drivers/regulator/mp886x.c:mp8867_set_voltage_sel() {
	...
	delta = di->sel - sel;
	if (abs(delta) <= 5)
		ret = regmap_update_bits(rdev->regmap, MP886X_SYSCNTLREG1,
					 MP886X_GO, 0);
	...
}

If di->sel holds an error code like -5 (0xFFFFFFFB), and sel is 0,
delta becomes -5. Since abs(-5) <= 5 is true, the driver incorrectly
writes to the register to clear MP886X_GO, masquerading an I2C error as a
valid small voltage step.

Additionally, hardware initialization errors appear to be ignored elsewhere,
such as in mp886x_set_switch_freq() where the return value of
regmap_update_bits() is not checked. Could the driver end up succeeding in
its probe even if the hardware is completely unresponsive?

[Severity: Medium]
This is also a pre-existing issue, but is there a risk of a division by zero
when reading the divider values from the device tree?

In mp886x_i2c_probe(), the driver reads the mps,fb-voltage-divider array:

drivers/regulator/mp886x.c:mp886x_i2c_probe() {
	...
	ret = of_property_read_u32_array(np, "mps,fb-voltage-divider",
					 di->r, 2);
	if (ret)
		return ret;
	...
}

There does not seem to be a check ensuring that the denominator (di->r[1])
is strictly positive. If the device tree provides a zero value for this second
resistor, wouldn't this cause a kernel panic when mp8869_scale() divides by
r2:

drivers/regulator/mp886x.c:mp8869_scale() {
	...
	u32 tmp = uv * r1 / r2;
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901043123.5401-1-jszhang@kernel.org?part=1