[PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Giacinto Cifelli]

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

---
 include/gprs-context.h |  1 +
 include/lte.h          | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/gprs-context.h b/include/gprs-context.h
index 20ca9ef..8869c12 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
 enum ofono_gprs_auth_method {
 	OFONO_GPRS_AUTH_METHOD_CHAP = 0,
 	OFONO_GPRS_AUTH_METHOD_PAP,
+	OFONO_GPRS_AUTH_METHOD_NONE,
 };
 
 struct ofono_gprs_primary_context {
diff --git a/include/lte.h b/include/lte.h
index ef84ab9..38587c3 100644
--- a/include/lte.h
+++ b/include/lte.h
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2016  Endocode AG. 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
@@ -28,14 +29,18 @@ extern "C" {
 
 #include <ofono/types.h>
 
-struct ofono_lte;
-
 struct ofono_lte_default_attach_info {
 	char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
+	enum ofono_gprs_proto proto;
+	enum ofono_gprs_auth_method auth_method;
+	char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
+	char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
 };
 
 typedef void (*ofono_lte_cb_t)(const struct ofono_error *error, void *data);
 
+struct ofono_lte;
+
 struct ofono_lte_driver {
 	const char *name;
 	int (*probe)(struct ofono_lte *lte, unsigned int vendor, void *data);
@@ -61,6 +66,8 @@ void ofono_lte_set_data(struct ofono_lte *lte, void *data);
 
 void *ofono_lte_get_data(const struct ofono_lte *lte);
 
+struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.9.1

[PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Giacinto Cifelli]

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

---
 src/gprs.c |  13 ++-
 src/lte.c  | 372 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 src/main.c |   5 -
 3 files changed, 341 insertions(+), 49 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 377eced..40f43e3 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -261,6 +261,10 @@ static const char *gprs_auth_method_to_string(enum ofono_gprs_auth_method auth)
 		return "chap";
 	case OFONO_GPRS_AUTH_METHOD_PAP:
 		return "pap";
+	case OFONO_GPRS_AUTH_METHOD_NONE:
+		return "none";
+	default:
+		return NULL;
 	};
 
 	return NULL;
@@ -275,6 +279,9 @@ static gboolean gprs_auth_method_from_string(const char *str,
 	} else if (g_str_equal(str, "pap")) {
 		*auth = OFONO_GPRS_AUTH_METHOD_PAP;
 		return TRUE;
+	} else if (g_str_equal(str, "none")) {
+		*auth = OFONO_GPRS_AUTH_METHOD_NONE;
+		return TRUE;
 	}
 
 	return FALSE;
@@ -1008,7 +1015,7 @@ static void pri_read_settings_callback(const struct ofono_error *error,
 
 	value = pri_ctx->active;
 
-	gprs->flags &= !GPRS_FLAG_ATTACHING;
+	gprs->flags &= ~GPRS_FLAG_ATTACHING;
 
 	gprs->driver_attached = TRUE;
 	gprs_set_attached_property(gprs, TRUE);
@@ -1635,6 +1642,9 @@ static void release_active_contexts(struct ofono_gprs *gprs)
 
 		if (gc->driver->detach_shutdown != NULL)
 			gc->driver->detach_shutdown(gc, ctx->context.cid);
+
+		/* Make sure the context is properly cleared */
+		release_context(ctx);
 	}
 }
 
@@ -2234,6 +2244,7 @@ static DBusMessage *gprs_remove_context(DBusConnection *conn,
 	}
 
 	DBG("Unregistering context: %s", ctx->path);
+	release_context(ctx);
 	context_dbus_unregister(ctx);
 	gprs->contexts = g_slist_remove(gprs->contexts, ctx);
 
diff --git a/src/lte.c b/src/lte.c
index a6d26b3..21b6a19 100644
--- a/src/lte.c
+++ b/src/lte.c
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2016  Endocode AG. 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
@@ -39,7 +40,11 @@
 
 #define SETTINGS_STORE "lte"
 #define SETTINGS_GROUP "Settings"
-#define DEFAULT_APN_KEY "DefaultAccessPointName"
+#define LTE_APN "AccessPointName"
+#define LTE_PROTO "Protocol"
+#define LTE_USERNAME "Username"
+#define LTE_PASSWORD "Password"
+#define LTE_AUTH_METHOD "AuthenticationMethod"
 
 struct ofono_lte {
 	const struct ofono_lte_driver *driver;
@@ -50,13 +55,82 @@ struct ofono_lte {
 	DBusMessage *pending;
 	struct ofono_lte_default_attach_info pending_info;
 	struct ofono_lte_default_attach_info info;
+	const char *prop_changed;
 };
 
 static GSList *g_drivers = NULL;
 
+static const char *gprs_proto_to_string(enum ofono_gprs_proto proto)
+{
+	switch (proto) {
+	case OFONO_GPRS_PROTO_IP:
+		return "ip";
+	case OFONO_GPRS_PROTO_IPV6:
+		return "ipv6";
+	case OFONO_GPRS_PROTO_IPV4V6:
+		return "dual";
+	};
+
+	return NULL;
+}
+
+static gboolean gprs_proto_from_string(const char *str,
+					enum ofono_gprs_proto *proto)
+{
+	if (g_str_equal(str, "ip")) {
+		*proto = OFONO_GPRS_PROTO_IP;
+		return TRUE;
+	} else if (g_str_equal(str, "ipv6")) {
+		*proto = OFONO_GPRS_PROTO_IPV6;
+		return TRUE;
+	} else if (g_str_equal(str, "dual")) {
+		*proto = OFONO_GPRS_PROTO_IPV4V6;
+		return TRUE;
+	}
+
+	return FALSE;
+}
+
+static const char *gprs_auth_method_to_string(enum ofono_gprs_auth_method auth)
+{
+	switch (auth) {
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+		return "chap";
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		return "pap";
+	case OFONO_GPRS_AUTH_METHOD_NONE:
+		return "none";
+	default:
+		return NULL;
+	};
+
+	return NULL;
+}
+
+static gboolean gprs_auth_method_from_string(const char *str,
+					enum ofono_gprs_auth_method *auth)
+{
+	if (g_str_equal(str, "chap")) {
+		*auth = OFONO_GPRS_AUTH_METHOD_CHAP;
+		return TRUE;
+	} else if (g_str_equal(str, "pap")) {
+		*auth = OFONO_GPRS_AUTH_METHOD_PAP;
+		return TRUE;
+	} else if (g_str_equal(str, "none")) {
+		*auth = OFONO_GPRS_AUTH_METHOD_NONE;
+		return TRUE;
+	}
+
+	return FALSE;
+}
+
 static void lte_load_settings(struct ofono_lte *lte)
 {
+	char *proto_str;
 	char *apn;
+	char *auth_method_str;
+	char *username;
+	char *password;
 
 	if (lte->imsi == NULL)
 		return;
@@ -69,114 +143,276 @@ static void lte_load_settings(struct ofono_lte *lte)
 		return;
 	}
 
-	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
-					DEFAULT_APN_KEY, NULL);
-	if (apn) {
+	proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_PROTO, NULL);
+	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_APN, NULL);
+	auth_method_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_AUTH_METHOD, NULL);
+	username = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_USERNAME, NULL);
+	password = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_PASSWORD, NULL);
+	if (proto_str == NULL)
+		proto_str = g_strdup("ip");
+
+	/* this must have a valid default */
+	if (!gprs_proto_from_string(proto_str, &lte->info.proto))
+		lte->info.proto = OFONO_GPRS_PROTO_IP;
+
+	if (apn)
 		strcpy(lte->info.apn, apn);
-		g_free(apn);
-	}
+
+	if (auth_method_str == NULL)
+		auth_method_str = g_strdup("none");
+
+	/* this must have a valid default */
+	if (!gprs_auth_method_from_string(auth_method_str,
+							&lte->info.auth_method))
+		lte->info.auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+	if (username)
+		strcpy(lte->info.username, username);
+
+	if (password)
+		strcpy(lte->info.password, password);
+
+	g_free(proto_str);
+	g_free(apn);
+	g_free(auth_method_str);
+	g_free(username);
+	g_free(password);
 }
 
 static DBusMessage *lte_get_properties(DBusConnection *conn,
 					DBusMessage *msg, void *data)
 {
 	struct ofono_lte *lte = data;
+	const char *proto = gprs_proto_to_string(lte->info.proto);
 	const char *apn = lte->info.apn;
+	const char* auth_method =
+			gprs_auth_method_to_string(lte->info.auth_method);
+	const char *username = lte->info.username;
+	const char *password = lte->info.password;
 	DBusMessage *reply;
 	DBusMessageIter iter;
 	DBusMessageIter dict;
 
 	reply = dbus_message_new_method_return(msg);
+
 	if (reply == NULL)
 		return NULL;
 
 	dbus_message_iter_init_append(reply, &iter);
-
 	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
 					OFONO_PROPERTIES_ARRAY_SIGNATURE,
 					&dict);
-	ofono_dbus_dict_append(&dict, DEFAULT_APN_KEY, DBUS_TYPE_STRING, &apn);
+	ofono_dbus_dict_append(&dict, LTE_PROTO, DBUS_TYPE_STRING, &proto);
+	ofono_dbus_dict_append(&dict, LTE_APN, DBUS_TYPE_STRING, &apn);
+	ofono_dbus_dict_append(&dict, LTE_AUTH_METHOD, DBUS_TYPE_STRING,
+					&auth_method);
+	ofono_dbus_dict_append(&dict, LTE_USERNAME, DBUS_TYPE_STRING,
+					&username);
+	ofono_dbus_dict_append(&dict, LTE_PASSWORD, DBUS_TYPE_STRING,
+					&password);
 	dbus_message_iter_close_container(&iter, &dict);
-
 	return reply;
 }
 
 static void lte_set_default_attach_info_cb(const struct ofono_error *error,
-						void *data)
+								void *data)
 {
 	struct ofono_lte *lte = data;
 	const char *path = __ofono_atom_get_path(lte->atom);
 	DBusConnection *conn = ofono_dbus_get_connection();
 	DBusMessage *reply;
-	const char *apn = lte->info.apn;
+	const char *propstr;
 
-	DBG("%s error %d", path, error->type);
+	if (error != NULL) {
+		DBG("%s error %d", path, error->type);
 
-	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
-		__ofono_dbus_pending_reply(&lte->pending,
-				__ofono_error_failed(lte->pending));
-		return;
+		if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+			__ofono_dbus_pending_reply(&lte->pending,
+					__ofono_error_failed(lte->pending));
+			return;
+		}
 	}
 
-	g_strlcpy(lte->info.apn, lte->pending_info.apn,
-			OFONO_GPRS_MAX_APN_LENGTH + 1);
+	if (g_str_equal(lte->prop_changed, LTE_PROTO)) {
+		lte->info.proto = lte->pending_info.proto;
+		propstr = gprs_proto_to_string(lte->info.proto);
 
-	if (lte->settings) {
-		if (strlen(lte->info.apn) == 0)
-			/* Clear entry on empty APN. */
-			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
-						DEFAULT_APN_KEY, NULL);
-		else
+		if (lte->settings)
 			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
-						DEFAULT_APN_KEY, lte->info.apn);
+							LTE_PROTO, propstr);
 
-		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
+	} else if (g_str_equal(lte->prop_changed, LTE_APN)) {
+		g_strlcpy(lte->info.apn, lte->pending_info.apn,
+						OFONO_GPRS_MAX_APN_LENGTH + 1);
+		propstr = lte->info.apn;
+
+		if (lte->settings) {
+
+			if (!*lte->info.apn)
+				/* Clear entry on empty APN. */
+				g_key_file_remove_key(lte->settings,
+					SETTINGS_GROUP, LTE_APN, NULL);
+			else
+				g_key_file_set_string(lte->settings,
+					SETTINGS_GROUP, LTE_APN, lte->info.apn);
+		}
+
+	} else if (g_str_equal(lte->prop_changed, LTE_AUTH_METHOD)) {
+		lte->info.auth_method = lte->pending_info.auth_method;
+		propstr = gprs_auth_method_to_string(lte->info.auth_method);
+
+		if (lte->settings)
+			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
+						LTE_AUTH_METHOD, propstr);
+
+	} else if (g_str_equal(lte->prop_changed, LTE_USERNAME)) {
+		g_strlcpy(lte->info.username, lte->pending_info.username,
+			OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+		propstr = lte->info.username;
+
+		if (lte->settings) {
+
+			if (!*lte->info.username)
+				/* Clear entry on empty Username. */
+				g_key_file_remove_key(lte->settings,
+					SETTINGS_GROUP, LTE_USERNAME, NULL);
+			else
+				g_key_file_set_string(lte->settings,
+					SETTINGS_GROUP,
+					LTE_USERNAME, lte->info.username);
+		}
+
+	} else if (g_str_equal(lte->prop_changed, LTE_PASSWORD)) {
+		g_strlcpy(lte->info.password, lte->pending_info.password,
+			OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+		propstr = lte->info.password;
+
+		if (lte->settings) {
+
+			if (strlen(lte->info.password) == 0)
+				/* Clear entry on empty Password. */
+				g_key_file_remove_key(lte->settings,
+					SETTINGS_GROUP, LTE_PASSWORD, NULL);
+			else
+				g_key_file_set_string(lte->settings,
+					SETTINGS_GROUP,
+					LTE_PASSWORD, lte->info.password);
+		}
+
+	} else {
+		return;
 	}
 
+	if (lte->settings)
+		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
+
 	reply = dbus_message_new_method_return(lte->pending);
 	__ofono_dbus_pending_reply(&lte->pending, reply);
-
 	ofono_dbus_signal_property_changed(conn, path,
 					OFONO_CONNECTION_CONTEXT_INTERFACE,
-					DEFAULT_APN_KEY,
-					DBUS_TYPE_STRING, &apn);
+					lte->prop_changed,
+					DBUS_TYPE_STRING, &propstr);
 }
 
-static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
+static DBusMessage *lte_set_proto(struct ofono_lte *lte,
 				DBusConnection *conn, DBusMessage *msg,
-				const char *apn)
+				enum ofono_gprs_proto proto)
 {
-	if (lte->driver->set_default_attach_info == NULL)
-		return __ofono_error_not_implemented(msg);
-
-	if (lte->pending)
-		return __ofono_error_busy(msg);
+	void *data = lte;
 
-	if (g_str_equal(apn, lte->info.apn))
+	if (proto == lte->info.proto)
 		return dbus_message_new_method_return(msg);
 
+	lte->pending = dbus_message_ref(msg);
+	lte->pending_info.proto = proto;
+	lte_set_default_attach_info_cb(NULL, data);
+	return dbus_message_ref(msg);;
+}
+
+static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
+				DBusConnection *conn, DBusMessage *msg,
+				const char *apn)
+{
 	/* We do care about empty value: it can be used for reset. */
 	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
 		return __ofono_error_invalid_format(msg);
 
 	lte->pending = dbus_message_ref(msg);
+	g_strlcpy(lte->info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
+	lte->driver->set_default_attach_info(lte, &lte->info,
+					lte_set_default_attach_info_cb, lte);
+	return dbus_message_ref(msg);;
+}
 
-	g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
+static DBusMessage *lte_set_auth_method(struct ofono_lte *lte,
+				DBusConnection *conn, DBusMessage *msg,
+				enum ofono_gprs_auth_method auth_method)
+{
+	void *data = lte;
 
-	lte->driver->set_default_attach_info(lte, &lte->pending_info,
-					lte_set_default_attach_info_cb, lte);
+	if (auth_method == lte->info.auth_method)
+		return dbus_message_new_method_return(msg);
 
-	return NULL;
+	lte->pending = dbus_message_ref(msg);
+	lte->pending_info.auth_method = auth_method;
+	lte_set_default_attach_info_cb(NULL, data);
+	return dbus_message_ref(msg);;
+}
+
+static DBusMessage *lte_set_username(struct ofono_lte *lte,
+				DBusConnection *conn, DBusMessage *msg,
+				const char *username)
+{
+	void *data = lte;
+
+	if (g_str_equal(username, lte->info.username))
+		return dbus_message_new_method_return(msg);
+
+	lte->pending = dbus_message_ref(msg);
+	g_strlcpy(lte->pending_info.username, username,
+					OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+	lte_set_default_attach_info_cb(NULL, data);
+	return dbus_message_ref(msg);;
+}
+
+static DBusMessage *lte_set_password(struct ofono_lte *lte,
+				DBusConnection *conn, DBusMessage *msg,
+				const char *password)
+{
+	void *data = lte;
+
+	if (g_str_equal(password, lte->info.password))
+		return dbus_message_new_method_return(msg);
+
+	lte->pending = dbus_message_ref(msg);
+	g_strlcpy(lte->pending_info.password, password,
+					OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+	lte_set_default_attach_info_cb(NULL, data);
+
+	return dbus_message_ref(msg);;
 }
 
 static DBusMessage *lte_set_property(DBusConnection *conn,
-					DBusMessage *msg, void *data)
+						DBusMessage *msg, void *data)
 {
 	struct ofono_lte *lte = data;
 	DBusMessageIter iter;
 	DBusMessageIter var;
 	const char *property;
 	const char *str;
+	enum ofono_gprs_auth_method auth_method;
+	enum ofono_gprs_proto proto;
+
+	if (lte->driver->set_default_attach_info == NULL)
+		return __ofono_error_not_implemented(msg);
+
+	if (lte->pending)
+		return __ofono_error_busy(msg);
 
 	if (!dbus_message_iter_init(msg, &iter))
 		return __ofono_error_invalid_args(msg);
@@ -192,13 +428,58 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
 
 	dbus_message_iter_recurse(&iter, &var);
 
-	if (!strcmp(property, DEFAULT_APN_KEY)) {
+	lte->prop_changed=property;
+
+	if (!strcmp(property, LTE_PROTO)) {
+
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &str);
+
+		if (gprs_proto_from_string(str, &proto))
+			return lte_set_proto(lte, conn, msg, proto);
+		else
+			return __ofono_error_invalid_format(msg);
+
+	} else if (!strcmp(property, LTE_APN)) {
+
 		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
 			return __ofono_error_invalid_args(msg);
 
 		dbus_message_iter_get_basic(&var, &str);
 
 		return lte_set_default_apn(lte, conn, msg, str);
+
+	} else if (!strcmp(property, LTE_AUTH_METHOD)) {
+
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &str);
+
+		if (gprs_auth_method_from_string(str, &auth_method))
+			return lte_set_auth_method(lte, conn, msg, auth_method);
+		else
+			return __ofono_error_invalid_format(msg);
+
+	} else  if (!strcmp(property, LTE_USERNAME)) {
+
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &str);
+
+		return lte_set_username(lte, conn, msg, str);
+
+	} else  if (!strcmp(property, LTE_PASSWORD)) {
+
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &str);
+
+		return lte_set_password(lte, conn, msg, str);
 	}
 
 	return __ofono_error_invalid_args(msg);
@@ -373,3 +654,8 @@ void *ofono_lte_get_data(const struct ofono_lte *lte)
 {
 	return lte->driver_data;
 }
+
+struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
+{
+	return __ofono_atom_get_modem(lte->atom);
+}
\ No newline at end of file
diff --git a/src/main.c b/src/main.c
index 2d359dd..d8a06ba 100644
--- a/src/main.c
+++ b/src/main.c
@@ -211,11 +211,6 @@ int main(int argc, char **argv)
 	struct ell_event_source *source;
 #endif
 
-#ifdef NEED_THREADS
-	if (g_thread_supported() == FALSE)
-		g_thread_init(NULL);
-#endif
-
 	context = g_option_context_new(NULL);
 	g_option_context_add_main_entries(context, options, NULL);
 
-- 
1.9.1

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Denis Kenzior]

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

Hi Giacinto,

On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:
> ---
>   src/gprs.c |  13 ++-
>   src/lte.c  | 372 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>   src/main.c |   5 -
>   3 files changed, 341 insertions(+), 49 deletions(-)

So you seem to have 3 completely unrelated things going on here...  At 
the very minimum this should be 3 commits.

Also, you're adding LTE D-Bus implementation without updating or 
proposing changes to doc/lte-api.txt.

> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 377eced..40f43e3 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -261,6 +261,10 @@ static const char *gprs_auth_method_to_string(enum ofono_gprs_auth_method auth)
>   		return "chap";
>   	case OFONO_GPRS_AUTH_METHOD_PAP:
>   		return "pap";
> +	case OFONO_GPRS_AUTH_METHOD_NONE:
> +		return "none";
> +	default:
> +		return NULL;
>   	};

Okay, but this patch likely needs to also ensure that username / 
password are not settable if method is NONE.  And follow up with an 
update of all things that depend on OFONO_GPRS_AUTH_METHOD usage.  E.g. 
drivers, provisioning plugins, etc.

>   
>   	return NULL;
> @@ -275,6 +279,9 @@ static gboolean gprs_auth_method_from_string(const char *str,
>   	} else if (g_str_equal(str, "pap")) {
>   		*auth = OFONO_GPRS_AUTH_METHOD_PAP;
>   		return TRUE;
> +	} else if (g_str_equal(str, "none")) {
> +		*auth = OFONO_GPRS_AUTH_METHOD_NONE;
> +		return TRUE;
>   	}
>   
>   	return FALSE;
> @@ -1008,7 +1015,7 @@ static void pri_read_settings_callback(const struct ofono_error *error,
>   
>   	value = pri_ctx->active;
>   
> -	gprs->flags &= !GPRS_FLAG_ATTACHING;
> +	gprs->flags &= ~GPRS_FLAG_ATTACHING;

Okay, but this is a separate fix and should be documented properly.

>   
>   	gprs->driver_attached = TRUE;
>   	gprs_set_attached_property(gprs, TRUE);
> @@ -1635,6 +1642,9 @@ static void release_active_contexts(struct ofono_gprs *gprs)
>   
>   		if (gc->driver->detach_shutdown != NULL)
>   			gc->driver->detach_shutdown(gc, ctx->context.cid);
> +
> +		/* Make sure the context is properly cleared */
> +		release_context(ctx);

As above, seems to be an unrelated fix.

>   	}
>   }
>   
> @@ -2234,6 +2244,7 @@ static DBusMessage *gprs_remove_context(DBusConnection *conn,
>   	}
>   
>   	DBG("Unregistering context: %s", ctx->path);
> +	release_context(ctx);

As above.  You can't just lump these changes into something unrelated. 
You need to submit these fixes separately and describe what each one is 
fixing and why.

>   	context_dbus_unregister(ctx);
>   	gprs->contexts = g_slist_remove(gprs->contexts, ctx);
>   
> diff --git a/src/lte.c b/src/lte.c
> index a6d26b3..21b6a19 100644
> --- a/src/lte.c
> +++ b/src/lte.c
> @@ -3,6 +3,7 @@
>    *  oFono - Open Source Telephony
>    *
>    *  Copyright (C) 2016  Endocode AG. 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
> @@ -39,7 +40,11 @@
>   
>   #define SETTINGS_STORE "lte"
>   #define SETTINGS_GROUP "Settings"
> -#define DEFAULT_APN_KEY "DefaultAccessPointName"
> +#define LTE_APN "AccessPointName"

No.  You can't do that.  The D-Bus API is stable and cannot be changed. 
This is why you propose D-Bus API changes first, so that they can be 
reviewed separately for any impacts.

> +#define LTE_PROTO "Protocol"
> +#define LTE_USERNAME "Username"
> +#define LTE_PASSWORD "Password"
> +#define LTE_AUTH_METHOD "AuthenticationMethod"
>   
>   struct ofono_lte {
>   	const struct ofono_lte_driver *driver;
> @@ -50,13 +55,82 @@ struct ofono_lte {
>   	DBusMessage *pending;
>   	struct ofono_lte_default_attach_info pending_info;
>   	struct ofono_lte_default_attach_info info;
> +	const char *prop_changed;

??  What memory location is this const char pointing to?

Why don't you just use an enum.  Or even better, don't do this at all 
and simply compare pending_info & info to generate the right signal.

>   };
>   
>   static GSList *g_drivers = NULL;
>   
> +static const char *gprs_proto_to_string(enum ofono_gprs_proto proto)
> +{
> +	switch (proto) {
> +	case OFONO_GPRS_PROTO_IP:
> +		return "ip";
> +	case OFONO_GPRS_PROTO_IPV6:
> +		return "ipv6";
> +	case OFONO_GPRS_PROTO_IPV4V6:
> +		return "dual";
> +	};
> +
> +	return NULL;
> +}

This needs to be moved to common.c

> +
> +static gboolean gprs_proto_from_string(const char *str,
> +					enum ofono_gprs_proto *proto)
> +{
> +	if (g_str_equal(str, "ip")) {
> +		*proto = OFONO_GPRS_PROTO_IP;
> +		return TRUE;
> +	} else if (g_str_equal(str, "ipv6")) {
> +		*proto = OFONO_GPRS_PROTO_IPV6;
> +		return TRUE;
> +	} else if (g_str_equal(str, "dual")) {
> +		*proto = OFONO_GPRS_PROTO_IPV4V6;
> +		return TRUE;
> +	}
> +
> +	return FALSE;
> +}
> +
> +static const char *gprs_auth_method_to_string(enum ofono_gprs_auth_method auth)
> +{
> +	switch (auth) {
> +	case OFONO_GPRS_AUTH_METHOD_CHAP:
> +		return "chap";
> +	case OFONO_GPRS_AUTH_METHOD_PAP:
> +		return "pap";
> +	case OFONO_GPRS_AUTH_METHOD_NONE:
> +		return "none";
> +	default:
> +		return NULL;
> +	};
> +
> +	return NULL;
> +}
> +
> +static gboolean gprs_auth_method_from_string(const char *str,
> +					enum ofono_gprs_auth_method *auth)
> +{
> +	if (g_str_equal(str, "chap")) {
> +		*auth = OFONO_GPRS_AUTH_METHOD_CHAP;
> +		return TRUE;
> +	} else if (g_str_equal(str, "pap")) {
> +		*auth = OFONO_GPRS_AUTH_METHOD_PAP;
> +		return TRUE;
> +	} else if (g_str_equal(str, "none")) {
> +		*auth = OFONO_GPRS_AUTH_METHOD_NONE;
> +		return TRUE;
> +	}
> +
> +	return FALSE;
> +}
> +

And all these as well

>   static void lte_load_settings(struct ofono_lte *lte)
>   {
> +	char *proto_str;
>   	char *apn;
> +	char *auth_method_str;
> +	char *username;
> +	char *password;
>   
>   	if (lte->imsi == NULL)
>   		return;
> @@ -69,114 +143,276 @@ static void lte_load_settings(struct ofono_lte *lte)
>   		return;
>   	}
>   
> -	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
> -					DEFAULT_APN_KEY, NULL);
> -	if (apn) {
> +	proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> +				LTE_PROTO, NULL);
> +	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> +				LTE_APN, NULL);

