Re: [PATCH 1/2] Bluetooth: Add quirk for reading BD_ADDR from fwnode property

[Matthias Kaehlcke <mka@chromium.org>]

On Fri, Oct 26, 2018 at 10:31:37AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> I missed to add a point here.
> 
> On 2018-10-25 20:06, Balakrishna Godavarthi wrote:
> > On 2018-10-25 05:51, Matthias Kaehlcke 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 <mka@chromium.org>
> > > ---
> > > hci_dev_get_bd_addr_from_property() currently assumes that the
> > > firmware node with 'local-bd-address' is from hdev->dev.parent, not
> > > sure if this universally true. However if it is true for existing
> > > device that might use this interface we can assume this for now
> > > (unless there is a clear solution now), and cross the bridge of
> > > finding an alternative when we actually encounter the situation.
> > > One option could be to look for the first parent that has a fwnode.
> > > ---
> > >  include/net/bluetooth/hci.h | 12 +++++++++++
> > >  net/bluetooth/hci_core.c    | 42
> > > +++++++++++++++++++++++++++++++++++++
> > >  net/bluetooth/mgmt.c        |  6 ++++--
> > >  3 files changed, 58 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > index cdd9f1fe7cfa..a5d748099752 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 74b29c7d841c..97214262c4fb 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,9 +1356,40 @@ 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;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int hci_dev_do_open(struct hci_dev *hdev)
> > >  {
> > >  	int ret = 0;
> > > +	bool bd_addr_set = false;
> > > 
> > >  	BT_DBG("%s %p", hdev->name, hdev);
> > > 
> > > @@ -1422,6 +1454,16 @@ static int hci_dev_do_open(struct hci_dev
> > > *hdev)
> > >  		if (hdev->setup)
> > >  			ret = hdev->setup(hdev);
> > > 
> > > +		if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
> > > +			if (!hci_dev_get_bd_addr_from_property(hdev))
> > > +				if (hdev->set_bdaddr &&
> > > +				    !hdev->set_bdaddr(hdev, &hdev->public_addr))
> > > +					bd_addr_set = true;
> 
> Can we check the return status of hdev->setup() before calling
> hdev->set_bdaddr().
> some vendors assign hdev->set_baddr helper before calling hdev->setup().
> https://elixir.bootlin.com/linux/v4.19-rc8/source/drivers/bluetooth/btqcomsmd.c#L194
> There will no use in calling hdev->set_baddr() if hdev->setup() fails.

Thanks for pointing this out, I'll add the check.

This is more a question for Marcel: independently from this change I
wonder how robust the error flow in this function is. Is there are
reason to not bail out directly when a seemingly vital function like
->setup() fails, and instead continue and potentially overwrite the
error code? And there are other similar patterns in hci_dev_do_open().

Bailing out would certainly add a bit more code and probably gotos to
a cleanup section (currently in the else branch at the bottom of the
function), but might improve readability and robustness (I don't claim
there is an actual problem, but overwriting the error code seems
brittle).

Cheers

Matthias