[PATCHv2] plugin: Add ste modem initd integration

[PATCHv2] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch introduces auto discovery of ST-Ericsson modems.
ST-Ericsson modems (M5XX, M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed
from the Modem Init Damon's Dbus API. When the modem is in state
"on" the STE modem is created and registered. Multiple modem
instances, and reset handling is supported.
---
Changes:
- Support for multiple STE modems.
- Better naming of STE modem instances, using received dbus path.
- CAIF Link layer to use for AT channels specified pr modem instance.
- Added support for reset.

 Makefile.am      |    5 +
 configure.ac     |    6 +
 plugins/stemid.c |  310 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemid.c

diff --git a/Makefile.am b/Makefile.am
index 12b3c33..98096ae 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -82,6 +82,11 @@ gatchat_sources = gatchat/gatchat.h gatchat/gatchat.c \
 
 udev_files = plugins/ofono.rules
 
+if STE_MODEM_INITD
+builtin_modules += stemid
+builtin_sources += plugins/stemid.c
+endif
+
 if UDEV
 builtin_modules += udev
 builtin_sources += plugins/udev.c
diff --git a/configure.ac b/configure.ac
index 5c18f68..f733fc4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -177,6 +177,12 @@ AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles],
 
 AM_CONDITIONAL(DATAFILES, test "${enable_datafiles}" != "no")
 
+AC_ARG_ENABLE(ste_modem_initd, AC_HELP_STRING([--disable-ste-modem-initd],
+			[disable auto discovery of STE modem using modem init daemon]),
+					[enable_ste_modem_initd=${enableval}])
+
+AM_CONDITIONAL(STE_MODEM_INITD, test "${enable_ste_modem_initd}" != "no")
+
 if (test "${prefix}" = "NONE"); then
 	dnl no prefix and no localstatedir, so default to /var
 	if (test "$localstatedir" = '${prefix}/var'); then
diff --git a/plugins/stemid.c b/plugins/stemid.c
new file mode 100644
index 0000000..9f5da22
--- /dev/null
+++ b/plugins/stemid.c
@@ -0,0 +1,310 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *
+ *  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 <errno.h>
+#include <string.h>
+#include <net/if.h>
+
+#include <glib.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+
+/*
+ * ST-Ericsson's Modem Init Daemon is used for controling the modem power
+ * cycles and provides a dbus API for modem state and properties.
+ */
+#define MID_SERVICE		"com.stericsson.modeminit"
+#define MID_INTERFACE		MID_SERVICE ".Modem"
+
+/* The signal StateChange sends the modem state.*/
+#define STATUS_CHANGED		"StateChange"
+
+#define MID_ACTION_ON		"on"		/* Modem is on */
+#define MID_ACTION_RESET	"dumping"	/* Modem is resetting */
+#define MID_ACTION_UNKNOWN	"unknown"	/* Don't care */
+
+/* ResetState requests resending of StateChange. */
+#define MID_RESEND_STATE	"ResendState"
+
+/* GetCaifIfName requests the CAIF Interface Name. */
+#define MID_GET_CAIFIF		"GetCaifIfName"
+
+#define TIMEOUT		5000
+#define STE_DRIVER_NAME	"ste"
+#define STE_INTERFACE_PROPERTY	"Interface"
+
+enum ste_state {
+	STE_PENDING,    /* Waiting for caifif and "on" notification */
+	STE_PENDING_IF, /* Waiting for caifif, already got "on" */
+	STE_REGISTERED, /* Modem is registered */
+	STE_RESETTING   /* Modem is resetting */
+};
+
+struct ste_modem {
+	char *path;
+	struct ofono_modem *modem;
+	enum ste_state state;
+	gboolean got_caifif;
+	gboolean pending_call;
+};
+
+static GHashTable *modem_list;
+static guint mid_api_watch;
+static guint mid_state_watch;
+static DBusConnection *connection;
+
+static void get_caifif_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply;
+	char *ifname;
+	struct ste_modem *stemodem = user_data;
+
+	stemodem->pending_call = FALSE;
+	stemodem->got_caifif = TRUE;
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (dbus_message_is_error(reply, DBUS_ERROR_SERVICE_UNKNOWN)) {
+		ofono_error("STE Modem Init Daemon: unavailable");
+		goto error;
+	}
+
+	if (dbus_message_get_args(reply, NULL, DBUS_TYPE_STRING, &ifname,
+					DBUS_TYPE_INVALID) == FALSE) {
+		ofono_error("STE Modem Init Daemon: bad signature for %s",
+				MID_GET_CAIFIF);
+		goto error;
+	}
+
+	if (strlen(ifname) > IF_NAMESIZE) {
+		ofono_error("STE Modem Init Daemon: %s bad response %s\n",
+				MID_GET_CAIFIF, ifname);
+		goto error;
+	}
+
+	if (ifname != NULL) {
+		DBG("STE Modem '%s' uses CAIF interface '%s'\n", stemodem->path,
+			ifname);
+		ofono_modem_set_string(stemodem->modem, STE_INTERFACE_PROPERTY,
+			ifname);
+	}
+
+error:
+	if (stemodem->state == STE_PENDING_IF) {
+		DBG("Register STE modem %s", stemodem->path);
+		ofono_modem_register(stemodem->modem);
+		stemodem->state = STE_REGISTERED;
+	}
+
+	dbus_message_unref(reply);
+}
+
+static void get_caif_interface(const char *path, struct ste_modem *stemodem)
+{
+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	stemodem->pending_call = TRUE;
+	message = dbus_message_new_method_call(MID_SERVICE, path,
+					MID_INTERFACE, MID_GET_CAIFIF);
+
+	if (!message) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	dbus_message_set_auto_start(message, FALSE);
+
+	if (dbus_connection_send_with_reply(connection, message,
+						&call, TIMEOUT) == FALSE) {
+		ofono_error("Sending D-Bus message failed");
+		stemodem->got_caifif = TRUE;
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		stemodem->got_caifif = TRUE;
+		goto error;
+	}
+
+	dbus_pending_call_set_notify(call, get_caifif_reply, stemodem, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static void handle_stemodem(const char *action, const char *path)
+{
+	struct ste_modem *stemodem = g_hash_table_lookup(modem_list, path);
+
+	if (stemodem == NULL) {
+		DBG("created STE modem %s", path);
+		stemodem = g_try_new0(struct ste_modem, 1);
+		if (stemodem == NULL)
+			return;
+
+		/* remove '/' from path to create a leagal modem name */
+		stemodem->modem = ofono_modem_create(path+1, STE_DRIVER_NAME);
+		if (stemodem->modem == NULL) {
+			ofono_error("STE Modem Init: Bad path: '%s'\n", path);
+			g_free(stemodem);
+			return;
+		}
+
+		stemodem->path = g_strdup(path);
+		stemodem->state = STE_PENDING;
+		g_hash_table_insert(modem_list, stemodem->path, stemodem);
+	}
+
+	if (!stemodem->got_caifif) {
+		DBG("request caif interface %s", path);
+		get_caif_interface(path, stemodem);
+		stemodem->state = STE_PENDING;
+	}
+
+	switch (stemodem->state) {
+	case STE_PENDING:
+		if (g_strcmp0(action, MID_ACTION_ON) != 0)
+			break;
+		if (stemodem->got_caifif) {
+			DBG("register STE modem %s", path);
+			ofono_modem_register(stemodem->modem);
+			stemodem->state = STE_REGISTERED;
+		} else
+			stemodem->state = STE_PENDING_IF;
+		break;
+	case STE_PENDING_IF:
+		break;
+	case STE_REGISTERED:
+		if (g_strcmp0(action, MID_ACTION_RESET) == 0) {
+			DBG("reset ongoing %s", path);
+			/* NOTE: Should we set powered = FALSE here? */
+			stemodem->state = STE_RESETTING;
+		} else if (g_strcmp0(action, MID_ACTION_ON) != 0) {
+			DBG("STE modem unregistering %s %s", path, action);
+			g_hash_table_remove(modem_list, path);
+		}
+
+		break;
+	case STE_RESETTING:
+		if (g_strcmp0(action, MID_ACTION_ON) == 0) {
+			DBG("STE modem reset complete %s", path);
+			if (ofono_modem_get_powered(stemodem->modem))
+				ofono_modem_reset(stemodem->modem);
+		} else if (g_strcmp0(action, MID_ACTION_RESET) != 0) {
+			DBG("STE modem unregistering %s %s", path, action);
+			g_hash_table_remove(modem_list, path);
+		}
+
+		break;
+	}
+}
+
+static gboolean mid_signal_status_change(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	const char *state;
+	const char *path = dbus_message_get_path(message);
+
+	if (dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &state,
+					DBUS_TYPE_INVALID) == FALSE) {
+		ofono_error("STE Modem Init Daemon: invalid signal signature");
+		return FALSE;
+	}
+
+	if (g_strcmp0(path, "/") == 0)
+		return TRUE;
+
+	handle_stemodem(state, path);
+	return TRUE;
+}
+
+static void mid_connect(DBusConnection *connection, void *user_data)
+{
+	DBusMessage *message;
+
+	mid_state_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
+						MID_INTERFACE,
+						STATUS_CHANGED,
+						mid_signal_status_change,
+						NULL, NULL);
+
+	message = dbus_message_new_method_call(MID_SERVICE, "/",
+					MID_INTERFACE, MID_RESEND_STATE);
+	if (!message) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	if (dbus_connection_send(connection, message, NULL) == FALSE)
+		ofono_error("Sending D-Bus message failed");
+
+error:
+	dbus_message_unref(message);
+}
+
+static void mid_disconnect(DBusConnection *connection, void *user_data)
+{
+	g_hash_table_remove_all(modem_list);
+	g_dbus_remove_watch(connection, mid_state_watch);
+}
+
+static void destroy_stemodem(gpointer data)
+{
+	struct ste_modem *stemodem = data;
+
+	ofono_modem_remove(stemodem->modem);
+	g_free(stemodem->path);
+	g_free(stemodem);
+}
+
+static int stemid_init()
+{
+	connection = ofono_dbus_get_connection();
+	if (connection == NULL)
+		return -EIO;
+
+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);
+	mid_api_watch = g_dbus_add_service_watch(connection, MID_SERVICE,
+				mid_connect, mid_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemid_exit()
+{
+	DBusConnection *connection = ofono_dbus_get_connection();
+
+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, mid_api_watch);
+}
+
+OFONO_PLUGIN_DEFINE(stemid, "STE Modem Init Daemon dbus api", VERSION,
+			OFONO_PLUGIN_PRIORITY_DEFAULT, stemid_init, stemid_exit)
-- 
1.7.0.4

[PATCHv2 1/2] stemodem: Create network interfaces statically

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Changes:
o Restructure code so interfaces are created statically when probe is called,
  instead of creating interfaces dynamically at activation.
o Changed debug messages.
o Removed some of the unnecessary initializations at declaration.
o No longer reporting "default gateway" for PtP IP Interface.
o Bugfix: Handle network initiated deactivation.
---
 drivers/stemodem/gprs-context.c |  149 +++++++++++++++++++++-----------------
 1 files changed, 82 insertions(+), 67 deletions(-)

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index faa3124..7bdcdb8 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -28,6 +28,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <errno.h>
 
 #include <glib.h>
 
@@ -65,10 +66,18 @@ struct gprs_context_data {
 };
 
 struct conn_info {
+	/*
+	 * cid is allocated in oFono Core and is identifying
+	 * the Account. cid = 0 indicates that it is currently unused.
+	 */
 	unsigned int cid;
-	unsigned int device;
+	/* Id used by CAIF and EPPSD to identify the CAIF channel*/
 	unsigned int channel_id;
-	char interface[10];
+	/* Linux Interface Id */
+	unsigned int ifindex;
+	/* Linux Interface name */
+	char interface[IF_NAMESIZE];
+	gboolean created;
 };
 
 struct eppsd_response {
@@ -76,7 +85,6 @@ struct eppsd_response {
 	char ip_address[IP_ADDR_LEN];
 	char subnet_mask[IP_ADDR_LEN];
 	char mtu[IP_ADDR_LEN];
-	char default_gateway[IP_ADDR_LEN];
 	char dns_server1[IP_ADDR_LEN];
 	char dns_server2[IP_ADDR_LEN];
 	char p_cscf_server[IP_ADDR_LEN];
@@ -96,8 +104,6 @@ static void start_element_handler(GMarkupParseContext *context,
 		rsp->current = rsp->subnet_mask;
 	else if (!strcmp(element_name, "mtu"))
 		rsp->current = rsp->mtu;
-	else if (!strcmp(element_name, "default_gateway"))
-		rsp->current = rsp->default_gateway;
 	else if (!strcmp(element_name, "dns_server") &&
 					rsp->dns_server1[0] == '\0')
 		rsp->current = rsp->dns_server1;
@@ -123,7 +129,7 @@ static void text_handler(GMarkupParseContext *context,
 
 	if (rsp->current) {
 		strncpy(rsp->current, text, IP_ADDR_LEN);
-		rsp->current[IP_ADDR_LEN] = 0;
+		rsp->current[IP_ADDR_LEN] = '\0';
 	}
 }
 
@@ -153,8 +159,7 @@ static gint conn_compare_by_cid(gconstpointer a, gconstpointer b)
 	return 0;
 }
 
-static struct conn_info *conn_info_create(unsigned int device,
-						unsigned int channel_id)
+static struct conn_info *conn_info_create(unsigned int channel_id)
 {
 	struct conn_info *connection = g_try_new0(struct conn_info, 1);
 
@@ -162,7 +167,6 @@ static struct conn_info *conn_info_create(unsigned int device,
 		return NULL;
 
 	connection->cid = 0;
-	connection->device = device;
 	connection->channel_id = channel_id;
 
 	return connection;
@@ -171,7 +175,7 @@ static struct conn_info *conn_info_create(unsigned int device,
 /*
  * Creates a new IP interface for CAIF.
  */
-static gboolean caif_if_create(const char *interface, unsigned int connid)
+static gboolean caif_if_create(struct conn_info *conn)
 {
 	return FALSE;
 }
@@ -179,9 +183,8 @@ static gboolean caif_if_create(const char *interface, unsigned int connid)
 /*
  * Removes IP interface for CAIF.
  */
-static gboolean caif_if_remove(const char *interface, unsigned int connid)
+static void caif_if_remove(struct conn_info *conn)
 {
-	return FALSE;
 }
 
 static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
@@ -210,21 +213,13 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
 		DBG("Did not find data (used caif device) for"
 					"connection with cid; %d",
 					gcd->active_context);
-		goto error;
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
 	}
 
 	conn = l->data;
-
-	if (!caif_if_remove(conn->interface, conn->channel_id)) {
-		DBG("Failed to remove caif interface %s.",
-				conn->interface);
-	}
-
 	conn->cid = 0;
-	return;
-
-error:
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	CALLBACK_WITH_SUCCESS(cb, cbd->data);
 }
 
 static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
@@ -233,7 +228,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	ofono_gprs_context_up_cb_t cb = cbd->cb;
 	struct ofono_gprs_context *gc = cbd->user;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-	struct conn_info *conn = NULL;
+	struct conn_info *conn;
 	GAtResultIter iter;
 	GSList *l;
 	int i;
@@ -241,17 +236,15 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	const char *res_string;
 	const char *dns[MAX_DNS + 1];
 	struct eppsd_response rsp;
-	GMarkupParseContext *context = NULL;
+	GMarkupParseContext *context;
 
 	l = g_slist_find_custom(g_caif_devices,
 				GUINT_TO_POINTER(gcd->active_context),
 				conn_compare_by_cid);
 
 	if (l == NULL) {
-		DBG("Did not find data (device and channel id)"
-					"for connection with cid; %d",
-					gcd->active_context);
-		goto error;
+		DBG("CAIF Device gone missing (cid:%d)", gcd->active_context);
+		goto error_no_device;
 	}
 
 	conn = l->data;
@@ -291,31 +284,21 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	dns[1] = rsp.dns_server2;
 	dns[2] = NULL;
 
-	sprintf(conn->interface, "caif%u", conn->device);
-
-	if (!caif_if_create(conn->interface, conn->channel_id)) {
-		ofono_error("Failed to create caif interface %s.",
-				conn->interface);
-		CALLBACK_WITH_SUCCESS(cb, NULL, FALSE, rsp.ip_address,
-				rsp.subnet_mask, rsp.default_gateway,
+	CALLBACK_WITH_SUCCESS(cb, conn->interface, TRUE, rsp.ip_address,
+				rsp.subnet_mask, NULL,
 				dns, cbd->data);
-	} else {
-		CALLBACK_WITH_SUCCESS(cb, conn->interface,
-				FALSE, rsp.ip_address, rsp.subnet_mask,
-				rsp.default_gateway, dns, cbd->data);
-	}
-
 	return;
 
 error:
-	DBG("ste_eppsd_up_cb error");
-
 	if (context)
 		g_markup_parse_context_free(context);
 
 	if (conn)
 		conn->cid = 0;
 
+error_no_device:
+	DBG("ste_eppsd_up_cb error");
+	gcd->active_context = 0;
 	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
 }
 
@@ -325,44 +308,45 @@ static void ste_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	ofono_gprs_context_up_cb_t cb = cbd->cb;
 	struct ofono_gprs_context *gc = cbd->user;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-	struct cb_data *ncbd = NULL;
+	struct cb_data *ncbd;
 	char buf[128];
 	struct conn_info *conn;
 	GSList *l;
 
+	l = g_slist_find_custom(g_caif_devices,
+				GUINT_TO_POINTER(gcd->active_context),
+				conn_compare_by_cid);
+
+	if (!l) {
+		DBG("CAIF Device gone missing (cid:%d)", gcd->active_context);
+		goto error_no_device;
+	}
+
+	conn = l->data;
+
 	if (!ok) {
 		struct ofono_error error;
 
+		conn->cid = 0;
 		gcd->active_context = 0;
 		decode_at_error(&error, g_at_result_final_response(result));
 		cb(&error, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
 		return;
 	}
 
-	ncbd = g_memdup(cbd, sizeof(struct cb_data));
-
-	l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(0),
-				conn_compare_by_cid);
-
-	if (l == NULL) {
-		DBG("at_cgdcont_cb, no more available devices");
-		goto error;
-	}
-
-	conn = l->data;
-	conn->cid = gcd->active_context;
 	snprintf(buf, sizeof(buf), "AT*EPPSD=1,%u,%u",
 			conn->channel_id, conn->cid);
+	ncbd = g_memdup(cbd, sizeof(struct cb_data));
 
 	if (g_at_chat_send(gcd->chat, buf, NULL,
 				ste_eppsd_up_cb, ncbd, g_free) > 0)
 		return;
 
-error:
 	g_free(ncbd);
+	conn->cid = 0;
 
+error_no_device:
 	gcd->active_context = 0;
-
 	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL,
 				NULL, NULL, cbd->data);
 }
@@ -375,13 +359,32 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
 	struct cb_data *cbd = cb_data_new(cb, data);
 	char buf[AUTH_BUF_LENGTH];
 	int len;
+	GSList *l;
+	struct conn_info *conn;
 
 	if (cbd == NULL)
-		goto error;
+		goto error_no_device;
 
 	gcd->active_context = ctx->cid;
 	cbd->user = gc;
 
+	/* Find free connection with cid zero */
+	l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(0),
+				conn_compare_by_cid);
+
+	if (!l) {
+		DBG("No more available CAIF devices");
+		goto error_no_device;
+	}
+
+	conn = l->data;
+	conn->cid = ctx->cid;
+
+	if (!conn->created) {
+		DBG("CAIF interface not created (rtnl error?)");
+		goto error;
+	}
+
 	len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"", ctx->cid);
 
 	if (ctx->apn)
