Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 9 Aug 2023 20:15:27 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Ioana Ciornei <ioana.ciornei@nxp.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Uwe =?utf-8?B?S2xlaW5lLUvilJzDgm5pZw==?= 
        <u.kleine-koenig@pengutronix.de>,
        Ioana Ciornei <ciorneiioana@gmail.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Alexandru Ardelean <alexandru.ardelean@analog.com>,
        Andre Edich <andre.edich@microchip.com>,
        Antoine Tenart <atenart@kernel.org>,
        Baruch Siach <baruch@tkos.co.il>,
        Christophe Leroy <christophe.leroy@c-s.fr>,
        Divya Koppera <Divya.Koppera@microchip.com>,
        Hauke Mehrtens <hauke@hauke-m.de>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Kavya Sree Kotagiri <kavyasree.kotagiri@microchip.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Marco Felsch <m.felsch@pengutronix.de>,
        Marek Vasut <marex@denx.de>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Mathias Kresin <dev@kresin.me>,
        Maxim Kochetkov <fido_max@inbox.ru>,
        Michael Walle <michael@walle.cc>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Nisar Sayed <Nisar.Sayed@microchip.com>,
        Oleksij Rempel <o.rempel@pengutronix.de>,
        Philippe Schenker <philippe.schenker@toradex.com>,
        Willy Liu <willy.liu@realtek.com>,
        Yuiko Oshino <yuiko.oshino@microchip.com>
Subject: Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is
 enabled
Message-ID: <ZNPlzyxYqgpPUn6K@shell.armlinux.org.uk>
References: <20230804071757.383971-1-u.kleine-koenig@pengutronix.de>
 <20230808145325.343c5098@kernel.org>
 <1e438a02-6964-ce65-5584-e8ea57a694bb@gmail.com>
 <ZNLIOEBXNgPOnFSf@shell.armlinux.org.uk>
 <20230809142155.4dtmnmmecaycbtum@LXL00007.wbi.nxp.com>
 <ZNOivVJ+G/sRiwai@shell.armlinux.org.uk>
 <20230809154418.hjkf43ndwfzretiy@LXL00007.wbi.nxp.com>
 <ZNO4RwYzZYUTu1uk@shell.armlinux.org.uk>
 <2f717c52-0ae5-4702-ab34-7ce0bffe8c86@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2f717c52-0ae5-4702-ab34-7ce0bffe8c86@lunn.ch>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Wed, Aug 09, 2023 at 06:21:58PM +0200, Andrew Lunn wrote:
> > Thinking about this, I wonder whether we could solve your issue by
> > disabling interrupts when the PHY is probed, rather than disabling
> > them on shutdown - something like this? (not build tested)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 3e9909b30938..4d1a37487923 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
> >  			goto out;
> >  	}
> >  
> > +        phy_disable_interrupts(phydev);
> > +
> >  	/* Start out supporting everything. Eventually,
> >  	 * a controller will attach, and may modify one
> >  	 * or both of these values
> 
> At some point, the interrupt is going to be enabled again. And then
> the WoL interrupt will fire. I think some PHY drivers actually need to
> see that WoL interrupt. e.g. the marvell driver has this comment:
> 
> static int m88e1318_set_wol(struct phy_device *phydev,
>                             struct ethtool_wolinfo *wol)
> {
> ....
>                 /* If WOL event happened once, the LED[2] interrupt pin
>                  * will not be cleared unless we reading the interrupt status
>                  * register. If interrupts are in use, the normal interrupt
>                  * handling will clear the WOL event. Clear the WOL event
>                  * before enabling it if !phy_interrupt_is_valid()
>                  */
> 
> So it seems like just probing the marvell PHY is not enough to clear
> the WoL interrupt.
> 
> Can we be sure that the other PHY has reached a state it can handle
> and clear an interrupt when we come to enable the interrupt? I think
> not, especially in cases like NFS root, where the interface will be
> put into use as soon as it exists, maybe before the other interface
> has probed.

I suppose the question to Ioana would be - are the two AR8031 PHYs on
the same MDIO bus? If they are, then we're safe, because both will be
probed consecutively (because they're using the same driver.)

I know it would be desirable to have a generic solution to this, but
I don't think that is sanely achievable if we have multiple different
PHYs sharing an interrupt line over multiple different MDIO buses
and multiple different PHY drivers.

So, I'm suggesting we try to do a best-effort solution to solve Ioana's
problem so that we can restore Uwe's WoL behaviour without causing
Ioana's issue to regress. It does mean that if someone has a more weird
setup (such as I describe in my paragraph above) it won't work, but
then it also didn't used to work before Ioana's patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!