[PATCH qmi lte v3 0/6] QMI LTE series

[PATCH qmi lte v3 0/6] QMI LTE series

[Jonas Bonn]

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

Changes since v2:
* Revert callback data setup changes; the original model was fine so
  this amounted mostly to churn and, unfortunately, ended up introducing
  memory leaks
* Use ofono_gprs_context_deactivated in the detach_shutdown path instead
  of modifying release_active_contexts()
* Add support for roaming registration state

Original cover letter follows:

LTE is different from other 'GPRS' technologies in that a default bearer
always gets set up when the modem registers to the network.  Ofono has
some support for this and this series tries to set this up for QMI
modems.

The key to understanding this series is to understand that QMI modems
do not automatically enable the network interface when connecting to
an LTE network, even though the default bearer is already set up.  A
call to "start network" needs to be made in order to allow packets to
flow.  So the jist of this series is:

i) detect network registration to LTE network
ii) call ofono_gprs_cid_activated
iii) in read_settings, start by calling "start network"

...and then there are some other adjustments in the series to make
this all work.

I've tested this with my EC21, both on LTE and UMTS networks.  It
all seems to work, including moving out of network connectivity and
bringing the modem repeatedly online/offline.

One open question at this point:

* What happens if the connection manager calls deactivate_context
on the default bearer context?  I don't see that any other drivers
guard against this, and the QMI driver will happily stop the network
interface even though the default bearer is still up in the background.

Jonas Bonn (6):
  qmi: free cb_data on error
  qmi: implement detach_shutdown method
  qmi: activate default bearer context for LTE networks
  qim: use named status value
  qmi: stop listening to packet service notifications
  qmi: consolidate ss_info handling functions

 drivers/qmimodem/gprs-context.c         |  58 ++++++--------
 drivers/qmimodem/gprs.c                 | 135 ++++++++++++++++++++++++--------
 drivers/qmimodem/network-registration.c |  53 ++++++-------
 3 files changed, 146 insertions(+), 100 deletions(-)

-- 
2.9.3

[PATCH qmi lte v3 0/7] QMI LTE series

[Jonas Bonn]

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

Changes since v2:
* Revert callback data setup changes; the original model was fine so
  this amounted mostly to churn and, unfortunately, ended up introducing
  memory leaks
* Use ofono_gprs_context_deactivated in the detach_shutdown path instead
  of modifying release_active_contexts()
* Add support for roaming registration state

Original cover letter follows:

LTE is different from other 'GPRS' technologies in that a default bearer
always gets set up when the modem registers to the network.  Ofono has
some support for this and this series tries to set this up for QMI
modems.

The key to understanding this series is to understand that QMI modems
do not automatically enable the network interface when connecting to
an LTE network, even though the default bearer is already set up.  A
call to "start network" needs to be made in order to allow packets to
flow.  So the jist of this series is:

i) detect network registration to LTE network
ii) call ofono_gprs_cid_activated
iii) in read_settings, start by calling "start network"

...and then there are some other adjustments in the series to make
this all work.

I've tested this with my EC21, both on LTE and UMTS networks.  It
all seems to work, including moving out of network connectivity and
bringing the modem repeatedly online/offline.

One open question at this point:

* What happens if the connection manager calls deactivate_context
on the default bearer context?  I don't see that any other drivers
guard against this, and the QMI driver will happily stop the network
interface even though the default bearer is still up in the background.

Jonas Bonn (7):
  qmi: free cb_data on error
  qmi: implement detach_shutdown method
  qmi: activate default bearer context for LTE networks
  qim: use named status value
  qmi: use destroy callback for activate_primary
  qmi: stop listening to packet service notifications
  qmi: consolidate ss_info handling functions

 drivers/qmimodem/gprs-context.c         |  69 +++++++---------
 drivers/qmimodem/gprs.c                 | 135 ++++++++++++++++++++++++--------
 drivers/qmimodem/network-registration.c |  53 ++++++-------
 3 files changed, 151 insertions(+), 106 deletions(-)

-- 
2.9.3

[PATCH qmi lte v3 1/6] qmi: free cb_data on error

[Jonas Bonn]

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

