All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/1] atmodem/lte: added Gemalto vendor
@ 2018-10-13  8:09 Giacinto Cifelli
  2018-10-13  8:09 ` [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension Giacinto Cifelli
  0 siblings, 1 reply; 10+ messages in thread
From: Giacinto Cifelli @ 2018-10-13  8:09 UTC (permalink / raw)
  To: ofono

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

I would like to submit for review the following patch, for adding
the Gemalto vendor. I submit in a single patch also the modifications
under discussion in a separate thread, for ease of reading.

The main point I would like to ask is where to put the vendor-specific
but shared functions for formating the AT commands, because the same
procedures are used for gprs-context.

The point is valid also for other vendors: for example uBlox requires
the following format: "CHAP:apn" or "PAP:apn", if there is a context
authentication, in the AccessPoint string, on top of the authentication
command.


Giacinto Cifelli (1):
  atmodem/lte: Gemalto vendor specific extension

 drivers/atmodem/atutil.c | 103 +++++++++++++++++++++++++
 drivers/atmodem/atutil.h |  27 +++++++
 drivers/atmodem/lte.c    | 161 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 281 insertions(+), 10 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-13  8:09 [RFC PATCH 0/1] atmodem/lte: added Gemalto vendor Giacinto Cifelli
@ 2018-10-13  8:09 ` Giacinto Cifelli
  2018-10-14 17:56   ` Jonas Bonn
  2018-10-15 18:29   ` Denis Kenzior
  0 siblings, 2 replies; 10+ messages in thread
From: Giacinto Cifelli @ 2018-10-13  8:09 UTC (permalink / raw)
  To: ofono

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

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

The formating code is in atutils, because it is shared also with
gprs-context.
---
 drivers/atmodem/atutil.c | 103 +++++++++++++++++++++++++
 drivers/atmodem/atutil.h |  27 +++++++
 drivers/atmodem/lte.c    | 161 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 281 insertions(+), 10 deletions(-)

diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
index 6f4e8a20..1201dbf3 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
@@ -26,6 +27,7 @@
 #include <glib.h>
 #include <gatchat.h>
 #include <string.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <errno.h>
 
@@ -33,6 +35,8 @@
 #include <ofono/log.h>
 #include <ofono/types.h>
 
+#include "ofono.h"
+
 #include "atutil.h"
 #include "vendor.h"
 
@@ -654,3 +658,102 @@ int at_util_get_ipv4_address_and_netmask(const char *addrnetmask,
 
 	return ret;
 }
+
+static int gemalto_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;
+}
+
+gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
+				enum ofono_gprs_auth_method auth_method,
+				const char *username, const char *password,
+				char *buf, guint buflen)
+{
+	int gemalto_auth = ofono_modem_get_integer(modem, "GemaltoAuthType");
+	int len;
+	/*
+	 * 0x01: use sgauth (0=use cgauth)
+	 * 0x02: pwd, user order (0=user, pwd order)
+	 * 0x04: always use all: method, user, password
+	 */
+
+	int auth_type = gemalto_get_auth_type(auth_method);
+
+	if (auth_type != 0 && (!*username || !*password))
+		return FALSE;
+
+	if (gemalto_auth & 0x01)
+		len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
+	else
+		len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
+
+	buflen -= len;
+
+	switch(auth_type) {
+	case 0:
+
+		if (gemalto_auth & 0x04)
+			snprintf(buf+len, buflen, ",0,\"\",\"\"");
+		else
+			snprintf(buf+len, buflen, ",0");
+
+		break;
+	case 1:
+	case 2:
+
+		if (gemalto_auth & 0x02)
+			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
+					auth_type, password, username);
+		else
+			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
+					auth_type, username, password);
+
+		break;
+	default:
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
+void gemalto_get_cgdcont_command(struct ofono_modem *modem,
+			guint cid, enum ofono_gprs_proto proto, const char *apn,
+							char *buf, guint buflen)
+{
+	int len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
+	buflen -= len;
+
+	/*
+	 * 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
+	 */
+
+	/*
+	 * if apn is null, it will remove the context.
+	 * but if apn is empty, it will create a context with empty apn
+	 */
+	if (!apn)
+		return;
+
+	switch (proto) {
+	case OFONO_GPRS_PROTO_IPV6:
+		snprintf(buf+len, buflen, ",\"IPV6\",\"%s\"", apn);
+		break;
+	case OFONO_GPRS_PROTO_IPV4V6:
+		snprintf(buf+len, buflen, ",\"IPV4V6\",\"%s\"", apn);
+		break;
+	case OFONO_GPRS_PROTO_IP:
+		snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
+		break;
+	}
+}
diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
index 7113a4cd..b74db9fe 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
@@ -19,6 +20,8 @@
  *
  */
 
+struct ofono_modem;
+
 enum at_util_sms_store {
 	AT_UTIL_SMS_STORE_SM =	0,
 	AT_UTIL_SMS_STORE_ME =	1,
@@ -86,6 +89,15 @@ 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);
 
+gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
+				enum ofono_gprs_auth_method auth_method,
+				const char *username, const char *password,
+				char *buf, guint buflen);
+
+void gemalto_get_cgdcont_command(struct ofono_modem *modem,
+			guint cid, enum ofono_gprs_proto proto, const char *apn,
+						char *buf, guint buflen);
+
 struct cb_data {
 	void *cb;
 	void *data;
@@ -115,6 +127,21 @@ static inline int at_util_convert_signal_strength(int strength)
 	return result;
 }
 
+static inline int at_util_convert_signal_strength_cesq(int strength_GSM,
+					int strength_UTRAN, int strength_EUTRAN)
+{
+	int result = -1;
+
+	if (strength_GSM != 99)
+		result = (strength_GSM * 100) / 63;
+	else if (strength_UTRAN != 255)
+		result = (strength_UTRAN * 100) / 96;
+	else if (strength_EUTRAN != 255)
+		result = (strength_EUTRAN * 100) / 97;
+
+	return result;
+}
+
 #define CALLBACK_WITH_FAILURE(cb, args...)		\
 	do {						\
 		struct ofono_error cb_e;		\
diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
index efa4e5fe..41de4197 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
@@ -43,12 +44,65 @@
 
 struct lte_driver_data {
 	GAtChat *chat;
+	unsigned int vendor;
 };
 
-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;
+	unsigned int vendor;
+	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,
+		unsigned int vendor, struct ofono_modem *modem)
+{
+	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;
+	cbd->vendor = vendor;
+	cbd->modem = modem;
+
+	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;
 
@@ -58,27 +112,113 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *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;
+
+	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;
+
+	if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
+
+		if (!gemalto_get_auth_command(modem, 1, auth_method,
+						info->username, info->password,
+						buf, sizeof(buf)))
+			goto error;
+
+	} else { /* default vendor*/
+		len = snprintf(buf, buflen, "AT+CGAUTH=0,");
+		buflen -= len;
+
+		switch (auth_method) {
+		case OFONO_GPRS_AUTH_METHOD_PAP:
+			snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
+					cbd->info->username, cbd->info->password);
+			break;
+		case OFONO_GPRS_AUTH_METHOD_CHAP:
+			snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
+					cbd->info->username, cbd->info->password);
+			break;
+		case OFONO_GPRS_AUTH_METHOD_NONE:
+			snprintf(buf + len, buflen, "0");
+			break;
+		}
+	}
+
+	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;
+
+error:
+	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);
+	const char *proto;
+	size_t len;
+	struct ofono_modem *modem = ofono_lte_get_modem(lte);
+	struct lte_cbd *cbd = lte_cb_data_new0(cb, data, ldd->chat, info,
+							ldd->vendor, modem);
 
 	DBG("LTE config with APN: %s", 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\"");
