Hi Heiner,

On Wed, 8 May 2024 07:44:22 +0200
Heiner Kallweit [off-list ref] wrote:

On 07.05.2024 12:28, Maxime Chevallier wrote:

quoted

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/ (local)
Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/ (local)
---
 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 :)

quoted

+		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.

quoted

 #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

quoted

+}
+
 /**
  * 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.

quoted

 	/*  Compatibility with error handling in drivers */
 	if (dev->reg_state == NETREG_UNINITIALIZED ||