[PATCH, net-next, v3, 0/2] net/ethtool: Introduce link_ksettings API for virtual network devices

[PATCH, net-next, v3, 0/2] net/ethtool: Introduce link_ksettings API for virtual network devices

[Cris Forno]

This series provides an API for drivers of virtual network devices that allows
users to alter initial device speed and duplex settings to reflect the actual
capabilities of underlying hardware. The changes made include two helper
functions ethtool_virtdev_get/set_link_ksettings, which are used to retrieve or
update alterable link settings, respectively. In addition, there is a new
ethtool operation defined to validate those settings provided by the user. This
operation can use either a generic validation function,
ethtool_virtdev_validate_cmd, or one defined by the driver. These changes
resolve code duplication for existing virtual network drivers that have already
implemented this behavior.  In the case of the ibmveth driver, this API is used
to provide this capability for the first time.

---
v3: Factored out duplicated code to core/ethtool to provide API to virtual
    drivers
    
v2: Updated default driver speed/duplex settings to avoid breaking existing
    setups
---

Cris Forno (2):
  net: Factored out similar ethtool link settings for virtual devices to
    core
  net: Enable virtual network devices to use ethtool's set/get link
    settings functions

 drivers/net/ethernet/ibm/ibmveth.c | 60 +++++++++++++++++++++-----------------
 drivers/net/ethernet/ibm/ibmveth.h |  3 ++
 drivers/net/hyperv/netvsc_drv.c    | 21 ++++---------
 drivers/net/virtio_net.c           | 45 ++++------------------------
 include/linux/ethtool.h            |  2 ++
 net/core/ethtool.c                 | 58 ++++++++++++++++++++++++++++++++++++
 6 files changed, 106 insertions(+), 83 deletions(-)

-- 
1.8.3.1

[PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

[Cris Forno]

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@linux.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)
+{
+	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)
+{
+	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

Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

[Andrew Lunn]

On Thu, Dec 19, 2019 at 02:54:09PM -0600, Cris Forno wrote:
> @@ -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)
> +{
> +	struct ethtool_link_ksettings diff1 = *cmd;
> +	struct ethtool_link_ksettings diff2 = {};

Hi Cris

These are not the best of names. How about request and valid?

> +
> +	/* 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)) &&

linkmode_equal()

> +		bitmap_empty(diff1.link_modes.supported,
> +			     __ETHTOOL_LINK_MODE_MASK_NBITS) &&

linkmode_empty()

	Andrew

Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

[Andrew Lunn]

On Fri, Dec 20, 2019 at 11:18:31AM +0100, Andrew Lunn wrote:
> On Thu, Dec 19, 2019 at 02:54:09PM -0600, Cris Forno wrote:
> > @@ -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)
> > +{
> > +	struct ethtool_link_ksettings diff1 = *cmd;
> > +	struct ethtool_link_ksettings diff2 = {};
> 
> Hi Cris
> 
> These are not the best of names. How about request and valid?
> 
> > +
> > +	/* 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)) &&
> 
> linkmode_equal()

Opps. That is wrong. base is not a link mode! Sorry for the noise.

      Andrew

Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

[Michal Kubecek]

On Fri, Dec 20, 2019 at 11:18:31AM +0100, Andrew Lunn wrote:
> On Thu, Dec 19, 2019 at 02:54:09PM -0600, Cris Forno wrote:
> > @@ -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)
> > +{
> > +	struct ethtool_link_ksettings diff1 = *cmd;
> > +	struct ethtool_link_ksettings diff2 = {};
> 
> Hi Cris
> 
> These are not the best of names. How about request and valid?
> 
> > +
> > +	/* 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)) &&
> 
> linkmode_equal()
> 
> > +		bitmap_empty(diff1.link_modes.supported,
> > +			     __ETHTOOL_LINK_MODE_MASK_NBITS) &&
> 
> linkmode_empty()
> 
> 	Andrew

Please note that this series was sent to netdev mailing list twice and
there is also some discussion at the other copy (both are marked "v3").
I didn't check if the two submissions are identical, though.

Michal

Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

[Willem de Bruijn]

On Thu, Dec 19, 2019 at 3:54 PM Cris Forno <cforno12@linux.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@linux.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
>

Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

[Cristobal Forno]

Thanks for your suggestions Willlem. I have a question on one of your 
suggestions for the ethtool_virtdev_get_link_ksettings function below.

On 22/12/2019 15:19, Willem de Bruijn wrote:
> On Thu, Dec 19, 2019 at 3:54 PM Cris Forno <cforno12@linux.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@linux.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.
Good point, I will incorporate this into the next version of the series.
>
>> +{
>> +       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.
Agreed.
>
> More fundamentally, these three assignments are simple enough that I
> don't think a helper actually simplifies anything here.

Although the function is simple, it does achieve the goal of this 
version of the patch series which is to eliminate duplication of code 
throughout the virtual devices. I think it's best to leave it like this, 
but I am open to more suggestions.

-Cris Forno

>
>
>
>> +{
>> +       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
>>

Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

[Willem de Bruijn]

On Tue, Jan 7, 2020 at 12:46 PM Cristobal Forno
<cforno12@linux.vnet.ibm.com> wrote:
>
> Thanks for your suggestions Willlem. I have a question on one of your
> suggestions for the ethtool_virtdev_get_link_ksettings function below.

> >> +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.
> Agreed.
> >
> > More fundamentally, these three assignments are simple enough that I
> > don't think a helper actually simplifies anything here.
>
> Although the function is simple, it does achieve the goal of this
> version of the patch series which is to eliminate duplication of code
> throughout the virtual devices. I think it's best to leave it like this,
> but I am open to more suggestions.

As said, I don't think three assignments warrant the effort of adding
a new callback.

Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

[Cristobal Forno]

Thanks for your suggestions Willlem. I have a question on one of your 
suggestions for the ethtool_virtdev_get_link_ksettings function below.

On 22/12/2019 15:19, Willem de Bruijn wrote:
> On Thu, Dec 19, 2019 at 3:54 PM Cris Forno <cforno12@linux.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@linux.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.
Good point, I will incorporate this into the next version of the series.
>> +{
>> +       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.
Agreed.
>
> More fundamentally, these three assignments are simple enough that I
> don't think a helper actually simplifies anything here.
>
Although the function is simple, it does achieve the goal of this 
version of the patch series which is to eliminate duplication of code 
throughout the virtual devices. I think it's best to leave it like this, 
but I am open to more suggestions.

-Cris Forno

>
>> +{
>> +       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
>>

[PATCH, net-next, v3, 2/2] net: Enable virtual network devices to use ethtool's set/get link settings functions

[Cris Forno]

With get/set link settings functions in core/ethtool.c, ibmveth,
netvsc, and virtio now use the core's helper function.

Signed-off-by: Cris Forno <cforno12@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ibmveth.c | 60 +++++++++++++++++++++-----------------
 drivers/net/ethernet/ibm/ibmveth.h |  3 ++
 drivers/net/hyperv/netvsc_drv.c    | 21 ++++---------
 drivers/net/virtio_net.c           | 45 ++++------------------------
 4 files changed, 46 insertions(+), 83 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index c5be4eb..6f9350ca5 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -712,31 +712,34 @@ static int ibmveth_close(struct net_device *netdev)
 	return 0;
 }
 
-static int netdev_get_link_ksettings(struct net_device *dev,
-				     struct ethtool_link_ksettings *cmd)
+static int ibmveth_set_link_ksettings(struct net_device *dev,
+				      const struct ethtool_link_ksettings *cmd)
 {
-	u32 supported, advertising;
-
-	supported = (SUPPORTED_1000baseT_Full | SUPPORTED_Autoneg |
-				SUPPORTED_FIBRE);
-	advertising = (ADVERTISED_1000baseT_Full | ADVERTISED_Autoneg |
-				ADVERTISED_FIBRE);
-	cmd->base.speed = SPEED_1000;
-	cmd->base.duplex = DUPLEX_FULL;
-	cmd->base.port = PORT_FIBRE;
-	cmd->base.phy_address = 0;
-	cmd->base.autoneg = AUTONEG_ENABLE;
-
-	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
-						supported);
-	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.advertising,
-						advertising);
+	struct ibmveth_adapter *adapter = netdev_priv(dev);
 
-	return 0;
+	return ethtool_virtdev_set_ksettings(dev, cmd,
+					     &adapter->speed, &adapter->duplex);
+}
+
+static int ibmveth_get_link_ksettings(struct net_device *dev,
+				      struct ethtool_link_ksettings *cmd)
+{
+	struct ibmveth_adapter *adapter = netdev_priv(dev);
+
+	return ethtool_virtdev_get_ksettings(dev, cmd,
+					     &adapter->speed, &adapter->duplex);
+}
+
+static void ibmveth_init_link_settings(struct net_device *dev)
+{
+	struct ibmveth_adapter *adapter = netdev_priv(dev);
+
+	adapter->speed = SPEED_1000;
+	adapter->duplex = DUPLEX_FULL;
 }
 
-static void netdev_get_drvinfo(struct net_device *dev,
-			       struct ethtool_drvinfo *info)
+static void ibmveth_get_drvinfo(struct net_device *dev,
+				struct ethtool_drvinfo *info)
 {
 	strlcpy(info->driver, ibmveth_driver_name, sizeof(info->driver));
 	strlcpy(info->version, ibmveth_driver_version, sizeof(info->version));
@@ -965,12 +968,14 @@ static void ibmveth_get_ethtool_stats(struct net_device *dev,
 }
 
 static const struct ethtool_ops netdev_ethtool_ops = {
-	.get_drvinfo		= netdev_get_drvinfo,
-	.get_link		= ethtool_op_get_link,
-	.get_strings		= ibmveth_get_strings,
-	.get_sset_count		= ibmveth_get_sset_count,
-	.get_ethtool_stats	= ibmveth_get_ethtool_stats,
-	.get_link_ksettings	= netdev_get_link_ksettings,
+	.get_drvinfo		         = ibmveth_get_drvinfo,
+	.get_link		         = ethtool_op_get_link,
+	.get_strings		         = ibmveth_get_strings,
+	.get_sset_count		         = ibmveth_get_sset_count,
+	.get_ethtool_stats	         = ibmveth_get_ethtool_stats,
+	.get_link_ksettings	         = ibmveth_get_link_ksettings,
+	.set_link_ksettings              = ibmveth_set_link_ksettings,
+	.virtdev_validate_link_ksettings = ethtool_virtdev_validate_cmd,
 };
 
 static int ibmveth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
@@ -1648,6 +1653,7 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	adapter->netdev = netdev;
 	adapter->mcastFilterSize = be32_to_cpu(*mcastFilterSize_p);
 	adapter->pool_config = 0;
+	ibmveth_init_link_settings(netdev);
 
 	netif_napi_add(netdev, &adapter->napi, ibmveth_poll, 16);
 
diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
index 4e9bf34..27dfff2 100644
--- a/drivers/net/ethernet/ibm/ibmveth.h
+++ b/drivers/net/ethernet/ibm/ibmveth.h
@@ -162,6 +162,9 @@ struct ibmveth_adapter {
     u64 tx_send_failed;
     u64 tx_large_packets;
     u64 rx_large_packets;
+    /* Ethtool settings */
+	u8 duplex;
+	u32 speed;
 };
 
 /*
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 5fa5c49..d0dfa8e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1084,29 +1084,17 @@ static int netvsc_get_link_ksettings(struct net_device *dev,
 {
 	struct net_device_context *ndc = netdev_priv(dev);
 
-	cmd->base.speed = ndc->speed;
-	cmd->base.duplex = ndc->duplex;
-	cmd->base.port = PORT_OTHER;
-
-	return 0;
+	return ethtool_virtdev_get_link_ksettings(dev, cmd,
+						  &ndc->speed, &ndc->duplex);
 }
 
 static int netvsc_set_link_ksettings(struct net_device *dev,
 				     const struct ethtool_link_ksettings *cmd)
 {
 	struct net_device_context *ndc = netdev_priv(dev);
-	u32 speed;
-
-	speed = cmd->base.speed;
-	if (!ethtool_validate_speed(speed) ||
-	    !ethtool_validate_duplex(cmd->base.duplex) ||
-	    !netvsc_validate_ethtool_ss_cmd(cmd))
-		return -EINVAL;
-
-	ndc->speed = speed;
-	ndc->duplex = cmd->base.duplex;
 
-	return 0;
+	return ethtool_virtdev_set_link_ksettings(dev, cmd,
+						  &ndc->speed, &ndc->duplex);
 }
 
 static int netvsc_change_mtu(struct net_device *ndev, int mtu)
@@ -1867,6 +1855,7 @@ static void netvsc_set_msglevel(struct net_device *ndev, u32 val)
 	.set_link_ksettings = netvsc_set_link_ksettings,
 	.get_ringparam	= netvsc_get_ringparam,
 	.set_ringparam	= netvsc_set_ringparam,
+	.virtdev_validate_link_ksettings = netvsc_validate_ethtool_ss_cmd,
 };
 
 static const struct net_device_ops device_ops = {
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5a635f0..5cbcb16 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2166,48 +2166,15 @@ static void virtnet_get_channels(struct net_device *dev,
 	channels->other_count = 0;
 }
 
-/* Check if the user is trying to change anything besides speed/duplex */
-static bool
-virtnet_validate_ethtool_cmd(const struct ethtool_link_ksettings *cmd)
-{
-	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);
 }
 
 static int virtnet_set_link_ksettings(struct net_device *dev,
 				      const struct ethtool_link_ksettings *cmd)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	u32 speed;
 
