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

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Thu, Apr 30, 2015 at 10:42:48PM -0400, David Ahern wrote:
> 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]
> 
> Changed signature of cm_mask_copy from u8 to u32 as suggested by Jason
> Gunthorpe.

+1 to everyone elses comments..

> -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

> -	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

> -		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 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, eg what you trivially
posted:

> #define IB_CM_COMPARE_SIZE 64/sizeof(u32)

Is wrong, missing brackets.

>> 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.

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