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 >
