* Re: [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops
2007-07-18 0:02 ` [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops Andrew Morton
@ 2007-07-18 6:41 ` Rafael J. Wysocki
2007-07-19 2:05 ` Len Brown
1 sibling, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2007-07-18 6:41 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-acpi, LKML, Alan Stern, David Brownell, Len Brown,
Nigel Cunningham, Pavel Machek, Shaohua Li, Johannes Berg,
Paul Mackerras, Russell King
On Wednesday, 18 July 2007 02:02, Andrew Morton wrote:
> On Tue, 17 Jul 2007 22:40:06 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > 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.
Well, in fact I didn't add those. They were just moved from below the comment
before acpi_pm_prepare(). Seems logical to me ...
> checkpatch will detect this error.
Well, I'd call it a false positive. :-)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops
2007-07-18 0:02 ` [Resend][PATCH 1/9] ACPI: Implement the set_target() callback from pm_ops Andrew Morton
2007-07-18 6:41 ` Rafael J. Wysocki
@ 2007-07-19 2:05 ` Len Brown
2007-07-19 9:17 ` Rafael J. Wysocki
1 sibling, 1 reply; 4+ messages in thread
From: Len Brown @ 2007-07-19 2:05 UTC (permalink / raw)
To: Andrew Morton, Rafael J. Wysocki
Cc: linux-acpi, LKML, Alan Stern, David Brownell, Nigel Cunningham,
Pavel Machek, Shaohua Li, Johannes Berg, Paul Mackerras,
Russell King
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@intel.com>
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@sisk.pl> wrote:
>
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > 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@sisk.pl>
>
> 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@sisk.pl>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> ---
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 4+ messages in thread