[PATCH 0/5] gemalto: gprs context driver updates

[PATCH 0/5] gemalto: gprs context driver updates

[Sergey Matyukevich]

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

Hi Denis and all,

This patch series includes several updates and fixes for gemalto gprs
context driver based on testing and troubleshooting in several regions.
Major changes include support for automatic context activation and fixes
for gprs context deactivation.

Besides support for automatic context activation uncovered an issue with
normal activation sequence. According to modem specs, AT+CGACT should
not be used for contexts handled by AT^SWWAN. However wrong sequence
used to work when re-activating contexts already activated automatically,
while correct sequence produced incorrect modem DHCP responces.

Regards,
Sergey

Sergey Matyukevich (5):
  plugin: gemalto: fix source of gprs notifications
  gemalto: gprs: cgev gprs context deactivation
  gemalto: gprs: support automatic context activation
  gemalto: gprs: support different gprs protocols
  gemalto: gprs: support authentication settings

 drivers/gemaltomodem/gprs-context.c | 176 ++++++++++++++++++++--------
 plugins/gemalto.c                   |   2 +-
 2 files changed, 125 insertions(+), 53 deletions(-)

-- 
2.29.2

[PATCH 1/5] plugin: gemalto: fix source of gprs notifications

[Sergey Matyukevich]

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

Modem USB interface does not receive certain gprs context notifications.
Fix gprs chat: use Application USB interface to receive all the modem
notifications.
---
 plugins/gemalto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 238c7cc4..28ee3aff 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -606,7 +606,7 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 		driver = "atmodem";
 	}
 
-	gc = ofono_gprs_context_create(modem, 0, driver, data->mdm);
+	gc = ofono_gprs_context_create(modem, 0, driver, data->app);
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
-- 
2.29.2

[PATCH 2/5] gemalto: gprs: cgev gprs context deactivation

[Sergey Matyukevich]

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

Gemalto ELS81x modems use 'ME PDN DEACT' message to notify about gprs
context deactivation. Process this 'deactivate' event in CGEV handler.
---
 drivers/gemaltomodem/gprs-context.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gemaltomodem/gprs-context.c b/drivers/gemaltomodem/gprs-context.c
index 73cf81a8..322a5f98 100644
--- a/drivers/gemaltomodem/gprs-context.c
+++ b/drivers/gemaltomodem/gprs-context.c
@@ -216,6 +216,8 @@ static void cgev_notify(GAtResult *result, gpointer user_data)
 
 	if (g_str_has_prefix(event, "NW PDN DEACT"))
 		sscanf(event, "%*s %*s %*s %u", &cid);
+	else if (g_str_has_prefix(event, "ME PDN DEACT"))
+		sscanf(event, "%*s %*s %*s %u", &cid);
 	else if (g_str_has_prefix(event, "NW DEACT"))
 		sscanf(event, "%*s %*s %u", &cid);
 	else
-- 
2.29.2

[PATCH 3/5] gemalto: gprs: support automatic context activation

[Sergey Matyukevich]

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

From: Sergey Matyukevich <matyukevich@arrival.com>

Implement read_settings function to get configuration for automatic
contexts. Fix the issue uncovered by added support for automatic
context activation: do not use AT+CGACT for the contexts handled
by AT^SWWAN. As per modem specs, CGACT context can not be reused
for SWWAN. Though that worked for some reason when automatic
context was reactivated without proper deactivation.
---
 drivers/gemaltomodem/gprs-context.c | 110 +++++++++++++++-------------
 1 file changed, 59 insertions(+), 51 deletions(-)

diff --git a/drivers/gemaltomodem/gprs-context.c b/drivers/gemaltomodem/gprs-context.c
index 322a5f98..680f01ab 100644
--- a/drivers/gemaltomodem/gprs-context.c
+++ b/drivers/gemaltomodem/gprs-context.c
@@ -50,70 +50,61 @@ struct gprs_context_data {
 	void *cb_data;
 };
 
