IB/Verbs: IB Management Helpers

Add new callback query_transport() and implement for each HW.

Mapping List:
node-type link-layer old-transport new-transport
nes RNIC ETH IWARP IWARP
amso1100 RNIC ETH IWARP IWARP

cxgb3   	RNIC		ETH		IWARP		IWARP
cxgb4   	RNIC		ETH		IWARP		IWARP
usnic   	USNIC_UDP	ETH		USNIC_UDP	USNIC_UDP
ocrdma  	IB_CA		ETH		IB		IBOE
mlx4    	IB_CA		IB/ETH		IB		IB/IBOE
mlx5    	IB_CA		IB		IB		IB
ehca    	IB_CA		IB		IB		IB
ipath   	IB_CA		IB		IB		IB
mthca   	IB_CA		IB		IB		IB
qib     	IB_CA		IB		IB		IB

Cc Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Cc Doug Ledford <[email protected]>

Cc Ira Weiny <[email protected]>

Cc Sean Hefty <[email protected]>

Signed-off-by Michael Wang <[email protected]>

---
 drivers/infiniband/core/device.c             |  1 +
 drivers/infiniband/core/verbs.c              |  4 +++-
 drivers/infiniband/hw/amso1100/c2_provider.c |  7 +++++++
 drivers/infiniband/hw/cxgb3/iwch_provider.c  |  7 +++++++
 drivers/infiniband/hw/cxgb4/provider.c       |  7 +++++++
 drivers/infiniband/hw/ehca/ehca_hca.c        |  6 ++++++
 drivers/infiniband/hw/ehca/ehca_iverbs.h     |  3 +++
 drivers/infiniband/hw/ehca/ehca_main.c       |  1 +
 drivers/infiniband/hw/ipath/ipath_verbs.c    |  7 +++++++
 drivers/infiniband/hw/mlx4/main.c            | 10 ++++++++++
 drivers/infiniband/hw/mlx5/main.c            |  7 +++++++
 drivers/infiniband/hw/mthca/mthca_provider.c |  7 +++++++
 drivers/infiniband/hw/nes/nes_verbs.c        |  6 ++++++
 drivers/infiniband/hw/ocrdma/ocrdma_main.c   |  1 +
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.c  |  6 ++++++
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.h  |  3 +++
 drivers/infiniband/hw/qib/qib_verbs.c        |  7 +++++++
 drivers/infiniband/hw/usnic/usnic_ib_main.c  |  1 +
 drivers/infiniband/hw/usnic/usnic_ib_verbs.c |  6 ++++++
 drivers/infiniband/hw/usnic/usnic_ib_verbs.h |  2 ++
 include/rdma/ib_verbs.h                      |  7 ++++++-
 21 files changed, 104 insertions(+), 2 deletions(-)

drivers/infiniband/core/device.c +1 -0

index 18c1ece..a9587c4 100644
@@ -76,6 +76,7 @@ static int ib_device_check_mandatory(struct ib_device *device)
 	} mandatory_table[] = {
 		IB_MANDATORY_FUNC(query_device),
 		IB_MANDATORY_FUNC(query_port),
+		IB_MANDATORY_FUNC(query_transport),
 		IB_MANDATORY_FUNC(query_pkey),
 		IB_MANDATORY_FUNC(query_gid),
 		IB_MANDATORY_FUNC(alloc_pd),

drivers/infiniband/core/verbs.c +3 -1

index f93eb8d..83370de 100644
@@ -133,14 +133,16 @@ enum rdma_link_layer rdma_port_get_link_layer(struct ib_device *device, u8 port_
 	if (device->get_link_layer)
 		return device->get_link_layer(device, port_num);
 
-	switch (rdma_node_get_transport(device->node_type)) {
+	switch (device->query_transport(device, port_num)) {
 	case RDMA_TRANSPORT_IB:
+	case RDMA_TRANSPORT_IBOE:
 		return IB_LINK_LAYER_INFINIBAND;
 	case RDMA_TRANSPORT_IWARP:
 	case RDMA_TRANSPORT_USNIC:
 	case RDMA_TRANSPORT_USNIC_UDP:
 		return IB_LINK_LAYER_ETHERNET;
 	default:
+		BUG();
 		return IB_LINK_LAYER_UNSPECIFIED;
 	}
 }

drivers/infiniband/hw/amso1100/c2_provider.c +7 -0

index bdf3507..d46bbb0 100644
@@ -99,6 +99,12 @@ static int c2_query_port(struct ib_device *ibdev,
 	return 0;
 }
 
+static enum rdma_transport_type
+c2_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IWARP;
+}
+
 static int c2_query_pkey(struct ib_device *ibdev,
 			 u8 port, u16 index, u16 * pkey)
 {

@@ -801,6 +807,7 @@ int c2_register_device(struct c2_dev *dev)
 	dev->ibdev.dma_device = &dev->pcidev->dev;
 	dev->ibdev.query_device = c2_query_device;
 	dev->ibdev.query_port = c2_query_port;
+	dev->ibdev.query_transport = c2_query_transport;
 	dev->ibdev.query_pkey = c2_query_pkey;
 	dev->ibdev.query_gid = c2_query_gid;
 	dev->ibdev.alloc_ucontext = c2_alloc_ucontext;

drivers/infiniband/hw/cxgb3/iwch_provider.c +7 -0

index 811b24a..09682e9e 100644
@@ -1232,6 +1232,12 @@ static int iwch_query_port(struct ib_device *ibdev,
 	return 0;
 }
 
+static enum rdma_transport_type
+iwch_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IWARP;
+}
+
 static ssize_t show_rev(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {

@@ -1385,6 +1391,7 @@ int iwch_register_device(struct iwch_dev *dev)
 	dev->ibdev.dma_device = &(dev->rdev.rnic_info.pdev->dev);
 	dev->ibdev.query_device = iwch_query_device;
 	dev->ibdev.query_port = iwch_query_port;
+	dev->ibdev.query_transport = iwch_query_transport;
 	dev->ibdev.query_pkey = iwch_query_pkey;
 	dev->ibdev.query_gid = iwch_query_gid;
 	dev->ibdev.alloc_ucontext = iwch_alloc_ucontext;

drivers/infiniband/hw/cxgb4/provider.c +7 -0

index 66bd6a2..a445e0d 100644
@@ -390,6 +390,12 @@ static int c4iw_query_port(struct ib_device *ibdev, u8 port,
 	return 0;
 }
 
+static enum rdma_transport_type
+c4iw_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IWARP;
+}
+
 static ssize_t show_rev(struct device *dev, struct device_attribute *attr,
 			char *buf)
 {

@@ -506,6 +512,7 @@ int c4iw_register_device(struct c4iw_dev *dev)
 	dev->ibdev.dma_device = &(dev->rdev.lldi.pdev->dev);
 	dev->ibdev.query_device = c4iw_query_device;
 	dev->ibdev.query_port = c4iw_query_port;
+	dev->ibdev.query_transport = c4iw_query_transport;
 	dev->ibdev.query_pkey = c4iw_query_pkey;
 	dev->ibdev.query_gid = c4iw_query_gid;
 	dev->ibdev.alloc_ucontext = c4iw_alloc_ucontext;

drivers/infiniband/hw/ehca/ehca_hca.c +6 -0

index 9ed4d25..d5a34a6 100644
@@ -242,6 +242,12 @@ query_port1:
 	return ret;
 }
 
+enum rdma_transport_type
+ehca_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IB;
+}
+
 int ehca_query_sma_attr(struct ehca_shca *shca,
 			u8 port, struct ehca_sma_attr *attr)
 {

drivers/infiniband/hw/ehca/ehca_iverbs.h +3 -0

index 22f79af..cec945f 100644
@@ -49,6 +49,9 @@ int ehca_query_device(struct ib_device *ibdev, struct ib_device_attr *props);
 int ehca_query_port(struct ib_device *ibdev, u8 port,
 		    struct ib_port_attr *props);
 
+enum rdma_transport_type
+ehca_query_transport(struct ib_device *device, u8 port_num);
+
 int ehca_query_sma_attr(struct ehca_shca *shca, u8 port,
 			struct ehca_sma_attr *attr);
 

drivers/infiniband/hw/ehca/ehca_main.c +1 -0

index cd8d290..60e0a09 100644
@@ -467,6 +467,7 @@ static int ehca_init_device(struct ehca_shca *shca)
 	shca->ib_device.dma_device          = &shca->ofdev->dev;
 	shca->ib_device.query_device        = ehca_query_device;
 	shca->ib_device.query_port          = ehca_query_port;
+	shca->ib_device.query_transport     = ehca_query_transport;
 	shca->ib_device.query_gid           = ehca_query_gid;
 	shca->ib_device.query_pkey          = ehca_query_pkey;
 	/* shca->in_device.modify_device    = ehca_modify_device    */

drivers/infiniband/hw/ipath/ipath_verbs.c +7 -0

index 44ea939..58d36e3 100644
@@ -1638,6 +1638,12 @@ static int ipath_query_port(struct ib_device *ibdev,
 	return 0;
 }
 
+static enum rdma_transport_type
+ipath_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IB;
+}
+
 static int ipath_modify_device(struct ib_device *device,
 			       int device_modify_mask,
 			       struct ib_device_modify *device_modify)

@@ -2140,6 +2146,7 @@ int ipath_register_ib_device(struct ipath_devdata *dd)
 	dev->query_device = ipath_query_device;
 	dev->modify_device = ipath_modify_device;
 	dev->query_port = ipath_query_port;
+	dev->query_transport = ipath_query_transport;
 	dev->modify_port = ipath_modify_port;
 	dev->query_pkey = ipath_query_pkey;
 	dev->query_gid = ipath_query_gid;

drivers/infiniband/hw/mlx4/main.c +10 -0

index 0b280b1..28100bd 100644
@@ -413,6 +413,15 @@ static int mlx4_ib_query_port(struct ib_device *ibdev, u8 port,
 	return __mlx4_ib_query_port(ibdev, port, props, 0);
 }
 
+static enum rdma_transport_type
+mlx4_ib_query_transport(struct ib_device *device, u8 port_num)
+{
+	struct mlx4_dev *dev = to_mdev(device)->dev;
+
+	return dev->caps.port_mask[port_num] == MLX4_PORT_TYPE_IB ?
+		RDMA_TRANSPORT_IB : RDMA_TRANSPORT_IBOE;
+}
+
 int __mlx4_ib_query_gid(struct ib_device *ibdev, u8 port, int index,
 			union ib_gid *gid, int netw_view)
 {

@@ -2121,6 +2130,7 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
 
 	ibdev->ib_dev.query_device	= mlx4_ib_query_device;
 	ibdev->ib_dev.query_port	= mlx4_ib_query_port;
+	ibdev->ib_dev.query_transport	= mlx4_ib_query_transport;
 	ibdev->ib_dev.get_link_layer	= mlx4_ib_port_link_layer;
 	ibdev->ib_dev.query_gid		= mlx4_ib_query_gid;
 	ibdev->ib_dev.query_pkey	= mlx4_ib_query_pkey;

drivers/infiniband/hw/mlx5/main.c +7 -0

index cc4ac1e..209c796 100644
@@ -351,6 +351,12 @@ out:
 	return err;
 }
 
+static enum rdma_transport_type
+mlx5_ib_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IB;
+}
+
 static int mlx5_ib_query_gid(struct ib_device *ibdev, u8 port, int index,
 			     union ib_gid *gid)
 {

@@ -1336,6 +1342,7 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
 
 	dev->ib_dev.query_device	= mlx5_ib_query_device;
 	dev->ib_dev.query_port		= mlx5_ib_query_port;
+	dev->ib_dev.query_transport	= mlx5_ib_query_transport;
 	dev->ib_dev.query_gid		= mlx5_ib_query_gid;
 	dev->ib_dev.query_pkey		= mlx5_ib_query_pkey;
 	dev->ib_dev.modify_device	= mlx5_ib_modify_device;

drivers/infiniband/hw/mthca/mthca_provider.c +7 -0

index 415f8e1..67ac6a4 100644
@@ -179,6 +179,12 @@ static int mthca_query_port(struct ib_device *ibdev,
 	return err;
 }
 
+static enum rdma_transport_type
+mthca_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IB;
+}
+
 static int mthca_modify_device(struct ib_device *ibdev,
 			       int mask,
 			       struct ib_device_modify *props)

