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