[PATCH 1/2] qmimodem: Use l_queue instead of GList for notifications

[PATCH 1/2] qmimodem: Use l_queue instead of GList for notifications

[Steve Schrock]

---
 drivers/qmimodem/qmi.c | 52 ++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 268db1f7..168dee1a 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -112,7 +112,7 @@ struct qmi_service {
 	uint16_t minor;
 	uint8_t client_id;
 	uint16_t next_notify_id;
-	GList *notify_list;
+	struct l_queue *notify_list;
 };
 
 struct qmi_param {
@@ -256,7 +256,7 @@ static void __discovery_free(void *data)
 	destroy(d);
 }
 
-static void __notify_free(gpointer data, gpointer user_data)
+static void __notify_free(void *data)
 {
 	struct qmi_notify *notify = data;
 
@@ -266,12 +266,12 @@ static void __notify_free(gpointer data, gpointer user_data)
 	l_free(notify);
 }
 
-static gint __notify_compare(gconstpointer a, gconstpointer b)
+static bool __notify_compare(const void *data, const void *user_data)
 {
-	const struct qmi_notify *notify = a;
-	uint16_t id = L_PTR_TO_UINT(b);
+	const struct qmi_notify *notify = data;
+	uint16_t id = L_PTR_TO_UINT(user_data);
 
-	return notify->id - id;
+	return notify->id == id;
 }
 
 struct service_find_by_type_data {
@@ -717,23 +717,26 @@ static uint16_t __request_submit(struct qmi_device *device,
 	return req->tid;
 }
 
+static void service_notify_if_message_matches(void *data, void *user_data)
+{
+	struct qmi_notify *notify = data;
+	struct qmi_result *result = user_data;
+
+	if (notify->message == result->message)
+		notify->callback(result, notify->user_data);
+}
+
 static void service_notify(const void *key, void *value, void *user_data)
 {
 	struct qmi_service *service = value;
 	struct qmi_result *result = user_data;
-	GList *list;
 
 	/* ignore those that are in process of creation */
 	if (L_PTR_TO_UINT(key) & 0x80000000)
 		return;
 
-	for (list = g_list_first(service->notify_list); list;
-						list = g_list_next(list)) {
-		struct qmi_notify *notify = list->data;
-
-		if (notify->message == result->message)
-			notify->callback(result, notify->user_data);
-	}
+	l_queue_foreach(service->notify_list, service_notify_if_message_matches,
+				result);
 }
 
 static void handle_indication(struct qmi_device *device,
@@ -1666,6 +1669,7 @@ static void qmux_client_create_callback(uint16_t message, uint16_t length,
 	service->minor = data->minor;
 
 	service->client_id = client_id->client;
+	service->notify_list = l_queue_new();
 
 	__debug_device(device, "service created [client=%d,type=%d]",
 					service->client_id, service->type);
@@ -2452,7 +2456,7 @@ uint16_t qmi_service_register(struct qmi_service *service,
 	notify->user_data = user_data;
 	notify->destroy = destroy;
 
-	service->notify_list = g_list_append(service->notify_list, notify);
+	l_queue_push_tail(service->notify_list, notify);
 
 	return notify->id;
 }
@@ -2461,21 +2465,17 @@ bool qmi_service_unregister(struct qmi_service *service, uint16_t id)
 {
 	unsigned int nid = id;
 	struct qmi_notify *notify;
-	GList *list;
 
 	if (!service || !id)
 		return false;
 
-	list = g_list_find_custom(service->notify_list,
-				L_UINT_TO_PTR(nid), __notify_compare);
-	if (!list)
-		return false;
-
-	notify = list->data;
+	notify = l_queue_remove_if(service->notify_list, __notify_compare,
+					L_UINT_TO_PTR(nid));
 
-	service->notify_list = g_list_delete_link(service->notify_list, list);
+	if (!notify)
+		return false;
 
-	__notify_free(notify, NULL);
+	__notify_free(notify);
 
 	return true;
 }
@@ -2485,9 +2485,7 @@ bool qmi_service_unregister_all(struct qmi_service *service)
 	if (!service)
 		return false;
 
-	g_list_foreach(service->notify_list, __notify_free, NULL);
-	g_list_free(service->notify_list);
-
+	l_queue_destroy(service->notify_list, __notify_free);
 	service->notify_list = NULL;
 
 	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 2/2] qmimodem: Use l_queue instead of GList for pending service creation

[Steve Schrock]

---
 drivers/qmimodem/qmi.c | 43 ++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 168dee1a..99b3d84d 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1382,24 +1382,27 @@ static void service_create_shared_reply(struct l_idle *idle, void *user_data)
 	__qmi_device_discovery_complete(data->device, &data->super);
 }
 
+static void service_create_shared_schedule_callback(void *data, void *user_data)
+{
+	struct service_create_shared_data *shared_data = data;
+	struct qmi_service *service = user_data;
+
+	shared_data->service = qmi_service_ref(service);
+	shared_data->idle = l_idle_create(service_create_shared_reply,
+						shared_data, NULL);
+
+}
+
 static void service_create_shared_pending_reply(struct qmi_device *device,
 						unsigned int type,
 						struct qmi_service *service)
 {
-	gpointer key = L_UINT_TO_PTR(type | 0x80000000);
-	GList **shared = l_hashmap_remove(device->service_list, key);
-	GList *l;
-
-	for (l = *shared; l; l = l->next) {
-		struct service_create_shared_data *shared_data = l->data;
-
-		shared_data->service = qmi_service_ref(service);
-		shared_data->idle = l_idle_create(service_create_shared_reply,
-							shared_data, NULL);
-	}
+	void *key = L_UINT_TO_PTR(type | 0x80000000);
+	struct l_queue *shared = l_hashmap_remove(device->service_list, key);
 
-	g_list_free(*shared);
-	l_free(shared);
+	l_queue_foreach(shared, service_create_shared_schedule_callback,
+						service);
+	l_queue_destroy(shared, NULL);
 }
 
 static void service_create_shared_data_free(gpointer user_data)
@@ -1699,14 +1702,14 @@ static int qmi_device_qmux_client_create(struct qmi_device *device,
 	unsigned char client_req[] = { 0x01, 0x01, 0x00, service_type };
 	struct qmi_request *req;
 	struct qmux_client_create_data *data;
-	GList **shared;
+	struct l_queue *shared;
 	unsigned int type_val = service_type;
 	int i;
 
 	if (!device->version_list)
 		return -ENOENT;
 
-	shared = l_new(GList *, 1);
+	shared = l_queue_new();
 	data = l_new(struct qmux_client_create_data, 1);
 
 	data->super.destroy = qmux_client_create_data_free;
@@ -2148,7 +2151,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 			qmi_create_func_t func, void *user_data,
 			qmi_destroy_func_t destroy)
 {
-	GList **l = NULL;
+	struct l_queue *shared = NULL;
 	struct qmi_service *service = NULL;
 	unsigned int type_val = type;
 	int r;
@@ -2159,10 +2162,10 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 	if (type == QMI_SERVICE_CONTROL)
 		return false;
 
-	l = l_hashmap_lookup(device->service_list,
+	shared = l_hashmap_lookup(device->service_list,
 				L_UINT_TO_PTR(type_val | 0x80000000));
 
-	if (!l) {
+	if (!shared) {
 		/*
 		 * There is no way to find in an l_hashmap using a custom
 		 * function. Instead we use a temporary struct to store the
@@ -2179,7 +2182,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 	} else
 		type_val |= 0x80000000;
 
-	if (l || service) {
+	if (shared || service) {
 		struct service_create_shared_data *data;
 
 		data = l_new(struct service_create_shared_data, 1);
@@ -2195,7 +2198,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
 			data->idle = l_idle_create(service_create_shared_reply,
 							data, NULL);
 		} else
-			*l = g_list_prepend(*l, data);
+			l_queue_push_head(shared, data);
 
 		__qmi_device_discovery_started(device, &data->super);
 
-- 
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/2] qmimodem: Use l_queue instead of GList for notifications

[patchwork-bot+ofono]

Hello:

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

On Thu, 22 Feb 2024 23:53:55 +0000 you wrote:
> ---
>  drivers/qmimodem/qmi.c | 52 ++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 27 deletions(-)

Here is the summary with links:
  - [1/2] qmimodem: Use l_queue instead of GList for notifications
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=6254bf0ce2d7
  - [2/2] qmimodem: Use l_queue instead of GList for pending service creation
    (no matching commit)

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

Re: [PATCH 2/2] qmimodem: Use l_queue instead of GList for pending service creation

[Denis Kenzior]

Hi Steve,

On 2/22/24 17:53, Steve Schrock wrote:
> ---
>   drivers/qmimodem/qmi.c | 43 ++++++++++++++++++++++--------------------
>   1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
> index 168dee1a..99b3d84d 100644
> --- a/drivers/qmimodem/qmi.c
> +++ b/drivers/qmimodem/qmi.c
> @@ -1382,24 +1382,27 @@ static void service_create_shared_reply(struct l_idle *idle, void *user_data)
>   	__qmi_device_discovery_complete(data->device, &data->super);
>   }
>   
> +static void service_create_shared_schedule_callback(void *data, void *user_data)
> +{
> +	struct service_create_shared_data *shared_data = data;
> +	struct qmi_service *service = user_data;
> +
> +	shared_data->service = qmi_service_ref(service);
> +	shared_data->idle = l_idle_create(service_create_shared_reply,
> +						shared_data, NULL);
> +

nit: empty line at end of function

> +}
> +
>   static void service_create_shared_pending_reply(struct qmi_device *device,
>   						unsigned int type,
>   						struct qmi_service *service)
>   {
> -	gpointer key = L_UINT_TO_PTR(type | 0x80000000);
> -	GList **shared = l_hashmap_remove(device->service_list, key);
> -	GList *l;
> -
> -	for (l = *shared; l; l = l->next) {
> -		struct service_create_shared_data *shared_data = l->data;
> -
> -		shared_data->service = qmi_service_ref(service);
> -		shared_data->idle = l_idle_create(service_create_shared_reply,
> -							shared_data, NULL);
> -	}
> +	void *key = L_UINT_TO_PTR(type | 0x80000000);
> +	struct l_queue *shared = l_hashmap_remove(device->service_list, key);
>   
> -	g_list_free(*shared);
> -	l_free(shared);
> +	l_queue_foreach(shared, service_create_shared_schedule_callback,
> +						service);

You could use l_queue_get_entries() here to avoid a foreach / creating a new 
function if you think that will look better.  Something like:

const struct l_queue_entry *entry;

for (entry = l_queue_get_entries(q); entry; entry = entry->next)
...

It looks like this function is always called from two places:

- qmux_client_create_reply()
This call site seems to be invoked on a timeout with a NULL service.  Don't 
think that makes sense?  Should this invocation just be dropped?

- qmux_client_create_callback()
This call site happens when we receive a reply, which should be on a different 
iteration of the event loop compared to qmi_device_qmux_client_create().  Can we 
invoke the callbacks directly here instead of scheduling another l_idle callback?

> +	l_queue_destroy(shared, NULL);
>   }
>   
>   static void service_create_shared_data_free(gpointer user_data)

<snip>

> @@ -2148,7 +2151,7 @@ bool qmi_service_create_shared(struct qmi_device *device, uint16_t type,
>   			qmi_create_func_t func, void *user_data,
>   			qmi_destroy_func_t destroy)
>   {
> -	GList **l = NULL;
> +	struct l_queue *shared = NULL;

nit: This doesn't need to be initialized.  Let the compiler warn us in case 
uninitiated variables are used.

>   	struct qmi_service *service = NULL;
>   	unsigned int type_val = type;
>   	int r;

Regards,
-Denis