Re: [PATCH v2] IB/core: Fix unaligned accesses

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/1/15 10:24 AM, Jason Gunthorpe wrote:
-static void cm_mask_copy(u8 *dst, u8 *src, u8 *mask)
+static void cm_mask_copy(u32 *dst, u32 *src, u32 *mask)

masy as well add the const while here

noted by Bart.


-	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 / sizeof(u32); i++)
+		dst[i] =  src[i] & mask[i];

for (i = 0; i < IB_CM_COMPARE_SIZE / sizeof(*dst); i++)

Is a little more idiomatic

Sizing of the array using some macro / enum that accounts for u32 walking versus u8 walking means the sizeof goes away.


-		data_cmp = cm_compare_private_data(private_data,
+		data_cmp = cm_compare_private_data((u32 *) private_data,
  						   cm_id_priv->compare_data);

I'd be happier without this cast, at the worst move it up one more
layer, at the best, fix the two impacted structures as well.

I tried to fix private_data and it blows the patch up.


I feel like I stepped into a tar pit... It's a bit odd that
IB_CM_COMPARE_SIZE is part of an enum rather than a #define.

This is, again, idiomatic. '#define CONSTANT' is often frowned upon,
the semantics of enum are much cleaner/safer,

'#define NAME VALUE' is used throughout the kernel. Why does ib_rdma need to be different?

Anyway, you might want to compile this part of the kernel with
-Wcast-align and ignore the false positive (in particular
every other use of container_of()).

Why?

To look for more sketchy things that might impact SPARC, ARM, etc.

Seems like a topic for another patch.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux