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 C5365C27C53 for ; Sun, 16 Jun 2024 14:04:04 +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:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=l+UzjARY5QneLdidg3fAtVri5ykcOQxvdQ7I86EceL0=; b=omkW3fiNKoyODvwz+xCLt1J9eh ekiqW/HxQk+wH75QhHgyHSkRTydURCORLz0zWvbP95S+/Nck+EAqezbEy3O/8oTBcM9QLvCV+RatS K2OwB8WoFXcj0CxcwvuoAAGamSkHNJZ0KyTB1a9w+miuCVWEQlBRCVz79ehCTmQkxq7xznrixTL7d bgPtmXOuJtaQgrMOl4ENhX729W5916OU6/QW96jRNHvJlfzQAwE4zUfU5p/D3j59yHGERRHL4LSrV MoBb7KGA+UTjnfPFL8tvKiG7cZ/2vYSp73GF4gowkrooFF3pNIdS0rmfiAGR9eliV3q63B8IXqEgh QnH6UCLQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sIqU4-00000007f29-2S2y; Sun, 16 Jun 2024 14:03:52 +0000 Received: from relay2-d.mail.gandi.net ([217.70.183.194]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sIqU0-00000007f0n-3iuW for linux-arm-kernel@lists.infradead.org; Sun, 16 Jun 2024 14:03:51 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id A8C0040005; Sun, 16 Jun 2024 14:03:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1718546623; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=l+UzjARY5QneLdidg3fAtVri5ykcOQxvdQ7I86EceL0=; b=TvQyiG1myWFoEx3lDRhv/zCFqFdKrEOvhBvc0GykO+vhEJo3rORGweBiL8xDodZNL+vNv7 vxEubofWrwUW6yLrjmvjLSGadtmrujbw27y8b/V6pk11mkMa3JmObHyz09DWzipi1vQfMn IUYsHPR1oZuOWc/spHYXbdGy7r5EWG3OIw41rNklV7VmOteIrlF4jGCJ/323AHtGwImjPW ZLpvu39+3g733oZNA7M64lHO6xjJkxv5RZ1zRwgNstojulc19JODDXrAw1y5NlbCyIZI3p EM6/h6IbBdvmllgS5uzJpdUKHmc5oZPz0zJMR6wOlIpjt5s5elzL1CtMH2CQ/g== Date: Sun, 16 Jun 2024 18:02:31 +0200 From: Maxime Chevallier To: Jakub Kicinski Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com, Andrew Lunn , Eric Dumazet , Paolo Abeni , Russell King , linux-arm-kernel@lists.infradead.org, Christophe Leroy , Herve Codina , Florian Fainelli , Heiner Kallweit , Vladimir Oltean , =?UTF-8?B?S8O2cnk=?= Maincent , Jesse Brandeburg , Marek =?UTF-8?B?QmVow7pu?= , Piergiorgio Beruto , Oleksij Rempel , =?UTF-8?B?Tmljb2zDsg==?= 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: <20240616180231.338c2e6c@fedora> In-Reply-To: <20240613182613.5a11fca5@kernel.org> References: <20240607071836.911403-1-maxime.chevallier@bootlin.com> <20240607071836.911403-6-maxime.chevallier@bootlin.com> <20240613182613.5a11fca5@kernel.org> Organization: Bootlin X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-GND-Sasl: maxime.chevallier@bootlin.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240616_070349_466554_6479B4EA X-CRM114-Status: GOOD ( 20.56 ) 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 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. Maybe instead we should grab a reference to the phydev when we add it to the topology ? > > > + if (!phydev) { > > + NL_SET_BAD_ATTR(extack, phy_id); > > + return -ENODEV; > > + } > > + } else { > > + /* If we need a PHY but no phy index is specified, fallback > > + * to dev->phydev > > please double check the submission for going over 80 chars, this one > appears to be particularly pointlessly over 80 chars... Arg yes sorry about this one... > > + */ > > + phydev = dev->phydev; Thanks, Maxime