Re: [PATCH v12 10/11] clk: realtek: Add RTD1625-ISO clock controller driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The `.suppress_bind_attrs = true` flag was not removed, contradicting the v12 changelog explicitly claiming its removal.

Pre-existing issues:
- [High] Race condition during module teardown leading to NULL pointer dereference in clock operations.
--

--- Patch [10]: [PATCH v12 10/11] clk: realtek: Add RTD1625-ISO clock controller driver ---
commit 5805b8768a6198942f7b7e93baa5ff2d182a3863
Author: Cheng-Yu Lee <[email protected]>

clk: realtek: Add RTD1625-ISO clock controller driver

Add support for the ISO (Isolation) domain clock controller on the Realtek
RTD1625 SoC. This controller manages clocks in the always-on power domain,
ensuring essential services remain functional even when the main system
power is gated.

Because the reset controller shares the same register space with this ISO
clock controller, this driver also acts as the parent device and registers
the reset controller as an auxiliary device on the auxiliary bus.

> ---
> Changes in v12:
> - Replace of_device_get_match_data() with device_get_match_data().
> - Use module_platform_driver().
> - Add spaces near {}.
> - Remove .suppress_bind_attrs and add .remove callback.

[Severity: Medium]
This isn't a bug, but the v12 changelog mentions removing
.suppress_bind_attrs, yet it is still present in the driver definition below.
Was this accidentally left in during a rebase?

[ ... ]

> diff --git a/drivers/clk/realtek/clk-rtd1625-iso.c b/drivers/clk/realtek/clk-rtd1625-iso.c
> new file mode 100644
> index 0000000000000..6025c148adf83
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-rtd1625-iso.c
> @@ -0,0 +1,142 @@

[ ... ]

> +static struct platform_driver rtd1625_iso_driver = {
> +	.probe = rtd1625_iso_probe,
> +	.remove = rtk_clk_remove,

[Severity: High]
This is a pre-existing issue, but does setting the .remove callback here
expose a race condition leading to a NULL pointer dereference during module
unload?

In rtk_clk_probe(), clocks are registered using devm_clk_hw_register(),
meaning they are not deregistered from the Common Clock Framework until after
the .remove callback completes.

However, rtk_clk_remove() manually clears the regmap pointer before devres
has a chance to deregister the clocks:

drivers/clk/realtek/clk-rtk-common.c:rtk_clk_remove() {
    ...
	for (int i = 0; i < desc->num_clks; i++)
		desc->clks[i]->regmap = NULL;
    ...
}

If a concurrent operation (like reading /sys/kernel/debug/clk/clk_summary or
a consumer calling clk_enable) accesses the clock ops during this window,
would it dereference the NULL regmap pointer inside regmap_read()?

> +	.driver = {
> +		.name = "rtk-rtd1625-iso-clk",
> +		.of_match_table = rtd1625_iso_match,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +
> +module_platform_driver(rtd1625_iso_driver);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10