Re: IB/core: Use GID table in AH creation and dmac resolution
Thanks Dan and Matan.
We will take a look and revert on this
Thanks
Som
On Wed, Nov 4, 2015 at 9:31 AM, Somnath Kotur
wrote:
> Thanks Dan and Matan.
>
> We will take a look and revert on this
>
> Thanks
> Som
>
> On Tue, Nov 3, 2015 at 7:14 PM, Matan Barak wrote:
>>
>>
>>
>> On 11/3/2015 3:11 PM, Dan Carpenter wrote:
>>>
>>> Hello Matan Barak,
>>>
>>> This is a semi-automatic email about new static checker warnings.
>>>
>>> The patch dbf727de7440: "IB/core: Use GID table in AH creation and
>>> dmac resolution" from Oct 15, 2015, leads to the following Smatch
>>> complaint:
>>>
>>> drivers/infiniband/hw/ocrdma/ocrdma_ah.c:157 ocrdma_create_ah()
>>> error: we previously assumed 'sgid_attr.ndev' could be null (see
>>> line 146)
>>>
>>> drivers/infiniband/hw/ocrdma/ocrdma_ah.c
>>> 145 }
>>> 146 if (sgid_attr.ndev) {
>>> ^^
>>> Patch introduces a NULL check.
>>>
>>> 147 if (is_vlan_dev(sgid_attr.ndev))
>>> 148 vlan_tag =
>>> vlan_dev_vlan_id(sgid_attr.ndev);
>>> 149 dev_put(sgid_attr.ndev);
>>> 150 }
>>> 151
>>> 152 if ((pd->uctx) &&
>>> 153 (!rdma_is_multicast_addr((struct in6_addr
>>> *)attr->grh.dgid.raw)) &&
>>> 154 (!rdma_link_local_addr((struct in6_addr
>>> *)attr->grh.dgid.raw))) {
>>> 155 status = rdma_addr_find_dmac_by_grh(&sgid,
>>> &attr->grh.dgid,
>>> 156 attr->dmac,
>>> &vlan_tag,
>>> 157
>>> sgid_attr.ndev->ifindex);
>>>
>>>
>>> Patch introduces this new dereference. The warning might be a false
>>> positive if "pd->uctx" or rdma_is_multicast_addr() imply it's non-NULL
>>> but I don't know this code well enough to say for sure. Hence this
>>> email. :)
>>>
>>> 158 if (status) {
>>> 159 pr_err("%s(): Failed to resolve dmac from
>>> gid."
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> Thanks for the catch Dan.
>> As I wrote in the commit message - "ocrdma driver changes were done by
>> Somnath Kotur "
>> Somnath, RoCE implies non-NULL ndev, but dereferencing ifindex after
>> dev_put doesn't seem to be safe.
>> Could you please take a look?
>>
>> Thanks,
>> Matan
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: IB/core: Use GID table in AH creation and dmac resolution
On 11/3/2015 3:11 PM, Dan Carpenter wrote:
Hello Matan Barak,
This is a semi-automatic email about new static checker warnings.
The patch dbf727de7440: "IB/core: Use GID table in AH creation and
dmac resolution" from Oct 15, 2015, leads to the following Smatch
complaint:
drivers/infiniband/hw/ocrdma/ocrdma_ah.c:157 ocrdma_create_ah()
error: we previously assumed 'sgid_attr.ndev' could be null (see line
146)
drivers/infiniband/hw/ocrdma/ocrdma_ah.c
145 }
146 if (sgid_attr.ndev) {
^^
Patch introduces a NULL check.
147 if (is_vlan_dev(sgid_attr.ndev))
148 vlan_tag = vlan_dev_vlan_id(sgid_attr.ndev);
149 dev_put(sgid_attr.ndev);
150 }
151
152 if ((pd->uctx) &&
153 (!rdma_is_multicast_addr((struct in6_addr *)attr->grh.dgid.raw))
&&
154 (!rdma_link_local_addr((struct in6_addr
*)attr->grh.dgid.raw))) {
155 status = rdma_addr_find_dmac_by_grh(&sgid,
&attr->grh.dgid,
156 attr->dmac,
&vlan_tag,
157
sgid_attr.ndev->ifindex);
Patch introduces this new dereference. The warning might be a false
positive if "pd->uctx" or rdma_is_multicast_addr() imply it's non-NULL
but I don't know this code well enough to say for sure. Hence this
email. :)
158 if (status) {
159 pr_err("%s(): Failed to resolve dmac from gid."
regards,
dan carpenter
Thanks for the catch Dan.
As I wrote in the commit message - "ocrdma driver changes were done by
Somnath Kotur "
Somnath, RoCE implies non-NULL ndev, but dereferencing ifindex after
dev_put doesn't seem to be safe.
Could you please take a look?
Thanks,
Matan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
re: IB/core: Use GID table in AH creation and dmac resolution
Hello Matan Barak,
This is a semi-automatic email about new static checker warnings.
The patch dbf727de7440: "IB/core: Use GID table in AH creation and
dmac resolution" from Oct 15, 2015, leads to the following Smatch
complaint:
drivers/infiniband/hw/ocrdma/ocrdma_ah.c:157 ocrdma_create_ah()
error: we previously assumed 'sgid_attr.ndev' could be null (see line
146)
drivers/infiniband/hw/ocrdma/ocrdma_ah.c
145 }
146 if (sgid_attr.ndev) {
^^
Patch introduces a NULL check.
147 if (is_vlan_dev(sgid_attr.ndev))
148 vlan_tag = vlan_dev_vlan_id(sgid_attr.ndev);
149 dev_put(sgid_attr.ndev);
150 }
151
152 if ((pd->uctx) &&
153 (!rdma_is_multicast_addr((struct in6_addr
*)attr->grh.dgid.raw)) &&
154 (!rdma_link_local_addr((struct in6_addr
*)attr->grh.dgid.raw))) {
155 status = rdma_addr_find_dmac_by_grh(&sgid,
&attr->grh.dgid,
156 attr->dmac,
&vlan_tag,
157
sgid_attr.ndev->ifindex);
Patch introduces this new dereference. The warning might be a false
positive if "pd->uctx" or rdma_is_multicast_addr() imply it's non-NULL
but I don't know this code well enough to say for sure. Hence this
email. :)
158 if (status) {
159 pr_err("%s(): Failed to resolve dmac from gid."
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH for-next v2 07/10] IB/core: Use GID table in AH creation and dmac resolution
Previously, vlan id and source MAC were used from QP attributes. Since
the net device is now stored in the GID attributes, they could be used
instead of getting this information from the QP attributes.
IB_QP_SMAC, IB_QP_ALT_SMAC, IB_QP_VID and IB_QP_ALT_VID were removed
because there is no known libibverbs that uses them.
This commit also modifies the vendors (mlx4, ocrdma) drivers in order
to use the new approach.
ocrdma driver changes were done by Somnath Kotur
Signed-off-by: Matan Barak
---
drivers/infiniband/core/addr.c | 3 +-
drivers/infiniband/core/core_priv.h | 4 +-
drivers/infiniband/core/uverbs_cmd.c | 2 +-
drivers/infiniband/core/verbs.c | 160 ++-
drivers/infiniband/hw/mlx4/ah.c | 17 +++-
drivers/infiniband/hw/mlx4/mad.c | 12 ++-
drivers/infiniband/hw/mlx4/mcg.c | 2 +-
drivers/infiniband/hw/mlx4/mlx4_ib.h | 2 +-
drivers/infiniband/hw/mlx4/qp.c | 47 +++--
drivers/infiniband/hw/ocrdma/ocrdma_ah.c | 22 +++--
drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 30 +++---
include/rdma/ib_addr.h | 2 +-
12 files changed, 193 insertions(+), 110 deletions(-)
diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
index 746cdf5..d3c42b3 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -458,7 +458,7 @@ static void resolve_cb(int status, struct sockaddr
*src_addr,
}
int rdma_addr_find_dmac_by_grh(const union ib_gid *sgid, const union ib_gid
*dgid,
- u8 *dmac, u16 *vlan_id)
+ u8 *dmac, u16 *vlan_id, int if_index)
{
int ret = 0;
struct rdma_dev_addr dev_addr;
@@ -476,6 +476,7 @@ int rdma_addr_find_dmac_by_grh(const union ib_gid *sgid,
const union ib_gid *dgi
rdma_gid2ip(&dgid_addr._sockaddr, dgid);
memset(&dev_addr, 0, sizeof(dev_addr));
+ dev_addr.bound_dev_if = if_index;
ctx.addr = &dev_addr;
init_completion(&ctx.comp);
diff --git a/drivers/infiniband/core/core_priv.h
b/drivers/infiniband/core/core_priv.h
index 0df82b1..5cf6eb7 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -46,8 +46,8 @@ void ib_device_unregister_sysfs(struct ib_device *device);
void ib_cache_setup(void);
void ib_cache_cleanup(void);
-int ib_resolve_eth_l2_attrs(struct ib_qp *qp,
- struct ib_qp_attr *qp_attr, int *qp_attr_mask);
+int ib_resolve_eth_dmac(struct ib_qp *qp,
+ struct ib_qp_attr *qp_attr, int *qp_attr_mask);
typedef void (*roce_netdev_callback)(struct ib_device *device, u8 port,
struct net_device *idev, void *cookie);
diff --git a/drivers/infiniband/core/uverbs_cmd.c
b/drivers/infiniband/core/uverbs_cmd.c
index be4cb9f..b242480 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2221,7 +2221,7 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file,
attr->alt_ah_attr.port_num = cmd.alt_dest.port_num;
if (qp->real_qp == qp) {
- ret = ib_resolve_eth_l2_attrs(qp, attr, &cmd.attr_mask);
+ ret = ib_resolve_eth_dmac(qp, attr, &cmd.attr_mask);
if (ret)
goto release_qp;
ret = qp->device->modify_qp(qp, attr,
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 6012f5e..46d97f0 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -41,6 +41,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -308,6 +311,35 @@ struct ib_ah *ib_create_ah(struct ib_pd *pd, struct
ib_ah_attr *ah_attr)
}
EXPORT_SYMBOL(ib_create_ah);
+struct find_gid_index_context {
+ u16 vlan_id;
+};
+
+static bool find_gid_index(const union ib_gid *gid,
+ const struct ib_gid_attr *gid_attr,
+ void *context)
+{
+ struct find_gid_index_context *ctx =
+ (struct find_gid_index_context *)context;
+
+ if ((!!(ctx->vlan_id != 0x) == !is_vlan_dev(gid_attr->ndev)) ||
+ (is_vlan_dev(gid_attr->ndev) &&
+vlan_dev_vlan_id(gid_attr->ndev) != ctx->vlan_id))
+ return false;
+
+ return true;
+}
+
+static int get_sgid_index_from_eth(struct ib_device *device, u8 port_num,
+ u16 vlan_id, const union ib_gid *sgid,
+ u16 *gid_index)
+{
+ struct find_gid_index_context context = {.vlan_id = vlan_id};
+
+ return ib_find_gid_by_filter(device, sgid, port_num, find_gid_index,
+&context, gid_index);
+}
+
int ib_init_ah_from_wc(struct ib_device *device, u8 port_num,
const struct ib_wc *wc, const struct ib_grh *grh,
struc
[PATCH for-next V1 07/10] IB/core: Use GID table in AH creation and dmac resolution
Previously, vlan id and source MAC were used from QP attributes. Since
the net device is now stored in the GID attributes, they could be used
instead of getting this information from the QP attributes.
IB_QP_SMAC, IB_QP_ALT_SMAC, IB_QP_VID and IB_QP_ALT_VID were removed
because there is no known libibverbs that uses them.
This commit also modifies the vendors (mlx4, ocrdma) drivers in order
to use the new approach.
ocrdma driver changes were done by Somnath Kotur
Signed-off-by: Matan Barak
---
drivers/infiniband/core/addr.c | 3 +-
drivers/infiniband/core/core_priv.h | 4 +-
drivers/infiniband/core/uverbs_cmd.c | 2 +-
drivers/infiniband/core/verbs.c | 160 ++-
drivers/infiniband/hw/mlx4/ah.c | 17 +++-
drivers/infiniband/hw/mlx4/mad.c | 12 ++-
drivers/infiniband/hw/mlx4/mcg.c | 2 +-
drivers/infiniband/hw/mlx4/mlx4_ib.h | 2 +-
drivers/infiniband/hw/mlx4/qp.c | 47 +++--
drivers/infiniband/hw/ocrdma/ocrdma_ah.c | 22 +++--
drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 30 +++---
include/rdma/ib_addr.h | 2 +-
12 files changed, 193 insertions(+), 110 deletions(-)
diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
index 746cdf5..d3c42b3 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -458,7 +458,7 @@ static void resolve_cb(int status, struct sockaddr
*src_addr,
}
int rdma_addr_find_dmac_by_grh(const union ib_gid *sgid, const union ib_gid
*dgid,
- u8 *dmac, u16 *vlan_id)
+ u8 *dmac, u16 *vlan_id, int if_index)
{
int ret = 0;
struct rdma_dev_addr dev_addr;
@@ -476,6 +476,7 @@ int rdma_addr_find_dmac_by_grh(const union ib_gid *sgid,
const union ib_gid *dgi
rdma_gid2ip(&dgid_addr._sockaddr, dgid);
memset(&dev_addr, 0, sizeof(dev_addr));
+ dev_addr.bound_dev_if = if_index;
ctx.addr = &dev_addr;
init_completion(&ctx.comp);
diff --git a/drivers/infiniband/core/core_priv.h
b/drivers/infiniband/core/core_priv.h
index b968467..06df0f8 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -46,8 +46,8 @@ void ib_device_unregister_sysfs(struct ib_device *device);
void ib_cache_setup(void);
void ib_cache_cleanup(void);
-int ib_resolve_eth_l2_attrs(struct ib_qp *qp,
- struct ib_qp_attr *qp_attr, int *qp_attr_mask);
+int ib_resolve_eth_dmac(struct ib_qp *qp,
+ struct ib_qp_attr *qp_attr, int *qp_attr_mask);
typedef void (*roce_netdev_callback)(struct ib_device *device, u8 port,
struct net_device *idev, void *cookie);
diff --git a/drivers/infiniband/core/uverbs_cmd.c
b/drivers/infiniband/core/uverbs_cmd.c
index 29443c0..2a22180 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2210,7 +2210,7 @@ ssize_t ib_uverbs_modify_qp(struct ib_uverbs_file *file,
attr->alt_ah_attr.port_num = cmd.alt_dest.port_num;
if (qp->real_qp == qp) {
- ret = ib_resolve_eth_l2_attrs(qp, attr, &cmd.attr_mask);
+ ret = ib_resolve_eth_dmac(qp, attr, &cmd.attr_mask);
if (ret)
goto release_qp;
ret = qp->device->modify_qp(qp, attr,
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 49162cf..7194189 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -41,6 +41,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -257,6 +260,35 @@ struct ib_ah *ib_create_ah(struct ib_pd *pd, struct
ib_ah_attr *ah_attr)
}
EXPORT_SYMBOL(ib_create_ah);
+struct find_gid_index_context {
+ u16 vlan_id;
+};
+
+static bool find_gid_index(const union ib_gid *gid,
+ const struct ib_gid_attr *gid_attr,
+ void *context)
+{
+ struct find_gid_index_context *ctx =
+ (struct find_gid_index_context *)context;
+
+ if ((!!(ctx->vlan_id != 0x) == !is_vlan_dev(gid_attr->ndev)) ||
+ (is_vlan_dev(gid_attr->ndev) &&
+vlan_dev_vlan_id(gid_attr->ndev) != ctx->vlan_id))
+ return false;
+
+ return true;
+}
+
+static int get_sgid_index_from_eth(struct ib_device *device, u8 port_num,
+ u16 vlan_id, const union ib_gid *sgid,
+ u16 *gid_index)
+{
+ struct find_gid_index_context context = {.vlan_id = vlan_id};
+
+ return ib_find_gid_by_filter(device, sgid, port_num, find_gid_index,
+&context, gid_index);
+}
+
int ib_init_ah_from_wc(struct ib_device *device, u8 port_num,
const struct ib_wc *wc, const struct ib_grh *grh,
stru