+	if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
+		gemalto_get_cgdcont_command(modem, 1, info->proto, info->apn,
+					buf, sizeof(buf));
+	} else { /* default vendor*/
+
+		switch (info->proto) {
+		case OFONO_GPRS_PROTO_IPV6:
+			proto = "IPV6";
+			break;
+		case OFONO_GPRS_PROTO_IPV4V6:
+			proto = "IPV4V6";
+			break;
+		case OFONO_GPRS_PROTO_IP:
+			proto = "IP";
+			break;
+		}
+
+		len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
+
+		if (*info->apn)
+			snprintf(buf+len, sizeof(buf)-len, ",\"%s\"",
+								info->apn);
+	}
 
-	/* We can't do much in case of failure so don't check response. */
 	if (g_at_chat_send(ldd->chat, buf, NULL,
-			at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
+			at_lte_set_default_attach_info_cb,
+			cbd, lte_cb_data_unref) > 0)
 		return;
 
+	lte_cb_data_unref(cbd);
 	CALLBACK_WITH_FAILURE(cb, data);
 }
 
@@ -103,6 +243,7 @@ static int at_lte_probe(struct ofono_lte *lte, unsigned int vendor, void *data)
 		return -ENOMEM;
 
 	ldd->chat = g_at_chat_clone(chat);
+	ldd->vendor = vendor;
 
 	ofono_lte_set_data(lte, ldd);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-13  8:09 ` [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension Giacinto Cifelli
@ 2018-10-14 17:56   ` Jonas Bonn
  2018-10-14 18:04     ` Giacinto Cifelli
  2018-10-15 18:29   ` Denis Kenzior
  1 sibling, 1 reply; 10+ messages in thread
From: Jonas Bonn @ 2018-10-14 17:56 UTC (permalink / raw)
  To: ofono

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

Hi Giacinto,

Thanks for sending this as a patch.  I found this easier to understand 
than the question you sent earlier.

Where this stuff goes is Denis' call, but I'll explain how I see it:

There's _core_ ofono and then there are drivers.  Ideally, nothing 
vendor-specific ever goes into the core.  In a perfect world, you'd be 
able to select just a single modem to support and build ofono with 
support for exactly that modem and nothing else. The way ofono is 
structured, with vendor codes that pollute the "generic" drivers, this 
isn't quite possible, but it's not that far off in that vendor code is 
otherwise separated from each other in the drivers and plugins directories.

Given that, polluting atutil.c with vendor-specific code is not a great 
idea.  Even though you don't want to repeat the same code in lte.c and 
gprs-context.c, it's not a big deal to do so, and the code is probably 
easier to follow that way anyway.

Some comments below...

/Jonas

On 13/10/18 10:09, Giacinto Cifelli wrote:
> Added a vendor-specific extension for handling PDP context creation
> and authentication, using the actual formats for the vendor.
>
> The formating code is in atutils, because it is shared also with
> gprs-context.
> ---
>   drivers/atmodem/atutil.c | 103 +++++++++++++++++++++++++
>   drivers/atmodem/atutil.h |  27 +++++++
>   drivers/atmodem/lte.c    | 161 ++++++++++++++++++++++++++++++++++++---
>   3 files changed, 281 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
> index 6f4e8a20..1201dbf3 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
> @@ -26,6 +27,7 @@
>   #include <glib.h>
>   #include <gatchat.h>
>   #include <string.h>
> +#include <stdio.h>
>   #include <stdlib.h>
>   #include <errno.h>
>   
> @@ -33,6 +35,8 @@
>   #include <ofono/log.h>
>   #include <ofono/types.h>
>   
> +#include "ofono.h"
> +
>   #include "atutil.h"
>   #include "vendor.h"
>   
> @@ -654,3 +658,102 @@ int at_util_get_ipv4_address_and_netmask(const char *addrnetmask,
>   
>   	return ret;
>   }
> +
> +static int gemalto_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;
> +}



