| Message ID | 20240319152926.1288-4-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: qca: fix device-address endianness | expand |
Hi, 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. 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. -Doug
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
Hi, On Tue, Mar 19, 2024 at 9:38 AM Johan Hovold <johan@kernel.org> wrote: > > 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. I guess I have a different opinion on the matter. I often end up cherry-picking stuff to older branches and I generally assume that it's relatively safe to pick the beginning of a series without picking later patches because I assume everyone has a goal of bisectability. This breaks that assumption. IMO splitting up the Qualcomm Bluetooth patch into two patches doesn't help enough with clarity to justify. > > 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. Sure, that's your opinion and if the BT folks agree with you then they are free to land the patches without my Reviewed-by on them. ;-) Mine is not a strong Nak but I feel strongly enough that I'd prefer not to have my Reviewed-by added without the re-organization. -Doug
On Tue, Mar 19, 2024 at 10:12:25AM -0700, Doug Anderson wrote: > On Tue, Mar 19, 2024 at 9:38 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Mar 19, 2024 at 09:10:38AM -0700, Doug Anderson wrote: > > > 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. > > I guess I have a different opinion on the matter. I often end up > cherry-picking stuff to older branches and I generally assume that > it's relatively safe to pick the beginning of a series without picking > later patches because I assume everyone has a goal of bisectability. > This breaks that assumption. IMO splitting up the Qualcomm Bluetooth > patch into two patches doesn't help enough with clarity to justify. I did that in v2 because then the two patches had to be split to facilitate backporting as wcn3991 support was added later. But the big issue here is taking the patches through different trees. If Bjorn could ack the DT patch so that everything goes through the Bluetooth tree, then I guess I can reorder the DT patch and squash the two driver patches. But waiting several weeks just to make sure that the DT patch hits mainline (and the binding patch before that?) before the driver fixes can go in just does not seem worth it 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. > > Sure, that's your opinion and if the BT folks agree with you then they > are free to land the patches without my Reviewed-by on them. ;-) Mine > is not a strong Nak but I feel strongly enough that I'd prefer not to > have my Reviewed-by added without the re-organization. Of course, understood. Johan
Hi, On Tue, Mar 19, 2024 at 10:28 AM Johan Hovold <johan@kernel.org> wrote: > > > I guess I have a different opinion on the matter. I often end up > > cherry-picking stuff to older branches and I generally assume that > > it's relatively safe to pick the beginning of a series without picking > > later patches because I assume everyone has a goal of bisectability. > > This breaks that assumption. IMO splitting up the Qualcomm Bluetooth > > patch into two patches doesn't help enough with clarity to justify. > > I did that in v2 because then the two patches had to be split to > facilitate backporting as wcn3991 support was added later. > > But the big issue here is taking the patches through different trees. If > Bjorn could ack the DT patch so that everything goes through the > Bluetooth tree, then I guess I can reorder the DT patch and squash the > two driver patches. > > But waiting several weeks just to make sure that the DT patch hits > mainline (and the binding patch before that?) before the driver fixes > can go in just does not seem worth it to me. Personally, I don't care quite as much about them going through the same tree. It'd be nice, but I agree with you that it's probably not worth the hassle (though I wouldn't object if Bjorn wanted to Ack the dts) and it's fine with me if the patches "meet up" in mainline. In my case, though, I could imagine following the "Link" tag in the patches and arriving at the mailing list post. That's where I'd go back and look to see the order which I should apply the patches safely. ...and I'd prefer that it shows an order that lets things apply safely. -Doug
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c index b40b32fa7f1c..19cfc342fc7b 100644 --- a/drivers/bluetooth/btqca.c +++ b/drivers/bluetooth/btqca.c @@ -826,11 +826,15 @@ EXPORT_SYMBOL_GPL(qca_uart_setup); int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr) { + bdaddr_t bdaddr_swapped; struct sk_buff *skb; int err; - skb = __hci_cmd_sync_ev(hdev, EDL_WRITE_BD_ADDR_OPCODE, 6, bdaddr, - HCI_EV_VENDOR, HCI_INIT_TIMEOUT); + baswap(&bdaddr_swapped, bdaddr); + + skb = __hci_cmd_sync_ev(hdev, EDL_WRITE_BD_ADDR_OPCODE, 6, + &bdaddr_swapped, HCI_EV_VENDOR, + HCI_INIT_TIMEOUT); if (IS_ERR(skb)) { err = PTR_ERR(skb); bt_dev_err(hdev, "QCA Change address cmd failed (%d)", err);
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(-)