| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 9 Jun 2020 13:24:24 +0300
From: Amit Cohen <amitc@...lanox.com>
To: Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, kuba@...nel.org, corbet@....net,
jiri@...lanox.com, idosch@...lanox.com, shuah@...nel.org,
mkubecek@...e.cz, gustavo@...eddedor.com,
cforno12@...ux.vnet.ibm.com, andrew@...n.ch,
linux@...pel-privat.de, alexandru.ardelean@...log.com,
ayal@...lanox.com, petrm@...lanox.com, mlxsw@...lanox.com,
liuhangbin@...il.com, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [RFC PATCH net-next 04/10] ethtool: Add link extended state
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@...lanox.com>
>> Reviewed-by: Petr Machata <petrm@...lanox.com>
>> Reviewed-by: Jiri Pirko <jiri@...lanox.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.
>
Powered by blists - more mailing lists