All of lore.kernel.org
 help / color / mirror / Atom feed
* [Bluez PATCH 1/3] Listen and process remote name resolving failure
@ 2021-11-11 11:54 Archie Pusaka
  2021-11-11 11:54 ` [Bluez PATCH 2/3] device: Save remote name request attempts into cache file Archie Pusaka
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Archie Pusaka @ 2021-11-11 11:54 UTC (permalink / raw)
  To: linux-bluetooth, Luiz Augusto von Dentz, Marcel Holtmann
  Cc: CrosBT Upstreaming, Archie Pusaka, Sonny Sasaka, Miao-chen Chou

From: Archie Pusaka <apusaka@chromium.org>

When Remote Name Resolve ends with failure, record this occurrence and
prevent remote name resolving for the same device for some time.
Increase the time duration for subsequent failures.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---
Hi maintainers,

This is the patch series for remote name request as was discussed here.
https://patchwork.kernel.org/project/bluetooth/patch/20211028191805.1.I35b7f3a496f834de6b43a32f94b6160cb1467c94@changeid/
Please also review the corresponding kernel space change.

 lib/mgmt.h     |  7 ++++++-
 src/adapter.c  | 30 ++++++++++++++++++++++++------
 src/device.c   | 23 +++++++++++++++++++++++
 src/device.h   |  2 ++
 tools/btmgmt.c |  4 ++--
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index 0d1678f01d..c006793d84 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -299,10 +299,14 @@ struct mgmt_cp_stop_discovery {
 	uint8_t type;
 } __packed;
 
+#define MGMT_CONFIRM_NAME_UNKNOWN	0
+#define MGMT_CONFIRM_NAME_KNOWN		1
+#define MGMT_CONFIRM_NAME_DONT_CARE	2
+
 #define MGMT_OP_CONFIRM_NAME		0x0025
 struct mgmt_cp_confirm_name {
 	struct mgmt_addr_info addr;
-	uint8_t name_known;
+	uint8_t name_state;
 } __packed;
 struct mgmt_rp_confirm_name {
 	struct mgmt_addr_info addr;
@@ -856,6 +860,7 @@ struct mgmt_ev_auth_failed {
 #define MGMT_DEV_FOUND_CONFIRM_NAME	0x01
 #define MGMT_DEV_FOUND_LEGACY_PAIRING	0x02
 #define MGMT_DEV_FOUND_NOT_CONNECTABLE	0x04
+#define MGMT_DEV_FOUND_NAME_RESOLVE_FAIL 0x10
 
 #define MGMT_EV_DEVICE_FOUND		0x0012
 struct mgmt_ev_device_found {
diff --git a/src/adapter.c b/src/adapter.c
index d0d38621b8..9a43ca4ca5 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -6796,7 +6796,8 @@ static void confirm_name_complete(uint8_t status, uint16_t length,
 }
 
 static void confirm_name(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
-					uint8_t bdaddr_type, bool name_known)
+					uint8_t bdaddr_type, bool name_known,
+					bool name_resolve_allowed)
 {
 	struct mgmt_cp_confirm_name cp;
 	char addr[18];
@@ -6827,7 +6828,13 @@ static void confirm_name(struct btd_adapter *adapter, const bdaddr_t *bdaddr,
 	memset(&cp, 0, sizeof(cp));
 	bacpy(&cp.addr.bdaddr, bdaddr);
 	cp.addr.type = bdaddr_type;
-	cp.name_known = name_known;
+
+	if (name_known)
+		cp.name_state = MGMT_CONFIRM_NAME_KNOWN;
+	else if (name_resolve_allowed)
+		cp.name_state = MGMT_CONFIRM_NAME_UNKNOWN;
+	else
+		cp.name_state = MGMT_CONFIRM_NAME_DONT_CARE;
 
 	adapter->confirm_name_id = mgmt_reply(adapter->mgmt,
 					MGMT_OP_CONFIRM_NAME,
@@ -6989,12 +6996,13 @@ static void update_found_devices(struct btd_adapter *adapter,
 					uint8_t bdaddr_type, int8_t rssi,
 					bool confirm, bool legacy,
 					bool not_connectable,
+					bool name_resolve_fail,
 					const uint8_t *data, uint8_t data_len)
 {
 	struct btd_device *dev;
 	struct bt_ad *ad = NULL;
 	struct eir_data eir_data;
-	bool name_known, discoverable;
+	bool name_known, discoverable, name_resolve_allowed;
 	char addr[18];
 	bool duplicate = false;
 	struct queue *matched_monitors = NULL;
@@ -7081,6 +7089,9 @@ static void update_found_devices(struct btd_adapter *adapter,
 
 	device_set_legacy(dev, legacy);
 
+	if (name_resolve_fail)
+		device_name_resolve_fail(dev);
+
 	if (adapter->filtered_discovery)
 		device_set_rssi_with_delta(dev, rssi, 0);
 	else
@@ -7151,8 +7162,11 @@ static void update_found_devices(struct btd_adapter *adapter,
 	if (g_slist_find(adapter->discovery_found, dev))
 		return;
 
-	if (confirm)
-		confirm_name(adapter, bdaddr, bdaddr_type, name_known);
+	if (confirm) {
+		name_resolve_allowed = device_name_resolve_allowed(dev);
+		confirm_name(adapter, bdaddr, bdaddr_type, name_known,
+							name_resolve_allowed);
+	}
 
 	adapter->discovery_found = g_slist_prepend(adapter->discovery_found,
 									dev);
@@ -7201,6 +7215,8 @@ static void device_found_callback(uint16_t index, uint16_t length,
 	uint32_t flags;
 	bool confirm_name;
 	bool legacy;
+	bool not_connectable;
+	bool name_resolve_fail;
 	char addr[18];
 
 	if (length < sizeof(*ev)) {
@@ -7230,10 +7246,12 @@ static void device_found_callback(uint16_t index, uint16_t length,
 
 	confirm_name = (flags & MGMT_DEV_FOUND_CONFIRM_NAME);
 	legacy = (flags & MGMT_DEV_FOUND_LEGACY_PAIRING);
+	not_connectable = (flags & MGMT_DEV_FOUND_NOT_CONNECTABLE);
+	name_resolve_fail = (flags & MGMT_DEV_FOUND_NAME_RESOLVE_FAIL);
 
 	update_found_devices(adapter, &ev->addr.bdaddr, ev->addr.type,
 					ev->rssi, confirm_name, legacy,
-					flags & MGMT_DEV_FOUND_NOT_CONNECTABLE,
+					not_connectable, name_resolve_fail,
 					eir, eir_len);
 }
 
diff --git a/src/device.c b/src/device.c
index fdc2d50a47..699faeba3b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -79,6 +79,8 @@
 #define GATT_INCLUDE_UUID_STR "2802"
 #define GATT_CHARAC_UUID_STR "2803"
 
+#define NAME_RESOLVE_RETRY_DELAY	120 /* seconds */
+
 static DBusConnection *dbus_conn = NULL;
 static unsigned service_state_cb_id;
 
@@ -272,6 +274,9 @@ struct btd_device {
 
 	GIOChannel	*att_io;
 	guint		store_id;
+
+	time_t		name_resolve_earliest_allow_time;
+	uint8_t		name_resolve_fail_count;
 };
 
 static const uint16_t uuid_list[] = {
@@ -4361,6 +4366,24 @@ bool device_name_known(struct btd_device *device)
 	return device->name[0] != '\0';
 }
 
+bool device_name_resolve_allowed(struct btd_device *device)
+{
+	return time(NULL) >= device->name_resolve_earliest_allow_time;
+}
+
+void device_name_resolve_fail(struct btd_device *device)
+{
+	if (!device)
+		return;
+
+	/* Punish this device by not allowing name resolve for some time.
+	 * increase punishment time for subsequent failures.
+	 */
+	device->name_resolve_fail_count++;
+	device->name_resolve_earliest_allow_time = time(NULL) +
+		NAME_RESOLVE_RETRY_DELAY * device->name_resolve_fail_count;
+}
+
 void device_set_class(struct btd_device *device, uint32_t class)
 {
 	if (device->class == class)
diff --git a/src/device.h b/src/device.h
index 5f615cb4b6..76d79855f8 100644
--- a/src/device.h
+++ b/src/device.h
@@ -25,6 +25,8 @@ void btd_device_device_set_name(struct btd_device *device, const char *name);
 void device_store_cached_name(struct btd_device *dev, const char *name);
 void device_get_name(struct btd_device *device, char *name, size_t len);
 bool device_name_known(struct btd_device *device);
+bool device_name_resolve_allowed(struct btd_device *device);
+void device_name_resolve_fail(struct btd_device *device);
 void device_set_class(struct btd_device *device, uint32_t class);
 void device_update_addr(struct btd_device *device, const bdaddr_t *bdaddr,
 							uint8_t bdaddr_type);
diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 42ef9acefa..14d05efa45 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -667,9 +667,9 @@ static void device_found(uint16_t index, uint16_t len, const void *param,
 		memset(&cp, 0, sizeof(cp));
 		memcpy(&cp.addr, &ev->addr, sizeof(cp.addr));
 		if (resolve_names)
-			cp.name_known = 0;
+			cp.name_state = MGMT_CONFIRM_NAME_UNKNOWN;
 		else
-			cp.name_known = 1;
+			cp.name_state = MGMT_CONFIRM_NAME_DONT_CARE;
 
 		mgmt_reply(mgmt, MGMT_OP_CONFIRM_NAME, index, sizeof(cp), &cp,
 						confirm_name_rsp, NULL, NULL);
-- 
2.34.0.rc0.344.g81b53c2807-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Bluez PATCH 2/3] device: Save remote name request attempts into cache file
  2021-11-11 11:54 [Bluez PATCH 1/3] Listen and process remote name resolving failure Archie Pusaka
@ 2021-11-11 11:54 ` Archie Pusaka
  2021-11-11 11:54 ` [Bluez PATCH 3/3] doc: Add Name Resolve Fail flag in device found event Archie Pusaka
  2021-11-11 12:45 ` [Bluez,1/3] Listen and process remote name resolving failure bluez.test.bot
  2 siblings, 0 replies; 6+ messages in thread
