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

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

[Maxime Chevallier]

Hello everyone,

v19 addresses Jakub's comments on the netlink part, the only missing bit
is the phy_modes() rework as we'd need that even when CONFIG_NET is off
(for OF and generic PHY), so there's no clean place where to put it. It
also addresses the medium/high *ashiko issues.

Thanks to Christophe for reviewing and testing, this is much
appreciated.

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,
 - 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 v19:
 - Add Review/Tested tags (except on specs/docs)
 - grab rtnl when adding the mac-driver sfp cage
 - rework the port registration to avoid overriding a mod port
 - phy interface names are now exposed as strings
 - rework on the port_get/dump following Jakub's comments

Changes in v18:
https://lore.kernel.org/netdev/20260927133619.955236-1-maxime.chevallier@bootlin.com/
 - Add Review/Tested tags
 - add missing rtnl() in phylink when removing the cage port
 - add a missing NULL check on netdev ops
 - Added missing items in the netlink docs

Changes in v17:
https://lore.kernel.org/netdev/20260910170103.1029108-1-maxime.chevallier@bootlin.com/
 - 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  |  36 ++
 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                  | 192 +++++++--
 drivers/net/phy/phy_link_topology.c           | 101 ++++-
 drivers/net/phy/phylink.c                     | 137 ++++++-
 include/linux/phy.h                           |  10 +
 include/linux/phy_link_topology.h             |  43 ++
 include/linux/phy_port.h                      |   5 +
 include/uapi/linux/ethtool.h                  |   2 +
 .../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                            | 385 ++++++++++++++++++
 net/ethtool/strset.c                          |   5 +
 20 files changed, 1046 insertions(+), 38 deletions(-)
 create mode 100644 net/ethtool/port.c

-- 
2.56.0

