Re: [PATCH net-next v18 04/10] net: phy: Create SFP phy_port before registering upstream

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This commit fixes a real, user-visible bug but the commit
  message calls it preparation only ("This will allow passing the…
- [Medium] Now that module_insert can succeed synchronously inside
  phy_sfp_probe() (see the previous finding), it runs before…

--

net: phy: Create SFP phy_port before registering upstream

This commit moves creation of the SFP phy_port in phy_sfp_probe() so that
it happens before sfp_bus_add_upstream() instead of after it. It also
changes phy_setup_sfp_port() to return the port pointer, and reworks the
error path to delete the port and drop the bus reference on failure.

> We can move the port creation before registering the sfp upstream ops,
> as long as we know the SFP bus is there. This will allow passing the
> phy_port along with the upstream information to the SFP bus.

[Severity: Medium]
The message describes this as preparation only. Does the reordering also
fix an existing bug?

Before this patch, phy_sfp_probe() called sfp_bus_add_upstream() before
phy_setup_sfp_port(). Suppose the SFP cage driver has already probed the
module, so sm_mod_state is SFP_MOD_WAITDEV. Two common ways to get there
are a built-in sfp driver with a modular PHY driver loaded later, and an
unbind/rebind of the PHY driver while a module is plugged in. In that
case, registering the upstream runs module_insert synchronously:

phy_sfp_probe()
  sfp_bus_add_upstream()
    sfp_register_bus()
      bus->registered = true;
      bus->socket_ops->attach()
        sfp_attach()
          sfp_sm_event(SFP_E_DEV_ATTACH)
            sfp_sm_module()        /* SFP_MOD_WAITDEV */
              sfp_module_insert()
                phy_sfp_module_insert()

The SFP phy_port did not exist yet at that point, so
phy_sfp_module_insert() returned without printing anything:

	port = phy_get_sfp_port(phydev);
	if (!port)
		return -EINVAL;

sfp_sm_module() then moved the module to SFP_MOD_ERROR. It only leaves
that state on SFP_E_REMOVE or SFP_E_INSERT, so the module stayed unusable
until someone physically re-inserted it.

With the port now created before sfp_bus_add_upstream(), this path
succeeds. Should the commit message describe the fix? Should it also have
a Fixes: tag so that stable kernels with the phy_port SFP infrastructure
pick it up? Possible candidates:

  d7c6082f7e77 ("net: phy: Introduce generic SFP handling for PHY drivers")
  07f3ca9e092c ("net: phy: Create a phy_port for PHY-driven SFPs")

The first made module_insert depend on phy_get_sfp_port(). The second
put the port setup after upstream registration.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c4d2c03b8282b..3c20d4cc1a79d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1619,30 +1621,46 @@ static int phy_setup_sfp_port(struct phy_device *phydev)
>   */
>  static int phy_sfp_probe(struct phy_device *phydev)
>  {
> +	struct phy_port *port = NULL;
>  	struct sfp_bus *bus;
> -	int ret = 0;
> +	int ret;
>  
> -	if (phydev->mdio.dev.fwnode) {
> -		bus = sfp_bus_find_fwnode(phydev->mdio.dev.fwnode);
> -		if (IS_ERR(bus))
> -			return PTR_ERR(bus);
> +	if (!phydev->mdio.dev.fwnode)
> +		return 0;
>  
> -		phydev->sfp_bus = bus;
> +	bus = sfp_bus_find_fwnode(phydev->mdio.dev.fwnode);
> +	if (IS_ERR(bus))
> +		return PTR_ERR(bus);
>  
> -		ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
> -		sfp_bus_put(bus);
> +	phydev->sfp_bus = bus;
>  
> -		if (ret)
> -			phydev->sfp_bus = NULL;
> +	if (bus) {
> +		port = phy_setup_sfp_port(phydev);
> +		if (IS_ERR(port)) {
> +			ret = PTR_ERR(port);
> +			goto out_sfp;
> +		}
>  	}
>  
> -	if (!ret && phydev->sfp_bus) {
> -		ret = phy_setup_sfp_port(phydev);
> -		if (ret) {
> -			sfp_bus_del_upstream(phydev->sfp_bus);
> -			phydev->sfp_bus = NULL;
> -		}
> +	ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);

[Severity: Medium]
phy_sfp_module_insert() can now succeed from inside phy_sfp_probe(). What
happens to the phydev->port value it sets there?

Take a PHY whose SFP port is its only port (max_n_ports == 1, such as
at8031, 88e1510 or 88x2222). The synchronous insert does this in
phy_sfp_module_insert():

	if (phydev->n_ports == 1)
		phydev->port = caps->port;

Then phy_sfp_probe() returns, and phy_setup_ports() unconditionally
overwrites the value:

	/* For now, the phy->port field is set as the first active port's type */
	list_for_each_entry(port, &phydev->ports, head)
		if (port->active) {
			phydev->port = phy_port_get_type(port);
			break;
		}

The SFP port has is_mii set and no mediums, so phy_port_get_type()
returns PORT_OTHER for it.

As a result, a module that is already present when the PHY probes reports
PORT_OTHER. The same module hot-plugged after probe keeps caps->port,
for example PORT_FIBRE or PORT_DA.

qca807x in combo mode appears to have the same problem.
qca807x_configure_serdes() sets phydev->port = PORT_FIBRE during the
synchronous insert. phy_setup_ports() then adds the default copper port
and overwrites phydev->port with PORT_TP. The hot-plug path keeps
PORT_FIBRE.

at803x, 88x2222 and qca807x never recompute phydev->port in read_status,
so the port type that ethtool shows stays wrong until the module is
removed. marvell.c and marvell10g recompute it at runtime.

Before this patch, this path failed with -EINVAL, so the overwrite only
becomes reachable with this reordering. Later patches in the series leave
phy_setup_ports() and phy_sfp_module_insert() unchanged, so the behaviour
remains at the end of the series.

Should phy_setup_ports() leave phydev->port alone when an SFP module was
already inserted during phy_sfp_probe()?

> +	if (ret)
> +		goto out_port;

[ ... ]

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