[RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

Hi!

Background
==========

This series adds initial ethtool support for CMIS loopback.

The Common Management Interface Specification (CMIS) is an
industry-standard used by host devices (like switches and routers) to
talk to high-speed optical transceivers (like QSFP-DD, OSFP, and
QSFP112).

Ethtool already supports mechanism updating the transceiver firmware
via CMIS, and this series builds on top of this work.

In CMIS, four different types of loopback are defined by the
specification, characterized by the location of the loopback on Host
(electrical) or Media (optical) Side and the direction of the signal
being looped-back:

* Media Side Output (Tx->Rx) Loopback
* Media Side Input (Rx->Tx) Loopback
* Host Side Output (Tx->Rx) Loopback
* Host Side Input (Rx->Tx) Loopback 

To detect and enable loopback in a CMIS transceiver, the following
registers are used:

* Detect Support: Read Page 13h, Byte 128 indicate if the hardware
  supports host-side or media-side loopback.

* Enable Loopback: Write to Page 13h, Bytes 180–184. Each bit in these
  registers typically corresponds to a specific lane (0–7). Setting a
  bit to 1 requests loopback for that lane.


Implementation
==============

Patch 1/4 ethtool: module: Define CMIS loopback YAML spec and UAPI
  Adds the netlink YAML specification and UAPI for module loopback.
  Defines a flags enum with the four CMIS 5.2 diagnostic loopback
  types (media-side output/input, host-side output/input) and two new
  module attributes: loopback-capabilities (supported modes) and
  loopback-enabled (active modes). Regenerates the UAPI header.

Patch 2/4 ethtool: module: Add CMIS loopback GET/SET support
  Implements the core loopback GET/SET logic for CMIS modules. Reads
  capabilities from Page 01h Byte 142 and controls loopback via Page
  13h Bytes 180-183, using the existing get/set_module_eeprom_by_page
  driver ops. No new ethtool_ops callbacks are introduced.
  
Patch 3/4 ethtool: module: refactor fw flash init to reuse CMIS
  helpers Refactors module_flash_fw_work_init() to reuse the
  module_is_cmis() helper and ethtool_cmis_page_init() introduced in
  patch 2, removing open-coded CMIS type checking and manual EEPROM
  page setup from the firmware flash path.

Patch 4/4 net/mlx5e: Implement set_module_eeprom_by_page ethtool
  callback Adds EEPROM write support to mlx5 by implementing
  set_module_eeprom_by_page, mirroring the existing read path via the
  MCIA register. This enables the loopback SET path which requires
  both get and set callbacks.


Limitations
===========

Only four modes are supported host/media-side near-/far-end. No
per-lane support.

I'm working on kselftest; It's not part of the RFC.


RFC
===

I'm not familiar with the mlx5 internals, and need guidance if my
set_module_eeprom_by_page() hack is the right way forward. I've tested
this on a transceiver in a CX7 NIC, and it did switch on the loopback
mode, so it's somewhat working.

I'd like input from other NIC vendors, if the
{set,get}_module_eeprom_by_page() is the right interface/ops from a
driver POV.

Extensibility; Is this the right interface?


Related work
============

* New loopback modes [1].
* PHY loopback [2]
* bnxt_en: add .set_module_eeprom_by_page() support [3]
* ethtool: qsfp transceiver reset, interrupt and presence pin control
  [4]

[1] https://lore.kernel.org/netdev/20251024044849.1098222-1-hkelam@marvell.com/
[2] https://lore.kernel.org/netdev/20240911212713.2178943-1-maxime.chevallier@bootlin.com/
[3] https://lore.kernel.org/netdev/20250310183129.3154117-8-michael.chan@broadcom.com/
[4] https://lore.kernel.org/netdev/20250513224017.202236-1-mpazdan@arista.com/


Björn Töpel (4):
  ethtool: module: Define CMIS loopback YAML spec and UAPI
  ethtool: module: Add CMIS loopback GET/SET support
  ethtool: module: refactor fw flash init to reuse CMIS helpers
  net/mlx5e: Implement set_module_eeprom_by_page ethtool callback

 Documentation/netlink/specs/ethtool.yaml      |  27 ++
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  52 ++-
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |   6 +-
 .../net/ethernet/mellanox/mlx5/core/port.c    |  34 +-
 include/linux/ethtool.h                       |  12 +
 .../uapi/linux/ethtool_netlink_generated.h    |  22 ++
 net/ethtool/module.c                          | 302 ++++++++++++++++--
 net/ethtool/netlink.h                         |   2 +-
 8 files changed, 397 insertions(+), 60 deletions(-)


base-commit: 37a93dd5c49b5fda807fd204edf2547c3493319c
-- 
2.53.0

[RFC net-next 1/4] ethtool: module: Define CMIS loopback YAML spec and UAPI

[Björn Töpel]

Add the netlink YAML specification for module loopback attributes used
to query and control diagnostic loopback modes on CMIS-compliant
transceiver modules.

Define the module-loopback-types flags enum with four loopback types
from CMIS 5.2 Section 8.4.11 ("Diagnostic Loopback Capabilities"):
  - media-side-output (Rx to Tx)
  - media-side-input  (Tx to Rx)
  - host-side-output  (Rx to Tx)
  - host-side-input   (Tx to Rx)

Add two new attributes to the module attribute-set:
  - loopback-capabilities: bitmask of supported loopback types
  - loopback-enabled:      bitmask of currently enabled loopback types

Include both attributes in the module-get reply (and by YAML anchor
reuse, in the module-set request). Regenerate the UAPI header.

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 Documentation/netlink/specs/ethtool.yaml      | 27 +++++++++++++++++++
 .../uapi/linux/ethtool_netlink_generated.h    | 22 +++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 0a2d2343f79a..88dfd5f05b3c 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -11,6 +11,23 @@ c-family-name: ethtool-genl-name
 c-version-name: ethtool-genl-version
 
 definitions:
+  -
+    name: module-loopback-types
+    type: flags
+    doc: Defines the loopback types (host/electrical or media/optical side) and the signal's path direction (input/near-end or output/far-end).
+    entries:
+      -
+        name: media-side-output
+        doc: Media Side Output Loopback (Rx to Tx)
+      -
+        name: media-side-input
+        doc: Media Side Input Loopback (Tx to Rx)
+      -
+        name: host-side-output
+        doc: Host Side Output Loopback (Rx to Tx)
+      -
+        name: host-side-input
+        doc: Host Side Input Loopback (Tx to Rx)
   -
     name: udp-tunnel-type
     enum-name:
@@ -1438,6 +1455,14 @@ attribute-sets:
       -
         name: power-mode
         type: u8
+      -
+        name: loopback-capabilities
+        type: uint
+        enum: module-loopback-types
+      -
+        name: loopback-enabled
+        type: uint
+        enum: module-loopback-types
   -
     name: c33-pse-pw-limit
     attr-cnt-name: __ethtool-a-c33-pse-pw-limit-cnt
@@ -2501,6 +2526,8 @@ operations:
             - header
             - power-mode-policy
             - power-mode
+            - loopback-capabilities
+            - loopback-enabled
       dump: *module-get-op
     -
       name: module-set
diff --git a/include/uapi/linux/ethtool_netlink_generated.h b/include/uapi/linux/ethtool_netlink_generated.h
index 556a0c834df5..d94b75b27718 100644
--- a/include/uapi/linux/ethtool_netlink_generated.h
+++ b/include/uapi/linux/ethtool_netlink_generated.h
@@ -10,6 +10,26 @@
 #define ETHTOOL_GENL_NAME	"ethtool"
 #define ETHTOOL_GENL_VERSION	1
 
+/**
+ * enum ethtool_module_loopback_types - Defines the loopback types
+ *   (host/electrical or media/optical side) and the signal's path direction
+ *   (input/near-end or output/far-end).
+ * @ETHTOOL_MODULE_LOOPBACK_TYPES_MEDIA_SIDE_OUTPUT: Media Side Output Loopback
+ *   (Rx to Tx)
+ * @ETHTOOL_MODULE_LOOPBACK_TYPES_MEDIA_SIDE_INPUT: Media Side Input Loopback
+ *   (Tx to Rx)
+ * @ETHTOOL_MODULE_LOOPBACK_TYPES_HOST_SIDE_OUTPUT: Host Side Output Loopback
+ *   (Rx to Tx)
+ * @ETHTOOL_MODULE_LOOPBACK_TYPES_HOST_SIDE_INPUT: Host Side Input Loopback (Tx
+ *   to Rx)
+ */
+enum ethtool_module_loopback_types {
+	ETHTOOL_MODULE_LOOPBACK_TYPES_MEDIA_SIDE_OUTPUT = 1,
+	ETHTOOL_MODULE_LOOPBACK_TYPES_MEDIA_SIDE_INPUT = 2,
+	ETHTOOL_MODULE_LOOPBACK_TYPES_HOST_SIDE_OUTPUT = 4,
+	ETHTOOL_MODULE_LOOPBACK_TYPES_HOST_SIDE_INPUT = 8,
+};
+
 enum {
 	ETHTOOL_UDP_TUNNEL_TYPE_VXLAN,
 	ETHTOOL_UDP_TUNNEL_TYPE_GENEVE,
@@ -654,6 +674,8 @@ enum {
 	ETHTOOL_A_MODULE_HEADER,
 	ETHTOOL_A_MODULE_POWER_MODE_POLICY,
 	ETHTOOL_A_MODULE_POWER_MODE,
+	ETHTOOL_A_MODULE_LOOPBACK_CAPABILITIES,
+	ETHTOOL_A_MODULE_LOOPBACK_ENABLED,
 
 	__ETHTOOL_A_MODULE_CNT,
 	ETHTOOL_A_MODULE_MAX = (__ETHTOOL_A_MODULE_CNT - 1)
-- 
2.53.0

[RFC net-next 2/4] ethtool: module: Add CMIS loopback GET/SET support

[Björn Töpel]

Implement loopback mode control for CMIS-compliant transceiver modules
(QSFP-DD, OSFP, DSFP, etc.) by reading and writing EEPROM registers
directly via the existing get/set_module_eeprom_by_page driver ops.

Loopback capabilities are read from Page 01h Byte 142 (Table 8-40,
"Supported Diagnostic and Monitoring Capabilities", CMIS 5.2), where
the lower 4 bits advertise supported loopback types.

Loopback control is read/written via Page 13h Bytes 180-183 (Table
8-128 "Media Lane-Specific Loopback Controls", CMIS 5.2):
  - Byte 180: Media Side Output Loopback
  - Byte 181: Media Side Input Loopback
  - Byte 182: Host Side Output Loopback
  - Byte 183: Host Side Input Loopback

The SET path validates that the module is CMIS, that Page 13h is
advertised, and that requested modes are within the capabilities mask
before writing 0xFF (all-lanes enable) or 0x00 (disable) per control
byte.

No new ethtool_ops callbacks are introduced; all register access reuses
the existing EEPROM page infrastructure.

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 include/linux/ethtool.h |  12 ++
 net/ethtool/module.c    | 280 ++++++++++++++++++++++++++++++++++++++--
 net/ethtool/netlink.h   |   2 +-
 3 files changed, 279 insertions(+), 15 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 798abec67a1b..b4cf0314b38c 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -683,6 +683,18 @@ struct ethtool_module_power_mode_params {
 	enum ethtool_module_power_mode mode;
 };
 
+/**
+ * struct ethtool_module_loopback_params - module loopback parameters
+ * @caps: The loopback types, %ETHTOOL_MODULE_LOOPBACK_TYPES_*,
+ *	supported by the module.
+ * @enabled: The loopback types, %ETHTOOL_MODULE_LOOPBACK_TYPES_*,
+ *	currently enabled by the module.
+ */
+struct ethtool_module_loopback_params {
+	u32 caps;
+	u32 enabled;
+};
+
 /**
  * struct ethtool_mm_state - 802.3 MAC merge layer state
  * @verify_time:
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 4d4e0a82579a..21d2f59985af 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -10,6 +10,7 @@
 #include "common.h"
 #include "bitset.h"
 #include "module_fw.h"
+#include "cmis.h"
 
 struct module_req_info {
 	struct ethnl_req_info base;
@@ -18,6 +19,8 @@ struct module_req_info {
 struct module_reply_data {
 	struct ethnl_reply_data	base;
 	struct ethtool_module_power_mode_params power;
+	struct ethtool_module_loopback_params loopback;
+	bool loopback_supp;
 };
 
 #define MODULE_REPDATA(__reply_base) \
@@ -47,6 +50,144 @@ static int module_get_power_mode(struct net_device *dev,
 	return ops->get_module_power_mode(dev, &data->power, extack);
 }
 
+#define CMIS_PHYS_ADMINISTRATIVE_INFO_PAGE	0
+#define CMIS_PHYS_ADMINISTRATIVE_INFO_OFFSET	0
+
+#define CMIS_PAGE13_ADVERTISEMENT_PAGE		0x01
+#define CMIS_PAGE13_ADVERTISEMENT_OFFSET	0x8E
+#define CMIS_PAGE13_SUPPORTED			BIT(5)
+
+#define CMIS_MODULE_LOOPBACK_CAPS_PAGE		0x13
+#define CMIS_MODULE_LOOPBACK_CAPS_OFFSET	0x80
+
+#define CMIS_MODULE_LOOPBACK_CTRL_PAGE		0x13
+#define CMIS_MODULE_LOOPBACK_CTRL_OFFSET	0xB4	/* 4 consecutive bytes */
+#define CMIS_MODULE_LOOPBACK_CTRL_LEN		4
+
+/* Mapping from Page 13h control bytes to ethtool_module_loopback_types flags.
+ * Byte offset 0xB4+i maps to loopback_ctrl_flags[i].
+ */
+static const u32 loopback_ctrl_flags[] = {
+	ETHTOOL_MODULE_LOOPBACK_TYPES_MEDIA_SIDE_OUTPUT,
+	ETHTOOL_MODULE_LOOPBACK_TYPES_MEDIA_SIDE_INPUT,
+	ETHTOOL_MODULE_LOOPBACK_TYPES_HOST_SIDE_OUTPUT,
+	ETHTOOL_MODULE_LOOPBACK_TYPES_HOST_SIDE_INPUT,
+};
+
+static bool module_is_cmis(u8 phys_id)
+{
+	switch (phys_id) {
+	case SFF8024_ID_QSFP_DD:
+	case SFF8024_ID_OSFP:
+	case SFF8024_ID_DSFP:
+	case SFF8024_ID_QSFP_PLUS_CMIS:
+	case SFF8024_ID_SFP_DD_CMIS:
+	case SFF8024_ID_SFP_PLUS_CMIS:
+		return true;
+	default:
+	}
+	return false;
+}
+
+static bool module_has_cmis_page13(u8 supp)
+{
+	return !!(supp & CMIS_PAGE13_SUPPORTED);
+}
+
+/* Return <0 error, >0 loopback supported, =0 no support */
+static int module_get_loopback_caps(struct net_device *dev, struct netlink_ext_ack *extack,
+				    u8 *caps)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {};
+	u8 phys_id, page13;
+	int err;
+
+	*caps = 0;
+
+	/* Read module physical ID to verify CMIS type */
+	ethtool_cmis_page_init(&page_data, CMIS_PHYS_ADMINISTRATIVE_INFO_PAGE,
+			       CMIS_PHYS_ADMINISTRATIVE_INFO_OFFSET, sizeof(phys_id));
+	page_data.data = &phys_id;
+
+	err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
+	if (err < 0)
+		return err;
+
+	if (!module_is_cmis(phys_id))
+		return 0;
+
+	/* Read DiagnosticPagesSupported/Page13 availability */
+	ethtool_cmis_page_init(&page_data, CMIS_PAGE13_ADVERTISEMENT_PAGE,
+			       CMIS_PAGE13_ADVERTISEMENT_OFFSET, sizeof(page13));
+	page_data.data = &page13;
+
+	err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
+	if (err < 0)
+		return err;
+
+	if (!module_has_cmis_page13(page13))
+		return 0;
+
+	/* Read loopback capabilities */
+	ethtool_cmis_page_init(&page_data, CMIS_MODULE_LOOPBACK_CAPS_PAGE,
+			       CMIS_MODULE_LOOPBACK_CAPS_OFFSET, sizeof(*caps));
+	page_data.data = caps;
+
+	err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
+	if (err < 0)
+		return err;
+
+	/* Lower 4 bits map directly to ethtool_module_loopback_types flags */
+	*caps &= 0x0F;
+	return 1;
+}
+
+static int module_get_loopback(struct net_device *dev, struct module_reply_data *data,
+			       struct netlink_ext_ack *extack)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {};
+	u8 ctrl[CMIS_MODULE_LOOPBACK_CTRL_LEN];
+	int err, i;
+	u8 caps;
+
+	if (!ops->get_module_eeprom_by_page)
+		return 0;
+
+	if (dev->ethtool->module_fw_flash_in_progress) {
+		NL_SET_ERR_MSG(extack, "Module firmware flashing is in progress");
+		return -EBUSY;
+	}
+
+	err = module_get_loopback_caps(dev, extack, &caps);
+	if (err < 0)
+		return err;
+
+	data->loopback_supp = (err == 1);
+	data->loopback.caps = caps;
+
+	if (!data->loopback.caps)
+		return 0;
+
+	/* Read loopback control from Page 13h, Bytes 180-183 */
+	ethtool_cmis_page_init(&page_data, CMIS_MODULE_LOOPBACK_CTRL_PAGE,
+			       CMIS_MODULE_LOOPBACK_CTRL_OFFSET, sizeof(ctrl));
+	page_data.data = ctrl;
+
+	err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
+	if (err < 0)
+		return err;
+
+	/* Each non-zero byte means that loopback type is enabled */
+	for (i = 0; i < CMIS_MODULE_LOOPBACK_CTRL_LEN; i++) {
+		if (ctrl[i])
+			data->loopback.enabled |= loopback_ctrl_flags[i];
+	}
+
+	return 0;
+}
+
 static int module_prepare_data(const struct ethnl_req_info *req_base,
 			       struct ethnl_reply_data *reply_base,
 			       const struct genl_info *info)
@@ -63,6 +204,10 @@ static int module_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		goto out_complete;
 
+	ret = module_get_loopback(dev, data, info->extack);
+	if (ret < 0)
+		goto out_complete;
+
 out_complete:
 	ethnl_ops_complete(dev);
 	return ret;
@@ -80,6 +225,11 @@ static int module_reply_size(const struct ethnl_req_info *req_base,
 	if (data->power.mode)
 		len += nla_total_size(sizeof(u8));	/* _MODULE_POWER_MODE */
 
+	if (data->loopback_supp) {
+		/* _MODULE_LOOPBACK_{CAPABILITIES,ENABLED} */
+		len += nla_total_size(sizeof(u32)) * 2;
+	}
+
 	return len;
 }
 
@@ -98,16 +248,32 @@ static int module_fill_reply(struct sk_buff *skb,
 	    nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE, data->power.mode))
 		return -EMSGSIZE;
 
