Re: [PATCH v10 8/9] regulator: Add MediaTek MT6392 regulator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver mutates the statically defined global array `mt6392_regulators` during probe, causing state corruption and race conditions for multi-instance deployments or re-probing.
--

commit b458a23ed7fdbe7db2ebc29e150ed87ba4a12dbe
Author: Fabien Parent <parent.f@gmail.com>

regulator: Add MediaTek MT6392 regulator

This commit introduces support for the MediaTek MT6392 PMIC regulator.
The PMIC connects as a SPI slave wrapped inside PWRAP and is found on
MT8167 and MT8516 SoC based boards.

> diff --git a/drivers/regulator/mt6392-regulator.c b/drivers/regulator/mt6392-regulator.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/regulator/mt6392-regulator.c

[ ... ]

> +// Buck regulators can be in Register mode or Normal mode.
> +// Each mode uses a different register to set the desired voltage.
> +static int mt6392_set_buck_vsel_reg(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6392 = dev_get_drvdata(pdev->dev.parent);
> +	int i;
> +	u32 regval;
> +
> +	for (i = 0; i < MT6392_MAX_REGULATOR; i++) {
> +		if (mt6392_regulators[i].vselctrl_reg) {
> +			// Read the vselctrl_reg register
> +			if (regmap_read(mt6392->regmap,
> +					mt6392_regulators[i].vselctrl_reg,
> +					&regval) < 0) {
> +				dev_err(&pdev->dev,
> +					"Failed to read buck ctrl\n");
> +				return -EIO;
> +			}
> +
> +			// vselctrl_reg[vselctrl_mask] defines the mode
> +			if (regval & mt6392_regulators[i].vselctrl_mask) {
> +				// Regulator in Normal mode
> +				mt6392_regulators[i].desc.vsel_reg =
> +					mt6392_regulators[i].vsel_normal_mode_reg;
> +				mt6392_regulators[i].desc.vsel_mask =
> +					mt6392_regulators[i].vsel_normal_mode_mask;

[Severity: High]
Does this code mutate the shared global mt6392_regulators array based on
per-instance hardware state? If multiple instances of this PMIC are present,
or during unbind/re-bind cycles, won't this corrupt the state for other
instances since the regulator core expects regulator_desc to be static and
independent across deployments?

> +			} else {
> +				// Regulator in Register mode
> +				mt6392_regulators[i].desc.vsel_reg =
> +					mt6392_regulators[i].vsel_reg_mode_reg;
> +				mt6392_regulators[i].desc.vsel_mask =
> +					mt6392_regulators[i].vsel_reg_mode_mask;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6392_regulator_probe(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6392 = dev_get_drvdata(pdev->dev.parent);
> +	struct regulator_config config = {};
> +	struct regulator_dev *rdev;
> +	int i;
> +
> +	device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
> +
> +	// Initialize the bucks' vsel_reg and vsel_mask according to current HW state
> +	if (mt6392_set_buck_vsel_reg(pdev))
> +		return -EIO;
> +
> +	config.dev = mt6392->dev;
> +	config.regmap = mt6392->regmap;
> +	for (i = 0; i < MT6392_MAX_REGULATOR; i++) {
> +		config.driver_data = &mt6392_regulators[i];
> +
> +		rdev = devm_regulator_register(&pdev->dev,
> +					       &mt6392_regulators[i].desc,
> +					       &config);

[Severity: High]
Because the regulator core stores a pointer to the passed regulator_desc,
will passing the mutated shared descriptor here cause the first PMIC
instance to use the second PMIC's register offsets if a second instance
is probed? Could this result in writing to incorrect registers when changing
voltages?

> +		if (IS_ERR(rdev)) {
> +			dev_err(&pdev->dev, "failed to register %s\n",
> +				mt6392_regulators[i].desc.name);
> +			return PTR_ERR(rdev);
> +		}
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902193351.185771-1-l.scorcia@gmail.com?part=8