@@ -405,6 +408,9 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
 	return;
 
 error:
+	conn->cid = 0;
+
+error_no_device:
 	gcd->active_context = 0;
 	g_free(cbd);
 
@@ -431,8 +437,8 @@ static void ste_gprs_deactivate_primary(struct ofono_gprs_context *gc,
 				conn_compare_by_cid);
 
 	if (l == NULL) {
-		DBG("at_gprs_deactivate_primary, did not find"
-			"data (channel id) for connection with cid; %d", id);
+		DBG("did not find data (channel id) "
+				"for connection with cid; %d", id);
 		goto error;
 	}
 
@@ -446,7 +452,6 @@ static void ste_gprs_deactivate_primary(struct ofono_gprs_context *gc,
 
 error:
 	g_free(cbd);
-
 	CALLBACK_WITH_FAILURE(cb, data);
 }
 
@@ -457,6 +462,7 @@ static void ste_cgact_read_cb(gboolean ok, GAtResult *result,
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 	gint cid, state;
 	GAtResultIter iter;
+	GSList *l;
 
 	if (!ok)
 		return;
@@ -480,7 +486,13 @@ static void ste_cgact_read_cb(gboolean ok, GAtResult *result,
 		ofono_gprs_context_deactivated(gc, gcd->active_context);
 		gcd->active_context = 0;
 
-		break;
+		/* Mark interface as unused */
+		l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(cid),
+				conn_compare_by_cid);
+		if (l != NULL) {
+			struct conn_info *conn = l->data;
+			conn->cid = 0;
+		}
 	}
 }
 
@@ -524,9 +536,11 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
 	ofono_gprs_context_set_data(gc, gcd);
 
 	for (i = 0; i < MAX_CAIF_DEVICES; i++) {
-		ci = conn_info_create(i, i+1);
-		if (ci)
-			g_caif_devices = g_slist_append(g_caif_devices, ci);
+		ci = conn_info_create(i+1);
+		if (!ci)
+			return -ENOMEM;
+		caif_if_create(ci);
+		g_caif_devices = g_slist_append(g_caif_devices, ci);
 	}
 
 	return 0;
@@ -536,6 +550,7 @@ static void ste_gprs_context_remove(struct ofono_gprs_context *gc)
 {
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 
+	g_slist_foreach(g_caif_devices, (GFunc) caif_if_remove, NULL);
 	g_slist_foreach(g_caif_devices, (GFunc) g_free, NULL);
 	g_slist_free(g_caif_devices);
 	g_caif_devices = NULL;
-- 
1.7.0.4

[PATCHv2 2/2] stemodem: Use RTNL to create network interfaces.

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

---
 drivers/stemodem/gprs-context.c |   43 ++++++++++++++++++++++++++++++++------
 1 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
index 7bdcdb8..1762fb3 100644
--- a/drivers/stemodem/gprs-context.c
+++ b/drivers/stemodem/gprs-context.c
@@ -47,6 +47,7 @@
 #include "stemodem.h"
 #include "caif_socket.h"
 #include "if_caif.h"
+#include "caif_rtnl.h"
 
 #define MAX_CAIF_DEVICES 4
 #define MAX_DNS 2
@@ -172,12 +173,20 @@ static struct conn_info *conn_info_create(unsigned int channel_id)
 	return connection;
 }
 
-/*
- * Creates a new IP interface for CAIF.
- */
-static gboolean caif_if_create(struct conn_info *conn)
+static void rtnl_callback(int ifindex, const char *ifname, void *user_data)
 {
-	return FALSE;
+	struct conn_info *conn = user_data;
+
+	if (ifindex < 0) {
+		conn->created = FALSE;
+		ofono_error("Failed to create caif interface %s",
+			conn->interface);
+		return;
+	}
+
+	strncpy(conn->interface, ifname, sizeof(conn->interface));
+	conn->ifindex = ifindex;
+	conn->created = TRUE;
 }
 
 /*
@@ -185,6 +194,17 @@ static gboolean caif_if_create(struct conn_info *conn)
  */
 static void caif_if_remove(struct conn_info *conn)
 {
+	if (!conn->created)
+		return;
+
+	if (caif_rtnl_delete_interface(conn->ifindex) < 0) {
+		ofono_error("Failed to delete caif interface %s",
+			conn->interface);
+		return;
+	}
+
+	DBG("removed CAIF interface ch:%d ifname:%s ifindex:%d\n",
+		conn->channel_id, conn->interface, conn->ifindex);
 }
 
 static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
@@ -526,7 +546,7 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
 	GAtChat *chat = data;
 	struct gprs_context_data *gcd;
 	struct conn_info *ci;
-	int i;
+	int i, err;
 
 	gcd = g_new0(struct gprs_context_data, 1);
 	gcd->chat = g_at_chat_clone(chat);
@@ -539,7 +559,14 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
 		ci = conn_info_create(i+1);
 		if (!ci)
 			return -ENOMEM;
-		caif_if_create(ci);
+		err = caif_rtnl_create_interface(IFLA_CAIF_IPV4_CONNID,
+						ci->channel_id, FALSE,
+						rtnl_callback, ci);
+		if (err < 0) {
+			DBG("Failed to create IP interface for CAIF");
+			return err;
+		}
+
 		g_caif_devices = g_slist_append(g_caif_devices, ci);
 	}
 
@@ -571,10 +598,12 @@ static struct ofono_gprs_context_driver driver = {
 
 void ste_gprs_context_init()
 {
+	caif_rtnl_init();
 	ofono_gprs_context_driver_register(&driver);
 }
 
 void ste_gprs_context_exit()
 {
 	ofono_gprs_context_driver_unregister(&driver);
+	caif_rtnl_exit();
 }
-- 
1.7.0.4

Re: [PATCHv2] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> This patch introduces auto discovery of ST-Ericsson modems.
> ST-Ericsson modems (M5XX, M57XX, M7XX, M74XX) are managed by a
> Modem Init Daemon responsible for start, power cycles,
> flashing etc. This STE plugin monitors the modem state exposed
> from the Modem Init Damon's Dbus API. When the modem is in state
> "on" the STE modem is created and registered. Multiple modem
> instances, and reset handling is supported.

I don't really like on how STE, ST and TI do abbreviations these days.

It is MID for modem init daemon, it is UIM for user init manager, KIM
for kernel init manager and TI calls a shared transport ST. This will
end up in a massive confusion at some point ;)

> ---
> Changes:
> - Support for multiple STE modems.
> - Better naming of STE modem instances, using received dbus path.
> - CAIF Link layer to use for AT channels specified pr modem instance.
> - Added support for reset.
> 
>  Makefile.am      |    5 +
>  configure.ac     |    6 +
>  plugins/stemid.c |  310 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 321 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/stemid.c

Can we just call this something like stemgr for STE Manager or STE Init
Manager. The MID part is just confusing.

> diff --git a/Makefile.am b/Makefile.am
> index 12b3c33..98096ae 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -82,6 +82,11 @@ gatchat_sources = gatchat/gatchat.h gatchat/gatchat.c \
>  
>  udev_files = plugins/ofono.rules
>  
> +if STE_MODEM_INITD
> +builtin_modules += stemid
> +builtin_sources += plugins/stemid.c
> +endif
> +
>  if UDEV
>  builtin_modules += udev
>  builtin_sources += plugins/udev.c
> diff --git a/configure.ac b/configure.ac
> index 5c18f68..f733fc4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -177,6 +177,12 @@ AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles],
>  
>  AM_CONDITIONAL(DATAFILES, test "${enable_datafiles}" != "no")
>  
> +AC_ARG_ENABLE(ste_modem_initd, AC_HELP_STRING([--disable-ste-modem-initd],
> +			[disable auto discovery of STE modem using modem init daemon]),
> +					[enable_ste_modem_initd=${enableval}])
> +
> +AM_CONDITIONAL(STE_MODEM_INITD, test "${enable_ste_modem_initd}" != "no")
> +

Don't bother with this and just always compile this in. For now that is
fine and in case we start with more fine grained control here, I will do
that on modem technology based and not on plugins.

Right now we do atmodem and isimodem. If we feel like it is needed, we
would do stemodem, but right now, I don't see it.

>  if (test "${prefix}" = "NONE"); then
>  	dnl no prefix and no localstatedir, so default to /var
>  	if (test "$localstatedir" = '${prefix}/var'); then
> diff --git a/plugins/stemid.c b/plugins/stemid.c
> new file mode 100644
> index 0000000..9f5da22
> --- /dev/null
> +++ b/plugins/stemid.c
> @@ -0,0 +1,310 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2010 ST-Ericsson AB.
> + *
> + *  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 <errno.h>
> +#include <string.h>
> +#include <net/if.h>
> +
> +#include <glib.h>
> +
> +#define OFONO_API_SUBJECT_TO_CHANGE
> +#include <ofono/plugin.h>
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/dbus.h>
> +
> +/*
> + * ST-Ericsson's Modem Init Daemon is used for controling the modem power
> + * cycles and provides a dbus API for modem state and properties.
> + */
> +#define MID_SERVICE		"com.stericsson.modeminit"
> +#define MID_INTERFACE		MID_SERVICE ".Modem"
> +
> +/* The signal StateChange sends the modem state.*/
> +#define STATUS_CHANGED		"StateChange"
> +
> +#define MID_ACTION_ON		"on"		/* Modem is on */
> +#define MID_ACTION_RESET	"dumping"	/* Modem is resetting */
> +#define MID_ACTION_UNKNOWN	"unknown"	/* Don't care */
> +
> +/* ResetState requests resending of StateChange. */
> +#define MID_RESEND_STATE	"ResendState"

This is a bad D-Bus API btw. Using a D-Bus method to trigger signal
sending. You will wake up any listener for that signal. What is wrong
with just having a GetState?

> +/* GetCaifIfName requests the CAIF Interface Name. */
> +#define MID_GET_CAIFIF		"GetCaifIfName"
> +
> +#define TIMEOUT		5000
> +#define STE_DRIVER_NAME	"ste"

I don't think you should be doing the naming like this. Don't you have a
proper serial number? If not, then use at least

> +#define STE_INTERFACE_PROPERTY	"Interface"

One time it is a property, and then you have a method call? What is it
now?

> +enum ste_state {
> +	STE_PENDING,    /* Waiting for caifif and "on" notification */
> +	STE_PENDING_IF, /* Waiting for caifif, already got "on" */
> +	STE_REGISTERED, /* Modem is registered */
> +	STE_RESETTING   /* Modem is resetting */
> +};
> +
> +struct ste_modem {
> +	char *path;
> +	struct ofono_modem *modem;
> +	enum ste_state state;
> +	gboolean got_caifif;
> +	gboolean pending_call;
> +};
> +
> +static GHashTable *modem_list;
> +static guint mid_api_watch;
> +static guint mid_state_watch;
> +static DBusConnection *connection;

<snip>

I have not looked at anything in between just yet.

> +static int stemid_init()
> +{
> +	connection = ofono_dbus_get_connection();
> +	if (connection == NULL)
> +		return -EIO;

Don't bother with the check here. It will not fail. It if fails, you
have a seriously other problem.

> +	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
> +						NULL, destroy_stemodem);
> +	mid_api_watch = g_dbus_add_service_watch(connection, MID_SERVICE,
> +				mid_connect, mid_disconnect, NULL, NULL);
> +	return 0;
> +}
> +
> +static void stemid_exit()
> +{
> +	DBusConnection *connection = ofono_dbus_get_connection();

Why not use the global one you stored. Why call the function again?

> +
> +	g_hash_table_destroy(modem_list);
> +	g_dbus_remove_watch(connection, mid_api_watch);
> +}
> +
> +OFONO_PLUGIN_DEFINE(stemid, "STE Modem Init Daemon dbus api", VERSION,
> +			OFONO_PLUGIN_PRIORITY_DEFAULT, stemid_init, stemid_exit)

The plugin description is wrong. You are not providing a D-Bus API here.

Regards

Marcel

Re: [PATCHv2 1/2] stemodem: Create network interfaces statically

[Marcel Holtmann]

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

Hi Sjur,

> Changes:
> o Restructure code so interfaces are created statically when probe is called,
>   instead of creating interfaces dynamically at activation.
> o Changed debug messages.
> o Removed some of the unnecessary initializations at declaration.
> o No longer reporting "default gateway" for PtP IP Interface.
> o Bugfix: Handle network initiated deactivation.
> ---
>  drivers/stemodem/gprs-context.c |  149 +++++++++++++++++++++-----------------
>  1 files changed, 82 insertions(+), 67 deletions(-)

looks good to me now. Patch has been applied.

Regards

Marcel

Re: [PATCHv2 2/2] stemodem: Use RTNL to create network interfaces.

[Marcel Holtmann]

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

Hi Sjur,

>  drivers/stemodem/gprs-context.c |   43 ++++++++++++++++++++++++++++++++------
>  1 files changed, 36 insertions(+), 7 deletions(-)

patch has been applied.

Regards

Marcel

Re: [PATCHv2] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.

