* [PATCH] gprs: lte: set attached after successful activation
@ 2016-11-22 16:48 Dragos Tatulea
2016-11-22 16:51 ` Denis Kenzior
0 siblings, 1 reply; 5+ messages in thread
From: Dragos Tatulea @ 2016-11-22 16:48 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]
Otherwise the attached state gets to be set before the actual LTE
automatic context is ready. This triggers a race between connman
and ofono: connman sees status attached before the context is active
so connman will try to activate another context with same apn and will
fail over and over again.
---
src/gprs.c | 50 +++++++++++++++++++++++++++-----------------------
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/src/gprs.c b/src/gprs.c
index 5f7620b..5015a2f 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -959,6 +959,24 @@ static void pri_deactivate_callback(const struct ofono_error *error, void *data)
}
}
+static void gprs_set_attached_property(struct ofono_gprs *gprs,
+ ofono_bool_t attached)
+{
+ const char *path;
+ DBusConnection *conn = ofono_dbus_get_connection();
+ dbus_bool_t value = attached;
+
+ if (gprs->attached == attached)
+ return;
+
+ gprs->attached = attached;
+
+ path = __ofono_atom_get_path(gprs->atom);
+ ofono_dbus_signal_property_changed(conn, path,
+ OFONO_CONNECTION_MANAGER_INTERFACE,
+ "Attached", DBUS_TYPE_BOOLEAN, &value);
+}
+
static void pri_read_settings_callback(const struct ofono_error *error,
void *data)
{
@@ -987,6 +1005,9 @@ static void pri_read_settings_callback(const struct ofono_error *error,
}
value = pri_ctx->active;
+
+ 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);
@@ -1596,24 +1617,6 @@ static void release_active_contexts(struct ofono_gprs *gprs)
}
}
-static void gprs_set_attached_property(struct ofono_gprs *gprs,
- ofono_bool_t attached)
-{
- const char *path;
- DBusConnection *conn = ofono_dbus_get_connection();
- dbus_bool_t value = attached;
-
- if (gprs->attached == attached)
- return;
-
- gprs->attached = attached;
-
- path = __ofono_atom_get_path(gprs->atom);
- ofono_dbus_signal_property_changed(conn, path,
- OFONO_CONNECTION_MANAGER_INTERFACE,
- "Attached", DBUS_TYPE_BOOLEAN, &value);
-}
-
static void gprs_attached_update(struct ofono_gprs *gprs)
{
ofono_bool_t attached;
@@ -1717,11 +1720,12 @@ static void gprs_netreg_update(struct ofono_gprs *gprs)
DBG("attach: %u, driver_attached: %u", attach, gprs->driver_attached);
if (ofono_netreg_get_technology(gprs->netreg) ==
- ACCESS_TECHNOLOGY_EUTRAN) {
- /* Ignore attach logic for LTE. There is no such concept. */
- gprs_set_attached_property(gprs, attach);
- return;
- }
+ ACCESS_TECHNOLOGY_EUTRAN)
+ /*
+ * For LTE we set attached status only on successful
+ * context activation.
+ */
+ return;
if (gprs->driver_attached == attach)
return;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] gprs: lte: set attached after successful activation
@ 2016-11-22 15:03 Dragos Tatulea
2016-11-22 16:37 ` Denis Kenzior
0 siblings, 1 reply; 5+ messages in thread
From: Dragos Tatulea @ 2016-11-22 15:03 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 3108 bytes --]
Otherwise the attached state gets to be set before the actual LTE
automatic context is ready. This triggers a race between connman
and ofono: connman sees status attached before the context is active
so connman will try to activate another context with same apn and will
fail over and over again.
---
src/gprs.c | 52 +++++++++++++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 25 deletions(-)
diff --git a/src/gprs.c b/src/gprs.c
index 5f7620b..c5fda35 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -959,6 +959,24 @@ static void pri_deactivate_callback(const struct ofono_error *error, void *data)
}
}
+static void gprs_set_attached_property(struct ofono_gprs *gprs,
+ ofono_bool_t attached)
+{
+ const char *path;
+ DBusConnection *conn = ofono_dbus_get_connection();
+ dbus_bool_t value = attached;
+
+ if (gprs->attached == attached)
+ return;
+
+ gprs->attached = attached;
+
+ path = __ofono_atom_get_path(gprs->atom);
+ ofono_dbus_signal_property_changed(conn, path,
+ OFONO_CONNECTION_MANAGER_INTERFACE,
+ "Attached", DBUS_TYPE_BOOLEAN, &value);
+}
+
static void pri_read_settings_callback(const struct ofono_error *error,
void *data)
{
@@ -987,6 +1005,9 @@ static void pri_read_settings_callback(const struct ofono_error *error,
}
value = pri_ctx->active;
+
+ 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);
@@ -1596,24 +1617,6 @@ static void release_active_contexts(struct ofono_gprs *gprs)
}
}
-static void gprs_set_attached_property(struct ofono_gprs *gprs,
- ofono_bool_t attached)
-{
- const char *path;
- DBusConnection *conn = ofono_dbus_get_connection();
- dbus_bool_t value = attached;
-
- if (gprs->attached == attached)
- return;
-
- gprs->attached = attached;
-
- path = __ofono_atom_get_path(gprs->atom);
- ofono_dbus_signal_property_changed(conn, path,
- OFONO_CONNECTION_MANAGER_INTERFACE,
- "Attached", DBUS_TYPE_BOOLEAN, &value);
-}
-
static void gprs_attached_update(struct ofono_gprs *gprs)
{
ofono_bool_t attached;
@@ -1644,8 +1647,6 @@ static void gprs_attached_update(struct ofono_gprs *gprs)
gprs->flags |= GPRS_FLAG_ATTACHED_UPDATE;
return;
}
-
- gprs_set_attached_property(gprs, attached);
}
static void registration_status_cb(const struct ofono_error *error,
@@ -1717,11 +1718,12 @@ static void gprs_netreg_update(struct ofono_gprs *gprs)
DBG("attach: %u, driver_attached: %u", attach, gprs->driver_attached);
if (ofono_netreg_get_technology(gprs->netreg) ==
- ACCESS_TECHNOLOGY_EUTRAN) {
- /* Ignore attach logic for LTE. There is no such concept. */
- gprs_set_attached_property(gprs, attach);
- return;
- }
+ ACCESS_TECHNOLOGY_EUTRAN)
+ /*
+ * For LTE we set attached status only on successful
+ * context activation.
+ */
+ return;
if (gprs->driver_attached == attach)
return;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] gprs: lte: set attached after successful activation
2016-11-22 15:03 Dragos Tatulea
@ 2016-11-22 16:37 ` Denis Kenzior
2016-11-22 16:43 ` Dragos Tatulea
0 siblings, 1 reply; 5+ messages in thread
From: Denis Kenzior @ 2016-11-22 16:37 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 3502 bytes --]
Hi Dragos,
On 11/22/2016 09:03 AM, Dragos Tatulea wrote:
> Otherwise the attached state gets to be set before the actual LTE
> automatic context is ready. This triggers a race between connman
> and ofono: connman sees status attached before the context is active
> so connman will try to activate another context with same apn and will
> fail over and over again.
> ---
> src/gprs.c | 52 +++++++++++++++++++++++++++-------------------------
> 1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/src/gprs.c b/src/gprs.c
> index 5f7620b..c5fda35 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -959,6 +959,24 @@ static void pri_deactivate_callback(const struct ofono_error *error, void *data)
> }
> }
>
> +static void gprs_set_attached_property(struct ofono_gprs *gprs,
> + ofono_bool_t attached)
> +{
> + const char *path;
> + DBusConnection *conn = ofono_dbus_get_connection();
> + dbus_bool_t value = attached;
> +
> + if (gprs->attached == attached)
> + return;
> +
> + gprs->attached = attached;
> +
> + path = __ofono_atom_get_path(gprs->atom);
> + ofono_dbus_signal_property_changed(conn, path,
> + OFONO_CONNECTION_MANAGER_INTERFACE,
> + "Attached", DBUS_TYPE_BOOLEAN, &value);
> +}
> +
> static void pri_read_settings_callback(const struct ofono_error *error,
> void *data)
> {
> @@ -987,6 +1005,9 @@ static void pri_read_settings_callback(const struct ofono_error *error,
> }
>
> value = pri_ctx->active;
> +
> + 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);
> @@ -1596,24 +1617,6 @@ static void release_active_contexts(struct ofono_gprs *gprs)
> }
> }
>
> -static void gprs_set_attached_property(struct ofono_gprs *gprs,
> - ofono_bool_t attached)
> -{
> - const char *path;
> - DBusConnection *conn = ofono_dbus_get_connection();
> - dbus_bool_t value = attached;
> -
> - if (gprs->attached == attached)
> - return;
> -
> - gprs->attached = attached;
> -
> - path = __ofono_atom_get_path(gprs->atom);
> - ofono_dbus_signal_property_changed(conn, path,
> - OFONO_CONNECTION_MANAGER_INTERFACE,
> - "Attached", DBUS_TYPE_BOOLEAN, &value);
> -}
> -
> static void gprs_attached_update(struct ofono_gprs *gprs)
> {
> ofono_bool_t attached;
> @@ -1644,8 +1647,6 @@ static void gprs_attached_update(struct ofono_gprs *gprs)
> gprs->flags |= GPRS_FLAG_ATTACHED_UPDATE;
> return;
> }
> -
> - gprs_set_attached_property(gprs, attached);
This patch mostly makes sense to me, but this part here seems dangerous.
Are you sure this is needed?
> }
>
> static void registration_status_cb(const struct ofono_error *error,
> @@ -1717,11 +1718,12 @@ static void gprs_netreg_update(struct ofono_gprs *gprs)
> DBG("attach: %u, driver_attached: %u", attach, gprs->driver_attached);
>
> if (ofono_netreg_get_technology(gprs->netreg) ==
> - ACCESS_TECHNOLOGY_EUTRAN) {
> - /* Ignore attach logic for LTE. There is no such concept. */
> - gprs_set_attached_property(gprs, attach);
> - return;
> - }
> + ACCESS_TECHNOLOGY_EUTRAN)
> + /*
> + * For LTE we set attached status only on successful
> + * context activation.
> + */
> + return;
>
> if (gprs->driver_attached == attach)
> return;
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] gprs: lte: set attached after successful activation
2016-11-22 16:37 ` Denis Kenzior
@ 2016-11-22 16:43 ` Dragos Tatulea
0 siblings, 0 replies; 5+ messages in thread
From: Dragos Tatulea @ 2016-11-22 16:43 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 3542 bytes --]
On Tue, Nov 22, 2016 at 5:37 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Dragos,
>
>
> On 11/22/2016 09:03 AM, Dragos Tatulea wrote:
>>
>> Otherwise the attached state gets to be set before the actual LTE
>> automatic context is ready. This triggers a race between connman
>> and ofono: connman sees status attached before the context is active
>> so connman will try to activate another context with same apn and will
>> fail over and over again.
>> ---
>> src/gprs.c | 52 +++++++++++++++++++++++++++-------------------------
>> 1 file changed, 27 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/gprs.c b/src/gprs.c
>> index 5f7620b..c5fda35 100644
>> --- a/src/gprs.c
>> +++ b/src/gprs.c
>> @@ -959,6 +959,24 @@ static void pri_deactivate_callback(const struct
>> ofono_error *error, void *data)
>> }
>> }
>>
>> +static void gprs_set_attached_property(struct ofono_gprs *gprs,
>> + ofono_bool_t attached)
>> +{
>> + const char *path;
>> + DBusConnection *conn = ofono_dbus_get_connection();
>> + dbus_bool_t value = attached;
>> +
>> + if (gprs->attached == attached)
>> + return;
>> +
>> + gprs->attached = attached;
>> +
>> + path = __ofono_atom_get_path(gprs->atom);
>> + ofono_dbus_signal_property_changed(conn, path,
>> + OFONO_CONNECTION_MANAGER_INTERFACE,
>> + "Attached", DBUS_TYPE_BOOLEAN, &value);
>> +}
>> +
>> static void pri_read_settings_callback(const struct ofono_error *error,
>> void *data)
>> {
>> @@ -987,6 +1005,9 @@ static void pri_read_settings_callback(const struct
>> ofono_error *error,
>> }
>>
>> value = pri_ctx->active;
>> +
>> + 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);
>> @@ -1596,24 +1617,6 @@ static void release_active_contexts(struct
>> ofono_gprs *gprs)
>> }
>> }
>>
>> -static void gprs_set_attached_property(struct ofono_gprs *gprs,
>> - ofono_bool_t attached)
>> -{
>> - const char *path;
>> - DBusConnection *conn = ofono_dbus_get_connection();
>> - dbus_bool_t value = attached;
>> -
>> - if (gprs->attached == attached)
>> - return;
>> -
>> - gprs->attached = attached;
>> -
>> - path = __ofono_atom_get_path(gprs->atom);
>> - ofono_dbus_signal_property_changed(conn, path,
>> - OFONO_CONNECTION_MANAGER_INTERFACE,
>> - "Attached", DBUS_TYPE_BOOLEAN, &value);
>> -}
>> -
>> static void gprs_attached_update(struct ofono_gprs *gprs)
>> {
>> ofono_bool_t attached;
>> @@ -1644,8 +1647,6 @@ static void gprs_attached_update(struct ofono_gprs
>> *gprs)
>> gprs->flags |= GPRS_FLAG_ATTACHED_UPDATE;
>> return;
>> }
>> -
>> - gprs_set_attached_property(gprs, attached);
>
>
> This patch mostly makes sense to me, but this part here seems dangerous.
> Are you sure this is needed?
>
Wow, thanks! I made a mistake when solving the merge error of
cherry-picking this patch from my branch. I'll send another version.
Thanks,
Dragos
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-22 16:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-22 16:48 [PATCH] gprs: lte: set attached after successful activation Dragos Tatulea
2016-11-22 16:51 ` Denis Kenzior
-- strict thread matches above, loose matches on Subject: below --
2016-11-22 15:03 Dragos Tatulea
2016-11-22 16:37 ` Denis Kenzior
2016-11-22 16:43 ` Dragos Tatulea
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.