Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4186-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240426155801.25277-1-johan+linaro@kernel.org>
 <CAD=FV=V-pG9+5fLonNvydmjS=ziUFUHAyF8T7YTkEHiO405aSA@mail.gmail.com> <ZizKmtcUIYAMpvOQ@hovoldconsulting.com>
In-Reply-To: <ZizKmtcUIYAMpvOQ@hovoldconsulting.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 30 Apr 2024 10:21:37 -0400
Message-ID: <CABBYNZ+pPTTOSzb2HpStRo273EPsLYgwMFRPJTaJP9Xk038nwQ@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: qca: generalise device address check
To: Johan Hovold <johan@kernel.org>
Cc: Doug Anderson <dianders@chromium.org>, Johan Hovold <johan+linaro@kernel.org>, 
	Marcel Holtmann <marcel@holtmann.org>, linux-bluetooth@vger.kernel.org, 
	linux-kernel@vger.kernel.org, stable@vger.kernel.org, 
	Janaki Ramaiah Thota <quic_janathot@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Johan,

On Sat, Apr 27, 2024 at 5:51=E2=80=AFAM Johan Hovold <johan@kernel.org> wro=
te:
>
> On Fri, Apr 26, 2024 at 10:23:15AM -0700, Doug Anderson wrote:
> > On Fri, Apr 26, 2024 at 9:00=E2=80=AFAM Johan Hovold <johan+linaro@kern=
el.org> wrote:
> > >
> > > The default device address apparently comes from the NVM configuratio=
n
> > > file and can differ quite a bit.
> > >
> > > Store the default address when parsing the configuration file and use=
 it
> > > to determine whether the controller has been provisioned with an
> > > address.
> > >
> > > This makes sure that devices without a unique address start as
> > > unconfigured unless a valid address has been provided in the devicetr=
ee.
>
> > >  int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_vers=
ion *ver,
> > >                          enum qca_btsoc_type soc_type)
> > >  {
> > > @@ -351,6 +348,11 @@ static void qca_tlv_check_data(struct hci_dev *h=
dev,
> > >
> > >                         /* Update NVM tags as needed */
> > >                         switch (tag_id) {
> > > +                       case EDL_TAG_ID_BD_ADDR:
> > > +                               if (tag_len !=3D sizeof(bdaddr_t))
> > > +                                       break;
> > > +                               memcpy(&config->bdaddr, tlv_nvm->data=
, sizeof(bdaddr_t));
> > > +                               break;
> > >                         case EDL_TAG_ID_HCI:
> >
> > nit: blank line after "break" ?
>
> Possibly, the driver isn't really consistent here and only two case
> statements have such a newline after break.
>
> > Also note that on my firmware I never see this tag and thus your patch
> > breaks trogdor. Specifically I put a printout here and it never gets
> > hit.
>
> Thanks for the quick test. As the parser is modifying the configuration
> file I assumed it was correct and tested...
>
> > I printed all the tags/lengths:
> >
> > [   17.961087] DOUG: id 0xde02, len 0x0010
> > [   17.965081] DOUG: id 0x0000, len 0x0000
> > [   17.969050] DOUG: id 0x0000, len 0x0011
> > [   17.973025] DOUG: id 0x0000, len 0x0a00
> > [   17.976991] DOUG: id 0x0303, len 0x0303
> > [   17.981066] DOUG: id 0x0033, len 0x1001
> >
> > Probably EDL_TAG_ID_BD_ADDR should have been 0xde02, not just 2.
>
> No, the parser is apparently broken and fails to consider an extra
> four-byte header found in some NVM files and just happily parses and
> potentially modifies (sic!) random bytes.
>
> I've fixed the parser so that it works also on configuration files with
> the extra header (apnv??.bin, crnv??[u].bin) and can read out the
> default address for all NVM files in linux-firmware that have one
> (otherwise all-zeroes is printed below):
>
> bluetooth hci0: bd_addr =3D 39:80:10:00:00:20 (qca/apnv10.bin)
> bluetooth hci0: bd_addr =3D 39:80:12:74:08:00 (qca/apnv11.bin)
> bluetooth hci0: bd_addr =3D 39:90:21:64:07:00 (qca/crnv21.bin)
> bluetooth hci0: bd_addr =3D 39:98:00:00:5a:ad (qca/crnv32.bin)
> bluetooth hci0: bd_addr =3D 39:98:00:00:5a:ad (qca/crnv32u.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21.301)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21.302)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21.309)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21g.301)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21g.302)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21g.309)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/hpnv21g.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/htnv20.bin)
> bluetooth hci0: bd_addr =3D 64:90:00:00:5a:ad (qca/msnv11.b09)
> bluetooth hci0: bd_addr =3D 64:90:00:00:5a:ad (qca/msnv11.b0a)
> bluetooth hci0: bd_addr =3D 64:90:00:00:5a:ad (qca/msnv11.bin)
> bluetooth hci0: bd_addr =3D 61:47:aa:31:22:14 (qca/nvm_00130300.bin)
> bluetooth hci0: bd_addr =3D 61:47:aa:32:44:07 (qca/nvm_00130302.bin)
>
> bluetooth hci0: bd_addr =3D 00:00:00:00:00:00 (qca/nvm_00230302.bin)
>
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_00440302.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_00440302_eu.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_00440302_i2s_eu.bi=
n)
>
> bluetooth hci0: bd_addr =3D 00:00:00:00:00:00 (qca/nvm_usb_00000200.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:00:00 (qca/nvm_usb_00000201.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:00:00 (qca/nvm_usb_00000300.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:00:00 (qca/nvm_usb_00000302.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:00:00 (qca/nvm_usb_00000302_eu.bi=
n)
>
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0104.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0105.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0106.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0107.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0109.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0110.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130200.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010a.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010b.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130201_0303.=
bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130201.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_01=
0a.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_01=
0b.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_03=
03.bin)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf.bi=
n)
> bluetooth hci0: bd_addr =3D 00:00:00:00:5a:ad (qca/nvm_usb_00190200.bin)
>
> It looks like we're being lucky and the parser is at least not
> corrupting the configuration files with the extra header currently in
> linux-firmware, but if it ever interprets a random 0x0011 or 0x001b word
> as a tag it would.
>
> Fixing the parser means that we would start modifying the configuration
> also for files with the extra header. This involves configuring the baud
> rate and enabling a deep sleep feature.
>
> Presumably this is something that should be done also on Trogdor, but
> this would obviously have to be tested first. I guess we can keep
> skipping this step until it has been verified and just read out the
> address for now.
>
> > > @@ -624,6 +626,9 @@ static int qca_check_bdaddr(struct hci_dev *hdev)
> > >         if (bacmp(&hdev->public_addr, BDADDR_ANY))
> > >                 return 0;
> > >
> > > +       if (!bacmp(&config->bdaddr, BDADDR_ANY))
> > > +               return 0;
> >
> > The above test feels non-obvious enough to deserve a comment. Could
> > you add one? That would also help alleviate my confusion since I
> > _think_ your if test is unneeded and maybe wrong? Let's say that the
> > firmware didn't have a default address stored in it. It still seems
> > like we could try to read the address and then if the firmware gave
> > back BDADDR_ANY (0) we should set the `HCI_QUIRK_USE_BDADDR_PROPERTY`
> > property, right?
>
> You're right. I'll drop this check when revisiting this next week.

I assume you will spin another version then?

--=20
Luiz Augusto von Dentz