>> +
>> +/* ResetState requests resending of StateChange. */
>> +#define MID_RESEND_STATE     "ResendState"
>
> This is a bad D-Bus API btw. Using a D-Bus method to trigger signal
> sending. You will wake up any listener for that signal. What is wrong
> with just having a GetState?

My problem is that I need to get the path, or modem serial-number if
you like, to the
modem(s). As I don't know the start order I have to query the state at startup.
So this will happens only once. The simples way for me to get the path is
to send "ResendState" to path "/". This will then send the state info
for all modem
paths.

I don't think I have the path them available at connect...
If you have any other ideas of how to query the modem instances I'm all ears?


The other is feedback makes sens to me ;-)
Thanks for review and feedback.


Regards,
Sjur

Re: [PATCHv2] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> >> +
> >> +/* ResetState requests resending of StateChange. */
> >> +#define MID_RESEND_STATE     "ResendState"
> >
> > This is a bad D-Bus API btw. Using a D-Bus method to trigger signal
> > sending. You will wake up any listener for that signal. What is wrong
> > with just having a GetState?
> 
> My problem is that I need to get the path, or modem serial-number if
> you like, to the
> modem(s). As I don't know the start order I have to query the state at startup.
> So this will happens only once. The simples way for me to get the path is
> to send "ResendState" to path "/". This will then send the state info
> for all modem
> paths.
> 
> I don't think I have the path them available at connect...
> If you have any other ideas of how to query the modem instances I'm all ears?

I think you need to resend the D-Bus API of your init daemon. I need to
have a second look.

Regards

Marcel

Re: [PATCHv2] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.

>> > This is a bad D-Bus API btw. Using a D-Bus method to trigger signal
>> > sending. You will wake up any listener for that signal. What is wrong
>> > with just having a GetState?
>>
>> My problem is that I need to get the path, or modem serial-number if
>> you like, to the
>> modem(s). As I don't know the start order I have to query the state at startup.
>> So this will happens only once. The simples way for me to get the path is
>> to send "ResendState" to path "/". This will then send the state info
>> for all modem
>> paths.
>>
>> I don't think I have the path them available at connect...
>> If you have any other ideas of how to query the modem instances I'm all ears?
>
> I think you need to resend the D-Bus API of your init daemon. I need to
> have a second look.

This is an extract of the Modem Init Daemon D-Bus API.
I have removed the GetState API as we don't know the object
path of the modem when oFOno has just booted. Instead we
have added the ResendState API. This will be called initially
when oFono starts to query the modem instances and their
state. After startup state changes are handed as signals
with StateChange. (There are some exceptions to this but
they should be only corner cases).


Modem Init Deamon
================

Service com.stericsson.modeminit
Interface       com.stericsson.modeminit.Modem
Object path     /{modem-serial} or /

Methods
		void  ResendState()

                      Resends state information for all modems.
		      This method must use with '/' as object path.

		string GetCaifIfName()

                      Returns the CAIF Link Layer interface used for
		      AT channels for a specific modem. This method
		      must use the same object path as used in StateChange.


Signals  StateChange(string status)

                       The modems state sent from Modem Init Daemon when
                       a modem state change occurs. The object path must
		       be the identifier of the modem (typically
                       the serial-id or IMEI of the HW).

                       "booting"   Modem is powered up (flashed version)
                                   or Modem is powered up and firmware upload
                                   is completed. (flashless version)
                       "upgrading" Firmware upgrade on going
                                   or Flashing manager under execution -
                                   modem in service mode.
                       "on"     Modem has booted and is ready for use.
                                   This implies that all AT channels are
                                   available, the modem might be in
                                   e.g. flight mode.
                       "dumping"  Modem has crashed and dump is ongoing
                       "off"       Modem is powered off.


Regards,
Sjur

[PATCHv3] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch introduces auto discovery of ST-Ericsson modems.
ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed
from the Modem Init Damon's Dbus API. When the modem is in state
"on" the STE modem is created and registered. Muliple modem
instances, and reset handling is supported.
---
Hi Marcel.

I think I have resolved most of your comments on my last patch set,
including the massive confusion caused by calling this MID ;-)

The only remark left is "ResendState". I have done this as simple as 
possible where you just re-send all state information for all modem
instances and implicit get the object-path (serial-id) for each modem
when the StateChange signal is received.

I really don't think resending the state information should be of much
concern as this only happens once - when oFono starts. The extra cost
of waking the other two STE daemons should be negligible, as they most
likely are initializing and preparing for communicating with the STE-modem
anyway.

Regards,
Sjur

 Makefile.am      |    3 +
 plugins/stemgr.c |  310 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 313 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

diff --git a/Makefile.am b/Makefile.am
index 12b3c33..d9c0505 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -221,6 +221,9 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/ifxmodem/gprs-context.c \
 			drivers/ifxmodem/stk.c
 
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += stemodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/stemodem/stemodem.h \
diff --git a/plugins/stemgr.c b/plugins/stemgr.c
new file mode 100644
index 0000000..6317516
--- /dev/null
+++ b/plugins/stemgr.c
@@ -0,0 +1,309 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *
+ *  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 <errno.h>
+#include <string.h>
+#include <net/if.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+
+/*
+ * ST-Ericsson's Modem Init Daemon is used for controling the modem power
+ * cycles and provides a dbus API for modem state and properties.
+ */
+#define MGR_SERVICE		"com.stericsson.modeminit"
+#define MGR_INTERFACE		MGR_SERVICE ".Modem"
+
+/* The signal StateChange sends the modem state.*/
+#define STATUS_CHANGED		"StateChange"
+
+#define MGR_ACTION_ON		"on"		/* Modem is on */
+#define MGR_ACTION_RESET	"dumping"	/* Modem is resetting */
+
+/*
+ * ResendState requests resending of StateChange. This method is only
+ * called at startup, in order to retrieve the modem's serial id,
+ * (object path) and state.
+ */
+#define MGR_RESEND_STATE	"ResendState"
+
+/* GetCaifIfName requests the CAIF Interface Name. */
+#define MGR_GET_CAIFIF		"GetCaifIfName"
+
+#define TIMEOUT		5000
+
+enum ste_state {
+	STE_PENDING,    /* Waiting for caifif and "on" notification */
+	STE_PENDING_IF, /* Waiting for caifif, already got "on" */
+	STE_REGISTERED, /* Modem is registered */
+	STE_RESETTING   /* Modem is resetting */
+};
+
+struct ste_modem {
+	char *path;
+	struct ofono_modem *modem;
+	enum ste_state state;
+	gboolean got_caifif;
+	gboolean pending_call;
+};
+
+static GHashTable *modem_list;
+static guint mgr_api_watch;
+static guint mgr_state_watch;
+static DBusConnection *connection;
+
+static void get_caifif_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply;
+	char *ifname;
+	struct ste_modem *stemodem = user_data;
+
+	stemodem->pending_call = FALSE;
+	stemodem->got_caifif = TRUE;
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (dbus_message_is_error(reply, DBUS_ERROR_SERVICE_UNKNOWN)) {
+		ofono_error("STE Modem Init Daemon: unavailable");
+		goto error;
+	}
+
+	if (dbus_message_get_args(reply, NULL, DBUS_TYPE_STRING, &ifname,
+					DBUS_TYPE_INVALID) == FALSE) {
+		ofono_error("STE Modem Init Daemon: bad signature for %s",
+				MGR_GET_CAIFIF);
+		goto error;
+	}
+
+	if (strlen(ifname) > IF_NAMESIZE) {
+		ofono_error("STE Modem Init Daemon: %s bad response %s\n",
+				MGR_GET_CAIFIF, ifname);
+		goto error;
+	}
+
+	if (ifname != NULL) {
+		DBG("STE Modem '%s' uses CAIF interface '%s'\n", stemodem->path,
+			ifname);
+		ofono_modem_set_string(stemodem->modem, "Interface",
+			ifname);
+	}
+
+error:
+	if (stemodem->state == STE_PENDING_IF) {
+		DBG("Register STE modem %s", stemodem->path);
+		ofono_modem_register(stemodem->modem);
+		stemodem->state = STE_REGISTERED;
+	}
+
+	dbus_message_unref(reply);
+}
+
+static void get_caif_interface(const char *path, struct ste_modem *stemodem)
+{
+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	stemodem->pending_call = TRUE;
+	message = dbus_message_new_method_call(MGR_SERVICE, path,
+					MGR_INTERFACE, MGR_GET_CAIFIF);
+
+	if (!message) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	dbus_message_set_auto_start(message, FALSE);
+
+	if (dbus_connection_send_with_reply(connection, message,
+						&call, TIMEOUT) == FALSE) {
+		ofono_error("Sending D-Bus message failed");
+		stemodem->got_caifif = TRUE;
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		stemodem->got_caifif = TRUE;
+		goto error;
+	}
+
+	dbus_pending_call_set_notify(call, get_caifif_reply, stemodem, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static void handle_stemodem(const char *action, const char *path)
+{
+	struct ste_modem *stemodem = g_hash_table_lookup(modem_list, path);
+
+	if (stemodem == NULL) {
+		DBG("created STE modem %s", path);
+		stemodem = g_try_new0(struct ste_modem, 1);
+		if (stemodem == NULL)
+			return;
+
+		/* remove '/' from path to create a leagal modem name */
+		stemodem->modem = ofono_modem_create(path+1, "ste");
+		if (stemodem->modem == NULL) {
+			ofono_error("STE Modem Init: Bad path: '%s'\n", path);
+			g_free(stemodem);
+			return;
+		}
+
+		stemodem->path = g_strdup(path);
+		stemodem->state = STE_PENDING;
+		g_hash_table_insert(modem_list, stemodem->path, stemodem);
+	}
+
+	if (!stemodem->got_caifif) {
+		DBG("request caif interface %s", path);
+		get_caif_interface(path, stemodem);
+		stemodem->state = STE_PENDING;
+	}
+
+	switch (stemodem->state) {
+	case STE_PENDING:
+		if (g_strcmp0(action, MGR_ACTION_ON) != 0)
+			break;
+		if (stemodem->got_caifif) {
+			DBG("register STE modem %s", path);
+			ofono_modem_register(stemodem->modem);
+			stemodem->state = STE_REGISTERED;
+		} else
+			stemodem->state = STE_PENDING_IF;
+		break;
+	case STE_PENDING_IF:
+		break;
+	case STE_REGISTERED:
+		if (g_strcmp0(action, MGR_ACTION_RESET) == 0) {
+			DBG("reset ongoing %s", path);
+			/* NOTE: Should we set powered = FALSE here? */
+			stemodem->state = STE_RESETTING;
+		} else if (g_strcmp0(action, MGR_ACTION_ON) != 0) {
+			DBG("STE modem unregistering %s %s", path, action);
+			g_hash_table_remove(modem_list, path);
+		}
+
+		break;
+	case STE_RESETTING:
+		if (g_strcmp0(action, MGR_ACTION_ON) == 0) {
+			DBG("STE modem reset complete %s", path);
+			if (ofono_modem_get_powered(stemodem->modem))
+				ofono_modem_reset(stemodem->modem);
+		} else if (g_strcmp0(action, MGR_ACTION_RESET) != 0) {
+			DBG("STE modem unregistering %s %s", path, action);
+			g_hash_table_remove(modem_list, path);
+		}
+
+		break;
+	}
+}
+
+static gboolean mgr_signal_status_change(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	const char *state;
+	const char *path = dbus_message_get_path(message);
+
+	if (dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &state,
+					DBUS_TYPE_INVALID) == FALSE) {
+		ofono_error("STE Modem Init Daemon: invalid signal signature");
+		return FALSE;
+	}
+
+	if (g_strcmp0(path, "/") == 0)
+		return TRUE;
+
+	handle_stemodem(state, path);
+	return TRUE;
+}
+
+static void mgr_connect(DBusConnection *connection, void *user_data)
+{
+	DBusMessage *message;
+
+	mgr_state_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
+						MGR_INTERFACE,
+						STATUS_CHANGED,
+						mgr_signal_status_change,
+						NULL, NULL);
+
+	message = dbus_message_new_method_call(MGR_SERVICE, "/",
+					MGR_INTERFACE, MGR_RESEND_STATE);
+	if (!message) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	if (dbus_connection_send(connection, message, NULL) == FALSE)
+		ofono_error("Sending D-Bus message failed");
+
+error:
+	dbus_message_unref(message);
+}
+
+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+	g_hash_table_remove_all(modem_list);
+	g_dbus_remove_watch(connection, mgr_state_watch);
+}
+
+static void destroy_stemodem(gpointer data)
+{
+	struct ste_modem *stemodem = data;
+
+	ofono_modem_remove(stemodem->modem);
+	g_free(stemodem->path);
+	g_free(stemodem);
+}
+
+static int stemgr_init()
+{
+	connection = ofono_dbus_get_connection();
+
+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);
+	mgr_api_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
+				mgr_connect, mgr_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemgr_exit()
+{
+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, mgr_api_watch);
+}
+
+OFONO_PLUGIN_DEFINE(stemgr, "oFono client of ST-Ericsson Modem Init Daemon",
+			VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
+			stemgr_init, stemgr_exit)
-- 
1.7.0.4

Re: [PATCHv2] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> >> > This is a bad D-Bus API btw. Using a D-Bus method to trigger signal
> >> > sending. You will wake up any listener for that signal. What is wrong
> >> > with just having a GetState?
> >>
> >> My problem is that I need to get the path, or modem serial-number if
> >> you like, to the
> >> modem(s). As I don't know the start order I have to query the state at startup.
> >> So this will happens only once. The simples way for me to get the path is
> >> to send "ResendState" to path "/". This will then send the state info
> >> for all modem
> >> paths.
> >>
> >> I don't think I have the path them available at connect...
> >> If you have any other ideas of how to query the modem instances I'm all ears?
> >
> > I think you need to resend the D-Bus API of your init daemon. I need to
> > have a second look.
> 
> This is an extract of the Modem Init Daemon D-Bus API.
> I have removed the GetState API as we don't know the object
> path of the modem when oFOno has just booted. Instead we
> have added the ResendState API. This will be called initially
> when oFono starts to query the modem instances and their
> state. After startup state changes are handed as signals
> with StateChange. (There are some exceptions to this but
> they should be only corner cases).
> 
> 
> Modem Init Deamon
> ================
> 
> Service com.stericsson.modeminit
> Interface       com.stericsson.modeminit.Modem
> Object path     /{modem-serial} or /
> 
> Methods
> 		void  ResendState()
> 
>                       Resends state information for all modems.
> 		      This method must use with '/' as object path.
> 
> 		string GetCaifIfName()
> 
>                       Returns the CAIF Link Layer interface used for
> 		      AT channels for a specific modem. This method
> 		      must use the same object path as used in StateChange.
> 
> 
> Signals  StateChange(string status)
> 
>                        The modems state sent from Modem Init Daemon when
>                        a modem state change occurs. The object path must
> 		       be the identifier of the modem (typically
>                        the serial-id or IMEI of the HW).
> 
>                        "booting"   Modem is powered up (flashed version)
>                                    or Modem is powered up and firmware upload
>                                    is completed. (flashless version)
>                        "upgrading" Firmware upgrade on going
>                                    or Flashing manager under execution -
>                                    modem in service mode.
>                        "on"     Modem has booted and is ready for use.
>                                    This implies that all AT channels are
>                                    available, the modem might be in
>                                    e.g. flight mode.
>                        "dumping"  Modem has crashed and dump is ongoing
>                        "off"       Modem is powered off.

