[PATCH RESEND net-next v17 00/10] net: phy_port: SFP modules representation and phy_port listing

[PATCH RESEND net-next v17 00/10] net: phy_port: SFP modules representation and phy_port listing

[Maxime Chevallier]

Hello everyone,

Here's V17 (again) for the phy_port improved SFP support and netlink
interface. Resending as some of yesterday's patches didn't feel like
going to the netdev list...

V17 addresses the port_id wrapparound with the xarray insertion, and the
missing comment during the ethtool header regen from the YNL specs.

This work extends on the recent addition of phy_port representation to enable
listing the front-facing ports of an interface. For now, we don't control
these ports, we merely list their presence and their capabilities.

As the most common use-case of multi-port interfaces is combo-ports that
provide both RJ45 and SFP connectors on a single MAC, there's a lot of
SFP stuff in this series.

This series is in 2 main parts. The first one aims at representing the
SFP cages and modules using phy_port, as combo-ports with RJ45 + SFP are
by far the most common cases for multi-connector setups.

The second part is the netlink interface to list those ports, now that
most use-cases are covered.

Let's see what we can do with some examples of the new ethtool API :

- Get MII interfaces supported by an empty SFP cage :

# ethtool --show-ports eth3

Port for eth3:
	Port id: 1
	Supported MII interfaces : sgmii, 1000base-x, 2500base-x
	Port type: sfp

- Get Combo-ports supported modes, on each port :

# ethtool --show-ports eth1

Port for eth1:
	Port id: 1
	Supported link modes:  10baseT/Half 10baseT/Full
	                       100baseT/Half 100baseT/Full
	                       1000baseT/Full
	                       10000baseT/Full
	                       2500baseT/Full
	                       5000baseT/Full

	Port type: mdi

Port for eth1:
	Port id: 2
	Supported MII interfaces : 10gbase-r
	Port type: sfp

- Get Achievable linkmodes on a SFP module (combo port with a DAC in the
SFP cage)

# ethtool --show-ports eth1

Port for eth1:
	Port id: 1
	Supported link modes:  10baseT/Half 10baseT/Full
	                       100baseT/Half 100baseT/Full
	                       1000baseT/Full
	                       10000baseT/Full
	                       2500baseT/Full
	                       5000baseT/Full
	Port type: mdi

Port for eth1:
	Port id: 2
	Supported MII interfaces : 10gbase-r
	Port type: sfp

Port for eth1:
	Port id: 3
	Upstream id: 2
	Supported link modes:  10000baseCR/Full
	Port type: mdi

Note that here, we have 3 ports :
 - The Copper port
 - The SFP Cage itself, marked as 'occupied'
 - The SFP module

This series builds on top of phy_port and phy_link_topology to allow
tracking the ports of an interface. We maintain a list of supported
linkmodes/interfaces on each port, which allows for fine-grained
reporting of each port's capability.

What this series doesn't do :
 - We don't support selecting which port is active. This is the next step.
 - We only support PHY-driven combo ports. The end-goal of this whole
   journey that started with phy_link_topology is to get support for MII
   muxes, such as the one we have on the Turris Omnia. This will eventually
   be upstreamed as well.

If you want to play around with it, here's [1] the patched ethtool that I've
been using to produce the outputs above.

Thanks !

Maxime

[1] : https://github.com/minimaxwell/ethtool/tree/mc/ethtool_port

Changelog :

Changes in v17:
 - Handle port index wraparound
 - Regen the ethtool headers, now with the comment :)

Changes in v16:
https://lore.kernel.org/r/20260902212135.1805948-1-maxime.chevallier@bootlin.com
 - unregister phy_ports at phy_detach
 - typos in patch 1 (of -> or)
 - Fix the netlink specs

Changes in v15:
https://lore.kernel.org/netdev/20260801074332.98755-1-maxime.chevallier@bootlin.com/
 - Fail on port access for ops-locked devices
 - Use a direct phy_port pointer for upstream tracking
 - Addressed the kdoc issues

Changes in v14:
https://lore.kernel.org/netdev/20260720174808.139573-1-maxime.chevallier@bootlin.com/
 - Added a big comment on the first patch for the topo alloc potential
   races
 - Fixed the multi-buff DUMP on last patch with a separate iterator for
   dumps
 - Fixed some typos in the documentation

Changes in V13:
V13: https://lore.kernel.org/r/20260701110427.143945-1-maxime.chevallier@bootlin.com
 - Rebase on net-next
 - Fix the SFP bus cleanup path in patch 5

Changes in V12:
V12: https://lore.kernel.org/r/20260615153907.862987-1-maxime.chevallier@bootlin.com
 - Rebased on net-next, including fixes on the phy probing and cleanup
   paths
 - Rebased on Jakub's netdev_ops_locked changes in phy_link_topology
 - Fixed some typos reported by Andrew and sashiko in the documentation

Changes in v11:
V11:https://lore.kernel.org/r/20260521121040.1199622-1-maxime.chevallier@bootlin.com
 - Aggregated Andrew's reviews :)
 - Removed the "vacant" field, replaced it with "upstream_port"

Changes in V10:
V10: https://lore.kernel.org/r/20260513130521.1064094-1-maxime.chevallier@bootlin.com
 - Rebase on net-next
 - Rename phylink/phy_device sfp_bus_port to sfp_cage_port
 - Sashiko's reviews were mostly unrealistic or wrong :(

Changes in V9:
V9: https://lore.kernel.org/r/20260403123755.175742-1-maxime.chevallier@bootlin.com
 - Added missing netlink doc updates for u8->u32 conversion
 - Removed dead code with a condition that can never be true in
   phylink's mod_port code
 - Fixed the error path in phy_sfp_connect_phy

Changes in v8:
V8: https://lore.kernel.org/r/20260325081937.571115-1-maxime.chevallier@bootlin.com
 - Set the new phydev.has_sfp_mod_phy field when we're sure that no
   errors occured
 - Fix formatting of the copyright info in ethnl port
 - Use a policy to validate the range of port_id
 - Use GENL_REQ_ATTR_CHECK
 - alpha-sort headers
 - use u32 in netlink messages
 - return better error codes
 - don't check the skb len, the core does that

Changes in V7:
V7: https://lore.kernel.org/all/20260309152747.702373-1-maxime.chevallier@bootlin.com/
 - Changed the port cleanup path to use list_for_each_entry_continue_reverse
 - Adjusted the cleanup path in phylink for the port vacant state
 - Pass the right cmd for the netlink dump message

Changes in V6:
V6: https://lore.kernel.org/r/20260304145444.442334-1-maxime.chevallier@bootlin.com
 - Added some comments in th mod_port cleanup
 - changed some kmalloc to kmalloc_obj
 - Removed some phy_link_topo_del_port that wasn't needed

Changes in V5:
V5: https://lore.kernel.org/r/20260205092317.755906-1-maxime.chevallier@bootlin.com
 - Fixed a check on a potentially un-initialized pointer, reported by
   Simon
 - Fixed a documentation formatting issue
 - Remove a stray pr_info
 - Rebased on net-next

Changes in V4:
V4 : https://lore.kernel.org/netdev/20260203172839.548524-1-maxime.chevallier@bootlin.com/
 - Add a cleanup patch for the of port parsing
 - Added a match to sync the port's linkmodes with the PHY's for OF
   ports
 - Added RTNL assert in the port_get topo helper
 - nullify the bus port for phylink support
 - Fix some typos

Changes in V3:
V3: https://lore.kernel.org/netdev/20260201151249.642015-1-maxime.chevallier@bootlin.com/
 - Remove the sfp bus ops for nophy, and use .module_start() as
   suggested by Russell
 - Added missing cleanup for the topology, as per AI review
 - Fixed a few typos as per Romain's review
 - Changed "occupied" to "vacant" as per Romain's review
 - Added missing checks for null ports, per AI review

Changes in V2:
V2: https://lore.kernel.org/netdev/20260128204526.170927-1-maxime.chevallier@bootlin.com/
 - Fix the cleanup path of phy_link_topo_add_phy, as per AI review
 - Fix the cleanup path of phy_sfp_probe, as per AI review
 - Fix the call-site of the disconnect_nophy sfp bus ops, per AI review
 - Fix the netdev-less case uin phylink, per AI review
 - Fix the prototype of phy_link_topo_get_port for the stubs
 - Dropped patch 11. It ended-up breaking 'allnoconfig', so instead we
   built a phy_interface_names array in net/ethtool/netlink.c
 - Fix an ethool-netlink spec discrepancy with the type of an attribute
 - Fix the size computation in the netlink port API
 - Fix the cleanup path in the netlink port API

V1: https://lore.kernel.org/netdev/20260127134202.8208-1-maxime.chevallier@bootlin.com/



Maxime Chevallier (10):
  net: phy: phy_link_topology: Add a helper for opportunistic alloc
  net: phy: phy_link_topology: Track ports in phy_link_topology
  net: phylink: Register a phy_port for MAC-driven SFP cages
  net: phy: Create SFP phy_port before registering upstream
  net: phy: Represent PHY-less SFP modules with phy_port
  net: phy: phy_port: Store information about a port's upstream
  net: phy: phy_link_topology: Add a helper to retrieve ports
  netlink: specs: Add ethernet port listing with ethtool
  net: ethtool: Introduce ethtool command to list ports
  Documentation: networking: Update the phy_port infrastructure
    description

 Documentation/netlink/specs/ethtool.yaml      |  55 +++
 Documentation/networking/ethtool-netlink.rst  |  34 ++
 Documentation/networking/phy-port.rst         |  26 +-
 MAINTAINERS                                   |   1 +
 drivers/net/phy/phy-caps.h                    |   2 +
 drivers/net/phy/phy_caps.c                    |  26 ++
 drivers/net/phy/phy_device.c                  | 186 +++++++--
 drivers/net/phy/phy_link_topology.c           | 101 ++++-
 drivers/net/phy/phylink.c                     | 128 +++++-
 include/linux/phy.h                           |  10 +
 include/linux/phy_link_topology.h             |  44 ++
 include/linux/phy_port.h                      |   5 +
 .../uapi/linux/ethtool_netlink_generated.h    |  22 +
 net/core/dev.c                                |   1 +
 net/ethtool/Makefile                          |   2 +-
 net/ethtool/netlink.c                         |  25 ++
 net/ethtool/netlink.h                         |   8 +
 net/ethtool/port.c                            | 382 ++++++++++++++++++
 18 files changed, 1020 insertions(+), 38 deletions(-)
 create mode 100644 net/ethtool/port.c

-- 
2.55.0

[PATCH RESEND net-next v17 01/10] net: phy: phy_link_topology: Add a helper for opportunistic alloc

[Maxime Chevallier]

The phy_link_topology structure stores information about the PHY-related
components connected to a net_device. It is opportunistically allocated,
when we add the first item to the topology, as this is not relevant for
all kinds of net_devices.

In preparation for the addition of phy_port tracking in the topology,
let's make a dedicated helper for that allocation sequence.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_link_topology.c | 40 +++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index 4134de7ae313..0462283c8020 100644
--- a/drivers/net/phy/phy_link_topology.c
+++ b/drivers/net/phy/phy_link_topology.c
@@ -28,11 +28,39 @@ static int netdev_alloc_phy_link_topology(struct net_device *dev)
 	return 0;
 }
 
