netdev - Re: Cutting the link on ndo_stop - phy_stop or 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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Jun 2019 00:26:24 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ioana Ciornei <ioana.ciornei@....com>
Subject: Re: Cutting the link on ndo_stop - phy_stop or phy_disconnect?

On Tue, 4 Jun 2019 at 23:57, Florian Fainelli <f.fainelli@...il.com> wrote:
>
>
>
> On 6/4/2019 1:42 PM, Vladimir Oltean wrote:
> > On Tue, 4 Jun 2019 at 23:07, Andrew Lunn <andrew@...n.ch> wrote:
> >>
> >> On Tue, Jun 04, 2019 at 10:58:41PM +0300, Vladimir Oltean wrote:
> >>> Hi,
> >>>
> >>> I've been wondering what is the correct approach to cut the Ethernet link
> >>> when the user requests it to be administratively down (aka ip link set dev
> >>> eth0 down).
> >>> Most of the Ethernet drivers simply call phy_stop or the phylink equivalent.
> >>> This leaves an Ethernet link between the PHY and its link partner.
> >>> The Freescale gianfar driver (authored by Andy Fleming who also authored the
> >>> phylib) does a phy_disconnect here. It may seem a bit overkill, but of the
> >>> extra things it does, it calls phy_suspend where most PHY drivers set the
> >>> BMCR_PDOWN bit. Only this achieves the intended purpose of also cutting the
> >>> link partner's link on 'ip link set dev eth0 down'.
> >>
> >> Hi Vladimir
> >>
> >> Heiner knows the state machine better than i. But when we transition
> >> to PHY_HALTED, as part of phy_stop(), it should do a phy_suspend().
> >>
> >>    Andrew
> >
> > Hi Andrew, Florian,
> >
> > Thanks for giving me the PHY_HALTED hint!
> > Indeed it looks like I conflated two things - the Ehernet port that
> > uses phy_disconnect also happens to be connected to a PHY that has
> > phy_suspend implemented. Whereas the one that only does phy_stop is
> > connected to a PHY that doesn't have that... I thought that in absence
> > of .suspend, the PHY library automatically calls genphy_suspend.
>
> It does not fallback to genphy_suspend(), maybe we should change that,
> setting BMCR.PDOWN is a good power saving in itself, if the PHY can do
> more, you have to implement a .suspend() callback to get the additional
> power savings.
>
> > Oh well, looks like it doesn't. So of course, phy_stop calls phy_suspend
> > too.
> > But now the second question: between a phy_connect and a phy_start,
> > shouldn't the PHY be suspended too? Experimentally it looks like it
> > still isn't.
> > By the way, Florian, yes, PHY drivers that use WOL still set
> > BMCR_ISOLATE, which cuts the MII-side, so that's ok. However that's
> > not the case here - no WOL.
>
> I was just responding about what is IMHO sensible to do. There is an
> additional caveat/use case to possibly consider which I am sure some
> drivers intentionally support (or not), which is that bringing down the
> interface does stop the PHY state machine and then you are free to issue
> whatever SIO{S,G}MIIREG for diagnostics etc. Whether the MAC or PHYLIB
> is responsible for taking the PHY out of power down mode, or if it is up
> to the diagnostics software to do that is up for debate, I would go with
> the latter, which would always work regardless of what you are trying to do.

Well in that case the "diagnostics software" (we're probably all
looking at phytool) should just unset the ISOLATE/PDOWN bits...
The netdev is still going to have carrier off either way...

> --
> Florian

-Vladimir