[RFC,net-next,04/10] ethtool: Add link extended state
diff mbox series

Message ID 20200607145945.30559-5-amitc@mellanox.com
State New
Headers show
Series
  • Add extended state
Related show

Commit Message

Amit Cohen June 7, 2020, 2:59 p.m. UTC
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@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 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(+)

Comments

Andrew Lunn June 7, 2020, 3:30 p.m. UTC | #1
> +/**
> + * enum ethtool_ext_substate_cable_issue - more information in
> + * addition to ETHTOOL_EXT_STATE_CABLE_ISSUE.
> + */
> +enum ethtool_ext_substate_cable_issue {
> +	ETHTOOL_EXT_SUBSTATE_UNSUPPORTED_CABLE = 1,
> +	ETHTOOL_EXT_SUBSTATE_SHORTED_CABLE,
> +};

I'm not too happy about shorted cable. I can see this getting extended
to open cable, shorted to another pair, etc. It then becomes a
duplicate of the PHY cable testing infrastructure. A more generic

> +	ETHTOOL_EXT_SUBSTATE_CABLE_TEST_FAILURE,

would be better, and then the user can use then use the cable testing
infrastructure to get the full details.

	Andrew
Andrew Lunn June 7, 2020, 4:02 p.m. UTC | #2
> @@ -108,6 +131,12 @@ static int linkstate_reply_size(const struct ethnl_req_info *req_base,
>  	if (data->sqi_max != -EOPNOTSUPP)
>  		len += nla_total_size(sizeof(u32));
>  
> +	if (data->ext_state_provided)
> +		len += sizeof(u8); /* LINKSTATE_EXT_STATE */
> +
> +	if (data->ethtool_ext_state_info.__ext_substate)
> +		len += sizeof(u8); /* LINKSTATE_EXT_SUBSTATE */
> +

This looks wrong. A u8 attribute takes up a lot more space than
sizeof(u8) because of the TLV overheads. That is what the
nla_total_size() is for.

		 Andrew
Andrew Lunn June 7, 2020, 4:12 p.m. UTC | #3
> +static void linkstate_get_ext_state(struct net_device *dev,
> +				    struct linkstate_reply_data *data)
> +{
> +	int err;
> +
> +	if (!dev->ethtool_ops->get_ext_state)
> +		return;
> +
> +	err = dev->ethtool_ops->get_ext_state(dev, &data->ethtool_ext_state_info);
> +	if (err) {
> +		data->ext_state_provided = false;
> +		return;
> +	}
> +
> +	data->ext_state_provided = true;
>  }

A void function is rather odd for this sort of thing. It is much more
normal to return an error code, -EOPNOTSUPP if the op is not available,
or 0 if it all went well.

   Andrew
Andrew Lunn June 7, 2020, 4:27 p.m. UTC | #4
On Sun, Jun 07, 2020 at 05:59:39PM +0300, 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.

Maybe it is hiding somewhere, but shoudn't there be a check to see if
the interface is administratively up? I don't think the information
returned here makes much sense if the interface is configured down.

	 Andrew
Florian Fainelli June 7, 2020, 6:17 p.m. UTC | #5
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@mellanox.com>
> Reviewed-by: Petr Machata <petrm@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  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.
Amit Cohen June 8, 2020, 10:03 a.m. UTC | #6
Andrew Lunn <andrew@lunn.ch> writes:

>> +/**
>> + * enum ethtool_ext_substate_cable_issue - more information in
>> + * addition to ETHTOOL_EXT_STATE_CABLE_ISSUE.
>> + */
>> +enum ethtool_ext_substate_cable_issue {
>> +	ETHTOOL_EXT_SUBSTATE_UNSUPPORTED_CABLE = 1,
>> +	ETHTOOL_EXT_SUBSTATE_SHORTED_CABLE,
>> +};
>
>I'm not too happy about shorted cable. I can see this getting extended to open cable, shorted to another pair, etc. It then becomes a duplicate of the PHY cable testing infrastructure. A more generic
>
>> +	ETHTOOL_EXT_SUBSTATE_CABLE_TEST_FAILURE,
>
>would be better, and then the user can use then use the cable testing infrastructure to get the full details.
>

Ok, makes sense.

>	Andrew
Amit Cohen June 9, 2020, 10:24 a.m. UTC | #7
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@mellanox.com>
>> Reviewed-by: Petr Machata <petrm@mellanox.com>
>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  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.
>

Patch
diff mbox series

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.
  * @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,
+	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,
+};
+
+/**
+ * enum ethtool_ext_substate_logical_mismatch - more information in addition
+ * to ETHTOOL_EXT_STATE_LINK_LOGICAL_MISMATCH.
+ */
+enum ethtool_ext_substate_link_logical_mismatch {
+	ETHTOOL_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_BLOCK_LOCK = 1,
+	ETHTOOL_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_AM_LOCK,
+	ETHTOOL_EXT_SUBSTATE_LLM_PCS_DID_NOT_GET_ALIGN_STATUS,
+	ETHTOOL_EXT_SUBSTATE_LLM_FC_FEC_IS_NOT_LOCKED,
+	ETHTOOL_EXT_SUBSTATE_LLM_RS_FEC_IS_NOT_LOCKED,
+};
+
+/**
+ * enum ethtool_ext_substate_bad_signal_integrity - more information in
+ * addition to ETHTOOL_EXT_STATE_BAD_SIGNAL_INTEGRITY.
+ */
+enum ethtool_ext_substate_bad_signal_integrity {
+	ETHTOOL_EXT_SUBSTATE_BSI_LARGE_NUMBER_OF_PHYSICAL_ERRORS = 1,
+	ETHTOOL_EXT_SUBSTATE_BSI_UNSUPPORTED_RATE,
+};
+
+/**
+ * enum ethtool_ext_substate_cable_issue - more information in
+ * addition to ETHTOOL_EXT_STATE_CABLE_ISSUE.
+ */
+enum ethtool_ext_substate_cable_issue {
+	ETHTOOL_EXT_SUBSTATE_UNSUPPORTED_CABLE = 1,
+	ETHTOOL_EXT_SUBSTATE_SHORTED_CABLE,
+};
+
 #define ETH_GSTRING_LEN		32
 
 /**
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index e6f109b76c9a..1de8b77bedff 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -234,6 +234,8 @@  enum {
 	ETHTOOL_A_LINKSTATE_LINK,		/* u8 */
 	ETHTOOL_A_LINKSTATE_SQI,		/* u32 */
 	ETHTOOL_A_LINKSTATE_SQI_MAX,		/* u32 */
