Re: [net-next,v3,06/11] net: phy: Represent PHY-less SFP modules with phy_port

[Simon Horman]

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: Represent PHY-less SFP modules with phy_port

Now that the SFP bus infrastructure notifies when PHY-less modules are
connected, we can create a phy_port to represent it. Instead of letting
the SFP subsystem handle that, the Bus' upstream is in charge of
maintaining that phy_port and register it to the topology, as the
upstream (in this case a phy device) is directly interacting with the
underlying net_device.

Add a phy_caps helper to get the achievable modes on this module based
on what the phy_port representing the bus supports.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1609,6 +1613,74 @@ static void phy_sfp_link_down(void *upstream)
> +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;
> +
> +	port->active = true;
> +
> +	caps = sfp_get_module_caps(phydev->sfp_bus);
> +
> +	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
> +					phydev->sfp_bus_port->interfaces);

At line 1638, phy_add_sfp_mod_port() dereferences phydev->sfp_bus_port
without checking for NULL. Can this be NULL here?

Looking at the initialization in phy_sfp_probe(), when sfp_bus_add_upstream()
fails, the error path frees the port but then unconditionally stores it in
phydev->sfp_bus_port:

> @@ -1700,7 +1774,7 @@ static struct phy_port *phy_setup_sfp_port(struct phy_device *phydev)
>  static int phy_sfp_probe(struct phy_device *phydev)
>  {
> -	struct phy_port *port;
> +	struct phy_port *port = NULL;
>  	struct sfp_bus *bus;
>  	int ret;
>
> @@ -1729,6 +1803,8 @@ static int phy_sfp_probe(struct phy_device *phydev)
>  		phy_port_destroy(port);
>  	}
>
> +	phydev->sfp_bus_port = port;
> +
>  	return ret;
>  }

If sfp_bus_add_upstream() returns an error at line 1800, the cleanup path at
line 1802 checks 'ret && port', then calls phy_del_port() and
phy_port_destroy() to free the port. However, line 1806 still executes
'phydev->sfp_bus_port = port', which assigns the freed pointer to the
phydev structure.

When phy_sfp_probe() returns an error, probe fails and the PHY device is not
properly initialized. Later, if a PHY-less SFP module is inserted, the SFP
subsystem calls phy_sfp_module_start(), which calls phy_add_sfp_mod_port(),
which dereferences phydev->sfp_bus_port->interfaces at line 1638.

Should the assignment at line 1806 be conditional, or should port be set to
NULL after freeing?