Re: [PATCH v4 2/6] Bluetooth: btintel: Move operational checks after version check

["An, Tedd" <tedd.an@intel.com>]

On Wed, 2021-02-10 at 08:59 -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 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 {



>  }
>  
>  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;
>  	}