> +
> +gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> +				enum ofono_gprs_auth_method auth_method,
> +				const char *username, const char *password,
> +				char *buf, guint buflen)
> +{
> +	int gemalto_auth = ofono_modem_get_integer(modem, "GemaltoAuthType");
> +	int len;
> +	/*
> +	 * 0x01: use sgauth (0=use cgauth)
> +	 * 0x02: pwd, user order (0=user, pwd order)
> +	 * 0x04: always use all: method, user, password
> +	 */
> +
> +	int auth_type = gemalto_get_auth_type(auth_method);

> +
> +	if (auth_type != 0 && (!*username || !*password))
> +		return FALSE;

If auth_method != ...NONE is easier to read.


> +
> +	if (gemalto_auth & 0x01)

if (flags & GEMALTO_AUTH_F_WANT_SGAUTH)... I'm not even sure where these 
flags are being set.

> +		len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
> +	else
> +		len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
> +
> +	buflen -= len;
> +
> +	switch(auth_type) {
Switch auth_method?

> +	case 0:
> +
> +		if (gemalto_auth & 0x04)
> +			snprintf(buf+len, buflen, ",0,\"\",\"\"");
> +		else
> +			snprintf(buf+len, buflen, ",0");
> +
> +		break;
> +	case 1:
> +	case 2:
> +
> +		if (gemalto_auth & 0x02)
> +			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> +					auth_type, password, username);
> +		else
> +			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> +					auth_type, username, password);
> +
> +		break;
> +	default:
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
> +void gemalto_get_cgdcont_command(struct ofono_modem *modem,
> +			guint cid, enum ofono_gprs_proto proto, const char *apn,
> +							char *buf, guint buflen)
> +{
> +	int len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
> +	buflen -= len;
> +
> +	/*
> +	 * 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
> +	 */
> +
> +	/*
> +	 * if apn is null, it will remove the context.
> +	 * but if apn is empty, it will create a context with empty apn
> +	 */
> +	if (!apn)
> +		return;
> +
> +	switch (proto) {
> +	case OFONO_GPRS_PROTO_IPV6:
> +		snprintf(buf+len, buflen, ",\"IPV6\",\"%s\"", apn);
> +		break;
> +	case OFONO_GPRS_PROTO_IPV4V6:
> +		snprintf(buf+len, buflen, ",\"IPV4V6\",\"%s\"", apn);
> +		break;
> +	case OFONO_GPRS_PROTO_IP:
> +		snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
> +		break;
> +	}
> +}
> diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
> index 7113a4cd..b74db9fe 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
> @@ -19,6 +20,8 @@
>    *
>    */
>   
> +struct ofono_modem;
> +
>   enum at_util_sms_store {
>   	AT_UTIL_SMS_STORE_SM =	0,
>   	AT_UTIL_SMS_STORE_ME =	1,
> @@ -86,6 +89,15 @@ 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);
>   
> +gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> +				enum ofono_gprs_auth_method auth_method,
> +				const char *username, const char *password,
> +				char *buf, guint buflen);
> +
> +void gemalto_get_cgdcont_command(struct ofono_modem *modem,
> +			guint cid, enum ofono_gprs_proto proto, const char *apn,
> +						char *buf, guint buflen);
> +
>   struct cb_data {
>   	void *cb;
>   	void *data;
> @@ -115,6 +127,21 @@ static inline int at_util_convert_signal_strength(int strength)
>   	return result;
>   }
>   
> +static inline int at_util_convert_signal_strength_cesq(int strength_GSM,
> +					int strength_UTRAN, int strength_EUTRAN)
> +{
> +	int result = -1;
> +
> +	if (strength_GSM != 99)
> +		result = (strength_GSM * 100) / 63;
> +	else if (strength_UTRAN != 255)
> +		result = (strength_UTRAN * 100) / 96;
> +	else if (strength_EUTRAN != 255)
> +		result = (strength_EUTRAN * 100) / 97;
> +
> +	return result;
> +}
> +
>   #define CALLBACK_WITH_FAILURE(cb, args...)		\
>   	do {						\
>   		struct ofono_error cb_e;		\
> diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
> index efa4e5fe..41de4197 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
> @@ -43,12 +44,65 @@
>   
>   struct lte_driver_data {
>   	GAtChat *chat;
> +	unsigned int vendor;
>   };
>   
> -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;
> +	unsigned int vendor;
> +	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,
> +		unsigned int vendor, struct ofono_modem *modem)
> +{
> +	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;
> +	cbd->vendor = vendor;
> +	cbd->modem = modem;
> +
> +	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;
>   
> @@ -58,27 +112,113 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *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;
> +
> +	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;
> +
> +	if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
> +
> +		if (!gemalto_get_auth_command(modem, 1, auth_method,
> +						info->username, info->password,
> +						buf, sizeof(buf)))
> +			goto error;
> +
> +	} else { /* default vendor*/
> +		len = snprintf(buf, buflen, "AT+CGAUTH=0,");
> +		buflen -= len;
> +
> +		switch (auth_method) {
> +		case OFONO_GPRS_AUTH_METHOD_PAP:
> +			snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
> +					cbd->info->username, cbd->info->password);
> +			break;
> +		case OFONO_GPRS_AUTH_METHOD_CHAP:
> +			snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
> +					cbd->info->username, cbd->info->password);
> +			break;
> +		case OFONO_GPRS_AUTH_METHOD_NONE:
> +			snprintf(buf + len, buflen, "0");
> +			break;
> +		}
> +	}
> +
> +	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;
> +
> +error:
> +	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);
> +	const char *proto;
> +	size_t len;
> +	struct ofono_modem *modem = ofono_lte_get_modem(lte);
> +	struct lte_cbd *cbd = lte_cb_data_new0(cb, data, ldd->chat, info,
> +							ldd->vendor, modem);
>   
>   	DBG("LTE config with APN: %s", 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\"");
> +	if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
> +		gemalto_get_cgdcont_command(modem, 1, info->proto, info->apn,
> +					buf, sizeof(buf));
> +	} else { /* default vendor*/
> +
> +		switch (info->proto) {
> +		case OFONO_GPRS_PROTO_IPV6:
> +			proto = "IPV6";
> +			break;
> +		case OFONO_GPRS_PROTO_IPV4V6:
> +			proto = "IPV4V6";
> +			break;
> +		case OFONO_GPRS_PROTO_IP:
> +			proto = "IP";
> +			break;
> +		}
> +
> +		len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
> +
> +		if (*info->apn)
> +			snprintf(buf+len, sizeof(buf)-len, ",\"%s\"",
> +								info->apn);
> +	}
>   
> -	/* We can't do much in case of failure so don't check response. */
>   	if (g_at_chat_send(ldd->chat, buf, NULL,
> -			at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
> +			at_lte_set_default_attach_info_cb,
> +			cbd, lte_cb_data_unref) > 0)
>   		return;
>   
> +	lte_cb_data_unref(cbd);
>   	CALLBACK_WITH_FAILURE(cb, data);
>   }
>   
> @@ -103,6 +243,7 @@ static int at_lte_probe(struct ofono_lte *lte, unsigned int vendor, void *data)
>   		return -ENOMEM;
>   
>   	ldd->chat = g_at_chat_clone(chat);
> +	ldd->vendor = vendor;
>   
>   	ofono_lte_set_data(lte, ldd);
>   


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-14 17:56   ` Jonas Bonn
@ 2018-10-14 18:04     ` Giacinto Cifelli
  2018-10-15 18:37       ` Denis Kenzior
  0 siblings, 1 reply; 10+ messages in thread
From: Giacinto Cifelli @ 2018-10-14 18:04 UTC (permalink / raw)
  To: ofono

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

Hi Jonas,


On Sun, Oct 14, 2018 at 7:56 PM Jonas Bonn <jonas@southpole.se> wrote:
>
> Hi Giacinto,
>
> Thanks for sending this as a patch.  I found this easier to understand
> than the question you sent earlier.
>
> Where this stuff goes is Denis' call, but I'll explain how I see it:
>
> There's _core_ ofono and then there are drivers.  Ideally, nothing
> vendor-specific ever goes into the core.  In a perfect world, you'd be
> able to select just a single modem to support and build ofono with
> support for exactly that modem and nothing else. The way ofono is
> structured, with vendor codes that pollute the "generic" drivers, this
> isn't quite possible, but it's not that far off in that vendor code is
> otherwise separated from each other in the drivers and plugins directories.
>
> Given that, polluting atutil.c with vendor-specific code is not a great
> idea.  Even though you don't want to repeat the same code in lte.c and
> gprs-context.c, it's not a big deal to do so, and the code is probably
> easier to follow that way anyway.

What about introducing a vendor.c ?
I prefer not to duplicate exactly the same functions if possible.
Also, if we start duplicating, one might protest - in this case - that
for the lte version I doesn't need to pass the contextID because it is
fixed to 1, and then one needs to really maintain 2 versions, which is
in general not really practical.

Ok for the other comments, and let's wait for Denis before re-submitting.

The flags are all set (or will be) in the gemalto plugin.

>
> Some comments below...
>
> /Jonas
>
> On 13/10/18 10:09, Giacinto Cifelli wrote:
> > Added a vendor-specific extension for handling PDP context creation
> > and authentication, using the actual formats for the vendor.
> >
> > The formating code is in atutils, because it is shared also with
> > gprs-context.
> > ---
> >   drivers/atmodem/atutil.c | 103 +++++++++++++++++++++++++
> >   drivers/atmodem/atutil.h |  27 +++++++
> >   drivers/atmodem/lte.c    | 161 ++++++++++++++++++++++++++++++++++++---
> >   3 files changed, 281 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
> > index 6f4e8a20..1201dbf3 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
> > @@ -26,6 +27,7 @@
> >   #include <glib.h>
> >   #include <gatchat.h>
> >   #include <string.h>
> > +#include <stdio.h>
> >   #include <stdlib.h>
> >   #include <errno.h>
> >
> > @@ -33,6 +35,8 @@
> >   #include <ofono/log.h>
> >   #include <ofono/types.h>
> >
> > +#include "ofono.h"
> > +
> >   #include "atutil.h"
> >   #include "vendor.h"
> >
> > @@ -654,3 +658,102 @@ int at_util_get_ipv4_address_and_netmask(const char *addrnetmask,
> >
> >       return ret;
> >   }
> > +
> > +static int gemalto_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;
> > +}
>
>
>
> > +
> > +gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> > +                             enum ofono_gprs_auth_method auth_method,
> > +                             const char *username, const char *password,
> > +                             char *buf, guint buflen)
> > +{
> > +     int gemalto_auth = ofono_modem_get_integer(modem, "GemaltoAuthType");
> > +     int len;
> > +     /*
> > +      * 0x01: use sgauth (0=use cgauth)
> > +      * 0x02: pwd, user order (0=user, pwd order)
> > +      * 0x04: always use all: method, user, password
> > +      */
> > +
> > +     int auth_type = gemalto_get_auth_type(auth_method);
>
> > +
> > +     if (auth_type != 0 && (!*username || !*password))
> > +             return FALSE;
>
> If auth_method != ...NONE is easier to read.
>
>
> > +
> > +     if (gemalto_auth & 0x01)
>
> if (flags & GEMALTO_AUTH_F_WANT_SGAUTH)... I'm not even sure where these
> flags are being set.
>
> > +             len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
> > +     else
> > +             len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
> > +
> > +     buflen -= len;
> > +
> > +     switch(auth_type) {
> Switch auth_method?
>
> > +     case 0:
> > +
> > +             if (gemalto_auth & 0x04)
> > +                     snprintf(buf+len, buflen, ",0,\"\",\"\"");
> > +             else
> > +                     snprintf(buf+len, buflen, ",0");
> > +
> > +             break;
> > +     case 1:
> > +     case 2:
> > +
> > +             if (gemalto_auth & 0x02)
> > +                     snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> > +                                     auth_type, password, username);
> > +             else
> > +                     snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> > +                                     auth_type, username, password);
> > +
> > +             break;
> > +     default:
> > +             return FALSE;
> > +     }
> > +
> > +     return TRUE;
> > +}
> > +
> > +void gemalto_get_cgdcont_command(struct ofono_modem *modem,
> > +                     guint cid, enum ofono_gprs_proto proto, const char *apn,
> > +                                                     char *buf, guint buflen)
> > +{
> > +     int len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
> > +     buflen -= len;
> > +
> > +     /*
> > +      * 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
> > +      */
> > +
> > +     /*
> > +      * if apn is null, it will remove the context.
> > +      * but if apn is empty, it will create a context with empty apn
> > +      */
> > +     if (!apn)
> > +             return;
> > +
> > +     switch (proto) {
> > +     case OFONO_GPRS_PROTO_IPV6:
> > +             snprintf(buf+len, buflen, ",\"IPV6\",\"%s\"", apn);
> > +             break;
> > +     case OFONO_GPRS_PROTO_IPV4V6:
> > +             snprintf(buf+len, buflen, ",\"IPV4V6\",\"%s\"", apn);
> > +             break;
> > +     case OFONO_GPRS_PROTO_IP:
> > +             snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
> > +             break;
> > +     }
> > +}
> > diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
> > index 7113a4cd..b74db9fe 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
> > @@ -19,6 +20,8 @@
> >    *
> >    */
> >
> > +struct ofono_modem;
> > +
> >   enum at_util_sms_store {
> >       AT_UTIL_SMS_STORE_SM =  0,
> >       AT_UTIL_SMS_STORE_ME =  1,
> > @@ -86,6 +89,15 @@ 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);
> >
> > +gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> > +                             enum ofono_gprs_auth_method auth_method,
> > +                             const char *username, const char *password,
> > +                             char *buf, guint buflen);
> > +
> > +void gemalto_get_cgdcont_command(struct ofono_modem *modem,
> > +                     guint cid, enum ofono_gprs_proto proto, const char *apn,
> > +                                             char *buf, guint buflen);
> > +
> >   struct cb_data {
> >       void *cb;
> >       void *data;
> > @@ -115,6 +127,21 @@ static inline int at_util_convert_signal_strength(int strength)
> >       return result;
> >   }
> >
> > +static inline int at_util_convert_signal_strength_cesq(int strength_GSM,
> > +                                     int strength_UTRAN, int strength_EUTRAN)
> > +{
> > +     int result = -1;
> > +
> > +     if (strength_GSM != 99)
> > +             result = (strength_GSM * 100) / 63;
> > +     else if (strength_UTRAN != 255)
> > +             result = (strength_UTRAN * 100) / 96;
> > +     else if (strength_EUTRAN != 255)
> > +             result = (strength_EUTRAN * 100) / 97;
> > +
> > +     return result;
> > +}
> > +
> >   #define CALLBACK_WITH_FAILURE(cb, args...)          \
> >       do {                                            \
> >               struct ofono_error cb_e;                \
> > diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
> > index efa4e5fe..41de4197 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
> > @@ -43,12 +44,65 @@
> >
> >   struct lte_driver_data {
> >       GAtChat *chat;
> > +     unsigned int vendor;
> >   };
> >
> > -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;
> > +     unsigned int vendor;
> > +     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,
> > +             unsigned int vendor, struct ofono_modem *modem)
> > +{
> > +     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;
> > +     cbd->vendor = vendor;
> > +     cbd->modem = modem;
> > +
> > +     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;
> >
> > @@ -58,27 +112,113 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *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;
> > +
> > +     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;
> > +
> > +     if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
> > +
> > +             if (!gemalto_get_auth_command(modem, 1, auth_method,
> > +                                             info->username, info->password,
> > +                                             buf, sizeof(buf)))
> > +                     goto error;
> > +
> > +     } else { /* default vendor*/
> > +             len = snprintf(buf, buflen, "AT+CGAUTH=0,");
> > +             buflen -= len;
> > +
> > +             switch (auth_method) {
> > +             case OFONO_GPRS_AUTH_METHOD_PAP:
> > +                     snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
> > +                                     cbd->info->username, cbd->info->password);
> > +                     break;
> > +             case OFONO_GPRS_AUTH_METHOD_CHAP:
> > +                     snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
> > +                                     cbd->info->username, cbd->info->password);
> > +                     break;
> > +             case OFONO_GPRS_AUTH_METHOD_NONE:
> > +                     snprintf(buf + len, buflen, "0");
> > +                     break;
> > +             }
> > +     }
> > +
> > +     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;
> > +
> > +error:
> > +     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);
> > +     const char *proto;
> > +     size_t len;
> > +     struct ofono_modem *modem = ofono_lte_get_modem(lte);
> > +     struct lte_cbd *cbd = lte_cb_data_new0(cb, data, ldd->chat, info,
> > +                                                     ldd->vendor, modem);
> >
> >       DBG("LTE config with APN: %s", 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\"");
> > +     if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
> > +             gemalto_get_cgdcont_command(modem, 1, info->proto, info->apn,
> > +                                     buf, sizeof(buf));
> > +     } else { /* default vendor*/
> > +
> > +             switch (info->proto) {
> > +             case OFONO_GPRS_PROTO_IPV6:
> > +                     proto = "IPV6";
> > +                     break;
> > +             case OFONO_GPRS_PROTO_IPV4V6:
> > +                     proto = "IPV4V6";
> > +                     break;
> > +             case OFONO_GPRS_PROTO_IP:
> > +                     proto = "IP";
> > +                     break;
> > +             }
> > +
> > +             len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
> > +
> > +             if (*info->apn)
> > +                     snprintf(buf+len, sizeof(buf)-len, ",\"%s\"",
> > +                                                             info->apn);
> > +     }
> >
> > -     /* We can't do much in case of failure so don't check response. */
> >       if (g_at_chat_send(ldd->chat, buf, NULL,
> > -                     at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
> > +                     at_lte_set_default_attach_info_cb,
> > +                     cbd, lte_cb_data_unref) > 0)
> >               return;
> >
> > +     lte_cb_data_unref(cbd);
> >       CALLBACK_WITH_FAILURE(cb, data);
> >   }
> >
> > @@ -103,6 +243,7 @@ static int at_lte_probe(struct ofono_lte *lte, unsigned int vendor, void *data)
> >               return -ENOMEM;
> >
> >       ldd->chat = g_at_chat_clone(chat);
> > +     ldd->vendor = vendor;
> >
> >       ofono_lte_set_data(lte, ldd);
> >
>

Regards,
Giacinto

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-13  8:09 ` [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension Giacinto Cifelli
  2018-10-14 17:56   ` Jonas Bonn
@ 2018-10-15 18:29   ` Denis Kenzior
  2018-10-15 18:59     ` Giacinto Cifelli
  2018-10-16  5:22     ` Giacinto Cifelli
  1 sibling, 2 replies; 10+ messages in thread
From: Denis Kenzior @ 2018-10-15 18:29 UTC (permalink / raw)
  To: ofono

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

Hi Giacinto,

On 10/13/2018 03:09 AM, Giacinto Cifelli wrote:
> Added a vendor-specific extension for handling PDP context creation
> and authentication, using the actual formats for the vendor.
> 

No, please don't do this.  atutil is only for generic / utility code 
that is vendor neutral.  E.g. stuff described in 27.007.  We made one 
exception here for parsing CREG/CGREG and that is because some vendors 
just can't implement AT commands properly.

> The formating code is in atutils, because it is shared also with
> gprs-context.
> ---
>   drivers/atmodem/atutil.c | 103 +++++++++++++++++++++++++
>   drivers/atmodem/atutil.h |  27 +++++++
>   drivers/atmodem/lte.c    | 161 ++++++++++++++++++++++++++++++++++++---
>   3 files changed, 281 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
> index 6f4e8a20..1201dbf3 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
> @@ -26,6 +27,7 @@
>   #include <glib.h>
>   #include <gatchat.h>
>   #include <string.h>
> +#include <stdio.h>
>   #include <stdlib.h>
>   #include <errno.h>
>   
> @@ -33,6 +35,8 @@
>   #include <ofono/log.h>
>   #include <ofono/types.h>
>   
> +#include "ofono.h"
> +
>   #include "atutil.h"
>   #include "vendor.h"
>   
> @@ -654,3 +658,102 @@ int at_util_get_ipv4_address_and_netmask(const char *addrnetmask,
>   
>   	return ret;
>   }
> +
> +static int gemalto_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;

This looks to be exact same enumeration that 27.007 uses...?

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

Why don't you just simply pass the gemalto auth into this function 
directly instead of having to pass in the modem object?

> +	int len;
> +	/*
> +	 * 0x01: use sgauth (0=use cgauth)
> +	 * 0x02: pwd, user order (0=user, pwd order)
> +	 * 0x04: always use all: method, user, password
> +	 */

But certain combinations are not valid?  E.g. can we have a +CGAUTH with 
a wrong username/password order?

Have you considered simply writing a gemalto specific gprs.c driver that 
will use SGAUTH and using the default atmodem one on the sane devices?

> +
> +	int auth_type = gemalto_get_auth_type(auth_method);
> +
> +	if (auth_type != 0 && (!*username || !*password))
> +		return FALSE;
> +
> +	if (gemalto_auth & 0x01)
> +		len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
> +	else
> +		len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
> +
> +	buflen -= len;
> +
> +	switch(auth_type) {
> +	case 0:
> +
> +		if (gemalto_auth & 0x04)
> +			snprintf(buf+len, buflen, ",0,\"\",\"\"");
> +		else
> +			snprintf(buf+len, buflen, ",0");
> +
> +		break;
> +	case 1:
> +	case 2:
> +
> +		if (gemalto_auth & 0x02)
> +			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> +					auth_type, password, username);
> +		else
> +			snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> +					auth_type, username, password);
> +
> +		break;
> +	default:
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
> +void gemalto_get_cgdcont_command(struct ofono_modem *modem,

Err, why do you need the modem object?

> +			guint cid, enum ofono_gprs_proto proto, const char *apn,
> +							char *buf, guint buflen)

Why is CGDCONT specific to gemalto?  This looks pretty standard

> +{
> +	int len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
> +	buflen -= len;
> +
> +	/*
> +	 * 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
> +	 */
> +
> +	/*
> +	 * if apn is null, it will remove the context.
> +	 * but if apn is empty, it will create a context with empty apn
> +	 */
> +	if (!apn)
> +		return;
> +
> +	switch (proto) {
> +	case OFONO_GPRS_PROTO_IPV6:
> +		snprintf(buf+len, buflen, ",\"IPV6\",\"%s\"", apn);
> +		break;
> +	case OFONO_GPRS_PROTO_IPV4V6:
> +		snprintf(buf+len, buflen, ",\"IPV4V6\",\"%s\"", apn);
> +		break;
> +	case OFONO_GPRS_PROTO_IP:
> +		snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
> +		break;
> +	}
> +}
> diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
> index 7113a4cd..b74db9fe 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
> @@ -19,6 +20,8 @@
>    *
>    */
>   
> +struct ofono_modem;
> +

This should not be necessary if you follow the above advice...

>   enum at_util_sms_store {
>   	AT_UTIL_SMS_STORE_SM =	0,
>   	AT_UTIL_SMS_STORE_ME =	1,
> @@ -86,6 +89,15 @@ 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);
>   
> +gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> +				enum ofono_gprs_auth_method auth_method,
> +				const char *username, const char *password,
> +				char *buf, guint buflen);
> +
> +void gemalto_get_cgdcont_command(struct ofono_modem *modem,
> +			guint cid, enum ofono_gprs_proto proto, const char *apn,
> +						char *buf, guint buflen);
> +
>   struct cb_data {
>   	void *cb;
>   	void *data;
> @@ -115,6 +127,21 @@ static inline int at_util_convert_signal_strength(int strength)
>   	return result;
>   }
>   
> +static inline int at_util_convert_signal_strength_cesq(int strength_GSM,
> +					int strength_UTRAN, int strength_EUTRAN)
> +{
> +	int result = -1;
> +
> +	if (strength_GSM != 99)
> +		result = (strength_GSM * 100) / 63;
> +	else if (strength_UTRAN != 255)
> +		result = (strength_UTRAN * 100) / 96;
> +	else if (strength_EUTRAN != 255)
> +		result = (strength_EUTRAN * 100) / 97;
> +
> +	return result;
> +}
> +

