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@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