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

[PATCH, net-next, v6, 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.

---
v6:  - removed netvsc_validate_ethtool_ss_cmd(). netvsc_drv now uses
     ethtool_virtdev_validate_cmd() instead as suggested by Michal Kubecek
     and approved by Haiyang Zhang.

     - matched handler argument name of ethtool_virtdev_set_link_ksettings
     in declaration and definition as suggested by Michal Kubecek.

     - shortened validate variable assignment in
     ethtool_virtdev_set_link_ksettings as suggested by Michal Kubecek.

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    | 42 +++++++++-------------
 drivers/net/virtio_net.c           | 40 ++-------------------
 include/linux/ethtool.h            |  8 +++++
 net/ethtool/ioctl.c                | 44 +++++++++++++++++++++++
 6 files changed, 107 insertions(+), 87 deletions(-)

-- 
2.25.0

[PATCH, net-next, v6, 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     | 44 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 95991e4300bf..e12a29502e18 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 (*dev_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 b987052d91ef..af75a4c0f87c 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,31 @@ 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 ?: 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
-- 
2.25.0

[PATCH, net-next, v6, 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.

Functionality 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    | 42 +++++++++-------------
 drivers/net/virtio_net.c           | 40 ++-------------------
 4 files changed, 55 insertions(+), 87 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 84121aab7ff1..fe2405ad1c89 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 4e9bf3421f4f..27dfff200166 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 5ee282b20ecb..6c25bb439eae 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1139,23 +1139,6 @@ static int netvsc_set_channels(struct net_device *net,
 	return ret;
 }
 
-static bool
-netvsc_validate_ethtool_ss_cmd(const struct ethtool_link_ksettings *cmd)
-{
-	struct ethtool_link_ksettings diff1 = *cmd;
-	struct ethtool_link_ksettings diff2 = {};
-
-	diff1.base.speed = 0;
-	diff1.base.duplex = 0;
-	/* advertising and cmd are usually set */
-	ethtool_link_ksettings_zero_link_mode(&diff1, advertising);
-	diff1.base.cmd = 0;
-	/* We set port to PORT_OTHER */
-	diff2.base.port = PORT_OTHER;
-
-	return !memcmp(&diff1, &diff2, sizeof(diff1));
-}
-
 static void netvsc_init_settings(struct net_device *dev)
 {
 	struct net_device_context *ndc = netdev_priv(dev);
@@ -1172,6 +1155,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;
@@ -1184,18 +1173,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,
+						  NULL);
 }
 
 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 2fe7a3188282..48755c655428 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,
-- 
2.25.0

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

[Michal Kubecek]

On Tue, Feb 25, 2020 at 03:41:10PM -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>
> ---
[...]
> +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 ?: 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);

I didn't realize it when I asked about netvsc_validate_ethtool_ss_cmd() 
while reviewing v5 but after you got rid of it, all three callers of
ethtool_virtdev_set_link_ksettings() call it with NULL as validator,
i.e. use the default ethtool_virtdev_validate_cmd().

This brings a question if we really need the possibility to provide
a custom validator function. Do you think we should expect some driver
needing a custom validator function soon? If not, we should probably use
the default validation unconditionally for now and only add the option
to provide custom validator function when (if) there is use for it.

Michal

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

[Cris Forno]

Michal Kubecek <mkubecek@suse.cz> writes:

> On Tue, Feb 25, 2020 at 03:41:10PM -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>
>> ---
> [...]
>> +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 ?: 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);
>
> I didn't realize it when I asked about netvsc_validate_ethtool_ss_cmd() 
> while reviewing v5 but after you got rid of it, all three callers of
> ethtool_virtdev_set_link_ksettings() call it with NULL as validator,
> i.e. use the default ethtool_virtdev_validate_cmd().
>
> This brings a question if we really need the possibility to provide
> a custom validator function. Do you think we should expect some driver
> needing a custom validator function soon? If not, we should probably use
> the default validation unconditionally for now and only add the option
> to provide custom validator function when (if) there is use for it.
>
> Michal
I agree, we do not need the custom validator function pointer parameter
in the set_link_ksettings function since it is currently not needed. The
parameter will be removed in v7. Thanks again Michal!

--Cris Forno