Bluetooth: qca: fix invalid device address check

Commit Message

Johan Hovold April 16, 2024, 9:15 a.m. UTC

Qualcomm Bluetooth controllers may not have been provisioned with a
valid device address and instead end up using the default address
00:00:00:00:5a:ad.

This was previously believed to be due to lack of persistent storage for
the address but it may also be due to integrators opting to not use the
on-chip OTP memory and instead store the address elsewhere (e.g. in
storage managed by secure world firmware).

According to Qualcomm, at least WCN6750, WCN6855 and WCN7850 have
on-chip OTP storage for the address.

As the device type alone cannot be used to determine when the address is
valid, instead read back the address during setup() and only set the
HCI_QUIRK_USE_BDADDR_PROPERTY flag when needed.

This specifically makes sure that controllers that have been provisioned
with an address do not start as unconfigured.

Reported-by: Janaki Ramaiah Thota <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]/
Fixes: 5971752de44c ("Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990")
Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts")
Fixes: 6945795bc81a ("Bluetooth: fix use-bdaddr-property quirk")
Cc: [e-mail]	# 6.5
Cc: Matthias Kaehlcke <[e-mail]>
Signed-off-by: Johan Hovold <[e-mail]>
---
 drivers/bluetooth/btqca.c   | 38 +++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_qca.c |  2 --
 2 files changed, 38 insertions(+), 2 deletions(-)


Matthias and Doug,

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?

Johan

Comments

[e-mail] April 16, 2024, 9:56 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=844980

---Test result---

Test Summary:
CheckPatch                    FAIL      1.22 seconds
GitLint                       FAIL      0.50 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      30.99 seconds
CheckAllWarning               PASS      33.15 seconds
CheckSparse                   PASS      39.27 seconds
CheckSmatch                   FAIL      35.66 seconds
BuildKernel32                 PASS      29.47 seconds
TestRunnerSetup               PASS      531.53 seconds
TestRunner_l2cap-tester       PASS      20.80 seconds
TestRunner_iso-tester         PASS      31.43 seconds
TestRunner_bnep-tester        PASS      4.94 seconds
TestRunner_mgmt-tester        PASS      112.59 seconds
TestRunner_rfcomm-tester      PASS      7.62 seconds
TestRunner_sco-tester         PASS      15.30 seconds
TestRunner_ioctl-tester       PASS      7.89 seconds
TestRunner_mesh-tester        PASS      6.04 seconds
TestRunner_smp-tester         PASS      7.04 seconds
TestRunner_userchan-tester    PASS      5.12 seconds
IncrementalBuild              PASS      28.25 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: qca: fix invalid device address check
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#85: 
Reported-by: Janaki Ramaiah Thota <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]/

total: 0 errors, 1 warnings, 64 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13631544.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
Bluetooth: qca: fix invalid device address check

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
23: B1 Line exceeds max length (81>80): "Link: https://lore.kernel.org/r/[e-mail]/"
24: B3 Line contains hard tab characters (\t): "Cc: [e-mail]	# 6.5"
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2


---
Regards,
Linux Bluetooth

[e-mail] April 16, 2024, 3:10 p.m. UTC | #2

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Tue, 16 Apr 2024 11:15:09 +0200 you wrote:
> Qualcomm Bluetooth controllers may not have been provisioned with a
> valid device address and instead end up using the default address
> 00:00:00:00:5a:ad.
> 
> This was previously believed to be due to lack of persistent storage for
> the address but it may also be due to integrators opting to not use the
> on-chip OTP memory and instead store the address elsewhere (e.g. in
> storage managed by secure world firmware).
> 
> [...]

Here is the summary with links:
  - Bluetooth: qca: fix invalid device address check
    https://git.kernel.org/bluetooth/bluetooth-next/c/00567f70051a

You are awesome, thank you!

Doug Anderson April 22, 2024, 5:50 p.m. UTC | #3

Hi,

