On Mon, 2015-11-30 at 14:05 -0800, David Decotigny wrote: > From: David Decotigny <de...@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
signature.asc
Description: This is a digitally signed message part
