[PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Heiner Kallweit]

I have the issue that suspending the MAC-integrated PHY gives an
error during system suspend. The sequence is:

1. unconnected PHY/MAC are runtime-suspended already
2. system suspend commences
3. mdio_bus_phy_suspend is called
4. suspend callback of the network driver is called (implicitly
   MAC/PHY are runtime-resumed before)
5. suspend callback suspends MAC/PHY

The problem occurs in step 3. phy_suspend() fails because the MDIO
bus isn't accessible due to the chip being runtime-suspended.

This series mainly adds a check to not suspend the PHY if the
MDIO bus parent is runtime-suspended.

Changes in v2:
- Check for MDIO bus parent being runtime-suspended before calling
  phy_ethtool_get_wol() which could access the MDIO bus.

Heiner Kallweit (2):
  net: phy: improve check for when to call phy_resume in mdio_bus_phy_resume
  net: phy: improve checks when to suspend the PHY

 drivers/net/phy/phy_device.c | 45 +++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 19 deletions(-)

-- 
2.17.0

[PATCH net-next v2 1/2] net: phy: improve check for when to call phy_resume in mdio_bus_phy_resume

[Heiner Kallweit]

We don't have to do all the checks again which we did in
mdio_bus_phy_suspend already. Instead we can simply check whether
the PHY is actually suspended and needs to be resumed.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
 drivers/net/phy/phy_device.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9e4ba8e80..1662781fb 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -132,14 +132,12 @@ static int mdio_bus_phy_resume(struct device *dev)
 	struct phy_device *phydev = to_phy_device(dev);
 	int ret;
 
-	if (!mdio_bus_phy_may_suspend(phydev))
-		goto no_resume;
-
-	ret = phy_resume(phydev);
-	if (ret < 0)
-		return ret;
+	if (phydev->suspended) {
+		ret = phy_resume(phydev);
+		if (ret < 0)
+			return ret;
+	}
 
-no_resume:
 	if (phydev->attached_dev && phydev->adjust_link)
 		phy_start_machine(phydev);
 
-- 
2.17.0

[PATCH net-next v2 2/2] net: phy: improve checks when to suspend the PHY

[Heiner Kallweit]

If the parent of the MDIO bus is runtime-suspended, we may not be able
to access the MDIO bus. Therefore add a check for this situation.

So far phy_suspend() only checks for WoL being enabled, other checks
are in mdio_bus_phy_may_suspend(). Improve this and move all checks
to a new function phy_may_suspend() and call it from phy_suspend().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- Check for MDIO bus parent being runtime-suspended before calling
  phy_ethtool_get_wol() which could access the MDIO bus.
---
 drivers/net/phy/phy_device.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1662781fb..bc6a002b1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -35,6 +35,7 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/irq.h>
 
@@ -75,14 +76,27 @@ extern struct phy_driver genphy_10g_driver;
 static LIST_HEAD(phy_fixup_list);
 static DEFINE_MUTEX(phy_fixup_lock);
 
-#ifdef CONFIG_PM
-static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
+static bool phy_may_suspend(struct phy_device *phydev)
 {
 	struct device_driver *drv = phydev->mdio.dev.driver;
 	struct phy_driver *phydrv = to_phy_driver(drv);
 	struct net_device *netdev = phydev->attached_dev;
+	struct device *mdio_bus_parent = phydev->mdio.bus->parent;
+	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
 
-	if (!drv || !phydrv->suspend)
+	if (phydev->suspended || !drv || !phydrv->suspend)
+		return false;
+
+	/* If the parent of the MDIO bus is runtime-suspended, the MDIO bus may
+	 * not be accessible and we expect the parent to suspend all devices
+	 * on the MDIO bus when it suspends.
+	 */
+	if (mdio_bus_parent && pm_runtime_suspended(mdio_bus_parent))
+		return false;
+
+	/* If the device has WOL enabled, we cannot suspend the PHY */
+	phy_ethtool_get_wol(phydev, &wol);
+	if (wol.wolopts)
 		return false;
 
 	/* PHY not attached? May suspend if the PHY has not already been
@@ -91,7 +105,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	 * MDIO bus driver and clock gated at this point.
 	 */
 	if (!netdev)
-		return !phydev->suspended;
+		return true;
 
 	/* Don't suspend PHY if the attached netdev parent may wakeup.
 	 * The parent may point to a PCI device, as in tg3 driver.
@@ -109,6 +123,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 	return true;
 }
 
+#ifdef CONFIG_PM
 static int mdio_bus_phy_suspend(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
@@ -121,9 +136,6 @@ static int mdio_bus_phy_suspend(struct device *dev)
 	if (phydev->attached_dev && phydev->adjust_link)
 		phy_stop_machine(phydev);
 
-	if (!mdio_bus_phy_may_suspend(phydev))
-		return 0;
-
 	return phy_suspend(phydev);
 }
 
@@ -1162,13 +1174,10 @@ EXPORT_SYMBOL(phy_detach);
 int phy_suspend(struct phy_device *phydev)
 {
 	struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
-	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
 	int ret = 0;
 
-	/* If the device has WOL enabled, we cannot suspend the PHY */
-	phy_ethtool_get_wol(phydev, &wol);
-	if (wol.wolopts)
-		return -EBUSY;
+	if (!phy_may_suspend(phydev))
+		return 0;
 
 	if (phydev->drv && phydrv->suspend)
 		ret = phydrv->suspend(phydev);
-- 
2.17.0

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Andrew Lunn]

On Wed, May 23, 2018 at 10:15:29PM +0200, Heiner Kallweit wrote:
> I have the issue that suspending the MAC-integrated PHY gives an
> error during system suspend. The sequence is:
> 
> 1. unconnected PHY/MAC are runtime-suspended already
> 2. system suspend commences
> 3. mdio_bus_phy_suspend is called
> 4. suspend callback of the network driver is called (implicitly
>    MAC/PHY are runtime-resumed before)
> 5. suspend callback suspends MAC/PHY
> 
> The problem occurs in step 3. phy_suspend() fails because the MDIO
> bus isn't accessible due to the chip being runtime-suspended.

I think you are fixing the wrong problem. I've had the same with the
FEC driver. I fixed it by making the MDIO operations runtime-suspend
aware:

commit 8fff755e9f8d0f70a595e79f248695ce6aef5cc3
Author: Andrew Lunn <andrew@lunn.ch>
Date:   Sat Jul 25 22:38:02 2015 +0200

    net: fec: Ensure clocks are enabled while using mdio bus
    
    When a switch is attached to the mdio bus, the mdio bus can be used
    while the interface is not open. If the IPG clock is not enabled, MDIO
    reads/writes will simply time out.
    
    Add support for runtime PM to control this clock. Enable/disable this
    clock using runtime PM, with open()/close() and mdio read()/write()
    function triggering runtime PM operations. Since PM is optional, the
    IPG clock is enabled at probe and is no longer modified by
    fec_enet_clk_enable(), thus if PM is not enabled in the kernel, it is
    guaranteed the clock is running when MDIO operations are performed.

Don't copy this patch 1:1. I introduced a few bugs which took a while
to be shaken out :-(

   Andrew

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Heiner Kallweit]

Am 24.05.2018 um 00:04 schrieb Andrew Lunn:
> On Wed, May 23, 2018 at 10:15:29PM +0200, Heiner Kallweit wrote:
>> I have the issue that suspending the MAC-integrated PHY gives an
>> error during system suspend. The sequence is:
>>
>> 1. unconnected PHY/MAC are runtime-suspended already
>> 2. system suspend commences
>> 3. mdio_bus_phy_suspend is called
>> 4. suspend callback of the network driver is called (implicitly
>>    MAC/PHY are runtime-resumed before)
>> 5. suspend callback suspends MAC/PHY
>>
>> The problem occurs in step 3. phy_suspend() fails because the MDIO
>> bus isn't accessible due to the chip being runtime-suspended.
> 
> I think you are fixing the wrong problem. I've had the same with the
> FEC driver. I fixed it by making the MDIO operations runtime-suspend
> aware:
> 
Interesting, didn't see it from that angle yet. Sounds plausible.
Thanks a lot for the feedback and I'll have a look at the FEC driver.

Heiner

> commit 8fff755e9f8d0f70a595e79f248695ce6aef5cc3
> Author: Andrew Lunn <andrew@lunn.ch>
> Date:   Sat Jul 25 22:38:02 2015 +0200
> 
>     net: fec: Ensure clocks are enabled while using mdio bus
>     
>     When a switch is attached to the mdio bus, the mdio bus can be used
>     while the interface is not open. If the IPG clock is not enabled, MDIO
>     reads/writes will simply time out.
>     
>     Add support for runtime PM to control this clock. Enable/disable this
>     clock using runtime PM, with open()/close() and mdio read()/write()
>     function triggering runtime PM operations. Since PM is optional, the
>     IPG clock is enabled at probe and is no longer modified by
>     fec_enet_clk_enable(), thus if PM is not enabled in the kernel, it is
>     guaranteed the clock is running when MDIO operations are performed.
> 
> Don't copy this patch 1:1. I introduced a few bugs which took a while
> to be shaken out :-(
> 
>    Andrew
> 

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Heiner Kallweit]

Am 24.05.2018 um 00:04 schrieb Andrew Lunn:
> On Wed, May 23, 2018 at 10:15:29PM +0200, Heiner Kallweit wrote:
>> I have the issue that suspending the MAC-integrated PHY gives an
>> error during system suspend. The sequence is:
>>
>> 1. unconnected PHY/MAC are runtime-suspended already
>> 2. system suspend commences
>> 3. mdio_bus_phy_suspend is called
>> 4. suspend callback of the network driver is called (implicitly
>>    MAC/PHY are runtime-resumed before)
>> 5. suspend callback suspends MAC/PHY
>>
>> The problem occurs in step 3. phy_suspend() fails because the MDIO
>> bus isn't accessible due to the chip being runtime-suspended.
> 
> I think you are fixing the wrong problem. I've had the same with the
> FEC driver. I fixed it by making the MDIO operations runtime-suspend
> aware:
> 
I checked the fec driver and there it's reletively easy because
runtime suspend/resume is just about disabling/enabling one clock.

In case of r8169 runtime suspend/resume do much more and there would
be quite some potential deadlock issues to take care of.
To provide one example:
pm_runtime_get_sync() would be called with the mdio bus lock being
held (from mdio write op), runtime-resuming however includes PHY
writes what would result in a deadlock.

I think we need a better solution than spending the effort needed
to make the MDIO ops runtime-pm-aware. In general there seems to be
just one network driver using both phylib and runtime pm, so most
drivers aren't affected (yet).

I will spend few more thoughts on a solution ..

Regards, Heiner

> commit 8fff755e9f8d0f70a595e79f248695ce6aef5cc3
> Author: Andrew Lunn <andrew@lunn.ch>
> Date:   Sat Jul 25 22:38:02 2015 +0200
> 
>     net: fec: Ensure clocks are enabled while using mdio bus
>     
>     When a switch is attached to the mdio bus, the mdio bus can be used
>     while the interface is not open. If the IPG clock is not enabled, MDIO
>     reads/writes will simply time out.
>     
>     Add support for runtime PM to control this clock. Enable/disable this
>     clock using runtime PM, with open()/close() and mdio read()/write()
>     function triggering runtime PM operations. Since PM is optional, the
>     IPG clock is enabled at probe and is no longer modified by
>     fec_enet_clk_enable(), thus if PM is not enabled in the kernel, it is
>     guaranteed the clock is running when MDIO operations are performed.
> 
> Don't copy this patch 1:1. I introduced a few bugs which took a while
> to be shaken out :-(
> 
>    Andrew
> 

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Andrew Lunn]

> I think we need a better solution than spending the effort needed
> to make the MDIO ops runtime-pm-aware. In general there seems to be
> just one network driver using both phylib and runtime pm, so most
> drivers aren't affected (yet).
> 
> I will spend few more thoughts on a solution ..

Hi Heiner

Please keep in mind that MDIO is a generic bus. Many Ethernet switches
are connected via MDIO. Some of those switches have MDIO busses of
their own. Also, some Broadcom devices have USB-PHYs controlled over
MDIO, etc.

So you need a generic solution here.

   Andrew

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Heiner Kallweit]

On 30.05.2018 22:35, Andrew Lunn wrote:
>> I think we need a better solution than spending the effort needed
>> to make the MDIO ops runtime-pm-aware. In general there seems to be
>> just one network driver using both phylib and runtime pm, so most
>> drivers aren't affected (yet).
>>
>> I will spend few more thoughts on a solution ..
> 
> Hi Heiner
> 
> Please keep in mind that MDIO is a generic bus. Many Ethernet switches
> are connected via MDIO. Some of those switches have MDIO busses of
> their own. Also, some Broadcom devices have USB-PHYs controlled over
> MDIO, etc.
> 
> So you need a generic solution here.
> 
>    Andrew
> 
The following proposed change (I combined three patches here) is quite
small, generic, and solves my problem. Another advantage is that it
doesn't impact existing code / drivers.
We just would have to see whether Rafael likes the idea of adding this
flag to the PM core.

Other bus subsystems would be free to adopt the same mechanism with
minimal effort.

Alternatively we could just add a flag to struct mii_bus and not touch
the PM core. But then the solution would be much less generic.

By the way: The problem is related to an experimental patch series for
splitting r8169/r8168 drivers and switching r8168 to phylib.
Therefore the change to r8168.c won't apply to existing kernel code.

Heiner

