netdev - Re: [PATCH net-next v2 3/3] net: phy: marvell10g: set the PHY in low power by default

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:   Mon, 4 Mar 2019 11:47:00 +0100
From:   Antoine Tenart <antoine.tenart@...tlin.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Antoine Tenart <antoine.tenart@...tlin.com>,
        Andrew Lunn <andrew@...n.ch>, davem@...emloft.net,
        linux@...linux.org.uk, hkallweit1@...il.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        thomas.petazzoni@...tlin.com, maxime.chevallier@...tlin.com,
        gregory.clement@...tlin.com, miquel.raynal@...tlin.com,
        nadavh@...vell.com, stefanc@...vell.com, mw@...ihalf.com
Subject: Re: [PATCH net-next v2 3/3] net: phy: marvell10g: set the PHY in low
 power by default

Hi Florian,

On Fri, Mar 01, 2019 at 07:08:56PM -0800, Florian Fainelli wrote:
> On 3/1/2019 7:07 AM, Antoine Tenart wrote:
> > On Fri, Mar 01, 2019 at 03:19:53PM +0100, Andrew Lunn wrote:
> >> On Fri, Mar 01, 2019 at 12:00:47PM +0100, Antoine Tenart wrote:
> >>> When the Marvell 10G PHYs are set out of reset, the LPOWER bit is set
> >>> depending on an hardware configuration choice. We also do not know what
> >>> is the PHY state at boot time. Hence, set the PHY in low power by
> >>> default when this driver probes.
> >>
> >> Florian did some work for c22 PHYs so that the existing link state
> >> could be used at boot. So for example, the bootloader configured the
> >> PHY up and it got link, there is no need to down/up the PHY when linux
> >> takes control. The networking comes up faster that way.
> >>
> >> Can this work for this PHY?
> > 
> > This use case (the bootloader configures the PHY, Linux boots and sets
> > an interface using this PHY up) would work, and is what's happening in
> > some situations right now (the 3310 reset is never asserted prior to
> > this series).
> > 
> > But consider this case (let's say we use a 10G link):
> > 
> >   ----------------               ----------------
> >   |    Board 1   |               |    Board 2   |
> >   | MAC — 3310 — | — SFP cable — | — 3310 — MAC |
> >   ----------------               ----------------
> > 
> > Board 1: The userspace do not set the interface up. The MAC is in reset
> >          (default state during the MAC driver probe), the PHY was
> > 	 configured by the bootloader.
> > Board 2: The userspace set the interface up. The MAC is configured, the
> >          PHY is configured as well.
> > 
> > The two PHY's PCS will establish a link and report it as being up. In
> > this case, phylink's AN mode is MLO_AN_PHY and thus will report the
> > overall link as being the PHY's link status: up.
> > 
> > My understanding is that the issue arises because the PHYs were never
> > set in reset, or low power, and thus act as if the user wanted the port
> > to be up. As the default behaviour for networking ports is to be down at
> > boot, I thought to set the PHY as well in a default low power state.
> 
> The policy you are creating here for the marvell10g driver is entirely
> applicable to any PHY <=> PHY configuration where either of the two
> software agents on Board 1 or Board 2 has not had a chance to bring-up
> its bootloader/OS/applications to control the PHY.

Right.

> A number of PHYs come up fully on (or in isolate or super isolate mode)
> and will AN with their link partner if connected. For some people it's a
> feature, for some it is a waste of power. I don't necessarily have an
> issue with your patch per-se, but it does create an one off behavior
> that other PHY drivers may not follow.

I agree having a per-driver behaviour is not something we want. As I
understand it, there is no behaviour enforced currently regarding this
matter. I agree both cases have their pros and cons:
- It's weird to have an interface reporting being UP when it's not
  really.
- Having the link come up faster can be a feature.

I have some questions then:
- Do you think calling suspend() in the core when probing a PHY driver
  would work for all PHYs?
- Would a new Kconfig option selecting the default behaviour at boot
  time be a solution?
- Or this is a WONTFIX kind of (small) issues? :)

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com