| Message ID | 20200530140224.GA1330098@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [v2] IB/hfi1: Fix hfi1_netdev_rx_init() error handling | expand |
On Sat, May 30, 2020 at 05:02:24PM +0300, Dan Carpenter wrote: > The hfi1_vnic_up() function doesn't check whether hfi1_netdev_rx_init() > returns errors. In hfi1_vnic_init() we need to change the code to > preserve the error code instead of returning success. > > Fixes: 2280740f01ae ("IB/hfi1: Virtual Network Interface Controller (VNIC) HW support") > Fixes: 4730f4a6c6b2 ("IB/hfi1: Activate the dummy netdev") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: Add error handling in hfi1_vnic_up() and add second fixes tag > > drivers/infiniband/hw/hfi1/vnic_main.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c > index b183c56b7b6a4..03f8be8e9488e 100644 > --- a/drivers/infiniband/hw/hfi1/vnic_main.c > +++ b/drivers/infiniband/hw/hfi1/vnic_main.c > @@ -457,13 +457,19 @@ static int hfi1_vnic_up(struct hfi1_vnic_vport_info *vinfo) > if (rc < 0) > return rc; > > - hfi1_netdev_rx_init(dd); > + rc = hfi1_netdev_rx_init(dd); > + if (rc < 0) > + goto err_remove; Why did you check for the negative value here and didn't check below? Thanks > > netif_carrier_on(netdev); > netif_tx_start_all_queues(netdev); > set_bit(HFI1_VNIC_UP, &vinfo->flags); > > return 0; > + > +err_remove: > + hfi1_netdev_remove_data(dd, VNIC_ID(vinfo->vesw_id)); > + return rc; > } > > static void hfi1_vnic_down(struct hfi1_vnic_vport_info *vinfo) > @@ -512,7 +518,8 @@ static int hfi1_vnic_init(struct hfi1_vnic_vport_info *vinfo) > goto txreq_fail; > } > > - if (hfi1_netdev_rx_init(dd)) { > + rc = hfi1_netdev_rx_init(dd); > + if (rc) { > dd_dev_err(dd, "Unable to initialize netdev contexts\n"); > goto alloc_fail; > } > -- > 2.26.2 >
On Sun, May 31, 2020 at 01:05:12PM +0300, Leon Romanovsky wrote: > On Sat, May 30, 2020 at 05:02:24PM +0300, Dan Carpenter wrote: > > The hfi1_vnic_up() function doesn't check whether hfi1_netdev_rx_init() > > returns errors. In hfi1_vnic_init() we need to change the code to > > preserve the error code instead of returning success. > > > > Fixes: 2280740f01ae ("IB/hfi1: Virtual Network Interface Controller (VNIC) HW support") > > Fixes: 4730f4a6c6b2 ("IB/hfi1: Activate the dummy netdev") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > v2: Add error handling in hfi1_vnic_up() and add second fixes tag > > > > drivers/infiniband/hw/hfi1/vnic_main.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c > > index b183c56b7b6a4..03f8be8e9488e 100644 > > --- a/drivers/infiniband/hw/hfi1/vnic_main.c > > +++ b/drivers/infiniband/hw/hfi1/vnic_main.c > > @@ -457,13 +457,19 @@ static int hfi1_vnic_up(struct hfi1_vnic_vport_info *vinfo) > > if (rc < 0) > > return rc; > > > > - hfi1_netdev_rx_init(dd); > > + rc = hfi1_netdev_rx_init(dd); > > + if (rc < 0) > > + goto err_remove; > > Why did you check for the negative value here and didn't check below? > I just copied the pattern in the nearest code. I didn't realize until now that it was different in both functions... The checking isn't done consistently in this file. I can resend on Tuesday though if you want. regards, dan carpenter
On Sun, May 31, 2020 at 08:36:55PM +0300, Dan Carpenter wrote: > On Sun, May 31, 2020 at 01:05:12PM +0300, Leon Romanovsky wrote: > > On Sat, May 30, 2020 at 05:02:24PM +0300, Dan Carpenter wrote: > > > The hfi1_vnic_up() function doesn't check whether hfi1_netdev_rx_init() > > > returns errors. In hfi1_vnic_init() we need to change the code to > > > preserve the error code instead of returning success. > > > > > > Fixes: 2280740f01ae ("IB/hfi1: Virtual Network Interface Controller (VNIC) HW support") > > > Fixes: 4730f4a6c6b2 ("IB/hfi1: Activate the dummy netdev") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > v2: Add error handling in hfi1_vnic_up() and add second fixes tag > > > > > > drivers/infiniband/hw/hfi1/vnic_main.c | 11 +++++++++-- > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c > > > index b183c56b7b6a4..03f8be8e9488e 100644 > > > --- a/drivers/infiniband/hw/hfi1/vnic_main.c > > > +++ b/drivers/infiniband/hw/hfi1/vnic_main.c > > > @@ -457,13 +457,19 @@ static int hfi1_vnic_up(struct hfi1_vnic_vport_info *vinfo) > > > if (rc < 0) > > > return rc; > > > > > > - hfi1_netdev_rx_init(dd); > > > + rc = hfi1_netdev_rx_init(dd); > > > + if (rc < 0) > > > + goto err_remove; > > > > Why did you check for the negative value here and didn't check below? > > > > I just copied the pattern in the nearest code. I didn't realize until > now that it was different in both functions... The checking isn't done > consistently in this file. > > I can resend on Tuesday though if you want. I imagine that Jason will fix it once he will apply the patch. Thanks > > regards, > dan carpenter >
On Mon, Jun 01, 2020 at 07:24:33AM +0300, Leon Romanovsky wrote: > On Sun, May 31, 2020 at 08:36:55PM +0300, Dan Carpenter wrote: > > On Sun, May 31, 2020 at 01:05:12PM +0300, Leon Romanovsky wrote: > > > On Sat, May 30, 2020 at 05:02:24PM +0300, Dan Carpenter wrote: > > > > The hfi1_vnic_up() function doesn't check whether hfi1_netdev_rx_init() > > > > returns errors. In hfi1_vnic_init() we need to change the code to > > > > preserve the error code instead of returning success. > > > > > > > > Fixes: 2280740f01ae ("IB/hfi1: Virtual Network Interface Controller (VNIC) HW support") > > > > Fixes: 4730f4a6c6b2 ("IB/hfi1: Activate the dummy netdev") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > v2: Add error handling in hfi1_vnic_up() and add second fixes tag > > > > > > > > drivers/infiniband/hw/hfi1/vnic_main.c | 11 +++++++++-- > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c > > > > index b183c56b7b6a4..03f8be8e9488e 100644 > > > > +++ b/drivers/infiniband/hw/hfi1/vnic_main.c > > > > @@ -457,13 +457,19 @@ static int hfi1_vnic_up(struct hfi1_vnic_vport_info *vinfo) > > > > if (rc < 0) > > > > return rc; > > > > > > > > - hfi1_netdev_rx_init(dd); > > > > + rc = hfi1_netdev_rx_init(dd); > > > > + if (rc < 0) > > > > + goto err_remove; > > > > > > Why did you check for the negative value here and didn't check below? > > > > > > > I just copied the pattern in the nearest code. I didn't realize until > > now that it was different in both functions... The checking isn't done > > consistently in this file. > > > > I can resend on Tuesday though if you want. > > I imagine that Jason will fix it once he will apply the patch. If someone from hfi says which is the right one, sure.. Jason
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > owner@vger.kernel.org> On Behalf Of Jason Gunthorpe > Sent: Monday, June 01, 2020 8:27 AM > To: Leon Romanovsky <leon@kernel.org> > Cc: Dan Carpenter <dan.carpenter@oracle.com>; Marciniszyn, Mike > <mike.marciniszyn@intel.com>; Andrejczuk, Grzegorz > <grzegorz.andrejczuk@intel.com>; Dalessandro, Dennis > <dennis.dalessandro@intel.com>; Doug Ledford <dledford@redhat.com>; > linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org > Subject: Re: [PATCH v2] IB/hfi1: Fix hfi1_netdev_rx_init() error handling > > On Mon, Jun 01, 2020 at 07:24:33AM +0300, Leon Romanovsky wrote: > > On Sun, May 31, 2020 at 08:36:55PM +0300, Dan Carpenter wrote: > > > On Sun, May 31, 2020 at 01:05:12PM +0300, Leon Romanovsky wrote: > > > > On Sat, May 30, 2020 at 05:02:24PM +0300, Dan Carpenter wrote: > > > > > The hfi1_vnic_up() function doesn't check whether > > > > > hfi1_netdev_rx_init() returns errors. In hfi1_vnic_init() we > > > > > need to change the code to preserve the error code instead of > returning success. > > > > > > > > > > Fixes: 2280740f01ae ("IB/hfi1: Virtual Network Interface > > > > > Controller (VNIC) HW support") > > > > > Fixes: 4730f4a6c6b2 ("IB/hfi1: Activate the dummy netdev") > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > v2: Add error handling in hfi1_vnic_up() and add second fixes > > > > > tag > > > > > > > > > > drivers/infiniband/hw/hfi1/vnic_main.c | 11 +++++++++-- > > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c > > > > > b/drivers/infiniband/hw/hfi1/vnic_main.c > > > > > index b183c56b7b6a4..03f8be8e9488e 100644 > > > > > +++ b/drivers/infiniband/hw/hfi1/vnic_main.c > > > > > @@ -457,13 +457,19 @@ static int hfi1_vnic_up(struct > hfi1_vnic_vport_info *vinfo) > > > > > if (rc < 0) > > > > > return rc; > > > > > > > > > > - hfi1_netdev_rx_init(dd); > > > > > + rc = hfi1_netdev_rx_init(dd); > > > > > + if (rc < 0) Please use: if (rc) Thanks, Kaike > > > > > + goto err_remove; > > > > > > > > Why did you check for the negative value here and didn't check below? > > > > > > > > > > I just copied the pattern in the nearest code. I didn't realize > > > until now that it was different in both functions... The checking > > > isn't done consistently in this file. > > > > > > I can resend on Tuesday though if you want. > > > > I imagine that Jason will fix it once he will apply the patch. > > If someone from hfi says which is the right one, sure.. > > Jason
On Sat, May 30, 2020 at 05:02:24PM +0300, Dan Carpenter wrote: > The hfi1_vnic_up() function doesn't check whether hfi1_netdev_rx_init() > returns errors. In hfi1_vnic_init() we need to change the code to > preserve the error code instead of returning success. > > Fixes: 2280740f01ae ("IB/hfi1: Virtual Network Interface Controller (VNIC) HW support") > Fixes: 4730f4a6c6b2 ("IB/hfi1: Activate the dummy netdev") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: Add error handling in hfi1_vnic_up() and add second fixes tag > > drivers/infiniband/hw/hfi1/vnic_main.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) Applied to for-next with the 'if (rc)' fixup, thanks Jason
On Mon, Jun 01, 2020 at 11:14:50AM -0300, Jason Gunthorpe wrote: > On Sat, May 30, 2020 at 05:02:24PM +0300, Dan Carpenter wrote: > > The hfi1_vnic_up() function doesn't check whether hfi1_netdev_rx_init() > > returns errors. In hfi1_vnic_init() we need to change the code to > > preserve the error code instead of returning success. > > > > Fixes: 2280740f01ae ("IB/hfi1: Virtual Network Interface Controller (VNIC) HW support") > > Fixes: 4730f4a6c6b2 ("IB/hfi1: Activate the dummy netdev") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > v2: Add error handling in hfi1_vnic_up() and add second fixes tag > > > > drivers/infiniband/hw/hfi1/vnic_main.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > Applied to for-next with the 'if (rc)' fixup, thanks Thanks. I would have resent it, but it's a three day weekend here... regards, dan carpenter
diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c index b183c56b7b6a4..03f8be8e9488e 100644 --- a/drivers/infiniband/hw/hfi1/vnic_main.c +++ b/drivers/infiniband/hw/hfi1/vnic_main.c @@ -457,13 +457,19 @@ static int hfi1_vnic_up(struct hfi1_vnic_vport_info *vinfo) if (rc < 0) return rc; - hfi1_netdev_rx_init(dd); + rc = hfi1_netdev_rx_init(dd); + if (rc < 0) + goto err_remove; netif_carrier_on(netdev); netif_tx_start_all_queues(netdev); set_bit(HFI1_VNIC_UP, &vinfo->flags); return 0; + +err_remove: + hfi1_netdev_remove_data(dd, VNIC_ID(vinfo->vesw_id)); + return rc; } static void hfi1_vnic_down(struct hfi1_vnic_vport_info *vinfo) @@ -512,7 +518,8 @@ static int hfi1_vnic_init(struct hfi1_vnic_vport_info *vinfo) goto txreq_fail; } - if (hfi1_netdev_rx_init(dd)) { + rc = hfi1_netdev_rx_init(dd); + if (rc) { dd_dev_err(dd, "Unable to initialize netdev contexts\n"); goto alloc_fail; }
The hfi1_vnic_up() function doesn't check whether hfi1_netdev_rx_init() returns errors. In hfi1_vnic_init() we need to change the code to preserve the error code instead of returning success. Fixes: 2280740f01ae ("IB/hfi1: Virtual Network Interface Controller (VNIC) HW support") Fixes: 4730f4a6c6b2 ("IB/hfi1: Activate the dummy netdev") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: Add error handling in hfi1_vnic_up() and add second fixes tag drivers/infiniband/hw/hfi1/vnic_main.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)