On Tue, Apr 16, 2024 at 2:17 AM Johan Hovold <[e-mail]> wrote:
>
> Qualcomm Bluetooth controllers may not have been provisioned with a
> valid device address and instead end up using the default address
> 00:00:00:00:5a:ad.
>
> This was previously believed to be due to lack of persistent storage for
> the address but it may also be due to integrators opting to not use the
> on-chip OTP memory and instead store the address elsewhere (e.g. in
> storage managed by secure world firmware).
>
> According to Qualcomm, at least WCN6750, WCN6855 and WCN7850 have
> on-chip OTP storage for the address.
>
> As the device type alone cannot be used to determine when the address is
> valid, instead read back the address during setup() and only set the
> HCI_QUIRK_USE_BDADDR_PROPERTY flag when needed.
>
> This specifically makes sure that controllers that have been provisioned
> with an address do not start as unconfigured.
>
> Reported-by: Janaki Ramaiah Thota <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]/
> Fixes: 5971752de44c ("Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990")
> Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts")
> Fixes: 6945795bc81a ("Bluetooth: fix use-bdaddr-property quirk")
> Cc: [e-mail]      # 6.5
> Cc: Matthias Kaehlcke <[e-mail]>
> Signed-off-by: Johan Hovold <[e-mail]>
> ---
>  drivers/bluetooth/btqca.c   | 38 +++++++++++++++++++++++++++++++++++++
>  drivers/bluetooth/hci_qca.c |  2 --
>  2 files changed, 38 insertions(+), 2 deletions(-)
>
>
> Matthias and Doug,
>
> 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?


-Doug

Johan Hovold April 23, 2024, 9:08 a.m. UTC | #4

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 <[e-mail]> 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.

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?

Johan

Doug Anderson April 23, 2024, 3:09 p.m. UTC | #5

Hi,

On Tue, Apr 23, 2024 at 2:08 AM Johan Hovold <[e-mail]> 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 <[e-mail]> 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

Johan Hovold April 25, 2024, 8:40 a.m. UTC | #6

On Tue, Apr 23, 2024 at 08:09:55AM -0700, Doug Anderson wrote:
> On Tue, Apr 23, 2024 at 2:08 AM Johan Hovold <[e-mail]> 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 <[e-mail]> 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/[e-mail]/

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

Janaki Ramaiah Thota April 25, 2024, 3:01 p.m. UTC | #7

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 <[e-mail]> 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 <[e-mail]> 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/[e-mail]/
> 
> 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

Doug Anderson April 25, 2024, 3:22 p.m. UTC | #8

Hi,

On Thu, Apr 25, 2024 at 4:40 PM Johan Hovold <[e-mail]> wrote:
>
> > > 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.

Quick question. I haven't spent lots of time digging into the
Bluetooth subsystem, but it seems like if the device tree property is
there it should take precedence anyway, shouldn't it? In other words:
if we think there is built-in storage for the MAC address but we also
see a device tree property then we need to decide which of the two we
are going to use. Are there any instances where there's a bogus DT
property and we want the built-in storage to override it?

-Doug

Johan Hovold April 25, 2024, 3:58 p.m. UTC | #9

Hi Janaki,

On Thu, Apr 25, 2024 at 08:31:50PM +0530, Janaki Ramaiah Thota wrote:

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

Thanks a lot for these. I see now that the default Trogdor address Doug
reported (39:98:00:00:5a:ad) appears to comes from the fw too:

	$ od -x crnv32.bin | grep 5aad

	0000020 0000 0000 5aad 0000 3998 0008 0008 0000

which means that patch I sent this morning should be all that is needed
for those machines at least.

Can you please confirm that all the WCN39xx have OTP storage for an
address that an OEM can choose to use?

If that's not the case then we could simplify things by always marking
their addresses as invalid, but I assume that they all have address
storage.

Johan

Johan Hovold April 25, 2024, 4:13 p.m. UTC | #10

On Thu, Apr 25, 2024 at 11:22:50PM +0800, Doug Anderson wrote:

> Quick question. I haven't spent lots of time digging into the
> Bluetooth subsystem, but it seems like if the device tree property is
> there it should take precedence anyway, shouldn't it? In other words:
> if we think there is built-in storage for the MAC address but we also
> see a device tree property then we need to decide which of the two we
> are going to use. Are there any instances where there's a bogus DT
> property and we want the built-in storage to override it?

I guess we could decide to implement something like that, but note that
a devicetree may have an all-zero address defined by default which the
boot firmware may or may not fill in.

So we can't just use the presence of the address property as an
indication that the device has an address, but we could of course parse
it and see if it's non-zero first. (Actually, I think this bit about
checking for a non-zero address is already implemented.)

