Re: [PATCH v2] bluetooth: bcm: Use HCI_QUIRK_USE_BDADDR_PROPERTY

[Marcel Holtmann <marcel@holtmann.org>]

Hi Andre,

>>> Some devices ship with the controller default address, like the
>>> Orange Pi 3 (BCM4345C5).
>>> 
>>> Allow the bootloader to set a valid address through the device tree.
>>> 
>>> Signed-off-by: Andre Heider <a.heider@gmail.com>
>>> ---
>>> drivers/bluetooth/btbcm.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
>>> index 2d2e6d862068..9d16162d01ea 100644
>>> --- a/drivers/bluetooth/btbcm.c
>>> +++ b/drivers/bluetooth/btbcm.c
>>> @@ -439,6 +439,7 @@ int btbcm_finalize(struct hci_dev *hdev)
>>> 	btbcm_check_bdaddr(hdev);
>>> 
>>> 	set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
>>> +	set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>>> 
>>> 	return 0;
>>> }
>> have you actually tested this? I might be mistaken, but the code that I read in hci_dev_do_open() would drop this into unconfigured state since HCI_QURIK_INVALID_BDADDR is still set.
> 
> I thought so, but double-checking something obviously failed...
> 
> What would be an acceptable solution to this HCI_QUIRK_USE_BDADDR_PROPERTY|HCI_QUIRK_INVALID_BDADDR situation?
> 
> Getting rid of the quirk in the driver in e.g. set_bdaddr() doesn't sound right.
> 
> How about:
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 04bc79359a17..7bc384be89f8 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1470,7 +1470,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>         * start up as unconfigured.
>         */
>        if (test_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks) ||
> -           test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks))
> +           (test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) &&
> +            !test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)))
>            hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
> 
>        /* For an unconfigured controller it is required to
> 
> That works for me (double-checked this time ;)

I am not sure yet. I mean we define what HCI_QUIRK_USE_BDADDR_PROPERTY actually means. Right now it means this:

1) Run though ->setup
2) If no public BD_ADDR is set, then try to read from DT
3) If found, try to set, if set fails, abort dev_up

Now there is also another problem that no public BD_ADDR means BDADDR_ANY right now. Which means, for Broadcom chips that is never the case. So HCI_QUIRK_USE_BDADDR_PROPERTY doesn’t even work since their invalid addresses are not BDDADDR_ANY.

The first change needs to be something along these lines:

                if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
-                       if (!bacmp(&hdev->public_addr, BDADDR_ANY))
+                       if (!bacmp(&hdev->public_addr, BDADDR_ANY) ||
+                           test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks))
                                hci_dev_get_bd_addr_from_property(hdev);

But that is not fully correct either. We also have to consider HCI_QUIRK_NON_PERSISTENT_SETUP.

So this is not an easy fix since we need to spell out the semantics of the interactions of these 3 quirks first.

Regards

Marcel