Skip to contentSkip to conversation

mlx4: Add support for IBoE - address resolution

PATCH In review Eli Cohen 3 participants 15 messages Aug 26, 2010 – Oct 22, 2010 6 messages not in the archive

Subsystems (4)
lore
Share & tools
lore

Eli Cohen

author
#0 lore raw

PATCH v10 8/12 mlx4: Add support for IBoE - address resolution

The following patch handles address vectors creation for IBoE ports. mlx4 needs the MAC address of the remote node to include it in the WQE of a UD QP or in the QP context of connected QPs. Address resolution is done atomically in the case of a link local address or a multicast GID and otherwise -EINVAL is returned. mlx4 transport packets were changed too to accommodate for IBoE. Multicast groups attach/detach calls dev_mc_add/remove to update the NIC's multicast filters.Since attaching a QP to a multicast group does not require the QP to be in a state different then INIT - this is fine for IB. For IBoE however, we need the port assigned to the QP in order to call dev_mc_add() for the correct netdevice, while port is assigned when moving from INIT to RTR. Hence, we must keep track of all the multicast groups attached to a QP and call dev_mc_add() when the port becomes available.

---
Changes form v9:
Cosmentic change to a conditon that checks the link layer.

 drivers/infiniband/hw/mlx4/ah.c      |  161 +++++++++---
 drivers/infiniband/hw/mlx4/mad.c     |   32 ++-
 drivers/infiniband/hw/mlx4/main.c    |  474 +++++++++++++++++++++++++++++++---
 drivers/infiniband/hw/mlx4/mlx4_ib.h |   32 +++-
 drivers/infiniband/hw/mlx4/qp.c      |  140 ++++++++--
 drivers/net/mlx4/en_port.c           |    4 +-
 drivers/net/mlx4/en_port.h           |    3 +-
 drivers/net/mlx4/fw.c                |    3 +-
 include/linux/mlx4/cmd.h             |    1 +
 include/linux/mlx4/device.h          |   30 ++-
 include/linux/mlx4/qp.h              |    7 +-
 11 files changed, 771 insertions(+), 116 deletions(-)
drivers/infiniband/hw/mlx4/ah.c +128 -33
index 11a236f..57d99d2 100644
@@ -33,63 +33,157 @@
 #include <linux/slab.h>
 
 #include "mlx4_ib.h"
+#include <rdma/ib_addr.h>
+#include <linux/inet.h>
+#include <linux/string.h>
 
-struct ib_ah *mlx4_ib_create_ah(struct ib_pd *pd, struct ib_ah_attr *ah_attr)
+int mlx4_ib_resolve_grh(struct mlx4_ib_dev *dev, const struct ib_ah_attr *ah_attr,
+			u8 *mac, int *is_mcast, u8 port)
 {
-	struct mlx4_dev *dev = to_mdev(pd->device)->dev;
-	struct mlx4_ib_ah *ah;
+	struct mlx4_ib_iboe *iboe = &dev->iboe;
+	struct in6_addr in6;
 
-	ah = kmalloc(sizeof *ah, GFP_ATOMIC);
-	if (!ah)
-		return ERR_PTR(-ENOMEM);
+	*is_mcast = 0;
+	spin_lock(&iboe->lock);
+	if (!iboe->netdevs[port - 1]) {
+		spin_unlock(&iboe->lock);
+		return -EINVAL;
+	}
+	spin_unlock(&iboe->lock);
 
-	memset(&ah->av, 0, sizeof ah->av);
+	memcpy(&in6, ah_attr->grh.dgid.raw, sizeof in6);
+	if (rdma_link_local_addr(&in6))
+		rdma_get_ll_mac(&in6, mac);
+	else if (rdma_is_multicast_addr(&in6)) {
+		rdma_get_mcast_mac(&in6, mac);
+		*is_mcast = 1;
+	} else
+		return -EINVAL;
 
-	ah->av.port_pd = cpu_to_be32(to_mpd(pd)->pdn | (ah_attr->port_num << 24));
-	ah->av.g_slid  = ah_attr->src_path_bits;
-	ah->av.dlid    = cpu_to_be16(ah_attr->dlid);
-	if (ah_attr->static_rate) {
-		ah->av.stat_rate = ah_attr->static_rate + MLX4_STAT_RATE_OFFSET;
-		while (ah->av.stat_rate > IB_RATE_2_5_GBPS + MLX4_STAT_RATE_OFFSET &&
-		       !(1 << ah->av.stat_rate & dev->caps.stat_rate_support))
-			--ah->av.stat_rate;
-	}
-	ah->av.sl_tclass_flowlabel = cpu_to_be32(ah_attr->sl << 28);
+	return 0;
+}
+
+static struct ib_ah *create_ib_ah(struct ib_pd *pd, struct ib_ah_attr *ah_attr,
+				  struct mlx4_ib_ah *ah)
+{
+	struct mlx4_dev *dev = to_mdev(pd->device)->dev;
+
+	ah->av.ib.port_pd = cpu_to_be32(to_mpd(pd)->pdn | (ah_attr->port_num << 24));
+	ah->av.ib.g_slid  = ah_attr->src_path_bits;
 	if (ah_attr->ah_flags & IB_AH_GRH) {
-		ah->av.g_slid   |= 0x80;
-		ah->av.gid_index = ah_attr->grh.sgid_index;
-		ah->av.hop_limit = ah_attr->grh.hop_limit;
-		ah->av.sl_tclass_flowlabel |=
+		ah->av.ib.g_slid   |= 0x80;
+		ah->av.ib.gid_index = ah_attr->grh.sgid_index;
+		ah->av.ib.hop_limit = ah_attr->grh.hop_limit;
+		ah->av.ib.sl_tclass_flowlabel |=
 			cpu_to_be32((ah_attr->grh.traffic_class << 20) |
 				    ah_attr->grh.flow_label);
-		memcpy(ah->av.dgid, ah_attr->grh.dgid.raw, 16);
+		memcpy(ah->av.ib.dgid, ah_attr->grh.dgid.raw, 16);
+	}
+
+	ah->av.ib.dlid    = cpu_to_be16(ah_attr->dlid);
+	if (ah_attr->static_rate) {
+		ah->av.ib.stat_rate = ah_attr->static_rate + MLX4_STAT_RATE_OFFSET;
+		while (ah->av.ib.stat_rate > IB_RATE_2_5_GBPS + MLX4_STAT_RATE_OFFSET &&
+		       !(1 << ah->av.ib.stat_rate & dev->caps.stat_rate_support))
+			--ah->av.ib.stat_rate;
 	}
+	ah->av.ib.sl_tclass_flowlabel = cpu_to_be32(ah_attr->sl << 28);
 
 	return &ah->ibah;
 }
 
