linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
@ 2021-07-16 17:58 Pauli Virtanen
  2021-07-16 19:16 ` bluez.test.bot
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Pauli Virtanen @ 2021-07-16 17:58 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Hilda Wu

Some USB BT adapters don't satisfy the MTU requirement mentioned in
commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
and have ALT 3 setting that produces no/garbled audio. Check that the MTU
condition is satisfied, and fall back to ALT 1 if not.

Tested with USB adapters (mtu<72, produce sound only with ALT1)
BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3)
RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling ALT6).

Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 drivers/bluetooth/btusb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a9855a2dd561..3ee66e415c4d 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1763,9 +1763,11 @@ static void btusb_work(struct work_struct *work)
 			/* Because mSBC frames do not need to be aligned to the
 			 * SCO packet boundary. If support the Alt 3, use the
 			 * Alt 3 for HCI payload >= 60 Bytes let air packet
-			 * data satisfy 60 bytes.
+			 * data satisfy 60 bytes. USB Alt 3 support also needs
+			 * HFP transparent MTU >= 72 Bytes.
 			 */
-			if (new_alts == 1 && btusb_find_altsetting(data, 3))
+			if (new_alts == 1 && hdev->sco_mtu >= 72 &&
+			    btusb_find_altsetting(data, 3))
 				new_alts = 3;
 		}
 
-- 
2.31.1



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

* RE: Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
  2021-07-16 17:58 [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS Pauli Virtanen
@ 2021-07-16 19:16 ` bluez.test.bot
  2021-07-17 13:34 ` [PATCH] " Joseph Hwang
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: bluez.test.bot @ 2021-07-16 19:16 UTC (permalink / raw)
  To: linux-bluetooth, pav

[-- Attachment #1: Type: text/plain, Size: 3366 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=516813

---Test result---

Test Summary:
CheckPatch                    FAIL      0.57 seconds
GitLint                       PASS      0.12 seconds
BuildKernel                   PASS      613.39 seconds
TestRunner: Setup             PASS      431.82 seconds
TestRunner: l2cap-tester      PASS      3.11 seconds
TestRunner: bnep-tester       PASS      2.17 seconds
TestRunner: mgmt-tester       PASS      34.16 seconds
TestRunner: rfcomm-tester     PASS      2.43 seconds
TestRunner: sco-tester        PASS      2.34 seconds
TestRunner: smp-tester        FAIL      2.48 seconds
TestRunner: userchan-tester   PASS      2.28 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.57 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
WARNING: Unknown commit id 'e848dbd364ac', maybe rebased or not pulled?
#8: 
commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")

total: 0 errors, 1 warnings, 13 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS - 0.12 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 613.39 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 431.82 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 3.11 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 2.17 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 34.16 seconds
Run test-runner with mgmt-tester
Total: 446, Passed: 443 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.43 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.34 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.48 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.028 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 2.28 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44350 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3557 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 614404 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11676 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9912 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11705 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5453 bytes --]

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

* Re: [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
  2021-07-16 17:58 [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS Pauli Virtanen
  2021-07-16 19:16 ` bluez.test.bot
@ 2021-07-17 13:34 ` Joseph Hwang
  2021-07-21 15:40   ` Pauli Virtanen
  2021-07-20  9:15 ` Pasi Kärkkäinen
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Joseph Hwang @ 2021-07-17 13:34 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth, Hilda Wu

It looks reasonable to check sco_mtu. My suggestion is to use
hci_packet_size_usb_alt[3] instead of the magic number 72.

Thanks,
Joseph


On Sat, Jul 17, 2021 at 1:58 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Some USB BT adapters don't satisfy the MTU requirement mentioned in
> commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> and have ALT 3 setting that produces no/garbled audio. Check that the MTU
> condition is satisfied, and fall back to ALT 1 if not.
>
> Tested with USB adapters (mtu<72, produce sound only with ALT1)
> BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3)
> RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling ALT6).
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  drivers/bluetooth/btusb.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index a9855a2dd561..3ee66e415c4d 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1763,9 +1763,11 @@ static void btusb_work(struct work_struct *work)
>                         /* Because mSBC frames do not need to be aligned to the
>                          * SCO packet boundary. If support the Alt 3, use the
>                          * Alt 3 for HCI payload >= 60 Bytes let air packet
> -                        * data satisfy 60 bytes.
> +                        * data satisfy 60 bytes. USB Alt 3 support also needs
> +                        * HFP transparent MTU >= 72 Bytes.
>                          */
> -                       if (new_alts == 1 && btusb_find_altsetting(data, 3))
> +                       if (new_alts == 1 && hdev->sco_mtu >= 72 &&
> +                           btusb_find_altsetting(data, 3))
>                                 new_alts = 3;
>                 }
>
> --
> 2.31.1
>
>


-- 

Joseph Shyh-In Hwang
Email: josephsih@google.com

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

* Re: [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
  2021-07-16 17:58 [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS Pauli Virtanen
  2021-07-16 19:16 ` bluez.test.bot
  2021-07-17 13:34 ` [PATCH] " Joseph Hwang
@ 2021-07-20  9:15 ` Pasi Kärkkäinen
  2021-07-22 13:29 ` Michał Kępień
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Pasi Kärkkäinen @ 2021-07-20  9:15 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth, Hilda Wu

Hi Pauli,

On Fri, Jul 16, 2021 at 08:58:17PM +0300, Pauli Virtanen wrote:
> Some USB BT adapters don't satisfy the MTU requirement mentioned in
> commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> and have ALT 3 setting that produces no/garbled audio. Check that the MTU
> condition is satisfied, and fall back to ALT 1 if not.
> 
> Tested with USB adapters (mtu<72, produce sound only with ALT1)
> BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3)
> RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling ALT6).
> 
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>

Maybe add a Fixes: tag ?


Thanks,

-- Pasi

>  drivers/bluetooth/btusb.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index a9855a2dd561..3ee66e415c4d 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1763,9 +1763,11 @@ static void btusb_work(struct work_struct *work)
>  			/* Because mSBC frames do not need to be aligned to the
>  			 * SCO packet boundary. If support the Alt 3, use the
>  			 * Alt 3 for HCI payload >= 60 Bytes let air packet
> -			 * data satisfy 60 bytes.
> +			 * data satisfy 60 bytes. USB Alt 3 support also needs
> +			 * HFP transparent MTU >= 72 Bytes.
>  			 */
> -			if (new_alts == 1 && btusb_find_altsetting(data, 3))
> +			if (new_alts == 1 && hdev->sco_mtu >= 72 &&
> +			    btusb_find_altsetting(data, 3))
>  				new_alts = 3;
>  		}
>  
> -- 
> 2.31.1
> 
> 

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

* Re: [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
  2021-07-17 13:34 ` [PATCH] " Joseph Hwang
@ 2021-07-21 15:40   ` Pauli Virtanen
  2021-07-22 10:28     ` Hilda Wu
  0 siblings, 1 reply; 17+ messages in thread
From: Pauli Virtanen @ 2021-07-21 15:40 UTC (permalink / raw)
  To: Joseph Hwang; +Cc: linux-bluetooth, Hilda Wu

Hi,

la, 2021-07-17 kello 21:34 +0800, Joseph Hwang kirjoitti:
> It looks reasonable to check sco_mtu. My suggestion is to use
> hci_packet_size_usb_alt[3] instead of the magic number 72.

Thanks for looking at it. IIUC, hci_packet_size_usb_alt only appears in
previous discussions on this list, which did not seem to reach a
conclusion if this should be computed from wMaxPacketSize. I can do
less magical v2 and add the table.

Different solution would be to make the altsetting choice
manufacturer/device specific flag, as I'm not sure on what hardware
e848dbd364ac was tested on, so maybe it is intended Realtek specific.

Best,
Pauli


> On Sat, Jul 17, 2021 at 1:58 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Some USB BT adapters don't satisfy the MTU requirement mentioned in
> > commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> > and have ALT 3 setting that produces no/garbled audio. Check that the MTU
> > condition is satisfied, and fall back to ALT 1 if not.
> > 
> > Tested with USB adapters (mtu<72, produce sound only with ALT1)
> > BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3)
> > RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling ALT6).
> > 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >  drivers/bluetooth/btusb.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index a9855a2dd561..3ee66e415c4d 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -1763,9 +1763,11 @@ static void btusb_work(struct work_struct *work)
> >                         /* Because mSBC frames do not need to be aligned to the
> >                          * SCO packet boundary. If support the Alt 3, use the
> >                          * Alt 3 for HCI payload >= 60 Bytes let air packet
> > -                        * data satisfy 60 bytes.
> > +                        * data satisfy 60 bytes. USB Alt 3 support also needs
> > +                        * HFP transparent MTU >= 72 Bytes.
> >                          */
> > -                       if (new_alts == 1 && btusb_find_altsetting(data, 3))
> > +                       if (new_alts == 1 && hdev->sco_mtu >= 72 &&
> > +                           btusb_find_altsetting(data, 3))
> >                                 new_alts = 3;
> >                 }
> > 
> > --
> > 2.31.1
> > 
> > 
> 
> 



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

* RE: [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
  2021-07-21 15:40   ` Pauli Virtanen
@ 2021-07-22 10:28     ` Hilda Wu
  0 siblings, 0 replies; 17+ messages in thread
From: Hilda Wu @ 2021-07-22 10:28 UTC (permalink / raw)
  To: Pauli Virtanen, Joseph Hwang; +Cc: linux-bluetooth, Max Chou, alex_lu

Hi,

Thanks for your supplement.
Add the sco_mtu condition is better.

When USB BT adapters support the alternate 3, it will announce alternate 3.
However, USB Alt 3 supported also need HFP support transparent MTU in 72 Bytes.
Add this sco_mtu condition could avoid HFP profile transparent MTU not match or not suitable ALT 3.

Regards,
Hilda

-----Original Message-----
From: Pauli Virtanen <pav@iki.fi> 
Sent: Wednesday, July 21, 2021 11:40 PM
To: Joseph Hwang <josephsih@google.com>
Cc: linux-bluetooth <linux-bluetooth@vger.kernel.org>; Hilda Wu <hildawu@realtek.com>
Subject: Re: [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS

Hi,

la, 2021-07-17 kello 21:34 +0800, Joseph Hwang kirjoitti:
> It looks reasonable to check sco_mtu. My suggestion is to use 
> hci_packet_size_usb_alt[3] instead of the magic number 72.

Thanks for looking at it. IIUC, hci_packet_size_usb_alt only appears in previous discussions on this list, which did not seem to reach a conclusion if this should be computed from wMaxPacketSize. I can do less magical v2 and add the table.

Different solution would be to make the altsetting choice manufacturer/device specific flag, as I'm not sure on what hardware e848dbd364ac was tested on, so maybe it is intended Realtek specific.

Best,
Pauli


> On Sat, Jul 17, 2021 at 1:58 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Some USB BT adapters don't satisfy the MTU requirement mentioned in 
> > commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for 
> > WBS") and have ALT 3 setting that produces no/garbled audio. Check 
> > that the MTU condition is satisfied, and fall back to ALT 1 if not.
> > 
> > Tested with USB adapters (mtu<72, produce sound only with ALT1)
> > BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3) 
> > RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling ALT6).
> > 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> >  drivers/bluetooth/btusb.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c 
> > index a9855a2dd561..3ee66e415c4d 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -1763,9 +1763,11 @@ static void btusb_work(struct work_struct *work)
> >                         /* Because mSBC frames do not need to be aligned to the
> >                          * SCO packet boundary. If support the Alt 3, use the
> >                          * Alt 3 for HCI payload >= 60 Bytes let air packet
> > -                        * data satisfy 60 bytes.
> > +                        * data satisfy 60 bytes. USB Alt 3 support also needs
> > +                        * HFP transparent MTU >= 72 Bytes.
> >                          */
> > -                       if (new_alts == 1 && btusb_find_altsetting(data, 3))
> > +                       if (new_alts == 1 && hdev->sco_mtu >= 72 &&
> > +                           btusb_find_altsetting(data, 3))
> >                                 new_alts = 3;
> >                 }
> > 
> > --
> > 2.31.1
> > 
> > 
> 
> 