is this the full API or only part of it. If it is just a part of it,
please send the full API for the daemon. Just looking at this piece, I
am not really thinking that this is a good API. It is actually pretty
much broken :(

Regards

Marcel

Re: [PATCHv2] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.

> is this the full API or only part of it. If it is just a part of it,
> please send the full API for the daemon. Just looking at this piece, I
> am not really thinking that this is a good API. It is actually pretty
> much broken :(

Ok, here's an updated version of the Modem Init Daemon DBus API.
I hope this is aligned with what we discussed on the IRC.

STE Modem Init Deamon Manager
================

Service	com.stericsson.modeminit
Interface	com.stericsson.modeminit.Manager
Object path	/

Methods	array{object,dict} GetModems()

			Get array of STE Modem objects and their state and
			properties (out signature 'a(oa{sv})').

			The method should only be call once per application.
			Further changes shall be monitored via StateChange
			signals.

STE Modem
================
Service	com.stericsson.modeminit
Interface	com.stericsson.modeminit.Modem
Object path	variable

Signals	StateChange(string State)

			The modems state sent from when
			a modem state change occurs. State is the only
			dynamic property in this Interface.

Properties	string State [readonly]

			The modems state is dynamic can can have the following
			values:
			"booting"   Modem is powered up (flashed version)
				    or Modem is powered up and firmware upload
				    is completed. (flashless version)
			"upgrading" Firmware upgrade on going
				    or Flashing manager under execution -
				    modem in service mode.
			"on"     Modem has booted and is ready for use.
				    This implies that all AT channels are
				    available, the modem might be in
				    e.g. flight mode.
			"dumping"  Modem has crashed and dump is ongoing
			"off"       Modem is powered off.

		string CaifAtInterface [readonly]

			CAIF Link Layer interface to be used for
			AT channels for a modem.

Regards,
Sjur

Re: [PATCHv2] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> > is this the full API or only part of it. If it is just a part of it,
> > please send the full API for the daemon. Just looking at this piece, I
> > am not really thinking that this is a good API. It is actually pretty
> > much broken :(
> 
> Ok, here's an updated version of the Modem Init Daemon DBus API.
> I hope this is aligned with what we discussed on the IRC.
> 
> STE Modem Init Deamon Manager
> ================
> 
> Service	com.stericsson.modeminit
> Interface	com.stericsson.modeminit.Manager
> Object path	/
> 
> Methods	array{object,dict} GetModems()
> 
> 			Get array of STE Modem objects and their state and
> 			properties (out signature 'a(oa{sv})').
> 
> 			The method should only be call once per application.
> 			Further changes shall be monitored via StateChange
> 			signals.

You also need the following signals:

	ModemAdded(object, dict)

	ModemRemoved(object)

> STE Modem
> ================
> Service	com.stericsson.modeminit
> Interface	com.stericsson.modeminit.Modem
> Object path	variable
> 
> Signals	StateChange(string State)
> 
> 			The modems state sent from when
> 			a modem state change occurs. State is the only
> 			dynamic property in this Interface.

I would personally just go straight for PropertyChanged signal here and
not bother with StateChanged. It is actually "...Changed" since at that
time the state has already changed ;)

> Properties	string State [readonly]
> 
> 			The modems state is dynamic can can have the following
> 			values:
> 			"booting"   Modem is powered up (flashed version)
> 				    or Modem is powered up and firmware upload
> 				    is completed. (flashless version)
> 			"upgrading" Firmware upgrade on going
> 				    or Flashing manager under execution -
> 				    modem in service mode.
> 			"on"     Modem has booted and is ready for use.
> 				    This implies that all AT channels are
> 				    available, the modem might be in
> 				    e.g. flight mode.
> 			"dumping"  Modem has crashed and dump is ongoing
> 			"off"       Modem is powered off.

I personally would call it "ready" instead of "on" since the modem is
actually on, it is just not ready yet.

> 		string CaifAtInterface [readonly]
> 
> 			CAIF Link Layer interface to be used for
> 			AT channels for a modem.

I would really just call this "Interface" to make it simpler. Don't
think that you are expecting more than just CAIF interface here anyway.

And in addition if we can have the modem serial number here as "Serial"
as well would be good. Even it is is not right away available, you can
signal a change via PropertyChanged signal.

That way we can construct a proper modem object path inside oFono. I
really rather use the serial number and only fallback to the interface
name.

Regards

Marcel

Re: [PATCHv2] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel,

>> Signals       StateChange(string State)
>>
>>                       The modems state sent from when
>>                       a modem state change occurs. State is the only
>>                       dynamic property in this Interface.
>
>I would personally just go straight for PropertyChanged signal here and
>not bother with StateChanged. It is actually "...Changed" since at that
>time the state has already changed ;)

OK, I'll look into this. I thought StateChange was the only dynamic parameter,
but I might have been wrong here (see below).

> You also need the following signals:
>
>        ModemAdded(object, dict)
>
>        ModemRemoved(object)

I think I'd rather add this when I see a use case for it. The Modem
Init Deamon would need to
know what GPIOs are associated with what modems, and what CAIF
interfaces to use etc.
This information is not dynamic, at least not at the moment. So
ModemAdded and ModemRemoved
will not happen in the current implementation, all the modems will be
known when the Manager
interface becomes available.

...
>>               string CaifAtInterface [readonly]
>>
>>                       CAIF Link Layer interface to be used for
>>                       AT channels for a modem.
>
> I would really just call this "Interface" to make it simpler. Don't
> think that you are expecting more than just CAIF interface here anyway.

OK, Fair enough.

> And in addition if we can have the modem serial number here as "Serial"
> as well would be good. Even it is is not right away available, you can
> signal a change via PropertyChanged signal.
>
> That way we can construct a proper modem object path inside oFono. I
> really rather use the serial number and only fallback to the interface
> name.
>

OK, a Serial property is doable, but I think this is only available
after state "on" (ready)
has been reached. The drawback is that my assumption of State being the only
dynamic property wrong.
Crap you were right - I might need to add a PropertyChanged signal.

Regards,
Sjur

Re: [PATCHv2] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> >> Signals       StateChange(string State)
> >>
> >>                       The modems state sent from when
> >>                       a modem state change occurs. State is the only
> >>                       dynamic property in this Interface.
> >
> >I would personally just go straight for PropertyChanged signal here and
> >not bother with StateChanged. It is actually "...Changed" since at that
> >time the state has already changed ;)
> 
> OK, I'll look into this. I thought StateChange was the only dynamic parameter,
> but I might have been wrong here (see below).
> 
> > You also need the following signals:
> >
> >        ModemAdded(object, dict)
> >
> >        ModemRemoved(object)
> 
> I think I'd rather add this when I see a use case for it. The Modem
> Init Deamon would need to
> know what GPIOs are associated with what modems, and what CAIF
> interfaces to use etc.
> This information is not dynamic, at least not at the moment. So
> ModemAdded and ModemRemoved
> will not happen in the current implementation, all the modems will be
> known when the Manager
> interface becomes available.

what about potential USB based CAIF devices?

> ...
> >>               string CaifAtInterface [readonly]
> >>
> >>                       CAIF Link Layer interface to be used for
> >>                       AT channels for a modem.
> >
> > I would really just call this "Interface" to make it simpler. Don't
> > think that you are expecting more than just CAIF interface here anyway.
> 
> OK, Fair enough.
> 
> > And in addition if we can have the modem serial number here as "Serial"
> > as well would be good. Even it is is not right away available, you can
> > signal a change via PropertyChanged signal.
> >
> > That way we can construct a proper modem object path inside oFono. I
> > really rather use the serial number and only fallback to the interface
> > name.
> >
> 
> OK, a Serial property is doable, but I think this is only available
> after state "on" (ready)

That is fine. We have the same case in oFono that the SubscriberIdentity
only becomes available a bit later. That is in the end easy to handle.

I would just ask to send the property changed signal for the serial
number before sending the signal for on/ready.

> has been reached. The drawback is that my assumption of State being the only
> dynamic property wrong.
> Crap you were right - I might need to add a PropertyChanged signal.

This way you are a lot more flexible in the future.

Regards

Marcel

Re: [PATCHv2] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel,

> what about potential USB based CAIF devices?
Good question, but in a Bridge Setup for a phone this will not really
be devices that come and go.
I belive it will still be a fixed HW setup where the Modem Init Daemon
will have static knowledge of the
USB modems. But I'll discuss this with my coleagues tomorrow.
In case of true USB dongles the modem should look pretty much
identical to MBM USB dongles.

>I would just ask to send the property changed signal for the serial
>number before sending the signal for on/ready.
It will require a re-design of the state machine in the Modem Init Daemon,
but I'll check into this tomorrow.

Here is the latest version of the Dbus API:

STE Modem Init Deamon Manager
=============================
Service		com.stericsson.modeminit
Interface	com.stericsson.modeminit.ModemManager
Object path	/

Methods		array{object,dict} GetModems()

			Get array of STE Modem objects and their state and
			properties (out signature 'a(oa{sv})').

			The method should only be call once per application.
			Further changes shall be monitored via StateChange
			signals.

STE Modem
=========
Service		com.stericsson.modeminit
Interface	com.stericsson.modeminit.Modem
Object path	variable

Signals		PropertyChanged(string property, variant value)

			This signal indicates a changed value of the given
			property.

Properties	string State [readonly]

			The modems state is dynamic can can have the following
			values:
			"booting"   Modem is powered up (flashed version)
				    or Modem is powered up and firmware upload
				    is completed. (flashless version)
			"upgrading" Firmware upgrade on going
				    or Flashing manager under execution -
				    modem in service mode.
			"on"	    Modem has booted and is ready for use.
				    This implies that all AT channels are
				    available, the modem might be in
				    e.g. flight mode.
				    NOTE: Consider change name to "ready"

			"dumping"  Modem has crashed and dump is ongoing
			"off"	    Modem is powered off.

		string AtInterface[readonly]

			CAIF Link Layer interface to be used for
			AT channels for a modem.

		string Serial[optional,readonly]

			Serial Number or IMEI for the Modem. The Serial will
			not be available until the modem can open an AT channel.


Regards,
Sjur

Re: [PATCHv2] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> > what about potential USB based CAIF devices?
> Good question, but in a Bridge Setup for a phone this will not really
> be devices that come and go.
> I belive it will still be a fixed HW setup where the Modem Init Daemon
> will have static knowledge of the
> USB modems. But I'll discuss this with my coleagues tomorrow.
> In case of true USB dongles the modem should look pretty much
> identical to MBM USB dongles.

I personally would prefer that USB devices just expose CAIF directly.
The whole handling of multiple hardcoded TTY and network interface is
only sub-optimal.

I know it is kinda nice to use standard USB class drivers, but the
flexibility that CAIF actually gives you goes away.

From my point of view, it should be similar to Phonet/ISI over USB. At
least on Linux we have a CAIF subsystem ;)

> >I would just ask to send the property changed signal for the serial
> >number before sending the signal for on/ready.
> It will require a re-design of the state machine in the Modem Init Daemon,
> but I'll check into this tomorrow.
> 
> Here is the latest version of the Dbus API:
> 
> STE Modem Init Deamon Manager
> =============================
> Service		com.stericsson.modeminit
> Interface	com.stericsson.modeminit.ModemManager

Just call it com.stericsson.modeminit.Manager. That gives you the chance
to also include other methods/signals later on if needed.

> Object path	/
> 
> Methods		array{object,dict} GetModems()
> 
> 			Get array of STE Modem objects and their state and
> 			properties (out signature 'a(oa{sv})').
> 
> 			The method should only be call once per application.
> 			Further changes shall be monitored via StateChange
> 			signals.
> 
> STE Modem
> =========
> Service		com.stericsson.modeminit
> Interface	com.stericsson.modeminit.Modem
> Object path	variable
> 
> Signals		PropertyChanged(string property, variant value)
> 
> 			This signal indicates a changed value of the given
> 			property.
> 
> Properties	string State [readonly]
> 
> 			The modems state is dynamic can can have the following
> 			values:
> 			"booting"   Modem is powered up (flashed version)
> 				    or Modem is powered up and firmware upload
> 				    is completed. (flashless version)
> 			"upgrading" Firmware upgrade on going
> 				    or Flashing manager under execution -
> 				    modem in service mode.
> 			"on"	    Modem has booted and is ready for use.
> 				    This implies that all AT channels are
> 				    available, the modem might be in
> 				    e.g. flight mode.
> 				    NOTE: Consider change name to "ready"
> 
> 			"dumping"  Modem has crashed and dump is ongoing
> 			"off"	    Modem is powered off.
> 
> 		string AtInterface[readonly]
> 
> 			CAIF Link Layer interface to be used for
> 			AT channels for a modem.

Why is there an "At" in here? I know that mainly only AT command
channels will be used, but the CAIF interface can be also used to
request debug channels and other things. So I would just call it
Interface only to say what's the interface name is to bind to.

> 		string Serial[optional,readonly]
> 
> 			Serial Number or IMEI for the Modem. The Serial will
> 			not be available until the modem can open an AT channel.

Otherwise, this looks a lot simpler and cleaner.

Regards

Marcel

[PATCHv4 0/1] STE Modem Init Daemon integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This is the latest version of the Modem Init Daemon API.

STE Modem Init Deamon Manager
=============================

Service		com.stericsson.modeminit
Interface	com.stericsson.modeminit.Manager
Object path	/

Methods		array{object,dict} GetModems()

			Get array of STE Modem objects and their state and
			properties (out signature 'a(oa{sv})').

			The method should only be call once per application.
			Further changes shall be monitored via StateChange
			signals.

STE Modem
=========
Service		com.stericsson.modeminit
Interface	com.stericsson.modeminit.Modem
Object path	variable

Signals		PropertyChanged(string property, variant value)

			This signal indicates a changed value of the given
			property.

Properties	string State [readonly]

			The modems state is dynamic can can have the following
			values:
			"booting"   Modem is powered up (flashed version)
				    or Modem is powered up and firmware upload
				    is completed. (flashless version)
			"upgrading" Firmware upgrade on going
				    or Flashing manager under execution -
				    modem in service mode.
			"ready"	    Modem has booted and is ready for use.
				    This implies that all AT channels are
				    available, the modem might be in
				    e.g. flight mode.

			"dumping"  Modem has crashed and dump is ongoing
			"off"	    Modem is powered off.

		string Interface[readonly]

			CAIF Link Layer interface to be used for
			CAIF channels for a modem.

		string Serial[optional,readonly]

			Serial Number or IMEI for the Modem. The Serial will
			not be available until the modem can open an AT channel.


Sjur Brændeland (1):
  plugin: Add ste modem initd integration

 Makefile.am      |    4 +
 plugins/stemgr.c |  345 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 349 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

[PATCHv4 1/1] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch introduces auto discovery of ST-Ericsson modems.
ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed
from the Modem Init Damon's Dbus API. When the modem is in state
"on" the STE modem is created and registered. Muliple modem
instances, and reset handling is supported.
---
Changes:
- Adapted to new Dbus API, using GetModems method and PropertyChange signal
- Using Serial as modem id
- Review comments from last v3

 Makefile.am      |    4 +
 plugins/stemgr.c |  345 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 349 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

diff --git a/Makefile.am b/Makefile.am
index 8a8555d..6fc4c62 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -223,6 +223,10 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/ifxmodem/gprs-context.c \
 			drivers/ifxmodem/stk.c
 
