[PATCH 0/3] media: Fix missing warnings for llvm am32

[PATCH 0/3] media: Fix missing warnings for llvm am32

[Ricardo Ribalda]

When we were building the project with llvm for arm32 a number of
warnings where shown.

This is passing the ABI check:
https://gitlab.freedesktop.org/linux-media/media-staging/-/jobs/57432144

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (3):
      media: dvb: as102-fe: Fix as10x_register_addr packing
      media: dvb: Fix dtvs_stats packing.
      media: videodev2: Fix v4l2_ext-control packing.

 drivers/media/dvb-frontends/as102_fe_types.h | 2 +-
 include/uapi/linux/dvb/frontend.h            | 2 +-
 include/uapi/linux/videodev2.h               | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
---
base-commit: d8c9a6e204f1466d228428174ce6d40ccdfb583e
change-id: 20240410-pack-e719cdfa0a6f

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH 1/3] media: dvb: as102-fe: Fix as10x_register_addr packing

[Ricardo Ribalda]

This structure is embedded in multiple other structures that are packed,
which conflicts with it being aligned.

drivers/media/usb/as102/as10x_cmd.h:379:30: warning: field reg_addr within 'struct as10x_dump_memory::(unnamed at drivers/media/usb/as102/as10x_cmd.h:373:2)' is less aligned than 'struct as10x_register_addr' and is usually due to 'struct as10x_dump_memory::(unnamed at drivers/media/usb/as102/as10x_cmd.h:373:2)' being packed, which can lead to unaligned accesses [-Wunaligned-access]

Mark it as being packed.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/dvb-frontends/as102_fe_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/as102_fe_types.h b/drivers/media/dvb-frontends/as102_fe_types.h
index 297f9520ebf9d..8a4e392c88965 100644
--- a/drivers/media/dvb-frontends/as102_fe_types.h
+++ b/drivers/media/dvb-frontends/as102_fe_types.h
@@ -174,6 +174,6 @@ struct as10x_register_addr {
 	uint32_t addr;
 	/* register mode access */
 	uint8_t mode;
-};
+} __packed;
 
 #endif

-- 
2.44.0.478.gd926399ef9-goog

Re: [PATCH 1/3] media: dvb: as102-fe: Fix as10x_register_addr packing

[Hans Verkuil]

On 10/04/2024 14:24, Ricardo Ribalda wrote:
> This structure is embedded in multiple other structures that are packed,
> which conflicts with it being aligned.
> 
> drivers/media/usb/as102/as10x_cmd.h:379:30: warning: field reg_addr within 'struct as10x_dump_memory::(unnamed at drivers/media/usb/as102/as10x_cmd.h:373:2)' is less aligned than 'struct as10x_register_addr' and is usually due to 'struct as10x_dump_memory::(unnamed at drivers/media/usb/as102/as10x_cmd.h:373:2)' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> 
> Mark it as being packed.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/dvb-frontends/as102_fe_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/dvb-frontends/as102_fe_types.h b/drivers/media/dvb-frontends/as102_fe_types.h
> index 297f9520ebf9d..8a4e392c88965 100644
> --- a/drivers/media/dvb-frontends/as102_fe_types.h
> +++ b/drivers/media/dvb-frontends/as102_fe_types.h
> @@ -174,6 +174,6 @@ struct as10x_register_addr {
>  	uint32_t addr;
>  	/* register mode access */
>  	uint8_t mode;
> -};
> +} __packed;

Changing the layout of a struct is scary, but in this case I believe
as10x_register_addr isn't actually used in this driver at all.

So this looks OK.

Regards,

	Hans

>  
>  #endif
> 

[PATCH 2/3] media: dvb: Fix dtvs_stats packing.

[Ricardo Ribalda]

The structure is packed, which requires that all its fields need to be
also packed.

./include/uapi/linux/dvb/frontend.h:854:2: warning: field  within 'struct dtv_stats' is less aligned than 'union dtv_stats::(anonymous at ./include/uapi/linux/dvb/frontend.h:854:2)' and is usually due to 'struct dtv_stats' being packed, which can lead to unaligned accesses [-Wunaligned-access]

