[PATCH v2] qmimodem: include shutdown in callback cleanup queue

[PATCH v2] qmimodem: include shutdown in callback cleanup queue

[Lukasz Nowak]

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

From: Lukasz Nowak <lnowak@tycoint.com>

Changes since v1:
- fixed callback->timeout not being saved in qmi_device_shutdown()

Lukasz Nowak (1):
  qmimodem: include shutdown in callback cleanup queue

 drivers/qmimodem/qmi.c | 401 ++++++++++++++++++++++---------------------------
 1 file changed, 180 insertions(+), 221 deletions(-)

-- 
2.7.4

[PATCH v2] qmimodem: include shutdown in callback cleanup queue

[Lukasz Nowak]

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

From: Lukasz Nowak <lnowak@tycoint.com>

Create a unified callback_data structure, used for all timeout callbacks.
This allows to maintain a single callback_queue, which cancels any active
timeouts, and frees callback data memory when a usb device is hot unplugged.
---
 drivers/qmimodem/qmi.c | 401 ++++++++++++++++++++++---------------------------
 1 file changed, 180 insertions(+), 221 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 9b80455..ec38a87 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -43,11 +43,6 @@
 typedef void (*qmi_message_func_t)(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data);
 
-struct discovery {
-	void *discover_data;
-	qmi_destroy_func_t destroy;
-};
-
 struct qmi_device {
 	int ref_count;
 	int fd;
@@ -58,7 +53,7 @@ struct qmi_device {
 	GQueue *req_queue;
 	GQueue *control_queue;
 	GQueue *service_queue;
-	GQueue *discovery_queue;
+	GQueue *callback_queue;
 	uint8_t next_control_tid;
 	uint16_t next_service_tid;
 	qmi_debug_func_t debug_func;
@@ -154,6 +149,89 @@ void qmi_free(void *ptr)
 	free(ptr);
 }
 
+struct discover_data {
+	qmi_discover_func_t func;
+};
+
+struct shutdown_data {
+	qmi_shutdown_func_t func;
+};
+
+struct service_create_data {
+	bool shared;
+	uint8_t type;
+	uint16_t major;
+	uint16_t minor;
+	qmi_create_func_t func;
+};
+
+struct service_create_shared_data {
+	struct qmi_service *service;
+	qmi_create_func_t func;
+};
+
+struct callback_data {
+	struct qmi_device *device;
+	void *user_data;
+	qmi_destroy_func_t user_destroy;
+	qmi_destroy_func_t callback_destroy;
+	guint timeout;
+	union {
+		struct discover_data discover;
+		struct shutdown_data shutdown;
+		struct service_create_data service_create;
+		struct service_create_shared_data service_create_shared;
+	} data;
+};
+
+static void callback_data_free(struct callback_data *callback)
+{
+	if (callback->timeout) {
+		g_source_remove(callback->timeout);
+		callback->timeout = 0;
+	}
+
+	if (callback->callback_destroy)
+		callback->callback_destroy(callback);
+
+	if (callback->user_destroy)
+		callback->user_destroy(callback->user_data);
+
+	g_free(callback);
+}
+
+static void service_create_shared_free(gpointer user_data)
+{
+	struct callback_data *callback = user_data;
+
+	qmi_service_unref(callback->data.service_create_shared.service);
+}
+
+static void __callback_started(struct callback_data *callback)
+{
+	struct qmi_device *device = callback->device;
+
+	g_queue_push_tail(device->callback_queue, callback);
+}
+
+static void __callback_complete(struct callback_data *callback)
+{
+	struct qmi_device *device = callback->device;
+	GList *list;
+
+	list = g_queue_find(device->callback_queue, callback);
+	if (!list)
+		return;
+
+	g_queue_delete_link(device->callback_queue, list);
+	callback_data_free(callback);
+}
+
+static void __callback_free(gpointer data, gpointer user_data)
+{
+	callback_data_free(data);
+}
+
 static struct qmi_request *__request_alloc(uint8_t service,
 				uint8_t client, uint16_t message,
 				uint16_t headroom, const void *data,
@@ -219,24 +297,6 @@ static gint __request_compare(gconstpointer a, gconstpointer b)
 	return req->tid - tid;
 }
 
-static void __discovery_free(gpointer data, gpointer user_data)
-{
-	struct discovery *d = data;
-	qmi_destroy_func_t destroy = d->destroy;
-
-	destroy(d->discover_data);
-}
-
-static gint __discovery_compare(gconstpointer a, gconstpointer b)
-{
-	const struct discovery *d = a;
-
-	if (d->discover_data == b)
-		return 0;
-
-	return 1;
-}
-
 static void __notify_free(gpointer data, gpointer user_data)
 {
 	struct qmi_notify *notify = data;
@@ -868,37 +928,6 @@ static void read_watch_destroy(gpointer user_data)
 	device->read_watch = 0;
 }
 
-static void __qmi_device_discovery_started(struct qmi_device *device,
-						void *discover_data,
-						qmi_destroy_func_t destroy)
-{
-	struct discovery *d;
-
-	d = g_new0(struct discovery, 1);
-	d->discover_data = discover_data;
-	d->destroy = destroy;
-
-	g_queue_push_tail(device->discovery_queue, d);
-}
-
-static void __qmi_device_discovery_complete(struct qmi_device *device,
-						void *discover_data)
-{
-	GList *list;
-	struct discovery *d;
-
-	list = g_queue_find_custom(device->req_queue,
-				discover_data, __discovery_compare);
-	if (!list)
-		return;
-
-	d = list->data;
-	g_queue_delete_link(device->discovery_queue, list);
-
-	d->destroy(d->discover_data);
-	__discovery_free(d, NULL);
-}
-
 static void service_destroy(gpointer data)
 {
 	struct qmi_service *service = data;
@@ -952,7 +981,7 @@ struct qmi_device *qmi_device_new(int fd)
 	device->req_queue = g_queue_new();
 	device->control_queue = g_queue_new();
 	device->service_queue = g_queue_new();
-	device->discovery_queue = g_queue_new();
+	device->callback_queue = g_queue_new();
 
 	device->service_list = g_hash_table_new_full(g_direct_hash,
 					g_direct_equal, NULL, service_destroy);
@@ -989,8 +1018,8 @@ void qmi_device_unref(struct qmi_device *device)
 	g_queue_foreach(device->req_queue, __request_free, NULL);
 	g_queue_free(device->req_queue);
 
-	g_queue_foreach(device->discovery_queue, __discovery_free, NULL);
-	g_queue_free(device->discovery_queue);
+	g_queue_foreach(device->callback_queue, __callback_free, NULL);
+	g_queue_free(device->callback_queue);
 
 	if (device->write_watch > 0)
 		g_source_remove(device->write_watch);
@@ -1051,34 +1080,12 @@ static const void *tlv_get(const void *data, uint16_t size,
 	return NULL;
 }
 
-struct discover_data {
-	struct qmi_device *device;
-	qmi_discover_func_t func;
-	void *user_data;
-	qmi_destroy_func_t destroy;
-	guint timeout;
-};
-
-static void discover_data_free(gpointer user_data)
-{
-	struct discover_data *data = user_data;
-
-	if (data->timeout) {
-		g_source_remove(data->timeout);
-		data->timeout = 0;
-	}
-
-	if (data->destroy)
-		data->destroy(data->user_data);
-
-	g_free(data);
-}
-
 static void discover_callback(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data)
 {
-	struct discover_data *data = user_data;
-	struct qmi_device *device = data->device;
+	struct callback_data *callback = user_data;
+	struct discover_data *data = &callback->data.discover;
+	struct qmi_device *device = callback->device;
 	const struct qmi_result_code *result_code;
 	const struct qmi_service_list *service_list;
 	const void *ptr;
@@ -1155,23 +1162,22 @@ done:
 	device->version_count = count;
 
 	if (data->func)
-		data->func(count, list, data->user_data);
+		data->func(count, list, callback->user_data);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__callback_complete(callback);
 }
 
 static gboolean discover_reply(gpointer user_data)
 {
-	struct discover_data *data = user_data;
-	struct qmi_device *device = data->device;
-
-	data->timeout = 0;
+	struct callback_data *callback = user_data;
+	struct discover_data *data = &callback->data.discover;
+	struct qmi_device *device = callback->device;
 
 	if (data->func)
 		data->func(device->version_count,
-				device->version_list, data->user_data);
+				device->version_list, callback->user_data);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__callback_complete(callback);
 
 	return FALSE;
 }
@@ -1179,7 +1185,7 @@ static gboolean discover_reply(gpointer user_data)
 bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 				void *user_data, qmi_destroy_func_t destroy)
 {
-	struct discover_data *data;
+	struct callback_data *callback;
 	struct qmi_request *req;
 	struct qmi_control_hdr *hdr;
 
@@ -1188,27 +1194,27 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 
 	__debug_device(device, "device %p discover", device);
 
-	data = g_try_new0(struct discover_data, 1);
-	if (!data)
+	callback = g_try_new0(struct callback_data, 1);
+	if (!callback)
 		return false;
 
-	data->device = device;
-	data->func = func;
-	data->user_data = user_data;
-	data->destroy = destroy;
+	callback->device = device;
+	callback->user_data = user_data;
+	callback->user_destroy = destroy;
+	callback->data.discover.func = func;
 
 	if (device->version_list) {
-		data->timeout = g_timeout_add_seconds(0, discover_reply, data);
-		__qmi_device_discovery_started(device, data,
-							discover_data_free);
+		callback->timeout = g_timeout_add_seconds(0, discover_reply,
+								callback);
+		__callback_started(callback);
 		return true;
 	}
 
 	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_VERSION_INFO, QMI_CONTROL_HDR_SIZE,
-			NULL, 0, discover_callback, data, (void **) &hdr);
+			NULL, 0, discover_callback, callback, (void **) &hdr);
 	if (!req) {
-		g_free(data);
+		g_free(callback);
 		return false;
 	}
 
