Message-ID: <b153383c-797c-42f6-801d-a6dcc7bfc4f7@stanley.mountain>
Date: Wed, 23 Oct 2024 22:06:26 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Naresh Kamboju <naresh.kamboju@...aro.org>
Cc: clang-built-linux <llvm@...ts.linux.dev>,
	open list <linux-kernel@...r.kernel.org>,
	lkft-triage@...ts.linaro.org,
	Linux Regressions <regressions@...ts.linux.dev>,
	Anders Roxell <anders.roxell@...aro.org>, kobak@...dia.com,
	"rafael.j.wysocki" <rafael.j.wysocki@...el.com>,
	Ard Biesheuvel <ardb@...nel.org>, rui.zhang@...el.com,
	mochs@...dia.com, Nathan Chancellor <nathan@...nel.org>
Subject: Re: 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]

> Config: https://storage.tuxsuite.com/public/linaro/lkft/builds/2npIm4ZOkWenPJ71UOZG57R0jXE/config

> 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_guid_t is alighned but guid_t is not.  I would have
thought that Clang would say that even though the alignment in for &th->guid
isn't spelled out explicitly, it would still end up being aligned at 8 bytes.

typedef guid_t efi_guid_t __aligned(__alignof__(u32));

The relevant code looks like this:

include/linux/uuid.h
    13  #define UUID_SIZE 16
    14  
    15  typedef struct {
    16          __u8 b[UUID_SIZE];
    17  } guid_t;

drivers/acpi/prmt.c
    54  struct prm_handler_info {
    55          guid_t guid;
    56          efi_status_t (__efiapi *handler_addr)(u64, void *);
    57          u64 static_data_buffer_addr;
    58          u64 acpi_param_buffer_addr;
    59  
    60          struct list_head handler_list;
    61  };
    62  
    63  struct prm_module_info {
    64          guid_t guid;
    65          u16 major_rev;
    66          u16 minor_rev;
    67          u16 handler_count;
    68          struct prm_mmio_info *mmio_info;
    69          bool updatable;
    70  
    71          struct list_head module_list;
    72          struct prm_handler_info handlers[] __counted_by(handler_count);
                                        ^^^^^^^^^^
This is a 64bit config so it's 8 byte aligned, right?

    73  };

[ snip ]

drivers/acpi/prmt.c
    96  static int __init
    97  acpi_parse_prmt(union acpi_subtable_headers *header, const unsigned long end)
    98  {
    99          struct acpi_prmt_module_info *module_info;
   100          struct acpi_prmt_handler_info *handler_info;
   101          struct prm_handler_info *th;
   102          struct prm_module_info *tm;
   103          u64 *mmio_count;
   104          u64 cur_handler = 0;
   105          u32 module_info_size = 0;
   106          u64 mmio_range_size = 0;
   107          void *temp_mmio;
   108  
   109          module_info = (struct acpi_prmt_module_info *) header;
   110          module_info_size = struct_size(tm, handlers, module_info->handler_info_count);
   111          tm = kmalloc(module_info_size, GFP_KERNEL);
   112          if (!tm)
   113                  goto parse_prmt_out1;
   114  
   115          guid_copy(&tm->guid, (guid_t *) module_info->module_guid);
   116          tm->major_rev = module_info->major_rev;
   117          tm->minor_rev = module_info->minor_rev;
   118          tm->handler_count = module_info->handler_info_count;
   119          tm->updatable = true;
   120  
   121          if (module_info->mmio_list_pointer) {
   122                  /*
   123                   * Each module is associated with a list of addr
   124                   * ranges that it can use during the service
   125                   */
   126                  mmio_count = (u64 *) memremap(module_info->mmio_list_pointer, 8, MEMREMAP_WB);
   127                  if (!mmio_count)
   128                          goto parse_prmt_out2;
   129  
   130                  mmio_range_size = struct_size(tm->mmio_info, addr_ranges, *mmio_count);
   131                  tm->mmio_info = kmalloc(mmio_range_size, GFP_KERNEL);
   132                  if (!tm->mmio_info)
   133                          goto parse_prmt_out3;
   134  
   135                  temp_mmio = memremap(module_info->mmio_list_pointer, mmio_range_size, MEMREMAP_WB);
   136                  if (!temp_mmio)
   137                          goto parse_prmt_out4;
   138                  memmove(tm->mmio_info, temp_mmio, mmio_range_size);
   139          } else {
   140                  tm->mmio_info = kmalloc(sizeof(*tm->mmio_info), GFP_KERNEL);
   141                  if (!tm->mmio_info)
   142                          goto parse_prmt_out2;
   143  
   144                  tm->mmio_info->mmio_count = 0;
   145          }
   146  
   147          INIT_LIST_HEAD(&tm->module_list);
   148          list_add(&tm->module_list, &prm_module_list);
   149  
   150          handler_info = get_first_handler(module_info);
   151          do {
   152                  th = &tm->handlers[cur_handler];
   153  
   154                  guid_copy(&th->guid, (guid_t *)handler_info->handler_guid);
   155                  th->handler_addr =
   156                          (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address);
                                                         ^^^^^^^^^
Here is the warning/build error.

   157  
   158                  th->static_data_buffer_addr =
   159                          efi_pa_va_lookup(&th->guid, handler_info->static_data_buffer_address);
   160  

regards,
dan carpenter

Powered by blists - more mailing lists