Re: [PATCH v12 2/5] regulator: Add support for MediaTek MT6373 SPMI PMIC Regulators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Global state mutation of `mt6373_regulators[]` array during probe.
- [High] Missing cancellation of delayed work (`info->oc_work`) on driver removal.
- [High] Out-of-bounds stack read in `mt6373_buck_unlock` due to incorrect `val_count`.
- [High] Typo in register definitions causes `VMCH_EINT` to corrupt `VMC` calibration.
- [High] `VMCH_EINT` bypasses the regulator core and forcibly disables the `VMCH` parent regulator.
- [High] Duplicate IRQ mapping and double-free due to unconditionally mapping `hwirq` 0 and 25.
- [High] Double free of the IRQ mapping in the `devm_add_action_or_reset()` error path.
- [High] NULL pointer dereference when retrieving `fwspec.fwnode`.
- [High] Concurrent mode transitions across different buck regulators interleave and fail.
- [High] State machine logic error in `mt6373_regulator_set_mode()` traps regulators in the wrong state.
- [Medium] Discrepancy between commit message claims and implemented regulators.
--

commit 840d62ea908fb428a74ce6667830cc377258169f
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

regulator: Add support for MediaTek MT6373 SPMI PMIC Regulators

Add a driver for the regulators found on the MediaTek MT6373 PMIC, fully
controlled by SPMI interface.

> Similarly to MT6363, this PMIC regulates voltage with input range
> of 2.6-5.0V, and features 10 buck converters and 25 LDOs.

This isn't a bug, but the commit message states the PMIC features "25 LDOs",
yet the mt6373_regulators array only implements 23 LDOs.

> diff --git a/drivers/regulator/mt6373-regulator.c b/drivers/regulator/mt6373-regulator.c
> index 0000000000000..90672ae1eb80c
> --- /dev/null
> +++ b/drivers/regulator/mt6373-regulator.c

[ ... ]