------Please consider the environment before printing this e-mail.

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

* Re: [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
  2021-07-16 17:58 [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS Pauli Virtanen
                   ` (2 preceding siblings ...)
  2021-07-20  9:15 ` Pasi Kärkkäinen
@ 2021-07-22 13:29 ` Michał Kępień
  2021-07-22 14:26   ` Pauli Virtanen
  2021-07-23 10:07 ` [PATCH v2] Bluetooth: btusb: check conditions " Pauli Virtanen
  2021-07-26 18:02 ` [PATCH v3] " Pauli Virtanen
  5 siblings, 1 reply; 17+ messages in thread
From: Michał Kępień @ 2021-07-22 13:29 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth, Hilda Wu

Hi Pauli,

Thank you for your work on this patch!

> Some USB BT adapters don't satisfy the MTU requirement mentioned in
> commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> and have ALT 3 setting that produces no/garbled audio. Check that the MTU
> condition is satisfied, and fall back to ALT 1 if not.
> 
> Tested with USB adapters (mtu<72, produce sound only with ALT1)
> BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3)
> RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling ALT6).

Commit e848dbd364aca44c9d23c04bef964fab79e2b34f causes garbled mSBC
audio on my Bluetooth adapter (8087:0a2b), so I gave your patch a try.

Unfortunately, the patch in its current shape does not seem to be enough
to fix things for this adapter, which reports an SCO MTU of 96 bytes and
produces garbled mSBC audio with ALT 3.

For the record, reverting e848dbd364aca44c9d23c04bef964fab79e2b34f on
top of v5.14-rc2 makes mSBC work again with my adapter.

I am no Bluetooth expert, but I will be happy to perform any additional
debugging and/or testing.

-- 
Best regards,
Michał Kępień

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

* Re: [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
  2021-07-22 13:29 ` Michał Kępień
@ 2021-07-22 14:26   ` Pauli Virtanen
  2021-07-22 14:42     ` Michał Kępień
  0 siblings, 1 reply; 17+ messages in thread
From: Pauli Virtanen @ 2021-07-22 14:26 UTC (permalink / raw)
  To: Michał Kępień; +Cc: linux-bluetooth

Hi Michał,

to, 2021-07-22 kello 15:29 +0200, Michał Kępień kirjoitti:
> Thank you for your work on this patch!
> 
> > Some USB BT adapters don't satisfy the MTU requirement mentioned in
> > commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> > and have ALT 3 setting that produces no/garbled audio. Check that the MTU
> > condition is satisfied, and fall back to ALT 1 if not.
> > 
> > Tested with USB adapters (mtu<72, produce sound only with ALT1)
> > BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3)
> > RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling ALT6).
> 
> Commit e848dbd364aca44c9d23c04bef964fab79e2b34f causes garbled mSBC
> audio on my Bluetooth adapter (8087:0a2b), so I gave your patch a try.
> 
> Unfortunately, the patch in its current shape does not seem to be enough
> to fix things for this adapter, which reports an SCO MTU of 96 bytes and
> produces garbled mSBC audio with ALT 3.
> 
> For the record, reverting e848dbd364aca44c9d23c04bef964fab79e2b34f on
> top of v5.14-rc2 makes mSBC work again with my adapter.
> 
> I am no Bluetooth expert, but I will be happy to perform any additional
> debugging and/or testing.

Thanks for testing! What userspace software did you use? Pulseaudio at
least I think had assumption that mtu <= 60, which breaks here
(Pipewire works, don't know about bluez-alsa). If otherwise, maybe it
then needs also the manufacturer flag.

Best,
Pauli


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

* Re: [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS
  2021-07-22 14:26   ` Pauli Virtanen
@ 2021-07-22 14:42     ` Michał Kępień
  0 siblings, 0 replies; 17+ messages in thread
From: Michał Kępień @ 2021-07-22 14:42 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth

Pauli,

> Thanks for testing! What userspace software did you use? Pulseaudio at
> least I think had assumption that mtu <= 60, which breaks here
> (Pipewire works, don't know about bluez-alsa). If otherwise, maybe it
> then needs also the manufacturer flag.

I am using Pipewire 0.3.32.

When I wrote "this adapter (...) reports an SCO MTU of 96 bytes" in my
previous message, I meant that this was the value present in
hdev->sco_mtu around the problematic code location.  If this was the
wrong thing to take a look at, please let me know.

-- 
Best regards,
Michał Kępień

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

* [PATCH v2] Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
  2021-07-16 17:58 [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS Pauli Virtanen
                   ` (3 preceding siblings ...)
  2021-07-22 13:29 ` Michał Kępień
@ 2021-07-23 10:07 ` Pauli Virtanen
  2021-07-23 12:19   ` Marcel Holtmann
  2021-07-26 18:02 ` [PATCH v3] " Pauli Virtanen
  5 siblings, 1 reply; 17+ messages in thread
From: Pauli Virtanen @ 2021-07-23 10:07 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: Joseph Hwang, Hilda Wu, Marcel Holtmann, Johan Hedberg,
	Luiz Augusto von Dentz, Michał Kępień

Some USB BT adapters don't satisfy the MTU requirement mentioned in
commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
and have ALT 3 setting that produces no/garbled audio. Some adapters
with larger MTU were also reported to have problems with ALT 3.

Add a flag and check it and MTU before selecting ALT 3, falling back to
ALT 1. Enable the flag for Realtek, restoring the previous behavior for
non-Realtek devices.

Tested with USB adapters (mtu<72, no/garbled sound with ALT3, ALT1
works) BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3
works) RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling
ALT6). Also got reports for (mtu>=72, ALT 3 reported to produce bad
audio) Intel 8087:0a2b.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
Fixes: e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
---

Changes in v2:
- Explain magic number 72 in a comment; didn't add the table for them,
  because it's not used elsewhere and we need just one number from it.
- Add flag for ALT3 support, restoring the behavior
  for non-Realtek devices the same as before e848dbd364ac, due to
  the problems reported on an Intel adapter. Don't have the device
  myself.

 drivers/bluetooth/btusb.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 6d23308119d1..5cec719f6cba 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -516,6 +516,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
 #define BTUSB_HW_RESET_ACTIVE	12
 #define BTUSB_TX_WAIT_VND_EVT	13
 #define BTUSB_WAKEUP_DISABLE	14
+#define BTUSB_ALT3_OK_FOR_WBS	15
 
 struct btusb_data {
 	struct hci_dev       *hdev;
@@ -1748,16 +1749,20 @@ static void btusb_work(struct work_struct *work)
 			/* Bluetooth USB spec recommends alt 6 (63 bytes), but
 			 * many adapters do not support it.  Alt 1 appears to
 			 * work for all adapters that do not have alt 6, and
-			 * which work with WBS at all.
+			 * which work with WBS at all.  Some devices prefer
+			 * alt 3 (HCI payload >= 60 Bytes let air packet
+			 * data satisfy 60 bytes), requiring
+			 * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
+			 * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
 			 */
-			new_alts = btusb_find_altsetting(data, 6) ? 6 : 1;
-			/* Because mSBC frames do not need to be aligned to the
-			 * SCO packet boundary. If support the Alt 3, use the
-			 * Alt 3 for HCI payload >= 60 Bytes let air packet
-			 * data satisfy 60 bytes.
-			 */
-			if (new_alts == 1 && btusb_find_altsetting(data, 3))
+			if (btusb_find_altsetting(data, 6))
+				new_alts = 6;
+			else if (test_bit(BTUSB_ALT3_OK_FOR_WBS, &data->flags) &&
+				 hdev->sco_mtu >= 72 &&
+				 btusb_find_altsetting(data, 3))
 				new_alts = 3;
+			else
+				new_alts = 1;
 		}
 
 		if (btusb_switch_alt_setting(hdev, new_alts) < 0)
@@ -4733,6 +4738,7 @@ static int btusb_probe(struct usb_interface *intf,
 		 * (DEVICE_REMOTE_WAKEUP)
 		 */
 		set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
+		set_bit(BTUSB_ALT3_OK_FOR_WBS, &data->flags);
 	}
 
 	if (!reset)
-- 
2.31.1




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

* Re: [PATCH v2] Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
  2021-07-23 10:07 ` [PATCH v2] Bluetooth: btusb: check conditions " Pauli Virtanen
@ 2021-07-23 12:19   ` Marcel Holtmann
  2021-07-25  5:47     ` Michał Kępień
  2021-08-10 16:48     ` Pasi Kärkkäinen
  0 siblings, 2 replies; 17+ messages in thread
From: Marcel Holtmann @ 2021-07-23 12:19 UTC (permalink / raw)
  To: Pauli Virtanen
  Cc: linux-bluetooth, Joseph Hwang, Hilda Wu, Johan Hedberg,
	Luiz Augusto von Dentz, Michał Kępień

Hi Pauli,

> Some USB BT adapters don't satisfy the MTU requirement mentioned in
> commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> and have ALT 3 setting that produces no/garbled audio. Some adapters
> with larger MTU were also reported to have problems with ALT 3.
> 
> Add a flag and check it and MTU before selecting ALT 3, falling back to
> ALT 1. Enable the flag for Realtek, restoring the previous behavior for
> non-Realtek devices.
> 
> Tested with USB adapters (mtu<72, no/garbled sound with ALT3, ALT1
> works) BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3
> works) RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling
> ALT6). Also got reports for (mtu>=72, ALT 3 reported to produce bad
> audio) Intel 8087:0a2b.
> 
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> Fixes: e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")

before I will apply this, I need Tested-by or Ack-by people that confirm that this fixes their issues now.

> ---
> 
> Changes in v2:
> - Explain magic number 72 in a comment; didn't add the table for them,
>  because it's not used elsewhere and we need just one number from it.
> - Add flag for ALT3 support, restoring the behavior
>  for non-Realtek devices the same as before e848dbd364ac, due to
>  the problems reported on an Intel adapter. Don't have the device
>  myself.
> 
> drivers/bluetooth/btusb.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 6d23308119d1..5cec719f6cba 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -516,6 +516,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
> #define BTUSB_HW_RESET_ACTIVE	12
> #define BTUSB_TX_WAIT_VND_EVT	13
> #define BTUSB_WAKEUP_DISABLE	14
> +#define BTUSB_ALT3_OK_FOR_WBS	15

Rename this to BTUSB_USE_ALT3_FOR_WBS.

