[PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Jose Ignacio Tornos Martinez]

Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
Trust) hang when they are unbound from 'unbind' sysfs entry and
can not be bound again.

The reason is CSR chip hangs when usb configuration command with
index 0 (used to unconfigure) is sent during disconnection.

To avoid this unwanted result, it is necessary not to send this
command for CSR chip when usb device is unbound.
Besides, "skip_unconfigure" sysfs entry has been created for
testing purposes with these or other devices.

Athough device is not unconfigured, it is better to avoid device
hanging to be able to operate. Even bluetooth can be previously
turned off.
On the other hand, this is not important if usb device is going to
be bound again (normal behavior), i.e. with usbip.

Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
---
 drivers/bluetooth/btusb.c  |  8 +++++++-
 drivers/usb/core/generic.c |  2 +-
 drivers/usb/core/sysfs.c   | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/usb.h        |  2 ++
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index b7c72eb96c87..27ad3c2234db 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -4835,8 +4835,14 @@ static int btusb_probe(struct usb_interface *intf,
 
 		/* Fake CSR devices with broken commands */
 		if (le16_to_cpu(udev->descriptor.idVendor)  == 0x0a12 &&
-		    le16_to_cpu(udev->descriptor.idProduct) == 0x0001)
+		    le16_to_cpu(udev->descriptor.idProduct) == 0x0001) {
 			hdev->setup = btusb_setup_csr;
+			/* This device hangs when configuration command with
+			 * index 0 (unconfigure) is sent, avoid this at least
+			 * if it is unbound.
+			 */
+			udev->skip_unconfigure = 1;
+		}
 	}
 
 	if (id->driver_info & BTUSB_SNIFFER) {
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 26f9fb9f67ca..f25171284119 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -256,7 +256,7 @@ void usb_generic_driver_disconnect(struct usb_device *udev)
 
 	/* if this is only an unbind, not a physical disconnect, then
 	 * unconfigure the device */
-	if (udev->actconfig)
+	if (!udev->skip_unconfigure && udev->actconfig)
 		usb_set_configuration(udev, -1);
 }
 
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index fa2e49d432ff..2e5839b53072 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -1189,6 +1189,41 @@ static struct device_attribute dev_attr_interface_authorized =
 		__ATTR(authorized, S_IRUGO | S_IWUSR,
 		interface_authorized_show, interface_authorized_store);
 
