[PATCH v1 00/11] Support for U-Blox Toby L2 modems

[PATCH v1 00/11] Support for U-Blox Toby L2 modems

[Dragos Tatulea]

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

This patchset adds support for Toby L2 modems in high throughput
mode.

Besides basic support, the following things are supported:
* 2/3G bearers
* bridged AND routed modes.
* Multiple primary PDP contexts.

This version of the patches removes LTE specific hacks which will be
subsequently committed in their own patch sets:
* CREG status is no longer patched for ublox 
* The ublox-specific way of making LTE default bearers work has been
removed. This functionality needs to be supported by the core.

Dongsu Park (3):
  plugins/udevng: support different interface strings to detect TOBY
    series
  plugins/ublox: read network mode
  ubloxmodem: add routed mode support

Dragos Tatulea (8):
  plugins/ublox: allow enabling of TOBY L2 modems
  plugins/ublox: use vendor from structure instead of fixed
  atmodem: add support for U-Blox TOBY L2 modems
  plugins/ublox: give names to model ids
  ubloxmodem: add Toby L2 gprs context driver
  plugins/ublox: enable ubloxmodem driver when possible
  plugins/ublox: support more internet contexts
  ubloxmodem: support authentication

 Makefile.am                            |   7 +
 drivers/atmodem/gprs.c                 |   1 +
 drivers/atmodem/network-registration.c |  21 +-
 drivers/atmodem/sim.c                  |   1 +
 drivers/ubloxmodem/gprs-context.c      | 614 +++++++++++++++++++++++++++++++++
 drivers/ubloxmodem/ubloxmodem.c        |  49 +++
 drivers/ubloxmodem/ubloxmodem.h        |  25 ++
 plugins/ublox.c                        | 128 ++++++-
 plugins/udevng.c                       |  24 +-
 9 files changed, 845 insertions(+), 25 deletions(-)
 create mode 100644 drivers/ubloxmodem/gprs-context.c
 create mode 100644 drivers/ubloxmodem/ubloxmodem.c
 create mode 100644 drivers/ubloxmodem/ubloxmodem.h

-- 
2.5.0

[PATCH 01/11] plugins/udevng: support different interface strings to detect TOBY series

[Dragos Tatulea]

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

From: Dongsu Park <dongsu@endocode.com>

Each modem expresses their interfaces with its own interface string,
which is composed of 3 different USB attributes:
"bInterfaceClass/bInterfaceSubClass/bInterfaceProtocol".
While the old models like LISA support only "2/2/1" for modem
interfaces, TOBY-L2 also supports an unique string for NetworkInterface
for each profile.

* low-medium throughput profile : 2/6/0
* fairly backward-compatible profile : 10/0/0
* high throughput profile : 224/1/3

Besides the condition for checking NULL for mdm/aux/net should be relaxed
a little bit.
---
 plugins/udevng.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 40da2cc..be92664 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -838,7 +838,7 @@ static gboolean setup_quectel(struct modem_info *modem)
 
 static gboolean setup_ublox(struct modem_info *modem)
 {
-	const char *aux = NULL, *mdm = NULL;
+	const char *aux = NULL, *mdm = NULL, *net = NULL;
 	GSList *list;
 
 	DBG("%s", modem->syspath);
@@ -857,22 +857,40 @@ static gboolean setup_ublox(struct modem_info *modem)
 			mdm = info->devnode;
 			if (aux != NULL)
 				break;
+		/*
+		 * "2/2/1"
+		 *  - a common modem interface both for older models like LISA,
+		 *    and for newer models like TOBY.
+		 * For TOBY-L2, NetworkInterface can be detected for each
+		 * profile:
+		 *  - low-medium throughput profile : 2/6/0
+		 *  - fairly backward-compatible profile : 10/0/0
+		 *  - high throughput profile : 224/1/3
+		 */
 		} else if (g_strcmp0(info->interface, "2/2/1") == 0) {
 			if (g_strcmp0(info->number, "02") == 0)
 				aux = info->devnode;
 			else if (g_strcmp0(info->number, "00") == 0)
 				mdm = info->devnode;
+		} else if (g_strcmp0(info->interface, "2/6/0") == 0 ||
+			   g_strcmp0(info->interface, "10/0/0") == 0 ||
+			   g_strcmp0(info->interface, "224/1/3") == 0) {
+			net = info->devnode;
 		}
 	}
 
-	if (aux == NULL || mdm == NULL)
+	/* Abort only if both interfaces are NULL, as it's highly possible that
+	 * only one of 2 interfaces is available for U-blox modem.
+	 */
+	if (aux == NULL && mdm == NULL)
 		return FALSE;
 
-	DBG("aux=%s modem=%s", aux, mdm);
+	DBG("aux=%s modem=%s net=%s", aux, mdm, net);
 
 	ofono_modem_set_string(modem->modem, "Aux", aux);
 	ofono_modem_set_string(modem->modem, "Modem", mdm);
 	ofono_modem_set_string(modem->modem, "Model", modem->model);
+	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
 
 	return TRUE;
 }
-- 
2.5.0

Re: [PATCH 01/11] plugins/udevng: support different interface strings to detect TOBY series

[Denis Kenzior]

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

Hi Dongsu,

On 03/14/2016 10:50 AM, Dragos Tatulea wrote:
> From: Dongsu Park <dongsu@endocode.com>
>
> Each modem expresses their interfaces with its own interface string,
> which is composed of 3 different USB attributes:
> "bInterfaceClass/bInterfaceSubClass/bInterfaceProtocol".
> While the old models like LISA support only "2/2/1" for modem
> interfaces, TOBY-L2 also supports an unique string for NetworkInterface
> for each profile.
>
> * low-medium throughput profile : 2/6/0
> * fairly backward-compatible profile : 10/0/0
> * high throughput profile : 224/1/3
>
> Besides the condition for checking NULL for mdm/aux/net should be relaxed
> a little bit.
> ---
>   plugins/udevng.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH 02/11] plugins/ublox: allow enabling of TOBY L2 modems

[Dragos Tatulea]

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

For this we need to:
* Set the vendor family based on model id.
* Not use modem interface for the TOBY L2 family.
---
 plugins/ublox.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 9 deletions(-)

diff --git a/plugins/ublox.c b/plugins/ublox.c
index 89ca709..eab4ed4 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -47,6 +47,7 @@ static const char *none_prefix[] = { NULL };
 struct ublox_data {
 	GAtChat *modem;
 	GAtChat *aux;
+	enum ofono_vendor vendor_family;
 };
 
 static void ublox_debug(const char *str, void *user_data)