@@ -1220,8 +1226,8 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 
 	__request_submit(device, req, hdr->transaction);
 
-	data->timeout = g_timeout_add_seconds(5, discover_reply, data);
-	__qmi_device_discovery_started(device, data, discover_data_free);
+	callback->timeout = g_timeout_add_seconds(5, discover_reply, callback);
+	__callback_started(callback);
 
 	return true;
 }
@@ -1252,39 +1258,34 @@ static void release_client(struct qmi_device *device,
 	__request_submit(device, req, hdr->transaction);
 }
 
-struct shutdown_data {
-	struct qmi_device *device;
-	qmi_shutdown_func_t func;
-	void *user_data;
-	qmi_destroy_func_t destroy;
-};
-
 static gboolean shutdown_reply(gpointer user_data)
 {
-	struct shutdown_data *data = user_data;
+	struct callback_data *callback = user_data;
+	struct shutdown_data *data = &callback->data.shutdown;
 
 	if (data->func)
-		data->func(data->user_data);
+		data->func(callback->user_data);
 
-	g_free(data);
+	__callback_complete(callback);
 
 	return FALSE;
 }
 
 static gboolean shutdown_timeout(gpointer user_data)
 {
-	struct shutdown_data *data = user_data;
-	struct qmi_device *device = data->device;
+	struct callback_data *callback = user_data;
+	struct qmi_device *device = callback->device;
 
 	if (device->release_users > 0)
 		return TRUE;
 
-	return shutdown_reply(data);
+	return shutdown_reply(callback);
 }
 
 bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
 				void *user_data, qmi_destroy_func_t destroy)
 {
+	struct callback_data *callback;
 	struct shutdown_data *data;
 
 	if (!device)
@@ -1292,19 +1293,24 @@ bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
 
 	__debug_device(device, "device %p shutdown", device);
 
-	data = g_try_new0(struct shutdown_data, 1);
-	if (!data)
+	callback = g_try_new0(struct callback_data, 1);
+	if (!callback)
 		return false;
 
-	data->device = device;
+	callback->device = device;
+	callback->user_data = user_data;
+	callback->user_destroy = destroy;
+	data = &callback->data.shutdown;
 	data->func = func;
-	data->user_data = user_data;
-	data->destroy = destroy;
 
 	if (device->release_users > 0)
-		g_timeout_add_seconds(0, shutdown_timeout, data);
+		callback->timeout = g_timeout_add_seconds(0, shutdown_timeout,
+								callback);
 	else
-		g_timeout_add_seconds(0, shutdown_reply, data);
+		callback->timeout = g_timeout_add_seconds(0, shutdown_reply,
+								callback);
+
+	__callback_started(callback);
 
 	return true;
 }
