lkml.org 
[lkml]   [2015]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH net-next v3 03/17] net: ethtool: add new ETHTOOL_GSETTINGS/SSETTINGS API
From
Date
On Mon, 2015-11-30 at 14:05 -0800, David Decotigny wrote:
> From: David Decotigny <decot@googlers.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[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2015-12-01 03:01    [W:0.183 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site