How is this related to the current patch?

>   #define CALLBACK_WITH_FAILURE(cb, args...)		\
>   	do {						\
>   		struct ofono_error cb_e;		\
> diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
> index efa4e5fe..41de4197 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
> @@ -43,12 +44,65 @@
>   
>   struct lte_driver_data {
>   	GAtChat *chat;
> +	unsigned int vendor;
>   };
>   
> -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;

Why don't you just include a pointer to lte_driver_data and not store 
all this redundant stuff like vendor, chat, etc.

> +	const struct ofono_lte_default_attach_info *info;

And as I pointed out last time, you can't do this.  The core is not 
obligated to keep the object pointed to around after the driver method 
has been called.  So this might be pointing out into the ether, invalid 
memory, etc.

> +	unsigned int vendor;
> +	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,
> +		unsigned int vendor, struct ofono_modem *modem)
> +{
> +	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;
> +	cbd->vendor = vendor;
> +	cbd->modem = modem;
> +
> +	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;
>   
> @@ -58,27 +112,113 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *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;
> +
> +	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;
> +
> +	if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
> +
> +		if (!gemalto_get_auth_command(modem, 1, auth_method,
> +						info->username, info->password,
> +						buf, sizeof(buf)))
> +			goto error;
> +
> +	} else { /* default vendor*/
> +		len = snprintf(buf, buflen, "AT+CGAUTH=0,");
> +		buflen -= len;
> +
> +		switch (auth_method) {
> +		case OFONO_GPRS_AUTH_METHOD_PAP:
> +			snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
> +					cbd->info->username, cbd->info->password);
> +			break;
> +		case OFONO_GPRS_AUTH_METHOD_CHAP:
> +			snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
> +					cbd->info->username, cbd->info->password);
> +			break;
> +		case OFONO_GPRS_AUTH_METHOD_NONE:
> +			snprintf(buf + len, buflen, "0");
> +			break;
> +		}
> +	}
> +
> +	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;
> +
> +error:
> +	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);
> +	const char *proto;
> +	size_t len;
> +	struct ofono_modem *modem = ofono_lte_get_modem(lte);
> +	struct lte_cbd *cbd = lte_cb_data_new0(cb, data, ldd->chat, info,
> +							ldd->vendor, modem);
>   

Again, have you considered just writing a Gemalto specific LTE driver? 
The whole thing is only 140 lines of code right now.

>   	DBG("LTE config with APN: %s", 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\"");
> +	if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
> +		gemalto_get_cgdcont_command(modem, 1, info->proto, info->apn,
> +					buf, sizeof(buf));
> +	} else { /* default vendor*/
> +
> +		switch (info->proto) {
> +		case OFONO_GPRS_PROTO_IPV6:
> +			proto = "IPV6";
> +			break;
> +		case OFONO_GPRS_PROTO_IPV4V6:
> +			proto = "IPV4V6";
> +			break;
> +		case OFONO_GPRS_PROTO_IP:
> +			proto = "IP";
> +			break;
> +		}
> +
> +		len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
> +
> +		if (*info->apn)
> +			snprintf(buf+len, sizeof(buf)-len, ",\"%s\"",
> +								info->apn);
> +	}
>   
> -	/* We can't do much in case of failure so don't check response. */
>   	if (g_at_chat_send(ldd->chat, buf, NULL,
> -			at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
> +			at_lte_set_default_attach_info_cb,
> +			cbd, lte_cb_data_unref) > 0)
>   		return;
>   
> +	lte_cb_data_unref(cbd);
>   	CALLBACK_WITH_FAILURE(cb, data);
>   }
>   
> @@ -103,6 +243,7 @@ static int at_lte_probe(struct ofono_lte *lte, unsigned int vendor, void *data)
>   		return -ENOMEM;
>   
>   	ldd->chat = g_at_chat_clone(chat);
> +	ldd->vendor = vendor;
>   
>   	ofono_lte_set_data(lte, ldd);
>   
> 

Regards,
-Denis

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-14 18:04     ` Giacinto Cifelli
@ 2018-10-15 18:37       ` Denis Kenzior
  0 siblings, 0 replies; 10+ messages in thread
From: Denis Kenzior @ 2018-10-15 18:37 UTC (permalink / raw)
  To: ofono

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

Hi Giacinto,

>> Given that, polluting atutil.c with vendor-specific code is not a great
>> idea.  Even though you don't want to repeat the same code in lte.c and
>> gprs-context.c, it's not a big deal to do so, and the code is probably
>> easier to follow that way anyway.
> 
> What about introducing a vendor.c ?

I will mostly echo what Jonas already said.  There should be no vendor 
specific code in drivers/atmodem/atutil.c.  The VENDOR tweaks are really 
meant as just that.  Tweaks to AT commands that were broken by the 
vendor, or tweaks to behavior because the firmware says one thing but 
really means another.

Once you get into a situation where you have an entirely different set 
of AT commands being used, then you really need to consider using a 
separate driver.  Even if that means duplicating some code which is 
generic 27.007.  The generic 27.007 part can actually be put inside 
atutil.c, so you can avoid (some) code duplication that way.

