linux-kernel - Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

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]

Message-ID: <20220519145936.3ofmmnrehydba7t6@skbuf>
Date:   Thu, 19 May 2022 14:59:36 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Saravana Kannan <saravanak@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        John Stultz <jstultz@...gle.com>,
        Alvin ┼áipraga <alsi@...g-olufsen.dk>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that
 defers probe

Hi Andrew,

On Sat, May 14, 2022 at 02:23:51AM +0200, Andrew Lunn wrote:
> On Sat, May 14, 2022 at 02:36:38AM +0300, Vladimir Oltean wrote:
> > This patch set completes the picture described by
> > '[RFC,devicetree] of: property: mark "interrupts" as optional for fw_devlink'
> > https://patchwork.kernel.org/project/netdevbpf/patch/20220513201243.2381133-1-vladimir.oltean@nxp.com/
> > 
> > I've CCed non-networking maintainers just in case they want to gain a
> > better understanding. If not, apologies and please ignore the rest.
> > 
> > My use case is to migrate a PHY driver from poll mode to interrupt mode
> > without breaking compatibility between new device trees and old kernels
> > which did not have a driver for that IRQ parent, and therefore (for
> > things to work) did not even have that interrupt listed in the "vintage
> > correct" DT blobs. Note that current kernels as of today are also
> > "old kernels" in this description.
> > 
> > Creating some degree of compatibility has multiple components.
> > 
> > 1. A PHY driver must eventually give up waiting for an IRQ provider,
> >    since the dependency is optional and it can fall back to poll mode.
> >    This is currently supported thanks to commit 74befa447e68 ("net:
> >    mdio: don't defer probe forever if PHY IRQ provider is missing").
> > 
> > 2. Before it finally gives up, the PHY driver has a transient phase of
> >    returning -EPROBE_DEFER. That transient phase causes some breakage
> >    which is handled by this patch set, details below.
> > 
> > 3. PHY device probing and Ethernet controller finding it and connecting
> >    to it are async events. When both happen during probing, the problem
> >    is that finding the PHY fails if the PHY defers probe, which results
> >    in a missing PHY rather than waiting for it. Unfortunately there is
> >    no universal way to address this problem, because the majority of
> >    Ethernet drivers do not connect to the PHY during probe. So the
> >    problem is fixed only for the driver that is of interest to me in
> >    this context, DSA, and with special API exported by phylink
> >    specifically for this purpose, to limit the impact on other drivers.
> 
> There is a very different approach, which might be simpler.
> 
> We know polling will always work. And it should be possible to
> transition between polling and interrupt at any point, so long as the
> phylock is held. So if you get -EPROBE_DEFFER during probe, mark some
> state in phydev that there should be an irq, but it is not around yet.
> When the phy is started, and phylib starts polling, look for the state
> and try getting the IRQ again. If successful, swap to interrupts, if
> not, keep polling. Maybe after 60 seconds of polling and trying, give
> up trying to find the irq and stick with polling.

That doesn't sound like something that I'd backport to stable kernels.
Letting the PHY driver dynamically switch from poll to IRQ mode risks
racing with phylink's workqueue, and generally speaking, phylink doesn't
seem to be built around the idea that "bool poll" can change after
phylink_start().

What motivates me to make these changes in the first place is the idea
that current kernels should work with updated device trees. If I won't
be able to achieve that, I see no point in adding logic to transition
from poll to IRQ mode even in net-next, since I'd have to update the
kernel when I update the DT, and by then, I'd have a proper driver for
the IRQ parent anyway. Sorry.