phy_port is aimed at representing the various physical interfaces of a
net_device. They can be controlled by various components in the link,
such as the Ethernet PHY, the Ethernet MAC, and SFP module, etc.

Let's therefore make so we keep track of all the ports connected to a
netdev in phy_link_topology. The only ports added for now are phy-driven
ports.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_link_topology.c | 53 +++++++++++++++++++++++++++++
 include/linux/phy_link_topology.h   | 18 ++++++++++
 include/linux/phy_port.h            |  2 ++
 net/core/dev.c                      |  1 +
 4 files changed, 74 insertions(+)

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
@@ -7,6 +7,7 @@
  */
 
 #include <linux/phy_link_topology.h>
+#include <linux/phy_port.h>
 #include <linux/phy.h>
 #include <linux/rtnetlink.h>
 #include <linux/xarray.h>
@@ -22,6 +23,9 @@ static int netdev_alloc_phy_link_topology(struct net_device *dev)
 	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
 	topo->next_phy_index = 1;
 
+	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
+	topo->next_port_index = 1;
+
 	dev->link_topo = topo;
 
 	return 0;
@@ -44,12 +48,45 @@ static struct phy_link_topology *phy_link_topo_get_or_alloc(struct net_device *d
 	return dev->link_topo;
 }
 
+int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port)
+{
+	struct phy_link_topology *topo;
+	int ret;
+
+	topo = phy_link_topo_get_or_alloc(dev);
+	if (IS_ERR(topo))
+		return PTR_ERR(topo);
+
+	/* Attempt to re-use a previously allocated port_id */
+	if (port->id)
+		ret = xa_insert(&topo->ports, port->id, port, GFP_KERNEL);
+	else
+		ret = xa_alloc_cyclic(&topo->ports, &port->id, port,
+				      xa_limit_32b, &topo->next_port_index,
+				      GFP_KERNEL);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_link_topo_add_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);
+}
+EXPORT_SYMBOL_GPL(phy_link_topo_del_port);
+
 int phy_link_topo_add_phy(struct net_device *dev,
 			  struct phy_device *phy,
 			  enum phy_upstream upt, void *upstream)
 {
 	struct phy_link_topology *topo;
 	struct phy_device_node *pdn;
+	struct phy_port *port;
 	int ret;
 
 	topo = phy_link_topo_get_or_alloc(dev);
@@ -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);
+
+	xa_erase(&topo->phys, phy->phyindex);
 err:
 	kfree(pdn);
 	return ret;
@@ -102,10 +151,14 @@ void phy_link_topo_del_phy(struct net_device *dev,
 {
 	struct phy_link_topology *topo = dev->link_topo;
 	struct phy_device_node *pdn;
+	struct phy_port *port;
 
 	if (!topo)
 		return;
 
+	list_for_each_entry(port, &phy->ports, head)
+		phy_link_topo_del_port(dev, port);
+
 	pdn = xa_erase(&topo->phys, phy->phyindex);
 
 	/* We delete the PHY from the topology, however we don't re-set the
diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
index 68a59e25821c..66bceff72b19 100644
--- a/include/linux/phy_link_topology.h
+++ b/include/linux/phy_link_topology.h
@@ -16,11 +16,15 @@
 
 struct xarray;
 struct phy_device;
+struct phy_port;
 struct sfp_bus;
 
 struct phy_link_topology {
 	struct xarray phys;
 	u32 next_phy_index;
+
+	struct xarray ports;
+	u32 next_port_index;
 };
 
 struct phy_device_node {
@@ -43,6 +47,9 @@ int phy_link_topo_add_phy(struct net_device *dev,
 
 void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy);
 
+int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port);
+void phy_link_topo_del_port(struct net_device *dev, struct phy_port *port);
+
 static inline struct phy_device *
 phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
 {
@@ -72,6 +79,17 @@ static inline void phy_link_topo_del_phy(struct net_device *dev,
 {
 }
 
+static inline int phy_link_topo_add_port(struct net_device *dev,
+					 struct phy_port *port)
+{
+	return 0;
+}
+
+static inline void phy_link_topo_del_port(struct net_device *dev,
+					  struct phy_port *port)
+{
+}
+
 static inline struct phy_device *
 phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
 {
diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
index 0ef0f5ce4709..4e2a3fdd2f2e 100644
--- a/include/linux/phy_port.h
+++ b/include/linux/phy_port.h
@@ -36,6 +36,7 @@ struct phy_port_ops {
 /**
  * struct phy_port - A representation of a network device physical interface
  *
+ * @id: Unique identifier for the port within the topology
  * @head: Used by the port's parent to list ports
  * @parent_type: The type of device this port is directly connected to
  * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
@@ -52,6 +53,7 @@ struct phy_port_ops {
  * @is_sfp: Indicates if this port drives an SFP cage.
  */
 struct phy_port {
+	u32 id;
 	struct list_head head;
 	enum phy_port_parent parent_type;
 	union {
diff --git a/net/core/dev.c b/net/core/dev.c
index 1cf3ad840697..999f30ede7d1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11291,6 +11291,7 @@ static void netdev_free_phy_link_topology(struct net_device *dev)
 
 	if (IS_ENABLED(CONFIG_PHYLIB) && topo) {
 		xa_destroy(&topo->phys);
+		xa_destroy(&topo->ports);
 		kfree(topo);
 		dev->link_topo = NULL;
 	}
-- 
2.49.0

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.

> 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

[ ... ]

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

The existing xa_alloc_cyclic error handling in the same function uses
'if (ret < 0) goto err' which correctly skips the failed PHY in cleanup.

> +
> +	xa_erase(&topo->phys, phy->phyindex);
>  err:
>  	kfree(pdn);
>  	return ret;

[ ... ]

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.
> 
>> 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
> 
> [ ... ]
> 
>> @@ -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