Re: [PATCH RESEND net-next v17 05/10] net: phy: Represent PHY-less SFP modules with phy_port

[netdev-bot+sashiko]

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

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

- [High] FALSE POSITIVE (resolved later in the same series).
- [High] Bitfield data race on `has_sfp_mod_phy` can silently corrupt
  hardware interrupt flags
- [Medium] Failure to allocate/register the purely informational SFP-
  module phy_port now makes the whole SFP module bring-up fail…
- [Medium] Unbalanced registration teardown for `phydev->mod_port`:
  `phy_detach_internal()` unconditionally calls…
- [Low] FALSE POSITIVE.

--

net: phy: Represent PHY-less SFP modules with phy_port

This adds a phy_port to represent a PHY-less SFP module, created and
destroyed from the new .module_start / .module_stop upstream ops in both
phy_device.c and phylink.c, plus a phy_caps helper to filter the module
link modes by the cage port's MII interfaces.

A couple of questions on the new error paths below.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index f15c0fc339b4d..26634ef274f33 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1617,6 +1629,75 @@ static void phy_sfp_link_down(void *upstream)
>  		port->ops->link_down(port);
>  }
>  
> +static int phy_add_sfp_mod_port(struct phy_device *phydev)
> +{
> +	const struct sfp_module_caps *caps;
> +	struct phy_port *port;
> +	int ret = 0;
> +
> +	/* Create mod port */
> +	port = phy_port_alloc();
> +	if (!port)
> +		return -ENOMEM;
> +

[ ... ]

> +	if (phydev->attached_dev) {
> +		ret = phy_link_topo_add_port(phydev->attached_dev, port);
> +		if (ret) {
> +			phy_port_destroy(port);
> +			return ret;
> +		}
> +	}

[ ... ]

> +static int phy_sfp_module_start(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +
> +	/* If there's a downstream SFP module, and it doesn't contain a PHY
> +	 * device, let's create a phy_port to represent that module.
> +	 */
> +	if (!phydev->has_sfp_mod_phy)
> +		return phy_add_sfp_mod_port(phydev);
> +
> +	return 0;
> +}

[Severity: Medium]
Should a failure to create this port really abort the whole module
bring-up?

Both new .module_start implementations propagate the error: here
phy_sfp_module_start() returns phy_add_sfp_mod_port()'s -ENOMEM from
phy_port_alloc(), or whatever phy_link_topo_add_port() returns
(xa_alloc_cyclic() -ENOMEM, or -EOPNOTSUPP after the WARN_ON_ONCE for
netdev_need_ops_lock() devices), and phylink_sfp_module_start() does the
same in its new else branch:

	} else {
		ret = phylink_add_sfp_mod_port(pl);
		if (ret)
			return ret;
	}

On the SFP core side, any non-zero return is fatal, in sfp_sm_main():

	case SFP_S_INIT_PHY:
		...
		if (sfp_module_start(sfp->sfp_bus)) {
			sfp_sm_next(sfp, SFP_S_FAIL, 0);
			break;
		}

There is no case SFP_S_FAIL: in that switch, so the only way out is the
global handling at the top of the function, i.e. losing SFP_MOD_PRESENT
or clearing SFP_DEV_UP.

So does a transient kzalloc() failure for this port leave the link down
until the interface is taken down and up again, or the module is
physically re-inserted? Given that the module port is a purely
informational topology object with no data-path role, would it be
preferable to warn and continue rather than to fail the state machine?

The commit message describes only the representation of the module as a
phy_port and doesn't mention this new failure mode.

> @@ -1834,6 +1920,8 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
>  		phydev->attached_dev->phydev = NULL;
>  		phydev->attached_dev = NULL;
>  		phy_link_topo_del_phy(dev, phydev);
> +		if (phydev->mod_port)
> +			phy_link_topo_del_port(dev, phydev->mod_port);
>  	}
>  
>  	phydev->phy_link_change = NULL;
> @@ -1963,6 +2051,12 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  		err = phy_link_topo_add_phy(dev, phydev, PHY_UPSTREAM_MAC, dev);
>  		if (err)
>  			goto error;
> +
> +		if (phydev->mod_port) {
> +			err = phy_link_topo_add_port(dev, phydev->mod_port);
> +			if (err)
> +				goto error;
> +		}
>  	}

