Received-SPF: pass (google.com: domain of linux-kernel+bounces-142813-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <f7ca4107-0341-4631-8d8d-b9677782ac2f@xs4all.nl>
Date: Fri, 12 Apr 2024 16:21:40 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/3] media: dvb: Fix dtvs_stats packing.
Content-Language: en-US, nl
To: Ricardo Ribalda <ribalda@chromium.org>,
 Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240410-pack-v1-0-70f287dd8a66@chromium.org>
 <20240410-pack-v1-2-70f287dd8a66@chromium.org>
From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
In-Reply-To: <20240410-pack-v1-2-70f287dd8a66@chromium.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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