On Wed, 5 Jun 2019 at 12:31, Russell King - ARM Linux admin <li...@armlinux.org.uk> wrote: > > On Wed, Jun 05, 2019 at 11:27:59AM +0300, Vladimir Oltean wrote: > > On Wed, 5 Jun 2019 at 06:06, Florian Fainelli <f.faine...@gmail.com> wrote: > > > > > > > > > > > > On 6/4/2019 4:46 PM, Vladimir Oltean wrote: > > > > On Wed, 5 Jun 2019 at 02:24, Russell King - ARM Linux admin > > > > <li...@armlinux.org.uk> wrote: > > > >> > > > >> On Wed, Jun 05, 2019 at 02:03:19AM +0300, Vladimir Oltean wrote: > > > >>> On Wed, 5 Jun 2019 at 01:59, Russell King - ARM Linux admin > > > >>> <li...@armlinux.org.uk> wrote: > > > >>>> > > > >>>> On Wed, Jun 05, 2019 at 01:44:08AM +0300, Vladimir Oltean wrote: > > > >>>>> You caught me. > > > >>>>> > > > >>>>> But even ignoring the NIC case, isn't the PHY state machine > > > >>>>> inconsistent with itself? It is ok with callink phy_suspend upon > > > >>>>> ndo_stop, but it won't call phy_suspend after phy_connect, when the > > > >>>>> netdev is implicitly stopped? > > > >>>> > > > >>>> The PHY state machine isn't inconsistent with itself, but it does > > > >>>> have strange behaviour. > > > >>>> > > > >>>> When the PHY is attached, the PHY is resumed and the state machine > > > >>>> is in PHY_READY state. If it goes through a start/stop cycle, the > > > >>>> state machine transitions to PHY_HALTED and attempts to place the > > > >>>> PHY into a low power state. So the PHY state is consistent with > > > >>>> the state machine state (we don't end up in the same state but with > > > >>>> the PHY in a different state.) > > > >>>> > > > >>>> What we do have is a difference between the PHY state (and state > > > >>>> machine state) between the boot scenario, and the interface up/down > > > >>>> scenario, the latter behaviour having been introduced by a commit > > > >>>> back in 2013: > > > >>>> > > > >>>> net: phy: suspend phydev when going to HALTED > > > >>>> > > > >>>> When phydev is going to HALTED state, we can try to suspend it to > > > >>>> safe more power. phy_suspend helper will check if PHY can be > > > >>>> suspended, > > > >>>> so just call it when entering HALTED state. > > > >>>> > > > >>>> -- > > > >>>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > > >>>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down > > > >>>> 622kbps up > > > >>>> According to speedtest.net: 11.9Mbps down 500kbps up > > > >>> > > > >>> I am really not into the PHYLIB internals, but basically what you're > > > >>> telling me is that running "ip link set dev eth0 down" is a > > > >>> stronger/more imperative condition than not running "ip link set dev > > > >>> eth0 up"... Does it also suspend the PHY if I put the interface down > > > >>> while it was already down? > > > >> > > > >> No - but that has nothing to do with phylib internals, more to do with > > > >> the higher levels of networking. ndo_stop() will not be called unless > > > >> ndo_open() has already been called. In other words, setting an already > > > >> down device down via "ip link set dev eth0 down" is a no-op. > > > >> > > > >> So, let's a common scenario. You power up a board. The PHY comes up > > > >> and establishes a link. The boot loader runs, loads the kernel, which > > > > > > > > This may or may not be the case. As you pointed out a few emails back, > > > > this is a system-level issue that requires a system-level solution - > > > > so cutting the link in U-boot is not out of the question. > > > > > > > >> then boots. Your network driver is a module, and hasn't been loaded > > > >> yet. The link is still up. > > > >> > > > >> The modular network driver gets loaded, and initialises. Userspace > > > >> does not bring the network device up, and the network driver does not > > > >> attach or connect to the PHY (which is actually quite common). So, > > > >> the link is still up. > > > >> > > > >> The modular PHY driver gets loaded, and binds to the PHY. The link > > > >> is still up. > > > > > > > > I would rather say, 'even if the link is not up, Linux brings it up > > > > (possibly prematurely) via phy_resume'. > > > > But let's consider the case where the link *was* up. The general idea > > > > is 'implement your workarounds in whatever other way, that link is > > > > welcome!'. > > > > > > With the systems that I work with, we have enforced the following > > > behavior to happen: the boot loader and kernel only turn on what they > > > needs, at the time they need it, and nothing more, once done, they put > > > the blocks back into lowest power mode (clock and power gated if > > > available). So yes, there are multiple link re-negotiations throughput > > > the boot process, but when there is no device bound to a driver the > > > system conserves power by default which is deemed a higher goal than > > > speed. Your mileage may vary of course. > > > > > > There is not exactly a simple way of enforcing that kind (or another > > > kind for that matter) of policy kernel wide, so it's unfortunately up to > > > the driver writer to propose something that is deemed sensible. > > > > > > We could however, extend existing tools like iproute2 to offer the > > > ability to control whether the PHY should be completely powered off, in > > > a low power state allowing WoL, or remain UP when the network device is > > > brought down. That would not cover the case that Russell explained, but > > > it would be another monkey wrench you can throw at the system. > > > -- > > > Florian > > > > Hi Florian, > > > > By going to HALTED on phy_stop, the system already achieves what I am > > looking after - although maybe it is an unintended consequence for > > you. > > I'm only trying to make an argument for removing the phy_resume from > > phy_attach_direct now. > > Merely doing that will create problems. You may remember a few emails > ago, we discussed whether the physical PHY state was consistent with > the PHY state machine state. By making that change, the PHY state > machine vs the physical PHY state becomes inconsistent. > > Removing phy_resume() from phy_attach_direct() means that the PHY may > not be in a resumed state at this point, yet we set the PHY state > machine to PHY_READY. When phy_start() is called, the state will be > transitioned to PHY_UP rather than PHY_RESUMING, and we will try to > update the advertisement into the PHY (as I've previously described.) > > If the PHY is powered down, but the state machine transitions from > PHY_READY to PHY_UP state, programming the advertisement will have no > effect, and the PHY will remain in power-down mode. > > So, the PHY state machine state needs to also be set according to the > PHY mode. The same is true for phy_probe(), which also assumes that > the PHY is not powered down. > > > If there was a link, and it doesn't need > > re-negociation, fine, use it in phy_start, but at most leave U-boot to > > put that link down and don't force it up prior to the netdev's > > ndo_open. > > > > Regards, > > -Vladimir > > > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up
Thanks for explaining this to me in a neutral way. I need to take some time to figure whether changing the state machine has any value for anyone. Regards, -Vladimir