Note however that we still need to determine when the controller address
is invalid for the common case where there is no devicetree property and
user space needs to provide an address before the controller can be used.

Johan

Doug Anderson April 25, 2024, 4:19 p.m. UTC | #11

Hi,

On Thu, Apr 25, 2024 at 9:13 AM Johan Hovold <[e-mail]> wrote:
>
> On Thu, Apr 25, 2024 at 11:22:50PM +0800, Doug Anderson wrote:
>
> > Quick question. I haven't spent lots of time digging into the
> > Bluetooth subsystem, but it seems like if the device tree property is
> > there it should take precedence anyway, shouldn't it? In other words:
> > if we think there is built-in storage for the MAC address but we also
> > see a device tree property then we need to decide which of the two we
> > are going to use. Are there any instances where there's a bogus DT
> > property and we want the built-in storage to override it?
>
> I guess we could decide to implement something like that, but note that
> a devicetree may have an all-zero address defined by default which the
> boot firmware may or may not fill in.
>
> So we can't just use the presence of the address property as an
> indication that the device has an address, but we could of course parse
> it and see if it's non-zero first. (Actually, I think this bit about
> checking for a non-zero address is already implemented.)

This would make me feel safer. Given that you've now found that the
MAC address is in the firmware, I worry that someone will update the
firmware and change the default and we'll forget to update here.
_Hopefully_ someone would notice before pushing the firmware out to
the world, but it still seems like a more fragile solution than just
seeing that there's a perfectly valid BT address in the device tree
and using that.


> Note however that we still need to determine when the controller address
> is invalid for the common case where there is no devicetree property and
> user space needs to provide an address before the controller can be used.

Fair enough.

-Doug

Janaki Ramaiah Thota April 26, 2024, 6:23 a.m. UTC | #12

On 4/25/2024 9:28 PM, Johan Hovold wrote:
> Hi Janaki,
> 
> On Thu, Apr 25, 2024 at 08:31:50PM +0530, Janaki Ramaiah Thota wrote:
> 
>> 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			        |
>> ---------------------------------------------------------
> 
> Thanks a lot for these. I see now that the default Trogdor address Doug
> reported (39:98:00:00:5a:ad) appears to comes from the fw too:
> 
> 	$ od -x crnv32.bin | grep 5aad
> 
> 	0000020 0000 0000 5aad 0000 3998 0008 0008 0000
> 
> which means that patch I sent this morning should be all that is needed
> for those machines at least.
> 

Yes correct, it will work for Trogdor

> Can you please confirm that all the WCN39xx have OTP storage for an
> address that an OEM can choose to use?
> 

We are checking with internal FW team, will confirm on it.

> If that's not the case then we could simplify things by always marking
> their addresses as invalid, but I assume that they all have address
> storage.
> 
> Johan

-Janakiram

Janaki Ramaiah Thota April 26, 2024, 10:42 a.m. UTC | #13

Hi Johan,

Please note BDA values listed below are in the firmware (FW) data
order, but the actual BDA value should be in the reverse of that order.

On 4/26/2024 11:53 AM, Janaki Ramaiah Thota wrote:
> 
> 
> On 4/25/2024 9:28 PM, Johan Hovold wrote:
>> Hi Janaki,
>>
>> On Thu, Apr 25, 2024 at 08:31:50PM +0530, Janaki Ramaiah Thota wrote:
>>
>>> 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                    |
>>> ---------------------------------------------------------
>>
>> Thanks a lot for these. I see now that the default Trogdor address Doug
>> reported (39:98:00:00:5a:ad) appears to comes from the fw too:
>>
>>     $ od -x crnv32.bin | grep 5aad
>>
>>     0000020 0000 0000 5aad 0000 3998 0008 0008 0000
>>
>> which means that patch I sent this morning should be all that is needed
>> for those machines at least.
>>
> 
> Yes correct, it will work for Trogdor
> 
>> Can you please confirm that all the WCN39xx have OTP storage for an
>> address that an OEM can choose to use?
>>
> 
> We are checking with internal FW team, will confirm on it.
> 
>> If that's not the case then we could simplify things by always marking
>> their addresses as invalid, but I assume that they all have address
>> storage.
>>
>> Johan
> 
> -Janakiram

