On Renesas hardware, we had issues because the above flag was set during
'open'. It was concluded that it needs to be set during 'probe'. It
looks like FEC needs the same fix but I can't test it because I don't
have the hardware. At least, leave a note about the issue.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/net/ethernet/freescale/fec_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index c73e25f8995e..b16f56208d66 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2318,6 +2318,7 @@ static int fec_enet_mii_probe(struct net_device *ndev)
fep->link = 0;
fep->full_duplex = 0;
+ /* FIXME: should be set right after mdiobus is registered */
phy_dev->mac_managed_pm = true;
phy_attached_info(phy_dev);
--
2.30.2
Hello Wolfram,
> -----Original Message-----
> From: Wolfram Sang <[email protected]>
> Sent: 2023??3??14?? 21:15
> To: [email protected]
> Cc: [email protected]; [email protected]; Wolfram Sang
> <[email protected]>; Wei Fang <[email protected]>;
> Shenwei Wang <[email protected]>; Clark Wang
> <[email protected]>; dl-linux-imx <[email protected]>; David S.
> Miller <[email protected]>; Eric Dumazet <[email protected]>;
> Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>;
> [email protected]
> Subject: [PATCH net-next 3/4] fec: add FIXME to move 'mac_managed_pm' to
> probe
>
> On Renesas hardware, we had issues because the above flag was set during
> 'open'. It was concluded that it needs to be set during 'probe'. It looks like FEC
> needs the same fix but I can't test it because I don't have the hardware. At
> least, leave a note about the issue.
>
Could you describe this issue in more details? So that I can reproduce and fix this
issue and test it. Thanks!
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/net/ethernet/freescale/fec_main.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index c73e25f8995e..b16f56208d66 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2318,6 +2318,7 @@ static int fec_enet_mii_probe(struct net_device
> *ndev)
> fep->link = 0;
> fep->full_duplex = 0;
>
> + /* FIXME: should be set right after mdiobus is registered */
> phy_dev->mac_managed_pm = true;
>
> phy_attached_info(phy_dev);
> --
> 2.30.2
> > On Renesas hardware, we had issues because the above flag was set during
> > 'open'. It was concluded that it needs to be set during 'probe'. It looks like FEC
> > needs the same fix but I can't test it because I don't have the hardware. At
> > least, leave a note about the issue.
> >
>
> Could you describe this issue in more details? So that I can reproduce and fix this
> issue and test it. Thanks!
Yes, I will resend the series as RFC with more explanations.
> Yes, I will resend the series as RFC with more explanations.
Because I was able to fix SMSC myself, I'll just describe the procedure
here:
1) apply this debug patch:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1b2e253fce75..7b79c5979486 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -310,6 +310,8 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
if (phydev->mac_managed_pm)
return 0;
+printk(KERN_INFO "****** MDIO suspend\n");
+
/* Wakeup interrupts may occur during the system sleep transition when
* the PHY is inaccessible. Set flag to postpone handling until the PHY
* has resumed. Wait for concurrent interrupt handler to complete.
2) boot the device without bringing the interface (and thus the PHY) up.
Bringing it down after it was up is not the same! It is important
that it was never up before.
3) do a suspend-to-ram/resume cycle
4) your log should show the above debug message. If not, I was wrong
5) If yes, apply a similar fix to the one I did for the Renesas drivers
in this series
6) suspend/resume should not show the debug message anymore
7) test for regressions and send out :)
I hope this was understandable. If not, feel free to ask.
Happy hacking,
Wolfram
> -----Original Message-----
> From: Wolfram Sang <[email protected]>
> Sent: 2023??3??16?? 16:02
> To: Wei Fang <[email protected]>; [email protected];
> [email protected]; Shenwei Wang <[email protected]>; Clark
> Wang <[email protected]>; dl-linux-imx <[email protected]>; David S.
> Miller <[email protected]>; Eric Dumazet <[email protected]>;
> Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH net-next 3/4] fec: add FIXME to move 'mac_managed_pm'
> to probe
>
>
> > Yes, I will resend the series as RFC with more explanations.
>
> Because I was able to fix SMSC myself, I'll just describe the procedure
> here:
>
> 1) apply this debug patch:
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index
> 1b2e253fce75..7b79c5979486 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -310,6 +310,8 @@ static __maybe_unused int
> mdio_bus_phy_suspend(struct device *dev)
> if (phydev->mac_managed_pm)
> return 0;
>
> +printk(KERN_INFO "****** MDIO suspend\n");
> +
> /* Wakeup interrupts may occur during the system sleep transition when
> * the PHY is inaccessible. Set flag to postpone handling until the PHY
> * has resumed. Wait for concurrent interrupt handler to complete.
>
> 2) boot the device without bringing the interface (and thus the PHY) up.
> Bringing it down after it was up is not the same! It is important
> that it was never up before.
>
> 3) do a suspend-to-ram/resume cycle
>
> 4) your log should show the above debug message. If not, I was wrong
>
> 5) If yes, apply a similar fix to the one I did for the Renesas drivers
> in this series
>
> 6) suspend/resume should not show the debug message anymore
>
> 7) test for regressions and send out :)
>
> I hope this was understandable. If not, feel free to ask.
>
> Happy hacking,
>
Thank you Wolfram. But I'm not sure whether it's really an issue. The flag
" mac_managed_pm" indicates that the MAC driver will take care of
suspending/resuming the PHY, that is to say the MAC driver calls
phy_stop()/phy_start() in its PM callbacks. If a ethernet interface never
brings up, the MAC PM callbacks do nothing and just return 0 directly. So I
think it's fine for the MDIO PM to do suspend/resume the PHY unless the MDIO
bus can't be accessed.
> Thank you Wolfram. But I'm not sure whether it's really an issue. The flag
> " mac_managed_pm" indicates that the MAC driver will take care of
> suspending/resuming the PHY, that is to say the MAC driver calls
> phy_stop()/phy_start() in its PM callbacks. If a ethernet interface never
> brings up, the MAC PM callbacks do nothing and just return 0 directly. So I
> think it's fine for the MDIO PM to do suspend/resume the PHY unless the MDIO
> bus can't be accessed.
I have one board here where accessing the MDIO bus times out, although I
don't really understand why. And another one where the call to
phy_init_hw() during resume causes issues. There might be more problems
with these boards, but I think it is cleaner to avoid any MDIO bus
suspend/resume if we have 'mac_managed_pm' anyway.