qmimodem: Service creation fixes

qmimodem: Service creation fixes

[Ivaylo Dimitrov]

This series fixes several issues found in qmimodem service handling logic

[PATCH 1/2] qmimodem: Remove service create request on timeout

[Ivaylo Dimitrov]

Otherwise callback will be called on late response with dangling user_data.
---
 drivers/qmimodem/qmi.c | 66 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 4cd1530..fc4534a 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1147,7 +1147,7 @@ struct discover_data {
 	qmi_discover_func_t func;
 	void *user_data;
 	qmi_destroy_func_t destroy;
-	uint8_t tid;
+	uint16_t tid;
 	guint timeout;
 };
 
@@ -1245,41 +1245,53 @@ done:
 	__qmi_device_discovery_complete(data->device, &data->super);
 }
 
-static gboolean discover_reply(gpointer user_data)
+static struct qmi_request *find_control_request(struct qmi_device *device,
+						uint16_t tid)
 {
-	struct discover_data *data = user_data;
-	struct qmi_device *device = data->device;
-	unsigned int tid = data->tid;
 	GList *list;
 	struct qmi_request *req = NULL;
+	unsigned int _tid = tid;
 
-	data->timeout = 0;
-
-	/* remove request from queues */
-	if (tid != 0) {
+	if (_tid != 0) {
 		list = g_queue_find_custom(device->req_queue,
-				GUINT_TO_POINTER(tid), __request_compare);
+				GUINT_TO_POINTER(_tid), __request_compare);
 
 		if (list) {
 			req = list->data;
 			g_queue_delete_link(device->req_queue, list);
 		} else {
 			list = g_queue_find_custom(device->control_queue,
-				GUINT_TO_POINTER(tid), __request_compare);
+				GUINT_TO_POINTER(_tid), __request_compare);
 
 			if (list) {
 				req = list->data;
 				g_queue_delete_link(device->control_queue,
-								list);
+							list);
 			}
 		}
 	}
 
+	return req;
+}
+
+static gboolean discover_reply(gpointer user_data)
+{
+	struct discover_data *data = user_data;
+	struct qmi_device *device = data->device;
+	struct qmi_request *req;
+
+	/* remove request from queues */
+	req = find_control_request(device, data->tid);
+
+	data->timeout = 0;
+
 	if (data->func)
 		data->func(data->user_data);
 
-	__qmi_device_discovery_complete(data->device, &data->super);
-	__request_free(req, NULL);
+	__qmi_device_discovery_complete(device, &data->super);
+
+	if (req)
+		__request_free(req, NULL);
 
 	return FALSE;
 }
@@ -1289,7 +1301,6 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 {
 	struct discover_data *data;
 	struct qmi_request *req;
-	uint8_t tid;
 
 	if (!device)
 		return false;
@@ -1316,11 +1327,9 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data);
 
-	tid = __request_submit(device, req);
-
-	data->tid = tid;
-
+	data->tid = __request_submit(device, req);
 	data->timeout = g_timeout_add_seconds(5, discover_reply, data);
+
 	__qmi_device_discovery_started(device, &data->super);
 
 	return true;
@@ -1940,6 +1949,7 @@ struct service_create_data {
 	void *user_data;
 	qmi_destroy_func_t destroy;
 	guint timeout;
+	uint16_t tid;
 };
 
 static void service_create_data_free(gpointer user_data)
@@ -1960,11 +1970,21 @@ static void service_create_data_free(gpointer user_data)
 static gboolean service_create_reply(gpointer user_data)
 {
 	struct service_create_data *data = user_data;
+	struct qmi_device *device = data->device;
+	struct qmi_request *req;
+
+	/* remove request from queues */
+	req = find_control_request(device, data->tid);
 
 	data->timeout = 0;
-	data->func(NULL, data->user_data);
 
-	__qmi_device_discovery_complete(data->device, &data->super);
+	if (data->func)
+		data->func(NULL, data->user_data);
+
+	__qmi_device_discovery_complete(device, &data->super);
+
+	if (req)
+		__request_free(req, NULL);
 
 	return FALSE;
 }
@@ -2063,9 +2083,9 @@ static bool service_create(struct qmi_device *device,
 			client_req, sizeof(client_req),
 			service_create_callback, data);
 
-	__request_submit(device, req);
-
+	data->tid = __request_submit(device, req);
 	data->timeout = g_timeout_add_seconds(8, service_create_reply, data);
+
 	__qmi_device_discovery_started(device, &data->super);
 
 	return true;
