diff mbox series

[v2] Bluetooth: HCI: Fix not always setting Scan Response/Advertising Data

Message ID 20220614174645.518132-1-luiz.dentz@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] Bluetooth: HCI: Fix not always setting Scan Response/Advertising Data | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/subjectprefix success PASS
tedd_an/buildkernel success Build Kernel PASS
tedd_an/buildkernel32 success Build Kernel32 PASS
tedd_an/incremental_build success Pass
tedd_an/testrunnersetup success Test Runner Setup PASS
tedd_an/testrunnerl2cap-tester success Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerbnep-tester success Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnermgmt-tester success Total: 493, Passed: 493 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerrfcomm-tester success Total: 10, Passed: 10 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersco-tester success Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersmp-tester success Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunneruserchan-tester success Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0

Commit Message

Luiz Augusto von Dentz June 14, 2022, 5:46 p.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The scan response and advertising data needs to be tracked on a per
instance (adv_info) since when these instaces are removed so are their
data, to fix that new flags are introduced which is used to mark when
the data changes and then checked to confirm when the data needs to be
synced with the controller.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Mark scan_rsp_changed when either MGMT_ADV_FLAG_APPEARANCE or
MGMT_ADV_FLAG_LOCAL_NAME have been set.

 include/net/bluetooth/hci_core.h | 11 ++++++
 net/bluetooth/hci_core.c         | 42 ++++++++++----------
 net/bluetooth/hci_sync.c         | 66 ++++++++++++++++++++++----------
 3 files changed, 76 insertions(+), 43 deletions(-)

Comments

bluez.test.bot@gmail.com June 14, 2022, 6:07 p.m. UTC | #1 
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=650299

---Test result---

Test Summary:
CheckPatch                    PASS      1.73 seconds
GitLint                       PASS      0.73 seconds
SubjectPrefix                 PASS      0.60 seconds
BuildKernel                   PASS      31.61 seconds
BuildKernel32                 PASS      27.69 seconds
Incremental Build with patchesPASS      36.55 seconds
TestRunner: Setup             PASS      488.05 seconds
TestRunner: l2cap-tester      PASS      18.50 seconds
TestRunner: bnep-tester       PASS      6.18 seconds
TestRunner: mgmt-tester       PASS      102.56 seconds
TestRunner: rfcomm-tester     PASS      9.22 seconds
TestRunner: sco-tester        PASS      9.52 seconds
TestRunner: smp-tester        PASS      9.19 seconds
TestRunner: userchan-tester   PASS      6.08 seconds



---
Regards,
Linux Bluetooth
An, Tedd June 17, 2022, 6:18 a.m. UTC | #2 
Hi Luiz

I tested with my new test case added to mgmt-tester and passed.

On 6/14/22, 10:46 AM, "Luiz Augusto von Dentz" <luiz.dentz@gmail.com> wrote:

    From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

    The scan response and advertising data needs to be tracked on a per
    instance (adv_info) since when these instaces are removed so are their
    data, to fix that new flags are introduced which is used to mark when
    the data changes and then checked to confirm when the data needs to be
    synced with the controller.

