[PATCH 1/4] atmodem: sim: remove quectel serial vendor quirk

[PATCH 1/4] atmodem: sim: remove quectel serial vendor quirk

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

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

The sim inserted/initialized state is handled properly in the quectel
plugin now, so remove the "auto-initialized" quirk from the atmodem
sim driver.
---
 drivers/atmodem/sim.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index dd42cac4..e750a139 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -1354,7 +1354,6 @@ static void at_pin_send_cb(gboolean ok, GAtResult *result,
 	case OFONO_VENDOR_HUAWEI:
 	case OFONO_VENDOR_SIMCOM:
 	case OFONO_VENDOR_SIERRA:
-	case OFONO_VENDOR_QUECTEL_SERIAL:
 		/*
 		 * On ZTE modems, after pin is entered, SIM state is checked
 		 * by polling CPIN as their modem doesn't provide unsolicited
-- 
2.23.0

[PATCH 2/4] quectel: remove leftover reset of wakeup command

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

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

---
 plugins/quectel.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 1d21d6dd..a0e435b5 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -968,7 +968,6 @@ static void ate_cb(int ok, GAtResult *result, void *user_data)
 
 	DBG("%p", modem);
 
-	g_at_chat_set_wakeup_command(data->uart, NULL, 0, 0);
 	g_at_chat_send(data->uart, "AT+CMUX=0,0,5,127,10,3,30,10,2", NULL,
 			cmux_cb, modem, NULL);
 }
-- 
2.23.0

[PATCH 3/4] quectel: rework sim detection

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

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

Simplify sim handling by querying cpin state directly from
quectel_pre_sim().

The query is conducted by issuing a AT+CPIN?, where only CME errors are
catched by the command response handler. The +CPIN: <state> response is
instead catched by a listener.

The CME handler allows proper handling of the sim-busy and sim-not-
inserted states, which are returned as CME errors by the quectel
modems.

The +CPIN listener allows handling of both sim-locked and sim-ready
states. When the sim is not locked, the listener catches the +CPIN:
READY response from the query in quectel_pre_sim(). If the sim is
locked, the listener catched the +CPIN: READY indication received after
unlocking the sim.
---
 plugins/quectel.c | 401 +++++++++++++++++-----------------------------
 1 file changed, 149 insertions(+), 252 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index a0e435b5..4c94d305 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -59,7 +59,6 @@
 #include <drivers/atmodem/vendor.h>
 
 static const char *cfun_prefix[] = { "+CFUN:", NULL };
-static const char *cpin_prefix[] = { "+CPIN:", NULL };
 static const char *cbc_prefix[] = { "+CBC:", NULL };
 static const char *qinistat_prefix[] = { "+QINISTAT:", NULL };
 static const char *cgmm_prefix[] = { "UC15", "Quectel_M95", "Quectel_MC60",
@@ -84,21 +83,12 @@ enum quectel_model {
 	QUECTEL_MC60,
 };
 
-enum quectel_state {
-	QUECTEL_STATE_INITIALIZING = 0,
-	QUECTEL_STATE_POST_SIM,
-	QUECTEL_STATE_READY,
-	QUECTEL_STATE_INITIALIZED,
-};
-
 struct quectel_data {
 	GAtChat *modem;
 	GAtChat *aux;
 	enum ofono_vendor vendor;
 	enum quectel_model model;
-	enum quectel_state state;
 	struct ofono_sim *sim;
-	enum ofono_sim_state sim_state;
 	unsigned int sim_watch;
 
 	/* used by quectel uart driver */
@@ -531,213 +521,6 @@ static void dbus_hw_enable(struct ofono_modem *modem)
 	ofono_modem_add_interface(modem, dbus_hw_interface);
 }
 
