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 3891AC4345F for ; Tue, 30 Apr 2024 13:37:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id: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=iZiclnCqhSSiM4dvcomYFozEkNNLAQZ4AIXxYFE8R4M=; b=C2sRMHmyCDleYI rh/O30D3z81WFrBBgSAkt6NZYOY0G8hoqnKqQYtNVv21OV1fnD6BoDRtT3mcN+OcJBGRxCmXmKR0I 9XSQNbIAglJRTLPlVFkFO9WvH/QWh6K+1m7kuCwo32f5aWYvgUO1DEXIW5m0CP93ABwJicVtJ5f1q E/W7ssJdfsRb4yFNvRSODFstFVx6JcjdXLRgPC3GKPSO5pTEhvlaDVN1VfiUXHEIQRCBg/8DVbevn ZRtxtB/03g8zyVQUcGrqAsrSv4BCrvuC13GOx8gBfJHy0lJtqP7LNAm4x+BU3RFPenqB2AmzSP+dV Y4DYkEE21ECcOW7zxEiw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s1nfP-00000006Z7Z-0GsI; Tue, 30 Apr 2024 13:37:07 +0000 Received: from relay6-d.mail.gandi.net ([2001:4b98:dc4:8::226]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s1nfM-00000006Z5g-0YRh for linux-arm-kernel@lists.infradead.org; Tue, 30 Apr 2024 13:37:06 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id 124CBC0012; Tue, 30 Apr 2024 13:36:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1714484219; 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=gkM9d0xicBmnajscafdTXqIyu0TvtJ/Gq9ScB83e5TE=; b=T+sfuGgU5KSE1xqFD4DqmpJcEqUDQLEqw80ye09yk02ttYXASrPoS7CY4ixjU205l+r48X nuJHFs5/lTNcoWKdNiowEPUs8bSCcwB08eQ1HmHXATvgi/76W/JrmLnu3F/nVx3DtLAFFt 8dWs44KNLxdcvpvuJqSNjfizr8t7ovtI3DUbyFYE+GRCo23dyHA1aoHCbCUb1DD0f0yJFJ vZcjQJt01t2DFxKX2narbYb6vSLMzL4dzbLNyoqJOkZ9IJPI+3bVl8unhPXGcgcMUvblsV K7LuHYnmPQ3EBW3/e21Y46J624gYiHguO/Psw+ot1c6qZA876nSTd1DLkfnsbw== Date: Tue, 30 Apr 2024 15:36:55 +0200 From: Maxime Chevallier To: Heiner Kallweit Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com, Andrew Lunn , Jakub Kicinski , Eric Dumazet , Paolo Abeni , Russell King , linux-arm-kernel@lists.infradead.org, Christophe Leroy , Herve Codina , Florian Fainelli , 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] net: phy: Don't conditionally compile the phy_link_topology creation Message-ID: <20240430153655.2df7a54c@device-28.home> In-Reply-To: <20240430135734.503f51a2@device-28.home> References: <20240429131008.439231-1-maxime.chevallier@bootlin.com> <1ed5b8cb-c79b-44b9-8dbe-f78d7505b3b4@gmail.com> <20240430135734.503f51a2@device-28.home> Organization: Bootlin X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-GND-Sasl: maxime.chevallier@bootlin.com X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240430_063704_514296_AABE3FD5 X-CRM114-Status: GOOD ( 32.98 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, 30 Apr 2024 13:57:34 +0200 Maxime Chevallier wrote: > Hello Heiner, > > On Tue, 30 Apr 2024 10:17:31 +0200 > Heiner Kallweit wrote: > > > On 29.04.2024 15:10, Maxime Chevallier wrote: > > > The core of the phy_link_topology isn't directly tied to phylib, and at > > > the moment it's initialized, phylib might not be loaded yet. Move the > > > initialization of the topology to the phy_link_topology_core header, > > > which contains the bare minimum so that we can initialize it at netdev > > > creation. > > > > > > > The change fixes the issue for me, but according to my personal taste > > the code isn't intuitive and still error-prone. Also there's no good > > reason to inline a function like phy_link_topo_create() and make it > > publicly available. Do you expect it to be ever used outside net core? > > In general it may make sense to add a config symbol for the topology > > extension, there seem to be very few, specialized use cases for it. > > I think I'm missing the point here then. Do you mean adding a Kconfig > option to explicitely turn phy_link_topology on ? or build it as a > dedicated kernel module ? > > Or do you see something such as "if phylib is M or Y, then build the > topology stuff and make sure it's allocated when a netdev gets created > ?" I've prototyped something that's cleaner and should fit what you described, which is to have a Kconfig option for phy_topology and have it autoselected by CONFIG_SFP (for now, the only case where we can have multiple PHYs on the link). When phy mux support is added (I'll followup with that once the topology is settled), we can also make is select the phy_topology config option. I'll send that patch when I'll have properly tested it, especially with all the different bits (phylib, sfp, drivers) being tested as modules or builtin. Thanks for the tips, Maxime > > Thanks, > > Maxime > > > > > > Signed-off-by: Maxime Chevallier > > > Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/ > > > Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/ > > > --- > > > drivers/net/phy/phy_link_topology.c | 23 -------------------- > > > include/linux/phy_link_topology.h | 5 ----- > > > include/linux/phy_link_topology_core.h | 30 +++++++++++++++++--------- > > > 3 files changed, 20 insertions(+), 38 deletions(-) > > > > > > diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c > > > index 985941c5c558..960aedd73308 100644 > > > --- a/drivers/net/phy/phy_link_topology.c > > > +++ b/drivers/net/phy/phy_link_topology.c > > > @@ -12,29 +12,6 @@ > > > #include > > > #include > > > > > > -struct phy_link_topology *phy_link_topo_create(struct net_device *dev) > > > -{ > > > - struct phy_link_topology *topo; > > > - > > > - topo = kzalloc(sizeof(*topo), GFP_KERNEL); > > > - if (!topo) > > > - return ERR_PTR(-ENOMEM); > > > - > > > - xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1); > > > - topo->next_phy_index = 1; > > > - > > > - return topo; > > > -} > > > - > > > -void phy_link_topo_destroy(struct phy_link_topology *topo) > > > -{ > > > - if (!topo) > > > - return; > > > - > > > - xa_destroy(&topo->phys); > > > - kfree(topo); > > > -} > > > - > > > int phy_link_topo_add_phy(struct phy_link_topology *topo, > > > struct phy_device *phy, > > > enum phy_upstream upt, void *upstream) > > > diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h > > > index 6b79feb607e7..ad72d7881257 100644 > > > --- a/include/linux/phy_link_topology.h > > > +++ b/include/linux/phy_link_topology.h > > > @@ -32,11 +32,6 @@ struct phy_device_node { > > > struct phy_device *phy; > > > }; > > > > > > -struct phy_link_topology { > > > - struct xarray phys; > > > - u32 next_phy_index; > > > -}; > > > - > > > static inline struct phy_device * > > > phy_link_topo_get_phy(struct phy_link_topology *topo, u32 phyindex) > > > { > > > diff --git a/include/linux/phy_link_topology_core.h b/include/linux/phy_link_topology_core.h > > > index 0a6479055745..0116ec49cd1b 100644 > > > --- a/include/linux/phy_link_topology_core.h > > > +++ b/include/linux/phy_link_topology_core.h > > > @@ -2,24 +2,34 @@ > > > #ifndef __PHY_LINK_TOPOLOGY_CORE_H > > > #define __PHY_LINK_TOPOLOGY_CORE_H > > > > > > -struct phy_link_topology; > > > +#include > > > > > > -#if IS_REACHABLE(CONFIG_PHYLIB) > > > - > > > -struct phy_link_topology *phy_link_topo_create(struct net_device *dev); > > > -void phy_link_topo_destroy(struct phy_link_topology *topo); > > > - > > > -#else > > > +struct phy_link_topology { > > > + struct xarray phys; > > > + u32 next_phy_index; > > > +}; > > > > > > static inline struct phy_link_topology *phy_link_topo_create(struct net_device *dev) > > > { > > > - return NULL; > > > + struct phy_link_topology *topo; > > > + > > > + topo = kzalloc(sizeof(*topo), GFP_KERNEL); > > > + if (!topo) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1); > > > + topo->next_phy_index = 1; > > > + > > > + return topo; > > > } > > > > > > static inline void phy_link_topo_destroy(struct phy_link_topology *topo) > > > { > > > -} > > > + if (!topo) > > > + return; > > > > > > -#endif > > > + xa_destroy(&topo->phys); > > > + kfree(topo); > > > +} > > > > > > #endif /* __PHY_LINK_TOPOLOGY_CORE_H */ > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel