[PATCH v2 1/3] gprs: Add list_active_contexts

[PATCH v2 1/3] gprs: Add list_active_contexts

[richard.rojfors]

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

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

This is useful for instance during startup where early
activated contexts can be listed at a suitable point in time.
---
 include/gprs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/gprs.h b/include/gprs.h
index 988d6102..20bdb7a4 100644
--- a/include/gprs.h
+++ b/include/gprs.h
@@ -45,6 +45,8 @@ struct ofono_gprs_driver {
 				ofono_gprs_cb_t cb, void *data);
 	void (*attached_status)(struct ofono_gprs *gprs,
 					ofono_gprs_status_cb_t cb, void *data);
+	void (*list_active_contexts)(struct ofono_gprs *gprs,
+					ofono_gprs_cb_t cb, void *data);
 };
 
 enum gprs_suspend_cause {
-- 
2.20.1

[PATCH v2 2/3] gprs: Ignore activated contexts during init, list them later

[richard.rojfors]

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

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

There is an issue if an context gets auto activated early,
then provisioning might not have run yet for instance,
so a "new" context is created, which might be duplicated
by a provisioning context later.
So ignore the activated contexts until gprs is ready,
then it calls the driver to list active contexts.
---
 src/gprs.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/gprs.c b/src/gprs.c
index f7a2b739..3c6a8deb 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1965,7 +1965,12 @@ void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
 	struct pri_context *pri_ctx;
 	struct ofono_gprs_context *gc;
 
-	DBG("");
+	DBG("cid %u", cid);
+
+	if (!__ofono_atom_get_registered(gprs->atom)) {
+		DBG("cid %u activated before atom registered", cid);
+		return;
+	}
 
 	if (l_uintset_contains(gprs->used_cids, cid)) {
 		DBG("cid %u already activated", cid);
@@ -3355,11 +3360,20 @@ remove:
 		storage_sync(imsi, SETTINGS_STORE, gprs->settings);
 }
 
+static void gprs_list_active_contexts_callback(const struct ofono_error *error,
+						void *data)
+{
+	(void)data;
+
+	DBG("error = %d", error->type);
+}
+
 static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
 	const char *path = __ofono_atom_get_path(gprs->atom);
+	const struct ofono_gprs_driver *driver = gprs->driver;
 
 	if (gprs->contexts == NULL) /* Automatic provisioning failed */
 		add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
@@ -3383,6 +3397,12 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
 					netreg_watch, gprs, NULL);
 
 	__ofono_atom_register(gprs->atom, gprs_unregister);
+
+	/* Find any context activated during init */
+	if (driver->list_active_contexts)
+		driver->list_active_contexts(gprs,
+					     gprs_list_active_contexts_callback,
+					     gprs);
 }
 
 static void spn_read_cb(const char *spn, const char *dc, void *data)
-- 
2.20.1

[PATCH v2 3/3] atmodem: Implement the list_activated_contexts callback

[richard.rojfors]

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

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

The callback calls cgact and cgdcont to get information
regarding any activate context.
---
 drivers/atmodem/gprs.c | 167 +++++++++++++++++++++++++++++++++--------
 1 file changed, 134 insertions(+), 33 deletions(-)

diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index 10108281..81e4c30f 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -29,6 +29,7 @@
 #include <stdio.h>
 #include <errno.h>
 
+#include <ell/ell.h>
 #include <glib.h>
 
 #include <ofono/log.h>
@@ -41,6 +42,8 @@
 #include "atmodem.h"
 #include "vendor.h"
 
+#define MAX_CONTEXTS 255
+
 static const char *cgreg_prefix[] = { "+CGREG:", NULL };
 static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
 static const char *cgact_prefix[] = { "+CGACT:", NULL };
@@ -54,6 +57,47 @@ struct gprs_data {
 	int attached;
 };
 
