[PATCH net-next v2 09/12] net: phy: phy_port: Store information about a MII port's occupancy

Maxime Chevallier posted 12 patches 1 week, 4 days ago
There is a newer version of this series
[PATCH net-next v2 09/12] net: phy: phy_port: Store information about a MII port's occupancy
Posted by Maxime Chevallier 1 week, 4 days ago
MII phy_ports are not meant to be connected directly to a link partner.
They are meant to feed into some media converter devices, so far we only
support SFP modules for that.

We have information about what MII they can handle, however we don't
store anything about whether they are currently connected to an SFP
module or not. As phy_port aims at listing the front-facing ports, let's
store an "occupied" bit to know whether or not a MII port is currently
front-facing (i.e. there's no module in the SFP cage), or occupied (i.e.
there's an SFP module).

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 4 ++++
 drivers/net/phy/phylink.c    | 4 ++++
 include/linux/phy_port.h     | 3 +++
 3 files changed, 11 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b3125cf30bf3..9dba31b9cb86 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1636,6 +1636,8 @@ static int phy_sfp_connect_nophy(void *upstream)
 		}
 	}
 
+	phydev->sfp_bus_port->occupied = true;
+
 	/* we don't use phy_add_port() here as the module port isn't a direct
 	 * interface from the PHY, but rather an extension to the sfp-bus, that
 	 * is already represented by its own phy_port
@@ -1649,6 +1651,8 @@ static void phy_sfp_disconnect_nophy(void *upstream)
 {
 	struct phy_device *phydev = upstream;
 
+	phydev->sfp_bus_port->occupied = false;
+
 	if (phydev->attached_dev)
 		phy_link_topo_del_port(phydev->attached_dev, phydev->mod_port);
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 21af1dffa837..bb257e91cdf6 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -4047,6 +4047,8 @@ static int phylink_sfp_connect_nophy(void *upstream)
 		}
 	}
 
+	pl->sfp_bus_port->occupied = true;
+
 	pl->mod_port = port;
 
 	return 0;
@@ -4056,6 +4058,8 @@ static void phylink_sfp_disconnect_nophy(void *upstream)
 {
 	struct phylink *pl = upstream;
 
+	pl->sfp_bus_port->occupied = false;
+
 	if (pl->netdev)
 		phy_link_topo_del_port(pl->netdev, pl->mod_port);
 
diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
index 4e2a3fdd2f2e..8d4f20bfd9dd 100644
--- a/include/linux/phy_port.h
+++ b/include/linux/phy_port.h
@@ -51,6 +51,8 @@ struct phy_port_ops {
  * @is_mii: Indicates if this port is MII (Media Independent Interface),
  *          or MDI (Media Dependent Interface).
  * @is_sfp: Indicates if this port drives an SFP cage.
+ * @occupied: Indicates if this port feeds into an another component that has
+ *	      a front-facing interface.
  */
 struct phy_port {
 	u32 id;
@@ -71,6 +73,7 @@ struct phy_port {
 	unsigned int active:1;
 	unsigned int is_mii:1;
 	unsigned int is_sfp:1;
+	unsigned int occupied:1;
 };
 
 struct phy_port *phy_port_alloc(void);
-- 
2.49.0
Re: [net-next,v2,09/12] net: phy: phy_port: Store information about a MII port's occupancy
Posted by Jakub Kicinski 1 week, 2 days ago
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: phy_port: Store information about a MII port's occupancy

This patch adds an "occupied" bitfield to struct phy_port to track
whether an MII port has an SFP module inserted. The field is set in
connect_nophy callbacks and cleared in disconnect_nophy callbacks.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 21af1dffa837..bb257e91cdf6 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -4047,6 +4047,8 @@ static int phylink_sfp_connect_nophy(void *upstream)
>  		}
>  	}
>
> +	pl->sfp_bus_port->occupied = true;

Note that phylink_sfp_connect_nophy() has a NULL check for sfp_bus_port
at the start of the function that returns early if it is NULL.

> +
>  	pl->mod_port = port;
>
>  	return 0;
> @@ -4056,6 +4058,8 @@ static void phylink_sfp_disconnect_nophy(void *upstream)
>  {
>  	struct phylink *pl = upstream;
>
> +	pl->sfp_bus_port->occupied = false;

Can this dereference a NULL pointer? When phylink is created with
PHYLINK_DEV type (rather than PHYLINK_NETDEV), pl->netdev is NULL and
phylink_create_sfp_port() returns early without setting sfp_bus_port.
The callbacks are still registered via sfp_bus_add_upstream(), so when
a PHY-less SFP module is later inserted and removed, this function
would be called with sfp_bus_port being NULL.

The connect function checks for NULL sfp_bus_port, but the disconnect
function does not. Should this have a similar guard?

> +
>  	if (pl->netdev)
>  		phy_link_topo_del_port(pl->netdev, pl->mod_port);