Skip to contentSkip to conversation

stemodem: Create network interfaces statically

PATCH RESEND Superseded Sjur Brændeland 2 participants 5 messages Nov 30, 2010 – Dec 9, 2010 Kbuild

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Sjur Brændeland

author
#0 lore raw

PATCH RESEND v2 1/2 stemodem: Create network interfaces statically

From: Sjur Brændeland <[email protected]>

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.
---
Resend after re-basing with latest commit:
"drivers: explicitly compare pointers to NULL".

 drivers/stemodem/gprs-context.c |  149 +++++++++++++++++++++-----------------
 1 files changed, 82 insertions(+), 67 deletions(-)
drivers/stemodem/gprs-context.c +82 -67
index faa3124..7bdcdb8 100644
@@ -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;
Signature · 1 line
1.7.0.4
author
#1 lore raw

PATCH RESEND v2 2/2 stemodem: Use RTNL to create network interfaces.

From: Sjur Brændeland <[email protected]>

---
 drivers/stemodem/gprs-context.c |   44 ++++++++++++++++++++++++++++++++------
 1 files changed, 37 insertions(+), 7 deletions(-)
drivers/stemodem/gprs-context.c +37 -7
index bfc0d65..21ca126 100644
@@ -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,18 @@ 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);
+	return;
 }
 
 static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,

@@ -526,7 +547,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 +560,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 +599,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()
 {
+	caif_rtnl_exit();
 	ofono_gprs_context_driver_unregister(&driver);
 }
Signature · 1 line
1.6.3.3
#2 lore raw

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

Hi Sjur,

Quoted 44 lines
drivers/stemodem/gprs-context.c +16 -5
index bfc0d65..21ca126 100644
@@ -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,18 @@ static gboolean caif_if_create(struct conn_info *conn)
  */
 static void caif_if_remove(struct conn_info *conn)
 {
+	if (!conn->created)
+		return;

drivers/stemodem/gprs-context.c static gboolean caif_if_create(struct conn_info *conn) · line 208

Quoted 10 lines
drivers/stemodem/gprs-context.c +10 -0
+
+	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);
+	return;

You don't need return here. Please remove that.

drivers/stemodem/gprs-context.c static int ste_gprs_context_probe(struct ofono_gprs_context *gc, · line 550

Quoted 9 lines
drivers/stemodem/gprs-context.c +1 -1
 }
 
 static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,

@@ -526,7 +547,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;

Extra whitespace between i and err.

Quoted 22 lines
drivers/stemodem/gprs-context.c +8 -1
 	gcd = g_new0(struct gprs_context_data, 1);
 	gcd->chat = g_at_chat_clone(chat);

