[PATCH 1/8] qmi unit: Link to dl

[PATCH 1/8] qmi unit: Link to dl

[Steve Schrock]

One of my VMs was getting a linker error when building
test-qmimodem-qmi in maintainer mode:
undefined reference to `dladdr'

Add -ldl to fix this.
---
 Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 9a6f82c39552..a4880036eaf5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -932,7 +932,7 @@ unit_objects += $(unit_test_mbim_OBJECTS)
 unit_test_qmimodem_qmi_SOURCES = unit/test-qmimodem-qmi.c src/common.c \
 			src/util.c src/log.c \
 			drivers/qmimodem/qmi.c
-unit_test_qmimodem_qmi_LDADD = @GLIB_LIBS@ $(ell_ldadd)
+unit_test_qmimodem_qmi_LDADD = @GLIB_LIBS@ $(ell_ldadd) -ldl
 unit_objects += $(unit_test_qmimodem_qmi_OBJECTS)
 
 unit/test-provision.db: unit/test-provision.json
-- 
2.40.1


-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

[PATCH 2/8] qmi: Eliminate atomic ref counting

[Steve Schrock]

This code is single-threaded so there is no need to use atomics.
While here simplify qmi_service_ref.
---
 drivers/qmimodem/qmi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 7f91b57d15be..0811eaf2faec 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -2660,10 +2660,8 @@ bool qmi_service_create(struct qmi_device *device,
 
 struct qmi_service *qmi_service_ref(struct qmi_service *service)
 {
-	if (!service)
-		return NULL;
-
-	__sync_fetch_and_add(&service->ref_count, 1);
+	if (service)
+		service->ref_count++;
 
 	return service;
 }
@@ -2676,7 +2674,7 @@ void qmi_service_unref(struct qmi_service *service)
 	if (!service)
 		return;
 
-	if (__sync_sub_and_fetch(&service->ref_count, 1))
+	if (--service->ref_count)
 		return;
 
 	device = service->device;
-- 
2.40.1


-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

[PATCH 3/8] qmi: Create a better client service abstraction

[Steve Schrock]

Currently any client can cancel any other client's requests and
notifications. This change separates out the service "family" which is
shared among clients that create services for the same qmi service
type. The qmi_service gets its own unique handle so that clients
are more independent and cannot interfere with other clients as
easily.
---
 drivers/qmimodem/qmi.c | 290 ++++++++++++++++++++++++++---------------
 1 file changed, 185 insertions(+), 105 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 0811eaf2faec..e1091b872f23 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -64,6 +64,7 @@ struct qmi_service_info {
 struct qmi_request {
 	uint16_t tid;
 	unsigned int group_id;		/* Always 0 for control */
+	unsigned int service_handle;	/* Always 0 for control */
 	uint8_t client;			/* Always 0 for control and qrtr */
 	struct qmi_service_info info;	/* Not used for control requests */
 	qmi_message_func_t callback;
@@ -95,11 +96,12 @@ struct qmi_device {
 	struct l_queue *service_queue;
 	struct l_queue *discovery_queue;
 	unsigned int next_group_id;	/* Matches requests with services */
+	unsigned int next_service_handle;
 	uint16_t next_service_tid;
 	qmi_debug_func_t debug_func;
 	void *debug_data;
 	struct l_queue *service_infos;
-	struct l_hashmap *service_list;
+	struct l_hashmap *family_list;
 	const struct qmi_device_ops *ops;
 	bool writer_active : 1;
 	bool shutting_down : 1;
@@ -120,7 +122,7 @@ struct qmi_device_qmux {
 	struct l_queue *control_queue;
 };
 
-struct qmi_service {
+struct service_family {
 	int ref_count;
 	struct qmi_device *device;
 	struct qmi_service_info info;
@@ -130,6 +132,12 @@ struct qmi_service {
 	struct l_queue *notify_list;
 };
 
+struct qmi_service {
+	int ref_count;
+	unsigned int handle;	/* Uniquely identifies this client's reqs */
+	struct service_family *family;
+};
+
 struct qmi_param {
 	void *data;
 	uint16_t length;
@@ -146,6 +154,7 @@ struct qmi_result {
 struct qmi_notify {
 	uint16_t id;
 	uint16_t message;
+	unsigned int service_handle;
 	qmi_result_func_t callback;
 	void *user_data;
 	qmi_destroy_func_t destroy;
@@ -239,6 +248,7 @@ static struct qmi_request *__request_alloc(uint32_t service_type,
 	req = l_malloc(sizeof(struct qmi_request) + msglen);
 	req->tid = 0;
 	req->group_id = 0;
+	req->service_handle = 0;
 	req->len = msglen;
 	req->client = client;
 
@@ -330,21 +340,21 @@ static bool __notify_compare(const void *data, const void *user_data)
 
 struct service_find_by_type_data {
 	unsigned int type;
-	struct qmi_service *found_service;
+	struct service_family *found_family;
 };
 
-static void __service_find_by_type(const void *key, void *value,
+static void __family_find_by_type(const void *key, void *value,
 					void *user_data)
 {
-	struct qmi_service *service = value;
+	struct service_family *family = value;
 	struct service_find_by_type_data *data = user_data;
 
 	/* ignore those that are in process of creation */
 	if (L_PTR_TO_UINT(key) & 0x80000000)
 		return;
 
-	if (service->info.service_type == data->type)
-		data->found_service = service;
+	if (family->info.service_type == data->type)
+		data->found_family = family;
 }
 
 static const char *__service_type_to_string(uint8_t type)
@@ -744,7 +754,8 @@ static uint16_t __service_request_submit(struct qmi_device *device,
 	if (device->next_service_tid < 256)
 		device->next_service_tid = 256;
 
-	req->group_id = service->group_id;
+	req->group_id = service->family->group_id;
+	req->service_handle = service->handle;
 
 	hdr->type = 0x00;
 	hdr->transaction = L_CPU_TO_LE16(req->tid);
@@ -766,18 +777,18 @@ static void service_notify_if_message_matches(void *data, void *user_data)
 
 static void service_notify(const void *key, void *value, void *user_data)
 {
-	struct qmi_service *service = value;
+	struct service_family *family = value;
 	struct qmi_result *result = user_data;
 
 	/* ignore those that are in process of creation */
 	if (L_PTR_TO_UINT(key) & 0x80000000)
 		return;
 
-	l_queue_foreach(service->notify_list, service_notify_if_message_matches,
+	l_queue_foreach(family->notify_list, service_notify_if_message_matches,
 				result);
 }
 
-static unsigned int service_list_create_hash(uint16_t service_type,
+static unsigned int family_list_create_hash(uint16_t service_type,
 							uint8_t client_id)
 {
 	return (service_type | (client_id << 16));
@@ -787,7 +798,7 @@ static void handle_indication(struct qmi_device *device,
 			uint32_t service_type, uint8_t client_id,
 			uint16_t message, uint16_t length, const void *data)
 {
-	struct qmi_service *service;
+	struct service_family *family;
 	struct qmi_result result;
 	unsigned int hash_id;
 
@@ -801,19 +812,19 @@ static void handle_indication(struct qmi_device *device,
 	result.length = length;
 
 	if (client_id == 0xff) {
-		l_hashmap_foreach(device->service_list, service_notify,
+		l_hashmap_foreach(device->family_list, service_notify,
 					&result);
 		return;
 	}
 
-	hash_id = service_list_create_hash(service_type, client_id);
-	service = l_hashmap_lookup(device->service_list,
+	hash_id = family_list_create_hash(service_type, client_id);
+	family = l_hashmap_lookup(device->family_list,
 					L_UINT_TO_PTR(hash_id));
 
-	if (!service)
+	if (!family)
 		return;
 
-	service_notify(NULL, service, &result);
+	service_notify(NULL, family, &result);
 }
 
 static void __rx_message(struct qmi_device *device,
@@ -876,14 +887,14 @@ do {\
 	__discovery_free(&data->super);\
 } while (0)
 
-static void service_destroy(void *data)
+static void family_destroy(void *data)
 {
-	struct qmi_service *service = data;
+	struct service_family *family = data;
 
-	if (!service->device)
+	if (!family->device)
 		return;
 
-	service->device = NULL;
+	family->device = NULL;
 }
 
 static int qmi_device_init(struct qmi_device *device, int fd,
@@ -911,7 +922,7 @@ static int qmi_device_init(struct qmi_device *device, int fd,
 	device->service_queue = l_queue_new();
 	device->discovery_queue = l_queue_new();
 	device->service_infos = l_queue_new();
-	device->service_list = l_hashmap_new();
+	device->family_list = l_hashmap_new();
 
 	device->next_service_tid = 256;
 
@@ -939,7 +950,7 @@ void qmi_device_free(struct qmi_device *device)
 
 	l_io_destroy(device->io);
 
-	l_hashmap_destroy(device->service_list, service_destroy);
+	l_hashmap_destroy(device->family_list, family_destroy);
 
 	l_queue_destroy(device->service_infos, l_free);
 
@@ -1400,9 +1411,41 @@ static bool received_qmux_data(struct l_io *io, void *user_data)
 	return true;
 }
 
+static struct service_family *service_family_ref(struct service_family *family)
+{
+	family->ref_count++;
+
+	return family;
+}
+
+static void service_family_unref(struct service_family *family)
+{
+	struct qmi_device *device;
+	unsigned int hash_id;
+
+	if (--family->ref_count)
+		return;
+
+	device = family->device;
+	if (!device)
+		goto done;
+
+	hash_id = family_list_create_hash(family->info.service_type,
+							family->client_id);
+	l_hashmap_remove(device->family_list, L_UINT_TO_PTR(hash_id));
+
+	if (device->ops->client_release)
+		device->ops->client_release(device, family->info.service_type,
+							family->client_id);
+
+done:
+	l_queue_destroy(family->notify_list, NULL);
+	l_free(family);
+}
+
 struct service_create_shared_data {
 	struct discovery super;
-	struct qmi_service *service;
+	struct service_family *family;
 	struct qmi_device *device;
 	qmi_create_func_t func;
 	void *user_data;
@@ -1430,26 +1473,46 @@ static uint8_t __ctl_request_submit(struct qmi_device_qmux *qmux,
 	return req->tid;
 }
 
-static struct qmi_service *service_create(struct qmi_device *device,
+static struct service_family *service_family_create(struct qmi_device *device,
 			const struct qmi_service_info *info, uint8_t client_id)
 {
-	struct qmi_service *service = l_new(struct qmi_service, 1);
+	struct service_family *family = l_new(struct service_family, 1);
 
-	service->ref_count = 1;
-	service->device = device;
-	service->client_id = client_id;
-	service->notify_list = l_queue_new();
+	family->ref_count = 0;
+	family->device = device;
+	family->client_id = client_id;
+	family->notify_list = l_queue_new();
 
 	if (device->next_group_id == 0) /* 0 is reserved for control */
 		device->next_group_id = 1;
 
-	service->group_id = device->next_group_id++;
+	family->group_id = device->next_group_id++;
+
+	memcpy(&family->info, info, sizeof(family->info));
+
+	__debug_device(device, "service family created [client=%d,type=%d]",
+					family->client_id,
+					family->info.service_type);
+
+	return family;
+}
+
+static struct qmi_service *service_create(struct service_family *family)
+{
+	struct qmi_device *device = family->device;
+	struct qmi_service *service;
 
-	memcpy(&service->info, info, sizeof(service->info));
+	if (device->next_service_handle == 0) /* 0 is reserved for control */
+		device->next_service_handle = 1;
+
+	service = l_new(struct qmi_service, 1);
+	service->ref_count = 1;
+	service->handle = device->next_service_handle++;
+	service->family = service_family_ref(family);
 
 	__debug_device(device, "service created [client=%d,type=%d]",
-					service->client_id,
-					service->info.service_type);
+					family->client_id,
+					family->info.service_type);
 
 	return service;
 }
@@ -1457,25 +1520,28 @@ static struct qmi_service *service_create(struct qmi_device *device,
 static void service_create_shared_reply(struct l_idle *idle, void *user_data)
 {
 	struct service_create_shared_data *data = user_data;
+	struct qmi_service *service;
 
 	l_idle_remove(data->idle);
 	data->idle = NULL;
 
-	DISCOVERY_DONE(data, data->service, data->user_data);
+	service = service_create(data->family);
+	DISCOVERY_DONE(data, service, data->user_data);
+	qmi_service_unref(service);
 }
 
 static void service_create_shared_pending_reply(struct qmi_device *device,
 						unsigned int type,
-						struct qmi_service *service)
+						struct service_family *family)
 {
 	void *key = L_UINT_TO_PTR(type | 0x80000000);
-	struct l_queue *shared = l_hashmap_remove(device->service_list, key);
+	struct l_queue *shared = l_hashmap_remove(device->family_list, key);
 	const struct l_queue_entry *entry;
 
 	for (entry = l_queue_get_entries(shared); entry; entry = entry->next) {
 		struct service_create_shared_data *shared_data = entry->data;
 
-		shared_data->service = qmi_service_ref(service);
+		shared_data->family = service_family_ref(family);
 		shared_data->idle = l_idle_create(service_create_shared_reply,
 							shared_data, NULL);
 	}
@@ -1490,7 +1556,7 @@ static void service_create_shared_data_free(void *user_data)
 	if (data->idle)
 		l_idle_remove(data->idle);
 
-	qmi_service_unref(data->service);
+	service_family_unref(data->family);
 
 	if (data->destroy)
 		data->destroy(data->user_data);
@@ -1727,8 +1793,9 @@ static void qmux_client_create_callback(uint16_t message, uint16_t length,
 {
 	struct qmux_client_create_data *data = user_data;
 	struct qmi_device *device = data->device;
+	struct service_family *family = NULL;
+	struct service_family *old_family = NULL;
 	struct qmi_service *service = NULL;
-	struct qmi_service *old_service = NULL;
 	struct qmi_service_info info;
 	const struct qmi_result_code *result_code;
 	const struct qmi_client_id *client_id;
@@ -1752,23 +1819,25 @@ static void qmux_client_create_callback(uint16_t message, uint16_t length,
 	if (client_id->service != data->type)
 		goto done;
 
-	memset(&info, 0, sizeof(service->info));
+	memset(&info, 0, sizeof(family->info));
 	info.service_type = data->type;
 	info.major = data->major;
 	info.minor = data->minor;
 
-	service = service_create(device, &info, client_id->client);
+	family = service_family_create(device, &info, client_id->client);
 
-	hash_id = service_list_create_hash(service->info.service_type,
-							service->client_id);
-	l_hashmap_replace(device->service_list, L_UINT_TO_PTR(hash_id),
-				service, (void **) &old_service);
+	hash_id = family_list_create_hash(family->info.service_type,
+							family->client_id);
+	l_hashmap_replace(device->family_list, L_UINT_TO_PTR(hash_id),
+				family, (void **) &old_family);
 
-	if (old_service)
-		service_destroy(old_service);
+	if (old_family)
+		family_destroy(old_family);
+
+	service = service_create(family);
 
 done:
-	service_create_shared_pending_reply(device, data->type, service);
+	service_create_shared_pending_reply(device, data->type, family);
 
 	DISCOVERY_DONE(data, service, data->user_data);
 	qmi_service_unref(service);
@@ -1816,7 +1885,7 @@ static int qmi_device_qmux_client_create(struct qmi_device *device,
 	__qmi_device_discovery_started(device, &data->super);
 
 	/* Mark service creation as pending */
-	l_hashmap_insert(device->service_list,
+	l_hashmap_insert(device->family_list,
 			L_UINT_TO_PTR(type_val | 0x80000000), shared);
 
 	return 0;
@@ -2553,7 +2622,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 			qmi_destroy_func_t destroy)
 {
 	struct l_queue *shared;
-	struct qmi_service *service = NULL;
+	struct service_family *family = NULL;
 	unsigned int type_val = type;
 	int r;
 
@@ -2570,20 +2639,19 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 		 * The hash id is simply the service type in this case. There
 		 * is no "pending" state for discovery and no client id.
 		 */
-		service = l_hashmap_lookup(device->service_list,
+		family = l_hashmap_lookup(device->family_list,
 						L_UINT_TO_PTR(type_val));
-		if (!service) {
+		if (!family) {
 			const struct qmi_service_info *info;
 
 			info = __find_service_info_by_type(device, type);
 			if (!info)
 				return false;
 
-			service = service_create(device, info, 0);
-			l_hashmap_insert(device->service_list,
-					L_UINT_TO_PTR(type_val), service);
-		} else
-			service = qmi_service_ref(service);
+			family = service_family_create(device, info, 0);
+			l_hashmap_insert(device->family_list,
+					L_UINT_TO_PTR(type_val), family);
+		}
 
 		data = l_new(struct service_create_shared_data, 1);
 
@@ -2592,7 +2660,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 		data->func = func;
 		data->user_data = user_data;
 		data->destroy = destroy;
-		data->service = service;
+		data->family = service_family_ref(family);
 
 		data->idle = l_idle_create(service_create_shared_reply,
 							data, NULL);
@@ -2603,7 +2671,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 		return true;
 	}
 
-	shared = l_hashmap_lookup(device->service_list,
+	shared = l_hashmap_lookup(device->family_list,
 					L_UINT_TO_PTR(type_val | 0x80000000));
 
 	if (!shared) {
@@ -2616,14 +2684,14 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 		struct service_find_by_type_data data;
 
 		data.type = type_val;
-		data.found_service = NULL;
-		l_hashmap_foreach(device->service_list,	__service_find_by_type,
+		data.found_family = NULL;
+		l_hashmap_foreach(device->family_list,	__family_find_by_type,
 					&data);
-		service = data.found_service;
+		family = data.found_family;
 	} else
 		type_val |= 0x80000000;
 
-	if (shared || service) {
+	if (shared || family) {
 		struct service_create_shared_data *data;
 
 		data = l_new(struct service_create_shared_data, 1);
@@ -2635,7 +2703,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 		data->destroy = destroy;
 
 		if (!(type_val & 0x80000000)) {
-			data->service = qmi_service_ref(service);
+			data->family = service_family_ref(family);
 			data->idle = l_idle_create(service_create_shared_reply,
 							data, NULL);
 		} else
@@ -2668,31 +2736,16 @@ struct qmi_service *qmi_service_ref(struct qmi_service *service)
 
 void qmi_service_unref(struct qmi_service *service)
 {
-	struct qmi_device *device;
-	unsigned int hash_id;
-
 	if (!service)
 		return;
 
 	if (--service->ref_count)
 		return;
 
-	device = service->device;
-	if (!device) {
-		l_free(service);
-		return;
-	}
-
 	qmi_service_cancel_all(service);
 	qmi_service_unregister_all(service);
 
-	hash_id = service_list_create_hash(service->info.service_type,
-							service->client_id);
-	l_hashmap_remove(device->service_list, L_UINT_TO_PTR(hash_id));
-
-	if (device->ops->client_release)
-		device->ops->client_release(device, service->info.service_type,
-							service->client_id);
+	service_family_unref(service->family);
 
 	l_free(service);
 }
@@ -2702,7 +2755,7 @@ const char *qmi_service_get_identifier(struct qmi_service *service)
 	if (!service)
 		return NULL;
 
-	return __service_type_to_string(service->info.service_type);
+	return __service_type_to_string(service->family->info.service_type);
 }
 
 bool qmi_service_get_version(struct qmi_service *service,
@@ -2712,10 +2765,10 @@ bool qmi_service_get_version(struct qmi_service *service,
 		return false;
 
 	if (major)
-		*major = service->info.major;
+		*major = service->family->info.major;
 
 	if (minor)
-		*minor = service->info.minor;
+		*minor = service->family->info.minor;
 
 	return true;
 }
@@ -2769,6 +2822,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 				void *user_data, qmi_destroy_func_t destroy)
 {
 	struct qmi_device *device;
+	struct service_family *family;
 	struct service_send_data *data;
 	struct qmi_request *req;
 	uint16_t tid;
@@ -2776,10 +2830,12 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	if (!service)
 		return 0;
 
-	if (!service->group_id)
+	family = service->family;
+
+	if (!family->group_id)
 		return 0;
 
-	device = service->device;
+	device = family->device;
 	if (!device)
 		return 0;
 
@@ -2789,8 +2845,8 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	req = __service_request_alloc(&service->info,
-					service->client_id, message,
+	req = __service_request_alloc(&family->info,
+					family->client_id, message,
 					param ? param->data : NULL,
 					param ? param->length : 0,
 					service_send_callback, data);
@@ -2807,14 +2863,17 @@ bool qmi_service_cancel(struct qmi_service *service, uint16_t id)
 	unsigned int tid = id;
 	struct qmi_device *device;
 	struct qmi_request *req;
+	struct service_family *family;
 
 	if (!service || !tid)
 		return false;
 
-	if (!service->client_id)
+	family = service->family;
+
+	if (!family->client_id)
 		return false;
 
-	device = service->device;
+	device = family->device;
 	if (!device)
 		return false;
 
@@ -2838,9 +2897,9 @@ bool qmi_service_cancel(struct qmi_service *service, uint16_t id)
 static bool remove_req_if_match(void *data, void *user_data)
 {
 	struct qmi_request *req = data;
-	unsigned int group_id = L_PTR_TO_UINT(user_data);
+	unsigned int service_handle = L_PTR_TO_UINT(user_data);
 
-	if (req->group_id != group_id)
+	if (req->service_handle != service_handle)
 		return false;
 
 	service_send_free(req->user_data);
@@ -2849,10 +2908,10 @@ static bool remove_req_if_match(void *data, void *user_data)
 	return true;
 }
 
-static void remove_client(struct l_queue *queue, unsigned int group_id)
+static void remove_client(struct l_queue *queue, unsigned int service_handle)
 {
 	l_queue_foreach_remove(queue, remove_req_if_match,
-				L_UINT_TO_PTR(group_id));
+				L_UINT_TO_PTR(service_handle));
 }
 
 bool qmi_service_cancel_all(struct qmi_service *service)
@@ -2862,15 +2921,15 @@ bool qmi_service_cancel_all(struct qmi_service *service)
 	if (!service)
 		return false;
 
-	if (!service->group_id)
+	if (!service->family->group_id)
 		return false;
 
-	device = service->device;
+	device = service->family->device;
 	if (!device)
 		return false;
 
-	remove_client(device->req_queue, service->group_id);
-	remove_client(device->service_queue, service->group_id);
+	remove_client(device->req_queue, service->handle);
+	remove_client(device->service_queue, service->handle);
 
 	return true;
 }
@@ -2880,22 +2939,26 @@ uint16_t qmi_service_register(struct qmi_service *service,
 				void *user_data, qmi_destroy_func_t destroy)
 {
 	struct qmi_notify *notify;
+	struct service_family *family;
 
 	if (!service || !func)
 		return 0;
 
+	family = service->family;
+
 	notify = l_new(struct qmi_notify, 1);
 
-	if (service->next_notify_id < 1)
-		service->next_notify_id = 1;
+	if (family->next_notify_id < 1)
+		family->next_notify_id = 1;
 
-	notify->id = service->next_notify_id++;
+	notify->id = family->next_notify_id++;
 	notify->message = message;
+	notify->service_handle = service->handle;
 	notify->callback = func;
 	notify->user_data = user_data;
 	notify->destroy = destroy;
 
-	l_queue_push_tail(service->notify_list, notify);
+	l_queue_push_tail(family->notify_list, notify);
 
 	return notify->id;
 }
@@ -2903,12 +2966,15 @@ uint16_t qmi_service_register(struct qmi_service *service,
 bool qmi_service_unregister(struct qmi_service *service, uint16_t id)
 {
 	unsigned int nid = id;
+	struct service_family *family;
 	struct qmi_notify *notify;
 
 	if (!service || !id)
 		return false;
 
-	notify = l_queue_remove_if(service->notify_list, __notify_compare,
+	family = service->family;
+
+	notify = l_queue_remove_if(family->notify_list, __notify_compare,
 					L_UINT_TO_PTR(nid));
 
 	if (!notify)
@@ -2919,13 +2985,27 @@ bool qmi_service_unregister(struct qmi_service *service, uint16_t id)
 	return true;
 }
 
+static bool remove_notify_if_handle_match(void *data, void *user_data)
+{
+	struct qmi_notify *notify = data;
+	unsigned int handle = L_PTR_TO_UINT(user_data);
+
+	if (notify->service_handle != handle)
+		return false;
+
+	__notify_free(notify);
+
+	return true;
+}
+
 bool qmi_service_unregister_all(struct qmi_service *service)
 {
 	if (!service)
 		return false;
 
-	l_queue_destroy(service->notify_list, __notify_free);
-	service->notify_list = NULL;
+	l_queue_foreach_remove(service->family->notify_list,
+					remove_notify_if_handle_match,
+					L_UINT_TO_PTR(service->handle));
 
 	return true;
 }
-- 
2.40.1


-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

[PATCH 4/8] qmi: Eliminate unnecessary casting to unsigned int

[Steve Schrock]

uint16_t values were copied into unsigned ints before being passed to
L_UINT_TO_PTR. Perhaps this was necessary when the glib macro
GUINT_TO_POINTER was used, but it is not necessary now with the ell macro.
---
 drivers/qmimodem/qmi.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index e1091b872f23..f406d01a8df6 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1330,7 +1330,6 @@ static void __rx_ctl_message(struct qmi_device_qmux *qmux,
 	struct qmi_request *req;
 	uint16_t message;
 	uint16_t length;
-	unsigned int tid;
 
 	/* Ignore control messages with client identifier */
 	if (client_id != 0x00)
@@ -1338,7 +1337,6 @@ static void __rx_ctl_message(struct qmi_device_qmux *qmux,
 
 	message = L_LE16_TO_CPU(msg->message);
 	length = L_LE16_TO_CPU(msg->length);
-	tid = control->transaction;
 
 	if (control->type == 0x02 && control->transaction == 0x00) {
 		handle_indication(&qmux->super, service_type, client_id,
@@ -1347,7 +1345,7 @@ static void __rx_ctl_message(struct qmi_device_qmux *qmux,
 	}
 
 	req = l_queue_remove_if(qmux->control_queue, __request_compare,
-						L_UINT_TO_PTR(tid));
+					L_UINT_TO_PTR(control->transaction));
 	if (!req)
 		return;
 
@@ -2860,12 +2858,11 @@ uint16_t qmi_service_send(struct qmi_service *service,
 
 bool qmi_service_cancel(struct qmi_service *service, uint16_t id)
 {
-	unsigned int tid = id;
 	struct qmi_device *device;
 	struct qmi_request *req;
 	struct service_family *family;
 
-	if (!service || !tid)
+	if (!service || !id)
 		return false;
 
 	family = service->family;
@@ -2878,11 +2875,11 @@ bool qmi_service_cancel(struct qmi_service *service, uint16_t id)
 		return false;
 
 	req = l_queue_remove_if(device->req_queue, __request_compare,
-					L_UINT_TO_PTR(tid));
+					L_UINT_TO_PTR(id));
 	if (!req) {
 		req = l_queue_remove_if(device->service_queue,
 						__request_compare,
-						L_UINT_TO_PTR(tid));
+						L_UINT_TO_PTR(id));
 		if (!req)
 			return false;
 	}
@@ -2965,7 +2962,6 @@ uint16_t qmi_service_register(struct qmi_service *service,
 
 bool qmi_service_unregister(struct qmi_service *service, uint16_t id)
 {
-	unsigned int nid = id;
 	struct service_family *family;
 	struct qmi_notify *notify;
 
@@ -2975,7 +2971,7 @@ bool qmi_service_unregister(struct qmi_service *service, uint16_t id)
 	family = service->family;
 
 	notify = l_queue_remove_if(family->notify_list, __notify_compare,
-					L_UINT_TO_PTR(nid));
+					L_UINT_TO_PTR(id));
 
 	if (!notify)
 		return false;
-- 
2.40.1


-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

[PATCH 5/8] qmi: Prevent clients from unregistering for others

[Steve Schrock]

qmi_service_unregister was removing the registration that matched an
integer ID. This would allow a client to unregister a different
client's notification. While this is unlikely it could lead to very
confusing bugs. This is easy to prevent by checking both the ID and
the service handle.
---
 drivers/qmimodem/qmi.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index f406d01a8df6..5032233ec1ec 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -330,12 +330,18 @@ static void __notify_free(void *data)
 	l_free(notify);
 }
 
+struct notify_compare_details {
+	uint16_t id;
+	unsigned int service_handle;
+};
+
 static bool __notify_compare(const void *data, const void *user_data)
 {
 	const struct qmi_notify *notify = data;
-	uint16_t id = L_PTR_TO_UINT(user_data);
+	const struct notify_compare_details *details = user_data;
 
-	return notify->id == id;
+	return notify->id == details->id &&
+			notify->service_handle == details->service_handle;
 }
 
 struct service_find_by_type_data {
@@ -2962,16 +2968,17 @@ uint16_t qmi_service_register(struct qmi_service *service,
 
 bool qmi_service_unregister(struct qmi_service *service, uint16_t id)
 {
-	struct service_family *family;
 	struct qmi_notify *notify;
+	struct notify_compare_details details;
 
 	if (!service || !id)
 		return false;
 
-	family = service->family;
+	details.id = id;
+	details.service_handle = service->handle;
 
-	notify = l_queue_remove_if(family->notify_list, __notify_compare,
-					L_UINT_TO_PTR(id));
+	notify = l_queue_remove_if(service->family->notify_list,
+						__notify_compare, &details);
 
 	if (!notify)
 		return false;
-- 
2.40.1


-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

[PATCH 6/8] qmi unit: Validate creation of services of the same type

[Steve Schrock]

Confirm that there are no problems when clients create services for
the same qmi type.
---
 unit/test-qmimodem-qmi.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/unit/test-qmimodem-qmi.c b/unit/test-qmimodem-qmi.c
index 0e15f898f845..8df8b56a79eb 100644
--- a/unit/test-qmimodem-qmi.c
+++ b/unit/test-qmimodem-qmi.c
@@ -221,8 +221,9 @@ static void perform_all_pending_work(void)
 static void test_create_services(const void *data)
 {
 	struct test_info *info = test_setup();
+	struct qmi_service *services[3];
 	uint32_t service_type;
-	int i;
+	size_t i;
 
 	perform_discovery(info);
 
@@ -256,6 +257,22 @@ static void test_create_services(const void *data)
 	perform_all_pending_work();
 	assert(l_queue_isempty(info->services));
 
+	/* Confirm that multiple services may be created for the same type */
+	service_type = unique_service_type(0);
+
+	for (i = 0; i < L_ARRAY_SIZE(services); i++) {
+		assert(qmi_service_create(info->device, service_type,
+						create_service_cb, info, NULL));
+		perform_all_pending_work();
+
+		assert(l_queue_length(info->services) == 1);
+		services[i] = l_queue_pop_head(info->services);
+		assert(services[i]);
+	}
+
+	for (i = 0; i < L_ARRAY_SIZE(services); i++)
+		qmi_service_unref(services[i]);
+
 	test_cleanup(info);
 }
 
-- 
2.40.1


-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

[PATCH 7/8] qmi unit: Validate destroyed services do not notify

[Steve Schrock]

Confirm that client notifications do occur after unref'ing (destroying)
the service.
---
 unit/test-qmimodem-qmi.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/unit/test-qmimodem-qmi.c b/unit/test-qmimodem-qmi.c
index 8df8b56a79eb..43e1bad082ba 100644
--- a/unit/test-qmimodem-qmi.c
+++ b/unit/test-qmimodem-qmi.c
@@ -23,6 +23,13 @@
 #define TEST_SERVICE_COUNT	2
 #define TEST_TIMEOUT		5
 
+/*
+ * The amount of time to wait to validate that something did NOT occur. The
+ * value is fairly arbitrary -- the longer it is, the longer the tests will take
+ * to complete.
+ */
+#define ALLOWED_QRTR_TRANSFER_TIME 100 /* ms */
+
 struct test_info {
 	int service_fds[TEST_SERVICE_COUNT];
 	struct qmi_device *device;
@@ -36,6 +43,7 @@ struct test_info {
 
 	bool discovery_callback_called		: 1;
 	bool service_send_callback_called	: 1;
+	bool internal_timeout_callback_called	: 1;
 	bool notify_callback_called		: 1;
 };
 
@@ -473,12 +481,20 @@ static void notify_cb(struct qmi_result *result, void *user_data)
 	info->notify_callback_called = true;
 }
 
+static void internal_timeout_cb(struct l_timeout *timeout, void *user_data)
+{
+	struct test_info *info = user_data;
+
+	info->internal_timeout_callback_called = true;
+}
+
 static void test_notifications(const void *data)
 {
 	struct test_info *info = test_setup();
 	struct l_io *io;
 	uint32_t service_type;
 	struct qmi_service *service;
+	struct l_timeout *receive_timeout;
 
 	perform_discovery(info);
 
@@ -505,9 +521,26 @@ static void test_notifications(const void *data)
 	while (!info->notify_callback_called)
 		l_main_iterate(-1);
 
-	l_io_destroy(io);
 	qmi_service_unref(service);
 
+	/* Confirm no notifications received after the service is destroyed */
+	info->notify_callback_called = false;
+	send_message_to_client(&info->sender, io, QMI_MESSAGE_TYPE_IND, 0,
+						TEST_IND_MESSAGE_ID,
+						TEST_IND_DATA_VALUE);
+
+	receive_timeout = l_timeout_create_ms(ALLOWED_QRTR_TRANSFER_TIME,
+						internal_timeout_cb, info,
+						NULL);
+
+	while (!info->internal_timeout_callback_called)
+		perform_all_pending_work();
+
+	assert(!info->notify_callback_called);
+
+	l_timeout_remove(receive_timeout);
+
+	l_io_destroy(io);
 	test_cleanup(info);
 }
 
-- 
2.40.1


-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

[PATCH 8/8] qmi unit: Validate notifications are independent

[Steve Schrock]

Destroying one service should not affect notifications for different
services of the same type.
---
 unit/test-qmimodem-qmi.c | 57 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/unit/test-qmimodem-qmi.c b/unit/test-qmimodem-qmi.c
index 43e1bad082ba..7d1a33eff9b1 100644
--- a/unit/test-qmimodem-qmi.c
+++ b/unit/test-qmimodem-qmi.c
@@ -47,6 +47,13 @@ struct test_info {
 	bool notify_callback_called		: 1;
 };
 
+static void info_clear_received(struct test_info *info)
+{
+	l_free(info->received);
+	info->received = NULL;
+	info->received_len = 0;
+}
+
 static uint32_t unique_service_type(uint32_t index)
 {
 	/* Try to use a value that will not conflict with any real services. */
@@ -544,6 +551,54 @@ static void test_notifications(const void *data)
 	test_cleanup(info);
 }
 
+static void test_service_notification_independence(const void *data)
+{
+	struct test_info *info = test_setup();
+	struct l_io *io;
+	uint32_t service_type;
+	struct qmi_service *services[2];
+	size_t i;
+
+	perform_discovery(info);
+
+	service_type = unique_service_type(0); /* Use the first service */
+
+	io = l_io_new(info->service_fds[0]);
+	assert(io);
+	l_io_set_read_handler(io, received_data, info, NULL);
+
+	for (i = 0; i < L_ARRAY_SIZE(services); i++) {
+		assert(qmi_service_create(info->device, service_type,
+						create_service_cb, info, NULL));
+		perform_all_pending_work();
+		services[i] = l_queue_pop_head(info->services);
+		assert(services[i]);
+
+		send_request_via_qmi(info, services[i]);
+		send_response_to_client(info, io);
+
+		qmi_service_register(services[i], TEST_IND_MESSAGE_ID,
+						notify_cb, info, NULL);
+
+		info_clear_received(info);
+	}
+
+	qmi_service_unref(services[0]);
+
+	send_message_to_client(&info->sender, io, QMI_MESSAGE_TYPE_IND, 0,
+						TEST_IND_MESSAGE_ID,
+						TEST_IND_DATA_VALUE);
+
+	while (!info->notify_callback_called)
+		l_main_iterate(-1);
+
+	for (i = 1; i < L_ARRAY_SIZE(services); i++)
+		qmi_service_unref(services[i]);
+
+	l_io_destroy(io);
+	test_cleanup(info);
+}
+
 static void exit_if_qrtr_not_supported(void)
 {
 	int fd;
@@ -574,6 +629,8 @@ int main(int argc, char **argv)
 	l_test_add("QRTR services may be created", test_create_services, NULL);
 	l_test_add("QRTR service sends/responses", test_send_data, NULL);
 	l_test_add("QRTR notifications", test_notifications, NULL);
+	l_test_add("QRTR service notifications are independent",
+				test_service_notification_independence, NULL);
 	result = l_test_run();
 
 	__ofono_log_cleanup();
-- 
2.40.1


-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

Re: [PATCH 1/8] qmi unit: Link to dl

[Marcel Holtmann]

Hi Steve,

> One of my VMs was getting a linker error when building
> test-qmimodem-qmi in maintainer mode:
> undefined reference to `dladdr'
> 
> Add -ldl to fix this.
> ---
> Makefile.am | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 9a6f82c39552..a4880036eaf5 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -932,7 +932,7 @@ unit_objects += $(unit_test_mbim_OBJECTS)
> unit_test_qmimodem_qmi_SOURCES = unit/test-qmimodem-qmi.c src/common.c \
> src/util.c src/log.c \
> drivers/qmimodem/qmi.c
> -unit_test_qmimodem_qmi_LDADD = @GLIB_LIBS@ $(ell_ldadd)
> +unit_test_qmimodem_qmi_LDADD = @GLIB_LIBS@ $(ell_ldadd) -ldl