+	if (data->loopback_supp &&
+	    nla_put_uint(skb, ETHTOOL_A_MODULE_LOOPBACK_CAPABILITIES,
+			 data->loopback.caps))
+		return -EMSGSIZE;
+
+	if (data->loopback_supp &&
+	    nla_put_uint(skb, ETHTOOL_A_MODULE_LOOPBACK_ENABLED,
+			 data->loopback.enabled))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
 /* MODULE_SET */
 
-const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1] = {
+const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_LOOPBACK_ENABLED + 1] = {
 	[ETHTOOL_A_MODULE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
 	[ETHTOOL_A_MODULE_POWER_MODE_POLICY] =
 		NLA_POLICY_RANGE(NLA_U8, ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH,
 				 ETHTOOL_MODULE_POWER_MODE_POLICY_AUTO),
+	[ETHTOOL_A_MODULE_LOOPBACK_ENABLED] =
+		NLA_POLICY_MASK(NLA_UINT,
+				ETHTOOL_MODULE_LOOPBACK_TYPES_MEDIA_SIDE_OUTPUT |
+				ETHTOOL_MODULE_LOOPBACK_TYPES_MEDIA_SIDE_INPUT |
+				ETHTOOL_MODULE_LOOPBACK_TYPES_HOST_SIDE_OUTPUT |
+				ETHTOOL_MODULE_LOOPBACK_TYPES_HOST_SIDE_INPUT),
 };
 
 static int
@@ -117,7 +283,8 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info,
 	const struct ethtool_ops *ops = req_info->dev->ethtool_ops;
 	struct nlattr **tb = info->attrs;
 
-	if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY])
+	if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY] &&
+	    !tb[ETHTOOL_A_MODULE_LOOPBACK_ENABLED])
 		return 0;
 
 	if (req_info->dev->ethtool->module_fw_flash_in_progress) {
@@ -126,38 +293,123 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info,
 		return -EBUSY;
 	}
 
