Date:   Sun, 3 Sep 2017 14:10:12 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Marcel Holtmann <marcel@...tmann.org>
Cc:     Rob Herring <robh@...nel.org>,
        "Gustavo F. Padovan" <gustavo@...ovan.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        "open list:BLUETOOTH DRIVERS" <linux-bluetooth@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Loic Poulain <loic.poulain@...aro.org>
Subject: Re: [PATCH 2/2] Bluetooth: btqcomsmd: BD address setup

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@...aro.org>
> >>> Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.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

Powered by blists - more mailing lists