[for-next,v3,1/4] RDMA/core: Fix return code when modify_port isn't supported
diff mbox series

Message ID 20191018094115.13167-2-kamalheib1@gmail.com
State Superseded
Headers show
Series
  • RDMA: modify_port improvements
Related show

Commit Message

Kamal Heib Oct. 18, 2019, 9:41 a.m. UTC
Improving return code from ib_modify_port() by doing the following:
1- Use "-EOPNOTSUPP" instead "-ENOSYS" which is the proper return code.
2- Avoid confusion by return "-EOPNOTSUPP" when modify_port() isn't
   supplied by the provider and the protocol is IB, otherwise return
   success to avoid failure of the ib_modify_port() in CM layer.

Fixes: 61e0962d5221 ("IB: Avoid ib_modify_port() failure for RoCE devices")
Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
 drivers/infiniband/core/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michal Kalderon Oct. 18, 2019, 8:01 p.m. UTC | #1
> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Kamal Heib
> 
> Improving return code from ib_modify_port() by doing the following:
> 1- Use "-EOPNOTSUPP" instead "-ENOSYS" which is the proper return code.
> 2- Avoid confusion by return "-EOPNOTSUPP" when modify_port() isn't
>    supplied by the provider and the protocol is IB, otherwise return
>    success to avoid failure of the ib_modify_port() in CM layer.
> 
> Fixes: 61e0962d5221 ("IB: Avoid ib_modify_port() failure for RoCE devices")
> Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> ---
>  drivers/infiniband/core/device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/device.c
> b/drivers/infiniband/core/device.c
> index a667636f74bf..626ac18dd3a7 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2397,7 +2397,7 @@ int ib_modify_port(struct ib_device *device,
>  					     port_modify_mask,
>  					     port_modify);
>  	else
> -		rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
> +		rc = rdma_protocol_ib(device, port_num) ? -EOPNOTSUPP :
> 0;
>  	return rc;
>  }
>  EXPORT_SYMBOL(ib_modify_port);
> --
> 2.20.1

Thanks, 

Reviewed-by: Michal Kalderon <michal.kalderon@marvell.com>
Jason Gunthorpe Oct. 22, 2019, 7:20 p.m. UTC | #2
On Fri, Oct 18, 2019 at 12:41:12PM +0300, Kamal Heib wrote:
> Improving return code from ib_modify_port() by doing the following:
> 1- Use "-EOPNOTSUPP" instead "-ENOSYS" which is the proper return code.
> 2- Avoid confusion by return "-EOPNOTSUPP" when modify_port() isn't
>    supplied by the provider and the protocol is IB, otherwise return
>    success to avoid failure of the ib_modify_port() in CM layer.
> 
> Fixes: 61e0962d5221 ("IB: Avoid ib_modify_port() failure for RoCE devices")
> Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
>  drivers/infiniband/core/device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index a667636f74bf..626ac18dd3a7 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -2397,7 +2397,7 @@ int ib_modify_port(struct ib_device *device,
>  					     port_modify_mask,
>  					     port_modify);
>  	else
> -		rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
> +		rc = rdma_protocol_ib(device, port_num) ? -EOPNOTSUPP : 0;
>  	return rc;

Oh gross, this is such an ugly hack

roce mode should allow a fake IB_PORT_CM_SUP to be manipulated and
nothing else.

All other cases should return EOPNOTSUPP as something has gone really
wrong

Jason
Kamal Heib Oct. 22, 2019, 8:21 p.m. UTC | #3
On Tue, Oct 22, 2019 at 04:20:02PM -0300, Jason Gunthorpe wrote:
> On Fri, Oct 18, 2019 at 12:41:12PM +0300, Kamal Heib wrote:
> > Improving return code from ib_modify_port() by doing the following:
> > 1- Use "-EOPNOTSUPP" instead "-ENOSYS" which is the proper return code.
> > 2- Avoid confusion by return "-EOPNOTSUPP" when modify_port() isn't
> >    supplied by the provider and the protocol is IB, otherwise return
> >    success to avoid failure of the ib_modify_port() in CM layer.
> > 
> > Fixes: 61e0962d5221 ("IB: Avoid ib_modify_port() failure for RoCE devices")
> > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> >  drivers/infiniband/core/device.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > index a667636f74bf..626ac18dd3a7 100644
> > +++ b/drivers/infiniband/core/device.c
> > @@ -2397,7 +2397,7 @@ int ib_modify_port(struct ib_device *device,
> >  					     port_modify_mask,
> >  					     port_modify);
> >  	else
> > -		rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
> > +		rc = rdma_protocol_ib(device, port_num) ? -EOPNOTSUPP : 0;
> >  	return rc;
> 
> Oh gross, this is such an ugly hack
> 
> roce mode should allow a fake IB_PORT_CM_SUP to be manipulated and
> nothing else.
> 
> All other cases should return EOPNOTSUPP as something has gone really
> wrong
> 
> Jason

