| Message ID | 20231114-arm-build-bug-v1-1-458745fe32a4@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: acenv: Permit compilation from within the kernel | expand |
On 11/13/23 16:08, Linus Walleij wrote: > After commit a103f46633fd the kernel stopped compiling for > several ARM32 platforms that I am building with a bare metal > compiler. Bare metal compilers (arm-none-eabi-) don't > define __linux__. Hi Linus, I saw the same baremetal-compiler error here on the ARM64 side of the fence, and narrowed the problem to the same commit as you. > > This is because the header <acpi/platform/acenv.h> is now > in the include path for <linux/irq.h>: More generally, I think it's because of this addition to linux/acpi.h: +#include <linux/fw_table.h> linux/acpi.h is supposed to ensure _LINUX is defined (if it isn't already done by a non-baremetal compiler) before we start pulling in ACPICA includes, so that ACPICA knows the platform. But because fw_table.h contains: #include <linux/acpi.h> #include <acpi/acpi.h> ...the circular include does nothing (linux/acpi.h's include guard stops the include before _LINUX is defined) and we end up pulling in acpi/acpi.h before we're ready. > > CC arch/arm/kernel/irq.o > CC kernel/sysctl.o > CC crypto/api.o > In file included from ../include/acpi/acpi.h:22, > from ../include/linux/fw_table.h:29, > from ../include/linux/acpi.h:18, > from ../include/linux/irqchip.h:14, > from ../arch/arm/kernel/irq.c:25: > ../include/acpi/platform/acenv.h:218:2: error: #error Unknown target environment > 218 | #error Unknown target environment > | ^~~~~ > > One solution to make compilation with a bare metal compiler > work again is to say the file is used with Linux from within > the kernel if __KERNEL__ is defined so I did that. I am not an ACPI subsystem maintainer, but my understanding is that the files in include/acpi/ are copied verbatim from ACPICA, so any change to those files will have to be sent to the ACPICA project and wouldn't be accepted here. More likely, we'd want to do something about the circular-include situation between linux/fw_table.h<->linux/acpi.h. That may have further consequences down the road than just our problem here. Perhaps just dropping both #includes from fw_table.h, and lowering the fw_table.h include from within linux/acpi.h to be below <acpi/acpi.h>, is the way to go? Kind regards, Sam
On Tue, Nov 14, 2023 at 7:09 PM Sam Edwards <cfsworks@gmail.com> wrote: > I am not an ACPI subsystem maintainer, but my understanding is that the > files in include/acpi/ are copied verbatim from ACPICA, so any change to > those files will have to be sent to the ACPICA project and wouldn't be > accepted here. > > More likely, we'd want to do something about the circular-include > situation between linux/fw_table.h<->linux/acpi.h. I agree but I have no idea how to fix that really, should I just send a revert instead so the authors can get some time to figure it out? Yours, Linus Walleij
Hi Linus, On Tue, Nov 14, 2023 at 2:20 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Nov 14, 2023 at 7:09 PM Sam Edwards <cfsworks@gmail.com> wrote: > > > I am not an ACPI subsystem maintainer, but my understanding is that the > > files in include/acpi/ are copied verbatim from ACPICA, so any change to > > those files will have to be sent to the ACPICA project and wouldn't be > > accepted here. > > > > More likely, we'd want to do something about the circular-include > > situation between linux/fw_table.h<->linux/acpi.h. > > I agree but I have no idea how to fix that really, should I just send > a revert instead so the authors can get some time to figure it out? > Just want to confirm that linux-mainline and linux-next builds are broken for my ARM64 board as well, because of the commit you pin-pointed. I vote for reverting it and letting the author rework it properly. On a side note: I'm surprised there are no bots or automatic CI builds out there testing the kernel builds with baremetal toolchains. Can't believe everyone's using Linux toolchain, the kernel is supposed to be baremetal project. > Yours, > Linus Walleij
On Tue, Nov 14, 2023 at 7:09 PM Sam Edwards <cfsworks@gmail.com> wrote: > > On 11/13/23 16:08, Linus Walleij wrote: > > After commit a103f46633fd the kernel stopped compiling for > > several ARM32 platforms that I am building with a bare metal > > compiler. Bare metal compilers (arm-none-eabi-) don't > > define __linux__. > > Hi Linus, > > I saw the same baremetal-compiler error here on the ARM64 side of the > fence, and narrowed the problem to the same commit as you. > > > > > This is because the header <acpi/platform/acenv.h> is now > > in the include path for <linux/irq.h>: > > More generally, I think it's because of this addition to linux/acpi.h: > +#include <linux/fw_table.h> > > linux/acpi.h is supposed to ensure _LINUX is defined (if it isn't > already done by a non-baremetal compiler) before we start pulling in > ACPICA includes, so that ACPICA knows the platform. But because > fw_table.h contains: > #include <linux/acpi.h> > #include <acpi/acpi.h> > > ...the circular include does nothing (linux/acpi.h's include guard stops > the include before _LINUX is defined) and we end up pulling in > acpi/acpi.h before we're ready. Yes, that's the problem AFAICS. Dave? What about moving the fw_table.h include in linux/acpi.h below the mutex.h one, along with the EXPORT_SYMBOL_ACPI_LIB-related definitions? BTW, I'm not really sure why fw_table.h needs to include both linux/acpi.h and acpi/acpi.h, because it should get the latter through the former anyway. > > > > CC arch/arm/kernel/irq.o > > CC kernel/sysctl.o > > CC crypto/api.o > > In file included from ../include/acpi/acpi.h:22, > > from ../include/linux/fw_table.h:29, > > from ../include/linux/acpi.h:18, > > from ../include/linux/irqchip.h:14, > > from ../arch/arm/kernel/irq.c:25: > > ../include/acpi/platform/acenv.h:218:2: error: #error Unknown target environment > > 218 | #error Unknown target environment > > | ^~~~~ > > > > One solution to make compilation with a bare metal compiler > > work again is to say the file is used with Linux from within > > the kernel if __KERNEL__ is defined so I did that. > > I am not an ACPI subsystem maintainer, but my understanding is that the > files in include/acpi/ are copied verbatim from ACPICA, so any change to > those files will have to be sent to the ACPICA project and wouldn't be > accepted here. There are exceptions, but generally you are right. > More likely, we'd want to do something about the circular-include > situation between linux/fw_table.h<->linux/acpi.h. That may have further > consequences down the road than just our problem here. Perhaps just > dropping both #includes from fw_table.h, and lowering the fw_table.h > include from within linux/acpi.h to be below <acpi/acpi.h>, is the way > to go? Something like that.
On 11/20/23 08:46, Rafael J. Wysocki wrote: > On Tue, Nov 14, 2023 at 7:09 PM Sam Edwards <cfsworks@gmail.com> wrote: >> >> On 11/13/23 16:08, Linus Walleij wrote: >>> After commit a103f46633fd the kernel stopped compiling for >>> several ARM32 platforms that I am building with a bare metal >>> compiler. Bare metal compilers (arm-none-eabi-) don't >>> define __linux__. >> >> Hi Linus, >> >> I saw the same baremetal-compiler error here on the ARM64 side of the >> fence, and narrowed the problem to the same commit as you. >> >>> >>> This is because the header <acpi/platform/acenv.h> is now >>> in the include path for <linux/irq.h>: >> >> More generally, I think it's because of this addition to linux/acpi.h: >> +#include <linux/fw_table.h> >> >> linux/acpi.h is supposed to ensure _LINUX is defined (if it isn't >> already done by a non-baremetal compiler) before we start pulling in >> ACPICA includes, so that ACPICA knows the platform. But because >> fw_table.h contains: >> #include <linux/acpi.h> >> #include <acpi/acpi.h> >> >> ...the circular include does nothing (linux/acpi.h's include guard stops >> the include before _LINUX is defined) and we end up pulling in >> acpi/acpi.h before we're ready. Not including either causes compile errors for me. And directly including acpi/acpi.h w/o linux/acpi.h causes triggering the #error and some other stuff: ./include/acpi/platform/aclinux.h:18:2: error: #error "Please don't include <acpi/acpi.h> directly, include <linux/acpi.h> instead." 18 | #error "Please don't include <acpi/acpi.h> directly, include <linux/acpi.h> instead." | ^~~~~ Only including linux/acpi.h: In file included from ./include/linux/acpi.h:18, from init/main.c:30: ./include/linux/fw_table.h:32:37: error: field ‘common’ has incomplete type 32 | struct acpi_subtable_header common; | ^~~~~~ ./include/linux/fw_table.h:33:36: error: field ‘hmat’ has incomplete type 33 | struct acpi_hmat_structure hmat; | ^~~~ ./include/linux/fw_table.h:34:40: error: field ‘prmt’ has incomplete type 34 | struct acpi_prmt_module_header prmt; | ^~~~ ./include/linux/fw_table.h:35:33: error: field ‘cedt’ has incomplete type 35 | struct acpi_cedt_header cedt; | ^~~~ > > Yes, that's the problem AFAICS. Dave? > > What about moving the fw_table.h include in linux/acpi.h below the > mutex.h one, along with the EXPORT_SYMBOL_ACPI_LIB-related > definitions? This builds cleanly for me. > > BTW, I'm not really sure why fw_table.h needs to include both > linux/acpi.h and acpi/acpi.h, because it should get the latter through > the former anyway. > >>> >>> CC arch/arm/kernel/irq.o >>> CC kernel/sysctl.o >>> CC crypto/api.o >>> In file included from ../include/acpi/acpi.h:22, >>> from ../include/linux/fw_table.h:29, >>> from ../include/linux/acpi.h:18, >>> from ../include/linux/irqchip.h:14, >>> from ../arch/arm/kernel/irq.c:25: >>> ../include/acpi/platform/acenv.h:218:2: error: #error Unknown target environment >>> 218 | #error Unknown target environment >>> | ^~~~~ >>> >>> One solution to make compilation with a bare metal compiler >>> work again is to say the file is used with Linux from within >>> the kernel if __KERNEL__ is defined so I did that. >> >> I am not an ACPI subsystem maintainer, but my understanding is that the >> files in include/acpi/ are copied verbatim from ACPICA, so any change to >> those files will have to be sent to the ACPICA project and wouldn't be >> accepted here. > > There are exceptions, but generally you are right. > >> More likely, we'd want to do something about the circular-include >> situation between linux/fw_table.h<->linux/acpi.h. That may have further >> consequences down the road than just our problem here. Perhaps just >> dropping both #includes from fw_table.h, and lowering the fw_table.h >> include from within linux/acpi.h to be below <acpi/acpi.h>, is the way >> to go? > > Something like that.
On Mon, Nov 20, 2023 at 5:19 PM Dave Jiang <dave.jiang@intel.com> wrote: > > > > On 11/20/23 08:46, Rafael J. Wysocki wrote: > > On Tue, Nov 14, 2023 at 7:09 PM Sam Edwards <cfsworks@gmail.com> wrote: > >> > >> On 11/13/23 16:08, Linus Walleij wrote: > >>> After commit a103f46633fd the kernel stopped compiling for > >>> several ARM32 platforms that I am building with a bare metal > >>> compiler. Bare metal compilers (arm-none-eabi-) don't > >>> define __linux__. > >> > >> Hi Linus, > >> > >> I saw the same baremetal-compiler error here on the ARM64 side of the > >> fence, and narrowed the problem to the same commit as you. > >> > >>> > >>> This is because the header <acpi/platform/acenv.h> is now > >>> in the include path for <linux/irq.h>: > >> > >> More generally, I think it's because of this addition to linux/acpi.h: > >> +#include <linux/fw_table.h> > >> > >> linux/acpi.h is supposed to ensure _LINUX is defined (if it isn't > >> already done by a non-baremetal compiler) before we start pulling in > >> ACPICA includes, so that ACPICA knows the platform. But because > >> fw_table.h contains: > >> #include <linux/acpi.h> > >> #include <acpi/acpi.h> > >> > >> ...the circular include does nothing (linux/acpi.h's include guard stops > >> the include before _LINUX is defined) and we end up pulling in > >> acpi/acpi.h before we're ready. > > Not including either causes compile errors for me. Interesting. What errors do you get if you include linux/acpi.h only? It should not be necessary to include acpi/acpi.h in addition to linux/acpi.h, because the latter is expected to include the former. If it doesn't do that, something is amiss. > And directly including acpi/acpi.h w/o linux/acpi.h causes triggering the #error and some other stuff: > > ./include/acpi/platform/aclinux.h:18:2: error: #error "Please don't include <acpi/acpi.h> directly, include <linux/acpi.h> instead." > 18 | #error "Please don't include <acpi/acpi.h> directly, include <linux/acpi.h> instead." > | ^~~~~ > > > Only including linux/acpi.h: > In file included from ./include/linux/acpi.h:18, > from init/main.c:30: > ./include/linux/fw_table.h:32:37: error: field ‘common’ has incomplete type > 32 | struct acpi_subtable_header common; > | ^~~~~~ > ./include/linux/fw_table.h:33:36: error: field ‘hmat’ has incomplete type > 33 | struct acpi_hmat_structure hmat; > | ^~~~ > ./include/linux/fw_table.h:34:40: error: field ‘prmt’ has incomplete type > 34 | struct acpi_prmt_module_header prmt; > | ^~~~ > ./include/linux/fw_table.h:35:33: error: field ‘cedt’ has incomplete type > 35 | struct acpi_cedt_header cedt; > | ^~~~ > > > > > > Yes, that's the problem AFAICS. Dave? > > > > What about moving the fw_table.h include in linux/acpi.h below the > > mutex.h one, along with the EXPORT_SYMBOL_ACPI_LIB-related > > definitions? > > This builds cleanly for me. OK, so I'm wondering if this also helps the other people in this thread.
On 11/20/23 09:38, Rafael J. Wysocki wrote: > On Mon, Nov 20, 2023 at 5:19 PM Dave Jiang <dave.jiang@intel.com> wrote: >> >> >> >> On 11/20/23 08:46, Rafael J. Wysocki wrote: >>> On Tue, Nov 14, 2023 at 7:09 PM Sam Edwards <cfsworks@gmail.com> wrote: >>>> >>>> On 11/13/23 16:08, Linus Walleij wrote: >>>>> After commit a103f46633fd the kernel stopped compiling for >>>>> several ARM32 platforms that I am building with a bare metal >>>>> compiler. Bare metal compilers (arm-none-eabi-) don't >>>>> define __linux__. >>>> >>>> Hi Linus, >>>> >>>> I saw the same baremetal-compiler error here on the ARM64 side of the >>>> fence, and narrowed the problem to the same commit as you. >>>> >>>>> >>>>> This is because the header <acpi/platform/acenv.h> is now >>>>> in the include path for <linux/irq.h>: >>>> >>>> More generally, I think it's because of this addition to linux/acpi.h: >>>> +#include <linux/fw_table.h> >>>> >>>> linux/acpi.h is supposed to ensure _LINUX is defined (if it isn't >>>> already done by a non-baremetal compiler) before we start pulling in >>>> ACPICA includes, so that ACPICA knows the platform. But because >>>> fw_table.h contains: >>>> #include <linux/acpi.h> >>>> #include <acpi/acpi.h> >>>> >>>> ...the circular include does nothing (linux/acpi.h's include guard stops >>>> the include before _LINUX is defined) and we end up pulling in >>>> acpi/acpi.h before we're ready. >> >> Not including either causes compile errors for me. > > Interesting. What errors do you get if you include linux/acpi.h only? > > It should not be necessary to include acpi/acpi.h in addition to > linux/acpi.h, because the latter is expected to include the former. > If it doesn't do that, something is amiss. > CC arch/x86/video/fbdev.o In file included from ./include/linux/acpi.h:18, from ./include/linux/tpm.h:21, from ./include/keys/trusted-type.h:12, from security/keys/encrypted-keys/encrypted.c:22: ./include/linux/fw_table.h:32:37: error: field ‘common’ has incomplete type 32 | struct acpi_subtable_header common; | ^~~~~~ ./include/linux/fw_table.h:33:36: error: field ‘hmat’ has incomplete type 33 | struct acpi_hmat_structure hmat; | ^~~~ ./include/linux/fw_table.h:34:40: error: field ‘prmt’ has incomplete type 34 | struct acpi_prmt_module_header prmt; | ^~~~ ./include/linux/fw_table.h:35:33: error: field ‘cedt’ has incomplete type 35 | struct acpi_cedt_header cedt; | ^~~~ However, if I move fw_table.h in linux/acpi.h below include of asm/acpi.h, then we can build successfully w/o including acpi/acpi.h in fw_table.h. diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 54189e0e5f41..2789beb26138 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -15,7 +15,6 @@ #include <linux/mod_devicetable.h> #include <linux/property.h> #include <linux/uuid.h> -#include <linux/fw_table.h> struct irq_domain; struct irq_domain_ops; @@ -25,16 +24,6 @@ struct irq_domain_ops; #endif #include <acpi/acpi.h> -#ifdef CONFIG_ACPI_TABLE_LIB -#define EXPORT_SYMBOL_ACPI_LIB(x) EXPORT_SYMBOL_NS_GPL(x, ACPI) -#define __init_or_acpilib -#define __initdata_or_acpilib -#else -#define EXPORT_SYMBOL_ACPI_LIB(x) -#define __init_or_acpilib __init -#define __initdata_or_acpilib __initdata -#endif - #ifdef CONFIG_ACPI #include <linux/list.h> @@ -48,6 +37,18 @@ struct irq_domain_ops; #include <acpi/acpi_io.h> #include <asm/acpi.h> +#include <linux/fw_table.h> + +#ifdef CONFIG_ACPI_TABLE_LIB +#define EXPORT_SYMBOL_ACPI_LIB(x) EXPORT_SYMBOL_NS_GPL(x, ACPI) +#define __init_or_acpilib +#define __initdata_or_acpilib +#else +#define EXPORT_SYMBOL_ACPI_LIB(x) +#define __init_or_acpilib __init +#define __initdata_or_acpilib __initdata +#endif + static inline acpi_handle acpi_device_handle(struct acpi_device *adev) { return adev ? adev->handle : NULL; diff --git a/include/linux/fw_table.h b/include/linux/fw_table.h index ff8fa58d5818..a722300c215b 100644 --- a/include/linux/fw_table.h +++ b/include/linux/fw_table.h @@ -26,7 +26,6 @@ struct acpi_subtable_proc { }; #include <linux/acpi.h> -#include <acpi/acpi.h> union acpi_subtable_headers { struct acpi_subtable_header common; >> And directly including acpi/acpi.h w/o linux/acpi.h causes triggering the #error and some other stuff: >> >> ./include/acpi/platform/aclinux.h:18:2: error: #error "Please don't include <acpi/acpi.h> directly, include <linux/acpi.h> instead." >> 18 | #error "Please don't include <acpi/acpi.h> directly, include <linux/acpi.h> instead." >> | ^~~~~ >> >> >> Only including linux/acpi.h: >> In file included from ./include/linux/acpi.h:18, >> from init/main.c:30: >> ./include/linux/fw_table.h:32:37: error: field ‘common’ has incomplete type >> 32 | struct acpi_subtable_header common; >> | ^~~~~~ >> ./include/linux/fw_table.h:33:36: error: field ‘hmat’ has incomplete type >> 33 | struct acpi_hmat_structure hmat; >> | ^~~~ >> ./include/linux/fw_table.h:34:40: error: field ‘prmt’ has incomplete type >> 34 | struct acpi_prmt_module_header prmt; >> | ^~~~ >> ./include/linux/fw_table.h:35:33: error: field ‘cedt’ has incomplete type >> 35 | struct acpi_cedt_header cedt; >> | ^~~~ >> >> >>> >>> Yes, that's the problem AFAICS. Dave? >>> >>> What about moving the fw_table.h include in linux/acpi.h below the >>> mutex.h one, along with the EXPORT_SYMBOL_ACPI_LIB-related >>> definitions? >> >> This builds cleanly for me. > > OK, so I'm wondering if this also helps the other people in this thread.
On Mon, Nov 20, 2023 at 5:53 PM Dave Jiang <dave.jiang@intel.com> wrote: > However, if I move fw_table.h in linux/acpi.h below include of asm/acpi.h, then we > can build successfully w/o including acpi/acpi.h in fw_table.h. This looks reasonable to me, can you send a formal patch so I can test? Yours, Linus Walleij
On 11/21/23 06:32, Linus Walleij wrote: > On Mon, Nov 20, 2023 at 5:53 PM Dave Jiang <dave.jiang@intel.com> wrote: > >> However, if I move fw_table.h in linux/acpi.h below include of asm/acpi.h, then we >> can build successfully w/o including acpi/acpi.h in fw_table.h. > > This looks reasonable to me, can you send a formal patch so I can test? https://lore.kernel.org/linux-acpi/170058229266.2356592.11579977558324549374.stgit@djiang5-mobl3/ Thanks! > > Yours, > Linus Walleij
diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h index 337ffa931ee8..76444ffdc549 100644 --- a/include/acpi/platform/acenv.h +++ b/include/acpi/platform/acenv.h @@ -156,7 +156,7 @@ #endif -#if defined(_LINUX) || defined(__linux__) +#if defined(_LINUX) || defined(__linux__) || defined(__KERNEL__) #include <acpi/platform/aclinux.h> #elif defined(_APPLE) || defined(__APPLE__)
After commit a103f46633fd the kernel stopped compiling for several ARM32 platforms that I am building with a bare metal compiler. Bare metal compilers (arm-none-eabi-) don't define __linux__. This is because the header <acpi/platform/acenv.h> is now in the include path for <linux/irq.h>: CC arch/arm/kernel/irq.o CC kernel/sysctl.o CC crypto/api.o In file included from ../include/acpi/acpi.h:22, from ../include/linux/fw_table.h:29, from ../include/linux/acpi.h:18, from ../include/linux/irqchip.h:14, from ../arch/arm/kernel/irq.c:25: ../include/acpi/platform/acenv.h:218:2: error: #error Unknown target environment 218 | #error Unknown target environment | ^~~~~ One solution to make compilation with a bare metal compiler work again is to say the file is used with Linux from within the kernel if __KERNEL__ is defined so I did that. Fixes: a103f46633fd ("acpi: Move common tables helper functions to common lib") Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- include/acpi/platform/acenv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 9bacdd8996c77c42ca004440be610692275ff9d0 change-id: 20231113-arm-build-bug-ae543fd21199 Best regards,