-	speed = cmd->base.speed;
-	/* don't allow custom speed and duplex */
-	if (!ethtool_validate_speed(speed) ||
-	    !ethtool_validate_duplex(cmd->base.duplex) ||
-	    !virtnet_validate_ethtool_cmd(cmd))
-		return -EINVAL;
-	vi->speed = speed;
-	vi->duplex = cmd->base.duplex;
-
-	return 0;
+	return ethtool_virtdev_set_link_ksettings(dev, cmd,
+						  &vi->speed, &vi->duplex);
 }
 
 static int virtnet_get_link_ksettings(struct net_device *dev,
@@ -2215,11 +2182,8 @@ static int virtnet_get_link_ksettings(struct net_device *dev,
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 
-	cmd->base.speed = vi->speed;
-	cmd->base.duplex = vi->duplex;
-	cmd->base.port = PORT_OTHER;
-
-	return 0;
+	return ethtool_virtdev_get_link_ksettings(dev, cmd,
+						  vi->speed, vi->duplex);
 }
 
 static int virtnet_set_coalesce(struct net_device *dev,
@@ -2309,6 +2273,7 @@ static void virtnet_update_settings(struct virtnet_info *vi)
 	.set_link_ksettings = virtnet_set_link_ksettings,
 	.set_coalesce = virtnet_set_coalesce,
 	.get_coalesce = virtnet_get_coalesce,
+	.virtdev_validate_link_ksettings = ethtool_virtdev_validate_cmd,
 };
 
 static void virtnet_freeze_down(struct virtio_device *vdev)
-- 
1.8.3.1

Re: [PATCH, net-next, v3, 2/2] net: Enable virtual network devices to use ethtool's set/get link settings functions

[Willem de Bruijn]

On Thu, Dec 19, 2019 at 3:54 PM Cris Forno <cforno12@linux.vnet.ibm.com> wrote:
>
> With get/set link settings functions in core/ethtool.c, ibmveth,
> netvsc, and virtio now use the core's helper function.
>
> Signed-off-by: Cris Forno <cforno12@linux.vnet.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmveth.c | 60 +++++++++++++++++++++-----------------
>  drivers/net/ethernet/ibm/ibmveth.h |  3 ++

There appears to be more going on here than simply replacing the local
version of functions with equivalent shared helpers.

Please briefly document in the commit message anything that is not a
just a noop.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 5a635f0..5cbcb16 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2166,48 +2166,15 @@ static void virtnet_get_channels(struct net_device *dev,
>         channels->other_count = 0;
>  }
>
> -/* Check if the user is trying to change anything besides speed/duplex */
> -static bool
> -virtnet_validate_ethtool_cmd(const struct ethtool_link_ksettings *cmd)
> -{
> -       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);
>  }

Stray parenthesis: build failure.