Re: [PATCH] Bluetooth: qca: fix invalid device address check

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Johan,

Apologies for the delay. As of now, we have observed the following
values in the upstream firmware files for default BD addresses.
We will confirm ASAP if there are any changes.

---------------------------------------------------------
|   BDA	             |      Chipset		        |
---------------------------------------------------------	
| 20 00 00 10 80 39  |	WCN3988 with ROM Version 0x0200	|
---------------------------------------------------------	
| 00 08 74 12 80 39  |  WCN3988 with ROM Version 0x0201	|
---------------------------------------------------------	
| 00 07 64 21 90 39  |  WCN3990			        |
---------------------------------------------------------

On 4/25/2024 2:10 PM, Johan Hovold wrote:
On Tue, Apr 23, 2024 at 08:09:55AM -0700, Doug Anderson wrote:
On Tue, Apr 23, 2024 at 2:08 AM Johan Hovold <johan@xxxxxxxxxx> wrote:
On Mon, Apr 22, 2024 at 10:50:33AM -0700, Doug Anderson wrote:
On Tue, Apr 16, 2024 at 2:17 AM Johan Hovold <johan+linaro@xxxxxxxxxx> wrote:

As Chromium is the only known user of the 'local-bd-address' property,
could you please confirm that your controllers use the 00:00:00:00:5a:ad
address by default so that the quirk continues to be set as intended?

I was at EOSS last week so didn't get a chance to test this, but I
just tested it now and I can confirm that it breaks trogdor. It
appears that trogdor devices seem to have a variant of your "default"
address. Instead of:

00:00:00:00:5a:ad

We seem to have a default of this:

39:98:00:00:5a:ad

...so almost the same, but not enough the same to make it work with
your code. I checked 3 different trogdor boards and they were all the
same, though I can't 100% commit to saying that every trogdor device
out there has that same default address...

Given that this breaks devices and also that it's already landed and
tagged for stable, what's the plan here? Do we revert? Do we add the
second address in and hope that there aren't trogdor devices out in
the wild that somehow have a different default?

This patch is currently queued for 6.10 so there should be time to get
this sorted.

My fallback plan was to add further (device-specific) default addresses
in case this turned out to be needed (e.g. this is what the Broadcom
driver does).

The offending commit was just sent on to the networking tree for 6.9 so
I went ahead and added the Trogdor default address to the address check
for now:

	https://lore.kernel.org/r/20240425075503.24357-1-johan+linaro@xxxxxxxxxx/

We can always amend this later if it turns out to be needed.

I assume all Trogdor boards use the same controller, WCN3991 IIUC, but
if you're worried about there being devices out there using a different
address we could possibly also use the new
"qcom,local-bd-address-broken" DT property as an indicator to set the
bdaddr quirk.

They all should use the same controller, but I'm just worried because
I don't personally know anything about how this address gets
programmed nor if there is any guarantee from Qualcomm that it'll be
consistent. There are a whole pile of boards in the field, so unless
we have some certainty that they all have the same address it feels
risky.

Hopefully Janaki and Qualcomm will provide some answers soon.

And otherwise we have another fall back in that we can use the
"qcom,local-bd-address-broken" property for Trogdor.

We have Qualcomm on CC here so perhaps Janaki, who should have access to
the documentation, can tell us what the default address on these older
controllers looks like?

Janaki, are there further default addresses out there that we need to
consider?

Perhaps "39:98" can even be inferred from the hardware id somehow (cf.
bcm4377_is_valid_bdaddr())?

Doug, could you please also post the QCA version info for Trogdor that's
printed on boot?

You want this:

[    9.610575] ath10k_snoc 18800000.wifi: qmi chip_id 0x320
chip_family 0x4001 board_id 0x67 soc_id 0x400c0000
[    9.620634] ath10k_snoc 18800000.wifi: qmi fw_version 0x322102f2
fw_build_timestamp 2021-08-02 05:27 fw_build_id
QC_IMAGE_VERSION_STRING=WLAN.HL.3.2.2.c10-00754-QCAHLSWMTPL-1
[   14.607163] ath10k_snoc 18800000.wifi: wcn3990 hw1.0 target
0x00000008 chip_id 0x00000000 sub 0000:0000
...or this...

[   12.899095] Bluetooth: hci0: setting up wcn399x
[   13.526154] Bluetooth: hci0: QCA Product ID   :0x0000000a
[   13.531805] Bluetooth: hci0: QCA SOC Version  :0x40010320
[   13.537384] Bluetooth: hci0: QCA ROM Version  :0x00000302
[   13.543002] Bluetooth: hci0: QCA Patch Version:0x00000de9
[   13.565775] Bluetooth: hci0: QCA controller version 0x03200302

Thanks, the Bluetooth driver output was what I was looking for but the
wifi output may also provide some insight.

Just as a random guess from looking at "8" in the logs, maybe the
extra 8 in 3998 is the "target" above?

Yeah, possibly, but it seems we won't be able to use the version info
without further details from Qualcomm.

...though that also makes me think that perhaps this chip doesn't
actually have space for a MAC address at all. Maybe they decided to
re-use the space to store the hardware ID and other information on all
of these devices?

All of these controllers apparently have storage for the hardware ids so
I'd be surprised if they didn't have room also for the address.

Looking at the backstory for this, it seems like Qualcomm intentionally
broke the bdaddr quirk so that controllers which had been provisioned
with a valid address would continue to work back when WCN3990 was the
only device that set the quirk. So presumably WCN3990 and later
controllers all have OTP storage for the address (even if I guess in
theory it could have been done just for, say, WCN3998 which was added
just after):

   5971752de44c ("Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990") (2019-02-19, matthias)
   e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts") (2019-04-18, qcom)
   523760b7ff88 ("Bluetooth: hci_qca: Added support for WCN3998") (2019-04-26, qcom)

Johan

Thanks,
Janaki Ram




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux