Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Kai-Heng Feng]

Hi,

On Wed, Nov 9, 2016 at 8:32 PM, Bjørn Mork  wrote:
> Oliver Neukum  writes:
>
>> On Tue, 2016-11-08 at 13:44 -0500, Alan Stern wrote:
>>
>>> These problems could very well be caused by running at SuperSpeed
>>> (USB-3) instead of high speed (USB-2).

Yes, it's running at SuperSpeed, on a Kabylake laptop.

It does not have this issue on a Broadwell laptop, also running at SuperSpeed.

>>>
>>> Is there any way to test what happens when the device is attached to
>>> the computer by a USB-2 cable?  That would prevent it from operating at
>>> SuperSpeed.

I recall old Intel PCH can change the USB host from XHCI to EHCI,
newer PCH does not have this option.

Is there a way to force XHCI run at HighSpeed?

>>>
>>> The main point, however, is that the proposed patch doesn't seem to
>>> address the true problem, which is that the device gets suspended
>>> between probes.  The patch only tries to prevent it from being
>>> suspended during a probe -- which is already prevented by the USB core.
>>
>> But why doesn't it fail during normal operation?
>>
>> I suspect that its firmware requires the altsetting
>>
>> /* should we change control altsetting on a NCM/MBIM function? */
>> if (cdc_ncm_select_altsetting(intf) == CDC_NCM_COMM_ALTSETTING_MBIM) 
>> {
>> data_altsetting = CDC_NCM_DATA_ALTSETTING_MBIM;
>> ret = cdc_mbim_set_ctrlalt(dev, intf, 
>> CDC_NCM_COMM_ALTSETTING_MBIM);
>>
>> to be set before it accepts a suspension.
>
> Could be, but I don't think so.  The above code is effectively a noop
> unless the function is a combined NCM/MBIM function.  Something I've
> never seen on a Sierra Wireless device (ignoring the infamous EM7345,
> which really is an Intel device).
>
> This is a typical example of a Sierra Wireless modem configured for
> MBIM:
>
> P:  Vendor=1199 ProdID=9079 Rev= 0.06
> S:  Manufacturer=Sierra Wireless, Incorporated
> S:  Product=Sierra Wireless EM7455 Qualcomm Snapdragon X7 LTE-A
> S:  SerialNumber=LF615126xxx
> C:* #Ifs= 2 Cfg#= 1 Atr=a0 MxPwr=500mA
> A:  FirstIf#=12 IfCount= 2 Cls=02(comm.) Sub=0e Prot=00
> I:* If#=12 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=0e Prot=00 Driver=(none)
> E:  Ad=82(I) Atr=03(Int.) MxPS=  64 Ivl=32ms
> I:* If#=13 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=02 Driver=(none)
> I:  If#=13 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=(none)
> E:  Ad=81(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=01(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
>
>
> The control interface of plain MBIM functions will always have a single
> altsetting, like the example above. So cdc_ncm_select_altsetting(intf)
> returns "0", while CDC_NCM_COMM_ALTSETTING_MBIM is "1".
>
>
> Just for reference, using the Intel^H^H^H^H^HEM7345 as example, this is
> what a combined NCM/MBIM function looks like:
>
>
> P:  Vendor=1199 ProdID=a001 Rev=17.29
> S:  Manufacturer=Sierra Wireless Inc.
> S:  Product=Sierra Wireless EM7345 4G LTE
> S:  SerialNumber=013937000xx
> C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=100mA
> A:  FirstIf#= 0 IfCount= 2 Cls=02(comm.) Sub=0d Prot=00
> A:  FirstIf#= 2 IfCount= 2 Cls=02(comm.) Sub=02 Prot=01
> I:  If#= 0 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=0d Prot=00 Driver=cdc_mbim
> E:  Ad=81(I) Atr=03(Int.) MxPS=  64 Ivl=1ms
> I:* If#= 0 Alt= 1 #EPs= 1 Cls=02(comm.) Sub=0e Prot=00 Driver=cdc_mbim
> E:  Ad=81(I) Atr=03(Int.) MxPS=  64 Ivl=1ms
> I:  If#= 1 Alt= 0 #EPs= 0 Cls=0a(data ) Sub=00 Prot=01 Driver=cdc_mbim
> I:  If#= 1 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=01 Driver=cdc_mbim
> E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 1 Alt= 2 #EPs= 2 Cls=0a(data ) Sub=00 Prot=02 Driver=cdc_mbim
> E:  Ad=82(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=02(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> I:* If#= 2 Alt= 0 #EPs= 1 Cls=02(comm.) Sub=02 Prot=01 Driver=(none)
> E:  Ad=83(I) Atr=03(Int.) MxPS=  64 Ivl=1ms
> I:* If#= 3 Alt= 0 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=(none)
> E:  Ad=84(I) Atr=02(Bulk) MxPS= 512 Ivl=0ms
> E:  Ad=04(O) Atr=02(Bulk) MxPS= 512 Ivl=0ms
>
>
> And this is what the code you quote is trying to deal with.  Note the
> different subclass of altsetting 0 and 1 This is incredibly ugly.
>
> FWIW, the modem in question cannot be an EM7345. That modem does not
> have the static interface numbering oddity.  Another sign that it isn't
> a true Sierra device.

Yes, this modem is an EM7445.

>
>
>
>
> Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Kai-Heng Feng]

Hi,

On Mon, Nov 7, 2016 at 7:02 PM, Oliver Neukum <oneu...@suse.com> wrote:
> On Fri, 2016-11-04 at 17:57 +0800, Kai-Heng Feng wrote:
>> Sometimes cdc_mbim failed to probe if runtime pm is enabled:
>> [9.305626] cdc_mbim: probe of 2-2:1.12 failed with error -22
>>
>> This can be solved by increase its pm usage counter.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>
> For the record:
>
> NAK. This fixes a symptom. If this patch helps something is broken in
> device core. We need to find that.
>

Please check attached dmesg with usbcore.dyndbg="+p".

Thanks!

> Regards
> Oliver
>
>


dmesg
Description: Binary data

Re: cdc_mbim: probe of 2-2:1.12 failed with error -22

[Kai-Heng Feng]

On Wed, Nov 16, 2016 at 6:56 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Wed, Nov 16, 2016 at 6:47 PM, Greg KH <g...@kroah.com> wrote:
>> On Wed, Nov 16, 2016 at 06:42:27PM +0800, Kai-Heng Feng wrote:
>>> Originally I sent a not-working patch to the mailing list [1], turns
>>> out the patch is far from correct.
>>>
>>> Bjørn Mork suggests that we can cover the USB3 pair diff pins with
>>> tape to do some experiment, but the vendor told me that there's no
>>> such pin.
>>>
>>> According to the attached log, looks like xhci driver failed to get
>>> status from port after resume, eventually make the cdc_mbim probe
>>> failed:
>>>
>>> [   10.038428] xhci_hcd :00:14.0: get port status, actual port 1
>>> status  = 0x202c0
>>> [   10.038429] xhci_hcd :00:14.0: Get port status returned 0x102c0
>>> [   10.038463] usb usb2-port2: status 0001.02c0 after resume, -19
>>> [   10.038465] usb 2-2: can't resume, status -19
>>> [   10.038465] usb usb2-port2: logical disconnect
>>> [   10.038472] xhci_hcd :00:14.0: Cannot set link state.
>>
>> Odd.  What kernel version is this?
>>
>
> xhci/for-usb-next branch:
> 6005a545cadb2adca64350c7aee17d002563e8c7 usb: hub: Fix auto-remount of
> safely removed or ejected USB-3 devices
>
> The latest mainline tag in this branch is 4.9-rc3.

[CC to maintainers]
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cdc_mbim: probe of 2-2:1.12 failed with error -22

[Kai-Heng Feng]

On Wed, Nov 16, 2016 at 6:47 PM, Greg KH <g...@kroah.com> wrote:
> On Wed, Nov 16, 2016 at 06:42:27PM +0800, Kai-Heng Feng wrote:
>> Originally I sent a not-working patch to the mailing list [1], turns
>> out the patch is far from correct.
>>
>> Bjørn Mork suggests that we can cover the USB3 pair diff pins with
>> tape to do some experiment, but the vendor told me that there's no
>> such pin.
>>
>> According to the attached log, looks like xhci driver failed to get
>> status from port after resume, eventually make the cdc_mbim probe
>> failed:
>>
>> [   10.038428] xhci_hcd :00:14.0: get port status, actual port 1
>> status  = 0x202c0
>> [   10.038429] xhci_hcd :00:14.0: Get port status returned 0x102c0
>> [   10.038463] usb usb2-port2: status 0001.02c0 after resume, -19
>> [   10.038465] usb 2-2: can't resume, status -19
>> [   10.038465] usb usb2-port2: logical disconnect
>> [   10.038472] xhci_hcd :00:14.0: Cannot set link state.
>
> Odd.  What kernel version is this?
>

xhci/for-usb-next branch:
6005a545cadb2adca64350c7aee17d002563e8c7 usb: hub: Fix auto-remount of
safely removed or ejected USB-3 devices

The latest mainline tag in this branch is 4.9-rc3.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Kai-Heng Feng]

On Mon, Nov 14, 2016 at 3:34 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Fri, Nov 11, 2016 at 10:44 PM, Mathias Nyman
> <mathias.ny...@linux.intel.com> wrote:
>> On 10.11.2016 13:22, Oliver Neukum wrote:
>>>
>>> On Thu, 2016-11-10 at 12:09 +0100, Bjørn Mork wrote:
>>>>
>>>> Kai-Heng Feng <kai.heng.f...@canonical.com> writes:
>>>>>
>>>>> On Wed, Nov 9, 2016 at 8:32 PM, Bjørn Mork <bj...@mork.no> wrote:
>>>>>>
>>>>>> Oliver Neukum <oneu...@suse.com> writes:
>>>>>>
>>>>>>> On Tue, 2016-11-08 at 13:44 -0500, Alan Stern wrote:
>>>>>>>
>>>>>>>> These problems could very well be caused by running at SuperSpeed
>>>>>>>> (USB-3) instead of high speed (USB-2).
>>>>>
>>>>>
>>>>> Yes, it's running at SuperSpeed, on a Kabylake laptop.
>>>>>
>>>>> It does not have this issue on a Broadwell laptop, also running at
>>>>> SuperSpeed.
>>>>
>>>>
>>>> Then I must join Oliver, being very surprised by where in the stack you
>>>> attempt to fix the issue.  What you write above indicates a problem in
>>>> pci bridge or usb host controller, doesn't it?
>
> Yes, I was totally wrong about that.
>
>>>
>>>
>>> Indeed. And this means we need an XHCI specialist.
>>> Mathias, we have a failure specific to one implementation of XHCI.
>>>
>>
>>
>> Could be related to resume singnalling time.
>> Does the xhci fix for it in 4.9-rc3 help?
>>
>> commit 7d3b016a6f5a0fa610dfd02b05654c08fa4ae514
>> xhci: use default USB_RESUME_TIMEOUT when resuming ports.
>>
>> It doesn't directly explain why it would work on Broadwell but not Kabylake,
>> but it resolved very similar cases.
>>
>> If not, then adding dynamic debug for xhci could show something.
>
> I tried the latest commit, 6005a545cadb2adca64350c7aee17d002563e8c7,
> on for-usb-next branch.
>
> Now the cdc_mbim still probe failed at the first time, but somehow it
> re-probed again with a success.
>
> I reverted commit 7d3b016a6f5a0fa610dfd02b05654c08fa4ae514 and the
> behavior is the same, first time probed failed, second time probed
> success.
>
> The attached dmesg is with usbcore and xhci_hcd dynamic debug enabled.

I filed a bug report on bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=187861

>
>>
>> -Mathias
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbnet: prevent device rpm suspend in usbnet_probe function

[Kai-Heng Feng]

Sometimes cdc_mbim failed to probe if runtime pm is enabled:
[9.305626] cdc_mbim: probe of 2-2:1.12 failed with error -22

This can be solved by increase its pm usage counter.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/net/usb/usbnet.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index d5071e3..f77b4bf 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1674,12 +1674,15 @@ usbnet_probe (struct usb_interface *udev, const struct 
usb_device_id *prod)
net->watchdog_timeo = TX_TIMEOUT_JIFFIES;
net->ethtool_ops = _ethtool_ops;
 
+   if (usb_autopm_get_interface(dev->intf) < 0)
+   goto out1;
+
// allow device-specific bind/init procedures
// NOTE net->name still not usable ...
if (info->bind) {
status = info->bind (dev, udev);
if (status < 0)
-   goto out1;
+   goto out2;
 
// heuristic:  "usb%d" for links we know are two-host,
// else "eth%d" when there's reasonable doubt.  userspace
@@ -1772,6 +1775,8 @@ usbnet_probe (struct usb_interface *udev, const struct 
usb_device_id *prod)
 out3:
if (info->unbind)
info->unbind (dev, udev);
+out2:
+   usb_autopm_put_interface(dev->intf);
 out1:
/* subdrivers must undo all they did in bind() if they
 * fail it, but we may fail later and a deferred kevent
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

The Realtek r8153 ethernet does not work on Genesys Logic hub, no-lpm
quirk can make it work.

Since another r8153 dongle at my hand does not have the issue, so add
the quirk to the hub instead.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 3116edfcdc18..c96daf34431e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -150,6 +150,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* appletouch */
{ USB_DEVICE(0x05ac, 0x021a), .driver_info = USB_QUIRK_RESET_RESUME },
 
+   /* Moshi USB to Ethernet Adapter */
+   { USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
+
/* Avision AV600U */
{ USB_DEVICE(0x0638, 0x0a13), .driver_info =
  USB_QUIRK_STRING_FETCH_255 },
-- 
2.13.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: quirks: add delay init quirk for Corsair Strafe RGB keyboard

[Kai-Heng Feng]

Corsair Strafe RGB keyboard has trouble to initialize:

[ 1.679455] usb 3-6: new full-speed USB device number 4 using xhci_hcd
[ 6.871136] usb 3-6: unable to read config index 0 descriptor/all
[ 6.871138] usb 3-6: can't read configurations, error -110
[ 6.991019] usb 3-6: new full-speed USB device number 5 using xhci_hcd
[ 12.246642] usb 3-6: unable to read config index 0 descriptor/all
[ 12.246644] usb 3-6: can't read configurations, error -110
[ 12.366555] usb 3-6: new full-speed USB device number 6 using xhci_hcd
[ 17.622145] usb 3-6: unable to read config index 0 descriptor/all
[ 17.622147] usb 3-6: can't read configurations, error -110
[ 17.742093] usb 3-6: new full-speed USB device number 7 using xhci_hcd
[ 22.997715] usb 3-6: unable to read config index 0 descriptor/all
[ 22.997716] usb 3-6: can't read configurations, error -110

Although it may work after several times unpluging/pluging:

[ 68.195240] usb 3-6: new full-speed USB device number 11 using xhci_hcd
[ 68.337459] usb 3-6: New USB device found, idVendor=1b1c, idProduct=1b20
[ 68.337463] usb 3-6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 68.337466] usb 3-6: Product: Corsair STRAFE RGB Gaming Keyboard
[ 68.337468] usb 3-6: Manufacturer: Corsair
[ 68.337470] usb 3-6: SerialNumber: 0F013021AEB8046755A93ED3F5001941

Tried three quirks: USB_QUIRK_DELAY_INIT, USB_QUIRK_NO_LPM and
USB_QUIRK_DEVICE_QUALIFIER, user confirmed that USB_QUIRK_DELAY_INIT alone
can workaround this issue. Hence add the quirk for Corsair Strafe RGB.

BugLink: https://bugs.launchpad.net/bugs/1678477
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 574da2b4529c..1ea5060dae69 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -217,6 +217,9 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x1a0a, 0x0200), .driver_info =
USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL },
 
+   /* Corsair Strafe RGB */
+   { USB_DEVICE(0x1b1c, 0x1b20), .driver_info = USB_QUIRK_DELAY_INIT },
+
/* Acer C120 LED Projector */
{ USB_DEVICE(0x1de1, 0xc102), .driver_info = USB_QUIRK_NO_LPM },
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Runtime PM enabled USB port no longer responses to hotplug after commit dec08194ff

[Kai-Heng Feng]

Hi,

My ASUS PRIME B350M-A uses this XHCI chip:

03:00.0 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] USB
3.1 XHCI Controller [1022:43bb] (rev 02)

...which matches to the PCI_DEVICE_ID_AMD_PROMONTORYA_2.

Revert commit dec08194ffeccfa1cf085906b53d301930eae18f ("xhci: Limit
USB2 port wake support for AMD Promontory hosts") can solve the
regression.

Now both front panel USB ports stop responding to hotplug events now.
Is it an overkill to disable all wake up bits?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

On Tue, Aug 8, 2017 at 4:28 PM, Oliver Neukum <oneu...@suse.com> wrote:
> Am Dienstag, den 08.08.2017, 14:32 +0800 schrieb Kai-Heng Feng:
>> Moshi USB to Ethernet Adapter internally uses a Genesys Logic hub to
>> connect to Realtek r8153.
>>
>> The Realtek r8153 ethernet does not work on the internal hub, no-lpm quirk
>> can make it work.
>>
>> Since another r8153 dongle at my hand does not have the issue, so add
>> the quirk to the hub instead.
>
> But in the comment you say you add it to the device!
> This makes no sense.

You are right, I forgot to update the comment. My bad.

>
> [..]
>> + /* Moshi USB to Ethernet Adapter */
>> + { USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
>> +
>
> Now is this a hub or a device? If this is a Genesys Logic hub,
> you need to say that clearly and state that it is an internal hub.

It's the Genesys Logic hub that being used internally.

I'll update the comment more clearly.

Thanks.

>
> Regards
> Oliver
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

Moshi USB to Ethernet Adapter internally uses a Genesys Logic hub to
connect to Realtek r8153.

The Realtek r8153 ethernet does not work on the internal hub, no-lpm quirk
can make it work.

Since another r8153 dongle at my hand does not have the issue, so add
the quirk to the hub instead.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2: Clarify that the adapter uses a hub internally.

 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 3116edfcdc18..c96daf34431e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -150,6 +150,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* appletouch */
{ USB_DEVICE(0x05ac, 0x021a), .driver_info = USB_QUIRK_RESET_RESUME },
 
+   /* Moshi USB to Ethernet Adapter */
+   { USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
+
/* Avision AV600U */
{ USB_DEVICE(0x0638, 0x0a13), .driver_info =
  USB_QUIRK_STRING_FETCH_255 },
-- 
2.13.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

Moshi USB to Ethernet Adapter internally uses a Genesys Logic hub to
connect to Realtek r8153.

The Realtek r8153 ethernet does not work on the internal hub, no-lpm quirk
can make it work.

Since another r8153 dongle at my hand does not have the issue, so add
the quirk to the Genesys Logic hub instead.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v3: Update comment to reflect the quirk is for the hub.

v2: Clarify that the adapter uses a hub internally.

 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 3116edfcdc18..65a87efc100e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -150,6 +150,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* appletouch */
{ USB_DEVICE(0x05ac, 0x021a), .driver_info = USB_QUIRK_RESET_RESUME },
 
+   /* Genesys Logic hub, internally used by Moshi USB to Ethernet Adapter 
*/
+   { USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
+
/* Avision AV600U */
{ USB_DEVICE(0x0638, 0x0a13), .driver_info =
  USB_QUIRK_STRING_FETCH_255 },
-- 
2.14.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: quirks: Add no-lpm quirk for Moshi USB to Ethernet Adapter

[Kai-Heng Feng]

On Mon, Aug 7, 2017 at 5:08 PM, Oliver Neukum <oneu...@suse.com> wrote:
> Am Freitag, den 04.08.2017, 17:34 +0800 schrieb Kai-Heng Feng:
>> The Realtek r8153 ethernet does not work on Genesys Logic hub, no-lpm
>> quirk can make it work.
>
> So can you confirm it works with LPM on another hub?

Yes, another r8153 dongle (Dell Type-C to Ethernet) does not have this issue.

>
>> Since another r8153 dongle at my hand does not have the issue, so add
>> the quirk to the hub instead.
>
> This does not match the comment in the patch. This needs to
> be clarified.

The Moshi USB to Ethernet Adapter internally has a Genesys Logic hub.
Ethernet chip r8153 is connected to the Genesys Logic hub.

I'll resend a patch with this information if it's clear enough.

>
> Regards
> Oliver
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: xhci: Renesas uPD720202 needs short TX quirk

[Kai-Heng Feng]

When plugging Logitech C920 webcam, warning messages filled up dmesg:
[77117.655018] xhci_hcd :0c:00.0: WARN Successful completion on short TX: 
needs XHCI_TRUST_TX_LENGTH quirk?
[77117.659018] xhci_hcd :0c:00.0: WARN Successful completion on short TX: 
needs XHCI_TRUST_TX_LENGTH quirk?
[77122.622952] handle_tx_event: 541 callbacks suppressed

No more warning messages with XHCI_TRUST_TX_LENGTH applied.

BugLink: https://bugs.launchpad.net/bugs/1710548
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/host/xhci-pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 8071c8fdd15e..8566b43e19ba 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -202,8 +202,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
xhci->quirks |= XHCI_BROKEN_STREAMS;
}
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0015)
+   pdev->device == 0x0015) {
xhci->quirks |= XHCI_RESET_ON_RESUME;
+   xhci->quirks |= XHCI_TRUST_TX_LENGTH;
+   }
if (pdev->vendor == PCI_VENDOR_ID_VIA)
xhci->quirks |= XHCI_RESET_ON_RESUME;
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: xhci: Renesas uPD720202 needs short TX quirk

[Kai-Heng Feng]

Hi,

On Fri, Aug 18, 2017 at 3:22 PM, Felipe Balbi
<felipe.ba...@linux.intel.com> wrote:
>
> hi,
>
> Kai-Heng Feng <kai.heng.f...@canonical.com> writes:
>> When plugging Logitech C920 webcam, warning messages filled up dmesg:
>> [77117.655018] xhci_hcd :0c:00.0: WARN Successful completion on short 
>> TX: needs XHCI_TRUST_TX_LENGTH quirk?
>> [77117.659018] xhci_hcd :0c:00.0: WARN Successful completion on short 
>> TX: needs XHCI_TRUST_TX_LENGTH quirk?
>
> have you confirmed this is needed for this controller?
I just found commit d2f48f05cd2a2 ("usb: xhci: ASMedia ASM1042A
chipset need shorts TX quirk") and did the same thing for this
controller.

> Anybody from Renesas has confirmed it?
No, it's a user reported problem, please check the bug report in the link.

> Do you have an errata document to refer to?
No. Probably need Renesas guy to provide it.

>
>> [77122.622952] handle_tx_event: 541 callbacks suppressed
>>
>> No more warning messages with XHCI_TRUST_TX_LENGTH applied.
>>
>> BugLink: https://bugs.launchpad.net/bugs/1710548
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  drivers/usb/host/xhci-pci.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 8071c8fdd15e..8566b43e19ba 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -202,8 +202,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
>> xhci_hcd *xhci)
>>   xhci->quirks |= XHCI_BROKEN_STREAMS;
>>   }
>>   if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>> - pdev->device == 0x0015)
>> + pdev->device == 0x0015) {
>
> unnecessary
>
>>   xhci->quirks |= XHCI_RESET_ON_RESUME;
>> + xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>
> xhci->quirks |= XHCI_RESET_ON_RESUME |
> XHCI_TRUST_TX_LENGTH;
>
>> + }
>
> unnecessary

Do you mean that this quirk just hide the warning, it doesn't fix the
root cause?

>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

As Alan Stern points out [1], the PME signal is not enabled when
controller is in D3, therefore it's not being woken up when new deivces
get plugged in.

Workaround this bug by preventing the controller enters D3 power state.

[1] https://www.spinics.net/lists/linux-usb/msg157462.html

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/host/ehci-pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 93326974ff4b..616685f83954 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -181,6 +181,8 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
if (pdev->device == 0x7808) {
ehci->use_dummy_qh = 1;
ehci_info(ehci, "applying AMD SB700/SB800/Hudson-2/3 
EHCI dummy qh workaround\n");
+
+   pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
}
break;
case PCI_VENDOR_ID_VIA:
-- 
2.13.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Tue, Jun 20, 2017 at 2:32 AM, Alan Stern  wrote:
>
> It's possible that the test was invalid.  Kai-Heng did not say whether
> /sys/.../power/wakeup was set to "enabled" for both the EHCI controller
> and the USB root hub beneath it, before the test was started.  If
> either of them was set to "disabled" then we would not expect a plug or
> unplug event to wake up the system.

You are right, it's "disabled" on USB root hub.
Changed it to "enabled", the test result remains the same.

>
> In any case, the controller should be set to the lowest power setting
> that is consistent with the desired wakeup behavior.  If wakeup is set
> to "enabled" then the state should be D2 -- if possible.  That's the
> theory, anyway.  If the system supports putting devices only into D3
> during S3 sleep then there's no choice, but if we do have a choice then
> we should take it.
>
> BTW, I just noticed that pci_target_state() uses device_may_wakeup() to
> get the desired wakeup behavior.  That is correct for system sleep, but
> it is wrong for runtime PM.  For runtime PM, wakeup should be enabled
> whenever the hardware allows it, so the test should be
> device_can_wakeup().
>
> This means that pci_target_state() should behave differently depending
> on whether it is called from pci_prepare_to_sleep() or from
> pci_finish_runtime_suspend().  Probably nobody noticed this before
> because it usually doesn't make any difference.
>
> Alan Stern
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Runtime PM enabled EHCI does not respond to device plugging.

[Kai-Heng Feng]

On Tue, May 23, 2017 at 3:21 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Mon, 22 May 2017, Kai-Heng Feng wrote:
>
>> So in order to detect SD card under runtime PM, it's a perfectly
>> normal behavior, right?
>
> Yes.

Thanks for the info.

>
>> Sorry for not explaining the original question well enough - the real
>> problem is that after enabling runtime PM on EHCI, the two physical
>> ports on the right side of the laptop no longer response to any
>> device, e.g. an USB storage or an USB mouse.
>
> Please collect two usbmon traces: one with runtime PM disabled and one
> with runtime PM enabled.  For each trace, start with nothing plugged
> into the right-side ports.  Then after the trace has been running for a
> second or two, plug in a device.  Stop the trace a couple of seconds
> after that.

Output of `cat /sys/kernel/debug/usb/usbmon/1u`:
Runtime PM disabled as attachment.
It's empty when runtime PM is enabled.

Thanks.


rpm-disabled
Description: Binary data

Re: Runtime PM enabled EHCI does not respond to device plugging.

[Kai-Heng Feng]

On Wed, May 24, 2017 at 11:34 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Wed, 24 May 2017, Kai-Heng Feng wrote:
>
>> On Wed, May 24, 2017 at 12:43 AM, Alan Stern <st...@rowland.harvard.edu> 
>> wrote:
>> >>
>> >> Output of `cat /sys/kernel/debug/usb/usbmon/1u`:
>> >> Runtime PM disabled as attachment.
>> >
>> > When you say "runtime PM disabled", you mean that it is disabled for
>> > the EHCI controller but enabled for other devices, right?
>>
>> Yes, disabled for the ehci-hcd. Runtime PM is enabled for ehci-pci.
>>
>> >
>> >> It's empty when runtime PM is enabled.
>> >
>> > And nothing shows up in the dmesg log either?  This suggests that the
>> > PME signal isn't working properly.
>> >
>> > Try doing this: With runtime PM enabled, plug in a device.  When
>> > nothing happens, do:
>> >
>> > lspci -v -s 00:12.0
>> >
>> > The output should show whether the controller is sending a wakeup
>> > signal.  Does it cause the device to be detected?
>>
>> No.
>>
>> 00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB
>> EHCI Controller (rev 39) (prog-if 20 [EHCI])
>> Subsystem: Dell FCH USB EHCI Controller
>> Flags: 66MHz, medium devsel, IRQ 18
>> Memory at fe769000 (32-bit, non-prefetchable) [size=256]
>> Capabilities: [c0] Power Management version 2
>> Capabilities: [e4] Debug port: BAR=1 offset=00e0
>> Kernel driver in use: ehci-pci
>>
>> > If it doesn't, run lsusb.  That definitely should wake up the
>> > controller and cause the device to be detected.  Then run the lspci
>> > command again.  Let's see what the output from these commands shows.
>>
>> I plugged an optical mouse to test:
>> lsusb or "lsusb -s 001:001" does nothing, the mouse is not shown in
>> lsusb. The optical light is off.
>> "lsusb -s 001:001 -v" wakes up the controller and make the mouse to be
>> detected, the mouse is in lsusb and works correctly.
>> Once I turned on runtime PM on the mouse, it no longer works. The
>> optical light is still on but it did not response to movement or
>> click.
>>
>> This is the lspci right after I use "lsusb -s 001:001 -v" to wake up
>> the controller:
>> 00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB
>> EHCI Controller (rev 39) (prog-if 20 [EHCI])
>> Subsystem: Dell FCH USB EHCI Controller
>> Flags: bus master, 66MHz, medium devsel, latency 32, IRQ 18
>> Memory at fe769000 (32-bit, non-prefetchable) [size=256]
>> Capabilities: [c0] Power Management version 2
>> Capabilities: [e4] Debug port: BAR=1 offset=00e0
>> Kernel driver in use: ehci-pci
>
> My mistake; we need to see the information from "lspci -vv -s 00:12.0"
> with two "v"'s, not just one.

Before wakeup:
00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB
EHCI Controller (rev 39) (prog-if 20 [EHCI])
Subsystem: Dell FCH USB EHCI Controller
Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV+ VGASnoop-
ParErr- Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium
>TAbort- SERR- TAbort- SERR- 
> There's another piece of information you can collect.  Mount a
> debugfs filesystem at /sys/kernel/debug, and then copy the contents of
> the file
>
> /sys/kernel/debug/usb/ehci/:00:12.0/registers
>
> Do this while the controller is still asleep (after the mouse is
> plugged in) and then again after it is awake.

Before wakeup:
bus pci, device :00:12.0
EHCI Host Controller
SUSPENDED (no register access)

After wakeup:
bus pci, device :00:12.0
EHCI Host Controller
EHCI 1.00, rh state running
ownership 0001
SMI sts/enable 0xc008
structural params 0x0022
capability params 0xa076
status 4008 Periodic FLR
command 0010015 (park)=0 ithresh=1 Periodic period=512 RUN
intrenable 37 IAA FATAL PCD ERR INT
uframe 2d5f
port:1 status 001005 0  ACK POWER sig=se0 PE CONNECT
port:2 status 001000 0  ACK POWER sig=se0
irq normal 1855 err 10 iaa 98 (lost 0)
complete 1865 unlink 10
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Runtime PM enabled EHCI does not respond to device plugging.

[Kai-Heng Feng]

Hi,

On Sat, May 20, 2017 at 12:13 AM, Alan Stern  wrote:
[snip]
>
> Your log shows that the 2-1.1 device is a USB2.0-CRW card reader, and
> it uses the rtsx_usb driver.  This driver probes the reader every few
> seconds to see whether a card has been inserted (actually, it probes
> _twice_: once to see if an SD card is present and once to see if a
> MemoryStick card is present).  Each probe (or pair of probes) causes a
> resume followed by a suspend.
>
> The way to prevent all those resumes and suspends -- if that's what you
> want to do -- is to disable autosuspend for the card reader.  The best
> way to do this depends on your system, but
>
> echo on >/sys/bus/usb/devices/2-1.1/power/control
>
> should work.

So in order to detect SD card under runtime PM, it's a perfectly
normal behavior, right?

Sorry for not explaining the original question well enough - the real
problem is that after enabling runtime PM on EHCI, the two physical
ports on the right side of the laptop no longer response to any
device, e.g. an USB storage or an USB mouse.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Runtime PM enabled EHCI does not respond to device plugging.

[Kai-Heng Feng]

On Fri, May 26, 2017 at 1:01 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Thu, 25 May 2017, Kai-Heng Feng wrote:
>
>> > My mistake; we need to see the information from "lspci -vv -s 00:12.0"
>> > with two "v"'s, not just one.
>>
>> Before wakeup:
>> 00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB
>> EHCI Controller (rev 39) (prog-if 20 [EHCI])
>> Subsystem: Dell FCH USB EHCI Controller
>> Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV+ VGASnoop-
>> ParErr- Stepping- SERR- FastB2B- DisINTx-
>> Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium
>> >TAbort- SERR- > Interrupt: pin A routed to IRQ 18
>> Region 0: Memory at fe769000 (32-bit, non-prefetchable) [size=256]
>> Capabilities: [c0] Power Management version 2
>> Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA
>> PME(D0+,D1+,D2+,D3hot+,D3cold+)
>> Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>> Bridge: PM- B3+
>> Capabilities: [e4] Debug port: BAR=1 offset=00e0
>> Kernel driver in use: ehci-pci
>
> Was this before you plugged in the mouse or after?

After.

>
> If it was after, it means there is a bug in the EHCI controller
> hardware.  This line:
>
>> Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>
> says that the PME signal is not enabled, so the controller is not
> sending a wakeup request.  But when a new device gets plugged in, the
> controller is supposed to ask to be woken up.

Hmm, sounds bad.
Is there any known workaround?
Maybe wake up the host every two seconds?
Or use a quirk to disable runtime PM for this host?

>
>> After wakeup:
>> 00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB
>> EHCI Controller (rev 39) (prog-if 20 [EHCI])
>> Subsystem: Dell FCH USB EHCI Controller
>> Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV+ VGASnoop-
>> ParErr- Stepping- SERR- FastB2B- DisINTx-
>> Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium
>> >TAbort- SERR- > Latency: 32, Cache Line Size: 64 bytes
>> Interrupt: pin A routed to IRQ 18
>> Region 0: Memory at fe769000 (32-bit, non-prefetchable) [size=256]
>> Capabilities: [c0] Power Management version 2
>> Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA
>> PME(D0+,D1+,D2+,D3hot+,D3cold+)
>> Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
>> Bridge: PM- B3+
>> Capabilities: [e4] Debug port: BAR=1 offset=00e0
>> Kernel driver in use: ehci-pci
>
> Yes, that is normal.
>
>> > There's another piece of information you can collect.  Mount a
>> > debugfs filesystem at /sys/kernel/debug, and then copy the contents of
>> > the file
>> >
>> > /sys/kernel/debug/usb/ehci/:00:12.0/registers
>> >
>> > Do this while the controller is still asleep (after the mouse is
>> > plugged in) and then again after it is awake.
>>
>> Before wakeup:
>> bus pci, device :00:12.0
>> EHCI Host Controller
>> SUSPENDED (no register access)
>>
>> After wakeup:
>> bus pci, device :00:12.0
>> EHCI Host Controller
>> EHCI 1.00, rh state running
>> ownership 0001
>> SMI sts/enable 0xc008
>> structural params 0x0022
>> capability params 0xa076
>> status 4008 Periodic FLR
>> command 0010015 (park)=0 ithresh=1 Periodic period=512 RUN
>> intrenable 37 IAA FATAL PCD ERR INT
>> uframe 2d5f
>> port:1 status 001005 0  ACK POWER sig=se0 PE CONNECT
>> port:2 status 001000 0  ACK POWER sig=se0
>> irq normal 1855 err 10 iaa 98 (lost 0)
>> complete 1865 unlink 10
>
> Again, this is normal.
>
> Alan Stern
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Runtime PM enabled EHCI does not respond to device plugging.

[Kai-Heng Feng]

Hi,

This issue happens on Carrizo AMD laptops, only EHCI is affected, XHCI
works fine on the same machine.

I can see lots of USB wakeup and resume messages showed every two seconds.
Plug USB devices to the EHCI port does not change anything.

dmesg with ehci-hcd, ehci-pci and usbcore dynamic debug enabled is attached:

[   88.556535] ehci-pci :00:13.0: hcd_pci_runtime_resume: 0
[   88.556563] usb usb2: usb auto-resume
[   88.556588] hub 2-0:1.0: hub_resume
[   88.556676] usb usb2-port1: status 0507 change 
[   88.556794] hub 2-0:1.0: state 7 ports 2 chg  evt 
[   88.556862] usb 2-1: usb auto-resume
[   88.624577] usb 2-1: Waited 0ms for CONNECT
[   88.624585] usb 2-1: finish resume
[   88.624838] hub 2-1:1.0: hub_resume
[   88.624964] usb 2-1-port1: status 0507 change 
[   88.625085] usb 2-1-port2: status 0107 change 
[   88.625354] usb 2-1-port4: status 0507 change 
[   88.625403] hub 2-1:1.0: state 7 ports 4 chg  evt 
[   88.625605] usb 2-1.1: usb auto-resume
[   88.692635] usb 2-1.1: Waited 0ms for CONNECT
[   88.692640] usb 2-1.1: finish resume
[   90.800874] usb 2-1.1: usb auto-suspend, wakeup 1
[   90.820326] hub 2-1:1.0: hub_suspend
[   90.823161] usb 2-1: usb auto-suspend, wakeup 1
[   90.840362] hub 2-0:1.0: hub_suspend
[   90.840399] usb usb2: bus auto-suspend, wakeup 1
[   90.860346] ehci-pci :00:13.0: hcd_pci_runtime_suspend: 0

Thanks.


dmesg
Description: Binary data

Re: Runtime PM enabled EHCI does not respond to device plugging.

[Kai-Heng Feng]

On Wed, May 24, 2017 at 12:43 AM, Alan Stern  wrote:
>>
>> Output of `cat /sys/kernel/debug/usb/usbmon/1u`:
>> Runtime PM disabled as attachment.
>
> When you say "runtime PM disabled", you mean that it is disabled for
> the EHCI controller but enabled for other devices, right?

Yes, disabled for the ehci-hcd. Runtime PM is enabled for ehci-pci.

>
>> It's empty when runtime PM is enabled.
>
> And nothing shows up in the dmesg log either?  This suggests that the
> PME signal isn't working properly.
>
> Try doing this: With runtime PM enabled, plug in a device.  When
> nothing happens, do:
>
> lspci -v -s 00:12.0
>
> The output should show whether the controller is sending a wakeup
> signal.  Does it cause the device to be detected?

No.

00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB
EHCI Controller (rev 39) (prog-if 20 [EHCI])
Subsystem: Dell FCH USB EHCI Controller
Flags: 66MHz, medium devsel, IRQ 18
Memory at fe769000 (32-bit, non-prefetchable) [size=256]
Capabilities: [c0] Power Management version 2
Capabilities: [e4] Debug port: BAR=1 offset=00e0
Kernel driver in use: ehci-pci

> If it doesn't, run lsusb.  That definitely should wake up the
> controller and cause the device to be detected.  Then run the lspci
> command again.  Let's see what the output from these commands shows.

I plugged an optical mouse to test:
lsusb or "lsusb -s 001:001" does nothing, the mouse is not shown in
lsusb. The optical light is off.
"lsusb -s 001:001 -v" wakes up the controller and make the mouse to be
detected, the mouse is in lsusb and works correctly.
Once I turned on runtime PM on the mouse, it no longer works. The
optical light is still on but it did not response to movement or
click.

This is the lspci right after I use "lsusb -s 001:001 -v" to wake up
the controller:
00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB
EHCI Controller (rev 39) (prog-if 20 [EHCI])
Subsystem: Dell FCH USB EHCI Controller
Flags: bus master, 66MHz, medium devsel, latency 32, IRQ 18
Memory at fe769000 (32-bit, non-prefetchable) [size=256]
Capabilities: [c0] Power Management version 2
Capabilities: [e4] Debug port: BAR=1 offset=00e0
Kernel driver in use: ehci-pci
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas  wrote:
>
> The lspci output [1] shows:
>
>   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI 
> Controller (rev 39) (prog-if 20 [EHCI])
> Capabilities: [c0] Power Management version 2
>   Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA 
> PME(D0+,D1+,D2+,D3hot+,D3cold+)
>   Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>   Bridge: PM- B3+
>
> The device claims it can assert PME# from D3hot.  If it can't, that
> sounds like a hardware defect that should be addressed with a quirk.
> Ideally we would also have a pointer to the AMD hardware erratum.

Looks like it's pretty similar to "23 USB Wake on Connect/Disconnect
with Low Speed Devices" in [1].
It points to a workaround in appendix A2 from [2].
However it says this bug only effects Low Speed devices, but it
actually also happens on High Speed devices.

[1] https://support.amd.com/TechDocs/46837.pdf
[2] https://support.amd.com/TechDocs/42413.pdf

>
> Is the following path involved here?
>
>   pci_finish_runtime_suspend
> target_state = pci_target_state()
>   if (device_may_wakup())
> if (dev->pme_support)
>   ...
> pci_set_power_state(..., target_state)

Yes, it's involved.

>
> If so, I would naively expect that a quirk could clear the
> PCI_PM_CAP_PME_D3 and PCI_PM_CAP_PME_D3cold bits in dev->pme_support,
> and pci_target_state() would then avoid selecting D3 or D3cold.  But
> I'm not an expert in power management.

Clearing those two bits does the trick, thanks for the tip.

>
> Bjorn
>
> [1] http://marc.info/?l=linux-usb=149570231732519=2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Thu, Jun 15, 2017 at 2:55 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Tue, 13 Jun 2017, Bjorn Helgaas wrote:
>
>> [+cc Rafael, linux-pm]
>>
>> On Tue, Jun 13, 2017 at 12:21:15PM +0800, Kai-Heng Feng wrote:
>> > On Mon, Jun 12, 2017 at 10:18 PM, Alan Stern <st...@rowland.harvard.edu> 
>> > wrote:
>> > > Let's get some help from people who understand PCI well.
>> > >
>> > > Here's the general problem: Kai-Heng has a PCI-based USB host
>> > > controller that advertises wakeup capability from D3, but it doesn't
>> > > assert PME# from D3 when it should.  For "lspci -vv" output, see
>> > >
>> > > http://marc.info/?l=linux-usb=149570231732519=2
>> > >
>> > > On Mon, 12 Jun 2017, Kai-Heng Feng wrote:
>> > >
>> > >> On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
>> > >> <kai.heng.f...@canonical.com> wrote:
>> > >> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern 
>> > >> > <st...@rowland.harvard.edu> wrote:
>> > >> >> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>> > >> >>
>> > >> >> Is this really the right solution?  Maybe it would be better to allow
>> > >> >> the controller to go into D3 provided no wakeup signal is needed.  
>> > >> >> You
>> > >> >> could do:
>> > >> >>
>> > >> >> device_set_wakeup_capable(>dev, 0);
>> > >> >
>> > >> > This doesn't work.
>> > >> > After applying this function, still nothing happens when devices get 
>> > >> > plugged in.
>> > >> > IIUC this function disable the wakeup function, but what I want to do
>> > >> > here is to have PME signal works even when runtime PM is enabled.
>> > >
>> > > This may indicate a bug in either the PCI or USB stacks (or both!).  If
>> > > a driver requires wakeup capability from runtime suspend but the device
>> > > does not provide it, the PCI core should not allow the device to go
>> > > into runtime suspend.  Or is that the driver's responsibility?
>> > >
>> > >> > I also saw some legacy PCI PM stuff, so I also tried:
>> > >> > device_set_wakeup_capable(>dev, 1);
>> > >> > ...doesn't work either.
>> > >> >
>> > >> >>
>> > >> >> Another alternative is to put the controller into D2 instead of D3, 
>> > >> >> but
>> > >> >> (1) I don't know how to do that, and (2) we don't know if wakeup
>> > >> >> signalling works any better in D2 than it does in D3.
>> > >> >
>> > >> > I'll try if D2 works.
>> > >>
>> > >> Put the device into D2 instead of D3 can make the wakeup signaling
>> > >> work, i.e. USB devices can be correctly detected after plugged into
>> > >> EHCI port.
>> > >>
>> > >> Do you think this alternative an acceptable workaround?
>> > >
>> > > Yes, it is.  The difficulty is that I don't know how to tell the PCI
>> > > core that the device should go in D2 during runtime suspend instead of
>> > > D3.  Some sort of quirk may be needed -- perhaps Bjorn can help.
>
>> The lspci output [1] shows:
>>
>>   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI 
>> Controller (rev 39) (prog-if 20 [EHCI])
>> Capabilities: [c0] Power Management version 2
>>   Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA 
>> PME(D0+,D1+,D2+,D3hot+,D3cold+)
>>   Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>>   Bridge: PM- B3+
>>
>> The device claims it can assert PME# from D3hot.  If it can't, that
>> sounds like a hardware defect that should be addressed with a quirk.
>> Ideally we would also have a pointer to the AMD hardware erratum.
>>
>> Is the following path involved here?
>>
>>   pci_finish_runtime_suspend
>> target_state = pci_target_state()
>>   if (device_may_wakup())
>>   if (dev->pme_support)
>> ...
>> pci_set_power_state(..., target_state)
>>
>> If so, I would naively expect that a quirk could clear the
>> PCI_PM_CAP_PME_D3 and PCI_PM_CAP_PME_D3cold bits in dev->pme_support,
>> and pci_target_state() would then avoid selecting D3 or D3cold.

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>
>> As Alan Stern points out [1], the PME signal is not enabled when
>> controller is in D3, therefore it's not being woken up when new deivces
>> get plugged in.
>>
>> Workaround this bug by preventing the controller enters D3 power state.
>>
>> [1] https://www.spinics.net/lists/linux-usb/msg157462.html
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  drivers/usb/host/ehci-pci.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
>> index 93326974ff4b..616685f83954 100644
>> --- a/drivers/usb/host/ehci-pci.c
>> +++ b/drivers/usb/host/ehci-pci.c
>> @@ -181,6 +181,8 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>>   if (pdev->device == 0x7808) {
>>   ehci->use_dummy_qh = 1;
>>   ehci_info(ehci, "applying AMD SB700/SB800/Hudson-2/3 
>> EHCI dummy qh workaround\n");
>> +
>> + pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
>>   }
>>   break;
>>   case PCI_VENDOR_ID_VIA:
>
> Is this really the right solution?  Maybe it would be better to allow
> the controller to go into D3 provided no wakeup signal is needed.  You
> could do:
>
> device_set_wakeup_capable(>dev, 0);

This doesn't work.
After applying this function, still nothing happens when devices get plugged in.
IIUC this function disable the wakeup function, but what I want to do
here is to have PME signal works even when runtime PM is enabled.

I also saw some legacy PCI PM stuff, so I also tried:
device_set_wakeup_capable(>dev, 1);
...doesn't work either.

>
> Another alternative is to put the controller into D2 instead of D3, but
> (1) I don't know how to do that, and (2) we don't know if wakeup
> signalling works any better in D2 than it does in D3.

I'll try if D2 works.

Thanks for the review.

>
> Alan Stern
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Thu, 15 Jun 2017, Kai-Heng Feng wrote:
>
>> On Wed, Jun 14, 2017 at 1:28 AM, Bjorn Helgaas <helg...@kernel.org> wrote:
>> >
>> > The lspci output [1] shows:
>> >
>> >   00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB EHCI 
>> > Controller (rev 39) (prog-if 20 [EHCI])
>> > Capabilities: [c0] Power Management version 2
>> >   Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA 
>> > PME(D0+,D1+,D2+,D3hot+,D3cold+)
>> >   Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
>> >   Bridge: PM- B3+
>> >
>> > The device claims it can assert PME# from D3hot.  If it can't, that
>> > sounds like a hardware defect that should be addressed with a quirk.
>> > Ideally we would also have a pointer to the AMD hardware erratum.
>>
>> Looks like it's pretty similar to "23 USB Wake on Connect/Disconnect
>> with Low Speed Devices" in [1].
>> It points to a workaround in appendix A2 from [2].
>> However it says this bug only effects Low Speed devices, but it
>> actually also happens on High Speed devices.
>>
>> [1] https://support.amd.com/TechDocs/46837.pdf
>> [2] https://support.amd.com/TechDocs/42413.pdf
>
> Those documents refer to a hardware bug with a workaround in the BIOS.
> Have you checked to see if your BIOS is up to date?

Yes, it's up to date.

>
> Alan Stern
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <st...@rowland.harvard.edu> 
> wrote:
>> Those documents refer to a hardware bug with a workaround in the BIOS.
>> Have you checked to see if your BIOS is up to date?
>
> Yes, it's up to date.
>

Alan, I re-sent a patch but I forgot to add you to CC list:
http://marc.info/?l=linux-pci=149760607914628=2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Sat, Jun 17, 2017 at 1:30 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Sat, 17 Jun 2017, Kai-Heng Feng wrote:
>
>> On Fri, Jun 16, 2017 at 11:07 AM, Kai-Heng Feng
>> <kai.heng.f...@canonical.com> wrote:
>> > On Thu, Jun 15, 2017 at 10:12 PM, Alan Stern <st...@rowland.harvard.edu> 
>> > wrote:
>> >> Those documents refer to a hardware bug with a workaround in the BIOS.
>> >> Have you checked to see if your BIOS is up to date?
>> >
>> > Yes, it's up to date.
>> >
>>
>> Alan, I re-sent a patch but I forgot to add you to CC list:
>> http://marc.info/?l=linux-pci=149760607914628=2
>
> Thanks for letting me know.  The patch seems reasonable.
>
> Have you tested it with system suspend?  That is, if you suspend the
> whole computer, does plugging or unplugging a USB device cause the
> system to wake up?

No, the system will not wake up when plugging or unplugging.
Tried several times, nether runtime PM enabled nor runtime PM disabled
will wake up the system under S3, when (un)plugging USB devices.

>
> Alan Stern
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Mon, Jun 12, 2017 at 10:18 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> Let's get some help from people who understand PCI well.
>
> Here's the general problem: Kai-Heng has a PCI-based USB host
> controller that advertises wakeup capability from D3, but it doesn't
> assert PME# from D3 when it should.  For "lspci -vv" output, see
>
> http://marc.info/?l=linux-usb=149570231732519=2
>
> On Mon, 12 Jun 2017, Kai-Heng Feng wrote:
>
>> On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
>> <kai.heng.f...@canonical.com> wrote:
>> > On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <st...@rowland.harvard.edu> 
>> > wrote:
>> >> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>> >>
>> >> Is this really the right solution?  Maybe it would be better to allow
>> >> the controller to go into D3 provided no wakeup signal is needed.  You
>> >> could do:
>> >>
>> >> device_set_wakeup_capable(>dev, 0);
>> >
>> > This doesn't work.
>> > After applying this function, still nothing happens when devices get 
>> > plugged in.
>> > IIUC this function disable the wakeup function, but what I want to do
>> > here is to have PME signal works even when runtime PM is enabled.
>
> This may indicate a bug in either the PCI or USB stacks (or both!).  If
> a driver requires wakeup capability from runtime suspend but the device
> does not provide it, the PCI core should not allow the device to go
> into runtime suspend.  Or is that the driver's responsibility?
>
>> > I also saw some legacy PCI PM stuff, so I also tried:
>> > device_set_wakeup_capable(>dev, 1);
>> > ...doesn't work either.
>> >
>> >>
>> >> Another alternative is to put the controller into D2 instead of D3, but
>> >> (1) I don't know how to do that, and (2) we don't know if wakeup
>> >> signalling works any better in D2 than it does in D3.
>> >
>> > I'll try if D2 works.
>>
>> Put the device into D2 instead of D3 can make the wakeup signaling
>> work, i.e. USB devices can be correctly detected after plugged into
>> EHCI port.
>>
>> Do you think this alternative an acceptable workaround?
>
> Yes, it is.  The difficulty is that I don't know how to tell the PCI
> core that the device should go in D2 during runtime suspend instead of
> D3.  Some sort of quirk may be needed -- perhaps Bjorn can help.
>

FWIW, this is the diff I used to make the controller transits to D2
instead of D3:

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 563901cd9c06..36663688404a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -922,6 +922,9 @@ int pci_set_power_state(struct pci_dev *dev,
pci_power_t state)
if (dev->current_state == state)
return 0;

+   if (state > PCI_D2 && (dev->dev_flags & PCI_DEV_FLAGS_MAX_D2))
+   state = PCI_D2;
+
__pci_start_power_transition(dev, state);

/* This device is quirked not to be put into D3, so
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 93326974ff4b..a2c1fe62974a 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -181,6 +181,8 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
if (pdev->device == 0x7808) {
ehci->use_dummy_qh = 1;
ehci_info(ehci, "applying AMD
SB700/SB800/Hudson-2/3 EHCI dummy qh workaround\n");
+
+   pdev->dev_flags |= PCI_DEV_FLAGS_MAX_D2;
}
break;
case PCI_VENDOR_ID_VIA:
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8039f9f0ca05..6f86f2aa8548 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -188,6 +188,7 @@ enum pci_dev_flags {
 * the direct_complete optimization.
 */
PCI_DEV_FLAGS_NEEDS_RESUME = (__force pci_dev_flags_t) (1 << 11),
+   PCI_DEV_FLAGS_MAX_D2 = (__force pci_dev_flags_t) (1 << 12),
 };

 enum pci_irq_reroute_variant {
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: host: ehci: workaround PME bug on AMD EHCI controller

[Kai-Heng Feng]

On Mon, Jun 12, 2017 at 3:04 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Fri, Jun 9, 2017 at 10:43 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
>> On Fri, 9 Jun 2017, Kai-Heng Feng wrote:
>>
>> Is this really the right solution?  Maybe it would be better to allow
>> the controller to go into D3 provided no wakeup signal is needed.  You
>> could do:
>>
>> device_set_wakeup_capable(>dev, 0);
>
> This doesn't work.
> After applying this function, still nothing happens when devices get plugged 
> in.
> IIUC this function disable the wakeup function, but what I want to do
> here is to have PME signal works even when runtime PM is enabled.
>
> I also saw some legacy PCI PM stuff, so I also tried:
> device_set_wakeup_capable(>dev, 1);
> ...doesn't work either.
>
>>
>> Another alternative is to put the controller into D2 instead of D3, but
>> (1) I don't know how to do that, and (2) we don't know if wakeup
>> signalling works any better in D2 than it does in D3.
>
> I'll try if D2 works.

Put the device into D2 instead of D3 can make the wakeup signaling
work, i.e. USB devices can be correctly detected after plugged into
EHCI port.

Do you think this alternative an acceptable workaround?

>
> Thanks for the review.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "xhci: Limit USB2 port wake support for AMD Promontory hosts"

[Kai-Heng Feng]

On Mon, Aug 28, 2017 at 9:56 PM, Kai-Heng Feng
<kai.heng.f...@canonical.com> wrote:
> On Mon, Aug 28, 2017 at 6:14 PM, Mathias Nyman
> <mathias.ny...@linux.intel.com> wrote:
>> On 28.08.2017 12:29, Greg KH wrote:
>>
>> Adding more people who were involved in the original patch.
>>
>> Users are now seeing the unresponsive USB2 ports with Promontory hosts.
>> Is there any update on a better way to solve the original issue.
>>
>> To me a "dead" USB2 port seems like a much worse issue for a user
>> than a BIOS disabled port waking up on plug/unplug (wake on
>> connect/disconnect),
>> so I'm myself in favor of doing this revert.
>
> At least I can't find "Disable USB2" on my ASUS PRIME B350M-A, so the
> new behavior is quite surprising.
>
>>
>> But there was a strong push from Promontory developers to get the original
>> fix in,
>> and I would like to get some comment from them before I do anything about
>> it.
>
> You looped them to the mail thread which I reported the regression two
> weeks ago, and there is no response since then...

Still no response from AMD and ASMedia guys.

I don't like to use out-of-tree patches for myself, but probably it's
the only way here?

>
>>
>> Thanks
>> -Mathias
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] xhci : AMD Promontory USB disable port support

[Kai-Heng Feng]

Hi Mathias,

On Mon, Sep 25, 2017 at 4:16 PM, Mathias Nyman
<mathias.ny...@linux.intel.com> wrote:
> On 25.09.2017 10:08, Joe Lee wrote:
>>
>> From: Joe Lee <asmt.sw...@gmail.com>
>>
>> For AMD Promontory xHCI host,
>> although you can disable USB 2.0 ports in BIOSsettings,
>> those ports will be enabled anyway after you remove a device on
>> that port and re-plug it in again. It's a known limitation of the chip.
>> As a workaround we can clear the PORT_WAKE_BITS.
>>
>> Signed-off-by: Joe Lee <asmt.sw...@gmail.com>
>>
>> ---
>
>
> Kai-Heng Feng,
> Do you have the time to check if this works on your Promontory setup?
>
> Does the usb2 ports stay alive with runtime PM enabled after this patch?

Yes. I think the patch is good to go.

Thanks!

>
> Thanks
> -Mathias
>
>
>> v6: Fix coding format.
>> v5: Add check disable port setting before set PORT_WAKE_BITS
>> v4: Remove the patch code in case USB_PORT_FEAT_REMOTE_WAKE_MASK
>> v3: Fix some checkpatch.pl
>> ---
>>   drivers/usb/host/pci-quirks.c | 116
>> +-
>>   drivers/usb/host/pci-quirks.h |   1 +
>>   drivers/usb/host/xhci-hub.c   |   7 ++-
>>   3 files changed, 121 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
>> index 658d9d1..934220c 100644
>> --- a/drivers/usb/host/pci-quirks.c
>> +++ b/drivers/usb/host/pci-quirks.c
>> @@ -64,7 +64,22 @@
>>   #define   AB_DATA(addr)   ((addr) + 0x04)
>>   #define   AX_INDXC0x30
>>   #define   AX_DATAC0x34
>> -
>> +#define PT_ADDR_INDEX  0xE8
>> +#define PT_READ_INDEX  0xE4
>> +#define PT_SIG_1_ADDR  0xA520
>> +#define PT_SIG_2_ADDR  0xA521
>> +#define PT_SIG_3_ADDR  0xA522
>> +#define PT_SIG_4_ADDR  0xA523
>> +#define PT_SIG_1_DATA  0x78
>> +#define PT_SIG_2_DATA  0x56
>> +#define PT_SIG_3_DATA  0x34
>> +#define PT_SIG_4_DATA  0x12
>> +#define PT_4_PORT_1_REG0xB521
>> +#define PT_4_PORT_2_REG0xB522
>> +#define PT_2_PORT_1_REG0xD520
>> +#define PT_2_PORT_2_REG0xD521
>> +#define PT_1_PORT_1_REG0xD522
>> +#define PT_1_PORT_2_REG0xD523
>>   #define   NB_PCIE_INDX_ADDR   0xe0
>>   #define   NB_PCIE_INDX_DATA   0xe4
>>   #define   PCIE_P_CNTL 0x10040
>> @@ -511,6 +526,105 @@ void usb_amd_dev_put(void)
>>   }
>>   EXPORT_SYMBOL_GPL(usb_amd_dev_put);
>>
>> +int usb_amd_pt_check_port(struct device *device, int port)
>> +{
>> +   unsigned char value;
>> +   struct pci_dev *pdev;
>> +
>> +   pdev = to_pci_dev(device);
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_1_ADDR);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value != PT_SIG_1_DATA)
>> +   return 0;
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_2_ADDR);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value != PT_SIG_2_DATA)
>> +   return 0;
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_3_ADDR);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value != PT_SIG_3_DATA)
>> +   return 0;
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_4_ADDR);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value != PT_SIG_4_DATA)
>> +   return 0;
>> +   if ((pdev->device == 0x43b9) || (pdev->device == 0x43ba)) {
>> +   /* device is AMD_PROMONTORYA_4(0x43b9) or
>> +* PROMONTORYA_3(0x43ba)
>> +* disable port setting PT_4_PORT_1_REG[7..1] is
>> +* USB2.0 port6 to 0
>> +* PT_4_PORT_2_REG[6..0] is USB 13 to port 7
>> +* 0 : disable ;1 : enable
>> +*/
>> +   if (port > 6) {
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX,
>> +   PT_4_PORT_2_REG);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value & (1<<(port - 7)))
>> +   return 0;
>> +   else
>> +   return 1;
>> + 

Re: [PATCH v6] xhci : AMD Promontory USB disable port support

[Kai-Heng Feng]

On Mon, Sep 25, 2017 at 4:16 PM, Mathias Nyman
<mathias.ny...@linux.intel.com> wrote:
> On 25.09.2017 10:08, Joe Lee wrote:
>>
>> From: Joe Lee <asmt.sw...@gmail.com>
>>
>> For AMD Promontory xHCI host,
>> although you can disable USB 2.0 ports in BIOSsettings,
>> those ports will be enabled anyway after you remove a device on
>> that port and re-plug it in again. It's a known limitation of the chip.
>> As a workaround we can clear the PORT_WAKE_BITS.
>>
>> Signed-off-by: Joe Lee <asmt.sw...@gmail.com>
>>
>> ---

Hi Mathias,

>
>
> Kai-Heng Feng,
> Do you have the time to check if this works on your Promontory setup?

Yes.
But I just found a regression: the port doesn't suspend now, as of v4.13.
The runtime_suspended_time for the port remains a stale value.
I'll need time to bisect the regression. I'll test the patch after that.

>
> Does the usb2 ports stay alive with runtime PM enabled after this patch?
>
> Thanks
> -Mathias
>
>
>> v6: Fix coding format.
>> v5: Add check disable port setting before set PORT_WAKE_BITS
>> v4: Remove the patch code in case USB_PORT_FEAT_REMOTE_WAKE_MASK
>> v3: Fix some checkpatch.pl
>> ---
>>   drivers/usb/host/pci-quirks.c | 116
>> +-
>>   drivers/usb/host/pci-quirks.h |   1 +
>>   drivers/usb/host/xhci-hub.c   |   7 ++-
>>   3 files changed, 121 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
>> index 658d9d1..934220c 100644
>> --- a/drivers/usb/host/pci-quirks.c
>> +++ b/drivers/usb/host/pci-quirks.c
>> @@ -64,7 +64,22 @@
>>   #define   AB_DATA(addr)   ((addr) + 0x04)
>>   #define   AX_INDXC0x30
>>   #define   AX_DATAC0x34
>> -
>> +#define PT_ADDR_INDEX  0xE8
>> +#define PT_READ_INDEX  0xE4
>> +#define PT_SIG_1_ADDR  0xA520
>> +#define PT_SIG_2_ADDR  0xA521
>> +#define PT_SIG_3_ADDR  0xA522
>> +#define PT_SIG_4_ADDR  0xA523
>> +#define PT_SIG_1_DATA  0x78
>> +#define PT_SIG_2_DATA  0x56
>> +#define PT_SIG_3_DATA  0x34
>> +#define PT_SIG_4_DATA  0x12
>> +#define PT_4_PORT_1_REG0xB521
>> +#define PT_4_PORT_2_REG0xB522
>> +#define PT_2_PORT_1_REG0xD520
>> +#define PT_2_PORT_2_REG0xD521
>> +#define PT_1_PORT_1_REG0xD522
>> +#define PT_1_PORT_2_REG0xD523
>>   #define   NB_PCIE_INDX_ADDR   0xe0
>>   #define   NB_PCIE_INDX_DATA   0xe4
>>   #define   PCIE_P_CNTL 0x10040
>> @@ -511,6 +526,105 @@ void usb_amd_dev_put(void)
>>   }
>>   EXPORT_SYMBOL_GPL(usb_amd_dev_put);
>>
>> +int usb_amd_pt_check_port(struct device *device, int port)
>> +{
>> +   unsigned char value;
>> +   struct pci_dev *pdev;
>> +
>> +   pdev = to_pci_dev(device);
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_1_ADDR);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value != PT_SIG_1_DATA)
>> +   return 0;
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_2_ADDR);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value != PT_SIG_2_DATA)
>> +   return 0;
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_3_ADDR);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value != PT_SIG_3_DATA)
>> +   return 0;
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_4_ADDR);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value != PT_SIG_4_DATA)
>> +   return 0;
>> +   if ((pdev->device == 0x43b9) || (pdev->device == 0x43ba)) {
>> +   /* device is AMD_PROMONTORYA_4(0x43b9) or
>> +* PROMONTORYA_3(0x43ba)
>> +* disable port setting PT_4_PORT_1_REG[7..1] is
>> +* USB2.0 port6 to 0
>> +* PT_4_PORT_2_REG[6..0] is USB 13 to port 7
>> +* 0 : disable ;1 : enable
>> +*/
>> +   if (port > 6) {
>> +   pci_write_config_word(pdev, PT_ADDR_INDEX,
>> +   PT_4_PORT_2_REG);
>> +   pci_read_config_byte(pdev, PT_READ_INDEX, );
>> +   if (value & (1<<(port - 7)))

[PATCH] Revert "xhci: Limit USB2 port wake support for AMD Promontory hosts"

[Kai-Heng Feng]

This reverts commit dec08194ffeccfa1cf085906b53d301930eae18f.

Commit dec08194ffec ("xhci: Limit USB2 port wake support for AMD Promontory
hosts") makes all high speed USB ports on ASUS PRIME B350M-A cease to
function after enabling runtime PM.

All boards with this chipsets will be affected, so revert the commit.

Conflicts:
drivers/usb/host/xhci-pci.c
drivers/usb/host/xhci.h

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/host/xhci-hub.c |  3 ---
 drivers/usb/host/xhci-pci.c | 12 
 drivers/usb/host/xhci.h |  2 +-
 3 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 00721e8807ab..4bd8654b1233 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1473,9 +1473,6 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
t2 |= PORT_WKOC_E | PORT_WKCONN_E;
t2 &= ~PORT_WKDISC_E;
}
-   if ((xhci->quirks & XHCI_U2_DISABLE_WAKE) &&
-   (hcd->speed < HCD_USB3))
-   t2 &= ~PORT_WAKE_BITS;
} else
t2 &= ~PORT_WAKE_BITS;
 
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 8071c8fdd15e..76f392954733 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -54,11 +54,6 @@
 #define PCI_DEVICE_ID_INTEL_APL_XHCI   0x5aa8
 #define PCI_DEVICE_ID_INTEL_DNV_XHCI   0x19d0
 
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_40x43b9
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_30x43ba
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_20x43bb
-#define PCI_DEVICE_ID_AMD_PROMONTORYA_10x43bc
-
 #define PCI_DEVICE_ID_ASMEDIA_1042A_XHCI   0x1142
 
 static const char hcd_name[] = "xhci_hcd";
@@ -142,13 +137,6 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
if (pdev->vendor == PCI_VENDOR_ID_AMD)
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 
-   if ((pdev->vendor == PCI_VENDOR_ID_AMD) &&
-   ((pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_4) ||
-   (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_3) ||
-   (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_2) ||
-   (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
-   xhci->quirks |= XHCI_U2_DISABLE_WAKE;
-
if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
xhci->quirks |= XHCI_LPM_SUPPORT;
xhci->quirks |= XHCI_INTEL_HOST;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e3e935291ed6..d6b471823a0b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1819,7 +1819,7 @@ struct xhci_hcd {
 /* For controller with a broken Port Disable implementation */
 #define XHCI_BROKEN_PORT_PED   (1 << 25)
 #define XHCI_LIMIT_ENDPOINT_INTERVAL_7 (1 << 26)
-#define XHCI_U2_DISABLE_WAKE   (1 << 27)
+/* Reserved. It was XHCI_U2_DISABLE_WAKE */
 #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL(1 << 28)
 
unsigned intnum_active_eps;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Add device quirk for Logitech HD Pro Webcam C920-C

[Kai-Heng Feng]

On Mon, Aug 21, 2017 at 6:03 PM, Dmitry Fleytman  wrote:
> Commit e0429362ab15
> ("usb: Add device quirk for Logitech HD Pro Webcams C920 and C930e")
> introduced quirk to workaround an issue with some Logitech webcams.
>
> Apparently model C920-C has the same issue so applying
> the same quirk as well.

I think it's better to also mention "C920-C" in the comment section.

>
> See aforementioned commit message for detailed explanation of the problem.
>
> Signed-off-by: Dmitry Fleytman 
> ---
>  drivers/usb/core/quirks.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 574da2b..36d4841 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -59,6 +59,7 @@ static const struct usb_device_id usb_quirk_list[] = {
>
> /* Logitech HD Pro Webcams C920 and C930e */
> { USB_DEVICE(0x046d, 0x082d), .driver_info = USB_QUIRK_DELAY_INIT },
> +   { USB_DEVICE(0x046d, 0x0841), .driver_info = USB_QUIRK_DELAY_INIT },
> { USB_DEVICE(0x046d, 0x0843), .driver_info = USB_QUIRK_DELAY_INIT },
>
> /* Logitech ConferenceCam CC3000e */
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "xhci: Limit USB2 port wake support for AMD Promontory hosts"

[Kai-Heng Feng]

On Mon, Aug 28, 2017 at 6:14 PM, Mathias Nyman
<mathias.ny...@linux.intel.com> wrote:
> On 28.08.2017 12:29, Greg KH wrote:
>>
>> On Tue, Aug 22, 2017 at 05:14:47PM +0800, Kai-Heng Feng wrote:
>>>
>>> This reverts commit dec08194ffeccfa1cf085906b53d301930eae18f.
>>>
>>> Commit dec08194ffec ("xhci: Limit USB2 port wake support for AMD
>>> Promontory
>>> hosts") makes all high speed USB ports on ASUS PRIME B350M-A cease to
>>> function after enabling runtime PM.
>>>
>>> All boards with this chipsets will be affected, so revert the commit.
>>>
>>> Conflicts:
>>> drivers/usb/host/xhci-pci.c
>>> drivers/usb/host/xhci.h
>>
>>
>> Why are these "Conflicts:" lines here, you did fix up the issues, so
>> there shouldn't be any more conflicts.
>>
>> And if you revert this, don't we still have the original problem here?
>>
>
> Adding more people who were involved in the original patch.
>
> Users are now seeing the unresponsive USB2 ports with Promontory hosts.
> Is there any update on a better way to solve the original issue.
>
> To me a "dead" USB2 port seems like a much worse issue for a user
> than a BIOS disabled port waking up on plug/unplug (wake on
> connect/disconnect),
> so I'm myself in favor of doing this revert.

At least I can't find "Disable USB2" on my ASUS PRIME B350M-A, so the
new behavior is quite surprising.

>
> But there was a strong push from Promontory developers to get the original
> fix in,
> and I would like to get some comment from them before I do anything about
> it.

You looped them to the mail thread which I reported the regression two
weeks ago, and there is no response since then...

>
> Thanks
> -Mathias
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> On 27 Nov 2017, at 11:13 PM,  
>  wrote:
> 
> This is quite surprising to me too.  The externally plugged in r8153 dongle,
> was it connected over type C port or over type A port?  AFAIK Type C port is
> actually Alpine ridge pass through port.  It is not connected to XHCI 
> controller
> or USB hub.

Over the front type A port, which connects to SMSC hub then ASMedia controller.

The type C port indeed connects to AR directly, hence no such issue.

Kai-Heng

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> On 24 Nov 2017, at 4:28 PM, Greg KH  wrote:
> 
> The bcdDevice is different between the dock device and the "real"
> device, why not use that?

Yea, I’ll poke around and see if bcdDevice alone can be a good predicate.

> Then there is still a bug.  Who as ASMedia is working on this, have they
> posted anything to the linux-usb mailing list about it?

I think they are doing this internally. I’ll advice them to ask questions here 
if
they encounter any problem.

> Maybe.  Have you tried using usbmon to see what the data streams are for
> the two devices and where they have problems and diverge?  Is the dock
> device doing something different in response to something from the host
> that the "real" device does not do?

No I haven’t.
Not really sure how do debug network packets over USB. I’ll do some research
on the topic.

Kai-Heng
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> Also the MAC address is different, can you just trigger off of Dell's
> MAC address space instead of the address space of the dongle device?

A really good idea, never thought of this. Thanks for the hint :)
Still, I need to ask Dell folks to get all the answers.

Kai-Heng

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> On 23 Nov 2017, at 5:24 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> 
> On Thu, Nov 23, 2017 at 04:53:41PM +0800, Kai Heng Feng wrote:
>> 
>> What I want to do here is to finding this connection:
>> Realtek r8153 <-> SMSC hub (USD ID: 0424:5537) <-> 
>> ASMedia XHCI controller (PCI ID: 1b21:1142).
>> 
>> Is there a safer way to do this?
> 
> Nope!  You can't do that at all from within a USB driver, sorry.  As you
> really should not care at all :)

Got it :)

The r8153 in Dell TB dock has version information, RTL_VER_05.
We can use it to check for workaround, but many working RTL_VER_05 devices
will also be affected.
Do you think it’s an acceptable compromise?

>> I have a r8153 <-> USB 3.0 dongle which work just fine. I can’t find any 
>> information to differentiate them. Hence I want to use the connection to
>> identify if r8153 is on a Dell TB dock.
> 
> Are you sure there is nothing different in the version or release number
> of the device?  'lsusb -v' shows the exact same information for both
> devices?

Yes. I attached `lsusb -v` for r8153 on Dell TB dock, on a RJ45 <-> USB 3.0 
dongle,
and on a RJ45 <-> USB Type-C dongle.

>> Yes. From what I know, ASMedia is working on it, but not sure how long it
>> will take. In the meantime, I’d like to workaround this issue for the users.
> 
> Again, it's a host controller bug, it should be fixed there, don't try
> to paper over the real issue in different individual drivers.
> 
> I think I've seen various patches on the linux-usb list for this
> controller already, have you tried them?

Yes. These patches are all in mainline Linux now.

>> Actually no.
>> I just plugged r8153 dongle into the same hub, surprisingly the issue
>> doesn’t happen in this scenario.
> 
> Then something seems to be wrong with the device itself, as that would
> be the same exact electrical/logical path, right?

I have no idea why externally plugged one doesn’t have this issue.
Maybe it’s related how it’s wired inside the Dell TB dock...

Kai-Heng



lsusb-a
Description: Binary data


lsusb-c
Description: Binary data


lsusb-dock
Description: Binary data

> thanks,
> 
> greg k-h

Re: [PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai Heng Feng]

> On 23 Nov 2017, at 3:58 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> 
> On Thu, Nov 23, 2017 at 01:38:38AM -0500, Kai-Heng Feng wrote:
>> r8153 on Dell TB dock corrupts rx packets.
>> 
>> The root cause is not found yet, but disabling rx checksumming can
>> workaround the issue. We can use this connection to decide if it's
>> a Dell TB dock:
>> Realtek r8153 <-> SMSC hub <-> ASMedia XHCI controller
>> 
>> BugLink: https://bugs.launchpad.net/bugs/1729674
>> Cc: Mario Limonciello <mario.limoncie...@dell.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>> drivers/net/usb/r8152.c | 33 -
>> 1 file changed, 32 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>> index d51d9abf7986..58b80b5e7803 100644
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
>> @@ -27,6 +27,8 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> +#include 
>> 
>> /* Information for net-next */
>> #define NETNEXT_VERSION  "09"
>> @@ -5135,6 +5137,35 @@ static u8 rtl_get_version(struct usb_interface *intf)
>>  return version;
>> }
>> 
>> +/* Ethernet on Dell TB 15/16 dock is connected this way:
>> + * Realtek r8153 <-> SMSC hub <-> ASMedia XHCI controller
>> + * We use this connection to make sure r8153 is on the Dell TB dock.
>> + */
>> +static bool check_dell_tb_dock(struct usb_device *udev)
>> +{
>> +struct usb_device *hub = udev->parent;
>> +struct usb_device *root_hub;
>> +struct pci_dev *controller;
>> +
>> +if (!hub)
>> +return false;
>> +
>> +if (!(le16_to_cpu(hub->descriptor.idVendor) == 0x0424 &&
>> +  le16_to_cpu(hub->descriptor.idProduct) == 0x5537))
>> +return false;
>> +
>> +root_hub = hub->parent;
>> +if (!root_hub || root_hub->parent)
>> +return false;
>> +
>> +controller = to_pci_dev(bus_to_hcd(root_hub->bus)->self.controller);
> 
> That's a very scary, and dangerous, cast.  You can not ever be sure that
> the hub really is a "root hub" like this.

What I want to do here is to finding this connection:
Realtek r8153 <-> SMSC hub (USD ID: 0424:5537) <-> 
ASMedia XHCI controller (PCI ID: 1b21:1142).

Is there a safer way to do this?

> 
>> +if (controller->vendor == 0x1b21 && controller->device == 0x1142)
>> +return true;
> 
> Why can't you just look at the USB device itself and go off of a quirk
> in it?  Something like a version or string or something else?

I have a r8153 <-> USB 3.0 dongle which work just fine. I can’t find any 
information to differentiate them. Hence I want to use the connection to
identify if r8153 is on a Dell TB dock.

> 
> This sounds like a USB host controller issue, not a USB device issue,
> can't we fix the "real" problem here instead of this crazy work-around?

Yes. From what I know, ASMedia is working on it, but not sure how long it
will take. In the meantime, I’d like to workaround this issue for the users.

> Odds are any device plugged into the hub should have the same issue,
> right?

Actually no.
I just plugged r8153 dongle into the same hub, surprisingly the issue
doesn’t happen in this scenario.

Kai-Heng

> 
> thanks,
> 
> greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] r8152: disable rx checksum offload on Dell TB dock

[Kai-Heng Feng]

r8153 on Dell TB dock corrupts rx packets.

The root cause is not found yet, but disabling rx checksumming can
workaround the issue. We can use this connection to decide if it's
a Dell TB dock:
Realtek r8153 <-> SMSC hub <-> ASMedia XHCI controller

BugLink: https://bugs.launchpad.net/bugs/1729674
Cc: Mario Limonciello <mario.limoncie...@dell.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/net/usb/r8152.c | 33 -
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index d51d9abf7986..58b80b5e7803 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -27,6 +27,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /* Information for net-next */
 #define NETNEXT_VERSION"09"
@@ -5135,6 +5137,35 @@ static u8 rtl_get_version(struct usb_interface *intf)
return version;
 }
 
+/* Ethernet on Dell TB 15/16 dock is connected this way:
+ * Realtek r8153 <-> SMSC hub <-> ASMedia XHCI controller
+ * We use this connection to make sure r8153 is on the Dell TB dock.
+ */
+static bool check_dell_tb_dock(struct usb_device *udev)
+{
+   struct usb_device *hub = udev->parent;
+   struct usb_device *root_hub;
+   struct pci_dev *controller;
+
+   if (!hub)
+   return false;
+
+   if (!(le16_to_cpu(hub->descriptor.idVendor) == 0x0424 &&
+ le16_to_cpu(hub->descriptor.idProduct) == 0x5537))
+   return false;
+
+   root_hub = hub->parent;
+   if (!root_hub || root_hub->parent)
+   return false;
+
+   controller = to_pci_dev(bus_to_hcd(root_hub->bus)->self.controller);
+
+   if (controller->vendor == 0x1b21 && controller->device == 0x1142)
+   return true;
+
+   return false;
+}
+
 static int rtl8152_probe(struct usb_interface *intf,
 const struct usb_device_id *id)
 {
@@ -5202,7 +5233,7 @@ static int rtl8152_probe(struct usb_interface *intf,
NETIF_F_HIGHDMA | NETIF_F_FRAGLIST |
NETIF_F_IPV6_CSUM | NETIF_F_TSO6;
 
-   if (tp->version == RTL_VER_01) {
+   if (tp->version == RTL_VER_01 || check_dell_tb_dock(udev)) {
netdev->features &= ~NETIF_F_RXCSUM;
netdev->hw_features &= ~NETIF_F_RXCSUM;
}
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Several issues on AMD Promontory [1022:43bb]

[Kai Heng Feng]

> On 5 Dec 2017, at 4:52 PM, Joe Lee  wrote:
> 
> Hi Kai-Hen,
> I want to know what this kernel version?

v4.15-rc2. The issue happens on all kernel version though. 
> 
> >All three issue can still be reproduced with or without your patch,
> >"[PATCH v8] xhci : AMD Promontory USB disable port support”
> I think this patch has nothing to do with your issue.

I’ve already found the workaround that solves the issue for my machine.
Please review my approach when I send the patch.

Kai-Heng.

> 
> 
> Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Add device quirk for Logitech HD Pro Webcam C925e

[Kai-Heng Feng]

Hi, 

> On 19 Dec 2017, at 12:04 AM, Dmitry Fleytman  
> wrote:
> 
> From: Dmitry Fleytman Dmitry Fleytman 
> 
> Commit e0429362ab15
> ("usb: Add device quirk for Logitech HD Pro Webcams C920 and C930e")
> introduced quirk to workaround an issue with some Logitech webcams.
> 
> There is one more model that has the same issue - C925e, so applying
> the same quirk as well.
> 
> See aforementioned commit message for detailed explanation of the problem.
> 
> Signed-off-by: Dmitry Fleytman 
> ---
> drivers/usb/core/quirks.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index a10b346b9777..6d2d3b0bcc2a 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -52,10 +52,11 @@ static const struct usb_device_id usb_quirk_list[] = {
>   /* Microsoft LifeCam-VX700 v2.0 */
>   { USB_DEVICE(0x045e, 0x0770), .driver_info = USB_QUIRK_RESET_RESUME },
> 
> - /* Logitech HD Pro Webcams C920, C920-C and C930e */
> + /* Logitech HD Pro Webcams C920, C920-C, C925e and C930e */
>   { USB_DEVICE(0x046d, 0x082d), .driver_info = USB_QUIRK_DELAY_INIT },
>   { USB_DEVICE(0x046d, 0x0841), .driver_info = USB_QUIRK_DELAY_INIT },
>   { USB_DEVICE(0x046d, 0x0843), .driver_info = USB_QUIRK_DELAY_INIT },
> + { USB_DEVICE(0x046d, 0x085B), .driver_info = USB_QUIRK_DELAY_INIT },

Nitpicking here, but maybe use 0x085b instead of 0x085B?

Kai-Heng

> 
>   /* Logitech ConferenceCam CC3000e */
>   { USB_DEVICE(0x046d, 0x0847), .driver_info = USB_QUIRK_DELAY_INIT },
> -- 
> 2.14.1
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] Revert "Bluetooth: btusb: fix QCA Rome suspend/resume"

[Kai-Heng Feng]

This reverts commit fd865802c66bc451dc515ed89360f84376ce1a56.

This commit causes a regression on some QCA ROME chips. The USB device
reset happens in btusb_open(), hence firmware loading gets interrupted.

Furthermore, this commit stops working after commit
("a0085f2510e8976614ad8f766b209448b385492f Bluetooth: btusb: driver to
enable the usb-wakeup feature"). Reset-resume quirk only gets enabled in
btusb_suspend() when it's not a wakeup source.

If we really want to reset the USB device, we need to do it before
btusb_open(). Let's handle it in drivers/usb/core/quirks.c.

Cc: sta...@vger.kernel.org
Cc: Leif Liddy <leif.li...@gmail.com>
Cc: Matthias Kaehlcke <m...@chromium.org>
Cc: Brian Norris <briannor...@chromium.org>
Cc: Daniel Drake <dr...@endlessm.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>

---

Daniel, Cc you because this also affects your original quirk patch for
Realtek btusb.

 drivers/bluetooth/btusb.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index f7120c9eb9bd..da353c4acdc7 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3117,12 +3117,6 @@ static int btusb_probe(struct usb_interface *intf,
if (id->driver_info & BTUSB_QCA_ROME) {
data->setup_on_usb = btusb_setup_qca;
hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
-
-   /* QCA Rome devices lose their updated firmware over suspend,
-* but the USB hub doesn't notice any status change.
-* Explicitly request a device reset on resume.
-*/
-   set_bit(BTUSB_RESET_RESUME, >flags);
}
 
 #ifdef CONFIG_BT_HCIBTUSB_RTL
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] usb: quirks: Add reset-resume quirk for Dell DW1820 QCA Rome Bluetooth

[Kai-Heng Feng]

Commit ("fd865802c66bc451dc515ed89360f84376ce1a56 Bluetooth: btusb: fix
QCA Rome suspend/resume") enables reset_resume in btusb_probe(). This
makes the device resets during btusb_open(), firmware loading gets
interrupted as a result.

We still want to reset the device to solve the original issue, but we
should do it before btusb_open().

Hence, add reset-resume quirk in usb core intead of btusb.

Cc: sta...@vger.kernel.org
Cc: Leif Liddy <leif.li...@gmail.com>
Cc: Matthias Kaehlcke <m...@chromium.org>
Cc: Brian Norris <briannor...@chromium.org>
Cc: Daniel Drake <dr...@endlessm.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>

---
 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index a10b346b9777..96951104c45b 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -197,6 +197,9 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x0b05, 0x17e0), .driver_info =
USB_QUIRK_IGNORE_REMOTE_WAKEUP },
 
+   /* QCA Rome Bluetooth in Dell DW1820 wireless module */
+   { USB_DEVICE(0x0cf3, 0xe007), .driver_info = USB_QUIRK_RESET_RESUME },
+
/* Action Semiconductor flash disk */
{ USB_DEVICE(0x10d6, 0x2200), .driver_info =
USB_QUIRK_STRING_FETCH_255 },
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: quirks: Add no-lpm quirk for KY-688 USB 3.1 Type-C Hub

[Kai-Heng Feng]

KY-688 USB 3.1 Type-C Hub internally uses a Genesys Logic hub to connect
to Realtek r8153.

Similar to commit ("7496cfe5431f2 usb: quirks: Add no-lpm quirk for Moshi
USB to Ethernet Adapter"), no-lpm can make r8153 ethernet work.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/core/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index a6aaf2f193a4..12246da8fcf6 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -151,6 +151,9 @@ static const struct usb_device_id usb_quirk_list[] = {
/* appletouch */
{ USB_DEVICE(0x05ac, 0x021a), .driver_info = USB_QUIRK_RESET_RESUME },
 
+   /* Genesys Logic hub, internally used by KY-688 USB 3.1 Type-C Hub */
+   { USB_DEVICE(0x05e3, 0x0612), .driver_info = USB_QUIRK_NO_LPM },
+
/* Genesys Logic hub, internally used by Moshi USB to Ethernet Adapter 
*/
{ USB_DEVICE(0x05e3, 0x0616), .driver_info = USB_QUIRK_NO_LPM },
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: core: lower log level when device is not able to deal with string

[Kai-Heng Feng]

USB devices should work just fine when they don't support language id.

Lower the log level so user won't panic in the future.

BugLink: https://bugs.launchpad.net/bugs/1729618
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/core/message.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 371a07d874a3..dbd67b953e80 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -776,7 +776,7 @@ static int usb_get_langid(struct usb_device *dev, unsigned 
char *tbuf)
 * deal with strings at all. Set string_langid to -1 in order to
 * prevent any string to be retrieved from the device */
if (err < 0) {
-   dev_err(>dev, "string descriptor 0 read error: %d\n",
+   dev_info(>dev, "string descriptor 0 read error: %d\n",
err);
dev->string_langid = -1;
return -EPIPE;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] xhci: Fix front USB ports on ASUS PRIME B350M-A

[Kai-Heng Feng]

The board in question has three XHCI HCs:
02:00.0 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] USB 3.1 XHCI 
Controller [1022:43bb] (rev 02)
04:00.0 USB controller [0c03]: ASMedia Technology Inc. Device [1b21:1343]
0a:00.3 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] USB3 Host 
Controller [1022:145c]

The front ports are connected to HC [1022:43bb] (02:00.0).

After plugging high speed devices into front ports several times, it
stops responding to high speed devices. Super speed devices can still
get detected though.

When I plugged a high speed device, [1b21:1343] (04:00.0) also got woke
up:
[  140.834823] xhci_hcd :02:00.0: // Setting command ring address to 
0xff73b001
[  140.838220] xhci_hcd :02:00.0: xhci_resume: starting port polling.
[  140.838226] xhci_hcd :02:00.0: xhci_hub_status_data: stopping port 
polling.
[  140.838266] xhci_hcd :02:00.0: xhci_suspend: stopping port polling.
[  140.838362] xhci_hcd :02:00.0: // Setting command ring address to 
0xff73b001
[  140.858908] xhci_hcd :04:00.0: // Setting command ring address to 
0xfffd9001
[  140.862745] xhci_hcd :04:00.0: xhci_resume: starting port polling.
[  140.862754] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
polling.
[  140.862760] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
polling.
[  140.862787] xhci_hcd :04:00.0: xhci_suspend: stopping port polling.
[  140.862811] xhci_hcd :04:00.0: // Setting command ring address to 
0xfffd9001

Seems like [1022:43bb] is wired to [1b21:1343] internally.

Both HCs support D3hot PME, so I tried to disable D3cold on them.
Turns out disable D3cold on [1b21:1343] (04:00.0) can workaround the
issue for [1022:43bb] (02:00.0).

Also, [1022:43bb] (02:00.0) sometimes failed to suspend:
[  549.114587] xhci_hcd :02:00.0: WARN: xHC CMD_RUN timeout
[  549.114608] suspend_common(): xhci_pci_suspend+0x0/0xc0 returns -110
[  549.114638] xhci_hcd :02:00.0: can't suspend (hcd_pci_runtime_suspend 
returned -110)
[  549.116744] usb usb3: root hub lost power or was reset
[  549.116746] usb usb4: root hub lost power or was reset

Based on previous guesswork, the issue can be workaround by doing PCI
reset on [1b21:1343] (04:00.0).

Cc: Joe Lee <asmt.sw...@gmail.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/pci/quirks.c  | 2 ++
 drivers/usb/host/pci-quirks.c | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 10684b17d0bd..380061dae51d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1699,6 +1699,8 @@ static void quirk_radeon_pm(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
 
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1343, pci_d3cold_disable);
+
 #ifdef CONFIG_X86_IO_APIC
 static int dmi_disable_ioapicreroute(const struct dmi_system_id *d)
 {
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 161536717025..8c4318671e37 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1176,6 +1176,9 @@ bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
(pdev->device == 0x0014 || pdev->device == 0x0015))
return true;
 
+   if (pdev->vendor == PCI_VENDOR_ID_ASMEDIA && pdev->device == 0x1343)
+   return true;
+
return false;
 }
 EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Several issues on AMD Promontory [1022:43bb]

[Kai-Heng Feng]

Hi Joe,

I’ve had several issues on AMD Promontory [1022:43bb] XHCI controller,
apparently you are the right guy to ask ;)

Board: ASUS PRIME B350M-A
BIOS version: 3203

Here are the issues:
1. The port stops responding after plugging high speed devices several times.
The XHCI resumes then immediately suspend.

[  134.602886] xhci_hcd :02:00.0: // Setting command ring address to 
0xff73b001
[  134.606312] xhci_hcd :02:00.0: xhci_resume: starting port polling.
[  134.606318] xhci_hcd :02:00.0: xhci_hub_status_data: stopping port 
polling.
[  134.606346] xhci_hcd :02:00.0: xhci_suspend: stopping port polling.
[  134.606435] xhci_hcd :02:00.0: // Setting command ring address to 
0xff73b001
[  134.626860] xhci_hcd :04:00.0: // Setting command ring address to 
0xfffd9001
[  134.630717] xhci_hcd :04:00.0: xhci_resume: starting port polling.
[  134.630722] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
polling.
[  134.630726] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
polling.
[  134.630738] xhci_hcd :04:00.0: xhci_suspend: stopping port polling.
[  134.630760] xhci_hcd :04:00.0: // Setting command ring address to 
0xfffd9001
[  140.834823] xhci_hcd :02:00.0: // Setting command ring address to 
0xff73b001
[  140.838220] xhci_hcd :02:00.0: xhci_resume: starting port polling.
[  140.838226] xhci_hcd :02:00.0: xhci_hub_status_data: stopping port 
polling.
[  140.838266] xhci_hcd :02:00.0: xhci_suspend: stopping port polling.
[  140.838362] xhci_hcd :02:00.0: // Setting command ring address to 
0xff73b001
[  140.858908] xhci_hcd :04:00.0: // Setting command ring address to 
0xfffd9001
[  140.862745] xhci_hcd :04:00.0: xhci_resume: starting port polling.
[  140.862754] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
polling.
[  140.862760] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
polling.
[  140.862787] xhci_hcd :04:00.0: xhci_suspend: stopping port polling.
[  140.862811] xhci_hcd :04:00.0: // Setting command ring address to 
0xfffd9001
[  322.468521] xhci_hcd :02:00.0: // Setting command ring address to 
0xff73b001

Use XHCI_RESET_ON_RESUME quirk can workaround this issue. But
maybe a little overkill?

2. XHCI failed to suspend. USB ports stopped to work after that. 
[  549.114587] xhci_hcd :02:00.0: WARN: xHC CMD_RUN timeout
[  549.114608] suspend_common(): xhci_pci_suspend+0x0/0xc0 returns -110
[  549.114638] xhci_hcd :02:00.0: can't suspend (hcd_pci_runtime_suspend 
returned -110)
[  549.116744] usb usb3: root hub lost power or was reset
[  549.116746] usb usb4: root hub lost power or was reset

XHCI_RESET_ON_RESUME quirk cannot workaround issue.


3. Can’t detect high speed devices after plugging super speed device.
Unlike 1., the HC is totally unresponsive to high speed devices. No interrupt
gets generated.

Super speed devices can still be correctly detected though.

All three issue can still be reproduced with or without your patch,
"[PATCH v8] xhci : AMD Promontory USB disable port support”

So I guess we need more quirks to support Promontory properly.

Kai-Heng--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: core: Add "quirks" parameter for usbcore

[Kai Heng Feng]

> On 6 Dec 2017, at 10:10 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> 
> On Wed, Dec 06, 2017 at 06:26:21PM +0800, Kai-Heng Feng wrote:
>> Trying quirks in usbcore needs to rebuild the driver or the entire
>> kernel if it's builtin. It can save a lot of time if usbcore has similar
>> ability like "usbhid.quirks=" and "usb-storage.quirks=".
>> 
>> Rename the original quirk detection function to "static" as we introduce
>> this new "dynamic" function.
>> 
>> Now users can use "usbcore.quirks=" as short term workaround before the
>> next kernel release.
>> 
>> This is inspired by usbhid and usb-storage.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>> v2: use in-kernel tolower() function.
>> 
>> Documentation/admin-guide/kernel-parameters.txt |  55 +
>> drivers/usb/core/quirks.c   | 100 
>> ++--
>> include/linux/usb/quirks.h  |   2 +
>> 3 files changed, 151 insertions(+), 6 deletions(-)
>> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
>> b/Documentation/admin-guide/kernel-parameters.txt
>> index 6571fbfdb2a1..d42fb278320b 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4302,6 +4302,61 @@
>> 
>>  usbcore.nousb   [USB] Disable the USB subsystem
>> 
>> +usbcore.quirks=
>> +[USB] A list of quirks entries to supplement or
>> +override the built-in usb core quirk list.  List
>> +entries are separated by commas.  Each entry has
>> +the form VID:PID:Flags where VID and PID are Vendor
>> +and Product ID values (4-digit hex numbers) and
>> +Flags is a set of characters, each corresponding
>> +to a common usb core quirk flag as follows:
>> +a = USB_QUIRK_STRING_FETCH_255 (string
>> +descriptors must not be fetched using
>> +a 255-byte read);
>> +b = USB_QUIRK_RESET_RESUME (device can't resume
>> +correctly so reset it instead);
>> +c = USB_QUIRK_NO_SET_INTF (device can't handle
>> +Set-Interface requests);
>> +d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
>> +handle its Configuration or Interface
>> +strings);
>> +e = USB_QUIRK_RESET (device can't be reset
>> +(e.g morph devices), don't use reset);
>> +f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
>> +more interface descriptions than the
>> +bNumInterfaces count, and can't handle
>> +talking to these interfaces);
>> +g = USB_QUIRK_DELAY_INIT (device needs a pause
>> +during initialization, after we read
>> +the device descriptor);
>> +h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
>> +high speed and super speed interrupt
>> +endpoints, the USB 2.0 and USB 3.0 spec
>> +require the interval in microframes (1
>> +microframe = 125 microseconds) to be
>> +calculated as interval = 2 ^
>> +(bInterval-1).
>> +Devices with this quirk report their
>> +bInterval as the result of this
>> +calculation instead of the exponent
>> +variable used in the calculation);
>> +i = USB_QUIRK_DEVICE_QUALIFIER (device can't
>> +handle device_qualifier descriptor
>> +requests);
>> +j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
>> +generates spurious wakeup, ignore
&

