| Message ID | 1572296801-4789-3-git-send-email-haiyangz@microsoft.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | hv_netvsc: Add XDP support and some error handling fixes | expand |
> If rndis_filter_open() fails, we need to remove the rndis device created > in earlier steps, before returning an error code. Otherwise, the retry of > netvsc_attach() from its callers will fail and hang. How do you think about to choose a more “imperative mood” for your change description? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=0dbe6cb8f7e05bc9611602ef45980a6c57b245a3#n151 … > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev, > if (netif_running(ndev)) { > ret = rndis_filter_open(nvdev); > if (ret) > - return ret; > + goto err; > > rdev = nvdev->extension; > if (!rdev->link_state) … I would prefer to specify the completed exception handling (addition of two function calls) by a compound statement in the shown if branch directly. If you would insist to use a goto statement, I find an other label more appropriate according to the Linux coding style. Regards, Markus
> -----Original Message----- > From: Markus Elfring <Markus.Elfring@web.de> > Sent: Friday, November 1, 2019 4:43 PM > To: Haiyang Zhang <haiyangz@microsoft.com>; linux- > hyperv@vger.kernel.org; netdev@vger.kernel.org > Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org; David S. > Miller <davem@davemloft.net>; KY Srinivasan <kys@microsoft.com>; Olaf > Hering <olaf@aepfle.de>; Sasha Levin <sashal@kernel.org>; Stephen > Hemminger <sthemmin@microsoft.com>; vkuznets <vkuznets@redhat.com> > Subject: Re: [PATCH net-next, 2/4] hv_netvsc: Fix error handling in > netvsc_attach() > > > If rndis_filter_open() fails, we need to remove the rndis device > > created in earlier steps, before returning an error code. Otherwise, > > the retry of > > netvsc_attach() from its callers will fail and hang. > > How do you think about to choose a more “imperative mood” for your > change description? > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k > ernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git% > 2Ftree%2FDocumentation%2Fprocess%2Fsubmitting- > patches.rst%3Fid%3D0dbe6cb8f7e05bc9611602ef45980a6c57b245a3%23n151 > &data=02%7C01%7Chaiyangz%40microsoft.com%7C162aa016f45e4293c > abb08d75f0c0fee%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637 > 082377796295159&sdata=ytjxGYTPI2D4BoNbslKPvBbsfGUEM7hXj1YAiG > hn8Ik%3D&reserved=0 Agreed. Thanks. > > > … > > +++ b/drivers/net/hyperv/netvsc_drv.c > > @@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev, > > if (netif_running(ndev)) { > > ret = rndis_filter_open(nvdev); > > if (ret) > > - return ret; > > + goto err; > > > > rdev = nvdev->extension; > > if (!rdev->link_state) > … > > I would prefer to specify the completed exception handling (addition of two > function calls) by a compound statement in the shown if branch directly. > > If you would insist to use a goto statement, I find an other label more > appropriate according to the Linux coding style. I will have more patches that make multiple entry points of error clean up steps -- so I used goto instead of putting the functions in each if-branch. I will name the labels more meaningfully. Thanks, - Haiyang
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 734e411..a14fc8e 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev, if (netif_running(ndev)) { ret = rndis_filter_open(nvdev); if (ret) - return ret; + goto err; rdev = nvdev->extension; if (!rdev->link_state) @@ -990,6 +990,13 @@ static int netvsc_attach(struct net_device *ndev, } return 0; + +err: + netif_device_detach(ndev); + + rndis_filter_device_remove(hdev, nvdev); + + return ret; } static int netvsc_set_channels(struct net_device *net,
If rndis_filter_open() fails, we need to remove the rndis device created in earlier steps, before returning an error code. Otherwise, the retry of netvsc_attach() from its callers will fail and hang. Fixes: 7b2ee50c0cd5 ("hv_netvsc: common detach logic") Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> --- drivers/net/hyperv/netvsc_drv.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)