Re: [PATCH v4 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] Use-After-Free risk due to devm allocation of genpd combined with potentially failing pm_genpd_remove().
- [High] AB-BA deadlock between CCF prepare_lock and genpd_lock during runtime PM power on.
- [Low] Selecting PM_GENERIC_DOMAINS_OF without inheriting its dependencies causes Kconfig unmet dependency warnings.
--

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

clk: mediatek: Add mt8173-mfgtop driver

This commit introduces the mt8173-mfgtop driver to manage the MFG (GPU)
block's glue layer on the MT8173. It exposes the clock gates as a clock
controller and the power sequencing bits as a power domain.

> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> index 2c09fd729bab0..8eea3766a9108 100644
> --- 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: Low]
Does this select create an unmet dependency warning?

When building with COMPILE_TEST=y on architectures without OF enabled,
PM_GENERIC_DOMAINS_OF is forced on without its OF dependency being met.
Should this select be made conditional, or should the config depend on OF?

> +	help
> +	  This driver supports MediaTek MT8173 mfgtop clocks and power domain.

[ ... ]

> diff --git a/drivers/clk/mediatek/clk-mt8173-mfgtop.c b/drivers/clk/mediatek/clk-mt8173-mfgtop.c
> new file mode 100644
> index 0000000000000..5669ca9954dc0
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8173-mfgtop.c

[ ... ]

> +static int clk_mt8173_mfgtop_power_on(struct generic_pm_domain *domain)
> +{
> +	struct mt8173_mfgtop_data *data = container_of(domain, struct mt8173_mfgtop_data, genpd);
> +	int ret;
> +
> +	/* drives internal power management */
> +	ret = clk_prepare_enable(data->clk_26m);

[Severity: High]
Can this cause an AB-BA deadlock between the CCF prepare_lock and the
genpd_lock?

Because runtime PM is enabled before clock registration in probe, CCF will
manage runtime PM for clk_26m. Calling clk_prepare_enable() here acquires
the prepare_lock and then triggers a runtime PM resume, acquiring the
genpd_lock for mfg_async.

Concurrently, the provider (mtk-scpsys) holds the genpd_lock during its
power_on callback and calls clk_prepare_enable(), creating the reverse lock
order. Would moving devm_pm_runtime_enable() after clock registration avoid
this inversion?

> +	if (ret)
> +		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);

[Severity: High]
Could this lead to a use-after-free if the driver is unbound via sysfs?

The genpd structure is embedded within a devm-allocated data struct. If
pm_genpd_remove() fails and returns -EBUSY (e.g., if consumers are still
attached), the domain remains linked in the global gpd_list.

Because the error is ignored, devres will proceed to free the data
structure, leaving a dangling pointer in the global list.

> +	of_clk_del_provider(node);
> +	clk_put(data->clk_26m);
> +	mtk_clk_unregister_gates(mfg_clks, ARRAY_SIZE(mfg_clks), data->clk_data);
> +	of_node_put(data->parent_pd.np);
> +}

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