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

[Jakub Kicinski <kuba@kernel.org>]

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/