Re: [PATCH net-next v19 03/10] net: phylink: Register a phy_port for MAC-driven SFP cages

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] phylink_create_sfp_cage_port() in drivers/net/phy/phylink.c
  allocates the MAC/phylink-driven SFP cage port with phy_port_alloc()…

Pre-existing issues:
- [Medium] mtk_eth_soc creates a NETDEV phylink in mtk_add_mac() with
  phylink_create(&mac->phylink_config, of_fwnode_handle(mac->of_node),…

--

net: phylink: Register a phy_port for MAC-driven SFP cages

This commit makes phylink allocate a phy_port for an SFP cage driven by
the MAC and add it to the netdev's link topology. It does this only for
NETDEV phylink instances. The port is created in phylink_register_sfp()
and freed in phylink_destroy().

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a7d086cdc9b25..971f8961dcf1a 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1767,6 +1770,52 @@ static void phylink_fixed_poll(struct timer_list *t)
>  
>  static const struct sfp_upstream_ops sfp_phylink_ops;
>  
> +static int phylink_create_sfp_cage_port(struct phylink *pl)
> +{
> +	struct phy_port *port;
> +	int ret = 0;
> +
> +	if (!pl->netdev || !pl->sfp_bus)
> +		return 0;
> +
> +	port = phy_port_alloc();
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->is_sfp = true;
> +	port->is_mii = true;
> +	port->active = true;

[Severity: Low]
Should parent_type and phy be set here? phy_port_alloc() uses kzalloc, so
this port ends up with parent_type == PHY_PORT_PHY (the zero value) and
port->phy == NULL. It is then published through phy_link_topo_add_port().

include/linux/phy_port.h describes PHY_PORT_PHY as the port being driven
by a PHY device, with @phy as the controlling PHY:

enum phy_port_parent {
	PHY_PORT_PHY,
};

The PHY-driven cage port in phy_setup_sfp_port() sets both fields
together:

	port->parent_type = PHY_PORT_PHY;
	port->phy = phydev;

There is currently no enum value that can describe a port owned by the
MAC or by phylink.

Nothing reads parent_type today. port_phydev() is only called from PHY
drivers' own port->ops callbacks, and this port has ops == NULL. However,
port_phydev() returns port->phy without checking it. A future consumer
that trusts parent_type and calls port_phydev() would then get NULL for
this port.

The same pattern appears later in the series. In "net: phy: Represent
PHY-less SFP modules with phy_port", phylink_add_sfp_mod_port() and
phy_add_sfp_mod_port() also publish ports with the default parent_type
and phy == NULL.

Would it make sense to add a parent type for MAC/phylink-driven ports and
set it here?

> +
> +	phy_interface_and(port->interfaces, pl->config->supported_interfaces,
> +			  phylink_sfp_interfaces);
> +	phy_port_update_supported(port);
> +
> +	rtnl_lock();
> +	ret = phy_link_topo_add_port(pl->netdev, port);
> +	rtnl_unlock();

[ ... ]

> @@ -2069,6 +2127,7 @@ EXPORT_SYMBOL_GPL(phylink_create);
>  void phylink_destroy(struct phylink *pl)
>  {
>  	sfp_bus_del_upstream(pl->sfp_bus);
> +	phylink_destroy_sfp_cage_port(pl);

[Severity: Medium]
This is a pre-existing issue, but it now also leaks the new phy_port.
phylink_destroy() is the only path that frees sfp_cage_port, and some
drivers never call it.

For example, drivers/net/ethernet/mediatek/mtk_eth_soc.c creates a NETDEV
phylink in mtk_add_mac():

	phylink = phylink_create(&mac->phylink_config,
				 of_fwnode_handle(mac->of_node),
				 phy_mode, mac_ops);

No phylink_destroy() call exists anywhere in that driver. Both
mtk_free_dev() and the mtk_add_mac() error path call free_netdev()
directly.

If the MAC node has an SFP cage, this call chain allocates the port:

mtk_add_mac()->phylink_create()->phylink_register_sfp()
  ->phylink_create_sfp_cage_port()->phy_port_alloc()

Teardown then follows this chain:

mtk_free_dev()->free_netdev()->netdev_free_phy_link_topology()
  ->xa_destroy(&topo->ports)

xa_destroy() frees only the xarray's internal nodes, not the stored
entries. Is the phy_port leaked on every unbind or failed probe here?

The missing phylink_destroy() in mtk_eth_soc predates this patch. It
already leaks the struct phylink and leaves the SFP upstream registered.
This patch adds one more object to that leak.

>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>

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