[RFC PATCH 0/4] gemalto: USB ethernet data path for ELS81x

[RFC PATCH 0/4] gemalto: USB ethernet data path for ELS81x

[Sergey Matyukevich]

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

Hi Denis and all,

This patch series suggests several updates and fixes for gemalto modems,
in particular for ELS81x device. Major change enables data path via USB
ethernet interface for modems supporting this feature.

Before submitting this patch series, I figured out that similar changes
have been already submitted to ofono mailing list back in 2017. At that
time they were aimed at PLS8 device:

https://lists.syncevolution.org/hyperkitty/list/ofono(a)ofono.org/thread/CHQZ2LY6D7J75MLYENACM6JDFHNN5GJ5/#3JB3RF5WVW2U6R65UHA6POSRROUNV6ET

It looks like not all the pieces from the original patches finally
landed in the ofono code base. I am not sure why they were not
merged, so I marked this series as RFC.

Regards,
Sergey

Sergey Matyukevich (4):
  drivers: gemalto: add gprs-context driver
  plugins: udevng: detect gemalto network interfaces
  plugins: gemalto: add optional gprs-context driver
  plugins: gemalto: fix incomplete at-chat shutdown

 Makefile.am                         |   3 +-
 drivers/gemaltomodem/gemaltomodem.c |   4 +-
 drivers/gemaltomodem/gemaltomodem.h |   3 +
 drivers/gemaltomodem/gprs-context.c | 280 ++++++++++++++++++++++++++++
 plugins/gemalto.c                   |  20 +-
 plugins/udevng.c                    |  14 +-
 6 files changed, 318 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gemaltomodem/gprs-context.c

-- 
2.28.0

[RFC PATCH 1/4] drivers: gemalto: add gprs-context driver

[Sergey Matyukevich]

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

Some gemalto modems provide USB ethernet interfaces for data path.
Implement gprs-context driver for such modems to send data via
USB ethernet rather than fallback to PPP.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
---
 Makefile.am                         |   3 +-
 drivers/gemaltomodem/gemaltomodem.c |   4 +-
 drivers/gemaltomodem/gemaltomodem.h |   3 +
 drivers/gemaltomodem/gprs-context.c | 280 ++++++++++++++++++++++++++++
 4 files changed, 288 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gemaltomodem/gprs-context.c

diff --git a/Makefile.am b/Makefile.am
index fbb0eff4..67c3bcbf 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -482,7 +482,8 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/gemaltomodem/gemaltomodem.h \
 			drivers/gemaltomodem/gemaltomodem.c \
 			drivers/gemaltomodem/location-reporting.c \
-			drivers/gemaltomodem/voicecall.c
+			drivers/gemaltomodem/voicecall.c \
+			drivers/gemaltomodem/gprs-context.c
 
 builtin_modules += xmm7modem
 builtin_sources += drivers/atmodem/atutil.h \
diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
index 4818ac66..4b20dd1b 100644
--- a/drivers/gemaltomodem/gemaltomodem.c
+++ b/drivers/gemaltomodem/gemaltomodem.c
@@ -36,6 +36,7 @@
 static int gemaltomodem_init(void)
 {
 	gemalto_location_reporting_init();
+	gemalto_gprs_context_init();
 	gemalto_voicecall_init();
 
 	return 0;
@@ -43,8 +44,9 @@ static int gemaltomodem_init(void)
 
 static void gemaltomodem_exit(void)
 {
-	gemalto_voicecall_exit();
 	gemalto_location_reporting_exit();
+	gemalto_gprs_context_exit();
+	gemalto_voicecall_exit();
 }
 
 OFONO_PLUGIN_DEFINE(gemaltomodem, "Gemalto modem driver", VERSION,
diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
index 27b1460e..dc0d346b 100644
--- a/drivers/gemaltomodem/gemaltomodem.h
+++ b/drivers/gemaltomodem/gemaltomodem.h
@@ -27,3 +27,6 @@ extern void gemalto_location_reporting_exit();
 
 extern void gemalto_voicecall_init();
 extern void gemalto_voicecall_exit();
+
+extern void gemalto_gprs_context_init();
+extern void gemalto_gprs_context_exit();
diff --git a/drivers/gemaltomodem/gprs-context.c b/drivers/gemaltomodem/gprs-context.c
new file mode 100644
index 00000000..be3de537
--- /dev/null
+++ b/drivers/gemaltomodem/gprs-context.c
@@ -0,0 +1,280 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2020 Sergey Matyukevich. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+#include <sys/stat.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/gprs-context.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+#include "gattty.h"
+
+#include "gemaltomodem.h"
+
+static const char *none_prefix[] = { NULL };
+
+struct gprs_context_data {
+	GAtChat *chat;
+	unsigned int active_context;
+	ofono_gprs_context_cb_t cb;
+	void *cb_data;
+};
+
+static void cgact_enable_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	struct ofono_modem *modem;
+	const char *interface;
+	char buf[64];
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		struct ofono_error error;
+
+		gcd->active_context = 0;
+
+		decode_at_error(&error, g_at_result_final_response(result));
+		gcd->cb(&error, gcd->cb_data);
+
+		return;
+	}
+
+	snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
+	g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
+
+	modem = ofono_gprs_context_get_modem(gc);
+	interface = ofono_modem_get_string(modem, "NetworkInterface");
+	ofono_gprs_context_set_interface(gc, interface);
+
+	/* Use DHCP */
+	ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
+
+	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+}
+
+static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[64];
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		struct ofono_error error;
+
+		gcd->active_context = 0;
+
+		decode_at_error(&error, g_at_result_final_response(result));
+		gcd->cb(&error, gcd->cb_data);
+
+		return;
+	}
+
+	snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				cgact_enable_cb, gc, NULL) == 0)
+		goto error;
+
+	return;
+
+error:
+	CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+}
+
+static void gemalto_gprs_activate_primary(struct ofono_gprs_context *gc,
+				const struct ofono_gprs_primary_context *ctx,
+				ofono_gprs_context_cb_t cb, void *data)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
+	int len = 0;
+
+	DBG("cid %u", ctx->cid);
+
+	/* IPv6 support not implemented */
+	if (ctx->proto != OFONO_GPRS_PROTO_IP)
+		goto error;
+
+	gcd->active_context = ctx->cid;
+	gcd->cb_data = data;
+	gcd->cb = cb;
+
+	len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"", ctx->cid);
+
+	if (ctx->apn)
+		snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"", ctx->apn);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				cgdcont_enable_cb, gc, NULL) == 0)
+		goto error;
+
+	return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void cgact_disable_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[64];
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
+		return;
+	}
+
+	snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
+	g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
+
+	gcd->active_context = 0;
+
+	CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+}
+
+static void gemalto_gprs_deactivate_primary(struct ofono_gprs_context *gc,
+						unsigned int cid,
+						ofono_gprs_context_cb_t cb,
+						void *data)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	char buf[64];
+
+	DBG("cid %u", cid);
+
+	gcd->cb = cb;
+	gcd->cb_data = data;
+
+	snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", cid);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				cgact_disable_cb, gc, NULL) == 0)
+		goto error;
+
+	return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, data);
+
+}
+
+static void cgev_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	GAtResultIter iter;
+	const char *event;
+	gint cid;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CGEV:"))
+		return;
+
+	if (!g_at_result_iter_next_unquoted_string(&iter, &event))
+		return;
+
+	if (g_str_has_prefix(event, "NW PDN DEACT"))
+		sscanf(event, "%*s %*s %*s %u", &cid);
+	else if (g_str_has_prefix(event, "NW DEACT"))
+		sscanf(event, "%*s %*s %u", &cid);
+	else
+		return;
+
+	DBG("cid %d, active cid: %d", cid, gcd->active_context);
+
+	if ((unsigned int) cid != gcd->active_context)
+		return;
+
+	ofono_gprs_context_deactivated(gc, gcd->active_context);
+	gcd->active_context = 0;
+}
+
+static int gemalto_gprs_context_probe(struct ofono_gprs_context *gc,
+					unsigned int vendor, void *data)
+{
+	GAtChat *chat = data;
+	struct gprs_context_data *gcd;
+
+	DBG("");
+
+	gcd = g_try_new0(struct gprs_context_data, 1);
+	if (gcd == NULL)
+		return -ENOMEM;
+
+	gcd->chat = g_at_chat_clone(chat);
+
+	ofono_gprs_context_set_data(gc, gcd);
+	g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
+
+	return 0;
+}
+
+static void gemalto_gprs_context_remove(struct ofono_gprs_context *gc)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+
+	DBG("");
+
+	ofono_gprs_context_set_data(gc, NULL);
+	g_at_chat_unref(gcd->chat);
+	g_free(gcd);
+}
+
+static const struct ofono_gprs_context_driver driver = {
+	.name			= "gemaltomodem",
+	.probe			= gemalto_gprs_context_probe,
+	.remove			= gemalto_gprs_context_remove,
+	.activate_primary	= gemalto_gprs_activate_primary,
+	.deactivate_primary	= gemalto_gprs_deactivate_primary,
+};
+
+void gemalto_gprs_context_init(void)
+{
+	ofono_gprs_context_driver_register(&driver);
+}
+
+void gemalto_gprs_context_exit(void)
+{
+	ofono_gprs_context_driver_unregister(&driver);
+}
-- 
2.28.0

