Re: [PATCH] net: pcs: xpcs-plat: fix runtime PM initialization

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 21 Jul 2026 09:53:53 +0800 Coia Prant wrote:
> I came across what looks like a runtime PM initialization issue while
> using pcs-xpcs-plat.c as a reference for the Rockchip XPCS glue driver
> (drivers/net/pcs/pcs-xpcs-rk.c).
> 
> The current code in pcs-xpcs-plat.c does:
> 
>     pm_runtime_set_active(dev);
>     ret = devm_pm_runtime_enable(dev);
> 
> This sets the initial PM state to ACTIVE before runtime PM is fully
> enabled, and before the clock is prepared and enabled.
> 
> If the device is later suspended (e.g., during unbind), the suspend
> callback may try to disable a clock that was never enabled, leading to:
> 
>     clk_csr already disabled
>     clk_csr already unprepared
> 
> On Rockchip platforms, the CSR clock (PCLK_XPCS) is required for register
> access, and this pattern seems problematic when a clock is actually
> provided.

Thanks for a clear explanation! A couple of sentences to this effect in
the commit msg could help backporters understand the severity of the
issue.

> I have a few questions:
> 
> 1. Is there a reason this hasn't been noticed before?
>    As far as I can tell, there is currently no mainline device tree
>    user that enables this driver on a platform with a real clock
>    dependency. Out-of-tree users might be using it without any clock
>    at all, or ACPI users might behave differently.

Ditto. I think we should drop the Fixes / CC: stable and instead
add a sentence that no upstream platform can currently trigger this.

> 2. Should we select PM in Kconfig and drop __maybe_unused from the
>    PM callbacks? Since this driver relies on runtime PM for clock
>    management, it seems odd to allow !PM builds.

I'm no PM expert but if it works for current users I don't see the need
to force the dependency?

> 3. Should we add a .remove callback to force suspend the device on
>    unbind? Otherwise the clock might remain enabled if the driver is
>    removed while active.

Embedded experts would have to chime in on this one. AFAIK this is a bit
of a gray area. Some users may want the link to stay up, eg to allow
WoL or avoid link training, maybe?

> I'm happy to send a follow-up patch addressing these points if you
> agree with the direction. Let me know what you think.