linux-kernel - Re: [PATCH RFC] bluetooth: qca: generate nvm fw name from boardid for WCN6855

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8500baf7-59e4-4d8c-8485-3ef7aa106f16@quicinc.com>
Date: Thu, 14 Nov 2024 14:17:55 +0800
From: quic_zijuhu <quic_zijuhu@...cinc.com>
To: Jens Glathe <jens.glathe@...schoolsolutions.biz>,
        Marcel Holtmann
	<marcel@...tmann.org>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>
CC: <linux-bluetooth@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "Steev
 Klimaszewski" <steev@...i.org>,
        Bjorn Andersson <bjorande@...cinc.com>
Subject: Re: [PATCH RFC] bluetooth: qca: generate nvm fw name from boardid for
 WCN6855

On 10/4/2024 3:21 AM, Jens Glathe wrote:
> From: Steev Klimaszewski <steev@...i.org>
> 
> This is based on the 2066 work, which the 6855 is basically the same
> thing.
> 
> The already existing function qca_generate_hsp_nvm_name() appears to do
> the right steps to generate the hpnv file name. For WCN6855, the suffix
> seems to be the board id with prefix b, though.
> 
> Add specific masking for boardid to generate the full name.
> 
> boardid == 0 -> use 0 as parameter
> boardid <  0x100 -> add 0x0b00 to it, otherwise add 0xb000
> 
> This generates correct hpnv* file names for the files on the Windows
> partition that appear to work better with the hardware than the default
> .bin.
> Tested on Lenovo Thinkpad X13s, Microsoft WDK2023, and HP Omnibook X14.
> 
> The specific firmware is found on the Windows partition, and is supposed
> to work a little bit better than the default .bin.
> 
> Co-authored-by: Steev Klimaszewski <steev@...i.org>
> Signed-off-by: Jens Glathe <jens.glathe@...schoolsolutions.biz>
> Signed-off-by: Steev Klimaszewski <steev@...i.org>
> ---
> This is based on the 2066 work, which the 6855 is basically the same
> thing.
> 
> It generates the fw name to load from the board id, taking the file
> name format for the WCN6855 into account. The hpnv* firmware files 
> can be found on the Windows partition ofthe device. They usually 
> work better with the specific hardware. If the file is not found it 
> retries with the default name.
> 
> This has been tested on:
> 
> Lenovo Thinkpad X13s
> Microsoft Windows Dev Kit 2023 (Blackrock)
> HP Omnibook X14
> ---
>  drivers/bluetooth/btqca.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index dfbbac92242a8..ffc75680b322b 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -564,6 +564,21 @@ static int qca_download_firmware(struct hci_dev *hdev,
>  					   config->fwname, ret);
>  				return ret;
>  			}
> +			/* For WCN6855, if Windows firmware file isn't in place
> +			 * then use the default .bin file.
> +			 */
> +		} else if (soc_type == QCA_WCN6855) {
> +			bt_dev_dbg(hdev, "QCA Failed to request file: %s (%d)",
> +				   config->fwname, ret);
> +			snprintf(config->fwname, sizeof(config->fwname),
> +				 "qca/hpnv%02x.bin", rom_ver);
> +			bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
> +			ret = request_firmware(&fw, config->fwname, &hdev->dev);
> +			if (ret) {
> +				bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> +					   config->fwname, ret);
> +				return ret;
> +			}

NO

1)
qca_download_firmware() is  also called to download *PATCH* hpbtfw*, if
downloading PATCH failed, your logic will wrongly download *NVM* hpnv*
instead, right ?

2)
if board ID  is available, must use board id specific NVM, must not
fallback to the default NVM is not for QCA_WCN6855.

>  		} else {
>  			bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>  				   config->fwname, ret);
> @@ -773,6 +788,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  			 "qca/apbtfw%02x.tlv", rom_ver);
>  		break;
>  	case QCA_QCA2066:
> +	case QCA_WCN6855:
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/hpbtfw%02x.tlv", rom_ver);

>  		break;
> @@ -788,10 +804,6 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/msbtfw%02x.mbn", rom_ver);
>  		break;
> -	case QCA_WCN6855:
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> -		break;
>  	case QCA_WCN7850:
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/hmtbtfw%02x.tlv", rom_ver);
> @@ -810,7 +822,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	/* Give the controller some time to get ready to receive the NVM */
>  	msleep(10);
>  
> -	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 || soc_type == QCA_WCN6855)
>  		qca_read_fw_board_id(hdev, &boardid);

a switch since SOC types will become more and more
>  
>  	/* Download NVM configuration */
> @@ -848,8 +860,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  				 "qca/msnv%02x.bin", rom_ver);
>  			break;
>  		case QCA_WCN6855:
> -			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/hpnv%02x.bin", rom_ver);
> +			qca_generate_hsp_nvm_name(config.fwname,
> +				sizeof(config.fwname), ver, rom_ver,
> +				boardid == 0 ? boardid : (boardid < 0x0100 ?
> +				(boardid & 0x00ff)|0x0b00 : (boardid & 0x0fff)|0xb000));
>  			break;

why not to refer to existing qca_get_nvm_name_generic().

i will post a patch and cc you.

>  		case QCA_WCN7850:
>  			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
> 
> ---
> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> change-id: 20241003-bt-nvm-firmware-47e4d1b70a99
> 
> Best regards,