-- 
1.9.1

[PATCH 2/2] qmimodem: Fix shared service creation logic

[Ivaylo Dimitrov]

qmi_service_create_shared() tries to find already created service of the
same type and if it fails to find one, start a creation of a new service.
This creation takes some time, so if while it is not complete, any new
calls to qmi_service_create_shared() will still fail to find a service of
that type and will start creation. This can easily lead to client ids
exhaustion and service creation failures.

Fix that by adding logic that delays responses to any shared service
creation requests after the first one, until that request either fails or
succeeds.
---
 drivers/qmimodem/qmi.c | 132 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 97 insertions(+), 35 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index fc4534a..d2d1bce 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -260,6 +260,10 @@ static gboolean __service_compare_shared(gpointer key, gpointer value,
 	struct qmi_service *service = value;
 	uint8_t type = GPOINTER_TO_UINT(user_data);
 
+	/* ignore those that are in process of creation */
+	if (GPOINTER_TO_UINT(key) & 0x80000000)
+		return FALSE;
+
 	if (service->type == type)
 		return TRUE;
 
@@ -736,6 +740,10 @@ static void service_notify(gpointer key, gpointer value, gpointer user_data)
 	struct qmi_result *result = user_data;
 	GList *list;
 
+	/* ignore those that are in process of creation */
+	if (GPOINTER_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;
@@ -1967,12 +1975,58 @@ static void service_create_data_free(gpointer user_data)
 	g_free(data);
 }
 
+struct service_create_shared_data {
+	struct discovery super;
+	struct qmi_service *service;
+	struct qmi_device *device;
+	qmi_create_func_t func;
+	void *user_data;
+	qmi_destroy_func_t destroy;
+	guint timeout;
+};
+
+static gboolean service_create_shared_reply(gpointer user_data)
+{
+	struct service_create_shared_data *data = user_data;
+
+	data->timeout = 0;
+	data->func(data->service, data->user_data);
+
+	__qmi_device_discovery_complete(data->device, &data->super);
+
+	return FALSE;
+}
+
+static void service_create_shared_pending_reply(struct qmi_device *device,
+						unsigned int type,
+						struct qmi_service *service)
+{
+	gpointer key = GUINT_TO_POINTER(type | 0x80000000);
+	GList **shared = g_hash_table_lookup(device->service_list, key);
+	GList *l;
+
+	g_hash_table_steal(device->service_list, key);
+
+	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->timeout = g_timeout_add(
+				0, service_create_shared_reply, shared_data);
+	}
+
+	g_list_free(*shared);
+	g_free(shared);
+}
+
 static gboolean service_create_reply(gpointer user_data)
 {
 	struct service_create_data *data = user_data;
 	struct qmi_device *device = data->device;
 	struct qmi_request *req;
 
+	service_create_shared_pending_reply(device, data->type, NULL);
+
 	/* remove request from queues */
 	req = find_control_request(device, data->tid);
 
@@ -2039,6 +2093,8 @@ static void service_create_callback(uint16_t message, uint16_t length,
 				GUINT_TO_POINTER(hash_id), service);
 
 done:
+	service_create_shared_pending_reply(device, data->type, service);
+
 	data->func(service, data->user_data);
 	qmi_service_unref(service);
 
@@ -2052,14 +2108,22 @@ static bool service_create(struct qmi_device *device,
 	struct service_create_data *data;
 	unsigned char client_req[] = { 0x01, 0x01, 0x00, type };
 	struct qmi_request *req;
+	GList **shared;
+	unsigned int type_val = type;
 	int i;
 
-	data = g_try_new0(struct service_create_data, 1);
-	if (!data)
+	if (!device->version_list)
 		return false;
 
-	if (!device->version_list)
+	shared = g_try_new0(GList *, 1);
+	if (!shared)
+		return NULL;
+
+	data = g_try_new0(struct service_create_data, 1);
+	if (!data) {
+		g_free(shared);
 		return false;
+	}
 
 	data->super.destroy = service_create_data_free;
 	data->device = device;
@@ -2088,19 +2152,13 @@ static bool service_create(struct qmi_device *device,
 
 	__qmi_device_discovery_started(device, &data->super);
 
+	/* Mark service creation as pending */
+	g_hash_table_insert(device->service_list,
+			GUINT_TO_POINTER(type_val | 0x80000000), shared);
+
 	return true;
 }
 
-struct service_create_shared_data {
-	struct discovery super;
-	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;
@@ -2118,23 +2176,11 @@ static void service_create_shared_data_free(gpointer user_data)
 	g_free(data);
 }
 
-static gboolean service_create_shared_reply(gpointer user_data)
-{
-	struct service_create_shared_data *data = user_data;
-
-	data->timeout = 0;
-	data->func(data->service, data->user_data);
-
-	__qmi_device_discovery_complete(data->device, &data->super);
-
-	return FALSE;
-}
-
-bool qmi_service_create_shared(struct qmi_device *device,
-				uint8_t type, qmi_create_func_t func,
-				void *user_data, qmi_destroy_func_t destroy)
+bool qmi_service_create_shared(struct qmi_device *device, uint8_t type,
+			qmi_create_func_t func, void *user_data,
+			qmi_destroy_func_t destroy)
 {
-	struct qmi_service *service;
+	gpointer service;
 	unsigned int type_val = type;
 
 	if (!device || !func)
@@ -2143,27 +2189,43 @@ bool qmi_service_create_shared(struct qmi_device *device,
 	if (type == QMI_SERVICE_CONTROL)
 		return false;
 
-	service = g_hash_table_find(device->service_list,
+	service = g_hash_table_lookup(device->service_list,
+				GUINT_TO_POINTER(type_val | 0x80000000));
+	if (!service) {
+		service = g_hash_table_find(device->service_list,
 			__service_compare_shared, GUINT_TO_POINTER(type_val));
+	} else
+		type_val |= 0x80000000;
+
 	if (service) {
 		struct service_create_shared_data *data;
 
 		data = g_try_new0(struct service_create_shared_data, 1);
 		if (!data)
-			return false;
+			return NULL;
 
 		data->super.destroy = service_create_shared_data_free;
-		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);
+		if (!data)
+			return false;
+
+		if (!(type_val & 0x80000000)) {
+			data->service = qmi_service_ref(service);
+			data->timeout = g_timeout_add(
+					0, service_create_shared_reply, data);
+		} else {
+			GList **l = service;
+
+			*l = g_list_prepend(*l, data);
+		}
+
 		__qmi_device_discovery_started(device, &data->super);
 
-		return 0;
+		return true;
 	}
 
 	return service_create(device, type, func, user_data, destroy);
-- 
1.9.1

[PATCH 2/2] qmimodem: Fix shared service creation logic

[Ivaylo Dimitrov]

qmi_service_create_shared() tries to find already created service of the
same type and if it fails to find one, start a creation of a new service.
This creation takes some time, so if while it is not complete, any new
calls to qmi_service_create_shared() will still fail to find a service of
that type and will start creation. This can easily lead to client ids
exhaustion and service creation failures.

Fix that by adding logic that delays responses to any shared service
creation requests after the first one, until that request either fails or
succeeds.
---
 drivers/qmimodem/qmi.c | 130 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 96 insertions(+), 34 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index fc4534a..e66e8d8 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -260,6 +260,10 @@ static gboolean __service_compare_shared(gpointer key, gpointer value,
 	struct qmi_service *service = value;
 	uint8_t type = GPOINTER_TO_UINT(user_data);
 
+	/* ignore those that are in process of creation */
+	if (GPOINTER_TO_UINT(key) & 0x80000000)
+		return FALSE;
+
 	if (service->type == type)
 		return TRUE;
 
@@ -736,6 +740,10 @@ static void service_notify(gpointer key, gpointer value, gpointer user_data)
 	struct qmi_result *result = user_data;
 	GList *list;
 
+	/* ignore those that are in process of creation */
+	if (GPOINTER_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;
@@ -1967,12 +1975,58 @@ static void service_create_data_free(gpointer user_data)
 	g_free(data);
 }
 
+struct service_create_shared_data {
+	struct discovery super;
+	struct qmi_service *service;
+	struct qmi_device *device;
+	qmi_create_func_t func;
+	void *user_data;
+	qmi_destroy_func_t destroy;
+	guint timeout;
+};
+
+static gboolean service_create_shared_reply(gpointer user_data)
+{
+	struct service_create_shared_data *data = user_data;
+
+	data->timeout = 0;
+	data->func(data->service, data->user_data);
+
+	__qmi_device_discovery_complete(data->device, &data->super);
+
+	return FALSE;
+}
+
+static void service_create_shared_pending_reply(struct qmi_device *device,
+						unsigned int type,
+						struct qmi_service *service)
+{
+	gpointer key = GUINT_TO_POINTER(type | 0x80000000);
+	GList **shared = g_hash_table_lookup(device->service_list, key);
+	GList *l;
+
+	g_hash_table_steal(device->service_list, key);
+
+	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->timeout = g_timeout_add(
+				0, service_create_shared_reply, shared_data);
+	}
+
+	g_list_free(*shared);
+	g_free(shared);
+}
+
 static gboolean service_create_reply(gpointer user_data)
 {
 	struct service_create_data *data = user_data;
 	struct qmi_device *device = data->device;
 	struct qmi_request *req;
 
+	service_create_shared_pending_reply(device, data->type, NULL);
+
 	/* remove request from queues */
 	req = find_control_request(device, data->tid);
 
@@ -2039,6 +2093,8 @@ static void service_create_callback(uint16_t message, uint16_t length,
 				GUINT_TO_POINTER(hash_id), service);
 
 done:
+	service_create_shared_pending_reply(device, data->type, service);
+
 	data->func(service, data->user_data);
 	qmi_service_unref(service);
 
@@ -2052,15 +2108,23 @@ static bool service_create(struct qmi_device *device,
 	struct service_create_data *data;
 	unsigned char client_req[] = { 0x01, 0x01, 0x00, type };
 	struct qmi_request *req;
+	GList **shared;
+	unsigned int type_val = type;
 	int i;
 
-	data = g_try_new0(struct service_create_data, 1);
-	if (!data)
+	if (!device->version_list)
 		return false;
 
-	if (!device->version_list)
+	shared = g_try_new0(GList *, 1);
+	if (!shared)
 		return false;
 
+	data = g_try_new0(struct service_create_data, 1);
+	if (!data) {
+		g_free(shared);
+		return false;
+	}
+
 	data->super.destroy = service_create_data_free;
 	data->device = device;
 	data->type = type;
@@ -2088,19 +2152,13 @@ static bool service_create(struct qmi_device *device,
 
 	__qmi_device_discovery_started(device, &data->super);
 
+	/* Mark service creation as pending */
+	g_hash_table_insert(device->service_list,
+			GUINT_TO_POINTER(type_val | 0x80000000), shared);
+
 	return true;
 }
 
-struct service_create_shared_data {
-	struct discovery super;
-	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;
@@ -2118,23 +2176,11 @@ static void service_create_shared_data_free(gpointer user_data)
 	g_free(data);
 }
 
-static gboolean service_create_shared_reply(gpointer user_data)
+bool qmi_service_create_shared(struct qmi_device *device, uint8_t type,
+			qmi_create_func_t func, void *user_data,
+			qmi_destroy_func_t destroy)
 {
-	struct service_create_shared_data *data = user_data;
-
-	data->timeout = 0;
-	data->func(data->service, data->user_data);
-
-	__qmi_device_discovery_complete(data->device, &data->super);
-
-	return FALSE;
-}
-
-bool qmi_service_create_shared(struct qmi_device *device,
-				uint8_t type, qmi_create_func_t func,
-				void *user_data, qmi_destroy_func_t destroy)
-{
-	struct qmi_service *service;
+	gpointer service;
 	unsigned int type_val = type;
 
 	if (!device || !func)
@@ -2143,8 +2189,14 @@ bool qmi_service_create_shared(struct qmi_device *device,
 	if (type == QMI_SERVICE_CONTROL)
 		return false;
 
-	service = g_hash_table_find(device->service_list,
+	service = g_hash_table_lookup(device->service_list,
+				GUINT_TO_POINTER(type_val | 0x80000000));
+	if (!service) {
+		service = g_hash_table_find(device->service_list,
 			__service_compare_shared, GUINT_TO_POINTER(type_val));
+	} else
+		type_val |= 0x80000000;
+
 	if (service) {
 		struct service_create_shared_data *data;
 
@@ -2153,17 +2205,27 @@ bool qmi_service_create_shared(struct qmi_device *device,
 			return false;
 
 		data->super.destroy = service_create_shared_data_free;
-		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);
+		if (!data)
+			return false;
+
+		if (!(type_val & 0x80000000)) {
+			data->service = qmi_service_ref(service);
+			data->timeout = g_timeout_add(
+					0, service_create_shared_reply, data);
+		} else {
+			GList **l = service;
+
+			*l = g_list_prepend(*l, data);
+		}
+
 		__qmi_device_discovery_started(device, &data->super);
 
-		return 0;
+		return true;
 	}
 
 	return service_create(device, type, func, user_data, destroy);
-- 
1.9.1