[RFC 0/3] Fix crash when network is lost

[RFC 0/3] Fix crash when network is lost

[Guillaume Zajac]

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

Hi,

Problem description:
Some Huawei modems are trying to keep PPP session with the network alive even if the network is lost.
Thus oFono PPP session with the modem is never ended. Currently when network is lost, oFono will release the active contexts into the core however the context is still active at driver level. If the connection with the network is established again and we try to enable a data call, we have a crash.

This issue is very tricky there might be multiple solutions, here is one I discussed with Denis yesterday.
A new driver entry is added to release context into the driver in shutting down the PPP session between oFono and the modem.
Then ofono_context_deactivated() is called to signal the context is inactive at driver level.
In the case we lost the network and we are reattaching to the network but the PPP session had no time to be released, there is a mechanism to wait for the previous context to be released and then signal modem is reattached to the network. ConnMan can do again a data call.

Guillaume Zajac (3):
  gprs-context: Add new driver entry
  gprs-context: Add new driver entry definition
  gprs: Release context in driver and core when network is lost

 drivers/atmodem/gprs-context.c |   11 +++++
 include/gprs-context.h         |    2 +
 src/gprs.c                     |   84 ++++++++++++++++++++++++++++++----------
 3 files changed, 76 insertions(+), 21 deletions(-)

-- 
1.7.5.4

[RFC 1/3] gprs-context: Add new driver entry

[Guillaume Zajac]

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

---
 include/gprs-context.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/gprs-context.h b/include/gprs-context.h
index 6cae9a2..165792f 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -71,6 +71,8 @@ struct ofono_gprs_context_driver {
 	void (*deactivate_primary)(struct ofono_gprs_context *gc,
 					unsigned int id,
 					ofono_gprs_context_cb_t cb, void *data);
+	void (*release_primary)(struct ofono_gprs_context *gc,
+					unsigned int id);
 };
 
 void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
-- 
1.7.5.4

[RFC 2/3] gprs-context: Add new driver entry definition

[Guillaume Zajac]

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

---
 drivers/atmodem/gprs-context.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index 16893ce..2c70bf3 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -291,6 +291,16 @@ static void at_gprs_deactivate_primary(struct ofono_gprs_context *gc,
 	g_at_ppp_shutdown(gcd->ppp);
 }
 
+static void at_gprs_release_primary(struct ofono_gprs_context *gc,
+					unsigned int cid)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+
+	DBG("cid %u", cid);
+
+	g_at_ppp_shutdown(gcd->ppp);
+}
+
 static void cgev_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs_context *gc = user_data;
@@ -380,6 +390,7 @@ static struct ofono_gprs_context_driver driver = {
 	.remove			= at_gprs_context_remove,
 	.activate_primary	= at_gprs_activate_primary,
 	.deactivate_primary	= at_gprs_deactivate_primary,
+	.release_primary	= at_gprs_release_primary,
 };
 
 void at_gprs_context_init(void)
-- 
1.7.5.4

[RFC 3/3] gprs: Release context in driver and core when network is lost

[Guillaume Zajac]

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

---
 src/gprs.c |   84 +++++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 63 insertions(+), 21 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 994607d..9ec0ca1 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -48,6 +48,7 @@
 
 #define GPRS_FLAG_ATTACHING 0x1
 #define GPRS_FLAG_RECHECK 0x2
+#define GPRS_FLAG_ATTACHED_UPDATE 0x4
 
 #define SETTINGS_STORE "gprs"
 #define SETTINGS_GROUP "Settings"
@@ -1440,6 +1441,44 @@ void ofono_gprs_resume_notify(struct ofono_gprs *gprs)
 	update_suspended_property(gprs, FALSE);
 }
 
+static gboolean check_active_contexts(struct ofono_gprs *gprs)
+{
+	GSList *l;
+	struct pri_context *ctx;
+
+	for (l = gprs->contexts; l; l = l->next) {
+		ctx = l->data;
+
+		if (ctx->active == TRUE)
+			return TRUE;
+	}
+
+	return FALSE;
+}
+
+static void release_active_contexts(struct ofono_gprs *gprs)
+{
+	GSList *l;
+	struct pri_context *ctx;
+
+	for (l = gprs->contexts; l; l = l->next) {
+		struct ofono_gprs_context *gc;
+
+		ctx = l->data;
+
+		if (ctx->active == FALSE)
+			continue;
+
+		/* This context is already being messed with */
+		if (ctx->pending)
+			continue;
+
+		gc = ctx->context_driver;
+
+		gc->driver->release_primary(gc, ctx->context.cid);
+	}
+}
+
 static void gprs_attached_update(struct ofono_gprs *gprs)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -1454,30 +1493,23 @@ static void gprs_attached_update(struct ofono_gprs *gprs)
 	if (attached == gprs->attached)
 		return;
 
