[PATCH 0/2] Bluetooth: fix bdaddr quirks

[PATCH 0/2] Bluetooth: fix bdaddr quirks

[Johan Hovold]

These patches fixes 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.2

[PATCH 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 a8785126df75..f45598b5a532 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -4573,23 +4573,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.2

[PATCH 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 f45598b5a532..cc925a240f3b 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -4570,16 +4570,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.2

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

[Luiz Augusto von Dentz]

Hi Johan,

On Mon, Apr 24, 2023 at 6:35 AM 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.
>
> 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 a8785126df75..f45598b5a532 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -4573,23 +4573,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;

I'd keep the original comments since it appears you haven't changed
its logic with respect to invalid_bdaddr.

>                 }
>         }
>
> --
> 2.39.2
>


-- 
Luiz Augusto von Dentz

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

[Johan Hovold]

On Tue, May 02, 2023 at 04:44:48PM -0700, Luiz Augusto von Dentz wrote:
> Hi Johan,
> 
> On Mon, Apr 24, 2023 at 6:35 AM 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.
> >
> > 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 a8785126df75..f45598b5a532 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -4573,23 +4573,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;
> 
> I'd keep the original comments since it appears you haven't changed
> its logic with respect to invalid_bdaddr.

The comment no longer applies as set_bdaddr can now be called also when
there is no "device property" holding the address (e.g. when one has
been set using btmgmt and HCI_QUIRK_NON_PERSISTENT_SETUP is set).

And obviously we shouldn't mark the controller as unconfigured after
we've successfully set an address.

Johan

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

[Johan Hovold]

On Mon, Apr 24, 2023 at 03:35:40PM +0200, Johan Hovold wrote:
> These patches fixes 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 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(-)

Any further comments to this series, or can this one be merged for 6.5
now?

Johan

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

[Luiz Augusto von Dentz]

Hi Johan,

On Tue, May 30, 2023 at 7:25 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Apr 24, 2023 at 03:35:40PM +0200, Johan Hovold wrote:
> > These patches fixes 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 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(-)
>
> Any further comments to this series, or can this one be merged for 6.5
> now?

Looks like this was removed from Patchwork since it has passed 30 days
without updates, could you please resend it so CI can pick it up and
test it again.

> Johan



-- 
Luiz Augusto von Dentz

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

[Johan Hovold]

On Tue, May 30, 2023 at 01:06:01PM -0700, Luiz Augusto von Dentz wrote:
> On Tue, May 30, 2023 at 7:25 AM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Apr 24, 2023 at 03:35:40PM +0200, Johan Hovold wrote:

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

> > Any further comments to this series, or can this one be merged for 6.5
> > now?
> 
> Looks like this was removed from Patchwork since it has passed 30 days
> without updates, could you please resend it so CI can pick it up and
> test it again.

Both series resent:

	https://lore.kernel.org/lkml/20230531085759.2803-1-johan+linaro@kernel.org/
	https://lore.kernel.org/lkml/20230531090424.3187-1-johan+linaro@kernel.org/

Getting both of these, or at least the first one, into 6.4 would of
course be even better.

Thanks.

Johan