...and move allocation of structure up to variable declarations to
match the pattern used elsewhere in the code.
---
 drivers/qmimodem/gprs-context.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index c9caf64..3743bb2 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -207,13 +207,12 @@ static void qmi_gprs_read_settings(struct ofono_gprs_context* gc,
 					void *user_data)
 {
 	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
-	struct cb_data *cbd;
+	struct cb_data *cbd = cb_data_new(cb, user_data);
 
 	DBG("cid %u", cid);
 
 	data->active_context = cid;
 
-	cbd  = cb_data_new(cb, user_data);
 	cbd->user = gc;
 
 	if (qmi_service_send(data->wds, QMI_WDS_START_NET, NULL,
@@ -223,6 +222,8 @@ static void qmi_gprs_read_settings(struct ofono_gprs_context* gc,
 	data->active_context = 0;
 
 	CALLBACK_WITH_FAILURE(cb, cbd->data);
+
+	g_free(cbd);
 }
 
 static void qmi_activate_primary(struct ofono_gprs_context *gc,
-- 
2.9.3

Re: [PATCH qmi lte v3 1/6] qmi: free cb_data on error

[Denis Kenzior]

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

Hi Jonas,

On 04/15/2017 10:34 AM, Jonas Bonn wrote:
> ...and move allocation of structure up to variable declarations to
> match the pattern used elsewhere in the code.
> ---
>  drivers/qmimodem/gprs-context.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH qmi lte v3 2/6] qmi: implement detach_shutdown method

[Jonas Bonn]

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

The detach_shutdown method is invoked to unconditionally release
an active context.  For QMI, this is equivalent to a call to
deactivate_primary.

This patch makes the callback to deactivate_primary optional and
implements detach_shutdown to simply call it.  When there is no
callback, the stop_net callback notifies ofono about the context
release via an asynchronous ofono_gprs_context_deactivated() call.
---
 drivers/qmimodem/gprs-context.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index 3743bb2..2085898 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -309,14 +309,19 @@ static void stop_net_cb(struct qmi_result *result, void *user_data)
 	DBG("");
 
 	if (qmi_result_set_error(result, NULL)) {
-		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		if (cb)
+			CALLBACK_WITH_FAILURE(cb, cbd->data);
 		return;
 	}
 
-	data->active_context = 0;
 	data->pkt_handle = 0;
 
-	CALLBACK_WITH_SUCCESS(cb, cbd->data);
+	if (cb)
+		CALLBACK_WITH_SUCCESS(cb, cbd->data);
+	else
+		ofono_gprs_context_deactivated(gc, data->active_context); 
+
+	data->active_context = 0;
 }
 
 static void qmi_deactivate_primary(struct ofono_gprs_context *gc,
@@ -343,11 +348,20 @@ static void qmi_deactivate_primary(struct ofono_gprs_context *gc,
 	qmi_param_free(param);
 
 error:
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	if (cb)
+		CALLBACK_WITH_FAILURE(cb, user_data);
 
 	g_free(cbd);
 }
 
+static void qmi_gprs_context_detach_shutdown(struct ofono_gprs_context *gc,
+						unsigned int cid)
+{
+	DBG("");
+
+	qmi_deactivate_primary(gc, cid, NULL, NULL);
+}
+
 static void create_wds_cb(struct qmi_service *service, void *user_data)
 {
 	struct ofono_gprs_context *gc = user_data;
@@ -476,6 +490,7 @@ static struct ofono_gprs_context_driver driver = {
 	.activate_primary	= qmi_activate_primary,
 	.deactivate_primary	= qmi_deactivate_primary,
 	.read_settings		= qmi_gprs_read_settings,
+	.detach_shutdown	= qmi_gprs_context_detach_shutdown,
 };
 
 void qmi_gprs_context_init(void)
-- 
2.9.3

Re: [PATCH qmi lte v3 2/6] qmi: implement detach_shutdown method

[Denis Kenzior]

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

Hi Jonas,

On 04/15/2017 10:34 AM, Jonas Bonn wrote:
> The detach_shutdown method is invoked to unconditionally release
> an active context.  For QMI, this is equivalent to a call to
> deactivate_primary.
>
> This patch makes the callback to deactivate_primary optional and
> implements detach_shutdown to simply call it.  When there is no
> callback, the stop_net callback notifies ofono about the context
> release via an asynchronous ofono_gprs_context_deactivated() call.
> ---
>  drivers/qmimodem/gprs-context.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH qmi lte v3 3/6] qmi: activate default bearer context for LTE networks

[Jonas Bonn]

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

When the modem attaches to an LTE network, a default bearer is
automatically negotiated using the "defalt profile" settings.  The
QMI modem, however, does not given any explicit indication that
the bearer exists; instead, we must assume its existence based on
the network registration state.

This patch extends the GPRS atom to signal the presence of a
default bearer when it detects network connectivity on an LTE
network.

The GPRS atom gets a new 'attached' property based on whether it has
been manually attached or automatically "attached" by virtue of
connecting to an LTE network.  Here we abuse the word "attach"
since attach'ing isn't really an LTE concept, but it fits with
the model that Ofono wants.
---
 drivers/qmimodem/gprs.c | 135 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 101 insertions(+), 34 deletions(-)

diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
index 05ad4bd..3ac05ba 100644
--- a/drivers/qmimodem/gprs.c
+++ b/drivers/qmimodem/gprs.c
@@ -23,6 +23,8 @@
 #include <config.h>
 #endif
 
+#include <glib.h>	/* this include belongs in common.h */
+
 #include <ofono/log.h>
 #include <ofono/modem.h>
 #include <ofono/gprs.h>
@@ -30,16 +32,34 @@
 #include "qmi.h"
 #include "nas.h"
 
+#include "src/common.h"
 #include "qmimodem.h"
 
 struct gprs_data {
 	struct qmi_service *nas;
+	int attached;
 };
 
-static bool extract_ss_info(struct qmi_result *result, int *status)
+static int rat_to_tech(uint8_t rat)
+{
+	switch (rat) {
+	case QMI_NAS_NETWORK_RAT_GSM:
+		return ACCESS_TECHNOLOGY_GSM;
+	case QMI_NAS_NETWORK_RAT_UMTS:
+		return ACCESS_TECHNOLOGY_UTRAN;
+	case QMI_NAS_NETWORK_RAT_LTE:
+		return ACCESS_TECHNOLOGY_EUTRAN;
+	}
+
+	return -1;
+}
+
+static bool extract_ss_info(struct qmi_result *result, int *status, int *tech)
 {
 	const struct qmi_nas_serving_system *ss;
+	uint8_t roaming;
 	uint16_t len;
+	int i;
 
 	DBG("");
 
@@ -47,25 +67,80 @@ static bool extract_ss_info(struct qmi_result *result, int *status)
 	if (!ss)
 		return false;
 
-	if (ss->ps_state == QMI_NAS_ATTACH_STATE_ATTACHED)
-		*status = 0x01;
-	else
-		*status = 0x00;
+	*status = ss->status;
+
+	*tech = -1;
+	for (i = 0; i < ss->radio_if_count; i++) {
+		DBG("radio in use %d", ss->radio_if[i]);
+
+		*tech = rat_to_tech(ss->radio_if[i]);
+	}
+
+	if (qmi_result_get_uint8(result, QMI_NAS_RESULT_ROAMING_STATUS,
+								&roaming)) {
+		if (ss->status == 1 && roaming == 0)
+			*status = NETWORK_REGISTRATION_STATUS_ROAMING;
+	}
 
 	return true;
 }
 
-static void ss_info_notify(struct qmi_result *result, void *user_data)
+/*
+ * This method handles both Serving System Info requests and
+ * notifications.  The presence of a callback differentiates between
+ * the two and the result is handled slightly differently accordingly.
+ * If there's a callback, we invoke it; otherwise we send a notification
+ * to ofono about relevant state changes.
+ */
+static void handle_ss_info(struct qmi_result* result, void *user_data)
 {
-	struct ofono_gprs *gprs = user_data;
+	struct cb_data *cbd = user_data;
+	struct ofono_gprs* gprs = cbd->user;
+	struct gprs_data *data = ofono_gprs_get_data(gprs);
+	ofono_gprs_status_cb_t cb = cbd->cb;
 	int status;
+	int tech;
 
 	DBG("");
 
-	if (!extract_ss_info(result, &status))
-		return;
+	if (cb && qmi_result_set_error(result, NULL))
+			goto error;
+
+	if (!extract_ss_info(result, &status, &tech))
+		goto error;
+
+	switch (status) {
+	case NETWORK_REGISTRATION_STATUS_REGISTERED:
+	case NETWORK_REGISTRATION_STATUS_ROAMING:
+		if (tech == ACCESS_TECHNOLOGY_EUTRAN && !data->attached) {
+			/* On LTE we are effectively always attached; and
+			 * the default bearer is established as soon as the
+			 * network is joined.
+			 */
+			data->attached = 1;
+			/* FIXME: query default profile number and APN
+			 * instead of assuming profile 1
+			 */
+			ofono_gprs_cid_activated(gprs, 1, "");
+		}
+		break;
+	default:
+		break;
+	}
+
+	if (!data->attached)
+		status = NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
 
-	ofono_gprs_status_notify(gprs, status);
+	if (cb)
+		CALLBACK_WITH_SUCCESS(cb, status, cbd->data);
+	else
+		ofono_gprs_status_notify(gprs, status);
+
+	return;
+
+error:
+	if (cb)
+		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
 }
 
 static void attach_detach_cb(struct qmi_result *result, void *user_data)
@@ -100,6 +175,8 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
 
 	DBG("attached %d", attached);
 
+	data->attached = attached;
+
 	if (attached)
 		action = QMI_NAS_ATTACH_ACTION_ATTACH;
 	else
@@ -121,27 +198,6 @@ error:
 	g_free(cbd);
 }
 
-static void get_ss_info_cb(struct qmi_result *result, void *user_data)
-{
-	struct cb_data *cbd = user_data;
-	ofono_gprs_status_cb_t cb = cbd->cb;
-	int status;
-
-	DBG("");
-
-	if (qmi_result_set_error(result, NULL)) {
-		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-		return;
-	}
-
-	if (!extract_ss_info(result, &status)) {
-		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-		return;
-	}
-
-	CALLBACK_WITH_SUCCESS(cb, status, cbd->data);
-}
-
 static void qmi_attached_status(struct ofono_gprs *gprs,
 				ofono_gprs_status_cb_t cb, void *user_data)
 {
@@ -150,8 +206,14 @@ static void qmi_attached_status(struct ofono_gprs *gprs,
 
 	DBG("");
 
+	if (!data->attached) {
+		CALLBACK_WITH_SUCCESS(cb,
+			NETWORK_REGISTRATION_STATUS_NOT_REGISTERED, user_data);
+	}
+
+	cbd->user = gprs;
 	if (qmi_service_send(data->nas, QMI_NAS_GET_SS_INFO, NULL,
-					get_ss_info_cb, cbd, g_free) > 0)
+					handle_ss_info, cbd, g_free) > 0)
 		return;
 
 	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
@@ -163,6 +225,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 {
 	struct ofono_gprs *gprs = user_data;
 	struct gprs_data *data = ofono_gprs_get_data(gprs);
+	struct cb_data *cbd;
 
 	DBG("");
 
@@ -178,11 +241,15 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 	 * First get the SS info - the modem may already be connected,
 	 * and the state-change notification may never arrive
 	 */
+	cbd = cb_data_new(NULL, NULL);
+	cbd->user = gprs;
 	qmi_service_send(data->nas, QMI_NAS_GET_SS_INFO, NULL,
-					ss_info_notify, gprs, NULL);
+					handle_ss_info, cbd, g_free);
 
+	cbd = cb_data_new(NULL, NULL);
+	cbd->user = gprs;
 	qmi_service_register(data->nas, QMI_NAS_SS_INFO_IND,
-					ss_info_notify, gprs, NULL);
+					handle_ss_info, cbd, g_free);
 
 	ofono_gprs_set_cid_range(gprs, 1, 1);
 
-- 
2.9.3

Re: [PATCH qmi lte v3 3/6] qmi: activate default bearer context for LTE networks

[Denis Kenzior]

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

Hi Jonas,

On 04/15/2017 10:34 AM, Jonas Bonn wrote:
> When the modem attaches to an LTE network, a default bearer is
> automatically negotiated using the "defalt profile" settings.  The
> QMI modem, however, does not given any explicit indication that
> the bearer exists; instead, we must assume its existence based on
> the network registration state.
>
> This patch extends the GPRS atom to signal the presence of a
> default bearer when it detects network connectivity on an LTE
> network.
>
> The GPRS atom gets a new 'attached' property based on whether it has
> been manually attached or automatically "attached" by virtue of
> connecting to an LTE network.  Here we abuse the word "attach"
> since attach'ing isn't really an LTE concept, but it fits with
> the model that Ofono wants.
> ---
>  drivers/qmimodem/gprs.c | 135 ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 101 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
> index 05ad4bd..3ac05ba 100644
> --- a/drivers/qmimodem/gprs.c
> +++ b/drivers/qmimodem/gprs.c
> @@ -23,6 +23,8 @@
>  #include <config.h>
>  #endif
>
> +#include <glib.h>	/* this include belongs in common.h */
> +
>  #include <ofono/log.h>
>  #include <ofono/modem.h>
>  #include <ofono/gprs.h>
> @@ -30,16 +32,34 @@
>  #include "qmi.h"
>  #include "nas.h"
>
> +#include "src/common.h"
>  #include "qmimodem.h"
>
>  struct gprs_data {
>  	struct qmi_service *nas;
> +	int attached;
>  };
>
> -static bool extract_ss_info(struct qmi_result *result, int *status)
> +static int rat_to_tech(uint8_t rat)
> +{
> +	switch (rat) {
> +	case QMI_NAS_NETWORK_RAT_GSM:
> +		return ACCESS_TECHNOLOGY_GSM;
> +	case QMI_NAS_NETWORK_RAT_UMTS:
> +		return ACCESS_TECHNOLOGY_UTRAN;
> +	case QMI_NAS_NETWORK_RAT_LTE:
> +		return ACCESS_TECHNOLOGY_EUTRAN;
> +	}
> +
> +	return -1;
> +}
> +

You might want to put this into drivers/qmimodem/nas.c as:

qmi_nas_rat_to_tech() and use it from both the netreg and gprs drivers

> +static bool extract_ss_info(struct qmi_result *result, int *status, int *tech)
>  {
>  	const struct qmi_nas_serving_system *ss;
> +	uint8_t roaming;
>  	uint16_t len;
> +	int i;
>
>  	DBG("");
>
> @@ -47,25 +67,80 @@ static bool extract_ss_info(struct qmi_result *result, int *status)
>  	if (!ss)
>  		return false;
>
> -	if (ss->ps_state == QMI_NAS_ATTACH_STATE_ATTACHED)
> -		*status = 0x01;
> -	else
> -		*status = 0x00;
> +	*status = ss->status;
> +
> +	*tech = -1;
> +	for (i = 0; i < ss->radio_if_count; i++) {
> +		DBG("radio in use %d", ss->radio_if[i]);
> +
> +		*tech = rat_to_tech(ss->radio_if[i]);
> +	}
> +
> +	if (qmi_result_get_uint8(result, QMI_NAS_RESULT_ROAMING_STATUS,
> +								&roaming)) {
> +		if (ss->status == 1 && roaming == 0)
> +			*status = NETWORK_REGISTRATION_STATUS_ROAMING;
> +	}
>
>  	return true;
>  }
>
> -static void ss_info_notify(struct qmi_result *result, void *user_data)
> +/*
> + * This method handles both Serving System Info requests and
> + * notifications.  The presence of a callback differentiates between
> + * the two and the result is handled slightly differently accordingly.
> + * If there's a callback, we invoke it; otherwise we send a notification
> + * to ofono about relevant state changes.
> + */

No, please don't do this, the notification callback and the query 
callback should be different functions.  If you need to perform some 
common operations, then just extract those into a common function and 
call it from both.

> +static void handle_ss_info(struct qmi_result* result, void *user_data)
>  {
> -	struct ofono_gprs *gprs = user_data;
> +	struct cb_data *cbd = user_data;
> +	struct ofono_gprs* gprs = cbd->user;
> +	struct gprs_data *data = ofono_gprs_get_data(gprs);
> +	ofono_gprs_status_cb_t cb = cbd->cb;
>  	int status;
> +	int tech;
>
>  	DBG("");
>
> -	if (!extract_ss_info(result, &status))
> -		return;
> +	if (cb && qmi_result_set_error(result, NULL))
> +			goto error;
> +
> +	if (!extract_ss_info(result, &status, &tech))
> +		goto error;
> +
> +	switch (status) {
> +	case NETWORK_REGISTRATION_STATUS_REGISTERED:
> +	case NETWORK_REGISTRATION_STATUS_ROAMING:
> +		if (tech == ACCESS_TECHNOLOGY_EUTRAN && !data->attached) {
> +			/* On LTE we are effectively always attached; and
> +			 * the default bearer is established as soon as the
> +			 * network is joined.
> +			 */
> +			data->attached = 1;
> +			/* FIXME: query default profile number and APN
> +			 * instead of assuming profile 1
> +			 */
> +			ofono_gprs_cid_activated(gprs, 1, "");
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!data->attached)
> +		status = NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
>
> -	ofono_gprs_status_notify(gprs, status);
> +	if (cb)
> +		CALLBACK_WITH_SUCCESS(cb, status, cbd->data);
> +	else
> +		ofono_gprs_status_notify(gprs, status);
> +
> +	return;
> +
> +error:
> +	if (cb)
> +		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
>  }
>
>  static void attach_detach_cb(struct qmi_result *result, void *user_data)
> @@ -100,6 +175,8 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
>
>  	DBG("attached %d", attached);
>
> +	data->attached = attached;
> +
>  	if (attached)
>  		action = QMI_NAS_ATTACH_ACTION_ATTACH;
>  	else
> @@ -121,27 +198,6 @@ error:
>  	g_free(cbd);
>  }
>
> -static void get_ss_info_cb(struct qmi_result *result, void *user_data)
> -{
> -	struct cb_data *cbd = user_data;
> -	ofono_gprs_status_cb_t cb = cbd->cb;
> -	int status;
> -
> -	DBG("");
> -
> -	if (qmi_result_set_error(result, NULL)) {
> -		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> -		return;
> -	}
> -
> -	if (!extract_ss_info(result, &status)) {
> -		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> -		return;
> -	}
> -
> -	CALLBACK_WITH_SUCCESS(cb, status, cbd->data);
> -}
> -
>  static void qmi_attached_status(struct ofono_gprs *gprs,
>  				ofono_gprs_status_cb_t cb, void *user_data)
>  {
> @@ -150,8 +206,14 @@ static void qmi_attached_status(struct ofono_gprs *gprs,
>
>  	DBG("");
>
> +	if (!data->attached) {
> +		CALLBACK_WITH_SUCCESS(cb,
> +			NETWORK_REGISTRATION_STATUS_NOT_REGISTERED, user_data);
> +	}
> +
> +	cbd->user = gprs;
>  	if (qmi_service_send(data->nas, QMI_NAS_GET_SS_INFO, NULL,
> -					get_ss_info_cb, cbd, g_free) > 0)
> +					handle_ss_info, cbd, g_free) > 0)
>  		return;
>
>  	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> @@ -163,6 +225,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
>  {
>  	struct ofono_gprs *gprs = user_data;
>  	struct gprs_data *data = ofono_gprs_get_data(gprs);
> +	struct cb_data *cbd;
>
>  	DBG("");
>
> @@ -178,11 +241,15 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
>  	 * First get the SS info - the modem may already be connected,
>  	 * and the state-change notification may never arrive
>  	 */
> +	cbd = cb_data_new(NULL, NULL);
> +	cbd->user = gprs;
>  	qmi_service_send(data->nas, QMI_NAS_GET_SS_INFO, NULL,
> -					ss_info_notify, gprs, NULL);
> +					handle_ss_info, cbd, g_free);
>
> +	cbd = cb_data_new(NULL, NULL);
> +	cbd->user = gprs;
>  	qmi_service_register(data->nas, QMI_NAS_SS_INFO_IND,
> -					ss_info_notify, gprs, NULL);
> +					handle_ss_info, cbd, g_free);