[PATCH] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  55 +
 drivers/usb/core/quirks.c   | 103 ++--
 include/linux/usb/quirks.h  |   2 +
 3 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 6571fbfdb2a1..f1cb31aa25e3 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4302,6 +4302,61 @@
 
usbcore.nousb   [USB] Disable the USB subsystem
 
+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:
+   a = USB_QUIRK_STRING_FETCH_255 (string
+   descriptors must not be fetched using
+   a 255-byte read);
+   b = USB_QUIRK_RESET_RESUME (device can't resume
+   correctly so reset it instead);
+   c = USB_QUIRK_NO_SET_INTF (device can't handle
+   Set-Interface requests);
+   d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+   handle its Configuration or Interface
+   strings);
+   e = USB_QUIRK_RESET (device can't be reset
+   (e.g morph devices), don't use reset);
+   f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+   more interface descriptions than the
+   bNumInterfaces count, and can't handle
+   talking to these interfaces);
+   g = USB_QUIRK_DELAY_INIT (device needs a pause
+   during initialization, after we read
+   the device descriptor);
+   h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+   high speed and super speed interrupt
+   endpoints, the USB 2.0 and USB 3.0 spec
+   require the interval in microframes (1
+   microframe = 125 microseconds) to be
+   calculated as interval = 2 ^
+   (bInterval-1).
+   Devices with this quirk report their
+   bInterval as the result of this
+   calculation instead of the exponent
+   variable used in the calculation);
+   i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+   handle device_qualifier descriptor
+   requests);
+   j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+   generates spurious wakeup, ignore
+   remote wakeup capability);
+   k = USB_QUIRK_NO_LPM (device can't handle Link
+   Power Management);
+   l = USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL
+   (Device reports its bInterval as linear
+   frames instead of the USB 2.0
+   calculation);
+   m = USB_QUIRK_DISCONNECT_SUSPEND (Device needs
+   to be disconnected before suspend to
+   prevent spurious wakeup)
+   Example: quir