-static void qinistat_cb(gboolean ok, GAtResult *result, gpointer user_data)
-{
-	struct ofono_modem *modem = user_data;
-	struct quectel_data *data = ofono_modem_get_data(modem);
-	GAtResultIter iter;
-	int ready = 0;
-	int status;
-
-	DBG("%p", modem);
-
-	g_at_result_iter_init(&iter, result);
-
-	if (!g_at_result_iter_next(&iter, "+QINISTAT:"))
-		return;
-
-	if (!g_at_result_iter_next_number(&iter, &status))
-		return;
-
-	DBG("qinistat: %d", status);
-
-	switch (data->model) {
-	case QUECTEL_UC15:
-		/* UC15 uses a bitmap of 1 + 2 + 4 = 7 */
-		ready = 7;
-		break;
-	case QUECTEL_M95:
-	case QUECTEL_MC60:
-		/* M95 and MC60 uses a counter to 3 */
-		ready = 3;
-		break;
-	case QUECTEL_UNKNOWN:
-		ready = 0;
-		break;
-	}
-
-	if (status != ready) {
-		l_timeout_modify_ms(data->init_timeout, 500);
-		return;
-	}
-
-	l_timeout_remove(data->init_timeout);
-	data->init_timeout = NULL;
-
-	if (data->sim_state == OFONO_SIM_STATE_READY) {
-		/*
-		 * when initializing with a non-locked sim card, the sim atom
-		 * isn't created until now to avoid accessing it before the
-		 * modem is ready.
-		 *
-		 * call ofono_modem_set_powered() to make ofono call
-		 * quectel_pre_sim() where the sim atom is created.
-		 */
-		ofono_modem_set_powered(modem, true);
-	} else {
-		/*
-		 * When initialized with a locked sim card, the modem is already
-		 * powered up, and the inserted signal has been sent to allow
-		 * the pin to be entered. So simply update the state, and notify
-		 * about the finished initialization below.
-		 */
-		data->sim_state = OFONO_SIM_STATE_READY;
-	}
-
-	ofono_sim_initialized_notify(data->sim);
-
-	/*
-	 * If quectel_post_sim() has not yet been called, then postpone atom
-	 * creation until it is called. Otherwise create the atoms now.
-	 */
-	if (data->state != QUECTEL_STATE_POST_SIM) {
-		data->state = QUECTEL_STATE_READY;
-		return;
-	}
-
-	ofono_sms_create(modem, data->vendor, "atmodem", data->aux);
-	ofono_phonebook_create(modem, data->vendor, "atmodem", data->aux);
-	ofono_voicecall_create(modem, data->vendor, "atmodem", data->aux);
-	ofono_call_volume_create(modem, data->vendor, "atmodem", data->aux);
-	data->state = QUECTEL_STATE_INITIALIZED;
-}
-
-static void init_timer_cb(struct l_timeout *timeout, void *user_data)
-{
-	struct ofono_modem *modem = user_data;
-	struct quectel_data *data = ofono_modem_get_data(modem);
-
-	DBG("%p", modem);
-
-	g_at_chat_send(data->aux, "AT+QINISTAT", qinistat_prefix, qinistat_cb,
-			modem, NULL);
-}
-
-static void sim_watch_cb(GAtResult *result, void *user_data)
-{
-	struct ofono_modem *modem = user_data;
-	struct quectel_data *data = ofono_modem_get_data(modem);
-
-	DBG("%p", modem);
-
-	g_at_chat_unregister(data->aux, data->sim_watch);
-	data->sim_watch = 0;
-
-	data->init_timeout = l_timeout_create_ms(500, init_timer_cb, modem, NULL);
-	if (!data->init_timeout) {
-		close_serial(modem);
-		return;
-	}
-}
-
-static enum ofono_sim_state cme_parse(GAtResult *result)
-{
-	struct ofono_error error;
-
-	decode_at_error(&error, g_at_result_final_response(result));
-
-	if (error.type != OFONO_ERROR_TYPE_CME)
-		return OFONO_SIM_STATE_RESETTING;
-
-	switch (error.error) {
-	case 5:
-	case 6:
-	case 7:
-	case 11:
-	case 12:
-	case 17:
-	case 18:
-		return OFONO_SIM_STATE_LOCKED_OUT;
-	case 10:
-		return OFONO_SIM_STATE_NOT_PRESENT;
-	case 13:
-	case 14:
-	case 15:
-		return OFONO_SIM_STATE_RESETTING;
-	default:
-		ofono_error("unknown cpin error: %i", error.error);
-		return OFONO_SIM_STATE_RESETTING;
-	}
-}
-
-static enum ofono_sim_state cpin_parse(GAtResult *result)
-{
-	GAtResultIter iter;
-	const char *cpin;
-
-	g_at_result_iter_init(&iter, result);
-
-	if (!g_at_result_iter_next(&iter, "+CPIN:"))
-		return OFONO_SIM_STATE_RESETTING;
-
-	g_at_result_iter_next_unquoted_string(&iter, &cpin);
-
-	if (g_strcmp0(cpin, "NOT INSERTED") == 0)
-		return OFONO_SIM_STATE_NOT_PRESENT;
-
-	if (g_strcmp0(cpin, "READY") == 0)
-		return OFONO_SIM_STATE_READY;
-
-	return OFONO_SIM_STATE_LOCKED_OUT;
-}
-
-static void cpin_query(gboolean ok, GAtResult *result, gpointer user_data)
-{
-	struct ofono_modem *modem = user_data;
-	struct quectel_data *data = ofono_modem_get_data(modem);
-
-	DBG("%p ok %i", modem, ok);
-
-	if (ok)
-		data->sim_state = cpin_parse(result);
-	else
-		data->sim_state = cme_parse(result);
-
-	/* Turn off the radio. */
-	g_at_chat_send(data->aux, "AT+CFUN=4", none_prefix, NULL, NULL, NULL);
-
-	switch (data->sim_state) {
-	case OFONO_SIM_STATE_LOCKED_OUT:
-		ofono_modem_set_powered(modem, true);
-		data->sim_watch = g_at_chat_register(data->aux, "+CPIN: READY",
-							sim_watch_cb, FALSE,
-							modem, NULL);
-		if (!data->sim_watch) {
-			ofono_error("failed to create sim watch");
-			close_serial(modem);
-			return;
-		}
-		break;
-	case OFONO_SIM_STATE_READY:
-		data->init_timeout = l_timeout_create_ms(500, init_timer_cb,
-							modem, NULL);
-		if (!data->init_timeout) {
-			ofono_error("failed to create qinitstat timer");
-			close_serial(modem);
-			return;
-		}
-		break;
-	case OFONO_SIM_STATE_RESETTING:
-	case OFONO_SIM_STATE_INSERTED:
-		g_at_chat_send(data->aux, "AT+CPIN?", cpin_prefix, cpin_query,
-				modem, NULL);
-		break;
-	case OFONO_SIM_STATE_NOT_PRESENT:
-		ofono_warn("%s: sim not present", ofono_modem_get_path(modem));
-		ofono_modem_set_powered(modem, true);
-	}
-}
-
 static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_modem *modem = user_data;
@@ -751,9 +534,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 	}
 
 	dbus_hw_enable(modem);
-
-	g_at_chat_send(data->aux, "AT+CPIN?", cpin_prefix, cpin_query, modem,
-			NULL);
+	g_at_chat_send(data->aux, "AT+CFUN=4", none_prefix, NULL, NULL, NULL);
+	ofono_modem_set_powered(modem, true);
 }
 
 static void cfun_query(gboolean ok, GAtResult *result, gpointer user_data)
@@ -1108,8 +890,6 @@ static int quectel_disable(struct ofono_modem *modem)
 	g_at_chat_send(data->aux, "AT+CFUN=0", cfun_prefix, cfun_disable, modem,
 			NULL);
 
-	data->state = QUECTEL_STATE_INITIALIZING;
-
 	return -EINPROGRESS;
 }
 
@@ -1143,27 +923,166 @@ static void quectel_set_online(struct ofono_modem *modem, ofono_bool_t online,
 	g_free(cbd);
 }
 
-static void quectel_pre_sim(struct ofono_modem *modem)
+static void qinistat_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
+	struct ofono_modem *modem = user_data;
 	struct quectel_data *data = ofono_modem_get_data(modem);
+	GAtResultIter iter;
+	int ready = 0;
+	int status;
 
 	DBG("%p", modem);
 
