All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-next v2 0/4] RDMA: modify_port improvements
@ 2019-10-16  7:22 Kamal Heib
  2019-10-16  7:22 ` [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported Kamal Heib
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Kamal Heib @ 2019-10-16  7:22 UTC (permalink / raw)
  To: linux-rdma
  Cc: Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier,
	Michal Kalderon, Kamal Heib

Changelog:
v2: Include fixes lines.

This series includes three patches which fix the return values from
modify_port() callbacks when they aren't supported.

Kamal Heib (4):
  RDMA/core: Fix return code when modify_port isn't supported
  RDMA/hns: Remove unsupported modify_port callback
  RDMA/ocrdma: Remove unsupported modify_port callback
  RDMA/qedr: Remove unsupported modify_port callback

 drivers/infiniband/core/device.c            | 2 +-
 drivers/infiniband/hw/hns/hns_roce_main.c   | 7 -------
 drivers/infiniband/hw/ocrdma/ocrdma_main.c  | 1 -
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 6 ------
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.h | 2 --
 drivers/infiniband/hw/qedr/main.c           | 1 -
 drivers/infiniband/hw/qedr/verbs.c          | 6 ------
 drivers/infiniband/hw/qedr/verbs.h          | 2 --
 8 files changed, 1 insertion(+), 26 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported
  2019-10-16  7:22 [PATCH for-next v2 0/4] RDMA: modify_port improvements Kamal Heib
@ 2019-10-16  7:22 ` Kamal Heib
  2019-10-16  8:05   ` [EXT] " Michal Kalderon
  2019-10-16  7:22 ` [PATCH for-next v2 2/4] RDMA/hns: Remove unsupported modify_port callback Kamal Heib
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Kamal Heib @ 2019-10-16  7:22 UTC (permalink / raw)
  To: linux-rdma
  Cc: Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier,
	Michal Kalderon, Kamal Heib

The proper return code is "-EOPNOTSUPP" when modify_port callback is not
supported.

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..98a01caf7850 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_roce(device, port_num) ? 0 : -EOPNOTSUPP;
 	return rc;
 }
 EXPORT_SYMBOL(ib_modify_port);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH for-next v2 2/4] RDMA/hns: Remove unsupported modify_port callback
  2019-10-16  7:22 [PATCH for-next v2 0/4] RDMA: modify_port improvements Kamal Heib
  2019-10-16  7:22 ` [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported Kamal Heib
@ 2019-10-16  7:22 ` Kamal Heib
  2019-10-16  7:22 ` [PATCH for-next v2 3/4] RDMA/ocrdma: " Kamal Heib
  2019-10-16  7:22 ` [PATCH for-next v2 4/4] RDMA/qedr: " Kamal Heib
  3 siblings, 0 replies; 9+ messages in thread
From: Kamal Heib @ 2019-10-16  7:22 UTC (permalink / raw)
  To: linux-rdma
  Cc: Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier,
	Michal Kalderon, Kamal Heib

There is no need to return always zero for function which is not
supported.

Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver")
Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
 drivers/infiniband/hw/hns/hns_roce_main.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c
index b5d196c119ee..b241f74a7e3b 100644
--- a/drivers/infiniband/hw/hns/hns_roce_main.c
+++ b/drivers/infiniband/hw/hns/hns_roce_main.c
@@ -301,12 +301,6 @@ static int hns_roce_modify_device(struct ib_device *ib_dev, int mask,
 	return 0;
 }
 