From: Archie Pusaka @ 2021-11-11 11:54 UTC (permalink / raw)
  To: linux-bluetooth, Luiz Augusto von Dentz, Marcel Holtmann
  Cc: CrosBT Upstreaming, Archie Pusaka, Sonny Sasaka, Miao-chen Chou

From: Archie Pusaka <apusaka@chromium.org>

Since a peer device is potentially removed if not discovered for more
than 30 seconds, we would lost the remote name request activity when
the device is rediscovered. This could end up with a remote name
request much sooner than we intend it to be.

Therefore, put the RNR record into a cache file, so we can recover it
when the peer device is rediscovered.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

 src/device.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 94 insertions(+), 2 deletions(-)

diff --git a/src/device.c b/src/device.c
index 699faeba3b..fa6e3abc37 100644
--- a/src/device.c
+++ b/src/device.c
@@ -568,6 +568,63 @@ void device_store_cached_name(struct btd_device *dev, const char *name)
 	g_key_file_free(key_file);
 }
 
+static void device_store_cached_name_resolve_attempts(struct btd_device *dev)
+{
+	char filename[PATH_MAX];
+	char d_addr[18];
+	GKeyFile *key_file;
+	GError *gerr = NULL;
+	char *data;
+	char *data_old;
+	gsize length = 0;
+	gsize length_old = 0;
+	uint64_t earliest_allowed;
+	unsigned int num_failures;
+
+	if (device_address_is_private(dev)) {
+		DBG("Can't store name resolve for private addressed device %s",
+								dev->path);
+		return;
+	}
+
+	ba2str(&dev->bdaddr, d_addr);
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/cache/%s",
+			btd_adapter_get_storage_dir(dev->adapter), d_addr);
+	create_file(filename, 0600);
+
+	key_file = g_key_file_new();
+	if (!g_key_file_load_from_file(key_file, filename, 0, &gerr)) {
+		error("Unable to load key file from %s: (%s)", filename,
+								gerr->message);
+		g_error_free(gerr);
+	}
+
+	earliest_allowed = (uint64_t) dev->name_resolve_earliest_allow_time;
+	num_failures = dev->name_resolve_fail_count;
+
+	data_old = g_key_file_to_data(key_file, &length_old, NULL);
+
+	g_key_file_set_uint64(key_file, "NameResolve", "EarliestAllowed",
+							earliest_allowed);
+	g_key_file_set_integer(key_file, "NameResolve", "NumFailures",
+							num_failures);
+
+	data = g_key_file_to_data(key_file, &length, NULL);
+
+	if ((length != length_old) || (memcmp(data, data_old, length))) {
+		if (!g_file_set_contents(filename, data, length, &gerr)) {
+			error("Unable set contents for %s: (%s)", filename,
+								gerr->message);
+			g_error_free(gerr);
+		}
+	}
+
+	g_free(data);
+	g_free(data_old);
+
+	g_key_file_free(key_file);
+}
+
 static void browse_request_free(struct browse_req *req)
 {
 	struct btd_device *device = req->device;
@@ -3277,6 +3334,36 @@ failed:
 	return str;
 }
 