+struct list_contexts_data
+{
+	struct ofono_gprs *gprs;
+	void *cb;
+	void *data;
+	struct l_uintset *active_cids;
+	int ref_count;
+};
+
+static struct list_contexts_data *
+list_contexts_data_new(struct ofono_gprs *gprs, void *cb, void *data)
+{
+	struct list_contexts_data *ret;
+
+	ret = g_new0(struct list_contexts_data, 1);
+	ret->ref_count = 1;
+	ret->gprs = gprs;
+	ret->cb = cb;
+	ret->data = data;
+
+	return ret;
+}
+
+static struct list_contexts_data *
+list_contexts_data_ref(struct list_contexts_data *ld)
+{
+	ld->ref_count++;
+	return ld;
+}
+
+static void list_contexts_data_unref(gpointer user_data)
+{
+	struct list_contexts_data *ld = user_data;
+
+	if (--ld->ref_count)
+		return;
+
+	l_uintset_free(ld->active_cids);
+	g_free(ld);
+}
+
 static void at_cgatt_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -146,14 +190,43 @@ static void at_gprs_registration_status(struct ofono_gprs *gprs,
 	CALLBACK_WITH_FAILURE(cb, -1, data);
 }
 
+static void at_cgdcont_parse(struct ofono_gprs *gprs, GAtResult *result,
+				struct l_uintset *cids)
+{
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, "+CGDCONT:")) {
+		int read_cid;
+		const char *apn = NULL;
+
+		if (!g_at_result_iter_next_number(&iter, &read_cid))
+			break;
+
+		if (!l_uintset_contains(cids, read_cid))
+			continue;
+
+		/* ignore protocol */
+		g_at_result_iter_skip_next(&iter);
+
+		g_at_result_iter_next_string(&iter, &apn);
+
+		if (apn)
+			ofono_gprs_cid_activated(gprs, read_cid, apn);
+		else
+			ofono_warn("cid %d: Activated but no apn present",
+					read_cid);
+	}
+}
+
 static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
 				gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
 	struct gprs_data *gd = ofono_gprs_get_data(gprs);
 	int activated_cid = gd->last_auto_context_id;
-	const char *apn = NULL;
-	GAtResultIter iter;
+	struct l_uintset *cids;
 
 	DBG("ok %d", ok);
 
@@ -162,47 +235,52 @@ static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
 		return;
 	}
 
-	if (gd->last_auto_context_id == -1) {
+	if (activated_cid == -1) {
 		DBG("Context got deactivated while calling CGDCONT");
 		return;
 	}
 
-	g_at_result_iter_init(&iter, result);
+	cids = l_uintset_new(activated_cid);
 
-	while (g_at_result_iter_next(&iter, "+CGDCONT:")) {
-		int read_cid;
-
-		if (!g_at_result_iter_next_number(&iter, &read_cid))
-			break;
+	l_uintset_put(cids, activated_cid);
 
-		if (read_cid != activated_cid)
-			continue;
+	at_cgdcont_parse(gprs, result, cids);
 
-		/* ignore protocol */
-		g_at_result_iter_skip_next(&iter);
+	l_uintset_free(cids);
+}
 
-		g_at_result_iter_next_string(&iter, &apn);
+static void at_cgdcont_act_read_cb(gboolean ok, GAtResult *result,
+					gpointer user_data)
+{
+	struct list_contexts_data *ld = user_data;
+	ofono_gprs_cb_t cb = ld->cb;
+	struct ofono_error error;
 
-		break;
-	}
+	decode_at_error(&error, g_at_result_final_response(result));
 
-	if (apn)
-		ofono_gprs_cid_activated(gprs, activated_cid, apn);
+	if (!ok)
+		ofono_warn("Can't read CGDCONT context.");
 	else
-		ofono_warn("cid %u: Received activated but no apn present",
-				activated_cid);
+		at_cgdcont_parse(ld->gprs, result, ld->active_cids);
+
+	cb(&error, ld->data);
 }
 
 static void at_cgact_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
-	struct ofono_gprs *gprs = user_data;
-	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+	struct list_contexts_data *ld = user_data;
+	struct gprs_data *gd = ofono_gprs_get_data(ld->gprs);
+	ofono_gprs_cb_t cb = ld->cb;
+	struct ofono_error error;
 	GAtResultIter iter;
 
-	DBG("ok %d", ok);
+	decode_at_error(&error, g_at_result_final_response(result));
 
 	if (!ok) {
 		ofono_warn("Can't read CGACT contexts.");
+
+		cb(&error, ld->data);
+
 		return;
 	}
 
@@ -222,19 +300,45 @@ static void at_cgact_cb(gboolean ok, GAtResult *result, gpointer user_data)
 			continue;
 
 		/* Flag this as auto context as it was obviously active */