+static struct phy_link_topology *phy_link_topo_get_or_alloc(struct net_device *dev)
+{
+	int ret;
+
+	if (dev->link_topo)
+		return dev->link_topo;
+
+	/* The topology is allocated the first time we add an object to it.
+	 * It is freed alongside the netdev. It can be called on multiple
+	 * contexts:
+	 *  - It can be called from .probe() : No rtnl, no netdev_lock
+	 *  - .ndo_open() : rtnl and possibly netdev_lock
+	 *  - SFP state machine : rtnl held or not
+	 *
+	 *  However, we can't really have races :
+	 *  - If we have a PHY, phy_link_topo_add_phy() will always run first
+	 *    and trigger the alloc. Only then the ports can be added through
+	 *    phylib or sfp.
+	 *  - If we don't, the SFP port for the cage is registered first, and
+	 *    only then other ports/PHYs can be registered.
+	 */
+	ret = netdev_alloc_phy_link_topology(dev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return dev->link_topo;
+}
+
 int phy_link_topo_add_phy(struct net_device *dev,
 			  struct phy_device *phy,
 			  enum phy_upstream upt, void *upstream)
 {
-	struct phy_link_topology *topo = dev->link_topo;
+	struct phy_link_topology *topo;
 	struct phy_device_node *pdn;
 	int ret;
 
@@ -45,13 +73,9 @@ int phy_link_topo_add_phy(struct net_device *dev,
 	if (WARN_ON_ONCE(netdev_need_ops_lock(dev)))
 		return -EOPNOTSUPP;
 
-	if (!topo) {
-		ret = netdev_alloc_phy_link_topology(dev);
-		if (ret)
-			return ret;
-
-		topo = dev->link_topo;
-	}
+	topo = phy_link_topo_get_or_alloc(dev);
+	if (IS_ERR(topo))
+		return PTR_ERR(topo);
 
 	pdn = kzalloc_obj(*pdn);
 	if (!pdn)
-- 
2.55.0

[PATCH RESEND net-next v17 02/10] net: phy: phy_link_topology: Track ports in phy_link_topology

[Maxime Chevallier]

phy_port is aimed at representing the various physical interfaces of a
net_device. They can be controlled by various components in the link,
such as the Ethernet PHY, the Ethernet MAC, and SFP module, etc.

Let's therefore make so we keep track of all the ports connected to a
netdev in phy_link_topology. The only ports added for now are phy-driven
ports.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c        |  3 ++
 drivers/net/phy/phy_link_topology.c | 53 +++++++++++++++++++++++++++++
 include/linux/phy_link_topology.h   | 18 ++++++++++
 include/linux/phy_port.h            |  2 ++
 net/core/dev.c                      |  1 +
 5 files changed, 77 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 41d1b20cde2c..2d0fa3c7f18e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1670,6 +1670,9 @@ static void phy_del_port(struct phy_device *phydev, struct phy_port *port)
 
 	list_del(&port->head);
 
+	if (phydev->attached_dev)
+		phy_link_topo_del_port(phydev->attached_dev, port);
+
 	phydev->n_ports--;
 }
 
diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index 0462283c8020..28c5bf3a8849 100644
--- a/drivers/net/phy/phy_link_topology.c
+++ b/drivers/net/phy/phy_link_topology.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/phy_link_topology.h>
+#include <linux/phy_port.h>
 #include <linux/phy.h>
 #include <linux/rtnetlink.h>
 #include <linux/xarray.h>
@@ -23,6 +24,9 @@ static int netdev_alloc_phy_link_topology(struct net_device *dev)
 	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
 	topo->next_phy_index = 1;
 
+	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
+	topo->next_port_index = 1;
+
 	dev->link_topo = topo;
 
 	return 0;
@@ -56,12 +60,45 @@ static struct phy_link_topology *phy_link_topo_get_or_alloc(struct net_device *d
 	return dev->link_topo;
 }
 
+int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port)
+{
+	struct phy_link_topology *topo;
+	int ret;
+
+	topo = phy_link_topo_get_or_alloc(dev);
+	if (IS_ERR(topo))
+		return PTR_ERR(topo);
+
+	/* Attempt to re-use a previously allocated port_id */
+	if (port->id)
+		ret = xa_insert(&topo->ports, port->id, port, GFP_KERNEL);
+	else
+		ret = xa_alloc_cyclic(&topo->ports, &port->id, port,
+				      xa_limit_32b, &topo->next_port_index,
+				      GFP_KERNEL);
+
+	return ret < 0 ? ret : 0;
+}
+EXPORT_SYMBOL_GPL(phy_link_topo_add_port);
+
+void phy_link_topo_del_port(struct net_device *dev, struct phy_port *port)
+{
+	struct phy_link_topology *topo = dev->link_topo;
+
+	if (!topo)
+		return;
+
+	xa_erase(&topo->ports, port->id);
+}
+EXPORT_SYMBOL_GPL(phy_link_topo_del_port);
+
 int phy_link_topo_add_phy(struct net_device *dev,
 			  struct phy_device *phy,
 			  enum phy_upstream upt, void *upstream)
 {
 	struct phy_link_topology *topo;
 	struct phy_device_node *pdn;
+	struct phy_port *port;
 	int ret;
 
 	/* ethtool ops may run without rtnl_lock, and rtnl_lock is what
@@ -110,8 +147,20 @@ int phy_link_topo_add_phy(struct net_device *dev,
 	if (ret < 0)
 		goto err;
 
+	/* Add all the PHY's ports to the topology */
+	list_for_each_entry(port, &phy->ports, head) {
+		ret = phy_link_topo_add_port(dev, port);
+		if (ret)
+			goto del_ports;
+	}
+
 	return 0;
 
+del_ports:
+	list_for_each_entry_continue_reverse(port, &phy->ports, head)
+		phy_link_topo_del_port(dev, port);
+
+	xa_erase(&topo->phys, phy->phyindex);
 err:
 	kfree(pdn);
 	return ret;
@@ -123,10 +172,14 @@ void phy_link_topo_del_phy(struct net_device *dev,
 {
 	struct phy_link_topology *topo = dev->link_topo;
 	struct phy_device_node *pdn;
+	struct phy_port *port;
 
 	if (!topo)
 		return;
 
+	list_for_each_entry(port, &phy->ports, head)
+		phy_link_topo_del_port(dev, port);
+
 	pdn = xa_erase(&topo->phys, phy->phyindex);
 
 	/* We delete the PHY from the topology, however we don't re-set the
diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
index 95575f68d5bc..296ee514ba46 100644
--- a/include/linux/phy_link_topology.h
+++ b/include/linux/phy_link_topology.h
@@ -16,11 +16,15 @@
 
 struct xarray;
 struct phy_device;
+struct phy_port;
 struct sfp_bus;
 
 struct phy_link_topology {
 	struct xarray phys;
 	u32 next_phy_index;
+
+	struct xarray ports;
+	u32 next_port_index;
 };
 
 struct phy_device_node {
@@ -48,6 +52,9 @@ int phy_link_topo_add_phy(struct net_device *dev,
 
 void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy);
 
+int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port);
+void phy_link_topo_del_port(struct net_device *dev, struct phy_port *port);
+
 static inline struct phy_device *
 phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
 {
@@ -77,6 +84,17 @@ static inline void phy_link_topo_del_phy(struct net_device *dev,
 {
 }
 
+static inline int phy_link_topo_add_port(struct net_device *dev,
+					 struct phy_port *port)
+{
+	return 0;
+}
+
+static inline void phy_link_topo_del_port(struct net_device *dev,
+					  struct phy_port *port)
+{
+}
+
 static inline struct phy_device *
 phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
 {
diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
index 0ef0f5ce4709..4e2a3fdd2f2e 100644
--- a/include/linux/phy_port.h
+++ b/include/linux/phy_port.h
@@ -36,6 +36,7 @@ struct phy_port_ops {
 /**
  * struct phy_port - A representation of a network device physical interface
  *
+ * @id: Unique identifier for the port within the topology
  * @head: Used by the port's parent to list ports
  * @parent_type: The type of device this port is directly connected to
  * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
@@ -52,6 +53,7 @@ struct phy_port_ops {
  * @is_sfp: Indicates if this port drives an SFP cage.
  */
 struct phy_port {
+	u32 id;
 	struct list_head head;
 	enum phy_port_parent parent_type;
 	union {
diff --git a/net/core/dev.c b/net/core/dev.c
index 290e0f099e6b..8c061df5b6a8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11367,6 +11367,7 @@ static void netdev_free_phy_link_topology(struct net_device *dev)
 
 	if (IS_ENABLED(CONFIG_PHYLIB) && topo) {
 		xa_destroy(&topo->phys);
+		xa_destroy(&topo->ports);
 		kfree(topo);
 		dev->link_topo = NULL;
 	}
-- 
2.55.0

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

[Maxime Chevallier]

phy_port tracks the interfaces that a netdevice feeds into. SFP cages are
such ports, but so far we are only tracking the ones that are driven by
PHYs acting as media-converters.

Let's populate a phy_port for MAC driven SFP cages, handled by phylink.

This phy_port represents the SFP cage itself, and not the module that
may be plugged into it. It's therefore not an MDI interface, so only the
'interfaces' field is relevant here.

The phy_port is only populated for 'NETDEV' phylink instances, as
otherwise we don't have any topology to attach the port to.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phylink.c | 53 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index b9fc8599df74..7c6c4243c711 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -14,6 +14,8 @@
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
 #include <linux/phy_fixed.h>
+#include <linux/phy_link_topology.h>
+#include <linux/phy_port.h>
 #include <linux/phylink.h>
 #include <linux/rtnetlink.h>
 #include <linux/spinlock.h>
@@ -93,6 +95,7 @@ struct phylink {
 	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
 	u8 sfp_port;
+	struct phy_port *sfp_cage_port;
 
 	struct eee_config eee_cfg;
 
@@ -1765,6 +1768,46 @@ 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;
+
+	phy_interface_and(port->interfaces, pl->config->supported_interfaces,
+			  phylink_sfp_interfaces);
+	phy_port_update_supported(port);
+
+	ret = phy_link_topo_add_port(pl->netdev, port);
+	if (ret)
+		phy_port_destroy(port);
+	else
+		pl->sfp_cage_port = port;
+
+	return ret;
+}
+
+static void phylink_destroy_sfp_cage_port(struct phylink *pl)
+{
+	if (pl->netdev && pl->sfp_cage_port)
+		phy_link_topo_del_port(pl->netdev, pl->sfp_cage_port);
+
+	if (pl->sfp_cage_port)
+		phy_port_destroy(pl->sfp_cage_port);
+
+	pl->sfp_cage_port = NULL;
+}
+
 static int phylink_register_sfp(struct phylink *pl,
 				const struct fwnode_handle *fwnode)
 {
@@ -1782,9 +1825,18 @@ static int phylink_register_sfp(struct phylink *pl,
 
 	pl->sfp_bus = bus;
 
+	ret = phylink_create_sfp_cage_port(pl);
+	if (ret) {
+		sfp_bus_put(bus);
+		return ret;
+	}
+
 	ret = sfp_bus_add_upstream(bus, pl, &sfp_phylink_ops);
 	sfp_bus_put(bus);
 
+	if (ret)
+		phylink_destroy_sfp_cage_port(pl);
+
 	return ret;
 }
 
@@ -2067,6 +2119,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);
 	if (pl->link_gpio)
 		gpiod_put(pl->link_gpio);
 
-- 
2.55.0

[PATCH RESEND net-next v17 04/10] net: phy: Create SFP phy_port before registering upstream

[Maxime Chevallier]

When dealing with PHY-driven SFP, we create a phy_port representing the
SFP bus when we know we have such a bus.

We can move the port creation before registering the sfp upstream ops,
as long as we know the SFP bus is there. This will allow passing the
phy_port along with the upstream information to the SFP bus.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 58 +++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2d0fa3c7f18e..f15c0fc339b4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1676,13 +1676,13 @@ static void phy_del_port(struct phy_device *phydev, struct phy_port *port)
 	phydev->n_ports--;
 }
 
-static int phy_setup_sfp_port(struct phy_device *phydev)
+static struct phy_port *phy_setup_sfp_port(struct phy_device *phydev)
 {
 	struct phy_port *port = phy_port_alloc();
 	int ret;
 
 	if (!port)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	port->parent_type = PHY_PORT_PHY;
 	port->phy = phydev;
@@ -1697,10 +1697,12 @@ static int phy_setup_sfp_port(struct phy_device *phydev)
 	 * when attaching the port to the phydev.
 	 */
 	ret = phy_add_port(phydev, port);
-	if (ret)
+	if (ret) {
 		phy_port_destroy(port);
+		return ERR_PTR(ret);
+	}
 
-	return ret;
+	return port;
 }
 
 /**
@@ -1709,30 +1711,46 @@ static int phy_setup_sfp_port(struct phy_device *phydev)
  */
 static int phy_sfp_probe(struct phy_device *phydev)
 {
+	struct phy_port *port = NULL;
 	struct sfp_bus *bus;
-	int ret = 0;
+	int ret;
 
-	if (phydev->mdio.dev.fwnode) {
-		bus = sfp_bus_find_fwnode(phydev->mdio.dev.fwnode);
-		if (IS_ERR(bus))
-			return PTR_ERR(bus);
+	if (!phydev->mdio.dev.fwnode)
+		return 0;
 
-		phydev->sfp_bus = bus;
+	bus = sfp_bus_find_fwnode(phydev->mdio.dev.fwnode);
+	if (IS_ERR(bus))
+		return PTR_ERR(bus);
 
-		ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
-		sfp_bus_put(bus);
+	phydev->sfp_bus = bus;
 
-		if (ret)
-			phydev->sfp_bus = NULL;
+	if (bus) {
+		port = phy_setup_sfp_port(phydev);
+		if (IS_ERR(port)) {
+			ret = PTR_ERR(port);
+			goto out_sfp;
+		}
 	}
 
-	if (!ret && phydev->sfp_bus) {
-		ret = phy_setup_sfp_port(phydev);
-		if (ret) {
-			sfp_bus_del_upstream(phydev->sfp_bus);
-			phydev->sfp_bus = NULL;
-		}
+	ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
+	if (ret)
+		goto out_port;
+
+	/* sfp_bus_add_upstream() grabs a ref to the sfp bus on success, it's
+	 * safe to release it now.
+	 */
+	sfp_bus_put(bus);
+
+	return ret;
+
+out_port:
+	if (port) {
+		phy_del_port(phydev, port);
+		phy_port_destroy(port);
 	}
+out_sfp:
+	sfp_bus_put(bus);
+	phydev->sfp_bus = NULL;
 
 	return ret;
 }
-- 
2.55.0

[PATCH RESEND net-next v17 05/10] net: phy: Represent PHY-less SFP modules with phy_port

[Maxime Chevallier]

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.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy-caps.h   |   2 +
 drivers/net/phy/phy_caps.c   |  26 +++++++++
 drivers/net/phy/phy_device.c | 100 +++++++++++++++++++++++++++++++++--
 drivers/net/phy/phylink.c    |  76 ++++++++++++++++++++++++--
 include/linux/phy.h          |   6 +++
 5 files changed, 203 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy-caps.h b/drivers/net/phy/phy-caps.h
index 421088e6f6e8..ec3d39a0ae06 100644
--- a/drivers/net/phy/phy-caps.h
+++ b/drivers/net/phy/phy-caps.h
@@ -66,5 +66,7 @@ void phy_caps_medium_get_supported(unsigned long *supported,
 				   enum ethtool_link_medium medium,
 				   int lanes);
 u32 phy_caps_mediums_from_linkmodes(unsigned long *linkmodes);
+void phy_caps_linkmode_filter_ifaces(unsigned long *to, const unsigned long *from,
+				     const unsigned long *interfaces);
 
 #endif /* __PHY_CAPS_H */
diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
index 942d43191561..558e4df4d63c 100644
--- a/drivers/net/phy/phy_caps.c
+++ b/drivers/net/phy/phy_caps.c
@@ -445,3 +445,29 @@ u32 phy_caps_mediums_from_linkmodes(unsigned long *linkmodes)
 	return mediums;
 }
 EXPORT_SYMBOL_GPL(phy_caps_mediums_from_linkmodes);
+
+/**
+ * phy_caps_linkmode_filter_ifaces() - Filter linkmodes with an interface list
+ * @to: Stores the filtered linkmodes
+ * @from: Linkmodes to filter
+ * @interfaces: Bitfield of phy_interface_t that we use for filtering
+ *
+ * Filter the provided linkmodes, only to keep the ones we can possibly achieve
+ * when using any of the provided MII interfaces.
+ */
+void phy_caps_linkmode_filter_ifaces(unsigned long *to,
+				     const unsigned long *from,
+				     const unsigned long *interfaces)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(ifaces_supported) = {};
+	unsigned int ifaces_caps = 0;
+	phy_interface_t interface;
+
+	for_each_set_bit(interface, interfaces, PHY_INTERFACE_MODE_MAX)
+		ifaces_caps |= phy_caps_from_interface(interface);
+
+	phy_caps_linkmodes(ifaces_caps, ifaces_supported);
+
+	linkmode_and(to, from, ifaces_supported);
+}
+EXPORT_SYMBOL_GPL(phy_caps_linkmode_filter_ifaces);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f15c0fc339b4..26634ef274f3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1490,11 +1490,21 @@ static int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
 {
 	struct phy_device *phydev = upstream;
 	struct net_device *dev = phydev->attached_dev;
+	int ret;
 
-	if (dev)
-		return phy_link_topo_add_phy(dev, phy, PHY_UPSTREAM_PHY, phydev);
+	phydev->has_sfp_mod_phy = true;
 
-	return 0;
+	/* If we aren't attached to a netdev, we can't add the SFP PHY to its
+	 * topology.
+	 */
+	if (!dev)
+		return 0;
+
+	ret = phy_link_topo_add_phy(dev, phy, PHY_UPSTREAM_PHY, phydev);
+	if (ret)
+		phydev->has_sfp_mod_phy = false;
+
+	return ret;
 }
 
 /**
@@ -1512,6 +1522,8 @@ static void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
 	struct phy_device *phydev = upstream;
 	struct net_device *dev = phydev->attached_dev;
 
+	phydev->has_sfp_mod_phy = false;
+
 	if (dev)
 		phy_link_topo_del_phy(dev, phy);
 }
@@ -1617,6 +1629,75 @@ static void phy_sfp_link_down(void *upstream)
 		port->ops->link_down(port);
 }
 
+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_cage_port->interfaces);
+
+	if (phydev->attached_dev) {
+		ret = phy_link_topo_add_port(phydev->attached_dev, port);
+		if (ret) {
+			phy_port_destroy(port);
+			return ret;
+		}
+	}
+
+	/* 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
+	 */
+	phydev->mod_port = port;
+
+	return 0;
+}
+
+static void phy_del_sfp_mod_port(struct phy_device *phydev)
+{
+	if (!phydev->mod_port)
+		return;
+
+	if (phydev->attached_dev)
+		phy_link_topo_del_port(phydev->attached_dev, phydev->mod_port);
+
+	phy_port_destroy(phydev->mod_port);
+	phydev->mod_port = NULL;
+}
+
+static int phy_sfp_module_start(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+
+	/* If there's a downstream SFP module, and it doesn't contain a PHY
+	 * device, let's create a phy_port to represent that module.
+	 */
+	if (!phydev->has_sfp_mod_phy)
+		return phy_add_sfp_mod_port(phydev);
+
+	return 0;
+}
+
+static void phy_sfp_module_stop(void *upstream)
+{
+	struct phy_device *phydev = upstream;
+
+	/* Called upon module removal or upstream removal */
+	if (!phydev->has_sfp_mod_phy)
+		phy_del_sfp_mod_port(phydev);
+}
+
 static const struct sfp_upstream_ops sfp_phydev_ops = {
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
@@ -1626,6 +1707,8 @@ static const struct sfp_upstream_ops sfp_phydev_ops = {
 	.link_down = phy_sfp_link_down,
 	.connect_phy = phy_sfp_connect_phy,
 	.disconnect_phy = phy_sfp_disconnect_phy,
+	.module_start = phy_sfp_module_start,
+	.module_stop = phy_sfp_module_stop,
 };
 
 static int phy_add_port(struct phy_device *phydev, struct phy_port *port)
@@ -1728,6 +1811,7 @@ static int phy_sfp_probe(struct phy_device *phydev)
 		port = phy_setup_sfp_port(phydev);
 		if (IS_ERR(port)) {
 			ret = PTR_ERR(port);
+			port = NULL;
 			goto out_sfp;
 		}
 	}
@@ -1741,6 +1825,8 @@ static int phy_sfp_probe(struct phy_device *phydev)
 	 */
 	sfp_bus_put(bus);
 
+	phydev->sfp_cage_port = port;
+
 	return ret;
 
 out_port:
@@ -1834,6 +1920,8 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
 		phy_link_topo_del_phy(dev, phydev);
+		if (phydev->mod_port)
+			phy_link_topo_del_port(dev, phydev->mod_port);
 	}
 
 	phydev->phy_link_change = NULL;
@@ -1963,6 +2051,12 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 		err = phy_link_topo_add_phy(dev, phydev, PHY_UPSTREAM_MAC, dev);
 		if (err)
 			goto error;
+
+		if (phydev->mod_port) {
+			err = phy_link_topo_add_port(dev, phydev->mod_port);
+			if (err)
+				goto error;
+		}
 	}
 
 	/* Some Ethernet drivers try to connect to a PHY device before
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 7c6c4243c711..5a8c66866d17 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -96,6 +96,7 @@ struct phylink {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
 	u8 sfp_port;
 	struct phy_port *sfp_cage_port;
+	struct phy_port *mod_port;
 
 	struct eee_config eee_cfg;
 
@@ -1790,10 +1791,15 @@ static int phylink_create_sfp_cage_port(struct phylink *pl)
 
 	ret = phy_link_topo_add_port(pl->netdev, port);
 	if (ret)
-		phy_port_destroy(port);
-	else
-		pl->sfp_cage_port = port;
+		goto out_destroy_port;
+
+	pl->sfp_cage_port = port;
+
+	return 0;
 
+out_destroy_port:
+	phy_port_destroy(port);
+	pl->sfp_cage_port = NULL;
 	return ret;
 }
 
@@ -3992,14 +3998,65 @@ static void phylink_sfp_module_remove(void *upstream)
 	phy_interface_zero(pl->sfp_interfaces);
 }
 
+static int phylink_add_sfp_mod_port(struct phylink *pl)
+{
+	const struct sfp_module_caps *caps;
+	struct phy_port *port;
+	int ret = 0;
+
+	if (!pl->sfp_cage_port)
+		return 0;
+
+	/* Create mod port */
+	port = phy_port_alloc();
+	if (!port)
+		return -ENOMEM;
+
+	port->active = true;
+
+	caps = sfp_get_module_caps(pl->sfp_bus);
+
+	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
+					pl->sfp_cage_port->interfaces);
+
+	if (pl->netdev) {
+		ret = phy_link_topo_add_port(pl->netdev, port);
+		if (ret) {
+			phy_port_destroy(port);
+			return ret;
+		}
+	}
+
+	pl->mod_port = port;
+
+	return 0;
+}
+
+static void phylink_del_sfp_mod_port(struct phylink *pl)
+{
+	if (!pl->mod_port)
+		return;
+
+	if (pl->netdev)
+		phy_link_topo_del_port(pl->netdev, pl->mod_port);
+
+	phy_port_destroy(pl->mod_port);
+	pl->mod_port = NULL;
+}
+
 static int phylink_sfp_module_start(void *upstream)
 {
 	struct phylink *pl = upstream;
+	int ret;
 
 	/* If this SFP module has a PHY, start the PHY now. */
 	if (pl->phydev) {
 		phy_start(pl->phydev);
 		return 0;
+	} else {
+		ret = phylink_add_sfp_mod_port(pl);
+		if (ret)
+			return ret;
 	}
 
 	/* If the module may have a PHY but we didn't detect one we
@@ -4008,7 +4065,16 @@ static int phylink_sfp_module_start(void *upstream)
 	if (!pl->sfp_may_have_phy)
 		return 0;
 
-	return phylink_sfp_config_optical(pl);
+	ret = phylink_sfp_config_optical(pl);
+	if (ret)
+		goto del_mod_port;
+
+	return 0;
+
+del_mod_port:
+	phylink_del_sfp_mod_port(pl);
+
+	return ret;
 }
 
 static void phylink_sfp_module_stop(void *upstream)
@@ -4018,6 +4084,8 @@ static void phylink_sfp_module_stop(void *upstream)
 	/* If this SFP module has a PHY, stop it. */
 	if (pl->phydev)
 		phy_stop(pl->phydev);
+	else
+		phylink_del_sfp_mod_port(pl);
 }
 
 static void phylink_sfp_link_down(void *upstream)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3d8afe6b7f1c..cb579b8a5b5f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -600,6 +600,7 @@ struct phy_oatc14_sqi_capability {
  * @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN
  * 		 enabled.
  * @is_genphy_driven: PHY is driven by one of the generic PHY drivers
+ * @has_sfp_mod_phy: Set true if downstream SFP bus's module contains a PHY
  * @state: State of the PHY for management purposes
  * @dev_flags: Device-specific flags used by the PHY driver.
  *
@@ -612,6 +613,8 @@ struct phy_oatc14_sqi_capability {
  * @phylink: Pointer to phylink instance for this PHY
  * @sfp_bus_attached: Flag indicating whether the SFP bus has been attached
  * @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
  * @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.
@@ -724,6 +727,7 @@ struct phy_device {
 	unsigned irq_rerun:1;
 
 	unsigned default_timestamp:1;
+	unsigned has_sfp_mod_phy:1;
 
 	int rate_matching;
 
@@ -803,6 +807,8 @@ struct phy_device {
 	/* This may be modified under the rtnl lock */
 	bool sfp_bus_attached;
 	struct sfp_bus *sfp_bus;
+	struct phy_port *sfp_cage_port;
+	struct phy_port *mod_port;
 	struct phylink *phylink;
 	struct net_device *attached_dev;
 	struct mii_timestamper *mii_ts;
-- 
2.55.0

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

[Maxime Chevallier]

MII phy_ports are not meant to be connected directly to a link partner.
They are meant to feed into some media converter devices that will
expose an MDI phy_port, so far we only support SFP modules for that.

In the case an MDI phy_port is backed by an MII port (e.g. a SFP
module's port, backed by the SFP cage port), let's keep track of the
port id of the MII port backing it.

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 26634ef274f3..561d19edf331 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.
@@ -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);
 }
 
 /**
@@ -1661,6 +1664,8 @@ static int phy_add_sfp_mod_port(struct phy_device *phydev)
 	 */
 	phydev->mod_port = port;
 
+	port->upstream_port = phydev->sfp_cage_port;
+
 	return 0;
 }
 
@@ -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();
+
+	phydev->upstream_port = port;
+
+	phy_for_each_port(phydev, local_port)
+		local_port->upstream_port = port;
+}
+EXPORT_SYMBOL_GPL(phy_set_upstream_port);
+
 /**
  * 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 5a8c66866d17..d05cde44f401 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -4027,6 +4027,8 @@ static int phylink_add_sfp_mod_port(struct phylink *pl)
 		}
 	}
 
+	port->upstream_port = pl->sfp_cage_port;
+
 	pl->mod_port = port;
 
 	return 0;
@@ -4130,6 +4132,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);
 }
@@ -4138,6 +4142,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 cb579b8a5b5f..335821eb2217 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;
 	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
diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
index 4e2a3fdd2f2e..8f45c031cbc1 100644
--- a/include/linux/phy_port.h
+++ b/include/linux/phy_port.h
@@ -40,6 +40,8 @@ struct phy_port_ops {
  * @head: Used by the port's parent to list ports
  * @parent_type: The type of device this port is directly connected to
  * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
+ * @upstream_port: Indicates the MII port that feeds this port, if any,
+ *		   e.g. the SFP cage port for a SFP module port.
  * @ops: Callback ops implemented by the port controller
  * @pairs: The number of  pairs this port has, 0 if not applicable
  * @mediums: Bitmask of the physical mediums this port provides access to
@@ -59,6 +61,7 @@ struct phy_port {
 	union {
 		struct phy_device *phy;
 	};
+	struct phy_port *upstream_port;
 
 	const struct phy_port_ops *ops;
 
-- 
2.55.0

[PATCH RESEND net-next v17 07/10] net: phy: phy_link_topology: Add a helper to retrieve ports

[Maxime Chevallier]

In order to allow netlink access to phy_ports, let's add a helper to
retrieve them. When handling a port coming from phy_link_topology, the
caller must hold rtnl until it's done with it.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_link_topology.c |  8 ++++++++
 include/linux/phy_link_topology.h   | 26 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index 28c5bf3a8849..22885bf2e045 100644
--- a/drivers/net/phy/phy_link_topology.c
+++ b/drivers/net/phy/phy_link_topology.c
@@ -65,6 +65,14 @@ int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port)
 	struct phy_link_topology *topo;
 	int ret;
 
+	/* Ports can now be queried without rtnl for ops-locked devices, which
+	 * we don't support now as port retrieval is done under rtnl.
+	 * We don't have phy_port enabled devices yet, let's make sure
+	 * we are loudly warned about that when it happens.
+	 */
+	if (WARN_ON_ONCE(netdev_need_ops_lock(dev)))
+		return -EOPNOTSUPP;
+
 	topo = phy_link_topo_get_or_alloc(dev);
 	if (IS_ERR(topo))
 		return PTR_ERR(topo);
diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
index 296ee514ba46..9585282cae3f 100644
--- a/include/linux/phy_link_topology.h
+++ b/include/linux/phy_link_topology.h
@@ -13,6 +13,7 @@
 
 #include <linux/ethtool.h>
 #include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
 
 struct xarray;
 struct phy_device;
@@ -71,6 +72,25 @@ phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
 	return NULL;
 }
 