@@ -1771,41 +1777,13 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
 	return true;
 }
 
-struct service_create_data {
-	struct qmi_device *device;
-	bool shared;
-	uint8_t type;
-	uint16_t major;
-	uint16_t minor;
-	qmi_create_func_t func;
-	void *user_data;
-	qmi_destroy_func_t destroy;
-	guint timeout;
-};
-
-static void service_create_data_free(gpointer user_data)
-{
-	struct service_create_data *data = user_data;
-
-	if (data->timeout) {
-		g_source_remove(data->timeout);
-		data->timeout = 0;
-	}
-
-	if (data->destroy)
-		data->destroy(data->user_data);
-
-	g_free(data);
-}
-
 static gboolean service_create_reply(gpointer user_data)
 {
-	struct service_create_data *data = user_data;
+	struct callback_data *callback = user_data;
 
-	data->timeout = 0;
-	data->func(NULL, data->user_data);
+	callback->data.service_create.func(NULL, callback->user_data);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__callback_complete(callback);
 
 	return FALSE;
 }
@@ -1813,8 +1791,9 @@ static gboolean service_create_reply(gpointer user_data)
 static void service_create_callback(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data)
 {
-	struct service_create_data *data = user_data;
-	struct qmi_device *device = data->device;
+	struct callback_data *callback = user_data;
+	struct service_create_data *data = &callback->data.service_create;
+	struct qmi_device *device = callback->device;
 	struct qmi_service *service = NULL;
 	const struct qmi_result_code *result_code;
 	const struct qmi_client_id *client_id;
@@ -1843,7 +1822,7 @@ static void service_create_callback(uint16_t message, uint16_t length,
 		goto done;
 
 	service->ref_count = 1;
-	service->device = data->device;
+	service->device = callback->device;
 	service->shared = data->shared;
 
 	service->type = data->type;
@@ -1861,17 +1840,18 @@ static void service_create_callback(uint16_t message, uint16_t length,
 				GUINT_TO_POINTER(hash_id), service);
 
 done:
-	data->func(service, data->user_data);
+	data->func(service, callback->user_data);
 	qmi_service_unref(service);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__callback_complete(callback);
 }
 
 static void service_create_discover(uint8_t count,
 			const struct qmi_version *list, void *user_data)
 {
-	struct service_create_data *data = user_data;
-	struct qmi_device *device = data->device;
+	struct callback_data *callback = user_data;
+	struct service_create_data *data = &callback->data.service_create;
+	struct qmi_device *device = callback->device;
 	struct qmi_request *req;
 	struct qmi_control_hdr *hdr;
 	unsigned char client_req[] = { 0x01, 0x01, 0x00, data->type };
@@ -1890,15 +1870,14 @@ static void service_create_discover(uint8_t count,
 	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_CLIENT_ID, QMI_CONTROL_HDR_SIZE,
 			client_req, sizeof(client_req),
-			service_create_callback, data, (void **) &hdr);
+			service_create_callback, callback, (void **) &hdr);
 	if (!req) {
-		if (data->timeout > 0)
-			g_source_remove(data->timeout);
+		if (callback->timeout > 0)
+			g_source_remove(callback->timeout);
 
-		data->timeout = g_timeout_add_seconds(0,
-						service_create_reply, data);
-		__qmi_device_discovery_started(device, data,
-						service_create_data_free);
+		callback->timeout = g_timeout_add_seconds(0,
+						service_create_reply, callback);
+		__callback_started(callback);
 		return;
 	}
 
@@ -1915,36 +1894,39 @@ static bool service_create(struct qmi_device *device, bool shared,
 				uint8_t type, qmi_create_func_t func,
 				void *user_data, qmi_destroy_func_t destroy)
 {
+	struct callback_data *callback;
 	struct service_create_data *data;
 
-	data = g_try_new0(struct service_create_data, 1);
-	if (!data)
+	callback = g_try_new0(struct callback_data, 1);
+	if (!callback)
 		return false;
 
-	data->device = device;
+	callback->device = device;
+	callback->user_data = user_data;
+	callback->user_destroy = destroy;
+	data = &callback->data.service_create;
+	data->func = func;
 	data->shared = shared;
 	data->type = type;
-	data->func = func;
-	data->user_data = user_data;
-	data->destroy = destroy;
 
 	if (device->version_list) {
 		service_create_discover(device->version_count,
-						device->version_list, data);
+						device->version_list, callback);
 		goto done;
 	}
 
-	if (qmi_device_discover(device, service_create_discover, data, NULL))
+	if (qmi_device_discover(device, service_create_discover, callback,
+					NULL))
 		goto done;
 
-	g_free(data);
+	g_free(callback);
 
 	return false;
 
 done:
-	data->timeout = g_timeout_add_seconds(8, service_create_reply, data);
-	__qmi_device_discovery_started(device, data,
-						service_create_data_free);
+	callback->timeout = g_timeout_add_seconds(8, service_create_reply,
+							callback);
+	__callback_started(callback);
 
 	return true;
 }
