On 06/03/2026 15:09, Simon Horman wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: phy: phy_link_topology: Track ports in phy_link_topology

This commit adds port tracking to phy_link_topology by introducing a
ports xarray alongside the existing phys xarray. PHY-driven ports are
registered and tracked with unique IDs that can be reused.

quoted

diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index fdfafd951905..f7946c73dc5f 100644
--- a/drivers/net/phy/phy_link_topology.c
+++ b/drivers/net/phy/phy_link_topology.c

[ ... ]

quoted

@@ -89,8 +126,20 @@ int phy_link_topo_add_phy(struct net_device *dev,
 	if (ret < 0)
 		goto err;

+	/* Add all the PHY's ports to the topology */
+	list_for_each_entry(port, &phy->ports, head) {
+		ret = phy_link_topo_add_port(dev, port);
+		if (ret)
+			goto del_ports;
+	}
+
 	return 0;

+del_ports:
+	list_for_each_entry_from_reverse(port, &phy->ports, head)
+		phy_link_topo_del_port(dev, port);

Does this rollback sequence incorrectly delete a port that was never added?
list_for_each_entry_from_reverse() starts at the current entry (the port
that FAILED to add) and iterates backward. This means phy_link_topo_del_port()
is called on the failed port that was never successfully added to the xarray.

Looking at phy_link_topo_del_port():

void phy_link_topo_del_port(struct net_device *dev, struct phy_port *port)
{
	struct phy_link_topology *topo = dev->link_topo;

	if (!topo)
		return;

	xa_erase(&topo->ports, port->id);
}

The spurious xa_erase() is likely harmless in the -ENOMEM case from
xa_alloc_cyclic with port->id == 0, because XA_FLAGS_ALLOC1 reserves index 0.

However, in the xa_insert -EBUSY failure case where port->id is non-zero
and the slot is occupied by a different port, the rollback incorrectly
erases the other port's xarray entry, causing silent data corruption in
the topology's port tracking.

Should this use list_for_each_entry_continue_reverse() instead? That macro
starts from the entry BEFORE the current position, skipping the failed port
and only rolling back successfully-added ports.

Indeed ! thanks :)

Maxime