[PATCH net-next v19 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>
Tested-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Tested-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
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.56.0

[PATCH net-next v19 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>
Tested-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Tested-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
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 bfce8b893644..ab1aa8981c00 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1580,6 +1580,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 8eb9bf1ff9eb..1a09e7869118 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11521,6 +11521,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.56.0

[PATCH net-next v19 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>
Tested-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Tested-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phylink.c | 59 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a7d086cdc9b2..971f8961dcf1 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;
 
@@ -1767,6 +1770,52 @@ static void phylink_fixed_poll(struct timer_list *t)
 
 static const struct sfp_upstream_ops sfp_phylink_ops;
 
+static int phylink_create_sfp_cage_port(struct phylink *pl)
+{
+	struct phy_port *port;
+	int ret = 0;
+
+	if (!pl->netdev || !pl->sfp_bus)
+		return 0;
+
+	port = phy_port_alloc();
+	if (!port)
+		return -ENOMEM;
+
+	port->is_sfp = true;
+	port->is_mii = true;
+	port->active = true;
+
+	phy_interface_and(port->interfaces, pl->config->supported_interfaces,
+			  phylink_sfp_interfaces);
+	phy_port_update_supported(port);
+
+	rtnl_lock();
+	ret = phy_link_topo_add_port(pl->netdev, port);
+	rtnl_unlock();
+	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) {
+		/* The port may still be accessed under RTNL */
+		rtnl_lock();
+		phy_link_topo_del_port(pl->netdev, pl->sfp_cage_port);
+		rtnl_unlock();
+	}
+
+	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)
 {
@@ -1784,9 +1833,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;
 }
 
@@ -2069,6 +2127,7 @@ EXPORT_SYMBOL_GPL(phylink_create);
 void phylink_destroy(struct phylink *pl)
 {
 	sfp_bus_del_upstream(pl->sfp_bus);
+	phylink_destroy_sfp_cage_port(pl);
 	if (pl->link_gpio)
 		gpiod_put(pl->link_gpio);
 
-- 
2.56.0

[PATCH net-next v19 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>
Tested-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Tested-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 62 ++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ab1aa8981c00..7713418a6734 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1586,13 +1586,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;
@@ -1607,10 +1607,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;
 }
 
 /**
@@ -1619,32 +1621,48 @@ static int phy_setup_sfp_port(struct phy_device *phydev)
  */
 static int phy_sfp_probe(struct phy_device *phydev)
 {
+	struct phy_port *port;
 	struct sfp_bus *bus;
-	int ret = 0;
 
-	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)
+	if (bus) {
+		port = phy_setup_sfp_port(phydev);
+		if (IS_ERR(port)) {
+			sfp_bus_put(bus);
 			phydev->sfp_bus = NULL;
+			return PTR_ERR(port);
+		}
 	}
 
-	if (!ret && phydev->sfp_bus) {
-		ret = phy_setup_sfp_port(phydev);
-		if (ret) {
-			sfp_bus_del_upstream(phydev->sfp_bus);
-			phydev->sfp_bus = NULL;
-		}
+	return 0;
+}
+
+static int phy_sfp_add_upstream(struct phy_device *phydev)
+{
+	struct sfp_bus *bus = phydev->sfp_bus;
+	int ret;
+
+	ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
+	if (ret) {
+		sfp_bus_put(bus);
+		phydev->sfp_bus = NULL;
+		return ret;
 	}
 
-	return ret;
+	/* 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 0;
 }
 
 /**
@@ -3551,6 +3569,10 @@ static int phy_setup_ports(struct phy_device *phydev)
 			break;
 		}
 
+	ret = phy_sfp_add_upstream(phydev);
+	if (ret)
+		goto err_sfp;
+
 	return 0;
 
 err_sfp:
-- 
2.56.0

[PATCH net-next v19 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>
Tested-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Tested-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
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 | 103 ++++++++++++++++++++++++++++++++++-
 drivers/net/phy/phylink.c    |  79 +++++++++++++++++++++++++--
 include/linux/phy.h          |   6 ++
 5 files changed, 209 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 7713418a6734..13eb1d40badb 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1400,11 +1400,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;
 }
 
 /**
@@ -1422,6 +1432,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);
 }
@@ -1527,6 +1539,78 @@ static void phy_sfp_link_down(void *upstream)
 		port->ops->link_down(port);
 }
 
+static int phy_add_sfp_mod_port(struct phy_device *phydev)
+{
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+	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_interface_and(interfaces, phydev->sfp_cage_port->interfaces,
+			  caps->interfaces);
+	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
+					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,
@@ -1536,6 +1620,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)
@@ -1640,6 +1726,8 @@ static int phy_sfp_probe(struct phy_device *phydev)
 			phydev->sfp_bus = NULL;
 			return PTR_ERR(port);
 		}
+
+		phydev->sfp_cage_port = port;
 	}
 
 	return 0;
@@ -1654,6 +1742,7 @@ static int phy_sfp_add_upstream(struct phy_device *phydev)
 	if (ret) {
 		sfp_bus_put(bus);
 		phydev->sfp_bus = NULL;
+		phydev->sfp_cage_port = NULL;
 		return ret;
 	}
 
@@ -1744,6 +1833,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;
@@ -1876,6 +1967,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 971f8961dcf1..667f00ddfa5e 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;
 
@@ -1794,10 +1795,15 @@ static int phylink_create_sfp_cage_port(struct phylink *pl)
 	ret = phy_link_topo_add_port(pl->netdev, port);
 	rtnl_unlock();
 	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;
 }
 
@@ -4014,14 +4020,68 @@ static void phylink_sfp_module_remove(void *upstream)
 	phy_interface_zero(pl->sfp_interfaces);
 }
 
+static int phylink_add_sfp_mod_port(struct phylink *pl)
+{
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+	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_interface_and(interfaces, pl->sfp_cage_port->interfaces,
+			  caps->interfaces);
+	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
+					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
@@ -4030,7 +4090,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)
@@ -4040,6 +4109,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 7c5098a0dd6c..dbd6ed5864f2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -611,7 +611,10 @@ struct phy_oatc14_sqi_capability {
  * @irq: IRQ number of the PHY's interrupt (-1 if none)
  * @phylink: Pointer to phylink instance for this PHY
  * @sfp_bus_attached: Flag indicating whether the SFP bus has been attached
+ * @has_sfp_mod_phy: Set true if downstream SFP bus's module contains a PHY
  * @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.
@@ -802,7 +805,10 @@ struct phy_device {
 
 	/* This may be modified under the rtnl lock */
 	bool sfp_bus_attached;
+	bool has_sfp_mod_phy;
 	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.56.0

[PATCH net-next v19 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.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Tested-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 24 ++++++++++++++++++++++++
 drivers/net/phy/phylink.c    |  5 +++++
 include/linux/phy.h          |  4 ++++
 include/linux/phy_port.h     |  3 +++
 4 files changed, 36 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 13eb1d40badb..e9da6767faaf 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1403,6 +1403,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.
@@ -1436,6 +1437,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);
 }
 
 /**
@@ -1574,6 +1577,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;
 }
 
@@ -3698,6 +3703,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 667f00ddfa5e..e885da1c2831 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -4052,6 +4052,8 @@ static int phylink_add_sfp_mod_port(struct phylink *pl)
 		}
 	}
 
+	port->upstream_port = pl->sfp_cage_port;
+
 	pl->mod_port = port;
 
 	return 0;
@@ -4155,6 +4157,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);
 }
@@ -4163,6 +4167,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 dbd6ed5864f2..1fb87d12367b 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.56.0

[PATCH net-next v19 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.

We don't really support PHY and phy_port operation on ops-locked devices
yet, warn when we try to do so on such an interface.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Tested-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_link_topology.c |  8 ++++++++
 include/linux/phy_link_topology.h   | 25 +++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index 28c5bf3a8849..214ccce23cf6 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(dev->netdev_ops && 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..f2e493c433ff 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,24 @@ 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();
+
+	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 +119,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.56.0

[PATCH net-next v19 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.

Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml      | 55 +++++++++++++++++++
 Documentation/networking/ethtool-netlink.rst  | 36 ++++++++++++
 .../uapi/linux/ethtool_netlink_generated.h    | 22 ++++++++
 3 files changed, 113 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index a66594ad9e73..a9cb02538783 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -212,6 +212,11 @@ definitions:
       -
         name: discard
         value: 31
+  -
+    name: port-type
+    doc: type of physical connector
+    type: enum
+    entries: [mdi, sfp]
 
 attribute-sets:
   -
@@ -1907,6 +1912,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
@@ -2861,6 +2892,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 393f2b6d3863..eba226c79ec3 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -247,6 +247,7 @@ Userspace to kernel:
   ``ETHTOOL_MSG_RSS_CREATE_ACT``        create an additional RSS context
   ``ETHTOOL_MSG_RSS_DELETE_ACT``        delete an additional RSS context
   ``ETHTOOL_MSG_MSE_GET``               get MSE diagnostic data
+  ``ETHTOOL_MSG_PORT_GET``              get port information
   ===================================== =================================
 
 Kernel to userspace:
@@ -305,6 +306,7 @@ Kernel to userspace:
   ``ETHTOOL_MSG_RSS_CREATE_NTF``           additional RSS context created
   ``ETHTOOL_MSG_RSS_DELETE_NTF``           additional RSS context deleted
   ``ETHTOOL_MSG_MSE_GET_REPLY``            MSE diagnostic data
+  ``ETHTOOL_MSG_PORT_GET_REPLY``           port information
   ======================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -2537,6 +2539,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 +2682,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.56.0

[PATCH net-next v19 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.

Tested-by: Aleksei Sviridkin <f@lex.la>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Tested-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 MAINTAINERS                  |   1 +
 include/uapi/linux/ethtool.h |   2 +
 net/ethtool/Makefile         |   2 +-
 net/ethtool/netlink.c        |  25 +++
 net/ethtool/netlink.h        |   8 +
 net/ethtool/port.c           | 385 +++++++++++++++++++++++++++++++++++
 net/ethtool/strset.c         |   5 +
 7 files changed, 427 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/port.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 23e9194483d9..fc32d5ef1421 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19022,6 +19022,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/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 986d70caec33..9e60c17b48c8 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -685,6 +685,7 @@ enum ethtool_link_ext_substate_module {
  * @ETH_SS_STATS_RMON: names of RMON statistics
  * @ETH_SS_STATS_PHY: names of PHY(dev) statistics
  * @ETH_SS_TS_FLAGS: hardware timestamping flags
+ * @ETH_SS_PHY_INTERFACES: names of MAC to PHY interfaces
  *
  * @ETH_SS_COUNT: number of defined string sets
  */
@@ -712,6 +713,7 @@ enum ethtool_stringset {
 	ETH_SS_STATS_RMON,
 	ETH_SS_STATS_PHY,
 	ETH_SS_TS_FLAGS,
+	ETH_SS_PHY_INTERFACES,
 
 	/* add new constants above here */
 	ETH_SS_COUNT
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 383e911f50f7..dd8a53f89d54 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -28,6 +28,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,
@@ -433,6 +435,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)
@@ -1571,6 +1574,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[] = {
@@ -1593,10 +1605,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..425e5bbdca45
--- /dev/null
+++ b/net/ethtool/port.c
@@ -0,0 +1,385 @@
+// 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;
+
+	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;
+	}
+
+	port_type = ETHTOOL_PORT_TYPE_MDI;
+	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));
+
+	if (tb[ETHTOOL_A_PORT_ID]) {
+		NL_SET_BAD_ATTR(info->info.extack, tb[ETHTOOL_A_PORT_ID]);
+		return -EINVAL;
+	}
+
+	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;
+
+	ret = port_prepare_data(&ctx->req_info->base, &ctx->reply_data->base,
+				genl_info_dump(cb));
+	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 = 0;
+
+	dev = ctx->req_info->base.dev;
+
+	rtnl_lock();
+	netdev_lock_ops(dev);
+
+	if (!dev->link_topo)
+		goto out;
+
+	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)
+			goto out;
+	}
+
+	ret = 0;
+
+out:
+	netdev_unlock_ops(dev);
+	rtnl_unlock();
+
+	return ret;
+}
+
+static int port_dump_all_dev(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
+	unsigned long start_ifindex = ctx->pos_ifindex;
+	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) {
+		if (ctx->pos_ifindex != start_ifindex)
+			ctx->pos_portid = 0;
+
+		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)
+			break;
+	}
+	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);
+
+		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,
+};
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 94c4718d31ae..0974714c34ec 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -116,6 +116,11 @@ static const struct strset_info info_template[] = {
 		.count		= __ETHTOOL_A_STATS_PHY_CNT,
 		.strings	= stats_phy_names,
 	},
