Hi Luiz,
On Wed, 2021-02-10 at 11:20 -0800, Luiz Augusto von Dentz wrote:
> Hi Tedd,
>
> On Wed, Feb 10, 2021 at 11:10 AM An, Tedd <tedd.an@xxxxxxxxx> wrote:
> > On Wed, 2021-02-10 at 08:59 -0800, Luiz Augusto von Dentz wrote:
> > > From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > >
> > > In order to allow new firmware to be loaded it first needs to check if
> > > the firmware version on file matches the one loaded if it doesn't then
> > > it needs to revert to boorloader mode in order to load the new firmware.
> > >
> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > > ---
> > > drivers/bluetooth/btintel.c | 22 +++++++++++
> > > drivers/bluetooth/btusb.c | 74 +++++++++++++++----------------------
> > > 2 files changed, 52 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > > index 89f85d54ca64..0d0f643f972a 100644
> > > --- a/drivers/bluetooth/btintel.c
> > > +++ b/drivers/bluetooth/btintel.c
> > > @@ -949,6 +949,17 @@ int btintel_download_firmware(struct hci_dev *hdev,
> > > return -EALREADY;
> > > }
> > >
> > > + /* The firmware variant determines if the device is in bootloader
> > > + * mode or is running operational firmware. The value 0x06 identifies
> > > + * the bootloader and the value 0x23 identifies the operational
> > > + * firmware.
> > > + *
> > > + * If the firmware version has changed that means it needs to be reset
> > > + * to bootloader when operational so the new firmware can be loaded.
> > > + */
> > > + if (ver->fw_variant == 0x23)
> > > + return -EINVAL;
> > > +
> > > err = btintel_sfi_rsa_header_secure_send(hdev, fw);
> > > if (err)
> > > return err;
> > > @@ -976,6 +987,17 @@ int btintel_download_firmware_newgen(struct hci_dev *hdev,
> > > return -EALREADY;
> > > }
> > >
> > > + /* The firmware variant determines if the device is in bootloader
> > > + * mode or is running operational firmware. The value 0x03 identifies
> > > + * the bootloader and the value 0x23 identifies the operational
> > > + * firmware.
> > > + *
> > > + * If the firmware version has changed that means it needs to be reset
> > > + * to bootloader when operational so the new firmware can be loaded.
> > > + */
> > > + if (ver->img_type == 0x03)
> > > + return -EINVAL;
> > > +
> > > /* iBT hardware variants 0x0b, 0x0c, 0x11, 0x12, 0x13, 0x14 support
> > > * only RSA secure boot engine. Hence, the corresponding sfi file will
> > > * have RSA header of 644 bytes followed by Command Buffer.
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index c92060e7472c..a44f3cf25790 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -2469,14 +2469,30 @@ static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > return -EILSEQ;
> > > }
> > >
> > > -static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
> > > +static int btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
> > > struct intel_boot_params *params,
> > > char *fw_name, size_t len,
> > > const char *suffix)
> > > {
> > > + /* The hardware platform number has a fixed value of 0x37 and
> > > + * for now only accept this single value.
> > > + */
> > > + if (ver->hw_platform != 0x37)
> > > + return -EINVAL;
> > > +
> > > switch (ver->hw_variant) {
> > > case 0x0b: /* SfP */
> > > case 0x0c: /* WsP */
> > > + /* The firmware variant determines if the device is in
> > > + * bootloader mode or is running operational firmware.
> > > + *
> > > + * Version checking cannot be performed in these models since
> > > + * the firmware versioning depends on the firmware being in
> > > + * bootloader mode.
> > > + */
> > > + if (ver->fw_variant == 0x23)
> > > + return -EALREADY;
> > > +
> > > snprintf(fw_name, len, "intel/ibt-%u-%u.%s",
> > > le16_to_cpu(ver->hw_variant),
> > > le16_to_cpu(params->dev_revid),
> > > @@ -2493,9 +2509,10 @@ static bool btusb_setup_intel_new_get_fw_name(struct intel_version *ver,
> > > suffix);
> > > break;
> > > default:
> > > - return false;
> > > + return -EINVAL;
> > > }
> > > - return true;
> > > +
> > > + return 0;
> >
> > There is one more place in btusb_setup_intel_new()@btusb.c to update the handling of return
> > value of this funcion, which is related to loading the DDC.
> > Code like this...
> >
> > if (!err) {
> > bt_dev_err(hdev, "Unsupported Intel firmware naming");
> > } else {
>
> This should never fail though since the first one would check if the
> firmware name cannot be generated if would fail and never reach this
> one, that said perhaps static analyzer will complain about not
> checking the return here.
>
It is not for failure case. For success case, it should get the DDC file name.
It gets correct name with err=0, which should be an error.
> >
> > > }
> > >
> > > static void btusb_setup_intel_newgen_get_fw_name(const struct intel_version_tlv *ver_tlv,
> > > @@ -2550,7 +2567,6 @@ static int btusb_intel_download_firmware_newgen(struct hci_dev *hdev,
> > > if (ver->img_type == 0x03) {
> > > clear_bit(BTUSB_BOOTLOADER, &data->flags);
> > > btintel_check_bdaddr(hdev);
> > > - return 0;
> > > }
> > >
> > > /* Check for supported iBT hardware variants of this firmware
> > > @@ -2694,35 +2710,6 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> > > if (!ver || !params)
> > > return -EINVAL;
> > >
> > > - /* The hardware platform number has a fixed value of 0x37 and
> > > - * for now only accept this single value.
> > > - */
> > > - if (ver->hw_platform != 0x37) {
> > > - bt_dev_err(hdev, "Unsupported Intel hardware platform (%u)",
> > > - ver->hw_platform);
> > > - return -EINVAL;
> > > - }
> > > -
> > > - /* Check for supported iBT hardware variants of this firmware
> > > - * loading method.
> > > - *
> > > - * This check has been put in place to ensure correct forward
> > > - * compatibility options when newer hardware variants come along.
> > > - */
> > > - switch (ver->hw_variant) {
> > > - case 0x0b: /* SfP */
> > > - case 0x0c: /* WsP */
> > > - case 0x11: /* JfP */
> > > - case 0x12: /* ThP */
> > > - case 0x13: /* HrP */
> > > - case 0x14: /* CcP */
> > > - break;
> > > - default:
> > > - bt_dev_err(hdev, "Unsupported Intel hardware variant (%u)",
> > > - ver->hw_variant);
> > > - return -EINVAL;
> > > - }
> > > -
> > > btintel_version_info(hdev, ver);
> > >
> > > /* The firmware variant determines if the device is in bootloader
> > > @@ -2741,16 +2728,8 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> > > if (ver->fw_variant == 0x23) {
> > > clear_bit(BTUSB_BOOTLOADER, &data->flags);
> > > btintel_check_bdaddr(hdev);
> > > - return 0;
> > > - }
> > > -
> > > - /* If the device is not in bootloader mode, then the only possible
> > > - * choice is to return an error and abort the device initialization.
> > > - */
> > > - if (ver->fw_variant != 0x06) {
> > > - bt_dev_err(hdev, "Unsupported Intel firmware variant (%u)",
> > > - ver->fw_variant);
> > > - return -ENODEV;
> > > + /* Proceed to download to check if the version matches */
> > > + goto download;
> > > }
> > >
> > > /* Read the secure boot parameters to identify the operating
> > > @@ -2778,6 +2757,7 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> > > set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
> > > }
> > >
> > > +download:
> > > /* With this Intel bootloader only the hardware variant and device
> > > * revision information are used to select the right firmware for SfP
> > > * and WsP.
> > > @@ -2801,7 +2781,13 @@ static int btusb_intel_download_firmware(struct hci_dev *hdev,
> > > */
> > > err = btusb_setup_intel_new_get_fw_name(ver, params, fwname,
> > > sizeof(fwname), "sfi");
> > > - if (!err) {
> > > + if (err < 0) {
> > > + if (err == -EALREADY) {
> > > + /* Firmware has already been loaded */
> > > + set_bit(BTUSB_FIRMWARE_LOADED, &data->flags);
> > > + goto done;
> > > + }
> > > +
> > > bt_dev_err(hdev, "Unsupported Intel firmware naming");
> > > return -EINVAL;
> > > }
>
>