Re: [PATCH] usb: core: Add "quirks" parameter for usbcore

[Kai Heng Feng]

> On 6 Dec 2017, at 5:13 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> 
> On Wed, Dec 06, 2017 at 05:09:32PM +0800, Kai-Heng Feng wrote:
>> +/* Works only for digits and letters, but small and fast */
>> +#define TOLOWER(x) ((x) | 0x20)
> 
> What is wrong with the in-kernel version of tolower()?

Nothing’s wrong with the in-kernel version. I’ll use that in v2.

Other than that, is there anythings I can make to improve the code?

Kai-Heng

> 
> thanks,
> 
> greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2: use in-kernel tolower() function.

 Documentation/admin-guide/kernel-parameters.txt |  55 +
 drivers/usb/core/quirks.c   | 100 ++--
 include/linux/usb/quirks.h  |   2 +
 3 files changed, 151 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 6571fbfdb2a1..d42fb278320b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4302,6 +4302,61 @@
 
usbcore.nousb   [USB] Disable the USB subsystem
 
+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:
+   a = USB_QUIRK_STRING_FETCH_255 (string
+   descriptors must not be fetched using
+   a 255-byte read);
+   b = USB_QUIRK_RESET_RESUME (device can't resume
+   correctly so reset it instead);
+   c = USB_QUIRK_NO_SET_INTF (device can't handle
+   Set-Interface requests);
+   d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+   handle its Configuration or Interface
+   strings);
+   e = USB_QUIRK_RESET (device can't be reset
+   (e.g morph devices), don't use reset);
+   f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+   more interface descriptions than the
+   bNumInterfaces count, and can't handle
+   talking to these interfaces);
+   g = USB_QUIRK_DELAY_INIT (device needs a pause
+   during initialization, after we read
+   the device descriptor);
+   h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+   high speed and super speed interrupt
+   endpoints, the USB 2.0 and USB 3.0 spec
+   require the interval in microframes (1
+   microframe = 125 microseconds) to be
+   calculated as interval = 2 ^
+   (bInterval-1).
+   Devices with this quirk report their
+   bInterval as the result of this
+   calculation instead of the exponent
+   variable used in the calculation);
+   i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+   handle device_qualifier descriptor
+   requests);
+   j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+   generates spurious wakeup, ignore
+   remote wakeup capability);
+   k = USB_QUIRK_NO_LPM (device can't handle Link
+   Power Management);
+   l = USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL
+   (Device reports its bInterval as linear
+   frames instead of the USB 2.0
+   calculation);
+   m = USB_QUIRK_DISCONNECT_SUSPEND (Device needs
+   to be disconnected before suspend to
+   prevent spurious wakeup)
+

Re: [PATCH] xhci: Fix front USB ports on ASUS PRIME B350M-A

[Kai-Heng Feng]

On Wed, Dec 6, 2017 at 1:14 AM, Bjorn Helgaas <helg...@kernel.org> wrote:
> [+cc Rafael, linux-pm]
>
> On Wed, Dec 06, 2017 at 12:22:42AM +0800, Kai-Heng Feng wrote:
>> The board in question has three XHCI HCs:
>> 02:00.0 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] USB 3.1 
>> XHCI Controller [1022:43bb] (rev 02)
>> 04:00.0 USB controller [0c03]: ASMedia Technology Inc. Device [1b21:1343]
>> 0a:00.3 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] USB3 Host 
>> Controller [1022:145c]
>>
>> The front ports are connected to HC [1022:43bb] (02:00.0).
>>
>> After plugging high speed devices into front ports several times, it
>> stops responding to high speed devices. Super speed devices can still
>> get detected though.
>>
>> When I plugged a high speed device, [1b21:1343] (04:00.0) also got woke
>> up:
>> [  140.834823] xhci_hcd :02:00.0: // Setting command ring address to 
>> 0xff73b001
>> [  140.838220] xhci_hcd :02:00.0: xhci_resume: starting port polling.
>> [  140.838226] xhci_hcd :02:00.0: xhci_hub_status_data: stopping port 
>> polling.
>> [  140.838266] xhci_hcd :02:00.0: xhci_suspend: stopping port polling.
>> [  140.838362] xhci_hcd :02:00.0: // Setting command ring address to 
>> 0xff73b001
>> [  140.858908] xhci_hcd :04:00.0: // Setting command ring address to 
>> 0xfffd9001
>> [  140.862745] xhci_hcd :04:00.0: xhci_resume: starting port polling.
>> [  140.862754] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
>> polling.
>> [  140.862760] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
>> polling.
>> [  140.862787] xhci_hcd :04:00.0: xhci_suspend: stopping port polling.
>> [  140.862811] xhci_hcd :04:00.0: // Setting command ring address to 
>> 0xfffd9001
>>
>> Seems like [1022:43bb] is wired to [1b21:1343] internally.
>>
>> Both HCs support D3hot PME, so I tried to disable D3cold on them.
>> Turns out disable D3cold on [1b21:1343] (04:00.0) can workaround the
>> issue for [1022:43bb] (02:00.0).
>>
>> Also, [1022:43bb] (02:00.0) sometimes failed to suspend:
>> [  549.114587] xhci_hcd :02:00.0: WARN: xHC CMD_RUN timeout
>> [  549.114608] suspend_common(): xhci_pci_suspend+0x0/0xc0 returns -110
>> [  549.114638] xhci_hcd :02:00.0: can't suspend (hcd_pci_runtime_suspend 
>> returned -110)
>> [  549.116744] usb usb3: root hub lost power or was reset
>> [  549.116746] usb usb4: root hub lost power or was reset
>>
>> Based on previous guesswork, the issue can be workaround by doing PCI
>> reset on [1b21:1343] (04:00.0).
>
> Naive questions: Does this indicate that xhci_hcd is lacking something
> in the "resume from D3cold" path?

