linux-kernel - Re: [PATCH v5 2/4] Bluetooth: qca: Update firmware-name to support board specific nvm

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: <3a8fe16f-aca7-482e-b1cb-e6fa37717843@quicinc.com>
Date: Mon, 23 Dec 2024 10:47:13 +0800
From: "Cheng Jiang (IOE)" <quic_chejiang@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        Marcel Holtmann
	<marcel@...tmann.org>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        "Rob
 Herring" <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        "Conor
 Dooley" <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        "Konrad Dybcio" <konradybcio@...nel.org>,
        Balakrishna Godavarthi
	<quic_bgodavar@...cinc.com>,
        Rocky Liao <quic_rjliao@...cinc.com>
CC: <linux-bluetooth@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <quic_jiaymao@...cinc.com>, <quic_shuaz@...cinc.com>,
        <quic_zijuhu@...cinc.com>, <quic_mohamull@...cinc.com>
Subject: Re: [PATCH v5 2/4] Bluetooth: qca: Update firmware-name to support
 board specific nvm

Hi Konrad,

On 12/20/2024 9:46 PM, Konrad Dybcio wrote:
> On 13.12.2024 8:05 AM, Cheng Jiang (IOE) wrote:
> 
> [...]
> 
>>> /*
>>>  * If the board-specific file is missing, try loading the default
>>>  * one, unless that was attempted already
>>>  */
>>>
>>> But, even more importantly:
>>>
>>> a) do we want to load the "incorrect" file?
>> Normally, there is a default NVM file ending with .bin, which is suitable 
>> for most boards. However, some boards have different configurations that 
>> require a specific NVM. If a board-specific NVM is not found, a default 
>> NVM is preferred over not loading any NVM.
> 
> So, if one is specified, but not found, this should either be a loud error,
> or a very loud warning & fallback. Otherwise, the device may provide subpar
> user experience without the user getting a chance to know the reason.
> 
> I think failing is better here, as that sends a clearer message, and would
> only happen if the DT has a specific path (meaning the user put some
> intentions behind that choice)
> 
In the existing BT driver implementation, even if the rampatch/nvm are not found,
BT still works with ROM code only. No fails, just a warning in the dmesg. For this
new approach, we use the similar logic. 

The fallback to load a default nvm file is due to each board has a unique board
id, it's not necessary to upstream all the board-specific nvm since most of them 
may be the same, the default nvm file is suitable for them. But we can't set the 
default nvm file name in the DT, since the platform can attach different 
connectivity boards. This is a reasonable way to approach this. 

>>> b) why would we want to specify the .bin file if it's the default anyway?
>> The default NVM directory is the root of qca. The 'firmware-name' property 
>> can specify an NVM file in another directory. This can be either a default 
>> NVM like 'QCA6698/hpnv21.bin' or a board-specific NVM like 'QCA6698/hpnv21.b205'.
> 
> Do we expect QCA6698/hpnv21.bin and QCAabcd/hpnv21.bin to be compatible?
> 
No. It may be different. 
> [...]
> 
>>>> -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
>>>> -					    const char *stem, u8 rom_ver, u16 bid)
>>>> -{
>>>> -	if (bid == 0x0)
>>>> -		snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver);
>>>> -	else if (bid & 0xff00)
>>>> -		snprintf(cfg->fwname, sizeof(cfg->fwname),
>>>> -			 "qca/%snv%02x.b%x", stem, rom_ver, bid);
>>>> -	else
>>>> -		snprintf(cfg->fwname, sizeof(cfg->fwname),
>>>> -			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
>>>> +	if (soc_type == QCA_WCN6855 || soc_type == QCA_QCA2066) {
>>>> +		/* hsp gf chip */
>>>
>>> This is a good opportunity to explain what that means
>>>
>> Ack. This means the chip is produced by GlobalFoundries.
> 
> Please update the comment there
> 
ACK. 
> Konrad