+static struct ib_ah *create_iboe_ah(struct ib_pd *pd, struct ib_ah_attr *ah_attr,
+				    struct mlx4_ib_ah *ah)
+{
+	struct mlx4_ib_dev *ibdev = to_mdev(pd->device);
+	struct mlx4_dev *dev = ibdev->dev;
+	u8 mac[6];
+	int err;
+	int is_mcast;
+
+	err = mlx4_ib_resolve_grh(ibdev, ah_attr, mac, &is_mcast, ah_attr->port_num);
+	if (err)
+		return ERR_PTR(err);
+
+	memcpy(ah->av.eth.mac, mac, 6);
+	ah->av.eth.port_pd = cpu_to_be32(to_mpd(pd)->pdn | (ah_attr->port_num << 24));
+	ah->av.eth.gid_index = ah_attr->grh.sgid_index;
+	if (ah_attr->static_rate) {
+		ah->av.eth.stat_rate = ah_attr->static_rate + MLX4_STAT_RATE_OFFSET;
+		while (ah->av.eth.stat_rate > IB_RATE_2_5_GBPS + MLX4_STAT_RATE_OFFSET &&
+		       !(1 << ah->av.eth.stat_rate & dev->caps.stat_rate_support))
+			--ah->av.eth.stat_rate;
+	}
+
+	/*
+	 * HW requires multicast LID so we just choose one.
+	 */
+	if (is_mcast)
+		ah->av.ib.dlid = cpu_to_be16(0xc000);
+
+	memcpy(ah->av.eth.dgid, ah_attr->grh.dgid.raw, 16);
+	ah->av.eth.sl_tclass_flowlabel = cpu_to_be32(ah_attr->sl << 28);
+
+	return &ah->ibah;
+}
+
+struct ib_ah *mlx4_ib_create_ah(struct ib_pd *pd, struct ib_ah_attr *ah_attr)
+{
+	struct mlx4_ib_ah *ah;
+	struct ib_ah *ret;
+
+	ah = kzalloc(sizeof *ah, GFP_ATOMIC);
+	if (!ah)
+		return ERR_PTR(-ENOMEM);
+
+	if (rdma_port_get_link_layer(pd->device, ah_attr->port_num) == IB_LINK_LAYER_ETHERNET) {
+		if (!(ah_attr->ah_flags & IB_AH_GRH)) {
+			ret = ERR_PTR(-EINVAL);
+			goto out;
+		} else {
+			/* TBD: need to handle the case when we get called
+			in an atomic context and there we might sleep. We
+			don't expect this currently since we're working with
+			link local addresses which we can translate without
+			going to sleep */
+			ret = create_iboe_ah(pd, ah_attr, ah);
+			if (IS_ERR(ret))
+				goto out;
+			else
+				return ret;
+		}
+	} else
+		return create_ib_ah(pd, ah_attr, ah); /* never fails */
+
+out:
+	kfree(ah);
+	return ret;
+}
+
 int mlx4_ib_query_ah(struct ib_ah *ibah, struct ib_ah_attr *ah_attr)
 {
 	struct mlx4_ib_ah *ah = to_mah(ibah);
+	enum rdma_link_layer ll;
 
 	memset(ah_attr, 0, sizeof *ah_attr);
-	ah_attr->dlid	       = be16_to_cpu(ah->av.dlid);
-	ah_attr->sl	       = be32_to_cpu(ah->av.sl_tclass_flowlabel) >> 28;
-	ah_attr->port_num      = be32_to_cpu(ah->av.port_pd) >> 24;
-	if (ah->av.stat_rate)
-		ah_attr->static_rate = ah->av.stat_rate - MLX4_STAT_RATE_OFFSET;
-	ah_attr->src_path_bits = ah->av.g_slid & 0x7F;
+	ah_attr->sl = be32_to_cpu(ah->av.ib.sl_tclass_flowlabel) >> 28;
+	ah_attr->port_num = be32_to_cpu(ah->av.ib.port_pd) >> 24;
+	ll = rdma_port_get_link_layer(ibah->device, ah_attr->port_num);
+	ah_attr->dlid = ll == IB_LINK_LAYER_INFINIBAND ? be16_to_cpu(ah->av.ib.dlid) : 0;
+	if (ah->av.ib.stat_rate)
+		ah_attr->static_rate = ah->av.ib.stat_rate - MLX4_STAT_RATE_OFFSET;
+	ah_attr->src_path_bits = ah->av.ib.g_slid & 0x7F;
 
 	if (mlx4_ib_ah_grh_present(ah)) {
 		ah_attr->ah_flags = IB_AH_GRH;
 
 		ah_attr->grh.traffic_class =
-			be32_to_cpu(ah->av.sl_tclass_flowlabel) >> 20;
+			be32_to_cpu(ah->av.ib.sl_tclass_flowlabel) >> 20;
 		ah_attr->grh.flow_label =
-			be32_to_cpu(ah->av.sl_tclass_flowlabel) & 0xfffff;
-		ah_attr->grh.hop_limit  = ah->av.hop_limit;
-		ah_attr->grh.sgid_index = ah->av.gid_index;
-		memcpy(ah_attr->grh.dgid.raw, ah->av.dgid, 16);
+			be32_to_cpu(ah->av.ib.sl_tclass_flowlabel) & 0xfffff;
+		ah_attr->grh.hop_limit  = ah->av.ib.hop_limit;
+		ah_attr->grh.sgid_index = ah->av.ib.gid_index;
+		memcpy(ah_attr->grh.dgid.raw, ah->av.ib.dgid, 16);
 	}
 
 	return 0;
@@ -100,3 +194,4 @@ int mlx4_ib_destroy_ah(struct ib_ah *ah)
 	kfree(to_mah(ah));
 	return 0;
 }
