[PATCH] Bluetooth: qca: fix device-address endianness

[PATCH] Bluetooth: qca: fix device-address endianness

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

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

RE: Bluetooth: qca: fix device-address endianness

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1423 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=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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Nikita Travkin]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Matthias Kaehlcke]

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
> 

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Johan Hovold]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Matthias Kaehlcke]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Johan Hovold]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Doug Anderson]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Johan Hovold]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Luiz Augusto von Dentz]

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



-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Johan Hovold]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Luiz Augusto von Dentz]

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



-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Johan Hovold]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Johan Hovold]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Matthias Kaehlcke]

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.

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Johan Hovold]

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

Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Matthias Kaehlcke]

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.