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

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

[richard.rojfors]

[-- Attachment #1: Type: text/plain, Size: 3451 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 | 52 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index bba482cb..94d13f82 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,14 @@ 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 +1684,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 +2588,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 2/4] gprs: Always call gprs_attached_update on context deactivation.

[richard.rojfors]

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

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

Since its registered context is a condition for being attached
when running LTE, we might need to update the attached state
when a context is deactivated. Otherwise we might remain
attached, and connection managers (at least connman) start
to hammer us to connect a context, eventhough it happens
automatically in LTE.
---
 src/gprs.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 94d13f82..cb31e2d2 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -2738,11 +2738,12 @@ void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
 	 * 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.
+	 *
+	 * ... or in the case of LTE where the attached state were a registered
+	 * context is a condition for being attached
 	 */
-	if (gc->gprs->flags & GPRS_FLAG_ATTACHED_UPDATE) {
-		gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
-		gprs_attached_update(gc->gprs);
-	}
+	gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
+	gprs_attached_update(gc->gprs);
 }
 
 int ofono_gprs_context_driver_register(
-- 
2.20.1

[PATCH 3/4] 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 cb31e2d2..eb6cd30b 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 4/4] 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 eb6cd30b..6bdc125d 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,
@@ -2039,14 +2031,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 2/4] gprs: Always call gprs_attached_update on context deactivation.

[Denis Kenzior]

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

Hi Richard,

On 8/14/19 1:24 PM, richard.rojfors(a)gmail.com wrote:
> From: Richard Röjfors <richard@puffinpack.se>
> 
> Since its registered context is a condition for being attached
> when running LTE, we might need to update the attached state
> when a context is deactivated. Otherwise we might remain

Well, by definition if we're on LTE then we're attached.  So I'm not 
quite sure what you mean here?

> attached, and connection managers (at least connman) start
> to hammer us to connect a context, eventhough it happens
> automatically in LTE.

What happens automatically?  Yes, it is true that you need a default 
context to attach to LTE.  So activating a context is basically a part 
of the LTE attach procedure.

But that default context does not need to be an internet context (e.g. 
Verizon uses (or used to) use an ims context for this). So ConnMan still 
might need to activate an Internet context even if we're on LTE.

I'm also not sure that the heuristic of 0 active contexts means not 
attached is a good one.  Shouldn't you be using +CREG or +CEREG status 
for this?

> ---
>   src/gprs.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 94d13f82..cb31e2d2 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -2738,11 +2738,12 @@ void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
>   	 * 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.
> +	 *
> +	 * ... or in the case of LTE where the attached state were a registered
> +	 * context is a condition for being attached
>   	 */
> -	if (gc->gprs->flags & GPRS_FLAG_ATTACHED_UPDATE) {
> -		gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
> -		gprs_attached_update(gc->gprs);
> -	}
> +	gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
> +	gprs_attached_update(gc->gprs);
>   }
>   
>   int ofono_gprs_context_driver_register(
> 

Regards,
-Denis

Re: [PATCH 1/4] gprs: Check for LTE in gprs_attached_update

[Denis Kenzior]

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

Hi Richard,

On 8/14/19 1:24 PM, richard.rojfors(a)gmail.com wrote:
> 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.

So are you talking about a hand-over? Do we have logs to help visualize 
this?

> ---
>   src/gprs.c | 52 +++++++++++++++++++++++++++++-----------------------
>   1 file changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index bba482cb..94d13f82 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);

On LTE Attached should just be always True.  Someone can correct me if 
I'm wrong, but a default context is a pre-requisite for E-UTRAN attachment.