-static void cgact_enable_cb(gboolean ok, GAtResult *result,
-				gpointer user_data)
+static void set_gprs_context_interface(struct ofono_gprs_context *gc)
 {
-	struct ofono_gprs_context *gc = user_data;
-	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 	struct ofono_modem *modem;
 	const char *interface;
-	char buf[64];
+
+	modem = ofono_gprs_context_get_modem(gc);
+	interface = ofono_modem_get_string(modem, "NetworkInterface");
+	ofono_gprs_context_set_interface(gc, interface);
+}
+
+static void swwan_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);
+	struct ofono_error error;
 
 	DBG("ok %d", ok);
 
 	if (!ok) {
-		struct ofono_error error;
-
+		ofono_error("Unable to activate context");
+		ofono_gprs_context_deactivated(gc, gcd->active_context);
 		gcd->active_context = 0;
-
 		decode_at_error(&error, g_at_result_final_response(result));
 		gcd->cb(&error, gcd->cb_data);
-
 		return;
 	}
-
-	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
-	g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
-
-	modem = ofono_gprs_context_get_modem(gc);
-	interface = ofono_modem_get_string(modem, "NetworkInterface");
-	ofono_gprs_context_set_interface(gc, interface);
-
-	/* Use DHCP */
-	ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
-
-	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
 }
 
 static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
-				gpointer user_data)
+			gpointer user_data)
 {
 	struct ofono_gprs_context *gc = user_data;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	struct ofono_error error;
 	char buf[64];
 
 	DBG("ok %d", ok);
 
 	if (!ok) {
-		struct ofono_error error;
-
 		gcd->active_context = 0;
-
 		decode_at_error(&error, g_at_result_final_response(result));
 		gcd->cb(&error, gcd->cb_data);
-
 		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) == 0)
-		goto error;
+	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
+	if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {
+		set_gprs_context_interface(gc);
+		/* Use DHCP */
+		ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
 
-	return;
+		CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+		return;
+	}
 
-error:
 	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
 }
 
@@ -141,34 +132,28 @@ static void gemalto_gprs_activate_primary(struct ofono_gprs_context *gc,
 		snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"", ctx->apn);
 
 	if (g_at_chat_send(gcd->chat, buf, none_prefix,
-				cgdcont_enable_cb, gc, NULL) == 0)
-		goto error;
-
-	return;
+				cgdcont_enable_cb, gc, NULL))
+		return;
 
 error:
 	CALLBACK_WITH_FAILURE(cb, data);
 }
 
-static void cgact_disable_cb(gboolean ok, GAtResult *result,
-				gpointer user_data)
+static void deactivate_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);
 
+	gcd->active_context = 0;
+
 	if (!ok) {
 		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
 		return;
 	}
 
-	snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
-	g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
-
-	gcd->active_context = 0;
-
 	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
 }
 
@@ -185,17 +170,39 @@ static void gemalto_gprs_deactivate_primary(struct ofono_gprs_context *gc,
 	gcd->cb = cb;
 	gcd->cb_data = data;
 
-	snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", cid);
+	snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
 
 	if (g_at_chat_send(gcd->chat, buf, none_prefix,
-				cgact_disable_cb, gc, NULL) == 0)
-		goto error;
-
-	return;
+				deactivate_cb, gc, NULL))
+		return;
 
-error:
 	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void gemalto_gprs_read_settings(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->active_context = cid;
+	gcd->cb = cb;
+	gcd->cb_data = data;
+
+	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
+	if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {
+		set_gprs_context_interface(gc);
+		/* Use DHCP */
+		ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
 
+		CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+		return;
+	}
+
+	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
 }
 
 static void cgev_notify(GAtResult *result, gpointer user_data)
@@ -267,6 +274,7 @@ static const struct ofono_gprs_context_driver driver = {
 	.remove			= gemalto_gprs_context_remove,
 	.activate_primary	= gemalto_gprs_activate_primary,
 	.deactivate_primary	= gemalto_gprs_deactivate_primary,
+	.read_settings		= gemalto_gprs_read_settings,
 };
 
 void gemalto_gprs_context_init(void)
-- 
2.29.2

[PATCH 4/5] gemalto: gprs: support different gprs protocols

[Sergey Matyukevich]

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

From: Sergey Matyukevich <matyukevich@arrival.com>

