Bug#677472: [3.1->3.2 regression] Immediate wake on suspend, associated with OHCI on MCP51

[Lan Tianyu <tianyu.lan@intel.com>]

On 2012年12月18日 04:06, Alan Stern wrote:
> On Mon, 17 Dec 2012, Octavio Alvarez wrote:
> 
>> On Thu, 13 Dec 2012 00:45:05 -0800, Lan Tianyu <tianyu.lan@intel.com>
>> wrote:
>>
>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>> index f034716..9335f1b 100644
>>> --- a/drivers/usb/core/hcd.c
>>> +++ b/drivers/usb/core/hcd.c
>>> @@ -2509,7 +2509,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>>  	 * they only forward requests from the root hub.  Therefore
>>>  	 * controllers should always be enabled for remote wakeup.
>>>  	 */
>>> -	device_wakeup_enable(hcd->self.controller);
>>> +	if (!usb_hcd_wakeup_quirks(hcd->self.controller))
>>> +		device_wakeup_enable(hcd->self.controller);
>>>  	return retval;
>>>
>>>  error_create_attr_group:
>>> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
>>> index fdefd9c..ba847d3 100644
>>> --- a/drivers/usb/core/quirks.c
>>> +++ b/drivers/usb/core/quirks.c
>>> @@ -12,6 +12,7 @@
>>>   */
>>>
>>>  #include <linux/usb.h>
>>> +#include <linux/pci.h>
>>>  #include <linux/usb/quirks.h>
>>>  #include "usb.h"
>>>
>>> @@ -226,3 +227,33 @@ void usb_detect_interface_quirks(struct usb_device
>>> *udev)
>>>  		quirks);
>>>  	udev->quirks |= quirks;
>>>  }
>>> +
>>> +struct pci_hcd {
>>> +	u32 vendor;
>>> +	u32 device;
>>> +};
>>> +
>>> +static struct pci_hcd hcd_wakeup_qrk[] = {
>>> +	{PCI_VENDOR_ID_NVIDIA, 0x026d}, /* MCP51 OHCI */
>>> +	{PCI_VENDOR_ID_NVIDIA, 0x0aa5}, /* MCP79 OHCI */
>>> +	{PCI_VENDOR_ID_NVIDIA, 0x0aa7}, /* MCP79 OHCI */
>>> +	{ }
>>> +};
>>> +
>>> +int usb_hcd_wakeup_quirks(struct device *dev)
>>> +{
>>> +	struct pci_dev *pdev;
>>> +	int i;
>>> +
>>> +	if (dev->bus != (struct bus_type *)&pci_bus_type)
>>> +		return 0;
>>> +
>>> +	pdev = to_pci_dev(dev);
>>> +	for (i = 0; hcd_wakeup_qrk[i].vendor || hcd_wakeup_qrk[i].device; i++)
>>> +		if ((hcd_wakeup_qrk[i].vendor == pdev->vendor) &&
>>> +			(hcd_wakeup_qrk[i].device == pdev->device)) {
>>> +			return 1;
>>> +		}
>>> +
>>> +	return 0;
>>> +}
>>
>> I would informing the user via dmesg output about the applied quirk
>> and a point him to relevant documentation. Something like this:
>>
>> "Detected OHCI controller ID xxxx:yyyy, which requires no-wakeup quirk.
>> See Documentation/quirks/ohci-no-wakeup.txt"
> 
> Incidentally, this patch should be written differently.  Instead of a
> quirks routine, there should simply be a bad_wakeup bitflag added to
> the usb_hcd structure.  The flag should be set in ohci-pci.c by
> matching against nVidia's PCI vendor ID.
Hi Alan:

How about this patch?

Index: linux-pm/drivers/usb/host/ohci-pci.c
===================================================================
--- linux-pm.orig/drivers/usb/host/ohci-pci.c   2012-11-01
18:21:33.604460469 +0800
+++ linux-pm/drivers/usb/host/ohci-pci.c        2012-12-19
14:39:07.081601806 +0800
@@ -188,6 +188,15 @@
                pci_write_config_word(pdev, 0x50, misc | 0x0300);
 }

+static int ohci_quirk_bad_wakeup(struct usb_hcd *hcd)
+{
+       struct ohci_hcd *ohci = hcd_to_ohci (hcd);
+
+       ohci_dbg(ohci, "marked as bad wakeup.\n");
+       hcd->bad_wakeup = true;
+       return 0;
+}
+
 /* List of quirks for OHCI */
 static const struct pci_device_id ohci_pci_quirks[] = {
        {
@@ -238,6 +247,31 @@
                PCI_DEVICE(PCI_VENDOR_ID_ATI, 0x4399),
                .driver_data = (unsigned long)ohci_quirk_amd700,
        },
+       {
+               /* MCP51 OHCI */
+               PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, 0x026d),
+               .driver_data = (unsigned long)ohci_quirk_bad_wakeup,
+       },
+       {
+               /* MCP61 OHCI */
+               PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, 0x03f1),
+               .driver_data = (unsigned long)ohci_quirk_bad_wakeup,
+       },
+       {
+               /* MCP79 OHCI */
+               PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, 0x0aa5),
+               .driver_data = (unsigned long)ohci_quirk_bad_wakeup,
+       },
+       {
+               /* MCP79 OHCI */
+               PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, 0x0aa7),
+               .driver_data = (unsigned long)ohci_quirk_bad_wakeup,
+       },
+       {
+               /* SiS OHCI */
+               PCI_DEVICE(PCI_VENDOR_ID_SI, 7001),
+               .driver_data = (unsigned long)ohci_quirk_bad_wakeup,
+       },

        /* FIXME for some of the early AMD 760 southbridges, OHCI
         * won't work at all.  blacklist them.
Index: linux-pm/include/linux/usb/hcd.h
===================================================================
--- linux-pm.orig/include/linux/usb/hcd.h       2012-11-01
18:21:34.732460451 +0800
+++ linux-pm/include/linux/usb/hcd.h    2012-12-19 10:48:43.305822774 +0800
@@ -138,6 +138,7 @@
        resource_size_t         rsrc_start;     /* memory/io resource
start */
        resource_size_t         rsrc_len;       /* memory/io resource
length */
        unsigned                power_budget;   /* in mA, 0 = no limit */
+       bool                    bad_wakeup;

        /* bandwidth_mutex should be taken before adding or removing
         * any new bus bandwidth constraints:
Index: linux-pm/drivers/usb/core/hcd.c
===================================================================
--- linux-pm.orig/drivers/usb/core/hcd.c        2012-12-18
10:21:38.287229824 +0800
+++ linux-pm/drivers/usb/core/hcd.c     2012-12-19 13:51:57.085647043 +0800
@@ -2509,7 +2509,8 @@
         * they only forward requests from the root hub.  Therefore
         * controllers should always be enabled for remote wakeup.
         */
-       device_wakeup_enable(hcd->self.controller);
+       if (!hcd->bad_wakeup)
+               device_wakeup_enable(hcd->self.controller);
        return retval;

 error_create_attr_group:

> 
> Alan Stern
> 


-- 
Best regards
Tianyu Lan