-	if (!ops->get_module_power_mode || !ops->set_module_power_mode) {
+	if (tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY] &&
+	    (!ops->get_module_power_mode || !ops->set_module_power_mode)) {
 		NL_SET_ERR_MSG_ATTR(info->extack,
 				    tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY],
 				    "Setting power mode policy is not supported by this device");
 		return -EOPNOTSUPP;
 	}
 
+	if (tb[ETHTOOL_A_MODULE_LOOPBACK_ENABLED] &&
+	    (!ops->get_module_eeprom_by_page || !ops->set_module_eeprom_by_page)) {
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    tb[ETHTOOL_A_MODULE_LOOPBACK_ENABLED],
+				    "Setting loopback is not supported by this device");
+		return -EOPNOTSUPP;
+	}
+
+	if (tb[ETHTOOL_A_MODULE_LOOPBACK_ENABLED] &&
+	    (req_info->dev->flags & IFF_UP)) {
+		NL_SET_ERR_MSG(info->extack,
+			       "Netdevice is up, so setting loopback is not permitted");
+		return -EBUSY;
+	}
+
 	return 1;
 }
 
-static int
-ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
+static int module_set_loopback(struct net_device *dev, struct genl_info *info)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {};
+	u8 ctrl[CMIS_MODULE_LOOPBACK_CTRL_LEN], caps;
+	bool changed = false;
+	int err, i;
+	u32 req;
+
+	req = nla_get_uint(info->attrs[ETHTOOL_A_MODULE_LOOPBACK_ENABLED]);
+
+	err = module_get_loopback_caps(dev, info->extack, &caps);
+	if (err < 0)
+		return err;
+
+	if (err == 0 || req & ~(u32)caps) {
+		NL_SET_ERR_MSG(info->extack, "Requested loopback mode(s) not supported by module");
+		return -EOPNOTSUPP;
+	}
+
+	/* Read current enabled state from Page 13h */
+	ethtool_cmis_page_init(&page_data, CMIS_MODULE_LOOPBACK_CTRL_PAGE,
+			       CMIS_MODULE_LOOPBACK_CTRL_OFFSET, sizeof(ctrl));
+	page_data.data = ctrl;
+
+	err = ops->get_module_eeprom_by_page(dev, &page_data, info->extack);
+	if (err < 0)
+		return err;
+
+	/* Update control bytes: 0xFF for all-lanes enable, 0x00 for disable */
+	for (i = 0; i < CMIS_MODULE_LOOPBACK_CTRL_LEN; i++) {
+		u8 new_val = (req & loopback_ctrl_flags[i]) ? 0xFF : 0x00;
+
+		if (ctrl[i] != new_val) {
+			ctrl[i] = new_val;
+			changed = true;
+		}
+	}
+
+	if (!changed)
+		return 0;
+
+	/* Write updated control bytes */
+	ethtool_cmis_page_init(&page_data, CMIS_MODULE_LOOPBACK_CTRL_PAGE,
+			       CMIS_MODULE_LOOPBACK_CTRL_OFFSET, sizeof(ctrl));
+	page_data.data = ctrl;
+
+	err = ops->set_module_eeprom_by_page(dev, &page_data, info->extack);
+	if (err < 0)
+		return err;
+
+	return 1;
+}
+
+static int ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
 {
 	struct ethtool_module_power_mode_params power = {};
 	struct ethtool_module_power_mode_params power_new;
 	const struct ethtool_ops *ops;
 	struct net_device *dev = req_info->dev;
 	struct nlattr **tb = info->attrs;
-	int ret;
+	int ret = 0;
 
 	ops = dev->ethtool_ops;
 
-	power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
-	ret = ops->get_module_power_mode(dev, &power, info->extack);
-	if (ret < 0)
-		return ret;
+	if (tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]) {
+		power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
+		ret = ops->get_module_power_mode(dev, &power, info->extack);
+		if (ret < 0)
+			return ret;
 
-	if (power_new.policy == power.policy)
-		return 0;
+		if (power_new.policy != power.policy) {
+			ret = ops->set_module_power_mode(dev, &power_new,
+							 info->extack);
+			if (ret < 0)
+				return ret;
+			ret = 1;
+		}
+	}
 
-	ret = ops->set_module_power_mode(dev, &power_new, info->extack);
-	return ret < 0 ? ret : 1;
+	if (tb[ETHTOOL_A_MODULE_LOOPBACK_ENABLED]) {
+		int lret;
+
+		lret = module_set_loopback(dev, info);
+		if (lret < 0)
+			return lret;
+		if (lret > 0)
+			ret = lret;
+	}
+
+	return ret;
 }
 
 const struct ethnl_request_ops ethnl_module_request_ops = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 89010eaa67df..2284a333aa74 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -482,7 +482,7 @@ extern const struct nla_policy ethnl_module_eeprom_get_policy[ETHTOOL_A_MODULE_E
 extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_SRC + 1];
 extern const struct nla_policy ethnl_phc_vclocks_get_policy[ETHTOOL_A_PHC_VCLOCKS_HEADER + 1];
 extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1];
-extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1];
+extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_LOOPBACK_ENABLED + 1];
 extern const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1];
 extern const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1];
 extern const struct nla_policy ethnl_rss_get_policy[ETHTOOL_A_RSS_START_CONTEXT + 1];
-- 
2.53.0

[RFC net-next 3/4] ethtool: module: refactor fw flash init to reuse CMIS helpers

[Björn Töpel]

Simplify module_flash_fw_work_init() by reusing the module_is_cmis()
helper and ethtool_cmis_page_init() introduced in the loopback support
commit, replacing the open-coded CMIS type switch and manual EEPROM
page setup.

Remove the now-duplicate MODULE_EEPROM_PHYS_ID_I2C_ADDR define from
the firmware flash section since ethtool_cmis_page_init() sets the I2C
address internally.

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 net/ethtool/module.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 21d2f59985af..76e35dae1db2 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -486,7 +486,6 @@ static void module_flash_fw_work(struct work_struct *work)
 }
 
 #define MODULE_EEPROM_PHYS_ID_PAGE	0
