[PATCH 0/9] allow automatic context activation

[PATCH 0/9] allow automatic context activation

[Dragos Tatulea]

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

LTE uses default bearer activation. Once register to the network is
received of which context has been activated.

This patchset takes action in the atmodem gprs driver when receiving
the activation event. It reads the cid and apn and sends it to the
gprs core. From there a matching (my apn) pri_context is
found/created and a gprs-context driver is assigned.

A read_settings operation has been added to the gprs-context driver
to read context ip configuration without activating the context.

The last patch in the patchset adds this support to the U-Blox
Toby L2 modem.

Implementation based mostly on ideas from Denis Kenzior.

Dragos Tatulea (9):
  idmap: add api for finding a certain id in map
  gprs-context.h: add op for reading context config
  gprs.h: automatic context configuration notifier
  gprs: pri_activate_callback: optional msg reply
  gprs: pri_set_apn: make reply msg optional
  gprs: custom cid for assign_context
  gprs: implement ofono_gprs_cid_activated
  atmodem: gprs: handle automatic context activation
  ubloxmodem: support automatic ctx activation

 drivers/atmodem/gprs.c            | 53 ++++++++++++++++++++++
 drivers/ubloxmodem/gprs-context.c | 39 ++++++++++++-----
 include/gprs-context.h            |  3 ++
 include/gprs.h                    |  3 ++
 src/gprs.c                        | 92 ++++++++++++++++++++++++++++++++++++---
 src/idmap.c                       | 13 ++++++
 src/idmap.h                       |  1 +
 7 files changed, 188 insertions(+), 16 deletions(-)

-- 
2.5.0

[PATCH 1/9] idmap: add api for finding a certain id in map

[Dragos Tatulea]

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

---
 src/idmap.c | 13 +++++++++++++
 src/idmap.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/src/idmap.c b/src/idmap.c
index c097eb4..3b72830 100644
--- a/src/idmap.c
+++ b/src/idmap.c
@@ -166,6 +166,19 @@ void idmap_take(struct idmap *idmap, unsigned int id)
 	idmap->bits[offset] |= 1UL << (bit % BITS_PER_LONG);
 }
 