---
 drivers/net/ethernet/realtek/r8168.c | 1 +
 drivers/net/phy/phy_device.c         | 9 ++++++++-
 include/linux/pm.h                   | 6 ++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8168.c b/drivers/net/ethernet/realtek/r8168.c
index 473a147e..2e1af844 100644
--- a/drivers/net/ethernet/realtek/r8168.c
+++ b/drivers/net/ethernet/realtek/r8168.c
@@ -5063,6 +5063,7 @@ static int r8168_mdio_register(struct rtl8169_private *tp)
        new_bus->irq[0] = PHY_IGNORE_INTERRUPT;
        snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8168-%x",
                 PCI_DEVID(pdev->bus->number, pdev->devfn));
+       dev_pm_set_driver_flags(&new_bus->dev, DPM_FLAG_IGNORE_PM);

        new_bus->read = r8168_mdio_read_reg;
        new_bus->write = r8168_mdio_write_reg;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9e4ba8e8..459fd677 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -76,13 +76,20 @@ static LIST_HEAD(phy_fixup_list);
 static DEFINE_MUTEX(phy_fixup_lock);

 #ifdef CONFIG_PM
+static bool mdio_bus_ignore_pm(struct phy_device *phydev)
+{
+       struct mii_bus *bus = phydev->mdio.bus;
+
+       return dev_pm_test_driver_flags(&bus->dev, DPM_FLAG_IGNORE_PM);
+}
+
 static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 {
        struct device_driver *drv = phydev->mdio.dev.driver;
        struct phy_driver *phydrv = to_phy_driver(drv);
        struct net_device *netdev = phydev->attached_dev;

-       if (!drv || !phydrv->suspend)
+       if (!drv || !phydrv->suspend || mdio_bus_ignore_pm(phydev))
                return false;

        /* PHY not attached? May suspend if the PHY has not already been
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78d..922d2ded 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -560,6 +560,7 @@ struct pm_subsys_data {
  * SMART_PREPARE: Check the return value of the driver's ->prepare callback.
  * SMART_SUSPEND: No need to resume the device from runtime suspend.
  * LEAVE_SUSPENDED: Avoid resuming the device during system resume if possible.
+ * IGNORE_PM: Skip suspend/resume because the parent takes care.
  *
  * Setting SMART_PREPARE instructs bus types and PM domains which may want
  * system suspend/resume callbacks to be skipped for the device to return 0 from
@@ -576,11 +577,16 @@ struct pm_subsys_data {
  *
  * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that the
  * driver prefers the device to be left in suspend after system resume.
+ *
+ * Setting DPM_FLAG_IGNORE_PM instructs middle-layer code to skip suspending /
+ * resuming devices. This is meant for cases where the parent of a bus handles
+ * PM of the devices attached to the bus.
  */
 #define DPM_FLAG_NEVER_SKIP            BIT(0)
 #define DPM_FLAG_SMART_PREPARE         BIT(1)
 #define DPM_FLAG_SMART_SUSPEND         BIT(2)
 #define DPM_FLAG_LEAVE_SUSPENDED       BIT(3)
+#define DPM_FLAG_IGNORE_PM             BIT(4)

 struct dev_pm_info {
        pm_message_t            power_state;

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Andrew Lunn]

> By the way: The problem is related to an experimental patch series for
> splitting r8169/r8168 drivers and switching r8168 to phylib.
> Therefore the change to r8168.c won't apply to existing kernel code.

Hi Heiner

I still think you are trying to fix the wrong problem.

Lets take a look at these patches, particularly your code for
interfacing to phylib.

Thanks
	Andrew

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Heiner Kallweit]

On 31.05.2018 20:30, Andrew Lunn wrote:
>> By the way: The problem is related to an experimental patch series for
>> splitting r8169/r8168 drivers and switching r8168 to phylib.
>> Therefore the change to r8168.c won't apply to existing kernel code.
> 
> Hi Heiner
> 
> I still think you are trying to fix the wrong problem.
> 
> Lets take a look at these patches, particularly your code for
> interfacing to phylib.
> 
> Thanks
> 	Andrew
> 
Hi Andrew,

I skip some intermediate patches and in the following just list the
patch adding basic phylib support. The code shouldn't be a big
surprise ..

The network chip integrates subsystems like MAC and PHY, and the
driver takes care that in suspend / resume the different components
are suspended / resumed in the right order.
This includes actions like speeding down the PHY from 1GB to
preferably 10MB (to save energy) in case WoL is configured.

It causes issues like the one I face now, if subsystems like the
MDIO bus suddenly trigger own suspend / resume actions w/o knowing
about status of the other chip subsystems.
Main issue I think is that we loose control over what is done in
which order.

