[PATCH RESEND 0/2] Bluetooth: fix bdaddr quirks

[PATCH RESEND 0/2] Bluetooth: fix bdaddr quirks

[Johan Hovold]

These patches fix a couple of issues with the two bdaddr quirks:

The first one allows HCI_QUIRK_INVALID_BDADDR to be used with
HCI_QUIRK_NON_PERSISTENT_SETUP.

The second patch restores the original semantics of the
HCI_QUIRK_USE_BDADDR_PROPERTY so that the controller is marked as
unconfigured when no device address is specified in the devicetree (as
the quirk is documented to work).

This specifically makes sure that Qualcomm HCI controllers such as
wcn6855 found on the Lenovo X13s are marked as unconfigured until user
space has provided a valid address.

Long term, the HCI_QUIRK_USE_BDADDR_PROPERTY should probably be dropped
in favour of HCI_QUIRK_INVALID_BDADDR and always checking the devicetree
property.

Johan


Johan Hovold (2):
  Bluetooth: fix invalid-bdaddr quirk for non-persistent setup
  Bluetooth: fix use-bdaddr-property quirk

 net/bluetooth/hci_sync.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

-- 
2.39.3

[PATCH RESEND 1/2] Bluetooth: fix invalid-bdaddr quirk for non-persistent setup

[Johan Hovold]

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.

Once configured, the device address must be set on every setup for
controllers with HCI_QUIRK_NON_PERSISTENT_SETUP to avoid marking the
controller as unconfigured and requiring the address to be set again.

Fixes: 740011cfe948 ("Bluetooth: Add new quirk for non-persistent setup settings")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 net/bluetooth/hci_sync.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 0efc2253265e..c2a805ee55cc 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -4618,23 +4618,17 @@ static int hci_dev_setup_sync(struct hci_dev *hdev)
 	invalid_bdaddr = test_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
 
 	if (!ret) {
-		if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
-			if (!bacmp(&hdev->public_addr, BDADDR_ANY))
-				hci_dev_get_bd_addr_from_property(hdev);
-
-			if (bacmp(&hdev->public_addr, BDADDR_ANY) &&
-			    hdev->set_bdaddr) {
-				ret = hdev->set_bdaddr(hdev,
-						       &hdev->public_addr);
-
-				/* If setting of the BD_ADDR from the device
-				 * property succeeds, then treat the address
-				 * as valid even if the invalid BD_ADDR
-				 * quirk indicates otherwise.
-				 */
-				if (!ret)
-					invalid_bdaddr = false;
-			}
+		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) &&
+		    hdev->set_bdaddr) {
+			ret = hdev->set_bdaddr(hdev, &hdev->public_addr);
+			if (!ret)
+				invalid_bdaddr = false;
 		}
 	}
 
-- 
2.39.3

[PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Johan Hovold]

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

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

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Luiz Augusto von Dentz]

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.

-- 
Luiz Augusto von Dentz

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Johan Hovold]

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

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Amit Pundir]

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
>

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Johan Hovold]

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

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Amit Pundir]

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

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Johan Hovold]

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

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Amit Pundir]

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

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Linux regression tracking (Thorsten Leemhuis)]

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.

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Amit Pundir]

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.

Re: [PATCH RESEND 2/2] Bluetooth: fix use-bdaddr-property quirk

[Linux regression tracking #adding (Thorsten Leemhuis)]

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