+int idmap_find(struct idmap *idmap, unsigned int id)
+{
+	unsigned int bit = id - idmap->min;
+	unsigned int offset;
+
+	if (bit >= idmap->size)
+		return 0;
+
+	offset = bit / BITS_PER_LONG;
+	return (idmap->bits[offset] & (1UL << (bit % BITS_PER_LONG))) != 0;
+
+}
+
 /*
  * Allocate the next bit skipping the ids up to and including last.  If there
  * is no free ids until the max id is encountered, the counter is wrapped back
diff --git a/src/idmap.h b/src/idmap.h
index ebda177..97a6f04 100644
--- a/src/idmap.h
+++ b/src/idmap.h
@@ -25,6 +25,7 @@ struct idmap *idmap_new(unsigned int size);
 void idmap_free(struct idmap *idmap);
 void idmap_put(struct idmap *idmap, unsigned int id);
 void idmap_take(struct idmap *idmap, unsigned int id);
+int idmap_find(struct idmap *idmap, unsigned int id);
 unsigned int idmap_alloc(struct idmap *idmap);
 unsigned int idmap_alloc_next(struct idmap *idmap, unsigned int last);
 struct idmap *idmap_new_from_range(unsigned int min, unsigned int max);
-- 
2.5.0

[PATCH 2/9] gprs-context.h: add op for reading context config

[Dragos Tatulea]

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

This will be implemented by a gprs-context driver to support
automatic context activation. The gprs atom will call the driver
to read the ip configuration without activating the context.
---
 include/gprs-context.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/gprs-context.h b/include/gprs-context.h
index 0090cc4..b792fc6 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -79,6 +79,9 @@ struct ofono_gprs_context_driver {
 					ofono_gprs_context_cb_t cb, void *data);
 	void (*detach_shutdown)(struct ofono_gprs_context *gc,
 					unsigned int id);
+	void (*read_settings)(struct ofono_gprs_context *gc,
+				const struct ofono_gprs_primary_context *ctx,
+				ofono_gprs_context_cb_t cb, void *data);
 };
 
 void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
-- 
2.5.0

[PATCH 3/9] gprs.h: automatic context configuration notifier

[Dragos Tatulea]

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

Useful for LTE automatic bearer activation.

This is called from the gprs driver to let the gprs atom know that
a new context has been activated.
---
 include/gprs.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/gprs.h b/include/gprs.h
index 8d2a260..988d610 100644
--- a/include/gprs.h
+++ b/include/gprs.h
@@ -80,6 +80,9 @@ void ofono_gprs_set_cid_range(struct ofono_gprs *gprs,
 void ofono_gprs_add_context(struct ofono_gprs *gprs,
 				struct ofono_gprs_context *gc);
 
+void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
+				const char *apn);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.5.0

[PATCH 4/9] gprs: pri_activate_callback: optional msg reply

[Dragos Tatulea]

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

For automatic context activation there's no initial message to send a
reply to.
---
 src/gprs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 9192e68..233ab1a 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -887,8 +887,10 @@ static void pri_activate_callback(const struct ofono_error *error, void *data)
 	}
 
 	ctx->active = TRUE;
-	__ofono_dbus_pending_reply(&ctx->pending,
-				dbus_message_new_method_return(ctx->pending));
+
+	if (ctx->pending)
+		__ofono_dbus_pending_reply(&ctx->pending,
+				bus_message_new_method_return(ctx->pending));
 
 	if (gc->settings->interface != NULL) {
 		pri_ifupdown(gc->settings->interface, TRUE);
-- 
2.5.0

[PATCH 5/9] gprs: pri_set_apn: make reply msg optional

[Dragos Tatulea]

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

Automatic context activation will set the apn by itself. No
dbus message to reply to.
---
 src/gprs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/gprs.c b/src/gprs.c
index 233ab1a..664c377 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -957,7 +957,8 @@ static DBusMessage *pri_set_apn(struct pri_context *ctx, DBusConnection *conn,
 		storage_sync(ctx->gprs->imsi, SETTINGS_STORE, settings);
 	}
 
-	g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
+	if (msg)
+		g_dbus_send_reply(conn, msg, DBUS_TYPE_INVALID);
 
 	ofono_dbus_signal_property_changed(conn, ctx->path,
 					OFONO_CONNECTION_CONTEXT_INTERFACE,
-- 
2.5.0

[PATCH 6/9] gprs: custom cid for assign_context

[Dragos Tatulea]

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

It's optional though. If 0 a cid is picked up automatically from the
allowed range.
---
 src/gprs.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 664c377..e5e7abc 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -283,12 +283,17 @@ static unsigned int gprs_cid_alloc(struct ofono_gprs *gprs)
 	return idmap_alloc(gprs->cid_map);
 }
 
+static void gprs_cid_take(struct ofono_gprs *gprs, unsigned int id)
+{
+	idmap_take(gprs->cid_map, id);
+}
+
 static void gprs_cid_release(struct ofono_gprs *gprs, unsigned int id)
 {
 	idmap_put(gprs->cid_map, id);
 }
 
-static gboolean assign_context(struct pri_context *ctx)
+static gboolean assign_context(struct pri_context *ctx, int use_cid)
 {
 	struct idmap *cidmap = ctx->gprs->cid_map;
 	GSList *l;
@@ -296,7 +301,12 @@ static gboolean assign_context(struct pri_context *ctx)
 	if (cidmap == NULL)
 		return FALSE;
 
-	ctx->context.cid = gprs_cid_alloc(ctx->gprs);
+	if (use_cid > 0) {
+		gprs_cid_take(ctx->gprs, use_cid);
+		ctx->context.cid = use_cid;
+	} else
+		ctx->context.cid = gprs_cid_alloc(ctx->gprs);
+
 	if (ctx->context.cid == 0)
 		return FALSE;
 
@@ -1248,7 +1258,7 @@ static DBusMessage *pri_set_property(DBusConnection *conn,
 		if (ctx->gprs->flags & GPRS_FLAG_ATTACHING)
 			return __ofono_error_attach_in_progress(msg);
 
-		if (value && assign_context(ctx) == FALSE)
+		if (value && assign_context(ctx, 0) == FALSE)
 			return __ofono_error_not_implemented(msg);
 
 		gc = ctx->context_driver;
-- 
2.5.0

[PATCH 7/9] gprs: implement ofono_gprs_cid_activated

[Dragos Tatulea]

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

It works by looking for a context with the same APN and tries to use
that. Otherwise it will create it's own.

Then it assigns a gprs context driver and calls it's read_settings if
it exists.
---
 src/gprs.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/src/gprs.c b/src/gprs.c
index e5e7abc..74e9104 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -293,6 +293,11 @@ static void gprs_cid_release(struct ofono_gprs *gprs, unsigned int id)
 	idmap_put(gprs->cid_map, id);
 }
 
+static gboolean gprs_cid_taken(struct ofono_gprs *gprs, unsigned int id)
+{
+	return idmap_find(gprs->cid_map, id) != 0;
+}
+
 static gboolean assign_context(struct pri_context *ctx, int use_cid)
 {
 	struct idmap *cidmap = ctx->gprs->cid_map;
@@ -1842,6 +1847,25 @@ static void write_context_settings(struct ofono_gprs *gprs,
 	}
 }
 
+static struct pri_context *find_context_by_apn(struct ofono_gprs *gprs,
+					const char *apn)
+{
+	GSList *l;
+
+	for (l = gprs->contexts; l; l = l->next) {
+		struct pri_context *pri_ctx = l->data;
+
+		/* Looking only at prefix for the LTE case when a user APN is
+		 * web.provider.com but it apepars as
+		 * web.provider.com.mncX.mccY.gprs .
+		 */
+		if (g_str_has_prefix(apn, pri_ctx->context.apn))
+			return pri_ctx;
+	}
+
+	return NULL;
+}
+
 static struct pri_context *add_context(struct ofono_gprs *gprs,
 					const char *name,
 					enum ofono_gprs_context_type type)
