netdev - Re: [PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support

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: <ZE+6e7ZxJ2s9DHI1@corigine.com>
Date:   Mon, 1 May 2023 15:11:23 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Luca Weiss <luca.weiss@...rphone.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Balakrishna Godavarthi <bgodavar@...eaurora.org>,
        Rocky Liao <rjliao@...eaurora.org>,
        Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support

On Fri, Apr 21, 2023 at 04:11:39PM +0200, Luca Weiss wrote:
> Add support for the Bluetooth chip codenamed APACHE which is part of
> WCN3988.
> 
> The firmware for this chip has a slightly different naming scheme
> compared to most others. For ROM Version 0x0200 we need to use
> apbtfw10.tlv + apnv10.bin and for ROM version 0x201 apbtfw11.tlv +
> apnv11.bin
> 
> Signed-off-by: Luca Weiss <luca.weiss@...rphone.com>
> ---
>  drivers/bluetooth/btqca.c   | 13 +++++++++++--
>  drivers/bluetooth/btqca.h   | 12 ++++++++++--
>  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
>  3 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index fd0941fe8608..3ee1ef88a640 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -594,14 +594,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	/* Firmware files to download are based on ROM version.
>  	 * ROM version is derived from last two bytes of soc_ver.
>  	 */
> -	rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> +	if (soc_type == QCA_WCN3988)
> +		rom_ver = ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f);
> +	else
> +		rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);

Hi Luca,

perhaps it's just me. But I was wondering if this can be improved on a little.

* Move the common portion outside of the conditional
* And also, I think it's normal to use decimal for shift values.

e.g.
	unsigned shift;
	...

	shift = soc_type == QCA_WCN3988 ? 5 : 4;
	rom_ver = ((soc_ver & 0x00000f00) >> shift) | (soc_ver & 0x0000000f);

Using some helpers such as GENMASK and FIELD_PREP might also be nice.

>  
>  	if (soc_type == QCA_WCN6750)
>  		qca_send_patch_config_cmd(hdev);
>  
>  	/* Download rampatch file */
>  	config.type = TLV_TYPE_PATCH;
> -	if (qca_is_wcn399x(soc_type)) {
> +	if (soc_type == QCA_WCN3988) {
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/apbtfw%02x.tlv", rom_ver);
> +	} else if (qca_is_wcn399x(soc_type)) {
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/crbtfw%02x.tlv", rom_ver);
>  	} else if (soc_type == QCA_QCA6390) {
> @@ -636,6 +642,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	if (firmware_name)
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/%s", firmware_name);
> +	else if (soc_type == QCA_WCN3988)
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/apnv%02x.bin", rom_ver);
>  	else if (qca_is_wcn399x(soc_type)) {
>  		if (ver.soc_id == QCA_WCN3991_SOC_ID) {

Not strictly related to this patch, but while reviewing this I noticed that
ver.soc_id is __le32 but QCA_WCN3991_SOC_ID is in host byteorder.

Perhaps a cpu_to_le32() or le32_to_cpu() call is in order here?

>  			snprintf(config.fwname, sizeof(config.fwname),

...