Messages in this thread Patch in this message | | | Date | Wed, 8 May 2024 07:44:22 +0200 | From | Heiner Kallweit <> | Subject | Re: [PATCH net-next 2/2] net: phy: phy_link_topology: Lazy-initialize the link topology |
| |
On 07.05.2024 12:28, Maxime Chevallier wrote: > Having the net_device's init path for the link_topology depend on > IS_REACHABLE(PHYLIB)-protected helpers triggers errors when modules are being > built with phylib as a module as-well, as they expect netdev->link_topo > to be initialized. > > Move the link_topo initialization at the first PHY insertion, which will > both improve the memory usage, and make the behaviour more predicatble > and robust. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > Fixes: 6916e461e793 ("net: phy: Introduce ethernet link topology representation") > 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 | 31 ++++++--------------- > include/linux/netdevice.h | 2 ++ > include/linux/phy_link_topology.h | 23 ++++++++-------- > include/linux/phy_link_topology_core.h | 23 +++------------- > net/core/dev.c | 38 ++++++++++++++++++++++---- > 5 files changed, 58 insertions(+), 59 deletions(-) > > diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c > index 0e36bd7c15dc..b1aba9313e73 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 net_device *dev, > struct phy_device *phy, > enum phy_upstream upt, void *upstream) > @@ -43,6 +20,14 @@ int phy_link_topo_add_phy(struct net_device *dev, > struct phy_device_node *pdn; > int ret; > > + if (!topo) { > + ret = netdev_alloc_phy_link_topology(dev);
This function is implemented in net core, but used only here. So move the implementation here?
> + if (ret) > + return ret; > + > + topo = dev->link_topo; > + } > + > pdn = kzalloc(sizeof(*pdn), GFP_KERNEL); > if (!pdn) > return -ENOMEM; > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index cf261fb89d73..25a0a77cfadc 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -4569,6 +4569,8 @@ void __hw_addr_unsync_dev(struct netdev_hw_addr_list *list, > const unsigned char *)); > void __hw_addr_init(struct netdev_hw_addr_list *list); > > +int netdev_alloc_phy_link_topology(struct net_device *dev); > + > /* Functions used for device addresses handling */ > void dev_addr_mod(struct net_device *dev, unsigned int offset, > const void *addr, size_t len); > diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h > index 166a01710aa2..3501f9a9e932 100644 > --- a/include/linux/phy_link_topology.h > +++ b/include/linux/phy_link_topology.h > @@ -32,10 +32,12 @@ struct phy_device_node { > struct phy_device *phy; > }; > > -struct phy_link_topology { > - struct xarray phys; > - u32 next_phy_index; > -}; > +#if IS_ENABLED(CONFIG_PHYLIB) > +int phy_link_topo_add_phy(struct net_device *dev, > + struct phy_device *phy, > + enum phy_upstream upt, void *upstream); > + > +void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy); > > static inline struct phy_device > *phy_link_topo_get_phy(struct net_device *dev, u32 phyindex) > @@ -53,13 +55,6 @@ static inline struct phy_device > return NULL; > } > > -#if IS_REACHABLE(CONFIG_PHYLIB) > -int phy_link_topo_add_phy(struct net_device *dev, > - struct phy_device *phy, > - enum phy_upstream upt, void *upstream); > - > -void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy); > - > #else > static inline int phy_link_topo_add_phy(struct net_device *dev, > struct phy_device *phy, > @@ -72,6 +67,12 @@ static inline void phy_link_topo_del_phy(struct net_device *dev, > struct phy_device *phy) > { > } > + > +static inline struct phy_device * > +phy_link_topo_get_phy(struct net_device *dev, u32 phyindex) > +{ > + return NULL; > +} > #endif > > #endif /* __PHY_LINK_TOPOLOGY_H */ > diff --git a/include/linux/phy_link_topology_core.h b/include/linux/phy_link_topology_core.h > index 0a6479055745..f9c0520806fb 100644 > --- a/include/linux/phy_link_topology_core.h > +++ b/include/linux/phy_link_topology_core.h > @@ -2,24 +2,9 @@ > #ifndef __PHY_LINK_TOPOLOGY_CORE_H > #define __PHY_LINK_TOPOLOGY_CORE_H > > -struct phy_link_topology; > - > -#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 > - > -static inline struct phy_link_topology *phy_link_topo_create(struct net_device *dev) > -{ > - return NULL; > -} > - > -static inline void phy_link_topo_destroy(struct phy_link_topology *topo) > -{ > -} > - > -#endif > +struct phy_link_topology { > + struct xarray phys; > + u32 next_phy_index; > +}; > This is all which is left in this header. As this header is public anyway, better move this definition to phy_link_topology.h?
> #endif /* __PHY_LINK_TOPOLOGY_CORE_H */ > diff --git a/net/core/dev.c b/net/core/dev.c > index d2ce91a334c1..1b4ffc273a04 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -10256,6 +10256,35 @@ static void netdev_do_free_pcpu_stats(struct net_device *dev) > } > } > > +int netdev_alloc_phy_link_topology(struct net_device *dev) > +{ > + struct phy_link_topology *topo; > + > + topo = kzalloc(sizeof(*topo), GFP_KERNEL); > + if (!topo) > + return -ENOMEM; > + > + xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1); > + topo->next_phy_index = 1; > + > + dev->link_topo = topo; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(netdev_alloc_phy_link_topology); > + > +static void netdev_free_phy_link_topology(struct net_device *dev) > +{ > + struct phy_link_topology *topo = dev->link_topo; > + > + if (!topo) > + return; > + > + xa_destroy(&topo->phys); > + kfree(topo); > + dev->link_topo = NULL;
Give the compiler a chance to remove this function if CONFIG_PHYLIB isn't enabled.
if (IS_ENABLED(CONFIG_PHYLIB) && topo) { xa_destroy(&topo->phys); kfree(topo); dev->link_topo = NULL; }
> +} > + > /** > * register_netdevice() - register a network device > * @dev: device to register > @@ -10998,11 +11027,6 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, > #ifdef CONFIG_NET_SCHED > hash_init(dev->qdisc_hash); > #endif > - dev->link_topo = phy_link_topo_create(dev); > - if (IS_ERR(dev->link_topo)) { > - dev->link_topo = NULL; > - goto free_all; > - } > > dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM; > setup(dev); > @@ -11092,7 +11116,9 @@ void free_netdev(struct net_device *dev) > free_percpu(dev->xdp_bulkq); > dev->xdp_bulkq = NULL; > > - phy_link_topo_destroy(dev->link_topo); > +#if IS_ENABLED(CONFIG_PHYLIB) > + netdev_free_phy_link_topology(dev); > +#endif > Then the conditional compiling can be removed here.
> /* Compatibility with error handling in drivers */ > if (dev->reg_state == NETREG_UNINITIALIZED ||
| |