[PATCH 0/2] btqcomsmd: Allow specifying board mac address

[PATCH 0/2] btqcomsmd: Allow specifying board mac address

[Bjorn Andersson]

The btqcomsmd hardware lacks persistent storage of its mac address, so this
needs to be configured during initialization. The second patch in this series
reads the mac address from DT and does this, allowing the boot loader to
populate this board specific information.

Loic Poulain (2):
  Bluetooth: make baswap src const
  Bluetooth: btqcomsmd: BD address setup

 drivers/bluetooth/btqcomsmd.c     | 28 ++++++++++++++++++++++++++++
 include/net/bluetooth/bluetooth.h |  2 +-
 net/bluetooth/lib.c               |  4 ++--
 3 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.12.0

[PATCH 1/2] Bluetooth: make baswap src const

[Bjorn Andersson]

From: Loic Poulain <loic.poulain@linaro.org>

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 include/net/bluetooth/bluetooth.h | 2 +-
 net/bluetooth/lib.c               | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 01487192f628..020142bb9735 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -233,7 +233,7 @@ static inline void bacpy(bdaddr_t *dst, const bdaddr_t *src)
 	memcpy(dst, src, sizeof(bdaddr_t));
 }
 
-void baswap(bdaddr_t *dst, bdaddr_t *src);
+void baswap(bdaddr_t *dst, const bdaddr_t *src);
 
 /* Common socket structures and functions */
 
diff --git a/net/bluetooth/lib.c b/net/bluetooth/lib.c
index aa4cf64e32a6..6048cc07568b 100644
--- a/net/bluetooth/lib.c
+++ b/net/bluetooth/lib.c
@@ -30,10 +30,10 @@
 
 #include <net/bluetooth/bluetooth.h>
 
-void baswap(bdaddr_t *dst, bdaddr_t *src)
+void baswap(bdaddr_t *dst, const bdaddr_t *src)
 {
+	const unsigned char *s = (const unsigned char *)src;
 	unsigned char *d = (unsigned char *) dst;
-	unsigned char *s = (unsigned char *) src;
 	unsigned int i;
 
 	for (i = 0; i < 6; i++)
-- 
2.12.0

Re: [PATCH 1/2] Bluetooth: make baswap src const

[Marcel Holtmann]

Hi Bjorn,

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> include/net/bluetooth/bluetooth.h | 2 +-
> net/bluetooth/lib.c               | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

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

[Bjorn Andersson]

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(+)

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);
-- 
2.12.0

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

[Marcel Holtmann]

Hi Bjorn,

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

Regards

Marcel

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

[Rob Herring]

On Fri, Sep 1, 2017 at 3:47 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Bjorn,
>
>> 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.

Rob

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

[Marcel Holtmann]

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.

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.

Regards

Marcel

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

> Hi Bjorn,
> 
> > 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.
> 

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

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

[Marcel Holtmann]

Hi Bjorn,

>>> 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.
>> 
> 
> That's what we currently have, but we regularly get complaints from
> developers using our board (DB410c).

at least not in the upstream driver. It does not use HCI_QUIRK_INVALID_BDADDR to tell the system that its BD_ADDR is not valid. Which is something you still need to do if local-mac-address would not be found.

What BD_ADDR is actually returned by default. Can someone send me a “btmon -w trace.log” for an init procedure of this chip?

> 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 this has never been about btmgmt usage. That tool is really just for us to test the interface. What was needed is that we create a small daemon that can have backends for accessing the various OTPs. Or in dev mode just generate a random OUI from an unused OUI range. I would have put that into bluetoothd, but it seemed not a good idea since many companies were secret about their OTP access. So I assumed they build there own quick solution since mgmt API is fully documented and you only need to listen for Unconfigured Index event, send Set Public Address and leave. So something super simple.

For a LE only controller without a BD_ADDR, we recently added a pool of static addresses that it will generate and program. However that is specific since LE is capable of operating without a public address.

We could actually downgrade a dual-mode controller without a BD_ADDR into a single mode controller. That will automatically start using static addresses and be fully operational. That might be useful for people who get a dual-mode controller, but only care about LE. I have seen devices that only use the LE portion.

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

That I full agree. Storing the BD_ADDR in the firmware file is utterly pointless. That is just insane way of operation and that is why we added the ability to make interfaces clearly as unconfigured when they miss the BD_ADDR. And have one way to userspace to configure it.

The DT file is nothing better if it is something that is generated statically once. It has the same problem. You are just papering over it and telling someone else to deal with it.

When the bootloader dynamically generated the DT and inserts a local-mac-address field based on its own OTP, then I can see that this is possible, but for static DT that for example the kernel ships, this is a super bad idea. As bad as the BD_ADDR in the firmware file itself.

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

That is really not clear to me and I do not even think the documentation has all the needed warnings. The Bluetooth devices is super critical for having its unique BD_ADDR for Bluetooth Classic operation. Things will really go bonkers. And they will since I had to already fly half around the world to debug issues with wrong mac addresses.

If we now all want to blame the bootloader, then so be it. Nevertheless you are still not using HCI_QUIRK_INVALID_BDADDR if the bootloader can’t be blamed. That is actually a bad idea since now also userspace thinks it is all fine.

And I did write a lengthy email about this about 3 years ago:

http://linux-bluetooth.vger.kernel.narkive.com/9JnHPGAf/some-notes-about-device-provisioning

So since I think this is actually dangerous to have the BD_ADDR come from DT, I think these needs a few extra bits of documentation on the usage of local-mac-address in the DT documentation, in addition we might want to print that the Bluetooth address comes from DT and which one in dmesg. So that at least there is some chance of debugging if you get this fully wrong. And you need to use HCI_QUIRK_INVALID_BDADDR. That is actually a real bug right now if the hardware never has a valid address.

Regards

Marcel