[PATCH 1/2] ethtool: transceiver reset and presence pin control

[PATCH 1/2] ethtool: transceiver reset and presence pin control

[Marek Pazdan]

Signal Definition section (Other signals) of SFF-8636 Spec mentions that
additional signals like reset and module present may be implemented for
a specific hardware. There is currently no user space API for control of
those signals so user space management applications have no chance to
perform some diagnostic or configuration operations. This patch uses
get_phy_tunable/set_phy_tunable ioctl API to provide above control to
user space. Despite ethtool reset option seems to be more suitable for
transceiver module reset control, ethtool reset doesn't allow for reset
pin assertion and deassertion. Userspace API may require control over
when pin will be asserte and deasserted so we cannot trigger reset and
leave it to the driver when deassert should be perfromed.

Signed-off-by: Marek Pazdan <mpazdan@arista.com>
---
 include/uapi/linux/ethtool.h | 14 ++++++++++++++
 net/ethtool/common.c         |  1 +
 net/ethtool/ioctl.c          |  1 +
 3 files changed, 16 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 84833cca29fe..36c363b0ddd4 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -289,11 +289,24 @@ struct ethtool_tunable {
 #define ETHTOOL_PHY_EDPD_NO_TX			0xfffe
 #define ETHTOOL_PHY_EDPD_DISABLE		0
 
+/**
+ * SFF-8636 Spec in Signal Definition section (Other signals) mentions
+ * that 'Additional signals such as power, module present, interrupt, reset,
+ * and low-power mode may be implemented'. Below defines reflect present
+ * and reset signal statuses. Additionally ETHTOOL_PHY_MODULE_RESET
+ * in 'enum phy_tunable_id' will be used for reset pin toggle.
+ */
+#define ETHTOOL_PHY_MODULE_RESET_OFF            0x00
+#define ETHTOOL_PHY_MODULE_RESET_ON             0x01
+/* Not Available if module not present */
+#define ETHTOOL_PHY_MODULE_RESET_NA             0xff
+
 enum phy_tunable_id {
 	ETHTOOL_PHY_ID_UNSPEC,
 	ETHTOOL_PHY_DOWNSHIFT,
 	ETHTOOL_PHY_FAST_LINK_DOWN,
 	ETHTOOL_PHY_EDPD,
+	ETHTOOL_PHY_MODULE_RESET,
 	/*
 	 * Add your fresh new phy tunable attribute above and remember to update
 	 * phy_tunable_strings[] in net/ethtool/common.c
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 0cb6da1f692a..3d35d3e77348 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -101,6 +101,7 @@ phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_PHY_DOWNSHIFT]	= "phy-downshift",
 	[ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down",
 	[ETHTOOL_PHY_EDPD]	= "phy-energy-detect-power-down",
+	[ETHTOOL_PHY_MODULE_RESET]   = "phy-transceiver-module-reset",
 };
 
 #define __LINK_MODE_NAME(speed, type, duplex) \
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 221639407c72..f1f4270cdb69 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2954,6 +2954,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
 	switch (tuna->id) {
 	case ETHTOOL_PHY_DOWNSHIFT:
 	case ETHTOOL_PHY_FAST_LINK_DOWN:
+	case ETHTOOL_PHY_MODULE_RESET:
 		if (tuna->len != sizeof(u8) ||
 		    tuna->type_id != ETHTOOL_TUNABLE_U8)
 			return -EINVAL;
-- 
2.47.0

[PATCH 2/2] ice: add qsfp transceiver reset and presence pin control

[Marek Pazdan]

Commit f3c1c896f5a8 ("ethtool: transceiver reset and presence pin control")
adds ioctl API extension for get/set-phy-tunable so that transceiver
reset and presence pin control is enabled.
This commit adds functionality to utilize the API in ice driver.
According to E810 datasheet QSFP reset and presence pins are being
connected to SDP0 and SDP2 pins on controller host. Those pins can
be accessed using AQ commands for GPIO get/set.[O

Signed-off-by: Marek Pazdan <mpazdan@arista.com>
---
 drivers/net/ethernet/intel/ice/ice.h         |  2 +
 drivers/net/ethernet/intel/ice/ice_common.c  | 21 ++++++
 drivers/net/ethernet/intel/ice/ice_common.h  |  1 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 72 ++++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_main.c    |  1 +
 drivers/net/ethernet/intel/ice/ice_type.h    |  2 +
 6 files changed, 99 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index fd083647c14a..2dbbcb20decf 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -106,6 +106,8 @@
 #define ICE_Q_WAIT_MAX_RETRY	(5 * ICE_Q_WAIT_RETRY_LIMIT)
 #define ICE_MAX_LG_RSS_QS	256
 #define ICE_INVAL_Q_INDEX	0xffff
+#define ICE_GPIO_QSFP0_RESET	0
+#define ICE_GPIO_QSFP0_PRESENT	2
 
 #define ICE_MAX_RXQS_PER_TC		256	/* Used when setting VSI context per TC Rx queues */
 
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 59df31c2c83f..2d643a7cc90f 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -6096,3 +6096,24 @@ u32 ice_get_link_speed(u16 index)
 
 	return ice_aq_to_link_speed[index];
 }
+
+/**
+ * ice_set_has_gpios - Sets availability of SDP GPIO pins.
+ * @hw: pointer to the HW structure
+ *
+ * This function sets availability of GPIO software defined pins
+ * (SDP) which are connected to transceiver slots and are used
+ * for transceiver control.
+ */
+bool ice_set_has_gpios(struct ice_hw *hw)
+{
+	if (hw->vendor_id != PCI_VENDOR_ID_INTEL)
+		return false;
+
+	switch (hw->device_id) {
+	case ICE_DEV_ID_E810C_QSFP:
+		return true;
+	default:
+		return false;
+	}
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 9b00aa0ddf10..b64629b1d60d 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -304,4 +304,5 @@ ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
 int ice_get_pca9575_handle(struct ice_hw *hw, u16 *pca9575_handle);
 int ice_read_pca9575_reg(struct ice_hw *hw, u8 offset, u8 *data);
 bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw);
+bool ice_set_has_gpios(struct ice_hw *hw);
 #endif /* _ICE_COMMON_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 7c2dc347e4e5..20727c582ad5 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -4765,6 +4765,76 @@ static int ice_repr_ethtool_reset(struct net_device *dev, u32 *flags)
 	return ice_reset_vf(vf, ICE_VF_RESET_VFLR | ICE_VF_RESET_LOCK);
 }
 
