| Message ID | 20240426155801.25277-1-johan+linaro@kernel.org |
|---|---|
| State | Accepted |
| Commit | aa63c8b7b1c2a95b4569bc820aaad764a7df43c1 |
| Headers | show |
| Series | Bluetooth: qca: generalise device address check | expand |
Hi, On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <johan+linaro@kernel.org> 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@vger.kernel.org # 6.5 > Cc: Doug Anderson <dianders@chromium.org> > Cc: Janaki Ramaiah Thota <quic_janathot@quicinc.com> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > 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?
On Fri, Apr 26, 2024 at 10:23:15AM -0700, Doug Anderson wrote: > On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <johan+linaro@kernel.org> 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. > > 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" ? 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 = 39:80:10:00:00:20 (qca/apnv10.bin) bluetooth hci0: bd_addr = 39:80:12:74:08:00 (qca/apnv11.bin) bluetooth hci0: bd_addr = 39:90:21:64:07:00 (qca/crnv21.bin) bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32.bin) bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32u.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.301) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.302) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.309) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.301) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.302) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.309) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/htnv20.bin) bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b09) bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b0a) bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.bin) bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin) bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin) bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_00230302.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_eu.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_i2s_eu.bin) bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000200.bin) bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000201.bin) bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000300.bin) bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302.bin) bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302_eu.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0104.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0105.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0106.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0107.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0109.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0110.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010a.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010b.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_0303.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010a.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010b.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_0303.bin) bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf.bin) bluetooth hci0: bd_addr = 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. Johan
Hi Johan, Having a default BDA list from NVM BDA tag value will prevent developers from using the device if there is no user space app(In Fluoride) to set the BDA. Therefore, we are requesting to use default address check patch, so that developer can change the NVM BDA to make use of the device. List Of default Addresses: --------------------------------------------------------- | BDA | Chipset | --------------------------------------------------------- | 39 80 10 00 00 20 | WCN3988 with ROM Version 0x0200 | --------------------------------------------------------- | 39 80 12 74 08 00 | WCN3988 with ROM Version 0x0201 | --------------------------------------------------------- | 39 90 21 64 07 00 | WCN3990 | --------------------------------------------------------- | 39 98 00 00 5A AD | WCN3991 | --------------------------------------------------------- | 00 00 00 00 5A AD | QCA DEFAULT | --------------------------------------------------------- On 4/27/2024 3:21 PM, Johan Hovold wrote: > On Fri, Apr 26, 2024 at 10:23:15AM -0700, Doug Anderson wrote: >> On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <johan+linaro@kernel.org> 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. > >>> 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" ? > > 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 = 39:80:10:00:00:20 (qca/apnv10.bin) > bluetooth hci0: bd_addr = 39:80:12:74:08:00 (qca/apnv11.bin) > bluetooth hci0: bd_addr = 39:90:21:64:07:00 (qca/crnv21.bin) > bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32.bin) > bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32u.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.301) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.302) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.309) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.301) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.302) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.309) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/htnv20.bin) > bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b09) > bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b0a) > bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.bin) > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin) > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin) > > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_00230302.bin) > > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_eu.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_i2s_eu.bin) > > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000200.bin) > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000201.bin) > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000300.bin) > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302.bin) > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302_eu.bin) > > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0104.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0105.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0106.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0107.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0109.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0110.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010a.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010b.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_0303.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010a.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010b.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_0303.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf.bin) > bluetooth hci0: bd_addr = 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. > > Johan -Janaki Ram
Hi Janaki, Please avoid top and remember to trim unnecessary context when replying to the mailing lists. On Mon, Apr 29, 2024 at 03:34:32PM +0530, Janaki Ramaiah Thota wrote: > Having a default BDA list from NVM BDA tag value will prevent developers > from using the device if there is no user space app(In Fluoride) to set > the BDA. Therefore, we are requesting to use default address check patch, > so that developer can change the NVM BDA to make use of the device. But a developer on such an old platform that can patch and replace the NVM configuration file should also be able to just disable the check in the driver right (e.g. by commenting out the call to qca_check_bdaddr())? > List Of default Addresses: > --------------------------------------------------------- > | BDA | Chipset | > --------------------------------------------------------- > | 39 80 10 00 00 20 | WCN3988 with ROM Version 0x0200 | > --------------------------------------------------------- > | 39 80 12 74 08 00 | WCN3988 with ROM Version 0x0201 | > --------------------------------------------------------- > | 39 90 21 64 07 00 | WCN3990 | > --------------------------------------------------------- > | 39 98 00 00 5A AD | WCN3991 | > --------------------------------------------------------- > | 00 00 00 00 5A AD | QCA DEFAULT | > --------------------------------------------------------- What about WCN6750 and 64:90:00:00:5a:ad? And then there's currently also: > > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin) > > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin) Which controllers use these configurations? Johan
On Mon, Apr 29, 2024 at 04:02:44PM +0200, Johan Hovold wrote: > Please avoid top and remember to trim unnecessary context when replying > to the mailing lists. This was supposed to say: Please avoid top-posting and ... See also: https://people.kernel.org/tglx/notes-about-netiquette http://daringfireball.net/2007/07/on_top Johan
Hi, On Mon, Apr 29, 2024 at 10:02 AM Johan Hovold <johan@kernel.org> wrote: > > Hi Janaki, > > Please avoid top and remember to trim unnecessary context when replying > to the mailing lists. > > On Mon, Apr 29, 2024 at 03:34:32PM +0530, Janaki Ramaiah Thota wrote: > > > Having a default BDA list from NVM BDA tag value will prevent developers > > from using the device if there is no user space app(In Fluoride) to set > > the BDA. Therefore, we are requesting to use default address check patch, > > so that developer can change the NVM BDA to make use of the device. > > But a developer on such an old platform that can patch and replace the > NVM configuration file should also be able to just disable the check in > the driver right (e.g. by commenting out the call to > qca_check_bdaddr())? > > > List Of default Addresses: > > --------------------------------------------------------- > > | BDA | Chipset | > > --------------------------------------------------------- > > | 39 80 10 00 00 20 | WCN3988 with ROM Version 0x0200 | > > --------------------------------------------------------- > > | 39 80 12 74 08 00 | WCN3988 with ROM Version 0x0201 | > > --------------------------------------------------------- > > | 39 90 21 64 07 00 | WCN3990 | > > --------------------------------------------------------- > > | 39 98 00 00 5A AD | WCN3991 | > > --------------------------------------------------------- > > | 00 00 00 00 5A AD | QCA DEFAULT | > > --------------------------------------------------------- > > What about WCN6750 and 64:90:00:00:5a:ad? > > And then there's currently also: > > > > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin) > > > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin) > > Which controllers use these configurations? These are not unique addresses though, we can't just have addresses by chipset address mapping logic as that would cause address clashes over the air, e.g. if there are other devices with the same chipset in the vicinity.
Hi, On Mon, Apr 29, 2024 at 1:12 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi, > > On Mon, Apr 29, 2024 at 10:02 AM Johan Hovold <johan@kernel.org> wrote: > > > > Hi Janaki, > > > > Please avoid top and remember to trim unnecessary context when replying > > to the mailing lists. > > > > On Mon, Apr 29, 2024 at 03:34:32PM +0530, Janaki Ramaiah Thota wrote: > > > > > Having a default BDA list from NVM BDA tag value will prevent developers > > > from using the device if there is no user space app(In Fluoride) to set > > > the BDA. Therefore, we are requesting to use default address check patch, > > > so that developer can change the NVM BDA to make use of the device. > > > > But a developer on such an old platform that can patch and replace the > > NVM configuration file should also be able to just disable the check in > > the driver right (e.g. by commenting out the call to > > qca_check_bdaddr())? > > > > > List Of default Addresses: > > > --------------------------------------------------------- > > > | BDA | Chipset | > > > --------------------------------------------------------- > > > | 39 80 10 00 00 20 | WCN3988 with ROM Version 0x0200 | > > > --------------------------------------------------------- > > > | 39 80 12 74 08 00 | WCN3988 with ROM Version 0x0201 | > > > --------------------------------------------------------- > > > | 39 90 21 64 07 00 | WCN3990 | > > > --------------------------------------------------------- > > > | 39 98 00 00 5A AD | WCN3991 | > > > --------------------------------------------------------- > > > | 00 00 00 00 5A AD | QCA DEFAULT | > > > --------------------------------------------------------- > > > > What about WCN6750 and 64:90:00:00:5a:ad? > > > > And then there's currently also: > > > > > > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin) > > > > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin) > > > > Which controllers use these configurations? > > These are not unique addresses though, we can't just have addresses by > chipset address mapping logic as that would cause address clashes over > the air, e.g. if there are other devices with the same chipset in the > vicinity. I see where this is going now, the firmware actually contain these duplicated addresses which then are checked and cause HCI_QUIRK_USE_BDADDR_PROPERTY then the tries hci_dev_get_bd_addr_from_property which loads the local-bd-address property from the parente device (SOC?), btw that could also have an invalid/duplicated address. Anyway the fact that firmware loading itself is programming a potentially duplicated address already seems wrong enough to me, either it shall leave it as 00... or set a valid address otherwise we always risk missing yet another duplicate address being introduced and then used over the air causing all sorts of problems for users. So to be clear, QCA firmware shall never attempt to flash anything other than 00:00:00:00:00:00 if you don't have a valid and unique identity address, so we can get rid of this table altogether. ps: If the intention is to have these addresses for testing then these firmwares files shall probably be kept private, since as explained above the use of duplicated addresses will cause problems to users who have no idea they have to be changed. > > -- > Luiz Augusto von Dentz
On Mon, Apr 29, 2024 at 01:31:53PM -0400, Luiz Augusto von Dentz wrote: > On Mon, Apr 29, 2024 at 1:12 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > On Mon, Apr 29, 2024 at 10:02 AM Johan Hovold <johan@kernel.org> wrote: > > > On Mon, Apr 29, 2024 at 03:34:32PM +0530, Janaki Ramaiah Thota wrote: > > > > Having a default BDA list from NVM BDA tag value will prevent developers > > > > from using the device if there is no user space app(In Fluoride) to set > > > > the BDA. Therefore, we are requesting to use default address check patch, > > > > so that developer can change the NVM BDA to make use of the device. > > > > > > But a developer on such an old platform that can patch and replace the > > > NVM configuration file should also be able to just disable the check in > > > the driver right (e.g. by commenting out the call to > > > qca_check_bdaddr())? > > > > > > > List Of default Addresses: > > > > --------------------------------------------------------- > > > > | BDA | Chipset | > > > > --------------------------------------------------------- > > > > | 39 80 10 00 00 20 | WCN3988 with ROM Version 0x0200 | > > > > --------------------------------------------------------- > > > > | 39 80 12 74 08 00 | WCN3988 with ROM Version 0x0201 | > > > > --------------------------------------------------------- > > > > | 39 90 21 64 07 00 | WCN3990 | > > > > --------------------------------------------------------- > > > > | 39 98 00 00 5A AD | WCN3991 | > > > > --------------------------------------------------------- > > > > | 00 00 00 00 5A AD | QCA DEFAULT | > > > > --------------------------------------------------------- > > > > > > What about WCN6750 and 64:90:00:00:5a:ad? > > > > > > And then there's currently also: > > > > > > > > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin) > > > > > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin) > > > > > > Which controllers use these configurations? > > > > These are not unique addresses though, we can't just have addresses by > > chipset address mapping logic as that would cause address clashes over > > the air, e.g. if there are other devices with the same chipset in the > > vicinity. > > I see where this is going now, the firmware actually contain these > duplicated addresses which then are checked and cause > HCI_QUIRK_USE_BDADDR_PROPERTY then the tries > hci_dev_get_bd_addr_from_property which loads the local-bd-address > property from the parente device (SOC?), btw that could also have an > invalid/duplicated address. Right, the expectation is that vendors don't abuse this and leave the address in the devicetree as all-zero unless the boot firmware has access to a unique address. HCI_QUIRK_USE_BDADDR_PROPERTY effectively implies HCI_QUIRK_INVALID_BDADDR, that is, both quirks marks the controller address as invalid. The only difference is that the former also goes out and checks if there's an address in the devicetree that can be used instead. The 'local-bd-address' property is used on boards where the boot firmware has access to some storage for the address and updates the devicetree with the board-specific address before passing the DT to the kernel. As I've mentioned before, we should probably just drop HCI_QUIRK_USE_BDADDR_PROPERTY eventually and always look for an address in the devicetree when HCI_QUIRK_INVALID_BDADDR is set instead. We could take that one step further and always let the devicetree override the controller address as Doug suggested, but I'm not sure that's what we want to do generally. Either way, these are later questions. > Anyway the fact that firmware loading itself is programming a > potentially duplicated address already seems wrong enough to me, > either it shall leave it as 00... or set a valid address otherwise we > always risk missing yet another duplicate address being introduced and > then used over the air causing all sorts of problems for users. > > So to be clear, QCA firmware shall never attempt to flash anything > other than 00:00:00:00:00:00 if you don't have a valid and unique > identity address, so we can get rid of this table altogether. Nothing is being flashed, but when the controller has not been provisioned with an address, the address in the NVM configuration file is used. And we need to handle this in some way, as the configuration files are already out there (e.g. in linux-firmware) and are honoured by the QCA firmware. My patch reads out the default address from the configuration file before downloading it during setup() so that no matter what address is set this way, it will be treated as non-unique and invalid. This way we don't need to maintain any table in the kernel and we don't risk any regressions if the address is ever changed in a later firmware update. The only downside is that developers on old platforms that don't have any user space tools to set a valid address (e.g. btmgmt) cannot set an address by patching the firmware file. But I don't think we need to care about that. I assume that in most cases those developers all just use the default address, with the risk of collisions that that implies. We have a standard APIs for configuring the address, just use that. > ps: If the intention is to have these addresses for testing then these > firmwares files shall probably be kept private, since as explained > above the use of duplicated addresses will cause problems to users who > have no idea they have to be changed. Johan
Hi Johan and Luiz, On 4/30/2024 12:37 PM, Johan Hovold wrote: > On Mon, Apr 29, 2024 at 01:31:53PM -0400, Luiz Augusto von Dentz wrote: >> On Mon, Apr 29, 2024 at 1:12 PM Luiz Augusto von Dentz >> <luiz.dentz@gmail.com> wrote: >>> On Mon, Apr 29, 2024 at 10:02 AM Johan Hovold <johan@kernel.org> wrote: >>>> On Mon, Apr 29, 2024 at 03:34:32PM +0530, Janaki Ramaiah Thota wrote: > >>> >>> These are not unique addresses though, we can't just have addresses by >>> chipset address mapping logic as that would cause address clashes over >>> the air, e.g. if there are other devices with the same chipset in the >>> vicinity. >> >> I see where this is going now, the firmware actually contain these >> duplicated addresses which then are checked and cause >> HCI_QUIRK_USE_BDADDR_PROPERTY then the tries >> hci_dev_get_bd_addr_from_property which loads the local-bd-address >> property from the parente device (SOC?), btw that could also have an >> invalid/duplicated address. > > Right, the expectation is that vendors don't abuse this and leave the > address in the devicetree as all-zero unless the boot firmware has > access to a unique address. > > HCI_QUIRK_USE_BDADDR_PROPERTY effectively implies > HCI_QUIRK_INVALID_BDADDR, that is, both quirks marks the controller > address as invalid. The only difference is that the former also goes out > and checks if there's an address in the devicetree that can be used > instead. > > The 'local-bd-address' property is used on boards where the boot > firmware has access to some storage for the address and updates the > devicetree with the board-specific address before passing the DT to the > kernel. > > As I've mentioned before, we should probably just drop > HCI_QUIRK_USE_BDADDR_PROPERTY eventually and always look for an address > in the devicetree when HCI_QUIRK_INVALID_BDADDR is set instead. > > We could take that one step further and always let the devicetree > override the controller address as Doug suggested, but I'm not sure > that's what we want to do generally. > > Either way, these are later questions. > >> Anyway the fact that firmware loading itself is programming a >> potentially duplicated address already seems wrong enough to me, >> either it shall leave it as 00... or set a valid address otherwise we >> always risk missing yet another duplicate address being introduced and >> then used over the air causing all sorts of problems for users. >> >> So to be clear, QCA firmware shall never attempt to flash anything >> other than 00:00:00:00:00:00 if you don't have a valid and unique >> identity address, so we can get rid of this table altogether. > Yes agree with this point. BD address should be treated as invalid if it is 00:00:00:00:00:00. NVM Tag 2: bd address is default BD address (other than 0), should be configured as valid address and as its not unique address and it will be same for all devices so mark it is configured but still allow user-space to change the address. > Nothing is being flashed, but when the controller has not been > provisioned with an address, the address in the NVM configuration file > is used. > > And we need to handle this in some way, as the configuration files are > already out there (e.g. in linux-firmware) and are honoured by the QCA > firmware. > > My patch reads out the default address from the configuration file > before downloading it during setup() so that no matter what address is > set this way, it will be treated as non-unique and invalid. > > This way we don't need to maintain any table in the kernel and we don't > risk any regressions if the address is ever changed in a later firmware > update. > > The only downside is that developers on old platforms that don't have > any user space tools to set a valid address (e.g. btmgmt) cannot set > an address by patching the firmware file. > > But I don't think we need to care about that. I assume that in most > cases those developers all just use the default address, with the risk > of collisions that that implies. > > We have a standard APIs for configuring the address, just use that. > >> ps: If the intention is to have these addresses for testing then these >> firmwares files shall probably be kept private, since as explained >> above the use of duplicated addresses will cause problems to users who >> have no idea they have to be changed. > > Johan -Janaki Ram
On Tue, Apr 30, 2024 at 06:22:26PM +0530, Janaki Ramaiah Thota wrote: > On 4/30/2024 12:37 PM, Johan Hovold wrote: > > On Mon, Apr 29, 2024 at 01:31:53PM -0400, Luiz Augusto von Dentz wrote: > >> Anyway the fact that firmware loading itself is programming a > >> potentially duplicated address already seems wrong enough to me, > >> either it shall leave it as 00... or set a valid address otherwise we > >> always risk missing yet another duplicate address being introduced and > >> then used over the air causing all sorts of problems for users. > >> > >> So to be clear, QCA firmware shall never attempt to flash anything > >> other than 00:00:00:00:00:00 if you don't have a valid and unique > >> identity address, so we can get rid of this table altogether. > > > > Yes agree with this point. > BD address should be treated as invalid if it is 00:00:00:00:00:00. We all agree on that. > NVM Tag 2: bd address is default BD address (other than 0), should be > configured as valid address and as its not unique address and it will > be same for all devices so mark it is configured but still allow > user-space to change the address. But here we disagree. A non-unique address is not a valid one as it will cause collisions if you have more than one such controller. I understand that this may be convenient/good enough for developers in some cases, but this can hurt end users that do not realise why things break. And a developer can always configure an address manually or patch the driver as needed for internal use. Are there any other reasons that makes you want to keep the option to configure the device address through NVM files? I'm assuming you're not relying on patching NVM files to provision device-specific addresses after installation on target? Johan
Hi Johan, On Tue, Apr 30, 2024 at 9:07 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Apr 30, 2024 at 06:22:26PM +0530, Janaki Ramaiah Thota wrote: > > On 4/30/2024 12:37 PM, Johan Hovold wrote: > > > On Mon, Apr 29, 2024 at 01:31:53PM -0400, Luiz Augusto von Dentz wrote: > > > >> Anyway the fact that firmware loading itself is programming a > > >> potentially duplicated address already seems wrong enough to me, > > >> either it shall leave it as 00... or set a valid address otherwise we > > >> always risk missing yet another duplicate address being introduced and > > >> then used over the air causing all sorts of problems for users. > > >> > > >> So to be clear, QCA firmware shall never attempt to flash anything > > >> other than 00:00:00:00:00:00 if you don't have a valid and unique > > >> identity address, so we can get rid of this table altogether. > > > > > > > Yes agree with this point. > > BD address should be treated as invalid if it is 00:00:00:00:00:00. > > We all agree on that. > > > NVM Tag 2: bd address is default BD address (other than 0), should be > > configured as valid address and as its not unique address and it will > > be same for all devices so mark it is configured but still allow > > user-space to change the address. > > But here we disagree. A non-unique address is not a valid one as it will > cause collisions if you have more than one such controller. > > I understand that this may be convenient/good enough for developers in > some cases, but this can hurt end users that do not realise why things > break. > > And a developer can always configure an address manually or patch the > driver as needed for internal use. > > Are there any other reasons that makes you want to keep the option to > configure the device address through NVM files? I'm assuming you're not > relying on patching NVM files to provision device-specific addresses > after installation on target? Exactly, a duplicated address is not a valid public/identity address. Regarding them already been in use, we will need to have it fixed one way or the other, so it is better to change whatever it comer within the firmware file to 00:00:00:00:00:00 and have it setup a proper address after that rather than have a table that detect the use of duplicated addresses since the result would be the same since userspace stores pairing/devices based on adapter addresses they will be lost and the user will need to pair its peripherals again, so my recommendation is that this is done via firmware update rather than introducing a table containing duplicate addresses. That said it seems the patch in this thread actually reads the address with use of EDL_TAG_ID_BD_ADDR and then proceed to check if that is what the controller returns as address, while that is better than having a table I think there is still a risk that the duplicated address gets used on older kernels if that is not updated in the firmware directly, anyway perhaps we shall be doing both so we capture both cases where duplicated addresses are used or when BDADDR_ANY is.
Hi Johan, On Sat, Apr 27, 2024 at 5:51 AM Johan Hovold <johan@kernel.org> wrote: > > On Fri, Apr 26, 2024 at 10:23:15AM -0700, Doug Anderson wrote: > > On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <johan+linaro@kernel.org> 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. > > > > 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" ? > > 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 = 39:80:10:00:00:20 (qca/apnv10.bin) > bluetooth hci0: bd_addr = 39:80:12:74:08:00 (qca/apnv11.bin) > bluetooth hci0: bd_addr = 39:90:21:64:07:00 (qca/crnv21.bin) > bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32.bin) > bluetooth hci0: bd_addr = 39:98:00:00:5a:ad (qca/crnv32u.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.301) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.302) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.309) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.301) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.302) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.309) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/hpnv21g.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/htnv20.bin) > bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b09) > bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.b0a) > bluetooth hci0: bd_addr = 64:90:00:00:5a:ad (qca/msnv11.bin) > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin) > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin) > > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_00230302.bin) > > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_eu.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_00440302_i2s_eu.bin) > > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000200.bin) > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000201.bin) > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000300.bin) > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302.bin) > bluetooth hci0: bd_addr = 00:00:00:00:00:00 (qca/nvm_usb_00000302_eu.bin) > > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0104.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0105.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0106.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0107.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0109.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200_0110.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130200.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010a.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_010b.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_0303.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010a.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_010b.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf_0303.bin) > bluetooth hci0: bd_addr = 00:00:00:00:5a:ad (qca/nvm_usb_00130201_gf.bin) > bluetooth hci0: bd_addr = 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?
On Tue, Apr 30, 2024 at 10:04:05AM -0400, Luiz Augusto von Dentz wrote: > On Tue, Apr 30, 2024 at 9:07 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Apr 30, 2024 at 06:22:26PM +0530, Janaki Ramaiah Thota wrote: > > > NVM Tag 2: bd address is default BD address (other than 0), should be > > > configured as valid address and as its not unique address and it will > > > be same for all devices so mark it is configured but still allow > > > user-space to change the address. > > > > But here we disagree. A non-unique address is not a valid one as it will > > cause collisions if you have more than one such controller. > > > > I understand that this may be convenient/good enough for developers in > > some cases, but this can hurt end users that do not realise why things > > break. > > > > And a developer can always configure an address manually or patch the > > driver as needed for internal use. > > > > Are there any other reasons that makes you want to keep the option to > > configure the device address through NVM files? I'm assuming you're not > > relying on patching NVM files to provision device-specific addresses > > after installation on target? > > Exactly, a duplicated address is not a valid public/identity address. > > Regarding them already been in use, we will need to have it fixed one > way or the other, so it is better to change whatever it comer within > the firmware file to 00:00:00:00:00:00 and have it setup a proper > address after that rather than have a table that detect the use of > duplicated addresses since the result would be the same since > userspace stores pairing/devices based on adapter addresses they will > be lost and the user will need to pair its peripherals again, so my > recommendation is that this is done via firmware update rather than > introducing a table containing duplicate addresses. I'm not sure I fully understand you here. I agree that we should avoid the table if we can, but as you noted below this is what the patch in this thread does. And the firmware comes from Qualcomm which pushes it directly to linux-firmware so we can't control what they decide to put in these files. Perhaps the driver can clear the BD_ADDR tag instead of reading it back, but yes, the end result would be the same in case the firmware can handle that. May be better to just read out the address as this patch does to be sure. > That said it seems the patch in this thread actually reads the address > with use of EDL_TAG_ID_BD_ADDR and then proceed to check if that is > what the controller returns as address, while that is better than > having a table I think there is still a risk that the duplicated > address gets used on older kernels if that is not updated in the > firmware directly, anyway perhaps we shall be doing both so we capture > both cases where duplicated addresses are used or when BDADDR_ANY is. Not sure we need to care too much about older kernels here. The patches are currently marked for backport to 6.5 and that could serve as a cut-off point, but I guess there is nothing preventing us from backporting this to all stable trees if you prefer that. Johan
On Tue, Apr 30, 2024 at 10:21:37AM -0400, Luiz Augusto von Dentz wrote: > On Sat, Apr 27, 2024 at 5:51 AM Johan Hovold <johan@kernel.org> wrote: > > You're right. I'll drop this check when revisiting this next week. > > I assume you will spin another version then? Yes. I have prepared a v2 series. I was just waiting a bit to see where the discussion in this thread went. Johan
On 4/30/2024 6:37 PM, Johan Hovold wrote: > On Tue, Apr 30, 2024 at 06:22:26PM +0530, Janaki Ramaiah Thota wrote: >> On 4/30/2024 12:37 PM, Johan Hovold wrote: >>> On Mon, Apr 29, 2024 at 01:31:53PM -0400, Luiz Augusto von Dentz wrote: > >>>> Anyway the fact that firmware loading itself is programming a >>>> potentially duplicated address already seems wrong enough to me, >>>> either it shall leave it as 00... or set a valid address otherwise we >>>> always risk missing yet another duplicate address being introduced and >>>> then used over the air causing all sorts of problems for users. >>>> >>>> So to be clear, QCA firmware shall never attempt to flash anything >>>> other than 00:00:00:00:00:00 if you don't have a valid and unique >>>> identity address, so we can get rid of this table altogether. >>> >> >> Yes agree with this point. >> BD address should be treated as invalid if it is 00:00:00:00:00:00. > > We all agree on that. > >> NVM Tag 2: bd address is default BD address (other than 0), should be >> configured as valid address and as its not unique address and it will >> be same for all devices so mark it is configured but still allow >> user-space to change the address. > > But here we disagree. A non-unique address is not a valid one as it will > cause collisions if you have more than one such controller. > > I understand that this may be convenient/good enough for developers in > some cases, but this can hurt end users that do not realise why things > break. > > And a developer can always configure an address manually or patch the > driver as needed for internal use. > > Are there any other reasons that makes you want to keep the option to > configure the device address through NVM files? I'm assuming you're not > relying on patching NVM files to provision device-specific addresses > after installation on target? > We prefer unique address to be flashed on OTP (persistent) memory of BT-Chip, which is supported by almost all QC BT-chips. If someone is not able to do that/ does not prefer that, they still have an option to flash unique address in firmware binary (NVM)file. This does not require setting BD address from user space. Also until a developer flashes OTP/ keep unique BD-Address in NVM, he should be able to run most of the use cases from Device, that's why we want to make it as configured. In our opinion this provides best Out of box experience. > Johan -Janaki Ram
On Thu, May 02, 2024 at 12:35:19PM +0530, Janaki Ramaiah Thota wrote: > On 4/30/2024 6:37 PM, Johan Hovold wrote: > > But here we disagree. A non-unique address is not a valid one as it will > > cause collisions if you have more than one such controller. > > > > I understand that this may be convenient/good enough for developers in > > some cases, but this can hurt end users that do not realise why things > > break. > > > > And a developer can always configure an address manually or patch the > > driver as needed for internal use. > > > > Are there any other reasons that makes you want to keep the option to > > configure the device address through NVM files? I'm assuming you're not > > relying on patching NVM files to provision device-specific addresses > > after installation on target? > We prefer unique address to be flashed on OTP (persistent) memory of > BT-Chip, which is supported by almost all QC BT-chips. Yes, that is certainly the best option for everyone. > If someone is not able to do that/ does not prefer that, they still > have an option to flash unique address in firmware binary (NVM)file. > This does not require setting BD address from user space. > > Also until a developer flashes OTP/ keep unique BD-Address in NVM, > he should be able to run most of the use cases from Device, that's > why we want to make it as configured. Ok, but a developer can still do this since they can patch the driver to disable the check temporarily or, alternatively, just update the devicetree with a valid unique address. > In our opinion this provides best Out of box experience. You can also look into improving support in user space (e.g. bluez) for providing a valid unique address in a simple text-based configuration file. That would be useful for all Linux users and not require having access to Qualcomm specific tools to update the NVM configuration file (which could also be in a read-only file system, e.g. on Android). Johan
Hi Johan, On 5/2/2024 3:41 PM, Johan Hovold wrote: > On Thu, May 02, 2024 at 12:35:19PM +0530, Janaki Ramaiah Thota wrote: >> On 4/30/2024 6:37 PM, Johan Hovold wrote: > >>> But here we disagree. A non-unique address is not a valid one as it will >>> cause collisions if you have more than one such controller. >>> >>> I understand that this may be convenient/good enough for developers in >>> some cases, but this can hurt end users that do not realise why things >>> break. >>> >>> And a developer can always configure an address manually or patch the >>> driver as needed for internal use. >>> >>> Are there any other reasons that makes you want to keep the option to >>> configure the device address through NVM files? I'm assuming you're not >>> relying on patching NVM files to provision device-specific addresses >>> after installation on target? > >> We prefer unique address to be flashed on OTP (persistent) memory of >> BT-Chip, which is supported by almost all QC BT-chips. > > Yes, that is certainly the best option for everyone. > >> If someone is not able to do that/ does not prefer that, they still >> have an option to flash unique address in firmware binary (NVM)file. >> This does not require setting BD address from user space. >> >> Also until a developer flashes OTP/ keep unique BD-Address in NVM, >> he should be able to run most of the use cases from Device, that's >> why we want to make it as configured. > > Ok, but a developer can still do this since they can patch the driver to > disable the check temporarily or, alternatively, just update the > devicetree with a valid unique address. > >> In our opinion this provides best Out of box experience. > If a developer has to patch a code/update device-tree, that is not a "out of box" experience. By "out of box" we meant, things should work without much changes required. > You can also look into improving support in user space (e.g. bluez) for > providing a valid unique address in a simple text-based configuration > file. > We don't think putting a must-have dependency in user space is the right thing to do, especially when we own a code in kernel space. > That would be useful for all Linux users and not require having access > to Qualcomm specific tools to update the NVM configuration file (which > could also be in a read-only file system, e.g. on Android). > Having a non-unique valid address allows a developer to handle all scenarios where he/she is dealing with DUT + commercial device and in such case, default BD-Address from nvm file should also be okay. Only when 2/more similar devices are in the mix, they need unique address. In that case we are providing end developers with a NVM utility(part of Qcom build Not open source tool)to change this default BD-Address. > Johan -Janaki Ram
Hi Janaki, On Thu, May 2, 2024 at 1:03 PM Janaki Ramaiah Thota <quic_janathot@quicinc.com> wrote: > > Hi Johan, > > On 5/2/2024 3:41 PM, Johan Hovold wrote: > > On Thu, May 02, 2024 at 12:35:19PM +0530, Janaki Ramaiah Thota wrote: > >> On 4/30/2024 6:37 PM, Johan Hovold wrote: > > > >>> But here we disagree. A non-unique address is not a valid one as it will > >>> cause collisions if you have more than one such controller. > >>> > >>> I understand that this may be convenient/good enough for developers in > >>> some cases, but this can hurt end users that do not realise why things > >>> break. > >>> > >>> And a developer can always configure an address manually or patch the > >>> driver as needed for internal use. > >>> > >>> Are there any other reasons that makes you want to keep the option to > >>> configure the device address through NVM files? I'm assuming you're not > >>> relying on patching NVM files to provision device-specific addresses > >>> after installation on target? > > > >> We prefer unique address to be flashed on OTP (persistent) memory of > >> BT-Chip, which is supported by almost all QC BT-chips. > > > > Yes, that is certainly the best option for everyone. > > > >> If someone is not able to do that/ does not prefer that, they still > >> have an option to flash unique address in firmware binary (NVM)file. > >> This does not require setting BD address from user space. > >> > >> Also until a developer flashes OTP/ keep unique BD-Address in NVM, > >> he should be able to run most of the use cases from Device, that's > >> why we want to make it as configured. > > > > Ok, but a developer can still do this since they can patch the driver to > > disable the check temporarily or, alternatively, just update the > > devicetree with a valid unique address. > > > >> In our opinion this provides best Out of box experience. > > > > If a developer has to patch a code/update device-tree, that is not > a "out of box" experience. By "out of box" we meant, things should > work without much changes required. > > > You can also look into improving support in user space (e.g. bluez) for > > providing a valid unique address in a simple text-based configuration > > file. > > > > We don't think putting a must-have dependency in user space is the > right thing to do, especially when we own a code in kernel space. > > > That would be useful for all Linux users and not require having access > > to Qualcomm specific tools to update the NVM configuration file (which > > could also be in a read-only file system, e.g. on Android). > > > > Having a non-unique valid address allows a developer to handle all > scenarios where he/she is dealing with DUT + commercial device and > in such case, default BD-Address from nvm file should also be okay. > Only when 2/more similar devices are in the mix, they need unique > address. In that case we are providing end developers with a NVM > utility(part of Qcom build Not open source tool)to change this > default BD-Address. And we don't agree with doing that, that is why the controller shall be marked as unconfigured when a non-unique address is used and if you insist in doing that I will probably have to escalate that you guys are intentionally using addresses that can clash over the air. If the firmware is intended for developer, it shall be kept private, public firmware shall never use duplicate addresses, ever, and don't come back with arguments like that only when 2/more similar devices are in the mix but that would just stress even more the point that you are breaking stuff _on purpose_, which is pretty bad by itself, and then suggesting to use a non-open-source tool to fix the address is making things worse because end users can be affected by this, that really fills like you don't care if your hardware works on regular Linux distros and in that case I will probably move it to driver/staging. > > Johan > > -Janaki Ram
Hi Luiz, On 5/2/2024 11:02 PM, Luiz Augusto von Dentz wrote: > Hi Janaki, > > On Thu, May 2, 2024 at 1:03 PM Janaki Ramaiah Thota > <quic_janathot@quicinc.com> wrote: >> >> Hi Johan, >> >> On 5/2/2024 3:41 PM, Johan Hovold wrote: >>> On Thu, May 02, 2024 at 12:35:19PM +0530, Janaki Ramaiah Thota wrote: >>>> On 4/30/2024 6:37 PM, Johan Hovold wrote: >>> >>>>> But here we disagree. A non-unique address is not a valid one as it will >>>>> cause collisions if you have more than one such controller. >>>>> >>>>> I understand that this may be convenient/good enough for developers in >>>>> some cases, but this can hurt end users that do not realise why things >>>>> break. >>>>> >>>>> And a developer can always configure an address manually or patch the >>>>> driver as needed for internal use. >>>>> >>>>> Are there any other reasons that makes you want to keep the option to >>>>> configure the device address through NVM files? I'm assuming you're not >>>>> relying on patching NVM files to provision device-specific addresses >>>>> after installation on target? >>> >>>> We prefer unique address to be flashed on OTP (persistent) memory of >>>> BT-Chip, which is supported by almost all QC BT-chips. >>> >>> Yes, that is certainly the best option for everyone. >>> >>>> If someone is not able to do that/ does not prefer that, they still >>>> have an option to flash unique address in firmware binary (NVM)file. >>>> This does not require setting BD address from user space. >>>> >>>> Also until a developer flashes OTP/ keep unique BD-Address in NVM, >>>> he should be able to run most of the use cases from Device, that's >>>> why we want to make it as configured. >>> >>> Ok, but a developer can still do this since they can patch the driver to >>> disable the check temporarily or, alternatively, just update the >>> devicetree with a valid unique address. >>> >>>> In our opinion this provides best Out of box experience. >>> >> >> If a developer has to patch a code/update device-tree, that is not >> a "out of box" experience. By "out of box" we meant, things should >> work without much changes required. >> >>> You can also look into improving support in user space (e.g. bluez) for >>> providing a valid unique address in a simple text-based configuration >>> file. >>> >> >> We don't think putting a must-have dependency in user space is the >> right thing to do, especially when we own a code in kernel space. >> >>> That would be useful for all Linux users and not require having access >>> to Qualcomm specific tools to update the NVM configuration file (which >>> could also be in a read-only file system, e.g. on Android). >>> >> >> Having a non-unique valid address allows a developer to handle all >> scenarios where he/she is dealing with DUT + commercial device and >> in such case, default BD-Address from nvm file should also be okay. >> Only when 2/more similar devices are in the mix, they need unique >> address. In that case we are providing end developers with a NVM >> utility(part of Qcom build Not open source tool)to change this >> default BD-Address. > > And we don't agree with doing that, that is why the controller shall > be marked as unconfigured when a non-unique address is used and if you > insist in doing that I will probably have to escalate that you guys > are intentionally using addresses that can clash over the air. > > If the firmware is intended for developer, it shall be kept private, > public firmware shall never use duplicate addresses, ever, and don't > come back with arguments like that only when 2/more similar devices > are in the mix but that would just stress even more the point that you > are breaking stuff _on purpose_, which is pretty bad by itself, and > then suggesting to use a non-open-source tool to fix the address is > making things worse because end users can be affected by this, that > really fills like you don't care if your hardware works on regular > Linux distros and in that case I will probably move it to > driver/staging. > Our intention is not to break things, instead we wanted driver should be sufficient to set a BD-Address, without putting a necessary requirement on user space/Stack to configure BD-Address. Other solutions ( like Android ) were approaching this problem in this way. Now we also agree with your point that we should not leave any scope for having a non-unique BD-Address. Current bottleneck that we see with driver creating and managing unique BD-Address on its own is how to ensure persistence on reboot. If you are aware of any mechanism with which we can ensure persistence in kernel across reboot please let us know, otherwise we will write/reuse bluez-mgmt user space utility to solve this problem. >>> Johan >> >> -Janaki Ram > > > -- > Luiz Augusto von Dentz -Janaki Ram
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: /* HCI transport layer parameters * enabling software inband sleep @@ -615,7 +617,7 @@ 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) +static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *config) { struct hci_rp_read_bd_addr *bda; struct sk_buff *skb; @@ -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; + skb = __hci_cmd_sync(hdev, HCI_OP_READ_BD_ADDR, 0, NULL, HCI_INIT_TIMEOUT); if (IS_ERR(skb)) { @@ -639,10 +644,8 @@ static int qca_check_bdaddr(struct hci_dev *hdev) } bda = (struct hci_rp_read_bd_addr *)skb->data; - if (!bacmp(&bda->bdaddr, QCA_BDADDR_DEFAULT) || - !bacmp(&bda->bdaddr, QCA_BDADDR_WCN3991)) { + if (!bacmp(&bda->bdaddr, &config->bdaddr)) set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks); - } kfree_skb(skb); @@ -670,7 +673,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, enum qca_btsoc_type soc_type, struct qca_btsoc_version ver, const char *firmware_name) { - struct qca_fw_config config; + struct qca_fw_config config = {}; int err; u8 rom_ver = 0; u32 soc_ver; @@ -855,7 +858,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, break; } - err = qca_check_bdaddr(hdev); + err = qca_check_bdaddr(hdev, &config); if (err) return err; diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h index dc31984f71dc..49ad668d0d0b 100644 --- a/drivers/bluetooth/btqca.h +++ b/drivers/bluetooth/btqca.h @@ -29,6 +29,7 @@ #define EDL_PATCH_CONFIG_RES_EVT (0x00) #define QCA_DISABLE_LOGGING_SUB_OP (0x14) +#define EDL_TAG_ID_BD_ADDR 2 #define EDL_TAG_ID_HCI (17) #define EDL_TAG_ID_DEEP_SLEEP (27) @@ -94,6 +95,7 @@ struct qca_fw_config { uint8_t user_baud_rate; enum qca_tlv_dnld_mode dnld_mode; enum qca_tlv_dnld_mode dnld_type; + bdaddr_t bdaddr; }; struct edl_event_hdr {
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@vger.kernel.org # 6.5 Cc: Doug Anderson <dianders@chromium.org> Cc: Janaki Ramaiah Thota <quic_janathot@quicinc.com> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/bluetooth/btqca.c | 21 ++++++++++++--------- drivers/bluetooth/btqca.h | 2 ++ 2 files changed, 14 insertions(+), 9 deletions(-)