Re: [PATCH] Bluetooth: qca: fix device-address endianness

[Matthias Kaehlcke <mka@chromium.org>]

On Tue, Jan 09, 2024 at 06:12:26PM +0100, Johan Hovold wrote:
> On Tue, Jan 09, 2024 at 04:50:59PM +0000, Matthias Kaehlcke wrote:
> 
> > On Wed, Dec 27, 2023 at 07:03:06PM +0100, Johan Hovold wrote:
> > > The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth
> > > device address in MSB order when setting it using the
> > > EDL_WRITE_BD_ADDR_OPCODE command.
> > > 
> > > Presumably, this is the case for all non-ROME devices which all use the
> > > EDL_WRITE_BD_ADDR_OPCODE command for this (unlike the ROME devices which
> > > use a different command and expect the address in LSB order).
> > > 
> > > Reverse the little-endian address before setting it to make sure that
> > > the address can be configured using tools like btmgmt or using the
> > > 'local-bd-address' devicetree property.
> > > 
> > > Note that this can potentially break systems with boot firmware which
> > > has started relying on the broken behaviour and is incorrectly passing
> > > the address via devicetree in MSB order.
> > 
> > We should not break existing devices. Their byte order for
> > 'local-bd-address' may not adhere to the 'spec', however in practice
> > it is the correct format for existing kernels.
> 
> That depends on in what way the current devices are broken.
> 
> Any machines that correctly specify their address in little-endian order
> in the devicetree would no longer be configured using the wrong address.
> So no problem there (except requiring users to re-pair their gadgets).
> 
> And tools like btgmt is broken on all of these Qualcomm machine in any
> case and would now start working as expected. So no problem there either
> (unless user space had adapted an inverted the addresses to btmgmt).
> 
> So the first question is whether there actually is any boot firmware out
> there which passes the BD_ADDR in reverse order?

Yes, (at least) the boot firmware for sc7180-trogdor devices.

hexdump -C /proc/device-tree/soc\@0/geniqup\@8c0000/serial\@88c000/bluetooth/local-bd-address
00000000  8c fd f0 40 15 dc

hciconfig
hci0:   Type: Primary  Bus: UART
        BD Address: 8C:FD:F0:40:15:DC  ACL MTU: 1024:8  SCO MTU: 240:8
        UP RUNNING 
        RX bytes:1700 acl:0 sco:0 events:95 errors:0
        TX bytes:128949 acl:0 sco:0 commands:578 errors:0

> > I suggest adding a quirk like 'local-bd-address-msb-quirk' or
> > 'qcom,local-bd-address-msb-quirk' to make sure existing devices keep
> > working properly.
> 
> I don't think that would work. If this is something that we really need
> to handle, then there's probably no way around introducing new
> compatible strings for boot firmware that isn't broken while maintaining
> the current broken behaviour with respect to 'local-bd-address' for some
> of the current ones.

I think it should work for sc7180-trogdor. For these devices the device tree
is bundled with the kernel image and can be updated. That might not be true
for other devices though.

Matthias