+static void load_cached_name_resolve_attempts(struct btd_device *device,
+					const char *local, const char *peer)
+{
+	char filename[PATH_MAX];
+	GKeyFile *key_file;
+	uint64_t earliest_allowed;
+	unsigned int num_failures;
+
+	if (device_address_is_private(device))
+		return;
+
+	snprintf(filename, PATH_MAX, STORAGEDIR "/%s/cache/%s", local, peer);
+
+	key_file = g_key_file_new();
+
+	if (!g_key_file_load_from_file(key_file, filename, 0, NULL))
+		goto failed;
+
+	earliest_allowed = g_key_file_get_uint64(key_file, "NameResolve",
+						"EarliestAllowed", NULL);
+	num_failures = g_key_file_get_uint64(key_file, "NameResolve",
+						"NumFailures", NULL);
+
+	device->name_resolve_earliest_allow_time = earliest_allowed;
+	device->name_resolve_fail_count = (uint8_t) num_failures;
+
+failed:
+	g_key_file_free(key_file);
+}
+
 static struct csrk_info *load_csrk(GKeyFile *key_file, const char *group)
 {
 	struct csrk_info *csrk;
@@ -4284,6 +4371,7 @@ struct btd_device *device_create(struct btd_adapter *adapter,
 	struct btd_device *device;
 	char dst[18];
 	char *str;
+	const char *storage_dir;
 
 	ba2str(bdaddr, dst);
 	DBG("dst %s", dst);
@@ -4299,13 +4387,15 @@ struct btd_device *device_create(struct btd_adapter *adapter,
 	else
 		device->le = true;
 
-	str = load_cached_name(device, btd_adapter_get_storage_dir(adapter),
-									dst);
+	storage_dir = btd_adapter_get_storage_dir(adapter);
+	str = load_cached_name(device, storage_dir, dst);
 	if (str) {
 		strcpy(device->name, str);
 		g_free(str);
 	}
 
+	load_cached_name_resolve_attempts(device, storage_dir, dst);
+
 	return device;
 }
 
@@ -4382,6 +4472,8 @@ void device_name_resolve_fail(struct btd_device *device)
 	device->name_resolve_fail_count++;
 	device->name_resolve_earliest_allow_time = time(NULL) +
 		NAME_RESOLVE_RETRY_DELAY * device->name_resolve_fail_count;
+
+	device_store_cached_name_resolve_attempts(device);
 }
 
 void device_set_class(struct btd_device *device, uint32_t class)
-- 
2.34.0.rc0.344.g81b53c2807-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Bluez PATCH 3/3] doc: Add Name Resolve Fail flag in device found event
  2021-11-11 11:54 [Bluez PATCH 1/3] Listen and process remote name resolving failure Archie Pusaka
  2021-11-11 11:54 ` [Bluez PATCH 2/3] device: Save remote name request attempts into cache file Archie Pusaka
@ 2021-11-11 11:54 ` Archie Pusaka
  2021-11-11 16:35   ` Marcel Holtmann
  2021-11-11 12:45 ` [Bluez,1/3] Listen and process remote name resolving failure bluez.test.bot
  2 siblings, 1 reply; 6+ messages in thread
