Add error handling in case of failure to send switching data path message
to the host.
Reported-by: Shachar Raindel <[email protected]>
Signed-off-by: Haiyang Zhang <[email protected]>
---
drivers/net/hyperv/hyperv_net.h | 6 +++++-
drivers/net/hyperv/netvsc.c | 35 +++++++++++++++++++++++++++++----
drivers/net/hyperv/netvsc_drv.c | 18 +++++++++++------
3 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 59ac04a610ad..442c520ab8f3 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -269,7 +269,7 @@ int rndis_filter_receive(struct net_device *ndev,
int rndis_filter_set_device_mac(struct netvsc_device *ndev,
const char *mac);
-void netvsc_switch_datapath(struct net_device *nv_dev, bool vf);
+int netvsc_switch_datapath(struct net_device *nv_dev, bool vf);
#define NVSP_INVALID_PROTOCOL_VERSION ((u32)0xFFFFFFFF)
@@ -1718,4 +1718,8 @@ struct rndis_message {
#define TRANSPORT_INFO_IPV6_TCP 0x10
#define TRANSPORT_INFO_IPV6_UDP 0x20
+#define RETRY_US_LO 5000
+#define RETRY_US_HI 10000
+#define RETRY_MAX 2000 /* >10 sec */
+
#endif /* _HYPERV_NET_H */
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 5bce24731502..9d07c9ce4be2 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -31,12 +31,13 @@
* Switch the data path from the synthetic interface to the VF
* interface.
*/
-void netvsc_switch_datapath(struct net_device *ndev, bool vf)
+int netvsc_switch_datapath(struct net_device *ndev, bool vf)
{
struct net_device_context *net_device_ctx = netdev_priv(ndev);
struct hv_device *dev = net_device_ctx->device_ctx;
struct netvsc_device *nv_dev = rtnl_dereference(net_device_ctx->nvdev);
struct nvsp_message *init_pkt = &nv_dev->channel_init_pkt;
+ int ret, retry = 0;
/* Block sending traffic to VF if it's about to be gone */
if (!vf)
@@ -51,15 +52,41 @@ void netvsc_switch_datapath(struct net_device *ndev, bool vf)
init_pkt->msg.v4_msg.active_dp.active_datapath =
NVSP_DATAPATH_SYNTHETIC;
+again:
trace_nvsp_send(ndev, init_pkt);
- vmbus_sendpacket(dev->channel, init_pkt,
+ ret = vmbus_sendpacket(dev->channel, init_pkt,
sizeof(struct nvsp_message),
- (unsigned long)init_pkt,
- VM_PKT_DATA_INBAND,
+ (unsigned long)init_pkt, VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+
+ /* If failed to switch to/from VF, let data_path_is_vf stay false,
+ * so we use synthetic path to send data.
+ */
+ if (ret) {
+ if (ret != -EAGAIN) {
+ netdev_err(ndev,
+ "Unable to send sw datapath msg, err: %d\n",
+ ret);
+ return ret;
+ }
+
+ if (retry++ < RETRY_MAX) {
+ usleep_range(RETRY_US_LO, RETRY_US_HI);
+ goto again;
+ } else {
+ netdev_err(
+ ndev,
+ "Retry failed to send sw datapath msg, err: %d\n",
+ ret);
+ return ret;
+ }
+ }
+
wait_for_completion(&nv_dev->channel_init_wait);
net_device_ctx->data_path_is_vf = vf;
+
+ return 0;
}
/* Worker to setup sub channels on initial setup
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 97b5c9b60503..7349a70af083 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -38,9 +38,6 @@
#include "hyperv_net.h"
#define RING_SIZE_MIN 64
-#define RETRY_US_LO 5000
-#define RETRY_US_HI 10000
-#define RETRY_MAX 2000 /* >10 sec */
#define LINKCHANGE_INT (2 * HZ)
#define VF_TAKEOVER_INT (HZ / 10)
@@ -2402,6 +2399,7 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event)
struct netvsc_device *netvsc_dev;
struct net_device *ndev;
bool vf_is_up = false;
+ int ret;
if (event != NETDEV_GOING_DOWN)
vf_is_up = netif_running(vf_netdev);
@@ -2418,9 +2416,17 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event)
if (net_device_ctx->data_path_is_vf == vf_is_up)
return NOTIFY_OK;
- netvsc_switch_datapath(ndev, vf_is_up);
- netdev_info(ndev, "Data path switched %s VF: %s\n",
- vf_is_up ? "to" : "from", vf_netdev->name);
+ ret = netvsc_switch_datapath(ndev, vf_is_up);
+
+ if (ret) {
+ netdev_err(ndev,
+ "Data path failed to switch %s VF: %s, err: %d\n",
+ vf_is_up ? "to" : "from", vf_netdev->name, ret);
+ return NOTIFY_DONE;
+ } else {
+ netdev_info(ndev, "Data path switched %s VF: %s\n",
+ vf_is_up ? "to" : "from", vf_netdev->name);
+ }
return NOTIFY_OK;
}
--
2.25.1
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Mon, 29 Mar 2021 16:21:35 -0700 you wrote:
> Add error handling in case of failure to send switching data path message
> to the host.
>
> Reported-by: Shachar Raindel <[email protected]>
> Signed-off-by: Haiyang Zhang <[email protected]>
>
>
> [...]
Here is the summary with links:
- [net-next] hv_netvsc: Add error handling while switching data path
https://git.kernel.org/netdev/net-next/c/d0922bf79817
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Haiyang Zhang <[email protected]> writes:
> Add error handling in case of failure to send switching data path message
> to the host.
>
> Reported-by: Shachar Raindel <[email protected]>
> Signed-off-by: Haiyang Zhang <[email protected]>
>
> ---
> drivers/net/hyperv/hyperv_net.h | 6 +++++-
> drivers/net/hyperv/netvsc.c | 35 +++++++++++++++++++++++++++++----
> drivers/net/hyperv/netvsc_drv.c | 18 +++++++++++------
> 3 files changed, 48 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index 59ac04a610ad..442c520ab8f3 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -269,7 +269,7 @@ int rndis_filter_receive(struct net_device *ndev,
> int rndis_filter_set_device_mac(struct netvsc_device *ndev,
> const char *mac);
>
> -void netvsc_switch_datapath(struct net_device *nv_dev, bool vf);
> +int netvsc_switch_datapath(struct net_device *nv_dev, bool vf);
>
> #define NVSP_INVALID_PROTOCOL_VERSION ((u32)0xFFFFFFFF)
>
> @@ -1718,4 +1718,8 @@ struct rndis_message {
> #define TRANSPORT_INFO_IPV6_TCP 0x10
> #define TRANSPORT_INFO_IPV6_UDP 0x20
>
> +#define RETRY_US_LO 5000
> +#define RETRY_US_HI 10000
> +#define RETRY_MAX 2000 /* >10 sec */
> +
> #endif /* _HYPERV_NET_H */
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 5bce24731502..9d07c9ce4be2 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -31,12 +31,13 @@
> * Switch the data path from the synthetic interface to the VF
> * interface.
> */
> -void netvsc_switch_datapath(struct net_device *ndev, bool vf)
> +int netvsc_switch_datapath(struct net_device *ndev, bool vf)
> {
> struct net_device_context *net_device_ctx = netdev_priv(ndev);
> struct hv_device *dev = net_device_ctx->device_ctx;
> struct netvsc_device *nv_dev = rtnl_dereference(net_device_ctx->nvdev);
> struct nvsp_message *init_pkt = &nv_dev->channel_init_pkt;
> + int ret, retry = 0;
>
> /* Block sending traffic to VF if it's about to be gone */
> if (!vf)
> @@ -51,15 +52,41 @@ void netvsc_switch_datapath(struct net_device *ndev, bool vf)
> init_pkt->msg.v4_msg.active_dp.active_datapath =
> NVSP_DATAPATH_SYNTHETIC;
>
> +again:
> trace_nvsp_send(ndev, init_pkt);
>
> - vmbus_sendpacket(dev->channel, init_pkt,
> + ret = vmbus_sendpacket(dev->channel, init_pkt,
> sizeof(struct nvsp_message),
> - (unsigned long)init_pkt,
> - VM_PKT_DATA_INBAND,
> + (unsigned long)init_pkt, VM_PKT_DATA_INBAND,
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +
> + /* If failed to switch to/from VF, let data_path_is_vf stay false,
> + * so we use synthetic path to send data.
> + */
> + if (ret) {
> + if (ret != -EAGAIN) {
> + netdev_err(ndev,
> + "Unable to send sw datapath msg, err: %d\n",
> + ret);
> + return ret;
> + }
> +
> + if (retry++ < RETRY_MAX) {
> + usleep_range(RETRY_US_LO, RETRY_US_HI);
> + goto again;
> + } else {
> + netdev_err(
> + ndev,
> + "Retry failed to send sw datapath msg, err: %d\n",
> + ret);
err is always -EAGAIN here, right?
> + return ret;
> + }
Nitpicking: I think we can simplify the above a bit:
if (ret) {
if (ret == -EAGAIN && retry++ < RETRY_MAX) {
usleep_range(RETRY_US_LO, RETRY_US_HI);
goto again;
}
netdev_err(ndev, "Unable to send sw datapath msg, err: %d\n", ret);
return ret;
}
> + }
> +
> wait_for_completion(&nv_dev->channel_init_wait);
> net_device_ctx->data_path_is_vf = vf;
> +
> + return 0;
> }
>
> /* Worker to setup sub channels on initial setup
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 97b5c9b60503..7349a70af083 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -38,9 +38,6 @@
> #include "hyperv_net.h"
>
> #define RING_SIZE_MIN 64
> -#define RETRY_US_LO 5000
> -#define RETRY_US_HI 10000
> -#define RETRY_MAX 2000 /* >10 sec */
>
> #define LINKCHANGE_INT (2 * HZ)
> #define VF_TAKEOVER_INT (HZ / 10)
> @@ -2402,6 +2399,7 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event)
> struct netvsc_device *netvsc_dev;
> struct net_device *ndev;
> bool vf_is_up = false;
> + int ret;
>
> if (event != NETDEV_GOING_DOWN)
> vf_is_up = netif_running(vf_netdev);
> @@ -2418,9 +2416,17 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event)
> if (net_device_ctx->data_path_is_vf == vf_is_up)
> return NOTIFY_OK;
>
> - netvsc_switch_datapath(ndev, vf_is_up);
> - netdev_info(ndev, "Data path switched %s VF: %s\n",
> - vf_is_up ? "to" : "from", vf_netdev->name);
> + ret = netvsc_switch_datapath(ndev, vf_is_up);
> +
> + if (ret) {
> + netdev_err(ndev,
> + "Data path failed to switch %s VF: %s, err: %d\n",
> + vf_is_up ? "to" : "from", vf_netdev->name, ret);
> + return NOTIFY_DONE;
> + } else {
> + netdev_info(ndev, "Data path switched %s VF: %s\n",
> + vf_is_up ? "to" : "from", vf_netdev->name);
> + }
>
> return NOTIFY_OK;
> }
--
Vitaly
> -----Original Message-----
> From: Vitaly Kuznetsov <[email protected]>
> Sent: Tuesday, March 30, 2021 7:43 AM
> To: Haiyang Zhang <[email protected]>; linux-
> [email protected]; [email protected]
> Cc: Haiyang Zhang <[email protected]>; KY Srinivasan
> <[email protected]>; Stephen Hemminger <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH net-next] hv_netvsc: Add error handling while switching
> data path
>
> Haiyang Zhang <[email protected]> writes:
>
> > Add error handling in case of failure to send switching data path message
> > to the host.
> >
> > Reported-by: Shachar Raindel <[email protected]>
> > Signed-off-by: Haiyang Zhang <[email protected]>
> >
> > ---
> > drivers/net/hyperv/hyperv_net.h | 6 +++++-
> > drivers/net/hyperv/netvsc.c | 35 +++++++++++++++++++++++++++++-
> ---
> > drivers/net/hyperv/netvsc_drv.c | 18 +++++++++++------
> > 3 files changed, 48 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/hyperv_net.h
> b/drivers/net/hyperv/hyperv_net.h
> > index 59ac04a610ad..442c520ab8f3 100644
> > --- a/drivers/net/hyperv/hyperv_net.h
> > +++ b/drivers/net/hyperv/hyperv_net.h
> > @@ -269,7 +269,7 @@ int rndis_filter_receive(struct net_device *ndev,
> > int rndis_filter_set_device_mac(struct netvsc_device *ndev,
> > const char *mac);
> >
> > -void netvsc_switch_datapath(struct net_device *nv_dev, bool vf);
> > +int netvsc_switch_datapath(struct net_device *nv_dev, bool vf);
> >
> > #define NVSP_INVALID_PROTOCOL_VERSION ((u32)0xFFFFFFFF)
> >
> > @@ -1718,4 +1718,8 @@ struct rndis_message {
> > #define TRANSPORT_INFO_IPV6_TCP 0x10
> > #define TRANSPORT_INFO_IPV6_UDP 0x20
> >
> > +#define RETRY_US_LO 5000
> > +#define RETRY_US_HI 10000
> > +#define RETRY_MAX 2000 /* >10 sec */
> > +
> > #endif /* _HYPERV_NET_H */
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index 5bce24731502..9d07c9ce4be2 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -31,12 +31,13 @@
> > * Switch the data path from the synthetic interface to the VF
> > * interface.
> > */
> > -void netvsc_switch_datapath(struct net_device *ndev, bool vf)
> > +int netvsc_switch_datapath(struct net_device *ndev, bool vf)
> > {
> > struct net_device_context *net_device_ctx = netdev_priv(ndev);
> > struct hv_device *dev = net_device_ctx->device_ctx;
> > struct netvsc_device *nv_dev = rtnl_dereference(net_device_ctx-
> >nvdev);
> > struct nvsp_message *init_pkt = &nv_dev->channel_init_pkt;
> > + int ret, retry = 0;
> >
> > /* Block sending traffic to VF if it's about to be gone */
> > if (!vf)
> > @@ -51,15 +52,41 @@ void netvsc_switch_datapath(struct net_device
> *ndev, bool vf)
> > init_pkt->msg.v4_msg.active_dp.active_datapath =
> > NVSP_DATAPATH_SYNTHETIC;
> >
> > +again:
> > trace_nvsp_send(ndev, init_pkt);
> >
> > - vmbus_sendpacket(dev->channel, init_pkt,
> > + ret = vmbus_sendpacket(dev->channel, init_pkt,
> > sizeof(struct nvsp_message),
> > - (unsigned long)init_pkt,
> > - VM_PKT_DATA_INBAND,
> > + (unsigned long)init_pkt, VM_PKT_DATA_INBAND,
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +
> > + /* If failed to switch to/from VF, let data_path_is_vf stay false,
> > + * so we use synthetic path to send data.
> > + */
> > + if (ret) {
> > + if (ret != -EAGAIN) {
> > + netdev_err(ndev,
> > + "Unable to send sw datapath msg,
> err: %d\n",
> > + ret);
> > + return ret;
> > + }
> > +
> > + if (retry++ < RETRY_MAX) {
> > + usleep_range(RETRY_US_LO, RETRY_US_HI);
> > + goto again;
> > + } else {
> > + netdev_err(
> > + ndev,
> > + "Retry failed to send sw datapath msg,
> err: %d\n",
> > + ret);
>
> err is always -EAGAIN here, right?
>
> > + return ret;
> > + }
>
> Nitpicking: I think we can simplify the above a bit:
>
> if (ret) {
> if (ret == -EAGAIN && retry++ < RETRY_MAX) {
> usleep_range(RETRY_US_LO, RETRY_US_HI);
> goto again;
> }
> netdev_err(ndev, "Unable to send sw datapath msg,
> err: %d\n", ret);
> return ret;
> }
>
Yes this looks cleaner.
Thanks,
Haiyang