-	ofono_devinfo_create(modem, 0, "atmodem", data->aux);
-	data->sim = ofono_sim_create(modem, data->vendor, "atmodem", data->aux);
-	if (!data->sim)
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+QINISTAT:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &status))
+		return;
+
+	DBG("qinistat: %d", status);
+
+	switch (data->model) {
+	case QUECTEL_UC15:
+		/* UC15 uses a bitmap of 1 + 2 + 4 = 7 */
+		ready = 7;
+		break;
+	case QUECTEL_M95:
+	case QUECTEL_MC60:
+		/* M95 and MC60 uses a counter to 3 */
+		ready = 3;
+		break;
+	case QUECTEL_UNKNOWN:
+		ready = 0;
+		break;
+	}
+
+	if (status != ready) {
+		l_timeout_modify_ms(data->init_timeout, 500);
+		return;
+	}
+
+	l_timeout_remove(data->init_timeout);
+	data->init_timeout = NULL;
+
+	if (ofono_sim_get_state(data->sim) == OFONO_SIM_STATE_INSERTED)
+		ofono_sim_initialized_notify(data->sim);
+	else
+		ofono_sim_inserted_notify(data->sim, true);
+}
+
+static void init_timer_cb(struct l_timeout *timeout, void *user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	g_at_chat_send(data->aux, "AT+QINISTAT", qinistat_prefix, qinistat_cb,
+			modem, NULL);
+}
+
+static void sim_watch_cb(GAtResult *result, void *user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+	enum ofono_sim_state state = ofono_sim_get_state(data->sim);
+	const char *path = ofono_modem_get_path(modem);
+	GAtResultIter iter;
+	const char *cpin;
+
+	DBG("%p", modem);
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CPIN:"))
 		return;
 
-	switch (data->sim_state) {
-	case OFONO_SIM_STATE_LOCKED_OUT:
-	case OFONO_SIM_STATE_READY:
+	g_at_result_iter_next_unquoted_string(&iter, &cpin);
+
+	if (g_strcmp0(cpin, "NOT INSERTED") == 0) {
+		ofono_warn("%s: sim not present", path);
+		return;
+	}
+
+	if (g_strcmp0(cpin, "READY") != 0) {
+		if (state != OFONO_SIM_STATE_INSERTED) {
+			ofono_info("%s: sim locked", path);
+			ofono_sim_inserted_notify(data->sim, true);
+		}
+
+		return;
+	}
+
+	g_at_chat_unregister(data->aux, data->sim_watch);
+	data->sim_watch = 0;
+
+	data->init_timeout = l_timeout_create_ms(500, init_timer_cb, modem, NULL);
+	if (!data->init_timeout) {
+		close_serial(modem);
+		return;
+	}
+}
+
+static void cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+	struct ofono_error error;
+
+	if (ok)
+		return;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (error.type != OFONO_ERROR_TYPE_CME)
+		return;
+
+	switch (error.error) {
+	case 5:
+	case 6:
+	case 7:
+	case 11:
+	case 12:
+	case 17:
+	case 18:
 		ofono_sim_inserted_notify(data->sim, true);
 		break;
-	default:
+	case 10:
+		ofono_warn("%s: sim not present", ofono_modem_get_path(modem));
+		break;
+	case 13:
+	case 14:
+	case 15:
+		/* wait for +CPIN: indication */
 		break;
+	default:
+		ofono_error("unknown cpin error: %i", error.error);
 	}
 }
 
+static void quectel_pre_sim(struct ofono_modem *modem)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	ofono_voicecall_create(modem, data->vendor, "atmodem", data->aux);
+	data->sim = ofono_sim_create(modem, data->vendor, "atmodem", data->aux);
+	data->sim_watch = g_at_chat_register(data->aux, "+CPIN:", sim_watch_cb,
+						FALSE, modem, NULL);
+
+	/*
+	 * unsolicited indications about a missing or locked sim can occur before
+	 * the auto-baud dance in open_serial(), so issue a CPIN query here
+	 *
+	 * the callback is only called for CME errors to catch those. The +CPIN
+	 * response is catched by the watch registered above
+	 */
+	g_at_chat_send(data->aux, "AT+CPIN?", none_prefix, cpin_cb, modem, NULL);
+}
+
 static void quectel_post_sim(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
@@ -1179,31 +1098,9 @@ static void quectel_post_sim(struct ofono_modem *modem)
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
 
-	/*
-	 * the sim related atoms must not be created until the modem is really
-	 * ready, so check the state here
-	 */
-	switch (data->state) {
-	case QUECTEL_STATE_INITIALIZING:
-		/*
-		 * the modem is still initializing, so postpone the atom
-		 * creation until qinistat_cb() determines the modem is
-		 * ready
-		 */
-		data->state = QUECTEL_STATE_POST_SIM;
-		return;
-	case QUECTEL_STATE_READY:
-		/* the modem is ready, so create atoms below */
-		break;
-	default:
-		return;
-	}
-
 	ofono_sms_create(modem, data->vendor, "atmodem", data->aux);
 	ofono_phonebook_create(modem, data->vendor, "atmodem", data->aux);
-	ofono_voicecall_create(modem, data->vendor, "atmodem", data->aux);
 	ofono_call_volume_create(modem, data->vendor, "atmodem", data->aux);
-	data->state = QUECTEL_STATE_INITIALIZED;
 }
 
 static void quectel_post_online(struct ofono_modem *modem)
-- 
2.23.0

[PATCH 4/4] quectel: use own cmux implementation over kernel line discipline

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

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

The in-kernel implementation of gsm0710 causes deadlocks in the
kernel[1], so switch back to the user-space implementation in ofono.

[1] https://lore.kernel.org/lkml/4b2455c0-25ba-0187-6df6-c63b4ccc6a6e(a)geanix.com/
---
 plugins/quectel.c | 250 ++++++++++++++++++++++++----------------------
 1 file changed, 130 insertions(+), 120 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 4c94d305..4d33c008 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -28,14 +28,10 @@
 #include <stdbool.h>
 #include <unistd.h>
 
-#include <sys/stat.h>
-#include <sys/ioctl.h>
-#include <sys/socket.h>
-#include <linux/tty.h>
-#include <linux/gsmmux.h>
 #include <ell/ell.h>
 #include <gatchat.h>
 #include <gattty.h>
+#include <gatmux.h>
 
 #define OFONO_API_SUBJECT_TO_CHANGE
 #include <ofono.h>
@@ -92,7 +88,9 @@ struct quectel_data {
 	unsigned int sim_watch;
 
 	/* used by quectel uart driver */
+	GIOChannel *device;
 	GAtChat *uart;
+	GAtMux *mux;
 	int mux_ready_count;
 	int initial_ldisc;
 	struct l_gpio_writer *gpio;
@@ -188,6 +186,11 @@ static void quectel_remove(struct ofono_modem *modem)
 	g_at_chat_unref(data->aux);
 	g_at_chat_unref(data->modem);
 	g_at_chat_unref(data->uart);
+	g_at_mux_unref(data->mux);
+
+	if (data->device)
+		g_io_channel_unref(data->device);
+
 	l_free(data);
 }
 
@@ -195,27 +198,12 @@ static void close_mux_cb(struct l_timeout *timeout, void *user_data)
 {
 	struct ofono_modem *modem = user_data;
 	struct quectel_data *data = ofono_modem_get_data(modem);
-	GIOChannel *device;
 	uint32_t gpio_value = 0;
-	ssize_t write_count;
-	int fd;
 
 	DBG("%p", modem);
 
-	device = g_at_chat_get_channel(data->uart);
-	fd = g_io_channel_unix_get_fd(device);
-
-	/* restore initial tty line discipline */
-	if (ioctl(fd, TIOCSETD, &data->initial_ldisc) < 0)
-		ofono_warn("Failed to restore line discipline");
-
-	/* terminate gsm 0710 multiplexing on the modem side */
-	write_count = write(fd, gsm0710_terminate, sizeof(gsm0710_terminate));
-	if (write_count != sizeof(gsm0710_terminate))
-		ofono_warn("Failed to terminate gsm multiplexing");
-
-	g_at_chat_unref(data->uart);
-	data->uart = NULL;
+	g_at_mux_unref(data->mux);
+	data->mux = NULL;
 
 	l_timeout_remove(timeout);
 	l_gpio_writer_set(data->gpio, 1, &gpio_value);
@@ -225,6 +213,7 @@ static void close_mux_cb(struct l_timeout *timeout, void *user_data)
 static void close_serial(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
+	uint32_t gpio_value = 0;
 
 	DBG("%p", modem);
 
@@ -234,19 +223,27 @@ static void close_serial(struct ofono_modem *modem)
 	g_at_chat_unref(data->modem);
 	data->modem = NULL;
 
-	/*
-	 * if gsm0710 multiplexing is used, the aux and modem file descriptors
-	 * must be closed before closing the underlying serial device to avoid
-	 * an old kernel dead-lock:
-	 * https://lists.ofono.org/pipermail/ofono/2011-March/009405.html
-	 *
-	 * setup a timer to iterate the mainloop once to let gatchat close the
-	 * virtual file descriptors unreferenced above
-	 */
-	if (data->uart)
-		l_timeout_create_ms(1, close_mux_cb, modem, NULL);
-	else
-		ofono_modem_set_powered(modem, false);
+	if (data->device) {
+		g_at_chat_unref(data->uart);
+		data->uart = NULL;
+
+		g_io_channel_unref(data->device);
+		data->device = NULL;
+
+		/*
+		 * shutting down and freeing the cmux object in a single
+		 * iteration of the main-loop causes use-after-free errors in
+		 * gatmux.c, so shutdown the mux here, and free it after a
+		 * timeout.
+		 */
+		g_at_mux_shutdown(data->mux);
+		l_timeout_create_ms(50, close_mux_cb, modem, NULL);
+
+		return;
+	}
+
+	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	ofono_modem_set_powered(modem, FALSE);
 }
 
 static void dbus_hw_reply_properties(struct dbus_hw *hw)
@@ -524,7 +521,6 @@ static void dbus_hw_enable(struct ofono_modem *modem)
 static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_modem *modem = user_data;
-	struct quectel_data *data = ofono_modem_get_data(modem);
 
 	DBG("%p ok %d", modem, ok);
 
@@ -614,6 +610,19 @@ static void cgmm_cb(int ok, GAtResult *result, void *user_data)
 			NULL);
 }
 
+static void setup_aux(struct ofono_modem *modem)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	g_at_chat_set_slave(data->modem, data->aux);
+	g_at_chat_send(data->aux, "ATE0; &C0; +CMEE=1; +QIURC=0", none_prefix,
+			NULL, NULL, NULL);
+	g_at_chat_send(data->aux, "AT+CGMM", cgmm_prefix, cgmm_cb, modem,
+			NULL);
+}
+
 static int open_ttys(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
@@ -633,114 +642,80 @@ static int open_ttys(struct ofono_modem *modem)
 		return -EIO;
 	}
 
-	g_at_chat_set_slave(data->modem, data->aux);
-
-	g_at_chat_send(data->aux, "ATE0; &C0; +CMEE=1; +QIURC=0", none_prefix,
-			NULL, NULL, NULL);
-	g_at_chat_send(data->aux, "AT+CGMM", cgmm_prefix, cgmm_cb, modem,
-			NULL);
+	setup_aux(modem);
 
 	return -EINPROGRESS;
 }
 
-static void mux_ready_cb(struct l_timeout *timeout, void *user_data)
+static GAtChat *create_chat(struct ofono_modem *modem, char *debug)
 {
-	struct ofono_modem *modem = user_data;
 	struct quectel_data *data = ofono_modem_get_data(modem);
-	struct stat st;
-	int ret;
+	GIOChannel *channel;
+	GAtSyntax *syntax;
+	GAtChat *chat;
 
 	DBG("%p", modem);
 
-	/* check if the last (and thus all) virtual gsm tty's are created */
-	ret = stat(ofono_modem_get_string(modem, "Modem"), &st);
-	if (ret < 0) {
-		if (data->mux_ready_count++ < 5) {
-			/* not ready yet; try again in 100 ms*/
-			l_timeout_modify_ms(timeout, 100);
-			return;
-		}
+	channel = g_at_mux_create_channel(data->mux);
+	if (channel == NULL)
+		return NULL;
 
-		/* not ready after 500 ms; bail out */
-		close_serial(modem);
-		return;
-	}
+	syntax = g_at_syntax_new_gsmv1();
+	chat = g_at_chat_new(channel, syntax);
+	g_at_syntax_unref(syntax);
+	g_io_channel_unref(channel);
 
-	/* virtual gsm tty's are ready */
-	l_timeout_remove(timeout);
+	if (chat == NULL)
+		return NULL;
 
-	if (open_ttys(modem) != -EINPROGRESS)
-		close_serial(modem);
+	if (getenv("OFONO_AT_DEBUG"))
+		g_at_chat_set_debug(chat, quectel_debug, debug);
 
-	g_at_chat_set_slave(data->uart, data->modem);
+	return chat;
 }
 
 static void cmux_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_modem *modem = user_data;
 	struct quectel_data *data = ofono_modem_get_data(modem);
-	struct gsm_config gsm_config;
-	GIOChannel *device;
-	int ldisc = N_GSM0710;
-	int fd;
 
 	DBG("%p", modem);
 
-	device = g_at_chat_get_channel(data->uart);
-	fd = g_io_channel_unix_get_fd(device);
+	g_at_chat_unref(data->uart);
+	data->uart = NULL;
 
-	/* get initial line discipline to restore after use */
-	if (ioctl(fd, TIOCGETD, &data->initial_ldisc) < 0) {
-		ofono_error("Failed to get current line discipline: %s",
-				strerror(errno));
+	if (!ok) {
 		close_serial(modem);
 		return;
 	}
 
-	/* enable gsm 0710 multiplexing line discipline */
-	if (ioctl(fd, TIOCSETD, &ldisc) < 0) {
-		ofono_error("Failed to set multiplexer line discipline: %s",
-				strerror(errno));
+	data->mux = g_at_mux_new_gsm0710_basic(data->device, 127);
+	if (data->mux == NULL) {
+		ofono_error("failed to create gsm0710 mux");
 		close_serial(modem);
 		return;
 	}
 
-	/* get n_gsm configuration */
-	if (ioctl(fd, GSMIOC_GETCONF, &gsm_config) < 0) {
-		ofono_error("Failed to get gsm config: %s", strerror(errno));
-		close_serial(modem);
-		return;
-	}
+	if (getenv("OFONO_MUX_DEBUG"))
+		g_at_mux_set_debug(data->mux, quectel_debug, "Mux: ");
+
+	g_at_mux_start(data->mux);
 
-	gsm_config.initiator = 1;     /* cpu side is initiating multiplexing */
-	gsm_config.encapsulation = 0; /* basic transparency encoding */
-	gsm_config.mru = 127;         /* 127 bytes rx mtu */
-	gsm_config.mtu = 127;         /* 127 bytes tx mtu */
-	gsm_config.t1 = 10;           /* 100 ms ack timer */
-	gsm_config.n2 = 3;            /* 3 retries */
-	gsm_config.t2 = 30;           /* 300 ms response timer */
-	gsm_config.t3 = 10;           /* 100 ms wake up response timer */
-	gsm_config.i = 1;             /* subset */
-
-	/* set the new configuration */
-	if (ioctl(fd, GSMIOC_SETCONF, &gsm_config) < 0) {
-		ofono_error("Failed to set gsm config: %s", strerror(errno));
+	data->modem = create_chat(modem, "Modem: ");
+	if (!data->modem) {
+		ofono_error("failed to create modem channel");
 		close_serial(modem);
 		return;
 	}
 
-	/*
-	 * the kernel does not yet support mapping the underlying serial device
-	 * to its virtual gsm ttys, so hard-code gsmtty1 gsmtty2 for now
-	 */
-	ofono_modem_set_string(modem, "Modem", "/dev/gsmtty1");
-	ofono_modem_set_string(modem, "Aux", "/dev/gsmtty2");
-
-	/* wait for gsmtty devices to appear */
-	if (!l_timeout_create_ms(100, mux_ready_cb, modem, NULL)) {
+	data->aux = create_chat(modem, "Aux: ");
+	if (!data->aux) {
+		ofono_error("failed to create aux channel");
 		close_serial(modem);
 		return;
 	}
+
+	setup_aux(modem);
 }
 
 static void ate_cb(int ok, GAtResult *result, void *user_data)
@@ -771,8 +746,8 @@ static void init_cmd_cb(gboolean ok, GAtResult *result, void *user_data)
 		g_at_chat_send(data->uart, "AT+IFC=2,2; E0", none_prefix,
 				ate_cb, modem, NULL);
 	else
-		g_at_chat_send(data->uart, "ATE0", none_prefix, ate_cb, modem,
-				NULL);
+		g_at_chat_send(data->uart, "AT+IFC=0,0; E0", none_prefix,
+				ate_cb, modem, NULL);
 
 	l_timeout_remove(data->init_timeout);
 	data->init_timeout = NULL;
@@ -798,26 +773,58 @@ static void init_timeout_cb(struct l_timeout *timeout, void *user_data)
 static int open_serial(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
+	GHashTable *options;
+	GAtSyntax *syntax;
 	const uint32_t gpio_value = 1;
 	const char *rts_cts;
+	const char *device;
+	ssize_t written;
+	int fd;
 
 	DBG("%p", modem);
 
+	device = ofono_modem_get_string(modem, "Device");
 	rts_cts = ofono_modem_get_string(modem, "RtsCts");
 
-	data->uart = at_util_open_device(modem, "Device", quectel_debug,
-						"UART: ",
-						"Baud", "115200",
-						"Parity", "none",
-						"StopBits", "1",
-						"DataBits", "8",
-						"XonXoff", "off",
-						"Local", "on",
-						"Read", "on",
-						"RtsCts", rts_cts,
-						NULL);
-	if (data->uart == NULL)
-		return -EINVAL;
+	options = g_hash_table_new(g_str_hash, g_str_equal);
+	if (options == NULL)
+		return -ENOMEM;
+
+	g_hash_table_insert(options, "Baud", "115200");
+	g_hash_table_insert(options, "Parity", "none");
+	g_hash_table_insert(options, "StopBits", "1");
+	g_hash_table_insert(options, "DataBits", "8");
+	g_hash_table_insert(options, "XonXoff", "off");
+	g_hash_table_insert(options, "Local", "on");
+	g_hash_table_insert(options, "Read", "on");
+	g_hash_table_insert(options, "RtsCts", (char *)rts_cts);
+
+	data->device = g_at_tty_open(device, options);
+	g_hash_table_destroy(options);
+
+	if (data->device == NULL)
+		return -EIO;
+
+	/*
+	 * terminate gsm 0710 multiplexing on the modem side to make sure it
+	 * responds to plain AT commands
+	 * */
+	fd = g_io_channel_unix_get_fd(data->device);
+	written = write(fd, gsm0710_terminate, sizeof(gsm0710_terminate));
+	if (written != sizeof(gsm0710_terminate))
+		ofono_warn("Failed to terminate gsm multiplexing");
+
+	syntax = g_at_syntax_new_gsm_permissive();
+	data->uart = g_at_chat_new(data->device, syntax);
+	g_at_syntax_unref(syntax);
+
+	if (data->uart == NULL) {
+		close_serial(modem);
+		return -EIO;
+	}
+
+	if (getenv("OFONO_AT_DEBUG"))
+		g_at_chat_set_debug(data->uart, quectel_debug, "UART: ");
 
 	if (data->gpio && !l_gpio_writer_set(data->gpio, 1, &gpio_value)) {
 		close_serial(modem);
@@ -878,6 +885,9 @@ static int quectel_disable(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
+	if (data->aux == NULL)
+		return 0;
+
 	g_at_chat_cancel_all(data->modem);
 	g_at_chat_unregister_all(data->modem);
 
-- 
2.23.0

Re: [PATCH 1/4] atmodem: sim: remove quectel serial vendor quirk

[Denis Kenzior]

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

Hi Martin,

On 9/26/19 2:27 PM, Martin Hundebøll wrote:
> The sim inserted/initialized state is handled properly in the quectel
> plugin now, so remove the "auto-initialized" quirk from the atmodem
> sim driver.
> ---
>   drivers/atmodem/sim.c | 1 -
>   1 file changed, 1 deletion(-)
> 

Patch 1 & 2 applied, thanks.

Regards,
-Denis

Re: [PATCH 3/4] quectel: rework sim detection

[Denis Kenzior]

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

Hi Martin,

On 9/26/19 2:27 PM, Martin Hundebøll wrote:
> Simplify sim handling by querying cpin state directly from
> quectel_pre_sim().

Don't initiate commands from pre_sim/post_sim/post_online callbacks 
please.  There's almost never a reason to.

> 
> The query is conducted by issuing a AT+CPIN?, where only CME errors are
> catched by the command response handler. The +CPIN: <state> response is
> instead catched by a listener.

nit: s/catched/caught

> 
> The CME handler allows proper handling of the sim-busy and sim-not-
> inserted states, which are returned as CME errors by the quectel
> modems.
> 
> The +CPIN listener allows handling of both sim-locked and sim-ready
> states. When the sim is not locked, the listener catches the +CPIN:
> READY response from the query in quectel_pre_sim(). If the sim is
> locked, the listener catched the +CPIN: READY indication received after
> unlocking the sim.
> ---
>   plugins/quectel.c | 401 +++++++++++++++++-----------------------------
>   1 file changed, 149 insertions(+), 252 deletions(-)
> 

<snip>

> @@ -751,9 +534,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>   	}
>   
>   	dbus_hw_enable(modem);
> -
> -	g_at_chat_send(data->aux, "AT+CPIN?", cpin_prefix, cpin_query, modem,
> -			NULL);
> +	g_at_chat_send(data->aux, "AT+CFUN=4", none_prefix, NULL, NULL, NULL);

Don't you want to wait until this succeeds?

Also, you could issue a query for CPIN here and record whether the sim 
is inserted.  I assume no hot-swap of SIM is supported?

> +	ofono_modem_set_powered(modem, true);
>   }
>   
>   static void cfun_query(gboolean ok, GAtResult *result, gpointer user_data)

<snip>

> +
> +	if (ofono_sim_get_state(data->sim) == OFONO_SIM_STATE_INSERTED)
> +		ofono_sim_initialized_notify(data->sim);
> +	else
> +		ofono_sim_inserted_notify(data->sim, true);

This is a strange dance.  Shouldn't the modem tell you up front that the 
SIM is inserted or not?  So then you only need to run QINISTAT when it 
goes from pin locked to unlocked?

> +}
> +
> +static void init_timer_cb(struct l_timeout *timeout, void *user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	g_at_chat_send(data->aux, "AT+QINISTAT", qinistat_prefix, qinistat_cb,
> +			modem, NULL);
> +}
> +
> +static void sim_watch_cb(GAtResult *result, void *user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +	enum ofono_sim_state state = ofono_sim_get_state(data->sim);
> +	const char *path = ofono_modem_get_path(modem);
> +	GAtResultIter iter;
> +	const char *cpin;
> +
> +	DBG("%p", modem);
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CPIN:"))
>   		return;
>   
> -	switch (data->sim_state) {
> -	case OFONO_SIM_STATE_LOCKED_OUT:
> -	case OFONO_SIM_STATE_READY:
> +	g_at_result_iter_next_unquoted_string(&iter, &cpin);
> +
> +	if (g_strcmp0(cpin, "NOT INSERTED") == 0) {
> +		ofono_warn("%s: sim not present", path);
> +		return;
> +	}
> +
> +	if (g_strcmp0(cpin, "READY") != 0) {
> +		if (state != OFONO_SIM_STATE_INSERTED) {
> +			ofono_info("%s: sim locked", path);
> +			ofono_sim_inserted_notify(data->sim, true);

Okay, but  it might be easier to just know this up front whether the sim 
is inserted or not.

> +		}
> +
> +		return;
> +	}
> +
> +	g_at_chat_unregister(data->aux, data->sim_watch);
> +	data->sim_watch = 0;
> +
> +	data->init_timeout = l_timeout_create_ms(500, init_timer_cb, modem, NULL);
> +	if (!data->init_timeout) {
> +		close_serial(modem);
> +		return;
> +	}

So then this READY case becomes simple.  Didn't you also say that 
Quectel sends +CPIN: READY unsolicited?

> +}
> +
> +static void cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +	struct ofono_error error;
> +
> +	if (ok)
> +		return;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (error.type != OFONO_ERROR_TYPE_CME)
> +		return;
> +
> +	switch (error.error) {
> +	case 5:
> +	case 6:
> +	case 7:
> +	case 11:
> +	case 12:
> +	case 17:
> +	case 18:
>   		ofono_sim_inserted_notify(data->sim, true);
>   		break;
> -	default:
> +	case 10:
> +		ofono_warn("%s: sim not present", ofono_modem_get_path(modem));
> +		break;
> +	case 13:
> +	case 14:
> +	case 15:
> +		/* wait for +CPIN: indication */
>   		break;
> +	default:
> +		ofono_error("unknown cpin error: %i", error.error);
>   	}
>   }

at_util_sim_state_query_new does most of this for you btw...

>   
> +static void quectel_pre_sim(struct ofono_modem *modem)
> +{
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	ofono_voicecall_create(modem, data->vendor, "atmodem", data->aux);
> +	data->sim = ofono_sim_create(modem, data->vendor, "atmodem", data->aux);
> +	data->sim_watch = g_at_chat_register(data->aux, "+CPIN:", sim_watch_cb,
> +						FALSE, modem, NULL);
> +
> +	/*
> +	 * unsolicited indications about a missing or locked sim can occur before
> +	 * the auto-baud dance in open_serial(), so issue a CPIN query here
> +	 *
> +	 * the callback is only called for CME errors to catch those. The +CPIN
> +	 * response is catched by the watch registered above

nit: caught

> +	 */
> +	g_at_chat_send(data->aux, "AT+CPIN?", none_prefix, cpin_cb, modem, NULL);

So as mentioned before, don't run commands in pre_sim.  You can do this 
as part of .enable sequence instead, store sim presence info and simply 
call sim_create & sim_inserted if needed.

> +}
> +

Regards,
-Denis

Re: [PATCH 4/4] quectel: use own cmux implementation over kernel line discipline

[Denis Kenzior]

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

Hi Martin,

On 9/26/19 2:27 PM, Martin Hundebøll wrote:
> The in-kernel implementation of gsm0710 causes deadlocks in the
> kernel[1], so switch back to the user-space implementation in ofono.
> 
> [1] https://lore.kernel.org/lkml/4b2455c0-25ba-0187-6df6-c63b4ccc6a6e(a)geanix.com/
> ---
>   plugins/quectel.c | 250 ++++++++++++++++++++++++----------------------
>   1 file changed, 130 insertions(+), 120 deletions(-)
> 

Ugh.  That poor quectel.c file has so much churn its not funny :)

So I wonder whether instead of fully reverting back to CMUX (only to put 
in n_gsm in later) you might want to just keep both around and use a 
/etc/quectel.conf or OFONO_QUECTEL_MUX=n_gsm|internal instead?

Regards,
-Denis

Re: [PATCH 4/4] quectel: use own cmux implementation over kernel line discipline

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

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

Hi Denis,

On 27/09/2019 03.00, Denis Kenzior wrote:
> On 9/26/19 2:27 PM, Martin Hundebøll wrote:
>> The in-kernel implementation of gsm0710 causes deadlocks in the
>> kernel[1], so switch back to the user-space implementation in ofono.
>>
>> [1] 
>> https://lore.kernel.org/lkml/4b2455c0-25ba-0187-6df6-c63b4ccc6a6e(a)geanix.com/ 
>>
>> ---
>>   plugins/quectel.c | 250 ++++++++++++++++++++++++----------------------
>>   1 file changed, 130 insertions(+), 120 deletions(-)
>>
> 
> Ugh.  That poor quectel.c file has so much churn its not funny :)

As long as it is getting better along the way :)