From: Archie Pusaka @ 2021-11-11 11:54 UTC (permalink / raw)
  To: linux-bluetooth, Luiz Augusto von Dentz, Marcel Holtmann
  Cc: CrosBT Upstreaming, Archie Pusaka

From: Archie Pusaka <apusaka@chromium.org>

Userspace should use this new flag to decide whether to do the remote
name resolving or not, by sending Confirm Name MGMT command and set
the appropriate flag.

This patch also extends the Confirm Name command by allowing userspace
to send 0x02 to show it doesn't care about the peer devices names.
---

 doc/mgmt-api.txt | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
index 97d33e30a1..e4c8de39f0 100644
--- a/doc/mgmt-api.txt
+++ b/doc/mgmt-api.txt
@@ -1493,7 +1493,7 @@ Confirm Name Command
 	Controller Index:	<controller id>
 	Command Parameters:	Address (6 Octets)
 				Address_Type (1 Octet)
-				Name_Known (1 Octet)
+				Name_State (1 Octet)
 	Return Parameters:	Address (6 Octets)
 				Address_Type (1 Octet)
 
@@ -1506,10 +1506,11 @@ Confirm Name Command
 		1	LE Public
 		2	LE Random
 
-	The Name_Known parameter should be set to 0x01 if user space
-	knows the name for the device and 0x00 if it doesn't. If set to
-	0x00 the kernel will perform a name resolving procedure for the
-	device in question.
+	The Name_State parameter should be set to 0x00 if user space
+	doesn't know the name for the device to make the kernel
+	perform a name resolving procedure for the device in question.
+	Otherwise, set to 0x01 if user space already knew the device's
+	name, or 0x02 if it doesn't care.
 
 	This command can only be used when the controller is powered.
 