+static ssize_t skip_unconfigure_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *udev = interface_to_usbdev(intf);
+	u8 val;
+
+	if (usb_lock_device_interruptible(udev) < 0)
+		return -EINTR;
+	val = udev->skip_unconfigure;
+	usb_unlock_device(udev);
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t skip_unconfigure_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *udev = interface_to_usbdev(intf);
+	u8 val;
+
+	if (kstrtou8(buf, 0, &val) || val > 1)
+		return -EINVAL;
+
+	if (usb_lock_device_interruptible(udev) < 0)
+		return -EINTR;
+	udev->skip_unconfigure = val;
+	usb_unlock_device(udev);
+
+	return count;
+}
+static DEVICE_ATTR_RW(skip_unconfigure);
+
 static struct attribute *intf_attrs[] = {
 	&dev_attr_bInterfaceNumber.attr,
 	&dev_attr_bAlternateSetting.attr,
@@ -1199,6 +1234,7 @@ static struct attribute *intf_attrs[] = {
 	&dev_attr_modalias.attr,
 	&dev_attr_supports_autosuspend.attr,
 	&dev_attr_interface_authorized.attr,
+	&dev_attr_skip_unconfigure.attr,
 	NULL,
 };
 static const struct attribute_group intf_attr_grp = {
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 86a73d834e38..55828cd0a0d1 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -618,6 +618,7 @@ struct usb3_lpm_parameters {
  *	parent->hub_delay + wHubDelay + tTPTransmissionDelay (40ns)
  *	Will be used as wValue for SetIsochDelay requests.
  * @use_generic_driver: ask driver core to reprobe using the generic driver.
+ * @skip_unconfigure: disable unconfigure operation for devices without support.
  *
  * Notes:
  * Usbcore drivers should not set usbdev->state directly.  Instead use
@@ -704,6 +705,7 @@ struct usb_device {
 
 	u16 hub_delay;
 	unsigned use_generic_driver:1;
+	unsigned skip_unconfigure:1;
 };
 #define	to_usb_device(d) container_of(d, struct usb_device, dev)
 
-- 
2.27.0

Re: [PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Greg KH]

On Fri, Apr 29, 2022 at 05:31:38PM +0200, Jose Ignacio Tornos Martinez wrote:
> Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
> Trust) hang when they are unbound from 'unbind' sysfs entry and
> can not be bound again.
> 
> The reason is CSR chip hangs when usb configuration command with
> index 0 (used to unconfigure) is sent during disconnection.
> 
> To avoid this unwanted result, it is necessary not to send this
> command for CSR chip when usb device is unbound.
> Besides, "skip_unconfigure" sysfs entry has been created for
> testing purposes with these or other devices.
> 
> Athough device is not unconfigured, it is better to avoid device
> hanging to be able to operate. Even bluetooth can be previously
> turned off.
> On the other hand, this is not important if usb device is going to
> be bound again (normal behavior), i.e. with usbip.
> 
> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
> ---
>  drivers/bluetooth/btusb.c  |  8 +++++++-
>  drivers/usb/core/generic.c |  2 +-
>  drivers/usb/core/sysfs.c   | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h        |  2 ++
>  4 files changed, 46 insertions(+), 2 deletions(-)

No documentation for this new sysfs attribute you wish to create?
That's not going to help us with review :(

thanks,

greg k-h

Re: [PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Alan Stern]

On Fri, Apr 29, 2022 at 05:31:38PM +0200, Jose Ignacio Tornos Martinez wrote:
> Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
> Trust) hang when they are unbound from 'unbind' sysfs entry and
> can not be bound again.
> 
> The reason is CSR chip hangs when usb configuration command with
> index 0 (used to unconfigure) is sent during disconnection.
> 
> To avoid this unwanted result, it is necessary not to send this
> command for CSR chip when usb device is unbound.
> Besides, "skip_unconfigure" sysfs entry has been created for
> testing purposes with these or other devices.

I don't see any good reason for adding this sysfs entry.  Normal users 
won't want to do it, and developers can add their own quirks to their 
kernels.  Also, see below.

> Athough device is not unconfigured, it is better to avoid device
> hanging to be able to operate. Even bluetooth can be previously
> turned off.
> On the other hand, this is not important if usb device is going to
> be bound again (normal behavior), i.e. with usbip.
> 
> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
> ---
>  drivers/bluetooth/btusb.c  |  8 +++++++-
>  drivers/usb/core/generic.c |  2 +-
>  drivers/usb/core/sysfs.c   | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h        |  2 ++
>  4 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 86a73d834e38..55828cd0a0d1 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -618,6 +618,7 @@ struct usb3_lpm_parameters {
>   *	parent->hub_delay + wHubDelay + tTPTransmissionDelay (40ns)
>   *	Will be used as wValue for SetIsochDelay requests.
>   * @use_generic_driver: ask driver core to reprobe using the generic driver.
> + * @skip_unconfigure: disable unconfigure operation for devices without support.
>   *
>   * Notes:
>   * Usbcore drivers should not set usbdev->state directly.  Instead use
> @@ -704,6 +705,7 @@ struct usb_device {
>  
>  	u16 hub_delay;
>  	unsigned use_generic_driver:1;
> +	unsigned skip_unconfigure:1;
>  };
>  #define	to_usb_device(d) container_of(d, struct usb_device, dev)

This is not a good way to do it.  Instead you should create a new USB 
device quirk bit.  An advantage of this is that there is already a 
mechanism for users to manually set a quirk flag for a device (the 
"quirks" sysfs module file).

Alan Stern

Re: [PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Jose Ignacio Tornos Martinez]

Hello Alan,

Ok, I will prepare a new patch in that way.

Thanks

Best regards
José Ignacio


On Sat, Apr 30, 2022 at 3:18 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, Apr 29, 2022 at 05:31:38PM +0200, Jose Ignacio Tornos Martinez wrote:
> > Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
> > Trust) hang when they are unbound from 'unbind' sysfs entry and
> > can not be bound again.
> >
> > The reason is CSR chip hangs when usb configuration command with
> > index 0 (used to unconfigure) is sent during disconnection.
> >
> > To avoid this unwanted result, it is necessary not to send this
> > command for CSR chip when usb device is unbound.
> > Besides, "skip_unconfigure" sysfs entry has been created for
> > testing purposes with these or other devices.
>
> I don't see any good reason for adding this sysfs entry.  Normal users
> won't want to do it, and developers can add their own quirks to their
> kernels.  Also, see below.
>
> > Athough device is not unconfigured, it is better to avoid device
> > hanging to be able to operate. Even bluetooth can be previously
> > turned off.
> > On the other hand, this is not important if usb device is going to
> > be bound again (normal behavior), i.e. with usbip.
> >
> > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
> > ---
> >  drivers/bluetooth/btusb.c  |  8 +++++++-
> >  drivers/usb/core/generic.c |  2 +-
> >  drivers/usb/core/sysfs.c   | 36 ++++++++++++++++++++++++++++++++++++
> >  include/linux/usb.h        |  2 ++
> >  4 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 86a73d834e38..55828cd0a0d1 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -618,6 +618,7 @@ struct usb3_lpm_parameters {
> >   *   parent->hub_delay + wHubDelay + tTPTransmissionDelay (40ns)
> >   *   Will be used as wValue for SetIsochDelay requests.
> >   * @use_generic_driver: ask driver core to reprobe using the generic driver.
> > + * @skip_unconfigure: disable unconfigure operation for devices without support.
> >   *
> >   * Notes:
> >   * Usbcore drivers should not set usbdev->state directly.  Instead use
> > @@ -704,6 +705,7 @@ struct usb_device {
> >
> >       u16 hub_delay;
> >       unsigned use_generic_driver:1;
> > +     unsigned skip_unconfigure:1;
> >  };
> >  #define      to_usb_device(d) container_of(d, struct usb_device, dev)
>
> This is not a good way to do it.  Instead you should create a new USB
> device quirk bit.  An advantage of this is that there is already a
> mechanism for users to manually set a quirk flag for a device (the
> "quirks" sysfs module file).
>
> Alan Stern
>

[PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Jose Ignacio Tornos Martinez]

Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
Trust) hang when they are unbound from 'unbind' sysfs entry and
can not be bound again.

The reason is CSR chip hangs when usb configuration command with
index 0 (used to unconfigure) is sent during disconnection.

To avoid this unwanted result, it is necessary not to send this
command for CSR chip when usb device is unbound, so a new quirk
has been created for this device.

Athough device is not unconfigured, it is better to avoid device
hanging to be able to operate. Even bluetooth can be previously
turned off.
On the other hand, this is not important if usb device is going to
be bound again (normal behavior), i.e. with usbip.
---
 drivers/usb/core/generic.c | 3 ++-
 drivers/usb/core/quirks.c  | 3 +++
 include/linux/usb/quirks.h | 3 +++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 740342a2812a..ea770b83d876 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -22,6 +22,7 @@
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <uapi/linux/usb/audio.h>
+#include <linux/usb/quirks.h>
 #include "usb.h"
 
 static inline const char *plural(int n)
@@ -256,7 +257,7 @@ void usb_generic_driver_disconnect(struct usb_device *udev)
 
 	/* if this is only an unbind, not a physical disconnect, then
 	 * unconfigure the device */
-	if (udev->actconfig)
+	if (!(udev->quirks & USB_QUIRK_SKIP_UNCONFIGURE) && udev->actconfig)
 		usb_set_configuration(udev, -1);
 }
 
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index d3c14b5ed4a1..13989629d743 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -510,6 +510,9 @@ static const struct usb_device_id usb_quirk_list[] = {
 	/* INTEL VALUE SSD */
 	{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
 
+	/* CSR Bluetooth */
+	{ USB_DEVICE(0x0a12, 0x0001), .driver_info = USB_QUIRK_SKIP_UNCONFIGURE },
+
 	{ }  /* terminating entry must be last */
 };
 
diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
index eeb7c2157c72..84ab0a369931 100644
--- a/include/linux/usb/quirks.h
+++ b/include/linux/usb/quirks.h
@@ -72,4 +72,7 @@
 /* device has endpoints that should be ignored */
 #define USB_QUIRK_ENDPOINT_IGNORE		BIT(15)
 
+/* device doesn't support unconfigure when unbound. */
+#define USB_QUIRK_SKIP_UNCONFIGURE		BIT(16)
+
 #endif /* __LINUX_USB_QUIRKS_H */
-- 
2.35.1

Re: [PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Greg KH]

On Mon, May 02, 2022 at 09:07:58AM +0200, Jose Ignacio Tornos Martinez wrote:
> Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
> Trust) hang when they are unbound from 'unbind' sysfs entry and
> can not be bound again.
> 
> The reason is CSR chip hangs when usb configuration command with
> index 0 (used to unconfigure) is sent during disconnection.
> 
> To avoid this unwanted result, it is necessary not to send this
> command for CSR chip when usb device is unbound, so a new quirk
> has been created for this device.
> 
> Athough device is not unconfigured, it is better to avoid device
> hanging to be able to operate. Even bluetooth can be previously
> turned off.
> On the other hand, this is not important if usb device is going to
> be bound again (normal behavior), i.e. with usbip.
> ---
>  drivers/usb/core/generic.c | 3 ++-
>  drivers/usb/core/quirks.c  | 3 +++
>  include/linux/usb/quirks.h | 3 +++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index 740342a2812a..ea770b83d876 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -22,6 +22,7 @@
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
>  #include <uapi/linux/usb/audio.h>
> +#include <linux/usb/quirks.h>

Shouldn't that be above the uapi include?

>  #include "usb.h"
>  
>  static inline const char *plural(int n)
> @@ -256,7 +257,7 @@ void usb_generic_driver_disconnect(struct usb_device *udev)
>  
>  	/* if this is only an unbind, not a physical disconnect, then
>  	 * unconfigure the device */
> -	if (udev->actconfig)
> +	if (!(udev->quirks & USB_QUIRK_SKIP_UNCONFIGURE) && udev->actconfig)
>  		usb_set_configuration(udev, -1);
>  }
>  
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index d3c14b5ed4a1..13989629d743 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -510,6 +510,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>  	/* INTEL VALUE SSD */
>  	{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
>  
> +	/* CSR Bluetooth */
> +	{ USB_DEVICE(0x0a12, 0x0001), .driver_info = USB_QUIRK_SKIP_UNCONFIGURE },

Please read the big comment at the top of this list of entries.  It says
it must be in sorder order, which this is not :(

thanks,

greg k-h

Re: [PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Greg KH]

On Mon, May 02, 2022 at 09:07:58AM +0200, Jose Ignacio Tornos Martinez wrote:
> Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
> Trust) hang when they are unbound from 'unbind' sysfs entry and
> can not be bound again.
> 
> The reason is CSR chip hangs when usb configuration command with
> index 0 (used to unconfigure) is sent during disconnection.
> 
> To avoid this unwanted result, it is necessary not to send this
> command for CSR chip when usb device is unbound, so a new quirk
> has been created for this device.
> 
> Athough device is not unconfigured, it is better to avoid device
> hanging to be able to operate. Even bluetooth can be previously
> turned off.
> On the other hand, this is not important if usb device is going to
> be bound again (normal behavior), i.e. with usbip.
> ---
>  drivers/usb/core/generic.c | 3 ++-
>  drivers/usb/core/quirks.c  | 3 +++
>  include/linux/usb/quirks.h | 3 +++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index 740342a2812a..ea770b83d876 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -22,6 +22,7 @@
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
>  #include <uapi/linux/usb/audio.h>
> +#include <linux/usb/quirks.h>
>  #include "usb.h"
>  
>  static inline const char *plural(int n)
> @@ -256,7 +257,7 @@ void usb_generic_driver_disconnect(struct usb_device *udev)
>  
>  	/* if this is only an unbind, not a physical disconnect, then
>  	 * unconfigure the device */
> -	if (udev->actconfig)
> +	if (!(udev->quirks & USB_QUIRK_SKIP_UNCONFIGURE) && udev->actconfig)
>  		usb_set_configuration(udev, -1);
>  }
>  
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index d3c14b5ed4a1..13989629d743 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -510,6 +510,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>  	/* INTEL VALUE SSD */
>  	{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
>  
> +	/* CSR Bluetooth */
> +	{ USB_DEVICE(0x0a12, 0x0001), .driver_info = USB_QUIRK_SKIP_UNCONFIGURE },
> +
>  	{ }  /* terminating entry must be last */
>  };
>  
> diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> index eeb7c2157c72..84ab0a369931 100644
> --- a/include/linux/usb/quirks.h
> +++ b/include/linux/usb/quirks.h
> @@ -72,4 +72,7 @@
>  /* device has endpoints that should be ignored */
>  #define USB_QUIRK_ENDPOINT_IGNORE		BIT(15)
>  
> +/* device doesn't support unconfigure when unbound. */
> +#define USB_QUIRK_SKIP_UNCONFIGURE		BIT(16)
> +
>  #endif /* __LINUX_USB_QUIRKS_H */
> -- 
> 2.35.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch does not have a Signed-off-by: line.  Please read the
  kernel file, Documentation/SubmittingPatches and resend it after
  adding that line.  Note, the line needs to be in the body of the
  email, before the patch, not at the bottom of the patch or in the
  email signature.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Greg KH]

On Mon, May 02, 2022 at 09:07:58AM +0200, Jose Ignacio Tornos Martinez wrote:
> Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
> Trust) hang when they are unbound from 'unbind' sysfs entry and
> can not be bound again.
> 
> The reason is CSR chip hangs when usb configuration command with
> index 0 (used to unconfigure) is sent during disconnection.
> 
> To avoid this unwanted result, it is necessary not to send this
> command for CSR chip when usb device is unbound, so a new quirk
> has been created for this device.
> 
> Athough device is not unconfigured, it is better to avoid device
> hanging to be able to operate. Even bluetooth can be previously
> turned off.
> On the other hand, this is not important if usb device is going to
> be bound again (normal behavior), i.e. with usbip.
> ---

Also, the subject is totally wrong :(

Re: [PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Alan Stern]

On Mon, May 02, 2022 at 09:07:58AM +0200, Jose Ignacio Tornos Martinez wrote:
> Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
> Trust) hang when they are unbound from 'unbind' sysfs entry and
> can not be bound again.
> 
> The reason is CSR chip hangs when usb configuration command with
> index 0 (used to unconfigure) is sent during disconnection.
> 
> To avoid this unwanted result, it is necessary not to send this
> command for CSR chip when usb device is unbound, so a new quirk
> has been created for this device.
> 
> Athough device is not unconfigured, it is better to avoid device
> hanging to be able to operate. Even bluetooth can be previously
> turned off.
> On the other hand, this is not important if usb device is going to
> be bound again (normal behavior), i.e. with usbip.
> ---
>  drivers/usb/core/generic.c | 3 ++-
>  drivers/usb/core/quirks.c  | 3 +++
>  include/linux/usb/quirks.h | 3 +++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> index 740342a2812a..ea770b83d876 100644
> --- a/drivers/usb/core/generic.c
> +++ b/drivers/usb/core/generic.c
> @@ -22,6 +22,7 @@
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
>  #include <uapi/linux/usb/audio.h>
> +#include <linux/usb/quirks.h>
>  #include "usb.h"
>  
>  static inline const char *plural(int n)
> @@ -256,7 +257,7 @@ void usb_generic_driver_disconnect(struct usb_device *udev)
>  
>  	/* if this is only an unbind, not a physical disconnect, then
>  	 * unconfigure the device */
> -	if (udev->actconfig)
> +	if (!(udev->quirks & USB_QUIRK_SKIP_UNCONFIGURE) && udev->actconfig)

The usual programming convention is that when an "if" statement tests 
more than one condition, the test which is more likely to fail should 
come first.  In this case, it is more likely that udev->actconfig is 
non-NULL than that the USB_QUIRK_SKIP_UNCONFIGURE flag is set, because 
the quirk flag affects only one type of device whereas udev->actconfig 
can be non-NULL on any device.  So the test of udev->actconfig should 
come first.

>  		usb_set_configuration(udev, -1);
>  }
>  
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index d3c14b5ed4a1..13989629d743 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -510,6 +510,9 @@ static const struct usb_device_id usb_quirk_list[] = {
>  	/* INTEL VALUE SSD */
>  	{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
>  
> +	/* CSR Bluetooth */
> +	{ USB_DEVICE(0x0a12, 0x0001), .driver_info = USB_QUIRK_SKIP_UNCONFIGURE },
> +
>  	{ }  /* terminating entry must be last */
>  };

Did you want people to be able to test this quirk interactively?  If you 
do, you should add a flag character for this quirk to quirks_param_set() 
and document it.  But if you don't think people will need to test the 
new quirk then you don't need to add anything else.

> diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> index eeb7c2157c72..84ab0a369931 100644
> --- a/include/linux/usb/quirks.h
> +++ b/include/linux/usb/quirks.h
> @@ -72,4 +72,7 @@
>  /* device has endpoints that should be ignored */
>  #define USB_QUIRK_ENDPOINT_IGNORE		BIT(15)
>  
> +/* device doesn't support unconfigure when unbound. */
> +#define USB_QUIRK_SKIP_UNCONFIGURE		BIT(16)

The comment isn't right.  In fact, the device doesn't ever support 
unconfigure.  Whether it is bound or not makes no difference.

Alan Stern

Re: [PATCH] Bluetooth: btusb: CSR chip hangs when unbound

[Jose Ignacio Tornos Martinez]

Thanks everyone for your help.
I will prepare a new better patch.

Best regards
José Ignacio

On Mon, May 2, 2022 at 4:22 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, May 02, 2022 at 09:07:58AM +0200, Jose Ignacio Tornos Martinez wrote:
> > Bluetooth Dongles with CSR chip (i.e. USB Bluetooth V4.0 Dongle by
> > Trust) hang when they are unbound from 'unbind' sysfs entry and
> > can not be bound again.
> >
> > The reason is CSR chip hangs when usb configuration command with
> > index 0 (used to unconfigure) is sent during disconnection.
> >
> > To avoid this unwanted result, it is necessary not to send this
> > command for CSR chip when usb device is unbound, so a new quirk
> > has been created for this device.
> >
> > Athough device is not unconfigured, it is better to avoid device
> > hanging to be able to operate. Even bluetooth can be previously
> > turned off.
> > On the other hand, this is not important if usb device is going to
> > be bound again (normal behavior), i.e. with usbip.
> > ---
> >  drivers/usb/core/generic.c | 3 ++-
> >  drivers/usb/core/quirks.c  | 3 +++
> >  include/linux/usb/quirks.h | 3 +++
> >  3 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
> > index 740342a2812a..ea770b83d876 100644
> > --- a/drivers/usb/core/generic.c
> > +++ b/drivers/usb/core/generic.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/usb.h>
> >  #include <linux/usb/hcd.h>
> >  #include <uapi/linux/usb/audio.h>
> > +#include <linux/usb/quirks.h>
> >  #include "usb.h"
> >
> >  static inline const char *plural(int n)
> > @@ -256,7 +257,7 @@ void usb_generic_driver_disconnect(struct usb_device *udev)
> >
> >       /* if this is only an unbind, not a physical disconnect, then
> >        * unconfigure the device */
> > -     if (udev->actconfig)
> > +     if (!(udev->quirks & USB_QUIRK_SKIP_UNCONFIGURE) && udev->actconfig)
>
> The usual programming convention is that when an "if" statement tests
> more than one condition, the test which is more likely to fail should
> come first.  In this case, it is more likely that udev->actconfig is
> non-NULL than that the USB_QUIRK_SKIP_UNCONFIGURE flag is set, because
> the quirk flag affects only one type of device whereas udev->actconfig
> can be non-NULL on any device.  So the test of udev->actconfig should
> come first.
>
> >               usb_set_configuration(udev, -1);
> >  }
> >
> > diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> > index d3c14b5ed4a1..13989629d743 100644
> > --- a/drivers/usb/core/quirks.c
> > +++ b/drivers/usb/core/quirks.c
> > @@ -510,6 +510,9 @@ static const struct usb_device_id usb_quirk_list[] = {
> >       /* INTEL VALUE SSD */
> >       { USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
> >
> > +     /* CSR Bluetooth */
> > +     { USB_DEVICE(0x0a12, 0x0001), .driver_info = USB_QUIRK_SKIP_UNCONFIGURE },
> > +
> >       { }  /* terminating entry must be last */
> >  };
>
> Did you want people to be able to test this quirk interactively?  If you
> do, you should add a flag character for this quirk to quirks_param_set()
> and document it.  But if you don't think people will need to test the
> new quirk then you don't need to add anything else.
>
> > diff --git a/include/linux/usb/quirks.h b/include/linux/usb/quirks.h
> > index eeb7c2157c72..84ab0a369931 100644
> > --- a/include/linux/usb/quirks.h
> > +++ b/include/linux/usb/quirks.h
> > @@ -72,4 +72,7 @@
> >  /* device has endpoints that should be ignored */
> >  #define USB_QUIRK_ENDPOINT_IGNORE            BIT(15)
> >
> > +/* device doesn't support unconfigure when unbound. */
> > +#define USB_QUIRK_SKIP_UNCONFIGURE           BIT(16)
>
> The comment isn't right.  In fact, the device doesn't ever support
> unconfigure.  Whether it is bound or not makes no difference.
>
> Alan Stern
>