> So I wonder whether instead of fully reverting back to CMUX (only to put 
> in n_gsm in later) you might want to just keep both around and use a 
> /etc/quectel.conf or OFONO_QUECTEL_MUX=n_gsm|internal instead?

I'd prefer a single method to keep the needed testing to a minimum.

// Martin

Re: [PATCH 4/4] quectel: use own cmux implementation over kernel line discipline

[Denis Kenzior]

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

Hi Martin,

>> So I wonder whether instead of fully reverting back to CMUX (only to 
>> put in n_gsm in later) you might want to just keep both around and use 
>> a /etc/quectel.conf or OFONO_QUECTEL_MUX=n_gsm|internal instead?
> 
> I'd prefer a single method to keep the needed testing to a minimum.
> 

I know where you're coming from, but you will probably come back in 6 
months and want to use the kernel multiplexer again and this change will 
get reverted again.

Then someone with an old kernel will want the built-in one.  Might as 
well bite the bullet now..

Regards,
-Denis

Re: [PATCH 4/4] quectel: use own cmux implementation over kernel line discipline

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

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

Hi Denis,

On 30/09/2019 21.55, Denis Kenzior wrote:
> Hi Martin,
> 
>>> So I wonder whether instead of fully reverting back to CMUX (only to 
>>> put in n_gsm in later) you might want to just keep both around and 
>>> use a /etc/quectel.conf or OFONO_QUECTEL_MUX=n_gsm|internal instead?
>>
>> I'd prefer a single method to keep the needed testing to a minimum.
>>
> 
> I know where you're coming from, but you will probably come back in 6 
> months and want to use the kernel multiplexer again and this change will 
> get reverted again.
> 
> Then someone with an old kernel will want the built-in one.  Might as 
> well bite the bullet now..

