[PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[Kiran K]

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@intel.com>
---
 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));
 	/* For TLV type device, parse the tlv data */
 	err = btintel_parse_version_tlv(hdev, &ver_tlv, skb);
 	if (err) {
-- 
2.17.1

Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[Tedd Ho-Jeong An]

On Tue, 2021-10-05 at 09:56 +0530, Kiran K wrote:
> 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.
> 

Reviewed-by: Tedd Ho-Jeong An <tedd.an@intel.com>

> Signed-off-by: Kiran K <kiran.k@intel.com>
> ---
>  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));
>  	/* For TLV type device, parse the tlv data */
>  	err = btintel_parse_version_tlv(hdev, &ver_tlv, skb);
>  	if (err) {

Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[Marcel Holtmann]

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@intel.com>
> ---
> 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?

Regards

Marcel

Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[Luiz Augusto von Dentz]

Hi Marcel,

On Wed, Oct 6, 2021 at 1:52 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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@intel.com>
> > ---
> > 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.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz

RE: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[K, Kiran]

Hi Marcel, Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Sent: Wednesday, October 6, 2021 11:19 PM
> To: Marcel Holtmann <marcel@holtmann.org>
> Cc: K, Kiran <kiran.k@intel.com>; linux-bluetooth <linux-
> bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; An, Tedd <tedd.an@intel.com>
> Subject: Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with
> garbage value
> 
> Hi Marcel,
> 
> On Wed, Oct 6, 2021 at 1:52 AM Marcel Holtmann <marcel@holtmann.org>
> wrote:
> >
> > 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@intel.com>
> > > ---
> > > 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.
> 
> > Regards
> >
> > Marcel
> >
> 
> 
> --
> Luiz Augusto von Dentz

RE: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[K, Kiran]

Hi Marcel,

Friendly reminder.

> -----Original Message-----
> From: K, Kiran
> Sent: Thursday, October 7, 2021 11:36 AM
> To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>; Marcel Holtmann
> <marcel@holtmann.org>
> Cc: linux-bluetooth <linux-bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; An, Tedd <tedd.an@intel.com>
> Subject: RE: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with
> garbage value
> 
> Hi Marcel, Luiz,
> 
> > -----Original Message-----
> > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> > Sent: Wednesday, October 6, 2021 11:19 PM
> > To: Marcel Holtmann <marcel@holtmann.org>
> > Cc: K, Kiran <kiran.k@intel.com>; linux-bluetooth <linux-
> > bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> > <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> > <chethan.tumkur.narayan@intel.com>; An, Tedd <tedd.an@intel.com>
> > Subject: Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison
> > with garbage value
> >
> > Hi Marcel,
> >
> > On Wed, Oct 6, 2021 at 1:52 AM Marcel Holtmann
> <marcel@holtmann.org>
> > wrote:
> > >
> > > 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@intel.com>
> > > > ---
> > > > 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.
> >
> > > Regards
> > >
> > > Marcel
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz

Thanks,
Kiran

Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[Marcel Holtmann]

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@intel.com>
>>>> ---
>>>> 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

RE: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value

[K, Kiran]

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <marcel@holtmann.org>
> Sent: Tuesday, October 12, 2021 9:27 PM
> To: K, Kiran <kiran.k@intel.com>
> Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>; linux-bluetooth <linux-
> bluetooth@vger.kernel.org>; Srivatsa, Ravishankar
> <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@intel.com>; An, Tedd <tedd.an@intel.com>
> 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@intel.com>
> >>>> ---
> >>>> 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