[PATCH 1/2] gemaltomodem: added Gemalto-specific gprs-context atom This atom name is not the same as the driver, because it might be necessary to add other grps-context atoms, according to the needs. The name is therefore referring to the main command used, AT^SWWAN.

[PATCH 1/2] gemaltomodem: added Gemalto-specific gprs-context atom This atom name is not the same as the driver, because it might be necessary to add other grps-context atoms, according to the needs. The name is therefore referring to the main command used, AT^SWWAN.

[Giacinto Cifelli]

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

---
 drivers/gemaltomodem/gemaltomodem.c      |   2 +
 drivers/gemaltomodem/gemaltomodem.h      |   3 +
 drivers/gemaltomodem/gprs-context-wwan.c | 446 +++++++++++++++++++++++++++++++
 3 files changed, 451 insertions(+)
 create mode 100644 drivers/gemaltomodem/gprs-context-wwan.c

diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
index 614ca81..7afd3c1 100644
--- a/drivers/gemaltomodem/gemaltomodem.c
+++ b/drivers/gemaltomodem/gemaltomodem.c
@@ -35,6 +35,7 @@
 static int gemaltomodem_init(void)
 {
 	gemalto_location_reporting_init();
+	gemaltowwan_gprs_context_init();
 	gemalto_voicecall_init();
 	return 0;
 }
@@ -42,6 +43,7 @@ static int gemaltomodem_init(void)
 static void gemaltomodem_exit(void)
 {
 	gemalto_location_reporting_exit();
+	gemaltowwan_gprs_context_exit();
 	gemalto_voicecall_exit();
 }
 
diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
index 4e6ed16..f45aea9 100644
--- a/drivers/gemaltomodem/gemaltomodem.h
+++ b/drivers/gemaltomodem/gemaltomodem.h
@@ -24,5 +24,8 @@
 extern void gemalto_location_reporting_init();
 extern void gemalto_location_reporting_exit();
 
+extern void gemaltowwan_gprs_context_init();
+extern void gemaltowwan_gprs_context_exit();
+
 extern void gemalto_voicecall_init();
 extern void gemalto_voicecall_exit();
