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 16:58:23 +0300
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     Ioana Ciornei <ciorneiioana@gmail.com>,
        Florian Fainelli <f.fainelli@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>,
        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: <20230809135702.4dencx4ikij7d33y@skbuf>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230804071757.383971-1-u.kleine-koenig@pengutronix.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?Y+NrEOW2VzWLgI8BWCl+zs9uZ6MbFhWSAp+ZInMnOZ1VUuIdtcuQk7kZtW?=
 =?iso-8859-1?Q?QZLK4QgLZE9PpvzmN9pJVUi6fYRx/ExhyujXfltAJb5GvhGtsUlVLB3zBT?=
 =?iso-8859-1?Q?h1W6TUQUjllP2MsmM+WEY1qU0Yrf3lLSz11daUvBjoEcupXpeLZeZs95HA?=
 =?iso-8859-1?Q?cFcAid8AnSQdPUZro4FDkvYs4+FBaVXgTJ+7rFmDz2LtTjg3lWeCzRqnE7?=
 =?iso-8859-1?Q?8mSEtfE253AcOp3UnET1mmxufnjCBGuQm5tsT01sC1iezmwG7SD4hJCxMq?=
 =?iso-8859-1?Q?qOMW+BAY62vwhe0lHYE9m6/1/MSOoiw5pEQccyZ1l03MEFPtIcL5fLh5Vs?=
 =?iso-8859-1?Q?slH3TmyOueoUOflwLIwdCoJZwIZXm2R4LnM74jsvq5xeTWKJvFk7JLKqmE?=
 =?iso-8859-1?Q?yWYvUhJ5MMkjzN1IisXFzcLO+/mEjoZlHStHwkL6J79eJEvGkqDiFwmsRd?=
 =?iso-8859-1?Q?jiVX6p+fa9kD3JUILKHNg4kwR9OvUzwJnqNBtBLxskZN63raFGSgIkr5hg?=
 =?iso-8859-1?Q?XbEBlpBXUsIS/uIEA1sgN/H58DU7WvFZDf9aaF0afQfeAWiTWT19UqlI3E?=
 =?iso-8859-1?Q?J7JL683UebvE3vvTauNMx74HWAzG5QlsKiHMhHKFFZx5SgUxYFAGVeWHbQ?=
 =?iso-8859-1?Q?+Zbtote2qMNpqYyqIfuCugruWngoS5ZqoXSumMjASVk4ab71Tb0Hn0/Uxb?=
 =?iso-8859-1?Q?f0/JZZZ292BZl3nUxsmzkxE3qXZCDlBzegnXPT0vSdDx8Qooz3gW1tDxyU?=
 =?iso-8859-1?Q?4omjudlpM9QYkaS9vX+QlgfKtdJ8BEjozxwQXG3Y9mlACaB3tiXpUx/E1d?=
 =?iso-8859-1?Q?/GcrPYAwyLASRtEPVs8TT6+BP0UQUKskKrqBc8zKfUHoUTVwKvczpxCaJZ?=
 =?iso-8859-1?Q?YfCxSGUeSvtMNq3tbjFe2ltrTjey6mRzp+jQoVhRnMuRJqgcdTCLrf9GV6?=
 =?iso-8859-1?Q?L8QP/p76W1cUl24ybDFZU+stfVTxCvn0pJ0RkXRonAxDgD4z7OOidYO2Aw?=
 =?iso-8859-1?Q?7FDZbFZf8BfTn5DEW23j7MuYHZIEr7bxTjwH290/OJbYiPwCaW8lcp2AEk?=
 =?iso-8859-1?Q?cUHdYMPlQtffFVv0DpsjaacvMgO+YwZZAzH3JzybzIjAhjxPFBdKQLmPyx?=
 =?iso-8859-1?Q?QBnDHcj6gqPCUjsphwn5ToxLLq4qUM0rUrHRiJDLpoT9GMBn2MNjOxcB46?=
 =?iso-8859-1?Q?THVOqIZ9DnbDbZH/ONRjEgCDkuqo0OtPpmVjo+ZC2cwGQlPg0KTpdRub8G?=
 =?iso-8859-1?Q?skkVHRe2O71riv0dz3WWdaQ1NCTTFTj9CdROlVVIjc8g0LKIm9O+uqQFC4?=
 =?iso-8859-1?Q?a4fJTi3MY8ih13pkublUR02zlYX/2/gA57e4UMtdxLgnsiW3WyJ1RZRob2?=
 =?iso-8859-1?Q?xFd3+bRX2XE7RWPpTxu+cVSIv7NNMW1dcH68PbBFqEPzezhvyQsL+oz6Gx?=
 =?iso-8859-1?Q?Rdm/MTTeRLTZb0zl/RAnr9afZjX0koK4rt3h1UDDsSnROttAVYIZmRZ4O+?=
 =?iso-8859-1?Q?etVsQPFuWOK2bENewgB4sIau0vSYBK/kAABakXKw+zJkwIQAydkHemy37C?=
 =?iso-8859-1?Q?d5Y/mZW/ebNp/98ho2tJV5bfWTbdkwIYMEKa4DspbTvLmtJt2ZrdGDOVI9?=
 =?iso-8859-1?Q?wNuIxzjG+xDJrGL8h6kMBobQWzujfzX1elTlIW1rgSROjSDn5TBKQcEg?=
 =?iso-8859-1?Q?=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: de48ae2d-4260-46e6-cb58-08db98e0b51f
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6452.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Aug 2023 13:58:28.3924
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: m/mCLhE0ZKnuAWJ8y+aR1HlazOlmvEUAUGvo8Zkmq+y8Pc/VJ/D4ax2O09hSomIYteqA53HCPusM4E0VxGbMEg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR04MB9709
X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIM_INVALID,
        DKIM_SIGNED,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.

Confirmed this way:

$ ethtool -s end0 wol g # &enet0 in the device tree
$ reboot -f
Rebooting.
[  288.682444] Qualcomm Atheros AR8031/AR8033 mdio@2d24000:02: phy_shutdown: wol_enabled 0
[  288.690935] Qualcomm Atheros AR8031/AR8033 mdio@2d24000:01: phy_shutdown: wol_enabled 0
[  288.736145] reboot: Restarting system

This is on the arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts, the same board
as the one which Ioana worked on (with the shared AR8031 PHY interrupts).

Sure, it needs to be mentioned that WoL + shared PHY interrupts is not
by any means an impossible combination, but it still won't work reliably.
I guess that's ok temporarily, since WoL requires user opt-in, so in the
default configuration, the LS1021A-TSN is not broken by this change (in
its functional variant).

pw-bot: cr