Re: [net-next,v3,06/11] net: phy: Represent PHY-less SFP modules with phy_port
From: Simon Horman <horms@kernel.org>
Date: 2026-02-03 19:57:19
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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:
quoted hunk ↗ jump to hunk
@@ -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?