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

[PATCH, net-next, v5, 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_set_link_ksettings, which is used to retrieve
alterable link settings. 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.

---
v5:  - virtdev_validate_link_ksettings is taken out of the ethtool global structure
     and is instead added as an argument to ethtool_virtdev_set_link_ksettings
     as suggested by Jakub Kicinski.

v4:  - Cleaned up return statement in ethtool_virtdev_validate_cmd based off of
     Michal Kubecek's and Thomas Falcon's suggestion.

     - If the netvsc driver is using the VF device in order to get accelerated
     networking, the real speed and duplex is reported by using the VF device as
     suggested by Stephen Hemminger.

     - The speed and duplex variables are now passed by value rather than passed
     by pointer as suggested by Willem de Bruijin and Michal Kubecek.

     - Removed ethtool_virtdev_get_link_ksettings since it was too simple to
     warrant a helper function.

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):
  ethtool: Factored out similar ethtool link settings for virtual
    devices to core
  net/ethtool: Introduce link_ksettings API for virtual network devices

 drivers/net/ethernet/ibm/ibmveth.c | 57 ++++++++++++++++++++++----------------
 drivers/net/ethernet/ibm/ibmveth.h |  3 ++
 drivers/net/hyperv/netvsc_drv.c    | 25 +++++++++++------
 drivers/net/virtio_net.c           | 40 ++------------------------
 include/linux/ethtool.h            |  8 ++++++
 net/ethtool/ioctl.c                | 45 ++++++++++++++++++++++++++++++
 6 files changed, 108 insertions(+), 70 deletions(-)

-- 
1.8.3.1

[PATCH, net-next, v5, 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 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 |  8 ++++++++
 net/ethtool/ioctl.c     | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 95991e43..fbc38f0 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -420,4 +420,12 @@ struct ethtool_rx_flow_rule *
 ethtool_rx_flow_rule_create(const struct ethtool_rx_flow_spec_input *input);
 void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *rule);
 