Re: [RFC PATCH 1/4] drivers: gemalto: add gprs-context driver

[Denis Kenzior]

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

Hi Sergey,

On 8/15/20 4:43 PM, Sergey Matyukevich wrote:
> Some gemalto modems provide USB ethernet interfaces for data path.
> Implement gprs-context driver for such modems to send data via
> USB ethernet rather than fallback to PPP.
> 
> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> ---
>   Makefile.am                         |   3 +-
>   drivers/gemaltomodem/gemaltomodem.c |   4 +-
>   drivers/gemaltomodem/gemaltomodem.h |   3 +
>   drivers/gemaltomodem/gprs-context.c | 280 ++++++++++++++++++++++++++++
>   4 files changed, 288 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/gemaltomodem/gprs-context.c
> 

This looks fine to me.  My only 2 nitpicks are:

1. We do not use S-o-B, so feel free to drop those in the future or I will just 
drop them when applying.

> +
> +static int gemalto_gprs_context_probe(struct ofono_gprs_context *gc,
> +					unsigned int vendor, void *data)
> +{
> +	GAtChat *chat = data;
> +	struct gprs_context_data *gcd;
> +
> +	DBG("");
> +
> +	gcd = g_try_new0(struct gprs_context_data, 1);
> +	if (gcd == NULL)
> +		return -ENOMEM;
> +

2. We now prefer to use g_new0 for small structure allocations as these can't 
really fail on Linux userspace in practice.  Keeps the code shorter as well.

> +	gcd->chat = g_at_chat_clone(chat);
> +

Regards,
-Denis

[RFC PATCH 2/4] plugins: udevng: detect gemalto network interfaces

[Sergey Matyukevich]

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

Some gemalto modems, including ELS81x, may provide more than one
USB ethernet interface. Detect and save both network interfaces
rather than the last one.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
---
 plugins/udevng.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 7e6a3ab7..839fabdb 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1162,7 +1162,7 @@ static gboolean setup_ublox(struct modem_info *modem)
 static gboolean setup_gemalto(struct modem_info* modem)
 {
 	const char *app = NULL, *gps = NULL, *mdm = NULL,
-		*net = NULL, *qmi = NULL;
+		*net = NULL, *qmi = NULL, *net2 = NULL;
 
 	GSList *list;
 
@@ -1197,9 +1197,14 @@ static gboolean setup_gemalto(struct modem_info* modem)
 			else if (g_strcmp0(info->number, "04") == 0)
 				gps = info->devnode;
 		}
+
 		if (g_strcmp0(info->interface, "2/6/0") == 0) {
-			if (g_strcmp0(info->subsystem, "net") == 0)
-				net = info->devnode;
+			if (g_strcmp0(info->subsystem, "net") == 0) {
+				if (g_strcmp0(info->number, "0a") == 0)
+					net = info->devnode;
+				if (g_strcmp0(info->number, "0c") == 0)
+					net2 = info->devnode;
+			}
 		}
 	}
 