+	[ETH_SS_PHY_INTERFACES] = {
+		.per_dev	= false,
+		.count		= PHY_INTERFACE_MODE_MAX,
+		.strings	= phy_interface_names,
+	},
 };
 
 struct strset_req_info {
-- 
2.56.0

[PATCH net-next v19 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).

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
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.56.0

Re: [PATCH net-next v19 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 4 potential
issue(s) to consider.

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] topo->ports can keep pointers to freed phy_port objects
  belonging to an SFP-module PHY that sits behind a media-converter PHY.
- [Medium] phy_link_topo_del_port() (phy_link_topology.c:84-93) runs
  xa_erase(&topo->ports, port->id) without checking that the slot…
- [Medium] phy_add_port() and phy_del_port() no longer match.
- [Medium] The patch ties two pieces of state together that are protected
  by different locks.

--

net: phy: phy_link_topology: Track ports in phy_link_topology

This commit adds a ports xarray to struct phy_link_topology and gives each
tracked phy_port an id that is unique within the topology. A PHY's ports
are added when the PHY joins the topology in phy_link_topo_add_phy(). They
are removed in phy_link_topo_del_phy() and phy_del_port().

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index bfce8b893644f..ab1aa8981c00c 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1580,6 +1580,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]
phy_del_port() now removes the port from the attached netdev's topology.
phy_add_port() has no matching phy_link_topo_add_port() call, though. It
only does list_add() and n_ports++. Ports reach topo->ports only through
the loop in phy_link_topo_add_phy(), and that loop runs at attach time.

What happens if the PHY driver is unbound and rebound through sysfs while
the PHY is attached?

phy_remove()
  phy_cleanup_ports()
    phy_del_port()
      phy_link_topo_del_port(phydev->attached_dev, port)

phy_remove() does not detach the PHY, so attached_dev stays set. On
rebind, phy_probe()->phy_setup_ports()->phy_add_port() creates new ports
with id 0, and nothing adds them to topo->ports.

Won't the topology be missing this PHY's ports until the PHY is detached
and attached again? When those ports are removed later, the call is
xa_erase(&topo->ports, 0), which does nothing.

> 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);
> +}

[Severity: Medium]
phy_link_topo_del_port() erases whatever is stored at port->id and does
not check that the entry is this port.

port->id is also never cleared when a port leaves a topology.
phy_link_topo_add_port() re-inserts any nonzero id with xa_insert(), no
matter which topology handed that id out. The kernel-doc for @id says
"Unique identifier for the port within the topology".

Suppose a port keeps a stale id, and that id has since been given to
another port in the target topology. xa_insert() then returns -EBUSY,
phy_link_topo_add_phy() fails, and phy_attach_direct() fails. The commit
message doesn't mention this new way for attach to fail.

The error path also appears to clean up twice. phy_link_topo_add_phy()
has already rolled back through del_ports. phy_attach_direct() then does:

	err = phy_link_topo_add_phy(dev, phydev, PHY_UPSTREAM_MAC, dev);
	if (err)
		goto error;

This reaches phy_detach_internal() with attached_dev still set. Its
phy_link_topo_del_phy() call then runs xa_erase() for every port in
phy->ports, including the port that got -EBUSY and any later ports that
were never inserted.

Can this remove the other, still-live port's entry from topo->ports?

The same applies if phy_link_topo_add_phy() fails earlier (pdn
allocation, the ops-lock check, a phyindex collision) while ports still
carry stale ids. Would an xa_cmpxchg() or xa_load() check in
phy_link_topo_del_port() work better here?

[ ... ]

> @@ -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;
> +	}

[Severity: High]
Can this leave pointers to freed phy_port objects in topo->ports when an
SFP module PHY sits behind a media-converter PHY?

phy_sfp_connect_phy() adds the module PHY to the upstream PHY's netdev:

	if (dev)
		return phy_link_topo_add_phy(dev, phy, PHY_UPSTREAM_PHY, phydev);

With this loop, all of the module PHY's ports go into
dev->link_topo->ports. The module PHY is never attached itself, so its
attached_dev stays NULL.

Say the upstream PHY detaches first. For example, mvpp2 and mvneta call
phylink_disconnect_phy() in ndo_stop. Then
phy_detach_internal()->phy_link_topo_del_phy(dev, upstream) removes only
the upstream PHY's own ports. When the module is unplugged later:

sfp_sm_phy_detach()
  sfp_remove_phy()
    phy_sfp_disconnect_phy()   <- upstream attached_dev is NULL, no-op
  phy_device_remove()
    phy_remove()
      phy_cleanup_ports()
        phy_del_port()         <- module PHY attached_dev NULL, skipped
        phy_port_destroy()     <- port freed, still in topo->ports

Unbinding the module PHY's driver while the module is connected hits the
same check in phy_del_port(). The later phy_link_topo_del_phy() then
walks an empty phy->ports list.

Nothing reads these entries yet in this patch. Later in the series,
though, port_dump_one_dev() and port_prepare_data() in net/ethtool/port.c
iterate dev->link_topo->ports. They dereference port->supported,
port->interfaces and port->upstream_port->id.

ETHTOOL_MSG_PORT_GET does not require admin rights. Wouldn't that make
this a use-after-free that unprivileged users can reach? phy_del_port()
and phy_sfp_disconnect_phy() are still unchanged at the end of the
series.

The module PHY's entry in topo->phys already has the same gap. The stale
port entries are new with this patch.

> +
>  	return 0;

[ ... ]

> @@ -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);
> +

[Severity: Medium]
phy_link_topo_add_phy() and phy_link_topo_del_phy() now walk phy->ports
under rtnl.

phy->ports is changed and freed in phy_remove()->phy_cleanup_ports() and
in the phy_probe() error paths. Those paths hold only the PHY's
device_lock. They are reached through sysfs unbind, or through
mdiobus_unregister() on a shared MDIO bus while another MAC's PHY is
still attached.

Can this list_for_each_entry() follow a port that phy_cleanup_ports() on
another CPU has just list_del()'d and freed with phy_port_destroy()?

phy_del_port() also reads phydev->attached_dev with no lock held, then
modifies dev->link_topo->ports without rtnl. The comment in
phy_link_topo_add_phy() says that rtnl_lock is what protects the PHY
topology.