Add support for IPv6 and dual mode gprs contexts.
---
 drivers/gemaltomodem/gprs-context.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gemaltomodem/gprs-context.c b/drivers/gemaltomodem/gprs-context.c
index 680f01ab..bb09f0b0 100644
--- a/drivers/gemaltomodem/gprs-context.c
+++ b/drivers/gemaltomodem/gprs-context.c
@@ -46,6 +46,7 @@ static const char *none_prefix[] = { NULL };
 struct gprs_context_data {
 	GAtChat *chat;
 	unsigned int active_context;
+	enum ofono_gprs_proto proto;
 	ofono_gprs_context_cb_t cb;
 	void *cb_data;
 };
@@ -118,15 +119,25 @@ static void gemalto_gprs_activate_primary(struct ofono_gprs_context *gc,
 
 	DBG("cid %u", ctx->cid);
 
-	/* IPv6 support not implemented */
-	if (ctx->proto != OFONO_GPRS_PROTO_IP)
-		goto error;
-
 	gcd->active_context = ctx->cid;
 	gcd->cb_data = data;
 	gcd->cb = cb;
 
-	len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"", ctx->cid);
+
+	switch (ctx->proto) {
+	case OFONO_GPRS_PROTO_IP:
+		len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"",
+				ctx->cid);
+		break;
+	case OFONO_GPRS_PROTO_IPV6:
+		len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IPV6\"",
+				ctx->cid);
+		break;
+	case OFONO_GPRS_PROTO_IPV4V6:
+		len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IPV4V6\"",
+				ctx->cid);
+		break;
+	}
 
 	if (ctx->apn)
 		snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"", ctx->apn);
@@ -135,7 +146,6 @@ static void gemalto_gprs_activate_primary(struct ofono_gprs_context *gc,
 				cgdcont_enable_cb, gc, NULL))
 		return;
 
-error:
 	CALLBACK_WITH_FAILURE(cb, data);
 }
 
-- 
2.29.2

[PATCH 5/5] gemalto: gprs: support authentication settings

[Sergey Matyukevich]

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

From: Sergey Matyukevich <matyukevich@arrival.com>

Add support for gprs contexts with username, password, as well as
specific authentication type.
---
 drivers/gemaltomodem/gprs-context.c | 54 ++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/gemaltomodem/gprs-context.c b/drivers/gemaltomodem/gprs-context.c
index bb09f0b0..da68e959 100644
--- a/drivers/gemaltomodem/gprs-context.c
+++ b/drivers/gemaltomodem/gprs-context.c
@@ -46,6 +46,9 @@ static const char *none_prefix[] = { NULL };
 struct gprs_context_data {
 	GAtChat *chat;
 	unsigned int active_context;
+	char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
+	char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
+	int auth_type;
 	enum ofono_gprs_proto proto;
 	ofono_gprs_context_cb_t cb;
 	void *cb_data;
@@ -79,7 +82,7 @@ static void swwan_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	}
 }
 
-static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
+static void sgauth_enable_cb(gboolean ok, GAtResult *result,
 			gpointer user_data)
 {
 	struct ofono_gprs_context *gc = user_data;
@@ -109,6 +112,39 @@ static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
 	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
 }
 
+static void cgdcont_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);
+	struct ofono_error error;
+	char buf[384];
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		gcd->active_context = 0;
+		decode_at_error(&error, g_at_result_final_response(result));
+		gcd->cb(&error, gcd->cb_data);
+		return;
+	}
+
+	if (gcd->username[0] && gcd->password[0])
+		sprintf(buf, "AT^SGAUTH=%u,%u,\"%s\",\"%s\"",
+			gcd->active_context, gcd->auth_type,
+			gcd->username, gcd->password);
+	else
+		sprintf(buf, "AT^SGAUTH=%u,%u,\"\",\"\"",
+			gcd->active_context, gcd->auth_type);
+
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				sgauth_enable_cb, gc, NULL) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+}
+
 static void gemalto_gprs_activate_primary(struct ofono_gprs_context *gc,
 				const struct ofono_gprs_primary_context *ctx,
 				ofono_gprs_context_cb_t cb, void *data)
