Re: [PATCH net-next v18 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new comment in phy_link_topo_get_or_alloc()
  (drivers/net/phy/phy_link_topology.c) lists the contexts the helper…

Pre-existing issues:
- [Low] Documentation/networking/phy-link-topology.rst (API section) says
  struct phy_link_topology "is a per-netdevice resource, that gets…

--

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

This commit moves the lazy allocation of dev->link_topo out of
phy_link_topo_add_phy() and into a new helper,
phy_link_topo_get_or_alloc(). The helper returns the existing topology
or allocates a new one. It prepares for tracking phy_port objects in
the topology.

> 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

[Severity: Low]
Can phy_link_topo_get_or_alloc() actually be reached with netdev_lock
held?

At this commit, the only caller is phy_link_topo_add_phy(). It returns
early for ops-locked devices before it calls the helper:

phy_link_topo_add_phy() {
    ...
	if (WARN_ON_ONCE(netdev_need_ops_lock(dev)))
		return -EOPNOTSUPP;
    ...
}

Later in the series, phy_link_topo_add_port() rejects the same devices
with WARN_ON_ONCE(dev->netdev_ops && netdev_need_ops_lock(dev)).

On the open path, dev_open() only takes netdev_lock through
netdev_lock_ops():

include/net/netdev_lock.h:netdev_lock_ops() {
	if (netdev_need_ops_lock(dev))
		netdev_lock(dev);
}

That means netdev_lock is held only for the devices that both callers
reject. Would it be more accurate to drop "possibly netdev_lock" from
the .ndo_open() line?

This comment also mentions ports and the SFP cage port, which don't
exist yet at this commit. struct phy_link_topology only has phys and
next_phy_index. The rest of the series fixes that. "net: phy:
phy_link_topology: Track ports in phy_link_topology" adds the ports
xarray and phy_link_topo_add_port(). Later patches add
phylink_create_sfp_cage_port(), phylink_add_sfp_mod_port() and
phy_add_sfp_mod_port(). By the end of the series, only the netdev_lock
part is still inaccurate.

> +	 *  - 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.
> +	 */
> +	ret = netdev_alloc_phy_link_topology(dev);
> +	if (ret)
> +		return ERR_PTR(ret);

[Severity: Low]
This is a pre-existing issue, not something this patch introduced. The
API section of Documentation/networking/phy-link-topology.rst still
says struct phy_link_topology "is a per-netdevice resource, that gets
initialized at netdevice creation".

The code allocates dev->link_topo only when the first object is added.
phy_link_topo_empty() and phy_link_topo_get_phy() both handle a NULL
dev->link_topo.

The baseline already did this allocation inline in
phy_link_topo_add_phy(). The last patch in the series, "Documentation:
networking: Update the phy_port infrastructure description", leaves
that sentence unchanged.

Could the documentation say that the topology is allocated when the
first object is added?

[ ... ]

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