+
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += stemodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/stemodem/stemodem.h \
diff --git a/plugins/stemgr.c b/plugins/stemgr.c
new file mode 100644
index 0000000..467d0f8
--- /dev/null
+++ b/plugins/stemgr.c
@@ -0,0 +1,345 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2011 ST-Ericsson AB.
+ *
+ *  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 <errno.h>
+#include <string.h>
+#include <net/if.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+
+/*
+ * ST-Ericsson's Modem Init Daemon is used for controling the modem power
+ * cycles and provides a dbus API for modem state and properties.
+ */
+#define MGR_SERVICE		"com.stericsson.modeminit"
+#define MGR_MGR_INTERFACE	MGR_SERVICE ".Manager"
+#define MGR_GET_MODEMS		"GetModems"
+#define GET_MODEMS_TIMEOUT	5000
+
+#define MGR_MODEM_INTERFACE	MGR_SERVICE ".Modem"
+#define PROPERTY_CHANGED	"PropertyChanged"
+
+enum ste_state {
+	STE_PENDING,
+	STE_REGISTERED,
+	STE_RESETTING
+};
+
+struct ste_modem {
+	struct ofono_modem *modem;
+	char *path;
+	enum ste_state state;
+	char *serial;
+	char *interface;
+};
+
+static GHashTable *modem_list;
+static guint mgr_api_watch;
+static guint mgr_state_watch;
+static DBusConnection *connection;
+static gboolean get_modems_call_pending;
+
+static inline gboolean is_ready(const char *action)
+{
+	return g_strcmp0(action, "ready") == 0;
+}
+
+static inline gboolean is_reset(const char *action)
+{
+	return g_strcmp0(action, "dumping") == 0;
+}
+
+static void state_change(struct ste_modem *stemodem, const char *action)
+{
+
+	switch (stemodem->state) {
+	case STE_PENDING:
+
+		if (!is_ready(action))
+			break;
+
+		stemodem->modem = ofono_modem_create(stemodem->serial,
+							"ste");
+		if (stemodem->modem == NULL) {
+			ofono_error("Could not create modem %s, %s",
+					stemodem->path, stemodem->serial);
+			return;
+		}
+
+		DBG("register modem %s, %s", stemodem->path, stemodem->serial);
+		ofono_modem_set_string(stemodem->modem, "Interface",
+							stemodem->interface);
+		ofono_modem_register(stemodem->modem);
+		stemodem->state = STE_REGISTERED;
+		break;
+	case STE_REGISTERED:
+
+		if (is_reset(action)) {
+			DBG("reset ongoing %s", stemodem->path);
+			/* Note: Consider to power off modem here? */
+			stemodem->state = STE_RESETTING;
+		} else if (!is_ready(action)) {
+			DBG("STE modem unregistering %s %s", stemodem->path,
+				action);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_PENDING;
+		}
+
+		break;
+	case STE_RESETTING:
+
+		if (is_ready(action)) {
+			DBG("STE modem reset complete %s", stemodem->path);
+			if (ofono_modem_get_powered(stemodem->modem))
+				ofono_modem_reset(stemodem->modem);
+			stemodem->state = STE_REGISTERED;
+		} else if (!is_reset(action)) {
+			DBG("STE modem unregistering %s %s", stemodem->path,
+				action);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_PENDING;
+		}
+
+		break;
+	}
+}
+
+static void update_property(struct ste_modem *stemodem, const char *prop,
+				DBusMessageIter *iter, const char **state)
+{
+	const char *value;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
+		return;
+
+	dbus_message_iter_get_basic(iter, &value);
+
+	if (g_strcmp0(prop, "State") == 0)
+		*state = value;
+	else if (g_strcmp0(prop, "Interface") == 0) {
+		g_free(stemodem->interface);
+		stemodem->interface = g_strdup(value);
+	} else if (g_strcmp0(prop, "Serial") == 0) {
+		g_free(stemodem->serial);
+		stemodem->serial = g_strdup(value);
+	}
+}
+
+static void update_modem_properties(const char *path, DBusMessageIter *iter)
+{
+	const char *state = NULL;
+	struct ste_modem *stemodem = g_hash_table_lookup(modem_list, path);
+
+	if (stemodem == NULL) {
+		stemodem = g_try_new0(struct ste_modem, 1);
+		if (stemodem == NULL)
+			return;
+
+		stemodem->path = g_strdup(path);
+		stemodem->state = STE_PENDING;
+		g_hash_table_insert(modem_list, stemodem->path, stemodem);
+	}
+
+	while (dbus_message_iter_get_arg_type(iter) == DBUS_TYPE_DICT_ENTRY) {
+		DBusMessageIter entry, value;
+		const char *key, *str;
+
+		dbus_message_iter_recurse(iter, &entry);
+		dbus_message_iter_get_basic(&entry, &key);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &value);
+
+		update_property(stemodem, key, &value, &state);
+
+		dbus_message_iter_next(iter);
+	}
+
+	if (state != NULL)
+		state_change(stemodem, state);
+}
+
+static void get_modems_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessageIter iter, list;
+	DBusError err;
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+
+	get_modems_call_pending = FALSE;
+	dbus_error_init(&err);
+
+	if (dbus_set_error_from_message(&err, reply)) {
+		ofono_error("%s: %s\n", err.name, err.message);
+		dbus_error_free(&err);
+		goto done;
+	}
+
+	if (!dbus_message_has_signature(reply, "a(oa{sv})"))
+		goto done;
+
+	if (!dbus_message_iter_init(reply, &iter))
+		goto done;
+
+	dbus_message_iter_recurse(&iter, &list);
+
+	while (dbus_message_iter_get_arg_type(&list) == DBUS_TYPE_STRUCT) {
+		DBusMessageIter entry, dict;
+		const char *path;
+
+		dbus_message_iter_recurse(&list, &entry);
+		dbus_message_iter_get_basic(&entry, &path);
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &dict);
+
+		update_modem_properties(path, &dict);
+
+		dbus_message_iter_next(&list);
+	}
+
+done:
+	dbus_message_unref(reply);
+}
+
+static void get_modems(const char *path)
+{
+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	if (get_modems_call_pending)
+		return;
+
+	message = dbus_message_new_method_call(MGR_SERVICE, path,
+					MGR_MGR_INTERFACE, MGR_GET_MODEMS);
+	if (message == NULL) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	dbus_message_set_auto_start(message, FALSE);
+
+	if (!dbus_connection_send_with_reply(connection, message, &call,
+						GET_MODEMS_TIMEOUT)) {
+		ofono_error("Sending D-Bus message failed");
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		goto error;
+	}
+
+	get_modems_call_pending = TRUE;
+	dbus_pending_call_set_notify(call, get_modems_reply, NULL, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static gboolean property_changed(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	DBusMessageIter iter, value;
+	struct ste_modem *stemodem;
+	const char *key, *str;
+	const char *state = NULL;
+
+	stemodem = g_hash_table_lookup(modem_list,
+					dbus_message_get_path(message));
+
+	if (stemodem == NULL) {
+		get_modems("/");
+		return TRUE;
+	}
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+	dbus_message_iter_next(&iter);
+
+	update_property(stemodem, key, &iter, &state);
+
+	if (state != NULL)
+		state_change(stemodem, state);
+
+	return TRUE;
+}
+
+static void mgr_connect(DBusConnection *connection, void *user_data)
+{
+	mgr_state_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
+						MGR_MODEM_INTERFACE,
+						PROPERTY_CHANGED,
+						property_changed,
+						NULL, NULL);
+	get_modems("/");
+}
+
+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+	g_hash_table_remove_all(modem_list);
+	g_dbus_remove_watch(connection, mgr_state_watch);
+}
+
+static void destroy_stemodem(gpointer data)
+{
+	struct ste_modem *stemodem = data;
+
+	ofono_modem_remove(stemodem->modem);
+	g_free(stemodem->interface);
+	g_free(stemodem->path);
+	g_free(stemodem->serial);
+	g_free(stemodem);
+}
+
+static int stemgr_init()
+{
+	connection = ofono_dbus_get_connection();
+
+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);
+	mgr_api_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
+				mgr_connect, mgr_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemgr_exit()
+{
+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, mgr_api_watch);
+}
+
+OFONO_PLUGIN_DEFINE(stemgr, "ST-Ericsson Modem Init Daemon detection", VERSION,
+			OFONO_PLUGIN_PRIORITY_DEFAULT, stemgr_init, stemgr_exit)
-- 
1.7.0.4

Re: [PATCHv4 1/1] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> This patch introduces auto discovery of ST-Ericsson modems.
> ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
> Modem Init Daemon responsible for start, power cycles,
> flashing etc. This STE plugin monitors the modem state exposed
> from the Modem Init Damon's Dbus API. When the modem is in state
> "on" the STE modem is created and registered. Muliple modem
> instances, and reset handling is supported.
> ---
> Changes:
> - Adapted to new Dbus API, using GetModems method and PropertyChange signal
> - Using Serial as modem id
> - Review comments from last v3
> 
>  Makefile.am      |    4 +
>  plugins/stemgr.c |  345 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 349 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/stemgr.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 8a8555d..6fc4c62 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -223,6 +223,10 @@ builtin_sources += drivers/atmodem/atutil.h \
>  			drivers/ifxmodem/gprs-context.c \
>  			drivers/ifxmodem/stk.c
>  
> +
> +builtin_modules += stemgr
> +builtin_sources += plugins/stemgr.c
> +

This is a minor nitpick, but I think this is better placed between ste
and caif plugin details. And not on top of stemodem.

>  builtin_modules += stemodem
>  builtin_sources += drivers/atmodem/atutil.h \
>  			drivers/stemodem/stemodem.h \
> diff --git a/plugins/stemgr.c b/plugins/stemgr.c
> new file mode 100644
> index 0000000..467d0f8
> --- /dev/null
> +++ b/plugins/stemgr.c
> @@ -0,0 +1,345 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2011 ST-Ericsson AB.
> + *
> + *  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 <errno.h>
> +#include <string.h>
> +#include <net/if.h>
> +
> +#include <glib.h>
> +#include <gdbus.h>
> +
> +#define OFONO_API_SUBJECT_TO_CHANGE
> +#include <ofono/plugin.h>
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/dbus.h>
> +
> +/*
> + * ST-Ericsson's Modem Init Daemon is used for controling the modem power
> + * cycles and provides a dbus API for modem state and properties.
> + */
> +#define MGR_SERVICE		"com.stericsson.modeminit"
> +#define MGR_MGR_INTERFACE	MGR_SERVICE ".Manager"

Really? MGR_MGR and not MGR_MANAGER ;)

> +#define MGR_GET_MODEMS		"GetModems"
> +#define GET_MODEMS_TIMEOUT	5000
> +
> +#define MGR_MODEM_INTERFACE	MGR_SERVICE ".Modem"
> +#define PROPERTY_CHANGED	"PropertyChanged"
> +
> +enum ste_state {
> +	STE_PENDING,
> +	STE_REGISTERED,
> +	STE_RESETTING
> +};
> +
> +struct ste_modem {
> +	struct ofono_modem *modem;
> +	char *path;

You don't have to necessarily do it this way, but since you are using
the path memory also as index for your hash table it might be good to
put it first in your struct.

> +	enum ste_state state;
> +	char *serial;
> +	char *interface;
> +};
> +
> +static GHashTable *modem_list;
> +static guint mgr_api_watch;

It is more like a service watch or daemon watch.

> +static guint mgr_state_watch;

And this is property changed watch, right?

> +static DBusConnection *connection;
> +static gboolean get_modems_call_pending;

What is this exactly for? How many do you expect to have pending?

> +static inline gboolean is_ready(const char *action)
> +{
> +	return g_strcmp0(action, "ready") == 0;
> +}
> +
> +static inline gboolean is_reset(const char *action)
> +{
> +	return g_strcmp0(action, "dumping") == 0;
> +}

This is nasty. Why not convert the state into an enum once and be done
with it.

> +static void state_change(struct ste_modem *stemodem, const char *action)
> +{
> +

Scrap this empty line ;)

> +	switch (stemodem->state) {
> +	case STE_PENDING:
> +
> +		if (!is_ready(action))
> +			break;
> +
> +		stemodem->modem = ofono_modem_create(stemodem->serial,
> +							"ste");
> +		if (stemodem->modem == NULL) {
> +			ofono_error("Could not create modem %s, %s",
> +					stemodem->path, stemodem->serial);
> +			return;
> +		}
> +
> +		DBG("register modem %s, %s", stemodem->path, stemodem->serial);
> +		ofono_modem_set_string(stemodem->modem, "Interface",
> +							stemodem->interface);
> +		ofono_modem_register(stemodem->modem);
> +		stemodem->state = STE_REGISTERED;
> +		break;
> +	case STE_REGISTERED:
> +
> +		if (is_reset(action)) {
> +			DBG("reset ongoing %s", stemodem->path);
> +			/* Note: Consider to power off modem here? */
> +			stemodem->state = STE_RESETTING;
> +		} else if (!is_ready(action)) {
> +			DBG("STE modem unregistering %s %s", stemodem->path,
> +				action);
> +			ofono_modem_remove(stemodem->modem);
> +			stemodem->modem = NULL;
> +			stemodem->state = STE_PENDING;
> +		}
> +
> +		break;
> +	case STE_RESETTING:
> +
> +		if (is_ready(action)) {
> +			DBG("STE modem reset complete %s", stemodem->path);
> +			if (ofono_modem_get_powered(stemodem->modem))
> +				ofono_modem_reset(stemodem->modem);
> +			stemodem->state = STE_REGISTERED;
> +		} else if (!is_reset(action)) {
> +			DBG("STE modem unregistering %s %s", stemodem->path,
> +				action);
> +			ofono_modem_remove(stemodem->modem);
> +			stemodem->modem = NULL;
> +			stemodem->state = STE_PENDING;
> +		}
> +
> +		break;
> +	}
> +}

I find this all a bit complicated. Can you quickly describe the logic
here in plain words.

> +static void update_property(struct ste_modem *stemodem, const char *prop,
> +				DBusMessageIter *iter, const char **state)
> +{
> +	const char *value;
> +
> +	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
> +		return;
> +
> +	dbus_message_iter_get_basic(iter, &value);
> +
> +	if (g_strcmp0(prop, "State") == 0)
> +		*state = value;
> +	else if (g_strcmp0(prop, "Interface") == 0) {
> +		g_free(stemodem->interface);
> +		stemodem->interface = g_strdup(value);
> +	} else if (g_strcmp0(prop, "Serial") == 0) {
> +		g_free(stemodem->serial);
> +		stemodem->serial = g_strdup(value);
> +	}
> +}
> +
> +static void update_modem_properties(const char *path, DBusMessageIter *iter)
> +{
> +	const char *state = NULL;
> +	struct ste_modem *stemodem = g_hash_table_lookup(modem_list, path);
> +
> +	if (stemodem == NULL) {
> +		stemodem = g_try_new0(struct ste_modem, 1);
> +		if (stemodem == NULL)
> +			return;
> +
> +		stemodem->path = g_strdup(path);
> +		stemodem->state = STE_PENDING;
> +		g_hash_table_insert(modem_list, stemodem->path, stemodem);
> +	}
> +
> +	while (dbus_message_iter_get_arg_type(iter) == DBUS_TYPE_DICT_ENTRY) {
> +		DBusMessageIter entry, value;
> +		const char *key, *str;
> +
> +		dbus_message_iter_recurse(iter, &entry);
> +		dbus_message_iter_get_basic(&entry, &key);
> +
> +		dbus_message_iter_next(&entry);
> +		dbus_message_iter_recurse(&entry, &value);
> +
> +		update_property(stemodem, key, &value, &state);
> +
> +		dbus_message_iter_next(iter);
> +	}
> +
> +	if (state != NULL)
> +		state_change(stemodem, state);
> +}
> +
> +static void get_modems_reply(DBusPendingCall *call, void *user_data)
> +{
> +	DBusMessageIter iter, list;
> +	DBusError err;
> +	DBusMessage *reply = dbus_pending_call_steal_reply(call);
> +
> +	get_modems_call_pending = FALSE;
> +	dbus_error_init(&err);
> +
> +	if (dbus_set_error_from_message(&err, reply)) {
> +		ofono_error("%s: %s\n", err.name, err.message);
> +		dbus_error_free(&err);
> +		goto done;
> +	}
> +
> +	if (!dbus_message_has_signature(reply, "a(oa{sv})"))
> +		goto done;
> +
> +	if (!dbus_message_iter_init(reply, &iter))
> +		goto done;
> +
> +	dbus_message_iter_recurse(&iter, &list);
> +
> +	while (dbus_message_iter_get_arg_type(&list) == DBUS_TYPE_STRUCT) {
> +		DBusMessageIter entry, dict;
> +		const char *path;
> +
> +		dbus_message_iter_recurse(&list, &entry);
> +		dbus_message_iter_get_basic(&entry, &path);
> +		dbus_message_iter_next(&entry);
> +		dbus_message_iter_recurse(&entry, &dict);
> +
> +		update_modem_properties(path, &dict);
> +
> +		dbus_message_iter_next(&list);
> +	}
> +
> +done:
> +	dbus_message_unref(reply);
> +}
> +
> +static void get_modems(const char *path)
> +{

This path parameter is rather pointless. It is always "/".

> +	DBusMessage *message;
> +	DBusPendingCall *call;
> +
> +	if (get_modems_call_pending)
> +		return;

I still haven't figured out what you are trying to protect here.

> +	message = dbus_message_new_method_call(MGR_SERVICE, path,
> +					MGR_MGR_INTERFACE, MGR_GET_MODEMS);
> +	if (message == NULL) {
> +		ofono_error("Unable to allocate new D-Bus message");
> +		goto error;
> +	}
> +
> +	dbus_message_set_auto_start(message, FALSE);
> +
> +	if (!dbus_connection_send_with_reply(connection, message, &call,
> +						GET_MODEMS_TIMEOUT)) {
> +		ofono_error("Sending D-Bus message failed");
> +		goto error;
> +	}
> +
> +	if (call == NULL) {
> +		DBG("D-Bus connection not available");
> +		goto error;
> +	}
> +
> +	get_modems_call_pending = TRUE;
> +	dbus_pending_call_set_notify(call, get_modems_reply, NULL, NULL);
> +	dbus_pending_call_unref(call);
> +
> +error:
> +	dbus_message_unref(message);
> +}
> +
> +static gboolean property_changed(DBusConnection *connection,
> +					DBusMessage *message, void *user_data)
> +{
> +	DBusMessageIter iter, value;
> +	struct ste_modem *stemodem;
> +	const char *key, *str;
> +	const char *state = NULL;
> +
> +	stemodem = g_hash_table_lookup(modem_list,
> +					dbus_message_get_path(message));
> +
> +	if (stemodem == NULL) {
> +		get_modems("/");
> +		return TRUE;
> +	}

So here we go. So you are expecting a property changed signal before
GetModems finished and therefor you just call it again. This is rather
pointless since you get the properties with the return of the GetModems
call and these should be the actual ones.

So if the modem path is not in your hash-table, then just ignore the
property changed signal.

> +	if (!dbus_message_iter_init(message, &iter))
> +		return TRUE;
> +
> +	dbus_message_iter_get_basic(&iter, &key);
> +	dbus_message_iter_next(&iter);
> +
> +	update_property(stemodem, key, &iter, &state);
> +
> +	if (state != NULL)
> +		state_change(stemodem, state);
> +
> +	return TRUE;
> +}
> +
> +static void mgr_connect(DBusConnection *connection, void *user_data)
> +{
> +	mgr_state_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
> +						MGR_MODEM_INTERFACE,
> +						PROPERTY_CHANGED,
> +						property_changed,
> +						NULL, NULL);
> +	get_modems("/");
> +}
> +
> +static void mgr_disconnect(DBusConnection *connection, void *user_data)
> +{
> +	g_hash_table_remove_all(modem_list);
> +	g_dbus_remove_watch(connection, mgr_state_watch);
> +}
> +
> +static void destroy_stemodem(gpointer data)
> +{
> +	struct ste_modem *stemodem = data;
> +
> +	ofono_modem_remove(stemodem->modem);

As a small minor style nitpick, I would add an empty line here.

> +	g_free(stemodem->interface);
> +	g_free(stemodem->path);
> +	g_free(stemodem->serial);
> +	g_free(stemodem);
> +}
> +
> +static int stemgr_init()
> +{
> +	connection = ofono_dbus_get_connection();
> +
> +	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
> +						NULL, destroy_stemodem);
> +	mgr_api_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
> +				mgr_connect, mgr_disconnect, NULL, NULL);
> +	return 0;
> +}
> +
> +static void stemgr_exit()
> +{

The only thing you missed here is to remove the property changed watch
in case it was registered (aka the init daemon started).

> +	g_hash_table_destroy(modem_list);
> +	g_dbus_remove_watch(connection, mgr_api_watch);
> +}
> +
> +OFONO_PLUGIN_DEFINE(stemgr, "ST-Ericsson Modem Init Daemon detection", VERSION,
> +			OFONO_PLUGIN_PRIORITY_DEFAULT, stemgr_init, stemgr_exit)

