Re: [PATCH RESEND net-next v17 06/10] net: phy: phy_port: Store information about a port's upstream

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 0 (1) · Low: 3

- [Low] The commit message states the change keeps track of "the port id
  of the MII port backing it", but phy_set_upstream_port()…
- [Low] The kernel-doc of the newly exported phy_set_upstream_port()
  (drivers/net/phy/phy_device.c:3786-3791) documents only that it "Sets…
- [Low] Claimed missing error-path rollback: both new connect_phy
  implementations (phy_sfp_connect_phy() at…

Pre-existing issues:
- [Medium] phy_set_upstream_port()
  (drivers/net/phy/phy_device.c:3792-3803) traverses phydev->ports with…

--

net: phy: phy_port: Store information about a port's upstream

This adds an upstream_port pointer to struct phy_port and struct phy_device,
along with a new exported helper phy_set_upstream_port(), so that an MDI port
backed by an MII port (for example an SFP module port backed by the SFP cage
port) records the port feeding it. The SFP connect/disconnect callbacks in
both phy_device.c and phylink.c are updated to set and clear it.

A few questions below.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 26634ef274f33..561d19edf331c 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1493,6 +1493,7 @@ static int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  	int ret;
>  
>  	phydev->has_sfp_mod_phy = true;
> +	phy_set_upstream_port(phy, phydev->sfp_cage_port);
>  
>  	/* If we aren't attached to a netdev, we can't add the SFP PHY to its
>  	 * topology.

[Severity: Low]
Here and in phylink_sfp_connect_phy(), the cage port pointer is published
into the module PHY before a fallible step (phy_link_topo_add_phy() here,
phylink_sfp_config_phy() there) and is not cleared if that step fails.
sfp_register_bus() also returns a connect_phy() error without calling
disconnect_phy().

Is a rollback needed on those paths, or is it intentionally left out?

Tracing it through suggests no dangling pointer is reachable today:

  sfp_sm_probe_phy()
      err = sfp_add_phy(sfp->sfp_bus, phy);
      if (err) {
              phy_device_remove(phy);
              phy_device_free(phy);

so the module phy_device and its ports are destroyed on that failure, and
the cage port is still alive at that point. The two paths that free the
cage port (phy_sfp_probe() out_port below, and phylink_register_sfp() ->
phylink_destroy_sfp_cage_port()) only run when sfp_register_bus() fails,
which requires bus->phydev to be non-NULL:

  sfp_register_bus()
      if (ops->connect_phy && bus->phydev) {
              ret = ops->connect_phy(bus->upstream, bus->phydev);

and the state machine detaches the module PHY (sfp_sm_phy_detach() ->
sfp_remove_phy(), which clears bus->phydev and invokes disconnect_phy())
before any upstream can register again. Would it still be worth making
the failure path symmetric so this does not depend on that ordering?

> @@ -1526,6 +1527,8 @@ static void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
>  
>  	if (dev)
>  		phy_link_topo_del_phy(dev, phy);
> +
> +	phy_set_upstream_port(phy, NULL);
>  }
>  
>  /**

[ ... ]

> @@ -1816,6 +1821,8 @@ static int phy_sfp_probe(struct phy_device *phydev)
>  		}
>  	}
>  
> +	phydev->sfp_cage_port = port;
> +
>  	ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
>  	if (ret)
>  		goto out_port;
> @@ -1825,14 +1832,13 @@ static int phy_sfp_probe(struct phy_device *phydev)
>  	 */
>  	sfp_bus_put(bus);
>  
> -	phydev->sfp_cage_port = port;
> -
>  	return ret;
>  
>  out_port:
>  	if (port) {
>  		phy_del_port(phydev, port);
>  		phy_port_destroy(port);
> +		phydev->sfp_cage_port = NULL;
>  	}
>  out_sfp:
>  	sfp_bus_put(bus);

[ ... ]

> @@ -3777,6 +3783,25 @@ struct phy_port *phy_get_sfp_port(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL_GPL(phy_get_sfp_port);
>  
> +/**
> + * phy_set_upstream_port() - Sets the phy_port controlling the MII this PHY is
> + *			     attached to.
> + * @phydev: pointer to the PHY device we set the upstream of.
> + * @port: The phy_port upstream of this PHY, can be NULL.
> + */
> +void phy_set_upstream_port(struct phy_device *phydev, struct phy_port *port)
> +{
> +	struct phy_port *local_port;
> +
> +	ASSERT_RTNL();

[Severity: Low]
The kernel-doc for phy_set_upstream_port() describes only that it sets the
phy_port controlling the MII, but the body does two more things that callers
need to know about: it hard-asserts RTNL, and it overwrites upstream_port on
every entry of phydev->ports.

Since this is EXPORT_SYMBOL_GPL() and the prototype in include/linux/phy.h
carries no locking annotation either, a caller following only the documented
contract will trip the WARN_ONCE() in ASSERT_RTNL(). Could the kernel-doc
state that RTNL must be held and that the value is propagated to all of the
PHY's ports?

> +
> +	phydev->upstream_port = port;
> +
> +	phy_for_each_port(phydev, local_port)
> +		local_port->upstream_port = port;
> +}
> +EXPORT_SYMBOL_GPL(phy_set_upstream_port);

[Severity: Medium]
This isn't a bug introduced by this patch, but the phy_for_each_port()
traversal here relies solely on ASSERT_RTNL(), while the list entries are
unlinked and freed from the driver-core unbind path, which does not take
RTNL:

  phy_remove()
      phy_sfp_release() / phy_cleanup_ports()
          list_for_each_entry_safe(port, tmp, &phydev->ports, head) {
                  phy_del_port(phydev, port);
                  phy_port_destroy(port);
          }

phy_get_sfp_port() and phy_link_topo_del_phy() already walk the same list
under the same RTNL-only convention, so the pattern predates this change,
and all in-tree callers of the new helper run from the SFP state machine
with RTNL held (sfp.c takes rtnl_lock() around sfp_sm_event()). The only
concurrent mutator found is a root-initiated sysfs unbind of the module PHY
driver.

Would it make sense to either take phydev->lock around the traversal, or
write down the rule that phydev->ports is immutable while the PHY is
registered?

Also, phy_add_sfp_mod_port() and phylink_add_sfp_mod_port() assign
port->upstream_port directly, and this helper later rewrites every port of
the PHY. Which of the two is meant to be authoritative?

>  
>  /**
>   * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
>   * @fwnode: pointer to the mdio_device's fwnode

[ ... ]

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index c8f57d78f96eb..320c8c2dd206a 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -4132,6 +4134,8 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  	phy_interface_and(phy->host_interfaces, phylink_sfp_interfaces,
>  			  pl->config->supported_interfaces);
>  
> +	phy_set_upstream_port(phy, pl->sfp_cage_port);
> +
>  	/* Do the initial configuration */
>  	return phylink_sfp_config_phy(pl, phy);
>  }
> @@ -4140,6 +4144,7 @@ static void phylink_sfp_disconnect_phy(void *upstream,
>  				       struct phy_device *phydev)
>  {
>  	phylink_disconnect_phy(upstream);
> +	phy_set_upstream_port(phydev, NULL);
>  }
>  
>  static const struct sfp_upstream_ops sfp_phylink_ops = {

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index cb579b8a5b5f7..335821eb22171 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -615,6 +615,7 @@ struct phy_oatc14_sqi_capability {
>   * @sfp_bus: SFP bus attached to this PHY's fiber port
>   * @sfp_cage_port: The phy_port connected to the downstream SFP cage
>   * @mod_port: phy_port representing the SFP module, if it is phy-less
> + * @upstream_port: phy_port this PHY's MII attaches to, if any
>   * @attached_dev: The attached enet driver's device instance ptr
>   * @adjust_link: Callback for the enet controller to respond to changes: in the
>   *               link state.
> @@ -809,6 +810,7 @@ struct phy_device {
>  	struct sfp_bus *sfp_bus;
>  	struct phy_port *sfp_cage_port;
>  	struct phy_port *mod_port;
> +	struct phy_port *upstream_port;

[Severity: Low]
Is this new phy_device member read anywhere? phy_set_upstream_port() assigns
it, but grepping for upstream_port through the end of the series shows the
only reader is in net/ethtool/port.c:

  reply->upstream_port = port->upstream_port ? port->upstream_port->id : 0;

which uses the phy_port field, not the phy_device one. Could
phy_device::upstream_port be dropped, or is there a planned consumer?

Related to that, the commit message says:

  "let's keep track of the port id of the MII port backing it"

but what is stored is a raw struct phy_port pointer, not an id, and the new
phy_device field is not mentioned at all. Could the wording be adjusted to
match what the code stores?

>  	struct phylink *phylink;
>  	struct net_device *attached_dev;
>  	struct mii_timestamper *mii_ts;
> @@ -2493,6 +2495,8 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
>  
>  struct phy_port *phy_get_sfp_port(struct phy_device *phydev);
>  
> +void phy_set_upstream_port(struct phy_device *phydev, struct phy_port *port);
> +
>  /**
>   * phy_module_driver() - Helper macro for registering PHY drivers
>   * @__phy_drivers: array of PHY drivers to register

[ ... ]

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