To provide just one example of typical issues:
Configuring the different WoL options isn't handled by writing to
the PHY registers but by writing to chip / MAC registers.
Therefore phy_suspend() isn't able to figure out whether WoL is
enabled or not. Only the parent has the full picture.

To consider dependencies and ensure the right order of suspend /
resume actions I think a parent should be allowed to request that
it takes care of PM of its subsystems.

Regards,
Heiner

---
 drivers/net/ethernet/realtek/Kconfig |   1 +
 drivers/net/ethernet/realtek/r8168.c | 140 +++++++++++++++++++++------
 drivers/net/ethernet/realtek/r8169.h |   2 +
 3 files changed, 116 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 404e1f288..f1e9e8cc2 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -105,6 +105,7 @@ config R8168
 	select R8169_COMMON
 	select FW_LOADER
 	select CRC32
+	select PHYLIB
 	select MII
 	help
 	  Say Y here if you have a Realtek 8168 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8168.c b/drivers/net/ethernet/realtek/r8168.c
index 546115344..473a147ec 100644
--- a/drivers/net/ethernet/realtek/r8168.c
+++ b/drivers/net/ethernet/realtek/r8168.c
@@ -16,6 +16,7 @@
 #include <linux/delay.h>
 #include <linux/ethtool.h>
 #include <linux/mii.h>
+#include <linux/phy.h>
 #include <linux/if_vlan.h>
 #include <linux/in.h>
 #include <linux/ip.h>
@@ -868,25 +869,6 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
 	}
 }
 
-static void rtl8169_check_link_status(struct net_device *dev,
-				      struct rtl8169_private *tp)
-{
-	struct device *d = tp_to_dev(tp);
-
-	if (rtl8169_xmii_link_ok(tp)) {
-		rtl_link_chg_patch(tp);
-		/* This is to cancel a scheduled suspend if there's one. */
-		pm_request_resume(d);
-		netif_carrier_on(dev);
-		if (net_ratelimit())
-			netif_info(tp, ifup, dev, "link up\n");
-	} else {
-		netif_carrier_off(dev);
-		netif_info(tp, ifdown, dev, "link down\n");
-		pm_runtime_idle(d);
-	}
-}
-
 #define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST)
 
 static u32 __rtl8169_get_wol(struct rtl8169_private *tp)
@@ -4444,8 +4426,8 @@ static void rtl_reset_work(struct rtl8169_private *tp)
 
 	napi_enable(&tp->napi);
 	rtl8169_hw_start(tp);
+	phy_start(dev->phydev);
 	netif_wake_queue(dev);
-	rtl8169_check_link_status(dev, tp);
 }
 
 static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp, struct sk_buff *skb)
@@ -4615,7 +4597,7 @@ static void rtl_slow_event_work(struct rtl8169_private *tp)
 		rtl8169_pcierr_interrupt(dev);
 
 	if (status & LinkChg)
-		rtl8169_check_link_status(dev, tp);
+		phy_mac_interrupt(dev->phydev);
 
 	rtl_irq_enable_all(tp);
 }
@@ -4658,6 +4640,8 @@ static void rtl8169_down(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 
+	phy_stop(dev->phydev);
+
 	del_timer_sync(&tp->timer);
 
 	napi_disable(&tp->napi);
@@ -4762,14 +4746,13 @@ static int rtl_open(struct net_device *dev)
 	if (!rtl8169_init_counter_offsets(tp))
 		netif_warn(tp, hw, dev, "counter reset/update failed\n");
 
+	phy_start(dev->phydev);
 	netif_start_queue(dev);
 
 	rtl_unlock_work(tp);
 
 	tp->saved_wolopts = 0;
 	pm_runtime_put_sync(&pdev->dev);
-
-	rtl8169_check_link_status(dev, tp);
 out:
 	return retval;
 
@@ -4796,6 +4779,7 @@ static void rtl8169_net_suspend(struct net_device *dev)
 	if (!netif_running(dev))
 		return;
 
+	phy_stop(dev->phydev);
 	netif_device_detach(dev);
 	netif_stop_queue(dev);
 
@@ -4827,6 +4811,8 @@ static void __rtl8169_resume(struct net_device *dev)
 
 	rtl_pll_power_up(tp);
 
+	phy_start(tp->dev->phydev);
+
 	rtl_lock_work(tp);
 	napi_enable(&tp->napi);
 	set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -4979,6 +4965,8 @@ static void rtl_remove_one(struct pci_dev *pdev)
 	netif_napi_del(&tp->napi);
 
 	unregister_netdev(dev);
+	phy_disconnect(dev->phydev);
+	mdiobus_unregister(tp->mii_bus);
 
 	rtl_release_firmware(tp);
 
@@ -5041,6 +5029,92 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond)
 	return (RTL_R8(tp, MCU) & RXTX_EMPTY) == RXTX_EMPTY;
 }
 