Regards,
-denis

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-15 18:29   ` Denis Kenzior
@ 2018-10-15 18:59     ` Giacinto Cifelli
  2018-10-16  5:22     ` Giacinto Cifelli
  1 sibling, 0 replies; 10+ messages in thread
From: Giacinto Cifelli @ 2018-10-15 18:59 UTC (permalink / raw)
  To: ofono

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

Hi Denis,

> Again, have you considered just writing a Gemalto specific LTE driver?
> The whole thing is only 140 lines of code right now.
>

ok, but I just need the core to handle the additional properties.

Regards,
Giacinto

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-15 18:29   ` Denis Kenzior
  2018-10-15 18:59     ` Giacinto Cifelli
@ 2018-10-16  5:22     ` Giacinto Cifelli
  2018-10-16 18:06       ` Denis Kenzior
  1 sibling, 1 reply; 10+ messages in thread
From: Giacinto Cifelli @ 2018-10-16  5:22 UTC (permalink / raw)
  To: ofono

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

Hi Denis,

On Mon, Oct 15, 2018 at 8:48 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> On 10/13/2018 03:09 AM, Giacinto Cifelli wrote:
> > Added a vendor-specific extension for handling PDP context creation
> > and authentication, using the actual formats for the vendor.
> >
>
> No, please don't do this.  atutil is only for generic / utility code
> that is vendor neutral.  E.g. stuff described in 27.007.  We made one
> exception here for parsing CREG/CGREG and that is because some vendors
> just can't implement AT commands properly.

ok

>
> > The formating code is in atutils, because it is shared also with
> > gprs-context.
> > ---
> >   drivers/atmodem/atutil.c | 103 +++++++++++++++++++++++++
> >   drivers/atmodem/atutil.h |  27 +++++++
> >   drivers/atmodem/lte.c    | 161 ++++++++++++++++++++++++++++++++++++---
> >   3 files changed, 281 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
> > index 6f4e8a20..1201dbf3 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
> > @@ -26,6 +27,7 @@
> >   #include <glib.h>
> >   #include <gatchat.h>
> >   #include <string.h>
> > +#include <stdio.h>
> >   #include <stdlib.h>
> >   #include <errno.h>
> >
> > @@ -33,6 +35,8 @@
> >   #include <ofono/log.h>
> >   #include <ofono/types.h>
> >
> > +#include "ofono.h"
> > +
> >   #include "atutil.h"
> >   #include "vendor.h"
> >
> > @@ -654,3 +658,102 @@ int at_util_get_ipv4_address_and_netmask(const char *addrnetmask,
> >
> >       return ret;
> >   }
> > +
> > +static int gemalto_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;
>
> This looks to be exact same enumeration that 27.007 uses...?

yes, and?
I still need to convert the enum to int. Is there a function somewhere?

>
> > +}
> > +
> > +gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> > +                             enum ofono_gprs_auth_method auth_method,
> > +                             const char *username, const char *password,
> > +                             char *buf, guint buflen)
> > +{
> > +     int gemalto_auth = ofono_modem_get_integer(modem, "GemaltoAuthType");
>
> Why don't you just simply pass the gemalto auth into this function
> directly instead of having to pass in the modem object?

Because I need to pass further values in the future.
I still need handle the auto-provisioned modems.
I prefer that the calling function doesn't have to know how the buffer
is filled:
it will just receive the command.

>
> > +     int len;
> > +     /*
> > +      * 0x01: use sgauth (0=use cgauth)
> > +      * 0x02: pwd, user order (0=user, pwd order)
> > +      * 0x04: always use all: method, user, password
> > +      */
>
> But certain combinations are not valid?  E.g. can we have a +CGAUTH with
> a wrong username/password order?

yes, out of the 8 combinations possible, today I have counted 5 (the
R&D can be very creative sometimes),
If the point is to enumerate the configurations instead, this is how I
handled it in a previous submit, but was much worst
because there were switch-cases with 1, 2, or 3 labels together.
At least like this it is clear what the code is doing.

>
> Have you considered simply writing a gemalto specific gprs.c driver that
> will use SGAUTH and using the default atmodem one on the sane devices?

Even CGAUTH is not completely standard for some models, some require
all parameters for auth=NONE.
Besides, gemalto doesnt use the CID=0 as in the standard.

There are no sane and insane modems. At most standard and proprietary.

>
> > +
> > +     int auth_type = gemalto_get_auth_type(auth_method);
> > +
> > +     if (auth_type != 0 && (!*username || !*password))
> > +             return FALSE;
> > +
> > +     if (gemalto_auth & 0x01)
> > +             len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
> > +     else
> > +             len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
> > +
> > +     buflen -= len;
> > +
> > +     switch(auth_type) {
> > +     case 0:
> > +
> > +             if (gemalto_auth & 0x04)
> > +                     snprintf(buf+len, buflen, ",0,\"\",\"\"");
> > +             else
> > +                     snprintf(buf+len, buflen, ",0");
> > +
> > +             break;
> > +     case 1:
> > +     case 2:
> > +
> > +             if (gemalto_auth & 0x02)
> > +                     snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> > +                                     auth_type, password, username);
> > +             else
> > +                     snprintf(buf+len, buflen, ",%d,\"%s\",\"%s\"",
> > +                                     auth_type, username, password);
> > +
> > +             break;
> > +     default:
> > +             return FALSE;
> > +     }
> > +
> > +     return TRUE;
> > +}
> > +
> > +void gemalto_get_cgdcont_command(struct ofono_modem *modem,
>
> Err, why do you need the modem object?

As above, to get the autoprovisioning properties later.

>
> > +                     guint cid, enum ofono_gprs_proto proto, const char *apn,
> > +                                                     char *buf, guint buflen)
>
> Why is CGDCONT specific to gemalto?  This looks pretty standard

CID=1 for the default APN (for the current models).
And because for auto-provisioning models I need to return a totally
different command in some cases (like: 'AT').
And there are no cgdcont-building functions around.

>
> > +{
> > +     int len = snprintf(buf, buflen, "AT+CGDCONT=%u", cid);
> > +     buflen -= len;
> > +
> > +     /*
> > +      * 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
> > +      */
> > +
> > +     /*
> > +      * if apn is null, it will remove the context.
> > +      * but if apn is empty, it will create a context with empty apn
> > +      */
> > +     if (!apn)
> > +             return;
> > +
> > +     switch (proto) {
> > +     case OFONO_GPRS_PROTO_IPV6:
> > +             snprintf(buf+len, buflen, ",\"IPV6\",\"%s\"", apn);
> > +             break;
> > +     case OFONO_GPRS_PROTO_IPV4V6:
> > +             snprintf(buf+len, buflen, ",\"IPV4V6\",\"%s\"", apn);
> > +             break;
> > +     case OFONO_GPRS_PROTO_IP:
> > +             snprintf(buf+len, buflen, ",\"IP\",\"%s\"", apn);
> > +             break;
> > +     }
> > +}
> > diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
> > index 7113a4cd..b74db9fe 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
> > @@ -19,6 +20,8 @@
> >    *
> >    */
> >
> > +struct ofono_modem;
> > +
>
> This should not be necessary if you follow the above advice...
>
> >   enum at_util_sms_store {
> >       AT_UTIL_SMS_STORE_SM =  0,
> >       AT_UTIL_SMS_STORE_ME =  1,
> > @@ -86,6 +89,15 @@ 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);
> >
> > +gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
> > +                             enum ofono_gprs_auth_method auth_method,
> > +                             const char *username, const char *password,
> > +                             char *buf, guint buflen);
> > +
> > +void gemalto_get_cgdcont_command(struct ofono_modem *modem,
> > +                     guint cid, enum ofono_gprs_proto proto, const char *apn,
> > +                                             char *buf, guint buflen);
> > +
> >   struct cb_data {
> >       void *cb;
> >       void *data;
> > @@ -115,6 +127,21 @@ static inline int at_util_convert_signal_strength(int strength)
> >       return result;
> >   }
> >
> > +static inline int at_util_convert_signal_strength_cesq(int strength_GSM,
> > +                                     int strength_UTRAN, int strength_EUTRAN)
> > +{
> > +     int result = -1;
> > +
> > +     if (strength_GSM != 99)
> > +             result = (strength_GSM * 100) / 63;
> > +     else if (strength_UTRAN != 255)
> > +             result = (strength_UTRAN * 100) / 96;
> > +     else if (strength_EUTRAN != 255)
> > +             result = (strength_EUTRAN * 100) / 97;
> > +
> > +     return result;
> > +}
> > +
>
> How is this related to the current patch?

Sorry, I didn't see it. However I did submit a patch for it on the
21.09.2018 on which I have never received any feedback.


>
> >   #define CALLBACK_WITH_FAILURE(cb, args...)          \
> >       do {                                            \
> >               struct ofono_error cb_e;                \
> > diff --git a/drivers/atmodem/lte.c b/drivers/atmodem/lte.c
> > index efa4e5fe..41de4197 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
> > @@ -43,12 +44,65 @@
> >
> >   struct lte_driver_data {
> >       GAtChat *chat;
> > +     unsigned int vendor;
> >   };
> >
> > -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;
>
> Why don't you just include a pointer to lte_driver_data and not store
> all this redundant stuff like vendor, chat, etc.
>
> > +     const struct ofono_lte_default_attach_info *info;
>
> And as I pointed out last time, you can't do this.  The core is not
> obligated to keep the object pointed to around after the driver method
> has been called.  So this might be pointing out into the ether, invalid
> memory, etc.

yes, I have included the old version just for giving some context to
the patch I wanted commented.
I have changed this in the meanwhile.

>
> > +     unsigned int vendor;
> > +     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,
> > +             unsigned int vendor, struct ofono_modem *modem)
> > +{
> > +     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;
> > +     cbd->vendor = vendor;
> > +     cbd->modem = modem;
> > +
> > +     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;
> >
> > @@ -58,27 +112,113 @@ static void at_lte_set_default_attach_info_cb(gboolean ok, GAtResult *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;
> > +
> > +     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;
> > +
> > +     if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
> > +
> > +             if (!gemalto_get_auth_command(modem, 1, auth_method,
> > +                                             info->username, info->password,
> > +                                             buf, sizeof(buf)))
> > +                     goto error;
> > +
> > +     } else { /* default vendor*/
> > +             len = snprintf(buf, buflen, "AT+CGAUTH=0,");
> > +             buflen -= len;
> > +
> > +             switch (auth_method) {
> > +             case OFONO_GPRS_AUTH_METHOD_PAP:
> > +                     snprintf(buf + len, buflen, "1,\"%s\",\"%s\"",
> > +                                     cbd->info->username, cbd->info->password);
> > +                     break;
> > +             case OFONO_GPRS_AUTH_METHOD_CHAP:
> > +                     snprintf(buf + len, buflen, "2,\"%s\",\"%s\"",
> > +                                     cbd->info->username, cbd->info->password);
> > +                     break;
> > +             case OFONO_GPRS_AUTH_METHOD_NONE:
> > +                     snprintf(buf + len, buflen, "0");
> > +                     break;
> > +             }
> > +     }
> > +
> > +     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;
> > +
> > +error:
> > +     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);
> > +     const char *proto;
> > +     size_t len;
> > +     struct ofono_modem *modem = ofono_lte_get_modem(lte);
> > +     struct lte_cbd *cbd = lte_cb_data_new0(cb, data, ldd->chat, info,
> > +                                                     ldd->vendor, modem);
> >
>
> Again, have you considered just writing a Gemalto specific LTE driver?
> The whole thing is only 140 lines of code right now.

Ok.

>
> >       DBG("LTE config with APN: %s", 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\"");
> > +     if (ldd->vendor == OFONO_VENDOR_GEMALTO) {
> > +             gemalto_get_cgdcont_command(modem, 1, info->proto, info->apn,
> > +                                     buf, sizeof(buf));
> > +     } else { /* default vendor*/
> > +
> > +             switch (info->proto) {
> > +             case OFONO_GPRS_PROTO_IPV6:
> > +                     proto = "IPV6";
> > +                     break;
> > +             case OFONO_GPRS_PROTO_IPV4V6:
> > +                     proto = "IPV4V6";
> > +                     break;
> > +             case OFONO_GPRS_PROTO_IP:
> > +                     proto = "IP";
> > +                     break;
> > +             }
> > +
> > +             len = snprintf(buf, sizeof(buf), "AT+CGDCONT=0,\"%s\"", proto);
> > +
> > +             if (*info->apn)
> > +                     snprintf(buf+len, sizeof(buf)-len, ",\"%s\"",
> > +                                                             info->apn);
> > +     }
> >
> > -     /* We can't do much in case of failure so don't check response. */
> >       if (g_at_chat_send(ldd->chat, buf, NULL,
> > -                     at_lte_set_default_attach_info_cb, cbd, g_free) > 0)
> > +                     at_lte_set_default_attach_info_cb,
> > +                     cbd, lte_cb_data_unref) > 0)
> >               return;
> >
> > +     lte_cb_data_unref(cbd);
> >       CALLBACK_WITH_FAILURE(cb, data);
> >   }
> >
> > @@ -103,6 +243,7 @@ static int at_lte_probe(struct ofono_lte *lte, unsigned int vendor, void *data)
> >               return -ENOMEM;
> >
> >       ldd->chat = g_at_chat_clone(chat);
> > +     ldd->vendor = vendor;
> >
> >       ofono_lte_set_data(lte, ldd);
> >
> >
>
> Regards,
> -Denis

Regards,
Giacinto

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-16  5:22     ` Giacinto Cifelli
@ 2018-10-16 18:06       ` Denis Kenzior
  2018-10-18  7:57         ` Giacinto Cifelli
  0 siblings, 1 reply; 10+ messages in thread