@@ -1885,6 +1909,51 @@ static struct pri_context *add_context(struct ofono_gprs *gprs,
 	return context;
 }
 
+void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
+				const char *apn)
+{
+	struct pri_context *pri_ctx;
+	struct ofono_gprs_context *gc;
+
+	DBG("");
+
+	if (gprs_cid_taken(gprs, cid)) {
+		DBG("cid %u already activated", cid);
+		return;
+	}
+
+	pri_ctx = find_context_by_apn(gprs, apn);
+
+	if (!pri_ctx) {
+		pri_ctx = add_context(gprs, "auto",
+					OFONO_GPRS_CONTEXT_TYPE_INTERNET);
+		if (!pri_ctx) {
+			ofono_error("Can't find/create automatic context %d "
+					"with APN %s.", cid, apn);
+			return;
+		}
+	} else {
+		DBusConnection *conn = ofono_dbus_get_connection();
+		pri_set_apn(pri_ctx, conn, NULL, apn);
+	}
+
+	if (assign_context(pri_ctx, cid) == FALSE) {
+		ofono_warn("Can't assign context to driver for APN.");
+		release_context(pri_ctx);
+		return;
+	}
+
+	gc = pri_ctx->context_driver;
+
+	if (gc->driver->read_settings)
+		gc->driver->read_settings(gc, &pri_ctx->context,
+					     pri_activate_callback, pri_ctx);
+	else
+		ofono_warn("Context activated for driver that doesn't support "
+				"automatic context activation.");
+
+}
+
 static void send_context_added_signal(struct ofono_gprs *gprs,
 					struct pri_context *context,
 					DBusConnection *conn)
-- 
2.5.0

[PATCH 8/9] atmodem: gprs: handle automatic context activation

