| Message ID | 20191016072234.28442-2-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA: modify_port improvements | expand |
> 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
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 >
> 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 > >
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 > > >
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);
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(-)