On Mon, May 21, 2018 at 12:09:05PM +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.
> >
>
> Hi Jason,
>
> It is not enough, since you are breaking ABI, let's do it till the end.
>
> 1. Please create new uapi header file, let's call it ib_iba.h.
> This file will include IBA spec definitions, but PortInfo is
> enough for now.
Not super keen on that, we have not really had the kernel provide IBA
definitions in uapi headers..
We already have these definitions in various places in userspace,
userspace could copy the IBA definitions from the kernel without being
in the uapi directory.
I'm not really sure if rdma tool should even print this stuff. It
doesn't really seem in-scope for it, since it is very IB specific and
the roce ports just sort-of emulate it.
> > +/* PortInfo CapabilityMask */
> > +enum ib_port_capability_mask_bits {
> > + IB_PORT_SM = 1 << 1,
> > + IB_PORT_NOTICE_SUP = 1 << 2,
> > + IB_PORT_TRAP_SUP = 1 << 3,
> > + IB_PORT_OPT_IPD_SUP = 1 << 4,
> > + IB_PORT_AUTO_MIGR_SUP = 1 << 5,
> > + IB_PORT_SL_MAP_SUP = 1 << 6,
> > + IB_PORT_MKEY_NVRAM = 1 << 7,
> > + IB_PORT_PKEY_NVRAM = 1 << 8,
> > + IB_PORT_LED_INFO_SUP = 1 << 9,
> > + IB_PORT_SM_DISABLED = 1 << 10,
> > + IB_PORT_SYS_IMAGE_GUID_SUP = 1 << 11,
> > + IB_PORT_PKEY_SW_EXT_PORT_TRAP_SUP = 1 << 12,
> > + IB_PORT_EXTENDED_SPEEDS_SUP = 1 << 14,
> > + IB_PORT_CM_SUP = 1 << 16,
> > + IB_PORT_SNMP_TUNNEL_SUP = 1 << 17,
> > + IB_PORT_REINIT_SUP = 1 << 18,
> > + IB_PORT_DEVICE_MGMT_SUP = 1 << 19,
> > + IB_PORT_VENDOR_CLASS_SUP = 1 << 20,
> > + IB_PORT_DR_NOTICE_SUP = 1 << 21,
> > + IB_PORT_CAP_MASK_NOTICE_SUP = 1 << 22,
> > + IB_PORT_BOOT_MGMT_SUP = 1 << 23,
> > + IB_PORT_LINK_LATENCY_SUP = 1 << 24,
> > + IB_PORT_CLIENT_REG_SUP = 1 << 25,
> > + IB_PORT_OTHER_LOCAL_CHANGES_SUP = 1 << 26,
> > + IB_PORT_LINK_SPEED_WIDTH_TABLE_SUP = 1 << 27,
> > + IB_PORT_VENDOR_SPECIFIC_MADS_TABLE_SUP = 1 << 28,
> > + IB_PORT_MCAST_PKEY_TRAP_SUPPRESSION_SUP = 1 << 29,
> > + IB_PORT_MCAST_FDB_TOP_SUP = 1 << 30,
> > + IB_PORT_HIERARCHY_INFO_SUP = 1ULL << 31,
> > +};
>
> The main advantage of enums is type-checks during compilation, so let's
> do not do enum with bitshifts.
?? We almost always do this for bitfields. The enum isn't for type
checking because C provides no way to type check bitfields, it is done
to avoid #define which is broadly harmful.
Stylistically I want to see rdma continue to stay away from #define,
not add more..
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
