[PATCH v2 1/3] gprs: Check for LTE in gprs_attached_update

[PATCH v2 1/3] gprs: Check for LTE in gprs_attached_update

[richard.rojfors]

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

From: Richard Röjfors <richard@puffinpack.se>

Since we have a different condition for the attach state
when running on LTE, we should consider it in gprs_attached_update.
Previously it's done in some instances. But for instance if
the driver got detached from GPRS but now running on LTE with a
context up, we would be deattached.
---
 src/gprs.c | 51 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index bba482cb..5829a577 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1571,13 +1571,34 @@ static void release_active_contexts(struct ofono_gprs *gprs)
 	}
 }
 
+static gboolean on_lte(struct ofono_gprs *gprs)
+{
+	if (ofono_netreg_get_technology(gprs->netreg) ==
+			ACCESS_TECHNOLOGY_EUTRAN && have_read_settings(gprs))
+		return TRUE;
+
+	return FALSE;
+}
+
 static void gprs_attached_update(struct ofono_gprs *gprs)
 {
 	ofono_bool_t attached;
+	int status = gprs->status;
 
-	attached = gprs->driver_attached &&
-		(gprs->status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
-			gprs->status == NETWORK_REGISTRATION_STATUS_ROAMING);
+	if (on_lte(gprs))
+		/*
+		 * For LTE we attached status reflects successful context
+		 * activation.
+		 * Since we in gprs_netreg_update not even try to attach
+		 * to GPRS if we are running on LTE, we can on some modems
+		 * expect the gprs status to be unknown. That must not
+		 * result in detaching...
+		 */
+		attached = have_active_contexts(gprs);
+	else
+		attached = gprs->driver_attached &&
+			(status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
+				status == NETWORK_REGISTRATION_STATUS_ROAMING);
 
 	if (attached == gprs->attached)
 		return;
@@ -1591,11 +1612,13 @@ static void gprs_attached_update(struct ofono_gprs *gprs)
 	if (attached == FALSE) {
 		release_active_contexts(gprs);
 		gprs->bearer = -1;
-	} else if (have_active_contexts(gprs) == TRUE) {
+	} else if (have_active_contexts(gprs) == TRUE && !on_lte(gprs)) {
 		/*
 		 * Some times the context activates after a detach event and
 		 * right before an attach. We close it to avoid unexpected open
 		 * contexts.
+		 * Skip that for LTE since the condition to be attached on LTE
+		 * is that a context gets activated
 		 */
 		release_active_contexts(gprs);
 		gprs->flags |= GPRS_FLAG_ATTACHED_UPDATE;
@@ -1660,15 +1683,6 @@ static void gprs_netreg_removed(struct ofono_gprs *gprs)
 	gprs_attached_update(gprs);
 }
 
-static gboolean on_lte(struct ofono_gprs *gprs)
-{
-	if (ofono_netreg_get_technology(gprs->netreg) ==
-			ACCESS_TECHNOLOGY_EUTRAN && have_read_settings(gprs))
-		return TRUE;
-
-	return FALSE;
-}
-
 static void gprs_netreg_update(struct ofono_gprs *gprs)
 {
 	ofono_bool_t attach;
@@ -2573,16 +2587,7 @@ void ofono_gprs_status_notify(struct ofono_gprs *gprs, int status)
 
 	if (status != NETWORK_REGISTRATION_STATUS_REGISTERED &&
 			status != NETWORK_REGISTRATION_STATUS_ROAMING) {
-		/*
-		 * For LTE we attached status reflects successful context
-		 * activation.
-		 * Since we in gprs_netreg_update not even try to attach
-		 * to GPRS if we are running on LTE, we can on some modems
-		 * expect the gprs status to be unknown. That must not
-		 * result in detaching...
-		 */
-		if (!on_lte(gprs))
-			gprs_attached_update(gprs);
+		gprs_attached_update(gprs);
 		return;
 	}
 
-- 
2.20.1

[PATCH v2 2/3] Revert "gprs: set driver_attached when activating automatic contexts"

[richard.rojfors]

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

From: Richard Röjfors <richard@puffinpack.se>

This reverts commit 0167c3339ca8f17a653592af995f439d24405de8.

Since gprs_attached_update now takes LTE in account this
should be reverted. This was actually a bad solution since
in case LTE was lost, and we should fall back on GPRS we
have flagged the driver to already being attached, which is not...
---
 src/gprs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/gprs.c b/src/gprs.c
index 5829a577..bd9b3680 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -940,7 +940,6 @@ 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.20.1

[PATCH v2 3/3] Revert "gprs: _cid_activated is an 'attaching' state"

[richard.rojfors]

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

From: Richard Röjfors <richard@puffinpack.se>

This reverts commit 1fd419e5b4b3a87673f8e0219edb0f3ed9fca774.

Since gprs_attached_update now guarantees that we
will not get attached without having a context activated
in LTE, this is not needed anymore. It also potentially
interferes in case the driver was actually attaching.
---
 src/gprs.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index bd9b3680..de172979 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -913,7 +913,6 @@ 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;
 
@@ -938,18 +937,11 @@ static void pri_read_settings_callback(const struct ofono_error *error,
 
 	value = pri_ctx->active;
 
-	gprs->flags &= ~GPRS_FLAG_ATTACHING;
-
-	gprs_set_attached_property(gprs, TRUE);
+	gprs_set_attached_property(pri_ctx->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,
@@ -2038,14 +2030,6 @@ void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
 					DBUS_TYPE_STRING, &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.20.1

Re: [PATCH v2 3/3] Revert "gprs: _cid_activated is an 'attaching' state"

[Jonas Bonn]

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

Hi Richard,

On 20/08/2019 17:56, richard.rojfors(a)gmail.com wrote:
> From: Richard Röjfors <richard@puffinpack.se>
> 
> This reverts commit 1fd419e5b4b3a87673f8e0219edb0f3ed9fca774.

This patch is fine and certainly necessary in the context of this 
series.  Posting it as a 'revert', however, is a bit unsavoury.

A reverted patch implies that it was somehow wrong or inappropriate. 
That's not the case here.  This code has become superfluous in the face 
of other changes that have been made in the tree.

Just squash these two revert patches into one "forward looking" patch. 
The fact that the change effectively constitutes a 'revert' is moot.

/Jonas

> 
> Since gprs_attached_update now guarantees that we
> will not get attached without having a context activated
> in LTE, this is not needed anymore. It also potentially
> interferes in case the driver was actually attaching.
> ---
>   src/gprs.c | 18 +-----------------
>   1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index bd9b3680..de172979 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -913,7 +913,6 @@ 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;
>   
> @@ -938,18 +937,11 @@ static void pri_read_settings_callback(const struct ofono_error *error,
>   
>   	value = pri_ctx->active;
>   
> -	gprs->flags &= ~GPRS_FLAG_ATTACHING;
> -
> -	gprs_set_attached_property(gprs, TRUE);
> +	gprs_set_attached_property(pri_ctx->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,
> @@ -2038,14 +2030,6 @@ void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
>   					DBUS_TYPE_STRING, &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);
>   }
>   
> 

Re: [PATCH v2 3/3] Revert "gprs: _cid_activated is an 'attaching' state"

[Richard =?unknown-8bit?q?R=C3=B6jfors?=]

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

Hi Jonas,

Den ons 21 aug. 2019 kl 08:01 skrev Jonas Bonn <jonas@norrbonn.se>:

> Hi Richard,
>
> On 20/08/2019 17:56, richard.rojfors(a)gmail.com wrote:
> > From: Richard Röjfors <richard@puffinpack.se>
> >
> > This reverts commit 1fd419e5b4b3a87673f8e0219edb0f3ed9fca774.
>
> This patch is fine and certainly necessary in the context of this
> series.  Posting it as a 'revert', however, is a bit unsavoury.
>
> A reverted patch implies that it was somehow wrong or inappropriate.
> That's not the case here.  This code has become superfluous in the face
> of other changes that have been made in the tree.
>

I agree, this did not introduce any obvious bug, but the other revert did.


>
> Just squash these two revert patches into one "forward looking" patch.
> The fact that the change effectively constitutes a 'revert' is moot.
>

For me it doesn't matter its the same effect.

Denis, whats your opinion?
I think you have the final say.

--Richard

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1704 bytes --]

Re: [PATCH v2 3/3] Revert "gprs: _cid_activated is an 'attaching' state"

[Denis Kenzior]

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

Hi Richard, Jonas,

On 8/21/19 1:01 AM, Jonas Bonn wrote:
> Hi Richard,
> 
> On 20/08/2019 17:56, richard.rojfors(a)gmail.com wrote:
>> From: Richard Röjfors <richard@puffinpack.se>
>>
>> This reverts commit 1fd419e5b4b3a87673f8e0219edb0f3ed9fca774.
> 
> This patch is fine and certainly necessary in the context of this 
> series.  Posting it as a 'revert', however, is a bit unsavoury.
> 
> A reverted patch implies that it was somehow wrong or inappropriate. 
> That's not the case here.  This code has become superfluous in the face 
> of other changes that have been made in the tree.
> 
> Just squash these two revert patches into one "forward looking" patch. 
> The fact that the change effectively constitutes a 'revert' is moot.
> 

So what I ended up doing was to squash patch 2 & 3 and reword the 
description a bit, dropping the 'revert' heading per your wishes.  Let 
me know if you're fine with that.

Otherwise, all 3 applied.

Regards,
-Denis