@@ -123,6 +159,22 @@ static void gemalto_gprs_activate_primary(struct ofono_gprs_context *gc,
 	gcd->cb_data = data;
 	gcd->cb = cb;
 
+	memcpy(gcd->username, ctx->username, sizeof(ctx->username));
+	memcpy(gcd->password, ctx->password, sizeof(ctx->password));
+	gcd->proto = ctx->proto;
+
+	switch (ctx->auth_method) {
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		gcd->auth_type = 1;
+		break;
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+		gcd->auth_type = 2;
+		break;
+	case OFONO_GPRS_AUTH_METHOD_NONE:
+	default:
+		gcd->auth_type = 0;
+		break;
+	}
 
 	switch (ctx->proto) {
 	case OFONO_GPRS_PROTO_IP:
-- 
2.29.2

Re: [PATCH 1/5] plugin: gemalto: fix source of gprs notifications

[Denis Kenzior]

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

Hi Sergey,

On 12/21/20 2:01 PM, Sergey Matyukevich wrote:
> Modem USB interface does not receive certain gprs context notifications.
> Fix gprs chat: use Application USB interface to receive all the modem
> notifications.
> ---
>   plugins/gemalto.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Patch 1, 2 and 4 applied,  thanks.

Regards,
-Denis

Re: [PATCH 3/5] gemalto: gprs: support automatic context activation

[Denis Kenzior]

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

Hi Sergey,

On 12/21/20 2:01 PM, Sergey Matyukevich wrote:
> From: Sergey Matyukevich <matyukevich@arrival.com>
> 
> Implement read_settings function to get configuration for automatic
> contexts. Fix the issue uncovered by added support for automatic
> context activation: do not use AT+CGACT for the contexts handled
> by AT^SWWAN. As per modem specs, CGACT context can not be reused
> for SWWAN. Though that worked for some reason when automatic
> context was reactivated without proper deactivation.
> ---
>   drivers/gemaltomodem/gprs-context.c | 110 +++++++++++++++-------------
>   1 file changed, 59 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gemaltomodem/gprs-context.c b/drivers/gemaltomodem/gprs-context.c
> index 322a5f98..680f01ab 100644
> --- a/drivers/gemaltomodem/gprs-context.c
> +++ b/drivers/gemaltomodem/gprs-context.c
> @@ -50,70 +50,61 @@ struct gprs_context_data {
>   	void *cb_data;
>   };
>   
> -static void cgact_enable_cb(gboolean ok, GAtResult *result,
> -				gpointer user_data)
> +static void set_gprs_context_interface(struct ofono_gprs_context *gc)
>   {
> -	struct ofono_gprs_context *gc = user_data;
> -	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>   	struct ofono_modem *modem;
>   	const char *interface;
> -	char buf[64];
> +
> +	modem = ofono_gprs_context_get_modem(gc);
> +	interface = ofono_modem_get_string(modem, "NetworkInterface");
> +	ofono_gprs_context_set_interface(gc, interface);
> +}
> +
> +static void swwan_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);
> +	struct ofono_error error;
>   
>   	DBG("ok %d", ok);
>   
>   	if (!ok) {
> -		struct ofono_error error;
> -
> +		ofono_error("Unable to activate context");
> +		ofono_gprs_context_deactivated(gc, gcd->active_context);
>   		gcd->active_context = 0;

This seems a bit strange.  Are you signaling success to the core too early

> -
>   		decode_at_error(&error, g_at_result_final_response(result));
>   		gcd->cb(&error, gcd->cb_data);
> -
>   		return;
>   	}
> -
> -	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
> -	g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
> -
> -	modem = ofono_gprs_context_get_modem(gc);
> -	interface = ofono_modem_get_string(modem, "NetworkInterface");
> -	ofono_gprs_context_set_interface(gc, interface);
> -
> -	/* Use DHCP */
> -	ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
> -
> -	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
>   }
>   
>   static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
> -				gpointer user_data)
> +			gpointer user_data)

nit: This is superfluous and also see doc/coding-style.txt item M4

