From: Heiner Kallweit <hkallweit1@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
David Miller <davem@davemloft.net>,
Realtek linux nic maintainers <nic_swsd@realtek.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: [PATCH net v2] net: phy: fix WoL handling when suspending the PHY
Date: Sun, 23 Sep 2018 15:33:41 +0200 [thread overview]
Message-ID: <f8737cef-88d2-0f60-bc96-88251e77c1bd@gmail.com> (raw)
In-Reply-To: <4dbcdd2c-96c2-2921-5016-affc8fce1d19@gmail.com>
Actually there's nothing wrong with the two changes marked as "Fixes",
they just revealed a problem which has been existing before.
After having switched r8169 to phylib it was reported that WoL from
shutdown doesn't work any longer (WoL from suspend isn't affected).
Reason is that during shutdown phy_disconnect()->phy_detach()->
phy_suspend() is called.
A similar issue occurs when the phylib state machine calls
phy_suspend() when handling state PHY_HALTED.
Core of the problem is that phy_suspend() suspends the PHY when it
should not due to WoL. phy_suspend() checks for WoL already, but this
works only if the PHY driver handles WoL (what is rarely the case).
Typically WoL is handled by the MAC driver.
phylib knows about this and handles it in mdio_bus_phy_may_suspend(),
but that's used only when suspending the system, not in other cases
like shutdown.
Therefore factor out the relevant check from
mdio_bus_phy_may_suspend() to a new function phy_may_suspend() and
use it in phy_suspend().
Last but not least change phy_detach() to call phy_suspend() before
attached_dev is set to NULL. phy_suspend() accesses attached_dev
when checking whether the MAC driver activated WoL.
Fixes: f1e911d5d0df ("r8169: add basic phylib support")
Fixes: e8cfd9d6c772 ("net: phy: call state machine synchronously in phy_stop")
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- improved commit message
- reduced scope of patch, don't touch functionality of
mdio_bus_phy_suspend and mdio_bus_phy_resume
---
drivers/net/phy/phy_device.c | 42 ++++++++++++++++++++++--------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index af64a9320..4cab94bae 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -75,6 +75,26 @@ extern struct phy_driver genphy_10g_driver;
static LIST_HEAD(phy_fixup_list);
static DEFINE_MUTEX(phy_fixup_lock);
+static bool phy_may_suspend(struct phy_device *phydev)
+{
+ struct net_device *netdev = phydev->attached_dev;
+
+ if (!netdev)
+ 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.
+ */
+ if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent))
+ return false;
+
+ /* Also don't suspend PHY if the netdev itself may wakeup. This
+ * is the case for devices w/o underlaying pwr. mgmt. aware bus,
+ * e.g. SoC devices.
+ */
+ return !device_may_wakeup(&netdev->dev);
+}
+
#ifdef CONFIG_PM
static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
{
@@ -93,20 +113,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
if (!netdev)
return !phydev->suspended;
- /* Don't suspend PHY if the attached netdev parent may wakeup.
- * The parent may point to a PCI device, as in tg3 driver.
- */
- if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent))
- return false;
-
- /* Also don't suspend PHY if the netdev itself may wakeup. This
- * is the case for devices w/o underlaying pwr. mgmt. aware bus,
- * e.g. SoC devices.
- */
- if (device_may_wakeup(&netdev->dev))
- return false;
-
- return true;
+ return phy_may_suspend(phydev);
}
static int mdio_bus_phy_suspend(struct device *dev)
@@ -1132,9 +1139,9 @@ void phy_detach(struct phy_device *phydev)
sysfs_remove_link(&dev->dev.kobj, "phydev");
sysfs_remove_link(&phydev->mdio.dev.kobj, "attached_dev");
}
+ phy_suspend(phydev);
phydev->attached_dev->phydev = NULL;
phydev->attached_dev = NULL;
- phy_suspend(phydev);
phydev->phylink = NULL;
phy_led_triggers_unregister(phydev);
@@ -1171,9 +1178,12 @@ int phy_suspend(struct phy_device *phydev)
struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
int ret = 0;
+ if (phydev->suspended)
+ return 0;
+
/* If the device has WOL enabled, we cannot suspend the PHY */
phy_ethtool_get_wol(phydev, &wol);
- if (wol.wolopts)
+ if (wol.wolopts || !phy_may_suspend(phydev))
return -EBUSY;
if (phydev->drv && phydrv->suspend)
--
2.19.0
next prev parent reply other threads:[~2018-09-23 19:31 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-21 23:32 [PATCH net] net: phy: fix WoL handling when suspending the PHY Heiner Kallweit
2018-09-22 16:44 ` Andrew Lunn
2018-09-22 17:32 ` Heiner Kallweit
2018-09-23 13:33 ` Heiner Kallweit [this message]
2018-09-23 13:39 ` [PATCH net v2] " Heiner Kallweit
-- strict thread matches above, loose matches on Subject: below --
2018-09-23 13:38 Heiner Kallweit
2018-09-23 16:49 ` Florian Fainelli
2018-09-23 19:30 ` Heiner Kallweit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f8737cef-88d2-0f60-bc96-88251e77c1bd@gmail.com \
--to=hkallweit1@gmail.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=nic_swsd@realtek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).