From: Denis Kenzior @ 2018-10-16 18:06 UTC (permalink / raw)
  To: ofono

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

Hi Giacinto,
>>> +static int gemalto_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;
>>
>> This looks to be exact same enumeration that 27.007 uses...?
> 
> yes, and?
> I still need to convert the enum to int. Is there a function somewhere?

Right.  So what I'm hinting at is that this should be a generic utility 
method inside atutil.c (with no gemalto_ prefix) and you can simply use 
that.

> 
>>
>>> +}
>>> +
>>> +gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
>>> +                             enum ofono_gprs_auth_method auth_method,
>>> +                             const char *username, const char *password,
>>> +                             char *buf, guint buflen)
>>> +{
>>> +     int gemalto_auth = ofono_modem_get_integer(modem, "GemaltoAuthType");
>>
>> Why don't you just simply pass the gemalto auth into this function
>> directly instead of having to pass in the modem object?
> 
> Because I need to pass further values in the future.
> I still need handle the auto-provisioned modems.
> I prefer that the calling function doesn't have to know how the buffer
> is filled:
> it will just receive the command.

I wouldn't do it this way, but okay.  I don't much care how the vendor 
specific stuff looks :)  You may want to consider returning a newly 
allocated char * in this case to make the caller's job a bit easier.  I 
get a bit twitchy once the number of function arguments exceeds 5-6.

> 
>>
>>> +     int len;
>>> +     /*
>>> +      * 0x01: use sgauth (0=use cgauth)
>>> +      * 0x02: pwd, user order (0=user, pwd order)
>>> +      * 0x04: always use all: method, user, password
>>> +      */
>>
>> But certain combinations are not valid?  E.g. can we have a +CGAUTH with
>> a wrong username/password order?
> 
> yes, out of the 8 combinations possible, today I have counted 5 (the
> R&D can be very creative sometimes),
> If the point is to enumerate the configurations instead, this is how I
> handled it in a previous submit, but was much worst
> because there were switch-cases with 1, 2, or 3 labels together.
> At least like this it is clear what the code is doing.
> 

Okay, fair enough.  You might want to add a comment about which 
combinations are not possible.

>>
>> Have you considered simply writing a gemalto specific gprs.c driver that
>> will use SGAUTH and using the default atmodem one on the sane devices?
> 
> Even CGAUTH is not completely standard for some models, some require
> all parameters for auth=NONE.
> Besides, gemalto doesnt use the CID=0 as in the standard.
> 
> There are no sane and insane modems. At most standard and proprietary.

Lol, trust me there are :)  I've seen many insane ones and a few that 
are mostly sane.  The weird situation with +CGAUTH requiring all 
arguments can be VENDOR-ized in the atmodem lte driver easily.  But the 
use of +SGAUTH and stuff probably requires a separate vendor driver 
anyway.  So if you want to just lump this all into 
drivers/gemaltomodem/lte.c I'd be okay with that.

>>
>>> +                     guint cid, enum ofono_gprs_proto proto, const char *apn,
>>> +                                                     char *buf, guint buflen)
>>
>> Why is CGDCONT specific to gemalto?  This looks pretty standard
> 
> CID=1 for the default APN (for the current models).
> And because for auto-provisioning models I need to return a totally
> different command in some cases (like: 'AT').
> And there are no cgdcont-building functions around.
> 

I thought the default APN was cid=0.

Why don't we create a CGDCONT builder inside atutil.c and you can use 
that.  If you need some special ones then you can write a gemalto 
specific version later that can do something like:

gemalto_get_cgdcont()
{
	if (27007-compliant)
		return at_util_get_cgdcont();

	/* do stuff */
}

>>> +static inline int at_util_convert_signal_strength_cesq(int strength_GSM,
>>> +                                     int strength_UTRAN, int strength_EUTRAN)
>>> +{
>>> +     int result = -1;
>>> +
>>> +     if (strength_GSM != 99)
>>> +             result = (strength_GSM * 100) / 63;
>>> +     else if (strength_UTRAN != 255)
>>> +             result = (strength_UTRAN * 100) / 96;
>>> +     else if (strength_EUTRAN != 255)
>>> +             result = (strength_EUTRAN * 100) / 97;
>>> +
>>> +     return result;
>>> +}
>>> +
>>
>> How is this related to the current patch?
> 
> Sorry, I didn't see it. However I did submit a patch for it on the
> 21.09.2018 on which I have never received any feedback.
> 

There are too many patches flying about.  Just resubmit the relevant series.

Regards,
-Denis

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension
  2018-10-16 18:06       ` Denis Kenzior
@ 2018-10-18  7:57         ` Giacinto Cifelli
  0 siblings, 0 replies; 10+ messages in thread
From: Giacinto Cifelli @ 2018-10-18  7:57 UTC (permalink / raw)
  To: ofono

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

Hi Denis,

>
> I thought the default APN was cid=0.
>

In the standard yes.
I see in ofono code that the xmm7xxx modem uses the LTE atom as-is, so
maybe there is at least one modem that implements it.

Gemalto uses normally cid=1 and for some operators cid=3.
uBlox uses cid=1 and cid=4, depending on the models.
For other vendors, I don't know.

I have taken the rest of the suggestions in your message, and I am
going to resubmit shortly.

It would be good if someone could test on these xmm7xxx modems the
atmodem/lte atom, but I understand the lte attach authentication is
not a feature that everyone can access.
I am going to submit it anyway, and it can be amended in the future
when someone can test it.

> Regards,
> -Denis

Regards,
Giacinto

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-10-18  7:57 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-13  8:09 [RFC PATCH 0/1] atmodem/lte: added Gemalto vendor Giacinto Cifelli
2018-10-13  8:09 ` [RFC PATCH 1/1] atmodem/lte: Gemalto vendor specific extension Giacinto Cifelli
2018-10-14 17:56   ` Jonas Bonn
2018-10-14 18:04     ` Giacinto Cifelli
2018-10-15 18:37       ` Denis Kenzior
2018-10-15 18:29   ` Denis Kenzior
2018-10-15 18:59     ` Giacinto Cifelli
2018-10-16  5:22     ` Giacinto Cifelli
2018-10-16 18:06       ` Denis Kenzior
2018-10-18  7:57         ` Giacinto Cifelli

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.