On Thu, Oct 17, 2019 at 09:14:53AM +0000, Michal Kalderon wrote:
> > From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> > owner@xxxxxxxxxxxxxxx> On Behalf Of Kamal Heib
> >
> > On Wed, Oct 16, 2019 at 08:05:49AM +0000, Michal Kalderon wrote:
> > > > From: Kamal Heib <kamalheib1@xxxxxxxxx>
> > > > 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@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..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@xxxxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > Signed-off-by: Selvin Xavier <selvin.xavier@xxxxxxxxxxxx>
> > Reviewed-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > Signed-off-by: Doug Ledford <dledford@xxxxxxxxxx>
> >
> > 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
> > >
