Re: [PATCH v3 2/5] clk: mediatek: Add mt8173-mfgtop driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Using asynchronous pm_runtime_put() in the probe error path races with devres pm_runtime_disable(), leaving the parent power domain permanently active.
- [Medium] Selecting PM_GENERIC_DOMAINS_OF without inheriting its OF dependency causes a Kconfig unmet direct dependency warning.
- [Medium] The OF node reference for the parent power domain is leaked when the driver is removed.
--

--- Patch [2]: [PATCH v3 2/5] clk: mediatek: Add mt8173-mfgtop driver ---
commit 508299c34f75893862a63235dcb101d72591479d
Author: Chen-Yu Tsai <wenst@chromium.org>

clk: mediatek: Add mt8173-mfgtop driver

The MFG (GPU) block on the MT8173 has a small glue layer, named MFG_TOP
in the datasheet, that contains clock gates, some power sequence signal
delays, and other unknown registers that get toggled when the GPU is
powered on.

> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> --- a/drivers/clk/mediatek/Kconfig
> +++ b/drivers/clk/mediatek/Kconfig
> @@ -537,6 +537,16 @@ config COMMON_CLK_MT8173_IMGSYS
>  	help
>  	  This driver supports MediaTek MT8173 imgsys clocks.
>  
> +config COMMON_CLK_MT8173_MFGTOP
> +	tristate "Clock and power driver for MediaTek MT8173 mfgtop"
> +	depends on COMMON_CLK_MT8173
> +	depends on PM
> +	default COMMON_CLK_MT8173
> +	select PM_GENERIC_DOMAINS
> +	select PM_GENERIC_DOMAINS_OF

[Severity: Medium]
Will this select cause an unmet dependency warning? Since
PM_GENERIC_DOMAINS_OF depends on OF, but COMMON_CLK_MT8173_MFGTOP
doesn't explicitly depend on OF, selecting it could force it on even
when its OF dependency isn't met (e.g., during COMPILE_TEST builds
without OF).

[ ... ]

> diff --git a/drivers/clk/mediatek/clk-mt8173-mfgtop.c b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
[ ... ]
> +static int clk_mt8173_mfgtop_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	data->child_pd.np = node;
> +	data->child_pd.args_count = 0;
> +	ret = of_parse_phandle_with_args(node, "power-domains", "#power-domain-cells", 0,
> +					 &data->parent_pd);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to parse power domain\n");
[ ... ]
> +put_26m_clk:
> +	clk_put(data->clk_26m);
> +unregister_clks:
> +	mtk_clk_unregister_gates(mfg_clks, ARRAY_SIZE(mfg_clks), data->clk_data);
> +put_pm_runtime:
> +	pm_runtime_put(dev);

[Severity: High]
Could this asynchronous pm_runtime_put() leave the device in an incorrect
power state if the probe fails? 

When probe returns an error, devres will call pm_runtime_disable() which
synchronously cancels any pending async suspend requests. Because the
suspend is cancelled, the device would remain in the RPM_ACTIVE state,
which may cause dev_pm_domain_detach() to fail with -EAGAIN. Should
this use pm_runtime_put_sync() instead?

> +put_of_node:
> +	of_node_put(data->parent_pd.np);
> +	return ret;
> +}
> +
> +static void clk_mt8173_mfgtop_remove(struct platform_device *pdev)
> +{
> +	struct mt8173_mfgtop_data *data = platform_get_drvdata(pdev);
> +	struct device_node *node = pdev->dev.of_node;
> +
> +	of_genpd_remove_subdomain(&data->parent_pd, &data->child_pd);
> +	of_genpd_del_provider(node);
> +	pm_genpd_remove(&data->genpd);
> +	of_clk_del_provider(node);
> +	clk_put(data->clk_26m);
> +	mtk_clk_unregister_gates(mfg_clks, ARRAY_SIZE(mfg_clks), data->clk_data);

[Severity: Medium]
Does this code leak the OF node reference?

In clk_mt8173_mfgtop_probe(), of_parse_phandle_with_args() acquires a
reference to the parent power domain OF node (data->parent_pd.np).
While this reference is released in the probe error paths, it doesn't
appear to be released here in the remove function. Should there be an
of_node_put(data->parent_pd.np) call here?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727091555.1023910-1-wenst@chromium.org?part=2