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