+static inline struct phy_port *
+phy_link_topo_get_port(struct net_device *dev, u32 port_id)
+{
+	struct phy_link_topology *topo = dev->link_topo;
+
+	if (!topo)
+		return NULL;
+
+	/* ports in the topo are RTNL protected, but may be accessed under
+	 * netdev_lock for ops-locked devices. For now keep them under rtnl
+	 * protection, as no ops-locked devices have phy_port capabilities
+	 * yet.
+	 */
+	ASSERT_RTNL();
+
+	/* Caller must hold RTNL while handling the phy_port */
+	return xa_load(&topo->ports, port_id);
+}
+
 #else
 static inline int phy_link_topo_add_phy(struct net_device *dev,
 					struct phy_device *phy,
@@ -100,6 +120,12 @@ phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
 {
 	return NULL;
 }
+
+static inline struct phy_port *
+phy_link_topo_get_port(struct net_device *dev, u32 port_id)
+{
+	return NULL;
+}
 #endif
 
 #endif /* __PHY_LINK_TOPOLOGY_H */
-- 
2.55.0

[PATCH RESEND net-next v17 08/10] netlink: specs: Add ethernet port listing with ethtool

[Maxime Chevallier]

Ethernet network interfaces may have more than one front-facing port.
The phy_port infrastructure was introduced to keep track of
these ports, and allow userspace to know about the presence and
capability of these ports. Add a ethnl netlink message to report this
information.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml      | 55 +++++++++++++++++++
 Documentation/networking/ethtool-netlink.rst  | 34 ++++++++++++
 .../uapi/linux/ethtool_netlink_generated.h    | 22 ++++++++
 3 files changed, 111 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 5dd4d1b5d94b..35e028c0be29 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -210,6 +210,11 @@ definitions:
       -
         name: discard
         value: 31
+  -
+    name: port-type
+    doc: type of physical connector
+    type: enum
+    entries: [mdi, sfp]
 
 attribute-sets:
   -
@@ -1905,6 +1910,32 @@ attribute-sets:
         name: link
         type: nest
         nested-attributes: mse-snapshot
+  -
+    name: port
+    attr-cnt-name: --ethtool-a-port-cnt
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: id
+        type: u32
+      -
+        name: supported-modes
+        type: nest
+        nested-attributes: bitset
+      -
+        name: supported-interfaces
+        type: nest
+        nested-attributes: bitset
+      -
+        name: type
+        type: u32
+        enum: port-type
+      -
+        name: upstream-port
+        type: u32
 
 operations:
   enum-model: directional
@@ -2859,6 +2890,30 @@ operations:
             - worst-channel
             - link
       dump: *mse-get-op
+    -
+      name: port-get
+      doc: Get ports attached to an interface
+
+      attribute-set: port
+
+      do:
+        request:
+          attributes:
+            - header
+            - id
+        reply: &port-get-op-reply
+          attributes:
+            - header
+            - id
+            - supported-modes
+            - supported-interfaces
+            - type
+            - upstream-port
+      dump:
+        request:
+          attributes:
+            - header
+        reply: *port-get-op-reply
 
 mcast-groups:
   list:
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index e92abf45faf5..b4326c89b075 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -2537,6 +2537,39 @@ Within each channel nest, only the metrics supported by the PHY will be present.
 See ``struct phy_mse_snapshot`` kernel documentation in
 ``include/linux/phy.h``.
 
+PORT_GET
+========
+
+Retrieve information about the physical connection points of a network device,
+referred to as "ports". User needs to specify a PORT_ID for the DO operation,
+in which case the DO request returns information about that specific port.
+
+As there can be more than one port, the DUMP operation can be used to list the
+ports present on a given interface, by passing an interface index or name in
+the dump request.
+
+Request contents:
+
+  ===================================== ======  ===============================
+  ``ETHTOOL_A_PORT_HEADER``             nested  request header
+  ``ETHTOOL_A_PORT_ID``                 u32     port id
+  ===================================== ======  ===============================
+
+Kernel response contents:
+
+  ======================================= ======  =============================
+  ``ETHTOOL_A_PORT_HEADER``               nested  request header
+  ``ETHTOOL_A_PORT_ID``                   u32     the port's unique identifier,
+                                                  per netdevice.
+  ``ETHTOOL_A_PORT_SUPPORTED_MODES``      bitset  bitset of supported linkmodes
+  ``ETHTOOL_A_PORT_SUPPORTED_INTERFACES`` bitset  bitset of supported MII
+                                                  interfaces
+  ``ETHTOOL_A_PORT_TYPE``                 u32     the port type
+  ``ETHTOOL_A_PORT_UPSTREAM_PORT``        u32     If any, the index of the MII
+                                                  port that feeds into this
+                                                  port.
+  ======================================= ======  =============================
+
 Request translation
 ===================
 
@@ -2647,4 +2680,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_PHY_GET``
   ``SIOCGHWTSTAMP``                   ``ETHTOOL_MSG_TSCONFIG_GET``
   ``SIOCSHWTSTAMP``                   ``ETHTOOL_MSG_TSCONFIG_SET``
+  n/a                                 ``ETHTOOL_MSG_PORT_GET``
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink_generated.h b/include/uapi/linux/ethtool_netlink_generated.h
index 8134baf7860f..660e0c08a766 100644
--- a/include/uapi/linux/ethtool_netlink_generated.h
+++ b/include/uapi/linux/ethtool_netlink_generated.h
@@ -78,6 +78,14 @@ enum ethtool_pse_event {
 	ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR = 64,
 };
 
+/*
+ * type of physical connector
+ */
+enum ethtool_port_type {
+	ETHTOOL_PORT_TYPE_MDI,
+	ETHTOOL_PORT_TYPE_SFP,
+};
+
 enum {
 	ETHTOOL_A_HEADER_UNSPEC,
 	ETHTOOL_A_HEADER_DEV_INDEX,
@@ -840,6 +848,18 @@ enum {
 	ETHTOOL_A_MSE_MAX = (__ETHTOOL_A_MSE_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_PORT_HEADER = 1,
+	ETHTOOL_A_PORT_ID,
+	ETHTOOL_A_PORT_SUPPORTED_MODES,
+	ETHTOOL_A_PORT_SUPPORTED_INTERFACES,
+	ETHTOOL_A_PORT_TYPE,
+	ETHTOOL_A_PORT_UPSTREAM_PORT,
+
+	__ETHTOOL_A_PORT_CNT,
+	ETHTOOL_A_PORT_MAX = (__ETHTOOL_A_PORT_CNT - 1)
+};
+
 enum {
 	ETHTOOL_MSG_USER_NONE = 0,
 	ETHTOOL_MSG_STRSET_GET = 1,
@@ -893,6 +913,7 @@ enum {
 	ETHTOOL_MSG_RSS_CREATE_ACT,
 	ETHTOOL_MSG_RSS_DELETE_ACT,
 	ETHTOOL_MSG_MSE_GET,
+	ETHTOOL_MSG_PORT_GET,
 
 	__ETHTOOL_MSG_USER_CNT,
 	ETHTOOL_MSG_USER_MAX = (__ETHTOOL_MSG_USER_CNT - 1)
@@ -954,6 +975,7 @@ enum {
 	ETHTOOL_MSG_RSS_CREATE_NTF,
 	ETHTOOL_MSG_RSS_DELETE_NTF,
 	ETHTOOL_MSG_MSE_GET_REPLY,
+	ETHTOOL_MSG_PORT_GET_REPLY,
 
 	__ETHTOOL_MSG_KERNEL_CNT,
 	ETHTOOL_MSG_KERNEL_MAX = (__ETHTOOL_MSG_KERNEL_CNT - 1)
-- 
2.55.0

[PATCH RESEND net-next v17 09/10] net: ethtool: Introduce ethtool command to list ports

[Maxime Chevallier]

Expose the phy_port information to userspace, so that we can know how
many ports are available on a given interface, as well as their
capabilities. For MDI ports, we report the list of supported linkmodes
based on what the PHY that drives this port says.
For MII ports, i.e. empty SFP cages, we report the MII linkmodes that we
can output on this port.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 MAINTAINERS           |   1 +
 net/ethtool/Makefile  |   2 +-
 net/ethtool/netlink.c |  25 +++
 net/ethtool/netlink.h |   8 +
 net/ethtool/port.c    | 382 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 417 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/port.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 878ecf1fdf27..973bbb7d2419 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18994,6 +18994,7 @@ F:	Documentation/devicetree/bindings/net/ethernet-connector.yaml
 F:	Documentation/networking/phy-port.rst
 F:	drivers/net/phy/phy_port.c
 F:	include/linux/phy_port.h
+F:	net/ethtool/port.c
 K:	struct\s+phy_port|phy_port_
 
 NETWORKING [GENERAL]
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 629c10916670..9b5b09670008 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -9,4 +9,4 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
 		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o mm.o \
 		   module.o cmis_fw_update.o cmis_cdb.o pse-pd.o plca.o \
-		   phy.o tsconfig.o mse.o
+		   phy.o tsconfig.o mse.o port.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 1af395b54330..c076c07d0a08 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -26,6 +26,8 @@ static u32 ethnl_bcast_seq;
 			     ETHTOOL_FLAG_OMIT_REPLY)
 #define ETHTOOL_FLAGS_STATS (ETHTOOL_FLAGS_BASIC | ETHTOOL_FLAG_STATS)
 
+char phy_interface_names[PHY_INTERFACE_MODE_MAX][ETH_GSTRING_LEN] __ro_after_init;
+
 const struct nla_policy ethnl_header_policy[] = {
 	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
 	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
@@ -431,6 +433,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_TSCONFIG_SET]	= &ethnl_tsconfig_request_ops,
 	[ETHTOOL_MSG_PHY_GET]		= &ethnl_phy_request_ops,
 	[ETHTOOL_MSG_MSE_GET]		= &ethnl_mse_request_ops,
+	[ETHTOOL_MSG_PORT_GET]		= &ethnl_port_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1572,6 +1575,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_mse_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_mse_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PORT_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_port_dump_start,
+		.dumpit	= ethnl_port_dumpit,
+		.done	= ethnl_port_dump_done,
+		.policy = ethnl_port_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_port_get_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
@@ -1594,10 +1606,23 @@ static struct genl_family ethtool_genl_family __ro_after_init = {
 
 /* module setup */
 
+static void __init ethnl_phy_names_populate(void)
+{
+	const char *name;
+	int i;
+
+	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
+		name = phy_modes(i);
+		strscpy(phy_interface_names[i], name, ETH_GSTRING_LEN);
+	}
+}
+
 static int __init ethnl_init(void)
 {
 	int ret;
 
+	ethnl_phy_names_populate();
+
 	ret = genl_register_family(&ethtool_genl_family);
 	if (WARN(ret < 0, "ethtool: genetlink family registration failed"))
 		return ret;
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 3e969a070f9f..dc7211f67c4d 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -5,11 +5,14 @@
 
 #include <linux/ethtool_netlink.h>
 #include <linux/netdevice.h>
+#include <linux/phy.h>
 #include <net/genetlink.h>
 #include <net/sock.h>
 
 struct ethnl_req_info;
 
+extern char phy_interface_names[PHY_INTERFACE_MODE_MAX][ETH_GSTRING_LEN];
+
 int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 			       const struct nlattr *nest, struct net *net,
 			       struct netlink_ext_ack *extack,
@@ -474,6 +477,7 @@ extern const struct ethnl_request_ops ethnl_mm_request_ops;
 extern const struct ethnl_request_ops ethnl_phy_request_ops;
 extern const struct ethnl_request_ops ethnl_tsconfig_request_ops;
 extern const struct ethnl_request_ops ethnl_mse_request_ops;
+extern const struct ethnl_request_ops ethnl_port_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -530,6 +534,7 @@ extern const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1];
 extern const struct nla_policy ethnl_tsconfig_get_policy[ETHTOOL_A_TSCONFIG_HEADER + 1];
 extern const struct nla_policy ethnl_tsconfig_set_policy[ETHTOOL_A_TSCONFIG_MAX + 1];
 extern const struct nla_policy ethnl_mse_get_policy[ETHTOOL_A_MSE_HEADER + 1];
+extern const struct nla_policy ethnl_port_get_policy[ETHTOOL_A_PORT_ID + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
@@ -545,6 +550,9 @@ int ethnl_tsinfo_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_tsinfo_done(struct netlink_callback *cb);
 int ethnl_rss_create_doit(struct sk_buff *skb, struct genl_info *info);
 int ethnl_rss_delete_doit(struct sk_buff *skb, struct genl_info *info);
+int ethnl_port_dump_start(struct netlink_callback *cb);
+int ethnl_port_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
+int ethnl_port_dump_done(struct netlink_callback *cb);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
diff --git a/net/ethtool/port.c b/net/ethtool/port.c
new file mode 100644
index 000000000000..0d6150e5f66a
--- /dev/null
+++ b/net/ethtool/port.c
@@ -0,0 +1,382 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2026 Bootlin
+ */
+#include <linux/phy.h>
+#include <linux/phy_link_topology.h>
+#include <linux/phy_port.h>
+#include <net/netdev_lock.h>
+
+#include "bitset.h"
+#include "common.h"
+#include "netlink.h"
+
+struct port_req_info {
+	struct ethnl_req_info base;
+	u32 port_id;
+};
+
+struct port_reply_data {
+	struct ethnl_reply_data	base;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+	u32 port_id;
+	bool mii;
+	bool sfp;
+	u32 upstream_port;
+};
+
+#define PORT_REQINFO(__req_base) \
+	container_of(__req_base, struct port_req_info, base)
+
+#define PORT_REPDATA(__reply_base) \
+	container_of(__reply_base, struct port_reply_data, base)
+
+const struct nla_policy ethnl_port_get_policy[ETHTOOL_A_PORT_ID + 1] = {
+	[ETHTOOL_A_PORT_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_PORT_ID] = NLA_POLICY_MIN(NLA_U32, 1),
+};
+
+static int port_parse_request(struct ethnl_req_info *req_info,
+			      const struct genl_info *info,
+			      struct nlattr **tb,
+			      struct netlink_ext_ack *extack)
+{
+	struct port_req_info *request = PORT_REQINFO(req_info);
+
+	if (GENL_REQ_ATTR_CHECK(info, ETHTOOL_A_PORT_ID))
+		return -EINVAL;
+
+	request->port_id = nla_get_u32(tb[ETHTOOL_A_PORT_ID]);
+
+	return 0;
+}
+
+static int port_prepare_data(const struct ethnl_req_info *req_info,
+			     struct ethnl_reply_data *reply_data,
+			     const struct genl_info *info)
+{
+	struct port_reply_data *reply = PORT_REPDATA(reply_data);
+	struct port_req_info *request = PORT_REQINFO(req_info);
+	struct phy_port *port;
+
+	/* RTNL must be held while holding a ref to the phy_port. Here, caller
+	 * holds RTNL.
+	 */
+	port = phy_link_topo_get_port(req_info->dev, request->port_id);
+	if (!port)
+		return -ENODEV;
+
+	linkmode_copy(reply->supported, port->supported);
+	phy_interface_copy(reply->interfaces, port->interfaces);
+	reply->port_id = port->id;
+	reply->mii = port->is_mii;
+	reply->sfp = port->is_sfp;
+	reply->upstream_port = port->upstream_port ? port->upstream_port->id : 0;
+
+	return 0;
+}
+
+static int port_reply_size(const struct ethnl_req_info *req_info,
+			   const struct ethnl_reply_data *reply_data)
+{
+	bool compact = req_info->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
+	struct port_reply_data *reply = PORT_REPDATA(reply_data);
+	size_t size = 0;
+	int ret;
+
+	/* ETHTOOL_A_PORT_ID */
+	size += nla_total_size(sizeof(u32));
+
+	if (!reply->mii) {
+		/* ETHTOOL_A_PORT_SUPPORTED_MODES */
+		ret = ethnl_bitset_size(reply->supported, NULL,
+					__ETHTOOL_LINK_MODE_MASK_NBITS,
+					link_mode_names, compact);
+		if (ret < 0)
+			return ret;
+
+		size += ret;
+	} else {
+		/* ETHTOOL_A_PORT_SUPPORTED_INTERFACES */
+		ret = ethnl_bitset_size(reply->interfaces, NULL,
+					PHY_INTERFACE_MODE_MAX,
+					phy_interface_names, compact);
+		if (ret < 0)
+			return ret;
+
+		size += ret;
+	}
+
+	/* ETHTOOL_A_PORT_TYPE */
+	size += nla_total_size(sizeof(u32));
+
+	/* ETHTOOL_A_PORT_UPSTREAM_PORT */
+	if (reply->upstream_port)
+		size += nla_total_size(sizeof(u32));
+
+	return size;
+}
+
+static int port_fill_reply(struct sk_buff *skb,
+			   const struct ethnl_req_info *req_info,
+			   const struct ethnl_reply_data *reply_data)
+{
+	bool compact = req_info->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
+	struct port_reply_data *reply = PORT_REPDATA(reply_data);
+	int ret, port_type = ETHTOOL_PORT_TYPE_MDI;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PORT_ID, reply->port_id))
+		return -EMSGSIZE;
+
+	if (!reply->mii) {
+		ret = ethnl_put_bitset(skb, ETHTOOL_A_PORT_SUPPORTED_MODES,
+				       reply->supported, NULL,
+				       __ETHTOOL_LINK_MODE_MASK_NBITS,
+				       link_mode_names, compact);
+		if (ret < 0)
+			return ret;
+	} else {
+		ret = ethnl_put_bitset(skb, ETHTOOL_A_PORT_SUPPORTED_INTERFACES,
+				       reply->interfaces, NULL,
+				       PHY_INTERFACE_MODE_MAX,
+				       phy_interface_names, compact);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (reply->mii || reply->sfp)
+		port_type = ETHTOOL_PORT_TYPE_SFP;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PORT_TYPE, port_type))
+		return -EMSGSIZE;
+
+	if (reply->upstream_port &&
+	    nla_put_u32(skb, ETHTOOL_A_PORT_UPSTREAM_PORT,
+			reply->upstream_port))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+struct port_dump_ctx {
+	struct port_req_info	*req_info;
+	struct port_reply_data	*reply_data;
+	unsigned long		ifindex;
+	unsigned long		pos_ifindex;
+	unsigned long		pos_portid;
+};
+
+static struct port_dump_ctx *
+port_dump_ctx_get(struct netlink_callback *cb)
+{
+	return (struct port_dump_ctx *)cb->ctx;
+}
+
+int ethnl_port_dump_start(struct netlink_callback *cb)
+{
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
+	struct nlattr **tb = info->info.attrs;
+	struct port_reply_data *reply_data;
+	struct port_req_info *req_info;
+	int ret;
+
+	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
+
+	req_info = kzalloc_obj(*req_info);
+	if (!req_info)
+		return -ENOMEM;
+
+	reply_data = kmalloc_obj(*reply_data);
+	if (!reply_data) {
+		ret = -ENOMEM;
+		goto free_req_info;
+	}
+
+	ret = ethnl_parse_header_dev_get(&req_info->base, tb[ETHTOOL_A_PORT_HEADER],
+					 genl_info_net(&info->info),
+					 info->info.extack, false);
+	if (ret < 0)
+		goto free_rep_data;
+
+	ctx->ifindex = 0;
+	ctx->pos_ifindex = 0;
+
+	/* For filtered DUMP requests, let's just store the ifindex. We'll check
+	 * again if the netdev is still there when looping over the netdev list
+	 * in the DUMP loop.
+	 */
+	if (req_info->base.dev) {
+		ctx->ifindex = req_info->base.dev->ifindex;
+		netdev_put(req_info->base.dev, &req_info->base.dev_tracker);
+		req_info->base.dev = NULL;
+	}
+
+	ctx->req_info = req_info;
+	ctx->reply_data = reply_data;
+
+	return 0;
+
+free_rep_data:
+	kfree(reply_data);
+free_req_info:
+	kfree(req_info);
+
+	return ret;
+}
+
+static int port_dump_one(struct sk_buff *skb, struct net_device *dev,
+			 struct netlink_callback *cb)
+{
+	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
+	void *ehdr;
+	int ret;
+
+	ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_PORT_GET_REPLY);
+	if (!ehdr)
+		return -EMSGSIZE;
+
+	memset(ctx->reply_data, 0, sizeof(struct port_reply_data));
+	ctx->reply_data->base.dev = dev;
+
+	rtnl_lock();
+	netdev_lock_ops(dev);
+
+	ret = port_prepare_data(&ctx->req_info->base, &ctx->reply_data->base,
+				genl_info_dump(cb));
+
+	netdev_unlock_ops(dev);
+	rtnl_unlock();
+
+	if (ret < 0)
+		goto out;
+
+	ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_PORT_HEADER);
+	if (ret < 0)
+		goto out;
+
+	ret = port_fill_reply(skb, &ctx->req_info->base, &ctx->reply_data->base);
+
+out:
+	ctx->reply_data->base.dev = NULL;
+	if (ret < 0)
+		genlmsg_cancel(skb, ehdr);
+	else
+		genlmsg_end(skb, ehdr);
+
+	return ret;
+}
+
+static int port_dump_one_dev(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
+	struct net_device *dev;
+	struct phy_port *port;
+	int ret;
+
+	dev = ctx->req_info->base.dev;
+
+	if (!dev->link_topo)
+		return 0;
+
+	xa_for_each_start(&dev->link_topo->ports, ctx->pos_portid, port,
+			  ctx->pos_portid) {
+		ctx->req_info->port_id = ctx->pos_portid;
+
+		ret = port_dump_one(skb, dev, cb);
+		if (ret == -ENODEV)
+			continue;
+		if (ret)
+			return ret;
+	}
+
+	ctx->pos_portid = 0;
+
+	return 0;
+}
+
+static int port_dump_all_dev(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
+	struct net *net = sock_net(skb->sk);
+	netdevice_tracker dev_tracker;
+	struct net_device *dev;
+	int ret = 0;
+
+	rcu_read_lock();
+	for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
+		netdev_hold(dev, &dev_tracker, GFP_ATOMIC);
+		rcu_read_unlock();
+
+		ctx->req_info->base.dev = dev;
+		ret = port_dump_one_dev(skb, cb);
+
+		rcu_read_lock();
+		netdev_put(dev, &dev_tracker);
+		ctx->req_info->base.dev = NULL;
+
+		if (ret < 0 && ret != -EOPNOTSUPP) {
+			if (likely(skb->len))
+				ret = skb->len;
+			break;
+		}
+
+		ret = 0;
+	}
+	rcu_read_unlock();
+
+	return ret;
+}
+
+int ethnl_port_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
+	int ret = 0;
+
+	if (ctx->ifindex) {
+		netdevice_tracker dev_tracker;
+		struct net_device *dev;
+
+		dev = netdev_get_by_index(genl_info_net(&info->info),
+					  ctx->ifindex, &dev_tracker,
+					  GFP_KERNEL);
+		if (!dev)
+			return -ENODEV;
+
+		ctx->req_info->base.dev = dev;
+		ret = port_dump_one_dev(skb, cb);
+		if (ret < 0 && ret != -EOPNOTSUPP && likely(skb->len))
+			ret = skb->len;
+
+		netdev_put(dev, &dev_tracker);
+	} else {
+		ret = port_dump_all_dev(skb, cb);
+	}
+
+	return ret;
+}
+
+int ethnl_port_dump_done(struct netlink_callback *cb)
+{
+	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
+
+	kfree(ctx->req_info);
+	kfree(ctx->reply_data);
+
+	return 0;
+}
+
+const struct ethnl_request_ops ethnl_port_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PORT_GET,
+	.reply_cmd		= ETHTOOL_MSG_PORT_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_PORT_HEADER,
+	.req_info_size		= sizeof(struct port_req_info),
+	.reply_data_size	= sizeof(struct port_reply_data),
+
+	.parse_request		= port_parse_request,
+	.prepare_data		= port_prepare_data,
+	.reply_size		= port_reply_size,
+	.fill_reply		= port_fill_reply,
+};
-- 
2.55.0

[PATCH RESEND net-next v17 10/10] Documentation: networking: Update the phy_port infrastructure description

[Maxime Chevallier]

With SFP now properly supported with phy_port, add some details in the
documentation. Fix a typo along the way (driver -> driven).

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/networking/phy-port.rst | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/phy-port.rst b/Documentation/networking/phy-port.rst
index 6e28d9094bce..2a9b1ec93b70 100644
--- a/Documentation/networking/phy-port.rst
+++ b/Documentation/networking/phy-port.rst
@@ -99,13 +99,29 @@ will eventually be able to report its own ksettings::
             (_____)-----| Port |
                         +------+
 
+SFP ports
+=========
+
+SFP interfaces involve 2 distinct components, each represented by
+a :c:type:`struct phy_port <phy_port>` instance :
+
+ - The SFP cage itself is a :c:type:`struct phy_port <phy_port>`. It's special
+   in that it's not an MDI interface, but rather a hot-pluggable MII.
+   The :c:type:`struct phy_port <phy_port>` associated to it lists the different
+   MII interfaces we can use on the cage.
+
+ - The SFP module, when inserted, will also be associated to a
+   :c:type:`struct phy_port <phy_port>`, that represents the various linkmodes
+   that it gives access to. The module's :c:type:`struct phy_port <phy_port>`
+   doesn't supersede the cage's port, it references it through
+   the :c:type:`struct phy_port <phy_port>` :c:member:`upstream_port` field.
+
 Next steps
 ==========
 
-As of writing this documentation, only ports controlled by PHY devices are
-supported. The next steps will be to add the Netlink API to expose these
-to userspace and add support for raw ports (controlled by some firmware, and directly
-managed by the NIC driver).
+As of writing this documentation, the port's presence and information can only
+be queried, and it's not possible to change any of the port's settings or select
+which one should be used.
 
 Another parallel task is the introduction of a MII muxing framework to allow the
-control of non-PHY driver multi-port setups.
+control of non-PHY driven multi-port setups.
-- 
2.55.0

Re: [PATCH RESEND net-next v17 01/10] net: phy: phy_link_topology: Add a helper for opportunistic alloc

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] The comment added to phy_link_topo_get_or_alloc()
  (drivers/net/phy/phy_link_topology.c:38-51) justifies the lock-free…

--

net: phy: phy_link_topology: Add a helper for opportunistic alloc

This moves the opportunistic allocation of struct phy_link_topology out of
phy_link_topo_add_phy() into a new helper, phy_link_topo_get_or_alloc(),
which returns the existing topology or allocates one, reporting failures
via ERR_PTR().

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 4134de7ae313d..0462283c80202 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -28,11 +28,39 @@ static int netdev_alloc_phy_link_topology(struct net_device *dev)
>  	return 0;
>  }
>  
> +static struct phy_link_topology *phy_link_topo_get_or_alloc(struct net_device *dev)
> +{
> +	int ret;
> +
> +	if (dev->link_topo)
> +		return dev->link_topo;
> +
> +	/* The topology is allocated the first time we add an object to it.
> +	 * It is freed alongside the netdev. It can be called on multiple
> +	 * contexts:
> +	 *  - It can be called from .probe() : No rtnl, no netdev_lock
> +	 *  - .ndo_open() : rtnl and possibly netdev_lock
> +	 *  - SFP state machine : rtnl held or not
> +	 *
> +	 *  However, we can't really have races :
> +	 *  - If we have a PHY, phy_link_topo_add_phy() will always run first
> +	 *    and trigger the alloc. Only then the ports can be added through
> +	 *    phylib or sfp.
> +	 *  - If we don't, the SFP port for the cage is registered first, and
> +	 *    only then other ports/PHYs can be registered.
> +	 */

[Severity: Low]
This isn't a bug, but the new comment in phy_link_topo_get_or_alloc()
reasons about objects that don't exist yet at this commit.

At this point struct phy_link_topology only tracks PHYs:

include/linux/phy_link_topology.h:
	struct phy_link_topology {
		struct xarray phys;
		u32 next_phy_index;
	};

and there is no phy_link_topo_add_port(), so "Only then the ports can be
added through phylib or sfp" and "the SFP port for the cage is registered
first" have no counterpart in the tree at this commit.

Would it read better to move that part of the comment to the follow-up
patch that adds the ports xarray, next_port_index and
phy_link_topo_add_port()/phy_link_topo_del_port()?

For the record, this resolves itself later in the series: the next patch,
"net: phy: phy_link_topology: Track ports in phy_link_topology", adds the
ports xarray and the port add/del helpers that call
phy_link_topo_get_or_alloc(), so by the end of the series the comment
matches the code.

> +	ret = netdev_alloc_phy_link_topology(dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return dev->link_topo;
> +}

[ ... ]

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

Re: [PATCH RESEND net-next v17 02/10] net: phy: phy_link_topology: Track ports in phy_link_topology

[netdev-bot+sashiko]

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

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

- [High] `phy_link_topo_del_port()`
  (drivers/net/phy/phy_link_topology.c:84-93) erases `topo->ports` by…
- [High] phy_port objects belonging to an SFP-module PHY are published
  into the netdev's new `topo->ports` xarray by…
- [Medium] Half-wired port registration: `phy_del_port()`
  (drivers/net/phy/phy_device.c:1666) now calls…
- [Medium] The patch exports two new mutators of the per-netdev topology,
  `phy_link_topo_add_port()` / `phy_link_topo_del_port()`…
- [Low] `Documentation/networking/phy-link-topology.rst` and the header-
  level description in include/linux/phy_link_topology.h still describe…

Pre-existing issues:
- [Low] The `struct phy_port` kernel-doc block edited by this patch
  (include/linux/phy_port.h:41) documents '@phy: If the parent is…

--

net: phy: phy_link_topology: Track ports in phy_link_topology

This adds a second xarray to struct phy_link_topology so that the
phy_port objects of a netdev are tracked alongside the PHYs, with a new
per-netdev port id allocated cyclically. Ports are bulk-registered when a
PHY is added to the topology, and removed when the PHY or the port goes
away.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 41d1b20cde2c0..2d0fa3c7f18eb 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1670,6 +1670,9 @@ static void phy_del_port(struct phy_device *phydev, struct phy_port *port)
>  
>  	list_del(&port->head);
>  
> +	if (phydev->attached_dev)
> +		phy_link_topo_del_port(phydev->attached_dev, port);
> +
>  	phydev->n_ports--;
>  }

[Severity: Medium]
This adds the topology removal side in phy_del_port(), but is there a
matching add anywhere?  phy_add_port() is the only place a phy_port gets
linked into phydev->ports, and it does not call phy_link_topo_add_port().
Ports only ever get inserted by the bulk loop in
phy_link_topo_add_phy(), which is a one-shot snapshot taken at attach
time.

Can this be triggered by unbinding and re-binding the PHY driver while the
PHY stays attached to the netdev?  phydev->attached_dev is not cleared by
a driver unbind:

unbind:
  phy_remove() -> phy_cleanup_ports() -> phy_del_port()
      -> phy_link_topo_del_port()   /* old ports erased, fine */

rebind:
  phy_probe() -> phy_setup_ports() -> phy_add_port()
      /* new ports never inserted into dev->link_topo->ports */

After that the PHY's ports are absent from the netdev topology, and the
later phy_detach() runs the new loop in phy_link_topo_del_phy() with
port->id == 0, so xa_erase(&topo->ports, 0) is called on an
XA_FLAGS_ALLOC1 array.

[Severity: Medium]
What is the locking contract for the new call here?  It is gated only on a
plain read of phydev->attached_dev, with no ASSERT_RTNL(), while
phy_link_topo_del_port() reads dev->link_topo with a plain load and then
takes topo->ports.xa_lock via xa_erase().

phy_link_topo_add_phy() at least has:

	if (WARN_ON_ONCE(netdev_need_ops_lock(dev)))
		return -EOPNOTSUPP;

but the two new port helpers have no equivalent, and they enter
phy_link_topo_get_or_alloc(), whose own comment says it can be called
"from .probe() : No rtnl, no netdev_lock" and from the "SFP state machine
: rtnl held or not".  Since netdev_alloc_phy_link_topology() does:

	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
	topo->next_port_index = 1;

	dev->link_topo = topo;

can two concurrent first-adders both kzalloc, leaking one topology and
splitting phys/ports across two objects?  And can a reader observe the
plain store to dev->link_topo before the xa_init_flags() above, so that
xa_erase() takes a not-yet-initialized spinlock?

Later in the series phylink_destroy_sfp_cage_port() calls
phy_link_topo_del_port() from phylink_destroy(), which is documented as
"the rtnl lock must not be held when calling this function", immediately
followed by phy_port_destroy() (kfree), while the reader side added later
(phy_link_topo_get_port()) contains ASSERT_RTNL().  Are those two
contracts compatible?

Note the ops-lock part is addressed later in the series by
"net: phylink: Register a phy_port for MAC-driven SFP cages", which adds
WARN_ON_ONCE(netdev_need_ops_lock(dev)) to phy_link_topo_add_port(); the
allocation race, the missing publication barrier and the unlocked
del_port remain at the end of the series.

[Severity: High]
For an SFP-module PHY, is there any path that removes its ports from the
topology?  The module PHY's own attached_dev is always NULL (PHY-upstream
SFP never calls phy_attach_direct() for it), so this new hook is skipped,
yet its ports were inserted using the host PHY's netdev through
phy_sfp_connect_phy() -> phy_link_topo_add_phy().

The sequence looks like:

  ip link set eth0 up
      host PHY attach, module PHY connect
      -> module ports inserted into dev->link_topo->ports

  ip link set eth0 down
      phy_disconnect() -> phy_detach_internal():
          phydev->attached_dev = NULL;
          phy_link_topo_del_phy(dev, phydev);   /* host PHY only */

  unplug SFP module
      sfp_sm_phy_detach() -> sfp_remove_phy() -> phy_sfp_disconnect_phy():
          struct net_device *dev = phydev->attached_dev;

          if (dev)
                  phy_link_topo_del_phy(dev, phy);   /* dev == NULL, skipped */

      then phy_device_remove()/phy_device_free() -> phy_remove()
          -> phy_cleanup_ports() -> phy_del_port()  /* attached_dev NULL, skip */
          -> phy_port_destroy() kfree()s the ports

Does the live netdev's topo->ports then hold freed phy_port pointers
until the netdev itself is freed?  xa_destroy() only frees the nodes, not
the entries.

At the end of the series "net: ethtool: Introduce ethtool command to list
ports" walks these entries from an unprivileged dump:

	xa_for_each_start(&dev->link_topo->ports, ctx->pos_portid, port, ctx->pos_portid)
	...
	linkmode_copy(reply->supported, port->supported);
	reply->upstream_port = port->upstream_port ? port->upstream_port->id : 0;

Even at this commit the stale slots are observable, since the id re-use
path in phy_link_topo_add_port() does xa_insert() on the stale index and
fails a later add with -EBUSY.

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 0462283c80202..28c5bf3a8849a 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c

[ ... ]

> @@ -56,12 +60,45 @@ static struct phy_link_topology *phy_link_topo_get_or_alloc(struct net_device *d
>  	return dev->link_topo;
>  }
>  
> +int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port)
> +{
> +	struct phy_link_topology *topo;
> +	int ret;
> +
> +	topo = phy_link_topo_get_or_alloc(dev);
> +	if (IS_ERR(topo))
> +		return PTR_ERR(topo);
> +
> +	/* Attempt to re-use a previously allocated port_id */
> +	if (port->id)
> +		ret = xa_insert(&topo->ports, port->id, port, GFP_KERNEL);
> +	else
> +		ret = xa_alloc_cyclic(&topo->ports, &port->id, port,
> +				      xa_limit_32b, &topo->next_port_index,
> +				      GFP_KERNEL);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +EXPORT_SYMBOL_GPL(phy_link_topo_add_port);
> +
> +void phy_link_topo_del_port(struct net_device *dev, struct phy_port *port)
> +{
> +	struct phy_link_topology *topo = dev->link_topo;
> +
> +	if (!topo)
> +		return;
> +
> +	xa_erase(&topo->ports, port->id);
> +}
> +EXPORT_SYMBOL_GPL(phy_link_topo_del_port);

[Severity: High]
Two things about the id handling here.

The add path treats port->id == 0 as "not registered yet", but the del
path erases unconditionally, with no id != 0 guard and no check that the
slot actually holds this port.  port->id is also never cleared on
removal.

If the new port loop in phy_link_topo_add_phy() fails (xa_alloc_cyclic
-ENOMEM), the del_ports rollback leaves the failing port with id 0, then
phy_attach_direct() does "goto error" -> phy_detach_internal() ->
phy_link_topo_del_phy(), whose new loop calls del_port for every port in
the list.  Can that end up doing xa_erase(&topo->ports, 0) and dropping
the XA_ZERO_ENTRY reservation of the XA_FLAGS_ALLOC1 array?

The second case is the cross-netdev one.  port->id is a per-netdev index,
but it survives on the phy_port, so re-using it in a *different*
topology can collide:

	if (port->id)
		ret = xa_insert(&topo->ports, port->id, port, GFP_KERNEL);

If netdev B already has a live port at that id, xa_insert() returns
-EBUSY, phy_link_topo_add_phy() and therefore phy_attach_direct() fail so
the interface cannot come up, and the subsequent
phy_detach_internal() -> phy_link_topo_del_phy() loop erases that id from
B's xarray, removing the other, still-live port's entry.

At the end of the series this is reachable, since
phylink_create_sfp_cage_port() registers the MAC-driven cage port at
phylink/sfp attach time and it takes id 1 in the fresh topology, so a
phy_device persisting on an independent MDIO bus with ports carrying id 1
from a previous netdev collides on xa_insert(1).

Would an ownership check (xa_cmpxchg() or an xa_load() compare), an
id != 0 guard, plus clearing port->id in del_port, and falling back to
fresh allocation when xa_insert() returns -EBUSY, address both cases?
None of these were added through the end of the series.

>  int phy_link_topo_add_phy(struct net_device *dev,
>  			  struct phy_device *phy,
>  			  enum phy_upstream upt, void *upstream)
>  {
>  	struct phy_link_topology *topo;
>  	struct phy_device_node *pdn;
> +	struct phy_port *port;
>  	int ret;
>  
>  	/* ethtool ops may run without rtnl_lock, and rtnl_lock is what
> @@ -110,8 +147,20 @@ int phy_link_topo_add_phy(struct net_device *dev,
>  	if (ret < 0)
>  		goto err;
>  
> +	/* Add all the PHY's ports to the topology */
> +	list_for_each_entry(port, &phy->ports, head) {
> +		ret = phy_link_topo_add_port(dev, port);
> +		if (ret)
> +			goto del_ports;
> +	}
> +
>  	return 0;
>  
> +del_ports:
> +	list_for_each_entry_continue_reverse(port, &phy->ports, head)
> +		phy_link_topo_del_port(dev, port);
> +
> +	xa_erase(&topo->phys, phy->phyindex);
>  err:
>  	kfree(pdn);
>  	return ret;

[Severity: Low]
Documentation/networking/phy-link-topology.rst still describes the
topology as tracking PHY devices only, and its API section documents just
phy_link_topo_add_phy() / phy_link_topo_del_phy() and the phyindex
allocation rules.  Should it be extended for the new ports xarray,
next_port_index and the exported phy_link_topo_add_port() /
phy_link_topo_del_port()?

The same applies to the id semantics: the RST states "The value 0 is
reserved to indicate the PHY doesn't belong to any topology yet", which
now also applies to port ids but is documented only for PHYs.  At the end
of the series the documentation commit only touches phy-port.rst, so
phy-link-topology.rst stays inconsistent with the implementation.

> diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
> index 0ef0f5ce47092..4e2a3fdd2f2e4 100644
> --- a/include/linux/phy_port.h
> +++ b/include/linux/phy_port.h
> @@ -36,6 +36,7 @@ struct phy_port_ops {
>  /**
>   * struct phy_port - A representation of a network device physical interface
>   *
> + * @id: Unique identifier for the port within the topology
>   * @head: Used by the port's parent to list ports
>   * @parent_type: The type of device this port is directly connected to
>   * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port

[Severity: Low]
This isn't a bug introduced by this patch, but while the kernel-doc block
is being touched: the @phy line refers to PHY_PORT_PHYDEV, and the enum
in the same header only defines PHY_PORT_PHY:

	enum phy_port_parent {
		PHY_PORT_PHY,
	};

There is no PHY_PORT_PHYDEV in the tree.

[ ... ]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 2

- [High] phylink now mutates the rtnl-protected PHY link topology from
  contexts that explicitly do not hold rtnl, and frees a topology-…
- [Low] In `phylink_create_sfp_cage_port()`
  (drivers/net/phy/phylink.c:1773-1800) the newly allocated `struct…
- [Low] Concern (invalidated): Documentation/networking/phy-port.rst
  'Next steps' still claims that only PHY-controlled ports are supported…

--

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

This adds a phy_port object representing a MAC-driven SFP cage, allocated
and registered into the netdev's PHY link topology from
phylink_register_sfp(), and removed/freed from phylink_destroy(). The port
is only created for phylink instances that have a netdev.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 390d3eb3e6f7e..bc81ce7f766fa 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1767,6 +1770,46 @@ 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 this port also describe its parent?

phy_port_alloc() uses kzalloc_obj(), and enum phy_port_parent in
include/linux/phy_port.h currently only has PHY_PORT_PHY, which is 0:

	enum phy_port_parent {
		PHY_PORT_PHY,
	};

So this cage port ends up self-describing as "driven by a PHY device" while
port->phy stays NULL. Every other producer sets both fields together, for
example in phy_device.c:

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

and port_phydev() returns port->phy unconditionally, so a consumer keying
off parent_type would resolve this port to a NULL phy_device. Nothing
dereferences it today since this port has ops == NULL and the ethtool
reader added later in the series does not look at parent_type, but would it
make sense to add an enum value for a MAC/phylink-owned port (something
like PHY_PORT_MAC) and set it here, rather than leaving the zero value
meaning "PHY parent"?

> +
> +	phy_interface_and(port->interfaces, pl->config->supported_interfaces,
> +			  phylink_sfp_interfaces);
> +	phy_port_update_supported(port);
> +
> +	ret = phy_link_topo_add_port(pl->netdev, port);
> +	if (ret)
> +		phy_port_destroy(port);
> +	else
> +		pl->sfp_cage_port = port;
> +
> +	return ret;
> +}
> +
> +static void phylink_destroy_sfp_cage_port(struct phylink *pl)
> +{
> +	if (pl->netdev && pl->sfp_cage_port)
> +		phy_link_topo_del_port(pl->netdev, pl->sfp_cage_port);
> +
> +	if (pl->sfp_cage_port)
> +		phy_port_destroy(pl->sfp_cage_port);
> +
> +	pl->sfp_cage_port = NULL;
> +}
> +

[Severity: High]
What serializes this xa_erase() plus kfree() against topology readers?

phy_link_topo_del_port() erases the port from dev->link_topo->ports and
phy_port_destroy() immediately kfree()s it, with no rtnl, no RCU grace
period and no refcount. The topology is otherwise documented as being
rtnl-protected: phy_link_topo_add_phy() states "rtnl_lock is what currently
protects the PHY topology", and the reader added later in this series
carries an ASSERT_RTNL() plus "Caller must hold RTNL while handling the
phy_port".

The free side runs from phylink_destroy(), whose own kernel-doc says the
opposite:

drivers/net/phy/phylink.c:
	/*
	 * Note: the rtnl lock must not be held when calling this function.
	 */
	void phylink_destroy(struct phylink *pl)
	{
		sfp_bus_del_upstream(pl->sfp_bus);
		phylink_destroy_sfp_cage_port(pl);

Can this then race with an ethtool netlink reader on a live netdev?

CPU0 (ETHTOOL_MSG_PORT_GET, rtnl held)
net/ethtool/port.c:port_prepare_data()
	port = phy_link_topo_get_port(req_info->dev, request->port_id);
	...
	linkmode_copy(reply->supported, port->supported);

CPU1 (dpaa2 DPNI "endpoint changed" threaded IRQ, no rtnl)
dpaa2_eth_disconnect_mac() -> dpaa2_mac_disconnect() -> phylink_destroy()
	phylink_destroy_sfp_cage_port()
		phy_link_topo_del_port()	/* xa_erase */
		phy_port_destroy()		/* kfree */

dpaa2_mac_disconnect() drops rtnl before calling phylink_destroy():

drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c:
	rtnl_lock();
	phylink_disconnect_phy(mac->phylink);
	rtnl_unlock();

	phylink_destroy(mac->phylink);

and MAC-driven SFP cages on dpmac nodes do exist in tree, e.g.
fsl-ls1088a-ten64.dts has "&dpmac2 { sfp = <&sfp_xg0>; }", so pl->sfp_bus
is non-NULL and the cage port really is created and destroyed on that
runtime connect/disconnect path.

There is a second, related question on the add side: phylink_create() also
runs without rtnl in that driver, and phy_link_topo_get_or_alloc() does an
unlocked check-then-allocate with a plain store of dev->link_topo:

drivers/net/phy/phy_link_topology.c:
	if (dev->link_topo)
		return dev->link_topo;
	...
	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
	topo->next_port_index = 1;

	dev->link_topo = topo;

Is there something that prevents an rtnl-holding reader from observing
dev->link_topo before the xarrays are initialized, given there is no lock
and no release barrier on the publish? The comment in that function argues
races cannot happen based on ordering of PHY versus SFP registration, but
that reasoning does not seem to cover a concurrent reader on an
already-registered netdev.

>  static int phylink_register_sfp(struct phylink *pl,
>  				const struct fwnode_handle *fwnode)
>  {

[ ... ]

> @@ -2069,6 +2121,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);
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>

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

Re: [PATCH RESEND net-next v17 05/10] net: phy: Represent PHY-less SFP modules with phy_port

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 2 · Low: 1

- [High] FALSE POSITIVE (resolved later in the same series).
- [High] Bitfield data race on `has_sfp_mod_phy` can silently corrupt
  hardware interrupt flags
- [Medium] Failure to allocate/register the purely informational SFP-
  module phy_port now makes the whole SFP module bring-up fail…
- [Medium] Unbalanced registration teardown for `phydev->mod_port`:
  `phy_detach_internal()` unconditionally calls…
- [Low] FALSE POSITIVE.

--

net: phy: Represent PHY-less SFP modules with phy_port

This adds a phy_port to represent a PHY-less SFP module, created and
destroyed from the new .module_start / .module_stop upstream ops in both
phy_device.c and phylink.c, plus a phy_caps helper to filter the module
link modes by the cage port's MII interfaces.

A couple of questions on the new error paths below.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index f15c0fc339b4d..26634ef274f33 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1617,6 +1629,75 @@ static void phy_sfp_link_down(void *upstream)
>  		port->ops->link_down(port);
>  }
>  
> +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;
> +

[ ... ]

> +	if (phydev->attached_dev) {
> +		ret = phy_link_topo_add_port(phydev->attached_dev, port);
> +		if (ret) {
> +			phy_port_destroy(port);
> +			return ret;
> +		}
> +	}

[ ... ]

> +static int phy_sfp_module_start(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +
> +	/* If there's a downstream SFP module, and it doesn't contain a PHY
> +	 * device, let's create a phy_port to represent that module.
> +	 */
> +	if (!phydev->has_sfp_mod_phy)
> +		return phy_add_sfp_mod_port(phydev);
> +
> +	return 0;
> +}

[Severity: Medium]
Should a failure to create this port really abort the whole module
bring-up?

Both new .module_start implementations propagate the error: here
phy_sfp_module_start() returns phy_add_sfp_mod_port()'s -ENOMEM from
phy_port_alloc(), or whatever phy_link_topo_add_port() returns
(xa_alloc_cyclic() -ENOMEM, or -EOPNOTSUPP after the WARN_ON_ONCE for
netdev_need_ops_lock() devices), and phylink_sfp_module_start() does the
same in its new else branch:

	} else {
		ret = phylink_add_sfp_mod_port(pl);
		if (ret)
			return ret;
	}

On the SFP core side, any non-zero return is fatal, in sfp_sm_main():

	case SFP_S_INIT_PHY:
		...
		if (sfp_module_start(sfp->sfp_bus)) {
			sfp_sm_next(sfp, SFP_S_FAIL, 0);
			break;
		}

There is no case SFP_S_FAIL: in that switch, so the only way out is the
global handling at the top of the function, i.e. losing SFP_MOD_PRESENT
or clearing SFP_DEV_UP.

So does a transient kzalloc() failure for this port leave the link down
until the interface is taken down and up again, or the module is
physically re-inserted? Given that the module port is a purely
informational topology object with no data-path role, would it be
preferable to warn and continue rather than to fail the state machine?

The commit message describes only the representation of the module as a
phy_port and doesn't mention this new failure mode.

> @@ -1834,6 +1920,8 @@ static void phy_detach_internal(struct phy_device *phydev, bool notify_bus)
>  		phydev->attached_dev->phydev = NULL;
>  		phydev->attached_dev = NULL;
>  		phy_link_topo_del_phy(dev, phydev);
> +		if (phydev->mod_port)
> +			phy_link_topo_del_port(dev, phydev->mod_port);
>  	}
>  
>  	phydev->phy_link_change = NULL;
> @@ -1963,6 +2051,12 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  		err = phy_link_topo_add_phy(dev, phydev, PHY_UPSTREAM_MAC, dev);
>  		if (err)
>  			goto error;
> +
> +		if (phydev->mod_port) {
> +			err = phy_link_topo_add_port(dev, phydev->mod_port);
> +			if (err)
> +				goto error;
> +		}
>  	}

