linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] vhci_hcd:  USB port can get stuck in the disabled state
@ 2021-07-21 23:55 Michael Broadfoot
  2021-07-22  1:26 ` Shuah Khan
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Broadfoot @ 2021-07-21 23:55 UTC (permalink / raw)
  To: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel
  Cc: Michael Broadfoot

When a remote usb device is attached to the local Virtual USB
Host Controller Root Hub port, the bound device driver may send a
port reset command. For example to initialize firmware (eg. btusb does this).
This port reset command can be sent at any time, however the VHCI hcd
root hub is only expecting reset to occur before the device receives
SET_ADDRESS. The USB port should always be enabled after a reset
(because the port is virtual and there is no possibility of hardware errors)



Signed-off-by: Michael Broadfoot <msbroadf@gmail.com>
---
 drivers/usb/usbip/vhci_hcd.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 4ba6bcdaa8e9..d3cda1b2c15a 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -455,15 +455,10 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			vhci_hcd->port_status[rhport] &= ~(1 << USB_PORT_FEAT_RESET);
 			vhci_hcd->re_timeout = 0;
 
-			if (vhci_hcd->vdev[rhport].ud.status ==
-			    VDEV_ST_NOTASSIGNED) {
-				usbip_dbg_vhci_rh(
-					" enable rhport %d (status %u)\n",
-					rhport,
-					vhci_hcd->vdev[rhport].ud.status);
-				vhci_hcd->port_status[rhport] |=
-					USB_PORT_STAT_ENABLE;
-			}
+			usbip_dbg_vhci_rh(" enable rhport %d (status %u)\n",
+					  rhport,
+					  vhci_hcd->vdev[rhport].ud.status);
+			vhci_hcd->port_status[rhport] |= USB_PORT_STAT_ENABLE;
 
 			if (hcd->speed < HCD_USB3) {
 				switch (vhci_hcd->vdev[rhport].speed) {
-- 
2.30.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-07-21 23:55 [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state Michael Broadfoot
@ 2021-07-22  1:26 ` Shuah Khan
       [not found]   ` <CALdjXpA4_eXen6RjhsEBYt8CQs-2gzwYs9h9q0Z2LKZ=rXVp+Q@mail.gmail.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Shuah Khan @ 2021-07-22  1:26 UTC (permalink / raw)
  To: Michael Broadfoot, valentina.manea.m, shuah, gregkh, linux-usb,
	linux-kernel, Shuah Khan

On 7/21/21 5:55 PM, Michael Broadfoot wrote:
> When a remote usb device is attached to the local Virtual USB
> Host Controller Root Hub port, the bound device driver may send a
> port reset command. For example to initialize firmware (eg. btusb does this).
> This port reset command can be sent at any time, however the VHCI hcd
> root hub is only expecting reset to occur before the device receives
> SET_ADDRESS. The USB port should always be enabled after a reset
> (because the port is virtual and there is no possibility of hardware errors)
> 
> 
> 
> Signed-off-by: Michael Broadfoot <msbroadf@gmail.com>
> ---
>   drivers/usb/usbip/vhci_hcd.c | 13 ++++---------
>   1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 4ba6bcdaa8e9..d3cda1b2c15a 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -455,15 +455,10 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>   			vhci_hcd->port_status[rhport] &= ~(1 << USB_PORT_FEAT_RESET);
>   			vhci_hcd->re_timeout = 0;
>   
> -			if (vhci_hcd->vdev[rhport].ud.status ==
> -			    VDEV_ST_NOTASSIGNED) {
> -				usbip_dbg_vhci_rh(
> -					" enable rhport %d (status %u)\n",
> -					rhport,
> -					vhci_hcd->vdev[rhport].ud.status);
> -				vhci_hcd->port_status[rhport] |=
> -					USB_PORT_STAT_ENABLE;
> -			}

VDEV_ST_NOTASSIGNED status indicates that the vdev is in use without
address assigned - in other words port is initializing.

This is part of the attach request handling when user requests to
attach to a remote device. attach_store() will change the status
to VDEV_ST_NOTASSIGNED and then initiate port_connect sequence.

We don't want to touch the vdev when it is in other states.
   
> +			usbip_dbg_vhci_rh(" enable rhport %d (status %u)\n",
> +					  rhport,
> +					  vhci_hcd->vdev[rhport].ud.status);
> +			vhci_hcd->port_status[rhport] |= USB_PORT_STAT_ENABLE;
>   
>   			if (hcd->speed < HCD_USB3) {
>   				switch (vhci_hcd->vdev[rhport].speed) {
> 

How did you find this problem? Does the port get into stuck state
while attaching to a remote usbip device on the host?

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
       [not found]   ` <CALdjXpA4_eXen6RjhsEBYt8CQs-2gzwYs9h9q0Z2LKZ=rXVp+Q@mail.gmail.com>