@@ -1962,40 +1944,15 @@ bool qmi_service_create(struct qmi_device *device,
 	return service_create(device, false, type, func, user_data, destroy);
 }
 
-struct service_create_shared_data {
-	struct qmi_service *service;
-	struct qmi_device *device;
-	qmi_create_func_t func;
-	void *user_data;
-	qmi_destroy_func_t destroy;
-	guint timeout;
-};
-
-static void service_create_shared_data_free(gpointer user_data)
-{
-	struct service_create_shared_data *data = user_data;
-
-	if (data->timeout) {
-		g_source_remove(data->timeout);
-		data->timeout = 0;
-	}
-
-	qmi_service_unref(data->service);
-
-	if (data->destroy)
-		data->destroy(data->user_data);
-
-	g_free(data);
-}
-
 static gboolean service_create_shared_reply(gpointer user_data)
 {
-	struct service_create_shared_data *data = user_data;
+	struct callback_data *callback = user_data;
+	struct service_create_shared_data *data =
+					&callback->data.service_create_shared;
 
-	data->timeout = 0;
-	data->func(data->service, data->user_data);
+	data->func(data->service, callback->user_data);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__callback_complete(callback);
 
 	return FALSE;
 }
@@ -2016,22 +1973,24 @@ bool qmi_service_create_shared(struct qmi_device *device,
 	service = g_hash_table_find(device->service_list,
 			__service_compare_shared, GUINT_TO_POINTER(type_val));
 	if (service) {
+		struct callback_data *callback;
 		struct service_create_shared_data *data;
 
-		data = g_try_new0(struct service_create_shared_data, 1);
-		if (!data)
+		callback = g_try_new0(struct callback_data, 1);
+		if (!callback)
 			return false;
 
+		callback->device = device;
+		callback->user_data = user_data;
+		callback->user_destroy = destroy;
+		callback->callback_destroy = service_create_shared_free;
+		data = &callback->data.service_create_shared;
 		data->service = qmi_service_ref(service);
-		data->device = device;
 		data->func = func;
-		data->user_data = user_data;
-		data->destroy = destroy;
 
-		data->timeout = g_timeout_add(0,
-					service_create_shared_reply, data);
-		__qmi_device_discovery_started(device, data,
-					service_create_shared_data_free);
+		callback->timeout = g_timeout_add(0,
+					service_create_shared_reply, callback);
+		__callback_started(callback);
 
 		return 0;
 	}
-- 
2.7.4

Re: [PATCH v2] qmimodem: include shutdown in callback cleanup queue

[Denis Kenzior]

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

Hi Lukasz,

On 03/30/2017 08:51 AM, Lukasz Nowak wrote:
> From: Lukasz Nowak <lnowak@tycoint.com>
>
> Create a unified callback_data structure, used for all timeout callbacks.
> This allows to maintain a single callback_queue, which cancels any active
> timeouts, and frees callback data memory when a usb device is hot unplugged.
> ---
>  drivers/qmimodem/qmi.c | 401 ++++++++++++++++++++++---------------------------
>  1 file changed, 180 insertions(+), 221 deletions(-)
>

I didn't like how invasive this approach was, and also the union was a 
bit ugly as it required all the weird data structures to be defined 
first.  They were no longer local to actual functions that utilized them.

Please check [PATCH] qmi: Optimize structure allocations.  That one 
should simplify things quite a bit as well.  Not tested on my end, so 
please let me know / fix it if it is broken.

Regards,
-Denis