[Severity: Medium]
Can this pair of add/del calls unregister a port that belongs to someone
else?

The del side in phy_detach_internal() runs whenever phydev->mod_port is
non-NULL, with no record of whether the port was inserted into this
netdev's topology during this attach. And the del is by id only, without
checking the stored entry:

drivers/net/phy/phy_link_topology.c:phy_link_topo_del_port() {
	struct phy_link_topology *topo = dev->link_topo;

	if (!topo)
		return;

	xa_erase(&topo->ports, port->id);
}

Since a phy_port keeps its id across detach so it can be re-used,
phy_link_topo_add_port() re-inserts with xa_insert():

	if (port->id)
		ret = xa_insert(&topo->ports, port->id, port, GFP_KERNEL);
	else
		ret = xa_alloc_cyclic(&topo->ports, &port->id, port,
				      xa_limit_32b, &topo->next_port_index,
				      GFP_KERNEL);

Two sequences look problematic:

phy_link_topo_add_phy() fails (pdn allocation, -EBUSY on a colliding
index, or -EOPNOTSUPP for an ops-locked netdev)

	goto error;
	  phy_detach_internal(phydev, false)
	    phy_link_topo_del_port(dev, phydev->mod_port)
	      xa_erase(&topo->ports, port->id)

and the module port was never inserted into this topology, so whatever
occupies that index is erased.

The second one is the new call itself: xa_insert() returns -EBUSY when
mod_port's retained id is already taken by another port of that netdev,
and the cleanup then erases exactly that foreign entry.

Would it make sense to only call phy_link_topo_del_port() for a port that
was successfully added here (a flag, or clearing/zeroing the id), and to
have phy_link_topo_del_port() verify the stored pointer before erasing?

This needs mod_port to survive a previous detach, which looks possible
since neither phy_detach() nor phy_detach_internal() stop the SFP state
machine or clear mod_port - only .module_stop does.

