Re: [net-next,v3,03/11] net: phy: phy_link_topology: Track ports in phy_link_topology

[Simon Horman <horms@kernel.org>]

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, keeping track of all
ports connected to a netdev. Ports are added when a PHY is attached and
removed when it is detached.

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 7dc2ff10c74a..456f1144e83e 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c

[ ... ]

> +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;
> +}

[ ... ]

> @@ -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;
                   ^^^

xa_alloc_cyclic() returns 1 when ID allocation wraps around, which is still a
successful allocation. The check at line 126 above correctly uses `if (ret < 0)`
for the phyindex allocation:

    ret = xa_alloc_cyclic(&topo->phys, &phy->phyindex, pdn, ...);
    if (ret < 0)
        goto err;

Should this use `if (ret < 0)` as well for consistency? With the current code,
when the port ID wraps around, the PHY attachment will fail and trigger rollback
of all successfully added ports even though the allocation succeeded.