@@ -4089,6 +4090,7 @@ Device Connected Event
 		1	Legacy Pairing
 		2	Reserved (not in use)
 		3	Initiated Connection
+		4	Reserved (not in use)
 
 
 Device Disconnected Event
@@ -4263,6 +4265,7 @@ Device Found Event
 		1	Legacy Pairing
 		2	Not Connectable
 		3	Reserved (not in use)
+		4	Name Resolve Fail
 
 	For the RSSI field a value of 127 indicates that the RSSI is
 	not available. That can happen with Bluetooth 1.1 and earlier
@@ -4285,6 +4288,11 @@ Device Found Event
 	accept any connections. This can be indicated by Low Energy
 	devices that are in broadcaster role.
 
+	The Name Resolve Fail flag indicates that name resolving
+	procedure has ended with failure for this device. The user space
+	should use this information to determine when is a good time to
+	retry the name resolving procedure.
+
 
 Discovering Event
 =================
-- 
2.34.0.rc0.344.g81b53c2807-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: [Bluez,1/3] Listen and process remote name resolving failure
  2021-11-11 11:54 [Bluez PATCH 1/3] Listen and process remote name resolving failure Archie Pusaka
  2021-11-11 11:54 ` [Bluez PATCH 2/3] device: Save remote name request attempts into cache file Archie Pusaka
  2021-11-11 11:54 ` [Bluez PATCH 3/3] doc: Add Name Resolve Fail flag in device found event Archie Pusaka
@ 2021-11-11 12:45 ` bluez.test.bot
  2 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2021-11-11 12:45 UTC (permalink / raw)
  To: linux-bluetooth, apusaka

[-- Attachment #1: Type: text/plain, Size: 2956 bytes --]

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=578681

---Test result---

Test Summary:
CheckPatch                    PASS      4.61 seconds
GitLint                       FAIL      2.88 seconds
Prep - Setup ELL              PASS      49.89 seconds
Build - Prep                  PASS      0.50 seconds
Build - Configure             PASS      9.24 seconds
Build - Make                  FAIL      108.25 seconds
Make Check                    FAIL      0.84 seconds
Make Distcheck                FAIL      111.72 seconds
Build w/ext ELL - Configure   PASS      9.19 seconds
Build w/ext ELL - Make        FAIL      95.10 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
[Bluez,1/3] Listen and process remote name resolving failure
15: B1 Line exceeds max length (121>80): "https://patchwork.kernel.org/project/bluetooth/patch/20211028191805.1.I35b7f3a496f834de6b43a32f94b6160cb1467c94@changeid/"


##############################
Test: Build - Make - FAIL
Desc: Build the BlueZ source tree
Output:
android/bluetooth.c: In function ‘confirm_device_name’:
android/bluetooth.c:1534:5: error: ‘struct mgmt_cp_confirm_name’ has no member named ‘name_known’
 1534 |   cp.name_known = 1;
      |     ^
make[1]: *** [Makefile:6992: android/bluetooth.o] Error 1
make: *** [Makefile:4175: all] Error 2


##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
android/bluetooth.c: In function ‘confirm_device_name’:
android/bluetooth.c:1534:5: error: ‘struct mgmt_cp_confirm_name’ has no member named ‘name_known’
 1534 |   cp.name_known = 1;
      |     ^
make[1]: *** [Makefile:6992: android/bluetooth.o] Error 1
make: *** [Makefile:10501: check] Error 2


##############################
Test: Make Distcheck - FAIL
Desc: Run distcheck to check the distribution
Output:
../../android/bluetooth.c: In function ‘confirm_device_name’:
../../android/bluetooth.c:1534:5: error: ‘struct mgmt_cp_confirm_name’ has no member named ‘name_known’
 1534 |   cp.name_known = 1;
      |     ^
make[2]: *** [Makefile:6992: android/bluetooth.o] Error 1
make[1]: *** [Makefile:4175: all] Error 2
make: *** [Makefile:10422: distcheck] Error 1


##############################
Test: Build w/ext ELL - Make - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
android/bluetooth.c: In function ‘confirm_device_name’:
android/bluetooth.c:1534:5: error: ‘struct mgmt_cp_confirm_name’ has no member named ‘name_known’
 1534 |   cp.name_known = 1;
      |     ^
make[1]: *** [Makefile:6992: android/bluetooth.o] Error 1
make: *** [Makefile:4175: all] Error 2




---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Bluez PATCH 3/3] doc: Add Name Resolve Fail flag in device found event
  2021-11-11 11:54 ` [Bluez PATCH 3/3] doc: Add Name Resolve Fail flag in device found event Archie Pusaka
