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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1d79c2ae-6f63-694f-6c63-6369c854de69@gmail.com>
Date:   Thu, 31 May 2018 17:58:05 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

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