Message-ID: <CAD=FV=XoBwYmYGTdFNYMtJRnm6VAGf+-wq-ODVkxQqN3XeVHBw@mail.gmail.com>
Date: Tue, 23 Apr 2024 08:09:55 -0700
From: Doug Anderson <dianders@...omium.org>
To: Johan Hovold <johan@...nel.org>
Cc: Janaki Ramaiah Thota <quic_janathot@...cinc.com>, Johan Hovold <johan+linaro@...nel.org>, 
	Marcel Holtmann <marcel@...tmann.org>, Luiz Augusto von Dentz <luiz.dentz@...il.com>, 
	Matthias Kaehlcke <mka@...omium.org>, linux-bluetooth@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org, 
	Stephen Boyd <swboyd@...omium.org>
Subject: Re: [PATCH] Bluetooth: qca: fix invalid device address check

Hi,

On Tue, Apr 23, 2024 at 2:08 AM Johan Hovold <johan@...nel.org> wrote:
>
> Hi Doug and Janaki,
>
> 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@...nel.org> 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).
>
> 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.


> 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
[   14.616917] ath10k_snoc 18800000.wifi: kconfig debug 1 debugfs 1
tracing 0 dfs 0 testmode 1
[   14.625543] ath10k_snoc 18800000.wifi: firmware ver  api 5 features
wowlan,mfp,mgmt-tx-by-reference,non-bmi,single-chan-info-per-channel
crc32 3f19f7c1
[   14.682372] ath10k_snoc 18800000.wifi: htt-ver 3.87 wmi-op 4 htt-op
3 cal file max-sta 32 raw 0 hwcrypto 1
[   14.797210] ath: EEPROM regdomain: 0x406c
[   14.797223] ath: EEPROM indicates we should expect a direct regpair map
[   14.797231] ath: Country alpha2 being used: 00
[   14.797236] ath: Regpair used: 0x6c

..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
[   13.571838] Bluetooth: hci0: QCA Downloading qca/crbtfw32.tlv
[   14.096362] Bluetooth: hci0: QCA Downloading qca/crnv32.bin
[   14.770148] Bluetooth: hci0: QCA setup on UART is completed
[   14.805807] Bluetooth: hci0: AOSP extensions version v0.98
[   14.814793] Bluetooth: hci0: AOSP quality report is supported
[   15.011398] Bluetooth: hci0: unsupported parameter 28
[   15.016649] Bluetooth: hci0: unsupported parameter 28

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

..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?

-Doug

Powered by blists - more mailing lists