@ 2021-11-11 16:35   ` Marcel Holtmann
  2021-11-12  3:10     ` Archie Pusaka
  0 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2021-11-11 16:35 UTC (permalink / raw)
  To: Archie Pusaka
  Cc: linux-bluetooth, Luiz Augusto von Dentz, CrosBT Upstreaming,
	Archie Pusaka

Hi Archie,

> Userspace should use this new flag to decide whether to do the remote
> name resolving or not, by sending Confirm Name MGMT command and set
> the appropriate flag.
> 
> This patch also extends the Confirm Name command by allowing userspace
> to send 0x02 to show it doesn't care about the peer devices names.
> ---
> 
> doc/mgmt-api.txt | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> index 97d33e30a1..e4c8de39f0 100644
> --- a/doc/mgmt-api.txt
> +++ b/doc/mgmt-api.txt
> @@ -1493,7 +1493,7 @@ Confirm Name Command
> 	Controller Index:	<controller id>
> 	Command Parameters:	Address (6 Octets)
> 				Address_Type (1 Octet)
> -				Name_Known (1 Octet)
> +				Name_State (1 Octet)
> 	Return Parameters:	Address (6 Octets)
> 				Address_Type (1 Octet)
> 
> @@ -1506,10 +1506,11 @@ Confirm Name Command
> 		1	LE Public
> 		2	LE Random
> 
> -	The Name_Known parameter should be set to 0x01 if user space
> -	knows the name for the device and 0x00 if it doesn't. If set to
> -	0x00 the kernel will perform a name resolving procedure for the
> -	device in question.
> +	The Name_State parameter should be set to 0x00 if user space
> +	doesn't know the name for the device to make the kernel
> +	perform a name resolving procedure for the device in question.
> +	Otherwise, set to 0x01 if user space already knew the device's
> +	name, or 0x02 if it doesn't care.

I am a bit worried about userspace sending a 0x02 for a kernel that doesn’t understand it. Do you think the kernel can make use of this “don’t care” information? Or should we just keep it to userspace to send 0x01 / 0x00 based on its policy.

> 
> 	This command can only be used when the controller is powered.
> 
> @@ -4089,6 +4090,7 @@ Device Connected Event
> 		1	Legacy Pairing
> 		2	Reserved (not in use)
> 		3	Initiated Connection
> +		4	Reserved (not in use)
> 
> 
> Device Disconnected Event
> @@ -4263,6 +4265,7 @@ Device Found Event
> 		1	Legacy Pairing
> 		2	Not Connectable
> 		3	Reserved (not in use)
> +		4	Name Resolve Fail

I would do “Name Request Failed” here. Just to be a bit inline what the spec term is.

> 
> 	For the RSSI field a value of 127 indicates that the RSSI is
> 	not available. That can happen with Bluetooth 1.1 and earlier
> @@ -4285,6 +4288,11 @@ Device Found Event
> 	accept any connections. This can be indicated by Low Energy
> 	devices that are in broadcaster role.
> 
> +	The Name Resolve Fail flag indicates that name resolving
> +	procedure has ended with failure for this device. The user space
> +	should use this information to determine when is a good time to
> +	retry the name resolving procedure.
> +
> 

Regards

Marcel


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Bluez PATCH 3/3] doc: Add Name Resolve Fail flag in device found event
  2021-11-11 16:35   ` Marcel Holtmann
@ 2021-11-12  3:10     ` Archie Pusaka
  0 siblings, 0 replies; 6+ messages in thread