@@ -539,7 +560,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 +599,12 @@ static struct ofono_gprs_context_driver driver = {
 
 void ste_gprs_context_init()

drivers/stemodem/gprs-context.c static struct ofono_gprs_context_driver driver = { · line 610

Quoted 10 lines
drivers/stemodem/gprs-context.c +2 -0
 {
+	caif_rtnl_init();
 	ofono_gprs_context_driver_register(&driver);
 }
 
 void ste_gprs_context_exit()
 {
+	caif_rtnl_exit();
 	ofono_gprs_context_driver_unregister(&driver);
 }

Are you sure you want rtnl_exit before removing the driver?

Regards

Marcel

author
#3 lore raw

PATCH RESEND 1/1 plugin: Add ste modem initd integration

In reply to Sjur Brændeland

From: Sjur Brændeland <[email protected]>

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a Modem Init Daemon that is responsible for start/stop/restart flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon Dbus API. When the modem is in state "on" the STE modem is created and registered.

Currently only a single modem instance is supported, support for multiple modems should be added in the future.

---
 Makefile.am      |    5 ++
 configure.ac     |    6 ++
 plugins/stemid.c |  195 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemid.c
Makefile.am +5 -0
index f841b4c..aaf5de5 100644
@@ -81,6 +81,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
configure.ac +6 -0
index c0f4217..4dff372 100644
@@ -175,6 +175,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
plugins/stemid.c new file +195 -0

195 lines not on this page · raw message

Signature · 1 line
1.6.3.3
#4 lore raw

Re: [RESEND PATCH] plugin: Add ste modem initd integration

Hi Sjur,

Quoted 9 lines

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a Modem Init Daemon that is responsible for start/stop/restart flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon Dbus API. When the modem is in state "on" the STE modem is created and registered.

Currently only a single modem instance is supported, support for multiple modems should be added in the future.

I am not really happy about this here. I prefer that we do that right from the beginning. Multiple modem support should be default.

Quoted 28 lines
---
 Makefile.am      |    5 ++
 configure.ac     |    6 ++
 plugins/stemid.c |  195 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemid.c
Makefile.am +5 -0
index f841b4c..aaf5de5 100644
@@ -81,6 +81,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
configure.ac +0 -0
index c0f4217..4dff372 100644

configure.ac AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles], · line 183

Quoted 9 lines
configure.ac +6 -0
@@ -175,6 +175,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 the conditional stuff. Just always built in the plugin. There is no harm here and I can sort this out later cleanly when we have a bit better understanding of what requires what.

Quoted 72 lines
configure.ac +0 -0
 if (test "${prefix}" = "NONE"); then
 	dnl no prefix and no localstatedir, so default to /var
 	if (test "$localstatedir" = '${prefix}/var'); then
plugins/stemid.c new file +63 -0
new file mode 100644
index 0000000..7041e5e
@@ -0,0 +1,195 @@
+/*
+ *
+ *  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 <stdio.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <net/if.h>
+
+#include <gdbus.h>
+#include <glib.h>
+#include <gatchat.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+#include <ofono/version.h>
+
+/*
+ * ST-Ericsson's modem init daemon defines the signal StateChange
+ * and the method GetState. When state is "on" the STE modem is
+ * created and registered.
+ */
+#define STATUS_CHANGED	"StateChange"
+#define MID_SERVICE	"com.stericsson.modeminit"
+#define MID_INTERFACE	MID_SERVICE ".Modem"
+#define GET_STATE	"GetState"
+#define MID_STATE_ON	"on"
+#define MID_STATE_DISCONNECT	"disconnect"
+#define TIMEOUT	5000
+
+static struct ofono_modem *ste_modem;
+static guint mid_api_watch;
+static guint mid_state_watch;
+
+static void handle_stemodem(const char *state)
+{

plugins/stemid.c line 73

Quoted 10 lines
plugins/stemid.c new file +10 -0
+
+	DBG("state:%s", state);
+
+	if (strcmp(state, MID_STATE_ON) == 0) {
+		int err;
+
+		if (ste_modem != NULL)
+			return;
+
+		ste_modem = ofono_modem_create("ste", "ste");

If you do such an init daemon anyway, can't you not at least also expose the serial number in a proper format?

Quoted 99 lines
plugins/stemid.c new file +99 -0
+		DBG("register STE modem");
+		err = ofono_modem_register(ste_modem);
+	} else {
+
+		if (ste_modem == NULL)
+			return;
+
+		ofono_modem_remove(ste_modem);
+		ste_modem = NULL;
+	}
+}
+
+static void mid_getstate_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessage *reply;
+	char *state;
+
+	reply = dbus_pending_call_steal_reply(call);
+
+	if (dbus_message_is_error(reply, DBUS_ERROR_SERVICE_UNKNOWN)) {
+		ofono_error("STE Modem Init Daemon is unavailable");
+		goto error;
+	}
+
+	if (dbus_message_get_args(reply, NULL, DBUS_TYPE_STRING, &state,
+					DBUS_TYPE_INVALID) == FALSE) {
+		ofono_error("STE Modem Init Damon: bad signature for GetState");
+		goto error;
+	}
+
+	handle_stemodem(state);
+
+error:
+	dbus_message_unref(reply);
+}
+
+static gboolean mid_signal_status_change(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	const char *state;
+
+	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;
+	}
+
+	handle_stemodem(state);
+	return TRUE;
+}
+
+static void mid_connect(DBusConnection *connection, void *user_data)
+{
+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	DBG("");
+	message = dbus_message_new_method_call(MID_SERVICE, "/",
+					MID_INTERFACE, "GetState");
+
+	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");
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		goto error;
+	}
+
+	dbus_pending_call_set_notify(call, mid_getstate_reply, NULL, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static void mid_disconnect(DBusConnection *connection, void *user_data)
+{
+	handle_stemodem(MID_STATE_DISCONNECT);
+}
+
+static int stemid_init()
+{
+	DBusConnection *connection;
+
+	DBG("");
+
+	connection = ofono_dbus_get_connection();
+
+	if (connection == NULL)

plugins/stemid.c line 182

Quoted 10 lines
plugins/stemid.c new file +10 -0
+		return -EIO;
+
+	mid_api_watch = g_dbus_add_service_watch(connection, MID_SERVICE,
+				mid_connect, mid_disconnect, NULL, NULL);
+
+	mid_state_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
+						MID_INTERFACE,
+						STATUS_CHANGED,
+						mid_signal_status_change,
+						NULL, NULL);

I would prefer that the signal watch is done insider the mid_connect and mid_disconnect callback.

plugins/stemid.c line 188

Quoted 6 lines
plugins/stemid.c new file +6 -0
+	return 0;
+}
+
+static void stemid_exit()
+{
+	DBusConnection *connection = ofono_dbus_get_connection();

Please store the DBusConnection as a global public variable. Getting it twice here is not what I prefer to do.

plugins/stemid.c line 195

Quoted 7 lines
plugins/stemid.c new file +7 -0
+
+	g_dbus_remove_watch(connection, mid_state_watch);
+	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)

Regards

Marcel