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: [email protected] # 6.5
Cc: Doug Anderson <[email protected]>
Cc: Janaki Ramaiah Thota <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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:
/* HCI transport layer parameters
* enabling software inband sleep
@@ -615,7 +617,7 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
}
EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
-static int qca_check_bdaddr(struct hci_dev *hdev)
+static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *config)
{
struct hci_rp_read_bd_addr *bda;
struct sk_buff *skb;
@@ -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;
+
skb = __hci_cmd_sync(hdev, HCI_OP_READ_BD_ADDR, 0, NULL,
HCI_INIT_TIMEOUT);
if (IS_ERR(skb)) {
@@ -639,10 +644,8 @@ static int qca_check_bdaddr(struct hci_dev *hdev)
}
bda = (struct hci_rp_read_bd_addr *)skb->data;
- if (!bacmp(&bda->bdaddr, QCA_BDADDR_DEFAULT) ||
- !bacmp(&bda->bdaddr, QCA_BDADDR_WCN3991)) {
+ if (!bacmp(&bda->bdaddr, &config->bdaddr))
set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
- }
kfree_skb(skb);
@@ -670,7 +673,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
const char *firmware_name)
{
- struct qca_fw_config config;
+ struct qca_fw_config config = {};
int err;
u8 rom_ver = 0;
u32 soc_ver;
@@ -855,7 +858,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
break;
}
- err = qca_check_bdaddr(hdev);
+ err = qca_check_bdaddr(hdev, &config);
if (err)
return err;
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index dc31984f71dc..49ad668d0d0b 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -29,6 +29,7 @@
#define EDL_PATCH_CONFIG_RES_EVT (0x00)
#define QCA_DISABLE_LOGGING_SUB_OP (0x14)
+#define EDL_TAG_ID_BD_ADDR 2
#define EDL_TAG_ID_HCI (17)
#define EDL_TAG_ID_DEEP_SLEEP (27)
@@ -94,6 +95,7 @@ struct qca_fw_config {
uint8_t user_baud_rate;
enum qca_tlv_dnld_mode dnld_mode;
enum qca_tlv_dnld_mode dnld_type;
+ bdaddr_t bdaddr;
};
struct edl_event_hdr {
--
2.43.2
Hi,
On Fri, Apr 26, 2024 at 9:00 AM Johan Hovold <[email protected]> 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: [email protected] # 6.5
> Cc: Doug Anderson <[email protected]>
> Cc: Janaki Ramaiah Thota <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> 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?
Hi Janaki,
Please avoid top and remember to trim unnecessary context when replying
to the mailing lists.
On Mon, Apr 29, 2024 at 03:34:32PM +0530, Janaki Ramaiah Thota wrote:
> Having a default BDA list from NVM BDA tag value will prevent developers
> from using the device if there is no user space app(In Fluoride) to set
> the BDA. Therefore, we are requesting to use default address check patch,
> so that developer can change the NVM BDA to make use of the device.
But a developer on such an old platform that can patch and replace the
NVM configuration file should also be able to just disable the check in
the driver right (e.g. by commenting out the call to
qca_check_bdaddr())?
> List Of default Addresses:
> ---------------------------------------------------------
> | BDA | Chipset |
> ---------------------------------------------------------
> | 39 80 10 00 00 20 | WCN3988 with ROM Version 0x0200 |
> ---------------------------------------------------------
> | 39 80 12 74 08 00 | WCN3988 with ROM Version 0x0201 |
> ---------------------------------------------------------
> | 39 90 21 64 07 00 | WCN3990 |
> ---------------------------------------------------------
> | 39 98 00 00 5A AD | WCN3991 |
> ---------------------------------------------------------
> | 00 00 00 00 5A AD | QCA DEFAULT |
> ---------------------------------------------------------
What about WCN6750 and 64:90:00:00:5a:ad?
And then there's currently also:
> > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin)
> > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin)
Which controllers use these configurations?
Johan
Hi,
On Mon, Apr 29, 2024 at 1:12 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi,
>
> On Mon, Apr 29, 2024 at 10:02 AM Johan Hovold <[email protected]> wrote:
> >
> > Hi Janaki,
> >
> > Please avoid top and remember to trim unnecessary context when replying
> > to the mailing lists.
> >
> > On Mon, Apr 29, 2024 at 03:34:32PM +0530, Janaki Ramaiah Thota wrote:
> >
> > > Having a default BDA list from NVM BDA tag value will prevent developers
> > > from using the device if there is no user space app(In Fluoride) to set
> > > the BDA. Therefore, we are requesting to use default address check patch,
> > > so that developer can change the NVM BDA to make use of the device.
> >
> > But a developer on such an old platform that can patch and replace the
> > NVM configuration file should also be able to just disable the check in
> > the driver right (e.g. by commenting out the call to
> > qca_check_bdaddr())?
> >
> > > List Of default Addresses:
> > > ---------------------------------------------------------
> > > | BDA | Chipset |
> > > ---------------------------------------------------------
> > > | 39 80 10 00 00 20 | WCN3988 with ROM Version 0x0200 |
> > > ---------------------------------------------------------
> > > | 39 80 12 74 08 00 | WCN3988 with ROM Version 0x0201 |
> > > ---------------------------------------------------------
> > > | 39 90 21 64 07 00 | WCN3990 |
> > > ---------------------------------------------------------
> > > | 39 98 00 00 5A AD | WCN3991 |
> > > ---------------------------------------------------------
> > > | 00 00 00 00 5A AD | QCA DEFAULT |
> > > ---------------------------------------------------------
> >
> > What about WCN6750 and 64:90:00:00:5a:ad?
> >
> > And then there's currently also:
> >
> > > > bluetooth hci0: bd_addr = 61:47:aa:31:22:14 (qca/nvm_00130300.bin)
> > > > bluetooth hci0: bd_addr = 61:47:aa:32:44:07 (qca/nvm_00130302.bin)
> >
> > Which controllers use these configurations?
>
> These are not unique addresses though, we can't just have addresses by
> chipset address mapping logic as that would cause address clashes over
> the air, e.g. if there are other devices with the same chipset in the
> vicinity.
I see where this is going now, the firmware actually contain these
duplicated addresses which then are checked and cause
HCI_QUIRK_USE_BDADDR_PROPERTY then the tries
hci_dev_get_bd_addr_from_property which loads the local-bd-address
property from the parente device (SOC?), btw that could also have an
invalid/duplicated address.
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.
ps: If the intention is to have these addresses for testing then these
firmwares files shall probably be kept private, since as explained
above the use of duplicated addresses will cause problems to users who
have no idea they have to be changed.
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
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 <[email protected]> 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
On Tue, Apr 30, 2024 at 10:21:37AM -0400, Luiz Augusto von Dentz wrote:
> On Sat, Apr 27, 2024 at 5:51 AM Johan Hovold <[email protected]> wrote:
> > You're right. I'll drop this check when revisiting this next week.
>
> I assume you will spin another version then?
Yes. I have prepared a v2 series. I was just waiting a bit to see where
the discussion in this thread went.
Johan
On 4/30/2024 6:37 PM, Johan Hovold 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?
>
We prefer unique address to be flashed on OTP (persistent) memory of
BT-Chip, which is supported by almost all QC BT-chips. If someone is
not able to do that/ does not prefer that, they still have an option
to flash unique address in firmware binary (NVM)file. This does not
require setting BD address from user space.
Also until a developer flashes OTP/ keep unique BD-Address in NVM,
he should be able to run most of the use cases from Device, that's
why we want to make it as configured.
In our opinion this provides best Out of box experience.
> Johan
-Janaki Ram