And now you broke the default attach APN setting of every existing oFono 
user.  No, you cannot do that.

> +	auth_method_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> +				LTE_AUTH_METHOD, NULL);
> +	username = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> +				LTE_USERNAME, NULL);
> +	password = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> +				LTE_PASSWORD, NULL);
> +	if (proto_str == NULL)
> +		proto_str = g_strdup("ip");
> +
> +	/* this must have a valid default */
> +	if (!gprs_proto_from_string(proto_str, &lte->info.proto))
> +		lte->info.proto = OFONO_GPRS_PROTO_IP;
> +
> +	if (apn)
>   		strcpy(lte->info.apn, apn);
> -		g_free(apn);
> -	}
> +
> +	if (auth_method_str == NULL)
> +		auth_method_str = g_strdup("none");
> +
> +	/* this must have a valid default */
> +	if (!gprs_auth_method_from_string(auth_method_str,
> +							&lte->info.auth_method))
> +		lte->info.auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
> +
> +	if (username)
> +		strcpy(lte->info.username, username);
> +
> +	if (password)
> +		strcpy(lte->info.password, password);
> +
> +	g_free(proto_str);
> +	g_free(apn);
> +	g_free(auth_method_str);
> +	g_free(username);
> +	g_free(password);
>   }
>   
>   static DBusMessage *lte_get_properties(DBusConnection *conn,
>   					DBusMessage *msg, void *data)
>   {
>   	struct ofono_lte *lte = data;
> +	const char *proto = gprs_proto_to_string(lte->info.proto);
>   	const char *apn = lte->info.apn;
> +	const char* auth_method =
> +			gprs_auth_method_to_string(lte->info.auth_method);
> +	const char *username = lte->info.username;
> +	const char *password = lte->info.password;
>   	DBusMessage *reply;
>   	DBusMessageIter iter;
>   	DBusMessageIter dict;
>   
>   	reply = dbus_message_new_method_return(msg);
> +

Don't change random code that doesn't need to be changed.  You should be 
keeping your changes minimal and on-point.  Any changes unrelated to the 
purpose of the patch need to be submitted separately.

>   	if (reply == NULL)
>   		return NULL;
>   
>   	dbus_message_iter_init_append(reply, &iter);
> -
>   	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
>   					OFONO_PROPERTIES_ARRAY_SIGNATURE,
>   					&dict);
> -	ofono_dbus_dict_append(&dict, DEFAULT_APN_KEY, DBUS_TYPE_STRING, &apn);
> +	ofono_dbus_dict_append(&dict, LTE_PROTO, DBUS_TYPE_STRING, &proto);
> +	ofono_dbus_dict_append(&dict, LTE_APN, DBUS_TYPE_STRING, &apn);
> +	ofono_dbus_dict_append(&dict, LTE_AUTH_METHOD, DBUS_TYPE_STRING,
> +					&auth_method);
> +	ofono_dbus_dict_append(&dict, LTE_USERNAME, DBUS_TYPE_STRING,
> +					&username);
> +	ofono_dbus_dict_append(&dict, LTE_PASSWORD, DBUS_TYPE_STRING,
> +					&password);
>   	dbus_message_iter_close_container(&iter, &dict);
> -
>   	return reply;
>   }
>   
>   static void lte_set_default_attach_info_cb(const struct ofono_error *error,
> -						void *data)
> +								void *data)
>   {
>   	struct ofono_lte *lte = data;
>   	const char *path = __ofono_atom_get_path(lte->atom);
>   	DBusConnection *conn = ofono_dbus_get_connection();
>   	DBusMessage *reply;
> -	const char *apn = lte->info.apn;
> +	const char *propstr;
>   
> -	DBG("%s error %d", path, error->type);
> +	if (error != NULL) {
> +		DBG("%s error %d", path, error->type);

Why?  Error should never be NULL.  If you're faking a call to this 
function, why don't you just pass a proper error structure in instead of 
messing with this?  And even then I would think it would be much cleaner 
not to do this in the first place...

>   
> -	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> -		__ofono_dbus_pending_reply(&lte->pending,
> -				__ofono_error_failed(lte->pending));
> -		return;
> +		if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +			__ofono_dbus_pending_reply(&lte->pending,
> +					__ofono_error_failed(lte->pending));
> +			return;
> +		}

This change is non-sense.

>   	}
>   
> -	g_strlcpy(lte->info.apn, lte->pending_info.apn,
> -			OFONO_GPRS_MAX_APN_LENGTH + 1);
> +	if (g_str_equal(lte->prop_changed, LTE_PROTO)) {
> +		lte->info.proto = lte->pending_info.proto;
> +		propstr = gprs_proto_to_string(lte->info.proto);
>   
> -	if (lte->settings) {
> -		if (strlen(lte->info.apn) == 0)
> -			/* Clear entry on empty APN. */
> -			g_key_file_remove_key(lte->settings, SETTINGS_GROUP,
> -						DEFAULT_APN_KEY, NULL);
> -		else
> +		if (lte->settings)
>   			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
> -						DEFAULT_APN_KEY, lte->info.apn);
> +							LTE_PROTO, propstr);
>   
> -		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
> +	} else if (g_str_equal(lte->prop_changed, LTE_APN)) {
> +		g_strlcpy(lte->info.apn, lte->pending_info.apn,
> +						OFONO_GPRS_MAX_APN_LENGTH + 1);
> +		propstr = lte->info.apn;
> +
> +		if (lte->settings) {
> +
> +			if (!*lte->info.apn)
> +				/* Clear entry on empty APN. */
> +				g_key_file_remove_key(lte->settings,
> +					SETTINGS_GROUP, LTE_APN, NULL);
> +			else
> +				g_key_file_set_string(lte->settings,
> +					SETTINGS_GROUP, LTE_APN, lte->info.apn);
> +		}
> +
> +	} else if (g_str_equal(lte->prop_changed, LTE_AUTH_METHOD)) {
> +		lte->info.auth_method = lte->pending_info.auth_method;
> +		propstr = gprs_auth_method_to_string(lte->info.auth_method);
> +
> +		if (lte->settings)
> +			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
> +						LTE_AUTH_METHOD, propstr);
> +
> +	} else if (g_str_equal(lte->prop_changed, LTE_USERNAME)) {
> +		g_strlcpy(lte->info.username, lte->pending_info.username,
> +			OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
> +		propstr = lte->info.username;
> +
> +		if (lte->settings) {
> +
> +			if (!*lte->info.username)
> +				/* Clear entry on empty Username. */
> +				g_key_file_remove_key(lte->settings,
> +					SETTINGS_GROUP, LTE_USERNAME, NULL);
> +			else
> +				g_key_file_set_string(lte->settings,
> +					SETTINGS_GROUP,
> +					LTE_USERNAME, lte->info.username);
> +		}
> +

You have boiler-plate code for nearly all of these settings.  Why don't 
you actually use a function for this?

> +	} else if (g_str_equal(lte->prop_changed, LTE_PASSWORD)) {
> +		g_strlcpy(lte->info.password, lte->pending_info.password,
> +			OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
> +		propstr = lte->info.password;
> +
> +		if (lte->settings) {
> +
> +			if (strlen(lte->info.password) == 0)
> +				/* Clear entry on empty Password. */
> +				g_key_file_remove_key(lte->settings,
> +					SETTINGS_GROUP, LTE_PASSWORD, NULL);
> +			else
> +				g_key_file_set_string(lte->settings,
> +					SETTINGS_GROUP,
> +					LTE_PASSWORD, lte->info.password);
> +		}
> +
> +	} else {
> +		return;

You have a dbus message pending, you can't do that...

>   	}
>   
> +	if (lte->settings)
> +		storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
> +
>   	reply = dbus_message_new_method_return(lte->pending);
>   	__ofono_dbus_pending_reply(&lte->pending, reply);
> -
>   	ofono_dbus_signal_property_changed(conn, path,
>   					OFONO_CONNECTION_CONTEXT_INTERFACE,
> -					DEFAULT_APN_KEY,
> -					DBUS_TYPE_STRING, &apn);
> +					lte->prop_changed,
> +					DBUS_TYPE_STRING, &propstr);
>   }
>   
> -static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
> +static DBusMessage *lte_set_proto(struct ofono_lte *lte,
>   				DBusConnection *conn, DBusMessage *msg,
> -				const char *apn)
> +				enum ofono_gprs_proto proto)
>   {
> -	if (lte->driver->set_default_attach_info == NULL)
> -		return __ofono_error_not_implemented(msg);
> -
> -	if (lte->pending)
> -		return __ofono_error_busy(msg);
> +	void *data = lte;
>   
> -	if (g_str_equal(apn, lte->info.apn))
> +	if (proto == lte->info.proto)
>   		return dbus_message_new_method_return(msg);
>   
> +	lte->pending = dbus_message_ref(msg);
> +	lte->pending_info.proto = proto;
> +	lte_set_default_attach_info_cb(NULL, data);
> +	return dbus_message_ref(msg);;

I have no idea what is happening here.  Whatever it is, it is wrong.

> +}
> +
> +static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
> +				DBusConnection *conn, DBusMessage *msg,
> +				const char *apn)
> +{
>   	/* We do care about empty value: it can be used for reset. */
>   	if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
>   		return __ofono_error_invalid_format(msg);
>   
>   	lte->pending = dbus_message_ref(msg);
> +	g_strlcpy(lte->info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
> +	lte->driver->set_default_attach_info(lte, &lte->info,
> +					lte_set_default_attach_info_cb, lte);
> +	return dbus_message_ref(msg);
> +}
>   
> -	g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
> +static DBusMessage *lte_set_auth_method(struct ofono_lte *lte,
> +				DBusConnection *conn, DBusMessage *msg,
> +				enum ofono_gprs_auth_method auth_method)
> +{
> +	void *data = lte;
>   
> -	lte->driver->set_default_attach_info(lte, &lte->pending_info,
> -					lte_set_default_attach_info_cb, lte);
> +	if (auth_method == lte->info.auth_method)
> +		return dbus_message_new_method_return(msg);
>   
> -	return NULL;
> +	lte->pending = dbus_message_ref(msg);
> +	lte->pending_info.auth_method = auth_method;
> +	lte_set_default_attach_info_cb(NULL, data);
> +	return dbus_message_ref(msg);;
> +}
> +
> +static DBusMessage *lte_set_username(struct ofono_lte *lte,
> +				DBusConnection *conn, DBusMessage *msg,
> +				const char *username)
> +{
> +	void *data = lte;
> +
> +	if (g_str_equal(username, lte->info.username))
> +		return dbus_message_new_method_return(msg);
> +
> +	lte->pending = dbus_message_ref(msg);
> +	g_strlcpy(lte->pending_info.username, username,
> +					OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
> +	lte_set_default_attach_info_cb(NULL, data);
> +	return dbus_message_ref(msg);;
> +}
> +
> +static DBusMessage *lte_set_password(struct ofono_lte *lte,
> +				DBusConnection *conn, DBusMessage *msg,
> +				const char *password)
> +{
> +	void *data = lte;
> +
> +	if (g_str_equal(password, lte->info.password))
> +		return dbus_message_new_method_return(msg);
> +
> +	lte->pending = dbus_message_ref(msg);
> +	g_strlcpy(lte->pending_info.password, password,
> +					OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
> +	lte_set_default_attach_info_cb(NULL, data);
> +

You do realize you're never actually calling into the driver method, 
right?  So none of these changes actually go out to the modem.  Have you 
actually tested any of this?

> +	return dbus_message_ref(msg);;
>   }
>   
>   static DBusMessage *lte_set_property(DBusConnection *conn,
> -					DBusMessage *msg, void *data)
> +						DBusMessage *msg, void *data)
>   {
>   	struct ofono_lte *lte = data;
>   	DBusMessageIter iter;
>   	DBusMessageIter var;
>   	const char *property;
>   	const char *str;
> +	enum ofono_gprs_auth_method auth_method;
> +	enum ofono_gprs_proto proto;
> +
> +	if (lte->driver->set_default_attach_info == NULL)
> +		return __ofono_error_not_implemented(msg);
> +
> +	if (lte->pending)
> +		return __ofono_error_busy(msg);
>   
>   	if (!dbus_message_iter_init(msg, &iter))
>   		return __ofono_error_invalid_args(msg);
> @@ -192,13 +428,58 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
>   
>   	dbus_message_iter_recurse(&iter, &var);
>   
> -	if (!strcmp(property, DEFAULT_APN_KEY)) {
> +	lte->prop_changed=property;
> +
> +	if (!strcmp(property, LTE_PROTO)) {
> +
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &str);
> +
> +		if (gprs_proto_from_string(str, &proto))
> +			return lte_set_proto(lte, conn, msg, proto);

The return from this callback is always supposed to be a method_return 
or NULL if the method_return will be done asynchronously (in your case 
always)  You're somehow returning the method_call itself...

> +		else
> +			return __ofono_error_invalid_format(msg);
> +
> +	} else if (!strcmp(property, LTE_APN)) {
> +
>   		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
>   			return __ofono_error_invalid_args(msg);
>   
>   		dbus_message_iter_get_basic(&var, &str);
>   
>   		return lte_set_default_apn(lte, conn, msg, str);
> +
> +	} else if (!strcmp(property, LTE_AUTH_METHOD)) {
> +
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &str);
> +
> +		if (gprs_auth_method_from_string(str, &auth_method))
> +			return lte_set_auth_method(lte, conn, msg, auth_method);
> +		else
> +			return __ofono_error_invalid_format(msg);
> +
> +	} else  if (!strcmp(property, LTE_USERNAME)) {
> +
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &str);
> +
> +		return lte_set_username(lte, conn, msg, str);
> +
> +	} else  if (!strcmp(property, LTE_PASSWORD)) {
> +
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &str);
> +
> +		return lte_set_password(lte, conn, msg, str);
>   	}
>   
>   	return __ofono_error_invalid_args(msg);
> @@ -373,3 +654,8 @@ void *ofono_lte_get_data(const struct ofono_lte *lte)
>   {
>   	return lte->driver_data;
>   }
> +
> +struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
> +{
> +	return __ofono_atom_get_modem(lte->atom);
> +}
> \ No newline at end of file

Fix this.

> diff --git a/src/main.c b/src/main.c
> index 2d359dd..d8a06ba 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -211,11 +211,6 @@ int main(int argc, char **argv)
>   	struct ell_event_source *source;
>   #endif
>   
> -#ifdef NEED_THREADS
> -	if (g_thread_supported() == FALSE)
> -		g_thread_init(NULL);
> -#endif
> -

Why? This is completely unrelated and can be turned off via configure.

>   	context = g_option_context_new(NULL);
>   	g_option_context_add_main_entries(context, options, NULL);
>   
> 

Regards,
-Denis

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Giacinto Cifelli]

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

Hi Denis,

On Wed, Sep 19, 2018 at 5:04 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:
> > ---
> >   src/gprs.c |  13 ++-
> >   src/lte.c  | 372
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >   src/main.c |   5 -
> >   3 files changed, 341 insertions(+), 49 deletions(-)
>
> So you seem to have 3 completely unrelated things going on here...  At
> the very minimum this should be 3 commits.
>
> Also, you're adding LTE D-Bus implementation without updating or
> proposing changes to doc/lte-api.txt.
>

so, first the documentation?


>
> > diff --git a/src/gprs.c b/src/gprs.c
> > index 377eced..40f43e3 100644
> > --- a/src/gprs.c
> > +++ b/src/gprs.c
> > @@ -261,6 +261,10 @@ static const char *gprs_auth_method_to_string(enum
> ofono_gprs_auth_method auth)
> >               return "chap";
> >       case OFONO_GPRS_AUTH_METHOD_PAP:
> >               return "pap";
> > +     case OFONO_GPRS_AUTH_METHOD_NONE:
> > +             return "none";
> > +     default:
> > +             return NULL;
> >       };
>
> Okay, but this patch likely needs to also ensure that username /
> password are not settable if method is NONE.  And follow up with an
> update of all things that depend on OFONO_GPRS_AUTH_METHOD usage.  E.g.
> drivers, provisioning plugins, etc.
>

Ok, I will take care of this as well.

>
> >       return NULL;
> > @@ -275,6 +279,9 @@ static gboolean gprs_auth_method_from_string(const
> char *str,
> >       } else if (g_str_equal(str, "pap")) {
> >               *auth = OFONO_GPRS_AUTH_METHOD_PAP;
> >               return TRUE;
> > +     } else if (g_str_equal(str, "none")) {
> > +             *auth = OFONO_GPRS_AUTH_METHOD_NONE;
> > +             return TRUE;
> >       }
> >
> >       return FALSE;
> > @@ -1008,7 +1015,7 @@ static void pri_read_settings_callback(const
> struct ofono_error *error,
> >
> >       value = pri_ctx->active;
> >
> > -     gprs->flags &= !GPRS_FLAG_ATTACHING;
> > +     gprs->flags &= ~GPRS_FLAG_ATTACHING;
>
> Okay, but this is a separate fix and should be documented properly.
>

Okay


>
> >
> >       gprs->driver_attached = TRUE;
> >       gprs_set_attached_property(gprs, TRUE);
> > @@ -1635,6 +1642,9 @@ static void release_active_contexts(struct
> ofono_gprs *gprs)
> >
> >               if (gc->driver->detach_shutdown != NULL)
> >                       gc->driver->detach_shutdown(gc, ctx->context.cid);
> > +
> > +             /* Make sure the context is properly cleared */
> > +             release_context(ctx);
>
> As above, seems to be an unrelated fix.
>

It is. I will submit another patch. the same below.


> >       }
> >   }
> >
> > @@ -2234,6 +2244,7 @@ static DBusMessage
> *gprs_remove_context(DBusConnection *conn,
> >       }
> >
> >       DBG("Unregistering context: %s", ctx->path);
> > +     release_context(ctx);
>
> As above.  You can't just lump these changes into something unrelated.
> You need to submit these fixes separately and describe what each one is
> fixing and why.
>
> >       context_dbus_unregister(ctx);
> >       gprs->contexts = g_slist_remove(gprs->contexts, ctx);
> >
> > diff --git a/src/lte.c b/src/lte.c
> > index a6d26b3..21b6a19 100644
> > --- a/src/lte.c
> > +++ b/src/lte.c
> > @@ -3,6 +3,7 @@
> >    *  oFono - Open Source Telephony
> >    *
> >    *  Copyright (C) 2016  Endocode AG. 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
> > @@ -39,7 +40,11 @@
> >
> >   #define SETTINGS_STORE "lte"
> >   #define SETTINGS_GROUP "Settings"
> > -#define DEFAULT_APN_KEY "DefaultAccessPointName"
> > +#define LTE_APN "AccessPointName"
>
> No.  You can't do that.  The D-Bus API is stable and cannot be changed.
> This is why you propose D-Bus API changes first, so that they can be
> reviewed separately for any impacts.
>

So there can be no unification with the GPRS naming now that the D-Bus API
is set?



> > +#define LTE_PROTO "Protocol"
> > +#define LTE_USERNAME "Username"
> > +#define LTE_PASSWORD "Password"
> > +#define LTE_AUTH_METHOD "AuthenticationMethod"
> >
> >   struct ofono_lte {
> >       const struct ofono_lte_driver *driver;
> > @@ -50,13 +55,82 @@ struct ofono_lte {
> >       DBusMessage *pending;
> >       struct ofono_lte_default_attach_info pending_info;
> >       struct ofono_lte_default_attach_info info;
> > +     const char *prop_changed;
>
> ??  What memory location is this const char pointing to?
>

it is initialized to null with the containing structure.


> Why don't you just use an enum.  Or even better, don't do this at all
> and simply compare pending_info & info to generate the right signal.
>

I will try to change it this way.


>
> >   };
> >
> >   static GSList *g_drivers = NULL;
> >
> > +static const char *gprs_proto_to_string(enum ofono_gprs_proto proto)
> > +{
> > +     switch (proto) {
> > +     case OFONO_GPRS_PROTO_IP:
> > +             return "ip";
> > +     case OFONO_GPRS_PROTO_IPV6:
> > +             return "ipv6";
> > +     case OFONO_GPRS_PROTO_IPV4V6:
> > +             return "dual";
> > +     };
> > +
> > +     return NULL;
> > +}
>
> This needs to be moved to common.c
>

I have tried and failed miserably. But in your email to Slava you have
mentioned moving also stuff to types.h, maybe that is the key.


>
> > +
> > +static gboolean gprs_proto_from_string(const char *str,
> > +                                     enum ofono_gprs_proto *proto)
> > +{
> > +     if (g_str_equal(str, "ip")) {
> > +             *proto = OFONO_GPRS_PROTO_IP;
> > +             return TRUE;
> > +     } else if (g_str_equal(str, "ipv6")) {
> > +             *proto = OFONO_GPRS_PROTO_IPV6;
> > +             return TRUE;
> > +     } else if (g_str_equal(str, "dual")) {
> > +             *proto = OFONO_GPRS_PROTO_IPV4V6;
> > +             return TRUE;
> > +     }
> > +
> > +     return FALSE;
> > +}
> > +
> > +static const char *gprs_auth_method_to_string(enum
> ofono_gprs_auth_method auth)
> > +{
> > +     switch (auth) {
> > +     case OFONO_GPRS_AUTH_METHOD_CHAP:
> > +             return "chap";
> > +     case OFONO_GPRS_AUTH_METHOD_PAP:
> > +             return "pap";
> > +     case OFONO_GPRS_AUTH_METHOD_NONE:
> > +             return "none";
> > +     default:
> > +             return NULL;
> > +     };
> > +
> > +     return NULL;
> > +}
> > +
> > +static gboolean gprs_auth_method_from_string(const char *str,
> > +                                     enum ofono_gprs_auth_method *auth)
> > +{
> > +     if (g_str_equal(str, "chap")) {
> > +             *auth = OFONO_GPRS_AUTH_METHOD_CHAP;
> > +             return TRUE;
> > +     } else if (g_str_equal(str, "pap")) {
> > +             *auth = OFONO_GPRS_AUTH_METHOD_PAP;
> > +             return TRUE;
> > +     } else if (g_str_equal(str, "none")) {
> > +             *auth = OFONO_GPRS_AUTH_METHOD_NONE;
> > +             return TRUE;
> > +     }
> > +
> > +     return FALSE;
> > +}
> > +
>
> And all these as well
>
> >   static void lte_load_settings(struct ofono_lte *lte)
> >   {
> > +     char *proto_str;
> >       char *apn;
> > +     char *auth_method_str;
> > +     char *username;
> > +     char *password;
> >
> >       if (lte->imsi == NULL)
> >               return;
> > @@ -69,114 +143,276 @@ static void lte_load_settings(struct ofono_lte
> *lte)
> >               return;
> >       }
> >
> > -     apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP ,
> > -                                     DEFAULT_APN_KEY, NULL);
> > -     if (apn) {
> > +     proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> > +                             LTE_PROTO, NULL);
> > +     apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> > +                             LTE_APN, NULL);
>
> And now you broke the default attach APN setting of every existing oFono
> user.  No, you cannot do that.
>

What about reading also the previous key?

> +     auth_method_str = g_key_file_get_string(lte->settings,
> SETTINGS_GROUP,
> > +                             LTE_AUTH_METHOD, NULL);
> > +     username = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> > +                             LTE_USERNAME, NULL);
> > +     password = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
> > +                             LTE_PASSWORD, NULL);
> > +     if (proto_str == NULL)
> > +             proto_str = g_strdup("ip");
> > +
> > +     /* this must have a valid default */
> > +     if (!gprs_proto_from_string(proto_str, &lte->info.proto))
> > +             lte->info.proto = OFONO_GPRS_PROTO_IP;
> > +
> > +     if (apn)
> >               strcpy(lte->info.apn, apn);
> > -             g_free(apn);
> > -     }
> > +
> > +     if (auth_method_str == NULL)
> > +             auth_method_str = g_strdup("none");
> > +
> > +     /* this must have a valid default */
> > +     if (!gprs_auth_method_from_string(auth_method_str,
> > +
>  &lte->info.auth_method))
> > +             lte->info.auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
> > +
> > +     if (username)
> > +             strcpy(lte->info.username, username);
> > +
> > +     if (password)
> > +             strcpy(lte->info.password, password);
> > +
> > +     g_free(proto_str);
> > +     g_free(apn);
> > +     g_free(auth_method_str);
> > +     g_free(username);
> > +     g_free(password);
> >   }
> >
> >   static DBusMessage *lte_get_properties(DBusConnection *conn,
> >                                       DBusMessage *msg, void *data)
> >   {
> >       struct ofono_lte *lte = data;
> > +     const char *proto = gprs_proto_to_string(lte->info.proto);
> >       const char *apn = lte->info.apn;
> > +     const char* auth_method =
> > +                     gprs_auth_method_to_string(lte->info.auth_method);
> > +     const char *username = lte->info.username;
> > +     const char *password = lte->info.password;
> >       DBusMessage *reply;
> >       DBusMessageIter iter;
> >       DBusMessageIter dict;
> >
> >       reply = dbus_message_new_method_return(msg);
> > +
>
> Don't change random code that doesn't need to be changed.  You should be
> keeping your changes minimal and on-point.  Any changes unrelated to the
> purpose of the patch need to be submitted separately.
>

ok.


> >       if (reply == NULL)
> >               return NULL;
> >
> >       dbus_message_iter_init_append(reply, &iter);
> > -
> >       dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> >                                       OFONO_PROPERTIES_ARRAY_SIGNATURE,
> >                                       &dict);
> > -     ofono_dbus_dict_append(&dict, DEFAULT_APN_KEY, DBUS_TYPE_STRING,
> &apn);
> > +     ofono_dbus_dict_append(&dict, LTE_PROTO, DBUS_TYPE_STRING, &proto);
> > +     ofono_dbus_dict_append(&dict, LTE_APN, DBUS_TYPE_STRING, &apn);
> > +     ofono_dbus_dict_append(&dict, LTE_AUTH_METHOD, DBUS_TYPE_STRING,
> > +                                     &auth_method);
> > +     ofono_dbus_dict_append(&dict, LTE_USERNAME, DBUS_TYPE_STRING,
> > +                                     &username);
> > +     ofono_dbus_dict_append(&dict, LTE_PASSWORD, DBUS_TYPE_STRING,
> > +                                     &password);
> >       dbus_message_iter_close_container(&iter, &dict);
> > -
> >       return reply;
> >   }
> >
> >   static void lte_set_default_attach_info_cb(const struct ofono_error
> *error,
> > -                                             void *data)
> > +                                                             void *data)
> >   {
> >       struct ofono_lte *lte = data;
> >       const char *path = __ofono_atom_get_path(lte->atom);
> >       DBusConnection *conn = ofono_dbus_get_connection();
> >       DBusMessage *reply;
> > -     const char *apn = lte->info.apn;
> > +     const char *propstr;
> >
> > -     DBG("%s error %d", path, error->type);
> > +     if (error != NULL) {
> > +             DBG("%s error %d", path, error->type);
>
> Why?  Error should never be NULL.  If you're faking a call to this
> function, why don't you just pass a proper error structure in instead of
> messing with this?  And even then I would think it would be much cleaner
> not to do this in the first place...
>

ok.


>
> >
> > -     if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> > -             __ofono_dbus_pending_reply(&lte->pending,
> > -                             __ofono_error_failed(lte->pending));
> > -             return;
> > +             if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> > +                     __ofono_dbus_pending_reply(&lte->pending,
> > +
>  __ofono_error_failed(lte->pending));
> > +                     return;
> > +             }
>
> This change is non-sense.
>
> >       }
> >
> > -     g_strlcpy(lte->info.apn, lte->pending_info.apn,
> > -                     OFONO_GPRS_MAX_APN_LENGTH + 1);
> > +     if (g_str_equal(lte->prop_changed, LTE_PROTO)) {
> > +             lte->info.proto = lte->pending_info.proto;
> > +             propstr = gprs_proto_to_string(lte->info.proto);
> >
> > -     if (lte->settings) {
> > -             if (strlen(lte->info.apn) == 0)
> > -                     /* Clear entry on empty APN. */
> > -                     g_key_file_remove_key(lte->settings,
> SETTINGS_GROUP,
> > -                                             DEFAULT_APN_KEY, NULL);
> > -             else
> > +             if (lte->settings)
> >                       g_key_file_set_string(lte->settings,
> SETTINGS_GROUP,
> > -                                             DEFAULT_APN_KEY,
> lte->info.apn);
> > +                                                     LTE_PROTO,
> propstr);
> >
> > -             storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
> > +     } else if (g_str_equal(lte->prop_changed, LTE_APN)) {
> > +             g_strlcpy(lte->info.apn, lte->pending_info.apn,
> > +                                             OFONO_GPRS_MAX_APN_LENGTH
> + 1);
> > +             propstr = lte->info.apn;
> > +
> > +             if (lte->settings) {
> > +
> > +                     if (!*lte->info.apn)
> > +                             /* Clear entry on empty APN. */
> > +                             g_key_file_remove_key(lte->settings,
> > +                                     SETTINGS_GROUP, LTE_APN, NULL);
> > +                     else
> > +                             g_key_file_set_string(lte->settings,
> > +                                     SETTINGS_GROUP, LTE_APN,
> lte->info.apn);
> > +             }
> > +
> > +     } else if (g_str_equal(lte->prop_changed, LTE_AUTH_METHOD)) {
> > +             lte->info.auth_method = lte->pending_info.auth_method;
> > +             propstr =
> gprs_auth_method_to_string(lte->info.auth_method);
> > +
> > +             if (lte->settings)
> > +                     g_key_file_set_string(lte->settings,
> SETTINGS_GROUP,
> > +                                             LTE_AUTH_METHOD, propstr);
> > +
> > +     } else if (g_str_equal(lte->prop_changed, LTE_USERNAME)) {
> > +             g_strlcpy(lte->info.username, lte->pending_info.username,
> > +                     OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
> > +             propstr = lte->info.username;
> > +
> > +             if (lte->settings) {
> > +
> > +                     if (!*lte->info.username)
> > +                             /* Clear entry on empty Username. */
> > +                             g_key_file_remove_key(lte->settings,
> > +                                     SETTINGS_GROUP, LTE_USERNAME,
> NULL);
> > +                     else
> > +                             g_key_file_set_string(lte->settings,
> > +                                     SETTINGS_GROUP,
> > +                                     LTE_USERNAME, lte->info.username);
> > +             }
> > +
>
> You have boiler-plate code for nearly all of these settings.  Why don't
> you actually use a function for this?
>

Yes.


>
> > +     } else if (g_str_equal(lte->prop_changed, LTE_PASSWORD)) {
> > +             g_strlcpy(lte->info.password, lte->pending_info.password,
> > +                     OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
> > +             propstr = lte->info.password;
> > +
> > +             if (lte->settings) {
> > +
> > +                     if (strlen(lte->info.password) == 0)
> > +                             /* Clear entry on empty Password. */
> > +                             g_key_file_remove_key(lte->settings,
> > +                                     SETTINGS_GROUP, LTE_PASSWORD,
> NULL);
> > +                     else
> > +                             g_key_file_set_string(lte->settings,
> > +                                     SETTINGS_GROUP,
> > +                                     LTE_PASSWORD, lte->info.password);
> > +             }
> > +
> > +     } else {
> > +             return;
>
> You have a dbus message pending, you can't do that...
>

I looked at this line for a long time, and looked bad, but couldn't put my
finger on it.
Thank you for highlighting the issue.


>
> >       }
> >
> > +     if (lte->settings)
> > +             storage_sync(lte->imsi, SETTINGS_STORE, lte->settings);
> > +
> >       reply = dbus_message_new_method_return(lte->pending);
> >       __ofono_dbus_pending_reply(&lte->pending, reply);
> > -
> >       ofono_dbus_signal_property_changed(conn, path,
> >                                       OFONO_CONNECTION_CONTEXT_INTERFACE,
> > -                                     DEFAULT_APN_KEY,
> > -                                     DBUS_TYPE_STRING, &apn);
> > +                                     lte->prop_changed,
> > +                                     DBUS_TYPE_STRING, &propstr);
> >   }
> >
> > -static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
> > +static DBusMessage *lte_set_proto(struct ofono_lte *lte,
> >                               DBusConnection *conn, DBusMessage *msg,
> > -                             const char *apn)
> > +                             enum ofono_gprs_proto proto)
> >   {
> > -     if (lte->driver->set_default_attach_info == NULL)
> > -             return __ofono_error_not_implemented(msg);
> > -
> > -     if (lte->pending)
> > -             return __ofono_error_busy(msg);
> > +     void *data = lte;
> >
> > -     if (g_str_equal(apn, lte->info.apn))
> > +     if (proto == lte->info.proto)
> >               return dbus_message_new_method_return(msg);
> >
> > +     lte->pending = dbus_message_ref(msg);
> > +     lte->pending_info.proto = proto;
> > +     lte_set_default_attach_info_cb(NULL, data);
> > +     return dbus_message_ref(msg);;
>
> I have no idea what is happening here.  Whatever it is, it is wrong.
>

It works.


>
> > +}
> > +
> > +static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
> > +                             DBusConnection *conn, DBusMessage *msg,
> > +                             const char *apn)
> > +{
> >       /* We do care about empty value: it can be used for reset. */
> >       if (is_valid_apn(apn) == FALSE && apn[0] != '\0')
> >               return __ofono_error_invalid_format(msg);
> >
> >       lte->pending = dbus_message_ref(msg);
> > +     g_strlcpy(lte->info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
> > +     lte->driver->set_default_attach_info(lte, &lte->info,
> > +                                     lte_set_default_attach_info_cb,
> lte);
> > +     return dbus_message_ref(msg);
> > +}
> >
> > -     g_strlcpy(lte->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH +
> 1);
> > +static DBusMessage *lte_set_auth_method(struct ofono_lte *lte,
> > +                             DBusConnection *conn, DBusMessage *msg,
> > +                             enum ofono_gprs_auth_method auth_method)
> > +{
> > +     void *data = lte;
> >
> > -     lte->driver->set_default_attach_info(lte, &lte->pending_info,
> > -                                     lte_set_default_attach_info_cb,
> lte);
> > +     if (auth_method == lte->info.auth_method)
> > +             return dbus_message_new_method_return(msg);
> >
> > -     return NULL;
> > +     lte->pending = dbus_message_ref(msg);
> > +     lte->pending_info.auth_method = auth_method;
> > +     lte_set_default_attach_info_cb(NULL, data);
> > +     return dbus_message_ref(msg);;
> > +}
> > +
> > +static DBusMessage *lte_set_username(struct ofono_lte *lte,
> > +                             DBusConnection *conn, DBusMessage *msg,
> > +                             const char *username)
> > +{
> > +     void *data = lte;
> > +
> > +     if (g_str_equal(username, lte->info.username))
> > +             return dbus_message_new_method_return(msg);
> > +
> > +     lte->pending = dbus_message_ref(msg);
> > +     g_strlcpy(lte->pending_info.username, username,
> > +                                     OFONO_GPRS_MAX_USERNAME_LENGTH +
> 1);
> > +     lte_set_default_attach_info_cb(NULL, data);
> > +     return dbus_message_ref(msg);;
> > +}
> > +
> > +static DBusMessage *lte_set_password(struct ofono_lte *lte,
> > +                             DBusConnection *conn, DBusMessage *msg,
> > +                             const char *password)
> > +{
> > +     void *data = lte;
> > +
> > +     if (g_str_equal(password, lte->info.password))
> > +             return dbus_message_new_method_return(msg);
> > +
> > +     lte->pending = dbus_message_ref(msg);
> > +     g_strlcpy(lte->pending_info.password, password,
> > +                                     OFONO_GPRS_MAX_PASSWORD_LENGTH +
> 1);
> > +     lte_set_default_attach_info_cb(NULL, data);
> > +
>
> You do realize you're never actually calling into the driver method,
> right?  So none of these changes actually go out to the modem.  Have you
> actually tested any of this?
>

Yes, it works. Actually the only call is when the APN is set, as mentioned
in the lte-api.txt.
And at that point all parameters are also set in the module.
It is not possible to set separately protocol and apn, and auth_method,
username, and password.
For ublox modules, the auth_method is also part of the APN name.

So we kept the call into the module when the APN is set, and previously to
it all other parameters are set.

You have also mentioned that somewhere we should also verify that with
AUTH_NONE there are no user/pwd.
This also can only be verified at the end.

Any suggestions to improve this, given these limitations?


> +     return dbus_message_ref(msg);;
> >   }
> >
> >   static DBusMessage *lte_set_property(DBusConnection *conn,
> > -                                     DBusMessage *msg, void *data)
> > +                                             DBusMessage *msg, void
> *data)
> >   {
> >       struct ofono_lte *lte = data;
> >       DBusMessageIter iter;
> >       DBusMessageIter var;
> >       const char *property;
> >       const char *str;
> > +     enum ofono_gprs_auth_method auth_method;
> > +     enum ofono_gprs_proto proto;
> > +
> > +     if (lte->driver->set_default_attach_info == NULL)
> > +             return __ofono_error_not_implemented(msg);
> > +
> > +     if (lte->pending)
> > +             return __ofono_error_busy(msg);
> >
> >       if (!dbus_message_iter_init(msg, &iter))
> >               return __ofono_error_invalid_args(msg);
> > @@ -192,13 +428,58 @@ static DBusMessage
> *lte_set_property(DBusConnection *conn,
> >
> >       dbus_message_iter_recurse(&iter, &var);
> >
> > -     if (!strcmp(property, DEFAULT_APN_KEY)) {
> > +     lte->prop_changed=property;
> > +
> > +     if (!strcmp(property, LTE_PROTO)) {
> > +
> > +             if (dbus_message_iter_get_arg_type(&var) !=
> DBUS_TYPE_STRING)
> > +                     return __ofono_error_invalid_args(msg);
> > +
> > +             dbus_message_iter_get_basic(&var, &str);
> > +
> > +             if (gprs_proto_from_string(str, &proto))
> > +                     return lte_set_proto(lte, conn, msg, proto);
>
> The return from this callback is always supposed to be a method_return
> or NULL if the method_return will be done asynchronously (in your case
> always)  You're somehow returning the method_call itself...
>

I am not sure I follow you, but you suggested to restructure the code, so I
will come back on this later.


>
> > +             else
> > +                     return __ofono_error_invalid_format(msg);
> > +
> > +     } else if (!strcmp(property, LTE_APN)) {
> > +
> >               if (dbus_message_iter_get_arg_type(&var) !=
> DBUS_TYPE_STRING)
> >                       return __ofono_error_invalid_args(msg);
> >
> >               dbus_message_iter_get_basic(&var, &str);
> >
> >               return lte_set_default_apn(lte, conn, msg, str);
> > +
> > +     } else if (!strcmp(property, LTE_AUTH_METHOD)) {
> > +
> > +             if (dbus_message_iter_get_arg_type(&var) !=
> DBUS_TYPE_STRING)
> > +                     return __ofono_error_invalid_args(msg);
> > +
> > +             dbus_message_iter_get_basic(&var, &str);
> > +
> > +             if (gprs_auth_method_from_string(str, &auth_method))
> > +                     return lte_set_auth_method(lte, conn, msg,
> auth_method);
> > +             else
> > +                     return __ofono_error_invalid_format(msg);
> > +
> > +     } else  if (!strcmp(property, LTE_USERNAME)) {
> > +
> > +             if (dbus_message_iter_get_arg_type(&var) !=
> DBUS_TYPE_STRING)
> > +                     return __ofono_error_invalid_args(msg);
> > +
> > +             dbus_message_iter_get_basic(&var, &str);
> > +
> > +             return lte_set_username(lte, conn, msg, str);
> > +
> > +     } else  if (!strcmp(property, LTE_PASSWORD)) {
> > +
> > +             if (dbus_message_iter_get_arg_type(&var) !=
> DBUS_TYPE_STRING)
> > +                     return __ofono_error_invalid_args(msg);
> > +
> > +             dbus_message_iter_get_basic(&var, &str);
> > +
> > +             return lte_set_password(lte, conn, msg, str);
> >       }
> >
> >       return __ofono_error_invalid_args(msg);
> > @@ -373,3 +654,8 @@ void *ofono_lte_get_data(const struct ofono_lte *lte)
> >   {
> >       return lte->driver_data;
> >   }
> > +
> > +struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
> > +{
> > +     return __ofono_atom_get_modem(lte->atom);
> > +}
> > \ No newline at end of file
>
> Fix this.
>

?


>
> > diff --git a/src/main.c b/src/main.c
> > index 2d359dd..d8a06ba 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -211,11 +211,6 @@ int main(int argc, char **argv)
> >       struct ell_event_source *source;
> >   #endif
> >
> > -#ifdef NEED_THREADS
> > -     if (g_thread_supported() == FALSE)
> > -             g_thread_init(NULL);
> > -#endif
> > -
>
> Why? This is completely unrelated and can be turned off via configure.
>
> >       context = g_option_context_new(NULL);
> >       g_option_context_add_main_entries(context, options, NULL);
> >
> >
>
>
I agree it is unrelated, I will post a separate post, but I turn on
NEED_THREADS, and the build fails. Looking at the g_thread documentations,
nowadays it has to come out of the code:

g_thread_init has been deprecated since version 2.32 and should not be used
in newly-written code.

This function is no longer necessary. The GLib threading system is
automatically initialized at the start of your program.



> Regards,
> -Denis
>

Regards,
Giacinto

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

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Denis Kenzior]

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

Hi Giacinto,

> so, first the documentation?

Correct.  Whenever you touch something that affects the D-Bus API, it is 
really preferable to have the D-Bus API changes in the preceding commit. 
  That way the reviewers can cross-reference what is being proposed to 
the actual implementation.  Anything that breaks the D-Bus API can also 
be spotted much earlier.

oFono's D-Bus API is stable.  That means we can add new things, but we 
cannot change existing ones as that will break existing clients.

We also cannot break existing settings, as that would break oFono 
installations on existing devices if they are upgraded.

This is a handicap that we have to live with right now.

<snip>

> 
> So there can be no unification with the GPRS naming now that the D-Bus 
> API is set?

I'm afraid not.  But I'm not sure why this is a problem?

> 
>      > +#define LTE_PROTO "Protocol"
>      > +#define LTE_USERNAME "Username"
>      > +#define LTE_PASSWORD "Password"
>      > +#define LTE_AUTH_METHOD "AuthenticationMethod"
>      >
>      >   struct ofono_lte {
>      >       const struct ofono_lte_driver *driver;
>      > @@ -50,13 +55,82 @@ struct ofono_lte {
>      >       DBusMessage *pending;
>      >       struct ofono_lte_default_attach_info pending_info;
>      >       struct ofono_lte_default_attach_info info;
>      > +     const char *prop_changed;
> 
>     ??  What memory location is this const char pointing to?
> 
> 
> it is initialized to null with the containing structure.

That is not what I'm asking.  This pointer points into data owned by 
DBusMessage and the semantics of whether it is valid or not are not 
enforced.  Avoid that at all cost...

> 
> What about reading also the previous key?

Ideally you shouldn't change the key in the first place.  But if you 
are, then yes you need to be able to read legacy settings versions in 
order to be backwards-compatible.

> 
>     You do realize you're never actually calling into the driver method,
>     right?  So none of these changes actually go out to the modem.  Have
>     you
>     actually tested any of this?
> 
> 
> Yes, it works. Actually the only call is when the APN is set, as 
> mentioned in the lte-api.txt.

No, it really doesn't...

> And at that point all parameters are also set in the module.
> It is not possible to set separately protocol and apn, and auth_method, 
> username, and password.
> For ublox modules, the auth_method is also part of the APN name.
> 
> So we kept the call into the module when the APN is set, and previously 
> to it all other parameters are set.

You simply cannot do that.  You cannot assume that the client knows how 
your API is implemented underneath.  So if they set the APN first and 
then change the username, that has to work.  This is why the 
default_attach_info contains everything.  If anything is changed the 
entire structure is sent to the modem and every parameter is updated.

> 
> You have also mentioned that somewhere we should also verify that with 
> AUTH_NONE there are no user/pwd.
> This also can only be verified at the end.
> 
> Any suggestions to improve this, given these limitations?
> 

See above.  Any parameter change has to trigger 
set_default_attach_info() call.  If something is invalid, then you have 
to return a D-Bus error.

For example, if my method is set to 'none' and I try to do 
LongTermEvolution.SetProperty("DefaultUsername", "foo") that should 
return an error.

> 
>      > +     return dbus_message_ref(msg);;
>      >   }
>      >
>      >   static DBusMessage *lte_set_property(DBusConnection *conn,
>      > -                                     DBusMessage *msg, void *data)
>      > +                                             DBusMessage *msg,
>     void *data)
>      >   {
>      >       struct ofono_lte *lte = data;
>      >       DBusMessageIter iter;
>      >       DBusMessageIter var;
>      >       const char *property;
>      >       const char *str;
>      > +     enum ofono_gprs_auth_method auth_method;
>      > +     enum ofono_gprs_proto proto;
>      > +
>      > +     if (lte->driver->set_default_attach_info == NULL)
>      > +             return __ofono_error_not_implemented(msg);
>      > +
>      > +     if (lte->pending)
>      > +             return __ofono_error_busy(msg);
>      >
>      >       if (!dbus_message_iter_init(msg, &iter))
>      >               return __ofono_error_invalid_args(msg);
>      > @@ -192,13 +428,58 @@ static DBusMessage
>     *lte_set_property(DBusConnection *conn,
>      >
>      >       dbus_message_iter_recurse(&iter, &var);
>      >
>      > -     if (!strcmp(property, DEFAULT_APN_KEY)) {
>      > +     lte->prop_changed=property;
>      > +
>      > +     if (!strcmp(property, LTE_PROTO)) {
>      > +
>      > +             if (dbus_message_iter_get_arg_type(&var) !=
>     DBUS_TYPE_STRING)
>      > +                     return __ofono_error_invalid_args(msg);
>      > +
>      > +             dbus_message_iter_get_basic(&var, &str);
>      > +
>      > +             if (gprs_proto_from_string(str, &proto))
>      > +                     return lte_set_proto(lte, conn, msg, proto);
> 
>     The return from this callback is always supposed to be a method_return
>     or NULL if the method_return will be done asynchronously (in your case
>     always)  You're somehow returning the method_call itself...
> 
> 
> I am not sure I follow you, but you suggested to restructure the code, 
> so I will come back on this later.

You need to understand the semantics of GDBus.  GDBUS_ASYNC_METHOD() 
callback can only return the D-Bus method_return for message or NULL if 
the method return will be generated asynchronously.  You cannot return 
the message itself.  While this may 'work' the behavior is undefined.

>      > +struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte)
>      > +{
>      > +     return __ofono_atom_get_modem(lte->atom);
>      > +}
>      > \ No newline at end of file
> 
>     Fix this.
> 
> 
> ?

You have no newline after the last '}' and git complains.  If git 
complains I cannot apply the patch...

> 
> I agree it is unrelated, I will post a separate post, but I turn on 
> NEED_THREADS, and the build fails. Looking at the g_thread 
> documentations, nowadays it has to come out of the code:
> 
> |g_thread_init| has been deprecated since version 2.32 and should not be 
> used in newly-written code.
> 
> This function is no longer necessary. The GLib threading system is 
> automatically initialized at the start of your program.
> 

Okay, please send this as a separate patch.  We might need to remove 
this configure option as well.

Regards,
-Denis

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Giacinto Cifelli]

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

On Wed, Sep 19, 2018 at 6:30 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> > so, first the documentation?
>
> Correct.  Whenever you touch something that affects the D-Bus API, it is
> really preferable to have the D-Bus API changes in the preceding commit.
>   That way the reviewers can cross-reference what is being proposed to
> the actual implementation.  Anything that breaks the D-Bus API can also
> be spotted much earlier.
>
> oFono's D-Bus API is stable.  That means we can add new things, but we
> cannot change existing ones as that will break existing clients.
>
> We also cannot break existing settings, as that would break oFono
> installations on existing devices if they are upgraded.
>
> This is a handicap that we have to live with right now.
>
> <snip>
>
> >
> > So there can be no unification with the GPRS naming now that the D-Bus
> > API is set?
>
> I'm afraid not.  But I'm not sure why this is a problem?
>
> >
> >      > +#define LTE_PROTO "Protocol"
> >      > +#define LTE_USERNAME "Username"
> >      > +#define LTE_PASSWORD "Password"
> >      > +#define LTE_AUTH_METHOD "AuthenticationMethod"
> >      >
> >      >   struct ofono_lte {
> >      >       const struct ofono_lte_driver *driver;
> >      > @@ -50,13 +55,82 @@ struct ofono_lte {
> >      >       DBusMessage *pending;
> >      >       struct ofono_lte_default_attach_info pending_info;
> >      >       struct ofono_lte_default_attach_info info;
> >      > +     const char *prop_changed;
> >
> >     ??  What memory location is this const char pointing to?
> >
> >
> > it is initialized to null with the containing structure.
>
> That is not what I'm asking.  This pointer points into data owned by
> DBusMessage and the semantics of whether it is valid or not are not
> enforced.  Avoid that at all cost...
>
> >
> > What about reading also the previous key?
>
> Ideally you shouldn't change the key in the first place.  But if you
> are, then yes you need to be able to read legacy settings versions in
> order to be backwards-compatible.
>
> >
> >     You do realize you're never actually calling into the driver method,
> >     right?  So none of these changes actually go out to the modem.  Have
> >     you
> >     actually tested any of this?
> >
> >
> > Yes, it works. Actually the only call is when the APN is set, as
> > mentioned in the lte-api.txt.
>
> No, it really doesn't...
>
> > And at that point all parameters are also set in the module.
> > It is not possible to set separately protocol and apn, and auth_method,
> > username, and password.
> > For ublox modules, the auth_method is also part of the APN name.
> >
> > So we kept the call into the module when the APN is set, and previously
> > to it all other parameters are set.
>
> You simply cannot do that.  You cannot assume that the client knows how
> your API is implemented underneath.  So if they set the APN first and
> then change the username, that has to work.  This is why the
> default_attach_info contains everything.  If anything is changed the
> entire structure is sent to the modem and every parameter is updated.
>

Hi Denis,

I had a look at this all, and I have a problem. I cannot check the
parameters as they are entered one by one.
Example: if I blank user/pwd when the authentication is changed to NONE,
then if changed again to CHAP, the module will reject it.
Shall I store the parameters, or keep them also in case of error?

Another way would be to have a SetParameters() function, and set them all
together, including the APN, and not allowing writing them separately,
apart from the APN which already exists.
I don't really like it, either.

Or introduce an atom function that is called before modem->set_online(true)?

thanks,
Giacinto


>
> >
> > You have also mentioned that somewhere we should also verify that with
> > AUTH_NONE there are no user/pwd.
> > This also can only be verified at the end.
> >
> > Any suggestions to improve this, given these limitations?
> >
>
> See above.  Any parameter change has to trigger
> set_default_attach_info() call.  If something is invalid, then you have
> to return a D-Bus error.
>
> For example, if my method is set to 'none' and I try to do
> LongTermEvolution.SetProperty("DefaultUsername", "foo") that should
> return an error.
>
> >
> >      > +     return dbus_message_ref(msg);;
> >      >   }
> >      >
> >      >   static DBusMessage *lte_set_property(DBusConnection *conn,
> >      > -                                     DBusMessage *msg, void
> *data)
> >      > +                                             DBusMessage *msg,
> >     void *data)
> >      >   {
> >      >       struct ofono_lte *lte = data;
> >      >       DBusMessageIter iter;
> >      >       DBusMessageIter var;
> >      >       const char *property;
> >      >       const char *str;
> >      > +     enum ofono_gprs_auth_method auth_method;
> >      > +     enum ofono_gprs_proto proto;
> >      > +
> >      > +     if (lte->driver->set_default_attach_info == NULL)
> >      > +             return __ofono_error_not_implemented(msg);
> >      > +
> >      > +     if (lte->pending)
> >      > +             return __ofono_error_busy(msg);
> >      >
> >      >       if (!dbus_message_iter_init(msg, &iter))
> >      >               return __ofono_error_invalid_args(msg);
> >      > @@ -192,13 +428,58 @@ static DBusMessage
> >     *lte_set_property(DBusConnection *conn,
> >      >
> >      >       dbus_message_iter_recurse(&iter, &var);
> >      >
> >      > -     if (!strcmp(property, DEFAULT_APN_KEY)) {
> >      > +     lte->prop_changed=property;
> >      > +
> >      > +     if (!strcmp(property, LTE_PROTO)) {
> >      > +
> >      > +             if (dbus_message_iter_get_arg_type(&var) !=
> >     DBUS_TYPE_STRING)
> >      > +                     return __ofono_error_invalid_args(msg);
> >      > +
> >      > +             dbus_message_iter_get_basic(&var, &str);
> >      > +
> >      > +             if (gprs_proto_from_string(str, &proto))
> >      > +                     return lte_set_proto(lte, conn, msg, proto);
> >
> >     The return from this callback is always supposed to be a
> method_return
> >     or NULL if the method_return will be done asynchronously (in your
> case
> >     always)  You're somehow returning the method_call itself...
> >
> >
> > I am not sure I follow you, but you suggested to restructure the code,
> > so I will come back on this later.
>
> You need to understand the semantics of GDBus.  GDBUS_ASYNC_METHOD()
> callback can only return the D-Bus method_return for message or NULL if
> the method return will be generated asynchronously.  You cannot return
> the message itself.  While this may 'work' the behavior is undefined.
>
> >      > +struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte
> *lte)
> >      > +{
> >      > +     return __ofono_atom_get_modem(lte->atom);
> >      > +}
> >      > \ No newline at end of file
> >
> >     Fix this.
> >
> >
> > ?
>
> You have no newline after the last '}' and git complains.  If git
> complains I cannot apply the patch...
>
> >
> > I agree it is unrelated, I will post a separate post, but I turn on
> > NEED_THREADS, and the build fails. Looking at the g_thread
> > documentations, nowadays it has to come out of the code:
> >
> > |g_thread_init| has been deprecated since version 2.32 and should not be
> > used in newly-written code.
> >
> > This function is no longer necessary. The GLib threading system is
> > automatically initialized at the start of your program.
> >
>
> Okay, please send this as a separate patch.  We might need to remove
> this configure option as well.
>
> Regards,
> -Denis
>

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

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Denis Kenzior]

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

Hi Giacinto,

> I had a look at this all, and I have a problem. I cannot check the 
> parameters as they are entered one by one.
> Example: if I blank user/pwd when the authentication is changed to NONE, 
> then if changed again to CHAP, the module will reject it.
> Shall I store the parameters, or keep them also in case of error?

So in the case of ConnectionContext the parameters are not sent out to 
the driver until context activation is attempted.  Hence all the 
settings are immediate and the activation fails or it succeeds.

However, in the LongTermEvolution driver setup the settings are 
immediate.  Thus the D-Bus API you propose is thus not really suitable 
and needs to be modified. Since we're kind of stuck with the 
'DefaultAccessPointName' property at this point, the only two ways out 
of this I can think of are:

- Have the driver handle this.  So if PAP/CHAP is selected but username 
or password are invalid, default to no authentication.  The assumption 
will be that eventually valid parameters are given.

Or perhaps we only call out into the driver method once the parameters 
in their entirety are valid, accepting whatever the user puts in as long 
as the individual property input is valid according to core validity checks.

....

> 
> Another way would be to have a SetParameters() function, and set them 
> all together, including the APN, and not allowing writing them 
> separately, apart from the APN which already exists.
> I don't really like it, either.
> 

As you point out, this is the second alternative.  AuthenticationMethod, 
Username & Password would need to be set via a method call and 
optionally exposed as [readonly] properties.  Protocol could still be 
handled as per DefaultAccessPointName or inside the aforementioned 
method call.

> Or introduce an atom function that is called before modem->set_online(true)?
> 

This might be trickier, but could also work...

Regards,
-Denis

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Giacinto Cifelli]

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

Hi Denis,

On Wed, Sep 19, 2018 at 8:24 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> > I had a look at this all, and I have a problem. I cannot check the
> > parameters as they are entered one by one.
> > Example: if I blank user/pwd when the authentication is changed to NONE,
> > then if changed again to CHAP, the module will reject it.
> > Shall I store the parameters, or keep them also in case of error?
>
> So in the case of ConnectionContext the parameters are not sent out to
> the driver until context activation is attempted.  Hence all the
> settings are immediate and the activation fails or it succeeds.
>
> However, in the LongTermEvolution driver setup the settings are
> immediate.  Thus the D-Bus API you propose is thus not really suitable
> and needs to be modified. Since we're kind of stuck with the
> 'DefaultAccessPointName' property at this point, the only two ways out
> of this I can think of are:
>
> - Have the driver handle this.  So if PAP/CHAP is selected but username
> or password are invalid, default to no authentication.  The assumption
> will be that eventually valid parameters are given.
>
> Or perhaps we only call out into the driver method once the parameters
> in their entirety are valid, accepting whatever the user puts in as long
> as the individual property input is valid according to core validity
> checks.
>

We cannot change the external API, but internally we are free to rearrange
the code.

how do you feel about the proposal below for a second set of functions, to
override the existing ones
for newer lte atoms?

in src/lte.c

static DBusMessage *lte_set_property(DBusConnection *conn,
DBusMessage *msg, void *data)
{
[...]
if (lte->driver->set_property)
lte->driver->set_property(...);
else if (lte->driver->set_default_attach_info)
lte->driver->set_default_attach_info(...);
else
return __ofono_error_not_implemented(msg);
[...]

in src/modem.c:

static void sim_state_watch(enum ofono_sim_state new_state, void *user)
{
[...]
case OFONO_SIM_STATE_READY:
modem_change_state(modem, MODEM_STATE_OFFLINE);

ofono_lte_set_reginfo(modem);

/* Modem is always online, proceed to online state. */
if (modem_is_always_online(modem) == TRUE)
set_online(modem, TRUE);

if (modem->online == TRUE)
modem_change_state(modem, MODEM_STATE_ONLINE);
else if (modem->get_online)
modem->driver->set_online(modem, 1, common_online_cb,
modem);

modem->get_online = FALSE;

break;
}

I see that if(modem_is_always_online) maybe it will come too late, but in
that case also the current solution
with set_default_attach_info suffers from the same limitation, so no
regression.

Of course then, again in src/lte.c, the function ofono_lte_set_reginfo will
look for an lte atom, if present will call it without parameter for sending
the AT+CGDCONT and AT+CGAUTH to the modem.

Would this work for you?



> ....
>
> >
> > Another way would be to have a SetParameters() function, and set them
> > all together, including the APN, and not allowing writing them
> > separately, apart from the APN which already exists.
> > I don't really like it, either.
> >
>
> As you point out, this is the second alternative.  AuthenticationMethod,
> Username & Password would need to be set via a method call and
> optionally exposed as [readonly] properties.  Protocol could still be
> handled as per DefaultAccessPointName or inside the aforementioned
> method call.
>
> > Or introduce an atom function that is called before
> modem->set_online(true)?
> >
>
> This might be trickier, but could also work...
>
> Regards,
> -Denis
>

Regards,
Giacinto

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

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Denis Kenzior]

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

Hi Giacinto,

> static DBusMessage *lte_set_property(DBusConnection *conn,
> DBusMessage *msg, void *data)
> {
> [...]
> if (lte->driver->set_property)
> lte->driver->set_property(...);

What is this buying you?

> else if (lte->driver->set_default_attach_info)
> lte->driver->set_default_attach_info(...);
> else
> return __ofono_error_not_implemented(msg);
> [...]
> 
> in src/modem.c:
> 
> static void sim_state_watch(enum ofono_sim_state new_state, void *user)
> {
> [...]
> case OFONO_SIM_STATE_READY:
> modem_change_state(modem, MODEM_STATE_OFFLINE);
> 
> ofono_lte_set_reginfo(modem);

How does this help you?  You might be in the OFFLINE state for a while 
and the LTE settings might still be messed with in that time.

> 
> /* Modem is always online, proceed to online state. */
> if (modem_is_always_online(modem) == TRUE)
> set_online(modem, TRUE);
> 
> if (modem->online == TRUE)
> modem_change_state(modem, MODEM_STATE_ONLINE);
> else if (modem->get_online)
> modem->driver->set_online(modem, 1, common_online_cb,
> modem);
> 
> modem->get_online = FALSE;
> 
> break;
> }
> 
> I see that if(modem_is_always_online) maybe it will come too late, but 
> in that case also the current solution
> with set_default_attach_info suffers from the same limitation, so no 
> regression.

The modem_is_always_online case is a bit strange, it is for really 
primitive devices which likely won't have an LTE atom anyway.

> 
> Of course then, again in src/lte.c, the function ofono_lte_set_reginfo 
> will look for an lte atom, if present will call it without parameter for 
> sending the AT+CGDCONT and AT+CGAUTH to the modem.
> 
> Would this work for you?
> 

I don't see how this helps.

Regards,
-Denis

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Giacinto Cifelli]

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

Hi Denis,

sorry for writing on top, but I don't see a good place in the middle.

The idea would be to just store the values in the driver but not in the
module,
and then send them before attaching, so that the verification can be
consistent and all,
just like for the gprs-context atom.

so the idea would be to check if a new method, that I have tentatively
called set_property(),
exists. If so, use the reviewed stile above, otherwise fallback to the
current function.

Giacinto

On Thu, Sep 20, 2018 at 6:03 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> > static DBusMessage *lte_set_property(DBusConnection *conn,
> > DBusMessage *msg, void *data)
> > {
> > [...]
> > if (lte->driver->set_property)
> > lte->driver->set_property(...);
>

> What is this buying you?
>
> > else if (lte->driver->set_default_attach_info)
> > lte->driver->set_default_attach_info(...);
> > else
> > return __ofono_error_not_implemented(msg);
> > [...]
> >
> > in src/modem.c:
> >
> > static void sim_state_watch(enum ofono_sim_state new_state, void *user)
> > {
> > [...]
> > case OFONO_SIM_STATE_READY:
> > modem_change_state(modem, MODEM_STATE_OFFLINE);
> >
> > ofono_lte_set_reginfo(modem);
>
> How does this help you?  You might be in the OFFLINE state for a while
> and the LTE settings might still be messed with in that time.
>
> >
> > /* Modem is always online, proceed to online state. */
> > if (modem_is_always_online(modem) == TRUE)
> > set_online(modem, TRUE);
> >
> > if (modem->online == TRUE)
> > modem_change_state(modem, MODEM_STATE_ONLINE);
> > else if (modem->get_online)
> > modem->driver->set_online(modem, 1, common_online_cb,
> > modem);
> >
> > modem->get_online = FALSE;
> >
> > break;
> > }
> >
> > I see that if(modem_is_always_online) maybe it will come too late, but
> > in that case also the current solution
> > with set_default_attach_info suffers from the same limitation, so no
> > regression.
>
> The modem_is_always_online case is a bit strange, it is for really
> primitive devices which likely won't have an LTE atom anyway.
>
> >
> > Of course then, again in src/lte.c, the function ofono_lte_set_reginfo
> > will look for an lte atom, if present will call it without parameter for
> > sending the AT+CGDCONT and AT+CGAUTH to the modem.
> >
> > Would this work for you?
> >
>
> I don't see how this helps.
>
> Regards,
> -Denis
>

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

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Denis Kenzior]

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

Hi Giacinto,

> The idea would be to just store the values in the driver but not in the 
> module,
> and then send them before attaching, so that the verification can be 
> consistent and all,
> just like for the gprs-context atom.

Yes, but that is not what you outlined in the code.  You were watching 
for SIM_STATE_READY which only means just that.

Also note that we can bring the modem online even in pre_sim state (e.g. 
for emergency calls), in which case the lte atom doesn't exist yet.

> 
> so the idea would be to check if a new method, that I have tentatively 
> called set_property(),
> exists. If so, use the reviewed stile above, otherwise fallback to the 
> current function.
> 

I don't see what set_property is buying you.  Anyway, feel free to come 
up with a more detailed proposal, but my impression is that this 
approach will quickly become way too complicated.

Regards,
-Denis

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Giacinto Cifelli]

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

hi Denis,

On Thu, 20 Sep 2018, 18:31 Denis Kenzior, <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> > The idea would be to just store the values in the driver but not in the
> > module,
> > and then send them before attaching, so that the verification can be
> > consistent and all,
> > just like for the gprs-context atom.
>
> Yes, but that is not what you outlined in the code.  You were watching
> for SIM_STATE_READY which only means just that.
>
> Also note that we can bring the modem online even in pre_sim state (e.g.
> for emergency calls), in which case the lte atom doesn't exist yet.
>

where should a hook be placed then?
this looked to me a good place, just before bringing it online.
if there is no atom, no config and everything is ok anyway.

>
> > so the idea would be to check if a new method, that I have tentatively
> > called set_property(),
> > exists. If so, use the reviewed stile above, otherwise fallback to the
> > current function.
> >
>
> I don't see what set_property is buying you.  Anyway, feel free to come
> up with a more detailed proposal, but my impression is that this
> approach will quickly become way too complicated.
>

I think now i got you. The existing function could be used, but the driver
only stores the values internally because it knows it will ve called later
for sending the commands.


> Regards,
> -Denis
>

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

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Denis Kenzior]

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

Hi Giacinto,

On 09/20/2018 12:03 PM, Giacinto Cifelli wrote:
> hi Denis,
> 
> On Thu, 20 Sep 2018, 18:31 Denis Kenzior, <denkenz@gmail.com 
> <mailto:denkenz@gmail.com>> wrote:
> 
>     Hi Giacinto,
> 
>      > The idea would be to just store the values in the driver but not
>     in the
>      > module,
>      > and then send them before attaching, so that the verification can be
>      > consistent and all,
>      > just like for the gprs-context atom.
> 
>     Yes, but that is not what you outlined in the code.  You were watching
>     for SIM_STATE_READY which only means just that.
> 
>     Also note that we can bring the modem online even in pre_sim state
>     (e.g.
>     for emergency calls), in which case the lte atom doesn't exist yet.
> 
> 
> where should a hook be placed then?
> this looked to me a good place, just before bringing it online.
> if there is no atom, no config and everything is ok anyway.

That's kind of the problem, there is no good place.  The only thing you 
can do is have the core apply settings, if available, just prior to the 
set_online driver call.  However, it is tricky to do so:

- We're in pre-sim state and go online for emergency calls.  This is 
okay as no LTE atom is created.  If a SIM is initialized / inserted at 
this point, the modem can re-connect with a proper default settings. 
The LTE atom creation and modem performing a re-connect is now a race. 
Admittedly the solution we have today also suffers from this problem, so 
maybe there's not much we can do here.

- We're offline and going online.  You need to perform the online 
operation as a transaction.  First setting the default attach details 
and then actually setting the modem online.  We can't do it 
simultaneously, as again we cannot assume the multiplexing / queueing 
behavior of the driver.  This could perhaps be made to work, but there 
may be additional issues that are discovered as we go along.  Only way 
to find out is to prototype it.

> 
>      >
>      > so the idea would be to check if a new method, that I have
>     tentatively
>      > called set_property(),
>      > exists. If so, use the reviewed stile above, otherwise fallback
>     to the
>      > current function.
>      >
> 
>     I don't see what set_property is buying you.  Anyway, feel free to come
>     up with a more detailed proposal, but my impression is that this
>     approach will quickly become way too complicated.
> 
> 
> I think now i got you. The existing function could be used, but the 
> driver only stores the values internally because it knows it will ve 
> called later for sending the commands.
> 

Or we simply cache the values in the lte atom and call the driver method 
only when appropriate.

Regards,
-Denis

Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Giacinto Cifelli]

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

On Thu, Sep 20, 2018 at 7:18 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> On 09/20/2018 12:03 PM, Giacinto Cifelli wrote:
> > hi Denis,
> >
> > On Thu, 20 Sep 2018, 18:31 Denis Kenzior, <denkenz@gmail.com
> > <mailto:denkenz@gmail.com>> wrote:
> >
> >     Hi Giacinto,
> >
> >      > The idea would be to just store the values in the driver but not
> >     in the
> >      > module,
> >      > and then send them before attaching, so that the verification can
> be
> >      > consistent and all,
> >      > just like for the gprs-context atom.
> >
> >     Yes, but that is not what you outlined in the code.  You were
> watching
> >     for SIM_STATE_READY which only means just that.
> >
> >     Also note that we can bring the modem online even in pre_sim state
> >     (e.g.
> >     for emergency calls), in which case the lte atom doesn't exist yet.
> >
> >
> > where should a hook be placed then?
> > this looked to me a good place, just before bringing it online.
> > if there is no atom, no config and everything is ok anyway.
>
> That's kind of the problem, there is no good place.  The only thing you
> can do is have the core apply settings, if available, just prior to the
> set_online driver call.  However, it is tricky to do so:
>
> - We're in pre-sim state and go online for emergency calls.  This is
> okay as no LTE atom is created.  If a SIM is initialized / inserted at
> this point, the modem can re-connect with a proper default settings.
> The LTE atom creation and modem performing a re-connect is now a race.
> Admittedly the solution we have today also suffers from this problem, so
> maybe there's not much we can do here.
>
> - We're offline and going online.  You need to perform the online
> operation as a transaction.  First setting the default attach details
> and then actually setting the modem online.  We can't do it
> simultaneously, as again we cannot assume the multiplexing / queueing
> behavior of the driver.  This could perhaps be made to work, but there
> may be additional issues that are discovered as we go along.  Only way
> to find out is to prototype it.
>

ok, the AT driver is piping the commands, and I am going to implement it
for it.
Admittedly, for another driver could be more difficult (MBIM also pipes
them),
but there is still the possibility to set directly the parameters as they
are today,
not implementing this function in the atom (NULL function or simply
returning).


> >
> >      >
> >      > so the idea would be to check if a new method, that I have
> >     tentatively
> >      > called set_property(),
> >      > exists. If so, use the reviewed stile above, otherwise fallback
> >     to the
> >      > current function.
> >      >
> >
> >     I don't see what set_property is buying you.  Anyway, feel free to
> come
> >     up with a more detailed proposal, but my impression is that this
> >     approach will quickly become way too complicated.
> >
> >
> > I think now i got you. The existing function could be used, but the
> > driver only stores the values internally because it knows it will ve
> > called later for sending the commands.
> >
>
> Or we simply cache the values in the lte atom and call the driver method
> only when appropriate.
>

I believe it is better to call the atom and let it decide what to do, in
part to mitigate the issues above.


> Regards,
> -Denis
>

with this, I am good to go, I will try to prototype this new behavior.

Regards.
Giacinto

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

[PATCH 3/7] extended support for LTE and NR. Some minor fixes. part 3 of 7

[Giacinto Cifelli]

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

---
 drivers/atmodem/atutil.h                 |  14 +
 drivers/atmodem/cbs.c                    |   1 +
 drivers/atmodem/gprs-context.c           |   2 +
 drivers/atmodem/gprs.c                   | 554 ++++++++++++++++--
 drivers/atmodem/lte.c                    | 283 ++++++++-
 drivers/atmodem/network-registration.c   | 134 +++--
 drivers/atmodem/sim.c                    |  10 +-
 drivers/atmodem/sms.c                    |  21 +-
 drivers/atmodem/vendor.h                 |   1 +
 drivers/gemaltomodem/gemaltomodem.c      |   5 +-
 drivers/gemaltomodem/gemaltomodem.h      |  19 +-
 drivers/gemaltomodem/gprs-context-wwan.c | 445 ++++++++++++++
 drivers/gemaltomodem/voicecall.c         | 965 +++++++++++++++++++++++++++++++
 drivers/mbimmodem/gprs-context.c         |   2 +
 drivers/mbimmodem/mbim.c                 |   3 +
 drivers/mbimmodem/mbimmodem.c            |   2 +-
 drivers/rilmodem/call-forwarding.c       |   2 +-
 drivers/rilmodem/network-registration.c  |   2 +-
 18 files changed, 2343 insertions(+), 122 deletions(-)
 create mode 100644 drivers/gemaltomodem/gprs-context-wwan.c
 create mode 100644 drivers/gemaltomodem/voicecall.c

diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
index 7113a4c..57573cb 100644
--- a/drivers/atmodem/atutil.h
+++ b/drivers/atmodem/atutil.h
@@ -115,6 +115,20 @@ static inline int at_util_convert_signal_strength(int strength)
 	return result;
 }
 
+static inline int at_util_convert_signal_strength_cesq(int strength_GSM, int strength_UTRAN, int strength_EUTRAN)
+{
+	int result = -1;
+
+	if (strength_GSM != 99)
+		result = (strength_GSM * 100) / 63;
+	else if (strength_UTRAN != 255)
+		result = (strength_UTRAN * 100) / 96;
+	else if (strength_EUTRAN != 255)
+		result = (strength_EUTRAN * 100) / 97;
+
+	return result;
+}
+
 #define CALLBACK_WITH_FAILURE(cb, args...)		\
 	do {						\
 		struct ofono_error cb_e;		\
diff --git a/drivers/atmodem/cbs.c b/drivers/atmodem/cbs.c
index 0a76ba2..1824b47 100644
--- a/drivers/atmodem/cbs.c
+++ b/drivers/atmodem/cbs.c
@@ -166,6 +166,7 @@ static void at_cbs_set_topics(struct ofono_cbs *cbs, const char *topics,
 	switch (data->vendor) {
 	case OFONO_VENDOR_GOBI:
 	case OFONO_VENDOR_QUALCOMM_MSM:
+	case OFONO_VENDOR_GEMALTO:
 		g_at_chat_send(data->chat, "AT+CSCB=0", none_prefix,
 				NULL, NULL, NULL);
 		break;
diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index 79ac4c8..0585850 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -304,6 +304,8 @@ static void at_gprs_activate_primary(struct ofono_gprs_context *gc,
 				snprintf(buf + len, sizeof(buf) - len - 3,
 						",\"PAP:%s\"", ctx->apn);
 				break;
+			default:
+				break;
 			}
 			break;
 		default:
diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index df37d05..6aa2cf2 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -4,6 +4,7 @@
  *
  *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
  *  Copyright (C) 2010  ST-Ericsson AB.
+ *  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
@@ -35,6 +36,7 @@
 #include <ofono/log.h>
 #include <ofono/modem.h>
 #include <ofono/gprs.h>
+#include <common.h>
 
 #include "gatchat.h"
 #include "gatresult.h"
@@ -43,6 +45,8 @@
 #include "vendor.h"
 
 static const char *cgreg_prefix[] = { "+CGREG:", NULL };
+static const char *cereg_prefix[] = { "+CEREG:", NULL };
+static const char *c5greg_prefix[] = { "+C5GREG:", NULL };
 static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
 static const char *none_prefix[] = { NULL };
 
@@ -51,7 +55,23 @@ struct gprs_data {
 	unsigned int vendor;
 	unsigned int last_auto_context_id;
 	gboolean telit_try_reattach;
+	gboolean has_cgreg;
+	gboolean has_cereg;
+	gboolean has_c5greg;
+	gboolean nb_inds;
+	gboolean auto_attach; /* for LTE modems & co */
 	int attached;
+	int cgreg_status;
+	int cereg_status;
+	int c5greg_status;
+};
+
+struct netreg_info {
+	struct ofono_gprs *gprs;
+	struct gprs_data *gd;
+	const char *ind;
+	int status;
+	int bearer;
 };
 
 static void at_cgatt_cb(gboolean ok, GAtResult *result, gpointer user_data)
@@ -69,9 +89,15 @@ static void at_gprs_set_attached(struct ofono_gprs *gprs, int attached,
 					ofono_gprs_cb_t cb, void *data)
 {
 	struct gprs_data *gd = ofono_gprs_get_data(gprs);
-	struct cb_data *cbd = cb_data_new(cb, data);
+	struct cb_data *cbd;
 	char buf[64];
 
+	if(gd->auto_attach) {
+		CALLBACK_WITH_SUCCESS(cb, data);
+		return;
+	}
+
+	cbd = cb_data_new(cb, data);
 	snprintf(buf, sizeof(buf), "AT+CGATT=%i", attached ? 1 : 0);
 
 	if (g_at_chat_send(gd->chat, buf, none_prefix,
@@ -92,21 +118,101 @@ static void at_cgreg_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	struct ofono_error error;
 	int status;
 	struct gprs_data *gd = cbd->user;
+	gboolean last = !(gd->has_cereg || gd->has_c5greg);
 
 	decode_at_error(&error, g_at_result_final_response(result));
 
 	if (!ok) {
-		cb(&error, -1, cbd->data);
-		return;
+		status = -1;
+		goto end;
 	}
 
 	if (at_util_parse_reg(result, "+CGREG:", NULL, &status,
 				NULL, NULL, NULL, gd->vendor) == FALSE) {
-		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-		return;
+		error.type = OFONO_ERROR_TYPE_FAILURE;
+		error.error = 0;
+		status = -1;
+		goto end;
+	}
+
+end:
+	gd->cgreg_status = status;
+
+	if(last)
+		cb(&error, status, cbd->data);
+}
+
+static void at_cereg_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_gprs_status_cb_t cb = cbd->cb;
+	struct ofono_error error;
+	int status;
+	struct gprs_data *gd = cbd->user;
+	gboolean last = !gd->has_c5greg;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		status = -1;
+		goto end;
+	}
+
+	if (at_util_parse_reg(result, "+CEREG:", NULL, &status,
+				NULL, NULL, NULL, gd->vendor) == FALSE) {
+		error.type = OFONO_ERROR_TYPE_FAILURE;
+		error.error = 0;
+		status = -1;
+		goto end;
+	}
+
+end:
+	gd->cereg_status = status;
+
+	if(last) {
+
+		if (gd->cgreg_status==NETWORK_REGISTRATION_STATUS_DENIED ||
+			gd->cgreg_status==NETWORK_REGISTRATION_STATUS_REGISTERED)
+			cb(&error, gd->cgreg_status, cbd->data);
+		else
+			cb(&error, status, cbd->data);
+	}
+}
+
+static void at_c5greg_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_gprs_status_cb_t cb = cbd->cb;
+	struct ofono_error error;
+	int status;
+	struct gprs_data *gd = cbd->user;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		status = -1;
+		goto end;
+	}
+
+	if (at_util_parse_reg(result, "+C5GREG:", NULL, &status,
+				NULL, NULL, NULL, gd->vendor) == FALSE) {
+		error.type = OFONO_ERROR_TYPE_FAILURE;
+		error.error = 0;
+		status = -1;
+		goto end;
 	}
 
-	cb(&error, status, cbd->data);
+end:
+	gd->c5greg_status = status;
+
+	if (gd->cgreg_status==NETWORK_REGISTRATION_STATUS_DENIED ||
+		gd->cgreg_status==NETWORK_REGISTRATION_STATUS_REGISTERED)
+		cb(&error, gd->cgreg_status, cbd->data);
+	else if (gd->cereg_status==NETWORK_REGISTRATION_STATUS_DENIED ||
+		gd->cereg_status==NETWORK_REGISTRATION_STATUS_REGISTERED)
+		cb(&error, gd->cereg_status, cbd->data);
+	else
+		cb(&error, status, cbd->data);
 }
 
 static void at_gprs_registration_status(struct ofono_gprs *gprs,
@@ -114,9 +220,6 @@ static void at_gprs_registration_status(struct ofono_gprs *gprs,
 					void *data)
 {
 	struct gprs_data *gd = ofono_gprs_get_data(gprs);
-	struct cb_data *cbd = cb_data_new(cb, data);
-
-	cbd->user = gd;
 
 	switch (gd->vendor) {
 	case OFONO_VENDOR_GOBI:
@@ -137,13 +240,51 @@ static void at_gprs_registration_status(struct ofono_gprs *gprs,
 		break;
 	}
 
-	if (g_at_chat_send(gd->chat, "AT+CGREG?", cgreg_prefix,
-				at_cgreg_cb, cbd, g_free) > 0)
-		return;
+	/*
+	 * this is long: send all indicators, compare at the end if one reports
+	 * attached and use it, otherwise report status for last indicator
+	 * tested (higher technology).
+	 * Note: AT+CGATT? is not good because doesn't tell us if we are roaming
+	 */
+	if(gd->has_cgreg) {
+		struct cb_data *cbd = cb_data_new(cb, data);
+		cbd->user = gd;
+		gd->cgreg_status=-1; /* preset in case of fail of the at send */
+
+		/* g_at_chat_send fails only if g_new_try fails, so we stop */
+		if (g_at_chat_send(gd->chat, "AT+CGREG?", cgreg_prefix,
+					at_cgreg_cb, cbd, g_free) == 0) {
+			g_free(cbd);
+			CALLBACK_WITH_FAILURE(cb, -1, data);
+			return;
+		}
+	}
 
-	g_free(cbd);
+	if(gd->has_cereg) {
+		struct cb_data *cbd = cb_data_new(cb, data);
+		cbd->user = gd;
+		gd->cereg_status=-1;
+
+		if (g_at_chat_send(gd->chat, "AT+CEREG?", cereg_prefix,
+					at_cereg_cb, cbd, g_free) == 0) {
+			g_free(cbd);
+			CALLBACK_WITH_FAILURE(cb, -1, data);
+			return;
+		}
+	}
+
+	if(gd->has_c5greg) {
+		struct cb_data *cbd = cb_data_new(cb, data);
+		cbd->user = gd;
+		gd->c5greg_status=-1;
 
-	CALLBACK_WITH_FAILURE(cb, -1, data);
+		if (g_at_chat_send(gd->chat, "AT+C5GREG?", c5greg_prefix,
+					at_c5greg_cb, cbd, g_free) == 0) {
+			g_free(cbd);
+			CALLBACK_WITH_FAILURE(cb, -1, data);
+			return;
+		}
+	}
 }
 
 static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
@@ -188,14 +329,100 @@ static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
 				activated_cid);
 }
 
+static int cops_cb(gboolean ok, GAtResult *result)
+{
+	GAtResultIter iter;
+	int format, tech=-1;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+COPS:"))
+		goto error;
+
+	g_at_result_iter_skip_next(&iter); /* mode: automatic, manual, ... */
+
+	if (!g_at_result_iter_next_number(&iter, &format))
+		goto error;
+
+	g_at_result_iter_skip_next(&iter); /* operator name or code */
+
+	if (!g_at_result_iter_next_number(&iter, &tech))
+		tech = -1; /* make sure it has not been set to something */
+error:
+	return tech;
+}
+
+
+static void netreg_notify_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct netreg_info *nri = user_data;
+	int cops_tech = cops_cb(ok, result);
+
+	if(cops_tech==-1) { /* take the indicator status */
+		ofono_gprs_status_notify(nri->gprs, nri->status);
+		return;
+	}
+
+	/*
+	 * values taken from the 3GPP 27.007 rel.15
+	 * matching enum access_technology in common.h up to 7.
+	 */
+	if (g_str_equal(nri->ind,"CGREG") && (cops_tech<7 || cops_tech==8))
+		ofono_gprs_status_notify(nri->gprs, nri->status);
+	else if (g_str_equal(nri->ind,"CEREG") && (cops_tech==7 ||
+						cops_tech==9 || cops_tech==12))
+		ofono_gprs_status_notify(nri->gprs, nri->status);
+	else if (g_str_equal(nri->ind,"C5GREG") && (cops_tech==10 ||
+						cops_tech==11 || cops_tech==13))
+		ofono_gprs_status_notify(nri->gprs, nri->status);
+	/* all other cases ignored: indicator not for current AcT */
+}
+
+static void netreg_notify(struct ofono_gprs *gprs, const char* ind, int status,
+								int bearer)
+{
+	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+	struct netreg_info *nri;
+
+	if (status==NETWORK_REGISTRATION_STATUS_DENIED ||
+			status==NETWORK_REGISTRATION_STATUS_REGISTERED ||
+			status==NETWORK_REGISTRATION_STATUS_ROAMING ||
+			gd->nb_inds==1) {
+		/* accept this status and process */
+		ofono_gprs_status_notify(gprs, status);
+
+		if(bearer != -1)
+			ofono_gprs_bearer_notify(gprs, bearer);
+
+		return;
+	}
+
+	/*
+	 * in this case nb_inds>1 && status not listed above
+	 * we check AT+COPS? for a second opinion.
+	 */
+
+	nri = g_new0(struct netreg_info, 1);
+	nri->gprs = gprs;
+	nri->gd = gd;
+	nri->ind = ind;
+	nri->status = status;
+	nri->bearer = bearer;
+	g_at_chat_send(gd->chat, "AT+COPS?", none_prefix, netreg_notify_cb,
+		nri, g_free);
+}
+
 static void cgreg_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
-	int status;
 	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+	int status, bearer;
 
-	if (at_util_parse_reg_unsolicited(result, "+CGREG:", &status,
-				NULL, NULL, NULL, gd->vendor) == FALSE)
+	if (!at_util_parse_reg_unsolicited(result, "+CGREG:", &status,
+				NULL, NULL, &bearer, gd->vendor) == FALSE)
 		return;
 
 	/*
@@ -220,7 +447,33 @@ static void cgreg_notify(GAtResult *result, gpointer user_data)
 		gd->telit_try_reattach = FALSE;
 	}
 
-	ofono_gprs_status_notify(gprs, status);
+	netreg_notify(gprs, "CGREG", status, bearer);
+}
+
+static void cereg_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+	int status, bearer;
+
+	if (!at_util_parse_reg_unsolicited(result, "+CEREG:", &status,
+				NULL, NULL, &bearer, gd->vendor) == FALSE)
+		return;
+
+	netreg_notify(gprs, "CEREG", status, bearer);
+}
+
+static void c5greg_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+	int status, bearer;
+
+	if (!at_util_parse_reg_unsolicited(result, "+C5GREG:", &status,
+				NULL, NULL, &bearer, gd->vendor) == FALSE)
+		return;
+
+	netreg_notify(gprs, "C5GREG", status, bearer);
 }
 
 static void cgev_notify(GAtResult *result, gpointer user_data)
@@ -419,6 +672,91 @@ static void ublox_ureg_notify(GAtResult *result, gpointer user_data)
 	ofono_gprs_bearer_notify(gprs, bearer);
 }
 
+static void gemalto_ciev_ceer_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	const char *report;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CIEV: ceer,"))
+		return;
+	/*
+	 * No need to check release cause group
+	 * as we only subscribe to no. 5
+	 */
+	if (!g_at_result_iter_skip_next(&iter))
+		return;
+	if (!g_at_result_iter_next_string(&iter, &report))
+		return;
+
+	/* TODO: Handle more of these? */
+
+	if (g_str_equal(report, "Regular deactivation")) {
+		ofono_gprs_detached_notify(gprs);
+		return;
+	}
+}
+
+static void gemalto_ciev_bearer_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	int bearer;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CIEV: psinfo,"))
+		return;
+	if (!g_at_result_iter_next_number(&iter, &bearer))
+		return;
+
+	/* Go from Gemalto representation to oFono representation */
+	switch (bearer) {
+	case 0: /* GPRS/EGPRS not available */
+		/* Same as "no bearer"? */
+		bearer = 0;
+		break;
+	case 1: /* GPRS available, ignore this one */
+		return;
+	case 2: /* GPRS attached */
+		bearer = 1;
+		break;
+	case 3: /* EGPRS available, ignore this one */
+		return;
+	case 4: /* EGPRS attached */
+		bearer = 2;
+		break;
+	case 5: /* UMTS available, ignore this one */
+		return;
+	case 6: /* UMTS attached */
+		bearer = 3;
+		break;
+	case 7: /* HSDPA available, ignore this one */
+		return;
+	case 8: /* HSDPA attached */
+		bearer = 5;
+		break;
+	case 9: /* HSDPA/HSUPA available, ignore this one */
+		return;
+	case 10: /* HSDPA/HSUPA attached */
+		bearer = 6;
+		break;
+	/* TODO: Limit these cases to ALS3? */
+	case 16: /* E-UTRA available, ignore this one */
+		return;
+	case 17: /* E-UTRA attached */
+		bearer = 7;
+		break;
+	default: /* Assume that non-parsable values mean "no bearer" */
+		bearer = 0;
+		break;
+	}
+
+	ofono_gprs_bearer_notify(gprs, bearer);
+}
+
 static void cpsb_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
@@ -439,14 +777,48 @@ static void cpsb_notify(GAtResult *result, gpointer user_data)
 	ofono_gprs_bearer_notify(gprs, bearer);
 }
 
-static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
+static void gprs_initialized(struct ofono_gprs *gprs)
 {
-	struct ofono_gprs *gprs = user_data;
 	struct gprs_data *gd = ofono_gprs_get_data(gprs);
 
+	switch (gd->vendor) {
+	case OFONO_VENDOR_GEMALTO:
+		break;
+	default:
+		g_at_chat_send(gd->chat, "AT+CGAUTO=0", none_prefix, NULL, NULL,
+									NULL);
+	}
+
+	switch (gd->vendor) {
+	case OFONO_VENDOR_MBM:
+		/* Ericsson MBM and ST-E modems don't support AT+CGEREP=2,1 */
+		g_at_chat_send(gd->chat, "AT+CGEREP=1,0", none_prefix,
+			NULL, NULL, NULL);
+		break;
+	case OFONO_VENDOR_NOKIA:
+		/* Nokia data cards don't support AT+CGEREP=1,0 either */
+		g_at_chat_send(gd->chat, "AT+CGEREP=1", none_prefix,
+			NULL, NULL, NULL);
+		break;
+	case OFONO_VENDOR_GEMALTO:
+		g_at_chat_send(gd->chat, "AT+CGEREP=2", NULL,
+					NULL, NULL, NULL);
+		g_at_chat_send(gd->chat, "AT^SIND=\"psinfo\",1", none_prefix,
+			NULL, NULL, NULL);
+		break;
+	default:
+		g_at_chat_send(gd->chat, "AT+CGEREP=2,1", none_prefix,
+			NULL, NULL, NULL);
+		break;
+	}
+
 	g_at_chat_register(gd->chat, "+CGEV:", cgev_notify, FALSE, gprs, NULL);
-	g_at_chat_register(gd->chat, "+CGREG:", cgreg_notify,
-						FALSE, gprs, NULL);
+	g_at_chat_register(gd->chat, "+CGREG:", cgreg_notify, FALSE, gprs,
+									NULL);
+	g_at_chat_register(gd->chat, "+CEREG:", cereg_notify, FALSE, gprs,
+									NULL);
+	g_at_chat_register(gd->chat, "+C5GREG:", c5greg_notify, FALSE, gprs,
+									NULL);
 
 	switch (gd->vendor) {
 	case OFONO_VENDOR_HUAWEI:
@@ -468,6 +840,12 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
 		g_at_chat_send(gd->chat, "AT#PSNT=1", none_prefix,
 						NULL, NULL, NULL);
 		break;
+	case OFONO_VENDOR_GEMALTO:
+		g_at_chat_register(gd->chat, "+CIEV: psinfo,",
+			gemalto_ciev_bearer_notify, FALSE, gprs, NULL);
+		g_at_chat_register(gd->chat, "+CIEV: ceer,",
+			gemalto_ciev_ceer_notify, FALSE, gprs, NULL);
+		break;
 	default:
 		g_at_chat_register(gd->chat, "+CPSB:", cpsb_notify,
 						FALSE, gprs, NULL);
@@ -489,16 +867,33 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
 	ofono_gprs_register(gprs);
 }
 
-static void at_cgreg_test_cb(gboolean ok, GAtResult *result,
+static void set_indreg(struct gprs_data *gd, const char *ind, gboolean present)
+{
+	if(g_str_equal(ind,"CGREG"))
+		gd->has_cgreg = present;
+
+	if(g_str_equal(ind,"CEREG"))
+		gd->has_cereg = present;
+
+	if(g_str_equal(ind,"C5GREG"))
+		gd->has_c5greg = present;
+
+}
+
+static void at_indreg_test_cb(gboolean ok, GAtResult *result,
 				gpointer user_data)
 {
-	struct ofono_gprs *gprs = user_data;
+	struct cb_data *cbd = user_data;
+	struct ofono_gprs *gprs = cbd->cb;
+	const char *ind=cbd->data;
+	const char *last=cbd->user;
+
 	struct gprs_data *gd = ofono_gprs_get_data(gprs);
 	gint range[2];
 	GAtResultIter iter;
 	int cgreg1 = 0;
 	int cgreg2 = 0;
-	const char *cmd;
+	char buf[32];
 
 	if (!ok)
 		goto error;
@@ -506,7 +901,8 @@ static void at_cgreg_test_cb(gboolean ok, GAtResult *result,
 	g_at_result_iter_init(&iter, result);
 
 retry:
-	if (!g_at_result_iter_next(&iter, "+CGREG:"))
+	sprintf(buf,"+%s:",ind);
+	if (!g_at_result_iter_next(&iter, buf))
 		goto error;
 
 	if (!g_at_result_iter_open_list(&iter))
@@ -521,45 +917,75 @@ retry:
 
 	g_at_result_iter_close_list(&iter);
 
-	if (cgreg2)
-		cmd = "AT+CGREG=2";
-	else if (cgreg1)
-		cmd = "AT+CGREG=1";
-	else
+	if(gd->vendor==OFONO_VENDOR_GEMALTO) {
+		/*
+		 * Gemalto prefers to print as much information as available
+		 * for support purposes
+		 */
+		sprintf(buf, "AT+%s=%d",ind, range[1]);
+	} else if (cgreg1) {
+		sprintf(buf,"AT+%s=1", ind);
+	} else if (cgreg2) {
+		sprintf(buf,"AT+%s=2", ind);
+	} else
 		goto error;
 
-	g_at_chat_send(gd->chat, cmd, none_prefix, NULL, NULL, NULL);
-	g_at_chat_send(gd->chat, "AT+CGAUTO=0", none_prefix, NULL, NULL, NULL);
-
-	switch (gd->vendor) {
-	case OFONO_VENDOR_MBM:
-		/* Ericsson MBM and ST-E modems don't support AT+CGEREP=2,1 */
-		g_at_chat_send(gd->chat, "AT+CGEREP=1,0", none_prefix,
-			gprs_initialized, gprs, NULL);
-		break;
-	case OFONO_VENDOR_NOKIA:
-		/* Nokia data cards don't support AT+CGEREP=1,0 either */
-		g_at_chat_send(gd->chat, "AT+CGEREP=1", none_prefix,
-			gprs_initialized, gprs, NULL);
-		break;
-	default:
-		g_at_chat_send(gd->chat, "AT+CGEREP=2,1", none_prefix,
-			gprs_initialized, gprs, NULL);
-		break;
-	}
+	set_indreg(gd, ind,TRUE);
+	g_at_chat_send(gd->chat, buf, none_prefix, NULL, NULL, NULL);
 
+	if(last)
+		goto endcheck;
 	return;
 
 error:
-	ofono_info("GPRS not supported on this device");
-	ofono_gprs_remove(gprs);
+	set_indreg(gd, ind,FALSE);
+	if(!last)
+		return;
+
+endcheck:
+	if (gd->has_cgreg)
+		gd->nb_inds++;
+	if (gd->has_cereg)
+		gd->nb_inds++;
+	if (gd->has_c5greg)
+		gd->nb_inds++;
+
+	if(gd->nb_inds==0) {
+		ofono_info("GPRS not supported on this device");
+		ofono_gprs_remove(gprs);
+		return;
+	}
+
+	gprs_initialized(gprs);
+}
+
+static void test_and_set_regstatus(struct ofono_gprs *gprs) {
+	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+	struct cb_data *cbd_cg  = cb_data_new(gprs, "CGREG");
+	struct cb_data *cbd_ce  = cb_data_new(gprs, "CEREG");
+	struct cb_data *cbd_c5g = cb_data_new(gprs, "C5GREG");
+
+	cbd_c5g->user="last";
+
+	/*
+	 * modules can support one to all of the network registration indicators
+	 *
+	 * ofono will execute the next commands and related callbacks in order
+	 * therefore it is possible to verify all result on the last one.
+	 */
+
+	g_at_chat_send(gd->chat, "AT+CGREG=?", cgreg_prefix,
+					at_indreg_test_cb, cbd_cg, g_free);
+	g_at_chat_send(gd->chat, "AT+CEREG=?", cereg_prefix,
+					at_indreg_test_cb, cbd_ce, g_free);
+	g_at_chat_send(gd->chat, "AT+C5GREG=?", c5greg_prefix,
+					at_indreg_test_cb, cbd_c5g, g_free);
 }
 
 static void at_cgdcont_test_cb(gboolean ok, GAtResult *result,
 				gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
-	struct gprs_data *gd = ofono_gprs_get_data(gprs);
 	GAtResultIter iter;
 	int min, max;
 	const char *pdp_type;
@@ -592,18 +1018,16 @@ static void at_cgdcont_test_cb(gboolean ok, GAtResult *result,
 			continue;
 
 		/* We look for IP PDPs */
-		if (g_str_equal(pdp_type, "IP"))
+		if (g_str_equal(pdp_type, "IP")){
 			found = TRUE;
+		}
 	}
 
 	if (found == FALSE)
 		goto error;
 
 	ofono_gprs_set_cid_range(gprs, min, max);
-
-	g_at_chat_send(gd->chat, "AT+CGREG=?", cgreg_prefix,
-			at_cgreg_test_cb, gprs, NULL);
-
+	test_and_set_regstatus(gprs);
 	return;
 
 error:
@@ -616,6 +1040,8 @@ static int at_gprs_probe(struct ofono_gprs *gprs,
 {
 	GAtChat *chat = data;
 	struct gprs_data *gd;
+	int autoattach;
+	struct ofono_modem* modem=ofono_gprs_get_modem(gprs);
 
 	gd = g_try_new0(struct gprs_data, 1);
 	if (gd == NULL)
@@ -626,8 +1052,16 @@ static int at_gprs_probe(struct ofono_gprs *gprs,
 
 	ofono_gprs_set_data(gprs, gd);
 
-	g_at_chat_send(gd->chat, "AT+CGDCONT=?", cgdcont_prefix,
-			at_cgdcont_test_cb, gprs, NULL);
+	if (gd->vendor==OFONO_VENDOR_GEMALTO) {
+		autoattach=ofono_modem_get_integer(modem, "Gto_Autoattach");
+		/* set autoattach */
+		gd->auto_attach = (autoattach==1);
+		/* skip the cgdcont scanning: set manually */
+		test_and_set_regstatus(gprs);
+	} else {
+		g_at_chat_send(gd->chat, "AT+CGDCONT=?", cgdcont_prefix,
+						at_cgdcont_test_cb, gprs, NULL);
+	}
 
 	return 0;
 }
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5f..429b847 100644
--- a/drivers/atmodem/lte.c
+++ b/drivers/atmodem/lte.c
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2017  Intel Corporation. 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
@@ -40,22 +41,273 @@
 #include "gatresult.h"
 
 #include "atmodem.h"
+#include "vendor.h"
 
 struct lte_driver_data {
 	GAtChat *chat;
+	unsigned int vendor;
 };
 
+struct lte_cb_data {
+	const struct ofono_lte_default_attach_info *info;
+	ofono_lte_cb_t cb;
+	const struct ofono_lte *lte;
+	void *data;
+};
+
+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 at_lte_set_default_auth_info_cb(gboolean ok, GAtResult *result,
+							gpointer user_data)
+{
+	struct lte_cb_data *lcbd = user_data;
+	struct ofono_error error;
+	ofono_lte_cb_t cb = lcbd->cb;
+	void *data = lcbd->data;
+
+	DBG("ok %d", ok);
+
+	decode_at_error(&error, g_at_result_final_response(result));
+	cb(&error, data);
+}
+
+static void gemalto_lte_set_default_auth_info(const struct ofono_lte *lte,
+			const struct ofono_lte_default_attach_info *info,
+			ofono_lte_cb_t cb, void *data)
+{
+	struct lte_cb_data *lcbd = data;
+	void* ud = lcbd->data;
+	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+	struct ofono_modem *modem = ofono_lte_get_modem(lte);
+	char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+					OFONO_GPRS_MAX_PASSWORD_LENGTH +1];
+
+	if(!gemalto_get_auth_command(modem, 1, info->auth_method,
+			info->username, info->password, buf, sizeof(buf))) {
+		g_free(lcbd);
+		goto set_auth_failure;
+	}
+
+	if(g_at_chat_send(ldd->chat, buf, NULL, at_lte_set_default_auth_info_cb,
+							lcbd, g_free) > 0)
+		return;
+
+set_auth_failure:
+	CALLBACK_WITH_FAILURE(cb, ud);
+}
+
+static void at_lte_set_default_auth_info(const struct ofono_lte *lte,
+			const struct ofono_lte_default_attach_info *info,
+			ofono_lte_cb_t cb, void *data)
+{
+	struct lte_cb_data *lcbd = data;
+	void* ud = lcbd->data;
+	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+	char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+					OFONO_GPRS_MAX_PASSWORD_LENGTH +1];
+	guint buflen = sizeof(buf);
+
+	snprintf(buf, buflen, "AT+CGAUTH=0,");
+	buflen-=strlen(buf);
+
+	switch(info->auth_method) {
+	case OFONO_GPRS_AUTH_METHOD_NONE:
+		snprintf(buf+strlen(buf), buflen, "0");
+		break;
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		snprintf(buf+strlen(buf), buflen, "1,\"%s\",\"%s\"",
+						info->username, info->password);
+		break;
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+		snprintf(buf+strlen(buf), buflen, "2,\"%s\",\"%s\"",
+						info->username, info->password);
+		break;
+	default:
+		g_free(lcbd);
+		goto set_auth_failure;
+		break;
+	}
+
+	if(g_at_chat_send(ldd->chat, buf, NULL, at_lte_set_default_auth_info_cb,
+							lcbd, g_free) > 0)
+		return;
+
+set_auth_failure:
+	CALLBACK_WITH_FAILURE(cb, ud);
+}
+
 static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
 							gpointer user_data)
 {
-	struct cb_data *cbd = user_data;
-	ofono_lte_cb_t cb = cbd->cb;
+	struct lte_cb_data *lcbd = user_data;
+	struct lte_driver_data *ldd = ofono_lte_get_data(lcbd->data);
 	struct ofono_error error;
 
 	DBG("ok %d", ok);
 
+	if (ok) {
+		switch (ldd->vendor) {
+		case OFONO_VENDOR_GEMALTO:
+			gemalto_lte_set_default_auth_info(lcbd->lte,
+					lcbd->info, lcbd->cb, user_data);
+			return;
+			break;
+		default:
+			at_lte_set_default_auth_info(lcbd->lte,
+					lcbd->info, lcbd->cb, user_data);
+			return;
+			break;
+		}
+	}
+
 	decode_at_error(&error, g_at_result_final_response(result));
-	cb(&error, cbd->data);
+	lcbd->cb(&error, lcbd->data);
+}
+
+static void gemalto_lte_set_default_attach_info(const struct ofono_lte *lte,
+			const struct ofono_lte_default_attach_info *info,
+			ofono_lte_cb_t cb, void *data)
+{
+	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
+	struct ofono_modem *modem = ofono_lte_get_modem(lte);
+	char buf[32 + OFONO_GPRS_MAX_APN_LENGTH  +1];
+	struct lte_cb_data *lcbd;
+	int gto_autoconf = ofono_modem_get_integer(modem, "Gemalto_Autoconf");
+
+	/*
+	 * to be completed. May require additional properties in the driver
+	 * current values plan for Gemalto_Autoconf:
+	 * 0: no autoconf (or no param set)
+	 * 1: autoconf activated but fallback selected
+	 * 2: autoconf activated, profile selected,
+	 *		but custom attach apn required for this application
+	 * 3-9: rfu
+	 * 10: autoconf default bearer and ims
+	 * 20: autoconf 10 + autoconf private apn
+	 */
+	if(gto_autoconf>=10) {
+		CALLBACK_WITH_SUCCESS(cb, data);
+		return;
+	}
+
+	lcbd = g_new0(struct lte_cb_data, 1);
+	lcbd->data = data;
+	lcbd->info = info;
+	lcbd->cb = cb;
+	lcbd->lte = lte;
+
+	gemalto_get_cgdcont_command(modem, 1, info->proto, info->apn, buf,
+								sizeof(buf));
+
+	if (g_at_chat_send(ldd->chat, buf, NULL,
+			at_lte_set_default_attach_info_cb, lcbd, NULL) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(cb, data);
 }
 
 static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
@@ -63,20 +315,28 @@ static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
 			ofono_lte_cb_t cb, void *data)
 {
 	struct lte_driver_data *ldd = ofono_lte_get_data(lte);
-	char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
-	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[32 + OFONO_GPRS_MAX_APN_LENGTH  +1];
+	struct lte_cb_data *lcbd;
 
-	DBG("LTE config with APN: %s", info->apn);
+	if(ldd->vendor==OFONO_VENDOR_GEMALTO) {
+		gemalto_lte_set_default_attach_info(lte, info, cb, data);
+		return;
+	}
+
+	lcbd = g_new0(struct lte_cb_data, 1);
+	lcbd->data = data;
+	lcbd->info = info;
+	lcbd->cb = cb;
+	lcbd->lte = lte;
 
 	if (strlen(info->apn) > 0)
 		snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
-				info->apn);
+							info->apn);
 	else
 		snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\"");
 
-	/* We can't do much in case of failure so don't check response. */
 	if (g_at_chat_send(ldd->chat, buf, NULL,
-			at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
+			at_lte_set_default_attach_info_cb, lcbd, NULL) > 0)
 		return;
 
 	CALLBACK_WITH_FAILURE(cb, data);
@@ -84,9 +344,7 @@ static void at_lte_set_default_attach_info(const struct ofono_lte *lte,
 
 static gboolean lte_delayed_register(gpointer user_data)
 {
-	struct ofono_lte *lte = user_data;
-
-	ofono_lte_register(lte);
+	ofono_lte_register(user_data);
 
 	return FALSE;
 }
@@ -103,6 +361,7 @@ static int at_lte_probe(struct ofono_lte *lte, unsigned int vendor, void *data)
 		return -ENOMEM;
 
 	ldd->chat = g_at_chat_clone(chat);
+	ldd->vendor = vendor;
 
 	ofono_lte_set_data(lte, ldd);
 
diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index 0854cd1..c6d00a9 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -4,6 +4,7 @@
  *
  *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
  *  Copyright (C) 2010  ST-Ericsson AB.
+ *  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
@@ -56,6 +57,7 @@ struct netreg_data {
 	GAtChat *chat;
 	char mcc[OFONO_MAX_MCC_LENGTH + 1];
 	char mnc[OFONO_MAX_MNC_LENGTH + 1];
+	unsigned int csq_source;
 	int signal_index; /* If strength is reported via CIND */
 	int signal_min; /* min strength reported via CIND */
 	int signal_max; /* max strength reported via CIND */
@@ -179,7 +181,7 @@ static int option_parse_tech(GAtResult *result)
 	return tech;
 }
 
-static int cinterion_parse_tech(GAtResult *result)
+static int gemalto_parse_tech(GAtResult *result)
 {
 	int tech = -1;
 	GAtResultIter iter;
@@ -231,13 +233,13 @@ static void at_creg_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	cb(&error, status, lac, ci, tech, cbd->data);
 }
 
-static void cinterion_query_tech_cb(gboolean ok, GAtResult *result,
+static void gemalto_query_tech_cb(gboolean ok, GAtResult *result,
                                               gpointer user_data)
 {
 	struct tech_query *tq = user_data;
 	int tech;
 
-	tech = cinterion_parse_tech(result);
+	tech = gemalto_parse_tech(result);
 
 	ofono_netreg_status_notify(tq->netreg,
 			tq->status, tq->lac, tq->ci, tech);
@@ -664,6 +666,45 @@ static void csq_notify(GAtResult *result, gpointer user_data)
 				at_util_convert_signal_strength(strength));
 }
 
+static void cesq_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	int strength_GSM, strength_UTRAN, strength_EUTRAN;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CESQ:"))
+		return;
+
+	/* rxlevel gsm, use as a strength indicator */
+	if (!g_at_result_iter_next_number(&iter, &strength_GSM))
+		return;
+
+	/* ber gsm, ignore*/
+	if (!g_at_result_iter_skip_next(&iter))
+		return;
+
+	/* rscp utran, use as a strength indicator */
+	if (!g_at_result_iter_next_number(&iter, &strength_UTRAN))
+		return;
+
+	/* ecno utran, ignore */
+	if (!g_at_result_iter_skip_next(&iter))
+		return;
+
+	/* rsrq eutran, ignore */
+	if (!g_at_result_iter_skip_next(&iter))
+		return;
+
+	/* rsrp eutran, use as a strength indicator */
+	if (!g_at_result_iter_next_number(&iter, &strength_EUTRAN))
+		return;
+
+	ofono_netreg_strength_notify(netreg,
+				at_util_convert_signal_strength_cesq(strength_GSM, strength_UTRAN, strength_EUTRAN));
+}
+
 static void calypso_csq_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_netreg *netreg = user_data;
@@ -702,7 +743,6 @@ static void option_osigq_notify(GAtResult *result, gpointer user_data)
 
 static void ifx_xhomezr_notify(GAtResult *result, gpointer user_data)
 {
-	//struct ofono_netreg *netreg = user_data;
 	const char *label;
 	GAtResultIter iter;
 
@@ -767,7 +807,6 @@ static void ifx_xreg_notify(GAtResult *result, gpointer user_data)
 
 static void ifx_xciev_notify(GAtResult *result, gpointer user_data)
 {
-	//struct ofono_netreg *netreg = user_data;
 	int ind;
 	GAtResultIter iter;
 
@@ -876,7 +915,7 @@ static void telit_ciev_notify(GAtResult *result, gpointer user_data)
 	ofono_netreg_strength_notify(netreg, strength);
 }
 
-static void cinterion_ciev_notify(GAtResult *result, gpointer user_data)
+static void gemalto_ciev_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_netreg *netreg = user_data;
 	struct netreg_data *nd = ofono_netreg_get_data(netreg);
@@ -1233,18 +1272,23 @@ static void at_signal_strength(struct ofono_netreg *netreg,
 
 	cbd->user = nd;
 
-	/*
-	 * If we defaulted to using CIND, then keep using it,
-	 * otherwise fall back to CSQ
-	 */
-	if (nd->signal_index > 0) {
-		if (g_at_chat_send(nd->chat, "AT+CIND?", cind_prefix,
-					cind_cb, cbd, g_free) > 0)
-			return;
-	} else {
-		if (g_at_chat_send(nd->chat, "AT+CSQ", csq_prefix,
-				csq_cb, cbd, g_free) > 0)
-			return;
+	switch(nd->vendor) {
+	case OFONO_VENDOR_GEMALTO:
+		break;
+	default:
+		/*
+		 * If we defaulted to using CIND, then keep using it,
+		 * otherwise fall back to CSQ
+		 */
+		if (nd->signal_index > 0) {
+			if (g_at_chat_send(nd->chat, "AT+CIND?", cind_prefix,
+						cind_cb, cbd, g_free) > 0)
+				return;
+		} else {
+			if (g_at_chat_send(nd->chat, "AT+CSQ", csq_prefix,
+					csq_cb, cbd, g_free) > 0)
+				return;
+		}
 	}
 
 	g_free(cbd);
@@ -1534,6 +1578,9 @@ static void creg_notify(GAtResult *result, gpointer user_data)
 	tq->ci = ci;
 	tq->netreg = netreg;
 
+	if ((status == 1 || status == 5) && tech == -1)
+		tech = nd->tech;
+
 	switch (nd->vendor) {
 	case OFONO_VENDOR_GOBI:
 		if (g_at_chat_send(nd->chat, "AT*CNTI=0", none_prefix,
@@ -1556,19 +1603,18 @@ static void creg_notify(GAtResult *result, gpointer user_data)
 					option_query_tech_cb, tq, g_free) > 0)
 			return;
 		break;
-    case OFONO_VENDOR_CINTERION:
-              if (g_at_chat_send(nd->chat, "AT^SMONI",
-                                      smoni_prefix,
-                                      cinterion_query_tech_cb, tq, g_free) > 0)
-                      return;
-              break;
+    case OFONO_VENDOR_GEMALTO:
+		if (tech!=-1)
+			break;  /* technology already returned by +CREG, so run the notify label */
+		if (g_at_chat_send(nd->chat, "AT^SMONI",
+					smoni_prefix,
+					gemalto_query_tech_cb, tq, g_free) > 0)
+			return;
+		break;
 	}
 
 	g_free(tq);
 
-	if ((status == 1 || status == 5) && tech == -1)
-		tech = nd->tech;
-
 notify:
 	ofono_netreg_status_notify(netreg, status, lac, ci, tech);
 }
@@ -1861,6 +1907,17 @@ error:
 	ofono_netreg_remove(netreg);
 }
 
+static gboolean gemalto_csq_query(gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+
+	g_at_chat_send(nd->chat, "AT+CSQ", none_prefix, NULL, NULL, NULL);
+	g_at_chat_send(nd->chat, "AT+CESQ", none_prefix, NULL, NULL, NULL);
+
+	return TRUE;
+}
+
 static void at_creg_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_netreg *netreg = user_data;
@@ -2028,12 +2085,12 @@ static void at_creg_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
 		g_at_chat_send(nd->chat, "AT*TLTS=1", none_prefix,
 						NULL, NULL, NULL);
 		break;
-	case OFONO_VENDOR_CINTERION:
+	case OFONO_VENDOR_GEMALTO:
 		/*
-		 * We can't set rssi bounds from Cinterion responses
+		 * We can't set rssi bounds from Gemalto responses
 		 * so set them up to specified values here
 		 *
-		 * Cinterion rssi signal strength specified as:
+		 * Gemalto rssi signal strength specified as:
 		 * 0      <= -112dBm
 		 * 1 - 4  signal strengh in 15 dB steps
 		 * 5      >= -51 dBm
@@ -2042,12 +2099,20 @@ static void at_creg_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
 		nd->signal_min = 0;
 		nd->signal_max = 5;
 		nd->signal_invalid = 99;
-
 		/* Register for specific signal strength reports */
+		g_at_chat_register(nd->chat, "+CIEV:",
+				gemalto_ciev_notify, FALSE, netreg, NULL);
 		g_at_chat_send(nd->chat, "AT^SIND=\"rssi\",1", none_prefix,
 				NULL, NULL, NULL);
-		g_at_chat_register(nd->chat, "+CIEV:",
-				cinterion_ciev_notify, FALSE, netreg, NULL);
+		/* Register for +CSQ and +CESQ and then poll them periodically */
+		g_at_chat_register(nd->chat, "+CSQ:", csq_notify,
+						FALSE, netreg, NULL);
+		g_at_chat_register(nd->chat, "+CESQ:", cesq_notify,
+						FALSE, netreg, NULL);
+
+		if (!nd->csq_source) /* in case it has already be called, to avoid a memory leak */
+			nd->csq_source = g_timeout_add_seconds(60, gemalto_csq_query, netreg);
+
 		break;
 	case OFONO_VENDOR_NOKIA:
 	case OFONO_VENDOR_SAMSUNG:
@@ -2145,6 +2210,9 @@ static void at_netreg_remove(struct ofono_netreg *netreg)
 	if (nd->nitz_timeout)
 		g_source_remove(nd->nitz_timeout);
 
+	if (nd->csq_source)
+		g_source_remove(nd->csq_source);
+
 	ofono_netreg_set_data(netreg, NULL);
 
 	g_at_chat_unref(nd->chat);
diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index 0907635..2ce72c8 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -66,7 +66,7 @@ static const char *oercn_prefix[] = { "_OERCN:", NULL };
 static const char *cpinr_prefixes[] = { "+CPINR:", "+CPINRE:", NULL };
 static const char *epin_prefix[] = { "*EPIN:", NULL };
 static const char *simcom_spic_prefix[] = { "+SPIC:", NULL };
-static const char *cinterion_spic_prefix[] = { "^SPIC:", NULL };
+static const char *gemalto_spic_prefix[] = { "^SPIC:", NULL };
 static const char *pct_prefix[] = { "#PCT:", NULL };
 static const char *pnnm_prefix[] = { "+PNNM:", NULL };
 static const char *qpinc_prefix[] = { "+QPINC:", NULL };
@@ -1110,7 +1110,7 @@ error:
 	CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
 }
 
-static void cinterion_spic_cb(gboolean ok, GAtResult *result,
+static void gemalto_spic_cb(gboolean ok, GAtResult *result,
 							gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -1227,9 +1227,9 @@ static void at_pin_retries_query(struct ofono_sim *sim,
 					upincnt_cb, cbd, g_free) > 0)
 			return;
 		break;
-	case OFONO_VENDOR_CINTERION:
-		if (g_at_chat_send(sd->chat, "AT^SPIC", cinterion_spic_prefix,
-					cinterion_spic_cb, cbd, g_free) > 0)
+	case OFONO_VENDOR_GEMALTO:
+		if (g_at_chat_send(sd->chat, "AT^SPIC", gemalto_spic_prefix,
+					gemalto_spic_cb, cbd, g_free) > 0)
 			return;
 		break;
 	default:
diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index 7e78fcf..64d0f6e 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -219,10 +219,16 @@ static void at_cmgs(struct ofono_sms *sms, const unsigned char *pdu,
 	char buf[512];
 	int len;
 
-	if (mms) {
-		snprintf(buf, sizeof(buf), "AT+CMMS=%d", mms);
-		g_at_chat_send(data->chat, buf, none_prefix,
-				NULL, NULL, NULL);
+	switch(data->vendor) {
+	case OFONO_VENDOR_GEMALTO:
+		break;
+	default:
+		if (mms) {
+			snprintf(buf, sizeof(buf), "AT+CMMS=%d", mms);
+			g_at_chat_send(data->chat, buf, none_prefix,
+					NULL, NULL, NULL);
+		}
+		break;
 	}
 
 	len = snprintf(buf, sizeof(buf), "AT+CMGS=%d\r", tpdu_len);
@@ -329,7 +335,7 @@ static inline void at_ack_delivery(struct ofono_sms *sms)
 	/* We must acknowledge the PDU using CNMA */
 	if (data->cnma_ack_pdu) {
 		switch (data->vendor) {
-		case OFONO_VENDOR_CINTERION:
+		case OFONO_VENDOR_GEMALTO:
 			snprintf(buf, sizeof(buf), "AT+CNMA=1");
 			break;
 		default:
@@ -411,10 +417,10 @@ static void at_cmt_notify(GAtResult *result, gpointer user_data)
 		goto err;
 
 	switch (data->vendor) {
-	case OFONO_VENDOR_CINTERION:
+	case OFONO_VENDOR_GEMALTO:
 		if (!g_at_result_iter_next_number(&iter, &tpdu_len)) {
 			/*
-			 * Some cinterions modems (ALS3,PLS8...), act in
+			 * Some Gemalto modems (ALS3,PLS8...), act in
 			 * accordance with 3GPP 27.005.  So we need to skip
 			 * the first (<alpha>) field
 			 *  \r\n+CMT: ,23\r\nCAFECAFECAFE... ...\r\n
@@ -455,6 +461,7 @@ static void at_cmt_notify(GAtResult *result, gpointer user_data)
 
 	if (data->vendor != OFONO_VENDOR_SIMCOM)
 		at_ack_delivery(sms);
+	return;
 
 err:
 	ofono_error("Unable to parse CMT notification");
diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
index 721796e..abe2d89 100644
--- a/drivers/atmodem/vendor.h
+++ b/drivers/atmodem/vendor.h
@@ -49,4 +49,5 @@ enum ofono_vendor {
 	OFONO_VENDOR_UBLOX_TOBY_L2,
 	OFONO_VENDOR_CINTERION,
 	OFONO_VENDOR_XMM,
+	OFONO_VENDOR_GEMALTO,
 };
diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
index 91cf238..7afd3c1 100644
--- a/drivers/gemaltomodem/gemaltomodem.c
+++ b/drivers/gemaltomodem/gemaltomodem.c
@@ -35,13 +35,16 @@
 static int gemaltomodem_init(void)
 {
 	gemalto_location_reporting_init();
-
+	gemaltowwan_gprs_context_init();
+	gemalto_voicecall_init();
 	return 0;
 }
 
 static void gemaltomodem_exit(void)
 {
 	gemalto_location_reporting_exit();
+	gemaltowwan_gprs_context_exit();
+	gemalto_voicecall_exit();
 }
 
 OFONO_PLUGIN_DEFINE(gemaltomodem, "Gemalto modem driver", VERSION,
diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
index 7ea1e8f..183c370 100644
--- a/drivers/gemaltomodem/gemaltomodem.h
+++ b/drivers/gemaltomodem/gemaltomodem.h
@@ -19,7 +19,24 @@
  *
  */
 
-#include <drivers/atmodem/atutil.h>
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <glib.h>
+#include <gatchat.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/types.h>
+
+#include <drivers/atmodem/atmodem.h>
 
 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..669e025
--- /dev/null
+++ b/drivers/gemaltomodem/gprs-context-wwan.c
@@ -0,0 +1,445 @@
+/*
+ *
+ *  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);
+}
diff --git a/drivers/gemaltomodem/voicecall.c b/drivers/gemaltomodem/voicecall.c
new file mode 100644
index 0000000..e67da92
--- /dev/null
+++ b/drivers/gemaltomodem/voicecall.c
@@ -0,0 +1,965 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/voicecall.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "common.h"
+
+#include "gemaltomodem.h"
+
+static const char *clcc_prefix[] = { "+CLCC:", NULL };
+static const char *none_prefix[] = { NULL };
+
+/* According to 27.007 COLP is an intermediate status for ATD */
+static const char *atd_prefix[] = { "+COLP:", NULL };
+
+#define FLAG_NEED_CLIP 1
+
+struct voicecall_data {
+	GAtChat *chat;
+	GSList *calls;
+	unsigned int local_release;
+	unsigned int vendor;
+	unsigned char flags;
+};
+
+struct release_id_req {
+	struct ofono_voicecall *vc;
+	ofono_voicecall_cb_t cb;
+	void *data;
+	int id;
+};
+
+struct change_state_req {
+	struct ofono_voicecall *vc;
+	ofono_voicecall_cb_t cb;
+	void *data;
+	int affected_types;
+};
+
+static void generic_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct change_state_req *req = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+	struct ofono_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (ok && req->affected_types) {
+		GSList *l;
+		struct ofono_call *call;
+
+		for (l = vd->calls; l; l = l->next) {
+			call = l->data;
+
+			if (req->affected_types & (1 << call->status))
+				vd->local_release |= (1 << call->id);
+		}
+	}
+
+	req->cb(&error, req->data);
+}
+
+static void gemalto_template(const char *cmd, struct ofono_voicecall *vc,
+			GAtResultFunc result_cb, unsigned int affected_types,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct change_state_req *req = g_try_new0(struct change_state_req, 1);
+
+	if (req == NULL)
+		goto error;
+
+	req->vc = vc;
+	req->cb = cb;
+	req->data = data;
+	req->affected_types = affected_types;
+
+	if (g_at_chat_send(vd->chat, cmd, none_prefix,
+				result_cb, req, g_free) > 0)
+		return;
+
+error:
+	g_free(req);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void gemalto_answer(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	gemalto_template("ATA", vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_hangup_all(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
+				(1 << CALL_STATUS_DIALING) |
+				(1 << CALL_STATUS_ALERTING) |
+				(1 << CALL_STATUS_WAITING) |
+				(1 << CALL_STATUS_HELD) |
+				(1 << CALL_STATUS_ACTIVE);
+
+	/* Hangup all calls */
+	gemalto_template("AT+CHUP", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_hangup(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
+
+	/* Hangup current active call */
+	gemalto_template("AT+CHLD=1", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_hold_all_active(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
+	gemalto_template("AT+CHLD=2", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_release_all_held(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
+				(1 << CALL_STATUS_WAITING);
+
+	gemalto_template("AT+CHLD=0", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_set_udub(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
+				(1 << CALL_STATUS_WAITING);
+
+	gemalto_template("AT+CHLD=0", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_release_all_active(struct ofono_voicecall *vc,
+					ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
+
+	gemalto_template("AT+CHLD=1", vc, generic_cb, affected, cb, data);
+}
+
+static void release_id_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct release_id_req *req = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+	struct ofono_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (ok)
+		vd->local_release = 1 << req->id;
+
+	req->cb(&error, req->data);
+}
+
+static void gemalto_release_specific(struct ofono_voicecall *vc, int id,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct release_id_req *req = g_try_new0(struct release_id_req, 1);
+	char buf[32];
+
+	if (req == NULL)
+		goto error;
+
+	req->vc = vc;
+	req->cb = cb;
+	req->data = data;
+	req->id = id;
+
+	snprintf(buf, sizeof(buf), "AT+CHLD=1%d", id);
+
+	if (g_at_chat_send(vd->chat, buf, none_prefix,
+				release_id_cb, req, g_free) > 0)
+		return;
+
+error:
+	g_free(req);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void gemalto_private_chat(struct ofono_voicecall *vc, int id,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	char buf[32];
+
+	snprintf(buf, sizeof(buf), "AT+CHLD=2%d", id);
+	gemalto_template(buf, vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_create_multiparty(struct ofono_voicecall *vc,
+					ofono_voicecall_cb_t cb, void *data)
+{
+	gemalto_template("AT+CHLD=3", vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_transfer(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	/* Held & Active */
+	unsigned int affected = (1 << CALL_STATUS_ACTIVE) |
+				(1 << CALL_STATUS_HELD);
+
+	/* Transfer can puts held & active calls together and disconnects
+	 * from both.  However, some networks support transferring of
+	 * dialing/ringing calls as well.
+	 */
+	affected |= (1 << CALL_STATUS_DIALING) |
+				(1 << CALL_STATUS_ALERTING);
+
+	gemalto_template("AT+CHLD=4", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_send_dtmf(struct ofono_voicecall *vc, const char *dtmf,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	int len = strlen(dtmf);
+	int s;
+	int i;
+	char *buf;
+	struct ofono_modem *modem = ofono_voicecall_get_modem(vc);
+	int use_quotes = ofono_modem_get_integer(modem, "Gemalto_VTS_quotes");
+
+	/* strlen("+VTS=\"T\";") = 9 + initial AT + null */
+	buf = g_try_new(char, len * 9 + 3);
+
+	if (buf == NULL) {
+		CALLBACK_WITH_FAILURE(cb, data);
+		return;
+	}
+
+	if(use_quotes)
+		s = sprintf(buf, "AT+VTS=\"%c\"", dtmf[0]);
+	else
+		s = sprintf(buf, "AT+VTS=%c", dtmf[0]);
+
+	for (i = 1; i < len; i++) {
+
+		if(use_quotes)
+			s += sprintf(buf + s, ";+VTS=\"%c\"", dtmf[i]);
+		else
+			s += sprintf(buf + s, ";+VTS=%c", dtmf[i]);
+	}
+
+	g_at_chat_send(vd->chat, buf, NULL, NULL, NULL, NULL);
+	g_free(buf);
+}
+
+static struct ofono_call *create_call(struct ofono_voicecall *vc, int type,
+					int direction, int status,
+					const char *num, int num_type, int clip)
+{
+	struct voicecall_data *d = ofono_voicecall_get_data(vc);
+	struct ofono_call *call;
+
+	/* Generate a call structure for the waiting call */
+	call = g_try_new(struct ofono_call, 1);
+	if (call == NULL)
+		return NULL;
+
+	ofono_call_init(call);
+
+	call->id = ofono_voicecall_get_next_callid(vc);
+	call->type = type;
+	call->direction = direction;
+	call->status = status;
+
+	if (clip != 2) {
+		strncpy(call->phone_number.number, num,
+			OFONO_MAX_PHONE_NUMBER_LENGTH);
+		call->phone_number.type = num_type;
+	}
+
+	call->clip_validity = clip;
+	call->cnap_validity = CNAP_VALIDITY_NOT_AVAILABLE;
+
+	d->calls = g_slist_insert_sorted(d->calls, call, at_util_call_compare);
+
+	return call;
+}
+
+static void atd_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	struct ofono_voicecall *vc = cbd->user;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	ofono_voicecall_cb_t cb = cbd->cb;
+	GAtResultIter iter;
+	const char *num;
+	int type = 128;
+	int validity = 2;
+	struct ofono_error error;
+	struct ofono_call *call;
+	GSList *l;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok)
+		goto out;
+
+	/* On a success, make sure to put all active calls on hold */
+	for (l = vd->calls; l; l = l->next) {
+		call = l->data;
+
+		if (call->status != CALL_STATUS_ACTIVE)
+			continue;
+
+		call->status = CALL_STATUS_HELD;
+		ofono_voicecall_notify(vc, call);
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	if (g_at_result_iter_next(&iter, "+COLP:")) {
+		g_at_result_iter_next_string(&iter, &num);
+		g_at_result_iter_next_number(&iter, &type);
+
+		if (strlen(num) > 0)
+			validity = 0;
+		else
+			validity = 2;
+
+		DBG("colp_notify: %s %d %d", num, type, validity);
+	}
+
+	/* Generate a voice call that was just dialed, we guess the ID */
+	call = create_call(vc, 0, 0, CALL_STATUS_DIALING, num, type, validity);
+	if (call == NULL) {
+		ofono_error("Unable to malloc, call tracking will fail!");
+		return;
+	}
+
+	/* oFono core will generate a call with the dialed number
+	 * inside its dial callback.  Unless we got COLP information
+	 * we do not need to communicate that a call is being
+	 * dialed
+	 */
+	if (validity != 2)
+		ofono_voicecall_notify(vc, call);
+
+out:
+	cb(&error, cbd->data);
+}
+
+static void gemalto_dial(struct ofono_voicecall *vc,
+			const struct ofono_phone_number *ph,
+			enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
+			void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[256];
+
+	cbd->user = vc;
+
+	if (ph->type == 145)
+		snprintf(buf, sizeof(buf), "ATD+%s", ph->number);
+	else
+		snprintf(buf, sizeof(buf), "ATD%s", ph->number);
+
+	switch (clir) {
+	case OFONO_CLIR_OPTION_INVOCATION:
+		strcat(buf, "I");
+		break;
+	case OFONO_CLIR_OPTION_SUPPRESSION:
+		strcat(buf, "i");
+		break;
+	default:
+		break;
+	}
+
+	strcat(buf, ";");
+
+	if (g_at_chat_send(vd->chat, buf, atd_prefix,
+				atd_cb, cbd, g_free) > 0)
+		return;
+
+	g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static GSList *gemalto_parse_slcc(GAtResult *result, unsigned int *ret_mpty_ids)
+{
+	GAtResultIter iter;
+	GSList *l = NULL;
+	int id, dir, status, type;
+	ofono_bool_t mpty;
+	struct ofono_call *call;
+	unsigned int mpty_ids = 0;
+
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, "+CLCC:")) {
+		const char *str = "";
+		int number_type = 129;
+
+		if (!g_at_result_iter_next_number(&iter, &id))
+			continue;
+
+		if (id == 0)
+			continue;
+
+		if (!g_at_result_iter_next_number(&iter, &dir))
+			continue;
+
+		if (!g_at_result_iter_next_number(&iter, &status))
+			continue;
+
+		if (status > 5)
+			continue;
+
+		if (!g_at_result_iter_next_number(&iter, &type))
+			continue;
+
+		if (!g_at_result_iter_next_number(&iter, &mpty))
+			continue;
+
+		/* skip 'Reserved=0' parameter, only difference from CLCC */
+		if (!g_at_result_iter_skip_next(&iter))
+			continue;
+
+		if (g_at_result_iter_next_string(&iter, &str))
+			g_at_result_iter_next_number(&iter, &number_type);
+
+		call = g_try_new(struct ofono_call, 1);
+		if (call == NULL)
+			break;
+
+		ofono_call_init(call);
+
+		call->id = id;
+		call->direction = dir;
+		call->status = status;
+		call->type = type;
+		strncpy(call->phone_number.number, str,
+				OFONO_MAX_PHONE_NUMBER_LENGTH);
+		call->phone_number.type = number_type;
+
+		if (strlen(call->phone_number.number) > 0)
+			call->clip_validity = 0;
+		else
+			call->clip_validity = 2;
+
+		l = g_slist_insert_sorted(l, call, at_util_call_compare);
+
+		if (mpty)
+			mpty_ids |= 1 << id;
+	}
+
+	if (ret_mpty_ids)
+		*ret_mpty_ids = mpty_ids;
+
+	return l;
+}
+
+static void clcc_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GSList *l;
+
+	if (!ok)
+		return;
+
+	vd->calls = at_util_parse_clcc(result, NULL);
+
+	for (l = vd->calls; l; l = l->next)
+		ofono_voicecall_notify(vc, l->data);
+}
+
+static void slcc_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GSList *calls;
+	GSList *n, *o;
+	struct ofono_call *nc, *oc;
+
+	calls = gemalto_parse_slcc(result, NULL);
+
+	n = calls;
+	o = vd->calls;
+
+	while (n || o) {
+		nc = n ? n->data : NULL;
+		oc = o ? o->data : NULL;
+
+		if (oc && (nc == NULL || (nc->id > oc->id))) {
+			enum ofono_disconnect_reason reason;
+
+			if (vd->local_release & (1 << oc->id))
+				reason = OFONO_DISCONNECT_REASON_LOCAL_HANGUP;
+			else
+				reason = OFONO_DISCONNECT_REASON_REMOTE_HANGUP;
+
+			if (!oc->type)
+				ofono_voicecall_disconnected(vc, oc->id,
+								reason, NULL);
+
+			o = o->next;
+		} else if (nc && (oc == NULL || (nc->id < oc->id))) {
+
+			if (nc->type == 0) /* new call, signal it */
+				ofono_voicecall_notify(vc, nc);
+
+			n = n->next;
+		} else {
+			/*
+			 * Always use the clip_validity from old call
+			 * the only place this is truly told to us is
+			 * in the CLIP notify, the rest are fudged
+			 * anyway.  Useful when RING, CLIP is used,
+			 * and we're forced to use CLCC/SLCC and clip_validity
+			 * is 1
+			 */
+			if (oc->clip_validity == 1)
+				nc->clip_validity = oc->clip_validity;
+
+			/*
+			 * CNAP doesn't arrive as part of CLCC, always
+			 * re-use from the old call
+			 */
+			strncpy(nc->name, oc->name,
+					OFONO_MAX_CALLER_NAME_LENGTH);
+			nc->name[OFONO_MAX_CALLER_NAME_LENGTH] = '\0';
+			nc->cnap_validity = oc->cnap_validity;
+
+			/*
+			 * CDIP doesn't arrive as part of CLCC, always
+			 * re-use from the old call
+			 */
+			memcpy(&nc->called_number, &oc->called_number,
+					sizeof(oc->called_number));
+
+			/*
+			 * If the CLIP is not provided and the CLIP never
+			 * arrives, or RING is used, then signal the call
+			 * here
+			 */
+			if (nc->status == CALL_STATUS_INCOMING &&
+					(vd->flags & FLAG_NEED_CLIP)) {
+				if (nc->type == 0)
+					ofono_voicecall_notify(vc, nc);
+
+				vd->flags &= ~FLAG_NEED_CLIP;
+			} else if (memcmp(nc, oc, sizeof(*nc)) && nc->type == 0)
+				ofono_voicecall_notify(vc, nc);
+
+			n = n->next;
+			o = o->next;
+		}
+	}
+
+	g_slist_free_full(vd->calls, g_free);
+
+	vd->calls = calls;
+
+	vd->local_release = 0;
+}
+
+static void ring_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct ofono_call *call;
+
+	/* See comment in CRING */
+	if (g_slist_find_custom(vd->calls,
+				GINT_TO_POINTER(CALL_STATUS_WAITING),
+				at_util_call_compare_by_status))
+		return;
+
+	/* RING can repeat, ignore if we already have an incoming call */
+	if (g_slist_find_custom(vd->calls,
+				GINT_TO_POINTER(CALL_STATUS_INCOMING),
+				at_util_call_compare_by_status))
+		return;
+
+	/* Generate an incoming call of unknown type */
+	call = create_call(vc, 9, 1, CALL_STATUS_INCOMING, NULL, 128, 2);
+	if (call == NULL) {
+		ofono_error("Couldn't create call!");
+		return;
+	}
+
+	/* We don't know the call type, we must wait for the SLCC URC */
+	vd->flags = FLAG_NEED_CLIP;
+}
+
+static void cring_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GAtResultIter iter;
+	const char *line;
+	int type;
+
+	/* Handle the following situation:
+	 * Active Call + Waiting Call.  Active Call is Released.  The Waiting
+	 * call becomes Incoming and RING/CRING indications are signaled.
+	 * Sometimes these arrive before we get the SLCC URC to find about
+	 * the stage change.  If this happens, simply ignore the RING/CRING
+	 * when a waiting call exists (cannot have waiting + incoming in GSM)
+	 */
+	if (g_slist_find_custom(vd->calls,
+				GINT_TO_POINTER(CALL_STATUS_WAITING),
+				at_util_call_compare_by_status))
+		return;
+
+	/* CRING can repeat, ignore if we already have an incoming call */
+	if (g_slist_find_custom(vd->calls,
+				GINT_TO_POINTER(CALL_STATUS_INCOMING),
+				at_util_call_compare_by_status))
+		return;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CRING:"))
+		return;
+
+	line = g_at_result_iter_raw_line(&iter);
+	if (line == NULL)
+		return;
+
+	/* Ignore everything that is not voice for now */
+	if (!strcasecmp(line, "VOICE"))
+		type = 0;
+	else
+		type = 9;
+
+	/* Generate an incoming call */
+	create_call(vc, type, 1, CALL_STATUS_INCOMING, NULL, 128, 2);
+
+	/* We have a call, and call type but don't know the number and
+	 * must wait for the CLIP to arrive before announcing the call.
+	 * And we wait also for SLCC. If the CLIP arrives
+	 * earlier, we announce the call there
+	 */
+	vd->flags = FLAG_NEED_CLIP;
+
+	DBG("");
+}
+
+static void clip_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GAtResultIter iter;
+	const char *num;
+	int type, validity;
+	GSList *l;
+	struct ofono_call *call;
+
+	l = g_slist_find_custom(vd->calls,
+				GINT_TO_POINTER(CALL_STATUS_INCOMING),
+				at_util_call_compare_by_status);
+	if (l == NULL) {
+		ofono_error("CLIP for unknown call");
+		return;
+	}
+
+	/* We have already saw a CLIP for this call, no need to parse again */
+	if ((vd->flags & FLAG_NEED_CLIP) == 0)
+		return;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CLIP:"))
+		return;
+
+	if (!g_at_result_iter_next_string(&iter, &num))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &type))
+		return;
+
+	if (strlen(num) > 0)
+		validity = CLIP_VALIDITY_VALID;
+	else
+		validity = CLIP_VALIDITY_NOT_AVAILABLE;
+
+	/* Skip subaddr, satype and alpha */
+	g_at_result_iter_skip_next(&iter);
+	g_at_result_iter_skip_next(&iter);
+	g_at_result_iter_skip_next(&iter);
+
+	/* If we have CLI validity field, override our guessed value */
+	g_at_result_iter_next_number(&iter, &validity);
+
+	DBG("%s %d %d", num, type, validity);
+
+	call = l->data;
+
+	strncpy(call->phone_number.number, num,
+		OFONO_MAX_PHONE_NUMBER_LENGTH);
+	call->phone_number.number[OFONO_MAX_PHONE_NUMBER_LENGTH] = '\0';
+	call->phone_number.type = type;
+	call->clip_validity = validity;
+
+	if (call->type == 0)
+		ofono_voicecall_notify(vc, call);
+
+	vd->flags &= ~FLAG_NEED_CLIP;
+}
+
+static int class_to_call_type(int cls)
+{
+	switch (cls) {
+	case 1:
+		return 0;
+	case 4:
+		return 2;
+	case 8:
+		return 9;
+	default:
+		return 1;
+	}
+}
+
+static void ccwa_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GAtResultIter iter;
+	const char *num;
+	int num_type, validity, cls;
+	struct ofono_call *call;
+
+	/* if CCWA is resent, ignore it the second time around */
+	if (g_slist_find_custom(vd->calls,
+				GINT_TO_POINTER(CALL_STATUS_WAITING),
+				at_util_call_compare_by_status))
+		return;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CCWA:"))
+		return;
+
+	if (!g_at_result_iter_next_string(&iter, &num))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &num_type))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &cls))
+		return;
+
+	/* Skip alpha field */
+	g_at_result_iter_skip_next(&iter);
+
+	if (strlen(num) > 0)
+		validity = 0;
+	else
+		validity = 2;
+
+	/* If we have CLI validity field, override our guessed value */
+	g_at_result_iter_next_number(&iter, &validity);
+
+	DBG("%s %d %d %d", num, num_type, cls, validity);
+
+	call = create_call(vc, class_to_call_type(cls), 1, CALL_STATUS_WAITING,
+				num, num_type, validity);
+	if (call == NULL) {
+		ofono_error("Unable to malloc. Call management is fubar");
+		return;
+	}
+
+	if (call->type == 0) /* Only notify voice calls */
+		ofono_voicecall_notify(vc, call);
+}
+
+static void cssi_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	GAtResultIter iter;
+	int code, index;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CSSI:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &code))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &index))
+		index = 0;
+
+	ofono_voicecall_ssn_mo_notify(vc, 0, code, index);
+}
+
+static void cssu_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	GAtResultIter iter;
+	int code;
+	int index;
+	const char *num;
+	struct ofono_phone_number ph;
+
+	ph.number[0] = '\0';
+	ph.type = 129;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CSSU:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &code))
+		return;
+
+	if (!g_at_result_iter_next_number_default(&iter, -1, &index))
+		goto out;
+
+	if (!g_at_result_iter_next_string(&iter, &num))
+		goto out;
+
+	strncpy(ph.number, num, OFONO_MAX_PHONE_NUMBER_LENGTH);
+
+	if (!g_at_result_iter_next_number(&iter, &ph.type))
+		return;
+
+out:
+	ofono_voicecall_ssn_mt_notify(vc, 0, code, index, &ph);
+}
+
+static void gemalto_voicecall_initialized(gboolean ok, GAtResult *result,
+					gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	DBG("voicecall_init: registering to notifications");
+
+	/* NO CARRIER, NO ANSWER, BUSY, NO DIALTONE are handled through SLCC */
+	g_at_chat_register(vd->chat, "^SLCC:", slcc_notify, FALSE, vc, NULL);
+	g_at_chat_register(vd->chat, "RING", ring_notify, FALSE, vc, NULL);
+	g_at_chat_register(vd->chat, "+CRING:", cring_notify, FALSE, vc, NULL);
+	g_at_chat_register(vd->chat, "+CLIP:", clip_notify, FALSE, vc, NULL);
+	g_at_chat_register(vd->chat, "+CCWA:", ccwa_notify, FALSE, vc, NULL);
+	g_at_chat_register(vd->chat, "+CSSI:", cssi_notify, FALSE, vc, NULL);
+	g_at_chat_register(vd->chat, "+CSSU:", cssu_notify, FALSE, vc, NULL);
+
+	ofono_voicecall_register(vc);
+
+	/* Populate the call list */
+	g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix, clcc_cb, vc, NULL);
+}
+
+static int gemalto_voicecall_probe(struct ofono_voicecall *vc,
+					unsigned int vendor, void *data)
+{
+	GAtChat *chat = data;
+	struct voicecall_data *vd;
+
+	vd = g_try_new0(struct voicecall_data, 1);
+
+	if (vd == NULL)
+		return -ENOMEM;
+
+	vd->chat = g_at_chat_clone(chat);
+	vd->vendor = vendor;
+	ofono_voicecall_set_data(vc, vd);
+
+	// TODO: move to a config atom
+	g_at_chat_send(vd->chat, "AT^SNFS=5", NULL, NULL, NULL, NULL);
+
+	g_at_chat_send(vd->chat, "AT+CRC=1", NULL, NULL, NULL, NULL);
+	g_at_chat_send(vd->chat, "AT+COLP=1", NULL, NULL, NULL, NULL);
+	g_at_chat_send(vd->chat, "AT+CLIP=1", NULL, NULL, NULL, NULL);
+	g_at_chat_send(vd->chat, "AT+CCWA=1", NULL, NULL, NULL, NULL);
+	g_at_chat_send(vd->chat, "AT+CSSN=1,1", NULL, NULL, NULL, NULL);
+	g_at_chat_send(vd->chat, "AT^SLCC=1", NULL,
+				gemalto_voicecall_initialized, vc, NULL);
+	return 0;
+}
+
+static void gemalto_voicecall_remove(struct ofono_voicecall *vc)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	ofono_voicecall_set_data(vc, NULL);
+
+	g_at_chat_unref(vd->chat);
+	g_free(vd);
+}
+
+static struct ofono_voicecall_driver driver = {
+	.name			= "gemaltomodem",
+	.probe			= gemalto_voicecall_probe,
+	.remove			= gemalto_voicecall_remove,
+	.dial			= gemalto_dial,
+	.answer			= gemalto_answer,
+	.hangup_all		= gemalto_hangup_all,
+	.hangup_active		= gemalto_hangup,
+	.hold_all_active	= gemalto_hold_all_active,
+	.release_all_held	= gemalto_release_all_held,
+	.set_udub		= gemalto_set_udub,
+	.release_all_active	= gemalto_release_all_active,
+	.release_specific	= gemalto_release_specific,
+	.private_chat		= gemalto_private_chat,
+	.create_multiparty	= gemalto_create_multiparty,
+	.transfer		= gemalto_transfer,
+	.deflect		= NULL,
+	.swap_without_accept	= NULL,
+	.send_tones		= gemalto_send_dtmf
+};
+
+void gemalto_voicecall_init(void)
+{
+	ofono_voicecall_driver_register(&driver);
+}
+
+void gemalto_voicecall_exit(void)
+{
+	ofono_voicecall_driver_unregister(&driver);
+}
diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
index 79793c9..ce33b92 100644
--- a/drivers/mbimmodem/gprs-context.c
+++ b/drivers/mbimmodem/gprs-context.c
@@ -75,6 +75,8 @@ static uint32_t auth_method_to_auth_protocol(enum ofono_gprs_auth_method method)
 		return 2; /* MBIMAuthProtocolChap */
 	case OFONO_GPRS_AUTH_METHOD_PAP:
 		return 1; /* MBIMAuthProtocolPap */
+	default:
+		return 0;
 	}
 
 	return 0;
diff --git a/drivers/mbimmodem/mbim.c b/drivers/mbimmodem/mbim.c
index 9fcf44b..f4132d0 100644
--- a/drivers/mbimmodem/mbim.c
+++ b/drivers/mbimmodem/mbim.c
@@ -781,6 +781,9 @@ static bool open_read_handler(struct l_io *io, void *user_data)
 	/* Grab OPEN_DONE Status field */
 	if (l_get_le32(buf) != 0) {
 		close(fd);
+		if (device->disconnect_handler)
+			device->disconnect_handler(device->ready_data);
+		device->is_ready = false;
 		return false;
 	}
 
diff --git a/drivers/mbimmodem/mbimmodem.c b/drivers/mbimmodem/mbimmodem.c
index a4c9daa..2a01dd6 100644
--- a/drivers/mbimmodem/mbimmodem.c
+++ b/drivers/mbimmodem/mbimmodem.c
@@ -33,7 +33,7 @@ static int mbimmodem_init(void)
 	mbim_devinfo_init();
 	mbim_sim_init();
 	mbim_netreg_init();
-	mbim_sms_exit();
+	mbim_sms_init();
 	mbim_gprs_init();
 	mbim_gprs_context_init();
 	return 0;
diff --git a/drivers/rilmodem/call-forwarding.c b/drivers/rilmodem/call-forwarding.c
index 4aff4d3..0bdab3f 100644
--- a/drivers/rilmodem/call-forwarding.c
+++ b/drivers/rilmodem/call-forwarding.c
@@ -38,7 +38,7 @@
 #include <ofono/call-forwarding.h>
 #include "common.h"
 
-#pragma GCC diagnostic ignored "-Wrestrict"
+//#pragma GCC diagnostic ignored "-Wrestrict"
 
 #include "gril.h"
 
diff --git a/drivers/rilmodem/network-registration.c b/drivers/rilmodem/network-registration.c
index 809b3bc..9895c6d 100644
--- a/drivers/rilmodem/network-registration.c
+++ b/drivers/rilmodem/network-registration.c
@@ -37,7 +37,7 @@
 #include <ofono/modem.h>
 #include <ofono/netreg.h>
 
-#pragma GCC diagnostic ignored "-Wrestrict"
+//#pragma GCC diagnostic ignored "-Wrestrict"
 
 #include <gril/gril.h>
 
-- 
1.9.1

Re: [PATCH 3/7] extended support for LTE and NR. Some minor fixes. part 3 of 7

[Denis Kenzior]

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

Hi Giacinto,

On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:
> ---
>   drivers/atmodem/atutil.h                 |  14 +
>   drivers/atmodem/cbs.c                    |   1 +
>   drivers/atmodem/gprs-context.c           |   2 +
>   drivers/atmodem/gprs.c                   | 554 ++++++++++++++++--
>   drivers/atmodem/lte.c                    | 283 ++++++++-
>   drivers/atmodem/network-registration.c   | 134 +++--
>   drivers/atmodem/sim.c                    |  10 +-
>   drivers/atmodem/sms.c                    |  21 +-
>   drivers/atmodem/vendor.h                 |   1 +
>   drivers/gemaltomodem/gemaltomodem.c      |   5 +-
>   drivers/gemaltomodem/gemaltomodem.h      |  19 +-
>   drivers/gemaltomodem/gprs-context-wwan.c | 445 ++++++++++++++
>   drivers/gemaltomodem/voicecall.c         | 965 +++++++++++++++++++++++++++++++
>   drivers/mbimmodem/gprs-context.c         |   2 +
>   drivers/mbimmodem/mbim.c                 |   3 +
>   drivers/mbimmodem/mbimmodem.c            |   2 +-
>   drivers/rilmodem/call-forwarding.c       |   2 +-
>   drivers/rilmodem/network-registration.c  |   2 +-
>   18 files changed, 2343 insertions(+), 122 deletions(-)
>   create mode 100644 drivers/gemaltomodem/gprs-context-wwan.c
>   create mode 100644 drivers/gemaltomodem/voicecall.c
>

I'm not going to even bother looking at this until it is broken up 
properly and actually has a commit description that is longer than an 
empty string.

Regards,
-Denis

[PATCH 4/7] extended support for LTE and NR. Some minor fixes. part 4 of 7

[Giacinto Cifelli]

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

---
 doc/emergency-call-handling.txt |  2 +-
 doc/lte-api.txt                 | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/doc/emergency-call-handling.txt b/doc/emergency-call-handling.txt
index 69b217d..0436047 100644
--- a/doc/emergency-call-handling.txt
+++ b/doc/emergency-call-handling.txt
@@ -14,7 +14,7 @@ What oFono will do:
 	- Post online atoms will be created.
 	- Upon reception of Dial request, Emergency mode is activated.
 	- Once the call is ended, Emergency mode is deactivated.
-	- Modem remains in online mode with full funcationality.
+	- Modem remains in online mode with full functionality.
 
 Case 2: Call in SIM Present and PIN required state
 
diff --git a/doc/lte-api.txt b/doc/lte-api.txt
index 8a2a97d..22ba7b5 100644
--- a/doc/lte-api.txt
+++ b/doc/lte-api.txt
@@ -25,11 +25,46 @@ Signals		PropertyChanged(string property, variant value)
 			This signal indicates a changed value of the given
 			property.
 
-Properties	string DefaultAccessPointName [readwrite]
+Properties	string Protocol [readwrite]
+
+			Holds the protocol for this context.  Valid values
+			are: "ip", "ipv6" and "dual".
+
+		string AccessPointName [readwrite]
 
 			On LongTermEvolution, contexts activate automatically.
 			This property allows selection of an APN to be used on
 			next automatic activation.
+			By setting this property AT+CGDCONT is sent,
+			followed by AT+CGAUTH if needed.
 
 			Setting this property to an empty string clears the
 			default APN from the modem.
+
+		string AuthenticationMethod [readwrite]
+
+			Sets the Method used for the authentication
+			for the default APN.
+			The authentication information is sent to the module
+			when the AccessPointName property is set
+
+			Available values are "none", "pap" and "chap".
+			Default is "none".
+
+			"pap" and "chap" authentication methods require
+			both Username and Password set to nonempty values
+
+		string Username [readwrite]
+
+			Holds the username to be used for authentication
+			purposes.
+			The authentication information is sent to the module
+			when the AccessPointName property is set
+
+		string Password [readwrite]
+
+			Holds the password to be used for authentication
+			purposes.
+			The authentication information is sent to the module
+			when the AccessPointName property is set
+
-- 
1.9.1

[PATCH 5/7] extended support for LTE and NR. Some minor fixes. part 5 of 7

[Giacinto Cifelli]

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

---
 gatchat/gatmux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gatchat/gatmux.c b/gatchat/gatmux.c
index 9660006..21552d9 100644
--- a/gatchat/gatmux.c
+++ b/gatchat/gatmux.c
@@ -30,8 +30,8 @@
 #include <string.h>
 #include <alloca.h>
 
-#pragma GCC diagnostic ignored "-Wpragmas"
-#pragma GCC diagnostic ignored "-Wcast-function-type"
+//#pragma GCC diagnostic ignored "-Wpragmas"
+//#pragma GCC diagnostic ignored "-Wcast-function-type"
 
 #include <glib.h>
 
-- 
1.9.1

[PATCH 6/7] extended support for LTE and NR. Some minor fixes. part 6 of 7

[Giacinto Cifelli]

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

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

diff --git a/Makefile.am b/Makefile.am
index 6dee4ce..fabda0a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -397,6 +397,8 @@ 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
 
 builtin_modules += xmm7modem
-- 
1.9.1

[PATCH 7/7] extended support for LTE and NR. Some minor fixes. part 7 of 7

[Giacinto Cifelli]

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

---
 AUTHORS | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/AUTHORS b/AUTHORS
index 52f46e9..9488a41 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -137,3 +137,6 @@ Varun Gargi <varun.gargi@intel.com>
 Florent Beillonnet <florent.beillonnet@gmail.com>
 Martin Hundebøll <martin@geanix.com>
 Julien Tournier <tournier.julien@gmail.com>
+Sebastian Arnd <sebastianarnd@gmail.com>
+Giacinto Cifelli <gciofono@gmail.com>
+Martin Baschin <martin.baschin@gemalto.com>
-- 
1.9.1

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Slava Monich]

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