+
drivers/infiniband/hw/mlx4/mad.c +20 -12
index f38d5b1..c9a8dd6 100644
@@ -311,19 +311,25 @@ int mlx4_ib_mad_init(struct mlx4_ib_dev *dev)
 	struct ib_mad_agent *agent;
 	int p, q;
 	int ret;
+	enum rdma_link_layer ll;
 
-	for (p = 0; p < dev->num_ports; ++p)
+	for (p = 0; p < dev->num_ports; ++p) {
+		ll = rdma_port_get_link_layer(&dev->ib_dev, p + 1);
 		for (q = 0; q <= 1; ++q) {
-			agent = ib_register_mad_agent(&dev->ib_dev, p + 1,
-						      q ? IB_QPT_GSI : IB_QPT_SMI,
-						      NULL, 0, send_handler,
-						      NULL, NULL);
-			if (IS_ERR(agent)) {
-				ret = PTR_ERR(agent);
-				goto err;
-			}
-			dev->send_agent[p][q] = agent;
+			if (ll == IB_LINK_LAYER_INFINIBAND) {
+				agent = ib_register_mad_agent(&dev->ib_dev, p + 1,
+							      q ? IB_QPT_GSI : IB_QPT_SMI,
+							      NULL, 0, send_handler,
+							      NULL, NULL);
+				if (IS_ERR(agent)) {
+					ret = PTR_ERR(agent);
+					goto err;
+				}
+				dev->send_agent[p][q] = agent;
+			} else
+				dev->send_agent[p][q] = NULL;
 		}
+	}
 
 	return 0;
 