> 
> struct btusb_data {
> 	struct hci_dev       *hdev;
> @@ -1748,16 +1749,20 @@ static void btusb_work(struct work_struct *work)
> 			/* Bluetooth USB spec recommends alt 6 (63 bytes), but
> 			 * many adapters do not support it.  Alt 1 appears to
> 			 * work for all adapters that do not have alt 6, and
> -			 * which work with WBS at all.
> +			 * which work with WBS at all.  Some devices prefer
> +			 * alt 3 (HCI payload >= 60 Bytes let air packet
> +			 * data satisfy 60 bytes), requiring
> +			 * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
> +			 * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
> 			 */
> -			new_alts = btusb_find_altsetting(data, 6) ? 6 : 1;
> -			/* Because mSBC frames do not need to be aligned to the
> -			 * SCO packet boundary. If support the Alt 3, use the
> -			 * Alt 3 for HCI payload >= 60 Bytes let air packet
> -			 * data satisfy 60 bytes.
> -			 */
> -			if (new_alts == 1 && btusb_find_altsetting(data, 3))
> +			if (btusb_find_altsetting(data, 6))
> +				new_alts = 6;
> +			else if (test_bit(BTUSB_ALT3_OK_FOR_WBS, &data->flags) &&
> +				 hdev->sco_mtu >= 72 &&
> +				 btusb_find_altsetting(data, 3))

This is whitespace damaged.

> 				new_alts = 3;
> +			else
> +				new_alts = 1;
> 		}
> 
> 		if (btusb_switch_alt_setting(hdev, new_alts) < 0)
> @@ -4733,6 +4738,7 @@ static int btusb_probe(struct usb_interface *intf,
> 		 * (DEVICE_REMOTE_WAKEUP)
> 		 */
> 		set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
> +		set_bit(BTUSB_ALT3_OK_FOR_WBS, &data->flags);
> 	}

Regards

Marcel


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

* Re: [PATCH v2] Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
  2021-07-23 12:19   ` Marcel Holtmann
@ 2021-07-25  5:47     ` Michał Kępień
  2021-08-10 16:48     ` Pasi Kärkkäinen
  1 sibling, 0 replies; 17+ messages in thread
From: Michał Kępień @ 2021-07-25  5:47 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Pauli Virtanen, linux-bluetooth, Joseph Hwang, Hilda Wu,
	Johan Hedberg, Luiz Augusto von Dentz

> > Some USB BT adapters don't satisfy the MTU requirement mentioned in
> > commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> > and have ALT 3 setting that produces no/garbled audio. Some adapters
> > with larger MTU were also reported to have problems with ALT 3.
> > 
> > Add a flag and check it and MTU before selecting ALT 3, falling back to
> > ALT 1. Enable the flag for Realtek, restoring the previous behavior for
> > non-Realtek devices.
> > 
> > Tested with USB adapters (mtu<72, no/garbled sound with ALT3, ALT1
> > works) BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3
> > works) RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling
> > ALT6). Also got reports for (mtu>=72, ALT 3 reported to produce bad
> > audio) Intel 8087:0a2b.
> > 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > Fixes: e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> 
> before I will apply this, I need Tested-by or Ack-by people that confirm that this fixes their issues now.

For 8087:0a2b (with broken mSBC audio since e848dbd364ac):

Tested-by: Michał Kępień <kernel@kempniu.pl>

It would be nice to get this into the stable tree as e848dbd364ac got
there in v5.13.3 (as 15407b14e27b) and v5.10.51 (as 05298f1733c6).

Thanks!

-- 
Best regards,
Michał Kępień

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

* [PATCH v3] Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
  2021-07-16 17:58 [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS Pauli Virtanen
                   ` (4 preceding siblings ...)
  2021-07-23 10:07 ` [PATCH v2] Bluetooth: btusb: check conditions " Pauli Virtanen
@ 2021-07-26 18:02 ` Pauli Virtanen
  2021-07-27  2:11   ` [v3] " bluez.test.bot
  2021-07-30 12:35   ` [PATCH v3] " Pasi Kärkkäinen
  5 siblings, 2 replies; 17+ messages in thread
From: Pauli Virtanen @ 2021-07-26 18:02 UTC (permalink / raw)
  To: marcel
  Cc: johan.hedberg, kernel, linux-bluetooth, luiz.dentz, pav, hildawu,
	josephsih

Some USB BT adapters don't satisfy the MTU requirement mentioned in
commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
and have ALT 3 setting that produces no/garbled audio. Some adapters
with larger MTU were also reported to have problems with ALT 3.

Add a flag and check it and MTU before selecting ALT 3, falling back to
ALT 1. Enable the flag for Realtek, restoring the previous behavior for
non-Realtek devices.

Tested with USB adapters (mtu<72, no/garbled sound with ALT3, ALT1
works) BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3
works) RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling
ALT6). Also got reports for (mtu>=72, ALT 3 reported to produce bad
audio) Intel 8087:0a2b.

Signed-off-by: Pauli Virtanen <pav@iki.fi>
Fixes: e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
Tested-by: Michał Kępień <kernel@kempniu.pl>
---

Changes in v3:
- Rename flag to BTUSB_USE_ALT3_FOR_WBS.
- No spaces in indent.
- Added Tested-by: Michał Kępień

Changes in v2:
- Explain magic number 72 in a comment; didn't add the table for them,
  because it's not used elsewhere and we need just one number from it.
- Add flag for ALT3 support, restoring the behavior
  for non-Realtek devices the same as before e848dbd364ac, due to
  the problems reported on an Intel adapter. Don't have the device
  myself.

 drivers/bluetooth/btusb.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 6d23308119d1..e8062d0b7d4d 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -516,6 +516,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
 #define BTUSB_HW_RESET_ACTIVE	12
 #define BTUSB_TX_WAIT_VND_EVT	13
 #define BTUSB_WAKEUP_DISABLE	14
