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 B96A8C4345F for ; Thu, 2 May 2024 07:06:25 +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=7X8eQLw/ReEM0Ku5DVzYlf9SMy97xaCxvQfdIiILKew=; b=hacjdnWJDq59ts 0f335w6yngyEIgq1GmO03FM38D0hGyLtOmtWxE5Dpy/u7cNBtt3ompVbhLPc/j+wyt7IZ4oUvqb4X f5cD7UL4V6uDL+Kz+XGQj0BplCLAnSE/jgLdJksO2ijxRd+hKTsIiTGNN6hI0DS93osGYFG9wDpDp SezFgS7f58kyZ2oBkf70lcSId3R0alQGpQRID9aHRx0+jhwMscdBoDY2JSKfALjRwcyRoN4J18lN0 a+zl4O3HAOTC2t0ZYyDpKAUCtG41+GOagH2IXXw94hLtoR9HfSPqt7UqCCdZi2VzKxYVdjpLa5t+X lCgUYS1tM/JFbNEhwgiA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s2QW8-0000000BkKH-0QFI; Thu, 02 May 2024 07:06:08 +0000 Received: from relay9-d.mail.gandi.net ([2001:4b98:dc4:8::229]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s2QW2-0000000BkEx-0lmt for linux-arm-kernel@lists.infradead.org; Thu, 02 May 2024 07:06:06 +0000 Received: by mail.gandi.net (Postfix) with ESMTPSA id E4106FF80D; Thu, 2 May 2024 07:05:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1714633557; 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=Sik2Srpo/NMXeuyqGPdiEfnhnvExYt+wCnwEMFyOk40=; b=B8j1f5UuQ+vnS71+EkEfNv+tOaeI+G68TJyKlaSdEUr0+GqduZ45SWwDpOX7X4irVbzNS7 a4LaDpYPb7Jg8J3QlefxyiaPGvJsx8zgrXU5WnqTYAiKEf0KBI0DM4gbsaXEO5pqZbkmeB 3Rn3nZcsmy20kDJ+0AOPvKop4Pyu7Jg5CuCPsMNrRBhwFAo395d0n9BtC8da9mxa6kCq03 VdCexfNiKI7V0TYnqpbWJRLr6L0YiwNdVw8RHR+bGV6snnXZlCOfw9vvdO9cuqPbYwdQLx N3kQO+6SagPNJ0JLlsU4WG07zz2n5Eipa7j+1TRwajgFBFYld3xtZKV/SGhUkQ== Date: Thu, 2 May 2024 09:05:53 +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: <20240502090553.485bf36b@device-28.home> In-Reply-To: <2422613d-0f9c-4485-bad3-5aa7cf12c0b1@gmail.com> References: <20240429131008.439231-1-maxime.chevallier@bootlin.com> <1ed5b8cb-c79b-44b9-8dbe-f78d7505b3b4@gmail.com> <20240430135734.503f51a2@device-28.home> <2422613d-0f9c-4485-bad3-5aa7cf12c0b1@gmail.com> 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-20240502_000603_271781_4ACC7621 X-CRM114-Status: GOOD ( 37.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 Hi, On Tue, 30 Apr 2024 23:29:37 +0200 Heiner Kallweit wrote: > On 30.04.2024 13:57, 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 > > ?" > > > > 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 */ > >> > > > > To go a little bit more into detail: > > phy_link_topo_create() and phy_link_topo_destroy() are used in net/core/dev.c > only. Do you expect them to be ever used by other callers? > If not, their functionality could be moved to net/core/dev.c. > Supposedly guarded by IS_ENABLED(CONFIG_PHYLIB), alternatively a new config > symbol for link_topo support. > > To get rid of the dependency you could also lazy-inizialize > netdev->link_topo. For this phy_link_topo_add_phy() would have > to take the netdev as first argument, not the topo. > Then the first call to phy_link_topo_add_phy() would initialize > netdev->link_topo. > > I think functions like phy_link_topo_get_phy() should also check for > topo != NULL first, maybe combined with a WARN_ON(). > They are exported and you have no control over its use. Thanks Heiner for the explanations. I'll rework based on that. The original reason I didn't directly include the netdev as a parameter for these function, or didn't put any helper in net/core/dev.c is because I wanted to avoid too strong of a link between the topology and netdev. There are some PHYs for which we can't assign any netdev (PHYs that would sit in-between 2 chained DSA switches is the only example I have in mind though), but TBH the code in its actual shape doesn't address these either, so it's a useless design constraint. So, let me indeed do that, it will probably make for a simpler and more straightforward design. Thanks for the input, Maxime _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel