diff mbox series

[v3] USB: hub: Fix the broken detection of USB3 device in SMSC hub

Message ID 1580838253-31822-1-git-send-email-hgajjar@de.adit-jv.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series [v3] USB: hub: Fix the broken detection of USB3 device in SMSC hub | expand

Commit Message

Hardik Gajjar Feb. 4, 2020, 5:44 p.m. UTC 
Renesas R-Car H3ULCB + Kingfisher Infotainment Board is either not able
to detect the USB3.0 mass storage devices or is detecting those as
USB2.0 high speed devices.

The explanation given by Renesas is that, due to a HW issue, the XHCI
driver does not wake up after going to sleep on connecting a USB3.0
device.

In order to mitigate that, disable the auto-suspend feature
specifically for SMSC hubs from hub_probe() function, as a quirk.

Renesas Kingfisher Infotainment Board has two USB3.0 ports (CN2) which
are connected via USB5534B 4-port SuperSpeed/Hi-Speed, low-power,
configurable hub controller.

[1] SanDisk USB 3.0 device detected as USB-2.0 before the patch
 [   74.036390] usb 5-1.1: new high-speed USB device number 4 using xhci-hcd
 [   74.061598] usb 5-1.1: New USB device found, idVendor=0781, idProduct=5581, bcdDevice= 1.00
 [   74.069976] usb 5-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
 [   74.077303] usb 5-1.1: Product: Ultra
 [   74.080980] usb 5-1.1: Manufacturer: SanDisk
 [   74.085263] usb 5-1.1: SerialNumber: 4C530001110208116550

[2] SanDisk USB 3.0 device detected as USB-3.0 after the patch
 [   34.565078] usb 6-1.1: new SuperSpeed Gen 1 USB device number 3 using xhci-hcd
 [   34.588719] usb 6-1.1: New USB device found, idVendor=0781, idProduct=5581, bcdDevice= 1.00
 [   34.597098] usb 6-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
 [   34.604430] usb 6-1.1: Product: Ultra
 [   34.608110] usb 6-1.1: Manufacturer: SanDisk
 [   34.612397] usb 6-1.1: SerialNumber: 4C530001110208116550

Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>
---
Changes in v2:
 - [Alan Stern] Switched from pm_runtime_set_autosuspend_delay()
   to usb_autopm_get_interface()
 - Improved commit description
 - Rebased against v5.5
 - https://lore.kernel.org/linux-renesas-soc/1579876573-13741-1-git-send-email-hgajjar@de.adit-jv.com/

Changes in v3:
 - [Alan Stern] Called usb_autopm_put_interface() from
   hub_disconnect() to enable auto suspend for interface.
 - https://lore.kernel.org/linux-renesas-soc/1580403994-21076-1-git-send-email-hgajjar@de.adit-jv.com/

 drivers/usb/core/hub.c | 15 +++++++++++++++
 drivers/usb/core/hub.h |  1 +
 2 files changed, 16 insertions(+)

Comments

Alan Stern Feb. 4, 2020, 6:10 p.m. UTC | #1 
On Tue, 4 Feb 2020, Hardik Gajjar wrote:

> Renesas R-Car H3ULCB + Kingfisher Infotainment Board is either not able
> to detect the USB3.0 mass storage devices or is detecting those as
> USB2.0 high speed devices.
> 
> The explanation given by Renesas is that, due to a HW issue, the XHCI
> driver does not wake up after going to sleep on connecting a USB3.0
> device.
> 
> In order to mitigate that, disable the auto-suspend feature
> specifically for SMSC hubs from hub_probe() function, as a quirk.
> 
> Renesas Kingfisher Infotainment Board has two USB3.0 ports (CN2) which
> are connected via USB5534B 4-port SuperSpeed/Hi-Speed, low-power,
> configurable hub controller.
> 
> [1] SanDisk USB 3.0 device detected as USB-2.0 before the patch
>  [   74.036390] usb 5-1.1: new high-speed USB device number 4 using xhci-hcd
>  [   74.061598] usb 5-1.1: New USB device found, idVendor=0781, idProduct=5581, bcdDevice= 1.00
>  [   74.069976] usb 5-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>  [   74.077303] usb 5-1.1: Product: Ultra
>  [   74.080980] usb 5-1.1: Manufacturer: SanDisk
>  [   74.085263] usb 5-1.1: SerialNumber: 4C530001110208116550
> 
> [2] SanDisk USB 3.0 device detected as USB-3.0 after the patch
>  [   34.565078] usb 6-1.1: new SuperSpeed Gen 1 USB device number 3 using xhci-hcd
>  [   34.588719] usb 6-1.1: New USB device found, idVendor=0781, idProduct=5581, bcdDevice= 1.00
>  [   34.597098] usb 6-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>  [   34.604430] usb 6-1.1: Product: Ultra
>  [   34.608110] usb 6-1.1: Manufacturer: SanDisk
>  [   34.612397] usb 6-1.1: SerialNumber: 4C530001110208116550
> 
> Signed-off-by: Hardik Gajjar <hgajjar@de.adit-jv.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>