> +	else
> +		attached = gprs->driver_attached &&
> +			(status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
> +				status == NETWORK_REGISTRATION_STATUS_ROAMING);
>   
>   	if (attached == gprs->attached)
>   		return;
> @@ -1591,11 +1612,14 @@ 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)) {

Not our style.  See item M4.  Might want to see if it fits on the 
previous line.

>   		/*
>   		 * 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 +1684,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 +2588,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;
>   	}
>   
> 

Otherwise I'm fine moving the on_lte logic into gprs_attached_update 
itself...

Regards,
-Denis

Re: [PATCH 1/4] gprs: Check for LTE in gprs_attached_update

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

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

Hi Denis,

Den tors 15 aug. 2019 kl 22:47 skrev Denis Kenzior <denkenz@gmail.com>:

> Hi Richard,
>
> On 8/14/19 1:24 PM, richard.rojfors(a)gmail.com wrote:
> > 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.
>
> So are you talking about a hand-over? Do we have logs to help visualize
> this?
>

Sorry I have no logs.
I think an example would be;
Let say we are registered on non-LTE, and GPRS do attach.
Then we flip to LTE and if GPRS gets de-attached. We would brutally
exit the attached state, since LTE is currently not considered in that code
path.


>
> > ---
> >   src/gprs.c | 52 +++++++++++++++++++++++++++++-----------------------
> >   1 file changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/gprs.c b/src/gprs.c
> > index bba482cb..94d13f82 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);
>
> On LTE Attached should just be always True.  Someone can correct me if
> I'm wrong, but a default context is a pre-requisite for E-UTRAN attachment.
>

I guess this is about race conditions, since we read back settings of the
context,
so in theory I think we might not consider the context available yet.
Should be
a very short period of time.
In netreg status we do not immediately change to attached in case of
LTE, we require the context settings to have been read first.


>
> > +     else
> > +             attached = gprs->driver_attached &&
> > +                     (status == NETWORK_REGISTRATION_STATUS_REGISTERED
> ||
> > +                             status ==
> NETWORK_REGISTRATION_STATUS_ROAMING);
> >
> >       if (attached == gprs->attached)
> >               return;
> > @@ -1591,11 +1612,14 @@ 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)) {
>
> Not our style.  See item M4.  Might want to see if it fits on the
> previous line.
>

Will updat!


>
> >               /*
> >                * 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 +1684,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 +2588,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;
> >       }
> >
> >
>
> Otherwise I'm fine moving the on_lte logic into gprs_attached_update
> itself...
>
> Regards,
> -Denis
>

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

Re: [PATCH 2/4] gprs: Always call gprs_attached_update on context deactivation.

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

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

Hi Denis,

Den tors 15 aug. 2019 kl 22:41 skrev Denis Kenzior <denkenz@gmail.com>:

> Hi Richard,
>
> On 8/14/19 1:24 PM, richard.rojfors(a)gmail.com wrote:
> > From: Richard Röjfors <richard@puffinpack.se>
> >
> > Since its registered context is a condition for being attached
> > when running LTE, we might need to update the attached state
> > when a context is deactivated. Otherwise we might remain
>
> Well, by definition if we're on LTE then we're attached.  So I'm not
> quite sure what you mean here?
>

OK, maybe this is overkill. And about the order things are happening in
reporting from the modem.


>
> > attached, and connection managers (at least connman) start
> > to hammer us to connect a context, eventhough it happens
> > automatically in LTE.
>
> What happens automatically?  Yes, it is true that you need a default
> context to attach to LTE.  So activating a context is basically a part
> of the LTE attach procedure.
>
> But that default context does not need to be an internet context (e.g.
> Verizon uses (or used to) use an ims context for this). So ConnMan still
> might need to activate an Internet context even if we're on LTE.
>

To do that wouldn't we need to be able to flag attached even without
registered context for LTE?
Or will the modem not attach to 4G until the context is configured?
I can not test that case with my modems.


> I'm also not sure that the heuristic of 0 active contexts means not
> attached is a good one.  Shouldn't you be using +CREG or +CEREG status
> for this?
>

As you say that is probably more correct :)
First I wanted to get things running with the current solution which uses
an active
context as condition.


>
> > ---
> >   src/gprs.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/gprs.c b/src/gprs.c
> > index 94d13f82..cb31e2d2 100644
> > --- a/src/gprs.c
> > +++ b/src/gprs.c
> > @@ -2738,11 +2738,12 @@ void ofono_gprs_context_deactivated(struct
> ofono_gprs_context *gc,
> >        * 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.
> > +      *
> > +      * ... or in the case of LTE where the attached state were a
> registered
> > +      * context is a condition for being attached
> >        */
> > -     if (gc->gprs->flags & GPRS_FLAG_ATTACHED_UPDATE) {
> > -             gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
> > -             gprs_attached_update(gc->gprs);
> > -     }
> > +     gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
> > +     gprs_attached_update(gc->gprs);
> >   }
> >
> >   int ofono_gprs_context_driver_register(
> >
>
> Regards,
> -Denis
>

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

Re: [PATCH 2/4] gprs: Always call gprs_attached_update on context deactivation.

[Denis Kenzior]

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

Hi Richard,

>      > attached, and connection managers (at least connman) start
>      > to hammer us to connect a context, eventhough it happens
>      > automatically in LTE.
> 
>     What happens automatically?  Yes, it is true that you need a default
>     context to attach to LTE.  So activating a context is basically a part
>     of the LTE attach procedure.
> 
>     But that default context does not need to be an internet context (e.g.
>     Verizon uses (or used to) use an ims context for this). So ConnMan
>     still
>     might need to activate an Internet context even if we're on LTE.
> 
> 
> To do that wouldn't we need to be able to flag attached even without
> registered context for LTE?
> Or will the modem not attach to 4G until the context is configured?
> I can not test that case with my modems.

It sort of depends on the configuration.  But basically we will not get 
on LTE without a default bearer.  The default bearer configuration can 
come from the modem itself, or actually from the network.  That default 
bearer is essentially an active context.  This is why gprs.c goes to the 
trouble of matching it against existing contexts, but if it isn't found, 
then adds a new one (which is supposed to be transient/temporary).

Typically, on a well-configured system, you'll have the default bearer 
setup via LongTermEvolution.DefaultAccessPointName and also provisioned 
in the provisioning database.  But as I mentioned earlier, the default 
context doesn't have to be an internet one.

Regards,
-Denis

> 
> 
>     I'm also not sure that the heuristic of 0 active contexts means not
>     attached is a good one.  Shouldn't you be using +CREG or +CEREG status
>     for this?
> 
> 
> As you say that is probably more correct :)
> First I wanted to get things running with the current solution which 
> uses an active
> context as condition.
> 
> 
>      > ---
>      >   src/gprs.c | 9 +++++----
>      >   1 file changed, 5 insertions(+), 4 deletions(-)
>      >
>      > diff --git a/src/gprs.c b/src/gprs.c
>      > index 94d13f82..cb31e2d2 100644
>      > --- a/src/gprs.c
>      > +++ b/src/gprs.c
>      > @@ -2738,11 +2738,12 @@ void
>     ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
>      >        * 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.
>      > +      *
>      > +      * ... or in the case of LTE where the attached state were
>     a registered
>      > +      * context is a condition for being attached
>      >        */
>      > -     if (gc->gprs->flags & GPRS_FLAG_ATTACHED_UPDATE) {
>      > -             gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
>      > -             gprs_attached_update(gc->gprs);
>      > -     }
>      > +     gc->gprs->flags &= ~GPRS_FLAG_ATTACHED_UPDATE;
>      > +     gprs_attached_update(gc->gprs);
>      >   }
>      >
>      >   int ofono_gprs_context_driver_register(
>      >
> 
>     Regards,
>     -Denis
> 

Re: [PATCH 1/4] gprs: Check for LTE in gprs_attached_update

[Denis Kenzior]

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

Hi Richard,

On 8/16/19 5:44 AM, Richard Röjfors wrote:
> Hi Denis,
> 
> Den tors 15 aug. 2019 kl 22:47 skrev Denis Kenzior <denkenz@gmail.com 
> <mailto:denkenz@gmail.com>>:
> 
>     Hi Richard,
> 
>     On 8/14/19 1:24 PM, richard.rojfors(a)gmail.com
>     <mailto:richard.rojfors@gmail.com> wrote:
>      > From: Richard Röjfors <richard@puffinpack.se
>     <mailto: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.
> 
>     So are you talking about a hand-over? Do we have logs to help visualize
>     this?
> 
> 
> Sorry I have no logs.
> I think an example would be;
> Let say we are registered on non-LTE, and GPRS do attach.
> Then we flip to LTE and if GPRS gets de-attached. We would brutally
> exit the attached state, since LTE is currently not considered in that 
> code path.

Okay, so handover.  It would be helpful to have logs to help visualize 
what is happening.

> 
> 
>      > ---
>      >   src/gprs.c | 52
>     +++++++++++++++++++++++++++++-----------------------
>      >   1 file changed, 29 insertions(+), 23 deletions(-)
>      >
>      > diff --git a/src/gprs.c b/src/gprs.c
>      > index bba482cb..94d13f82 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);
> 
>     On LTE Attached should just be always True.  Someone can correct me if
>     I'm wrong, but a default context is a pre-requisite for E-UTRAN
>     attachment.
> 
> 
> I guess this is about race conditions, since we read back settings of 
> the context,
> so in theory I think we might not consider the context available yet. 
> Should be
> a very short period of time.
> In netreg status we do not immediately change to attached in case of
> LTE, we require the context settings to have been read first.

I think it makes sense to signal Attached right away in the case that we 
know we are on LTE.  But I can accept your argument of not signaling 
Attached until the first context is active.  That is likely more driven 
by ConMan not being updated for LTE, which likely won't work in the end.

Regards,
-Denis

Re: [PATCH 1/4] gprs: Check for LTE in gprs_attached_update

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

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

Hi Denis,

Den fre 16 aug. 2019 kl 17:55 skrev Denis Kenzior <denkenz@gmail.com>:

> Hi Richard,
>
> On 8/16/19 5:44 AM, Richard Röjfors wrote:
> > Hi Denis,
> >
> > Den tors 15 aug. 2019 kl 22:47 skrev Denis Kenzior <denkenz@gmail.com
> > <mailto:denkenz@gmail.com>>:
> >
> >     Hi Richard,
> >
> >     On 8/14/19 1:24 PM, richard.rojfors(a)gmail.com
> >     <mailto:richard.rojfors@gmail.com> wrote:
> >      > From: Richard Röjfors <richard@puffinpack.se
> >     <mailto: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.
> >
> >     So are you talking about a hand-over? Do we have logs to help
> visualize
> >     this?
> >
> >
> > Sorry I have no logs.
> > I think an example would be;
> > Let say we are registered on non-LTE, and GPRS do attach.
> > Then we flip to LTE and if GPRS gets de-attached. We would brutally
> > exit the attached state, since LTE is currently not considered in that
> > code path.
>
> Okay, so handover.  It would be helpful to have logs to help visualize
> what is happening.
>

