Received-SPF: pass (google.com: domain of linux-kernel+bounces-164177-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 30 Apr 2024 16:36:31 +0200
From: Johan Hovold <johan@kernel.org>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: Janaki Ramaiah Thota <quic_janathot@quicinc.com>,
	Doug Anderson <dianders@chromium.org>,
	Johan Hovold <johan+linaro@kernel.org>,
	Marcel Holtmann <marcel@holtmann.org>,
	linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org, quic_mohamull@quicinc.com,
	quic_hbandi@quicinc.com, quic_anubhavg@quicinc.com
Subject: Re: [PATCH] Bluetooth: qca: generalise device address check
Message-ID: <ZjEB75qDjoOb9F_3@hovoldconsulting.com>
References: <CAD=FV=V-pG9+5fLonNvydmjS=ziUFUHAyF8T7YTkEHiO405aSA@mail.gmail.com>
 <ZizKmtcUIYAMpvOQ@hovoldconsulting.com>
 <dbba45d2-f955-4d3a-aeab-26b0900d5823@quicinc.com>
 <Zi-ohCWv58d2h5VM@hovoldconsulting.com>
 <CABBYNZJyqrNKebwPPPqjOAdrkpBJ0fqHyD2iVtypeQKCDcL+AQ@mail.gmail.com>
 <CABBYNZJyRR9FA7TYN4+aWMtG9FPUBWMvCtMNUfvaEzxVcYOt-g@mail.gmail.com>
 <ZjCYu2pc8376rjXk@hovoldconsulting.com>
 <9eebd77b-c070-4260-a979-9b97f14eb5b1@quicinc.com>
 <ZjDtDRCHT3z-3nHh@hovoldconsulting.com>
 <CABBYNZLCw08oo+RRWkBYAdBLhFK5+pQi59dz-f+P1QusfYoAAw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CABBYNZLCw08oo+RRWkBYAdBLhFK5+pQi59dz-f+P1QusfYoAAw@mail.gmail.com>

On Tue, Apr 30, 2024 at 10:04:05AM -0400, Luiz Augusto von Dentz wrote:
> On Tue, Apr 30, 2024 at 9:07 AM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Apr 30, 2024 at 06:22:26PM +0530, Janaki Ramaiah Thota wrote:

> > > 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.

I'm not sure I fully understand you here. I agree that we should avoid
the table if we can, but as you noted below this is what the patch in
this thread does.

And the firmware comes from Qualcomm which pushes it directly to
linux-firmware so we can't control what they decide to put in these
files.

Perhaps the driver can clear the BD_ADDR tag instead of reading it back,
but yes, the end result would be the same in case the firmware can
handle that. May be better to just read out the address as this patch
does to be sure.

> 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.

Not sure we need to care too much about older kernels here. The patches
are currently marked for backport to 6.5 and that could serve as a
cut-off point, but I guess there is nothing preventing us from
backporting this to all stable trees if you prefer that.

Johan