Re: [EXT] [PATCH for-next v2 1/4] RDMA/core: Fix return code when modify_port isn't supported

[Kamal Heib <kamalheib1@gmail.com>]

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