+static int r8168_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
+{
+	struct rtl8169_private *tp = mii_bus->priv;
+
+	return tp->mdio_ops.read(tp, phyreg);
+}
+
+static int r8168_mdio_write_reg(struct mii_bus *mii_bus, int phyaddr,
+				int phyreg, u16 val)
+{
+	struct rtl8169_private *tp = mii_bus->priv;
+
+	tp->mdio_ops.write(tp, phyreg, val);
+
+	return 0;
+}
+
+static int r8168_mdio_register(struct rtl8169_private *tp)
+{
+	struct pci_dev *pdev = tp->pci_dev;
+	struct mii_bus *new_bus;
+	int ret;
+
+	new_bus = devm_mdiobus_alloc(&pdev->dev);
+	if (!new_bus)
+		return -ENOMEM;
+
+	new_bus->name = "r8168";
+	new_bus->priv = tp;
+	new_bus->phy_mask = ~1;
+	new_bus->parent = &pdev->dev;
+	new_bus->irq[0] = PHY_IGNORE_INTERRUPT;
+	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8168-%x",
+		 PCI_DEVID(pdev->bus->number, pdev->devfn));
+
+	new_bus->read = r8168_mdio_read_reg;
+	new_bus->write = r8168_mdio_write_reg;
+
+	ret = mdiobus_register(new_bus);
+	if (!ret)
+		tp->mii_bus = new_bus;
+
+	return ret;
+}
+
+static void r8168_phylink_handler(struct net_device *ndev)
+{
+	struct rtl8169_private *tp = netdev_priv(ndev);
+
+	if (netif_carrier_ok(ndev)) {
+		rtl_link_chg_patch(tp);
+		pm_request_resume(&tp->pci_dev->dev);
+	} else {
+		pm_runtime_idle(&tp->pci_dev->dev);
+	}
+
+	if (net_ratelimit())
+		phy_print_status(ndev->phydev);
+}
+
+static int r8168_phy_connect(struct rtl8169_private *tp)
+{
+	struct phy_device *phydev;
+	phy_interface_t phy_mode;
+	int ret;
+
+	phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+		   PHY_INTERFACE_MODE_MII;
+
+	phydev = phy_find_first(tp->mii_bus);
+	if (!phydev)
+		return -ENODEV;
+
+	if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+		netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
+		phy_set_max_speed(phydev, SPEED_100);
+	}
+
+	ret = phy_connect_direct(tp->dev, phydev, r8168_phylink_handler,
+				 phy_mode);
+	if (!ret)
+		phy_attached_info(phydev);
+
+	return ret;
+}
+
 static void rtl_hw_init_8168g(struct rtl8169_private *tp)
 {
 	u32 data;
@@ -5287,10 +5361,18 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pci_set_drvdata(pdev, dev);
 
-	rc = register_netdev(dev);
-	if (rc < 0)
+	rc = r8168_mdio_register(tp);
+	if (rc)
 		return rc;
 
+	rc = r8168_phy_connect(tp);
+	if (rc)
+		goto err_mdio_unregister;
+
+	rc = register_netdev(dev);
+	if (rc)
+		goto err_phy_disconnect;
+
 	netif_info(tp, probe, dev, "%s, %pM, XID %08x, IRQ %d\n",
 		   rtl_chip_infos[chipset].name, dev->dev_addr,
 		   (u32)(RTL_R32(tp, TxConfig) & 0xfcf0f8ff),
@@ -5303,12 +5385,16 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (r8168_check_dash(tp))
 		rtl8168_driver_start(tp);
 
-	netif_carrier_off(dev);
-
 	if (pci_dev_run_wake(pdev))
 		pm_runtime_put_sync(&pdev->dev);
 
 	return 0;
+
+err_phy_disconnect:
+	phy_disconnect(dev->phydev);
+err_mdio_unregister:
+	mdiobus_unregister(tp->mii_bus);
+	return rc;
 }
 
 static struct pci_driver rtl8168_pci_driver = {
diff --git a/drivers/net/ethernet/realtek/r8169.h b/drivers/net/ethernet/realtek/r8169.h
index 355bbcd0f..2f18e5dee 100644
--- a/drivers/net/ethernet/realtek/r8169.h
+++ b/drivers/net/ethernet/realtek/r8169.h
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/ethtool.h>
 #include <linux/mii.h>
+#include <linux/phy.h>
 #include <linux/if_vlan.h>
 #include <linux/in.h>
 #include <linux/ip.h>
@@ -475,6 +476,7 @@ struct rtl8169_private {
 	} wk;
 
 	struct mii_if_info mii;
+	struct mii_bus *mii_bus;
 	dma_addr_t counters_phys_addr;
 	struct rtl8169_counters *counters;
 	struct rtl8169_tc_offsets tc_offset;
-- 
2.17.0

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Andrew Lunn]

> Configuring the different WoL options isn't handled by writing to
> the PHY registers but by writing to chip / MAC registers.
> Therefore phy_suspend() isn't able to figure out whether WoL is
> enabled or not. Only the parent has the full picture.

Hi Heiner

I think you need to look at your different runtime PM domains.  If i
understand the code right, you runtime suspend if there is no
link. But for this to work correctly, your PHY needs to keep working.
You also cannot assume all accesses to the PHY go via the MAC. Some
calls will go direct to the PHY, and they can trigger MDIO bus
accesses.  So i think you need two runtime PM domains. MAC and MDIO
bus.  Maybe just the pll? An MDIO bus is a device, so it can have its
on PM callbacks. It is not clear what you need to resume in order to
make MDIO work.

It might also help if you do the phy_connect in .ndo_open and
disconnect in .ndo_stop. This is a common pattern in drivers. But some
also do it is probe and remove.

     Andrew

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Heiner Kallweit]