+#define BTUSB_USE_ALT3_FOR_WBS	15
 
 struct btusb_data {
 	struct hci_dev       *hdev;
@@ -1748,16 +1749,20 @@ static void btusb_work(struct work_struct *work)
 			/* Bluetooth USB spec recommends alt 6 (63 bytes), but
 			 * many adapters do not support it.  Alt 1 appears to
 			 * work for all adapters that do not have alt 6, and
-			 * which work with WBS at all.
+			 * which work with WBS at all.  Some devices prefer
+			 * alt 3 (HCI payload >= 60 Bytes let air packet
+			 * data satisfy 60 bytes), requiring
+			 * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
+			 * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
 			 */
-			new_alts = btusb_find_altsetting(data, 6) ? 6 : 1;
-			/* Because mSBC frames do not need to be aligned to the
-			 * SCO packet boundary. If support the Alt 3, use the
-			 * Alt 3 for HCI payload >= 60 Bytes let air packet
-			 * data satisfy 60 bytes.
-			 */
-			if (new_alts == 1 && btusb_find_altsetting(data, 3))
+			if (btusb_find_altsetting(data, 6))
+				new_alts = 6;
+			else if (test_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags) &&
+					hdev->sco_mtu >= 72 &&
+					btusb_find_altsetting(data, 3))
 				new_alts = 3;
+			else
+				new_alts = 1;
 		}
 
 		if (btusb_switch_alt_setting(hdev, new_alts) < 0)
@@ -4733,6 +4738,7 @@ static int btusb_probe(struct usb_interface *intf,
 		 * (DEVICE_REMOTE_WAKEUP)
 		 */
 		set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
+		set_bit(BTUSB_USE_ALT3_FOR_WBS, &data->flags);
 	}
 
 	if (!reset)
-- 
2.31.1


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

* RE: [v3] Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
  2021-07-26 18:02 ` [PATCH v3] " Pauli Virtanen
@ 2021-07-27  2:11   ` bluez.test.bot
  2021-07-30 12:35   ` [PATCH v3] " Pasi Kärkkäinen
  1 sibling, 0 replies; 17+ messages in thread
From: bluez.test.bot @ 2021-07-27  2:11 UTC (permalink / raw)
  To: linux-bluetooth, pav

[-- Attachment #1: Type: text/plain, Size: 3369 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=521451

---Test result---

Test Summary:
CheckPatch                    FAIL      0.51 seconds
GitLint                       PASS      0.11 seconds
BuildKernel                   PASS      553.49 seconds
TestRunner: Setup             PASS      361.85 seconds
TestRunner: l2cap-tester      PASS      2.61 seconds
TestRunner: bnep-tester       PASS      2.01 seconds
TestRunner: mgmt-tester       PASS      31.71 seconds
TestRunner: rfcomm-tester     PASS      2.19 seconds
TestRunner: sco-tester        PASS      2.16 seconds
TestRunner: smp-tester        FAIL      2.18 seconds
TestRunner: userchan-tester   PASS      2.01 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.51 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
WARNING: Unknown commit id 'e848dbd364ac', maybe rebased or not pulled?
#11: 
commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")

total: 0 errors, 1 warnings, 42 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] Bluetooth: btusb: check conditions before enabling USB ALT 3" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS - 0.11 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 553.49 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 361.85 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.61 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 2.01 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 31.71 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.19 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.16 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.18 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.023 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 2.01 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44386 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3593 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 616893 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11713 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9947 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11741 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5490 bytes --]

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

* Re: [PATCH v3] Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
  2021-07-26 18:02 ` [PATCH v3] " Pauli Virtanen
  2021-07-27  2:11   ` [v3] " bluez.test.bot
@ 2021-07-30 12:35   ` Pasi Kärkkäinen
  1 sibling, 0 replies; 17+ messages in thread
From: Pasi Kärkkäinen @ 2021-07-30 12:35 UTC (permalink / raw)
  To: Pauli Virtanen
  Cc: marcel, johan.hedberg, kernel, linux-bluetooth, luiz.dentz,
	hildawu, josephsih

Hi,

On Mon, Jul 26, 2021 at 09:02:06PM +0300, Pauli Virtanen wrote:
> Some USB BT adapters don't satisfy the MTU requirement mentioned in
> commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> and have ALT 3 setting that produces no/garbled audio. Some adapters
> with larger MTU were also reported to have problems with ALT 3.
> 
> Add a flag and check it and MTU before selecting ALT 3, falling back to
> ALT 1. Enable the flag for Realtek, restoring the previous behavior for
> non-Realtek devices.
> 
> Tested with USB adapters (mtu<72, no/garbled sound with ALT3, ALT1
> works) BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3
> works) RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling
> ALT6). Also got reports for (mtu>=72, ALT 3 reported to produce bad
> audio) Intel 8087:0a2b.
> 
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> Fixes: e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> Tested-by: Michał Kępień <kernel@kempniu.pl>
> ---
>

This probably also should have CC stable@kernel.org, as users have
started reporting this bug as distros have started shipping kernels with
the faulty patch in it.. so it'd be nice to have the fix backported to
stable kernel trees.


Thanks,

-- Pasi

> 
> Changes in v3:
> - Rename flag to BTUSB_USE_ALT3_FOR_WBS.
> - No spaces in indent.
> - Added Tested-by: Michał Kępień
> 
> Changes in v2:
> - Explain magic number 72 in a comment; didn't add the table for them,
>   because it's not used elsewhere and we need just one number from it.
> - Add flag for ALT3 support, restoring the behavior
>   for non-Realtek devices the same as before e848dbd364ac, due to
>   the problems reported on an Intel adapter. Don't have the device
>   myself.
> 

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