[Dragos Tatulea]

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

When the event comes, trigger CGCONT? to read the APN for the
activated cid and then call ogono_gprs_cid_activated to handle
the event.
---
 drivers/atmodem/gprs.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index 4505477..e85bbfb 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -49,6 +49,7 @@ static const char *none_prefix[] = { NULL };
 struct gprs_data {
 	GAtChat *chat;
 	unsigned int vendor;
+	unsigned int last_auto_context_id;
 };
 
 static void at_cgatt_cb(gboolean ok, GAtResult *result, gpointer user_data)
@@ -141,6 +142,49 @@ static void at_gprs_registration_status(struct ofono_gprs *gprs,
 	CALLBACK_WITH_FAILURE(cb, -1, data);
 }
 
+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;
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		ofono_warn("Can't read CGDCONT contexts.");
+		return;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, "+CGDCONT:")) {
+		int read_cid;
+
+		if (!g_at_result_iter_next_number(&iter, &read_cid))
+			break;
+
+		if (read_cid != activated_cid)
+			continue;
+
+		/* ignore protocol */
+		g_at_result_iter_skip_next(&iter);
+
+		g_at_result_iter_next_string(&iter, &apn);
+
+		break;
+	}
+
+	if (apn)
+		ofono_gprs_cid_activated(gprs, activated_cid, apn);
+	else
+		ofono_warn("cid %u: Received activated but no apn present",
+				activated_cid);
+}
+
+
 static void cgreg_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
@@ -157,6 +201,7 @@ static void cgreg_notify(GAtResult *result, gpointer user_data)
 static void cgev_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
+	struct gprs_data *gd = ofono_gprs_get_data(gprs);
 	GAtResultIter iter;
 	const char *event;
 
@@ -172,6 +217,14 @@ static void cgev_notify(GAtResult *result, gpointer user_data)
 			g_str_equal(event, "ME DETACH")) {
 		ofono_gprs_detached_notify(gprs);
 		return;
+	} else if (g_str_has_prefix(event, "ME PDN ACT")) {
+		char tmp[16] = {0};
+
+		sscanf(event, "%s %s %s %u",
+				tmp, tmp, tmp, &gd->last_auto_context_id);
+
+		g_at_chat_send(gd->chat, "AT+CGDCONT?", cgdcont_prefix,
+				at_cgdcont_read_cb, gprs, NULL);
 	}
 }
 
-- 
2.5.0

[PATCH 9/9] ubloxmodem: support automatic ctx activation

[Dragos Tatulea]

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

... by imlementing read_settings.
---
 drivers/ubloxmodem/gprs-context.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c
index 63f6ac2..25e0447 100644
--- a/drivers/ubloxmodem/gprs-context.c
+++ b/drivers/ubloxmodem/gprs-context.c
@@ -35,6 +35,7 @@
 #include <ofono/log.h>
 #include <ofono/modem.h>
 #include <ofono/gprs-context.h>
+#include <ofono/gprs.h>
 
 #include "gatchat.h"
 #include "gatresult.h"
@@ -181,6 +182,27 @@ static void ublox_read_settings(struct ofono_gprs_context *gc)
 		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
 }
 
+static void ublox_gprs_read_settings(struct ofono_gprs_context *gc,
+					const struct ofono_gprs_primary_context *ctx,
+					ofono_gprs_context_cb_t cb, void *data)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+
+	/* IPv6 support not implemented */
+	if (ctx->proto != OFONO_GPRS_PROTO_IP) {
+		CALLBACK_WITH_FAILURE(cb, data);
+		return;
+	}
+
+	DBG("cid %u", ctx->cid);
+
+	gcd->active_context = ctx->cid;
+	gcd->cb = cb;
+	gcd->cb_data = data;
+
+	ublox_read_settings(gc);
+}
+
 static void cgact_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs_context *gc = user_data;
@@ -370,6 +392,7 @@ static void cgev_notify(GAtResult *result, gpointer user_data)
 	GAtResultIter iter;
 	const char *event;
 	gint cid;
