Re: [net-next,v6,02/10] net: phy: phy_link_topology: Track ports in phy_link_topology

[Simon Horman]

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;

[ ... ]