[RFC PATCH 00/30] QMI rework

[RFC PATCH 00/30] QMI rework

[Jonas Bonn]

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

A couple of weeks ago I submitted a patch to fix an issue with QMI
modems where the LTE modem in the pre-sim state would prevent requests
from being cancelled for services created and destroyed in the online
state.  These uncancelled requests would result in callbacks into
destroyed atoms and resulting segfaults.  The comment to that patch was
that the _correct_ fix would be to rework the way QMI handles service
sharing.

So, that said, this is my attempt at cleaning up service sharing in the
QMI core.  The jist of this is that we switch to a model that looks a
bit like file descriptions/descriptors in Linux.  The QMI services
obtain a service description which manages their info and the underlying
connection (client id) state that the 'shared services' actually share.
On top of this service description we provide service descriptors which
provide private state.  Requests and notifications are registered on the
service descriptor and their lifetimes are tied to it; when the service
descriptor is closed (released), the requests and notifications can be
cleaned up without affecting other users of the shared unlying
description.

The patch series is quite long because I tried to do this systematically
in order to make the changes reviewable.  It turned out to be quite
messy and it took me a couple of attempts to actually get a sane series
together.  Everything builds and runs for every patch in the series, BUT
I am aware that there are some details that are ignored along the way
that mean that some subtle behavioural changes may be introduced.  By
the end of the series, though, these issues should mostly have been
taken care of... so please just bear this in mind when reviewing.

Perhaps controversial so I'll state it upfront:  I've introduced the
list.h header from the Linux kernel source tree into the project.  I
really hope this won't be a showstopper... the licensing matches so
there are no legal issues, in any case.  I included the entire header
even though just a few macros are actually used... if size is an issue,
we can pare the file down to about a quarter of its size.

Finally, this is still RFC as I haven't finished reviewing this
completely, myself.  But I'd be happy to get any feedback now if someone
has a chance to look at this.

Thanks,
Jonas


Jonas Bonn (30):
  qmi: remove unused fields of service_send_data
  qmi: remove headroom parameter from req_alloc
  qmi: unify common request header setup
  qmi: request_alloc has no meaningful failure path
  qmi: push request_submit into request_alloc
  qmi: rename request_alloc to request_submit
  qmi: figure out request id without accessing header
  qmi: make qmi_service_send return result
  qmi: drop 'head' pointer from request_submit
  qmi: remove unused qmi_service_cancel
  qmi: remove unused qmi_service_unregister
  qmi: replace GQueues for requests
  qmi: assume version_list is up to date
  qmi: absorb service_create_discover into service_create
  qmi: drop discovery_queue
  qmi: make services always shared
  qmi: switch service_list to list_head type
  qmi: switch notify_list to list_head type
  qmi: drop unused struct field
  qmi: reference version_info from device in services
  qmi: make version_list private
  qmi: move client_id to qmi_version
  qmi: use standard endian macros
  qmi: convert version_list to struct list head
  qmi: move service device to service_info
  qmi: make all services unique instances backed by common description
  qmi: drop qmi_service_ref function
  qmi: rename qmi_service_create/unref to open/close
  qmi: pass service directly to request_submit
  qmi: add requests to service queue

 drivers/qmimodem/devinfo.c              |    6 +-
 drivers/qmimodem/gprs-context.c         |   14 +-
 drivers/qmimodem/gprs.c                 |   12 +-
 drivers/qmimodem/list.h                 |  736 ++++++++++++++++++++
 drivers/qmimodem/location-reporting.c   |    6 +-
 drivers/qmimodem/lte.c                  |    6 +-
 drivers/qmimodem/netmon.c               |    6 +-
 drivers/qmimodem/network-registration.c |    6 +-
 drivers/qmimodem/qmi.c                  | 1120 ++++++++++++-------------------
 drivers/qmimodem/qmi.h                  |   27 +-
 drivers/qmimodem/radio-settings.c       |   12 +-
 drivers/qmimodem/sim-legacy.c           |    6 +-
 drivers/qmimodem/sim.c                  |   12 +-
 drivers/qmimodem/sms.c                  |    6 +-
 drivers/qmimodem/ussd.c                 |    6 +-
 drivers/qmimodem/voicecall.c            |    6 +-
 plugins/gobi.c                          |   76 +--
 17 files changed, 1270 insertions(+), 793 deletions(-)
 create mode 100644 drivers/qmimodem/list.h

-- 
2.15.1

[RFC PATCH 01/30] qmi: remove unused fields of service_send_data

[Jonas Bonn]

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

After setting up the request structure, qmi_service_send makes no
further use of the 'param' and 'service' fields of the service_send_data
structure.  This patch removes those fields and frees 'param'
immediately after the request has been allocated and the parameter data
thereby copied into the send buffer.
---
 drivers/qmimodem/qmi.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 257afd0d..56a87224 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -2238,8 +2238,6 @@ bool qmi_service_get_version(struct qmi_service *service,
 }
 
 struct service_send_data {
-	struct qmi_service *service;
-	struct qmi_param *param;
 	qmi_result_func_t func;
 	void *user_data;
 	qmi_destroy_func_t destroy;
@@ -2250,8 +2248,6 @@ static void service_send_free(struct service_send_data *data)
 	if (data->destroy)
 		data->destroy(data->user_data);
 
-	qmi_param_free(data->param);
-
 	g_free(data);
 }
 
@@ -2308,17 +2304,18 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	if (!data)
 		return 0;
 
-	data->service = service;
-	data->param = param;
 	data->func = func;
 	data->user_data = user_data;
 	data->destroy = destroy;
 
 	req = __request_alloc(service->type, service->client_id,
 				message, QMI_SERVICE_HDR_SIZE,
-				data->param ? data->param->data : NULL,
-				data->param ? data->param->length : 0,
+				param ? param->data : NULL,
+				param ? param->length : 0,
 				service_send_callback, data, (void **) &hdr);
+
+	qmi_param_free(param);
+
 	if (!req) {
 		g_free(data);
 		return 0;
-- 
2.15.1

Re: [RFC PATCH 01/30] qmi: remove unused fields of service_send_data

[Denis Kenzior]

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

Hi Jonas,

> @@ -2308,17 +2304,18 @@ uint16_t qmi_service_send(struct qmi_service *service,
>   	if (!data)
>   		return 0;
>   
> -	data->service = service;
> -	data->param = param;
>   	data->func = func;
>   	data->user_data = user_data;
>   	data->destroy = destroy;
>   
>   	req = __request_alloc(service->type, service->client_id,
>   				message, QMI_SERVICE_HDR_SIZE,
> -				data->param ? data->param->data : NULL,
> -				data->param ? data->param->length : 0,
> +				param ? param->data : NULL,
> +				param ? param->length : 0,
>   				service_send_callback, data, (void **) &hdr);
> +
> +	qmi_param_free(param);
> +

This looks dangerous.  The problem is that all the qmi code issues 
qmi_param_free in case qmi_service_send failed.  So you have a double 
free situation here.

>   	if (!req) {
>   		g_free(data);
>   		return 0;
> 

Regards,
-Denis

Re: [RFC PATCH 01/30] qmi: remove unused fields of service_send_data

[Jonas Bonn]

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



On 28/03/18 21:53, Denis Kenzior wrote:
> Hi Jonas,
>
>> @@ -2308,17 +2304,18 @@ uint16_t qmi_service_send(struct qmi_service 
>> *service,
>>       if (!data)
>>           return 0;
>>   -    data->service = service;
>> -    data->param = param;
>>       data->func = func;
>>       data->user_data = user_data;
>>       data->destroy = destroy;
>>         req = __request_alloc(service->type, service->client_id,
>>                   message, QMI_SERVICE_HDR_SIZE,
>> -                data->param ? data->param->data : NULL,
>> -                data->param ? data->param->length : 0,
>> +                param ? param->data : NULL,
>> +                param ? param->length : 0,
>>                   service_send_callback, data, (void **) &hdr);
>> +
>> +    qmi_param_free(param);
>> +
>
> This looks dangerous.  The problem is that all the qmi code issues 
> qmi_param_free in case qmi_service_send failed.  So you have a double 
> free situation here.

I agree, the patch looks weird.  I was sure this outside of any failure 
path, but the context in the patch doesn't reflect that. I'll check again.

I can say, though, that I've run this change thousands of iterations and 
I've never hit a double-free...

/Jonas

>
>>       if (!req) {
>>           g_free(data);
>>           return 0;
>>
>
> Regards,
> -Denis

Re: [RFC PATCH 01/30] qmi: remove unused fields of service_send_data

[Denis Kenzior]

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

Hi Jonas,

>>> +
>>> +    qmi_param_free(param);
>>> +
>>
>> This looks dangerous.  The problem is that all the qmi code issues 
>> qmi_param_free in case qmi_service_send failed.  So you have a double 
>> free situation here.
> 
> I agree, the patch looks weird.  I was sure this outside of any failure 
> path, but the context in the patch doesn't reflect that. I'll check again.
> 
> I can say, though, that I've run this change thousands of iterations and 
> I've never hit a double-free...

I believe you.  qmi_service_send probably cannot fail in the current 
setup.  However, we still need to stick to fundamental API guarantees. 
So if the API provides a legitimate failure possibility then it must 
ensure that failure semantics are clear.  For us that usually means:

- No side effects on error.  E.g. any input parameters are not touched / 
freed / generally messed with.
- No output parameters are allocated / touched / generally messed with

I have no issue applying this patch if we drop the qmi_param_free bit.

Regards,
-Denis

Re: [RFC PATCH 01/30] qmi: remove unused fields of service_send_data

[Jonas Bonn]

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

Hi Denis,


On 28/03/18 21:53, Denis Kenzior wrote:
> Hi Jonas,
>
>> @@ -2308,17 +2304,18 @@ uint16_t qmi_service_send(struct qmi_service 
>> *service,
>>       if (!data)
>>           return 0;
>>   -    data->service = service;
>> -    data->param = param;
>>       data->func = func;
>>       data->user_data = user_data;
>>       data->destroy = destroy;
>>         req = __request_alloc(service->type, service->client_id,
>>                   message, QMI_SERVICE_HDR_SIZE,
>> -                data->param ? data->param->data : NULL,
>> -                data->param ? data->param->length : 0,
>> +                param ? param->data : NULL,
>> +                param ? param->length : 0,
>>                   service_send_callback, data, (void **) &hdr);
>> +
>> +    qmi_param_free(param);
>> +
>
> This looks dangerous.  The problem is that all the qmi code issues 
> qmi_param_free in case qmi_service_send failed.  So you have a double 
> free situation here.
>
>>       if (!req) {
>>           g_free(data);
>>           return 0;
>>
>
So, I checked this again.  The above, as you pointed out, is not right.  
But just moving the qmi_param_free to the end of the qmi_service_send 
function, just before it returns success, is what this should have 
been.  If the function returns success, the user expects that we free 
param; if we do it there then we don't need the reference into the callback.

I'll fix the patch and resubmit it.

/Jonas

> Regards,
> -Denis

[RFC PATCH 02/30] qmi: remove headroom parameter from req_alloc

[Jonas Bonn]

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

The headroom can be established from the service type, so it's redundant
to pass it as a parameter.
---
 drivers/qmimodem/qmi.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 56a87224..4a3929ba 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -161,18 +161,24 @@ void qmi_free(void *ptr)
 
 static struct qmi_request *__request_alloc(uint8_t service,
 				uint8_t client, uint16_t message,
-				uint16_t headroom, const void *data,
+				const void *data,
 				uint16_t length, qmi_message_func_t func,
 				void *user_data, void **head)
 {
 	struct qmi_request *req;
 	struct qmi_mux_hdr *hdr;
 	struct qmi_message_hdr *msg;
+	uint16_t headroom;
 
 	req = g_try_new0(struct qmi_request, 1);
 	if (!req)
 		return NULL;
 
+	if (service == QMI_SERVICE_CONTROL)
+		headroom = QMI_CONTROL_HDR_SIZE;
+	else
+		headroom = QMI_SERVICE_HDR_SIZE;
+
 	req->len = QMI_MUX_HDR_SIZE + headroom + QMI_MESSAGE_HDR_SIZE + length;
 
 	req->buf = g_try_malloc(req->len);
@@ -1251,7 +1257,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 	}
 
 	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
-			QMI_CTL_GET_VERSION_INFO, QMI_CONTROL_HDR_SIZE,
+			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data, (void **) &hdr);
 	if (!req) {
 		g_free(data);
@@ -1282,7 +1288,7 @@ static void release_client(struct qmi_device *device,
 	struct qmi_control_hdr *hdr;
 
 	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
-			QMI_CTL_RELEASE_CLIENT_ID, QMI_CONTROL_HDR_SIZE,
+			QMI_CTL_RELEASE_CLIENT_ID,
 			release_req, sizeof(release_req),
 			func, user_data, (void **) &hdr);
 	if (!req) {
@@ -1387,7 +1393,7 @@ bool qmi_device_sync(struct qmi_device *device,
 	func_data->user_data = user_data;
 
 	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
-			QMI_CTL_SYNC, QMI_CONTROL_HDR_SIZE,
+			QMI_CTL_SYNC,
 			NULL, 0,
 			qmi_device_sync_callback, func_data, (void **) &hdr);
 
@@ -2013,7 +2019,7 @@ static void service_create_discover(uint8_t count,
 	}
 
 	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
-			QMI_CTL_GET_CLIENT_ID, QMI_CONTROL_HDR_SIZE,
+			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
 			service_create_callback, data, (void **) &hdr);
 	if (!req) {
@@ -2309,7 +2315,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->destroy = destroy;
 
 	req = __request_alloc(service->type, service->client_id,
-				message, QMI_SERVICE_HDR_SIZE,
+				message,
 				param ? param->data : NULL,
 				param ? param->length : 0,
 				service_send_callback, data, (void **) &hdr);
-- 
2.15.1

[RFC PATCH 03/30] qmi: unify common request header setup

[Jonas Bonn]

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

The service and control requests differ slightly in their headers, but
this difference is minor enough that we can handle it in a common
request allocation routine.  This patch unifies the header setup for the
two request types.
---
 drivers/qmimodem/qmi.c | 62 ++++++++++++++++++++++----------------------------
 1 file changed, 27 insertions(+), 35 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 4a3929ba..0c95dd18 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -159,7 +159,8 @@ void qmi_free(void *ptr)
 	free(ptr);
 }
 
-static struct qmi_request *__request_alloc(uint8_t service,
+static struct qmi_request *__request_alloc(struct qmi_device* device,
+				uint8_t service,
 				uint8_t client, uint16_t message,
 				const void *data,
 				uint16_t length, qmi_message_func_t func,
@@ -211,6 +212,23 @@ static struct qmi_request *__request_alloc(uint8_t service,
 
 	*head = req->buf + QMI_MUX_HDR_SIZE;
 
+	if (service == QMI_SERVICE_CONTROL) {
+		struct qmi_control_hdr* hdr;
+
+		hdr = req->buf + QMI_MUX_HDR_SIZE;
+		hdr->type = 0x00;
+		hdr->transaction = device->next_control_tid++;
+		if (device->next_control_tid == 0)
+			device->next_control_tid = 1;
+	} else {
+		struct qmi_service_hdr* hdr;
+		hdr = req->buf + QMI_MUX_HDR_SIZE;
+		hdr->type = 0x00;
+		hdr->transaction = device->next_service_tid++;
+		if (device->next_service_tid < 256)
+			device->next_service_tid = 256;
+	}
+
 	return req;
 }
 
@@ -967,6 +985,9 @@ struct qmi_device *qmi_device_new(int fd)
 	device->service_list = g_hash_table_new_full(g_direct_hash,
 					g_direct_equal, NULL, service_destroy);
 
+	device->next_control_tid = 1;
+	device->next_service_tid = 256;
+
 	return device;
 }
 
@@ -1256,7 +1277,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 		return true;
 	}
 
-	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
+	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data, (void **) &hdr);
 	if (!req) {
@@ -1264,11 +1285,6 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 		return false;
 	}
 
-	if (device->next_control_tid < 1)
-		device->next_control_tid = 1;
-
-	hdr->type = 0x00;
-	hdr->transaction = device->next_control_tid++;
 	data->tid = hdr->transaction;
 
 	__request_submit(device, req, hdr->transaction);
@@ -1287,7 +1303,7 @@ static void release_client(struct qmi_device *device,
 	struct qmi_request *req;
 	struct qmi_control_hdr *hdr;
 
-	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
+	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_RELEASE_CLIENT_ID,
 			release_req, sizeof(release_req),
 			func, user_data, (void **) &hdr);
@@ -1296,12 +1312,6 @@ static void release_client(struct qmi_device *device,
 		return;
 	}
 
-	if (device->next_control_tid < 1)
-		device->next_control_tid = 1;
-
-	hdr->type = 0x00;
-	hdr->transaction = device->next_control_tid++;
-
 	__request_submit(device, req, hdr->transaction);
 }
 
@@ -1392,17 +1402,11 @@ bool qmi_device_sync(struct qmi_device *device,
 	func_data->func = func;
 	func_data->user_data = user_data;
 
-	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
+	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_SYNC,
 			NULL, 0,
 			qmi_device_sync_callback, func_data, (void **) &hdr);
 
-	if (device->next_control_tid < 1)
-		device->next_control_tid = 1;
-
-	hdr->type = 0x00;
-	hdr->transaction = device->next_control_tid++;
-
 	__request_submit(device, req, hdr->transaction);
 
 	return true;
@@ -2018,7 +2022,7 @@ static void service_create_discover(uint8_t count,
 		}
 	}
 
-	req = __request_alloc(QMI_SERVICE_CONTROL, 0x00,
+	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
 			service_create_callback, data, (void **) &hdr);
@@ -2032,12 +2036,6 @@ static void service_create_discover(uint8_t count,
 		return;
 	}
 
-	if (device->next_control_tid < 1)
-		device->next_control_tid = 1;
-
-	hdr->type = 0x00;
-	hdr->transaction = device->next_control_tid++;
-
 	__request_submit(device, req, hdr->transaction);
 }
 
@@ -2314,7 +2312,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	req = __request_alloc(service->type, service->client_id,
+	req = __request_alloc(device, service->type, service->client_id,
 				message,
 				param ? param->data : NULL,
 				param ? param->length : 0,
@@ -2327,12 +2325,6 @@ uint16_t qmi_service_send(struct qmi_service *service,
 		return 0;
 	}
 
-	if (device->next_service_tid < 256)
-		device->next_service_tid = 256;
-
-	hdr->type = 0x00;
-	hdr->transaction = device->next_service_tid++;
-
 	__request_submit(device, req, hdr->transaction);
 
 	return hdr->transaction;
-- 
2.15.1

[RFC PATCH 04/30] qmi: request_alloc has no meaningful failure path

[Jonas Bonn]

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

The only way request_alloc can fail is if one of the memory allocation
routines fail to allocate memory.  However, Linux memory allocation
doesn't really fail in this manner; memory can be overcommited and the
out-of-memory reaper will take care of re-establishing the balance when
excess memory is actually accessed.

Given this, request_alloc will never return anything other than success
and the failure paths will never be exercised.
---
 drivers/qmimodem/qmi.c | 32 ++------------------------------
 1 file changed, 2 insertions(+), 30 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 0c95dd18..85ed9ddc 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -171,9 +171,7 @@ static struct qmi_request *__request_alloc(struct qmi_device* device,
 	struct qmi_message_hdr *msg;
 	uint16_t headroom;
 
-	req = g_try_new0(struct qmi_request, 1);
-	if (!req)
-		return NULL;
+	req = calloc(1, sizeof(*req));
 
 	if (service == QMI_SERVICE_CONTROL)
 		headroom = QMI_CONTROL_HDR_SIZE;
@@ -182,11 +180,7 @@ static struct qmi_request *__request_alloc(struct qmi_device* device,
 
 	req->len = QMI_MUX_HDR_SIZE + headroom + QMI_MESSAGE_HDR_SIZE + length;
 
-	req->buf = g_try_malloc(req->len);
-	if (!req->buf) {
-		g_free(req);
-		return NULL;
-	}
+	req->buf = malloc(req->len);
 
 	req->client = client;
 
@@ -1280,10 +1274,6 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data, (void **) &hdr);
-	if (!req) {
-		g_free(data);
-		return false;
-	}
 
 	data->tid = hdr->transaction;
 
@@ -1307,10 +1297,6 @@ static void release_client(struct qmi_device *device,
 			QMI_CTL_RELEASE_CLIENT_ID,
 			release_req, sizeof(release_req),
 			func, user_data, (void **) &hdr);
-	if (!req) {
-		func(0x0000, 0x0000, NULL, user_data);
-		return;
-	}
 
 	__request_submit(device, req, hdr->transaction);
 }
@@ -2026,15 +2012,6 @@ static void service_create_discover(uint8_t count,
 			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
 			service_create_callback, data, (void **) &hdr);
-	if (!req) {
-		if (data->timeout > 0)
-			g_source_remove(data->timeout);
-
-		data->timeout = g_timeout_add_seconds(0,
-						service_create_reply, data);
-		__qmi_device_discovery_started(device, &data->super);
-		return;
-	}
 
 	__request_submit(device, req, hdr->transaction);
 }
@@ -2320,11 +2297,6 @@ uint16_t qmi_service_send(struct qmi_service *service,
 
 	qmi_param_free(param);
 
-	if (!req) {
-		g_free(data);
-		return 0;
-	}
-
 	__request_submit(device, req, hdr->transaction);
 
 	return hdr->transaction;
-- 
2.15.1

Re: [RFC PATCH 04/30] qmi: request_alloc has no meaningful failure path

[Denis Kenzior]

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

Hi Jonas,

On 03/28/2018 01:59 PM, Jonas Bonn wrote:
> The only way request_alloc can fail is if one of the memory allocation
> routines fail to allocate memory.  However, Linux memory allocation
> doesn't really fail in this manner; memory can be overcommited and the
> out-of-memory reaper will take care of re-establishing the balance when
> excess memory is actually accessed.
> 
> Given this, request_alloc will never return anything other than success
> and the failure paths will never be exercised.

Right, we encourage the use of g_new instead of g_try_new...

> ---
>   drivers/qmimodem/qmi.c | 32 ++------------------------------
>   1 file changed, 2 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
> index 0c95dd18..85ed9ddc 100644
> --- a/drivers/qmimodem/qmi.c
> +++ b/drivers/qmimodem/qmi.c
> @@ -171,9 +171,7 @@ static struct qmi_request *__request_alloc(struct qmi_device* device,
>   	struct qmi_message_hdr *msg;
>   	uint16_t headroom;
>   
> -	req = g_try_new0(struct qmi_request, 1);
> -	if (!req)
> -		return NULL;
> +	req = calloc(1, sizeof(*req));

Why now g_new0?  What's your plan here, are you wanting to get rid of 
glib entirely?  If so, then we might as well start using ell for QMI, 
which is a long term goal anyway.

>   
>   	if (service == QMI_SERVICE_CONTROL)
>   		headroom = QMI_CONTROL_HDR_SIZE;

Regards,
-Denis

Re: [RFC PATCH 04/30] qmi: request_alloc has no meaningful failure path

[Jonas Bonn]

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



On 28/03/18 21:59, Denis Kenzior wrote:
> ---
>>   drivers/qmimodem/qmi.c | 32 ++------------------------------
>>   1 file changed, 2 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
>> index 0c95dd18..85ed9ddc 100644
>> --- a/drivers/qmimodem/qmi.c
>> +++ b/drivers/qmimodem/qmi.c
>> @@ -171,9 +171,7 @@ static struct qmi_request *__request_alloc(struct 
>> qmi_device* device,
>>       struct qmi_message_hdr *msg;
>>       uint16_t headroom;
>>   -    req = g_try_new0(struct qmi_request, 1);
>> -    if (!req)
>> -        return NULL;
>> +    req = calloc(1, sizeof(*req));
>
> Why now g_new0?  What's your plan here, are you wanting to get rid of 
> glib entirely?  If so, then we might as well start using ell for QMI, 
> which is a long term goal anyway.

Well, you did say that getting rid of glib was a long term goal. 
Introducing ell introduces a new library; calloc() is a standard libc 
function.  I  did look at ell but deemed that introducing ell at the 
same time as trying to untangle the QMI mess was beyond the budget that 
I have allocated for this work.

Based on what you have said earlier, I am working under the assumptions 
that:
i)  glib is of no importance and it would be desirable to remove the 
dependency on it
ii)  ofono is Linux-only

/Jonas

Re: [RFC PATCH 04/30] qmi: request_alloc has no meaningful failure path

[Denis Kenzior]

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

Hi Jonas,

>> Why now g_new0?  What's your plan here, are you wanting to get rid of 
>> glib entirely?  If so, then we might as well start using ell for QMI, 
>> which is a long term goal anyway.
> 
> Well, you did say that getting rid of glib was a long term goal. 
> Introducing ell introduces a new library; calloc() is a standard libc 
> function.  I  did look at ell but deemed that introducing ell at the 
> same time as trying to untangle the QMI mess was beyond the budget that 
> I have allocated for this work.

Yes, except you still need main event loop integration, timers, and 
various data structures.  GLib doesn't like you mixing g_free/free and 
malloc/g_malloc/g_new.  And we don't use malloc / calloc anywhere else. 
So you either stick to GLib or we take the opportunity and convert 
everything over to ell.  So pick your poison and stick with it.

> 
> Based on what you have said earlier, I am working under the assumptions 
> that:
> i)  glib is of no importance and it would be desirable to remove the 
> dependency on it
> ii)  ofono is Linux-only
> 

This is true, but really it means to switch to ell.  It doesn't mean we 
should go off into the woods and start making qmi completely unlike 
anything else in the project code base.

Regards,
-Denis

[RFC PATCH 05/30] qmi: push request_submit into request_alloc

[Jonas Bonn]

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

The same pattern, alloc and submit, is repeated for use of these
functions.  As such, merging them is a net simplification.

With this, the return value of request_alloc can be changed from
returning a request object to just returning a result code.
---
 drivers/qmimodem/qmi.c | 47 +++++++++++++++--------------------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 85ed9ddc..a007ae3d 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -159,7 +159,9 @@ void qmi_free(void *ptr)
 	free(ptr);
 }
 
-static struct qmi_request *__request_alloc(struct qmi_device* device,
+static void wakeup_writer(struct qmi_device *device);
+
+static int __request_alloc(struct qmi_device* device,
 				uint8_t service,
 				uint8_t client, uint16_t message,
 				const void *data,
@@ -214,6 +216,7 @@ static struct qmi_request *__request_alloc(struct qmi_device* device,
 		hdr->transaction = device->next_control_tid++;
 		if (device->next_control_tid == 0)
 			device->next_control_tid = 1;
+		req->tid = hdr->transaction;
 	} else {
 		struct qmi_service_hdr* hdr;
 		hdr = req->buf + QMI_MUX_HDR_SIZE;
@@ -221,9 +224,14 @@ static struct qmi_request *__request_alloc(struct qmi_device* device,
 		hdr->transaction = device->next_service_tid++;
 		if (device->next_service_tid < 256)
 			device->next_service_tid = 256;
+		req->tid = hdr->transaction;
 	}
 
-	return req;
+	g_queue_push_tail(device->req_queue, req);
+
+	wakeup_writer(device);
+
+	return 0;
 }
 
 static void __request_free(gpointer data, gpointer user_data)
@@ -713,16 +721,6 @@ static void wakeup_writer(struct qmi_device *device)
 				can_write_data, device, write_watch_destroy);
 }
 
-static void __request_submit(struct qmi_device *device,
-				struct qmi_request *req, uint16_t transaction)
-{
-	req->tid = transaction;
-
-	g_queue_push_tail(device->req_queue, req);
-
-	wakeup_writer(device);
-}
-
 static void service_notify(gpointer key, gpointer value, gpointer user_data)
 {
 	struct qmi_service *service = value;
@@ -1247,7 +1245,6 @@ 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 qmi_request *req;
 	struct qmi_control_hdr *hdr;
 
 	if (!device)
@@ -1271,14 +1268,12 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 		return true;
 	}
 
-	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data, (void **) &hdr);
 
 	data->tid = hdr->transaction;
 
-	__request_submit(device, req, hdr->transaction);
-
 	data->timeout = g_timeout_add_seconds(5, discover_reply, data);
 	__qmi_device_discovery_started(device, &data->super);
 
@@ -1290,15 +1285,12 @@ static void release_client(struct qmi_device *device,
 				qmi_message_func_t func, void *user_data)
 {
 	unsigned char release_req[] = { 0x01, 0x02, 0x00, type, client_id };
-	struct qmi_request *req;
 	struct qmi_control_hdr *hdr;
 
-	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_RELEASE_CLIENT_ID,
 			release_req, sizeof(release_req),
 			func, user_data, (void **) &hdr);
-
-	__request_submit(device, req, hdr->transaction);
 }
 
 static void shutdown_destroy(gpointer user_data)
@@ -1375,7 +1367,6 @@ static void qmi_device_sync_callback(uint16_t message, uint16_t length,
 bool qmi_device_sync(struct qmi_device *device,
 		     qmi_sync_func_t func, void *user_data)
 {
-	struct qmi_request *req;
 	struct qmi_control_hdr *hdr;
 	struct sync_data *func_data;
 
@@ -1388,13 +1379,11 @@ bool qmi_device_sync(struct qmi_device *device,
 	func_data->func = func;
 	func_data->user_data = user_data;
 
-	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_SYNC,
 			NULL, 0,
 			qmi_device_sync_callback, func_data, (void **) &hdr);
 
-	__request_submit(device, req, hdr->transaction);
-
 	return true;
 }
 
@@ -1993,7 +1982,6 @@ static void service_create_discover(uint8_t count,
 {
 	struct service_create_data *data = user_data;
 	struct qmi_device *device = data->device;
-	struct qmi_request *req;
 	struct qmi_control_hdr *hdr;
 	unsigned char client_req[] = { 0x01, 0x01, 0x00, data->type };
 	unsigned int i;
@@ -2008,12 +1996,10 @@ static void service_create_discover(uint8_t count,
 		}
 	}
 
-	req = __request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
 			service_create_callback, data, (void **) &hdr);
-
-	__request_submit(device, req, hdr->transaction);
 }
 
 static bool service_create(struct qmi_device *device, bool shared,
@@ -2268,7 +2254,6 @@ uint16_t qmi_service_send(struct qmi_service *service,
 {
 	struct qmi_device *device;
 	struct service_send_data *data;
-	struct qmi_request *req;
 	struct qmi_service_hdr *hdr;
 
 	if (!service)
@@ -2289,7 +2274,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	req = __request_alloc(device, service->type, service->client_id,
+	__request_alloc(device, service->type, service->client_id,
 				message,
 				param ? param->data : NULL,
 				param ? param->length : 0,
@@ -2297,8 +2282,6 @@ uint16_t qmi_service_send(struct qmi_service *service,
 
 	qmi_param_free(param);
 
-	__request_submit(device, req, hdr->transaction);
-
 	return hdr->transaction;
 }
 
-- 
2.15.1

[RFC PATCH 06/30] qmi: rename request_alloc to request_submit

[Jonas Bonn]

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

Submission of the request is the fundamental operation; allocation is
just a detail.
---
 drivers/qmimodem/qmi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index a007ae3d..3d6009fa 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -161,7 +161,7 @@ void qmi_free(void *ptr)
 
 static void wakeup_writer(struct qmi_device *device);
 
-static int __request_alloc(struct qmi_device* device,
+static int __request_submit(struct qmi_device* device,
 				uint8_t service,
 				uint8_t client, uint16_t message,
 				const void *data,
@@ -1268,7 +1268,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 		return true;
 	}
 
-	__request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data, (void **) &hdr);
 
@@ -1287,7 +1287,7 @@ static void release_client(struct qmi_device *device,
 	unsigned char release_req[] = { 0x01, 0x02, 0x00, type, client_id };
 	struct qmi_control_hdr *hdr;
 
-	__request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_RELEASE_CLIENT_ID,
 			release_req, sizeof(release_req),
 			func, user_data, (void **) &hdr);
@@ -1379,7 +1379,7 @@ bool qmi_device_sync(struct qmi_device *device,
 	func_data->func = func;
 	func_data->user_data = user_data;
 
-	__request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_SYNC,
 			NULL, 0,
 			qmi_device_sync_callback, func_data, (void **) &hdr);
@@ -1996,7 +1996,7 @@ static void service_create_discover(uint8_t count,
 		}
 	}
 
-	__request_alloc(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
 			service_create_callback, data, (void **) &hdr);
@@ -2274,7 +2274,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	__request_alloc(device, service->type, service->client_id,
+	__request_submit(device, service->type, service->client_id,
 				message,
 				param ? param->data : NULL,
 				param ? param->length : 0,
-- 
2.15.1

[RFC PATCH 07/30] qmi: figure out request id without accessing header

[Jonas Bonn]

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

We want to drop the 'head' parameter from request_submit as it barely
service any purpose.  This patch changes discover_data to figure out its
'tid' value from the device struct instead of looking into the request
header.
---
 drivers/qmimodem/qmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 3d6009fa..538da02c 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1272,7 +1272,9 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data, (void **) &hdr);
 
-	data->tid = hdr->transaction;
+	data->tid = device->next_control_tid - 1;
+	if (data->tid == 0)
+		data->tid = 255;
 
 	data->timeout = g_timeout_add_seconds(5, discover_reply, data);
 	__qmi_device_discovery_started(device, &data->super);
-- 
2.15.1

[RFC PATCH 08/30] qmi: make qmi_service_send return result

[Jonas Bonn]

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

The function qmi_service_send used to return the request's transaction
id, but nowhere in the codebase was this actually used.  This patch
switches the return value of that function to just indicate success or
failure, which is sufficient for all current users of the function.
---
 drivers/qmimodem/qmi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 538da02c..2c209d59 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -2257,6 +2257,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	struct qmi_device *device;
 	struct service_send_data *data;
 	struct qmi_service_hdr *hdr;
+	int r;
 
 	if (!service)
 		return 0;
@@ -2284,7 +2285,10 @@ uint16_t qmi_service_send(struct qmi_service *service,
 
 	qmi_param_free(param);
 
-	return hdr->transaction;
+	if (r)
+		return 0; /* request submission failed */
+
+	return 1;
 }
 
 bool qmi_service_cancel(struct qmi_service *service, uint16_t id)
-- 
2.15.1

[RFC PATCH 09/30] qmi: drop 'head' pointer from request_submit

[Jonas Bonn]

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

Returning a pointer of the request's header does not serve any
meaningful purpose anymore, so drop it.
---
 drivers/qmimodem/qmi.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 2c209d59..2e920abd 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -166,7 +166,7 @@ static int __request_submit(struct qmi_device* device,
 				uint8_t client, uint16_t message,
 				const void *data,
 				uint16_t length, qmi_message_func_t func,
-				void *user_data, void **head)
+				void *user_data)
 {
 	struct qmi_request *req;
 	struct qmi_mux_hdr *hdr;
@@ -206,8 +206,6 @@ static int __request_submit(struct qmi_device* device,
 	req->callback = func;
 	req->user_data = user_data;
 
-	*head = req->buf + QMI_MUX_HDR_SIZE;
-
 	if (service == QMI_SERVICE_CONTROL) {
 		struct qmi_control_hdr* hdr;
 
@@ -1245,7 +1243,6 @@ 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 qmi_control_hdr *hdr;
 
 	if (!device)
 		return false;
@@ -1270,7 +1267,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 
 	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_VERSION_INFO,
-			NULL, 0, discover_callback, data, (void **) &hdr);
+			NULL, 0, discover_callback, data);
 
 	data->tid = device->next_control_tid - 1;
 	if (data->tid == 0)
@@ -1287,12 +1284,11 @@ static void release_client(struct qmi_device *device,
 				qmi_message_func_t func, void *user_data)
 {
 	unsigned char release_req[] = { 0x01, 0x02, 0x00, type, client_id };
-	struct qmi_control_hdr *hdr;
 
 	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_RELEASE_CLIENT_ID,
 			release_req, sizeof(release_req),
-			func, user_data, (void **) &hdr);
+			func, user_data);
 }
 
 static void shutdown_destroy(gpointer user_data)
@@ -1369,7 +1365,6 @@ static void qmi_device_sync_callback(uint16_t message, uint16_t length,
 bool qmi_device_sync(struct qmi_device *device,
 		     qmi_sync_func_t func, void *user_data)
 {
-	struct qmi_control_hdr *hdr;
 	struct sync_data *func_data;
 
 	if (!device)
@@ -1384,7 +1379,7 @@ bool qmi_device_sync(struct qmi_device *device,
 	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_SYNC,
 			NULL, 0,
-			qmi_device_sync_callback, func_data, (void **) &hdr);
+			qmi_device_sync_callback, func_data);
 
 	return true;
 }
@@ -1984,7 +1979,6 @@ static void service_create_discover(uint8_t count,
 {
 	struct service_create_data *data = user_data;
 	struct qmi_device *device = data->device;
-	struct qmi_control_hdr *hdr;
 	unsigned char client_req[] = { 0x01, 0x01, 0x00, data->type };
 	unsigned int i;
 
@@ -2001,7 +1995,7 @@ static void service_create_discover(uint8_t count,
 	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
-			service_create_callback, data, (void **) &hdr);
+			service_create_callback, data);
 }
 
 static bool service_create(struct qmi_device *device, bool shared,
@@ -2256,7 +2250,6 @@ uint16_t qmi_service_send(struct qmi_service *service,
 {
 	struct qmi_device *device;
 	struct service_send_data *data;
-	struct qmi_service_hdr *hdr;
 	int r;
 
 	if (!service)
@@ -2277,11 +2270,11 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	__request_submit(device, service->type, service->client_id,
+	r = __request_submit(device, service->type, service->client_id,
 				message,
 				param ? param->data : NULL,
 				param ? param->length : 0,
-				service_send_callback, data, (void **) &hdr);
+				service_send_callback, data);
 
 	qmi_param_free(param);
 
-- 
2.15.1

[RFC PATCH 10/30] qmi: remove unused qmi_service_cancel

[Jonas Bonn]

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

The function is not used anywhere in the codebase.
---
 drivers/qmimodem/qmi.c | 41 -----------------------------------------
 drivers/qmimodem/qmi.h |  1 -
 2 files changed, 42 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 2e920abd..6cc858be 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -2284,47 +2284,6 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	return 1;
 }
 
-bool qmi_service_cancel(struct qmi_service *service, uint16_t id)
-{
-	unsigned int tid = id;
-	struct qmi_device *device;
-	struct qmi_request *req;
-	GList *list;
-
-	if (!service || !tid)
-		return false;
-
-	if (!service->client_id)
-		return false;
-
-	device = service->device;
-	if (!device)
-		return false;
-
-	list = g_queue_find_custom(device->req_queue,
-				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->service_queue,
-				GUINT_TO_POINTER(tid), __request_compare);
-		if (!list)
-			return false;
-
-		req = list->data;
-
-		g_queue_delete_link(device->service_queue, list);
-	}
-
-	service_send_free(req->user_data);
-
-	__request_free(req, NULL);
-
-	return true;
-}
-
 static GQueue *remove_client(GQueue *queue, uint8_t client)
 {
 	GQueue *new_queue;
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index e1801045..bb8eb4a0 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -170,7 +170,6 @@ uint16_t qmi_service_send(struct qmi_service *service,
 				uint16_t message, struct qmi_param *param,
 				qmi_result_func_t func,
 				void *user_data, qmi_destroy_func_t destroy);
-bool qmi_service_cancel(struct qmi_service *service, uint16_t id);
 bool qmi_service_cancel_all(struct qmi_service *service);
 
 uint16_t qmi_service_register(struct qmi_service *service,
-- 
2.15.1

Re: [RFC PATCH 10/30] qmi: remove unused qmi_service_cancel

[Denis Kenzior]

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

Hi Jonas,

On 03/28/2018 01:59 PM, Jonas Bonn wrote:
> The function is not used anywhere in the codebase.

I think it would be fundamentally wrong to drop this.  Similar 
capability is used in other drivers all the time.

Regards,
-Denis

[RFC PATCH 11/30] qmi: remove unused qmi_service_unregister

[Jonas Bonn]

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

Nothing in the codebase saves the notify ID when calling
qmi_service_register.  Nothing, therefore ever calls
qmi_service_unregister, either.
---
 drivers/qmimodem/qmi.c | 43 +++----------------------------------------
 drivers/qmimodem/qmi.h |  3 +--
 2 files changed, 4 insertions(+), 42 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 6cc858be..5ed41e0a 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -85,7 +85,6 @@ struct qmi_service {
 	uint16_t major;
 	uint16_t minor;
 	uint8_t client_id;
-	uint16_t next_notify_id;
 	GList *notify_list;
 };
 
@@ -112,7 +111,6 @@ struct qmi_request {
 };
 
 struct qmi_notify {
-	uint16_t id;
 	uint16_t message;
 	qmi_result_func_t callback;
 	void *user_data;
@@ -266,14 +264,6 @@ static void __notify_free(gpointer data, gpointer user_data)
 	g_free(notify);
 }
 
-static gint __notify_compare(gconstpointer a, gconstpointer b)
-{
-	const struct qmi_notify *notify = a;
-	uint16_t id = GPOINTER_TO_UINT(b);
-
-	return notify->id - id;
-}
-
 static gboolean __service_compare_shared(gpointer key, gpointer value,
 							gpointer user_data)
 {
@@ -2338,23 +2328,19 @@ bool qmi_service_cancel_all(struct qmi_service *service)
 	return true;
 }
 
-uint16_t qmi_service_register(struct qmi_service *service,
+bool qmi_service_register(struct qmi_service *service,
 				uint16_t message, qmi_result_func_t func,
 				void *user_data, qmi_destroy_func_t destroy)
 {
 	struct qmi_notify *notify;
 
 	if (!service || !func)
-		return 0;
+		return false;
 
 	notify = g_try_new0(struct qmi_notify, 1);
 	if (!notify)
-		return 0;
-
-	if (service->next_notify_id < 1)
-		service->next_notify_id = 1;
+		return false;
 
-	notify->id = service->next_notify_id++;
 	notify->message = message;
 	notify->callback = func;
 	notify->user_data = user_data;
@@ -2362,29 +2348,6 @@ uint16_t qmi_service_register(struct qmi_service *service,
 
 	service->notify_list = g_list_append(service->notify_list, notify);
 
-	return notify->id;
-}
-
-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,
-				GUINT_TO_POINTER(nid), __notify_compare);
-	if (!list)
-		return false;
-
-	notify = list->data;
-
-	service->notify_list = g_list_delete_link(service->notify_list, list);
-
-	__notify_free(notify, NULL);
-
 	return true;
 }
 
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index bb8eb4a0..2c0d5aff 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -172,8 +172,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 				void *user_data, qmi_destroy_func_t destroy);
 bool qmi_service_cancel_all(struct qmi_service *service);
 
-uint16_t qmi_service_register(struct qmi_service *service,
+bool qmi_service_register(struct qmi_service *service,
 				uint16_t message, qmi_result_func_t func,
 				void *user_data, qmi_destroy_func_t destroy);
-bool qmi_service_unregister(struct qmi_service *service, uint16_t id);
 bool qmi_service_unregister_all(struct qmi_service *service);
-- 
2.15.1

[RFC PATCH 12/30] qmi: replace GQueues for requests

[Jonas Bonn]

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

This brings in list.h from the Linux kernel and replaces the GQueues
used for enqueuing requests with struct list_heads.  These list
operations are both more efficient and generally easier to work with.
Since ofono and Linux share a common license, this code borrowing should
not be problematic in any way.
---
 drivers/qmimodem/list.h | 736 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/qmimodem/qmi.c  | 172 ++++++-----
 2 files changed, 820 insertions(+), 88 deletions(-)
 create mode 100644 drivers/qmimodem/list.h

diff --git a/drivers/qmimodem/list.h b/drivers/qmimodem/list.h
new file mode 100644
index 00000000..d5dddcc1
--- /dev/null
+++ b/drivers/qmimodem/list.h
@@ -0,0 +1,736 @@
+#ifndef __POC_LIST_H
+#define __POC_LIST_H
+
+#include <stdlib.h>
+#include <stddef.h>
+
+#ifndef __GNUC__
+#error list.h relies on GCC extensions...
+#endif
+
+/**
+ * container_of - cast a member of a structure out to the containing structure
+ * @ptr:	the pointer to the member.
+ * @type:	the type of the container struct this is embedded in.
+ * @member:	the name of the member within the struct.
+ *
+ */
+#define container_of(ptr, type, member) ({			\
+	const typeof( ((type *)0)->member ) *__mptr = (ptr);	\
+	(type *)(void *)( (char *)__mptr - offsetof(type,member) );})
+
+#define LIST_POISON1 0
+#define LIST_POISON2 0
+
+struct list_head {
+	struct list_head *next, *prev;
+};
+
+struct hlist_head {
+	struct hlist_node *first;
+};
+
+struct hlist_node {
+	struct hlist_node *next, **pprev;
+};
+
+/*
+ * Simple doubly linked list implementation.
+ *
+ * Some of the internal functions ("__xxx") are useful when
+ * manipulating whole lists rather than single entries, as
+ * sometimes we already know the next/prev entries and we can
+ * generate better code by using them directly rather than
+ * using the generic single-entry routines.
+ */
+
+#define LIST_HEAD_INIT(name) { &(name), &(name) }
+
+#define LIST_HEAD(name) \
+	struct list_head name = LIST_HEAD_INIT(name)
+
+static inline void INIT_LIST_HEAD(struct list_head *list)
+{
+	list->next = list;
+	list->prev = list;
+}
+
+/*
+ * Insert a new entry between two known consecutive entries.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_add(struct list_head *new,
+			      struct list_head *prev,
+			      struct list_head *next)
+{
+	next->prev = new;
+	new->next = next;
+	new->prev = prev;
+	prev->next = new;
+}
+
+/**
+ * list_add - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it after
+ *
+ * Insert a new entry after the specified head.
+ * This is good for implementing stacks.
+ */
+static inline void list_add(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head, head->next);
+}
+
+
+/**
+ * list_add_tail - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it before
+ *
+ * Insert a new entry before the specified head.
+ * This is useful for implementing queues.
+ */
+static inline void list_add_tail(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head->prev, head);
+}
+
+/*
+ * Delete a list entry by making the prev/next entries
+ * point to each other.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_del(struct list_head * prev, struct list_head * next)
+{
+	next->prev = prev;
+	prev->next = next;
+}
+
+/**
+ * list_del - deletes entry from list.
+ * @entry: the element to delete from the list.
+ * Note: list_empty() on entry does not return true after this, the entry is
+ * in an undefined state.
+ */
+static inline void __list_del_entry(struct list_head *entry)
+{
+	__list_del(entry->prev, entry->next);
+}
+
+static inline void list_del(struct list_head *entry)
+{
+	__list_del(entry->prev, entry->next);
+	entry->next = LIST_POISON1;
+	entry->prev = LIST_POISON2;
+}
+
+/**
+ * list_replace - replace old entry by new one
+ * @old : the element to be replaced
+ * @new : the new element to insert
+ *
+ * If @old was empty, it will be overwritten.
+ */
+static inline void list_replace(struct list_head *old,
+				struct list_head *new)
+{
+	new->next = old->next;
+	new->next->prev = new;
+	new->prev = old->prev;
+	new->prev->next = new;
+}
+
+static inline void list_replace_init(struct list_head *old,
+					struct list_head *new)
+{
+	list_replace(old, new);
+	INIT_LIST_HEAD(old);
+}
+
+/**
+ * list_del_init - deletes entry from list and reinitialize it.
+ * @entry: the element to delete from the list.
+ */
+static inline void list_del_init(struct list_head *entry)
+{
+	__list_del_entry(entry);
+	INIT_LIST_HEAD(entry);
+}
+
+/**
+ * list_move - delete from one list and add as another's head
+ * @list: the entry to move
+ * @head: the head that will precede our entry
+ */
+static inline void list_move(struct list_head *list, struct list_head *head)
+{
+	__list_del_entry(list);
+	list_add(list, head);
+}
+
+/**
+ * list_move_tail - delete from one list and add as another's tail
+ * @list: the entry to move
+ * @head: the head that will follow our entry
+ */
+static inline void list_move_tail(struct list_head *list,
+				  struct list_head *head)
+{
+	__list_del_entry(list);
+	list_add_tail(list, head);
+}
+
+/**
+ * list_is_last - tests whether @list is the last entry in list @head
+ * @list: the entry to test
+ * @head: the head of the list
+ */
+static inline int list_is_last(const struct list_head *list,
+				const struct list_head *head)
+{
+	return list->next == head;
+}
+
+/**
+ * list_empty - tests whether a list is empty
+ * @head: the list to test.
+ */
+static inline int list_empty(const struct list_head *head)
+{
+	return head->next == head;
+}
+
+/**
+ * list_empty_careful - tests whether a list is empty and not being modified
+ * @head: the list to test
+ *
+ * Description:
+ * tests whether a list is empty _and_ checks that no other CPU might be
+ * in the process of modifying either member (next or prev)
+ *
+ * NOTE: using list_empty_careful() without synchronization
+ * can only be safe if the only activity that can happen
+ * to the list entry is list_del_init(). Eg. it cannot be used
+ * if another CPU could re-list_add() it.
+ */
+static inline int list_empty_careful(const struct list_head *head)
+{
+	struct list_head *next = head->next;
+	return (next == head) && (next == head->prev);
+}
+
+/**
+ * list_rotate_left - rotate the list to the left
+ * @head: the head of the list
+ */
+static inline void list_rotate_left(struct list_head *head)
+{
+	struct list_head *first;
+
+	if (!list_empty(head)) {
+		first = head->next;
+		list_move_tail(first, head);
+	}
+}
+
+/**
+ * list_is_singular - tests whether a list has just one entry.
+ * @head: the list to test.
+ */
+static inline int list_is_singular(const struct list_head *head)
+{
+	return !list_empty(head) && (head->next == head->prev);
+}
+
+static inline void __list_cut_position(struct list_head *list,
+		struct list_head *head, struct list_head *entry)
+{
+	struct list_head *new_first = entry->next;
+	list->next = head->next;
+	list->next->prev = list;
+	list->prev = entry;
+	entry->next = list;
+	head->next = new_first;
+	new_first->prev = head;
+}
+
+/**
+ * list_cut_position - cut a list into two
+ * @list: a new list to add all removed entries
+ * @head: a list with entries
+ * @entry: an entry within head, could be the head itself
+ *	and if so we won't cut the list
+ *
+ * This helper moves the initial part of @head, up to and
+ * including @entry, from @head to @list. You should
+ * pass on @entry an element you know is on @head. @list
+ * should be an empty list or a list you do not care about
+ * losing its data.
+ *
+ */
+static inline void list_cut_position(struct list_head *list,
+		struct list_head *head, struct list_head *entry)
+{
+	if (list_empty(head))
+		return;
+	if (list_is_singular(head) &&
+		(head->next != entry && head != entry))
+		return;
+	if (entry == head)
+		INIT_LIST_HEAD(list);
+	else
+		__list_cut_position(list, head, entry);
+}
+
+static inline void __list_splice(const struct list_head *list,
+				 struct list_head *prev,
+				 struct list_head *next)
+{
+	struct list_head *first = list->next;
+	struct list_head *last = list->prev;
+
+	first->prev = prev;
+	prev->next = first;
+
+	last->next = next;
+	next->prev = last;
+}
+
+/**
+ * list_splice - join two lists, this is designed for stacks
+ * @list: the new list to add.
+ * @head: the place to add it in the first list.
+ */
+static inline void list_splice(const struct list_head *list,
+				struct list_head *head)
+{
+	if (!list_empty(list))
+		__list_splice(list, head, head->next);
+}
+
+/**
+ * list_splice_tail - join two lists, each list being a queue
+ * @list: the new list to add.
+ * @head: the place to add it in the first list.
+ */
+static inline void list_splice_tail(struct list_head *list,
+				struct list_head *head)
+{
+	if (!list_empty(list))
+		__list_splice(list, head->prev, head);
+}
+
+/**
+ * list_splice_init - join two lists and reinitialise the emptied list.
+ * @list: the new list to add.
+ * @head: the place to add it in the first list.
+ *
+ * The list at @list is reinitialised
+ */
+static inline void list_splice_init(struct list_head *list,
+				    struct list_head *head)
+{
+	if (!list_empty(list)) {
+		__list_splice(list, head, head->next);
+		INIT_LIST_HEAD(list);
+	}
+}
+
+/**
+ * list_splice_tail_init - join two lists and reinitialise the emptied list
+ * @list: the new list to add.
+ * @head: the place to add it in the first list.
+ *
+ * Each of the lists is a queue.
+ * The list at @list is reinitialised
+ */
+static inline void list_splice_tail_init(struct list_head *list,
+					 struct list_head *head)
+{
+	if (!list_empty(list)) {
+		__list_splice(list, head->prev, head);
+		INIT_LIST_HEAD(list);
+	}
+}
+
+/**
+ * list_entry - get the struct for this entry
+ * @ptr:	the &struct list_head pointer.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_entry(ptr, type, member) \
+	container_of(ptr, type, member)
+
+/**
+ * list_first_entry - get the first element from a list
+ * @ptr:	the list head to take the element from.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_struct within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_first_entry(ptr, type, member) \
+	list_entry((ptr)->next, type, member)
+
+/**
+ * list_for_each	-	iterate over a list
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @head:	the head for your list.
+ */
+#define list_for_each(pos, head) \
+	for (pos = (head)->next; pos != (head); pos = pos->next)
+
+/**
+ * __list_for_each	-	iterate over a list
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @head:	the head for your list.
+ *
+ * This variant doesn't differ from list_for_each() any more.
+ * We don't do prefetching in either case.
+ */
+#define __list_for_each(pos, head) \
+	for (pos = (head)->next; pos != (head); pos = pos->next)
+
+/**
+ * list_for_each_prev	-	iterate over a list backwards
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @head:	the head for your list.
+ */
+#define list_for_each_prev(pos, head) \
+	for (pos = (head)->prev; pos != (head); pos = pos->prev)
+
+/**
+ * list_for_each_safe - iterate over a list safe against removal of list entry
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @n:		another &struct list_head to use as temporary storage
+ * @head:	the head for your list.
+ */
+#define list_for_each_safe(pos, n, head) \
+	for (pos = (head)->next, n = pos->next; pos != (head); \
+		pos = n, n = pos->next)
+
+/**
+ * list_for_each_prev_safe - iterate over a list backwards safe against removal of list entry
+ * @pos:	the &struct list_head to use as a loop cursor.
+ * @n:		another &struct list_head to use as temporary storage
+ * @head:	the head for your list.
+ */
+#define list_for_each_prev_safe(pos, n, head) \
+	for (pos = (head)->prev, n = pos->prev; \
+	     pos != (head); \
+	     pos = n, n = pos->prev)
+
+/**
+ * list_for_each_entry	-	iterate over list of given type
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_each_entry(pos, head, member)				\
+	for (pos = list_entry((head)->next, typeof(*pos), member);	\
+	     &pos->member != (head); 	\
+	     pos = list_entry(pos->member.next, typeof(*pos), member))
+
+/**
+ * list_for_each_entry_reverse - iterate backwards over list of given type.
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_each_entry_reverse(pos, head, member)			\
+	for (pos = list_entry((head)->prev, typeof(*pos), member);	\
+	     &pos->member != (head); 	\
+	     pos = list_entry(pos->member.prev, typeof(*pos), member))
+
+/**
+ * list_prepare_entry - prepare a pos entry for use in list_for_each_entry_continue()
+ * @pos:	the type * to use as a start point
+ * @head:	the head of the list
+ * @member:	the name of the list_struct within the struct.
+ *
+ * Prepares a pos entry for use as a start point in list_for_each_entry_continue().
+ */
+#define list_prepare_entry(pos, head, member) \
+	((pos) ? : list_entry(head, typeof(*pos), member))
+
+/**
+ * list_for_each_entry_continue - continue iteration over list of given type
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ *
+ * Continue to iterate over list of given type, continuing after
+ * the current position.
+ */
+#define list_for_each_entry_continue(pos, head, member) 		\
+	for (pos = list_entry(pos->member.next, typeof(*pos), member);	\
+	     &pos->member != (head);	\
+	     pos = list_entry(pos->member.next, typeof(*pos), member))
+
+/**
+ * list_for_each_entry_continue_reverse - iterate backwards from the given point
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ *
+ * Start to iterate over list of given type backwards, continuing after
+ * the current position.
+ */
+#define list_for_each_entry_continue_reverse(pos, head, member)		\
+	for (pos = list_entry(pos->member.prev, typeof(*pos), member);	\
+	     &pos->member != (head);	\
+	     pos = list_entry(pos->member.prev, typeof(*pos), member))
+
+/**
+ * list_for_each_entry_from - iterate over list of given type from the current point
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ *
+ * Iterate over list of given type, continuing from current position.
+ */
+#define list_for_each_entry_from(pos, head, member) 			\
+	for (; &pos->member != (head);	\
+	     pos = list_entry(pos->member.next, typeof(*pos), member))
+
+/**
+ * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_each_entry_safe(pos, n, head, member)			\
+	for (pos = list_entry((head)->next, typeof(*pos), member),	\
+		n = list_entry(pos->member.next, typeof(*pos), member);	\
+	     &pos->member != (head); 					\
+	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
+
+/**
+ * list_for_each_entry_safe_continue - continue list iteration safe against removal
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ *
+ * Iterate over list of given type, continuing after current point,
+ * safe against removal of list entry.
+ */
+#define list_for_each_entry_safe_continue(pos, n, head, member) 		\
+	for (pos = list_entry(pos->member.next, typeof(*pos), member), 		\
+		n = list_entry(pos->member.next, typeof(*pos), member);		\
+	     &pos->member != (head);						\
+	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
+
+/**
+ * list_for_each_entry_safe_from - iterate over list from current point safe against removal
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ *
+ * Iterate over list of given type from current point, safe against
+ * removal of list entry.
+ */
+#define list_for_each_entry_safe_from(pos, n, head, member) 			\
+	for (n = list_entry(pos->member.next, typeof(*pos), member);		\
+	     &pos->member != (head);						\
+	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
+
+/**
+ * list_for_each_entry_safe_reverse - iterate backwards over list safe against removal
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_struct within the struct.
+ *
+ * Iterate backwards over list of given type, safe against removal
+ * of list entry.
+ */
+#define list_for_each_entry_safe_reverse(pos, n, head, member)		\
+	for (pos = list_entry((head)->prev, typeof(*pos), member),	\
+		n = list_entry(pos->member.prev, typeof(*pos), member);	\
+	     &pos->member != (head); 					\
+	     pos = n, n = list_entry(n->member.prev, typeof(*n), member))
+
+/**
+ * list_safe_reset_next - reset a stale list_for_each_entry_safe loop
+ * @pos:	the loop cursor used in the list_for_each_entry_safe loop
+ * @n:		temporary storage used in list_for_each_entry_safe
+ * @member:	the name of the list_struct within the struct.
+ *
+ * list_safe_reset_next is not safe to use in general if the list may be
+ * modified concurrently (eg. the lock is dropped in the loop body). An
+ * exception to this is if the cursor element (pos) is pinned in the list,
+ * and list_safe_reset_next is called after re-taking the lock and before
+ * completing the current iteration of the loop body.
+ */
+#define list_safe_reset_next(pos, n, member)				\
+	n = list_entry(pos->member.next, typeof(*pos), member)
+
+/*
+ * Double linked lists with a single pointer list head.
+ * Mostly useful for hash tables where the two pointer list head is
+ * too wasteful.
+ * You lose the ability to access the tail in O(1).
+ */
+
+#define HLIST_HEAD_INIT { .first = NULL }
+#define HLIST_HEAD(name) struct hlist_head name = {  .first = NULL }
+#define INIT_HLIST_HEAD(ptr) ((ptr)->first = NULL)
+static inline void INIT_HLIST_NODE(struct hlist_node *h)
+{
+	h->next = NULL;
+	h->pprev = NULL;
+}
+
+static inline int hlist_unhashed(const struct hlist_node *h)
+{
+	return !h->pprev;
+}
+
+static inline int hlist_empty(const struct hlist_head *h)
+{
+	return !h->first;
+}
+
+static inline void __hlist_del(struct hlist_node *n)
+{
+	struct hlist_node *next = n->next;
+	struct hlist_node **pprev = n->pprev;
+	*pprev = next;
+	if (next)
+		next->pprev = pprev;
+}
+
+static inline void hlist_del(struct hlist_node *n)
+{
+	__hlist_del(n);
+	n->next = LIST_POISON1;
+	n->pprev = LIST_POISON2;
+}
+
+static inline void hlist_del_init(struct hlist_node *n)
+{
+	if (!hlist_unhashed(n)) {
+		__hlist_del(n);
+		INIT_HLIST_NODE(n);
+	}
+}
+
+static inline void hlist_add_head(struct hlist_node *n, struct hlist_head *h)
+{
+	struct hlist_node *first = h->first;
+	n->next = first;
+	if (first)
+		first->pprev = &n->next;
+	h->first = n;
+	n->pprev = &h->first;
+}
+
+/* next must be != NULL */
+static inline void hlist_add_before(struct hlist_node *n,
+					struct hlist_node *next)
+{
+	n->pprev = next->pprev;
+	n->next = next;
+	next->pprev = &n->next;
+	*(n->pprev) = n;
+}
+
+static inline void hlist_add_after(struct hlist_node *n,
+					struct hlist_node *next)
+{
+	next->next = n->next;
+	n->next = next;
+	next->pprev = &n->next;
+
+	if(next->next)
+		next->next->pprev  = &next->next;
+}
+
+/* after that we'll appear to be on some hlist and hlist_del will work */
+static inline void hlist_add_fake(struct hlist_node *n)
+{
+	n->pprev = &n->next;
+}
+
+/*
+ * Move a list from one list head to another. Fixup the pprev
+ * reference of the first entry if it exists.
+ */
+static inline void hlist_move_list(struct hlist_head *old,
+				   struct hlist_head *new)
+{
+	new->first = old->first;
+	if (new->first)
+		new->first->pprev = &new->first;
+	old->first = NULL;
+}
+
+#define hlist_entry(ptr, type, member) container_of(ptr,type,member)
+
+#define hlist_for_each(pos, head) \
+	for (pos = (head)->first; pos ; pos = pos->next)
+
+#define hlist_for_each_safe(pos, n, head) \
+	for (pos = (head)->first; pos && ({ n = pos->next; 1; }); \
+	     pos = n)
+
+/**
+ * hlist_for_each_entry	- iterate over list of given type
+ * @tpos:	the type * to use as a loop cursor.
+ * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the hlist_node within the struct.
+ */
+#define hlist_for_each_entry(tpos, pos, head, member)			 \
+	for (pos = (head)->first;					 \
+	     pos &&							 \
+		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
+	     pos = pos->next)
+
+/**
+ * hlist_for_each_entry_continue - iterate over a hlist continuing after current point
+ * @tpos:	the type * to use as a loop cursor.
+ * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @member:	the name of the hlist_node within the struct.
+ */
+#define hlist_for_each_entry_continue(tpos, pos, member)		 \
+	for (pos = (pos)->next;						 \
+	     pos &&							 \
+		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
+	     pos = pos->next)
+
+/**
+ * hlist_for_each_entry_from - iterate over a hlist continuing from current point
+ * @tpos:	the type * to use as a loop cursor.
+ * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @member:	the name of the hlist_node within the struct.
+ */
+#define hlist_for_each_entry_from(tpos, pos, member)			 \
+	for (; pos &&							 \
+		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
+	     pos = pos->next)
+
+/**
+ * hlist_for_each_entry_safe - iterate over list of given type safe against removal of list entry
+ * @tpos:	the type * to use as a loop cursor.
+ * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @n:		another &struct hlist_node to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the hlist_node within the struct.
+ */
+#define hlist_for_each_entry_safe(tpos, pos, n, head, member) 		 \
+	for (pos = (head)->first;					 \
+	     pos && ({ n = pos->next; 1; }) && 				 \
+		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
+	     pos = n)
+
+#endif
diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 5ed41e0a..e11e05b1 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -37,6 +37,8 @@
 
 #include <ofono/log.h>
 
+#include "list.h"
+
 #include "qmi.h"
 #include "ctl.h"
 
@@ -54,9 +56,9 @@ struct qmi_device {
 	bool close_on_unref;
 	guint read_watch;
 	guint write_watch;
-	GQueue *req_queue;
-	GQueue *control_queue;
-	GQueue *service_queue;
+	struct list_head req_queue;
+	struct list_head control_queue;
+	struct list_head service_queue;
 	GQueue *discovery_queue;
 	uint8_t next_control_tid;
 	uint16_t next_service_tid;
@@ -102,6 +104,7 @@ struct qmi_result {
 };
 
 struct qmi_request {
+	struct list_head node;
 	uint16_t tid;
 	uint8_t client;
 	void *buf;
@@ -172,6 +175,7 @@ static int __request_submit(struct qmi_device* device,
 	uint16_t headroom;
 
 	req = calloc(1, sizeof(*req));
+	INIT_LIST_HEAD(&req->node);
 
 	if (service == QMI_SERVICE_CONTROL)
 		headroom = QMI_CONTROL_HDR_SIZE;
@@ -223,7 +227,7 @@ static int __request_submit(struct qmi_device* device,
 		req->tid = hdr->transaction;
 	}
 
-	g_queue_push_tail(device->req_queue, req);
+	list_add_tail(&req->node, &device->req_queue);
 
 	wakeup_writer(device);
 
@@ -238,12 +242,17 @@ static void __request_free(gpointer data, gpointer user_data)
 	g_free(req);
 }
 
-static gint __request_compare(gconstpointer a, gconstpointer b)
+static struct qmi_request* __request_find_by_tid(struct list_head *queue,
+						uint16_t tid)
 {
-	const struct qmi_request *req = a;
-	uint16_t tid = GPOINTER_TO_UINT(b);
+	struct qmi_request* req;
+
+	list_for_each_entry(req, queue, node) {
+		if (req->tid == tid)
+			return req;
+	}
 
-	return req->tid - tid;
+	return NULL;
 }
 
 static void __discovery_free(gpointer data, gpointer user_data)
@@ -662,10 +671,11 @@ static gboolean can_write_data(GIOChannel *channel, GIOCondition cond,
 	struct qmi_request *req;
 	ssize_t bytes_written;
 
-	req = g_queue_pop_head(device->req_queue);
-	if (!req)
+	if (list_empty(&device->req_queue))
 		return FALSE;
 
+	req = list_first_entry(&device->req_queue, struct qmi_request, node);
+
 	bytes_written = write(device->fd, req->buf, req->len);
 	if (bytes_written < 0)
 		return FALSE;
@@ -679,14 +689,14 @@ static gboolean can_write_data(GIOChannel *channel, GIOCondition cond,
 	hdr = req->buf;
 
 	if (hdr->service == QMI_SERVICE_CONTROL)
-		g_queue_push_tail(device->control_queue, req);
+		list_move(&req->node, &device->control_queue);
 	else
-		g_queue_push_tail(device->service_queue, req);
+		list_move(&req->node, &device->service_queue);
 
 	g_free(req->buf);
 	req->buf = NULL;
 
-	if (g_queue_get_length(device->req_queue) > 0)
+	if (!list_empty(&device->req_queue))
 		return TRUE;
 
 	return FALSE;
@@ -768,7 +778,6 @@ static void handle_packet(struct qmi_device *device,
 		const struct qmi_control_hdr *control = buf;
 		const struct qmi_message_hdr *msg;
 		unsigned int tid;
-		GList *list;
 
 		/* Ignore control messages with client identifier */
 		if (hdr->client != 0x00)
@@ -789,19 +798,15 @@ static void handle_packet(struct qmi_device *device,
 			return;
 		}
 
-		list = g_queue_find_custom(device->control_queue,
-				GUINT_TO_POINTER(tid), __request_compare);
-		if (!list)
+		req = __request_find_by_tid(&device->control_queue, tid);
+		if (!req)
 			return;
 
-		req = list->data;
-
-		g_queue_delete_link(device->control_queue, list);
+		list_del(&req->node);
 	} else {
 		const struct qmi_service_hdr *service = buf;
 		const struct qmi_message_hdr *msg;
 		unsigned int tid;
-		GList *list;
 
 		msg = buf + QMI_SERVICE_HDR_SIZE;
 
@@ -818,14 +823,11 @@ static void handle_packet(struct qmi_device *device,
 			return;
 		}
 
-		list = g_queue_find_custom(device->service_queue,
-				GUINT_TO_POINTER(tid), __request_compare);
-		if (!list)
+		req = __request_find_by_tid(&device->service_queue, tid);
+		if (!req)
 			return;
 
-		req = list->data;
-
-		g_queue_delete_link(device->service_queue, list);
+		list_del(&req->node);
 	}
 
 	if (req->callback)
@@ -957,9 +959,9 @@ struct qmi_device *qmi_device_new(int fd)
 
 	g_io_channel_unref(device->io);
 
-	device->req_queue = g_queue_new();
-	device->control_queue = g_queue_new();
-	device->service_queue = g_queue_new();
+	INIT_LIST_HEAD(&device->req_queue);
+	INIT_LIST_HEAD(&device->control_queue);
+	INIT_LIST_HEAD(&device->service_queue);
 	device->discovery_queue = g_queue_new();
 
 	device->service_list = g_hash_table_new_full(g_direct_hash,
@@ -983,6 +985,8 @@ struct qmi_device *qmi_device_ref(struct qmi_device *device)
 
 void qmi_device_unref(struct qmi_device *device)
 {
+	struct qmi_request *req, *req_t;
+
 	if (!device)
 		return;
 
@@ -991,14 +995,32 @@ void qmi_device_unref(struct qmi_device *device)
 
 	__debug_device(device, "device %p free", device);
 
-	g_queue_foreach(device->control_queue, __request_free, NULL);
-	g_queue_free(device->control_queue);
+	list_for_each_entry_safe(req, req_t, &device->control_queue, node) {
+		/* FIXME: this fails to free user data attached to the
+		 * request with a 'destroy' function
+		 */
+		list_del(&req->node);
+		g_free(req->buf);
+		g_free(req);
+	}
 
-	g_queue_foreach(device->service_queue, __request_free, NULL);
-	g_queue_free(device->service_queue);
+	list_for_each_entry_safe(req, req_t, &device->service_queue, node) {
+		/* FIXME: this fails to free user data attached to the
+		 * request with a 'destroy' function
+		 */
+		list_del(&req->node);
+		g_free(req->buf);
+		g_free(req);
+	}
 
-	g_queue_foreach(device->req_queue, __request_free, NULL);
-	g_queue_free(device->req_queue);
+	list_for_each_entry_safe(req, req_t, &device->req_queue, node) {
+		/* FIXME: this fails to free user data attached to the
+		 * request with a 'destroy' function
+		 */
+		list_del(&req->node);
+		g_free(req->buf);
+		g_free(req);
+	}
 
 	g_queue_foreach(device->discovery_queue, __discovery_free, NULL);
 	g_queue_free(device->discovery_queue);
@@ -1008,7 +1030,7 @@ void qmi_device_unref(struct qmi_device *device)
 
 	if (device->read_watch > 0)
 		g_source_remove(device->read_watch);
-
+ 
 	if (device->close_on_unref)
 		close(device->fd);
 
@@ -1194,29 +1216,17 @@ static gboolean discover_reply(gpointer user_data)
 	struct discover_data *data = user_data;
 	struct qmi_device *device = data->device;
 	unsigned int tid = data->tid;
-	GList *list;
 	struct qmi_request *req = NULL;
 
 	data->timeout = 0;
 
 	/* remove request from queues */
 	if (tid != 0) {
-		list = g_queue_find_custom(device->req_queue,
-				GUINT_TO_POINTER(tid), __request_compare);
+		req = __request_find_by_tid(&device->req_queue, tid);
+		if (!req) 
+			req = __request_find_by_tid(&device->control_queue,tid);
 
-		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);
-
-			if (list) {
-				req = list->data;
-				g_queue_delete_link(device->control_queue,
-								list);
-			}
-		}
+		list_del(&req->node);
 	}
 
 	if (data->func)
@@ -2274,40 +2284,10 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	return 1;
 }
 
-static GQueue *remove_client(GQueue *queue, uint8_t client)
-{
-	GQueue *new_queue;
-	GList *list;
-
-	new_queue = g_queue_new();
-
-	while (1) {
-		struct qmi_request *req;
-
-		list = g_queue_pop_head_link(queue);
-		if (!list)
-			break;
-
-		req = list->data;
-
-		if (!req->client || req->client != client) {
-			g_queue_push_tail_link(new_queue, list);
-			continue;
-		}
-
-		service_send_free(req->user_data);
-
-		__request_free(req, NULL);
-	}
-
-	g_queue_free(queue);
-
-	return new_queue;
-}
-
 bool qmi_service_cancel_all(struct qmi_service *service)
 {
 	struct qmi_device *device;
+	struct qmi_request *req, *req_t;
 
 	if (!service)
 		return false;
@@ -2319,11 +2299,27 @@ bool qmi_service_cancel_all(struct qmi_service *service)
 	if (!device)
 		return false;
 
-	device->req_queue = remove_client(device->req_queue,
-						service->client_id);
+	list_for_each_entry_safe(req, req_t, &device->req_queue, node) {
+		if (req->client == service->client_id) {
+			list_del(&req->node);
+
+			service_send_free(req->user_data);
+
+			g_free(req->buf);
+			g_free(req);
+		}
+	}
+
+	list_for_each_entry_safe(req, req_t, &device->service_queue, node) {
+		if (req->client == service->client_id) {
+			list_del(&req->node);
+
+			service_send_free(req->user_data);
 
-	device->service_queue = remove_client(device->service_queue,
-							service->client_id);
+			g_free(req->buf);
+			g_free(req);
+		}
+	}
 
 	return true;
 }
-- 
2.15.1

Re: [RFC PATCH 12/30] qmi: replace GQueues for requests

[Denis Kenzior]

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

Hi Jonas,

On 03/28/2018 01:59 PM, Jonas Bonn wrote:
> This brings in list.h from the Linux kernel and replaces the GQueues
> used for enqueuing requests with struct list_heads.  These list
> operations are both more efficient and generally easier to work with.

In general I agree, but do they really have any significant advantages 
for our use cases?  Its not like we maintain items on multiple lists...

> Since ofono and Linux share a common license, this code borrowing should
> not be problematic in any way.

While there's no problem with the license, I really don't want QMI to 
look completely alien to the rest of the code base.  We use GLib style 
lists everywhere, so I'm really hesitant to go this way.  I think I'd 
rather we stuck to that (or switch to ell) for code maintenance & 
readability reasons.

Regards,
-Denis

Re: [RFC PATCH 12/30] qmi: replace GQueues for requests

[Jonas Bonn]

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



On 28/03/18 22:16, Denis Kenzior wrote:
> Hi Jonas,
>
> On 03/28/2018 01:59 PM, Jonas Bonn wrote:
>> This brings in list.h from the Linux kernel and replaces the GQueues
>> used for enqueuing requests with struct list_heads.  These list
>> operations are both more efficient and generally easier to work with.
>
> In general I agree, but do they really have any significant advantages 
> for our use cases?  Its not like we maintain items on multiple lists...

By the end of the series we have requests on both the device list and on 
the service list.

The biggest advantage is when removing items from the lists that they 
are on.  Since the object on the list knows how it is attached to the 
list (via its list head), it can detach itself from the list without 
need access to the "list object" itself.  With glib lists, a request on 
both a device and service list would need to be able to access the 
device in order to get the device's list in order to be able to ask the 
list to delete it and then do the same for the service.  With the Linux 
lists you just do "detach me" (list_del(node)) and you're done... much 
better code

Aside from that, the Linux lists are really simple macros, not function 
calls; all the state they require is in the list items itself, no extra 
list entry holders required; they are cache friendly; etc.  Lots of 
advantages, though these are mostly moot in ofono where list performance 
isn't really an issue.

Anyway, please bear with these lists for this cleanup.  When the shared 
service cleanup is done, we can return to GQueues if necessary and then 
we'll have a clearer picture of what the difference is.  As things stand 
though, things are complicated enough that the GQueues are just in the way.

Trying to find a series of small patches through this cleanup hasn't 
been easy.  The first attempts I did ended up ballooning because when 
you start trying to make the change in patch 29 you pretty much 
automatically end up with most of the other changes in the preceding 28 
patches before this system is operational again... so the alternative to 
some of these (possibly) oddball patches was one _big_ patch that wasn't 
reviewable.

/Jonas

>
>> Since ofono and Linux share a common license, this code borrowing should
>> not be problematic in any way.
>
> While there's no problem with the license, I really don't want QMI to 
> look completely alien to the rest of the code base.  We use GLib style 
> lists everywhere, so I'm really hesitant to go this way.  I think I'd 
> rather we stuck to that (or switch to ell) for code maintenance & 
> readability reasons.
>
> Regards,
> -Denis

Re: [RFC PATCH 12/30] qmi: replace GQueues for requests

[Denis Kenzior]

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

Hi Jonas,
> Anyway, please bear with these lists for this cleanup.  When the shared 
> service cleanup is done, we can return to GQueues if necessary and then 
> we'll have a clearer picture of what the difference is.  As things stand 
> though, things are complicated enough that the GQueues are just in the way.

I'm not introducing Linux lists just to remove them later.  That just 
won't happen :)

> 
> Trying to find a series of small patches through this cleanup hasn't 
> been easy.  The first attempts I did ended up ballooning because when 
> you start trying to make the change in patch 29 you pretty much 
> automatically end up with most of the other changes in the preceding 28 
> patches before this system is operational again... so the alternative to 
> some of these (possibly) oddball patches was one _big_ patch that wasn't 
> reviewable.
> 

I feel your pain :)  I think before you go crazy we should nail down 
your proposed API semantics and how you want things to look like.  I 
already outlined my vision in a previous email and it sounds like you're 
trying for something completely different.  So the obvious question is 
why are we reinventing the wheel again?

Regards,
-Denis

[RFC PATCH 13/30] qmi: assume version_list is up to date

[Jonas Bonn]

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

The way things are currently coded, the gobi plugin calls
qmi_device_discover and does nothing else until it succeeds.  As such,
we can safely assume that the version_list is set up when we go to
create a service.
---
 drivers/qmimodem/qmi.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index e11e05b1..3eb9e8c3 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -2016,20 +2016,9 @@ static bool service_create(struct qmi_device *device, bool shared,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	if (device->version_list) {
-		service_create_discover(device->version_count,
-						device->version_list, data);
-		goto done;
-	}
-
-	if (qmi_device_discover(device, service_create_discover, data, NULL))
-		goto done;
+	service_create_discover(device->version_count,
+					device->version_list, data);
 
-	g_free(data);
-
-	return false;
-
-done:
 	data->timeout = g_timeout_add_seconds(8, service_create_reply, data);
 	__qmi_device_discovery_started(device, &data->super);
 
-- 
2.15.1

[RFC PATCH 14/30] qmi: absorb service_create_discover into service_create

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 3eb9e8c3..5af35a23 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1974,35 +1974,13 @@ done:
 	__qmi_device_discovery_complete(data->device, &data->super);
 }
 
-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;
-	unsigned char client_req[] = { 0x01, 0x01, 0x00, data->type };
-	unsigned int i;
-
-	__debug_device(device, "service create [type=%d]", data->type);
-
-	for (i = 0; i < count; i++) {
-		if (list[i].type == data->type) {
-			data->major = list[i].major;
-			data->minor = list[i].minor;
-			break;
-		}
-	}
-
-	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
-			QMI_CTL_GET_CLIENT_ID,
-			client_req, sizeof(client_req),
-			service_create_callback, data);
-}
-
 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 service_create_data *data;
+	unsigned char client_req[] = { 0x01, 0x01, 0x00, type };
+	int i;
 
 	data = g_try_new0(struct service_create_data, 1);
 	if (!data)
@@ -2016,8 +1994,20 @@ static bool service_create(struct qmi_device *device, bool shared,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	service_create_discover(device->version_count,
-					device->version_list, data);
+	__debug_device(device, "service create [type=%d]", type);
+
+	for (i = 0; i < device->version_count; i++) {
+		if (device->version_list[i].type == data->type) {
+			data->major = device->version_list[i].major;
+			data->minor = device->version_list[i].minor;
+			break;
+		}
+	}
+
+	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
+			QMI_CTL_GET_CLIENT_ID,
+			client_req, sizeof(client_req),
+			service_create_callback, data);
 
 	data->timeout = g_timeout_add_seconds(8, service_create_reply, data);
 	__qmi_device_discovery_started(device, &data->super);
-- 
2.15.1

[RFC PATCH 15/30] qmi: drop discovery_queue

[Jonas Bonn]

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

The discovery queue mechanism provides a form of timeout handling so
that device discovery and service creation operations return in bounded
time.  For the modem I have available, these operations never time out
and the "failure" paths are never exercised.

A better way to handle this would be to put proper timeout handling into
the QMI protocol handling itself.  This will be done in a later patch,
but for now we drop this pessimistic timeout handling altogether in
order to simplify the code and thereby facilitate further cleanups.
---
 drivers/qmimodem/qmi.c | 87 +++++---------------------------------------------
 1 file changed, 8 insertions(+), 79 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 5af35a23..f212aa96 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -45,10 +45,6 @@
 typedef void (*qmi_message_func_t)(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data);
 
-struct discovery {
-	qmi_destroy_func_t destroy;
-};
-
 struct qmi_device {
 	int ref_count;
 	int fd;
@@ -59,7 +55,6 @@ struct qmi_device {
 	struct list_head req_queue;
 	struct list_head control_queue;
 	struct list_head service_queue;
-	GQueue *discovery_queue;
 	uint8_t next_control_tid;
 	uint16_t next_service_tid;
 	qmi_debug_func_t debug_func;
@@ -255,14 +250,6 @@ static struct qmi_request* __request_find_by_tid(struct list_head *queue,
 	return NULL;
 }
 
-static void __discovery_free(gpointer data, gpointer user_data)
-{
-	struct discovery *d = data;
-	qmi_destroy_func_t destroy = d->destroy;
-
-	destroy(d);
-}
-
 static void __notify_free(gpointer data, gpointer user_data)
 {
 	struct qmi_notify *notify = data;
@@ -894,21 +881,6 @@ static void read_watch_destroy(gpointer user_data)
 	device->read_watch = 0;
 }
 
-static void __qmi_device_discovery_started(struct qmi_device *device,
-						struct discovery *d)
-{
-	g_queue_push_tail(device->discovery_queue, d);
-}
-
-static void __qmi_device_discovery_complete(struct qmi_device *device,
-						struct discovery *d)
-{
-	if (g_queue_remove(device->discovery_queue, d) != TRUE)
-		return;
-
-	__discovery_free(d, NULL);
-}
-
 static void service_destroy(gpointer data)
 {
 	struct qmi_service *service = data;
@@ -962,7 +934,6 @@ struct qmi_device *qmi_device_new(int fd)
 	INIT_LIST_HEAD(&device->req_queue);
 	INIT_LIST_HEAD(&device->control_queue);
 	INIT_LIST_HEAD(&device->service_queue);
-	device->discovery_queue = g_queue_new();
 
 	device->service_list = g_hash_table_new_full(g_direct_hash,
 					g_direct_equal, NULL, service_destroy);
@@ -1022,9 +993,6 @@ void qmi_device_unref(struct qmi_device *device)
 		g_free(req);
 	}
 
-	g_queue_foreach(device->discovery_queue, __discovery_free, NULL);
-	g_queue_free(device->discovery_queue);
-
 	if (device->write_watch > 0)
 		g_source_remove(device->write_watch);
 
@@ -1108,12 +1076,10 @@ static const void *tlv_get(const void *data, uint16_t size,
 }
 
 struct discover_data {
-	struct discovery super;
 	struct qmi_device *device;
 	qmi_discover_func_t func;
 	void *user_data;
 	qmi_destroy_func_t destroy;
-	uint8_t tid;
 	guint timeout;
 };
 
@@ -1208,33 +1174,21 @@ done:
 	if (data->func)
 		data->func(count, list, data->user_data);
 
-	__qmi_device_discovery_complete(data->device, &data->super);
+	discover_data_free(data);
 }
 
 static gboolean discover_reply(gpointer user_data)
 {
 	struct discover_data *data = user_data;
 	struct qmi_device *device = data->device;
-	unsigned int tid = data->tid;
-	struct qmi_request *req = NULL;
 
 	data->timeout = 0;
 
-	/* remove request from queues */
-	if (tid != 0) {
-		req = __request_find_by_tid(&device->req_queue, tid);
-		if (!req) 
-			req = __request_find_by_tid(&device->control_queue,tid);
-
-		list_del(&req->node);
-	}
-
 	if (data->func)
 		data->func(device->version_count,
 				device->version_list, data->user_data);
 
-	__qmi_device_discovery_complete(data->device, &data->super);
-	__request_free(req, NULL);
+	discover_data_free(data);
 
 	return FALSE;
 }
@@ -1253,7 +1207,6 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 	if (!data)
 		return false;
 
-	data->super.destroy = discover_data_free;
 	data->device = device;
 	data->func = func;
 	data->user_data = user_data;
@@ -1261,7 +1214,6 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 
 	if (device->version_list) {
 		data->timeout = g_timeout_add_seconds(0, discover_reply, data);
-		__qmi_device_discovery_started(device, &data->super);
 		return true;
 	}
 
@@ -1269,13 +1221,6 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data);
 
-	data->tid = device->next_control_tid - 1;
-	if (data->tid == 0)
-		data->tid = 255;
-
-	data->timeout = g_timeout_add_seconds(5, discover_reply, data);
-	__qmi_device_discovery_started(device, &data->super);
-
 	return true;
 }
 
@@ -1878,7 +1823,6 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
 }
 
 struct service_create_data {
-	struct discovery super;
 	struct qmi_device *device;
 	bool shared;
 	uint8_t type;
@@ -1905,18 +1849,6 @@ static void service_create_data_free(gpointer user_data)
 	g_free(data);
 }
 
-static gboolean service_create_reply(gpointer user_data)
-{
-	struct service_create_data *data = user_data;
-
-	data->timeout = 0;
-	data->func(NULL, data->user_data);
-
-	__qmi_device_discovery_complete(data->device, &data->super);
-
-	return FALSE;
-}
-
 static void service_create_callback(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data)
 {
@@ -1971,7 +1903,7 @@ done:
 	data->func(service, data->user_data);
 	qmi_service_unref(service);
 
-	__qmi_device_discovery_complete(data->device, &data->super);
+	service_create_data_free(data);
 }
 
 static bool service_create(struct qmi_device *device, bool shared,
@@ -1986,7 +1918,6 @@ static bool service_create(struct qmi_device *device, bool shared,
 	if (!data)
 		return false;
 
-	data->super.destroy = service_create_data_free;
 	data->device = device;
 	data->shared = shared;
 	data->type = type;
@@ -2009,9 +1940,6 @@ static bool service_create(struct qmi_device *device, bool shared,
 			client_req, sizeof(client_req),
 			service_create_callback, data);
 
-	data->timeout = g_timeout_add_seconds(8, service_create_reply, data);
-	__qmi_device_discovery_started(device, &data->super);
-
 	return true;
 }
 
@@ -2029,7 +1957,6 @@ bool qmi_service_create(struct qmi_device *device,
 }
 
 struct service_create_shared_data {
-	struct discovery super;
 	struct qmi_service *service;
 	struct qmi_device *device;
 	qmi_create_func_t func;
@@ -2062,7 +1989,7 @@ static gboolean service_create_shared_reply(gpointer user_data)
 	data->timeout = 0;
 	data->func(data->service, data->user_data);
 
-	__qmi_device_discovery_complete(data->device, &data->super);
+	service_create_shared_data_free(data);
 
 	return FALSE;
 }
@@ -2089,16 +2016,18 @@ bool qmi_service_create_shared(struct qmi_device *device,
 		if (!data)
 			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;
 
+		/* FIXME: There's a little window here where we might leak
+		 * data... we'll leave it for now because the correct fix
+		 * is a rework of the way services are shared.
+		 */
 		data->timeout = g_timeout_add(0,
 					service_create_shared_reply, data);
-		__qmi_device_discovery_started(device, &data->super);
 
 		return 0;
 	}
-- 
2.15.1

[RFC PATCH 16/30] qmi: make services always shared

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index f212aa96..fcb9a5e4 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -77,7 +77,6 @@ struct qmi_device {
 struct qmi_service {
 	int ref_count;
 	struct qmi_device *device;
-	bool shared;
 	uint8_t type;
 	uint16_t major;
 	uint16_t minor;
@@ -266,9 +265,6 @@ static gboolean __service_compare_shared(gpointer key, gpointer value,
 	struct qmi_service *service = value;
 	uint8_t type = GPOINTER_TO_UINT(user_data);
 
-	if (!service->shared)
-		return FALSE;
-
 	if (service->type == type)
 		return TRUE;
 
@@ -1824,7 +1820,6 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
 
 struct service_create_data {
 	struct qmi_device *device;
-	bool shared;
 	uint8_t type;
 	uint16_t major;
 	uint16_t minor;
@@ -1883,7 +1878,6 @@ static void service_create_callback(uint16_t message, uint16_t length,
 
 	service->ref_count = 1;
 	service->device = data->device;
-	service->shared = data->shared;
 
 	service->type = data->type;
 	service->major = data->major;
@@ -1906,7 +1900,7 @@ done:
 	service_create_data_free(data);
 }
 
-static bool service_create(struct qmi_device *device, bool shared,
+static bool service_create(struct qmi_device *device,
 				uint8_t type, qmi_create_func_t func,
 				void *user_data, qmi_destroy_func_t destroy)
 {
@@ -1919,7 +1913,6 @@ static bool service_create(struct qmi_device *device, bool shared,
 		return false;
 
 	data->device = device;
-	data->shared = shared;
 	data->type = type;
 	data->func = func;
 	data->user_data = user_data;
@@ -1943,19 +1936,6 @@ static bool service_create(struct qmi_device *device, bool shared,
 	return true;
 }
 
-bool qmi_service_create(struct qmi_device *device,
-				uint8_t type, qmi_create_func_t func,
-				void *user_data, qmi_destroy_func_t destroy)
-{
-	if (!device || !func)
-		return false;
-
-	if (type == QMI_SERVICE_CONTROL)
-		return false;
-
-	return service_create(device, false, type, func, user_data, destroy);
-}
-
 struct service_create_shared_data {
 	struct qmi_service *service;
 	struct qmi_device *device;
@@ -2032,7 +2012,15 @@ bool qmi_service_create_shared(struct qmi_device *device,
 		return 0;
 	}
 
-	return service_create(device, true, type, func, user_data, destroy);
+	return service_create(device, type, func, user_data, destroy);
+}
+
+bool qmi_service_create(struct qmi_device *device,
+				uint8_t type, qmi_create_func_t func,
+				void *user_data, qmi_destroy_func_t destroy)
+{
+	return qmi_service_create_shared(device, type, func,
+						user_data, destroy);
 }
 
 static void service_release_callback(uint16_t message, uint16_t length,
-- 
2.15.1

[RFC PATCH 17/30] qmi: switch service_list to list_head type

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 126 +++++++++++++++++++------------------------------
 1 file changed, 49 insertions(+), 77 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index fcb9a5e4..a3a04768 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -64,7 +64,7 @@ struct qmi_device {
 	char *version_str;
 	struct qmi_version *version_list;
 	uint8_t version_count;
-	GHashTable *service_list;
+	struct list_head service_list;
 	unsigned int release_users;
 	qmi_shutdown_func_t shutdown_func;
 	void *shutdown_user_data;
@@ -75,6 +75,7 @@ struct qmi_device {
 };
 
 struct qmi_service {
+	struct list_head node;
 	int ref_count;
 	struct qmi_device *device;
 	uint8_t type;
@@ -259,18 +260,6 @@ static void __notify_free(gpointer data, gpointer user_data)
 	g_free(notify);
 }
 
-static gboolean __service_compare_shared(gpointer key, gpointer value,
-							gpointer user_data)
-{
-	struct qmi_service *service = value;
-	uint8_t type = GPOINTER_TO_UINT(user_data);
-
-	if (service->type == type)
-		return TRUE;
-
-	return FALSE;
-}
-
 static void __hexdump(const char dir, const unsigned char *buf, size_t len,
 				qmi_debug_func_t function, void *user_data)
 {
@@ -702,10 +691,9 @@ static void wakeup_writer(struct qmi_device *device)
 				can_write_data, device, write_watch_destroy);
 }
 
-static void service_notify(gpointer key, gpointer value, gpointer user_data)
+static void service_notify(struct qmi_service* service,
+				struct qmi_result* result)
 {
-	struct qmi_service *service = value;
-	struct qmi_result *result = user_data;
 	GList *list;
 
 	for (list = g_list_first(service->notify_list); list;
@@ -723,7 +711,6 @@ static void handle_indication(struct qmi_device *device,
 {
 	struct qmi_service *service;
 	struct qmi_result result;
-	unsigned int hash_id;
 
 	if (service_type == QMI_SERVICE_CONTROL)
 		return;
@@ -734,20 +721,10 @@ static void handle_indication(struct qmi_device *device,
 	result.data = data;
 	result.length = length;
 
-	if (client_id == 0xff) {
-		g_hash_table_foreach(device->service_list,
-						service_notify, &result);
-		return;
+	list_for_each_entry(service, &device->service_list, node) {
+		if (client_id == 0xff || service_type == service->type)
+			service_notify(service, &result);
 	}
-
-	hash_id = service_type | (client_id << 8);
-
-	service = g_hash_table_lookup(device->service_list,
-					GUINT_TO_POINTER(hash_id));
-	if (!service)
-		return;
-
-	service_notify(NULL, service, &result);
 }
 
 static void handle_packet(struct qmi_device *device,
@@ -877,16 +854,6 @@ static void read_watch_destroy(gpointer user_data)
 	device->read_watch = 0;
 }
 
-static void service_destroy(gpointer data)
-{
-	struct qmi_service *service = data;
-
-	if (!service->device)
-		return;
-
-	service->device = NULL;
-}
-
 struct qmi_device *qmi_device_new(int fd)
 {
 	struct qmi_device *device;
@@ -931,8 +898,7 @@ struct qmi_device *qmi_device_new(int fd)
 	INIT_LIST_HEAD(&device->control_queue);
 	INIT_LIST_HEAD(&device->service_queue);
 
-	device->service_list = g_hash_table_new_full(g_direct_hash,
-					g_direct_equal, NULL, service_destroy);
+	INIT_LIST_HEAD(&device->service_list);
 
 	device->next_control_tid = 1;
 	device->next_service_tid = 256;
@@ -953,6 +919,7 @@ struct qmi_device *qmi_device_ref(struct qmi_device *device)
 void qmi_device_unref(struct qmi_device *device)
 {
 	struct qmi_request *req, *req_t;
+	struct qmi_service *service, *service_t;
 
 	if (!device)
 		return;
@@ -1001,7 +968,11 @@ void qmi_device_unref(struct qmi_device *device)
 	if (device->shutdown_source)
 		g_source_remove(device->shutdown_source);
 
-	g_hash_table_destroy(device->service_list);
+	list_for_each_entry_safe(service, service_t,
+					&device->service_list, node) {
+		list_del(&service->node);
+		g_free(service);
+	}
 
 	g_free(device->version_str);
 	g_free(device->version_list);
@@ -1818,6 +1789,19 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
 	return true;
 }
 
+static struct qmi_service* device_get_service(struct qmi_device* device,
+							uint8_t type)
+{
+	struct qmi_service* service;
+
+	list_for_each_entry(service, &device->service_list, node) {
+		if (service->type == type)
+			return service;
+	}
+
+	return NULL;
+}
+
 struct service_create_data {
 	struct qmi_device *device;
 	uint8_t type;
@@ -1853,7 +1837,6 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	const struct qmi_result_code *result_code;
 	const struct qmi_client_id *client_id;
 	uint16_t len;
-	unsigned int hash_id;
 
 	result_code = tlv_get(buffer, length, 0x02, &len);
 	if (!result_code)
@@ -1876,6 +1859,7 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	if (!service)
 		goto done;
 
+	INIT_LIST_HEAD(&service->node);
 	service->ref_count = 1;
 	service->device = data->device;
 
@@ -1888,10 +1872,8 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	__debug_device(device, "service created [client=%d,type=%d]",
 					service->client_id, service->type);
 
-	hash_id = service->type | (service->client_id << 8);
-
-	g_hash_table_replace(device->service_list,
-				GUINT_TO_POINTER(hash_id), service);
+	/* FIXME: maybe sort these by type...??? */
+	list_add_tail(&service->node, &device->service_list);
 
 done:
 	data->func(service, data->user_data);
@@ -1978,8 +1960,8 @@ 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;
 	struct qmi_service *service;
-	unsigned int type_val = type;
 
 	if (!device || !func)
 		return false;
@@ -1987,32 +1969,27 @@ 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_compare_shared, GUINT_TO_POINTER(type_val));
-	if (service) {
-		struct service_create_shared_data *data;
-
-		data = g_try_new0(struct service_create_shared_data, 1);
-		if (!data)
-			return false;
+	service = device_get_service(device, type);
+	if (!service)
+		return service_create(device, type, func, user_data, destroy);
 
-		data->service = qmi_service_ref(service);
-		data->device = device;
-		data->func = func;
-		data->user_data = user_data;
-		data->destroy = destroy;
+	data = g_try_new0(struct service_create_shared_data, 1);
+	if (!data)
+		return false;
 
-		/* FIXME: There's a little window here where we might leak
-		 * data... we'll leave it for now because the correct fix
-		 * is a rework of the way services are shared.
-		 */
-		data->timeout = g_timeout_add(0,
-					service_create_shared_reply, data);
+	data->service = qmi_service_ref(service);
+	data->device = device;
+	data->func = func;
+	data->user_data = user_data;
+	data->destroy = destroy;
 
-		return 0;
-	}
+	/* FIXME: There's a little window here where we might leak
+	 * data... we'll leave it for now because the correct fix
+	 * is a rework of the way services are shared.
+	 */
+	data->timeout = g_timeout_add(0, service_create_shared_reply, data);
 
-	return service_create(device, type, func, user_data, destroy);
+	return true;
 }
 
 bool qmi_service_create(struct qmi_device *device,
@@ -2046,8 +2023,6 @@ struct qmi_service *qmi_service_ref(struct qmi_service *service)
 
 void qmi_service_unref(struct qmi_service *service)
 {
-	unsigned int hash_id;
-
 	if (!service)
                 return;
 
@@ -2062,10 +2037,7 @@ void qmi_service_unref(struct qmi_service *service)
 	qmi_service_cancel_all(service);
 	qmi_service_unregister_all(service);
 
-	hash_id = service->type | (service->client_id << 8);
-
-	g_hash_table_steal(service->device->service_list,
-					GUINT_TO_POINTER(hash_id));
+	list_del(&service->node);
 
 	service->device->release_users++;
 
-- 
2.15.1

[RFC PATCH 18/30] qmi: switch notify_list to list_head type

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index a3a04768..75c68434 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -82,7 +82,7 @@ struct qmi_service {
 	uint16_t major;
 	uint16_t minor;
 	uint8_t client_id;
-	GList *notify_list;
+	struct list_head notify_list;
 };
 
 struct qmi_param {
@@ -109,6 +109,7 @@ struct qmi_request {
 };
 
 struct qmi_notify {
+	struct list_head node;
 	uint16_t message;
 	qmi_result_func_t callback;
 	void *user_data;
@@ -250,16 +251,6 @@ static struct qmi_request* __request_find_by_tid(struct list_head *queue,
 	return NULL;
 }
 
-static void __notify_free(gpointer data, gpointer user_data)
-{
-	struct qmi_notify *notify = data;
-
-	if (notify->destroy)
-		notify->destroy(notify->user_data);
-
-	g_free(notify);
-}
-
 static void __hexdump(const char dir, const unsigned char *buf, size_t len,
 				qmi_debug_func_t function, void *user_data)
 {
@@ -694,12 +685,9 @@ static void wakeup_writer(struct qmi_device *device)
 static void service_notify(struct qmi_service* service,
 				struct qmi_result* result)
 {
-	GList *list;
-
-	for (list = g_list_first(service->notify_list); list;
-						list = g_list_next(list)) {
-		struct qmi_notify *notify = list->data;
+	struct qmi_notify *notify;
 
+	list_for_each_entry(notify, &service->notify_list, node) {
 		if (notify->message == result->message)
 			notify->callback(result, notify->user_data);
 	}
@@ -1860,6 +1848,7 @@ static void service_create_callback(uint16_t message, uint16_t length,
 		goto done;
 
 	INIT_LIST_HEAD(&service->node);
+	INIT_LIST_HEAD(&service->notify_list);
 	service->ref_count = 1;
 	service->device = data->device;
 
@@ -2205,25 +2194,31 @@ bool qmi_service_register(struct qmi_service *service,
 	if (!notify)
 		return false;
 
+	INIT_LIST_HEAD(&notify->node);
 	notify->message = message;
 	notify->callback = func;
 	notify->user_data = user_data;
 	notify->destroy = destroy;
 
-	service->notify_list = g_list_append(service->notify_list, notify);
+	list_add_tail(&notify->node, &service->notify_list);
 
 	return true;
 }
 
 bool qmi_service_unregister_all(struct qmi_service *service)
 {
+	struct qmi_notify *notify, *notify_t;
+
 	if (!service)
 		return false;
 
-	g_list_foreach(service->notify_list, __notify_free, NULL);
-	g_list_free(service->notify_list);
-
-	service->notify_list = NULL;
+	list_for_each_entry_safe(notify, notify_t,
+				&service->notify_list, node) {
+		list_del(&notify->node);
+		if (notify->destroy)
+			notify->destroy(notify->user_data);
+		g_free(notify);
+	}
 
 	return true;
 }
-- 
2.15.1

[RFC PATCH 19/30] qmi: drop unused struct field

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 75c68434..a65a4bf4 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1909,7 +1909,6 @@ static bool service_create(struct qmi_device *device,
 
 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;
@@ -1967,7 +1966,6 @@ bool qmi_service_create_shared(struct qmi_device *device,
 		return false;
 
 	data->service = qmi_service_ref(service);
-	data->device = device;
 	data->func = func;
 	data->user_data = user_data;
 	data->destroy = destroy;
-- 
2.15.1

[RFC PATCH 20/30] qmi: reference version_info from device in services

[Jonas Bonn]

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

The device struct already maintains version information, so let the
service instances reference that data directly instead of copying it.
---
 drivers/qmimodem/qmi.c | 44 +++++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index a65a4bf4..43c54482 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -78,9 +78,7 @@ struct qmi_service {
 	struct list_head node;
 	int ref_count;
 	struct qmi_device *device;
-	uint8_t type;
-	uint16_t major;
-	uint16_t minor;
+	struct qmi_version *info;
 	uint8_t client_id;
 	struct list_head notify_list;
 };
@@ -710,7 +708,7 @@ static void handle_indication(struct qmi_device *device,
 	result.length = length;
 
 	list_for_each_entry(service, &device->service_list, node) {
-		if (client_id == 0xff || service_type == service->type)
+		if (client_id == 0xff || service_type == service->info->type)
 			service_notify(service, &result);
 	}
 }
@@ -1783,7 +1781,7 @@ static struct qmi_service* device_get_service(struct qmi_device* device,
 	struct qmi_service* service;
 
 	list_for_each_entry(service, &device->service_list, node) {
-		if (service->type == type)
+		if (service->info->type == type)
 			return service;
 	}
 
@@ -1793,8 +1791,6 @@ static struct qmi_service* device_get_service(struct qmi_device* device,
 struct service_create_data {
 	struct qmi_device *device;
 	uint8_t type;
-	uint16_t major;
-	uint16_t minor;
 	qmi_create_func_t func;
 	void *user_data;
 	qmi_destroy_func_t destroy;
@@ -1824,7 +1820,9 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	struct qmi_service *service = NULL;
 	const struct qmi_result_code *result_code;
 	const struct qmi_client_id *client_id;
+	struct qmi_version* info;
 	uint16_t len;
+	int i;
 
 	result_code = tlv_get(buffer, length, 0x02, &len);
 	if (!result_code)
@@ -1852,14 +1850,19 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	service->ref_count = 1;
 	service->device = data->device;
 
-	service->type = data->type;
-	service->major = data->major;
-	service->minor = data->minor;
+	for (i = 0, info = device->version_list;
+			i < device->version_count;
+			i++, info++) {
+		if (info->type == data->type) {
+			service->info = info;
+			break;
+		}
+	}
 
 	service->client_id = client_id->client;
 
 	__debug_device(device, "service created [client=%d,type=%d]",
-					service->client_id, service->type);
+					service->client_id, service->info->type);
 
 	/* FIXME: maybe sort these by type...??? */
 	list_add_tail(&service->node, &device->service_list);
@@ -1877,7 +1880,6 @@ static bool service_create(struct qmi_device *device,
 {
 	struct service_create_data *data;
 	unsigned char client_req[] = { 0x01, 0x01, 0x00, type };
-	int i;
 
 	data = g_try_new0(struct service_create_data, 1);
 	if (!data)
@@ -1891,14 +1893,6 @@ static bool service_create(struct qmi_device *device,
 
 	__debug_device(device, "service create [type=%d]", type);
 
-	for (i = 0; i < device->version_count; i++) {
-		if (device->version_list[i].type == data->type) {
-			data->major = device->version_list[i].major;
-			data->minor = device->version_list[i].minor;
-			break;
-		}
-	}
-
 	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
@@ -2028,7 +2022,7 @@ void qmi_service_unref(struct qmi_service *service)
 
 	service->device->release_users++;
 
-	release_client(service->device, service->type, service->client_id,
+	release_client(service->device, service->info->type, service->client_id,
 					service_release_callback, service);
 }
 
@@ -2037,7 +2031,7 @@ const char *qmi_service_get_identifier(struct qmi_service *service)
 	if (!service)
 		return NULL;
 
-	return __service_type_to_string(service->type);
+	return __service_type_to_string(service->info->type);
 }
 
 bool qmi_service_get_version(struct qmi_service *service,
@@ -2047,10 +2041,10 @@ bool qmi_service_get_version(struct qmi_service *service,
 		return false;
 
 	if (major)
-		*major = service->major;
+		*major = service->info->major;
 
 	if (minor)
-		*minor = service->minor;
+		*minor = service->info->minor;
 
 	return true;
 }
@@ -2125,7 +2119,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	r = __request_submit(device, service->type, service->client_id,
+	r = __request_submit(device, service->info->type, service->client_id,
 				message,
 				param ? param->data : NULL,
 				param ? param->length : 0,
-- 
2.15.1

[RFC PATCH 21/30] qmi: make version_list private

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 51 ++++++++++++++++++++++++++++++++++---
 drivers/qmimodem/qmi.h | 14 ++++-------
 plugins/gobi.c         | 68 +++++++++++++++++++-------------------------------
 3 files changed, 77 insertions(+), 56 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 43c54482..76af132b 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -45,6 +45,13 @@
 typedef void (*qmi_message_func_t)(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data);
 
+struct qmi_version {
+	uint8_t type;
+	uint16_t major;
+	uint16_t minor;
+	const char *name;
+};
+
 struct qmi_device {
 	int ref_count;
 	int fd;
@@ -1028,6 +1035,44 @@ static const void *tlv_get(const void *data, uint16_t size,
 	return NULL;
 }
 
+bool qmi_device_get_service_version(struct qmi_device* device, uint8_t type,
+					uint16_t *major, uint16_t *minor)
+{
+	struct qmi_version* info;
+	int i;
+
+	for (i = 0, info = device->version_list;
+			i < device->version_count;
+			i++, info++) {
+		if (info->type == type) {
+			*major = info->major;
+			*minor = info->minor;
+			return true;
+		}
+	}
+
+	return false;
+}
+
+/*
+ *  Check if service exists and return implementation version if so.
+ */
+bool qmi_device_has_service(struct qmi_device* device, uint8_t type)
+{
+	struct qmi_version* info;
+	int i;
+
+	for (i = 0, info = device->version_list;
+			i < device->version_count;
+			i++, info++) {
+		if (info->type == type) {
+			return true;
+		}
+	}
+
+	return false;
+}
+
 struct discover_data {
 	struct qmi_device *device;
 	qmi_discover_func_t func;
@@ -1125,7 +1170,7 @@ done:
 	device->version_count = count;
 
 	if (data->func)
-		data->func(count, list, data->user_data);
+		data->func(data->user_data);
 
 	discover_data_free(data);
 }
@@ -1133,13 +1178,11 @@ done:
 static gboolean discover_reply(gpointer user_data)
 {
 	struct discover_data *data = user_data;
-	struct qmi_device *device = data->device;
 
 	data->timeout = 0;
 
 	if (data->func)
-		data->func(device->version_count,
-				device->version_list, data->user_data);
+		data->func(data->user_data);
 
 	discover_data_free(data);
 
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index 2c0d5aff..9cbf0a72 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -61,13 +61,6 @@ enum qmi_device_expected_data_format {
 	QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP,
 };
 
-struct qmi_version {
-	uint8_t type;
-	uint16_t major;
-	uint16_t minor;
-	const char *name;
-};
-
 void qmi_free(void *ptr);
 
 typedef void (*qmi_destroy_func_t)(void *user_data);
@@ -78,8 +71,7 @@ struct qmi_device;
 typedef void (*qmi_debug_func_t)(const char *str, void *user_data);
 typedef void (*qmi_sync_func_t)(void *user_data);
 typedef void (*qmi_shutdown_func_t)(void *user_data);
-typedef void (*qmi_discover_func_t)(uint8_t count,
-			const struct qmi_version *list, void *user_data);
+typedef void (*qmi_discover_func_t)(void *user_data);
 
 struct qmi_device *qmi_device_new(int fd);
 
@@ -96,6 +88,10 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
 				void *user_data, qmi_destroy_func_t destroy);
 
+bool qmi_device_has_service(struct qmi_device* device, uint8_t type);
+bool qmi_device_get_service_version(struct qmi_device* device, uint8_t type,
+					uint16_t *major, uint16_t *minor);
+
 bool qmi_device_sync(struct qmi_device *device,
 		     qmi_sync_func_t func, void *user_data);
 bool qmi_device_is_sync_supported(struct qmi_device *device);
diff --git a/plugins/gobi.c b/plugins/gobi.c
index dc466a86..85218913 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -264,56 +264,38 @@ static void create_shared_dms(void *user_data)
 				  create_dms_cb, modem, NULL);
 }
 
-static void discover_cb(uint8_t count, const struct qmi_version *list,
-							void *user_data)
+static void discover_cb(void *user_data)
 {
 	struct ofono_modem *modem = user_data;
 	struct gobi_data *data = ofono_modem_get_data(modem);
-	uint8_t i;
+	uint16_t major, minor;
 
 	DBG("");
 
-	for (i = 0; i < count; i++) {
-		DBG("%s %d.%d - %d", list[i].name, list[i].major, list[i].minor,
-				list[i].type);
-
-		switch (list[i].type) {
-		case QMI_SERVICE_DMS:
-			data->features |= GOBI_DMS;
-			break;
-		case QMI_SERVICE_NAS:
-			data->features |= GOBI_NAS;
-			break;
-		case QMI_SERVICE_WMS:
-			data->features |= GOBI_WMS;
-			break;
-		case QMI_SERVICE_WDS:
-			data->features |= GOBI_WDS;
-			break;
-		case QMI_SERVICE_WDA:
-			data->features |= GOBI_WDA;
-			break;
-		case QMI_SERVICE_PDS:
-			data->features |= GOBI_PDS;
-			break;
-		case QMI_SERVICE_PBM:
-			data->features |= GOBI_PBM;
-			break;
-		case QMI_SERVICE_UIM:
-			data->features |= GOBI_UIM;
-			break;
-		case QMI_SERVICE_CAT:
-			data->features |= GOBI_CAT;
-			break;
-		case QMI_SERVICE_CAT_OLD:
-			if (list[i].major > 0)
-				data->features |= GOBI_CAT_OLD;
-			break;
-		case QMI_SERVICE_VOICE:
+	if (qmi_device_has_service(data->device, QMI_SERVICE_DMS))
+		data->features |= GOBI_DMS;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_NAS))
+		data->features |= GOBI_NAS;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_WMS))
+		data->features |= GOBI_WMS;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_WDS))
+		data->features |= GOBI_WDS;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_WDA))
+		data->features |= GOBI_WDA;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_PDS))
+		data->features |= GOBI_PDS;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_PBM))
+		data->features |= GOBI_PBM;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_UIM))
+		data->features |= GOBI_UIM;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_CAT))
+		data->features |= GOBI_CAT;
+	if (qmi_device_get_service_version(data->device,
+				QMI_SERVICE_CAT_OLD, &major, &minor))
+		if (major > 0)
+			data->features |= GOBI_CAT_OLD;
+	if (qmi_device_has_service(data->device, QMI_SERVICE_VOICE))
 			data->features |= GOBI_VOICE;
-			break;
-		}
-	}
 
 	if (!(data->features & GOBI_DMS)) {
 		if (++data->discover_attempts < 3) {
-- 
2.15.1

[RFC PATCH 22/30] qmi: move client_id to qmi_version

[Jonas Bonn]

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

Given that services are always shared now, there will ever only be one
instance of a service of a given type and therefore only one client_id
per service type.  As such, we can move  the client_id in to the service
description proper.

Calling the service description qmi_version is a bit confusing and, as
such, the struct will be renamed later.  For now, the target is to try
to unify the version_list and the service_list as they currently
describe pretty much the same thing; the services from the service list
are already set up to point to a struct in version_list for a subset of
their data.
---
 drivers/qmimodem/qmi.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 76af132b..5178228b 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -50,6 +50,7 @@ struct qmi_version {
 	uint16_t major;
 	uint16_t minor;
 	const char *name;
+	uint8_t client_id;
 };
 
 struct qmi_device {
@@ -86,7 +87,6 @@ struct qmi_service {
 	int ref_count;
 	struct qmi_device *device;
 	struct qmi_version *info;
-	uint8_t client_id;
 	struct list_head notify_list;
 };
 
@@ -1902,10 +1902,10 @@ static void service_create_callback(uint16_t message, uint16_t length,
 		}
 	}
 
-	service->client_id = client_id->client;
+	service->info->client_id = client_id->client;
 
 	__debug_device(device, "service created [client=%d,type=%d]",
-					service->client_id, service->info->type);
+					service->info->client_id, service->info->type);
 
 	/* FIXME: maybe sort these by type...??? */
 	list_add_tail(&service->node, &device->service_list);
@@ -2032,6 +2032,8 @@ static void service_release_callback(uint16_t message, uint16_t length,
 	if (service->device)
 		service->device->release_users--;
 
+	service->info->client_id = 0;
+
 	g_free(service);
 }
 
@@ -2065,7 +2067,8 @@ void qmi_service_unref(struct qmi_service *service)
 
 	service->device->release_users++;
 
-	release_client(service->device, service->info->type, service->client_id,
+	release_client(service->device, service->info->type,
+					service->info->client_id,
 					service_release_callback, service);
 }
 
@@ -2147,7 +2150,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	if (!service)
 		return 0;
 
-	if (!service->client_id)
+	if (!service->info->client_id)
 		return 0;
 
 	device = service->device;
@@ -2162,7 +2165,8 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	r = __request_submit(device, service->info->type, service->client_id,
+	r = __request_submit(device, service->info->type,
+				service->info->client_id,
 				message,
 				param ? param->data : NULL,
 				param ? param->length : 0,
@@ -2184,7 +2188,7 @@ bool qmi_service_cancel_all(struct qmi_service *service)
 	if (!service)
 		return false;
 
-	if (!service->client_id)
+	if (!service->info->client_id)
 		return false;
 
 	device = service->device;
@@ -2192,7 +2196,7 @@ bool qmi_service_cancel_all(struct qmi_service *service)
 		return false;
 
 	list_for_each_entry_safe(req, req_t, &device->req_queue, node) {
-		if (req->client == service->client_id) {
+		if (req->client == service->info->client_id) {
 			list_del(&req->node);
 
 			service_send_free(req->user_data);
@@ -2203,7 +2207,7 @@ bool qmi_service_cancel_all(struct qmi_service *service)
 	}
 
 	list_for_each_entry_safe(req, req_t, &device->service_queue, node) {
-		if (req->client == service->client_id) {
+		if (req->client == service->info->client_id) {
 			list_del(&req->node);
 
 			service_send_free(req->user_data);
-- 
2.15.1

[RFC PATCH 23/30] qmi: use standard endian macros

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 58 +++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 5178228b..b6280c05 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -192,15 +192,15 @@ static int __request_submit(struct qmi_device* device,
 	hdr = req->buf;
 
 	hdr->frame = 0x01;
-	hdr->length = GUINT16_TO_LE(req->len - 1);
+	hdr->length = htole16(req->len - 1);
 	hdr->flags = 0x00;
 	hdr->service = service;
 	hdr->client = client;
 
 	msg = req->buf + QMI_MUX_HDR_SIZE + headroom;
 
-	msg->message = GUINT16_TO_LE(message);
-	msg->length = GUINT16_TO_LE(length);
+	msg->message = htole16(message);
+	msg->length = htole16(length);
 
 	if (data && length > 0)
 		memcpy(req->buf + QMI_MUX_HDR_SIZE + headroom +
@@ -526,13 +526,13 @@ static void __debug_msg(const char dir, const void *buf, size_t len,
 		}
 
 		str += sprintf(str, "%s msg=%d len=%d", type,
-					GUINT16_FROM_LE(msg->message),
-					GUINT16_FROM_LE(msg->length));
+					le16toh(msg->message),
+					le16toh(msg->length));
 
 		str += sprintf(str, " [client=%d,type=%d,tid=%d,len=%d]",
 					hdr->client, ctl->type,
 					ctl->transaction,
-					GUINT16_FROM_LE(hdr->length));
+					le16toh(hdr->length));
 	} else {
 		const struct qmi_service_hdr *srv;
 		const char *type;
@@ -558,13 +558,13 @@ static void __debug_msg(const char dir, const void *buf, size_t len,
 		}
 
 		str += sprintf(str, "%s msg=%d len=%d", type,
-					GUINT16_FROM_LE(msg->message),
-					GUINT16_FROM_LE(msg->length));
+					le16toh(msg->message),
+					le16toh(msg->length));
 
 		str += sprintf(str, " [client=%d,type=%d,tid=%d,len=%d]",
 					hdr->client, srv->type,
-					GUINT16_FROM_LE(srv->transaction),
-					GUINT16_FROM_LE(hdr->length));
+					le16toh(srv->transaction),
+					le16toh(hdr->length));
 	}
 
 	function(strbuf, user_data);
@@ -576,14 +576,14 @@ static void __debug_msg(const char dir, const void *buf, size_t len,
 	str += sprintf(str, "      ");
 	offset = 0;
 
-	while (offset + QMI_TLV_HDR_SIZE < GUINT16_FROM_LE(msg->length)) {
+	while (offset + QMI_TLV_HDR_SIZE < le16toh(msg->length)) {
 		const struct qmi_tlv_hdr *tlv = ptr + offset;
-		uint16_t tlv_length = GUINT16_FROM_LE(tlv->length);
+		uint16_t tlv_length = le16toh(tlv->length);
 
 		if (tlv->type == 0x02 && tlv_length == QMI_RESULT_CODE_SIZE) {
 			const struct qmi_result_code *result = ptr + offset +
 							QMI_TLV_HDR_SIZE;
-			uint16_t error = GUINT16_FROM_LE(result->error);
+			uint16_t error = le16toh(result->error);
 			const char *error_str;
 
 			error_str = __error_to_string(error);
@@ -738,8 +738,8 @@ static void handle_packet(struct qmi_device *device,
 
 		msg = buf + QMI_CONTROL_HDR_SIZE;
 
-		message = GUINT16_FROM_LE(msg->message);
-		length = GUINT16_FROM_LE(msg->length);
+		message = le16toh(msg->message);
+		length = le16toh(msg->length);
 
 		data = buf + QMI_CONTROL_HDR_SIZE + QMI_MESSAGE_HDR_SIZE;
 
@@ -763,12 +763,12 @@ static void handle_packet(struct qmi_device *device,
 
 		msg = buf + QMI_SERVICE_HDR_SIZE;
 
-		message = GUINT16_FROM_LE(msg->message);
-		length = GUINT16_FROM_LE(msg->length);
+		message = le16toh(msg->message);
+		length = le16toh(msg->length);
 
 		data = buf + QMI_SERVICE_HDR_SIZE + QMI_MESSAGE_HDR_SIZE;
 
-		tid = GUINT16_FROM_LE(service->transaction);
+		tid = le16toh(service->transaction);
 
 		if (service->type == 0x04) {
 			handle_indication(device, hdr->service, hdr->client,
@@ -823,7 +823,7 @@ static gboolean received_data(GIOChannel *channel, GIOCondition cond,
 		if (hdr->frame != 0x01 || hdr->flags != 0x80)
 			break;
 
-		len = GUINT16_FROM_LE(hdr->length) + 1;
+		len = le16toh(hdr->length) + 1;
 
 		/* Check that packet size matches frame size */
 		if (bytes_read - offset < len)
@@ -1001,7 +1001,7 @@ void qmi_result_print_tlvs(struct qmi_result *result)
 
 	while (len > QMI_TLV_HDR_SIZE) {
 		const struct qmi_tlv_hdr *tlv = ptr;
-		uint16_t tlv_length = GUINT16_FROM_LE(tlv->length);
+		uint16_t tlv_length = le16toh(tlv->length);
 
 		DBG("tlv: 0x%02x len 0x%04x", tlv->type, tlv->length);
 
@@ -1019,7 +1019,7 @@ static const void *tlv_get(const void *data, uint16_t size,
 
 	while (len > QMI_TLV_HDR_SIZE) {
 		const struct qmi_tlv_hdr *tlv = ptr;
-		uint16_t tlv_length = GUINT16_FROM_LE(tlv->length);
+		uint16_t tlv_length = le16toh(tlv->length);
 
 		if (tlv->type == type) {
 			if (length)
@@ -1132,9 +1132,9 @@ static void discover_callback(uint16_t message, uint16_t length,
 
 	for (i = 0; i < service_list->count; i++) {
 		uint16_t major =
-			GUINT16_FROM_LE(service_list->services[i].major);
+			le16toh(service_list->services[i].major);
 		uint16_t minor =
-			GUINT16_FROM_LE(service_list->services[i].minor);
+			le16toh(service_list->services[i].minor);
 		uint8_t type = service_list->services[i].type;
 		const char *name = __service_type_to_string(type);
 
@@ -1578,7 +1578,7 @@ bool qmi_param_append(struct qmi_param *param, uint8_t type,
 	tlv = ptr + param->length;
 
 	tlv->type = type;
-	tlv->length = GUINT16_TO_LE(length);
+	tlv->length = htole16(length);
 	memcpy(tlv->value, data, length);
 
 	param->data = ptr;
@@ -1769,7 +1769,7 @@ bool qmi_result_get_uint16(struct qmi_result *result, uint8_t type,
 	memcpy(&tmp, ptr, 2);
 
 	if (value)
-		*value = GUINT16_FROM_LE(tmp);
+		*value = le16toh(tmp);
 
 	return true;
 }
@@ -1791,7 +1791,7 @@ bool qmi_result_get_uint32(struct qmi_result *result, uint8_t type,
 	memcpy(&tmp, ptr, 4);
 
 	if (value)
-		*value = GUINT32_FROM_LE(tmp);
+		*value = le32toh(tmp);
 
 	return true;
 }
@@ -1813,7 +1813,7 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
 	memcpy(&tmp, ptr, 8);
 
 	if (value)
-		*value = GUINT64_FROM_LE(tmp);
+		*value = le64toh(tmp);
 
 	return true;
 }
@@ -2128,8 +2128,8 @@ static void service_send_callback(uint16_t message, uint16_t length,
 	if (len != QMI_RESULT_CODE_SIZE)
 		goto done;
 
-	result.result = GUINT16_FROM_LE(result_code->result);
-	result.error = GUINT16_FROM_LE(result_code->error);
+	result.result = le16toh(result_code->result);
+	result.error = le16toh(result_code->error);
 
 done:
 	if (data->func)
-- 
2.15.1

[RFC PATCH 24/30] qmi: convert version_list to struct list head

[Jonas Bonn]

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

...and rename to service_info.

As we intend to merge services and their info, we prepare by making sure
they share a common list structure and rename version_list to
service_info to make it clear that the struct is now more than just
version information.

Previoulsy, the version_list memory was allocated as a single chunk
whereas this patch allocates the service_info structs one-by-one.  This
shouldn't make any difference as this isn't performance critical in any
way.
---
 drivers/qmimodem/qmi.c | 72 ++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 43 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index b6280c05..fb70861a 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -45,7 +45,8 @@
 typedef void (*qmi_message_func_t)(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data);
 
-struct qmi_version {
+struct qmi_service_info {
+	struct list_head node;
 	uint8_t type;
 	uint16_t major;
 	uint16_t minor;
@@ -70,8 +71,7 @@ struct qmi_device {
 	uint16_t control_major;
 	uint16_t control_minor;
 	char *version_str;
-	struct qmi_version *version_list;
-	uint8_t version_count;
+	struct list_head service_info;
 	struct list_head service_list;
 	unsigned int release_users;
 	qmi_shutdown_func_t shutdown_func;
@@ -86,7 +86,7 @@ struct qmi_service {
 	struct list_head node;
 	int ref_count;
 	struct qmi_device *device;
-	struct qmi_version *info;
+	struct qmi_service_info *info;
 	struct list_head notify_list;
 };
 
@@ -891,6 +891,8 @@ struct qmi_device *qmi_device_new(int fd)
 	INIT_LIST_HEAD(&device->control_queue);
 	INIT_LIST_HEAD(&device->service_queue);
 
+	INIT_LIST_HEAD(&device->service_info);
+
 	INIT_LIST_HEAD(&device->service_list);
 
 	device->next_control_tid = 1;
@@ -913,6 +915,7 @@ void qmi_device_unref(struct qmi_device *device)
 {
 	struct qmi_request *req, *req_t;
 	struct qmi_service *service, *service_t;
+	struct qmi_service_info *info, *info_t;
 
 	if (!device)
 		return;
@@ -968,7 +971,11 @@ void qmi_device_unref(struct qmi_device *device)
 	}
 
 	g_free(device->version_str);
-	g_free(device->version_list);
+
+	list_for_each_entry_safe(info, info_t, &device->service_info, node) {
+		list_del(&info->node);
+		free(info);
+	}
 
 	if (device->shutting_down)
 		device->destroyed = true;
@@ -1038,18 +1045,14 @@ static const void *tlv_get(const void *data, uint16_t size,
 bool qmi_device_get_service_version(struct qmi_device* device, uint8_t type,
 					uint16_t *major, uint16_t *minor)
 {
-	struct qmi_version* info;
-	int i;
+	struct qmi_service_info* info;
 
-	for (i = 0, info = device->version_list;
-			i < device->version_count;
-			i++, info++) {
+	list_for_each_entry(info, &device->service_info, node)
 		if (info->type == type) {
 			*major = info->major;
 			*minor = info->minor;
 			return true;
 		}
-	}
 
 	return false;
 }
@@ -1059,16 +1062,11 @@ bool qmi_device_get_service_version(struct qmi_device* device, uint8_t type,
  */
 bool qmi_device_has_service(struct qmi_device* device, uint8_t type)
 {
-	struct qmi_version* info;
-	int i;
+	struct qmi_service_info* info;
 
-	for (i = 0, info = device->version_list;
-			i < device->version_count;
-			i++, info++) {
-		if (info->type == type) {
+	list_for_each_entry(info, &device->service_info, node)
+		if (info->type == type)
 			return true;
-		}
-	}
 
 	return false;
 }
@@ -1105,13 +1103,9 @@ static void discover_callback(uint16_t message, uint16_t length,
 	const struct qmi_service_list *service_list;
 	const void *ptr;
 	uint16_t len;
-	struct qmi_version *list;
-	uint8_t count;
+	struct qmi_service_info *info;
 	unsigned int i;
 
-	count = 0;
-	list = NULL;
-
 	result_code = tlv_get(buffer, length, 0x02, &len);
 	if (!result_code)
 		goto done;
@@ -1126,10 +1120,6 @@ static void discover_callback(uint16_t message, uint16_t length,
 	if (len < QMI_SERVICE_LIST_SIZE)
 		goto done;
 
-	list = g_try_malloc(sizeof(struct qmi_version) * service_list->count);
-	if (!list)
-		goto done;
-
 	for (i = 0; i < service_list->count; i++) {
 		uint16_t major =
 			le16toh(service_list->services[i].major);
@@ -1151,12 +1141,15 @@ static void discover_callback(uint16_t message, uint16_t length,
 			continue;
 		}
 
-		list[count].type = type;
-		list[count].major = major;
-		list[count].minor = minor;
-		list[count].name = name;
+		info = calloc(1, sizeof(*info));
+		INIT_LIST_HEAD(&info->node);
 
-		count++;
+		info->type = type;
+		info->major = major;
+		info->minor = minor;
+		info->name = name;
+
+		list_add_tail(&info->node, &device->service_info);
 	}
 
 	ptr = tlv_get(buffer, length, 0x10, &len);
@@ -1166,9 +1159,6 @@ static void discover_callback(uint16_t message, uint16_t length,
 	device->version_str = strndup(ptr + 1, *((uint8_t *) ptr));
 
 done:
-	device->version_list = list;
-	device->version_count = count;
-
 	if (data->func)
 		data->func(data->user_data);
 
@@ -1208,7 +1198,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	if (device->version_list) {
+	if (!list_empty(&device->service_info)) {
 		data->timeout = g_timeout_add_seconds(0, discover_reply, data);
 		return true;
 	}
@@ -1863,9 +1853,8 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	struct qmi_service *service = NULL;
 	const struct qmi_result_code *result_code;
 	const struct qmi_client_id *client_id;
-	struct qmi_version* info;
+	struct qmi_service_info* info;
 	uint16_t len;
-	int i;
 
 	result_code = tlv_get(buffer, length, 0x02, &len);
 	if (!result_code)
@@ -1893,14 +1882,11 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	service->ref_count = 1;
 	service->device = data->device;
 
-	for (i = 0, info = device->version_list;
-			i < device->version_count;
-			i++, info++) {
+	list_for_each_entry(info, &device->service_info, node)
 		if (info->type == data->type) {
 			service->info = info;
 			break;
 		}
-	}
 
 	service->info->client_id = client_id->client;
 
-- 
2.15.1

[RFC PATCH 25/30] qmi: move service device to service_info

[Jonas Bonn]

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

---
 drivers/qmimodem/qmi.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index fb70861a..00b3ccbe 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -47,6 +47,7 @@ typedef void (*qmi_message_func_t)(uint16_t message, uint16_t length,
 
 struct qmi_service_info {
 	struct list_head node;
+	struct qmi_device *device;
 	uint8_t type;
 	uint16_t major;
 	uint16_t minor;
@@ -85,7 +86,6 @@ struct qmi_device {
 struct qmi_service {
 	struct list_head node;
 	int ref_count;
-	struct qmi_device *device;
 	struct qmi_service_info *info;
 	struct list_head notify_list;
 };
@@ -1880,7 +1880,6 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	INIT_LIST_HEAD(&service->node);
 	INIT_LIST_HEAD(&service->notify_list);
 	service->ref_count = 1;
-	service->device = data->device;
 
 	list_for_each_entry(info, &device->service_info, node)
 		if (info->type == data->type) {
@@ -1888,6 +1887,7 @@ static void service_create_callback(uint16_t message, uint16_t length,
 			break;
 		}
 
+	service->info->device = data->device;
 	service->info->client_id = client_id->client;
 
 	__debug_device(device, "service created [client=%d,type=%d]",
@@ -2015,8 +2015,8 @@ static void service_release_callback(uint16_t message, uint16_t length,
 {
 	struct qmi_service *service = user_data;
 
-	if (service->device)
-		service->device->release_users--;
+	if (service->info->device)
+		service->info->device->release_users--;
 
 	service->info->client_id = 0;
 
@@ -2041,7 +2041,7 @@ void qmi_service_unref(struct qmi_service *service)
 	if (__sync_sub_and_fetch(&service->ref_count, 1))
 		return;
 
-	if (!service->device) {
+	if (!service->info->device) {
 		g_free(service);
 		return;
 	}
@@ -2051,9 +2051,9 @@ void qmi_service_unref(struct qmi_service *service)
 
 	list_del(&service->node);
 
-	service->device->release_users++;
+	service->info->device->release_users++;
 
-	release_client(service->device, service->info->type,
+	release_client(service->info->device, service->info->type,
 					service->info->client_id,
 					service_release_callback, service);
 }
@@ -2139,7 +2139,7 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	if (!service->info->client_id)
 		return 0;
 
-	device = service->device;
+	device = service->info->device;
 	if (!device)
 		return 0;
 
@@ -2177,7 +2177,7 @@ bool qmi_service_cancel_all(struct qmi_service *service)
 	if (!service->info->client_id)
 		return false;
 
-	device = service->device;
+	device = service->info->device;
 	if (!device)
 		return false;
 
-- 
2.15.1

[RFC PATCH 26/30] qmi: make all services unique instances backed by common description

[Jonas Bonn]

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

This patch changes shared services from sharing the service instance
itself to sharing the underlying service description.

This allows each service instance to have callbacks and requests whose
lifetime are tied directly to the lifetime of the service instance
itself.  For example, already with this patch each service has its own
notify_list for callbacks from indications; when the service is
destroyed, the callbacks can be cancelled without affecting callbacks
registered by other users of the service.

This is the meat and potatoes patch of this series.  With this, services
can be safely used across unrelated modules without having to worry
about other users of the same service.  Other cleanups will follow on
this:  pending requests are still not tied to the service descriptor,
for example.
---
 drivers/qmimodem/qmi.c | 323 +++++++++++++++++++++++++++----------------------
 1 file changed, 180 insertions(+), 143 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 00b3ccbe..c87885cd 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -45,6 +45,27 @@
 typedef void (*qmi_message_func_t)(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data);
 
+/*
+ * Service description:  an instance of a service that the device provides.
+ * These are referenced by service descriptors (see struct qmi_service below)
+ * that provide their own private state over top of the shared description.
+ * The description exists in 3 states:
+ * i)  client_id = 0 and services list is empty:  service not 'connected'
+ *     and no client_id has been claimed
+ * ii) client_id =0 and services list has entries:  service is 'connecting',
+ *     attempting to get a client ID for use with requests.  In this state,
+ *     the services list contains struct service_connect_data objects which
+ *     are incomplete service objects for which the service_create callback
+ *     has not yet been invoked.  Once the connection is established (get
+ *     client ID returns), then the callbacks will be invoked  and the
+ *     temporary structs replaced with proper service objects.
+ * iii) client_id != 0:  there are service descriptors in the services list
+ *      and these are ready for use.
+ * When the last service description is removed from the services list, then
+ * the client ID will be released by the description.
+ * The behaviour and terminology are loosely based on file descriptions and
+ * file descriptors as seen in Linux and other UNIX OS.
+ */
 struct qmi_service_info {
 	struct list_head node;
 	struct qmi_device *device;
@@ -53,6 +74,7 @@ struct qmi_service_info {
 	uint16_t minor;
 	const char *name;
 	uint8_t client_id;
+	struct list_head services;
 };
 
 struct qmi_device {
@@ -73,7 +95,6 @@ struct qmi_device {
 	uint16_t control_minor;
 	char *version_str;
 	struct list_head service_info;
-	struct list_head service_list;
 	unsigned int release_users;
 	qmi_shutdown_func_t shutdown_func;
 	void *shutdown_user_data;
@@ -83,9 +104,11 @@ struct qmi_device {
 	bool destroyed : 1;
 };
 
+/* Service descriptor: an instance of a service backed by a shared service
+ * description (see above)
+ */
 struct qmi_service {
 	struct list_head node;
-	int ref_count;
 	struct qmi_service_info *info;
 	struct list_head notify_list;
 };
@@ -702,6 +725,7 @@ static void handle_indication(struct qmi_device *device,
 			uint8_t service_type, uint8_t client_id,
 			uint16_t message, uint16_t length, const void *data)
 {
+	struct qmi_service_info *info;
 	struct qmi_service *service;
 	struct qmi_result result;
 
@@ -714,10 +738,10 @@ static void handle_indication(struct qmi_device *device,
 	result.data = data;
 	result.length = length;
 
-	list_for_each_entry(service, &device->service_list, node) {
-		if (client_id == 0xff || service_type == service->info->type)
-			service_notify(service, &result);
-	}
+	list_for_each_entry(info, &device->service_info, node)
+		if (client_id == 0xff || service_type == info->type)
+			list_for_each_entry(service, &info->services, node)
+				service_notify(service, &result);
 }
 
 static void handle_packet(struct qmi_device *device,
@@ -893,8 +917,6 @@ struct qmi_device *qmi_device_new(int fd)
 
 	INIT_LIST_HEAD(&device->service_info);
 
-	INIT_LIST_HEAD(&device->service_list);
-
 	device->next_control_tid = 1;
 	device->next_service_tid = 256;
 
@@ -911,6 +933,32 @@ struct qmi_device *qmi_device_ref(struct qmi_device *device)
 	return device;
 }
 
+static void service_connect_data_free(gpointer user_data);
+
+struct service_connect_data {
+	struct list_head node;
+	struct qmi_service *service;
+	qmi_create_func_t func;
+	void * user_data;
+	qmi_destroy_func_t destroy;
+	guint timeout;
+};
+
+static void service_connect_data_free(gpointer user_data)
+{
+	struct service_connect_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);
+}
+
 void qmi_device_unref(struct qmi_device *device)
 {
 	struct qmi_request *req, *req_t;
@@ -964,15 +1012,25 @@ void qmi_device_unref(struct qmi_device *device)
 	if (device->shutdown_source)
 		g_source_remove(device->shutdown_source);
 
-	list_for_each_entry_safe(service, service_t,
-					&device->service_list, node) {
-		list_del(&service->node);
-		g_free(service);
-	}
-
 	g_free(device->version_str);
 
 	list_for_each_entry_safe(info, info_t, &device->service_info, node) {
+		if (!info->client_id) {
+			/* Entries in service list are connect_data structs */
+			struct service_connect_data *data, *data_t;
+			list_for_each_entry_safe(data, data_t,
+						&info->services, node) {
+				list_del(&data->node);
+				service_connect_data_free(data);
+			}
+		} else {
+			list_for_each_entry_safe(service, service_t,
+						&info->services, node) {
+				list_del(&service->node);
+				g_free(service);
+			}
+		}
+
 		list_del(&info->node);
 		free(info);
 	}
@@ -1143,7 +1201,9 @@ static void discover_callback(uint16_t message, uint16_t length,
 
 		info = calloc(1, sizeof(*info));
 		INIT_LIST_HEAD(&info->node);
+		INIT_LIST_HEAD(&info->services);
 
+		info->device = device;
 		info->type = type;
 		info->major = major;
 		info->minor = minor;
@@ -1808,53 +1868,30 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
 	return true;
 }
 
-static struct qmi_service* device_get_service(struct qmi_device* device,
-							uint8_t type)
+static struct qmi_service_info* device_get_service_info(
+					struct qmi_device* device,
+					uint8_t type)
 {
-	struct qmi_service* service;
+	struct qmi_service_info* info;
 
-	list_for_each_entry(service, &device->service_list, node) {
-		if (service->info->type == type)
-			return service;
+	list_for_each_entry(info, &device->service_info, node) {
+		if (info->type == type)
+			return info;
 	}
 
 	return NULL;
 }
 
-struct service_create_data {
-	struct qmi_device *device;
-	uint8_t type;
-	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 void service_create_callback(uint16_t message, uint16_t length,
+static void service_connect_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 qmi_service *service = NULL;
+	struct qmi_service_info* info = user_data;
 	const struct qmi_result_code *result_code;
 	const struct qmi_client_id *client_id;
-	struct qmi_service_info* info;
 	uint16_t len;
+	struct service_connect_data *data, *data_t;
+	LIST_HEAD(cdlist);
+	uint8_t cid = 0;
 
 	result_code = tlv_get(buffer, length, 0x02, &len);
 	if (!result_code)
@@ -1867,102 +1904,91 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	if (!client_id)
 		goto done;
 
-	if (len != QMI_CLIENT_ID_SIZE)
-		goto done;
+	cid = client_id->client;
 
-	if (client_id->service != data->type)
+	if (len != QMI_CLIENT_ID_SIZE) {
+		cid = 0;
 		goto done;
+	}
 
-	service = g_try_new0(struct qmi_service, 1);
-	if (!service)
+	if (client_id->service != info->type) {
+		cid = 0;
 		goto done;
+	}
 
-	INIT_LIST_HEAD(&service->node);
-	INIT_LIST_HEAD(&service->notify_list);
-	service->ref_count = 1;
-
-	list_for_each_entry(info, &device->service_info, node)
-		if (info->type == data->type) {
-			service->info = info;
-			break;
-		}
+	info->client_id = cid;
 
-	service->info->device = data->device;
-	service->info->client_id = client_id->client;
+	__debug_device(info->device, "service created [client=%d,type=%d]",
+					info->client_id, info->type);
 
-	__debug_device(device, "service created [client=%d,type=%d]",
-					service->info->client_id, service->info->type);
+done:
+	/* Move the connect_data list over to a temporary location
+	 * so that we can fill up the info->services list properly with
+	 * service objects.
+	 */
+	/* Set up a temporary list to move the services to, clear the
+	 * connect_data list, and then move the services back to the
+	 * info node where they belong now that we are connected
+	 */
+	INIT_LIST_HEAD(&cdlist);
 
-	/* FIXME: maybe sort these by type...??? */
-	list_add_tail(&service->node, &device->service_list);
+	list_for_each_entry_safe(data, data_t, &info->services, node) {
+		list_move(&data->node, &cdlist);
+	}
 
-done:
-	data->func(service, data->user_data);
-	qmi_service_unref(service);
+	list_for_each_entry_safe(data, data_t, &cdlist, node) {
+		if (!cid) {
+			data->func(NULL, data->user_data);
+			g_free(data->service);
+		} else {
+			list_add_tail(&data->service->node, &info->services);
+			data->func(data->service, data->user_data);
+		}
 
-	service_create_data_free(data);
+		list_del(&data->node);
+		service_connect_data_free(data);
+	}
 }
 
-static bool service_create(struct qmi_device *device,
-				uint8_t type, qmi_create_func_t func,
-				void *user_data, qmi_destroy_func_t destroy)
+static bool service_connect(struct service_connect_data *data)
 {
-	struct service_create_data *data;
-	unsigned char client_req[] = { 0x01, 0x01, 0x00, type };
-
-	data = g_try_new0(struct service_create_data, 1);
-	if (!data)
-		return false;
+	struct qmi_service_info* info = data->service->info;
+	unsigned char client_req[] = { 0x01, 0x01, 0x00, info->type };
 
-	data->device = device;
-	data->type = type;
-	data->func = func;
-	data->user_data = user_data;
-	data->destroy = destroy;
+	if (!list_empty(&info->services))
+		/* Already connecting: while connection is in progress,
+		 * the 'info->services' list contains service_connect_data
+		 * structs instead of service structs.  If the list is
+		 * not empty, then connection is already in progess so
+		 * the data is just appended to the list and the callback
+		 * will be invoked when connection is complete.
+		 */
+		goto done;
 
-	__debug_device(device, "service create [type=%d]", type);
+	__debug_device(info->device, "service create [type=%d]", info->type);
 
-	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(info->device, QMI_SERVICE_CONTROL, 0x00,
 			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
-			service_create_callback, data);
+			service_connect_callback, info);
 
+done:
+	list_add_tail(&data->node, &info->services);
 	return true;
 }
 
-struct service_create_shared_data {
-	struct qmi_service *service;
-	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 service_connect_data *data = user_data;
+	struct qmi_service_info *info = data->service->info;
 
 	data->timeout = 0;
+
+	list_add_tail(&data->service->node, &info->services);
+
 	data->func(data->service, data->user_data);
 
-	service_create_shared_data_free(data);
+	service_connect_data_free(data);
 
 	return FALSE;
 }
@@ -1971,8 +1997,9 @@ 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;
+	struct qmi_service_info *info;
 	struct qmi_service *service;
+	struct service_connect_data *data;
 
 	if (!device || !func)
 		return false;
@@ -1980,19 +2007,25 @@ bool qmi_service_create_shared(struct qmi_device *device,
 	if (type == QMI_SERVICE_CONTROL)
 		return false;
 
-	service = device_get_service(device, type);
-	if (!service)
-		return service_create(device, type, func, user_data, destroy);
-
-	data = g_try_new0(struct service_create_shared_data, 1);
-	if (!data)
+	info = device_get_service_info(device, type);
+	if (!info)
 		return false;
 
-	data->service = qmi_service_ref(service);
+	service = calloc(1, sizeof(*service));
+	INIT_LIST_HEAD(&service->node);
+	INIT_LIST_HEAD(&service->notify_list);
+	service->info = info;
+
+	data = calloc(1, sizeof(*data));
+	INIT_LIST_HEAD(&data->node);
+	data->service = service;
 	data->func = func;
 	data->user_data = user_data;
 	data->destroy = destroy;
 
+	if (!info->client_id)
+		return service_connect(data);
+
 	/* FIXME: There's a little window here where we might leak
 	 * data... we'll leave it for now because the correct fix
 	 * is a rework of the way services are shared.
@@ -2013,33 +2046,30 @@ bool qmi_service_create(struct qmi_device *device,
 static void service_release_callback(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data)
 {
-	struct qmi_service *service = user_data;
-
-	if (service->info->device)
-		service->info->device->release_users--;
-
-	service->info->client_id = 0;
+	struct qmi_service_info *info = user_data;
 
-	g_free(service);
+	info->device->release_users--;
 }
 
 struct qmi_service *qmi_service_ref(struct qmi_service *service)
 {
-	if (!service)
-		return NULL;
-
-	__sync_fetch_and_add(&service->ref_count, 1);
-
 	return service;
 }
 
+/*
+ * FIXME: we want to rename this to qmi_service_destroy because the 'create'
+ * function returns an instance and this effectively destroys it... reference
+ * counting is no longer relevant as these service objects are no longer meant
+ * to be shared between unrelated entities.
+ */
 void qmi_service_unref(struct qmi_service *service)
 {
+	struct qmi_service_info *info;
+
 	if (!service)
                 return;
 
-	if (__sync_sub_and_fetch(&service->ref_count, 1))
-		return;
+	info = service->info;
 
 	if (!service->info->device) {
 		g_free(service);
@@ -2050,12 +2080,19 @@ void qmi_service_unref(struct qmi_service *service)
 	qmi_service_unregister_all(service);
 
 	list_del(&service->node);
+	g_free(service);
+
+	if (list_empty(&info->services)) {
+		/* No more service instances that refer to this
+		 * service description so we can release the client ID
+		 */
+		info->device->release_users++;
 
-	service->info->device->release_users++;
+		info->client_id = 0;
 
-	release_client(service->info->device, service->info->type,
-					service->info->client_id,
-					service_release_callback, service);
+		release_client(info->device, info->type, info->client_id,
+					service_release_callback, info);
+	}
 }
 
 const char *qmi_service_get_identifier(struct qmi_service *service)
-- 
2.15.1

[RFC PATCH 27/30] qmi: drop qmi_service_ref function

[Jonas Bonn]

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

Services no longer need to be referenced.  qmi_service_create returns an
instance that is not shared with anyone else.
---
 drivers/qmimodem/devinfo.c              | 2 +-
 drivers/qmimodem/gprs-context.c         | 4 ++--
 drivers/qmimodem/gprs.c                 | 4 ++--
 drivers/qmimodem/location-reporting.c   | 2 +-
 drivers/qmimodem/lte.c                  | 2 +-
 drivers/qmimodem/netmon.c               | 2 +-
 drivers/qmimodem/network-registration.c | 2 +-
 drivers/qmimodem/qmi.c                  | 5 -----
 drivers/qmimodem/qmi.h                  | 1 -
 drivers/qmimodem/radio-settings.c       | 4 ++--
 drivers/qmimodem/sim-legacy.c           | 2 +-
 drivers/qmimodem/sim.c                  | 4 ++--
 drivers/qmimodem/sms.c                  | 2 +-
 drivers/qmimodem/ussd.c                 | 2 +-
 drivers/qmimodem/voicecall.c            | 2 +-
 plugins/gobi.c                          | 2 +-
 16 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/qmimodem/devinfo.c b/drivers/qmimodem/devinfo.c
index f5028657..9aa5dc9d 100644
--- a/drivers/qmimodem/devinfo.c
+++ b/drivers/qmimodem/devinfo.c
@@ -172,7 +172,7 @@ static void create_dms_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->dms = qmi_service_ref(service);
+	data->dms = service;
 
 	ofono_devinfo_register(devinfo);
 }
diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index 9a22b89f..534dc996 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -379,7 +379,7 @@ static void create_wds_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->wds = qmi_service_ref(service);
+	data->wds = service;
 
 	qmi_service_register(data->wds, QMI_WDS_PKT_STATUS_IND,
 					pkt_status_notify, gc, NULL);
@@ -437,7 +437,7 @@ static void create_wda_cb(struct qmi_service *service, void *user_data)
 		goto error;
 	}
 
-	data->wda = qmi_service_ref(service);
+	data->wda = service;
 
 	if (qmi_service_send(data->wda, QMI_WDA_GET_DATA_FORMAT, NULL,
 					get_data_format_cb, gc, NULL) > 0)
diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
index db07f310..a684f5f1 100644
--- a/drivers/qmimodem/gprs.c
+++ b/drivers/qmimodem/gprs.c
@@ -334,7 +334,7 @@ static void create_wds_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->wds = qmi_service_ref(service);
+	data->wds = service;
 
 	/*
 	 * First get the SS info - the modem may already be connected,
@@ -364,7 +364,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->nas = qmi_service_ref(service);
+	data->nas = service;
 
 	qmi_service_create_shared(data->dev, QMI_SERVICE_WDS,
 						create_wds_cb, gprs, NULL);
diff --git a/drivers/qmimodem/location-reporting.c b/drivers/qmimodem/location-reporting.c
index e4ce2331..ed1ec891 100644
--- a/drivers/qmimodem/location-reporting.c
+++ b/drivers/qmimodem/location-reporting.c
@@ -219,7 +219,7 @@ static void create_pds_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->pds = qmi_service_ref(service);
+	data->pds = service;
 
 	qmi_service_register(data->pds, QMI_PDS_EVENT,
 					event_notify, lr, NULL);
diff --git a/drivers/qmimodem/lte.c b/drivers/qmimodem/lte.c
index 841e7751..135d74ad 100644
--- a/drivers/qmimodem/lte.c
+++ b/drivers/qmimodem/lte.c
@@ -190,7 +190,7 @@ static void create_wds_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	ldd->wds = qmi_service_ref(service);
+	ldd->wds = service;
 
 	/* Query the default profile */
 	param = qmi_param_new();
diff --git a/drivers/qmimodem/netmon.c b/drivers/qmimodem/netmon.c
index 6ef5d09c..0165bbc5 100644
--- a/drivers/qmimodem/netmon.c
+++ b/drivers/qmimodem/netmon.c
@@ -230,7 +230,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	nmd->nas = qmi_service_ref(service);
+	nmd->nas = service;
 
 	ofono_netmon_register(netmon);
 }
diff --git a/drivers/qmimodem/network-registration.c b/drivers/qmimodem/network-registration.c
index 6c1f50ba..cab312cd 100644
--- a/drivers/qmimodem/network-registration.c
+++ b/drivers/qmimodem/network-registration.c
@@ -543,7 +543,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->nas = qmi_service_ref(service);
+	data->nas = service;
 
 	param = qmi_param_new();
 	if (!param)
diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index c87885cd..204e189e 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -2051,11 +2051,6 @@ static void service_release_callback(uint16_t message, uint16_t length,
 	info->device->release_users--;
 }
 
-struct qmi_service *qmi_service_ref(struct qmi_service *service)
-{
-	return service;
-}
-
 /*
  * FIXME: we want to rename this to qmi_service_destroy because the 'create'
  * function returns an instance and this effectively destroys it... reference
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index 9cbf0a72..c8cec5c6 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -155,7 +155,6 @@ 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 *qmi_service_ref(struct qmi_service *service);
 void qmi_service_unref(struct qmi_service *service);
 
 const char *qmi_service_get_identifier(struct qmi_service *service);
diff --git a/drivers/qmimodem/radio-settings.c b/drivers/qmimodem/radio-settings.c
index 36ad25cf..0cc95828 100644
--- a/drivers/qmimodem/radio-settings.c
+++ b/drivers/qmimodem/radio-settings.c
@@ -222,7 +222,7 @@ static void create_dms_cb(struct qmi_service *service, void *user_data)
 	if (!service)
 		return;
 
-	data->dms = qmi_service_ref(service);
+	data->dms = service;
 }
 
 static void create_nas_cb(struct qmi_service *service, void *user_data)
@@ -244,7 +244,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->nas = qmi_service_ref(service);
+	data->nas = service;
 
 	ofono_radio_settings_register(rs);
 }
diff --git a/drivers/qmimodem/sim-legacy.c b/drivers/qmimodem/sim-legacy.c
index 318b1aed..55fbadc1 100644
--- a/drivers/qmimodem/sim-legacy.c
+++ b/drivers/qmimodem/sim-legacy.c
@@ -319,7 +319,7 @@ static void create_dms_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->dms = qmi_service_ref(service);
+	data->dms = service;
 
 	qmi_service_register(data->dms, QMI_DMS_EVENT,
 					event_notify, sim, NULL);
diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index 1d9befc9..15d224fd 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -818,7 +818,7 @@ static void create_uim_cb(struct qmi_service *service, void *user_data)
 		goto error;
 	}
 
-	data->uim = qmi_service_ref(service);
+	data->uim = service;
 
 	param = qmi_param_new_uint32(QMI_UIM_PARAM_EVENT_MASK, mask);
 	if (!param)
@@ -845,7 +845,7 @@ static void create_dms_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->dms = qmi_service_ref(service);
+	data->dms = service;
 
 	qmi_service_create(data->qmi_dev, QMI_SERVICE_UIM, create_uim_cb, sim,
 					NULL);
diff --git a/drivers/qmimodem/sms.c b/drivers/qmimodem/sms.c
index 7e6baec5..8893480b 100644
--- a/drivers/qmimodem/sms.c
+++ b/drivers/qmimodem/sms.c
@@ -522,7 +522,7 @@ static void create_wms_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->wms = qmi_service_ref(service);
+	data->wms = service;
 
 	qmi_service_register(data->wms, QMI_WMS_EVENT,
 					event_notify, sms, NULL);
diff --git a/drivers/qmimodem/ussd.c b/drivers/qmimodem/ussd.c
index 174e354d..d62eddd9 100644
--- a/drivers/qmimodem/ussd.c
+++ b/drivers/qmimodem/ussd.c
@@ -140,7 +140,7 @@ static void create_voice_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->voice = qmi_service_ref(service);
+	data->voice = service;
 
 	qmi_service_register(data->voice, QMI_VOICE_ASYNC_ORIG_USSD,
 					async_orig_ind, ussd, NULL);
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 29166b08..d8c63171 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -56,7 +56,7 @@ static void create_voice_cb(struct qmi_service *service, void *user_data)
 		return;
 	}
 
-	data->voice = qmi_service_ref(service);
+	data->voice = service;
 
 	ofono_voicecall_register(vc);
 }
diff --git a/plugins/gobi.c b/plugins/gobi.c
index 85218913..adac5c89 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -245,7 +245,7 @@ static void create_dms_cb(struct qmi_service *service, void *user_data)
 	if (!service)
 		goto error;
 
-	data->dms = qmi_service_ref(service);
+	data->dms = service;
 
 	if (qmi_service_send(data->dms, QMI_DMS_GET_CAPS, NULL,
 					get_caps_cb, modem, NULL) > 0)
-- 
2.15.1

[RFC PATCH 28/30] qmi: rename qmi_service_create/unref to open/close

[Jonas Bonn]

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

Continuing the analogy to file descriptors, this patch renames the
qmi_service_create/unref pair to qmi_service_open/close.  This reflects
the design and usage pattern better.
---
 drivers/qmimodem/devinfo.c              |  4 ++--
 drivers/qmimodem/gprs-context.c         | 10 +++++-----
 drivers/qmimodem/gprs.c                 |  8 ++++----
 drivers/qmimodem/location-reporting.c   |  4 ++--
 drivers/qmimodem/lte.c                  |  4 ++--
 drivers/qmimodem/netmon.c               |  4 ++--
 drivers/qmimodem/network-registration.c |  4 ++--
 drivers/qmimodem/qmi.c                  | 18 ++----------------
 drivers/qmimodem/qmi.h                  |  8 ++------
 drivers/qmimodem/radio-settings.c       |  8 ++++----
 drivers/qmimodem/sim-legacy.c           |  4 ++--
 drivers/qmimodem/sim.c                  |  8 ++++----
 drivers/qmimodem/sms.c                  |  4 ++--
 drivers/qmimodem/ussd.c                 |  4 ++--
 drivers/qmimodem/voicecall.c            |  4 ++--
 plugins/gobi.c                          |  6 +++---
 16 files changed, 42 insertions(+), 60 deletions(-)

diff --git a/drivers/qmimodem/devinfo.c b/drivers/qmimodem/devinfo.c
index 9aa5dc9d..7d08b762 100644
--- a/drivers/qmimodem/devinfo.c
+++ b/drivers/qmimodem/devinfo.c
@@ -189,7 +189,7 @@ static int qmi_devinfo_probe(struct ofono_devinfo *devinfo,
 
 	ofono_devinfo_set_data(devinfo, data);
 
-	qmi_service_create_shared(device, QMI_SERVICE_DMS,
+	qmi_service_open(device, QMI_SERVICE_DMS,
 					create_dms_cb, devinfo, NULL);
 
 	return 0;
@@ -203,7 +203,7 @@ static void qmi_devinfo_remove(struct ofono_devinfo *devinfo)
 
 	ofono_devinfo_set_data(devinfo, NULL);
 
-	qmi_service_unref(data->dms);
+	qmi_service_close(data->dms);
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index 534dc996..12aca163 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -421,7 +421,7 @@ static void get_data_format_cb(struct qmi_result *result, void *user_data)
 	}
 
 done:
-	qmi_service_create_shared(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
+	qmi_service_open(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
 									NULL);
 }
 
@@ -444,7 +444,7 @@ static void create_wda_cb(struct qmi_service *service, void *user_data)
 		return;
 
 error:
-	qmi_service_create_shared(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
+	qmi_service_open(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
 									NULL);
 }
 
@@ -461,7 +461,7 @@ static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
 	ofono_gprs_context_set_data(gc, data);
 	data->dev = device;
 
-	qmi_service_create(device, QMI_SERVICE_WDA, create_wda_cb, gc, NULL);
+	qmi_service_open(device, QMI_SERVICE_WDA, create_wda_cb, gc, NULL);
 
 	return 0;
 }
@@ -476,12 +476,12 @@ static void qmi_gprs_context_remove(struct ofono_gprs_context *gc)
 
 	if (data->wds) {
 		qmi_service_unregister_all(data->wds);
-		qmi_service_unref(data->wds);
+		qmi_service_close(data->wds);
 	}
 
 	if (data->wda) {
 		qmi_service_unregister_all(data->wda);
-		qmi_service_unref(data->wda);
+		qmi_service_close(data->wda);
 	}
 
 	g_free(data);
diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
index a684f5f1..0e4342ca 100644
--- a/drivers/qmimodem/gprs.c
+++ b/drivers/qmimodem/gprs.c
@@ -366,7 +366,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 
 	data->nas = service;
 
-	qmi_service_create_shared(data->dev, QMI_SERVICE_WDS,
+	qmi_service_open(data->dev, QMI_SERVICE_WDS,
 						create_wds_cb, gprs, NULL);
 }
 
@@ -384,7 +384,7 @@ static int qmi_gprs_probe(struct ofono_gprs *gprs,
 
 	data->dev = device;
 
-	qmi_service_create_shared(device, QMI_SERVICE_NAS,
+	qmi_service_open(device, QMI_SERVICE_NAS,
 						create_nas_cb, gprs, NULL);
 
 	return 0;
@@ -399,11 +399,11 @@ static void qmi_gprs_remove(struct ofono_gprs *gprs)
 	ofono_gprs_set_data(gprs, NULL);
 
 	qmi_service_unregister_all(data->wds);
-	qmi_service_unref(data->wds);
+	qmi_service_close(data->wds);
 
 	qmi_service_unregister_all(data->nas);
 
-	qmi_service_unref(data->nas);
+	qmi_service_close(data->nas);
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/location-reporting.c b/drivers/qmimodem/location-reporting.c
index ed1ec891..c6bb48c1 100644
--- a/drivers/qmimodem/location-reporting.c
+++ b/drivers/qmimodem/location-reporting.c
@@ -258,7 +258,7 @@ static int qmi_location_reporting_probe(struct ofono_location_reporting *lr,
 
 	ofono_location_reporting_set_data(lr, data);
 
-	qmi_service_create(device, QMI_SERVICE_PDS, create_pds_cb, lr, NULL);
+	qmi_service_open(device, QMI_SERVICE_PDS, create_pds_cb, lr, NULL);
 
 	return 0;
 }
@@ -273,7 +273,7 @@ static void qmi_location_reporting_remove(struct ofono_location_reporting *lr)
 
 	qmi_service_unregister_all(data->pds);
 
-	qmi_service_unref(data->pds);
+	qmi_service_close(data->pds);
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/lte.c b/drivers/qmimodem/lte.c
index 135d74ad..ebd49cad 100644
--- a/drivers/qmimodem/lte.c
+++ b/drivers/qmimodem/lte.c
@@ -226,7 +226,7 @@ static int qmimodem_lte_probe(struct ofono_lte *lte,
 
 	ofono_lte_set_data(lte, ldd);
 
-	qmi_service_create_shared(device, QMI_SERVICE_WDS,
+	qmi_service_open(device, QMI_SERVICE_WDS,
 					create_wds_cb, lte, NULL);
 
 	return 0;
@@ -242,7 +242,7 @@ static void qmimodem_lte_remove(struct ofono_lte *lte)
 
 	qmi_service_unregister_all(ldd->wds);
 
-	qmi_service_unref(ldd->wds);
+	qmi_service_close(ldd->wds);
 
 	g_free(ldd);
 }
diff --git a/drivers/qmimodem/netmon.c b/drivers/qmimodem/netmon.c
index 0165bbc5..8520b2fa 100644
--- a/drivers/qmimodem/netmon.c
+++ b/drivers/qmimodem/netmon.c
@@ -247,7 +247,7 @@ static int qmi_netmon_probe(struct ofono_netmon *netmon,
 
 	ofono_netmon_set_data(netmon, nmd);
 
-	qmi_service_create_shared(device, QMI_SERVICE_NAS,
+	qmi_service_open(device, QMI_SERVICE_NAS,
 					create_nas_cb, netmon, NULL);
 
 	return 0;
@@ -263,7 +263,7 @@ static void qmi_netmon_remove(struct ofono_netmon *netmon)
 
 	qmi_service_unregister_all(nmd->nas);
 
-	qmi_service_unref(nmd->nas);
+	qmi_service_close(nmd->nas);
 
 	g_free(nmd);
 }
diff --git a/drivers/qmimodem/network-registration.c b/drivers/qmimodem/network-registration.c
index cab312cd..b80e9522 100644
--- a/drivers/qmimodem/network-registration.c
+++ b/drivers/qmimodem/network-registration.c
@@ -583,7 +583,7 @@ static int qmi_netreg_probe(struct ofono_netreg *netreg,
 
 	ofono_netreg_set_data(netreg, data);
 
-	qmi_service_create_shared(device, QMI_SERVICE_NAS,
+	qmi_service_open(device, QMI_SERVICE_NAS,
 					create_nas_cb, netreg, NULL);
 
 	return 0;
@@ -599,7 +599,7 @@ static void qmi_netreg_remove(struct ofono_netreg *netreg)
 
 	qmi_service_unregister_all(data->nas);
 
-	qmi_service_unref(data->nas);
+	qmi_service_close(data->nas);
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 204e189e..8c1696bf 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1993,7 +1993,7 @@ static gboolean service_create_shared_reply(gpointer user_data)
 	return FALSE;
 }
 
-bool qmi_service_create_shared(struct qmi_device *device,
+bool qmi_service_open(struct qmi_device *device,
 				uint8_t type, qmi_create_func_t func,
 				void *user_data, qmi_destroy_func_t destroy)
 {
@@ -2035,14 +2035,6 @@ bool qmi_service_create_shared(struct qmi_device *device,
 	return true;
 }
 
-bool qmi_service_create(struct qmi_device *device,
-				uint8_t type, qmi_create_func_t func,
-				void *user_data, qmi_destroy_func_t destroy)
-{
-	return qmi_service_create_shared(device, type, func,
-						user_data, destroy);
-}
-
 static void service_release_callback(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data)
 {
@@ -2051,13 +2043,7 @@ static void service_release_callback(uint16_t message, uint16_t length,
 	info->device->release_users--;
 }
 
-/*
- * FIXME: we want to rename this to qmi_service_destroy because the 'create'
- * function returns an instance and this effectively destroys it... reference
- * counting is no longer relevant as these service objects are no longer meant
- * to be shared between unrelated entities.
- */
-void qmi_service_unref(struct qmi_service *service)
+void qmi_service_close(struct qmi_service *service)
 {
 	struct qmi_service_info *info;
 
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index c8cec5c6..de47837e 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -148,14 +148,10 @@ typedef void (*qmi_result_func_t)(struct qmi_result *result, void *user_data);
 
 typedef void (*qmi_create_func_t)(struct qmi_service *service, void *user_data);
 
-bool qmi_service_create(struct qmi_device *device,
+bool qmi_service_open(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);
-
-void qmi_service_unref(struct qmi_service *service);
+void qmi_service_close(struct qmi_service *service);
 
 const char *qmi_service_get_identifier(struct qmi_service *service);
 bool qmi_service_get_version(struct qmi_service *service,
diff --git a/drivers/qmimodem/radio-settings.c b/drivers/qmimodem/radio-settings.c
index 0cc95828..87fadfea 100644
--- a/drivers/qmimodem/radio-settings.c
+++ b/drivers/qmimodem/radio-settings.c
@@ -261,9 +261,9 @@ static int qmi_radio_settings_probe(struct ofono_radio_settings *rs,
 
 	ofono_radio_settings_set_data(rs, data);
 
-	qmi_service_create_shared(device, QMI_SERVICE_DMS,
+	qmi_service_open(device, QMI_SERVICE_DMS,
 						create_dms_cb, rs, NULL);
-	qmi_service_create_shared(device, QMI_SERVICE_NAS,
+	qmi_service_open(device, QMI_SERVICE_NAS,
 						create_nas_cb, rs, NULL);
 
 	return 0;
@@ -278,11 +278,11 @@ static void qmi_radio_settings_remove(struct ofono_radio_settings *rs)
 	ofono_radio_settings_set_data(rs, NULL);
 
 	qmi_service_unregister_all(data->dms);
-	qmi_service_unref(data->dms);
+	qmi_service_close(data->dms);
 
 	qmi_service_unregister_all(data->nas);
 
-	qmi_service_unref(data->nas);
+	qmi_service_close(data->nas);
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/sim-legacy.c b/drivers/qmimodem/sim-legacy.c
index 55fbadc1..fbb3d3c0 100644
--- a/drivers/qmimodem/sim-legacy.c
+++ b/drivers/qmimodem/sim-legacy.c
@@ -358,7 +358,7 @@ static int qmi_sim_probe(struct ofono_sim *sim,
 
 	ofono_sim_set_data(sim, data);
 
-	qmi_service_create_shared(device, QMI_SERVICE_DMS,
+	qmi_service_open(device, QMI_SERVICE_DMS,
 						create_dms_cb, sim, NULL);
 
 	return 0;
@@ -374,7 +374,7 @@ static void qmi_sim_remove(struct ofono_sim *sim)
 
 	qmi_service_unregister_all(data->dms);
 
-	qmi_service_unref(data->dms);
+	qmi_service_close(data->dms);
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index 15d224fd..a8b168d7 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -847,7 +847,7 @@ static void create_dms_cb(struct qmi_service *service, void *user_data)
 
 	data->dms = service;
 
-	qmi_service_create(data->qmi_dev, QMI_SERVICE_UIM, create_uim_cb, sim,
+	qmi_service_open(data->qmi_dev, QMI_SERVICE_UIM, create_uim_cb, sim,
 					NULL);
 }
 
@@ -865,7 +865,7 @@ static int qmi_sim_probe(struct ofono_sim *sim,
 
 	ofono_sim_set_data(sim, data);
 
-	qmi_service_create_shared(device, QMI_SERVICE_DMS,
+	qmi_service_open(device, QMI_SERVICE_DMS,
 						create_dms_cb, sim, NULL);
 
 	return 0;
@@ -884,12 +884,12 @@ static void qmi_sim_remove(struct ofono_sim *sim)
 
 	if (data->uim) {
 		qmi_service_unregister_all(data->uim);
-		qmi_service_unref(data->uim);
+		qmi_service_close(data->uim);
 		data->uim = NULL;
 	}
 	if (data->dms) {
 		qmi_service_unregister_all(data->dms);
-		qmi_service_unref(data->dms);
+		qmi_service_close(data->dms);
 	}
 
 	g_free(data);
diff --git a/drivers/qmimodem/sms.c b/drivers/qmimodem/sms.c
index 8893480b..a6d88ca5 100644
--- a/drivers/qmimodem/sms.c
+++ b/drivers/qmimodem/sms.c
@@ -551,7 +551,7 @@ static int qmi_sms_probe(struct ofono_sms *sms,
 
 	ofono_sms_set_data(sms, data);
 
-	qmi_service_create(device, QMI_SERVICE_WMS, create_wms_cb, sms, NULL);
+	qmi_service_open(device, QMI_SERVICE_WMS, create_wms_cb, sms, NULL);
 
 	return 0;
 }
@@ -566,7 +566,7 @@ static void qmi_sms_remove(struct ofono_sms *sms)
 
 	qmi_service_unregister_all(data->wms);
 
-	qmi_service_unref(data->wms);
+	qmi_service_close(data->wms);
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/ussd.c b/drivers/qmimodem/ussd.c
index d62eddd9..df0649ff 100644
--- a/drivers/qmimodem/ussd.c
+++ b/drivers/qmimodem/ussd.c
@@ -160,7 +160,7 @@ static int qmi_ussd_probe(struct ofono_ussd *ussd,
 
 	ofono_ussd_set_data(ussd, data);
 
-	qmi_service_create_shared(device, QMI_SERVICE_VOICE,
+	qmi_service_open(device, QMI_SERVICE_VOICE,
 						create_voice_cb, ussd, NULL);
 
 	return 0;
@@ -174,7 +174,7 @@ static void qmi_ussd_remove(struct ofono_ussd *ussd)
 
 	ofono_ussd_set_data(ussd, NULL);
 
-	qmi_service_unref(data->voice);
+	qmi_service_close(data->voice);
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index d8c63171..db827f25 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -73,7 +73,7 @@ static int qmi_voicecall_probe(struct ofono_voicecall *vc,
 
 	ofono_voicecall_set_data(vc, data);
 
-	qmi_service_create(device, QMI_SERVICE_VOICE,
+	qmi_service_open(device, QMI_SERVICE_VOICE,
 					create_voice_cb, vc, NULL);
 
 	return 0;
@@ -90,7 +90,7 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc)
 
 	qmi_service_unregister_all(data->voice);
 
-	qmi_service_unref(data->voice);
+	qmi_service_close(data->voice);
 
 	g_free(data);
 }
diff --git a/plugins/gobi.c b/plugins/gobi.c
index adac5c89..84ef7432 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -104,7 +104,7 @@ static void gobi_remove(struct ofono_modem *modem)
 
 	ofono_modem_set_data(modem, NULL);
 
-	qmi_service_unref(data->dms);
+	qmi_service_close(data->dms);
 
 	qmi_device_unref(data->device);
 
@@ -132,7 +132,7 @@ static void shutdown_device(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
-	qmi_service_unref(data->dms);
+	qmi_service_close(data->dms);
 	data->dms = NULL;
 
 	qmi_device_shutdown(data->device, shutdown_cb, modem, NULL);
@@ -260,7 +260,7 @@ static void create_shared_dms(void *user_data)
 	struct ofono_modem *modem = user_data;
 	struct gobi_data *data = ofono_modem_get_data(modem);
 
-	qmi_service_create_shared(data->device, QMI_SERVICE_DMS,
+	qmi_service_open(data->device, QMI_SERVICE_DMS,
 				  create_dms_cb, modem, NULL);
 }
 
-- 
2.15.1

[RFC PATCH 29/30] qmi: pass service directly to request_submit

[Jonas Bonn]

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

For CTL requests, pass NULL instead.
---
 drivers/qmimodem/qmi.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 8c1696bf..2286f703 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -187,8 +187,8 @@ void qmi_free(void *ptr)
 static void wakeup_writer(struct qmi_device *device);
 
 static int __request_submit(struct qmi_device* device,
-				uint8_t service,
-				uint8_t client, uint16_t message,
+				struct qmi_service *service,
+				uint16_t message,
 				const void *data,
 				uint16_t length, qmi_message_func_t func,
 				void *user_data)
@@ -197,28 +197,35 @@ static int __request_submit(struct qmi_device* device,
 	struct qmi_mux_hdr *hdr;
 	struct qmi_message_hdr *msg;
 	uint16_t headroom;
+	uint8_t type;
+	uint8_t client_id;
+
+	if (service) {
+		type = service->info->type;
+		client_id = service->info->client_id;
+		headroom = QMI_SERVICE_HDR_SIZE;
+	} else {
+		type = QMI_SERVICE_CONTROL;
+		client_id = 0;
+		headroom = QMI_CONTROL_HDR_SIZE;
+	}
 
 	req = calloc(1, sizeof(*req));
 	INIT_LIST_HEAD(&req->node);
 
-	if (service == QMI_SERVICE_CONTROL)
-		headroom = QMI_CONTROL_HDR_SIZE;
-	else
-		headroom = QMI_SERVICE_HDR_SIZE;
-
 	req->len = QMI_MUX_HDR_SIZE + headroom + QMI_MESSAGE_HDR_SIZE + length;
 
 	req->buf = malloc(req->len);
 
-	req->client = client;
+	req->client = client_id;
 
 	hdr = req->buf;
 
 	hdr->frame = 0x01;
 	hdr->length = htole16(req->len - 1);
 	hdr->flags = 0x00;
-	hdr->service = service;
-	hdr->client = client;
+	hdr->service = type;
+	hdr->client = client_id;
 
 	msg = req->buf + QMI_MUX_HDR_SIZE + headroom;
 
@@ -232,7 +239,8 @@ static int __request_submit(struct qmi_device* device,
 	req->callback = func;
 	req->user_data = user_data;
 
-	if (service == QMI_SERVICE_CONTROL) {
+	if (!service) {
+		/* CTL service */
 		struct qmi_control_hdr* hdr;
 
 		hdr = req->buf + QMI_MUX_HDR_SIZE;
@@ -1263,7 +1271,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 		return true;
 	}
 
-	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(device, NULL,
 			QMI_CTL_GET_VERSION_INFO,
 			NULL, 0, discover_callback, data);
 
@@ -1276,7 +1284,7 @@ static void release_client(struct qmi_device *device,
 {
 	unsigned char release_req[] = { 0x01, 0x02, 0x00, type, client_id };
 
-	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(device, NULL,
 			QMI_CTL_RELEASE_CLIENT_ID,
 			release_req, sizeof(release_req),
 			func, user_data);
@@ -1367,7 +1375,7 @@ bool qmi_device_sync(struct qmi_device *device,
 	func_data->func = func;
 	func_data->user_data = user_data;
 
-	__request_submit(device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(device, NULL,
 			QMI_CTL_SYNC,
 			NULL, 0,
 			qmi_device_sync_callback, func_data);
@@ -1967,7 +1975,7 @@ static bool service_connect(struct service_connect_data *data)
 
 	__debug_device(info->device, "service create [type=%d]", info->type);
 
-	__request_submit(info->device, QMI_SERVICE_CONTROL, 0x00,
+	__request_submit(info->device, NULL,
 			QMI_CTL_GET_CLIENT_ID,
 			client_req, sizeof(client_req),
 			service_connect_callback, info);
@@ -2169,8 +2177,8 @@ uint16_t qmi_service_send(struct qmi_service *service,
 	data->user_data = user_data;
 	data->destroy = destroy;
 
-	r = __request_submit(device, service->info->type,
-				service->info->client_id,
+	r = __request_submit(device,
+				service,
 				message,
 				param ? param->data : NULL,
 				param ? param->length : 0,
-- 
2.15.1

[RFC PATCH 30/30] qmi: add requests to service queue

[Jonas Bonn]

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

Each service description wants to keep its own list of pending requests.
This allows the requests to be safely and efficently cancelled when the
service is 'closed'.
---
 drivers/qmimodem/qmi.c | 41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 2286f703..921f3205 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -110,6 +110,7 @@ struct qmi_device {
 struct qmi_service {
 	struct list_head node;
 	struct qmi_service_info *info;
+	struct list_head requests;
 	struct list_head notify_list;
 };
 
@@ -128,6 +129,7 @@ struct qmi_result {
 
 struct qmi_request {
 	struct list_head node;
+	struct list_head service_node;
 	uint16_t tid;
 	uint8_t client;
 	void *buf;
@@ -212,6 +214,7 @@ static int __request_submit(struct qmi_device* device,
 
 	req = calloc(1, sizeof(*req));
 	INIT_LIST_HEAD(&req->node);
+	INIT_LIST_HEAD(&req->service_node);
 
 	req->len = QMI_MUX_HDR_SIZE + headroom + QMI_MESSAGE_HDR_SIZE + length;
 
@@ -257,6 +260,8 @@ static int __request_submit(struct qmi_device* device,
 		if (device->next_service_tid < 256)
 			device->next_service_tid = 256;
 		req->tid = hdr->transaction;
+
+		list_add_tail(&req->service_node, &service->requests);
 	}
 
 	list_add_tail(&req->node, &device->req_queue);
@@ -813,6 +818,7 @@ static void handle_packet(struct qmi_device *device,
 			return;
 
 		list_del(&req->node);
+		list_del(&req->service_node);
 	}
 
 	if (req->callback)
@@ -995,6 +1001,7 @@ void qmi_device_unref(struct qmi_device *device)
 		 * request with a 'destroy' function
 		 */
 		list_del(&req->node);
+		list_del(&req->service_node);
 		g_free(req->buf);
 		g_free(req);
 	}
@@ -1004,6 +1011,7 @@ void qmi_device_unref(struct qmi_device *device)
 		 * request with a 'destroy' function
 		 */
 		list_del(&req->node);
+		list_del(&req->service_node);
 		g_free(req->buf);
 		g_free(req);
 	}
@@ -2021,6 +2029,7 @@ bool qmi_service_open(struct qmi_device *device,
 
 	service = calloc(1, sizeof(*service));
 	INIT_LIST_HEAD(&service->node);
+	INIT_LIST_HEAD(&service->requests);
 	INIT_LIST_HEAD(&service->notify_list);
 	service->info = info;
 
@@ -2194,39 +2203,19 @@ uint16_t qmi_service_send(struct qmi_service *service,
 
 bool qmi_service_cancel_all(struct qmi_service *service)
 {
-	struct qmi_device *device;
 	struct qmi_request *req, *req_t;
 
 	if (!service)
 		return false;
 
-	if (!service->info->client_id)
-		return false;
-
-	device = service->info->device;
-	if (!device)
-		return false;
-
-	list_for_each_entry_safe(req, req_t, &device->req_queue, node) {
-		if (req->client == service->info->client_id) {
-			list_del(&req->node);
-
-			service_send_free(req->user_data);
-
-			g_free(req->buf);
-			g_free(req);
-		}
-	}
-
-	list_for_each_entry_safe(req, req_t, &device->service_queue, node) {
-		if (req->client == service->info->client_id) {
-			list_del(&req->node);
+	list_for_each_entry_safe(req, req_t, &service->requests, service_node) {
+		list_del(&req->node);
+		list_del(&req->service_node);
 
-			service_send_free(req->user_data);
+		service_send_free(req->user_data);
 
-			g_free(req->buf);
-			g_free(req);
-		}
+		g_free(req->buf);
+		g_free(req);
 	}
 
 	return true;
-- 
2.15.1

Re: [RFC PATCH 00/30] QMI rework

[Denis Kenzior]

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

Hi Jonas,

> 
> So, that said, this is my attempt at cleaning up service sharing in the
> QMI core.  The jist of this is that we switch to a model that looks a
> bit like file descriptions/descriptors in Linux.  The QMI services
> obtain a service description which manages their info and the underlying
> connection (client id) state that the 'shared services' actually share.

So can you save me the trouble of looking through 30 patches and 
describe what the end resulting API is going to look like?

> On top of this service description we provide service descriptors which
> provide private state.  Requests and notifications are registered on the
> service descriptor and their lifetimes are tied to it; when the service
> descriptor is closed (released), the requests and notifications can be
> cleaned up without affecting other users of the shared unlying
> description.
> 

This sounds an awful lot like what GAtChat does.  Except less flexible 
since you can't unregister or cancel an individual notification/request.

Regards,
-Denis

Re: [RFC PATCH 00/30] QMI rework

[Jonas Bonn]

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

On 28/03/18 23:51, Denis Kenzior wrote:
> Hi Jonas,
>
>>
>> So, that said, this is my attempt at cleaning up service sharing in the
>> QMI core.  The jist of this is that we switch to a model that looks a
>> bit like file descriptions/descriptors in Linux.  The QMI services
>> obtain a service description which manages their info and the underlying
>> connection (client id) state that the 'shared services' actually share.
>
> So can you save me the trouble of looking through 30 patches and 
> describe what the end resulting API is going to look like?

Right. So this what I'm aiming for:

QMI Device -> Service1
                  -> ServiceHandle1
                  -> ServiceHandle2
            -> Service2
                  -> ServiceHandle1
                  -> ServiceHandle2
            -> Service3
                  -> ServiceHandle1
                  -> ServiceHandle2
                  -> ServiceHandle3
                  -> ServiceHandle4

ServiceN contains all the shared service data.  This is an underlying 
structure; it is not made available directly to any user.

ServiceHandleN is an service instance that requests can be sent to.  
Notifications and requests are tied to the ServiceHandle they originate 
on so that they can be properly cancelled when the ServiceHandle i

This differs from what ofono has today in that today qmi_service_create 
returns (effecitvely) a reference counted ServiceN instance.  
qmi_service is a ServiceN object.  In order to not have to change the 
API, I want to push the shared details of the service down a level and 
make the user visible qmi_service type be a ServiceHandle.

This is effectively the same as what you proposed with a little twist 
(improvement :) ):
i)  you wanted to add a list of groups to qmi_service so that groups of 
notifations/requests could be cancelled independently of other groups:  
that what ServiceHandle is above, effectively
ii)  the improvement is making ServiceHandle the user-visible type 
instead of ServiceN so that user doesn't need to be aware of this 
grouping or even the fact that the underlying service is shared... 
things just work

The reason the patch series becomes long is that reworking the layering 
in this way is a bit intrusive.  It's worth it though, as the end 
results is _no API change_.  qmi_service_ref becomes a no-op because the 
ServiceHandles are not shareable; qmi_service_unref becomes effectively 
free(ServiceHandle).

The above is sound.  Now we take things further, just for fun, and put 
new windows on the house.  Since the above looks like "file 
descriptions" and "file descriptors" from UNIX, we _could_ use similar 
terminology.  Just for fun, I proposed a rename:

qmi_service_create -> qmi_service_open
qmi_service_unref -> qmi_service_close
...and we drop qmi_service_ref altogether.

Even better would probably be qmi_device_open(device, SERVICE_NAME) 
because the services (files) live on the device and that's the 'object' 
we are requesting a service be opend on... but whatever.  The naming 
isn't important.


>
>> On top of this service description we provide service descriptors which
>> provide private state.  Requests and notifications are registered on the
>> service descriptor and their lifetimes are tied to it; when the service
>> descriptor is closed (released), the requests and notifications can be
>> cleaned up without affecting other users of the shared unlying
>> description.
>>
>
> This sounds an awful lot like what GAtChat does.  Except less flexible 
> since you can't unregister or cancel an individual notification/request.

It doesn't say that anywhere above  Just because I  removed those 
_unused_ functions in a patch doesn't mean they are impossible to 
provide.  If there was _one_ user of such a function then having them 
makes sense, but since they are totally unused they are just untested code.

/Jonas

>
> Regards,
> -Denis

Re: [RFC PATCH 00/30] QMI rework

[Denis Kenzior]

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

Hi Jonas,

On 03/29/2018 05:31 AM, Jonas Bonn wrote:
> On 28/03/18 23:51, Denis Kenzior wrote:
>> Hi Jonas,
>>
>>>
>>> So, that said, this is my attempt at cleaning up service sharing in the
>>> QMI core.  The jist of this is that we switch to a model that looks a
>>> bit like file descriptions/descriptors in Linux.  The QMI services
>>> obtain a service description which manages their info and the underlying
>>> connection (client id) state that the 'shared services' actually share.
>>
>> So can you save me the trouble of looking through 30 patches and 
>> describe what the end resulting API is going to look like?
> 
> Right. So this what I'm aiming for:
> 
> QMI Device -> Service1
>                   -> ServiceHandle1
>                   -> ServiceHandle2
>             -> Service2
>                   -> ServiceHandle1
>                   -> ServiceHandle2
>             -> Service3
>                   -> ServiceHandle1
>                   -> ServiceHandle2
>                   -> ServiceHandle3
>                   -> ServiceHandle4
> 
> ServiceN contains all the shared service data.  This is an underlying 
> structure; it is not made available directly to any user.
> 
> ServiceHandleN is an service instance that requests can be sent to. 
> Notifications and requests are tied to the ServiceHandle they originate 
> on so that they can be properly cancelled when the ServiceHandle i
> 
> This differs from what ofono has today in that today qmi_service_create 
> returns (effecitvely) a reference counted ServiceN instance. qmi_service 
> is a ServiceN object.  In order to not have to change the API, I want to 
> push the shared details of the service down a level and make the user 
> visible qmi_service type be a ServiceHandle.

Okay, so essentially GAtChat (ServiceHandle) and at_chat (Service).

> 
> This is effectively the same as what you proposed with a little twist 
> (improvement :) ):
> i)  you wanted to add a list of groups to qmi_service so that groups of 
> notifations/requests could be cancelled independently of other groups: 
> that what ServiceHandle is above, effectively
Note that MBIM doesn't really have a concept of a service that is 
'opened'.  It is just a UUID, so having everything sit on the device 
object and using groups was more convenient.

> ii)  the improvement is making ServiceHandle the user-visible type 
> instead of ServiceN so that user doesn't need to be aware of this 
> grouping or even the fact that the underlying service is shared... 
> things just work

So same as GAtChat, which is essentially a group id and a pointer to the 
underlying object.

> 
> The reason the patch series becomes long is that reworking the layering 
> in this way is a bit intrusive.  It's worth it though, as the end 
> results is _no API change_.  qmi_service_ref becomes a no-op because the 
> ServiceHandles are not shareable; qmi_service_unref becomes effectively 
> free(ServiceHandle).
> 
> The above is sound.  Now we take things further, just for fun, and put 
> new windows on the house.  Since the above looks like "file 
> descriptions" and "file descriptors" from UNIX, we _could_ use similar 
> terminology.  Just for fun, I proposed a rename:

These are not file descriptors.  You can't cancel an individual send 
request on file descriptors ;)  But anyway, the naming doesn't matter.

> 
> qmi_service_create -> qmi_service_open
> qmi_service_unref -> qmi_service_close
> ...and we drop qmi_service_ref altogether.

We're moving away from reference counting for the most part.  It is a 
glib-ness that we never adopted whole-heartedly and is generally 
pointless in the way oFono is setup.

> 
> Even better would probably be qmi_device_open(device, SERVICE_NAME) 
> because the services (files) live on the device and that's the 'object' 
> we are requesting a service be opend on... but whatever.  The naming 
> isn't important.

I'd just go in the direction of:

uint32_t qmi_device_service_get/open(device, service, callback)
and
qmi_service_free()

Note that the above should be cancellable, otherwise you can crash if 
the atom driver is removed prior to the callback being called.  This is 
a major issue with the existing implementation actually.

> 
> 
>>
>>> On top of this service description we provide service descriptors which
>>> provide private state.  Requests and notifications are registered on the
>>> service descriptor and their lifetimes are tied to it; when the service
>>> descriptor is closed (released), the requests and notifications can be
>>> cleaned up without affecting other users of the shared unlying
>>> description.
>>>
>>
>> This sounds an awful lot like what GAtChat does.  Except less flexible 
>> since you can't unregister or cancel an individual notification/request.
> 
> It doesn't say that anywhere above  Just because I  removed those 
> _unused_ functions in a patch doesn't mean they are impossible to 
> provide.  If there was _one_ user of such a function then having them 
> makes sense, but since they are totally unused they are just untested code.
> 

We should still have it, there are countless examples where this 
capability made things much easier to implement certain behavior.  The 
QMI driver might not use this capability now, but you'll be glad you 
have it when you need it.

Regards,
-Denis

Re: [RFC PATCH 00/30] QMI rework

[Jonas Bonn]

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



On 29/03/18 17:55, Denis Kenzior wrote:
>
>> This is effectively the same as what you proposed with a little twist 
>> (improvement :) ):
>> i)  you wanted to add a list of groups to qmi_service so that groups 
>> of notifations/requests could be cancelled independently of other 
>> groups: that what ServiceHandle is above, effectively
> Note that MBIM doesn't really have a concept of a service that is 
> 'opened'.  It is just a UUID, so having everything sit on the device 
> object and using groups was more convenient.

I don't know enough about MBIM... the UUID could just live in the 
"shared" object and the service reference like it I've described above, 
right?
>>
>> qmi_service_create -> qmi_service_open
>> qmi_service_unref -> qmi_service_close
>> ...and we drop qmi_service_ref altogether.
>
> We're moving away from reference counting for the most part.  It is a 
> glib-ness that we never adopted whole-heartedly and is generally 
> pointless in the way oFono is setup.

Good to know.

>
>>
>> Even better would probably be qmi_device_open(device, SERVICE_NAME) 
>> because the services (files) live on the device and that's the 
>> 'object' we are requesting a service be opend on... but whatever.  
>> The naming isn't important.
>
> I'd just go in the direction of:
>
> uint32_t qmi_device_service_get/open(device, service, callback)
> and
> qmi_service_free()
>

OK.



> Note that the above should be cancellable, otherwise you can crash if 
> the atom driver is removed prior to the callback being called. This is 
> a major issue with the existing implementation actually.
>
Yes, I saw this, too.

There doesn't seem to be timeouts set on QMI requests either.  That 
should be part of the core protocol handling.  The whole "struct 
discovery" bit of the code seems to be a way of providing timeouts for 
some initial interaction with the device... after that, things are 
expected to just work.

>>
>> It doesn't say that anywhere above  Just because I  removed those 
>> _unused_ functions in a patch doesn't mean they are impossible to 
>> provide.  If there was _one_ user of such a function then having them 
>> makes sense, but since they are totally unused they are just untested 
>> code.
>>
>
> We should still have it, there are countless examples where this 
> capability made things much easier to implement certain behavior. The 
> QMI driver might not use this capability now, but you'll be glad you 
> have it when you need it.

Yes, agreed.  I  already added them back in my code... even though they 
don't have users.

/Jonas
>
> Regards,
> -Denis

Re: [RFC PATCH 00/30] QMI rework

[Denis Kenzior]

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

Hi Jonas,

>>> This is effectively the same as what you proposed with a little twist 
>>> (improvement :) ):
>>> i)  you wanted to add a list of groups to qmi_service so that groups 
>>> of notifations/requests could be cancelled independently of other 
>>> groups: that what ServiceHandle is above, effectively
>> Note that MBIM doesn't really have a concept of a service that is 
>> 'opened'.  It is just a UUID, so having everything sit on the device 
>> object and using groups was more convenient.
> 
> I don't know enough about MBIM... the UUID could just live in the 
> "shared" object and the service reference like it I've described above, 
> right?

You could, but it didn't seem worthwhile.  Conceptually it looks quite 
similar to D-Bus where the service UUID is somewhat equivalent to 
interface name.  QMI is a bit more involved with client ids, major/minor 
versions, etc.

Regards,
-Denis