diff --git a/drivers/gemaltomodem/gprs-context-wwan.c b/drivers/gemaltomodem/gprs-context-wwan.c
new file mode 100644
index 0000000..12378a3
--- /dev/null
+++ b/drivers/gemaltomodem/gprs-context-wwan.c
@@ -0,0 +1,446 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2017 Piotr Haber. All rights reserved.
+ *  Copyright (C) 2018 Sebastian Arnd. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
+ *
+ *  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.
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+#include <sys/stat.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/gprs-context.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "gemaltomodem.h"
+
+static const char *none_prefix[] = { NULL };
+
+enum state {
+	STATE_IDLE,
+	STATE_ENABLING,
+	STATE_DISABLING,
+	STATE_ACTIVE,
+};
+
+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];
+	enum ofono_gprs_auth_method auth_method;
+	enum state state;
+	enum ofono_gprs_proto proto;
+	char address[64];
+	char netmask[64];
+	char gateway[64];
+	char dns1[64];
+	char dns2[64];
+	ofono_gprs_context_cb_t cb;
+	void *cb_data;
+	int use_wwan;
+};
+
+static gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
+				enum ofono_gprs_auth_method auth_method,
+				const char *username, const char *password,
+				char *buf, guint buflen)
+{
+	int gto_auth = ofono_modem_get_integer(modem, "Gemalto_Auth");
+	int len;
+	/*
+	 * 0: use cgauth
+	 * 1: use sgauth(pwd, user)
+	 * 2: use sgauth(user, pwd)
+	 */
+
+	int auth_type;
+
+	switch (auth_method) {
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		auth_type = 1;
+		break;
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+		auth_type = 2;
+		break;
+	case OFONO_GPRS_AUTH_METHOD_NONE:
+	default:
+		auth_type = 0;
+		break;
+	}
+
+	if (auth_type != 0 && (!*username || !*password))
+		return FALSE;
+
+	switch (gto_auth) {
+	case 1:
+	case 2:
+		len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
+		break;
+	case 0:
+	default:
+		len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
+		break;
+	}
+
+	buflen -= len;
+
+	switch(auth_type) {
+	case 0:
+
+		switch (gto_auth) {
+		case 2:
+			snprintf(buf+len, buflen, ",0,\"\",\"\"");
+			break;
+		case 0:
+		case 1:
+		default:
+			snprintf(buf+len, buflen, ",0");
+			break;
+		}
+		break;
+
+	case 1:
+	case 2:
+
+		switch (gto_auth) {
+		case 1:
+			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
+					auth_type, password, username);
+			break;
+		case 0:
+		case 2:
+		default:
+			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
+					auth_type, username, password);
+		}
+		break;
+
+	default:
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
+static void gemalto_get_cgdcont_command(struct ofono_modem *modem,
+			guint cid, enum ofono_gprs_proto proto, const char *apn,
+							char *buf, guint buflen)
+{
+	int len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
+	buflen -= len;
+
+	if (!apn) /* it will remove the context */
+		return;
+
+	switch (proto) {
+	case OFONO_GPRS_PROTO_IPV6:
+		snprintf(buf+len, buflen, ",\"IPV6\",\"%s\"", apn);
+		break;
+	case OFONO_GPRS_PROTO_IPV4V6:
+		snprintf(buf+len, buflen, ",\"IPV4V6\",\"%s\"", apn);
+		break;
+	case OFONO_GPRS_PROTO_IP:
+	default:
+		snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
+		break;
+	}
+}
+
+static void failed_setup(struct ofono_gprs_context *gc,
+				GAtResult *result, gboolean deactivate)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	struct ofono_error error;
+	char buf[64];
+
+	DBG("deactivate %d", deactivate);
+
+	if (deactivate == TRUE) {
+
+		if (gcd->use_wwan)
+			sprintf(buf, "AT^SWWAN=0,%u", gcd->active_context);
+		else
+			sprintf(buf, "AT+CGACT=0,%u", gcd->active_context);
+
+		g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
+	}
+
+	gcd->active_context = 0;
+	gcd->state = STATE_IDLE;
+
+	if (result == NULL) {
+		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+		return;
+	}
+
+	decode_at_error(&error, g_at_result_final_response(result));
+	gcd->cb(&error, gcd->cb_data);
+}
+
+static void activate_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("Unable activate context");
+		/*
+		 * We've reported sucess already, so can't just call
+		 * failed_setup we call ofono_gprs_context_deactivated instead.
+		 * Thats not a clean solution at all, but as it seems there is
+		 * no clean way to determine whether it is possible to activate
+		 * the context before issuing AT^SWWAN. A possible workaround
+		 * might be to issue AT+CGACT=1 and AT+CGACT=0 and try if that
+		 * works, before calling CALLBACK_WITH_SUCCESS.
+		 */
+		ofono_gprs_context_deactivated(gc, gcd->active_context);
+		gcd->active_context = 0;
+		gcd->state = STATE_IDLE;
+		return;
+	}
+	/* We've reported sucess already */
+}
+
+static void setup_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[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+					OFONO_GPRS_MAX_PASSWORD_LENGTH +1];
+	struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
+	const char *interface;
+
+	if (!ok) {
+		ofono_error("Failed to setup context");
+		failed_setup(gc, result, FALSE);
+		return;
+	}
+
+	if (gemalto_get_auth_command(modem, gcd->active_context,
+				gcd->auth_method, gcd->username, gcd->password,
+							buf, sizeof(buf))) {
+		if (!g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL,
+									NULL))
+		goto error;
+	}
+	/*
+	 * note that if the auth command is not ok we skip it and continue
+	 * but if the sending fails we do an error
+	 */
+
+	if (gcd->use_wwan)
+		sprintf(buf, "AT^SWWAN=1,%u", gcd->active_context);
+	else
+		sprintf(buf, "AT+CGACT=%u,1", gcd->active_context);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+					activate_cb, gc, NULL) > 0){
+
+		interface = ofono_modem_get_string(modem, "NetworkInterface");
+
+		ofono_gprs_context_set_interface(gc, interface);
+		ofono_gprs_context_set_ipv4_address(gc, NULL, FALSE);
+
+		/*
+		 * We report sucess already here because some modules need a
+		 * DHCP request to complete the AT^SWWAN command sucessfully
+		 */
+		gcd->state = STATE_ACTIVE;
+
+		CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+
+		return;
+	}
+
+error:
+	failed_setup(gc, NULL, FALSE);
+}
+
+static void gemaltowwan_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);
+	char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
+	struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
+
+	DBG("cid %u", ctx->cid);
+
+	gcd->use_wwan = ofono_modem_get_integer(modem, "Gemalto_WWAN");
+	gcd->active_context = ctx->cid;
+	gcd->cb = cb;
+	gcd->cb_data = data;
+	memcpy(gcd->username, ctx->username, sizeof(ctx->username));
+	memcpy(gcd->password, ctx->password, sizeof(ctx->password));
+	gcd->state = STATE_ENABLING;
+	gcd->proto = ctx->proto;
+	gcd->auth_method = ctx->auth_method;
+
+	gemalto_get_cgdcont_command(modem, ctx->cid, ctx->proto, ctx->apn, buf,
+								sizeof(buf));
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				setup_cb, gc, NULL) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(cb, 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);
+
+	DBG("ok %d", ok);
+
+	gcd->active_context = 0;
+	gcd->state = STATE_IDLE;
+
+	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+}
+
+static void gemaltowwan_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->state = STATE_DISABLING;
+	gcd->cb = cb;
+	gcd->cb_data = data;
+
+	if (gcd->use_wwan)
+		sprintf(buf, "AT^SWWAN=0,%u", gcd->active_context);
+	else
+		sprintf(buf, "AT+CGACT=%u,0", gcd->active_context);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				deactivate_cb, gc, NULL) > 0)
+		return;
+
+	CALLBACK_WITH_SUCCESS(cb, data);
+}
+
+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);
+	const char *event;
+	int cid = 0;
+	GAtResultIter iter;
+
+	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, "ME PDN DEACT"))
+		return;
+
+	sscanf(event, "%*s %*s %*s %u", &cid);
+
+	DBG("cid %d", cid);
+
+	if ((unsigned int) cid != gcd->active_context)
+		return;
+
+	ofono_gprs_context_deactivated(gc, gcd->active_context);
+
+	gcd->active_context = 0;
+	gcd->state = STATE_IDLE;
+}
+
+static int gemaltowwan_gprs_context_probe(struct ofono_gprs_context *gc,
+					unsigned int model, void *data)
+{
+	GAtChat *chat = data;
+	struct gprs_context_data *gcd;
+	struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
+
+	DBG("");
+
+	gcd = g_try_new0(struct gprs_context_data, 1);
+	if (gcd == NULL)
+		return -ENOMEM;
+
+	if (modem)
+		gcd->use_wwan = ofono_modem_get_integer(modem, "Gemalto_WWAN");
+	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 gemaltowwan_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);
+	g_free(gcd);
+}
+
+static void gemaltowwan_gprs_detach_shutdown(struct ofono_gprs_context *gc,
+					unsigned int cid)
+{
+	DBG("cid %u", cid);
+
+	ofono_gprs_context_deactivated(gc, cid);
+}
+
+static struct ofono_gprs_context_driver driver = {
+	.name			= "gemaltowwanmodem",
+	.probe			= gemaltowwan_gprs_context_probe,
+	.remove			= gemaltowwan_gprs_context_remove,
+	.activate_primary	= gemaltowwan_gprs_activate_primary,
+	.deactivate_primary	= gemaltowwan_gprs_deactivate_primary,
+	.detach_shutdown	= gemaltowwan_gprs_detach_shutdown,
+};
+
+void gemaltowwan_gprs_context_init(void)
+{
+	ofono_gprs_context_driver_register(&driver);
+}
+
+void gemaltowwan_gprs_context_exit(void)
+{
+	ofono_gprs_context_driver_unregister(&driver);
+}
-- 
1.9.1

[PATCH 2/2] gemaltomodem: added gprs-context-wwan.c in the makefile

[Giacinto Cifelli]

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

---
 Makefile.am | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile.am b/Makefile.am
index e2363e2..fabda0a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -397,6 +397,7 @@ builtin_modules += gemaltomodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/gemaltomodem/gemaltomodem.h \
 			drivers/gemaltomodem/gemaltomodem.c \
+			drivers/gemaltomodem/gprs-context-wwan.c \
 			drivers/gemaltomodem/voicecall.c \
 			drivers/gemaltomodem/location-reporting.c
 
-- 
1.9.1

Re: [PATCH 1/2] gemaltomodem: added Gemalto-specific gprs-context atom This atom name is not the same as the driver, because it might be necessary to add other grps-context atoms, according to the needs. The name is therefore referring to the main command used, AT^SWWAN.

[Denis Kenzior]

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

Hi Giacinto,

So please fix your commit description in the next version.

