Hi,

On 25.05.2012 22:00, Rafael J. Wysocki wrote:

On Friday, May 25, 2012, Alan Stern wrote:

quoted

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.

It might check adev->wakeup.flags.valid instead of device_may_wakeup(dev),
but that's a different story.

So the only difference made by the patch is te hunk in pci_target_state().