@@ -1281,6 +1287,7 @@ int mthca_register_device(struct mthca_dev *dev)
 	dev->ib_dev.dma_device           = &dev->pdev->dev;
 	dev->ib_dev.query_device         = mthca_query_device;
 	dev->ib_dev.query_port           = mthca_query_port;
+	dev->ib_dev.query_transport      = mthca_query_transport;
 	dev->ib_dev.modify_device        = mthca_modify_device;
 	dev->ib_dev.modify_port          = mthca_modify_port;
 	dev->ib_dev.query_pkey           = mthca_query_pkey;

drivers/infiniband/hw/nes/nes_verbs.c +6 -0

index c0d0296..8df5b61 100644
@@ -606,6 +606,11 @@ static int nes_query_port(struct ib_device *ibdev, u8 port, struct ib_port_attr
 	return 0;
 }
 
+static enum rdma_transport_type
+nes_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IWARP;
+}
 
 /**
  * nes_query_pkey

@@ -3879,6 +3884,7 @@ struct nes_ib_device *nes_init_ofa_device(struct net_device *netdev)
 	nesibdev->ibdev.dev.parent = &nesdev->pcidev->dev;
 	nesibdev->ibdev.query_device = nes_query_device;
 	nesibdev->ibdev.query_port = nes_query_port;
+	nesibdev->ibdev.query_transport = nes_query_transport;
 	nesibdev->ibdev.query_pkey = nes_query_pkey;
 	nesibdev->ibdev.query_gid = nes_query_gid;
 	nesibdev->ibdev.alloc_ucontext = nes_alloc_ucontext;

drivers/infiniband/hw/ocrdma/ocrdma_main.c +1 -0

index 7a2b59a..9f4d182 100644
@@ -244,6 +244,7 @@ static int ocrdma_register_device(struct ocrdma_dev *dev)
 	/* mandatory verbs. */
 	dev->ibdev.query_device = ocrdma_query_device;
 	dev->ibdev.query_port = ocrdma_query_port;
+	dev->ibdev.query_transport = ocrdma_query_transport;
 	dev->ibdev.modify_port = ocrdma_modify_port;
 	dev->ibdev.query_gid = ocrdma_query_gid;
 	dev->ibdev.get_link_layer = ocrdma_link_layer;

drivers/infiniband/hw/ocrdma/ocrdma_verbs.c +6 -0

index 8771755..73bace4 100644
@@ -187,6 +187,12 @@ int ocrdma_query_port(struct ib_device *ibdev,
 	return 0;
 }
 
+enum rdma_transport_type
+ocrdma_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IBOE;
+}
+
 int ocrdma_modify_port(struct ib_device *ibdev, u8 port, int mask,
 		       struct ib_port_modify *props)
 {

drivers/infiniband/hw/ocrdma/ocrdma_verbs.h +3 -0

index b8f7853..4a81b63 100644
@@ -41,6 +41,9 @@ int ocrdma_query_port(struct ib_device *, u8 port, struct ib_port_attr *props);
 int ocrdma_modify_port(struct ib_device *, u8 port, int mask,
 		       struct ib_port_modify *props);
 
+enum rdma_transport_type
+ocrdma_query_transport(struct ib_device *device, u8 port_num);
+
 void ocrdma_get_guid(struct ocrdma_dev *, u8 *guid);
 int ocrdma_query_gid(struct ib_device *, u8 port,
 		     int index, union ib_gid *gid);

drivers/infiniband/hw/qib/qib_verbs.c +7 -0

index 4a35998..caad665 100644
@@ -1650,6 +1650,12 @@ static int qib_query_port(struct ib_device *ibdev, u8 port,
 	return 0;
 }
 
+static enum rdma_transport_type
+qib_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_IB;
+}
+
 static int qib_modify_device(struct ib_device *device,
 			     int device_modify_mask,
 			     struct ib_device_modify *device_modify)

@@ -2184,6 +2190,7 @@ int qib_register_ib_device(struct qib_devdata *dd)
 	ibdev->query_device = qib_query_device;
 	ibdev->modify_device = qib_modify_device;
 	ibdev->query_port = qib_query_port;
+	ibdev->query_transport = qib_query_transport;
 	ibdev->modify_port = qib_modify_port;
 	ibdev->query_pkey = qib_query_pkey;
 	ibdev->query_gid = qib_query_gid;

drivers/infiniband/hw/usnic/usnic_ib_main.c +1 -0

index 0d0f986..03ea9f3 100644
@@ -360,6 +360,7 @@ static void *usnic_ib_device_add(struct pci_dev *dev)
 
 	us_ibdev->ib_dev.query_device = usnic_ib_query_device;
 	us_ibdev->ib_dev.query_port = usnic_ib_query_port;
+	us_ibdev->ib_dev.query_transport = usnic_ib_query_transport;
 	us_ibdev->ib_dev.query_pkey = usnic_ib_query_pkey;
 	us_ibdev->ib_dev.query_gid = usnic_ib_query_gid;
 	us_ibdev->ib_dev.get_link_layer = usnic_ib_port_link_layer;

drivers/infiniband/hw/usnic/usnic_ib_verbs.c +6 -0

index 53bd6a2..ff9a5f7 100644
@@ -348,6 +348,12 @@ int usnic_ib_query_port(struct ib_device *ibdev, u8 port,
 	return 0;
 }
 
+enum rdma_transport_type
+usnic_ib_query_transport(struct ib_device *device, u8 port_num)
+{
+	return RDMA_TRANSPORT_USNIC_UDP;
+}
+
 int usnic_ib_query_qp(struct ib_qp *qp, struct ib_qp_attr *qp_attr,
 				int qp_attr_mask,
 				struct ib_qp_init_attr *qp_init_attr)

drivers/infiniband/hw/usnic/usnic_ib_verbs.h +2 -0

index bb864f5..0b1633b 100644
@@ -27,6 +27,8 @@ int usnic_ib_query_device(struct ib_device *ibdev,
 				struct ib_device_attr *props);
 int usnic_ib_query_port(struct ib_device *ibdev, u8 port,
 				struct ib_port_attr *props);
+enum rdma_transport_type
+usnic_ib_query_transport(struct ib_device *device, u8 port_num);
 int usnic_ib_query_qp(struct ib_qp *qp, struct ib_qp_attr *qp_attr,
 				int qp_attr_mask,
 				struct ib_qp_init_attr *qp_init_attr);

include/rdma/ib_verbs.h +4 -1

index 65994a1..d54f91e 100644
@@ -75,10 +75,13 @@ enum rdma_node_type {
 };
 
 enum rdma_transport_type {
+	/* legacy for users */
 	RDMA_TRANSPORT_IB,
 	RDMA_TRANSPORT_IWARP,
 	RDMA_TRANSPORT_USNIC,
-	RDMA_TRANSPORT_USNIC_UDP
+	RDMA_TRANSPORT_USNIC_UDP,
+	/* new transport */
+	RDMA_TRANSPORT_IBOE,
 };
 
 __attribute_const__ enum rdma_transport_type

Add new callback query_transport() and implement for each HW.

Mapping List:
node-type link-layer old-transport new-transport
nes RNIC ETH IWARP IWARP
amso1100 RNIC ETH IWARP IWARP

cxgb3   	RNIC		ETH		IWARP		IWARP
cxgb4   	RNIC		ETH		IWARP		IWARP
usnic   	USNIC_UDP	ETH		USNIC_UDP	USNIC_UDP
ocrdma  	IB_CA		ETH		IB		IBOE
mlx4    	IB_CA		IB/ETH		IB		IB/IBOE
mlx5    	IB_CA		IB		IB		IB
ehca    	IB_CA		IB		IB		IB
ipath   	IB_CA		IB		IB		IB
mthca   	IB_CA		IB		IB		IB
qib     	IB_CA		IB		IB		IB

Cc Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

Cc Doug Ledford <[email protected]>

Cc Ira Weiny <[email protected]>

Cc Sean Hefty <[email protected]>

Signed-off-by Michael Wang <[email protected]>