Ports are freed right after they are erased from the xarray, with no
rtnl or RCU in between. Later in the series, port_prepare_data() in
net/ethtool/port.c looks up ports under rtnl, and its comment says "RTNL
must be held while holding a ref to the phy_port".
phylink_destroy_sfp_cage_port() takes rtnl around phy_link_topo_del_port()
for this reason.

Does phy_del_port() need the same protection? The attached_dev it reads
can also go stale if phy_detach() and free_netdev() run at the same
time.

Neither exported helper has an ASSERT_RTNL() or lockdep annotation.

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

Re: [PATCH net-next v19 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

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

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

--

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

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

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

[ ... ]

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

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

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

enum phy_port_parent {
	PHY_PORT_PHY,
};

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

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

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

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

Teardown then follows this chain:

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

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

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

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

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

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

[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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message presents the reorder in
  phy_sfp_probe()/phy_setup_ports() as pure preparation.
- [Low] The patch opens a new window in phy_setup_ports().
- [Low] In phy_setup_ports() (lines 3534-3542), the comment says the `if
  (!phydev->is_genphy_driven)` guard exists because genphy binding runs…

--

net: phy: Create SFP phy_port before registering upstream

This patch splits the SFP setup in phy_sfp_probe(). That function now
looks up the SFP bus and creates the SFP phy_port without registering the
upstream. Registration moves to a new phy_sfp_add_upstream() helper, which
runs at the end of phy_setup_ports(). The stated goal is to let the
phy_port be passed to the SFP bus together with the upstream information.

> 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.

[Severity: Medium]
This describes the reorder as preparation only. Does it also fix a
cold-plug problem that should be described, possibly with a Fixes: tag?

Before this patch, phy_sfp_probe() called sfp_bus_add_upstream() before
phy_setup_sfp_port(). If the SFP socket had already probed and read the
module EEPROM (sm_mod_state == SFP_MOD_WAITDEV), the module insert ran
synchronously:

sfp_bus_add_upstream()
  sfp_register_bus()
    sfp_attach()
      sfp_sm_event(SFP_E_DEV_ATTACH)
        sfp_sm_module()
          sfp_module_insert()
            phy_sfp_module_insert()

The is_sfp port did not exist yet at that point, so
phy_sfp_module_insert() returned early:

	port = phy_get_sfp_port(phydev);
	if (!port)
		return -EINVAL;

sfp_sm_module() then moved the module to SFP_MOD_ERROR:

		err = sfp_module_insert(sfp->sfp_bus, &sfp->id,
					sfp->quirk);
		if (err < 0) {
			sfp_sm_mod_next(sfp, SFP_MOD_ERROR, 0);

SFP_MOD_ERROR is below SFP_MOD_WAITDEV, so no later DEV_UP or attach event
retries the insert. The module stays rejected until it is physically
re-plugged. This can happen on boards where the SFP socket probes before
a media-converter PHY such as 88e1512 or 88x2222.

After this patch the SFP port already exists when phy_sfp_add_upstream()
runs, so the insert succeeds. The old ordering seems to come from
07f3ca9e092cb8 ("net: phy: Create a phy_port for PHY-driven SFPs") and
d7c6082f7e7771 ("net: phy: Introduce generic SFP handling for PHY
drivers"). Without a Fixes: tag, stable trees that contain those commits
would not pick this up.

There is a related behaviour change too. The cold-plug
phy_sfp_module_insert() now runs after phy_setup_ports() has computed
phydev->supported and phydev->port. As a result, this assignment now
persists, while before the cold-plug path never reached it:

	if (phydev->n_ports == 1)
		phydev->port = caps->port;

Could the commit message mention this as well?

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index ab1aa8981c00c..7713418a6734e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1619,32 +1621,48 @@ static int phy_setup_sfp_port(struct phy_device *phydev)
>   */
>  static int phy_sfp_probe(struct phy_device *phydev)
>  {

[ ... ]

> -		ret = sfp_bus_add_upstream(bus, phydev, &sfp_phydev_ops);
> -		sfp_bus_put(bus);
> +	phydev->sfp_bus = bus;
>  
> -		if (ret)
> +	if (bus) {
> +		port = phy_setup_sfp_port(phydev);
> +		if (IS_ERR(port)) {
> +			sfp_bus_put(bus);
>  			phydev->sfp_bus = NULL;
> +			return PTR_ERR(port);
> +		}
>  	}
>  
> -	if (!ret && phydev->sfp_bus) {
> -		ret = phy_setup_sfp_port(phydev);
> -		if (ret) {
> -			sfp_bus_del_upstream(phydev->sfp_bus);
> -			phydev->sfp_bus = NULL;
> -		}
> +	return 0;
> +}

[Severity: Low]
On success, phy_sfp_probe() now returns with phydev->sfp_bus holding only
the sfp_bus_find_fwnode() reference, and no upstream registered yet.

What happens if phy_default_setup_single_port() fails in phy_setup_ports()
before phy_sfp_add_upstream() runs?

phy_setup_ports()
    if (phydev->n_ports < phydev->max_n_ports) {
        ret = phy_default_setup_single_port(phydev);
        if (ret)
            goto err_sfp;
    }
    ...
err_sfp:
    phy_sfp_release(phydev);

phy_sfp_release() then calls sfp_bus_del_upstream() on a bus that was
never passed to sfp_bus_add_upstream(). The documented precondition of
sfp_bus_del_upstream() is "@bus should have been added by
sfp_bus_add_upstream()".

This only works today by coincidence. bus->registered is false, so
sfp_unregister_bus() does nothing. upstream_ops and upstream are already
NULL. The final sfp_bus_put() drops the find_fwnode reference, not the
add_upstream reference it was written to release.

The kernel-doc of phy_sfp_release() is also out of date. It still says it
releases what "a successful phy_sfp_probe()" set up and that it
unregisters the upstream. phy_sfp_probe() no longer registers an upstream;
phy_sfp_add_upstream() does that now.

Could sfp_bus_del_upstream() be called only after the upstream has been
added, with a plain sfp_bus_put() otherwise? The kernel-doc should then
be updated to match.

The end of the series ("Documentation: networking: Update the phy_port
infrastructure description") still has the same err_sfp path and the
same kernel-doc.

> +
> +static int phy_sfp_add_upstream(struct phy_device *phydev)
> +{

[ ... ]

> @@ -3551,6 +3569,10 @@ static int phy_setup_ports(struct phy_device *phydev)
>  			break;
>  		}
>  
> +	ret = phy_sfp_add_upstream(phydev);
> +	if (ret)
> +		goto err_sfp;
> +

[Severity: Low]
Is the comment above the genphy check in phy_setup_ports() still
accurate?

	/* We don't support SFP with genphy drivers. Also, genphy driver
	 * binding occurs with RTNL help, which will deadlock the call to
	 * sfp_bus_add_upstream().
	 */
	if (!phydev->is_genphy_driven) {
		ret = phy_sfp_probe(phydev);
		...
	}

sfp_bus_add_upstream() is no longer inside that guard. It is now reached
through phy_sfp_add_upstream() for every PHY, including genphy-driven
ones.

This is safe today only because phydev->sfp_bus stays NULL for genphy,
and sfp_bus_add_upstream(NULL) returns 0 before taking rtnl_lock().
Nothing near the new call states that dependency.

Could the call go under the same is_genphy_driven check, or could the
comment be updated to describe this? The end of the series has the same
code.

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

Re: [PATCH net-next v19 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: 0 · Medium: 4 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The module port's ->supported mask is computed by the new
  phy_caps_linkmode_filter_ifaces(), and the result does not match the…
- [Medium] phy_detach_internal() always calls phy_link_topo_del_port(dev,
  phydev->mod_port) when mod_port is non-NULL, even on the…
- [Medium] phydev->mod_port and phydev->attached_dev are accessed from
  two contexts that share no lock.
- [Medium] The new topology mutations from the SFP
  module_start/module_stop callbacks run without rtnl and without any…
- [Low] phy_add_sfp_mod_port() (phy_device.c) and
  phylink_add_sfp_mod_port() (phylink.c) leave the module port only…

--

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

This commit creates a phy_port to represent a PHY-less SFP module. The
port is created in the SFP module_start callback and destroyed in
module_stop, for both the phylib (media converter PHY) and phylink
upstreams. It also adds phy_caps_linkmode_filter_ifaces(), which computes
the module port's supported link modes from the cage port's interfaces.

> diff --git a/drivers/net/phy/phy_caps.c b/drivers/net/phy/phy_caps.c
> index 942d43191561e..558e4df4d63c0 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)

[ ... ]

> +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);

[Severity: Medium]
Does this filter always clear Autoneg, Pause and Asym_Pause from the
module port's supported mask?

ifaces_supported is built only from link_caps[].linkmodes.
phy_caps_init() fills that array only for link modes that have a valid
speed and duplex. sfp_module_parse_support() always sets these bits in
caps->link_modes:

drivers/net/phy/sfp-bus.c:sfp_module_parse_support() {
    ...
	phylink_set(modes, Autoneg);
	phylink_set(modes, Pause);
	phylink_set(modes, Asym_Pause);
    ...
}

so linkmode_and() drops them. In contrast, phy_caps_medium_get_supported()
keeps these special bits on purpose ("will be masked away by the port
parent"), and phylink_validate_mask_caps() keeps Autoneg and the port
modes. Is it intended that they are dropped here?

There is a second issue. In phylink_add_sfp_mod_port() the interface
mask comes from pl->sfp_cage_port->interfaces, which is
config->supported_interfaces & phylink_sfp_interfaces:

	phy_interface_and(interfaces, pl->sfp_cage_port->interfaces,
			  caps->interfaces);
	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
					interfaces);

This skips the mac_capabilities/PCS validation that
phylink_sfp_config_optical() applies to pl->sfp_support through
phylink_validate_mask(). For example, phy_caps_from_interface(SGMII)
includes 1000HD. A PHY-less 1000BASE-T module could then report
1000baseT_Half on a MAC without MAC_1000HD.

Can the port list modes that phylink will never configure? Later in the
series this mask is reported to userspace as
ETHTOOL_A_PORT_SUPPORTED_MODES through port_fill_reply().

> +}
> +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 7713418a6734e..13eb1d40badb5 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1527,6 +1539,78 @@ static void phy_sfp_link_down(void *upstream)
>  		port->ops->link_down(port);
>  }
>  
> +static int phy_add_sfp_mod_port(struct phy_device *phydev)
> +{
> +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> +	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;

[Severity: Low]
Is this port initialized enough before it is published to the topology?

Only ->active and ->supported are set here, and the same is true of
phylink_add_sfp_mod_port(). ->mediums and ->pairs stay 0, because
neither phy_caps_mediums_from_linkmodes() nor
phy_port_update_supported() is called, and caps->port from the EEPROM
is not used.

With empty mediums, phy_port_get_type() would return PORT_OTHER for a
fibre or DAC module, and phy_port_is_fiber() and phy_port_is_copper()
would both return false. phy_port_restrict_mediums() documents:

	/* We forbid ending-up with a port with empty mediums */

and phy_default_setup_single_port() does:

	port->mediums = phy_caps_mediums_from_linkmodes(port->supported);

->parent_type is also left at its zero value, PHY_PORT_PHY, while ->phy
stays NULL. port_phydev() therefore returns NULL for a port that claims
a PHY parent. phy_setup_sfp_port() sets:

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

In the phylink case there is no PHY parent at all.

Nothing reads these fields on the mod port yet. net/ethtool/port.c
port_prepare_data() only uses supported, interfaces, id, is_mii, is_sfp
and upstream_port, so this is latent for now.

Should the mediums be derived from the supported modes, and the parent
fields be set consistently?

> +
> +	caps = sfp_get_module_caps(phydev->sfp_bus);
> +
> +	phy_interface_and(interfaces, phydev->sfp_cage_port->interfaces,
> +			  caps->interfaces);
> +	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
> +					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;

[Severity: Medium]
What serializes phydev->mod_port and phydev->attached_dev between this
SFP state machine path and phy_attach_direct()/phy_detach_internal()?

phy_sfp_module_start() and phy_sfp_module_stop() run under sm_mutex, and
under rtnl when called from sfp_timeout() or sfp_check_state().
phy_attach_direct() and phy_detach_internal() do not assert rtnl, and
driver .remove() paths often call phy_disconnect() without it.

phy_disconnect() only calls phy_stop() when phy_is_started() is true.
For a PHY in PHY_ERROR, the SFP upstream therefore stays started and
mod_port is still alive when phy_detach() runs. If the module is pulled
at the same time:

CPU1 (driver .remove)              CPU2 (SFP state machine)
phy_detach_internal()
  if (phydev->mod_port)
                                   sfp_check_state()
                                     phy_sfp_module_stop()
                                       phy_del_sfp_mod_port()
                                         phy_port_destroy(phydev->mod_port);
                                         phydev->mod_port = NULL;
    phy_link_topo_del_port(dev, phydev->mod_port);
      xa_erase(&topo->ports, port->id);

Could this read port->id from the freed port and erase an arbitrary
index?

The add side has a similar window. phy_add_sfp_mod_port() checks
attached_dev and only then publishes mod_port, while phy_attach_direct()
sets attached_dev and then checks mod_port. Depending on the
interleaving, the port is either missing from the topology or added
twice. In the second case xa_insert() returns -EBUSY and the attach
fails.

> +}
> +
> +static int phy_sfp_module_start(void *upstream)
> +{

[ ... ]

> +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);

[Severity: Medium]
Can this erase and free a topology port without rtnl held?

phy_stop() takes only phydev->lock and runs the SFP stop synchronously:

phy_stop()
  sfp_upstream_stop()
    sfp_stop()
      sfp_sm_event(SFP_E_DEV_DOWN)
        sfp_sm_main()
          sfp_module_stop()
            phy_sfp_module_stop()
              phy_del_sfp_mod_port()
                phy_link_topo_del_port()
                phy_port_destroy()

phylib MAC drivers often call phy_stop() from .suspend(), or through
phy_disconnect() in .remove(), without rtnl. phy_link_topo_add_phy()
says "rtnl_lock is what currently protects the PHY topology", and
phy_link_topo_get_or_alloc() describes the SFP state machine as "rtnl
held or not".

phy_port_destroy() is a plain kfree(). An ETHTOOL_MSG_PORT_GET dump
holding rtnl (port_dump_one_dev() walking topo->ports, added later in
the series) could still be using the port when it is freed.

phylink_add_sfp_mod_port() and phylink_del_sfp_mod_port() also call
phy_link_topo_add_port() and phy_link_topo_del_port() without any rtnl
assertion. phylink_create_sfp_cage_port() in this same patch wraps the
same call:

	rtnl_lock();
	ret = phy_link_topo_add_port(pl->netdev, port);
	rtnl_unlock();

Should these paths assert or take rtnl?

> +}
> +

[ ... ]

> @@ -1744,6 +1833,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;
> @@ -1876,6 +1967,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]
If phy_link_topo_add_port() fails here, or phy_link_topo_add_phy() fails
just above, goto error lands in phy_detach_internal(). That function
calls phy_link_topo_del_port(dev, phydev->mod_port) even though mod_port
was never inserted into dev's topology.

Can this remove some other port from the topology?

phy_link_topo_del_port() does a blind xa_erase(&topo->ports, port->id).
mod_port and its id survive a detach, for example for a PHY in PHY_ERROR
where phy_disconnect() skips phy_stop(), or when phy_detach() is called
on a started PHY. phy_link_topo_add_port() reuses a non-zero id with
xa_insert().

Suppose the PHY is then attached to a different netdev whose topology
already has a port at that index. xa_insert() returns -EBUSY, and the
error path erases that unrelated live port.

phy_link_topo_add_phy() only undoes the ports it actually added, using
list_for_each_entry_continue_reverse(). Should the mod_port unwind also
track whether its insert succeeded?

> +		}
>  	}
>  
>  	/* Some Ethernet drivers try to connect to a PHY device before

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

Re: [PATCH net-next v19 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 2 potential
issue(s) to consider.

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: phy: UAF in upstream_port when connect_phy fails
- [Low] Concern: phy_set_upstream_port()
  (drivers/net/phy/phy_device.c:3712-3722) writes phydev->upstream_port…

--

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

This commit adds an upstream_port pointer to struct phy_port and struct
phy_device. For an MDI port that is fed by an MII port, such as an SFP
module port fed by the SFP cage port, the pointer records that MII port.
A new helper, phy_set_upstream_port(), is called from the SFP
connect_phy/disconnect_phy paths in both phy_device.c and phylink.c.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 13eb1d40badb5..e9da6767faaf4 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -3698,6 +3703,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;
> +}

[Severity: Low]
Can phydev->upstream_port and port->upstream_port get out of sync here?
This helper only updates the ports that are on phydev->ports when it
runs.

Ports added later through phy_probe()->phy_setup_ports()->phy_add_port()
do not copy phydev->upstream_port:

drivers/net/phy/phy_device.c:phy_add_port() {
    ...
	phy_port_update_supported(port);

	list_add(&port->head, &phydev->ports);

	phydev->n_ports++;
    ...
}

This can happen if the module PHY driver binds after connect. The
phy_sfp_connect_phy() path has no phy->drv check. It can also happen if
the driver is unbound and rebound while the PHY is connected. In both
cases phydev->upstream_port still points at the SFP cage port, but the
new ports have upstream_port == NULL.

Today the mismatch is not visible. Nothing in the series reads
phydev->upstream_port. The only reader of port->upstream_port is
port_prepare_data() in net/ethtool/port.c, and it only sees ports in
the link topology. Ports created later by phy_add_port() are never
added to the topology.

Would it make sense for phy_add_port() to set port->upstream_port from
phydev->upstream_port, so that both fields stay consistent for future
users?

> +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

[ ... ]

> @@ -1403,6 +1403,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: High]
If a later step in the connect path fails, do phy->upstream_port and
the module PHY's port->upstream_port pointers stay pointing at the cage
port?

phy_sfp_connect_phy() calls phy_set_upstream_port() before
phy_link_topo_add_phy(). When phy_link_topo_add_phy() fails, it only
resets phydev->has_sfp_mod_phy before returning the error.

phylink_sfp_connect_phy() follows the same pattern. It sets the
upstream port and then returns whatever phylink_sfp_config_phy()
returns. That call can fail in phylink_validate(),
phylink_sfp_select_interface(), phylink_attach_phy() or
phylink_bringup_phy().

The sfp-bus core does not call ->disconnect_phy() when ->connect_phy()
fails, so the NULL reset added to phy_sfp_disconnect_phy() and
phylink_sfp_disconnect_phy() does not cover this case.

When the module PHY is first probed through sfp_sm_probe_phy(), a
failure destroys the PHY, so the stale pointer goes away with it.
However, if I read sfp_register_bus() correctly, ->connect_phy() is
also reached when the upstream registers on a bus that already has a
module PHY in bus->phydev. The upstream here is a PHY driver via
phy_sfp_probe(), or phylink.

In that case the error goes back to the upstream, which then tears
down and frees its sfp_cage_port. Meanwhile bus->phydev stays alive on
the SFP bus. Its upstream_port, and the upstream_port of each of its
ports, still point at the freed phy_port. Anything that later follows
port->upstream_port, such as the ethtool port code reading
upstream_port->id later in this series, would then hit a
use-after-free.

Could phy_set_upstream_port(phy, NULL) be added to the error paths of
both connect callbacks? Or, if the pointer does not need to be valid
while phy_link_topo_add_phy() and phylink_sfp_config_phy() run, could
the assignment simply move after those calls succeed?

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

Re: [PATCH net-next v19 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 2 potential
issue(s) to consider.

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Most concerning: the RTNL-only lifetime rule that this patch
  sets for phy_link_topo_get_port() is broken by the phylib teardown…
- [High] net: phy: NULL pointer dereference in netdev_need_ops_lock()

--

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

This adds phy_link_topo_get_port() so netlink can look up phy_ports
stored in the link topology. Callers must hold rtnl while they use the
returned port. phy_link_topo_add_port() now also warns and refuses to add
ports on ops-locked devices.

> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> index 296ee514ba463..f2e493c433fff 100644
> --- a/include/linux/phy_link_topology.h
> +++ b/include/linux/phy_link_topology.h

[ ... ]

> @@ -71,6 +72,24 @@ 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();
> +
> +	return xa_load(&topo->ports, port_id);

[Severity: High]
Is RTNL really enough to keep the returned port alive?

This helper takes no reference on the port. It relies on every writer
holding RTNL when it erases and frees ports from topo->ports. The phylib
teardown path does not seem to do that.

phy_remove() is the driver-core remove callback and runs without RTNL:

drivers/net/phy/phy_device.c:phy_remove() {
    ...
	phy_sfp_release(phydev);
	phy_cleanup_ports(phydev);
    ...
}

Both of these go through phy_del_port() and then free the port:

drivers/net/phy/phy_device.c:phy_cleanup_ports() {
	list_for_each_entry_safe(port, tmp, &phydev->ports, head) {
		phy_del_port(phydev, port);
		phy_port_destroy(port);
	}
}

drivers/net/phy/phy_device.c:phy_del_port() {
    ...
	if (phydev->attached_dev)
		phy_link_topo_del_port(phydev->attached_dev, port);
    ...
}

phy_link_topo_del_port() does the xa_erase(), and phy_port_destroy() does
the kfree(). phy_sfp_release() takes and drops RTNL inside
sfp_bus_del_upstream(). Its later erase and free therefore also run
without RTNL.

phy_remove() can run while attached_dev is still set. One case is a sysfs
unbind of the PHY driver: try_module_get() in phy_attach_direct() only
blocks rmmod. The other is removal of the PHY device itself: the
device_link created in phy_attach_direct() is DL_FLAG_STATELESS, so it
does not force the MAC to unbind first.

Later in the series, port_prepare_data() in net/ethtool/port.c holds RTNL
and uses this helper:

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

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

It then reads port->interfaces, port->id, port->is_mii, port->is_sfp and
port->upstream_port->id.

Could a PORT_GET doit or dump run concurrently with phy_remove() and
read the struct phy_port after phy_port_destroy() has freed it?

The phylink side already follows the intended rule:

drivers/net/phy/phylink.c:phylink_destroy_sfp_cage_port() {
    ...
		/* The port may still be accessed under RTNL */
		rtnl_lock();
		phy_link_topo_del_port(pl->netdev, pl->sfp_cage_port);
		rtnl_unlock();
    ...
}

