[Regression] Linux v5.10 breaks AR3011 fw loading

[Regression] Linux v5.10 breaks AR3011 fw loading

[Florian Albrechtskirchinger]

Hey everyone,

Linux v5.10 broke firmware loading for my AR3011 Bluetooth USB adapter.
A git bisect identified the following commit as the culprit:

> commit b40f58b973865ee98ead884d2bdc7880b896ddb8
> Author: Rocky Liao <rjliao@codeaurora.org>
> Date:   Tue Sep 29 12:23:51 2020 +0800
>
>     Bluetooth: btusb: Add Qualcomm Bluetooth SoC WCN6855 support
>   
>     This patch add support for WCN6855 i.e. patch and nvm download
>     support.

The accompanying patch describes the issue further and attempts to offer
a fix based on my superficial understanding, which may or may not be
adequate.

Regards,
Florian

[PATCH] Bluetooth: btusb: Fallback to 16 bit ROM version lookup

[Florian Albrechtskirchinger]

Commit b40f58b97386 ("Bluetooth: btusb: Add Qualcomm Bluetooth SoC WCN6855
support") changes ROM version lookup from 16 bit to 32 bit. Previously, the
upper 16 bit of the version number were ignored. This breaks setups, where the
upper 16 bits are non-zero, but are now assumed to be zero.

An example of such a device would be
0cf3:3008 Qualcomm Atheros Communications Bluetooth (AR3011)
with ROM version 0x1020200 and this corresponding entry in the device table:
{ 0x00000200, 28, 4, 16 }, /* Rome 2.0 */

This patch adds a potential second round of lookups that mimics the old
behavior, should no version have been matched by comparing the full 32 bits.
During this second round only the lower 16 bits are compared, but only where
the upper 16 bits are defined zero in the lookup table.

Fixes: b40f58b97386 ("Bluetooth: btusb: Add Qualcomm Bluetooth SoC WCN6855
support")

Signed-off-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
---
 drivers/bluetooth/btusb.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 03b83aa91277..d8c4c6474f14 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -4054,6 +4054,7 @@ static int btusb_setup_qca(struct hci_dev *hdev)
 	const struct qca_device_info *info = NULL;
 	struct qca_version ver;
 	u32 ver_rom;
+	u16 ver_rom_low;
 	u8 status;
 	int i, err;
 
@@ -4065,8 +4066,22 @@ static int btusb_setup_qca(struct hci_dev *hdev)
 	ver_rom = le32_to_cpu(ver.rom_version);
 
 	for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
-		if (ver_rom == qca_devices_table[i].rom_version)
+		if (ver_rom == qca_devices_table[i].rom_version) {
 			info = &qca_devices_table[i];
+			break;
+		}
+	}
+	if (!info) {
+		// If we don't find an exact version match, try with
+		// the lower half, but only where the upper half is 0
+		ver_rom_low = ver_rom & 0xffff;
+		for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
+			if (!(qca_devices_table[i].rom_version & 0xffff0000) &&
+			    ver_rom_low == qca_devices_table[i].rom_version) {
+				info = &qca_devices_table[i];
+				break;
+			}
+		}
 	}
 	if (!info) {
 		bt_dev_err(hdev, "don't support firmware rome 0x%x", ver_rom);
-- 
2.30.0

Re: [PATCH] Bluetooth: btusb: Fallback to 16 bit ROM version lookup

[Marcel Holtmann]

Hi Florian,

> Commit b40f58b97386 ("Bluetooth: btusb: Add Qualcomm Bluetooth SoC WCN6855
> support") changes ROM version lookup from 16 bit to 32 bit. Previously, the
> upper 16 bit of the version number were ignored. This breaks setups, where the
> upper 16 bits are non-zero, but are now assumed to be zero.
> 
> An example of such a device would be
> 0cf3:3008 Qualcomm Atheros Communications Bluetooth (AR3011)
> with ROM version 0x1020200 and this corresponding entry in the device table:
> { 0x00000200, 28, 4, 16 }, /* Rome 2.0 */
> 
> This patch adds a potential second round of lookups that mimics the old
> behavior, should no version have been matched by comparing the full 32 bits.
> During this second round only the lower 16 bits are compared, but only where
> the upper 16 bits are defined zero in the lookup table.
> 
> Fixes: b40f58b97386 ("Bluetooth: btusb: Add Qualcomm Bluetooth SoC WCN6855
> support")
> 
> Signed-off-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
> ---
> drivers/bluetooth/btusb.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 03b83aa91277..d8c4c6474f14 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4054,6 +4054,7 @@ static int btusb_setup_qca(struct hci_dev *hdev)
> 	const struct qca_device_info *info = NULL;
> 	struct qca_version ver;
> 	u32 ver_rom;
> +	u16 ver_rom_low;
> 	u8 status;
> 	int i, err;
> 
> @@ -4065,8 +4066,22 @@ static int btusb_setup_qca(struct hci_dev *hdev)
> 	ver_rom = le32_to_cpu(ver.rom_version);
> 
> 	for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
> -		if (ver_rom == qca_devices_table[i].rom_version)
> +		if (ver_rom == qca_devices_table[i].rom_version) {
> 			info = &qca_devices_table[i];
> +			break;
> +		}
> +	}
> +	if (!info) {
> +		// If we don't find an exact version match, try with
> +		// the lower half, but only where the upper half is 0

please use correct comment style.

> +		ver_rom_low = ver_rom & 0xffff;
> +		for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
> +			if (!(qca_devices_table[i].rom_version & 0xffff0000) &&
> +			    ver_rom_low == qca_devices_table[i].rom_version) {
> +				info = &qca_devices_table[i];
> +				break;
> +			}
> +		}
> 	}
> 	if (!info) {
> 		bt_dev_err(hdev, "don't support firmware rome 0x%x", ver_rom);

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Fallback to 16 bit ROM version lookup

[Florian Albrechtskirchinger]

On Tue, Feb 9, 2021 at 2:42 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Florian,
>
> > Commit b40f58b97386 ("Bluetooth: btusb: Add Qualcomm Bluetooth SoC WCN6855
> > support") changes ROM version lookup from 16 bit to 32 bit. Previously, the
> > upper 16 bit of the version number were ignored. This breaks setups, where the
> > upper 16 bits are non-zero, but are now assumed to be zero.
> >
> > An example of such a device would be
> > 0cf3:3008 Qualcomm Atheros Communications Bluetooth (AR3011)
> > with ROM version 0x1020200 and this corresponding entry in the device table:
> > { 0x00000200, 28, 4, 16 }, /* Rome 2.0 */
> >
> > This patch adds a potential second round of lookups that mimics the old
> > behavior, should no version have been matched by comparing the full 32 bits.
> > During this second round only the lower 16 bits are compared, but only where
> > the upper 16 bits are defined zero in the lookup table.
> >
> > Fixes: b40f58b97386 ("Bluetooth: btusb: Add Qualcomm Bluetooth SoC WCN6855
> > support")
> >
> > Signed-off-by: Florian Albrechtskirchinger <falbrechtskirchinger@gmail.com>
> > ---
> > drivers/bluetooth/btusb.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 03b83aa91277..d8c4c6474f14 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -4054,6 +4054,7 @@ static int btusb_setup_qca(struct hci_dev *hdev)
> >       const struct qca_device_info *info = NULL;
> >       struct qca_version ver;
> >       u32 ver_rom;
> > +     u16 ver_rom_low;
> >       u8 status;
> >       int i, err;
> >
> > @@ -4065,8 +4066,22 @@ static int btusb_setup_qca(struct hci_dev *hdev)
> >       ver_rom = le32_to_cpu(ver.rom_version);
> >
> >       for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
> > -             if (ver_rom == qca_devices_table[i].rom_version)
> > +             if (ver_rom == qca_devices_table[i].rom_version) {
> >                       info = &qca_devices_table[i];
> > +                     break;
> > +             }
> > +     }
> > +     if (!info) {
> > +             // If we don't find an exact version match, try with
> > +             // the lower half, but only where the upper half is 0
>
> please use correct comment style.
Noted for next time. I would've expected checkpatch.pl to catch issues
like that?
A proper fix by Hui Wang has already been committed to bluetooth-next.

> > +             ver_rom_low = ver_rom & 0xffff;
> > +             for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
> > +                     if (!(qca_devices_table[i].rom_version & 0xffff0000) &&
> > +                         ver_rom_low == qca_devices_table[i].rom_version) {
> > +                             info = &qca_devices_table[i];
> > +                             break;
> > +                     }
> > +             }
> >       }
> >       if (!info) {
> >               bt_dev_err(hdev, "don't support firmware rome 0x%x", ver_rom);
>
> Regards
>
> Marcel

- Florian