diff mbox

[v2,4/6] ACPI: ARM: exclude DMI calls

Message ID 1385080915-23430-5-git-send-email-al.stone@linaro.org
State New
Headers show

Commit Message

Al Stone Nov. 22, 2013, 12:41 a.m. UTC
From: Al Stone <al.stone@linaro.org>

Modified #ifdef so that DMI is not used on ARM platforms which
are currently implementing ACPI reduced HW mode.

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 include/linux/dmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Rob Herring Nov. 22, 2013, 1:25 p.m. UTC | #1
On Thu, Nov 21, 2013 at 6:41 PM,  <al.stone@linaro.org> wrote:
> From: Al Stone <al.stone@linaro.org>
>
> Modified #ifdef so that DMI is not used on ARM platforms which
> are currently implementing ACPI reduced HW mode.

It is really not allowed or is optional? There are various people that
want DMI tables on ARM.

Rob

> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  include/linux/dmi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index f820f0a..a03deb8 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -83,7 +83,7 @@ struct dmi_device {
>         void *device_data;      /* Type specific data */
>  };
>
> -#ifdef CONFIG_DMI
> +#if IS_ENABLED(CONFIG_DMI) && !IS_ENABLED(CONFIG_ACPI_REDUCED_HARDWARE)
>
>  struct dmi_dev_onboard {
>         struct dmi_device dev;
> --
> 1.8.3.1
>
>
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel
Al Stone Nov. 22, 2013, 6:03 p.m. UTC | #2
On 11/22/2013 06:25 AM, Rob Herring wrote:
> On Thu, Nov 21, 2013 at 6:41 PM,  <al.stone@linaro.org> wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Modified #ifdef so that DMI is not used on ARM platforms which
>> are currently implementing ACPI reduced HW mode.
>
> It is really not allowed or is optional? There are various people that
> want DMI tables on ARM.
>
> Rob

True.  DMI is optional.  I see it as orthogonal to
reduced HW mode; I have to hope that when DMI patches
are forthcoming they'll do the right thing here.

Is there a better way to do this in the #if ?

>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   include/linux/dmi.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
>> index f820f0a..a03deb8 100644
>> --- a/include/linux/dmi.h
>> +++ b/include/linux/dmi.h
>> @@ -83,7 +83,7 @@ struct dmi_device {
>>          void *device_data;      /* Type specific data */
>>   };
>>
>> -#ifdef CONFIG_DMI
>> +#if IS_ENABLED(CONFIG_DMI) && !IS_ENABLED(CONFIG_ACPI_REDUCED_HARDWARE)
>>
>>   struct dmi_dev_onboard {
>>          struct dmi_device dev;
>> --
>> 1.8.3.1
>>
>>
>> _______________________________________________
>> linaro-kernel mailing list
>> linaro-kernel@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/linaro-kernel
Olof Johansson Nov. 22, 2013, 6:53 p.m. UTC | #3
On Fri, Nov 22, 2013 at 10:03 AM, Al Stone <al.stone@linaro.org> wrote:
> On 11/22/2013 06:25 AM, Rob Herring wrote:
>>
>> On Thu, Nov 21, 2013 at 6:41 PM,  <al.stone@linaro.org> wrote:
>>>
>>> From: Al Stone <al.stone@linaro.org>
>>>
>>> Modified #ifdef so that DMI is not used on ARM platforms which
>>> are currently implementing ACPI reduced HW mode.
>>
>>
>> It is really not allowed or is optional? There are various people that
>> want DMI tables on ARM.
>>
>> Rob
>
>
> True.  DMI is optional.  I see it as orthogonal to
> reduced HW mode; I have to hope that when DMI patches
> are forthcoming they'll do the right thing here.
>
> Is there a better way to do this in the #if ?


Doing all of these things at compile time seems odd, shouldn't it be
handled at runtime? What happens when someone wants to build a kernel
that boots both on the reduced hw mode platforms and regular ones?


-Olof
Rafael J. Wysocki Nov. 22, 2013, 11:15 p.m. UTC | #4
On Friday, November 22, 2013 10:53:09 AM Olof Johansson wrote:
> On Fri, Nov 22, 2013 at 10:03 AM, Al Stone <al.stone@linaro.org> wrote:
> > On 11/22/2013 06:25 AM, Rob Herring wrote:
> >>
> >> On Thu, Nov 21, 2013 at 6:41 PM,  <al.stone@linaro.org> wrote:
> >>>
> >>> From: Al Stone <al.stone@linaro.org>
> >>>
> >>> Modified #ifdef so that DMI is not used on ARM platforms which
> >>> are currently implementing ACPI reduced HW mode.
> >>
> >>
> >> It is really not allowed or is optional? There are various people that
> >> want DMI tables on ARM.
> >>
> >> Rob
> >
> >
> > True.  DMI is optional.  I see it as orthogonal to
> > reduced HW mode; I have to hope that when DMI patches
> > are forthcoming they'll do the right thing here.
> >
> > Is there a better way to do this in the #if ?
> 
> 
> Doing all of these things at compile time seems odd, shouldn't it be
> handled at runtime? What happens when someone wants to build a kernel
> that boots both on the reduced hw mode platforms and regular ones?

I agree.

My suggestion would be to harden dmi_check_system() so that it works
if DMI is not present (if it doesn't already).

Thanks!
Al Stone Nov. 23, 2013, 12:05 a.m. UTC | #5
On 11/22/2013 04:15 PM, Rafael J. Wysocki wrote:
> On Friday, November 22, 2013 10:53:09 AM Olof Johansson wrote:
>> On Fri, Nov 22, 2013 at 10:03 AM, Al Stone <al.stone@linaro.org> wrote:
>>> On 11/22/2013 06:25 AM, Rob Herring wrote:
>>>>
>>>> On Thu, Nov 21, 2013 at 6:41 PM,  <al.stone@linaro.org> wrote:
>>>>>
>>>>> From: Al Stone <al.stone@linaro.org>
>>>>>
>>>>> Modified #ifdef so that DMI is not used on ARM platforms which
>>>>> are currently implementing ACPI reduced HW mode.
>>>>
>>>>
>>>> It is really not allowed or is optional? There are various people that
>>>> want DMI tables on ARM.
>>>>
>>>> Rob
>>>
>>>
>>> True.  DMI is optional.  I see it as orthogonal to
>>> reduced HW mode; I have to hope that when DMI patches
>>> are forthcoming they'll do the right thing here.
>>>
>>> Is there a better way to do this in the #if ?
>>
>>
>> Doing all of these things at compile time seems odd, shouldn't it be
>> handled at runtime? What happens when someone wants to build a kernel
>> that boots both on the reduced hw mode platforms and regular ones?
>
> I agree.
>
> My suggestion would be to harden dmi_check_system() so that it works
> if DMI is not present (if it doesn't already).
>
> Thanks!
>

I agree -- runtime would be better.  For dmi_check_system(), that
can be done and I'll look into that.  I'll also go double check the
rest of my #ifdefs and see if I can remove any more of them from the
Linux side of the ACPI code.

For reduced hardware mode, however, I have to rely on the underlying
ACPICA reference implementation to behave properly.  Right now, ACPICA
relies on compile time changes to implement either reduced HW mode or
legacy mode so I have to follow suit.  When I looked at making ACPICA
change behavior at runtime, the changes became more and more invasive.
Since x86/ia64 depend on ACPICA to behave also, that seemed a far more 
dangerous approach to me.
Matthew Garrett Nov. 23, 2013, 4:38 p.m. UTC | #6
On Fri, Nov 22, 2013 at 05:05:48PM -0700, Al Stone wrote:

> For reduced hardware mode, however, I have to rely on the underlying
> ACPICA reference implementation to behave properly.  Right now, ACPICA
> relies on compile time changes to implement either reduced HW mode or
> legacy mode so I have to follow suit.  When I looked at making ACPICA
> change behavior at runtime, the changes became more and more invasive.
> Since x86/ia64 depend on ACPICA to behave also, that seemed a far
> more dangerous approach to me.

Ugh. Really? People have been fairly careful about making sure that the 
x86 SoC code is selected correctly at runtime, and losing that because 
ACPICA is broken would be a shame. I think this is something that needs 
to support runtime switching even if there's also support for building 
kernels that only implement the reduced hardware profile.
Zheng, Lv Nov. 25, 2013, 5:10 a.m. UTC | #7
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Matthew Garrett
> Sent: Sunday, November 24, 2013 12:39 AM
> 
> On Fri, Nov 22, 2013 at 05:05:48PM -0700, Al Stone wrote:
> 
> > For reduced hardware mode, however, I have to rely on the underlying
> > ACPICA reference implementation to behave properly.  Right now, ACPICA
> > relies on compile time changes to implement either reduced HW mode or
> > legacy mode so I have to follow suit.  When I looked at making ACPICA
> > change behavior at runtime, the changes became more and more invasive.
> > Since x86/ia64 depend on ACPICA to behave also, that seemed a far
> > more dangerous approach to me.
> 
> Ugh. Really? People have been fairly careful about making sure that the
> x86 SoC code is selected correctly at runtime, and losing that because
> ACPICA is broken would be a shame. I think this is something that needs
> to support runtime switching even if there's also support for building
> kernels that only implement the reduced hardware profile.

If my reading is correct, do you mean x86 SoCs should have already tested the code.

So if ARM need ACPI_REDUCED_HARDWARE to be defined, the <include/acpi/platform/aclinux.h> should have lines like:
#ifdef CONFIG_ARCH_IS_ACPI_REDUCED_HARDWARE
#define ACPI_REDUCED_HARDWARE TRUE
#endif
And ARCH_IS_ACPI_REDUCED_HARDWARE should only be selected by CONFIG_ARM.

Well, I don't have knowledge about "x86 ACPI_REDUCED_HARDWARE tests", so I don't have any idea on whether the above code or original one can reflect the real world.

Thanks and best regards
-Lv

> 
> --
> Matthew Garrett | mjg59@srcf.ucam.org
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Garrett Nov. 25, 2013, 3:30 p.m. UTC | #8
On Mon, Nov 25, 2013 at 05:10:46AM +0000, Zheng, Lv wrote:
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Matthew Garrett
> > Ugh. Really? People have been fairly careful about making sure that the
> > x86 SoC code is selected correctly at runtime, and losing that because
> > ACPICA is broken would be a shame. I think this is something that needs
> > to support runtime switching even if there's also support for building
> > kernels that only implement the reduced hardware profile.
> 
> If my reading is correct, do you mean x86 SoCs should have already tested the code.

I don't know if anyone has deployed x86 SoCs with reduced hardware yet, 
but it seems like something that might happen.

> So if ARM need ACPI_REDUCED_HARDWARE to be defined, the <include/acpi/platform/aclinux.h> should have lines like:
> #ifdef CONFIG_ARCH_IS_ACPI_REDUCED_HARDWARE
> #define ACPI_REDUCED_HARDWARE TRUE
> #endif
> And ARCH_IS_ACPI_REDUCED_HARDWARE should only be selected by CONFIG_ARM.

Is ACPI_REDUCED_HARDWARE supposed to indicate support for the reduced 
hardware profile, or that the platform *only* implements the reduced 
hardware profile?
Al Stone Nov. 25, 2013, 5:43 p.m. UTC | #9
On 11/25/2013 08:30 AM, Matthew Garrett wrote:
> On Mon, Nov 25, 2013 at 05:10:46AM +0000, Zheng, Lv wrote:
>>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Matthew Garrett
>>> Ugh. Really? People have been fairly careful about making sure that the
>>> x86 SoC code is selected correctly at runtime, and losing that because
>>> ACPICA is broken would be a shame. I think this is something that needs
>>> to support runtime switching even if there's also support for building
>>> kernels that only implement the reduced hardware profile.
>>
>> If my reading is correct, do you mean x86 SoCs should have already tested the code.
>
> I don't know if anyone has deployed x86 SoCs with reduced hardware yet,
> but it seems like something that might happen.
>
>> So if ARM need ACPI_REDUCED_HARDWARE to be defined, the <include/acpi/platform/aclinux.h> should have lines like:
>> #ifdef CONFIG_ARCH_IS_ACPI_REDUCED_HARDWARE
>> #define ACPI_REDUCED_HARDWARE TRUE
>> #endif
>> And ARCH_IS_ACPI_REDUCED_HARDWARE should only be selected by CONFIG_ARM.
>
> Is ACPI_REDUCED_HARDWARE supposed to indicate support for the reduced
> hardware profile, or that the platform *only* implements the reduced
> hardware profile?
>

 From what I can see in ACPICA, ACPI_REDUCED_HARDWARE indicates the
platform *only* implements the reduced hardware profile.  This *seems*
to be consistent with the specification -- see 3.11.1, second bullet,
for example:

    Boot in ACPI mode only (ACPI Enable, ACPI Disable, SMI_CMD and
    Legacy mode are not supported)

...if by "not supported" one takes that to mean "does not exist when
compiled."  I can look at the ACPICA code again, just the same; perhaps
there is some reasonable way to at least select one or the other at boot
as the first step, and then allow switching between modes as a later
step.

To Lv's point, since hardware reduced mode was added in ACPI 5.0, I
don't think there has been a lot of exposure to it, especially on
working platforms on the Linux side, so I doubt there has been any
significant Linux testing of it until now.
Matthew Garrett Nov. 25, 2013, 5:45 p.m. UTC | #10
On Mon, Nov 25, 2013 at 10:43:27AM -0700, Al Stone wrote:
> On 11/25/2013 08:30 AM, Matthew Garrett wrote:
> >Is ACPI_REDUCED_HARDWARE supposed to indicate support for the reduced
> >hardware profile, or that the platform *only* implements the reduced
> >hardware profile?
> 
> From what I can see in ACPICA, ACPI_REDUCED_HARDWARE indicates the
> platform *only* implements the reduced hardware profile.  This *seems*
> to be consistent with the specification -- see 3.11.1, second bullet,
> for example:

Ok, so a kernel built without ACPI_REDUCED_HARDWARE would still support 
the reduced hardware profile?

> ...if by "not supported" one takes that to mean "does not exist when
> compiled."  I can look at the ACPICA code again, just the same; perhaps
> there is some reasonable way to at least select one or the other at boot
> as the first step, and then allow switching between modes as a later
> step.

I don't think you'd ever want to switch after init time. There's a flag 
in the FADT that indicates whether a system is implementing the reduced 
hardware profile or not.
Al Stone Nov. 25, 2013, 6:01 p.m. UTC | #11
On 11/25/2013 10:45 AM, Matthew Garrett wrote:
> On Mon, Nov 25, 2013 at 10:43:27AM -0700, Al Stone wrote:
>> On 11/25/2013 08:30 AM, Matthew Garrett wrote:
>>> Is ACPI_REDUCED_HARDWARE supposed to indicate support for the reduced
>>> hardware profile, or that the platform *only* implements the reduced
>>> hardware profile?
>>
>>  From what I can see in ACPICA, ACPI_REDUCED_HARDWARE indicates the
>> platform *only* implements the reduced hardware profile.  This *seems*
>> to be consistent with the specification -- see 3.11.1, second bullet,
>> for example:
>
> Ok, so a kernel built without ACPI_REDUCED_HARDWARE would still support
> the reduced hardware profile?

Let me check on that.  The reduced hardware profile is a pretty
strict subset, and I think I can see a way where I could force
selecting the reduced HW profile on boot if the kernel has been
built *without* ACPI_REDUCED_HARDWARE.  What I am not convinced
of is that all of the proper guards are in place in ACPICA -- I
trust that they have been, but I would like to double check.

If I can get that to work properly, I'll add it to this patch set.

>> ...if by "not supported" one takes that to mean "does not exist when
>> compiled."  I can look at the ACPICA code again, just the same; perhaps
>> there is some reasonable way to at least select one or the other at boot
>> as the first step, and then allow switching between modes as a later
>> step.
>
> I don't think you'd ever want to switch after init time. There's a flag
> in the FADT that indicates whether a system is implementing the reduced
> hardware profile or not.
>

Agreed.  I could see it as something to use when experimenting perhaps,
but I think just allowing the switch at boot would cover the majority
of the use cases.
Al Stone Dec. 4, 2013, 1:30 a.m. UTC | #12
On 11/25/2013 11:01 AM, Al Stone wrote:
> On 11/25/2013 10:45 AM, Matthew Garrett wrote:
>> On Mon, Nov 25, 2013 at 10:43:27AM -0700, Al Stone wrote:
>>> On 11/25/2013 08:30 AM, Matthew Garrett wrote:
>>>> Is ACPI_REDUCED_HARDWARE supposed to indicate support for the reduced
>>>> hardware profile, or that the platform *only* implements the reduced
>>>> hardware profile?
>>>
>>>  From what I can see in ACPICA, ACPI_REDUCED_HARDWARE indicates the
>>> platform *only* implements the reduced hardware profile.  This *seems*
>>> to be consistent with the specification -- see 3.11.1, second bullet,
>>> for example:
>>
>> Ok, so a kernel built without ACPI_REDUCED_HARDWARE would still support
>> the reduced hardware profile?
>
> Let me check on that.  The reduced hardware profile is a pretty
> strict subset, and I think I can see a way where I could force
> selecting the reduced HW profile on boot if the kernel has been
> built *without* ACPI_REDUCED_HARDWARE.  What I am not convinced
> of is that all of the proper guards are in place in ACPICA -- I
> trust that they have been, but I would like to double check.
>
> If I can get that to work properly, I'll add it to this patch set.

I thought I could get this to work, but I am going to have to defer
to the ACPICA upstream folks.  For the time being, I think that all
of the architectures that want to use ACPI in either legacy mode or
in stripped down reduced HW mode will have to use different kernels,
one for each mode.  I thought there might be enough safety checks to
allow the kernel to boot in legacy mode and switch into reduced HW at
boot, but there are not, in my opinion.  I don't see a way to make the
switch *and* maintain conformance with the spec without significant
change to ACPICA itself.

For example, enforcing that various functions are not allowed while
in reduced HW mode could be done by a check of the reduced HW flag on
entry.  If it is set, return the value the function would have returned
had it been stubbed out for reduced HW mode.  This is simple enough but
to do so would require modifying at least 29 functions in ACPICA, by my
count, not something upstream is particularly keen on -- nor am I.  I'd
rather step back and work with ACPICA over the longer term and see if
there's some way to get this functionality implemented properly instead
of trying to bolt it on somehow.

>>> ...if by "not supported" one takes that to mean "does not exist when
>>> compiled."  I can look at the ACPICA code again, just the same; perhaps
>>> there is some reasonable way to at least select one or the other at boot
>>> as the first step, and then allow switching between modes as a later
>>> step.
>>
>> I don't think you'd ever want to switch after init time. There's a flag
>> in the FADT that indicates whether a system is implementing the reduced
>> hardware profile or not.
>>
>
> Agreed.  I could see it as something to use when experimenting perhaps,
> but I think just allowing the switch at boot would cover the majority
> of the use cases.
>
Matthew Garrett Dec. 4, 2013, 1:34 a.m. UTC | #13
On Tue, Dec 03, 2013 at 06:30:48PM -0700, Al Stone wrote:

> I thought I could get this to work, but I am going to have to defer
> to the ACPICA upstream folks.  For the time being, I think that all
> of the architectures that want to use ACPI in either legacy mode or
> in stripped down reduced HW mode will have to use different kernels,
> one for each mode.  I thought there might be enough safety checks to
> allow the kernel to boot in legacy mode and switch into reduced HW at
> boot, but there are not, in my opinion.  I don't see a way to make the
> switch *and* maintain conformance with the spec without significant
> change to ACPICA itself.

Ugh. That needs fixing. There's been a huge amount of work done to 
ensure that x86 only needs a single kernel image for 64-bit, it really 
needs to be runtime. We shouldn't have merged it in this state.

> For example, enforcing that various functions are not allowed while
> in reduced HW mode could be done by a check of the reduced HW flag on
> entry.  If it is set, return the value the function would have returned
> had it been stubbed out for reduced HW mode.  This is simple enough but
> to do so would require modifying at least 29 functions in ACPICA, by my
> count, not something upstream is particularly keen on -- nor am I.  I'd
> rather step back and work with ACPICA over the longer term and see if
> there's some way to get this functionality implemented properly instead
> of trying to bolt it on somehow.

How many of those are calls that we'll actually execute in the HW 
reduced case?
Grant Likely Dec. 10, 2013, 12:45 p.m. UTC | #14
On Sat, 23 Nov 2013 16:38:54 +0000, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Fri, Nov 22, 2013 at 05:05:48PM -0700, Al Stone wrote:
> 
> > For reduced hardware mode, however, I have to rely on the underlying
> > ACPICA reference implementation to behave properly.  Right now, ACPICA
> > relies on compile time changes to implement either reduced HW mode or
> > legacy mode so I have to follow suit.  When I looked at making ACPICA
> > change behavior at runtime, the changes became more and more invasive.
> > Since x86/ia64 depend on ACPICA to behave also, that seemed a far
> > more dangerous approach to me.
> 
> Ugh. Really? People have been fairly careful about making sure that the 
> x86 SoC code is selected correctly at runtime, and losing that because 
> ACPICA is broken would be a shame. I think this is something that needs 
> to support runtime switching even if there's also support for building 
> kernels that only implement the reduced hardware profile.

Yeah, that is a really big problem. At the very least push the hacks
back into ACPICA and make that project sort it out (add stub functions
if needed). I don't like seeing the kernel having #ifdef blocks to stub
out normal ACPI paths.

g.
diff mbox

Patch

diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index f820f0a..a03deb8 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -83,7 +83,7 @@  struct dmi_device {
 	void *device_data;	/* Type specific data */
 };
 
-#ifdef CONFIG_DMI
+#if IS_ENABLED(CONFIG_DMI) && !IS_ENABLED(CONFIG_ACPI_REDUCED_HARDWARE)
 
 struct dmi_dev_onboard {
 	struct dmi_device dev;