Messages in this thread | | | Date | Thu, 2 May 2024 09:05:53 +0200 | From | Maxime Chevallier <> | Subject | Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation |
| |
Hi,
On Tue, 30 Apr 2024 23:29:37 +0200 Heiner Kallweit <hkallweit1@gmail.com> wrote:
> On 30.04.2024 13:57, Maxime Chevallier wrote: > > Hello Heiner, > > > > On Tue, 30 Apr 2024 10:17:31 +0200 > > Heiner Kallweit <hkallweit1@gmail.com> 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 <maxime.chevallier@bootlin.com> > >>> 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 <linux/rtnetlink.h> > >>> #include <linux/xarray.h> > >>> > >>> -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 <linux/xarray.h> > >>> > >>> -#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
| |