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

[Janaki Ramaiah Thota]

Hi Johan and Luiz,

On 4/30/2024 12:37 PM, Johan Hovold wrote:
> 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:
>
> > > 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.

Yes agree with this point.
BD address should be treated as invalid if it is 00:00:00:00:00:00.
NVM Tag 2: bd address is default BD address (other than 0), should be
configured as valid address and as its not unique address and it will
be same for all devices so mark it is configured but still allow
user-space to change the address.

> 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
-Janaki Ram