---
 drivers/infiniband/core/device.c             |  1 +
 drivers/infiniband/core/verbs.c              |  4 +++-
 drivers/infiniband/hw/amso1100/c2_provider.c |  7 +++++++
 drivers/infiniband/hw/cxgb3/iwch_provider.c  |  7 +++++++
 drivers/infiniband/hw/cxgb4/provider.c       |  7 +++++++
 drivers/infiniband/hw/ehca/ehca_hca.c        |  6 ++++++
 drivers/infiniband/hw/ehca/ehca_iverbs.h     |  3 +++
 drivers/infiniband/hw/ehca/ehca_main.c       |  1 +
 drivers/infiniband/hw/ipath/ipath_verbs.c    |  7 +++++++
 drivers/infiniband/hw/mlx4/main.c            | 10 ++++++++++
 drivers/infiniband/hw/mlx5/main.c            |  7 +++++++
 drivers/infiniband/hw/mthca/mthca_provider.c |  7 +++++++
 drivers/infiniband/hw/nes/nes_verbs.c        |  6 ++++++
 drivers/infiniband/hw/ocrdma/ocrdma_main.c   |  1 +
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.c  |  6 ++++++
 drivers/infiniband/hw/ocrdma/ocrdma_verbs.h  |  3 +++
 drivers/infiniband/hw/qib/qib_verbs.c        |  7 +++++++
 drivers/infiniband/hw/usnic/usnic_ib_main.c  |  1 +
 drivers/infiniband/hw/usnic/usnic_ib_verbs.c |  6 ++++++
 drivers/infiniband/hw/usnic/usnic_ib_verbs.h |  2 ++
 include/rdma/ib_verbs.h                      |  7 ++++++-
 21 files changed, 104 insertions(+), 2 deletions(-)

drivers/infiniband/core/device.c +0 -0

index 18c1ece..a9587c4 100644

drivers/infiniband/core/verbs.c +1 -0

 	case RDMA_TRANSPORT_IWARP:
 	case RDMA_TRANSPORT_USNIC:
 	case RDMA_TRANSPORT_USNIC_UDP:
 		return IB_LINK_LAYER_ETHERNET;
 	default:
+		BUG();
 		return IB_LINK_LAYER_UNSPECIFIED;
 	}
 }

include/rdma/ib_verbs.h +0 -0

index 65994a1..d54f91e 100644
@@ -75,10 +75,13 @@ enum rdma_node_type {
 };
 

+/**
+ * cap_ib_mad - Check if the port of device has the capability Infiniband
+ * Management Datagrams.
+ *
+ * @device: Device to be checked
+ * @port_num: Port number of the device
+ *
+ * Return 0 when port of the device don't support Infiniband
+ * Management Datagrams.
+ */

+/**
+ * cap_ib_smi - Check if the port of device has the capability Infiniband
+ * Subnet Management Interface.
+ *
+ * @device: Device to be checked
+ * @port_num: Port number of the device
+ *
+ * Return 0 when port of the device don't support Infiniband
+ * Subnet Management Interface.
+ */

I too would like to see this as the end result, but I think it's possible to stage the changes by having the static inline calls being added convert to using these sort of attributes.

To straighten all this out, lets break management out into the two distinct types:

rdma_port_ib_fabric_mgmt() <- fabric specific management tasks: MAD, SM, multicast. The proper test for this with my bitmap above is a simple transport & RDMA_MGMT_IB test. If will be true for IB and OPA fabrics.

rdma_port_conn_mgmt() <- connection management, which we currently support everything except USNIC (correct Sean?), so a test would be something like !(transport & RDMA_TRANSPORT_USNIC). This is then split out into two subgroups, IB style and iWARP stype connection management (aka, rdma_port_iw_conn_mgmt() and rdma_port_ib_conn_mgmt()). In my above bitmap, since I didn't give IBOE its own transport type, these subgroups still boil down to the simple tests transport & iWARP and transport & IB like they do today.

Quoted 3 lines

+ * cap_ib_mad - Check if the port of device has the capability Infiniband
+ * Management Datagrams.

Patch 9/17:

Most of the other comments on this patch stand as they are. I would add the test:

rdma_port_separate_read_sge(dev, port)

{
	return dev->port[port]->transport & RDMA_SEPERATE_READ_SGE;
}

and add the helper function:

rdma_port_get_read_sge(dev, port)

{
	if (rdma_transport_is_iwarp)
		return 1;
	return dev->port[port]->max_sge;
}

As Sean pointed out, force_grh should be rdma_dev_is_iboe(). The cm

On Wed, Apr 08, 2015 at 02:29:46PM -0400, Doug Ledford wrote:

...

Quoted 7 lines

rdma_port_get_read_sge(dev, port)

{
	if (rdma_transport_is_iwarp)
		return 1;
	return dev->port[port]->max_sge;
}

Hum, that is nice, but it doesn't quite fit with how the ULP needs to work. The max limit when creating a WR is the value passed into the qp_cap, not the device maximum limit.

To do this properly we need to extend the qp_cap, and that is just too big a change. A one bit iWarp quirk is OK for now.

Quoted 2 lines

As Sean pointed out, force_grh should be rdma_dev_is_iboe(). The cm

I actually really prefer cap_mandatory_grh - that is what is going on here. ie based on that name (as a reviewer) I'd expect to see the mad layer check that the mandatory GRH is always present, or blow up.

Some of the other checks in this file revolve around pkey, I'm not sure what rocee does there? cap_pkey_supported ?

Jason

Some of the other checks in this file revolve around pkey, I'm not sure what rocee does there? cap_pkey_supported ?

Jason

On 04/08/2015 10:10 PM, Jason Gunthorpe wrote:
[snip]

Quoted 3 lines

Some of the other checks in this file revolve around pkey, I'm not sure what rocee does there? cap_pkey_supported ?

I'm not sure if this count in capability... how shall we describe it?

Quoted 7 lines · Michael Wang

On 04/08/2015 10:10 PM, Jason Gunthorpe wrote:
[snip]

Quoted 3 lines

Some of the other checks in this file revolve around pkey, I'm not sure what rocee does there? cap_pkey_supported ?

I'm not sure if this count in capability... how shall we describe it?

I'm not sure how rocee uses pkey, but maybe the the GRH and pkey thing would work well together under a single 'cap_ethernet_ah' ?

Jason

Quoted 7 lines · Doug Ledford

To straighten all this out, lets break management out into the two distinct types:

rdma_port_ib_fabric_mgmt() <- fabric specific management tasks: MAD, SM, multicast. The proper test for this with my bitmap above is a simple transport & RDMA_MGMT_IB test. If will be true for IB and OPA fabrics.

Quoted 8 lines

rdma_port_conn_mgmt() <- connection management, which we currently support everything except USNIC (correct Sean?), so a test would be something like !(transport & RDMA_TRANSPORT_USNIC). This is then split out into two subgroups, IB style and iWARP stype connection management (aka, rdma_port_iw_conn_mgmt() and rdma_port_ib_conn_mgmt()). In my above bitmap, since I didn't give IBOE its own transport type, these subgroups still boil down to the simple tests transport & iWARP and transport & IB like they do today.

There is a lot more variation here than just these two tests, and those two tests won't scale to include OPA.

IB ROCEE OPA

SMI     Y  N     Y    (though the OPA smi looked a bit different)
IB SMP  Y  N     N
OPA SMP N  N     Y
GMP     Y  Y     Y
SA      Y  N     Y
PM      Y  Y     Y    (? guessing for OPA)
CM      Y  Y     Y

GMP needs GRH N Y N

These are exactly the tests I proposed Jason. I'm not sure I see your point here. I guess my point is that although the scenario of all the different items seems complex, it really does boil down to needing only exactly what I proposed earlier to fulfill the entire test matrix.

Quoted 4 lines · Doug Ledford

These are exactly the tests I proposed Jason. I'm not sure I see your point here. I guess my point is that although the scenario of all the different items seems complex, it really does boil down to needing only exactly what I proposed earlier to fulfill the entire test matrix.

I have no problem with minimizing a bitmap, but I want the accessors to make sense first.

My specific problem with your suggestion was combining cap_ib_mad, cap_ib_sa, and cap_ib_smi into rdma_port_ib_fabric_mgmt.

Not only do the three cap things not return the same value for all situations, the documentary knowledge is lost by the reduction.

I'd prefer we look at this from a 'what do the call sites need' view, not a 'how do we minimize' view.

I've written this before: The mess here is that it is too hard to know what the call sites are actually checking for when it is some baroque conditional.

The two goals: being specific about what the test is returning and minimizing the bitmap footprint; are not necessarily opposed. One can do both at the same time.

Quoted 4 lines

I've written this before: The mess here is that it is too hard to know what the call sites are actually checking for when it is some baroque conditional.

The two goals: being specific about what the test is returning and minimizing the bitmap footprint; are not necessarily opposed. One can do both at the same time.

On 04/09/2015 11:19 PM, Doug Ledford wrote:
[snip]

Quoted 5 lines

Quoted 4 lines

I've written this before: The mess here is that it is too hard to know what the call sites are actually checking for when it is some baroque conditional.

Quoted 3 lines

The two goals: being specific about what the test is returning and minimizing the bitmap footprint; are not necessarily opposed. One can do both at the same time.

This could be internal reforming after the cap_XX() stuff works for core layer, at that time we don't need to touch core layer anymore, just introducing this bitmap stuff in verb layer, replacing the implementation of these helpers with the bitmap check, and following the semantic (description).

Quoted 7 lines · Doug Ledford

To straighten all this out, lets break management out into the two distinct types:

rdma_port_ib_fabric_mgmt() <- fabric specific management tasks: MAD, SM, multicast. The proper test for this with my bitmap above is a simple transport & RDMA_MGMT_IB test. If will be true for IB and OPA fabrics.

Quoted 8 lines

rdma_port_conn_mgmt() <- connection management, which we currently support everything except USNIC (correct Sean?), so a test would be something like !(transport & RDMA_TRANSPORT_USNIC). This is then split out into two subgroups, IB style and iWARP stype connection management (aka, rdma_port_iw_conn_mgmt() and rdma_port_ib_conn_mgmt()). In my above bitmap, since I didn't give IBOE its own transport type, these subgroups still boil down to the simple tests transport & iWARP and transport & IB like they do today.

