Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762063AbZATPyu (ORCPT ); Tue, 20 Jan 2009 10:54:50 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1756876AbZATPyj (ORCPT ); Tue, 20 Jan 2009 10:54:39 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:45324 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1756721AbZATPyi (ORCPT ); Tue, 20 Jan 2009 10:54:38 -0500 Date: Tue, 20 Jan 2009 10:54:35 -0500 (EST) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Greg KH cc: "Rafael J. Wysocki" , Andrew Morton , Andrey Borzenkov , LKML , USB list , pm list , Christian Borntraeger , Zdenek Kabelac , Ingo Molnar , Jeff Chua Subject: Re: [PATCH] USB: Fix suspend-resume of PCI USB controllers In-Reply-To: <20090120013602.GA8377@suse.de> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3030 Lines: 72 On Mon, 19 Jan 2009, Greg KH wrote: > On Tue, Jan 20, 2009 at 01:26:56AM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki > > > > Commit a0d4922da2e4ccb0973095d8d29f36f6b1b5f703 > > (USB: fix up suspend and resume for PCI host controllers) attempted > > to fix the suspend-resume of PCI USB controllers, but unfortunately > > it did that incorrectly and interrupts are left enabled by the USB > > controllers' ->suspend_late() callback as a result. This leads to > > serious problems during suspend which are very difficult to debug. > > > > Fix the issue by removing the ->suspend_late() callback of PCI > > USB controllers and moving the code from there to the ->suspend() > > callback executed with interrupts enabled. Additionally, make > > the ->resume() callback of PCI USB controllers execute > > pci_enable_wake(dev, PCI_D0, false) to disable wake-up from the > > full power state (PCI_D0). > > > > Signed-off-by: Rafael J. Wysocki > > Tested-by: Andrey Borzenkov > > Tested-by: "Jeff Chua" > > Cc: Alan Stern > > Cc: Andrew Morton > > Cc: Christian Borntraeger > > Cc: "Zdenek Kabelac" > > Cc: Ingo Molnar > > > Alan, does this look ok for you? There are a few things I don't like about it: In usb_hcd_pci_suspend, the failure path for pci_set_power_state doesn't undo all the changes that have already been made. In fact, the easiest way to do the rest of the recovery probably is to call usb_hcd_pci_resume directly. In the !has_pci_pm path we don't call pci_set_power_state. This means controllers with legacy power management won't get suspended. Maybe pci_set_power_state should be called always, and has_pci_pm be used only for interpreting the return code and printing debug messages. In usb_hcd_pci_resume, the pci_wake_from_d3 call should be moved up right next to the pci_enable_wake call. It makes no sense for them to be separated by pci_enable_device and pci_set_master. After all, even if you can't re-enable the device you probably don't want it to continue being a wakeup source. It's a little annoying that several debug messages in usb_hcd_pci_resume have been removed. Can't we display the power state upon entry, before trying to change it? It stands out that the resume method contains no call to match pci_set_power_state() in the suspend method. There should at least be a comment about it. Some of these problems predate Rafael's patch. Given that it has already helped a number of people, we might want to merge the patch and then make additional changes to address these issues. Alan Stern -- 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/