[Severity: Medium]
Can this pair of add/del calls unregister a port that belongs to someone
else?

The del side in phy_detach_internal() runs whenever phydev->mod_port is
non-NULL, with no record of whether the port was inserted into this
netdev's topology during this attach. And the del is by id only, without
checking the stored entry:

drivers/net/phy/phy_link_topology.c:phy_link_topo_del_port() {
	struct phy_link_topology *topo = dev->link_topo;

	if (!topo)
		return;

	xa_erase(&topo->ports, port->id);
}

Since a phy_port keeps its id across detach so it can be re-used,
phy_link_topo_add_port() re-inserts with xa_insert():

	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);

Two sequences look problematic:

phy_link_topo_add_phy() fails (pdn allocation, -EBUSY on a colliding
index, or -EOPNOTSUPP for an ops-locked netdev)

	goto error;
	  phy_detach_internal(phydev, false)
	    phy_link_topo_del_port(dev, phydev->mod_port)
	      xa_erase(&topo->ports, port->id)

and the module port was never inserted into this topology, so whatever
occupies that index is erased.

The second one is the new call itself: xa_insert() returns -EBUSY when
mod_port's retained id is already taken by another port of that netdev,
and the cleanup then erases exactly that foreign entry.

Would it make sense to only call phy_link_topo_del_port() for a port that
was successfully added here (a flag, or clearing/zeroing the id), and to
have phy_link_topo_del_port() verify the stored pointer before erasing?

This needs mod_port to survive a previous detach, which looks possible
since neither phy_detach() nor phy_detach_internal() stop the SFP state
machine or clear mod_port - only .module_stop does.

[ ... ]

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -724,6 +727,7 @@ struct phy_device {
>  	unsigned irq_rerun:1;
>  
>  	unsigned default_timestamp:1;
> +	unsigned has_sfp_mod_phy:1;
>  
>  	int rate_matching;
>  

[Severity: High]
Which lock protects the word this new bit lives in?

has_sfp_mod_phy is added right after default_timestamp, so it lands
in the same machine word as the interrupt bookkeeping bits
interrupts:1, irq_suspended:1 and irq_rerun:1. A C bitfield
assignment is not atomic with respect to its neighbours: the compiler
is free to emit a load of the enclosing word, an OR/AND of one bit,
and a store back. Any concurrent update of a different bit in that
same word is then lost.

The writers of has_sfp_mod_phy are phy_sfp_connect_phy() and
phy_sfp_disconnect_phy(), which run from the SFP state machine work
under rtnl_lock() but without phydev->lock and with interrupts
enabled. irq_rerun (and irq_suspended) are written from
phy_interrupt() and from the mdio_bus_phy_suspend()/_resume() PM
paths, neither of which take rtnl. So a store to has_sfp_mod_phy
that races with phy_interrupt() setting irq_rerun = 1 can drop the
irq_rerun update, and the pending wakeup interrupt is then never
re-run after resume - a silently missed PHY interrupt / broken WoL
wakeup rather than an obvious failure.

The existing rtnl-protected SFP state in this struct is a plain bool
(sfp_bus_attached, with the "This may be modified under the rtnl
lock" comment above it), and the new sfp_cage_port / mod_port
pointers you add sit in that same group. Would it be cleaner to make
has_sfp_mod_phy a bool next to sfp_bus_attached, so it is covered by
the same rule as the rest of the SFP fields and shares no word with
the IRQ flags? If you would rather keep it a bitfield, which lock is
intended to serialise the enclosing word, and how is that
synchronised against phy_interrupt()?

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