linux-kernel - Re: [PATCH net-next v1 1/7] net: ethtool: extend link mode support to 48 bits

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: <1420408867.5686.145.camel@decadent.org.uk>
Date:	Sun, 04 Jan 2015 22:01:07 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	David Decotigny <ddecotig@...il.com>
Cc:	Amir Vadai <amirv@...lanox.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-api@...r.kernel.org, David Decotigny <decot@...glers.com>,
	"David S. Miller" <davem@...emloft.net>,
	Jason Wang <jasowang@...hat.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Al Viro <viro@...iv.linux.org.uk>,
	Masatake YAMATO <yamato@...hat.com>, Xi Wang <xii@...gle.com>,
	Neil Horman <nhorman@...driver.com>,
	WANG Cong <xiyou.wangcong@...il.com>,
	Flavio Leitner <fbl@...hat.com>, Tom Gundersen <teg@...m.no>,
	Jiri Pirko <jiri@...nulli.us>,
	Vlad Yasevich <vyasevic@...hat.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Saeed Mahameed <saeedm@...lanox.com>,
	Venkata Duvvuru <VenkatKumar.Duvvuru@...lex.Com>,
	Govindarajulu Varadarajan <_govind@....com>
Subject: Re: [PATCH net-next v1 1/7] net: ethtool: extend link mode support
 to 48 bits

On Sun, 2015-01-04 at 12:56 -0800, David Decotigny wrote:
> From: David Decotigny <decot@...glers.com>

This is mostly fine, with just a few minor issues.

> Signed-off-by: David Decotigny <decot@...glers.com>
> ---
>  include/uapi/linux/ethtool.h | 130 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 110 insertions(+), 20 deletions(-)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 5f66d9c..61e7734 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
[...]
> @@ -123,6 +140,46 @@ static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
>  	return (ep->speed_hi << 16) | ep->speed;
>  }
>  
> +/**
> + * MAKE_ETHTOOL_LINK_MODE_ACCESSORS - create the link_mode accessors
> + * Macro to generate the %ethtool_cmd_supported_*,
> + * %ethtool_cmd_advertising_*, %ethtool_cmd_lp_advertising_*,
> + * %ethtool_eee_supported_*, %ethtool_eee_advertised_*,
> + * %ethtool_eee_lp_advertised_* families of functions.
> + *
> + * Macro args:

Delete the 'Macro args:' heading; that's implied by the @ prefixes.

> + *  @struct_name: either %ethtool_cmd or %ethtool_eee
> + *  @field_name: name of the fields in %struct_name to
> + *      access. supported/advertising/lp_advertising for ethtool_cmd,
> + *      supported/advertised/lp_advertised for ethtool_eee
> + *
> + * Generates the following static functions:
> + *  @ethtool_cmd_supported(const struct ethtool_cmd*): returns
> + *      the 64b value of %supported fields (the upper bits 63..48 are 0)
> + *  @ethtool_cmd_supported_set(struct ethtool_cmd*,
> + *      ethtool_link_mode_mask_t value): set the %supported fields to
> + *      given %value (only the lower 48 bits used, upper bits 63..48
> + *      ignored)

Delete the @ prefixes from these headings.

> + * Same doc for all the other functions.
> + */
> +#define MAKE_ETHTOOL_LINK_MODE_ACCESSORS(struct_name, field_name)	\

I think the name should begin with ETHTOOL but it's not a big deal.

[...]
> @@ -1192,6 +1273,9 @@ enum ethtool_sfeatures_retval_bits {
>  #define SPARC_ETH_GSET		ETHTOOL_GSET
>  #define SPARC_ETH_SSET		ETHTOOL_SSET
>  
> +/* Do not use the following macros directly to update
> + * ethtool_cmd::supported, ethtool_eee::supported. Please use
> + * ethtool_(cmd|eee)_supported(|_set) instead */

The ending */ belongs on a new line.

>  #define SUPPORTED_10baseT_Half		(1 << 0)
>  #define SUPPORTED_10baseT_Full		(1 << 1)
>  #define SUPPORTED_100baseT_Half		(1 << 2)
> @@ -1223,7 +1307,12 @@ enum ethtool_sfeatures_retval_bits {
>  #define SUPPORTED_56000baseCR4_Full	(1 << 28)
>  #define SUPPORTED_56000baseSR4_Full	(1 << 29)
>  #define SUPPORTED_56000baseLR4_Full	(1 << 30)
> +/* TODO: for bit indices >= 31, make sure to shift 1ULL instead of 1 */
[...]

I don't think that comment is necessary; the compiler will surely warn
if someone forgets.

Ben.

-- 
Ben Hutchings
This sentence contradicts itself - no actually it doesn't.

Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)