On 09/19/2018 10:50 PM, Giacinto Cifelli wrote:
> ---
>   drivers/gemaltomodem/gemaltomodem.c      |   2 +
>   drivers/gemaltomodem/gemaltomodem.h      |   3 +
>   drivers/gemaltomodem/gprs-context-wwan.c | 446 +++++++++++++++++++++++++++++++
>   3 files changed, 451 insertions(+)
>   create mode 100644 drivers/gemaltomodem/gprs-context-wwan.c
> 
> diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
> index 614ca81..7afd3c1 100644
> --- a/drivers/gemaltomodem/gemaltomodem.c
> +++ b/drivers/gemaltomodem/gemaltomodem.c
> @@ -35,6 +35,7 @@
>   static int gemaltomodem_init(void)
>   {
>   	gemalto_location_reporting_init();
> +	gemaltowwan_gprs_context_init();
>   	gemalto_voicecall_init();
>   	return 0;
>   }
> @@ -42,6 +43,7 @@ static int gemaltomodem_init(void)
>   static void gemaltomodem_exit(void)
>   {
>   	gemalto_location_reporting_exit();
> +	gemaltowwan_gprs_context_exit();

I'm not really a fan of the name.  How many of gprs_context types do you 
have, and what is fundamentally different between them that they cannot 
be handled by the same driver?

>   	gemalto_voicecall_exit();
>   }
>   
> diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
> index 4e6ed16..f45aea9 100644
> --- a/drivers/gemaltomodem/gemaltomodem.h
> +++ b/drivers/gemaltomodem/gemaltomodem.h
> @@ -24,5 +24,8 @@
>   extern void gemalto_location_reporting_init();
>   extern void gemalto_location_reporting_exit();
>   
> +extern void gemaltowwan_gprs_context_init();
> +extern void gemaltowwan_gprs_context_exit();
> +
>   extern void gemalto_voicecall_init();
>   extern void gemalto_voicecall_exit();
> diff --git a/drivers/gemaltomodem/gprs-context-wwan.c b/drivers/gemaltomodem/gprs-context-wwan.c
> new file mode 100644
> index 0000000..12378a3
> --- /dev/null
> +++ b/drivers/gemaltomodem/gprs-context-wwan.c
> @@ -0,0 +1,446 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2017 Piotr Haber. All rights reserved.
> + *  Copyright (C) 2018 Sebastian Arnd. All rights reserved.
> + *  Copyright (C) 2018 Gemalto M2M
> + *
> + *  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.
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +
> +#include <glib.h>
> +
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/gprs-context.h>
> +
> +#include "gatchat.h"
> +#include "gatresult.h"
> +
> +#include "gemaltomodem.h"
> +
> +static const char *none_prefix[] = { NULL };
> +
> +enum state {
> +	STATE_IDLE,
> +	STATE_ENABLING,
> +	STATE_DISABLING,
> +	STATE_ACTIVE,
> +};
> +
> +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];
> +	enum ofono_gprs_auth_method auth_method;
> +	enum state state;
> +	enum ofono_gprs_proto proto;
> +	char address[64];
> +	char netmask[64];
> +	char gateway[64];
> +	char dns1[64];
> +	char dns2[64];
> +	ofono_gprs_context_cb_t cb;
> +	void *cb_data;
> +	int use_wwan;
> +};
> +
> +static gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> +				enum ofono_gprs_auth_method auth_method,
> +				const char *username, const char *password,
> +				char *buf, guint buflen)
> +{
> +	int gto_auth = ofono_modem_get_integer(modem, "Gemalto_Auth");

Please use a CamelCase-d variable for this.  E.g. GemaltoAuthType

> +	int len;
> +	/*
> +	 * 0: use cgauth
> +	 * 1: use sgauth(pwd, user)
> +	 * 2: use sgauth(user, pwd)
> +	 */
> +
> +	int auth_type;
> +
> +	switch (auth_method) {
> +	case OFONO_GPRS_AUTH_METHOD_PAP:
> +		auth_type = 1;
> +		break;
> +	case OFONO_GPRS_AUTH_METHOD_CHAP:
> +		auth_type = 2;
> +		break;
> +	case OFONO_GPRS_AUTH_METHOD_NONE:

So until the GPRS_AUTH_METHOD_NONE is actually introduced, you might 
want to just get this upstream with the existing enumeration.  E.g. just 
assume that if username & password is empty, the auth method is none.

> +	default:
> +		auth_type = 0;
> +		break;
> +	}
> +
> +	if (auth_type != 0 && (!*username || !*password)) > +		return FALSE;
> +
> +	switch (gto_auth) {
> +	case 1:
> +	case 2:
> +		len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
> +		break;
> +	case 0:
> +	default:
> +		len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
> +		break;
> +	}
> +
> +	buflen -= len;
> +
> +	switch(auth_type) {
> +	case 0:
> +
> +		switch (gto_auth) {
> +		case 2:
> +			snprintf(buf+len, buflen, ",0,\"\",\"\"");
> +			break;
> +		case 0:
> +		case 1:
> +		default:
> +			snprintf(buf+len, buflen, ",0");
> +			break;
> +		}
> +		break;

This switch might be more compact as an if statement.  Also the coding 
style is all wrong in this function.  See doc/coding-style.txt item M3 
in particular.

> +
> +	case 1:
> +	case 2:
> +
> +		switch (gto_auth) {
> +		case 1:
> +			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> +					auth_type, password, username);
> +			break;
> +		case 0:
> +		case 2:
> +		default:
> +			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> +					auth_type, username, password);
> +		}
> +		break;
> +
> +	default:
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
> +static void gemalto_get_cgdcont_command(struct ofono_modem *modem,
> +			guint cid, enum ofono_gprs_proto proto, const char *apn,
> +							char *buf, guint buflen)
> +{
> +	int len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
> +	buflen -= len;
> +
> +	if (!apn) /* it will remove the context */
> +		return;
> +
> +	switch (proto) {
> +	case OFONO_GPRS_PROTO_IPV6:
> +		snprintf(buf+len, buflen, ",\"IPV6\",\"%s\"", apn);
> +		break;
> +	case OFONO_GPRS_PROTO_IPV4V6:
> +		snprintf(buf+len, buflen, ",\"IPV4V6\",\"%s\"", apn);
> +		break;
> +	case OFONO_GPRS_PROTO_IP:
> +	default:
> +		snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
> +		break;
> +	}
> +}
> +
> +static void failed_setup(struct ofono_gprs_context *gc,
> +				GAtResult *result, gboolean deactivate)
> +{
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	struct ofono_error error;
> +	char buf[64];
> +
> +	DBG("deactivate %d", deactivate);
> +
> +	if (deactivate == TRUE) {
> +
> +		if (gcd->use_wwan)
> +			sprintf(buf, "AT^SWWAN=0,%u", gcd->active_context);
> +		else
> +			sprintf(buf, "AT+CGACT=0,%u", gcd->active_context);
> +
> +		g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
> +	}
> +
> +	gcd->active_context = 0;
> +	gcd->state = STATE_IDLE;
> +
> +	if (result == NULL) {
> +		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> +		return;
> +	}
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +	gcd->cb(&error, gcd->cb_data);
> +}
> +
> +static void activate_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("Unable activate context");
> +		/*
> +		 * We've reported sucess already, so can't just call
> +		 * failed_setup we call ofono_gprs_context_deactivated instead.
> +		 * Thats not a clean solution at all, but as it seems there is
> +		 * no clean way to determine whether it is possible to activate
> +		 * the context before issuing AT^SWWAN. A possible workaround
> +		 * might be to issue AT+CGACT=1 and AT+CGACT=0 and try if that
> +		 * works, before calling CALLBACK_WITH_SUCCESS.
> +		 */
> +		ofono_gprs_context_deactivated(gc, gcd->active_context);
> +		gcd->active_context = 0;
> +		gcd->state = STATE_IDLE;
> +		return;
> +	}
> +	/* We've reported sucess already */
> +}
> +
> +static void setup_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[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
> +					OFONO_GPRS_MAX_PASSWORD_LENGTH +1];
> +	struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> +	const char *interface;
> +
> +	if (!ok) {
> +		ofono_error("Failed to setup context");
> +		failed_setup(gc, result, FALSE);
> +		return;
> +	}
> +
> +	if (gemalto_get_auth_command(modem, gcd->active_context,
> +				gcd->auth_method, gcd->username, gcd->password,
> +							buf, sizeof(buf))) {
> +		if (!g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL,
> +									NULL))
> +		goto error;

