Re: [RFC PATCH net-next 04/10] ethtool: Add link extended state

[Florian Fainelli]

On 6/7/2020 7:59 AM, Amit Cohen wrote:
> Currently, drivers can only tell whether the link is up/down using
> LINKSTATE_GET, but no additional information is given.
> 
> Add attributes to LINKSTATE_GET command in order to allow drivers
> to expose the user more information in addition to link state to ease
> the debug process, for example, reason for link down state.
> 
> Extended state consists of two attributes - ext_state and ext_substate.
> The idea is to avoid 'vendor specific' states in order to prevent
> drivers to use specific ext_state that can be in the future common
> ext_state.
> 
> The substates allows drivers to add more information to the common
> ext_state. For example, vendor can expose 'Autoneg failure' as
> ext_state and add 'No partner detected during force mode' as
> ext_substate.
> 
> If a driver cannot pinpoint the extended state with the substate
> accuracy, it is free to expose only the extended state and omit the
> substate attribute.
> 
> Signed-off-by: Amit Cohen <amitc@xxxxxxxxxxxx>
> Reviewed-by: Petr Machata <petrm@xxxxxxxxxxxx>
> Reviewed-by: Jiri Pirko <jiri@xxxxxxxxxxxx>
> ---
>  include/linux/ethtool.h              | 22 +++++++++
>  include/uapi/linux/ethtool.h         | 70 ++++++++++++++++++++++++++++
>  include/uapi/linux/ethtool_netlink.h |  2 +
>  net/ethtool/linkstate.c              | 40 ++++++++++++++++
>  4 files changed, 134 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index a23b26eab479..48ec542f4504 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -86,6 +86,22 @@ struct net_device;
>  u32 ethtool_op_get_link(struct net_device *dev);
>  int ethtool_op_get_ts_info(struct net_device *dev, struct ethtool_ts_info *eti);
>  
> +
> +/**
> + * struct ethtool_ext_state_info - link extended state and substate.
> + */
> +struct ethtool_ext_state_info {
> +	enum ethtool_ext_state ext_state;
> +	union {
> +		enum ethtool_ext_substate_autoneg autoneg;
> +		enum ethtool_ext_substate_link_training link_training;
> +		enum ethtool_ext_substate_link_logical_mismatch link_logical_mismatch;
> +		enum ethtool_ext_substate_bad_signal_integrity bad_signal_integrity;
> +		enum ethtool_ext_substate_cable_issue cable_issue;
> +		int __ext_substate;
> +	};
> +};
> +
>  /**
>   * ethtool_rxfh_indir_default - get default value for RX flow hash indirection
>   * @index: Index in RX flow hash indirection table
> @@ -245,6 +261,10 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>   * @get_link: Report whether physical link is up.  Will only be called if
>   *	the netdev is up.  Should usually be set to ethtool_op_get_link(),
>   *	which uses netif_carrier_ok().
> + * @get_ext_state: Report link extended state. Should set ext_state and
> + *	ext_substate (ext_substate of 0 means ext_substate is unknown,
> + *	do not attach ext_substate attribute to netlink message). If not
> + *	implemented, ext_state and ext_substate will not be sent to userspace.

For consistency with the other link-related operations, I would name
this get_link_ext_state.

>   * @get_eeprom: Read data from the device EEPROM.
>   *	Should fill in the magic field.  Don't need to check len for zero
>   *	or wraparound.  Fill in the data argument with the eeprom values
> @@ -384,6 +404,8 @@ struct ethtool_ops {
>  	void	(*set_msglevel)(struct net_device *, u32);
>  	int	(*nway_reset)(struct net_device *);
>  	u32	(*get_link)(struct net_device *);
> +	int	(*get_ext_state)(struct net_device *,
> +				 struct ethtool_ext_state_info *);
>  	int	(*get_eeprom_len)(struct net_device *);
>  	int	(*get_eeprom)(struct net_device *,
>  			      struct ethtool_eeprom *, u8 *);
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f4662b3a9e1e..830fa0d6aebe 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -579,6 +579,76 @@ struct ethtool_pauseparam {
>  	__u32	tx_pause;
>  };
>  
> +/**
> + * enum ethtool_ext_state - link extended state
> + */
> +enum ethtool_ext_state {
> +	ETHTOOL_EXT_STATE_AUTONEG_FAILURE,
> +	ETHTOOL_EXT_STATE_LINK_TRAINING_FAILURE,
> +	ETHTOOL_EXT_STATE_LINK_LOGICAL_MISMATCH,
> +	ETHTOOL_EXT_STATE_BAD_SIGNAL_INTEGRITY,
> +	ETHTOOL_EXT_STATE_NO_CABLE,
> +	ETHTOOL_EXT_STATE_CABLE_ISSUE,
> +	ETHTOOL_EXT_STATE_EEPROM_ISSUE,

Does the EEPROM issue would indicate for instance that it was not
possile for the firmware/kernel to determine what transceiver
capabilities are supported from e.g.: a SFP or SFF EEPROM, and therefore
the link state could be down because of that. Is this the idea?

> +	ETHTOOL_EXT_STATE_CALIBRATION_FAILURE,
> +	ETHTOOL_EXT_STATE_POWER_BUDGET_EXCEEDED,
> +	ETHTOOL_EXT_STATE_OVERHEAT,
> +};
> +
> +/**
> + * enum ethtool_ext_substate_autoneg - more information in addition to
> + * ETHTOOL_EXT_STATE_AUTONEG_FAILURE.
> + */
> +enum ethtool_ext_substate_autoneg {
> +	ETHTOOL_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED = 1,
> +	ETHTOOL_EXT_SUBSTATE_AN_ACK_NOT_RECEIVED,
> +	ETHTOOL_EXT_SUBSTATE_AN_NEXT_PAGE_EXCHANGE_FAILED,
> +	ETHTOOL_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED_FORCE_MODE,
> +	ETHTOOL_EXT_SUBSTATE_AN_FEC_MISMATCH_DURING_OVERRIDE,
> +	ETHTOOL_EXT_SUBSTATE_AN_NO_HCD,
> +};
> +
> +/**
> + * enum ethtool_ext_substate_link_training - more information in addition to
> + * ETHTOOL_EXT_STATE_LINK_TRAINING_FAILURE.
> + */
> +enum ethtool_ext_substate_link_training {
> +	ETHTOOL_EXT_SUBSTATE_LT_KR_FRAME_LOCK_NOT_ACQUIRED = 1,
> +	ETHTOOL_EXT_SUBSTATE_LT_KR_LINK_INHIBIT_TIMEOUT,
> +	ETHTOOL_EXT_SUBSTATE_LT_KR_LINK_PARTNER_DID_NOT_SET_RECEIVER_READY,
> +	ETHTOOL_EXT_SUBSTATE_LT_REMOTE_FAULT,
> +};

OK, so we leave it to the driver to report link sub-state information
that is relevnt to the supported/avertised link modes, such that for
instance, reporting LT_KR_FRAME_LOCK_NOT_ACQUIRED would not happen if we
were only advertising 1000baseT for instance. That sounds fair.
-- 
Florian