| Message ID | 1430515349-183903-1-git-send-email-david.ahern@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi, The patch is broken - you did not update > -----Original Message----- > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > owner@vger.kernel.org] On Behalf Of David Ahern > Sent: Saturday, May 02, 2015 12:22 AM <snipped> > > static int cm_compare_data(struct ib_cm_compare_data *src_data, > struct ib_cm_compare_data *dst_data) > { > - u8 src[IB_CM_COMPARE_SIZE]; > - u8 dst[IB_CM_COMPARE_SIZE]; > + u32 src[IB_CM_COMPARE_SIZE]; > + u32 dst[IB_CM_COMPARE_SIZE]; > > if (!src_data || !dst_data) > return 0; > @@ -460,10 +459,10 @@ static int cm_compare_data(struct > ib_cm_compare_data *src_data, > return memcmp(src, dst, IB_CM_COMPARE_SIZE); You added a BUG here - you should use (IB_CM_COMPARE_SIZE * sizeof(u32)). Same bug is also appearing at http://lxr.free-electrons.com/source/drivers/infiniband/core/cm.c#L472 http://lxr.free-electrons.com/source/drivers/infiniband/core/cm.c#L956 It might be better to keep IB_CM_COMPARE_SIZE to the original value, and divide by sizeof(u32) in the uses which iterate over a 32-bit data type. Thanks, --Shachar -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/3/15 4:20 AM, Shachar Raindel wrote: > Hi, > > The patch is broken - you did not update --verbose. I did not update what? > > >> -----Original Message----- >> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- >> owner@vger.kernel.org] On Behalf Of David Ahern >> Sent: Saturday, May 02, 2015 12:22 AM That is filled in by git and the time on the server is proper. So what do you disagree with? > > <snipped> > >> >> static int cm_compare_data(struct ib_cm_compare_data *src_data, >> struct ib_cm_compare_data *dst_data) >> { >> - u8 src[IB_CM_COMPARE_SIZE]; >> - u8 dst[IB_CM_COMPARE_SIZE]; >> + u32 src[IB_CM_COMPARE_SIZE]; >> + u32 dst[IB_CM_COMPARE_SIZE]; >> >> if (!src_data || !dst_data) >> return 0; >> @@ -460,10 +459,10 @@ static int cm_compare_data(struct >> ib_cm_compare_data *src_data, >> return memcmp(src, dst, IB_CM_COMPARE_SIZE); > > You added a BUG here - you should use (IB_CM_COMPARE_SIZE * sizeof(u32)). Thank you for pointing that out. Changed those to sizeof(src) and sizeof(compare_data->mask). -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: David Ahern [mailto:david.ahern@oracle.com] > > On 5/3/15 4:20 AM, Shachar Raindel wrote: > > Hi, > > > > The patch is broken - you did not update > > --verbose. I did not update what? Had a slight brain-body sync issue. Was meaning to point out that IB_CM_COMPARE_SIZE changed, but Some of its users were not updated. > > > > > >> -----Original Message----- > >> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > >> owner@vger.kernel.org] On Behalf Of David Ahern > >> Sent: Saturday, May 02, 2015 12:22 AM > > That is filled in by git and the time on the server is proper. So what > do you disagree with? > No disagreement there, was just meaning to help everyone find the relevant original e-mail without a doubt. My MTA is bit "special." > > > > <snipped> > > > >> > >> static int cm_compare_data(struct ib_cm_compare_data *src_data, > >> struct ib_cm_compare_data *dst_data) > >> { > >> - u8 src[IB_CM_COMPARE_SIZE]; > >> - u8 dst[IB_CM_COMPARE_SIZE]; > >> + u32 src[IB_CM_COMPARE_SIZE]; > >> + u32 dst[IB_CM_COMPARE_SIZE]; > >> > >> if (!src_data || !dst_data) > >> return 0; > >> @@ -460,10 +459,10 @@ static int cm_compare_data(struct > >> ib_cm_compare_data *src_data, > >> return memcmp(src, dst, IB_CM_COMPARE_SIZE); > > > > You added a BUG here - you should use (IB_CM_COMPARE_SIZE * > sizeof(u32)). > > Thank you for pointing that out. Changed those to sizeof(src) and > sizeof(compare_data->mask). > This sounds good. Thanks, --Shachar -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index e28a494e2a3a..0c276a53b8a8 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -437,20 +437,19 @@ static struct cm_id_private * cm_acquire_id(__be32 local_id, __be32 remote_id) return cm_id_priv; } -static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask) +static void cm_mask_copy(u32 *dst, const u32 *src, const u32 *mask) { int i; - for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(unsigned long); i++) - ((unsigned long *) dst)[i] = ((unsigned long *) src)[i] & - ((unsigned long *) mask)[i]; + for (i = 0; i < IB_CM_COMPARE_SIZE; i++) + dst[i] = src[i] & mask[i]; } static int cm_compare_data(struct ib_cm_compare_data *src_data, struct ib_cm_compare_data *dst_data) { - u8 src[IB_CM_COMPARE_SIZE]; - u8 dst[IB_CM_COMPARE_SIZE]; + u32 src[IB_CM_COMPARE_SIZE]; + u32 dst[IB_CM_COMPARE_SIZE]; if (!src_data || !dst_data) return 0; @@ -460,10 +459,10 @@ static int cm_compare_data(struct ib_cm_compare_data *src_data, return memcmp(src, dst, IB_CM_COMPARE_SIZE); } -static int cm_compare_private_data(u8 *private_data, +static int cm_compare_private_data(u32 *private_data, struct ib_cm_compare_data *dst_data) { - u8 src[IB_CM_COMPARE_SIZE]; + u32 src[IB_CM_COMPARE_SIZE]; if (!dst_data) return 0; @@ -538,7 +537,7 @@ static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv) static struct cm_id_private * cm_find_listen(struct ib_device *device, __be64 service_id, - u8 *private_data) + u32 *private_data) { struct rb_node *node = cm.listen_service_table.rb_node; struct cm_id_private *cm_id_priv; diff --git a/drivers/infiniband/core/cm_msgs.h b/drivers/infiniband/core/cm_msgs.h index be068f47e47e..8b76f0ef965e 100644 --- a/drivers/infiniband/core/cm_msgs.h +++ b/drivers/infiniband/core/cm_msgs.h @@ -103,7 +103,7 @@ struct cm_req_msg { /* local ACK timeout:5, rsvd:3 */ u8 alt_offset139; - u8 private_data[IB_CM_REQ_PRIVATE_DATA_SIZE]; + u32 private_data[IB_CM_REQ_PRIVATE_DATA_SIZE / sizeof(u32)]; } __attribute__ ((packed)); @@ -801,7 +801,7 @@ struct cm_sidr_req_msg { __be16 rsvd; __be64 service_id; - u8 private_data[IB_CM_SIDR_REQ_PRIVATE_DATA_SIZE]; + u32 private_data[IB_CM_SIDR_REQ_PRIVATE_DATA_SIZE / sizeof(u32)]; } __attribute__ ((packed)); struct cm_sidr_rep_msg { diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h index 0e3ff30647d5..39ed2d2fbd51 100644 --- a/include/rdma/ib_cm.h +++ b/include/rdma/ib_cm.h @@ -105,7 +105,8 @@ enum ib_cm_data_size { IB_CM_SIDR_REQ_PRIVATE_DATA_SIZE = 216, IB_CM_SIDR_REP_PRIVATE_DATA_SIZE = 136, IB_CM_SIDR_REP_INFO_LENGTH = 72, - IB_CM_COMPARE_SIZE = 64 + /* compare done u32 at a time */ + IB_CM_COMPARE_SIZE = (64 / sizeof(u32)) }; struct ib_cm_id; @@ -337,8 +338,8 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id); #define IB_SDP_SERVICE_ID_MASK cpu_to_be64(0xFFFFFFFFFFFF0000ULL) struct ib_cm_compare_data { - u8 data[IB_CM_COMPARE_SIZE]; - u8 mask[IB_CM_COMPARE_SIZE]; + u32 data[IB_CM_COMPARE_SIZE]; + u32 mask[IB_CM_COMPARE_SIZE]; }; /**
Addresses the following kernel logs seen during boot of sparc systems: Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Kernel unaligned access at TPC[103bce50] cm_find_listen+0x34/0xf8 [ib_cm] Signed-off-by: David Ahern <david.ahern@oracle.com> --- v3 - updated definition of IB_CM_COMPARE_SIZE to account for u32 walking per Yann's comment - added const to src/mask args to cm_mask_copy and removed extra whitespace per Bart's comments - changed 2 private_data declarations to u32 as suggested by Jason v2 - updated per Jason's comments drivers/infiniband/core/cm.c | 17 ++++++++--------- drivers/infiniband/core/cm_msgs.h | 4 ++-- include/rdma/ib_cm.h | 7 ++++--- 3 files changed, 14 insertions(+), 14 deletions(-)