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
