On Thu, May 17, 2018 at 03:53:54PM -0600, Jason Gunthorpe wrote:
> On Fri, May 18, 2018 at 12:44:22AM +0300, Leon Romanovsky wrote:
> > On Thu, May 17, 2018 at 02:45:13PM -0600, Jason Gunthorpe wrote:
> > > The internal flag IP_BASED_GIDS was added to a field that was being used
> > > to hold the port Info CapabilityMask without considering the effects
> > > this will have. Since most drivers just use the value from the HW MAD
> > > it means IP_BASED_GIDS will also become set on any HW that sets the IBA
> > > flag IsOtherLocalChangesNoticeSupported - which is not intended.
> > >
> > > Fix this by keeping port_cap_flags only for the IBA CapabilityMask value
> > > and store unrelated flags externally. Move the bit definitions for this to
> > > ib_mad.h to make it clear what is happening.
> > >
> > > To keep the uAPI unchanged define a new set of flags in the uapi header
> > > that are only used by ib_uverbs_query_port_resp.port_cap_flags which match
> > > the current flags supported in rdma-core, and the values exposed by the
> > > current kernel.
> > >
> > > Fixes: b4a26a27287a ("IB: Report using RoCE IP based gids in port caps")
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > drivers/infiniband/core/uverbs_cmd.c | 23 ++++++++++-
> > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 4 +-
> > > drivers/infiniband/hw/mlx4/main.c | 3 +-
> > > drivers/infiniband/hw/mlx5/main.c | 4 +-
> > > drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 9 ++---
> > > drivers/infiniband/hw/qedr/verbs.c | 2 +-
> > > drivers/infiniband/hw/qib/qib_verbs.h | 3 --
> > > .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.c | 3 +-
> > > include/rdma/ib_mad.h | 33 ++++++++++++++++
> > > include/rdma/ib_verbs.h | 31 ++-------------
> > > include/uapi/rdma/ib_user_ioctl_verbs.h | 38 +++++++++++++++++++
> > > include/uapi/rdma/ib_user_verbs.h | 2 +-
> > > 12 files changed, 110 insertions(+), 45 deletions(-)
> > >
> > > This is the patch I alluded to for the IB_PORT_GRH_RQUIRED series.
> > >
> > > Since all old kernels will set bits in port_cap_mask based on
> > > IBAvalues and HW support, all bits in that field are already taken.
> > >
> > > Leon, this also obsoletes your patch moving to the uapi header.
> > >
> >
> > It is too late for me to do proper review, but I wanted to raise your
> > attention that IP_BASED_GIDS is used by rdmatool too, through
> > port_cap_flag field.
> >
> > https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/tree/rdma/link.c#n46
>
> I think rdmatool might be new enough, IP_GIDS rare enough, and the
> defect harmless enough that we can fix it properly.
>
> Netlink should report the raw IB port capability mask in that
> attribute, not some data from the uverbs interface.
>
> IMHO, just drop IP_GIDS from iproute2 and we are good.
RDMtool is part of my current distribution and deployment of mlx4/mlx5
is so large, so I can't agree with you that it is "rare enough".
I'll review your change on Monday, but I have gut feeling that you broke
backward compatibility with already deployed rdmatool.
Thanks
>
> Jason
Attachment:
signature.asc
Description: PGP signature