+bool ethtool_virtdev_validate_cmd(const struct ethtool_link_ksettings *cmd);
+int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
+				       const struct ethtool_link_ksettings *cmd,
+				       u32 *dev_speed, u8 *dev_duplex,
+				       bool (*ethtool_virtdev_validate_cmd)
+				       (const struct ethtool_link_ksettings *));
+
+
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index b987052..173e083 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -459,6 +459,25 @@ 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 */
+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);
+}
+EXPORT_SYMBOL(ethtool_virtdev_validate_cmd);
+
 /* convert a kernel internal ethtool_link_ksettings to
  * ethtool_link_usettings in user space. return 0 on success, errno on
  * error.
@@ -581,6 +600,32 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
 	return err;
 }
 
+int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
+				       const struct ethtool_link_ksettings *cmd,
+				       u32 *dev_speed, u8 *dev_duplex,
+				       bool (*dev_virtdev_validate_cmd)
+				       (const struct ethtool_link_ksettings *))
+{
+	bool (*validate)(const struct ethtool_link_ksettings *);
+	u32 speed;
+	u8 duplex;
+
+	validate = dev_virtdev_validate_cmd ? dev_virtdev_validate_cmd :
+		ethtool_virtdev_validate_cmd;
+	speed = cmd->base.speed;
+	duplex = cmd->base.duplex;
+	/* don't allow custom speed and duplex */
+	if (!ethtool_validate_speed(speed) ||
+	    !ethtool_validate_duplex(duplex) ||
+	    !(*validate)(cmd))
+		return -EINVAL;
+	*dev_speed = speed;
+	*dev_duplex = duplex;
+
+	return 0;
+}
+EXPORT_SYMBOL(ethtool_virtdev_set_link_ksettings);
+
 /* Query device for its ethtool_cmd settings.
  *
  * Backward compatibility note: for compatibility with legacy ethtool, this is
-- 
1.8.3.1

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

[Cris Forno]

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

Funtionality changes that pertain to ibmveth driver include:

  1. Changed the initial hardcoded link speed to 1GB.

  2. Added support for allowing a user to change the reported link
  speed via ethtool.

Changes to the netvsc driver include:

  1. When netvsc_get_link_ksettings is called, it will defer to the VF
  device if it exists to pull accelerated networking values, otherwise
  pull default or user-defined values.

  2. Similarly, if netvsc_set_link_ksettings called and a VF device
  exists, the real values of speed and duplex are changed.

Signed-off-by: Cris Forno <cforno12@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ibmveth.c | 57 ++++++++++++++++++++++----------------
 drivers/net/ethernet/ibm/ibmveth.h |  3 ++
 drivers/net/hyperv/netvsc_drv.c    | 25 +++++++++++------
 drivers/net/virtio_net.c           | 40 ++------------------------
 4 files changed, 55 insertions(+), 70 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 84121aa..fe2405a 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -712,29 +712,36 @@ 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 ethtool_virtdev_set_link_ksettings(dev, cmd,
+						  &adapter->speed,
+						  &adapter->duplex, NULL);
+}
+
+static int ibmveth_get_link_ksettings(struct net_device *dev,
+				      struct ethtool_link_ksettings *cmd)
+{
+	struct ibmveth_adapter *adapter = netdev_priv(dev);
+
+	cmd->base.speed = adapter->speed;
+	cmd->base.duplex = adapter->duplex;
+	cmd->base.port = PORT_OTHER;
 
 	return 0;
 }
 
+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)
 {
@@ -965,12 +972,13 @@ 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		         = 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	         = ibmveth_get_link_ksettings,
+	.set_link_ksettings              = ibmveth_set_link_ksettings,
 };
 
 static int ibmveth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
@@ -1674,6 +1682,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 65e12cb..f733ec5 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1175,6 +1175,12 @@ 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;
+
+	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;
@@ -1187,18 +1193,19 @@ 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;
+	struct net_device *vf_netdev = rtnl_dereference(ndc->vf_netdev);
 
-	speed = cmd->base.speed;
-	if (!ethtool_validate_speed(speed) ||
-	    !ethtool_validate_duplex(cmd->base.duplex) ||
-	    !netvsc_validate_ethtool_ss_cmd(cmd))
-		return -EINVAL;
+	if (vf_netdev) {
+		if (!vf_netdev->ethtool_ops->set_link_ksettings)
+			return -EOPNOTSUPP;
 
-	ndc->speed = speed;
-	ndc->duplex = cmd->base.duplex;
+		return vf_netdev->ethtool_ops->set_link_ksettings(vf_netdev,
+								  cmd);
+	}
 
-	return 0;
+	return ethtool_virtdev_set_link_ksettings(dev, cmd,
+						  &ndc->speed, &ndc->duplex,
+						  &netvsc_validate_ethtool_ss_cmd);
 }
 
 static int netvsc_change_mtu(struct net_device *ndev, int mtu)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2fe7a31..48755c6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2166,48 +2166,14 @@ 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,
+						  NULL);
 }
 
 static int virtnet_get_link_ksettings(struct net_device *dev,
-- 
1.8.3.1

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

[Michal Kubecek]

On Tue, Feb 18, 2020 at 11:52:26AM -0600, Cris Forno wrote:
> Three virtual devices (ibmveth, virtio_net, and netvsc) all have
> similar code to 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 |  8 ++++++++
>  net/ethtool/ioctl.c     | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 95991e43..fbc38f0 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -420,4 +420,12 @@ struct ethtool_rx_flow_rule *
>  ethtool_rx_flow_rule_create(const struct ethtool_rx_flow_spec_input *input);
>  void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *rule);
>  
> +bool ethtool_virtdev_validate_cmd(const struct ethtool_link_ksettings *cmd);
> +int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
> +				       const struct ethtool_link_ksettings *cmd,
> +				       u32 *dev_speed, u8 *dev_duplex,
> +				       bool (*ethtool_virtdev_validate_cmd)
> +				       (const struct ethtool_link_ksettings *));

Using the same argument name as the default function used for it is
rather confusing.

Using a typedef for the type might make the declaration a bit easier to
read.

> +
> +
>  #endif /* _LINUX_ETHTOOL_H */
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index b987052..173e083 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -459,6 +459,25 @@ 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 */
> +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;

These could go into the initialization but I guess it's a matter of
taste.