-static int hns_roce_modify_port(struct ib_device *ib_dev, u8 port_num, int mask,
-				struct ib_port_modify *props)
-{
-	return 0;
-}
-
 static int hns_roce_alloc_ucontext(struct ib_ucontext *uctx,
 				   struct ib_udata *udata)
 {
@@ -438,7 +432,6 @@ static const struct ib_device_ops hns_roce_dev_ops = {
 	.get_port_immutable = hns_roce_port_immutable,
 	.mmap = hns_roce_mmap,
 	.modify_device = hns_roce_modify_device,
-	.modify_port = hns_roce_modify_port,
 	.modify_qp = hns_roce_modify_qp,
 	.query_ah = hns_roce_query_ah,
 	.query_device = hns_roce_query_device,
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH for-next v2 3/4] RDMA/ocrdma: Remove unsupported modify_port callback
  2019-10-16  7:22 [PATCH for-next v2 0/4] RDMA: modify_port improvements Kamal Heib
  2019-10-16  7:22 ` [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported Kamal Heib
  2019-10-16  7:22 ` [PATCH for-next v2 2/4] RDMA/hns: Remove unsupported modify_port callback Kamal Heib
@ 2019-10-16  7:22 ` Kamal Heib
  2019-10-16  7:22 ` [PATCH for-next v2 4/4] RDMA/qedr: " Kamal Heib
  3 siblings, 0 replies; 9+ messages in thread
From: Kamal Heib @ 2019-10-16  7:22 UTC (permalink / raw)
  To: linux-rdma
  Cc: Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier,
	Michal Kalderon, Kamal Heib

There is no need to return always zero for function which is not
supported.

Fixes: fe2caefcdf58 ("RDMA/ocrdma: Add driver for Emulex OneConnect IBoE RDMAadapter")
Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
 drivers/infiniband/hw/ocrdma/ocrdma_main.c  | 1 -
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 6 ------
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.h | 2 --
 3 files changed, 9 deletions(-)

diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c b/drivers/infiniband/hw/ocrdma/ocrdma_main.c
index c15cfc6cef81..d8c47d24d6d6 100644
--- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c
+++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c
@@ -166,7 +166,6 @@ static const struct ib_device_ops ocrdma_dev_ops = {
 	.get_port_immutable = ocrdma_port_immutable,
 	.map_mr_sg = ocrdma_map_mr_sg,
 	.mmap = ocrdma_mmap,
-	.modify_port = ocrdma_modify_port,
 	.modify_qp = ocrdma_modify_qp,
 	.poll_cq = ocrdma_poll_cq,
 	.post_recv = ocrdma_post_recv,
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c
index e8267e590772..e72050de5734 100644
--- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c
+++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c
@@ -190,12 +190,6 @@ int ocrdma_query_port(struct ib_device *ibdev,
 	return 0;
 }
 
-int ocrdma_modify_port(struct ib_device *ibdev, u8 port, int mask,
-		       struct ib_port_modify *props)
-{
-	return 0;
-}
-
 static int ocrdma_add_mmap(struct ocrdma_ucontext *uctx, u64 phy_addr,
 			   unsigned long len)
 {
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.h b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.h
index 32488da1b752..3a5010881be5 100644
--- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.h
+++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.h
@@ -54,8 +54,6 @@ int ocrdma_arm_cq(struct ib_cq *, enum ib_cq_notify_flags flags);
 int ocrdma_query_device(struct ib_device *, struct ib_device_attr *props,
 			struct ib_udata *uhw);
 int ocrdma_query_port(struct ib_device *, u8 port, struct ib_port_attr *props);
-int ocrdma_modify_port(struct ib_device *, u8 port, int mask,
-		       struct ib_port_modify *props);
 
 enum rdma_protocol_type
 ocrdma_query_protocol(struct ib_device *device, u8 port_num);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH for-next v2 4/4] RDMA/qedr: Remove unsupported modify_port callback
  2019-10-16  7:22 [PATCH for-next v2 0/4] RDMA: modify_port improvements Kamal Heib
                   ` (2 preceding siblings ...)
  2019-10-16  7:22 ` [PATCH for-next v2 3/4] RDMA/ocrdma: " Kamal Heib
@ 2019-10-16  7:22 ` Kamal Heib
  3 siblings, 0 replies; 9+ messages in thread
From: Kamal Heib @ 2019-10-16  7:22 UTC (permalink / raw)
  To: linux-rdma
  Cc: Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier,
	Michal Kalderon, Kamal Heib

There is no need to return always zero for function which is not
supported.

Fixes: ac1b36e55a51 ("qedr: Add support for user context verbs")
Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
 drivers/infiniband/hw/qedr/main.c  | 1 -
 drivers/infiniband/hw/qedr/verbs.c | 6 ------
 drivers/infiniband/hw/qedr/verbs.h | 2 --
 3 files changed, 9 deletions(-)

diff --git a/drivers/infiniband/hw/qedr/main.c b/drivers/infiniband/hw/qedr/main.c
index 5136b835e1ba..e5f36adb0120 100644
--- a/drivers/infiniband/hw/qedr/main.c
+++ b/drivers/infiniband/hw/qedr/main.c
@@ -212,7 +212,6 @@ static const struct ib_device_ops qedr_dev_ops = {
 	.get_link_layer = qedr_link_layer,
 	.map_mr_sg = qedr_map_mr_sg,
 	.mmap = qedr_mmap,
-	.modify_port = qedr_modify_port,
 	.modify_qp = qedr_modify_qp,
 	.modify_srq = qedr_modify_srq,
 	.poll_cq = qedr_poll_cq,
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index 6f3ce86019b7..fee02ac47f32 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -250,12 +250,6 @@ int qedr_query_port(struct ib_device *ibdev, u8 port, struct ib_port_attr *attr)
 	return 0;
 }
 
-int qedr_modify_port(struct ib_device *ibdev, u8 port, int mask,
-		     struct ib_port_modify *props)
-{
-	return 0;
-}
-
 static int qedr_add_mmap(struct qedr_ucontext *uctx, u64 phy_addr,
 			 unsigned long len)
 {
diff --git a/drivers/infiniband/hw/qedr/verbs.h b/drivers/infiniband/hw/qedr/verbs.h
index 9aaa90283d6e..d81b81f86f0a 100644
--- a/drivers/infiniband/hw/qedr/verbs.h
+++ b/drivers/infiniband/hw/qedr/verbs.h
@@ -35,8 +35,6 @@
 int qedr_query_device(struct ib_device *ibdev,
 		      struct ib_device_attr *attr, struct ib_udata *udata);
 int qedr_query_port(struct ib_device *, u8 port, struct ib_port_attr *props);
-int qedr_modify_port(struct ib_device *, u8 port, int mask,
-		     struct ib_port_modify *props);
 
 int qedr_iw_query_gid(struct ib_device *ibdev, u8 port,
 		      int index, union ib_gid *gid);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [EXT] [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported
  2019-10-16  7:22 ` [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported Kamal Heib
@ 2019-10-16  8:05   ` Michal Kalderon
  2019-10-17  9:09     ` Kamal Heib
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Kalderon @ 2019-10-16  8:05 UTC (permalink / raw)
  To: Kamal Heib, linux-rdma
  Cc: Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier

> From: Kamal Heib <kamalheib1@gmail.com>
> Sent: Wednesday, October 16, 2019 10:23 AM
> 
> External Email
> 
> ----------------------------------------------------------------------
> The proper return code is "-EOPNOTSUPP" when modify_port callback is not
> supported.
> 
> 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..98a01caf7850 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_roce(device, port_num) ? 0 : -
> EOPNOTSUPP;

This is a bit confusing, looks like for RoCE it's ok not to have a callback but for the 
The other protocols it's required. For iWARP for example there also isn't a modify-port.
Is there any other protocol except ib that this is relevant to ? 
If not perhaps modify rdma_protocol_roce(..)? to rdma_protocol_ib(...)? -EOPNOTSUPP : 0?



>  	return rc;
>  }
>  EXPORT_SYMBOL(ib_modify_port);
> --
> 2.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [EXT] [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported
  2019-10-16  8:05   ` [EXT] " Michal Kalderon
@ 2019-10-17  9:09     ` Kamal Heib
  2019-10-17  9:14       ` Michal Kalderon
  0 siblings, 1 reply; 9+ messages in thread
From: Kamal Heib @ 2019-10-17  9:09 UTC (permalink / raw)
  To: Michal Kalderon
  Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier

On Wed, Oct 16, 2019 at 08:05:49AM +0000, Michal Kalderon wrote:
> > From: Kamal Heib <kamalheib1@gmail.com>
> > Sent: Wednesday, October 16, 2019 10:23 AM
> > 
> > External Email
> > 
> > ----------------------------------------------------------------------
> > The proper return code is "-EOPNOTSUPP" when modify_port callback is not
> > supported.
> > 
> > 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..98a01caf7850 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_roce(device, port_num) ? 0 : -
> > EOPNOTSUPP;
> 
> This is a bit confusing, looks like for RoCE it's ok not to have a callback but for the 
> The other protocols it's required. For iWARP for example there also isn't a modify-port.
> Is there any other protocol except ib that this is relevant to ? 
> If not perhaps modify rdma_protocol_roce(..)? to rdma_protocol_ib(...)? -EOPNOTSUPP : 0?
>

Yes, I agree this is confusing.

This change was introduced by the following commit to avoid the failures
of ib_modify_port() calls from CM when the protocol is RoCE, I also see
that almost all providers that support RoCE return success from the
modify_port() callback (hns, mlx4, mlx5, ocrdma, qedr), except rxe and
vmw_pvrdma which I think they shouldn't.

So, I suggest adding a check to CM avoid calling ib_modify_port() when the
protocol is RoCE and cleanup the mess from the providers, thoughts? 

commit 61e0962d52216f2e5bab59bb055f1210e41f484f
Author: Selvin Xavier <selvin.xavier@broadcom.com>
Date:   Wed Aug 23 01:08:07 2017 -0700

    IB: Avoid ib_modify_port() failure for RoCE devices
    
    IB CM calls ib_modify_port() irrespective of link layer. If the
    failure is returned, the mad agent gets unregistered for those
    devices. Recently, modify_port() hook was removed from some of the
    low level drivers as it was always returning success. This breaks
    rdma connection establishment over those devices.
    For ethernet devices, Qkey violation and port capabilities are not
    applicable. So returning success for RoCE when modify_port hook is
    is not implemented.
    
    Cc: Leon Romanovsky <leon@kernel.org>
    Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
    Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
    Signed-off-by: Doug Ledford <dledford@redhat.com>

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index fc6be1175183..2466ffc6362d 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -1005,14 +1005,17 @@ int ib_modify_port(struct ib_device *device,
                   u8 port_num, int port_modify_mask,
                   struct ib_port_modify *port_modify)
 {
-       if (!device->modify_port)
-               return -ENOSYS;
+       int rc;
 
        if (!rdma_is_port_valid(device, port_num))
                return -EINVAL;
 
-       return device->modify_port(device, port_num, port_modify_mask,
-                                  port_modify);
+       if (device->modify_port)
+               rc = device->modify_port(device, port_num, port_modify_mask,
+                                          port_modify);
+       else
+               rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
+       return rc;
 }
 EXPORT_SYMBOL(ib_modify_port);


> 
> 
> >  	return rc;
> >  }
> >  EXPORT_SYMBOL(ib_modify_port);
> > --
> > 2.20.1
> 

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [EXT] [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported
  2019-10-17  9:09     ` Kamal Heib
@ 2019-10-17  9:14       ` Michal Kalderon
  2019-10-18  9:12         ` Kamal Heib
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Kalderon @ 2019-10-17  9:14 UTC (permalink / raw)
  To: Kamal Heib
  Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier

> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Kamal Heib
> 
> On Wed, Oct 16, 2019 at 08:05:49AM +0000, Michal Kalderon wrote:
> > > From: Kamal Heib <kamalheib1@gmail.com>
> > > Sent: Wednesday, October 16, 2019 10:23 AM
> > >
> > > External Email
> > >
> > > --------------------------------------------------------------------
> > > -- The proper return code is "-EOPNOTSUPP" when modify_port callback
> > > is not supported.
> > >
> > > 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..98a01caf7850 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_roce(device, port_num) ? 0 : -
> > > EOPNOTSUPP;
> >
> > This is a bit confusing, looks like for RoCE it's ok not to have a
> > callback but for the The other protocols it's required. For iWARP for
> example there also isn't a modify-port.
> > Is there any other protocol except ib that this is relevant to ?
> > If not perhaps modify rdma_protocol_roce(..)? to rdma_protocol_ib(...)? -
> EOPNOTSUPP : 0?
> >
> 
> Yes, I agree this is confusing.
> 
> This change was introduced by the following commit to avoid the failures of
> ib_modify_port() calls from CM when the protocol is RoCE, I also see that
> almost all providers that support RoCE return success from the
> modify_port() callback (hns, mlx4, mlx5, ocrdma, qedr), except rxe and
> vmw_pvrdma which I think they shouldn't.
> 
> So, I suggest adding a check to CM avoid calling ib_modify_port() when the
> protocol is RoCE and cleanup the mess from the providers, thoughts?
I think we can leave the logic inside the function ib_modify_port, and just return
Success if the protocol isn't IB. 

> 
> commit 61e0962d52216f2e5bab59bb055f1210e41f484f
> Author: Selvin Xavier <selvin.xavier@broadcom.com>
> Date:   Wed Aug 23 01:08:07 2017 -0700
> 
>     IB: Avoid ib_modify_port() failure for RoCE devices
> 
>     IB CM calls ib_modify_port() irrespective of link layer. If the
>     failure is returned, the mad agent gets unregistered for those
>     devices. Recently, modify_port() hook was removed from some of the
>     low level drivers as it was always returning success. This breaks
>     rdma connection establishment over those devices.
>     For ethernet devices, Qkey violation and port capabilities are not
>     applicable. So returning success for RoCE when modify_port hook is
>     is not implemented.
> 
>     Cc: Leon Romanovsky <leon@kernel.org>
>     Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
>     Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
>     Signed-off-by: Doug Ledford <dledford@redhat.com>
> 
> diff --git a/drivers/infiniband/core/device.c
> b/drivers/infiniband/core/device.c
> index fc6be1175183..2466ffc6362d 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1005,14 +1005,17 @@ int ib_modify_port(struct ib_device *device,
>                    u8 port_num, int port_modify_mask,
>                    struct ib_port_modify *port_modify)  {
> -       if (!device->modify_port)
> -               return -ENOSYS;
> +       int rc;
> 
>         if (!rdma_is_port_valid(device, port_num))
>                 return -EINVAL;
> 
> -       return device->modify_port(device, port_num, port_modify_mask,
> -                                  port_modify);
> +       if (device->modify_port)
> +               rc = device->modify_port(device, port_num, port_modify_mask,
> +                                          port_modify);
> +       else
> +               rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
> +       return rc;
>  }
>  EXPORT_SYMBOL(ib_modify_port);
> 
> 
> >
> >
> > >  	return rc;
> > >  }
> > >  EXPORT_SYMBOL(ib_modify_port);
> > > --
> > > 2.20.1
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [EXT] [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported
  2019-10-17  9:14       ` Michal Kalderon
@ 2019-10-18  9:12         ` Kamal Heib
  0 siblings, 0 replies; 9+ messages in thread
From: Kamal Heib @ 2019-10-18  9:12 UTC (permalink / raw)
  To: Michal Kalderon
  Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Lijun Ou, Selvin Xavier

On Thu, Oct 17, 2019 at 09:14:53AM +0000, Michal Kalderon wrote:
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Kamal Heib
> > 
> > On Wed, Oct 16, 2019 at 08:05:49AM +0000, Michal Kalderon wrote:
> > > > From: Kamal Heib <kamalheib1@gmail.com>
> > > > Sent: Wednesday, October 16, 2019 10:23 AM
> > > >
> > > > External Email
> > > >
> > > > --------------------------------------------------------------------
> > > > -- The proper return code is "-EOPNOTSUPP" when modify_port callback
> > > > is not supported.
> > > >
> > > > 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..98a01caf7850 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_roce(device, port_num) ? 0 : -
> > > > EOPNOTSUPP;
> > >
> > > This is a bit confusing, looks like for RoCE it's ok not to have a
> > > callback but for the The other protocols it's required. For iWARP for
> > example there also isn't a modify-port.
> > > Is there any other protocol except ib that this is relevant to ?
> > > If not perhaps modify rdma_protocol_roce(..)? to rdma_protocol_ib(...)? -
> > EOPNOTSUPP : 0?
> > >
> > 
> > Yes, I agree this is confusing.
> > 
> > This change was introduced by the following commit to avoid the failures of
> > ib_modify_port() calls from CM when the protocol is RoCE, I also see that
> > almost all providers that support RoCE return success from the
> > modify_port() callback (hns, mlx4, mlx5, ocrdma, qedr), except rxe and
> > vmw_pvrdma which I think they shouldn't.
> > 
> > So, I suggest adding a check to CM avoid calling ib_modify_port() when the
> > protocol is RoCE and cleanup the mess from the providers, thoughts?
> I think we can leave the logic inside the function ib_modify_port, and just return
> Success if the protocol isn't IB. 
>

OK, I'll fix it in v3.

> > 
> > commit 61e0962d52216f2e5bab59bb055f1210e41f484f
> > Author: Selvin Xavier <selvin.xavier@broadcom.com>
> > Date:   Wed Aug 23 01:08:07 2017 -0700
> > 
> >     IB: Avoid ib_modify_port() failure for RoCE devices
> > 
> >     IB CM calls ib_modify_port() irrespective of link layer. If the
> >     failure is returned, the mad agent gets unregistered for those
> >     devices. Recently, modify_port() hook was removed from some of the
> >     low level drivers as it was always returning success. This breaks
> >     rdma connection establishment over those devices.
> >     For ethernet devices, Qkey violation and port capabilities are not
> >     applicable. So returning success for RoCE when modify_port hook is
> >     is not implemented.
> > 
> >     Cc: Leon Romanovsky <leon@kernel.org>
> >     Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> >     Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
> >     Signed-off-by: Doug Ledford <dledford@redhat.com>
> > 
> > diff --git a/drivers/infiniband/core/device.c
> > b/drivers/infiniband/core/device.c
> > index fc6be1175183..2466ffc6362d 100644
> > --- a/drivers/infiniband/core/device.c
> > +++ b/drivers/infiniband/core/device.c
> > @@ -1005,14 +1005,17 @@ int ib_modify_port(struct ib_device *device,
> >                    u8 port_num, int port_modify_mask,
> >                    struct ib_port_modify *port_modify)  {
> > -       if (!device->modify_port)
> > -               return -ENOSYS;
> > +       int rc;
> > 
> >         if (!rdma_is_port_valid(device, port_num))
> >                 return -EINVAL;
> > 
> > -       return device->modify_port(device, port_num, port_modify_mask,
> > -                                  port_modify);
> > +       if (device->modify_port)
> > +               rc = device->modify_port(device, port_num, port_modify_mask,
> > +                                          port_modify);
> > +       else
> > +               rc = rdma_protocol_roce(device, port_num) ? 0 : -ENOSYS;
> > +       return rc;
> >  }
> >  EXPORT_SYMBOL(ib_modify_port);
> > 
> > 
> > >
> > >
> > > >  	return rc;
> > > >  }
> > > >  EXPORT_SYMBOL(ib_modify_port);
> > > > --
> > > > 2.20.1
> > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-10-18  9:12 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-16  7:22 [PATCH for-next v2 0/4] RDMA: modify_port improvements Kamal Heib
2019-10-16  7:22 ` [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported Kamal Heib
2019-10-16  8:05   ` [EXT] " Michal Kalderon
2019-10-17  9:09     ` Kamal Heib
2019-10-17  9:14       ` Michal Kalderon
2019-10-18  9:12         ` Kamal Heib
2019-10-16  7:22 ` [PATCH for-next v2 2/4] RDMA/hns: Remove unsupported modify_port callback Kamal Heib
2019-10-16  7:22 ` [PATCH for-next v2 3/4] RDMA/ocrdma: " Kamal Heib
2019-10-16  7:22 ` [PATCH for-next v2 4/4] RDMA/qedr: " Kamal Heib

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.