Explicitly set the inner union as packed.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 include/uapi/linux/dvb/frontend.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
index 7e0983b987c2d..8d38c6befda8d 100644
--- a/include/uapi/linux/dvb/frontend.h
+++ b/include/uapi/linux/dvb/frontend.h
@@ -854,7 +854,7 @@ struct dtv_stats {
 	union {
 		__u64 uvalue;	/* for counters and relative scales */
 		__s64 svalue;	/* for 0.001 dB measures */
-	};
+	}  __attribute__ ((packed));
 } __attribute__ ((packed));
 
 

-- 
2.44.0.478.gd926399ef9-goog

Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

[Hans Verkuil]

On 10/04/2024 14:24, Ricardo Ribalda wrote:
> The structure is packed, which requires that all its fields need to be
> also packed.
> 
> ./include/uapi/linux/dvb/frontend.h:854:2: warning: field  within 'struct dtv_stats' is less aligned than 'union dtv_stats::(anonymous at ./include/uapi/linux/dvb/frontend.h:854:2)' and is usually due to 'struct dtv_stats' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> 
> Explicitly set the inner union as packed.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  include/uapi/linux/dvb/frontend.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
> index 7e0983b987c2d..8d38c6befda8d 100644
> --- a/include/uapi/linux/dvb/frontend.h
> +++ b/include/uapi/linux/dvb/frontend.h
> @@ -854,7 +854,7 @@ struct dtv_stats {
>  	union {
>  		__u64 uvalue;	/* for counters and relative scales */
>  		__s64 svalue;	/* for 0.001 dB measures */
> -	};
> +	}  __attribute__ ((packed));
>  } __attribute__ ((packed));

This is used in the public API, and I think this change can cause ABI changes.

Can you compare the layouts? Also between gcc and llvm since gcc never warned
about this.

I'm not going to accept this unless it is clear that there are no ABI changes.

Note that the ABI test in the build scripts only tests V4L2 at the moment,
not the DVB API.

Regards,

	Hans

Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

[Ricardo Ribalda]

Hi Hans

On Fri, 12 Apr 2024 at 16:21, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 10/04/2024 14:24, Ricardo Ribalda wrote:
> > The structure is packed, which requires that all its fields need to be
> > also packed.
> >
> > ./include/uapi/linux/dvb/frontend.h:854:2: warning: field  within 'struct dtv_stats' is less aligned than 'union dtv_stats::(anonymous at ./include/uapi/linux/dvb/frontend.h:854:2)' and is usually due to 'struct dtv_stats' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> >
> > Explicitly set the inner union as packed.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  include/uapi/linux/dvb/frontend.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
> > index 7e0983b987c2d..8d38c6befda8d 100644
> > --- a/include/uapi/linux/dvb/frontend.h
> > +++ b/include/uapi/linux/dvb/frontend.h
> > @@ -854,7 +854,7 @@ struct dtv_stats {
> >       union {
> >               __u64 uvalue;   /* for counters and relative scales */
> >               __s64 svalue;   /* for 0.001 dB measures */
> > -     };
> > +     }  __attribute__ ((packed));
> >  } __attribute__ ((packed));
>
> This is used in the public API, and I think this change can cause ABI changes.
>
> Can you compare the layouts? Also between gcc and llvm since gcc never warned
> about this.

The pahole output looks the same in both cases:

https://godbolt.org/z/oK4desv7Y
vs
https://godbolt.org/z/E36MjPr7v

And it is also the same for all the compiler versions that I tried.


struct dtv_stats {
uint8_t                    scale;                /*     0     1 */
union {
uint64_t           uvalue;               /*     1     8 */
int64_t            svalue;               /*     1     8 */
};                                               /*     1     8 */

/* size: 9, cachelines: 1, members: 2 */
/* last cacheline: 9 bytes */
} __attribute__((__packed__));



struct dtv_stats {
uint8_t scale; /* 0 1 */
union {
uint64_t uvalue; /* 1 8 */
int64_t svalue; /* 1 8 */
}; /* 1 8 */

/* size: 9, cachelines: 1, members: 2 */
/* last cacheline: 9 bytes */
} __attribute__((__packed__));


>
> I'm not going to accept this unless it is clear that there are no ABI changes.

Is there something else that I can try?

Regards!

>
> Note that the ABI test in the build scripts only tests V4L2 at the moment,
> not the DVB API.
>
> Regards,
>
>         Hans
>


-- 
Ricardo Ribalda

Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

