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 8358CC2BD09 for ; Sun, 23 Jun 2024 01:21:52 +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=bPaoqEeTpSx86CMKltblLZ3g74Ywhx8FS7aopirkn84=; b=dQlSCkLuLzjdos64Lg0robwPtx jgfPgqLqH515lbYsE0XR2mC1/E1KidKObCJola1SGVcoLazX+SGYCpG1qDGrlsrABI5m2DvaaoWDm W/hwW9iWCwdrF5MN4GhgSd7MOeSYZoFOikC0OCjb7a9IB21p7VT8GAmC2lk+P4bOo/BAc1C8xX8jI g3NzxYocMsjSF1A3RGvLaUos1RDqT2I43gn/TIqk6EGyGLv4x9xIVgh/LG0ktj89azS2h0IFbsNfR o2rGNJP1+pQu7XvZaiM0dVqgdr/mUiKbXG9ySbSOUtiNqIjsXXkK9UHM9y+vRRfqADbw9AmqrhzJH SXVP3Nfw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sLBvA-0000000D7If-0wnZ; Sun, 23 Jun 2024 01:21:32 +0000 Received: from relay7-d.mail.gandi.net ([2001:4b98:dc4:8::227]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sLBv4-0000000D7HS-3geL for linux-arm-kernel@lists.infradead.org; Sun, 23 Jun 2024 01:21:28 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id A688A20002; Sun, 23 Jun 2024 01:21:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1719105679; 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=bPaoqEeTpSx86CMKltblLZ3g74Ywhx8FS7aopirkn84=; b=EbJTqJW57oSOIYUtca2OpA6QEtF7xPc7grSo0iy5ZUrH+B2/uawpTZJdxXVQGuZviTyiXU 0tpPf+10EPRZDi+0MfETrTnR6jV1utM8ACTbB5o49HvaRJbAq2JL2/byfr/TXqg3Q2PWdC B4PLbio6lwU+NPLCQ56bdJUxE7dXK7HhgKvjdld5TG2ejnd9BZEO9lkeuHEbyA7OMuyaIJ zzi39DjBvDUFg2MRG+UyxnWee2/qVp5ywgD9uyljj6NVlrfdBZS56a9KJC605Dzgr/6leZ a87O3vIDbq/xHYN++6ip+AHWLG0Eswd4l7x3fofdpxUrokct7sB2rkoQsqqWbg== Date: Sun, 23 Jun 2024 03:21:06 +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: <20240623032106.3e854124@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-20240622_182127_245228_7C132DE8 X-CRM114-Status: GOOD ( 16.68 ) 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, Andrew, Russell, 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 After scratching my head maybe a bit too hard and re-reading the replies from Andrew and Russell, I think there's indeed a problem. The SFP case as described by Russell, from my understanding, leads me to believe that the way PHY's are tracked by phy_link_topology is correct, but that doesn't mean that what I try do to in this exact patch is right. After the phydev has been retrieved from the topology and stored in the req_info, nothing guarantees that the PHY won't vanish between the moment we get it here and the moment we use it in the ethnl command handling (SFP removal being a good example, and probably(?) the only problematic case). A solution would be, as Russell says, to make sure we get the PHY and do whatever we need to do with it with rtnl held. Fortunately that shouldn't require significant rework of individual netlink commands that use the phydev, as they already manipulate it while holding rtnl(). So, I'll ditch this idea of storing the phydev pointer in the req_info, I'll just store the phy_index (if it was passed by user) and grab the phy whenever we need to. Let me know if you find some flaw in my analysis, and thanks for spotting this. Best regards, Maxime