Sparse bugs #97
Sparse bugs #97
Conversation
Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Sparse says the slid is BE in this context. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Based on sparse output. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
From sparse Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
The kernel uses the upper 32 bits of the TID for the agent ID (see drivers/infiniband/core/user_mad.c ib_umad_write) This worked on x86 because the TID is in BE at this point and the upper 32 bit masking is correct when working with BE data. Noticed by sparse. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
It looks like this didn't matter because this sin_port is only used for resolve_route which doesn't use sin_port data. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
It looks like endpoint->pkey is always used as host except for here. The routine always returns 0 on failure, so it is likely nobody noticed it was wrong. Found by sparse. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
|
@shefty, can't put you on the review list for some reason.. Please take a look Thnks |
|
I may need to be added to at least the reader list for the group to be added to any list. |
| @@ -656,7 +656,7 @@ acmp_record_mc_av(struct acmp_port *port, struct ib_mc_member_rec *mc_rec, | |||
| dest->path.dgid = mc_rec->mgid; | |||
| dest->path.sgid = mc_rec->port_gid; | |||
| dest->path.dlid = mc_rec->mlid; | |||
| dest->path.slid = htobe16(port->lid) | port->sa_dest.av.src_path_bits; | |||
| dest->path.slid = htobe16(port->lid | port->sa_dest.av.src_path_bits); | |||
There was a problem hiding this comment.
This (and the changes below) look correct but I think we should also change the declaration of dest->path to use be16_t (or __be64, what is the glibc equivalent?) defines to make it clear.
There was a problem hiding this comment.
I have more patches that add annotations, this series is just the bug fixes that change code behaviour.
| @@ -2419,8 +2419,8 @@ static struct acmp_port *acmp_get_port(struct acm_endpoint *endpoint) | |||
| struct acmp_device *dev; | |||
|
|
|||
| acm_log(1, "dev 0x%" PRIx64 " port %d pkey 0x%x\n", | |||
| endpoint->port->dev->dev_guid, endpoint->port->port_num, | |||
| endpoint->pkey); | |||
| be64toh(endpoint->port->dev->dev_guid), | |||
There was a problem hiding this comment.
same comment above... It is frustrating that ibv_get_device_guid returns uint64_t instead of a be64 variant.
| @@ -2458,7 +2459,7 @@ static uint16_t acmp_get_pkey_index(struct acm_endpoint *endpoint) | |||
|
|
|||
| for (i = 0, ret = 0; !ret; i++) { | |||
| ret = ibv_query_pkey(port->dev->verbs, port->port_num, i, &pkey); | |||
| if (!ret && endpoint->pkey == pkey) | |||
| if (!ret && endpoint->pkey == be16toh(pkey)) | |||
There was a problem hiding this comment.
gurgle - how did this ever work? i guess only for pkey index 0?
There was a problem hiding this comment.
This is the one patch I was most uncertain about.. There are lots of places in acm that store be and le values in the same struct member. However, I could not find any place that set endpoint->pkey to a BE value...
|
Merging now so I can test another -rc release. |
Now that Bart got sparse working these little bugs were detected by sparse.
They change behaviour so I think Sean/etc should look at rdma cm bits.