[PATCH net-next 0/3] net: make PHY PM ops a no-op if MAC driver manages PHY PM

[PATCH net-next 0/3] net: make PHY PM ops a no-op if MAC driver manages PHY PM

[Heiner Kallweit]

Resume callback of the PHY driver is called after the one for the MAC
driver. The PHY driver resume callback calls phy_init_hw(), and this is
potentially problematic if the MAC driver calls phy_start() in its resume
callback. One issue was reported with the fec driver and a KSZ8081 PHY
which seems to become unstable if a soft reset is triggered during aneg.

The new flag allows MAC drivers to indicate that they take care of
suspending/resuming the PHY. Then the MAC PM callbacks can handle
any dependency between MAC and PHY PM.

Heiner Kallweit (3):
  net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM
  net: fec: use mac-managed PHY PM
  r8169: use mac-managed PHY PM

 drivers/net/ethernet/freescale/fec_main.c | 3 +++
 drivers/net/ethernet/realtek/r8169_main.c | 3 +++
 drivers/net/phy/phy_device.c              | 6 ++++++
 include/linux/phy.h                       | 2 ++
 4 files changed, 14 insertions(+)

-- 
2.31.1

[PATCH net-next 1/3] net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM

[Heiner Kallweit]

Resume callback of the PHY driver is called after the one for the MAC
driver. The PHY driver resume callback calls phy_init_hw(), and this is
potentially problematic if the MAC driver calls phy_start() in its resume
callback. One issue was reported with the fec driver and a KSZ8081 PHY
which seems to become unstable if a soft reset is triggered during aneg.

The new flag allows MAC drivers to indicate that they take care of
suspending/resuming the PHY. Then the MAC PM callbacks can handle
any dependency between MAC and PHY PM.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 6 ++++++
 include/linux/phy.h          | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a009d1769..73d29fd5e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -273,6 +273,9 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
 
+	if (phydev->mac_managed_pm)
+		return 0;
+
 	/* We must stop the state machine manually, otherwise it stops out of
 	 * control, possibly with the phydev->lock held. Upon resume, netdev
 	 * may call phy routines that try to grab the same lock, and that may
@@ -294,6 +297,9 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 	struct phy_device *phydev = to_phy_device(dev);
 	int ret;
 
+	if (phydev->mac_managed_pm)
+		return 0;
+
 	if (!phydev->suspended_by_mdio_bus)
 		goto no_resume;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 8e2cf84b2..98fb441dd 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -493,6 +493,7 @@ struct macsec_ops;
  * @loopback_enabled: Set true if this PHY has been loopbacked successfully.
  * @downshifted_rate: Set true if link speed has been downshifted.
  * @is_on_sfp_module: Set true if PHY is located on an SFP module.
+ * @mac_managed_pm: Set true if MAC driver takes of suspending/resuming PHY
  * @state: State of the PHY for management purposes
  * @dev_flags: Device-specific flags used by the PHY driver.
  * @irq: IRQ number of the PHY's interrupt (-1 if none)
@@ -567,6 +568,7 @@ struct phy_device {
 	unsigned loopback_enabled:1;
 	unsigned downshifted_rate:1;
 	unsigned is_on_sfp_module:1;
+	unsigned mac_managed_pm:1;
 
 	unsigned autoneg:1;
 	/* The most recently read link state */
-- 
2.31.1

[PATCH net-next 2/3] net: fec: use mac-managed PHY PM

[Heiner Kallweit]

Use the new mac_managed_pm flag to work around an issue with KSZ8081 PHY
that becomes unstable when a soft reset is triggered during aneg.

Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3db882322..70aea9c27 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2048,6 +2048,8 @@ static int fec_enet_mii_probe(struct net_device *ndev)
 	fep->link = 0;
 	fep->full_duplex = 0;
 
+	phy_dev->mac_managed_pm = 1;
+
 	phy_attached_info(phy_dev);
 
 	return 0;
@@ -3864,6 +3866,7 @@ static int __maybe_unused fec_resume(struct device *dev)
 		netif_device_attach(ndev);
 		netif_tx_unlock_bh(ndev);
 		napi_enable(&fep->napi);
+		phy_init_hw(ndev->phydev);
 		phy_start(ndev->phydev);
 	}
 	rtnl_unlock();
-- 
2.31.1

RE: [PATCH net-next 2/3] net: fec: use mac-managed PHY PM

[Joakim Zhang]

> -----Original Message-----
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Sent: 2021年4月7日 23:53
> To: Andrew Lunn <andrew@lunn.ch>; Russell King - ARM Linux
> <linux@armlinux.org.uk>; Jakub Kicinski <kuba@kernel.org>; David Miller
> <davem@davemloft.net>; Fugang Duan <fugang.duan@nxp.com>
> Cc: netdev@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH net-next 2/3] net: fec: use mac-managed PHY PM
> 
> Use the new mac_managed_pm flag to work around an issue with KSZ8081
> PHY that becomes unstable when a soft reset is triggered during aneg.
> 
> Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 3db882322..70aea9c27 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2048,6 +2048,8 @@ static int fec_enet_mii_probe(struct net_device
> *ndev)
>  	fep->link = 0;
>  	fep->full_duplex = 0;
> 
> +	phy_dev->mac_managed_pm = 1;
> +
>  	phy_attached_info(phy_dev);
> 
>  	return 0;
> @@ -3864,6 +3866,7 @@ static int __maybe_unused fec_resume(struct
> device *dev)
>  		netif_device_attach(ndev);
>  		netif_tx_unlock_bh(ndev);
>  		napi_enable(&fep->napi);
> +		phy_init_hw(ndev->phydev);


For now, I think we doesn't need to re-initialize PHY after MAC resume back, it also can be done by PHY driver if it needed.

Best Regards,
Joakim Zhang
>  		phy_start(ndev->phydev);
>  	}
>  	rtnl_unlock();
> --
> 2.31.1
> 

Re: [PATCH net-next 2/3] net: fec: use mac-managed PHY PM

[Heiner Kallweit]

On 08.04.2021 07:45, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> Sent: 2021年4月7日 23:53
>> To: Andrew Lunn <andrew@lunn.ch>; Russell King - ARM Linux
>> <linux@armlinux.org.uk>; Jakub Kicinski <kuba@kernel.org>; David Miller
>> <davem@davemloft.net>; Fugang Duan <fugang.duan@nxp.com>
>> Cc: netdev@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>
>> Subject: [PATCH net-next 2/3] net: fec: use mac-managed PHY PM
>>
>> Use the new mac_managed_pm flag to work around an issue with KSZ8081
>> PHY that becomes unstable when a soft reset is triggered during aneg.
>>
>> Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>> Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/ethernet/freescale/fec_main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 3db882322..70aea9c27 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -2048,6 +2048,8 @@ static int fec_enet_mii_probe(struct net_device
>> *ndev)
>>  	fep->link = 0;
>>  	fep->full_duplex = 0;
>>
>> +	phy_dev->mac_managed_pm = 1;
>> +
>>  	phy_attached_info(phy_dev);
>>
>>  	return 0;
>> @@ -3864,6 +3866,7 @@ static int __maybe_unused fec_resume(struct
>> device *dev)
>>  		netif_device_attach(ndev);
>>  		netif_tx_unlock_bh(ndev);
>>  		napi_enable(&fep->napi);
>> +		phy_init_hw(ndev->phydev);
> 
> 
> For now, I think we doesn't need to re-initialize PHY after MAC resume back, it also can be done by PHY driver if it needed.
> 
The PHY PM resume callback (that used to call phy_init_hw) is a no-op now.
So we have to call it from the MAC resume callback. Power to the PHY may be
off during system suspend, therefore it may be reset to power-on defaults.
That's why phy_init_hw() should be called, that includes calling the
PHY drivers config_init callback.

> Best Regards,
> Joakim Zhang
>>  		phy_start(ndev->phydev);
>>  	}
>>  	rtnl_unlock();
>> --
>> 2.31.1
>>
> 

RE: [PATCH net-next 2/3] net: fec: use mac-managed PHY PM

[Joakim Zhang]

