| Message ID | 20220429153138.935435-1-jtornosm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: btusb: CSR chip hangs when unbound | expand |
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
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
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 >
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)
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(-)