Hi Marcel,
> -----Original Message-----
> From: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Sent: Tuesday, October 12, 2021 9:27 PM
> To: K, Kiran <kiran.k@xxxxxxxxx>
> Cc: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>; linux-bluetooth <linux-
> bluetooth@xxxxxxxxxxxxxxx>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; An, Tedd <tedd.an@xxxxxxxxx>
> Subject: Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with
> garbage value
>
> 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.
I will add a comment and send an updated patch. Thanks.
>
> Regards
>
> Marcel