From: Archie Pusaka @ 2021-11-12  3:10 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, Luiz Augusto von Dentz, CrosBT Upstreaming,
	Archie Pusaka

Hi Marcel,

On Fri, 12 Nov 2021 at 00:35, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Archie,
>
> > Userspace should use this new flag to decide whether to do the remote
> > name resolving or not, by sending Confirm Name MGMT command and set
> > the appropriate flag.
> >
> > This patch also extends the Confirm Name command by allowing userspace
> > to send 0x02 to show it doesn't care about the peer devices names.
> > ---
> >
> > doc/mgmt-api.txt | 18 +++++++++++++-----
> > 1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> > index 97d33e30a1..e4c8de39f0 100644
> > --- a/doc/mgmt-api.txt
> > +++ b/doc/mgmt-api.txt
> > @@ -1493,7 +1493,7 @@ Confirm Name Command
> >       Controller Index:       <controller id>
> >       Command Parameters:     Address (6 Octets)
> >                               Address_Type (1 Octet)
> > -                             Name_Known (1 Octet)
> > +                             Name_State (1 Octet)
> >       Return Parameters:      Address (6 Octets)
> >                               Address_Type (1 Octet)
> >
> > @@ -1506,10 +1506,11 @@ Confirm Name Command
> >               1       LE Public
> >               2       LE Random
> >
> > -     The Name_Known parameter should be set to 0x01 if user space
> > -     knows the name for the device and 0x00 if it doesn't. If set to
> > -     0x00 the kernel will perform a name resolving procedure for the
> > -     device in question.
> > +     The Name_State parameter should be set to 0x00 if user space
> > +     doesn't know the name for the device to make the kernel
> > +     perform a name resolving procedure for the device in question.
> > +     Otherwise, set to 0x01 if user space already knew the device's
> > +     name, or 0x02 if it doesn't care.
>
> I am a bit worried about userspace sending a 0x02 for a kernel that doesn’t understand it. Do you think the kernel can make use of this “don’t care” information? Or should we just keep it to userspace to send 0x01 / 0x00 based on its policy.
>
On the current kernel, 0x02 would be casted to True, and the kernel
would treat this as "name is known".
Indeed, this "don't care" information is of no use to the Kernel (at
least for now).

If this is concerning, I'd propose to just express the "don't care" by
not sending a Confirm Name command in the first place, similar to what
we do when we receive a Device Found event for a device which is
already discovered before.

> >
> >       This command can only be used when the controller is powered.
> >
> > @@ -4089,6 +4090,7 @@ Device Connected Event
> >               1       Legacy Pairing
> >               2       Reserved (not in use)
> >               3       Initiated Connection
> > +             4       Reserved (not in use)
> >
> >
> > Device Disconnected Event
> > @@ -4263,6 +4265,7 @@ Device Found Event
> >               1       Legacy Pairing
> >               2       Not Connectable
> >               3       Reserved (not in use)
> > +             4       Name Resolve Fail
>
> I would do “Name Request Failed” here. Just to be a bit inline what the spec term is.
>
Ack, will update.

> >
> >       For the RSSI field a value of 127 indicates that the RSSI is
> >       not available. That can happen with Bluetooth 1.1 and earlier
> > @@ -4285,6 +4288,11 @@ Device Found Event
> >       accept any connections. This can be indicated by Low Energy
> >       devices that are in broadcaster role.
> >
> > +     The Name Resolve Fail flag indicates that name resolving
> > +     procedure has ended with failure for this device. The user space
> > +     should use this information to determine when is a good time to
> > +     retry the name resolving procedure.
> > +
> >
>
> Regards
>
> Marcel
>

Thanks,
Archie

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-11-12  3:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-11 11:54 [Bluez PATCH 1/3] Listen and process remote name resolving failure Archie Pusaka
2021-11-11 11:54 ` [Bluez PATCH 2/3] device: Save remote name request attempts into cache file Archie Pusaka
2021-11-11 11:54 ` [Bluez PATCH 3/3] doc: Add Name Resolve Fail flag in device found event Archie Pusaka
2021-11-11 16:35   ` Marcel Holtmann
2021-11-12  3:10     ` Archie Pusaka
2021-11-11 12:45 ` [Bluez,1/3] Listen and process remote name resolving failure bluez.test.bot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.