Date: Tue, 30 Apr 2024 10:04:05 -0400
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Johan Hovold <johan@...nel.org>
Cc: Janaki Ramaiah Thota <quic_janathot@...cinc.com>, Doug Anderson <dianders@...omium.org>, 
	Johan Hovold <johan+linaro@...nel.org>, Marcel Holtmann <marcel@...tmann.org>, 
	linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org, quic_mohamull@...cinc.com, quic_hbandi@...cinc.com, 
	quic_anubhavg@...cinc.com
Subject: Re: [PATCH] Bluetooth: qca: generalise device address check

Hi Johan,

On Tue, Apr 30, 2024 at 9:07 AM Johan Hovold <johan@...nel.org> 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

Powered by blists - more mailing lists