[PATCH 0/4] bugfixes + revectored isimodem GPRS context driver

[PATCH 0/4] bugfixes + revectored isimodem GPRS context driver

[Mika Liljeberg]

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

Hi All,

The first two patches are bugfixes. The rest adapts the isimodem
GPRS context driver to the current multiple PDP context support
in oFono core.

Br,

	MikaL


[PATCH 1/4] gisi: fix crash bug in g_isi_remove_subscription
[PATCH 2/4] gisi: return NULL instead of asserting
[PATCH 3/4] isimodem: revector GPRS context driver
[PATCH 4/4] isigen: make number of PDP contexts configurable

 drivers/isimodem/gprs-context.c |  280 +++++++++++++++++----------------------
 gisi/client.c                   |    7 +-
 gisi/pep.c                      |    6 +-
 plugins/isigen.c                |   23 +++-
 plugins/udev.c                  |    6 +-
 5 files changed, 152 insertions(+), 170 deletions(-)

[PATCH 1/4] gisi: fix crash bug in g_isi_remove_subscription

[Mika Liljeberg]

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

---
 gisi/client.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gisi/client.c b/gisi/client.c
index 8ab3dc9..8e41331 100644
--- a/gisi/client.c
+++ b/gisi/client.c
@@ -744,16 +744,19 @@ int g_isi_subscribe(GIsiClient *client, uint8_t type,
  */
 void g_isi_remove_subscription(GIsiClient *client, uint8_t res, uint8_t type)
 {
+	void *ret;
 	GIsiIndication *ind;
 	unsigned int id = (res << 8) | type;
 
 	if (!client)
 		return;
 
-	ind = tdelete(&id, &client->inds.subs, g_isi_cmp);
-	if (!ind)
+	ret = tfind(&id, &client->inds.subs, g_isi_cmp);
+	if (!ret)
 		return;
 
+	ind = *(GIsiIndication **)ret;
+	tdelete(ind, &client->inds.subs, g_isi_cmp);
 	client->inds.count--;
 	g_free(ind);
 }
-- 
1.7.0.4

[PATCH 2/4] gisi: return NULL instead of asserting

[Mika Liljeberg]

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

---
 gisi/pep.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gisi/pep.c b/gisi/pep.c
index f1ff589..bea1902 100644
--- a/gisi/pep.c
+++ b/gisi/pep.c
@@ -148,6 +148,8 @@ unsigned g_isi_pep_get_ifindex(const GIsiPEP *pep)
 
 char *g_isi_pep_get_ifname(const GIsiPEP *pep, char *ifname)
 {
-	unsigned ifi = g_isi_pep_get_ifindex(pep);
-	return if_indextoname(ifi, ifname);
+	if (pep->gprs_fd == -1)
+		return NULL;
+
+	return if_indextoname(g_isi_pep_get_ifindex(pep), ifname);
 }
-- 
1.7.0.4

[PATCH 3/4] isimodem: revector GPRS context driver

[Mika Liljeberg]

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

---
 drivers/isimodem/gprs-context.c |  280 +++++++++++++++++----------------------
 1 files changed, 121 insertions(+), 159 deletions(-)

diff --git a/drivers/isimodem/gprs-context.c b/drivers/isimodem/gprs-context.c
index 47e858a..62f7ac2 100644
--- a/drivers/isimodem/gprs-context.c
+++ b/drivers/isimodem/gprs-context.c
@@ -50,22 +50,20 @@
 #include "debug.h"
 
 #define STATIC_IP_NETMASK "255.255.255.255"
+#define ACTIVATE_TIMEOUT	(6 * 30)	/* 6 * T3380 */
+#define DEACTIVATE_TIMEOUT	(6 * 8)		/* 6 * T3390 */
 
 #define INVALID_ID (0xff)
 # if (INVALID_ID < GPDS_MAX_CONTEXT_COUNT)
 #   error Uho! This should not happen!
 #endif
 
-struct gprs_context_data {
+struct context_data {
 	GIsiClient *client;
 	GIsiModem *idx;
 	uint16_t gpds;	/* GPDS object handle */
-	GSList *contexts;
-};
-
-struct context_data {
 	unsigned cid;	/* oFono core context ID */
-	struct ofono_gprs_context *driver;
+	struct ofono_gprs_context *context;
 	union {
 		ofono_gprs_context_up_cb_t up_cb;
 		ofono_gprs_context_cb_t down_cb;
@@ -74,6 +72,8 @@ struct context_data {
 
 	GIsiPEP *pep;
 	GIsiPipe *pipe;
+	guint activate_timeout;
+	guint deactivate_timeout;
 
 	char apn[GPDS_MAX_APN_STRING_LENGTH + 1];
 	char username[GPDS_MAX_USERNAME_LENGTH + 1];
@@ -83,40 +83,24 @@ struct context_data {
 	uint8_t type;
 };
 
-static struct context_data *find_context_by_cid(GSList *contexts,
-						unsigned int cid)
-{
-	GSList *m = NULL;
-
-	for (m = contexts; m; m = m->next) {
-		struct context_data *cd = m->data;
-
-		if (cd->cid == cid)
-			return cd;
-	}
-	return NULL;
-}
-
-static struct context_data *find_context_by_handle(GSList *contexts,
-							uint8_t handle)
-{
-	GSList *m = NULL;
-
-	for (m = contexts; m; m = m->next) {
-		struct context_data *cd = m->data;
-
-		if (cd->handle == handle)
-			return cd;
-	}
-	return NULL;
-}
-
-static void destroy_context(struct context_data *cd)
+static void reset_context(struct context_data *cd)
 {
 	if (!cd)
 		return;
 
-	DBG("destroying %p (cid=%u)", cd, cd->cid);
+	g_isi_remove_subscription(cd->client, PN_GPDS,
+				GPDS_CONTEXT_ACTIVATE_IND);
+	g_isi_remove_subscription(cd->client,
+				PN_GPDS, GPDS_CONTEXT_ACTIVATE_FAIL_IND);
+	g_isi_remove_subscription(cd->client,
+				PN_GPDS, GPDS_CONTEXT_DEACTIVATE_IND);
+	g_isi_commit_subscriptions(cd->client);
+
+	if (cd->activate_timeout)
+		g_source_remove(cd->activate_timeout);
+
+	if (cd->deactivate_timeout)
+		g_source_remove(cd->deactivate_timeout);
 
 	if (cd->pipe)
 		g_isi_pipe_destroy(cd->pipe);
@@ -124,34 +108,48 @@ static void destroy_context(struct context_data *cd)
 	if (cd->pep)
 		g_isi_pep_destroy(cd->pep);
 
-	g_free(cd);
+	cd->activate_timeout = 0;
+	cd->deactivate_timeout = 0;
+	cd->pep = NULL;
+	cd->pipe = NULL;
+	cd->handle = INVALID_ID;
 }
 
 static gboolean gprs_up_fail(struct context_data *cd)
 {
-	struct ofono_gprs_context *gc = cd->driver;
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-
 	CALLBACK_WITH_FAILURE(cd->up_cb, NULL, 0, NULL, NULL, NULL, NULL,
 				cd->data);
 
-	gcd->contexts = g_slist_remove(gcd->contexts, cd);
-	destroy_context(cd);
+	reset_context(cd);
 	return TRUE;
 }
 
 static gboolean gprs_down_fail(struct context_data *cd)
 {
-	struct ofono_gprs_context *gc = cd->driver;
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-
 	CALLBACK_WITH_FAILURE(cd->down_cb, cd->data);
 
-	gcd->contexts = g_slist_remove(gcd->contexts, cd);
-	destroy_context(cd);
+	reset_context(cd);
 	return TRUE;
 }
 
+static gboolean gprs_up_timeout(gpointer data)
+{
+	struct context_data *cd = data;
+
+	cd->activate_timeout = 0;
+	gprs_up_fail(cd);
+	return FALSE;
+}
+
+static gboolean gprs_down_timeout(gpointer data)
+{
+	struct context_data *cd = data;
+
+	cd->deactivate_timeout = 0;
+	gprs_down_fail(cd);
+	return FALSE;
+}
+
 static gboolean check_resp(GIsiClient *client,
 				const uint8_t *restrict msg, size_t len,
 				uint_fast8_t cmd, struct context_data *cd)
@@ -191,12 +189,29 @@ static gboolean check_resp(GIsiClient *client,
 	return TRUE;
 }
 
+static void deactivate_ind_cb(GIsiClient *client,
+				const void *restrict data, size_t len,
+				uint16_t object, void *opaque)
+{
+	struct context_data *cd = opaque;
+	const unsigned char *msg = data;
+
+	if (!msg || len < 3 || msg[0] != GPDS_CONTEXT_DEACTIVATE_IND ||
+		msg[1] != cd->handle)
+		return;
+
+	DBG("context deactivated: %s (0x%02"PRIx8")",
+		gpds_isi_cause_name(msg[3]), msg[3]);
+
+	ofono_gprs_context_deactivated(cd->context, cd->cid);
+	reset_context(cd);
+}
+
 static void activate_ind_cb(GIsiClient *client,
 				const void *restrict data, size_t len,
 				uint16_t object, void *opaque)
 {
-	struct gprs_context_data *gcd = opaque;
-	struct context_data *cd;
+	struct context_data *cd = opaque;
 
 	const unsigned char *msg = data;
 	GIsiSubBlockIter iter;
@@ -207,15 +222,10 @@ static void activate_ind_cb(GIsiClient *client,
 	char *sdns = NULL;
 	const char *dns[3];
 
-	if (!msg || len < 3 || msg[0] != GPDS_CONTEXT_ACTIVATE_IND)
+	if (!msg || len < 3 || msg[0] != GPDS_CONTEXT_ACTIVATE_IND ||
+		msg[1] != cd->handle)
 		return;
 
-	cd = find_context_by_handle(gcd->contexts, msg[1]);
-	if (!cd) {
-		DBG("unknown context: 0x%02"PRIx8, msg[1]);
-		return;
-	}
-
 	for (g_isi_sb_iter_init(&iter, msg, len, 3);
 		g_isi_sb_iter_is_valid(&iter);
 		g_isi_sb_iter_next(&iter)) {
@@ -277,6 +287,9 @@ static void activate_ind_cb(GIsiClient *client,
 	CALLBACK_WITH_SUCCESS(cd->up_cb, ifname, TRUE, (const char *)ip,
 					STATIC_IP_NETMASK, NULL,
 					dns, cd->data);
+
+	g_source_remove(cd->activate_timeout);
+	cd->activate_timeout = 0;
 	return;
 
 error:
@@ -288,18 +301,11 @@ static void activate_fail_ind_cb(GIsiClient *client,
 					uint16_t object, void *opaque)
 {
 	const unsigned char *msg = data;
-	struct gprs_context_data *gcd = opaque;
-	struct context_data *cd;
+	struct context_data *cd = opaque;
 
 	if (!msg || len < 3 || msg[0] != GPDS_CONTEXT_ACTIVATE_FAIL_IND)
 		return;
 
-	cd = find_context_by_handle(gcd->contexts, msg[1]);
-	if (cd == NULL) {
-		DBG("unknown context: 0x%02"PRIx8, msg[1]);
-		return;
-	}
-
 	gprs_up_fail(cd);
 }
 
@@ -312,8 +318,6 @@ static gboolean context_activate_cb(GIsiClient *client,
 	if (!check_resp(client, data, len, GPDS_CONTEXT_ACTIVATE_RESP, cd))
 		return gprs_up_fail(cd);
 
-	/* TODO: Add timeout here in case indications never come */
-
 	return TRUE;
 }
 
@@ -327,6 +331,15 @@ static void send_context_activate(GIsiClient *client, void *opaque)
 		0,		/* sub blocks */
 	};
 
+
+	g_isi_add_subscription(client, PN_GPDS, GPDS_CONTEXT_ACTIVATE_IND,
+			activate_ind_cb, cd);
+	g_isi_add_subscription(client, PN_GPDS, GPDS_CONTEXT_ACTIVATE_FAIL_IND,
+			activate_fail_ind_cb, cd);
+	g_isi_add_subscription(client, PN_GPDS, GPDS_CONTEXT_DEACTIVATE_IND,
+			deactivate_ind_cb, cd);
+	g_isi_commit_subscriptions(client);
+
 	if (g_isi_request_make(client, msg, sizeof(msg), GPDS_TIMEOUT,
 				context_activate_cb, cd))
 		g_isi_pipe_start(cd->pipe);
@@ -468,58 +481,30 @@ static gboolean create_context_cb(GIsiClient *client,
 static void create_pipe_cb(GIsiPipe *pipe)
 {
 	struct context_data *cd = g_isi_pipe_get_userdata(pipe);
-	struct ofono_gprs_context *gc = cd->driver;
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 
 	const unsigned char msg[] = {
 		GPDS_CONTEXT_ID_CREATE_REQ,
 	};
 
-	if (!g_isi_request_make(gcd->client, msg, sizeof(msg), GPDS_TIMEOUT,
+	if (!g_isi_request_make(cd->client, msg, sizeof(msg), GPDS_TIMEOUT,
 				create_context_cb, cd))
 		gprs_up_fail(cd);
 }
 
-static void deactivate_ind_cb(GIsiClient *client,
-				const void *restrict data, size_t len,
-				uint16_t object, void *opaque)
-{
-	struct gprs_context_data *gcd = opaque;
-	struct context_data *cd;
-
-	const unsigned char *msg = data;
-
-	if (!msg || len < 3 || msg[0] != GPDS_CONTEXT_DEACTIVATE_IND)
-		return;
-
-	cd = find_context_by_handle(gcd->contexts, msg[1]);
-	if (cd == NULL) {
-		DBG("unknown context: 0x%02"PRIx8, msg[1]);
-		return;
-	}
-
-	DBG("context deactivated: %s (0x%02"PRIx8")",
-		gpds_isi_cause_name(msg[3]), msg[3]);
-
-	ofono_gprs_context_deactivated(cd->driver, cd->cid);
-
-	gcd->contexts = g_slist_remove(gcd->contexts, cd);
-	destroy_context(cd);
-}
-
 static void isi_gprs_activate_primary(struct ofono_gprs_context *gc,
 				const struct ofono_gprs_primary_context *ctx,
 				ofono_gprs_context_up_cb_t cb, void *data)
 {
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-	struct context_data *cd = g_try_new0(struct context_data, 1);
-	struct context_data *old = NULL;
+	struct context_data *cd = ofono_gprs_context_get_data(gc);
 
-	if (!gcd || !cd)
+	if (!cd->gpds) {
+		/* GPDS is not reachable */
+		CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL, NULL,
+					NULL, data);
 		return;
+	}
 
 	cd->cid = ctx->cid;
-	cd->driver = gc;
 	cd->up_cb = cb;
 	cd->data = data;
 	cd->pep = NULL;
@@ -527,14 +512,6 @@ static void isi_gprs_activate_primary(struct ofono_gprs_context *gc,
 	cd->handle = INVALID_ID;
 	cd->type = GPDS_PDP_TYPE_IPV4;
 
-	old = find_context_by_cid(gcd->contexts, ctx->cid);
-	if (old) {
-		DBG("duplicate context: %u", ctx->cid);
-		goto error;
-	}
-
-	gcd->contexts = g_slist_append(gcd->contexts, cd);
-
 	if (strlen(ctx->apn) >= GPDS_MAX_APN_STRING_LENGTH
 		|| strlen(ctx->username) >= GPDS_MAX_USERNAME_LENGTH
 		|| strlen(ctx->password) >= GPDS_MAX_PASSWORD_LENGTH)
@@ -549,18 +526,20 @@ static void isi_gprs_activate_primary(struct ofono_gprs_context *gc,
 	strncpy(cd->password, ctx->password, GPDS_MAX_PASSWORD_LENGTH);
 	cd->username[GPDS_MAX_PASSWORD_LENGTH] = '\0';
 
-	cd->pep = g_isi_pep_create(gcd->idx, NULL, NULL);
+	cd->pep = g_isi_pep_create(cd->idx, NULL, NULL);
 	if (cd->pep == NULL)
 		goto error;
 
-	cd->pipe = g_isi_pipe_create(gcd->idx, create_pipe_cb,
+	cd->pipe = g_isi_pipe_create(cd->idx, create_pipe_cb,
 					g_isi_pep_get_object(cd->pep),
-					gcd->gpds, PN_PEP_TYPE_GPRS,
+					cd->gpds, PN_PEP_TYPE_GPRS,
 					PN_PEP_TYPE_GPRS);
 	if (cd->pipe == NULL)
 		goto error;
 
 	g_isi_pipe_set_userdata(cd->pipe, cd);
+	cd->activate_timeout = g_timeout_add_seconds(ACTIVATE_TIMEOUT,
+							gprs_up_timeout, cd);
 	return;
 
 error:
@@ -573,16 +552,12 @@ static gboolean context_deactivate_cb(GIsiClient *client,
 					void *opaque)
 {
 	struct context_data *cd = opaque;
-	struct ofono_gprs_context *gc = cd->driver;
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 
 	if (!check_resp(client, data, len, GPDS_CONTEXT_DEACTIVATE_RESP, cd))
 		return gprs_down_fail(cd);
 
-	gcd->contexts = g_slist_remove(gcd->contexts, cd);
-
 	CALLBACK_WITH_SUCCESS(cd->down_cb, cd->data);
-	destroy_context(cd);
+	reset_context(cd);
 
 	return TRUE;
 }
@@ -591,39 +566,36 @@ static void isi_gprs_deactivate_primary(struct ofono_gprs_context *gc,
 					unsigned int cid,
 					ofono_gprs_context_cb_t cb, void *data)
 {
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-	struct context_data *cd;
+	struct context_data *cd = ofono_gprs_context_get_data(gc);
 
 	unsigned char msg[] = {
 		GPDS_CONTEXT_DEACTIVATE_REQ,
 		0x00,	/* GPDS context ID, added later */
 	};
 
-	if (!gcd)
-		return;
-
-	cd = find_context_by_cid(gcd->contexts, cid);
-	if (!cd) {
-		DBG("unknown context: %u", cid);
+	if (!cd)
 		return;
-	}
 
 	cd->down_cb = cb;
 	cd->data = data;
 
 	msg[1] = cd->handle;
 
-	if (!g_isi_request_make(gcd->client, msg, sizeof(msg), GPDS_TIMEOUT,
-				context_deactivate_cb, cd))
+	if (!g_isi_request_make(cd->client, msg, sizeof(msg), GPDS_TIMEOUT,
+					context_deactivate_cb, cd)) {
 		gprs_down_fail(cd);
+		return;
+	}
+
+	cd->deactivate_timeout = g_timeout_add_seconds(DEACTIVATE_TIMEOUT,
+							gprs_down_timeout, cd);
 }
 
 static void gpds_ctx_reachable_cb(GIsiClient *client, gboolean alive,
 					uint16_t object,
 					void *opaque)
 {
-	struct ofono_gprs_context *gc = opaque;
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	struct context_data *cd = opaque;
 	const char *debug;
 
 	if (!alive) {
@@ -636,61 +608,51 @@ static void gpds_ctx_reachable_cb(GIsiClient *client, gboolean alive,
 		g_isi_version_major(client),
 		g_isi_version_minor(client));
 
-	gcd->gpds = object;
+	cd->gpds = object;
 
 	debug = getenv("OFONO_ISI_DEBUG");
 	if (debug && (strcmp(debug, "all") == 0 || strcmp(debug, "gpds") == 0))
-		g_isi_client_set_debug(gcd->client, gpds_debug, NULL);
-
-	g_isi_subscribe(client, GPDS_CONTEXT_ACTIVATE_IND,
-			activate_ind_cb, gcd);
-	g_isi_subscribe(client, GPDS_CONTEXT_ACTIVATE_FAIL_IND,
-			activate_fail_ind_cb, gcd);
-	g_isi_subscribe(client, GPDS_CONTEXT_DEACTIVATE_IND,
-			deactivate_ind_cb, gcd);
+		g_isi_client_set_debug(cd->client, gpds_debug, NULL);
 }
 
 static int isi_gprs_context_probe(struct ofono_gprs_context *gc,
 					unsigned int vendor, void *user)
 {
 	GIsiModem *idx = user;
-	struct gprs_context_data *gcd = g_try_new0(struct gprs_context_data, 1);
+	struct context_data *cd = g_try_new0(struct context_data, 1);
 
-	if (!gcd)
+	if (!cd)
 		return -ENOMEM;
 
-	gcd->client = g_isi_client_create(idx, PN_GPDS);
-	if (!gcd->client) {
-		g_free(gcd);
+	cd->client = g_isi_client_create(idx, PN_GPDS);
+	if (!cd->client) {
+		g_free(cd);
 		return -ENOMEM;
 	}
 
-	ofono_gprs_context_set_data(gc, gcd);
-
-	gcd->idx = idx;
-	gcd->contexts = NULL;
+	cd->idx = idx;
+	cd->context = gc;
+	ofono_gprs_context_set_data(gc, cd);
 
-	g_isi_verify(gcd->client, gpds_ctx_reachable_cb, gc);
+	g_isi_verify(cd->client, gpds_ctx_reachable_cb, cd);
 
 	return 0;
 }
 
 static void isi_gprs_context_remove(struct ofono_gprs_context *gc)
 {
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-	GSList *m;
+	struct context_data *cd = ofono_gprs_context_get_data(gc);
 
-	ofono_gprs_context_set_data(gc, NULL);
-
-	for (m = gcd->contexts; m; m = m->next)
-		destroy_context(m->data);
+	if (!cd)
+		return;
 
-	g_slist_free(gcd->contexts);
+	ofono_gprs_context_set_data(gc, NULL);
+	reset_context(cd);
 
-	if (gcd->client)
-		g_isi_client_destroy(gcd->client);
+	if (cd->client)
+		g_isi_client_destroy(cd->client);
 
-	g_free(gcd);
+	g_free(cd);
 }
 
 static struct ofono_gprs_context_driver driver = {
-- 
1.7.0.4

[PATCH 4/4] isigen: make number of PDP contexts configurable

[Mika Liljeberg]

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

---
 plugins/isigen.c |   23 +++++++++++++++++------
 plugins/udev.c   |    6 +++++-
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/plugins/isigen.c b/plugins/isigen.c
index 838d060..cee4e44 100644
--- a/plugins/isigen.c
+++ b/plugins/isigen.c
@@ -58,6 +58,8 @@
 #include "drivers/isimodem/mtc.h"
 #include "drivers/isimodem/debug.h"
 
+#define ISI_DEFAULT_PDPS 4	/* Number of supported PDP contexts */
+
 struct isi_data {
 	struct ofono_modem *modem;
 	char const *ifname;
@@ -407,6 +409,8 @@ static void isigen_post_online(struct ofono_modem *modem)
 	struct isi_data *isi = ofono_modem_get_data(modem);
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
+	int pdps = ofono_modem_get_integer(modem, "PDPS");
+	int i;
 
 	DBG("(%p) with %s", modem, isi->ifname);
 
@@ -420,13 +424,20 @@ static void isigen_post_online(struct ofono_modem *modem)
 	ofono_call_barring_create(isi->modem, 0, "isimodem", isi->idx);
 	ofono_call_meter_create(isi->modem, 0, "isimodem", isi->idx);
 	ofono_radio_settings_create(isi->modem, 0, "isimodem", isi->idx);
-	gprs = ofono_gprs_create(isi->modem, 0, "isimodem", isi->idx);
-	gc = ofono_gprs_context_create(isi->modem, 0, "isimodem", isi->idx);
 
-	if (gprs && gc)
-		ofono_gprs_add_context(gprs, gc);
-	else
-		DBG("Failed to add context");
+	gprs = ofono_gprs_create(isi->modem, 0, "isimodem", isi->idx);
+	if (!gprs)
+		return;
+	if (!pdps)
+		pdps = ISI_DEFAULT_PDPS;
+	for (i = 1; i <= pdps; i++) {
+		gc = ofono_gprs_context_create(isi->modem, 0,
+						"isimodem", isi->idx);
+		if (gc)
+			ofono_gprs_add_context(gprs, gc);
+		else
+			DBG("Failed to add context %d", i);
+	}
 }
 
 static int isigen_enable(struct ofono_modem *modem)
diff --git a/plugins/udev.c b/plugins/udev.c
index 737a637..a25adcd 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -423,7 +423,7 @@ static void add_nokia(struct ofono_modem *modem,
 static void add_isi(struct ofono_modem *modem,
 					struct udev_device *udev_device)
 {
-	const char *ifname, *type, *addr;
+	const char *ifname, *type, *addr, *pdps;
 
 	DBG("modem %p", modem);
 
@@ -434,6 +434,10 @@ static void add_isi(struct ofono_modem *modem,
 	if (addr != NULL)
 		ofono_modem_set_integer(modem, "Address", atoi(addr));
 
+	pdps = get_property(udev_device, "OFONO_ISI_PDPS");
+	if (pdps != NULL)
+		ofono_modem_set_integer(modem, "PDPS", atoi(pdps));
+
 	if (g_strcmp0(udev_device_get_subsystem(udev_device), "net") != 0)
 		return;
 
-- 
1.7.0.4

Re: [PATCH 4/4] isigen: make number of PDP contexts configurable

[Marcel Holtmann]

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

Hi Mika,

>  plugins/isigen.c |   23 +++++++++++++++++------
>  plugins/udev.c   |    6 +++++-
>  2 files changed, 22 insertions(+), 7 deletions(-)

<snip>
 
> @@ -434,6 +434,10 @@ static void add_isi(struct ofono_modem *modem,
>  	if (addr != NULL)
>  		ofono_modem_set_integer(modem, "Address", atoi(addr));
>  
> +	pdps = get_property(udev_device, "OFONO_ISI_PDPS");
> +	if (pdps != NULL)
> +		ofono_modem_set_integer(modem, "PDPS", atoi(pdps));
> +
>  	if (g_strcmp0(udev_device_get_subsystem(udev_device), "net") != 0)
>  		return;

why to do you bother making this a configurable option? What isthe
benefit here?

Personally I think that always enabling 4 context if the hardware
supports it should be enough. If you do support more then just enable
more all the time. There are no real resources used in context of ISI
anyway. The AT command based modems have a different problem since for
most of them we need an extra TTY/DLC and an extra GAtChat object, but
ISI does not have that problem.

Regards

Marcel

Re: [PATCH 1/4] gisi: fix crash bug in g_isi_remove_subscription

[Aki Niemi]

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

Hi Mika,

2010/11/10 Mika Liljeberg <mika.liljeberg@nokia.com>:
> ---
>  gisi/client.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)

Patch has been applied. Thanks.

Cheers,
Aki

Re: [PATCH 2/4] gisi: return NULL instead of asserting

[Aki Niemi]

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

Hi Mika,

2010/11/10 Mika Liljeberg <mika.liljeberg@nokia.com>:
> ---
>  gisi/pep.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)

Patch has been applied. Thanks.

Cheers,
Aki

Re: [PATCH 3/4] isimodem: revector GPRS context driver

[Aki Niemi]

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

Hi Mika,

2010/11/10 Mika Liljeberg <mika.liljeberg@nokia.com>:
> ---
>  drivers/isimodem/gprs-context.c |  280 +++++++++++++++++----------------------
>  1 files changed, 121 insertions(+), 159 deletions(-)

Patch has been applied. Thanks!

Cheers,
Aki

RE: [PATCH 4/4] isigen: make number of PDP contexts configurable

[Mika.Liljeberg]

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

Hi Marcel,

> why to do you bother making this a configurable option? What isthe
> benefit here?

The maximum context count is a compile time option for the ISI modem. Having this option in oFono makes it possible to optimize the APE side resource usage instead of overallocating drivers and contexts. Not a huge benefit, I guess, but the cost is not huge either.

Not a big deal, though. I'll try to see if the context limit could be probed somehow.

> Personally I think that always enabling 4 context if the hardware
> supports it should be enough. If you do support more then just enable
> more all the time. There are no real resources used in context of ISI
> anyway. The AT command based modems have a different problem since for
> most of them we need an extra TTY/DLC and an extra GAtChat object, but
> ISI does not have that problem.

I don't see the difference. The AT modem resources should be allocated dynamically as well, at context activation time. (Maybe that's the case already, I didn't really check.) In any case, I believe oFono has to allow things like external AT command processors and vendor specific modem tools to access the TTY's directly bypassing oFono. This means the mux channels should not be preallocated.

	MikaL

RE: [PATCH 4/4] isigen: make number of PDP contexts configurable

[Marcel Holtmann]

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

Hi Mika,

> > why to do you bother making this a configurable option? What isthe
> > benefit here?
> 
> The maximum context count is a compile time option for the ISI modem. Having this option in oFono makes it possible to optimize the APE side resource usage instead of overallocating drivers and contexts. Not a huge benefit, I guess, but the cost is not huge either.
> 
> Not a big deal, though. I'll try to see if the context limit could be probed somehow.

if it can be probed from the ISI modem that would be perfect.

> > Personally I think that always enabling 4 context if the hardware
> > supports it should be enough. If you do support more then just enable
> > more all the time. There are no real resources used in context of ISI
> > anyway. The AT command based modems have a different problem since for
> > most of them we need an extra TTY/DLC and an extra GAtChat object, but
> > ISI does not have that problem.
> 
> I don't see the difference. The AT modem resources should be allocated dynamically as well, at context activation time. (Maybe that's the case already, I didn't really check.) In any case, I believe oFono has to allow things like external AT command processors and vendor specific modem tools to access the TTY's directly bypassing oFono. This means the mux channels should not be preallocated.

That is sort of a dream world. AT command based modems where you need a
TTY/DLC for every active GPRS context are not cheap. It is not a crazy
waste of resources, but it is some kind of waste.

Regards

Marcel

Re: [PATCH 4/4] isigen: make number of PDP contexts configurable

[Denis Kenzior]

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

On 11/11/2010 02:16 AM, Mika.Liljeberg(a)nokia.com wrote:
> Hi Marcel,
> 
>> why to do you bother making this a configurable option? What isthe
>> benefit here?
> 
> The maximum context count is a compile time option for the ISI modem. Having this option in oFono makes it possible to optimize the APE side resource usage instead of overallocating drivers and contexts. Not a huge benefit, I guess, but the cost is not huge either.
> 
> Not a big deal, though. I'll try to see if the context limit could be probed somehow.
> 

The gprs context structure is about the size of 5 pointers, so there's
not much to be saved here.  If you're worried about space usage of the
isi specific data, you can always allocate it during the activation stage.

>> Personally I think that always enabling 4 context if the hardware
>> supports it should be enough. If you do support more then just enable
>> more all the time. There are no real resources used in context of ISI
>> anyway. The AT command based modems have a different problem since for
>> most of them we need an extra TTY/DLC and an extra GAtChat object, but
>> ISI does not have that problem.
> 
> I don't see the difference. The AT modem resources should be allocated dynamically as well, at context activation time. (Maybe that's the case already, I didn't really check.) In any case, I believe oFono has to allow things like external AT command processors and vendor specific modem tools to access the TTY's directly bypassing oFono. This means the mux channels should not be preallocated.

Hah, I know some people who vehemently disagree ;)  Of course I'm not
one of them...  Strictly speaking what you say is possible even with
today's architecture, however almost no AT modem we have supports more
than about 3-4 contexts.  So doing this to save 300-400 bytes / context
is simply not worth it at this point.

Regards,
-Denis