@ 2021-07-22  6:19     ` Michael
  2021-07-23 16:29     ` Shuah Khan
  1 sibling, 0 replies; 13+ messages in thread
From: Michael @ 2021-07-22  6:19 UTC (permalink / raw)
  To: linux-usb; +Cc: linux-kernel

You can pass through any bluetooth dongle (or xbox wireless dongle)
through usbip and it will cause this issue.

For example, here is one of my customers
https://www.virtualhere.com/comment/9432#comment-9432 with the issue.

The device is in the  VDEV_ST_USED state when a reset occurs and so
its never re-enabled

Furthermore there is a bug in the line pr_err("vhci_device speed not set\n");
(L479) because resetting a full-speed device is not an error.


 On Thu, 22 Jul 2021 at 11:26, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 7/21/21 5:55 PM, Michael Broadfoot wrote:
> > When a remote usb device is attached to the local Virtual USB
> > Host Controller Root Hub port, the bound device driver may send a
> > port reset command. For example to initialize firmware (eg. btusb does this).
> > This port reset command can be sent at any time, however the VHCI hcd
> > root hub is only expecting reset to occur before the device receives
> > SET_ADDRESS. The USB port should always be enabled after a reset
> > (because the port is virtual and there is no possibility of hardware errors)
> >
> >
> >
> > Signed-off-by: Michael Broadfoot <msbroadf@gmail.com>
> > ---
> >   drivers/usb/usbip/vhci_hcd.c | 13 ++++---------
> >   1 file changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> > index 4ba6bcdaa8e9..d3cda1b2c15a 100644
> > --- a/drivers/usb/usbip/vhci_hcd.c
> > +++ b/drivers/usb/usbip/vhci_hcd.c
> > @@ -455,15 +455,10 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> >                       vhci_hcd->port_status[rhport] &= ~(1 << USB_PORT_FEAT_RESET);
> >                       vhci_hcd->re_timeout = 0;
> >
> > -                     if (vhci_hcd->vdev[rhport].ud.status ==
> > -                         VDEV_ST_NOTASSIGNED) {
> > -                             usbip_dbg_vhci_rh(
> > -                                     " enable rhport %d (status %u)\n",
> > -                                     rhport,
> > -                                     vhci_hcd->vdev[rhport].ud.status);
> > -                             vhci_hcd->port_status[rhport] |=
> > -                                     USB_PORT_STAT_ENABLE;
> > -                     }
>
> VDEV_ST_NOTASSIGNED status indicates that the vdev is in use without
> address assigned - in other words port is initializing.
>
> This is part of the attach request handling when user requests to
> attach to a remote device. attach_store() will change the status
> to VDEV_ST_NOTASSIGNED and then initiate port_connect sequence.
>
> We don't want to touch the vdev when it is in other states.
>
> > +                     usbip_dbg_vhci_rh(" enable rhport %d (status %u)\n",
> > +                                       rhport,
> > +                                       vhci_hcd->vdev[rhport].ud.status);
> > +                     vhci_hcd->port_status[rhport] |= USB_PORT_STAT_ENABLE;
> >
> >                       if (hcd->speed < HCD_USB3) {
> >                               switch (vhci_hcd->vdev[rhport].speed) {
> >
>
> How did you find this problem? Does the port get into stuck state
> while attaching to a remote usbip device on the host?
>
> thanks,
> -- Shuah

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
       [not found]   ` <CALdjXpA4_eXen6RjhsEBYt8CQs-2gzwYs9h9q0Z2LKZ=rXVp+Q@mail.gmail.com>
  2021-07-22  6:19     ` Michael
@ 2021-07-23 16:29     ` Shuah Khan
  2021-07-23 23:53       ` Michael
  2021-07-23 23:58       ` Michael
  1 sibling, 2 replies; 13+ messages in thread
From: Shuah Khan @ 2021-07-23 16:29 UTC (permalink / raw)
  To: Michael
  Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel, Shuah Khan

On 7/21/21 8:27 PM, Michael wrote:
> You can pass through any bluetooth dongle (or xbox wireless dongle) through usbip and it will cause this issue.
> 
> For example, here is one of my customers https://www.virtualhere.com/comment/9432#comment-9432 <https://www.virtualhere.com/comment/9432#comment-9432> with the issue.
> 
> The device is in the VDEV_ST_USED state when a reset occurs and so its never re-enabled
> 
> Furthermore there is a bug in the line pr_err("vhci_device speed not set\n");
> (L479) because resetting a full-speed device is not an error.
> 

Can you provide me the full demsg from host and client including the part
where device is attached? I assume usbip tools from Linux sources are used?

Also send me lsusb output on host and client

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-07-23 16:29     ` Shuah Khan
@ 2021-07-23 23:53       ` Michael
  2021-07-23 23:58       ` Michael
  1 sibling, 0 replies; 13+ messages in thread
From: Michael @ 2021-07-23 23:53 UTC (permalink / raw)
  To: Shuah Khan; +Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel

Here is an xbox controller. (Note this usb is just a mt76 usb->wifi
chip slightly modified).

Host commands
----------------------
sudo usbipd &
sudo modprobe usbip-host
sudo usbip bind -b 1-1.4

Client commands
-----------------------
sudo modprobe vhci-hcd
sudo usbip attach -r 192.168.1.189 -b 1-1.4

Host dmesg
-----------------
[   61.713164] usbcore: registered new device driver usbip-host
[   99.964196] mt76x2u 1-1.4:1.0: mac specific condition occurred
[  100.212477] usbip-host 1-1.4: usbip-host: register new device (bus 1 dev 3)
[  128.195578] usbip-host 1-1.4: stub up

Client dmesg
--------------------
[  184.764560] vhci_hcd vhci_hcd.0: pdev(0) rhport(0) sockfd(3)
[  184.764569] vhci_hcd vhci_hcd.0: devid(65539) speed(3) speed_str(high-speed)
[  184.764592] vhci_hcd vhci_hcd.0: Device attached
[  184.999634] usb 5-1: new high-speed USB device number 2 using vhci_hcd
[  185.127653] usb 5-1: SetAddress Request (2) to port 0
[  185.173988] usb 5-1: New USB device found, idVendor=045e,
idProduct=02e6, bcdDevice= 1.00
[  185.173996] usb 5-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[  185.174000] usb 5-1: Product: XBOX ACC
[  185.174003] usb 5-1: Manufacturer: Microsoft Inc.
[  185.174005] usb 5-1: SerialNumber: 203124
[  185.357262] cfg80211: Loading compiled-in X.509 certificates for
regulatory database
[  185.357936] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
[  186.387801] usb usb5-port1: Cannot enable. Maybe the USB cable is bad?
[  187.287921] usb usb5-port1: Cannot enable. Maybe the USB cable is bad?
[  188.188011] usb usb5-port1: Cannot enable. Maybe the USB cable is bad?
[  189.088116] usb usb5-port1: Cannot enable. Maybe the USB cable is bad?
[  189.088169] usb 5-1: USB disconnect, device number 2
[  189.088789] mt76x2u 5-1:1.0: ASIC revision: ffffffff
[  189.089569] usbcore: registered new interface driver mt76x2u
[  189.988201] usb usb5-port1: Cannot enable. Maybe the USB cable is bad?
[  190.888294] usb usb5-port1: Cannot enable. Maybe the USB cable is bad?
[  190.888321] usb usb5-port1: attempt power cycle
[  192.104419] usb usb5-port1: Cannot enable. Maybe the USB cable is bad?
[  193.004503] usb usb5-port1: Cannot enable. Maybe the USB cable is bad?
[  193.004531] usb usb5-port1: unable to enumerate USB device


On Sat, 24 Jul 2021 at 02:29, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 7/21/21 8:27 PM, Michael wrote:
> > You can pass through any bluetooth dongle (or xbox wireless dongle) through usbip and it will cause this issue.
> >
> > For example, here is one of my customers https://www.virtualhere.com/comment/9432#comment-9432 <https://www.virtualhere.com/comment/9432#comment-9432> with the issue.
> >
> > The device is in the VDEV_ST_USED state when a reset occurs and so its never re-enabled
> >
> > Furthermore there is a bug in the line pr_err("vhci_device speed not set\n");
> > (L479) because resetting a full-speed device is not an error.
> >
>
> Can you provide me the full demsg from host and client including the part
> where device is attached? I assume usbip tools from Linux sources are used?
>
> Also send me lsusb output on host and client
>
> thanks,
> -- Shuah

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-07-23 16:29     ` Shuah Khan
  2021-07-23 23:53       ` Michael
@ 2021-07-23 23:58       ` Michael
  2021-07-30 22:52         ` Shuah Khan
  1 sibling, 1 reply; 13+ messages in thread
From: Michael @ 2021-07-23 23:58 UTC (permalink / raw)
  To: Shuah Khan; +Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel

Here is the lsusb on the client when the device fails to attach
---------------------------------------
michael@ubuntu:~$ lsusb
Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 004 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 002 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub


Here is the lsusb on the host before use
-----------------------------------------------------
pi@raspberrypi:~ $ lsusb
Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
Bus 001 Device 003: ID 045e:02e6 Microsoft Corp. Wireless XBox Controller Dongle
Bus 001 Device 002: ID 2109:3431 VIA Labs, Inc. Hub
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

On Sat, 24 Jul 2021 at 02:29, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 7/21/21 8:27 PM, Michael wrote:
> > You can pass through any bluetooth dongle (or xbox wireless dongle) through usbip and it will cause this issue.
> >
> > For example, here is one of my customers https://www.virtualhere.com/comment/9432#comment-9432 <https://www.virtualhere.com/comment/9432#comment-9432> with the issue.
> >
> > The device is in the VDEV_ST_USED state when a reset occurs and so its never re-enabled
> >
> > Furthermore there is a bug in the line pr_err("vhci_device speed not set\n");
> > (L479) because resetting a full-speed device is not an error.
> >
>
> Can you provide me the full demsg from host and client including the part
> where device is attached? I assume usbip tools from Linux sources are used?
>
> Also send me lsusb output on host and client
>
> thanks,
> -- Shuah

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-07-23 23:58       ` Michael
@ 2021-07-30 22:52         ` Shuah Khan
  2021-07-30 23:52           ` Michael
  0 siblings, 1 reply; 13+ messages in thread
From: Shuah Khan @ 2021-07-30 22:52 UTC (permalink / raw)
  To: Michael
  Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel, Shuah Khan

On 7/23/21 5:58 PM, Michael wrote:
> Here is the lsusb on the client when the device fails to attach
> ---------------------------------------
> michael@ubuntu:~$ lsusb
> Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> Bus 004 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> Bus 002 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> 
> 
> Here is the lsusb on the host before use
> -----------------------------------------------------
> pi@raspberrypi:~ $ lsusb
> Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 001 Device 003: ID 045e:02e6 Microsoft Corp. Wireless XBox Controller Dongle
> Bus 001 Device 002: ID 2109:3431 VIA Labs, Inc. Hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> 

Sorry for the delay on this. I had to make time to do some research on
usb_reset_device() calls from probe routines.

 From what you said in another email:

"The device is in the  VDEV_ST_USED state when a reset occurs and so its
never re-enabled"

Right if device is in VDEV_ST_USED state, vhci treats reset as invalid.

Your commit log says:

"When a remote usb device is attached to the local Virtual USB
Host Controller Root Hub port, the bound device driver may send a
port reset command. For example to initialize firmware (eg. btusb does this).
This port reset command can be sent at any time, however the VHCI hcd
root hub is only expecting reset to occur before the device receives
SET_ADDRESS. The USB port should always be enabled after a reset
(because the port is virtual and there is no possibility of hardware errors)"

It appears btusb driver issues reset as a workaround: btusb_setup_intel()

        /* The controller has a bug with the first HCI command sent to it
          * returning number of completed commands as zero. This would stall the
          * command processing in the Bluetooth core.
          *
          * As a workaround, send HCI Reset command first which will reset the
          * number of completed commands and allow normal command processing
          * from now on.
          */

Another "Toggle the hard reset line" workaround for Realtek devices:
See btusb_rtl_cmd_timeout()

Both of these cases are workarounds. Is this what you are referring to about
btusb doing reset?

Specific to this bug report and mt76, other network/wireless usb drivers
call usb_reset_device() from their probe routines unconditionally. These
are the calls from normal paths and not fw load/error paths.

rtl8152_probe(), carl9170_usb_probe(), mt7663u_probe() and so on. Looking
at these probe routines, it appears some of them do that to avoid problems
in disconnect path.

I have two questions:
- Is it necessary to do usb_reset_device() in net/wireless usb driver
   probe routines?
- Are these legit calls according to protocol?

If yes, we can look into changing vhci to handle this case for net/wireless
usb drivers. I would not delete the check though. I would add VDEV_ST_USED
check in addition to VDEV_ST_NOTASSIGNED with some comments on why this is
necessary.

Furthermore there is a bug in the line pr_err("vhci_device speed not set\n");
(L479) because resetting a full-speed device is not an error.

This is a separate issue is separate. vhci is missing USB_SPEED_FULL checks
at various places. This has to be done as a separate work.

thanks,
-- Shuah



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-07-30 22:52         ` Shuah Khan
@ 2021-07-30 23:52           ` Michael
  2021-08-02 23:14             ` Shuah Khan
  0 siblings, 1 reply; 13+ messages in thread
From: Michael @ 2021-07-30 23:52 UTC (permalink / raw)
  To: Shuah Khan; +Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel

Yes i think just adding the VDEV_ST_USED check in addition to the
VDEV_ST_NOT_ASSIGNED state is fine and would fix the issue.

After many years of writing virtualhere (a similar system to usb/ip
but cross-platform and different non-kernel way of doing it server
side) I've seen many drivers that issue reset at any time. Loading
firmware is usually the reason.  Also sometimes switching
configurations requires a reset also, for example some gaming wheels
do this. I don't think you should make this VDEV_ST_USED check
specific to Wifi devices, as a lot of devices don't follow too closely
to the USB protocol to begin with from my experience. They primarily
base their USB interactions assuming the windows platform and its
quirks.

On Sat, 31 Jul 2021 at 08:52, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 7/23/21 5:58 PM, Michael wrote:
> > Here is the lsusb on the client when the device fails to attach
> > ---------------------------------------
> > michael@ubuntu:~$ lsusb
> > Bus 006 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> > Bus 004 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> > Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> > Bus 002 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
> >
> >
> > Here is the lsusb on the host before use
> > -----------------------------------------------------
> > pi@raspberrypi:~ $ lsusb
> > Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> > Bus 001 Device 003: ID 045e:02e6 Microsoft Corp. Wireless XBox Controller Dongle
> > Bus 001 Device 002: ID 2109:3431 VIA Labs, Inc. Hub
> > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
> >
>
> Sorry for the delay on this. I had to make time to do some research on
> usb_reset_device() calls from probe routines.
>
>  From what you said in another email:
>
> "The device is in the  VDEV_ST_USED state when a reset occurs and so its
> never re-enabled"
>
> Right if device is in VDEV_ST_USED state, vhci treats reset as invalid.
>
> Your commit log says:
>
> "When a remote usb device is attached to the local Virtual USB
> Host Controller Root Hub port, the bound device driver may send a
> port reset command. For example to initialize firmware (eg. btusb does this).
> This port reset command can be sent at any time, however the VHCI hcd
> root hub is only expecting reset to occur before the device receives
> SET_ADDRESS. The USB port should always be enabled after a reset
> (because the port is virtual and there is no possibility of hardware errors)"
>
> It appears btusb driver issues reset as a workaround: btusb_setup_intel()
>
>         /* The controller has a bug with the first HCI command sent to it
>           * returning number of completed commands as zero. This would stall the
>           * command processing in the Bluetooth core.
>           *
>           * As a workaround, send HCI Reset command first which will reset the
>           * number of completed commands and allow normal command processing
>           * from now on.
>           */
>
> Another "Toggle the hard reset line" workaround for Realtek devices:
> See btusb_rtl_cmd_timeout()
>
> Both of these cases are workarounds. Is this what you are referring to about
> btusb doing reset?
>
> Specific to this bug report and mt76, other network/wireless usb drivers
> call usb_reset_device() from their probe routines unconditionally. These
> are the calls from normal paths and not fw load/error paths.
>
> rtl8152_probe(), carl9170_usb_probe(), mt7663u_probe() and so on. Looking
> at these probe routines, it appears some of them do that to avoid problems
> in disconnect path.
>
> I have two questions:
> - Is it necessary to do usb_reset_device() in net/wireless usb driver
>    probe routines?
> - Are these legit calls according to protocol?
>
> If yes, we can look into changing vhci to handle this case for net/wireless
> usb drivers. I would not delete the check though. I would add VDEV_ST_USED
> check in addition to VDEV_ST_NOTASSIGNED with some comments on why this is
> necessary.
>
> Furthermore there is a bug in the line pr_err("vhci_device speed not set\n");
> (L479) because resetting a full-speed device is not an error.
>
> This is a separate issue is separate. vhci is missing USB_SPEED_FULL checks
> at various places. This has to be done as a separate work.
>
> thanks,
> -- Shuah
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-07-30 23:52           ` Michael
@ 2021-08-02 23:14             ` Shuah Khan
  2021-08-03  1:00               ` Michael
  0 siblings, 1 reply; 13+ messages in thread
From: Shuah Khan @ 2021-08-02 23:14 UTC (permalink / raw)
  To: Michael
  Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel, Shuah Khan

On 7/30/21 5:52 PM, Michael wrote:
> Yes i think just adding the VDEV_ST_USED check in addition to the
> VDEV_ST_NOT_ASSIGNED state is fine and would fix the issue.
> 

Can you please confirm if this works?

> After many years of writing virtualhere (a similar system to usb/ip
> but cross-platform and different non-kernel way of doing it server
> side) I've seen many drivers that issue reset at any time. Loading
> firmware is usually the reason.  Also sometimes switching
> configurations requires a reset also, for example some gaming wheels
> do this. I don't think you should make this VDEV_ST_USED check
> specific to Wifi devices, as a lot of devices don't follow too closely
> to the USB protocol to begin with from my experience. They primarily
> base their USB interactions assuming the windows platform and its
> quirks.
> 

When sending responses to Linux kernel mailing lists, please use bottom post.
This check will be used for all drivers. We don't add checks for specific cases
in the code.

thanks,
-- Shuah


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-08-02 23:14             ` Shuah Khan
@ 2021-08-03  1:00               ` Michael
  2021-08-10 17:46                 ` Shuah Khan
  0 siblings, 1 reply; 13+ messages in thread
From: Michael @ 2021-08-03  1:00 UTC (permalink / raw)
  To: Shuah Khan; +Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel

On Tue, 3 Aug 2021 at 09:14, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 7/30/21 5:52 PM, Michael wrote:
> > Yes i think just adding the VDEV_ST_USED check in addition to the
> > VDEV_ST_NOT_ASSIGNED state is fine and would fix the issue.
> >
>
> Can you please confirm if this works?
>
> > After many years of writing virtualhere (a similar system to usb/ip
> > but cross-platform and different non-kernel way of doing it server
> > side) I've seen many drivers that issue reset at any time. Loading
> > firmware is usually the reason.  Also sometimes switching
> > configurations requires a reset also, for example some gaming wheels
> > do this. I don't think you should make this VDEV_ST_USED check
> > specific to Wifi devices, as a lot of devices don't follow too closely
> > to the USB protocol to begin with from my experience. They primarily
> > base their USB interactions assuming the windows platform and its
> > quirks.
> >
>
> When sending responses to Linux kernel mailing lists, please use bottom post.
> This check will be used for all drivers. We don't add checks for specific cases
> in the code.
>
> thanks,
> -- Shuah
>

Yes it works with that change.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-08-03  1:00               ` Michael
@ 2021-08-10 17:46                 ` Shuah Khan
  2021-08-11  2:30                   ` Michael
  0 siblings, 1 reply; 13+ messages in thread
From: Shuah Khan @ 2021-08-10 17:46 UTC (permalink / raw)
  To: Michael
  Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel, Shuah Khan

On 8/2/21 7:00 PM, Michael wrote:
> On Tue, 3 Aug 2021 at 09:14, Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> On 7/30/21 5:52 PM, Michael wrote:
>>> Yes i think just adding the VDEV_ST_USED check in addition to the
>>> VDEV_ST_NOT_ASSIGNED state is fine and would fix the issue.
>>>
>>
>> Can you please confirm if this works?
>>
>>> After many years of writing virtualhere (a similar system to usb/ip
>>> but cross-platform and different non-kernel way of doing it server
>>> side) I've seen many drivers that issue reset at any time. Loading
>>> firmware is usually the reason.  Also sometimes switching
>>> configurations requires a reset also, for example some gaming wheels
>>> do this. I don't think you should make this VDEV_ST_USED check
>>> specific to Wifi devices, as a lot of devices don't follow too closely
>>> to the USB protocol to begin with from my experience. They primarily
>>> base their USB interactions assuming the windows platform and its
>>> quirks.
>>>
>>
>> When sending responses to Linux kernel mailing lists, please use bottom post.
>> This check will be used for all drivers. We don't add checks for specific cases
>> in the code.
>>
>> thanks,
>> -- Shuah
>>
> 
> Yes it works with that change.
> 

Would you like to send me a patch for this?

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-08-10 17:46                 ` Shuah Khan
@ 2021-08-11  2:30                   ` Michael
  2021-08-19 22:57                     ` Shuah Khan
  0 siblings, 1 reply; 13+ messages in thread
From: Michael @ 2021-08-11  2:30 UTC (permalink / raw)
  To: Shuah Khan; +Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel

On Wed, 11 Aug 2021 at 03:46, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 8/2/21 7:00 PM, Michael wrote:
> > On Tue, 3 Aug 2021 at 09:14, Shuah Khan <skhan@linuxfoundation.org> wrote:
> >>
> >> On 7/30/21 5:52 PM, Michael wrote:
> >>> Yes i think just adding the VDEV_ST_USED check in addition to the
> >>> VDEV_ST_NOT_ASSIGNED state is fine and would fix the issue.
> >>>
> >>
> >> Can you please confirm if this works?
> >>
> >>> After many years of writing virtualhere (a similar system to usb/ip
> >>> but cross-platform and different non-kernel way of doing it server
> >>> side) I've seen many drivers that issue reset at any time. Loading
> >>> firmware is usually the reason.  Also sometimes switching
> >>> configurations requires a reset also, for example some gaming wheels
> >>> do this. I don't think you should make this VDEV_ST_USED check
> >>> specific to Wifi devices, as a lot of devices don't follow too closely
> >>> to the USB protocol to begin with from my experience. They primarily
> >>> base their USB interactions assuming the windows platform and its
> >>> quirks.
> >>>
> >>
> >> When sending responses to Linux kernel mailing lists, please use bottom post.
> >> This check will be used for all drivers. We don't add checks for specific cases
> >> in the code.
> >>
> >> thanks,
> >> -- Shuah
> >>
> >
> > Yes it works with that change.
> >
>
> Would you like to send me a patch for this?
>
> thanks,
> -- Shuah

usbip: Allow port reset to occur when the port is also in the ST_USED state

 Signed-off-by: Michael <mail@virtualhere.com>
---
 drivers/usb/usbip/vhci_hcd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 4ba6bcdaa8e9..300131ae5897 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -456,7 +456,9 @@ static int vhci_hub_control(struct usb_hcd *hcd,
u16 typeReq, u16 wValue,
                        vhci_hcd->re_timeout = 0;

                        if (vhci_hcd->vdev[rhport].ud.status ==
-                           VDEV_ST_NOTASSIGNED) {
+                           VDEV_ST_NOTASSIGNED ||
+                               vhci_hcd->vdev[rhport].ud.status ==
+                               VDEV_ST_USED) {
                                usbip_dbg_vhci_rh(
                                        " enable rhport %d (status %u)\n",
                                        rhport,
--
2.30.2

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state
  2021-08-11  2:30                   ` Michael
@ 2021-08-19 22:57                     ` Shuah Khan
  0 siblings, 0 replies; 13+ messages in thread
From: Shuah Khan @ 2021-08-19 22:57 UTC (permalink / raw)
  To: Michael
  Cc: valentina.manea.m, shuah, gregkh, linux-usb, linux-kernel, Shuah Khan

On 8/10/21 8:30 PM, Michael wrote:
> On Wed, 11 Aug 2021 at 03:46, Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> On 8/2/21 7:00 PM, Michael wrote:
>>> On Tue, 3 Aug 2021 at 09:14, Shuah Khan <skhan@linuxfoundation.org> wrote:
>>>>
>>>> On 7/30/21 5:52 PM, Michael wrote:
>>>>> Yes i think just adding the VDEV_ST_USED check in addition to the
>>>>> VDEV_ST_NOT_ASSIGNED state is fine and would fix the issue.
>>>>>
>>>>
>>>> Can you please confirm if this works?
>>>>
>>>>> After many years of writing virtualhere (a similar system to usb/ip
>>>>> but cross-platform and different non-kernel way of doing it server
>>>>> side) I've seen many drivers that issue reset at any time. Loading
>>>>> firmware is usually the reason.  Also sometimes switching
>>>>> configurations requires a reset also, for example some gaming wheels
>>>>> do this. I don't think you should make this VDEV_ST_USED check
>>>>> specific to Wifi devices, as a lot of devices don't follow too closely
>>>>> to the USB protocol to begin with from my experience. They primarily
>>>>> base their USB interactions assuming the windows platform and its
>>>>> quirks.
>>>>>
>>>>
>>>> When sending responses to Linux kernel mailing lists, please use bottom post.
>>>> This check will be used for all drivers. We don't add checks for specific cases
>>>> in the code.
>>>>
>>>> thanks,
>>>> -- Shuah
>>>>
>>>
>>> Yes it works with that change.
>>>
>>
>> Would you like to send me a patch for this?
>>
>> thanks,
>> -- Shuah
> 
> usbip: Allow port reset to occur when the port is also in the ST_USED state
> 
>   Signed-off-by: Michael <mail@virtualhere.com>
> ---
>   drivers/usb/usbip/vhci_hcd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 4ba6bcdaa8e9..300131ae5897 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -456,7 +456,9 @@ static int vhci_hub_control(struct usb_hcd *hcd,
> u16 typeReq, u16 wValue,
>                          vhci_hcd->re_timeout = 0;
> 
>                          if (vhci_hcd->vdev[rhport].ud.status ==
> -                           VDEV_ST_NOTASSIGNED) {
> +                           VDEV_ST_NOTASSIGNED ||
> +                               vhci_hcd->vdev[rhport].ud.status ==
> +                               VDEV_ST_USED) {
>                                  usbip_dbg_vhci_rh(
>                                          " enable rhport %d (status %u)\n",
>                                          rhport,
> --
> 2.30.2
> 

Unfortunately we can't apply this diff. I turned this into a proper
patch giving you credit for reporting the problem, suggesting the
fix and testing it. Patch will be sent shortly.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-08-19 22:57 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 23:55 [PATCH v2] vhci_hcd: USB port can get stuck in the disabled state Michael Broadfoot
2021-07-22  1:26 ` Shuah Khan
     [not found]   ` <CALdjXpA4_eXen6RjhsEBYt8CQs-2gzwYs9h9q0Z2LKZ=rXVp+Q@mail.gmail.com>
2021-07-22  6:19     ` Michael
2021-07-23 16:29     ` Shuah Khan
2021-07-23 23:53       ` Michael
2021-07-23 23:58       ` Michael
2021-07-30 22:52         ` Shuah Khan
2021-07-30 23:52           ` Michael
2021-08-02 23:14             ` Shuah Khan
2021-08-03  1:00               ` Michael
2021-08-10 17:46                 ` Shuah Khan
2021-08-11  2:30                   ` Michael
2021-08-19 22:57                     ` Shuah Khan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).