indentation is wrong here?

> +	}
> +	/*
> +	 * note that if the auth command is not ok we skip it and continue
> +	 * but if the sending fails we do an error
> +	 */
> +
> +	if (gcd->use_wwan)
> +		sprintf(buf, "AT^SWWAN=1,%u", gcd->active_context);
> +	else
> +		sprintf(buf, "AT+CGACT=%u,1", gcd->active_context);
> +
> +	if (g_at_chat_send(gcd->chat, buf, none_prefix,
> +					activate_cb, gc, NULL) > 0){
> +
> +		interface = ofono_modem_get_string(modem, "NetworkInterface");
> +
> +		ofono_gprs_context_set_interface(gc, interface);
> +		ofono_gprs_context_set_ipv4_address(gc, NULL, FALSE);
> +
> +		/*
> +		 * We report sucess already here because some modules need a
> +		 * DHCP request to complete the AT^SWWAN command sucessfully
> +		 */
> +		gcd->state = STATE_ACTIVE;
> +
> +		CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);

So why are you calling SUCCESS even in the case of +CGACT?   Also, 
context activations via +CGACT need to be queried for static IPs, no? 
E.g. via +CGCONTRDP?  I don't see you doing that.

Fundamentally, your context activation has succeeded or it didn't.  The 
DHCP operation is purely local between the modem and the host.  No other 
vendor in the world has this weird setup that you do.  So I don't really 
know what to tell you here as this ^SWWAN interaction is fundamentally 
broken and it can't go upstream in this form.

Can ^SWWAN support static ip reporting instead of DHCP?  That would be 
preferred anyway as DHCP is slow and unneeded.

> +
> +		return;
> +	}
> +
> +error:
> +	failed_setup(gc, NULL, FALSE);
> +}
> +
> +static void gemaltowwan_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);
> +	char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
> +	struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> +
> +	DBG("cid %u", ctx->cid);
> +
> +	gcd->use_wwan = ofono_modem_get_integer(modem, "Gemalto_WWAN");
> +	gcd->active_context = ctx->cid;
> +	gcd->cb = cb;
> +	gcd->cb_data = data;
> +	memcpy(gcd->username, ctx->username, sizeof(ctx->username));
> +	memcpy(gcd->password, ctx->password, sizeof(ctx->password));
> +	gcd->state = STATE_ENABLING;
> +	gcd->proto = ctx->proto;
> +	gcd->auth_method = ctx->auth_method;
> +
> +	gemalto_get_cgdcont_command(modem, ctx->cid, ctx->proto, ctx->apn, buf,
> +								sizeof(buf));
> +
> +	if (g_at_chat_send(gcd->chat, buf, none_prefix,
> +				setup_cb, gc, NULL) > 0)
> +		return;
> +
> +	CALLBACK_WITH_FAILURE(cb, 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);
> +
> +	DBG("ok %d", ok);
> +
> +	gcd->active_context = 0;
> +	gcd->state = STATE_IDLE;
> +
> +	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> +}
> +
> +static void gemaltowwan_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->state = STATE_DISABLING;
> +	gcd->cb = cb;
> +	gcd->cb_data = data;
> +
> +	if (gcd->use_wwan)
> +		sprintf(buf, "AT^SWWAN=0,%u", gcd->active_context);
> +	else
> +		sprintf(buf, "AT+CGACT=%u,0", gcd->active_context);
> +
> +	if (g_at_chat_send(gcd->chat, buf, none_prefix,
> +				deactivate_cb, gc, NULL) > 0)
> +		return;
> +
> +	CALLBACK_WITH_SUCCESS(cb, data);
> +}
> +
> +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);
> +	const char *event;
> +	int cid = 0;
> +	GAtResultIter iter;
> +
> +	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, "ME PDN DEACT"))
> +		return;
> +
> +	sscanf(event, "%*s %*s %*s %u", &cid);
> +
> +	DBG("cid %d", cid);
> +
> +	if ((unsigned int) cid != gcd->active_context)
> +		return;
> +
> +	ofono_gprs_context_deactivated(gc, gcd->active_context);
> +
> +	gcd->active_context = 0;
> +	gcd->state = STATE_IDLE;
> +}
> +
> +static int gemaltowwan_gprs_context_probe(struct ofono_gprs_context *gc,
> +					unsigned int model, void *data)
> +{
> +	GAtChat *chat = data;
> +	struct gprs_context_data *gcd;
> +	struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> +
> +	DBG("");
> +
> +	gcd = g_try_new0(struct gprs_context_data, 1);
> +	if (gcd == NULL)
> +		return -ENOMEM;
> +
> +	if (modem)
> +		gcd->use_wwan = ofono_modem_get_integer(modem, "Gemalto_WWAN");
> +	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 gemaltowwan_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);
> +	g_free(gcd);
> +}
> +
> +static void gemaltowwan_gprs_detach_shutdown(struct ofono_gprs_context *gc,
> +					unsigned int cid)
> +{
> +	DBG("cid %u", cid);
> +
> +	ofono_gprs_context_deactivated(gc, cid);
> +}
> +
> +static struct ofono_gprs_context_driver driver = {
> +	.name			= "gemaltowwanmodem",
> +	.probe			= gemaltowwan_gprs_context_probe,
> +	.remove			= gemaltowwan_gprs_context_remove,
> +	.activate_primary	= gemaltowwan_gprs_activate_primary,
> +	.deactivate_primary	= gemaltowwan_gprs_deactivate_primary,
> +	.detach_shutdown	= gemaltowwan_gprs_detach_shutdown,
> +};
> +
> +void gemaltowwan_gprs_context_init(void)
> +{
> +	ofono_gprs_context_driver_register(&driver);
> +}
> +
> +void gemaltowwan_gprs_context_exit(void)
> +{
> +	ofono_gprs_context_driver_unregister(&driver);
> +}
> 

Regards,
-Denis

Re: [PATCH 1/2] gemaltomodem: added Gemalto-specific gprs-context atom This atom name is not the same as the driver, because it might be necessary to add other grps-context atoms, according to the needs. The name is therefore referring to the main command used, AT^SWWAN.

[Giacinto Cifelli]

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

Hi Denis,

On Fri, Sep 21, 2018 at 6:12 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> So please fix your commit description in the next version.
>

certainly.


