linux-kernel - Re: [PATCH] Revert "Bluetooth: hci_qca: Set BDA quirk bit if fwnode exists in DT"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zg1KmcFQ3bAJa8qJ@hovoldconsulting.com>
Date: Wed, 3 Apr 2024 14:24:57 +0200
From: Johan Hovold <johan@...nel.org>
To: Janaki Ramaiah Thota <quic_janathot@...cinc.com>
Cc: Johan Hovold <johan+linaro@...nel.org>, luiz.dentz@...il.com,
	marcel@...tmann.org, linux-bluetooth@...r.kernel.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org,
	quic_mohamull@...cinc.com, quic_hbandi@...cinc.com
Subject: Re: [PATCH] Revert "Bluetooth: hci_qca: Set BDA quirk bit if fwnode
 exists in DT"

On Fri, Mar 29, 2024 at 12:55:40PM +0530, Janaki Ramaiah Thota wrote:
> On 3/28/2024 8:53 PM, Johan Hovold wrote:
> > On Thu, Mar 28, 2024 at 08:25:16PM +0530, Janaki Ramaiah Thota wrote:

> >> We made this change to configure the device which supports persistent
> >> memory for the BD-Address
> > 
> > Can you say something more about which devices support persistent
> > storage for the address? Is that all or just some of the chip variants?

> Most of the devices support persistent storage, and bd-address storage
> is chosen based on the OEM and Target.

Can you be more specific about which devices support it (or say which do
not)?

Is the address stored in some external eeprom or similar which the OEM
can choose to populate?

> >> So to make device functional in both scenarios we are adding a new
> >> property in dts file to distinguish persistent and non-persistent
> >> support of BD Address and set HCI_QUIRK_USE_BDADDR_PROPERTY bit
> >> accordingly
> > 
> > Depending on the answer to my questions above, you may be able to infer
> > this from the compatible string and/or you can read out the address from
> > the device and only set the quirk if it's set to the default address.
> > 
> > You should not need to add a new property for this.

> As per my understanding, altering the compatible string may cause duplicate
> configuration, right ?

If it's the same device and just a different configuration then we can't
use the compatible string for this.

It seems we need a patch like the below to address this. But please
provide some more details (e.g. answers to the questions above) so I can
determine what the end result should look like.

Johan


>From 9719effe80fcc17518131816fdfeb1824cfa08b6 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan+linaro@...nel.org>
Date: Thu, 20 Apr 2023 14:10:55 +0200
Subject: [PATCH] Bluetooth: btqca: add invalid device address check

Some Qualcomm Bluetooth controllers lack persistent storage for the
device address and therefore end up using the default address
00:00:00:00:5a:ad.

Apparently this depends on how the controller has been integrated so we
can not use the device type alone to determine when the address is
valid.

Instead read back the address during setup() and only set the
HCI_QUIRK_USE_BDADDR_PROPERTY flag when needed.

Fixes: de79a9df1692 ("Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY")
Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts")
Fixes: 6945795bc81a ("Bluetooth: fix use-bdaddr-property quirk")
Cc: stable@...r.kernel.org	# 6.5
Signed-off-by: Johan Hovold <johan+linaro@...nel.org>
---
 drivers/bluetooth/btqca.c   | 33 +++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_qca.c |  2 --
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 19cfc342fc7b..15124157372c 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -15,6 +15,8 @@
 
 #define VERSION "0.1"
 
+#define QCA_BDADDR_DEFAULT (&(bdaddr_t) {{ 0xad, 0x5a, 0x00, 0x00, 0x00, 0x00 }})
+
 int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
 			 enum qca_btsoc_type soc_type)
 {
@@ -612,6 +614,35 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 }
 EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
 
+static void qca_check_bdaddr(struct hci_dev *hdev)
+{
+	struct hci_rp_read_bd_addr *bda;
+	struct sk_buff *skb;
+	int err;
+
+	if (bacmp(&hdev->public_addr, BDADDR_ANY))
+		return;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_READ_BD_ADDR, 0, NULL,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hdev, "Failed to read device address (%d)", err);
+		return;
+	}
+
+	if (skb->len != sizeof(*bda)) {
+		bt_dev_err(hdev, "Device address length mismatch");
+		goto free_skb;
+	}
+
+	bda = (struct hci_rp_read_bd_addr *)skb->data;
+	if (!bacmp(&bda->bdaddr, QCA_BDADDR_DEFAULT))
+		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
+free_skb:
+	kfree_skb(skb);
+}
+
 static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
 		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
 {
@@ -818,6 +849,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		break;
 	}
 
+	qca_check_bdaddr(hdev);
+
 	bt_dev_info(hdev, "QCA setup on UART is completed");
 
 	return 0;
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index b266db18c8cc..b621a0a40ea4 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1908,8 +1908,6 @@ static int qca_setup(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
-		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
-
 		qcadev = serdev_device_get_drvdata(hu->serdev);
 		if (qcadev->bdaddr_property_broken)
 			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
-- 
2.43.2