diff mbox series

[v2,1/4] dt-bindings: bluetooth: add new wcn3991 compatible to fix bd_addr

Message ID 20240318110855.31954-2-johan+linaro@kernel.org (mailing list archive)
State Superseded
Headers show
Series Bluetooth: qca: fix device-address endianness | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success

Commit Message

Johan Hovold March 18, 2024, 11:08 a.m. UTC
Several Qualcomm Bluetooth controllers lack persistent storage for the
device address and instead one can be provided by the boot firmware
using the 'local-bd-address' devicetree property.

The Bluetooth bindings clearly says that the address should be specified
in little-endian order, but due to a long-standing bug in the Qualcomm
driver which reversed the address some bootloaders have been providing
the address in big-endian order instead.

The only device out there that should be affected by this is the WCN3991
used in some Chromebooks. To maintain backwards compatibility, mark the
current compatible string as deprecated and add a new
'qcom,wcn3991-bt-bdaddr-le' for firmware which conforms with the
binding.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../net/bluetooth/qualcomm-bluetooth.yaml     | 29 +++++++++++--------
 1 file changed, 17 insertions(+), 12 deletions(-)

Comments

bluez.test.bot@gmail.com March 18, 2024, 11:36 a.m. UTC | #1
This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: drivers/bluetooth/hci_qca.c:1904
error: drivers/bluetooth/hci_qca.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth
Dmitry Baryshkov March 18, 2024, 1 p.m. UTC | #2
On Mon, 18 Mar 2024 at 13:09, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Several Qualcomm Bluetooth controllers lack persistent storage for the
> device address and instead one can be provided by the boot firmware
> using the 'local-bd-address' devicetree property.
>
> The Bluetooth bindings clearly says that the address should be specified
> in little-endian order, but due to a long-standing bug in the Qualcomm
> driver which reversed the address some bootloaders have been providing
> the address in big-endian order instead.
>
> The only device out there that should be affected by this is the WCN3991
> used in some Chromebooks. To maintain backwards compatibility, mark the
> current compatible string as deprecated and add a new
> 'qcom,wcn3991-bt-bdaddr-le' for firmware which conforms with the
> binding.
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../net/bluetooth/qualcomm-bluetooth.yaml     | 29 +++++++++++--------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> index eba2f3026ab0..b6fce6d02138 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> @@ -15,18 +15,22 @@ description:
>
>  properties:
>    compatible:
> -    enum:
> -      - qcom,qca2066-bt
> -      - qcom,qca6174-bt
> -      - qcom,qca9377-bt
> -      - qcom,wcn3988-bt
> -      - qcom,wcn3990-bt
> -      - qcom,wcn3991-bt
> -      - qcom,wcn3998-bt
> -      - qcom,qca6390-bt
> -      - qcom,wcn6750-bt
> -      - qcom,wcn6855-bt
> -      - qcom,wcn7850-bt
> +    oneOf:
> +      - enum:
> +          - qcom,qca2066-bt
> +          - qcom,qca6174-bt
> +          - qcom,qca9377-bt
> +          - qcom,wcn3988-bt
> +          - qcom,wcn3990-bt
> +          - qcom,wcn3991-bt-bdaddr-le

This compatible doesn't describe new hardware kind. As such, I think,
the better way would be to continue using qcom,wcn3991-bt compatible
string + add some kind of qcom,bt-addr-le property.

> +          - qcom,wcn3998-bt
> +          - qcom,qca6390-bt
> +          - qcom,wcn6750-bt
> +          - qcom,wcn6855-bt
> +          - qcom,wcn7850-bt
> +      - enum:
> +          - qcom,wcn3991-bt
> +        deprecated: true
>
>    enable-gpios:
>      maxItems: 1
> @@ -122,6 +126,7 @@ allOf:
>                - qcom,wcn3988-bt
>                - qcom,wcn3990-bt
>                - qcom,wcn3991-bt
> +              - qcom,wcn3991-bt-bdaddr-le
>                - qcom,wcn3998-bt
>      then:
>        required:
> --
> 2.43.2
>
>
Johan Hovold March 18, 2024, 1:17 p.m. UTC | #3
On Mon, Mar 18, 2024 at 03:00:40PM +0200, Dmitry Baryshkov wrote:
> On Mon, 18 Mar 2024 at 13:09, Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > Several Qualcomm Bluetooth controllers lack persistent storage for the
> > device address and instead one can be provided by the boot firmware
> > using the 'local-bd-address' devicetree property.
> >
> > The Bluetooth bindings clearly says that the address should be specified
> > in little-endian order, but due to a long-standing bug in the Qualcomm
> > driver which reversed the address some bootloaders have been providing
> > the address in big-endian order instead.
> >
> > The only device out there that should be affected by this is the WCN3991
> > used in some Chromebooks. To maintain backwards compatibility, mark the
> > current compatible string as deprecated and add a new
> > 'qcom,wcn3991-bt-bdaddr-le' for firmware which conforms with the
> > binding.