[Hans Verkuil]

Hi Ricardo,

On 12/04/2024 17:00, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Fri, 12 Apr 2024 at 16:21, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 10/04/2024 14:24, Ricardo Ribalda wrote:
>>> The structure is packed, which requires that all its fields need to be
>>> also packed.
>>>
>>> ./include/uapi/linux/dvb/frontend.h:854:2: warning: field  within 'struct dtv_stats' is less aligned than 'union dtv_stats::(anonymous at ./include/uapi/linux/dvb/frontend.h:854:2)' and is usually due to 'struct dtv_stats' being packed, which can lead to unaligned accesses [-Wunaligned-access]
>>>
>>> Explicitly set the inner union as packed.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  include/uapi/linux/dvb/frontend.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
>>> index 7e0983b987c2d..8d38c6befda8d 100644
>>> --- a/include/uapi/linux/dvb/frontend.h
>>> +++ b/include/uapi/linux/dvb/frontend.h
>>> @@ -854,7 +854,7 @@ struct dtv_stats {
>>>       union {
>>>               __u64 uvalue;   /* for counters and relative scales */
>>>               __s64 svalue;   /* for 0.001 dB measures */
>>> -     };
>>> +     }  __attribute__ ((packed));
>>>  } __attribute__ ((packed));
>>
>> This is used in the public API, and I think this change can cause ABI changes.
>>
>> Can you compare the layouts? Also between gcc and llvm since gcc never warned
>> about this.
> 
> The pahole output looks the same in both cases:
> 
> https://godbolt.org/z/oK4desv7Y
> vs
> https://godbolt.org/z/E36MjPr7v
> 
> And it is also the same for all the compiler versions that I tried.
> 
> 
> struct dtv_stats {
> uint8_t                    scale;                /*     0     1 */
> union {
> uint64_t           uvalue;               /*     1     8 */
> int64_t            svalue;               /*     1     8 */
> };                                               /*     1     8 */
> 
> /* size: 9, cachelines: 1, members: 2 */
> /* last cacheline: 9 bytes */
> } __attribute__((__packed__));
> 
> 
> 
> struct dtv_stats {
> uint8_t scale; /* 0 1 */
> union {
> uint64_t uvalue; /* 1 8 */
> int64_t svalue; /* 1 8 */
> }; /* 1 8 */
> 
> /* size: 9, cachelines: 1, members: 2 */
> /* last cacheline: 9 bytes */
> } __attribute__((__packed__));
> 
> 
>>
>> I'm not going to accept this unless it is clear that there are no ABI changes.
> 
> Is there something else that I can try?

No, that's what I needed. I also found some clang discussions here:

https://github.com/llvm/llvm-project/issues/55520

I propose that I add the following sentence to these three packing patches:

"Marking the inner union as 'packed' does not change the layout, since the
whole struct is already packed, it just silences the clang warning. See
also this llvm discussion: https://github.com/llvm/llvm-project/issues/55520"

If you are OK with that, then I can add that to your patches.

Related to this: I added CEC and DVB support to the ABI checks in the build
scripts. And fixed a bunch of mistakes there (e.g. 'false=true' where I meant
to write 'fail=true'!) that made the ABI checks useless.

