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@xxxxxxxxx>
> > > 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