Ok, drop my v2 cmux patch then. I'll spin a new one.

// Martin

Re: [PATCH 3/4] quectel: rework sim detection

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

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

Hi Denis,

On 27/09/2019 00.10, Denis Kenzior wrote:
> On 9/26/19 2:27 PM, Martin Hundebøll wrote:
>> Simplify sim handling by querying cpin state directly from
>> quectel_pre_sim().
> 
> Don't initiate commands from pre_sim/post_sim/post_online callbacks 
> please.  There's almost never a reason to.

Ok.

>>
>> The query is conducted by issuing a AT+CPIN?, where only CME errors are
>> catched by the command response handler. The +CPIN: <state> response is
>> instead catched by a listener.
> 
> nit: s/catched/caught
> 
>>
>> The CME handler allows proper handling of the sim-busy and sim-not-
>> inserted states, which are returned as CME errors by the quectel
>> modems.
>>
>> The +CPIN listener allows handling of both sim-locked and sim-ready
>> states. When the sim is not locked, the listener catches the +CPIN:
>> READY response from the query in quectel_pre_sim(). If the sim is
>> locked, the listener catched the +CPIN: READY indication received after
>> unlocking the sim.
>> ---
>>   plugins/quectel.c | 401 +++++++++++++++++-----------------------------
>>   1 file changed, 149 insertions(+), 252 deletions(-)
>>
> 
> <snip>
> 
>> @@ -751,9 +534,8 @@ static void cfun_enable(gboolean ok, GAtResult 
>> *result, gpointer user_data)
>>       }
>>       dbus_hw_enable(modem);
>> -
>> -    g_at_chat_send(data->aux, "AT+CPIN?", cpin_prefix, cpin_query, 
>> modem,
>> -            NULL);
>> +    g_at_chat_send(data->aux, "AT+CFUN=4", none_prefix, NULL, NULL, 
>> NULL);
> 
> Don't you want to wait until this succeeds?

