| Message ID | 20240507102822.2023826-3-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix phy_link_topology initialization | expand |
Hi Maxime,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/net-phy-phy_link_topology-Pass-netdevice-to-phy_link_topo-helpers/20240507-183130
base: net-next/main
patch link: https://lore.kernel.org/r/20240507102822.2023826-3-maxime.chevallier%40bootlin.com
patch subject: [PATCH net-next 2/2] net: phy: phy_link_topology: Lazy-initialize the link topology
config: parisc-defconfig (https://download.01.org/0day-ci/archive/20240508/202405080732.pSwJSarc-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/202405080732.pSwJSarc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405080732.pSwJSarc-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> net/core/dev.c:10276:13: warning: 'netdev_free_phy_link_topology' defined but not used [-Wunused-function]
10276 | static void netdev_free_phy_link_topology(struct net_device *dev)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/netdev_free_phy_link_topology +10276 net/core/dev.c
10275
10276 static void netdev_free_phy_link_topology(struct net_device *dev)
10277 {
10278 struct phy_link_topology *topo = dev->link_topo;
10279
10280 if (!topo)
10281 return;
10282
10283 xa_destroy(&topo->phys);
10284 kfree(topo);
10285 dev->link_topo = NULL;
10286 }
10287
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 ||
Hi Heiner, On Wed, 8 May 2024 07:44:22 +0200 Heiner Kallweit <hkallweit1@gmail.com> wrote: > 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 it's OK not to have both helpers to alloc and destroy in different files, then I'll move it :) > > > + 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? Well I'll have to include the whole phy_link_topology.h in net/core/dev.c, and I was trying to avoid including that whole header, and keep the included content to a bare minimum. > > > #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; > } Well if we add more things to the link topology, then it's going to be easy to forget updating that without clear helpers for alloc/destroy, don't you think ? I can try to squeeze another iteration before net-next closes. Maxime > > +} > > + > > /** > > * 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 || > > > >
Hello again Heiner, On Wed, 8 May 2024 07:44:22 +0200 Heiner Kallweit <hkallweit1@gmail.com> wrote: > 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. I agree with some of the comments, as stated in my previous mail, however I'm struggling to find the time to fix, and re-test everything, especially before net-next closes. Would it be OK if I re-send with a fix for the kbuild bot warning, improve the commit log as you mentionned for patch 1 so that at least the issue can be solved ? I still have the netlink part of this work to send, so I definitely will have to rework that, but with a bit less time constraints so that I can properly re-test everything. Best regards, Maxime
On Mon, 13 May 2024 10:07:29 +0200 Maxime Chevallier <maxime.chevallier@bootlin.com> wrote: > Hello again Heiner, > > On Wed, 8 May 2024 07:44:22 +0200 > Heiner Kallweit <hkallweit1@gmail.com> wrote: > > > 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. > > I agree with some of the comments, as stated in my previous mail, > however I'm struggling to find the time to fix, and re-test everything, > especially before net-next closes. Would it be OK if I re-send with a > fix for the kbuild bot warning, improve the commit log as you > mentionned for patch 1 so that at least the issue can be solved ? > > I still have the netlink part of this work to send, so I definitely > will have to rework that, but with a bit less time constraints so that > I can properly re-test everything. To clarify, I'm mostly talking about the merge of phy_link_topology_core.h into phy_link_topology.h, I fear that this could get rejected because of the added #include that would clutter a bit net/core/dev.c with functions that are barely used. All your other comments make perfect sense to me and I'm testing these as we speak. Regards, Maxime > > Best regards, > > Maxime
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); + 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; +}; #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; +} + /** * 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 /* Compatibility with error handling in drivers */ if (dev->reg_state == NETREG_UNINITIALIZED ||
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(-)