On 01.06.2018 02:10, Andrew Lunn wrote:
>> Configuring the different WoL options isn't handled by writing to
>> the PHY registers but by writing to chip / MAC registers.
>> Therefore phy_suspend() isn't able to figure out whether WoL is
>> enabled or not. Only the parent has the full picture.
> 
> Hi Heiner
> 
> I think you need to look at your different runtime PM domains.  If i
> understand the code right, you runtime suspend if there is no
> link. But for this to work correctly, your PHY needs to keep working.
> You also cannot assume all accesses to the PHY go via the MAC. Some
> calls will go direct to the PHY, and they can trigger MDIO bus
> accesses.  So i think you need two runtime PM domains. MAC and MDIO
> bus.  Maybe just the pll? An MDIO bus is a device, so it can have its
> on PM callbacks. It is not clear what you need to resume in order to
> make MDIO work.
> 
Thanks for your comments!
The actual problem is quite small: I get an error at MDIO suspend,
the PHY however is suspended later by the driver's suspend callback
anyway. Because the problem is small I'm somewhat reluctant to
consider bigger changes like introducing different PM domains.

Primary reason for the error is that the network chip is in PCI D3hot
at that moment. In addition to that for some of the chips supported by
the driver also MDIO-relevant PLL's might be disabled.

By the way:
When checking PM handling for PHY/MDIO I stumbled across something
that can be improved IMO, I'll send a patch for your review.

> It might also help if you do the phy_connect in .ndo_open and
> disconnect in .ndo_stop. This is a common pattern in drivers. But some
> also do it is probe and remove.
> 
Thanks for the hint. I will move phy_connect_direct accordingly.

>      Andrew
> 
Heiner

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Heiner Kallweit]

On 02.06.2018 22:27, Heiner Kallweit wrote:
> On 01.06.2018 02:10, Andrew Lunn wrote:
>>> Configuring the different WoL options isn't handled by writing to
>>> the PHY registers but by writing to chip / MAC registers.
>>> Therefore phy_suspend() isn't able to figure out whether WoL is
>>> enabled or not. Only the parent has the full picture.
>>
>> Hi Heiner
>>
>> I think you need to look at your different runtime PM domains.  If i
>> understand the code right, you runtime suspend if there is no
>> link. But for this to work correctly, your PHY needs to keep working.
>> You also cannot assume all accesses to the PHY go via the MAC. Some
>> calls will go direct to the PHY, and they can trigger MDIO bus
>> accesses.  So i think you need two runtime PM domains. MAC and MDIO
>> bus.  Maybe just the pll? An MDIO bus is a device, so it can have its
>> on PM callbacks. It is not clear what you need to resume in order to
>> make MDIO work.
>>
> Thanks for your comments!
> The actual problem is quite small: I get an error at MDIO suspend,
> the PHY however is suspended later by the driver's suspend callback
> anyway. Because the problem is small I'm somewhat reluctant to
> consider bigger changes like introducing different PM domains.
> 
> Primary reason for the error is that the network chip is in PCI D3hot
> at that moment. In addition to that for some of the chips supported by
> the driver also MDIO-relevant PLL's might be disabled.
> 
> By the way:
> When checking PM handling for PHY/MDIO I stumbled across something
> that can be improved IMO, I'll send a patch for your review.
> 
I experimented a little and with adding Runtime PM to MDIO bus and
PHY device I can make it work:
PHY runtime-resumes before entering suspend and resumes its parent
(MDIO bus) which then resumes its parent (PCI device).
However this needs quite some code and is hard to read / understand
w/o reading through this mail thread.

