diff mbox series

[v2] Bluetooth: btusb: Fix regression with CSR controllers

Message ID 20241015153719.497388-1-luiz.dentz@gmail.com (mailing list archive)
State Accepted
Commit b29d4ac729754fa1b515a024386f50dadcaa8c7b
Headers show
Series [v2] Bluetooth: btusb: Fix regression with CSR controllers | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS

Commit Message

Luiz Augusto von Dentz Oct. 15, 2024, 3:37 p.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

CSR controllers don't seem to handle short-transfer properly which cause
command to timeout.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=219365
Fixes: 7b05933340f4 ("Bluetooth: btusb: Fix not handling ZPL/short-transfer")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/btusb.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Paul Menzel Oct. 16, 2024, 4:40 a.m. UTC | #1 
Dear Luiz,


Thank you for the patch, and fixing the regression. Could 
short-transfers be mentioned in the summary/title to make it more specific?


Am 15.10.24 um 17:37 schrieb Luiz Augusto von Dentz:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> CSR controllers don't seem to handle short-transfer properly which cause
> command to timeout.

The verb is spelled with a space: time out.

Could you elaborate more? Why is starting a quirk list the right thing 
to do? It’s unlikely more controllers are affected? For people using 
`git log --grep` the product name would be nice to have in the commit 
message:

Product: BT DONGLE10
Bus 001 Device 004: ID 0a12:0001 Cambridge Silicon Radio, Ltd Bluetooth 
Dongle (HCI mode)

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=219365
> Fixes: 7b05933340f4 ("Bluetooth: btusb: Fix not handling ZPL/short-transfer")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>   drivers/bluetooth/btusb.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index c0b6ef8ee5da..f72218c1037e 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1366,10 +1366,15 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
>   	if (!urb)
>   		return -ENOMEM;
>   
> -	/* Use maximum HCI Event size so the USB stack handles
> -	 * ZPL/short-transfer automatically.
> -	 */
> -	size = HCI_MAX_EVENT_SIZE;
> +	if (le16_to_cpu(data->udev->descriptor.idVendor)  == 0x0a12 &&
> +	    le16_to_cpu(data->udev->descriptor.idProduct) == 0x0001)
> +		/* Fake CSR devices don't seem to support sort-transter */

s*h*ort

> +		size = le16_to_cpu(data->intr_ep->wMaxPacketSize);

A warning would be nice to have so motivated users could bug Cambridge 
Silicon Radio to fix/improve their devices.

> +	else
> +		/* Use maximum HCI Event size so the USB stack handles
> +		 * ZPL/short-transfer automatically.
> +		 */
> +		size = HCI_MAX_EVENT_SIZE;
>   
>   	buf = kmalloc(size, mem_flags);
>   	if (!buf) {

Is Intel going to get such a device to add to a test lab?


Kind regards,

Paul
Luiz Augusto von Dentz Oct. 16, 2024, 2:46 p.m. UTC | #2 
Hi Paul,

On Wed, Oct 16, 2024 at 12:41 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Luiz,
>
>
> Thank you for the patch, and fixing the regression. Could
> short-transfers be mentioned in the summary/title to make it more specific?

Sure.

>
> Am 15.10.24 um 17:37 schrieb Luiz Augusto von Dentz:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > CSR controllers don't seem to handle short-transfer properly which cause
> > command to timeout.
>
> The verb is spelled with a space: time out.
>
> Could you elaborate more? Why is starting a quirk list the right thing
> to do? It’s unlikely more controllers are affected?

Elaborate on what? USB short-transfer is a mandatory feature of USB, I
can quote the USB spec if that makes it clearer, now if there are more
controllers affected only time will tell since I don't think anyone
has a lab to validate these changes on all supported models by btusb
driver.

> For people using
> `git log --grep` the product name would be nice to have in the commit
> message:
>
> Product: BT DONGLE10
> Bus 001 Device 004: ID 0a12:0001 Cambridge Silicon Radio, Ltd Bluetooth
> Dongle (HCI mode)

Ack

> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=219365
> > Fixes: 7b05933340f4 ("Bluetooth: btusb: Fix not handling ZPL/short-transfer")
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >   drivers/bluetooth/btusb.c | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index c0b6ef8ee5da..f72218c1037e 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -1366,10 +1366,15 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
> >       if (!urb)
> >               return -ENOMEM;
> >
> > -     /* Use maximum HCI Event size so the USB stack handles
> > -      * ZPL/short-transfer automatically.
> > -      */
> > -     size = HCI_MAX_EVENT_SIZE;
> > +     if (le16_to_cpu(data->udev->descriptor.idVendor)  == 0x0a12 &&
> > +         le16_to_cpu(data->udev->descriptor.idProduct) == 0x0001)
> > +             /* Fake CSR devices don't seem to support sort-transter */
>
> s*h*ort
>
> > +             size = le16_to_cpu(data->intr_ep->wMaxPacketSize);
>
> A warning would be nice to have so motivated users could bug Cambridge
> Silicon Radio to fix/improve their devices.

These are fake CSR dongles, there are many things wrong with them and
it is already warned when detected.

> > +     else
> > +             /* Use maximum HCI Event size so the USB stack handles
> > +              * ZPL/short-transfer automatically.
> > +              */
> > +             size = HCI_MAX_EVENT_SIZE;
> >
> >       buf = kmalloc(size, mem_flags);
> >       if (!buf) {
>
> Is Intel going to get such a device to add to a test lab?

Don't think we can get a hold of one of these dongles, they are not
really from CSR and they are really old as well, so I guess the only
option would be for someone to donate it or something.

>
> Kind regards,
>
> Paul
patchwork-bot+bluetooth@kernel.org Oct. 16, 2024, 8 p.m. UTC | #3 
Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Tue, 15 Oct 2024 11:37:19 -0400 you wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> CSR controllers don't seem to handle short-transfer properly which cause
> command to timeout.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=219365
> Fixes: 7b05933340f4 ("Bluetooth: btusb: Fix not handling ZPL/short-transfer")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> [...]

Here is the summary with links:
  - [v2] Bluetooth: btusb: Fix regression with CSR controllers
    https://git.kernel.org/bluetooth/bluetooth-next/c/b29d4ac72975

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index c0b6ef8ee5da..f72218c1037e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1366,10 +1366,15 @@  static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
 	if (!urb)
 		return -ENOMEM;
 
-	/* Use maximum HCI Event size so the USB stack handles
-	 * ZPL/short-transfer automatically.
-	 */
-	size = HCI_MAX_EVENT_SIZE;
+	if (le16_to_cpu(data->udev->descriptor.idVendor)  == 0x0a12 &&
+	    le16_to_cpu(data->udev->descriptor.idProduct) == 0x0001)
+		/* Fake CSR devices don't seem to support sort-transter */
+		size = le16_to_cpu(data->intr_ep->wMaxPacketSize);
+	else
+		/* Use maximum HCI Event size so the USB stack handles
+		 * ZPL/short-transfer automatically.
+		 */
+		size = HCI_MAX_EVENT_SIZE;
 
 	buf = kmalloc(size, mem_flags);
 	if (!buf) {