>
> On 09/19/2018 10:50 PM, Giacinto Cifelli wrote:
> > ---
> >   drivers/gemaltomodem/gemaltomodem.c      |   2 +
> >   drivers/gemaltomodem/gemaltomodem.h      |   3 +
> >   drivers/gemaltomodem/gprs-context-wwan.c | 446
> +++++++++++++++++++++++++++++++
> >   3 files changed, 451 insertions(+)
> >   create mode 100644 drivers/gemaltomodem/gprs-context-wwan.c
> >
> > diff --git a/drivers/gemaltomodem/gemaltomodem.c
> b/drivers/gemaltomodem/gemaltomodem.c
> > index 614ca81..7afd3c1 100644
> > --- a/drivers/gemaltomodem/gemaltomodem.c
> > +++ b/drivers/gemaltomodem/gemaltomodem.c
> > @@ -35,6 +35,7 @@
> >   static int gemaltomodem_init(void)
> >   {
> >       gemalto_location_reporting_init();
> > +     gemaltowwan_gprs_context_init();
> >       gemalto_voicecall_init();
> >       return 0;
> >   }
> > @@ -42,6 +43,7 @@ static int gemaltomodem_init(void)
> >   static void gemaltomodem_exit(void)
> >   {
> >       gemalto_location_reporting_exit();
> > +     gemaltowwan_gprs_context_exit();
>
> I'm not really a fan of the name.  How many of gprs_context types do you
> have, and what is fundamentally different between them that they cannot
> be handled by the same driver?
>
>
Ok, I will rename it.

>       gemalto_voicecall_exit();
> >   }
> >
> > diff --git a/drivers/gemaltomodem/gemaltomodem.h
> b/drivers/gemaltomodem/gemaltomodem.h
> > index 4e6ed16..f45aea9 100644
> > --- a/drivers/gemaltomodem/gemaltomodem.h
> > +++ b/drivers/gemaltomodem/gemaltomodem.h
> > @@ -24,5 +24,8 @@
> >   extern void gemalto_location_reporting_init();
> >   extern void gemalto_location_reporting_exit();
> >
> > +extern void gemaltowwan_gprs_context_init();
> > +extern void gemaltowwan_gprs_context_exit();
> > +
> >   extern void gemalto_voicecall_init();
> >   extern void gemalto_voicecall_exit();
> > diff --git a/drivers/gemaltomodem/gprs-context-wwan.c
> b/drivers/gemaltomodem/gprs-context-wwan.c
> > new file mode 100644
> > index 0000000..12378a3
> > --- /dev/null
> > +++ b/drivers/gemaltomodem/gprs-context-wwan.c
> > @@ -0,0 +1,446 @@
> > +/*
> > + *
> > + *  oFono - Open Source Telephony
> > + *
> > + *  Copyright (C) 2017 Piotr Haber. All rights reserved.
> > + *  Copyright (C) 2018 Sebastian Arnd. All rights reserved.
> > + *  Copyright (C) 2018 Gemalto M2M
> > + *
> > + *  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.
> > + *
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#define _GNU_SOURCE
> > +#include <string.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +
> > +#include <glib.h>
> > +
> > +#include <ofono/log.h>
> > +#include <ofono/modem.h>
> > +#include <ofono/gprs-context.h>
> > +
> > +#include "gatchat.h"
> > +#include "gatresult.h"
> > +
> > +#include "gemaltomodem.h"
> > +
> > +static const char *none_prefix[] = { NULL };
> > +
> > +enum state {
> > +     STATE_IDLE,
> > +     STATE_ENABLING,
> > +     STATE_DISABLING,
> > +     STATE_ACTIVE,
> > +};
> > +
> > +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];
> > +     enum ofono_gprs_auth_method auth_method;
> > +     enum state state;
> > +     enum ofono_gprs_proto proto;
> > +     char address[64];
> > +     char netmask[64];
> > +     char gateway[64];
> > +     char dns1[64];
> > +     char dns2[64];
> > +     ofono_gprs_context_cb_t cb;
> > +     void *cb_data;
> > +     int use_wwan;
> > +};
> > +
> > +static gboolean gemalto_get_auth_command(struct ofono_modem *modem, int
> cid,
> > +                             enum ofono_gprs_auth_method auth_method,
> > +                             const char *username, const char *password,
> > +                             char *buf, guint buflen)
> > +{
> > +     int gto_auth = ofono_modem_get_integer(modem, "Gemalto_Auth");
>
> Please use a CamelCase-d variable for this.  E.g. GemaltoAuthType
>

ok. This is new for this code, because I have always seen only variables
with underscores, but no problem.
Personally I would prefer camelCase 'gemaltoAuthType', because it is more
coherent in case of 1-part names like 'user',
but I'll do as you say.


>
> > +     int len;
> > +     /*
> > +      * 0: use cgauth
> > +      * 1: use sgauth(pwd, user)
> > +      * 2: use sgauth(user, pwd)
> > +      */
> > +
> > +     int auth_type;
> > +
> > +     switch (auth_method) {
> > +     case OFONO_GPRS_AUTH_METHOD_PAP:
> > +             auth_type = 1;
> > +             break;
> > +     case OFONO_GPRS_AUTH_METHOD_CHAP:
> > +             auth_type = 2;
> > +             break;
> > +     case OFONO_GPRS_AUTH_METHOD_NONE:
>
> So until the GPRS_AUTH_METHOD_NONE is actually introduced, you might
> want to just get this upstream with the existing enumeration.  E.g. just
> assume that if username & password is empty, the auth method is none.
>

I have pushed the new method NONE again, and I prefer to wait until it is
taken.

The current solution is really a non-documented hack.
Checking whether user&password are empty to change the type of
authentication goes against the caller settings.
It is better to give an error so that there is a chance to have good
settings.


>
> > +     default:
> > +             auth_type = 0;
> > +             break;
> > +     }
> > +
> > +     if (auth_type != 0 && (!*username || !*password)) > +
>  return FALSE;
> > +
> > +     switch (gto_auth) {
> > +     case 1:
> > +     case 2:
> > +             len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
> > +             break;
> > +     case 0:
> > +     default:
> > +             len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
> > +             break;
> > +     }
> > +
> > +     buflen -= len;
> > +
> > +     switch(auth_type) {
> > +     case 0:
> > +
> > +             switch (gto_auth) {
> > +             case 2:
> > +                     snprintf(buf+len, buflen, ",0,\"\",\"\"");
> > +                     break;
> > +             case 0:
> > +             case 1:
> > +             default:
> > +                     snprintf(buf+len, buflen, ",0");
> > +                     break;
> > +             }
> > +             break;
>
> This switch might be more compact as an if statement.  Also the coding
> style is all wrong in this function.  See doc/coding-style.txt item M3
> in particular.
>

the idea of the switch is to let a reader see the differences among the
various cases,
but in reality there are 2 or 3 flags in this variable, so I am going to
restructure the eval as a bitmap,
and then I will call if's instead of switch's.
Thank you for this review, it helped pointing out the flaw in the design.


>
> > +
> > +     case 1:
> > +     case 2:
> > +
> > +             switch (gto_auth) {
> > +             case 1:
> > +                     snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> > +                                     auth_type, password, username);
> > +                     break;
> > +             case 0:
> > +             case 2:
> > +             default:
> > +                     snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> > +                                     auth_type, username, password);
> > +             }
> > +             break;
> > +
> > +     default:
> > +             return FALSE;
> > +     }
> > +
> > +     return TRUE;
> > +}
> > +
> > +static void gemalto_get_cgdcont_command(struct ofono_modem *modem,
> > +                     guint cid, enum ofono_gprs_proto proto, const char
> *apn,
> > +                                                     char *buf, guint
> buflen)
> > +{
> > +     int len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
> > +     buflen -= len;
> > +
> > +     if (!apn) /* it will remove the context */
> > +             return;
> > +
> > +     switch (proto) {
> > +     case OFONO_GPRS_PROTO_IPV6:
> > +             snprintf(buf+len, buflen, ",\"IPV6\",\"%s\"", apn);
> > +             break;
> > +     case OFONO_GPRS_PROTO_IPV4V6:
> > +             snprintf(buf+len, buflen, ",\"IPV4V6\",\"%s\"", apn);
> > +             break;
> > +     case OFONO_GPRS_PROTO_IP:
> > +     default:
> > +             snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
> > +             break;
> > +     }
> > +}
> > +
> > +static void failed_setup(struct ofono_gprs_context *gc,
> > +                             GAtResult *result, gboolean deactivate)
> > +{
> > +     struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> > +     struct ofono_error error;
> > +     char buf[64];
> > +
> > +     DBG("deactivate %d", deactivate);
> > +
> > +     if (deactivate == TRUE) {
> > +
> > +             if (gcd->use_wwan)
> > +                     sprintf(buf, "AT^SWWAN=0,%u", gcd->active_context);
> > +             else
> > +                     sprintf(buf, "AT+CGACT=0,%u", gcd->active_context);
> > +
> > +             g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL,
> NULL);
> > +     }
> > +
> > +     gcd->active_context = 0;
> > +     gcd->state = STATE_IDLE;
> > +
> > +     if (result == NULL) {
> > +             CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> > +             return;
> > +     }
> > +
> > +     decode_at_error(&error, g_at_result_final_response(result));
> > +     gcd->cb(&error, gcd->cb_data);
> > +}
> > +
> > +static void activate_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("Unable activate context");
> > +             /*
> > +              * We've reported sucess already, so can't just call
> > +              * failed_setup we call ofono_gprs_context_deactivated
> instead.
> > +              * Thats not a clean solution at all, but as it seems
> there is
> > +              * no clean way to determine whether it is possible to
> activate
> > +              * the context before issuing AT^SWWAN. A possible
> workaround
> > +              * might be to issue AT+CGACT=1 and AT+CGACT=0 and try if
> that
> > +              * works, before calling CALLBACK_WITH_SUCCESS.
> > +              */
> > +             ofono_gprs_context_deactivated(gc, gcd->active_context);
> > +             gcd->active_context = 0;
> > +             gcd->state = STATE_IDLE;
> > +             return;
> > +     }
> > +     /* We've reported sucess already */
> > +}
> > +
> > +static void setup_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[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
> > +                                     OFONO_GPRS_MAX_PASSWORD_LENGTH +1];
> > +     struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> > +     const char *interface;
> > +
> > +     if (!ok) {
> > +             ofono_error("Failed to setup context");
> > +             failed_setup(gc, result, FALSE);
> > +             return;
> > +     }
> > +
> > +     if (gemalto_get_auth_command(modem, gcd->active_context,
> > +                             gcd->auth_method, gcd->username,
> gcd->password,
> > +                                                     buf, sizeof(buf)))
> {
> > +             if (!g_at_chat_send(gcd->chat, buf, none_prefix, NULL,
> NULL,
> > +
>  NULL))
> > +             goto error;
>
> indentation is wrong here?
>

yes, i will fix it.


>
> > +     }
> > +     /*
> > +      * note that if the auth command is not ok we skip it and continue
> > +      * but if the sending fails we do an error
> > +      */
> > +
> > +     if (gcd->use_wwan)
> > +             sprintf(buf, "AT^SWWAN=1,%u", gcd->active_context);
> > +     else
> > +             sprintf(buf, "AT+CGACT=%u,1", gcd->active_context);
> > +
> > +     if (g_at_chat_send(gcd->chat, buf, none_prefix,
> > +                                     activate_cb, gc, NULL) > 0){
> > +
> > +             interface = ofono_modem_get_string(modem,
> "NetworkInterface");
> > +
> > +             ofono_gprs_context_set_interface(gc, interface);
> > +             ofono_gprs_context_set_ipv4_address(gc, NULL, FALSE);
> > +
> > +             /*
> > +              * We report sucess already here because some modules need
> a
> > +              * DHCP request to complete the AT^SWWAN command
> sucessfully
> > +              */
> > +             gcd->state = STATE_ACTIVE;
> > +
> > +             CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
>
> So why are you calling SUCCESS even in the case of +CGACT?   Also,
> context activations via +CGACT need to be queried for static IPs, no?
> E.g. via +CGCONTRDP?  I don't see you doing that.
>
> Fundamentally, your context activation has succeeded or it didn't.  The
> DHCP operation is purely local between the modem and the host.  No other
> vendor in the world has this weird setup that you do.  So I don't really
> know what to tell you here as this ^SWWAN interaction is fundamentally
> broken and it can't go upstream in this form.
>
> Can ^SWWAN support static ip reporting instead of DHCP?  That would be
> preferred anyway as DHCP is slow and unneeded.
>

This is a discussion about how Gemalto products work.

Admittedly, there are a few models, in the high-volume low-price range
(already deployed),
that won't return from AT^SWWAN until a DHCP exchange is performed, as per
Sebastian's comment above.

The rest (which are the large majority) of the modules would allow a
'standard' treatment, but also work with the code above.

As per DHCP vs static, we have both, depending on the module.

Just for information, I have stepped upon networks *requiring* DHCP:
in this kind of networks, an MBIM module that would normally return a
static settings,
returns 0x00 in the "IPv4 configuration Available" field in of the
IP_CONFIGURATION answer,
instead of the usual 0x0F (address, gateway, dns, mtu).
So, it looks like that DHCP has to be considered as a valid alternative
also in this case.

We cannot change every module and every network to fit our wishes.

Back to the code, I don't see a solution at the moment other than that one.
I can add an 'if' to filter only the models that need it, but cannot remove
it completely.
And this code would be specific for the driver, I don't see why it couldn't
go upstream.



> > +
> > +             return;
> > +     }
> > +
> > +error:
> > +     failed_setup(gc, NULL, FALSE);
> > +}
> > +
> > +static void gemaltowwan_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);
> > +     char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
> > +     struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> > +
> > +     DBG("cid %u", ctx->cid);
> > +
> > +     gcd->use_wwan = ofono_modem_get_integer(modem, "Gemalto_WWAN");
> > +     gcd->active_context = ctx->cid;
> > +     gcd->cb = cb;
> > +     gcd->cb_data = data;
> > +     memcpy(gcd->username, ctx->username, sizeof(ctx->username));
> > +     memcpy(gcd->password, ctx->password, sizeof(ctx->password));
> > +     gcd->state = STATE_ENABLING;
> > +     gcd->proto = ctx->proto;
> > +     gcd->auth_method = ctx->auth_method;
> > +
> > +     gemalto_get_cgdcont_command(modem, ctx->cid, ctx->proto, ctx->apn,
> buf,
> > +
>  sizeof(buf));
> > +
> > +     if (g_at_chat_send(gcd->chat, buf, none_prefix,
> > +                             setup_cb, gc, NULL) > 0)
> > +             return;
> > +
> > +     CALLBACK_WITH_FAILURE(cb, 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);
> > +
> > +     DBG("ok %d", ok);
> > +
> > +     gcd->active_context = 0;
> > +     gcd->state = STATE_IDLE;
> > +
> > +     CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> > +}
> > +
> > +static void gemaltowwan_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->state = STATE_DISABLING;
> > +     gcd->cb = cb;
> > +     gcd->cb_data = data;
> > +
> > +     if (gcd->use_wwan)
> > +             sprintf(buf, "AT^SWWAN=0,%u", gcd->active_context);
> > +     else
> > +             sprintf(buf, "AT+CGACT=%u,0", gcd->active_context);
> > +
> > +     if (g_at_chat_send(gcd->chat, buf, none_prefix,
> > +                             deactivate_cb, gc, NULL) > 0)
> > +             return;
> > +
> > +     CALLBACK_WITH_SUCCESS(cb, data);
> > +}
> > +
> > +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);
> > +     const char *event;
> > +     int cid = 0;
> > +     GAtResultIter iter;
> > +
> > +     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, "ME PDN DEACT"))
> > +             return;
> > +
> > +     sscanf(event, "%*s %*s %*s %u", &cid);
> > +
> > +     DBG("cid %d", cid);
> > +
> > +     if ((unsigned int) cid != gcd->active_context)
> > +             return;
> > +
> > +     ofono_gprs_context_deactivated(gc, gcd->active_context);
> > +
> > +     gcd->active_context = 0;
> > +     gcd->state = STATE_IDLE;
> > +}
> > +
> > +static int gemaltowwan_gprs_context_probe(struct ofono_gprs_context *gc,
> > +                                     unsigned int model, void *data)
> > +{
> > +     GAtChat *chat = data;
> > +     struct gprs_context_data *gcd;
> > +     struct ofono_modem *modem = ofono_gprs_context_get_modem(gc);
> > +
> > +     DBG("");
> > +
> > +     gcd = g_try_new0(struct gprs_context_data, 1);
> > +     if (gcd == NULL)
> > +             return -ENOMEM;
> > +
> > +     if (modem)
> > +             gcd->use_wwan = ofono_modem_get_integer(modem,
> "Gemalto_WWAN");
> > +     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 gemaltowwan_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);
> > +     g_free(gcd);
> > +}
> > +
> > +static void gemaltowwan_gprs_detach_shutdown(struct ofono_gprs_context
> *gc,
> > +                                     unsigned int cid)
> > +{
> > +     DBG("cid %u", cid);
> > +
> > +     ofono_gprs_context_deactivated(gc, cid);
> > +}
> > +
> > +static struct ofono_gprs_context_driver driver = {
> > +     .name                   = "gemaltowwanmodem",
> > +     .probe                  = gemaltowwan_gprs_context_probe,
> > +     .remove                 = gemaltowwan_gprs_context_remove,
> > +     .activate_primary       = gemaltowwan_gprs_activate_primary,
> > +     .deactivate_primary     = gemaltowwan_gprs_deactivate_primary,
> > +     .detach_shutdown        = gemaltowwan_gprs_detach_shutdown,
> > +};
> > +
> > +void gemaltowwan_gprs_context_init(void)
> > +{
> > +     ofono_gprs_context_driver_register(&driver);
> > +}
> > +
> > +void gemaltowwan_gprs_context_exit(void)
> > +{
> > +     ofono_gprs_context_driver_unregister(&driver);
> > +}
> >
>
> Regards,
> -Denis
>

