Re: [PATCH v2 07/14] clk: samsung: exynos8855: add block top and peric clock support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: samsung: missing CLK_IS_CRITICAL flag on critical clocks causes system hang
- [High] clk: samsung: platform_driver_register() used with __init probe function
--

commit a53c705d96b4a86686927f8b6b05c07feca713d6
Author: Raghav Sharma <raghav.s@samsung.com>

clk: samsung: exynos8855: add block top and peric clock support

This commit registers the compatible and cmu_info data to support CMU_TOP
and CMU_PERIC clocks for the Exynos 8855 SoC.

> diff --git a/drivers/clk/samsung/clk-exynos8855.c b/drivers/clk/samsung/clk-exynos8855.c
> new file mode 100644
> index 0000000000000..5394706d214c9
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos8855.c

[ ... ]

> +static const struct samsung_gate_clock peric_gate_clks[] __initconst = {
> +	/* System will hang if this critical clock is gated */
> +	GATE(CLK_GOUT_CMU_PERIC, "gout_cmu_peric",
> +	     "dout_clkcmu_peric_nocp",
> +	     CMU_PERIC_CLK_CON_GAT_BLK_PERIC_UID_CMU_PERIC_IPCLKPORT_PCLK,
> +	     21, 0, 0),

[Severity: High]
Does this code inadvertently allow the clock to be gated despite the comment?

The GATE macro passes 0 as the 6th argument (flags). During the late
initialization phase, the Common Clock Framework's clk_disable_unused()
routine automatically disables all clocks that do not have active consumers.
Without the CLK_IS_CRITICAL or CLK_IGNORE_UNUSED flag, couldn't this cause
the exact system hang the comment warns about?

> +	GATE(CLK_GOUT_SYSREG_PERIC, "gout_sysreg_peric",
> +	     "dout_clkcmu_peric_nocp",
> +	     CMU_PERIC_CLK_CON_GAT_BLK_PERIC_UID_SYSREG_PERIC_IPCLKPORT_PCLK,
> +	     21, 0, 0),
> +	/* System will hang if this critical clock is gated */
> +	GATE(CLK_GOUT_UART_DBG_PERIC, "gout_uart_dbg_peric",
> +	     "dout_clkcmu_peric_uart_dbg",
> +	     CMU_PERIC_CLK_CON_GAT_BLK_PERIC_UID_UART_DBG_IPCLKPORT_PCLK,
> +	     21, 0, 0),

[Severity: High]
Similarly, could this clock also be disabled by the framework since it lacks
the CLK_IS_CRITICAL flag?

[ ... ]

> +static int __init exynos8855_cmu_probe(struct platform_device *pdev)
> +{
> +	const struct samsung_cmu_info *info;
> +	struct device *dev = &pdev->dev;
> +
> +	info = of_device_get_match_data(dev);
> +	exynos_arm64_register_cmu(dev, dev->of_node, info);
> +
> +	return 0;
> +}

[ ... ]

> +static struct platform_driver exynos8855_cmu_driver __refdata = {
> +	.driver = {
> +		.name = "exynos8855-cmu",
> +		.of_match_table = exynos8855_cmu_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = exynos8855_cmu_probe,
> +};
> +
> +static int __init exynos8855_cmu_init(void)
> +{
> +	return platform_driver_register(&exynos8855_cmu_driver);
> +}

[Severity: High]
Can this cause a use-after-free regression if the driver is probed after boot?

Because exynos8855_cmu_probe() is marked __init, its memory is freed after
the early boot phase. The driver uses platform_driver_register() which does
not clear the .probe pointer, unlike platform_driver_probe().

Even with suppress_bind_attrs = true preventing sysfs binding, couldn't a
late device instantiation (such as through a device tree overlay) trigger a
call to the reclaimed exynos8855_cmu_probe() memory? Also, the __refdata
annotation on the driver struct hides the section mismatch warning rather
than fixing the underlying lifetime issue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009120137.2801911-1-raghav.s@samsung.com?part=7