| 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: <CAA8EJppmTSovZKTPb+syrc0Vvfu8U=HoP18tW072OEZ5nYyOgg@mail.gmail.com>
Date: Fri, 6 Dec 2024 10:34:04 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: "Cheng Jiang (IOE)" <quic_chejiang@...cinc.com>
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
On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
<quic_chejiang@...cinc.com> wrote:
>
> 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.
Please read my question before answering it.
> >> + 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.
What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.
> >
> >> + 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.
What will ".b%02x" write in those two cases?
> >> + else
> >> + snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
> >> +}
> >> +
--
With best wishes
Dmitry
Powered by blists - more mailing lists