I updated the abi/* files accordingly as well.

Regards,

	Hans

> 
> Regards!
> 
>>
>> Note that the ABI test in the build scripts only tests V4L2 at the moment,
>> not the DVB API.
>>
>> Regards,
>>
>>         Hans
>>
> 
> 

Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.

[Ricardo Ribalda]

Hi Hans

On Mon, 15 Apr 2024 at 11:51, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Hi Ricardo,
>
> On 12/04/2024 17:00, Ricardo Ribalda wrote:
> > Hi Hans
> >
> > On Fri, 12 Apr 2024 at 16:21, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> On 10/04/2024 14:24, Ricardo Ribalda wrote:
> >>> The structure is packed, which requires that all its fields need to be
> >>> also packed.
> >>>
> >>> ./include/uapi/linux/dvb/frontend.h:854:2: warning: field  within 'struct dtv_stats' is less aligned than 'union dtv_stats::(anonymous at ./include/uapi/linux/dvb/frontend.h:854:2)' and is usually due to 'struct dtv_stats' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> >>>
> >>> Explicitly set the inner union as packed.
> >>>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>>  include/uapi/linux/dvb/frontend.h | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/uapi/linux/dvb/frontend.h b/include/uapi/linux/dvb/frontend.h
> >>> index 7e0983b987c2d..8d38c6befda8d 100644
> >>> --- a/include/uapi/linux/dvb/frontend.h
> >>> +++ b/include/uapi/linux/dvb/frontend.h
> >>> @@ -854,7 +854,7 @@ struct dtv_stats {
> >>>       union {
> >>>               __u64 uvalue;   /* for counters and relative scales */
> >>>               __s64 svalue;   /* for 0.001 dB measures */
> >>> -     };
> >>> +     }  __attribute__ ((packed));
> >>>  } __attribute__ ((packed));
> >>
> >> This is used in the public API, and I think this change can cause ABI changes.
> >>
> >> Can you compare the layouts? Also between gcc and llvm since gcc never warned
> >> about this.
> >
> > The pahole output looks the same in both cases:
> >
> > https://godbolt.org/z/oK4desv7Y
> > vs
> > https://godbolt.org/z/E36MjPr7v
> >
> > And it is also the same for all the compiler versions that I tried.
> >
> >
> > struct dtv_stats {
> > uint8_t                    scale;                /*     0     1 */
> > union {
> > uint64_t           uvalue;               /*     1     8 */
> > int64_t            svalue;               /*     1     8 */
> > };                                               /*     1     8 */
> >
> > /* size: 9, cachelines: 1, members: 2 */
> > /* last cacheline: 9 bytes */
> > } __attribute__((__packed__));
> >
> >
> >
> > struct dtv_stats {
> > uint8_t scale; /* 0 1 */
> > union {
> > uint64_t uvalue; /* 1 8 */
> > int64_t svalue; /* 1 8 */
> > }; /* 1 8 */
> >
> > /* size: 9, cachelines: 1, members: 2 */
> > /* last cacheline: 9 bytes */
> > } __attribute__((__packed__));
> >
> >
> >>
> >> I'm not going to accept this unless it is clear that there are no ABI changes.
> >
> > Is there something else that I can try?
>
> No, that's what I needed. I also found some clang discussions here:
>
> https://github.com/llvm/llvm-project/issues/55520
>
> I propose that I add the following sentence to these three packing patches:
>
> "Marking the inner union as 'packed' does not change the layout, since the
> whole struct is already packed, it just silences the clang warning. See
> also this llvm discussion: https://github.com/llvm/llvm-project/issues/55520"
>
> If you are OK with that, then I can add that to your patches.

That sounds great. Thanks!

>
> Related to this: I added CEC and DVB support to the ABI checks in the build
> scripts. And fixed a bunch of mistakes there (e.g. 'false=true' where I meant
> to write 'fail=true'!) that made the ABI checks useless.

Ferpect!, I will update it in media-ci

Thanks!
>
> I updated the abi/* files accordingly as well.
>
> Regards,
>
>         Hans
>
> >
> > Regards!
> >
> >>
> >> Note that the ABI test in the build scripts only tests V4L2 at the moment,
> >> not the DVB API.
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >
> >
>


-- 
Ricardo Ribalda

[PATCH 3/3] media: videodev2: Fix v4l2_ext-control packing.

[Ricardo Ribalda]

The structure is packed, which requires that all its fields need to be
also packed.

./include/uapi/linux/videodev2.h:1810:2: warning: field  within 'struct v4l2_ext_control' is less aligned than 'union v4l2_ext_control::(anonymous at ./include/uapi/linux/videodev2.h:1810:2)' and is usually due to 'struct v4l2_ext_control' being packed, which can lead to unaligned accesses [-Wunaligned-access]

Explicitly set the inner union as packed.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 include/uapi/linux/videodev2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 2663213b76a49..bf12860d570af 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1842,7 +1842,7 @@ struct v4l2_ext_control {
 		struct v4l2_ctrl_hdr10_cll_info __user *p_hdr10_cll_info;
 		struct v4l2_ctrl_hdr10_mastering_display __user *p_hdr10_mastering_display;
 		void __user *ptr;
-	};
+	} __attribute__ ((packed));
 } __attribute__ ((packed));
 
 struct v4l2_ext_controls {

-- 
2.44.0.478.gd926399ef9-goog