Re: [PATCH net-next v3 03/17] net: ethtool: add new ETHTOOL_GSETTINGS/SSETTINGS API

[Ben Hutchings]

On Mon, 2015-11-30 at 14:05 -0800, David Decotigny wrote:
> From: David Decotigny <decot@xxxxxxxxxxxx>
> 
> 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

Attachment: signature.asc
Description: This is a digitally signed message part