Hi Kiran,
>>>> Intel Read Verision(TLV) data is parsed into a local structure
>>>> variable and it contains a field for bd address. Bd address is
>>>> returned only in bootloader mode and hence bd address in TLV
>>>> structure needs to be validated only if controller is present in boot loader
>> mode.
>>>>
>>>> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
>>>> ---
>>>> drivers/bluetooth/btintel.c | 19 +++++++++++--------
>>>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btintel.c
>>>> b/drivers/bluetooth/btintel.c index 9359bff47296..d1703cc99705
>>>> 100644
>>>> --- a/drivers/bluetooth/btintel.c
>>>> +++ b/drivers/bluetooth/btintel.c
>>>> @@ -2081,14 +2081,16 @@ static int
>> btintel_prepare_fw_download_tlv(struct hci_dev *hdev,
>>>> if (ver->img_type == 0x03) {
>>>> btintel_clear_flag(hdev, INTEL_BOOTLOADER);
>>>> btintel_check_bdaddr(hdev);
>>>> - }
>>>> -
>>>> - /* If the OTP has no valid Bluetooth device address, then there will
>>>> - * also be no valid address for the operational firmware.
>>>> - */
>>>> - if (!bacmp(&ver->otp_bd_addr, BDADDR_ANY)) {
>>>> - bt_dev_info(hdev, "No device address configured");
>>>> - set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
>>>> + } else {
>>>> + /*
>>>> + * Check for valid bd address in boot loader mode. Device
>>>> + * will be marked as unconfigured if empty bd address is
>>>> + * found.
>>>> + */
>>>> + if (!bacmp(&ver->otp_bd_addr, BDADDR_ANY)) {
>>>> + bt_dev_info(hdev, "No device address configured");
>>>> + set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
>>>> + }
>>>> }
>>>>
>>>> btintel_get_fw_name_tlv(ver, fwname, sizeof(fwname), "sfi");
>>>> @@ -2466,6 +2468,7 @@ static int btintel_setup_combined(struct
>> hci_dev *hdev)
>>>> goto exit_error;
>>>> }
>>>>
>>>> + memset(&ver_tlv, 0, sizeof(ver_tlv));
>>>
>>> this change is not described in the commit message. Why is that now out of
>> a sudden needed?
>>
>> I guess this is just to make sure the ver_tlv is initialized so its otp_bd_addr be
>> set to all zeros (BDADDR_ANY) otherwise the code above doesn't work as it
>> attempts to compare to BDADDR_ANY.
>
> Yes. If not memset, then garbage value is compared against BDADDR_ANY.
since that is not obviously clear, the takeaway from my review should have been that you either describe this properly in the commit message or you add a comment.
Regards
Marcel
