[PATCH QMI LTE v2 00/13] QMI LTE series

[PATCH QMI LTE v2 00/13] QMI LTE series

[Jonas Bonn]

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

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 (13):
  qmi: duplicate callback data correctly
  qmi: fix typo
  gprs: release active contexts completely
  gprs: _cid_activated is an 'attaching' state
  gprs: set driver_attached when activating automatic contexts
  qmi: implement detach_shutdown method
  qmi: read_settings needs to call start network
  qmi: don't leak cbd and rely on destroy function
  qmi: activate default bearer context for LTE networks
  qmi: use destroy callback for activate_primary
  qmi: stop listening to packet service notifications
  qmi: rely on destroy callback
  qmi: consolidate ss_info handling functions

 drivers/qmimodem/gprs-context.c         | 126 +++++++++++++---------------
 drivers/qmimodem/gprs.c                 | 140 ++++++++++++++++++++++----------
 drivers/qmimodem/nas.h                  |   2 +-
 drivers/qmimodem/network-registration.c |  73 +++++++----------
 src/gprs.c                              |  22 ++++-
 5 files changed, 205 insertions(+), 158 deletions(-)

-- 
2.9.3

[PATCH QMI LTE v2 01/13] qmi: duplicate callback data correctly

[Jonas Bonn]

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

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

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index a1ee217..247ee9e 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -198,7 +198,7 @@ static void start_net_cb(struct qmi_result *result, void *user_data)
 	data->pkt_handle = handle;
 
 	/* Duplicate cbd, the old one will be freed when this method returns */
-	cbd = cb_data_new(cb, user_data);
+	cbd = cb_data_new(cb, cbd->data);
 	cbd->user = gc;
 
 	if (qmi_service_send(data->wds, QMI_WDS_GET_SETTINGS, NULL,
-- 
2.9.3

Re: [PATCH QMI LTE v2 01/13] qmi: duplicate callback data correctly

[Denis Kenzior]

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

Hi Jonas,

On 04/14/2017 04:36 PM, Jonas Bonn wrote:
> ---
>  drivers/qmimodem/gprs-context.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
> index a1ee217..247ee9e 100644
> --- a/drivers/qmimodem/gprs-context.c
> +++ b/drivers/qmimodem/gprs-context.c
> @@ -198,7 +198,7 @@ static void start_net_cb(struct qmi_result *result, void *user_data)
>  	data->pkt_handle = handle;
>
>  	/* Duplicate cbd, the old one will be freed when this method returns */
> -	cbd = cb_data_new(cb, user_data);
> +	cbd = cb_data_new(cb, cbd->data);

Ah darn, my fault for not double-checking my earlier fix. Thanks for 
catching this.

>  	cbd->user = gc;
>
>  	if (qmi_service_send(data->wds, QMI_WDS_GET_SETTINGS, NULL,
>

Applied, thanks.

Regards,
-Denis

[PATCH QMI LTE v2 02/13] qmi: fix typo

[Jonas Bonn]

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

---
 drivers/qmimodem/nas.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/qmimodem/nas.h b/drivers/qmimodem/nas.h
index 9bb9a9a..b39b425 100644
--- a/drivers/qmimodem/nas.h
+++ b/drivers/qmimodem/nas.h
@@ -142,7 +142,7 @@ struct qmi_nas_current_plmn {
 
 /* qmi_nas_serving_system.status */
 #define QMI_NAS_REGISTRATION_STATE_NOT_REGISTERED	0x00
-#define QMI_NAS_REGISTRATION_STATE_REGISTEREID		0x01
+#define QMI_NAS_REGISTRATION_STATE_REGISTERED		0x01
 #define QMI_NAS_REGISTRATION_STATE_SEARCHING		0x02
 #define QMI_NAS_REGISTRATION_STATE_DENIED		0x03
 #define QMI_NAS_REGISTRATION_STATE_UNKNOWN		0x04
-- 
2.9.3

Re: [PATCH QMI LTE v2 02/13] qmi: fix typo

[Denis Kenzior]

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

Hi Jonas,

On 04/14/2017 04:36 PM, Jonas Bonn wrote:
> ---
>  drivers/qmimodem/nas.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH QMI LTE v2 03/13] gprs: release active contexts completely

[Jonas Bonn]

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

The release_active_contexts method ask the driver to deactive all
the active contexts it knows about; however, after doing so, the
context state needs to be released, as well, so that the contexts
do not continue to appear to be active.
---
 src/gprs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/gprs.c b/src/gprs.c
index 6ed1c89..b6e11e8 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1614,6 +1614,9 @@ static void release_active_contexts(struct ofono_gprs *gprs)
 
 		if (gc->driver->detach_shutdown != NULL)
 			gc->driver->detach_shutdown(gc, ctx->context.cid);
+
+		pri_reset_context_settings(ctx);
+		release_context(ctx);
 	}
 }
 
-- 
2.9.3

Re: [PATCH QMI LTE v2 03/13] gprs: release active contexts completely

[Denis Kenzior]

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

Hi Jonas,

On 04/14/2017 04:36 PM, Jonas Bonn wrote:
> The release_active_contexts method ask the driver to deactive all
> the active contexts it knows about; however, after doing so, the
> context state needs to be released, as well, so that the contexts
> do not continue to appear to be active.
> ---
>  src/gprs.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/src/gprs.c b/src/gprs.c
> index 6ed1c89..b6e11e8 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -1614,6 +1614,9 @@ static void release_active_contexts(struct ofono_gprs *gprs)
>
>  		if (gc->driver->detach_shutdown != NULL)
>  			gc->driver->detach_shutdown(gc, ctx->context.cid);
> +
> +		pri_reset_context_settings(ctx);
> +		release_context(ctx);

Hmm, this seems wrong.  The original commit (according to git blame, git 
show 05b8fe47) was aimed at PPP contexts.  The gprs_context driver will 
call g_at_ppp_shutdown which in turn will result in eventual 
ppp_disconnect.  The context driver will then call 
ofono_gprs_cid_deactivated.

For the other context drivers, it was assumed that the modem will be 
sane enough to issue a +CGEV with a context deactivation.

I'm assuming you need this for QMI as well, so you might want to use 
similar semantics.  E.g. have detach_shutdown call STOP_NET and then 
call ofono_gprs_cid_deactivated.

>  	}
>  }
>
>

Regards,
-Denis

[PATCH QMI LTE v2 04/13] gprs: _cid_activated is an 'attaching' state

[Jonas Bonn]

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

ofono_gprs_status_notify is an asynchronous notification that messes
with the 'attached' state of the GPRS atom.  This method is normally
prevented from running while an attach is in progress because the
attachment machinery wants to finish up and make it's own determination
of attach state.

When automatic context activation is relevant, as for LTE networks,
the ofono_gprs_cid_activated machinery replaces the usual set_attach
machinery for attaching to the network.  The cid_activated variant,
however, does not guard against simulatenous invocations of
ofono_gprs_status_notify.  This causes a race whereby status_notify
sets the state to 'attached' before the context is fully constructed
and set to active.  If the connection manager sees the 'attached'
state before there are any 'active' contexts, it may decide to
activate a context manually which is not the correct behaviour for
this type of network.

This patch makes the *_cid_activated machinery an 'attaching' state,
introducing the same guards that set_attached has to prevent
ofono_gprs_status_notify from running concurrently.
---
 src/gprs.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/gprs.c b/src/gprs.c
index b6e11e8..0da9c32 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -982,6 +982,7 @@ static void pri_read_settings_callback(const struct ofono_error *error,
 {
 	struct pri_context *pri_ctx = data;
 	struct ofono_gprs_context *gc = pri_ctx->context_driver;
+	struct ofono_gprs *gprs = pri_ctx->gprs;
 	DBusConnection *conn = ofono_dbus_get_connection();
 	dbus_bool_t value;
 
@@ -1006,11 +1007,18 @@ static void pri_read_settings_callback(const struct ofono_error *error,
 
 	value = pri_ctx->active;
 
-	gprs_set_attached_property(pri_ctx->gprs, TRUE);
+	gprs->flags &= !GPRS_FLAG_ATTACHING;
+
+	gprs_set_attached_property(gprs, TRUE);
 
 	ofono_dbus_signal_property_changed(conn, pri_ctx->path,
 					OFONO_CONNECTION_CONTEXT_INTERFACE,
 					"Active", DBUS_TYPE_BOOLEAN, &value);
+
+	if (gprs->flags & GPRS_FLAG_RECHECK) {
+		gprs->flags &= ~GPRS_FLAG_RECHECK;
+		gprs_netreg_update(gprs);
+	}
 }
 
 static DBusMessage *pri_set_apn(struct pri_context *ctx, DBusConnection *conn,
@@ -2034,6 +2042,14 @@ void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
 		pri_set_apn(pri_ctx, conn, NULL, apn);
 	}
 
+	/* Prevent ofono_gprs_status_notify from changing the 'attached'
+	 * state until after the context has been set to 'active' in
+	 * the pri_read_settings_callback; this prevents a race where
+	 * the connection manager sees the modem as attached before there
+	 * is an active context.
+	 */
+	gprs->flags |= GPRS_FLAG_ATTACHING;
+
 	gc->driver->read_settings(gc, cid, pri_read_settings_callback, pri_ctx);
 }
 
-- 
2.9.3

Re: [PATCH QMI LTE v2 04/13] gprs: _cid_activated is an 'attaching' state

[Denis Kenzior]

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

Hi Jonas,

On 04/14/2017 04:36 PM, Jonas Bonn wrote:
> ofono_gprs_status_notify is an asynchronous notification that messes
> with the 'attached' state of the GPRS atom.  This method is normally
> prevented from running while an attach is in progress because the
> attachment machinery wants to finish up and make it's own determination
> of attach state.
>
> When automatic context activation is relevant, as for LTE networks,
> the ofono_gprs_cid_activated machinery replaces the usual set_attach
> machinery for attaching to the network.  The cid_activated variant,
> however, does not guard against simulatenous invocations of
> ofono_gprs_status_notify.  This causes a race whereby status_notify
> sets the state to 'attached' before the context is fully constructed
> and set to active.  If the connection manager sees the 'attached'
> state before there are any 'active' contexts, it may decide to
> activate a context manually which is not the correct behaviour for
> this type of network.
>
> This patch makes the *_cid_activated machinery an 'attaching' state,
> introducing the same guards that set_attached has to prevent
> ofono_gprs_status_notify from running concurrently.
> ---
>  src/gprs.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>

This looks reasonable to me.  Applied, thanks.

Regards,
-Denis

[PATCH QMI LTE v2 05/13] gprs: set driver_attached when activating automatic contexts

[Jonas Bonn]

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

The ofono_gprs_cid_activated attachment machinery cannot go through
ofono_gprs_status_notify for getting the attached property set because
that would result in the automatic contexts that were just set up
being released.  As such, it needs to call gprs_set_attached_property
manually.  Doing so, however, means that the driver_attached property
never gets set, resulting in all contexts being released when the
network transitions between registered states (roaming/non-roaming).
---
 src/gprs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/gprs.c b/src/gprs.c
index 0da9c32..091305a 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1009,6 +1009,7 @@ static void pri_read_settings_callback(const struct ofono_error *error,
 
 	gprs->flags &= !GPRS_FLAG_ATTACHING;
 
+	gprs->driver_attached = TRUE;
 	gprs_set_attached_property(gprs, TRUE);
 
 	ofono_dbus_signal_property_changed(conn, pri_ctx->path,
-- 
2.9.3

Re: [PATCH QMI LTE v2 05/13] gprs: set driver_attached when activating automatic contexts

[Denis Kenzior]

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

Hi Jonas,

On 04/14/2017 04:36 PM, Jonas Bonn wrote:
> The ofono_gprs_cid_activated attachment machinery cannot go through
> ofono_gprs_status_notify for getting the attached property set because
> that would result in the automatic contexts that were just set up
> being released.  As such, it needs to call gprs_set_attached_property
> manually.  Doing so, however, means that the driver_attached property
> never gets set, resulting in all contexts being released when the
> network transitions between registered states (roaming/non-roaming).
> ---
>  src/gprs.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

Regards,
-Denis

[PATCH QMI LTE v2 06/13] qmi: implement detach_shutdown method

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 2557 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 that function.
---
 drivers/qmimodem/gprs-context.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index 247ee9e..b72245c 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -304,14 +304,16 @@ 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);
 }
 
 static void qmi_deactivate_primary(struct ofono_gprs_context *gc,
@@ -319,18 +321,19 @@ static void qmi_deactivate_primary(struct ofono_gprs_context *gc,
 				ofono_gprs_context_cb_t cb, void *user_data)
 {
 	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct cb_data *cbd;
 	struct qmi_param *param;
 
 	DBG("cid %u", cid);
 
-	cbd->user = gc;
-
 	param = qmi_param_new_uint32(QMI_WDS_PARAM_PKT_HANDLE,
 						data->pkt_handle);
 	if (!param)
 		goto error;
 
+	cbd = cb_data_new(cb, user_data);
+	cbd->user = gc;
+
 	if (qmi_service_send(data->wds, QMI_WDS_STOP_NET, param,
 					stop_net_cb, cbd, g_free) > 0)
 		return;
@@ -338,9 +341,16 @@ 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)
@@ -471,6 +481,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

[PATCH QMI LTE v2 07/13] qmi: read_settings needs to call start network

[Jonas Bonn]

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

For LTE networks, a default bearer is automatically activated when
the modem registers to the network.  QMI modems, however, do not
automatically enable the network interface just because the bearer
exists; a call to "start network" needs to be made in order to
get the packet handle before get_settings will return any data and
the network interface can be configured.

This patch makes read_settings call "start network" in order to
enable the interface for the default bearer.  No new bearer will
be created with this call and the settings for the bearer will come
from the default profile, irregardless of what parameters are passed
to the "start network" method.
---
 drivers/qmimodem/gprs-context.c | 58 ++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index b72245c..1708ce0 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -150,31 +150,6 @@ done:
 	CALLBACK_WITH_SUCCESS(cb, cbd->data);
 }
 
-static void qmi_gprs_read_settings(struct ofono_gprs_context* gc,
-					unsigned int cid,
-					ofono_gprs_context_cb_t cb,
-					void *user_data)
-{
-	struct cb_data *cbd = cb_data_new(cb, user_data);
-	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
-
-	DBG("cid %u", cid);
-
-	data->active_context = cid;
-
-	cbd->user = gc;
-
-	if (qmi_service_send(data->wds, QMI_WDS_GET_SETTINGS, NULL,
-					get_settings_cb, cbd, g_free) > 0)
-		return;
-
-	data->active_context = 0;
-
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
-
-	g_free(cbd);
-}
-
 static void start_net_cb(struct qmi_result *result, void *user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -212,8 +187,6 @@ static void start_net_cb(struct qmi_result *result, void *user_data)
 
 	CALLBACK_WITH_SUCCESS(cb, cbd->data);
 
-	g_free(cbd);
-
 	return;
 
 error:
@@ -221,6 +194,37 @@ error:
 	CALLBACK_WITH_FAILURE(cb, cbd->data);
 }
 
+/*
+ * This function gets called for "automatic" contexts, those which are
+ * not activated via activate_primary.  For these, we will still need
+ * to call start_net in order to get the packet handle for the context.
+ * The process for automatic contexts is essentially identical to that
+ * for others.
+ */
+static void qmi_gprs_read_settings(struct ofono_gprs_context* gc,
+					unsigned int cid,
+					ofono_gprs_context_cb_t cb,
+					void *user_data)
+{
+	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
+	struct cb_data *cbd;
+
+	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,
+					start_net_cb, cbd, g_free) > 0)
+		return;
+
+	data->active_context = 0;
+
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+}
+
 static void qmi_activate_primary(struct ofono_gprs_context *gc,
 				const struct ofono_gprs_primary_context *ctx,
 				ofono_gprs_context_cb_t cb, void *user_data)