The phy_device.c delete paths do not follow it. That includes the
mod_port removal in phy_detach_internal().

Should phy_del_port() and its callers take RTNL, or at least assert it,
around the topology removal and the free? This also appears to be
unchanged at the end of the series.

> +}
> +

[ ... ]

> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> --- 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(dev->netdev_ops && netdev_need_ops_lock(dev)))
> +		return -EOPNOTSUPP;
> +
>  	topo = phy_link_topo_get_or_alloc(dev);
>  	if (IS_ERR(topo))
>  		return PTR_ERR(topo);

[Severity: High]
The dev->netdev_ops test here is needed because netdev_need_ops_lock()
dereferences dev->netdev_ops->net_shaper_ops when CONFIG_NET_SHAPER is
enabled. Doesn't phy_link_topo_add_phy() in the same file need the same
guard?

As far as I can see it does this, with no check on dev->netdev_ops:

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

phy_attach_direct() calls phy_link_topo_add_phy() for whatever
net_device the PHY is attached to. Some in-tree users attach a PHY to
a net_device that never gets netdev_ops. For example, gbe_probe() in
drivers/net/ethernet/ti/netcp_ethss.c passes gbe_dev->dummy_ndev to
of_phy_connect(). With CONFIG_NET_SHAPER=y that is a NULL pointer
dereference during probe.