Do you mean something like the following:

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index a667636f74bf..1db97f6f5dec 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2396,8 +2396,12 @@ int ib_modify_port(struct ib_device *device,
                rc = device->ops.modify_port(device, port_num,
                                             port_modify_mask,
                                             port_modify);
+       else if (rdma_protocol_roce(device, port_num) &&
+                ((port_modify->set_port_cap_mask & IB_PORT_CM_SUP) ||
+                (port_modify->clr_port_cap_mask & IB_PORT_CM_SUP)))
+               rc = 0;
        else
-               rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
+               rc = -EOPNOTSUPP;
        return rc;
 }
 EXPORT_SYMBOL(ib_modify_port);
Jason Gunthorpe Oct. 23, 2019, 2:40 p.m. UTC | #4
On Tue, Oct 22, 2019 at 11:21:38PM +0300, Kamal Heib wrote:
> On Tue, Oct 22, 2019 at 04:20:02PM -0300, Jason Gunthorpe wrote:
> > On Fri, Oct 18, 2019 at 12:41:12PM +0300, Kamal Heib wrote:
> > > Improving return code from ib_modify_port() by doing the following:
> > > 1- Use "-EOPNOTSUPP" instead "-ENOSYS" which is the proper return code.
> > > 2- Avoid confusion by return "-EOPNOTSUPP" when modify_port() isn't
> > >    supplied by the provider and the protocol is IB, otherwise return
> > >    success to avoid failure of the ib_modify_port() in CM layer.
> > > 
> > > Fixes: 61e0962d5221 ("IB: Avoid ib_modify_port() failure for RoCE devices")
> > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > >  drivers/infiniband/core/device.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > index a667636f74bf..626ac18dd3a7 100644
> > > +++ b/drivers/infiniband/core/device.c
> > > @@ -2397,7 +2397,7 @@ int ib_modify_port(struct ib_device *device,
> > >  					     port_modify_mask,
> > >  					     port_modify);
> > >  	else
> > > -		rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
> > > +		rc = rdma_protocol_ib(device, port_num) ? -EOPNOTSUPP : 0;
> > >  	return rc;
> > 
> > Oh gross, this is such an ugly hack
> > 
> > roce mode should allow a fake IB_PORT_CM_SUP to be manipulated and
> > nothing else.
> > 
> > All other cases should return EOPNOTSUPP as something has gone really
> > wrong
> > 
> > Jason
> 
> Do you mean something like the following:
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index a667636f74bf..1db97f6f5dec 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -2396,8 +2396,12 @@ int ib_modify_port(struct ib_device *device,
>                 rc = device->ops.modify_port(device, port_num,
>                                              port_modify_mask,
>                                              port_modify);
> +       else if (rdma_protocol_roce(device, port_num) &&
> +                ((port_modify->set_port_cap_mask & IB_PORT_CM_SUP) ||
> +                (port_modify->clr_port_cap_mask & IB_PORT_CM_SUP)))

That isn't the right logic.. it is more like & ~IB_PORT_CM_SUP) == 0

Jason

Patch
diff mbox series

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index a667636f74bf..626ac18dd3a7 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2397,7 +2397,7 @@  int ib_modify_port(struct ib_device *device,
 					     port_modify_mask,
 					     port_modify);
 	else
-		rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
+		rc = rdma_protocol_ib(device, port_num) ? -EOPNOTSUPP : 0;
 	return rc;
 }
 EXPORT_SYMBOL(ib_modify_port);