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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07-Jun-20 21:17, Florian Fainelli wrote:
> 
> 
> 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.
> 

ok.

>>   * @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?
> 

We get this reason from firmware if the cable identifier is not spec compliant, missing or was not able to be read on time (I2C reading issue). 


>> +	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.
> 




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux