[PATCH v7 0/7] lte atom auth and IP protocol

[PATCH v7 0/7] lte atom auth and IP protocol

[Giacinto Cifelli]

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

 would like to submit this patch for the lte atom

This patch adds in core atom:
-  the protocol for the default LTE APN, ip, ipv6 and both
- the authentication handling, with 3 properties:
	method, username, password

then these are used in two atoms:
- atmodem, with standard AT commands syntax
- gemaltomodem, with vendor-specific syntax

The behavior of the patch is described in the api document (part 1),
and in the core atom header (part 2).

Related to the previous version, this version:
- introduces a separate atom for Gemalto vendor-specific syntax
- implements in the core the suggestions of Denis

history of versions fixes:
- a missing return in case of error in at_lte_set_default_attach_info_cb
- discards the use of lte_driver_data in favour of its member chat
	for passing among callbacks, as the persistence of the first
	cannot be guaranteed, or at least controlled by the driver atom
- fixes the memory management of the object passed among callbacks
	by using a reference counted structure


Giacinto Cifelli (7):
  lte-api: protocol and authentication properties
  lte.h: added proto and authentication handling
  lte: protocol and authentication for default ctx
  atmodem/atutil: shared functions for cgdcont
  atmodem/lte: proto and authentication handling
  gemalto/lte: Gemalto vendor lte atom
  plugins/gemalto: use Gemalto lte atom

 Makefile.am                         |   5 +-
 doc/lte-api.txt                     |  39 +++++
 drivers/atmodem/atutil.c            |  49 ++++++
 drivers/atmodem/atutil.h            |   6 +
 drivers/atmodem/lte.c               | 119 ++++++++++++---
 drivers/gemaltomodem/gemaltomodem.c |   2 +
 drivers/gemaltomodem/gemaltomodem.h |   4 +
 drivers/gemaltomodem/gemaltoutil.c  | 106 +++++++++++++
 drivers/gemaltomodem/gemaltoutil.h  |  31 ++++
 drivers/gemaltomodem/lte.c          | 221 +++++++++++++++++++++++++++
 include/lte.h                       |   5 +
 plugins/gemalto.c                   |  16 +-
 src/lte.c                           | 222 +++++++++++++++++++++-------
 13 files changed, 749 insertions(+), 76 deletions(-)
 create mode 100644 drivers/gemaltomodem/gemaltoutil.c
 create mode 100644 drivers/gemaltomodem/gemaltoutil.h
 create mode 100644 drivers/gemaltomodem/lte.c

-- 
2.17.1

[PATCH v7 1/7] lte-api: protocol and authentication properties

[Giacinto Cifelli]

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

added 4 properties for handling the type of context and the
authentication method, exactly like in any gprs context handling.
The properties are named after the equivalent gprs-context one, for
compatibility and uniformity.

Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
---
 doc/lte-api.txt | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/doc/lte-api.txt b/doc/lte-api.txt
index 8a2a97d9..c9544f60 100644
--- a/doc/lte-api.txt
+++ b/doc/lte-api.txt
@@ -33,3 +33,42 @@ Properties	string DefaultAccessPointName [readwrite]
 
 			Setting this property to an empty string clears the
 			default APN from the modem.
+
+		string Protocol [readwrite]
+
+			Holds the protocol for this context.  Valid values
+			are: "ip", "ipv6" and "dual". Default value is "ip".
+
+		string AuthenticationMethod [readwrite]
+
+			Sets the Method used for the authentication
+			for the default APN.
+
+			Available values are "none", "pap" and "chap".
+			Default is "none".
+
+			If the AuthenticationMethod is set to 'none' it remove
+			the authentication for the DefaultAPN.
+			In case of AuthenticationMethod 'none',
+			if the Username and Password properties are not empty,
+			the values are preserved in the properties, but they
+			are not used or transmitted to the module.
+			Conversely, if Username or Password are empty, the
+			authentication method selected internally is 'none',
+			but the property AuthenticationMethod is left unchanged.
+
+			If the default APN supports authentication and it
+			fails, then it is up to the network how to proceed.
+			In general LTE access is denied and the modem can
+			fallback to a legacy technology if capable and another
+			radio technology is available.
+
+		string Username [readwrite]
+
+			Holds the username to be used for authentication
+			purposes.
+
+		string Password [readwrite]
+
+			Holds the password to be used for authentication
+			purposes.
-- 
2.17.1

[PATCH v7 2/7] lte.h: added proto and authentication handling

[Giacinto Cifelli]

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

The ofono_lte_default_attach_info is extended with protocol,
authentication method, username and password.
The transmission of this info from the src to the atom happens
through the existing set_default_attach_info.
A signal is emitted when one of these properties changes

Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
---
 include/lte.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/lte.h b/include/lte.h
index 0f2501c0..2f12ac29 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
@@ -32,6 +33,10 @@ 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);
-- 
2.17.1

[PATCH v7 3/7] lte: protocol and authentication for default ctx

[Giacinto Cifelli]

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

Many LTE networks require user authentication, even for the default
context. In particular, most of the private APNs use this facility
to add some control on top of the MNO providing the service, so that
another user of the same network cannot access the private one.
As such, we add these parameters to the default context
settings that will attempt to use when registering to the network.

The additional parameters added by this patch are:  protocol, user, and
password.  These are sufficient to allow to connect to networks
available to the patch author where ofono previously failed to register
to the network at all.

Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
Co-authored-by: Denis Kenzior <denis.kenzior@intel.com>
---
 src/lte.c | 222 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 167 insertions(+), 55 deletions(-)

diff --git a/src/lte.c b/src/lte.c
index 23fe8e1c..fc0a3442 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 "DefaultAccessPointName"
+#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;
@@ -57,6 +62,10 @@ static GSList *g_drivers = NULL;
 static void lte_load_settings(struct ofono_lte *lte)
 {
 	char *apn;
+	char *proto_str;
+	char *auth_method_str;
+	char *username;
+	char *password;
 
 	if (lte->imsi == NULL)
 		return;
@@ -69,19 +78,57 @@ 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) {
+	apn = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_APN, NULL);
+	proto_str = g_key_file_get_string(lte->settings, SETTINGS_GROUP,
+				LTE_PROTO, 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 (apn && is_valid_apn(apn))
 		strcpy(lte->info.apn, apn);
-		g_free(apn);
-	}
+
+	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 (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 && strlen(username) <= OFONO_GPRS_MAX_USERNAME_LENGTH)
+		strcpy(lte->info.username, username);
+
+	if (password && strlen(password) <= OFONO_GPRS_MAX_PASSWORD_LENGTH)
+		strcpy(lte->info.password, password);
+
+	g_free(apn);
+	g_free(proto_str);
+	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;
@@ -95,20 +142,31 @@ static DBusMessage *lte_get_properties(DBusConnection *conn,
 	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_APN, DBUS_TYPE_STRING, &apn);
+	ofono_dbus_dict_append(&dict, LTE_PROTO, DBUS_TYPE_STRING, &proto);
+	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;
+	char *key;
+	char *value;
+	const char *str;
+	DBusMessageIter iter;
+	DBusMessageIter var;
 
 	DBG("%s error %d", path, error->type);
 
@@ -118,55 +176,48 @@ static void lte_set_default_attach_info_cb(const struct ofono_error *error,
 		return;
 	}
 
-	g_strlcpy(lte->info.apn, lte->pending_info.apn,
-			OFONO_GPRS_MAX_APN_LENGTH + 1);
+	/*
+	 * Reparsing of the message to extract the key and value
+	 * No error checking needed since we already validated pending
+	 */
+	dbus_message_iter_init(lte->pending, &iter);
+	dbus_message_iter_get_basic(&iter, &str);
+	key = strdup(str);
+
+	dbus_message_iter_next(&iter);
+	dbus_message_iter_recurse(&iter, &var);
+	dbus_message_iter_get_basic(&var, &str);
+	value = strdup(str);
+
+	memcpy(&lte->info, &lte->pending_info, sizeof(lte->info));
+
+	reply = dbus_message_new_method_return(lte->pending);
+	__ofono_dbus_pending_reply(&lte->pending, reply);
 
 	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);
+		/*
+		 * the following code removes from storage empty APN, user, pwd
+		 * for proto and auth_method, given that they always
+		 * have defaults, it will not do anything.
+		 */
+		if (!*value)
+			/* Clear entry on empty string. */
+			g_key_file_remove_key(lte->settings,
+				SETTINGS_GROUP, key, NULL);
 		else
-			g_key_file_set_string(lte->settings, SETTINGS_GROUP,
-						DEFAULT_APN_KEY, lte->info.apn);
+			g_key_file_set_string(lte->settings,
+				SETTINGS_GROUP, key, value);
 
 		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);
-}
-
-static DBusMessage *lte_set_default_apn(struct ofono_lte *lte,
-				DBusConnection *conn, DBusMessage *msg,
-				const char *apn)
-{
-	if (lte->driver->set_default_attach_info == NULL)
-		return __ofono_error_not_implemented(msg);
+					key,
+					DBUS_TYPE_STRING, &value);
 
-	if (lte->pending)
-		return __ofono_error_busy(msg);
-
-	if (g_str_equal(apn, lte->info.apn))
-		return dbus_message_new_method_return(msg);
-
-	/* 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->pending_info.apn, apn, OFONO_GPRS_MAX_APN_LENGTH + 1);
-
-	lte->driver->set_default_attach_info(lte, &lte->pending_info,
-					lte_set_default_attach_info_cb, lte);
-
-	return NULL;
+	g_free(value);
+	g_free(key);
 }
 
 static DBusMessage *lte_set_property(DBusConnection *conn,
@@ -177,6 +228,14 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
 	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,16 +251,69 @@ static DBusMessage *lte_set_property(DBusConnection *conn,
 
 	dbus_message_iter_recurse(&iter, &var);
 
-	if (!strcmp(property, DEFAULT_APN_KEY)) {
-		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
-			return __ofono_error_invalid_args(msg);
+	if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
 
-		dbus_message_iter_get_basic(&var, &str);
+	dbus_message_iter_get_basic(&var, &str);
 
-		return lte_set_default_apn(lte, conn, msg, str);
-	}
+	memcpy(&lte->pending_info, &lte->info, sizeof(lte->info));
+
+	if ((strcmp(property, LTE_APN) == 0)) {
+
+		if (g_str_equal(str, lte->info.apn))
+			return dbus_message_new_method_return(msg);
+
+		/* We do care about empty value: it can be used for reset. */
+		if (is_valid_apn(str) == FALSE && str[0] != '\0')
+			return __ofono_error_invalid_format(msg);
+
+		g_strlcpy(lte->pending_info.apn, str,
+					OFONO_GPRS_MAX_APN_LENGTH + 1);
+
+	} else if ((strcmp(property, LTE_PROTO) == 0)) {
+
+		if (!gprs_proto_from_string(str, &proto))
+			return __ofono_error_invalid_format(msg);
+
+		if (proto == lte->info.proto)
+			return dbus_message_new_method_return(msg);
 
-	return __ofono_error_invalid_args(msg);
+		lte->pending_info.proto = proto;
+
+	} else if (strcmp(property, LTE_AUTH_METHOD) == 0) {
+
+		if (!gprs_auth_method_from_string(str, &auth_method))
+			return __ofono_error_invalid_format(msg);
+
+		if (auth_method == lte->info.auth_method)
+			return dbus_message_new_method_return(msg);
+
+		lte->pending_info.auth_method = auth_method;
+
+	} else if (strcmp(property, LTE_USERNAME) == 0) {
+
+		if (g_str_equal(str, lte->info.username))
+			return dbus_message_new_method_return(msg);
+
+		g_strlcpy(lte->pending_info.username, str,
+					OFONO_GPRS_MAX_USERNAME_LENGTH + 1);
+
+	} else if (strcmp(property, LTE_PASSWORD) == 0) {
+
+		if (g_str_equal(str, lte->info.password))
+			return dbus_message_new_method_return(msg);
+
+		g_strlcpy(lte->pending_info.password, str,
+					OFONO_GPRS_MAX_PASSWORD_LENGTH + 1);
+
+	} else
+		return __ofono_error_invalid_args(msg);
+
+	lte->pending = dbus_message_ref(msg);
+	lte->driver->set_default_attach_info(lte, &lte->pending_info,
+					lte_set_default_attach_info_cb, lte);
+
+	return NULL;
 }
 
 static const GDBusMethodTable lte_methods[] = {
-- 
2.17.1

[PATCH v7 4/7] atmodem/atutil: shared functions for cgdcont

[Giacinto Cifelli]

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

included two new functions:

at_get_auth_type: converts the ofono internal ofono_gprs_auth_method
	into the value of the 3GPP 27.007 to pass to the AT command

at_get_cgdcont_command: computes the AT+CGDCONT string, standard version
---
 drivers/atmodem/atutil.c | 49 ++++++++++++++++++++++++++++++++++++++++
 drivers/atmodem/atutil.h |  6 +++++
 2 files changed, 55 insertions(+)

diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
index 6f4e8a20..dc1ef3d3 100644
--- a/drivers/atmodem/atutil.c
+++ b/drivers/atmodem/atutil.c
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2008-2011  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
@@ -27,6 +28,7 @@
 #include <gatchat.h>
 #include <string.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <errno.h>
 
 #define OFONO_API_SUBJECT_TO_CHANGE
@@ -654,3 +656,50 @@ int at_util_get_ipv4_address_and_netmask(const char *addrnetmask,
 
 	return ret;
 }
+
+int at_get_auth_type(enum ofono_gprs_auth_method auth_method)
+{
+	switch (auth_method) {
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+		return 1;
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+		return 2;
+	case OFONO_GPRS_AUTH_METHOD_NONE:
+		return 0;
+	}
+
+	return 0;
+}
+
+char *at_get_cgdcont_command(guint cid, enum ofono_gprs_proto proto,
+							const char *apn)
+{
+	size_t buflen = 32 + OFONO_GPRS_MAX_APN_LENGTH + 1;
+	char *buf = g_new(char, buflen);
+	int len;
+
+	len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
+	buflen -= len;
+
+	/*
+	 * if apn is null, it will remove the context.
+	 * but if apn is empty, it will create a context with empty apn
+	 */
+	if (!apn)
+		goto finished;
+
+	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:
+		snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
+		break;
+	}
+
+finished:
+	return buf;
+}
diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
index 7113a4cd..6a40d045 100644
--- a/drivers/atmodem/atutil.h
+++ b/drivers/atmodem/atutil.h
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2008-2011  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
@@ -86,6 +87,11 @@ void at_util_sim_state_query_free(struct at_util_sim_state_query *req);
 int at_util_get_ipv4_address_and_netmask(const char *addrnetmask,
 						char *address, char *netmask);
 
+int at_get_auth_type(enum ofono_gprs_auth_method auth_method);
+
+char *at_get_cgdcont_command(guint cid, enum ofono_gprs_proto proto,
+							const char *apn);
+
 struct cb_data {
 	void *cb;
 	void *data;
-- 
2.17.1

[PATCH v7 5/7] atmodem/lte: proto and authentication handling

[Giacinto Cifelli]

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

The ofono_lte_default_attach_info now handles also the protocol and the
authentication method, username and password.

Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
---
 drivers/atmodem/lte.c | 119 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 102 insertions(+), 17 deletions(-)

diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index c4866623..b50aa0bc 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
@@ -44,41 +45,125 @@ struct lte_driver_data {
 	GAtChat *chat;
 };
 
-static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+struct lte_cbd {
+	gint ref_count; /* Ref count */
+	ofono_lte_cb_t cb;
+	void *data;
+	GAtChat *chat;
+	const struct ofono_lte_default_attach_info *info;
+	struct ofono_modem *modem;
+};
+
+static struct lte_cbd *lte_cb_data_new0(void *cb, void *data,
+		GAtChat *chat, const struct ofono_lte_default_attach_info *info)
+{
+	struct lte_cbd *cbd = g_new0(struct lte_cbd, 1);
+
+	cbd->ref_count = 1;
+	cbd->cb = cb;
+	cbd->data = data;
+	cbd->chat = chat;
+	cbd->info = info;
+
+	return cbd;
+}
+
+static inline struct lte_cbd *lte_cb_data_ref(struct lte_cbd *cbd)
+{
+	if (cbd == NULL)
+		return NULL;
+
+	g_atomic_int_inc(&cbd->ref_count);
+
+	return cbd;
+}
+
+static void lte_cb_data_unref(gpointer user_data)
+{
+	gboolean is_zero;
+	struct lte_cbd *cbd = user_data;
+
+	if (cbd == NULL)
+		return;
+
+	is_zero = g_atomic_int_dec_and_test(&cbd->ref_count);
+
+	if (is_zero == TRUE)
+		g_free(cbd);
+}
+
+static void at_lte_set_auth_cb(gboolean ok, GAtResult *result,
 							gpointer user_data)
 {
-	struct cb_data *cbd = user_data;
+	struct lte_cbd *cbd = user_data;
 	ofono_lte_cb_t cb = cbd->cb;
 	struct ofono_error error;
 
-	DBG("ok %d", ok);
-
 	decode_at_error(&error, g_at_result_final_response(result));
 	cb(&error, cbd->data);
 }
 
+static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *result,
+							gpointer user_data)
+{
+	struct lte_cbd *cbd = user_data;
+	ofono_lte_cb_t cb = cbd->cb;
+	void *data = cbd->data;
+	struct ofono_error error;
+	char buf[32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+					OFONO_GPRS_MAX_PASSWORD_LENGTH  + 1];
+	size_t buflen = sizeof(buf);
+	size_t len;
+	enum ofono_gprs_auth_method auth_method;
+	int auth_type;
+
+	if (!ok) {
+		lte_cb_data_unref(cbd);
+		decode_at_error(&error, g_at_result_final_response(result));
+		cb(&error, data);
+		return;
+	}
+
+	auth_method = cbd->info->auth_method;
+
+	/* change the authentication method if the  parameters are invalid */
+	if (!*cbd->info->username || !*cbd->info->password)
+		auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+	auth_type = at_get_auth_type(auth_method);
+	len = snprintf(buf, buflen, "AT+CGAUTH=0,%d",auth_type);
+	buflen -= len;
+
+	if (auth_method != OFONO_GPRS_AUTH_METHOD_NONE)
+		snprintf(buf + len, buflen, ",\"%s\",\"%s\"",
+				cbd->info->username, cbd->info->password);
+
+	cbd = lte_cb_data_ref(cbd);
+	if (g_at_chat_send(cbd->chat, buf, NULL,
+			at_lte_set_auth_cb, cbd, lte_cb_data_unref) > 0)
+		return;
+
+	lte_cb_data_unref(cbd);
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
 static void at_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);
-	char buf[32 + OFONO_GPRS_MAX_APN_LENGTH + 1];
-	struct cb_data *cbd = cb_data_new(cb, data);
-
-	DBG("LTE config with APN: %s", info->apn);
+	struct lte_cbd *cbd = lte_cb_data_new0(cb, data, ldd->chat, info);
+	char *buf = at_get_cgdcont_command(0, info->proto, info->apn);
 
-	if (strlen(info->apn) > 0)
-		snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"IP\",\"%s\"",
-				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)
-		return;
+					at_lte_set_default_attach_info_cb,
+					cbd, lte_cb_data_unref) > 0)
+		goto end;
 
+	lte_cb_data_unref(cbd);
 	CALLBACK_WITH_FAILURE(cb, data);
+end:
+	g_free(buf);
 }
 
 static gboolean lte_delayed_register(gpointer user_data)
-- 
2.17.1

[PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Giacinto Cifelli]

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

Added a vendor-specific lte atom, for handling PDP context creation
and authentication, using the actual formats for the vendor.

The code for PDP context and authentication command formating
is in a separate file, gemaltoutil, because in the future it can be
shared with the gprs-context atom.

Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
Co-authored-by: Sebastian Arnd <sebastianarnd@gmail.com>
---
 Makefile.am                         |   5 +-
 drivers/gemaltomodem/gemaltomodem.c |   2 +
 drivers/gemaltomodem/gemaltomodem.h |   4 +
 drivers/gemaltomodem/gemaltoutil.c  | 106 +++++++++++++
 drivers/gemaltomodem/gemaltoutil.h  |  31 ++++
 drivers/gemaltomodem/lte.c          | 221 ++++++++++++++++++++++++++++
 6 files changed, 368 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gemaltomodem/gemaltoutil.c
 create mode 100644 drivers/gemaltomodem/gemaltoutil.h
 create mode 100644 drivers/gemaltomodem/lte.c

diff --git a/Makefile.am b/Makefile.am
index e8e4ed95..dd0bc0bb 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -397,8 +397,11 @@ builtin_modules += gemaltomodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/gemaltomodem/gemaltomodem.h \
 			drivers/gemaltomodem/gemaltomodem.c \
+			drivers/gemaltomodem/gemaltoutil.h \
+			drivers/gemaltomodem/gemaltoutil.c \
 			drivers/gemaltomodem/location-reporting.c \
-			drivers/gemaltomodem/voicecall.c
+			drivers/gemaltomodem/voicecall.c \
+			drivers/gemaltomodem/lte.c
 
 builtin_modules += xmm7modem
 builtin_sources += drivers/atmodem/atutil.h \
diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
index 4818ac66..459c3583 100644
--- a/drivers/gemaltomodem/gemaltomodem.c
+++ b/drivers/gemaltomodem/gemaltomodem.c
@@ -37,12 +37,14 @@ static int gemaltomodem_init(void)
 {
 	gemalto_location_reporting_init();
 	gemalto_voicecall_init();
+	gemalto_lte_init();
 
 	return 0;
 }
 
 static void gemaltomodem_exit(void)
 {
+	gemalto_lte_exit();
 	gemalto_voicecall_exit();
 	gemalto_location_reporting_exit();
 }
diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
index 27b1460e..9f285563 100644
--- a/drivers/gemaltomodem/gemaltomodem.h
+++ b/drivers/gemaltomodem/gemaltomodem.h
@@ -21,9 +21,13 @@
  */
 
 #include <drivers/atmodem/atutil.h>
+#include "gemaltoutil.h"
 
 extern void gemalto_location_reporting_init();
 extern void gemalto_location_reporting_exit();
 
 extern void gemalto_voicecall_init();
 extern void gemalto_voicecall_exit();
+
+extern void gemalto_lte_init();
+extern void gemalto_lte_exit();
diff --git a/drivers/gemaltomodem/gemaltoutil.c b/drivers/gemaltomodem/gemaltoutil.c
new file mode 100644
index 00000000..d8c08e16
--- /dev/null
+++ b/drivers/gemaltomodem/gemaltoutil.c
@@ -0,0 +1,106 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <glib.h>
+#include <gatchat.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/log.h>
+#include <ofono/types.h>
+
+#include "ofono.h"
+#include "gemaltomodem.h"
+
+enum gemalto_auth_option {
+	GEMALTO_AUTH_DEFAULTS		= 0x00,
+	GEMALTO_AUTH_USE_SGAUTH		= 0x01,
+	GEMALTO_AUTH_ORDER_PWD_USR	= 0x02,
+	GEMALTO_AUTH_ALW_ALL_PARAMS	= 0x04,
+};
+
+char *gemalto_get_auth_command(struct ofono_modem *modem, int cid,
+				enum ofono_gprs_auth_method auth_method,
+				const char *username, const char *password)
+{
+	int gemalto_auth = ofono_modem_get_integer(modem, "GemaltoAuthType");
+	int len;
+	int auth_type = at_get_auth_type(auth_method);
+	size_t buflen = 32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
+					OFONO_GPRS_MAX_PASSWORD_LENGTH  + 1;
+	char *buf = g_new(char, buflen);
+
+	/* for now. Later to consider modules where the LTE attach CID=3 */
+	if (cid==0)
+		cid=1;
+
+	if (gemalto_auth & GEMALTO_AUTH_USE_SGAUTH)
+		len = snprintf(buf, buflen, "AT^SGAUTH=%d,%d", cid, auth_type);
+	else
+		len = snprintf(buf, buflen, "AT+CGAUTH=%d,%d", cid, auth_type);
+
+	buflen -= len;
+
+	switch(auth_method) {
+	case OFONO_GPRS_AUTH_METHOD_NONE:
+
+		if (gemalto_auth & GEMALTO_AUTH_ALW_ALL_PARAMS)
+			snprintf(buf+len, buflen, ",\"\",\"\"");
+
+		break;
+	case OFONO_GPRS_AUTH_METHOD_PAP:
+	case OFONO_GPRS_AUTH_METHOD_CHAP:
+
+		if (gemalto_auth & GEMALTO_AUTH_ORDER_PWD_USR)
+			snprintf(buf+len, buflen, ",\"%s\",\"%s\"",
+							password, username);
+		else
+			snprintf(buf+len, buflen, ",\"%s\",\"%s\"",
+							username, password);
+
+		break;
+	}
+
+	return buf;
+}
+
+char *gemalto_get_cgdcont_command(struct ofono_modem *modem, guint cid,
+				enum ofono_gprs_proto proto, const char *apn)
+{
+	/*
+	 * For future extension: verify if the module supports automatic
+	 * context provisioning and if so, also if there is a manual override
+	 * This is an ofono_modem_get_integer property
+	 */
+
+	/* for now. Later to consider modules where the LTE attach CID=3 */
+	if (cid==0)
+		cid=1;
+
+	return at_get_cgdcont_command(cid, proto, apn);
+}
diff --git a/drivers/gemaltomodem/gemaltoutil.h b/drivers/gemaltomodem/gemaltoutil.h
new file mode 100644
index 00000000..d653f61d
--- /dev/null
+++ b/drivers/gemaltomodem/gemaltoutil.h
@@ -0,0 +1,31 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  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.
+ *
+ *  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
+ *
+ */
+
+struct ofono_modem;
+
+char *gemalto_get_auth_command(struct ofono_modem *modem, int cid,
+				enum ofono_gprs_auth_method auth_method,
+				const char *username, const char *password);
+
+
+char *gemalto_get_cgdcont_command(struct ofono_modem *modem, guint cid,
+				enum ofono_gprs_proto proto, const char *apn);
+
diff --git a/drivers/gemaltomodem/lte.c b/drivers/gemaltomodem/lte.c
new file mode 100644
index 00000000..1c72173f
--- /dev/null
+++ b/drivers/gemaltomodem/lte.c
@@ -0,0 +1,221 @@
+/*
+ *
+ *  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
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/modem.h>
+#include <ofono/gprs-context.h>
+#include <ofono/log.h>
+#include <ofono/lte.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "gemaltomodem.h"
+
+struct gemalto_lte_driver_data {
+	GAtChat *chat;
+};
+
+struct gemalto_lte_cbd {
+	gint ref_count; /* Ref count */
+	ofono_lte_cb_t cb;
+	void *data;
+	GAtChat *chat;
+	const struct ofono_lte_default_attach_info *info;
+	struct ofono_modem *modem;
+};
+
+static struct gemalto_lte_cbd *gemalto_lte_cb_data_new0(void *cb, void *data,
+		GAtChat *chat, const struct ofono_lte_default_attach_info *info,
+		struct ofono_modem *modem)
+{
+	struct gemalto_lte_cbd *cbd = g_new0(struct gemalto_lte_cbd, 1);
+
+	cbd->ref_count = 1;
+	cbd->cb = cb;
+	cbd->data = data;
+	cbd->chat = chat;
+	cbd->info = info;
+	cbd->modem = modem;
+
+	return cbd;
+}
+
+static inline struct gemalto_lte_cbd *gemalto_lte_cb_data_ref(
+						struct gemalto_lte_cbd *cbd)
+{
+	if (cbd == NULL)
+		return NULL;
+
+	g_atomic_int_inc(&cbd->ref_count);
+
+	return cbd;
+}
+
+static void gemalto_lte_cb_data_unref(gpointer user_data)
+{
+	gboolean is_zero;
+	struct gemalto_lte_cbd *cbd = user_data;
+
+	if (cbd == NULL)
+		return;
+
+	is_zero = g_atomic_int_dec_and_test(&cbd->ref_count);
+
+	if (is_zero == TRUE)
+		g_free(cbd);
+}
+
+static void gemalto_lte_set_auth_cb(gboolean ok, GAtResult *result,
+							gpointer user_data)
+{
+	struct gemalto_lte_cbd *cbd = user_data;
+	ofono_lte_cb_t cb = cbd->cb;
+	struct ofono_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+	cb(&error, cbd->data);
+}
+
+static void gemalto_lte_set_default_attach_info_cb(gboolean ok,
+					GAtResult *result, gpointer user_data)
+{
+	struct gemalto_lte_cbd *cbd = user_data;
+	ofono_lte_cb_t cb = cbd->cb;
+	void *data = cbd->data;
+	struct ofono_error error;
+	char *buf;
+	enum ofono_gprs_auth_method auth_method;
+
+	if (!ok) {
+		gemalto_lte_cb_data_unref(cbd);
+		decode_at_error(&error, g_at_result_final_response(result));
+		cb(&error, data);
+		return;
+	}
+
+	auth_method = cbd->info->auth_method;
+
+	/* change the authentication method if the  parameters are invalid */
+	if (!*cbd->info->username || !*cbd->info->password)
+		auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
+
+	buf = gemalto_get_auth_command(cbd->modem, 0, auth_method,
+			cbd->info->username, cbd->info->password);
+	cbd = gemalto_lte_cb_data_ref(cbd);
+
+	if (g_at_chat_send(cbd->chat, buf, NULL,
+				gemalto_lte_set_auth_cb, cbd,
+				gemalto_lte_cb_data_unref) > 0)
+		goto end;
+
+	gemalto_lte_cb_data_unref(cbd);
+	CALLBACK_WITH_FAILURE(cb, data);
+end:
+	g_free(buf);
+}
+
+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 gemalto_lte_driver_data *ldd = ofono_lte_get_data(lte);
+	struct ofono_modem *modem = ofono_lte_get_modem(lte);
+	struct gemalto_lte_cbd *cbd = gemalto_lte_cb_data_new0(cb, data,
+					ldd->chat, info, modem);
+	char *buf = gemalto_get_cgdcont_command(modem, 0, info->proto,
+								info->apn);
+
+	if (g_at_chat_send(ldd->chat, buf, NULL,
+					gemalto_lte_set_default_attach_info_cb,
+					cbd, gemalto_lte_cb_data_unref) > 0)
+		goto end;
+
+	gemalto_lte_cb_data_unref(cbd);
+	CALLBACK_WITH_FAILURE(cb, data);
+end:
+	g_free(buf);
+}
+
+static gboolean gemalto_lte_delayed_register(gpointer user_data)
+{
+	struct ofono_lte *lte = user_data;
+
+	ofono_lte_register(lte);
+
+	return FALSE;
+}
+
+static int gemalto_lte_probe(struct ofono_lte *lte, unsigned int vendor,
+								void *data)
+{
+	GAtChat *chat = data;
+	struct gemalto_lte_driver_data *ldd;
+
+	ldd = g_new0(struct gemalto_lte_driver_data, 1);
+
+	ldd->chat = g_at_chat_clone(chat);
+
+	ofono_lte_set_data(lte, ldd);
+
+	g_idle_add(gemalto_lte_delayed_register, lte);
+
+	return 0;
+}
+
+static void gemalto_lte_remove(struct ofono_lte *lte)
+{
+	struct gemalto_lte_driver_data *ldd = ofono_lte_get_data(lte);
+
+	g_at_chat_unref(ldd->chat);
+
+	ofono_lte_set_data(lte, NULL);
+
+	g_free(ldd);
+}
+
+static const struct ofono_lte_driver driver = {
+	.name				= "gemaltomodem",
+	.probe				= gemalto_lte_probe,
+	.remove				= gemalto_lte_remove,
+	.set_default_attach_info	= gemalto_lte_set_default_attach_info,
+};
+
+void gemalto_lte_init(void)
+{
+	ofono_lte_driver_register(&driver);
+}
+
+void gemalto_lte_exit(void)
+{
+	ofono_lte_driver_unregister(&driver);
+}
-- 
2.17.1

[PATCH v7 7/7] plugins/gemalto: use Gemalto lte atom

[Giacinto Cifelli]

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

Switch from atmodem to gemaltomodem for the lte atom, in order to
benefit from the correct syntax for the AT commands.
The current models included in the Gemalto plugin only use one type
of authentication syntax, despite the atom supporting the syntax for
all Gemalto modules.
Therefore for now the variable GemaltoAuthType is set to a fixed value,
independent on the model.
---
 plugins/gemalto.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 5d3c77a9..3ca9f2d6 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -63,6 +63,13 @@ static const char *none_prefix[] = { NULL };
 static const char *sctm_prefix[] = { "^SCTM:", NULL };
 static const char *sbv_prefix[] = { "^SBV:", NULL };
 
+enum auth_option {
+	GEMALTO_AUTH_DEFAULTS		= 0x00,
+	GEMALTO_AUTH_USE_SGAUTH		= 0x01,
+	GEMALTO_AUTH_ORDER_PWD_USR	= 0x02,
+	GEMALTO_AUTH_ALW_ALL_PARAMS	= 0x04,
+};
+
 struct gemalto_hardware_monitor {
 	DBusMessage *msg;
 	int32_t temperature;
@@ -609,9 +616,12 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 	ofono_call_meter_create(modem, 0, "atmodem", data->app);
 	ofono_call_barring_create(modem, 0, "atmodem", data->app);
 
-	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x))
-		ofono_lte_create(modem, OFONO_VENDOR_CINTERION,
-						"atmodem", data->app);
+	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x)) {
+		ofono_modem_set_integer(modem, "GemaltoAuthType",
+						GEMALTO_AUTH_USE_SGAUTH |
+						GEMALTO_AUTH_ORDER_PWD_USR);
+		ofono_lte_create(modem, 0, "gemaltomodem", data->app);
+	}
 }
 
 static void gemalto_post_online(struct ofono_modem *modem)
-- 
2.17.1

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Jonas Bonn]

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

Hi Giacinto,

This looks pretty good to me...  Some comments below.

On 18/10/18 20:49, Giacinto Cifelli wrote:
> Added a vendor-specific lte atom, for handling PDP context creation
> and authentication, using the actual formats for the vendor.
> 
> The code for PDP context and authentication command formating
> is in a separate file, gemaltoutil, because in the future it can be
> shared with the gprs-context atom.
> 
> Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
> Co-authored-by: Sebastian Arnd <sebastianarnd@gmail.com>
> ---
>   Makefile.am                         |   5 +-
>   drivers/gemaltomodem/gemaltomodem.c |   2 +
>   drivers/gemaltomodem/gemaltomodem.h |   4 +
>   drivers/gemaltomodem/gemaltoutil.c  | 106 +++++++++++++
>   drivers/gemaltomodem/gemaltoutil.h  |  31 ++++
>   drivers/gemaltomodem/lte.c          | 221 ++++++++++++++++++++++++++++
>   6 files changed, 368 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gemaltomodem/gemaltoutil.c
>   create mode 100644 drivers/gemaltomodem/gemaltoutil.h
>   create mode 100644 drivers/gemaltomodem/lte.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index e8e4ed95..dd0bc0bb 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -397,8 +397,11 @@ builtin_modules += gemaltomodem
>   builtin_sources += drivers/atmodem/atutil.h \
>   			drivers/gemaltomodem/gemaltomodem.h \
>   			drivers/gemaltomodem/gemaltomodem.c \
> +			drivers/gemaltomodem/gemaltoutil.h \
> +			drivers/gemaltomodem/gemaltoutil.c \
>   			drivers/gemaltomodem/location-reporting.c \
> -			drivers/gemaltomodem/voicecall.c
> +			drivers/gemaltomodem/voicecall.c \
> +			drivers/gemaltomodem/lte.c
>   
>   builtin_modules += xmm7modem
>   builtin_sources += drivers/atmodem/atutil.h \
> diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
> index 4818ac66..459c3583 100644
> --- a/drivers/gemaltomodem/gemaltomodem.c
> +++ b/drivers/gemaltomodem/gemaltomodem.c
> @@ -37,12 +37,14 @@ static int gemaltomodem_init(void)
>   {
>   	gemalto_location_reporting_init();
>   	gemalto_voicecall_init();
> +	gemalto_lte_init();
>   
>   	return 0;
>   }
>   
>   static void gemaltomodem_exit(void)
>   {
> +	gemalto_lte_exit();
>   	gemalto_voicecall_exit();
>   	gemalto_location_reporting_exit();
>   }
> diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
> index 27b1460e..9f285563 100644
> --- a/drivers/gemaltomodem/gemaltomodem.h
> +++ b/drivers/gemaltomodem/gemaltomodem.h
> @@ -21,9 +21,13 @@
>    */
>   
>   #include <drivers/atmodem/atutil.h>
> +#include "gemaltoutil.h"
>   
>   extern void gemalto_location_reporting_init();
>   extern void gemalto_location_reporting_exit();
>   
>   extern void gemalto_voicecall_init();
>   extern void gemalto_voicecall_exit();
> +
> +extern void gemalto_lte_init();
> +extern void gemalto_lte_exit();
> diff --git a/drivers/gemaltomodem/gemaltoutil.c b/drivers/gemaltomodem/gemaltoutil.c
> new file mode 100644
> index 00000000..d8c08e16
> --- /dev/null
> +++ b/drivers/gemaltomodem/gemaltoutil.c
> @@ -0,0 +1,106 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  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.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <glib.h>
> +#include <gatchat.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +
> +#define OFONO_API_SUBJECT_TO_CHANGE

You shouldn't need this here... only if you pull in ofono/plugin.h directly.

> +#include <ofono/log.h>
> +#include <ofono/types.h>
> +
> +#include "ofono.h"
> +#include "gemaltomodem.h"
> +
> +enum gemalto_auth_option {
> +	GEMALTO_AUTH_DEFAULTS		= 0x00,
> +	GEMALTO_AUTH_USE_SGAUTH		= 0x01,
> +	GEMALTO_AUTH_ORDER_PWD_USR	= 0x02,
> +	GEMALTO_AUTH_ALW_ALL_PARAMS	= 0x04,
> +};

Using an enumeration to name flags feels awkward.  I would #define these:

#define AUTH_F_USE_SGATH (1<<0)
#define AUTH_F_SWAP_CREDENTIALS (1<<1)
#define AUTH_F_ALWAYS_ALL_PARAMS (1<<2)

Abbreviating ALWAYS to ALW is awkward...

Furthermore, considering that these flags are supposed to be set in 
udevng.c or the gemalto plugin, these flags really need to be defined 
elsewhere... include/gemalto.h, perhaps?


> +
> +char *gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> +				enum ofono_gprs_auth_method auth_method,
> +				const char *username, const char *password)
> +{
> +	int gemalto_auth = ofono_modem_get_integer(modem, "GemaltoAuthType");

Can we call these flags, or authflags, or something along those lines? 
Would be much clear, especially since we already have auth_method and 
auth_type to confuse with gemalto_auth.


> +	int len;
> +	int auth_type = at_get_auth_type(auth_method);
> +	size_t buflen = 32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
> +					OFONO_GPRS_MAX_PASSWORD_LENGTH  + 1;
> +	char *buf = g_new(char, buflen);
> +
> +	/* for now. Later to consider modules where the LTE attach CID=3 */
> +	if (cid==0)
> +		cid=1;
> +
> +	if (gemalto_auth & GEMALTO_AUTH_USE_SGAUTH)
> +		len = snprintf(buf, buflen, "AT^SGAUTH=%d,%d", cid, auth_type);

Instead of declaring the auth_type variable, just call the function 
directly in the parameter list.  Rename the function to 
at_auth_type_from_method() (or similar) so that it's obvious what it does.

(This stems from the fact that I keep losing track of the meaning of the 
similarly named variables... auth_type, auth_method, gemalto_auth).


> +	else
> +		len = snprintf(buf, buflen, "AT+CGAUTH=%d,%d", cid, auth_type);
> +
> +	buflen -= len;
> +
> +	switch(auth_method) {
> +	case OFONO_GPRS_AUTH_METHOD_NONE:
> +
> +		if (gemalto_auth & GEMALTO_AUTH_ALW_ALL_PARAMS)
> +			snprintf(buf+len, buflen, ",\"\",\"\"");
> +
> +		break;
> +	case OFONO_GPRS_AUTH_METHOD_PAP:
> +	case OFONO_GPRS_AUTH_METHOD_CHAP:
> +
> +		if (gemalto_auth & GEMALTO_AUTH_ORDER_PWD_USR)
> +			snprintf(buf+len, buflen, ",\"%s\",\"%s\"",
> +							password, username);
> +		else
> +			snprintf(buf+len, buflen, ",\"%s\",\"%s\"",
> +							username, password);
> +
> +		break;
> +	}
> +
> +	return buf;
> +}
> +
> +char *gemalto_get_cgdcont_command(struct ofono_modem *modem, guint cid,
> +				enum ofono_gprs_proto proto, const char *apn)
> +{
> +	/*
> +	 * For future extension: verify if the module supports automatic
> +	 * context provisioning and if so, also if there is a manual override
> +	 * This is an ofono_modem_get_integer property
> +	 */
> +
> +	/* for now. Later to consider modules where the LTE attach CID=3 */
> +	if (cid==0)
> +		cid=1;

I'm a bit confused why cid==0 means cid should be 1... is this because 
the AT network-registration atom assumes CID == 0 for the default 
context?  This should probably be fixed at the source, in that case, in 
atmodem/network-registration.c, with a vendor quirk...???

> +
> +	return at_get_cgdcont_command(cid, proto, apn);
> +}
> diff --git a/drivers/gemaltomodem/gemaltoutil.h b/drivers/gemaltomodem/gemaltoutil.h
> new file mode 100644
> index 00000000..d653f61d
> --- /dev/null
> +++ b/drivers/gemaltomodem/gemaltoutil.h
> @@ -0,0 +1,31 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  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.
> + *
> + *  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
> + *
> + */
> +
> +struct ofono_modem;

I think you might as well just pull in modem.h here.

> +
> +char *gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> +				enum ofono_gprs_auth_method auth_method,

This enum isn't defined either.

> +				const char *username, const char *password);
> +
> +
> +char *gemalto_get_cgdcont_command(struct ofono_modem *modem, guint cid,
> +				enum ofono_gprs_proto proto, const char *apn);
> +
> diff --git a/drivers/gemaltomodem/lte.c b/drivers/gemaltomodem/lte.c
> new file mode 100644
> index 00000000..1c72173f
> --- /dev/null
> +++ b/drivers/gemaltomodem/lte.c
> @@ -0,0 +1,221 @@
> +/*
> + *
> + *  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
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +
> +#include <ofono/modem.h>
> +#include <ofono/gprs-context.h>
> +#include <ofono/log.h>
> +#include <ofono/lte.h>
> +
> +#include "gatchat.h"
> +#include "gatresult.h"
> +
> +#include "gemaltomodem.h"
> +
> +struct gemalto_lte_driver_data {
> +	GAtChat *chat;
> +};
> +
> +struct gemalto_lte_cbd {
> +	gint ref_count; /* Ref count */
> +	ofono_lte_cb_t cb;
> +	void *data;
> +	GAtChat *chat;
> +	const struct ofono_lte_default_attach_info *info;
> +	struct ofono_modem *modem;
> +};
> +
> +static struct gemalto_lte_cbd *gemalto_lte_cb_data_new0(void *cb, void *data,
> +		GAtChat *chat, const struct ofono_lte_default_attach_info *info,
> +		struct ofono_modem *modem)
> +{
> +	struct gemalto_lte_cbd *cbd = g_new0(struct gemalto_lte_cbd, 1);
> +
> +	cbd->ref_count = 1;
> +	cbd->cb = cb;
> +	cbd->data = data;
> +	cbd->chat = chat;
> +	cbd->info = info;
> +	cbd->modem = modem;
> +
> +	return cbd;
> +}
> +
> +static inline struct gemalto_lte_cbd *gemalto_lte_cb_data_ref(
> +						struct gemalto_lte_cbd *cbd)
> +{
> +	if (cbd == NULL)
> +		return NULL;
> +
> +	g_atomic_int_inc(&cbd->ref_count);
> +
> +	return cbd;
> +}
> +
> +static void gemalto_lte_cb_data_unref(gpointer user_data)
> +{
> +	gboolean is_zero;
> +	struct gemalto_lte_cbd *cbd = user_data;
> +
> +	if (cbd == NULL)
> +		return;
> +
> +	is_zero = g_atomic_int_dec_and_test(&cbd->ref_count);
> +
> +	if (is_zero == TRUE)
> +		g_free(cbd);
> +}
> +
> +static void gemalto_lte_set_auth_cb(gboolean ok, GAtResult *result,
> +							gpointer user_data)
> +{
> +	struct gemalto_lte_cbd *cbd = user_data;
> +	ofono_lte_cb_t cb = cbd->cb;
> +	struct ofono_error error;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +	cb(&error, cbd->data);
> +}
> +
> +static void gemalto_lte_set_default_attach_info_cb(gboolean ok,
> +					GAtResult *result, gpointer user_data)
> +{
> +	struct gemalto_lte_cbd *cbd = user_data;
> +	ofono_lte_cb_t cb = cbd->cb;
> +	void *data = cbd->data;
> +	struct ofono_error error;
> +	char *buf;
> +	enum ofono_gprs_auth_method auth_method;
> +
> +	if (!ok) {
> +		gemalto_lte_cb_data_unref(cbd);
> +		decode_at_error(&error, g_at_result_final_response(result));
> +		cb(&error, data);
> +		return;
> +	}
> +
> +	auth_method = cbd->info->auth_method;
> +
> +	/* change the authentication method if the  parameters are invalid */
> +	if (!*cbd->info->username || !*cbd->info->password)
> +		auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
> +
> +	buf = gemalto_get_auth_command(cbd->modem, 0, auth_method,
> +			cbd->info->username, cbd->info->password);
> +	cbd = gemalto_lte_cb_data_ref(cbd);
> +
> +	if (g_at_chat_send(cbd->chat, buf, NULL,
> +				gemalto_lte_set_auth_cb, cbd,
> +				gemalto_lte_cb_data_unref) > 0)
> +		goto end;
> +
> +	gemalto_lte_cb_data_unref(cbd);
> +	CALLBACK_WITH_FAILURE(cb, data);
> +end:
> +	g_free(buf);
> +}
> +
> +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 gemalto_lte_driver_data *ldd = ofono_lte_get_data(lte);
> +	struct ofono_modem *modem = ofono_lte_get_modem(lte);
> +	struct gemalto_lte_cbd *cbd = gemalto_lte_cb_data_new0(cb, data,
> +					ldd->chat, info, modem);
> +	char *buf = gemalto_get_cgdcont_command(modem, 0, info->proto,
> +								info->apn);
> +
> +	if (g_at_chat_send(ldd->chat, buf, NULL,
> +					gemalto_lte_set_default_attach_info_cb,
> +					cbd, gemalto_lte_cb_data_unref) > 0)
> +		goto end;
> +
> +	gemalto_lte_cb_data_unref(cbd);
> +	CALLBACK_WITH_FAILURE(cb, data);
> +end:
> +	g_free(buf);
> +}
> +
> +static gboolean gemalto_lte_delayed_register(gpointer user_data)
> +{
> +	struct ofono_lte *lte = user_data;
> +
> +	ofono_lte_register(lte);
> +
> +	return FALSE;
> +}
> +
> +static int gemalto_lte_probe(struct ofono_lte *lte, unsigned int vendor,
> +								void *data)
> +{
> +	GAtChat *chat = data;
> +	struct gemalto_lte_driver_data *ldd;
> +
> +	ldd = g_new0(struct gemalto_lte_driver_data, 1);
> +
> +	ldd->chat = g_at_chat_clone(chat);
> +
> +	ofono_lte_set_data(lte, ldd);
> +
> +	g_idle_add(gemalto_lte_delayed_register, lte);

Why do you need to delay the registration?

> +
> +	return 0;
> +}
> +
> +static void gemalto_lte_remove(struct ofono_lte *lte)
> +{
> +	struct gemalto_lte_driver_data *ldd = ofono_lte_get_data(lte);
> +
> +	g_at_chat_unref(ldd->chat);
> +
> +	ofono_lte_set_data(lte, NULL);
> +
> +	g_free(ldd);
> +}
> +
> +static const struct ofono_lte_driver driver = {
> +	.name				= "gemaltomodem",
> +	.probe				= gemalto_lte_probe,
> +	.remove				= gemalto_lte_remove,
> +	.set_default_attach_info	= gemalto_lte_set_default_attach_info,
> +};
> +
> +void gemalto_lte_init(void)
> +{
> +	ofono_lte_driver_register(&driver);
> +}
> +
> +void gemalto_lte_exit(void)
> +{
> +	ofono_lte_driver_unregister(&driver);
> +}
> 

/Jonas

Re: [PATCH v7 7/7] plugins/gemalto: use Gemalto lte atom

[Jonas Bonn]

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

Hi Giacinto,

On 18/10/18 20:49, Giacinto Cifelli wrote:
> Switch from atmodem to gemaltomodem for the lte atom, in order to
> benefit from the correct syntax for the AT commands.
> The current models included in the Gemalto plugin only use one type
> of authentication syntax, despite the atom supporting the syntax for
> all Gemalto modules.
> Therefore for now the variable GemaltoAuthType is set to a fixed value,
> independent on the model.
> ---
>   plugins/gemalto.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 5d3c77a9..3ca9f2d6 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -63,6 +63,13 @@ static const char *none_prefix[] = { NULL };
>   static const char *sctm_prefix[] = { "^SCTM:", NULL };
>   static const char *sbv_prefix[] = { "^SBV:", NULL };
>   
> +enum auth_option {
> +	GEMALTO_AUTH_DEFAULTS		= 0x00,
> +	GEMALTO_AUTH_USE_SGAUTH		= 0x01,
> +	GEMALTO_AUTH_ORDER_PWD_USR	= 0x02,
> +	GEMALTO_AUTH_ALW_ALL_PARAMS	= 0x04,
> +};
> +
>   struct gemalto_hardware_monitor {
>   	DBusMessage *msg;
>   	int32_t temperature;
> @@ -609,9 +616,12 @@ static void gemalto_post_sim(struct ofono_modem *modem)
>   	ofono_call_meter_create(modem, 0, "atmodem", data->app);
>   	ofono_call_barring_create(modem, 0, "atmodem", data->app);
>   
> -	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x))
> -		ofono_lte_create(modem, OFONO_VENDOR_CINTERION,
> -						"atmodem", data->app);
> +	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x)) {
> +		ofono_modem_set_integer(modem, "GemaltoAuthType",
> +						GEMALTO_AUTH_USE_SGAUTH |
> +						GEMALTO_AUTH_ORDER_PWD_USR);
> +		ofono_lte_create(modem, 0, "gemaltomodem", data->app);

Why not call ofono_modem_get_string(modem, "Model") directly in the LTE 
atom in order to get the flags?

/Jonas

> +	}
>   }
>   
>   static void gemalto_post_online(struct ofono_modem *modem)
> 

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Slava Monich]

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

On 18/10/18 22:32, Jonas Bonn wrote:
> ...
>
>> +#include <ofono/log.h>
>> +#include <ofono/types.h>
>> +
>> +#include "ofono.h"
>> +#include "gemaltomodem.h"
>> +
>> +enum gemalto_auth_option {
>> +    GEMALTO_AUTH_DEFAULTS        = 0x00,
>> +    GEMALTO_AUTH_USE_SGAUTH        = 0x01,
>> +    GEMALTO_AUTH_ORDER_PWD_USR    = 0x02,
>> +    GEMALTO_AUTH_ALW_ALL_PARAMS    = 0x04,
>> +};
>
> Using an enumeration to name flags feels awkward.  I would #define these:
>
> #define AUTH_F_USE_SGATH (1<<0)
> #define AUTH_F_SWAP_CREDENTIALS (1<<1)
> #define AUTH_F_ALWAYS_ALL_PARAMS (1<<2)
>


It may feel awkward but I actually find that (enum'ing flags) quite 
useful for debugging with gdb - it allows the debugger to refer to those 
flags by name.

Cheers,
-Slava

Re: [PATCH v7 7/7] plugins/gemalto: use Gemalto lte atom

[Giacinto Cifelli]

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

Hi Jonas,

On Thu, Oct 18, 2018 at 9:41 PM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Hi Giacinto,
>
> On 18/10/18 20:49, Giacinto Cifelli wrote:
> > Switch from atmodem to gemaltomodem for the lte atom, in order to
> > benefit from the correct syntax for the AT commands.
> > The current models included in the Gemalto plugin only use one type
> > of authentication syntax, despite the atom supporting the syntax for
> > all Gemalto modules.
> > Therefore for now the variable GemaltoAuthType is set to a fixed value,
> > independent on the model.
> > ---
> >   plugins/gemalto.c | 16 +++++++++++++---
> >   1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> > index 5d3c77a9..3ca9f2d6 100644
> > --- a/plugins/gemalto.c
> > +++ b/plugins/gemalto.c
> > @@ -63,6 +63,13 @@ static const char *none_prefix[] = { NULL };
> >   static const char *sctm_prefix[] = { "^SCTM:", NULL };
> >   static const char *sbv_prefix[] = { "^SBV:", NULL };
> >
> > +enum auth_option {
> > +     GEMALTO_AUTH_DEFAULTS           = 0x00,
> > +     GEMALTO_AUTH_USE_SGAUTH         = 0x01,
> > +     GEMALTO_AUTH_ORDER_PWD_USR      = 0x02,
> > +     GEMALTO_AUTH_ALW_ALL_PARAMS     = 0x04,
> > +};
> > +
> >   struct gemalto_hardware_monitor {
> >       DBusMessage *msg;
> >       int32_t temperature;
> > @@ -609,9 +616,12 @@ static void gemalto_post_sim(struct ofono_modem *modem)
> >       ofono_call_meter_create(modem, 0, "atmodem", data->app);
> >       ofono_call_barring_create(modem, 0, "atmodem", data->app);
> >
> > -     if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x))
> > -             ofono_lte_create(modem, OFONO_VENDOR_CINTERION,
> > -                                             "atmodem", data->app);
> > +     if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x)) {
> > +             ofono_modem_set_integer(modem, "GemaltoAuthType",
> > +                                             GEMALTO_AUTH_USE_SGAUTH |
> > +                                             GEMALTO_AUTH_ORDER_PWD_USR);
> > +             ofono_lte_create(modem, 0, "gemaltomodem", data->app);
>
> Why not call ofono_modem_get_string(modem, "Model") directly in the LTE
> atom in order to get the flags?

Because it is not the atom's job.
There are other flags planned, and not all will be determined by model.
Beside, remembering to update each atom for each new modem is error-prone.


>
> /Jonas

Regards,
Giacinto

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Giacinto Cifelli]

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

Hi Jonas,

thank you for taking the time to go through the code.

I haven't seen any comments on the first 5 patches. Is it possible to
apply them?


On Thu, Oct 18, 2018 at 9:32 PM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Hi Giacinto,
>
> This looks pretty good to me...  Some comments below.
>
> On 18/10/18 20:49, Giacinto Cifelli wrote:
> > Added a vendor-specific lte atom, for handling PDP context creation
> > and authentication, using the actual formats for the vendor.
> >
> > The code for PDP context and authentication command formating
> > is in a separate file, gemaltoutil, because in the future it can be
> > shared with the gprs-context atom.
> >
> > Co-authored-by: Martin Baschin <martin.baschin@googlemail.com>
> > Co-authored-by: Sebastian Arnd <sebastianarnd@gmail.com>
> > ---
> >   Makefile.am                         |   5 +-
> >   drivers/gemaltomodem/gemaltomodem.c |   2 +
> >   drivers/gemaltomodem/gemaltomodem.h |   4 +
> >   drivers/gemaltomodem/gemaltoutil.c  | 106 +++++++++++++
> >   drivers/gemaltomodem/gemaltoutil.h  |  31 ++++
> >   drivers/gemaltomodem/lte.c          | 221 ++++++++++++++++++++++++++++
> >   6 files changed, 368 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/gemaltomodem/gemaltoutil.c
> >   create mode 100644 drivers/gemaltomodem/gemaltoutil.h
> >   create mode 100644 drivers/gemaltomodem/lte.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index e8e4ed95..dd0bc0bb 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -397,8 +397,11 @@ builtin_modules += gemaltomodem
> >   builtin_sources += drivers/atmodem/atutil.h \
> >                       drivers/gemaltomodem/gemaltomodem.h \
> >                       drivers/gemaltomodem/gemaltomodem.c \
> > +                     drivers/gemaltomodem/gemaltoutil.h \
> > +                     drivers/gemaltomodem/gemaltoutil.c \
> >                       drivers/gemaltomodem/location-reporting.c \
> > -                     drivers/gemaltomodem/voicecall.c
> > +                     drivers/gemaltomodem/voicecall.c \
> > +                     drivers/gemaltomodem/lte.c
> >
> >   builtin_modules += xmm7modem
> >   builtin_sources += drivers/atmodem/atutil.h \
> > diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
> > index 4818ac66..459c3583 100644
> > --- a/drivers/gemaltomodem/gemaltomodem.c
> > +++ b/drivers/gemaltomodem/gemaltomodem.c
> > @@ -37,12 +37,14 @@ static int gemaltomodem_init(void)
> >   {
> >       gemalto_location_reporting_init();
> >       gemalto_voicecall_init();
> > +     gemalto_lte_init();
> >
> >       return 0;
> >   }
> >
> >   static void gemaltomodem_exit(void)
> >   {
> > +     gemalto_lte_exit();
> >       gemalto_voicecall_exit();
> >       gemalto_location_reporting_exit();
> >   }
> > diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
> > index 27b1460e..9f285563 100644
> > --- a/drivers/gemaltomodem/gemaltomodem.h
> > +++ b/drivers/gemaltomodem/gemaltomodem.h
> > @@ -21,9 +21,13 @@
> >    */
> >
> >   #include <drivers/atmodem/atutil.h>
> > +#include "gemaltoutil.h"
> >
> >   extern void gemalto_location_reporting_init();
> >   extern void gemalto_location_reporting_exit();
> >
> >   extern void gemalto_voicecall_init();
> >   extern void gemalto_voicecall_exit();
> > +
> > +extern void gemalto_lte_init();
> > +extern void gemalto_lte_exit();
> > diff --git a/drivers/gemaltomodem/gemaltoutil.c b/drivers/gemaltomodem/gemaltoutil.c
> > new file mode 100644
> > index 00000000..d8c08e16
> > --- /dev/null
> > +++ b/drivers/gemaltomodem/gemaltoutil.c
> > @@ -0,0 +1,106 @@
> > +/*
> > + *
> > + *  oFono - Open Source Telephony
> > + *
> > + *  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.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with this program; if not, write to the Free Software
> > + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > + *
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#include <glib.h>
> > +#include <gatchat.h>
> > +#include <string.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <errno.h>
> > +
> > +#define OFONO_API_SUBJECT_TO_CHANGE
>
> You shouldn't need this here... only if you pull in ofono/plugin.h directly.
>
> > +#include <ofono/log.h>
> > +#include <ofono/types.h>
> > +
> > +#include "ofono.h"
> > +#include "gemaltomodem.h"
> > +
> > +enum gemalto_auth_option {
> > +     GEMALTO_AUTH_DEFAULTS           = 0x00,
> > +     GEMALTO_AUTH_USE_SGAUTH         = 0x01,
> > +     GEMALTO_AUTH_ORDER_PWD_USR      = 0x02,
> > +     GEMALTO_AUTH_ALW_ALL_PARAMS     = 0x04,
> > +};
>
> Using an enumeration to name flags feels awkward.  I would #define these:
>
> #define AUTH_F_USE_SGATH (1<<0)
> #define AUTH_F_SWAP_CREDENTIALS (1<<1)
> #define AUTH_F_ALWAYS_ALL_PARAMS (1<<2)

I prefer enum because I have several groups of flags, so it is clearer
which values a variable can take.
And also the advantage of gdb mentioned by Slava.

>
> Abbreviating ALWAYS to ALW is awkward...

ALW is a standard abbreviation. the 3GPP 31.102 is full of it.
But this I more willing to change, along with the shift of bits
instead of the hexa mapping.

>
> Furthermore, considering that these flags are supposed to be set in
> udevng.c or the gemalto plugin, these flags really need to be defined
> elsewhere... include/gemalto.h, perhaps?
>
>
> > +
> > +char *gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> > +                             enum ofono_gprs_auth_method auth_method,
> > +                             const char *username, const char *password)
> > +{
> > +     int gemalto_auth = ofono_modem_get_integer(modem, "GemaltoAuthType");
>
> Can we call these flags, or authflags, or something along those lines?
> Would be much clear, especially since we already have auth_method and
> auth_type to confuse with gemalto_auth.

ok, I will call them GemaltoAuthFlags.

>
>
> > +     int len;
> > +     int auth_type = at_get_auth_type(auth_method);
> > +     size_t buflen = 32 + OFONO_GPRS_MAX_USERNAME_LENGTH +
> > +                                     OFONO_GPRS_MAX_PASSWORD_LENGTH  + 1;
> > +     char *buf = g_new(char, buflen);
> > +
> > +     /* for now. Later to consider modules where the LTE attach CID=3 */
> > +     if (cid==0)
> > +             cid=1;
> > +
> > +     if (gemalto_auth & GEMALTO_AUTH_USE_SGAUTH)
> > +             len = snprintf(buf, buflen, "AT^SGAUTH=%d,%d", cid, auth_type);
>
> Instead of declaring the auth_type variable, just call the function
> directly in the parameter list.  Rename the function to
> at_auth_type_from_method() (or similar) so that it's obvious what it does.
>
> (This stems from the fact that I keep losing track of the meaning of the
> similarly named variables... auth_type, auth_method, gemalto_auth).
>

ok, good idea, this variable is only used once.

>
> > +     else
> > +             len = snprintf(buf, buflen, "AT+CGAUTH=%d,%d", cid, auth_type);
> > +
> > +     buflen -= len;
> > +
> > +     switch(auth_method) {
> > +     case OFONO_GPRS_AUTH_METHOD_NONE:
> > +
> > +             if (gemalto_auth & GEMALTO_AUTH_ALW_ALL_PARAMS)
> > +                     snprintf(buf+len, buflen, ",\"\",\"\"");
> > +
> > +             break;
> > +     case OFONO_GPRS_AUTH_METHOD_PAP:
> > +     case OFONO_GPRS_AUTH_METHOD_CHAP:
> > +
> > +             if (gemalto_auth & GEMALTO_AUTH_ORDER_PWD_USR)
> > +                     snprintf(buf+len, buflen, ",\"%s\",\"%s\"",
> > +                                                     password, username);
> > +             else
> > +                     snprintf(buf+len, buflen, ",\"%s\",\"%s\"",
> > +                                                     username, password);
> > +
> > +             break;
> > +     }
> > +
> > +     return buf;
> > +}
> > +
> > +char *gemalto_get_cgdcont_command(struct ofono_modem *modem, guint cid,
> > +                             enum ofono_gprs_proto proto, const char *apn)
> > +{
> > +     /*
> > +      * For future extension: verify if the module supports automatic
> > +      * context provisioning and if so, also if there is a manual override
> > +      * This is an ofono_modem_get_integer property
> > +      */
> > +
> > +     /* for now. Later to consider modules where the LTE attach CID=3 */
> > +     if (cid==0)
> > +             cid=1;
>
> I'm a bit confused why cid==0 means cid should be 1... is this because
> the AT network-registration atom assumes CID == 0 for the default
> context?  This should probably be fixed at the source, in that case, in
> atmodem/network-registration.c, with a vendor quirk...???

the 3GPP 27.007 says that the LTE attach context ID is 0, but actual
modems use other values.
For now there are around only models which use CID=1, but Gemalto also
has models with CID=3.
I will have to set another variable for this.

>
> > +
> > +     return at_get_cgdcont_command(cid, proto, apn);
> > +}
> > diff --git a/drivers/gemaltomodem/gemaltoutil.h b/drivers/gemaltomodem/gemaltoutil.h
> > new file mode 100644
> > index 00000000..d653f61d
> > --- /dev/null
> > +++ b/drivers/gemaltomodem/gemaltoutil.h
> > @@ -0,0 +1,31 @@
> > +/*
> > + *
> > + *  oFono - Open Source Telephony
> > + *
> > + *  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.
> > + *
> > + *  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
> > + *
> > + */
> > +
> > +struct ofono_modem;
>
> I think you might as well just pull in modem.h here.
>
> > +
> > +char *gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> > +                             enum ofono_gprs_auth_method auth_method,
>
> This enum isn't defined either.

That's strange. I might have included the relevant headers in the c file.
I'll look into it.

>
> > +                             const char *username, const char *password);
> > +
> > +
> > +char *gemalto_get_cgdcont_command(struct ofono_modem *modem, guint cid,
> > +                             enum ofono_gprs_proto proto, const char *apn);
> > +
> > diff --git a/drivers/gemaltomodem/lte.c b/drivers/gemaltomodem/lte.c
> > new file mode 100644
> > index 00000000..1c72173f
> > --- /dev/null
> > +++ b/drivers/gemaltomodem/lte.c
> > @@ -0,0 +1,221 @@
> > +/*
> > + *
> > + *  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
> > + *  published by the Free Software Foundation.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with this program; if not, write to the Free Software
> > + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > + *
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +
> > +#include <glib.h>
> > +
> > +#include <ofono/modem.h>
> > +#include <ofono/gprs-context.h>
> > +#include <ofono/log.h>
> > +#include <ofono/lte.h>
> > +
> > +#include "gatchat.h"
> > +#include "gatresult.h"
> > +
> > +#include "gemaltomodem.h"
> > +
> > +struct gemalto_lte_driver_data {
> > +     GAtChat *chat;
> > +};
> > +
> > +struct gemalto_lte_cbd {
> > +     gint ref_count; /* Ref count */
> > +     ofono_lte_cb_t cb;
> > +     void *data;
> > +     GAtChat *chat;
> > +     const struct ofono_lte_default_attach_info *info;
> > +     struct ofono_modem *modem;
> > +};
> > +
> > +static struct gemalto_lte_cbd *gemalto_lte_cb_data_new0(void *cb, void *data,
> > +             GAtChat *chat, const struct ofono_lte_default_attach_info *info,
> > +             struct ofono_modem *modem)
> > +{
> > +     struct gemalto_lte_cbd *cbd = g_new0(struct gemalto_lte_cbd, 1);
> > +
> > +     cbd->ref_count = 1;
> > +     cbd->cb = cb;
> > +     cbd->data = data;
> > +     cbd->chat = chat;
> > +     cbd->info = info;
> > +     cbd->modem = modem;
> > +
> > +     return cbd;
> > +}
> > +
> > +static inline struct gemalto_lte_cbd *gemalto_lte_cb_data_ref(
> > +                                             struct gemalto_lte_cbd *cbd)
> > +{
> > +     if (cbd == NULL)
> > +             return NULL;
> > +
> > +     g_atomic_int_inc(&cbd->ref_count);
> > +
> > +     return cbd;
> > +}
> > +
> > +static void gemalto_lte_cb_data_unref(gpointer user_data)
> > +{
> > +     gboolean is_zero;
> > +     struct gemalto_lte_cbd *cbd = user_data;
> > +
> > +     if (cbd == NULL)
> > +             return;
> > +
> > +     is_zero = g_atomic_int_dec_and_test(&cbd->ref_count);
> > +
> > +     if (is_zero == TRUE)
> > +             g_free(cbd);
> > +}
> > +
> > +static void gemalto_lte_set_auth_cb(gboolean ok, GAtResult *result,
> > +                                                     gpointer user_data)
> > +{
> > +     struct gemalto_lte_cbd *cbd = user_data;
> > +     ofono_lte_cb_t cb = cbd->cb;
> > +     struct ofono_error error;
> > +
> > +     decode_at_error(&error, g_at_result_final_response(result));
> > +     cb(&error, cbd->data);
> > +}
> > +
> > +static void gemalto_lte_set_default_attach_info_cb(gboolean ok,
> > +                                     GAtResult *result, gpointer user_data)
> > +{
> > +     struct gemalto_lte_cbd *cbd = user_data;
> > +     ofono_lte_cb_t cb = cbd->cb;
> > +     void *data = cbd->data;
> > +     struct ofono_error error;
> > +     char *buf;
> > +     enum ofono_gprs_auth_method auth_method;
> > +
> > +     if (!ok) {
> > +             gemalto_lte_cb_data_unref(cbd);
> > +             decode_at_error(&error, g_at_result_final_response(result));
> > +             cb(&error, data);
> > +             return;
> > +     }
> > +
> > +     auth_method = cbd->info->auth_method;
> > +
> > +     /* change the authentication method if the  parameters are invalid */
> > +     if (!*cbd->info->username || !*cbd->info->password)
> > +             auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
> > +
> > +     buf = gemalto_get_auth_command(cbd->modem, 0, auth_method,
> > +                     cbd->info->username, cbd->info->password);
> > +     cbd = gemalto_lte_cb_data_ref(cbd);
> > +
> > +     if (g_at_chat_send(cbd->chat, buf, NULL,
> > +                             gemalto_lte_set_auth_cb, cbd,
> > +                             gemalto_lte_cb_data_unref) > 0)
> > +             goto end;
> > +
> > +     gemalto_lte_cb_data_unref(cbd);
> > +     CALLBACK_WITH_FAILURE(cb, data);
> > +end:
> > +     g_free(buf);
> > +}
> > +
> > +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 gemalto_lte_driver_data *ldd = ofono_lte_get_data(lte);
> > +     struct ofono_modem *modem = ofono_lte_get_modem(lte);
> > +     struct gemalto_lte_cbd *cbd = gemalto_lte_cb_data_new0(cb, data,
> > +                                     ldd->chat, info, modem);
> > +     char *buf = gemalto_get_cgdcont_command(modem, 0, info->proto,
> > +                                                             info->apn);
> > +
> > +     if (g_at_chat_send(ldd->chat, buf, NULL,
> > +                                     gemalto_lte_set_default_attach_info_cb,
> > +                                     cbd, gemalto_lte_cb_data_unref) > 0)
> > +             goto end;
> > +
> > +     gemalto_lte_cb_data_unref(cbd);
> > +     CALLBACK_WITH_FAILURE(cb, data);
> > +end:
> > +     g_free(buf);
> > +}
> > +
> > +static gboolean gemalto_lte_delayed_register(gpointer user_data)
> > +{
> > +     struct ofono_lte *lte = user_data;
> > +
> > +     ofono_lte_register(lte);
> > +
> > +     return FALSE;
> > +}
> > +
> > +static int gemalto_lte_probe(struct ofono_lte *lte, unsigned int vendor,
> > +                                                             void *data)
> > +{
> > +     GAtChat *chat = data;
> > +     struct gemalto_lte_driver_data *ldd;
> > +
> > +     ldd = g_new0(struct gemalto_lte_driver_data, 1);
> > +
> > +     ldd->chat = g_at_chat_clone(chat);
> > +
> > +     ofono_lte_set_data(lte, ldd);
> > +
> > +     g_idle_add(gemalto_lte_delayed_register, lte);
>
> Why do you need to delay the registration?
>
> > +
> > +     return 0;
> > +}
> > +
> > +static void gemalto_lte_remove(struct ofono_lte *lte)
> > +{
> > +     struct gemalto_lte_driver_data *ldd = ofono_lte_get_data(lte);
> > +
> > +     g_at_chat_unref(ldd->chat);
> > +
> > +     ofono_lte_set_data(lte, NULL);
> > +
> > +     g_free(ldd);
> > +}
> > +
> > +static const struct ofono_lte_driver driver = {
> > +     .name                           = "gemaltomodem",
> > +     .probe                          = gemalto_lte_probe,
> > +     .remove                         = gemalto_lte_remove,
> > +     .set_default_attach_info        = gemalto_lte_set_default_attach_info,
> > +};
> > +
> > +void gemalto_lte_init(void)
> > +{
> > +     ofono_lte_driver_register(&driver);
> > +}
> > +
> > +void gemalto_lte_exit(void)
> > +{
> > +     ofono_lte_driver_unregister(&driver);
> > +}
> >
>
> /Jonas

Giacinto

Re: [PATCH v7 7/7] plugins/gemalto: use Gemalto lte atom

[Jonas Bonn]

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



On 19/10/18 05:09, Giacinto Cifelli wrote:
> Hi Jonas,
> 
> On Thu, Oct 18, 2018 at 9:41 PM Jonas Bonn <jonas@norrbonn.se> wrote:
>>
>> Hi Giacinto,
>>
>> On 18/10/18 20:49, Giacinto Cifelli wrote:
>>> Switch from atmodem to gemaltomodem for the lte atom, in order to
>>> benefit from the correct syntax for the AT commands.
>>> The current models included in the Gemalto plugin only use one type
>>> of authentication syntax, despite the atom supporting the syntax for
>>> all Gemalto modules.
>>> Therefore for now the variable GemaltoAuthType is set to a fixed value,
>>> independent on the model.
>>> ---
>>>    plugins/gemalto.c | 16 +++++++++++++---
>>>    1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
>>> index 5d3c77a9..3ca9f2d6 100644
>>> --- a/plugins/gemalto.c
>>> +++ b/plugins/gemalto.c
>>> @@ -63,6 +63,13 @@ static const char *none_prefix[] = { NULL };
>>>    static const char *sctm_prefix[] = { "^SCTM:", NULL };
>>>    static const char *sbv_prefix[] = { "^SBV:", NULL };
>>>
>>> +enum auth_option {
>>> +     GEMALTO_AUTH_DEFAULTS           = 0x00,
>>> +     GEMALTO_AUTH_USE_SGAUTH         = 0x01,
>>> +     GEMALTO_AUTH_ORDER_PWD_USR      = 0x02,
>>> +     GEMALTO_AUTH_ALW_ALL_PARAMS     = 0x04,
>>> +};
>>> +
>>>    struct gemalto_hardware_monitor {
>>>        DBusMessage *msg;
>>>        int32_t temperature;
>>> @@ -609,9 +616,12 @@ static void gemalto_post_sim(struct ofono_modem *modem)
>>>        ofono_call_meter_create(modem, 0, "atmodem", data->app);
>>>        ofono_call_barring_create(modem, 0, "atmodem", data->app);
>>>
>>> -     if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x))
>>> -             ofono_lte_create(modem, OFONO_VENDOR_CINTERION,
>>> -                                             "atmodem", data->app);
>>> +     if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x)) {
>>> +             ofono_modem_set_integer(modem, "GemaltoAuthType",
>>> +                                             GEMALTO_AUTH_USE_SGAUTH |
>>> +                                             GEMALTO_AUTH_ORDER_PWD_USR);
>>> +             ofono_lte_create(modem, 0, "gemaltomodem", data->app);
>>
>> Why not call ofono_modem_get_string(modem, "Model") directly in the LTE
>> atom in order to get the flags?
> 
> Because it is not the atom's job.
> There are other flags planned, and not all will be determined by model.
> Beside, remembering to update each atom for each new modem is error-prone.

It's difficult to provide patch review when part of the job involves 
reading the author's mind to know whither they are going... :)

As things currently stand, the flags are determined _only_ by model and 
are accessed only by gemaltoutil.c.  There's no reason to go from model 
to flags at such a high level.  Put a model_to_flags() function in 
gemaltoutil and keep everything in one place.

/Jonas

Re: [PATCH v7 7/7] plugins/gemalto: use Gemalto lte atom

[Giacinto Cifelli]

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

hi,

On Fri, 19 Oct 2018, 07:57 Jonas Bonn, <jonas@norrbonn.se> wrote:

>
>
> On 19/10/18 05:09, Giacinto Cifelli wrote:
> > Hi Jonas,
> >
> > On Thu, Oct 18, 2018 at 9:41 PM Jonas Bonn <jonas@norrbonn.se> wrote:
> >>
> >> Hi Giacinto,
> >>
> >> On 18/10/18 20:49, Giacinto Cifelli wrote:
> >>> Switch from atmodem to gemaltomodem for the lte atom, in order to
> >>> benefit from the correct syntax for the AT commands.
> >>> The current models included in the Gemalto plugin only use one type
> >>> of authentication syntax, despite the atom supporting the syntax for
> >>> all Gemalto modules.
> >>> Therefore for now the variable GemaltoAuthType is set to a fixed value,
> >>> independent on the model.
> >>> ---
> >>>    plugins/gemalto.c | 16 +++++++++++++---
> >>>    1 file changed, 13 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> >>> index 5d3c77a9..3ca9f2d6 100644
> >>> --- a/plugins/gemalto.c
> >>> +++ b/plugins/gemalto.c
> >>> @@ -63,6 +63,13 @@ static const char *none_prefix[] = { NULL };
> >>>    static const char *sctm_prefix[] = { "^SCTM:", NULL };
> >>>    static const char *sbv_prefix[] = { "^SBV:", NULL };
> >>>
> >>> +enum auth_option {
> >>> +     GEMALTO_AUTH_DEFAULTS           = 0x00,
> >>> +     GEMALTO_AUTH_USE_SGAUTH         = 0x01,
> >>> +     GEMALTO_AUTH_ORDER_PWD_USR      = 0x02,
> >>> +     GEMALTO_AUTH_ALW_ALL_PARAMS     = 0x04,
> >>> +};
> >>> +
> >>>    struct gemalto_hardware_monitor {
> >>>        DBusMessage *msg;
> >>>        int32_t temperature;
> >>> @@ -609,9 +616,12 @@ static void gemalto_post_sim(struct ofono_modem
> *modem)
> >>>        ofono_call_meter_create(modem, 0, "atmodem", data->app);
> >>>        ofono_call_barring_create(modem, 0, "atmodem", data->app);
> >>>
> >>> -     if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x))
> >>> -             ofono_lte_create(modem, OFONO_VENDOR_CINTERION,
> >>> -                                             "atmodem", data->app);
> >>> +     if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x)) {
> >>> +             ofono_modem_set_integer(modem, "GemaltoAuthType",
> >>> +                                             GEMALTO_AUTH_USE_SGAUTH |
> >>> +
>  GEMALTO_AUTH_ORDER_PWD_USR);
> >>> +             ofono_lte_create(modem, 0, "gemaltomodem", data->app);
> >>
> >> Why not call ofono_modem_get_string(modem, "Model") directly in the LTE
> >> atom in order to get the flags?
> >
> > Because it is not the atom's job.
> > There are other flags planned, and not all will be determined by model.
> > Beside, remembering to update each atom for each new modem is
> error-prone.
>
> It's difficult to provide patch review when part of the job involves
> reading the author's mind to know whither they are going... :)
>
> As things currently stand, the flags are determined _only_ by model and
> are accessed only by gemaltoutil.c.  There's no reason to go from model
> to flags at such a high level.  Put a model_to_flags() function in
> gemaltoutil and keep everything in one place.
>

I think I will remove these last two patches from the series for today.
I see if I can start submitting the Gemalto plugin over the weekend.
It is planned anyway, and even if several features depend on atom support
not yet there, I can at least give the new structure.

Then I will re-submit the lte gemalto afterwards.


> /Jonas
>

Giacinto

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

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Jonas Bonn]

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



On 19/10/18 05:21, Giacinto Cifelli wrote:
> Hi Jonas,
> 
> thank you for taking the time to go through the code.
> 
> I haven't seen any comments on the first 5 patches. Is it possible to
> apply them?

On the whole, things look pretty good.  Denis applies the patches; I can 
only expedite the process a bit by providing some review while he 
sleeps... ;)

/Jonas

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Denis Kenzior]

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

Hi Jonas,

> Using an enumeration to name flags feels awkward.  I would #define these:
> 
> #define AUTH_F_USE_SGATH (1<<0)
> #define AUTH_F_SWAP_CREDENTIALS (1<<1)
> #define AUTH_F_ALWAYS_ALL_PARAMS (1<<2)

Actually we prefer enums for enumerations with multiple related values 
or ones that can be extended.  It is fine to use a #define if you have a 
single random value, but if you have a series of related flags, just use 
an enum.  This also forces one to keep naming consistent per item M11 of 
doc/coding-style.txt.

The compiler can also treat enums as first class citizens, making them 
available in the debugger, etc.

> 
> Abbreviating ALWAYS to ALW is awkward...

+1

>> +
>> +struct ofono_modem;
> 
> I think you might as well just pull in modem.h here.
> 

This is bad advice.  If the header file is only using a pointer to a 
structure, a forward declaration is enough.  Including the entire 
modem.h file here would just be extra work for the preprocessor and 
compiler, nothing more.  I prefer my compilation times to be fast ;)

>> +static int gemalto_lte_probe(struct ofono_lte *lte, unsigned int vendor,
>> +                                void *data)
>> +{
>> +    GAtChat *chat = data;
>> +    struct gemalto_lte_driver_data *ldd;
>> +
>> +    ldd = g_new0(struct gemalto_lte_driver_data, 1);
>> +
>> +    ldd->chat = g_at_chat_clone(chat);
>> +
>> +    ofono_lte_set_data(lte, ldd);
>> +
>> +    g_idle_add(gemalto_lte_delayed_register, lte);
> 
> Why do you need to delay the registration?

It is not allowed to call register from within the probe method.  That 
is because register might trigger driver method calls, but until probe() 
returns the driver isn't set.  Some atoms do not do this (yet) and you 
can get away with it, but it is bad practice in general.

Regards,
-Denis

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Denis Kenzior]

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

Hi Giacinto,

>>> +char *gemalto_get_cgdcont_command(struct ofono_modem *modem, guint cid,
>>> +                             enum ofono_gprs_proto proto, const char *apn)
>>> +{
>>> +     /*
>>> +      * For future extension: verify if the module supports automatic
>>> +      * context provisioning and if so, also if there is a manual override
>>> +      * This is an ofono_modem_get_integer property
>>> +      */
>>> +
>>> +     /* for now. Later to consider modules where the LTE attach CID=3 */
>>> +     if (cid==0)
>>> +             cid=1;
>>
>> I'm a bit confused why cid==0 means cid should be 1... is this because
>> the AT network-registration atom assumes CID == 0 for the default
>> context?  This should probably be fixed at the source, in that case, in
>> atmodem/network-registration.c, with a vendor quirk...???
> 
> the 3GPP 27.007 says that the LTE attach context ID is 0, but actual
> modems use other values.
> For now there are around only models which use CID=1, but Gemalto also
> has models with CID=3.
> I will have to set another variable for this.
>

I still don't get this explanation.  You have a gemalto specific 
function, which is being passed the CID directly by the caller.  Why is 
there weird logic here to mess with the CID, just have the caller pass 
in the proper CID directly.

Also note that cid 1 and 3 are by default valid CIDs for context 
activations.  So your 'default attach' profile can be overridden by the 
core at any time.  So you may want to address this by setting the cid 
range appropriately.

Regards,
-Denis

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Giacinto Cifelli]

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

Hi Denis,
>
> I still don't get this explanation.  You have a gemalto specific
> function, which is being passed the CID directly by the caller.  Why is
> there weird logic here to mess with the CID, just have the caller pass
> in the proper CID directly.
>
> Also note that cid 1 and 3 are by default valid CIDs for context
> activations.  So your 'default attach' profile can be overridden by the
> core at any time.  So you may want to address this by setting the cid
> range appropriately.

The function is to be called also from gprs-context, therefore passing
0 it knows it has to check whether it has to use 1 or 3 or something
else.
The cid-ranges are set in the plugin, also because I cannot use
AT+CGDCONT=? with all models.
There are some that return an answer to this command like (1-16),
which is ok, but then we have some that return (1,2,3,4,5,6), and
still other (1-10,17)...

>
> Regards,
> -Denis

Regards,
Giacinto

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Denis Kenzior]

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

Hi Giacinto,

On 10/20/2018 11:24 AM, Giacinto Cifelli wrote:
> Hi Denis,
>>
>> I still don't get this explanation.  You have a gemalto specific
>> function, which is being passed the CID directly by the caller.  Why is
>> there weird logic here to mess with the CID, just have the caller pass
>> in the proper CID directly.
>>
>> Also note that cid 1 and 3 are by default valid CIDs for context
>> activations.  So your 'default attach' profile can be overridden by the
>> core at any time.  So you may want to address this by setting the cid
>> range appropriately.
> 
> The function is to be called also from gprs-context, therefore passing
> 0 it knows it has to check whether it has to use 1 or 3 or something
> else.

Sure, but fundamentally it is still gemalto specific, so why would you 
call it with cid=0 anyway?

> The cid-ranges are set in the plugin, also because I cannot use
> AT+CGDCONT=? with all models.
> There are some that return an answer to this command like (1-16),
> which is ok, but then we have some that return (1,2,3,4,5,6), and
> still other (1-10,17)...

That is all fine, but the logic to determine the cid belongs outside the 
utility function.

Regards,
-Denis

Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom

[Giacinto Cifelli]

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

Hi Denis,

On Sat, Oct 20, 2018 at 6:27 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> On 10/20/2018 11:24 AM, Giacinto Cifelli wrote:
> > Hi Denis,
> >>
> >> I still don't get this explanation.  You have a gemalto specific
> >> function, which is being passed the CID directly by the caller.  Why is
> >> there weird logic here to mess with the CID, just have the caller pass
> >> in the proper CID directly.
> >>
> >> Also note that cid 1 and 3 are by default valid CIDs for context
> >> activations.  So your 'default attach' profile can be overridden by the
> >> core at any time.  So you may want to address this by setting the cid
> >> range appropriately.
> >
> > The function is to be called also from gprs-context, therefore passing
> > 0 it knows it has to check whether it has to use 1 or 3 or something
> > else.
>
> Sure, but fundamentally it is still gemalto specific, so why would you
> call it with cid=0 anyway?
>
> > The cid-ranges are set in the plugin, also because I cannot use
> > AT+CGDCONT=? with all models.
> > There are some that return an answer to this command like (1-16),
> > which is ok, but then we have some that return (1,2,3,4,5,6), and
> > still other (1-10,17)...
>
> That is all fine, but the logic to determine the cid belongs outside the
> utility function.

ok.

>
> Regards,
> -Denis

Regards,
Giacinto