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

On Fri 01 Sep 23:12 PDT 2017, Marcel Holtmann wrote:

Hi Rob,

quoted

quoted

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.

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.

Use of "local-mac-address" for ethernet at least has existed as long
at OpenFirmware I think. For some platforms, DT is the only OTP. And
sometimes, the bootloader (like u-boot) stores MAC addresses and then
populates them on boot.

Seems like if we just let userspace deal with it, then we're back to a
btattach tool with every platform's specific way of reading the MAC
address.

for Bluetooth that is not true. We have Set Public Address command
that is uniquely handling this and the HCI_QUIRK_INVALID_BDADDR
address does the right magic to allow userspace to identify a missing
address. It is done nicely and correctly and works fine.

You're right in that we have a nice standardized way of informing user
space that the bd address is invalid and a nice standardized way for the
user to specify an address.

What I believe Rob is saying (and what is my problem) is that the user
space tool reading the address from somewhere and calling this API is
not standard - so we end up maintaining some custom
"read-address-and-call-public-address"-tool for both Debian and
OpenEmbedded, plus we need to ensure that all our customers include and
launch this tool in their own systems.

Mind you this is even used when there actually is a BD_ADDR, but the
device manufacturer wants to have one from its own OUI range compared
to the chip manufacturer’s OUI range.

If DT is really the only place for the BD_ADDR and the bootloader
kinda does add / merge it into the DT, then by all means that is fine.
However if it is not, then this feature is dangerous since it can lead
to multiple devices with the same address. I rather have these devices
leave the kernel in unconfigured mode. And then force a userspace tool
to use Set Public Address to bring it into configured mode.

The "new" property is optional and think that for devices that has not
been provisioned with a bd address the boot loader should not add this
property.


Base on the fact that the firmware is built with the assumption that the
host will set the bd address I think the patch should be amended to
specify HCI_QUIRK_INVALID_BDADDR in the absence of this property.

Regards,
Bjorn