[PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[Maxime Chevallier]

Hello everyone,

This is V17 of the phy_link_topology series, aiming at improving support
for multiple PHYs being attached to the same MAC.

V17 is mostly a rebase of V16 on net-next, as the addition of new
features in the PSE-PD command raised a conflict on the ethtool netlink
spec, and patch 10 was updated :

	("net: ethtool: pse-pd: Target the command to the requested PHY")

The new code was updated to make use of the new helpers to retrieve the
PHY from the ethnl request, and an error message was also updated to
better reflect the fact that we don't only rely on the attached PHY for
configuration.

As a remainder, here's what the PHY listings would look like :
 - eth0 has a 88x3310 acting as media converter, and an SFP module with
   an embedded 88e1111 PHY
 - eth2 has a 88e1510 PHY

# ethtool --show-phys *

PHY for eth0:
PHY index: 1
Driver name: mv88x3310
PHY device name: f212a600.mdio-mii:00
Downstream SFP bus name: sfp-eth0
Upstream type: MAC

PHY for eth0:
PHY index: 2
Driver name: Marvell 88E1111
PHY device name: i2c:sfp-eth0:16
Upstream type: PHY
Upstream PHY index: 1
Upstream SFP name: sfp-eth0

PHY for eth2:
PHY index: 1
Driver name: Marvell 88E1510
PHY device name: f212a200.mdio-mii:00
Upstream type: MAC

Ethtool patches : https://github.com/minimaxwell/ethtool/tree/mc/topo-v16

Link to V16: https://lore.kernel.org/netdev/20240705132706.13588-1-maxime.chevallier@bootlin.com/
Link to V15: https://lore.kernel.org/netdev/20240703140806.271938-1-maxime.chevallier@bootlin.com/
Link to V14: https://lore.kernel.org/netdev/20240701131801.1227740-1-maxime.chevallier@bootlin.com/
Link to V13: https://lore.kernel.org/netdev/20240607071836.911403-1-maxime.chevallier@bootlin.com/
Link to v12: https://lore.kernel.org/netdev/20240605124920.720690-1-maxime.chevallier@bootlin.com/
Link to v11: https://lore.kernel.org/netdev/20240404093004.2552221-1-maxime.chevallier@bootlin.com/
Link to V10: https://lore.kernel.org/netdev/20240304151011.1610175-1-maxime.chevallier@bootlin.com/
Link to V9: https://lore.kernel.org/netdev/20240228114728.51861-1-maxime.chevallier@bootlin.com/
Link to V8: https://lore.kernel.org/netdev/20240220184217.3689988-1-maxime.chevallier@bootlin.com/
Link to V7: https://lore.kernel.org/netdev/20240213150431.1796171-1-maxime.chevallier@bootlin.com/
Link to V6: https://lore.kernel.org/netdev/20240126183851.2081418-1-maxime.chevallier@bootlin.com/
Link to V5: https://lore.kernel.org/netdev/20231221180047.1924733-1-maxime.chevallier@bootlin.com/
Link to V4: https://lore.kernel.org/netdev/20231215171237.1152563-1-maxime.chevallier@bootlin.com/
Link to V3: https://lore.kernel.org/netdev/20231201163704.1306431-1-maxime.chevallier@bootlin.com/
Link to V2: https://lore.kernel.org/netdev/20231117162323.626979-1-maxime.chevallier@bootlin.com/
Link to V1: https://lore.kernel.org/netdev/20230907092407.647139-1-maxime.chevallier@bootlin.com/

More discussions on specific issues that happened in 6.9-rc:

https://lore.kernel.org/netdev/20240412104615.3779632-1-maxime.chevallier@bootlin.com/
https://lore.kernel.org/netdev/20240429131008.439231-1-maxime.chevallier@bootlin.com/
https://lore.kernel.org/netdev/20240507102822.2023826-1-maxime.chevallier@bootlin.com/

Maxime Chevallier (14):
  net: phy: Introduce ethernet link topology representation
  net: sfp: pass the phy_device when disconnecting an sfp module's PHY
  net: phy: add helpers to handle sfp phy connect/disconnect
  net: sfp: Add helper to return the SFP bus name
  net: ethtool: Allow passing a phy index for some commands
  netlink: specs: add phy-index as a header parameter
  net: ethtool: Introduce a command to list PHYs on an interface
  netlink: specs: add ethnl PHY_GET command set
  net: ethtool: plca: Target the command to the requested PHY
  net: ethtool: pse-pd: Target the command to the requested PHY
  net: ethtool: cable-test: Target the command to the requested PHY
  net: ethtool: strset: Remove unnecessary check on genl_info
  net: ethtool: strset: Allow querying phy stats by index
  Documentation: networking: document phy_link_topology

 Documentation/netlink/specs/ethtool.yaml      |  58 ++++
 Documentation/networking/ethtool-netlink.rst  |  51 +++
 Documentation/networking/index.rst            |   1 +
 .../networking/phy-link-topology.rst          | 121 +++++++
 MAINTAINERS                                   |   1 +
 drivers/net/phy/Makefile                      |   2 +-
 drivers/net/phy/marvell-88x2222.c             |   2 +
 drivers/net/phy/marvell.c                     |   2 +
 drivers/net/phy/marvell10g.c                  |   2 +
 drivers/net/phy/phy_device.c                  |  48 +++
 drivers/net/phy/phy_link_topology.c           | 105 ++++++
 drivers/net/phy/phylink.c                     |   3 +-
 drivers/net/phy/qcom/at803x.c                 |   2 +
 drivers/net/phy/qcom/qca807x.c                |   2 +
 drivers/net/phy/sfp-bus.c                     |  26 +-
 include/linux/netdevice.h                     |   4 +-
 include/linux/phy.h                           |   6 +
 include/linux/phy_link_topology.h             |  82 +++++
 include/linux/sfp.h                           |   8 +-
 include/uapi/linux/ethtool.h                  |  16 +
 include/uapi/linux/ethtool_netlink.h          |  20 ++
 net/core/dev.c                                |  15 +
 net/ethtool/Makefile                          |   3 +-
 net/ethtool/cabletest.c                       |  35 +-
 net/ethtool/netlink.c                         |  66 +++-
 net/ethtool/netlink.h                         |  33 ++
 net/ethtool/phy.c                             | 308 ++++++++++++++++++
 net/ethtool/plca.c                            |  30 +-
 net/ethtool/pse-pd.c                          |  31 +-
 net/ethtool/strset.c                          |  27 +-
 30 files changed, 1057 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/networking/phy-link-topology.rst
 create mode 100644 drivers/net/phy/phy_link_topology.c
 create mode 100644 include/linux/phy_link_topology.h
 create mode 100644 net/ethtool/phy.c

-- 
2.45.1

[PATCH net-next v17 01/14] net: phy: Introduce ethernet link topology representation

[Maxime Chevallier]

Link topologies containing multiple network PHYs attached to the same
net_device can be found when using a PHY as a media converter for use
with an SFP connector, on which an SFP transceiver containing a PHY can
be used.

With the current model, the transceiver's PHY can't be used for
operations such as cable testing, timestamping, macsec offload, etc.

The reason being that most of the logic for these configuration, coming
from either ethtool netlink or ioctls tend to use netdev->phydev, which
in multi-phy systems will reference the PHY closest to the MAC.

Introduce a numbering scheme allowing to enumerate PHY devices that
belong to any netdev, which can in turn allow userspace to take more
precise decisions with regard to each PHY's configuration.

The numbering is maintained per-netdev, in a phy_device_list.
The numbering works similarly to a netdevice's ifindex, with
identifiers that are only recycled once INT_MAX has been reached.

This prevents races that could occur between PHY listing and SFP
transceiver removal/insertion.

The identifiers are assigned at phy_attach time, as the numbering
depends on the netdevice the phy is attached to. The PHY index can be
re-used for PHYs that are persistent.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 MAINTAINERS                         |   1 +
 drivers/net/phy/Makefile            |   2 +-
 drivers/net/phy/phy_device.c        |   6 ++
 drivers/net/phy/phy_link_topology.c | 105 ++++++++++++++++++++++++++++
 include/linux/netdevice.h           |   4 +-
 include/linux/phy.h                 |   4 ++
 include/linux/phy_link_topology.h   |  82 ++++++++++++++++++++++
 include/uapi/linux/ethtool.h        |  16 +++++
 net/core/dev.c                      |  15 ++++
 9 files changed, 233 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/phy/phy_link_topology.c
 create mode 100644 include/linux/phy_link_topology.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e0f28278e504..5135c3379234 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8194,6 +8194,7 @@ F:	include/linux/mii.h
 F:	include/linux/of_net.h
 F:	include/linux/phy.h
 F:	include/linux/phy_fixed.h
+F:	include/linux/phy_link_topology.h
 F:	include/linux/phylib_stubs.h
 F:	include/linux/platform_data/mdio-bcm-unimac.h
 F:	include/linux/platform_data/mdio-gpio.h
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 202ed7f450da..1d8be374915f 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -2,7 +2,7 @@
 # Makefile for Linux PHY drivers
 
 libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o \
-				   linkmode.o
+				   linkmode.o phy_link_topology.o
 mdio-bus-y			+= mdio_bus.o mdio_device.o
 
 ifdef CONFIG_MDIO_DEVICE
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 70b07e621fb2..e68acaba1b4f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -29,6 +29,7 @@
 #include <linux/phy.h>
 #include <linux/phylib_stubs.h>
 #include <linux/phy_led_triggers.h>
+#include <linux/phy_link_topology.h>
 #include <linux/pse-pd/pse.h>
 #include <linux/property.h>
 #include <linux/rtnetlink.h>
@@ -1511,6 +1512,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 		if (phydev->sfp_bus_attached)
 			dev->sfp_bus = phydev->sfp_bus;
+
+		err = phy_link_topo_add_phy(dev, phydev, PHY_UPSTREAM_MAC, dev);
+		if (err)
+			goto error;
 	}
 
 	/* Some Ethernet drivers try to connect to a PHY device before
@@ -1938,6 +1943,7 @@ void phy_detach(struct phy_device *phydev)
 	if (dev) {
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
+		phy_link_topo_del_phy(dev, phydev);
 	}
 	phydev->phylink = NULL;
 
diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
new file mode 100644
index 000000000000..4a5d73002a1a
--- /dev/null
+++ b/drivers/net/phy/phy_link_topology.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Infrastructure to handle all PHY devices connected to a given netdev,
+ * either directly or indirectly attached.
+ *
+ * Copyright (c) 2023 Maxime Chevallier<maxime.chevallier@bootlin.com>
+ */
+
+#include <linux/phy_link_topology.h>
+#include <linux/phy.h>
+#include <linux/rtnetlink.h>
+#include <linux/xarray.h>
+
+static int netdev_alloc_phy_link_topology(struct net_device *dev)
+{
+	struct phy_link_topology *topo;
+
+	topo = kzalloc(sizeof(*topo), GFP_KERNEL);
+	if (!topo)
+		return -ENOMEM;
+
+	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
+	topo->next_phy_index = 1;
+
+	dev->link_topo = topo;
+
+	return 0;
+}
+
+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_device_node *pdn;
+	int ret;
+
+	if (!topo) {
+		ret = netdev_alloc_phy_link_topology(dev);
+		if (ret)
+			return ret;
+
+		topo = dev->link_topo;
+	}
+
+	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
+	if (!pdn)
+		return -ENOMEM;
+
+	pdn->phy = phy;
+	switch (upt) {
+	case PHY_UPSTREAM_MAC:
+		pdn->upstream.netdev = (struct net_device *)upstream;
+		if (phy_on_sfp(phy))
+			pdn->parent_sfp_bus = pdn->upstream.netdev->sfp_bus;
+		break;
+	case PHY_UPSTREAM_PHY:
+		pdn->upstream.phydev = (struct phy_device *)upstream;
+		if (phy_on_sfp(phy))
+			pdn->parent_sfp_bus = pdn->upstream.phydev->sfp_bus;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+	pdn->upstream_type = upt;
+
+	/* Attempt to re-use a previously allocated phy_index */
+	if (phy->phyindex)
+		ret = xa_insert(&topo->phys, phy->phyindex, pdn, GFP_KERNEL);
+	else
+		ret = xa_alloc_cyclic(&topo->phys, &phy->phyindex, pdn,
+				      xa_limit_32b, &topo->next_phy_index,
+				      GFP_KERNEL);
+
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	kfree(pdn);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
+
+void phy_link_topo_del_phy(struct net_device *dev,
+			   struct phy_device *phy)
+{
+	struct phy_link_topology *topo = dev->link_topo;
+	struct phy_device_node *pdn;
+
+	if (!topo)
+		return;
+
+	pdn = xa_erase(&topo->phys, phy->phyindex);
+
+	/* We delete the PHY from the topology, however we don't re-set the
+	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
+	 * same index next time it's added back to the topology
+	 */
+
+	kfree(pdn);
+}
+EXPORT_SYMBOL_GPL(phy_link_topo_del_phy);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 93558645c6d0..937da1dfcb2c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -40,7 +40,6 @@
 #include <net/dcbnl.h>
 #endif
 #include <net/netprio_cgroup.h>
-
 #include <linux/netdev_features.h>
 #include <linux/neighbour.h>
 #include <linux/netdevice_xmit.h>
@@ -81,6 +80,7 @@ struct xdp_frame;
 struct xdp_metadata_ops;
 struct xdp_md;
 struct ethtool_netdev_state;
+struct phy_link_topology;
 
 typedef u32 xdp_features_t;
 
@@ -1977,6 +1977,7 @@ enum netdev_reg_state {
  *	@fcoe_ddp_xid:	Max exchange id for FCoE LRO by ddp
  *
  *	@priomap:	XXX: need comments on this one
+ *	@link_topo:	Physical link topology tracking attached PHYs
  *	@phydev:	Physical device may attach itself
  *			for hardware timestamping
  *	@sfp_bus:	attached &struct sfp_bus structure.
@@ -2368,6 +2369,7 @@ struct net_device {
 #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
 	struct netprio_map __rcu *priomap;
 #endif
+	struct phy_link_topology	*link_topo;
 	struct phy_device	*phydev;
 	struct sfp_bus		*sfp_bus;
 	struct lock_class_key	*qdisc_tx_busylock;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index bd68f9d8e74f..2d477eb2809a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -554,6 +554,9 @@ struct macsec_ops;
  * @drv: Pointer to the driver for this PHY instance
  * @devlink: Create a link between phy dev and mac dev, if the external phy
  *           used by current mac interface is managed by another mac interface.
+ * @phyindex: Unique id across the phy's parent tree of phys to address the PHY
+ *	      from userspace, similar to ifindex. A zero index means the PHY
+ *	      wasn't assigned an id yet.
  * @phy_id: UID for this device found during discovery
  * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
  * @is_c45:  Set to true if this PHY uses clause 45 addressing.
@@ -654,6 +657,7 @@ struct phy_device {
 
 	struct device_link *devlink;
 
+	u32 phyindex;
 	u32 phy_id;
 
 	struct phy_c45_device_ids c45_ids;
diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
new file mode 100644
index 000000000000..68a59e25821c
--- /dev/null
+++ b/include/linux/phy_link_topology.h
@@ -0,0 +1,82 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PHY device list allow maintaining a list of PHY devices that are
+ * part of a netdevice's link topology. PHYs can for example be chained,
+ * as is the case when using a PHY that exposes an SFP module, on which an
+ * SFP transceiver that embeds a PHY is connected.
+ *
+ * This list can then be used by userspace to leverage individual PHY
+ * capabilities.
+ */
+#ifndef __PHY_LINK_TOPOLOGY_H
+#define __PHY_LINK_TOPOLOGY_H
+
+#include <linux/ethtool.h>
+#include <linux/netdevice.h>
+
+struct xarray;
+struct phy_device;
+struct sfp_bus;
+
+struct phy_link_topology {
+	struct xarray phys;
+	u32 next_phy_index;
+};
+
+struct phy_device_node {
+	enum phy_upstream upstream_type;
+
+	union {
+		struct net_device	*netdev;
+		struct phy_device	*phydev;
+	} upstream;
+
+	struct sfp_bus *parent_sfp_bus;
+
+	struct phy_device *phy;
+};
+
+#if IS_ENABLED(CONFIG_PHYLIB)
+int phy_link_topo_add_phy(struct net_device *dev,
+			  struct phy_device *phy,
+			  enum phy_upstream upt, void *upstream);
+
+void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy);
+
+static inline struct phy_device *
+phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
+{
+	struct phy_link_topology *topo = dev->link_topo;
+	struct phy_device_node *pdn;
+
+	if (!topo)
+		return NULL;
+
+	pdn = xa_load(&topo->phys, phyindex);
+	if (pdn)
+		return pdn->phy;
+
+	return NULL;
+}
+
+#else
+static inline int phy_link_topo_add_phy(struct net_device *dev,
+					struct phy_device *phy,
+					enum phy_upstream upt, void *upstream)
+{
+	return 0;
+}
+
+static inline void phy_link_topo_del_phy(struct net_device *dev,
+					 struct phy_device *phy)
+{
+}
+
+static inline struct phy_device *
+phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __PHY_LINK_TOPOLOGY_H */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 230110b97029..baf9e4d1855b 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -2532,4 +2532,20 @@ struct ethtool_link_settings {
 	 * __u32 map_lp_advertising[link_mode_masks_nwords];
 	 */
 };
+
+/**
+ * enum phy_upstream - Represents the upstream component a given PHY device
+ * is connected to, as in what is on the other end of the MII bus. Most PHYs
+ * will be attached to an Ethernet MAC controller, but in some cases, there's
+ * an intermediate PHY used as a media-converter, which will driver another
+ * MII interface as its output.
+ * @PHY_UPSTREAM_MAC: Upstream component is a MAC (a switch port,
+ *		      or ethernet controller)
+ * @PHY_UPSTREAM_PHY: Upstream component is a PHY (likely a media converter)
+ */
+enum phy_upstream {
+	PHY_UPSTREAM_MAC,
+	PHY_UPSTREAM_PHY,
+};
+
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index 73e5af6943c3..cd316d97c145 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -158,6 +158,7 @@
 #include <net/page_pool/types.h>
 #include <net/page_pool/helpers.h>
 #include <net/rps.h>
+#include <linux/phy_link_topology.h>
 
 #include "dev.h"
 #include "net-sysfs.h"
@@ -10312,6 +10313,17 @@ static void netdev_do_free_pcpu_stats(struct net_device *dev)
 	}
 }
 
+static void netdev_free_phy_link_topology(struct net_device *dev)
+{
+	struct phy_link_topology *topo = dev->link_topo;
+
+	if (IS_ENABLED(CONFIG_PHYLIB) && topo) {
+		xa_destroy(&topo->phys);
+		kfree(topo);
+		dev->link_topo = NULL;
+	}
+}
+
 /**
  * register_netdevice() - register a network device
  * @dev: device to register
@@ -11108,6 +11120,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 #ifdef CONFIG_NET_SCHED
 	hash_init(dev->qdisc_hash);
 #endif
+
 	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
 	setup(dev);
 
@@ -11200,6 +11213,8 @@ void free_netdev(struct net_device *dev)
 	free_percpu(dev->xdp_bulkq);
 	dev->xdp_bulkq = NULL;
 
+	netdev_free_phy_link_topology(dev);
+
 	/*  Compatibility with error handling in drivers */
 	if (dev->reg_state == NETREG_UNINITIALIZED ||
 	    dev->reg_state == NETREG_DUMMY) {
-- 
2.45.1

Re: [PATCH net-next v17 01/14] net: phy: Introduce ethernet link topology representation

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> Link topologies containing multiple network PHYs attached to the same
> net_device can be found when using a PHY as a media converter for use
> with an SFP connector, on which an SFP transceiver containing a PHY can
> be used.
> 
> With the current model, the transceiver's PHY can't be used for
> operations such as cable testing, timestamping, macsec offload, etc.
> 
> The reason being that most of the logic for these configuration, coming
> from either ethtool netlink or ioctls tend to use netdev->phydev, which
> in multi-phy systems will reference the PHY closest to the MAC.
> 
> Introduce a numbering scheme allowing to enumerate PHY devices that
> belong to any netdev, which can in turn allow userspace to take more
> precise decisions with regard to each PHY's configuration.
> 
> The numbering is maintained per-netdev, in a phy_device_list.
> The numbering works similarly to a netdevice's ifindex, with
> identifiers that are only recycled once INT_MAX has been reached.
> 
> This prevents races that could occur between PHY listing and SFP
> transceiver removal/insertion.
> 
> The identifiers are assigned at phy_attach time, as the numbering
> depends on the netdevice the phy is attached to. The PHY index can be
> re-used for PHYs that are persistent.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   MAINTAINERS                         |   1 +
>   drivers/net/phy/Makefile            |   2 +-
>   drivers/net/phy/phy_device.c        |   6 ++
>   drivers/net/phy/phy_link_topology.c | 105 ++++++++++++++++++++++++++++
>   include/linux/netdevice.h           |   4 +-
>   include/linux/phy.h                 |   4 ++
>   include/linux/phy_link_topology.h   |  82 ++++++++++++++++++++++
>   include/uapi/linux/ethtool.h        |  16 +++++
>   net/core/dev.c                      |  15 ++++
>   9 files changed, 233 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/net/phy/phy_link_topology.c
>   create mode 100644 include/linux/phy_link_topology.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e0f28278e504..5135c3379234 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8194,6 +8194,7 @@ F:	include/linux/mii.h
>   F:	include/linux/of_net.h
>   F:	include/linux/phy.h
>   F:	include/linux/phy_fixed.h
> +F:	include/linux/phy_link_topology.h
>   F:	include/linux/phylib_stubs.h
>   F:	include/linux/platform_data/mdio-bcm-unimac.h
>   F:	include/linux/platform_data/mdio-gpio.h
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 202ed7f450da..1d8be374915f 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -2,7 +2,7 @@
>   # Makefile for Linux PHY drivers
>   
>   libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o \
> -				   linkmode.o
> +				   linkmode.o phy_link_topology.o
>   mdio-bus-y			+= mdio_bus.o mdio_device.o
>   
>   ifdef CONFIG_MDIO_DEVICE
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 70b07e621fb2..e68acaba1b4f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -29,6 +29,7 @@
>   #include <linux/phy.h>
>   #include <linux/phylib_stubs.h>
>   #include <linux/phy_led_triggers.h>
> +#include <linux/phy_link_topology.h>
>   #include <linux/pse-pd/pse.h>
>   #include <linux/property.h>
>   #include <linux/rtnetlink.h>
> @@ -1511,6 +1512,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>   
>   		if (phydev->sfp_bus_attached)
>   			dev->sfp_bus = phydev->sfp_bus;
> +
> +		err = phy_link_topo_add_phy(dev, phydev, PHY_UPSTREAM_MAC, dev);
> +		if (err)
> +			goto error;
>   	}
>   
>   	/* Some Ethernet drivers try to connect to a PHY device before
> @@ -1938,6 +1943,7 @@ void phy_detach(struct phy_device *phydev)
>   	if (dev) {
>   		phydev->attached_dev->phydev = NULL;
>   		phydev->attached_dev = NULL;
> +		phy_link_topo_del_phy(dev, phydev);
>   	}
>   	phydev->phylink = NULL;
>   
> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> new file mode 100644
> index 000000000000..4a5d73002a1a
> --- /dev/null
> +++ b/drivers/net/phy/phy_link_topology.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Infrastructure to handle all PHY devices connected to a given netdev,
> + * either directly or indirectly attached.
> + *
> + * Copyright (c) 2023 Maxime Chevallier<maxime.chevallier@bootlin.com>
> + */
> +
> +#include <linux/phy_link_topology.h>
> +#include <linux/phy.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/xarray.h>
> +
> +static int netdev_alloc_phy_link_topology(struct net_device *dev)
> +{
> +	struct phy_link_topology *topo;
> +
> +	topo = kzalloc(sizeof(*topo), GFP_KERNEL);
> +	if (!topo)
> +		return -ENOMEM;
> +
> +	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
> +	topo->next_phy_index = 1;
> +
> +	dev->link_topo = topo;
> +
> +	return 0;
> +}
> +
> +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_device_node *pdn;
> +	int ret;
> +
> +	if (!topo) {
> +		ret = netdev_alloc_phy_link_topology(dev);
> +		if (ret)
> +			return ret;
> +
> +		topo = dev->link_topo;
> +	}
> +
> +	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> +	if (!pdn)
> +		return -ENOMEM;
> +
> +	pdn->phy = phy;
> +	switch (upt) {
> +	case PHY_UPSTREAM_MAC:
> +		pdn->upstream.netdev = (struct net_device *)upstream;
> +		if (phy_on_sfp(phy))
> +			pdn->parent_sfp_bus = pdn->upstream.netdev->sfp_bus;
> +		break;
> +	case PHY_UPSTREAM_PHY:
> +		pdn->upstream.phydev = (struct phy_device *)upstream;
> +		if (phy_on_sfp(phy))
> +			pdn->parent_sfp_bus = pdn->upstream.phydev->sfp_bus;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +	pdn->upstream_type = upt;
> +
> +	/* Attempt to re-use a previously allocated phy_index */
> +	if (phy->phyindex)
> +		ret = xa_insert(&topo->phys, phy->phyindex, pdn, GFP_KERNEL);
> +	else
> +		ret = xa_alloc_cyclic(&topo->phys, &phy->phyindex, pdn,
> +				      xa_limit_32b, &topo->next_phy_index,
> +				      GFP_KERNEL);
> +
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	kfree(pdn);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
> +
> +void phy_link_topo_del_phy(struct net_device *dev,
> +			   struct phy_device *phy)
> +{
> +	struct phy_link_topology *topo = dev->link_topo;
> +	struct phy_device_node *pdn;
> +
> +	if (!topo)
> +		return;
> +
> +	pdn = xa_erase(&topo->phys, phy->phyindex);
> +
> +	/* We delete the PHY from the topology, however we don't re-set the
> +	 * phy->phyindex field. If the PHY isn't gone, we can re-assign it the
> +	 * same index next time it's added back to the topology
> +	 */
> +
> +	kfree(pdn);
> +}
> +EXPORT_SYMBOL_GPL(phy_link_topo_del_phy);
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 93558645c6d0..937da1dfcb2c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -40,7 +40,6 @@
>   #include <net/dcbnl.h>
>   #endif
>   #include <net/netprio_cgroup.h>
> -
>   #include <linux/netdev_features.h>
>   #include <linux/neighbour.h>
>   #include <linux/netdevice_xmit.h>
> @@ -81,6 +80,7 @@ struct xdp_frame;
>   struct xdp_metadata_ops;
>   struct xdp_md;
>   struct ethtool_netdev_state;
> +struct phy_link_topology;
>   
>   typedef u32 xdp_features_t;
>   
> @@ -1977,6 +1977,7 @@ enum netdev_reg_state {
>    *	@fcoe_ddp_xid:	Max exchange id for FCoE LRO by ddp
>    *
>    *	@priomap:	XXX: need comments on this one
> + *	@link_topo:	Physical link topology tracking attached PHYs
>    *	@phydev:	Physical device may attach itself
>    *			for hardware timestamping
>    *	@sfp_bus:	attached &struct sfp_bus structure.
> @@ -2368,6 +2369,7 @@ struct net_device {
>   #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
>   	struct netprio_map __rcu *priomap;
>   #endif
> +	struct phy_link_topology	*link_topo;
>   	struct phy_device	*phydev;
>   	struct sfp_bus		*sfp_bus;
>   	struct lock_class_key	*qdisc_tx_busylock;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index bd68f9d8e74f..2d477eb2809a 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -554,6 +554,9 @@ struct macsec_ops;
>    * @drv: Pointer to the driver for this PHY instance
>    * @devlink: Create a link between phy dev and mac dev, if the external phy
>    *           used by current mac interface is managed by another mac interface.
> + * @phyindex: Unique id across the phy's parent tree of phys to address the PHY
> + *	      from userspace, similar to ifindex. A zero index means the PHY
> + *	      wasn't assigned an id yet.
>    * @phy_id: UID for this device found during discovery
>    * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
>    * @is_c45:  Set to true if this PHY uses clause 45 addressing.
> @@ -654,6 +657,7 @@ struct phy_device {
>   
>   	struct device_link *devlink;
>   
> +	u32 phyindex;
>   	u32 phy_id;
>   
>   	struct phy_c45_device_ids c45_ids;
> diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> new file mode 100644
> index 000000000000..68a59e25821c
> --- /dev/null
> +++ b/include/linux/phy_link_topology.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * PHY device list allow maintaining a list of PHY devices that are
> + * part of a netdevice's link topology. PHYs can for example be chained,
> + * as is the case when using a PHY that exposes an SFP module, on which an
> + * SFP transceiver that embeds a PHY is connected.
> + *
> + * This list can then be used by userspace to leverage individual PHY
> + * capabilities.
> + */
> +#ifndef __PHY_LINK_TOPOLOGY_H
> +#define __PHY_LINK_TOPOLOGY_H
> +
> +#include <linux/ethtool.h>
> +#include <linux/netdevice.h>
> +
> +struct xarray;
> +struct phy_device;
> +struct sfp_bus;
> +
> +struct phy_link_topology {
> +	struct xarray phys;
> +	u32 next_phy_index;
> +};
> +
> +struct phy_device_node {
> +	enum phy_upstream upstream_type;
> +
> +	union {
> +		struct net_device	*netdev;
> +		struct phy_device	*phydev;
> +	} upstream;
> +
> +	struct sfp_bus *parent_sfp_bus;
> +
> +	struct phy_device *phy;
> +};
> +
> +#if IS_ENABLED(CONFIG_PHYLIB)
> +int phy_link_topo_add_phy(struct net_device *dev,
> +			  struct phy_device *phy,
> +			  enum phy_upstream upt, void *upstream);
> +
> +void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy);
> +
> +static inline struct phy_device *
> +phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
> +{
> +	struct phy_link_topology *topo = dev->link_topo;
> +	struct phy_device_node *pdn;
> +
> +	if (!topo)
> +		return NULL;
> +
> +	pdn = xa_load(&topo->phys, phyindex);
> +	if (pdn)
> +		return pdn->phy;
> +
> +	return NULL;
> +}
> +
> +#else
> +static inline int phy_link_topo_add_phy(struct net_device *dev,
> +					struct phy_device *phy,
> +					enum phy_upstream upt, void *upstream)
> +{
> +	return 0;
> +}
> +
> +static inline void phy_link_topo_del_phy(struct net_device *dev,
> +					 struct phy_device *phy)
> +{
> +}
> +
> +static inline struct phy_device *
> +phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
> +{
> +	return NULL;
> +}
> +#endif
> +
> +#endif /* __PHY_LINK_TOPOLOGY_H */
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 230110b97029..baf9e4d1855b 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -2532,4 +2532,20 @@ struct ethtool_link_settings {
>   	 * __u32 map_lp_advertising[link_mode_masks_nwords];
>   	 */
>   };
> +
> +/**
> + * enum phy_upstream - Represents the upstream component a given PHY device
> + * is connected to, as in what is on the other end of the MII bus. Most PHYs
> + * will be attached to an Ethernet MAC controller, but in some cases, there's
> + * an intermediate PHY used as a media-converter, which will driver another
> + * MII interface as its output.
> + * @PHY_UPSTREAM_MAC: Upstream component is a MAC (a switch port,
> + *		      or ethernet controller)
> + * @PHY_UPSTREAM_PHY: Upstream component is a PHY (likely a media converter)
> + */
> +enum phy_upstream {
> +	PHY_UPSTREAM_MAC,
> +	PHY_UPSTREAM_PHY,
> +};
> +
>   #endif /* _UAPI_LINUX_ETHTOOL_H */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 73e5af6943c3..cd316d97c145 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -158,6 +158,7 @@
>   #include <net/page_pool/types.h>
>   #include <net/page_pool/helpers.h>
>   #include <net/rps.h>
> +#include <linux/phy_link_topology.h>
>   
>   #include "dev.h"
>   #include "net-sysfs.h"
> @@ -10312,6 +10313,17 @@ static void netdev_do_free_pcpu_stats(struct net_device *dev)
>   	}
>   }
>   
> +static void netdev_free_phy_link_topology(struct net_device *dev)
> +{
> +	struct phy_link_topology *topo = dev->link_topo;
> +
> +	if (IS_ENABLED(CONFIG_PHYLIB) && topo) {
> +		xa_destroy(&topo->phys);
> +		kfree(topo);
> +		dev->link_topo = NULL;
> +	}
> +}
> +
>   /**
>    * register_netdevice() - register a network device
>    * @dev: device to register
> @@ -11108,6 +11120,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>   #ifdef CONFIG_NET_SCHED
>   	hash_init(dev->qdisc_hash);
>   #endif
> +
>   	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
>   	setup(dev);
>   
> @@ -11200,6 +11213,8 @@ void free_netdev(struct net_device *dev)
>   	free_percpu(dev->xdp_bulkq);
>   	dev->xdp_bulkq = NULL;
>   
> +	netdev_free_phy_link_topology(dev);
> +
>   	/*  Compatibility with error handling in drivers */
>   	if (dev->reg_state == NETREG_UNINITIALIZED ||
>   	    dev->reg_state == NETREG_DUMMY) {

[PATCH net-next v17 02/14] net: sfp: pass the phy_device when disconnecting an sfp module's PHY

[Maxime Chevallier]

Pass the phy_device as a parameter to the sfp upstream .disconnect_phy
operation. This is preparatory work to help track phy devices across
a net_device's link.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phylink.c | 3 ++-
 drivers/net/phy/sfp-bus.c | 4 ++--
 include/linux/sfp.h       | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 51c526d227fa..ab4e9fc03017 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3423,7 +3423,8 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 	return ret;
 }
 
-static void phylink_sfp_disconnect_phy(void *upstream)
+static void phylink_sfp_disconnect_phy(void *upstream,
+				       struct phy_device *phydev)
 {
 	phylink_disconnect_phy(upstream);
 }
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 2f44fc51848f..56953e66bb7b 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -487,7 +487,7 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
 			bus->socket_ops->stop(bus->sfp);
 		bus->socket_ops->detach(bus->sfp);
 		if (bus->phydev && ops && ops->disconnect_phy)
-			ops->disconnect_phy(bus->upstream);
+			ops->disconnect_phy(bus->upstream, bus->phydev);
 	}
 	bus->registered = false;
 }