No, the original form was completely fine.

>
>  	ofono_gprs_set_cid_range(gprs, 1, 1);
>
>

Regards,
-Denis

Re: [PATCH qmi lte v3 3/6] qmi: activate default bearer context for LTE networks

[Jonas Bonn]

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

Hi Denis,

On 04/17/2017 07:37 PM, Denis Kenzior wrote:
> Hi Jonas,
> -static bool extract_ss_info(struct qmi_result *result, int *status)
>> +static int rat_to_tech(uint8_t rat)
>> +{
>> +    switch (rat) {
>> +    case QMI_NAS_NETWORK_RAT_GSM:
>> +        return ACCESS_TECHNOLOGY_GSM;
>> +    case QMI_NAS_NETWORK_RAT_UMTS:
>> +        return ACCESS_TECHNOLOGY_UTRAN;
>> +    case QMI_NAS_NETWORK_RAT_LTE:
>> +        return ACCESS_TECHNOLOGY_EUTRAN;
>> +    }
>> +
>> +    return -1;
>> +}
>> +
>
> You might want to put this into drivers/qmimodem/nas.c as:
>
> qmi_nas_rat_to_tech() and use it from both the netreg and gprs drivers

Good idea... will do!

>
>> -static void ss_info_notify(struct qmi_result *result, void *user_data)
>> +/*
>> + * This method handles both Serving System Info requests and
>> + * notifications.  The presence of a callback differentiates between
>> + * the two and the result is handled slightly differently accordingly.
>> + * If there's a callback, we invoke it; otherwise we send a 
>> notification
>> + * to ofono about relevant state changes.
>> + */
>
> No, please don't do this, the notification callback and the query 
> callback should be different functions.  If you need to perform some 
> common operations, then just extract those into a common function and 
> call it from both.

drivers/rilmodem/gprs-context.c, as one example, uses this pattern. 
Personally, I find it rather elegant!

>>      CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
>> @@ -163,6 +225,7 @@ static void create_nas_cb(struct qmi_service 
>> *service, void *user_data)
>>  {
>>      struct ofono_gprs *gprs = user_data;
>>      struct gprs_data *data = ofono_gprs_get_data(gprs);
>> +    struct cb_data *cbd;
>>
>>      DBG("");
>>
>> @@ -178,11 +241,15 @@ static void create_nas_cb(struct qmi_service 
>> *service, void *user_data)
>>       * First get the SS info - the modem may already be connected,
>>       * and the state-change notification may never arrive
>>       */
>> +    cbd = cb_data_new(NULL, NULL);
>> +    cbd->user = gprs;
>>      qmi_service_send(data->nas, QMI_NAS_GET_SS_INFO, NULL,
>> -                    ss_info_notify, gprs, NULL);
>> +                    handle_ss_info, cbd, g_free);
>>
>> +    cbd = cb_data_new(NULL, NULL);
>> +    cbd->user = gprs;
>>      qmi_service_register(data->nas, QMI_NAS_SS_INFO_IND,
>> -                    ss_info_notify, gprs, NULL);
>> +                    handle_ss_info, cbd, g_free);
>
> No, the original form was completely fine.

