Re: [PATCH] Bluetooth: btrtl: Load FW v2 otherwise FW v1 for RTL8852C

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[CCing the linux-firmware maintainer, the network maintainers, and the
regressions lists]

On 04.08.23 07:54, max.chou@xxxxxxxxxxx wrote:
> From: Max Chou <max.chou@xxxxxxxxxxx>
> 
> In the commit of linux-firmware project, rtl8852cu_fw.bin is updated as
> FW v2 format[1]. Consider the case that if driver did not be updated fo> FW v2 supported[2], it can not use FW v2.
>
> By Canonical's suggestion, older driver should be able to load FW v1,

Well, that's not only Canonical suggestion, that is how things are
supposed to be handled in general. See
Documentation/driver-api/firmware/firmware-usage-guidelines.rst (alt:
https://docs.kernel.org/driver-api/firmware/firmware-usage-guidelines.html
) for details.

We had a similar situation in March already with a Wifi driver:
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/?id=416a66cae796939d7d096988c72c641656c81c5a

Makes me wonder: is there a problem here we should try harder to avoid?
Especially as the problem in this case would have been simple to predict
by reading the changelog of the linux-firmware change. To quote from
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/?id=55e7448533e7

```
Note: This firmware patch needs to be used with driver
"btrtl: Firmware format v2 support". Please update the
driver with this commit: 9a24ce5e29b15c4c6b0c89c04f9df6ce14addefa
```

> so rtl8852cu_fw.bin will be revert to the previous commit as FW v1 and
> add rtl8852cu_fw_v2.bin as FW v2. This item will be started on
> linux-firmware project.
> 
> In this commit, the driver prefers to load FW v2 if available. Fallback to
> FW v1 otherwise.
> 
> To do on linux-firmware project.
> rtl_bt/rtl8852cu_fw.bin: FW v1 (stay at ver. 0xD7B8_FABF)
> rtl_bt/rtl8852cu_fw_v2.bin: FW v2 (to be maintained)
> 
> [1]'9a24ce5e29b1 ("Bluetooth: btrtl: Firmware format v2 support")'
> [2]'55e7448533e7 ("rtl_bt: Update RTL8852C BT USB firmware
>     to 0x040D_7225")'

You seem to have mixed up [1] and [2] here, or did my brain go sideways
somewhere?

> Suggested-by: Juerg Haefliger <juerg.haefliger@xxxxxxxxxxxxx>
> Tested-by: Hilda Wu <hildawu@xxxxxxxxxxx>

All that sounds like this should also have these tags to to ensure this
makes it back too all trees that applied 9a24ce5e29b:

 Fixes: 9a24ce5e29b ("Bluetooth: btrtl: Firmware format v2 support")
 Cc: stable@xxxxxxxxxxxxxxx

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