On 19/09/18 08:37, Giacinto Cifelli wrote:
> ---
>   include/gprs-context.h |  1 +
>   include/lte.h          | 11 +++++++++--
>   2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/gprs-context.h b/include/gprs-context.h
> index 20ca9ef..8869c12 100644
> --- a/include/gprs-context.h
> +++ b/include/gprs-context.h
> @@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
>   enum ofono_gprs_auth_method {
>   	OFONO_GPRS_AUTH_METHOD_CHAP = 0,
>   	OFONO_GPRS_AUTH_METHOD_PAP,
> +	OFONO_GPRS_AUTH_METHOD_NONE,

I think there should be OFONO_GPRS_AUTH_METHOD_ANY (or 
OFONO_GPRS_AUTH_METHOD_PAP_CHAP) here as well, for completeness. Many 
modems support that too (and we had to add it in our fork).


>   };
>   
>   struct ofono_gprs_primary_context {
> diff --git a/include/lte.h b/include/lte.h
> index ef84ab9..38587c3 100644
> --- a/include/lte.h
> +++ b/include/lte.h
> @@ -3,6 +3,7 @@
>    *  oFono - Open Source Telephony
>    *
>    *  Copyright (C) 2016  Endocode AG. 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
> @@ -28,14 +29,18 @@ extern "C" {
>   
>   #include <ofono/types.h>
>   
> -struct ofono_lte;
> -
>   struct ofono_lte_default_attach_info {
>   	char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
> +	enum ofono_gprs_proto proto;
> +	enum ofono_gprs_auth_method auth_method;
> +	char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
> +	char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
>   };

This is starting to look suspiciously similar to struct 
ofono_gprs_primary_context (the only thing left is cid). Is it really 
necessary to maintain two copies of essentially the same structure or is 
there some room for unification here?

Cheers,
-Slava

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Giacinto Cifelli]

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

On Wed, Sep 19, 2018 at 10:35 AM Slava Monich <slava.monich@jolla.com>
wrote:

> On 19/09/18 08:37, Giacinto Cifelli wrote:
> > ---
> >   include/gprs-context.h |  1 +
> >   include/lte.h          | 11 +++++++++--
> >   2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/gprs-context.h b/include/gprs-context.h
> > index 20ca9ef..8869c12 100644
> > --- a/include/gprs-context.h
> > +++ b/include/gprs-context.h
> > @@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
> >   enum ofono_gprs_auth_method {
> >       OFONO_GPRS_AUTH_METHOD_CHAP = 0,
> >       OFONO_GPRS_AUTH_METHOD_PAP,
> > +     OFONO_GPRS_AUTH_METHOD_NONE,
>
> I think there should be OFONO_GPRS_AUTH_METHOD_ANY (or
> OFONO_GPRS_AUTH_METHOD_PAP_CHAP) here as well, for completeness. Many
> modems support that too (and we had to add it in our fork).
>
>
I agree. Let me collect all comments, then I will also add it.
I would favour also renumbering with NONE on top, but I am not sure of the
side effects everywhere, in case the values are used directly in commands.


>
> >   };
> >
> >   struct ofono_gprs_primary_context {
> > diff --git a/include/lte.h b/include/lte.h
> > index ef84ab9..38587c3 100644
> > --- a/include/lte.h
> > +++ b/include/lte.h
> > @@ -3,6 +3,7 @@
> >    *  oFono - Open Source Telephony
> >    *
> >    *  Copyright (C) 2016  Endocode AG. 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
> > @@ -28,14 +29,18 @@ extern "C" {
> >
> >   #include <ofono/types.h>
> >
> > -struct ofono_lte;
> > -
> >   struct ofono_lte_default_attach_info {
> >       char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
> > +     enum ofono_gprs_proto proto;
> > +     enum ofono_gprs_auth_method auth_method;
> > +     char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
> > +     char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
> >   };
>
> This is starting to look suspiciously similar to struct
> ofono_gprs_primary_context (the only thing left is cid). Is it really
> necessary to maintain two copies of essentially the same structure or is
> there some room for unification here?
>
>
There is a lot of duplication between gprs and lte, yes, if you look in the
rest of the patches there is a lot in common.
I havent found a way to do it smoothly. I even had to copy the
gprs_proto_to/from_string, gprs_auth_method_to/from_string in src/gprs.c in
src/lte.c, because I could not export them in common.c (without removing
the enums).
If there is a smart way to do it, I am more than willing to recraft the
code. Like this every change is to be duplicated.


> Cheers,
> -Slava
>

Regards,
Giacinto

_______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono
>

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

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Denis Kenzior]

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

Hi Giacinto,

> I would favour also renumbering with NONE on top, but I am not sure of 
> the side effects everywhere, in case the values are used directly in 
> commands.

Actually there is no problem doing that.  The internal API is not 
stable.  Besides, it will give all the guys who insist on maintaining 
out-of-tree plugins some extra work ;)

Regards,
-Denis

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Slava Monich]

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

On 19/09/18 18:21, Denis Kenzior wrote:

> Hi Giacinto,
>
>> I would favour also renumbering with NONE on top, but I am not sure 
>> of the side effects everywhere, in case the values are used directly 
>> in commands.
>
> Actually there is no problem doing that.  The internal API is not 
> stable.  Besides, it will give all the guys who insist on maintaining 
> out-of-tree plugins some extra work ;)
>

Anything in include is external API. We do maintain not just 
out-of-tree, but binary plugins. Backward (binary!) compatibility a must 
for us. Please do not break it.

-Slava

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Denis Kenzior]

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

Hi Slava,

> Anything in include is external API. We do maintain not just 
> out-of-tree, but binary plugins. Backward (binary!) compatibility a must 
> for us. Please do not break it.

That is why we have 'OFONO_API_SUBJECT_TO_CHANGE' as a reminder.  We 
mean it.  D-Bus API is stable, we never made any guarantees about the 
internal APIs.

Regards,
-Denis

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Slava Monich]

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

On 19/09/18 19:32, Denis Kenzior wrote:
> Hi Slava,
>
>> Anything in include is external API. We do maintain not just 
>> out-of-tree, but binary plugins. Backward (binary!) compatibility a 
>> must for us. Please do not break it.
>
> That is why we have 'OFONO_API_SUBJECT_TO_CHANGE' as a reminder. We 
> mean it.  D-Bus API is stable, we never made any guarantees about the 
> internal APIs.
>

