linux-kernel - Re: [PATCH v1] Bluetooth: support download nvm with different board id 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] [day] [month] [year] [list]

Message-ID: <X8bzNMT3o0GWxz8A@builder.lan>
Date:   Tue, 1 Dec 2020 19:51:48 -0600
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Zijun Hu <zijuhu@...eaurora.org>
Cc:     marcel@...tmann.org, johan.hedberg@...il.com,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, bgodavar@...eaurora.org,
        c-hbandi@...eaurora.org, hemantg@...eaurora.org, mka@...omium.org,
        rjliao@...eaurora.org, tjiang@...eaurora.org
Subject: Re: [PATCH v1] Bluetooth: support download nvm with different board
 id for wcn6855

On Mon 30 Nov 20:59 CST 2020, Zijun Hu wrote:

> From: Tim Jiang <tjiang@...eaurora.org>
> 
> we define many nvm files for wcn6855 btsoc and host driver
> should find the correct nvm file based on board ID and then
> download it.
> 
> Signed-off-by: Tim Jiang <tjiang@...eaurora.org>
> ---
>  drivers/bluetooth/btusb.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 3bbe8f43e7fa..66e19085e0fa 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3474,7 +3474,8 @@ struct qca_version {
>  	__le32	rom_version;
>  	__le32	patch_version;
>  	__le32	ram_version;
> -	__le32	ref_clock;
> +	__u16	board_id;

You should follow the scheme of describing the endianess of the fields.

> +	__u8	flag[2];

It seems more reasonable to make this a 16-bit flags. And either way
there are more than one of these, so plural "flags" seems appropriate.


PS. Can you confirm that no firmware actually used these 16 bits of the
"ref_clock"? Why wasn't the reserved bytes used to add the new
properties?

>  	__u8	reserved[4];
>  } __packed;
>  
> @@ -3657,8 +3658,13 @@ static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
>  	char fwname[64];
>  	int err;
>  
> -	snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
> -		 le32_to_cpu(ver->rom_version));
> +	if (ver->flag[1] == 0x80) { //check board id for wcn6855

Is this BIT(7) in the second byte of the flags, or is the second flag
0x80?

Based on the comment you should be able to provide a

#define QCA_VERSION_SECOND_FLAG_IS_WCN6855 0x80

to use instead of this magic number.

Regards,
Bjorn

> +		snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x_%04x.bin",
> +			le32_to_cpu(ver->rom_version), le16_to_cpu(ver->board_id));
> +	} else {
> +		snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
> +			le32_to_cpu(ver->rom_version));
> +	}
>  
>  	err = request_firmware(&fw, fwname, &hdev->dev);
>  	if (err) {
> @@ -3725,6 +3731,11 @@ static int btusb_setup_qca(struct hci_dev *hdev)
>  			return err;
>  	}
>  
> +	err = btusb_qca_send_vendor_req(udev, QCA_GET_TARGET_VERSION, &ver,
> +					sizeof(ver));
> +	if (err < 0)
> +		return err;
> +
>  	if (!(status & QCA_SYSCFG_UPDATED)) {
>  		err = btusb_setup_qca_load_nvm(hdev, &ver, info);
>  		if (err < 0)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>