As I said, I'm afraid I have no logs for this. I tried to visualize how it
could happen.

But anyway I think it makes more sense to "calculate" the attached state in
one place,
(gprs_attached_update) and one of the places this gets called from is when
GPRS detaches.


>
> >
> >
> >      > ---
> >      >   src/gprs.c | 52
> >     +++++++++++++++++++++++++++++-----------------------
> >      >   1 file changed, 29 insertions(+), 23 deletions(-)
> >      >
> >      > diff --git a/src/gprs.c b/src/gprs.c
> >      > index bba482cb..94d13f82 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);
> >
> >     On LTE Attached should just be always True.  Someone can correct me
> if
> >     I'm wrong, but a default context is a pre-requisite for E-UTRAN
> >     attachment.
> >
> >
> > I guess this is about race conditions, since we read back settings of
> > the context,
> > so in theory I think we might not consider the context available yet.
> > Should be
> > a very short period of time.
> > In netreg status we do not immediately change to attached in case of
> > LTE, we require the context settings to have been read first.
>
> I think it makes sense to signal Attached right away in the case that we
> know we are on LTE.  But I can accept your argument of not signaling
> Attached until the first context is active.  That is likely more driven
> by ConMan not being updated for LTE, which likely won't work in the end.
>

And I would like to add, that this how things are today, its nothing I have
or
inteded to change.
I just want to make sure we don't get deattached
because we miss to check for LTE in some places in the code.

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