-Janaki Ram

Johan Hovold April 26, 2024, 12:45 p.m. UTC | #14

On Fri, Apr 26, 2024 at 04:12:07PM +0530, Janaki Ramaiah Thota wrote:

> Please note BDA values listed below are in the firmware (FW) data
> order, but the actual BDA value should be in the reverse of that order.

Thanks for clarifying. I realised this when I looked at the hexdump for
the Trogdor firmware.

> On 4/26/2024 11:53 AM, Janaki Ramaiah Thota wrote:
> > On 4/25/2024 9:28 PM, Johan Hovold wrote:
> >>> ---------------------------------------------------------
> >>> |   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                    |
> >>> ---------------------------------------------------------
> >>
> >> Thanks a lot for these. I see now that the default Trogdor address Doug
> >> reported (39:98:00:00:5a:ad) appears to comes from the fw too:
> >>
> >>     $ od -x crnv32.bin | grep 5aad
> >>
> >>     0000020 0000 0000 5aad 0000 3998 0008 0008 0000

It seems the most significant bytes here indeed do reflect the hardware
even if it's not entirely consistent:

	WCN3988		39:80

	WCN3990		39:90
	WCN3991		39:98

but I guess that doesn't help much unless also the remaining bytes on
WCN3988 and WCN3990 can be inferred somehow.

Johan

Johan Hovold April 26, 2024, 2:15 p.m. UTC | #15

On Fri, Apr 26, 2024 at 02:45:26PM +0200, Johan Hovold wrote:
> On Fri, Apr 26, 2024 at 04:12:07PM +0530, Janaki Ramaiah Thota wrote:
> > On 4/26/2024 11:53 AM, Janaki Ramaiah Thota wrote:
> > > On 4/25/2024 9:28 PM, Johan Hovold wrote:
> > >>> ---------------------------------------------------------
> > >>> |   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                    |
> > >>> ---------------------------------------------------------
> > >>
> > >> Thanks a lot for these. I see now that the default Trogdor address Doug
> > >> reported (39:98:00:00:5a:ad) appears to comes from the fw too:
> > >>
> > >>     $ od -x crnv32.bin | grep 5aad
> > >>
> > >>     0000020 0000 0000 5aad 0000 3998 0008 0008 0000

I took a closer look at the configuration file format and it seems we
can just fetch the default address from the file. The driver is already
parsing it so this should be straight forward.

I'll cook up a patch.

Johan

Patch

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 19cfc342fc7b..216826c31ee3 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -15,6 +15,8 @@ 
 
 #define VERSION "0.1"
 
+#define QCA_BDADDR_DEFAULT (&(bdaddr_t) {{ 0xad, 0x5a, 0x00, 0x00, 0x00, 0x00 }})
+
 int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
 			 enum qca_btsoc_type soc_type)
 {
@@ -612,6 +614,38 @@  int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 }
 EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
 
+static int qca_check_bdaddr(struct hci_dev *hdev)
+{
+	struct hci_rp_read_bd_addr *bda;
+	struct sk_buff *skb;
+	int err;
+
+	if (bacmp(&hdev->public_addr, BDADDR_ANY))
+		return 0;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_READ_BD_ADDR, 0, NULL,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hdev, "Failed to read device address (%d)", err);
+		return err;
+	}
+
+	if (skb->len != sizeof(*bda)) {
+		bt_dev_err(hdev, "Device address length mismatch");
+		kfree_skb(skb);
+		return -EIO;
+	}
+
+	bda = (struct hci_rp_read_bd_addr *)skb->data;
+	if (!bacmp(&bda->bdaddr, QCA_BDADDR_DEFAULT))
+		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
 static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
 		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
 {
@@ -818,6 +852,10 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		break;
 	}
 
+	err = qca_check_bdaddr(hdev);
+	if (err)
+		return err;
+
 	bt_dev_info(hdev, "QCA setup on UART is completed");
 
 	return 0;
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index ecbc52eaf101..92fa20f5ac7d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1905,8 +1905,6 @@  static int qca_setup(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
-		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
-
 		qcadev = serdev_device_get_drvdata(hu->serdev);
 		if (qcadev->bdaddr_property_broken)
 			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);