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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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