-- 
2.9.3

Re: [PATCH QMI LTE v2 07/13] qmi: read_settings needs to call start network

[Denis Kenzior]

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

Hi Jonas,

On 04/14/2017 04:36 PM, Jonas Bonn wrote:
> For LTE networks, a default bearer is automatically activated when
> the modem registers to the network.  QMI modems, however, do not
> automatically enable the network interface just because the bearer
> exists; a call to "start network" needs to be made in order to
> get the packet handle before get_settings will return any data and
> the network interface can be configured.
>
> This patch makes read_settings call "start network" in order to
> enable the interface for the default bearer.  No new bearer will
> be created with this call and the settings for the bearer will come
> from the default profile, irregardless of what parameters are passed
> to the "start network" method.
> ---
>  drivers/qmimodem/gprs-context.c | 58 ++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 27 deletions(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH QMI LTE v2 08/13] qmi: don't leak cbd and rely on destroy function

[Jonas Bonn]

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

---
 drivers/qmimodem/gprs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
index 05ad4bd..404c975 100644
--- a/drivers/qmimodem/gprs.c
+++ b/drivers/qmimodem/gprs.c
@@ -94,7 +94,7 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
 					ofono_gprs_cb_t cb, void *user_data)
 {
 	struct gprs_data *data = ofono_gprs_get_data(gprs);
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct cb_data *cbd;
 	struct qmi_param *param;
 	uint8_t action;
 
@@ -109,6 +109,7 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
 	if (!param)
 		goto error;
 
+	cbd = cb_data_new(cb, user_data);
 	if (qmi_service_send(data->nas, QMI_NAS_ATTACH_DETACH, param,
 					attach_detach_cb, cbd, g_free) > 0)
 		return;
@@ -117,8 +118,6 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
 
 error:
 	CALLBACK_WITH_FAILURE(cb, cbd->data);
-
-	g_free(cbd);
 }
 
 static void get_ss_info_cb(struct qmi_result *result, void *user_data)