* Re: [PATCH v2] Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
  2021-07-23 12:19   ` Marcel Holtmann
  2021-07-25  5:47     ` Michał Kępień
@ 2021-08-10 16:48     ` Pasi Kärkkäinen
  2021-08-10 18:37       ` Luiz Augusto von Dentz
  1 sibling, 1 reply; 17+ messages in thread
From: Pasi Kärkkäinen @ 2021-08-10 16:48 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Pauli Virtanen, linux-bluetooth, Joseph Hwang, Hilda Wu,
	Johan Hedberg, Luiz Augusto von Dentz,
	Michał Kępień

Hello Marcel,

On Fri, Jul 23, 2021 at 02:19:09PM +0200, Marcel Holtmann wrote:
> Hi Pauli,
> 
> > Some USB BT adapters don't satisfy the MTU requirement mentioned in
> > commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> > and have ALT 3 setting that produces no/garbled audio. Some adapters
> > with larger MTU were also reported to have problems with ALT 3.
> > 
> > Add a flag and check it and MTU before selecting ALT 3, falling back to
> > ALT 1. Enable the flag for Realtek, restoring the previous behavior for
> > non-Realtek devices.
> > 
> > Tested with USB adapters (mtu<72, no/garbled sound with ALT3, ALT1
> > works) BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3
> > works) RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling
> > ALT6). Also got reports for (mtu>=72, ALT 3 reported to produce bad
> > audio) Intel 8087:0a2b.
> > 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > Fixes: e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> 
> before I will apply this, I need Tested-by or Ack-by people that confirm that this fixes their issues now.
>

Is v3 ok/enough? It has one Tested-by.
It would probably be good to send v4 anyway with CC stable@kernel.org ..


Thanks,

-- Pasi

> 
> > ---
> > 
> > Changes in v2:
> > - Explain magic number 72 in a comment; didn't add the table for them,
> >  because it's not used elsewhere and we need just one number from it.
> > - Add flag for ALT3 support, restoring the behavior
> >  for non-Realtek devices the same as before e848dbd364ac, due to
> >  the problems reported on an Intel adapter. Don't have the device
> >  myself.
> > 
> > drivers/bluetooth/btusb.c | 22 ++++++++++++++--------
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 6d23308119d1..5cec719f6cba 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -516,6 +516,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
> > #define BTUSB_HW_RESET_ACTIVE	12
> > #define BTUSB_TX_WAIT_VND_EVT	13
> > #define BTUSB_WAKEUP_DISABLE	14
> > +#define BTUSB_ALT3_OK_FOR_WBS	15
> 
> Rename this to BTUSB_USE_ALT3_FOR_WBS.
> 
> > 
> > struct btusb_data {
> > 	struct hci_dev       *hdev;
> > @@ -1748,16 +1749,20 @@ static void btusb_work(struct work_struct *work)
> > 			/* Bluetooth USB spec recommends alt 6 (63 bytes), but
> > 			 * many adapters do not support it.  Alt 1 appears to
> > 			 * work for all adapters that do not have alt 6, and
> > -			 * which work with WBS at all.
> > +			 * which work with WBS at all.  Some devices prefer
> > +			 * alt 3 (HCI payload >= 60 Bytes let air packet
> > +			 * data satisfy 60 bytes), requiring
> > +			 * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
> > +			 * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
> > 			 */
> > -			new_alts = btusb_find_altsetting(data, 6) ? 6 : 1;
> > -			/* Because mSBC frames do not need to be aligned to the
> > -			 * SCO packet boundary. If support the Alt 3, use the
> > -			 * Alt 3 for HCI payload >= 60 Bytes let air packet
> > -			 * data satisfy 60 bytes.
> > -			 */
> > -			if (new_alts == 1 && btusb_find_altsetting(data, 3))
> > +			if (btusb_find_altsetting(data, 6))
> > +				new_alts = 6;
> > +			else if (test_bit(BTUSB_ALT3_OK_FOR_WBS, &data->flags) &&
> > +				 hdev->sco_mtu >= 72 &&
> > +				 btusb_find_altsetting(data, 3))
> 
> This is whitespace damaged.
> 
> > 				new_alts = 3;
> > +			else
> > +				new_alts = 1;
> > 		}
> > 
> > 		if (btusb_switch_alt_setting(hdev, new_alts) < 0)
> > @@ -4733,6 +4738,7 @@ static int btusb_probe(struct usb_interface *intf,
> > 		 * (DEVICE_REMOTE_WAKEUP)
> > 		 */
> > 		set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
> > +		set_bit(BTUSB_ALT3_OK_FOR_WBS, &data->flags);
> > 	}
> 
> Regards
> 
> Marcel
> 

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