this depends on HAVE_BACKTRACE.

Regards

Marcel

Re: [EXT] Re: [PATCH 1/8] qmi unit: Link to dl

[Steve Schrock]

Thanks for the explanation, Marcel. That makes sense -- the VM that
had the linker error is a fairly minimal installation.

Steve


On Fri, Apr 19, 2024 at 11:53 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Steve,
>
> > One of my VMs was getting a linker error when building
> > test-qmimodem-qmi in maintainer mode:
> > undefined reference to `dladdr'
> >
> > Add -ldl to fix this.
> > ---
> > Makefile.am | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 9a6f82c39552..a4880036eaf5 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -932,7 +932,7 @@ unit_objects += $(unit_test_mbim_OBJECTS)
> > unit_test_qmimodem_qmi_SOURCES = unit/test-qmimodem-qmi.c src/common.c \
> > src/util.c src/log.c \
> > drivers/qmimodem/qmi.c
> > -unit_test_qmimodem_qmi_LDADD = @GLIB_LIBS@ $(ell_ldadd)
> > +unit_test_qmimodem_qmi_LDADD = @GLIB_LIBS@ $(ell_ldadd) -ldl
>
> this depends on HAVE_BACKTRACE.
>
> Regards
>
> Marcel
>

-- 


*Confidentiality Note:* We care about protecting our proprietary 
information, confidential material, and trade secrets. This message may 
contain some or all of those things. Cruise will suffer material harm if 
anyone other than the intended recipient disseminates or takes any action 
based on this message. If you have received this message (including any 
attachments) in error, please delete it immediately and notify the sender 
promptly.

Re: [PATCH 1/8] qmi unit: Link to dl

[patchwork-bot+ofono]

Hello:

This series was applied to ofono.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Fri, 19 Apr 2024 16:44:51 +0000 you wrote:
> One of my VMs was getting a linker error when building
> test-qmimodem-qmi in maintainer mode:
> undefined reference to `dladdr'
> 
> Add -ldl to fix this.
> ---
>  Makefile.am | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Here is the summary with links:
  - [1/8] qmi unit: Link to dl
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=279b04c15127
  - [2/8] qmi: Eliminate atomic ref counting
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=39b7437a0044
  - [3/8] qmi: Create a better client service abstraction
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=6131bfc81468
  - [4/8] qmi: Eliminate unnecessary casting to unsigned int
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=28d60206ba2d
  - [5/8] qmi: Prevent clients from unregistering for others
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=e13fb8f498cc
  - [6/8] qmi unit: Validate creation of services of the same type
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=af2718346a28
  - [7/8] qmi unit: Validate destroyed services do not notify
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=2d4e571d1598
  - [8/8] qmi unit: Validate notifications are independent
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=1c999cd5a0a8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html