@@ -344,8 +350,10 @@ void mlx4_ib_mad_cleanup(struct mlx4_ib_dev *dev)
 	for (p = 0; p < dev->num_ports; ++p) {
 		for (q = 0; q <= 1; ++q) {
 			agent = dev->send_agent[p][q];
-			dev->send_agent[p][q] = NULL;
-			ib_unregister_mad_agent(agent);
+			if (agent) {
+				dev->send_agent[p][q] = NULL;
+				ib_unregister_mad_agent(agent);
+			}
 		}
 
 		if (dev->sm_ah[p])
drivers/infiniband/hw/mlx4/main.c +440 -34

611 lines not on this page · raw message

drivers/infiniband/hw/mlx4/mlx4_ib.h +30 -2
index 3486d76..382a898 100644
@@ -112,6 +112,13 @@ enum mlx4_ib_qp_flags {
 	MLX4_IB_QP_BLOCK_MULTICAST_LOOPBACK	= 1 << 1,
 };
 
+struct gid_entry {
+	struct list_head	list;
+	union ib_gid		gid;
+	int			added;
+	u8			port;
+};
+
 struct mlx4_ib_qp {
 	struct ib_qp		ibqp;
 	struct mlx4_qp		mqp;
@@ -138,6 +145,8 @@ struct mlx4_ib_qp {
 	u8			resp_depth;
 	u8			sq_no_prefetch;
 	u8			state;
+	int			mlx_type;
+	struct list_head	gid_list;
 };
 
 struct mlx4_ib_srq {
@@ -157,7 +166,14 @@ struct mlx4_ib_srq {
 
 struct mlx4_ib_ah {
 	struct ib_ah		ibah;
-	struct mlx4_av		av;
+	union mlx4_ext_av       av;
+};
+
+struct mlx4_ib_iboe {
+	spinlock_t		lock;
+	struct net_device      *netdevs[MLX4_MAX_PORTS];
+	struct notifier_block 	nb;
+	union ib_gid		gid_table[MLX4_MAX_PORTS][128];
 };
 
 struct mlx4_ib_dev {
@@ -176,6 +192,7 @@ struct mlx4_ib_dev {
 
 	struct mutex		cap_mask_mutex;
 	bool			ib_active;
+	struct mlx4_ib_iboe	iboe;
 };
 
 static inline struct mlx4_ib_dev *to_mdev(struct ib_device *ibdev)
@@ -314,9 +331,20 @@ int mlx4_ib_map_phys_fmr(struct ib_fmr *ibfmr, u64 *page_list, int npages,
 int mlx4_ib_unmap_fmr(struct list_head *fmr_list);
 int mlx4_ib_fmr_dealloc(struct ib_fmr *fmr);
 
+int mlx4_ib_resolve_grh(struct mlx4_ib_dev *dev, const struct ib_ah_attr *ah_attr,
+			u8 *mac, int *is_mcast, u8 port);
+
 static inline int mlx4_ib_ah_grh_present(struct mlx4_ib_ah *ah)
 {
-	return !!(ah->av.g_slid & 0x80);
+	u8 port = be32_to_cpu(ah->av.ib.port_pd) >> 24 & 3;
+
+	if (rdma_port_get_link_layer(ah->ibah.device, port) == IB_LINK_LAYER_ETHERNET)
+		return 1;
+
+	return !!(ah->av.ib.g_slid & 0x80);
 }
 
+int mlx4_ib_add_mc(struct mlx4_ib_dev *mdev, struct mlx4_ib_qp *mqp,
+		   union ib_gid *gid);
+
 #endif /* MLX4_IB_H */
drivers/infiniband/hw/mlx4/qp.c +112 -28

255 lines not on this page · raw message

drivers/net/mlx4/en_port.c +2 -2
index a29abe8..a249887 100644
@@ -127,8 +127,8 @@ int mlx4_SET_PORT_qpn_calc(struct mlx4_dev *dev, u8 port, u32 base_qpn,
 	memset(context, 0, sizeof *context);
 
 	context->base_qpn = cpu_to_be32(base_qpn);
-	context->promisc = cpu_to_be32(promisc << SET_PORT_PROMISC_SHIFT | base_qpn);
-	context->mcast = cpu_to_be32(1 << SET_PORT_PROMISC_SHIFT | base_qpn);
+	context->promisc = cpu_to_be32(promisc << SET_PORT_PROMISC_EN_SHIFT | base_qpn);
+	context->mcast = cpu_to_be32(1 << SET_PORT_PROMISC_MODE_SHIFT | base_qpn);
 	context->intra_no_vlan = 0;
 	context->no_vlan = MLX4_NO_VLAN_IDX;
 	context->intra_vlan_miss = 0;
drivers/net/mlx4/en_port.h +2 -1
index e6477f1..9354891 100644
@@ -36,7 +36,8 @@
 
 
 #define SET_PORT_GEN_ALL_VALID	0x7
-#define SET_PORT_PROMISC_SHIFT	31
+#define SET_PORT_PROMISC_EN_SHIFT	31
+#define SET_PORT_PROMISC_MODE_SHIFT	30
 
 enum {
 	MLX4_CMD_SET_VLAN_FLTR  = 0x47,
drivers/net/mlx4/fw.c +2 -1
index 04f42ae..5b3593d 100644
@@ -98,7 +98,8 @@ static void dump_dev_cap_flags(struct mlx4_dev *dev, u32 flags)
 		[20] = "Address vector port checking support",
 		[21] = "UD multicast support",
 		[24] = "Demand paging support",
-		[25] = "Router support"
+		[25] = "Router support",
+		[30] = "IBoE support"
 	};
 	int i;
 
include/linux/mlx4/cmd.h +1 -0
index 0f82293..22bd8d3 100644
@@ -140,6 +140,7 @@ enum {
 	MLX4_SET_PORT_MAC_TABLE = 0x2,
 	MLX4_SET_PORT_VLAN_TABLE = 0x3,
 	MLX4_SET_PORT_PRIO_MAP  = 0x4,
+	MLX4_SET_PORT_GID_TABLE = 0x5,
 };
 
 struct mlx4_dev;
include/linux/mlx4/device.h +29 -1
index 7a7f9c1..ca5645c 100644
@@ -67,7 +67,8 @@ enum {
 	MLX4_DEV_CAP_FLAG_ATOMIC	= 1 << 18,
 	MLX4_DEV_CAP_FLAG_RAW_MCAST	= 1 << 19,
 	MLX4_DEV_CAP_FLAG_UD_AV_PORT	= 1 << 20,
-	MLX4_DEV_CAP_FLAG_UD_MCAST	= 1 << 21
+	MLX4_DEV_CAP_FLAG_UD_MCAST	= 1 << 21,
+	MLX4_DEV_CAP_FLAG_IBOE		= 1 << 30
 };
 
 enum {
@@ -373,6 +374,27 @@ struct mlx4_av {
 	u8			dgid[16];
 };
 
+struct mlx4_eth_av {
+	__be32		port_pd;
+	u8		reserved1;
+	u8		smac_idx;
+	u16		reserved2;
+	u8		reserved3;
+	u8		gid_index;
+	u8		stat_rate;
+	u8		hop_limit;
+	__be32		sl_tclass_flowlabel;
+	u8		dgid[16];
+	u32		reserved4[2];
+	__be16		vlan;
+	u8		mac[6];
+};
+
+union mlx4_ext_av {
+	struct mlx4_av		ib;
+	struct mlx4_eth_av	eth;
+};
+
 struct mlx4_dev {
 	struct pci_dev	       *pdev;
 	unsigned long		flags;
@@ -401,6 +423,12 @@ struct mlx4_init_port_param {
 		if (((type) == MLX4_PORT_TYPE_IB ? (dev)->caps.port_mask : \
 		     ~(dev)->caps.port_mask) & 1 << ((port) - 1))
 
+#define mlx4_foreach_ib_transport_port(port, dev)			\
+	for ((port) = 1; (port) <= (dev)->caps.num_ports; (port)++)	\
+		if (((dev)->caps.port_mask & 1 << ((port) - 1)) ||	\
+		    ((dev)->caps.flags & MLX4_DEV_CAP_FLAG_IBOE))
+
+
 int mlx4_buf_alloc(struct mlx4_dev *dev, int size, int max_direct,
 		   struct mlx4_buf *buf);
 void mlx4_buf_free(struct mlx4_dev *dev, int size, struct mlx4_buf *buf);
include/linux/mlx4/qp.h +5 -2
index 7abe643..97cfdc8 100644
@@ -112,7 +112,8 @@ struct mlx4_qp_path {
 	u8			snooper_flags;
 	u8			reserved3[2];
 	u8			counter_index;
-	u8			reserved4[7];
+	u8			reserved4;
+	u8			dmac[6];
 };
 
 struct mlx4_qp_context {
@@ -166,6 +167,7 @@ enum {
 	MLX4_WQE_CTRL_TCP_UDP_CSUM	= 1 << 5,
 	MLX4_WQE_CTRL_INS_VLAN		= 1 << 6,
 	MLX4_WQE_CTRL_STRONG_ORDER	= 1 << 7,
+	MLX4_WQE_CTRL_FORCE_LOOPBACK	= 1 << 0,
 };
 
 struct mlx4_wqe_ctrl_seg {
@@ -219,7 +221,8 @@ struct mlx4_wqe_datagram_seg {
 	__be32			av[8];
 	__be32			dqpn;
 	__be32			qkey;
-	__be32			reservd[2];
+	__be16			vlan;
+	u8			mac[6];
 };
 
 struct mlx4_wqe_lso_seg {
Signature · 6 lines
1.7.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#1 lore raw

Just curious -- what's up with this change here? Is this connected to IBoE support, or is this an independent fix?

Quoted 20 lines
drivers/net/mlx4/en_port.c +2 -2
index a29abe8..a249887 100644
@@ -127,8 +127,8 @@ int mlx4_SET_PORT_qpn_calc(struct mlx4_dev *dev, u8 port, u32 base_qpn,
 	memset(context, 0, sizeof *context);
 
 	context->base_qpn = cpu_to_be32(base_qpn);
-	context->promisc = cpu_to_be32(promisc << SET_PORT_PROMISC_SHIFT | base_qpn);
-	context->mcast = cpu_to_be32(1 << SET_PORT_PROMISC_SHIFT | base_qpn);
+	context->promisc = cpu_to_be32(promisc << SET_PORT_PROMISC_EN_SHIFT | base_qpn);
+	context->mcast = cpu_to_be32(1 << SET_PORT_PROMISC_MODE_SHIFT | base_qpn);
 	context->intra_no_vlan = 0;
 	context->no_vlan = MLX4_NO_VLAN_IDX;
 	context->intra_vlan_miss = 0;
drivers/net/mlx4/en_port.h +0 -0
index e6477f1..9354891 100644

drivers/net/mlx4/en_port.h line 43

Quoted 9 lines
drivers/net/mlx4/en_port.h +2 -1
@@ -36,7 +36,8 @@
 
 
 #define SET_PORT_GEN_ALL_VALID	0x7
-#define SET_PORT_PROMISC_SHIFT	31
+#define SET_PORT_PROMISC_EN_SHIFT	31
+#define SET_PORT_PROMISC_MODE_SHIFT	30
 
 enum {
 	MLX4_CMD_SET_VLAN_FLTR  = 0x47,

Also as far as I can tell this variable sgid is write-only (ie you do the ib_get_cached_gid() but then never do anything with the value that that returns). Am I missing something subtle?

Quoted 9 lines
drivers/net/mlx4/en_port.h +4 -0
@@ -1226,43 +1281,59 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr,
 	int header_size;
 	int spc;
 	int i;
+	union ib_gid sgid;
+	int is_eth;
+	int is_grh;
+	int err;
 

drivers/net/mlx4/en_port.h static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr, · line 1297

Quoted 10 lines
drivers/net/mlx4/en_port.h +4 -1
 	send_size = 0;
 	for (i = 0; i < wr->num_sge; ++i)
 		send_size += wr->sg_list[i].length;
 
-	ib_ud_header_init(send_size, 1, 0, mlx4_ib_ah_grh_present(ah), 0, &sqp->ud_header);
+	is_eth = rdma_port_get_link_layer(sqp->qp.ibqp.device, sqp->qp.port) == IB_LINK_LAYER_ETHERNET;
+	is_grh = mlx4_ib_ah_grh_present(ah);
+	err = ib_get_cached_gid(ib_dev, be32_to_cpu(ah->av.ib.port_pd) >> 24,
+				ah->av.ib.gid_index, &sgid);
 

Finally this patch would have been easier to review without extraneous whitespace noise like

Quoted 10 lines
drivers/net/mlx4/en_port.h +1 -1
@@ -1337,7 +1419,7 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr,
 	}
 
 	*mlx_seg_len =
-		ALIGN(i * sizeof (struct mlx4_wqe_inline_seg) + header_size, 16);
+	ALIGN(i * sizeof (struct mlx4_wqe_inline_seg) + header_size, 16);
 	return 0;
 }
 

drivers/net/mlx4/en_port.h int mlx4_ib_destroy_ah(struct ib_ah *ah) · line 197

Quoted 4 lines
drivers/net/mlx4/en_port.h +1 -0
@@ -100,3 +194,4 @@ int mlx4_ib_destroy_ah(struct ib_ah *ah)
 	kfree(to_mah(ah));
 	return 0;
 }
+

etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#3 lore raw
Quoted 2 lines · Roland Dreier

Just curious -- what's up with this change here? Is this connected to IBoE support, or is this an independent fix?

It is required for proper distribution of IBoE multicast packets while still not hurting the mlx4_en driver's operation.

Quoted 4 lines

Also as far as I can tell this variable sgid is write-only (ie you do the ib_get_cached_gid() but then never do anything with the value that that returns). Am I missing something subtle?

It is not required in the context of this patch. It should have been part of patch 12/12 which adds VLAN support, in which case it is used to extract the required VLAN ID.
Roland, would you like me to send patches 8-12 again with this fixed?

Quoted 4 lines

Finally this patch would have been easier to review without extraneous whitespace noise like

Sorry about that. I pass them through checkpatch but this must have been hiden within the "line over ..." messages.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 5 lines
Quoted 2 lines

Just curious -- what's up with this change here? Is this connected to IBoE support, or is this an independent fix?

It is required for proper distribution of IBoE multicast packets while still not hurting the mlx4_en driver's operation.

OK, I split this into a separate patch for clarity.

Quoted 5 lines
Quoted 2 lines

Finally this patch would have been easier to review without extraneous whitespace noise like

Sorry about that. I pass them through checkpatch but this must have been hiden within the "line over ..." messages.

It's not a checkpatch issue. It's just that you made random formatting changes to parts of the code that weren't otherwise touched.

Quoted 1 line

Roland, would you like me to send patches 8-12 again with this fixed?

I think I can still handle it.

However I notice one other thing that looks like a bug.

You add a function that starts:

int mlx4_ib_resolve_grh(struct mlx4_ib_dev *dev, const struct ib_ah_attr *ah_attr,
u8 *mac, int *is_mcast, u8 port)

{
	struct mlx4_ib_iboe *iboe = &dev->iboe;
	struct in6_addr in6;

	*is_mcast = 0;
	spin_lock(&iboe->lock);

which is called from create_iboe_ah() which is called from mlx4_ib_create_ah(), which can be called from both interrupt and process context. So as far as I can tell, this spin_lock() needs to actually do spin_lock_irqsave()?

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#6 lore raw
Quoted 9 lines · Roland Dreier

int mlx4_ib_resolve_grh(struct mlx4_ib_dev *dev, const struct ib_ah_attr *ah_attr,
u8 *mac, int *is_mcast, u8 port)

{
	struct mlx4_ib_iboe *iboe = &dev->iboe;
	struct in6_addr in6;

	*is_mcast = 0;
	spin_lock(&iboe->lock);
Quoted 5 lines

which is called from create_iboe_ah() which is called from mlx4_ib_create_ah(), which can be called from both interrupt and process context. So as far as I can tell, this spin_lock() needs to actually do spin_lock_irqsave()?

This was added in the past when I wanted to use the pointer to the net device to resolve any IPv6 address. Since we only resolve link local addresses, why don't we just remove the spinlocks and the check inside it?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw
Quoted 4 lines

This was added in the past when I wanted to use the pointer to the net device to resolve any IPv6 address. Since we only resolve link local addresses, why don't we just remove the spinlocks and the check inside it?

OK, easy enough.

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#8 lore raw
In reply to Eli Cohen

Also I don't see anywhere that the global functions

drivers/infiniband/hw/mlx4/main.c static int mlx4_ib_query_port(struct ib_device *ibdev, u8 port, · line 220

Quoted 2 lines
drivers/infiniband/hw/mlx4/main.c +2 -0
+int eth_to_ib_width(int w)
+int eth_to_ib_speed(int s)

are called?

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#10 lore raw
Quoted 7 lines · Roland Dreier

Also I don't see anywhere that the global functions

drivers/infiniband/hw/mlx4/main.c static int mlx4_ib_query_port(struct ib_device *ibdev, u8 port, · line 220

Quoted 2 lines
drivers/infiniband/hw/mlx4/main.c +2 -0
+int eth_to_ib_width(int w)
+int eth_to_ib_speed(int s)

are called?

Right. Let's remove them.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#11 lore raw
In reply to Eli Cohen

drivers/infiniband/hw/mlx4/qp.c enum { · line 62

Quoted 7 lines

enum {

drivers/infiniband/hw/mlx4/qp.c +2 -1
 	/*
 	 * Largest possible UD header: send with GRH and immediate data.
+	 * 4 bytes added to accommodate for eth header instead of lrh
 	 */
-	MLX4_IB_UD_HEADER_SIZE		= 72,
+	MLX4_IB_UD_HEADER_SIZE		= 76,

I don't understand this change either. As far as I can tell, a 14-byte Ethernet header is 6 bytes longer than an 8-byte LRH, and with .1q the 18-byte header will require 10 bytes more than the LRH. So shouldn't this value be 78 in this patch and updated to 82 in the VLAN patch?

(It probably works in practice because the allocation of sqp structs gets rounded up enough)

- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#13 lore raw
Quoted 2 lines

(It probably works in practice because the allocation of sqp structs gets rounded up enough)

And I guess also no one actually ends up sending MADs with immediate data, so that saves 4 bytes off the worst case too...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#15 lore raw
Quoted 5 lines · Roland Dreier
Quoted 2 lines

(It probably works in practice because the allocation of sqp structs gets rounded up enough)

And I guess also no one actually ends up sending MADs with immediate data, so that saves 4 bytes off the worst case too...

I don't get this. For IB, we have 72 without immediate - did you forget to add the ICRC? I think we count it with the header... --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#16 lore raw
Quoted 2 lines

I don't get this. For IB, we have 72 without immediate - did you forget to add the ICRC? I think we count it with the header...

We don't need ICRC in the header_buf, do we? We're talking about MLX4_IB_UD_HEADER_SIZE, which is used to hold the packet header we get from ib_ud_header_pack(). The ICRC is accounted for in a different segment of the WQE.

In any case, it shouldn't matter -- I don't see how it could be correct to increase the buffer size by 4 bytes to account for the change from LRH to Ethernet header; an LRH is 8 bytes and an Ethernet header (without .1q etc) is 14 bytes. So we should add 6 bytes, right?

The math I get is

IB: LRH + GRH + BTH + DETH + IMM

       8    40    12     8      4   = 72

Eth: MAC + GRH + BTH + DETH + IMM

      14    40    12     8      4   = 78

I don't see how your change

drivers/infiniband/hw/mlx4/qp.c enum { · line 62

Quoted 2 lines
drivers/infiniband/hw/mlx4/qp.c +1 -1
-	MLX4_IB_UD_HEADER_SIZE		= 72,
+	MLX4_IB_UD_HEADER_SIZE		= 76,

could possibly be correct, although as I said it probably works because of padding and not hitting the worst case in practice anyway.

- R.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#18 lore raw
Quoted 18 lines · Roland Dreier

We don't need ICRC in the header_buf, do we? We're talking about MLX4_IB_UD_HEADER_SIZE, which is used to hold the packet header we get from ib_ud_header_pack(). The ICRC is accounted for in a different segment of the WQE.

In any case, it shouldn't matter -- I don't see how it could be correct to increase the buffer size by 4 bytes to account for the change from LRH to Ethernet header; an LRH is 8 bytes and an Ethernet header (without .1q etc) is 14 bytes. So we should add 6 bytes, right?

The math I get is

IB: LRH + GRH + BTH + DETH + IMM

       8    40    12     8      4   = 72

Eth: MAC + GRH + BTH + DETH + IMM

      14    40    12     8      4   = 78
Quoted 8 lines

I don't see how your change

Review

Quoted 2 lines
(quoted diff) +1 -1
-	MLX4_IB_UD_HEADER_SIZE		= 72,
+	MLX4_IB_UD_HEADER_SIZE		= 76,

could possibly be correct, although as I said it probably works because of padding and not hitting the worst case in practice anyway.

Sure, all clear.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#19 lore raw
In reply to a message not in the archive
Quoted 13 lines · Roland Dreier

drivers/infiniband/hw/mlx4/qp.c enum { · line 62

Quoted 7 lines

enum {

drivers/infiniband/hw/mlx4/qp.c +2 -1
 	/*
 	 * Largest possible UD header: send with GRH and immediate data.
+	 * 4 bytes added to accommodate for eth header instead of lrh
 	 */
-	MLX4_IB_UD_HEADER_SIZE		= 72,
+	MLX4_IB_UD_HEADER_SIZE		= 76,

I don't understand this change either. As far as I can tell, a 14-byte Ethernet header is 6 bytes longer than an 8-byte LRH, and with .1q the 18-byte header will require 10 bytes more than the LRH. So shouldn't this value be 78 in this patch and updated to 82 in the VLAN patch?

Quoted 3 lines

(It probably works in practice because the allocation of sqp structs gets rounded up enough)

Agree, I made a wrong calculation.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#20 lore raw
In reply to Eli Cohen
Quoted 5 lines · Eli Cohen

[...] Address resolution is done atomically in the
case of a link local address or a multicast GID and otherwise -EINVAL is returned.  mlx4 transport packets were changed too to accommodate for IBoE. Multicast groups attach/detach calls dev_mc_add/remove to update the NIC's multicast filters.

This change log and also I assume the patch as well, deals alot with multicast, however, patch 0/10 says "With these patches, IBoE multicast frames may be broadcast as there is
currently no use of a L2 multicast group membership protocol." - does this means some/much of the code added/changed by this patch is dead code or not needed at this point?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html