Messages in this thread | | | Date | Tue, 30 Apr 2024 10:17:31 +0200 | Subject | Re: [PATCH net-next] net: phy: Don't conditionally compile the phy_link_topology creation | From | Heiner Kallweit <> |
| |
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.
> 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 */
| |