Regards

Marcel

[PATCHv5] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch introduces auto discovery of ST-Ericsson modems.
ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed
from the Modem Init Damon's Dbus API. When the modem is in state
"on" the STE modem is created and registered. Muliple modem
instances, and reset handling is supported.
---
Hi Marcel.

>> +
>> +builtin_modules += stemgr
>> +builtin_sources += plugins/stemgr.c
>> +
>
> This is a minor nitpick, but I think this is better placed between ste
> and caif plugin details. And not on top of stemodem.
Sure, I've moved this entry.

>> +#define MGR_SERVICE          "com.stericsson.modeminit"
>> +#define MGR_MGR_INTERFACE    MGR_SERVICE ".Manager"
>
> Really? MGR_MGR and not MGR_MANAGER ;)
Oops, s/mid/mgr didn't do it right this time ;-)
Let's call it MGR_INTERFACE instead.

>> +struct ste_modem {
>> +     struct ofono_modem *modem;
>> +     char *path;
>
> You don't have to necessarily do it this way, but since you are using
> the path memory also as index for your hash table it might be good to
> put it first in your struct.

Ok, I've put path first in struct.
>> +static guint mgr_api_watch;
>
> It is more like a service watch or daemon watch.
Yes, I renamed this to modem_daemon_watch.
>
>> +static guint mgr_state_watch;
>
> And this is property changed watch, right?
Yes, renamed this to property_changed_watch

> What is this exactly for? How many do you expect to have pending?
I have removed this one. get_modems is *only* called
when the Modem Init Daemon is first connected.

>> +static inline gboolean is_ready(const char *action)
>> +{
>> +     return g_strcmp0(action, "ready") == 0;
>> +}
...
> This is nasty. Why not convert the state into an enum once and be done
> with it.
Nasty? Anyway I've changed this to a ste_operation enum,
and I'll give you that - it does make the state machine a more readable.

>
> I find this all a bit complicated. Can you quickly describe the logic
> here in plain words.
Sure, I've written some prose describing the state machine.
I've also done this function like a standard FSM implementation:

switch(state) {
case STATE_A:
     switch (operation) {
     case OPERATION_1:
...


>> +static void get_modems(const char *path)
>> +{
>
> This path parameter is rather pointless. It is always "/".
Agree.

>> +     if (get_modems_call_pending)
>> +             return;
>
> I still haven't figured out what you are trying to protect here.
....
>> +static gboolean property_changed(DBusConnection *connection,
>> +                                     DBusMessage *message, void *user_data)
>> +{
...
>> +     stemodem = g_hash_table_lookup(modem_list,
>> +                                     dbus_message_get_path(message));
>> +
>> +     if (stemodem == NULL) {
>> +             get_modems("/");
>> +             return TRUE;
>> +     }
>
> So here we go. So you are expecting a property changed signal before
> GetModems finished and therefor you just call it again. This is rather
> pointless since you get the properties with the return of the GetModems
> call and these should be the actual ones.
>
> So if the modem path is not in your hash-table, then just ignore the
> property changed signal.

Yes, agree. This was a left over from previous API. I have removed this 
completely. get_modems() is only called when Modem Init Daemon API 
becomes available. If modems ever pop up dynamically I will add a new
signal for this in the DBus API as suggested.

>> +static void stemgr_exit()
>> +{
>
> The only thing you missed here is to remove the property changed watch
> in case it was registered (aka the init daemon started).
Good catch thanks, I'v removed this watch.

Thank you for reviewing this code, hopefully we're close to completion now ;-)

Regards,
Sjur

 Makefile.am      |    3 +
 plugins/stemgr.c |  385 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 388 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

diff --git a/Makefile.am b/Makefile.am
index 8a8555d..1e4f9df 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -292,6 +292,9 @@ builtin_sources += plugins/ifx.c
 builtin_modules += ste
 builtin_sources += plugins/ste.c
 
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += caif
 builtin_sources += plugins/caif.c
 endif
diff --git a/plugins/stemgr.c b/plugins/stemgr.c
new file mode 100644
index 0000000..0e2745c
--- /dev/null
+++ b/plugins/stemgr.c
@@ -0,0 +1,385 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2011 ST-Ericsson AB.
+ *
+ *  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 <errno.h>
+#include <string.h>
+#include <net/if.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+
+/*
+ * ST-Ericsson's Modem Init Daemon is used for controling the modem power
+ * cycles and provides a dbus API for modem state and properties.
+ */
+#define MGR_SERVICE		"com.stericsson.modeminit"
+#define MGR_INTERFACE		MGR_SERVICE ".Manager"
+#define MGR_GET_MODEMS		"GetModems"
+#define GET_MODEMS_TIMEOUT	5000
+
+#define MGR_MODEM_INTERFACE	MGR_SERVICE ".Modem"
+#define PROPERTY_CHANGED	"PropertyChanged"
+
+enum ste_state {
+	STE_STATE_OFF,
+	STE_STATE_READY,
+	STE_STATE_RESET
+};
+
+enum ste_operation {
+	STE_OP_STARTING,
+	STE_OP_READY,
+	STE_OP_RESTART,
+	STE_OP_OFF
+};
+
+struct ste_modem {
+	char *path;
+	struct ofono_modem *modem;
+	enum ste_state state;
+	char *serial;
+	char *interface;
+};
+
+static GHashTable *modem_list;
+static guint modem_daemon_watch;
+static guint property_changed_watch;
+static DBusConnection *connection;
+
+static void state_change(struct ste_modem *stemodem, enum ste_operation op)
+{
+	switch (stemodem->state) {
+	case STE_STATE_OFF:
+		/*
+		 * The STE Modem is in state OFF and we're waiting for
+		 * the Modem Init Daemon to signal that modem is ready
+		 * in order to create and register the modem.
+		 */
+		switch (op) {
+		case STE_OP_READY:
+			stemodem->modem = ofono_modem_create(stemodem->serial,
+								"ste");
+			if (stemodem->modem == NULL) {
+				ofono_error("Could not create modem %s, %s",
+						stemodem->path,
+						stemodem->serial);
+				return;
+			}
+
+			DBG("register modem %s, %s", stemodem->path,
+				stemodem->serial);
+
+			if (stemodem->interface != NULL)
+				ofono_modem_set_string(stemodem->modem,
+							"Interface",
+							stemodem->interface);
+
+			ofono_modem_register(stemodem->modem);
+			stemodem->state = STE_STATE_READY;
+			break;
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+		case STE_OP_OFF:
+			break;
+		}
+		break;
+	case STE_STATE_READY:
+		/*
+		 * The STE Modem is ready and the modem has been created
+		 * and registered in oFono. In this state two things can
+		 * happen: Modem restarts or is turned off. Turning off
+		 * the modem is an exceptional situation e.g. high-temperature,
+		 * low battery or upgrade. In this scenario we remove the
+		 * STE modem from oFono.
+		 */
+		switch (op) {
+		case STE_OP_READY:
+			break;
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+			DBG("reset ongoing %s", stemodem->path);
+			/* Note: Consider to power off modem here? */
+			stemodem->state = STE_STATE_RESET;
+			break;
+		case STE_OP_OFF:
+			DBG("STE modem unregistering %s", stemodem->path);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_STATE_OFF;
+			break;
+		}
+		break;
+	case STE_STATE_RESET:
+		/*
+		 * The STE Modem is resetting.In this state two things can
+		 * happen: Modem restarts succeeds, or modem is turned off.
+		 */
+		switch (op) {
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+			break;
+		case STE_OP_READY:
+			DBG("STE modem reset complete %s", stemodem->path);
+			if (ofono_modem_get_powered(stemodem->modem))
+				ofono_modem_reset(stemodem->modem);
+			stemodem->state = STE_STATE_READY;
+			break;
+		case STE_OP_OFF:
+			DBG("STE modem unregistering %s", stemodem->path);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_STATE_OFF;
+			break;
+		}
+		break;
+	}
+}
+
+static void update_property(struct ste_modem *stemodem, const char *prop,
+				DBusMessageIter *iter, enum ste_operation *op,
+				gboolean *op_valid)
+{
+	const char *value;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
+		return;
+
+	dbus_message_iter_get_basic(iter, &value);
+
+	if (g_strcmp0(prop, "State") == 0) {
+		*op_valid = TRUE;
+		if (g_strcmp0(value, "booting") == 0)
+			*op = STE_OP_STARTING;
+		else if (g_strcmp0(value, "upgrading") == 0)
+			*op = STE_OP_OFF;
+		else if (g_strcmp0(value, "ready") == 0)
+			*op = STE_OP_READY;
+		else if (g_strcmp0(value, "off") == 0)
+			*op = STE_OP_OFF;
+		else if (g_strcmp0(value, "dumping") == 0)
+			*op = STE_OP_RESTART;
+		else
+			*op_valid = FALSE;
+	} else if (g_strcmp0(prop, "Interface") == 0) {
+		g_free(stemodem->interface);
+		stemodem->interface = g_strdup(value);
+	} else if (g_strcmp0(prop, "Serial") == 0) {
+		g_free(stemodem->serial);
+		stemodem->serial = g_strdup(value);
+	}
+}
+
+static void update_modem_properties(const char *path, DBusMessageIter *iter)
+{
+	enum ste_operation operation;
+	gboolean operation_valid;
+	struct ste_modem *stemodem = g_hash_table_lookup(modem_list, path);
+
+	if (stemodem == NULL) {
+		stemodem = g_try_new0(struct ste_modem, 1);
+		if (stemodem == NULL)
+			return;
+
+		stemodem->path = g_strdup(path);
+		stemodem->state = STE_STATE_OFF;
+		g_hash_table_insert(modem_list, stemodem->path, stemodem);
+	}
+
+	while (dbus_message_iter_get_arg_type(iter) == DBUS_TYPE_DICT_ENTRY) {
+		DBusMessageIter entry, value;
+		const char *key;
+
+		dbus_message_iter_recurse(iter, &entry);
+		dbus_message_iter_get_basic(&entry, &key);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &value);
+
+		update_property(stemodem, key, &value, &operation,
+					&operation_valid);
+
+		dbus_message_iter_next(iter);
+	}
+
+	if (operation_valid)
+		state_change(stemodem, operation);
+}
+
+static void get_modems_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessageIter iter, list;
+	DBusError err;
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&err);
+
+	if (dbus_set_error_from_message(&err, reply)) {
+		ofono_error("%s: %s\n", err.name, err.message);
+		dbus_error_free(&err);
+		goto done;
+	}
+
+	if (!dbus_message_has_signature(reply, "a(oa{sv})"))
+		goto done;
+
+	if (!dbus_message_iter_init(reply, &iter))
+		goto done;
+
+	dbus_message_iter_recurse(&iter, &list);
+
+	while (dbus_message_iter_get_arg_type(&list) == DBUS_TYPE_STRUCT) {
+		DBusMessageIter entry, dict;
+		const char *path;
+
+		dbus_message_iter_recurse(&list, &entry);
+		dbus_message_iter_get_basic(&entry, &path);
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &dict);
+
+		update_modem_properties(path, &dict);
+
+		dbus_message_iter_next(&list);
+	}
+
+done:
+	dbus_message_unref(reply);
+}
+
+static void get_modems()
+{
+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	message = dbus_message_new_method_call(MGR_SERVICE, "/",
+					MGR_INTERFACE, MGR_GET_MODEMS);
+	if (message == NULL) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	dbus_message_set_auto_start(message, FALSE);
+
+	if (!dbus_connection_send_with_reply(connection, message, &call,
+						GET_MODEMS_TIMEOUT)) {
+		ofono_error("Sending D-Bus message failed");
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		goto error;
+	}
+
+	dbus_pending_call_set_notify(call, get_modems_reply, NULL, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static gboolean property_changed(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	DBusMessageIter iter;
+	struct ste_modem *stemodem;
+	const char *key;
+	enum ste_operation operation;
+	gboolean operation_valid;
+
+	stemodem = g_hash_table_lookup(modem_list,
+					dbus_message_get_path(message));
+
+	if (stemodem == NULL)
+		return TRUE;
+
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+	dbus_message_iter_next(&iter);
+
+	update_property(stemodem, key, &iter, &operation, &operation_valid);
+
+	if (operation_valid)
+		state_change(stemodem, operation);
+
+	return TRUE;
+}
+
+static void mgr_connect(DBusConnection *connection, void *user_data)
+{
+	property_changed_watch = g_dbus_add_signal_watch(connection, NULL,
+						NULL,
+						MGR_MODEM_INTERFACE,
+						PROPERTY_CHANGED,
+						property_changed,
+						NULL, NULL);
+	get_modems();
+}
+
+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+	g_hash_table_remove_all(modem_list);
+	g_dbus_remove_watch(connection, property_changed_watch);
+}
+
+static void destroy_stemodem(gpointer data)
+{
+	struct ste_modem *stemodem = data;
+
+	ofono_modem_remove(stemodem->modem);
+	g_free(stemodem->interface);
+	g_free(stemodem->path);
+	g_free(stemodem->serial);
+	g_free(stemodem);
+}
+
+static int stemgr_init()
+{
+	connection = ofono_dbus_get_connection();
+
+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);
+	modem_daemon_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
+				mgr_connect, mgr_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemgr_exit()
+{
+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, modem_daemon_watch);
+	g_dbus_remove_watch(connection, property_changed_watch);
+}
+
+OFONO_PLUGIN_DEFINE(stemgr, "ST-Ericsson Modem Init Daemon detection", VERSION,
+			OFONO_PLUGIN_PRIORITY_DEFAULT, stemgr_init, stemgr_exit)
-- 
1.7.0.4

Re: [PATCHv5] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> +static void get_modems()
> +{

so these are declared as get_modems(void) btw. The compiler should have
warned you about this.

> +	DBusMessage *message;
> +	DBusPendingCall *call;
> +
> +	message = dbus_message_new_method_call(MGR_SERVICE, "/",
> +					MGR_INTERFACE, MGR_GET_MODEMS);
> +	if (message == NULL) {
> +		ofono_error("Unable to allocate new D-Bus message");
> +		goto error;
> +	}
> +
> +	dbus_message_set_auto_start(message, FALSE);
> +
> +	if (!dbus_connection_send_with_reply(connection, message, &call,
> +						GET_MODEMS_TIMEOUT)) {
> +		ofono_error("Sending D-Bus message failed");
> +		goto error;
> +	}
> +
> +	if (call == NULL) {
> +		DBG("D-Bus connection not available");
> +		goto error;
> +	}
> +
> +	dbus_pending_call_set_notify(call, get_modems_reply, NULL, NULL);
> +	dbus_pending_call_unref(call);
> +
> +error:
> +	dbus_message_unref(message);
> +}
> +
> +static gboolean property_changed(DBusConnection *connection,
> +					DBusMessage *message, void *user_data)
> +{
> +	DBusMessageIter iter;
> +	struct ste_modem *stemodem;
> +	const char *key;
> +	enum ste_operation operation;
> +	gboolean operation_valid;
> +
> +	stemodem = g_hash_table_lookup(modem_list,
> +					dbus_message_get_path(message));
> +
> +	if (stemodem == NULL)
> +		return TRUE;
> +
> +
> +	if (!dbus_message_iter_init(message, &iter))
> +		return TRUE;
> +
> +	dbus_message_iter_get_basic(&iter, &key);
> +	dbus_message_iter_next(&iter);
> +
> +	update_property(stemodem, key, &iter, &operation, &operation_valid);
> +
> +	if (operation_valid)
> +		state_change(stemodem, operation);
> +
> +	return TRUE;
> +}
> +
> +static void mgr_connect(DBusConnection *connection, void *user_data)
> +{
> +	property_changed_watch = g_dbus_add_signal_watch(connection, NULL,
> +						NULL,
> +						MGR_MODEM_INTERFACE,
> +						PROPERTY_CHANGED,
> +						property_changed,
> +						NULL, NULL);
> +	get_modems();
> +}
> +
> +static void mgr_disconnect(DBusConnection *connection, void *user_data)
> +{
> +	g_hash_table_remove_all(modem_list);
> +	g_dbus_remove_watch(connection, property_changed_watch);
> +}
> +
> +static void destroy_stemodem(gpointer data)
> +{
> +	struct ste_modem *stemodem = data;
> +
> +	ofono_modem_remove(stemodem->modem);

I would add an extra empty line for pure visual sake here.

> +	g_free(stemodem->interface);
> +	g_free(stemodem->path);
> +	g_free(stemodem->serial);
> +	g_free(stemodem);
> +}
> +
> +static int stemgr_init()
> +{
> +	connection = ofono_dbus_get_connection();
> +
> +	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
> +						NULL, destroy_stemodem);
> +	modem_daemon_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
> +				mgr_connect, mgr_disconnect, NULL, NULL);
> +	return 0;
> +}
> +
> +static void stemgr_exit()
> +{
> +	g_hash_table_destroy(modem_list);
> +	g_dbus_remove_watch(connection, modem_daemon_watch);
> +	g_dbus_remove_watch(connection, property_changed_watch);

This is not right. You need to remove the property_changed_watch in the
disconnect callback. Otherwise if your daemon restarts twice you
actually have two watches.

Regards

Marcel

Re: [PATCHv5] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.

>> +static void get_modems()
>> +{
>
> so these are declared as get_modems(void) btw. The compiler should have
> warned you about this.

I don't get a compiler warning for this one, not even with -Wall
-Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:

M15: Use void if function has no parameters
====================================
A function with no parameters must use void in the parameter list.
Example:
1)
void foo(void)
{
}

2)
void foo()	// Wrong
{
}

...
>> +static void mgr_disconnect(DBusConnection *connection, void *user_data)
>> +{
>> +     g_hash_table_remove_all(modem_list);
>> +     g_dbus_remove_watch(connection, property_changed_watch);
>> +}
>> +
....
>> +static void stemgr_exit()
>> +{
>> +     g_hash_table_destroy(modem_list);
>> +     g_dbus_remove_watch(connection, modem_daemon_watch);
>> +     g_dbus_remove_watch(connection, property_changed_watch);
>
> This is not right. You need to remove the property_changed_watch in the
> disconnect callback. Otherwise if your daemon restarts twice you
> actually have two watches.
It's already present in mgr_disconnect, did you miss that?

In the previous review 2011/1/5 you said:
> +static void stemgr_exit()
> +{
>The only thing you missed here is to remove the property changed watch
>in case it was registered (aka the init daemon started).

Anyway, I think the watch handing in the latest patch is correct,
we need to do remove_watch for property_changed_watch
in both stemgr_exit and in stemgr_disconnect.

Regards,
Sjur

Re: [PATCHv5] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> > so these are declared as get_modems(void) btw. The compiler should have
> > warned you about this.
> 
> I don't get a compiler warning for this one, not even with -Wall
> -Wextra -Wpedantic on GCC 4.4.3.
> But a new rule in the coding-style should perhaps be added, e.g:
> 
> M15: Use void if function has no parameters
> ====================================
> A function with no parameters must use void in the parameter list.
> Example:
> 1)
> void foo(void)
> {
> }
> 
> 2)
> void foo()	// Wrong
> {
> }

please go ahead and send a patch for this. I am happily adding it.

> ...
> >> +static void mgr_disconnect(DBusConnection *connection, void *user_data)
> >> +{
> >> +     g_hash_table_remove_all(modem_list);
> >> +     g_dbus_remove_watch(connection, property_changed_watch);
> >> +}
> >> +
> ....
> >> +static void stemgr_exit()
> >> +{
> >> +     g_hash_table_destroy(modem_list);
> >> +     g_dbus_remove_watch(connection, modem_daemon_watch);
> >> +     g_dbus_remove_watch(connection, property_changed_watch);
> >
> > This is not right. You need to remove the property_changed_watch in the
> > disconnect callback. Otherwise if your daemon restarts twice you
> > actually have two watches.
> It's already present in mgr_disconnect, did you miss that?

I did miss that. So you are correct, but you should add

	property_changed_watch = 0

to mgr_disconnect.

And then do this in stemgr_exit

	if (property_changed_watch > 0)
		g_dbus_remove_watch(...


> In the previous review 2011/1/5 you said:
> > +static void stemgr_exit()
> > +{
> >The only thing you missed here is to remove the property changed watch
> >in case it was registered (aka the init daemon started).
> 
> Anyway, I think the watch handing in the latest patch is correct,
> we need to do remove_watch for property_changed_watch
> in both stemgr_exit and in stemgr_disconnect.

I got lost a little bit, but you still need to make sure to not remove a
watch on plugin exit in case modem init manager not running as I
mentioned above.

Regards

Marcel

Re: [PATCHv5] plugin: Add ste modem initd integration

[Denis Kenzior]

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

Hi Marcel,

On 01/11/2011 03:52 PM, Marcel Holtmann wrote:
> Hi Sjur,
> 
>>> so these are declared as get_modems(void) btw. The compiler should have
>>> warned you about this.
>>
>> I don't get a compiler warning for this one, not even with -Wall
>> -Wextra -Wpedantic on GCC 4.4.3.
>> But a new rule in the coding-style should perhaps be added, e.g:
>>
>> M15: Use void if function has no parameters
>> ====================================
>> A function with no parameters must use void in the parameter list.
>> Example:
>> 1)
>> void foo(void)
>> {
>> }
>>
>> 2)
>> void foo()	// Wrong
>> {
>> }
> 
> please go ahead and send a patch for this. I am happily adding it.
> 

Sorry I'm not following why this is a good idea?

Regards,
-Denis

Re: [PATCHv5] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Denis,

> >>> so these are declared as get_modems(void) btw. The compiler should have
> >>> warned you about this.
> >>
> >> I don't get a compiler warning for this one, not even with -Wall
> >> -Wextra -Wpedantic on GCC 4.4.3.
> >> But a new rule in the coding-style should perhaps be added, e.g:
> >>
> >> M15: Use void if function has no parameters
> >> ====================================
> >> A function with no parameters must use void in the parameter list.
> >> Example:
> >> 1)
> >> void foo(void)
> >> {
> >> }
> >>
> >> 2)
> >> void foo()	// Wrong
> >> {
> >> }
> > 
> > please go ahead and send a patch for this. I am happily adding it.
> > 
> 
> Sorry I'm not following why this is a good idea?

we have always declared functions with no parameters as (void) and not
as (). At least I have.

Regards

Marcel

[PATCHv6] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch introduces auto discovery of ST-Ericsson modems.
ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed
from the Modem Init Damon's Dbus API. When the modem is in state
"on" the STE modem is created and registered. Muliple modem
instances, and reset handling is supported.
---
 Makefile.am      |    3 +
 plugins/stemgr.c |  390 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 393 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

diff --git a/Makefile.am b/Makefile.am
index 09dc9ad..6ccc13b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -293,6 +293,9 @@ builtin_sources += plugins/ifx.c
 builtin_modules += ste
 builtin_sources += plugins/ste.c
 
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += caif
 builtin_sources += plugins/caif.c
 endif
diff --git a/plugins/stemgr.c b/plugins/stemgr.c
new file mode 100644
index 0000000..377e115
--- /dev/null
+++ b/plugins/stemgr.c
@@ -0,0 +1,390 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2011 ST-Ericsson AB.
+ *
+ *  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 <errno.h>
+#include <string.h>
+#include <net/if.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+
+/*
+ * ST-Ericsson's Modem Init Daemon is used for controling the modem power
+ * cycles and provides a dbus API for modem state and properties.
+ */
+#define MGR_SERVICE		"com.stericsson.modeminit"
+#define MGR_INTERFACE		MGR_SERVICE ".Manager"
+#define MGR_GET_MODEMS		"GetModems"
+#define GET_MODEMS_TIMEOUT	5000
+
+#define MGR_MODEM_INTERFACE	MGR_SERVICE ".Modem"
+#define PROPERTY_CHANGED	"PropertyChanged"
+
+enum ste_state {
+	STE_STATE_OFF,
+	STE_STATE_READY,
+	STE_STATE_RESET
+};
+
+enum ste_operation {
+	STE_OP_STARTING,
+	STE_OP_READY,
+	STE_OP_RESTART,
+	STE_OP_OFF
+};
+
+struct ste_modem {
+	char *path;
+	struct ofono_modem *modem;
+	enum ste_state state;
+	char *serial;
+	char *interface;
+};
+
+static GHashTable *modem_list;
+static guint modem_daemon_watch;
+static guint property_changed_watch;
+static DBusConnection *connection;
+
+static void state_change(struct ste_modem *stemodem, enum ste_operation op)
+{
+	switch (stemodem->state) {
+	case STE_STATE_OFF:
+		/*
+		 * The STE Modem is in state OFF and we're waiting for
+		 * the Modem Init Daemon to signal that modem is ready
+		 * in order to create and register the modem.
+		 */
+		switch (op) {
+		case STE_OP_READY:
+			stemodem->modem = ofono_modem_create(stemodem->serial,
+								"ste");
+			if (stemodem->modem == NULL) {
+				ofono_error("Could not create modem %s, %s",
+						stemodem->path,
+						stemodem->serial);
+				return;
+			}
+
+			DBG("register modem %s, %s", stemodem->path,
+				stemodem->serial);
+
+			if (stemodem->interface != NULL)
+				ofono_modem_set_string(stemodem->modem,
+							"Interface",
+							stemodem->interface);
+
+			ofono_modem_register(stemodem->modem);
+			stemodem->state = STE_STATE_READY;
+			break;
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+		case STE_OP_OFF:
+			break;
+		}
+		break;
+	case STE_STATE_READY:
+		/*
+		 * The STE Modem is ready and the modem has been created
+		 * and registered in oFono. In this state two things can
+		 * happen: Modem restarts or is turned off. Turning off
+		 * the modem is an exceptional situation e.g. high-temperature,
+		 * low battery or upgrade. In this scenario we remove the
+		 * STE modem from oFono.
+		 */
+		switch (op) {
+		case STE_OP_READY:
+			break;
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+			DBG("reset ongoing %s", stemodem->path);
+			/* Note: Consider to power off modem here? */
+			stemodem->state = STE_STATE_RESET;
+			break;
+		case STE_OP_OFF:
+			DBG("STE modem unregistering %s", stemodem->path);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_STATE_OFF;
+			break;
+		}
+		break;
+	case STE_STATE_RESET:
+		/*
+		 * The STE Modem is resetting.In this state two things can
+		 * happen: Modem restarts succeeds, or modem is turned off.
+		 */
+		switch (op) {
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+			break;
+		case STE_OP_READY:
+			DBG("STE modem reset complete %s", stemodem->path);
+			if (ofono_modem_get_powered(stemodem->modem))
+				ofono_modem_reset(stemodem->modem);
+			stemodem->state = STE_STATE_READY;
+			break;
+		case STE_OP_OFF:
+			DBG("STE modem unregistering %s", stemodem->path);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_STATE_OFF;
+			break;
+		}
+		break;
+	}
+}
+
+static void update_property(struct ste_modem *stemodem, const char *prop,
+				DBusMessageIter *iter, enum ste_operation *op,
+				gboolean *op_valid)
+{
+	const char *value;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
+		return;
+
+	dbus_message_iter_get_basic(iter, &value);
+
+	if (g_strcmp0(prop, "State") == 0) {
+		*op_valid = TRUE;
+		if (g_strcmp0(value, "booting") == 0)
+			*op = STE_OP_STARTING;
+		else if (g_strcmp0(value, "upgrading") == 0)
+			*op = STE_OP_OFF;
+		else if (g_strcmp0(value, "ready") == 0)
+			*op = STE_OP_READY;
+		else if (g_strcmp0(value, "off") == 0)
+			*op = STE_OP_OFF;
+		else if (g_strcmp0(value, "dumping") == 0)
+			*op = STE_OP_RESTART;
+		else
+			*op_valid = FALSE;
+	} else if (g_strcmp0(prop, "Interface") == 0) {
+		g_free(stemodem->interface);
+		stemodem->interface = g_strdup(value);
+	} else if (g_strcmp0(prop, "Serial") == 0) {
+		g_free(stemodem->serial);
+		stemodem->serial = g_strdup(value);
+	}
+}
+
+static void update_modem_properties(const char *path, DBusMessageIter *iter)
+{
+	enum ste_operation operation;
+	gboolean operation_valid;
+	struct ste_modem *stemodem = g_hash_table_lookup(modem_list, path);
+
+	if (stemodem == NULL) {
+		stemodem = g_try_new0(struct ste_modem, 1);
+		if (stemodem == NULL)
+			return;
+
+		stemodem->path = g_strdup(path);
+		stemodem->state = STE_STATE_OFF;
+		g_hash_table_insert(modem_list, stemodem->path, stemodem);
+	}
+
+	while (dbus_message_iter_get_arg_type(iter) == DBUS_TYPE_DICT_ENTRY) {
+		DBusMessageIter entry, value;
+		const char *key;
+
+		dbus_message_iter_recurse(iter, &entry);
+		dbus_message_iter_get_basic(&entry, &key);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &value);
+
+		update_property(stemodem, key, &value, &operation,
+					&operation_valid);
+
+		dbus_message_iter_next(iter);
+	}
+
+	if (operation_valid)
+		state_change(stemodem, operation);
+}
+
+static void get_modems_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessageIter iter, list;
+	DBusError err;
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&err);
+
+	if (dbus_set_error_from_message(&err, reply)) {
+		ofono_error("%s: %s\n", err.name, err.message);
+		dbus_error_free(&err);
+		goto done;
+	}
+
+	if (!dbus_message_has_signature(reply, "a(oa{sv})"))
+		goto done;
+
+	if (!dbus_message_iter_init(reply, &iter))
+		goto done;
+
+	dbus_message_iter_recurse(&iter, &list);
+
+	while (dbus_message_iter_get_arg_type(&list) == DBUS_TYPE_STRUCT) {
+		DBusMessageIter entry, dict;
+		const char *path;
+
+		dbus_message_iter_recurse(&list, &entry);
+		dbus_message_iter_get_basic(&entry, &path);
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &dict);
+
+		update_modem_properties(path, &dict);
+
+		dbus_message_iter_next(&list);
+	}
+
+done:
+	dbus_message_unref(reply);
+}
+
+static void get_modems(void)
+{
+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	message = dbus_message_new_method_call(MGR_SERVICE, "/",
+					MGR_INTERFACE, MGR_GET_MODEMS);
+	if (message == NULL) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	dbus_message_set_auto_start(message, FALSE);
+
+	if (!dbus_connection_send_with_reply(connection, message, &call,
+						GET_MODEMS_TIMEOUT)) {
+		ofono_error("Sending D-Bus message failed");
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		goto error;
+	}
+
+	dbus_pending_call_set_notify(call, get_modems_reply, NULL, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static gboolean property_changed(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	DBusMessageIter iter;
+	struct ste_modem *stemodem;
+	const char *key;
+	enum ste_operation operation;
+	gboolean operation_valid;
+
+	stemodem = g_hash_table_lookup(modem_list,
+					dbus_message_get_path(message));
+
+	if (stemodem == NULL)
+		return TRUE;
+
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+	dbus_message_iter_next(&iter);
+
+	update_property(stemodem, key, &iter, &operation, &operation_valid);
+
+	if (operation_valid)
+		state_change(stemodem, operation);
+
+	return TRUE;
+}
+
+static void mgr_connect(DBusConnection *connection, void *user_data)
+{
+	property_changed_watch = g_dbus_add_signal_watch(connection, NULL,
+						NULL,
+						MGR_MODEM_INTERFACE,
+						PROPERTY_CHANGED,
+						property_changed,
+						NULL, NULL);
+	get_modems();
+}
+
+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+	g_hash_table_remove_all(modem_list);
+	g_dbus_remove_watch(connection, property_changed_watch);
+	property_changed_watch = 0;
+}
+
+static void destroy_stemodem(gpointer data)
+{
+	struct ste_modem *stemodem = data;
+
+	ofono_modem_remove(stemodem->modem);
+
+	g_free(stemodem->interface);
+	g_free(stemodem->path);
+	g_free(stemodem->serial);
+	g_free(stemodem);
+}
+
+static int stemgr_init(void)
+{
+	connection = ofono_dbus_get_connection();
+
+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);
+	modem_daemon_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
+				mgr_connect, mgr_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemgr_exit(void)
+{
+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, modem_daemon_watch);
+
+	if (property_changed_watch > 0)
+		g_dbus_remove_watch(connection, property_changed_watch);
+
+}
+
+OFONO_PLUGIN_DEFINE(stemgr, "ST-Ericsson Modem Init Daemon detection", VERSION,
+			OFONO_PLUGIN_PRIORITY_DEFAULT, stemgr_init, stemgr_exit)
-- 
1.7.0.4

