| 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
| ||
|
Message-ID: <541a5682-5b99-4793-84ee-a7c9168cb9a0@quicinc.com>
Date: Fri, 6 Dec 2024 11:05:12 +0800
From: "Cheng Jiang (IOE)" <quic_chejiang@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: 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>,
<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 v3 2/3] Bluetooth: qca: Expand firmware-name to load
specific nvm and rampatch
Hi Dmitry,
On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
>> The firmware-name property has been expanded to specify the names of NVM
>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
>> has different RF components and RAM sizes, necessitating new firmware
>> files. This change allows for the configuration of NVM and rampatch in
>> DT.
>>
>> Different connectivity boards may be attached to the same platform. For
>> example, QCA6698-based boards can support either a two-antenna or
>> three-antenna solution, both of which work on the sa8775p-ride platform.
>> Due to differences in connectivity boards and variations in RF
>> performance from different foundries, different NVM configurations are
>> used based on the board ID.
>
> Two separate commits, one for NVM, another one for RAM patch.
>
Ack.
>>
>> Therefore, in the firmware-name property, if the NVM file has an
>> extension, the NVM file will be used. Otherwise, the system will first
>> try the .bNN (board ID) file, and if that fails, it will fall back to
>> the .bin file.
>>
>> Possible configurations:
>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
>> firmware-name = "QCA6698/hpnv21.bin";
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@...cinc.com>
>> ---
>> drivers/bluetooth/btqca.c | 154 ++++++++++++++++++++++++++----------
>> drivers/bluetooth/btqca.h | 5 +-
>> drivers/bluetooth/hci_qca.c | 21 ++++-
>> 3 files changed, 134 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index dfbbac922..e8b89b8cc 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>> }
>> EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
>>
>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
>
> int is usually for errors, the code suggests bool return type.
>
Ack.
>> +{
>> + char fwname[64];
>> + const char *suffix;
>> +
>> + suffix = strrchr(path, '.');
>> +
>> + if (!suffix)
>> + return 0;
>> +
>> + strscpy(fwname, path, strlen(path));
>
> 64 bytes ought to be enough for anybody, correct?
>
Yes, in current driver, the max f/w path length is 64.
>> + fwname[suffix - path] = 0;
>
> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
>
Let me test this and fix in next patch.
>> +
>> + snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
>> +
>> + /* If nvm file is already the default one, return false to
>> + * skip the retry.
>> + */
>> + if (strcmp(fwname, path) == 0)
>> + return 0;
>> +
>> + snprintf(path, max_size, "%s", fwname);
>> + return 1;
>> +}
>> +
>> static int qca_tlv_check_data(struct hci_dev *hdev,
>> struct qca_fw_config *config,
>> u8 *fw_data, size_t fw_size,
>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
>> config->fwname, ret);
>> return ret;
>> }
>> + }
>> + /* For nvm, if desired nvm file is not present and it's not the
>> + * default nvm file(ends with .bin), try to load the default nvm.
>> + */
>> + else if (config->type == TLV_TYPE_NVM &&
>> + qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
>
> Please, don't rewrite the config. The file may be not present now, but
> it will reappear later (e.g. when rootfs gets mounted).
>
This tries to load a default NVM file if the board-specific NVM is not found.
It is called when request_firmware fails. It's safe to rewrite the config->fwname
here since we have already tried to load the board-specific NVM. The config
is a local variable in qca_uart_setup and will return after downloading the NVM.
>> + 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;
>> + }
>> } else {
>> bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>> config->fwname, ret);
>> @@ -730,15 +768,38 @@ static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
>> "qca/%snv%02x.b%02x", stem, rom_ver, bid);
>> }
>>
>> +static void qca_get_nvm_name_by_board(char *fwname, size_t max_size,
>> + const char *firmware_name, struct qca_btsoc_version ver,
>> + enum qca_btsoc_type soc_type, u16 bid)
>> +{
>> + const char *variant;
>> +
>> + /* Set the variant to empty by default */
>> + variant = "";
>> + /* hsp gf chip */
>> + if (soc_type == QCA_WCN6855) {
>> + if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
>> + variant = "g";
>
> Didn't you get the 'set but unused' here?
>
Yes, miss this part. Thank you!
>> + }
>> +
>> + if (bid == 0x0)
>
> 0x0 or 0xff?
board is set to 0 by default, 0x0 means read board id fails, then we should use
the default one.
>
>> + snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
>> + else if (bid & 0xff00)
>> + snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
>
> Doesn't ".b%02x" work in this case too?
>
No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
0x000a, then we need .b0a.
>> + else
>> + snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
>> +}
>> +
>> int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>> enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
>> - const char *firmware_name)
>> + const char *firmware_name, const char *rampatch_name)
>> {
>> struct qca_fw_config config = {};
>> int err;
>> u8 rom_ver = 0;
>> u32 soc_ver;
>> u16 boardid = 0;
>> + const char *suffix;
>>
>> bt_dev_dbg(hdev, "QCA setup on UART");
>>
>> @@ -761,44 +822,48 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>
>> /* Download rampatch file */
>> config.type = TLV_TYPE_PATCH;
>> - switch (soc_type) {
>> - case QCA_WCN3990:
>> - case QCA_WCN3991:
>> - case QCA_WCN3998:
>> - snprintf(config.fwname, sizeof(config.fwname),
>> - "qca/crbtfw%02x.tlv", rom_ver);
>> - break;
>> - case QCA_WCN3988:
>> - snprintf(config.fwname, sizeof(config.fwname),
>> - "qca/apbtfw%02x.tlv", rom_ver);
>> - break;
>> - case QCA_QCA2066:
>> - snprintf(config.fwname, sizeof(config.fwname),
>> - "qca/hpbtfw%02x.tlv", rom_ver);
>> - break;
>> - case QCA_QCA6390:
>> - snprintf(config.fwname, sizeof(config.fwname),
>> - "qca/htbtfw%02x.tlv", rom_ver);
>> - break;
>> - case QCA_WCN6750:
>> - /* Choose mbn file by default.If mbn file is not found
>> - * then choose tlv file
>> - */
>> - config.type = ELF_TYPE_PATCH;
>> - 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);
>> - break;
>> - default:
>> - snprintf(config.fwname, sizeof(config.fwname),
>> - "qca/rampatch_%08x.bin", soc_ver);
>> + if (rampatch_name) {
>> + snprintf(config.fwname, sizeof(config.fwname), "qca/%s", rampatch_name);
>> + } else {
>> + switch (soc_type) {
>> + case QCA_WCN3990:
>> + case QCA_WCN3991:
>> + case QCA_WCN3998:
>> + snprintf(config.fwname, sizeof(config.fwname),
>> + "qca/crbtfw%02x.tlv", rom_ver);
>> + break;
>> + case QCA_WCN3988:
>> + snprintf(config.fwname, sizeof(config.fwname),
>> + "qca/apbtfw%02x.tlv", rom_ver);
>> + break;
>> + case QCA_QCA2066:
>> + snprintf(config.fwname, sizeof(config.fwname),
>> + "qca/hpbtfw%02x.tlv", rom_ver);
>> + break;
>> + case QCA_QCA6390:
>> + snprintf(config.fwname, sizeof(config.fwname),
>> + "qca/htbtfw%02x.tlv", rom_ver);
>> + break;
>> + case QCA_WCN6750:
>> + /* Choose mbn file by default.If mbn file is not found
>> + * then choose tlv file
>> + */
>> + config.type = ELF_TYPE_PATCH;
>> + 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);
>> + break;
>> + default:
>> + snprintf(config.fwname, sizeof(config.fwname),
>> + "qca/rampatch_%08x.bin", soc_ver);
>> + }
>> }
>>
>> err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
>> @@ -816,8 +881,15 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>> /* Download NVM configuration */
>> config.type = TLV_TYPE_NVM;
>> if (firmware_name) {
>> - snprintf(config.fwname, sizeof(config.fwname),
>> - "qca/%s", firmware_name);
>> + /* The firmware name has suffix, use it directly */
>> + suffix = strrchr(firmware_name, '.');
>> + if (suffix && *(suffix + 1) != '\0' && *(suffix + 1) != '.') {
>> + snprintf(config.fwname, sizeof(config.fwname), "qca/%s", firmware_name);
>> + } else {
>> + qca_read_fw_board_id(hdev, &boardid);
>> + qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
>> + firmware_name, ver, soc_type, boardid);
>> + }
>> } else {
>> switch (soc_type) {
>> case QCA_WCN3990:
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index bb5207d7a..9d28c8800 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -161,7 +161,7 @@ enum qca_btsoc_type {
>> int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>> int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>> enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
>> - const char *firmware_name);
>> + const char *firmware_name, const char *rampatch_name);
>> int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>> enum qca_btsoc_type);
>> int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>> @@ -176,7 +176,8 @@ static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdad
>> static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>> enum qca_btsoc_type soc_type,
>> struct qca_btsoc_version ver,
>> - const char *firmware_name)
>> + const char *firmware_name,
>> + const char *rampatch_name)
>> {
>> return -EOPNOTSUPP;
>> }
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 37129e6cb..c566d0bf2 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -229,6 +229,7 @@ struct qca_serdev {
>> u32 oper_speed;
>> bool bdaddr_property_broken;
>> const char *firmware_name;
>> + const char *rampatch_name;
>> };
>>
>> static int qca_regulator_enable(struct qca_serdev *qcadev);
>> @@ -264,6 +265,17 @@ static const char *qca_get_firmware_name(struct hci_uart *hu)
>> }
>> }
>>
>> +static const char *qca_get_rampatch_name(struct hci_uart *hu)
>> +{
>> + if (hu->serdev) {
>> + struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
>> +
>> + return qsd->rampatch_name;
>> + } else {
>> + return NULL;
>> + }
>> +}
>> +
>> static void __serial_clock_on(struct tty_struct *tty)
>> {
>> /* TODO: Some chipset requires to enable UART clock on client
>> @@ -1855,6 +1867,7 @@ static int qca_setup(struct hci_uart *hu)
>> unsigned int retries = 0;
>> enum qca_btsoc_type soc_type = qca_soc_type(hu);
>> const char *firmware_name = qca_get_firmware_name(hu);
>> + const char *rampatch_name = qca_get_rampatch_name(hu);
>> int ret;
>> struct qca_btsoc_version ver;
>> struct qca_serdev *qcadev;
>> @@ -1963,7 +1976,7 @@ static int qca_setup(struct hci_uart *hu)
>>
>> /* Setup patch / NVM configurations */
>> ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
>> - firmware_name);
>> + firmware_name, rampatch_name);
>> if (!ret) {
>> clear_bit(QCA_IBS_DISABLED, &qca->flags);
>> qca_debugfs_init(hdev);
>> @@ -2309,8 +2322,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>> qcadev->serdev_hu.serdev = serdev;
>> data = device_get_match_data(&serdev->dev);
>> serdev_device_set_drvdata(serdev, qcadev);
>> - device_property_read_string(&serdev->dev, "firmware-name",
>> - &qcadev->firmware_name);
>> + of_property_read_string_index(serdev->dev.of_node, "firmware-name",
>> + 0, &qcadev->firmware_name);
>> + of_property_read_string_index(serdev->dev.of_node, "firmware-name",
>> + 1, &qcadev->rampatch_name);
>> device_property_read_u32(&serdev->dev, "max-speed",
>> &qcadev->oper_speed);
>> if (!qcadev->oper_speed)
>> --
>> 2.25.1
>>
>
Powered by blists - more mailing lists