-#define MODULE_EEPROM_PHYS_ID_I2C_ADDR	0x50
 
 static int module_flash_fw_work_init(struct ethtool_module_fw_flash *module_fw,
 				     struct net_device *dev,
@@ -501,31 +500,20 @@ static int module_flash_fw_work_init(struct ethtool_module_fw_flash *module_fw,
 	 * is located at I2C address 0x50, byte 0. See section 4.1 in SFF-8024,
 	 * revision 4.9.
 	 */
-	page_data.page = MODULE_EEPROM_PHYS_ID_PAGE;
-	page_data.offset = SFP_PHYS_ID;
-	page_data.length = sizeof(phys_id);
-	page_data.i2c_address = MODULE_EEPROM_PHYS_ID_I2C_ADDR;
+	ethtool_cmis_page_init(&page_data, MODULE_EEPROM_PHYS_ID_PAGE,
+			       SFP_PHYS_ID, sizeof(phys_id));
 	page_data.data = &phys_id;
 
 	err = ops->get_module_eeprom_by_page(dev, &page_data, extack);
 	if (err < 0)
 		return err;
 
-	switch (phys_id) {
-	case SFF8024_ID_QSFP_DD:
-	case SFF8024_ID_OSFP:
-	case SFF8024_ID_DSFP:
-	case SFF8024_ID_QSFP_PLUS_CMIS:
-	case SFF8024_ID_SFP_DD_CMIS:
-	case SFF8024_ID_SFP_PLUS_CMIS:
-		INIT_WORK(&module_fw->work, module_flash_fw_work);
-		break;
-	default:
-		NL_SET_ERR_MSG(extack,
-			       "Module type does not support firmware flashing");
+	if (!module_is_cmis(phys_id)) {
+		NL_SET_ERR_MSG(extack, "Module type does not support firmware flashing");
 		return -EOPNOTSUPP;
 	}
 
+	INIT_WORK(&module_fw->work, module_flash_fw_work);
 	return 0;
 }
 
-- 
2.53.0

[RFC net-next 4/4] net/mlx5e: Implement set_module_eeprom_by_page ethtool callback

[Björn Töpel]

Add EEPROM write support by implementing the set_module_eeprom_by_page
ethtool_ops callback, mirroring the existing get_module_eeprom_by_page
read path. This enables the kernel's module loopback SET path which
requires both get and set callbacks.

The write path reuses the MCIA register (MLX5_REG_MCIA) via
mlx5_core_access_reg() with write=1 instead of write=0.

This seems to work, but I'd like some proper feedback from the mlx5
folks! ;-)

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  | 52 +++++++++++++------
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |  6 +--
 .../net/ethernet/mellanox/mlx5/core/port.c    | 34 ++++++++----
 3 files changed, 64 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 4a8dc85d5924..1dce67485651 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -2103,22 +2103,24 @@ static int mlx5e_get_module_eeprom(struct net_device *netdev,
 	return 0;
 }
 
-static int mlx5e_get_module_eeprom_by_page(struct net_device *netdev,
-					   const struct ethtool_module_eeprom *page_data,
-					   struct netlink_ext_ack *extack)
+static int __mlx5e_access_module_eeprom_by_page(struct net_device *netdev,
+						const struct ethtool_module_eeprom *page_data,
+						struct netlink_ext_ack *extack,
+						bool write)
 {
 	struct mlx5e_priv *priv = netdev_priv(netdev);
 	struct mlx5_module_eeprom_query_params query;
 	struct mlx5_core_dev *mdev = priv->mdev;
 	u8 *data = page_data->data;
-	int size_read;
+	int size_xfered;
 	u8 status = 0;
 	int i = 0;
 
 	if (!page_data->length)
 		return -EINVAL;
 
-	memset(data, 0, page_data->length);
+	if (!write)
+		memset(data, 0, page_data->length);
 
 	query.offset = page_data->offset;
 	query.i2c_address = page_data->i2c_address;
@@ -2126,28 +2128,47 @@ static int mlx5e_get_module_eeprom_by_page(struct net_device *netdev,
 	query.page = page_data->page;
 	while (i < page_data->length) {
 		query.size = page_data->length - i;
-		size_read = mlx5_query_module_eeprom_by_page(mdev, &query,
-							     data + i, &status);
+		size_xfered = mlx5_access_module_eeprom_by_page(mdev, &query,
+								data + i, &status,
+								write);
 
-		/* Done reading, return how many bytes was read */
-		if (!size_read)
+		if (!size_xfered)
 			return i;
 
-		if (size_read < 0) {
+		if (size_xfered < 0) {
 			NL_SET_ERR_MSG_FMT_MOD(
 				extack,
-				"Query module eeprom by page failed, read %u bytes, err %d, status %u",
-				i, size_read, status);
-			return size_read;
+				"%s module eeprom by page failed, %s %u bytes, err %d, status %u",
+				write ? "Set" : "Query",
+				write ? "wrote" : "read",
+				i, size_xfered, status);
+			return size_xfered;
 		}
 
-		i += size_read;
-		query.offset += size_read;
+		i += size_xfered;
+		query.offset += size_xfered;
 	}
 
 	return i;
 }
 
+static int mlx5e_get_module_eeprom_by_page(struct net_device *netdev,
+					   const struct ethtool_module_eeprom *page_data,
+					   struct netlink_ext_ack *extack)
+{
+	return __mlx5e_access_module_eeprom_by_page(netdev, page_data, extack, false);
+}
+
+static int mlx5e_set_module_eeprom_by_page(struct net_device *netdev,
+					   const struct ethtool_module_eeprom *page_data,
+					   struct netlink_ext_ack *extack)
+{
+	int err;
+
+	err = __mlx5e_access_module_eeprom_by_page(netdev, page_data, extack, true);
+	return err < 0 ? err : 0;
+}
+
 int mlx5e_ethtool_flash_device(struct mlx5e_priv *priv,
 			       struct ethtool_flash *flash)
 {
@@ -2776,6 +2797,7 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
 	.get_module_info   = mlx5e_get_module_info,
 	.get_module_eeprom = mlx5e_get_module_eeprom,
 	.get_module_eeprom_by_page = mlx5e_get_module_eeprom_by_page,
+	.set_module_eeprom_by_page = mlx5e_set_module_eeprom_by_page,
 	.flash_device      = mlx5e_flash_device,
 	.get_priv_flags    = mlx5e_get_priv_flags,
 	.set_priv_flags    = mlx5e_set_priv_flags,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
index b635b423d972..b69730727764 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
@@ -361,9 +361,9 @@ void mlx5_query_port_fcs(struct mlx5_core_dev *mdev, bool *supported,
 int mlx5_query_module_eeprom(struct mlx5_core_dev *dev,
 			     u16 offset, u16 size, u8 *data, u8 *status);
 int
-mlx5_query_module_eeprom_by_page(struct mlx5_core_dev *dev,
-				 struct mlx5_module_eeprom_query_params *params,
-				 u8 *data, u8 *status);
+mlx5_access_module_eeprom_by_page(struct mlx5_core_dev *dev,
+				  struct mlx5_module_eeprom_query_params *params,
+				  u8 *data, u8 *status, bool write);
 
 int mlx5_query_port_dcbx_param(struct mlx5_core_dev *mdev, u32 *out);
 int mlx5_set_port_dcbx_param(struct mlx5_core_dev *mdev, u32 *in);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/port.c b/drivers/net/ethernet/mellanox/mlx5/core/port.c
index ee8b9765c5ba..d68f9f9d7a80 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/port.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/port.c
@@ -369,9 +369,9 @@ static int mlx5_mcia_max_bytes(struct mlx5_core_dev *dev)
 	return (MLX5_CAP_MCAM_FEATURE(dev, mcia_32dwords) ? 32 : 12) * sizeof(u32);
 }
 
-static int mlx5_query_mcia(struct mlx5_core_dev *dev,
-			   struct mlx5_module_eeprom_query_params *params,
-			   u8 *data, u8 *status)
+static int __mlx5_access_mcia(struct mlx5_core_dev *dev,
+			      struct mlx5_module_eeprom_query_params *params,
+			      u8 *data, u8 *status, bool write)
 {
 	u32 in[MLX5_ST_SZ_DW(mcia_reg)] = {};
 	u32 out[MLX5_ST_SZ_DW(mcia_reg)];
@@ -388,8 +388,13 @@ static int mlx5_query_mcia(struct mlx5_core_dev *dev,
 	MLX5_SET(mcia_reg, in, page_number, params->page);
 	MLX5_SET(mcia_reg, in, i2c_device_address, params->i2c_address);
 
+	if (write) {
+		ptr = MLX5_ADDR_OF(mcia_reg, in, dword_0);
+		memcpy(ptr, data, size);
+	}
+
 	err = mlx5_core_access_reg(dev, in, sizeof(in), out,
-				   sizeof(out), MLX5_REG_MCIA, 0, 0);
+				   sizeof(out), MLX5_REG_MCIA, 0, write);
 	if (err)
 		return err;
 
@@ -399,12 +404,21 @@ static int mlx5_query_mcia(struct mlx5_core_dev *dev,
 		return -EIO;
 	}
 
-	ptr = MLX5_ADDR_OF(mcia_reg, out, dword_0);
-	memcpy(data, ptr, size);
+	if (!write) {
+		ptr = MLX5_ADDR_OF(mcia_reg, out, dword_0);
+		memcpy(data, ptr, size);
+	}
 
 	return size;
 }
 
+static int mlx5_query_mcia(struct mlx5_core_dev *dev,
+			   struct mlx5_module_eeprom_query_params *params,
+			   u8 *data, u8 *status)
+{
+	return __mlx5_access_mcia(dev, params, data, status, false);
+}
+
 int mlx5_query_module_eeprom(struct mlx5_core_dev *dev,
 			     u16 offset, u16 size, u8 *data, u8 *status)
 {
@@ -446,9 +460,9 @@ int mlx5_query_module_eeprom(struct mlx5_core_dev *dev,
 	return mlx5_query_mcia(dev, &query, data, status);
 }
 
-int mlx5_query_module_eeprom_by_page(struct mlx5_core_dev *dev,
-				     struct mlx5_module_eeprom_query_params *params,
-				     u8 *data, u8 *status)
+int mlx5_access_module_eeprom_by_page(struct mlx5_core_dev *dev,
+				      struct mlx5_module_eeprom_query_params *params,
+				      u8 *data, u8 *status, bool write)
 {
 	int err;
 
@@ -462,7 +476,7 @@ int mlx5_query_module_eeprom_by_page(struct mlx5_core_dev *dev,
 		return -EINVAL;
 	}
 
-	return mlx5_query_mcia(dev, params, data, status);
+	return __mlx5_access_mcia(dev, params, data, status, write);
 }
 
 static int mlx5_query_port_pvlc(struct mlx5_core_dev *dev, u32 *pvlc,
-- 
2.53.0

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

On Thu, 19 Feb 2026 at 14:01, Björn Töpel <bjorn@kernel.org> wrote:
>
> Hi!
>
> Background
> ==========
>
> This series adds initial ethtool support for CMIS loopback.

For the brave, there's userland support here [1], and use, e.g., as below:

  | [root@machine ~/ethtool]# ./ethtool --show-module eth0
  | Module parameters for eth0:
  | loopback-capabilities: media-side-input, host-side-input
  | loopback-enabled: none
  | [root@machine ~/ethtool]# ./ethtool --set-module eth0
loopback-enabled host-side-input
  | netlink error: Netdevice is up, so setting loopback is not permitted
  | netlink error: Device or resource busy
  | [root@machine ~/ethtool]# ip link set dev eth0 down
  | [root@machine ~/ethtool]# ./ethtool --set-module eth0
loopback-enabled host-side-input
  | [root@machine ~/ethtool]# ./ethtool --show-module eth0
  | Module parameters for eth0:
  | loopback-capabilities: media-side-input, host-side-input
  | loopback-enabled: host-side-input
  | [root@machine ~/ethtool]# ip link set dev eth0 up


[1] https://github.com/fb-bjorn/ethtool/tree/module-loopback

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Andrew Lunn]

On Thu, Feb 19, 2026 at 02:00:41PM +0100, Björn Töpel wrote:
> Hi!
> 
> Background
> ==========
> 
> This series adds initial ethtool support for CMIS loopback.
 
> Related work
> ============
> 
> * New loopback modes [1].
> * PHY loopback [2]

Hi Björn

Great to see you looked around at the problem space.

> [2] https://lore.kernel.org/netdev/20240911212713.2178943-1-maxime.chevallier@bootlin.com/

Quoting myself from this thread:

> We might want to take a step back and think about loopback some more.
>
> Loopback can be done at a number of points in the device(s). Some
> Marvell PHYs can do loopback in the PHY PCS layer. Some devices also
> support loopback in the PHY SERDES layer. I've not seen it for Marvell
> devices, but maybe some PHYs allow loopback much closer to the line?
> And i expect some MAC PCS allow loopback.
> 
> So when talking about loopback, we might also want to include the
> concept of where the loopback occurs, and maybe it needs to be a NIC
> wide concept, not a PHY concept?

I still think this is true. We want a generic kAPI for loopback, not a
PHY loopback kAPI, and a MAC loopback kAPI, a PCS loopback kAPI, and
an SFP loopback kAPI, and a CAN bus transceiver loopback kAPI,
assuming CAN bus supports loopback?

So i think we need one ethtool API for loopback. We probably want an
API call to enumerate what loopbacks are supported for a netdev. The
MAC will fill in bits indicating what it can do. If the MAC has a PCS,
it will ask the PCS what it can do. If there is a PHY, it will ask the
PHY to fill in the bits indicating what it can do, if there is an SFP,
it will ask it what it can do, and if there is a CAN bus transceiver,
it will fill in its bits. And we probably want two values for each
loopback location, is it looping the media side, or the MAC side?

So the return value lists all the different loopbacks associated to a
netdev.

And then we need a set operation, to enable/disable a specific
loopback, and a get operation to return the status of all the
different loopbacks of a netdev. The MAC will again need to call into
the PCS, the PHY, the SFP to implement these.

I'm not saying you need to implement all these, you just need to make
what you do implement generic, and plumb it through the network stack
so that others can later easily add PHY, PCS, and MAC loopback
support. And from your background research, you know others are
interested in this, so you might be able get some help with parts you
are not particularly interested in.

	Andrew

Re: [RFC net-next 2/4] ethtool: module: Add CMIS loopback GET/SET support

[Andrew Lunn]

> + * struct ethtool_module_loopback_params - module loopback parameters
> + * @caps: The loopback types, %ETHTOOL_MODULE_LOOPBACK_TYPES_*,
> + *	supported by the module.
> + * @enabled: The loopback types, %ETHTOOL_MODULE_LOOPBACK_TYPES_*,
> + *	currently enabled by the module.
> + */
> +struct ethtool_module_loopback_params {
> +	u32 caps;
> +	u32 enabled;

You probably should be using ethtools bitset API for this.

    Andrew

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Jakub Kicinski]

On Thu, 19 Feb 2026 16:51:47 +0100 Andrew Lunn wrote:
> > We might want to take a step back and think about loopback some more.
> >
> > Loopback can be done at a number of points in the device(s). Some
> > Marvell PHYs can do loopback in the PHY PCS layer. Some devices also
> > support loopback in the PHY SERDES layer. I've not seen it for Marvell
> > devices, but maybe some PHYs allow loopback much closer to the line?
> > And i expect some MAC PCS allow loopback.
> > 
> > So when talking about loopback, we might also want to include the
> > concept of where the loopback occurs, and maybe it needs to be a NIC
> > wide concept, not a PHY concept?  
> 
> I still think this is true. We want a generic kAPI for loopback, not a
> PHY loopback kAPI, and a MAC loopback kAPI, a PCS loopback kAPI, and
> an SFP loopback kAPI, and a CAN bus transceiver loopback kAPI,
> assuming CAN bus supports loopback?
> 
> So i think we need one ethtool API for loopback. We probably want an
> API call to enumerate what loopbacks are supported for a netdev. The
> MAC will fill in bits indicating what it can do. If the MAC has a PCS,
> it will ask the PCS what it can do. If there is a PHY, it will ask the
> PHY to fill in the bits indicating what it can do, if there is an SFP,
> it will ask it what it can do, and if there is a CAN bus transceiver,
> it will fill in its bits. And we probably want two values for each
> loopback location, is it looping the media side, or the MAC side?
> 
> So the return value lists all the different loopbacks associated to a
> netdev.
> 
> And then we need a set operation, to enable/disable a specific
> loopback, and a get operation to return the status of all the
> different loopbacks of a netdev. The MAC will again need to call into
> the PCS, the PHY, the SFP to implement these.
> 
> I'm not saying you need to implement all these, you just need to make
> what you do implement generic, and plumb it through the network stack
> so that others can later easily add PHY, PCS, and MAC loopback
> support. And from your background research, you know others are
> interested in this, so you might be able get some help with parts you
> are not particularly interested in.

Something like:

  struct {
     enum type;   // MAC, PHY, SFP
     int type_id; // if type=PHY - phy id
     int depth;   // counting from CPU, first loopback opportunity is 1
                  // second is 2, etc.
     bool direction; // towards CPU/host vs towards network
     char name[16];  // "pcs", "far", "near", "post-fec", whatever
  }

?

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

Andrew!

Thanks for the swift response!

On Thu, 19 Feb 2026 at 16:51, Andrew Lunn <andrew@lunn.ch> wrote:

> Great to see you looked around at the problem space.
>
> > [2] https://lore.kernel.org/netdev/20240911212713.2178943-1-maxime.chevallier@bootlin.com/
>
> Quoting myself from this thread:
>
> > We might want to take a step back and think about loopback some more.
> >
> > Loopback can be done at a number of points in the device(s). Some
> > Marvell PHYs can do loopback in the PHY PCS layer. Some devices also
> > support loopback in the PHY SERDES layer. I've not seen it for Marvell
> > devices, but maybe some PHYs allow loopback much closer to the line?
> > And i expect some MAC PCS allow loopback.
> >
> > So when talking about loopback, we might also want to include the
> > concept of where the loopback occurs, and maybe it needs to be a NIC
> > wide concept, not a PHY concept?
>
> I still think this is true. We want a generic kAPI for loopback, not a
> PHY loopback kAPI, and a MAC loopback kAPI, a PCS loopback kAPI, and
> an SFP loopback kAPI, and a CAN bus transceiver loopback kAPI,
> assuming CAN bus supports loopback?
>
> So i think we need one ethtool API for loopback. We probably want an
> API call to enumerate what loopbacks are supported for a netdev. The
> MAC will fill in bits indicating what it can do. If the MAC has a PCS,
> it will ask the PCS what it can do. If there is a PHY, it will ask the
> PHY to fill in the bits indicating what it can do, if there is an SFP,
> it will ask it what it can do, and if there is a CAN bus transceiver,
> it will fill in its bits. And we probably want two values for each
> loopback location, is it looping the media side, or the MAC side?
>
> So the return value lists all the different loopbacks associated to a
> netdev.
>
> And then we need a set operation, to enable/disable a specific
> loopback, and a get operation to return the status of all the
> different loopbacks of a netdev. The MAC will again need to call into
> the PCS, the PHY, the SFP to implement these.
>
> I'm not saying you need to implement all these, you just need to make
> what you do implement generic, and plumb it through the network stack
> so that others can later easily add PHY, PCS, and MAC loopback
> support. And from your background research, you know others are
> interested in this, so you might be able get some help with parts you
> are not particularly interested in.

All good points here; Thanks for the elaborative feedback. I like the
idea of a generic loopback API. Back to the drawing board!


Björn

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Andrew Lunn]

> Something like:
> 
>   struct {
>      enum type;   // MAC, PHY, SFP
>      int type_id; // if type=PHY - phy id
>      int depth;   // counting from CPU, first loopback opportunity is 1
>                   // second is 2, etc.
>      bool direction; // towards CPU/host vs towards network
>      char name[16];  // "pcs", "far", "near", "post-fec", whatever
>   }

Lets see what comes from the drawing board, but i was more thinking
about expanding the bitmap this proposal already has, extending it to
other layers. As use cases are implemented, we define the bits needed
in the map. The ethtool kAPI has the needed infrastructure to map bits
to names, it is used for link modes etc, and that can be used here. So
the ethtool(1) part should be reasonably generic.

    Andrew

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Jakub Kicinski]

On Fri, 20 Feb 2026 15:18:40 +0100 Andrew Lunn wrote:
> > Something like:
> > 
> >   struct {
> >      enum type;   // MAC, PHY, SFP
> >      int type_id; // if type=PHY - phy id
> >      int depth;   // counting from CPU, first loopback opportunity is 1
> >                   // second is 2, etc.
> >      bool direction; // towards CPU/host vs towards network
> >      char name[16];  // "pcs", "far", "near", "post-fec", whatever
> >   }  
> 
> Lets see what comes from the drawing board, but i was more thinking
> about expanding the bitmap this proposal already has, extending it to
> other layers.

IIUC the bitmap this proposal has is basically a product of
direction x depth: [host, network] x [nearest, furthest]
plus its scoped to SFP.

> As use cases are implemented, we define the bits needed
> in the map. 

Sure, but if we are creating a dedicated API we should decompose 
the information from the start. Direction, and entity (MAC, PHY, SFP)
don't have to be part of the bitmap?

> The ethtool kAPI has the needed infrastructure to map bits
> to names, it is used for link modes etc, and that can be used here. So
> the ethtool(1) part should be reasonably generic.

Dunno if link modes are the right point of reference. Link mode is 
a combination of various parameters which must match on both sides
exactly. For the loopback the config is very simple, the expressiveness
is needed to explain where the configuration is applied.

IOW for link modes it's important to have an ID for the combination of
all params to easily check if the whole thing is as expected.
For loopback it's easier to think of it as traversing attribute by
attribute: MAC / PHY / SFP -> which one -> which depth -> which dir.
Single id has no benefit and would be cumbersome to define.

Or at least that's my intuition, I haven't use loopback much myself.

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

Andrew/Kuba!

Thanks for having a look!

On Fri, 20 Feb 2026 at 22:12, Jakub Kicinski <kuba@kernel.org> wrote:

>> > Something like:
>> >
>> >   struct {
>> >      enum type;   // MAC, PHY, SFP
>> >      int type_id; // if type=PHY - phy id
>> >      int depth;   // counting from CPU, first loopback opportunity is 1
>> >                   // second is 2, etc.
>> >      bool direction; // towards CPU/host vs towards network
>> >      char name[16];  // "pcs", "far", "near", "post-fec", whatever
>> >   }
>>
>> Lets see what comes from the drawing board, but i was more thinking
>> about expanding the bitmap this proposal already has, extending it to
>> other layers.
>
>IIUC the bitmap this proposal has is basically a product of
>direction x depth: [host, network] x [nearest, furthest]
>plus its scoped to SFP.

(Digging into different ways to loopback, rather than drawing board,
but here goes! ;-))

I'd agree with Jakub here, unless I'm not getting the details of what
you mean, Andrew. Sounds like we'd end up with a huge bitmap? I
suggest tweaking Jakub's idea to something like:

/* loopback.c: A new ETHTOOL_MSG_LOOPBACK_{GET,SET} */

/* Loopback layers/scope
enum ethtool_loopback_layer {
    ETHTOOL_LB_LAYER_SW = 0,    /* Software/Kernel stack loopback */
    ETHTOOL_LB_LAYER_MAC,        /* MAC/Controller internal */
    ETHTOOL_LB_LAYER_PCS,        /* Physical Coding Sublayer (Digital) */
    ETHTOOL_LB_LAYER_PMA,        /* SerDes / Analog-Digital boundary */
    ETHTOOL_LB_LAYER_PMD,        /* Transceiver / Module internal */
    ETHTOOL_LB_LAYER_EXT,        /* External physical plug/cable */
};