Regards,
Giacinto

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 27929 bytes --]

Re: [PATCH 1/2] gemaltomodem: added Gemalto-specific gprs-context atom This atom name is not the same as the driver, because it might be necessary to add other grps-context atoms, according to the needs. The name is therefore referring to the main command used, AT^SWWAN.

[Giacinto Cifelli]

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

Hi again Denis,

On Sat, Sep 22, 2018 at 7:47 AM Giacinto Cifelli <gciofono@gmail.com> wrote:

> Hi Denis,
>
> On Fri, Sep 21, 2018 at 6:12 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
>> Hi Giacinto,
>>
>>
On 09/19/2018 10:50 PM, Giacinto Cifelli wrote:
>> > ---
>> >   drivers/gemaltomodem/gemaltomodem.c      |   2 +
>> >   drivers/gemaltomodem/gemaltomodem.h      |   3 +
>> >   drivers/gemaltomodem/gprs-context-wwan.c | 446
>> +++++++++++++++++++++++++++++++
>> >   3 files changed, 451 insertions(+)
>> >   create mode 100644 drivers/gemaltomodem/gprs-context-wwan.c
>> >
>> > diff --git a/drivers/gemaltomodem/gemaltomodem.c
>> b/drivers/gemaltomodem/gemaltomodem.c
>> > index 614ca81..7afd3c1 100644
>> > --- a/drivers/gemaltomodem/gemaltomodem.c
>> > +++ b/drivers/gemaltomodem/gemaltomodem.c
>
>
> +
>>
> > +static gboolean gemalto_get_auth_command(struct ofono_modem *modem, int
>> cid,
>> > +                             enum ofono_gprs_auth_method auth_method,
>> > +                             const char *username, const char
>> *password,
>> > +                             char *buf, guint buflen)
>> > +{
>> > +     int gto_auth = ofono_modem_get_integer(modem, "Gemalto_Auth");
>>
>> Please use a CamelCase-d variable for this.  E.g. GemaltoAuthType
>>
>
> ok. This is new for this code, because I have always seen only variables
> with underscores, but no problem.
> Personally I would prefer camelCase 'gemaltoAuthType', because it is more
> coherent in case of 1-part names like 'user',
> but I'll do as you say.
>

On a second reading, I think you meant the "Gemalto_Auth" ->
"GemaltoAuthType".
I have renamed it, and also gto_auth -> gemalto_auth, clearer.



>> Regards,
>> -Denis
>>
>
> Regards,
> Giacinto
>
>
Regards,
Giacinto

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 3917 bytes --]

Re: [PATCH 1/2] gemaltomodem: added Gemalto-specific gprs-context atom....

[Denis Kenzior]

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

Hi Giacinto,

>     So until the GPRS_AUTH_METHOD_NONE is actually introduced, you might
>     want to just get this upstream with the existing enumeration.  E.g.
>     just
>     assume that if username & password is empty, the auth method is none.
> 
> 
> I have pushed the new method NONE again, and I prefer to wait until it 
> is taken.
> 
> The current solution is really a non-documented hack.
> Checking whether user&password are empty to change the type of 
> authentication goes against the caller settings.
> It is better to give an error so that there is a chance to have good 
> settings.
> 

The issue is that you haven't properly addressed all the points that 
arise from adding a new enumeration.  You're touching something that 
essentially affects every driver and not updating these properly.  So 
take your pick, but nothing is going upstream until either this patch 
fits into the old framework, or the new framework is ready...

>     Fundamentally, your context activation has succeeded or it didn't.  The
>     DHCP operation is purely local between the modem and the host.  No
>     other
>     vendor in the world has this weird setup that you do.  So I don't
>     really
>     know what to tell you here as this ^SWWAN interaction is fundamentally
>     broken and it can't go upstream in this form.
> 
>     Can ^SWWAN support static ip reporting instead of DHCP?  That would be
>     preferred anyway as DHCP is slow and unneeded.
> 
> 
> This is a discussion about how Gemalto products work.
> 
> Admittedly, there are a few models, in the high-volume low-price range 
> (already deployed),
> that won't return from AT^SWWAN until a DHCP exchange is performed, as 
> per Sebastian's comment above.
> 