handle_ss_info takes a cbd user_data argument, not a gprs argument. But 
if handle_ss_info is not acceptable, then I guess the above is fine.

/Jonas

Re: [PATCH qmi lte v3 3/6] qmi: activate default bearer context for LTE networks

[Denis Kenzior]

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

Hi Jonas,

>> No, please don't do this, the notification callback and the query
>> callback should be different functions.  If you need to perform some
>> common operations, then just extract those into a common function and
>> call it from both.
>
> drivers/rilmodem/gprs-context.c, as one example, uses this pattern.
> Personally, I find it rather elegant!

I don't see an example of such in there, but I didn't look hard enough. 
Do you mean the use of ril_gprs_context_deactivate_primary inside 
ril_gprs_context_deactivate_primary?

Regardless, we really prefer unsolicited notification and command 
execution result callbacks to be different.  Generally because the 
callback data is different and there's no nice way to hide the 
differences besides a bunch of if statements.  We prefer the code to be 
easier to read, even if longer.  Lots of if-else statements make the 
code harder to read.

Regards,
-Denis

Re: [PATCH qmi lte v3 3/6] qmi: activate default bearer context for LTE networks

[Jonas Bonn]

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

On 04/17/2017 10:12 PM, Denis Kenzior wrote:
> Hi Jonas,
>
>>> No, please don't do this, the notification callback and the query
>>> callback should be different functions.  If you need to perform some
>>> common operations, then just extract those into a common function and
>>> call it from both.
>>
>> drivers/rilmodem/gprs-context.c, as one example, uses this pattern.
>> Personally, I find it rather elegant!
>
> I don't see an example of such in there, but I didn't look hard 
> enough. Do you mean the use of ril_gprs_context_deactivate_primary 
> inside ril_gprs_context_deactivate_primary?

Line 706... but no worries, I'll expand to separate functions since 
that's the preference.

/Jonas

>
> Regardless, we really prefer unsolicited notification and command 
> execution result callbacks to be different.  Generally because the 
> callback data is different and there's no nice way to hide the 
> differences besides a bunch of if statements.  We prefer the code to 
> be easier to read, even if longer.  Lots of if-else statements make 
> the code harder to read.
>
> Regards,
> -Denis

[PATCH qmi lte v3 4/6] qim: use named status value

[Jonas Bonn]

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

---
 drivers/qmimodem/network-registration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/qmimodem/network-registration.c b/drivers/qmimodem/network-registration.c
index a3f9cf9..c29a733 100644
--- a/drivers/qmimodem/network-registration.c
+++ b/drivers/qmimodem/network-registration.c
@@ -88,7 +88,7 @@ static bool extract_ss_info(struct qmi_result *result, int *status,
 	if (qmi_result_get_uint8(result, QMI_NAS_RESULT_ROAMING_STATUS,
 								&roaming)) {
 		if (ss->status == 1 && roaming == 0)
-			*status = 5;
+			*status = NETWORK_REGISTRATION_STATUS_ROAMING;
 	}
 
 	if (!operator)
-- 
2.9.3

Re: [PATCH qmi lte v3 4/6] qim: use named status value

[Denis Kenzior]

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

Hi Jonas,

On 04/15/2017 10:34 AM, Jonas Bonn wrote:
> ---
>  drivers/qmimodem/network-registration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH qmi lte v3 5/6] qmi: stop listening to packet service notifications

[Jonas Bonn]

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

The packet service status notification tells whether there is _any_
active and enabled context.  Using this to decide whether to
release any given context makes no sense.

It might make sense to monitor this status in the GPRS atom, but
not here in the gprs-context atom.
---
 drivers/qmimodem/gprs-context.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index 2085898..5b1d778 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -44,33 +44,6 @@ struct gprs_context_data {
 	uint32_t pkt_handle;
 };
 
