Re: [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops

From: Len Brown
Date: Wed Jul 18 2007 - 22:06:54 EST

Next message: Nigel Cunningham: "Re: [PATH 0/1] Kexec jump - v2 - the first step to kexec based hibernation"
Previous message: Benjamin Herrenschmidt: "Re: [git patches] two warning fixes"
In reply to: Rafael J. Wysocki: "Re: [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops"
Next in thread: Rafael J. Wysocki: "Re: [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Rafael,
Please delete the instances of the string ACPI_STATE_S2 -- it doesn't exist in practice
and we don't want to imply it exists by inventing it here.

otherwise:
Acked-by: Len Brown <len.brown@xxxxxxxxx>

thanks,
-Len

On Tuesday 17 July 2007 20:02, Andrew Morton wrote:
> On Tue, 17 Jul 2007 22:40:06 +0200
> "Rafael J. Wysocki" <rjw@xxxxxxx> wrote:
>
> > From: Rafael J. Wysocki <rjw@xxxxxxx>
> >
> > In the future some drivers may need to use ACPI to determine the low power
> > states in which to place their devices, but to provide the drivers with this
> > information the ACPI core needs to know what sleep state the system is going to
> > enter. Namely, the device's state should not be too high power for given system
> > sleep state and, if the device is supposed to be able to wake up the system, its
> > state should not be too low power for the wake up to be possible). For this
> > purpose, the ACPI core needs to implement the set_target() method in 'struct
> > pm_ops' and store the target system sleep state passed by the PM core in a
> > variable.
> >
>
> Len, I can add this to the to-send-to-Len queue, but it would be more
> convenient at this end if you were to ack it and I'll send it in.
> Preferences?
>
>
> >
> > Index: linux-2.6.22-git5/drivers/acpi/sleep/main.c
> > ===================================================================
> > --- linux-2.6.22-git5.orig/drivers/acpi/sleep/main.c
> > +++ linux-2.6.22-git5/drivers/acpi/sleep/main.c
> > @@ -34,34 +34,54 @@ static u32 acpi_suspend_states[] = {
> >
> > static int init_8259A_after_S1;
> >
> > +extern int acpi_sleep_prepare(u32 acpi_state);
> > +extern void acpi_power_off(void);
>
> weep. Please don't do this.
>
> checkpatch will detect this error. Please check patches with checkpatch.
>
>
> (Full patch reproduced below for the benefit of the newly-added linux-acpi
> cc):
>
>
> From: Rafael J. Wysocki <rjw@xxxxxxx>
>
> In the future some drivers may need to use ACPI to determine the low power
> states in which to place their devices, but to provide the drivers with this
> information the ACPI core needs to know what sleep state the system is going to
> enter. Namely, the device's state should not be too high power for given system
> sleep state and, if the device is supposed to be able to wake up the system, its
> state should not be too low power for the wake up to be possible). For this
> purpose, the ACPI core needs to implement the set_target() method in 'struct
> pm_ops' and store the target system sleep state passed by the PM core in a
> variable.
>
> Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx>
> Acked-by: Pavel Machek <pavel@xxxxxx>
> ---
> drivers/acpi/sleep/main.c | 84 ++++++++++++++++++++++++++++------------------
> 1 file changed, 52 insertions(+), 32 deletions(-)
>
> Index: linux-2.6.22-git5/drivers/acpi/sleep/main.c
> ===================================================================
> --- linux-2.6.22-git5.orig/drivers/acpi/sleep/main.c
> +++ linux-2.6.22-git5/drivers/acpi/sleep/main.c
> @@ -34,34 +34,54 @@ static u32 acpi_suspend_states[] = {
>
> static int init_8259A_after_S1;
>
> +extern int acpi_sleep_prepare(u32 acpi_state);
> +extern void acpi_power_off(void);
> +
> +static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> +
> +/**
> + * acpi_pm_set_target - Set the target system sleep state to the state
> + * associated with given @pm_state, if supported.
> + */
> +
> +static int acpi_pm_set_target(suspend_state_t pm_state)
> +{
> + u32 acpi_state = acpi_suspend_states[pm_state];
> + int error = 0;
> +
> + if (sleep_states[acpi_state]) {
> + acpi_target_sleep_state = acpi_state;
> + } else {
> + printk(KERN_ERR "ACPI does not support this state: %d\n",
> + pm_state);
> + error = -ENOSYS;
> + }
> + return error;
> +}
> +
> /**
> * acpi_pm_prepare - Do preliminary suspend work.
> - * @pm_state: suspend state we're entering.
> + * @pm_state: ignored
> *
> - * Make sure we support the state. If we do, and we need it, set the
> - * firmware waking vector and do arch-specific nastiness to get the
> - * wakeup code to the waking vector.
> + * If necessary, set the firmware waking vector and do arch-specific
> + * nastiness to get the wakeup code to the waking vector.
> */
>
> -extern int acpi_sleep_prepare(u32 acpi_state);
> -extern void acpi_power_off(void);
> -
> static int acpi_pm_prepare(suspend_state_t pm_state)
> {
> - u32 acpi_state = acpi_suspend_states[pm_state];
> + int error = acpi_sleep_prepare(acpi_target_sleep_state);
>
> - if (!sleep_states[acpi_state]) {
> - printk("acpi_pm_prepare does not support %d \n", pm_state);
> - return -EPERM;
> - }
> - return acpi_sleep_prepare(acpi_state);
> + if (error)
> + acpi_target_sleep_state = ACPI_STATE_S0;
> +
> + return error;
> }
>
> /**
> * acpi_pm_enter - Actually enter a sleep state.
> - * @pm_state: State we're entering.
> + * @pm_state: ignored
> *
> - * Flush caches and go to sleep. For STR or STD, we have to call
> + * Flush caches and go to sleep. For STR or S2, we have to call
> * arch-specific assembly, which in turn call acpi_enter_sleep_state().
> * It's unfortunate, but it works. Please fix if you're feeling frisky.
> */
> @@ -70,31 +90,32 @@ static int acpi_pm_enter(suspend_state_t
> {
> acpi_status status = AE_OK;
> unsigned long flags = 0;
> - u32 acpi_state = acpi_suspend_states[pm_state];
> + u32 acpi_state = acpi_target_sleep_state;
>
> ACPI_FLUSH_CPU_CACHE();
>
> /* Do arch specific saving of state. */
> - if (pm_state > PM_SUSPEND_STANDBY) {
> + if (acpi_state == ACPI_STATE_S2 || acpi_state == ACPI_STATE_S3) {
> int error = acpi_save_state_mem();
> - if (error)
> +
> + if (error) {
> + acpi_target_sleep_state = ACPI_STATE_S0;
> return error;
> + }
> }
>
> local_irq_save(flags);
> acpi_enable_wakeup_device(acpi_state);
> - switch (pm_state) {
> - case PM_SUSPEND_STANDBY:
> + switch (acpi_state) {
> + case ACPI_STATE_S1:
> barrier();
> status = acpi_enter_sleep_state(acpi_state);
> break;
>
> - case PM_SUSPEND_MEM:
> + case ACPI_STATE_S2:
> + case ACPI_STATE_S3:
> do_suspend_lowlevel();
> break;
> -
> - default:
> - return -EINVAL;
> }
>
> /* ACPI 3.0 specs (P62) says that it's the responsabilty
> @@ -107,12 +128,8 @@ static int acpi_pm_enter(suspend_state_t
> local_irq_restore(flags);
> printk(KERN_DEBUG "Back to C!\n");
>
> - /* restore processor state
> - * We should only be here if we're coming back from STR or STD.
> - * And, in the case of the latter, the memory image should have already
> - * been loaded from disk.
> - */
> - if (pm_state > PM_SUSPEND_STANDBY)
> + /* restore processor state */
> + if (acpi_state == ACPI_STATE_S2 || acpi_state == ACPI_STATE_S3)
> acpi_restore_state_mem();
>
> return ACPI_SUCCESS(status) ? 0 : -EFAULT;
> @@ -120,7 +137,7 @@ static int acpi_pm_enter(suspend_state_t
>
> /**
> * acpi_pm_finish - Finish up suspend sequence.
> - * @pm_state: State we're coming out of.
> + * @pm_state: ignored
> *
> * This is called after we wake back up (or if entering the sleep state
> * failed).
> @@ -128,7 +145,7 @@ static int acpi_pm_enter(suspend_state_t
>
> static int acpi_pm_finish(suspend_state_t pm_state)
> {
> - u32 acpi_state = acpi_suspend_states[pm_state];
> + u32 acpi_state = acpi_target_sleep_state;
>
> acpi_leave_sleep_state(acpi_state);
> acpi_disable_wakeup_device(acpi_state);
> @@ -136,6 +153,8 @@ static int acpi_pm_finish(suspend_state_
> /* reset firmware waking vector */
> acpi_set_firmware_waking_vector((acpi_physical_address) 0);
>
> + acpi_target_sleep_state = ACPI_STATE_S0;
> +
> if (init_8259A_after_S1) {
> printk("Broken toshiba laptop -> kicking interrupts\n");
> init_8259A(0);
> @@ -176,6 +195,7 @@ static int acpi_pm_state_valid(suspend_s
>
> static struct pm_ops acpi_pm_ops = {
> .valid = acpi_pm_state_valid,
> + .set_target = acpi_pm_set_target,
> .prepare = acpi_pm_prepare,
> .enter = acpi_pm_enter,
> .finish = acpi_pm_finish,
>
> -
> 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/
>
-
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/

Next message: Nigel Cunningham: "Re: [PATH 0/1] Kexec jump - v2 - the first step to kexec based hibernation"
Previous message: Benjamin Herrenschmidt: "Re: [git patches] two warning fixes"
In reply to: Rafael J. Wysocki: "Re: [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops"
Next in thread: Rafael J. Wysocki: "Re: [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]