-	gprs->attached = attached;
-
-	if (gprs->attached == FALSE) {
-		GSList *l;
-		struct pri_context *ctx;
-
-		for (l = gprs->contexts; l; l = l->next) {
-			ctx = l->data;
-
-			if (ctx->active == FALSE)
-				continue;
-
-			pri_reset_context_settings(ctx);
-			release_context(ctx);
-
-			value = FALSE;
-			ofono_dbus_signal_property_changed(conn, ctx->path,
-					OFONO_CONNECTION_CONTEXT_INTERFACE,
-					"Active", DBUS_TYPE_BOOLEAN, &value);
-		}
-
+	/*
+	 * If an active context is found, a PPP session might be still active
+	 * at driver level. "Attached" = TRUE property can't be signalled to
+	 * the applications registered on GPRS properties.
+	 * Active contexts have to be release at driver level.
+	 */
+	if (attached == FALSE) {
+		release_active_contexts(gprs);
 		gprs->bearer = -1;
+	} else if (check_active_contexts(gprs) == TRUE) {
+		gprs->flags |= GPRS_FLAG_ATTACHED_UPDATE;
+		release_active_contexts(gprs);
+		return;
 	}
 
+	gprs->attached = attached;
+
 	path = __ofono_atom_get_path(gprs->atom);
 	value = attached;
 	ofono_dbus_signal_property_changed(conn, path,
@@ -2266,6 +2298,16 @@ void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
 					OFONO_CONNECTION_CONTEXT_INTERFACE,
 					"Active", DBUS_TYPE_BOOLEAN, &value);
 	}
+
+	/*
+	 * If "Attached" property was about to be signalled as TRUE but there
+	 * were still active contexts, try again to signal "Attached" property
+	 * to registered applications after active contexts have been released.
+	 */
+	if (gc->gprs->flags & GPRS_FLAG_ATTACHED_UPDATE) {
+		gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
+		gprs_attached_update(gc->gprs);
+	}
 }
 
 int ofono_gprs_context_driver_register(
-- 
1.7.5.4

Re: [RFC 3/3] gprs: Release context in driver and core when network is lost

[Denis Kenzior]

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

Hi Guillaume,

On 05/31/2012 04:59 AM, Guillaume Zajac wrote:
> ---
>   src/gprs.c |   84 +++++++++++++++++++++++++++++++++++++++++++++---------------
>   1 files changed, 63 insertions(+), 21 deletions(-)
>
> diff --git a/src/gprs.c b/src/gprs.c
> index 994607d..9ec0ca1 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -48,6 +48,7 @@
>
>   #define GPRS_FLAG_ATTACHING 0x1
>   #define GPRS_FLAG_RECHECK 0x2
> +#define GPRS_FLAG_ATTACHED_UPDATE 0x4
>
>   #define SETTINGS_STORE "gprs"
>   #define SETTINGS_GROUP "Settings"
> @@ -1440,6 +1441,44 @@ void ofono_gprs_resume_notify(struct ofono_gprs *gprs)
>   	update_suspended_property(gprs, FALSE);
>   }
>
> +static gboolean check_active_contexts(struct ofono_gprs *gprs)

Name this have_active_contexts

> +{
> +	GSList *l;
> +	struct pri_context *ctx;
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		ctx = l->data;
> +
> +		if (ctx->active == TRUE)
> +			return TRUE;
> +	}
> +
> +	return FALSE;
> +}
> +
> +static void release_active_contexts(struct ofono_gprs *gprs)
> +{
> +	GSList *l;
> +	struct pri_context *ctx;
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		struct ofono_gprs_context *gc;
> +
> +		ctx = l->data;
> +
> +		if (ctx->active == FALSE)
> +			continue;
> +
> +		/* This context is already being messed with */
> +		if (ctx->pending)
> +			continue;
> +
> +		gc = ctx->context_driver;
> +
> +		gc->driver->release_primary(gc, ctx->context.cid);

You better check that this driver function is implemented

> +	}
> +}
> +
>   static void gprs_attached_update(struct ofono_gprs *gprs)
>   {
>   	DBusConnection *conn = ofono_dbus_get_connection();
> @@ -1454,30 +1493,23 @@ static void gprs_attached_update(struct ofono_gprs *gprs)
>   	if (attached == gprs->attached)
>   		return;
>
> -	gprs->attached = attached;
> -
> -	if (gprs->attached == FALSE) {
> -		GSList *l;
> -		struct pri_context *ctx;
> -
> -		for (l = gprs->contexts; l; l = l->next) {
> -			ctx = l->data;
> -
> -			if (ctx->active == FALSE)
> -				continue;
> -
> -			pri_reset_context_settings(ctx);
> -			release_context(ctx);
> -
> -			value = FALSE;
> -			ofono_dbus_signal_property_changed(conn, ctx->path,
> -					OFONO_CONNECTION_CONTEXT_INTERFACE,
> -					"Active", DBUS_TYPE_BOOLEAN,&value);
> -		}
> -
> +	/*
> +	 * If an active context is found, a PPP session might be still active
> +	 * at driver level. "Attached" = TRUE property can't be signalled to
> +	 * the applications registered on GPRS properties.
> +	 * Active contexts have to be release at driver level.
> +	 */
> +	if (attached == FALSE) {
> +		release_active_contexts(gprs);
>   		gprs->bearer = -1;
> +	} else if (check_active_contexts(gprs) == TRUE) {
> +		gprs->flags |= GPRS_FLAG_ATTACHED_UPDATE;
> +		release_active_contexts(gprs);

Why do we run the release operation here, wasn't it sufficient to run it 
once on the Attached goes Detached transition above?

> +		return;
>   	}
>
> +	gprs->attached = attached;
> +
>   	path = __ofono_atom_get_path(gprs->atom);
>   	value = attached;
>   	ofono_dbus_signal_property_changed(conn, path,
> @@ -2266,6 +2298,16 @@ void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
>   					OFONO_CONNECTION_CONTEXT_INTERFACE,
>   					"Active", DBUS_TYPE_BOOLEAN,&value);
>   	}
> +
> +	/*
> +	 * If "Attached" property was about to be signalled as TRUE but there
> +	 * were still active contexts, try again to signal "Attached" property
> +	 * to registered applications after active contexts have been released.
> +	 */
> +	if (gc->gprs->flags&  GPRS_FLAG_ATTACHED_UPDATE) {
> +		gc->gprs->flags&= ~GPRS_FLAG_ATTACHED_UPDATE;
> +		gprs_attached_update(gc->gprs);
> +	}
>   }
>
>   int ofono_gprs_context_driver_register(

Overall I like this approach, but lets get this tested really well.

Regards,
-Denis

Re: [RFC 1/3] gprs-context: Add new driver entry

[Denis Kenzior]

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

Hi Guillaume,

On 05/31/2012 04:59 AM, Guillaume Zajac wrote:
> ---
>   include/gprs-context.h |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/include/gprs-context.h b/include/gprs-context.h
> index 6cae9a2..165792f 100644
> --- a/include/gprs-context.h
> +++ b/include/gprs-context.h
> @@ -71,6 +71,8 @@ struct ofono_gprs_context_driver {
>   	void (*deactivate_primary)(struct ofono_gprs_context *gc,
>   					unsigned int id,
>   					ofono_gprs_context_cb_t cb, void *data);
> +	void (*release_primary)(struct ofono_gprs_context *gc,
> +					unsigned int id);

Please just call it detach_shutdown for now.

>   };
>
>   void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,

Regards,
-Denis

Re: [RFC 3/3] gprs: Release context in driver and core when network is lost

[Guillaume Zajac]

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

Hi Denis,

On 01/06/2012 00:45, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 05/31/2012 04:59 AM, Guillaume Zajac wrote:
>> ---
>>   src/gprs.c |   84 
>> +++++++++++++++++++++++++++++++++++++++++++++---------------
>>   1 files changed, 63 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/gprs.c b/src/gprs.c
>> index 994607d..9ec0ca1 100644
>> --- a/src/gprs.c
>> +++ b/src/gprs.c
>> @@ -48,6 +48,7 @@
>>
>>   #define GPRS_FLAG_ATTACHING 0x1
>>   #define GPRS_FLAG_RECHECK 0x2
>> +#define GPRS_FLAG_ATTACHED_UPDATE 0x4
>>
>>   #define SETTINGS_STORE "gprs"
>>   #define SETTINGS_GROUP "Settings"
>> @@ -1440,6 +1441,44 @@ void ofono_gprs_resume_notify(struct 
>> ofono_gprs *gprs)
>>       update_suspended_property(gprs, FALSE);
>>   }
>>
>> +static gboolean check_active_contexts(struct ofono_gprs *gprs)
>
> Name this have_active_contexts