>   {
>   	struct ofono_gprs_context *gc = user_data;
>   	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	struct ofono_error error;
>   	char buf[64];
>   
>   	DBG("ok %d", ok);
>   
>   	if (!ok) {
> -		struct ofono_error error;
> -
>   		gcd->active_context = 0;
> -
>   		decode_at_error(&error, g_at_result_final_response(result));
>   		gcd->cb(&error, gcd->cb_data);
> -
>   		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) == 0)
> -		goto error;
> +	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
> +	if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {

Note, this returns > 0 when the command has been queued, not that it has been 
sent or replied to yet...

> +		set_gprs_context_interface(gc);
> +		/* Use DHCP */
> +		ofono_gprs_context_set_ipv4_address(gc, NULL, 0);

If these modems can only do DHCP, then might be cleaner to move the 'Use DHCP' 
bits into set_gprs_context_interface.  And maybe rename it to make things 
clearer, if needed.

>   
> -	return;
> +		CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);

Are you sure you don't want to wait until swwan_cb to return success to the 
core?  AT^SWWAN can still fail...?

> +		return;
> +	}
>   
> -error:
>   	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>   }
>   

<snip>

> @@ -185,17 +170,39 @@ static void gemalto_gprs_deactivate_primary(struct ofono_gprs_context *gc,
>   	gcd->cb = cb;
>   	gcd->cb_data = data;
>   
> -	snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", cid);
> +	snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
>   
>   	if (g_at_chat_send(gcd->chat, buf, none_prefix,
> -				cgact_disable_cb, gc, NULL) == 0)
> -		goto error;
> -
> -	return;
> +				deactivate_cb, gc, NULL))
> +		return;
>   
> -error:
>   	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void gemalto_gprs_read_settings(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->active_context = cid;
> +	gcd->cb = cb;
> +	gcd->cb_data = data;
> +
> +	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);

nit: doc/coding-style.txt item M1

> +	if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {
> +		set_gprs_context_interface(gc);
> +		/* Use DHCP */
> +		ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
>   
> +		CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);

Are you sure you don't want to wait until swwan_cb to return success to the 
core?  I mean AT^SWWAN might still fail...?

I also find it a bit strange that an already-active context needs to go through 
the SWWAN dance, but if that's the way the firmware works, OK.   Might be worth 
a comment...?

> +		return;
> +	}
> +
> +	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
>   }
>   
>   static void cgev_notify(GAtResult *result, gpointer user_data)
> @@ -267,6 +274,7 @@ static const struct ofono_gprs_context_driver driver = {
>   	.remove			= gemalto_gprs_context_remove,
>   	.activate_primary	= gemalto_gprs_activate_primary,
>   	.deactivate_primary	= gemalto_gprs_deactivate_primary,
> +	.read_settings		= gemalto_gprs_read_settings,
>   };
>   
>   void gemalto_gprs_context_init(void)
> 

Regards,
-Denis

Re: [PATCH 5/5] gemalto: gprs: support authentication settings

[Denis Kenzior]

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

Hi Sergey,

On 12/21/20 2:01 PM, Sergey Matyukevich wrote:
> From: Sergey Matyukevich <matyukevich@arrival.com>
> 
> Add support for gprs contexts with username, password, as well as
> specific authentication type.
> ---
>   drivers/gemaltomodem/gprs-context.c | 54 ++++++++++++++++++++++++++++-
>   1 file changed, 53 insertions(+), 1 deletion(-)
> 

Looks good to me.

Regards,
-Denis

Re: [PATCH 3/5] gemalto: gprs: support automatic context activation

[Sergey Matyukevich]

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

Hello Denis,

> > Implement read_settings function to get configuration for automatic
> > contexts. Fix the issue uncovered by added support for automatic
> > context activation: do not use AT+CGACT for the contexts handled
> > by AT^SWWAN. As per modem specs, CGACT context can not be reused
> > for SWWAN. Though that worked for some reason when automatic
> > context was reactivated without proper deactivation.
> > ---
> >   drivers/gemaltomodem/gprs-context.c | 110 +++++++++++++++-------------
> >   1 file changed, 59 insertions(+), 51 deletions(-)

<snip>

> > +static void swwan_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);
> > +	struct ofono_error error;
> >   	DBG("ok %d", ok);
> >   	if (!ok) {
> > -		struct ofono_error error;
> > -
> > +		ofono_error("Unable to activate context");
> > +		ofono_gprs_context_deactivated(gc, gcd->active_context);
> >   		gcd->active_context = 0;
> 
> This seems a bit strange.  Are you signaling success to the core too early

Correct, this is intended behavior. See my further comments regarding
SWWAN command and its processing.
 
> > -
> >   		decode_at_error(&error, g_at_result_final_response(result));
> >   		gcd->cb(&error, gcd->cb_data);
> > -
> >   		return;
> >   	}
> > -
> > -	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
> > -	g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
> > -
> > -	modem = ofono_gprs_context_get_modem(gc);
> > -	interface = ofono_modem_get_string(modem, "NetworkInterface");
> > -	ofono_gprs_context_set_interface(gc, interface);
> > -
> > -	/* Use DHCP */
> > -	ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
> > -
> > -	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> >   }
> >   static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
> > -				gpointer user_data)
> > +			gpointer user_data)
> 
> nit: This is superfluous and also see doc/coding-style.txt item M4

Fixed in v2.

<snip>

> > -	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) == 0)
> > -		goto error;
> > +	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
> > +	if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {
> 
> Note, this returns > 0 when the command has been queued, not that it has
> been sent or replied to yet...
> 
> > +		set_gprs_context_interface(gc);
> > +		/* Use DHCP */
> > +		ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
> 
> If these modems can only do DHCP, then might be cleaner to move the 'Use
> DHCP' bits into set_gprs_context_interface.  And maybe rename it to make
> things clearer, if needed.

Good point, fixed in v2.

> 
> > -	return;
> > +		CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> 
> Are you sure you don't want to wait until swwan_cb to return success to the
> core?  AT^SWWAN can still fail...?

AT^SWWAN command is yet another way to activate a PDP context. AT commands
spec for this modem is a bit vague about SWWAN details, but according to
other materials from Gemalto as well as my experiments, this command
activates internal DHCP server, so DHCP client can be started for modem
USB ethernet interfaces. Based on my observations, SWWAN command does
not return until DHCP flow is completed.

So the idea is to send SWWAN command to modem and make it possible to
start DHCP flow right away. I assume that I need both things: mark
interface for DHCP and signal success to the core. Callback swwan_cb is
supposed to handle the case when SWWAN command fails: mark context as
deactivated and let oFono proceed with further connection attempts.

> > +		return;
> > +	}
> > -error:
> >   	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> >   }
> 
> <snip>
> 
> > @@ -185,17 +170,39 @@ static void gemalto_gprs_deactivate_primary(struct ofono_gprs_context *gc,
> >   	gcd->cb = cb;
> >   	gcd->cb_data = data;
> > -	snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", cid);
> > +	snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
> >   	if (g_at_chat_send(gcd->chat, buf, none_prefix,
> > -				cgact_disable_cb, gc, NULL) == 0)
> > -		goto error;
> > -
> > -	return;
> > +				deactivate_cb, gc, NULL))
> > +		return;
> > -error:
> >   	CALLBACK_WITH_FAILURE(cb, data);
> > +}
> > +
> > +static void gemalto_gprs_read_settings(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->active_context = cid;
> > +	gcd->cb = cb;
> > +	gcd->cb_data = data;
> > +
> > +	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
> 
> nit: doc/coding-style.txt item M1

Fixed in v2.

> > +	if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {
> > +		set_gprs_context_interface(gc);
> > +		/* Use DHCP */
> > +		ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
> > +		CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> 
> Are you sure you don't want to wait until swwan_cb to return success to the
> core?  I mean AT^SWWAN might still fail...?
> 
> I also find it a bit strange that an already-active context needs to go
> through the SWWAN dance, but if that's the way the firmware works, OK.
> Might be worth a comment...?

As I mentioned earlier, according to my understanding, SWWAN dance is
required to enable link on USB ethernet interface provided by modem
and to start internal DHCP server so that host software (e.g. ConnMan or
NetworkManager) could setup interface properly.

Sure, I can add a comment. What whould be better: to add a comment in
driver or to write a more detailed commit message ?

Regards,
Sergey

Re: [PATCH 3/5] gemalto: gprs: support automatic context activation

[Denis Kenzior]

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

Hi Sergey,

>> Are you sure you don't want to wait until swwan_cb to return success to the
>> core?  AT^SWWAN can still fail...?
> 
> AT^SWWAN command is yet another way to activate a PDP context. AT commands
> spec for this modem is a bit vague about SWWAN details, but according to
> other materials from Gemalto as well as my experiments, this command
> activates internal DHCP server, so DHCP client can be started for modem
> USB ethernet interfaces. Based on my observations, SWWAN command does
> not return until DHCP flow is completed.

Ugh.  I'd 'window.throw(modem)'...

> 
> So the idea is to send SWWAN command to modem and make it possible to
> start DHCP flow right away. I assume that I need both things: mark
> interface for DHCP and signal success to the core. Callback swwan_cb is

So the problem with this is that you've now blocked the app/modem port until 
that DHCP negotiation happens.  Maybe it does, maybe it doesn't.  It is less 
than ideal to depend on some external component; there are frequently situations 
where people would be running without ConnMan for example.

> supposed to handle the case when SWWAN command fails: mark context as
> deactivated and let oFono proceed with further connection attempts.
> 

Another thing to consider is to just run dhcp yourself.  ell has had a DHCPv4 
client for a while now.  So you could just run l_dhcp_client to obtain the 
address and signal it to the core, leaving the app port in a known state...

Or better yet, don't use SWWAN if you can help it...

<snip>

> Sure, I can add a comment. What whould be better: to add a comment in
> driver or to write a more detailed commit message ?

Given how unusual this behavior is, I'd add a comment directly in the code.

Regards,
-Denis

Re: [PATCH 3/5] gemalto: gprs: support automatic context activation

[Sergey Matyukevich]

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

Hi Denis,

> > > Are you sure you don't want to wait until swwan_cb to return success to the
> > > core?  AT^SWWAN can still fail...?
> > 
> > AT^SWWAN command is yet another way to activate a PDP context. AT commands
> > spec for this modem is a bit vague about SWWAN details, but according to
> > other materials from Gemalto as well as my experiments, this command
> > activates internal DHCP server, so DHCP client can be started for modem
> > USB ethernet interfaces. Based on my observations, SWWAN command does
> > not return until DHCP flow is completed.
> 
> Ugh.  I'd 'window.throw(modem)'...

Well, suggested revision works fairly well in conjunction with ConnMan.
So I would give it a chance )
 
> > 
> > So the idea is to send SWWAN command to modem and make it possible to
> > start DHCP flow right away. I assume that I need both things: mark
> > interface for DHCP and signal success to the core. Callback swwan_cb is
> 
> So the problem with this is that you've now blocked the app/modem port until
> that DHCP negotiation happens.  Maybe it does, maybe it doesn't.  It is less
> than ideal to depend on some external component; there are frequently
> situations where people would be running without ConnMan for example.
> 
> > supposed to handle the case when SWWAN command fails: mark context as
> > deactivated and let oFono proceed with further connection attempts.
> > 
> 
> Another thing to consider is to just run dhcp yourself.  ell has had a
> DHCPv4 client for a while now.  So you could just run l_dhcp_client to
> obtain the address and signal it to the core, leaving the app port in a
> known state...
> 
> Or better yet, don't use SWWAN if you can help it...

I guess that can be done as well. As I mentioned previously, SWWAN is
just one possible option. I assume that another option is to use
CGDCONT/CGACT and then to retrieve IP settings from modem. There were
earlier patches on the mail list for PLS8x modem, IIRC at least one
of them suggested both CGACT and SWWAN. I think I will take a look
and update those patches as soon as I am done with this SWWAN part.
Though at the moment I have no idea how to make configurable gprs
driver selection in gemalto plugin...

> <snip>
> 
> > Sure, I can add a comment. What whould be better: to add a comment in
> > driver or to write a more detailed commit message ?
> 
> Given how unusual this behavior is, I'd add a comment directly in the code.

Ok, I will do both in v2: clarify commit message and add a comment into
the driver.

Regards,
Sergey