Re: [PATCHv6] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> This patch introduces auto discovery of ST-Ericsson modems.
> ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
> Modem Init Daemon responsible for start, power cycles,
> flashing etc. This STE plugin monitors the modem state exposed
> from the Modem Init Damon's Dbus API. When the modem is in state
> "on" the STE modem is created and registered. Muliple modem
> instances, and reset handling is supported.
> ---
>  Makefile.am      |    3 +
>  plugins/stemgr.c |  390 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 393 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/stemgr.c

you are out of luck today ;)

Applying: plugin: Add ste modem initd integration
error: Makefile.am: does not match index
Patch failed at 0001 plugin: Add ste modem initd integration

Regards

Marcel

[PATCH] coding-style: Use void if function has no parameters

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

---
Marcel, Denis,

Here is the patch, please apply it if you agree. I'm easy :-)

Regards,
Sjur

 doc/coding-style.txt |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/doc/coding-style.txt b/doc/coding-style.txt
index 3bee240..40bb36b 100644
--- a/doc/coding-style.txt
+++ b/doc/coding-style.txt
@@ -279,6 +279,21 @@ memset(stuff, 0, sizeof(*stuff));
 memset(stuff, 0, sizeof *stuff); // Wrong
 
 
+M15: Use void if function has no parameters
+===========================================================
+A function with no parameters must use void in the parameter list.
+
+Example:
+1)
+void foo(void)
+{
+}
+
+2)
+void foo()	// Wrong
+{
+}
+
 O1: Shorten the name
 ====================
 Better to use abbreviation, rather than full name, to name a variable,
-- 
1.7.0.4

Re: [PATCHv6] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.
> you are out of luck today ;)

Darn, I forgot to pull. Hang on I'll be back shortly....

Regards,
Sjur

Re: [PATCH] coding-style: Use void if function has no parameters

[Marcel Holtmann]

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

Hi Sjur,

>  doc/coding-style.txt |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)

patch has been applied. Thanks.

Regards

Marcel

[PATCHv7] plugin: Add ste modem initd integration

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch introduces auto discovery of ST-Ericsson modems.
ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed
from the Modem Init Damon's Dbus API. When the modem is in state
"on" the STE modem is created and registered. Muliple modem
instances, and reset handling is supported.
---
Hi Marcel,

Am I still out of luck?

$ git pull
Already up-to-date.
$ git am 0001-plugin-Add-ste-modem-initd-integration.patch
Applying: plugin: Add ste modem initd integration

Regards,
Sjur

 Makefile.am      |    3 +
 plugins/stemgr.c |  390 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 393 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

diff --git a/Makefile.am b/Makefile.am
index cc30624..4dec90a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -298,6 +298,9 @@ builtin_sources += plugins/ifx.c
 builtin_modules += ste
 builtin_sources += plugins/ste.c
 
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += caif
 builtin_sources += plugins/caif.c
 
diff --git a/plugins/stemgr.c b/plugins/stemgr.c
new file mode 100644
index 0000000..377e115
--- /dev/null
+++ b/plugins/stemgr.c
@@ -0,0 +1,390 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2011 ST-Ericsson AB.
+ *
+ *  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 <errno.h>
+#include <string.h>
+#include <net/if.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+
+/*
+ * ST-Ericsson's Modem Init Daemon is used for controling the modem power
+ * cycles and provides a dbus API for modem state and properties.
+ */
+#define MGR_SERVICE		"com.stericsson.modeminit"
+#define MGR_INTERFACE		MGR_SERVICE ".Manager"
+#define MGR_GET_MODEMS		"GetModems"
+#define GET_MODEMS_TIMEOUT	5000
+
+#define MGR_MODEM_INTERFACE	MGR_SERVICE ".Modem"
+#define PROPERTY_CHANGED	"PropertyChanged"
+
+enum ste_state {
+	STE_STATE_OFF,
+	STE_STATE_READY,
+	STE_STATE_RESET
+};
+
+enum ste_operation {
+	STE_OP_STARTING,
+	STE_OP_READY,
+	STE_OP_RESTART,
+	STE_OP_OFF
+};
+
+struct ste_modem {
+	char *path;
+	struct ofono_modem *modem;
+	enum ste_state state;
+	char *serial;
+	char *interface;
+};
+
+static GHashTable *modem_list;
+static guint modem_daemon_watch;
+static guint property_changed_watch;
+static DBusConnection *connection;
+
+static void state_change(struct ste_modem *stemodem, enum ste_operation op)
+{
+	switch (stemodem->state) {
+	case STE_STATE_OFF:
+		/*
+		 * The STE Modem is in state OFF and we're waiting for
+		 * the Modem Init Daemon to signal that modem is ready
+		 * in order to create and register the modem.
+		 */
+		switch (op) {
+		case STE_OP_READY:
+			stemodem->modem = ofono_modem_create(stemodem->serial,
+								"ste");
+			if (stemodem->modem == NULL) {
+				ofono_error("Could not create modem %s, %s",
+						stemodem->path,
+						stemodem->serial);
+				return;
+			}
+
+			DBG("register modem %s, %s", stemodem->path,
+				stemodem->serial);
+
+			if (stemodem->interface != NULL)
+				ofono_modem_set_string(stemodem->modem,
+							"Interface",
+							stemodem->interface);
+
+			ofono_modem_register(stemodem->modem);
+			stemodem->state = STE_STATE_READY;
+			break;
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+		case STE_OP_OFF:
+			break;
+		}
+		break;
+	case STE_STATE_READY:
+		/*
+		 * The STE Modem is ready and the modem has been created
+		 * and registered in oFono. In this state two things can
+		 * happen: Modem restarts or is turned off. Turning off
+		 * the modem is an exceptional situation e.g. high-temperature,
+		 * low battery or upgrade. In this scenario we remove the
+		 * STE modem from oFono.
+		 */
+		switch (op) {
+		case STE_OP_READY:
+			break;
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+			DBG("reset ongoing %s", stemodem->path);
+			/* Note: Consider to power off modem here? */
+			stemodem->state = STE_STATE_RESET;
+			break;
+		case STE_OP_OFF:
+			DBG("STE modem unregistering %s", stemodem->path);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_STATE_OFF;
+			break;
+		}
+		break;
+	case STE_STATE_RESET:
+		/*
+		 * The STE Modem is resetting.In this state two things can
+		 * happen: Modem restarts succeeds, or modem is turned off.
+		 */
+		switch (op) {
+		case STE_OP_STARTING:
+		case STE_OP_RESTART:
+			break;
+		case STE_OP_READY:
+			DBG("STE modem reset complete %s", stemodem->path);
+			if (ofono_modem_get_powered(stemodem->modem))
+				ofono_modem_reset(stemodem->modem);
+			stemodem->state = STE_STATE_READY;
+			break;
+		case STE_OP_OFF:
+			DBG("STE modem unregistering %s", stemodem->path);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_STATE_OFF;
+			break;
+		}
+		break;
+	}
+}
+
+static void update_property(struct ste_modem *stemodem, const char *prop,
+				DBusMessageIter *iter, enum ste_operation *op,
+				gboolean *op_valid)
+{
+	const char *value;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
+		return;
+
+	dbus_message_iter_get_basic(iter, &value);
+
+	if (g_strcmp0(prop, "State") == 0) {
+		*op_valid = TRUE;
+		if (g_strcmp0(value, "booting") == 0)
+			*op = STE_OP_STARTING;
+		else if (g_strcmp0(value, "upgrading") == 0)
+			*op = STE_OP_OFF;
+		else if (g_strcmp0(value, "ready") == 0)
+			*op = STE_OP_READY;
+		else if (g_strcmp0(value, "off") == 0)
+			*op = STE_OP_OFF;
+		else if (g_strcmp0(value, "dumping") == 0)
+			*op = STE_OP_RESTART;
+		else
+			*op_valid = FALSE;
+	} else if (g_strcmp0(prop, "Interface") == 0) {
+		g_free(stemodem->interface);
+		stemodem->interface = g_strdup(value);
+	} else if (g_strcmp0(prop, "Serial") == 0) {
+		g_free(stemodem->serial);
+		stemodem->serial = g_strdup(value);
+	}
+}
+
+static void update_modem_properties(const char *path, DBusMessageIter *iter)
+{
+	enum ste_operation operation;
+	gboolean operation_valid;
+	struct ste_modem *stemodem = g_hash_table_lookup(modem_list, path);
+
+	if (stemodem == NULL) {
+		stemodem = g_try_new0(struct ste_modem, 1);
+		if (stemodem == NULL)
+			return;
+
+		stemodem->path = g_strdup(path);
+		stemodem->state = STE_STATE_OFF;
+		g_hash_table_insert(modem_list, stemodem->path, stemodem);
+	}
+
+	while (dbus_message_iter_get_arg_type(iter) == DBUS_TYPE_DICT_ENTRY) {
+		DBusMessageIter entry, value;
+		const char *key;
+
+		dbus_message_iter_recurse(iter, &entry);
+		dbus_message_iter_get_basic(&entry, &key);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &value);
+
+		update_property(stemodem, key, &value, &operation,
+					&operation_valid);
+
+		dbus_message_iter_next(iter);
+	}
+
+	if (operation_valid)
+		state_change(stemodem, operation);
+}
+
+static void get_modems_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessageIter iter, list;
+	DBusError err;
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&err);
+
+	if (dbus_set_error_from_message(&err, reply)) {
+		ofono_error("%s: %s\n", err.name, err.message);
+		dbus_error_free(&err);
+		goto done;
+	}
+
+	if (!dbus_message_has_signature(reply, "a(oa{sv})"))
+		goto done;
+
+	if (!dbus_message_iter_init(reply, &iter))
+		goto done;
+
+	dbus_message_iter_recurse(&iter, &list);
+
+	while (dbus_message_iter_get_arg_type(&list) == DBUS_TYPE_STRUCT) {
+		DBusMessageIter entry, dict;
+		const char *path;
+
+		dbus_message_iter_recurse(&list, &entry);
+		dbus_message_iter_get_basic(&entry, &path);
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &dict);
+
+		update_modem_properties(path, &dict);
+
+		dbus_message_iter_next(&list);
+	}
+
+done:
+	dbus_message_unref(reply);
+}
+
+static void get_modems(void)
+{
+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	message = dbus_message_new_method_call(MGR_SERVICE, "/",
+					MGR_INTERFACE, MGR_GET_MODEMS);
+	if (message == NULL) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	dbus_message_set_auto_start(message, FALSE);
+
+	if (!dbus_connection_send_with_reply(connection, message, &call,
+						GET_MODEMS_TIMEOUT)) {
+		ofono_error("Sending D-Bus message failed");
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		goto error;
+	}
+
+	dbus_pending_call_set_notify(call, get_modems_reply, NULL, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static gboolean property_changed(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	DBusMessageIter iter;
+	struct ste_modem *stemodem;
+	const char *key;
+	enum ste_operation operation;
+	gboolean operation_valid;
+
+	stemodem = g_hash_table_lookup(modem_list,
+					dbus_message_get_path(message));
+
+	if (stemodem == NULL)
+		return TRUE;
+
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+	dbus_message_iter_next(&iter);
+
+	update_property(stemodem, key, &iter, &operation, &operation_valid);
+
+	if (operation_valid)
+		state_change(stemodem, operation);
+
+	return TRUE;
+}
+
+static void mgr_connect(DBusConnection *connection, void *user_data)
+{
+	property_changed_watch = g_dbus_add_signal_watch(connection, NULL,
+						NULL,
+						MGR_MODEM_INTERFACE,
+						PROPERTY_CHANGED,
+						property_changed,
+						NULL, NULL);
+	get_modems();
+}
+
+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+	g_hash_table_remove_all(modem_list);
+	g_dbus_remove_watch(connection, property_changed_watch);
+	property_changed_watch = 0;
+}
+
+static void destroy_stemodem(gpointer data)
+{
+	struct ste_modem *stemodem = data;
+
+	ofono_modem_remove(stemodem->modem);
+
+	g_free(stemodem->interface);
+	g_free(stemodem->path);
+	g_free(stemodem->serial);
+	g_free(stemodem);
+}
+
+static int stemgr_init(void)
+{
+	connection = ofono_dbus_get_connection();
+
+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);
+	modem_daemon_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
+				mgr_connect, mgr_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemgr_exit(void)
+{
+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, modem_daemon_watch);
+
+	if (property_changed_watch > 0)
+		g_dbus_remove_watch(connection, property_changed_watch);
+
+}
+
+OFONO_PLUGIN_DEFINE(stemgr, "ST-Ericsson Modem Init Daemon detection", VERSION,
+			OFONO_PLUGIN_PRIORITY_DEFAULT, stemgr_init, stemgr_exit)
-- 
1.7.0.4

Re: [PATCHv7] plugin: Add ste modem initd integration

[Marcel Holtmann]

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

Hi Sjur,

> This patch introduces auto discovery of ST-Ericsson modems.
> ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
> Modem Init Daemon responsible for start, power cycles,
> flashing etc. This STE plugin monitors the modem state exposed
> from the Modem Init Damon's Dbus API. When the modem is in state
> "on" the STE modem is created and registered. Muliple modem
> instances, and reset handling is supported.

<snip>

> Am I still out of luck?
> 
> $ git pull
> Already up-to-date.
> $ git am 0001-plugin-Add-ste-modem-initd-integration.patch
> Applying: plugin: Add ste modem initd integration

patch has been applied. Thanks.

Regards

Marcel