linux-kernel - Re: [PATCH v2] Bluetooth: btusb: Add support for variant WCN6855 by using different 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: <YXA1+tEiCoY8yPRR@google.com>
Date:   Wed, 20 Oct 2021 08:30:02 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     tjiang@...eaurora.org
Cc:     marcel@...tmann.org, johan.hedberg@...il.com, luiz.dentz@...il.com,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, bgodavar@...eaurora.org,
        c-hbandi@...eaurora.org, hemantg@...eaurora.org,
        rjliao@...eaurora.org, zijuhu@...eaurora.org
Subject: Re: [PATCH v2] Bluetooth: btusb: Add support for variant WCN6855 by
 using different nvm

On Wed, Oct 20, 2021 at 12:00:52PM +0800, tjiang@...eaurora.org wrote:
> Thanks Matthias for the comments. please see my comments inline .
> 
> BTW: marcel , do you agree with Matthias comments ? if fine , I will align
> Matthias comments and make the final version.
> 
> regards.
> tim
> On 2021-10-20 03:29, Matthias Kaehlcke wrote:
> > On Tue, Oct 12, 2021 at 03:55:56PM +0800, tjiang@...eaurora.org wrote:
> > > 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: Tim Jiang <tjiang@...eaurora.org>
> > > ---
> > >  drivers/bluetooth/btusb.c | 56
> > > +++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 42 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index 75c83768c257..f352ff351b61 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -3190,6 +3190,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;
> > > @@ -3221,6 +3224,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,
> > > @@ -3375,6 +3379,43 @@ 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)
> > 
> > => const struct qca_version *ver
> > 
> > > +{
> > > +	u32 rom_version = le32_to_cpu(ver->rom_version);
> > > +	u16 flag = le16_to_cpu(ver->flag);
> > > +
> > > +	if (((flag >> 8) & 0xff) == QCA_FLAG_MULTI_NVM) {
> > > +		u16 board_id = le16_to_cpu(ver->board_id);
> > > +		u32 ram_version = le32_to_cpu(ver->ram_version);
> > > +		const char *variant;
> > > +
> > > +		switch (ram_version) {
> > > +		case WCN6855_2_0_RAM_VERSION_GF:
> > > +		case WCN6855_2_1_RAM_VERSION_GF:
> > > +			variant = "_gf";
> > > +			break;
> > > +		default:
> > > +			variant = "";
> > 
> > instead of the default branch you could assign a default to 'variant' at
> > declaration time, but it's fine either way.
> 
> [Tim] this code style is recommend by marcel.

Both are ok, if Marcel prefers the default branch let's keep it that way.

> > 
> > > +			break;
> > > +		}
> > > +
> > > +		/* if boardid equal 0, use default nvm without suffix */
> > 
> > delete the comment, it just states the obvious
> > 
> > > +		if (board_id == 0x0) {
> > 
> > nit: is there really any value in using a hex number here instead of a
> > plain decimal 0?
> 
> [Tim] this line is inherit from last change , if you think I should change
> 0x0 to 0 , I am fine.

Since this patch touches/moves this code it seems a good opportunity to clean
things up a bit. It's also true that there are quite a few instances of this
and comparisons with '0x00' in other parts of the kernel, so I guess it's
also fine to leave it as is.