+static int ice_get_phy_tunable(struct net_device *netdev,
+			       const struct ethtool_tunable *tuna, void *data)
+{
+	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
+	struct ice_hw *hw = &pf->hw;
+	u16 gpio_handle = 0; /* SOC/on-chip GPIO */
+	u8 *enabled = data;
+	bool value;
+
+	switch (tuna->id) {
+	case ETHTOOL_PHY_MODULE_RESET:
+		if (!hw->has_gpios)
+			return -EOPNOTSUPP;
+
+		if (ice_aq_get_gpio(hw, gpio_handle, ICE_GPIO_QSFP0_PRESENT,
+				    &value, NULL))
+			return -EIO;
+		if (!value) {
+			*enabled = ETHTOOL_PHY_MODULE_RESET_NA;
+		} else {
+			if (ice_aq_get_gpio(hw, gpio_handle, ICE_GPIO_QSFP0_RESET,
+					    &value, NULL))
+				return -EIO;
+			*enabled = value ? ETHTOOL_PHY_MODULE_RESET_ON :
+						ETHTOOL_PHY_FAST_LINK_DOWN_OFF;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ice_set_phy_tunable(struct net_device *netdev,
+			       const struct ethtool_tunable *tuna, const void *data)
+{
+	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_vsi *vsi = np->vsi;
+	struct ice_pf *pf = vsi->back;
+	struct ice_hw *hw = &pf->hw;
+	u16 gpio_handle = 0; /* SOC/on-chip GPIO */
+	const u8 *enable = data;
+	bool value;
+
+	switch (tuna->id) {
+	case ETHTOOL_PHY_MODULE_RESET:
+		if (!hw->has_gpios)
+			return -EOPNOTSUPP;
+
+		if (*enable == ETHTOOL_PHY_MODULE_RESET_ON)
+			value = true;
+		else if (*enable == ETHTOOL_PHY_FAST_LINK_DOWN_OFF)
+			value = false;
+		else
+			return -EINVAL;
+
+		if (ice_aq_set_gpio(hw, gpio_handle, ICE_GPIO_QSFP0_RESET,
+				    value, NULL))
+			return -EIO;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct ethtool_ops ice_ethtool_ops = {
 	.cap_rss_ctx_supported  = true,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
@@ -4815,6 +4885,8 @@ static const struct ethtool_ops ice_ethtool_ops = {
 	.set_fecparam		= ice_set_fecparam,
 	.get_module_info	= ice_get_module_info,
 	.get_module_eeprom	= ice_get_module_eeprom,
+	.get_phy_tunable	= ice_get_phy_tunable,
+	.set_phy_tunable	= ice_set_phy_tunable,
 };
 
 static const struct ethtool_ops ice_ethtool_safe_mode_ops = {
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 049edeb60104..fa18fc965649 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5294,6 +5294,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	hw->port_info = NULL;
 	hw->vendor_id = pdev->vendor;
 	hw->device_id = pdev->device;
+	hw->has_gpios = ice_set_has_gpios(hw);
 	pci_read_config_byte(pdev, PCI_REVISION_ID, &hw->revision_id);
 	hw->subsystem_vendor_id = pdev->subsystem_vendor;
 	hw->subsystem_device_id = pdev->subsystem_device;
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 0aab21113cc4..ff758b4b7070 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -896,6 +896,8 @@ struct ice_hw {
 	u64 debug_mask;		/* bitmap for debug mask */
 	enum ice_mac_type mac_type;
 
+	bool has_gpios;
+
 	u16 fd_ctr_base;	/* FD counter base index */
 
 	/* pci info */
-- 
2.47.0

Re: [PATCH 1/2] ethtool: transceiver reset and presence pin control

[Kory Maincent]

On Mon,  7 Apr 2025 12:35:37 +0000
Marek Pazdan <mpazdan@arista.com> wrote:

> Signal Definition section (Other signals) of SFF-8636 Spec mentions that
> additional signals like reset and module present may be implemented for
> a specific hardware. There is currently no user space API for control of
> those signals so user space management applications have no chance to
> perform some diagnostic or configuration operations. This patch uses
> get_phy_tunable/set_phy_tunable ioctl API to provide above control to
> user space. Despite ethtool reset option seems to be more suitable for
> transceiver module reset control, ethtool reset doesn't allow for reset
> pin assertion and deassertion. Userspace API may require control over
> when pin will be asserte and deasserted so we cannot trigger reset and
> leave it to the driver when deassert should be perfromed.

nit: performed

ETHTOOL_PHY_G/STUNABLE IOCTLs are targeting the PHY of the NIC but IIUC in your
case you are targeting the reset of the QSFP module. Maybe phylink API is more
appropriate for this feature.

You have to add net-next prefix in the subject like this [PATCH net-next 1/2]
when you add new support to net subsystem.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 2/2] ice: add qsfp transceiver reset and presence pin control

[Kory Maincent]

On Mon,  7 Apr 2025 12:35:38 +0000
Marek Pazdan <mpazdan@arista.com> wrote:

> Commit f3c1c896f5a8 ("ethtool: transceiver reset and presence pin control")
> adds ioctl API extension for get/set-phy-tunable so that transceiver
> reset and presence pin control is enabled.

I don't think pointing and explaining the first commit is relevant here.

> This commit adds functionality to utilize the API in ice driver.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

You could simply write:
Add support for the newly introduced transceiver reset feature in the ice
driver.

> According to E810 datasheet QSFP reset and presence pins are being
> connected to SDP0 and SDP2 pins on controller host. Those pins can
> be accessed using AQ commands for GPIO get/set.[O

Weird character at the end.

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 2/2] ice: add qsfp transceiver reset and presence pin control

[Andrew Lunn]

On Mon, Apr 07, 2025 at 12:35:38PM +0000, Marek Pazdan wrote:
> Commit f3c1c896f5a8 ("ethtool: transceiver reset and presence pin control")
> adds ioctl API extension for get/set-phy-tunable so that transceiver
> reset and presence pin control is enabled.

As the name get/set-phy-tunable suggests, these are for PHY
properties, like downshift, fast link down, energy detected power
down.

What PHY are you using here?

     Andrew

Re: [PATCH 1/2] ethtool: transceiver reset and presence pin control

[Andrew Lunn]

On Mon, Apr 07, 2025 at 12:35:37PM +0000, Marek Pazdan wrote:
> Signal Definition section (Other signals) of SFF-8636 Spec mentions that
> additional signals like reset and module present may be implemented for
> a specific hardware. There is currently no user space API for control of
> those signals so user space management applications have no chance to
> perform some diagnostic or configuration operations.

How do you tell the kernel to stop managing the SFP? If you hit the
module with a reset from user space, the kernel is going to get
confused. And how are you talking to the module? Are you going to
hijack the i2c device via i2-dev? Again, you need to stop the kernel
from using the device.

Before you go any further, i think you need to zoom out and tell us
the big picture....

    Andrew

Re: [Intel-wired-lan] [PATCH 2/2] ice: add qsfp transceiver reset and presence pin control

[Marek Pazdan]

On Mon, 7 Apr 2025 22:30:54 +0200 Andrew Lunn wrote:

> As the name get/set-phy-tunable suggests, these are for PHY
> properties, like downshift, fast link down, energy detected power
> down.
> 
> What PHY are you using here?

Thanks for review.
It's PHY E810-C in this case. According to spreadsheet: E810_Datasheet_Rev2.4.pdf
(Chapter 17.3.3 E810 SDP[0:7] (GPIO) Connection), it's SDP0 and SDP2 GPIOs 
are being connected to QSFP Reset and Presence pins correspondingly.
I assume you may suggest this use case is not directly PHY related. In first approach
I tried to use reset operation which has following flag in enum ethtool_reset_flags:
ETH_RESET_PHY           = 1 << 6,       /* Transceiver/PHY */
but this doesn't allow for reset asserting and later deasserting so I took 'get/set-phy-tunable'.

Re: [Intel-wired-lan] [PATCH 2/2] ice: add qsfp transceiver reset and presence pin control

[Andrew Lunn]

On Tue, Apr 08, 2025 at 02:22:43PM +0000, Marek Pazdan wrote:
> On Mon, 7 Apr 2025 22:30:54 +0200 Andrew Lunn wrote:
> 
> > As the name get/set-phy-tunable suggests, these are for PHY
> > properties, like downshift, fast link down, energy detected power
> > down.
> > 
> > What PHY are you using here?
> 
> Thanks for review.
> It's PHY E810-C in this case. According to spreadsheet: E810_Datasheet_Rev2.4.pdf
> (Chapter 17.3.3 E810 SDP[0:7] (GPIO) Connection), it's SDP0 and SDP2 GPIOs 
> are being connected to QSFP Reset and Presence pins correspondingly.
> I assume you may suggest this use case is not directly PHY related. In first approach
> I tried to use reset operation which has following flag in enum ethtool_reset_flags:
> ETH_RESET_PHY           = 1 << 6,       /* Transceiver/PHY */
> but this doesn't allow for reset asserting and later deasserting so I took 'get/set-phy-tunable'.

This does look like an abuse of the phy tunables. Lets get the big
picture, and then we can maybe suggest a better API.

Please consider my previous questions, how do you tell the kernel not
to use the SFP?  How do you ensure you don't reset the SFP in the
middle of the kernel performing a firmware upgrade of the SFP, etc.

	Andrew

Re: [Intel-wired-lan] [PATCH 1/2] ethtool: transceiver reset and presence pin control

[Marek Pazdan]

On Mon, 7 Apr 2025 22:39:17 +0200 Andrew Lunn wrote:
> How do you tell the kernel to stop managing the SFP? If you hit the
> module with a reset from user space, the kernel is going to get
> confused. And how are you talking to the module? Are you going to
> hijack the i2c device via i2-dev? Again, you need to stop the kernel
> from using the device.

This is something to implement in driver code. For ice driver this reset will
be executed through AQ command (Admin Queue) which is communication channel
between driver and firmware. What I probably need to do is to add additional PHY
state (like USER_MODULE_RESET) and check it when driver wants to execute AQ command.

> Before you go any further, i think you need to zoom out and tell us
> the big picture....

In my use case I need to have ability to reset transceiver module. There are 
several reasons for that. Most common is to reinit module if case of error state.
(this according to CMIS spec). Another use case is that in our switch's cli there
is a command for transceiver reinitialisation which involves transceiver reset.

Re: [Intel-wired-lan] [PATCH 1/2] ethtool: transceiver reset and presence pin control

[Marek Pazdan]

On  Mon, 7 Apr 2025 15:32:03 +0200 Kory Maincent wrote:
> ETHTOOL_PHY_G/STUNABLE IOCTLs are targeting the PHY of the NIC but IIUC in your
> case you are targeting the reset of the QSFP module. Maybe phylink API is more
> appropriate for this feature.
> 
> You have to add net-next prefix in the subject like this [PATCH net-next 1/2]
> when you add new support to net subsystem.

Thanks for review.
From up to now replies I see that there are concerns regarding usage phy-tunable ethtool
option for this purpose, so I will post updated patches after we clarify proper way to go. 
I need to check more on phylink API, from the overview I read:
"phylink is a mechanism to support hot-pluggable networking modules directly connected
to a MAC without needing to re-initialise the adapter on hot-plug events.

phylink supports conventional phylib-based setups, fixed link setups
and SFP (Small Formfactor Pluggable) modules at present."

I don't see QSFP modules are being supported but I need to verify impact of this.
As I mentioned in other reply this API should allow for transceiver module reset 
from user space if orchestration agent detects transceiver failure state or when
it gets direct request from Cli.

Re: [Intel-wired-lan] [PATCH 1/2] ethtool: transceiver reset and presence pin control

[Andrew Lunn]

On Tue, Apr 08, 2025 at 03:32:30PM +0000, Marek Pazdan wrote:
> On Mon, 7 Apr 2025 22:39:17 +0200 Andrew Lunn wrote:
> > How do you tell the kernel to stop managing the SFP? If you hit the
> > module with a reset from user space, the kernel is going to get
> > confused. And how are you talking to the module? Are you going to
> > hijack the i2c device via i2-dev? Again, you need to stop the kernel
> > from using the device.
> 
> This is something to implement in driver code. For ice driver this reset will
> be executed through AQ command (Admin Queue) which is communication channel
> between driver and firmware. What I probably need to do is to add additional PHY
> state (like USER_MODULE_RESET) and check it when driver wants to execute AQ command.
> 
> > Before you go any further, i think you need to zoom out and tell us
> > the big picture....
> 
> In my use case I need to have ability to reset transceiver module. There are 
> several reasons for that. Most common is to reinit module if case of error state.
> (this according to CMIS spec). Another use case is that in our switch's cli there
> is a command for transceiver reinitialisation which involves transceiver reset.

Now zoom out, ignore your hardware, look at the Linux abstraction for
an SFP, across all NICs and switches.

There are ethtool calls to retrieve the modules eeprom contents. There
is an ethtool call to program the modules firmware. There is an
ethtool call to get/set the power mode. Modules can also export their
sensors via HWMON, the temperature, receive power, etc.

How does your wish to reset the module fit into the general Linux
model of an SFP? Should it be allowed in the middle of a firmware
upgrade? Should the power mode be lost due to a reset? Can you still
read from the EEPROM etc while it is in reset? What should HWMON
report? Should it be a foot gun?

It does however seem to me, what you want should somehow be an ethtool
operation. And it probably needs to be plumbed through
net/ethtool/module.c, and you need to look at how it interacts with
all the other code in that file. And you maybe need to force the
netdev to do a down/up so that it gets the new state of the module, or
you only allow it on an netdev which is admin down?

Your patch needs to explain the big picture, how it fits into the
Linux abstraction of an SFP, and how other vendors should implement
the same operation, if they wish to implement it.

    Andrew

[PATCH net-next v2 1/2] ethtool: qsfp transceiver reset, interrupt and presence pin control

[Marek Pazdan]

Common Management Interface Specification defines
Management Signaling Layer (MSL) control and status signals. This change
provides API for following signals status reading:
- signal allowing the host to request module reset (Reset)
- signal allowing the host to detect module presence (Presence)
- signal allowing the host to detect module interrupt (Int)
Additionally API allows for Reset signal assertion with
following constraints:
- reset cannot be asserted if firmware update is in progress
- if reset is asserted, firmware update cannot be started
- if reset is asserted, power mode cannot be get/set
In all above constraint cases -EBUSY error is returned.

After reset, module will set all registers to default
values. Default value for Page0 byte 93 register is 0x00 what implies that
module power mode after reset depends on LPMode HW pin state.
If software power mode control is required, bit 0 of Page0 byte93 needs
to be enabled.
Module reset assertion implies failure of every module's related
SMBus transactions. Device driver developers should take this into
consideration if driver provides API for querying module's related data.
One example can be HWMON providing module temperature report.
In such case driver should monitor module status and in time of reset
assertion it should return HWMON report which informs that temperature
data is not available due to module's reset state.
The same applies to power mode set/get. Ethtool API has already
checking for module reset state but similar checking needs to be
implemented in the driver if it requests power mode for other
functionality.
Additionally module reset is link hitful operation. Link is brought down
when reset is asserted. If device driver doesn't provide functionality
for monitoring transceiver state, it needs to be implemented in parallel
to get/set_module_mgmt_signal API. When module reset gets deasserted,
transceiver process reinitialization. The end of reinitialization
process is signalled via Page 00h Byte 6 bit 0 "Initialization complete
flags". If there is no implementation for monitoring this bit in place,
it needs to be added to bring up the link after transceiver
initialization is complete.

Signed-off-by: Marek Pazdan <mpazdan@arista.com>
---
 Documentation/netlink/specs/ethtool.yaml |  48 +++++
 include/linux/ethtool.h                  |  29 ++-
 include/uapi/linux/ethtool.h             |  29 +++
 net/ethtool/module.c                     | 230 +++++++++++++++++++++--
 net/ethtool/netlink.c                    |  20 ++
 net/ethtool/netlink.h                    |   3 +
 6 files changed, 347 insertions(+), 12 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index c650cd3dcb80..38eebbe18f55 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1528,6 +1528,24 @@ attribute-sets:
         name: hwtstamp-flags
         type: nest
         nested-attributes: bitset
+  -
+    name: module-mgmt
+    attr-cnt-name: __ethtool-a-module-mgmt-cnt
+    attributes:
+      -
+        name: unspec
+        type: unused
+        value: 0
+      -
+        name: header
+        type: nest
+        nested-attributes: header
+      -
+        name: type
+        type: u8
+      -
+        name: value
+        type: u8
 
 operations:
   enum-model: directional
@@ -2384,3 +2402,33 @@ operations:
           attributes: *tsconfig
         reply:
           attributes: *tsconfig
+    -
+      name: module-mgmt-get
+      doc: Get module management signal status.
+
+      attribute-set: module-mgmt
+
+      do: &module-mgmt-get-op
+        request:
+          attributes:
+            - header
+            - type
+        reply:
+          attributes: &module-mgmt
+            - header
+            - type
+            - value
+      dump: *module-mgmt-get-op
+    -
+      name: module-mgmt-set
+      doc: Set module management signal state.
+
+      attribute-set: module-mgmt
+
+      do:
+        request:
+          attributes: *module-mgmt
+    -
+      name: module-mgmt-ntf
+      doc: Notification for change in module management signal status.
+      notify: module-mgmt-get
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 8210ece94fa6..d5dd238b4b61 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -126,6 +126,7 @@ enum ethtool_supported_ring_param {
 
 struct net_device;
 struct netlink_ext_ack;
+struct ethtool_module_mgmt_params;
 
 /* Link extended state and substate. */
 struct ethtool_link_ext_state_info {
@@ -626,6 +627,19 @@ struct ethtool_module_power_mode_params {
 	enum ethtool_module_power_mode mode;
 };
 
+/**
+ * struct ethtool_module_mgmt_params - module management signal parameters
+ * @type: The management signal type of the MSL (Management Signaling Layer) signal
+ *		connecting host with the plug-in module to be set or get.
+ * @value: The management signal value of the MSL signal connecting host with
+ *		the plug-in module. It is filled by user for set operation or by
+ *		the driver for get operation.
+ */
+struct ethtool_module_mgmt_params {
+	enum ethtool_module_mgmt_signal_type type;
+	enum ethtool_module_mgmt_signal_value value;
+};
+
 /**
  * struct ethtool_mm_state - 802.3 MAC merge layer state
  * @verify_time:
@@ -985,6 +999,11 @@ struct kernel_ethtool_ts_info {
  *	plugged-in.
  * @set_module_power_mode: Set the power mode policy for the plug-in module
  *	used by the network device.
+ * @get_module_mgmt_signal: Get the MSL (Management Signaling Layer) signal
+ *	value for the plug-in module used by network device. MSL layer
+ *	description is included in CMIS Common Management Interface Specification.
+ * @set_module_mgmt_signal: Set the MSL (Management Signaling Layer) output
+ *	signal value for the plug-in module used by network device, if plugged-in.
  * @get_mm: Query the 802.3 MAC Merge layer state.
  * @set_mm: Set the 802.3 MAC Merge layer parameters.
  * @get_mm_stats: Query the 802.3 MAC Merge layer statistics.
@@ -1146,6 +1165,12 @@ struct ethtool_ops {
 	int	(*set_module_power_mode)(struct net_device *dev,
 					 const struct ethtool_module_power_mode_params *params,
 					 struct netlink_ext_ack *extack);
+	int	(*get_module_mgmt_signal)(struct net_device *dev,
+					  struct ethtool_module_mgmt_params *params,
+					  struct netlink_ext_ack *extack);
+	int	(*set_module_mgmt_signal)(struct net_device *dev,
+					  const struct ethtool_module_mgmt_params *params,
+					  struct netlink_ext_ack *extack);
 	int	(*get_mm)(struct net_device *dev, struct ethtool_mm_state *state);
 	int	(*set_mm)(struct net_device *dev, struct ethtool_mm_cfg *cfg,
 			  struct netlink_ext_ack *extack);
@@ -1179,13 +1204,15 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
  * @rss_lock:		Protects entries in @rss_ctx.  May be taken from
  *			within RTNL.
  * @wol_enabled:	Wake-on-LAN is enabled
- * @module_fw_flash_in_progress: Module firmware flashing is in progress.
+ * @module_fw_flash_in_progress:	Module firmware flashing is in progress.
+ * @module_reset_asserted:		Module reset signal is asserted.
  */
 struct ethtool_netdev_state {
 	struct xarray		rss_ctx;
 	struct mutex		rss_lock;
 	unsigned		wol_enabled:1;
 	unsigned		module_fw_flash_in_progress:1;
+	unsigned		module_reset_asserted:1;
 };
 
 struct phy_device;
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 84833cca29fe..097b81334798 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -756,6 +756,35 @@ enum ethtool_module_power_mode {
 	ETHTOOL_MODULE_POWER_MODE_HIGH,
 };
 
+/**
+ * enum ethtool_module_mgmt_signal_type - plug-in module discrete
+ *	status hardware signals for management as per CMIS spec.
+ * @ETHTOOL_MODULE_MGMT_RESET: Signal allowing the host to request
+ *	a module reset.
+ * @ETHTOOL_MODULE_MGMT_INT: Signal allowing the module to assert
+ *	an interrupt request to the host.
+ * @ETHTOOL_MODULE_MGMT_PRESENT: Signal allowing the module to signal
+ *	its presence status to the host.
+ */
+enum ethtool_module_mgmt_signal_type {
+	ETHTOOL_MODULE_MGMT_RESET = 1,
+	ETHTOOL_MODULE_MGMT_INT,
+	ETHTOOL_MODULE_MGMT_PRESENT,
+};
+
+/**
+ * enum ethtool_module_mgmt_signal_value - Value of plug-in module
+ *	hardware signal status for management signaling
+ *	as specified in CMIS spec.
+ * @ETHTOOL_MODULE_MGMT_SIGNAL_LOW: Signal low value.
+ * @ETHTOOL_MODULE_MGMT_SIGNAL_HIGH: Signal high value.
+ *	for reset signal when plug-in module is not inserted.
+ */
+enum ethtool_module_mgmt_signal_value {
+	ETHTOOL_MODULE_MGMT_SIGNAL_LOW = 1,
+	ETHTOOL_MODULE_MGMT_SIGNAL_HIGH,
+};
+
 /**
  * enum ethtool_c33_pse_ext_state - groups of PSE extended states
  *      functions. IEEE 802.3-2022 33.2.4.4 Variables
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 4d4e0a82579a..d0ae2c1e0966 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -23,6 +23,24 @@ struct module_reply_data {
 #define MODULE_REPDATA(__reply_base) \
 	container_of(__reply_base, struct module_reply_data, base)
 
+static bool module_busy(const struct ethtool_netdev_state *state,
+			struct netlink_ext_ack *extack)
+{
+	if (state->module_fw_flash_in_progress) {
+		if (extack)
+			NL_SET_ERR_MSG(extack,
+				       "Module firmware flashing is in progress");
+		return true;
+	}
+	if (state->module_reset_asserted) {
+		if (extack)
+			NL_SET_ERR_MSG(extack,
+				       "Module reset is in progress");
+		return true;
+	}
+	return false;
+}
+
 /* MODULE_GET */
 
 const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1] = {
@@ -38,11 +56,8 @@ static int module_get_power_mode(struct net_device *dev,
 	if (!ops->get_module_power_mode)
 		return 0;
 
-	if (dev->ethtool->module_fw_flash_in_progress) {
-		NL_SET_ERR_MSG(extack,
-			       "Module firmware flashing is in progress");
+	if (module_busy(dev->ethtool, extack))
 		return -EBUSY;
-	}
 
 	return ops->get_module_power_mode(dev, &data->power, extack);
 }
@@ -120,11 +135,8 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info,
 	if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY])
 		return 0;
 
-	if (req_info->dev->ethtool->module_fw_flash_in_progress) {
-		NL_SET_ERR_MSG(info->extack,
-			       "Module firmware flashing is in progress");
+	if (module_busy(req_info->dev->ethtool, info->extack))
 		return -EBUSY;
-	}
 
 	if (!ops->get_module_power_mode || !ops->set_module_power_mode) {
 		NL_SET_ERR_MSG_ATTR(info->extack,
@@ -176,6 +188,204 @@ const struct ethnl_request_ops ethnl_module_request_ops = {
 	.set_ntf_cmd		= ETHTOOL_MSG_MODULE_NTF,
 };
 
+/* MODULE_MGMT_GET */
+struct module_mgmt_req_data {
+	struct ethnl_req_info base;
+	struct ethtool_module_mgmt_params mgmt;
+};
+
+#define MODULE_MGMT_REQINFO(__req_base) \
+	container_of(__req_base, struct module_mgmt_req_data, base)
+
+struct module_mgmt_reply_data {
+	struct ethnl_reply_data	base;
+	struct ethtool_module_mgmt_params mgmt;
+};
+
+#define MODULE_MGMT_REPDATA(__reply_base) \
+	container_of(__reply_base, struct module_mgmt_reply_data, base)
+
+const struct nla_policy ethnl_module_mgmt_get_policy[ETHTOOL_A_MODULE_MGMT_TYPE + 1] = {
+	[ETHTOOL_A_MODULE_MGMT_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_MODULE_MGMT_TYPE] =
+		NLA_POLICY_RANGE(NLA_U8, ETHTOOL_MODULE_MGMT_RESET,
+				 ETHTOOL_MODULE_MGMT_PRESENT),
+};
+
+static int module_mgmt_get(struct net_device *dev,
+			   struct module_mgmt_reply_data *data,
+			   const struct genl_info *info)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct netlink_ext_ack *extack = info ? info->extack : NULL;
+
+	if (!ops->get_module_mgmt_signal)
+		return -EOPNOTSUPP;
+
+	return ops->get_module_mgmt_signal(dev, &data->mgmt, extack);
+}
+
+static int module_mgmt_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
+				     struct netlink_ext_ack *extack)
+{
+	struct module_mgmt_req_data *req_data = MODULE_MGMT_REQINFO(req_info);
+
+	if (!tb[ETHTOOL_A_MODULE_MGMT_TYPE])
+		return -EINVAL;
+	req_data->mgmt.type = nla_get_u8(tb[ETHTOOL_A_MODULE_MGMT_TYPE]);
+
+	return 0;
+}
+
+static int module_mgmt_prepare_data(const struct ethnl_req_info *req_base,
+				    struct ethnl_reply_data *reply_base,
+				    const struct genl_info *info)
+{
+	struct module_mgmt_reply_data *data = MODULE_MGMT_REPDATA(reply_base);
+	struct module_mgmt_req_data *req = MODULE_MGMT_REQINFO(req_base);
+	struct net_device *dev = reply_base->dev;
+	int ret;
+
+	if (!info || !info->attrs[ETHTOOL_A_MODULE_MGMT_TYPE])
+		return -EINVAL;
+	req->mgmt.type = nla_get_u8(info->attrs[ETHTOOL_A_MODULE_MGMT_TYPE]);
+	data->mgmt.type = req->mgmt.type;
+
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		return ret;
+
+	ret = module_mgmt_get(dev, data, info);
+	if (ret < 0)
+		goto out_complete;
+
+out_complete:
+	ethnl_ops_complete(dev);
+	return ret;
+}
+
+static int module_mgmt_reply_size(const struct ethnl_req_info *req_base,
+				  const struct ethnl_reply_data *reply_base)
+{
+	struct module_mgmt_reply_data *data = MODULE_MGMT_REPDATA(reply_base);
+	int len = 0;
+
+	if (data->mgmt.type)
+		len += nla_total_size(sizeof(u8));	/* _MODULE_MGMT_TYPE */
+
+	if (data->mgmt.value)
+		len += nla_total_size(sizeof(u8));	/* _MODULE_MGMT_VALUE */
+
+	return len;
+}
+
+static int module_mgmt_fill_reply(struct sk_buff *skb,
+				  const struct ethnl_req_info *req_base,
+				  const struct ethnl_reply_data *reply_base)
+{
+	struct module_mgmt_reply_data *data = MODULE_MGMT_REPDATA(reply_base);
+
+	if (data->mgmt.type &&
+	    nla_put_u8(skb, ETHTOOL_A_MODULE_MGMT_TYPE,
+		       data->mgmt.type))
+		return -EMSGSIZE;
+
+	if (data->mgmt.value &&
+	    nla_put_u8(skb, ETHTOOL_A_MODULE_MGMT_VALUE, data->mgmt.value))
+		return -EMSGSIZE;
+
+	if (data->mgmt.type == ETHTOOL_MODULE_MGMT_RESET) {
+		req_base->dev->ethtool->module_reset_asserted =
+			(data->mgmt.value == ETHTOOL_MODULE_MGMT_SIGNAL_HIGH) ? 1 : 0;
+	}
+
+	return 0;
+}
+
+/* MODULE_MGMT_SET */
+const struct nla_policy ethnl_module_mgmt_set_policy[ETHTOOL_A_MODULE_MGMT_VALUE + 1] = {
+	[ETHTOOL_A_MODULE_MGMT_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
+	[ETHTOOL_A_MODULE_MGMT_TYPE] =
+		NLA_POLICY_RANGE(NLA_U8, ETHTOOL_MODULE_MGMT_RESET,
+				 ETHTOOL_MODULE_MGMT_PRESENT),
+	[ETHTOOL_A_MODULE_MGMT_VALUE] =
+		NLA_POLICY_RANGE(NLA_U8, ETHTOOL_MODULE_MGMT_SIGNAL_LOW,
+				 ETHTOOL_MODULE_MGMT_SIGNAL_HIGH),
+};
+
+static int
+ethnl_module_mgmt_validate(struct ethnl_req_info *req_info,
+			   struct genl_info *info)
+{
+	const struct ethtool_ops *ops = req_info->dev->ethtool_ops;
+	struct netlink_ext_ack *extack = info ? info->extack : NULL;
+	struct nlattr **tb = info->attrs;
+
+	if (!tb[ETHTOOL_A_MODULE_MGMT_TYPE] || !tb[ETHTOOL_A_MODULE_MGMT_VALUE])
+		return 0;
+
+	if (req_info->dev->ethtool->module_fw_flash_in_progress) {
+		if (extack)
+			NL_SET_ERR_MSG(extack, "Module firmware flashing is in progress");
+		return -EBUSY;
+	}
+
+	if (!ops->get_module_mgmt_signal || !ops->set_module_mgmt_signal) {
+		if (extack)
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[ETHTOOL_A_MODULE_MGMT_TYPE],
+					    "Setting module management signal is not supported by this device");
+		return -EOPNOTSUPP;
+	}
+
+	return 1;
+}
+
+static int
+ethnl_module_mgmt_set(struct ethnl_req_info *req_info, struct genl_info *info)
+{
+	struct netlink_ext_ack *extack = info ? info->extack : NULL;
+	struct ethtool_module_mgmt_params mgmt = {};
+	struct ethtool_module_mgmt_params mgmt_new;
+	const struct ethtool_ops *ops;
+	struct net_device *dev = req_info->dev;
+	struct nlattr **tb = info->attrs;
+	int ret;
+
+	ops = dev->ethtool_ops;
+
+	mgmt_new.type = nla_get_u8(tb[ETHTOOL_A_MODULE_MGMT_TYPE]);
+	mgmt.type = mgmt_new.type;
+	mgmt_new.value = nla_get_u8(tb[ETHTOOL_A_MODULE_MGMT_VALUE]);
+	ret = ops->get_module_mgmt_signal(dev, &mgmt, extack);
+	if (ret < 0)
+		return ret;
+
+	if (mgmt.value == mgmt_new.value)
+		return 0;
+
+	ret = ops->set_module_mgmt_signal(dev, &mgmt_new, extack);
+
+	return ret < 0 ? ret : 1;
+}
+
+const struct ethnl_request_ops ethnl_module_mgmt_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_MODULE_MGMT_GET,
+	.reply_cmd		= ETHTOOL_MSG_MODULE_MGMT_GET_REPLY,
+	.hdr_attr		= ETHTOOL_A_MODULE_MGMT_HEADER,
+	.req_info_size		= sizeof(struct module_mgmt_req_data),
+	.reply_data_size	= sizeof(struct module_mgmt_reply_data),
+
+	.parse_request		= module_mgmt_parse_request,
+	.prepare_data		= module_mgmt_prepare_data,
+	.reply_size		= module_mgmt_reply_size,
+	.fill_reply		= module_mgmt_fill_reply,
+
+	.set_validate		= ethnl_module_mgmt_validate,
+	.set			= ethnl_module_mgmt_set,
+	.set_ntf_cmd		= ETHTOOL_MSG_MODULE_MGMT_NTF,
+};
+
 /* MODULE_FW_FLASH_ACT */
 
 const struct nla_policy
@@ -386,10 +596,8 @@ static int ethnl_module_fw_flash_validate(struct net_device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (dev->ethtool->module_fw_flash_in_progress) {
-		NL_SET_ERR_MSG(extack, "Module firmware flashing already in progress");
+	if (module_busy(dev->ethtool, extack))
 		return -EBUSY;
-	}
 
 	if (dev->flags & IFF_UP) {
 		NL_SET_ERR_MSG(extack, "Netdevice is up, so flashing is not permitted");
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 977beeaaa2f9..10dc56830943 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -400,6 +400,8 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_TSCONFIG_GET]	= &ethnl_tsconfig_request_ops,
 	[ETHTOOL_MSG_TSCONFIG_SET]	= &ethnl_tsconfig_request_ops,
+	[ETHTOOL_MSG_MODULE_MGMT_GET]	= &ethnl_module_mgmt_request_ops,
+	[ETHTOOL_MSG_MODULE_MGMT_SET]	= &ethnl_module_mgmt_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -755,6 +757,7 @@ ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_MODULE_NTF]	= &ethnl_module_request_ops,
 	[ETHTOOL_MSG_PLCA_NTF]		= &ethnl_plca_cfg_request_ops,
 	[ETHTOOL_MSG_MM_NTF]		= &ethnl_mm_request_ops,
+	[ETHTOOL_MSG_MODULE_MGMT_NTF]	= &ethnl_module_mgmt_request_ops,
 };
 
 /* default notification handler */
@@ -856,6 +859,7 @@ static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_MODULE_NTF]	= ethnl_default_notify,
 	[ETHTOOL_MSG_PLCA_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_MM_NTF]		= ethnl_default_notify,
+	[ETHTOOL_MSG_MODULE_MGMT_NTF]	= ethnl_default_notify,
 };
 
 void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
@@ -1294,6 +1298,22 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_tsconfig_set_policy,
 		.maxattr = ARRAY_SIZE(ethnl_tsconfig_set_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_MODULE_MGMT_GET,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_module_mgmt_get_policy,
+		.maxattr = ARRAY_SIZE(ethnl_module_mgmt_get_policy) - 1,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_MODULE_MGMT_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_default_set_doit,
+		.policy = ethnl_module_mgmt_set_policy,
+		.maxattr = ARRAY_SIZE(ethnl_module_mgmt_set_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index ec6ab5443a6f..e200f8193328 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -437,6 +437,7 @@ 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 ethnl_request_ops ethnl_tsconfig_request_ops;
+extern const struct ethnl_request_ops ethnl_module_mgmt_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];
@@ -489,6 +490,8 @@ extern const struct nla_policy ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE
 extern const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1];
 extern const struct nla_policy ethnl_tsconfig_get_policy[ETHTOOL_A_TSCONFIG_HEADER + 1];
 extern const struct nla_policy ethnl_tsconfig_set_policy[ETHTOOL_A_TSCONFIG_MAX + 1];
+extern const struct nla_policy ethnl_module_mgmt_get_policy[ETHTOOL_A_MODULE_MGMT_TYPE + 1];
+extern const struct nla_policy ethnl_module_mgmt_set_policy[ETHTOOL_A_MODULE_MGMT_VALUE + 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);
-- 
2.45.2

[Intel-wired-lan] [PATCH net-next v2 2/2] ice: add qsfp transceiver reset, interrupt and presence pin control

[Marek Pazdan]

Add get/set implenentation for ethtool's module management signal
API.
Examples:
ethtool --get-module-mgmt-signal eth16 type reset
reset: low

ethtool --get-module-mgmt-signal eth16 type int
reset: low

ethtool --get-module-mgmt-signal eth16 type present
reset: high

sudo ethtool --set-module-mgmt-signal eth16 type reset value high
ethtool --get-module-mgmt-signal eth16 type reset
reset: high

sudo ethtool --set-module-mgmt-signal eth16 type reset value low
ethtool --get-module-mgmt-signal eth16 type reset
reset: low

Ice driver gets link event notification when module gets restarted.
There is 'ice_handle_link_event' which handles the notification and
updates link status information.

Signed-off-by: Marek Pazdan <mpazdan@arista.com>
---
 drivers/net/ethernet/intel/ice/ice.h         |  6 ++
 drivers/net/ethernet/intel/ice/ice_common.c  | 21 +++++
 drivers/net/ethernet/intel/ice/ice_common.h  |  1 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 94 ++++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_main.c    |  1 +
 drivers/net/ethernet/intel/ice/ice_type.h    |  2 +-
 6 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index fd083647c14a..3b95a69140e8 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -195,6 +195,12 @@
 
 #define ice_pf_src_tmr_owned(pf) ((pf)->hw.func_caps.ts_func_info.src_tmr_owned)
 
+enum ice_mgmt_pin {
+	ICE_MGMT_PIN_RESET = 0,
+	ICE_MGMT_PIN_INT,
+	ICE_MGMT_PIN_PRESENT
+};
+
 enum ice_feature {
 	ICE_F_DSCP,
 	ICE_F_PHY_RCLK,
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 59df31c2c83f..2d643a7cc90f 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -6096,3 +6096,24 @@ u32 ice_get_link_speed(u16 index)
 
 	return ice_aq_to_link_speed[index];
 }
+
+/**
+ * ice_set_has_gpios - Sets availability of SDP GPIO pins.
+ * @hw: pointer to the HW structure
+ *
+ * This function sets availability of GPIO software defined pins
+ * (SDP) which are connected to transceiver slots and are used
+ * for transceiver control.
+ */
+bool ice_set_has_gpios(struct ice_hw *hw)
+{
+	if (hw->vendor_id != PCI_VENDOR_ID_INTEL)
+		return false;
+
+	switch (hw->device_id) {
+	case ICE_DEV_ID_E810C_QSFP:
+		return true;
+	default:
+		return false;
+	}
+}
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 9b00aa0ddf10..b64629b1d60d 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -304,4 +304,5 @@ ice_aq_write_i2c(struct ice_hw *hw, struct ice_aqc_link_topo_addr topo_addr,
 int ice_get_pca9575_handle(struct ice_hw *hw, u16 *pca9575_handle);
 int ice_read_pca9575_reg(struct ice_hw *hw, u8 offset, u8 *data);
 bool ice_fw_supports_report_dflt_cfg(struct ice_hw *hw);
+bool ice_set_has_gpios(struct ice_hw *hw);
 #endif /* _ICE_COMMON_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 7c2dc347e4e5..bf6a803729d4 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3848,6 +3848,96 @@ ice_get_channels(struct net_device *dev, struct ethtool_channels *ch)
 	ch->max_other = ch->other_count;
 }
 
+/**
+ * ice_get_module_mgmt_signal - get module management signal status
+ * @dev: network interface device structure
+ * @params: ethtool module management signal params
+ * @extack: extended ACK from the Netlink message
+ *
+ * Returns -EIO if AQ command for GPIO get failed, otherwise
+ * returns 0 and current status of requested signal in params.
+ */
+static int
+ice_get_module_mgmt_signal(struct net_device *dev,
+			   struct ethtool_module_mgmt_params *params,
+			   struct netlink_ext_ack *extack)
+{
+	struct ice_netdev_priv *np = netdev_priv(dev);
+	struct ice_pf *pf = np->vsi->back;
+	struct ice_hw *hw = &pf->hw;
+	u16 gpio_handle = 0; /* SOC/on-chip GPIO */
+	bool value;
+	int ret = 0;
+
+	if (hw->has_module_mgmt_gpio) {
+		switch (params->type) {
+		case ETHTOOL_MODULE_MGMT_RESET:
+			ret = ice_aq_get_gpio(hw, gpio_handle,
+					      ICE_MGMT_PIN_RESET, &value, NULL);
+			break;
+		case ETHTOOL_MODULE_MGMT_INT:
+			ret = ice_aq_get_gpio(hw, gpio_handle,
+					      ICE_MGMT_PIN_INT, &value, NULL);
+			break;
+		case ETHTOOL_MODULE_MGMT_PRESENT:
+			ret = ice_aq_get_gpio(hw, gpio_handle,
+					      ICE_MGMT_PIN_PRESENT, &value, NULL);
+			break;
+		default:
+			dev_dbg(ice_pf_to_dev(pf), "Incorrect management signal requested: %d\n",
+				params->type);
+			return -EINVAL;
+		}
+	} else {
+		return -EOPNOTSUPP;
+	}
+
+	if (ret == 0) {
+		params->value = value ? ETHTOOL_MODULE_MGMT_SIGNAL_HIGH :
+			ETHTOOL_MODULE_MGMT_SIGNAL_LOW;
+	}
+	return ret;
+}
+
+/**
+ * ice_set_module_mgmt_signal - set module management signal config
+ * @dev: network interface device structure
+ * @params: ethtool module management signal params
+ * @extack: extended ACK from the Netlink message
+ *
+ * Returns -EIO if AQ command for GPIO set failed, otherwise
+ * returns 0.
+ */
+static int
+ice_set_module_mgmt_signal(struct net_device *dev,
+			   const struct ethtool_module_mgmt_params *params,
+			   struct netlink_ext_ack *extack)
+{
+	struct ice_netdev_priv *np = netdev_priv(dev);
+	struct ice_pf *pf = np->vsi->back;
+	struct ice_hw *hw = &pf->hw;
+	u16 gpio_handle = 0; /* SOC/on-chip GPIO */
+	bool value = params->value == ETHTOOL_MODULE_MGMT_SIGNAL_HIGH ? true : false;
+	int ret = 0;
+
+	if (hw->has_module_mgmt_gpio) {
+		switch (params->type) {
+		case ETHTOOL_MODULE_MGMT_RESET:
+			ret = ice_aq_set_gpio(hw, gpio_handle,
+					      ICE_MGMT_PIN_RESET, value, NULL);
+			break;
+		default:
+			dev_dbg(ice_pf_to_dev(pf), "Incorrect management signal requested: %d\n",
+				params->type);
+			return -EINVAL;
+		}
+	} else {
+		return -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
 /**
  * ice_get_valid_rss_size - return valid number of RSS queues
  * @hw: pointer to the HW structure
@@ -4815,6 +4905,8 @@ static const struct ethtool_ops ice_ethtool_ops = {
 	.set_fecparam		= ice_set_fecparam,
 	.get_module_info	= ice_get_module_info,
 	.get_module_eeprom	= ice_get_module_eeprom,
+	.get_module_mgmt_signal	= ice_get_module_mgmt_signal,
+	.set_module_mgmt_signal = ice_set_module_mgmt_signal,
 };
 
 static const struct ethtool_ops ice_ethtool_safe_mode_ops = {
@@ -4837,6 +4929,8 @@ static const struct ethtool_ops ice_ethtool_safe_mode_ops = {
 	.set_ringparam		= ice_set_ringparam,
 	.nway_reset		= ice_nway_reset,
 	.get_channels		= ice_get_channels,
+	.get_module_mgmt_signal	= ice_get_module_mgmt_signal,
+	.set_module_mgmt_signal = ice_set_module_mgmt_signal,
 };
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d390157b59fe..02b9809561e1 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5294,6 +5294,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	hw->port_info = NULL;
 	hw->vendor_id = pdev->vendor;
 	hw->device_id = pdev->device;
+	hw->has_module_mgmt_gpio = ice_set_has_gpios(hw);
 	pci_read_config_byte(pdev, PCI_REVISION_ID, &hw->revision_id);
 	hw->subsystem_vendor_id = pdev->subsystem_vendor;
 	hw->subsystem_device_id = pdev->subsystem_device;
diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
index 0aab21113cc4..e88075ae4c8a 100644
--- a/drivers/net/ethernet/intel/ice/ice_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_type.h
@@ -895,7 +895,7 @@ struct ice_hw {
 	u32 psm_clk_freq;
 	u64 debug_mask;		/* bitmap for debug mask */
 	enum ice_mac_type mac_type;
-
+	bool has_module_mgmt_gpio;	/* has GPIO for module management */
 	u16 fd_ctr_base;	/* FD counter base index */
 
 	/* pci info */
-- 
2.45.2

Re: [PATCH net-next v2 1/2] ethtool: qsfp transceiver reset, interrupt and presence pin control

[Jakub Kicinski]

On Tue, 13 May 2025 22:40:00 +0000 Marek Pazdan wrote:
> Common Management Interface Specification defines
> Management Signaling Layer (MSL) control and status signals. This change
> provides API for following signals status reading:
> - signal allowing the host to request module reset (Reset)
> - signal allowing the host to detect module presence (Presence)
> - signal allowing the host to detect module interrupt (Int)
> Additionally API allows for Reset signal assertion with
> following constraints:
> - reset cannot be asserted if firmware update is in progress
> - if reset is asserted, firmware update cannot be started
> - if reset is asserted, power mode cannot be get/set
> In all above constraint cases -EBUSY error is returned.
> 
> After reset, module will set all registers to default
> values. Default value for Page0 byte 93 register is 0x00 what implies that
> module power mode after reset depends on LPMode HW pin state.
> If software power mode control is required, bit 0 of Page0 byte93 needs
> to be enabled.
> Module reset assertion implies failure of every module's related
> SMBus transactions. Device driver developers should take this into
> consideration if driver provides API for querying module's related data.
> One example can be HWMON providing module temperature report.
> In such case driver should monitor module status and in time of reset
> assertion it should return HWMON report which informs that temperature
> data is not available due to module's reset state.
> The same applies to power mode set/get. Ethtool API has already
> checking for module reset state but similar checking needs to be
> implemented in the driver if it requests power mode for other
> functionality.
> Additionally module reset is link hitful operation. Link is brought down
> when reset is asserted. If device driver doesn't provide functionality
> for monitoring transceiver state, it needs to be implemented in parallel
> to get/set_module_mgmt_signal API. When module reset gets deasserted,
> transceiver process reinitialization. The end of reinitialization
> process is signalled via Page 00h Byte 6 bit 0 "Initialization complete
> flags". If there is no implementation for monitoring this bit in place,
> it needs to be added to bring up the link after transceiver
> initialization is complete.
> 
> Signed-off-by: Marek Pazdan <mpazdan@arista.com>

A few drive by comments, I leave the real review to Andrew.

Instead of posting in-reply-to please add lore links to previous
versions, eg.
v2:
https://lore.kernel.org/all/20250513224017.202236-1-mpazdan@arista.com/ under the --- separator.

I almost missed this posting.

When you post v3 please make sure to CC Ido and Danielle who
implemented the FW flashing for modules.

> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index c650cd3dcb80..38eebbe18f55 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -1528,6 +1528,24 @@ attribute-sets:
>          name: hwtstamp-flags
>          type: nest
>          nested-attributes: bitset
> +  -
> +    name: module-mgmt
> +    attr-cnt-name: __ethtool-a-module-mgmt-cnt
> +    attributes:
> +      -
> +        name: unspec
> +        type: unused
> +        value: 0

no need, just skip the unspec attr and YNL will number the first one
from 1 keeping 0 as rejected type.

> +      -
> +        name: header
> +        type: nest
> +        nested-attributes: header
> +      -
> +        name: type
> +        type: u8

u32 will give us more flexibility later. And attr sizes in netlink are
aligned to 4B so a u8 consumes 4 bytes anyway.

> +      -
> +        name: value
> +        type: u8

Do you think we'll never need to set / clear / change multiple bits at
once? We could wrap the type / value into a nest and support repeating
that nest (multi-attr: true)

> +/**
> + * enum ethtool_module_mgmt_signal_type - plug-in module discrete
> + *	status hardware signals for management as per CMIS spec.
> + * @ETHTOOL_MODULE_MGMT_RESET: Signal allowing the host to request
> + *	a module reset.
> + * @ETHTOOL_MODULE_MGMT_INT: Signal allowing the module to assert
> + *	an interrupt request to the host.
> + * @ETHTOOL_MODULE_MGMT_PRESENT: Signal allowing the module to signal
> + *	its presence status to the host.

Not sure what the use case would be for setting INT and PRESENT.
So the combined API (driver facing) to treat RESET and read-only
bits as equivalent may not be the best fit. Just a feeling tho.

> + */
> +enum ethtool_module_mgmt_signal_type {
> +	ETHTOOL_MODULE_MGMT_RESET = 1,
> +	ETHTOOL_MODULE_MGMT_INT,
> +	ETHTOOL_MODULE_MGMT_PRESENT,

Please define the enums in the YNL spec, see
https://lore.kernel.org/all/20250508193645.78e1e4d9@kernel.org/

Re: [PATCH net-next v2 1/2] ethtool: qsfp transceiver reset, interrupt and presence pin control

[Andrew Lunn]

On Tue, May 13, 2025 at 10:40:00PM +0000, Marek Pazdan wrote:
> Common Management Interface Specification defines
> Management Signaling Layer (MSL) control and status signals. This change
> provides API for following signals status reading:
> - signal allowing the host to request module reset (Reset)
> - signal allowing the host to detect module presence (Presence)
> - signal allowing the host to detect module interrupt (Int)

What is missing from here is the use cases you are trying to
address. Why should user space want to reset the module? Why does user
spare care if there is a module inserted or not. What is user space
going to do with an interrupt?

> Additionally API allows for Reset signal assertion with
> following constraints:
> - reset cannot be asserted if firmware update is in progress
> - if reset is asserted, firmware update cannot be started
> - if reset is asserted, power mode cannot be get/set
> In all above constraint cases -EBUSY error is returned.

Seems like there should be one more condition. Reset cannot be
asserted if the interface is admin up. I assume a reset is disruptive
to the link, so you don't want it to happen when the link is in use.

> +static int module_mgmt_get(struct net_device *dev,
> +			   struct module_mgmt_reply_data *data,
> +			   const struct genl_info *info)
> +{
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	struct netlink_ext_ack *extack = info ? info->extack : NULL;
> +
> +	if (!ops->get_module_mgmt_signal)
> +		return -EOPNOTSUPP;
> +
> +	return ops->get_module_mgmt_signal(dev, &data->mgmt, extack);

Should there be a module_busy() check here? Can you get these
parameters if the module is in reset?

	Andrew

Re: [Intel-wired-lan] [PATCH net-next v2 2/2] ice: add qsfp transceiver reset, interrupt and presence pin control

[Andrew Lunn]

> + * ice_get_module_mgmt_signal - get module management signal status
> + * @dev: network interface device structure
> + * @params: ethtool module management signal params
> + * @extack: extended ACK from the Netlink message
> + *
> + * Returns -EIO if AQ command for GPIO get failed, otherwise
> + * returns 0 and current status of requested signal in params.
> + */
> +static int
> +ice_get_module_mgmt_signal(struct net_device *dev,
> +			   struct ethtool_module_mgmt_params *params,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct ice_netdev_priv *np = netdev_priv(dev);
> +	struct ice_pf *pf = np->vsi->back;
> +	struct ice_hw *hw = &pf->hw;
> +	u16 gpio_handle = 0; /* SOC/on-chip GPIO */
> +	bool value;
> +	int ret = 0;
> +
> +	if (hw->has_module_mgmt_gpio) {
> +		switch (params->type) {
> +		case ETHTOOL_MODULE_MGMT_RESET:
> +			ret = ice_aq_get_gpio(hw, gpio_handle,
> +					      ICE_MGMT_PIN_RESET, &value, NULL);
> +			break;

Reset, i can kind of understand being used this way.

> +		case ETHTOOL_MODULE_MGMT_INT:
> +			ret = ice_aq_get_gpio(hw, gpio_handle,
> +					      ICE_MGMT_PIN_INT, &value, NULL);
> +			break;
> +		case ETHTOOL_MODULE_MGMT_PRESENT:
> +			ret = ice_aq_get_gpio(hw, gpio_handle,
> +					      ICE_MGMT_PIN_PRESENT, &value, NULL);
> +			break;

but not these two. These represent events. I've not looked at the
datasheet... Does the GPIO controller support interrupts? For PRESENT
you are interested in the edges, maybe with some debounce logic. For
INT, i _guess_ it is active low? But i've no idea what user space is
going to actually do on an interrupt, and how is it going to clear the
interrupt? This smells like a user space driver, which is not
something we want. Even if there is a legitimate use case, which there
might be for PRESENT, polling does not make much sense when the kernel
can broadcast a netlink message to user space if the GPIO controller
has interrupt support.

    Andrew

---
pw-bot: cr