There is a lot more variation here than just these two tests, and those two tests won't scale to include OPA.

IB ROCEE OPA
SMI Y N Y (though the OPA smi looked a bit different)

IB SMP Y N N

OPA SMP N N Y

GMP     Y  Y     Y
SA      Y  N     Y
PM      Y  Y     Y    (? guessing for OPA)

CM Y Y Y
GMP needs GRH N Y N

It may be unrealistic, but I was hoping we could largely scrub the opaque 'is spec iWARP, is spec ROCEE' kinds of tests because they don't tell anyone what it is the code cares about.

Maybe what is needed is a more precise language for the functions:

Quoted 3 lines

Quoted 3 lines

+ * cap_ib_mad - Check if the port of device has the capability Infiniband
+ * Management Datagrams.

As used this seems to mean:

True if the port can do IB/OPA SMP, or GMP management packets on QP0 or QP1. (Y Y Y) ie: Do we need the MAD layer at all.

ib_smi seems to be true if QP0 is supported (Y N Y)

Maybe the above set would make a lot more sense as:
cap_ib_qp0
cap_ib_qp1
cap_opa_qp0

ib_cm seems to mean that the CM protocol from the IBA is used on the port (Y Y Y)

ib_sa means the IBA SA protocol is supported (Y Y Y)

ib_mcast true if the IBA SA protocol is used for multicast GIDs (Y N
Y)

ipoib means the port supports the ipoib protocol (Y N ?)

This seem reasonable and understandable, even if they are currently a bit duplicating.

Quoted 10 lines

Patch 9/17:

Most of the other comments on this patch stand as they are. I would add the test:

rdma_port_separate_read_sge(dev, port)

{
	return dev->port[port]->transport & RDMA_SEPERATE_READ_SGE;
}

Quoted 8 lines

and add the helper function:

rdma_port_get_read_sge(dev, port)

{
	if (rdma_transport_is_iwarp)
		return 1;
	return dev->port[port]->max_sge;
}

Hum, that is nice, but it doesn't quite fit with how the ULP needs to work. The max limit when creating a WR is the value passed into the qp_cap, not the device maximum limit.

To do this properly we need to extend the qp_cap, and that is just too big a change. A one bit iWarp quirk is OK for now.

Quoted 2 lines

As Sean pointed out, force_grh should be rdma_dev_is_iboe(). The cm

I actually really prefer cap_mandatory_grh - that is what is going on here. ie based on that name (as a reviewer) I'd expect to see the mad layer check that the mandatory GRH is always present, or blow up.

Some of the other checks in this file revolve around pkey, I'm not sure what rocee does there? cap_pkey_supported ?

First off there are 2 separate issues here:

1) We need to communicate if a port supports or requires various management support from the ib_mad, ib_cm, and/or ib_sa modules.

2) We need to communicate how a addresses are formated and resolved for a particular port

In general I don't think we need to remove all uses of the Transport or Link Layer.

Although we may be able to remove most of the transport uses.

On Wed, Apr 08, 2015 at 02:10:15PM -0600, Jason Gunthorpe wrote:

Quoted 4 lines

Some of the other checks in this file revolve around pkey, I'm not sure what rocee does there? cap_pkey_supported ?

It seems IBoE just hardcodes the pkey to 0xffff. I don't see it used anywhere.

Function where port requires "real" PKey

	-- cma.c: cma_ib_init_qp_attr

Check rdma_port_req_pkey()?

Over all for the addressing choices:

The "Transport" (or protocol, or whatever) is Verbs. The Layer below Verbs (OPA/IB/Eth/TCP) defines how addressing, route, and connection information is generated, communicated, and used.

As Jason and Doug have been saying sometimes we want to know when that requires SA interaction or the use of the CM protocol (or neither). Other times we just need to know what the Address format or Link Layer is.

Ira

Quoted 15 lines · ira.weiny

First off there are 2 separate issues here:

1) We need to communicate if a port supports or requires various management support from the ib_mad, ib_cm, and/or ib_sa modules.

2) We need to communicate how a addresses are formated and resolved for a particular port

In general I don't think we need to remove all uses of the Transport or Link Layer.

Although we may be able to remove most of the transport uses.

On Wed, Apr 08, 2015 at 02:10:15PM -0600, Jason Gunthorpe wrote:

[snip]

Quoted 7 lines

Quoted 4 lines

Some of the other checks in this file revolve around pkey, I'm not sure what rocee does there? cap_pkey_supported ?

It seems IBoE just hardcodes the pkey to 0xffff. I don't see it used anywhere.

Quoted 5 lines

Function where port requires "real" PKey

	-- cma.c: cma_ib_init_qp_attr

Check rdma_port_req_pkey()?

What about cap_eth_ah() for all the cases need eth addressing handling?

Quoted 9 lines

Over all for the addressing choices:

The "Transport" (or protocol, or whatever) is Verbs. The Layer below Verbs (OPA/IB/Eth/TCP) defines how addressing, route, and connection information is generated, communicated, and used.

Quoted 3 lines

As Jason and Doug have been saying sometimes we want to know when that requires SA interaction or the use of the CM protocol (or neither). Other times we just need to know what the Address format or Link Layer is.

Till now it seems like we could be able to eliminate the link layer helper in core layer, but I'll reserve that helper in next version, if later we do not need it anymore, let's erase it then ;-)

Regards,
Michael Wang

Quoted 4 lines

Ira

Quoted 2 lines

IB ROCEE OPA
SMI Y N Y (though the OPA smi looked a bit different)

Yes OPA is different but it is based on the class version of the individual MADs not any particular device/port support.

Quoted 1 line

OPA SMP N N Y

How is this different from the SMI?

Quoted 4 lines

Maybe the above set would make a lot more sense as:
cap_ib_qp0
cap_ib_qp1
cap_opa_qp0

I disagree.

All we need right now is is cap_qp0. All devices currently support QP1.

Quoted 1 line

ib_sa means the IBA SA protocol is supported (Y Y Y)

I think this should be (Y N Y)

IBoE has no SA. The IBoE code "fabricates" a Path Record it does not need to interact with the SA.

Quoted 2 lines

ib_mcast true if the IBA SA protocol is used for multicast GIDs (Y N
Y)

Given the above why can't we just have the "ib_sa" flag?

However, I think checking the link layer is more appropriate here. It does not make sense to do IP over IB over Eth. Even though the IBoE can do the "IB" protocol.

Quoted 3 lines

I actually really prefer cap_mandatory_grh - that is what is going on here. ie based on that name (as a reviewer) I'd expect to see the mad layer check that the mandatory GRH is always present, or blow up.

While GRH mandatory (for the GMP) is what this is. The function ib_init_ah_from_path generically is really handling an "IBoE address" to send to and therefore we need to force the GRH in the AH.

Quoted 5 lines · ira.weiny

Quoted 2 lines

IB ROCEE OPA
SMI Y N Y (though the OPA smi looked a bit different)

Yes OPA is different but it is based on the class version of the individual MADs not any particular device/port support.

Quoted 3 lines

Quoted 1 line

OPA SMP N N Y

How is this different from the SMI?

Any code that generates SMPs and SMIs is going to need to know what format to generate them in. It seems we have a sort of weird world where IB SMPs are supported on OPA but not the IB SMI.

Not sure any users exist though..

Quoted 6 lines

Quoted 5 lines

Maybe the above set would make a lot more sense as:
cap_ib_qp0
cap_ib_qp1
cap_opa_qp0

Quoted 3 lines

I disagree.

All we need right now is is cap_qp0. All devices currently support QP1.

I didn't list iWarp in the table because everything is no, but it doesn't support QP1.

Quoted 7 lines

Quoted 2 lines

ib_sa means the IBA SA protocol is supported (Y Y Y)

I think this should be (Y N Y)

IBoE has no SA. The IBoE code "fabricates" a Path Record it does not need to interact with the SA.

I was wondering why there are so many checks in the SA code, I know RoCEE doesn't use it, but why are there there?

Quoted 5 lines

Quoted 3 lines

ib_mcast true if the IBA SA protocol is used for multicast GIDs (Y N
Y)

Given the above why can't we just have the "ib_sa" flag?

Maybe I got it wrong, but yes, if it really means 'IBA SA protocol for multicast then it can just be cap_sa.

But there is also the idea that some devices can't do multicast at all (iWarp), we must care about that at some point?

Quoted 4 lines

However, I think checking the link layer is more appropriate here. It does not make sense to do IP over IB over Eth. Even though the IBoE can do the "IB" protocol.

Yes, it is ugly.

I think if we look closely we'll find that IPoIB today has a hard requirement on cap_sa being true, so lets use that?

In fact any ULP that unconditionally uses the SA can use that.

Quoted 5 lines

Quoted 4 lines

I actually really prefer cap_mandatory_grh - that is what is going on here. ie based on that name (as a reviewer) I'd expect to see the mad layer check that the mandatory GRH is always present, or blow up.

Quoted 3 lines

While GRH mandatory (for the GMP) is what this is. The function ib_init_ah_from_path generically is really handling an "IBoE address" to send to and therefore we need to force the GRH in the AH.

This make sense to me.

It appears we have at least rocee, rocee v2 (udp?), tcp, ib and opa address and AH formats?

opa would support ib addresses too I guess.

A
bool rdma_port_addr_is_XXX()

along with a

enum AddrType rdma_port_addr_type()

Might be the thing? The latter should only be used with switch()

Isn't ocrdma an iWarp device?

Quoted 2 lines

I think if we look closely we'll find that IPoIB today has a hard requirement on cap_sa being true, so lets use that?

I don't think that is appropriate. You have been advocating that the checks be clear as to what support we need. While currently the IPoIB layer does (for IB and OPA) require an SA I think those checks are only appropriate when it is attempting an SA query.

The choice to run IPoIB at all is a different matter.

Quoted 2 lines

In fact any ULP that unconditionally uses the SA can use that.

They _can_ use that but the point of this exercise (and additional checks going forward) is that we don't "hide" meaning like this.

IPoIB should restrict itself to running on IB link layers. Should additional link layers be added which IPoIB works on then we add that check.

