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@xxxxxxxxx>
>>> ---
>>> 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