-- 
2.9.3

Re: [PATCH QMI LTE v2 08/13] qmi: don't leak cbd and rely on destroy function

[Denis Kenzior]

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

Hi Jonas,

On 04/14/2017 04:36 PM, Jonas Bonn wrote:
> ---
>  drivers/qmimodem/gprs.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
> index 05ad4bd..404c975 100644
> --- a/drivers/qmimodem/gprs.c
> +++ b/drivers/qmimodem/gprs.c
> @@ -94,7 +94,7 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
>  					ofono_gprs_cb_t cb, void *user_data)
>  {
>  	struct gprs_data *data = ofono_gprs_get_data(gprs);
> -	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	struct cb_data *cbd;
>  	struct qmi_param *param;
>  	uint8_t action;
>
> @@ -109,6 +109,7 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
>  	if (!param)
>  		goto error;
>
> +	cbd = cb_data_new(cb, user_data);
>  	if (qmi_service_send(data->nas, QMI_NAS_ATTACH_DETACH, param,
>  					attach_detach_cb, cbd, g_free) > 0)
>  		return;

So cbd is not freed if qmi_service_send fails.  So you are introducing a 
leak on the fail path.

> @@ -117,8 +118,6 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
>
>  error:
>  	CALLBACK_WITH_FAILURE(cb, cbd->data);
> -
> -	g_free(cbd);
>  }
>
>  static void get_ss_info_cb(struct qmi_result *result, void *user_data)
>