> This compatible doesn't describe new hardware kind. As such, I think,
> the better way would be to continue using qcom,wcn3991-bt compatible
> string + add some kind of qcom,bt-addr-le property.

No, you can't handle backwards compatibility by *adding* a property.

I wanted to avoid doing this, but if we have to support Google's broken
boot firmware for these devices, then this is how it needs to be done.

Johan
Dmitry Baryshkov March 18, 2024, 2:17 p.m. UTC | #4
On Mon, 18 Mar 2024 at 15:17, Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 03:00:40PM +0200, Dmitry Baryshkov wrote:
> > On Mon, 18 Mar 2024 at 13:09, Johan Hovold <johan+linaro@kernel.org> wrote:
> > >
> > > Several Qualcomm Bluetooth controllers lack persistent storage for the
> > > device address and instead one can be provided by the boot firmware
> > > using the 'local-bd-address' devicetree property.
> > >
> > > The Bluetooth bindings clearly says that the address should be specified
> > > in little-endian order, but due to a long-standing bug in the Qualcomm
> > > driver which reversed the address some bootloaders have been providing
> > > the address in big-endian order instead.
> > >
> > > The only device out there that should be affected by this is the WCN3991
> > > used in some Chromebooks. To maintain backwards compatibility, mark the
> > > current compatible string as deprecated and add a new
> > > 'qcom,wcn3991-bt-bdaddr-le' for firmware which conforms with the
> > > binding.
>
> > This compatible doesn't describe new hardware kind. As such, I think,
> > the better way would be to continue using qcom,wcn3991-bt compatible
> > string + add some kind of qcom,bt-addr-le property.
>
> No, you can't handle backwards compatibility by *adding* a property.
>
> I wanted to avoid doing this, but if we have to support Google's broken
> boot firmware for these devices, then this is how it needs to be done.

One hardware compat string per hardware type.
Johan Hovold March 18, 2024, 2:45 p.m. UTC | #5
On Mon, Mar 18, 2024 at 04:17:24PM +0200, Dmitry Baryshkov wrote:
> On Mon, 18 Mar 2024 at 15:17, Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Mar 18, 2024 at 03:00:40PM +0200, Dmitry Baryshkov wrote:
> > > On Mon, 18 Mar 2024 at 13:09, Johan Hovold <johan+linaro@kernel.org> wrote:
> > > > The only device out there that should be affected by this is the WCN3991
> > > > used in some Chromebooks. To maintain backwards compatibility, mark the
> > > > current compatible string as deprecated and add a new
> > > > 'qcom,wcn3991-bt-bdaddr-le' for firmware which conforms with the
> > > > binding.
> >
> > > This compatible doesn't describe new hardware kind. As such, I think,
> > > the better way would be to continue using qcom,wcn3991-bt compatible
> > > string + add some kind of qcom,bt-addr-le property.
> >
> > No, you can't handle backwards compatibility by *adding* a property.
> >
> > I wanted to avoid doing this, but if we have to support Google's broken
> > boot firmware for these devices, then this is how it needs to be done.
> 
> One hardware compat string per hardware type.

Again, no. Not when there is an incompatible change in the binding. Then
we add a new compatible string and deprecate the old binding.

Johan
Rob Herring (Arm) March 18, 2024, 2:48 p.m. UTC | #6
On Mon, Mar 18, 2024 at 02:17:47PM +0100, Johan Hovold wrote:
> On Mon, Mar 18, 2024 at 03:00:40PM +0200, Dmitry Baryshkov wrote:
> > On Mon, 18 Mar 2024 at 13:09, Johan Hovold <johan+linaro@kernel.org> wrote:
> > >
> > > Several Qualcomm Bluetooth controllers lack persistent storage for the
> > > device address and instead one can be provided by the boot firmware
> > > using the 'local-bd-address' devicetree property.
> > >
> > > The Bluetooth bindings clearly says that the address should be specified
> > > in little-endian order, but due to a long-standing bug in the Qualcomm
> > > driver which reversed the address some bootloaders have been providing
> > > the address in big-endian order instead.
> > >
> > > The only device out there that should be affected by this is the WCN3991
> > > used in some Chromebooks. To maintain backwards compatibility, mark the
> > > current compatible string as deprecated and add a new
> > > 'qcom,wcn3991-bt-bdaddr-le' for firmware which conforms with the
> > > binding.
> 
> > This compatible doesn't describe new hardware kind. As such, I think,
> > the better way would be to continue using qcom,wcn3991-bt compatible
> > string + add some kind of qcom,bt-addr-le property.
> 
> No, you can't handle backwards compatibility by *adding* a property.

But you could add a property for the not broken case. That's a bit odd, 
but so is your compatible.

> I wanted to avoid doing this, but if we have to support Google's broken
> boot firmware for these devices, then this is how it needs to be done.

Don't Chromebooks update everything together. So maybe we don't care in 
this case?

Rob
Johan Hovold March 18, 2024, 3:10 p.m. UTC | #7
On Mon, Mar 18, 2024 at 09:48:06AM -0500, Rob Herring wrote:
> On Mon, Mar 18, 2024 at 02:17:47PM +0100, Johan Hovold wrote:
> > On Mon, Mar 18, 2024 at 03:00:40PM +0200, Dmitry Baryshkov wrote:
> > > On Mon, 18 Mar 2024 at 13:09, Johan Hovold <johan+linaro@kernel.org> wrote:

> > > > The only device out there that should be affected by this is the WCN3991
> > > > used in some Chromebooks. To maintain backwards compatibility, mark the
> > > > current compatible string as deprecated and add a new
> > > > 'qcom,wcn3991-bt-bdaddr-le' for firmware which conforms with the
> > > > binding.
> > 
> > > This compatible doesn't describe new hardware kind. As such, I think,
> > > the better way would be to continue using qcom,wcn3991-bt compatible
> > > string + add some kind of qcom,bt-addr-le property.
> > 
> > No, you can't handle backwards compatibility by *adding* a property.
> 
> But you could add a property for the not broken case. That's a bit odd, 
> but so is your compatible.

Sure, we could have a property that we only add for wcn3991-bt going
forward.

But we can't go back in time and add this property to all devicetrees
and say that 'local-bd-address' is big endian unless that property is
present (as that would leave all the non-wcn3991 devicetrees broken).

> > I wanted to avoid doing this, but if we have to support Google's broken
> > boot firmware for these devices, then this is how it needs to be done.
> 
> Don't Chromebooks update everything together. So maybe we don't care in 
> this case?

That was my hope, but Matthias seemed to suggest that we need to
continue supporting the current (broken) binding because doing such a
coordinated update may be easier said than done:

	https://lore.kernel.org/lkml/ZcuQ2qRX0zsLSVRL@google.com/

A new compatible string (or one-off property) would allow them do make a
change when they are ready (e.g. by only updating the devicetrees after
all boot firmware has been patched and pushed out).

Johan
Doug Anderson March 18, 2024, 3:26 p.m. UTC | #8
Hi,

On Mon, Mar 18, 2024 at 8:10 AM Johan Hovold <johan@kernel.org> wrote:
>
> > > I wanted to avoid doing this, but if we have to support Google's broken
> > > boot firmware for these devices, then this is how it needs to be done.
> >
> > Don't Chromebooks update everything together. So maybe we don't care in
> > this case?
>
> That was my hope, but Matthias seemed to suggest that we need to
> continue supporting the current (broken) binding because doing such a
> coordinated update may be easier said than done:
>
>         https://lore.kernel.org/lkml/ZcuQ2qRX0zsLSVRL@google.com/

Chromebooks update kernel and devicetree together, but not firmware.
Firmware is relatively hard to get updated trying to have kernel and
firmware updates coordinated at the exact same time has challenges.
This would further be complicated by the fact that firmware
qualification for each variant happens on its own timeline.


> A new compatible string (or one-off property) would allow them do make a
> change when they are ready (e.g. by only updating the devicetrees after
> all boot firmware has been patched and pushed out).

