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

[Jacopo Mondi]

Hi David
   thanks for the investigation

On Thu, Jul 09, 2026 at 06:00:58AM +0100, David CARLIER 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

Out of curiosity: why is (u32 *) case a UB ?

> 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

ack

> 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

So, I run this through goldbot
https://godbolt.org/z/xTf8jd884

And it seems to me the usage of __packed triggers the compiler to emit
an 'LDUR' instruction instead of an LDR.

I'm reading a bit around
https://stackoverflow.com/questions/52894765/ldur-and-stur-in-arm-v8
https://developer.arm.com/documentation/dui0802/b/A64-Data-Transfer-Instructions/LDR--immediate-
https://developer.arm.com/documentation/dui0802/b/A64-Data-Transfer-Instructions/LDUR
and it seems to me that while less efficient LDUR is meant to support
byte-indexed access while LDR requires the indexing to be a multiple
of 4 or 8 bytes depending on the destination register.

What are the implications of using LDUR vs LDR on "unaligned access"
is however not 100% clear to me.

> concern about memcpy().
>
> So if you'd like it cleaned up, in mali-c55-config.h:
>
>       union {
>               __u32 zone_weights_32[56] __attribute__((__packed__));
>               __u8  zone_weights[MALI_C55_MAX_ZONES];
>       };
>
> and index zone_weights_32[i] in the driver. 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.

I would be a bit hesitant in changing the uAPI if there is actually
nothing broken, but I'm happy to defer the call to anyone who knows
best here :)

>
> Cheers