Received-SPF: pass (google.com: domain of linux-kernel+bounces-144912-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240410-pack-v1-0-70f287dd8a66@chromium.org> <20240410-pack-v1-2-70f287dd8a66@chromium.org>
 <f7ca4107-0341-4631-8d8d-b9677782ac2f@xs4all.nl> <CANiDSCvkRWZXuG7dfw0WXvgT+LHQqG3fx9F1M2P0_9dkB9VOKA@mail.gmail.com>
 <3f8660b0-e29c-47e2-b877-10da058388f9@xs4all.nl>
In-Reply-To: <3f8660b0-e29c-47e2-b877-10da058388f9@xs4all.nl>
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Mon, 15 Apr 2024 12:29:13 +0200
Message-ID: <CANiDSCuQoN0AKBDjYakHE6rhpBYymd_r8Gv-AEaOGiZmijN3Eg@mail.gmail.com>
Subject: Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.
To: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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> w=
rote:
> >>
> >> On 10/04/2024 14:24, Ricardo Ribalda wrote:
> >>> The structure is packed, which requires that all its fields need to b=
e
> >>> also packed.
> >>>
> >>> ./include/uapi/linux/dvb/frontend.h:854:2: warning: field  within 'st=
ruct dtv_stats' is less aligned than 'union dtv_stats::(anonymous at ./incl=
ude/uapi/linux/dvb/frontend.h:854:2)' and is usually due to 'struct dtv_sta=
ts' 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/d=
vb/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 patche=
s:
>
> "Marking the inner union as 'packed' does not change the layout, since th=
e
> whole struct is already packed, it just silences the clang warning. See
> also this llvm discussion: https://github.com/llvm/llvm-project/issues/55=
520"
>
> 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 bui=
ld
> scripts. And fixed a bunch of mistakes there (e.g. 'false=3Dtrue' where I=
 meant
> to write 'fail=3Dtrue'!) 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 mom=
ent,
> >> not the DVB API.
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >
> >
>


--=20
Ricardo Ribalda