Tested-by: Tedd Ho-Jeong An <tedd.an@intel.com>

    Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

    ---
    v2: Mark scan_rsp_changed when either MGMT_ADV_FLAG_APPEARANCE or
    MGMT_ADV_FLAG_LOCAL_NAME have been set.

     include/net/bluetooth/hci_core.h | 11 ++++++
     net/bluetooth/hci_core.c         | 42 ++++++++++----------
     net/bluetooth/hci_sync.c         | 66 ++++++++++++++++++++++----------
     3 files changed, 76 insertions(+), 43 deletions(-)

    diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
    index 5b92a9abe141..15237ee5f761 100644
    --- a/include/net/bluetooth/hci_core.h
    +++ b/include/net/bluetooth/hci_core.h
    @@ -246,8 +246,10 @@ struct adv_info {
     	__u16	duration;
     	__u16	adv_data_len;
     	__u8	adv_data[HCI_MAX_EXT_AD_LENGTH];
    +	bool	adv_data_changed;
     	__u16	scan_rsp_len;
     	__u8	scan_rsp_data[HCI_MAX_EXT_AD_LENGTH];
    +	bool	scan_rsp_changed;
     	__s8	tx_power;
     	__u32   min_interval;
     	__u32   max_interval;
    @@ -261,6 +263,15 @@ struct adv_info {

     #define HCI_ADV_TX_POWER_NO_PREFERENCE 0x7F

    +#define DATA_CMP(_d1, _l1, _d2, _l2) \
    +	(_l1 == _l2 ? memcmp(_d1, _d2, _l1) : _l1 - _l2)
    +
    +#define ADV_DATA_CMP(_adv, _data, _len) \
    +	DATA_CMP((_adv)->adv_data, (_adv)->adv_data_len, _data, _len)
    +
    +#define SCAN_RSP_CMP(_adv, _data, _len) \
    +	DATA_CMP((_adv)->scan_rsp_data, (_adv)->scan_rsp_len, _data, _len)
    +
     struct monitored_device {
     	struct list_head list;

    diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
    index 6faae50d933d..05c13f639b94 100644
    --- a/net/bluetooth/hci_core.c
    +++ b/net/bluetooth/hci_core.c
    @@ -1727,18 +1727,12 @@ int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
     	}

     	adv_instance->flags = flags;
    -	adv_instance->adv_data_len = adv_data_len;
    -	adv_instance->scan_rsp_len = scan_rsp_len;
     	adv_instance->min_interval = min_interval;
     	adv_instance->max_interval = max_interval;
     	adv_instance->tx_power = tx_power;

    -	if (adv_data_len)
    -		memcpy(adv_instance->adv_data, adv_data, adv_data_len);
    -
    -	if (scan_rsp_len)
    -		memcpy(adv_instance->scan_rsp_data,
    -		       scan_rsp_data, scan_rsp_len);
    +	hci_set_adv_instance_data(hdev, instance, adv_data_len, adv_data,
    +				  scan_rsp_len, scan_rsp_data);

     	adv_instance->timeout = timeout;
     	adv_instance->remaining_time = timeout;
    @@ -1761,29 +1755,33 @@ int hci_set_adv_instance_data(struct hci_dev *hdev, u8 instance,
     			      u16 adv_data_len, u8 *adv_data,
     			      u16 scan_rsp_len, u8 *scan_rsp_data)
     {
    -	struct adv_info *adv_instance;
    +	struct adv_info *adv;

    -	adv_instance = hci_find_adv_instance(hdev, instance);
    +	adv = hci_find_adv_instance(hdev, instance);

     	/* If advertisement doesn't exist, we can't modify its data */
    -	if (!adv_instance)
    +	if (!adv)
     		return -ENOENT;

    -	if (adv_data_len) {
    -		memset(adv_instance->adv_data, 0,
    -		       sizeof(adv_instance->adv_data));
    -		memcpy(adv_instance->adv_data, adv_data, adv_data_len);
    -		adv_instance->adv_data_len = adv_data_len;
    +	if (adv_data_len && ADV_DATA_CMP(adv, adv_data, adv_data_len)) {
    +		memset(adv->adv_data, 0, sizeof(adv->adv_data));
    +		memcpy(adv->adv_data, adv_data, adv_data_len);
    +		adv->adv_data_len = adv_data_len;
    +		adv->adv_data_changed = true;
     	}

    -	if (scan_rsp_len) {
    -		memset(adv_instance->scan_rsp_data, 0,
    -		       sizeof(adv_instance->scan_rsp_data));
    -		memcpy(adv_instance->scan_rsp_data,
    -		       scan_rsp_data, scan_rsp_len);
    -		adv_instance->scan_rsp_len = scan_rsp_len;
    +	if (scan_rsp_len && SCAN_RSP_CMP(adv, scan_rsp_data, scan_rsp_len)) {
    +		memset(adv->scan_rsp_data, 0, sizeof(adv->scan_rsp_data));
    +		memcpy(adv->scan_rsp_data, scan_rsp_data, scan_rsp_len);
    +		adv->scan_rsp_len = scan_rsp_len;
    +		adv->scan_rsp_changed = true;
     	}

    +	/* Mark as changed if there are flags which would affect it */
    +	if (((adv->flags & MGMT_ADV_FLAG_APPEARANCE) && hdev->appearance) ||
    +	    adv->flags & MGMT_ADV_FLAG_LOCAL_NAME)
    +		adv->scan_rsp_changed = true;
    +
     	return 0;
     }

    diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
    index 0fa013816a9b..e974a888c0f3 100644
    --- a/net/bluetooth/hci_sync.c
    +++ b/net/bluetooth/hci_sync.c
    @@ -849,26 +849,38 @@ static int hci_set_ext_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance)
     		u8 data[HCI_MAX_EXT_AD_LENGTH];
     	} pdu;
     	u8 len;
    +	struct adv_info *adv = NULL;
    +	int err;

     	memset(&pdu, 0, sizeof(pdu));

    +	if (instance) {
    +		adv = hci_find_adv_instance(hdev, instance);
    +		if (!adv || !adv->scan_rsp_changed)
    +			return 0;
    +	}
    +
     	len = eir_create_scan_rsp(hdev, instance, pdu.data);

    -	if (hdev->scan_rsp_data_len == len &&
    -	    !memcmp(pdu.data, hdev->scan_rsp_data, len))
    -		return 0;
    -
    -	memcpy(hdev->scan_rsp_data, pdu.data, len);
    -	hdev->scan_rsp_data_len = len;
    -
     	pdu.cp.handle = instance;
     	pdu.cp.length = len;
     	pdu.cp.operation = LE_SET_ADV_DATA_OP_COMPLETE;
     	pdu.cp.frag_pref = LE_SET_ADV_DATA_NO_FRAG;

    -	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_SCAN_RSP_DATA,
    -				     sizeof(pdu.cp) + len, &pdu.cp,
    -				     HCI_CMD_TIMEOUT);
    +	err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_SCAN_RSP_DATA,
    +				    sizeof(pdu.cp) + len, &pdu.cp,
    +				    HCI_CMD_TIMEOUT);
    +	if (err)
    +		return err;
    +
    +	if (adv) {
    +		adv->scan_rsp_changed = false;
    +	} else {
    +		memcpy(hdev->scan_rsp_data, pdu.data, len);
    +		hdev->scan_rsp_data_len = len;
    +	}
    +
    +	return 0;
     }

     static int __hci_set_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance)
    @@ -1119,27 +1131,39 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
     		u8 data[HCI_MAX_EXT_AD_LENGTH];
     	} pdu;
     	u8 len;
    +	struct adv_info *adv = NULL;
    +	int err;

     	memset(&pdu, 0, sizeof(pdu));

    +	if (instance) {
    +		adv = hci_find_adv_instance(hdev, instance);
    +		if (!adv || !adv->adv_data_changed)
    +			return 0;
    +	}
    +
     	len = eir_create_adv_data(hdev, instance, pdu.data);

    -	/* There's nothing to do if the data hasn't changed */
    -	if (hdev->adv_data_len == len &&
    -	    memcmp(pdu.data, hdev->adv_data, len) == 0)
    -		return 0;
    -
    -	memcpy(hdev->adv_data, pdu.data, len);
    -	hdev->adv_data_len = len;
    -
     	pdu.cp.length = len;
     	pdu.cp.handle = instance;
     	pdu.cp.operation = LE_SET_ADV_DATA_OP_COMPLETE;
     	pdu.cp.frag_pref = LE_SET_ADV_DATA_NO_FRAG;

    -	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
    -				     sizeof(pdu.cp) + len, &pdu.cp,
    -				     HCI_CMD_TIMEOUT);
    +	err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
    +				    sizeof(pdu.cp) + len, &pdu.cp,
    +				    HCI_CMD_TIMEOUT);
    +	if (err)
    +		return err;
    +
    +	/* Update data if the command succeed */
    +	if (adv) {
    +		adv->adv_data_changed = false;
    +	} else {
    +		memcpy(hdev->adv_data, pdu.data, len);
    +		hdev->adv_data_len = len;
    +	}
    +
    +	return 0;
     }

     static int hci_set_adv_data_sync(struct hci_dev *hdev, u8 instance)
    -- 
    2.35.3

