Re: [PATCH] ACPI / PCI: Make _SxD/_SxW check follow ACPI 4.0a spec

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Saturday, May 26, 2012, Oleksij Rempel wrote:
> Am 26.05.2012 08:03, schrieb Oleksij Rempel (fishor):
> > Hi,
> > 
> > On 25.05.2012 22:00, Rafael J. Wysocki wrote:
> >> On Friday, May 25, 2012, Alan Stern wrote:
> >>> Oleksij:
> >>>
> >>> Please take a look at this bug report:
> >>>
> >>>     https://bugzilla.kernel.org/show_bug.cgi?id=43278
> >>>
> >>> Apparently your patch breaks wakeup on this machine by preventing the
> >>> USB host controllers from being put into D3.
> >>
> >> I think the patch is incorrect, actually.
> >>
> >> First, if you look at the first hunk:
> >>
> >> -       if (acpi_target_sleep_state>  ACPI_STATE_S0)
> >> +       if (acpi_target_sleep_state>  ACPI_STATE_S0) {
> >> +               acpi_status status;
> >> +
> >>                  acpi_evaluate_integer(handle, acpi_method, NULL,&d_min);
> >>
> >> +               if (device_may_wakeup(dev)) {
> >> +                       acpi_method[3] = 'W';
> >> +                       status = acpi_evaluate_integer(handle,
> >> acpi_method,
> >> +                                                      NULL,&d_max);
> >> +                       if (ACPI_FAILURE(status))
> >> +                               d_max = d_min;
> >> +               }
> >> +       }
> >>
> >> it will do something like this: if the device is wakeup-capable, get
> >> d_max
> >> from _SxW, unless it fails.  However, the code just below in that
> >> function:
> >>
> >>     if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> >>         (device_may_wakeup(dev)&&
> >>     adev->wakeup.sleep_state<= acpi_target_sleep_state)) {
> >>         acpi_status status;
> >>
> >>         acpi_method[3] = 'W';
> >>         status = acpi_evaluate_integer(handle, acpi_method, NULL,
> >>                         &d_max);
> >>         if (ACPI_FAILURE(status)) {
> >>             if (acpi_target_sleep_state != ACPI_STATE_S0 ||
> >>                 status != AE_NOT_FOUND)
> >>                 d_max = d_min;
> >>         } else if (d_max<  d_min) {
> >>             /* Warn the user of the broken DSDT */
> >>             printk(KERN_WARNING "ACPI: Wrong value from %s\n",
> >>                 acpi_method);
> >>             /* Sanitize it */
> >>             d_min = d_max;
> >>         }
> >>     }
> >>
> >> does _exactly_ the same thing (it only has a more sophisticated error
> >> code path).
> > 
> > Not really correct. The code below check _SxW state on wake up. This
> > code checks _SxW on suspend.
> 
> oops i misreaded the code. it really do the same. except this part is
> not executed because of this check: "adev->wakeup.sleep_state <=
> acpi_target_sleep_state"

Precisely.

Thanks,
Rafael