[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):
  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.
  With get/set link settings functions in core/ethtool.c, ibmveth,
    netvsc, and virtio now use the core's helper function.

 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                 | 56 +++++++++++++++++++++++++++++++++++
 6 files changed, 104 insertions(+), 83 deletions(-)

--
1.8.3.1

[PATCH, net-next, v3, 1/2] 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.

[Cris Forno]

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

[PATCH, net-next, v3, 2/2] With get/set link settings functions in core/ethtool.c, ibmveth, netvsc, and virtio now use the core's helper function.

[Cris Forno]

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, 0/2] net/ethtool: Introduce link_ksettings API for virtual network devices

[Stephen Hemminger]

On Thu, 19 Dec 2019 13:40:55 -0600
Cris Forno <cforno12@linux.vnet.ibm.com> wrote:

> 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):
>   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.
>   With get/set link settings functions in core/ethtool.c, ibmveth,
>     netvsc, and virtio now use the core's helper function.
> 
>  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                 | 56 +++++++++++++++++++++++++++++++++++
>  6 files changed, 104 insertions(+), 83 deletions(-)
> 
> --
> 1.8.3.1

I don't think this makes sense for netvsc. The speed and duplex have no
meaning, why do you want to allow overriding it? If this is to try and make
some dashboard look good; then you aren't seeing the real speed which is
what only the host knows. Plus it does take into account the accelerated
networking path.

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

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Thu, 19 Dec 2019 13:11:56 -0800

> I don't think this makes sense for netvsc. The speed and duplex have no
> meaning, why do you want to allow overriding it? If this is to try and make
> some dashboard look good; then you aren't seeing the real speed which is
> what only the host knows. Plus it does take into account the accelerated
> networking path.

Maybe that's the point, userspace has extraneous knowledge it might
use to set it accurately.

This helps for bonding/team etc. as well.

I don't think there is any real harm in allowing to set this, and
we've done this in the past I think.

Re: [PATCH, net-next, v3, 1/2] 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.

[Michal Kubecek]

On Thu, Dec 19, 2019 at 01:40:56PM -0600, Cris Forno wrote:
> 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

You should probably rebase on top of current net-next; this file has
been moved to net/ethtool/ioctl.c recently.

> @@ -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) &&

Isn't this condition always true? You zeroed the advertising bitmap
above. Could you just omit this part and clearing of advertising above?

> +		bitmap_empty(diff1.link_modes.lp_advertising,
> +			     __ETHTOOL_LINK_MODE_MASK_NBITS);
> +}

Another idea: instead of zeroing parts of diff1, you could copy these
members from *cmd to diff2 and compare cmd->base with diff2.base. You
could then drop diff1. And you wouldn't even need whole struct
ethtool_link_ksettings for diff2 as you only compare embedded struct
ethtool_link_settings (and check two bitmaps in cmd->link_modes).

> +
>  /* 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;
> +}

speed and duplex can be passed by value here; if you prefer pointers,
please make them const.

Michal Kubecek

Re: [PATCH, net-next, v3, 1/2] 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.

[Thomas Falcon]

On 12/19/19 4:36 PM, Michal Kubecek wrote:
> On Thu, Dec 19, 2019 at 01:40:56PM -0600, Cris Forno wrote:
>> Signed-off-by: Cris Forno <cforno12@linux.vnet.ibm.com>

Hi Michal, thanks for your comments.  I have a question on your 
suggestions for the ethtool_virtdev_validate_cmd below.

>> ---
>>   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
> You should probably rebase on top of current net-next; this file has
> been moved to net/ethtool/ioctl.c recently.
>
>> @@ -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) &&
> Isn't this condition always true? You zeroed the advertising bitmap
> above. Could you just omit this part and clearing of advertising above?
>
>> +		bitmap_empty(diff1.link_modes.lp_advertising,
>> +			     __ETHTOOL_LINK_MODE_MASK_NBITS);
>> +}
> Another idea: instead of zeroing parts of diff1, you could copy these
> members from *cmd to diff2 and compare cmd->base with diff2.base. You
> could then drop diff1. And you wouldn't even need whole struct
> ethtool_link_ksettings for diff2 as you only compare embedded struct
> ethtool_link_settings (and check two bitmaps in cmd->link_modes).

If I understand your suggestion correctly, then the validate function 
might look something like this?

/* 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_settings base2 = {};

     base2.speed = cmd->base.speed;
     base2.port = PORT_OTHER;
     base2.duplex = cmd->base.duplex;
     base2.cmd = cmd->base.cmd;
     base2.link_mode_masks_nwords = cmd->base.link_mode_masks_nwords;

     return !memcmp(&base2, cmd->base, sizeof(base2)) &&
         bitmap_empty(cmd->link_modes.supported,
                  __ETHTOOL_LINK_MODE_MASK_NBITS) &&
         bitmap_empty(cmd->link_modes.lp_advertising,
                  __ETHTOOL_LINK_MODE_MASK_NBITS);
}

Thanks again,

Tom

>
>> +
>>   /* 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;
>> +}
> speed and duplex can be passed by value here; if you prefer pointers,
> please make them const.
>
> Michal Kubecek

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

[Stephen Hemminger]

On Thu, 19 Dec 2019 14:16:19 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 19 Dec 2019 13:11:56 -0800
> 
> > I don't think this makes sense for netvsc. The speed and duplex have no
> > meaning, why do you want to allow overriding it? If this is to try and make
> > some dashboard look good; then you aren't seeing the real speed which is
> > what only the host knows. Plus it does take into account the accelerated
> > networking path.  
> 
> Maybe that's the point, userspace has extraneous knowledge it might
> use to set it accurately.
> 
> This helps for bonding/team etc. as well.
> 
> I don't think there is any real harm in allowing to set this, and
> we've done this in the past I think.

My preference would be to have host report some real data.
But that might take host side changes which have a long lead time to
get done.

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

[Stephen Hemminger]

On Thu, 19 Dec 2019 14:16:19 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 19 Dec 2019 13:11:56 -0800
> 
> > I don't think this makes sense for netvsc. The speed and duplex have no
> > meaning, why do you want to allow overriding it? If this is to try and make
> > some dashboard look good; then you aren't seeing the real speed which is
> > what only the host knows. Plus it does take into account the accelerated
> > networking path.  
> 
> Maybe that's the point, userspace has extraneous knowledge it might
> use to set it accurately.
> 
> This helps for bonding/team etc. as well.
> 
> I don't think there is any real harm in allowing to set this, and
> we've done this in the past I think.

The most widely used case with netvsc is using VF to get accelerated networking
in that case real speed and duplex value is reported by the VF device.

Maybe something like the following (COMPLETELY UNTESTED):


diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index eff8fef4f775..111847ca7e8c 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1083,11 +1083,14 @@ static int netvsc_get_link_ksettings(struct net_device *dev,
 				     struct ethtool_link_ksettings *cmd)
 {
 	struct net_device_context *ndc = netdev_priv(dev);
+	struct net_device *vf_netdev = rtnl_dereference(ndc->vf_netdev);
+
+	if (vf_netdev)
+		return __ethtool_get_link_ksettings(vf_netdev, cmd);
 
 	cmd->base.speed = ndc->speed;
 	cmd->base.duplex = ndc->duplex;
 	cmd->base.port = PORT_OTHER;
-
 	return 0;
 }
 
@@ -1095,8 +1098,17 @@ static int netvsc_set_link_ksettings(struct net_device *dev,
 				     const struct ethtool_link_ksettings *cmd)
 {
 	struct net_device_context *ndc = netdev_priv(dev);
+	struct net_device *vf_netdev = rtnl_dereference(ndc->vf_netdev);
 	u32 speed;
 
+	if (vf_netdev) {
+		if (!vf_netdev->ethtool_ops->set_link_ksettings)
+			return -EOPNOTSUPP;
+
+		return vf_netdev->ethtool_ops->set_link_ksettings(vf_netdev,
+								  cmd);
+	}
+
 	speed = cmd->base.speed;
 	if (!ethtool_validate_speed(speed) ||
 	    !ethtool_validate_duplex(cmd->base.duplex) ||
-- 
2.20.1

Re: [PATCH, net-next, v3, 1/2] 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.

[Michal Kubecek]

On Thu, Dec 19, 2019 at 07:26:14PM -0600, Thomas Falcon wrote:
> On 12/19/19 4:36 PM, Michal Kubecek wrote:
> > On Thu, Dec 19, 2019 at 01:40:56PM -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 = {};
> > > +
> > > +	/* 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) &&
> > Isn't this condition always true? You zeroed the advertising bitmap
> > above. Could you just omit this part and clearing of advertising above?
> > 
> > > +		bitmap_empty(diff1.link_modes.lp_advertising,
> > > +			     __ETHTOOL_LINK_MODE_MASK_NBITS);
> > > +}
> > Another idea: instead of zeroing parts of diff1, you could copy these
> > members from *cmd to diff2 and compare cmd->base with diff2.base. You
> > could then drop diff1. And you wouldn't even need whole struct
> > ethtool_link_ksettings for diff2 as you only compare embedded struct
> > ethtool_link_settings (and check two bitmaps in cmd->link_modes).
> 
> If I understand your suggestion correctly, then the validate function might
> look something like this?
> 
> /* 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_settings base2 = {};
> 
>     base2.speed = cmd->base.speed;
>     base2.port = PORT_OTHER;
>     base2.duplex = cmd->base.duplex;
>     base2.cmd = cmd->base.cmd;
>     base2.link_mode_masks_nwords = cmd->base.link_mode_masks_nwords;
> 
>     return !memcmp(&base2, cmd->base, sizeof(base2)) &&
>         bitmap_empty(cmd->link_modes.supported,
>                  __ETHTOOL_LINK_MODE_MASK_NBITS) &&
>         bitmap_empty(cmd->link_modes.lp_advertising,
>                  __ETHTOOL_LINK_MODE_MASK_NBITS);
> }

Yes, that is what I wanted to suggest (the second argument of memcmp()
should be "&cmd->base", I think).

Michal

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

[Stephen Hemminger]

On Thu, 19 Dec 2019 14:16:19 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 19 Dec 2019 13:11:56 -0800
> 
> > I don't think this makes sense for netvsc. The speed and duplex have no
> > meaning, why do you want to allow overriding it? If this is to try and make
> > some dashboard look good; then you aren't seeing the real speed which is
> > what only the host knows. Plus it does take into account the accelerated
> > networking path.  
> 
> Maybe that's the point, userspace has extraneous knowledge it might
> use to set it accurately.
> 
> This helps for bonding/team etc. as well.
> 
> I don't think there is any real harm in allowing to set this, and
> we've done this in the past I think.

Looked a little more. The netvsc driver does query the host already
and get a correct link speed.  See drivers/net/hyperv/rndis_filter.c

If running on Windows Server 2019 you will see speed report of 40G
if using 40G Mellanox SRIOV, and 10G if using a private virtual network.

On Azure, the guest sees the speed of the underlying network connection
which varies based on machine type.

Bottom line: it behaves like real hardware now, why should we allow user
to set it for netvsc and not other real hardware.

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

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Thu, 19 Dec 2019 19:29:09 -0800

> On Thu, 19 Dec 2019 14:16:19 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
> 
>> From: Stephen Hemminger <stephen@networkplumber.org>
>> Date: Thu, 19 Dec 2019 13:11:56 -0800
>> 
>> > I don't think this makes sense for netvsc. The speed and duplex have no
>> > meaning, why do you want to allow overriding it? If this is to try and make
>> > some dashboard look good; then you aren't seeing the real speed which is
>> > what only the host knows. Plus it does take into account the accelerated
>> > networking path.  
>> 
>> Maybe that's the point, userspace has extraneous knowledge it might
>> use to set it accurately.
>> 
>> This helps for bonding/team etc. as well.
>> 
>> I don't think there is any real harm in allowing to set this, and
>> we've done this in the past I think.
> 
> My preference would be to have host report some real data.
> But that might take host side changes which have a long lead time to
> get done.

_Iff_ they ever get done.

Meanwhile... we should provide some way to address this in the short
term.

Re: [PATCH, net-next, v3, 1/2] 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.

[Cristobal Forno]

Thanks for your suggestion Micheal and Thomas. This will be included in 
the next version of the patch series.

-Cris Forno

On 20/12/2019 01:04, Michal Kubecek wrote:
> On Thu, Dec 19, 2019 at 07:26:14PM -0600, Thomas Falcon wrote:
>> On 12/19/19 4:36 PM, Michal Kubecek wrote:
>>> On Thu, Dec 19, 2019 at 01:40:56PM -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 = {};
>>>> +
>>>> +	/* 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) &&
>>> Isn't this condition always true? You zeroed the advertising bitmap
>>> above. Could you just omit this part and clearing of advertising above?
>>>
>>>> +		bitmap_empty(diff1.link_modes.lp_advertising,
>>>> +			     __ETHTOOL_LINK_MODE_MASK_NBITS);
>>>> +}
>>> Another idea: instead of zeroing parts of diff1, you could copy these
>>> members from *cmd to diff2 and compare cmd->base with diff2.base. You
>>> could then drop diff1. And you wouldn't even need whole struct
>>> ethtool_link_ksettings for diff2 as you only compare embedded struct
>>> ethtool_link_settings (and check two bitmaps in cmd->link_modes).
>> If I understand your suggestion correctly, then the validate function might
>> look something like this?
>>
>> /* 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_settings base2 = {};
>>
>>      base2.speed = cmd->base.speed;
>>      base2.port = PORT_OTHER;
>>      base2.duplex = cmd->base.duplex;
>>      base2.cmd = cmd->base.cmd;
>>      base2.link_mode_masks_nwords = cmd->base.link_mode_masks_nwords;
>>
>>      return !memcmp(&base2, cmd->base, sizeof(base2)) &&
>>          bitmap_empty(cmd->link_modes.supported,
>>                   __ETHTOOL_LINK_MODE_MASK_NBITS) &&
>>          bitmap_empty(cmd->link_modes.lp_advertising,
>>                   __ETHTOOL_LINK_MODE_MASK_NBITS);
>> }
> Yes, that is what I wanted to suggest (the second argument of memcmp()
> should be "&cmd->base", I think).
>
> Michal