GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com Signed-off-by: Selvin Xavier selvin.xavier@broadcom.com --- drivers/infiniband/hw/bnxt_re/ib_verbs.c | 7 +++++-- drivers/infiniband/hw/bnxt_re/qplib_res.c | 13 +++++++++---- drivers/infiniband/hw/bnxt_re/qplib_res.h | 2 +- drivers/infiniband/hw/bnxt_re/qplib_sp.c | 14 +++++++++----- drivers/infiniband/hw/bnxt_re/qplib_sp.h | 7 ++++++- 5 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c index 2c3685faa57a..a4a9f90f2482 100644 --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c @@ -308,6 +308,7 @@ int bnxt_re_del_gid(const struct ib_gid_attr *attr, void **context) struct bnxt_re_dev *rdev = to_bnxt_re_dev(attr->device, ibdev); struct bnxt_qplib_sgid_tbl *sgid_tbl = &rdev->qplib_res.sgid_tbl; struct bnxt_qplib_gid *gid_to_del; + u16 vlan_id = 0xFFFF;
/* Delete the entry from the hardware */ ctx = *context; @@ -317,7 +318,8 @@ int bnxt_re_del_gid(const struct ib_gid_attr *attr, void **context) if (sgid_tbl && sgid_tbl->active) { if (ctx->idx >= sgid_tbl->max) return -EINVAL; - gid_to_del = &sgid_tbl->tbl[ctx->idx]; + gid_to_del = &sgid_tbl->tbl[ctx->idx].gid; + vlan_id = sgid_tbl->tbl[ctx->idx].vlan_id; /* DEL_GID is called in WQ context(netdevice_event_work_handler) * or via the ib_unregister_device path. In the former case QP1 * may not be destroyed yet, in which case just return as FW @@ -335,7 +337,8 @@ int bnxt_re_del_gid(const struct ib_gid_attr *attr, void **context) } ctx->refcnt--; if (!ctx->refcnt) { - rc = bnxt_qplib_del_sgid(sgid_tbl, gid_to_del, true); + rc = bnxt_qplib_del_sgid(sgid_tbl, gid_to_del, + vlan_id, true); if (rc) { dev_err(rdev_to_dev(rdev), "Failed to remove GID: %#x", rc); diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c b/drivers/infiniband/hw/bnxt_re/qplib_res.c index 37928b1111df..bdbde8e22420 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_res.c +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c @@ -488,7 +488,7 @@ static int bnxt_qplib_alloc_sgid_tbl(struct bnxt_qplib_res *res, struct bnxt_qplib_sgid_tbl *sgid_tbl, u16 max) { - sgid_tbl->tbl = kcalloc(max, sizeof(struct bnxt_qplib_gid), GFP_KERNEL); + sgid_tbl->tbl = kcalloc(max, sizeof(*sgid_tbl->tbl), GFP_KERNEL); if (!sgid_tbl->tbl) return -ENOMEM;
@@ -526,9 +526,10 @@ static void bnxt_qplib_cleanup_sgid_tbl(struct bnxt_qplib_res *res, for (i = 0; i < sgid_tbl->max; i++) { if (memcmp(&sgid_tbl->tbl[i], &bnxt_qplib_gid_zero, sizeof(bnxt_qplib_gid_zero))) - bnxt_qplib_del_sgid(sgid_tbl, &sgid_tbl->tbl[i], true); + bnxt_qplib_del_sgid(sgid_tbl, &sgid_tbl->tbl[i].gid, + sgid_tbl->tbl[i].vlan_id, true); } - memset(sgid_tbl->tbl, 0, sizeof(struct bnxt_qplib_gid) * sgid_tbl->max); + memset(sgid_tbl->tbl, 0, sizeof(*sgid_tbl->tbl) * sgid_tbl->max); memset(sgid_tbl->hw_id, -1, sizeof(u16) * sgid_tbl->max); memset(sgid_tbl->vlan, 0, sizeof(u8) * sgid_tbl->max); sgid_tbl->active = 0; @@ -537,7 +538,11 @@ static void bnxt_qplib_cleanup_sgid_tbl(struct bnxt_qplib_res *res, static void bnxt_qplib_init_sgid_tbl(struct bnxt_qplib_sgid_tbl *sgid_tbl, struct net_device *netdev) { - memset(sgid_tbl->tbl, 0, sizeof(struct bnxt_qplib_gid) * sgid_tbl->max); + u32 i; + + for (i = 0; i < sgid_tbl->max; i++) + sgid_tbl->tbl[i].vlan_id = 0xffff; + memset(sgid_tbl->hw_id, -1, sizeof(u16) * sgid_tbl->max); }
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.h b/drivers/infiniband/hw/bnxt_re/qplib_res.h index 30c42c92fac7..fbda11a7ab1a 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_res.h +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.h @@ -111,7 +111,7 @@ struct bnxt_qplib_pd_tbl { };
struct bnxt_qplib_sgid_tbl { - struct bnxt_qplib_gid *tbl; + struct bnxt_qplib_gid_info *tbl; u16 *hw_id; u16 max; u16 active; diff --git a/drivers/infiniband/hw/bnxt_re/qplib_sp.c b/drivers/infiniband/hw/bnxt_re/qplib_sp.c index 48793d3512ac..40296b97d21e 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_sp.c +++ b/drivers/infiniband/hw/bnxt_re/qplib_sp.c @@ -213,12 +213,12 @@ int bnxt_qplib_get_sgid(struct bnxt_qplib_res *res, index, sgid_tbl->max); return -EINVAL; } - memcpy(gid, &sgid_tbl->tbl[index], sizeof(*gid)); + memcpy(gid, &sgid_tbl->tbl[index].gid, sizeof(*gid)); return 0; }
int bnxt_qplib_del_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl, - struct bnxt_qplib_gid *gid, bool update) + struct bnxt_qplib_gid *gid, u16 vlan_id, bool update) { struct bnxt_qplib_res *res = to_bnxt_qplib(sgid_tbl, struct bnxt_qplib_res, @@ -236,7 +236,8 @@ int bnxt_qplib_del_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl, return -ENOMEM; } for (index = 0; index < sgid_tbl->max; index++) { - if (!memcmp(&sgid_tbl->tbl[index], gid, sizeof(*gid))) + if (!memcmp(&sgid_tbl->tbl[index].gid, gid, sizeof(*gid)) && + vlan_id == sgid_tbl->tbl[index].vlan_id) break; } if (index == sgid_tbl->max) { @@ -262,8 +263,9 @@ int bnxt_qplib_del_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl, if (rc) return rc; } - memcpy(&sgid_tbl->tbl[index], &bnxt_qplib_gid_zero, + memcpy(&sgid_tbl->tbl[index].gid, &bnxt_qplib_gid_zero, sizeof(bnxt_qplib_gid_zero)); + sgid_tbl->tbl[index].vlan_id = 0xFFFF; sgid_tbl->vlan[index] = 0; sgid_tbl->active--; dev_dbg(&res->pdev->dev, @@ -296,7 +298,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 +354,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..13d9432d5ce2 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_sp.h +++ b/drivers/infiniband/hw/bnxt_re/qplib_sp.h @@ -84,6 +84,11 @@ struct bnxt_qplib_gid { u8 data[16]; };
+struct bnxt_qplib_gid_info { + struct bnxt_qplib_gid gid; + u16 vlan_id; +}; + struct bnxt_qplib_ah { struct bnxt_qplib_gid dgid; struct bnxt_qplib_pd *pd; @@ -221,7 +226,7 @@ int bnxt_qplib_get_sgid(struct bnxt_qplib_res *res, struct bnxt_qplib_sgid_tbl *sgid_tbl, int index, struct bnxt_qplib_gid *gid); int bnxt_qplib_del_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl, - struct bnxt_qplib_gid *gid, bool update); + struct bnxt_qplib_gid *gid, u16 vlan_id, bool update); int bnxt_qplib_add_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl, struct bnxt_qplib_gid *gid, u8 *mac, u16 vlan_id, bool update, u32 *index);
Verified this patch on my nvme rdma bnxt_re environment, thanks.
Tested-by: Yi Zhang yi.zhang@redhat.com
On 7/15/19 5:19 PM, Selvin Xavier wrote:
GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com Signed-off-by: Selvin Xavier selvin.xavier@broadcom.com
drivers/infiniband/hw/bnxt_re/ib_verbs.c | 7 +++++-- drivers/infiniband/hw/bnxt_re/qplib_res.c | 13 +++++++++---- drivers/infiniband/hw/bnxt_re/qplib_res.h | 2 +- drivers/infiniband/hw/bnxt_re/qplib_sp.c | 14 +++++++++----- drivers/infiniband/hw/bnxt_re/qplib_sp.h | 7 ++++++- 5 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c index 2c3685faa57a..a4a9f90f2482 100644 --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c @@ -308,6 +308,7 @@ int bnxt_re_del_gid(const struct ib_gid_attr *attr, void **context) struct bnxt_re_dev *rdev = to_bnxt_re_dev(attr->device, ibdev); struct bnxt_qplib_sgid_tbl *sgid_tbl = &rdev->qplib_res.sgid_tbl; struct bnxt_qplib_gid *gid_to_del;
- u16 vlan_id = 0xFFFF;
/* Delete the entry from the hardware */ ctx = *context; @@ -317,7 +318,8 @@ int bnxt_re_del_gid(const struct ib_gid_attr *attr, void **context) if (sgid_tbl && sgid_tbl->active) { if (ctx->idx >= sgid_tbl->max) return -EINVAL;
gid_to_del = &sgid_tbl->tbl[ctx->idx];
gid_to_del = &sgid_tbl->tbl[ctx->idx].gid;vlan_id = sgid_tbl->tbl[ctx->idx].vlan_id;
- or via the ib_unregister_device path. In the former case QP1
- may not be destroyed yet, in which case just return as FW
@@ -335,7 +337,8 @@ int bnxt_re_del_gid(const struct ib_gid_attr *attr, void **context) } ctx->refcnt--; if (!ctx->refcnt) {
rc = bnxt_qplib_del_sgid(sgid_tbl, gid_to_del, true);
rc = bnxt_qplib_del_sgid(sgid_tbl, gid_to_del,vlan_id, true); if (rc) { dev_err(rdev_to_dev(rdev), "Failed to remove GID: %#x", rc);diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c b/drivers/infiniband/hw/bnxt_re/qplib_res.c index 37928b1111df..bdbde8e22420 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_res.c +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c @@ -488,7 +488,7 @@ static int bnxt_qplib_alloc_sgid_tbl(struct bnxt_qplib_res *res, struct bnxt_qplib_sgid_tbl *sgid_tbl, u16 max) {
- sgid_tbl->tbl = kcalloc(max, sizeof(struct bnxt_qplib_gid), GFP_KERNEL);
- sgid_tbl->tbl = kcalloc(max, sizeof(*sgid_tbl->tbl), GFP_KERNEL); if (!sgid_tbl->tbl) return -ENOMEM;
@@ -526,9 +526,10 @@ static void bnxt_qplib_cleanup_sgid_tbl(struct bnxt_qplib_res *res, for (i = 0; i < sgid_tbl->max; i++) { if (memcmp(&sgid_tbl->tbl[i], &bnxt_qplib_gid_zero, sizeof(bnxt_qplib_gid_zero)))
bnxt_qplib_del_sgid(sgid_tbl, &sgid_tbl->tbl[i], true);
bnxt_qplib_del_sgid(sgid_tbl, &sgid_tbl->tbl[i].gid,sgid_tbl->tbl[i].vlan_id, true);
- memset(sgid_tbl->tbl, 0, sizeof(struct bnxt_qplib_gid) * sgid_tbl->max);
- memset(sgid_tbl->tbl, 0, sizeof(*sgid_tbl->tbl) * sgid_tbl->max); memset(sgid_tbl->hw_id, -1, sizeof(u16) * sgid_tbl->max); memset(sgid_tbl->vlan, 0, sizeof(u8) * sgid_tbl->max); sgid_tbl->active = 0;
@@ -537,7 +538,11 @@ static void bnxt_qplib_cleanup_sgid_tbl(struct bnxt_qplib_res *res, static void bnxt_qplib_init_sgid_tbl(struct bnxt_qplib_sgid_tbl *sgid_tbl, struct net_device *netdev) {
- memset(sgid_tbl->tbl, 0, sizeof(struct bnxt_qplib_gid) * sgid_tbl->max);
- u32 i;
- for (i = 0; i < sgid_tbl->max; i++)
sgid_tbl->tbl[i].vlan_id = 0xffff;- memset(sgid_tbl->hw_id, -1, sizeof(u16) * sgid_tbl->max); }
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.h b/drivers/infiniband/hw/bnxt_re/qplib_res.h index 30c42c92fac7..fbda11a7ab1a 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_res.h +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.h @@ -111,7 +111,7 @@ struct bnxt_qplib_pd_tbl { }; struct bnxt_qplib_sgid_tbl {
- struct bnxt_qplib_gid *tbl;
- struct bnxt_qplib_gid_info *tbl; u16 *hw_id; u16 max; u16 active;
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_sp.c b/drivers/infiniband/hw/bnxt_re/qplib_sp.c index 48793d3512ac..40296b97d21e 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_sp.c +++ b/drivers/infiniband/hw/bnxt_re/qplib_sp.c @@ -213,12 +213,12 @@ int bnxt_qplib_get_sgid(struct bnxt_qplib_res *res, index, sgid_tbl->max); return -EINVAL; }
- memcpy(gid, &sgid_tbl->tbl[index], sizeof(*gid));
- memcpy(gid, &sgid_tbl->tbl[index].gid, sizeof(*gid)); return 0; }
int bnxt_qplib_del_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl,
struct bnxt_qplib_gid *gid, bool update)
struct bnxt_qplib_gid *gid, u16 vlan_id, bool update)@@ -236,7 +236,8 @@ int bnxt_qplib_del_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl, return -ENOMEM; } for (index = 0; index < sgid_tbl->max; index++) {
if (!memcmp(&sgid_tbl->tbl[index], gid, sizeof(*gid)))
if (!memcmp(&sgid_tbl->tbl[index].gid, gid, sizeof(*gid)) &&vlan_id == sgid_tbl->tbl[index].vlan_id) break;@@ -262,8 +263,9 @@ int bnxt_qplib_del_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl, if (rc) return rc; }
- memcpy(&sgid_tbl->tbl[index], &bnxt_qplib_gid_zero,
- memcpy(&sgid_tbl->tbl[index].gid, &bnxt_qplib_gid_zero, sizeof(bnxt_qplib_gid_zero));
- sgid_tbl->tbl[index].vlan_id = 0xFFFF; sgid_tbl->vlan[index] = 0; sgid_tbl->active--; dev_dbg(&res->pdev->dev,
@@ -296,7 +298,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 +354,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..13d9432d5ce2 100644 --- a/drivers/infiniband/hw/bnxt_re/qplib_sp.h +++ b/drivers/infiniband/hw/bnxt_re/qplib_sp.h @@ -84,6 +84,11 @@ struct bnxt_qplib_gid { u8 data[16]; }; +struct bnxt_qplib_gid_info {
- struct bnxt_qplib_gid gid;
- u16 vlan_id;
+};
- struct bnxt_qplib_ah { struct bnxt_qplib_gid dgid; struct bnxt_qplib_pd *pd;
@@ -221,7 +226,7 @@ int bnxt_qplib_get_sgid(struct bnxt_qplib_res *res, struct bnxt_qplib_sgid_tbl *sgid_tbl, int index, struct bnxt_qplib_gid *gid); int bnxt_qplib_del_sgid(struct bnxt_qplib_sgid_tbl *sgid_tbl,
struct bnxt_qplib_gid *gid, bool update);
struct bnxt_qplib_gid *gid, u16 vlan_id, bool update);
On Mon, Jul 15, 2019 at 05:19:13AM -0400, Selvin Xavier wrote:
GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com
Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com
I never understood why bad habits are so stinky.
Can you please explain us what does it mean Co-developed-by and Signed-off-by of the same person in the same patch?
Signed-off-by: Selvin Xavier selvin.xavier@broadcom.com
Thanks
On Tue, Jul 16, 2019 at 10:10:30AM +0300, Leon Romanovsky wrote:
On Mon, Jul 15, 2019 at 05:19:13AM -0400, Selvin Xavier wrote:
GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com
Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com
I never understood why bad habits are so stinky.
Can you please explain us what does it mean Co-developed-by and Signed-off-by of the same person in the same patch?
See Documentation/process/submitting-patches.rst for what that tag means.
thanks,
greg k-h
On Tue, Jul 16, 2019 at 04:16:44PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 10:10:30AM +0300, Leon Romanovsky wrote:
On Mon, Jul 15, 2019 at 05:19:13AM -0400, Selvin Xavier wrote:
GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com
Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com
I never understood why bad habits are so stinky.
Can you please explain us what does it mean Co-developed-by and Signed-off-by of the same person in the same patch?
See Documentation/process/submitting-patches.rst for what that tag means.
Read it, it doesn't help me to understand if I should now add Co-developed-by tag to most of RDMA Mellanox upstreamed patches, which already care my Signed-off-by, because I'm changing and fixing them many times.
Maybe I should write for my upstreamed patched something like this? Co-developed-by: Leon R... Reviewed-By: Leon R... Tested-by: Leon R... Signed-by: Leon R...
Thanks
thanks,
greg k-h
On Tue, Jul 16, 2019 at 11:41:26AM +0300, Leon Romanovsky wrote:
On Tue, Jul 16, 2019 at 04:16:44PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 10:10:30AM +0300, Leon Romanovsky wrote:
On Mon, Jul 15, 2019 at 05:19:13AM -0400, Selvin Xavier wrote:
GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com
Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com
I never understood why bad habits are so stinky.
Can you please explain us what does it mean Co-developed-by and Signed-off-by of the same person in the same patch?
See Documentation/process/submitting-patches.rst for what that tag means.
Read it, it doesn't help me to understand if I should now add Co-developed-by tag to most of RDMA Mellanox upstreamed patches, which already care my Signed-off-by, because I'm changing and fixing them many times.
It depends, it's your call, if you think you deserve the credit, sure, add it. If you are just doing basic "review" where you tell people what needs to be done better, that's probably not what you need to do here.
One example, where I just added myself to a patch happened last week where the developer submitted one solution, I took it and rewrote the whole implementation (from raw kobjects to using the driver model). The original author got the "From:" and I got a Co-developed-by line.
Does that help?
thanks,
greg k-h
On Tue, Jul 16, 2019 at 06:09:17PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 11:41:26AM +0300, Leon Romanovsky wrote:
On Tue, Jul 16, 2019 at 04:16:44PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 10:10:30AM +0300, Leon Romanovsky wrote:
On Mon, Jul 15, 2019 at 05:19:13AM -0400, Selvin Xavier wrote:
GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com
Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com
I never understood why bad habits are so stinky.
Can you please explain us what does it mean Co-developed-by and Signed-off-by of the same person in the same patch?
See Documentation/process/submitting-patches.rst for what that tag means.
Read it, it doesn't help me to understand if I should now add Co-developed-by tag to most of RDMA Mellanox upstreamed patches, which already care my Signed-off-by, because I'm changing and fixing them many times.
It depends, it's your call, if you think you deserve the credit, sure, add it. If you are just doing basic "review" where you tell people what needs to be done better, that's probably not what you need to do here.
I'll probably not use this and not because I don't deserve credit, but because it looks ridiculously to me to see my name repeated N times for my work.
One example, where I just added myself to a patch happened last week where the developer submitted one solution, I took it and rewrote the whole implementation (from raw kobjects to using the driver model). The original author got the "From:" and I got a Co-developed-by line.
In old days, we simply changed Author field if changes were above some arbitrary threshold (usually half of the original patch) and added SOB.
Why wasn't this approach enough?
Does that help?
Yes, and it makes me wonder when we will need to hire compliance officer who will review all our upstreamed patches to comply with more and more bureaucracy.
Thanks.
thanks,
greg k-h
On Tue, Jul 16, 2019 at 12:50:07PM +0300, Leon Romanovsky wrote:
On Tue, Jul 16, 2019 at 06:09:17PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 11:41:26AM +0300, Leon Romanovsky wrote:
On Tue, Jul 16, 2019 at 04:16:44PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 10:10:30AM +0300, Leon Romanovsky wrote:
On Mon, Jul 15, 2019 at 05:19:13AM -0400, Selvin Xavier wrote:
GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com
Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com
I never understood why bad habits are so stinky.
Can you please explain us what does it mean Co-developed-by and Signed-off-by of the same person in the same patch?
See Documentation/process/submitting-patches.rst for what that tag means.
Read it, it doesn't help me to understand if I should now add Co-developed-by tag to most of RDMA Mellanox upstreamed patches, which already care my Signed-off-by, because I'm changing and fixing them many times.
It depends, it's your call, if you think you deserve the credit, sure, add it. If you are just doing basic "review" where you tell people what needs to be done better, that's probably not what you need to do here.
I'll probably not use this and not because I don't deserve credit, but because it looks ridiculously to me to see my name repeated N times for my work.
That's up to you, and your fellow co-authors to decide.
One example, where I just added myself to a patch happened last week where the developer submitted one solution, I took it and rewrote the whole implementation (from raw kobjects to using the driver model). The original author got the "From:" and I got a Co-developed-by line.
In old days, we simply changed Author field if changes were above some arbitrary threshold (usually half of the original patch) and added SOB.
Why wasn't this approach enough?
Because we have had some patches where it really was a work of multiple people and it is good to show the correct authorship wherever possible.
If you look, this tag was added based on a document in the kernel tree that Thomas and I worked on together and we both wanted the "blame" for it :)
Does that help?
Yes, and it makes me wonder when we will need to hire compliance officer who will review all our upstreamed patches to comply with more and more bureaucracy.
Oh come on, this is about the ability to give people credit where they did not have it before. It's not about being "compliant", it's about being "nice" and "fair". Something that no one should complain about.
There is no one forcing you to add this tag to patches with your name on it if you do not want to. But for those who work on changes together, it is important to give them that type of credit.
thanks,
greg k-h
On Tue, Jul 16, 2019 at 06:58:52PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 12:50:07PM +0300, Leon Romanovsky wrote:
On Tue, Jul 16, 2019 at 06:09:17PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 11:41:26AM +0300, Leon Romanovsky wrote:
On Tue, Jul 16, 2019 at 04:16:44PM +0900, Greg KH wrote:
On Tue, Jul 16, 2019 at 10:10:30AM +0300, Leon Romanovsky wrote:
On Mon, Jul 15, 2019 at 05:19:13AM -0400, Selvin Xavier wrote: > GID entry consist of GID, vlan, netdev and smac. > Extend GID duplicate check companions to consider vlan_id as well > to support IPv6 VLAN based link local addresses. Introduce > a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information. > > The issue is discussed in the following thread > https://www.spinics.net/lists/linux-rdma/msg81594.html > > Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") > Cc: stable@vger.kernel.org # v5.2+ > Reported-by: Yi Zhang yi.zhang@redhat.com
> Co-developed-by: Parav Pandit parav@mellanox.com > Signed-off-by: Parav Pandit parav@mellanox.com
I never understood why bad habits are so stinky.
Can you please explain us what does it mean Co-developed-by and Signed-off-by of the same person in the same patch?
See Documentation/process/submitting-patches.rst for what that tag means.
Read it, it doesn't help me to understand if I should now add Co-developed-by tag to most of RDMA Mellanox upstreamed patches, which already care my Signed-off-by, because I'm changing and fixing them many times.
It depends, it's your call, if you think you deserve the credit, sure, add it. If you are just doing basic "review" where you tell people what needs to be done better, that's probably not what you need to do here.
I'll probably not use this and not because I don't deserve credit, but because it looks ridiculously to me to see my name repeated N times for my work.
That's up to you, and your fellow co-authors to decide.
One example, where I just added myself to a patch happened last week where the developer submitted one solution, I took it and rewrote the whole implementation (from raw kobjects to using the driver model). The original author got the "From:" and I got a Co-developed-by line.
In old days, we simply changed Author field if changes were above some arbitrary threshold (usually half of the original patch) and added SOB.
Why wasn't this approach enough?
Because we have had some patches where it really was a work of multiple people and it is good to show the correct authorship wherever possible.
If you look, this tag was added based on a document in the kernel tree that Thomas and I worked on together and we both wanted the "blame" for it :)
Does that help?
Yes, and it makes me wonder when we will need to hire compliance officer who will review all our upstreamed patches to comply with more and more bureaucracy.
Oh come on, this is about the ability to give people credit where they did not have it before. It's not about being "compliant", it's about being "nice" and "fair". Something that no one should complain about.
There is no one forcing you to add this tag to patches with your name on it if you do not want to. But for those who work on changes together, it is important to give them that type of credit.
It is partly true, I agree that for my own patches I can do more or less whatever I want, but my responsibilities are broader and I need to guide internal development teams on how to develop for upstream and how to upstream their work later on.
Exactly like Theodore (if I'm not mistaken here) mentioned in last reply to ksummit thread about meaningful Reviewed-by and Acked-by tags, I got complains when I changed/fixed inappropriate tags. Now, I'll get extra complains for not allowing to use Co-... tag too.
So it is not true for my second responsibility, where I must to do it right and with minimal number of my personal preferences.
This extra documented tag puts me in position where I don't want to be - in the middle between documentation and personal opinion on not important thing.
Thanks
thanks,
greg k-h
On Mon, Jul 15, 2019 at 05:19:13AM -0400, Selvin Xavier wrote:
GID entry consist of GID, vlan, netdev and smac. Extend GID duplicate check companions to consider vlan_id as well to support IPv6 VLAN based link local addresses. Introduce a new structure (bnxt_qplib_gid_info) to hold gid and vlan_id information.
The issue is discussed in the following thread https://www.spinics.net/lists/linux-rdma/msg81594.html
Fixes: 823b23da7113 ("IB/core: Allow vlan link local address based RoCE GIDs") Cc: stable@vger.kernel.org # v5.2+ Reported-by: Yi Zhang yi.zhang@redhat.com Co-developed-by: Parav Pandit parav@mellanox.com Signed-off-by: Parav Pandit parav@mellanox.com Signed-off-by: Selvin Xavier selvin.xavier@broadcom.com Tested-by: Yi Zhang yi.zhang@redhat.com Reviewed-By: Leon R... Tested-by: Leon R...
drivers/infiniband/hw/bnxt_re/ib_verbs.c | 7 +++++-- drivers/infiniband/hw/bnxt_re/qplib_res.c | 13 +++++++++---- drivers/infiniband/hw/bnxt_re/qplib_res.h | 2 +- drivers/infiniband/hw/bnxt_re/qplib_sp.c | 14 +++++++++----- drivers/infiniband/hw/bnxt_re/qplib_sp.h | 7 ++++++- 5 files changed, 30 insertions(+), 13 deletions(-)
Applied to for-rc, thanks
Please also fix that sketchy use of the gid_index
Jason
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Jason Gunthorpe -
Leon Romanovsky -
Selvin Xavier -
Yi Zhang