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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] regulator: mt6392: OF node leak in mt6392_regulator_probe()
- [Low] regulator: mt6392: malformed kernel-doc comment on mt6392_regulator_info
--

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

regulator: Add MediaTek MT6392 regulator

This commit adds a regulator driver for the MediaTek MT6392 power management
integrated circuit, supporting buck and linear regulators controlled over
regmap.

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

[ ... ]

> +/**
> + * MT6392 regulators' information
> + *
> + * @desc: standard fields of regulator description.
> + * @qi_status_reg: Register to query enable signal status of regulators
> + * @qi_status_mask: Mask to query enable signal status of regulators (RO)
> + * @vselctrl_reg: Vsel control mode selector register
> + * @vselctrl_mask: Vsel control mode selector mask (RO)
> + * @vsel_reg_mode_reg: Vsel register when Vsel control mode selector = 0 (Register mode)
> + * @vsel_reg_mode_mask: Vsel register mask in Register mode (RW)
> + * @vsel_normal_mode_reg: Vsel register when Vsel control mode selector = 1 (Normal mode)
> + * @vsel_normal_mode_mask: Vsel register mask in Register mode (RW)
> + * @pwm_modeset_reg: Register to control buck mode (Auto/Force PWM)
> + * @pwm_modeset_mask: Mask to control buck mode (RW)
> + * @lp_modeget_reg: Register to get LDO low-power mode
> + * @lp_modeget_mask: Mask to get LDO low-power mode (RO)
> + * @lp_modeset_reg: Register to control LDO low-power mode
> + * @lp_modeset_mask: Mask to control LDO low-power mode (WO)
> + */
> +struct mt6392_regulator_info {

[Severity: Low]
Because this comment above struct mt6392_regulator_info in
drivers/regulator/mt6392-regulator.c starts with /**, scripts/kernel-doc
parses it during W=1 builds and warns that the opening line does not match
"struct mt6392_regulator_info - ...".

Also, @vsel_normal_mode_mask is described as "Register mode" rather than
"Normal mode".

Should the header line be formatted as a kernel-doc struct header and the
@vsel_normal_mode_mask description updated?

[ ... ]

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

[Severity: Medium]
Does calling device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent) in
mt6392_regulator_probe() in drivers/regulator/mt6392-regulator.c leak a
reference to pdev->dev.parent->of_node when the platform device is released
(for example, if mt6392_set_buck_vsel_reg() or devm_regulator_register()
fails, or when the parent device is unbound)?

In drivers/mfd/mt6397-core.c, the regulator child is defined with
MFD_CELL_NAME("mt6392-regulator"), so mfd_add_device() allocates the child
platform_device with pdev->dev.of_node and pdev->dev.fwnode both set to
null.

Calling device_set_of_node_from_dev() increments the reference count on
dev2->of_node and assigns it to pdev->dev.of_node without setting
pdev->dev.fwnode:

drivers/base/core.c:device_set_of_node_from_dev() {
	of_node_put(dev->of_node);
	dev->of_node = of_node_get(dev2->of_node);
	dev_set_of_node_reused(dev);
}

When the platform device is destroyed, platform_device_release() only drops
the reference on pa->pdev.dev.fwnode:

drivers/base/platform.c:platform_device_release() {
	...
	if (!is_software_node(dev_fwnode(dev)))
		fwnode_handle_put(pa->pdev.dev.fwnode);
	...
}

Because pdev->dev.fwnode is still null, the reference held by
pdev->dev.of_node is never dropped (unlike
platform_device_set_of_node_from_dev(), which also assigns
pdev->dev.fwnode = of_fwnode_handle(pdev->dev.of_node)).

Since mt6392_regulator_probe() already sets config.dev = mt6392->dev for
regulator_register() to look up the regulators node and supplies on the
parent device, should device_set_of_node_from_dev() be removed here (or
replaced with platform_device_set_of_node_from_dev())?

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