@@ -138,24 +139,56 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 static int ublox_enable(struct ofono_modem *modem)
 {
 	struct ublox_data *data = ofono_modem_get_data(modem);
+	const char *model_str = NULL;
+	int model_id;
 
 	DBG("%p", modem);
 
-	data->modem = open_device(modem, "Modem", "Modem: ");
-	if (data->modem == NULL)
+	model_str = ofono_modem_get_string(modem, "Model");
+	if (model_str == NULL)
 		return -EINVAL;
 
+	/*
+	 * Toby L2 devices are more complex and special than previously
+	 * supported U-Blox devices. So they need a vendor of their own.
+	 */
+	model_id = atoi(model_str);
+	switch (model_id) {
+	case 1102:
+		data->vendor_family = OFONO_VENDOR_UBLOX;
+		break;
+	case 1141:
+	case 1146:
+		data->vendor_family = OFONO_VENDOR_UBLOX_TOBY_L2;
+		break;
+	case 1143:
+		DBG("low/medium throughtput profile unsupported");
+	default:
+		DBG("unknown ublox model id %d", model_id);
+		return -EINVAL;
+	}
+
 	data->aux = open_device(modem, "Aux", "Aux: ");
-	if (data->aux == NULL) {
-		g_at_chat_unref(data->modem);
-		data->modem = NULL;
-		return -EIO;
+	if (data->aux == NULL)
+		return -EINVAL;
+
+	if (data->vendor_family == OFONO_VENDOR_UBLOX) {
+		data->modem = open_device(modem, "Modem", "Modem: ");
+		if (data->modem == NULL) {
+			g_at_chat_unref(data->aux);
+			data->aux = NULL;
+			return -EIO;
+		}
+
+		g_at_chat_set_slave(data->modem, data->aux);
+
+		g_at_chat_send(data->modem, "ATE0 +CMEE=1", none_prefix,
+						NULL, NULL, NULL);
 	}
 
-	g_at_chat_set_slave(data->modem, data->aux);
+	/* The modem can take a while to wake up if just powered on. */
+	g_at_chat_set_wakeup_command(data->aux, "AT\r", 1000, 11000);
 
-	g_at_chat_send(data->modem, "ATE0 +CMEE=1", none_prefix,
-					NULL, NULL, NULL);
 	g_at_chat_send(data->aux, "ATE0 +CMEE=1", none_prefix,
 					NULL, NULL, NULL);
 
-- 
2.5.0

Re: [PATCH 02/11] plugins/ublox: allow enabling of TOBY L2 modems

[Denis Kenzior]

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

Hi Dragos,

On 03/14/2016 10:50 AM, Dragos Tatulea wrote:
> For this we need to:
> * Set the vendor family based on model id.
> * Not use modem interface for the TOBY L2 family.
> ---
>   plugins/ublox.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 42 insertions(+), 9 deletions(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH 03/11] plugins/ublox: use vendor from structure instead of fixed

[Dragos Tatulea]

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

That's because we need to differentiate between multiple ublox
devices.
---
 plugins/ublox.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/plugins/ublox.c b/plugins/ublox.c
index eab4ed4..d930efe 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -267,8 +267,8 @@ static void ublox_pre_sim(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
-	ofono_devinfo_create(modem, 0, "atmodem", data->aux);
-	sim = ofono_sim_create(modem, OFONO_VENDOR_UBLOX, "atmodem",
+	ofono_devinfo_create(modem, data->vendor_family, "atmodem", data->aux);
+	sim = ofono_sim_create(modem, data->vendor_family, "atmodem",
 					data->aux);
 
 	if (sim)
@@ -283,10 +283,10 @@ static void ublox_post_sim(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
-	gprs = ofono_gprs_create(modem, OFONO_VENDOR_UBLOX, "atmodem",
+	gprs = ofono_gprs_create(modem, data->vendor_family, "atmodem",
 					data->aux);
-	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_UBLOX, "atmodem",
-					data->modem);
+	gc = ofono_gprs_context_create(modem, data->vendor_family, "atmodem",
+					data->modem ? data->modem : data->aux);
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
@@ -296,7 +296,7 @@ static void ublox_post_online(struct ofono_modem *modem)
 {
 	struct ublox_data *data = ofono_modem_get_data(modem);
 
-	ofono_netreg_create(modem, 0, "atmodem", data->aux);
+	ofono_netreg_create(modem, data->vendor_family, "atmodem", data->aux);
 }
 
 static struct ofono_modem_driver ublox_driver = {
-- 
2.5.0

Re: [PATCH 03/11] plugins/ublox: use vendor from structure instead of fixed

[Denis Kenzior]

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

Hi Dragos,

On 03/14/2016 10:50 AM, Dragos Tatulea wrote:
> That's because we need to differentiate between multiple ublox
> devices.
> ---
>   plugins/ublox.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/plugins/ublox.c b/plugins/ublox.c
> index eab4ed4..d930efe 100644
> --- a/plugins/ublox.c
> +++ b/plugins/ublox.c
> @@ -267,8 +267,8 @@ static void ublox_pre_sim(struct ofono_modem *modem)
>
>   	DBG("%p", modem);
>
> -	ofono_devinfo_create(modem, 0, "atmodem", data->aux);
> -	sim = ofono_sim_create(modem, OFONO_VENDOR_UBLOX, "atmodem",
> +	ofono_devinfo_create(modem, data->vendor_family, "atmodem", data->aux);

There's no vendor-specific code inside the devinfo driver.  In general, 
unless vendor-specific paths are actually needed, set the vendor field 
to 0.  That makes it easier to know when vendor specific behavior is used.

> +	sim = ofono_sim_create(modem, data->vendor_family, "atmodem",
>   					data->aux);
>
>   	if (sim)
> @@ -283,10 +283,10 @@ static void ublox_post_sim(struct ofono_modem *modem)
>
>   	DBG("%p", modem);
>
> -	gprs = ofono_gprs_create(modem, OFONO_VENDOR_UBLOX, "atmodem",
> +	gprs = ofono_gprs_create(modem, data->vendor_family, "atmodem",
>   					data->aux);
> -	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_UBLOX, "atmodem",
> -					data->modem);
> +	gc = ofono_gprs_context_create(modem, data->vendor_family, "atmodem",
> +					data->modem ? data->modem : data->aux);
>
>   	if (gprs && gc)
>   		ofono_gprs_add_context(gprs, gc);
> @@ -296,7 +296,7 @@ static void ublox_post_online(struct ofono_modem *modem)
>   {
>   	struct ublox_data *data = ofono_modem_get_data(modem);
>
> -	ofono_netreg_create(modem, 0, "atmodem", data->aux);
> +	ofono_netreg_create(modem, data->vendor_family, "atmodem", data->aux);
>   }
>
>   static struct ofono_modem_driver ublox_driver = {
>

Applied, thanks.

Regards,
-Denis

[PATCH 04/11] atmodem: add support for U-Blox TOBY L2 modems

[Dragos Tatulea]

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

Besides exceptions below, act like normal U-Blox devices.

gprs-context: don't set auth for TOBY L2. U-Blox Toby L2
doesn't support PAP/CHAP APN auth method.

atmodem: TOBY L2 supports only CMER mode 1. Also chaged original
mode variable to ind, which is a more appropriate name.
mode is what is being set first.
---
 drivers/atmodem/gprs.c                 |  1 +
 drivers/atmodem/network-registration.c | 21 ++++++++++++++++-----
 drivers/atmodem/sim.c                  |  1 +
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index 0165253..4505477 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -351,6 +351,7 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
 						FALSE, gprs, NULL);
 		break;
 	case OFONO_VENDOR_UBLOX:
+	case OFONO_VENDOR_UBLOX_TOBY_L2:
 		g_at_chat_register(gd->chat, "+UREG:", ublox_ureg_notify,
 						FALSE, gprs, NULL);
 		g_at_chat_send(gd->chat, "AT+UREG=1", none_prefix,
diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index 7cfd6b2..c2df0ce 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -1580,17 +1580,28 @@ static inline ofono_bool_t append_cmer_element(char *buf, int *len, int cap,
 static ofono_bool_t build_cmer_string(char *buf, int *cmer_opts,
 					struct netreg_data *nd)
 {
-	const char *mode;
+	const char *ind;
 	int len = sprintf(buf, "AT+CMER=");
+	const char *mode;
 
 	DBG("");
 
+	switch (nd->vendor) {
+	case OFONO_VENDOR_UBLOX_TOBY_L2:
+		/* UBX-13002752 R33: TOBY L2 doesn't support mode 2 and 3 */
+		mode = "1";
+		break;
+	default:
+		mode = "3";
+		break;
+	}
+
 	/*
 	 * Forward unsolicited result codes directly to the TE;
 	 * TA‑TE link specific inband technique used to embed result codes and
 	 * data when TA is in on‑line data mode
 	 */
-	if (!append_cmer_element(buf, &len, cmer_opts[0], "3", FALSE))
+	if (!append_cmer_element(buf, &len, cmer_opts[0], mode, FALSE))
 		return FALSE;
 
 	/* No keypad event reporting */
@@ -1607,14 +1618,14 @@ static ofono_bool_t build_cmer_string(char *buf, int *cmer_opts,
 		 * Telit does not support mode 1.
 		 * All indicator events shall be directed from TA to TE.
 		 */
-		mode = "2";
+		ind = "2";
 		break;
 	default:
 		/*
 		 * Only those indicator events, which are not caused by +CIND
 		 * shall be indicated by the TA to the TE.
 		 */
-		mode = "1";
+		ind = "1";
 		break;
 	}
 
@@ -1623,7 +1634,7 @@ static ofono_bool_t build_cmer_string(char *buf, int *cmer_opts,
 	 * <ind> indicates the indicator order number (as specified for +CIND)
 	 * and <value> is the new value of indicator.
 	 */
-	if (!append_cmer_element(buf, &len, cmer_opts[3], mode, TRUE))
+	if (!append_cmer_element(buf, &len, cmer_opts[3], ind, TRUE))
 		return FALSE;
 
 	return TRUE;
diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index d6a0dcf..081d342 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -1120,6 +1120,7 @@ static void at_pin_retries_query(struct ofono_sim *sim,
 			return;
 		break;
 	case OFONO_VENDOR_UBLOX:
+	case OFONO_VENDOR_UBLOX_TOBY_L2:
 		if (g_at_chat_send(sd->chat, "AT+UPINCNT", upincnt_prefix,
 					upincnt_cb, cbd, g_free) > 0)
 			return;
-- 
2.5.0

Re: [PATCH 04/11] atmodem: add support for U-Blox TOBY L2 modems

[Denis Kenzior]

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

Hi Dragos,

On 03/14/2016 10:50 AM, Dragos Tatulea wrote:
> Besides exceptions below, act like normal U-Blox devices.
>
> gprs-context: don't set auth for TOBY L2. U-Blox Toby L2
> doesn't support PAP/CHAP APN auth method.
>
> atmodem: TOBY L2 supports only CMER mode 1. Also chaged original
> mode variable to ind, which is a more appropriate name.
> mode is what is being set first.
> ---
>   drivers/atmodem/gprs.c                 |  1 +
>   drivers/atmodem/network-registration.c | 21 ++++++++++++++++-----
>   drivers/atmodem/sim.c                  |  1 +
>   3 files changed, 18 insertions(+), 5 deletions(-)
>

Applied, thanks.

Regards,
-Denis

[PATCH 05/11] plugins/ublox: give names to model ids

[Dragos Tatulea]

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

To make it easier to understand the code.
---
 plugins/ublox.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/plugins/ublox.c b/plugins/ublox.c
index d930efe..23ed2cb 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -44,9 +44,18 @@
 
 static const char *none_prefix[] = { NULL };
 
+enum supported_models {
+	UNKNOWN				= 0,
+	SARA_G270			= 1102,
+	TOBYL2_COMPATIBLE_MODE 		= 1141,
+	TOBYL2_MEDIUM_THROUGHPUT_MODE 	= 1143,
+	TOBYL2_HIGH_THROUGHPUT_MODE 	= 1146,
+};
+
 struct ublox_data {
 	GAtChat *modem;
 	GAtChat *aux;
+	int model_id;
 	enum ofono_vendor vendor_family;
 };
 
@@ -140,7 +149,6 @@ static int ublox_enable(struct ofono_modem *modem)
 {
 	struct ublox_data *data = ofono_modem_get_data(modem);
 	const char *model_str = NULL;
-	int model_id;
 
 	DBG("%p", modem);
 
@@ -152,19 +160,20 @@ static int ublox_enable(struct ofono_modem *modem)
 	 * Toby L2 devices are more complex and special than previously
 	 * supported U-Blox devices. So they need a vendor of their own.
 	 */
-	model_id = atoi(model_str);
-	switch (model_id) {
-	case 1102:
+
+	data->model_id = atoi(model_str);
+	switch (data->model_id) {
+	case SARA_G270:
 		data->vendor_family = OFONO_VENDOR_UBLOX;
 		break;
-	case 1141:
-	case 1146:
+	case TOBYL2_COMPATIBLE_MODE:
+	case TOBYL2_HIGH_THROUGHPUT_MODE:
 		data->vendor_family = OFONO_VENDOR_UBLOX_TOBY_L2;
 		break;
-	case 1143:
+	case TOBYL2_MEDIUM_THROUGHPUT_MODE:
 		DBG("low/medium throughtput profile unsupported");
 	default:
-		DBG("unknown ublox model id %d", model_id);
+		DBG("unknown ublox model id %d", data->model_id);
 		return -EINVAL;
 	}
 
-- 
2.5.0

Re: [PATCH 05/11] plugins/ublox: give names to model ids

[Denis Kenzior]

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

Hi Dragos,

On 03/14/2016 10:50 AM, Dragos Tatulea wrote:
> To make it easier to understand the code.
> ---
>   plugins/ublox.c | 25 +++++++++++++++++--------
>   1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/plugins/ublox.c b/plugins/ublox.c
> index d930efe..23ed2cb 100644
> --- a/plugins/ublox.c
> +++ b/plugins/ublox.c
> @@ -44,9 +44,18 @@
>
>   static const char *none_prefix[] = { NULL };
>
> +enum supported_models {
> +	UNKNOWN				= 0,

Please take this UNKNOWN value out, it is pointless

> +	SARA_G270			= 1102,
> +	TOBYL2_COMPATIBLE_MODE 		= 1141,
> +	TOBYL2_MEDIUM_THROUGHPUT_MODE 	= 1143,
> +	TOBYL2_HIGH_THROUGHPUT_MODE 	= 1146,
> +};
> +
>   struct ublox_data {
>   	GAtChat *modem;
>   	GAtChat *aux;
> +	int model_id;
>   	enum ofono_vendor vendor_family;
>   };
>
> @@ -140,7 +149,6 @@ static int ublox_enable(struct ofono_modem *modem)
>   {
>   	struct ublox_data *data = ofono_modem_get_data(modem);
>   	const char *model_str = NULL;
> -	int model_id;
>
>   	DBG("%p", modem);
>
> @@ -152,19 +160,20 @@ static int ublox_enable(struct ofono_modem *modem)
>   	 * Toby L2 devices are more complex and special than previously
>   	 * supported U-Blox devices. So they need a vendor of their own.
>   	 */
> -	model_id = atoi(model_str);
> -	switch (model_id) {
> -	case 1102:
> +
No empty line here
> +	data->model_id = atoi(model_str);
empty line here
> +	switch (data->model_id) {
> +	case SARA_G270:
>   		data->vendor_family = OFONO_VENDOR_UBLOX;
>   		break;
> -	case 1141:
> -	case 1146:
> +	case TOBYL2_COMPATIBLE_MODE:
> +	case TOBYL2_HIGH_THROUGHPUT_MODE:
>   		data->vendor_family = OFONO_VENDOR_UBLOX_TOBY_L2;
>   		break;
> -	case 1143:
> +	case TOBYL2_MEDIUM_THROUGHPUT_MODE:
>   		DBG("low/medium throughtput profile unsupported");
>   	default:
> -		DBG("unknown ublox model id %d", model_id);
> +		DBG("unknown ublox model id %d", data->model_id);
>   		return -EINVAL;
>   	}
>
>

Regards,
-Denis

[PATCH 06/11] ubloxmodem: add Toby L2 gprs context driver

[Dragos Tatulea]

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

For now the driver works only with bridged mode for 2G/3G.

Once it activates the context it reads the ip, netmask,
gw, dns and sets them in the context settings.
---
 Makefile.am                       |   7 +
 drivers/ubloxmodem/gprs-context.c | 415 ++++++++++++++++++++++++++++++++++++++
 drivers/ubloxmodem/ubloxmodem.c   |  49 +++++
 drivers/ubloxmodem/ubloxmodem.h   |  25 +++
 4 files changed, 496 insertions(+)
 create mode 100644 drivers/ubloxmodem/gprs-context.c
 create mode 100644 drivers/ubloxmodem/ubloxmodem.c
 create mode 100644 drivers/ubloxmodem/ubloxmodem.h

diff --git a/Makefile.am b/Makefile.am
index cde998d..7652cfe 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -374,6 +374,13 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/speedupmodem/speedupmodem.c \
 			drivers/speedupmodem/ussd.c
 
+builtin_modules += ubloxmodem
+builtin_sources += drivers/atmodem/atutil.h \
+			drivers/ubloxmodem/ubloxmodem.h \
+			drivers/ubloxmodem/ubloxmodem.c \
+			drivers/ubloxmodem/gprs-context.c
+
+
 if PHONESIM
 builtin_modules += phonesim
 builtin_sources += plugins/phonesim.c
diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c
new file mode 100644
index 0000000..4b8fb6c
--- /dev/null
+++ b/drivers/ubloxmodem/gprs-context.c
@@ -0,0 +1,415 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2016  EndoCode AG. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/gprs-context.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "ubloxmodem.h"
+
+static const char *none_prefix[] = { NULL };
+static const char *cgcontrdp_prefix[] = { "+CGCONTRDP:", NULL };
+
+struct gprs_context_data {
+	GAtChat *chat;
+	unsigned int active_context;
+	char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
+	ofono_gprs_context_cb_t cb;
+	void *cb_data;
+};
+
+#define UBLOX_MAX_DEF_CONTEXT 8
+
+/*
+ * CGCONTRDP returns addr + netmask in the same string in the form
+ * of "a.b.c.d.m.m.m.m" for IPv4. IPv6 is not supported so we ignore it.
+ */
+static int read_addrnetmask(struct ofono_gprs_context *gc,
+				const char *addrnetmask)
+{
+	char *dup = strdup(addrnetmask);
+	char *s = dup;
+
+	const char *addr = s;
+	const char *netmask = NULL;
+
+	int ret = -EINVAL;
+	int i;
+
+	/* Count 7 dots for ipv4, less or more means error. */
+	for (i = 0; i < 8; i++, s++) {
+		s = strchr(s, '.');
+		if (!s)
+			break;
+
+		if (i == 3) {
+			/* set netmask ptr and break the string */
+			netmask = s+1;
+			s[0] = 0;
+		}
+	}
+
+	if (i == 7) {
+		ofono_gprs_context_set_ipv4_address(gc, addr, 1);
+		ofono_gprs_context_set_ipv4_netmask(gc, netmask);
+
+		ret = 0;
+	}
+
+	free(dup);
+
+	return ret;
+}
+
+static void callback_with_error(struct gprs_context_data *gcd, GAtResult *res)
+{
+	struct ofono_error error;
+
+	decode_at_error(&error, g_at_result_final_response(res));
+	gcd->cb(&error, gcd->cb_data);
+}
+
+
+static void set_gprs_context_interface(struct ofono_gprs_context *gc)
+{
+	struct ofono_modem *modem;
+	const char *interface;
+
+	/* read interface name read at detection time */
+	modem = ofono_gprs_context_get_modem(gc);
+	interface = ofono_modem_get_string(modem, "NetworkInterface");
+	ofono_gprs_context_set_interface(gc, interface);
+}
+
+static void cgcontrdp_bridge_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	GAtResultIter iter;
+	int cid = -1;
+
+	const char *laddrnetmask = NULL;
+	const char *gw = NULL;
+	const char *dns[2+1] = { NULL, NULL, NULL };
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		callback_with_error(gcd, result);
+
+		return;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, "+CGCONTRDP:")) {
+		/* tmp vals for ignored fields */
+		int bearer_id;
+		const char *apn;
+
+		if (!g_at_result_iter_next_number(&iter, &cid))
+			break;
+
+		if (!g_at_result_iter_next_number(&iter, &bearer_id))
+			break;
+
+		if (!g_at_result_iter_next_string(&iter, &apn))
+			break;
+
+		if (!g_at_result_iter_next_string(&iter, &laddrnetmask))
+			break;
+
+		if (!g_at_result_iter_next_string(&iter, &gw))
+			break;
+
+		if (!g_at_result_iter_next_string(&iter, &dns[0]))
+			break;
+
+		if (!g_at_result_iter_next_string(&iter, &dns[1]))
+			break;
+	}
+
+	set_gprs_context_interface(gc);
+
+	if (!laddrnetmask || read_addrnetmask(gc, laddrnetmask) < 0) {
+		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+		return;
+	}
+
+	if (gw)
+		ofono_gprs_context_set_ipv4_gateway(gc, gw);
+
+	if (dns[0])
+		ofono_gprs_context_set_ipv4_dns_servers(gc, dns);
+
+	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+}
+
+static int ublox_read_ip_config_bridge(struct ofono_gprs_context *gc)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[64];
+
+	/* read ip configuration info */
+	snprintf(buf, sizeof(buf), "AT+CGCONTRDP=%u", gcd->active_context);
+	return g_at_chat_send(gcd->chat, buf, cgcontrdp_prefix,
+				cgcontrdp_bridge_cb, gc, NULL);
+
+}
+
+static void ublox_post_activation(struct ofono_gprs_context *gc)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+
+	if (ublox_read_ip_config_bridge(gc) < 0)
+		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+}
+
+static void cgact_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+
+	DBG("ok %d", ok);
+	if (!ok) {
+		gcd->active_context = 0;
+		callback_with_error(gcd, result);
+
+		return;
+	}
+
+	ublox_post_activation(gc);
+}
+
+static void cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[64];
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		gcd->active_context = 0;
+		callback_with_error(gcd, result);
+
+		return;
+	}
+
+	snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				cgact_enable_cb, gc, NULL))
+		return;
+
+	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+}
+
+static void ublox_activate_ctx(struct ofono_gprs_context *gc)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
+	int len;
+
+	len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"",
+				gcd->active_context);
+
+	if (gcd->apn)
+		snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"",
+					gcd->apn);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				cgdcont_cb, gc, NULL) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+}
+
+#define UBLOX_MAX_USER_LEN 50
+#define UBLOX_MAX_PASS_LEN 50
+
+static void ublox_gprs_activate_primary(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;
+	if (!gcd->active_context) {
+		ofono_error("can't activate more contexts");
+		CALLBACK_WITH_FAILURE(cb, data);
+		return;
+	}
+
+	gcd->cb = cb;
+	gcd->cb_data = data;
+	memcpy(gcd->apn, ctx->apn, sizeof(ctx->apn));
+
+	ublox_activate_ctx(gc);
+}
+
+static void cgact_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+
+	DBG("ok %d", ok);
+	if (!ok) {
+		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+		return;
+	}
+
+	gcd->active_context = 0;
+
+	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+}
+
+static void ublox_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);
+	char buf[64];
+
+	DBG("cid %u", cid);
+
+	gcd->cb = cb;
+	gcd->cb_data = data;
+
+	snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", gcd->active_context);
+	g_at_chat_send(gcd->chat, buf, none_prefix,
+			cgact_disable_cb, gc, NULL);
+}
+
+static void cgev_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	GAtResultIter iter;
+	const char *event;
+	gint cid;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CGEV:"))
+		return;
+
+	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))
+		return;
+
+	DBG("cid %d", cid);
+
+	if ((unsigned int) cid != gcd->active_context)
+		return;
+
+	ofono_gprs_context_deactivated(gc, gcd->active_context);
+	gcd->active_context = 0;
+}
+
+
+static int ublox_gprs_context_probe(struct ofono_gprs_context *gc,
+					unsigned int vendor, void *data)
+{
+	GAtChat *chat = data;
+	struct gprs_context_data *gcd;
+
+	DBG("");
+
+	gcd = g_try_new0(struct gprs_context_data, 1);
+	if (gcd == NULL)
+		return -ENOMEM;
+
+	gcd->chat = g_at_chat_clone(chat);
+
+	ofono_gprs_context_set_data(gc, gcd);
+
+	g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
+
+	return 0;
+}
+
+static void ublox_gprs_context_remove(struct ofono_gprs_context *gc)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+
+	DBG("");
+
+	ofono_gprs_context_set_data(gc, NULL);
+
+	g_at_chat_unref(gcd->chat);
+
+	memset(gcd, 0, sizeof(*gcd));
+}
+
+static struct ofono_gprs_context_driver driver = {
+	.name			= "ubloxmodem",
+	.probe			= ublox_gprs_context_probe,
+	.remove			= ublox_gprs_context_remove,
+	.activate_primary	= ublox_gprs_activate_primary,
+	.deactivate_primary	= ublox_gprs_deactivate_primary,
+};
+
+
+void ublox_gprs_context_init(void)
+{
+	ofono_gprs_context_driver_register(&driver);
+}
+
+void ublox_gprs_context_exit(void)
+{
+	ofono_gprs_context_driver_unregister(&driver);
+}
diff --git a/drivers/ubloxmodem/ubloxmodem.c b/drivers/ubloxmodem/ubloxmodem.c
new file mode 100644
index 0000000..7fc671e
--- /dev/null
+++ b/drivers/ubloxmodem/ubloxmodem.c
@@ -0,0 +1,49 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <glib.h>
+#include <gatchat.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/types.h>
+
+#include "ubloxmodem.h"
+
+static int ubloxmodem_init(void)
+{
+	ublox_gprs_context_init();
+
+	return 0;
+}
+
+static void ubloxmodem_exit(void)
+{
+	ublox_gprs_context_exit();
+}
+
+OFONO_PLUGIN_DEFINE(ubloxmodem, "U-Blox Toby L2 high speed modem driver",
+			VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
+			ubloxmodem_init, ubloxmodem_exit)
diff --git a/drivers/ubloxmodem/ubloxmodem.h b/drivers/ubloxmodem/ubloxmodem.h
new file mode 100644
index 0000000..0c8a621
--- /dev/null
+++ b/drivers/ubloxmodem/ubloxmodem.h
@@ -0,0 +1,25 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#include <drivers/atmodem/atutil.h>
+
+extern void ublox_gprs_context_init(void);
+extern void ublox_gprs_context_exit(void);
-- 
2.5.0

