Bluetooth: qca: fix device-address endianness

The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth device address in MSB order when setting it using the EDL_WRITE_BD_ADDR_OPCODE command.

Presumably, this is the case for all non-ROME devices which all use the EDL_WRITE_BD_ADDR_OPCODE command for this (unlike the ROME devices which use a different command and expect the address in LSB order).

Reverse the little-endian address before setting it to make sure that the address can be configured using tools like btmgmt or using the 'local-bd-address' devicetree property.

Note that this can potentially break systems with boot firmware which has started relying on the broken behaviour and is incorrectly passing the address via devicetree in MSB order.

Fixes 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address")

Cc [email protected] # 5.1

Cc Balakrishna Godavarthi <[email protected]>

Cc Matthias Kaehlcke <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Hi Qualcomm people,

Could you please verify with your documentation that all non-ROME devices expect the address provided in the EDL_WRITE_BD_ADDR_OPCODE command in MSB order?

I assume this is not something that anyone would change between firmware revisions, but if that turns out to be the case, we'd need to reverse the address based on firmware revision or similar.

Johan

 drivers/bluetooth/btqca.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/bluetooth/btqca.c +6 -2

index fdb0fae88d1c..29035daf21bc 100644
@@ -826,11 +826,15 @@ EXPORT_SYMBOL_GPL(qca_uart_setup);
 
 int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 {
+	bdaddr_t bdaddr_swapped;
 	struct sk_buff *skb;
 	int err;
 
-	skb = __hci_cmd_sync_ev(hdev, EDL_WRITE_BD_ADDR_OPCODE, 6, bdaddr,
-				HCI_EV_VENDOR, HCI_INIT_TIMEOUT);
+	baswap(&bdaddr_swapped, bdaddr);
+
+	skb = __hci_cmd_sync_ev(hdev, EDL_WRITE_BD_ADDR_OPCODE, 6,
+				&bdaddr_swapped, HCI_EV_VENDOR,
+				HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
 		bt_dev_err(hdev, "QCA Change address cmd failed (%d)", err);

Signature · 1 line

2.41.0

The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth device address in MSB order when setting it using the EDL_WRITE_BD_ADDR_OPCODE command.

Presumably, this is the case for all non-ROME devices which all use the EDL_WRITE_BD_ADDR_OPCODE command for this (unlike the ROME devices which use a different command and expect the address in LSB order).

Reverse the little-endian address before setting it to make sure that the address can be configured using tools like btmgmt or using the 'local-bd-address' devicetree property.

Note that this can potentially break systems with boot firmware which has started relying on the broken behaviour and is incorrectly passing the address via devicetree in MSB order.

Fixes 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address")

Cc [email protected] # 5.1

Cc Balakrishna Godavarthi <[email protected]>

Cc Matthias Kaehlcke <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---

Hi Qualcomm people,

Could you please verify with your documentation that all non-ROME devices expect the address provided in the EDL_WRITE_BD_ADDR_OPCODE command in MSB order?

I assume this is not something that anyone would change between firmware revisions, but if that turns out to be the case, we'd need to reverse the address based on firmware revision or similar.

Johan

 drivers/bluetooth/btqca.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/bluetooth/btqca.c +6 -2

index fdb0fae88d1c..29035daf21bc 100644
@@ -826,11 +826,15 @@ EXPORT_SYMBOL_GPL(qca_uart_setup);
 
 int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 {
+	bdaddr_t bdaddr_swapped;
 	struct sk_buff *skb;
 	int err;
 
-	skb = __hci_cmd_sync_ev(hdev, EDL_WRITE_BD_ADDR_OPCODE, 6, bdaddr,
-				HCI_EV_VENDOR, HCI_INIT_TIMEOUT);
+	baswap(&bdaddr_swapped, bdaddr);
+
+	skb = __hci_cmd_sync_ev(hdev, EDL_WRITE_BD_ADDR_OPCODE, 6,
+				&bdaddr_swapped, HCI_EV_VENDOR,
+				HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
 		bt_dev_err(hdev, "QCA Change address cmd failed (%d)", err);

Signature · 1 line

2.41.0

Quoted 12 lines · Johan Hovold

The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth device address in MSB order when setting it using the EDL_WRITE_BD_ADDR_OPCODE command.

Presumably, this is the case for all non-ROME devices which all use the EDL_WRITE_BD_ADDR_OPCODE command for this (unlike the ROME devices which use a different command and expect the address in LSB order).

Reverse the little-endian address before setting it to make sure that the address can be configured using tools like btmgmt or using the 'local-bd-address' devicetree property.

Quoted 3 lines

Note that this can potentially break systems with boot firmware which has started relying on the broken behaviour and is incorrectly passing the address via devicetree in MSB order.

We should not break existing devices. Their byte order for 'local-bd-address' may not adhere to the 'spec', however in practice it is the correct format for existing kernels.

I suggest adding a quirk like 'local-bd-address-msb-quirk' or 'qcom,local-bd-address-msb-quirk' to make sure existing devices keep working properly.

Quoted 13 lines · Matthias Kaehlcke

Quoted 12 lines · Johan Hovold

The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth device address in MSB order when setting it using the EDL_WRITE_BD_ADDR_OPCODE command.

Presumably, this is the case for all non-ROME devices which all use the EDL_WRITE_BD_ADDR_OPCODE command for this (unlike the ROME devices which use a different command and expect the address in LSB order).

Reverse the little-endian address before setting it to make sure that the address can be configured using tools like btmgmt or using the 'local-bd-address' devicetree property.

Quoted 7 lines

Quoted 3 lines

Note that this can potentially break systems with boot firmware which has started relying on the broken behaviour and is incorrectly passing the address via devicetree in MSB order.

We should not break existing devices. Their byte order for 'local-bd-address' may not adhere to the 'spec', however in practice it is the correct format for existing kernels.

That depends on in what way the current devices are broken.

Any machines that correctly specify their address in little-endian order in the devicetree would no longer be configured using the wrong address. So no problem there (except requiring users to re-pair their gadgets).

And tools like btgmt is broken on all of these Qualcomm machine in any case and would now start working as expected. So no problem there either (unless user space had adapted an inverted the addresses to btmgmt).

So the first question is whether there actually is any boot firmware out there which passes the BD_ADDR in reverse order?

Quoted 3 lines

I suggest adding a quirk like 'local-bd-address-msb-quirk' or 'qcom,local-bd-address-msb-quirk' to make sure existing devices keep working properly.

I don't think that would work. If this is something that we really need to handle, then there's probably no way around introducing new compatible strings for boot firmware that isn't broken while maintaining the current broken behaviour with respect to 'local-bd-address' for some of the current ones.

Quoted 10 lines · Johan Hovold

That depends on in what way the current devices are broken.

Any machines that correctly specify their address in little-endian order in the devicetree would no longer be configured using the wrong address. So no problem there (except requiring users to re-pair their gadgets).

And tools like btgmt is broken on all of these Qualcomm machine in any case and would now start working as expected. So no problem there either (unless user space had adapted an inverted the addresses to btmgmt).

Quoted 2 lines

So the first question is whether there actually is any boot firmware out there which passes the BD_ADDR in reverse order?

Yes, (at least) the boot firmware for sc7180-trogdor devices.

hexdump -C /proc/device-tree/soc\@0/geniqup\@8c0000/serial\@88c000/bluetooth/local-bd-address 00000000 8c fd f0 40 15 dc

hciconfig
hci0: Type: Primary Bus: UART

        BD Address: 8C:FD:F0:40:15:DC  ACL MTU: 1024:8  SCO MTU: 240:8
        UP RUNNING 
        RX bytes:1700 acl:0 sco:0 events:95 errors:0
        TX bytes:128949 acl:0 sco:0 commands:578 errors:0

Quoted 9 lines

Quoted 3 lines

I suggest adding a quirk like 'local-bd-address-msb-quirk' or 'qcom,local-bd-address-msb-quirk' to make sure existing devices keep working properly.

I don't think that would work. If this is something that we really need to handle, then there's probably no way around introducing new compatible strings for boot firmware that isn't broken while maintaining the current broken behaviour with respect to 'local-bd-address' for some of the current ones.

I think it should work for sc7180-trogdor. For these devices the device tree is bundled with the kernel image and can be updated. That might not be true for other devices though.

Quoted 8 lines

Quoted 3 lines

So the first question is whether there actually is any boot firmware out there which passes the BD_ADDR in reverse order?

Yes, (at least) the boot firmware for sc7180-trogdor devices.

hexdump -C /proc/device-tree/soc\@0/geniqup\@8c0000/serial\@88c000/bluetooth/local-bd-address 00000000 8c fd f0 40 15 dc

Indeed, this should have been LE order.

Quoted 6 lines

hciconfig
hci0: Type: Primary Bus: UART

        BD Address: 8C:FD:F0:40:15:DC  ACL MTU: 1024:8  SCO MTU: 240:8
        UP RUNNING
        RX bytes:1700 acl:0 sco:0 events:95 errors:0
        TX bytes:128949 acl:0 sco:0 commands:578 errors:0

And any user space tool overriding the address would currently need to provide the address in reverse order on Qualcomm platforms like this one (e.g. if generating the address for privacy reasons).

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

I suggest adding a quirk like 'local-bd-address-msb-quirk' or 'qcom,local-bd-address-msb-quirk' to make sure existing devices keep working properly.

I don't think that would work. If this is something that we really need to handle, then there's probably no way around introducing new compatible strings for boot firmware that isn't broken while maintaining the current broken behaviour with respect to 'local-bd-address' for some of the current ones.

Quoted 3 lines

I think it should work for sc7180-trogdor. For these devices the device tree is bundled with the kernel image and can be updated. That might not be true for other devices though.

Thanks for confirming.

I'm still hoping we can get away with not having to add quirks to Bluetooth core for broken Qualcomm boot firmware. Let's see if anyone knows of a use case that makes that impossible to avoid.

Johan

Hi,

Quoted 10 lines · Johan Hovold

Quoted 8 lines

Quoted 3 lines

So the first question is whether there actually is any boot firmware out there which passes the BD_ADDR in reverse order?

Yes, (at least) the boot firmware for sc7180-trogdor devices.

hexdump -C /proc/device-tree/soc\@0/geniqup\@8c0000/serial\@88c000/bluetooth/local-bd-address 00000000 8c fd f0 40 15 dc

Indeed, this should have been LE order.

In case it adds any extra data points, we also do similar with the WiFi MAC address and it also seems to be big endian.

lazor-rev9 /proc/device-tree/soc@0/wifi@18800000 # hexdump -C local-mac-address
00000000 8c fd f0 3e 3e 86 |...>>.|
00000006

lazor-rev9 /proc/device-tree/soc@0/wifi@18800000 # ifconfig wlan0 | grep ether

        ether 8c:fd:f0:3e:3e:86  txqueuelen 1000  (Ethernet)

Quoted 12 lines · Matthias Kaehlcke

Quoted 10 lines · Johan Hovold

That depends on in what way the current devices are broken.

Any machines that correctly specify their address in little-endian order in the devicetree would no longer be configured using the wrong address. So no problem there (except requiring users to re-pair their gadgets).

And tools like btgmt is broken on all of these Qualcomm machine in any case and would now start working as expected. So no problem there either (unless user space had adapted an inverted the addresses to btmgmt).

Quoted 7 lines

Quoted 2 lines

So the first question is whether there actually is any boot firmware out there which passes the BD_ADDR in reverse order?

Yes, (at least) the boot firmware for sc7180-trogdor devices.

hexdump -C /proc/device-tree/soc\@0/geniqup\@8c0000/serial\@88c000/bluetooth/local-bd-address 00000000 8c fd f0 40 15 dc

Indeed, this should have been LE order.

Quoted 6 lines

hciconfig
hci0: Type: Primary Bus: UART

        BD Address: 8C:FD:F0:40:15:DC  ACL MTU: 1024:8  SCO MTU: 240:8
        UP RUNNING
        RX bytes:1700 acl:0 sco:0 events:95 errors:0
        TX bytes:128949 acl:0 sco:0 commands:578 errors:0

And any user space tool overriding the address would currently need to provide the address in reverse order on Qualcomm platforms like this one (e.g. if generating the address for privacy reasons).

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

I suggest adding a quirk like 'local-bd-address-msb-quirk' or 'qcom,local-bd-address-msb-quirk' to make sure existing devices keep working properly.

I don't think that would work. If this is something that we really need to handle, then there's probably no way around introducing new compatible strings for boot firmware that isn't broken while maintaining the current broken behaviour with respect to 'local-bd-address' for some of the current ones.

Quoted 3 lines

I think it should work for sc7180-trogdor. For these devices the device tree is bundled with the kernel image and can be updated. That might not be true for other devices though.

Thanks for confirming.

I'm still hoping we can get away with not having to add quirks to Bluetooth core for broken Qualcomm boot firmware. Let's see if anyone knows of a use case that makes that impossible to avoid.

Johan

Quoted 9 lines · Johan Hovold

Quoted 6 lines · Matthias Kaehlcke

hciconfig
hci0: Type: Primary Bus: UART

        BD Address: 8C:FD:F0:40:15:DC  ACL MTU: 1024:8  SCO MTU: 240:8
        UP RUNNING
        RX bytes:1700 acl:0 sco:0 events:95 errors:0
        TX bytes:128949 acl:0 sco:0 commands:578 errors:0

Quoted 3 lines

And any user space tool overriding the address would currently need to provide the address in reverse order on Qualcomm platforms like this one (e.g. if generating the address for privacy reasons).

Perhaps we could attempt to resolve the address byteorder, in userspace we use hwdb_get_company to resolve the company but since this shall only really care about Qualcomm range(s) perhaps we can hardcode them check in which order the address is, that said if the device is configured with a Static Random Address then that would not work, but that is only really possible for BLE only devices.

Quoted 14 lines · Luiz Augusto von Dentz

Quoted 9 lines · Johan Hovold

Quoted 6 lines · Matthias Kaehlcke

hciconfig
hci0: Type: Primary Bus: UART

        BD Address: 8C:FD:F0:40:15:DC  ACL MTU: 1024:8  SCO MTU: 240:8
        UP RUNNING
        RX bytes:1700 acl:0 sco:0 events:95 errors:0
        TX bytes:128949 acl:0 sco:0 commands:578 errors:0

Quoted 3 lines

And any user space tool overriding the address would currently need to provide the address in reverse order on Qualcomm platforms like this one (e.g. if generating the address for privacy reasons).

Quoted 6 lines

Perhaps we could attempt to resolve the address byteorder, in userspace we use hwdb_get_company to resolve the company but since this shall only really care about Qualcomm range(s) perhaps we can hardcode them check in which order the address is, that said if the device is configured with a Static Random Address then that would not work, but that is only really possible for BLE only devices.

It's not just Qualcomm ranges; The Lenovo ThinkPad X13s that I noticed this on has been assigned a Wistron OUI, for example.

We're still hoping to learn how to retrieve this address (from the secure world firmware) so that we can set it directly from the driver, but for now it needs to be set using btmgmt (or the local-bd-address devicetree property).

As was discussed here:

        https://github.com/bluez/bluez/issues/107

it would be useful to teach bluetoothd to (generate and) set an address for devices that lack (accessible) persistent storage. And any such generic tool would need to work using the standard interfaces and the address endianness that those interfaces expect.

And from skimming the Bluetooth spec, I was under the impression that random addresses applied also to non-BLE devices (e.g. requiring the two most-significants bits to be 1).

But to summarise, I don't really see any way around fixing the Qualcomm driver.

Johan

Quoted 15 lines · Johan Hovold

Quoted 13 lines · Luiz Augusto von Dentz

Quoted 5 lines · Johan Hovold

On Tue, Jan 09, 2024 at 05:54:01PM +0000, Matthias Kaehlcke wrote:

And any user space tool overriding the address would currently need to provide the address in reverse order on Qualcomm platforms like this one (e.g. if generating the address for privacy reasons).

Perhaps we could attempt to resolve the address byteorder, in userspace we use hwdb_get_company to resolve the company but since this shall only really care about Qualcomm range(s) perhaps we can hardcode them check in which order the address is, that said if the device is configured with a Static Random Address then that would not work, but that is only really possible for BLE only devices.

Quoted 2 lines

It's not just Qualcomm ranges; The Lenovo ThinkPad X13s that I noticed this on has been assigned a Wistron OUI, for example.

Well we could still attempt to check if it has a valid OUI and then it fail swap and check again.

Quoted 5 lines

We're still hoping to learn how to retrieve this address (from the secure world firmware) so that we can set it directly from the driver, but for now it needs to be set using btmgmt (or the local-bd-address devicetree property).

Quoted 8 lines

As was discussed here:

        https://github.com/bluez/bluez/issues/107

it would be useful to teach bluetoothd to (generate and) set an address for devices that lack (accessible) persistent storage. And any such generic tool would need to work using the standard interfaces and the address endianness that those interfaces expect.

Yep, patches are welcome in this regard, note that we do something like this:

https://github.com/bluez/bluez/blob/master/src/adapter.c#L9847

But the first thing it checks is if the controller supports BR/EDR, so if you want to extend that we need at least the OUI portion to be able to allocate a valid public address, we could perhaps attempt to fetch the manufacturer somehow or use the controller manufacturer (adapter->manufacturer) in case there is nothing else to use.

Quoted 3 lines

And from skimming the Bluetooth spec, I was under the impression that random addresses applied also to non-BLE devices (e.g. requiring the two most-significants bits to be 1).

Not really, BR/EDR/classic addresses are always considered public addresses, the HCI interface doesn't even have an address type to be able to handle something like a random address or privacy for the same reason.

The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth device address in MSB order when setting it using the EDL_WRITE_BD_ADDR_OPCODE command.

Presumably, this is the case for all non-ROME devices which all use the EDL_WRITE_BD_ADDR_OPCODE command for this (unlike the ROME devices which use a different command and expect the address in LSB order).

Reverse the little-endian address before setting it to make sure that the address can be configured using tools like btmgmt or using the 'local-bd-address' devicetree property.

Note that this can potentially break systems with boot firmware which has started relying on the broken behaviour and is incorrectly passing the address via devicetree in MSB order.

Fixes 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address")

Cc [email protected] # 5.1

Cc Balakrishna Godavarthi <[email protected]>

Cc Matthias Kaehlcke <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

Hi Luiz,

Quoted 16 lines · Johan Hovold

The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth device address in MSB order when setting it using the EDL_WRITE_BD_ADDR_OPCODE command.

Presumably, this is the case for all non-ROME devices which all use the EDL_WRITE_BD_ADDR_OPCODE command for this (unlike the ROME devices which use a different command and expect the address in LSB order).

Reverse the little-endian address before setting it to make sure that the address can be configured using tools like btmgmt or using the 'local-bd-address' devicetree property.

Note that this can potentially break systems with boot firmware which has started relying on the broken behaviour and is incorrectly passing the address via devicetree in MSB order.

Quoted 5 lines

Fixes 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address")

Cc [email protected] # 5.1

Cc Balakrishna Godavarthi <[email protected]>

Cc Matthias Kaehlcke <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

Can we go ahead and merge this one to get this fixed in 6.8?

I've spoken to Bjorn Andersson at Qualcomm about this and he is in favour of doing so. The only people actually using the devicetree property should be the Chromium team and they control their own boot firmware and should be able to update it in lockstep (and Android uses some custom hacks to set the address that are not in mainline).

Quoted 19 lines · Johan Hovold

Quoted 12 lines · Johan Hovold

The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth device address in MSB order when setting it using the EDL_WRITE_BD_ADDR_OPCODE command.

Reverse the little-endian address before setting it to make sure that the address can be configured using tools like btmgmt or using the 'local-bd-address' devicetree property.

Note that this can potentially break systems with boot firmware which has started relying on the broken behaviour and is incorrectly passing the address via devicetree in MSB order.

Quoted 5 lines

Fixes 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address")

Cc [email protected] # 5.1

Cc Balakrishna Godavarthi <[email protected]>

Cc Matthias Kaehlcke <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

Quoted 7 lines

Can we go ahead and merge this one to get this fixed in 6.8?

I've spoken to Bjorn Andersson at Qualcomm about this and he is in favour of doing so. The only people actually using the devicetree property should be the Chromium team and they control their own boot firmware and should be able to update it in lockstep (and Android uses some custom hacks to set the address that are not in mainline).

Unfortunately it's not as trivial as it sounds for Chrome OS. The boot firmware is controlled by Chrome OS, however for any baseboard (e.g. 'trogdor') there is a larger number binary firmware packages, one for every model derived from that baseboard. There can be dozens of models. Chrome OS Firmware releases are qualified and rolled out per model. FW qual may involve the ODM, usually there are multiple ODMs per board. In an absolute emergency it would be possible to coordinate a qual and synced rollout for all models, but it's definitely non-trivial in terms of operations.

Quoted 28 lines · Matthias Kaehlcke

Quoted 19 lines · Johan Hovold

Quoted 12 lines · Johan Hovold

The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth device address in MSB order when setting it using the EDL_WRITE_BD_ADDR_OPCODE command.

Reverse the little-endian address before setting it to make sure that the address can be configured using tools like btmgmt or using the 'local-bd-address' devicetree property.

Note that this can potentially break systems with boot firmware which has started relying on the broken behaviour and is incorrectly passing the address via devicetree in MSB order.

Quoted 5 lines

Fixes 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address")

Cc [email protected] # 5.1

Cc Balakrishna Godavarthi <[email protected]>

Cc Matthias Kaehlcke <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

Quoted 7 lines

Can we go ahead and merge this one to get this fixed in 6.8?

I've spoken to Bjorn Andersson at Qualcomm about this and he is in favour of doing so. The only people actually using the devicetree property should be the Chromium team and they control their own boot firmware and should be able to update it in lockstep (and Android uses some custom hacks to set the address that are not in mainline).

Quoted 9 lines

Unfortunately it's not as trivial as it sounds for Chrome OS. The boot firmware is controlled by Chrome OS, however for any baseboard (e.g. 'trogdor') there is a larger number binary firmware packages, one for every model derived from that baseboard. There can be dozens of models. Chrome OS Firmware releases are qualified and rolled out per model. FW qual may involve the ODM, usually there are multiple ODMs per board. In an absolute emergency it would be possible to coordinate a qual and synced rollout for all models, but it's definitely non-trivial in terms of operations.

Ok, fair enough.

Could you please provide a list of the compatible strings that you guys currently use and I can add new compatible strings for those, while keeping the current ones for backwards compatibility with older boot firmware?