I understand that! It makes things easier for you guys.

But we had to avoid certain compile-time dependencies in ofono, and a 
straightforward (and perhaps the only) way to achieve that was to use 
binary plugins. For us plugin API is not subject to change (plugins 
don't necessarily get upgraded together with ofono), meaning more 
changes between our fork and upstream in case if upstream breaks it, 
more maintenance work and more room for errors. Obviously, I would like 
to keep differences to a minimum.

I'm just humbly asking - if there's a way to keep plugin API backward 
compatible, please do it that way. There is at least one person in the 
world who would appreciate it.

Cheers,
-Slava

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Giacinto Cifelli]

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

On Wed, Sep 19, 2018 at 6:54 PM Slava Monich <slava.monich@jolla.com> wrote:

> On 19/09/18 19:32, Denis Kenzior wrote:
> > Hi Slava,
> >
> >> Anything in include is external API. We do maintain not just
> >> out-of-tree, but binary plugins. Backward (binary!) compatibility a
> >> must for us. Please do not break it.
> >
> > That is why we have 'OFONO_API_SUBJECT_TO_CHANGE' as a reminder. We
> > mean it.  D-Bus API is stable, we never made any guarantees about the
> > internal APIs.
> >
>
> I understand that! It makes things easier for you guys.
>
> But we had to avoid certain compile-time dependencies in ofono, and a
> straightforward (and perhaps the only) way to achieve that was to use
> binary plugins. For us plugin API is not subject to change (plugins
> don't necessarily get upgraded together with ofono), meaning more
> changes between our fork and upstream in case if upstream breaks it,
> more maintenance work and more room for errors. Obviously, I would like
> to keep differences to a minimum.
>
> I'm just humbly asking - if there's a way to keep plugin API backward
> compatible, please do it that way. There is at least one person in the
> world who would appreciate it.
>
>
Hi Slava,

this special case will not be backward compatible.
This enum for the authentication methods is handled through switch/cases in
the code, and some of them miss default. so an additional value will give
an unpredictable behavior (depending also on how the switch has been
optimized by the compiler).

Do you think it still makes sense to add an OFONO_GPRS_AUTH_METHOD_ANY ?
It looks like Denis will not take it anyway if it is not explained clearly
what it is for and how it is used. I have to say, I cannot see its benefit
either.


> Cheers,
> -Slava
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono


BR,
Giacinto

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

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Slava Monich]

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

