[PATCH net] hv_netvsc: preserve hw_features on mtu/channels/ringparam changes

[PATCH net] hv_netvsc: preserve hw_features on mtu/channels/ringparam changes

[Vitaly Kuznetsov]

rndis_filter_device_add() is called both from netvsc_probe() when we
initially create the device and from set channels/mtu/ringparam
routines where we basically remove the device and add it back.

hw_features is reset in rndis_filter_device_add() and filled with
host data. However, we lose all additional flags which are set outside
of the driver, e.g. register_netdevice() adds NETIF_F_SOFT_FEATURES and
many others.

Fill hw_features in only when we're called from netvsc_probe() and
hw_features is empty.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/net/hyperv/rndis_filter.c | 127 +++++++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 57 deletions(-)

diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 065b204d8e17..e9c12b66bf08 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1128,69 +1128,20 @@ void rndis_set_subchannel(struct work_struct *w)
 	rtnl_unlock();
 }
 
-struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
-				      struct netvsc_device_info *device_info)
+static int rndis_netdev_set_hwcaps(struct netvsc_device *nvdev,
+				   struct rndis_device *rndis_device)
 {
-	struct net_device *net = hv_get_drvdata(dev);
+	struct net_device *net = rndis_device->ndev;
 	struct net_device_context *net_device_ctx = netdev_priv(net);
-	struct netvsc_device *net_device;
-	struct rndis_device *rndis_device;
 	struct ndis_offload hwcaps;
 	struct ndis_offload_params offloads;
-	struct ndis_recv_scale_cap rsscap;
-	u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
 	unsigned int gso_max_size = GSO_MAX_SIZE;
-	u32 mtu, size;
-	const struct cpumask *node_cpu_mask;
-	u32 num_possible_rss_qs;
-	int i, ret;
-
-	rndis_device = get_rndis_device();
-	if (!rndis_device)
-		return ERR_PTR(-ENODEV);
-
-	/*
-	 * Let the inner driver handle this first to create the netvsc channel
-	 * NOTE! Once the channel is created, we may get a receive callback
-	 * (RndisFilterOnReceive()) before this call is completed
-	 */
-	net_device = netvsc_device_add(dev, device_info);
-	if (IS_ERR(net_device)) {
-		kfree(rndis_device);
-		return net_device;
-	}
-
-	/* Initialize the rndis device */
-	net_device->max_chn = 1;
-	net_device->num_chn = 1;
-
-	net_device->extension = rndis_device;
-	rndis_device->ndev = net;
-
-	/* Send the rndis initialization message */
-	ret = rndis_filter_init_device(rndis_device, net_device);
-	if (ret != 0)
-		goto err_dev_remv;
-
-	/* Get the MTU from the host */
-	size = sizeof(u32);
-	ret = rndis_filter_query_device(rndis_device, net_device,
-					RNDIS_OID_GEN_MAXIMUM_FRAME_SIZE,
-					&mtu, &size);
-	if (ret == 0 && size == sizeof(u32) && mtu < net->mtu)
-		net->mtu = mtu;
-
-	/* Get the mac address */
-	ret = rndis_filter_query_device_mac(rndis_device, net_device);
-	if (ret != 0)
-		goto err_dev_remv;
-
-	memcpy(device_info->mac_adr, rndis_device->hw_mac_adr, ETH_ALEN);
+	int ret;
 
 	/* Find HW offload capabilities */
-	ret = rndis_query_hwcaps(rndis_device, net_device, &hwcaps);
+	ret = rndis_query_hwcaps(rndis_device, nvdev, &hwcaps);
 	if (ret != 0)
-		goto err_dev_remv;
+		return ret;
 
 	/* A value of zero means "no change"; now turn on what we want. */
 	memset(&offloads, 0, sizeof(struct ndis_offload_params));
@@ -1245,10 +1196,72 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
 
 	netif_set_gso_max_size(net, gso_max_size);
 
-	ret = rndis_filter_set_offload_params(net, net_device, &offloads);
-	if (ret)
+	ret = rndis_filter_set_offload_params(net, nvdev, &offloads);
+
+	return ret;
+}
+
+struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
+				      struct netvsc_device_info *device_info)
+{
+	struct net_device *net = hv_get_drvdata(dev);
+	struct netvsc_device *net_device;
+	struct rndis_device *rndis_device;
+	struct ndis_recv_scale_cap rsscap;
+	u32 rsscap_size = sizeof(struct ndis_recv_scale_cap);
+	u32 mtu, size;
+	const struct cpumask *node_cpu_mask;
+	u32 num_possible_rss_qs;
+	int i, ret;
+
+	rndis_device = get_rndis_device();
+	if (!rndis_device)
+		return ERR_PTR(-ENODEV);
+
+	/* Let the inner driver handle this first to create the netvsc channel
+	 * NOTE! Once the channel is created, we may get a receive callback
+	 * (RndisFilterOnReceive()) before this call is completed
+	 */
+	net_device = netvsc_device_add(dev, device_info);
+	if (IS_ERR(net_device)) {
+		kfree(rndis_device);
+		return net_device;
+	}
+
+	/* Initialize the rndis device */
+	net_device->max_chn = 1;
+	net_device->num_chn = 1;
+
+	net_device->extension = rndis_device;
+	rndis_device->ndev = net;
+
+	/* Send the rndis initialization message */
+	ret = rndis_filter_init_device(rndis_device, net_device);
+	if (ret != 0)
+		goto err_dev_remv;
+
+	/* Get the MTU from the host */
+	size = sizeof(u32);
+	ret = rndis_filter_query_device(rndis_device, net_device,
+					RNDIS_OID_GEN_MAXIMUM_FRAME_SIZE,
+					&mtu, &size);
+	if (ret == 0 && size == sizeof(u32) && mtu < net->mtu)
+		net->mtu = mtu;
+
+	/* Get the mac address */
+	ret = rndis_filter_query_device_mac(rndis_device, net_device);
+	if (ret != 0)
 		goto err_dev_remv;
 
+	memcpy(device_info->mac_adr, rndis_device->hw_mac_adr, ETH_ALEN);
+
+	/* Query hardware capabilities if we're called from netvsc_probe() */
+	if (!net->hw_features) {
+		ret = rndis_netdev_set_hwcaps(net_device, rndis_device);
+		if (ret != 0)
+			goto err_dev_remv;
+	}
+
 	rndis_filter_query_device_link_status(rndis_device, net_device);
 
 	netdev_dbg(net, "Device MAC %pM link state %s\n",
-- 
2.13.6

Re: [PATCH net] hv_netvsc: preserve hw_features on mtu/channels/ringparam changes

[Stephen Hemminger]

On Tue, 14 Nov 2017 16:22:05 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

Yes, this looks like a real issue.



> +	/* Query hardware capabilities if we're called from netvsc_probe() */
> +	if (!net->hw_features) {
> +		ret = rndis_netdev_set_hwcaps(net_device, rndis_device);
> +		if (ret != 0)
> +			goto err_dev_remv;
> +	}
> +

Rather than conditional behavior in rndis_filter_device_add, it would be cleaner
to make the call to get hardware capabilities there.

Please respin and make the query of host a separate function.

Re: [PATCH net] hv_netvsc: preserve hw_features on mtu/channels/ringparam changes

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Tue, 14 Nov 2017 16:22:05 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Yes, this looks like a real issue.
>
>> +	/* Query hardware capabilities if we're called from netvsc_probe() */
>> +	if (!net->hw_features) {
>> +		ret = rndis_netdev_set_hwcaps(net_device, rndis_device);
>> +		if (ret != 0)
>> +			goto err_dev_remv;
>> +	}
>> +
>
> Rather than conditional behavior in rndis_filter_device_add, it would be cleaner
> to make the call to get hardware capabilities there.
>
> Please respin and make the query of host a separate function.

You mean call rndis_netdev_set_hwcaps() from netvsc_probe()? Will do.

One question though: in case we'll be avoiding
rndis_filter_set_offload_params() call on mtu/channels/ringparam
changes -- can we trust the host to preserve what was there before the
RNDIS reset? In case not we'll have to untangle what is
rndis_netdev_set_hwcaps() in my patch splitting it into two: hw_features
setup and rndis_filter_set_offload_params() and leaving the later in
rndis_filter_device_add().

Thanks,

-- 
  Vitaly

RE: [PATCH net] hv_netvsc: preserve hw_features on mtu/channels/ringparam changes

[Haiyang Zhang]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, November 14, 2017 11:58 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; KY Srinivasan <kys@microsoft.com>; Haiyang
> Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Mohammed Gamal <mmorsy@redhat.com>
> Subject: Re: [PATCH net] hv_netvsc: preserve hw_features on
> mtu/channels/ringparam changes
> 
> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > On Tue, 14 Nov 2017 16:22:05 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >
> > Yes, this looks like a real issue.
> >
> >> +	/* Query hardware capabilities if we're called from netvsc_probe() */
> >> +	if (!net->hw_features) {
> >> +		ret = rndis_netdev_set_hwcaps(net_device, rndis_device);
> >> +		if (ret != 0)
> >> +			goto err_dev_remv;
> >> +	}
> >> +
> >
> > Rather than conditional behavior in rndis_filter_device_add, it would
> > be cleaner to make the call to get hardware capabilities there.
> >
> > Please respin and make the query of host a separate function.
> 
> You mean call rndis_netdev_set_hwcaps() from netvsc_probe()? Will do.
> 
> One question though: in case we'll be avoiding
> rndis_filter_set_offload_params() call on mtu/channels/ringparam changes -
> - can we trust the host to preserve what was there before the RNDIS reset?
> In case not we'll have to untangle what is
> rndis_netdev_set_hwcaps() in my patch splitting it into two: hw_features
> setup and rndis_filter_set_offload_params() and leaving the later in
> rndis_filter_device_add().

After remove/re-add RNDIS dev, you should pass the parameters to the host
again.

Thanks,
- Haiyang

Re: [PATCH net] hv_netvsc: preserve hw_features on mtu/channels/ringparam changes

[Vitaly Kuznetsov]

Haiyang Zhang <haiyangz@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Tuesday, November 14, 2017 11:58 AM
>> To: Stephen Hemminger <stephen@networkplumber.org>
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devel@linuxdriverproject.org; KY Srinivasan <kys@microsoft.com>; Haiyang
>> Zhang <haiyangz@microsoft.com>; Stephen Hemminger
>> <sthemmin@microsoft.com>; Mohammed Gamal <mmorsy@redhat.com>
>> Subject: Re: [PATCH net] hv_netvsc: preserve hw_features on
>> mtu/channels/ringparam changes
>> 
>> Stephen Hemminger <stephen@networkplumber.org> writes:
>> 
>> > On Tue, 14 Nov 2017 16:22:05 +0100
>> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> >
>> > Yes, this looks like a real issue.
>> >
>> >> +	/* Query hardware capabilities if we're called from netvsc_probe() */
>> >> +	if (!net->hw_features) {
>> >> +		ret = rndis_netdev_set_hwcaps(net_device, rndis_device);
>> >> +		if (ret != 0)
>> >> +			goto err_dev_remv;
>> >> +	}
>> >> +
>> >
>> > Rather than conditional behavior in rndis_filter_device_add, it would
>> > be cleaner to make the call to get hardware capabilities there.
>> >
>> > Please respin and make the query of host a separate function.
>> 
>> You mean call rndis_netdev_set_hwcaps() from netvsc_probe()? Will do.
>> 
>> One question though: in case we'll be avoiding
>> rndis_filter_set_offload_params() call on mtu/channels/ringparam changes -
>> - can we trust the host to preserve what was there before the RNDIS reset?
>> In case not we'll have to untangle what is
>> rndis_netdev_set_hwcaps() in my patch splitting it into two: hw_features
>> setup and rndis_filter_set_offload_params() and leaving the later in
>> rndis_filter_device_add().
>
> After remove/re-add RNDIS dev, you should pass the parameters to the host
> again.
>

Thanks, this changes a lot. I'll prepare v2.

-- 
  Vitaly