| Message ID | 20231227180306.6319-1-johan+linaro@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: qca: fix device-address endianness | expand |
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=813083 ---Test result--- Test Summary: CheckPatch PASS 0.62 seconds GitLint PASS 0.37 seconds SubjectPrefix PASS 0.12 seconds BuildKernel PASS 27.83 seconds CheckAllWarning PASS 30.66 seconds CheckSparse PASS 37.40 seconds CheckSmatch PASS 99.70 seconds BuildKernel32 PASS 27.34 seconds TestRunnerSetup PASS 434.37 seconds TestRunner_l2cap-tester PASS 22.76 seconds TestRunner_iso-tester PASS 48.67 seconds TestRunner_bnep-tester PASS 6.93 seconds TestRunner_mgmt-tester PASS 159.41 seconds TestRunner_rfcomm-tester PASS 11.22 seconds TestRunner_sco-tester PASS 14.91 seconds TestRunner_ioctl-tester PASS 12.03 seconds TestRunner_mesh-tester PASS 8.75 seconds TestRunner_smp-tester PASS 11.73 seconds TestRunner_userchan-tester PASS 7.16 seconds IncrementalBuild PASS 26.43 seconds --- Regards, Linux Bluetooth
On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote: > 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: stable@vger.kernel.org # 5.1 > Cc: Balakrishna Godavarthi <quic_bgodavar@quicinc.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> The same issue was present on sc7180 (qcom,wcn3991-bt) and this patch fixes it. Tested-by: Nikita Travkin <nikita@trvn.ru> # sc7180 Thanks! > --- > > 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(-) > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > index fdb0fae88d1c..29035daf21bc 100644 > --- a/drivers/bluetooth/btqca.c > +++ b/drivers/bluetooth/btqca.c > @@ -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); > -- > 2.41.0
Hi Johan, On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote: > 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. 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. Thanks Matthias > > Fixes: 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address") > Cc: stable@vger.kernel.org # 5.1 > Cc: Balakrishna Godavarthi <quic_bgodavar@quicinc.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > > 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(-) > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > index fdb0fae88d1c..29035daf21bc 100644 > --- a/drivers/bluetooth/btqca.c > +++ b/drivers/bluetooth/btqca.c > @@ -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); > -- > 2.41.0 >
On Tue, Jan 09, 2024 at 04:50:59PM +0000, Matthias Kaehlcke wrote: > On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote: > > 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. > > 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? > 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. Johan
On Tue, Jan 09, 2024 at 06:12:26PM +0100, Johan Hovold wrote: > On Tue, Jan 09, 2024 at 04:50:59PM +0000, Matthias Kaehlcke wrote: > > > On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote: > > > 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. > > > > 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? 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 > > 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. Matthias
On Tue, Jan 09, 2024 at 05:54:01PM +0000, Matthias Kaehlcke wrote: > On Tue, Jan 09, 2024 at 06:12:26PM +0100, Johan Hovold wrote: > > 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? > > 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. > 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). > > > 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. 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, On Wed, Jan 10, 2024 at 12:12 AM Johan Hovold <johan@kernel.org> wrote: > > > > 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) > > 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). > > > > > 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. > > 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 Johan, On Wed, Jan 10, 2024 at 3:12 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Jan 09, 2024 at 05:54:01PM +0000, Matthias Kaehlcke wrote: > > On Tue, Jan 09, 2024 at 06:12:26PM +0100, Johan Hovold wrote: > > > > 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? > > > > 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. > > > 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). 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. > > > > 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. > > 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
On Wed, Jan 17, 2024 at 01:52:08PM -0800, Doug Anderson wrote: > Hi, > > On Wed, Jan 10, 2024 at 12:12 AM Johan Hovold <johan@kernel.org> wrote: > > > > > > 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) Yes, wifi and ethernet MAC addresses are always big endian (i.e. on the wire as well as in UIs). When the corresponding devicetree property for Bluetooth device addresses was added, Marcel explicitly requested that the address be provided in little-endian order: "I would prefer the boot loader actually providing the BD Address in the correct byte order as the protocol expects and not yet another form. The string representation is just for reference since that is what most people have seen so far." https://lore.kernel.org/all/41A0C162-4AC5-4969-813D-9E2C7F5D5031@holtmann.org/ and this is also what made it into the binding: 28517c02e1dd ("dt-bindings: net: document Bluetooth bindings in one place") Perhaps someone should have pushed back at the time to avoid this (apparent) inconsistency, but this is what we have since 2017. Johan
On Wed, Jan 17, 2024 at 05:49:07PM -0500, Luiz Augusto von Dentz wrote: > On Wed, Jan 10, 2024 at 3:12 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Jan 09, 2024 at 05:54:01PM +0000, Matthias Kaehlcke wrote: > > > 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). > > 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
Hi Johan, On Thu, Jan 18, 2024 at 3:40 AM Johan Hovold <johan@kernel.org> wrote: > > On Wed, Jan 17, 2024 at 05:49:07PM -0500, Luiz Augusto von Dentz wrote: > > On Wed, Jan 10, 2024 at 3:12 AM Johan Hovold <johan@kernel.org> wrote: > > > On Tue, Jan 09, 2024 at 05:54:01PM +0000, Matthias Kaehlcke wrote: > > > > > 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). > > > > 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. Well we could still attempt to check if it has a valid OUI and then it fail swap and check again. > 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. 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. > 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. > But to summarise, I don't really see any way around fixing the Qualcomm > driver. > > Johan
On Thu, Jan 18, 2024 at 10:30:50AM -0500, Luiz Augusto von Dentz wrote: > On Thu, Jan 18, 2024 at 3:40 AM Johan Hovold <johan@kernel.org> wrote: > > On Wed, Jan 17, 2024 at 05:49:07PM -0500, Luiz Augusto von Dentz wrote: > > > On Wed, Jan 10, 2024 at 3:12 AM Johan Hovold <johan@kernel.org> wrote: > > > > 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. > > > > 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. So in the kernel you would parse any address coming from firmware or user space to try to determine if it's given in reverse order? I don't see how this would work as presumably some of the least significant bytes would occasionally match a valid OUI even if you were somehow able to determine that. > > 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. > > 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. Thanks for the pointer. I'm trying nudge some of the distro folks to look into this. > > 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. Ah, ok. Then generating an address is perhaps not an option, but reading one out from a file and setting it would still be useful for cases like the X13s which do have an address assigned (e.g. accessible through windows or written on the box the machine came in). Johan
Hi Luiz, On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote: > 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: stable@vger.kernel.org # 5.1 > Cc: Balakrishna Godavarthi <quic_bgodavar@quicinc.com> > Cc: Matthias Kaehlcke <mka@chromium.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> 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). Johan
On Tue, Feb 13, 2024 at 03:41:56PM +0100, Johan Hovold wrote: > Hi Luiz, > > On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote: > > 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: stable@vger.kernel.org # 5.1 > > Cc: Balakrishna Godavarthi <quic_bgodavar@quicinc.com> > > Cc: Matthias Kaehlcke <mka@chromium.org> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > 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.
On Tue, Feb 13, 2024 at 03:55:06PM +0000, Matthias Kaehlcke wrote: > On Tue, Feb 13, 2024 at 03:41:56PM +0100, Johan Hovold wrote: > > On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote: > > > 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. > > > > > > Fixes: 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address") > > > Cc: stable@vger.kernel.org # 5.1 > > > Cc: Balakrishna Godavarthi <quic_bgodavar@quicinc.com> > > > Cc: Matthias Kaehlcke <mka@chromium.org> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > > 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. 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? Johan
On Tue, Feb 13, 2024 at 05:03:15PM +0100, Johan Hovold wrote: > On Tue, Feb 13, 2024 at 03:55:06PM +0000, Matthias Kaehlcke wrote: > > On Tue, Feb 13, 2024 at 03:41:56PM +0100, Johan Hovold wrote: > > > On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote: > > > > 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. > > > > > > > > Fixes: 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address") > > > > Cc: stable@vger.kernel.org # 5.1 > > > > Cc: Balakrishna Godavarthi <quic_bgodavar@quicinc.com> > > > > Cc: Matthias Kaehlcke <mka@chromium.org> > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > > > > > 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. > > 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? 'qcom,wcn3991-bt' should be the only impacted compatible string for released devices. Thanks for maintaining backwards compatibility, and sorry for the inconvenience.
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c index fdb0fae88d1c..29035daf21bc 100644 --- a/drivers/bluetooth/btqca.c +++ b/drivers/bluetooth/btqca.c @@ -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);
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: stable@vger.kernel.org # 5.1 Cc: Balakrishna Godavarthi <quic_bgodavar@quicinc.com> Cc: Matthias Kaehlcke <mka@chromium.org> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- 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(-)