I think it's not related to xhci_hcd, it's weird to 04:00.0 gets woke
up because
something plugged into 02:00.0's port at first place.

> Is there a bugzilla for this? If
> not, can you open one and attach the complete dmesg log and complete
> "lspci -vv" output (as root)?  Google found some possibly related
> issues but I can't quite tie them together yet.

https://bugzilla.kernel.org/show_bug.cgi?id=198097

>
> I don't have an issue with the quirk per se, but in general I think
> it's better to debug an issue than simply avoid it.

Thanks. I hope ASMedia can shed some light on this issue.

>
>> Cc: Joe Lee <asmt.sw...@gmail.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
>> ---
>>  drivers/pci/quirks.c  | 2 ++
>>  drivers/usb/host/pci-quirks.c | 3 +++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 10684b17d0bd..380061dae51d 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -1699,6 +1699,8 @@ static void quirk_radeon_pm(struct pci_dev *dev)
>>  }
>>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
>>
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ASMEDIA, 0x1343, pci_d3cold_disable);
>> +
>>  #ifdef CONFIG_X86_IO_APIC
>>  static int dmi_disable_ioapicreroute(const struct dmi_system_id *d)
>>  {
>> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
>> index 161536717025..8c4318671e37 100644
>> --- a/drivers/usb/host/pci-quirks.c
>> +++ b/drivers/usb/host/pci-quirks.c
>> @@ -1176,6 +1176,9 @@ bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
>>   (pdev->device == 0x0014 || pdev->device == 0x0015))
>>   return true;
>>
>> + if (pdev->vendor == PCI_VENDOR_ID_ASMEDIA && pdev->device == 0x1343)
>> + return true;
>> +
>>   return false;
>>  }
>>  EXPORT_SYMBOL_GPL(usb_xhci_needs_pci_reset);
>> --
>> 2.14.1
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] r8152: disable RX aggregation on Dell TB16 dock

[Kai Heng Feng]

> On 18 Jan 2018, at 10:50 PM, David Miller  wrote:
> 
> From: Hayes Wang 
> Date: Thu, 18 Jan 2018 03:04:08 +
> 
>> [...]
 r8153 on Dell TB15/16 dock corrupts rx packets.
 
 This change is suggested by Realtek. They guess that the XHCI
 controller doesn't have enough buffer, and their guesswork is correct,
 once the RX aggregation gets disabled, the issue is gone.
 
 ASMedia is currently working on a real sulotion for this issue.
 
 Dell and ODM confirm the bcdDevice and iSerialNumber is unique for TB16.
 
 Note that TB15 has different bcdDevice and iSerialNumber, which are
 not unique values. If you still have TB15, please contact Dell to
 replace it with TB16.
>> 
>> Excuse me. I don't understand why this patch is for specific USB nic rather 
>> than xHCI.
>> It seems to make the specific USB nic working and the other ones keeping 
>> error.
> 
> Well, are we sure that the device being in the TB16 dock doesn't
> contribute to the issue as well?

This is what vendors concluded for now. The very same NIC on WD15 doesn’t
have the issue.

> 
> If the problem only shows up with XHCI and this dock, then this patch
> was the appropriate way to deal with the problem for now.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] r8152: disable RX aggregation on Dell TB16 dock

[Kai-Heng Feng]

r8153 on Dell TB15/16 dock corrupts rx packets.

This change is suggested by Realtek. They guess that the XHCI controller
doesn't have enough buffer, and their guesswork is correct, once the RX
aggregation gets disabled, the issue is gone.

ASMedia is currently working on a real sulotion for this issue.

Dell and ODM confirm the bcdDevice and iSerialNumber is unique for TB16.

Note that TB15 has different bcdDevice and iSerialNumber, which are not
unique values. If you still have TB15, please contact Dell to replace it
with TB16.

BugLink: https://bugs.launchpad.net/bugs/1729674
Cc: Mario Limonciello <mario.limoncie...@dell.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2:
- Disable RX aggregation instead of disable RX checksum
- Use bcdDevice and iSerialNumber to uniquely identify Dell TB16

 drivers/net/usb/r8152.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index d51d9abf7986..0657203ffb91 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -606,6 +606,7 @@ enum rtl8152_flags {
PHY_RESET,
SCHEDULE_NAPI,
GREEN_ETHERNET,
+   DELL_TB_RX_AGG_BUG,
 };
 
 /* Define these values to match your device */
@@ -1798,6 +1799,9 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct 
tx_agg *agg)
dev_kfree_skb_any(skb);
 
remain = agg_buf_sz - (int)(tx_agg_align(tx_data) - agg->head);
+
+   if (test_bit(DELL_TB_RX_AGG_BUG, >flags))
+   break;
}
 
if (!skb_queue_empty(_head)) {
@@ -4133,6 +4137,9 @@ static void r8153_init(struct r8152 *tp)
/* rx aggregation */
ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
+   if (test_bit(DELL_TB_RX_AGG_BUG, >flags))
+   ocp_data |= RX_AGG_DISABLE;
+
ocp_write_word(tp, MCU_TYPE_USB, USB_USB_CTRL, ocp_data);
 
rtl_tally_reset(tp);
@@ -5207,6 +5214,12 @@ static int rtl8152_probe(struct usb_interface *intf,
netdev->hw_features &= ~NETIF_F_RXCSUM;
}
 
+   if (le16_to_cpu(udev->descriptor.bcdDevice) == 0x3011 &&
+   udev->serial && !strcmp(udev->serial, "0100")) {
+   dev_info(>dev, "Dell TB16 Dock, disable RX aggregation");
+   set_bit(DELL_TB_RX_AGG_BUG, >flags);
+   }
+
netdev->ethtool_ops = 
netif_set_gso_max_size(netdev, RTL_LIMITED_TSO_SIZE);
 
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] r8152: disable RX aggregation on Dell TB16 dock

[Kai Heng Feng]

> On 18 Jan 2018, at 11:04 AM, Hayes Wang  wrote:
> 
> [...]
>>> r8153 on Dell TB15/16 dock corrupts rx packets.
>>> 
>>> This change is suggested by Realtek. They guess that the XHCI
>>> controller doesn't have enough buffer, and their guesswork is correct,
>>> once the RX aggregation gets disabled, the issue is gone.
>>> 
>>> ASMedia is currently working on a real sulotion for this issue.
>>> 
>>> Dell and ODM confirm the bcdDevice and iSerialNumber is unique for TB16.
>>> 
>>> Note that TB15 has different bcdDevice and iSerialNumber, which are
>>> not unique values. If you still have TB15, please contact Dell to
>>> replace it with TB16.
> 
> Excuse me. I don't understand why this patch is for specific USB nic rather 
> than xHCI.
> It seems to make the specific USB nic working and the other ones keeping 
> error.

This patch is transient, once ASMedia find the correct solution, the patch
can be reverted.

This patch only targets the builtin 8153 because externally plugged r8152 or
asix do not suffered from this issue.

Kai-Heng

> 
> 
> Best Regards,
> Hayes
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add delay-init quirk for Corsair K70 RGB keyboards

[Kai Heng Feng]

> On 2 Feb 2018, at 11:51 PM, JackStocker  wrote:
> 
> From: Jack Stocker 
> 
> Following on from this patch: https://lkml.org/lkml/2017/11/3/516,
> Corsair K70 RGB keyboards also require the DELAY_INIT quirk to
> start correctly at boot.
> 
> Device ids found here:
> usb 3-3: New USB device found, idVendor=1b1c, idProduct=1b13
> usb 3-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> usb 3-3: Product: Corsair K70 RGB Gaming Keyboard 
> 
> Signed-off-by: Jack Stocker 
> ---
> drivers/usb/core/quirks.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index a6aaf2f..9eb92dc 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -221,6 +221,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>   /* Corsair Strafe RGB */
>   { USB_DEVICE(0x1b1c, 0x1b20), .driver_info = USB_QUIRK_DELAY_INIT },
> 
> + /* Corsair K70 RGB */
> + { USB_DEVICE(0x1b1c, 0x1b13), .driver_info = USB_QUIRK_DELAY_INIT },
> +

I think this change should move up, to make the table follows an ascending 
order.

>   /* MIDI keyboard WORLDE MINI */
>   { USB_DEVICE(0x1c75, 0x0204), .driver_info =
>   USB_QUIRK_CONFIG_INTF_STRINGS },
> -- 
> 2.7.4
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Add delay-init quirk for Corsair K70 RGB keyboards

[Kai Heng Feng]

> On 5 Feb 2018, at 12:18 AM, JackStocker  wrote:
> 
> Do you mean like this?

Yes, with proper commit message.

> 
> Signed-off-by: JackStocker 
> ---
> drivers/usb/core/quirks.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index a6aaf2f..0405d68 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -218,6 +218,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>   { USB_DEVICE(0x1a0a, 0x0200), .driver_info =
>   USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL },
> 
> + /* Corsair K70 RGB */
> + { USB_DEVICE(0x1b1c, 0x1b13), .driver_info = USB_QUIRK_DELAY_INIT },
> +
>   /* Corsair Strafe RGB */
>   { USB_DEVICE(0x1b1c, 0x1b20), .driver_info = USB_QUIRK_DELAY_INIT },
> 
> -- 
> 2.7.4
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paired bluetooth keyboards lead to system auto resume from S3 when WoL enabled for e1000e

[Kai Heng Feng]

Hi,

This is actually a continuation of [1] with more findings.
The machine in question is a CFL desktop. The auto resume roughly
happens in 5 ~ 10 mins.

After further digging, I found out that the e1000e and XHCI always
shares the same wakeup count in /sys/kernel/debug/wakeup_sources,
so I did some experiment on e1000e.

Here’s the tricky finding, disable WoL for e1000e can stop the erratic
behaviour - the machine no longer auto resume. One caveat though -
I can no longer wake up the system by pressing the BT keyboard.

No idea where to file the bug so include all three subsystems.

[1] https://www.spinics.net/lists/linux-bluetooth/msg73683.html

Kai-Heng--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xhci: Fix front USB ports on ASUS PRIME B350M-A

[Kai-Heng Feng]

> On 20 Feb 2018, at 4:24 PM, Mathias Nyman <mathias.ny...@linux.intel.com> 
> wrote:
> 
> Hi
> 
> On 19.02.2018 21:06, Kai-Heng Feng wrote:
>> When a USB device gets plugged on ASUS PRIME B350M-A's front ports, the
>> xHC stops working:
>> [  549.114587] xhci_hcd :02:00.0: WARN: xHC CMD_RUN timeout
>> [  549.114608] suspend_common(): xhci_pci_suspend+0x0/0xc0 returns -110
>> [  549.114638] xhci_hcd :02:00.0: can't suspend (hcd_pci_runtime_suspend 
>> returned -110)
> 
> Is the xhci runtime suspend something that you expected here?
> I mean was the plugged device first enumerated normally, driver bound, 
> inactive and runtime suspended
> first?

I guess this is how it behaves, here’s the log after a device gets plugged:

[ 7535.716578] xhci_hcd :02:00.0: // Setting command ring address to 
0xff86c001
[ 7535.720107] xhci_hcd :02:00.0: xhci_resume: starting port polling.
[ 7535.720114] xhci_hcd :02:00.0: xhci_hub_status_data: stopping port 
polling.
[ 7535.720134] xhci_hcd :02:00.0: xhci_suspend: stopping port polling.
[ 7535.720190] xhci_hcd :02:00.0: Port Status Change Event for port 8
[ 7535.720194] xhci_hcd :02:00.0: resume root hub
[ 7535.720201] xhci_hcd :02:00.0: handle_port_status: starting port polling.
[ 7535.721675] xhci_hcd :02:00.0: // Setting command ring address to 
0xff86c001
[ 7535.722106] xhci_hcd :02:00.0: // Setting command ring address to 
0xff86c001
[ 7535.725538] xhci_hcd :02:00.0: xhci_resume: starting port polling.

So the xHC resumes and suspends, then resume again. 

[ 7535.725543] xhci_hcd :02:00.0: xhci_hub_status_data: stopping port 
polling.
[ 7535.725587] xhci_hcd :02:00.0: get port status, actual port 0 status  = 
0x2a0
[ 7535.725590] xhci_hcd :02:00.0: Get port status returned 0x100
[ 7535.725606] xhci_hcd :02:00.0: get port status, actual port 1 status  = 
0x2a0
[ 7535.725608] xhci_hcd :02:00.0: Get port status returned 0x100
[ 7535.725616] xhci_hcd :02:00.0: get port status, actual port 2 status  = 
0x2a0
[ 7535.725618] xhci_hcd :02:00.0: Get port status returned 0x100
[ 7535.725625] xhci_hcd :02:00.0: get port status, actual port 3 status  = 
0x202e1
[ 7535.725627] xhci_hcd :02:00.0: Get port status returned 0x10101
[ 7535.725632] xhci_hcd :02:00.0: Port Status Change Event for port 8
[ 7535.725636] xhci_hcd :02:00.0: handle_port_status: starting port polling.
[ 7535.725656] xhci_hcd :02:00.0: clear port connect change, actual port 3 
status  = 0x2e1
[ 7535.725663] xhci_hcd :02:00.0: get port status, actual port 4 status  = 
0x2a0
[ 7535.725665] xhci_hcd :02:00.0: Get port status returned 0x100
[ 7535.725672] xhci_hcd :02:00.0: get port status, actual port 5 status  = 
0x2a0
[ 7535.725674] xhci_hcd :02:00.0: Get port status returned 0x100
[ 7535.725680] xhci_hcd :02:00.0: get port status, actual port 6 status  = 
0x2a0
[ 7535.725682] xhci_hcd :02:00.0: Get port status returned 0x100
[ 7535.725689] xhci_hcd :02:00.0: get port status, actual port 7 status  = 
0x2a0
[ 7535.725691] xhci_hcd :02:00.0: Get port status returned 0x100
[ 7535.725698] xhci_hcd :02:00.0: get port status, actual port 8 status  = 
0x2a0
[ 7535.725699] xhci_hcd :02:00.0: Get port status returned 0x100
[ 7535.725706] xhci_hcd :02:00.0: get port status, actual port 9 status  = 
0x2a0
[ 7535.725708] xhci_hcd :02:00.0: Get port status returned 0x100

[ 7535.736165] xhci_hcd :04:00.0: // Setting command ring address to 
0xfff58001
[ 7535.740033] xhci_hcd :04:00.0: xhci_resume: starting port polling.
[ 7535.740040] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
polling.
[ 7535.740047] xhci_hcd :04:00.0: xhci_hub_status_data: stopping port 
polling.
[ 7535.740133] xhci_hcd :04:00.0: xhci_suspend: stopping port polling.
[ 7535.740161] xhci_hcd :04:00.0: // Setting command ring address to 
0xfff58001

This is the “other” xHC that I mentioned in patch v1. It provides USB 3.1 
capabilities to front USB port.

[ 7535.776003] xhci_hcd :02:00.0: xhci_hub_status_data: stopping port 
polling.
[ 7535.832028] xhci_hcd :02:00.0: get port status, actual port 3 status  = 
0x2e1
[ 7535.832031] xhci_hcd :02:00.0: Get port status returned 0x101
[ 7535.832051] xhci_hcd :02:00.0: // Ding dong!
[ 7535.832150] xhci_hcd :02:00.0: Slot 3 output ctx = 0xff125000 (dma)
[ 7535.832153] xhci_hcd :02:00.0: Slot 3 input ctx = 0xff16c000 (dma)
[ 7535.832160] xhci_hcd :02:00.0: Set slot id 3 dcbaa entry 
0bd2158a to 0xff125000
[ 7535.832189] xhci_hcd :02:00.0: set port reset, actual port 3 status  = 
0x2e1
[ 7535.835982] xhci_hcd :02:00.0: xhci_hub_status_data: stopping port 
polling.
[ 7535.892049] xhci_hcd :02:00.0: Port Status Change Event for port 8
[ 7535.892056] xhci_hcd :02:00.0: handle_port_status: starting port polling.

[PATCH v2] xhci: Fix front USB ports on ASUS PRIME B350M-A

[Kai-Heng Feng]

When a USB device gets plugged on ASUS PRIME B350M-A's front ports, the
xHC stops working:
[  549.114587] xhci_hcd :02:00.0: WARN: xHC CMD_RUN timeout
[  549.114608] suspend_common(): xhci_pci_suspend+0x0/0xc0 returns -110
[  549.114638] xhci_hcd :02:00.0: can't suspend (hcd_pci_runtime_suspend 
returned -110)

Delay before running xHC command CMD_RUN can workaround the issue.

Use a new quirk to make the delay only targets to the affected xHC.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v2: Instead of doing xHC reset and disabling D3cold, a simple delay can
workaround the issue. Now both high-speed and super-speed devices work
fine with the v2 quirk.

 drivers/usb/host/xhci-pci.c | 3 +++
 drivers/usb/host/xhci.c | 3 +++
 drivers/usb/host/xhci.h | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 6c79037876db..9a820b3cae56 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -122,6 +122,9 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_find_chipset_info())
xhci->quirks |= XHCI_AMD_PLL_FIX;
 
+   if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x43bb)
+   xhci->quirks |= XHCI_SUSPEND_DELAY;
+
if (pdev->vendor == PCI_VENDOR_ID_AMD)
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 1eeb3396300f..d554bbd694d3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -878,6 +878,9 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
clear_bit(HCD_FLAG_POLL_RH, >shared_hcd->flags);
del_timer_sync(>shared_hcd->rh_timer);
 
+   if (xhci->quirks & XHCI_SUSPEND_DELAY)
+   usleep_range(1000, 1500);
+
spin_lock_irq(>lock);
clear_bit(HCD_FLAG_HW_ACCESSIBLE, >flags);
clear_bit(HCD_FLAG_HW_ACCESSIBLE, >shared_hcd->flags);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 96099a245c69..6f1f52f3cb40 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1825,6 +1825,7 @@ struct xhci_hcd {
 /* Reserved. It was XHCI_U2_DISABLE_WAKE */
 #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL(1 << 28)
 #define XHCI_HW_LPM_DISABLE(1 << 29)
+#define XHCI_SUSPEND_DELAY (1 << 30)
 
unsigned intnum_active_eps;
unsigned intlimit_active_eps;
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] Revert "Bluetooth: btusb: fix QCA Rome suspend/resume"

[Kai Heng Feng]

> On 21 Dec 2017, at 7:43 PM, Daniel Drake <dr...@endlessm.com> wrote:
> 
> On Wed, Dec 20, 2017 at 6:53 PM, Brian Norris <briannor...@chromium.org> 
> wrote:
>> 
>> On Wed, Dec 20, 2017 at 07:00:07PM +0800, Kai-Heng Feng wrote:
>>> This commit causes a regression on some QCA ROME chips. The USB device
>>> reset happens in btusb_open(), hence firmware loading gets interrupted.
>> 
>> Oh, did you really confirm that's the root of the problem? I was only
>> hypothesizing, with some informed observation and code review; but I
>> didn't fully convince myself. If so, that's interesting.
> 
> I have the same doubt. Can you explain how/why firmware uploading and
> btusb_open() overlap, and how this is avoided with your patch?

QCA ROME chip uploads its firmware inside btusb_open().