@@ -743,7 +743,7 @@ void sfp_remove_phy(struct sfp_bus *bus)
 	const struct sfp_upstream_ops *ops = sfp_get_upstream_ops(bus);
 
 	if (ops && ops->disconnect_phy)
-		ops->disconnect_phy(bus->upstream);
+		ops->disconnect_phy(bus->upstream, bus->phydev);
 	bus->phydev = NULL;
 }
 EXPORT_SYMBOL_GPL(sfp_remove_phy);
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index b14be59550e3..54abb4d22b2e 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -550,7 +550,7 @@ struct sfp_upstream_ops {
 	void (*link_down)(void *priv);
 	void (*link_up)(void *priv);
 	int (*connect_phy)(void *priv, struct phy_device *);
-	void (*disconnect_phy)(void *priv);
+	void (*disconnect_phy)(void *priv, struct phy_device *);
 };
 
 #if IS_ENABLED(CONFIG_SFP)
-- 
2.45.1

Re: [PATCH net-next v17 02/14] net: sfp: pass the phy_device when disconnecting an sfp module's PHY

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> Pass the phy_device as a parameter to the sfp upstream .disconnect_phy
> operation. This is preparatory work to help track phy devices across
> a net_device's link.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   drivers/net/phy/phylink.c | 3 ++-
>   drivers/net/phy/sfp-bus.c | 4 ++--
>   include/linux/sfp.h       | 2 +-
>   3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 51c526d227fa..ab4e9fc03017 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -3423,7 +3423,8 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
>   	return ret;
>   }
>   
> -static void phylink_sfp_disconnect_phy(void *upstream)
> +static void phylink_sfp_disconnect_phy(void *upstream,
> +				       struct phy_device *phydev)
>   {
>   	phylink_disconnect_phy(upstream);
>   }
> diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> index 2f44fc51848f..56953e66bb7b 100644
> --- a/drivers/net/phy/sfp-bus.c
> +++ b/drivers/net/phy/sfp-bus.c
> @@ -487,7 +487,7 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
>   			bus->socket_ops->stop(bus->sfp);
>   		bus->socket_ops->detach(bus->sfp);
>   		if (bus->phydev && ops && ops->disconnect_phy)
> -			ops->disconnect_phy(bus->upstream);
> +			ops->disconnect_phy(bus->upstream, bus->phydev);
>   	}
>   	bus->registered = false;
>   }
> @@ -743,7 +743,7 @@ void sfp_remove_phy(struct sfp_bus *bus)
>   	const struct sfp_upstream_ops *ops = sfp_get_upstream_ops(bus);
>   
>   	if (ops && ops->disconnect_phy)
> -		ops->disconnect_phy(bus->upstream);
> +		ops->disconnect_phy(bus->upstream, bus->phydev);
>   	bus->phydev = NULL;
>   }
>   EXPORT_SYMBOL_GPL(sfp_remove_phy);
> diff --git a/include/linux/sfp.h b/include/linux/sfp.h
> index b14be59550e3..54abb4d22b2e 100644
> --- a/include/linux/sfp.h
> +++ b/include/linux/sfp.h
> @@ -550,7 +550,7 @@ struct sfp_upstream_ops {
>   	void (*link_down)(void *priv);
>   	void (*link_up)(void *priv);
>   	int (*connect_phy)(void *priv, struct phy_device *);
> -	void (*disconnect_phy)(void *priv);
> +	void (*disconnect_phy)(void *priv, struct phy_device *);
>   };
>   
>   #if IS_ENABLED(CONFIG_SFP)

[PATCH net-next v17 03/14] net: phy: add helpers to handle sfp phy connect/disconnect

[Maxime Chevallier]

There are a few PHY drivers that can handle SFP modules through their
sfp_upstream_ops. Introduce Phylib helpers to keep track of connected
SFP PHYs in a netdevice's namespace, by adding the SFP PHY to the
upstream PHY's netdev's namespace.

By doing so, these SFP PHYs can be enumerated and exposed to users,
which will be able to use their capabilities.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell-88x2222.c |  2 ++
 drivers/net/phy/marvell.c         |  2 ++
 drivers/net/phy/marvell10g.c      |  2 ++
 drivers/net/phy/phy_device.c      | 42 +++++++++++++++++++++++++++++++
 drivers/net/phy/qcom/at803x.c     |  2 ++
 drivers/net/phy/qcom/qca807x.c    |  2 ++
 include/linux/phy.h               |  2 ++
 7 files changed, 54 insertions(+)

diff --git a/drivers/net/phy/marvell-88x2222.c b/drivers/net/phy/marvell-88x2222.c
index b88398e6872b..0b777cdd7078 100644
--- a/drivers/net/phy/marvell-88x2222.c
+++ b/drivers/net/phy/marvell-88x2222.c
@@ -553,6 +553,8 @@ static const struct sfp_upstream_ops sfp_phy_ops = {
 	.link_down = mv2222_sfp_link_down,
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 };
 
 static int mv2222_probe(struct phy_device *phydev)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index b89fbffa6a93..9964bf3dea2f 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -3613,6 +3613,8 @@ static const struct sfp_upstream_ops m88e1510_sfp_ops = {
 	.module_remove = m88e1510_sfp_remove,
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 };
 
 static int m88e1510_probe(struct phy_device *phydev)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index ad43e280930c..6642eb642d4b 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -503,6 +503,8 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
 static const struct sfp_upstream_ops mv3310_sfp_ops = {
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 	.module_insert = mv3310_sfp_insert,
 };
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e68acaba1b4f..a3309782220c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1370,6 +1370,48 @@ phy_standalone_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(phy_standalone);
 
