Re: [PATCH v2 4/6] pmdomain: renesas: Add R-Car X5H MDLC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Hardcoded index `0` in `of_parse_phandle_with_args()` breaks consumer devices attached to multiple power domains.
- [High] Use-After-Free of `priv` due to premature release of `r8a78000_mdlc_lock`.
- [High] AB-BA deadlock (lockdep splat) between `r8a78000_mdlc_lock` and the global `gpd_list_lock`.
- [Medium] Missing dummy clocks despite claims in the commit message.
--

--- Patch [4]: [PATCH v2 4/6] pmdomain: renesas: Add R-Car X5H MDLC driver ---
commit 1970ea51b8c9b00b6533ff3db2920ecb99d83f17
Author: Geert Uytterhoeven <geert+renesas@glider.be>

pmdomain: renesas: Add R-Car X5H MDLC driver

This adds a minimal Module Controller driver for the R-Car X5H (R8A78000)
SoC. It provides support for always-on power domains and dummy resets for
the serial console.

> For now this just supports the always-on power domains, and dummy clocks
> and resets for the serial console (which is enabled by the boot loader).

[Severity: Medium]
The commit message mentions adding dummy clocks, but looking at
r8a78000_mdlc_probe(), it appears only the genpd and reset controller
providers are registered. Will consumers defer indefinitely if they
attempt to use this driver as a clock provider?

> diff --git a/drivers/pmdomain/renesas/r8a78000-mdlc.c b/drivers/pmdomain/renesas/r8a78000-mdlc.c
> new file mode 100644
> index 0000000000000..ed367e921a334
> --- /dev/null
> +++ b/drivers/pmdomain/renesas/r8a78000-mdlc.c
[ ... ]
> +static int r8a78000_mdlc_attach_dev(struct generic_pm_domain *domain,
> +				    struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct r8a78000_mdlc_priv *priv;
> +	struct of_phandle_args pd_spec;
> +	const struct mod_map *map;
> +	unsigned int id;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_args(np, "power-domains",
> +					 "#power-domain-cells", 0, &pd_spec);

[Severity: High]
Is it possible for a consumer device to have multiple power domains
configured in its device tree (e.g., <&sysc 1>, <&mdlc 2>)? If the
MDLC domain is not the first one in the property, won't this hardcoded
index of 0 fail to find the correct provider and cause the consumer
probe to fail with -ENODEV?

> +	if (ret < 0)
> +		return ret;
> +
> +	if (pd_spec.args_count != 2) {
> +		of_node_put(pd_spec.np);
> +		return -EINVAL;
> +	}
> +
> +	scoped_guard(mutex, &r8a78000_mdlc_lock) {
> +		hlist_for_each_entry(priv, &r8a78000_mdlc_list, link) {
> +			if (priv->np == pd_spec.np)
> +				break;
> +		}
> +	}
> +
> +	if (!priv) {
> +		dev_err(dev, "%s: MDLC %pOF not found\n", __func__, pd_spec.np);
> +		of_node_put(pd_spec.np);
> +		return -ENODEV;
> +	}
> +
> +	id = pd_spec.args[1];
> +	of_node_put(pd_spec.np);
> +
> +	map = mod_map_find(priv->mod_map, id);

[Severity: High]
Could priv be freed before we dereference it here? The lock protecting
the r8a78000_mdlc_list is released at the end of the scoped_guard() block
above.

If the provider's probe fails or the driver is unbound concurrently,
r8a78000_mdlc_unlink() will acquire the lock, remove priv from the list,
and free the memory. Since we access priv->mod_map here without holding
the lock, does this introduce a use-after-free window?

> +	if (!map) {
> +		dev_err(dev, "Unknown module 0x%x\n", id);
> +		return -ENOENT;
> +	}
[ ... ]
> +static int r8a78000_genpd_always_on_singleton(struct device *dev)
> +{
> +	struct generic_pm_domain *genpd;
> +	int ret;
> +
> +	guard(mutex)(&r8a78000_mdlc_lock);
> +
> +	if (r8a78000_genpd_always_on)
> +		return 0;
> +
> +	genpd = kzalloc_obj(*genpd);
> +	if (!genpd)
> +		return -ENOMEM;
> +
> +	genpd->name = "always-on";
> +	genpd->attach_dev = r8a78000_mdlc_attach_dev;
> +
> +	ret = pm_genpd_init(genpd, &pm_domain_always_on_gov, false);

[Severity: High]
Will this sequence trigger a lockdep splat?

During this first probe, r8a78000_genpd_always_on_singleton() acquires
r8a78000_mdlc_lock and calls pm_genpd_init(), which acquires the global
gpd_list_lock. This establishes r8a78000_mdlc_lock -> gpd_list_lock.

Later, when a consumer device attaches, the PM domain core calls
r8a78000_mdlc_attach_dev() with gpd_list_lock held. That function then
acquires r8a78000_mdlc_lock via the scoped_guard() block, completing an
inverse lock order.

Should pm_genpd_init() be called outside the r8a78000_mdlc_lock critical
section to avoid this?

> +	if (ret) {
> +		kfree(genpd);
> +		return dev_err_probe(dev, ret,
> +				     "Failed to create always-on domain\n");
> +	}
> +
> +	r8a78000_genpd_always_on = genpd;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783505142.git.geert+renesas@glider.be?part=4