From: Doug Anderson <dianders@chromium.org>
To: Johan Hovold <johan+linaro@kernel.org>
Cc: Marcel Holtmann <marcel@holtmann.org>,
Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
stable@vger.kernel.org,
Janaki Ramaiah Thota <quic_janathot@quicinc.com>
Subject: Re: [PATCH] Bluetooth: qca: generalise device address check
Date: Fri, 26 Apr 2024 10:23:15 -0700 [thread overview]
Message-ID: <CAD=FV=V-pG9+5fLonNvydmjS=ziUFUHAyF8T7YTkEHiO405aSA@mail.gmail.com> (raw)
In-Reply-To: <20240426155801.25277-1-johan+linaro@kernel.org>
Hi,
On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The default device address apparently comes from the NVM configuration
> file and can differ quite a bit.
>
> Store the default address when parsing the configuration file and use it
> to determine whether the controller has been provisioned with an
> address.
>
> This makes sure that devices without a unique address start as
> unconfigured unless a valid address has been provided in the devicetree.
>
> Fixes: 00567f70051a ("Bluetooth: qca: fix invalid device address check")
> Cc: stable@vger.kernel.org # 6.5
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Janaki Ramaiah Thota <quic_janathot@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> drivers/bluetooth/btqca.c | 21 ++++++++++++---------
> drivers/bluetooth/btqca.h | 2 ++
> 2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index cfa71708397b..d7a6738e4691 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -15,9 +15,6 @@
>
> #define VERSION "0.1"
>
> -#define QCA_BDADDR_DEFAULT (&(bdaddr_t) {{ 0xad, 0x5a, 0x00, 0x00, 0x00, 0x00 }})
> -#define QCA_BDADDR_WCN3991 (&(bdaddr_t) {{ 0xad, 0x5a, 0x00, 0x00, 0x98, 0x39 }})
> -
> int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
> enum qca_btsoc_type soc_type)
> {
> @@ -351,6 +348,11 @@ static void qca_tlv_check_data(struct hci_dev *hdev,
>
> /* Update NVM tags as needed */
> switch (tag_id) {
> + case EDL_TAG_ID_BD_ADDR:
> + if (tag_len != sizeof(bdaddr_t))
> + break;
> + memcpy(&config->bdaddr, tlv_nvm->data, sizeof(bdaddr_t));
> + break;
> case EDL_TAG_ID_HCI:
nit: blank line after "break" ?
Also note that on my firmware I never see this tag and thus your patch
breaks trogdor. Specifically I put a printout here and it never gets
hit.
I printed all the tags/lengths:
[ 17.961087] DOUG: id 0xde02, len 0x0010
[ 17.965081] DOUG: id 0x0000, len 0x0000
[ 17.969050] DOUG: id 0x0000, len 0x0011
[ 17.973025] DOUG: id 0x0000, len 0x0a00
[ 17.976991] DOUG: id 0x0303, len 0x0303
[ 17.981066] DOUG: id 0x0033, len 0x1001
Probably EDL_TAG_ID_BD_ADDR should have been 0xde02, not just 2.
...but then the size is wrong? When I print out the bytes in ID 0xde02
I see the address you're looking for 4 bytes in...
[ 17.663602] DOUG: 0x00
[ 17.666132] DOUG: 0x00
[ 17.668638] DOUG: 0x00
[ 17.671237] DOUG: 0x00
[ 17.673689] DOUG: 0xad
[ 17.676120] DOUG: 0x5a
[ 17.678551] DOUG: 0x00
[ 17.680980] DOUG: 0x00
[ 17.683409] DOUG: 0x98
[ 17.685846] DOUG: 0x39
[ 17.688278] DOUG: 0x08
[ 17.690704] DOUG: 0x00
[ 17.693137] DOUG: 0x08
[ 17.693139] DOUG: 0x00
[ 17.693139] DOUG: 0x00
[ 17.693140] DOUG: 0x00
> @@ -624,6 +626,9 @@ static int qca_check_bdaddr(struct hci_dev *hdev)
> if (bacmp(&hdev->public_addr, BDADDR_ANY))
> return 0;
>
> + if (!bacmp(&config->bdaddr, BDADDR_ANY))
> + return 0;
The above test feels non-obvious enough to deserve a comment. Could
you add one? That would also help alleviate my confusion since I
_think_ your if test is unneeded and maybe wrong? Let's say that the
firmware didn't have a default address stored in it. It still seems
like we could try to read the address and then if the firmware gave
back BDADDR_ANY (0) we should set the `HCI_QUIRK_USE_BDADDR_PROPERTY`
property, right?
next prev parent reply other threads:[~2024-04-26 17:23 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 15:58 [PATCH] Bluetooth: qca: generalise device address check Johan Hovold
2024-04-26 17:23 ` Doug Anderson [this message]
2024-04-27 9:51 ` Johan Hovold
2024-04-29 10:04 ` Janaki Ramaiah Thota
2024-04-29 14:02 ` Johan Hovold
2024-04-29 14:06 ` Johan Hovold
2024-04-29 17:12 ` Luiz Augusto von Dentz
2024-04-29 17:31 ` Luiz Augusto von Dentz
2024-04-30 7:07 ` Johan Hovold
2024-04-30 12:52 ` Janaki Ramaiah Thota
2024-04-30 13:07 ` Johan Hovold
2024-04-30 14:04 ` Luiz Augusto von Dentz
2024-04-30 14:36 ` Johan Hovold
2024-05-02 7:05 ` Janaki Ramaiah Thota
2024-05-02 10:11 ` Johan Hovold
2024-05-02 17:03 ` Janaki Ramaiah Thota
2024-05-02 17:32 ` Luiz Augusto von Dentz
2024-05-03 7:12 ` Janaki Ramaiah Thota
2024-04-30 14:21 ` Luiz Augusto von Dentz
2024-04-30 14:38 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=V-pG9+5fLonNvydmjS=ziUFUHAyF8T7YTkEHiO405aSA@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=johan+linaro@kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=quic_janathot@quicinc.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).