[PATCH] ACPI: PRM: Clean Up guid type in struct prm_handler_info

[PATCH] ACPI: PRM: Clean Up guid type in struct prm_handler_info

[Dan Carpenter]

Clang 19 prints a warning when we pass &th->guid to efi_pa_va_lookup():

drivers/acpi/prmt.c:156:29: error: passing 1-byte aligned argument to
4-byte aligned parameter 1 of 'efi_pa_va_lookup' may result in an
unaligned pointer access [-Werror,-Walign-mismatch]
  156 |                         (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address);
      |                                                  ^

The problem is that efi_pa_va_lookup() takes a efi_guid_t and &th->guid
is a regular guid_t.  The difference between the two types is the
alignment.  efi_guid_t is a typedef.

	typedef guid_t efi_guid_t __aligned(__alignof__(u32));

It's possible that this a bug in Clang 19.  Even though the alignment of
&th->guid is not explicitly specified, it will still end up being aligned
at 4 or 8 bytes.

Anyway, as Ard points out, it's cleaner to change guid to efi_guid_t type
and that also makes the warning go away.

Fixes: 088984c8d54c ("ACPI: PRM: Find EFI_MEMORY_RUNTIME block for PRM handler and context")
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Tested-by: Paul E. McKenney <paulmck@kernel.org>
---
Sorry for the unfair Fixes tags since you obviously aren't to blame.  But it's
more practical if we avoid breaking the build in backports or etc.  Fixes tags
are quite often unfair in this way...

 drivers/acpi/prmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
index d59307a76ca3..747f83f7114d 100644
--- a/drivers/acpi/prmt.c
+++ b/drivers/acpi/prmt.c
@@ -52,7 +52,7 @@ struct prm_context_buffer {
 static LIST_HEAD(prm_module_list);
 
 struct prm_handler_info {
-	guid_t guid;
+	efi_guid_t guid;
 	efi_status_t (__efiapi *handler_addr)(u64, void *);
 	u64 static_data_buffer_addr;
 	u64 acpi_param_buffer_addr;
-- 
2.45.2

Re: [PATCH] ACPI: PRM: Clean Up guid type in struct prm_handler_info

[Ard Biesheuvel]

On Thu, 24 Oct 2024 at 10:07, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Clang 19 prints a warning when we pass &th->guid to efi_pa_va_lookup():
>
> drivers/acpi/prmt.c:156:29: error: passing 1-byte aligned argument to
> 4-byte aligned parameter 1 of 'efi_pa_va_lookup' may result in an
> unaligned pointer access [-Werror,-Walign-mismatch]
>   156 |                         (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address);
>       |                                                  ^
>
> The problem is that efi_pa_va_lookup() takes a efi_guid_t and &th->guid
> is a regular guid_t.  The difference between the two types is the
> alignment.  efi_guid_t is a typedef.
>
>         typedef guid_t efi_guid_t __aligned(__alignof__(u32));
>
> It's possible that this a bug in Clang 19.  Even though the alignment of
> &th->guid is not explicitly specified, it will still end up being aligned
> at 4 or 8 bytes.
>
> Anyway, as Ard points out, it's cleaner to change guid to efi_guid_t type
> and that also makes the warning go away.
>
> Fixes: 088984c8d54c ("ACPI: PRM: Find EFI_MEMORY_RUNTIME block for PRM handler and context")
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> Tested-by: Paul E. McKenney <paulmck@kernel.org>

In case this wasn't implied already,

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
> Sorry for the unfair Fixes tags since you obviously aren't to blame.  But it's
> more practical if we avoid breaking the build in backports or etc.  Fixes tags
> are quite often unfair in this way...
>
>  drivers/acpi/prmt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
> index d59307a76ca3..747f83f7114d 100644
> --- a/drivers/acpi/prmt.c
> +++ b/drivers/acpi/prmt.c
> @@ -52,7 +52,7 @@ struct prm_context_buffer {
>  static LIST_HEAD(prm_module_list);
>
>  struct prm_handler_info {
> -       guid_t guid;
> +       efi_guid_t guid;
>         efi_status_t (__efiapi *handler_addr)(u64, void *);
>         u64 static_data_buffer_addr;
>         u64 acpi_param_buffer_addr;
> --
> 2.45.2
>

Re: [PATCH] ACPI: PRM: Clean Up guid type in struct prm_handler_info

[Rafael J. Wysocki]

On Thu, Oct 24, 2024 at 12:40 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 24 Oct 2024 at 10:07, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > Clang 19 prints a warning when we pass &th->guid to efi_pa_va_lookup():
> >
> > drivers/acpi/prmt.c:156:29: error: passing 1-byte aligned argument to
> > 4-byte aligned parameter 1 of 'efi_pa_va_lookup' may result in an
> > unaligned pointer access [-Werror,-Walign-mismatch]
> >   156 |                         (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address);
> >       |                                                  ^
> >
> > The problem is that efi_pa_va_lookup() takes a efi_guid_t and &th->guid
> > is a regular guid_t.  The difference between the two types is the
> > alignment.  efi_guid_t is a typedef.
> >
> >         typedef guid_t efi_guid_t __aligned(__alignof__(u32));
> >
> > It's possible that this a bug in Clang 19.  Even though the alignment of
> > &th->guid is not explicitly specified, it will still end up being aligned
> > at 4 or 8 bytes.
> >
> > Anyway, as Ard points out, it's cleaner to change guid to efi_guid_t type
> > and that also makes the warning go away.
> >
> > Fixes: 088984c8d54c ("ACPI: PRM: Find EFI_MEMORY_RUNTIME block for PRM handler and context")
> > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> > Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Tested-by: Paul E. McKenney <paulmck@kernel.org>
>
> In case this wasn't implied already,
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>

Applied, thanks!

> > ---
> > Sorry for the unfair Fixes tags since you obviously aren't to blame.  But it's
> > more practical if we avoid breaking the build in backports or etc.  Fixes tags
> > are quite often unfair in this way...
> >
> >  drivers/acpi/prmt.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
> > index d59307a76ca3..747f83f7114d 100644
> > --- a/drivers/acpi/prmt.c
> > +++ b/drivers/acpi/prmt.c
> > @@ -52,7 +52,7 @@ struct prm_context_buffer {
> >  static LIST_HEAD(prm_module_list);
> >
> >  struct prm_handler_info {
> > -       guid_t guid;
> > +       efi_guid_t guid;
> >         efi_status_t (__efiapi *handler_addr)(u64, void *);
> >         u64 static_data_buffer_addr;
> >         u64 acpi_param_buffer_addr;
> > --