> -----Original Message-----
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Sent: 2021年4月8日 14:00
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Andrew Lunn
> <andrew@lunn.ch>; Russell King - ARM Linux <linux@armlinux.org.uk>; Jakub
> Kicinski <kuba@kernel.org>; David Miller <davem@davemloft.net>; Fugang
> Duan <fugang.duan@nxp.com>
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 2/3] net: fec: use mac-managed PHY PM
> 
> On 08.04.2021 07:45, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: Heiner Kallweit <hkallweit1@gmail.com>
> >> Sent: 2021年4月7日 23:53
> >> To: Andrew Lunn <andrew@lunn.ch>; Russell King - ARM Linux
> >> <linux@armlinux.org.uk>; Jakub Kicinski <kuba@kernel.org>; David
> >> Miller <davem@davemloft.net>; Fugang Duan <fugang.duan@nxp.com>
> >> Cc: netdev@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>
> >> Subject: [PATCH net-next 2/3] net: fec: use mac-managed PHY PM
> >>
> >> Use the new mac_managed_pm flag to work around an issue with KSZ8081
> >> PHY that becomes unstable when a soft reset is triggered during aneg.
> >>
> >> Reported-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> >> Tested-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/net/ethernet/freescale/fec_main.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> >> b/drivers/net/ethernet/freescale/fec_main.c
> >> index 3db882322..70aea9c27 100644
> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> @@ -2048,6 +2048,8 @@ static int fec_enet_mii_probe(struct net_device
> >> *ndev)
> >>  	fep->link = 0;
> >>  	fep->full_duplex = 0;
> >>
> >> +	phy_dev->mac_managed_pm = 1;
> >> +
> >>  	phy_attached_info(phy_dev);
> >>
> >>  	return 0;
> >> @@ -3864,6 +3866,7 @@ static int __maybe_unused fec_resume(struct
> >> device *dev)
> >>  		netif_device_attach(ndev);
> >>  		netif_tx_unlock_bh(ndev);
> >>  		napi_enable(&fep->napi);
> >> +		phy_init_hw(ndev->phydev);
> >
> >
> > For now, I think we doesn't need to re-initialize PHY after MAC resume back,
> it also can be done by PHY driver if it needed.
> >
> The PHY PM resume callback (that used to call phy_init_hw) is a no-op now.
> So we have to call it from the MAC resume callback. Power to the PHY may be
> off during system suspend, therefore it may be reset to power-on defaults.
> That's why phy_init_hw() should be called, that includes calling the PHY drivers
> config_init callback.

Yes, it is reasonable to invoke phy_init_hw() here to cover more cases. What I want to describe, we have not run into such case for FEC driver before.
At least no one complain it before as I know. Anyway, re-initialize PHY here indeed can benefit more scenarios.

Best Regards,
Joakim Zhang
> > Best Regards,
> > Joakim Zhang
> >>  		phy_start(ndev->phydev);
> >>  	}
> >>  	rtnl_unlock();
> >> --
> >> 2.31.1
> >>
> >

[PATCH net-next 3/3] r8169: use mac-managed PHY PM

[Heiner Kallweit]

Use the new mac_managed_pm flag to indicate that the driver takes care
of PHY power management.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index a838187b9..eb6da93ac 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -4646,6 +4646,7 @@ static void rtl8169_down(struct rtl8169_private *tp)
 static void rtl8169_up(struct rtl8169_private *tp)
 {
 	pci_set_master(tp->pci_dev);
+	phy_init_hw(tp->phydev);
 	phy_resume(tp->phydev);
 	rtl8169_init_phy(tp);
 	napi_enable(&tp->napi);
@@ -5071,6 +5072,8 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
 		return -EUNATCH;
 	}
 
+	tp->phydev->mac_managed_pm = 1;
+
 	/* PHY will be woken up in rtl_open() */
 	phy_suspend(tp->phydev);
 
-- 
2.31.1

RE: [PATCH net-next 0/3] net: make PHY PM ops a no-op if MAC driver manages PHY PM

[Joakim Zhang]

Hi Heiner,

Why not target this patch set to net repo as a bug fixes? Others may also suffer from this.

Best Regards,
Joakim Zhang

> -----Original Message-----
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Sent: 2021年4月7日 23:51
> To: Andrew Lunn <andrew@lunn.ch>; Russell King - ARM Linux
> <linux@armlinux.org.uk>; Jakub Kicinski <kuba@kernel.org>; David Miller
> <davem@davemloft.net>; Fugang Duan <fugang.duan@nxp.com>
> Cc: netdev@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH net-next 0/3] net: make PHY PM ops a no-op if MAC driver
> manages PHY PM
> 
> Resume callback of the PHY driver is called after the one for the MAC driver.
> The PHY driver resume callback calls phy_init_hw(), and this is potentially
> problematic if the MAC driver calls phy_start() in its resume callback. One issue
> was reported with the fec driver and a KSZ8081 PHY which seems to become
> unstable if a soft reset is triggered during aneg.
> 
> The new flag allows MAC drivers to indicate that they take care of
> suspending/resuming the PHY. Then the MAC PM callbacks can handle any
> dependency between MAC and PHY PM.
> 
> Heiner Kallweit (3):
>   net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM
>   net: fec: use mac-managed PHY PM
>   r8169: use mac-managed PHY PM
> 
>  drivers/net/ethernet/freescale/fec_main.c | 3 +++
> drivers/net/ethernet/realtek/r8169_main.c | 3 +++
>  drivers/net/phy/phy_device.c              | 6 ++++++
>  include/linux/phy.h                       | 2 ++
>  4 files changed, 14 insertions(+)
> 
> --
> 2.31.1

Re: [PATCH net-next 0/3] net: make PHY PM ops a no-op if MAC driver manages PHY PM

[Heiner Kallweit]

On 08.04.2021 07:42, Joakim Zhang wrote:
> 
> Hi Heiner,
> 
> Why not target this patch set to net repo as a bug fixes? Others may also suffer from this.
> 
Reason is that the patch includes new functionality that usually
doesn't get backported. I'd like to see whether your case remains
the only one or whether there will be more similar reports.
In this case we had to think about a fix that doesn't need new
functionality or check whether backporting the new functionality
would be acceptable.

> Best Regards,
> Joakim Zhang
> 
Heiner

>> -----Original Message-----
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> Sent: 2021年4月7日 23:51
>> To: Andrew Lunn <andrew@lunn.ch>; Russell King - ARM Linux
>> <linux@armlinux.org.uk>; Jakub Kicinski <kuba@kernel.org>; David Miller
>> <davem@davemloft.net>; Fugang Duan <fugang.duan@nxp.com>
>> Cc: netdev@vger.kernel.org; Joakim Zhang <qiangqing.zhang@nxp.com>
>> Subject: [PATCH net-next 0/3] net: make PHY PM ops a no-op if MAC driver
>> manages PHY PM
>>
>> Resume callback of the PHY driver is called after the one for the MAC driver.
>> The PHY driver resume callback calls phy_init_hw(), and this is potentially
>> problematic if the MAC driver calls phy_start() in its resume callback. One issue
>> was reported with the fec driver and a KSZ8081 PHY which seems to become
>> unstable if a soft reset is triggered during aneg.
>>
>> The new flag allows MAC drivers to indicate that they take care of
>> suspending/resuming the PHY. Then the MAC PM callbacks can handle any
>> dependency between MAC and PHY PM.
>>
>> Heiner Kallweit (3):
>>   net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM
>>   net: fec: use mac-managed PHY PM
>>   r8169: use mac-managed PHY PM
>>
>>  drivers/net/ethernet/freescale/fec_main.c | 3 +++
>> drivers/net/ethernet/realtek/r8169_main.c | 3 +++
>>  drivers/net/phy/phy_device.c              | 6 ++++++
>>  include/linux/phy.h                       | 2 ++
>>  4 files changed, 14 insertions(+)
>>
>> --
>> 2.31.1
> 

Re: [PATCH net-next 0/3] net: make PHY PM ops a no-op if MAC driver manages PHY PM

[Heiner Kallweit]

On 07.04.2021 17:50, Heiner Kallweit wrote:
> Resume callback of the PHY driver is called after the one for the MAC
> driver. The PHY driver resume callback calls phy_init_hw(), and this is
> potentially problematic if the MAC driver calls phy_start() in its resume
> callback. One issue was reported with the fec driver and a KSZ8081 PHY
> which seems to become unstable if a soft reset is triggered during aneg.
> 
> The new flag allows MAC drivers to indicate that they take care of
> suspending/resuming the PHY. Then the MAC PM callbacks can handle
> any dependency between MAC and PHY PM.
> 
> Heiner Kallweit (3):
>   net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM
>   net: fec: use mac-managed PHY PM
>   r8169: use mac-managed PHY PM
> 
>  drivers/net/ethernet/freescale/fec_main.c | 3 +++
>  drivers/net/ethernet/realtek/r8169_main.c | 3 +++
>  drivers/net/phy/phy_device.c              | 6 ++++++
>  include/linux/phy.h                       | 2 ++
>  4 files changed, 14 insertions(+)
> 

This series has status "Needs ACK". For which part an ACK is needed?

Regarding the fec driver:
The mail to Fugang, the current fec maintainer, bounced and Joakim
confirmed that he left NXP. Joakim will take over, see
https://patchwork.kernel.org/project/netdevbpf/patch/20210409091145.27488-1-qiangqing.zhang@nxp.com/
Joakim also tested the patch.

Heiner

Re: [PATCH net-next 0/3] net: make PHY PM ops a no-op if MAC driver manages PHY PM

[Jakub Kicinski]

On Wed, 7 Apr 2021 17:50:46 +0200 Heiner Kallweit wrote:
> Resume callback of the PHY driver is called after the one for the MAC
> driver. The PHY driver resume callback calls phy_init_hw(), and this is
> potentially problematic if the MAC driver calls phy_start() in its resume
> callback. One issue was reported with the fec driver and a KSZ8081 PHY
> which seems to become unstable if a soft reset is triggered during aneg.
> 
> The new flag allows MAC drivers to indicate that they take care of
> suspending/resuming the PHY. Then the MAC PM callbacks can handle
> any dependency between MAC and PHY PM.

Applied, thanks!