netdev - Re: [PATCH net-next 05/10] ethtool: Add link extended state

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Jun 2020 23:37:32 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Ido Schimmel <idosch@...sch.org>, davem@...emloft.net,
        kuba@...nel.org, jiri@...lanox.com, petrm@...lanox.com,
        amitc@...lanox.com, mlxsw@...lanox.com, jacob.e.keller@...el.com,
        andrew@...n.ch, f.fainelli@...il.com, linux@...pel-privat.de,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 05/10] ethtool: Add link extended state

On Wed, Jun 24, 2020 at 11:19:18AM +0300, Ido Schimmel wrote:
> From: Amit Cohen <amitc@...lanox.com>
> 
> 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 - link_ext_state and link_ext_substate.

Please keep the commit message within 75 columns.

> The idea is to avoid 'vendor specific' states in order to prevent
> drivers to use specific link_ext_state that can be in the future common
> link_ext_state.
> 
> The substates allows drivers to add more information to the common
> link_ext_state. For example, vendor can expose 'Autoneg' as link_ext_state and add
> 'No partner detected during force mode' as link_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>
> Signed-off-by: Ido Schimmel <idosch@...lanox.com>
> ---
>  include/linux/ethtool.h              | 22 +++++++++
>  include/uapi/linux/ethtool.h         | 70 ++++++++++++++++++++++++++++
>  include/uapi/linux/ethtool_netlink.h |  2 +
>  net/ethtool/linkstate.c              | 56 ++++++++++++++++++++--
>  4 files changed, 145 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index a23b26eab479..be2fd37ec84a 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_link_ext_state_info - link extended state and substate.
> + */
> +struct ethtool_link_ext_state_info {
> +	enum ethtool_link_ext_state link_ext_state;
> +	union {
> +		enum ethtool_link_ext_substate_autoneg autoneg;
> +		enum ethtool_link_ext_substate_link_training link_training;
> +		enum ethtool_link_ext_substate_link_logical_mismatch link_logical_mismatch;
> +		enum ethtool_link_ext_substate_bad_signal_integrity bad_signal_integrity;
> +		enum ethtool_link_ext_substate_cable_issue cable_issue;
> +		int __link_ext_substate;

Shouldn't __link_ext_substate rather be u8?

> +	};
> +};
> +
>  /**
>   * ethtool_rxfh_indir_default - get default value for RX flow hash indirection
>   * @index: Index in RX flow hash indirection table
[...]
> @@ -61,6 +65,25 @@ static int linkstate_get_sqi_max(struct net_device *dev)
>  	mutex_unlock(&phydev->lock);
>  
>  	return ret;
> +};
> +
> +static int linkstate_get_link_ext_state(struct net_device *dev,
> +					struct linkstate_reply_data *data)
> +{
> +	int err;
> +
> +	if (!dev->ethtool_ops->get_link_ext_state)
> +		return -EOPNOTSUPP;
> +
> +	err = dev->ethtool_ops->get_link_ext_state(dev, &data->ethtool_link_ext_state_info);
> +	if (err) {
> +		data->link_ext_state_provided = false;

This is not needed, request specific part of the structure is guaranteed
to be zero initialized.

> +		return err;
> +	}
> +
> +	data->link_ext_state_provided = true;
> +
> +	return 0;
>  }
>  
>  static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
> @@ -69,7 +92,7 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>  {
>  	struct linkstate_reply_data *data = LINKSTATE_REPDATA(reply_base);
>  	struct net_device *dev = reply_base->dev;
> -	int ret;
> +	int ret, err;

It doesn't seem necessary to introduce a second variable for the same
purpose.

>  
>  	ret = ethnl_ops_begin(dev);
>  	if (ret < 0)
> @@ -88,6 +111,12 @@ static int linkstate_prepare_data(const struct ethnl_req_info *req_base,
>  
>  	data->sqi_max = ret;
>  
> +	if (dev->flags & IFF_UP) {
> +		err = linkstate_get_link_ext_state(dev, data);
> +		if (err < 0 && err != -EOPNOTSUPP && err != -ENODATA)
> +			return err;

This return statement would bypass ethnl_ops_complete(). (We already
have the same problem with SQI related error handling as I just
noticed.) Also, the special meaning of -ENODATA should be probably
documented in ->get_link_ext_state() callback description.

Michal