Message-ID: <e8f160a0-3fc7-99a4-befd-2d73452230a0@gmail.com>
Date:   Sun, 23 Sep 2018 21:30:49 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2] net: phy: fix WoL handling when suspending the PHY

On 23.09.2018 18:49, Florian Fainelli wrote:
> 
> 
> On September 23, 2018 6:38:21 AM PDT, Heiner Kallweit <hkallweit1@...il.com> wrote:
>> 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.
> 
> The rationale for the change makes sense but I am worried about a couple of things:
> 
> - we have seen drivers fail to call SET_NETDEV_DEV() correctly, or sometimes it is made possible because they are Device Tree only objects without a backing parent struct device (CONFIG_OF makes it possible), so we could be missing some cases here but this is not a big deal
> 
> - most Ethernet controller implementations typically set the device as a wake-up enabled device when an user issues the appropriate ethtool -s iface wol <parameters> and not at driver probe or ndo_open() time. There are exceptions like ASIX USB Ethernet adapters that AFAIR are wake-up enabled all the time. The main concern here is that at the time of suspend device_may_wakeup() evaluates to true and we keep the PHY on even though the driver/user was not requesting for WoL per-se
> 
> What you encode here is definitely the behavior that Ethernet controllers should have, but we should also audit drivers that use PHYLIB and implement WoL that this is not going to regresses their power management or ability to wake up from PHY, and finally, that there is not redundant code already in place.
> 
Good points. It's somehow unfortunate that we don't have a clear info
whether WoL is enabled for a netdevice or not. Instead we have to
evaluate different hints and make an educated guess (which can be
wrong like in the potential cases you described).

One idea that comes to my mind: Add a flag wol_enabled to struct net_device
and set it in ethtool_set_wol. Then we could use this new flag in phy_suspend().
This would transparently fix the issue for the most common cases.
Still we would have to think about cases like WoL being enabled w/o explicitly
configuring it (e.g. BIOS enables WoL).
What do you think?

>>
>> Fixes: f1e911d5d0df ("r8169: add basic phylib support")
>> Fixes: e8cfd9d6c772 ("net: phy: call state machine synchronously in
>> phy_stop")
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.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)
> 

Powered by blists - more mailing lists