> ---
> Changes in v2:
>  - [Alan Stern] Switched from pm_runtime_set_autosuspend_delay()
>    to usb_autopm_get_interface()
>  - Improved commit description
>  - Rebased against v5.5
>  - https://lore.kernel.org/linux-renesas-soc/1579876573-13741-1-git-send-email-hgajjar@de.adit-jv.com/
> 
> Changes in v3:
>  - [Alan Stern] Called usb_autopm_put_interface() from
>    hub_disconnect() to enable auto suspend for interface.
>  - https://lore.kernel.org/linux-renesas-soc/1580403994-21076-1-git-send-email-hgajjar@de.adit-jv.com/
> 
>  drivers/usb/core/hub.c | 15 +++++++++++++++
>  drivers/usb/core/hub.h |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3405b14..1c74485 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -38,7 +38,9 @@
>  #include "otg_whitelist.h"
>  
>  #define USB_VENDOR_GENESYS_LOGIC		0x05e3
> +#define USB_VENDOR_SMSC				0x0424
>  #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
> +#define HUB_QUIRK_DISABLE_AUTOSUSPEND		0x02
>  
>  #define USB_TP_TRANSMISSION_DELAY	40	/* ns */
>  #define USB_TP_TRANSMISSION_DELAY_MAX	65535	/* ns */
> @@ -1731,6 +1733,10 @@ static void hub_disconnect(struct usb_interface *intf)
>  	kfree(hub->buffer);
>  
>  	pm_suspend_ignore_children(&intf->dev, false);
> +
> +	if (hub->quirk_disable_autosuspend)
> +		usb_autopm_put_interface(intf);
> +
>  	kref_put(&hub->kref, hub_release);
>  }
>  
> @@ -1863,6 +1869,11 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND)
>  		hub->quirk_check_port_auto_suspend = 1;
>  
> +	if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) {
> +		hub->quirk_disable_autosuspend = 1;
> +		usb_autopm_get_interface(intf);
> +	}
> +
>  	if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
>  		return 0;
>  
> @@ -5599,6 +5610,10 @@ static void hub_event(struct work_struct *work)
>  }
>  
>  static const struct usb_device_id hub_id_table[] = {
> +    { .match_flags = USB_DEVICE_ID_MATCH_VENDOR | USB_DEVICE_ID_MATCH_INT_CLASS,
> +      .idVendor = USB_VENDOR_SMSC,
> +      .bInterfaceClass = USB_CLASS_HUB,
> +      .driver_info = HUB_QUIRK_DISABLE_AUTOSUSPEND},
>      { .match_flags = USB_DEVICE_ID_MATCH_VENDOR
>  			| USB_DEVICE_ID_MATCH_INT_CLASS,
>        .idVendor = USB_VENDOR_GENESYS_LOGIC,
> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index a9e24e4..2fe9c9f 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -61,6 +61,7 @@ struct usb_hub {
>  	unsigned		quiescing:1;
>  	unsigned		disconnected:1;
>  	unsigned		in_reset:1;
> +	unsigned                quirk_disable_autosuspend:1;
>  
>  	unsigned		quirk_check_port_auto_suspend:1;
>  
>
Eugeniu Rosca Feb. 6, 2020, 11:06 a.m. UTC | #2 
Hi Hardik,

On Tue, Feb 04, 2020 at 06:44:13PM +0100, Hardik Gajjar wrote:
> Renesas R-Car H3ULCB + Kingfisher Infotainment Board is either not able
> to detect the USB3.0 mass storage devices or is detecting those as
> USB2.0 high speed devices.
> 
> The explanation given by Renesas is that, due to a HW issue, the XHCI
> driver does not wake up after going to sleep on connecting a USB3.0
> device.
> 
> In order to mitigate that, disable the auto-suspend feature
> specifically for SMSC hubs from hub_probe() function, as a quirk.
> 
> Renesas Kingfisher Infotainment Board has two USB3.0 ports (CN2) which
> are connected via USB5534B 4-port SuperSpeed/Hi-Speed, low-power,
> configurable hub controller.
> 
> [1] SanDisk USB 3.0 device detected as USB-2.0 before the patch
>  [   74.036390] usb 5-1.1: new high-speed USB device number 4 using xhci-hcd
>  [   74.061598] usb 5-1.1: New USB device found, idVendor=0781, idProduct=5581, bcdDevice= 1.00
>  [   74.069976] usb 5-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>  [   74.077303] usb 5-1.1: Product: Ultra
>  [   74.080980] usb 5-1.1: Manufacturer: SanDisk
>  [   74.085263] usb 5-1.1: SerialNumber: 4C530001110208116550
> 
> [2] SanDisk USB 3.0 device detected as USB-3.0 after the patch
>  [   34.565078] usb 6-1.1: new SuperSpeed Gen 1 USB device number 3 using xhci-hcd
>  [   34.588719] usb 6-1.1: New USB device found, idVendor=0781, idProduct=5581, bcdDevice= 1.00
>  [   34.597098] usb 6-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
>  [   34.604430] usb 6-1.1: Product: Ultra
>  [   34.608110] usb 6-1.1: Manufacturer: SanDisk
>  [   34.612397] usb 6-1.1: SerialNumber: 4C530001110208116550

Thanks for the comprehensible and accurate write-up.

> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index a9e24e4..2fe9c9f 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -61,6 +61,7 @@ struct usb_hub {
>  	unsigned		quiescing:1;
>  	unsigned		disconnected:1;
>  	unsigned		in_reset:1;
> +	unsigned                quirk_disable_autosuspend:1;

I think it would make sense to respin the patch once again with the
above spaces converted to tabs, for consistency.

I would also include Alan's Acked-by (thanks!) and since Alan reshaped
the patch significantly in the last iteration, I would add his
Suggested-by too.

In addition to the above (please, also include):

Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks!
diff mbox series

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3405b14..1c74485 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -38,7 +38,9 @@ 
 #include "otg_whitelist.h"
 
 #define USB_VENDOR_GENESYS_LOGIC		0x05e3
+#define USB_VENDOR_SMSC				0x0424
 #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
+#define HUB_QUIRK_DISABLE_AUTOSUSPEND		0x02
 
 #define USB_TP_TRANSMISSION_DELAY	40	/* ns */
 #define USB_TP_TRANSMISSION_DELAY_MAX	65535	/* ns */
@@ -1731,6 +1733,10 @@  static void hub_disconnect(struct usb_interface *intf)
 	kfree(hub->buffer);
 
 	pm_suspend_ignore_children(&intf->dev, false);
+
+	if (hub->quirk_disable_autosuspend)
+		usb_autopm_put_interface(intf);
+
 	kref_put(&hub->kref, hub_release);
 }
 
@@ -1863,6 +1869,11 @@  static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND)
 		hub->quirk_check_port_auto_suspend = 1;
 
+	if (id->driver_info & HUB_QUIRK_DISABLE_AUTOSUSPEND) {
+		hub->quirk_disable_autosuspend = 1;
+		usb_autopm_get_interface(intf);
+	}
+
 	if (hub_configure(hub, &desc->endpoint[0].desc) >= 0)
 		return 0;
 
@@ -5599,6 +5610,10 @@  static void hub_event(struct work_struct *work)
 }
 
 static const struct usb_device_id hub_id_table[] = {
+    { .match_flags = USB_DEVICE_ID_MATCH_VENDOR | USB_DEVICE_ID_MATCH_INT_CLASS,
+      .idVendor = USB_VENDOR_SMSC,
+      .bInterfaceClass = USB_CLASS_HUB,
+      .driver_info = HUB_QUIRK_DISABLE_AUTOSUSPEND},
     { .match_flags = USB_DEVICE_ID_MATCH_VENDOR
 			| USB_DEVICE_ID_MATCH_INT_CLASS,
       .idVendor = USB_VENDOR_GENESYS_LOGIC,
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index a9e24e4..2fe9c9f 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -61,6 +61,7 @@  struct usb_hub {
 	unsigned		quiescing:1;
 	unsigned		disconnected:1;
 	unsigned		in_reset:1;
+	unsigned                quirk_disable_autosuspend:1;
 
 	unsigned		quirk_check_port_auto_suspend:1;