diff mbox

[1/7] ACPICA: Only include ACPI asm files if ACPI is enabled

Message ID 1401883796-17841-2-git-send-email-lee.jones@linaro.org
State Superseded
Headers show

Commit Message

Lee Jones June 4, 2014, 12:09 p.m. UTC
Any drivers which support ACPI and Device Tree probing need to include
both respective header files.  Without this patch, if a driver is being
used on a platform which does not support ACPI and subsequently does not
have the config option enabled, but includes linux/acpi.h the build
breaks with:

  In file included from ../include/acpi/platform/acenv.h:150:0,
                   from ../include/acpi/acpi.h:56,
                   from ../include/linux/match.h:2,
                   from ../drivers/i2c/i2c-core.c:43:
  ../include/acpi/platform/aclinux.h:73:23:
   fatal error: asm/acenv.h: No such file or directory
   #include <asm/acenv.h>
                       ^
Cc: Lv Zheng <lv.zheng@intel.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: linux-acpi@vger.kernel.org
Cc: devel@acpica.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 include/acpi/platform/aclinux.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Rafael J. Wysocki June 4, 2014, 12:35 p.m. UTC | #1
On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> Any drivers which support ACPI and Device Tree probing need to include
> both respective header files.  Without this patch, if a driver is being
> used on a platform which does not support ACPI and subsequently does not
> have the config option enabled, but includes linux/acpi.h the build
> breaks with:
> 
>   In file included from ../include/acpi/platform/acenv.h:150:0,
>                    from ../include/acpi/acpi.h:56,
>                    from ../include/linux/match.h:2,
>                    from ../drivers/i2c/i2c-core.c:43:
>   ../include/acpi/platform/aclinux.h:73:23:
>    fatal error: asm/acenv.h: No such file or directory
>    #include <asm/acenv.h>
>                        ^

Which kernel does this happen with?

> Cc: Lv Zheng <lv.zheng@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: linux-acpi@vger.kernel.org
> Cc: devel@acpica.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  include/acpi/platform/aclinux.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index cd1f052..fdf7663 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -70,9 +70,10 @@
>  #ifdef EXPORT_ACPI_INTERFACES
>  #include <linux/export.h>
>  #endif
> -#include <asm/acenv.h>
>  
> -#ifndef CONFIG_ACPI
> +#ifdef CONFIG_ACPI
> +#include <asm/acenv.h>
> +#else
>  
>  /* External globals for __KERNEL__, stubs is needed */
>  
>
Lee Jones June 4, 2014, 12:51 p.m. UTC | #2
On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:

> On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> > Any drivers which support ACPI and Device Tree probing need to include
> > both respective header files.  Without this patch, if a driver is being
> > used on a platform which does not support ACPI and subsequently does not
> > have the config option enabled, but includes linux/acpi.h the build
> > breaks with:
> > 
> >   In file included from ../include/acpi/platform/acenv.h:150:0,
> >                    from ../include/acpi/acpi.h:56,
> >                    from ../include/linux/match.h:2,
> >                    from ../drivers/i2c/i2c-core.c:43:
> >   ../include/acpi/platform/aclinux.h:73:23:
> >    fatal error: asm/acenv.h: No such file or directory
> >    #include <asm/acenv.h>
> >                        ^
> 
> Which kernel does this happen with?

a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)
  Add linux-next specific files for 20140602

