Date:   Sun, 22 Dec 2019 16:19:56 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Cris Forno <cforno12@...ux.vnet.ibm.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>, haiyangz@...rosoft.com,
        sthemmin@...rosoft.com, sashal@...nel.org, tlfalcon@...ux.ibm.com
Subject: Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool
 link settings for virtual devices to core

On Thu, Dec 19, 2019 at 3:54 PM Cris Forno <cforno12@...ux.vnet.ibm.com> wrote:
>
> Three virtual devices (ibmveth, virtio_net, and netvsc) all have
> similar code to set/get link settings and validate ethtool command. To
> eliminate duplication of code, it is factored out into core/ethtool.c.
>
> Signed-off-by: Cris Forno <cforno12@...ux.vnet.ibm.com>
> ---
>  include/linux/ethtool.h |  2 ++
>  net/core/ethtool.c      | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 95991e43..1b0417b 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -394,6 +394,8 @@ struct ethtool_ops {
>                                           struct ethtool_coalesce *);
>         int     (*set_per_queue_coalesce)(struct net_device *, u32,
>                                           struct ethtool_coalesce *);
> +       bool    (*virtdev_validate_link_ksettings)(const struct
> +                                                  ethtool_link_ksettings *);
>         int     (*get_link_ksettings)(struct net_device *,
>                                       struct ethtool_link_ksettings *);
>         int     (*set_link_ksettings)(struct net_device *,
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index cd9bc67..4091a94 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -579,6 +579,32 @@ static int load_link_ksettings_from_user(struct ethtool_link_ksettings *to,
>         return 0;
>  }
>
> +/* Check if the user is trying to change anything besides speed/duplex */
> +static bool
> +ethtool_virtdev_validate_cmd(const struct ethtool_link_ksettings *cmd)

If called from other modules like drivers/net/virtio_net.ko, these
functions cannot be static, need a declaration in
include/linux/ethtool.h and an EXPORT_SYMBOL_GPL.

Also return type should be on the same line.

> +{
> +       struct ethtool_link_ksettings diff1 = *cmd;
> +       struct ethtool_link_ksettings diff2 = {};
> +
> +       /* cmd is always set so we need to clear it, validate the port type
> +        * and also without autonegotiation we can ignore advertising
> +        */
> +       diff1.base.speed = 0;
> +       diff2.base.port = PORT_OTHER;
> +       ethtool_link_ksettings_zero_link_mode(&diff1, advertising);
> +       diff1.base.duplex = 0;
> +       diff1.base.cmd = 0;
> +       diff1.base.link_mode_masks_nwords = 0;
> +
> +       return !memcmp(&diff1.base, &diff2.base, sizeof(diff1.base)) &&
> +               bitmap_empty(diff1.link_modes.supported,
> +                            __ETHTOOL_LINK_MODE_MASK_NBITS) &&
> +               bitmap_empty(diff1.link_modes.advertising,
> +                            __ETHTOOL_LINK_MODE_MASK_NBITS) &&
> +               bitmap_empty(diff1.link_modes.lp_advertising,
> +                            __ETHTOOL_LINK_MODE_MASK_NBITS);
> +}
> +
>  /* convert a kernel internal ethtool_link_ksettings to
>   * ethtool_link_usettings in user space. return 0 on success, errno on
>   * error.
> @@ -660,6 +686,17 @@ static int ethtool_get_link_ksettings(struct net_device *dev,
>         return store_link_ksettings_for_user(useraddr, &link_ksettings);
>  }
>
> +static int
> +ethtool_virtdev_get_link_ksettings(struct net_device *dev,
> +                                  struct ethtool_link_ksettings *cmd,
> +                                  u32 *speed, u8 *duplex)

No need to pass by reference, really. Indeed, the virtio_net caller
passes vi->speed and vi->duplex instead of &vi->speed and &vi->duplex.

More fundamentally, these three assignments are simple enough that I
don't think a helper actually simplifies anything here.



> +{
> +       cmd->base.speed = *speed;
> +       cmd->base.duplex = *duplex;
> +       cmd->base.port = PORT_OTHER;
> +       return 0;
> +}
> +
>  /* Update device ethtool_link_settings. */
>  static int ethtool_set_link_ksettings(struct net_device *dev,
>                                       void __user *useraddr)
> @@ -696,6 +733,27 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
>         return dev->ethtool_ops->set_link_ksettings(dev, &link_ksettings);
>  }
>
> +static int
> +ethtool_virtdev_set_link_ksettings(struct net_device *dev,
> +                                  const struct ethtool_link_ksettings *cmd,
> +                                  u32 *dev_speed, u8 *dev_duplex)
> +{
> +       u32 speed;
> +       u8 duplex;
> +
> +       speed = cmd->base.speed;
> +       duplex = cmd->base.duplex;
> +       /* don't allow custom speed and duplex */
> +       if (!ethtool_validate_speed(speed) ||
> +           !ethtool_validate_duplex(duplex) ||
> +           !dev->ethtool_ops->virtdev_validate_link_ksettings(cmd))
> +               return -EINVAL;
> +       *dev_speed = speed;
> +       *dev_duplex = duplex;
> +
> +       return 0;
> +}
> +
>  /* Query device for its ethtool_cmd settings.
>   *
>   * Backward compatibility note: for compatibility with legacy ethtool, this is
> --
> 1.8.3.1
>

Powered by blists - more mailing lists