> +	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);
> +}
> +EXPORT_SYMBOL(ethtool_virtdev_validate_cmd);
> +
>  /* convert a kernel internal ethtool_link_ksettings to
>   * ethtool_link_usettings in user space. return 0 on success, errno on
>   * error.
> @@ -581,6 +600,32 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
>  	return err;
>  }
>  
> +int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
> +				       const struct ethtool_link_ksettings *cmd,
> +				       u32 *dev_speed, u8 *dev_duplex,
> +				       bool (*dev_virtdev_validate_cmd)
> +				       (const struct ethtool_link_ksettings *))

The argument is named different than in the declaration which is also
a bit confusing.

> +{
> +	bool (*validate)(const struct ethtool_link_ksettings *);
> +	u32 speed;
> +	u8 duplex;
> +
> +	validate = dev_virtdev_validate_cmd ? dev_virtdev_validate_cmd :
> +		ethtool_virtdev_validate_cmd;

This can be shortened to

	validate = dev_virtdev_validate_cmd ?: ethtool_virtdev_validate_cmd;

The only thing I really don't like is the confusing naming of handler
argument (different between declaration and definition and one of them
conflicting with function used as default for that argument). The rest
is just nitpicking.

Michal Kubecek

> +	speed = cmd->base.speed;
> +	duplex = cmd->base.duplex;
> +	/* don't allow custom speed and duplex */
> +	if (!ethtool_validate_speed(speed) ||
> +	    !ethtool_validate_duplex(duplex) ||
> +	    !(*validate)(cmd))
> +		return -EINVAL;
> +	*dev_speed = speed;
> +	*dev_duplex = duplex;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ethtool_virtdev_set_link_ksettings);
> +
>  /* 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, v5, 2/2] net/ethtool: Introduce link_ksettings API for virtual network devices

[Michal Kubecek]

On Tue, Feb 18, 2020 at 11:52:27AM -0600, Cris Forno wrote:
> With get/set link settings functions in core/ethtool.c, ibmveth,
> netvsc, and virtio now use the core's helper function.
> 
> Funtionality changes that pertain to ibmveth driver include:
> 
>   1. Changed the initial hardcoded link speed to 1GB.
> 
>   2. Added support for allowing a user to change the reported link
>   speed via ethtool.
> 
> Changes to the netvsc driver include:
> 
>   1. When netvsc_get_link_ksettings is called, it will defer to the VF
>   device if it exists to pull accelerated networking values, otherwise
>   pull default or user-defined values.
> 
>   2. Similarly, if netvsc_set_link_ksettings called and a VF device
>   exists, the real values of speed and duplex are changed.
> 
> Signed-off-by: Cris Forno <cforno12@linux.vnet.ibm.com>
> ---
[...]
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 65e12cb..f733ec5 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
[...]
> @@ -1187,18 +1193,19 @@ 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;
> +	struct net_device *vf_netdev = rtnl_dereference(ndc->vf_netdev);
>  
> -	speed = cmd->base.speed;
> -	if (!ethtool_validate_speed(speed) ||
> -	    !ethtool_validate_duplex(cmd->base.duplex) ||
> -	    !netvsc_validate_ethtool_ss_cmd(cmd))
> -		return -EINVAL;
> +	if (vf_netdev) {
> +		if (!vf_netdev->ethtool_ops->set_link_ksettings)
> +			return -EOPNOTSUPP;
>  
> -	ndc->speed = speed;
> -	ndc->duplex = cmd->base.duplex;
> +		return vf_netdev->ethtool_ops->set_link_ksettings(vf_netdev,
> +								  cmd);
> +	}
>  
> -	return 0;
> +	return ethtool_virtdev_set_link_ksettings(dev, cmd,
> +						  &ndc->speed, &ndc->duplex,
> +						  &netvsc_validate_ethtool_ss_cmd);
>  }

I may be missing something obvious but I cannot see how does
netvsc_validate_ethtool_ss_cmd() differ from ethtool_virtdev_validate_cmd().
If it does, it would be probably worth a comment at the function.

Michal Kubecek

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

[Cris Forno]

These changes have been applied to v6. Thanks for the suggestions Michal!

Michal Kubecek <mkubecek@suse.cz> writes:

> On Tue, Feb 18, 2020 at 11:52:27AM -0600, Cris Forno wrote:
>> With get/set link settings functions in core/ethtool.c, ibmveth,
>> netvsc, and virtio now use the core's helper function.
>> 
>> Funtionality changes that pertain to ibmveth driver include:
>> 
>>   1. Changed the initial hardcoded link speed to 1GB.
>> 
>>   2. Added support for allowing a user to change the reported link
>>   speed via ethtool.
>> 
>> Changes to the netvsc driver include:
>> 
>>   1. When netvsc_get_link_ksettings is called, it will defer to the VF
>>   device if it exists to pull accelerated networking values, otherwise
>>   pull default or user-defined values.
>> 
>>   2. Similarly, if netvsc_set_link_ksettings called and a VF device
>>   exists, the real values of speed and duplex are changed.
>> 
>> Signed-off-by: Cris Forno <cforno12@linux.vnet.ibm.com>
>> ---
> [...]
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index 65e12cb..f733ec5 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
> [...]
>> @@ -1187,18 +1193,19 @@ 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;
>> +	struct net_device *vf_netdev = rtnl_dereference(ndc->vf_netdev);
>>  
>> -	speed = cmd->base.speed;
>> -	if (!ethtool_validate_speed(speed) ||
>> -	    !ethtool_validate_duplex(cmd->base.duplex) ||
>> -	    !netvsc_validate_ethtool_ss_cmd(cmd))
>> -		return -EINVAL;
>> +	if (vf_netdev) {
>> +		if (!vf_netdev->ethtool_ops->set_link_ksettings)
>> +			return -EOPNOTSUPP;
>>  
>> -	ndc->speed = speed;
>> -	ndc->duplex = cmd->base.duplex;
>> +		return vf_netdev->ethtool_ops->set_link_ksettings(vf_netdev,
>> +								  cmd);
>> +	}
>>  
>> -	return 0;
>> +	return ethtool_virtdev_set_link_ksettings(dev, cmd,
>> +						  &ndc->speed, &ndc->duplex,
>> +						  &netvsc_validate_ethtool_ss_cmd);
>>  }
>
> I may be missing something obvious but I cannot see how does
> netvsc_validate_ethtool_ss_cmd() differ from ethtool_virtdev_validate_cmd().
> If it does, it would be probably worth a comment at the function.
>
> Michal Kubecek