Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-2631-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 19 Mar 2024 17:38:56 +0100
From: Johan Hovold <johan@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Johan Hovold <johan+linaro@kernel.org>,
	Marcel Holtmann <marcel@holtmann.org>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
	Bjorn Andersson <andersson@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	cros-qcom-dts-watchers@chromium.org,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	Balakrishna Godavarthi <quic_bgodavar@quicinc.com>,
	Matthias Kaehlcke <mka@chromium.org>,
	Rocky Liao <quic_rjliao@quicinc.com>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org,
	Nikita Travkin <nikita@trvn.ru>
Subject: Re: [PATCH v3 3/5] Bluetooth: qca: fix device-address endianness
Message-ID: <Zfm_oFLNgPHqJKtG@hovoldconsulting.com>
References: <20240319152926.1288-1-johan+linaro@kernel.org>
 <20240319152926.1288-4-johan+linaro@kernel.org>
 <CAD=FV=WqwY07fMV-TuO8QMRnk555BJYEysv4urcugsELufHr4A@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: <CAD=FV=WqwY07fMV-TuO8QMRnk555BJYEysv4urcugsELufHr4A@mail.gmail.com>

On Tue, Mar 19, 2024 at 09:10:38AM -0700, Doug Anderson wrote:
> On Tue, Mar 19, 2024 at 8:30 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > The WCN6855 firmware on the Lenovo ThinkPad X13s expects the Bluetooth
> > device address in big-endian order when setting it using the
> > EDL_WRITE_BD_ADDR_OPCODE command.
> >
> > Presumably, this is the case for all non-ROME devices which all use the
> > EDL_WRITE_BD_ADDR_OPCODE command for this (unlike the ROME devices which
> > use a different command and expect the address in little-endian order).
> >
> > Reverse the little-endian address before setting it to make sure that
> > the address can be configured using tools like btmgmt or using the
> > 'local-bd-address' devicetree property.
> >
> > Note that this can potentially break systems with boot firmware which
> > has started relying on the broken behaviour and is incorrectly passing
> > the address via devicetree in big-endian order.
> >
> > Fixes: 5c0a1001c8be ("Bluetooth: hci_qca: Add helper to set device address")
> > Cc: stable@vger.kernel.org      # 5.1
> > Cc: Balakrishna Godavarthi <quic_bgodavar@quicinc.com>
> > Cc: Matthias Kaehlcke <mka@chromium.org>
> > Tested-by: Nikita Travkin <nikita@trvn.ru> # sc7180
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/bluetooth/btqca.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> Personally, I'd prefer it if you didn't break bisectability with your
> series. As it is, if someone applies just the first 3 patches they'll
> end up with broken Bluetooth.

It doesn't break the build, but yes, the device address would be
reversed for Trogdor machines for two commits and possible break any
previous pairings. That's hardly something to worry about.

So I consider this to be acceptable for sake of clarity, and especially
since these patches will be coming in from separate trees anyway.

> IMO the order should be:
> 1. Binding (currently patch #1)
> 2. Trogdor dt patch, which won't hurt on its own (currently patch #5)
> 3. Bluetooth subsystem patch handling the quirk (currently patch #2)
> 4. Qualcomm change to fix the endianness and handle the quirk squashed
> into 1 patch (currently patch #3 + #4)
> 
> ..and the patch that changes the Qualcomm driver should make it
> obvious that it depends on the trogdor DT patch in the change
> description.
> 
> With patches #3 and #4 combined, feel free to add my Reviewed-by tag
> as both patches look fine to me.

I don't think it's worth spending more time and effort on this issue
(which should have been caught and fixed years ago) for this.

Johan