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

[Jonas Bonn]

Hi Giacinto,

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

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.
---
Makefile.am | 5 +-
drivers/gemaltomodem/gemaltomodem.c | 2 +
drivers/gemaltomodem/gemaltomodem.h | 4 +
drivers/gemaltomodem/gemaltoutil.c | 106 +++++++++++++
drivers/gemaltomodem/gemaltoutil.h | 31 ++++
drivers/gemaltomodem/lte.c | 221 ++++++++++++++++++++++++++++
6 files changed, 368 insertions(+), 1 deletion(-)
create mode 100644 drivers/gemaltomodem/gemaltoutil.c
create mode 100644 drivers/gemaltomodem/gemaltoutil.h
create mode 100644 drivers/gemaltomodem/lte.c
diff --git a/Makefile.am b/Makefile.am
index e8e4ed95..dd0bc0bb 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -397,8 +397,11 @@ builtin_modules += gemaltomodem
builtin_sources += drivers/atmodem/atutil.h \
drivers/gemaltomodem/gemaltomodem.h \
drivers/gemaltomodem/gemaltomodem.c \
+ drivers/gemaltomodem/gemaltoutil.h \
+ drivers/gemaltomodem/gemaltoutil.c \
drivers/gemaltomodem/location-reporting.c \
- drivers/gemaltomodem/voicecall.c
+ drivers/gemaltomodem/voicecall.c \
+ drivers/gemaltomodem/lte.c
builtin_modules += xmm7modem
builtin_sources += drivers/atmodem/atutil.h \
diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
index 4818ac66..459c3583 100644
--- a/drivers/gemaltomodem/gemaltomodem.c
+++ b/drivers/gemaltomodem/gemaltomodem.c
@@ -37,12 +37,14 @@ static int gemaltomodem_init(void)
{
gemalto_location_reporting_init();
gemalto_voicecall_init();
+ gemalto_lte_init();
return 0;
}
static void gemaltomodem_exit(void)
{
+ gemalto_lte_exit();
gemalto_voicecall_exit();
gemalto_location_reporting_exit();
}
diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
index 27b1460e..9f285563 100644
--- a/drivers/gemaltomodem/gemaltomodem.h
+++ b/drivers/gemaltomodem/gemaltomodem.h
@@ -21,9 +21,13 @@
*/
#include <drivers/atmodem/atutil.h>
+#include "gemaltoutil.h"
extern void gemalto_location_reporting_init();
extern void gemalto_location_reporting_exit();
extern void gemalto_voicecall_init();
extern void gemalto_voicecall_exit();
+
+extern void gemalto_lte_init();
+extern void gemalto_lte_exit();
diff --git a/drivers/gemaltomodem/gemaltoutil.c b/drivers/gemaltomodem/gemaltoutil.c
new file mode 100644
index 00000000..d8c08e16
--- /dev/null
+++ b/drivers/gemaltomodem/gemaltoutil.c
@@ -0,0 +1,106 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2018 Gemalto M2M
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <glib.h>
+#include <gatchat.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE

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

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

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

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

Abbreviating ALWAYS to ALW is awkward...

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

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

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

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

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

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

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

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

+
+ return at_get_cgdcont_command(cid, proto, apn);
+}
diff --git a/drivers/gemaltomodem/gemaltoutil.h b/drivers/gemaltomodem/gemaltoutil.h
new file mode 100644
index 00000000..d653f61d
--- /dev/null
+++ b/drivers/gemaltomodem/gemaltoutil.h
@@ -0,0 +1,31 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2018 Gemalto M2M
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+struct ofono_modem;

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

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

This enum isn't defined either.

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

Why do you need to delay the registration?

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

/Jonas

[Jonas Bonn]

Hi Giacinto,

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

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

/Jonas

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

[Jonas Bonn]

Hi Jonas,

Post by Jonas Bonn
Hi Giacinto,

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

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

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

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

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

/Jonas

[Slava Monich]

Post by Jonas Bonn
...

+#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,
+};

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

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

Cheers,
-Slava

[Jonas Bonn]

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

[Denis Kenzior]

Hi Jonas,

Post by Jonas Bonn
#define AUTH_F_USE_SGATH (1<<0)
#define AUTH_F_SWAP_CREDENTIALS (1<<1)
#define AUTH_F_ALWAYS_ALL_PARAMS (1<<2)

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

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

Post by Jonas Bonn
Abbreviating ALWAYS to ALW is awkward...

+1

Post by Jonas Bonn

+
+struct ofono_modem;

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

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

Post by Jonas Bonn

+static int gemalto_lte_probe(struct ofono_lte *lte, unsigned int vendor,
+                                void *data)
+{
+    GAtChat *chat = data;
+    struct gemalto_lte_driver_data *ldd;
+
+    ldd = g_new0(struct gemalto_lte_driver_data, 1);
+
+    ldd->chat = g_at_chat_clone(chat);
+
+    ofono_lte_set_data(lte, ldd);
+
+    g_idle_add(gemalto_lte_delayed_register, lte);

Why do you need to delay the registration?

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

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Post by Jonas Bonn

+char *gemalto_get_cgdcont_command(struct ofono_modem *modem, guint cid,
+ enum ofono_gprs_proto proto, const char *apn)
+{
+ /*
+ * For future extension: verify if the module supports automatic
+ * context provisioning and if so, also if there is a manual override
+ * This is an ofono_modem_get_integer property
+ */
+
+ /* for now. Later to consider modules where the LTE attach CID=3 */
+ if (cid==0)
+ cid=1;

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

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

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

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

Regards,
-Denis

[Denis Kenzior]

Hi Giacinto,

Hi Denis,

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

The function is to be called also from gprs-context, therefore passing
0 it knows it has to check whether it has to use 1 or 3 or something
else.

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

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

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

Regards,
-Denis