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
