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

[Andre Heider <a.heider@gmail.com>]

On 19/11/2019 09:16, Marcel Holtmann wrote:
> 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);
> 

Maybe I misunderstood, but just for the record: It works for me even 
without this hunk (with just my hunk above and the v2 bcm patch). The 
bdaddr via dt is used and the controller works without any userland 
interaction.

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