-static void pkt_status_notify(struct qmi_result *result, void *user_data)
-{
-	struct ofono_gprs_context *gc = user_data;
-	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
-	const struct qmi_wds_notify_conn_status *status;
-	uint16_t len;
-	uint8_t ip_family;
-
-	DBG("");
-
-	status = qmi_result_get(result, QMI_WDS_NOTIFY_CONN_STATUS, &len);
-	if (!status)
-		return;
-
-	DBG("conn status %d", status->status);
-
-	if (qmi_result_get_uint8(result, QMI_WDS_NOTIFY_IP_FAMILY, &ip_family))
-		DBG("ip family %d", ip_family);
-
-	switch (status->status) {
-	case QMI_WDS_CONN_STATUS_DISCONNECTED:
-		ofono_gprs_context_deactivated(gc, data->active_context);
-		data->active_context = 0;
-		break;
-	}
-}
-
 static void get_settings_cb(struct qmi_result *result, void *user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -376,9 +349,6 @@ static void create_wds_cb(struct qmi_service *service, void *user_data)
 	}
 
 	data->wds = qmi_service_ref(service);
-
-	qmi_service_register(data->wds, QMI_WDS_PKT_STATUS_IND,
-					pkt_status_notify, gc, NULL);
 }
 
 static void get_data_format_cb(struct qmi_result *result, void *user_data)
-- 
2.9.3

Re: [PATCH qmi lte v3 5/6] qmi: stop listening to packet service notifications

[Denis Kenzior]

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

Hi Jonas,

On 04/15/2017 10:34 AM, Jonas Bonn wrote:
> The packet service status notification tells whether there is _any_
> active and enabled context.  Using this to decide whether to
> release any given context makes no sense.

The gobi driver only supports a single context and afaik only a single 
network interface is available.  Do you have hardware with support for 
multiple active contexts?

Otherwise I think it would be safe to assume that in a single-context 
case, this code is doing the right thing.

>
> It might make sense to monitor this status in the GPRS atom, but
> not here in the gprs-context atom.
> ---
>  drivers/qmimodem/gprs-context.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
>

Regards,
-Denis

Re: [PATCH qmi lte v3 5/6] qmi: stop listening to packet service notifications

[Jonas Bonn]

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

On 04/17/2017 07:19 PM, Denis Kenzior wrote:
> Hi Jonas,
>
> On 04/15/2017 10:34 AM, Jonas Bonn wrote:
>> The packet service status notification tells whether there is _any_
>> active and enabled context.  Using this to decide whether to
>> release any given context makes no sense.
>
> The gobi driver only supports a single context and afaik only a single 
> network interface is available.  Do you have hardware with support for 
> multiple active contexts?

On an LTE network you can start a "dedicated" bearer.  As far as I 
understand it, the way this is done is to call "start network" 
specifying a profile ID other than the default.  In the ofono context, I 
presume this would be done by creating a new context and activating it:  
it just needs the same APN as the default bearer, and normally one would 
specify other QoS settings.  Given that, then yes, I presume my hardware 
support multiple active contexts; if the Gobi driver only supports one 
context it's presumably because it doesn't support LTE.

>
> Otherwise I think it would be safe to assume that in a single-context 
> case, this code is doing the right thing.

I think, for LTE, that the netreg atom gives you everything you need and 
that this notification is superflous.  I'm thinking now that for non-LTE 
this code might be relevant:  can the context detach spontaneously 
without unregistering from the network?  If yes, then we probably need 
this, still.

/Jonas

>
>>
>> It might make sense to monitor this status in the GPRS atom, but
>> not here in the gprs-context atom.
>> ---
>>  drivers/qmimodem/gprs-context.c | 30 ------------------------------
>>  1 file changed, 30 deletions(-)
>>
>
> Regards,
> -Denis
>

Re: [PATCH qmi lte v3 5/6] qmi: stop listening to packet service notifications

[Denis Kenzior]

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

Hi Jonas,

On 04/17/2017 02:45 PM, Jonas Bonn wrote:
> On 04/17/2017 07:19 PM, Denis Kenzior wrote:
>> Hi Jonas,
>>
>> On 04/15/2017 10:34 AM, Jonas Bonn wrote:
>>> The packet service status notification tells whether there is _any_
>>> active and enabled context.  Using this to decide whether to
>>> release any given context makes no sense.
>>
>> The gobi driver only supports a single context and afaik only a single
>> network interface is available.  Do you have hardware with support for
>> multiple active contexts?
>
> On an LTE network you can start a "dedicated" bearer.  As far as I
> understand it, the way this is done is to call "start network"
> specifying a profile ID other than the default.  In the ofono context, I
> presume this would be done by creating a new context and activating it:
> it just needs the same APN as the default bearer, and normally one would
> specify other QoS settings.  Given that, then yes, I presume my hardware
> support multiple active contexts; if the Gobi driver only supports one
> context it's presumably because it doesn't support LTE.

Okay, so now you're opening a whole new can of worms.  Dedicated bearers 
are similar to secondary PDP contexts in 2G/3G.  Their handling is even 
more special than primary PDP contexts / default bearers.

I wouldn't presume that your hardware supports this.  You need to check. 
  How many physical network interfaces are exposed?  If its just one 
like the gobi has, then you have your answer.

>
>>
>> Otherwise I think it would be safe to assume that in a single-context
>> case, this code is doing the right thing.
>
> I think, for LTE, that the netreg atom gives you everything you need and
> that this notification is superflous.  I'm thinking now that for non-LTE
> this code might be relevant:  can the context detach spontaneously
> without unregistering from the network?  If yes, then we probably need
> this, still.
>

A context can be forcefully deactivated by the network operator at any 
time.  So yes, this is actually needed.

Regards,
-Denis

Re: [PATCH qmi lte v3 5/6] qmi: stop listening to packet service notifications

[Jonas Bonn]

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

