netdev - Re: [PATCH net v2] net: phy: fix WoL handling when suspending the PHY

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] [day] [month] [year] [list]

Message-ID: <4dc9ecd3-4b3e-ef6e-1bb7-432ce051d2c9@gmail.com>
Date:   Sun, 23 Sep 2018 15:39:39 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        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

Sorry, this patch was accidentally sent as reply.

On 23.09.2018 15:33, Heiner Kallweit 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.
> 
> 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)
>