+/**
+ * phy_sfp_connect_phy - Connect the SFP module's PHY to the upstream PHY
+ * @upstream: pointer to the upstream phy device
+ * @phy: pointer to the SFP module's phy device
+ *
+ * This helper allows keeping track of PHY devices on the link. It adds the
+ * SFP module's phy to the phy namespace of the upstream phy
+ *
+ * Return: 0 on success, otherwise a negative error code.
+ */
+int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
+{
+	struct phy_device *phydev = upstream;
+	struct net_device *dev = phydev->attached_dev;
+
+	if (dev)
+		return phy_link_topo_add_phy(dev, phy, PHY_UPSTREAM_PHY, phydev);
+
+	return 0;
+}
+EXPORT_SYMBOL(phy_sfp_connect_phy);
+
+/**
+ * phy_sfp_disconnect_phy - Disconnect the SFP module's PHY from the upstream PHY
+ * @upstream: pointer to the upstream phy device
+ * @phy: pointer to the SFP module's phy device
+ *
+ * This helper allows keeping track of PHY devices on the link. It removes the
+ * SFP module's phy to the phy namespace of the upstream phy. As the module phy
+ * will be destroyed, re-inserting the same module will add a new phy with a
+ * new index.
+ */
+void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
+{
+	struct phy_device *phydev = upstream;
+	struct net_device *dev = phydev->attached_dev;
+
+	if (dev)
+		phy_link_topo_del_phy(dev, phy);
+}
+EXPORT_SYMBOL(phy_sfp_disconnect_phy);
+
 /**
  * phy_sfp_attach - attach the SFP bus to the PHY upstream network device
  * @upstream: pointer to the phy device
diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index c8f83e5f78ab..105602581a03 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -770,6 +770,8 @@ static const struct sfp_upstream_ops at8031_sfp_ops = {
 	.attach = phy_sfp_attach,
 	.detach = phy_sfp_detach,
 	.module_insert = at8031_sfp_insert,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 };
 
 static int at8031_parse_dt(struct phy_device *phydev)
diff --git a/drivers/net/phy/qcom/qca807x.c b/drivers/net/phy/qcom/qca807x.c
index 672c6929119a..5eb0ab1cb70e 100644
--- a/drivers/net/phy/qcom/qca807x.c
+++ b/drivers/net/phy/qcom/qca807x.c
@@ -699,6 +699,8 @@ static const struct sfp_upstream_ops qca807x_sfp_ops = {
 	.detach = phy_sfp_detach,
 	.module_insert = qca807x_sfp_insert,
 	.module_remove = qca807x_sfp_remove,
+	.connect_phy = phy_sfp_connect_phy,
+	.disconnect_phy = phy_sfp_disconnect_phy,
 };
 
 static int qca807x_probe(struct phy_device *phydev)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2d477eb2809a..f7ef7ed6d5ce 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1762,6 +1762,8 @@ int phy_suspend(struct phy_device *phydev);
 int phy_resume(struct phy_device *phydev);
 int __phy_resume(struct phy_device *phydev);
 int phy_loopback(struct phy_device *phydev, bool enable);
+int phy_sfp_connect_phy(void *upstream, struct phy_device *phy);
+void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy);
 void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
 void phy_sfp_detach(void *upstream, struct sfp_bus *bus);
 int phy_sfp_probe(struct phy_device *phydev,
-- 
2.45.1

Re: [PATCH net-next v17 03/14] net: phy: add helpers to handle sfp phy connect/disconnect

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> There are a few PHY drivers that can handle SFP modules through their
> sfp_upstream_ops. Introduce Phylib helpers to keep track of connected
> SFP PHYs in a netdevice's namespace, by adding the SFP PHY to the
> upstream PHY's netdev's namespace.
> 
> By doing so, these SFP PHYs can be enumerated and exposed to users,
> which will be able to use their capabilities.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   drivers/net/phy/marvell-88x2222.c |  2 ++
>   drivers/net/phy/marvell.c         |  2 ++
>   drivers/net/phy/marvell10g.c      |  2 ++
>   drivers/net/phy/phy_device.c      | 42 +++++++++++++++++++++++++++++++
>   drivers/net/phy/qcom/at803x.c     |  2 ++
>   drivers/net/phy/qcom/qca807x.c    |  2 ++
>   include/linux/phy.h               |  2 ++
>   7 files changed, 54 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell-88x2222.c b/drivers/net/phy/marvell-88x2222.c
> index b88398e6872b..0b777cdd7078 100644
> --- a/drivers/net/phy/marvell-88x2222.c
> +++ b/drivers/net/phy/marvell-88x2222.c
> @@ -553,6 +553,8 @@ static const struct sfp_upstream_ops sfp_phy_ops = {
>   	.link_down = mv2222_sfp_link_down,
>   	.attach = phy_sfp_attach,
>   	.detach = phy_sfp_detach,
> +	.connect_phy = phy_sfp_connect_phy,
> +	.disconnect_phy = phy_sfp_disconnect_phy,
>   };
>   
>   static int mv2222_probe(struct phy_device *phydev)
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index b89fbffa6a93..9964bf3dea2f 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -3613,6 +3613,8 @@ static const struct sfp_upstream_ops m88e1510_sfp_ops = {
>   	.module_remove = m88e1510_sfp_remove,
>   	.attach = phy_sfp_attach,
>   	.detach = phy_sfp_detach,
> +	.connect_phy = phy_sfp_connect_phy,
> +	.disconnect_phy = phy_sfp_disconnect_phy,
>   };
>   
>   static int m88e1510_probe(struct phy_device *phydev)
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index ad43e280930c..6642eb642d4b 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -503,6 +503,8 @@ static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
>   static const struct sfp_upstream_ops mv3310_sfp_ops = {
>   	.attach = phy_sfp_attach,
>   	.detach = phy_sfp_detach,
> +	.connect_phy = phy_sfp_connect_phy,
> +	.disconnect_phy = phy_sfp_disconnect_phy,
>   	.module_insert = mv3310_sfp_insert,
>   };
>   
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index e68acaba1b4f..a3309782220c 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1370,6 +1370,48 @@ phy_standalone_show(struct device *dev, struct device_attribute *attr,
>   }
>   static DEVICE_ATTR_RO(phy_standalone);
>   
> +/**
> + * phy_sfp_connect_phy - Connect the SFP module's PHY to the upstream PHY
> + * @upstream: pointer to the upstream phy device
> + * @phy: pointer to the SFP module's phy device
> + *
> + * This helper allows keeping track of PHY devices on the link. It adds the
> + * SFP module's phy to the phy namespace of the upstream phy
> + *
> + * Return: 0 on success, otherwise a negative error code.
> + */
> +int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct net_device *dev = phydev->attached_dev;
> +
> +	if (dev)
> +		return phy_link_topo_add_phy(dev, phy, PHY_UPSTREAM_PHY, phydev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(phy_sfp_connect_phy);
> +
> +/**
> + * phy_sfp_disconnect_phy - Disconnect the SFP module's PHY from the upstream PHY
> + * @upstream: pointer to the upstream phy device
> + * @phy: pointer to the SFP module's phy device
> + *
> + * This helper allows keeping track of PHY devices on the link. It removes the
> + * SFP module's phy to the phy namespace of the upstream phy. As the module phy
> + * will be destroyed, re-inserting the same module will add a new phy with a
> + * new index.
> + */
> +void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct net_device *dev = phydev->attached_dev;
> +
> +	if (dev)
> +		phy_link_topo_del_phy(dev, phy);
> +}
> +EXPORT_SYMBOL(phy_sfp_disconnect_phy);
> +
>   /**
>    * phy_sfp_attach - attach the SFP bus to the PHY upstream network device
>    * @upstream: pointer to the phy device
> diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
> index c8f83e5f78ab..105602581a03 100644
> --- a/drivers/net/phy/qcom/at803x.c
> +++ b/drivers/net/phy/qcom/at803x.c
> @@ -770,6 +770,8 @@ static const struct sfp_upstream_ops at8031_sfp_ops = {
>   	.attach = phy_sfp_attach,
>   	.detach = phy_sfp_detach,
>   	.module_insert = at8031_sfp_insert,
> +	.connect_phy = phy_sfp_connect_phy,
> +	.disconnect_phy = phy_sfp_disconnect_phy,
>   };
>   
>   static int at8031_parse_dt(struct phy_device *phydev)
> diff --git a/drivers/net/phy/qcom/qca807x.c b/drivers/net/phy/qcom/qca807x.c
> index 672c6929119a..5eb0ab1cb70e 100644
> --- a/drivers/net/phy/qcom/qca807x.c
> +++ b/drivers/net/phy/qcom/qca807x.c
> @@ -699,6 +699,8 @@ static const struct sfp_upstream_ops qca807x_sfp_ops = {
>   	.detach = phy_sfp_detach,
>   	.module_insert = qca807x_sfp_insert,
>   	.module_remove = qca807x_sfp_remove,
> +	.connect_phy = phy_sfp_connect_phy,
> +	.disconnect_phy = phy_sfp_disconnect_phy,
>   };
>   
>   static int qca807x_probe(struct phy_device *phydev)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 2d477eb2809a..f7ef7ed6d5ce 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1762,6 +1762,8 @@ int phy_suspend(struct phy_device *phydev);
>   int phy_resume(struct phy_device *phydev);
>   int __phy_resume(struct phy_device *phydev);
>   int phy_loopback(struct phy_device *phydev, bool enable);
> +int phy_sfp_connect_phy(void *upstream, struct phy_device *phy);
> +void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy);
>   void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
>   void phy_sfp_detach(void *upstream, struct sfp_bus *bus);
>   int phy_sfp_probe(struct phy_device *phydev,

[PATCH net-next v17 04/14] net: sfp: Add helper to return the SFP bus name

[Maxime Chevallier]

Knowing the bus name is helpful when we want to expose the link topology
to userspace, add a helper to return the SFP bus name.

This call will always be made while holding the RTNL which ensures
that the SFP driver won't unbind from the device. The returned pointer
to the bus name will only be used while RTNL is held.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Suggested-by: "Russell King (Oracle)" <linux@armlinux.org.uk>
---
 drivers/net/phy/sfp-bus.c | 22 ++++++++++++++++++++++
 include/linux/sfp.h       |  6 ++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 56953e66bb7b..f13c00b5b449 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -722,6 +722,28 @@ void sfp_bus_del_upstream(struct sfp_bus *bus)
 }
 EXPORT_SYMBOL_GPL(sfp_bus_del_upstream);
 
+/**
+ * sfp_get_name() - Get the SFP device name
+ * @bus: a pointer to the &struct sfp_bus structure for the sfp module
+ *
+ * Gets the SFP device's name, if @bus has a registered socket. Callers must
+ * hold RTNL, and the returned name is only valid until RTNL is released.
+ *
+ * Returns:
+ *	- The name of the SFP device registered with sfp_register_socket()
+ *	- %NULL if no device was registered on @bus
+ */
+const char *sfp_get_name(struct sfp_bus *bus)
+{
+	ASSERT_RTNL();
+
+	if (bus->sfp_dev)
+		return dev_name(bus->sfp_dev);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(sfp_get_name);
+
 /* Socket driver entry points */
 int sfp_add_phy(struct sfp_bus *bus, struct phy_device *phydev)
 {
diff --git a/include/linux/sfp.h b/include/linux/sfp.h
index 54abb4d22b2e..60c65cea74f6 100644
--- a/include/linux/sfp.h
+++ b/include/linux/sfp.h
@@ -576,6 +576,7 @@ struct sfp_bus *sfp_bus_find_fwnode(const struct fwnode_handle *fwnode);
 int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
 			 const struct sfp_upstream_ops *ops);
 void sfp_bus_del_upstream(struct sfp_bus *bus);
+const char *sfp_get_name(struct sfp_bus *bus);
 #else
 static inline int sfp_parse_port(struct sfp_bus *bus,
 				 const struct sfp_eeprom_id *id,
@@ -654,6 +655,11 @@ static inline int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
 static inline void sfp_bus_del_upstream(struct sfp_bus *bus)
 {
 }
+
+static inline const char *sfp_get_name(struct sfp_bus *bus)
+{
+	return NULL;
+}
 #endif
 
 #endif
-- 
2.45.1

Re: [PATCH net-next v17 04/14] net: sfp: Add helper to return the SFP bus name

[LEROY Christophe]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> Knowing the bus name is helpful when we want to expose the link topology
> to userspace, add a helper to return the SFP bus name.
> 
> This call will always be made while holding the RTNL which ensures
> that the SFP driver won't unbind from the device. The returned pointer
> to the bus name will only be used while RTNL is held.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Suggested-by: "Russell King (Oracle)" <linux@armlinux.org.uk>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   drivers/net/phy/sfp-bus.c | 22 ++++++++++++++++++++++
>   include/linux/sfp.h       |  6 ++++++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> index 56953e66bb7b..f13c00b5b449 100644
> --- a/drivers/net/phy/sfp-bus.c
> +++ b/drivers/net/phy/sfp-bus.c
> @@ -722,6 +722,28 @@ void sfp_bus_del_upstream(struct sfp_bus *bus)
>   }
>   EXPORT_SYMBOL_GPL(sfp_bus_del_upstream);
>   
> +/**
> + * sfp_get_name() - Get the SFP device name
> + * @bus: a pointer to the &struct sfp_bus structure for the sfp module
> + *
> + * Gets the SFP device's name, if @bus has a registered socket. Callers must
> + * hold RTNL, and the returned name is only valid until RTNL is released.
> + *
> + * Returns:
> + *	- The name of the SFP device registered with sfp_register_socket()
> + *	- %NULL if no device was registered on @bus
> + */
> +const char *sfp_get_name(struct sfp_bus *bus)
> +{
> +	ASSERT_RTNL();
> +
> +	if (bus->sfp_dev)
> +		return dev_name(bus->sfp_dev);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(sfp_get_name);
> +
>   /* Socket driver entry points */
>   int sfp_add_phy(struct sfp_bus *bus, struct phy_device *phydev)
>   {
> diff --git a/include/linux/sfp.h b/include/linux/sfp.h
> index 54abb4d22b2e..60c65cea74f6 100644
> --- a/include/linux/sfp.h
> +++ b/include/linux/sfp.h
> @@ -576,6 +576,7 @@ struct sfp_bus *sfp_bus_find_fwnode(const struct fwnode_handle *fwnode);
>   int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
>   			 const struct sfp_upstream_ops *ops);
>   void sfp_bus_del_upstream(struct sfp_bus *bus);
> +const char *sfp_get_name(struct sfp_bus *bus);
>   #else
>   static inline int sfp_parse_port(struct sfp_bus *bus,
>   				 const struct sfp_eeprom_id *id,
> @@ -654,6 +655,11 @@ static inline int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
>   static inline void sfp_bus_del_upstream(struct sfp_bus *bus)
>   {
>   }
> +
> +static inline const char *sfp_get_name(struct sfp_bus *bus)
> +{
> +	return NULL;
> +}
>   #endif
>   
>   #endif

Re: [PATCH net-next v17 04/14] net: sfp: Add helper to return the SFP bus name

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> Knowing the bus name is helpful when we want to expose the link topology
> to userspace, add a helper to return the SFP bus name.
> 
> This call will always be made while holding the RTNL which ensures
> that the SFP driver won't unbind from the device. The returned pointer
> to the bus name will only be used while RTNL is held.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Suggested-by: "Russell King (Oracle)" <linux@armlinux.org.uk>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   drivers/net/phy/sfp-bus.c | 22 ++++++++++++++++++++++
>   include/linux/sfp.h       |  6 ++++++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> index 56953e66bb7b..f13c00b5b449 100644
> --- a/drivers/net/phy/sfp-bus.c
> +++ b/drivers/net/phy/sfp-bus.c
> @@ -722,6 +722,28 @@ void sfp_bus_del_upstream(struct sfp_bus *bus)
>   }
>   EXPORT_SYMBOL_GPL(sfp_bus_del_upstream);
>   
> +/**
> + * sfp_get_name() - Get the SFP device name
> + * @bus: a pointer to the &struct sfp_bus structure for the sfp module
> + *
> + * Gets the SFP device's name, if @bus has a registered socket. Callers must
> + * hold RTNL, and the returned name is only valid until RTNL is released.
> + *
> + * Returns:
> + *	- The name of the SFP device registered with sfp_register_socket()
> + *	- %NULL if no device was registered on @bus
> + */
> +const char *sfp_get_name(struct sfp_bus *bus)
> +{
> +	ASSERT_RTNL();
> +
> +	if (bus->sfp_dev)
> +		return dev_name(bus->sfp_dev);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(sfp_get_name);
> +
>   /* Socket driver entry points */
>   int sfp_add_phy(struct sfp_bus *bus, struct phy_device *phydev)
>   {
> diff --git a/include/linux/sfp.h b/include/linux/sfp.h
> index 54abb4d22b2e..60c65cea74f6 100644
> --- a/include/linux/sfp.h
> +++ b/include/linux/sfp.h
> @@ -576,6 +576,7 @@ struct sfp_bus *sfp_bus_find_fwnode(const struct fwnode_handle *fwnode);
>   int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
>   			 const struct sfp_upstream_ops *ops);
>   void sfp_bus_del_upstream(struct sfp_bus *bus);
> +const char *sfp_get_name(struct sfp_bus *bus);
>   #else
>   static inline int sfp_parse_port(struct sfp_bus *bus,
>   				 const struct sfp_eeprom_id *id,
> @@ -654,6 +655,11 @@ static inline int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,
>   static inline void sfp_bus_del_upstream(struct sfp_bus *bus)
>   {
>   }
> +
> +static inline const char *sfp_get_name(struct sfp_bus *bus)
> +{
> +	return NULL;
> +}
>   #endif
>   
>   #endif

[PATCH net-next v17 05/14] net: ethtool: Allow passing a phy index for some commands

[Maxime Chevallier]

Some netlink commands are target towards ethernet PHYs, to control some
of their features. As there's several such commands, add the ability to
pass a PHY index in the ethnl request, which will populate the generic
ethnl_req_info with the passed phy_index.

Add a helper that netlink command handlers need to use to grab the
targeted PHY from the req_info. This helper needs to hold rtnl_lock()
while interacting with the PHY, as it may be removed at any point.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/networking/ethtool-netlink.rst |  7 +++
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/netlink.c                        | 57 +++++++++++++++++++-
 net/ethtool/netlink.h                        | 28 ++++++++++
 4 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 3ab423b80e91..a6ea3716315f 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -57,6 +57,7 @@ Structure of this header is
   ``ETHTOOL_A_HEADER_DEV_INDEX``  u32     device ifindex
   ``ETHTOOL_A_HEADER_DEV_NAME``   string  device name
   ``ETHTOOL_A_HEADER_FLAGS``      u32     flags common for all requests
+  ``ETHTOOL_A_HEADER_PHY_INDEX``  u32     phy device index
   ==============================  ======  =============================
 
 ``ETHTOOL_A_HEADER_DEV_INDEX`` and ``ETHTOOL_A_HEADER_DEV_NAME`` identify the
@@ -81,6 +82,12 @@ the behaviour is backward compatible, i.e. requests from old clients not aware
 of the flag should be interpreted the way the client expects. A client must
 not set flags it does not understand.
 
+``ETHTOOL_A_HEADER_PHY_INDEX`` identifies the Ethernet PHY the message relates to.
+As there are numerous commands that are related to PHY configuration, and because
+there may be more than one PHY on the link, the PHY index can be passed in the
+request for the commands that needs it. It is, however, not mandatory, and if it
+is not passed for commands that target a PHY, the net_device.phydev pointer
+is used.
 
 Bit sets
 ========
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 6d5bdcc67631..c5af23139e63 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -134,6 +134,7 @@ enum {
 	ETHTOOL_A_HEADER_DEV_INDEX,		/* u32 */
 	ETHTOOL_A_HEADER_DEV_NAME,		/* string */
 	ETHTOOL_A_HEADER_FLAGS,			/* u32 - ETHTOOL_FLAG_* */
+	ETHTOOL_A_HEADER_PHY_INDEX,		/* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_HEADER_CNT,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index cb1eea00e349..7b95571a6efb 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -2,6 +2,7 @@
 
 #include <net/sock.h>
 #include <linux/ethtool_netlink.h>
+#include <linux/phy_link_topology.h>
 #include <linux/pm_runtime.h>
 #include "netlink.h"
 #include "module_fw.h"
@@ -31,6 +32,24 @@ const struct nla_policy ethnl_header_policy_stats[] = {
 							  ETHTOOL_FLAGS_STATS),
 };
 
+const struct nla_policy ethnl_header_policy_phy[] = {
+	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
+	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
+					    .len = ALTIFNAMSIZ - 1 },
+	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
+							  ETHTOOL_FLAGS_BASIC),
+	[ETHTOOL_A_HEADER_PHY_INDEX]		= NLA_POLICY_MIN(NLA_U32, 1),
+};
+
+const struct nla_policy ethnl_header_policy_phy_stats[] = {
+	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
+	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
+					    .len = ALTIFNAMSIZ - 1 },
+	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
+							  ETHTOOL_FLAGS_STATS),
+	[ETHTOOL_A_HEADER_PHY_INDEX]		= NLA_POLICY_MIN(NLA_U32, 1),
+};
+
 int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid,
 			enum ethnl_sock_type type)
 {
@@ -119,7 +138,7 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 			       const struct nlattr *header, struct net *net,
 			       struct netlink_ext_ack *extack, bool require_dev)
 {
-	struct nlattr *tb[ARRAY_SIZE(ethnl_header_policy)];
+	struct nlattr *tb[ARRAY_SIZE(ethnl_header_policy_phy)];
 	const struct nlattr *devname_attr;
 	struct net_device *dev = NULL;
 	u32 flags = 0;
@@ -134,7 +153,7 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 	/* No validation here, command policy should have a nested policy set
 	 * for the header, therefore validation should have already been done.
 	 */
-	ret = nla_parse_nested(tb, ARRAY_SIZE(ethnl_header_policy) - 1, header,
+	ret = nla_parse_nested(tb, ARRAY_SIZE(ethnl_header_policy_phy) - 1, header,
 			       NULL, extack);
 	if (ret < 0)
 		return ret;
@@ -175,11 +194,45 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 		return -EINVAL;
 	}
 
+	if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
+		if (dev) {
+			req_info->phy_index = nla_get_u32(tb[ETHTOOL_A_HEADER_PHY_INDEX]);
+		} else {
+			NL_SET_ERR_MSG_ATTR(extack, header,
+					    "phy_index set without a netdev");
+			return -EINVAL;
+		}
+	}
+
 	req_info->dev = dev;
 	req_info->flags = flags;
 	return 0;
 }
 
+struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
+					const struct nlattr *header,
+					struct netlink_ext_ack *extack)
+{
+	struct phy_device *phydev;
+
+	ASSERT_RTNL();
+
+	if (!req_info->dev)
+		return NULL;
+
+	if (!req_info->phy_index)
+		return req_info->dev->phydev;
+
+	phydev = phy_link_topo_get_phy(req_info->dev, req_info->phy_index);
+	if (!phydev) {
+		NL_SET_ERR_MSG_ATTR(extack, header,
+				    "no phy matching phyindex");
+		return ERR_PTR(-ENODEV);
+	}
+
+	return phydev;
+}
+
 /**
  * ethnl_fill_reply_header() - Put common header into a reply message
  * @skb:      skb with the message
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 46ec273a87c5..4db16048f8d4 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -251,6 +251,9 @@ static inline unsigned int ethnl_reply_header_size(void)
  * @dev:   network device the request is for (may be null)
  * @dev_tracker: refcount tracker for @dev reference
  * @flags: request flags common for all request types
+ * @phy_index: phy_device index connected to @dev this request is for. Can be
+ *	       0 if the request doesn't target a phy, or if the @dev's attached
+ *	       phy is targeted.
  *
  * This is a common base for request specific structures holding data from
  * parsed userspace request. These always embed struct ethnl_req_info at
@@ -260,6 +263,7 @@ struct ethnl_req_info {
 	struct net_device	*dev;
 	netdevice_tracker	dev_tracker;
 	u32			flags;
+	u32			phy_index;
 };
 
 static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
@@ -267,6 +271,27 @@ static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
 	netdev_put(req_info->dev, &req_info->dev_tracker);
 }
 
+/**
+ * ethnl_req_get_phydev() - Gets the phy_device targeted by this request,
+ *			    if any. Must be called under rntl_lock().
+ * @req_info:	The ethnl request to get the phy from.
+ * @header:	The netlink header, used for error reporting.
+ * @extack:	The netlink extended ACK, for error reporting.
+ *
+ * The caller must hold RTNL, until it's done interacting with the returned
+ * phy_device.
+ *
+ * Return: A phy_device pointer corresponding either to the passed phy_index
+ *	   if one is provided. If not, the phy_device attached to the
+ *	   net_device targeted by this request is returned. If there's no
+ *	   targeted net_device, or no phy_device is attached, NULL is
+ *	   returned. If the provided phy_index is invalid, an error pointer
+ *	   is returned.
+ */
+struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
+					const struct nlattr *header,
+					struct netlink_ext_ack *extack);
+
 /**
  * struct ethnl_reply_data - base type of reply data for GET requests
  * @dev:       device for current reply message; in single shot requests it is
@@ -409,9 +434,12 @@ extern const struct ethnl_request_ops ethnl_rss_request_ops;
 extern const struct ethnl_request_ops ethnl_plca_cfg_request_ops;
 extern const struct ethnl_request_ops ethnl_plca_status_request_ops;
 extern const struct ethnl_request_ops ethnl_mm_request_ops;
+extern const struct ethnl_request_ops ethnl_phy_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];
+extern const struct nla_policy ethnl_header_policy_phy[ETHTOOL_A_HEADER_PHY_INDEX + 1];
+extern const struct nla_policy ethnl_header_policy_phy_stats[ETHTOOL_A_HEADER_PHY_INDEX + 1];
 extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_COUNTS_ONLY + 1];
 extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_HEADER + 1];
 extern const struct nla_policy ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL + 1];
-- 
2.45.1

Re: [PATCH net-next v17 05/14] net: ethtool: Allow passing a phy index for some commands

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> Some netlink commands are target towards ethernet PHYs, to control some
> of their features. As there's several such commands, add the ability to
> pass a PHY index in the ethnl request, which will populate the generic
> ethnl_req_info with the passed phy_index.
> 
> Add a helper that netlink command handlers need to use to grab the
> targeted PHY from the req_info. This helper needs to hold rtnl_lock()
> while interacting with the PHY, as it may be removed at any point.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   Documentation/networking/ethtool-netlink.rst |  7 +++
>   include/uapi/linux/ethtool_netlink.h         |  1 +
>   net/ethtool/netlink.c                        | 57 +++++++++++++++++++-
>   net/ethtool/netlink.h                        | 28 ++++++++++
>   4 files changed, 91 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 3ab423b80e91..a6ea3716315f 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -57,6 +57,7 @@ Structure of this header is
>     ``ETHTOOL_A_HEADER_DEV_INDEX``  u32     device ifindex
>     ``ETHTOOL_A_HEADER_DEV_NAME``   string  device name
>     ``ETHTOOL_A_HEADER_FLAGS``      u32     flags common for all requests
> +  ``ETHTOOL_A_HEADER_PHY_INDEX``  u32     phy device index
>     ==============================  ======  =============================
>   
>   ``ETHTOOL_A_HEADER_DEV_INDEX`` and ``ETHTOOL_A_HEADER_DEV_NAME`` identify the
> @@ -81,6 +82,12 @@ the behaviour is backward compatible, i.e. requests from old clients not aware
>   of the flag should be interpreted the way the client expects. A client must
>   not set flags it does not understand.
>   
> +``ETHTOOL_A_HEADER_PHY_INDEX`` identifies the Ethernet PHY the message relates to.
> +As there are numerous commands that are related to PHY configuration, and because
> +there may be more than one PHY on the link, the PHY index can be passed in the
> +request for the commands that needs it. It is, however, not mandatory, and if it
> +is not passed for commands that target a PHY, the net_device.phydev pointer
> +is used.
>   
>   Bit sets
>   ========
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 6d5bdcc67631..c5af23139e63 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -134,6 +134,7 @@ enum {
>   	ETHTOOL_A_HEADER_DEV_INDEX,		/* u32 */
>   	ETHTOOL_A_HEADER_DEV_NAME,		/* string */
>   	ETHTOOL_A_HEADER_FLAGS,			/* u32 - ETHTOOL_FLAG_* */
> +	ETHTOOL_A_HEADER_PHY_INDEX,		/* u32 */
>   
>   	/* add new constants above here */
>   	__ETHTOOL_A_HEADER_CNT,
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index cb1eea00e349..7b95571a6efb 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -2,6 +2,7 @@
>   
>   #include <net/sock.h>
>   #include <linux/ethtool_netlink.h>
> +#include <linux/phy_link_topology.h>
>   #include <linux/pm_runtime.h>
>   #include "netlink.h"
>   #include "module_fw.h"
> @@ -31,6 +32,24 @@ const struct nla_policy ethnl_header_policy_stats[] = {
>   							  ETHTOOL_FLAGS_STATS),
>   };
>   
> +const struct nla_policy ethnl_header_policy_phy[] = {
> +	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
> +	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
> +					    .len = ALTIFNAMSIZ - 1 },
> +	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
> +							  ETHTOOL_FLAGS_BASIC),
> +	[ETHTOOL_A_HEADER_PHY_INDEX]		= NLA_POLICY_MIN(NLA_U32, 1),
> +};
> +
> +const struct nla_policy ethnl_header_policy_phy_stats[] = {
> +	[ETHTOOL_A_HEADER_DEV_INDEX]	= { .type = NLA_U32 },
> +	[ETHTOOL_A_HEADER_DEV_NAME]	= { .type = NLA_NUL_STRING,
> +					    .len = ALTIFNAMSIZ - 1 },
> +	[ETHTOOL_A_HEADER_FLAGS]	= NLA_POLICY_MASK(NLA_U32,
> +							  ETHTOOL_FLAGS_STATS),
> +	[ETHTOOL_A_HEADER_PHY_INDEX]		= NLA_POLICY_MIN(NLA_U32, 1),
> +};
> +
>   int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid,
>   			enum ethnl_sock_type type)
>   {
> @@ -119,7 +138,7 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
>   			       const struct nlattr *header, struct net *net,
>   			       struct netlink_ext_ack *extack, bool require_dev)
>   {
> -	struct nlattr *tb[ARRAY_SIZE(ethnl_header_policy)];
> +	struct nlattr *tb[ARRAY_SIZE(ethnl_header_policy_phy)];
>   	const struct nlattr *devname_attr;
>   	struct net_device *dev = NULL;
>   	u32 flags = 0;
> @@ -134,7 +153,7 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
>   	/* No validation here, command policy should have a nested policy set
>   	 * for the header, therefore validation should have already been done.
>   	 */
> -	ret = nla_parse_nested(tb, ARRAY_SIZE(ethnl_header_policy) - 1, header,
> +	ret = nla_parse_nested(tb, ARRAY_SIZE(ethnl_header_policy_phy) - 1, header,
>   			       NULL, extack);
>   	if (ret < 0)
>   		return ret;
> @@ -175,11 +194,45 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
>   		return -EINVAL;
>   	}
>   
> +	if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> +		if (dev) {
> +			req_info->phy_index = nla_get_u32(tb[ETHTOOL_A_HEADER_PHY_INDEX]);
> +		} else {
> +			NL_SET_ERR_MSG_ATTR(extack, header,
> +					    "phy_index set without a netdev");
> +			return -EINVAL;
> +		}
> +	}
> +
>   	req_info->dev = dev;
>   	req_info->flags = flags;
>   	return 0;
>   }
>   
> +struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
> +					const struct nlattr *header,
> +					struct netlink_ext_ack *extack)
> +{
> +	struct phy_device *phydev;
> +
> +	ASSERT_RTNL();
> +
> +	if (!req_info->dev)
> +		return NULL;
> +
> +	if (!req_info->phy_index)
> +		return req_info->dev->phydev;
> +
> +	phydev = phy_link_topo_get_phy(req_info->dev, req_info->phy_index);
> +	if (!phydev) {
> +		NL_SET_ERR_MSG_ATTR(extack, header,
> +				    "no phy matching phyindex");
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	return phydev;
> +}
> +
>   /**
>    * ethnl_fill_reply_header() - Put common header into a reply message
>    * @skb:      skb with the message
> diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
> index 46ec273a87c5..4db16048f8d4 100644
> --- a/net/ethtool/netlink.h
> +++ b/net/ethtool/netlink.h
> @@ -251,6 +251,9 @@ static inline unsigned int ethnl_reply_header_size(void)
>    * @dev:   network device the request is for (may be null)
>    * @dev_tracker: refcount tracker for @dev reference
>    * @flags: request flags common for all request types
> + * @phy_index: phy_device index connected to @dev this request is for. Can be
> + *	       0 if the request doesn't target a phy, or if the @dev's attached
> + *	       phy is targeted.
>    *
>    * This is a common base for request specific structures holding data from
>    * parsed userspace request. These always embed struct ethnl_req_info at
> @@ -260,6 +263,7 @@ struct ethnl_req_info {
>   	struct net_device	*dev;
>   	netdevice_tracker	dev_tracker;
>   	u32			flags;
> +	u32			phy_index;
>   };
>   
>   static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
> @@ -267,6 +271,27 @@ static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
>   	netdev_put(req_info->dev, &req_info->dev_tracker);
>   }
>   
> +/**
> + * ethnl_req_get_phydev() - Gets the phy_device targeted by this request,
> + *			    if any. Must be called under rntl_lock().
> + * @req_info:	The ethnl request to get the phy from.
> + * @header:	The netlink header, used for error reporting.
> + * @extack:	The netlink extended ACK, for error reporting.
> + *
> + * The caller must hold RTNL, until it's done interacting with the returned
> + * phy_device.
> + *
> + * Return: A phy_device pointer corresponding either to the passed phy_index
> + *	   if one is provided. If not, the phy_device attached to the
> + *	   net_device targeted by this request is returned. If there's no
> + *	   targeted net_device, or no phy_device is attached, NULL is
> + *	   returned. If the provided phy_index is invalid, an error pointer
> + *	   is returned.
> + */
> +struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
> +					const struct nlattr *header,
> +					struct netlink_ext_ack *extack);
> +
>   /**
>    * struct ethnl_reply_data - base type of reply data for GET requests
>    * @dev:       device for current reply message; in single shot requests it is
> @@ -409,9 +434,12 @@ extern const struct ethnl_request_ops ethnl_rss_request_ops;
>   extern const struct ethnl_request_ops ethnl_plca_cfg_request_ops;
>   extern const struct ethnl_request_ops ethnl_plca_status_request_ops;
>   extern const struct ethnl_request_ops ethnl_mm_request_ops;
> +extern const struct ethnl_request_ops ethnl_phy_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];
> +extern const struct nla_policy ethnl_header_policy_phy[ETHTOOL_A_HEADER_PHY_INDEX + 1];
> +extern const struct nla_policy ethnl_header_policy_phy_stats[ETHTOOL_A_HEADER_PHY_INDEX + 1];
>   extern const struct nla_policy ethnl_strset_get_policy[ETHTOOL_A_STRSET_COUNTS_ONLY + 1];
>   extern const struct nla_policy ethnl_linkinfo_get_policy[ETHTOOL_A_LINKINFO_HEADER + 1];
>   extern const struct nla_policy ethnl_linkinfo_set_policy[ETHTOOL_A_LINKINFO_TP_MDIX_CTRL + 1];

[PATCH net-next v17 06/14] netlink: specs: add phy-index as a header parameter

[Maxime Chevallier]

Update the spec to take the newly introduced phy-index as a generic
request parameter.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/netlink/specs/ethtool.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 495e35fcfb21..586f1da8eb7b 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -54,6 +54,9 @@ attribute-sets:
         name: flags
         type: u32
         enum: header-flags
+      -
+        name: phy-index
+        type: u32
 
   -
     name: bitset-bit
-- 
2.45.1

Re: [PATCH net-next v17 06/14] netlink: specs: add phy-index as a header parameter

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> Update the spec to take the newly introduced phy-index as a generic
> request parameter.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   Documentation/netlink/specs/ethtool.yaml | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index 495e35fcfb21..586f1da8eb7b 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -54,6 +54,9 @@ attribute-sets:
>           name: flags
>           type: u32
>           enum: header-flags
> +      -
> +        name: phy-index
> +        type: u32
>   
>     -
>       name: bitset-bit

[PATCH net-next v17 07/14] net: ethtool: Introduce a command to list PHYs on an interface

[Maxime Chevallier]

As we have the ability to track the PHYs connected to a net_device
through the link_topology, we can expose this list to userspace. This
allows userspace to use these identifiers for phy-specific commands and
take the decision of which PHY to target by knowing the link topology.

Add PHY_GET and PHY_DUMP, which can be a filtered DUMP operation to list
devices on only one interface.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/networking/ethtool-netlink.rst |  41 +++
 include/uapi/linux/ethtool_netlink.h         |  19 ++
 net/ethtool/Makefile                         |   3 +-
 net/ethtool/netlink.c                        |   9 +
 net/ethtool/netlink.h                        |   5 +
 net/ethtool/phy.c                            | 308 +++++++++++++++++++
 6 files changed, 384 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/phy.c

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index a6ea3716315f..d9f0c0dba1e5 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -2182,6 +2182,46 @@ string.
 The ``ETHTOOL_A_MODULE_FW_FLASH_DONE`` and ``ETHTOOL_A_MODULE_FW_FLASH_TOTAL``
 attributes encode the completed and total amount of work, respectively.
 
+PHY_GET
+=======
+
+Retrieve information about a given Ethernet PHY sitting on the link. The DO
+operation returns all available information about dev->phydev. User can also
+specify a PHY_INDEX, in which case the DO request returns information about that
+specific PHY.
+As there can be more than one PHY, the DUMP operation can be used to list the PHYs
+present on a given interface, by passing an interface index or name in
+the dump request.
+
+Request contents:
+
+  ====================================  ======  ==========================
+  ``ETHTOOL_A_PHY_HEADER``              nested  request header
+  ====================================  ======  ==========================
+
+Kernel response contents:
+
+  ===================================== ======  ===============================
+  ``ETHTOOL_A_PHY_HEADER``              nested  request header
+  ``ETHTOOL_A_PHY_INDEX``               u32     the phy's unique index, that can
+                                                be used for phy-specific
+                                                requests
+  ``ETHTOOL_A_PHY_DRVNAME``             string  the phy driver name
+  ``ETHTOOL_A_PHY_NAME``                string  the phy device name
+  ``ETHTOOL_A_PHY_UPSTREAM_TYPE``       u32     the type of device this phy is
+                                                connected to
+  ``ETHTOOL_A_PHY_UPSTREAM_INDEX``      u32     the PHY index of the upstream
+                                                PHY
+  ``ETHTOOL_A_PHY_UPSTREAM_SFP_NAME``   string  if this PHY is connected to
+                                                its parent PHY through an SFP
+                                                bus, the name of this sfp bus
+  ``ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME`` string  if the phy controls an sfp bus,
+                                                the name of the sfp bus
+  ===================================== ======  ===============================
+
+When ``ETHTOOL_A_PHY_UPSTREAM_TYPE`` is PHY_UPSTREAM_PHY, the PHY's parent is
+another PHY.
+
 Request translation
 ===================
 
@@ -2289,4 +2329,5 @@ are netlink only.
   n/a                                 ``ETHTOOL_MSG_MM_GET``
   n/a                                 ``ETHTOOL_MSG_MM_SET``
   n/a                                 ``ETHTOOL_MSG_MODULE_FW_FLASH_ACT``
+  n/a                                 ``ETHTOOL_MSG_PHY_GET``
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index c5af23139e63..6ad32843b32a 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -58,6 +58,7 @@ enum {
 	ETHTOOL_MSG_MM_GET,
 	ETHTOOL_MSG_MM_SET,
 	ETHTOOL_MSG_MODULE_FW_FLASH_ACT,
+	ETHTOOL_MSG_PHY_GET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -111,6 +112,8 @@ enum {
 	ETHTOOL_MSG_MM_GET_REPLY,
 	ETHTOOL_MSG_MM_NTF,
 	ETHTOOL_MSG_MODULE_FW_FLASH_NTF,
+	ETHTOOL_MSG_PHY_GET_REPLY,
+	ETHTOOL_MSG_PHY_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -1050,6 +1053,22 @@ enum {
 	ETHTOOL_A_MODULE_FW_FLASH_MAX = (__ETHTOOL_A_MODULE_FW_FLASH_CNT - 1)
 };
 
+enum {
+	ETHTOOL_A_PHY_UNSPEC,
+	ETHTOOL_A_PHY_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_PHY_INDEX,			/* u32 */
+	ETHTOOL_A_PHY_DRVNAME,			/* string */
+	ETHTOOL_A_PHY_NAME,			/* string */
+	ETHTOOL_A_PHY_UPSTREAM_TYPE,		/* u32 */
+	ETHTOOL_A_PHY_UPSTREAM_INDEX,		/* u32 */
+	ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,	/* string */
+	ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,	/* string */
+
+	/* add new constants above here */
+	__ETHTOOL_A_PHY_CNT,
+	ETHTOOL_A_PHY_MAX = (__ETHTOOL_A_PHY_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 9a190635fe95..9b540644ba31 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,5 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.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 mm.o
+		   module.o cmis_fw_update.o cmis_cdb.o pse-pd.o plca.o mm.o \
+		   phy.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 7b95571a6efb..153d34dbc48a 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1232,6 +1232,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy	= ethnl_module_fw_flash_act_policy,
 		.maxattr = ARRAY_SIZE(ethnl_module_fw_flash_act_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PHY_GET,
+		.doit	= ethnl_phy_doit,
+		.start	= ethnl_phy_start,
+		.dumpit	= ethnl_phy_dumpit,
+		.done	= ethnl_phy_done,
+		.policy = ethnl_phy_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_phy_get_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 4db16048f8d4..4a702dc853bf 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -484,6 +484,7 @@ extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADE
 extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
 extern const struct nla_policy ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD + 1];
+extern const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 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);
@@ -492,6 +493,10 @@ int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_start(struct netlink_callback *cb);
 int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info);
