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