/* Loopback Direction (XXX is local/remote easier to understand?) */
enum ethtool_loopback_dir {
    ETHTOOL_LB_DIR_NEAR_END = 0,    /* Host -> Loop -> Host  */
    ETHTOOL_LB_DIR_FAR_END,        /* Line -> Loop -> Line  */
};

struct ethtool_loopback_layer_cfg {
    enum ethtool_loopback_layer layer;    /* ETHTOOL_LB_L_MAC, etc. */
    enum ethtool_loopback_dir direction;  /* NEAR or FAR */
    u32 lane_mask;                        /* Specific lanes */
    u32  flags;                           /* patterns? reserved... */
    bool enabled;
    char name[16];
};

struct ethtool_loopback_cfg {
    struct ethtool_loopback_layer_cfg *entries;
    u32 num_layers;
};

struct ethtool_ops {
    /* ... */

    /* Query which layers/lane-combos are physically possible */
    int (*get_loopback_caps)(struct net_device *dev,
                             struct ethtool_loopback_cfg *caps);

    /* Get current active status for all layers */
    int (*get_loopback_state)(struct net_device *dev,
                              struct ethtool_loopback_cfg *state);

    /* Set one or more layer/lane configurations atomically */
    int (*set_loopback)(struct net_device *dev,
                        const struct ethtool_loopback_cfg *cfg,
                        struct netlink_ext_ack *extack);
};

As for layers; EXT vs PMD? EXT could be a loopback plug, whereas PMD
would be CMIS, or whatever the driver detects.

Userland would be something like:

# ethtool --show-loopback eth0
Loopback Status for eth0:
  Layer: SW  | State: OFF
  Layer: MAC   | State: OFF
  Layer: PMA   | State: ON  | Lanes: 0x1 (Lane 0) | Direction: Near-End (Local)
  Layer: PMD   | State: ON  | Lanes: 0xF (All)    | Direction: Far-End (Remote)
  Layer: EXT   | State: ON  | Detected: External Loopback Plug

# ethtool --set-loopback <dev> [layer[:lanes][:direction]] ... [off]

# # Simple MAC loopback:
# ethtool --set-loopback eth0 mac (Defaults: lanes=all, dir=near)
# # Specific SerDes (PMA) lane:
# ethtool --set-loopback eth0 pma:lane0
# # Complex multi-layer (PMA Near + PMD Far):
# ethtool --set-loopback eth0 pma:0x1:near pmd:all:far
# # Disable all loopbacks:
ethtool --set-loopback eth0 off

Thoughts? Is this somewhat close to what you had in mind, Andrew?

I'm far from an expert on the details here, so the folks with more
knowledge, please chime in!


Cheers,
Björn

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Andrew Lunn]

> /* Loopback Direction (XXX is local/remote easier to understand?) */
> enum ethtool_loopback_dir {
>     ETHTOOL_LB_DIR_NEAR_END = 0,    /* Host -> Loop -> Host  */
>     ETHTOOL_LB_DIR_FAR_END,        /* Line -> Loop -> Line  */
> };

I like host->loop->host, it is much clearer than NEAR_END or
FAR_END. Where there is space, would use this description, even if it
is a bit verbose.

> struct ethtool_loopback_layer_cfg {
>     enum ethtool_loopback_layer layer;    /* ETHTOOL_LB_L_MAC, etc. */
>     enum ethtool_loopback_dir direction;  /* NEAR or FAR */
>     u32 lane_mask;                        /* Specific lanes */
>     u32  flags;                           /* patterns? reserved... */
>     bool enabled;
>     char name[16];

What would name be used for. I don't see it in your example. The nice
thing about netlink messages is that they are extendable, unlike
system calls. If there is no current use for a field, don't add it. It
can be added later when actually needed. So i would drop flags and
name.

Does CMIS, when used with a splitter cable, allow you to set loopback
on lanes? What is your use case for lane_mask?

> };
> 
> struct ethtool_loopback_cfg {
>     struct ethtool_loopback_layer_cfg *entries;
>     u32 num_layers;

What is num_layers used for?

> };
> 
> struct ethtool_ops {
>     /* ... */
> 
>     /* Query which layers/lane-combos are physically possible */
>     int (*get_loopback_caps)(struct net_device *dev,
>                              struct ethtool_loopback_cfg *caps);
> 
>     /* Get current active status for all layers */
>     int (*get_loopback_state)(struct net_device *dev,
>                               struct ethtool_loopback_cfg *state);
> 
>     /* Set one or more layer/lane configurations atomically */
>     int (*set_loopback)(struct net_device *dev,
>                         const struct ethtool_loopback_cfg *cfg,
>                         struct netlink_ext_ack *extack);
> };
> 
> As for layers; EXT vs PMD? EXT could be a loopback plug, whereas PMD
> would be CMIS, or whatever the driver detects.
> 
> Userland would be something like:
> 
> # ethtool --show-loopback eth0
> Loopback Status for eth0:
>   Layer: SW  | State: OFF
>   Layer: MAC   | State: OFF
>   Layer: PMA   | State: ON  | Lanes: 0x1 (Lane 0) | Direction: Near-End (Local)
>   Layer: PMD   | State: ON  | Lanes: 0xF (All)    | Direction: Far-End (Remote)

ETHTOOL_LINK_MODE_800000baseKR8_Full_BIT has 8 lanes, so 0xff would be
All in this case. Lanes adds quite a bit of complexity. Do we have a
real use case for it?

>   Layer: EXT   | State: ON  | Detected: External Loopback Plug
> 
> # ethtool --set-loopback <dev> [layer[:lanes][:direction]] ... [off]
> 
> # # Simple MAC loopback:
> # ethtool --set-loopback eth0 mac (Defaults: lanes=all, dir=near)
> # # Specific SerDes (PMA) lane:
> # ethtool --set-loopback eth0 pma:lane0
> # # Complex multi-layer (PMA Near + PMD Far):
> # ethtool --set-loopback eth0 pma:0x1:near pmd:all:far

Is this something we actually want? Again it adds complexity,
especially in the error handling, when pma:0x1:near works, but
pmd:all:far fails, and you need to unwind the pma:0x1:near. Is there a
use case for atomically setting two loopbacks, rather than having the
user make two different calls?

> # # Disable all loopbacks:
> ethtool --set-loopback eth0 off
> 
> Thoughts? Is this somewhat close to what you had in mind, Andrew?

I'm happy with the basic shape of this. I just needs the details
nailing down.

    Andrew

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

Hey!

On Mon, 23 Feb 2026 at 15:30, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > /* Loopback Direction (XXX is local/remote easier to understand?) */
> > enum ethtool_loopback_dir {
> >     ETHTOOL_LB_DIR_NEAR_END = 0,    /* Host -> Loop -> Host  */
> >     ETHTOOL_LB_DIR_FAR_END,        /* Line -> Loop -> Line  */
> > };
>
> I like host->loop->host, it is much clearer than NEAR_END or
> FAR_END. Where there is space, would use this description, even if it
> is a bit verbose.

Ok!

> > struct ethtool_loopback_layer_cfg {
> >     enum ethtool_loopback_layer layer;    /* ETHTOOL_LB_L_MAC, etc. */
> >     enum ethtool_loopback_dir direction;  /* NEAR or FAR */
> >     u32 lane_mask;                        /* Specific lanes */
> >     u32  flags;                           /* patterns? reserved... */
> >     bool enabled;
> >     char name[16];
>
> What would name be used for. I don't see it in your example. The nice
> thing about netlink messages is that they are extendable, unlike
> system calls. If there is no current use for a field, don't add it. It
> can be added later when actually needed. So i would drop flags and
> name.

Yeah, good call (I dropped flags on my local hack), name *and*
lane_mask. More on that below.

> Does CMIS, when used with a splitter cable, allow you to set loopback
> on lanes? What is your use case for lane_mask?

At least the spec exposed that it could be supported. Thinking more
about it, I think it can be added later if someone cares about it.
IOW, flags, name, and lane_mask shouldn't be part of the initial
patches.

> > };
> >
> > struct ethtool_loopback_cfg {
> >     struct ethtool_loopback_layer_cfg *entries;
> >     u32 num_layers;
>
> What is num_layers used for?

I'll change the _cfg to have a proper array instead. The idea is that
_cfg is a set of layers (MAC, PMA, etc.). The num_layers is the number
of entries. I'll make sure this is more obvious.

...

> > # ethtool --show-loopback eth0
> > Loopback Status for eth0:
> >   Layer: SW  | State: OFF
> >   Layer: MAC   | State: OFF
> >   Layer: PMA   | State: ON  | Lanes: 0x1 (Lane 0) | Direction: Near-End (Local)
> >   Layer: PMD   | State: ON  | Lanes: 0xF (All)    | Direction: Far-End (Remote)
>
> ETHTOOL_LINK_MODE_800000baseKR8_Full_BIT has 8 lanes, so 0xff would be
> All in this case. Lanes adds quite a bit of complexity. Do we have a
> real use case for it?

I don't. As outlined above -- let's leave it out for now. If somebody
needs it in the future, we can have the discussion then.

> >   Layer: EXT   | State: ON  | Detected: External Loopback Plug
> >
> > # ethtool --set-loopback <dev> [layer[:lanes][:direction]] ... [off]
> >
> > # # Simple MAC loopback:
> > # ethtool --set-loopback eth0 mac (Defaults: lanes=all, dir=near)
> > # # Specific SerDes (PMA) lane:
> > # ethtool --set-loopback eth0 pma:lane0
> > # # Complex multi-layer (PMA Near + PMD Far):
> > # ethtool --set-loopback eth0 pma:0x1:near pmd:all:far
>
> Is this something we actually want? Again it adds complexity,
> especially in the error handling, when pma:0x1:near works, but
> pmd:all:far fails, and you need to unwind the pma:0x1:near. Is there a
> use case for atomically setting two loopbacks, rather than having the
> user make two different calls?

Good point! Simpler is better.

> > # # Disable all loopbacks:
> > ethtool --set-loopback eth0 off
> >
> > Thoughts? Is this somewhat close to what you had in mind, Andrew?
>
> I'm happy with the basic shape of this. I just needs the details
> nailing down.

Ok! I'll roll something we can discuss more. Thanks for all input!


Björn

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Jakub Kicinski]

On Sun, 22 Feb 2026 20:58:30 +0100 Björn Töpel wrote:
> /* Loopback layers/scope
> enum ethtool_loopback_layer {
>     ETHTOOL_LB_LAYER_SW = 0,    /* Software/Kernel stack loopback */

What would that be? :)

>     ETHTOOL_LB_LAYER_MAC,        /* MAC/Controller internal */
>     ETHTOOL_LB_LAYER_PCS,        /* Physical Coding Sublayer (Digital) */
>     ETHTOOL_LB_LAYER_PMA,        /* SerDes / Analog-Digital boundary */
>     ETHTOOL_LB_LAYER_PMD,        /* Transceiver / Module internal */

In my mind the "layer" was supposed to tell core which driver to send
the request to. Same concept is used in the timestamp source selection.
PCS/PMA/PMD is both too fine grained when you have multiple PHYs in the
path, and does not cover all the possible loopback points.

>     ETHTOOL_LB_LAYER_EXT,        /* External physical plug/cable */

is EXT used somewhere to mean SFP already?

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

Good feedback, Kuba! ...more thinking...

On Tue, 24 Feb 2026 at 00:04, Jakub Kicinski <kuba@kernel.org> wrote:
...
As a reference, there's a v2-ish version here (API I suggested +
netdevsim plumbing) [1].

What I discovered was that the "layer"/IEEE abstraction I suggested
didn't even fit nicely with the CMIS module loopback work -- I had to
split PMD into PMD-HOST/PMD-MODULE to fit the CMIS spec, which felt
like a hack.

I've obviously taken too high-level a "PCIe device" view of the world.
Looking at Maxime's slides from netdev 0x17 [2] reveals a more
complicated world; multiple PHY-like things in the path (onboard PHY,
module, external PHY, ...). My model doesn't cover what Maxime
outlines.

Instead, like Jakub hinted at, userspace should reason in terms of
"loopback entities" (or IDs) that correspond to "places where loopback
can be applied”, not in terms of PCS/PMA/PMD topology.

A loopback entity would have:
    an ID / type (which matches a driver),
    a direction,
    an optional descriptive name.

Reiterating what Jakub said earlier, using hwstamp-source as a
precedent, you could imagine something like:

  struct {
     enum type;   // MAC, PHY, SFP
     int type_id; // if type=PHY - phy id
     int depth;   // counting from CPU, first loopback opportunity is 1
                  // second is 2, etc.
     bool direction; // towards CPU/host vs towards network
     char name[16];  // "pcs", "far", "near", "post-fec", whatever
  }

I'm not sure that depth/name are actually useful here.

Concretely, that could look like this in ethtool:

  /* What kind of kernel object owns this loopback. */
  enum ethtool_loopback_owner {
          ETHTOOL_LB_OWNER_MAC,       /* struct net_device / MAC driver */
          ETHTOOL_LB_OWNER_PHY,       /* struct phy_device (phylib PHY) */
          ETHTOOL_LB_OWNER_MODULE,    /* module / SFP driver, e.g. CMIS */
  };

  enum ethtool_loopback_dir {
          ETHTOOL_LB_DIR_HOST_LOOP_HOST,
          ETHTOOL_LB_DIR_LINE_LOOP_LINE,
  };

  /* One "loopback entity" associated with the netdev. */
  struct ethtool_loopback_entry {
          u32 id;                               /* per-netdev */
          enum ethtool_loopback_owner owner;
          u32 depth;                            /* ??? */
          enum ethtool_loopback_dir direction;  /* enum ethtool_loopback_dir */
          bool  enabled;
          char  name[16];                       /* ??? "mac",
"phy-pcs", "module-far", ... */
  };

  /* Aggregate for GET: list all possible endpoints + their status. */
  struct ethtool_loopback_state {
          struct ethtool_loopback_entry entries[16];
          unsigned int n_entries;
  };

From a driver POV:
* The MAC driver can add its own MAC-level loopback entity (owner =
  MAC).
* If there is one or more phydevs, it can call into a phylib helper
  (e.g. phy_get_loopback_state()) to append PHY entities (owner =
  PHY).
* If there is a CMIS module, it can append its entities (owner = MODULE).

# ethtool --show-loopback eth0
Loopback endpoints for eth0:
  id  owner  depth  direction          name       enabled
   0  mac       1   host->loop->host   mac        off
   1  phy       2   host->loop->host   phy-pcs    on
   2  module    3   line->loop->line   cmis-far   off

# Enable endpoint 2
ethtool --set-loopback eth0 id 2 on


Does this model make more sense, especially for the multi-PHY / CMIS
cases Maxime described?


Björn

[1] https://github.com/fb-bjorn/linux/compare/master...fb-bjorn:linux:ethtool-loopback-rfc-v2
[2] https://netdevconf.info/0x17/docs/netdev-0x17-paper2-talk-slides/multi-port-multi-phy-interfaces.pdf

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Andrew Lunn]

> # ethtool --show-loopback eth0
> Loopback endpoints for eth0:
>   id  owner  depth  direction          name       enabled
>    0  mac       1   host->loop->host   mac        off
>    1  phy       2   host->loop->host   phy-pcs    on
>    2  module    3   line->loop->line   cmis-far   off
> 
> # Enable endpoint 2
> ethtool --set-loopback eth0 id 2 on

We have to be careful about what is ABI here. Initially you plan to
implement module, so you will have:

# ethtool --show-loopback eth0
Loopback endpoints for eth0:
  id  owner  depth  direction          name       enabled
   0  module    1   line->loop->line   cmis-far   off

ethtool --set-loopback eth0 id 0 on

And then somebody implements loopback at the mac:

# ethtool --show-loopback eth0
Loopback endpoints for eth0:
  id  owner  depth  direction          name       enabled
   0  mac       1   host->loop->host   mac        off
   1  module    2   line->loop->line   cmis-far   off

You script doing

ethtool --set-loopback eth0 id 0 on

Suddenly does something else.

Is this an ABI break? How do we make this reliable so implementing
more loopbacks at different levels does not change how you use
--set-loopback?

	Andrew

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

On Wed, 25 Feb 2026 at 05:05, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > # ethtool --show-loopback eth0
> > Loopback endpoints for eth0:
> >   id  owner  depth  direction          name       enabled
> >    0  mac       1   host->loop->host   mac        off
> >    1  phy       2   host->loop->host   phy-pcs    on
> >    2  module    3   line->loop->line   cmis-far   off
> >
> > # Enable endpoint 2
> > ethtool --set-loopback eth0 id 2 on
>
> We have to be careful about what is ABI here. Initially you plan to
> implement module, so you will have:
>
> # ethtool --show-loopback eth0
> Loopback endpoints for eth0:
>   id  owner  depth  direction          name       enabled
>    0  module    1   line->loop->line   cmis-far   off
>
> ethtool --set-loopback eth0 id 0 on
>
> And then somebody implements loopback at the mac:
>
> # ethtool --show-loopback eth0
> Loopback endpoints for eth0:
>   id  owner  depth  direction          name       enabled
>    0  mac       1   host->loop->host   mac        off
>    1  module    2   line->loop->line   cmis-far   off
>
> You script doing
>
> ethtool --set-loopback eth0 id 0 on
>
> Suddenly does something else.

Indeed!

> Is this an ABI break? How do we make this reliable so implementing
> more loopbacks at different levels does not change how you use
> --set-loopback?

Isn't this somewhat similar to what we have with ifindex/phy_index,
but potentially unstable when modules are swapped/changed?

Instead of ids, use string name and/or topology indices (e.g.
phy_index)? All three -- owner, phy_index, name tuple?


Björn

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Maxime Chevallier]

Hi Björn,

On 22/02/2026 20:58, Björn Töpel wrote:

> # # Simple MAC loopback:
> # ethtool --set-loopback eth0 mac (Defaults: lanes=all, dir=near)
> # # Specific SerDes (PMA) lane:
> # ethtool --set-loopback eth0 pma:lane0
> # # Complex multi-layer (PMA Near + PMD Far):
> # ethtool --set-loopback eth0 pma:0x1:near pmd:all:far
> # # Disable all loopbacks:
> ethtool --set-loopback eth0 off
> 
> Thoughts? Is this somewhat close to what you had in mind, Andrew?
> 
> I'm far from an expert on the details here, so the folks with more
> knowledge, please chime in!

I'm very sorry not to have looked into this yet, I'm having some family
events to handle right-now but I hope to be back next monday to take a
close look at this :)

Thanks for this work,

Maxime

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

Maxime!

On Wed, 25 Feb 2026 at 11:22, Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:

> I'm very sorry not to have looked into this yet, I'm having some family
> events to handle right-now but I hope to be back next monday to take a
> close look at this :)

No rush, and thank you! Take care of your family -- the patches will
still be around when you're back!

Cheers,
Björn

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Andrew Lunn]

> > Suddenly does something else.
> 
> Indeed!
> 
> > Is this an ABI break? How do we make this reliable so implementing
> > more loopbacks at different levels does not change how you use
> > --set-loopback?
> 
> Isn't this somewhat similar to what we have with ifindex/phy_index,
> but potentially unstable when modules are swapped/changed?

If you hot plug hardware, a new PHY pops into existence, i don't think
it is too unreasonable for the hot plugable parts to change ids. I
would however expect the fixed parts to keep there IDs.

But here we are talking about software, a kernel upgrade/downgrade
causing the IDs to change.
 
> Instead of ids, use string name and/or topology indices (e.g.
> phy_index)? All three -- owner, phy_index, name tuple?

Probably.

	Andrew

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Maxime Chevallier]

Hello Andrew, BJörn,

On 25/02/2026 14:14, Andrew Lunn wrote:
>>> Suddenly does something else.
>>
>> Indeed!
>>
>>> Is this an ABI break? How do we make this reliable so implementing
>>> more loopbacks at different levels does not change how you use
>>> --set-loopback?
>>
>> Isn't this somewhat similar to what we have with ifindex/phy_index,
>> but potentially unstable when modules are swapped/changed?
> 
> If you hot plug hardware, a new PHY pops into existence, i don't think
> it is too unreasonable for the hot plugable parts to change ids. I
> would however expect the fixed parts to keep there IDs.

That's indeed the phy index behaviour.

> 
> But here we are talking about software, a kernel upgrade/downgrade
> causing the IDs to change.
>  
>> Instead of ids, use string name and/or topology indices (e.g.
>> phy_index)? All three -- owner, phy_index, name tuple?

The overall approach after all these discussions sounds fine to me, I do
think that the index of the component that does the loopback needs to be
there somewhere, when relevant.

Either through a name string, or a combo of an enum indicating the
component type (MAC/PHY/Module/etc.) + its index. I think it's safe to
assume that indices will fit in u32 ?

something like :

# MAC PCS loopback
ethtool --set-loopback eth0 loc mac name pcs

# PHY id 2 PMA loopback (I'm making things up here)
ethtool --set-loopback eth0 loc phy id 2 name pma

That way we can extend that fairly easily for, say, combo-port devices
where we could select which of the port we want to loopback :)

Maxime

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Björn Töpel]

Hey!

On Mon, 2 Mar 2026 at 10:01, Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:

> The overall approach after all these discussions sounds fine to me, I do
> think that the index of the component that does the loopback needs to be
> there somewhere, when relevant.
>
> Either through a name string, or a combo of an enum indicating the
> component type (MAC/PHY/Module/etc.) + its index. I think it's safe to
> assume that indices will fit in u32 ?
>
> something like :
>
> # MAC PCS loopback
> ethtool --set-loopback eth0 loc mac name pcs
>
> # PHY id 2 PMA loopback (I'm making things up here)
> ethtool --set-loopback eth0 loc phy id 2 name pma
>
> That way we can extend that fairly easily for, say, combo-port devices
> where we could select which of the port we want to loopback :)

Ok! I'll spin a new version with this in mind. To improve my mental
model, could you give an example how you would use a combo-port from a
userland perspective?


Cheers,
Björn

Re: [RFC net-next 0/4] ethtool: CMIS module diagnostic loopback support

[Maxime Chevallier]

Hi Björn,

On 04/03/2026 16:52, Björn Töpel wrote:
> Hey!
> 
> On Mon, 2 Mar 2026 at 10:01, Maxime Chevallier
> <maxime.chevallier@bootlin.com> wrote:
> 
>> The overall approach after all these discussions sounds fine to me, I do
>> think that the index of the component that does the loopback needs to be
>> there somewhere, when relevant.
>>
>> Either through a name string, or a combo of an enum indicating the
>> component type (MAC/PHY/Module/etc.) + its index. I think it's safe to
>> assume that indices will fit in u32 ?
>>
>> something like :
>>
>> # MAC PCS loopback
>> ethtool --set-loopback eth0 loc mac name pcs
>>
>> # PHY id 2 PMA loopback (I'm making things up here)
>> ethtool --set-loopback eth0 loc phy id 2 name pma
>>
>> That way we can extend that fairly easily for, say, combo-port devices
>> where we could select which of the port we want to loopback :)
> 
> Ok! I'll spin a new version with this in mind. To improve my mental
> model, could you give an example how you would use a combo-port from a
> userland perspective?

Of course :)

Considering this setup :

 +-----+     +-----+
 | MAC |     | PHY |----- SFP
 |     |-----|     |----- RJ45
 +-----+     +-----+

It's still WIP but the current state of what I have in the pipe looks like :

# List the ports
ethtool --show-ports eth0

Port for eth10:       # <- This port represents the RJ45 port of the PHY
	Port id: 1
	Supported link modes:  10baseT/Half 10baseT/Full
	                       100baseT/Half 100baseT/Full
	                       1000baseT/Full
	                       10000baseT/Full
	                       2500baseT/Full
	                       5000baseT/Full
	Port type: mdi
	Active: yes
	Link: up

Port for eth1:          # <- This port represents the SFP cage
	Port id: 2
	Vacant: no
	Supported MII interfaces : 10gbase-r
	Port type: sfp
	Active: no

Port for eth1:          # <- This port represents the SFP module inside the cage
	Port id: 4
	Supported link modes:  10000baseCR/Full
	Port type: mdi
	Active: no
	Link: up


# Select the SFP port as the active one (note that we could either use
port 2 or 4 here for the same result) :

ethtool --set-port eth0 id 4 active on 

I may add something like :

ethtool --set-port eth0 type sfp active on
ethtool --set-port eth0 type tp active on
 
Maxime