RE: regression: nvme rdma with bnxt_re0 broken

From: Parav Pandit <parav@mellanox.com>
To: Selvin Xavier <selvin.xavier@broadcom.com>
Cc: Yi Zhang <yi.zhang@redhat.com>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	Daniel Jurgens <danielj@mellanox.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	Devesh Sharma <devesh.sharma@broadcom.com>
Subject: RE: regression: nvme rdma with bnxt_re0 broken
Date: Fri, 12 Jul 2019 09:49:42 +0000	[thread overview]
Message-ID: <AM0PR05MB4866665D5CACB34AE885BCA2D1F20@AM0PR05MB4866.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <AM0PR05MB4866CCD487C9D99BD9526BA8D1F20@AM0PR05MB4866.eurprd05.prod.outlook.com>


> > Hi Selvin,
> >
> > > -----Original Message-----
> > > From: Selvin Xavier <selvin.xavier@broadcom.com>
> > > Sent: Friday, July 12, 2019 9:16 AM
> > > To: Parav Pandit <parav@mellanox.com>
> > > Cc: Yi Zhang <yi.zhang@redhat.com>; linux-nvme@lists.infradead.org;
> > > Daniel Jurgens <danielj@mellanox.com>; linux-rdma@vger.kernel.org;
> > > Devesh Sharma <devesh.sharma@broadcom.com>
> > > Subject: Re: regression: nvme rdma with bnxt_re0 broken
> > >
> > > On Fri, Jul 12, 2019 at 8:19 AM Parav Pandit <parav@mellanox.com>
> wrote:
> > > >
> > > > GID table looks fine.
> > > >
> > > The GID table has  fe80:0000:0000:0000:020a:f7ff:fee3:6e32 entry
> > > repeated 6 times. 2 for each interface bnxt_roce, bnxt_roce.43 and
> > > bnxt_roce.45. Is this expected to have same gid entries for vlan and
> > > base interfaces? As you mentioned earlier, driver's assumption that
> > > only 2 GID entries identical (one for RoCE v1 and one for RoCE
> > > v2)   is breaking here.
> > >
> > Yes, this is correct behavior. Each vlan netdev interface is in
> > different L2 segment.
> > Vlan netdev has this ipv6 link local address. Hence, it is added to the GID
> table.
> > A given GID table entry is identified uniquely by GID+ndev+type(v1/v2).
> >
> > Reviewing bnxt_qplib_add_sgid() does the comparison below.
> > if (!memcmp(&sgid_tbl->tbl[i], gid, sizeof(*gid))) {
> >
> > This comparison looks incomplete which ignore netdev and type.
> > But even with that, I would expect GID entry addition failure for
> > vlans for ipv6 link local entries.
> >
> > But I am puzzled now, that , with above memcmp() check, how does both
> > v1 and v2 entries get added in your table and for vlans too?
> > I expect add_gid() and core/cache.c add_roce_gid () to fail for the
> > duplicate entry.
> > But GID table that Yi Zhang dumped has these vlan entries.
> > I am missing something.
> >
> Ah, found it.
> bnxt_re_add_gid() checks for -EALREADY and returns 0 to add_gid() callback.
> Ok. so you just need to extend bnxt_qplib_add_sgid() for considering vlan too.
> Let me see if I can share a patch in few minutes.
> 
> > Yi Zhang,
> > Instead of last 15 lines of dmesg, can you please share the whole dmsg
> > log which should be enabled before creating vlans.
> > using
> > echo -n "module ib_core +p" /sys/kernel/debug/dynamic_debug/control
> >
> > Selvin,
> > Additionally, driver shouldn't be looking at the duplicate entries.
> > core already does it.
> > You might only want to do for v1/v2 case as bnxt driver has some
> > dependency with it.
> > Can you please fix this part?
> >

How about below fix?

From f3f17008d34b5a0c38c190010281a3030a8e5771 Mon Sep 17 00:00:00 2001
From: Parav Pandit <parav@mellanox.com>
Date: Fri, 12 Jul 2019 04:34:52 -0500
Subject: [PATCH] IB/bnxt_re: Honor vlan_id in GID entry comparision

GID entry consist of GID, vlan, netdev and smac.
Extend GID duplicate check comparions to consider vlan_id as well
to support IPv6 VLAN based link local addresses.

Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs")
Change-Id: I2e026ec8065c8425ba24fad8525323d112a2f4e4
Signed-off-by: Parav Pandit <parav@mellanox.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_sp.c | 4 +++-
 drivers/infiniband/hw/bnxt_re/qplib_sp.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_sp.c b/drivers/infiniband/hw/bnxt_re/qplib_sp.c
index 48793d3512ac..8567b7367669 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_sp.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_sp.c
@@ -296,7 +296,8 @@ int bnxt_qplib_add_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl,
 	}
 	free_idx = sgid_tbl->max;
 	for (i = 0; i < sgid_tbl->max; i++) {
-		if (!memcmp(&sgid_tbl->tbl[i], gid, sizeof(*gid))) {
+		if (!memcmp(&sgid_tbl->tbl[i], gid, sizeof(*gid)) &&
+		    sgid_tbl->tbl[i].vlan_id == vlan_id) {
 			dev_dbg(&res->pdev->dev,
 				"SGID entry already exist in entry %d!\n", i);
 			*index = i;
@@ -351,6 +352,7 @@ int bnxt_qplib_add_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl,
 	}
 	/* Add GID to the sgid_tbl */
 	memcpy(&sgid_tbl->tbl[free_idx], gid, sizeof(*gid));
+	sgid_tbl->tbl[free_idx].vlan_id = vlan_id;
 	sgid_tbl->active++;
 	if (vlan_id != 0xFFFF)
 		sgid_tbl->vlan[free_idx] = 1;
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_sp.h b/drivers/infiniband/hw/bnxt_re/qplib_sp.h
index 0ec3b12b0bcd..7a1957c9dc6f 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_sp.h
+++ b/drivers/infiniband/hw/bnxt_re/qplib_sp.h
@@ -82,6 +82,7 @@ struct bnxt_qplib_pd {
 
 struct bnxt_qplib_gid {
 	u8				data[16];
+	u16 vlan_id;
 };
 
 struct bnxt_qplib_ah {
-- 
2.19.2