> > Cc: Lv Zheng <lv.zheng@intel.com>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: linux-acpi@vger.kernel.org
> > Cc: devel@acpica.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  include/acpi/platform/aclinux.h | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> > index cd1f052..fdf7663 100644
> > --- a/include/acpi/platform/aclinux.h
> > +++ b/include/acpi/platform/aclinux.h
> > @@ -70,9 +70,10 @@
> >  #ifdef EXPORT_ACPI_INTERFACES
> >  #include <linux/export.h>
> >  #endif
> > -#include <asm/acenv.h>
> >  
> > -#ifndef CONFIG_ACPI
> > +#ifdef CONFIG_ACPI
> > +#include <asm/acenv.h>
> > +#else
> >  
> >  /* External globals for __KERNEL__, stubs is needed */
> >  
> > 
>
Rafael J. Wysocki June 4, 2014, 9:29 p.m. UTC | #3
On Wednesday, June 04, 2014 01:51:37 PM Lee Jones wrote:
> On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
> 
> > On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
> > > Any drivers which support ACPI and Device Tree probing need to include
> > > both respective header files.  Without this patch, if a driver is being
> > > used on a platform which does not support ACPI and subsequently does not
> > > have the config option enabled, but includes linux/acpi.h the build
> > > breaks with:
> > > 
> > >   In file included from ../include/acpi/platform/acenv.h:150:0,
> > >                    from ../include/acpi/acpi.h:56,
> > >                    from ../include/linux/match.h:2,
> > >                    from ../drivers/i2c/i2c-core.c:43:
> > >   ../include/acpi/platform/aclinux.h:73:23:
> > >    fatal error: asm/acenv.h: No such file or directory
> > >    #include <asm/acenv.h>
> > >                        ^
> > 
> > Which kernel does this happen with?
> 
> a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)
>   Add linux-next specific files for 20140602

It looks like the problem is with include/linux/match.h that should not
include acpi/acpi.h directly.

But I can't find this file in the Linus' next branch even, so I guess it's
on its way to that branch?

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng June 5, 2014, 12:56 a.m. UTC | #4
Hi, Lee

> From: Lee Jones [mailto:lee.jones@linaro.org]
> Sent: Wednesday, June 04, 2014 8:10 PM
> 
> Any drivers which support ACPI and Device Tree probing need to include
> both respective header files.  Without this patch, if a driver is being
> used on a platform which does not support ACPI and subsequently does not
> have the config option enabled, but includes linux/acpi.h the build
> breaks with:
> 
>   In file included from ../include/acpi/platform/acenv.h:150:0,
>                    from ../include/acpi/acpi.h:56,
>                    from ../include/linux/match.h:2,
>                    from ../drivers/i2c/i2c-core.c:43:
>   ../include/acpi/platform/aclinux.h:73:23:
>    fatal error: asm/acenv.h: No such file or directory
>    #include <asm/acenv.h>
>                        ^
> Cc: Lv Zheng <lv.zheng@intel.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: linux-acpi@vger.kernel.org
> Cc: devel@acpica.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  include/acpi/platform/aclinux.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index cd1f052..fdf7663 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -70,9 +70,10 @@
>  #ifdef EXPORT_ACPI_INTERFACES
>  #include <linux/export.h>
>  #endif
> -#include <asm/acenv.h>
> 
> -#ifndef CONFIG_ACPI
> +#ifdef CONFIG_ACPI
> +#include <asm/acenv.h>
> +#else

This is exactly what I want to do in the next step.
But you are a bit faster here.
I believe:
The miss-ordered inclusions of <asm/acpi.h> is the culprit of all of the miss-ordered inclusions in arch/x86/include/asm.
You should have noted that <asm/acpi.h> was originally unexpected included by some x86 specific headers.
Simply doing <asm/acenv.h> exlusion in this way might be able to fix your issue for your architecture, but it could be very likely breaking x86 builds.
You might be able to find another way to solve your build issue - for example, creating an empty <asm/acenv.h> for arch/arm.

Thanks and best regards
-Lv

> 
>  /* External globals for __KERNEL__, stubs is needed */
> 
> --
> 1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng June 5, 2014, 1:01 a.m. UTC | #5
Hi,

> From: linux-i2c-owner@vger.kernel.org [mailto:linux-i2c-owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki

> Sent: Thursday, June 05, 2014 5:30 AM

> 

> On Wednesday, June 04, 2014 01:51:37 PM Lee Jones wrote:

> > On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:

> >

> > > On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:

> > > > Any drivers which support ACPI and Device Tree probing need to include

> > > > both respective header files.  Without this patch, if a driver is being

> > > > used on a platform which does not support ACPI and subsequently does not

> > > > have the config option enabled, but includes linux/acpi.h the build

> > > > breaks with:

> > > >

> > > >   In file included from ../include/acpi/platform/acenv.h:150:0,

> > > >                    from ../include/acpi/acpi.h:56,

> > > >                    from ../include/linux/match.h:2,

> > > >                    from ../drivers/i2c/i2c-core.c:43:

> > > >   ../include/acpi/platform/aclinux.h:73:23:

> > > >    fatal error: asm/acenv.h: No such file or directory

> > > >    #include <asm/acenv.h>

> > > >                        ^

> > >

> > > Which kernel does this happen with?

> >

> > a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)

