Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] bluetooth: bcm: Use HCI_QUIRK_USE_BDADDR_PROPERTY
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Johan Hedberg <johan.hedberg@gmail.com>,
        linux-bluetooth@vger.kernel.org, Ondrej Jirman <megous@megous.com>,
        Jernej Skrabec <jernej.skrabec@siol.net>
References: <20191119060221.3297340-1-a.heider@gmail.com>
 <4DB6C9B7-8454-449C-90B4-4A1B3AD82495@holtmann.org>
From:   Andre Heider <a.heider@gmail.com>
Message-ID: <e813a126-f0fc-afe8-ad4c-5296f0a2dc06@gmail.com>
Date:   Tue, 19 Nov 2019 08:44:08 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <4DB6C9B7-8454-449C-90B4-4A1B3AD82495@holtmann.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi,

On 19/11/2019 07: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 ;)

Thanks,
Andre