This entire patch seems unnecessary?  The code looks fine as is.  Or am 
I missing something?

Regards,
-Denis

[PATCH QMI LTE v2 09/13] qmi: activate default bearer context for LTE networks

[Jonas Bonn]

[-- Attachment #1: Type: text/plain, Size: 6765 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, 96 insertions(+), 39 deletions(-)

diff --git a/drivers/qmimodem/gprs.c b/drivers/qmimodem/gprs.c
index 404c975..fdfd08c 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,33 @@
 #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;
 	uint16_t len;
+	int i;
 
 	DBG("");
 
@@ -47,25 +66,72 @@ 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;
+	/* FIXME: include ROAMING in status */
+	*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]);
+	}
 
 	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;
+
+	/* FIXME: what about ROAMING status? */
+
+	if (status == QMI_NAS_REGISTRATION_STATE_REGISTERED) {
+		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, "");
+		}
+	}
+
+	if (!data->attached)
+		status = NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
+
+	if (cb)
+		CALLBACK_WITH_SUCCESS(cb, status, cbd->data);
+	else
+		ofono_gprs_status_notify(gprs, status);
+
+	return;
 
-	ofono_gprs_status_notify(gprs, status);
+error:
+	if (cb)
+		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
 }
 
 static void attach_detach_cb(struct qmi_result *result, void *user_data)
@@ -100,6 +166,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
@@ -117,51 +185,36 @@ static void qmi_set_attached(struct ofono_gprs *gprs, int attached,
 	qmi_param_free(param);
 
 error:
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
-}
-
-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);
+	CALLBACK_WITH_FAILURE(cb, user_data);
 }
 
 static void qmi_attached_status(struct ofono_gprs *gprs,
 				ofono_gprs_status_cb_t cb, void *user_data)
 {
 	struct gprs_data *data = ofono_gprs_get_data(gprs);
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct cb_data *cbd;
 
 	DBG("");
 
+	if (!data->attached) {
+		CALLBACK_WITH_SUCCESS(cb,
+			NETWORK_REGISTRATION_STATUS_NOT_REGISTERED, user_data);
+	}
+
+	cbd = cb_data_new(cb, 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);
-
-	g_free(cbd);
+	CALLBACK_WITH_FAILURE(cb, -1, user_data);
 }
 
 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("");
 
@@ -177,11 +230,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

[PATCH QMI LTE v2 10/13] qmi: use destroy callback for activate_primary

[Jonas Bonn]

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

