[PATCH net 0/4] net: set 'mac_managed_pm' at probe time

[PATCH net 0/4] net: set 'mac_managed_pm' at probe time

[Wolfram Sang]

When suspending/resuming an interface which was not up, we saw mdiobus
related PM handling despite 'mac_managed_pm' being set for RAVB/SH_ETH.
Heiner kindly suggested the fix to set this flag at probe time, not at
init/open time. I implemented his suggestion and it works fine on these
two Renesas drivers. These are patches 1+2.

I then looked which other drivers could be affected by the same problem.
I could only identify two where I am quite sure. Because I don't have
any HW, I opted to at least add a FIXME and send this out as patches
3+4. Ideally, they will never need to go upstream because the relevant
people will see their patch and do something like I did for patches 1+2.

Looking forward to comments. Thanks and happy hacking!


Wolfram Sang (4):
  ravb: avoid PHY being resumed when interface is not up
  sh_eth: avoid PHY being resumed when interface is not up
  fec: add FIXME to move 'mac_managed_pm' to probe
  smsc911x: add FIXME to move 'mac_managed_pm' to probe

 drivers/net/ethernet/freescale/fec_main.c |  1 +
 drivers/net/ethernet/renesas/ravb_main.c  | 12 ++++++++++--
 drivers/net/ethernet/renesas/sh_eth.c     | 12 ++++++++++--
 drivers/net/ethernet/smsc/smsc911x.c      |  1 +
 4 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH net 1/4] ravb: avoid PHY being resumed when interface is not up

[Wolfram Sang]

RAVB doesn't need mdiobus suspend/resume, that's why it sets
'mac_managed_pm'. However, setting it needs to be moved from init to
probe, so mdiobus PM functions will really never be called (e.g. when
the interface is not up yet during suspend/resume).

Fixes: 4924c0cdce75 ("net: ravb: Fix PHY state warning splat during system resume")
Suggested-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 0f54849a3823..894e2690c643 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1455,8 +1455,6 @@ static int ravb_phy_init(struct net_device *ndev)
 		phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
 	}
 
-	/* Indicate that the MAC is responsible for managing PHY PM */
-	phydev->mac_managed_pm = true;
 	phy_attached_info(phydev);
 
 	return 0;