On 04/17/2017 09:51 PM, Denis Kenzior wrote:
> Hi Jonas,
>
> On 04/17/2017 02:45 PM, Jonas Bonn wrote:
>> On 04/17/2017 07:19 PM, Denis Kenzior wrote:
>>> Hi Jonas,
>>>
>>> On 04/15/2017 10:34 AM, Jonas Bonn wrote:
>>>> The packet service status notification tells whether there is _any_
>>>> active and enabled context.  Using this to decide whether to
>>>> release any given context makes no sense.
>>>
>>> The gobi driver only supports a single context and afaik only a single
>>> network interface is available.  Do you have hardware with support for
>>> multiple active contexts?
>>
>> On an LTE network you can start a "dedicated" bearer.  As far as I
>> understand it, the way this is done is to call "start network"
>> specifying a profile ID other than the default.  In the ofono context, I
>> presume this would be done by creating a new context and activating it:
>> it just needs the same APN as the default bearer, and normally one would
>> specify other QoS settings.  Given that, then yes, I presume my hardware
>> support multiple active contexts; if the Gobi driver only supports one
>> context it's presumably because it doesn't support LTE.
>
> Okay, so now you're opening a whole new can of worms.  Dedicated 
> bearers are similar to secondary PDP contexts in 2G/3G.  Their 
> handling is even more special than primary PDP contexts / default 
> bearers.
>
> I wouldn't presume that your hardware supports this.  You need to 
> check.  How many physical network interfaces are exposed?  If its just 
> one like the gobi has, then you have your answer.

Don't all LTE modems that support voice support dedicated bearers?

