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

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

[Coia Prant]

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).

Fixes: f6bb3e9d98c2 ("net: pcs: xpcs: Add Synopsys DW xPCS platform device driver")
Cc: stable@vger.kernel.org
Signed-off-by: Coia Prant <coiaprant@gmail.com>
---
 drivers/net/pcs/pcs-xpcs-plat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/pcs/pcs-xpcs-plat.c b/drivers/net/pcs/pcs-xpcs-plat.c
index f4b1b8246ce96..fb80773379df5 100644
--- a/drivers/net/pcs/pcs-xpcs-plat.c
+++ b/drivers/net/pcs/pcs-xpcs-plat.c
@@ -285,7 +285,7 @@ static int xpcs_plat_init_clk(struct dw_xpcs_plat *pxpcs)
 		return dev_err_probe(dev, PTR_ERR(pxpcs->cclk),
 				     "Failed to get CSR clock\n");
 
-	pm_runtime_set_active(dev);
+	pm_runtime_set_suspended(dev);
 	ret = devm_pm_runtime_enable(dev);
 	if (ret) {
 		dev_err(dev, "Failed to enable runtime-PM\n");
-- 
2.47.3

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

[Jakub Kicinski]

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?

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@kernel.org> 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

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

[Jakub Kicinski]

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.

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

[Maxime Chevallier]

>> 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?

Avoiding link flaps/training is mostly a concern at boot time if the
bootloader set everything up beforehand, but I'd say for unbinding,
this should be OK and would mirror the probe.

Maxime

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

[Coia Prant]

On July 21, 2026 11:00:23 PM GMT+08:00, Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:
>
>>> 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?
>
>Avoiding link flaps/training is mostly a concern at boot time if the
>bootloader set everything up beforehand, but I'd say for unbinding,
>this should be OK and would mirror the probe.
>
>Maxime

Hi Maxime,

Thanks for the feedback on the .remove callback — I'll include it in
the next version.

One more question regarding PM dependency: the current pcs-xpcs-plat
driver uses runtime PM to manage the CSR clock, but the driver works
fine without PM only if the platform doesn't have a CSR clock at all.

On Rockchip, the CSR clock (PCLK_XPCS) is required for register access,
and without PM enabled, the clock is never prepared/enabled, so the
driver simply won't work.

That said, I think in practice, almost no one disables CONFIG_PM in
their kernel builds these days — it's enabled by default on most
architectures. So forcing the dependency (e.g., `select PM` or
`depends on PM`) wouldn't really hurt anyone, and it would ensure
the driver works correctly on platforms that do need the clock.

Do you think it's worth adding `select PM` in Kconfig for
pcs-xpcs-plat, or should we just leave it as-is since PM is almost
always already enabled anyway?

Thanks,
Coia