And in general I still have doubts this is the right way. Let's
consider the following scenario:

A network driver configures WoL in its suspend callback
(incl. setting the device to wakeup-enabled).
The suspend callback of the PHY is called before this and therefore
has no clue that WoL will be configured a little later, with the
consequence that it will do an unsolicited power-down.
The network driver then has to detect this and power-up the PHY again.
This doesn't seem to make much sense and still my best idea is to
establish a mechanism that a device can state: I orchestrate PM
of my children.

Heiner

>> It might also help if you do the phy_connect in .ndo_open and
>> disconnect in .ndo_stop. This is a common pattern in drivers. But some
>> also do it is probe and remove.
>>
> Thanks for the hint. I will move phy_connect_direct accordingly.
> 
>>      Andrew
>>
> Heiner
> 

Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

[Heiner Kallweit]

On 05.06.2018 21:39, Heiner Kallweit wrote:
> On 02.06.2018 22:27, Heiner Kallweit wrote:
>> On 01.06.2018 02:10, Andrew Lunn wrote:
>>>> Configuring the different WoL options isn't handled by writing to
>>>> the PHY registers but by writing to chip / MAC registers.
>>>> Therefore phy_suspend() isn't able to figure out whether WoL is
>>>> enabled or not. Only the parent has the full picture.
>>>
>>> Hi Heiner
>>>
>>> I think you need to look at your different runtime PM domains.  If i
>>> understand the code right, you runtime suspend if there is no
>>> link. But for this to work correctly, your PHY needs to keep working.
>>> You also cannot assume all accesses to the PHY go via the MAC. Some
>>> calls will go direct to the PHY, and they can trigger MDIO bus
>>> accesses.  So i think you need two runtime PM domains. MAC and MDIO
>>> bus.  Maybe just the pll? An MDIO bus is a device, so it can have its
>>> on PM callbacks. It is not clear what you need to resume in order to
>>> make MDIO work.
>>>
>> Thanks for your comments!
>> The actual problem is quite small: I get an error at MDIO suspend,
>> the PHY however is suspended later by the driver's suspend callback
>> anyway. Because the problem is small I'm somewhat reluctant to
>> consider bigger changes like introducing different PM domains.
>>
>> Primary reason for the error is that the network chip is in PCI D3hot
>> at that moment. In addition to that for some of the chips supported by
>> the driver also MDIO-relevant PLL's might be disabled.
>>
>> By the way:
>> When checking PM handling for PHY/MDIO I stumbled across something
>> that can be improved IMO, I'll send a patch for your review.
>>
> I experimented a little and with adding Runtime PM to MDIO bus and
> PHY device I can make it work:
> PHY runtime-resumes before entering suspend and resumes its parent
> (MDIO bus) which then resumes its parent (PCI device).
> However this needs quite some code and is hard to read / understand
> w/o reading through this mail thread.
> 
> And in general I still have doubts this is the right way. Let's
> consider the following scenario:
> 
> A network driver configures WoL in its suspend callback
> (incl. setting the device to wakeup-enabled).
> The suspend callback of the PHY is called before this and therefore
> has no clue that WoL will be configured a little later, with the
> consequence that it will do an unsolicited power-down.
> The network driver then has to detect this and power-up the PHY again.
> This doesn't seem to make much sense and still my best idea is to
> establish a mechanism that a device can state: I orchestrate PM
> of my children.
> 
There's one more way to deal with the issue, an empty dev_pm_domain.
We could do:

struct dev_pm_domain empty = { .ops = NULL };
phydev->mdio.dev.pm_domain = empty;

This overrides the device_type pm ops, however I wouldn't necessarily
consider it an elegant solution. Do you have an opinion on that?

I also checked device links, however this doesn't seem to be the right
concept in our case.

> Heiner
> 
>>> It might also help if you do the phy_connect in .ndo_open and
>>> disconnect in .ndo_stop. This is a common pattern in drivers. But some
>>> also do it is probe and remove.
>>>
>> Thanks for the hint. I will move phy_connect_direct accordingly.
>>
>>>      Andrew
>>>
>> Heiner
>>
>