Quoted 8 lines · ira.weiny

Quoted 2 lines

I think if we look closely we'll find that IPoIB today has a hard requirement on cap_sa being true, so lets use that?

I don't think that is appropriate. You have been advocating that the checks be clear as to what support we need. While currently the IPoIB layer does (for IB and OPA) require an SA I think those checks are only appropriate when it is attempting an SA query.

Quoted 1 line

The choice to run IPoIB at all is a different matter.

Appropriately named or not, Jason's choice of words "has a hard requirement" is correct ;-)

Isn't ocrdma an iWarp device?

Quoted 2 lines

I was wondering why there are so many checks in the SA code, I know RoCEE doesn't use it, but why are there there?

Which checks are you referring to? I think there are separate calls to query the SA when running on IB for both the Route resolution and the Multicast join operations. The choice of those calls could be made by a "cap_sa()" helper.

Quoted 2 lines

I think if we look closely we'll find that IPoIB today has a hard requirement on cap_sa being true, so lets use that?

I don't think that is appropriate. You have been advocating that the checks be clear as to what support we need.

While currently the IPoIB layer does (for IB and OPA) require an SA I think those checks are only appropriate when it is attempting an SA query.

Quoted 2 lines

It appears we have at least rocee, rocee v2 (udp?), tcp, ib and opa address and AH formats?

Seems that way. But has the rocee v2 been accepted?

Quoted 1 line

opa would support ib addresses too I guess.

Yes opa address == ib addresses. So there is no need to distinguish them.

On Fri, Apr 10, 2015 at 01:38:38PM -0400, ira.weiny wrote:

Hiding meaning is to say 'only run on IB or OPA': WHY are we limited to those two?

We can see how this might work in future, lets say OPAv2 *requires* the 32 bit LID, for that case cap_ib_address = 0 cap_opa_address = 1. If we don't update IPoIB and it uses the tests from above then it immediately, and correctly, stops running on those OPAv2 devices.

Once patched to support cap_op_address then it will begin working again. That seems very sane..

IPoIB is more than just an ULP. It's a spec. And it's very IB specific. It will only work with OPA because OPA is imitating IB. To run it on another fabric, you would need more than just to make it work. If the new fabric doesn't have a broadcast group, or has multicast registration like IB does, you need the equivalent of IBTA, whatever that may be for this new fabric, buy in on the pre-defined multicast groups and you might need firmware support in the switches.

Quoted 7 lines

We can see how this might work in future, lets say OPAv2 *requires* the 32 bit LID, for that case cap_ib_address = 0 cap_opa_address = 1. If we don't update IPoIB and it uses the tests from above then it immediately, and correctly, stops running on those OPAv2 devices.

Once patched to support cap_op_address then it will begin working again. That seems very sane..

It is very sane from an implementation standpoint, but from the larger interoperability standpoint, you need that spec to be extended to the new fabric simultaneously.

Quoted 8 lines · Doug Ledford

IPoIB is more than just an ULP. It's a spec. And it's very IB specific. It will only work with OPA because OPA is imitating IB. To run it on another fabric, you would need more than just to make it work. If the new fabric doesn't have a broadcast group, or has multicast registration like IB does, you need the equivalent of IBTA, whatever that may be for this new fabric, buy in on the pre-defined multicast groups and you might need firmware support in the switches.

It feels like the 'cap_ib_addressing' or whatever we call it captures this very well. The IPoIB RFC is very much concerned with GID's and MGID's and broadly requires the IBA addressing
scheme. cap_ib_addressing asserts the port uses that scheme.

We wouldn't accept patches to IPoIB to add a new addressing scheme without seeing proper diligence to the standards work.

Looking away from the stadards, using cap_XX seems very sane: We are building a well defined system of invarients, You can't call into the sa functions if cap_sa is not set, you can't call into the mcast functions if cap_mcast is not set, you can't form a AH from IB GIDs/MGID/LID without cap_ib_addressing.

I makes so much sense for the ULP to directly require the needed cap's for the kernel APIs it intends to call, or not use the RDMA port at all.

Quoted 9 lines

Quoted 8 lines

We can see how this might work in future, lets say OPAv2 *requires* the 32 bit LID, for that case cap_ib_address = 0 cap_opa_address = 1. If we don't update IPoIB and it uses the tests from above then it immediately, and correctly, stops running on those OPAv2 devices.

Once patched to support cap_op_address then it will begin working again. That seems very sane..

Quoted 3 lines

It is very sane from an implementation standpoint, but from the larger interoperability standpoint, you need that spec to be extended to the new fabric simultaneously.

I liked the OPAv2 hypothetical because it doesn't actually touch the IPoIB spec. IPoIB spec has little to say about LIDs or LRHs it works entirely at the GID/MGID/GRH level.

On Fri, Apr 10, 2015 at 01:17:23PM -0600, Jason Gunthorpe wrote:
...
So trying to sum up.

Have we settled on the following "capabilities"? Helper function names aside.

/* legacy to communicate to userspace */

RDMA_LINK_LAYER_IB       = 0x0000000000000001,
RDMA_LINK_LAYER_ETH      = 0x0000000000000002,
RDMA_LINK_LAYER_MASK     = 0x000000000000000f, /* more bits? */
	/* I'm hoping we don't need more bits here */

/* legacy to communicate to userspace */

RDMA_TRANSPORT_IB        = 0x0000000000000010,
RDMA_TRANSPORT_IWARP     = 0x0000000000000020,
RDMA_TRANSPORT_USNIC     = 0x0000000000000040,
RDMA_TRANSPORT_USNIC_UDP = 0x0000000000000080,
RDMA_TRANSPORT_MASK      = 0x00000000000000f0, /* more bits? */
	/* I'm hoping we don't need more bits here */

/* New flags */

RDMA_MGMT_IB_MAD         = 0x0000000000000100, /* ib_mad module support */
RDMA_MGMT_QP0            = 0x0000000000000200, /* ib_mad QP0 support */
RDMA_MGMT_IB_SA          = 0x0000000000000400, /* ib_sa module support */
					       /* NOTE includes IB Mcast */
RDMA_MGMT_IB_CM          = 0x0000000000000800, /* ib_cm module support */
RDMA_MGMT_OPA_MAD        = 0x0000000000001000, /* ib_mad OPA MAD support */
RDMA_MGMT_MASK           = 0x00000000000fff00,

RDMA_ADDR_IB = 0x0000000000100000, /* Port does IB AH, PR, Pkey */
RDMA_ADDR_IBoE = 0x0000000000200000, /* Port does IBoE AH, PR, Pkey */
/* Do we need iWarp (TCP) here? */
RDMA_ADDR_IB_MASK = 0x000000000ff00000,

RDMA_SEPARATE_READ_SGE = 0x0000000010000000,
RDMA_QUIRKS_MASK = 0x000000fff0000000

Quoted 3 lines · ira.weiny

I don't think that is appropriate. You have been advocating that the checks be clear as to what support we need.

Right, but this is narrow, and we are not hiding meaning.

Look at the IPoIB ULP, and look at the hard requiments of the code, then translate those back to our new cap scheme. We see today's IPoIB will not run without:
- UD support
- IB addressing
- IB multicast
- IB SA
- CM (optional)

It seems perfectly correct for a ULP to say at the very start, I need all these caps, or I will not run (how could it run?). This is true of any ULP that has a hard need to use those APIs.

That would seem to be the very essance of the cap scheme. Declare what you need, not what standard you think you need.

Hiding meaning is to say 'only run on IB or OPA': WHY are we limited to those two?

Quoted 4 lines

While currently the IPoIB layer does (for IB and OPA) require an SA I think those checks are only appropriate when it is attempting an SA query.

That doesn't make any sense unless someone also adds support for handling the !SA case.

Quoted 5 lines

Quoted 3 lines

It appears we have at least rocee, rocee v2 (udp?), tcp, ib and opa address and AH formats?

Seems that way. But has the rocee v2 been accepted?

Don't know much about it yet, patches exist, it seems to have a slightly different addressing format.

Quoted 3 lines

Quoted 1 line

opa would support ib addresses too I guess.

Yes opa address == ib addresses. So there is no need to distinguish them.

The patches you sent showed a different LRH format for OPA (eg 32 bit LID), so someday we will need to know that the full 32 bit LID is available.

We can see how this might work in future, lets say OPAv2 *requires* the 32 bit LID, for that case cap_ib_address = 0 cap_opa_address = 1. If we don't update IPoIB and it uses the tests from above then it immediately, and correctly, stops running on those OPAv2 devices.

Once patched to support cap_op_address then it will begin working again. That seems very sane..

Quoted 1 line · Michael Wang

Add new callback query_transport() and implement for each HW.

My response here is going to be a long email, but that's because it's easier to respond to the various patches all in one response in order to preserve context. So, while I'm responding to patch 1 of 17, my response will cover all 17 patches in whole.

Quoted 2 lines

Mapping List:

drivers/infiniband/core/device.c static int ib_device_check_mandatory(struct ib_device *device) · line 82

Quoted 13 lines

drivers/infiniband/core/device.c +1 -0

