Message-ID: <1448934751.30642.32.camel@decadent.org.uk>
Date:	Tue, 01 Dec 2015 01:52:31 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	David Decotigny <ddecotig@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Amir Vadai <amirv@...lanox.com>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, linux-api@...r.kernel.org,
	linux-mips@...ux-mips.org, fcoe-devel@...n-fcoe.org
Cc:	Eric Dumazet <edumazet@...gle.com>,
	Eugenia Emantayev <eugenia@...lanox.co.il>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Ido Shamay <idos@...lanox.com>, Joe Perches <joe@...ches.com>,
	Saeed Mahameed <saeedm@...lanox.com>,
	Govindarajulu Varadarajan <_govind@....com>,
	Venkata Duvvuru <VenkatKumar.Duvvuru@...lex.Com>,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	Eyal Perry <eyalpe@...lanox.com>,
	Pravin B Shelar <pshelar@...ira.com>,
	Ed Swierk <eswierk@...portsystems.com>,
	Robert Love <robert.w.love@...el.com>,
	"James E.J. Bottomley" <JBottomley@...allels.com>,
	Yuval Mintz <Yuval.Mintz@...gic.com>,
	David Decotigny <decot@...glers.com>
Subject: Re: [PATCH net-next v3 03/17] net: ethtool: add new
 ETHTOOL_GSETTINGS/SSETTINGS API

On Mon, 2015-11-30 at 14:05 -0800, David Decotigny wrote:
> From: David Decotigny <decot@...glers.com>
> 
> This patch defines a new ETHTOOL_GSETTINGS/SSETTINGS API, handled by
> the new get_ksettings/set_ksettings callbacks. This API provides
> support for most legacy ethtool_cmd fields, adds support for larger
> link mode masks (up to 4064 bits, variable length), and removes
> ethtool_cmd deprecated fields (transceiver/maxrxpkt/maxtxpkt).

As you have to introduce new commands and a new structure, please
include the word 'link' in their names.

[...]
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 653dc9c..6de122d 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -12,6 +12,7 @@
>  #ifndef _LINUX_ETHTOOL_H
>  #define _LINUX_ETHTOOL_H
>  
> +#include 
>  #include 
>  #include 
>  
> @@ -40,9 +41,6 @@ struct compat_ethtool_rxnfc {
>  
>  #include 
>  
> -extern int __ethtool_get_settings(struct net_device *dev,
> -				  struct ethtool_cmd *cmd);
> -
>  /**
>   * enum ethtool_phys_id_state - indicator state for physical identification
>   * @ETHTOOL_ID_INACTIVE: Physical ID indicator should be deactivated
> @@ -97,13 +95,85 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>  	return index % n_rx_rings;
>  }
>  
> +#define __ETHTOOL_LINK_MODE_IS_VALID_BIT(indice)	\
> +	((indice) >= 0 && (indice) <= __ETHTOOL_LINK_MODE_LAST)

'indice'?  Shoudn't this be 'index' or 'mode'?

> 
> +/* number of link mode bits handled internally by kernel */
> +#define __ETHTOOL_LINK_MODE_MASK_NBITS (__ETHTOOL_LINK_MODE_LAST+1)

Spaces around the + sign.

> 
> +typedef struct {
> +	unsigned long mask[BITS_TO_LONGS(__ETHTOOL_LINK_MODE_MASK_NBITS)];
> +} ethtool_link_mode_mask_t;

checkpatch claims you shouldn't introduce such typedefs.

[...]
> 
> +/**
> + * struct ethtool_settings - link control and status
> + * This structure deprecates struct %ethtool_cmd.

We do the deprecating; the structures are purely passive.

[...]
> 
> + * Deprecated fields should be ignored by both users and drivers.

Delete this last paragraph.

[...]
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -352,6 +352,388 @@ static int __ethtool_set_flags(struct net_device *dev, u32 data)
>  	return 0;
>  }
>  
> +/* TODO: remove when %ETHTOOL_GSET/%ETHTOOL_SSET disappear */

Please delete this TODO and all the similar ones; we don't remove
userland APIs just because they're deprecated.

[...]
> 
> +/* number of 32-bit words to store the user's link mode bitmaps */
> +#define __ETHTOOL_LINK_MODE_MASK_NU32			\
> +	((__ETHTOOL_LINK_MODE_MASK_NBITS + 31) / 32)

Should use DIV_ROUND_UP().

> 
> +/* layout of the struct passed from/to userland */
> +struct ethtool_usettings {
> +	struct ethtool_settings parent;
> +	struct {
> +		__u32 supported[__ETHTOOL_LINK_MODE_MASK_NU32] __aligned(4);
> +		__u32 advertising[__ETHTOOL_LINK_MODE_MASK_NU32] __aligned(4);
> +		__u32 lp_advertising[
> +			__ETHTOOL_LINK_MODE_MASK_NU32] __aligned(4);

Why __aligned(4)?  Do you have any reason to think that some padding
might be added otherwise?

[...]
> +#if BITS_PER_LONG == 64
> +static unsigned long _shl32(__u32 v)
> +{
> +	return ((unsigned long)v) << 32;
> +}
> +#endif
> +
> +/* convert a user's __u32[] bitmap in user space to a kernel internal
> + * bitmap. return 0 on success, errno on error. this assumes that
> + * link_mode_masks_nwords was already verified
> + */
> +static int load_ksettings_from_user(struct ethtool_ksettings *to,
> +				    const void __user *from)
> +{
> +	struct ethtool_usettings usettings;
> +#if BITS_PER_LONG != 32
> +	unsigned i;
> +#endif
> +
> +	if (copy_from_user(&usettings, from, sizeof(usettings)))
> +		return -EFAULT;
> +
> +	/* make sure we didn't receive garbage between last allowed bit
> +	 * and end of last u32 word
> +	 */
> +	if (__ETHTOOL_LINK_MODE_MASK_NBITS % 32) {
> +		const u32 allowed = (
> +			1U << (__ETHTOOL_LINK_MODE_MASK_NBITS % 32)) - 1;
> +		if (usettings.link_modes.supported[
> +			    __ETHTOOL_LINK_MODE_MASK_NU32 - 1] & ~allowed)
> +			return -EINVAL;
> +		if (usettings.link_modes.advertising[
> +			    __ETHTOOL_LINK_MODE_MASK_NU32 - 1] & ~allowed)
> +			return -EINVAL;
> +		if (usettings.link_modes.lp_advertising[
> +			    __ETHTOOL_LINK_MODE_MASK_NU32 - 1] & ~allowed)
> +			return -EINVAL;
> +	}
> +
> +#if BITS_PER_LONG == 32
> +	compiletime_assert(sizeof(*to) == sizeof(usettings),
> +			   "sizeof(ulong) != 32");
> +	memcpy(to, &usettings, sizeof(*to));
> +#elif BITS_PER_LONG == 64
> +	memset(to, 0, sizeof(*to));

This memset() looks redundant.

> +	memcpy(&to->parent, &usettings.parent, sizeof(to->parent));
> +	for (i = 0 ; i < __ETHTOOL_LINK_MODE_MASK_NU32 ; ++i) {
> +		if (0 == (i & 1)) {
> +			to->link_modes.supported.mask[i >> 1]
> +				= usettings.link_modes.supported[i];
> +			to->link_modes.advertising.mask[i >> 1]
> +				= usettings.link_modes.advertising[i];
> +			to->link_modes.lp_advertising.mask[i >> 1]
> +				= usettings.link_modes.lp_advertising[i];
> +		} else {
> +			to->link_modes.supported.mask[i >> 1] |= _shl32(
> +				usettings.link_modes.supported[i]);
> +			to->link_modes.advertising.mask[i >> 1] |= _shl32(
> +				usettings.link_modes.advertising[i]);
> +			to->link_modes.lp_advertising.mask[i >> 1] |= _shl32(
> +				usettings.link_modes.lp_advertising[i]);
> +		}
> +	}
> +#else
> +# error "unsupported ulong width"
> +#endif
> +	return 0;
> +}

I think the array conversion ought to be a separate function that you
can call 3 times here, rather than repeating it here.  In fact that
could be a general function in lib/bitmap.c.

Similarly for the opposite conversion below.

[...]
>  static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
>  {
> -	int err;
>  	struct ethtool_cmd cmd;
>  
> -	err = __ethtool_get_settings(dev, &cmd);
> -	if (err < 0)
> -		return err;
> +	ASSERT_RTNL();
> +
> +	if (dev->ethtool_ops->get_ksettings) {
> +		/* First, use ksettings API if it is supported */
> +		int err;
> +		struct ethtool_ksettings ksettings;
> +
> +		memset(&ksettings, 0, sizeof(ksettings));
> +		err = dev->ethtool_ops->get_ksettings(dev, &ksettings);
> +		if (err < 0)
> +			return err;
> +		if (!convert_ksettings_to_legacy_settings(&cmd, &ksettings)) {
> +			static int __warned;
> +
> +			/* not all bitmaps could be translated
> +			 * acurately to legacy API: print warning with
> +			 * netdev name, but does still succeed
> +			 */
> +			if (!__warned)
> +				netdev_warn(dev, "please upgrade ethtool\n");

ethtool isn't the only program that uses this API, not by a long way. 
And since it's the program at fault, not the device, it doesn't make a
lot of sense to use netdev_warn().

So I suggest a message more like that in warn_legacy_capability_use()
in kernel/capability.c.

[...]
>  static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
>  {
>  	struct ethtool_cmd cmd;
>  
> -	if (!dev->ethtool_ops->set_settings)
> -		return -EOPNOTSUPP;
> +	ASSERT_RTNL();
>  
>  	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
>  		return -EFAULT;
>  
> +	/* first, try new %ethtool_ksettings API. */
> +	if (dev->ethtool_ops->set_ksettings) {
> +		struct ethtool_ksettings ksettings;
> +
> +		if (!convert_legacy_settings_to_ksettings(&ksettings, &cmd)) {
> +			static int __warned;
> +
> +			/* rejecting setting deprecated fields
> +			 * transceiver/maxtxpkt/maxrxpkt
> +			 */
> +			if (!__warned)
> +				netdev_warn(dev, "please upgrade ethtool");

I don't think this makes sense - it's not as if ethtool will
automatically try to set these without it being explicitly requested by
the user.  Just return -EINVAL without logging anything.

> +			__warned = 1;
> +			return -EINVAL;
> +		}
[...]

Ben.

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
                                - John Levine, moderator of comp.compilers
Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)

Powered by blists - more mailing lists