Ok

>
>> +{
>> +    GSList *l;
>> +    struct pri_context *ctx;
>> +
>> +    for (l = gprs->contexts; l; l = l->next) {
>> +        ctx = l->data;
>> +
>> +        if (ctx->active == TRUE)
>> +            return TRUE;
>> +    }
>> +
>> +    return FALSE;
>> +}
>> +
>> +static void release_active_contexts(struct ofono_gprs *gprs)
>> +{
>> +    GSList *l;
>> +    struct pri_context *ctx;
>> +
>> +    for (l = gprs->contexts; l; l = l->next) {
>> +        struct ofono_gprs_context *gc;
>> +
>> +        ctx = l->data;
>> +
>> +        if (ctx->active == FALSE)
>> +            continue;
>> +
>> +        /* This context is already being messed with */
>> +        if (ctx->pending)
>> +            continue;
>> +
>> +        gc = ctx->context_driver;
>> +
>> +        gc->driver->release_primary(gc, ctx->context.cid);
>
> You better check that this driver function is implemented

Yes

>
>
>> +    }
>> +}
>> +
>>   static void gprs_attached_update(struct ofono_gprs *gprs)
>>   {
>>       DBusConnection *conn = ofono_dbus_get_connection();
>> @@ -1454,30 +1493,23 @@ static void gprs_attached_update(struct 
>> ofono_gprs *gprs)
>>       if (attached == gprs->attached)
>>           return;
>>
>> -    gprs->attached = attached;
>> -
>> -    if (gprs->attached == FALSE) {
>> -        GSList *l;
>> -        struct pri_context *ctx;
>> -
>> -        for (l = gprs->contexts; l; l = l->next) {
>> -            ctx = l->data;
>> -
>> -            if (ctx->active == FALSE)
>> -                continue;
>> -
>> -            pri_reset_context_settings(ctx);
>> -            release_context(ctx);
>> -
>> -            value = FALSE;
>> -            ofono_dbus_signal_property_changed(conn, ctx->path,
>> -                    OFONO_CONNECTION_CONTEXT_INTERFACE,
>> -                    "Active", DBUS_TYPE_BOOLEAN,&value);
>> -        }
>> -
>> +    /*
>> +     * If an active context is found, a PPP session might be still 
>> active
>> +     * at driver level. "Attached" = TRUE property can't be 
>> signalled to
>> +     * the applications registered on GPRS properties.
>> +     * Active contexts have to be release at driver level.
>> +     */
>> +    if (attached == FALSE) {
>> +        release_active_contexts(gprs);
>>           gprs->bearer = -1;
>> +    } else if (check_active_contexts(gprs) == TRUE) {
>> +        gprs->flags |= GPRS_FLAG_ATTACHED_UPDATE;
>> +        release_active_contexts(gprs);
>
> Why do we run the release operation here, wasn't it sufficient to run 
> it once on the Attached goes Detached transition above?

Yes you are right, if context is still active it must be being shut down.

>
>> +        return;
>>       }
>>
>> +    gprs->attached = attached;
>> +
>>       path = __ofono_atom_get_path(gprs->atom);
>>       value = attached;
>>       ofono_dbus_signal_property_changed(conn, path,
>> @@ -2266,6 +2298,16 @@ void ofono_gprs_context_deactivated(struct 
>> ofono_gprs_context *gc,
>>                       OFONO_CONNECTION_CONTEXT_INTERFACE,
>>                       "Active", DBUS_TYPE_BOOLEAN,&value);
>>       }
>> +
>> +    /*
>> +     * If "Attached" property was about to be signalled as TRUE but 
>> there
>> +     * were still active contexts, try again to signal "Attached" 
>> property
>> +     * to registered applications after active contexts have been 
>> released.
>> +     */
>> +    if (gc->gprs->flags&  GPRS_FLAG_ATTACHED_UPDATE) {
>> +        gc->gprs->flags&= ~GPRS_FLAG_ATTACHED_UPDATE;
>> +        gprs_attached_update(gc->gprs);
>> +    }
>>   }
>>
>>   int ofono_gprs_context_driver_register(
>
> Overall I like this approach, but lets get this tested really well.
>
> Regards,
> -Denis
>

Kind regards,
Guillaume