| Message ID | 20230531090424.3187-3-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6ac517d8cf8b2e654280b4eca09c1d9edbd7d493 |
| Headers | show |
| Series | Bluetooth: fix bdaddr quirks | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
On 31.05.2023 11:04, Johan Hovold wrote: > Devices that lack persistent storage for the device address can indicate > this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller > to be marked as unconfigured until user space has set a valid address. > > The related HCI_QUIRK_USE_BDADDR_PROPERTY was later added to similarly > indicate that the device lacks a valid address but that one may be > specified in the devicetree. > > As is clear from commit 7a0e5b15ca45 ("Bluetooth: Add quirk for reading > BD_ADDR from fwnode property") that added and documented this quirk and > commits like de79a9df1692 ("Bluetooth: btqcomsmd: use > HCI_QUIRK_USE_BDADDR_PROPERTY"), the device address of controllers with > this flag should be treated as invalid until user space has had a chance > to configure the controller in case the devicetree property is missing. > > As it does not make sense to allow controllers with invalid addresses, > restore the original semantics, which also makes sure that the > implementation is consistent (e.g. get_missing_options() indicates that > the address must be set) and matches the documentation (including > comments in the code, such as, "In case any of them is set, the > controller has to start up as unconfigured."). > > Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts") > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> This patch has been recently merged to linux-next as commit 6ac517d8cf8b ("Bluetooth: fix use-bdaddr-property quirk"). Unfortunately it breaks bluetooth operation on my Raspberry Pi 3b+, 4b+ and Khadas VIM3 based test systems. Before this patch on Raspberry Pi 4b+: root@target:~# dmesg | grep hci0 [ 14.459292] Bluetooth: hci0: BCM: chip id 107 [ 14.464283] Bluetooth: hci0: BCM: features 0x2f [ 14.470632] Bluetooth: hci0: BCM4345C0 [ 14.474483] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0000 [ 14.487275] Bluetooth: hci0: BCM4345C0 'brcm/BCM4345C0.hcd' Patch [ 15.347542] Bluetooth: hci0: BCM: features 0x2f [ 15.354588] Bluetooth: hci0: BCM43455 37.4MHz Raspberry Pi 3+-0159 [ 15.361076] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0290 root@target:~# hcitool dev Devices: hci0 DC:A6:32:12:38:D1 root@target:~# root@target:~# hcitool scan Scanning ... 88:57:1D:AB:19:B2 Samsung Family Hub root@target:~# hcitool | head -n1 hcitool - HCI Tool ver 5.50 root@target:~# After this patch: root@target:~# dmesg | grep hci0 [ 13.979860] Bluetooth: hci0: BCM: chip id 107 [ 13.984969] Bluetooth: hci0: BCM: features 0x2f [ 13.991444] Bluetooth: hci0: BCM4345C0 [ 13.995300] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0000 [ 14.005131] Bluetooth: hci0: BCM4345C0 'brcm/BCM4345C0.hcd' Patch [ 14.839465] Bluetooth: hci0: BCM: features 0x2f [ 14.846047] Bluetooth: hci0: BCM43455 37.4MHz Raspberry Pi 3+-0159 [ 14.859859] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0290 root@target:~# hcitool dev Devices: root@target:~# hcitool scan Device is not available: No such device root@target:~# hcitool | head -n1 hcitool - HCI Tool ver 5.50 root@target:~# Reverting $subject on top of linux-next fixes this 'issue'. Let me know if you need more information about my test systems or to make some other tests. > ... Best regards
Hi Johan, On Thu, Jun 1, 2023 at 3:01 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 31.05.2023 11:04, Johan Hovold wrote: > > Devices that lack persistent storage for the device address can indicate > > this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller > > to be marked as unconfigured until user space has set a valid address. > > > > The related HCI_QUIRK_USE_BDADDR_PROPERTY was later added to similarly > > indicate that the device lacks a valid address but that one may be > > specified in the devicetree. > > > > As is clear from commit 7a0e5b15ca45 ("Bluetooth: Add quirk for reading > > BD_ADDR from fwnode property") that added and documented this quirk and > > commits like de79a9df1692 ("Bluetooth: btqcomsmd: use > > HCI_QUIRK_USE_BDADDR_PROPERTY"), the device address of controllers with > > this flag should be treated as invalid until user space has had a chance > > to configure the controller in case the devicetree property is missing. > > > > As it does not make sense to allow controllers with invalid addresses, > > restore the original semantics, which also makes sure that the > > implementation is consistent (e.g. get_missing_options() indicates that > > the address must be set) and matches the documentation (including > > comments in the code, such as, "In case any of them is set, the > > controller has to start up as unconfigured."). > > > > Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts") > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > This patch has been recently merged to linux-next as commit 6ac517d8cf8b > ("Bluetooth: fix use-bdaddr-property quirk"). Unfortunately it breaks > bluetooth operation on my Raspberry Pi 3b+, 4b+ and Khadas VIM3 based > test systems. > > Before this patch on Raspberry Pi 4b+: > > root@target:~# dmesg | grep hci0 > [ 14.459292] Bluetooth: hci0: BCM: chip id 107 > [ 14.464283] Bluetooth: hci0: BCM: features 0x2f > [ 14.470632] Bluetooth: hci0: BCM4345C0 > [ 14.474483] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0000 > [ 14.487275] Bluetooth: hci0: BCM4345C0 'brcm/BCM4345C0.hcd' Patch > [ 15.347542] Bluetooth: hci0: BCM: features 0x2f > [ 15.354588] Bluetooth: hci0: BCM43455 37.4MHz Raspberry Pi 3+-0159 > [ 15.361076] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0290 > root@target:~# hcitool dev > Devices: > hci0 DC:A6:32:12:38:D1 > root@target:~# > root@target:~# hcitool scan > Scanning ... > 88:57:1D:AB:19:B2 Samsung Family Hub > root@target:~# hcitool | head -n1 > hcitool - HCI Tool ver 5.50 > root@target:~# > > > After this patch: > > root@target:~# dmesg | grep hci0 > [ 13.979860] Bluetooth: hci0: BCM: chip id 107 > [ 13.984969] Bluetooth: hci0: BCM: features 0x2f > [ 13.991444] Bluetooth: hci0: BCM4345C0 > [ 13.995300] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0000 > [ 14.005131] Bluetooth: hci0: BCM4345C0 'brcm/BCM4345C0.hcd' Patch > [ 14.839465] Bluetooth: hci0: BCM: features 0x2f > [ 14.846047] Bluetooth: hci0: BCM43455 37.4MHz Raspberry Pi 3+-0159 > [ 14.859859] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0290 > root@target:~# hcitool dev > Devices: > root@target:~# hcitool scan > Device is not available: No such device > root@target:~# hcitool | head -n1 > hcitool - HCI Tool ver 5.50 > root@target:~# > > Reverting $subject on top of linux-next fixes this 'issue'. > > Let me know if you need more information about my test systems or to > make some other tests. Can you give it a look, looks like different manufacturers have different expectations, anyway we should probably figure out a way to get these controllers working otherwise we will need to revert.
On Fri, Jun 02, 2023 at 12:01:55AM +0200, Marek Szyprowski wrote: > On 31.05.2023 11:04, Johan Hovold wrote: > > Devices that lack persistent storage for the device address can indicate > > this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller > > to be marked as unconfigured until user space has set a valid address. > > > > The related HCI_QUIRK_USE_BDADDR_PROPERTY was later added to similarly > > indicate that the device lacks a valid address but that one may be > > specified in the devicetree. > > > > As is clear from commit 7a0e5b15ca45 ("Bluetooth: Add quirk for reading > > BD_ADDR from fwnode property") that added and documented this quirk and > > commits like de79a9df1692 ("Bluetooth: btqcomsmd: use > > HCI_QUIRK_USE_BDADDR_PROPERTY"), the device address of controllers with > > this flag should be treated as invalid until user space has had a chance > > to configure the controller in case the devicetree property is missing. > > > > As it does not make sense to allow controllers with invalid addresses, > > restore the original semantics, which also makes sure that the > > implementation is consistent (e.g. get_missing_options() indicates that > > the address must be set) and matches the documentation (including > > comments in the code, such as, "In case any of them is set, the > > controller has to start up as unconfigured."). > > > > Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts") > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > This patch has been recently merged to linux-next as commit 6ac517d8cf8b > ("Bluetooth: fix use-bdaddr-property quirk"). Unfortunately it breaks > bluetooth operation on my Raspberry Pi 3b+, 4b+ and Khadas VIM3 based > test systems. > > Before this patch on Raspberry Pi 4b+: > > root@target:~# dmesg | grep hci0 > [ 14.459292] Bluetooth: hci0: BCM: chip id 107 > [ 14.464283] Bluetooth: hci0: BCM: features 0x2f > [ 14.470632] Bluetooth: hci0: BCM4345C0 > [ 14.474483] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0000 > [ 14.487275] Bluetooth: hci0: BCM4345C0 'brcm/BCM4345C0.hcd' Patch > [ 15.347542] Bluetooth: hci0: BCM: features 0x2f > [ 15.354588] Bluetooth: hci0: BCM43455 37.4MHz Raspberry Pi 3+-0159 > [ 15.361076] Bluetooth: hci0: BCM4345C0 (003.001.025) build 0290 > root@target:~# hcitool dev > Devices: > hci0 DC:A6:32:12:38:D1 Thanks for the report and sorry about the breakage. Turns out that the Broadcom driver has so far been setting the HCI_QUIRK_USE_BDADDR_PROPERTY also for devices such as yours which already have a valid address. I've sent a fix for the Broadcom driver here: https://lore.kernel.org/lkml/20230602081912.4708-1-johan+linaro@kernel.org which should address this. Could you give it a try? Johan
Hi Johan, On Wed, 31 May 2023 at 14:35, Johan Hovold <johan+linaro@kernel.org> wrote: > > Devices that lack persistent storage for the device address can indicate > this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller > to be marked as unconfigured until user space has set a valid address. > > The related HCI_QUIRK_USE_BDADDR_PROPERTY was later added to similarly > indicate that the device lacks a valid address but that one may be > specified in the devicetree. > > As is clear from commit 7a0e5b15ca45 ("Bluetooth: Add quirk for reading > BD_ADDR from fwnode property") that added and documented this quirk and > commits like de79a9df1692 ("Bluetooth: btqcomsmd: use > HCI_QUIRK_USE_BDADDR_PROPERTY"), the device address of controllers with > this flag should be treated as invalid until user space has had a chance > to configure the controller in case the devicetree property is missing. > > As it does not make sense to allow controllers with invalid addresses, > restore the original semantics, which also makes sure that the > implementation is consistent (e.g. get_missing_options() indicates that > the address must be set) and matches the documentation (including > comments in the code, such as, "In case any of them is set, the > controller has to start up as unconfigured."). > This patch broke Bluetooth on Dragonboard 845c (SDM845) devboard. Reverting this patch fixes the BT breakage and I see the following messages in dmesg: Bluetooth: hci0: setting up wcn399x Bluetooth: hci0: QCA Product ID :0x0000000a Bluetooth: hci0: QCA SOC Version :0x40010214 Bluetooth: hci0: QCA ROM Version :0x00000201 Bluetooth: hci0: QCA Patch Version:0x00000001 Bluetooth: hci0: QCA controller version 0x02140201 Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv Bluetooth: hci0: QCA Downloading qca/crnv21.bin Bluetooth: hci0: QCA setup on UART is completed Regards, Amit Pundir > Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts") > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > net/bluetooth/hci_sync.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index c2a805ee55cc..ce03038b3460 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -4615,16 +4615,14 @@ static int hci_dev_setup_sync(struct hci_dev *hdev) > * BD_ADDR invalid before creating the HCI device or in > * its setup callback. > */ > - invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); > - > + invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) || > + test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks); > if (!ret) { > if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks) && > !bacmp(&hdev->public_addr, BDADDR_ANY)) > hci_dev_get_bd_addr_from_property(hdev); > > - if ((invalid_bdaddr || > - test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) && > - bacmp(&hdev->public_addr, BDADDR_ANY) && > + if (invalid_bdaddr && bacmp(&hdev->public_addr, BDADDR_ANY) && > hdev->set_bdaddr) { > ret = hdev->set_bdaddr(hdev, &hdev->public_addr); > if (!ret) > -- > 2.39.3 >
On Fri, Jul 07, 2023 at 03:11:11PM +0530, Amit Pundir wrote: > On Wed, 31 May 2023 at 14:35, Johan Hovold <johan+linaro@kernel.org> wrote: > > > > Devices that lack persistent storage for the device address can indicate > > this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller > > to be marked as unconfigured until user space has set a valid address. > > > > The related HCI_QUIRK_USE_BDADDR_PROPERTY was later added to similarly > > indicate that the device lacks a valid address but that one may be > > specified in the devicetree. > > > > As is clear from commit 7a0e5b15ca45 ("Bluetooth: Add quirk for reading > > BD_ADDR from fwnode property") that added and documented this quirk and > > commits like de79a9df1692 ("Bluetooth: btqcomsmd: use > > HCI_QUIRK_USE_BDADDR_PROPERTY"), the device address of controllers with > > this flag should be treated as invalid until user space has had a chance > > to configure the controller in case the devicetree property is missing. > > > > As it does not make sense to allow controllers with invalid addresses, > > restore the original semantics, which also makes sure that the > > implementation is consistent (e.g. get_missing_options() indicates that > > the address must be set) and matches the documentation (including > > comments in the code, such as, "In case any of them is set, the > > controller has to start up as unconfigured."). > This patch broke Bluetooth on Dragonboard 845c (SDM845) devboard. > Reverting this patch fixes the BT breakage and I see the following > messages in dmesg: > > Bluetooth: hci0: setting up wcn399x > Bluetooth: hci0: QCA Product ID :0x0000000a > Bluetooth: hci0: QCA SOC Version :0x40010214 > Bluetooth: hci0: QCA ROM Version :0x00000201 > Bluetooth: hci0: QCA Patch Version:0x00000001 > Bluetooth: hci0: QCA controller version 0x02140201 > Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv > Bluetooth: hci0: QCA Downloading qca/crnv21.bin > Bluetooth: hci0: QCA setup on UART is completed That's odd. You should still see the above messages also with this patch applied, but you may now need to provide a valid device address before being able to use device in case the bootloader has not provided one (e.g. using btmgmt). Are there any error messages in the log when running with this patch? Does btmgmt --index 0 public-addr <bdaddr> work? Johan
On Fri, 7 Jul 2023 at 16:37, Johan Hovold <johan@kernel.org> wrote: > > On Fri, Jul 07, 2023 at 03:11:11PM +0530, Amit Pundir wrote: > > > On Wed, 31 May 2023 at 14:35, Johan Hovold <johan+linaro@kernel.org> wrote: > > > > > > Devices that lack persistent storage for the device address can indicate > > > this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller > > > to be marked as unconfigured until user space has set a valid address. > > > > > > The related HCI_QUIRK_USE_BDADDR_PROPERTY was later added to similarly > > > indicate that the device lacks a valid address but that one may be > > > specified in the devicetree. > > > > > > As is clear from commit 7a0e5b15ca45 ("Bluetooth: Add quirk for reading > > > BD_ADDR from fwnode property") that added and documented this quirk and > > > commits like de79a9df1692 ("Bluetooth: btqcomsmd: use > > > HCI_QUIRK_USE_BDADDR_PROPERTY"), the device address of controllers with > > > this flag should be treated as invalid until user space has had a chance > > > to configure the controller in case the devicetree property is missing. > > > > > > As it does not make sense to allow controllers with invalid addresses, > > > restore the original semantics, which also makes sure that the > > > implementation is consistent (e.g. get_missing_options() indicates that > > > the address must be set) and matches the documentation (including > > > comments in the code, such as, "In case any of them is set, the > > > controller has to start up as unconfigured."). > > > This patch broke Bluetooth on Dragonboard 845c (SDM845) devboard. > > Reverting this patch fixes the BT breakage and I see the following > > messages in dmesg: > > > > Bluetooth: hci0: setting up wcn399x > > Bluetooth: hci0: QCA Product ID :0x0000000a > > Bluetooth: hci0: QCA SOC Version :0x40010214 > > Bluetooth: hci0: QCA ROM Version :0x00000201 > > Bluetooth: hci0: QCA Patch Version:0x00000001 > > Bluetooth: hci0: QCA controller version 0x02140201 > > Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv > > Bluetooth: hci0: QCA Downloading qca/crnv21.bin > > Bluetooth: hci0: QCA setup on UART is completed > > That's odd. You should still see the above messages also with this patch > applied, but you may now need to provide a valid device address before > being able to use device in case the bootloader has not provided one > (e.g. using btmgmt). Sorry for the confusion, I missed the part where I do see these messages when the kernel module is loaded but the direct firmware loading fails. Bluetooth: hci0: setting up wcn399x Bluetooth: hci0: QCA Product ID :0x0000000a Bluetooth: hci0: QCA SOC Version :0x40010214 Bluetooth: hci0: QCA ROM Version :0x00000201 Bluetooth: hci0: QCA Patch Version:0x00000001 Bluetooth: hci0: QCA controller version 0x02140201 Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv bluetooth hci0: Direct firmware load for qca/crbtfw21.tlv failed with error -2 Bluetooth: hci0: QCA Failed to request file: qca/crbtfw21.tlv (-2) Bluetooth: hci0: QCA Failed to download patch (-2) Bluetooth: hci0: QCA preshutdown_cmd failed (-56) This happens in all the cases (working and non-working BT) because filesystem is not mounted by that time. I'm running AOSP and all the kernel modules get loaded from a ramdisk. But in the working case, the firmware loading kicks in again later in the boot process and BT gets initiazed.. With this patch, after the first attempt to load the firmware fails, the firmware loading doesn't kick-in again. Also even if I keep the firmware in ramdisk then the direct firmware loading from ramdisk happens but BT still doesn't work https://bugs.linaro.org/attachment.cgi?id=1148. > > Are there any error messages in the log when running with this patch? I don't see any relevant error message in dmesg. I'll check if I can find a command line BT debug tool which I can use on AOSP for debugging. There used to be a few hci command line tools, when I looked into it a few years ago. Not sure if they are still around and useful. Regards, Amit Pundir > > Does > > btmgmt --index 0 public-addr <bdaddr> > > work? > > Johan
[TLDR: I'm adding this report to the list of tracked Linux kernel regressions; the text you find below is based on a few templates paragraphs you might have encountered already in similar form. See link in footer if these mails annoy you.] On 07.07.23 11:41, Amit Pundir wrote: > Hi Johan, > > On Wed, 31 May 2023 at 14:35, Johan Hovold <johan+linaro@kernel.org> wrote: >> >> Devices that lack persistent storage for the device address can indicate >> this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller >> to be marked as unconfigured until user space has set a valid address. >> >> The related HCI_QUIRK_USE_BDADDR_PROPERTY was later added to similarly >> indicate that the device lacks a valid address but that one may be >> specified in the devicetree. >> >> As is clear from commit 7a0e5b15ca45 ("Bluetooth: Add quirk for reading >> BD_ADDR from fwnode property") that added and documented this quirk and >> commits like de79a9df1692 ("Bluetooth: btqcomsmd: use >> HCI_QUIRK_USE_BDADDR_PROPERTY"), the device address of controllers with >> this flag should be treated as invalid until user space has had a chance >> to configure the controller in case the devicetree property is missing. >> >> As it does not make sense to allow controllers with invalid addresses, >> restore the original semantics, which also makes sure that the >> implementation is consistent (e.g. get_missing_options() indicates that >> the address must be set) and matches the documentation (including >> comments in the code, such as, "In case any of them is set, the >> controller has to start up as unconfigured."). >> > > This patch broke Bluetooth on Dragonboard 845c (SDM845) devboard. > Reverting this patch fixes the BT breakage and I see the following > messages in dmesg: > > Bluetooth: hci0: setting up wcn399x > Bluetooth: hci0: QCA Product ID :0x0000000a > Bluetooth: hci0: QCA SOC Version :0x40010214 > Bluetooth: hci0: QCA ROM Version :0x00000201 > Bluetooth: hci0: QCA Patch Version:0x00000001 > Bluetooth: hci0: QCA controller version 0x02140201 > Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv > Bluetooth: hci0: QCA Downloading qca/crnv21.bin > Bluetooth: hci0: QCA setup on UART is completed Thanks for the report. To be sure the issue doesn't fall through the cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression tracking bot: #regzbot ^introduced 6945795bc81 #regzbot title Bluetooth: Dragonboard 845c (SDM845) devboard broken #regzbot ignore-activity This isn't a regression? This issue or a fix for it are already discussed somewhere else? It was fixed already? You want to clarify when the regression started to happen? Or point out I got the title or something else totally wrong? Then just reply and tell me -- ideally while also telling regzbot about it, as explained by the page listed in the footer of this mail. Developers: When fixing the issue, remember to add 'Link:' tags pointing to the report (the parent of this mail). See page linked in footer for details. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you.
On Fri, Jul 07, 2023 at 07:12:35PM +0530, Amit Pundir wrote: > On Fri, 7 Jul 2023 at 16:37, Johan Hovold <johan@kernel.org> wrote: > > That's odd. You should still see the above messages also with this patch > > applied, but you may now need to provide a valid device address before > > being able to use device in case the bootloader has not provided one > > (e.g. using btmgmt). > > Sorry for the confusion, I missed the part where I do see these > messages when the kernel module is loaded but the direct firmware > loading fails. > > Bluetooth: hci0: setting up wcn399x > Bluetooth: hci0: QCA Product ID :0x0000000a > Bluetooth: hci0: QCA SOC Version :0x40010214 > Bluetooth: hci0: QCA ROM Version :0x00000201 > Bluetooth: hci0: QCA Patch Version:0x00000001 > Bluetooth: hci0: QCA controller version 0x02140201 > Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv > bluetooth hci0: Direct firmware load for qca/crbtfw21.tlv failed with error -2 > Bluetooth: hci0: QCA Failed to request file: qca/crbtfw21.tlv (-2) > Bluetooth: hci0: QCA Failed to download patch (-2) > Bluetooth: hci0: QCA preshutdown_cmd failed (-56) > > This happens in all the cases (working and non-working BT) because > filesystem is not mounted by that time. I'm running AOSP and all the > kernel modules get loaded from a ramdisk. But in the working case, the > firmware loading kicks in again later in the boot process and BT gets > initiazed.. > > With this patch, after the first attempt to load the firmware fails, > the firmware loading doesn't kick-in again. Also even if I keep the > firmware in ramdisk then the direct firmware loading from ramdisk > happens but BT still doesn't work > https://bugs.linaro.org/attachment.cgi?id=1148. So everything appears to work as intended. First, the firmware needs to be in your initramfs if you want to avoid that initial fw load failure. And after that you need to provide a valid device address as these devices ship without one. Once you set the address, the firmware should be loaded if it hasn't been already. > > Are there any error messages in the log when running with this patch? > > I don't see any relevant error message in dmesg. I'll check if I can > find a command line BT debug tool which I can use on AOSP for > debugging. There used to be a few hci command line tools, when I > looked into it a few years ago. Not sure if they are still around and > useful. Yeah, I'm not sure how you set the device address with the Android stack, but there must be some way as there are other bluetooth controllers out there which similarly need a valid address before they can be used. Johan
On Mon, 10 Jul 2023 at 17:14, Johan Hovold <johan@kernel.org> wrote: > > On Fri, Jul 07, 2023 at 07:12:35PM +0530, Amit Pundir wrote: > > On Fri, 7 Jul 2023 at 16:37, Johan Hovold <johan@kernel.org> wrote: > > > > That's odd. You should still see the above messages also with this patch > > > applied, but you may now need to provide a valid device address before > > > being able to use device in case the bootloader has not provided one > > > (e.g. using btmgmt). > > > > Sorry for the confusion, I missed the part where I do see these > > messages when the kernel module is loaded but the direct firmware > > loading fails. > > > > Bluetooth: hci0: setting up wcn399x > > Bluetooth: hci0: QCA Product ID :0x0000000a > > Bluetooth: hci0: QCA SOC Version :0x40010214 > > Bluetooth: hci0: QCA ROM Version :0x00000201 > > Bluetooth: hci0: QCA Patch Version:0x00000001 > > Bluetooth: hci0: QCA controller version 0x02140201 > > Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv > > bluetooth hci0: Direct firmware load for qca/crbtfw21.tlv failed with error -2 > > Bluetooth: hci0: QCA Failed to request file: qca/crbtfw21.tlv (-2) > > Bluetooth: hci0: QCA Failed to download patch (-2) > > Bluetooth: hci0: QCA preshutdown_cmd failed (-56) > > > > This happens in all the cases (working and non-working BT) because > > filesystem is not mounted by that time. I'm running AOSP and all the > > kernel modules get loaded from a ramdisk. But in the working case, the > > firmware loading kicks in again later in the boot process and BT gets > > initiazed.. > > > > With this patch, after the first attempt to load the firmware fails, > > the firmware loading doesn't kick-in again. Also even if I keep the > > firmware in ramdisk then the direct firmware loading from ramdisk > > happens but BT still doesn't work > > https://bugs.linaro.org/attachment.cgi?id=1148. > > So everything appears to work as intended. First, the firmware needs to > be in your initramfs if you want to avoid that initial fw load failure. > > And after that you need to provide a valid device address as these > devices ship without one. > > Once you set the address, the firmware should be loaded if it hasn't > been already. Thanks a lot for explanation Johan. I just booted up Debian on DB845c and btmgmt works as you pointed out above. root@linaro-gnome:~# root@linaro-gnome:~# uname -a Linux linaro-gnome 6.5.0-rc1 #1 SMP PREEMPT Mon Jul 10 15:42:50 IST 2023 aarch64 GNU/Linux root@linaro-gnome:~# root@linaro-gnome:~# btmgmt public-addr 01:02:03:04:05:06 [ 165.708146] Bluetooth: MGMT ver 1.22 hci0 Set Public Address complete, options: [ 165.715275] Bluetooth: hci0: setting up wcn399x root@linaro-gnome:~# [ 165.868755] Bluetooth: hci0: QCA Product ID :0x0000000a [ 165.874272] Bluetooth: hci0: QCA SOC Version :0x40010214 [ 165.879758] Bluetooth: hci0: QCA ROM Version :0x00000201 [ 165.885226] Bluetooth: hci0: QCA Patch Version:0x00000001 [ 165.903506] Bluetooth: hci0: QCA controller version 0x02140201 [ 165.909424] Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv [ 166.794992] Bluetooth: hci0: QCA Downloading qca/crnv21.bin [ 166.870882] Bluetooth: hci0: QCA setup on UART is completed > > > > Are there any error messages in the log when running with this patch? > > > > I don't see any relevant error message in dmesg. I'll check if I can > > find a command line BT debug tool which I can use on AOSP for > > debugging. There used to be a few hci command line tools, when I > > looked into it a few years ago. Not sure if they are still around and > > useful. > > Yeah, I'm not sure how you set the device address with the Android > stack, but there must be some way as there are other bluetooth > controllers out there which similarly need a valid address before they > can be used. > I'll look if I can reuse/simplify "btmgmt public-addr" command on Android or find an equivalent tool to do that. Regards, Amit Pundir
On 10.07.23 14:22, Amit Pundir wrote: > On Mon, 10 Jul 2023 at 17:14, Johan Hovold <johan@kernel.org> wrote: >> On Fri, Jul 07, 2023 at 07:12:35PM +0530, Amit Pundir wrote: >>> On Fri, 7 Jul 2023 at 16:37, Johan Hovold <johan@kernel.org> wrote: >>>> Are there any error messages in the log when running with this patch? >>> I don't see any relevant error message in dmesg. I'll check if I can >>> find a command line BT debug tool which I can use on AOSP for >>> debugging. There used to be a few hci command line tools, when I >>> looked into it a few years ago. Not sure if they are still around and >>> useful. >> Yeah, I'm not sure how you set the device address with the Android >> stack, but there must be some way as there are other bluetooth >> controllers out there which similarly need a valid address before they >> can be used. > I'll look if I can reuse/simplify "btmgmt public-addr" command on > Android or find an equivalent tool to do that. Please correct me if I'm wrong: the avove to me sounds like you are happy with this approach, even if this is kind of a regression; but likely one that is rare and thus not worth making a fuzz about. In that case I'll remove it from the regression tracking: #regzbot resolve: minor issue, workaround found #regzbot ignore-activity Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page.
On Tue, 25 Jul 2023 at 15:11, Linux regression tracking (Thorsten Leemhuis) <regressions@leemhuis.info> wrote: > > On 10.07.23 14:22, Amit Pundir wrote: > > On Mon, 10 Jul 2023 at 17:14, Johan Hovold <johan@kernel.org> wrote: > >> On Fri, Jul 07, 2023 at 07:12:35PM +0530, Amit Pundir wrote: > >>> On Fri, 7 Jul 2023 at 16:37, Johan Hovold <johan@kernel.org> wrote: > >>>> Are there any error messages in the log when running with this patch? > >>> I don't see any relevant error message in dmesg. I'll check if I can > >>> find a command line BT debug tool which I can use on AOSP for > >>> debugging. There used to be a few hci command line tools, when I > >>> looked into it a few years ago. Not sure if they are still around and > >>> useful. > >> Yeah, I'm not sure how you set the device address with the Android > >> stack, but there must be some way as there are other bluetooth > >> controllers out there which similarly need a valid address before they > >> can be used. > > I'll look if I can reuse/simplify "btmgmt public-addr" command on > > Android or find an equivalent tool to do that. > > Please correct me if I'm wrong: the avove to me sounds like you are > happy with this approach, even if this is kind of a regression; but > likely one that is rare and thus not worth making a fuzz about. In that > case I'll remove it from the regression tracking: Hi. Thanks to Stephan, this had been taken care of from the userspace https://android.googlesource.com/device/linaro/dragonboard/+/f70f12a826af So please remove it from the kernel regression tracking. Regards, Amit Pundir > > #regzbot resolve: minor issue, workaround found > #regzbot ignore-activity > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > -- > Everything you wanna know about Linux kernel regression tracking: > https://linux-regtracking.leemhuis.info/about/#tldr > If I did something stupid, please tell me, as explained on that page.
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index c2a805ee55cc..ce03038b3460 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -4615,16 +4615,14 @@ static int hci_dev_setup_sync(struct hci_dev *hdev) * BD_ADDR invalid before creating the HCI device or in * its setup callback. */ - invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks); - + invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks) || + test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks); if (!ret) { if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks) && !bacmp(&hdev->public_addr, BDADDR_ANY)) hci_dev_get_bd_addr_from_property(hdev); - if ((invalid_bdaddr || - test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) && - bacmp(&hdev->public_addr, BDADDR_ANY) && + if (invalid_bdaddr && bacmp(&hdev->public_addr, BDADDR_ANY) && hdev->set_bdaddr) { ret = hdev->set_bdaddr(hdev, &hdev->public_addr); if (!ret)
Devices that lack persistent storage for the device address can indicate this by setting the HCI_QUIRK_INVALID_BDADDR which causes the controller to be marked as unconfigured until user space has set a valid address. The related HCI_QUIRK_USE_BDADDR_PROPERTY was later added to similarly indicate that the device lacks a valid address but that one may be specified in the devicetree. As is clear from commit 7a0e5b15ca45 ("Bluetooth: Add quirk for reading BD_ADDR from fwnode property") that added and documented this quirk and commits like de79a9df1692 ("Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY"), the device address of controllers with this flag should be treated as invalid until user space has had a chance to configure the controller in case the devicetree property is missing. As it does not make sense to allow controllers with invalid addresses, restore the original semantics, which also makes sure that the implementation is consistent (e.g. get_missing_options() indicates that the address must be set) and matches the documentation (including comments in the code, such as, "In case any of them is set, the controller has to start up as unconfigured."). Fixes: e668eb1e1578 ("Bluetooth: hci_core: Don't stop BT if the BD address missing in dts") Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- net/bluetooth/hci_sync.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)