+	char tmp[16] = {0};
 
 	g_at_result_iter_init(&iter, result);
 
@@ -379,16 +402,11 @@ static void cgev_notify(GAtResult *result, gpointer user_data)
 	if (!g_at_result_iter_next_unquoted_string(&iter, &event))
 		return;
 
-	if (g_str_has_prefix(event, "NW PDN DEACT")) {
-		if (!g_at_result_iter_skip_next(&iter))
-			return;
-	} else if (g_str_has_prefix(event, "NW DEACT") == FALSE)
-		return;
-
-	if (!g_at_result_iter_skip_next(&iter))
-		return;
-
-	if (!g_at_result_iter_next_number(&iter, &cid))
+	if (g_str_has_prefix(event, "NW PDN DEACT"))
+		sscanf(event, "%s %s %s %u", tmp, tmp, tmp, &cid);
+	else if (g_str_has_prefix(event, "NW DEACT"))
+		sscanf(event, "%s %s %u", tmp, tmp, &cid);
+	else
 		return;
 
 	DBG("cid %d", cid);
@@ -440,6 +458,7 @@ static struct ofono_gprs_context_driver driver = {
 	.remove			= ublox_gprs_context_remove,
 	.activate_primary	= ublox_gprs_activate_primary,
 	.deactivate_primary	= ublox_gprs_deactivate_primary,
+	.read_settings		= ublox_gprs_read_settings,
 };
 
 void ublox_gprs_context_init(void)
-- 
2.5.0

Re: [PATCH 6/9] gprs: custom cid for assign_context

[Denis Kenzior]

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

Hi Dragos,

On 03/17/2016 12:02 PM, Dragos Tatulea wrote:
> It's optional though. If 0 a cid is picked up automatically from the
> allowed range.
> ---
>   src/gprs.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/9] gprs-context.h: add op for reading context config

[Denis Kenzior]

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

Hi Dragos,

On 03/17/2016 12:02 PM, Dragos Tatulea wrote:
> This will be implemented by a gprs-context driver to support
> automatic context activation. The gprs atom will call the driver
> to read the ip configuration without activating the context.
> ---
>   include/gprs-context.h | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/include/gprs-context.h b/include/gprs-context.h
> index 0090cc4..b792fc6 100644
> --- a/include/gprs-context.h
> +++ b/include/gprs-context.h
> @@ -79,6 +79,9 @@ struct ofono_gprs_context_driver {
>   					ofono_gprs_context_cb_t cb, void *data);
>   	void (*detach_shutdown)(struct ofono_gprs_context *gc,
>   					unsigned int id);
> +	void (*read_settings)(struct ofono_gprs_context *gc,
> +				const struct ofono_gprs_primary_context *ctx,
> +				ofono_gprs_context_cb_t cb, void *data);

Why does it take ofono_gprs_primary_context as input.  Wouldn't the cid 
be enough?

>   };
>
>   void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
>

Regards,
-Denis

Re: [PATCH 3/9] gprs.h: automatic context configuration notifier

[Denis Kenzior]

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

Hi Dragos,

On 03/17/2016 12:02 PM, Dragos Tatulea wrote:
> Useful for LTE automatic bearer activation.
>
> This is called from the gprs driver to let the gprs atom know that
> a new context has been activated.
> ---
>   include/gprs.h | 3 +++
>   1 file changed, 3 insertions(+)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 5/9] gprs: pri_set_apn: make reply msg optional

[Denis Kenzior]

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

Hi Dragos,

On 03/17/2016 12:02 PM, Dragos Tatulea wrote:
> Automatic context activation will set the apn by itself. No
> dbus message to reply to.
> ---
>   src/gprs.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 4/9] gprs: pri_activate_callback: optional msg reply

[Denis Kenzior]

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

Hi Dragos,

On 03/17/2016 12:02 PM, Dragos Tatulea wrote:
> For automatic context activation there's no initial message to send a
> reply to.
> ---
>   src/gprs.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/gprs.c b/src/gprs.c
> index 9192e68..233ab1a 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -887,8 +887,10 @@ static void pri_activate_callback(const struct ofono_error *error, void *data)
>   	}
>
>   	ctx->active = TRUE;
> -	__ofono_dbus_pending_reply(&ctx->pending,
> -				dbus_message_new_method_return(ctx->pending));
> +
> +	if (ctx->pending)
> +		__ofono_dbus_pending_reply(&ctx->pending,
> +				bus_message_new_method_return(ctx->pending));

So this is a bit tricky.  While this is strictly okay to do right now, 
since pending is only used for Activate/Deactivate, I'd prefer that we 
make a separate callback for the read_settings case.  Even if it does 
mean a bit of code duplication.

>
>   	if (gc->settings->interface != NULL) {
>   		pri_ifupdown(gc->settings->interface, TRUE);
>

Regards,
-Denis

Re: [PATCH 1/9] idmap: add api for finding a certain id in map

[Denis Kenzior]

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

Hi Dragos,

On 03/17/2016 12:02 PM, Dragos Tatulea wrote:
> ---
>   src/idmap.c | 13 +++++++++++++
>   src/idmap.h |  1 +
>   2 files changed, 14 insertions(+)
>
> diff --git a/src/idmap.c b/src/idmap.c
> index c097eb4..3b72830 100644
> --- a/src/idmap.c
> +++ b/src/idmap.c
> @@ -166,6 +166,19 @@ void idmap_take(struct idmap *idmap, unsigned int id)
>   	idmap->bits[offset] |= 1UL << (bit % BITS_PER_LONG);
>   }
>
> +int idmap_find(struct idmap *idmap, unsigned int id)
> +{
> +	unsigned int bit = id - idmap->min;
> +	unsigned int offset;
> +
> +	if (bit >= idmap->size)
> +		return 0;
> +
> +	offset = bit / BITS_PER_LONG;
> +	return (idmap->bits[offset] & (1UL << (bit % BITS_PER_LONG))) != 0;
> +

And another spurious whitespace :)

> +}
> +
>   /*
>    * Allocate the next bit skipping the ids up to and including last.  If there
>    * is no free ids until the max id is encountered, the counter is wrapped back
> diff --git a/src/idmap.h b/src/idmap.h
> index ebda177..97a6f04 100644
> --- a/src/idmap.h
> +++ b/src/idmap.h
> @@ -25,6 +25,7 @@ struct idmap *idmap_new(unsigned int size);
>   void idmap_free(struct idmap *idmap);
>   void idmap_put(struct idmap *idmap, unsigned int id);
>   void idmap_take(struct idmap *idmap, unsigned int id);
> +int idmap_find(struct idmap *idmap, unsigned int id);
>   unsigned int idmap_alloc(struct idmap *idmap);
>   unsigned int idmap_alloc_next(struct idmap *idmap, unsigned int last);
>   struct idmap *idmap_new_from_range(unsigned int min, unsigned int max);
>

Regards,
-Denis

Re: [PATCH 7/9] gprs: implement ofono_gprs_cid_activated

[Denis Kenzior]

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

On 03/17/2016 12:02 PM, Dragos Tatulea wrote:
> It works by looking for a context with the same APN and tries to use
> that. Otherwise it will create it's own.
>
> Then it assigns a gprs context driver and calls it's read_settings if
> it exists.
> ---
>   src/gprs.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 69 insertions(+)
>
> diff --git a/src/gprs.c b/src/gprs.c
> index e5e7abc..74e9104 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -293,6 +293,11 @@ static void gprs_cid_release(struct ofono_gprs *gprs, unsigned int id)
>   	idmap_put(gprs->cid_map, id);
>   }
>
> +static gboolean gprs_cid_taken(struct ofono_gprs *gprs, unsigned int id)
> +{
> +	return idmap_find(gprs->cid_map, id) != 0;
> +}
> +
>   static gboolean assign_context(struct pri_context *ctx, int use_cid)
>   {
>   	struct idmap *cidmap = ctx->gprs->cid_map;
> @@ -1842,6 +1847,25 @@ static void write_context_settings(struct ofono_gprs *gprs,
>   	}
>   }
>
> +static struct pri_context *find_context_by_apn(struct ofono_gprs *gprs,
> +					const char *apn)
> +{
> +	GSList *l;
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		struct pri_context *pri_ctx = l->data;
> +
> +		/* Looking only at prefix for the LTE case when a user APN is
> +		 * web.provider.com but it apepars as
> +		 * web.provider.com.mncX.mccY.gprs .
> +		 */
> +		if (g_str_has_prefix(apn, pri_ctx->context.apn))
> +			return pri_ctx;
> +	}
> +
> +	return NULL;
> +}
> +
>   static struct pri_context *add_context(struct ofono_gprs *gprs,
>   					const char *name,
>   					enum ofono_gprs_context_type type)
> @@ -1885,6 +1909,51 @@ static struct pri_context *add_context(struct ofono_gprs *gprs,
>   	return context;
>   }
>
> +void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
> +				const char *apn)
> +{
> +	struct pri_context *pri_ctx;
> +	struct ofono_gprs_context *gc;
> +
> +	DBG("");
> +
> +	if (gprs_cid_taken(gprs, cid)) {
> +		DBG("cid %u already activated", cid);
> +		return;
> +	}
> +
> +	pri_ctx = find_context_by_apn(gprs, apn);

Also, check the special case of settings provisioning failed.  It is 
signified by an a list with exactly 1 context.  The APN of that context 
will be NULL.  This is done in a couple of places by doing the following:

if (gprs->contexts == NULL) /* Automatic provisioning failed */
	add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);

So lets high-jack that special context in this case.

> +
> +	if (!pri_ctx) {
> +		pri_ctx = add_context(gprs, "auto",
shouldn't 'auto' be the APN?
> +					OFONO_GPRS_CONTEXT_TYPE_INTERNET);
> +		if (!pri_ctx) {
> +			ofono_error("Can't find/create automatic context %d "
> +					"with APN %s.", cid, apn);
> +			return;
> +		}
> +	} else {
> +		DBusConnection *conn = ofono_dbus_get_connection();
> +		pri_set_apn(pri_ctx, conn, NULL, apn);
> +	}
> +
> +	if (assign_context(pri_ctx, cid) == FALSE) {
> +		ofono_warn("Can't assign context to driver for APN.");
> +		release_context(pri_ctx);
> +		return;
> +	}
> +
> +	gc = pri_ctx->context_driver;
> +
> +	if (gc->driver->read_settings)
> +		gc->driver->read_settings(gc, &pri_ctx->context,
> +					     pri_activate_callback, pri_ctx);
> +	else
> +		ofono_warn("Context activated for driver that doesn't support "
> +				"automatic context activation.");

Lets check this part right away and not assign contexts or do anything 
if the read_settings method is not implemented

> +

And another spurious whitespace :)
> +}
> +
>   static void send_context_added_signal(struct ofono_gprs *gprs,
>   					struct pri_context *context,
>   					DBusConnection *conn)
>

Regards,
-Denis

Re: [PATCH 8/9] atmodem: gprs: handle automatic context activation

[Denis Kenzior]

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

Hi Dragos,