> Signed-off-by: Max Chou <max.chou@xxxxxxxxxxx>
> ---
>  drivers/bluetooth/btrtl.c | 68 +++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index ddae6524106d..8bfa86dd12f7 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -104,7 +104,7 @@ static const struct id_table ic_id_table[] = {
>  	{ IC_INFO(RTL_ROM_LMP_8723A, 0xb, 0x6, HCI_USB),
>  	  .config_needed = false,
>  	  .has_rom_version = false,
> -	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
> +	  .fw_name = "rtl_bt/rtl8723a_fw",
>  	  .cfg_name = NULL,
>  	  .hw_info = "rtl8723au" },
>  
> @@ -112,7 +112,7 @@ static const struct id_table ic_id_table[] = {
>  	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb, 0x6, HCI_UART),
>  	  .config_needed = true,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8723bs_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8723bs_fw",
>  	  .cfg_name = "rtl_bt/rtl8723bs_config",
>  	  .hw_info  = "rtl8723bs" },
>  
> @@ -120,7 +120,7 @@ static const struct id_table ic_id_table[] = {
>  	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb, 0x6, HCI_USB),
>  	  .config_needed = false,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8723b_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8723b_fw",
>  	  .cfg_name = "rtl_bt/rtl8723b_config",
>  	  .hw_info  = "rtl8723bu" },
>  
> @@ -132,7 +132,7 @@ static const struct id_table ic_id_table[] = {
>  	  .hci_bus = HCI_UART,
>  	  .config_needed = true,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8723cs_cg_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8723cs_cg_fw",
>  	  .cfg_name = "rtl_bt/rtl8723cs_cg_config",
>  	  .hw_info  = "rtl8723cs-cg" },
>  
> @@ -144,7 +144,7 @@ static const struct id_table ic_id_table[] = {
>  	  .hci_bus = HCI_UART,
>  	  .config_needed = true,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8723cs_vf_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8723cs_vf_fw",
>  	  .cfg_name = "rtl_bt/rtl8723cs_vf_config",
>  	  .hw_info  = "rtl8723cs-vf" },
>  
> @@ -156,7 +156,7 @@ static const struct id_table ic_id_table[] = {
>  	  .hci_bus = HCI_UART,
>  	  .config_needed = true,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8723cs_xx_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8723cs_xx_fw",
>  	  .cfg_name = "rtl_bt/rtl8723cs_xx_config",
>  	  .hw_info  = "rtl8723cs" },
>  
> @@ -164,7 +164,7 @@ static const struct id_table ic_id_table[] = {
>  	{ IC_INFO(RTL_ROM_LMP_8723B, 0xd, 0x8, HCI_USB),
>  	  .config_needed = true,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8723d_fw",
>  	  .cfg_name = "rtl_bt/rtl8723d_config",
>  	  .hw_info  = "rtl8723du" },
>  
> @@ -172,7 +172,7 @@ static const struct id_table ic_id_table[] = {
>  	{ IC_INFO(RTL_ROM_LMP_8723B, 0xd, 0x8, HCI_UART),
>  	  .config_needed = true,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8723ds_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8723ds_fw",
>  	  .cfg_name = "rtl_bt/rtl8723ds_config",
>  	  .hw_info  = "rtl8723ds" },
>  
> @@ -180,7 +180,7 @@ static const struct id_table ic_id_table[] = {
>  	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa, 0x6, HCI_USB),
>  	  .config_needed = false,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8821a_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8821a_fw",
>  	  .cfg_name = "rtl_bt/rtl8821a_config",
>  	  .hw_info  = "rtl8821au" },
>  
> @@ -189,7 +189,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = false,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8821c_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8821c_fw",
>  	  .cfg_name = "rtl_bt/rtl8821c_config",
>  	  .hw_info  = "rtl8821cu" },
>  
> @@ -198,7 +198,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = true,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8821cs_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8821cs_fw",
>  	  .cfg_name = "rtl_bt/rtl8821cs_config",
>  	  .hw_info  = "rtl8821cs" },
>  
> @@ -206,7 +206,7 @@ static const struct id_table ic_id_table[] = {
>  	{ IC_INFO(RTL_ROM_LMP_8761A, 0xa, 0x6, HCI_USB),
>  	  .config_needed = false,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8761a_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8761a_fw",
>  	  .cfg_name = "rtl_bt/rtl8761a_config",
>  	  .hw_info  = "rtl8761au" },
>  
> @@ -215,7 +215,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = false,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8761b_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8761b_fw",
>  	  .cfg_name = "rtl_bt/rtl8761b_config",
>  	  .hw_info  = "rtl8761btv" },
>  
> @@ -223,7 +223,7 @@ static const struct id_table ic_id_table[] = {
>  	{ IC_INFO(RTL_ROM_LMP_8761A, 0xb, 0xa, HCI_USB),
>  	  .config_needed = false,
>  	  .has_rom_version = true,
> -	  .fw_name  = "rtl_bt/rtl8761bu_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8761bu_fw",
>  	  .cfg_name = "rtl_bt/rtl8761bu_config",
>  	  .hw_info  = "rtl8761bu" },
>  
> @@ -232,7 +232,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = true,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8822cs_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8822cs_fw",
>  	  .cfg_name = "rtl_bt/rtl8822cs_config",
>  	  .hw_info  = "rtl8822cs" },
>  
> @@ -241,7 +241,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = true,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8822cs_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8822cs_fw",
>  	  .cfg_name = "rtl_bt/rtl8822cs_config",
>  	  .hw_info  = "rtl8822cs" },
>  
> @@ -250,7 +250,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = false,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8822cu_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8822cu_fw",
>  	  .cfg_name = "rtl_bt/rtl8822cu_config",
>  	  .hw_info  = "rtl8822cu" },
>  
> @@ -259,7 +259,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = true,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8822b_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8822b_fw",
>  	  .cfg_name = "rtl_bt/rtl8822b_config",
>  	  .hw_info  = "rtl8822bu" },
>  
> @@ -268,7 +268,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = false,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8852au_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8852au_fw",
>  	  .cfg_name = "rtl_bt/rtl8852au_config",
>  	  .hw_info  = "rtl8852au" },
>  
> @@ -277,7 +277,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = true,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8852bs_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8852bs_fw",
>  	  .cfg_name = "rtl_bt/rtl8852bs_config",
>  	  .hw_info  = "rtl8852bs" },
>  
> @@ -286,7 +286,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = false,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8852bu_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8852bu_fw",
>  	  .cfg_name = "rtl_bt/rtl8852bu_config",
>  	  .hw_info  = "rtl8852bu" },
>  
> @@ -295,7 +295,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = false,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = true,
> -	  .fw_name  = "rtl_bt/rtl8852cu_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8852cu_fw",
>  	  .cfg_name = "rtl_bt/rtl8852cu_config",
>  	  .hw_info  = "rtl8852cu" },
>  
> @@ -304,7 +304,7 @@ static const struct id_table ic_id_table[] = {
>  	  .config_needed = false,
>  	  .has_rom_version = true,
>  	  .has_msft_ext = false,
> -	  .fw_name  = "rtl_bt/rtl8851bu_fw.bin",
> +	  .fw_name  = "rtl_bt/rtl8851bu_fw",
>  	  .cfg_name = "rtl_bt/rtl8851bu_config",
>  	  .hw_info  = "rtl8851bu" },
>  	};
> @@ -1045,10 +1045,12 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
>  	struct sk_buff *skb;
>  	struct hci_rp_read_local_version *resp;
>  	struct hci_command_hdr *cmd;
> +	char fw_name[40];
>  	char cfg_name[40];
>  	u16 hci_rev, lmp_subver;
>  	u8 hci_ver, lmp_ver, chip_type = 0;
>  	int ret;
> +	int fw_load_retry = 0;
>  	u8 reg_val[2];
>  
>  	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
> @@ -1154,9 +1156,26 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
>  			goto err_free;
>  	}
>  
> -	btrtl_dev->fw_len = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
> +fw_name_load:
> +	if (btrtl_dev->ic_info->fw_name) {
> +		if (lmp_subver == RTL_ROM_LMP_8852A && hci_rev == 0x000c &&
> +				  fw_load_retry == 0) {
> +			fw_load_retry = 1;
> +			snprintf(fw_name, sizeof(fw_name), "%s_v2.bin",
> +				 btrtl_dev->ic_info->fw_name);
> +		} else {
> +			fw_load_retry = 0;
> +			snprintf(fw_name, sizeof(fw_name), "%s.bin",
> +				 btrtl_dev->ic_info->fw_name);
> +		}
> +		btrtl_dev->fw_len = rtl_load_file(hdev, fw_name,
>  					  &btrtl_dev->fw_data);
> +	}
> +
>  	if (btrtl_dev->fw_len < 0) {
> +		if (fw_load_retry == 1)
> +			goto fw_name_load;
> +
>  		rtl_dev_err(hdev, "firmware file %s not found",
>  			    btrtl_dev->ic_info->fw_name);
>  		ret = btrtl_dev->fw_len;
> @@ -1491,4 +1510,5 @@ MODULE_FIRMWARE("rtl_bt/rtl8852bs_config.bin");
>  MODULE_FIRMWARE("rtl_bt/rtl8852bu_fw.bin");
>  MODULE_FIRMWARE("rtl_bt/rtl8852bu_config.bin");
>  MODULE_FIRMWARE("rtl_bt/rtl8852cu_fw.bin");
> +MODULE_FIRMWARE("rtl_bt/rtl8852cu_fw_v2.bin");
>  MODULE_FIRMWARE("rtl_bt/rtl8852cu_config.bin");



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux