Hi Marcel,

thanks for the review!

On Wed, Dec 19, 2018 at 03:22:12PM +0100, Marcel Holtmann wrote:

> > Add HCI_QUIRK_USE_BDADDR_PROPERTY to allow controllers to retrieve
> > the public Bluetooth address from the firmware node property
> > 'local-bd-address'. If quirk is set and the property does not exist
> > or is invalid the controller is marked as unconfigured.
> > 
> > Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> > Reviewed-by: Balakrishna Godavarthi <bgoda...@codeaurora.org>
> > Tested-by: Balakrishna Godavarthi <bgoda...@codeaurora.org>
> > ---
> > Changes in v2:
> > - added check for return value of ->setup()
> > - only read BD_ADDR from the property if it isn't assigned yet. This
> >  is needed to support configuration from user space
> > - refactored the branch of the new quirk to get rid of 'bd_addr_set'
> > - added 'Reviewed-by: Balakrishna Godavarthi <bgoda...@codeaurora.org>' tag
> > ---
> > include/net/bluetooth/hci.h | 12 ++++++++++
> > net/bluetooth/hci_core.c    | 45 +++++++++++++++++++++++++++++++++++++
> > net/bluetooth/mgmt.c        |  6 +++--
> > 3 files changed, 61 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index c36dc1e20556a..fbba43e9bef5b 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -158,6 +158,18 @@ enum {
> >      */
> >     HCI_QUIRK_INVALID_BDADDR,
> > 
> > +   /* When this quirk is set, the public Bluetooth address
> > +    * initially reported by HCI Read BD Address command
> > +    * is considered invalid. The public BD Address can be
> > +    * specified in the fwnode property 'local-bd-address'.
> > +    * If this property does not exist or is invalid controller
> > +    * configuration is required before this device can be used.
> > +    *
> > +    * This quirk can be set before hci_register_dev is called or
> > +    * during the hdev->setup vendor callback.
> > +    */
> > +   HCI_QUIRK_USE_BDADDR_PROPERTY,
> > +
> >     /* When this quirk is set, the duplicate filtering during
> >      * scanning is based on Bluetooth devices addresses. To allow
> >      * RSSI based updates, restart scanning if needed.
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 7352fe85674be..d4149005a661e 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -30,6 +30,7 @@
> > #include <linux/rfkill.h>
> > #include <linux/debugfs.h>
> > #include <linux/crypto.h>
> > +#include <linux/property.h>
> > #include <asm/unaligned.h>
> > 
> > #include <net/bluetooth/bluetooth.h>
> > @@ -1355,6 +1356,36 @@ int hci_inquiry(void __user *arg)
> >     return err;
> > }
> > 
> > +/**
> > + * hci_dev_get_bd_addr_from_property - Get the Bluetooth Device Address
> > + *                                (BD_ADDR) for a HCI device from
> > + *                                a firmware node property.
> > + * @hdev:  The HCI device
> > + *
> > + * Search the firmware node for 'local-bd-address'.
> > + *
> > + * All-zero BD addresses are rejected, because those could be properties
> > + * that exist in the firmware tables, but were not updated by the 
> > firmware. For
> > + * example, the DTS could define 'local-bd-address', with zero BD 
> > addresses.
> > + */
> > +static int hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
> > +{
> > +   struct fwnode_handle *fwnode = dev_fwnode(hdev->dev.parent);
> > +   bdaddr_t ba;
> > +   int ret;
> > +
> > +   ret = fwnode_property_read_u8_array(fwnode, "local-bd-address",
> > +                                       (u8 *)&ba, sizeof(ba));
> > +   if (ret < 0)
> > +           return ret;
> > +   if (!bacmp(&ba, BDADDR_ANY))
> > +           return -ENODATA;
> > +
> > +   hdev->public_addr = ba;
> 
> this needs to use bacpy btw.

will change

> > +
> > +   return 0;
> > +}
> 
> Make this void since the return value is actually not used right now.

ok

> > +
> > static int hci_dev_do_open(struct hci_dev *hdev)
> > {
> >     int ret = 0;
> > @@ -1422,6 +1453,20 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> >             if (hdev->setup)
> >                     ret = hdev->setup(hdev);
> > 
> > +           if (ret)
> > +                   goto setup_failed;
> > +
> > +           if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
> > +                   if (!bacmp(&hdev->public_addr, BDADDR_ANY))
> > +                           hci_dev_get_bd_addr_from_property(hdev);
> 
> So I would move the bacmp() into
> hci_dev_get_bd_addr_from_property. Mainly since you also write the
> field there.

Personally I'm not a fan of functions that pretend to do something and
then do it conditionally, IMO it obscures the actual code flow.

I'm open to change it though if you have a strong preference for not
having the check in hci_dev_do_open().

> > +                   if (!bacmp(&hdev->public_addr, BDADDR_ANY) ||
> > +                       !hdev->set_bdaddr ||
> > +                       hdev->set_bdaddr(hdev, &hdev->public_addr))
> > +                           hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
> > +           }
> 
> So this one I don’t like since it makes my brain hurt when I have to read it 
> and understand it.

I agree, it's an ugly construct.

> I think this needs to be like this:
> 
>       if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
>           hdev->set_bdaddr)
>               ret = hdev->set_bdaddr(hdev, &hdev->public_addr);
>       else
>               err = -EADDRNOTAVAIL;
> 
> That will fail the power on procedure which is the right thing to do if the 
> local-bd-address is not present. The driver decided that it should be in DT 
> and so enforce that.

Ok, will change as suggested.

Thanks

Matthias

Reply via email to