Can firmware be fixed / upgraded on these?

> The rest (which are the large majority) of the modules would allow a 
> 'standard' treatment, but also work with the code above.

So why don't we enable sane ones for now and deal with the insane ones 
later?

> 
> As per DHCP vs static, we have both, depending on the module.
> 
> Just for information, I have stepped upon networks *requiring* DHCP:
> in this kind of networks, an MBIM module that would normally return a 
> static settings,
> returns 0x00 in the "IPv4 configuration Available" field in of the 
> IP_CONFIGURATION answer,
> instead of the usual 0x0F (address, gateway, dns, mtu).
> So, it looks like that DHCP has to be considered as a valid alternative 
> also in this case.

This is fine.  You just set the IP configuration method accordingly.  We 
already have a few examples where the gprs_context driver might choose 
one or the other depending on whether support for a given command 
exists.  This can be extended to support cases where the network 
requires DHCP.  Which by the way is completely weird for cases such as 
PPP where no concept of DHCP really exists and your address has to be 
obtained by the time the context is activated.  So I wouldn't mind some 
extra details on what is *actually* going on here under the hood.

> 
> We cannot change every module and every network to fit our wishes.
> 
> Back to the code, I don't see a solution at the moment other than that one.
> I can add an 'if' to filter only the models that need it, but cannot 
> remove it completely.
> And this code would be specific for the driver, I don't see why it 
> couldn't go upstream.

So I would filter these models out from this driver.  Or see if they can 
work with PPP based driver and we can deal with these later.

Regards,
-Denis

Re: [PATCH 1/2] gemaltomodem: added Gemalto-specific gprs-context atom....

[Giacinto Cifelli]

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

Hi Denis,

On Mon, Sep 24, 2018 at 10:50 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> >     So until the GPRS_AUTH_METHOD_NONE is actually introduced, you might
> >     want to just get this upstream with the existing enumeration.  E.g.
> >     just
> >     assume that if username & password is empty, the auth method is none.
> >
> >
> > I have pushed the new method NONE again, and I prefer to wait until it
> > is taken.
> >
> > The current solution is really a non-documented hack.
> > Checking whether user&password are empty to change the type of
> > authentication goes against the caller settings.
> > It is better to give an error so that there is a chance to have good
> > settings.
> >
>
> The issue is that you haven't properly addressed all the points that
> arise from adding a new enumeration.  You're touching something that
> essentially affects every driver and not updating these properly.  So
> take your pick, but nothing is going upstream until either this patch
> fits into the old framework, or the new framework is ready...
>

Actually I have. But I am gong to re-send the relevant patches I have
already sent all together again, with some more verbose explanation.
Admittedly, I have looked into atmodem, mbimmodem, qmimodem, and a few
others, but I will check all of them before re-submitting.
qmi is already defaulting to AUTH_NONE internally if not PAP and not CHAP.
for mbim I have a similar patch, atmodem checks that it is one of {PAP,
CHAP} and does an error if not. So it doesn't need update. Also for PPP the
specification clearly says that it only takes the two methods (even if it
doesn't mention that if user or pwd are empty it goes to NONE).



> >     Fundamentally, your context activation has succeeded or it didn't.
> The
> >     DHCP operation is purely local between the modem and the host.  No
> >     other
> >     vendor in the world has this weird setup that you do.  So I don't
> >     really
> >     know what to tell you here as this ^SWWAN interaction is
> fundamentally
> >     broken and it can't go upstream in this form.
> >
> >     Can ^SWWAN support static ip reporting instead of DHCP?  That would
> be
> >     preferred anyway as DHCP is slow and unneeded.
> >
> >
> > This is a discussion about how Gemalto products work.
> >
> > Admittedly, there are a few models, in the high-volume low-price range
> > (already deployed),
> > that won't return from AT^SWWAN until a DHCP exchange is performed, as
> > per Sebastian's comment above.
> >
>
> Can firmware be fixed / upgraded on these?


No, too many on the field, and nobody is willing to change A change needs
re-certifications around the world,
for both Gemalto and the customers, with the risk of new certification
rules that will require even more changes.

Unfortunately it is for these low-end models that the customers expect to
have a ready-made framework,
in order to have also limited costs on software development.

> The rest (which are the large majority) of the modules would allow a
>
> 'standard' treatment, but also work with the code above.
>
> So why don't we enable sane ones for now and deal with the insane ones
> later?
>

I can do this, but I need to know what options I have in the code.
If there is no way out, it means we have to maintain an out-of-tree branch.


> >
> > As per DHCP vs static, we have both, depending on the module.
> >
> > Just for information, I have stepped upon networks *requiring* DHCP:
> > in this kind of networks, an MBIM module that would normally return a
> > static settings,
> > returns 0x00 in the "IPv4 configuration Available" field in of the
> > IP_CONFIGURATION answer,
> > instead of the usual 0x0F (address, gateway, dns, mtu).
> > So, it looks like that DHCP has to be considered as a valid alternative
> > also in this case.
>
> This is fine.  You just set the IP configuration method accordingly.  We
> already have a few examples where the gprs_context driver might choose
> one or the other depending on whether support for a given command
> exists.  This can be extended to support cases where the network
> requires DHCP.  Which by the way is completely weird for cases such as
> PPP where no concept of DHCP really exists and your address has to be
> obtained by the time the context is activated.  So I wouldn't mind some
> extra details on what is *actually* going on here under the hood.
>
> >
> > We cannot change every module and every network to fit our wishes.
> >
> > Back to the code, I don't see a solution at the moment other than that
> one.
> > I can add an 'if' to filter only the models that need it, but cannot
> > remove it completely.
> > And this code would be specific for the driver, I don't see why it
> > couldn't go upstream.
>
> So I would filter these models out from this driver.  Or see if they can
> work with PPP based driver and we can deal with these later.
>

I have tried PPP on these models, it is accepted but fails. So no PPP
either.

Regards,
> -Denis
>

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 6597 bytes --]