@@ -1216,6 +1221,9 @@ static gboolean setup_gemalto(struct modem_info* modem)
 	ofono_modem_set_string(modem->modem, "Model", modem->model);
 	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
 
+	if (net2)
+		ofono_modem_set_string(modem->modem, "NetworkInterface2", net2);
+
 	return TRUE;
 }
 
-- 
2.28.0

[RFC PATCH 3/4] plugins: gemalto: add optional gprs-context driver

[Sergey Matyukevich]

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

Setup custom gprs context driver for modems that provide
USB ethernet interfaces for data path.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
---
 plugins/gemalto.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 13950742..238c7cc4 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -588,6 +588,8 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
 	const char *model = ofono_modem_get_string(modem, "Model");
+	const char *driver = NULL;
+	const char *iface = NULL;
 
 	DBG("%p", modem);
 
@@ -596,7 +598,15 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 	ofono_sms_create(modem, OFONO_VENDOR_GEMALTO, "atmodem", data->app);
 
 	gprs = ofono_gprs_create(modem, 0, "atmodem", data->app);
-	gc = ofono_gprs_context_create(modem, 0, "atmodem", data->mdm);
+
+	iface = ofono_modem_get_string(modem, "NetworkInterface");
+	if (iface) {
+		driver = "gemaltomodem";
+	} else {
+		driver = "atmodem";
+	}
+
+	gc = ofono_gprs_context_create(modem, 0, driver, data->mdm);
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
-- 
2.28.0

[RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown

[Sergey Matyukevich]

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

Function gemalto_modem_ready attempts to restart AT chat data->app
after  incomplete shutdown. As a result, new AT chat does not work
as expected loosing AT commands.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
---
 plugins/gemalto.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 238c7cc4..321c8c1b 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -222,6 +222,8 @@ static void sim_state_cb(gboolean present, gpointer user_data)
 	struct ofono_modem *modem = user_data;
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 
+	DBG("");
+
 	at_util_sim_state_query_free(data->sim_state_query);
 	data->sim_state_query = NULL;
 
@@ -241,6 +243,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 	struct ofono_modem *modem = user_data;
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 
+	DBG("");
+
 	if (!ok) {
 		g_at_chat_unref(data->app);
 		data->app = NULL;
@@ -451,6 +455,8 @@ static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
 	data->modem_ready_id = 0;
 	data->trial_cmd_id = 0;
 
+	g_at_chat_cancel_all(data->app);
+	g_at_chat_unregister_all(data->app);
 	g_at_chat_unref(data->app);
 
 	data->app = open_device(app);
@@ -466,6 +472,8 @@ static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	struct ofono_modem *modem = user_data;
 	struct gemalto_data *data = ofono_modem_get_data(modem);
 
+	DBG("");
+
 	g_at_chat_unregister(data->app, data->modem_ready_id);
 	data->modem_ready_id = 0;
 
-- 
2.28.0

Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown

[Sergey Matyukevich]

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

> Function gemalto_modem_ready attempts to restart AT chat data->app
> after  incomplete shutdown. As a result, new AT chat does not work
> as expected loosing AT commands.
> 
> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> ---
>  plugins/gemalto.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 238c7cc4..321c8c1b 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c

...

> @@ -451,6 +455,8 @@ static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
>  	data->modem_ready_id = 0;
>  	data->trial_cmd_id = 0;
>  
> +	g_at_chat_cancel_all(data->app);
> +	g_at_chat_unregister_all(data->app);
>  	g_at_chat_unref(data->app);
>  
>  	data->app = open_device(app);

Please disregard this patch in the series. This change does not resolve
the root cause. It looks like when gemalto_modem_ready is called on
^SYSSTART, occasionally modem is not ready to process AT commands.
As a result, the upcoming gemalto_initialize function fails due to
timed out AT commands.

It turns out that the approach implemented in ublox plugin (repeated
probe AT command) provides more reliable results, in particular
in power-off tests. I will do more testing and update this patch
in the next revision of the patch set.

Regards,
Sergey

Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown

[Denis Kenzior]

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

Hi Sergey,

On 8/15/20 4:43 PM, Sergey Matyukevich wrote:
> Function gemalto_modem_ready attempts to restart AT chat data->app
> after  incomplete shutdown. As a result, new AT chat does not work
> as expected loosing AT commands.
> 
> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> ---
>   plugins/gemalto.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

Patch 2, 3 look good to me.

For this one, what is it trying to do actually?

> 
> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
> index 238c7cc4..321c8c1b 100644
> --- a/plugins/gemalto.c
> +++ b/plugins/gemalto.c
> @@ -222,6 +222,8 @@ static void sim_state_cb(gboolean present, gpointer user_data)
>   	struct ofono_modem *modem = user_data;
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
>   
> +	DBG("");
> +
>   	at_util_sim_state_query_free(data->sim_state_query);
>   	data->sim_state_query = NULL;
>   
> @@ -241,6 +243,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>   	struct ofono_modem *modem = user_data;
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
>   
> +	DBG("");
> +
>   	if (!ok) {
>   		g_at_chat_unref(data->app);
>   		data->app = NULL;
> @@ -451,6 +455,8 @@ static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
>   	data->modem_ready_id = 0;
>   	data->trial_cmd_id = 0;
>   
> +	g_at_chat_cancel_all(data->app);
> +	g_at_chat_unregister_all(data->app);

These get called automatically on unref below, assuming  this is the last (it 
should be) reference.

>   	g_at_chat_unref(data->app);

Instead of this dance of opening / closing the chat in order to force-cancel the 
'AT' command queued in gemalto_enable(), maybe there should be something in 
GAtChat itself to handle this case.  Either a 'force cancel the top of the queue 
because I know the modem is broken' or maybe some form of 'send this on the port 
until it responds properly'.  Alternatively, you could simply use GAtIO and a 
timer to write stuff to the port until it responds and only create the chat at 
that time.

There's already something a bit similar in the form of 
g_at_chat_set_wakeup_command().  It was used on the old Neo Freerunner modem 
which would go to 'sleep' after several seconds of inactivity.  So the 
subsequent AT command would get eaten.  You basically had to poke it with an 
empty command, have it timeout / return OK and then you could go on with 
submitting AT commands again.  But I don't think it is a good fit for this 
particular case.

>   
>   	data->app = open_device(app);
> @@ -466,6 +472,8 @@ static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   	struct ofono_modem *modem = user_data;
>   	struct gemalto_data *data = ofono_modem_get_data(modem);
>   
> +	DBG("");
> +
>   	g_at_chat_unregister(data->app, data->modem_ready_id);
>   	data->modem_ready_id = 0;
>   
> 

Regards,
-Denis

Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown

[Sergey Matyukevich]

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

Hello Denis,

> > Function gemalto_modem_ready attempts to restart AT chat data->app
> > after  incomplete shutdown. As a result, new AT chat does not work
> > as expected loosing AT commands.
> > 
> > Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> > ---
> >   plugins/gemalto.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> 
> Patch 2, 3 look good to me.
> 
> For this one, what is it trying to do actually?
> 

Thanks for review. I will re-send second revision of the patches 1-3
with minor changes according to review comments.

Concerning the 4th patch, I thought the problem was in the incomplete
cleanup on data->app re-open in gemalto_modem_ready function. However
this is not the case. Sometimes, in particular on cold start, modem
is not really ready even after it sends ^SYSSTART. As a result,
subsequent AT+CFUN=4 command never completes. I have been experimenting
with the same approach as in ublox plugin with much better results.
So I will complete my experiments and send new fix in a separate patch.

Regards,
Sergey

Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Hi Denis and Sergey,

On 17/08/2020 21.54, Denis Kenzior wrote:
>> diff --git a/plugins/gemalto.c b/plugins/gemalto.c
>> index 238c7cc4..321c8c1b 100644
>> --- a/plugins/gemalto.c
>> +++ b/plugins/gemalto.c
>> @@ -222,6 +222,8 @@ static void sim_state_cb(gboolean present, 
>> gpointer user_data)
>>       struct ofono_modem *modem = user_data;
>>       struct gemalto_data *data = ofono_modem_get_data(modem);
>> +    DBG("");
>> +
>>       at_util_sim_state_query_free(data->sim_state_query);
>>       data->sim_state_query = NULL;
>> @@ -241,6 +243,8 @@ static void cfun_enable(gboolean ok, GAtResult 
>> *result, gpointer user_data)
>>       struct ofono_modem *modem = user_data;
>>       struct gemalto_data *data = ofono_modem_get_data(modem);
>> +    DBG("");
>> +
>>       if (!ok) {
>>           g_at_chat_unref(data->app);
>>           data->app = NULL;
>> @@ -451,6 +455,8 @@ static void gemalto_modem_ready(GAtResult *result, 
>> gpointer user_data)
>>       data->modem_ready_id = 0;
>>       data->trial_cmd_id = 0;
>> +    g_at_chat_cancel_all(data->app);
>> +    g_at_chat_unregister_all(data->app);
> 
> These get called automatically on unref below, assuming  this is the 
> last (it should be) reference.
> 
>>       g_at_chat_unref(data->app);
> 
> Instead of this dance of opening / closing the chat in order to 
> force-cancel the 'AT' command queued in gemalto_enable(), maybe there 
> should be something in GAtChat itself to handle this case.  Either a 
> 'force cancel the top of the queue because I know the modem is broken' 
> or maybe some form of 'send this on the port until it responds 
> properly'.  Alternatively, you could simply use GAtIO and a timer to 
> write stuff to the port until it responds and only create the chat at 
> that time.
> 
> There's already something a bit similar in the form of 
> g_at_chat_set_wakeup_command().  It was used on the old Neo Freerunner 
> modem which would go to 'sleep' after several seconds of inactivity.  So 
> the subsequent AT command would get eaten.  You basically had to poke it 
> with an empty command, have it timeout / return OK and then you could go 
> on with submitting AT commands again.  But I don't think it is a good 
> fit for this particular case.

There's also the retry logic used on quectel modems with auto-detection 
of baud rates:
https://git.kernel.org/pub/scm/network/ofono/ofono.git/tree/plugins/quectel.c#n1082

// Martin

Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown

[Denis Kenzior]

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

Hi Martin,

>> Instead of this dance of opening / closing the chat in order to force-cancel 
>> the 'AT' command queued in gemalto_enable(), maybe there should be something 
>> in GAtChat itself to handle this case.  

<snip>

> 
> There's also the retry logic used on quectel modems with auto-detection of baud 
> rates:
> https://git.kernel.org/pub/scm/network/ofono/ofono.git/tree/plugins/quectel.c#n1082
> 

Ah right, I completely forgot about g_at_chat_retry that you added for exactly 
the use case I outlined above.  Thanks for the reminder.

Maybe we should just add this modem poking logic into 
drivers/atmodem/atutil.[ch] or something if it will be used across multiple drivers.

Regards,
-Denis