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

Message ID 20170901204118.17123-3-bjorn.andersson@linaro.org
State New
Headers show
Series
  • btqcomsmd: Allow specifying board mac address
Related show

Commit Message

Bjorn Andersson Sept. 1, 2017, 8:41 p.m.
From: Loic Poulain <loic.poulain@linaro.org>


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 <loic.poulain@linaro.org>

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---
 drivers/bluetooth/btqcomsmd.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

-- 
2.12.0

Comments

Bjorn Andersson Sept. 3, 2017, 9:10 p.m. | #1
On Fri 01 Sep 23:12 PDT 2017, Marcel Holtmann wrote:

> Hi Rob,

> 

> >>> 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 <loic.poulain@linaro.org>

> >>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> >>> ---

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

Patch

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;
 
 	ret = hci_register_dev(hdev);