| Message ID | 20170901204118.17123-3-bjorn.andersson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | btqcomsmd: Allow specifying board mac address | expand |
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
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);