Regards,
Tedd Ho-Jeong An
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5b92a9abe141..15237ee5f761 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -246,8 +246,10 @@  struct adv_info {
 	__u16	duration;
 	__u16	adv_data_len;
 	__u8	adv_data[HCI_MAX_EXT_AD_LENGTH];
+	bool	adv_data_changed;
 	__u16	scan_rsp_len;
 	__u8	scan_rsp_data[HCI_MAX_EXT_AD_LENGTH];
+	bool	scan_rsp_changed;
 	__s8	tx_power;
 	__u32   min_interval;
 	__u32   max_interval;
@@ -261,6 +263,15 @@  struct adv_info {
 
 #define HCI_ADV_TX_POWER_NO_PREFERENCE 0x7F
 
+#define DATA_CMP(_d1, _l1, _d2, _l2) \
+	(_l1 == _l2 ? memcmp(_d1, _d2, _l1) : _l1 - _l2)
+
+#define ADV_DATA_CMP(_adv, _data, _len) \
+	DATA_CMP((_adv)->adv_data, (_adv)->adv_data_len, _data, _len)
+
+#define SCAN_RSP_CMP(_adv, _data, _len) \
+	DATA_CMP((_adv)->scan_rsp_data, (_adv)->scan_rsp_len, _data, _len)
+
 struct monitored_device {
 	struct list_head list;
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 6faae50d933d..05c13f639b94 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1727,18 +1727,12 @@  int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
 	}
 
 	adv_instance->flags = flags;
-	adv_instance->adv_data_len = adv_data_len;
-	adv_instance->scan_rsp_len = scan_rsp_len;
 	adv_instance->min_interval = min_interval;
 	adv_instance->max_interval = max_interval;
 	adv_instance->tx_power = tx_power;
 
-	if (adv_data_len)
-		memcpy(adv_instance->adv_data, adv_data, adv_data_len);
-
-	if (scan_rsp_len)
-		memcpy(adv_instance->scan_rsp_data,
-		       scan_rsp_data, scan_rsp_len);
+	hci_set_adv_instance_data(hdev, instance, adv_data_len, adv_data,
+				  scan_rsp_len, scan_rsp_data);
 
 	adv_instance->timeout = timeout;
 	adv_instance->remaining_time = timeout;
@@ -1761,29 +1755,33 @@  int hci_set_adv_instance_data(struct hci_dev *hdev, u8 instance,
 			      u16 adv_data_len, u8 *adv_data,
 			      u16 scan_rsp_len, u8 *scan_rsp_data)
 {
-	struct adv_info *adv_instance;
+	struct adv_info *adv;
 
-	adv_instance = hci_find_adv_instance(hdev, instance);
+	adv = hci_find_adv_instance(hdev, instance);
 
 	/* If advertisement doesn't exist, we can't modify its data */
-	if (!adv_instance)
+	if (!adv)
 		return -ENOENT;
 
-	if (adv_data_len) {
-		memset(adv_instance->adv_data, 0,
-		       sizeof(adv_instance->adv_data));
-		memcpy(adv_instance->adv_data, adv_data, adv_data_len);
-		adv_instance->adv_data_len = adv_data_len;
+	if (adv_data_len && ADV_DATA_CMP(adv, adv_data, adv_data_len)) {
+		memset(adv->adv_data, 0, sizeof(adv->adv_data));
+		memcpy(adv->adv_data, adv_data, adv_data_len);
+		adv->adv_data_len = adv_data_len;
+		adv->adv_data_changed = true;
 	}
 
-	if (scan_rsp_len) {
-		memset(adv_instance->scan_rsp_data, 0,
-		       sizeof(adv_instance->scan_rsp_data));
-		memcpy(adv_instance->scan_rsp_data,
-		       scan_rsp_data, scan_rsp_len);
-		adv_instance->scan_rsp_len = scan_rsp_len;
+	if (scan_rsp_len && SCAN_RSP_CMP(adv, scan_rsp_data, scan_rsp_len)) {
+		memset(adv->scan_rsp_data, 0, sizeof(adv->scan_rsp_data));
+		memcpy(adv->scan_rsp_data, scan_rsp_data, scan_rsp_len);
+		adv->scan_rsp_len = scan_rsp_len;
+		adv->scan_rsp_changed = true;
 	}
 
+	/* Mark as changed if there are flags which would affect it */
+	if (((adv->flags & MGMT_ADV_FLAG_APPEARANCE) && hdev->appearance) ||
+	    adv->flags & MGMT_ADV_FLAG_LOCAL_NAME)
+		adv->scan_rsp_changed = true;
+
 	return 0;
 }
 
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 0fa013816a9b..e974a888c0f3 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -849,26 +849,38 @@  static int hci_set_ext_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance)
 		u8 data[HCI_MAX_EXT_AD_LENGTH];
 	} pdu;
 	u8 len;
+	struct adv_info *adv = NULL;
+	int err;
 
 	memset(&pdu, 0, sizeof(pdu));
 
+	if (instance) {
+		adv = hci_find_adv_instance(hdev, instance);
+		if (!adv || !adv->scan_rsp_changed)
+			return 0;
+	}
+
 	len = eir_create_scan_rsp(hdev, instance, pdu.data);
 
-	if (hdev->scan_rsp_data_len == len &&
-	    !memcmp(pdu.data, hdev->scan_rsp_data, len))
-		return 0;
-
-	memcpy(hdev->scan_rsp_data, pdu.data, len);
-	hdev->scan_rsp_data_len = len;
-
 	pdu.cp.handle = instance;
 	pdu.cp.length = len;
 	pdu.cp.operation = LE_SET_ADV_DATA_OP_COMPLETE;
 	pdu.cp.frag_pref = LE_SET_ADV_DATA_NO_FRAG;
 
-	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_SCAN_RSP_DATA,
-				     sizeof(pdu.cp) + len, &pdu.cp,
-				     HCI_CMD_TIMEOUT);
+	err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_SCAN_RSP_DATA,
+				    sizeof(pdu.cp) + len, &pdu.cp,
+				    HCI_CMD_TIMEOUT);
+	if (err)
+		return err;
+
+	if (adv) {
+		adv->scan_rsp_changed = false;
+	} else {
+		memcpy(hdev->scan_rsp_data, pdu.data, len);
+		hdev->scan_rsp_data_len = len;
+	}
+
+	return 0;
 }
 
 static int __hci_set_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance)
@@ -1119,27 +1131,39 @@  static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance)
 		u8 data[HCI_MAX_EXT_AD_LENGTH];
 	} pdu;
 	u8 len;
+	struct adv_info *adv = NULL;
+	int err;
 
 	memset(&pdu, 0, sizeof(pdu));
 
+	if (instance) {
+		adv = hci_find_adv_instance(hdev, instance);
+		if (!adv || !adv->adv_data_changed)
+			return 0;
+	}
+
 	len = eir_create_adv_data(hdev, instance, pdu.data);
 
-	/* There's nothing to do if the data hasn't changed */
-	if (hdev->adv_data_len == len &&
-	    memcmp(pdu.data, hdev->adv_data, len) == 0)
-		return 0;
-
-	memcpy(hdev->adv_data, pdu.data, len);
-	hdev->adv_data_len = len;
-
 	pdu.cp.length = len;
 	pdu.cp.handle = instance;
 	pdu.cp.operation = LE_SET_ADV_DATA_OP_COMPLETE;
 	pdu.cp.frag_pref = LE_SET_ADV_DATA_NO_FRAG;
 
-	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
-				     sizeof(pdu.cp) + len, &pdu.cp,
-				     HCI_CMD_TIMEOUT);
+	err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA,
+				    sizeof(pdu.cp) + len, &pdu.cp,
+				    HCI_CMD_TIMEOUT);
+	if (err)
+		return err;
+
+	/* Update data if the command succeed */
+	if (adv) {
+		adv->adv_data_changed = false;
+	} else {
+		memcpy(hdev->adv_data, pdu.data, len);
+		hdev->adv_data_len = len;
+	}
+
+	return 0;
 }
 
 static int hci_set_adv_data_sync(struct hci_dev *hdev, u8 instance)