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

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

[David Carlier]

mali_c55_params_aexp_hist_weights() packs the 225 per-zone u8 weights
into the ISP registers four at a time by casting the zone_weights array
to u32 and dereferencing it. The array sits at offset 10 within the
parameter block, so it is only 2-byte aligned: the u32 access is
unaligned, which is undefined behaviour and can fault on strict-align
configurations or once the loop is auto-vectorised.

The cast also reads the four weights in host byte order before they are
written to the little-endian register, so on big-endian hosts the four
weights packed into each register end up in the wrong byte lanes.

Read the weights with get_unaligned_le32() instead, which is both
alignment-safe and fixes the byte order regardless of host endianness.

Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
Cc: stable@vger.kernel.org
Signed-off-by: David Carlier <devnexen@gmail.com>
---
 drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
index de0e9d898..1aaf64dde 100644
--- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
@@ -6,6 +6,7 @@
  */
 #include <linux/media/arm/mali-c55-config.h>
 #include <linux/pm_runtime.h>
+#include <linux/unaligned.h>
 
 #include <media/media-entity.h>
 #include <media/v4l2-dev.h>
@@ -203,7 +204,7 @@ mali_c55_params_aexp_hist_weights(struct mali_c55 *mali_c55,
 	 * of overwriting other registers.
 	 */
 	for (unsigned int i = 0; i < 56; i++) {
-		val = ((u32 *)params->zone_weights)[i]
+		val = get_unaligned_le32(&params->zone_weights[i * 4])
 			    & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK;
 		addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
 
-- 
2.53.0

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

[Jacopo Mondi]

Hi David,
   thanks for sending a patch to address this issue

On Thu, Jul 02, 2026 at 11:34:53AM +0100, David Carlier wrote:
> mali_c55_params_aexp_hist_weights() packs the 225 per-zone u8 weights
> into the ISP registers four at a time by casting the zone_weights array
> to u32 and dereferencing it. The array sits at offset 10 within the
> parameter block, so it is only 2-byte aligned: the u32 access is
> unaligned, which is undefined behaviour and can fault on strict-align
> configurations or once the loop is auto-vectorised.

well, I don't there is a risk of undefined behaviour on ARMv8, it's
just less efficient

>
> The cast also reads the four weights in host byte order before they are
> written to the little-endian register, so on big-endian hosts the four
> weights packed into each register end up in the wrong byte lanes.

Also we don't have any endianess issue as the IP is only found on
little endian systems

>
> Read the weights with get_unaligned_le32() instead, which is both
> alignment-safe and fixes the byte order regardless of host endianness.
>

mmm, I read in Documentation/core-api/unaligned-memory-access.rst
that:

------------------------------------------------------------------------------
	u32 value = get_unaligned((u32 *) data);

These macros work for memory accesses of any length (not just 32 bits as
in the examples above). Be aware that when compared to standard access of
aligned memory, using these macros to access unaligned memory can be costly in
terms of performance.

If use of such macros is not convenient, another option is to use memcpy(),
where the source or destination (or both) are of type u8* or unsigned char*.
Due to the byte-wise nature of this operation, unaligned accesses are avoided.
------------------------------------------------------------------------------

Which seems to suggest, if the issue here is performances, we should
aim for something different ? (honest question here, any kind of
guidance is appreciated)

> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> Cc: stable@vger.kernel.org

If it's only about performances, does this qualifies as a fix ?

> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
>  drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> index de0e9d898..1aaf64dde 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> @@ -6,6 +6,7 @@
>   */
>  #include <linux/media/arm/mali-c55-config.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/unaligned.h>
>
>  #include <media/media-entity.h>
>  #include <media/v4l2-dev.h>
> @@ -203,7 +204,7 @@ mali_c55_params_aexp_hist_weights(struct mali_c55 *mali_c55,
>  	 * of overwriting other registers.
>  	 */
>  	for (unsigned int i = 0; i < 56; i++) {
> -		val = ((u32 *)params->zone_weights)[i]
> +		val = get_unaligned_le32(&params->zone_weights[i * 4])
>  			    & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK;


We could do:

        memcpy(&val, &params->zone_weights[4 * i], 4);
        addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);

        mali_c55_ctx_write(mali_c55, addr,
                           val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);

Or this could be an alternative:

        const u8 *w = &params->zone_weights[4 * i];

        val = w[0] | w[1] << 8 | w[2] << 16 | w[3] << 24;
        addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);

        mali_c55_ctx_write(mali_c55, addr,
                           val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);

What do you think ?

>  		addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
>
> --
> 2.53.0
>
>

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

[David Laight]

On Fri, 3 Jul 2026 11:44:31 +0200
Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:

> Hi David,
>    thanks for sending a patch to address this issue
> 
> On Thu, Jul 02, 2026 at 11:34:53AM +0100, David Carlier wrote:
> > mali_c55_params_aexp_hist_weights() packs the 225 per-zone u8 weights
> > into the ISP registers four at a time by casting the zone_weights array
> > to u32 and dereferencing it. The array sits at offset 10 within the
> > parameter block, so it is only 2-byte aligned: the u32 access is
> > unaligned, which is undefined behaviour and can fault on strict-align
> > configurations or once the loop is auto-vectorised.  
> 
> well, I don't there is a risk of undefined behaviour on ARMv8, it's
> just less efficient
> 
> >
> > The cast also reads the four weights in host byte order before they are
> > written to the little-endian register, so on big-endian hosts the four
> > weights packed into each register end up in the wrong byte lanes.  
> 
> Also we don't have any endianess issue as the IP is only found on
> little endian systems
> 
> >
> > Read the weights with get_unaligned_le32() instead, which is both
> > alignment-safe and fixes the byte order regardless of host endianness.
> >  
> 
> mmm, I read in Documentation/core-api/unaligned-memory-access.rst
> that:
> 
> ------------------------------------------------------------------------------
> 	u32 value = get_unaligned((u32 *) data);
> 
> These macros work for memory accesses of any length (not just 32 bits as
> in the examples above). Be aware that when compared to standard access of
> aligned memory, using these macros to access unaligned memory can be costly in
> terms of performance.
> 
> If use of such macros is not convenient, another option is to use memcpy(),
> where the source or destination (or both) are of type u8* or unsigned char*.
> Due to the byte-wise nature of this operation, unaligned accesses are avoided.
> ------------------------------------------------------------------------------
> 
> Which seems to suggest, if the issue here is performances, we should
> aim for something different ? (honest question here, any kind of
> guidance is appreciated)
> 
> > Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> > Cc: stable@vger.kernel.org  
> 
> If it's only about performances, does this qualifies as a fix ?
> 
> > Signed-off-by: David Carlier <devnexen@gmail.com>
> > ---
> >  drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > index de0e9d898..1aaf64dde 100644
> > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > @@ -6,6 +6,7 @@
> >   */
> >  #include <linux/media/arm/mali-c55-config.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/unaligned.h>
> >
> >  #include <media/media-entity.h>
> >  #include <media/v4l2-dev.h>
> > @@ -203,7 +204,7 @@ mali_c55_params_aexp_hist_weights(struct mali_c55 *mali_c55,
> >  	 * of overwriting other registers.
> >  	 */
> >  	for (unsigned int i = 0; i < 56; i++) {
> > -		val = ((u32 *)params->zone_weights)[i]
> > +		val = get_unaligned_le32(&params->zone_weights[i * 4])
> >  			    & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK;  

On LE with HAVE_EFFICIENT_UNALIGNED_ACCESS the latter generates what you
expect the former to generate.
But gcc can unroll loops and use (IIRC) 'rdp' to read two registers at once.
That will crash and burn.

The best thing would be to have a union of the two arrays with the
member marked __packed to remove the padding before it.

> 
> 
> We could do:
> 
>         memcpy(&val, &params->zone_weights[4 * i], 4);

Some of the KASAN (etc) builds might make a mess of that.
Without compiler optimisations of memcpy() it is horrid.

>         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> 
>         mali_c55_ctx_write(mali_c55, addr,
>                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> 
> Or this could be an alternative:
> 
>         const u8 *w = &params->zone_weights[4 * i];
> 
>         val = w[0] | w[1] << 8 | w[2] << 16 | w[3] << 24;

That is a possible implementation of get_unaligned_le32() no point
doing it explicitly.

A late enough gcc will convert that to a 32bit memory read (with any
byteswap in the read or after) if unaligned accesses are supported.
Otherwise you get byte loads, shifts and ors.

	David

>         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> 
>         mali_c55_ctx_write(mali_c55, addr,
>                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> 
> What do you think ?
> 
> >  		addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> >
> > --
> > 2.53.0
> >
> >  
> 

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

[Jacopo Mondi]

Hi David

On Fri, Jul 03, 2026 at 10:16:51PM +0100, David Laight wrote:
> On Fri, 3 Jul 2026 11:44:31 +0200
> Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> > Hi David,
> >    thanks for sending a patch to address this issue
> >
> > On Thu, Jul 02, 2026 at 11:34:53AM +0100, David Carlier wrote:
> > > mali_c55_params_aexp_hist_weights() packs the 225 per-zone u8 weights
> > > into the ISP registers four at a time by casting the zone_weights array
> > > to u32 and dereferencing it. The array sits at offset 10 within the
> > > parameter block, so it is only 2-byte aligned: the u32 access is
> > > unaligned, which is undefined behaviour and can fault on strict-align
> > > configurations or once the loop is auto-vectorised.
> >
> > well, I don't there is a risk of undefined behaviour on ARMv8, it's
> > just less efficient
> >
> > >
> > > The cast also reads the four weights in host byte order before they are
> > > written to the little-endian register, so on big-endian hosts the four
> > > weights packed into each register end up in the wrong byte lanes.
> >
> > Also we don't have any endianess issue as the IP is only found on
> > little endian systems
> >
> > >
> > > Read the weights with get_unaligned_le32() instead, which is both
> > > alignment-safe and fixes the byte order regardless of host endianness.
> > >
> >
> > mmm, I read in Documentation/core-api/unaligned-memory-access.rst
> > that:
> >
> > ------------------------------------------------------------------------------
> > 	u32 value = get_unaligned((u32 *) data);
> >
> > These macros work for memory accesses of any length (not just 32 bits as
> > in the examples above). Be aware that when compared to standard access of
> > aligned memory, using these macros to access unaligned memory can be costly in
> > terms of performance.
> >
> > If use of such macros is not convenient, another option is to use memcpy(),
> > where the source or destination (or both) are of type u8* or unsigned char*.
> > Due to the byte-wise nature of this operation, unaligned accesses are avoided.
> > ------------------------------------------------------------------------------
> >
> > Which seems to suggest, if the issue here is performances, we should
> > aim for something different ? (honest question here, any kind of
> > guidance is appreciated)
> >
> > > Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> > > Cc: stable@vger.kernel.org
> >
> > If it's only about performances, does this qualifies as a fix ?
> >
> > > Signed-off-by: David Carlier <devnexen@gmail.com>
> > > ---
> > >  drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > index de0e9d898..1aaf64dde 100644
> > > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > @@ -6,6 +6,7 @@
> > >   */
> > >  #include <linux/media/arm/mali-c55-config.h>
> > >  #include <linux/pm_runtime.h>
> > > +#include <linux/unaligned.h>
> > >
> > >  #include <media/media-entity.h>
> > >  #include <media/v4l2-dev.h>
> > > @@ -203,7 +204,7 @@ mali_c55_params_aexp_hist_weights(struct mali_c55 *mali_c55,
> > >  	 * of overwriting other registers.
> > >  	 */
> > >  	for (unsigned int i = 0; i < 56; i++) {
> > > -		val = ((u32 *)params->zone_weights)[i]
> > > +		val = get_unaligned_le32(&params->zone_weights[i * 4])
> > >  			    & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK;
>
> On LE with HAVE_EFFICIENT_UNALIGNED_ACCESS the latter generates what you
> expect the former to generate.
> But gcc can unroll loops and use (IIRC) 'rdp' to read two registers at once.
> That will crash and burn.
>
> The best thing would be to have a union of the two arrays with the
> member marked __packed to remove the padding before it.
>

I'm not sure I got what are the "two arrays" you mentioned here.

params->zone_weights[] is uABI, it's hard to change its definition
without really good motivations.

> >
> >
> > We could do:
> >
> >         memcpy(&val, &params->zone_weights[4 * i], 4);
>
> Some of the KASAN (etc) builds might make a mess of that.
> Without compiler optimisations of memcpy() it is horrid.
>
> >         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> >
> >         mali_c55_ctx_write(mali_c55, addr,
> >                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> >
> > Or this could be an alternative:
> >
> >         const u8 *w = &params->zone_weights[4 * i];
> >
> >         val = w[0] | w[1] << 8 | w[2] << 16 | w[3] << 24;
>
> That is a possible implementation of get_unaligned_le32() no point
> doing it explicitly.
>
> A late enough gcc will convert that to a 32bit memory read (with any
> byteswap in the read or after) if unaligned accesses are supported.
> Otherwise you get byte loads, shifts and ors.
>

To sum it up: since we can't change uABI easily, the best thing here
is not change anything and drop this patch ?


> 	David
>
> >         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> >
> >         mali_c55_ctx_write(mali_c55, addr,
> >                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> >
> > What do you think ?
> >
> > >  		addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > >
> > > --
> > > 2.53.0
> > >
> > >
> >
>

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

[Laurent Pinchart]

On Mon, Jul 06, 2026 at 07:38:58AM +0200, Jacopo Mondi wrote:
> On Fri, Jul 03, 2026 at 10:16:51PM +0100, David Laight wrote:
> > On Fri, 3 Jul 2026 11:44:31 +0200 Jacopo Mondi wrote:
> > > On Thu, Jul 02, 2026 at 11:34:53AM +0100, David Carlier wrote:
> > > > mali_c55_params_aexp_hist_weights() packs the 225 per-zone u8 weights
> > > > into the ISP registers four at a time by casting the zone_weights array
> > > > to u32 and dereferencing it. The array sits at offset 10 within the
> > > > parameter block, so it is only 2-byte aligned: the u32 access is
> > > > unaligned, which is undefined behaviour and can fault on strict-align
> > > > configurations or once the loop is auto-vectorised.
> > >
> > > well, I don't there is a risk of undefined behaviour on ARMv8, it's
> > > just less efficient
> > >
> > > > The cast also reads the four weights in host byte order before they are
> > > > written to the little-endian register, so on big-endian hosts the four
> > > > weights packed into each register end up in the wrong byte lanes.
> > >
> > > Also we don't have any endianess issue as the IP is only found on
> > > little endian systems
> > >
> > > > Read the weights with get_unaligned_le32() instead, which is both
> > > > alignment-safe and fixes the byte order regardless of host endianness.
> > >
> > > mmm, I read in Documentation/core-api/unaligned-memory-access.rst
> > > that:
> > >
> > > ------------------------------------------------------------------------------
> > > 	u32 value = get_unaligned((u32 *) data);
> > >
> > > These macros work for memory accesses of any length (not just 32 bits as
> > > in the examples above). Be aware that when compared to standard access of
> > > aligned memory, using these macros to access unaligned memory can be costly in
> > > terms of performance.
> > >
> > > If use of such macros is not convenient, another option is to use memcpy(),
> > > where the source or destination (or both) are of type u8* or unsigned char*.
> > > Due to the byte-wise nature of this operation, unaligned accesses are avoided.
> > > ------------------------------------------------------------------------------
> > >
> > > Which seems to suggest, if the issue here is performances, we should
> > > aim for something different ? (honest question here, any kind of
> > > guidance is appreciated)
> > >
> > > > Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> > > > Cc: stable@vger.kernel.org
> > >
> > > If it's only about performances, does this qualifies as a fix ?
> > >
> > > > Signed-off-by: David Carlier <devnexen@gmail.com>
> > > > ---
> > > >  drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > index de0e9d898..1aaf64dde 100644
> > > > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > @@ -6,6 +6,7 @@
> > > >   */
> > > >  #include <linux/media/arm/mali-c55-config.h>
> > > >  #include <linux/pm_runtime.h>
> > > > +#include <linux/unaligned.h>
> > > >
> > > >  #include <media/media-entity.h>
> > > >  #include <media/v4l2-dev.h>
> > > > @@ -203,7 +204,7 @@ mali_c55_params_aexp_hist_weights(struct mali_c55 *mali_c55,
> > > >  	 * of overwriting other registers.
> > > >  	 */
> > > >  	for (unsigned int i = 0; i < 56; i++) {
> > > > -		val = ((u32 *)params->zone_weights)[i]
> > > > +		val = get_unaligned_le32(&params->zone_weights[i * 4])
> > > >  			    & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK;
> >
> > On LE with HAVE_EFFICIENT_UNALIGNED_ACCESS the latter generates what you
> > expect the former to generate.
> > But gcc can unroll loops and use (IIRC) 'rdp' to read two registers at once.
> > That will crash and burn.
> >
> > The best thing would be to have a union of the two arrays with the
> > member marked __packed to remove the padding before it.
> 
> I'm not sure I got what are the "two arrays" you mentioned here.
> 
> params->zone_weights[] is uABI, it's hard to change its definition
> without really good motivations.
> 
> > > We could do:
> > >
> > >         memcpy(&val, &params->zone_weights[4 * i], 4);
> >
> > Some of the KASAN (etc) builds might make a mess of that.
> > Without compiler optimisations of memcpy() it is horrid.
> >
> > >         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > >
> > >         mali_c55_ctx_write(mali_c55, addr,
> > >                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> > >
> > > Or this could be an alternative:
> > >
> > >         const u8 *w = &params->zone_weights[4 * i];
> > >
> > >         val = w[0] | w[1] << 8 | w[2] << 16 | w[3] << 24;
> >
> > That is a possible implementation of get_unaligned_le32() no point
> > doing it explicitly.
> >
> > A late enough gcc will convert that to a 32bit memory read (with any
> > byteswap in the read or after) if unaligned accesses are supported.
> > Otherwise you get byte loads, shifts and ors.
> 
> To sum it up: since we can't change uABI easily, the best thing here
> is not change anything and drop this patch ?

Doesn't the patch fix a real problem ?

Fixing the uABI would be best, but as you mentioned that's more
difficult (the faulty structure got merged recently in v6.19 and we
most likely control userspace, but still).

> > >         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > >
> > >         mali_c55_ctx_write(mali_c55, addr,
> > >                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> > >
> > > What do you think ?
> > >
> > > >  		addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > > >

-- 
Regards,

Laurent Pinchart

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

[David Laight]

On Mon, 6 Jul 2026 13:46:52 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> On Mon, Jul 06, 2026 at 07:38:58AM +0200, Jacopo Mondi wrote:
> > On Fri, Jul 03, 2026 at 10:16:51PM +0100, David Laight wrote:  
> > > On Fri, 3 Jul 2026 11:44:31 +0200 Jacopo Mondi wrote:  
> > > > On Thu, Jul 02, 2026 at 11:34:53AM +0100, David Carlier wrote:  
> > > > > mali_c55_params_aexp_hist_weights() packs the 225 per-zone u8 weights
> > > > > into the ISP registers four at a time by casting the zone_weights array
> > > > > to u32 and dereferencing it. The array sits at offset 10 within the
> > > > > parameter block, so it is only 2-byte aligned: the u32 access is
> > > > > unaligned, which is undefined behaviour and can fault on strict-align
> > > > > configurations or once the loop is auto-vectorised.  
> > > >
> > > > well, I don't there is a risk of undefined behaviour on ARMv8, it's
> > > > just less efficient
> > > >  
> > > > > The cast also reads the four weights in host byte order before they are
> > > > > written to the little-endian register, so on big-endian hosts the four
> > > > > weights packed into each register end up in the wrong byte lanes.  
> > > >
> > > > Also we don't have any endianess issue as the IP is only found on
> > > > little endian systems
> > > >  
> > > > > Read the weights with get_unaligned_le32() instead, which is both
> > > > > alignment-safe and fixes the byte order regardless of host endianness.  
> > > >
> > > > mmm, I read in Documentation/core-api/unaligned-memory-access.rst
> > > > that:
> > > >
> > > > ------------------------------------------------------------------------------
> > > > 	u32 value = get_unaligned((u32 *) data);
> > > >
> > > > These macros work for memory accesses of any length (not just 32 bits as
> > > > in the examples above). Be aware that when compared to standard access of
> > > > aligned memory, using these macros to access unaligned memory can be costly in
> > > > terms of performance.
> > > >
> > > > If use of such macros is not convenient, another option is to use memcpy(),
> > > > where the source or destination (or both) are of type u8* or unsigned char*.
> > > > Due to the byte-wise nature of this operation, unaligned accesses are avoided.
> > > > ------------------------------------------------------------------------------
> > > >
> > > > Which seems to suggest, if the issue here is performances, we should
> > > > aim for something different ? (honest question here, any kind of
> > > > guidance is appreciated)
> > > >  
> > > > > Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> > > > > Cc: stable@vger.kernel.org  
> > > >
> > > > If it's only about performances, does this qualifies as a fix ?
> > > >  
> > > > > Signed-off-by: David Carlier <devnexen@gmail.com>
> > > > > ---
> > > > >  drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > > index de0e9d898..1aaf64dde 100644
> > > > > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > > @@ -6,6 +6,7 @@
> > > > >   */
> > > > >  #include <linux/media/arm/mali-c55-config.h>
> > > > >  #include <linux/pm_runtime.h>
> > > > > +#include <linux/unaligned.h>
> > > > >
> > > > >  #include <media/media-entity.h>
> > > > >  #include <media/v4l2-dev.h>
> > > > > @@ -203,7 +204,7 @@ mali_c55_params_aexp_hist_weights(struct mali_c55 *mali_c55,
> > > > >  	 * of overwriting other registers.
> > > > >  	 */
> > > > >  	for (unsigned int i = 0; i < 56; i++) {
> > > > > -		val = ((u32 *)params->zone_weights)[i]
> > > > > +		val = get_unaligned_le32(&params->zone_weights[i * 4])
> > > > >  			    & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK;  
> > >
> > > On LE with HAVE_EFFICIENT_UNALIGNED_ACCESS the latter generates what you
> > > expect the former to generate.
> > > But gcc can unroll loops and use (IIRC) 'rdp' to read two registers at once.
> > > That will crash and burn.
> > >
> > > The best thing would be to have a union of the two arrays with the
> > > member marked __packed to remove the padding before it.  
> > 
> > I'm not sure I got what are the "two arrays" you mentioned here.
> > 
> > params->zone_weights[] is uABI, it's hard to change its definition
> > without really good motivations.
> >   
> > > > We could do:
> > > >
> > > >         memcpy(&val, &params->zone_weights[4 * i], 4);  
> > >
> > > Some of the KASAN (etc) builds might make a mess of that.
> > > Without compiler optimisations of memcpy() it is horrid.
> > >  
> > > >         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > > >
> > > >         mali_c55_ctx_write(mali_c55, addr,
> > > >                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> > > >
> > > > Or this could be an alternative:
> > > >
> > > >         const u8 *w = &params->zone_weights[4 * i];
> > > >
> > > >         val = w[0] | w[1] << 8 | w[2] << 16 | w[3] << 24;  
> > >
> > > That is a possible implementation of get_unaligned_le32() no point
> > > doing it explicitly.
> > >
> > > A late enough gcc will convert that to a 32bit memory read (with any
> > > byteswap in the read or after) if unaligned accesses are supported.
> > > Otherwise you get byte loads, shifts and ors.  
> > 
> > To sum it up: since we can't change uABI easily, the best thing here
> > is not change anything and drop this patch ?  
> 
> Doesn't the patch fix a real problem ?
> 
> Fixing the uABI would be best, but as you mentioned that's more
> difficult (the faulty structure got merged recently in v6.19 and we
> most likely control userspace, but still).

It is certainly possible to change how the structure is described without
changing the binary format.
eg:
	union {
		u8 zone_weights[256];
		u32 zone_weights_32[64] __attribute__((packed));
	};

Quite what has to happen on BE is another matter.

	David

> 
> > > >         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > > >
> > > >         mali_c55_ctx_write(mali_c55, addr,
> > > >                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> > > >
> > > > What do you think ?
> > > >  
> > > > >  		addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > > > >  
> 

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

[Jacopo Mondi]

Hi Laurent, David

On Mon, Jul 06, 2026 at 01:39:56PM +0100, David Laight wrote:
> On Mon, 6 Jul 2026 13:46:52 +0300
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>
> > On Mon, Jul 06, 2026 at 07:38:58AM +0200, Jacopo Mondi wrote:
> > > On Fri, Jul 03, 2026 at 10:16:51PM +0100, David Laight wrote:
> > > > On Fri, 3 Jul 2026 11:44:31 +0200 Jacopo Mondi wrote:
> > > > > On Thu, Jul 02, 2026 at 11:34:53AM +0100, David Carlier wrote:
> > > > > > mali_c55_params_aexp_hist_weights() packs the 225 per-zone u8 weights
> > > > > > into the ISP registers four at a time by casting the zone_weights array
> > > > > > to u32 and dereferencing it. The array sits at offset 10 within the
> > > > > > parameter block, so it is only 2-byte aligned: the u32 access is
> > > > > > unaligned, which is undefined behaviour and can fault on strict-align
> > > > > > configurations or once the loop is auto-vectorised.
> > > > >
> > > > > well, I don't there is a risk of undefined behaviour on ARMv8, it's
> > > > > just less efficient
> > > > >
> > > > > > The cast also reads the four weights in host byte order before they are
> > > > > > written to the little-endian register, so on big-endian hosts the four
> > > > > > weights packed into each register end up in the wrong byte lanes.
> > > > >
> > > > > Also we don't have any endianess issue as the IP is only found on
> > > > > little endian systems
> > > > >
> > > > > > Read the weights with get_unaligned_le32() instead, which is both
> > > > > > alignment-safe and fixes the byte order regardless of host endianness.
> > > > >
> > > > > mmm, I read in Documentation/core-api/unaligned-memory-access.rst
> > > > > that:
> > > > >
> > > > > ------------------------------------------------------------------------------
> > > > > 	u32 value = get_unaligned((u32 *) data);
> > > > >
> > > > > These macros work for memory accesses of any length (not just 32 bits as
> > > > > in the examples above). Be aware that when compared to standard access of
> > > > > aligned memory, using these macros to access unaligned memory can be costly in
> > > > > terms of performance.
> > > > >
> > > > > If use of such macros is not convenient, another option is to use memcpy(),
> > > > > where the source or destination (or both) are of type u8* or unsigned char*.
> > > > > Due to the byte-wise nature of this operation, unaligned accesses are avoided.
> > > > > ------------------------------------------------------------------------------
> > > > >
> > > > > Which seems to suggest, if the issue here is performances, we should
> > > > > aim for something different ? (honest question here, any kind of
> > > > > guidance is appreciated)
> > > > >
> > > > > > Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> > > > > > Cc: stable@vger.kernel.org
> > > > >
> > > > > If it's only about performances, does this qualifies as a fix ?
> > > > >
> > > > > > Signed-off-by: David Carlier <devnexen@gmail.com>
> > > > > > ---
> > > > > >  drivers/media/platform/arm/mali-c55/mali-c55-params.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > > > index de0e9d898..1aaf64dde 100644
> > > > > > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > > > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > > > > > @@ -6,6 +6,7 @@
> > > > > >   */
> > > > > >  #include <linux/media/arm/mali-c55-config.h>
> > > > > >  #include <linux/pm_runtime.h>
> > > > > > +#include <linux/unaligned.h>
> > > > > >
> > > > > >  #include <media/media-entity.h>
> > > > > >  #include <media/v4l2-dev.h>
> > > > > > @@ -203,7 +204,7 @@ mali_c55_params_aexp_hist_weights(struct mali_c55 *mali_c55,
> > > > > >  	 * of overwriting other registers.
> > > > > >  	 */
> > > > > >  	for (unsigned int i = 0; i < 56; i++) {
> > > > > > -		val = ((u32 *)params->zone_weights)[i]
> > > > > > +		val = get_unaligned_le32(&params->zone_weights[i * 4])
> > > > > >  			    & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK;
> > > >
> > > > On LE with HAVE_EFFICIENT_UNALIGNED_ACCESS the latter generates what you
> > > > expect the former to generate.
> > > > But gcc can unroll loops and use (IIRC) 'rdp' to read two registers at once.
> > > > That will crash and burn.
> > > >
> > > > The best thing would be to have a union of the two arrays with the
> > > > member marked __packed to remove the padding before it.
> > >
> > > I'm not sure I got what are the "two arrays" you mentioned here.
> > >
> > > params->zone_weights[] is uABI, it's hard to change its definition
> > > without really good motivations.
> > >
> > > > > We could do:
> > > > >
> > > > >         memcpy(&val, &params->zone_weights[4 * i], 4);
> > > >
> > > > Some of the KASAN (etc) builds might make a mess of that.
> > > > Without compiler optimisations of memcpy() it is horrid.
> > > >
> > > > >         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > > > >
> > > > >         mali_c55_ctx_write(mali_c55, addr,
> > > > >                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> > > > >
> > > > > Or this could be an alternative:
> > > > >
> > > > >         const u8 *w = &params->zone_weights[4 * i];
> > > > >
> > > > >         val = w[0] | w[1] << 8 | w[2] << 16 | w[3] << 24;
> > > >
> > > > That is a possible implementation of get_unaligned_le32() no point
> > > > doing it explicitly.
> > > >
> > > > A late enough gcc will convert that to a 32bit memory read (with any
> > > > byteswap in the read or after) if unaligned accesses are supported.
> > > > Otherwise you get byte loads, shifts and ors.
> > >
> > > To sum it up: since we can't change uABI easily, the best thing here
> > > is not change anything and drop this patch ?
> >
> > Doesn't the patch fix a real problem ?
> >

Does it ?

I'm still going under the assumption unaligned access on ARMv8 is
supported at the cost of loosing atomicity. Unfortunately, I can't
find any clear answer in

https://developer.arm.com/documentation/ddi0487/mc/-Part-B-The-AArch64-Application-Level-Architecture/-Chapter-B2-The-AArch64-Application-Level-Memory-Model/-B2-10-Memory-types-and-attributes/-B2-10-1-Normal-memory?lang=en

or
https://developer.arm.com/documentation/ddi0487/mc/-Part-B-The-AArch64-Application-Level-Architecture/-Chapter-B2-The-AArch64-Application-Level-Memory-Model/-B2-8-Alignment-support?lang=en

However, looking at HAVE_EFFICIENT_UNALIGNED_ACCESS

arch/Kconfig:config HAVE_EFFICIENT_UNALIGNED_ACCESS
arch/Kconfig-   bool
arch/Kconfig-   help
arch/Kconfig-     Some architectures are unable to perform unaligned accesses
arch/Kconfig-     without the use of get_unaligned/put_unaligned. Others are
arch/Kconfig-     unable to perform such accesses efficiently (e.g. trap on
arch/Kconfig-     unaligned access and require fixing it up in the exception
arch/Kconfig-     handler.)
arch/Kconfig-
arch/Kconfig-     This symbol should be selected by an architecture if it can
arch/Kconfig-     perform unaligned accesses efficiently to allow different
arch/Kconfig-     code paths to be selected for these cases. Some network
arch/Kconfig-     drivers, for example, could opt to not fix up alignment
arch/Kconfig-     problems with received packets if doing so would not help
arch/Kconfig-     much.
arch/Kconfig-
arch/Kconfig-     See Documentation/core-api/unaligned-memory-access.rst for more
arch/Kconfig-     information on the topic of unaligned memory accesses.

which is selected by arm64

$ git grep HAVE_EFFICIENT_UNALIGNED_ACCESS  arch/arm64/Kconfig
arch/arm64/Kconfig:     select HAVE_EFFICIENT_UNALIGNED_ACCESS

The symbol description defers to
Documentation/core-api/unaligned-memory-access.rst which in chapter

Code that causes unaligned access
=================================

seems to clarify this is a non-issue ?


> > Fixing the uABI would be best, but as you mentioned that's more
> > difficult (the faulty structure got merged recently in v6.19 and we
> > most likely control userspace, but still).
>
> It is certainly possible to change how the structure is described without
> changing the binary format.
> eg:
> 	union {
> 		u8 zone_weights[256];
> 		u32 zone_weights_32[64] __attribute__((packed));
> 	};

Maybe I mis-understood your suggestion, but I don't see why this would
change the offset at which zone_weights is placed in the struct which
contains it.

Running this through pahole (note I've s/64/56 as the zone_weights[]
array is 255 bytes)

struct mali_c55_params_aexp_weights {
	struct v4l2_isp_params_block_header header;
	__u8 nodes_used_horiz;
	__u8 nodes_used_vert;
	union {
		__u32 zone_weights_32[56] __attribute__((packed));
		__u8 zone_weights[MALI_C55_MAX_ZONES];
	};
};

I still see zone_weights[] at offset 10 which is not 4 bytes aligned.

struct mali_c55_params_aexp_weights {
	struct v4l2_isp_params_block_header header __attribute__((__aligned__(8))); /*     0     8 */
	__u8                       nodes_used_horiz;     /*     8     1 */
	__u8                       nodes_used_vert;      /*     9     1 */
	union {
		__u32              zone_weights_32[56];  /*    10   224 */
		__u8               zone_weights[225];    /*    10   225 */
	};                                               /*    10   225 */

	/* size: 240, cachelines: 4, members: 4 */
	/* padding: 5 */
	/* forced alignments: 1 */
	/* last cacheline: 48 bytes */
} __attribute__((__aligned__(8)));

What have I missed ?

>
> Quite what has to happen on BE is another matter.
>

I don't think BE is an issue for this IP.

Thanks
   j


> 	David
>
> >
> > > > >         addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > > > >
> > > > >         mali_c55_ctx_write(mali_c55, addr,
> > > > >                            val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
> > > > >
> > > > > What do you think ?
> > > > >
> > > > > >  		addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> > > > > >
> >
>

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

[David CARLIER]

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

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.

Cheers

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

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

[David CARLIER]

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

Alignment, not aliasing. zone_weights is a u8[] at offset 10, so it's
only 2-byte aligned, and casting that to a u32* (which wants 4) is
already UB - 6.3.2.3p7 - before you even load through it.

> the usage of __packed triggers the compiler to emit an 'LDUR'
> ... implications of using LDUR vs LDR on "unaligned access" ... not
> 100% clear to me.

LDUR vs LDR is only about how the offset is encoded, it's got nothing to
do with alignment safety. LDR's scaled form needs the immediate to be a
multiple of the access size, +10 isn't, so gcc can't use it and drops to
LDUR (unscaled offset). Both happily load from an unaligned address on
arm64 with SCTLR.A off - LDUR isn't "the unaligned one". The multiple-of-4
you found is about the immediate field, not the address.

So on arm64 __packed doesn't buy you a safer load, the plain cast already
worked. What it buys you is not lying to the compiler about the alignment
(so the UB is gone), plus correct codegen on the arches that do trap -
which is David's point.

> I would be a bit hesitant in changing the uAPI if there is actually
> nothing broken ... happy to defer

Fair enough, and you're right that nothing's actually broken - it's arm64
only so it never faults, this is tidy-up not a fix. I don't feel strongly
either way. Leave the uAPI as is and I'll drop it, or if you'd rather have
it cleaned up I'll send the packed union (with MALI_C55_MAX_ZONES / 4 like
David said). Whatever you prefer.

Cheers

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

[David Laight]

On Thu, 9 Jul 2026 20:16:51 +0100
David CARLIER <devnexen@gmail.com> wrote:

> > Out of curiosity: why is (u32 *) case a UB ?  
> 
> Alignment, not aliasing. zone_weights is a u8[] at offset 10, so it's
> only 2-byte aligned, and casting that to a u32* (which wants 4) is
> already UB - 6.3.2.3p7 - before you even load through it.
> 
> > the usage of __packed triggers the compiler to emit an 'LDUR'
> > ... implications of using LDUR vs LDR on "unaligned access" ... not
> > 100% clear to me.  
> 
> LDUR vs LDR is only about how the offset is encoded, it's got nothing to
> do with alignment safety. LDR's scaled form needs the immediate to be a
> multiple of the access size, +10 isn't, so gcc can't use it and drops to
> LDUR (unscaled offset). Both happily load from an unaligned address on
> arm64 with SCTLR.A off - LDUR isn't "the unaligned one". The multiple-of-4
> you found is about the immediate field, not the address.
> 
> So on arm64 __packed doesn't buy you a safer load, the plain cast already
> worked.

It stops the compiler using 'ldp' (to load two values) which IIRC will trap.
Although the function call following probably ensures that never happens
in this particular case.

	David

> What it buys you is not lying to the compiler about the alignment
> (so the UB is gone), plus correct codegen on the arches that do trap -
> which is David's point.
> 
> > I would be a bit hesitant in changing the uAPI if there is actually
> > nothing broken ... happy to defer  
> 
> Fair enough, and you're right that nothing's actually broken - it's arm64
> only so it never faults, this is tidy-up not a fix. I don't feel strongly
> either way. Leave the uAPI as is and I'll drop it, or if you'd rather have
> it cleaned up I'll send the packed union (with MALI_C55_MAX_ZONES / 4 like
> David said). Whatever you prefer.
> 
> Cheers

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

[David Laight]

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