Re: [PATCH 06/11] ubloxmodem: add Toby L2 gprs context driver

[Denis Kenzior]

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

Hi Dragos,

On 03/14/2016 10:50 AM, Dragos Tatulea wrote:
> For now the driver works only with bridged mode for 2G/3G.
>
> Once it activates the context it reads the ip, netmask,
> gw, dns and sets them in the context settings.
> ---
>   Makefile.am                       |   7 +
>   drivers/ubloxmodem/gprs-context.c | 415 ++++++++++++++++++++++++++++++++++++++
>   drivers/ubloxmodem/ubloxmodem.c   |  49 +++++
>   drivers/ubloxmodem/ubloxmodem.h   |  25 +++
>   4 files changed, 496 insertions(+)
>   create mode 100644 drivers/ubloxmodem/gprs-context.c
>   create mode 100644 drivers/ubloxmodem/ubloxmodem.c
>   create mode 100644 drivers/ubloxmodem/ubloxmodem.h
>
> diff --git a/Makefile.am b/Makefile.am
> index cde998d..7652cfe 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -374,6 +374,13 @@ builtin_sources += drivers/atmodem/atutil.h \
>   			drivers/speedupmodem/speedupmodem.c \
>   			drivers/speedupmodem/ussd.c
>
> +builtin_modules += ubloxmodem
> +builtin_sources += drivers/atmodem/atutil.h \
> +			drivers/ubloxmodem/ubloxmodem.h \
> +			drivers/ubloxmodem/ubloxmodem.c \
> +			drivers/ubloxmodem/gprs-context.c
> +
> +
>   if PHONESIM
>   builtin_modules += phonesim
>   builtin_sources += plugins/phonesim.c
> diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c
> new file mode 100644
> index 0000000..4b8fb6c
> --- /dev/null
> +++ b/drivers/ubloxmodem/gprs-context.c
> @@ -0,0 +1,415 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2016  EndoCode AG. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/gprs-context.h>
> +
> +#include "gatchat.h"
> +#include "gatresult.h"
> +
> +#include "ubloxmodem.h"
> +
> +static const char *none_prefix[] = { NULL };
> +static const char *cgcontrdp_prefix[] = { "+CGCONTRDP:", NULL };
> +
> +struct gprs_context_data {
> +	GAtChat *chat;
> +	unsigned int active_context;
> +	char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
> +	ofono_gprs_context_cb_t cb;
> +	void *cb_data;
> +};
> +
> +#define UBLOX_MAX_DEF_CONTEXT 8

What does this do?  Doesn't seem to be used in this file.

> +
> +/*
> + * CGCONTRDP returns addr + netmask in the same string in the form
> + * of "a.b.c.d.m.m.m.m" for IPv4. IPv6 is not supported so we ignore it.
> + */
> +static int read_addrnetmask(struct ofono_gprs_context *gc,
> +				const char *addrnetmask)

Since this function sets both parameters, how about:

set_address_and_netmask()

> +{
> +	char *dup = strdup(addrnetmask);
> +	char *s = dup;
> +
> +	const char *addr = s;
> +	const char *netmask = NULL;
> +
> +	int ret = -EINVAL;
> +	int i;
> +
> +	/* Count 7 dots for ipv4, less or more means error. */
> +	for (i = 0; i < 8; i++, s++) {
> +		s = strchr(s, '.');
> +		if (!s)
> +			break;
> +
> +		if (i == 3) {
> +			/* set netmask ptr and break the string */
> +			netmask = s+1;
> +			s[0] = 0;
> +		}
> +	}
> +
> +	if (i == 7) {
> +		ofono_gprs_context_set_ipv4_address(gc, addr, 1);
> +		ofono_gprs_context_set_ipv4_netmask(gc, netmask);
> +
> +		ret = 0;
> +	}
> +
> +	free(dup);
> +
> +	return ret;
> +}
> +
> +static void callback_with_error(struct gprs_context_data *gcd, GAtResult *res)

Please inline this function to be more consistent with the rest of the 
codebase.

> +{
> +	struct ofono_error error;
> +
> +	decode_at_error(&error, g_at_result_final_response(res));
> +	gcd->cb(&error, gcd->cb_data);
> +}
> +
> +

1 empty line is enough. We don't use more than 1 empty line in a row. 
Yes, we're that pedantic :)