Re: [PATCH 2/4] gprs: Always call gprs_attached_update on context deactivation.

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

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

Hi Denis,

Den fre 16 aug. 2019 kl 17:42 skrev Denis Kenzior <denkenz@gmail.com>:

> Hi Richard,
>
> >      > attached, and connection managers (at least connman) start
> >      > to hammer us to connect a context, eventhough it happens
> >      > automatically in LTE.
> >
> >     What happens automatically?  Yes, it is true that you need a default
> >     context to attach to LTE.  So activating a context is basically a
> part
> >     of the LTE attach procedure.
> >
> >     But that default context does not need to be an internet context
> (e.g.
> >     Verizon uses (or used to) use an ims context for this). So ConnMan
> >     still
> >     might need to activate an Internet context even if we're on LTE.
> >
> >
> > To do that wouldn't we need to be able to flag attached even without
> > registered context for LTE?
> > Or will the modem not attach to 4G until the context is configured?
> > I can not test that case with my modems.
>
> It sort of depends on the configuration.  But basically we will not get
> on LTE without a default bearer.  The default bearer configuration can
> come from the modem itself, or actually from the network.  That default
> bearer is essentially an active context.  This is why gprs.c goes to the
> trouble of matching it against existing contexts, but if it isn't found,
> then adds a new one (which is supposed to be transient/temporary).
>
> Typically, on a well-configured system, you'll have the default bearer
> setup via LongTermEvolution.DefaultAccessPointName and also provisioned
> in the provisioning database.  But as I mentioned earlier, the default
> context doesn't have to be an internet one.
>

For the modem I use, it would be a bit messy to update the default APN on
the fly,
because it requires to turn off the radio to change that. We have written
it into
non volatile memory beforehand.

But anyway, you have convinced me so I think we can skip this patch,
I guess its just the matter of the order things happen and the time span
will be very short.

--Richard

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

Re: [PATCH 1/4] gprs: Check for LTE in gprs_attached_update

[Denis Kenzior]

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

Hi Richard,

> As I said, I'm afraid I have no logs for this. I tried to visualize how 
> it could happen.

Sorry for the repeat.  Just take it as a strong hint for the "next time" :)

> 
> But anyway I think it makes more sense to "calculate" the attached state 
> in one place,
> (gprs_attached_update) and one of the places this gets called from is 
> when GPRS detaches.

Care to resend a cleaned up patch? I'm fine taking at least 1/4.

Regards,
-Denis

Re: [PATCH 1/4] gprs: Check for LTE in gprs_attached_update

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

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

Hi Denis,

Den mån 19 aug. 2019 kl 21:48 skrev Denis Kenzior <denkenz@gmail.com>:

> Hi Richard,
>
> > As I said, I'm afraid I have no logs for this. I tried to visualize how
> > it could happen.
>
> Sorry for the repeat.  Just take it as a strong hint for the "next time" :)
>

I got the hint ;-)


>
> >
> > But anyway I think it makes more sense to "calculate" the attached state
> > in one place,
> > (gprs_attached_update) and one of the places this gets called from is
> > when GPRS detaches.
>
> Care to resend a cleaned up patch? I'm fine taking at least 1/4.
>
> Will do. I think we can skip 2/4. But I think 3 and 4/4 should be applied
too. But we can start with the first.

--Richard

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