Re: [PATCH] Bluetooth: qca: generalise device address check

[Johan Hovold]

On Mon, Apr 29, 2024 at 01:31:53PM -0400, Luiz Augusto von Dentz wrote:
> On Mon, Apr 29, 2024 at 1:12 PM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> > On Mon, Apr 29, 2024 at 10:02 AM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > > On Mon, Apr 29, 2024 at 03:34:32PM +0530, Janaki Ramaiah Thota wrote:

> > > > Having a default BDA list from NVM BDA tag value will prevent developers
> > > > from using the device if there is no user space app(In Fluoride) to set
> > > > the BDA. Therefore, we are requesting to use default address check patch,
> > > > so that developer can change the NVM BDA to make use of the device.
> > >
> > > But a developer on such an old platform that can patch and replace the
> > > NVM configuration file should also be able to just disable the check in
> > > the driver right (e.g. by commenting out the call to
> > > qca_check_bdaddr())?
> > >
> > > >   List Of default Addresses:
> > > >   ---------------------------------------------------------
> > > > |       BDA          |      Chipset                       |
> > > >   ---------------------------------------------------------
> > > > | 39 80 10 00 00 20  |  WCN3988 with ROM Version 0x0200   |
> > > >   ---------------------------------------------------------
> > > > | 39 80 12 74 08 00  |  WCN3988 with ROM Version 0x0201   |
> > > >   ---------------------------------------------------------
> > > > | 39 90 21 64 07 00  |  WCN3990                           |
> > > >   ---------------------------------------------------------
> > > > | 39 98 00 00 5A AD  |  WCN3991                           |
> > > >   ---------------------------------------------------------
> > > > | 00 00 00 00 5A AD  |  QCA DEFAULT                       |
> > > >   ---------------------------------------------------------
> > >
> > > What about WCN6750 and 64:90:00:00:5a:ad?
> > >
> > > And then there's currently also:
> > >
> > > > > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin)
> > > > > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin)
> > >
> > > Which controllers use these configurations?
> >
> > These are not unique addresses though, we can't just have addresses by
> > chipset address mapping logic as that would cause address clashes over
> > the air, e.g. if there are other devices with the same chipset in the
> > vicinity.
> 
> I see where this is going now, the firmware actually contain these
> duplicated addresses which then are checked and cause
> HCI_QUIRK_USE_BDADDR_PROPERTY then the tries
> hci_dev_get_bd_addr_from_property which loads the local-bd-address
> property from the parente device (SOC?), btw that could also have an
> invalid/duplicated address.

Right, the expectation is that vendors don't abuse this and leave the
address in the devicetree as all-zero unless the boot firmware has
access to a unique address.

HCI_QUIRK_USE_BDADDR_PROPERTY effectively implies
HCI_QUIRK_INVALID_BDADDR, that is, both quirks marks the controller
address as invalid. The only difference is that the former also goes out
and checks if there's an address in the devicetree that can be used
instead.

The 'local-bd-address' property is used on boards where the boot
firmware has access to some storage for the address and updates the
devicetree with the board-specific address before passing the DT to the
kernel.

As I've mentioned before, we should probably just drop
HCI_QUIRK_USE_BDADDR_PROPERTY eventually and always look for an address
in the devicetree when HCI_QUIRK_INVALID_BDADDR is set instead.

We could take that one step further and always let the devicetree
override the controller address as Doug suggested, but I'm not sure
that's what we want to do generally.

Either way, these are later questions.

> Anyway the fact that firmware loading itself is programming a
> potentially duplicated address already seems wrong enough to me,
> either it shall leave it as 00... or set a valid address otherwise we
> always risk missing yet another duplicate address being introduced and
> then used over the air causing all sorts of problems for users.
> 
> So to be clear, QCA firmware shall never attempt to flash anything
> other than 00:00:00:00:00:00 if you don't have a valid and unique
> identity address, so we can get rid of this table altogether.

Nothing is being flashed, but when the controller has not been
provisioned with an address, the address in the NVM configuration file
is used.

And we need to handle this in some way, as the configuration files are
already out there (e.g. in linux-firmware) and are honoured by the QCA
firmware.

My patch reads out the default address from the configuration file
before downloading it during setup() so that no matter what address is
set this way, it will be treated as non-unique and invalid.

This way we don't need to maintain any table in the kernel and we don't
risk any regressions if the address is ever changed in a later firmware
update.

The only downside is that developers on old platforms that don't have
any user space tools to set a valid address (e.g. btmgmt) cannot set
an address by patching the firmware file.

But I don't think we need to care about that. I assume that in most
cases those developers all just use the default address, with the risk
of collisions that that implies.

We have a standard APIs for configuring the address, just use that.

> ps: If the intention is to have these addresses for testing then these
> firmwares files shall probably be kept private, since as explained
> above the use of duplicated addresses will cause problems to users who
> have no idea they have to be changed.

Johan