> +static void set_gprs_context_interface(struct ofono_gprs_context *gc)
> +{
> +	struct ofono_modem *modem;
> +	const char *interface;
> +
> +	/* read interface name read at detection time */
> +	modem = ofono_gprs_context_get_modem(gc);
> +	interface = ofono_modem_get_string(modem, "NetworkInterface");
> +	ofono_gprs_context_set_interface(gc, interface);
> +}
> +
> +static void cgcontrdp_bridge_cb(gboolean ok, GAtResult *result,
> +				gpointer user_data)
> +{
> +	struct ofono_gprs_context *gc = user_data;
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	GAtResultIter iter;
> +	int cid = -1;
> +
> +	const char *laddrnetmask = NULL;
> +	const char *gw = NULL;
> +	const char *dns[2+1] = { NULL, NULL, NULL };

Do dns[3] here.

> +
> +	DBG("ok %d", ok);
> +
> +	if (!ok) {
> +		callback_with_error(gcd, result);

as mentioned above, lets inline the contents of callback_with_error here:
e.g.
decode_at_error(...);
cb(...)

> +
> +		return;
> +	}
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	while (g_at_result_iter_next(&iter, "+CGCONTRDP:")) {
> +		/* tmp vals for ignored fields */
> +		int bearer_id;
> +		const char *apn;
> +
> +		if (!g_at_result_iter_next_number(&iter, &cid))
> +			break;
> +
> +		if (!g_at_result_iter_next_number(&iter, &bearer_id))
> +			break;
> +
> +		if (!g_at_result_iter_next_string(&iter, &apn))
> +			break;
> +
> +		if (!g_at_result_iter_next_string(&iter, &laddrnetmask))
> +			break;
> +
> +		if (!g_at_result_iter_next_string(&iter, &gw))
> +			break;
> +
> +		if (!g_at_result_iter_next_string(&iter, &dns[0]))
> +			break;
> +
> +		if (!g_at_result_iter_next_string(&iter, &dns[1]))
> +			break;
> +	}
> +
> +	set_gprs_context_interface(gc);
> +
> +	if (!laddrnetmask || read_addrnetmask(gc, laddrnetmask) < 0) {
> +		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> +		return;
> +	}
> +
> +	if (gw)
> +		ofono_gprs_context_set_ipv4_gateway(gc, gw);
> +
> +	if (dns[0])
> +		ofono_gprs_context_set_ipv4_dns_servers(gc, dns);
> +
> +	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> +}
> +
> +static int ublox_read_ip_config_bridge(struct ofono_gprs_context *gc)
> +{
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	char buf[64];
> +
> +	/* read ip configuration info */
> +	snprintf(buf, sizeof(buf), "AT+CGCONTRDP=%u", gcd->active_context);
> +	return g_at_chat_send(gcd->chat, buf, cgcontrdp_prefix,
> +				cgcontrdp_bridge_cb, gc, NULL);
> +
> +}
> +
> +static void ublox_post_activation(struct ofono_gprs_context *gc)
> +{
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +
> +	if (ublox_read_ip_config_bridge(gc) < 0)
> +		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);

Since this is a two line function, it would be easier to read the code 
if it is moved into cgact_enable_cb

In fact, just sending CGCONTRDP inside cgact_enable_cb would be the 
simplest.

> +}
> +
> +static void cgact_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs_context *gc = user_data;
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +
> +	DBG("ok %d", ok);
> +	if (!ok) {
> +		gcd->active_context = 0;
> +		callback_with_error(gcd, result);
> +
> +		return;
> +	}
> +
> +	ublox_post_activation(gc);
> +}
> +
> +static void cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs_context *gc = user_data;
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	char buf[64];
> +
> +	DBG("ok %d", ok);
> +
> +	if (!ok) {
> +		gcd->active_context = 0;
> +		callback_with_error(gcd, result);
> +
> +		return;
> +	}
> +
> +	snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);
> +	if (g_at_chat_send(gcd->chat, buf, none_prefix,
> +				cgact_enable_cb, gc, NULL))
> +		return;
> +
> +	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> +}
> +
> +static void ublox_activate_ctx(struct ofono_gprs_context *gc)

Lets name this ublox_send_cgdcont, so it is clear what this helper 
function actually does

> +{
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
> +	int len;
> +
> +	len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"",
> +				gcd->active_context);
> +
> +	if (gcd->apn)
> +		snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"",
> +					gcd->apn);
> +
> +	if (g_at_chat_send(gcd->chat, buf, none_prefix,
> +				cgdcont_cb, gc, NULL) > 0)
> +		return;
> +
> +	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> +}
> +
> +#define UBLOX_MAX_USER_LEN 50
> +#define UBLOX_MAX_PASS_LEN 50
> +

Why is this defined here?  Does it belong in a later patch?

> +static void ublox_gprs_activate_primary(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;
> +	if (!gcd->active_context) {
> +		ofono_error("can't activate more contexts");
> +		CALLBACK_WITH_FAILURE(cb, data);
> +		return;
> +	}
> +
> +	gcd->cb = cb;
> +	gcd->cb_data = data;
> +	memcpy(gcd->apn, ctx->apn, sizeof(ctx->apn));

Why do we memcpy the APN into this structure?  If you don't want to make 
this function too long, then just pass the apn as a parameter to 
ublox_activate_ctx.

> +
> +	ublox_activate_ctx(gc);
> +}
> +
> +static void cgact_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs_context *gc = user_data;
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +
> +	DBG("ok %d", ok);

Empty line here after DBG, see item M1 in doc/coding-style.txt

> +	if (!ok) {
> +		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> +		return;
> +	}
> +
> +	gcd->active_context = 0;
> +
> +	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> +}
> +
> +static void ublox_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);
> +	char buf[64];
> +
> +	DBG("cid %u", cid);
> +
> +	gcd->cb = cb;
> +	gcd->cb_data = data;
> +
> +	snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", gcd->active_context);
> +	g_at_chat_send(gcd->chat, buf, none_prefix,
> +			cgact_disable_cb, gc, NULL);
> +}
> +
> +static void cgev_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs_context *gc = user_data;
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	GAtResultIter iter;
> +	const char *event;
> +	gint cid;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CGEV:"))
> +		return;
> +
> +	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))
> +		return;
> +
> +	DBG("cid %d", cid);
> +
> +	if ((unsigned int) cid != gcd->active_context)
> +		return;
> +
> +	ofono_gprs_context_deactivated(gc, gcd->active_context);
> +	gcd->active_context = 0;
> +}
> +
> +
> +static int ublox_gprs_context_probe(struct ofono_gprs_context *gc,
> +					unsigned int vendor, void *data)
> +{
> +	GAtChat *chat = data;
> +	struct gprs_context_data *gcd;
> +
> +	DBG("");
> +
> +	gcd = g_try_new0(struct gprs_context_data, 1);
> +	if (gcd == NULL)
> +		return -ENOMEM;
> +
> +	gcd->chat = g_at_chat_clone(chat);
> +
> +	ofono_gprs_context_set_data(gc, gcd);
> +
> +	g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
> +
> +	return 0;
> +}
> +
> +static void ublox_gprs_context_remove(struct ofono_gprs_context *gc)
> +{
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +
> +	DBG("");
> +
> +	ofono_gprs_context_set_data(gc, NULL);
> +
> +	g_at_chat_unref(gcd->chat);
> +
> +	memset(gcd, 0, sizeof(*gcd));
> +}
> +
> +static struct ofono_gprs_context_driver driver = {
> +	.name			= "ubloxmodem",
> +	.probe			= ublox_gprs_context_probe,
> +	.remove			= ublox_gprs_context_remove,
> +	.activate_primary	= ublox_gprs_activate_primary,
> +	.deactivate_primary	= ublox_gprs_deactivate_primary,
> +};
> +
> +


> +void ublox_gprs_context_init(void)
> +{
> +	ofono_gprs_context_driver_register(&driver);
> +}
> +
> +void ublox_gprs_context_exit(void)
> +{
> +	ofono_gprs_context_driver_unregister(&driver);
> +}

<snip>

Looks great. Lets fix these minor nitpicks and I'll apply it.

Regards,
-Denis

Re: [PATCH 06/11] ubloxmodem: add Toby L2 gprs context driver

[Dragos Tatulea]

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

Hi Denis,

Thanks for looking into this. Please see my 2 objections below.

On 03/17/2016 12:25 AM, Denis Kenzior wrote:
> Hi Dragos,
> 
> On 03/14/2016 10:50 AM, Dragos Tatulea wrote:
>> For now the driver works only with bridged mode for 2G/3G.
>>
>> Once it activates the context it reads the ip, netmask,
>> gw, dns and sets them in the context settings.
>> ---
>>   Makefile.am                       |   7 +
>>   drivers/ubloxmodem/gprs-context.c | 415 ++++++++++++++++++++++++++++++++++++++
>>   drivers/ubloxmodem/ubloxmodem.c   |  49 +++++
>>   drivers/ubloxmodem/ubloxmodem.h   |  25 +++
>>   4 files changed, 496 insertions(+)
>>   create mode 100644 drivers/ubloxmodem/gprs-context.c
>>   create mode 100644 drivers/ubloxmodem/ubloxmodem.c
>>   create mode 100644 drivers/ubloxmodem/ubloxmodem.h
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index cde998d..7652cfe 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -374,6 +374,13 @@ builtin_sources += drivers/atmodem/atutil.h \
>>               drivers/speedupmodem/speedupmodem.c \
>>               drivers/speedupmodem/ussd.c
>>
>> +builtin_modules += ubloxmodem
>> +builtin_sources += drivers/atmodem/atutil.h \
>> +            drivers/ubloxmodem/ubloxmodem.h \
>> +            drivers/ubloxmodem/ubloxmodem.c \
>> +            drivers/ubloxmodem/gprs-context.c
>> +
>> +
>>   if PHONESIM
>>   builtin_modules += phonesim
>>   builtin_sources += plugins/phonesim.c
>> diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c
>> new file mode 100644
>> index 0000000..4b8fb6c
>> --- /dev/null
>> +++ b/drivers/ubloxmodem/gprs-context.c
>> @@ -0,0 +1,415 @@
>> +/*
>> + *
>> + *  oFono - Open Source Telephony
>> + *
>> + *  Copyright (C) 2016  EndoCode AG. All rights reserved.
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License version 2 as
>> + *  published by the Free Software Foundation.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with this program; if not, write to the Free Software
>> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> + *
>> + */
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#define _GNU_SOURCE
>> +#include <string.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <stdbool.h>
>> +#include <errno.h>
>> +
>> +#include <glib.h>
>> +
>> +#include <ofono/log.h>
>> +#include <ofono/modem.h>
>> +#include <ofono/gprs-context.h>
>> +
>> +#include "gatchat.h"
>> +#include "gatresult.h"
>> +
>> +#include "ubloxmodem.h"
>> +
>> +static const char *none_prefix[] = { NULL };
>> +static const char *cgcontrdp_prefix[] = { "+CGCONTRDP:", NULL };
>> +
>> +struct gprs_context_data {
>> +    GAtChat *chat;
>> +    unsigned int active_context;
>> +    char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
>> +    ofono_gprs_context_cb_t cb;
>> +    void *cb_data;
>> +};
>> +
>> +#define UBLOX_MAX_DEF_CONTEXT 8
> 
> What does this do?  Doesn't seem to be used in this file.
> 
>> +
>> +/*
>> + * CGCONTRDP returns addr + netmask in the same string in the form
>> + * of "a.b.c.d.m.m.m.m" for IPv4. IPv6 is not supported so we ignore it.
>> + */
>> +static int read_addrnetmask(struct ofono_gprs_context *gc,
>> +                const char *addrnetmask)
> 
> Since this function sets both parameters, how about:
> 
> set_address_and_netmask()
> 
>> +{
>> +    char *dup = strdup(addrnetmask);
>> +    char *s = dup;
>> +
>> +    const char *addr = s;
>> +    const char *netmask = NULL;
>> +
>> +    int ret = -EINVAL;
>> +    int i;
>> +
>> +    /* Count 7 dots for ipv4, less or more means error. */
>> +    for (i = 0; i < 8; i++, s++) {
>> +        s = strchr(s, '.');
>> +        if (!s)
>> +            break;
>> +
>> +        if (i == 3) {
>> +            /* set netmask ptr and break the string */
>> +            netmask = s+1;
>> +            s[0] = 0;
>> +        }
>> +    }
>> +
>> +    if (i == 7) {
>> +        ofono_gprs_context_set_ipv4_address(gc, addr, 1);
>> +        ofono_gprs_context_set_ipv4_netmask(gc, netmask);
>> +
>> +        ret = 0;
>> +    }
>> +
>> +    free(dup);
>> +
>> +    return ret;
>> +}
>> +
>> +static void callback_with_error(struct gprs_context_data *gcd, GAtResult *res)
> 
> Please inline this function to be more consistent with the rest of the codebase.
> 
>> +{
>> +    struct ofono_error error;
>> +
>> +    decode_at_error(&error, g_at_result_final_response(res));
>> +    gcd->cb(&error, gcd->cb_data);
>> +}
>> +
>> +
> 
> 1 empty line is enough. We don't use more than 1 empty line in a row. Yes, we're that pedantic :)
> 
>> +static void set_gprs_context_interface(struct ofono_gprs_context *gc)
>> +{
>> +    struct ofono_modem *modem;
>> +    const char *interface;
>> +
>> +    /* read interface name read at detection time */
>> +    modem = ofono_gprs_context_get_modem(gc);
>> +    interface = ofono_modem_get_string(modem, "NetworkInterface");
>> +    ofono_gprs_context_set_interface(gc, interface);
>> +}
>> +
>> +static void cgcontrdp_bridge_cb(gboolean ok, GAtResult *result,
>> +                gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    GAtResultIter iter;
>> +    int cid = -1;
>> +
>> +    const char *laddrnetmask = NULL;
>> +    const char *gw = NULL;
>> +    const char *dns[2+1] = { NULL, NULL, NULL };
> 
> Do dns[3] here.
> 
>> +
>> +    DBG("ok %d", ok);
>> +
>> +    if (!ok) {
>> +        callback_with_error(gcd, result);
> 
> as mentioned above, lets inline the contents of callback_with_error here:
> e.g.
> decode_at_error(...);
> cb(...)
> 
>> +
>> +        return;
>> +    }
>> +
>> +    g_at_result_iter_init(&iter, result);
>> +
>> +    while (g_at_result_iter_next(&iter, "+CGCONTRDP:")) {
>> +        /* tmp vals for ignored fields */
>> +        int bearer_id;
>> +        const char *apn;
>> +
>> +        if (!g_at_result_iter_next_number(&iter, &cid))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_number(&iter, &bearer_id))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &apn))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &laddrnetmask))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &gw))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &dns[0]))
>> +            break;
>> +
>> +        if (!g_at_result_iter_next_string(&iter, &dns[1]))
>> +            break;
>> +    }
>> +
>> +    set_gprs_context_interface(gc);
>> +
>> +    if (!laddrnetmask || read_addrnetmask(gc, laddrnetmask) < 0) {
>> +        CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>> +        return;
>> +    }
>> +
>> +    if (gw)
>> +        ofono_gprs_context_set_ipv4_gateway(gc, gw);
>> +
>> +    if (dns[0])
>> +        ofono_gprs_context_set_ipv4_dns_servers(gc, dns);
>> +
>> +    CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
>> +}
>> +
>> +static int ublox_read_ip_config_bridge(struct ofono_gprs_context *gc)
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    char buf[64];
>> +
>> +    /* read ip configuration info */
>> +    snprintf(buf, sizeof(buf), "AT+CGCONTRDP=%u", gcd->active_context);
>> +    return g_at_chat_send(gcd->chat, buf, cgcontrdp_prefix,
>> +                cgcontrdp_bridge_cb, gc, NULL);
>> +
>> +}
>> +
>> +static void ublox_post_activation(struct ofono_gprs_context *gc)
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    if (ublox_read_ip_config_bridge(gc) < 0)
>> +        CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> 
> Since this is a two line function, it would be easier to read the code if it is moved into cgact_enable_cb
> 
> In fact, just sending CGCONTRDP inside cgact_enable_cb would be the simplest.

The next patches will need this as a standalone function. Another line for reading the router mode ip config will be introduced. And the function will be called from 3 places:
* directly in pri_context_activate (this patch)
* after authentication command (authentication support patch)
* from read_settings function (automatic context activation patch)

> 
>> +}
>> +
>> +static void cgact_enable_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    DBG("ok %d", ok);
>> +    if (!ok) {
>> +        gcd->active_context = 0;
>> +        callback_with_error(gcd, result);
>> +
>> +        return;
>> +    }
>> +
>> +    ublox_post_activation(gc);
>> +}
>> +
>> +static void cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    char buf[64];
>> +
>> +    DBG("ok %d", ok);
>> +
>> +    if (!ok) {
>> +        gcd->active_context = 0;
>> +        callback_with_error(gcd, result);
>> +
>> +        return;
>> +    }
>> +
>> +    snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);
>> +    if (g_at_chat_send(gcd->chat, buf, none_prefix,
>> +                cgact_enable_cb, gc, NULL))
>> +        return;
>> +
>> +    CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>> +}
>> +
>> +static void ublox_activate_ctx(struct ofono_gprs_context *gc)
> 
> Lets name this ublox_send_cgdcont, so it is clear what this helper function actually does
> 
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
>> +    int len;
>> +
>> +    len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"",
>> +                gcd->active_context);
>> +
>> +    if (gcd->apn)
>> +        snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"",
>> +                    gcd->apn);
>> +
>> +    if (g_at_chat_send(gcd->chat, buf, none_prefix,
>> +                cgdcont_cb, gc, NULL) > 0)
>> +        return;
>> +
>> +    CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>> +}
>> +
>> +#define UBLOX_MAX_USER_LEN 50
>> +#define UBLOX_MAX_PASS_LEN 50
>> +
> 
> Why is this defined here?  Does it belong in a later patch?
> 
>> +static void ublox_gprs_activate_primary(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;
>> +    if (!gcd->active_context) {
>> +        ofono_error("can't activate more contexts");
>> +        CALLBACK_WITH_FAILURE(cb, data);
>> +        return;
>> +    }
>> +
>> +    gcd->cb = cb;
>> +    gcd->cb_data = data;
>> +    memcpy(gcd->apn, ctx->apn, sizeof(ctx->apn));
> 
> Why do we memcpy the APN into this structure?  If you don't want to make this function too long, then just pass the apn as a parameter to ublox_activate_ctx.
> 
The APN will be required by another patch that sends authentication command and then sends CGDCONT.

>> +
>> +    ublox_activate_ctx(gc);
>> +}
>> +
>> +static void cgact_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    DBG("ok %d", ok);
> 
> Empty line here after DBG, see item M1 in doc/coding-style.txt
> 
>> +    if (!ok) {
>> +        CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>> +        return;
>> +    }
>> +
>> +    gcd->active_context = 0;
>> +
>> +    CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
>> +}
>> +
>> +static void ublox_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);
>> +    char buf[64];
>> +
>> +    DBG("cid %u", cid);
>> +
>> +    gcd->cb = cb;
>> +    gcd->cb_data = data;
>> +
>> +    snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", gcd->active_context);
>> +    g_at_chat_send(gcd->chat, buf, none_prefix,
>> +            cgact_disable_cb, gc, NULL);
>> +}
>> +
>> +static void cgev_notify(GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_gprs_context *gc = user_data;
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +    GAtResultIter iter;
>> +    const char *event;
>> +    gint cid;
>> +
>> +    g_at_result_iter_init(&iter, result);
>> +
>> +    if (!g_at_result_iter_next(&iter, "+CGEV:"))
>> +        return;
>> +
>> +    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))
>> +        return;
>> +
>> +    DBG("cid %d", cid);
>> +
>> +    if ((unsigned int) cid != gcd->active_context)
>> +        return;
>> +
>> +    ofono_gprs_context_deactivated(gc, gcd->active_context);
>> +    gcd->active_context = 0;
>> +}
>> +
>> +
>> +static int ublox_gprs_context_probe(struct ofono_gprs_context *gc,
>> +                    unsigned int vendor, void *data)
>> +{
>> +    GAtChat *chat = data;
>> +    struct gprs_context_data *gcd;
>> +
>> +    DBG("");
>> +
>> +    gcd = g_try_new0(struct gprs_context_data, 1);
>> +    if (gcd == NULL)
>> +        return -ENOMEM;
>> +
>> +    gcd->chat = g_at_chat_clone(chat);
>> +
>> +    ofono_gprs_context_set_data(gc, gcd);
>> +
>> +    g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
>> +
>> +    return 0;
>> +}
>> +
>> +static void ublox_gprs_context_remove(struct ofono_gprs_context *gc)
>> +{
>> +    struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +
>> +    DBG("");
>> +
>> +    ofono_gprs_context_set_data(gc, NULL);
>> +
>> +    g_at_chat_unref(gcd->chat);
>> +
>> +    memset(gcd, 0, sizeof(*gcd));
>> +}
>> +
>> +static struct ofono_gprs_context_driver driver = {
>> +    .name            = "ubloxmodem",
>> +    .probe            = ublox_gprs_context_probe,
>> +    .remove            = ublox_gprs_context_remove,
>> +    .activate_primary    = ublox_gprs_activate_primary,
>> +    .deactivate_primary    = ublox_gprs_deactivate_primary,
>> +};
>> +
>> +
> 
> 
>> +void ublox_gprs_context_init(void)
>> +{
>> +    ofono_gprs_context_driver_register(&driver);
>> +}
>> +
>> +void ublox_gprs_context_exit(void)
>> +{
>> +    ofono_gprs_context_driver_unregister(&driver);
>> +}
> 
> <snip>
> 
> Looks great. Lets fix these minor nitpicks and I'll apply it.
> 
> Regards,
> -Denis
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono

Thanks
-- 
Dragos Tatulea
Software Developer @ Endocode AG
dragos(a)endocode.com

Endocode AG, Brückenstraße 5A, 10179 Berlin
+49 30 1206 4472 | info(a)endocode.com | www.endocode.com

Vorstandsvorsitzender: Mirko Boehm
Vorstände: Dr. Thomas Fricke, Sebastian Sucker
Aufsichtsratsvorsitzende: Alexandra Boehm

Registergericht: Amtsgericht Charlottenburg - HRB 150748 B

Re: [PATCH 06/11] ubloxmodem: add Toby L2 gprs context driver

[Denis Kenzior]

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

Hi Dragos,

 >>
>> Since this is a two line function, it would be easier to read the code if it is moved into cgact_enable_cb
>>
>> In fact, just sending CGCONTRDP inside cgact_enable_cb would be the simplest.
>
> The next patches will need this as a standalone function. Another line for reading the router mode ip config will be introduced. And the function will be called from 3 places:
> * directly in pri_context_activate (this patch)
> * after authentication command (authentication support patch)
> * from read_settings function (automatic context activation patch)
>

Okay, go ahead with your proposal.

<snip>

>>> +    gcd->cb = cb;
>>> +    gcd->cb_data = data;
>>> +    memcpy(gcd->apn, ctx->apn, sizeof(ctx->apn));
>>
>> Why do we memcpy the APN into this structure?  If you don't want to make this function too long, then just pass the apn as a parameter to ublox_activate_ctx.
>>
> The APN will be required by another patch that sends authentication command and then sends CGDCONT.
>

Okay, but then logically it should be part of that patch :)

We prefer that data structure members are added on as needed basis.  So 
it becomes much easier to understand what is used how when reviewing 
large patch sets.

This is also useful in e.g. git blame and git show when reviewing what 
was done previously and for what reason.

Regards,
-Denis

[PATCH 07/11] plugins/ublox: enable ubloxmodem driver when possible

[Dragos Tatulea]

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

Where possible means Toby L2 in high speed mode.

The bridge mode is set before enabling the modem because the
driver requires this.
---
 plugins/ublox.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/plugins/ublox.c b/plugins/ublox.c
index 23ed2cb..c47c6aa 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -142,6 +142,11 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 		return;
 	}
 
+	if (data->model_id == TOBYL2_HIGH_THROUGHPUT_MODE)
+		/* use bridged mode until routed mode support is added */
+		g_at_chat_send(data->aux, "AT+UBMCONF=2", none_prefix,
+						NULL, NULL, NULL);
+
 	ofono_modem_set_powered(modem, TRUE);
 }
 
@@ -289,13 +294,19 @@ static void ublox_post_sim(struct ofono_modem *modem)
 	struct ublox_data *data = ofono_modem_get_data(modem);
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
+	GAtChat *chat = data->modem ? data->modem : data->aux;
 
 	DBG("%p", modem);
 
 	gprs = ofono_gprs_create(modem, data->vendor_family, "atmodem",
 					data->aux);
-	gc = ofono_gprs_context_create(modem, data->vendor_family, "atmodem",
-					data->modem ? data->modem : data->aux);
+	if (data->model_id == TOBYL2_HIGH_THROUGHPUT_MODE)
+		gc = ofono_gprs_context_create(modem, data->vendor_family,
+						"ubloxmodem", chat);
+
+	else
+		gc = ofono_gprs_context_create(modem, data->vendor_family,
+						"atmodem", chat);
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
-- 
2.5.0

[PATCH 08/11] plugins/ublox: support more internet contexts

[Dragos Tatulea]

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

Create multiple gprs-context instances and let the gprs core use
them as it sees fit.

Only for Toby L2.
---
 plugins/ublox.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/plugins/ublox.c b/plugins/ublox.c
index c47c6aa..8f6b3f1 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -295,21 +295,25 @@ static void ublox_post_sim(struct ofono_modem *modem)
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
 	GAtChat *chat = data->modem ? data->modem : data->aux;
+	const char *driver = data->model_id == TOBYL2_HIGH_THROUGHPUT_MODE ?
+						"ubloxmodem" : "atmodem";
+	/* Toby L2: Create same number of contexts as supported PDP contexts. */
+	int ncontexts = data->model_id == TOBYL2_HIGH_THROUGHPUT_MODE ? 8 : 1;
 
 	DBG("%p", modem);
 
 	gprs = ofono_gprs_create(modem, data->vendor_family, "atmodem",
 					data->aux);
-	if (data->model_id == TOBYL2_HIGH_THROUGHPUT_MODE)
-		gc = ofono_gprs_context_create(modem, data->vendor_family,
-						"ubloxmodem", chat);
 
-	else
+	while (ncontexts) {
 		gc = ofono_gprs_context_create(modem, data->vendor_family,
-						"atmodem", chat);
+						driver, chat);
 
-	if (gprs && gc)
-		ofono_gprs_add_context(gprs, gc);
+		if (gprs && gc)
+			ofono_gprs_add_context(gprs, gc);
+
+		--ncontexts;
+	}
 }
 
 static void ublox_post_online(struct ofono_modem *modem)
-- 
2.5.0

[PATCH 09/11] ubloxmodem: support authentication

[Dragos Tatulea]

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

If username and password specified, issue an UAUTHREQ
command with the configured authentication method, selected cid
and credentials.
---
 drivers/ubloxmodem/gprs-context.c | 61 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c
index 4b8fb6c..1a410b0 100644
--- a/drivers/ubloxmodem/gprs-context.c
+++ b/drivers/ubloxmodem/gprs-context.c
@@ -48,6 +48,9 @@ struct gprs_context_data {
 	GAtChat *chat;
 	unsigned int active_context;
 	char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
+	char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
+	char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
+	enum ofono_gprs_auth_method auth_method;
 	ofono_gprs_context_cb_t cb;
 	void *cb_data;
 };
@@ -259,9 +262,58 @@ static void ublox_activate_ctx(struct ofono_gprs_context *gc)
 	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
 }
 
+static void uauthreq_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		ofono_error("can't authenticate");
+		gcd->active_context = 0;
+		callback_with_error(gcd, result);
+
+		return;
+	}
+
+	ublox_activate_ctx(gc);
+}
+
 #define UBLOX_MAX_USER_LEN 50
 #define UBLOX_MAX_PASS_LEN 50
 
+static void ublox_authenticate(struct ofono_gprs_context *gc)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[UBLOX_MAX_USER_LEN + UBLOX_MAX_PASS_LEN + 32];
+	unsigned auth_method;
+
+	switch (gcd->auth_method) {
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		auth_method = 1;
+		break;
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+		auth_method = 2;
+		break;
+	default:
+		ofono_error("Unsupported auth type %u", gcd->auth_method);
+		goto error;
+	}
+
+	snprintf(buf, sizeof(buf), "AT+UAUTHREQ=%u,%u,\"%s\",\"%s\"",
+			gcd->active_context, auth_method,
+			gcd->username, gcd->password);
+
+	/* If this failed, we will see it during context activation. */
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				uauthreq_cb, gc, NULL) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+}
+
 static void ublox_gprs_activate_primary(struct ofono_gprs_context *gc,
 				const struct ofono_gprs_primary_context *ctx,
 				ofono_gprs_context_cb_t cb, void *data)
@@ -285,9 +337,16 @@ static void ublox_gprs_activate_primary(struct ofono_gprs_context *gc,
 
 	gcd->cb = cb;
 	gcd->cb_data = data;
+
+	gcd->auth_method = ctx->auth_method;
 	memcpy(gcd->apn, ctx->apn, sizeof(ctx->apn));
 
-	ublox_activate_ctx(gc);
+	if (strlen(ctx->username) && strlen(ctx->password)) {
+		memcpy(gcd->username, ctx->username, sizeof(ctx->username));
+		memcpy(gcd->password, ctx->password, sizeof(ctx->password));
+		ublox_authenticate(gc);
+	} else
+		ublox_activate_ctx(gc);
 }
 
 static void cgact_disable_cb(gboolean ok, GAtResult *result, gpointer user_data)
-- 
2.5.0

[PATCH 10/11] plugins/ublox: read network mode

[Dragos Tatulea]

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

From: Dongsu Park <dongsu@endocode.com>

Read network mode into modem string. This will let the gprs-context
drive know what to do.
---
 plugins/ublox.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/plugins/ublox.c b/plugins/ublox.c
index 8f6b3f1..b71e2c0 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -42,6 +42,7 @@
 #include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
 
+static const char *ubmconf_prefix[] = { "+UBMCONF:", NULL };
 static const char *none_prefix[] = { NULL };
 
 enum supported_models {
@@ -52,11 +53,17 @@ enum supported_models {
 	TOBYL2_HIGH_THROUGHPUT_MODE 	= 1146,
 };
 
+enum ublox_net_mode {
+	UBLOX_TOBYL2_NET_MODE_ROUTER = 1,
+	UBLOX_TOBYL2_NET_MODE_BRIDGE = 2,
+};
+
 struct ublox_data {
 	GAtChat *modem;
 	GAtChat *aux;
 	int model_id;
 	enum ofono_vendor vendor_family;
+	enum ublox_net_mode net_mode;
 };
 
 static void ublox_debug(const char *str, void *user_data)
@@ -93,6 +100,39 @@ static void ublox_remove(struct ofono_modem *modem)
 	g_free(data);
 }
 
+static void read_ubmconf_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = (struct ofono_modem *) user_data;
+	struct ublox_data *data = ofono_modem_get_data(modem);
+	int mode = 0;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	while (!g_at_result_iter_next(&iter, "+UBMCONF:"))
+		;	/* skip every other line that is not UBMCONF */
+
+	g_at_result_iter_next_number(&iter, &mode);
+
+	data->net_mode = mode;
+	DBG("mode=%d", mode);
+	if (mode == 1)
+		ofono_modem_set_string(modem, "NetworkMode", "routed");
+	else if (mode == 2)
+		ofono_modem_set_string(modem, "NetworkMode", "bridged");
+}
+
+static void read_net_mode(struct ofono_modem *modem)
+{
+	struct ublox_data *data = ofono_modem_get_data(modem);
+
+	if (!data->aux)
+		return;
+
+	g_at_chat_send(data->aux, "AT+UBMCONF?",
+			ubmconf_prefix, read_ubmconf_cb, modem, NULL);
+}
+
 static GAtChat *open_device(struct ofono_modem *modem,
 				const char *key, char *debug)
 {
@@ -142,11 +182,6 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 		return;
 	}
 
-	if (data->model_id == TOBYL2_HIGH_THROUGHPUT_MODE)
-		/* use bridged mode until routed mode support is added */
-		g_at_chat_send(data->aux, "AT+UBMCONF=2", none_prefix,
-						NULL, NULL, NULL);
-
 	ofono_modem_set_powered(modem, TRUE);
 }
 
@@ -206,6 +241,8 @@ static int ublox_enable(struct ofono_modem *modem)
 	g_at_chat_send(data->aux, "ATE0 +CMEE=1", none_prefix,
 					NULL, NULL, NULL);
 
+	read_net_mode(modem);
+
 	g_at_chat_send(data->aux, "AT+CFUN=4", none_prefix,
 					cfun_enable, modem, NULL);
 
-- 
2.5.0

[PATCH 11/11] ubloxmodem: add routed mode support

[Dragos Tatulea]

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

From: Dongsu Park <dongsu@endocode.com>

Routed mode needs a different treatment than bridged mode:

* UIPCONF needs to be used for reading the interface ip configuratuion.
The spec says that DHCP should work on that interface but it doesn't. So
we read the first ip in the dhcp range and use that.

* CGCONTRDP: only APN and DNS configuration is read
---
 drivers/ubloxmodem/gprs-context.c | 142 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 1 deletion(-)

diff --git a/drivers/ubloxmodem/gprs-context.c b/drivers/ubloxmodem/gprs-context.c
index 1a410b0..40f02f0 100644
--- a/drivers/ubloxmodem/gprs-context.c
+++ b/drivers/ubloxmodem/gprs-context.c
@@ -43,6 +43,7 @@
 
 static const char *none_prefix[] = { NULL };
 static const char *cgcontrdp_prefix[] = { "+CGCONTRDP:", NULL };
+static const char *uipconf_prefix[] = { "+UIPCONF:", NULL };
 
 struct gprs_context_data {
 	GAtChat *chat;
@@ -183,6 +184,56 @@ static void cgcontrdp_bridge_cb(gboolean ok, GAtResult *result,
 	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
 }
 
+static void cgcontrdp_router_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	GAtResultIter iter;
+
+	const char *dns[2+1] = { NULL, NULL, NULL };
+	const char *apn = NULL;
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		callback_with_error(gcd, result);
+
+		return;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, "+CGCONTRDP:")) {
+		/* skip cid, bearer_id */
+		g_at_result_iter_skip_next(&iter);
+		g_at_result_iter_skip_next(&iter);
+
+		/* read apn */
+		if (!g_at_result_iter_next_string(&iter, &apn))
+			break;
+
+		/* skip laddrnetmask, gw */
+		g_at_result_iter_skip_next(&iter);
+		g_at_result_iter_skip_next(&iter);
+
+		/* read dns servers */
+		if (!g_at_result_iter_next_string(&iter, &dns[0]))
+			break;
+
+		if (!g_at_result_iter_next_string(&iter, &dns[1]))
+			break;
+	}
+
+	set_gprs_context_interface(gc);
+
+	if (dns[0])
+		ofono_gprs_context_set_ipv4_dns_servers(gc, dns);
+
+	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+	return;
+}
+
 static int ublox_read_ip_config_bridge(struct ofono_gprs_context *gc)
 {
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
@@ -195,11 +246,100 @@ static int ublox_read_ip_config_bridge(struct ofono_gprs_context *gc)
 
 }
 
+static void read_uipconf_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	GAtResultIter iter;
+	const char *gw, *netmask, *ipaddr, *dhcp_range_start, *dhcp_range_end;
+	gboolean found = FALSE;
+	char buf[64];
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		gcd->active_context = 0;
+		callback_with_error(gcd, result);
+
+		return;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	/* for example, +UIPCONF: entry looks like:
+	 * +UIPCONF: "192.168.1.1","255.255.255.0","192.168.1.100",
+	 *           "192.168.1.100","fe80::48a5:b2ff:fe6f:5f86/64"
+	 */
+	while (g_at_result_iter_next(&iter, "+UIPCONF:")) {
+		if (!g_at_result_iter_next_string(&iter, &gw))
+			continue;
+
+		if (!g_at_result_iter_next_string(&iter, &netmask))
+			continue;
+
+		if (!g_at_result_iter_next_string(&iter, &dhcp_range_start))
+			continue;
+
+		if (!g_at_result_iter_next_string(&iter, &dhcp_range_end))
+			continue;
+
+		/* skip other entries like IPv6 networks */
+		found = TRUE;
+		break;
+	}
+
+	if (!found)
+		goto error;
+
+	if (dhcp_range_start && dhcp_range_end) {
+		ipaddr = dhcp_range_start;
+		ofono_gprs_context_set_ipv4_address(gc, ipaddr, 1);
+	}
+
+	if (netmask)
+		ofono_gprs_context_set_ipv4_netmask(gc, netmask);
+
+	if (gw)
+		ofono_gprs_context_set_ipv4_gateway(gc, gw);
+
+	/* read ip configuration info */
+	snprintf(buf, sizeof(buf), "AT+CGCONTRDP");
+	if (g_at_chat_send(gcd->chat, buf, cgcontrdp_prefix,
+				cgcontrdp_router_cb, gc, NULL) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+}
+
+static int ublox_read_ip_config_router(struct ofono_gprs_context *gc)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[64];
+
+	/* read ip configuration info */
+	snprintf(buf, sizeof(buf), "AT+UIPCONF?");
+	return g_at_chat_send(gcd->chat, buf, uipconf_prefix,
+				read_uipconf_cb, gc, NULL);
+
+}
+
 static void ublox_post_activation(struct ofono_gprs_context *gc)
 {
+	struct ofono_modem *modem;
+	const char *network_mode;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	int ret = 0;
+
+	modem = ofono_gprs_context_get_modem(gc);
+	network_mode = ofono_modem_get_string(modem, "NetworkMode");
+
+	if (g_str_equal(network_mode, "routed"))
+		ret = ublox_read_ip_config_router(gc);
+	else
+		ret = ublox_read_ip_config_bridge(gc);
 
-	if (ublox_read_ip_config_bridge(gc) < 0)
+	if (ret <= 0)
 		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
 }
 
-- 
2.5.0