Hi, On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <johan+linaro@xxxxxxxxxx> wrote: > > The default device address apparently comes from the NVM configuration > 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 devicetree. > > Fixes: 00567f70051a ("Bluetooth: qca: fix invalid device address check") > Cc: stable@xxxxxxxxxxxxxxx # 6.5 > Cc: Doug Anderson <dianders@xxxxxxxxxxxx> > Cc: Janaki Ramaiah Thota <quic_janathot@xxxxxxxxxxx> > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx> > --- > drivers/bluetooth/btqca.c | 21 ++++++++++++--------- > drivers/bluetooth/btqca.h | 2 ++ > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > index cfa71708397b..d7a6738e4691 100644 > --- a/drivers/bluetooth/btqca.c > +++ b/drivers/bluetooth/btqca.c > @@ -15,9 +15,6 @@ > > #define VERSION "0.1" > > -#define QCA_BDADDR_DEFAULT (&(bdaddr_t) {{ 0xad, 0x5a, 0x00, 0x00, 0x00, 0x00 }}) > -#define QCA_BDADDR_WCN3991 (&(bdaddr_t) {{ 0xad, 0x5a, 0x00, 0x00, 0x98, 0x39 }}) > - > int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver, > enum qca_btsoc_type soc_type) > { > @@ -351,6 +348,11 @@ static void qca_tlv_check_data(struct hci_dev *hdev, > > /* Update NVM tags as needed */ > switch (tag_id) { > + case EDL_TAG_ID_BD_ADDR: > + if (tag_len != sizeof(bdaddr_t)) > + break; > + memcpy(&config->bdaddr, tlv_nvm->data, sizeof(bdaddr_t)); > + break; > case EDL_TAG_ID_HCI: nit: blank line 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. 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. ...but then the size is wrong? When I print out the bytes in ID 0xde02 I see the address you're looking for 4 bytes in... [ 17.663602] DOUG: 0x00 [ 17.666132] DOUG: 0x00 [ 17.668638] DOUG: 0x00 [ 17.671237] DOUG: 0x00 [ 17.673689] DOUG: 0xad [ 17.676120] DOUG: 0x5a [ 17.678551] DOUG: 0x00 [ 17.680980] DOUG: 0x00 [ 17.683409] DOUG: 0x98 [ 17.685846] DOUG: 0x39 [ 17.688278] DOUG: 0x08 [ 17.690704] DOUG: 0x00 [ 17.693137] DOUG: 0x08 [ 17.693139] DOUG: 0x00 [ 17.693139] DOUG: 0x00 [ 17.693140] DOUG: 0x00 > @@ -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?