> >   Add linux-next specific files for 20140602

> 

> It looks like the problem is with include/linux/match.h that should not

> include acpi/acpi.h directly.


This is another example that many mis-ordered inclusions are caused by the mis-ordered <asm/acpi.h> inclusion.

> 

> But I can't find this file in the Linus' next branch even, so I guess it's

> on its way to that branch?

> 


I guess,
In their tree, they have CONFIG_ACPI enabled for ARM, but we've changed to make:
1. <asm/acenv.h> the architecture specific layer for ACPICA, and
2. <asm/acpi.h> is now the architecture specific layer for Linux ACPI.
So they need to follow this.

Thanks and best regards
-Lv

> Rafael

> 

> --

> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng June 5, 2014, 1:14 a.m. UTC | #6
Hi, Lee

> From: Lee Jones [mailto:lee.jones@linaro.org]

> Sent: Wednesday, June 04, 2014 8:52 PM

> To: Rafael J. Wysocki

> 

> On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:

> 

> > On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:

> > > Any drivers which support ACPI and Device Tree probing need to include

> > > both respective header files.  Without this patch, if a driver is being

> > > used on a platform which does not support ACPI and subsequently does not

> > > have the config option enabled, but includes linux/acpi.h the build

> > > breaks with:

> > >

> > >   In file included from ../include/acpi/platform/acenv.h:150:0,

> > >                    from ../include/acpi/acpi.h:56,

> > >                    from ../include/linux/match.h:2,

> > >                    from ../drivers/i2c/i2c-core.c:43:

> > >   ../include/acpi/platform/aclinux.h:73:23:

> > >    fatal error: asm/acenv.h: No such file or directory

> > >    #include <asm/acenv.h>

> > >                        ^


Note that:
In our tree:
<asm/acenv.h> is only included by <acpi/acpi.h>.
And <acpi/acpi.h> is only included by
1. <linux/acpi.h> when CONFIG_ACPI enabled
2. <linux/sfi_acpi.h> - this is x86 specific, we'll clean it up by implementing stubs for all ACPI external interfaces.
So there is no case we need to exclude <asm/acenv.h> when CONFIG_ACPI is not enabled.

I cannot find linux/match.h here.
If <linux/match.h> want to include ACPI features, it shouldn't include <acpi/acpi.h>, but should include <linux/acpi.h>.
Please refer to:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8b48463f
And stop including <acpi/acpi.h> directly in any cases.

Thanks and best regards
-Lv


> >

> > Which kernel does this happen with?

> 

> a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)

>   Add linux-next specific files for 20140602

> 

> > > Cc: Lv Zheng <lv.zheng@intel.com>

> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> > > Cc: linux-acpi@vger.kernel.org

> > > Cc: devel@acpica.org

> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > > ---

> > >  include/acpi/platform/aclinux.h | 5 +++--

> > >  1 file changed, 3 insertions(+), 2 deletions(-)

> > >

> > > diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h

> > > index cd1f052..fdf7663 100644

> > > --- a/include/acpi/platform/aclinux.h

> > > +++ b/include/acpi/platform/aclinux.h

> > > @@ -70,9 +70,10 @@

> > >  #ifdef EXPORT_ACPI_INTERFACES

> > >  #include <linux/export.h>

> > >  #endif

> > > -#include <asm/acenv.h>

> > >

> > > -#ifndef CONFIG_ACPI

> > > +#ifdef CONFIG_ACPI

> > > +#include <asm/acenv.h>

> > > +#else

> > >

> > >  /* External globals for __KERNEL__, stubs is needed */

> > >

> > >

> >

> 

> --

> Lee Jones

