RE: RAP: streamline session handling, reflector setup

RE: RAP: streamline session handling, reflector setup

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 987 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=1131452

---Test result---

Test Summary:
CheckPatch                    PASS      0.89 seconds
GitLint                       PASS      0.50 seconds
BuildEll                      PASS      14.91 seconds
BluezMake                     PASS      369.85 seconds
MakeCheck                     PASS      0.83 seconds
MakeDistcheck                 PASS      118.70 seconds
CheckValgrind                 PASS      112.98 seconds
CheckSmatch                   PASS      203.48 seconds
bluezmakeextell               PASS      73.27 seconds
IncrementalBuild              PASS      371.90 seconds
ScanBuild                     PASS      626.25 seconds



https://github.com/bluez/bluez/pull/2336

---
Regards,
Linux Bluetooth

[PATCH BlueZ v6 0/2] RAP: streamline session handling, reflector setup

[Naga Bhavani Akella]

This series adds RAP users to the direct session lookup functionality.

The first patch refactors the shared RAP layer by introducing explicit
attachment/disconnection helpers, adding bt_rap_get_session(), and
using session lookup directly from the RAS GATT server callbacks.

The second patch updates the ranging RAP profile accordingly.
Reflector setup is consolidated into a dedicated helper, D-Bus registration
state is tracked explicitly, cleanup is centralized in rap_data_free()


Naga Bhavani Akella (2):
  rap: use session lookup for RAS operations
  profiles: centralize reflector setup

 profiles/ranging/rap.c | 209 ++++++++++++++++++++++++++++-------------
 src/shared/rap.c       | 143 +++++++++++++++++++++++++++-
 src/shared/rap.h       |   2 +
 3 files changed, 285 insertions(+), 69 deletions(-)

-- 

[PATCH BlueZ v6 1/2] rap: use session lookup for RAS operations

[Naga Bhavani Akella]

Store disconnect information in RAP state and
centralize attachment handling through dedicated helpers.
Add bt_rap_get_session() and use it from all RAS
GATT server callbacks
---
 src/shared/rap.c | 143 +++++++++++++++++++++++++++++++++++++++++++++--
 src/shared/rap.h |   2 +
 2 files changed, 141 insertions(+), 4 deletions(-)

diff --git a/src/shared/rap.c b/src/shared/rap.c
index dfb272d3a..b0c5f8ae7 100644
--- a/src/shared/rap.c
+++ b/src/shared/rap.c
@@ -19,7 +19,6 @@
 
 #include "src/shared/queue.h"
 #include "src/shared/util.h"
-#include "src/shared/timeout.h"
 #include "src/shared/att.h"
 #include "src/shared/gatt-db.h"
 #include "src/shared/gatt-server.h"
@@ -291,6 +290,7 @@ struct bt_rap {
 	struct bt_att *att;
 
 	unsigned int idle_id;
+	unsigned int disconn_id;
 
 	struct queue *notify;
 	struct queue *pending;
@@ -564,6 +564,40 @@ static void rap_detached(void *data, void *user_data)
 	cb->detached(rap, cb->user_data);
 }
 
+static void rap_attached(void *data, void *user_data)
+{
+	struct bt_rap_cb *cb = data;
+	struct bt_rap *rap = user_data;
+
+	if (!cb->attached)
+		return;
+
+	cb->attached(rap, cb->user_data);
+}
+
+static void rap_disconnected(int err, void *user_data)
+{
+	struct bt_rap *rap = user_data;
+
+	rap->disconn_id = 0;
+
+	bt_rap_detach(rap);
+}
+
+static void rap_attach_att(struct bt_rap *rap, struct bt_att *att)
+{
+	if (rap->disconn_id) {
+		if (att == bt_rap_get_att(rap))
+			return;
+		bt_att_unregister_disconnect(rap->att, rap->disconn_id);
+	}
+
+	rap->att = att;
+	rap->disconn_id = bt_att_register_disconnect(rap->att,
+							rap_disconnected,
+							rap, NULL);
+}
+
 void bt_rap_detach(struct bt_rap *rap)
 {
 	if (!queue_remove(sessions, rap))
@@ -573,6 +607,9 @@ void bt_rap_detach(struct bt_rap *rap)
 	bt_gatt_client_unref(rap->client);
 	rap->client = NULL;
 
+	bt_att_unregister_disconnect(rap->att, rap->disconn_id);
+	rap->att = NULL;
+
 	queue_foreach(bt_rap_cbs, rap_detached, rap);
 }
 
