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;
Message-ID: <50f0b253-8eae-a4bd-0c5d-d23b020ce8df@gmail.com>
Date:   Wed, 9 Aug 2023 08:35:24 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] net: phy: Don't disable irqs on shutdown if WoL is
 enabled
To:     Vladimir Oltean <vladimir.oltean@nxp.com>,
        =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     Ioana Ciornei <ciorneiioana@gmail.com>,
        "Russell King (Oracle)" <linux@armlinux.org.uk>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Ioana Ciornei <ioana.ciornei@nxp.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>
References: <20230809135702.4dencx4ikij7d33y@skbuf>
Content-Language: en-US
From:   Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <20230809135702.4dencx4ikij7d33y@skbuf>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 8/9/2023 6:58 AM, Vladimir Oltean wrote:
> Hi Uwe,
> 
> (I hope the threading won't be broken)
> 
> On Fri, Aug 04, 2023 at 09:17:57AM +0200, Uwe Kleine-König wrote:
>> Most PHYs signal WoL using an interrupt. So disabling interrupts breaks
>> WoL at least on PHYs covered by the marvell driver. So skip disabling
>> irqs on shutdown if WoL is enabled.
>>
>> While at it also explain the motivation that irqs are disabled at all.
>>
>> Fixes: e2f016cf7751 ("net: phy: add a shutdown procedure")
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>> Hello,
>>
>> while I'm not sure that disabling interrupts is a good idea in general,
>> this change at least should fix the WoL case. Note that this change is
>> only compile tested as next doesn't boot on my test machine (because of
>> https://git.kernel.org/linus/b3574f579ece24439c90e9a179742c61205fbcfa)
>> and 6.1 (which is the other kernel I have running) doesn't know about
>> .wol_enabled. I don't want to delay this fix until I bisected this new
>> issue.
>>
>> Assuming this patch is eligible for backporting to stable, maybe point
>> out that it depends on v6.5-rc1~163^2~286^2~2 ("net: phy: Allow drivers
>> to always call into ->suspend()"). Didn't try to backport that.
>>
>> Best regards
>> Uwe
>>
>>   drivers/net/phy/phy_device.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 61921d4dbb13..6d1526bdd1d7 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -3340,6 +3340,15 @@ static void phy_shutdown(struct device *dev)
>>   	if (phydev->state == PHY_READY || !phydev->attached_dev)
>>   		return;
>>   
>> +	/* Most phys signal WoL via the irq line. So for these irqs shouldn't be
>> +	 * disabled.
>> +	 */
>> +	if (phydev->wol_enabled)
>> +		return;
>> +
>> +	/* On shutdown disable irqs to prevent an irq storm on systems where the
>> +	 * irq line is shared by several devices.
>> +	 */
>>   	phy_disable_interrupts(phydev);
>>   }
>>   
>> -- 
>> 2.40.1
>>
>>
> 
> I think the idea is not bad and something along these lines might be the
> way to go, but I don't think it works (as currently implemented, and
> tested by me, prints below).
> 
> Upon a quick search, phydev->wol_enabled is only set from phy_suspend(),
> and phy_suspend() isn't invoked from the ethnl_set_wol() call stack.

You are right, this was an ill advised suggestion from my side here. In 
principle however we need to have something like this in the shutdown 
routine to have this patch work:

        struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
        struct net_device *netdev = phydev->attached_dev;
        int ret;

        phy_ethtool_get_wol(phydev, &wol);
        phydev->wol_enabled = wol.wolopts || (netdev && 
netdev->wol_enabled);

	if (!phydev->wol_enabled)
		return;

this does make me wonder whether Uwe tested with a prior system 
suspend/resume cycle before shutting down?
-- 
Florian