The behavior is like below:
- btusb_probe()
- btusb_open()
- btusb_suspend(), reset_resume gets set.
- btusb_open() again, hub resets the device, then the issue happens.

I didn’t dig really deep for the issue, I simply tried usb core quirks, it reset
the USB device before btusb_probe().

It might be that using the USB quirk only papers over the real issue.

> If they do overlap, is that not a bug in the stack that should be fixed 
> instead?
> If the fix belongs in btusb and this BTUSB_RESET_RESUME thing really
> is problematic, should it be totally removed instead?

I think so. That’s why I need some insight from the original patch author.

Kai-Heng

> 
> Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release. Also, the quirk parameter can XOR the builtin
quirks for debugging purpose.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v3: Stop overridding static quirks.
Use XOR for dynamic quirks.
Save parsed quirk as a list to avoid parsing quirk table everytime.

v2: Use in-kernel tolower() function.

 Documentation/admin-guide/kernel-parameters.txt |  55 
 drivers/usb/core/quirks.c   | 160 +++-
 drivers/usb/core/usb.c  |   1 +
 drivers/usb/core/usb.h  |   1 +
 4 files changed, 212 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..70a7398c20e2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4368,6 +4368,61 @@
 
usbcore.nousb   [USB] Disable the USB subsystem
 
+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:
+   a = USB_QUIRK_STRING_FETCH_255 (string
+   descriptors must not be fetched using
+   a 255-byte read);
+   b = USB_QUIRK_RESET_RESUME (device can't resume
+   correctly so reset it instead);
+   c = USB_QUIRK_NO_SET_INTF (device can't handle
+   Set-Interface requests);
+   d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+   handle its Configuration or Interface
+   strings);
+   e = USB_QUIRK_RESET (device can't be reset
+   (e.g morph devices), don't use reset);
+   f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+   more interface descriptions than the
+   bNumInterfaces count, and can't handle
+   talking to these interfaces);
+   g = USB_QUIRK_DELAY_INIT (device needs a pause
+   during initialization, after we read
+   the device descriptor);
+   h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+   high speed and super speed interrupt
+   endpoints, the USB 2.0 and USB 3.0 spec
+   require the interval in microframes (1
+   microframe = 125 microseconds) to be
+   calculated as interval = 2 ^
+   (bInterval-1).
+   Devices with this quirk report their
+   bInterval as the result of this
+   calculation instead of the exponent
+   variable used in the calculation);
+   i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+   handle device_qualifier descriptor
+   requests);
+   j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+   generates spurious wakeup, ignore
+   remote wakeup capability);
+   k = USB_QUIRK_NO_LPM (device can't handle Link
+   Power Management);
+   l = USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL
+   (Device reports its bInterval as linear
+   frames instead of the USB

[PATCH 0/5 v3] Keep rtsx_usb suspended when there's no card

[Kai-Heng Feng]

Hi,

This is based on Ulf's work [1] [2].

This patch series can keep rtsx_usb suspended, to save ~0.5W on Intel
platforms and ~1.5W on AMD platforms.

[1] https://patchwork.kernel.org/patch/10440583/
[2] https://patchwork.kernel.org/patch/10445725/

Kai-Heng Feng (5):
  misc: rtsx_usb: Use USB remote wakeup signaling for card insertion
detection
  memstick: Prevent memstick host from getting runtime suspended during
card detection
  memstick: rtsx_usb_ms: Use ms_dev() helper
  memstick: rtsx_usb_ms: Support runtime power management
  misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before
system suspend
--
v3: Skip parent device check in rtsx_usb_resume_child() .
Remove dev_dbg() if it only prints function name.
Use ms_dev() helper at more places.
Remove const qualifier for UNIVERSAL_DEV_PM_OPS.

v2: Resend to linux-usb and LKML.

 drivers/memstick/core/memstick.c|   4 +
 drivers/memstick/host/rtsx_usb_ms.c | 148 +++-
 drivers/misc/cardreader/rtsx_usb.c  |   9 ++
 3 files changed, 91 insertions(+), 70 deletions(-)

-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper

[Kai-Heng Feng]

Use ms_dev() helper for consistency.

Signed-off-by: Kai-Heng Feng 
---
 drivers/memstick/host/rtsx_usb_ms.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/memstick/host/rtsx_usb_ms.c 
b/drivers/memstick/host/rtsx_usb_ms.c
index 4f64563df7de..cd12f3d1c088 100644
--- a/drivers/memstick/host/rtsx_usb_ms.c
+++ b/drivers/memstick/host/rtsx_usb_ms.c
@@ -785,7 +785,7 @@ static int rtsx_usb_ms_drv_remove(struct platform_device 
*pdev)
 
mutex_lock(>host_mutex);
if (host->req) {
-   dev_dbg(&(pdev->dev),
+   dev_dbg(ms_dev(host),
"%s: Controller removed during transfer\n",
dev_name(>dev));
host->req->error = -ENOMEDIUM;
@@ -807,10 +807,10 @@ static int rtsx_usb_ms_drv_remove(struct platform_device 
*pdev)
if (pm_runtime_active(ms_dev(host)))
pm_runtime_put(ms_dev(host));
 
-   pm_runtime_disable(>dev);
+   pm_runtime_disable(ms_dev(host));
platform_set_drvdata(pdev, NULL);
 
-   dev_dbg(&(pdev->dev),
+   dev_dbg(ms_dev(host),
": Realtek USB Memstick controller has been removed\n");
 
return 0;
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] memstick: Prevent memstick host from getting runtime suspended during card detection

[Kai-Heng Feng]

We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
helpers to let memstick host support runtime pm.

There's a small window between memstick_detect_change() and its queued
work, memstick_check(). In this window the rpm count may go down to zero
before the memstick host powers on, so the host can be inadvertently
suspended.

Increment rpm count before calling memstick_check(), and decrement rpm
count afterward, as now we are sure the memstick host should be
suspended or not.

Signed-off-by: Kai-Heng Feng 
---
 drivers/memstick/core/memstick.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index 76382c858c35..944fe0c93fa7 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define DRIVER_NAME "memstick"
 
@@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
  */
 void memstick_detect_change(struct memstick_host *host)
 {
+   pm_runtime_get_noresume(host->dev.parent);
queue_work(workqueue, >media_checker);
 }
 EXPORT_SYMBOL(memstick_detect_change);
@@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work)
host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF);
 
mutex_unlock(>lock);
+
+   pm_runtime_put_noidle(host->dev.parent);
dev_dbg(>dev, "memstick_check finished\n");
 }
 
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management

[Kai-Heng Feng]

In order to let host's parent device, rtsx_usb, to use USB remote wake
up signaling to do card detection, it needs to be suspended. Hence it's
necessary to add runtime PM support for the memstick host.

To keep memstick host stays suspended when it's not in use, convert the
card detection function from kthread to delayed_work, which can be
scheduled when the host is resumed and can be canceled when the host is
suspended.

Use an idle function check if there's no card and the power mode is
MEMSTICK_POWER_OFF. If both criteria are met, put the device to suspend.

Signed-off-by: Kai-Heng Feng 
---
 drivers/memstick/host/rtsx_usb_ms.c | 138 ++--
 1 file changed, 71 insertions(+), 67 deletions(-)

diff --git a/drivers/memstick/host/rtsx_usb_ms.c 
b/drivers/memstick/host/rtsx_usb_ms.c
index cd12f3d1c088..126eb310980a 100644
--- a/drivers/memstick/host/rtsx_usb_ms.c
+++ b/drivers/memstick/host/rtsx_usb_ms.c
@@ -40,15 +40,14 @@ struct rtsx_usb_ms {
 
struct mutexhost_mutex;
struct work_struct  handle_req;
-
-   struct task_struct  *detect_ms;
-   struct completion   detect_ms_exit;
+   struct delayed_work poll_card;
 
u8  ssc_depth;
unsigned intclock;
int power_mode;
unsigned char   ifmode;
booleject;
+   boolsuspend;
 };
 
 static inline struct device *ms_dev(struct rtsx_usb_ms *host)
@@ -524,7 +523,7 @@ static void rtsx_usb_ms_handle_req(struct work_struct *work)
int rc;
 
if (!host->req) {
-   pm_runtime_get_sync(ms_dev(host));
+   pm_runtime_get_noresume(ms_dev(host));
do {
rc = memstick_next_req(msh, >req);
dev_dbg(ms_dev(host), "next req %d\n", rc);
@@ -545,7 +544,7 @@ static void rtsx_usb_ms_handle_req(struct work_struct *work)
host->req->error);
}
} while (!rc);
-   pm_runtime_put(ms_dev(host));
+   pm_runtime_put_noidle(ms_dev(host));
}
 
 }
@@ -572,7 +571,7 @@ static int rtsx_usb_ms_set_param(struct memstick_host *msh,
dev_dbg(ms_dev(host), "%s: param = %d, value = %d\n",
__func__, param, value);
 
-   pm_runtime_get_sync(ms_dev(host));
+   pm_runtime_get_noresume(ms_dev(host));
mutex_lock(>dev_mutex);
 
err = rtsx_usb_card_exclusive_check(ucr, RTSX_USB_MS_CARD);
@@ -585,14 +584,14 @@ static int rtsx_usb_ms_set_param(struct memstick_host 
*msh,
break;
 
if (value == MEMSTICK_POWER_ON) {
-   pm_runtime_get_sync(ms_dev(host));
+   pm_runtime_get_noresume(ms_dev(host));
err = ms_power_on(host);
+   if (err)
+   pm_runtime_put_noidle(ms_dev(host));
} else if (value == MEMSTICK_POWER_OFF) {
err = ms_power_off(host);
-   if (host->msh->card)
+   if (!err)
pm_runtime_put_noidle(ms_dev(host));
-   else
-   pm_runtime_put(ms_dev(host));
} else
err = -EINVAL;
if (!err)
@@ -638,7 +637,7 @@ static int rtsx_usb_ms_set_param(struct memstick_host *msh,
}
 out:
mutex_unlock(>dev_mutex);
-   pm_runtime_put(ms_dev(host));
+   pm_runtime_put_noidle(ms_dev(host));
 
/* power-on delay */
if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON)
@@ -648,75 +647,86 @@ static int rtsx_usb_ms_set_param(struct memstick_host 
*msh,
return err;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int rtsx_usb_ms_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int rtsx_usb_ms_runtime_suspend(struct device *dev)
 {
struct rtsx_usb_ms *host = dev_get_drvdata(dev);
struct memstick_host *msh = host->msh;
 
-   dev_dbg(ms_dev(host), "--> %s\n", __func__);
-
+   host->suspend = true;
memstick_suspend_host(msh);
+
return 0;
 }
 
-static int rtsx_usb_ms_resume(struct device *dev)
+static int rtsx_usb_ms_runtime_resume(struct device *dev)
 {
struct rtsx_usb_ms *host = dev_get_drvdata(dev);
struct memstick_host *msh = host->msh;
 
-   dev_dbg(ms_dev(host), "--> %s\n", __func__);
-
memstick_resume_host(msh);
+   host->suspend = false;
+   schedule_delayed_work(>poll_card, 100);
+
return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
 
-/*
- * Thread function of ms card slot detection. The thread starts right after
- * successful host addition. It stops while the driver removal function 

[PATCH 5/5] misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend

[Kai-Heng Feng]

There's a long power-on delay at the end of rtsx_usb_ms_set_param().
This delay is noticeable right before system suspend.

To prevent already suspended memstick host from getting powered on by PM
core, use DPM_FLAG_SMART_SUSPEND to avoid the situation.

Signed-off-by: Kai-Heng Feng 
---
 drivers/memstick/host/rtsx_usb_ms.c | 4 
 drivers/misc/cardreader/rtsx_usb.c  | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/memstick/host/rtsx_usb_ms.c 
b/drivers/memstick/host/rtsx_usb_ms.c
index 126eb310980a..e3b635d1220f 100644
--- a/drivers/memstick/host/rtsx_usb_ms.c
+++ b/drivers/memstick/host/rtsx_usb_ms.c
@@ -763,6 +763,10 @@ static int rtsx_usb_ms_drv_probe(struct platform_device 
*pdev)
msh->set_param = rtsx_usb_ms_set_param;
msh->caps = MEMSTICK_CAP_PAR4;
 
+   /* DPM_FLAG_LEAVE_SUSPENDED is not needed, the parent device will wake
+* up memstick host.
+*/
+   dev_pm_set_driver_flags(ms_dev(host), DPM_FLAG_SMART_SUSPEND);
pm_runtime_set_active(ms_dev(host));
pm_runtime_enable(ms_dev(host));
 
diff --git a/drivers/misc/cardreader/rtsx_usb.c 
b/drivers/misc/cardreader/rtsx_usb.c
index f7a66f614085..98bb878a6ade 100644
--- a/drivers/misc/cardreader/rtsx_usb.c
+++ b/drivers/misc/cardreader/rtsx_usb.c
@@ -671,6 +671,7 @@ static int rtsx_usb_probe(struct usb_interface *intf,
goto out_init_fail;
 
 #ifdef CONFIG_PM
+   dev_pm_set_driver_flags(>dev, DPM_FLAG_SMART_SUSPEND | 
DPM_FLAG_LEAVE_SUSPENDED);
intf->needs_remote_wakeup = 1;
usb_enable_autosuspend(usb_dev);
 #endif
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection

[Kai-Heng Feng]

Although rtsx_usb doesn't support card removal detection, card insertion
will resume rtsx_usb by USB remote wakeup signaling.

When rtsx_usb gets resumed, also resumes its child devices,
rtsx_usb_sdmmc and rtsx_usb_ms, to notify them there's a card in its
slot.

Signed-off-by: Kai-Heng Feng 
---
 drivers/misc/cardreader/rtsx_usb.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/misc/cardreader/rtsx_usb.c 
b/drivers/misc/cardreader/rtsx_usb.c
index b97903ff1a72..f7a66f614085 100644
--- a/drivers/misc/cardreader/rtsx_usb.c
+++ b/drivers/misc/cardreader/rtsx_usb.c
@@ -723,8 +723,15 @@ static int rtsx_usb_suspend(struct usb_interface *intf, 
pm_message_t message)
return 0;
 }
 
+static int rtsx_usb_resume_child(struct device *dev, void *data)
+{
+   pm_request_resume(dev);
+   return 0;
+}
+
 static int rtsx_usb_resume(struct usb_interface *intf)
 {
+   device_for_each_child(>dev, NULL, rtsx_usb_resume_child);
return 0;
 }
 
@@ -734,6 +741,7 @@ static int rtsx_usb_reset_resume(struct usb_interface *intf)
(struct rtsx_ucr *)usb_get_intfdata(intf);
 
rtsx_usb_reset_chip(ucr);
+   device_for_each_child(>dev, NULL, rtsx_usb_resume_child);
return 0;
 }
 
-- 
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [1/5] xhci: Fix perceived dead host due to runtime suspend race with event handler

[Kai-Heng Feng]

Hi Mathias,

at 21:19, Mathias Nyman  wrote:


Don't rely on event interrupt (EINT) bit alone to detect pending port
change in resume. If no change event is detected the host may be suspended
again, oterwise roothubs are resumed.

There is a lag in xHC setting EINT. If we don't notice the pending change
in resume, and the controller is runtime suspeded again, it causes the
event handler to assume host is dead as it will fail to read xHC registers
once PCI puts the controller to D3 state.

[  268.520969] xhci_hcd: xhci_resume: starting port polling.
[  268.520985] xhci_hcd: xhci_hub_status_data: stopping port polling.
[  268.521030] xhci_hcd: xhci_suspend: stopping port polling.
[  268.521040] xhci_hcd: // Setting command ring address to 0x349bd001
[  268.521139] xhci_hcd: Port Status Change Event for port 3
[  268.521149] xhci_hcd: resume root hub
[  268.521163] xhci_hcd: port resume event for port 3
[  268.521168] xhci_hcd: xHC is not running.
[  268.521174] xhci_hcd: handle_port_status: starting port polling.
[  268.596322] xhci_hcd: xhci_hc_died: xHCI host controller not  
responding, assume dead


The EINT lag is described in a additional note in xhci specs 4.19.2:

"Due to internal xHC scheduling and system delays, there will be a lag
between a change bit being set and the Port Status Change Event that it
generated being written to the Event Ring. If SW reads the PORTSC and
sees a change bit set, there is no guarantee that the corresponding Port
Status Change Event has already been written into the Event Ring."

Cc: 



I tried to backport this patch to v4.15, and "xhci: Create new structures  
to store xhci port information" series is a dependency for this patch.


The series brings substantial changes, so I am wondering if you have any  
plan to backport this patch to older kernels?


Kai-Heng



Signed-off-by: Mathias Nyman 



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xhci: Fix front USB ports on ASUS PRIME B350M-A

[Kai Heng Feng]

Hi Matthias,

Do you have any concern about this patch?

Hopefully this can get merged for v4.16…

Kai-Heng
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release. Also, the quirk parameter can XOR the builtin
quirks for debugging purpose.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v5: Use module_param_cb() to get notified when parameter changes.
Replace linked list with array.

v4: Use kmalloc() to reduce memory usage.
Remove tolower() so we can use capital character in the future.

v3: Stop overridding static quirks.
Use XOR for dynamic quirks.
Save parsed quirk as a list to avoid parsing quirk table everytime.

v2: Use in-kernel tolower() function.

 Documentation/admin-guide/kernel-parameters.txt |  55 
 drivers/usb/core/quirks.c   | 177 +++-
 drivers/usb/core/usb.c  |   1 +
 drivers/usb/core/usb.h  |   1 +
 4 files changed, 229 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..70a7398c20e2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4368,6 +4368,61 @@
 
usbcore.nousb   [USB] Disable the USB subsystem
 
+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:
+   a = USB_QUIRK_STRING_FETCH_255 (string
+   descriptors must not be fetched using
+   a 255-byte read);
+   b = USB_QUIRK_RESET_RESUME (device can't resume
+   correctly so reset it instead);
+   c = USB_QUIRK_NO_SET_INTF (device can't handle
+   Set-Interface requests);
+   d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+   handle its Configuration or Interface
+   strings);
+   e = USB_QUIRK_RESET (device can't be reset
+   (e.g morph devices), don't use reset);
+   f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+   more interface descriptions than the
+   bNumInterfaces count, and can't handle
+   talking to these interfaces);
+   g = USB_QUIRK_DELAY_INIT (device needs a pause
+   during initialization, after we read
+   the device descriptor);
+   h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+   high speed and super speed interrupt
+   endpoints, the USB 2.0 and USB 3.0 spec
+   require the interval in microframes (1
+   microframe = 125 microseconds) to be
+   calculated as interval = 2 ^
+   (bInterval-1).
+   Devices with this quirk report their
+   bInterval as the result of this
+   calculation instead of the exponent
+   variable used in the calculation);
+   i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+   handle device_qualifier descriptor
+   requests);
+   j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+   generates spurious wakeup, ignore
+   remote wakeup capability);
+   k = USB_QUIRK_NO_LPM (device can't handle Link
+   Power Management);
+

[PATCH v6] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release. Also, the quirk parameter can XOR the builtin
quirks for debugging purpose.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v6: Add missed header file .

v5: Use module_param_cb() to get notified when parameter changes.
Replace linked list with array.

v4: Use kmalloc() to reduce memory usage.
Remove tolower() so we can use capital character in the future.

v3: Stop overridding static quirks.
Use XOR for dynamic quirks.
Save parsed quirk as a list to avoid parsing quirk table everytime.

v2: Use in-kernel tolower() function.
 Documentation/admin-guide/kernel-parameters.txt |  55 
 drivers/usb/core/quirks.c   | 178 +++-
 drivers/usb/core/usb.c  |   1 +
 drivers/usb/core/usb.h  |   1 +
 4 files changed, 230 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..70a7398c20e2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4368,6 +4368,61 @@
 
usbcore.nousb   [USB] Disable the USB subsystem
 
