linux-kernel - Re: [PATCH v10] Bluetooth: btusb: Add support using different nvm for variant WCN6855 controller

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: <YU3Ym8Oa9yJVqN44@google.com>
Date:   Fri, 24 Sep 2021 06:54:35 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Marcel Holtmann <marcel@...tmann.org>
Cc:     Zijun Hu <zijuhu@...eaurora.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        linux-bluetooth <linux-bluetooth@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org, bgodavar@...eaurora.org,
        c-hbandi@...eaurora.org, hemantg@...eaurora.org,
        rjliao@...eaurora.org, tjiang@...eaurora.org
Subject: Re: [PATCH v10] Bluetooth: btusb: Add support using different nvm
 for variant WCN6855 controller

On Fri, Sep 24, 2021 at 12:10:41PM +0200, Marcel Holtmann wrote:
> Hi Zijun,
> 
> > the RF performance of wcn6855 soc chip from different foundries will be
> > difference, so we should use different nvm to configure them.
> > 
> > Signed-off-by: Zijun Hu <zijuhu@...eaurora.org>
> > ---
> > drivers/bluetooth/btusb.c | 50 +++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 37 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 928cbfa4c42d..6dc645698e30 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -3161,6 +3161,9 @@ static int btusb_set_bdaddr_wcn6855(struct hci_dev *hdev,
> > #define QCA_DFU_TIMEOUT		3000
> > #define QCA_FLAG_MULTI_NVM      0x80
> > 
> > +#define WCN6855_2_0_RAM_VERSION_GF 0x400c1200
> > +#define WCN6855_2_1_RAM_VERSION_GF 0x400c1211
> > +
> > struct qca_version {
> > 	__le32	rom_version;
> > 	__le32	patch_version;
> > @@ -3192,6 +3195,7 @@ static const struct qca_device_info qca_devices_table[] = {
> > 	{ 0x00000302, 28, 4, 16 }, /* Rome 3.2 */
> > 	{ 0x00130100, 40, 4, 16 }, /* WCN6855 1.0 */
> > 	{ 0x00130200, 40, 4, 16 }, /* WCN6855 2.0 */
> > +	{ 0x00130201, 40, 4, 16 }, /* WCN6855 2.1 */
> > };
> > 
> > static int btusb_qca_send_vendor_req(struct usb_device *udev, u8 request,
> > @@ -3346,6 +3350,31 @@ static int btusb_setup_qca_load_rampatch(struct hci_dev *hdev,
> > 	return err;
> > }
> > 
> > +static void btusb_generate_qca_nvm_name(char *fwname,
> > +					size_t max_size,
> > +					struct qca_version *ver,
> > +					char *variant)
> > +{
> 
> you are really not listening to review feedback. Use “const char *variant”.

Yeah, also as I mentioned earlier, this function has all the information
to determine the variant itself, there is no need to clutter
btusb_setup_qca_load_nvm() and the interface of this function with
variant stuff, it should be internal business of btusb_generate_qca_nvm_name().

> 
> > +	char *sep = (strlen(variant) == 0) ? "" : "_";
> 
> This is crazy.

It can also go away if the function determines the variant itself.