RE: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reducedhardware sleep path
From: Moore, Robert
Date: Wed Jul 24 2013 - 10:38:51 EST
I haven't found a high-level description of "acpi_os_prepare_sleep", perhaps I missed it.
Can someone point me to the overall description of this change and why it is being considered?
Thanks,
Bob
> -----Original Message-----
> From: Ben Guthro [mailto:Benjamin.Guthro@xxxxxxxxxx]
> Sent: Wednesday, July 24, 2013 6:23 AM
> To: Moore, Robert
> Cc: Zheng, Lv; Konrad Rzeszutek Wilk; Jan Beulich; Rafael J . Wysocki;
> linux-kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; xen-
> devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in
> reduced hardware sleep path
>
> On 07/24/2013 09:18 AM, Moore, Robert wrote:
> > I have not looked closely at this, but we typically do things like this
> in ACPICA so that they only need to be implemented once to support all of
> the various acpica-hosted operating systems - linux, solaris, hp-ux,
> apple, freebsd, etc. -- even if they could be implemented "cleaner" in
> some way on any given host.
>
> Even when the resulting "simplification" results in reduced functionality?
>
> Maybe I am misunderstanding the suggestion...but it sounded like it was
> basically to mimic the traditional behavior, and mask out the reduced
> hardware capabilities on these system types.
>
> It seems to me that if the system supports the reduced hardware ACPI
> sleep, you would want to make use of it...
>
>
>
> >
> >
> >
> >> -----Original Message-----
> >> From: Ben Guthro [mailto:Benjamin.Guthro@xxxxxxxxxx]
> >> Sent: Wednesday, July 24, 2013 5:01 AM
> >> To: Zheng, Lv
> >> Cc: Konrad Rzeszutek Wilk; Jan Beulich; Rafael J . Wysocki; linux-
> >> kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; xen-
> >> devel@xxxxxxxxxxxxx; Moore, Robert
> >> Subject: Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in
> >> reduced hardware sleep path
> >>
> >>
> >>
> >> On 07/24/2013 02:24 AM, Zheng, Lv wrote:
> >>> Hi,
> >>>
> >>> Sorry for the delayed response.
> >>>
> >>>> From: Ben Guthro [mailto:Benjamin.Guthro@xxxxxxxxxx]
> >>>> Sent: Tuesday, July 02, 2013 7:43 PM
> >>>>
> >>>>
> >>>> On 07/02/2013 02:19 AM, Zheng, Lv wrote:
> >>>>> Thanks for your efforts!
> >>>>>
> >>>>> I wonder if it is possible to remove the argument - "u8 extended"
> >>>>> and convert
> >>>> "pm1a_control, pm1b_control" into some u8 values that are
> >>>> equivalent to "acpi_gbl_sleep_type_a, acpi_gbl_sleep_type_b" in the
> >>>> legacy sleep
> >> path.
> >>>>> It can also simplify Xen codes.
> >>>>
> >>>> Thanks for your time to review this.
> >>>>
> >>>> I'm not sure that this simplifies things. I think that, in fact, it
> >>>> would make them quite a bit more complicated, but perhaps I
> >> misunderstand.
> >>>>
> >>>> Is it not preferred to use the reduced hardware sleep, over the old
> >> method?
> >>>> While these register definitions may be equivalent below, doing the
> >>>> translation in linux, only to translate them back again at a lower
> >> layer seems unnecessary.
> >>>>
> >>>
> >>> Yes, it would require tboot layer to be able to be aware of how such
> >> fields locate in the PM registers.
> >>> So I think you can pass the register address of the field and the
> >>> field
> >> name/value pair to the tboot, this could simplify things, no lower
> >> layer effort will be needed.
> >>> Please don't worry about the case that a register field could be
> >>> split
> >> into PM1a and PM1b, it could be a hardware design issue.
> >>> IMO, one field should always be in one register, either PM1a or PM1b.
> >>> Or there could be hardware issues cannot be addressed by the ACPICA
> >> architecture (something like natural atomicity).
> >>> But maybe I'm wrong.
> >>
> >> Again, I don't think this simplifies things, but complicates them
> >> unnecessarily. Converting the reduced hardware sleep to the legacy
> >> sleep seems like it would be an unnecessary layer of translation.
> >>
> >> The interface now simply passes the information from ACPICA down to
> >> the lower layers (xen, tboot) - and then lets them worry about the
> >> reduced hardware implementation.
> >>
> >> FWIW, xen has shipped with this implemetation, and enterprise kernels
> >> using the traditional xen kernel (like Suse) are making use of it.
> >>
> >> It may benefit tboot, in this case, but not Xen.
> >>
> >> I personally see it as an undesirable complication.
> >>
> >> Best regards,
> >> Ben
> >>
> >>>
> >>> Thanks and best regards
> >>> -Lv
> >>>
> >>>> The hypervisor knows how to deal with both the reduced hardware
> >>>> sleep as well as the legacy sleep path - it merely need to
> >>>> distinguish these two paths, when performing the hypercall.
> >>>>
> >>>> Since there are two paths through the higher level ACPICA code -
> >>>> that in hwsleep.c, and hwesleep.c - there needs to be some
> >>>> distinction between the two paths, when calling through to the
> >>>> lower level
> >>>> acpi_os_prepare_sleep() call.
> >>>>
> >>>> An alternate method would be to create another interface named
> >>>> acpi_os_prepare_esleep() which would do the equivalent of this
> >>>> patch series, with an "extended" parameter hidden from upper level
> >> interfaces.
> >>>>
> >>>> This, however, would also add another function to
> >>>> include/acpi/acpiosxf.h - which, I thought was undesirable, in the
> >>>> impression that I got from Bob Moore, and Rafael Wysocki (though,
> >>>> please correct me on this point, if I have
> >>>> misunderstood)
> >>>>
> >>>> Best Regards
> >>>>
> >>>> Ben
> >>>>
> >>>>>
> >>>>> As in ACPI specification, the bit definitions between the legacy
> >>>>> sleep registers
> >>>> and the extended sleep registers are equivalent.
> >>>>>
> >>>>> The legacy sleep register definition:
> >>>>> Table 4-16 PM1 Status Registers Fixed Hardware Feature Status Bits
> >>>>> - WAK_STS(bit 15) Table 4-18 PM1 Control Registers Fixed Hardware
> >>>>> Feature Control Bits - SLP_TYPx (bit 10-12), SLP_EN (bit 13)
> >>>>>
> >>>>> The extended sleep register definition:
> >>>>> Table 4-24 Sleep Control Register - SLP_TYPx (3 bits from offset
> >>>>> 2), SLP_EN (1
> >>>> bit from offset 5), here 10-8 = 2, and 13-8 = 5, this definition is
> >>>> equivalent to Table 4-18.
> >>>>> Table 4-25 Sleep Status Register - WAK_STS (1 bit 7), 15-8 = 7,
> >>>>> this definition is
> >>>> equivalent to Table 4-16.
> >>>>>
> >>>>> Thanks and best regards
> >>>>> -Lv
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: linux-acpi-owner@xxxxxxxxxxxxxxx
> >>>>>> [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] On Behalf Of Ben Guthro
> >>>>>> Sent: Wednesday, June 26, 2013 10:06 PM
> >>>>>> To: Konrad Rzeszutek Wilk; Jan Beulich; Rafaell J . Wysocki;
> >>>>>> linux-kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx;
> >>>>>> xen-devel@xxxxxxxxxxxxx
> >>>>>> Cc: Ben Guthro; Moore, Robert
> >>>>>> Subject: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in
> >>>>>> reduced hardware sleep path
> >>>>>>
> >>>>>> In version 3.4 acpi_os_prepare_sleep() got introduced in parallel
> >>>>>> with reduced hardware sleep support, and the two changes didn't
> >>>>>> get
> >>>>>> synchronized: The new code doesn't call the hook function (if so
> >>>>>> requested). Fix this, requiring a parameter to be added to the
> >>>>>> hook function to distinguish "extended" from "legacy" sleep.
> >>>>>>
> >>>>>> Signed-off-by: Ben Guthro <benjamin.guthro@xxxxxxxxxx>
> >>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>>>> Cc: Bob Moore <robert.moore@xxxxxxxxx>
> >>>>>> Cc: Rafaell J. Wysocki <rjw@xxxxxxx>
> >>>>>> Cc: linux-acpi@xxxxxxxxxxxxxxx
> >>>>>> ---
> >>>>>> drivers/acpi/acpica/hwesleep.c | 8 ++++++++
> >>>>>> drivers/acpi/acpica/hwsleep.c | 2 +-
> >>>>>> drivers/acpi/osl.c | 16 ++++++++--------
> >>>>>> include/linux/acpi.h | 10 +++++-----
> >>>>>> 4 files changed, 22 insertions(+), 14 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/acpi/acpica/hwesleep.c
> >>>>>> b/drivers/acpi/acpica/hwesleep.c index 5e5f762..6834dd7 100644
> >>>>>> --- a/drivers/acpi/acpica/hwesleep.c
> >>>>>> +++ b/drivers/acpi/acpica/hwesleep.c
> >>>>>> @@ -43,6 +43,7 @@
> >>>>>> */
> >>>>>>
> >>>>>> #include <acpi/acpi.h>
> >>>>>> +#include <linux/acpi.h>
> >>>>>> #include "accommon.h"
> >>>>>>
> >>>>>> #define _COMPONENT ACPI_HARDWARE
> >>>>>> @@ -128,6 +129,13 @@ acpi_status acpi_hw_extended_sleep(u8
> >>>>>> sleep_state)
> >>>>>>
> >>>>>> ACPI_FLUSH_CPU_CACHE();
> >>>>>>
> >>>>>> + status = acpi_os_prepare_sleep(sleep_state,
> >> acpi_gbl_sleep_type_a,
> >>>>>> + acpi_gbl_sleep_type_b, true);
> >>>>>> + if (ACPI_SKIP(status))
> >>>>>> + return_ACPI_STATUS(AE_OK);
> >>>>>> + if (ACPI_FAILURE(status))
> >>>>>> + return_ACPI_STATUS(status);
> >>>>>> +
> >>>>>> /*
> >>>>>> * Set the SLP_TYP and SLP_EN bits.
> >>>>>> *
> >>>>>> diff --git a/drivers/acpi/acpica/hwsleep.c
> >>>>>> b/drivers/acpi/acpica/hwsleep.c index e3828cc..a93c299 100644
> >>>>>> --- a/drivers/acpi/acpica/hwsleep.c
> >>>>>> +++ b/drivers/acpi/acpica/hwsleep.c
> >>>>>> @@ -153,7 +153,7 @@ acpi_status acpi_hw_legacy_sleep(u8
> sleep_state)
> >>>>>> ACPI_FLUSH_CPU_CACHE();
> >>>>>>
> >>>>>> status = acpi_os_prepare_sleep(sleep_state, pm1a_control,
> >>>>>> - pm1b_control);
> >>>>>> + pm1b_control, false);
> >>>>>> if (ACPI_SKIP(status))
> >>>>>> return_ACPI_STATUS(AE_OK);
> >>>>>> if (ACPI_FAILURE(status))
> >>>>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> >>>>>> e721863..3fc2801 100644
> >>>>>> --- a/drivers/acpi/osl.c
> >>>>>> +++ b/drivers/acpi/osl.c
> >>>>>> @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger);
> >>>>>> extern char line_buf[80];
> >>>>>> #endif /*ENABLE_DEBUGGER */
> >>>>>>
> >>>>>> -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32
> pm1a_ctrl,
> >>>>>> - u32 pm1b_ctrl);
> >>>>>> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a,
> >>>>>> +u32
> >>>> val_b,
> >>>>>> + u8 extended);
> >>>>>>
> >>>>>> static acpi_osd_handler acpi_irq_handler;
> >>>>>> static void *acpi_irq_context;
> >>>>>> @@ -1757,13 +1757,13 @@ acpi_status acpi_os_terminate(void)
> >>>>>> return AE_OK;
> >>>>>> }
> >>>>>>
> >>>>>> -acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32
> pm1a_control,
> >>>>>> - u32 pm1b_control)
> >>>>>> +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32
> >> val_b,
> >>>>>> + u8 extended)
> >>>>>> {
> >>>>>> int rc = 0;
> >>>>>> if (__acpi_os_prepare_sleep)
> >>>>>> - rc = __acpi_os_prepare_sleep(sleep_state,
> >>>>>> - pm1a_control, pm1b_control);
> >>>>>> + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b,
> >>>>>> + extended);
> >>>>>> if (rc < 0)
> >>>>>> return AE_ERROR;
> >>>>>> else if (rc > 0)
> >>>>>> @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8
> >>>>>> sleep_state,
> >>>>>> u32 pm1a_control,
> >>>>>> return AE_OK;
> >>>>>> }
> >>>>>>
> >>>>>> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> >>>>>> - u32 pm1a_ctrl, u32 pm1b_ctrl))
> >>>>>> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32
> >> val_a,
> >>>>>> + u32 val_b, u8 extended))
> >>>>>> {
> >>>>>> __acpi_os_prepare_sleep = func;
> >>>>>> }
> >>>>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h index
> >>>>>> 17b5b59..de99022 100644
> >>>>>> --- a/include/linux/acpi.h
> >>>>>> +++ b/include/linux/acpi.h
> >>>>>> @@ -477,11 +477,11 @@ static inline bool
> >>>>>> acpi_driver_match_device(struct device *dev,
> >>>>>> #endif /* !CONFIG_ACPI */
> >>>>>>
> >>>>>> #ifdef CONFIG_ACPI
> >>>>>> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> >>>>>> - u32 pm1a_ctrl, u32 pm1b_ctrl));
> >>>>>> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32
> >> val_a,
> >>>>>> + u32 val_b, u8 extended));
> >>>>>>
> >>>>>> -acpi_status acpi_os_prepare_sleep(u8 sleep_state,
> >>>>>> - u32 pm1a_control, u32 pm1b_control);
> >>>>>> +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32
> >> val_b,
> >>>>>> + u8 extended);
> >>>>>> #ifdef CONFIG_X86
> >>>>>> void arch_reserve_mem_area(acpi_physical_address addr, size_t
> >> size);
> >>>>>> #else
> >>>>>> @@ -491,7 +491,7 @@ static inline void
> >>>>>> arch_reserve_mem_area(acpi_physical_address addr,
> >>>>>> }
> >>>>>> #endif /* CONFIG_X86 */
> >>>>>> #else
> >>>>>> -#define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do
> >>>>>> { } while
> >>>>>> (0)
> >>>>>> +#define acpi_os_set_prepare_sleep(func, val_a, val_b, ext) do {
> >>>>>> +} while (0)
> >>>>>> #endif
> >>>>>>
> >>>>>> #if defined(CONFIG_ACPI) && defined(CONFIG_PM_RUNTIME)
> >>>>>> --
> >>>>>> 1.7.9.5
> >>>>>>
> >>>>>> --
> >>>>>> To unsubscribe from this list: send the line "unsubscribe linux-
> acpi"
> >>>>>> in the body of a message to majordomo@xxxxxxxxxxxxxxx More
> >>>>>> majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/