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

[Coia Prant]

On July 21, 2026 8:24:33 AM GMT+08:00, Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>On Sun,  5 Jul 2026 05:48:08 +0800 Coia Prant wrote:
>> The driver calls `pm_runtime_set_active()` before runtime PM is enabled,
>> and before the clock is prepared and enabled.
>> 
>> This causes the clock to be unprepared/disabled later in the suspend
>> callback even though it was never prepared/enabled, resulting in warnings:
>> 
>> clk_csr already disabled
>> clk_csr already unprepared
>> 
>> Fix this by setting the initial runtime PM status to SUSPENDED instead
>> of ACTIVE.
>> 
>> The clock will be properly enabled when the device is first resumed
>> via runtime PM (e.g., during MDIO access).
>
>Seems a bit odd that this hasn't been discovered until now.
>Could you add more details about your platform and maybe
>a hypothesis why we haven't noticed?

Hi,

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.

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.

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.

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.

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

Thanks,
Coia