netdev - Re: [RFC PATCH net-next 1/3] ethtool: Add link down reason callback

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]

Message-ID: <8bdd521e-0a6f-b02d-55c3-051d1acbd777@mellanox.com>
Date:   Mon, 26 Jun 2017 18:48:18 +0300
From:   Gal Pressman <galp@...lanox.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        "John W. Linville" <linville@...driver.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Vidya Sagar Ravipati <vidya@...ulusnetworks.com>,
        Jiri Pirko <jiri@...lanox.com>,
        David Decotigny <decot@...glers.com>, kernel-team@...com
Subject: Re: [RFC PATCH net-next 1/3] ethtool: Add link down reason callback


> On Wed, Jun 21, 2017 at 04:04:44PM +0300, Gal Pressman wrote:
>> Currently, drivers can only tell whether the link is up/down through
>> ETHTOOL_GLINK callback, but no additional information is given in case
>> of link down.
>> This patch provides an infrastructure that allows drivers to hint
>> the user with the reason why the link is down, in order to ease the
>> debug process.
>>
>> Reasons are separated to two types, generic and vendor specific.
>> Drivers can reply with a generic reason using the enums provided in this
>> patch (and the ones that will be added in the future), which will be
>> translated to strings by the userspace ethtool.
>> In case of a vendor specific reason (not suitable for most vendors),
>> drivers can reply with ETHTOOL_VENDOR_SPECIFIC reason, in this case the
>> vendor_reason field should be filled with a vendor specific status code
>> which will be parsed by the vendor specific userspace parser if one is
>> available.
>>
>> This kind of information can save system administrators precious time
>> which will not be wasted trying to understand why the link won't go
>> up.
>>
>> For example, when the cable is unplugged:
>> $ ethtool ethXX
>> ...
>> Link detected: no (Cable unplugged)
>>
>> Signed-off-by: Gal Pressman <galp@...lanox.com>
>> Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
>> ---
>>  include/linux/ethtool.h      |  2 ++
>>  include/uapi/linux/ethtool.h | 33 +++++++++++++++++++++++++++++++++
>>  net/core/ethtool.c           | 24 ++++++++++++++++++++++++
>>  3 files changed, 59 insertions(+)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index 83cc986..d472047 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -374,5 +374,7 @@ struct ethtool_ops {
>>  				      struct ethtool_link_ksettings *);
>>  	int	(*set_link_ksettings)(struct net_device *,
>>  				      const struct ethtool_link_ksettings *);
>> +	int	(*get_link_down_reason)(struct net_device *,
>> +					struct ethtool_link_down_reason *);
>>  };
>>  #endif /* _LINUX_ETHTOOL_H */
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index 7d4a594..8cf9d2c 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -550,6 +550,13 @@ struct ethtool_pauseparam {
>>  
>>  #define ETH_GSTRING_LEN		32
>>  
>> +struct ethtool_link_down_reason {
>> +	__u32	cmd;
>> +	__u32	reason;
>> +	__u32	vendor_reason;
>> +	__u32	reserved[4];
>> +};
> Shouldn't this be a list? The device is over its power budget,
> overheating and does not have a cable plugged in. There can be
> multiple reasons it is down.
Current implementation will report one issue at a time, fix it to reveal the next one :).

Reporting more than one issue is possible, for example:
Add another callback to return the number of issues and allocate a buffer accordingly, sounds like an overkill to me.
Alternatively, use a bitmap instead of the enum, which will probably get too big very quickly.
I don't think it's worth it in this particular case since in most cases one reason should suffice.

>
> 	 Andrew