* Re: [PATCH v2] Bluetooth: btusb: check conditions before enabling USB ALT 3 for WBS
  2021-08-10 16:48     ` Pasi Kärkkäinen
@ 2021-08-10 18:37       ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 17+ messages in thread
From: Luiz Augusto von Dentz @ 2021-08-10 18:37 UTC (permalink / raw)
  To: Pasi Kärkkäinen
  Cc: Marcel Holtmann, Pauli Virtanen, linux-bluetooth, Joseph Hwang,
	Hilda Wu, Johan Hedberg, Michał Kępień

Hi Pasi, Pauli,

On Tue, Aug 10, 2021 at 9:50 AM Pasi Kärkkäinen <pasik@iki.fi> wrote:
>
> Hello Marcel,
>
> On Fri, Jul 23, 2021 at 02:19:09PM +0200, Marcel Holtmann wrote:
> > Hi Pauli,
> >
> > > Some USB BT adapters don't satisfy the MTU requirement mentioned in
> > > commit e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> > > and have ALT 3 setting that produces no/garbled audio. Some adapters
> > > with larger MTU were also reported to have problems with ALT 3.
> > >
> > > Add a flag and check it and MTU before selecting ALT 3, falling back to
> > > ALT 1. Enable the flag for Realtek, restoring the previous behavior for
> > > non-Realtek devices.
> > >
> > > Tested with USB adapters (mtu<72, no/garbled sound with ALT3, ALT1
> > > works) BCM20702A1 0b05:17cb, CSR8510A10 0a12:0001, and (mtu>=72, ALT3
> > > works) RTL8761BU 0bda:8771, Intel AX200 8087:0029 (after disabling
> > > ALT6). Also got reports for (mtu>=72, ALT 3 reported to produce bad
> > > audio) Intel 8087:0a2b.
> > >
> > > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > > Fixes: e848dbd364ac ("Bluetooth: btusb: Add support USB ALT 3 for WBS")
> >
> > before I will apply this, I need Tested-by or Ack-by people that confirm that this fixes their issues now.
> >
>
> Is v3 ok/enough? It has one Tested-by.
> It would probably be good to send v4 anyway with CC stable@kernel.org ..
>
>
> Thanks,
>
> -- Pasi
>
> >
> > > ---
> > >
> > > Changes in v2:
> > > - Explain magic number 72 in a comment; didn't add the table for them,
> > >  because it's not used elsewhere and we need just one number from it.
> > > - Add flag for ALT3 support, restoring the behavior
> > >  for non-Realtek devices the same as before e848dbd364ac, due to
> > >  the problems reported on an Intel adapter. Don't have the device
> > >  myself.
> > >
> > > drivers/bluetooth/btusb.c | 22 ++++++++++++++--------
> > > 1 file changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index 6d23308119d1..5cec719f6cba 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -516,6 +516,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
> > > #define BTUSB_HW_RESET_ACTIVE       12
> > > #define BTUSB_TX_WAIT_VND_EVT       13
> > > #define BTUSB_WAKEUP_DISABLE        14
> > > +#define BTUSB_ALT3_OK_FOR_WBS      15
> >
> > Rename this to BTUSB_USE_ALT3_FOR_WBS.
> >
> > >
> > > struct btusb_data {
> > >     struct hci_dev       *hdev;
> > > @@ -1748,16 +1749,20 @@ static void btusb_work(struct work_struct *work)
> > >                     /* Bluetooth USB spec recommends alt 6 (63 bytes), but
> > >                      * many adapters do not support it.  Alt 1 appears to
> > >                      * work for all adapters that do not have alt 6, and
> > > -                    * which work with WBS at all.
> > > +                    * which work with WBS at all.  Some devices prefer
> > > +                    * alt 3 (HCI payload >= 60 Bytes let air packet
> > > +                    * data satisfy 60 bytes), requiring
> > > +                    * MTU >= 3 (packets) * 25 (size) - 3 (headers) = 72
> > > +                    * see also Core spec 5, vol 4, B 2.1.1 & Table 2.1.
> > >                      */
> > > -                   new_alts = btusb_find_altsetting(data, 6) ? 6 : 1;
> > > -                   /* Because mSBC frames do not need to be aligned to the
> > > -                    * SCO packet boundary. If support the Alt 3, use the
> > > -                    * Alt 3 for HCI payload >= 60 Bytes let air packet
> > > -                    * data satisfy 60 bytes.
> > > -                    */
> > > -                   if (new_alts == 1 && btusb_find_altsetting(data, 3))
> > > +                   if (btusb_find_altsetting(data, 6))
> > > +                           new_alts = 6;
> > > +                   else if (test_bit(BTUSB_ALT3_OK_FOR_WBS, &data->flags) &&
> > > +                            hdev->sco_mtu >= 72 &&
> > > +                            btusb_find_altsetting(data, 3))
> >
> > This is whitespace damaged.

Ive fixed this and made it fit in 80 columns by reordering the checks.

> >
> > >                             new_alts = 3;
> > > +                   else
> > > +                           new_alts = 1;
> > >             }
> > >
> > >             if (btusb_switch_alt_setting(hdev, new_alts) < 0)
> > > @@ -4733,6 +4738,7 @@ static int btusb_probe(struct usb_interface *intf,
> > >              * (DEVICE_REMOTE_WAKEUP)
> > >              */
> > >             set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
> > > +           set_bit(BTUSB_ALT3_OK_FOR_WBS, &data->flags);
> > >     }
> >
> > Regards
> >
> > Marcel

Applied, thanks.


-- 
Luiz Augusto von Dentz

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

end of thread, other threads:[~2021-08-10 18:37 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-16 17:58 [PATCH] Bluetooth: btusb: check SCO MTU before enabling USB ALT 3 for WBS Pauli Virtanen
2021-07-16 19:16 ` bluez.test.bot
2021-07-17 13:34 ` [PATCH] " Joseph Hwang
2021-07-21 15:40   ` Pauli Virtanen
2021-07-22 10:28     ` Hilda Wu
2021-07-20  9:15 ` Pasi Kärkkäinen
2021-07-22 13:29 ` Michał Kępień
2021-07-22 14:26   ` Pauli Virtanen
2021-07-22 14:42     ` Michał Kępień
2021-07-23 10:07 ` [PATCH v2] Bluetooth: btusb: check conditions " Pauli Virtanen
2021-07-23 12:19   ` Marcel Holtmann
2021-07-25  5:47     ` Michał Kępień
2021-08-10 16:48     ` Pasi Kärkkäinen
2021-08-10 18:37       ` Luiz Augusto von Dentz
2021-07-26 18:02 ` [PATCH v3] " Pauli Virtanen
2021-07-27  2:11   ` [v3] " bluez.test.bot
2021-07-30 12:35   ` [PATCH v3] " Pasi Kärkkäinen

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).