Re: [PATCH 2/2] regulator: mp8899: Add MPS MP8899 PMIC regulator driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect `n_voltages` configuration prevents accessing the full voltage range.
- [Medium] Missing `.active_discharge_on` initialization always disables active discharge regardless of the requested state.
- [Medium] Time-Of-Check to Time-Of-Use (TOCTOU) race condition in custom generic debugfs register access due to shared state.
- [Medium] Lack of memory barriers between updating the voltage cache and its validity flag allows the panic handler to read uninitialized or torn data.
- [Low] The driver violates I2C subsystem guidelines by manually creating a custom debugfs root directory and uses a dangerously small buffer for its name.
--

commit 0f71ad8288b0930fc14cef9f2d4480d94d55a12d
Author: Manikanta Mylavarapu <manikanta.mylavarapu@oss.qualcomm.com>

regulator: mp8899: Add MPS MP8899 PMIC regulator driver

Add a regulator driver for the MPS MP8899 PMIC, which provides four
synchronous buck converters controlled over I2C. The driver supports
programmable output voltages and registers a panic notifier to log the
last known voltage state.

> diff --git a/drivers/regulator/mp8899.c b/drivers/regulator/mp8899.c
> index 0000000000000..ebd1f75d9e2f5 100644
> --- /dev/null
> +++ b/drivers/regulator/mp8899.c
[ ... ]
> @@ -234,6 +234,31 @@ static int mp8899_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel)
[ ... ]
> +	/* Cache the successfully set voltage for panic handler */
> +	voltage_uv = regulator_list_voltage_linear_range(rdev, sel);
> +	if (voltage_uv > 0) {
> +		info->cached_voltage_uv[buck_id] = voltage_uv;
> +		info->voltage_cache_valid[buck_id] = true;

[Severity: Medium]
Could the panic handler observe voltage_cache_valid as true before the new
voltage is fully written to memory?

Since the panic handler runs asynchronously (e.g., from an NMI), would using
memory barrier primitives like smp_store_release() and smp_load_acquire()
ensure the handler doesn't log uninitialized or stale voltage data?

> +		dev_dbg(info->dev, "Buck%d: Cached voltage %duV\n", buck_id + 1, voltage_uv);
> +	}
[ ... ]
> @@ -405,6 +405,37 @@ static ssize_t mp8899_debugfs_reg_addr_write(struct file *file,
[ ... ]
> +	/* Validate register address */
> +	if (!mp8899_is_valid_reg(reg_addr)) {
> +		dev_err(info->dev, "Invalid register address 0x%02x (valid: 0x00-0x21, 0x2D)\n",
> +			reg_addr);
> +		return -EINVAL;
> +	}
> +
> +	info->debug_reg_addr = reg_addr;

[Severity: Medium]
Is there a Time-Of-Check to Time-Of-Use (TOCTOU) race condition here if
multiple processes interact with these debugfs files concurrently?

If process A sets the target address, but process B overwrites
info->debug_reg_addr before process A can execute its read or write on
reg_value, could this cause a user to inadvertently write to an unintended
register (such as the write-only CLEAR register)?

> +	dev_dbg(info->dev, "Register address set to 0x%02x\n", reg_addr);
> +
> +	return count;
> +}
[ ... ]
> @@ -597,6 +597,29 @@ static const struct regulator_ops mp8899_buck_ops = {
[ ... ]
> +#define MP8899_BUCK_DESC(_name, _id) \
> +	[MP8899_BUCK##_id] = { \
> +		.id = MP8899_BUCK##_id, \
> +		.name = _name, \
> +		.of_match = _name, \
> +		.regulators_node = "regulators", \
> +		.of_parse_cb = mp8899_parse_cb, \
> +		.ops = &mp8899_buck_ops, \
> +		.type = REGULATOR_VOLTAGE, \
> +		.min_uV = MP8899_VREF_MIN_UV, \
> +		.n_voltages = MP8899_N_VOLTAGES, \
> +		.vsel_reg = MP8899_BUCK##_id##_CTL5, \
> +		.vsel_mask = 0xFF, \
> +		.apply_reg = MP8899_BUCK##_id##_CTL6, \
> +		.apply_bit = 7, \
> +		.enable_reg = MP8899_SYSTEM1, \
> +		.enable_mask = MP8899_EN##_id##_MASK, \
> +		.curr_table = mp8899_current_limits, \
> +		.n_current_limits = ARRAY_SIZE(mp8899_current_limits), \
> +		.csel_reg = MP8899_BUCK##_id##_CTL3, \
> +		.csel_mask = MP8899_CURRENT_LIMIT_MASK, \
> +		.active_discharge_reg = MP8899_BUCK##_id##_CTL1, \
> +		.active_discharge_mask = MP8899_VOUT_DIS_EN_MASK, \

[Severity: Medium]
Since .active_discharge_on is not explicitly initialized here, it will
default to 0.

When regulator_set_active_discharge_regmap() attempts to enable active
discharge, will it write a 0 and inadvertently clear the
MP8899_VOUT_DIS_EN_MASK bit instead of setting it?

> +		.soft_start_reg = MP8899_BUCK##_id##_CTL2, \
[ ... ]
> @@ -746,6 +746,26 @@ static void mp8899_debugfs_init(struct mp8899_regulator_info *info,
> +				struct i2c_client *client)
> +{
> +	char name[16];
> +
> +	/* Create root debugfs directory: /sys/kernel/debug/mp8899-<bus>-<addr> */
> +	snprintf(name, sizeof(name), "mp8899-%d-%04x",
> +		 client->adapter->nr, client->addr);
> +	info->debugfs_root = debugfs_create_dir(name, NULL);

[Severity: Low]
Should this driver use the standard client->debugfs dentry provided by the I2C
subsystem instead of manually creating and managing its own root directory?

Additionally, if the system dynamically assigns an I2C bus number >= 1000,
will this 16-byte buffer truncate the directory name?

> +	if (IS_ERR_OR_NULL(info->debugfs_root)) {
> +		dev_warn(info->dev, "Failed to create debugfs root directory\n");
[ ... ]
> @@ -851,6 +851,46 @@ static int mp8899_i2c_probe(struct i2c_client *client)
[ ... ]
> +		if (vout_select & MP8899_VOUT_SELECT_MASK) {
> +			/* 1.0mV step mode */
> +			info->rdesc[i].linear_ranges = mp8899_buck_ranges_1000uv;
> +			info->rdesc[i].n_linear_ranges = ARRAY_SIZE(mp8899_buck_ranges_1000uv);
> +			info->rdesc[i].n_voltages = MP8899_N_VOLTAGES_1MV;  /* 3201 voltages */
> +			dev_dbg(dev, "Buck%d: 1.0mV step mode\n", i + 1);
> +		} else {
> +			/* 0.5mV step mode */
> +			info->rdesc[i].linear_ranges = mp8899_buck_ranges_500uv;
> +			info->rdesc[i].n_linear_ranges = ARRAY_SIZE(mp8899_buck_ranges_500uv);
> +			info->rdesc[i].n_voltages = MP8899_N_VOLTAGES;  /* 3296 voltages */

[Severity: High]
The linear range for 0.5mV mode maps voltages to hardware selectors up to 4095:

static const struct linear_range mp8899_buck_ranges_500uv[] = {
    REGULATOR_LINEAR_RANGE(400000, 800, 4095, 500),
};

Since the regulator core uses rdev->desc->n_voltages for bounds checking
(expecting it to be max_selector + 1), does setting n_voltages to 3296
silently reject valid requests in the upper ranges of the PMIC?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-mp8899-regulator-driver-v1-0-30d14421b7f1@oss.qualcomm.com?part=2