index 18c1ece..a9587c4 100644
@@ -76,6 +76,7 @@ static int ib_device_check_mandatory(struct ib_device *device)
 	} mandatory_table[] = {
 		IB_MANDATORY_FUNC(query_device),
 		IB_MANDATORY_FUNC(query_port),
+		IB_MANDATORY_FUNC(query_transport),
 		IB_MANDATORY_FUNC(query_pkey),
 		IB_MANDATORY_FUNC(query_gid),
 		IB_MANDATORY_FUNC(alloc_pd),

I'm concerned about the performance implications of this. The size of

this patchset already points out just how many places in the code we have to check for various aspects of the device transport in order to do the right thing. Without going through the entire list to see how many are on critical hot paths, I'm sure some of them are on at least partially critical hot paths (like creation of new connections). I would prefer to see this change be implemented via a device attribute, not a functional call query. That adds a needless function call in these paths.

Quoted 9 lines

drivers/infiniband/core/verbs.c +0 -0

index f93eb8d..83370de 100644
@@ -133,14 +133,16 @@ enum rdma_link_layer rdma_port_get_link_layer(struct ib_device *device, u8 port_
 	if (device->get_link_layer)
 		return device->get_link_layer(device, port_num);
 

drivers/infiniband/core/verbs.c enum rdma_link_layer rdma_port_get_link_layer(struct ib_device *device, u8 port_ · line 139

Quoted 5 lines

drivers/infiniband/core/verbs.c +2 -1

-	switch (rdma_node_get_transport(device->node_type)) {
+	switch (device->query_transport(device, port_num)) {
 	case RDMA_TRANSPORT_IB:
+	case RDMA_TRANSPORT_IBOE:
 		return IB_LINK_LAYER_INFINIBAND;

If we are perserving ABI, then this looks wrong. Currently, IBOE returnsi transport IB and link layer Ethernet. It should not return link layer IB, it does not support IB link layer operations (such as MAD access).

Quoted 1 line

};

I'm also concerned about this. I would like to see this enum essentially turned into a bitmap. One that is constructed in such a way that we can always get the specific test we need with only one compare against the overall value. In order to do so, we need to break it down into the essential elements that are part of each of the transports. So, for instance, we can define the two link layers we have so far, plus reserve one for OPA which we know is coming:

RDMA_LINK_LAYER_IB       = 0x00000001,
RDMA_LINK_LAYER_ETH      = 0x00000002,
RDMA_LINK_LAYER_OPA      = 0x00000004,
RDMA_LINK_LAYER_MASK     = 0x0000000f,

From patch 2/17:

drivers/infiniband/core/verbs.c

Quoted 7 lines

drivers/infiniband/core/verbs.c +3 -0

+static inline int rdma_ib_mgmt(struct ib_device *device, u8 port_num)
+{
+       enum rdma_transport_type tp = device->query_transport(device,

port_num);

drivers/infiniband/core/verbs.c +3 -0

+
+       return (tp == RDMA_TRANSPORT_IB || tp == RDMA_TRANSPORT_IBOE);
+}

This looks wrong. IBOE doesn't have IB management. At least it doesn't have subnet management.

Actually, reading through the remainder of the patches, there is some serious confusion taking place here. In later patches, you use this as a surrogate for cap_cm, which implies you are talking about connection management. This is very different than the rdma_dev_ib_mgmt() test that I create above, which specifically refers to IB management tasks unique to IB/OPA: MAD, SM, multicast.

drivers/infiniband/core/verbs.c

Quoted 5 lines

drivers/infiniband/core/verbs.c +5 -0

+static inline int cap_ib_mad(struct ib_device *device, u8 port_num)
+{
+       return rdma_ib_mgmt(device, port_num);
+}
+

Why add cap_ib_mad? It's nothing more than rdma_port_ib_fabric_mgmt with a new name. Just use rdma_port_ib_fabric_mgmt() everywhere you have cap_ib_mad.

rdma_port_get_read_sge(dev, port)

{
	if (rdma_transport_is_iwarp)
		return 1;
	return dev->port[port]->max_sge;
}

Then, as Jason points out, if at some point in the future the kernel is modified to support devices with assymetrical read/write SGE sizes, this function can be modified to support those devices.

Patch 10/17:

As Sean pointed out, force_grh should be rdma_dev_is_iboe(). The cm handles iw devices, but you notice all of the functions you modify here start with ib_. The iwarp connections are funneled through iw_ specific function variants, and so even though the cm handles iwarp, ib, and roce devices, you never see anything other than ib/iboe (and opa in the future) get to the ib_ variants of the functions. So, they wrote the original tests as tests against the link layer being ethernet and used that to differentiate between ib and iboe devices. It works, but can confuse people. So, everyplace that has !rdma_transport_ib should really be rdma_dev_is_iboe instead. If we ever merge the iw_ and ib_ functions in the future, having this right will help avoid problems.

Patch 11/17:

I wouldn't reform the link_layer_show except to make it compile with the new defines I used above.

OK.

Patch 17/17:

I would drop this patch. In the future, the mlx5 driver will support both Ethernet and IB like mlx4 does, and we would just need to pull this code back to core instead of only in mlx4.

drivers/infiniband/core/device.c static int ib_device_check_mandatory(struct ib_device *device) · line 82

Quoted 12 lines

drivers/infiniband/core/device.c +1 -0

index 18c1ece..a9587c4 100644
@@ -76,6 +76,7 @@ static int ib_device_check_mandatory(struct ib_device *device)
 	} mandatory_table[] = {
 		IB_MANDATORY_FUNC(query_device),
 		IB_MANDATORY_FUNC(query_port),
+		IB_MANDATORY_FUNC(query_transport),
 		IB_MANDATORY_FUNC(query_pkey),
 		IB_MANDATORY_FUNC(query_gid),
 		IB_MANDATORY_FUNC(alloc_pd),

I'm concerned about the performance implications of this. The size of

this patchset already points out just how many places in the code we have to check for various aspects of the device transport in order to do the right thing. Without going through the entire list to see how many are on critical hot paths, I'm sure some of them are on at least partially critical hot paths (like creation of new connections). I would prefer to see this change be implemented via a device attribute, not a functional call query. That adds a needless function call in these paths.

Quoted 9 lines

drivers/infiniband/core/verbs.c +0 -0

index f93eb8d..83370de 100644
@@ -133,14 +133,16 @@ enum rdma_link_layer rdma_port_get_link_layer(struct ib_device *device, u8 port_
 	if (device->get_link_layer)
 		return device->get_link_layer(device, port_num);
 

drivers/infiniband/core/verbs.c enum rdma_link_layer rdma_port_get_link_layer(struct ib_device *device, u8 port_ · line 139

Quoted 5 lines

drivers/infiniband/core/verbs.c +2 -1

-	switch (rdma_node_get_transport(device->node_type)) {
+	switch (device->query_transport(device, port_num)) {
 	case RDMA_TRANSPORT_IB:
+	case RDMA_TRANSPORT_IBOE:
 		return IB_LINK_LAYER_INFINIBAND;

If we are perserving ABI, then this looks wrong. Currently, IBOE returnsi transport IB and link layer Ethernet. It should not return link layer IB, it does not support IB link layer operations (such as MAD access).

Quoted 10 lines

drivers/infiniband/core/verbs.c +1 -0

 	case RDMA_TRANSPORT_IWARP:
 	case RDMA_TRANSPORT_USNIC:
 	case RDMA_TRANSPORT_USNIC_UDP:
 		return IB_LINK_LAYER_ETHERNET;
 	default:
+		BUG();
 		return IB_LINK_LAYER_UNSPECIFIED;
 	}
 }

[ snip ]

Quoted 7 lines

include/rdma/ib_verbs.h +0 -0

index 65994a1..d54f91e 100644
@@ -75,10 +75,13 @@ enum rdma_node_type {
 };
 

include/rdma/ib_verbs.h enum rdma_node_type { · line 85

Quoted 10 lines

include/rdma/ib_verbs.h +4 -1

 enum rdma_transport_type {
+	/* legacy for users */
 	RDMA_TRANSPORT_IB,
 	RDMA_TRANSPORT_IWARP,
 	RDMA_TRANSPORT_USNIC,
-	RDMA_TRANSPORT_USNIC_UDP
+	RDMA_TRANSPORT_USNIC_UDP,
+	/* new transport */
+	RDMA_TRANSPORT_IBOE,
 };

I'm also concerned about this. I would like to see this enum essentially turned into a bitmap. One that is constructed in such a way that we can always get the specific test we need with only one compare against the overall value. In order to do so, we need to break it down into the essential elements that are part of each of the transports. So, for instance, we can define the two link layers we have so far, plus reserve one for OPA which we know is coming:

RDMA_LINK_LAYER_IB       = 0x00000001,
RDMA_LINK_LAYER_ETH      = 0x00000002,
RDMA_LINK_LAYER_OPA      = 0x00000004,
RDMA_LINK_LAYER_MASK     = 0x0000000f,

We can then define the currently known high level transport types:

RDMA_TRANSPORT_IB        = 0x00000010,
RDMA_TRANSPORT_IWARP     = 0x00000020,
RDMA_TRANSPORT_USNIC     = 0x00000040,

RDMA_TRANSPORT_USNIC_UDP = 0x00000080,
RDMA_TRANSPORT_MASK = 0x000000f0,

We could then define bits for the IB management types:

RDMA_MGMT_IB             = 0x00000100,
RDMA_MGMT_OPA            = 0x00000200,
RDMA_MGMT_MASK           = 0x00000f00,

Then we have space to define specific quirks:

RDMA_SEPARATE_READ_SGE = 0x00001000,
RDMA_QUIRKS_MASK = 0xfffff000

Once those are defined, a few definitions for device drivers to use when they initialize a device to set the bitmap to the right values:

#define IS_IWARP (RDMA_LINK_LAYER_ETH | RDMA_TRANSPORT_IWARP | RDMA_SEPARATE_READ_SGE)
#define IS_IB (RDMA_LINK_LAYER_IB | RDMA_TRANSPORT_IB | RDMA_MGMT_IB) #define IS_IBOE (RDMA_LINK_LAYER_ETH | RDMA_TRANSPORT_IB) #define IS_USNIC (RDMA_LINK_LAYER_ETH | RDMA_TRANSPORT_USNIC) #define IS_OPA (RDMA_LINK_LAYER_OPA | RDMA_TRANSPORT_IB | RDMA_MGMT_IB | RDMA_MGMT_OPA)

Then you need to define the tests:

static inline bool rdma_transport_is_iwarp(struct ib_device *dev, u8 port)

{
	return dev->port[port]->transport & RDMA_TRANSPORT_IWARP;
}

/* Note: this intentionally covers IB, IBOE, and OPA...use
rdma_dev_is_ib if you want to only get physical IB devices */
static inline bool
rdma_transport_is_ib(struct ibdev *dev)

{
	return dev->port[port]->transport & RDMA_TRANSPORT_IB;
}

rdma_port_is_ib(struct ib_device *dev, u8 port)

{
	return dev->port[port]->transport & RDMA_LINK_LAYER_IB;
}

rdma_port_is_iboe(struct ib_device *dev, u8 port)

{
	return dev->port[port]->transport & IS_IBOE == IS_IBOE;
}

rdma_port_is_usnic(struct ib_device *dev, u8 port)

{
	return dev->port[port]->transport & RDMA_TRANSPORT_USNIC;
}

rdma_port_is_opa(struct ib_device *dev, u8 port)

{
	return dev->port[port]->transport & RDMA_LINK_LAYER_OPA;
}

rdma_port_is_iwarp(struct ib_device *dev, u8 port)

{
	return rdma_transport_is_iwarp(dev, port);

}

rdma_port_ib_fabric_mgmt(struct ibdev *dev, u8 port)

{
	return dev->port[port]->transport & RDMA_MGMT_IB;

}

rdma_port_opa_mgmt(struct ibdev *dev, u8 port)

{
	return dev->port[port]->transport & RDMA_MGMT_OPA;
}

Other things can be changed too. Like rdma_port_get_link_layer can become this:

{
	return dev->transport & RDMA_LINK_LAYER_MASK;
}

From patch 2/17:

include/rdma/ib_verbs.h

Quoted 7 lines

include/rdma/ib_verbs.h +3 -0

+static inline int rdma_ib_mgmt(struct ib_device *device, u8 port_num)
+{
+       enum rdma_transport_type tp = device->query_transport(device,

port_num);

include/rdma/ib_verbs.h +3 -0

+
+       return (tp == RDMA_TRANSPORT_IB || tp == RDMA_TRANSPORT_IBOE);
+}

This looks wrong. IBOE doesn't have IB management. At least it doesn't have subnet management.

Actually, reading through the remainder of the patches, there is some serious confusion taking place here. In later patches, you use this as a surrogate for cap_cm, which implies you are talking about connection management. This is very different than the rdma_dev_ib_mgmt() test that I create above, which specifically refers to IB management tasks unique to IB/OPA: MAD, SM, multicast.

The kernel connection management code is not really limited. It supports IB, IBOE, iWARP, and in the future it will support OPA. There are some places in the CM code were we test for just IB/IBOE currently, but that's only because we split iWARP out higher up in the abstraction hierarchy. So, calling something rdma_ib_mgmt and meaning a rather specialized tested in the CM is probably misleading.

To straighten all this out, lets break management out into the two distinct types:

rdma_port_ib_fabric_mgmt() <- fabric specific management tasks: MAD, SM, multicast. The proper test for this with my bitmap above is a simple transport & RDMA_MGMT_IB test. If will be true for IB and OPA fabrics.

rdma_port_conn_mgmt() <- connection management, which we currently support everything except USNIC (correct Sean?), so a test would be something like !(transport & RDMA_TRANSPORT_USNIC). This is then split out into two subgroups, IB style and iWARP stype connection management (aka, rdma_port_iw_conn_mgmt() and rdma_port_ib_conn_mgmt()). In my above bitmap, since I didn't give IBOE its own transport type, these subgroups still boil down to the simple tests transport & iWARP and transport & IB like they do today.

From patch 3/17:

Quoted 12 lines

+/**
+ * cap_ib_mad - Check if the port of device has the capability Infiniband
+ * Management Datagrams.
+ *
+ * @device: Device to be checked
+ * @port_num: Port number of the device
+ *
+ * Return 0 when port of the device don't support Infiniband
+ * Management Datagrams.
+ */
+static inline int cap_ib_mad(struct ib_device *device, u8 port_num)

include/rdma/ib_verbs.h

Quoted 4 lines

include/rdma/ib_verbs.h +4 -0

+{
+       return rdma_ib_mgmt(device, port_num);
+}
+

Why add cap_ib_mad? It's nothing more than rdma_port_ib_fabric_mgmt with a new name. Just use rdma_port_ib_fabric_mgmt() everywhere you have cap_ib_mad.

From patch 4/17:

Quoted 11 lines

+/**
+ * cap_ib_smi - Check if the port of device has the capability Infiniband
+ * Subnet Management Interface.
+ *
+ * @device: Device to be checked
+ * @port_num: Port number of the device
+ *
+ * Return 0 when port of the device don't support Infiniband
+ * Subnet Management Interface.
+ */

include/rdma/ib_verbs.h

Quoted 5 lines

include/rdma/ib_verbs.h +5 -0

+static inline int cap_ib_smi(struct ib_device *device, u8 port_num)
+{
+       return rdma_transport_ib(device, port_num);
+}
+

Same as the previous patch. This is needless indirection. Just use rdma_port_ib_fabric_mgmt directly.

Patch 5/17:

Again, just use rdma_port_ib_conn_mgmt() directly.

Patch 6/17:

Again, just use rdma_port_ib_fabric_mgmt() directly.

Patch 7/17:

Again, just use rdma_port_ib_fabric_mgmt() directly. It's perfectly applicable to the IB mcast registration requirements.

Patch 8/17:

Here we can create a new test if we are using the bitmap I created above:

rdma_port_ipoib(struct ib_device *dev, u8 port)

{
	return !(dev->port[port]->transport & RDMA_LINK_LAYER_ETH);

}

This is presuming that OPA will need ipoib devices. This will cause all non-Ethernet link layer devices to return true, and right now, that is all IB and all OPA devices.

Patch 9/17:

Most of the other comments on this patch stand as they are. I would add the test:

rdma_port_separate_read_sge(dev, port)

{
	return dev->port[port]->transport & RDMA_SEPERATE_READ_SGE;
}

and add the helper function:

rdma_port_get_read_sge(dev, port)

{
	if (rdma_transport_is_iwarp)
		return 1;
	return dev->port[port]->max_sge;
}

Then, as Jason points out, if at some point in the future the kernel is modified to support devices with assymetrical read/write SGE sizes, this function can be modified to support those devices.

Patch 10/17:

As Sean pointed out, force_grh should be rdma_dev_is_iboe(). The cm handles iw devices, but you notice all of the functions you modify here start with ib_. The iwarp connections are funneled through iw_ specific function variants, and so even though the cm handles iwarp, ib, and roce devices, you never see anything other than ib/iboe (and opa in the future) get to the ib_ variants of the functions. So, they wrote the original tests as tests against the link layer being ethernet and used that to differentiate between ib and iboe devices. It works, but can confuse people. So, everyplace that has !rdma_transport_ib should really be rdma_dev_is_iboe instead. If we ever merge the iw_ and ib_ functions in the future, having this right will help avoid problems.

Patch 11/17:

I wouldn't reform the link_layer_show except to make it compile with the new defines I used above.

Patch 12/17:

Go ahead and add a helper to check all ports on a dev, just make it rdma_hca_ib_conn_mgmt() and have it loop through rdma_port_ib_conn_mgmt() return codes.

Patch 13/17:

This patch is largely unneeded if we reworked the bitmap like I have above. A lot of the changes you made to switch from case statements to multiple if statements can go back to being case statements because in the bitmap IB and IBOE are still both transport IB, so you just do the case on the transport bits and not on the link layer bits.

Patch 14/17:

Seems ok.

Patch 15/17:

If you implement the bitmap like I list above, then this code will need fixed up to use the bitmap. Otherwise it looks OK.

Patch 16/17:

OK.

Patch 17/17:

I would drop this patch. In the future, the mlx5 driver will support both Ethernet and IB like mlx4 does, and we would just need to pull this code back to core instead of only in mlx4.

Quoted 14 lines

drivers/infiniband/core/device.c static int ib_device_check_mandatory(struct ib_device *device) · line 82

Quoted 12 lines

drivers/infiniband/core/device.c +1 -0

index 18c1ece..a9587c4 100644
@@ -76,6 +76,7 @@ static int ib_device_check_mandatory(struct ib_device *device)
 	} mandatory_table[] = {
 		IB_MANDATORY_FUNC(query_device),
 		IB_MANDATORY_FUNC(query_port),
+		IB_MANDATORY_FUNC(query_transport),
 		IB_MANDATORY_FUNC(query_pkey),
 		IB_MANDATORY_FUNC(query_gid),
 		IB_MANDATORY_FUNC(alloc_pd),

I'm concerned about the performance implications of this. The size of

Quoted 8 lines

this patchset already points out just how many places in the code we have to check for various aspects of the device transport in order to do the right thing. Without going through the entire list to see how many are on critical hot paths, I'm sure some of them are on at least partially critical hot paths (like creation of new connections). I would prefer to see this change be implemented via a device attribute, not a functional call query. That adds a needless function call in these paths.

I like the idea of a query_transport but at the same time would like to see the use of "transport" reduced. A reduction in the use of this call could eliminate most performance concerns.

So can we keep this abstraction if at the end of the series we limit its use?

Quoted 9 lines

Quoted 9 lines

drivers/infiniband/core/verbs.c +0 -0

index f93eb8d..83370de 100644
@@ -133,14 +133,16 @@ enum rdma_link_layer rdma_port_get_link_layer(struct ib_device *device, u8 port_
 	if (device->get_link_layer)
 		return device->get_link_layer(device, port_num);
 

Quoted 10 lines

drivers/infiniband/core/verbs.c enum rdma_link_layer rdma_port_get_link_layer(struct ib_device *device, u8 port_ · line 139

Quoted 5 lines

drivers/infiniband/core/verbs.c +2 -1

-	switch (rdma_node_get_transport(device->node_type)) {
+	switch (device->query_transport(device, port_num)) {
 	case RDMA_TRANSPORT_IB:
+	case RDMA_TRANSPORT_IBOE:
 		return IB_LINK_LAYER_INFINIBAND;

If we are perserving ABI, then this looks wrong. Currently, IBOE returnsi transport IB and link layer Ethernet. It should not return link layer IB, it does not support IB link layer operations (such as MAD access).

I think the original code has the bug.

IBoE devices currently return a transport of IB but they probably never get here because they support the get_link_layer callback used a few lines above. So this "bug" was probably never hit.

Quoted 39 lines

Quoted 10 lines

drivers/infiniband/core/verbs.c +1 -0

 	case RDMA_TRANSPORT_IWARP:
 	case RDMA_TRANSPORT_USNIC:
 	case RDMA_TRANSPORT_USNIC_UDP:
 		return IB_LINK_LAYER_ETHERNET;
 	default:
+		BUG();
 		return IB_LINK_LAYER_UNSPECIFIED;
 	}
 }

[ snip ]

Quoted 7 lines

include/rdma/ib_verbs.h +0 -0

index 65994a1..d54f91e 100644
@@ -75,10 +75,13 @@ enum rdma_node_type {
 };
 

include/rdma/ib_verbs.h enum rdma_node_type { · line 85

Quoted 10 lines

include/rdma/ib_verbs.h +4 -1

 enum rdma_transport_type {
+	/* legacy for users */
 	RDMA_TRANSPORT_IB,
 	RDMA_TRANSPORT_IWARP,
 	RDMA_TRANSPORT_USNIC,
-	RDMA_TRANSPORT_USNIC_UDP
+	RDMA_TRANSPORT_USNIC_UDP,
+	/* new transport */
+	RDMA_TRANSPORT_IBOE,
 };

I'm also concerned about this. I would like to see this enum essentially turned into a bitmap. One that is constructed in such a way that we can always get the specific test we need with only one compare against the overall value. In order to do so, we need to break it down into the essential elements that are part of each of the transports. So, for instance, we can define the two link layers we have so far, plus reserve one for OPA which we know is coming:

Quoted 4 lines

RDMA_LINK_LAYER_IB       = 0x00000001,
RDMA_LINK_LAYER_ETH      = 0x00000002,
RDMA_LINK_LAYER_OPA      = 0x00000004,
RDMA_LINK_LAYER_MASK     = 0x0000000f,

I would reserve more bits here.

Quoted 8 lines

We can then define the currently known high level transport types:

RDMA_TRANSPORT_IB        = 0x00000010,
RDMA_TRANSPORT_IWARP     = 0x00000020,
RDMA_TRANSPORT_USNIC     = 0x00000040,

RDMA_TRANSPORT_USNIC_UDP = 0x00000080,
RDMA_TRANSPORT_MASK = 0x000000f0,

I would reserve more bits here.

Quoted 6 lines

We could then define bits for the IB management types:

RDMA_MGMT_IB             = 0x00000100,
RDMA_MGMT_OPA            = 0x00000200,
RDMA_MGMT_MASK           = 0x00000f00,

We at least need bits for SA / CM support.

I said previously all device types support QP1 I was wrong... I forgot about USNIC devices. So the full management bit mask is.

RDMA_MGMT_IB_MAD         = 0x00000100,
RDMA_MGMT_QP0            = 0x00000200,
RDMA_MGMT_SA             = 0x00000400,
RDMA_MGMT_CM             = 0x00000800,
RDMA_MGMT_OPA_MAD        = 0x00001000,
RDMA_MGMT_MASK           = 0x000fff00,

With a couple of spares.

The MAD stack is pretty agnostic to the types of MADs passing through it so we don't really need PM flags etc.

Quoted 5 lines

Then we have space to define specific quirks:

RDMA_SEPARATE_READ_SGE = 0x00001000,
RDMA_QUIRKS_MASK = 0xfffff000

shift for spares...

RDMA_SEPARATE_READ_SGE = 0x00100000,
RDMA_QUIRKS_MASK = 0xfff00000

Quoted 28 lines

Once those are defined, a few definitions for device drivers to use when they initialize a device to set the bitmap to the right values:

#define IS_IWARP (RDMA_LINK_LAYER_ETH | RDMA_TRANSPORT_IWARP | RDMA_SEPARATE_READ_SGE)
#define IS_IB (RDMA_LINK_LAYER_IB | RDMA_TRANSPORT_IB | RDMA_MGMT_IB) #define IS_IBOE (RDMA_LINK_LAYER_ETH | RDMA_TRANSPORT_IB) #define IS_USNIC (RDMA_LINK_LAYER_ETH | RDMA_TRANSPORT_USNIC) #define IS_OPA (RDMA_LINK_LAYER_OPA | RDMA_TRANSPORT_IB | RDMA_MGMT_IB | RDMA_MGMT_OPA)

Then you need to define the tests:

static inline bool rdma_transport_is_iwarp(struct ib_device *dev, u8 port)

{
	return dev->port[port]->transport & RDMA_TRANSPORT_IWARP;
}

/* Note: this intentionally covers IB, IBOE, and OPA...use
rdma_dev_is_ib if you want to only get physical IB devices */
static inline bool
rdma_transport_is_ib(struct ibdev *dev)

{
	return dev->port[port]->transport & RDMA_TRANSPORT_IB;
}

Quoted 1 line

rdma_port_is_ib(struct ib_device *dev, u8 port)

I prefer

rdma_port_link_layer_is_ib
rdma_port_link_layer_is_eth

Quoted 5 lines

{
	return dev->port[port]->transport & RDMA_LINK_LAYER_IB;
}

Quoted 1 line

rdma_port_is_iboe(struct ib_device *dev, u8 port)

I'm not sure what this means.

rdma_port_req_iboe_addr seems more appropriate because what we really need to know is that this device requires an IBoE address format. (PKey is "fake", PathRecord is fabricated rather than queried, GRH is required in the AH conversion.)

Quoted 6 lines

{
	return dev->port[port]->transport & IS_IBOE == IS_IBOE;
}

rdma_port_is_usnic(struct ib_device *dev, u8 port)

rdma_transport_is_usnic

Quoted 5 lines

{
	return dev->port[port]->transport & RDMA_TRANSPORT_USNIC;
}

rdma_port_is_opa(struct ib_device *dev, u8 port)

rdma_port_link_layer_is_opa

Quoted 4 lines

{
	return dev->port[port]->transport & RDMA_LINK_LAYER_OPA;
}

Quoted 3 lines

rdma_port_is_iwarp(struct ib_device *dev, u8 port)

{
	return rdma_transport_is_iwarp(dev, port);

Why not call rdma_transport_is_iwarp?

Quoted 8 lines

Patch 8/17:

Here we can create a new test if we are using the bitmap I created above:

rdma_port_ipoib(struct ib_device *dev, u8 port)

{
	return !(dev->port[port]->transport & RDMA_LINK_LAYER_ETH);

I would prefer a link layer (or generic) bit mask rather than '&' of "transport" and "link layer". That just seems wrong.

I guess rdma_dev_is_iboe is ok. But it seems like we are keying off the addresses not necessarily the devices.

documented. I get why you link the address variant, because it pops out all the things that are special about IBoE addressing and calls out that the issues need to be handled. However, saying requires_iboe_addr(), while foreshadowing the work that needs done, doesn't actually document the work that needs done. Whether we call is dev_is_iboe() or requires_iboe_addr(), it would be good if the documentation spelled out those specific requirements for reference sake.

Quoted 7 lines · Doug Ledford

documented. I get why you link the address variant, because it pops out all the things that are special about IBoE addressing and calls out that the issues need to be handled. However, saying requires_iboe_addr(), while foreshadowing the work that needs done, doesn't actually document the work that needs done. Whether we call is dev_is_iboe() or requires_iboe_addr(), it would be good if the documentation spelled out those specific requirements for reference sake.

My deep hope for this, was that the test 'requires_iboe_addr' or whatever we call it would have a *really good* kdoc.

List all the ways iboe_addr's work, how they differ from IB addresses, refer to the specs people should read to understand it, etc.

The patches don't do this, and maybe Michael is the wrong person to fill that in, but we can get it done..

Jason

BTW: Michael, next time you post the series, please trim the CC list...

As per my above statement, rdma_transport* tests were testing the high level transport type, rdma_port* types were testing link layers. iWARP has an Eth link layer, so technically port_is_iwarp makes no sense. But since all the other types had a check too, I included port_is_iwarp just to be complete, and if you are going to ask if a specific port is iwarp as a link layer, it makes sense to say yes if the transport is iwarp, not if the link layer is eth.

Quoted 7 lines · Doug Ledford

As per my above statement, rdma_transport* tests were testing the high level transport type, rdma_port* types were testing link layers. iWARP has an Eth link layer, so technically port_is_iwarp makes no sense. But since all the other types had a check too, I included port_is_iwarp just to be complete, and if you are going to ask if a specific port is iwarp as a link layer, it makes sense to say yes if the transport is iwarp, not if the link layer is eth.

Not wanting to split hairs, but I would not rule out the possibility of a future device supporting iWARP on one port and another RDMA protocol on another. One could also imagine softiWARP and softROCE co-existing atop a single ethernet NIC.

So, I disagree that port_is_iwarp() is a nonsequitur.

include/rdma/ib_verbs.h int ib_query_port(struct ib_device *device, · line 1761

Quoted 13 lines

+static inline int rdma_transport_ib(struct ib_device *device, u8 port_num)

include/rdma/ib_verbs.h +6 -0

+{
+	return device->query_transport(device, port_num)
+			== RDMA_TRANSPORT_IB;
+}
+
+static inline int rdma_transport_iboe(struct ib_device *device, u8

port_num)

include/rdma/ib_verbs.h +4 -0

+{
+	return device->query_transport(device, port_num)
+			== RDMA_TRANSPORT_IBOE;
+}

We need to do something with the function names to make their use more obvious. Both IB and IBoE have transport IB. I think Jason suggested rdma_tech_ib / rdma_tech_iboe.

Regarding transport types, I believe that usnic supports 2 different transports. Although usnic isn't used by anything else in the core layer, we should probably be able to handle a device that supports multiple protocols. I'm not sure what the 'transport' should be for iWarp, since iWarp is layered over TCP. But that may just mean that the term transport isn't great.

- Sean

drivers/infiniband/core/mad.c static void ib_mad_init_device(struct ib_device *device) · line 3071

Quoted 13 lines · Michael Wang

drivers/infiniband/core/mad.c +3 -3

-	if (rdma_node_get_transport(device->node_type) != RDMA_TRANSPORT_IB)
-		return;
-
 	if (device->node_type == RDMA_NODE_IB_SWITCH) {
 		start = 0;
 		end   = 0;

@@ -3069,6 +3066,9 @@ static void ib_mad_init_device(struct ib_device *device)
 	}
 
 	for (i = start; i <= end; i++) {
+		if (!cap_ib_mad(device, i))
+			continue;
+

I would prefer to see these changes in control flow as dedicated patches, at the top of your patch stack.

For this kind of work a patch should be mechanical changes only, it is easier to review that way.

Same comment applies throughout.

Jason