lte atom auth and IP protocol
v7 cover lte atom auth and IP protocol
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
Signature · 1 line
2.17.1
PATCH v7 1/7 lte-api: protocol and authentication properties
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.
--- doc/lte-api.txt | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)
doc/lte-api.txt +39 -0
@@ -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.
Signature · 1 line
2.17.1
PATCH v7 2/7 lte.h: added proto and authentication handling
In reply to Giacinto CifelliThe 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
--- include/lte.h | 5 +++++ 1 file changed, 5 insertions(+)
include/lte.h +5 -0
@@ -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);
Signature · 1 line
2.17.1
PATCH v7 3/7 lte: protocol and authentication for default ctx
In reply to Giacinto CifelliMany 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.
--- src/lte.c | 222 ++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 167 insertions(+), 55 deletions(-)
src/lte.c +167 -55
@@ -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, <e->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, + <e->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 (<e->info, <e->pending_info,sizeof (lte->info)); + + reply =dbus_message_new_method_return (lte->pending); +__ofono_dbus_pending_reply (<e->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 (<e->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, <e->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 (<e->pending_info, <e->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, <e->pending_info, + lte_set_default_attach_info_cb, lte); + +return NULL ; }static const GDBusMethodTable lte_methods[] = {
Signature · 1 line
2.17.1
PATCH v7 4/7 atmodem/atutil: shared functions for cgdcont
In reply to Giacinto Cifelliincluded 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(+)
drivers/atmodem/atutil.c +49 -0
@@ -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; +}
drivers/atmodem/atutil.h +6 -0
@@ -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;
Signature · 1 line
2.17.1
PATCH v7 5/7 atmodem/lte: proto and authentication handling
In reply to Giacinto CifelliThe ofono_lte_default_attach_info now handles also the protocol and the authentication method, username and password.
--- drivers/atmodem/lte.c | 119 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 102 insertions(+), 17 deletions(-)
drivers/atmodem/lte.c +102 -17
@@ -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)
Signature · 1 line
2.17.1
PATCH v7 6/7 gemalto/lte: Gemalto vendor lte atom
In reply to Giacinto CifelliAdded 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.
--- 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
Makefile.am +4 -1
@@ -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 \
drivers/gemaltomodem/gemaltomodem.c +2 -0
@@ -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 (); }
drivers/gemaltomodem/gemaltomodem.h +4 -0
@@ -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 ();
drivers/gemaltomodem/gemaltoutil.c new file +106 -0
106 lines not on this page · raw message
drivers/gemaltomodem/gemaltoutil.h new file +31 -0
@@ -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); +
drivers/gemaltomodem/lte.c new file +221 -0
221 lines not on this page · raw message
Signature · 1 line
2.17.1
Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom
Hi Giacinto,
This looks pretty good to me... Some comments below.
Quoted 104 lines · Giacinto Cifelli
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 <[email protected]>Co-authored-by Sebastian Arnd <[email protected]>--- 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.ccreate mode 100644drivers/gemaltomodem/gemaltoutil.hcreate mode 100644drivers/gemaltomodem/lte.cMakefile.am +4
-1@@ -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 \drivers/gemaltomodem/gemaltomodem.c +2
-0@@ -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 (); }drivers/gemaltomodem/gemaltomodem.h +4
-0@@ -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 ();drivers/gemaltomodem/gemaltoutil.c new file +23
-0@@ -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>
drivers/gemaltomodem/gemaltoutil.c line 33
Quoted 10 lines
drivers/gemaltomodem/gemaltoutil.c new file +10
-0+#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.
drivers/gemaltomodem/gemaltoutil.c line 45
Quoted 12 lines
drivers/gemaltomodem/gemaltoutil.c new file +12
-0+#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?
drivers/gemaltomodem/gemaltoutil.c line 51
Quoted 6 lines
drivers/gemaltomodem/gemaltoutil.c new file +6
-0+ +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.
drivers/gemaltomodem/gemaltoutil.c line 63
Quoted 12 lines
drivers/gemaltomodem/gemaltoutil.c new file +12
-0+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).
Quoted 30 lines
drivers/gemaltomodem/gemaltoutil.c new file +30
-0+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)
drivers/gemaltomodem/gemaltoutil.c line 103
Quoted 10 lines
drivers/gemaltomodem/gemaltoutil.c new file +10
-0+{ +/* +* 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...???
Quoted 21 lines
drivers/gemaltomodem/gemaltoutil.c new file +3
-0+ +return at_get_cgdcont_command (cid, proto, apn); +}drivers/gemaltomodem/gemaltoutil.h new file +12
-0@@ -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
drivers/gemaltomodem/gemaltoutil.h line 22
Quoted 10 lines
drivers/gemaltomodem/gemaltoutil.h new file +10
-0+* 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.
drivers/gemaltomodem/gemaltoutil.h line 25
Quoted 3 lines
drivers/gemaltomodem/gemaltoutil.h new file +3
-0+ +char *gemalto_get_auth_command(struct ofono_modem *modem, int cid, + enum ofono_gprs_auth_method auth_method,
This enum isn't defined either.
Quoted 192 lines
drivers/gemaltomodem/gemaltoutil.h new file +6
-0+ 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); +drivers/gemaltomodem/lte.c new file +180
-0@@ -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) +{
drivers/gemaltomodem/lte.c line 190
Quoted 10 lines
drivers/gemaltomodem/lte.c new file +10
-0+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?
Quoted 22 lines
drivers/gemaltomodem/lte.c new file +21
-0+ +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, +};
drivers/gemaltomodem/lte.c line 221
Quoted 10 lines
drivers/gemaltomodem/lte.c new file +10
-0+ +void gemalto_lte_init (void ) +{ +ofono_lte_driver_register (&driver); +} + +void gemalto_lte_exit (void ) +{ +ofono_lte_driver_unregister (&driver); +}
/Jonas
Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom
Quoted 17 lines · Jonas Bonn
...
drivers/gemaltomodem/gemaltoutil.c line 45
Quoted 12 lines
drivers/gemaltomodem/gemaltoutil.c new file +12
-0+#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:
Quoted 4 lines
#define
AUTH_F_USE_SGATH(1<<0)
#defineAUTH_F_SWAP_CREDENTIALS(1<<1)
#defineAUTH_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 6/7] gemalto/lte: Gemalto vendor lte atom
In reply to Jonas BonnHi 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?
Quoted 138 lines · Jonas Bonn
Hi Giacinto,
This looks pretty good to me... Some comments below.
Quoted 104 lines · Giacinto Cifelli
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 <[email protected]>Co-authored-by Sebastian Arnd <[email protected]>--- 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.ccreate mode 100644drivers/gemaltomodem/gemaltoutil.hcreate mode 100644drivers/gemaltomodem/lte.cMakefile.am +4
-1@@ -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 \drivers/gemaltomodem/gemaltomodem.c +2
-0@@ -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 (); }drivers/gemaltomodem/gemaltomodem.h +4
-0@@ -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 ();drivers/gemaltomodem/gemaltoutil.c new file +23
-0@@ -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> drivers/gemaltomodem/gemaltoutil.c line 33
Quoted 10 lines
drivers/gemaltomodem/gemaltoutil.c new file +10
-0+#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.hdirectly.drivers/gemaltomodem/gemaltoutil.c line 45
Quoted 12 lines
drivers/gemaltomodem/gemaltoutil.c new file +12
-0+#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:
Quoted 3 lines
#define
AUTH_F_USE_SGATH(1<<0)
#defineAUTH_F_SWAP_CREDENTIALS(1<<1)
#defineAUTH_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.
Quoted 2 lines
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.
Quoted 6 lines
Furthermore, considering that these flags are supposed to be set in
udevng.cor the gemalto plugin, these flags really need to be defined elsewhere...include/gemalto.h, perhaps?
Quoted 10 lines
drivers/gemaltomodem/gemaltoutil.c line 51
Quoted 6 lines
drivers/gemaltomodem/gemaltoutil.c new file +6
-0+ +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_methodandauth_typeto confuse withgemalto_auth.
ok, I will call them GemaltoAuthFlags.
Quoted 19 lines
Quoted 4 lines
drivers/gemaltomodem/gemaltoutil.c new file +2
-0+int len; +int auth_type =at_get_auth_type (auth_method);drivers/gemaltomodem/gemaltoutil.c line 63
Quoted 10 lines
drivers/gemaltomodem/gemaltoutil.c new file +10
-0+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_typevariable, just call the function directly in the parameter list. Rename the function toat_auth_type_from_method()(or similar) so that it's obvious what it does.
Quoted 3 lines
(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.
Quoted 38 lines
Quoted 38 lines
drivers/gemaltomodem/gemaltoutil.c new file +37
-0+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 +*/ +
Quoted 8 lines
drivers/gemaltomodem/gemaltoutil.c line 60
Quoted 3 lines
drivers/gemaltomodem/gemaltoutil.c new file +3
-0+/* 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.
Quoted 35 lines
Quoted 22 lines
drivers/gemaltomodem/gemaltoutil.c new file +3
-0+ +return at_get_cgdcont_command (cid, proto, apn); +}drivers/gemaltomodem/gemaltoutil.h new file +12
-0@@ -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 drivers/gemaltomodem/gemaltoutil.h line 22
Quoted 10 lines
drivers/gemaltomodem/gemaltoutil.h new file +10
-0+* 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.hhere.
Quoted 5 lines
drivers/gemaltomodem/gemaltoutil.h line 25
Quoted 3 lines
drivers/gemaltomodem/gemaltoutil.h new file +3
-0+ +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.
Quoted 232 lines
Quoted 193 lines
drivers/gemaltomodem/gemaltoutil.h new file +6
-0+ 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); +drivers/gemaltomodem/lte.c new file +180
-0@@ -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) +{drivers/gemaltomodem/lte.c line 190
Quoted 10 lines
drivers/gemaltomodem/lte.c new file +10
-0+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?
Quoted 26 lines
drivers/gemaltomodem/lte.c new file +26
-0+ +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); +}
Quoted 8 lines
drivers/gemaltomodem/lte.c line 221
Quoted 6 lines
drivers/gemaltomodem/lte.c new file +5
-0+ +void gemalto_lte_exit (void ) +{ +ofono_lte_driver_unregister (&driver); +}/Jonas
Giacinto
Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom
Quoted 6 lines · Giacinto Cifelli
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
In reply to Giacinto CifelliHi Giacinto,
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
drivers/gemaltomodem/gemaltoutil.c new file +9
-0+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 +*/ +Quoted 8 lines
drivers/gemaltomodem/gemaltoutil.c line 60
Quoted 3 lines
drivers/gemaltomodem/gemaltoutil.c new file +3
-0+/* 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...???
Quoted 6 lines
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
Hi Denis,
Quoted 10 lines
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)...
Quoted 3 lines
Regards,
-Denis
Regards,
Giacinto
Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom
Hi Giacinto,
Quoted 7 lines · Giacinto Cifelli
Hi Denis,
Quoted 6 lines
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.
Quoted 8 lines
Quoted 4 lines
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?
Quoted 5 lines
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
Hi Denis,
Quoted 23 lines · Denis Kenzior
Hi Giacinto,
Quoted 7 lines · Giacinto Cifelli
Hi Denis,
Quoted 6 lines
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.
Quoted 8 lines
Quoted 4 lines
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?
Quoted 8 lines
Quoted 5 lines
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.
Quoted 3 lines
Regards,
-Denis
Regards,
Giacinto
Re: [PATCH v7 6/7] gemalto/lte: Gemalto vendor lte atom
In reply to Jonas BonnHi Jonas,
Quoted 5 lines
Using an enumeration to name flags feels awkward. I would #define these:
#define
AUTH_F_USE_SGATH(1<<0)
#defineAUTH_F_SWAP_CREDENTIALS(1<<1)
#defineAUTH_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.
Quoted 2 lines
Abbreviating ALWAYS to ALW is awkward...
+1
Quoted 5 lines
drivers/gemaltomodem/gemaltoutil.h line 22
Quoted 2 lines
drivers/gemaltomodem/gemaltoutil.h new file +2
-0+ +struct ofono_modem ;I think you might as well just pull in
modem.hhere.
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 ;)
Quoted 7 lines
Quoted 7 lines
drivers/gemaltomodem/lte.c new file +7
-0+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);
Quoted 8 lines
drivers/gemaltomodem/lte.c line 190
Quoted 6 lines
drivers/gemaltomodem/lte.c new file +6
-0+ + 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
PATCH v7 7/7 plugins/gemalto: use Gemalto lte atom
In reply to Giacinto CifelliSwitch 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(-)
plugins/gemalto.c +13 -3
@@ -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)
Signature · 1 line
2.17.1
Re: [PATCH v7 7/7] plugins/gemalto: use Gemalto lte atom
Hi Giacinto,
Quoted 31 lines · Giacinto Cifelli
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(-)plugins/gemalto.c +7
-0@@ -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);
plugins/gemalto.c static void gemalto_post_sim(struct ofono_modem *modem) · line 623
Quoted 10 lines
plugins/gemalto.c +5
-3ofono_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
Quoted 5 lines
plugins/gemalto.c +1
-0+ } }static void gemalto_post_online (struct ofono_modem *modem)
Re: [PATCH v7 7/7] plugins/gemalto: use Gemalto lte atom
Hi Jonas,
Quoted 37 lines · Jonas Bonn
Hi Giacinto,
Quoted 33 lines · Giacinto Cifelli
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(-)plugins/gemalto.c +7
-0@@ -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);
Quoted 11 lines
plugins/gemalto.c static void gemalto_post_sim(struct ofono_modem *modem) · line 623
Quoted 8 lines
plugins/gemalto.c +5
-3-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.
Quoted 2 lines
/Jonas
Regards,
Giacinto
Re: [PATCH v7 7/7] plugins/gemalto: use Gemalto lte atom
Quoted 52 lines · Giacinto Cifelli
Hi Jonas,
Quoted 37 lines · Jonas Bonn
Hi Giacinto,
Quoted 33 lines · Giacinto Cifelli
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(-)plugins/gemalto.c +7
-0@@ -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);Quoted 11 lines
plugins/gemalto.c static void gemalto_post_sim(struct ofono_modem *modem) · line 623
Quoted 8 lines
plugins/gemalto.c +5
-3-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?
Quoted 3 lines
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
hi,
Quoted 62 lines · On Fri, 19 Oct 2018, 07:57 Jonas Bonn, <[email protected]> wrote:
Quoted 26 lines · Giacinto Cifelli
Hi Jonas,
Quoted 23 lines · Jonas Bonn
Hi Giacinto,
Quoted 19 lines · Giacinto Cifelli
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(-)plugins/gemalto.c +0
-0@@ -63,6 +63,13 @@ static const char *none_prefix[] = { NULL };static const char *sctm_prefix[] = {"^SCTM:" ,NULL };static const char *sbv_prefix[] = {"^SBV:" ,NULL };Quoted 11 lines
Quoted 11 lines
plugins/gemalto.c static void gemalto_post_sim(struct ofono_modem
Quoted 11 lines
plugins/gemalto.c +7
-0+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)
Quoted 10 lines
Quoted 10 lines
plugins/gemalto.c static void gemalto_post_sim(struct ofono_modem *modem)
Quoted 10 lines
plugins/gemalto.c +4
-3ofono_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);Quoted 8 lines
Quoted 4 lines
plugins/gemalto.c static void gemalto_post_sim(struct ofono_modem *modem) · line 623
Quoted 1 line
plugins/gemalto.c +1
-0+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 iserror-prone.
Quoted 8 lines
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 amodel_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.
Quoted 2 lines
/Jonas
Giacinto