From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CA743C27C53 for ; Sun, 16 Jun 2024 16:16:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=DI8ao85LkW0rGY8C34vKTHTfc1vvHRa0HNhrgN4f+Qs=; b=g1af/OrnkR+B81WJTDpd3fev/I sq0ejgyfEUonAvD9f5VSEl9hSDMxqPOI/5qmwEfYtf27KwsijOb86AmZaZwioTczZbLQu95O05Vt6 u5P9MTQSiOib62QMgNmJ1lVT2XiRBncYBHz8sLLHZTjgBe9Mqb4Jo4OXzUSHzYCCakMbAGqzD3gqh f4yaePQgoev8kluSwYpfZRfNDgKwF55b2JJbMZimszIWaMU694b4VQV2/b1P1NKC3jpZ8trwOj0KS 8YPJuuacBrS8JoGt0W5qt9f2/+k/2jR7hwbKflIKUP+Ubx43B1XqeQNX8ljpCwkMmdNZtCMJZeHnP +oRM0Tzw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sIsXy-00000007ruq-0QLw; Sun, 16 Jun 2024 16:16:02 +0000 Received: from pandora.armlinux.org.uk ([2001:4d48:ad52:32c8:5054:ff:fe00:142]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sIsXu-00000007ruI-3hWF for linux-arm-kernel@lists.infradead.org; Sun, 16 Jun 2024 16:16:00 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=DI8ao85LkW0rGY8C34vKTHTfc1vvHRa0HNhrgN4f+Qs=; b=Ibx6coRhBdGtgLYoRkBMnNKLGC GbXU88eI/XDtIAVwIcgUb97faEjw0h113h7HrNpY7JgR6yHrS0YMzOxVRy/NDsQLQcjRbYCwcS2vW sqa5ny+e8ipXy8qRTwBteJo11z1h0Z9oqVQbtKyFLMnHffAlXBbQo6LQDHnxnPzuZIEFBrOp6Meuw 7d2qG7pvj+3vIcOGglstX2XprraTZPHZcG+0DFc9L3ZoUGZGfhD3F0xWXAwM1LpZdLFbSEpSpSXR6 yv2yMXt+flz4K7vKGH+sTF7qvg/M/QXPvl/MB+3ri2ZLmg3Poei/L3xuexFgoAKO0jMMhFcDICvhl GmESJctQ==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:56890) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1sIsXf-0004F3-0p; Sun, 16 Jun 2024 17:15:43 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1sIsXg-00047j-Vn; Sun, 16 Jun 2024 17:15:45 +0100 Date: Sun, 16 Jun 2024 17:15:44 +0100 From: "Russell King (Oracle)" To: Maxime Chevallier Cc: Jakub Kicinski , davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com, Andrew Lunn , Eric Dumazet , Paolo Abeni , linux-arm-kernel@lists.infradead.org, Christophe Leroy , Herve Codina , Florian Fainelli , Heiner Kallweit , Vladimir Oltean , =?iso-8859-1?Q?K=F6ry?= Maincent , Jesse Brandeburg , Marek =?iso-8859-1?Q?Beh=FAn?= , Piergiorgio Beruto , Oleksij Rempel , =?iso-8859-1?Q?Nicol=F2?= Veronese , Simon Horman , mwojtas@chromium.org, Nathan Chancellor , Antoine Tenart Subject: Re: [PATCH net-next v13 05/13] net: ethtool: Allow passing a phy index for some commands Message-ID: References: <20240607071836.911403-1-maxime.chevallier@bootlin.com> <20240607071836.911403-6-maxime.chevallier@bootlin.com> <20240613182613.5a11fca5@kernel.org> <20240616180231.338c2e6c@fedora> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240616180231.338c2e6c@fedora> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240616_091558_948480_606C639E X-CRM114-Status: GOOD ( 26.42 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sun, Jun 16, 2024 at 06:02:31PM +0200, Maxime Chevallier wrote: > Hello Jakub, > > On Thu, 13 Jun 2024 18:26:13 -0700 > Jakub Kicinski wrote: > > > On Fri, 7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote: > > > + if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) { > > > + struct nlattr *phy_id; > > > + > > > + phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX]; > > > + phydev = phy_link_topo_get_phy(dev, > > > + nla_get_u32(phy_id)); > > > > Sorry for potentially repeating question (please put the answer in the > > commit message) - are phys guaranteed not to disappear, even if the > > netdev gets closed? this has no rtnl protection > > I'll answer here so that people can correct me if I'm wrong, but I'll > also add it in the commit logs as well (and possibly with some fixes > depending on how this discussion goes) > > While a PHY can be attached to/detached from a netdevice at open/close, > the phy_device itself will keep on living, as its lifetime is tied to > the underlying mdio_device (however phy_attach/detach take a ref on the > phy_device, preventing it from vanishing while it's attached to a > netdev) > > I think the worst that could happen is that phy_detach() gets > called (at ndo_close() for example, but that's not the only possible > call site for that), and right after we manually unbind the PHY, which > will drop its last refcount, while we hold a pointer to it : > > phydev = phy_link_topo_get_phy() > phy_detach(phydev) > unbind on phydev > /* access phydev */ > > PHY device lifetime is, from my understanding, not protected by > rtnl() so should a lock be added, I don't think rtnl_lock() would be > the one to use. ... and that will cause deadlocks. For example, ethernet drivers can call phy_disconnect() from their .ndo_close method, which will be called with the RTNL lock held. This calls phy_detach(), so phy_detach() also gets called while the RTNL lock is held. SFP will call all phylib methods while holding the RTNL lock as well (because that's the only safe way to add or remove a PHY, as it stops other changes to the config that may conflict, and also ensures that e.g. paths in phylib will not be in-use when the PHY is being destroyed.) So, rather than thinking that phylib should add RTNL locking, it would be much more sensible to do what phylink does, and enforce that the RTNL will be held when netdev related methods are called, but also require that paths that end up changing phylib's configuration (e.g. removing a PHY driver) end up taking the RTNL lock - because that is the only way to be sure that none of the phylib methods that call into the driver are currently executing in another thread. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!