Should phy_link_topo_add_phy() use the same
dev->netdev_ops && netdev_need_ops_lock(dev) test as
phy_link_topo_add_port()? Alternatively, netdev_need_ops_lock() could
tolerate a NULL netdev_ops itself, so callers don't each have to
remember the check. Which do you prefer?

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

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

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] Series-ordering note only, not a bug in the final series.
- [Low] The new uAPI enum port-type
  (Documentation/netlink/specs/ethtool.yaml line 216, generated as enum…

--

netlink: specs: Add ethernet port listing with ethtool

This adds the YAML spec, the documentation and the generated uAPI header
definitions for a new ETHTOOL_MSG_PORT_GET command. The command reports
the front-facing ports tracked by the phy_port infrastructure. It defines
a new port-type enum (mdi, sfp) and a port attribute set with id,
supported modes, supported interfaces, type and upstream port.

> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index a66594ad9e739..a9cb025387830 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -212,6 +212,11 @@ definitions:
>        -
>          name: discard
>          value: 31
> +  -
> +    name: port-type
> +    doc: type of physical connector
> +    type: enum
> +    entries: [mdi, sfp]

[Severity: Low]
Could the meaning of this enum be spelled out more clearly? The doc
string "type of physical connector" is close to what
ETHTOOL_A_LINKINFO_PORT already carries with the legacy PORT_* values
(PORT_TP, PORT_FIBRE, PORT_OTHER, ...).

The existing phy_port_get_type() helper also uses the "port type" name,
but it returns PORT_* values:

drivers/net/phy/phy_port.c:phy_port_get_type() {
	if (port->mediums & BIT(ETHTOOL_LINK_MEDIUM_BASET))
		return PORT_TP;
    ...
}

So ETHTOOL_PORT_TYPE_MDI and PORT_TP are both 0, but they mean different
things. The PORT_GET section in ethtool-netlink.rst describes
ETHTOOL_A_PORT_TYPE only as "the port type". It does not name enum
ethtool_port_type or list the values.

The mdi and sfp entries also have no per-entry doc. Here is
port_fill_reply() in net/ethtool/port.c from the next patch in the
series:

	port_type = ETHTOOL_PORT_TYPE_MDI;
	if (reply->mii || reply->sfp)
		port_type = ETHTOOL_PORT_TYPE_SFP;

With this code an SFP cage reports sfp. The port of an SFP module plugged
into that cage (upstream_port set, is_sfp not set) reports mdi.

These values become fixed uAPI once released. Would it make sense to
document what mdi and sfp mean in each of these cases, and to reference
the enum from the rst?

[ ... ]

> 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]
This isn't a bug in the final series. At this commit, though, nothing
under net/ethtool/ handles ETHTOOL_MSG_PORT_GET: there is no policy, no
ethnl_default_requests entry and no genl op. A request therefore fails
with -EOPNOTSUPP. Meanwhile, the new PORT_GET section in
ethtool-netlink.rst already describes working DO and DUMP behaviour.