-		if (gd->last_auto_context_id == 0)
+		if (gd->last_auto_context_id == -1)
 			gd->last_auto_context_id = read_cid;
 
-		if (read_cid != gd->last_auto_context_id)
-			continue;
+		if (!ld->active_cids)
+			ld->active_cids = l_uintset_new(MAX_CONTEXTS);
 
-		g_at_chat_send(gd->chat, "AT+CGDCONT?", cgdcont_prefix,
-				at_cgdcont_read_cb, gprs, NULL);
+		l_uintset_put(ld->active_cids, read_cid);
+	}
 
-		break;
+	if (ld->active_cids != NULL) {
+		if (g_at_chat_send(gd->chat, "AT+CGDCONT?", cgdcont_prefix,
+					at_cgdcont_act_read_cb, ld,
+					list_contexts_data_unref)) {
+			list_contexts_data_ref(ld);
+			return;
+		}
+
+		CALLBACK_WITH_FAILURE(cb, ld->data);
+	} else {
+		/* No active contexts found */
+		cb(&error, ld->data);
 	}
 }
 
+static void at_gprs_list_active_contexts(struct ofono_gprs *gprs,
+						ofono_gprs_cb_t cb, void *data)
+{
+	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+	struct list_contexts_data *ld = list_contexts_data_new(gprs, cb, data);
+
+	if (g_at_chat_send(gd->chat, "AT+CGACT?", cgact_prefix,
+				at_cgact_cb, ld, list_contexts_data_unref))
+		return;
+
+	list_contexts_data_unref(ld);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
 static void cgreg_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
@@ -538,10 +642,6 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
 		break;
 	}
 
-	/* Check if there is any already activated contexts at init */
-	g_at_chat_send(gd->chat, "AT+CGACT?", cgact_prefix,
-			at_cgact_cb, gprs, NULL);
-
 	ofono_gprs_register(gprs);
 }
 
@@ -705,6 +805,7 @@ static const struct ofono_gprs_driver driver = {
 	.remove			= at_gprs_remove,
 	.set_attached		= at_gprs_set_attached,
 	.attached_status	= at_gprs_registration_status,
+	.list_active_contexts	= at_gprs_list_active_contexts,
 };
 
 void at_gprs_init(void)
-- 
2.20.1

Re: [PATCH v2 1/3] gprs: Add list_active_contexts

[Denis Kenzior]

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

Hi Richard,

On 8/12/19 4:31 PM, richard.rojfors(a)gmail.com wrote:
> From: Richard Röjfors <richard@puffinpack.se>
> 
> This is useful for instance during startup where early
> activated contexts can be listed at a suitable point in time.
> ---
>   include/gprs.h | 2 ++
>   1 file changed, 2 insertions(+)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH v2 2/3] gprs: Ignore activated contexts during init, list them later

[Denis Kenzior]

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

Hi Richard,

On 8/12/19 4:31 PM, richard.rojfors(a)gmail.com wrote:
> From: Richard Röjfors <richard@puffinpack.se>
> 
> There is an issue if an context gets auto activated early,
> then provisioning might not have run yet for instance,
> so a "new" context is created, which might be duplicated
> by a provisioning context later.
> So ignore the activated contexts until gprs is ready,
> then it calls the driver to list active contexts.
> ---
>   src/gprs.c | 22 +++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
> 

<snip>

>   
> +static void gprs_list_active_contexts_callback(const struct ofono_error *error,
> +						void *data)
> +{
> +	(void)data;
> +

Squashed this pointless line and...

> +	DBG("error = %d", error->type);
> +}
> +

applied, thanks.

Regards,
-Denis

Re: [PATCH v2 3/3] atmodem: Implement the list_activated_contexts callback

[Denis Kenzior]

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

Hi Richard,

On 8/12/19 4:31 PM, richard.rojfors(a)gmail.com wrote:
> From: Richard Röjfors <richard@puffinpack.se>
> 
> The callback calls cgact and cgdcont to get information
> regarding any activate context.
> ---
>   drivers/atmodem/gprs.c | 167 +++++++++++++++++++++++++++++++++--------
>   1 file changed, 134 insertions(+), 33 deletions(-)
> 

Applied, thanks.

Regards,
-Denis