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

Hi Marcel,

thanks for the review!

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

quoted

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>
Reviewed-by: Balakrishna Godavarthi <redacted>
Tested-by: Balakrishna Godavarthi <redacted>
---
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 [off-list ref]' 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

quoted

+
+	return 0;
+}

Make this void since the return value is actually not used right now.

ok

quoted

+
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().

quoted

+			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