+	ETHTOOL_A_LINKSTATE_EXT_STATE,		/* u8 */
+	ETHTOOL_A_LINKSTATE_EXT_SUBSTATE,	/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKSTATE_CNT,
diff --git a/net/ethtool/linkstate.c b/net/ethtool/linkstate.c
index 7f47ba89054e..0f3a15fe9919 100644
--- a/net/ethtool/linkstate.c
+++ b/net/ethtool/linkstate.c
@@ -13,6 +13,8 @@  struct linkstate_reply_data {
 	int				link;
 	int				sqi;
 	int				sqi_max;
+	bool				ext_state_provided;
+	struct ethtool_ext_state_info	ethtool_ext_state_info;
 };
 
 #define LINKSTATE_REPDATA(__reply_base) \
@@ -25,6 +27,8 @@  linkstate_get_policy[ETHTOOL_A_LINKSTATE_MAX + 1] = {
 	[ETHTOOL_A_LINKSTATE_LINK]		= { .type = NLA_REJECT },
 	[ETHTOOL_A_LINKSTATE_SQI]		= { .type = NLA_REJECT },
 	[ETHTOOL_A_LINKSTATE_SQI_MAX]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_LINKSTATE_EXT_STATE]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE]	= { .type = NLA_REJECT },
 };
 
 static int linkstate_get_sqi(struct net_device *dev)
@@ -61,6 +65,23 @@  static int linkstate_get_sqi_max(struct net_device *dev)
 	mutex_unlock(&phydev->lock);
 
 	return ret;
+};
+
+static void linkstate_get_ext_state(struct net_device *dev,
+				    struct linkstate_reply_data *data)
+{
+	int err;
+
+	if (!dev->ethtool_ops->get_ext_state)
+		return;
+
+	err = dev->ethtool_ops->get_ext_state(dev, &data->ethtool_ext_state_info);
+	if (err) {
+		data->ext_state_provided = false;
+		return;
+	}
+
+	data->ext_state_provided = true;
 }
 
 static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
@@ -88,6 +109,8 @@  static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
 
 	data->sqi_max = ret;
 
+	linkstate_get_ext_state(dev, data);
+
 	ethnl_ops_complete(dev);
 
 	return 0;
@@ -108,6 +131,12 @@  static int linkstate_reply_size(const struct ethnl_req_info *req_base,
 	if (data->sqi_max != -EOPNOTSUPP)
 		len += nla_total_size(sizeof(u32));
 
+	if (data->ext_state_provided)
+		len += sizeof(u8); /* LINKSTATE_EXT_STATE */
+
+	if (data->ethtool_ext_state_info.__ext_substate)
+		len += sizeof(u8); /* LINKSTATE_EXT_SUBSTATE */
+
 	return len;
 }
 
@@ -129,6 +158,17 @@  static int linkstate_fill_reply(struct sk_buff *skb,
 	    nla_put_u32(skb, ETHTOOL_A_LINKSTATE_SQI_MAX, data->sqi_max))
 		return -EMSGSIZE;
 
+	if (data->ext_state_provided) {
+		if (nla_put_u8(skb, ETHTOOL_A_LINKSTATE_EXT_STATE,
+			       data->ethtool_ext_state_info.ext_state))
+			return -EMSGSIZE;
+
+		if (data->ethtool_ext_state_info.__ext_substate &&
+		    nla_put_u8(skb, ETHTOOL_A_LINKSTATE_EXT_SUBSTATE,
+			       data->ethtool_ext_state_info.__ext_substate))
+			return -EMSGSIZE;
+	}
+
 	return 0;
 }