+int ethnl_phy_start(struct netlink_callback *cb);
+int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info);
+int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
+int ethnl_phy_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/phy.c b/net/ethtool/phy.c
new file mode 100644
index 000000000000..560dd039c662
--- /dev/null
+++ b/net/ethtool/phy.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2023 Bootlin
+ *
+ */
+#include "common.h"
+#include "netlink.h"
+
+#include <linux/phy.h>
+#include <linux/phy_link_topology.h>
+#include <linux/sfp.h>
+
+struct phy_req_info {
+	struct ethnl_req_info		base;
+	struct phy_device_node		*pdn;
+};
+
+#define PHY_REQINFO(__req_base) \
+	container_of(__req_base, struct phy_req_info, base)
+
+const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1] = {
+	[ETHTOOL_A_PHY_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+};
+
+/* Caller holds rtnl */
+static ssize_t
+ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
+		     struct netlink_ext_ack *extack)
+{
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct phy_device_node *pdn = req_info->pdn;
+	struct phy_device *phydev = pdn->phy;
+	size_t size = 0;
+
+	ASSERT_RTNL();
+
+	/* ETHTOOL_A_PHY_INDEX */
+	size += nla_total_size(sizeof(u32));
+
+	/* ETHTOOL_A_DRVNAME */
+	if (phydev->drv)
+		size += nla_total_size(strlen(phydev->drv->name) + 1);
+
+	/* ETHTOOL_A_NAME */
+	size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)) + 1);
+
+	/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
+	size += nla_total_size(sizeof(u32));
+
+	if (phy_on_sfp(phydev)) {
+		const char *upstream_sfp_name = sfp_get_name(pdn->parent_sfp_bus);
+
+		/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
+		if (upstream_sfp_name)
+			size += nla_total_size(strlen(upstream_sfp_name) + 1);
+
+		/* ETHTOOL_A_PHY_UPSTREAM_INDEX */
+		size += nla_total_size(sizeof(u32));
+	}
+
+	/* ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME */
+	if (phydev->sfp_bus) {
+		const char *sfp_name = sfp_get_name(phydev->sfp_bus);
+
+		if (sfp_name)
+			size += nla_total_size(strlen(sfp_name) + 1);
+	}
+
+	return size;
+}
+
+static int
+ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
+{
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct phy_device_node *pdn = req_info->pdn;
+	struct phy_device *phydev = pdn->phy;
+	enum phy_upstream ptype;
+
+	ptype = pdn->upstream_type;
+
+	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, phydev->phyindex) ||
+	    nla_put_string(skb, ETHTOOL_A_PHY_NAME, dev_name(&phydev->mdio.dev)) ||
+	    nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, ptype))
+		return -EMSGSIZE;
+
+	if (phydev->drv &&
+	    nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name))
+		return -EMSGSIZE;
+
+	if (ptype == PHY_UPSTREAM_PHY) {
+		struct phy_device *upstream = pdn->upstream.phydev;
+		const char *sfp_upstream_name;
+
+		/* Parent index */
+		if (nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX, upstream->phyindex))
+			return -EMSGSIZE;
+
+		if (pdn->parent_sfp_bus) {
+			sfp_upstream_name = sfp_get_name(pdn->parent_sfp_bus);
+			if (sfp_upstream_name &&
+			    nla_put_string(skb, ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
+					   sfp_upstream_name))
+				return -EMSGSIZE;
+		}
+	}
+
+	if (phydev->sfp_bus) {
+		const char *sfp_name = sfp_get_name(phydev->sfp_bus);
+
+		if (sfp_name &&
+		    nla_put_string(skb, ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,
+				   sfp_name))
+			return -EMSGSIZE;
+	}
+
+	return 0;
+}
+
+static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
+				   struct nlattr **tb,
+				   struct netlink_ext_ack *extack)
+{
+	struct phy_link_topology *topo = req_base->dev->link_topo;
+	struct phy_req_info *req_info = PHY_REQINFO(req_base);
+	struct phy_device *phydev;
+
+	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PHY_HEADER],
+				      extack);
+	if (!phydev)
+		return 0;
+
+	if (IS_ERR(phydev))
+		return PTR_ERR(phydev);
+
+	if (!topo)
+		return 0;
+
+	req_info->pdn = xa_load(&topo->phys, phydev->phyindex);
+
+	return 0;
+}
+
+int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct phy_req_info req_info = {};
+	struct nlattr **tb = info->attrs;
+	struct sk_buff *rskb;
+	void *reply_payload;
+	int reply_len;
+	int ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info.base,
+					 tb[ETHTOOL_A_PHY_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+
+	rtnl_lock();
+
+	ret = ethnl_phy_parse_request(&req_info.base, tb, info->extack);
+	if (ret < 0)
+		goto err_unlock_rtnl;
+
+	/* No PHY, return early */
+	if (!req_info.pdn->phy)
+		goto err_unlock_rtnl;
+
+	ret = ethnl_phy_reply_size(&req_info.base, info->extack);
+	if (ret < 0)
+		goto err_unlock_rtnl;
+	reply_len = ret + ethnl_reply_header_size();
+
+	rskb = ethnl_reply_init(reply_len, req_info.base.dev,
+				ETHTOOL_MSG_PHY_GET_REPLY,
+				ETHTOOL_A_PHY_HEADER,
+				info, &reply_payload);
+	if (!rskb) {
+		ret = -ENOMEM;
+		goto err_unlock_rtnl;
+	}
+
+	ret = ethnl_phy_fill_reply(&req_info.base, rskb);
+	if (ret)
+		goto err_free_msg;
+
+	rtnl_unlock();
+	ethnl_parse_header_dev_put(&req_info.base);
+	genlmsg_end(rskb, reply_payload);
+
+	return genlmsg_reply(rskb, info);
+
+err_free_msg:
+	nlmsg_free(rskb);
+err_unlock_rtnl:
+	rtnl_unlock();
+	ethnl_parse_header_dev_put(&req_info.base);
+	return ret;
+}
+
+struct ethnl_phy_dump_ctx {
+	struct phy_req_info	*phy_req_info;
+	unsigned long ifindex;
+	unsigned long phy_index;
+};
+
+int ethnl_phy_start(struct netlink_callback *cb)
+{
+	const struct genl_info *info = genl_info_dump(cb);
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	int ret;
+
+	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
+
+	ctx->phy_req_info = kzalloc(sizeof(*ctx->phy_req_info), GFP_KERNEL);
+	if (!ctx->phy_req_info)
+		return -ENOMEM;
+
+	ret = ethnl_parse_header_dev_get(&ctx->phy_req_info->base,
+					 info->attrs[ETHTOOL_A_PHY_HEADER],
+					 sock_net(cb->skb->sk), cb->extack,
+					 false);
+	ctx->ifindex = 0;
+	ctx->phy_index = 0;
+
+	if (ret)
+		kfree(ctx->phy_req_info);
+
+	return ret;
+}
+
+int ethnl_phy_done(struct netlink_callback *cb)
+{
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+
+	if (ctx->phy_req_info->base.dev)
+		ethnl_parse_header_dev_put(&ctx->phy_req_info->base);
+
+	kfree(ctx->phy_req_info);
+
+	return 0;
+}
+
+static int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
+				  struct netlink_callback *cb)
+{
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	struct phy_req_info *pri = ctx->phy_req_info;
+	struct phy_device_node *pdn;
+	int ret = 0;
+	void *ehdr;
+
+	pri->base.dev = dev;
+
+	if (!dev->link_topo)
+		return 0;
+
+	xa_for_each_start(&dev->link_topo->phys, ctx->phy_index, pdn, ctx->phy_index) {
+		ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_PHY_GET_REPLY);
+		if (!ehdr) {
+			ret = -EMSGSIZE;
+			break;
+		}
+
+		ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_PHY_HEADER);
+		if (ret < 0) {
+			genlmsg_cancel(skb, ehdr);
+			break;
+		}
+
+		pri->pdn = pdn;
+		ret = ethnl_phy_fill_reply(&pri->base, skb);
+		if (ret < 0) {
+			genlmsg_cancel(skb, ehdr);
+			break;
+		}
+
+		genlmsg_end(skb, ehdr);
+	}
+
+	return ret;
+}
+
+int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev;
+	int ret = 0;
+
+	rtnl_lock();
+
+	if (ctx->phy_req_info->base.dev) {
+		ret = ethnl_phy_dump_one_dev(skb, ctx->phy_req_info->base.dev, cb);
+	} else {
+		for_each_netdev_dump(net, dev, ctx->ifindex) {
+			ret = ethnl_phy_dump_one_dev(skb, dev, cb);
+			if (ret)
+				break;
+
+			ctx->phy_index = 0;
+		}
+	}
+	rtnl_unlock();
+
+	return ret;
+}
-- 
2.45.1

Re: [PATCH net-next v17 07/14] net: ethtool: Introduce a command to list PHYs on an interface

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> As we have the ability to track the PHYs connected to a net_device
> through the link_topology, we can expose this list to userspace. This
> allows userspace to use these identifiers for phy-specific commands and
> take the decision of which PHY to target by knowing the link topology.
> 
> Add PHY_GET and PHY_DUMP, which can be a filtered DUMP operation to list
> devices on only one interface.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   Documentation/networking/ethtool-netlink.rst |  41 +++
>   include/uapi/linux/ethtool_netlink.h         |  19 ++
>   net/ethtool/Makefile                         |   3 +-
>   net/ethtool/netlink.c                        |   9 +
>   net/ethtool/netlink.h                        |   5 +
>   net/ethtool/phy.c                            | 308 +++++++++++++++++++
>   6 files changed, 384 insertions(+), 1 deletion(-)
>   create mode 100644 net/ethtool/phy.c
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index a6ea3716315f..d9f0c0dba1e5 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -2182,6 +2182,46 @@ string.
>   The ``ETHTOOL_A_MODULE_FW_FLASH_DONE`` and ``ETHTOOL_A_MODULE_FW_FLASH_TOTAL``
>   attributes encode the completed and total amount of work, respectively.
>   
> +PHY_GET
> +=======
> +
> +Retrieve information about a given Ethernet PHY sitting on the link. The DO
> +operation returns all available information about dev->phydev. User can also
> +specify a PHY_INDEX, in which case the DO request returns information about that
> +specific PHY.
> +As there can be more than one PHY, the DUMP operation can be used to list the PHYs
> +present on a given interface, by passing an interface index or name in
> +the dump request.
> +
> +Request contents:
> +
> +  ====================================  ======  ==========================
> +  ``ETHTOOL_A_PHY_HEADER``              nested  request header
> +  ====================================  ======  ==========================
> +
> +Kernel response contents:
> +
> +  ===================================== ======  ===============================
> +  ``ETHTOOL_A_PHY_HEADER``              nested  request header
> +  ``ETHTOOL_A_PHY_INDEX``               u32     the phy's unique index, that can
> +                                                be used for phy-specific
> +                                                requests
> +  ``ETHTOOL_A_PHY_DRVNAME``             string  the phy driver name
> +  ``ETHTOOL_A_PHY_NAME``                string  the phy device name
> +  ``ETHTOOL_A_PHY_UPSTREAM_TYPE``       u32     the type of device this phy is
> +                                                connected to
> +  ``ETHTOOL_A_PHY_UPSTREAM_INDEX``      u32     the PHY index of the upstream
> +                                                PHY
> +  ``ETHTOOL_A_PHY_UPSTREAM_SFP_NAME``   string  if this PHY is connected to
> +                                                its parent PHY through an SFP
> +                                                bus, the name of this sfp bus
> +  ``ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME`` string  if the phy controls an sfp bus,
> +                                                the name of the sfp bus
> +  ===================================== ======  ===============================
> +
> +When ``ETHTOOL_A_PHY_UPSTREAM_TYPE`` is PHY_UPSTREAM_PHY, the PHY's parent is
> +another PHY.
> +
>   Request translation
>   ===================
>   
> @@ -2289,4 +2329,5 @@ are netlink only.
>     n/a                                 ``ETHTOOL_MSG_MM_GET``
>     n/a                                 ``ETHTOOL_MSG_MM_SET``
>     n/a                                 ``ETHTOOL_MSG_MODULE_FW_FLASH_ACT``
> +  n/a                                 ``ETHTOOL_MSG_PHY_GET``
>     =================================== =====================================
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index c5af23139e63..6ad32843b32a 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -58,6 +58,7 @@ enum {
>   	ETHTOOL_MSG_MM_GET,
>   	ETHTOOL_MSG_MM_SET,
>   	ETHTOOL_MSG_MODULE_FW_FLASH_ACT,
> +	ETHTOOL_MSG_PHY_GET,
>   
>   	/* add new constants above here */
>   	__ETHTOOL_MSG_USER_CNT,
> @@ -111,6 +112,8 @@ enum {
>   	ETHTOOL_MSG_MM_GET_REPLY,
>   	ETHTOOL_MSG_MM_NTF,
>   	ETHTOOL_MSG_MODULE_FW_FLASH_NTF,
> +	ETHTOOL_MSG_PHY_GET_REPLY,
> +	ETHTOOL_MSG_PHY_NTF,
>   
>   	/* add new constants above here */
>   	__ETHTOOL_MSG_KERNEL_CNT,
> @@ -1050,6 +1053,22 @@ enum {
>   	ETHTOOL_A_MODULE_FW_FLASH_MAX = (__ETHTOOL_A_MODULE_FW_FLASH_CNT - 1)
>   };
>   
> +enum {
> +	ETHTOOL_A_PHY_UNSPEC,
> +	ETHTOOL_A_PHY_HEADER,			/* nest - _A_HEADER_* */
> +	ETHTOOL_A_PHY_INDEX,			/* u32 */
> +	ETHTOOL_A_PHY_DRVNAME,			/* string */
> +	ETHTOOL_A_PHY_NAME,			/* string */
> +	ETHTOOL_A_PHY_UPSTREAM_TYPE,		/* u32 */
> +	ETHTOOL_A_PHY_UPSTREAM_INDEX,		/* u32 */
> +	ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,	/* string */
> +	ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,	/* string */
> +
> +	/* add new constants above here */
> +	__ETHTOOL_A_PHY_CNT,
> +	ETHTOOL_A_PHY_MAX = (__ETHTOOL_A_PHY_CNT - 1)
> +};
> +
>   /* generic netlink info */
>   #define ETHTOOL_GENL_NAME "ethtool"
>   #define ETHTOOL_GENL_VERSION 1
> diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
> index 9a190635fe95..9b540644ba31 100644
> --- a/net/ethtool/Makefile
> +++ b/net/ethtool/Makefile
> @@ -8,4 +8,5 @@ ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
>   		   linkstate.o debug.o wol.o features.o privflags.o rings.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 mm.o
> +		   module.o cmis_fw_update.o cmis_cdb.o pse-pd.o plca.o mm.o \
> +		   phy.o
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 7b95571a6efb..153d34dbc48a 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -1232,6 +1232,15 @@ static const struct genl_ops ethtool_genl_ops[] = {
>   		.policy	= ethnl_module_fw_flash_act_policy,
>   		.maxattr = ARRAY_SIZE(ethnl_module_fw_flash_act_policy) - 1,
>   	},
> +	{
> +		.cmd	= ETHTOOL_MSG_PHY_GET,
> +		.doit	= ethnl_phy_doit,
> +		.start	= ethnl_phy_start,
> +		.dumpit	= ethnl_phy_dumpit,
> +		.done	= ethnl_phy_done,
> +		.policy = ethnl_phy_get_policy,
> +		.maxattr = ARRAY_SIZE(ethnl_phy_get_policy) - 1,
> +	},
>   };
>   
>   static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
> diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
> index 4db16048f8d4..4a702dc853bf 100644
> --- a/net/ethtool/netlink.h
> +++ b/net/ethtool/netlink.h
> @@ -484,6 +484,7 @@ extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADE
>   extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
>   extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
>   extern const struct nla_policy ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD + 1];
> +extern const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 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);
> @@ -492,6 +493,10 @@ int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info);
>   int ethnl_tunnel_info_start(struct netlink_callback *cb);
>   int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
>   int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info);
> +int ethnl_phy_start(struct netlink_callback *cb);
> +int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info);
> +int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
> +int ethnl_phy_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/phy.c b/net/ethtool/phy.c
> new file mode 100644
> index 000000000000..560dd039c662
> --- /dev/null
> +++ b/net/ethtool/phy.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2023 Bootlin
> + *
> + */
> +#include "common.h"
> +#include "netlink.h"
> +
> +#include <linux/phy.h>
> +#include <linux/phy_link_topology.h>
> +#include <linux/sfp.h>
> +
> +struct phy_req_info {
> +	struct ethnl_req_info		base;
> +	struct phy_device_node		*pdn;
> +};
> +
> +#define PHY_REQINFO(__req_base) \
> +	container_of(__req_base, struct phy_req_info, base)
> +
> +const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1] = {
> +	[ETHTOOL_A_PHY_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
> +};
> +
> +/* Caller holds rtnl */
> +static ssize_t
> +ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
> +		     struct netlink_ext_ack *extack)
> +{
> +	struct phy_req_info *req_info = PHY_REQINFO(req_base);
> +	struct phy_device_node *pdn = req_info->pdn;
> +	struct phy_device *phydev = pdn->phy;
> +	size_t size = 0;
> +
> +	ASSERT_RTNL();
> +
> +	/* ETHTOOL_A_PHY_INDEX */
> +	size += nla_total_size(sizeof(u32));
> +
> +	/* ETHTOOL_A_DRVNAME */
> +	if (phydev->drv)
> +		size += nla_total_size(strlen(phydev->drv->name) + 1);
> +
> +	/* ETHTOOL_A_NAME */
> +	size += nla_total_size(strlen(dev_name(&phydev->mdio.dev)) + 1);
> +
> +	/* ETHTOOL_A_PHY_UPSTREAM_TYPE */
> +	size += nla_total_size(sizeof(u32));
> +
> +	if (phy_on_sfp(phydev)) {
> +		const char *upstream_sfp_name = sfp_get_name(pdn->parent_sfp_bus);
> +
> +		/* ETHTOOL_A_PHY_UPSTREAM_SFP_NAME */
> +		if (upstream_sfp_name)
> +			size += nla_total_size(strlen(upstream_sfp_name) + 1);
> +
> +		/* ETHTOOL_A_PHY_UPSTREAM_INDEX */
> +		size += nla_total_size(sizeof(u32));
> +	}
> +
> +	/* ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME */
> +	if (phydev->sfp_bus) {
> +		const char *sfp_name = sfp_get_name(phydev->sfp_bus);
> +
> +		if (sfp_name)
> +			size += nla_total_size(strlen(sfp_name) + 1);
> +	}
> +
> +	return size;
> +}
> +
> +static int
> +ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
> +{
> +	struct phy_req_info *req_info = PHY_REQINFO(req_base);
> +	struct phy_device_node *pdn = req_info->pdn;
> +	struct phy_device *phydev = pdn->phy;
> +	enum phy_upstream ptype;
> +
> +	ptype = pdn->upstream_type;
> +
> +	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, phydev->phyindex) ||
> +	    nla_put_string(skb, ETHTOOL_A_PHY_NAME, dev_name(&phydev->mdio.dev)) ||
> +	    nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, ptype))
> +		return -EMSGSIZE;
> +
> +	if (phydev->drv &&
> +	    nla_put_string(skb, ETHTOOL_A_PHY_DRVNAME, phydev->drv->name))
> +		return -EMSGSIZE;
> +
> +	if (ptype == PHY_UPSTREAM_PHY) {
> +		struct phy_device *upstream = pdn->upstream.phydev;
> +		const char *sfp_upstream_name;
> +
> +		/* Parent index */
> +		if (nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_INDEX, upstream->phyindex))
> +			return -EMSGSIZE;
> +
> +		if (pdn->parent_sfp_bus) {
> +			sfp_upstream_name = sfp_get_name(pdn->parent_sfp_bus);
> +			if (sfp_upstream_name &&
> +			    nla_put_string(skb, ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,
> +					   sfp_upstream_name))
> +				return -EMSGSIZE;
> +		}
> +	}
> +
> +	if (phydev->sfp_bus) {
> +		const char *sfp_name = sfp_get_name(phydev->sfp_bus);
> +
> +		if (sfp_name &&
> +		    nla_put_string(skb, ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,
> +				   sfp_name))
> +			return -EMSGSIZE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ethnl_phy_parse_request(struct ethnl_req_info *req_base,
> +				   struct nlattr **tb,
> +				   struct netlink_ext_ack *extack)
> +{
> +	struct phy_link_topology *topo = req_base->dev->link_topo;
> +	struct phy_req_info *req_info = PHY_REQINFO(req_base);
> +	struct phy_device *phydev;
> +
> +	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PHY_HEADER],
> +				      extack);
> +	if (!phydev)
> +		return 0;
> +
> +	if (IS_ERR(phydev))
> +		return PTR_ERR(phydev);
> +
> +	if (!topo)
> +		return 0;
> +
> +	req_info->pdn = xa_load(&topo->phys, phydev->phyindex);
> +
> +	return 0;
> +}
> +
> +int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct phy_req_info req_info = {};
> +	struct nlattr **tb = info->attrs;
> +	struct sk_buff *rskb;
> +	void *reply_payload;
> +	int reply_len;
> +	int ret;
> +
> +	ret = ethnl_parse_header_dev_get(&req_info.base,
> +					 tb[ETHTOOL_A_PHY_HEADER],
> +					 genl_info_net(info), info->extack,
> +					 true);
> +	if (ret < 0)
> +		return ret;
> +
> +	rtnl_lock();
> +
> +	ret = ethnl_phy_parse_request(&req_info.base, tb, info->extack);
> +	if (ret < 0)
> +		goto err_unlock_rtnl;
> +
> +	/* No PHY, return early */
> +	if (!req_info.pdn->phy)
> +		goto err_unlock_rtnl;
> +
> +	ret = ethnl_phy_reply_size(&req_info.base, info->extack);
> +	if (ret < 0)
> +		goto err_unlock_rtnl;
> +	reply_len = ret + ethnl_reply_header_size();
> +
> +	rskb = ethnl_reply_init(reply_len, req_info.base.dev,
> +				ETHTOOL_MSG_PHY_GET_REPLY,
> +				ETHTOOL_A_PHY_HEADER,
> +				info, &reply_payload);
> +	if (!rskb) {
> +		ret = -ENOMEM;
> +		goto err_unlock_rtnl;
> +	}
> +
> +	ret = ethnl_phy_fill_reply(&req_info.base, rskb);
> +	if (ret)
> +		goto err_free_msg;
> +
> +	rtnl_unlock();
> +	ethnl_parse_header_dev_put(&req_info.base);
> +	genlmsg_end(rskb, reply_payload);
> +
> +	return genlmsg_reply(rskb, info);
> +
> +err_free_msg:
> +	nlmsg_free(rskb);
> +err_unlock_rtnl:
> +	rtnl_unlock();
> +	ethnl_parse_header_dev_put(&req_info.base);
> +	return ret;
> +}
> +
> +struct ethnl_phy_dump_ctx {
> +	struct phy_req_info	*phy_req_info;
> +	unsigned long ifindex;
> +	unsigned long phy_index;
> +};
> +
> +int ethnl_phy_start(struct netlink_callback *cb)
> +{
> +	const struct genl_info *info = genl_info_dump(cb);
> +	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
> +
> +	ctx->phy_req_info = kzalloc(sizeof(*ctx->phy_req_info), GFP_KERNEL);
> +	if (!ctx->phy_req_info)
> +		return -ENOMEM;
> +
> +	ret = ethnl_parse_header_dev_get(&ctx->phy_req_info->base,
> +					 info->attrs[ETHTOOL_A_PHY_HEADER],
> +					 sock_net(cb->skb->sk), cb->extack,
> +					 false);
> +	ctx->ifindex = 0;
> +	ctx->phy_index = 0;
> +
> +	if (ret)
> +		kfree(ctx->phy_req_info);
> +
> +	return ret;
> +}
> +
> +int ethnl_phy_done(struct netlink_callback *cb)
> +{
> +	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> +
> +	if (ctx->phy_req_info->base.dev)
> +		ethnl_parse_header_dev_put(&ctx->phy_req_info->base);
> +
> +	kfree(ctx->phy_req_info);
> +
> +	return 0;
> +}
> +
> +static int ethnl_phy_dump_one_dev(struct sk_buff *skb, struct net_device *dev,
> +				  struct netlink_callback *cb)
> +{
> +	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> +	struct phy_req_info *pri = ctx->phy_req_info;
> +	struct phy_device_node *pdn;
> +	int ret = 0;
> +	void *ehdr;
> +
> +	pri->base.dev = dev;
> +
> +	if (!dev->link_topo)
> +		return 0;
> +
> +	xa_for_each_start(&dev->link_topo->phys, ctx->phy_index, pdn, ctx->phy_index) {
> +		ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_PHY_GET_REPLY);
> +		if (!ehdr) {
> +			ret = -EMSGSIZE;
> +			break;
> +		}
> +
> +		ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_PHY_HEADER);
> +		if (ret < 0) {
> +			genlmsg_cancel(skb, ehdr);
> +			break;
> +		}
> +
> +		pri->pdn = pdn;
> +		ret = ethnl_phy_fill_reply(&pri->base, skb);
> +		if (ret < 0) {
> +			genlmsg_cancel(skb, ehdr);
> +			break;
> +		}
> +
> +		genlmsg_end(skb, ehdr);
> +	}
> +
> +	return ret;
> +}
> +
> +int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
> +	struct net *net = sock_net(skb->sk);
> +	struct net_device *dev;
> +	int ret = 0;
> +
> +	rtnl_lock();
> +
> +	if (ctx->phy_req_info->base.dev) {
> +		ret = ethnl_phy_dump_one_dev(skb, ctx->phy_req_info->base.dev, cb);
> +	} else {
> +		for_each_netdev_dump(net, dev, ctx->ifindex) {
> +			ret = ethnl_phy_dump_one_dev(skb, dev, cb);
> +			if (ret)
> +				break;
> +
> +			ctx->phy_index = 0;
> +		}
> +	}
> +	rtnl_unlock();
> +
> +	return ret;
> +}

[PATCH net-next v17 08/14] netlink: specs: add ethnl PHY_GET command set

[Maxime Chevallier]

The PHY_GET command, supporting both DUMP and GET operations, is used to
retrieve the list of PHYs connected to a netdevice, and get topology
information to know where exactly it sits on the physical link.

Add the netlink specs corresponding to that command.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 55 ++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 586f1da8eb7b..d96a8050172b 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -39,6 +39,11 @@ definitions:
         - ovld-detected
         - power-not-available
         - short-detected
+  -
+    name: phy-upstream-type
+    enum-name:
+    type: enum
+    entries: [ mac, phy ]
 
 attribute-sets:
   -
@@ -1088,6 +1093,35 @@ attribute-sets:
       -
         name: total
         type: uint
+  -
+    name: phy
+    attributes:
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: index
+        type: u32
+      -
+        name: drvname
+        type: string
+      -
+        name: name
+        type: string
+      -
+        name: upstream-type
+        type: u32
+        enum: phy-upstream-type
+      -
+        name: upstream-index
+        type: u32
+      -
+        name: upstream-sfp-name
+        type: string
+      -
+        name: downstream-sfp-name
+        type: string
 
 operations:
   enum-model: directional
@@ -1880,3 +1914,24 @@ operations:
           - status-msg
           - done
           - total
+    -
+      name: phy-get
+      doc: Get PHY devices attached to an interface
+
+      attribute-set: phy
+
+      do: &phy-get-op
+        request:
+          attributes:
+            - header
+        reply:
+          attributes:
+            - header
+            - index
+            - drvname
+            - name
+            - upstream-type
+            - upstream-index
+            - upstream-sfp-name
+            - downstream-sfp-name
+      dump: *phy-get-op
-- 
2.45.1

Re: [PATCH net-next v17 08/14] netlink: specs: add ethnl PHY_GET command set

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> The PHY_GET command, supporting both DUMP and GET operations, is used to
> retrieve the list of PHYs connected to a netdevice, and get topology
> information to know where exactly it sits on the physical link.
> 
> Add the netlink specs corresponding to that command.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   Documentation/netlink/specs/ethtool.yaml | 55 ++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
> 
> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index 586f1da8eb7b..d96a8050172b 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -39,6 +39,11 @@ definitions:
>           - ovld-detected
>           - power-not-available
>           - short-detected
> +  -
> +    name: phy-upstream-type
> +    enum-name:
> +    type: enum
> +    entries: [ mac, phy ]
>   
>   attribute-sets:
>     -
> @@ -1088,6 +1093,35 @@ attribute-sets:
>         -
>           name: total
>           type: uint
> +  -
> +    name: phy
> +    attributes:
> +      -
> +        name: header
> +        type: nest
> +        nested-attributes: header
> +      -
> +        name: index
> +        type: u32
> +      -
> +        name: drvname
> +        type: string
> +      -
> +        name: name
> +        type: string
> +      -
> +        name: upstream-type
> +        type: u32
> +        enum: phy-upstream-type
> +      -
> +        name: upstream-index
> +        type: u32
> +      -
> +        name: upstream-sfp-name
> +        type: string
> +      -
> +        name: downstream-sfp-name
> +        type: string
>   
>   operations:
>     enum-model: directional
> @@ -1880,3 +1914,24 @@ operations:
>             - status-msg
>             - done
>             - total
> +    -
> +      name: phy-get
> +      doc: Get PHY devices attached to an interface
> +
> +      attribute-set: phy
> +
> +      do: &phy-get-op
> +        request:
> +          attributes:
> +            - header
> +        reply:
> +          attributes:
> +            - header
> +            - index
> +            - drvname
> +            - name
> +            - upstream-type
> +            - upstream-index
> +            - upstream-sfp-name
> +            - downstream-sfp-name
> +      dump: *phy-get-op

[PATCH net-next v17 09/14] net: ethtool: plca: Target the command to the requested PHY

[Maxime Chevallier]

PLCA is a PHY-specific command. Instead of targeting the command
towards dev->phydev, use the request to pick the targeted PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/plca.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
index b1e2e3b5027f..d95d92f173a6 100644
--- a/net/ethtool/plca.c
+++ b/net/ethtool/plca.c
@@ -25,7 +25,7 @@ struct plca_reply_data {
 
 const struct nla_policy ethnl_plca_get_cfg_policy[] = {
 	[ETHTOOL_A_PLCA_HEADER]		=
-		NLA_POLICY_NESTED(ethnl_header_policy),
+		NLA_POLICY_NESTED(ethnl_header_policy_phy),
 };
 
 static void plca_update_sint(int *dst, struct nlattr **tb, u32 attrid,
@@ -58,10 +58,14 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 	struct plca_reply_data *data = PLCA_REPDATA(reply_base);
 	struct net_device *dev = reply_base->dev;
 	const struct ethtool_phy_ops *ops;
+	struct nlattr **tb = info->attrs;
+	struct phy_device *phydev;
 	int ret;
 
+	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PLCA_HEADER],
+				      info->extack);
 	// check that the PHY device is available and connected
-	if (!dev->phydev) {
+	if (IS_ERR_OR_NULL(phydev)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
@@ -80,7 +84,7 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 	memset(&data->plca_cfg, 0xff,
 	       sizeof_field(struct plca_reply_data, plca_cfg));
 
-	ret = ops->get_plca_cfg(dev->phydev, &data->plca_cfg);
+	ret = ops->get_plca_cfg(phydev, &data->plca_cfg);
 	ethnl_ops_complete(dev);
 
 out:
@@ -129,7 +133,7 @@ static int plca_get_cfg_fill_reply(struct sk_buff *skb,
 
 const struct nla_policy ethnl_plca_set_cfg_policy[] = {
 	[ETHTOOL_A_PLCA_HEADER]		=
-		NLA_POLICY_NESTED(ethnl_header_policy),
+		NLA_POLICY_NESTED(ethnl_header_policy_phy),
 	[ETHTOOL_A_PLCA_ENABLED]	= NLA_POLICY_MAX(NLA_U8, 1),
 	[ETHTOOL_A_PLCA_NODE_ID]	= NLA_POLICY_MAX(NLA_U32, 255),
 	[ETHTOOL_A_PLCA_NODE_CNT]	= NLA_POLICY_RANGE(NLA_U32, 1, 255),
@@ -141,15 +145,17 @@ const struct nla_policy ethnl_plca_set_cfg_policy[] = {
 static int
 ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 {
-	struct net_device *dev = req_info->dev;
 	const struct ethtool_phy_ops *ops;
 	struct nlattr **tb = info->attrs;
 	struct phy_plca_cfg plca_cfg;
+	struct phy_device *phydev;
 	bool mod = false;
 	int ret;
 
+	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PLCA_HEADER],
+				      info->extack);
 	// check that the PHY device is available and connected
-	if (!dev->phydev)
+	if (IS_ERR_OR_NULL(phydev))
 		return -EOPNOTSUPP;
 
 	ops = ethtool_phy_ops;
@@ -168,7 +174,7 @@ ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
 	if (!mod)
 		return 0;
 
-	ret = ops->set_plca_cfg(dev->phydev, &plca_cfg, info->extack);
+	ret = ops->set_plca_cfg(phydev, &plca_cfg, info->extack);
 	return ret < 0 ? ret : 1;
 }
 
@@ -191,7 +197,7 @@ const struct ethnl_request_ops ethnl_plca_cfg_request_ops = {
 
 const struct nla_policy ethnl_plca_get_status_policy[] = {
 	[ETHTOOL_A_PLCA_HEADER]		=
-		NLA_POLICY_NESTED(ethnl_header_policy),
+		NLA_POLICY_NESTED(ethnl_header_policy_phy),
 };
 
 static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
@@ -201,10 +207,14 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 	struct plca_reply_data *data = PLCA_REPDATA(reply_base);
 	struct net_device *dev = reply_base->dev;
 	const struct ethtool_phy_ops *ops;
+	struct nlattr **tb = info->attrs;
+	struct phy_device *phydev;
 	int ret;
 
+	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PLCA_HEADER],
+				      info->extack);
 	// check that the PHY device is available and connected
-	if (!dev->phydev) {
+	if (IS_ERR_OR_NULL(phydev)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
@@ -223,7 +233,7 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 	memset(&data->plca_st, 0xff,
 	       sizeof_field(struct plca_reply_data, plca_st));
 
-	ret = ops->get_plca_status(dev->phydev, &data->plca_st);
+	ret = ops->get_plca_status(phydev, &data->plca_st);
 	ethnl_ops_complete(dev);
 out:
 	return ret;
-- 
2.45.1

Re: [PATCH net-next v17 09/14] net: ethtool: plca: Target the command to the requested PHY

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> PLCA is a PHY-specific command. Instead of targeting the command
> towards dev->phydev, use the request to pick the targeted PHY.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   net/ethtool/plca.c | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
> index b1e2e3b5027f..d95d92f173a6 100644
> --- a/net/ethtool/plca.c
> +++ b/net/ethtool/plca.c
> @@ -25,7 +25,7 @@ struct plca_reply_data {
>   
>   const struct nla_policy ethnl_plca_get_cfg_policy[] = {
>   	[ETHTOOL_A_PLCA_HEADER]		=
> -		NLA_POLICY_NESTED(ethnl_header_policy),
> +		NLA_POLICY_NESTED(ethnl_header_policy_phy),
>   };
>   
>   static void plca_update_sint(int *dst, struct nlattr **tb, u32 attrid,
> @@ -58,10 +58,14 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
>   	struct plca_reply_data *data = PLCA_REPDATA(reply_base);
>   	struct net_device *dev = reply_base->dev;
>   	const struct ethtool_phy_ops *ops;
> +	struct nlattr **tb = info->attrs;
> +	struct phy_device *phydev;
>   	int ret;
>   
> +	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PLCA_HEADER],
> +				      info->extack);
>   	// check that the PHY device is available and connected
> -	if (!dev->phydev) {
> +	if (IS_ERR_OR_NULL(phydev)) {
>   		ret = -EOPNOTSUPP;
>   		goto out;
>   	}
> @@ -80,7 +84,7 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
>   	memset(&data->plca_cfg, 0xff,
>   	       sizeof_field(struct plca_reply_data, plca_cfg));
>   
> -	ret = ops->get_plca_cfg(dev->phydev, &data->plca_cfg);
> +	ret = ops->get_plca_cfg(phydev, &data->plca_cfg);
>   	ethnl_ops_complete(dev);
>   
>   out:
> @@ -129,7 +133,7 @@ static int plca_get_cfg_fill_reply(struct sk_buff *skb,
>   
>   const struct nla_policy ethnl_plca_set_cfg_policy[] = {
>   	[ETHTOOL_A_PLCA_HEADER]		=
> -		NLA_POLICY_NESTED(ethnl_header_policy),
> +		NLA_POLICY_NESTED(ethnl_header_policy_phy),
>   	[ETHTOOL_A_PLCA_ENABLED]	= NLA_POLICY_MAX(NLA_U8, 1),
>   	[ETHTOOL_A_PLCA_NODE_ID]	= NLA_POLICY_MAX(NLA_U32, 255),
>   	[ETHTOOL_A_PLCA_NODE_CNT]	= NLA_POLICY_RANGE(NLA_U32, 1, 255),
> @@ -141,15 +145,17 @@ const struct nla_policy ethnl_plca_set_cfg_policy[] = {
>   static int
>   ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
>   {
> -	struct net_device *dev = req_info->dev;
>   	const struct ethtool_phy_ops *ops;
>   	struct nlattr **tb = info->attrs;
>   	struct phy_plca_cfg plca_cfg;
> +	struct phy_device *phydev;
>   	bool mod = false;
>   	int ret;
>   
> +	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PLCA_HEADER],
> +				      info->extack);
>   	// check that the PHY device is available and connected
> -	if (!dev->phydev)
> +	if (IS_ERR_OR_NULL(phydev))
>   		return -EOPNOTSUPP;
>   
>   	ops = ethtool_phy_ops;
> @@ -168,7 +174,7 @@ ethnl_set_plca(struct ethnl_req_info *req_info, struct genl_info *info)
>   	if (!mod)
>   		return 0;
>   
> -	ret = ops->set_plca_cfg(dev->phydev, &plca_cfg, info->extack);
> +	ret = ops->set_plca_cfg(phydev, &plca_cfg, info->extack);
>   	return ret < 0 ? ret : 1;
>   }
>   
> @@ -191,7 +197,7 @@ const struct ethnl_request_ops ethnl_plca_cfg_request_ops = {
>   
>   const struct nla_policy ethnl_plca_get_status_policy[] = {
>   	[ETHTOOL_A_PLCA_HEADER]		=
> -		NLA_POLICY_NESTED(ethnl_header_policy),
> +		NLA_POLICY_NESTED(ethnl_header_policy_phy),
>   };
>   
>   static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
> @@ -201,10 +207,14 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
>   	struct plca_reply_data *data = PLCA_REPDATA(reply_base);
>   	struct net_device *dev = reply_base->dev;
>   	const struct ethtool_phy_ops *ops;
> +	struct nlattr **tb = info->attrs;
> +	struct phy_device *phydev;
>   	int ret;
>   
> +	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PLCA_HEADER],
> +				      info->extack);
>   	// check that the PHY device is available and connected
> -	if (!dev->phydev) {
> +	if (IS_ERR_OR_NULL(phydev)) {
>   		ret = -EOPNOTSUPP;
>   		goto out;
>   	}
> @@ -223,7 +233,7 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
>   	memset(&data->plca_st, 0xff,
>   	       sizeof_field(struct plca_reply_data, plca_st));
>   
> -	ret = ops->get_plca_status(dev->phydev, &data->plca_st);
> +	ret = ops->get_plca_status(phydev, &data->plca_st);
>   	ethnl_ops_complete(dev);
>   out:
>   	return ret;

[PATCH net-next v17 10/14] net: ethtool: pse-pd: Target the command to the requested PHY

[Maxime Chevallier]

PSE and PD configuration is a PHY-specific command. Instead of targeting
the command towards dev->phydev, use the request to pick the targeted
PHY device.

As we don't get the PHY directly from the netdev's attached phydev, also
adjust the error messages.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/pse-pd.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index ba46c9c8b12d..0f37ff5de7f0 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -28,17 +28,15 @@ struct pse_reply_data {
 /* PSE_GET */
 
 const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1] = {
-	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy_phy),
 };
 
-static int pse_get_pse_attributes(struct net_device *dev,
+static int pse_get_pse_attributes(struct phy_device *phydev,
 				  struct netlink_ext_ack *extack,
 				  struct pse_reply_data *data)
 {
-	struct phy_device *phydev = dev->phydev;
-
 	if (!phydev) {
-		NL_SET_ERR_MSG(extack, "No PHY is attached");
+		NL_SET_ERR_MSG(extack, "No PHY found");
 		return -EOPNOTSUPP;
 	}
 
@@ -58,13 +56,20 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 {
 	struct pse_reply_data *data = PSE_REPDATA(reply_base);
 	struct net_device *dev = reply_base->dev;
+	struct nlattr **tb = info->attrs;
+	struct phy_device *phydev;
 	int ret;
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		return ret;
 
-	ret = pse_get_pse_attributes(dev, info->extack, data);
+	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PSE_HEADER],
+				      info->extack);
+	if (IS_ERR(phydev))
+		return -ENODEV;
+
+	ret = pse_get_pse_attributes(phydev, info->extack, data);
 
 	ethnl_ops_complete(dev);
 
@@ -206,7 +211,7 @@ static void pse_cleanup_data(struct ethnl_reply_data *reply_base)
 /* PSE_SET */
 
 const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1] = {
-	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy_phy),
 	[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] =
 		NLA_POLICY_RANGE(NLA_U32, ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED,
 				 ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED),
@@ -219,12 +224,12 @@ const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1] = {
 static int
 ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
 {
-	struct net_device *dev = req_info->dev;
 	struct nlattr **tb = info->attrs;
 	struct phy_device *phydev;
 
-	phydev = dev->phydev;
-	if (!phydev) {
+	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
+				      info->extack);
+	if (IS_ERR_OR_NULL(phydev)) {
 		NL_SET_ERR_MSG(info->extack, "No PHY is attached");
 		return -EOPNOTSUPP;
 	}
@@ -255,12 +260,14 @@ ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
 static int
 ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 {
-	struct net_device *dev = req_info->dev;
 	struct nlattr **tb = info->attrs;
 	struct phy_device *phydev;
 	int ret = 0;
 
-	phydev = dev->phydev;
+	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
+				      info->extack);
+	if (IS_ERR_OR_NULL(phydev))
+		return -ENODEV;
 
 	if (tb[ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT]) {
 		unsigned int pw_limit;
-- 
2.45.1

Re: [PATCH net-next v17 10/14] net: ethtool: pse-pd: Target the command to the requested PHY

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> PSE and PD configuration is a PHY-specific command. Instead of targeting
> the command towards dev->phydev, use the request to pick the targeted
> PHY device.
> 
> As we don't get the PHY directly from the netdev's attached phydev, also
> adjust the error messages.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   net/ethtool/pse-pd.c | 31 +++++++++++++++++++------------
>   1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
> index ba46c9c8b12d..0f37ff5de7f0 100644
> --- a/net/ethtool/pse-pd.c
> +++ b/net/ethtool/pse-pd.c
> @@ -28,17 +28,15 @@ struct pse_reply_data {
>   /* PSE_GET */
>   
>   const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1] = {
> -	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy_phy),
>   };
>   
> -static int pse_get_pse_attributes(struct net_device *dev,
> +static int pse_get_pse_attributes(struct phy_device *phydev,
>   				  struct netlink_ext_ack *extack,
>   				  struct pse_reply_data *data)
>   {
> -	struct phy_device *phydev = dev->phydev;
> -
>   	if (!phydev) {
> -		NL_SET_ERR_MSG(extack, "No PHY is attached");
> +		NL_SET_ERR_MSG(extack, "No PHY found");
>   		return -EOPNOTSUPP;
>   	}
>   
> @@ -58,13 +56,20 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
>   {
>   	struct pse_reply_data *data = PSE_REPDATA(reply_base);
>   	struct net_device *dev = reply_base->dev;
> +	struct nlattr **tb = info->attrs;
> +	struct phy_device *phydev;
>   	int ret;
>   
>   	ret = ethnl_ops_begin(dev);
>   	if (ret < 0)
>   		return ret;
>   
> -	ret = pse_get_pse_attributes(dev, info->extack, data);
> +	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_PSE_HEADER],
> +				      info->extack);
> +	if (IS_ERR(phydev))
> +		return -ENODEV;
> +
> +	ret = pse_get_pse_attributes(phydev, info->extack, data);
>   
>   	ethnl_ops_complete(dev);
>   
> @@ -206,7 +211,7 @@ static void pse_cleanup_data(struct ethnl_reply_data *reply_base)
>   /* PSE_SET */
>   
>   const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1] = {
> -	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_PSE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy_phy),
>   	[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] =
>   		NLA_POLICY_RANGE(NLA_U32, ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED,
>   				 ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED),
> @@ -219,12 +224,12 @@ const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1] = {
>   static int
>   ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
>   {
> -	struct net_device *dev = req_info->dev;
>   	struct nlattr **tb = info->attrs;
>   	struct phy_device *phydev;
>   
> -	phydev = dev->phydev;
> -	if (!phydev) {
> +	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
> +				      info->extack);
> +	if (IS_ERR_OR_NULL(phydev)) {
>   		NL_SET_ERR_MSG(info->extack, "No PHY is attached");
>   		return -EOPNOTSUPP;
>   	}
> @@ -255,12 +260,14 @@ ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
>   static int
>   ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
>   {
> -	struct net_device *dev = req_info->dev;
>   	struct nlattr **tb = info->attrs;
>   	struct phy_device *phydev;
>   	int ret = 0;
>   
> -	phydev = dev->phydev;
> +	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
> +				      info->extack);
> +	if (IS_ERR_OR_NULL(phydev))
> +		return -ENODEV;
>   
>   	if (tb[ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT]) {
>   		unsigned int pw_limit;

[PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Maxime Chevallier]

Cable testing is a PHY-specific command. Instead of targeting the command
towards dev->phydev, use the request to pick the targeted PHY.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/cabletest.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index f6f136ec7ddf..01db8f394869 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -13,7 +13,7 @@
 
 const struct nla_policy ethnl_cable_test_act_policy[] = {
 	[ETHTOOL_A_CABLE_TEST_HEADER]		=
-		NLA_POLICY_NESTED(ethnl_header_policy),
+		NLA_POLICY_NESTED(ethnl_header_policy_phy),
 };
 
 static int ethnl_cable_test_started(struct phy_device *phydev, u8 cmd)
@@ -58,6 +58,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	struct ethnl_req_info req_info = {};
 	const struct ethtool_phy_ops *ops;
 	struct nlattr **tb = info->attrs;
+	struct phy_device *phydev;
 	struct net_device *dev;
 	int ret;
 
@@ -69,12 +70,16 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	dev = req_info.dev;
-	if (!dev->phydev) {
+
+	rtnl_lock();
+	phydev = ethnl_req_get_phydev(&req_info,
+				      tb[ETHTOOL_A_CABLE_TEST_HEADER],
+				      info->extack);
+	if (IS_ERR_OR_NULL(phydev)) {
 		ret = -EOPNOTSUPP;
 		goto out_dev_put;
 	}
 
-	rtnl_lock();
 	ops = ethtool_phy_ops;
 	if (!ops || !ops->start_cable_test) {
 		ret = -EOPNOTSUPP;
@@ -85,13 +90,12 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = ops->start_cable_test(dev->phydev, info->extack);
+	ret = ops->start_cable_test(phydev, info->extack);
 
 	ethnl_ops_complete(dev);
 
 	if (!ret)
-		ethnl_cable_test_started(dev->phydev,
-					 ETHTOOL_MSG_CABLE_TEST_NTF);
+		ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_NTF);
 
 out_rtnl:
 	rtnl_unlock();
@@ -216,7 +220,7 @@ static const struct nla_policy cable_test_tdr_act_cfg_policy[] = {
 
 const struct nla_policy ethnl_cable_test_tdr_act_policy[] = {
 	[ETHTOOL_A_CABLE_TEST_TDR_HEADER]	=
-		NLA_POLICY_NESTED(ethnl_header_policy),
+		NLA_POLICY_NESTED(ethnl_header_policy_phy),
 	[ETHTOOL_A_CABLE_TEST_TDR_CFG]		= { .type = NLA_NESTED },
 };
 
@@ -305,6 +309,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 	struct ethnl_req_info req_info = {};
 	const struct ethtool_phy_ops *ops;
 	struct nlattr **tb = info->attrs;
+	struct phy_device *phydev;
 	struct phy_tdr_config cfg;
 	struct net_device *dev;
 	int ret;
@@ -317,10 +322,6 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	dev = req_info.dev;
-	if (!dev->phydev) {
-		ret = -EOPNOTSUPP;
-		goto out_dev_put;
-	}
 
 	ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG],
 					   info, &cfg);
@@ -328,6 +329,14 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 		goto out_dev_put;
 
 	rtnl_lock();
+	phydev = ethnl_req_get_phydev(&req_info,
+				      tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
+				      info->extack);
+	if (!IS_ERR_OR_NULL(phydev)) {
+		ret = -EOPNOTSUPP;
+		goto out_dev_put;
+	}
+
 	ops = ethtool_phy_ops;
 	if (!ops || !ops->start_cable_test_tdr) {
 		ret = -EOPNOTSUPP;
@@ -338,12 +347,12 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg);
+	ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg);
 
 	ethnl_ops_complete(dev);
 
 	if (!ret)
-		ethnl_cable_test_started(dev->phydev,
+		ethnl_cable_test_started(phydev,
 					 ETHTOOL_MSG_CABLE_TEST_TDR_NTF);
 
 out_rtnl:
-- 
2.45.1

Re: [PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> Cable testing is a PHY-specific command. Instead of targeting the command
> towards dev->phydev, use the request to pick the targeted PHY.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   net/ethtool/cabletest.c | 35 ++++++++++++++++++++++-------------
>   1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
> index f6f136ec7ddf..01db8f394869 100644
> --- a/net/ethtool/cabletest.c
> +++ b/net/ethtool/cabletest.c
> @@ -13,7 +13,7 @@
>   
>   const struct nla_policy ethnl_cable_test_act_policy[] = {
>   	[ETHTOOL_A_CABLE_TEST_HEADER]		=
> -		NLA_POLICY_NESTED(ethnl_header_policy),
> +		NLA_POLICY_NESTED(ethnl_header_policy_phy),
>   };
>   
>   static int ethnl_cable_test_started(struct phy_device *phydev, u8 cmd)
> @@ -58,6 +58,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
>   	struct ethnl_req_info req_info = {};
>   	const struct ethtool_phy_ops *ops;
>   	struct nlattr **tb = info->attrs;
> +	struct phy_device *phydev;
>   	struct net_device *dev;
>   	int ret;
>   
> @@ -69,12 +70,16 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
>   		return ret;
>   
>   	dev = req_info.dev;
> -	if (!dev->phydev) {
> +
> +	rtnl_lock();
> +	phydev = ethnl_req_get_phydev(&req_info,
> +				      tb[ETHTOOL_A_CABLE_TEST_HEADER],
> +				      info->extack);
> +	if (IS_ERR_OR_NULL(phydev)) {
>   		ret = -EOPNOTSUPP;
>   		goto out_dev_put;
>   	}
>   
> -	rtnl_lock();
>   	ops = ethtool_phy_ops;
>   	if (!ops || !ops->start_cable_test) {
>   		ret = -EOPNOTSUPP;
> @@ -85,13 +90,12 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
>   	if (ret < 0)
>   		goto out_rtnl;
>   
> -	ret = ops->start_cable_test(dev->phydev, info->extack);
> +	ret = ops->start_cable_test(phydev, info->extack);
>   
>   	ethnl_ops_complete(dev);
>   
>   	if (!ret)
> -		ethnl_cable_test_started(dev->phydev,
> -					 ETHTOOL_MSG_CABLE_TEST_NTF);
> +		ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_NTF);
>   
>   out_rtnl:
>   	rtnl_unlock();
> @@ -216,7 +220,7 @@ static const struct nla_policy cable_test_tdr_act_cfg_policy[] = {
>   
>   const struct nla_policy ethnl_cable_test_tdr_act_policy[] = {
>   	[ETHTOOL_A_CABLE_TEST_TDR_HEADER]	=
> -		NLA_POLICY_NESTED(ethnl_header_policy),
> +		NLA_POLICY_NESTED(ethnl_header_policy_phy),
>   	[ETHTOOL_A_CABLE_TEST_TDR_CFG]		= { .type = NLA_NESTED },
>   };
>   
> @@ -305,6 +309,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>   	struct ethnl_req_info req_info = {};
>   	const struct ethtool_phy_ops *ops;
>   	struct nlattr **tb = info->attrs;
> +	struct phy_device *phydev;
>   	struct phy_tdr_config cfg;
>   	struct net_device *dev;
>   	int ret;
> @@ -317,10 +322,6 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>   		return ret;
>   
>   	dev = req_info.dev;
> -	if (!dev->phydev) {
> -		ret = -EOPNOTSUPP;
> -		goto out_dev_put;
> -	}
>   
>   	ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG],
>   					   info, &cfg);
> @@ -328,6 +329,14 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>   		goto out_dev_put;
>   
>   	rtnl_lock();
> +	phydev = ethnl_req_get_phydev(&req_info,
> +				      tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
> +				      info->extack);
> +	if (!IS_ERR_OR_NULL(phydev)) {
> +		ret = -EOPNOTSUPP;
> +		goto out_dev_put;
> +	}
> +
>   	ops = ethtool_phy_ops;
>   	if (!ops || !ops->start_cable_test_tdr) {
>   		ret = -EOPNOTSUPP;
> @@ -338,12 +347,12 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>   	if (ret < 0)
>   		goto out_rtnl;
>   
> -	ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg);
> +	ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg);
>   
>   	ethnl_ops_complete(dev);
>   
>   	if (!ret)
> -		ethnl_cable_test_started(dev->phydev,
> +		ethnl_cable_test_started(phydev,
>   					 ETHTOOL_MSG_CABLE_TEST_TDR_NTF);
>   
>   out_rtnl:

Re: [PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Pengfei Xu]

Hi Maxime Chevallier,

I used syzkaller and found that: there was general protection fault in
phy_start_cable_test_tdr in Linux next:next-20240826.

Bisected and found first bad commit:
"
3688ff3077d3 net: ethtool: cable-test: Target the command to the requested PHY
"
After reverted below commit on top of next-20240826, this issue was gone.

All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240826_202302_phy_start_cable_test_tdr
Syzkaller repro code: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.c
Syzkaller repro syscall steps: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.prog
Syzkaller report: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.report
Kconfig(make olddefconfig): https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/kconfig_origin
Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/bisect_info.log
bzImage: https://github.com/xupengfe/syzkaller_logs/raw/main/240826_202302_phy_start_cable_test_tdr/bzImage_1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8.tar.gz
Issue dmesg: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8_dmesg.log

"
[   27.323262] Oops: general protection fault, probably for non-canonical address 0xdffffc00000000fe: 0000 [#1] PREEMPT SMP KASAN NOPTI
[   27.324087] KASAN: null-ptr-deref in range [0x00000000000007f0-0x00000000000007f7]
[   27.324587] CPU: 0 UID: 0 PID: 729 Comm: repro Not tainted 6.11.0-rc5-next-20240826-1ca4237ad9ce-dirty #1
[   27.325203] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
[   27.325931] RIP: 0010:phy_start_cable_test_tdr+0x43/0x690
[   27.326320] Code: 48 83 ec 20 48 89 55 c0 48 89 75 c8 e8 b6 a6 1e fd 49 8d bc 24 f0 07 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 9c 05 00 00 4d 8d bc 24 e8 04 00 00 49 8b 9c 24
[   27.327485] RSP: 0018:ffff888022397370 EFLAGS: 00010202
[   27.327828] RAX: dffffc0000000000 RBX: ffff888022397560 RCX: 1ffffffff0c6423b
[   27.328291] RDX: 00000000000000fe RSI: ffffffff84482e0a RDI: 00000000000007f0
[   27.328763] RBP: ffff8880223973b8 R08: 0000000000000000 R09: ffffed1002715815
[   27.329232] R10: 0000000000000000 R11: 0000000000000005 R12: 0000000000000000
[   27.329691] R13: 0000000000000000 R14: 0000000000000000 R15: ffffffff84482de0
[   27.330155] FS:  00007ff6a8b4e740(0000) GS:ffff88806c400000(0000) knlGS:0000000000000000
[   27.330678] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   27.331060] CR2: 00007ffc26df3eb8 CR3: 0000000020c62001 CR4: 0000000000770ef0
[   27.331529] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   27.331991] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400
[   27.332452] PKRU: 55555554
[   27.332639] Call Trace:
[   27.332808]  <TASK>
[   27.332960]  ? show_regs+0x6d/0x80
[   27.333211]  ? die_addr+0x45/0xb0
[   27.333445]  ? exc_general_protection+0x1ae/0x340
[   27.333780]  ? asm_exc_general_protection+0x2b/0x30
[   27.334120]  ? __pfx_phy_start_cable_test_tdr+0x10/0x10
[   27.334473]  ? phy_start_cable_test_tdr+0x2a/0x690
[   27.334797]  ? phy_start_cable_test_tdr+0x43/0x690
[   27.335120]  ? __sanitizer_cov_trace_const_cmp1+0x1e/0x30
[   27.335489]  ? __pfx_phy_start_cable_test_tdr+0x10/0x10
[   27.335834]  ethnl_act_cable_test_tdr+0x718/0xe70
[   27.336161]  ? __pfx_ethnl_act_cable_test_tdr+0x10/0x10
[   27.336513]  ? debug_smp_processor_id+0x20/0x30
[   27.336822]  ? __nla_parse+0x49/0x60
[   27.337087]  ? __sanitizer_cov_trace_const_cmp4+0x1a/0x20
[   27.337446]  ? genl_family_rcv_msg_attrs_parse.constprop.0+0xbc/0x2a0
[   27.337877]  genl_family_rcv_msg_doit+0x22f/0x330
[   27.338192]  ? __pfx_genl_family_rcv_msg_doit+0x10/0x10
[   27.338543]  ? cap_capable+0x1d3/0x240
[   27.338813]  ? __sanitizer_cov_trace_const_cmp4+0x1a/0x20
[   27.339173]  ? ns_capable+0xec/0x130
[   27.339425]  genl_rcv_msg+0x582/0x850
[   27.339683]  ? __pfx_genl_rcv_msg+0x10/0x10
[   27.339964]  ? __pfx_ethnl_act_cable_test_tdr+0x10/0x10
[   27.340313]  ? __this_cpu_preempt_check+0x21/0x30
[   27.340632]  ? lock_acquire.part.0+0x152/0x390
[   27.340943]  netlink_rcv_skb+0x187/0x470
[   27.341216]  ? __pfx_genl_rcv_msg+0x10/0x10
[   27.341498]  ? __pfx_netlink_rcv_skb+0x10/0x10
[   27.341809]  ? __pfx_down_read+0x10/0x10
[   27.342079]  ? netlink_deliver_tap+0x1b9/0xca0
[   27.342388]  genl_rcv+0x32/0x50
[   27.342607]  netlink_unicast+0x5a3/0x870
[   27.342878]  ? __pfx_netlink_unicast+0x10/0x10
[   27.343184]  ? __sanitizer_cov_trace_cmp8+0x1c/0x30
[   27.343514]  ? __check_object_size+0x43/0x8e0
[   27.343821]  netlink_sendmsg+0x956/0xe80
[   27.344096]  ? __pfx_netlink_sendmsg+0x10/0x10
[   27.344401]  ? __import_iovec+0x1f5/0x6c0
[   27.344686]  ? __might_fault+0xf1/0x1b0
[   27.344961]  ? __pfx_netlink_sendmsg+0x10/0x10
[   27.345268]  ____sys_sendmsg+0xaba/0xc90
[   27.345544]  ? __pfx_____sys_sendmsg+0x10/0x10
[   27.345848]  ? lock_release+0x441/0x870
[   27.346115]  ___sys_sendmsg+0x122/0x1c0
[   27.346386]  ? __pfx____sys_sendmsg+0x10/0x10
[   27.346689]  ? __pfx___lock_acquire+0x10/0x10
[   27.346987]  ? __sanitizer_cov_trace_const_cmp8+0x1c/0x30
[   27.347351]  ? put_user_ifreq+0xa5/0xc0
[   27.347614]  ? __this_cpu_preempt_check+0x21/0x30
[   27.347933]  ? __sanitizer_cov_trace_const_cmp4+0x1a/0x20
[   27.348294]  ? fdget+0x188/0x230
[   27.348531]  ? __sanitizer_cov_trace_const_cmp8+0x1c/0x30
[   27.348895]  __sys_sendmsg+0x11f/0x200
[   27.349157]  ? __pfx___sys_sendmsg+0x10/0x10
[   27.349453]  ? __this_cpu_preempt_check+0x21/0x30
[   27.349777]  ? __audit_syscall_entry+0x39c/0x500
[   27.350096]  __x64_sys_sendmsg+0x80/0xc0
[   27.350371]  ? syscall_trace_enter+0x14a/0x230
[   27.350678]  x64_sys_call+0x932/0x2140
[   27.350941]  do_syscall_64+0x6d/0x140
[   27.351199]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   27.351543] RIP: 0033:0x7ff6a883ee5d
[   27.351791] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 93 af 1b 00 f7 d8 64 89 01 48
[   27.352965] RSP: 002b:00007ffc26df4f28 EFLAGS: 00000282 ORIG_RAX: 000000000000002e
[   27.353457] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ff6a883ee5d
[   27.353916] RDX: 0000000000000000 RSI: 00000000200003c0 RDI: 0000000000000003
[   27.354429] RBP: 00007ffc26df4f40 R08: 000000000000000c R09: 000000000000000c
[   27.354874] R10: 000000000000000c R11: 0000000000000282 R12: 00007ffc26df5088
[   27.355321] R13: 000000000040257c R14: 0000000000404e08 R15: 00007ff6a8b99000
[   27.355775]  </TASK>
[   27.355923] Modules linked in:
[   27.356255] ---[ end trace 0000000000000000 ]---
"

I hope it's helpful.

---

If you don't need the following environment to reproduce the problem or if you
already have one reproduced environment, please ignore the following information.

How to reproduce:
git clone https://gitlab.com/xupengfe/repro_vm_env.git
cd repro_vm_env
tar -xvf repro_vm_env.tar.gz
cd repro_vm_env; ./start3.sh  // it needs qemu-system-x86_64 and I used v7.1.0
  // start3.sh will load bzImage_2241ab53cbb5cdb08a6b2d4688feb13971058f65 v6.2-rc5 kernel
  // You could change the bzImage_xxx as you want
  // Maybe you need to remove line "-drive if=pflash,format=raw,readonly=on,file=./OVMF_CODE.fd \" for different qemu version
You could use below command to log in, there is no password for root.
ssh -p 10023 root@localhost

After login vm(virtual machine) successfully, you could transfer reproduced
binary to the vm by below way, and reproduce the problem in vm:
gcc -pthread -o repro repro.c
scp -P 10023 repro root@localhost:/root/

Get the bzImage for target kernel:
Please use target kconfig and copy it to kernel_src/.config
make olddefconfig
make -jx bzImage           //x should equal or less than cpu num your pc has

Fill the bzImage file into above start3.sh to load the target kernel in vm.


Tips:
If you already have qemu-system-x86_64, please ignore below info.
If you want to install qemu v7.1.0 version:
git clone https://github.com/qemu/qemu.git
cd qemu
git checkout -f v7.1.0
mkdir build
cd build
yum install -y ninja-build.x86_64
yum -y install libslirp-devel.x86_64
../configure --target-list=x86_64-softmmu --enable-kvm --enable-vnc --enable-gtk --enable-sdl --enable-usb-redir --enable-slirp
make
make install

Best Regards,
Thanks!



On 2024-07-09 at 08:30:34 +0200, Maxime Chevallier wrote:
> Cable testing is a PHY-specific command. Instead of targeting the command
> towards dev->phydev, use the request to pick the targeted PHY.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  net/ethtool/cabletest.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
> index f6f136ec7ddf..01db8f394869 100644
> --- a/net/ethtool/cabletest.c
> +++ b/net/ethtool/cabletest.c
> @@ -13,7 +13,7 @@
>  
>  const struct nla_policy ethnl_cable_test_act_policy[] = {
>  	[ETHTOOL_A_CABLE_TEST_HEADER]		=
> -		NLA_POLICY_NESTED(ethnl_header_policy),
> +		NLA_POLICY_NESTED(ethnl_header_policy_phy),
>  };
>  
>  static int ethnl_cable_test_started(struct phy_device *phydev, u8 cmd)
> @@ -58,6 +58,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
>  	struct ethnl_req_info req_info = {};
>  	const struct ethtool_phy_ops *ops;
>  	struct nlattr **tb = info->attrs;
> +	struct phy_device *phydev;
>  	struct net_device *dev;
>  	int ret;
>  
> @@ -69,12 +70,16 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
>  		return ret;
>  
>  	dev = req_info.dev;
> -	if (!dev->phydev) {
> +
> +	rtnl_lock();
> +	phydev = ethnl_req_get_phydev(&req_info,
> +				      tb[ETHTOOL_A_CABLE_TEST_HEADER],
> +				      info->extack);
> +	if (IS_ERR_OR_NULL(phydev)) {
>  		ret = -EOPNOTSUPP;
>  		goto out_dev_put;
>  	}
>  
> -	rtnl_lock();
>  	ops = ethtool_phy_ops;
>  	if (!ops || !ops->start_cable_test) {
>  		ret = -EOPNOTSUPP;
> @@ -85,13 +90,12 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
>  	if (ret < 0)
>  		goto out_rtnl;
>  
> -	ret = ops->start_cable_test(dev->phydev, info->extack);
> +	ret = ops->start_cable_test(phydev, info->extack);
>  
>  	ethnl_ops_complete(dev);
>  
>  	if (!ret)
> -		ethnl_cable_test_started(dev->phydev,
> -					 ETHTOOL_MSG_CABLE_TEST_NTF);
> +		ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_NTF);
>  
>  out_rtnl:
>  	rtnl_unlock();
> @@ -216,7 +220,7 @@ static const struct nla_policy cable_test_tdr_act_cfg_policy[] = {
>  
>  const struct nla_policy ethnl_cable_test_tdr_act_policy[] = {
>  	[ETHTOOL_A_CABLE_TEST_TDR_HEADER]	=
> -		NLA_POLICY_NESTED(ethnl_header_policy),
> +		NLA_POLICY_NESTED(ethnl_header_policy_phy),
>  	[ETHTOOL_A_CABLE_TEST_TDR_CFG]		= { .type = NLA_NESTED },
>  };
>  
> @@ -305,6 +309,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>  	struct ethnl_req_info req_info = {};
>  	const struct ethtool_phy_ops *ops;
>  	struct nlattr **tb = info->attrs;
> +	struct phy_device *phydev;
>  	struct phy_tdr_config cfg;
>  	struct net_device *dev;
>  	int ret;
> @@ -317,10 +322,6 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>  		return ret;
>  
>  	dev = req_info.dev;
> -	if (!dev->phydev) {
> -		ret = -EOPNOTSUPP;
> -		goto out_dev_put;
> -	}
>  
>  	ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG],
>  					   info, &cfg);
> @@ -328,6 +329,14 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>  		goto out_dev_put;
>  
>  	rtnl_lock();
> +	phydev = ethnl_req_get_phydev(&req_info,
> +				      tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
> +				      info->extack);
> +	if (!IS_ERR_OR_NULL(phydev)) {
> +		ret = -EOPNOTSUPP;
> +		goto out_dev_put;
> +	}
> +
>  	ops = ethtool_phy_ops;
>  	if (!ops || !ops->start_cable_test_tdr) {
>  		ret = -EOPNOTSUPP;
> @@ -338,12 +347,12 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>  	if (ret < 0)
>  		goto out_rtnl;
>  
> -	ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg);
> +	ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg);
>  
>  	ethnl_ops_complete(dev);
>  
>  	if (!ret)
> -		ethnl_cable_test_started(dev->phydev,
> +		ethnl_cable_test_started(phydev,
>  					 ETHTOOL_MSG_CABLE_TEST_TDR_NTF);
>  
>  out_rtnl:
> -- 
> 2.45.1
> 

Re: [PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Maxime Chevallier]

Hi,

On Tue, 27 Aug 2024 13:25:52 +0800
Pengfei Xu <pengfei.xu@intel.com> wrote:

> Hi Maxime Chevallier,
> 
> I used syzkaller and found that: there was general protection fault in
> phy_start_cable_test_tdr in Linux next:next-20240826.
> 
> Bisected and found first bad commit:
> "
> 3688ff3077d3 net: ethtool: cable-test: Target the command to the requested PHY
> "
> After reverted below commit on top of next-20240826, this issue was gone.
> 
> All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240826_202302_phy_start_cable_test_tdr
> Syzkaller repro code: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.c
> Syzkaller repro syscall steps: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.prog
> Syzkaller report: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.report
> Kconfig(make olddefconfig): https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/kconfig_origin
> Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/bisect_info.log
> bzImage: https://github.com/xupengfe/syzkaller_logs/raw/main/240826_202302_phy_start_cable_test_tdr/bzImage_1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8.tar.gz
> Issue dmesg: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8_dmesg.log

Thanks for the report !

This issue has indeed been detected, and is being addressed, see :

https://lore.kernel.org/netdev/20240826134656.94892-1-djahchankoike@gmail.com/

Thanks,

Maxime

Re: [PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Pengfei Xu]

Hi Maxime Chevallier,

On 2024-08-27 at 07:33:59 +0200, Maxime Chevallier wrote:
> Hi,
> 
> On Tue, 27 Aug 2024 13:25:52 +0800
> Pengfei Xu <pengfei.xu@intel.com> wrote:
> 
> > Hi Maxime Chevallier,
> > 
> > I used syzkaller and found that: there was general protection fault in
> > phy_start_cable_test_tdr in Linux next:next-20240826.
> > 
> > Bisected and found first bad commit:
> > "
> > 3688ff3077d3 net: ethtool: cable-test: Target the command to the requested PHY
> > "
> > After reverted below commit on top of next-20240826, this issue was gone.
> > 
> > All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240826_202302_phy_start_cable_test_tdr
> > Syzkaller repro code: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.c
> > Syzkaller repro syscall steps: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.prog
> > Syzkaller report: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.report
> > Kconfig(make olddefconfig): https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/kconfig_origin
> > Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/bisect_info.log
> > bzImage: https://github.com/xupengfe/syzkaller_logs/raw/main/240826_202302_phy_start_cable_test_tdr/bzImage_1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8.tar.gz
> > Issue dmesg: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8_dmesg.log
> 
> Thanks for the report !
> 
> This issue has indeed been detected, and is being addressed, see :
> 
> https://lore.kernel.org/netdev/20240826134656.94892-1-djahchankoike@gmail.com/

Thanks for your sharing of the solution!

Best Regards,
Thanks!


> 
> Thanks,
> 
> Maxime

Re: [PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Dan Carpenter]

On Tue, Aug 27, 2024 at 07:33:59AM +0200, Maxime Chevallier wrote:
> 
> This issue has indeed been detected, and is being addressed, see :
> 
> https://lore.kernel.org/netdev/20240826134656.94892-1-djahchankoike@gmail.com/
> 

There is a similar bug in ethnl_act_cable_test_tdr() that needs to be fixed
as well.

net/ethtool/cabletest.c
   307  int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
   308  {
   309          struct ethnl_req_info req_info = {};
   310          const struct ethtool_phy_ops *ops;
   311          struct nlattr **tb = info->attrs;
   312          struct phy_device *phydev;
   313          struct phy_tdr_config cfg;
   314          struct net_device *dev;
   315          int ret;
   316  
   317          ret = ethnl_parse_header_dev_get(&req_info,
   318                                           tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
   319                                           genl_info_net(info), info->extack,
   320                                           true);
   321          if (ret < 0)
   322                  return ret;
   323  
   324          dev = req_info.dev;
   325  
   326          ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG],
   327                                             info, &cfg);
   328          if (ret)
   329                  goto out_dev_put;
   330  
   331          rtnl_lock();
                ^^^^^^^^^^^^

   332          phydev = ethnl_req_get_phydev(&req_info,
   333                                        tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
   334                                        info->extack);
   335          if (!IS_ERR_OR_NULL(phydev)) {
                    ^
This test is reversed so it will lead to a crash.

Could you add some comments to ethnl_req_get_phydev() what the NULL return
means vs the error pointers?  I figured it out because the callers have comments
but it should be next to ethnl_req_get_phydev() as well.

   336                  ret = -EOPNOTSUPP;
   337                  goto out_dev_put;
   338          }
   339  
   340          ops = ethtool_phy_ops;
   341          if (!ops || !ops->start_cable_test_tdr) {
   342                  ret = -EOPNOTSUPP;
   343                  goto out_rtnl;
   344          }
   345  
   346          ret = ethnl_ops_begin(dev);
   347          if (ret < 0)
   348                  goto out_rtnl;
   349  
   350          ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg);
   351  
   352          ethnl_ops_complete(dev);
   353  
   354          if (!ret)
   355                  ethnl_cable_test_started(phydev,
   356                                           ETHTOOL_MSG_CABLE_TEST_TDR_NTF);
   357  
   358  out_rtnl:
   359          rtnl_unlock();
   360  out_dev_put:
   361          ethnl_parse_header_dev_put(&req_info);
   362          return ret;
   363  }

regards,
dan carpenter

Re: [PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Maxime Chevallier]

Hello Dan,

On Tue, 27 Aug 2024 11:27:48 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Tue, Aug 27, 2024 at 07:33:59AM +0200, Maxime Chevallier wrote:
> > 
> > This issue has indeed been detected, and is being addressed, see :
> > 
> > https://lore.kernel.org/netdev/20240826134656.94892-1-djahchankoike@gmail.com/
> >   
> 
> There is a similar bug in ethnl_act_cable_test_tdr() that needs to be fixed
> as well.
> 
> net/ethtool/cabletest.c
>    307  int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
>    308  {
>    309          struct ethnl_req_info req_info = {};
>    310          const struct ethtool_phy_ops *ops;
>    311          struct nlattr **tb = info->attrs;
>    312          struct phy_device *phydev;
>    313          struct phy_tdr_config cfg;
>    314          struct net_device *dev;
>    315          int ret;
>    316  
>    317          ret = ethnl_parse_header_dev_get(&req_info,
>    318                                           tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
>    319                                           genl_info_net(info), info->extack,
>    320                                           true);
>    321          if (ret < 0)
>    322                  return ret;
>    323  
>    324          dev = req_info.dev;
>    325  
>    326          ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG],
>    327                                             info, &cfg);
>    328          if (ret)
>    329                  goto out_dev_put;
>    330  
>    331          rtnl_lock();
>                 ^^^^^^^^^^^^
> 
>    332          phydev = ethnl_req_get_phydev(&req_info,
>    333                                        tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
>    334                                        info->extack);
>    335          if (!IS_ERR_OR_NULL(phydev)) {
>                     ^
> This test is reversed so it will lead to a crash.
> 
> Could you add some comments to ethnl_req_get_phydev() what the NULL return
> means vs the error pointers?  I figured it out because the callers have comments
> but it should be next to ethnl_req_get_phydev() as well.

Good catch ! I'll send some followup to address this report as
well as update the doc.

Thanks,

Maxime

Re: [PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Maxime Chevallier]

Hi again Dan,

On Tue, 27 Aug 2024 11:27:48 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:


> Could you add some comments to ethnl_req_get_phydev() what the NULL return
> means vs the error pointers?  I figured it out because the callers have comments
> but it should be next to ethnl_req_get_phydev() as well.

Actually I replied a bit too fast, this is already documented :
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/ethtool/netlink.h#n284

Is this doc clear enough or should I still add some more explicit
comments ?

Thanks,

Maxime

Re: [PATCH net-next v17 11/14] net: ethtool: cable-test: Target the command to the requested PHY

[Dan Carpenter]

On Tue, Aug 27, 2024 at 10:48:25AM +0200, Maxime Chevallier wrote:
> Hi again Dan,
> 
> On Tue, 27 Aug 2024 11:27:48 +0300
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> 
> > Could you add some comments to ethnl_req_get_phydev() what the NULL return
> > means vs the error pointers?  I figured it out because the callers have comments
> > but it should be next to ethnl_req_get_phydev() as well.
> 
> Actually I replied a bit too fast, this is already documented :
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/ethtool/netlink.h#n284
> 
> Is this doc clear enough or should I still add some more explicit
> comments ?
> 

Ah, I didn't see that.  Thanks.

That comment is fine but we normally would have put it next to the function
implementation instead of the header file.  There are lots of comments in the
header file, sure, but those are for inline functions so it's the same rule of
thumb that the comments are next to the implementation.

regards,
dan carpenter

[PATCH net-next v17 12/14] net: ethtool: strset: Remove unnecessary check on genl_info

[Maxime Chevallier]

All call paths coming from genetlink initialize the genl_info structure,
so that command handlers may use them.

Remove an un-needed check for NULL when crafting error messages in the
strset command. This prevents smatch from assuming this pointer may be
NULL, and therefore warn if it's being used without a NULL check.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reported-by: Simon Horman <horms@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202407030529.aOYGI0u2-lkp@intel.com/
---
 net/ethtool/strset.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index c678b484a079..56b99606f00b 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -289,8 +289,7 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
 		for (i = 0; i < ETH_SS_COUNT; i++) {
 			if ((req_info->req_ids & (1U << i)) &&
 			    data->sets[i].per_dev) {
-				if (info)
-					GENL_SET_ERR_MSG(info, "requested per device strings without dev");
+				GENL_SET_ERR_MSG(info, "requested per device strings without dev");
 				return -EINVAL;
 			}
 		}
-- 
2.45.1

Re: [PATCH net-next v17 12/14] net: ethtool: strset: Remove unnecessary check on genl_info

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> All call paths coming from genetlink initialize the genl_info structure,
> so that command handlers may use them.
> 
> Remove an un-needed check for NULL when crafting error messages in the
> strset command. This prevents smatch from assuming this pointer may be
> NULL, and therefore warn if it's being used without a NULL check.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reported-by: Simon Horman <horms@kernel.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202407030529.aOYGI0u2-lkp@intel.com/

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   net/ethtool/strset.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
> index c678b484a079..56b99606f00b 100644
> --- a/net/ethtool/strset.c
> +++ b/net/ethtool/strset.c
> @@ -289,8 +289,7 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
>   		for (i = 0; i < ETH_SS_COUNT; i++) {
>   			if ((req_info->req_ids & (1U << i)) &&
>   			    data->sets[i].per_dev) {
> -				if (info)
> -					GENL_SET_ERR_MSG(info, "requested per device strings without dev");
> +				GENL_SET_ERR_MSG(info, "requested per device strings without dev");
>   				return -EINVAL;
>   			}
>   		}

[PATCH net-next v17 13/14] net: ethtool: strset: Allow querying phy stats by index

[Maxime Chevallier]

The ETH_SS_PHY_STATS command gets PHY statistics. Use the phydev pointer
from the ethnl request to allow query phy stats from each PHY on the
link.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 net/ethtool/strset.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 56b99606f00b..b3382b3cf325 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -126,7 +126,7 @@ struct strset_reply_data {
 
 const struct nla_policy ethnl_strset_get_policy[] = {
 	[ETHTOOL_A_STRSET_HEADER]	=
-		NLA_POLICY_NESTED(ethnl_header_policy),
+		NLA_POLICY_NESTED(ethnl_header_policy_phy),
 	[ETHTOOL_A_STRSET_STRINGSETS]	= { .type = NLA_NESTED },
 	[ETHTOOL_A_STRSET_COUNTS_ONLY]	= { .type = NLA_FLAG },
 };
@@ -233,17 +233,18 @@ static void strset_cleanup_data(struct ethnl_reply_data *reply_base)
 }
 
 static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
-			      unsigned int id, bool counts_only)
+			      struct phy_device *phydev, unsigned int id,
+			      bool counts_only)
 {
 	const struct ethtool_phy_ops *phy_ops = ethtool_phy_ops;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	void *strings;
 	int count, ret;
 
-	if (id == ETH_SS_PHY_STATS && dev->phydev &&
+	if (id == ETH_SS_PHY_STATS && phydev &&
 	    !ops->get_ethtool_phy_stats && phy_ops &&
 	    phy_ops->get_sset_count)
-		ret = phy_ops->get_sset_count(dev->phydev);
+		ret = phy_ops->get_sset_count(phydev);
 	else if (ops->get_sset_count && ops->get_strings)
 		ret = ops->get_sset_count(dev, id);
 	else
@@ -258,10 +259,10 @@ static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
 		strings = kcalloc(count, ETH_GSTRING_LEN, GFP_KERNEL);
 		if (!strings)
 			return -ENOMEM;
-		if (id == ETH_SS_PHY_STATS && dev->phydev &&
+		if (id == ETH_SS_PHY_STATS && phydev &&
 		    !ops->get_ethtool_phy_stats && phy_ops &&
 		    phy_ops->get_strings)
-			phy_ops->get_strings(dev->phydev, strings);
+			phy_ops->get_strings(phydev, strings);
 		else
 			ops->get_strings(dev, id, strings);
 		info->strings = strings;
@@ -279,6 +280,8 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
 	const struct strset_req_info *req_info = STRSET_REQINFO(req_base);
 	struct strset_reply_data *data = STRSET_REPDATA(reply_base);
 	struct net_device *dev = reply_base->dev;
+	struct nlattr **tb = info->attrs;
+	struct phy_device *phydev;
 	unsigned int i;
 	int ret;
 
@@ -296,6 +299,13 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
 		return 0;
 	}
 
+	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_HEADER_FLAGS],
+				      info->extack);
+
+	/* phydev can be NULL, check for errors only */
+	if (IS_ERR(phydev))
+		return PTR_ERR(phydev);
+
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		goto err_strset;
@@ -304,7 +314,7 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
 		    !data->sets[i].per_dev)
 			continue;
 
-		ret = strset_prepare_set(&data->sets[i], dev, i,
+		ret = strset_prepare_set(&data->sets[i], dev, phydev, i,
 					 req_info->counts_only);
 		if (ret < 0)
 			goto err_ops;
-- 
2.45.1

Re: [PATCH net-next v17 13/14] net: ethtool: strset: Allow querying phy stats by index

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> The ETH_SS_PHY_STATS command gets PHY statistics. Use the phydev pointer
> from the ethnl request to allow query phy stats from each PHY on the
> link.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   net/ethtool/strset.c | 24 +++++++++++++++++-------
>   1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
> index 56b99606f00b..b3382b3cf325 100644
> --- a/net/ethtool/strset.c
> +++ b/net/ethtool/strset.c
> @@ -126,7 +126,7 @@ struct strset_reply_data {
>   
>   const struct nla_policy ethnl_strset_get_policy[] = {
>   	[ETHTOOL_A_STRSET_HEADER]	=
> -		NLA_POLICY_NESTED(ethnl_header_policy),
> +		NLA_POLICY_NESTED(ethnl_header_policy_phy),
>   	[ETHTOOL_A_STRSET_STRINGSETS]	= { .type = NLA_NESTED },
>   	[ETHTOOL_A_STRSET_COUNTS_ONLY]	= { .type = NLA_FLAG },
>   };
> @@ -233,17 +233,18 @@ static void strset_cleanup_data(struct ethnl_reply_data *reply_base)
>   }
>   
>   static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
> -			      unsigned int id, bool counts_only)
> +			      struct phy_device *phydev, unsigned int id,
> +			      bool counts_only)
>   {
>   	const struct ethtool_phy_ops *phy_ops = ethtool_phy_ops;
>   	const struct ethtool_ops *ops = dev->ethtool_ops;
>   	void *strings;
>   	int count, ret;
>   
> -	if (id == ETH_SS_PHY_STATS && dev->phydev &&
> +	if (id == ETH_SS_PHY_STATS && phydev &&
>   	    !ops->get_ethtool_phy_stats && phy_ops &&
>   	    phy_ops->get_sset_count)
> -		ret = phy_ops->get_sset_count(dev->phydev);
> +		ret = phy_ops->get_sset_count(phydev);
>   	else if (ops->get_sset_count && ops->get_strings)
>   		ret = ops->get_sset_count(dev, id);
>   	else
> @@ -258,10 +259,10 @@ static int strset_prepare_set(struct strset_info *info, struct net_device *dev,
>   		strings = kcalloc(count, ETH_GSTRING_LEN, GFP_KERNEL);
>   		if (!strings)
>   			return -ENOMEM;
> -		if (id == ETH_SS_PHY_STATS && dev->phydev &&
> +		if (id == ETH_SS_PHY_STATS && phydev &&
>   		    !ops->get_ethtool_phy_stats && phy_ops &&
>   		    phy_ops->get_strings)
> -			phy_ops->get_strings(dev->phydev, strings);
> +			phy_ops->get_strings(phydev, strings);
>   		else
>   			ops->get_strings(dev, id, strings);
>   		info->strings = strings;
> @@ -279,6 +280,8 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
>   	const struct strset_req_info *req_info = STRSET_REQINFO(req_base);
>   	struct strset_reply_data *data = STRSET_REPDATA(reply_base);
>   	struct net_device *dev = reply_base->dev;
> +	struct nlattr **tb = info->attrs;
> +	struct phy_device *phydev;
>   	unsigned int i;
>   	int ret;
>   
> @@ -296,6 +299,13 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
>   		return 0;
>   	}
>   
> +	phydev = ethnl_req_get_phydev(req_base, tb[ETHTOOL_A_HEADER_FLAGS],
> +				      info->extack);
> +
> +	/* phydev can be NULL, check for errors only */
> +	if (IS_ERR(phydev))
> +		return PTR_ERR(phydev);
> +
>   	ret = ethnl_ops_begin(dev);
>   	if (ret < 0)
>   		goto err_strset;
> @@ -304,7 +314,7 @@ static int strset_prepare_data(const struct ethnl_req_info *req_base,
>   		    !data->sets[i].per_dev)
>   			continue;
>   
> -		ret = strset_prepare_set(&data->sets[i], dev, i,
> +		ret = strset_prepare_set(&data->sets[i], dev, phydev, i,
>   					 req_info->counts_only);
>   		if (ret < 0)
>   			goto err_ops;

[PATCH net-next v17 14/14] Documentation: networking: document phy_link_topology

[Maxime Chevallier]

The newly introduced phy_link_topology tracks all ethernet PHYs that are
attached to a netdevice. Document the base principle, internal and
external APIs. As the phy_link_topology is expected to be extended, this
documentation will hold any further improvements and additions made
relative to topology handling.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/networking/ethtool-netlink.rst  |   3 +
 Documentation/networking/index.rst            |   1 +
 .../networking/phy-link-topology.rst          | 121 ++++++++++++++++++
 3 files changed, 125 insertions(+)
 create mode 100644 Documentation/networking/phy-link-topology.rst

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index d9f0c0dba1e5..81ddb750c1f9 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -2189,10 +2189,13 @@ Retrieve information about a given Ethernet PHY sitting on the link. The DO
 operation returns all available information about dev->phydev. User can also
 specify a PHY_INDEX, in which case the DO request returns information about that
 specific PHY.
+
 As there can be more than one PHY, the DUMP operation can be used to list the PHYs
 present on a given interface, by passing an interface index or name in
 the dump request.
 
+For more information, refer to :ref:`phy_link_topology`
+
 Request contents:
 
   ====================================  ======  ==========================
diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
index d1af04b952f8..c71b87346178 100644
--- a/Documentation/networking/index.rst
+++ b/Documentation/networking/index.rst
@@ -91,6 +91,7 @@ Contents:
    operstates
    packet_mmap
    phonet
+   phy-link-topology
    pktgen
    plip
    ppp_generic
diff --git a/Documentation/networking/phy-link-topology.rst b/Documentation/networking/phy-link-topology.rst
new file mode 100644
index 000000000000..4dec5d7d6513
--- /dev/null
+++ b/Documentation/networking/phy-link-topology.rst
@@ -0,0 +1,121 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. _phy_link_topology:
+
+=================
+PHY link topology
+=================
+
+Overview
+========
+
+The PHY link topology representation in the networking stack aims at representing
+the hardware layout for any given Ethernet link.
+
+An Ethernet interface from userspace's point of view is nothing but a
+:c:type:`struct net_device <net_device>`, which exposes configuration options
+through the legacy ioctls and the ethtool netlink commands. The base assumption
+when designing these configuration APIs were that the link looks something like ::
+
+  +-----------------------+        +----------+      +--------------+
+  | Ethernet Controller / |        | Ethernet |      | Connector /  |
+  |       MAC             | ------ |   PHY    | ---- |    Port      | ---... to LP
+  +-----------------------+        +----------+      +--------------+
+  struct net_device               struct phy_device
+
+Commands that needs to configure the PHY will go through the net_device.phydev
+field to reach the PHY and perform the relevant configuration.
+
+This assumption falls apart in more complex topologies that can arise when,
+for example, using SFP transceivers (although that's not the only specific case).
+
+Here, we have 2 basic scenarios. Either the MAC is able to output a serialized
+interface, that can directly be fed to an SFP cage, such as SGMII, 1000BaseX,
+10GBaseR, etc.
+
+The link topology then looks like this (when an SFP module is inserted) ::
+
+  +-----+  SGMII  +------------+
+  | MAC | ------- | SFP Module |
+  +-----+         +------------+
+
+Knowing that some modules embed a PHY, the actual link is more like ::
+
+  +-----+  SGMII   +--------------+
+  | MAC | -------- | PHY (on SFP) |
+  +-----+          +--------------+
+
+In this case, the SFP PHY is handled by phylib, and registered by phylink through
+its SFP upstream ops.
+
+Now some Ethernet controllers aren't able to output a serialized interface, so
+we can't directly connect them to an SFP cage. However, some PHYs can be used
+as media-converters, to translate the non-serialized MAC MII interface to a
+serialized MII interface fed to the SFP ::
+
+  +-----+  RGMII  +-----------------------+  SGMII  +--------------+
+  | MAC | ------- | PHY (media converter) | ------- | PHY (on SFP) |
+  +-----+         +-----------------------+         +--------------+
+
+This is where the model of having a single net_device.phydev pointer shows its
+limitations, as we now have 2 PHYs on the link.
+
+The phy_link topology framework aims at providing a way to keep track of every
+PHY on the link, for use by both kernel drivers and subsystems, but also to
+report the topology to userspace, allowing to target individual PHYs in configuration
+commands.
+
+API
+===
+
+The :c:type:`struct phy_link_topology <phy_link_topology>` is a per-netdevice
+resource, that gets initialized at netdevice creation. Once it's initialized,
+it is then possible to register PHYs to the topology through :
+
+:c:func:`phy_link_topo_add_phy`
+
+Besides registering the PHY to the topology, this call will also assign a unique
+index to the PHY, which can then be reported to userspace to refer to this PHY
+(akin to the ifindex). This index is a u32, ranging from 1 to U32_MAX. The value
+0 is reserved to indicate the PHY doesn't belong to any topology yet.
+
+The PHY can then be removed from the topology through
+
+:c:func:`phy_link_topo_del_phy`
+
+These function are already hooked into the phylib subsystem, so all PHYs that
+are linked to a net_device through :c:func:`phy_attach_direct` will automatically
+join the netdev's topology.
+
+PHYs that are on a SFP module will also be automatically registered IF the SFP
+upstream is phylink (so, no media-converter).
+
+PHY drivers that can be used as SFP upstream need to call :c:func:`phy_sfp_attach_phy`
+and :c:func:`phy_sfp_detach_phy`, which can be used as a
+.attach_phy / .detach_phy implementation for the
+:c:type:`struct sfp_upstream_ops <sfp_upstream_ops>`.
+
+UAPI
+====
+
+There exist a set of netlink commands to query the link topology from userspace,
+see ``Documentation/networking/ethtool-netlink.rst``.
+
+The whole point of having a topology representation is to assign the phyindex
+field in :c:type:`struct phy_device <phy_device>`. This index is reported to
+userspace using the ``ETHTOOL_MSG_PHY_GET`` ethtnl command. Performing a DUMP operation
+will result in all PHYs from all net_device being listed. The DUMP command
+accepts either a ``ETHTOOL_A_HEADER_DEV_INDEX`` or ``ETHTOOL_A_HEADER_DEV_NAME``
+to be passed in the request to filter the DUMP to a single net_device.
+
+The retrieved index can then be passed as a request parameter using the
+``ETHTOOL_A_HEADER_PHY_INDEX`` field in the following ethnl commands :
+
+* ``ETHTOOL_MSG_STRSET_GET`` to get the stats string set from a given PHY
+* ``ETHTOOL_MSG_CABLE_TEST_ACT`` and ``ETHTOOL_MSG_CABLE_TEST_ACT``, to perform
+  cable testing on a given PHY on the link (most likely the outermost PHY)
+* ``ETHTOOL_MSG_PSE_SET`` and ``ETHTOOL_MSG_PSE_GET`` for PHY-controlled PoE and PSE settings
+* ``ETHTOOL_MSG_PLCA_GET_CFG``, ``ETHTOOL_MSG_PLCA_SET_CFG`` and ``ETHTOOL_MSG_PLCA_GET_STATUS``
+  to set the PLCA (Physical Layer Collision Avoidance) parameters
+
+Note that the PHY index can be passed to other requests, which will silently
+ignore it if present and irrelevant.
-- 
2.45.1

Re: [PATCH net-next v17 14/14] Documentation: networking: document phy_link_topology

[Christophe Leroy]

Le 09/07/2024 à 08:30, Maxime Chevallier a écrit :
> The newly introduced phy_link_topology tracks all ethernet PHYs that are
> attached to a netdevice. Document the base principle, internal and
> external APIs. As the phy_link_topology is expected to be extended, this
> documentation will hold any further improvements and additions made
> relative to topology handling.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   Documentation/networking/ethtool-netlink.rst  |   3 +
>   Documentation/networking/index.rst            |   1 +
>   .../networking/phy-link-topology.rst          | 121 ++++++++++++++++++
>   3 files changed, 125 insertions(+)
>   create mode 100644 Documentation/networking/phy-link-topology.rst
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index d9f0c0dba1e5..81ddb750c1f9 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -2189,10 +2189,13 @@ Retrieve information about a given Ethernet PHY sitting on the link. The DO
>   operation returns all available information about dev->phydev. User can also
>   specify a PHY_INDEX, in which case the DO request returns information about that
>   specific PHY.
> +
>   As there can be more than one PHY, the DUMP operation can be used to list the PHYs
>   present on a given interface, by passing an interface index or name in
>   the dump request.
>   
> +For more information, refer to :ref:`phy_link_topology`
> +
>   Request contents:
>   
>     ====================================  ======  ==========================
> diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
> index d1af04b952f8..c71b87346178 100644
> --- a/Documentation/networking/index.rst
> +++ b/Documentation/networking/index.rst
> @@ -91,6 +91,7 @@ Contents:
>      operstates
>      packet_mmap
>      phonet
> +   phy-link-topology
>      pktgen
>      plip
>      ppp_generic
> diff --git a/Documentation/networking/phy-link-topology.rst b/Documentation/networking/phy-link-topology.rst
> new file mode 100644
> index 000000000000..4dec5d7d6513
> --- /dev/null
> +++ b/Documentation/networking/phy-link-topology.rst
> @@ -0,0 +1,121 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. _phy_link_topology:
> +
> +=================
> +PHY link topology
> +=================
> +
> +Overview
> +========
> +
> +The PHY link topology representation in the networking stack aims at representing
> +the hardware layout for any given Ethernet link.
> +
> +An Ethernet interface from userspace's point of view is nothing but a
> +:c:type:`struct net_device <net_device>`, which exposes configuration options
> +through the legacy ioctls and the ethtool netlink commands. The base assumption
> +when designing these configuration APIs were that the link looks something like ::
> +
> +  +-----------------------+        +----------+      +--------------+
> +  | Ethernet Controller / |        | Ethernet |      | Connector /  |
> +  |       MAC             | ------ |   PHY    | ---- |    Port      | ---... to LP
> +  +-----------------------+        +----------+      +--------------+
> +  struct net_device               struct phy_device
> +
> +Commands that needs to configure the PHY will go through the net_device.phydev
> +field to reach the PHY and perform the relevant configuration.
> +
> +This assumption falls apart in more complex topologies that can arise when,
> +for example, using SFP transceivers (although that's not the only specific case).
> +
> +Here, we have 2 basic scenarios. Either the MAC is able to output a serialized
> +interface, that can directly be fed to an SFP cage, such as SGMII, 1000BaseX,
> +10GBaseR, etc.
> +
> +The link topology then looks like this (when an SFP module is inserted) ::
> +
> +  +-----+  SGMII  +------------+
> +  | MAC | ------- | SFP Module |
> +  +-----+         +------------+
> +
> +Knowing that some modules embed a PHY, the actual link is more like ::
> +
> +  +-----+  SGMII   +--------------+
> +  | MAC | -------- | PHY (on SFP) |
> +  +-----+          +--------------+
> +
> +In this case, the SFP PHY is handled by phylib, and registered by phylink through
> +its SFP upstream ops.
> +
> +Now some Ethernet controllers aren't able to output a serialized interface, so
> +we can't directly connect them to an SFP cage. However, some PHYs can be used
> +as media-converters, to translate the non-serialized MAC MII interface to a
> +serialized MII interface fed to the SFP ::
> +
> +  +-----+  RGMII  +-----------------------+  SGMII  +--------------+
> +  | MAC | ------- | PHY (media converter) | ------- | PHY (on SFP) |
> +  +-----+         +-----------------------+         +--------------+
> +
> +This is where the model of having a single net_device.phydev pointer shows its
> +limitations, as we now have 2 PHYs on the link.
> +
> +The phy_link topology framework aims at providing a way to keep track of every
> +PHY on the link, for use by both kernel drivers and subsystems, but also to
> +report the topology to userspace, allowing to target individual PHYs in configuration
> +commands.
> +
> +API
> +===
> +
> +The :c:type:`struct phy_link_topology <phy_link_topology>` is a per-netdevice
> +resource, that gets initialized at netdevice creation. Once it's initialized,
> +it is then possible to register PHYs to the topology through :
> +
> +:c:func:`phy_link_topo_add_phy`
> +
> +Besides registering the PHY to the topology, this call will also assign a unique
> +index to the PHY, which can then be reported to userspace to refer to this PHY
> +(akin to the ifindex). This index is a u32, ranging from 1 to U32_MAX. The value
> +0 is reserved to indicate the PHY doesn't belong to any topology yet.
> +
> +The PHY can then be removed from the topology through
> +
> +:c:func:`phy_link_topo_del_phy`
> +
> +These function are already hooked into the phylib subsystem, so all PHYs that
> +are linked to a net_device through :c:func:`phy_attach_direct` will automatically
> +join the netdev's topology.
> +
> +PHYs that are on a SFP module will also be automatically registered IF the SFP
> +upstream is phylink (so, no media-converter).
> +
> +PHY drivers that can be used as SFP upstream need to call :c:func:`phy_sfp_attach_phy`
> +and :c:func:`phy_sfp_detach_phy`, which can be used as a
> +.attach_phy / .detach_phy implementation for the
> +:c:type:`struct sfp_upstream_ops <sfp_upstream_ops>`.
> +
> +UAPI
> +====
> +
> +There exist a set of netlink commands to query the link topology from userspace,
> +see ``Documentation/networking/ethtool-netlink.rst``.
> +
> +The whole point of having a topology representation is to assign the phyindex
> +field in :c:type:`struct phy_device <phy_device>`. This index is reported to
> +userspace using the ``ETHTOOL_MSG_PHY_GET`` ethtnl command. Performing a DUMP operation
> +will result in all PHYs from all net_device being listed. The DUMP command
> +accepts either a ``ETHTOOL_A_HEADER_DEV_INDEX`` or ``ETHTOOL_A_HEADER_DEV_NAME``
> +to be passed in the request to filter the DUMP to a single net_device.
> +
> +The retrieved index can then be passed as a request parameter using the
> +``ETHTOOL_A_HEADER_PHY_INDEX`` field in the following ethnl commands :
> +
> +* ``ETHTOOL_MSG_STRSET_GET`` to get the stats string set from a given PHY
> +* ``ETHTOOL_MSG_CABLE_TEST_ACT`` and ``ETHTOOL_MSG_CABLE_TEST_ACT``, to perform
> +  cable testing on a given PHY on the link (most likely the outermost PHY)
> +* ``ETHTOOL_MSG_PSE_SET`` and ``ETHTOOL_MSG_PSE_GET`` for PHY-controlled PoE and PSE settings
> +* ``ETHTOOL_MSG_PLCA_GET_CFG``, ``ETHTOOL_MSG_PLCA_SET_CFG`` and ``ETHTOOL_MSG_PLCA_GET_STATUS``
> +  to set the PLCA (Physical Layer Collision Avoidance) parameters
> +
> +Note that the PHY index can be passed to other requests, which will silently
> +ignore it if present and irrelevant.

Re: [PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[Jakub Kicinski]

On Tue,  9 Jul 2024 08:30:23 +0200 Maxime Chevallier wrote:
> This is V17 of the phy_link_topology series, aiming at improving support
> for multiple PHYs being attached to the same MAC.
> 
> V17 is mostly a rebase of V16 on net-next, as the addition of new
> features in the PSE-PD command raised a conflict on the ethtool netlink
> spec, and patch 10 was updated :
> 
> 	("net: ethtool: pse-pd: Target the command to the requested PHY")
> 
> The new code was updated to make use of the new helpers to retrieve the
> PHY from the ethnl request, and an error message was also updated to
> better reflect the fact that we don't only rely on the attached PHY for
> configuration.

I lack the confidence to take this during the merge window, without
Russell's acks. So Deferred, sorry :(

Re: [PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[Maxime Chevallier]

Hello Jakub,

On Mon, 15 Jul 2024 08:31:06 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue,  9 Jul 2024 08:30:23 +0200 Maxime Chevallier wrote:
> > This is V17 of the phy_link_topology series, aiming at improving support
> > for multiple PHYs being attached to the same MAC.
> > 
> > V17 is mostly a rebase of V16 on net-next, as the addition of new
> > features in the PSE-PD command raised a conflict on the ethtool netlink
> > spec, and patch 10 was updated :
> > 
> > 	("net: ethtool: pse-pd: Target the command to the requested PHY")
> > 
> > The new code was updated to make use of the new helpers to retrieve the
> > PHY from the ethnl request, and an error message was also updated to
> > better reflect the fact that we don't only rely on the attached PHY for
> > configuration.  
> 
> I lack the confidence to take this during the merge window, without
> Russell's acks. So Deferred, sorry :(

Understood. Is there anything I can make next time to make that series
more digestable and easy to review ? I didn't want to split the netlink
part from the core part, as just the phy_link_topology alone doesn't
make much sense for now, but it that makes the lives of reviewers
easier I could submit these separately.

Thanks,

Maxime

Re: [PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[Jakub Kicinski]

On Tue, 16 Jul 2024 10:16:26 +0200 Maxime Chevallier wrote:
> > I lack the confidence to take this during the merge window, without
> > Russell's acks. So Deferred, sorry :(  
> 
> Understood. Is there anything I can make next time to make that series
> more digestable and easy to review ? I didn't want to split the netlink
> part from the core part, as just the phy_link_topology alone doesn't
> make much sense for now, but it that makes the lives of reviewers
> easier I could submit these separately.

TBH I can only review this from coding and netlink perspective, and 
it looks solid. Folk who actually know PHYs and SFPs may have more
meaningful feedback :(

Re: [PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[Christophe Leroy]

Hi Jakub, Russell

Le 17/07/2024 à 17:26, Jakub Kicinski a écrit :
> On Tue, 16 Jul 2024 10:16:26 +0200 Maxime Chevallier wrote:
>>> I lack the confidence to take this during the merge window, without
>>> Russell's acks. So Deferred, sorry :(
>>
>> Understood. Is there anything I can make next time to make that series
>> more digestable and easy to review ? I didn't want to split the netlink
>> part from the core part, as just the phy_link_topology alone doesn't
>> make much sense for now, but it that makes the lives of reviewers
>> easier I could submit these separately.
> 
> TBH I can only review this from coding and netlink perspective, and
> it looks solid. Folk who actually know PHYs and SFPs may have more
> meaningful feedback :(

How can we progress on this ?

Russell, have you been able to have a look at that latest version of the 
series ? I know you reviewed earlier versions already but I understand 
Jakub is willing some feedback from you.

Jakub, as you say it looks solid. I can add to that that I have been 
using this series widely through the double Ethernet attachment on 
several boards and it works well, it is stable and more performant than 
the dirty home-made solution we had on v4.14.

So it would be great if the series could be merged for v6.12, and I 
guess the earliest it is merged into net-next the more time it spends in 
linux-next before the merge window. Any chance to get it merged anytime 
soon even without a formal feedback from Russell ? We are really looking 
forward to getting that series merged and step forward with all the work 
that depends on it and is awaiting.

Thanks
Christophe

Re: [PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[Jakub Kicinski]

On Fri, 16 Aug 2024 19:02:20 +0200 Christophe Leroy wrote:
> So it would be great if the series could be merged for v6.12, and I 
> guess the earliest it is merged into net-next the more time it spends in 
> linux-next before the merge window. Any chance to get it merged anytime 
> soon even without a formal feedback from Russell ? We are really looking 
> forward to getting that series merged and step forward with all the work 
> that depends on it and is awaiting.

Give Russell a few days to respond, then repost. 
Russell said his ability to review code right now may be limited.
I'm not sure whether he would like us to wait for him or just do
our best. In the absence of an opinion - we'll do the latter.

Re: [PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[Andrew Lunn]

> Jakub, as you say it looks solid. I can add to that that I have been using
> this series widely through the double Ethernet attachment on several boards
> and it works well, it is stable and more performant than the dirty home-made
> solution we had on v4.14.

Have you posted a Tested-by:

You can also post Reviewed-by: if you have taken a look at the
code. It won't have the same value as one from Rusell, but it does add
some degree of warm fuzzy feeling this code is O.K, and it starts
building your reputation as a reviewer.

	Andrew

Re: [PATCH net-next v17 00/14] Introduce PHY listing and link_topology tracking

[Thomas Petazzoni]

Hello Andrew,

On Sat, 17 Aug 2024 17:43:52 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > Jakub, as you say it looks solid. I can add to that that I have been using
> > this series widely through the double Ethernet attachment on several boards
> > and it works well, it is stable and more performant than the dirty home-made
> > solution we had on v4.14.  
> 
> Have you posted a Tested-by:
> 
> You can also post Reviewed-by: if you have taken a look at the
> code. It won't have the same value as one from Rusell, but it does add
> some degree of warm fuzzy feeling this code is O.K, and it starts
> building your reputation as a reviewer.

Hmm did you check the replies from Christophe to each patch of this
series? He has already posted a Tested-by and Reviewed-by to every
single patch in the series :-)

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com