Message ID | 20231227180306.6319-1-johan+linaro@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-12190-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:6f82:b0:100:9c79:88ff with SMTP id tb2csp1584540dyb; Wed, 27 Dec 2023 10:04:57 -0800 (PST) X-Google-Smtp-Source: AGHT+IFwUGCxCOfZmeAPPdWiiLOLIOzeJLLuZWnH+bnsHCXZEijb0pjuU1oWG8LH7XHOziURUPx9 X-Received: by 2002:a05:6102:3f8b:b0:465:d6ee:14a6 with SMTP id o11-20020a0561023f8b00b00465d6ee14a6mr6958258vsv.23.1703700297089; Wed, 27 Dec 2023 10:04:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703700297; cv=none; d=google.com; s=arc-20160816; b=DwQpBodYZoB4PGXKbJdPIxeMDoek9gpcthK/uIMmKqTplltuBWAPdnxtt+j9DX8gK6 Qryq23teT1qZqDzo2uNnHlLjGAWrjVV231c+IjZrUKaZVXLTeHJBueF2cbcvvsQIzZmd jrBx7tt9l2BJ4+l3khkgUr6hNDZwquLf16nvW6aRw1YCkFFuIZQDAyJGc81ZdqA/1p1A arqnZ+m2UB4mTNlE6Q1OZVZOzC7WSGiDahoxS8wmjDFUKoA06AQlRwFG0EizfFQvzyxp ZNclUJssO6Sl3nwg1yhM096IIspfItyWkJVckMOKNEudQL2cbyIc29G6ndIlE6wsPH9w tbbw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=4u5qrCcvMhdCaF5+4+MYnREuWT6QxFRyC6uNkm1VQMw=; fh=QkA2fdtUjJ3r14NLOCiFydbWOO3juImWjSP1e0qldP8=; b=kBh1TqsYUZzquS/WGMZJz6KsY6sozoPTmY9lTWgNs5UWZLwo37Q7fRnYQpJmoSYTGB eaLQfR5asOCdup8QCFCOLOfsxNQyMZBrxDYeM4AdiYlIqgVZB/yzr97yD4HVIcg6p+1B rKqa95WRSldh2X7r65lGDBrt1/SRf4zEWuqrSMRj90kKSTI2LDPKsD2c2sa3VTdVgjI5 Eba0xnRzqV76F6uSDQdJf7B3kvEc2EnUNVtOyYtEiPHT/mnwY6itDBGuOC8PTLvI3l+3 s9MkHu4AZ4WSDLCZ59wdBXPJj1D8Fb40k//qg1dX4PfZ+gT7jsdtdUtWEYt3SOvzEbfD /6xA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=ZhpGayBt; spf=pass (google.com: domain of linux-kernel+bounces-12190-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-12190-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [147.75.199.223]) by mx.google.com with ESMTPS id e22-20020a056102355600b00466f88727b5si790438vss.573.2023.12.27.10.04.56 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 Dec 2023 10:04:57 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-12190-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=ZhpGayBt; spf=pass (google.com: domain of linux-kernel+bounces-12190-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) smtp.mailfrom="linux-kernel+bounces-12190-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id B59281C2144C for <ouuuleilei@gmail.com>; Wed, 27 Dec 2023 18:04:55 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 587364776D; Wed, 27 Dec 2023 18:04:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ZhpGayBt" X-Original-To: linux-kernel@vger.kernel.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A115346522; Wed, 27 Dec 2023 18:04:34 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 10EF6C433C8; Wed, 27 Dec 2023 18:04:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1703700274; bh=Z5Xx4Awhm2yDAU7eR4KCbbH90zyc67q6J3SQ9KIFSvE=; h=From:To:Cc:Subject:Date:From; b=ZhpGayBt8lKxIXkCy85p6bKIupO7GjLDohiqZns/nqowkDm6XfGlaj50PGSgHvQOq JhklV8O3Gk9WP227ddow0eJBwVcggrz/iZBd2aYKn6MR3VaDf1+0xdDaqZ1Qp14+KW HrRaDj6JxnnNvmDe1ysZckUpI24H3qqo4UKVq1+UocGe3mTGIa0wiWiflwuhxY3Gkk xkr3KaY9NGiBFpR5xrWkih/GDFh1u3JhZj6id/6nEZ+ZAZ2YMoxA1OihjvSnTQZCky junga5yF5xEDplOuDZArTheqGozslPGnfZ+0Aq7mezsAfU6TGgt7EG4CSryARtcjWc lmWRFG+B+wJZA== Received: from johan by xi.lan with local (Exim 4.96.2) (envelope-from <johan+linaro@kernel.org>) id 1rIYGb-0001fI-1l; Wed, 27 Dec 2023 19:04:29 +0100 From: Johan Hovold <johan+linaro@kernel.org> To: Marcel Holtmann <marcel@holtmann.org>, Johan Hedberg <johan.hedberg@gmail.com>, Luiz Augusto von Dentz <luiz.dentz@gmail.com> Cc: Bjorn Andersson <quic_bjorande@quicinc.com>, Konrad Dybcio <konrad.dybcio@linaro.org>, linux-bluetooth@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org>, stable@vger.kernel.org, Balakrishna Godavarthi <quic_bgodavar@quicinc.com>, Matthias Kaehlcke <mka@chromium.org> Subject: [PATCH] Bluetooth: qca: fix device-address endianness Date: Wed, 27 Dec 2023 19:03:06 +0100 Message-ID: <20231227180306.6319-1-johan+linaro@kernel.org> X-Mailer: git-send-email 2.41.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1786459242455400301 X-GMAIL-MSGID: 1786459242455400301 |
Series |
Bluetooth: qca: fix device-address endianness
|
|
Commit Message
Johan Hovold
Dec. 27, 2023, 6:03 p.m. UTC
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(-)
Comments
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);