From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume Date: Fri, 8 Jun 2018 08:09:06 +0200 Message-ID: <23e97d5b-9b83-ec8f-04ff-817ba5ab6c16@gmail.com> References: <8fe93623-9d05-6182-fe5f-da0bd32bae0b@gmail.com> <20180523220418.GB5128@lunn.ch> <5b9eceab-35b4-922c-7410-d8968d8364c7@gmail.com> <20180530203512.GA16286@lunn.ch> <1d79c2ae-6f63-694f-6c63-6369c854de69@gmail.com> <20180531183040.GA7378@lunn.ch> <20180601001043.GA2303@lunn.ch> <28a8846a-f7a7-3266-3aec-7f58fe8dac72@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Florian Fainelli , David Miller , "netdev@vger.kernel.org" To: Andrew Lunn Return-path: Received: from mail-wm0-f68.google.com ([74.125.82.68]:32999 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750823AbeFHGJN (ORCPT ); Fri, 8 Jun 2018 02:09:13 -0400 Received: by mail-wm0-f68.google.com with SMTP id z6-v6so5776255wma.0 for ; Thu, 07 Jun 2018 23:09:12 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 05.06.2018 21:39, Heiner Kallweit wrote: > On 02.06.2018 22:27, Heiner Kallweit wrote: >> On 01.06.2018 02:10, Andrew Lunn wrote: >>>> Configuring the different WoL options isn't handled by writing to >>>> the PHY registers but by writing to chip / MAC registers. >>>> Therefore phy_suspend() isn't able to figure out whether WoL is >>>> enabled or not. Only the parent has the full picture. >>> >>> Hi Heiner >>> >>> I think you need to look at your different runtime PM domains. If i >>> understand the code right, you runtime suspend if there is no >>> link. But for this to work correctly, your PHY needs to keep working. >>> You also cannot assume all accesses to the PHY go via the MAC. Some >>> calls will go direct to the PHY, and they can trigger MDIO bus >>> accesses. So i think you need two runtime PM domains. MAC and MDIO >>> bus. Maybe just the pll? An MDIO bus is a device, so it can have its >>> on PM callbacks. It is not clear what you need to resume in order to >>> make MDIO work. >>> >> Thanks for your comments! >> The actual problem is quite small: I get an error at MDIO suspend, >> the PHY however is suspended later by the driver's suspend callback >> anyway. Because the problem is small I'm somewhat reluctant to >> consider bigger changes like introducing different PM domains. >> >> Primary reason for the error is that the network chip is in PCI D3hot >> at that moment. In addition to that for some of the chips supported by >> the driver also MDIO-relevant PLL's might be disabled. >> >> By the way: >> When checking PM handling for PHY/MDIO I stumbled across something >> that can be improved IMO, I'll send a patch for your review. >> > I experimented a little and with adding Runtime PM to MDIO bus and > PHY device I can make it work: > PHY runtime-resumes before entering suspend and resumes its parent > (MDIO bus) which then resumes its parent (PCI device). > However this needs quite some code and is hard to read / understand > w/o reading through this mail thread. > > And in general I still have doubts this is the right way. Let's > consider the following scenario: > > A network driver configures WoL in its suspend callback > (incl. setting the device to wakeup-enabled). > The suspend callback of the PHY is called before this and therefore > has no clue that WoL will be configured a little later, with the > consequence that it will do an unsolicited power-down. > The network driver then has to detect this and power-up the PHY again. > This doesn't seem to make much sense and still my best idea is to > establish a mechanism that a device can state: I orchestrate PM > of my children. > There's one more way to deal with the issue, an empty dev_pm_domain. We could do: struct dev_pm_domain empty = { .ops = NULL }; phydev->mdio.dev.pm_domain = empty; This overrides the device_type pm ops, however I wouldn't necessarily consider it an elegant solution. Do you have an opinion on that? I also checked device links, however this doesn't seem to be the right concept in our case. > Heiner > >>> It might also help if you do the phy_connect in .ndo_open and >>> disconnect in .ndo_stop. This is a common pattern in drivers. But some >>> also do it is probe and remove. >>> >> Thanks for the hint. I will move phy_connect_direct accordingly. >> >>> Andrew >>> >> Heiner >> >