ACPI: acenv: Permit compilation from within the kernel

  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,
  

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