> Linaro STMicroelectronics Landing Team Lead

> Linaro.org │ Open source software for ARM SoCs

> Follow Linaro: Facebook | Twitter | Blog
Hanjun Guo June 5, 2014, 4:06 a.m. UTC | #7
Hi Lv,

On 2014-6-5 8:56, Zheng, Lv wrote:
> Hi, Lee
> 
>> From: Lee Jones [mailto:lee.jones@linaro.org]
>> Sent: Wednesday, June 04, 2014 8:10 PM
>>
>> Any drivers which support ACPI and Device Tree probing need to include
>> both respective header files.  Without this patch, if a driver is being
>> used on a platform which does not support ACPI and subsequently does not
>> have the config option enabled, but includes linux/acpi.h the build
>> breaks with:
>>
>>   In file included from ../include/acpi/platform/acenv.h:150:0,
>>                    from ../include/acpi/acpi.h:56,
>>                    from ../include/linux/match.h:2,
>>                    from ../drivers/i2c/i2c-core.c:43:
>>   ../include/acpi/platform/aclinux.h:73:23:
>>    fatal error: asm/acenv.h: No such file or directory
>>    #include <asm/acenv.h>
>>                        ^
>> Cc: Lv Zheng <lv.zheng@intel.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: linux-acpi@vger.kernel.org
>> Cc: devel@acpica.org
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>>  include/acpi/platform/aclinux.h | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
>> index cd1f052..fdf7663 100644
>> --- a/include/acpi/platform/aclinux.h
>> +++ b/include/acpi/platform/aclinux.h
>> @@ -70,9 +70,10 @@
>>  #ifdef EXPORT_ACPI_INTERFACES
>>  #include <linux/export.h>
>>  #endif
>> -#include <asm/acenv.h>
>>
>> -#ifndef CONFIG_ACPI
>> +#ifdef CONFIG_ACPI
>> +#include <asm/acenv.h>
>> +#else
> 
> This is exactly what I want to do in the next step.
> But you are a bit faster here.
> I believe:
> The miss-ordered inclusions of <asm/acpi.h> is the culprit of all of the miss-ordered inclusions in arch/x86/include/asm.
> You should have noted that <asm/acpi.h> was originally unexpected included by some x86 specific headers.
> Simply doing <asm/acenv.h> exlusion in this way might be able to fix your issue for your architecture, but it could be very likely breaking x86 builds.
> You might be able to find another way to solve your build issue - for example, creating an empty <asm/acenv.h> for arch/arm.

Yes, we solve this issue as you suggested for arch/arm64.
since ARM32 will not support ACPI in the near future,
we may find another way to fix it.

Thanks
Hanjun

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo June 5, 2014, 4:11 a.m. UTC | #8
On 2014-6-5 9:14, Zheng, Lv wrote:
> Hi, Lee
> 
>> From: Lee Jones [mailto:lee.jones@linaro.org]
>> Sent: Wednesday, June 04, 2014 8:52 PM
>> To: Rafael J. Wysocki
>>
>> On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
>>
>>> On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
>>>> Any drivers which support ACPI and Device Tree probing need to include
>>>> both respective header files.  Without this patch, if a driver is being
>>>> used on a platform which does not support ACPI and subsequently does not
>>>> have the config option enabled, but includes linux/acpi.h the build
>>>> breaks with:
>>>>
>>>>   In file included from ../include/acpi/platform/acenv.h:150:0,
>>>>                    from ../include/acpi/acpi.h:56,
>>>>                    from ../include/linux/match.h:2,
>>>>                    from ../drivers/i2c/i2c-core.c:43:
>>>>   ../include/acpi/platform/aclinux.h:73:23:
>>>>    fatal error: asm/acenv.h: No such file or directory
>>>>    #include <asm/acenv.h>
>>>>                        ^
> 
> Note that:
> In our tree:
> <asm/acenv.h> is only included by <acpi/acpi.h>.
> And <acpi/acpi.h> is only included by
> 1. <linux/acpi.h> when CONFIG_ACPI enabled
> 2. <linux/sfi_acpi.h> - this is x86 specific, we'll clean it up by implementing stubs for all ACPI external interfaces.
> So there is no case we need to exclude <asm/acenv.h> when CONFIG_ACPI is not enabled.
> 
> I cannot find linux/match.h here.
> If <linux/match.h> want to include ACPI features, it shouldn't include <acpi/acpi.h>, but should include <linux/acpi.h>.
> Please refer to:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8b48463f
> And stop including <acpi/acpi.h> directly in any cases.

Ah, I agree, please ignore my previous email,
sorry for the noise.

Since it is very important to include <linux/acpi.h> but not <acpi/acpi.h>,
can we document it somewhere as the guidance? Then people will not
make such mistake :)

Thanks
Hanjun

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng June 5, 2014, 4:46 a.m. UTC | #9
Hi, 

> From: Hanjun Guo [mailto:hanjun.guo@linaro.org]

> Sent: Thursday, June 05, 2014 12:11 PM

> To: Zheng, Lv; Lee Jones; Rafael J. Wysocki

> 

> On 2014-6-5 9:14, Zheng, Lv wrote:

> > Hi, Lee

> >

> >> From: Lee Jones [mailto:lee.jones@linaro.org]

> >> Sent: Wednesday, June 04, 2014 8:52 PM

> >> To: Rafael J. Wysocki

> >>

> >> On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:

> >>

> >>> On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:

> >>>> Any drivers which support ACPI and Device Tree probing need to include

> >>>> both respective header files.  Without this patch, if a driver is being

> >>>> used on a platform which does not support ACPI and subsequently does not

> >>>> have the config option enabled, but includes linux/acpi.h the build

> >>>> breaks with:

> >>>>

> >>>>   In file included from ../include/acpi/platform/acenv.h:150:0,

> >>>>                    from ../include/acpi/acpi.h:56,

> >>>>                    from ../include/linux/match.h:2,

> >>>>                    from ../drivers/i2c/i2c-core.c:43:

> >>>>   ../include/acpi/platform/aclinux.h:73:23:

> >>>>    fatal error: asm/acenv.h: No such file or directory

> >>>>    #include <asm/acenv.h>

> >>>>                        ^

> >

> > Note that:

> > In our tree:

> > <asm/acenv.h> is only included by <acpi/acpi.h>.

> > And <acpi/acpi.h> is only included by

> > 1. <linux/acpi.h> when CONFIG_ACPI enabled

> > 2. <linux/sfi_acpi.h> - this is x86 specific, we'll clean it up by implementing stubs for all ACPI external interfaces.

> > So there is no case we need to exclude <asm/acenv.h> when CONFIG_ACPI is not enabled.

> >

> > I cannot find linux/match.h here.

> > If <linux/match.h> want to include ACPI features, it shouldn't include <acpi/acpi.h>, but should include <linux/acpi.h>.

> > Please refer to:

> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8b48463f

> > And stop including <acpi/acpi.h> directly in any cases.

> 

> Ah, I agree, please ignore my previous email,

> sorry for the noise.

> 

> Since it is very important to include <linux/acpi.h> but not <acpi/acpi.h>,

> can we document it somewhere as the guidance? Then people will not

> make such mistake :)


After I test the ACPICA stubs and remove the only abnormal direct <acpi/acpi.h> inclusion from <linux/sfi_acpi.h>.
We could have a commit to add something like:

#ifndef _LINUX_ACPI_H
#error ....
#endif
To the <acpi/platform/aclinux.h> (the Linux OSL for ACPICA headers, it is included by <acpi/acpi.h>).
Hope this can be the hint to prevent future mistakes.

Thanks and best regards
-Lv


> 

> Thanks

> Hanjun
diff mbox

Patch

diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index cd1f052..fdf7663 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -70,9 +70,10 @@ 
 #ifdef EXPORT_ACPI_INTERFACES
 #include <linux/export.h>
 #endif
-#include <asm/acenv.h>
 
-#ifndef CONFIG_ACPI
+#ifdef CONFIG_ACPI
+#include <asm/acenv.h>
+#else
 
 /* External globals for __KERNEL__, stubs is needed */