@@ -653,6 +690,34 @@ struct bt_att *bt_rap_get_att(struct bt_rap *rap)
 	return bt_gatt_client_get_att(rap->client);
 }
 
+struct bt_rap *bt_rap_get_session(struct bt_att *att, struct gatt_db *db)
+{
+	const struct queue_entry *entry;
+	struct bt_rap *rap;
+
+	for (entry = queue_get_entries(sessions); entry; entry = entry->next) {
+		rap = entry->data;
+
+		if (att == bt_rap_get_att(rap))
+			return rap;
+	}
+
+	rap = bt_rap_new(db, NULL);
+	if (!rap)
+		return NULL;
+
+	rap->att = att;
+
+	/*
+	 * A rejected attach (e.g. a duplicate session for a device that
+	 * already has one) leaves rap already freed - don't touch it.
+	 */
+	if (!bt_rap_attach(rap, NULL))
+		return NULL;
+
+	return rap;
+}
+
 struct bt_rap *bt_rap_ref(struct bt_rap *rap)
 {
 	if (!rap)
@@ -729,6 +794,7 @@ static void ras_features_read_cb(struct gatt_db_attribute *attrib,
 				 uint8_t opcode, struct bt_att *att,
 				 void *user_data)
 {
+	struct ras *ras = user_data;
 	/*
 	 * Feature mask: bits 0-2 set:
 	 *  - Real-time ranging
@@ -737,6 +803,9 @@ static void ras_features_read_cb(struct gatt_db_attribute *attrib,
 	 */
 	uint8_t value[4] = { 0x01, 0x00, 0x00, 0x00 };
 
+	if (ras)
+		bt_rap_get_session(att, ras->rapdb->db);
+
 	gatt_db_attribute_read_result(attrib, id, 0, value, sizeof(value));
 }
 
@@ -745,6 +814,11 @@ static void ras_ondemand_read_cb(struct gatt_db_attribute *attrib,
 				 uint8_t opcode, struct bt_att *att,
 				 void *user_data)
 {
+	struct ras *ras = user_data;
+
+	if (ras)
+		bt_rap_get_session(att, ras->rapdb->db);
+
 	/* No static read data – on‑demand data is pushed via
 	 * notifications
 	 */
@@ -761,6 +835,11 @@ static void ras_control_point_write_cb(struct gatt_db_attribute *attrib,
 				       uint8_t opcode, struct bt_att *att,
 				       void *user_data)
 {
+	struct ras *ras = user_data;
+
+	if (ras)
+		bt_rap_get_session(att, ras->rapdb->db);
+
 	/* Control point handler - implementation TBD */
 }
 
@@ -770,9 +849,13 @@ static void ras_data_ready_read_cb(struct gatt_db_attribute *attrib,
 				   uint8_t opcode, struct bt_att *att,
 				   void *user_data)
 {
+	struct ras *ras = user_data;
 	uint16_t counter = 0;
 	uint8_t value[2];
 
+	if (ras)
+		bt_rap_get_session(att, ras->rapdb->db);
+
 	put_le16(counter, value);
 	gatt_db_attribute_read_result(attrib, id, 0, value, sizeof(value));
 }
@@ -783,8 +866,12 @@ static void ras_data_overwritten_read_cb(struct gatt_db_attribute *attrib,
 					 uint8_t opcode, struct bt_att *att,
 					 void *user_data)
 {
+	struct ras *ras = user_data;
 	uint8_t value[2] = { 0x00, 0x00 };
 
+	if (ras)
+		bt_rap_get_session(att, ras->rapdb->db);
+
 	gatt_db_attribute_read_result(attrib, id, 0, value, sizeof(value));
 }
 
@@ -806,6 +893,8 @@ static void ras_ranging_data_ccc_write_cb(struct gatt_db_attribute *attrib,
 		return;
 	}
 
+	bt_rap_get_session(att, ras->rapdb->db);
+
 	if (offset) {
 		gatt_db_attribute_write_result(attrib, id,
 					BT_ATT_ERROR_INVALID_OFFSET);
@@ -2794,28 +2883,74 @@ static void rap_idle(void *data)
 bool bt_rap_attach(struct bt_rap *rap, struct bt_gatt_client *client)
 {
 	bt_uuid_t uuid;
+	bool attached;
+
+	if (queue_find(sessions, NULL, rap)) {
+		if (client && !rap->client)
+			goto clone;
+		/*
+		 * Already attached: treat re-attach as idempotent rather
+		 * than failing, so e.g. a second rap_accept() on the same
+		 * session succeeds instead of erroring out.
+		 */
+		return true;
+	}
 
 	if (!sessions)
 		sessions = queue_new();
 
 	queue_push_tail(sessions, rap);
 
-	if (!client)
+	/*
+	 * Notify profile callbacks before rap_attach_att() below runs, so
+	 * bt_rap_get_att(rap) inside a rap_attached callback only sees the
+	 * att already set on rap (e.g. by bt_rap_get_session()), not the
+	 * one about to be registered a few lines down.
+	 *
+	 * A callback may reject this session (e.g. a duplicate ad-hoc
+	 * session for a device that already has one) by unref'ing rap,
+	 * which can free it. Hold our own reference across the
+	 * notification so a rejection can never drop the count to zero
+	 * while callbacks are still running.
+	 *
+	 * Releasing that reference below is what may actually free rap
+	 * (via rap_free() -> bt_rap_detach(), which removes it from
+	 * `sessions` before the object is freed). Check membership only
+	 * *after* the unref, and only by pointer identity - never by
+	 * dereferencing rap - so the check itself is safe even if rap no
+	 * longer exists.
+	 */
+	bt_rap_ref(rap);
+	queue_foreach(bt_rap_cbs, rap_attached, rap);
+	bt_rap_unref(rap);
+
+	attached = queue_find(sessions, NULL, rap);
+	if (!attached)
+		return false;
+
+	if (!client) {
+		if (rap->att)
+			rap_attach_att(rap, rap->att);
 		return true;
+	}
 
 	if (rap->client)
 		return false;
 
+clone:
 	rap->client = bt_gatt_client_clone(client);
 	if (!rap->client)
 		return false;
 
+	rap_attach_att(rap, bt_gatt_client_get_att(client));
+
 	bt_gatt_client_idle_register(rap->client, rap_idle, rap, NULL);
 
 	bt_uuid16_create(&uuid, RAS_UUID16);
 
-	gatt_db_foreach_service(rap->rrapdb->db, &uuid,
-				foreach_rap_service, rap);
+	if (rap->rrapdb)
+		gatt_db_foreach_service(rap->rrapdb->db, &uuid,
+					foreach_rap_service, rap);
 
 	return true;
 }
diff --git a/src/shared/rap.h b/src/shared/rap.h
index 5582635e6..a44831e6e 100644
--- a/src/shared/rap.h
+++ b/src/shared/rap.h
@@ -15,6 +15,7 @@
 struct bt_rap;
 struct gatt_db;
 struct bt_gatt_client;
+struct bt_att;
 
 /* Channel Sounding Events */
 struct bt_rap_hci_cs_options {
@@ -171,6 +172,7 @@ bool bt_rap_attach(struct bt_rap *rap, struct bt_gatt_client *client);
 void bt_rap_detach(struct bt_rap *rap);
 
 struct bt_att *bt_rap_get_att(struct bt_rap *rap);
+struct bt_rap *bt_rap_get_session(struct bt_att *att, struct gatt_db *db);
 
 bool bt_rap_set_user_data(struct bt_rap *rap, void *user_data);
 
-- 

[PATCH BlueZ v6 2/2] profiles: centralize reflector setup

[Naga Bhavani Akella]

Add a helper for reflector initialization and track D-Bus registration
state in rap_data. Simplify connection handling, move interface cleanup
to object destruction, and streamline removal logic.
---
 profiles/ranging/rap.c | 209 ++++++++++++++++++++++++++++-------------
 1 file changed, 144 insertions(+), 65 deletions(-)

diff --git a/profiles/ranging/rap.c b/profiles/ranging/rap.c
index 082c35558..91f30ac30 100644
--- a/profiles/ranging/rap.c
+++ b/profiles/ranging/rap.c
@@ -65,10 +65,12 @@ struct rap_data {
 	void *hci_sm;  /* Per-device HCI state machine */
 	uint16_t conn_handle;  /* Last known connection handle */
 	struct cs_session active_session;  /* active==false when idle */
+	bool dbus_registered;
 };
 
 static struct queue *sessions;
 static struct queue *adapter_list;  /* List of rap_adapter_data */
+static int rap_setup_reflector(struct rap_data *data);
 
 /* Adapter data management */
 static bool match_adapter(const void *data, const void *match_data)
@@ -188,13 +190,34 @@ static void rap_debug(const char *str, void *user_data)
 	DBG_IDX(0xffff, "%s", str);
 }
 
-static void rap_data_add(struct rap_data *data)
+static bool match_data(const void *data, const void *match_data)
+{
+	const struct rap_data *mdata = data;
+	const struct bt_rap *rap = match_data;
+
+	return mdata->rap == rap;
+}
+
+static bool match_device(const void *data, const void *match_data)
+{
+	const struct rap_data *mdata = data;
+	const struct btd_device *device = match_data;
+
+	return mdata->device == device;
+}
+
+static bool rap_data_add(struct rap_data *data)
 {
 	DBG("%p", data);
 
 	if (queue_find(sessions, NULL, data)) {
 		error("data %p already added", data);
-		return;
+		return false;
+	}
+
+	if (queue_find(sessions, match_device, data->device)) {
+		error("rap_data already exists for device %p", data->device);
+		return false;
 	}
 
 	bt_rap_set_debug(data->rap, rap_debug, NULL, NULL);
@@ -206,18 +229,19 @@ static void rap_data_add(struct rap_data *data)
 
 	if (data->service)
 		btd_service_set_user_data(data->service, data);
-}
-
-static bool match_data(const void *data, const void *match_data)
-{
-	const struct rap_data *mdata = data;
-	const struct bt_rap *rap = match_data;
 
-	return mdata->rap == rap;
+	return true;
 }
 
 static void rap_data_free(struct rap_data *data)
 {
+	if (data->dbus_registered) {
+		g_dbus_unregister_interface(btd_get_dbus_connection(),
+					    device_get_path(data->device),
+					    CS_INTERFACE);
+		data->dbus_registered = false;
+	}
+
 	if (data->service) {
 		btd_service_set_user_data(data->service, NULL);
 		bt_rap_set_user_data(data->rap, NULL);
@@ -281,31 +305,60 @@ static void rap_attached(struct bt_rap *rap, void *user_data)
 	struct rap_data *data;
 	struct bt_att *att;
 	struct btd_device *device;
+	int err;
 
 	DBG("%p", rap);
 
 	data = queue_find(sessions, match_data, rap);
-	if (data) {
-		DBG("data is already present");
-		return;
-	}
+	if (data)
+		goto setup;
 
 	att = bt_rap_get_att(rap);
 	if (!att) {
 		error("Unable to get att");
+		bt_rap_unref(rap);
 		return;
 	}
 
 	device = btd_adapter_find_device_by_fd(bt_att_get_fd(att));
 	if (!device) {
 		error("unable to find device");
+		bt_rap_unref(rap);
 		return;
 	}
 
+	/*
+	 * A separate bt_rap session for this device may already have a
+	 * rap_data (e.g. rap_probe()/rap_accept() ran first for the GATT
+	 * client role, while this "rap" came from bt_rap_get_session()'s
+	 * ad-hoc att-only session). Setting up a second HCI state machine
+	 * and D-Bus interface registration for the same device path would
+	 * race with the existing one, so rap_data_add() rejects it below
+	 * regardless of which side (ad-hoc vs. probe/accept) runs first.
+	 */
 	data = rap_data_new(device);
 	data->rap = rap;
 
-	rap_data_add(data);
+	if (!rap_data_add(data)) {
+		free(data);
+		bt_rap_unref(rap);
+		return;
+	}
+
+setup:
+	err = rap_setup_reflector(data);
+	if (err)
+		error("Failed to set up reflector session: %d", err);
+	/*
+	 * NOTE: on failure here, this rap_data is left half-constructed
+	 * (missing hci_sm and/or dbus_registered) with no retry. For a
+	 * session created via rap_probe()/rap_accept(), a later GATT client
+	 * idle event re-invokes rap_setup_reflector() and can still succeed.
+	 * But for an ad-hoc, client-less session (created above, from
+	 * bt_rap_get_session()), there is no ready/idle callback registered
+	 * anywhere to retry it, so a one-time failure here is permanent
+	 * until the ATT connection drops and rap_detached() cleans it up.
+	 */
 }
 
 enum cs_dict_target {
@@ -638,7 +691,11 @@ static int rap_probe(struct btd_service *service)
 		return -EINVAL;
 	}
 
-	rap_data_add(data);
+	if (!rap_data_add(data)) {
+		bt_rap_unref(data->rap);
+		free(data);
+		return -EINVAL;
+	}
 
 	data->ready_id = bt_rap_ready_register(data->rap, rap_ready, service,
 								NULL);
@@ -663,19 +720,13 @@ static void rap_remove(struct btd_service *service)
 		return;
 	}
 
-	g_dbus_unregister_interface(btd_get_dbus_connection(),
-				    device_get_path(device),
-				    CS_INTERFACE);
-
 	rap_data_remove(data);
 }
 
-static int rap_accept(struct btd_service *service)
+static int rap_setup_reflector(struct rap_data *data)
 {
-	struct btd_device *device = btd_service_get_device(service);
+	struct btd_device *device = data->device;
 	struct btd_adapter *adapter = device_get_adapter(device);
-	struct bt_gatt_client *client = btd_device_get_gatt_client(device);
-	struct rap_data *data = btd_service_get_user_data(service);
 	struct bt_att *att;
 	const bdaddr_t *bdaddr;
 	uint8_t bdaddr_type;
@@ -683,26 +734,18 @@ static int rap_accept(struct btd_service *service)
 	char addr[18];
 
 	ba2str(device_get_address(device), addr);
-	DBG("%s", addr);
 
-	if (!data) {
-		error("RAP Service not handled by profile");
-		return -EINVAL;
-	}
-
-	/* init shared adapter HCI channel */
-	if (!data->adapter_data) {
-		data->adapter_data = rap_adapter_data_ref(adapter);
+	if (!data->hci_sm) {
 		if (!data->adapter_data) {
-			error("Failed to get adapter HCI channel");
-			return -EINVAL;
+			data->adapter_data = rap_adapter_data_ref(adapter);
+			if (!data->adapter_data) {
+				error("Failed to get adapter HCI channel");
+				return -EINVAL;
+			}
+			DBG("Using shared HCI channel for adapter (ref=%d)",
+				data->adapter_data->ref_count);
 		}
-		DBG("Using shared HCI channel for adapter (ref_count=%d)",
-			data->adapter_data->ref_count);
-	}
 
-	/* per-device HCI state machine */
-	if (!data->hci_sm) {
 		data->hci_sm = bt_rap_attach_hci(data->rap,
 					data->adapter_data->hci,
 					btd_opts.defaults.bcs.role,
@@ -722,38 +765,74 @@ static int rap_accept(struct btd_service *service)
 					data);
 	}
 
-	if (!bt_rap_attach(data->rap, client)) {
-		error("RAP unable to attach");
-		return -EINVAL;
-	}
-
-	/* Set up connection handle mapping for CS event routing */
-	att = bt_rap_get_att(data->rap);
-	bdaddr = device_get_address(device);
-	bdaddr_type = device_get_le_address_type(device);
-
-	if (att && data->adapter_data->hci && data->hci_sm) {
-		if (bt_hci_get_conn_handle(data->adapter_data->hci,
-					(const uint8_t *) bdaddr, &handle)) {
-			DBG("Found conn handle 0x%04X for %s", handle, addr);
-			data->conn_handle = handle;
-			bt_rap_set_conn_hndl(data->hci_sm,
+	/* Set up connection handle mapping for CS event routing. Retried on
+	 * every call until it succeeds, since att may not be available yet
+	 * the first time this runs (e.g. called from bt_rap_attach()'s
+	 * attached callback before rap->client/att are set).
+	 */
+	if (!data->conn_handle) {
+		att = bt_rap_get_att(data->rap);
+		bdaddr = device_get_address(device);
+		bdaddr_type = device_get_le_address_type(device);
+
+		if (att && data->adapter_data->hci) {
+			if (bt_hci_get_conn_handle(data->adapter_data->hci,
+				(const uint8_t *)bdaddr, &handle)) {
+				DBG("conn handle 0x%04X for %s", handle, addr);
+				data->conn_handle = handle;
+				bt_rap_set_conn_hndl(data->hci_sm,
 					data->rap, handle,
-					(const uint8_t *) bdaddr,
+					(const uint8_t *)bdaddr,
 					bdaddr_type,
 					btd_device_is_initiator(device));
-		} else {
-			error("Failed to find connection handle for device %s",
-				addr);
+			} else {
+				error("Failed to find conn for device %s",
+					addr);
+			}
 		}
 	}
 
-	btd_service_connecting_complete(service, 0);
+	if (!data->dbus_registered) {
+		if (!g_dbus_register_interface(btd_get_dbus_connection(),
+					device_get_path(device),
+					CS_INTERFACE, cs_dbus_methods,
+					NULL, cs_dbus_properties, data, NULL)) {
+			error("Failed to register %s interface for %s",
+				CS_INTERFACE, addr);
+			return -EINVAL;
+		}
+		data->dbus_registered = true;
+	}
+
+	return 0;
+}
 
-	g_dbus_register_interface(btd_get_dbus_connection(),
-				  device_get_path(data->device),
-				  CS_INTERFACE, cs_dbus_methods,
-				  NULL, cs_dbus_properties, data, NULL);
+static int rap_accept(struct btd_service *service)
+{
+	struct btd_device *device = btd_service_get_device(service);
+	struct bt_gatt_client *client = btd_device_get_gatt_client(device);
+	struct rap_data *data = btd_service_get_user_data(service);
+	char addr[18];
+	int err;
+
+	ba2str(device_get_address(device), addr);
+	DBG("%s", addr);
+
+	if (!data) {
+		error("RAP Service not handled by profile");
+		return -EINVAL;
+	}
+
+	if (!bt_rap_attach(data->rap, client)) {
+		error("RAP unable to attach");
+		return -EINVAL;
+	}
+
+	err = rap_setup_reflector(data);
+	if (err)
+		return err;
+
+	btd_service_connecting_complete(service, 0);
 
 	return 0;
 }
@@ -814,7 +893,7 @@ static void rap_server_remove(struct btd_profile *p,
 static struct btd_profile rap_profile = {
 	.name		= "rap",
 	.priority	= BTD_PROFILE_PRIORITY_MEDIUM,
-	.remote_uuid	= GATT_UUID,
+	.remote_uuid	= RAS_UUID,
 	.local_uuid	= RAS_UUID,
 
 	.device_probe	= rap_probe,
-- 

RE: RAP: streamline session handling, reflector setup

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 987 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=1134915

---Test result---

Test Summary:
CheckPatch                    PASS      1.05 seconds
GitLint                       PASS      0.60 seconds
BuildEll                      PASS      20.50 seconds
BluezMake                     PASS      574.08 seconds
MakeCheck                     PASS      1.03 seconds
MakeDistcheck                 PASS      153.47 seconds
CheckValgrind                 PASS      146.38 seconds
CheckSmatch                   PASS      295.25 seconds
bluezmakeextell               PASS      95.22 seconds
IncrementalBuild              PASS      504.70 seconds
ScanBuild                     PASS      889.55 seconds



https://github.com/bluez/bluez/pull/2352

---
Regards,
Linux Bluetooth