* [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value @ 2021-10-05 4:26 Kiran K 2021-10-06 4:14 ` Tedd Ho-Jeong An 2021-10-06 8:51 ` Marcel Holtmann 0 siblings, 2 replies; 8+ messages in thread From: Kiran K @ 2021-10-05 4:26 UTC (permalink / raw) To: linux-bluetooth Cc: ravishankar.srivatsa, chethan.tumkur.narayan, tedd.an, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value 2021-10-05 4:26 [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value Kiran K @ 2021-10-06 4:14 ` Tedd Ho-Jeong An 2021-10-06 8:51 ` Marcel Holtmann 1 sibling, 0 replies; 8+ messages in thread From: Tedd Ho-Jeong An @ 2021-10-06 4:14 UTC (permalink / raw) To: Kiran K, linux-bluetooth Cc: ravishankar.srivatsa, chethan.tumkur.narayan, tedd.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) { ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value 2021-10-05 4:26 [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value Kiran K 2021-10-06 4:14 ` Tedd Ho-Jeong An @ 2021-10-06 8:51 ` Marcel Holtmann 2021-10-06 17:49 ` Luiz Augusto von Dentz 1 sibling, 1 reply; 8+ messages in thread From: Marcel Holtmann @ 2021-10-06 8:51 UTC (permalink / raw) To: Kiran K Cc: linux-bluetooth, Srivatsa Ravishankar, chethan.tumkur.narayan, tedd.an 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value 2021-10-06 8:51 ` Marcel Holtmann @ 2021-10-06 17:49 ` Luiz Augusto von Dentz 2021-10-07 6:06 ` K, Kiran 0 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2021-10-06 17:49 UTC (permalink / raw) To: Marcel Holtmann Cc: Kiran K, linux-bluetooth, Srivatsa Ravishankar, Chethan T N, An, Tedd 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value 2021-10-06 17:49 ` Luiz Augusto von Dentz @ 2021-10-07 6:06 ` K, Kiran 2021-10-12 15:51 ` K, Kiran 2021-10-12 15:56 ` Marcel Holtmann 0 siblings, 2 replies; 8+ messages in thread From: K, Kiran @ 2021-10-07 6:06 UTC (permalink / raw) To: Luiz Augusto von Dentz, Marcel Holtmann Cc: linux-bluetooth, Srivatsa, Ravishankar, Tumkur Narayan, Chethan, An, Tedd 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value 2021-10-07 6:06 ` K, Kiran @ 2021-10-12 15:51 ` K, Kiran 2021-10-12 15:56 ` Marcel Holtmann 1 sibling, 0 replies; 8+ messages in thread From: K, Kiran @ 2021-10-12 15:51 UTC (permalink / raw) To: Luiz Augusto von Dentz, Marcel Holtmann Cc: linux-bluetooth, Srivatsa, Ravishankar, Tumkur Narayan, Chethan, An, Tedd 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value 2021-10-07 6:06 ` K, Kiran 2021-10-12 15:51 ` K, Kiran @ 2021-10-12 15:56 ` Marcel Holtmann 2021-10-13 7:55 ` K, Kiran 1 sibling, 1 reply; 8+ messages in thread From: Marcel Holtmann @ 2021-10-12 15:56 UTC (permalink / raw) To: K, Kiran Cc: Luiz Augusto von Dentz, linux-bluetooth, Srivatsa, Ravishankar, Tumkur Narayan, Chethan, An, Tedd 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value 2021-10-12 15:56 ` Marcel Holtmann @ 2021-10-13 7:55 ` K, Kiran 0 siblings, 0 replies; 8+ messages in thread From: K, Kiran @ 2021-10-13 7:55 UTC (permalink / raw) To: Marcel Holtmann Cc: Luiz Augusto von Dentz, linux-bluetooth, Srivatsa, Ravishankar, Tumkur Narayan, Chethan, An, Tedd 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-13 7:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-05 4:26 [PATCH v1] Bluetooth: btintel: Fix bdaddress comparison with garbage value Kiran K 2021-10-06 4:14 ` Tedd Ho-Jeong An 2021-10-06 8:51 ` Marcel Holtmann 2021-10-06 17:49 ` Luiz Augusto von Dentz 2021-10-07 6:06 ` K, Kiran 2021-10-12 15:51 ` K, Kiran 2021-10-12 15:56 ` Marcel Holtmann 2021-10-13 7:55 ` K, Kiran
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.