I think I was assuming you'd just get another IP address to set on the 
network interface... packets from that source would be handled with a 
different QoS setting.  Poking around the libqmi archive it seems that 
there is some activity around dedicated bearers, but all the modems in 
question only one have network interface.  But I really don't know 
enough about this... :(

My hardware has only network interface in any case.

>>
>>>
>>> Otherwise I think it would be safe to assume that in a single-context
>>> case, this code is doing the right thing.
>>
>> I think, for LTE, that the netreg atom gives you everything you need and
>> that this notification is superflous.  I'm thinking now that for non-LTE
>> this code might be relevant:  can the context detach spontaneously
>> without unregistering from the network?  If yes, then we probably need
>> this, still.
>>
>
> A context can be forcefully deactivated by the network operator at any 
> time.  So yes, this is actually needed.

OK, this patch is then plainly wrong.  I'll drop it.

/Jonas

>
> Regards,
> -Denis
>

Re: [PATCH qmi lte v3 5/6] qmi: stop listening to packet service notifications

[Denis Kenzior]

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

Hi Jonas,

>> Okay, so now you're opening a whole new can of worms.  Dedicated
>> bearers are similar to secondary PDP contexts in 2G/3G.  Their
>> handling is even more special than primary PDP contexts / default
>> bearers.
>>
>> I wouldn't presume that your hardware supports this.  You need to
>> check.  How many physical network interfaces are exposed?  If its just
>> one like the gobi has, then you have your answer.
>
> Don't all LTE modems that support voice support dedicated bearers?

The answer is it depends.  Even if they do, you still need an IMS VoLTE 
stack to do anything useful with them.  Most modems have an IMS stack 
internal to the firmware, but not all will ship with one for your 
typical broadband connectivity stick.

>
> I think I was assuming you'd just get another IP address to set on the
> network interface... packets from that source would be handled with a
> different QoS setting.  Poking around the libqmi archive it seems that
> there is some activity around dedicated bearers, but all the modems in
> question only one have network interface.  But I really don't know
> enough about this... :(

The IP address will be the same.  All the dedicated bearer does is apply 
QoS to a particular service.

Maybe with QMI the packets will flow on the original network interface, 
even if a dedicated bearer was setup.  But then volte/ims usually uses a 
separate APN/bearer anyway.

>
> My hardware has only network interface in any case.
>

There you go.  I think QMI is mostly meant for data-only cards.

Regards,
-Denis

[PATCH qmi lte v3 6/6] qmi: consolidate ss_info handling functions

[Jonas Bonn]

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

---
 drivers/qmimodem/network-registration.c | 51 ++++++++++++++-------------------
 1 file changed, 22 insertions(+), 29 deletions(-)

diff --git a/drivers/qmimodem/network-registration.c b/drivers/qmimodem/network-registration.c
index c29a733..72b76d9 100644
--- a/drivers/qmimodem/network-registration.c
+++ b/drivers/qmimodem/network-registration.c
@@ -135,42 +135,32 @@ static bool extract_ss_info(struct qmi_result *result, int *status,
 	return true;
 }
 
-static void ss_info_notify(struct qmi_result *result, void *user_data)
+static void handle_ss_info(struct qmi_result *result, void *user_data)
 {
-	struct ofono_netreg *netreg = user_data;
+	struct cb_data *cbd = user_data;
+	struct ofono_netreg *netreg = cbd->user;
 	struct netreg_data *data = ofono_netreg_get_data(netreg);
+	ofono_netreg_status_cb_t cb = cbd->cb;
 	int status, lac, cellid, tech;
 
 	DBG("");
 
-	if (!extract_ss_info(result, &status, &lac, &cellid, &tech,
-							&data->operator))
-		return;
-
-	ofono_netreg_status_notify(netreg, status, lac, cellid, tech);
-}
-
-static void get_ss_info_cb(struct qmi_result *result, void *user_data)
-{
-	struct cb_data *cbd = user_data;
-	ofono_netreg_status_cb_t cb = cbd->cb;
-	struct netreg_data *data = cbd->user;
-	int status, lac, cellid, tech;
+	if (cb && qmi_result_set_error(result, NULL))
+		goto error;
 
-	DBG("");
+	if (!extract_ss_info(result, &status, &lac,
+				&cellid, &tech, &data->operator))
+		goto error;
 
-	if (qmi_result_set_error(result, NULL)) {
-		CALLBACK_WITH_FAILURE(cb, -1, -1, -1, -1, cbd->data);
-		return;
-	}
+	if (cb)
+		CALLBACK_WITH_SUCCESS(cb, status,
+					lac, cellid, tech, cbd->data);
+	else
+		ofono_netreg_status_notify(netreg, status, lac, cellid, tech);
 
-	if (!extract_ss_info(result, &status, &lac, &cellid, &tech,
-							&data->operator)) {
+error:
+	if (cb)
 		CALLBACK_WITH_FAILURE(cb, -1, -1, -1, -1, cbd->data);
-		return;
-	}
-
-	CALLBACK_WITH_SUCCESS(cb, status, lac, cellid, tech, cbd->data);
 }
 
 static void qmi_registration_status(struct ofono_netreg *netreg,
@@ -181,10 +171,10 @@ static void qmi_registration_status(struct ofono_netreg *netreg,
 
 	DBG("");
 
-	cbd->user = data;
+	cbd->user = netreg;
 
 	if (qmi_service_send(data->nas, QMI_NAS_GET_SS_INFO, NULL,
-					get_ss_info_cb, cbd, g_free) > 0)
+					handle_ss_info, cbd, g_free) > 0)
 		return;
 
 	CALLBACK_WITH_FAILURE(cb, -1, -1, -1, -1, cbd->data);
@@ -498,6 +488,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 	struct ofono_netreg *netreg = user_data;
 	struct netreg_data *data = ofono_netreg_get_data(netreg);
 	struct qmi_param *param;
+	struct cb_data *cbd;
 	struct qmi_nas_param_event_signal_strength ss = { .report = 0x01,
 				.count = 5, .dbm[0] = -55, .dbm[1] = -65,
 				.dbm[2] = -75, .dbm[3] = -85, .dbm[4] = -95 };
@@ -515,8 +506,10 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
 	qmi_service_register(data->nas, QMI_NAS_EVENT,
 					event_notify, netreg, NULL);
 
+	cbd = cb_data_new(NULL, NULL);
+	cbd->user = netreg;
 	qmi_service_register(data->nas, QMI_NAS_SS_INFO_IND,
-					ss_info_notify, netreg, NULL);
+					handle_ss_info, cbd, g_free);
 
 	param = qmi_param_new();
 	if (!param)
-- 
2.9.3

Re: [PATCH qmi lte v3 6/6] qmi: consolidate ss_info handling functions

[Denis Kenzior]

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

Hi Jonas,

On 04/15/2017 10:34 AM, Jonas Bonn wrote:
> ---
>  drivers/qmimodem/network-registration.c | 51 ++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/qmimodem/network-registration.c b/drivers/qmimodem/network-registration.c
> index c29a733..72b76d9 100644
> --- a/drivers/qmimodem/network-registration.c
> +++ b/drivers/qmimodem/network-registration.c
> @@ -135,42 +135,32 @@ static bool extract_ss_info(struct qmi_result *result, int *status,
>  	return true;
>  }
>
> -static void ss_info_notify(struct qmi_result *result, void *user_data)
> +static void handle_ss_info(struct qmi_result *result, void *user_data)
>  {
> -	struct ofono_netreg *netreg = user_data;
> +	struct cb_data *cbd = user_data;
> +	struct ofono_netreg *netreg = cbd->user;
>  	struct netreg_data *data = ofono_netreg_get_data(netreg);
> +	ofono_netreg_status_cb_t cb = cbd->cb;
>  	int status, lac, cellid, tech;
>
>  	DBG("");
>
> -	if (!extract_ss_info(result, &status, &lac, &cellid, &tech,
> -							&data->operator))
> -		return;
> -
> -	ofono_netreg_status_notify(netreg, status, lac, cellid, tech);
> -}
> -

Actually I prefer to have two separate callbacks.

> -static void get_ss_info_cb(struct qmi_result *result, void *user_data)
> -{
> -	struct cb_data *cbd = user_data;
> -	ofono_netreg_status_cb_t cb = cbd->cb;
> -	struct netreg_data *data = cbd->user;
> -	int status, lac, cellid, tech;
> +	if (cb && qmi_result_set_error(result, NULL))
> +		goto error;
>
> -	DBG("");
> +	if (!extract_ss_info(result, &status, &lac,
> +				&cellid, &tech, &data->operator))
> +		goto error;
>
> -	if (qmi_result_set_error(result, NULL)) {
> -		CALLBACK_WITH_FAILURE(cb, -1, -1, -1, -1, cbd->data);
> -		return;
> -	}
> +	if (cb)
> +		CALLBACK_WITH_SUCCESS(cb, status,
> +					lac, cellid, tech, cbd->data);
> +	else
> +		ofono_netreg_status_notify(netreg, status, lac, cellid, tech);
>
> -	if (!extract_ss_info(result, &status, &lac, &cellid, &tech,
> -							&data->operator)) {
> +error:
> +	if (cb)
>  		CALLBACK_WITH_FAILURE(cb, -1, -1, -1, -1, cbd->data);
> -		return;
> -	}
> -
> -	CALLBACK_WITH_SUCCESS(cb, status, lac, cellid, tech, cbd->data);

The if (cb) stuff is just harder to read.

>  }
>
>  static void qmi_registration_status(struct ofono_netreg *netreg,
> @@ -181,10 +171,10 @@ static void qmi_registration_status(struct ofono_netreg *netreg,
>
>  	DBG("");
>
> -	cbd->user = data;
> +	cbd->user = netreg;
>
>  	if (qmi_service_send(data->nas, QMI_NAS_GET_SS_INFO, NULL,
> -					get_ss_info_cb, cbd, g_free) > 0)
> +					handle_ss_info, cbd, g_free) > 0)
>  		return;
>
>  	CALLBACK_WITH_FAILURE(cb, -1, -1, -1, -1, cbd->data);
> @@ -498,6 +488,7 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
>  	struct ofono_netreg *netreg = user_data;
>  	struct netreg_data *data = ofono_netreg_get_data(netreg);
>  	struct qmi_param *param;
> +	struct cb_data *cbd;
>  	struct qmi_nas_param_event_signal_strength ss = { .report = 0x01,
>  				.count = 5, .dbm[0] = -55, .dbm[1] = -65,
>  				.dbm[2] = -75, .dbm[3] = -85, .dbm[4] = -95 };
> @@ -515,8 +506,10 @@ static void create_nas_cb(struct qmi_service *service, void *user_data)
>  	qmi_service_register(data->nas, QMI_NAS_EVENT,
>  					event_notify, netreg, NULL);
>
> +	cbd = cb_data_new(NULL, NULL);
> +	cbd->user = netreg;

Please don't ever do this, it just looks weird and tells the reader 
you're abusing the system (e.g. its a crude hack).

>  	qmi_service_register(data->nas, QMI_NAS_SS_INFO_IND,
> -					ss_info_notify, netreg, NULL);
> +					handle_ss_info, cbd, g_free);
>
>  	param = qmi_param_new();
>  	if (!param)
>

Regards,
-Denis