On 19/09/18 19:54, Slava Monich wrote:
> On 19/09/18 19:32, Denis Kenzior wrote:
>> Hi Slava,
>>
>>> Anything in include is external API. We do maintain not just 
>>> out-of-tree, but binary plugins. Backward (binary!) compatibility a 
>>> must for us. Please do not break it.
>>
>> That is why we have 'OFONO_API_SUBJECT_TO_CHANGE' as a reminder. We 
>> mean it.  D-Bus API is stable, we never made any guarantees about the 
>> internal APIs.
>>
>
> I understand that! It makes things easier for you guys.
>
> But we had to avoid certain compile-time dependencies in ofono, and a 
> straightforward (and perhaps the only) way to achieve that was to use 
> binary plugins. For us plugin API is not subject to change (plugins 
> don't necessarily get upgraded together with ofono), meaning more 
> changes between our fork and upstream in case if upstream breaks it, 
> more maintenance work and more room for errors. Obviously, I would 
> like to keep differences to a minimum.
>
> I'm just humbly asking - if there's a way to keep plugin API backward 
> compatible, please do it that way. There is at least one person in the 
> world who would appreciate it.
>

Actually, in our fork we have already modified enum 
ofono_gprs_auth_method and that's what we have there:

enum ofono_gprs_auth_method {
     OFONO_GPRS_AUTH_METHOD_ANY = 0,
     OFONO_GPRS_AUTH_METHOD_NONE,
     OFONO_GPRS_AUTH_METHOD_CHAP,
     OFONO_GPRS_AUTH_METHOD_PAP,
};

(ANY = PAP_CHAP, and don't ask me why we added new values to the 
beginning of the enum - it was before we started using binary plugins). 
I would be more than happy if upstream started to use the same enum!

Cheers,
-Slava

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Denis Kenzior]

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

Hi Slava,

>> I understand that! It makes things easier for you guys.
>>
>> But we had to avoid certain compile-time dependencies in ofono, and a 
>> straightforward (and perhaps the only) way to achieve that was to use 
>> binary plugins. For us plugin API is not subject to change (plugins 
>> don't necessarily get upgraded together with ofono), meaning more 
>> changes between our fork and upstream in case if upstream breaks it, 
>> more maintenance work and more room for errors. Obviously, I would 
>> like to keep differences to a minimum.

So I sympathize, but really it might also be a hint to finally start 
getting things upstream.

>>
>> I'm just humbly asking - if there's a way to keep plugin API backward 
>> compatible, please do it that way. There is at least one person in the 
>> world who would appreciate it.
>>

The problem is, the next time this comes up there may be no way to avoid 
it...  Or we break binary compatibility inadvertently.  It is just not 
something we're going to be looking out for.

> 
> Actually, in our fork we have already modified enum 
> ofono_gprs_auth_method and that's what we have there:
> 
> enum ofono_gprs_auth_method {
>      OFONO_GPRS_AUTH_METHOD_ANY = 0,
>      OFONO_GPRS_AUTH_METHOD_NONE,
>      OFONO_GPRS_AUTH_METHOD_CHAP,
>      OFONO_GPRS_AUTH_METHOD_PAP,
> };

So you already made life very hard for yourself ;)

> 
> (ANY = PAP_CHAP, and don't ask me why we added new values to the 
> beginning of the enum - it was before we started using binary plugins). 
> I would be more than happy if upstream started to use the same enum!
> 

That assumes that we should support your METHOD_ANY thing.  I've not 
heard any good arguments for that yet...

Regards,
-Denis

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Slava Monich]

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

On 20/09/18 00:45, Denis Kenzior wrote:
>> (ANY = PAP_CHAP, and don't ask me why we added new values to the 
>> beginning of the enum - it was before we started using binary 
>> plugins). I would be more than happy if upstream started to use the 
>> same enum!
>>
>
> That assumes that we should support your METHOD_ANY thing.  I've not 
> heard any good arguments for that yet...

At least one Chinese modem I dealt with had AT+ EGPAU = <op>,<cid>, 
<proto> where <proto> is 0 for PAP, 1 for CHAP, 2 for NONE and 3 for 
PAP_CHAP. Also, Android RIL interface has value 3 reserved for PAP&CHAP 
(see ril.h for older Androids and ApnAuthType in binder radio interface 
for Android 8+). To me that sounds like a valid use case.

-Slava

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Denis Kenzior]

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

Hi Slava,

On 09/19/2018 06:14 PM, Slava Monich wrote:
> On 20/09/18 00:45, Denis Kenzior wrote:
>>> (ANY = PAP_CHAP, and don't ask me why we added new values to the 
>>> beginning of the enum - it was before we started using binary 
>>> plugins). I would be more than happy if upstream started to use the 
>>> same enum!
>>>
>>
>> That assumes that we should support your METHOD_ANY thing.  I've not 
>> heard any good arguments for that yet...
> 
> At least one Chinese modem I dealt with had AT+ EGPAU = <op>,<cid>, 
> <proto> where <proto> is 0 for PAP, 1 for CHAP, 2 for NONE and 3 for 
> PAP_CHAP. Also, Android RIL interface has value 3 reserved for PAP&CHAP 
> (see ril.h for older Androids and ApnAuthType in binder radio interface 
> for Android 8+). To me that sounds like a valid use case.

So I went in and checked (someone can correct me if I'm wrong)

- The three-four generations of Intel modems I checked do not support this
- QMI doesn't support this
- MBIM doesn't support this (but supports MSChap/MSChapV2, funnily enough)
- 3GPP doesn't support this
- Telit doesn't support this
- Model Broadband Provider Info doesn't support this

Are you sure it isn't some weird vendor feature where they try all 
possibilities one at a time?  E.g. it tries CHAP first, then tries PAP, 
then tries None?

And how do you plan on supporting this on modems that physically do not 
support such an enumeration?  What would be the exact semantics?  Is 
this modem supported by oFono upstream?

This conversation is moot until you introduce support for such a modem 
or more specific information is introduced.  And until / unless this 
happens, we're not introducing random enumerations into the D-Bus API 
that we have to live with for a long time.

Regards,
-Denis

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Denis Kenzior]

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

Hi Slava,

On 09/19/2018 03:35 AM, Slava Monich wrote:
> On 19/09/18 08:37, Giacinto Cifelli wrote:
>> ---
>>   include/gprs-context.h |  1 +
>>   include/lte.h          | 11 +++++++++--
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/gprs-context.h b/include/gprs-context.h
>> index 20ca9ef..8869c12 100644
>> --- a/include/gprs-context.h
>> +++ b/include/gprs-context.h
>> @@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
>>   enum ofono_gprs_auth_method {
>>       OFONO_GPRS_AUTH_METHOD_CHAP = 0,
>>       OFONO_GPRS_AUTH_METHOD_PAP,
>> +    OFONO_GPRS_AUTH_METHOD_NONE,
> 
> I think there should be OFONO_GPRS_AUTH_METHOD_ANY (or 
> OFONO_GPRS_AUTH_METHOD_PAP_CHAP) here as well, for completeness. Many 
> modems support that too (and we had to add it in our fork).

There is no such thing in 3GPP, so how does that work?

And by the way,  none of the provisioning databases I'm aware of have 
this possibility and CHAP is actually a sane default.  While there are 
probably 2 providers left on this planet that might still insist on PAP, 
the 3GPP specs actually mandate CHAP anyway.

> 
> 
>>   };
>>   struct ofono_gprs_primary_context {
>> diff --git a/include/lte.h b/include/lte.h
>> index ef84ab9..38587c3 100644
>> --- a/include/lte.h
>> +++ b/include/lte.h
>> @@ -3,6 +3,7 @@
>>    *  oFono - Open Source Telephony
>>    *
>>    *  Copyright (C) 2016  Endocode AG. 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
>> @@ -28,14 +29,18 @@ extern "C" {
>>   #include <ofono/types.h>
>> -struct ofono_lte;
>> -
>>   struct ofono_lte_default_attach_info {
>>       char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
>> +    enum ofono_gprs_proto proto;
>> +    enum ofono_gprs_auth_method auth_method;
>> +    char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
>> +    char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
>>   };
> 
> This is starting to look suspiciously similar to struct 
> ofono_gprs_primary_context (the only thing left is cid). Is it really 
> necessary to maintain two copies of essentially the same structure or is 
> there some room for unification here?
> 

I don't really see a problem here.  One is active context details, the 
other is default attach details.  But if OFONO_GPRS_* defines are going 
to be used in multiple include/ofono files, then these defines should be 
moved to types.h.

Regards,
-Denis

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Denis Kenzior]

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

Hi Giacinto,

Your patch series has 7 commits with the same header.  That is nonsense.
Each commit header should be specific to what is contained inside and 
preferably followed by a commit description.

Read any one of the many 'How to submit a kernel patch' howtos in order 
to understand the best practices here.  oFono does not use Signed-off-by 
lines and a few things are different, but the commit header/description 
requirements and the overall patch submission process are the same.

On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:
> ---
>   include/gprs-context.h |  1 +
>   include/lte.h          | 11 +++++++++--
>   2 files changed, 10 insertions(+), 2 deletions(-)

This really should be two patches because you're changing unrelated things.

> 
> diff --git a/include/gprs-context.h b/include/gprs-context.h
> index 20ca9ef..8869c12 100644
> --- a/include/gprs-context.h
> +++ b/include/gprs-context.h
> @@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
>   enum ofono_gprs_auth_method {
>   	OFONO_GPRS_AUTH_METHOD_CHAP = 0,
>   	OFONO_GPRS_AUTH_METHOD_PAP,
> +	OFONO_GPRS_AUTH_METHOD_NONE,

So strictly speaking we already support NONE as a method if username is 
empty. I don't have a problem with this change, but it does imply that 
you need to fix up the existing code depending on this enumeration to 
behave properly.

>   };
>   
>   struct ofono_gprs_primary_context {
> diff --git a/include/lte.h b/include/lte.h
> index ef84ab9..38587c3 100644
> --- a/include/lte.h
> +++ b/include/lte.h
> @@ -3,6 +3,7 @@
>    *  oFono - Open Source Telephony
>    *
>    *  Copyright (C) 2016  Endocode AG. 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
> @@ -28,14 +29,18 @@ extern "C" {
>   
>   #include <ofono/types.h>
>   
> -struct ofono_lte;
> -

So why are you changing the order seemingly randomly?  And anyway, this 
is wrong.  See doc/coding-style.txt item M9.

>   struct ofono_lte_default_attach_info {
>   	char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
> +	enum ofono_gprs_proto proto;
> +	enum ofono_gprs_auth_method auth_method;
> +	char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
> +	char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];

Okay, but you might want to start at the D-Bus API level first...

>   };
>   
>   typedef void (*ofono_lte_cb_t)(const struct ofono_error *error, void *data);
>   
> +struct ofono_lte;
> +
>   struct ofono_lte_driver {
>   	const char *name;
>   	int (*probe)(struct ofono_lte *lte, unsigned int vendor, void *data);
> @@ -61,6 +66,8 @@ void ofono_lte_set_data(struct ofono_lte *lte, void *data);
>   
>   void *ofono_lte_get_data(const struct ofono_lte *lte);
>   
> +struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte);
> + >   #ifdef __cplusplus
>   }
>   #endif
> 

Regards,
-Denis

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Giacinto Cifelli]

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

On Wed, Sep 19, 2018 at 4:09 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> Your patch series has 7 commits with the same header.  That is nonsense.
> Each commit header should be specific to what is contained inside and
> preferably followed by a commit description.
>
> Read any one of the many 'How to submit a kernel patch' howtos in order
> to understand the best practices here.  oFono does not use Signed-off-by
> lines and a few things are different, but the commit header/description
> requirements and the overall patch submission process are the same.
>

Hi Denis,

I will break down things further. This series of patches is only the first
one to support our modules.
I have submitted this first part also to gather comments.
From the ./HACKING file I understood that the requirement is to
split according to the top-level directories only, and to make sure not to
break compilation.
Definitely I will add more comments to the commits, too.


>
> On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:
> > ---
> >   include/gprs-context.h |  1 +
> >   include/lte.h          | 11 +++++++++--
> >   2 files changed, 10 insertions(+), 2 deletions(-)
>
> This really should be two patches because you're changing unrelated things.
>

well, this is the interesting part of this series of patches.
What the lte atom does is really parallel to the gprs-context, at the end
is a big code duplication.
Isn't there a smart way to reduce this?


> >
> > diff --git a/include/gprs-context.h b/include/gprs-context.h
> > index 20ca9ef..8869c12 100644
> > --- a/include/gprs-context.h
> > +++ b/include/gprs-context.h
> > @@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
> >   enum ofono_gprs_auth_method {
> >       OFONO_GPRS_AUTH_METHOD_CHAP = 0,
> >       OFONO_GPRS_AUTH_METHOD_PAP,
> > +     OFONO_GPRS_AUTH_METHOD_NONE,
>
> So strictly speaking we already support NONE as a method if username is
> empty. I don't have a problem with this change, but it does imply that
> you need to fix up the existing code depending on this enumeration to
> behave properly.
>

already fixed in the rest of this patch series. But this implies that I
will have to submit again a multi-part patch for this change.


> >   };
> >
> >   struct ofono_gprs_primary_context {
> > diff --git a/include/lte.h b/include/lte.h
> > index ef84ab9..38587c3 100644
> > --- a/include/lte.h
> > +++ b/include/lte.h
> > @@ -3,6 +3,7 @@
> >    *  oFono - Open Source Telephony
> >    *
> >    *  Copyright (C) 2016  Endocode AG. 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
> > @@ -28,14 +29,18 @@ extern "C" {
> >
> >   #include <ofono/types.h>
> >
> > -struct ofono_lte;
> > -
>
> So why are you changing the order seemingly randomly?  And anyway, this
> is wrong.  See doc/coding-style.txt item M9.
>
> >   struct ofono_lte_default_attach_info {
> >       char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
> > +     enum ofono_gprs_proto proto;
> > +     enum ofono_gprs_auth_method auth_method;
> > +     char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
> > +     char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
>
> Okay, but you might want to start at the D-Bus API level first...
>

this means submitting a patch for the D-Bus API before the others ?
In any case it has to go with the rest if it has to compile.
I still haven't figured out completely how to split properly to ensure
compilation.


> >   };
> >
> >   typedef void (*ofono_lte_cb_t)(const struct ofono_error *error, void
> *data);
> >
> > +struct ofono_lte;
> > +
> >   struct ofono_lte_driver {
> >       const char *name;
> >       int (*probe)(struct ofono_lte *lte, unsigned int vendor, void
> *data);
> > @@ -61,6 +66,8 @@ void ofono_lte_set_data(struct ofono_lte *lte, void
> *data);
> >
> >   void *ofono_lte_get_data(const struct ofono_lte *lte);
> >
> > +struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte);
> > + >   #ifdef __cplusplus
> >   }
> >   #endif
> >
>
> Regards,
> -Denis
>

Regards,
Giacinto

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

Re: [PATCH 1/7] extended support for LTE and NR. Some minor fixes. part 1 of 7

[Denis Kenzior]

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

Hi Giacinto,

> From the ./HACKING file I understood that the requirement is to
> split according to the top-level directories only, and to make sure not 
> to break compilation.

The rule provides a baseline minimum.  It also implies that sometimes 
breaking compilation is inevitable.  So long as the entire series is 
results in a clean build, that is fine.  Other projects are stricter 
here in order to not break 'git bisect' functionality.  But we are not.

> Definitely I will add more comments to the commits, too.

Don't be afraid to do that.  The more commits and the smaller the 
patches, the easier it is to review things.  In fact, as a rule of 
thumb, it is much faster to get 10 small patches upstream than 1 big patch.

> well, this is the interesting part of this series of patches.
> What the lte atom does is really parallel to the gprs-context, at the 
> end is a big code duplication.

I see them as related but the gprs change itself can stand on its own. 
Also much of the duplicated code can be factored out into common.c

A good rule of thumb is to ask yourself this question: "Are these 
changes useful just by themselves?"  If the answer is yes, then you 
should separate them into a distinct commit.  And this is part of the 
reason why it is easier to get upstream when it is broken down into 
smaller chunks.  Even if some part of the series is controversial, the 
other parts might not be and could easily be applied right away.

> Isn't there a smart way to reduce this?

Are you referring to the number of commits? Why would you want to?

... or code duplication?  In which case yes, you need to factor things 
out into common.c.

> 
> already fixed in the rest of this patch series. But this implies that I 
> will have to submit again a multi-part patch for this change.

Which is why a few days ago I suggested you start with a small subset to 
learn the process.  It is far easier to re-factor smaller chunks than 
3-4k of code.

> 
> this means submitting a patch for the D-Bus API before the others ?

Yes, preferably.

> In any case it has to go with the rest if it has to compile.
> I still haven't figured out completely how to split properly to ensure 
> compilation.
> 

Browse git history and see how this is done if you're not sure.  Our git 
history tends to be quite clean.  But in general, this is a practical 
skill and like anything will take some time to get comfortable with.

Regards,
-Denis