Re: [PATCH v8 2/4] clk: cix: add sky1 audss clock controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Clock operations access CRU registers without ensuring the device is runtime resumed, causing bus faults.
- [High] Exposing freed composite clocks via the OF clock provider due to inverted cleanup order.
- [High] Hardware reset and clocks leak when CONFIG_PM is disabled or synchronous suspend fails.
- [High] The driver uses a static global array to save register state during runtime suspend, causing cross-device state corruption.
- [Medium] Runtime PM usage count leak on the early probe error paths.
- [Medium] Missing reset assert in `runtime_resume` error path.
- [Low] Unconditional assignment of `dev_pm_ops` prevents dead code elimination when PM is disabled.
--

Note: Due to system recitation restrictions, this review uses a free-form summary format instead of inline patch quoting.

commit 515a69917f63979abc30478efe45a1239363fb83
Author: Joakim Zhang <joakim.zhang@cixtech.com>

clk: cix: add sky1 audss clock controller

This commit adds a platform driver for the Cix Sky1 AUDSS CRU, mapping
CRU registers and registering various clocks for the audio subsystem.

[Severity: High]
In sky1_audss_clk_remove(), the driver manually unregisters and frees the
composite clocks via clk_hw_unregister_composite(). However, the OF clock
provider and auxiliary reset device are registered using devres APIs, meaning
they will be cleaned up after the remove function returns.

Does this create a use-after-free window where the OF clock provider is still
accessible but returns freed clk_hw pointers? Could this also leave the reset
device bound while its parent is fully suspended?

[Severity: Critical]
At the end of probe, the device is put into synchronous suspend via
pm_runtime_put_sync(), which disables parent clocks and asserts the reset.
When clock framework callbacks like sky1_audss_clk_divider_recalc_rate() or
sky1_audss_clk_mux_get_parent() are later invoked, they perform regmap_read()
operations without first waking the device via pm_runtime_resume_and_get().

Since the regmap config doesn't set pm_runtime = true, will these unclocked
MMIO accesses trigger a synchronous external abort (bus fault) when reading
from the powered-down hardware?

[Severity: High]
The driver delegates the teardown of hardware resources entirely to PM runtime
functions. In the probe error path it uses pm_runtime_put_sync(), and in
remove it uses pm_runtime_force_suspend().

If the kernel is compiled with CONFIG_PM=n, these functions compile to empty
no-ops. Does this cause a permanent leak of the parent clocks and the
deasserted reset state if the probe fails or the driver is removed?

[Severity: Medium]
In the probe function, the PM usage counter is explicitly incremented using
pm_runtime_get_noresume(). If an early initialization step fails, the code
jumps to the err_pm label, which calls pm_runtime_disable() but does not
decrement the usage counter.

Should there be a call to pm_runtime_put_noidle() here to prevent the usage
counter from remaining permanently elevated?

[Severity: High]
The sky1_reg_save array is declared as a single static global array, but
during sky1_audss_clk_runtime_suspend(), the driver writes per-device hardware
register states back into this shared array.

If multiple instances of this driver are ever probed, won't they race and
overwrite each other's saved register states? Should this backup array be
allocated dynamically within the per-device sky1_audss_clks_priv structure
instead?

[Severity: Medium]
In sky1_audss_clk_runtime_resume(), the reset is first deasserted. If the
subsequent call to sky1_audss_clks_enable() fails, the function returns
the error immediately.

Does this error path need a call to reset_control_assert() to ensure the
hardware is actually left in the expected reset state when the resume fails?

[Severity: Low]
The sky1_audss_clk_pm_ops structure is unconditionally assigned to the driver
pm field.

To allow the compiler to eliminate dead code when power management is
disabled, should this assignment be wrapped with the pm_ptr() macro?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630124413.1814379-1-joakim.zhang@cixtech.com?part=2