Re: [PATCH RESEND 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:
- [Medium] regulator: mt6392: OF node reference leak in mt6392_regulator_probe()
--

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

regulator: Add MediaTek MT6392 regulator

This commit adds a regulator driver for the MediaTek MT6392 PMIC.

> 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
> @@ -0,0 +1,764 @@

[ ... ]

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

[Severity: Medium]
Does calling device_set_of_node_from_dev() here leak a reference on
pdev->dev.parent->of_node when the platform device is released?

In drivers/mfd/mt6397-core.c, the mt6392-regulator cell is defined without an
of_compatible string, so mfd_add_device() allocates the child device via
platform_device_alloc() with pdev->dev.of_node and pdev->dev.fwnode set to
NULL.

Calling device_set_of_node_from_dev() in mt6392_regulator_probe() increments
the reference count on pdev->dev.parent->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 later torn down, platform_device_release() only
puts 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);
	...
}

Since pdev->dev.fwnode remains NULL, fwnode_handle_put() is a no-op and the
reference on pdev->dev.of_node is never dropped.

Also, since config.dev is already set to mt6392->dev (the parent device),
which the regulator core uses for DT node and supply lookups, could the
device_set_of_node_from_dev() call simply be dropped (or replaced with
platform_device_set_of_node_from_dev() if needed)?

[ ... ]

> +	return 0;
> +}

[ ... ]

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