From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH net] net: phy: fix WoL handling when suspending the PHY Date: Sat, 22 Sep 2018 19:32:52 +0200 Message-ID: References: <4dbcdd2c-96c2-2921-5016-affc8fce1d19@gmail.com> <20180922164441.GB13316@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Florian Fainelli , David Miller , Realtek linux nic maintainers , "netdev@vger.kernel.org" To: Andrew Lunn Return-path: Received: from mail-wm1-f68.google.com ([209.85.128.68]:54036 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726004AbeIVX1U (ORCPT ); Sat, 22 Sep 2018 19:27:20 -0400 Received: by mail-wm1-f68.google.com with SMTP id b19-v6so5918658wme.3 for ; Sat, 22 Sep 2018 10:33:00 -0700 (PDT) In-Reply-To: <20180922164441.GB13316@lunn.ch> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 22.09.2018 18:44, Andrew Lunn wrote: > On Sat, Sep 22, 2018 at 01:32:55AM +0200, Heiner Kallweit wrote: >> Actually there's nothing wrong with the two changes, they just >> revealed a problem which has been existing before. > > Hi Heiner > > This is missing a bit of context. Which two changes? I assume you mean > the two Fixes: > Right. I should have mentioned that the actual trigger were reported problems with WoL. >> >> Core of the problem is that phy_suspend() suspends the PHY when it >> should not because of 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. >> >> This patch basically moves the check for WoL having been activated >> by the MAC driver into phy_suspend(). mdio_bus_phy_resume() now >> resumes the PHY only if it's actually suspended. Also don't do >> anything in phy_suspend() if the PHY is suspended already. >> >> 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. > > This sounds like it should be multiple patches, not one. Can it be > split, or is there two much inter connectivity? Being able to bisect > could be useful here. > I could split it to two patches: 1. Add phy_may_suspend() and use it in phy_suspend() 2. Remove mdio_bus_phy_may_suspend() and change mdio_bus_phy_suspend() and mdio_bus_phy_resume() Heiner > Thanks > Andrew >