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
> <[email protected]> wrote:
> > On Mon, Apr 29, 2024 at 10:02 AM Johan Hovold <[email protected]> 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
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
>> <[email protected]> wrote:
>>> On Mon, Apr 29, 2024 at 10:02 AM Johan Hovold <[email protected]> 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
On Tue, Apr 30, 2024 at 06:22:26PM +0530, Janaki Ramaiah Thota wrote:
> 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:
> >> 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.
We all agree on that.
> 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.
But here we disagree. A non-unique address is not a valid one as it will
cause collisions if you have more than one such controller.
I understand that this may be convenient/good enough for developers in
some cases, but this can hurt end users that do not realise why things
break.
And a developer can always configure an address manually or patch the
driver as needed for internal use.
Are there any other reasons that makes you want to keep the option to
configure the device address through NVM files? I'm assuming you're not
relying on patching NVM files to provision device-specific addresses
after installation on target?
Johan
Hi Johan,
On Tue, Apr 30, 2024 at 9:07 AM Johan Hovold <[email protected]> wrote:
>
> On Tue, Apr 30, 2024 at 06:22:26PM +0530, Janaki Ramaiah Thota wrote:
> > 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:
>
> > >> 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.
>
> We all agree on that.
>
> > 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.
>
> But here we disagree. A non-unique address is not a valid one as it will
> cause collisions if you have more than one such controller.
>
> I understand that this may be convenient/good enough for developers in
> some cases, but this can hurt end users that do not realise why things
> break.
>
> And a developer can always configure an address manually or patch the
> driver as needed for internal use.
>
> Are there any other reasons that makes you want to keep the option to
> configure the device address through NVM files? I'm assuming you're not
> relying on patching NVM files to provision device-specific addresses
> after installation on target?
Exactly, a duplicated address is not a valid public/identity address.
Regarding them already been in use, we will need to have it fixed one
way or the other, so it is better to change whatever it comer within
the firmware file to 00:00:00:00:00:00 and have it setup a proper
address after that rather than have a table that detect the use of
duplicated addresses since the result would be the same since
userspace stores pairing/devices based on adapter addresses they will
be lost and the user will need to pair its peripherals again, so my
recommendation is that this is done via firmware update rather than
introducing a table containing duplicate addresses.
That said it seems the patch in this thread actually reads the address
with use of EDL_TAG_ID_BD_ADDR and then proceed to check if that is
what the controller returns as address, while that is better than
having a table I think there is still a risk that the duplicated
address gets used on older kernels if that is not updated in the
firmware directly, anyway perhaps we shall be doing both so we capture
both cases where duplicated addresses are used or when BDADDR_ANY is.
--
Luiz Augusto von Dentz