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@xxxxxxxxxx>
> >>> Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> >>> ---
> >>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
