| Message ID | 20210209233404.2121960-2-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/6] Bluetooth: btintel: Check firmware version before download | expand |
Hi Luiz, On Tue, 2021-02-09 at 15:34 -0800, Luiz Augusto von Dentz wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > 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@intel.com> > --- > 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 153989bd8d5f..ccab05f67df9 100644 > --- a/drivers/bluetooth/btintel.c > +++ b/drivers/bluetooth/btintel.c > @@ -948,6 +948,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; > @@ -974,6 +985,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 e896c6702d60..113ff780232f 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; > } > > 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 > @@ -2693,35 +2709,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 > @@ -2740,16 +2727,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 > @@ -2777,6 +2756,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. > @@ -2800,7 +2780,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) { There is one more place to change the handling of return code for this function. err = btusb_setup_intel_new_get_fw_name(&ver, ¶ms, ddcname, sizeof(ddcname), "ddc"); if (!err) { bt_dev_err(hdev, "Unsupported Intel firmware naming"); } else { > + 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; > }
Hi Luiz, > 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@intel.com> > --- > 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 153989bd8d5f..ccab05f67df9 100644 > --- a/drivers/bluetooth/btintel.c > +++ b/drivers/bluetooth/btintel.c > @@ -948,6 +948,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; > @@ -974,6 +985,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 e896c6702d60..113ff780232f 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; > } > > 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 > @@ -2693,35 +2709,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; > - } > - why are you removing this? I put this in so that really only supported platforms are tried. We want to fail if you run on hardware that isn’t officially tested with this driver. Regards Marcel
Hi Marcel, On Wed, Feb 10, 2021 at 7:20 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Luiz, > > > 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@intel.com> > > --- > > 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 153989bd8d5f..ccab05f67df9 100644 > > --- a/drivers/bluetooth/btintel.c > > +++ b/drivers/bluetooth/btintel.c > > @@ -948,6 +948,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; > > @@ -974,6 +985,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 e896c6702d60..113ff780232f 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; > > } > > > > 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 > > @@ -2693,35 +2709,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; > > - } > > - > > why are you removing this? I put this in so that really only supported platforms are tried. We want to fail if you run on hardware that isn’t officially tested with this driver. It is not really removed it actually checked on btintel_version_info or btintel_version_info_tlv so we keep the checks in one place. > Regards > > Marcel >
diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index 153989bd8d5f..ccab05f67df9 100644 --- a/drivers/bluetooth/btintel.c +++ b/drivers/bluetooth/btintel.c @@ -948,6 +948,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; @@ -974,6 +985,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 e896c6702d60..113ff780232f 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; } 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 @@ -2693,35 +2709,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 @@ -2740,16 +2727,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 @@ -2777,6 +2756,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. @@ -2800,7 +2780,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; }