> 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.
>
> 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
> >
