Re: [PATCH RESEND net-next v17 01/10] net: phy: phy_link_topology: Add a helper for opportunistic alloc

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] The comment added to phy_link_topo_get_or_alloc()
  (drivers/net/phy/phy_link_topology.c:38-51) justifies the lock-free…

--

net: phy: phy_link_topology: Add a helper for opportunistic alloc

This moves the opportunistic allocation of struct phy_link_topology out of
phy_link_topo_add_phy() into a new helper, phy_link_topo_get_or_alloc(),
which returns the existing topology or allocates one, reporting failures
via ERR_PTR().

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 4134de7ae313d..0462283c80202 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -28,11 +28,39 @@ static int netdev_alloc_phy_link_topology(struct net_device *dev)
>  	return 0;
>  }
>  
> +static struct phy_link_topology *phy_link_topo_get_or_alloc(struct net_device *dev)
> +{
> +	int ret;
> +
> +	if (dev->link_topo)
> +		return dev->link_topo;
> +
> +	/* The topology is allocated the first time we add an object to it.
> +	 * It is freed alongside the netdev. It can be called on multiple
> +	 * contexts:
> +	 *  - It can be called from .probe() : No rtnl, no netdev_lock
> +	 *  - .ndo_open() : rtnl and possibly netdev_lock
> +	 *  - SFP state machine : rtnl held or not
> +	 *
> +	 *  However, we can't really have races :
> +	 *  - If we have a PHY, phy_link_topo_add_phy() will always run first
> +	 *    and trigger the alloc. Only then the ports can be added through
> +	 *    phylib or sfp.
> +	 *  - If we don't, the SFP port for the cage is registered first, and
> +	 *    only then other ports/PHYs can be registered.
> +	 */

[Severity: Low]
This isn't a bug, but the new comment in phy_link_topo_get_or_alloc()
reasons about objects that don't exist yet at this commit.

At this point struct phy_link_topology only tracks PHYs:

include/linux/phy_link_topology.h:
	struct phy_link_topology {
		struct xarray phys;
		u32 next_phy_index;
	};

and there is no phy_link_topo_add_port(), so "Only then the ports can be
added through phylib or sfp" and "the SFP port for the cage is registered
first" have no counterpart in the tree at this commit.

Would it read better to move that part of the comment to the follow-up
patch that adds the ports xarray, next_port_index and
phy_link_topo_add_port()/phy_link_topo_del_port()?

For the record, this resolves itself later in the series: the next patch,
"net: phy: phy_link_topology: Track ports in phy_link_topology", adds the
ports xarray and the port add/del helpers that call
phy_link_topo_get_or_alloc(), so by the end of the series the comment
matches the code.

> +	ret = netdev_alloc_phy_link_topology(dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return dev->link_topo;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910170103.1029108-1-maxime.chevallier%40bootlin.com