On 03/17/2016 12:02 PM, Dragos Tatulea wrote:
> When the event comes, trigger CGCONT? to read the APN for the
> activated cid and then call ogono_gprs_cid_activated to handle
> the event.
> ---
>   drivers/atmodem/gprs.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>
> diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
> index 4505477..e85bbfb 100644
> --- a/drivers/atmodem/gprs.c
> +++ b/drivers/atmodem/gprs.c
> @@ -49,6 +49,7 @@ static const char *none_prefix[] = { NULL };
>   struct gprs_data {
>   	GAtChat *chat;
>   	unsigned int vendor;
> +	unsigned int last_auto_context_id;
>   };
>
>   static void at_cgatt_cb(gboolean ok, GAtResult *result, gpointer user_data)
> @@ -141,6 +142,49 @@ static void at_gprs_registration_status(struct ofono_gprs *gprs,
>   	CALLBACK_WITH_FAILURE(cb, -1, data);
>   }
>
> +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;
> +
> +	DBG("ok %d", ok);
> +
> +	if (!ok) {
> +		ofono_warn("Can't read CGDCONT contexts.");
> +		return;
> +	}
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	while (g_at_result_iter_next(&iter, "+CGDCONT:")) {
> +		int read_cid;
> +
> +		if (!g_at_result_iter_next_number(&iter, &read_cid))
> +			break;
> +
> +		if (read_cid != activated_cid)
> +			continue;
> +
> +		/* ignore protocol */
> +		g_at_result_iter_skip_next(&iter);
> +
> +		g_at_result_iter_next_string(&iter, &apn);
> +
> +		break;
> +	}
> +
> +	if (apn)
> +		ofono_gprs_cid_activated(gprs, activated_cid, apn);
> +	else
> +		ofono_warn("cid %u: Received activated but no apn present",
> +				activated_cid);
> +}
> +
> +
>   static void cgreg_notify(GAtResult *result, gpointer user_data)
>   {
>   	struct ofono_gprs *gprs = user_data;
> @@ -157,6 +201,7 @@ static void cgreg_notify(GAtResult *result, gpointer user_data)
>   static void cgev_notify(GAtResult *result, gpointer user_data)
>   {
>   	struct ofono_gprs *gprs = user_data;
> +	struct gprs_data *gd = ofono_gprs_get_data(gprs);
>   	GAtResultIter iter;
>   	const char *event;
>
> @@ -172,6 +217,14 @@ static void cgev_notify(GAtResult *result, gpointer user_data)
>   			g_str_equal(event, "ME DETACH")) {
>   		ofono_gprs_detached_notify(gprs);
>   		return;
> +	} else if (g_str_has_prefix(event, "ME PDN ACT")) {
> +		char tmp[16] = {0};

This is not our initialization style.

> +
> +		sscanf(event, "%s %s %s %u",

man sscanf.  You can use %*s to skip assignment to tmp completely.

> +				tmp, tmp, tmp, &gd->last_auto_context_id);
> +
> +		g_at_chat_send(gd->chat, "AT+CGDCONT?", cgdcont_prefix,
> +				at_cgdcont_read_cb, gprs, NULL);
>   	}
>   }
>
>

Regards,
-Denis

[PATCH 3/9] gprs.h: automatic context configuration notifier

[Dragos Tatulea]

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

Useful for LTE automatic bearer activation.

This is called from the gprs driver to let the gprs atom know that
a new context has been activated.
---
 include/gprs.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/gprs.h b/include/gprs.h
index 8d2a260..988d610 100644
--- a/include/gprs.h
+++ b/include/gprs.h
@@ -80,6 +80,9 @@ void ofono_gprs_set_cid_range(struct ofono_gprs *gprs,
 void ofono_gprs_add_context(struct ofono_gprs *gprs,
 				struct ofono_gprs_context *gc);
 
+void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
+				const char *apn);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.5.0

[PATCH 3/9] gprs.h: automatic context configuration notifier

[Dragos Tatulea]

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

Useful for LTE automatic bearer activation.

This is called from the gprs driver to let the gprs atom know that
a new context has been activated.
---
 include/gprs.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/gprs.h b/include/gprs.h
index 8d2a260..988d610 100644
--- a/include/gprs.h
+++ b/include/gprs.h
@@ -80,6 +80,9 @@ void ofono_gprs_set_cid_range(struct ofono_gprs *gprs,
 void ofono_gprs_add_context(struct ofono_gprs *gprs,
 				struct ofono_gprs_context *gc);
 
+void ofono_gprs_cid_activated(struct ofono_gprs  *gprs, unsigned int cid,
+				const char *apn);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.5.0