[net-next,3/4] fec: add FIXME to move 'mac_managed_pm' to probe

Commit Message

Wolfram Sang March 14, 2023, 1:14 p.m. UTC

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 <[e-mail]>
---
 drivers/net/ethernet/freescale/fec_main.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Wei Fang March 15, 2023, 5:48 a.m. UTC | #1

Hello Wolfram,

> -----Original Message-----
> From: Wolfram Sang <[e-mail]>
> Sent: 2023年3月14日 21:15
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; Wolfram Sang
> <[e-mail]>; Wei Fang <[e-mail]>;
> Shenwei Wang <[e-mail]>; Clark Wang
> <[e-mail]>; dl-linux-imx <[e-mail]>; David S.
> Miller <[e-mail]>; Eric Dumazet <[e-mail]>;
> Jakub Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>;
> [e-mail]
> 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 <[e-mail]>
> ---
>  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

Wolfram Sang March 15, 2023, 7:27 a.m. UTC | #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.

Wolfram Sang March 16, 2023, 8:02 a.m. UTC | #3

> 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

Wei Fang March 16, 2023, 9:10 a.m. UTC | #4

> -----Original Message-----
> From: Wolfram Sang <[e-mail]>
> Sent: 2023年3月16日 16:02
> To: Wei Fang <[e-mail]>; [e-mail];
> [e-mail]; Shenwei Wang <[e-mail]>; Clark
> Wang <[e-mail]>; dl-linux-imx <[e-mail]>; David S.
> Miller <[e-mail]>; Eric Dumazet <[e-mail]>;
> Jakub Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>;
> [e-mail]; [e-mail]
> 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.

Wolfram Sang March 16, 2023, 1:53 p.m. UTC | #5

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

Patch

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