Might as well.

> Also, you could issue a query for CPIN here and record whether the sim 
> is inserted.  I assume no hot-swap of SIM is supported?
> 
>> +    ofono_modem_set_powered(modem, true);
>>   }
>>   static void cfun_query(gboolean ok, GAtResult *result, gpointer 
>> user_data)
> 
> <snip>
> 
>> +
>> +    if (ofono_sim_get_state(data->sim) == OFONO_SIM_STATE_INSERTED)
>> +        ofono_sim_initialized_notify(data->sim);
>> +    else
>> +        ofono_sim_inserted_notify(data->sim, true);
> 
> This is a strange dance.  Shouldn't the modem tell you up front that the 
> SIM is inserted or not?  So then you only need to run QINISTAT when it 
> goes from pin locked to unlocked?

These modems are really picky about when to do what.

If the sim is locked, we need to do some initial work to enable entering 
the pin.

If it is not, then we really need to wait with reading sim files until 
qinistat is ready.

I've made another attempt in the v2 sim-rework patch.

>> +}
>> +
>> +static void init_timer_cb(struct l_timeout *timeout, void *user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct quectel_data *data = ofono_modem_get_data(modem);
>> +
>> +    DBG("%p", modem);
>> +
>> +    g_at_chat_send(data->aux, "AT+QINISTAT", qinistat_prefix, 
>> qinistat_cb,
>> +            modem, NULL);
>> +}
>> +
>> +static void sim_watch_cb(GAtResult *result, void *user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct quectel_data *data = ofono_modem_get_data(modem);
>> +    enum ofono_sim_state state = ofono_sim_get_state(data->sim);
>> +    const char *path = ofono_modem_get_path(modem);
>> +    GAtResultIter iter;
>> +    const char *cpin;
>> +
>> +    DBG("%p", modem);
>> +
>> +    g_at_result_iter_init(&iter, result);
>> +
>> +    if (!g_at_result_iter_next(&iter, "+CPIN:"))
>>           return;
>> -    switch (data->sim_state) {
>> -    case OFONO_SIM_STATE_LOCKED_OUT:
>> -    case OFONO_SIM_STATE_READY:
>> +    g_at_result_iter_next_unquoted_string(&iter, &cpin);
>> +
>> +    if (g_strcmp0(cpin, "NOT INSERTED") == 0) {
>> +        ofono_warn("%s: sim not present", path);
>> +        return;
>> +    }
>> +
>> +    if (g_strcmp0(cpin, "READY") != 0) {
>> +        if (state != OFONO_SIM_STATE_INSERTED) {
>> +            ofono_info("%s: sim locked", path);
>> +            ofono_sim_inserted_notify(data->sim, true);
> 
> Okay, but  it might be easier to just know this up front whether the sim 
> is inserted or not.

I think I am getting close in the v2 patch.

>> +        }
>> +
>> +        return;
>> +    }
>> +
>> +    g_at_chat_unregister(data->aux, data->sim_watch);
>> +    data->sim_watch = 0;
>> +
>> +    data->init_timeout = l_timeout_create_ms(500, init_timer_cb, 
>> modem, NULL);
>> +    if (!data->init_timeout) {
>> +        close_serial(modem);
>> +        return;
>> +    }
> 
> So then this READY case becomes simple.  Didn't you also say that 
> Quectel sends +CPIN: READY unsolicited?

It does, but we might miss the initial one during the retry-dance after 
powerup, or if ofono is started long after the modem is initialized 
(sans gpio).

It does send a +CPIN: READY after the pin is entered, though.

>> +}
>> +
>> +static void cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +    struct ofono_modem *modem = user_data;
>> +    struct quectel_data *data = ofono_modem_get_data(modem);
>> +    struct ofono_error error;
>> +
>> +    if (ok)
>> +        return;
>> +
>> +    decode_at_error(&error, g_at_result_final_response(result));
>> +
>> +    if (error.type != OFONO_ERROR_TYPE_CME)
>> +        return;
>> +
>> +    switch (error.error) {
>> +    case 5:
>> +    case 6:
>> +    case 7:
>> +    case 11:
>> +    case 12:
>> +    case 17:
>> +    case 18:
>>           ofono_sim_inserted_notify(data->sim, true);
>>           break;
>> -    default:
>> +    case 10:
>> +        ofono_warn("%s: sim not present", ofono_modem_get_path(modem));
>> +        break;
>> +    case 13:
>> +    case 14:
>> +    case 15:
>> +        /* wait for +CPIN: indication */
>>           break;
>> +    default:
>> +        ofono_error("unknown cpin error: %i", error.error);
>>       }
>>   }
> 
> at_util_sim_state_query_new does most of this for you btw...

Thanks for the hint.

I tried this once some time ago, but I needed not only present 
true/false, but also locked/unlocked. This is queried in the v2 patch now.

>> +static void quectel_pre_sim(struct ofono_modem *modem)
>> +{
>> +    struct quectel_data *data = ofono_modem_get_data(modem);
>> +
>> +    DBG("%p", modem);
>> +
>> +    ofono_voicecall_create(modem, data->vendor, "atmodem", data->aux);
>> +    data->sim = ofono_sim_create(modem, data->vendor, "atmodem", 
>> data->aux);
>> +    data->sim_watch = g_at_chat_register(data->aux, "+CPIN:", 
>> sim_watch_cb,
>> +                        FALSE, modem, NULL);
>> +
>> +    /*
>> +     * unsolicited indications about a missing or locked sim can 
>> occur before
>> +     * the auto-baud dance in open_serial(), so issue a CPIN query here
>> +     *
>> +     * the callback is only called for CME errors to catch those. The 
>> +CPIN
>> +     * response is catched by the watch registered above
> 
> nit: caught
> 
>> +     */
>> +    g_at_chat_send(data->aux, "AT+CPIN?", none_prefix, cpin_cb, 
>> modem, NULL);
> 
> So as mentioned before, don't run commands in pre_sim.  You can do this 
> as part of .enable sequence instead, store sim presence info and simply 
> call sim_create & sim_inserted if needed.

Got it.

// Martin