Re: [PATCH net-next v5 01/13] net: phy: Introduce ethernet link topology representation

Hi Jakub,

On Thu, 4 Jan 2024 15:12:42 -0800
Jakub Kicinski [off-list ref] wrote:

On Thu, 21 Dec 2023 19:00:34 +0100 Maxime Chevallier wrote:

quoted

@@ -2441,6 +2442,7 @@ struct net_device {
 #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
 	struct netprio_map __rcu *priomap;
 #endif
+	struct phy_link_topology	link_topo;  

Perhaps others would disagree but can we make this a pointer instead?
Only allocate it on demand, when first PHY gets attached?
Both saves space and netdevice.h will no longer need to know the
definition of the struct.

Complete noob question but I thought PHYs get attached at ndo_open
time for drivers, don't they? We shouldn't want to re-ID in that case.

I'll give it a try, it could be doable to try to keep the index for
_some_ PHYs. I had a first try at this in the very first iteration, but
that was lost when converting to xarray for the index management. Since
the phy_device keeps track of its own index, we can try to re-use it,
but I can see it becoming difficult if we destroy the topology when all
PHYs are detached (after ndo_close).

When re-creating the xarray at the next ndo_open, we would need to know
what was the previous next_index, and we can't know that until PHYs are
all re-attached.

For now it would work because in most cases, we only have one PHY
behind the MAC we can guarantee won't go away, but there are devices
out-there with 2 PHYs connected to the same MAC with a MUX in the
middle (that's one thing this series prepares for), and here it would
be more complex to re-allocate the topology while knowing what's the
next unused index. Hope that makes sense :)

To summarize, I don't think we can easily both dynamically
allocate/destroy the topology based only on the presence of at least
one PHY, and try to keep the PHY index the same for non-hot-pluggable
PHYs.

quoted

 	struct phy_device	*phydev;
 	struct sfp_bus		*sfp_bus;
 	struct lock_class_key	*qdisc_tx_busylock;  

quoted

@@ -10872,6 +10873,8 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 #ifdef CONFIG_NET_SCHED
 	hash_init(dev->qdisc_hash);
 #endif
+	phy_link_topo_init(&dev->link_topo);
+
 	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
 	setup(dev);
   

I think you're missing a call to xa_destroy() somewhere, no?

Arg you're right... I was under the false assumption that because the
xarray wasn't dynamically allocated, we didn't need to perform any
cleanup, thanks for spotting this.

Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel