Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4144-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <dbba45d2-f955-4d3a-aeab-26b0900d5823@quicinc.com>
Date: Mon, 29 Apr 2024 15:34:32 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Bluetooth: qca: generalise device address check
To: Johan Hovold <johan@kernel.org>, Doug Anderson <dianders@chromium.org>
CC: Johan Hovold <johan+linaro@kernel.org>,
        Marcel Holtmann
	<marcel@holtmann.org>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        <linux-bluetooth@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <stable@vger.kernel.org>, <quic_mohamull@quicinc.com>,
        <quic_hbandi@quicinc.com>, <quic_anubhavg@quicinc.com>
References: <20240426155801.25277-1-johan+linaro@kernel.org>
 <CAD=FV=V-pG9+5fLonNvydmjS=ziUFUHAyF8T7YTkEHiO405aSA@mail.gmail.com>
 <ZizKmtcUIYAMpvOQ@hovoldconsulting.com>
Content-Language: en-US
From: Janaki Ramaiah Thota <quic_janathot@quicinc.com>
In-Reply-To: <ZizKmtcUIYAMpvOQ@hovoldconsulting.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

Hi Johan,

Having a default BDA list from NVM BDA tag value will prevent developers
from using the device if there is no user space app(In Fluoride) to set
the BDA. Therefore, we are requesting to use default address check patch,
so that developer can change the NVM BDA to make use of the device.
  
  List Of default Addresses:
  ---------------------------------------------------------
|       BDA          |      Chipset                       |
  ---------------------------------------------------------
| 39 80 10 00 00 20  |  WCN3988 with ROM Version 0x0200   |
  ---------------------------------------------------------
| 39 80 12 74 08 00  |  WCN3988 with ROM Version 0x0201   |
  ---------------------------------------------------------
| 39 90 21 64 07 00  |  WCN3990                           |
  ---------------------------------------------------------
| 39 98 00 00 5A AD  |  WCN3991                           |
  ---------------------------------------------------------
| 00 00 00 00 5A AD  |  QCA DEFAULT                       |
  ---------------------------------------------------------

On 4/27/2024 3:21 PM, Johan Hovold wrote:
> On Fri, Apr 26, 2024 at 10:23:15AM -0700, Doug Anderson wrote:
>> On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>>>
>>> The default device address apparently comes from the NVM configuration
>>> file and can differ quite a bit.
>>>
>>> Store the default address when parsing the configuration file and use it
>>> to determine whether the controller has been provisioned with an
>>> address.
>>>
>>> This makes sure that devices without a unique address start as
>>> unconfigured unless a valid address has been provided in the devicetree.
> 
>>>   int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>>>                           enum qca_btsoc_type soc_type)
>>>   {
>>> @@ -351,6 +348,11 @@ static void qca_tlv_check_data(struct hci_dev *hdev,
>>>
>>>                          /* Update NVM tags as needed */
>>>                          switch (tag_id) {
>>> +                       case EDL_TAG_ID_BD_ADDR:
>>> +                               if (tag_len != sizeof(bdaddr_t))
>>> +                                       break;
>>> +                               memcpy(&config->bdaddr, tlv_nvm->data, sizeof(bdaddr_t));
>>> +                               break;
>>>                          case EDL_TAG_ID_HCI:
>>
>> nit: blank line after "break" ?
> 
> Possibly, the driver isn't really consistent here and only two case
> statements have such a newline after break.
> 
>> Also note that on my firmware I never see this tag and thus your patch
>> breaks trogdor. Specifically I put a printout here and it never gets
>> hit.
> 
> Thanks for the quick test. As the parser is modifying the configuration
> file I assumed it was correct and tested...
>   
>> I printed all the tags/lengths:
>>
>> [   17.961087] DOUG: id 0xde02, len 0x0010
>> [   17.965081] DOUG: id 0x0000, len 0x0000
>> [   17.969050] DOUG: id 0x0000, len 0x0011
>> [   17.973025] DOUG: id 0x0000, len 0x0a00
>> [   17.976991] DOUG: id 0x0303, len 0x0303
>> [   17.981066] DOUG: id 0x0033, len 0x1001
>>
>> Probably EDL_TAG_ID_BD_ADDR should have been 0xde02, not just 2.
> 
> No, the parser is apparently broken and fails to consider an extra
> four-byte header found in some NVM files and just happily parses and
> potentially modifies (sic!) random bytes.
> 
> I've fixed the parser so that it works also on configuration files with
> the extra header (apnv??.bin, crnv??[u].bin) and can read out the
> default address for all NVM files in linux-firmware that have one
> (otherwise all-zeroes is printed below):
> 
> bluetooth hci0: bd_addr = 39:80:10:00:00:20 (qca/apnv10.bin)
> bluetooth hci0: bd_addr = 39:80:12:74:08:00 (qca/apnv11.bin)
> bluetooth hci0: bd_addr = 39:90:21:64:07:00 (qca/crnv21.bin)
> bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32.bin)
> bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32u.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.301)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.302)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.309)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.301)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.302)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.309)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/htnv20.bin)
> bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b09)
> bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b0a)
> bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.bin)
> bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin)
> bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin)
> 
> bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_00230302.bin)
> 
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_eu.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_i2s_eu.bin)
> 
> bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000200.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000201.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000300.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302_eu.bin)
> 
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0104.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0105.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0106.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0107.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0109.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0110.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010a.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010b.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_0303.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010a.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010b.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_0303.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf.bin)
> bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00190200.bin)
> 
> It looks like we're being lucky and the parser is at least not
> corrupting the configuration files with the extra header currently in
> linux-firmware, but if it ever interprets a random 0x0011 or 0x001b word
> as a tag it would.
> 
> Fixing the parser means that we would start modifying the configuration
> also for files with the extra header. This involves configuring the baud
> rate and enabling a deep sleep feature.
> 
> Presumably this is something that should be done also on Trogdor, but
> this would obviously have to be tested first. I guess we can keep
> skipping this step until it has been verified and just read out the
> address for now.
> 
>>> @@ -624,6 +626,9 @@ static int qca_check_bdaddr(struct hci_dev *hdev)
>>>          if (bacmp(&hdev->public_addr, BDADDR_ANY))
>>>                  return 0;
>>>
>>> +       if (!bacmp(&config->bdaddr, BDADDR_ANY))
>>> +               return 0;
>>
>> The above test feels non-obvious enough to deserve a comment. Could
>> you add one? That would also help alleviate my confusion since I
>> _think_ your if test is unneeded and maybe wrong? Let's say that the
>> firmware didn't have a default address stored in it. It still seems
>> like we could try to read the address and then if the firmware gave
>> back BDADDR_ANY (0) we should set the `HCI_QUIRK_USE_BDADDR_PROPERTY`
>> property, right?
> 
> You're right. I'll drop this check when revisiting this next week.
> 
> Johan

-Janaki Ram