Re: [PATCH] media: mali-c55: Fix unaligned access of AEC histogram zone weights

[David Laight <david.laight.linux@gmail.com>]

On Thu, 9 Jul 2026 06:00:58 +0100
David CARLIER <devnexen@gmail.com> wrote:

> > Does it ?  
> [...]
> > seems to clarify this is a non-issue ?  
> 
> I think you're right that there's no runtime fault: arm64 has
> HAVE_EFFICIENT_UNALIGNED_ACCESS and runs with SCTLR.A off, so the
> unaligned load doesn't trap. It's really just a C-level thing - the
> (u32 *) cast is UB and -fsanitize=alignment would moan - rather than a
> real bug, which is why v2 already dropped Fixes:/stable.
> 
> > I still see zone_weights[] at offset 10 which is not 4 bytes aligned.
> > What have I missed ?  
> 
> I don't think you missed anything - the union isn't trying to move the
> array, offset 10 has to stay. The idea is just the __packed member: it
> makes zone_weights_32[i] an alignment-1 read, so the compiler does the
> right thing (a plain LDR on arm64) with no cast, no get_unaligned() and
> no memcpy(). Same 240-byte layout, and it also avoids David's KASAN
> concern about memcpy().

I think you'll also find that gcc will generate a real call to memcpy()
on both sparc64 and riscv64 (and possibly all architectures that fault
misaligned accesses) even if the char[] is at an aligned structure offset.
Either that or, if you include the (u32) cast, it will assume the pointer
is a valid 'u32' pointer and generate faulting misaligned access. 
So while this is arm64 specific code it is a bad idea in general.

> 
> So if you'd like it cleaned up, in mali-c55-config.h:
> 
>       union {
>               __u32 zone_weights_32[56] __attribute__((__packed__));

Should be MALI_C55_MAX_ZONES/4.

>               __u8  zone_weights[MALI_C55_MAX_ZONES];
>       };
> 
> and index zone_weights_32[i] in the driver.

That is the safe way to do it.

Even on x86 I've fallen foul of misaligned data traps when gcc has
used simd instructions to unroll a loop.
I knew the buffer could be unaligned, but needed a sum of all the
32bit words (to set a checksum). Worked find until it didn't...

	David


> And if you'd rather not
> carry the uapi churn for something that isn't a fault, I'm equally happy
> to just drop it - whichever you prefer.
> 
> Cheers