The next patch, "net: ethtool: Introduce ethtool command to list ports",
adds net/ethtool/port.c with ethnl_port_request_ops, ethnl_port_dumpit()
and ethnl_port_get_policy. It also registers ETHTOOL_MSG_PORT_GET in
net/ethtool/netlink.c, so the gap is closed by the end of the series.

Is this intermediate state intended? Splitting the spec from the
implementation is common netdev practice, so this is only a note.

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] PORT_GET can read a phy_port after it has been freed if a PHY
  driver is unbound at the same time.
- [Medium] The ETHTOOL_MSG_PORT_GET doit path does not always take RTNL,
  but port_prepare_data() relies on it.
- [Medium] net/ethtool: uapi breakage risk from exposing internal
  phy_interface_t enum
- [Low] When a DO request names an ETHTOOL_A_PORT_ID that does not exist,
  port_prepare_data() returns a bare -ENODEV without setting extack (no…

--

net: ethtool: Introduce ethtool command to list ports

This commit adds a new ethtool netlink command, ETHTOOL_MSG_PORT_GET. It
reports phy_port information to user space: supported linkmodes for MDI
ports, supported MII interfaces for empty SFP cages, the port type and
the upstream port. It also adds an ETH_SS_PHY_INTERFACES string set
backed by phy_interface_names[].

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

[ ... ]

> +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.
> +	 */

[Severity: Medium]
Is the "caller holds RTNL" statement true for the doit path?

PORT_GET uses ethnl_default_doit(), which only takes RTNL in some cases:

net/ethtool/netlink.c:ethnl_default_doit() {
    ...
		need_rtnl = !netdev_need_ops_lock(req_info->dev) ||
			    ethtool_nl_msg_needs_rtnl(req_info->dev, cmd);
		if (need_rtnl)
			rtnl_lock();
    ...
}

ethtool_nl_msg_needs_rtnl() in net/ethtool/common.h has no
ETHTOOL_MSG_PORT_GET case, so it returns false. On an ops-locked netdev,
port_prepare_data() then runs with only netdev_lock_ops() held.

If dev->link_topo is non-NULL, phy_link_topo_get_port() would hit
ASSERT_RTNL(). Also, nothing would order the xa_load() and the reads of
the port against writers that only hold RTNL, such as phylink SFP
add/del or phy_attach/detach.

port_dump_one_dev() always calls rtnl_lock(), so doit and dump use
different locking for the same command.

Right now the WARN in phy_link_topo_add_port() keeps ops-locked devices
from getting ports. That check is skipped when dev->netdev_ops is still
NULL:

    if (WARN_ON_ONCE(dev->netdev_ops && netdev_need_ops_lock(dev)))
        return -EOPNOTSUPP;

One example is the SFP cage port created from phylink_create() before
the driver sets netdev_ops. The check is also not repeated if
request_ops_lock or queue_mgmt_ops is set later.

Would adding "case ETHTOOL_MSG_PORT_GET: return true;" to
ethtool_nl_msg_needs_rtnl() give the doit path the locking this comment
describes?

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

[Severity: Low]
When the requested ETHTOOL_A_PORT_ID doesn't exist, this returns -ENODEV
with no extack set. ethnl_parse_header_dev_get() returns the same -ENODEV
for an unknown ifindex or name, but it does set an extack message.

In the doit path, could this use NL_SET_BAD_ATTR() or
NL_SET_ERR_MSG_ATTR() on info->attrs[ETHTOOL_A_PORT_ID]? That would let
user space tell an unknown port apart from an unknown device.
ethnl_req_get_phydev() already does this for phy_index.

> +
> +	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;

[Severity: High]
Can this read a phy_port after it has been freed, if the PHY driver is
unbound at the same time?

Only RTNL protects the port here. The unbind path seems to remove and
free ports without holding RTNL:

phy_remove()
  phy_sfp_release(phydev)
  phy_cleanup_ports(phydev)
    phy_del_port(phydev, port)
      phy_link_topo_del_port(phydev->attached_dev, port)  /* xa_erase */
    phy_port_destroy(port)                                /* kfree */

phy_remove() is the mdio bus .remove callback and holds only
device_lock. In phy_sfp_release(), sfp_bus_del_upstream() takes and
then drops RTNL itself:

    rtnl_lock();
    if (bus->sfp)
        sfp_unregister_bus(bus);
    sfp_upstream_clear(bus);
    rtnl_unlock();

So the is_sfp ports are deleted and freed after RTNL has been released.

The device link created in phy_attach_direct() is DL_FLAG_STATELESS.
Unbinding the PHY driver therefore does not unbind the MAC first.

Before this series phy_del_port() did not touch the topology. This patch
adds the first reader of topo->ports.

Suppose a PORT_GET doit or dump loads the port, and then phy_remove()
erases it and calls kfree() on it. Would linkmode_copy() and
phy_interface_copy() here copy freed memory into the reply? Would
port->upstream_port->id follow a stale pointer?

PORT_GET has no GENL_UNS_ADMIN_PERM, so the reader does not need any
privilege.

[ ... ]

> @@ -0,0 +1,385 @@
> +// 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;
> +
> +	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;
> +	}
> +
> +	port_type = ETHTOOL_PORT_TYPE_MDI;
> +	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));
> +
> +	if (tb[ETHTOOL_A_PORT_ID]) {
> +		NL_SET_BAD_ATTR(info->info.extack, tb[ETHTOOL_A_PORT_ID]);
> +		return -EINVAL;
> +	}
> +
> +	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;
> +
> +	ret = port_prepare_data(&ctx->req_info->base, &ctx->reply_data->base,
> +				genl_info_dump(cb));
> +	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 = 0;
> +
> +	dev = ctx->req_info->base.dev;
> +
> +	rtnl_lock();
> +	netdev_lock_ops(dev);
> +
> +	if (!dev->link_topo)
> +		goto out;
> +
> +	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)
> +			goto out;
> +	}
> +
> +	ret = 0;
> +
> +out:
> +	netdev_unlock_ops(dev);
> +	rtnl_unlock();
> +
> +	return ret;
> +}
> +
> +static int port_dump_all_dev(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	struct port_dump_ctx *ctx = port_dump_ctx_get(cb);
> +	unsigned long start_ifindex = ctx->pos_ifindex;
> +	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) {
> +		if (ctx->pos_ifindex != start_ifindex)
> +			ctx->pos_portid = 0;
> +
> +		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)
> +			break;
> +	}
> +	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);
> +
> +		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,
> +};

[Severity: Medium]
The bit positions in ETHTOOL_A_PORT_SUPPORTED_INTERFACES, and the
string IDs in the new ETH_SS_PHY_INTERFACES set, come directly from
phy_interface_t. That enum lives in include/linux/phy.h, not in a uapi
header, and nothing marks it as append-only. New modes have also not
always been added at the end; some were inserted next to related modes
in the middle of the enum.

Compare this with ETH_SS_LINK_MODES, which is backed by
enum ethtool_link_mode_bit_indices in include/uapi/linux/ethtool.h,
where new entries may only be appended.

With this patch, any later insertion into phy_interface_t renumbers
every interface after it on the wire. A compact bitset (the
ETHTOOL_FLAG_COMPACT_BITSETS case in port_fill_reply()) carries only
the bit positions. A tool that caches or hardcodes those indices would
then silently decode the reply as the wrong interfaces. The
phy_interface_names[] table filled by ethnl_phy_names_populate()
follows the renumbering, so only clients that fetch the string set
every time are unaffected.

Once this is merged the numbering becomes uAPI and can't be fixed
afterwards. Should phy_interface_t, or a uapi copy of it with a
translation table, live in include/uapi/linux/ethtool.h with an
append-only rule? Or, at a minimum, should include/linux/phy.h say
that the values are now ABI and must not be reordered? If the intent
is that user space always resolves these bits through
ETH_SS_PHY_INTERFACES, could that be documented in the netlink spec?

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