[ ... ]

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -724,6 +727,7 @@ struct phy_device {
>  	unsigned irq_rerun:1;
>  
>  	unsigned default_timestamp:1;
> +	unsigned has_sfp_mod_phy:1;
>  
>  	int rate_matching;
>  

[Severity: High]
Which lock protects the word this new bit lives in?

has_sfp_mod_phy is added right after default_timestamp, so it lands
in the same machine word as the interrupt bookkeeping bits
interrupts:1, irq_suspended:1 and irq_rerun:1. A C bitfield
assignment is not atomic with respect to its neighbours: the compiler
is free to emit a load of the enclosing word, an OR/AND of one bit,
and a store back. Any concurrent update of a different bit in that
same word is then lost.

The writers of has_sfp_mod_phy are phy_sfp_connect_phy() and
phy_sfp_disconnect_phy(), which run from the SFP state machine work
under rtnl_lock() but without phydev->lock and with interrupts
enabled. irq_rerun (and irq_suspended) are written from
phy_interrupt() and from the mdio_bus_phy_suspend()/_resume() PM
paths, neither of which take rtnl. So a store to has_sfp_mod_phy
that races with phy_interrupt() setting irq_rerun = 1 can drop the
irq_rerun update, and the pending wakeup interrupt is then never
re-run after resume - a silently missed PHY interrupt / broken WoL
wakeup rather than an obvious failure.

The existing rtnl-protected SFP state in this struct is a plain bool
(sfp_bus_attached, with the "This may be modified under the rtnl
lock" comment above it), and the new sfp_cage_port / mod_port
pointers you add sit in that same group. Would it be cleaner to make
has_sfp_mod_phy a bool next to sfp_bus_attached, so it is covered by
the same rule as the rest of the SFP fields and shares no word with
the IRQ flags? If you would rather keep it a bitfield, which lock is
intended to serialise the enclosing word, and how is that
synchronised against phy_interrupt()?

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

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

[netdev-bot+sashiko]

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

Re: [PATCH RESEND net-next v17 07/10] net: phy: phy_link_topology: Add a helper to retrieve ports

[netdev-bot+sashiko]

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

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

- [High] NULL pointer dereference newly introduced in
  phy_link_topo_add_port(): the added `if…
- [High] phy_link_topo_get_port() hands out a raw, unreferenced `struct
  phy_port *` and documents RTNL as the sole lifetime guarantee ("Caller…
- [Medium] The ops-locked guard added to phy_link_topo_add_port() is
  sampled too early to establish the invariant it exists for.
- [Medium] The commit message ("net: phy: phy_link_topology: Add a helper
  to retrieve ports") describes only the addition of the read helper,…
- [Low] Reported for completeness, assessed as not currently a bug:
  phy_link_topo_add_port() turns the ops-locked case into a hard failure…

Pre-existing issues:
- [Medium] Reported for completeness, assessed as a pre-existing pattern
  with unproven reachability: phy_link_topo_get_port() loads…

--

net: phy: phy_link_topology: Add a helper to retrieve ports

This adds phy_link_topo_get_port(), an inline helper that looks up a
phy_port in a netdev's link topology xarray under rtnl, and adds an
ops-locked guard to phy_link_topo_add_port().

> In order to allow netlink access to phy_ports, let's add a helper to
> retrieve them. When handling a port coming from phy_link_topology, the
> caller must hold rtnl until it's done with it.

[Severity: Medium]
The message describes only the new read helper, but the patch also changes
the behaviour of the existing exported write path.  Should the message
mention that phy_link_topo_add_port() now refuses registration with
-EOPNOTSUPP (plus a WARN splat) for ops-locked devices?

All callers turn that into a hard failure:

  phylink_create_sfp_cage_port() -> phylink_register_sfp() -> phylink_create()
  phylink_add_sfp_mod_port()   (SFP module insertion)
  phy_add_sfp_mod_port() / phy_attach_direct()  (PHY attach)

The new getter has no callers in this patch, so the add_port() hunk is the
only behaviour-affecting change here, which makes it easy to misread the
patch as an inert helper addition.

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> index 28c5bf3a8849a..22885bf2e0455 100644
> --- a/drivers/net/phy/phy_link_topology.c
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -65,6 +65,14 @@ int phy_link_topo_add_port(struct net_device *dev, struct phy_port *port)
>  	struct phy_link_topology *topo;
>  	int ret;
>  
> +	/* Ports can now be queried without rtnl for ops-locked devices, which
> +	 * we don't support now as port retrieval is done under rtnl.
> +	 * We don't have phy_port enabled devices yet, let's make sure
> +	 * we are loudly warned about that when it happens.
> +	 */
> +	if (WARN_ON_ONCE(netdev_need_ops_lock(dev)))
> +		return -EOPNOTSUPP;
> +

[Severity: High]
Can this dereference a NULL dev->netdev_ops during probe?  With
CONFIG_NET_SHAPER=y, netdev_need_ops_lock() in include/net/netdev_lock.h
does:

	bool ret = dev->request_ops_lock || !!dev->queue_mgmt_ops;

  #if IS_ENABLED(CONFIG_NET_SHAPER)
	ret |= !!dev->netdev_ops->net_shaper_ops;
  #endif

phy_link_topo_add_port() is reachable from probe via

  phylink_create() -> phylink_register_sfp() -> phylink_create_sfp_cage_port()
  -> phy_link_topo_add_port()

and some drivers call phylink_create() before installing netdev_ops.  In
mtk_add_mac() the phylink_create() call comes first and
"eth->netdev[id]->netdev_ops = &mtk_netdev_ops;" is only executed further
down; the netdev comes from alloc_etherdev_mqs() and net/core does not
install a default netdev_ops, so dev->netdev_ops is still NULL at the time
of the new check.  MediaTek boards with "sfp = <&sfpN>" on the eth-mac node
exist in tree (mt7986a-bananapi-bpi-r3.dts, mt7988a-bananapi-bpi-r4.dts),
so pl->sfp_bus is non-NULL and the cage port path is taken.

Note the identical pre-existing check in phy_link_topo_add_phy() is only
reached from phy_attach_direct(), i.e. after netdev_ops is set, so this is
new exposure for the port path.

[Severity: Medium]
Related to the above ordering: is this guard sampled early enough to be
meaningful for the SFP cage port?

For the cage port the check runs inside phylink_create(), before drivers
populate the very fields netdev_need_ops_lock() inspects
(dev->request_ops_lock, dev->queue_mgmt_ops,
dev->netdev_ops->net_shaper_ops).  mtk_add_mac() again shows the ordering:
phylink_create() first, netdev_ops assignment afterwards, with the netdev
freshly zeroed by alloc_etherdev_mqs().

A driver that becomes ops-locked after phylink_create() would therefore
register ports with no warning at all, which leaves exactly the
rtnl-versus-netdev_lock combination that the new
phy_link_topo_get_port() ASSERT_RTNL() assumes has been excluded.  Would
checking at a later point, or asserting in phy_link_topo_get_port()'s
caller path, catch that case?

[Severity: Low]
This isn't a bug today, but is the hard -EOPNOTSUPP the intended
granularity?  Every caller propagates it as fatal (probe failure via
phylink_create(), or a failed SFP module insertion / PHY attach), whereas
simply declining to register the port (return 0, leave it out of the
xarray) would be enough for an rtnl-only getter.

For what it is worth, no in-tree ops-locked driver hits this yet: among the
drivers with queue_mgmt_ops / request_ops_lock / net_shaper_ops only fbnic
uses phylink, and fbnic_phylink_create() passes fwnode == NULL, so
phylink_register_sfp() returns early and the cage port is never created.

>  	topo = phy_link_topo_get_or_alloc(dev);
>  	if (IS_ERR(topo))
>  		return PTR_ERR(topo);
> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> index 296ee514ba463..9585282cae3fc 100644
> --- a/include/linux/phy_link_topology.h
> +++ b/include/linux/phy_link_topology.h

[ ... ]

> @@ -71,6 +72,25 @@ phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
>  	return NULL;
>  }
>  
> +static inline struct phy_port *
> +phy_link_topo_get_port(struct net_device *dev, u32 port_id)
> +{
> +	struct phy_link_topology *topo = dev->link_topo;
> +
> +	if (!topo)
> +		return NULL;
> +

[Severity: Medium]
This is a pre-existing pattern rather than something introduced here
(phy_link_topo_get_phy() reads dev->link_topo the same way, and
netdev_alloc_phy_link_topology() is untouched), but the plain load of
dev->link_topo happens before ASSERT_RTNL(), and the NULL path skips the
assertion entirely.

The publishing side has no release semantics either:

  drivers/net/phy/phy_link_topology.c:netdev_alloc_phy_link_topology() {
	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
	topo->next_port_index = 1;

	dev->link_topo = topo;
  }

and phy_link_topo_get_or_alloc() documents that this can run "from
.probe() : No rtnl, no netdev_lock".  In practice register_netdevice()
takes rtnl after the probe-time allocation, which appears to order the
xarray init against any rtnl-holding reader, so I could not construct a
reachable interleaving.  Would publishing dev->link_topo with release
semantics and asserting rtnl before touching it be worth doing anyway?

> +	/* ports in the topo are RTNL protected, but may be accessed under
> +	 * netdev_lock for ops-locked devices. For now keep them under rtnl
> +	 * protection, as no ops-locked devices have phy_port capabilities
> +	 * yet.
> +	 */
> +	ASSERT_RTNL();
> +
> +	/* Caller must hold RTNL while handling the phy_port */
> +	return xa_load(&topo->ports, port_id);
> +}

[Severity: High]
Is rtnl really sufficient to keep the returned phy_port alive?  The helper
hands out a raw pointer with no reference taken, but the write side of
topo->ports neither takes nor asserts rtnl, and phy_port_destroy() is a
bare kfree() with no kref and no RCU deferral:

  drivers/net/phy/phy_port.c:phy_port_destroy() {
	kfree(port);
  }

phy_link_topo_del_port() has no lock assertion either:

  drivers/net/phy/phy_link_topology.c:phy_link_topo_del_port() {
	struct phy_link_topology *topo = dev->link_topo;

	if (!topo)
		return;

	xa_erase(&topo->ports, port->id);
  }

Two free paths appear to run without rtnl.  On PHY driver unbind:

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

and phylink teardown, whose kerneldoc states the opposite of the getter's
contract:

  drivers/net/phy/phylink.c
   * Note: the rtnl lock must not be held when calling this function.
   */
  void phylink_destroy(struct phylink *pl)

  phylink_destroy_sfp_cage_port() {
	if (pl->netdev && pl->sfp_cage_port)
		phy_link_topo_del_port(pl->netdev, pl->sfp_cage_port);

	if (pl->sfp_cage_port)
		phy_port_destroy(pl->sfp_cage_port);
  }

xa_erase() stops new lookups but gives no grace period, so a reader that
already obtained the pointer under rtnl can dereference freed memory.
Later in this series net/ethtool/port.c does exactly that:

	port = phy_link_topo_get_port(req_info->dev, request->port_id);
	if (!port)
		return -ENODEV;

	linkmode_copy(reply->supported, port->supported);

Would a kref on phy_port (or kfree_rcu, or taking and asserting the same
lock on every add/del/destroy path) close this?  For reference, the SFP
state machine paths do look safe here: drivers/net/phy/sfp.c takes
rtnl_lock() around sfp_sm_event(), so the module-port add/del are
serialized; it is the probe/unbind/destroy paths above that are not.

> +
>  #else
>  static inline int phy_link_topo_add_phy(struct net_device *dev,
>  					struct phy_device *phy,

[ ... ]

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

Re: [PATCH RESEND net-next v17 08/10] netlink: specs: Add ethernet port listing with ethtool

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

- [Medium] ETHTOOL_A_PORT_SUPPORTED_INTERFACES is a bitset whose bit
  indices are the kernel-internal phy_interface_t enum values (struct…
- [Low] ETHTOOL_MSG_PORT_GET / ETHTOOL_MSG_PORT_GET_REPLY, the
  ETHTOOL_A_PORT_* attribute set, the YAML port-get operation and the…
- [Low] Documentation/networking/ethtool-netlink.rst gains a PORT_GET
  section and a request-translation entry, but the document's two…

--

netlink: specs: Add ethernet port listing with ethtool

This adds a new ethtool netlink operation, port-get, to the ethtool YAML
spec, along with the generated uAPI enums for the ETHTOOL_A_PORT_*
attribute set and the ETHTOOL_MSG_PORT_GET / ETHTOOL_MSG_PORT_GET_REPLY
messages. A PORT_GET section is also added to the ethtool-netlink
documentation.

> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index 5dd4d1b5d94bb..35e028c0be296 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml

[ ... ]

> @@ -1905,6 +1910,32 @@ attribute-sets:
>          name: link
>          type: nest
>          nested-attributes: mse-snapshot
> +  -
> +    name: port
> +    attr-cnt-name: --ethtool-a-port-cnt
> +    attributes:
> +      -
> +        name: header
> +        type: nest
> +        nested-attributes: header
> +      -
> +        name: id
> +        type: u32
> +      -
> +        name: supported-modes
> +        type: nest
> +        nested-attributes: bitset
> +      -
> +        name: supported-interfaces
> +        type: nest
> +        nested-attributes: bitset

[Severity: Medium]
How is userspace expected to interpret the bit positions of
supported-interfaces?  The spec declares it as a plain bitset with no enum
attached, and the implementation in the following patch fills it from the
internal phy_interface_t namespace:

net/ethtool/port.c:port_fill_reply() {
	...
		ret = ethnl_put_bitset(skb, ETHTOOL_A_PORT_SUPPORTED_INTERFACES,
				       reply->interfaces, NULL,
				       PHY_INTERFACE_MODE_MAX,
				       phy_interface_names, compact);
	...
}

PHY_INTERFACE_MODE_* lives in include/linux/phy.h and has no counterpart in
include/uapi/, and there is no ETH_SS_* string set for interface names in
net/ethtool/strset.c info_template[] the way ETH_SS_LINK_MODES exists for
link modes.

A client that sets ETHTOOL_FLAG_COMPACT_BITSETS therefore receives raw bit
indices with no names and no way to resolve them.  Would it also mean that a
future insertion or renumbering inside phy_interface_t silently changes the
meaning of bits that have already been released as ABI?

Would adding a uAPI enum (and referencing it from the attribute in the spec)
be preferable here, as is done for port-type?

> +      -
> +        name: type
> +        type: u32
> +        enum: port-type
> +      -
> +        name: upstream-port
> +        type: u32
>  
>  operations:
>    enum-model: directional

[ ... ]

> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index e92abf45faf50..b4326c89b0751 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -2537,6 +2537,39 @@ Within each channel nest, only the metrics supported by the PHY will be present.
>  See ``struct phy_mse_snapshot`` kernel documentation in
>  ``include/linux/phy.h``.
>  
> +PORT_GET
> +========

[Severity: Low]
Should the two message index tables near the top of this document be updated
as well?  The userspace to kernel table still ends with:

  ``ETHTOOL_MSG_MSE_GET``               get MSE diagnostic data
  ===================================== =================================

and the kernel to userspace table still ends with:

  ``ETHTOOL_MSG_MSE_GET_REPLY``            MSE diagnostic data
  ======================================== =================================

so neither ETHTOOL_MSG_PORT_GET nor ETHTOOL_MSG_PORT_GET_REPLY appears in
the lists that enumerate all valid messages, even though both are now
defined in include/uapi/linux/ethtool_netlink_generated.h.  Every other
message, including the recently added MSE_GET pair, is listed there.

> +
> +Retrieve information about the physical connection points of a network device,
> +referred to as "ports". User needs to specify a PORT_ID for the DO operation,
> +in which case the DO request returns information about that specific port.
> +
> +As there can be more than one port, the DUMP operation can be used to list the
> +ports present on a given interface, by passing an interface index or name in
> +the dump request.
> +

[ ... ]

> +  ``ETHTOOL_A_PORT_SUPPORTED_INTERFACES`` bitset  bitset of supported MII
> +                                                  interfaces

Related to the bitset question above: could this row name the namespace the
bits belong to, so that a reader can map a bit index to an interface mode?

[ ... ]

> diff --git a/include/uapi/linux/ethtool_netlink_generated.h b/include/uapi/linux/ethtool_netlink_generated.h
> index 8134baf7860f3..660e0c08a766a 100644
> --- a/include/uapi/linux/ethtool_netlink_generated.h
> +++ b/include/uapi/linux/ethtool_netlink_generated.h

[ ... ]

> @@ -893,6 +913,7 @@ enum {
>  	ETHTOOL_MSG_RSS_CREATE_ACT,
>  	ETHTOOL_MSG_RSS_DELETE_ACT,
>  	ETHTOOL_MSG_MSE_GET,
> +	ETHTOOL_MSG_PORT_GET,
>  
>  	__ETHTOOL_MSG_USER_CNT,
>  	ETHTOOL_MSG_USER_MAX = (__ETHTOOL_MSG_USER_CNT - 1)

[Severity: Low]
At this commit the message ID and the documented operation exist but nothing
in net/ethtool dispatches them: there is no ethnl_port_request_ops, no
ethnl_default_requests[] entry, no ethtool_genl_ops entry and no policy, so
a port-get request built from the spec is rejected.

This is closed by the next patch in the series, "net: ethtool: Introduce
ethtool command to list ports", which adds:

net/ethtool/netlink.c:
	[ETHTOOL_MSG_PORT_GET]		= &ethnl_port_request_ops,
	...
		.cmd	= ETHTOOL_MSG_PORT_GET,
		.doit	= ethnl_default_doit,
		.start	= ethnl_port_dump_start,
		.dumpit	= ethnl_port_dumpit,

so the only residual effect is a one-commit window where the documented
command is not handled.  Is that split intentional, or would folding the
spec, uAPI header and documentation into the implementation patch be
preferred?

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

Re: [PATCH RESEND net-next v17 09/10] net: ethtool: Introduce ethtool command to list ports

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 4

- [Medium] ETHTOOL_A_PORT_SUPPORTED_INTERFACES publishes bit indices that
  are the raw values of the kernel-internal `enum phy_interface_t` (0 ..…
- [Medium] `ctx->pos_portid` in struct port_dump_ctx is a per-device sub-
  cursor but is not bound to `ctx->pos_ifindex`, and it is reset only…
- [Medium] port_dump_one_dev() reads and dereferences dev->link_topo and
  walks the RTNL-protected link_topo->ports xarray with no lock at all:…
- [Low] The commit message describes the new command inaccurately in two
  ways.
- [Low] The YAML spec declares the port-get DUMP request as carrying only
  `header` (Documentation/netlink/specs/ethtool.yaml:2912-2916), but the…
- [Low] Concern (dismissed after verification):
  Documentation/networking/phy-port.rst was said to still claim that…
- [Low] The "List of message types" inventory tables in
  Documentation/networking/ethtool-netlink.rst are not updated: at the…

--

net: ethtool: Introduce ethtool command to list ports

This adds a new ETHTOOL_MSG_PORT_GET netlink command that exposes the
phy_port objects registered in a netdev's link topology, with a custom
dump implementation in the new net/ethtool/port.c.  MDI ports report
supported linkmodes, MII ports report supported PHY interface modes.

A couple of questions on the commit message and on the new dump code
below.

[Severity: Low]
The changelog says:

    "For MDI ports, we report the list of supported linkmodes based on
    what the PHY that drives this port says.  For MII ports, i.e. empty
    SFP cages, we report the MII linkmodes that we can output on this
    port."

Are both halves of that accurate for this series?

For the first half, phylink_add_sfp_mod_port() fills the MDI port's
supported mask from the module, not from a PHY:

	caps = sfp_get_module_caps(pl->sfp_bus);
	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
					pl->sfp_cage_port->interfaces);
	...
	port->upstream_port = pl->sfp_cage_port;

For the second half, the cage's MII port stays registered while a module
is inserted (pl->sfp_cage_port lives from sfp bus attach to detach), and
a PHY-less module registers a second, MDI port pointing back at the cage
via upstream_port.  So one populated SFP connector yields two dump
records, which makes "we can know how many ports are available on a given
interface" ambiguous.  Could the wording be adjusted to describe the
cage port and the module port separately?

[Severity: Low]
Should the message-type inventory tables in
Documentation/networking/ethtool-netlink.rst be extended too?  At the end
of the series the userspace-to-kernel table still ends at
``ETHTOOL_MSG_MSE_GET`` and the kernel-to-userspace table at
``ETHTOOL_MSG_MSE_GET_REPLY``, even though the PORT_GET section and the
ioctl translation table in the same file already mention the new command.

> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 1af395b54330e..c076c07d0a080 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -26,6 +26,8 @@ static u32 ethnl_bcast_seq;
>  			     ETHTOOL_FLAG_OMIT_REPLY)
>  #define ETHTOOL_FLAGS_STATS (ETHTOOL_FLAGS_BASIC | ETHTOOL_FLAG_STATS)
>  
> +char phy_interface_names[PHY_INTERFACE_MODE_MAX][ETH_GSTRING_LEN] __ro_after_init;
> +
>  const struct nla_policy ethnl_header_policy[] = {

[ ... ]

> @@ -1594,10 +1606,23 @@ static struct genl_family ethtool_genl_family __ro_after_init = {
>  
>  /* module setup */
>  
> +static void __init ethnl_phy_names_populate(void)
> +{
> +	const char *name;
> +	int i;
> +
> +	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++) {
> +		name = phy_modes(i);
> +		strscpy(phy_interface_names[i], name, ETH_GSTRING_LEN);
> +	}
> +}

[Severity: Medium]
These names are built at init time from phy_modes() and are not exported
through any ETH_SS_* string set.  enum ethtool_stringset in
include/uapi/linux/ethtool.h ends at ETH_SS_TS_FLAGS, and
net/ethtool/strset.c info_template has no entry for them, so userspace
cannot resolve the bit indices via ETHTOOL_MSG_STRSET_GET the way it can
for ETH_SS_LINK_MODES.

Does that make the raw values of enum phy_interface_t part of the uAPI?
See the port_fill_reply() comment below.

> diff --git a/net/ethtool/port.c b/net/ethtool/port.c
> new file mode 100644
> index 0000000000000..0d6150e5f66a2
> --- /dev/null
> +++ b/net/ethtool/port.c
> @@ -0,0 +1,382 @@

[ ... ]

> +const struct nla_policy ethnl_port_get_policy[ETHTOOL_A_PORT_ID + 1] = {
> +	[ETHTOOL_A_PORT_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_PORT_ID] = NLA_POLICY_MIN(NLA_U32, 1),
> +};

[Severity: Low]
The YAML spec declares the port-get dump request as carrying only
"header", but this single policy is shared by both doit and dumpit, so a
dump request that carries ETHTOOL_A_PORT_ID passes validation.
ethnl_port_dump_start() only looks at tb[ETHTOOL_A_PORT_HEADER], so the
port id is silently ignored and every port is returned.

Would it be better to reject it explicitly, the way
ethnl_rss_dump_start() does for its do-only filter attribute?

	/* Filtering by context not supported */
	if (tb[ETHTOOL_A_RSS_CONTEXT]) {
		NL_SET_BAD_ATTR(info->extack, tb[ETHTOOL_A_RSS_CONTEXT]);
		return -EINVAL;
	}

[ ... ]

> +static int port_fill_reply(struct sk_buff *skb,
> +			   const struct ethnl_req_info *req_info,
> +			   const struct ethnl_reply_data *reply_data)
> +{

[ ... ]

> +	} else {
> +		ret = ethnl_put_bitset(skb, ETHTOOL_A_PORT_SUPPORTED_INTERFACES,
> +				       reply->interfaces, NULL,
> +				       PHY_INTERFACE_MODE_MAX,
> +				       phy_interface_names, compact);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: Medium]
This publishes bit indices that are the raw values of the kernel-internal
enum phy_interface_t, and Documentation/netlink/specs/ethtool.yaml
declares supported-interfaces only as a plain bitset nest with no enum:

        name: supported-interfaces
        type: nest
        nested-attributes: bitset

A client that sets ETHTOOL_FLAG_COMPACT_BITSETS gets only
ETHTOOL_A_BITSET_VALUE, with no way to map bits to names, so it has to
hard-code the enum values.  Does that freeze the ordering of
enum phy_interface_t as ABI?

The enum has been edited in the middle before:
PHY_INTERFACE_MODE_100BASEX sits between TRGMII and 1000BASEX in the
enum while its phy_modes() case is appended near the end, which suggests
it was inserted rather than appended.  Any future insertion would
re-label already-published bits.

Would adding an ETH_SS_* string set for these names (or defining the
values in the YAML spec) avoid that?

[ ... ]

> +static int port_dump_one_dev(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
> +	struct net_device *dev;
> +	struct phy_port *port;
> +	int ret;
> +
> +	dev = ctx->req_info->base.dev;
> +
> +	if (!dev->link_topo)
> +		return 0;
> +
> +	xa_for_each_start(&dev->link_topo->ports, ctx->pos_portid, port,
> +			  ctx->pos_portid) {
> +		ctx->req_info->port_id = ctx->pos_portid;
> +
> +		ret = port_dump_one(skb, dev, cb);
> +		if (ret == -ENODEV)
> +			continue;
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ctx->pos_portid = 0;
> +
> +	return 0;
> +}

[Severity: Medium]
Can ctx->pos_portid be applied to the wrong device?  It is a per-device
sub-cursor but it is not tied to ctx->pos_ifindex, and it is only reset
after the port loop runs to completion.

In port_dump_all_dev(), the -EMSGSIZE break leaves pos_ifindex pointing
at the interrupted device (for_each_netdev_dump() increments only after
the body) and pos_portid non-zero.  No reference is held on that device
between two ->dumpit() invocations, so if it goes away, xa_find() in
for_each_netdev_dump() returns the next higher ifindex, and this loop
resumes a different device's xarray at the stale index.  Port ids start
at 1:

drivers/net/phy/phy_link_topology.c:netdev_alloc_phy_link_topology() {
	...
	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
	topo->next_port_index = 1;
	...
}

so ports 1..N-1 of that device would be dropped from the reply with no
error reported.

The "if (!dev->link_topo) return 0;" early exit also returns before
"ctx->pos_portid = 0;", so a stale cursor survives every topology-less
device visited in the same invocation.  Should the reset happen at the
top of the function, or should pos_portid be cleared whenever
pos_ifindex changes?

[Severity: Medium]
What protects dev->link_topo and the ports xarray during this walk?
port_dump_all_dev() calls rcu_read_unlock() before invoking this
function, the filtered path in ethnl_port_dumpit() holds nothing, and
RTNL is taken only inside the loop body by port_dump_one().

The accessor added in this same series documents the opposite contract:

include/linux/phy_link_topology.h:phy_link_topo_get_port() {
	/* ports in the topo are RTNL protected, ... */
	ASSERT_RTNL();

	/* Caller must hold RTNL while handling the phy_port */
	return xa_load(&topo->ports, port_id);
}

Two things follow from that.  First, dev->link_topo is published with a
plain store after the xarray is initialised:

drivers/net/phy/phy_link_topology.c:netdev_alloc_phy_link_topology() {
	...
	xa_init_flags(&topo->ports, XA_FLAGS_ALLOC1);
	topo->next_port_index = 1;

	dev->link_topo = topo;
	...
}

and read here with a plain load.  Can a first port registration (for
instance phy_attach_direct() or phylink SFP cage creation from ndo_open)
racing with the dump expose a non-NULL topology whose xarray stores are
not yet visible on a weakly-ordered architecture?  At minimum this looks
like a data race KCSAN would flag.

Second, since RTNL is dropped between every port, every netdev and every
recvmsg resumption, ports added or removed mid-dump (for example
phylink_add_sfp_mod_port() / phylink_del_sfp_mod_port() on module
hotplug) can be skipped or repeated.  There is no generation counter and
NLM_F_DUMP_INTR is never set, so userspace cannot detect it.  Would
holding rtnl_lock() across the xa_for_each_start() loop, or snapshotting
the ids under RTNL, be preferable?

For what it is worth, the returned port pointer is never dereferenced
here and port_prepare_data() re-looks-up the id under
rtnl_lock()+netdev_lock_ops(), so a stale id turns into -ENODEV rather
than a use-after-free.  The same lockless pattern also already exists in
ethnl_perphy_dump_one_dev() for link_topo->phys.

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