Re: [PATCH 2/2] Bluetooth: btqcomsmd: BD address setup

On Fri 01 Sep 13:47 PDT 2017, Marcel Holtmann wrote:

Hi Bjorn,

quoted

Bluetooth BD address can be retrieved in the same way as
for wcnss-wlan MAC address. This patch mainly stores the
local-mac-address property and sets the BD address during
hci device setup.

Signed-off-by: Loic Poulain <redacted>
Signed-off-by: Bjorn Andersson <redacted>
---
drivers/bluetooth/btqcomsmd.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)

diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index d00c4fdae924..443bb2099329 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c

@@ -26,6 +26,7 @@

struct btqcomsmd {
	struct hci_dev *hdev;

+	const bdaddr_t *addr;
	struct rpmsg_endpoint *acl_channel;
	struct rpmsg_endpoint *cmd_channel;
};

@@ -100,6 +101,27 @@ static int btqcomsmd_close(struct hci_dev *hdev)

	return 0;
}

+static int btqcomsmd_setup(struct hci_dev *hdev)
+{
+	struct btqcomsmd *btq = hci_get_drvdata(hdev);
+	struct sk_buff *skb;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+	kfree_skb(skb);
+
+	if (btq->addr) {
+		bdaddr_t bdaddr;
+
+		/* btq->addr stored with most significant byte first */
+		baswap(&bdaddr, btq->addr);
+		return qca_set_bdaddr_rome(hdev, &bdaddr);
+	}
+
+	return 0;
+}
+
static int btqcomsmd_probe(struct platform_device *pdev)
{
	struct btqcomsmd *btq;

@@ -123,6 +145,11 @@ static int btqcomsmd_probe(struct platform_device *pdev)

	if (IS_ERR(btq->cmd_channel))
		return PTR_ERR(btq->cmd_channel);

+	btq->addr = of_get_property(pdev->dev.of_node, "local-mac-address",
+				    &ret);
+	if (ret != sizeof(bdaddr_t))
+		btq->addr = NULL;
+
	hdev = hci_alloc_dev();
	if (!hdev)
		return -ENOMEM;

@@ -135,6 +162,7 @@ static int btqcomsmd_probe(struct platform_device *pdev)

	hdev->open = btqcomsmd_open;
	hdev->close = btqcomsmd_close;
	hdev->send = btqcomsmd_send;
+	hdev->setup = btqcomsmd_setup;
	hdev->set_bdaddr = qca_set_bdaddr_rome;

I do not like this patch. Why not just set HCI_QUIRK_INVALID_BDADDR
and let a userspace tool deal with reading the BD_ADDR from some
storage.

That's what we currently have, but we regularly get complaints from
developers using our board (DB410c).

We're maintaining a Debian-based and an OpenEmbedded-based build and at
least in the past btmgmt was not available in these - so we would have
to maintain both a custom BlueZ package and then some scripts to inject
the appropriate mac address.

Beyond these reference builds our users tend to build their own system
images and I was hoping that they would not be forced to have a custom
hook running each time hci0 is registered.

Frankly I do not get this WiFI MAC address or BD_ADDR stored in DT. I
assumed the DT is suppose to describe hardware and not some value that
is normally retrieved for OTP or alike.

While I share your skepticism here I find it way superior over the
various cases where this information is hard coded in some firmware file
that has to be patched for each device - in particular when considering
the out-of-tree workarounds that follow when said firmware file is not
allowed to be modified on the device (e.g. in Android).

And note that it's not _stored_ in DT, it's passed from the boot loader
in DT - and it's still optional, so if an OEM has other means to
provision the BD_ADDR they can still handle this in user space.

Regards,
Bjorn