---
 drivers/qmimodem/gprs-context.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index 1708ce0..02b0671 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -230,15 +230,13 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc,
 				ofono_gprs_context_cb_t cb, void *user_data)
 {
 	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct cb_data *cbd;
 	struct qmi_param *param;
 	uint8_t ip_family;
 	uint8_t auth;
 
 	DBG("cid %u", ctx->cid);
 
-	cbd->user = gc;
-
 	data->active_context = ctx->cid;
 
 	switch (ctx->proto) {
@@ -284,6 +282,9 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc,
 		qmi_param_append(param, QMI_WDS_PARAM_PASSWORD,
 					strlen(ctx->password), ctx->password);
 
+	cbd = cb_data_new(cb, user_data);
+	cbd->user = gc;
+
 	if (qmi_service_send(data->wds, QMI_WDS_START_NET, param,
 					start_net_cb, cbd, g_free) > 0)
 		return;
@@ -293,9 +294,7 @@ static void qmi_activate_primary(struct ofono_gprs_context *gc,
 error:
 	data->active_context = 0;
 
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
-
-	g_free(cbd);
+	CALLBACK_WITH_FAILURE(cb, user_data);
 }
 
 static void stop_net_cb(struct qmi_result *result, void *user_data)
-- 
2.9.3

[PATCH QMI LTE v2 11/13] 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 02b0671..b4c2823 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;
@@ -370,9 +343,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

[PATCH QMI LTE v2 12/13] qmi: rely on destroy callback

[Jonas Bonn]

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

...and move creation of cb_data structures closer to their usage point
in order to prevent them from leaking when earlier code fails.
---
 drivers/qmimodem/network-registration.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/qmimodem/network-registration.c b/drivers/qmimodem/network-registration.c
index a3f9cf9..d0f4853 100644
--- a/drivers/qmimodem/network-registration.c
+++ b/drivers/qmimodem/network-registration.c
@@ -188,8 +188,6 @@ static void qmi_registration_status(struct ofono_netreg *netreg,
 		return;
 
 	CALLBACK_WITH_FAILURE(cb, -1, -1, -1, -1, cbd->data);
-
-	g_free(cbd);
 }
 
 static void qmi_current_operator(struct ofono_netreg *netreg,
@@ -300,8 +298,6 @@ static void qmi_list_operators(struct ofono_netreg *netreg,
 		return;
 
 	CALLBACK_WITH_FAILURE(cb, 0, NULL, cbd->data);
-
-	g_free(cbd);
 }
 
 static void register_net_cb(struct qmi_result *result, void *user_data)
@@ -330,7 +326,7 @@ static void qmi_register_auto(struct ofono_netreg *netreg,
 				ofono_netreg_register_cb_t cb, void *user_data)
 {
 	struct netreg_data *data = ofono_netreg_get_data(netreg);
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct cb_data *cbd;
 	struct qmi_param *param;
 
 	DBG("");
@@ -340,6 +336,8 @@ static void qmi_register_auto(struct ofono_netreg *netreg,
 	if (!param)
 		goto error;
 
+	cbd = cb_data_new(cb, user_data);
+
 	if (qmi_service_send(data->nas, QMI_NAS_REGISTER_NET, param,
 					register_net_cb, cbd, g_free) > 0)
 		return;
@@ -347,9 +345,7 @@ static void qmi_register_auto(struct ofono_netreg *netreg,
 	qmi_param_free(param);
 
 error:
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
-
-	g_free(cbd);
+	CALLBACK_WITH_FAILURE(cb, user_data);
 }
 
 static void qmi_register_manual(struct ofono_netreg *netreg,
@@ -357,7 +353,7 @@ static void qmi_register_manual(struct ofono_netreg *netreg,
 				ofono_netreg_register_cb_t cb, void *user_data)
 {
 	struct netreg_data *data = ofono_netreg_get_data(netreg);
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct cb_data *cbd;
 	struct qmi_nas_param_register_manual_info info;
 	struct qmi_param *param;
 
@@ -375,6 +371,8 @@ static void qmi_register_manual(struct ofono_netreg *netreg,
 	qmi_param_append(param, QMI_NAS_PARAM_REGISTER_MANUAL_INFO,
 						sizeof(info), &info);
 
+	cbd = cb_data_new(cb, user_data);
+
 	if (qmi_service_send(data->nas, QMI_NAS_REGISTER_NET, param,
 					register_net_cb, cbd, g_free) > 0)
 		return;
@@ -382,9 +380,7 @@ static void qmi_register_manual(struct ofono_netreg *netreg,
 	qmi_param_free(param);
 
 error:
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
-
-	g_free(cbd);
+	CALLBACK_WITH_FAILURE(cb, user_data);
 }
 
 static int dbm_to_strength(int8_t dbm)
@@ -446,8 +442,6 @@ static void qmi_signal_strength(struct ofono_netreg *netreg,
 		return;
 
 	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-
-	g_free(cbd);
 }
 
 static void event_notify(struct qmi_result *result, void *user_data)
-- 
2.9.3

[PATCH QMI LTE v2 13/13] 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 d0f4853..9bd88df 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);
@@ -492,6 +482,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 };
@@ -509,8 +500,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