@@ -2379,6 +2377,8 @@ static int ravb_mdio_init(struct ravb_private *priv)
 {
 	struct platform_device *pdev = priv->pdev;
 	struct device *dev = &pdev->dev;
+	struct phy_device *phydev;
+	struct device_node *pn;
 	int error;
 
 	/* Bitbang init */
@@ -2400,6 +2400,14 @@ static int ravb_mdio_init(struct ravb_private *priv)
 	if (error)
 		goto out_free_bus;
 
+	pn = of_parse_phandle(dev->of_node, "phy-handle", 0);
+	phydev = of_phy_find_device(pn);
+	if (phydev) {
+		phydev->mac_managed_pm = true;
+		put_device(&phydev->mdio.dev);
+	}
+	of_node_put(pn);
+
 	return 0;
 
 out_free_bus:
-- 
2.30.2

[PATCH net 2/4] sh_eth: avoid PHY being resumed when interface is not up

[Wolfram Sang]

SH_ETH doesn't need mdiobus suspend/resume, that's why it sets
'mac_managed_pm'. However, setting it needs to be moved from init to
probe, so mdiobus PM functions will really never be called (e.g. when
the interface is not up yet during suspend/resume).

Fixes: 6a1dbfefdae4 ("net: sh_eth: Fix PHY state warning splat during system resume")
Suggested-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index ed17163d7811..d8ec729825be 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2029,8 +2029,6 @@ static int sh_eth_phy_init(struct net_device *ndev)
 	if (mdp->cd->register_type != SH_ETH_REG_GIGABIT)
 		phy_set_max_speed(phydev, SPEED_100);
 
-	/* Indicate that the MAC is responsible for managing PHY PM */
-	phydev->mac_managed_pm = true;
 	phy_attached_info(phydev);
 
 	return 0;
@@ -3097,6 +3095,8 @@ static int sh_mdio_init(struct sh_eth_private *mdp,
 	struct bb_info *bitbang;
 	struct platform_device *pdev = mdp->pdev;
 	struct device *dev = &mdp->pdev->dev;
+	struct phy_device *phydev;
+	struct device_node *pn;
 
 	/* create bit control struct for PHY */
 	bitbang = devm_kzalloc(dev, sizeof(struct bb_info), GFP_KERNEL);
@@ -3133,6 +3133,14 @@ static int sh_mdio_init(struct sh_eth_private *mdp,
 	if (ret)
 		goto out_free_bus;
 
+	pn = of_parse_phandle(dev->of_node, "phy-handle", 0);
+	phydev = of_phy_find_device(pn);
+	if (phydev) {
+		phydev->mac_managed_pm = true;
+		put_device(&phydev->mdio.dev);
+	}
+	of_node_put(pn);
+
 	return 0;
 
 out_free_bus:
-- 
2.30.2

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

[Wolfram Sang]

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 <wsa+renesas@sang-engineering.com>
---
 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

[PATCH net-next 4/4] smsc911x: add FIXME to move 'mac_managed_pm' to probe

[Wolfram Sang]

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 SMS911x 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 <wsa+renesas@sang-engineering.com>
---
 drivers/net/ethernet/smsc/smsc911x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index a2e511912e6a..745e0180eb34 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -1038,6 +1038,7 @@ static int smsc911x_mii_probe(struct net_device *dev)
 	}
 
 	/* Indicate that the MAC is responsible for managing PHY PM */
+	/* FIXME: should be set right after mdiobus is registered */
 	phydev->mac_managed_pm = true;
 	phy_attached_info(phydev);
 
-- 
2.30.2

Re: [PATCH net 0/4] net: set 'mac_managed_pm' at probe time

[Michal Kubiak]

On Tue, Mar 14, 2023 at 02:14:38PM +0100, Wolfram Sang wrote:
> When suspending/resuming an interface which was not up, we saw mdiobus
> related PM handling despite 'mac_managed_pm' being set for RAVB/SH_ETH.
> Heiner kindly suggested the fix to set this flag at probe time, not at
> init/open time. I implemented his suggestion and it works fine on these
> two Renesas drivers. These are patches 1+2.
> 
> I then looked which other drivers could be affected by the same problem.
> I could only identify two where I am quite sure. Because I don't have
> any HW, I opted to at least add a FIXME and send this out as patches
> 3+4. Ideally, they will never need to go upstream because the relevant
> people will see their patch and do something like I did for patches 1+2.
> 
> Looking forward to comments. Thanks and happy hacking!
> 
> 
> Wolfram Sang (4):
>   ravb: avoid PHY being resumed when interface is not up
>   sh_eth: avoid PHY being resumed when interface is not up
>   fec: add FIXME to move 'mac_managed_pm' to probe
>   smsc911x: add FIXME to move 'mac_managed_pm' to probe
> 
>  drivers/net/ethernet/freescale/fec_main.c |  1 +
>  drivers/net/ethernet/renesas/ravb_main.c  | 12 ++++++++++--
>  drivers/net/ethernet/renesas/sh_eth.c     | 12 ++++++++++--
>  drivers/net/ethernet/smsc/smsc911x.c      |  1 +
>  4 files changed, 22 insertions(+), 4 deletions(-)
> 
> -- 
> 2.30.2
> 

Unfortunately, I wasn't able to check the series in terms of content, but
I have no objections to the logic and coding style.

For the series.
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Thanks,
Michal

Re: [PATCH net 0/4] net: set 'mac_managed_pm' at probe time

[Simon Horman]

On Tue, Mar 14, 2023 at 02:14:38PM +0100, Wolfram Sang wrote:
> When suspending/resuming an interface which was not up, we saw mdiobus
> related PM handling despite 'mac_managed_pm' being set for RAVB/SH_ETH.
> Heiner kindly suggested the fix to set this flag at probe time, not at
> init/open time. I implemented his suggestion and it works fine on these
> two Renesas drivers. These are patches 1+2.
> 
> I then looked which other drivers could be affected by the same problem.
> I could only identify two where I am quite sure. Because I don't have
> any HW, I opted to at least add a FIXME and send this out as patches
> 3+4. Ideally, they will never need to go upstream because the relevant
> people will see their patch and do something like I did for patches 1+2.
> 
> Looking forward to comments. Thanks and happy hacking!

Hi Wolfram,

an initial comment is, and I know this is a bit of a pain,
that 'net' and 'net-next' patches need to be split into separate serries.

And if there are dependencies, the 'net' series should go first, and then
the 'net-next' series should go out once the 'net' patches have been
accepted and 'net' has been merged into 'net-next', which I believe occurs
after Linus has pulled the weekly 'net' pull request, which typically
occurs on Thursdays.

> Wolfram Sang (4):
>   ravb: avoid PHY being resumed when interface is not up
>   sh_eth: avoid PHY being resumed when interface is not up
>   fec: add FIXME to move 'mac_managed_pm' to probe
>   smsc911x: add FIXME to move 'mac_managed_pm' to probe
> 
>  drivers/net/ethernet/freescale/fec_main.c |  1 +
>  drivers/net/ethernet/renesas/ravb_main.c  | 12 ++++++++++--
>  drivers/net/ethernet/renesas/sh_eth.c     | 12 ++++++++++--
>  drivers/net/ethernet/smsc/smsc911x.c      |  1 +
>  4 files changed, 22 insertions(+), 4 deletions(-)
> 
> -- 
> 2.30.2
> 

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

[Wei Fang]

Hello Wolfram,

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: 2023年3月14日 21:15
> To: netdev@vger.kernel.org
> Cc: linux-renesas-soc@vger.kernel.org; kernel@pengutronix.de; Wolfram Sang
> <wsa+renesas@sang-engineering.com>; Wei Fang <wei.fang@nxp.com>;
> Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; David S.
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> linux-kernel@vger.kernel.org
> 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 <wsa+renesas@sang-engineering.com>
> ---
>  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

Re: [PATCH net 0/4] net: set 'mac_managed_pm' at probe time

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

Hi Simon!

> an initial comment is, and I know this is a bit of a pain,
> that 'net' and 'net-next' patches need to be split into separate serries.

Thanks for the heads up. I will refactor the series. First a part which
shall go into "net" and I will make the second part RFC with more
explanations how to reproduce the issue.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next 4/4] smsc911x: add FIXME to move 'mac_managed_pm' to probe

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Mar 14, 2023 at 2:30 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> 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 SMS911x 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.

You no longer have the APE6-EVM?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH net-next 4/4] smsc911x: add FIXME to move 'mac_managed_pm' to probe

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]


> > 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 SMS911x 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.
> 
> You no longer have the APE6-EVM?

I should have it somewhere. Cool, thanks for the pointer!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]


> 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wei Fang]

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: 2023年3月16日 16:02
> To: Wei Fang <wei.fang@nxp.com>; linux-renesas-soc@vger.kernel.org;
> kernel@pengutronix.de; Shenwei Wang <shenwei.wang@nxp.com>; Clark
> Wang <xiaoning.wang@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; David S.
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> linux-kernel@vger.kernel.org; netdev@vger.kernel.org
> 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.

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]


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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]