+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:
+   a = USB_QUIRK_STRING_FETCH_255 (string
+   descriptors must not be fetched using
+   a 255-byte read);
+   b = USB_QUIRK_RESET_RESUME (device can't resume
+   correctly so reset it instead);
+   c = USB_QUIRK_NO_SET_INTF (device can't handle
+   Set-Interface requests);
+   d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+   handle its Configuration or Interface
+   strings);
+   e = USB_QUIRK_RESET (device can't be reset
+   (e.g morph devices), don't use reset);
+   f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+   more interface descriptions than the
+   bNumInterfaces count, and can't handle
+   talking to these interfaces);
+   g = USB_QUIRK_DELAY_INIT (device needs a pause
+   during initialization, after we read
+   the device descriptor);
+   h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+   high speed and super speed interrupt
+   endpoints, the USB 2.0 and USB 3.0 spec
+   require the interval in microframes (1
+   microframe = 125 microseconds) to be
+   calculated as interval = 2 ^
+   (bInterval-1).
+   Devices with this quirk report their
+   bInterval as the result of this
+   calculation instead of the exponent
+   variable used in the calculation);
+   i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+   handle device_qualifier descriptor
+   requests);
+   j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+   generates spurious wakeup, ignore
+   remote wakeup capability);
+   k = USB_QUIRK_NO_LPM (device can't handle Link
+   

Re: [PATCH 4/4] xhci: Fix front USB ports on ASUS PRIME B350M-A

[Kai Heng Feng]

Hi,


On Mar 9, 2018, at 1:06 AM, Greg KH <gre...@linuxfoundation.org> wrote:

On Thu, Mar 08, 2018 at 05:17:17PM +0200, Mathias Nyman wrote:

From: Kai-Heng Feng <kai.heng.f...@canonical.com>

When a USB device gets plugged on ASUS PRIME B350M-A's front ports, the
xHC stops working:
[  549.114587] xhci_hcd :02:00.0: WARN: xHC CMD_RUN timeout
[  549.114608] suspend_common(): xhci_pci_suspend+0x0/0xc0 returns -110
[  549.114638] xhci_hcd :02:00.0: can't suspend  
(hcd_pci_runtime_suspend returned -110)


Delay before running xHC command CMD_RUN can workaround the issue.

Use a new quirk to make the delay only targets to the affected xHC.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
Signed-off-by: Mathias Nyman <mathias.ny...@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 3 +++
 drivers/usb/host/xhci.c | 3 +++
 drivers/usb/host/xhci.h | 1 +
 3 files changed, 7 insertions(+)


Any reason this shouldn't go to the stable kernel trees?


It would be really nice to have this in stable tree.

Kai-Heng



thanks,

greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: core: Add "quirks" parameter for usbcore

[Kai Heng Feng]

Matthew Wilcox <wi...@infradead.org> wrote:


On Tue, Mar 13, 2018 at 03:26:19PM +0800, Kai-Heng Feng wrote:

+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:


This doesn't really tell me that the specified quirks will be XORed with
the built-in quirks.  Maybe something like ...

[USB] A list of quirk entries to augment the
built-in usb core quirk list.  List entries are
separated by commas.  Each entry has the form
VendorID:ProductID:Flags.  The IDs are 4-digit hex
numbers and Flags is a set of letters.  Each letter
will change the built-in quirk; setting it if it is
clear and clearing it if it is set.  The letters
have the following meanings:


Thanks, will update the description to specify the XOR behavior.




+   /* Each entry consists of VID:PID:flags */
+   field = strsep(, ":");
+   if (!field)
+   break;
+
+   if (kstrtou16(field, 16, ))
+   break;
+
+   field = strsep(, ":");
+   if (!field)
+   break;
+
+   if (kstrtou16(field, 16, ))
+   break;


Is there a reason to not use sscanf here like hid-quirks.c does?


Actually I use sscanf in previous patch version. Using kstrto* isn't needed  
anymore, I'll use sscanf in next version.


Kai-Heng


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xhci: Fix USB ports for Dell Inspiron 5775

[Kai Heng Feng]

Hi Matthias,

On Mar 18, 2018, at 11:11 PM, Kai-Heng Feng <kai.heng.f...@canonical.com>  
wrote:


The Dell Inspiron 5775 is a Raven Ridge. The Enable Slot command timed
out when a USB device gets plugged:
[ 212.156326] xhci_hcd :03:00.3: Error while assigning device slot ID
[ 212.156340] xhci_hcd :03:00.3: Max number of devices this xHCI host  
supports is 64.

[ 212.156348] usb usb2-port3: couldn't allocate usb_device

AMD suggests that a delay before xHC suspends can fix the issue.

I can confirm it fixes the issue, so use the suspend delay quirk for
Raven Ridge's xHC.


I am hoping this patch can get merged in v4.17...

Thanks,
Kai-Heng



Cc: sta...@vger.kernel.org
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/host/xhci-pci.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index d9f831b67e57..93ce34bce7b5 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -126,7 +126,10 @@ static void xhci_pci_quirks(struct device *dev,  
struct xhci_hcd *xhci)

if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_find_chipset_info())
xhci->quirks |= XHCI_AMD_PLL_FIX;

-   if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x43bb)
+   if (pdev->vendor == PCI_VENDOR_ID_AMD &&
+   (pdev->device == 0x15e0 ||
+pdev->device == 0x15e1 ||
+pdev->device == 0x43bb))
xhci->quirks |= XHCI_SUSPEND_DELAY;

if (pdev->vendor == PCI_VENDOR_ID_AMD)
--
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] xhci: Fix USB ports for Dell Inspiron 5775

[Kai-Heng Feng]

The Dell Inspiron 5775 is a Raven Ridge. The Enable Slot command timed
out when a USB device gets plugged:
[ 212.156326] xhci_hcd :03:00.3: Error while assigning device slot ID
[ 212.156340] xhci_hcd :03:00.3: Max number of devices this xHCI host 
supports is 64.
[ 212.156348] usb usb2-port3: couldn't allocate usb_device

AMD suggests that a delay before xHC suspends can fix the issue.

I can confirm it fixes the issue, so use the suspend delay quirk for
Raven Ridge's xHC.

Cc: sta...@vger.kernel.org
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/host/xhci-pci.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index d9f831b67e57..93ce34bce7b5 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -126,7 +126,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_find_chipset_info())
xhci->quirks |= XHCI_AMD_PLL_FIX;
 
-   if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x43bb)
+   if (pdev->vendor == PCI_VENDOR_ID_AMD &&
+   (pdev->device == 0x15e0 ||
+pdev->device == 0x15e1 ||
+pdev->device == 0x43bb))
xhci->quirks |= XHCI_SUSPEND_DELAY;
 
if (pdev->vendor == PCI_VENDOR_ID_AMD)
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release. Also, the quirk parameter can XOR the builtin
quirks for debugging purpose.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v7: Update documentation to specify the quirks parameter can XOR the
built-in quirks.

v6: Add missed header file .

v5: Use module_param_cb() to get notified when parameter changes.
Replace linked list with array.

v4: Use kmalloc() to reduce memory usage.
Remove tolower() so we can use capital character in the future.

v3: Stop overridding static quirks.
Use XOR for dynamic quirks.
Save parsed quirk as a list to avoid parsing quirk table everytime.

v2: Use in-kernel tolower() function.

 Documentation/admin-guide/kernel-parameters.txt |  56 
 drivers/usb/core/quirks.c   | 178 +++-
 drivers/usb/core/usb.c  |   1 +
 drivers/usb/core/usb.h  |   1 +
 4 files changed, 231 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..e00cdd313dc2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4368,6 +4368,62 @@
 
usbcore.nousb   [USB] Disable the USB subsystem
 
+   usbcore.quirks=
+   [USB] A list of quirk entries to augment the built-in
+   usb core quirk list. List entries are separated by
+   commas. Each entry has the form
+   VendorID:ProductID:Flags. The IDs are 4-digit hex
+   numbers and Flags is a set of letters. Each letter
+   will change the built-in quirk; setting it if it is
+   clear and clearing it if it is set. The letters have
+   the following meanings:
+   a = USB_QUIRK_STRING_FETCH_255 (string
+   descriptors must not be fetched using
+   a 255-byte read);
+   b = USB_QUIRK_RESET_RESUME (device can't resume
+   correctly so reset it instead);
+   c = USB_QUIRK_NO_SET_INTF (device can't handle
+   Set-Interface requests);
+   d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+   handle its Configuration or Interface
+   strings);
+   e = USB_QUIRK_RESET (device can't be reset
+   (e.g morph devices), don't use reset);
+   f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+   more interface descriptions than the
+   bNumInterfaces count, and can't handle
+   talking to these interfaces);
+   g = USB_QUIRK_DELAY_INIT (device needs a pause
+   during initialization, after we read
+   the device descriptor);
+   h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+   high speed and super speed interrupt
+   endpoints, the USB 2.0 and USB 3.0 spec
+   require the interval in microframes (1
+   microframe = 125 microseconds) to be
+   calculated as interval = 2 ^
+   (bInterval-1).
+   Devices with this quirk report their
+   bInterval as the result of this
+   calculation instead of the exponent
+   variable used in the calculation);
+   i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+   handle device_qualifier descriptor
+   requests);
+   j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+   generates spurious wakeup, ignore
+   remo

Re: [PATCH v6] usb: core: Add "quirks" parameter for usbcore

[Kai Heng Feng]

Kai Heng Feng <kai.heng.f...@canonical.com> wrote:


Matthew Wilcox <wi...@infradead.org> wrote:


On Tue, Mar 13, 2018 at 03:26:19PM +0800, Kai-Heng Feng wrote:

+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:


This doesn't really tell me that the specified quirks will be XORed with
the built-in quirks.  Maybe something like ...

[USB] A list of quirk entries to augment the
built-in usb core quirk list.  List entries are
separated by commas.  Each entry has the form
VendorID:ProductID:Flags.  The IDs are 4-digit hex
numbers and Flags is a set of letters.  Each letter
will change the built-in quirk; setting it if it is
clear and clearing it if it is set.  The letters
have the following meanings:


Thanks, will update the description to specify the XOR behavior.


+   /* Each entry consists of VID:PID:flags */
+   field = strsep(, ":");
+   if (!field)
+   break;
+
+   if (kstrtou16(field, 16, ))
+   break;
+
+   field = strsep(, ":");
+   if (!field)
+   break;
+
+   if (kstrtou16(field, 16, ))
+   break;


Is there a reason to not use sscanf here like hid-quirks.c does?


Actually I use sscanf in previous patch version. Using kstrto* isn't  
needed anymore, I'll use sscanf in next version.


Okay, I remember why I don't use sscanf, mainly because the quirk_param in  
hid-quirks.c is charp instead of char, so using sscanf is trivia for it.


We use plain char* here because of module_param_cb(), so I guess using  
sscanf doesn't make much sense here. strsep() isn't the most elegant  
solution but it does its job well.


I'll update the description and send a new patch.

Kai-Heng



Kai-Heng



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Hi Dan,

Dan Carpenter <dan.carpen...@oracle.com> wrote:


Hello Kai-Heng Feng,

This is a semi-automatic email about new static checker warnings.


I ran Smatch but didn't see the error message:
$ make -j`nproc` CHECK="~/smatch/smatch -p=kernel" C=1 bzImage modules |  
tee warns.txt`"


Also, "val" will never get passed as null in quirks_param_set() by the  
.set() caller, so it's actually superfluous.


Kai-Heng



The patch 027bd6cafd9a: "usb: core: Add "quirks" parameter for
usbcore" from Mar 20, 2018, leads to the following Smatch complaint:

drivers/usb/core/quirks.c:136 quirks_param_set()
error: we previously assumed 'val' could be null (see line 37)

drivers/usb/core/quirks.c
36  
37  if (!val || !*val) {

Patch adds check for NULL

38  quirk_count = 0;
39  kfree(quirk_list);
40  quirk_list = NULL;
41  goto unlock;
42  }
43  
44  for (quirk_count = 1, i = 0; val[i]; i++)
45  if (val[i] == ',')
46  quirk_count++;
47  
48  if (quirk_list) {
49  kfree(quirk_list);
50  quirk_list = NULL;
51  }
52  
53  quirk_list = kcalloc(quirk_count, sizeof(struct quirk_entry),
54   GFP_KERNEL);
55  if (!quirk_list) {
56  mutex_unlock(_mutex);
57  return -ENOMEM;
58  }
59  
60  for (i = 0, p = (char *)val; p && *p;) {
61  /* Each entry consists of VID:PID:flags */
62  field = strsep(, ":");
63  if (!field)
64  break;
65  
66  if (kstrtou16(field, 16, ))
67  break;
68  
69  field = strsep(, ":");
70  if (!field)
71  break;
72  
73  if (kstrtou16(field, 16, ))
74  break;
75  
76  field = strsep(, ",");
77  if (!field || !*field)
78  break;
79  
80  /* Collect the flags */
81  for (flags = 0; *field; field++) {
82  switch (*field) {
83  case 'a':
84  flags |= USB_QUIRK_STRING_FETCH_255;
85  break;
86  case 'b':
87  flags |= USB_QUIRK_RESET_RESUME;
88  break;
89  case 'c':
90  flags |= USB_QUIRK_NO_SET_INTF;
91  break;
92  case 'd':
93  flags |= USB_QUIRK_CONFIG_INTF_STRINGS;
94  break;
95  case 'e':
96  flags |= USB_QUIRK_RESET;
97  break;
98  case 'f':
99  flags |= USB_QUIRK_HONOR_BNUMINTERFACES;
   100  break;
   101  case 'g':
   102  flags |= USB_QUIRK_DELAY_INIT;
   103  break;
   104  case 'h':
   105  flags |= 
USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL;
   106  break;
   107  case 'i':
   108  flags |= USB_QUIRK_DEVICE_QUALIFIER;
   109  break;
   110  case 'j':
   111  flags |= USB_QUIRK_IGNORE_REMOTE_WAKEUP;
   112  break;
   113  case 'k':
   114  flags |= USB_QUIRK_NO_LPM;
   115  break;
   116  case 'l':
   117  flags |= 
USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL;
   118  break;
   119  case 'm':
   120  flags |= USB_QUIRK_DISCONNECT_SUSPEND;
   121  break;
   122  /* Ignore unrecognized flag characters */
   123  }
   124  }
   125  
   126  quirk_list[i++] = (struct quirk_entry)
   127  { .vid = vid, .pid = pid, .flags = flags };
   128

[PATCH 2/2] usb: core: Add USB_QUIRK_DELAY_CTRL_MSG to usbcore quirks

[Kai-Heng Feng]

There's a new quirk, USB_QUIRK_DELAY_CTRL_MSG. Add it to usbcore quirks
for completeness.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 4 +++-
 drivers/usb/core/quirks.c   | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 61ab660f7fdc..a0b4377cb8da 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4454,7 +4454,9 @@
calculation);
m = USB_QUIRK_DISCONNECT_SUSPEND (Device needs
to be disconnected before suspend to
-   prevent spurious wakeup)
+   prevent spurious wakeup);
+   n = USB_QUIRK_DELAY_CTRL_MSG (Device needs a
+   pause after every control message);
Example: quirks=0781:5580:bk,0a5c:5834:gij
 
usbhid.mousepoll=
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 29e5f32b38df..920f48a49a87 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -124,6 +124,9 @@ static int quirks_param_set(const char *val, const struct 
kernel_param *kp)
case 'm':
flags |= USB_QUIRK_DISCONNECT_SUSPEND;
break;
+   case 'n':
+   flags |= USB_QUIRK_DELAY_CTRL_MSG;
+   break;
/* Ignore unrecognized flag characters */
}
}
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] usb: core: Copy parameter string correctly and remove superfluous null check

[Kai-Heng Feng]

strsep() slices string, so the string gets copied by
param_set_copystring() at the end of quirks_param_set() is not the
original value.
Fix that by calling param_set_copystring() earlier.

The null check for val is unnecessary, the caller of quirks_param_set()
does not pass null string.
Remove the superfluous null check. This is found by Smatch.

Fixes: 027bd6cafd9a ("usb: core: Add "quirks" parameter for usbcore")
Cc: Dan Carpenter <dan.carpen...@oracle.com>
Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
 drivers/usb/core/quirks.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 6fb8d5433268..29e5f32b38df 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -31,10 +31,15 @@ static int quirks_param_set(const char *val, const struct 
kernel_param *kp)
u16 vid, pid;
u32 flags;
size_t i;
+   int err;
+
+   err = param_set_copystring(val, kp);
+   if (err)
+   return err;
 
mutex_lock(_mutex);
 
-   if (!val || !*val) {
+   if (!*val) {
quirk_count = 0;
kfree(quirk_list);
quirk_list = NULL;
@@ -133,7 +138,7 @@ static int quirks_param_set(const char *val, const struct 
kernel_param *kp)
 unlock:
mutex_unlock(_mutex);
 
-   return param_set_copystring(val, kp);
+   return 0;
 }
 
 static const struct kernel_param_ops quirks_param_ops = {
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release. Also, the quirk parameter can XOR the builtin
quirks for debugging purpose.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <kai.heng.f...@canonical.com>
---
v4: Use kmalloc() to reduce memory usage.
Remove tolower() so we can use capital character in the future.

v3: Stop overridding static quirks.
Use XOR for dynamic quirks.
Save parsed quirk as a list to avoid parsing quirk table everytime.

v2: Use in-kernel tolower() function.

 Documentation/admin-guide/kernel-parameters.txt |  55 
 drivers/usb/core/quirks.c   | 160 +++-
 drivers/usb/core/usb.c  |   1 +
 drivers/usb/core/usb.h  |   1 +
 4 files changed, 212 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..70a7398c20e2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4368,6 +4368,61 @@
 
usbcore.nousb   [USB] Disable the USB subsystem
 
+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:
+   a = USB_QUIRK_STRING_FETCH_255 (string
+   descriptors must not be fetched using
+   a 255-byte read);
+   b = USB_QUIRK_RESET_RESUME (device can't resume
+   correctly so reset it instead);
+   c = USB_QUIRK_NO_SET_INTF (device can't handle
+   Set-Interface requests);
+   d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+   handle its Configuration or Interface
+   strings);
+   e = USB_QUIRK_RESET (device can't be reset
+   (e.g morph devices), don't use reset);
+   f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+   more interface descriptions than the
+   bNumInterfaces count, and can't handle
+   talking to these interfaces);
+   g = USB_QUIRK_DELAY_INIT (device needs a pause
+   during initialization, after we read
+   the device descriptor);
+   h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+   high speed and super speed interrupt
+   endpoints, the USB 2.0 and USB 3.0 spec
+   require the interval in microframes (1
+   microframe = 125 microseconds) to be
+   calculated as interval = 2 ^
+   (bInterval-1).
+   Devices with this quirk report their
+   bInterval as the result of this
+   calculation instead of the exponent
+   variable used in the calculation);
+   i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+   handle device_qualifier descriptor
+   requests);
+   j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+   generates spurious wakeup, ignore
+   remote wakeup capability);
+   k = USB_QUIRK_NO_LPM (device can't handle Link
+   Power Management);
+   l = USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL
+  

Re: [PATCH v3] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

On Sun, Feb 25, 2018 at 11:18 PM, Matthew Wilcox <wi...@infradead.org> wrote:
> On Sun, Feb 25, 2018 at 08:38:33PM +0800, Kai-Heng Feng wrote:
>> v2: Use in-kernel tolower() function.
>
> ... why are you using tolower at all?
>
> You've got 13 quirks already; you may need to use upper case as well
> before too long.

Makes sense. I'll remove tolower() in next version.

>
>> + quirk = vmalloc(sizeof(struct quirk_entry));
>
> vmalloc?  For a struct that's 24 bytes?  You know that allocates an
> entire 4k page, right?
>

Right, I'll use kmalloc() instead. Or is there any more efficient
malloc function?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] usb: core: Add "quirks" parameter for usbcore

[Kai Heng Feng]

On 28 Feb 2018, at 10:47 PM, Matthew Wilcox <wi...@infradead.org> wrote:

On Mon, Feb 26, 2018 at 11:04:57PM +0800, Kai-Heng Feng wrote:

+static char quirks_param[128];
+module_param_string(quirks, quirks_param, sizeof(quirks_param), 0644);
+MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying  
quirks=vendorID:productID:quirks");

+
+static char quirks_param_orig[128];



+static u32 usb_detect_dynamic_quirks(struct usb_device *udev)
+{
+   u16 vid = le16_to_cpu(udev->descriptor.idVendor);
+   u16 pid = le16_to_cpu(udev->descriptor.idProduct);
+   struct quirk_entry *quirk;
+
+   mutex_lock(_mutex);
+   if (strcmp(quirks_param, quirks_param_orig) != 0) {
+   strcpy(quirks_param_orig, quirks_param);


What happens if the user is writing to quirks_param at the same time
that you memcpy it?

I think you're going about this wrong by trying to use the
module_param_string machinery.  You should be using module_param_cb()
to build the quirks list when the user writes it (and then translate
back into a string when the user wants to read from it.


Thanks! module_param_cb() is exactly what I want.
I’ll use it in next version.


Also, you won't need to use a linked list for this; you can just allocate
an array of quirks.



I use linked list because the total quirks number is known after the entire  
string gets parsed.
Do you suggest that I should just alloc a predefined number (like 16) quirk  
entries, instead of doing it dynamically?


Kai-Heng
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html