> @@ -215,22 +215,16 @@ static unsigned int mt6373_map_mode(unsigned int mode)
>  	}
>  }
>  
>  static int mt6373_vmch_eint_disable(struct regulator_dev *rdev)
>  {
>  	const struct regulator_desc *rdesc = rdev->desc;
>  	int ret;
>  
>  	ret = regmap_clear_bits(rdev->regmap,
>  				MT6373_PMIC_RG_LDO_VMCH_ADDR,
>  				rdesc->enable_mask);

If another consumer has legitimately enabled VMCH via the regulator core,
does clearing the enable mask of MT6373_PMIC_RG_LDO_VMCH_ADDR forcibly
turn off the underlying hardware and break the regulator framework's reference
counting?

[ ... ]

> @@ -267,11 +274,8 @@ static int mt6373_buck_unlock(struct regmap *map, bool unlock)
>  {
>  	u16 buf = unlock ? MT6373_BUCK_TOP_UNLOCK_VALUE : 0;
>  
>  	return regmap_bulk_write(map, MT6373_BUCK_TOP_KEY_PROT_LO, &buf, sizeof(buf));
>  }

For a regmap configured with val_bits = 16, val_count represents the number
of 16-bit registers to write. Since sizeof(buf) is 2, does passing 2 instruct
regmap to read 4 bytes starting from &buf, causing an out-of-bounds read of
the kernel stack and writing uninitialized stack data to the PMIC hardware
register?

>  static int mt6373_regulator_set_mode(struct regulator_dev *rdev,
>  				     unsigned int mode)
>  {

[ ... ]

>  	switch (mode) {
>  	case REGULATOR_MODE_FAST:
>  		ret = mt6373_buck_unlock(regmap, true);
>  		if (ret)
>  			break;
>  
>  		ret = regmap_set_bits(regmap, info->modeset_reg, info->modeset_mask);
>  
>  		mt6373_buck_unlock(regmap, false);
>  		break;

Could concurrent mode transitions across different buck regulators interleave?

mt6373_buck_unlock writes to MT6373_BUCK_TOP_KEY_PROT_LO, which is a global
PMIC hardware lock. Since this function is only protected by the per-regulator
rdev->mutex, could one thread silently fail to write its mode setting if
another thread locks the hardware prematurely?

>  	case REGULATOR_MODE_NORMAL:
>  		cur_mode = mt6373_regulator_get_mode(rdev);
>  		if (cur_mode < 0) {
>  			ret = cur_mode;
>  			break;
>  		}
>  
>  		if (cur_mode == REGULATOR_MODE_FAST) {
>  			ret = mt6373_buck_unlock(regmap, true);
>  			if (ret)
>  				break;
>  
>  			ret = regmap_clear_bits(regmap, info->modeset_reg, info->modeset_mask);
>  
>  			mt6373_buck_unlock(regmap, false);
>  			break;

If LP was previously set when returning from FAST to NORMAL mode, does the
regulator unexpectedly drop into IDLE mode because the LP bit is not cleared?

>  		} else if (cur_mode == REGULATOR_MODE_IDLE) {
>  			ret = regmap_clear_bits(regmap, info->lp_mode_reg, info->lp_mode_mask);
>  			if (ret == 0)
>  				usleep_range(100, 200);
>  		} else {
>  			ret = 0;
>  		}
>  		break;
>  	case REGULATOR_MODE_IDLE:
>  		ret = regmap_set_bits(regmap, info->lp_mode_reg, info->lp_mode_mask);
>  		break;

If transitioning to REGULATOR_MODE_IDLE, does the driver leave the FCCM (FAST)
bit set, keeping the hardware stuck in FAST mode?

[ ... ]

> @@ -546,80 +541,75 @@ static int mt6373_regulator_probe(struct platform_device *pdev)
>  {

[ ... ]

>  	interrupt_parent = of_irq_find_parent(dev->of_node);
>  	if (!interrupt_parent)
>  		return -EINVAL;
>  
>  	domain = irq_find_host(interrupt_parent);
>  	of_node_put(interrupt_parent);
>  	fwspec.fwnode = domain->fwnode;

If the interrupt controller has not yet probed and irq_find_host returns NULL,
will this unconditionally dereference a NULL pointer for domain->fwnode
instead of gracefully handling the missing dependency with -EPROBE_DEFER?

[ ... ]

>  	for (i = 0; i < ARRAY_SIZE(mt6373_regulators); i++) {
>  		info = &mt6373_regulators[i];
>  
>  		/* MT6373CW VBUCK4 constraints are different */
>  		if (info->desc.id == MT6373_ID_VBUCK4) {
>  			unsigned int vbuck4_max_uV;
>  
>  			/* VBUCK4 vreg software control not allowed in hw_ctrl mode */
>  			if (is_vbuck4_hw_ctrl)
>  				continue;
>  
>  			if (is_cw_variant) {
>  				info->desc.uV_step = 6250;
>  				vbuck4_max_uV = 1193750;
>  			} else {
>  				info->desc.uV_step = 13875;
>  				vbuck4_max_uV = 2650125;
>  			}
>  			info->desc.n_voltages = vbuck4_max_uV / info->desc.uV_step + 1;
>  		}
>  
>  		fwspec.param[0] = to_spmi_device(dev->parent)->usid;
>  		fwspec.param[1] = info->hwirq;
>  		info->virq = irq_create_fwspec_mapping(&fwspec);

Does this modify the globally shared mt6373_regulators array directly during
probe?

If multiple MT6373 PMIC instances are present or the driver is unbound and
rebound, would this overwrite shared state, potentially causing workqueue
corruption and devres double-freeing of overwritten IRQ mappings?

Furthermore, since several regulators in mt6373_regulators share the same
hwirq (such as 0 or 25), will irq_create_fwspec_mapping return the same virq
multiple times?

>  		if (!info->virq)
>  			return dev_err_probe(dev, -EINVAL,
>  					     "Failed to map IRQ%d\n", info->hwirq);
>  
>  		ret = devm_add_action_or_reset(dev, mt6373_irq_remove, &info->virq);

Would registering mt6373_irq_remove via devm_add_action_or_reset for each
regulator cause irq_dispose_mapping to be called multiple times on the same
virq during teardown, resulting in a use-after-free of the IRQ descriptor?

>  		if (ret) {
>  			irq_dispose_mapping(info->virq);
>  			return ret;
>  		}

If devm_add_action_or_reset fails, it automatically executes the cleanup
action, which calls irq_dispose_mapping. Does calling irq_dispose_mapping
here result in a direct double-free?

[ ... ]

>  static struct platform_driver mt6373_regulator_driver = {
>  	.driver = {
>  		.name = "mt6373-regulator",
>  		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>  		.of_match_table = mt6373_regulator_match,
>  	},
>  	.probe = mt6373_regulator_probe,
>  	.shutdown = mt6373_regulator_shutdown
>  };

Is there a mechanism to cancel the info->oc_work delayed work on driver
removal?

If the device is unbound or the module is unloaded while the work is pending,
would the workqueue attempt to execute mt6373_oc_irq_enable_work after the
module's memory has been freed, causing a use-after-free?

> diff --git a/include/linux/regulator/mt6373-regulator.h b/include/linux/regulator/mt6373-regulator.h
> index 0000000000000..dd791717d2a11
> --- /dev/null
> +++ b/include/linux/regulator/mt6373-regulator.h

[ ... ]

> @@ -118,6 +118,8 @@
>  #define MT6373_PMIC_RG_VCN33_3_VOSEL_ADDR	0x1de9
>  #define MT6373_PMIC_RG_VMCH_VOCAL_ADDR		0x1dec
>  #define MT6373_PMIC_RG_VMCH_VOSEL_ADDR		0x1ded
>  #define MT6373_PMIC_RG_VMCH_EINT_HIGH_VOSEL_ADDR MT6373_PMIC_RG_VMCH_VOSEL_ADDR
>  #define MT6373_PMIC_RG_VMCH_EINT_LOW_VOSEL_ADDR	MT6373_PMIC_RG_VMCH_VOSEL_ADDR
>  #define MT6373_PMIC_RG_VEFUSE_VOCAL_ADDR	0x1df0
>  #define MT6373_PMIC_RG_VEFUSE_VOSEL_ADDR	0x1df1
>  #define MT6373_PMIC_RG_VMC_VOCAL_ADDR		0x1df4
>  #define MT6373_PMIC_RG_VMCH_EINT_HIGH_VOCAL_ADDR MT6373_PMIC_RG_VMC_VOCAL_ADDR
>  #define MT6373_PMIC_RG_VMCH_EINT_LOW_VOCAL_ADDR	MT6373_PMIC_RG_VMC_VOCAL_ADDR

Is it intentional that these are defined as MT6373_PMIC_RG_VMC_VOCAL_ADDR
instead of MT6373_PMIC_RG_VMCH_VOCAL_ADDR? When voltage selection is updated
for vmch-eint-high or vmch-eint-low, does it inadvertently overwrite the
voltage calibration register of the entirely separate VMC regulator?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511101355.122478-1-angelogioacchino.delregno@collabora.com?part=2