I have no real opinion about the exact way this is solved so happy to
let DT folks decide on how they want this. I will note, however, that
device trees are never shipped separately and thus we have no
intrinsic need for DT backward compatbility here. It would be OK from
a ChromeOS perspective to add a property or compatible string for the
broken case.

-Doug
Doug Anderson March 18, 2024, 3:31 p.m. UTC | #9
Hi,

On Mon, Mar 18, 2024 at 8:26 AM Doug Anderson <dianders@google.com> wrote:
>
> Hi,
>
> On Mon, Mar 18, 2024 at 8:10 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > > > I wanted to avoid doing this, but if we have to support Google's broken
> > > > boot firmware for these devices, then this is how it needs to be done.
> > >
> > > Don't Chromebooks update everything together. So maybe we don't care in
> > > this case?
> >
> > That was my hope, but Matthias seemed to suggest that we need to
> > continue supporting the current (broken) binding because doing such a
> > coordinated update may be easier said than done:
> >
> >         https://lore.kernel.org/lkml/ZcuQ2qRX0zsLSVRL@google.com/
>
> Chromebooks update kernel and devicetree together, but not firmware.
> Firmware is relatively hard to get updated trying to have kernel and
> firmware updates coordinated at the exact same time has challenges.
> This would further be complicated by the fact that firmware
> qualification for each variant happens on its own timeline.
>
>
> > A new compatible string (or one-off property) would allow them do make a
> > change when they are ready (e.g. by only updating the devicetrees after
> > all boot firmware has been patched and pushed out).
>
> I have no real opinion about the exact way this is solved so happy to
> let DT folks decide on how they want this. I will note, however, that
> device trees are never shipped separately and thus we have no
> intrinsic need for DT backward compatbility here. It would be OK from
> a ChromeOS perspective to add a property or compatible string for the
> broken case.

Actually, I should probably say more about this to make it clear how it works.

Chromebooks ship the kernel as a FIT image which bundles the kernel
and device trees together. The firmware looks at all the bundled
device trees and picks the proper one based on the board name,
revision, and SKU ID. The firmware then looks for the bluetooth node
(I believe it finds it from the "aliases" section) and adds the MAC
address there.

...so we could update the DT to add a property (if that's desired)
even if we don't update the firmware.

-Doug
Johan Hovold March 18, 2024, 3:47 p.m. UTC | #10
On Mon, Mar 18, 2024 at 08:31:09AM -0700, Doug Anderson wrote:
> On Mon, Mar 18, 2024 at 8:26 AM Doug Anderson <dianders@google.com> wrote:

> > > A new compatible string (or one-off property) would allow them do make a
> > > change when they are ready (e.g. by only updating the devicetrees after
> > > all boot firmware has been patched and pushed out).
> >
> > I have no real opinion about the exact way this is solved so happy to
> > let DT folks decide on how they want this. I will note, however, that
> > device trees are never shipped separately and thus we have no
> > intrinsic need for DT backward compatbility here. It would be OK from
> > a ChromeOS perspective to add a property or compatible string for the
> > broken case.
> 
> Actually, I should probably say more about this to make it clear how it works.
> 
> Chromebooks ship the kernel as a FIT image which bundles the kernel
> and device trees together. The firmware looks at all the bundled
> device trees and picks the proper one based on the board name,
> revision, and SKU ID. The firmware then looks for the bluetooth node
> (I believe it finds it from the "aliases" section) and adds the MAC
> address there.
> 
> ...so we could update the DT to add a property (if that's desired)
> even if we don't update the firmware.

Thanks for the details. Sounds like we could get away with adding a new
property for the broken firmware in this case, which should resolve this
nicely without having to deprecate anything.

Could you carry such a devicetree patch out-of-tree until the firmware
has been fixed?

Johan
Doug Anderson March 18, 2024, 3:58 p.m. UTC | #11
Hi,

On Mon, Mar 18, 2024 at 8:47 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Mar 18, 2024 at 08:31:09AM -0700, Doug Anderson wrote:
> > On Mon, Mar 18, 2024 at 8:26 AM Doug Anderson <dianders@google.com> wrote:
>
> > > > A new compatible string (or one-off property) would allow them do make a
> > > > change when they are ready (e.g. by only updating the devicetrees after
> > > > all boot firmware has been patched and pushed out).
> > >
> > > I have no real opinion about the exact way this is solved so happy to
> > > let DT folks decide on how they want this. I will note, however, that
> > > device trees are never shipped separately and thus we have no
> > > intrinsic need for DT backward compatbility here. It would be OK from
> > > a ChromeOS perspective to add a property or compatible string for the
> > > broken case.
> >
> > Actually, I should probably say more about this to make it clear how it works.
> >
> > Chromebooks ship the kernel as a FIT image which bundles the kernel
> > and device trees together. The firmware looks at all the bundled
> > device trees and picks the proper one based on the board name,
> > revision, and SKU ID. The firmware then looks for the bluetooth node
> > (I believe it finds it from the "aliases" section) and adds the MAC
> > address there.
> >
> > ...so we could update the DT to add a property (if that's desired)
> > even if we don't update the firmware.
>
> Thanks for the details. Sounds like we could get away with adding a new
> property for the broken firmware in this case, which should resolve this
> nicely without having to deprecate anything.
>
> Could you carry such a devicetree patch out-of-tree until the firmware
> has been fixed?

IMO we shouldn't try to fix the firmware at all. Given the fact that
it took me a year to get a firmware uprev completed for one trogdor
variant for fixes that actually had functional impact, it's possible
we'll never actually get an uprev completed that includes this fix or
it will happen years from now when nobody remembers about it. I'm also
certain this whole issue will also cause a bunch of debugging over the
years if we try to fix it in firmware like that. There are cases where
people end up running with old firmware since the developer workflow
doesn't automatically update it.

The handling should be added upstream and we should just accept that
the trogdor firmware gets it backward.

-Doug
Johan Hovold March 18, 2024, 4:34 p.m. UTC | #12
On Mon, Mar 18, 2024 at 08:58:40AM -0700, Doug Anderson wrote:
> On Mon, Mar 18, 2024 at 8:47 AM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Mar 18, 2024 at 08:31:09AM -0700, Doug Anderson wrote:

> > Thanks for the details. Sounds like we could get away with adding a new
> > property for the broken firmware in this case, which should resolve this
> > nicely without having to deprecate anything.
> >
> > Could you carry such a devicetree patch out-of-tree until the firmware
> > has been fixed?
> 
> IMO we shouldn't try to fix the firmware at all. Given the fact that
> it took me a year to get a firmware uprev completed for one trogdor
> variant for fixes that actually had functional impact, it's possible
> we'll never actually get an uprev completed that includes this fix or
> it will happen years from now when nobody remembers about it. I'm also
> certain this whole issue will also cause a bunch of debugging over the
> years if we try to fix it in firmware like that. There are cases where
> people end up running with old firmware since the developer workflow
> doesn't automatically update it.
> 
> The handling should be added upstream and we should just accept that
> the trogdor firmware gets it backward.

Fair enough.

Rob, are you OK with adding a 'qcom,local-bd-address-broken' or
similarly named property to indicate that the boot firmware passes the
address in the wrong order?

I'd then add that property to sc7180-trogdor.dtsi in mainline.

Johan
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
index eba2f3026ab0..b6fce6d02138 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
@@ -15,18 +15,22 @@  description:
 
 properties:
   compatible:
-    enum:
-      - qcom,qca2066-bt
-      - qcom,qca6174-bt
-      - qcom,qca9377-bt
-      - qcom,wcn3988-bt
-      - qcom,wcn3990-bt
-      - qcom,wcn3991-bt
-      - qcom,wcn3998-bt
-      - qcom,qca6390-bt
-      - qcom,wcn6750-bt
-      - qcom,wcn6855-bt
-      - qcom,wcn7850-bt
+    oneOf:
+      - enum:
+          - qcom,qca2066-bt
+          - qcom,qca6174-bt
+          - qcom,qca9377-bt
+          - qcom,wcn3988-bt
+          - qcom,wcn3990-bt
+          - qcom,wcn3991-bt-bdaddr-le
+          - qcom,wcn3998-bt
+          - qcom,qca6390-bt
+          - qcom,wcn6750-bt
+          - qcom,wcn6855-bt
+          - qcom,wcn7850-bt
+      - enum:
+          - qcom,wcn3991-bt
+        deprecated: true
 
   enable-gpios:
     maxItems: 1
@@ -122,6 +126,7 @@  allOf:
               - qcom,wcn3988-bt
               - qcom,wcn3990-bt
               - qcom,wcn3991-bt
+              - qcom,wcn3991-bt-bdaddr-le
               - qcom,wcn3998-bt
     then:
       required: