[PATCH udev 00/15] Device handling cleanup series

[PATCH udev 00/15] Device handling cleanup series

[Jonas Bonn]

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

This is a series of incremental cleanups to the libudev device handling
code.

- the early patches in the series are all good cleanups that can be
  taken irregardless of whether the last patches are desired
- the last patches are more intrusive as they attempt to move the
  functionality of udev.c into udevng.c

Somebody with access to old hardware really needs to take a look at
this series to see if the migration to udevng works properly.  I do
not have any such hardware, myself.  I suspect that there's not many
such devices in circulation; my hope is that this legacy code works
for now and can be ripped out in time.

In any case, if someone has such old hardware, hopefully these changes
are sufficiently correct that the last bugs can shake themselves out
easily... we'll see.

Jonas Bonn (15):
  udevng: simplify logic in check_usb_device
  ofono.rules: remove 'change' action
  udev: remove extraneous subsystem check
  udev: get udev property via lib function
  udev: remove unused modem property
  udev: simplify add_modem
  udev: add common modem registration code
  udev: simplify ifx modem registration
  udev: simplify wavecom modem registration
  udev: remove extraneous subsystem check
  udevng: add legacy device handling functions
  udevng: match on the hsi subsystem for legacy devices
  udevng: hook up legacy devices
  udevng: get properties from interface
  plugins: remove udev module

 Makefile.am         |   2 -
 plugins/ofono.rules |   2 +-
 plugins/udev.c      | 545 ----------------------------------------------------
 plugins/udevng.c    | 307 ++++++++++++++++++++++++++---
 4 files changed, 283 insertions(+), 573 deletions(-)
 delete mode 100644 plugins/udev.c

-- 
2.9.3

[PATCH udev 01/15] udevng: simplify logic in check_usb_device

[Jonas Bonn]

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

This patch simplifies and cleans up the check_usb_device function a bit
by doing the two following (slightly intertwined) things:

1)  The parent "usb_device" is searched for early in this function and this
device will always have the ID_VENDOR_ID and ID_MODEL_ID properties.
As such, we can get them from this device and thereby be certain that
we _always_ have them available.

2)  The logic of iterating the vendor_list table is cleaned up.  It's
easier to follow and won't be any less efficient.
---
 plugins/udevng.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 2279bbe..ce2bc28 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1225,9 +1225,12 @@ static void check_usb_device(struct udev_device *device)
 	if (devname == NULL)
 		return;
 
+	vendor = udev_device_get_property_value(usb_device, "ID_VENDOR_ID");
+	model = udev_device_get_property_value(usb_device, "ID_MODEL_ID");
+
 	driver = udev_device_get_property_value(usb_device, "OFONO_DRIVER");
 	if (driver == NULL) {
-		const char *drv, *vid, *pid;
+		const char *drv;
 		unsigned int i;
 
 		drv = udev_device_get_property_value(device, "ID_USB_DRIVER");
@@ -1246,40 +1249,24 @@ static void check_usb_device(struct udev_device *device)
 			}
 		}
 
-		vid = udev_device_get_property_value(device, "ID_VENDOR_ID");
-		pid = udev_device_get_property_value(device, "ID_MODEL_ID");
 
-		DBG("%s [%s:%s]", drv, vid, pid);
+		DBG("%s [%s:%s]", drv, vendor, model);
 
 		for (i = 0; vendor_list[i].driver; i++) {
 			if (g_str_equal(vendor_list[i].drv, drv) == FALSE)
 				continue;
 
-			if (vendor_list[i].vid == NULL) {
-				driver = vendor_list[i].driver;
-				vendor = vid;
-				model = pid;
-				continue;
+			if (vendor_list[i].vid) {
+				if (!g_str_equal(vendor_list[i].vid, vendor))
+					continue;
 			}
 
-			if (vid == NULL || pid == NULL)
-				continue;
-
-			if (g_str_equal(vendor_list[i].vid, vid) == TRUE) {
-				if (vendor_list[i].pid == NULL) {
-					driver = vendor_list[i].driver;
-					vendor = vid;
-					model = pid;
+			if (vendor_list[i].pid) {
+				if (!g_str_equal(vendor_list[i].pid, model))
 					continue;
-				}
-
-				if (g_strcmp0(vendor_list[i].pid, pid) == 0) {
-					driver = vendor_list[i].driver;
-					vendor = vid;
-					model = pid;
-					break;
-				}
 			}
+
+			driver = vendor_list[i].driver;
 		}
 
 		if (driver == NULL)
-- 
2.9.3

[PATCH udev 02/15] ofono.rules: remove 'change' action

[Jonas Bonn]

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

The code doesn't do anything with this action so don't bother setting
extra device properties for it.
---
 plugins/ofono.rules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/ofono.rules b/plugins/ofono.rules
index 3ed9f16..450f6c6 100644
--- a/plugins/ofono.rules
+++ b/plugins/ofono.rules
@@ -1,6 +1,6 @@
 # do not edit this file, it will be overwritten on update
 
-ACTION!="add|change", GOTO="ofono_end"
+ACTION!="add", GOTO="ofono_end"
 
 # ISI/Phonet drivers
 SUBSYSTEM!="net", GOTO="ofono_isi_end"
-- 
2.9.3

[PATCH udev 03/15] udev: remove extraneous subsystem check

[Jonas Bonn]

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

The udev-monitor already guarantees that only devices with these
subsystems will be returned so we don't need to check again.
---
 plugins/udev.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index 3c90e40..196c528 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -410,7 +410,7 @@ static gboolean udev_event(GIOChannel *channel, GIOCondition cond,
 							gpointer user_data)
 {
 	struct udev_device *device;
-	const char *subsystem, *action;
+	const char *action;
 
 	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
 		ofono_warn("Error with udev monitor channel");
@@ -422,30 +422,16 @@ static gboolean udev_event(GIOChannel *channel, GIOCondition cond,
 	if (device == NULL)
 		return TRUE;
 
-	subsystem = udev_device_get_subsystem(device);
-	if (subsystem == NULL)
-		goto done;
-
 	action = udev_device_get_action(device);
 	if (action == NULL)
 		goto done;
 
-	DBG("subsystem %s %s", subsystem, action);
-
 	if (g_str_equal(action, "add") == TRUE) {
-		if (g_strcmp0(subsystem, "tty") == 0 ||
-				g_strcmp0(subsystem, "net") == 0 ||
-					g_strcmp0(subsystem, "hsi") == 0)
 			add_modem(device);
 	} else if (g_str_equal(action, "remove") == TRUE) {
-		if (g_strcmp0(subsystem, "tty") == 0 ||
-				g_strcmp0(subsystem, "net") == 0 ||
-					g_strcmp0(subsystem, "hsi") == 0)
 			remove_modem(device);
 	}
 
-	DBG("subsystem %s finished", subsystem);
-
 done:
 	udev_device_unref(device);
 
-- 
2.9.3

[PATCH udev 04/15] udev: get udev property via lib function

[Jonas Bonn]

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

---
 plugins/udev.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index 196c528..7c090aa 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -55,34 +55,16 @@ static struct ofono_modem *find_modem(const char *devpath)
 	return NULL;
 }
 
-static const char *get_property(struct udev_device *device,
-				char const *property_name)
-{
-	struct udev_list_entry *entry;
-
-	entry = udev_device_get_properties_list_entry(device);
-	while (entry) {
-		const char *name = udev_list_entry_get_name(entry);
-
-		if (g_strcmp0(name, property_name) == 0)
-			return udev_list_entry_get_value(entry);
-
-		entry = udev_list_entry_get_next(entry);
-	}
-
-	return NULL;
-}
-
 static const char *get_driver(struct udev_device *udev_device)
 {
-	return get_property(udev_device, "OFONO_DRIVER");
+	return udev_device_get_property_value(udev_device, "OFONO_DRIVER");
 }
 
 static const char *get_serial(struct udev_device *udev_device)
 {
 	const char *serial;
 
-	serial = get_property(udev_device, "ID_SERIAL_SHORT");
+	serial = udev_device_get_property_value(udev_device, "ID_SERIAL_SHORT");
 
 	if (serial != NULL) {
 		unsigned int i, len = strlen(serial);
@@ -135,7 +117,7 @@ static void add_isi(struct ofono_modem *modem,
 	if (ofono_modem_get_string(modem, "Interface"))
 		return;
 
-	addr = get_property(udev_device, "OFONO_ISI_ADDRESS");
+	addr = udev_device_get_property_value(udev_device, "OFONO_ISI_ADDRESS");
 	if (addr != NULL)
 		ofono_modem_set_integer(modem, "Address", atoi(addr));
 
-- 
2.9.3

[PATCH udev 05/15] udev: remove unused modem property

[Jonas Bonn]

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

The property 'Registered' is not used anywhere.
---
 plugins/udev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index 7c090aa..dc07fc6 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -270,7 +270,6 @@ static void add_modem(struct udev_device *udev_device)
 			return;
 
 		ofono_modem_set_string(modem, "Path", devpath);
-		ofono_modem_set_integer(modem, "Registered", 0);
 
 		modem_list = g_slist_prepend(modem_list, modem);
 	}
-- 
2.9.3

[PATCH udev 06/15] udev: simplify add_modem

[Jonas Bonn]

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

Adding a modem boils down to finding the device in the hierarchy with
the OFONO_DRIVER property.  The original code special-cased the property
being on the device itself rather than on a parent device.  This patch
combines the two cases.
---
 plugins/udev.c | 72 ++++++++++++++++++++++++++--------------------------------
 1 file changed, 32 insertions(+), 40 deletions(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index dc07fc6..5d1080d 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -55,9 +55,27 @@ static struct ofono_modem *find_modem(const char *devpath)
 	return NULL;
 }
 
-static const char *get_driver(struct udev_device *udev_device)
+/*
+ * Here we try to find the "modem device".
+ *
+ * In this variant we identify the "modem device" as simply the device
+ * that has the OFONO_DRIVER property.  If the device node doesn't
+ * have this property itself, then we do a brute force search for it
+ * through the device hierarchy. 
+ *
+ */
+static struct udev_device* get_modem_device(struct udev_device *dev)
 {
-	return udev_device_get_property_value(udev_device, "OFONO_DRIVER");
+	const char* driver;
+
+	while (dev) {
+		driver = udev_device_get_property_value(dev, "OFONO_DRIVER");
+		if (driver)
+			return dev;
+		dev = udev_device_get_parent(dev);
+	}
+
+	return NULL; 
 }
 
 static const char *get_serial(struct udev_device *udev_device)
@@ -216,54 +234,29 @@ static void add_sim900(struct ofono_modem *modem,
 static void add_modem(struct udev_device *udev_device)
 {
 	struct ofono_modem *modem;
-	struct udev_device *parent;
 	const char *devpath, *curpath, *driver;
+	struct udev_device *mdev;
 
-	driver = get_driver(udev_device);
-	if (driver != NULL) {
-		devpath = udev_device_get_devpath(udev_device);
-		if (devpath == NULL)
-			return;
-
-		if(g_strcmp0(driver, "tc65") == 0)
-			driver = "cinterion";
-		if(g_strcmp0(driver, "ehs6") == 0)
-			driver = "cinterion";
-
-		modem = ofono_modem_create(NULL, driver);
-		if (modem == NULL)
-			return;
-
-		ofono_modem_set_string(modem, "Path", devpath);
-
-		modem_list = g_slist_prepend(modem_list, modem);
-
-		goto done;
+	mdev = get_modem_device(udev_device);
+	if (!mdev) {
+		DBG("Device has no OFONO_DRIVER property");
+		return;
 	}
 
-	parent = udev_device_get_parent(udev_device);
-	if (parent == NULL)
-		return;
+	driver = udev_device_get_property_value(mdev, "OFONO_DRIVER");
 
-	driver = get_driver(parent);
-	if (driver == NULL) {
-		parent = udev_device_get_parent(parent);
-		driver = get_driver(parent);
-		if (driver == NULL) {
-			parent = udev_device_get_parent(parent);
-			driver = get_driver(parent);
-			if (driver == NULL)
-				return;
-		}
-	}
+	if(g_strcmp0(driver, "tc65") == 0)
+		driver = "cinterion";
+	if(g_strcmp0(driver, "ehs6") == 0)
+		driver = "cinterion";
 
-	devpath = udev_device_get_devpath(parent);
+	devpath = udev_device_get_devpath(mdev);
 	if (devpath == NULL)
 		return;
 
 	modem = find_modem(devpath);
 	if (modem == NULL) {
-		const char *serial = get_serial(parent);
+		const char *serial = get_serial(mdev);
 
 		modem = ofono_modem_create(serial, driver);
 		if (modem == NULL)
@@ -274,7 +267,6 @@ static void add_modem(struct udev_device *udev_device)
 		modem_list = g_slist_prepend(modem_list, modem);
 	}
 
-done:
 	curpath = udev_device_get_devpath(udev_device);
 	if (curpath == NULL)
 		return;
-- 
2.9.3

[PATCH udev 07/15] udev: add common modem registration code

[Jonas Bonn]

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

Many of these drivers do exactly the same setup when registering
the modem.  Consolidate these setup functions into common code.
---
 plugins/udev.c | 49 +++++--------------------------------------------
 1 file changed, 5 insertions(+), 44 deletions(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index 5d1080d..05945a0 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -154,7 +154,7 @@ static void add_isi(struct ofono_modem *modem,
 	ofono_modem_register(modem);
 }
 
-static void add_calypso(struct ofono_modem *modem,
+static void __add_common(struct ofono_modem *modem,
 					struct udev_device *udev_device)
 {
 	const char *devnode;
@@ -192,45 +192,6 @@ static void add_wavecom(struct ofono_modem *modem,
 	ofono_modem_register(modem);
 }
 
-static void add_cinterion(struct ofono_modem *modem,
-			struct udev_device *udev_device)
-{
-	const char *devnode;
-
-	DBG("modem %p", modem);
-
-	devnode = udev_device_get_devnode(udev_device);
-	ofono_modem_set_string(modem, "Device", devnode);
-
-	ofono_modem_register(modem);
-}
-
-static void add_nokiacdma(struct ofono_modem *modem,
-					struct udev_device *udev_device)
-{
-	const char *devnode;
-
-	DBG("modem %p", modem);
-
-	devnode = udev_device_get_devnode(udev_device);
-	ofono_modem_set_string(modem, "Device", devnode);
-
-	ofono_modem_register(modem);
-}
-
-static void add_sim900(struct ofono_modem *modem,
-					struct udev_device *udev_device)
-{
-	const char *devnode;
-
-	DBG("modem %p", modem);
-
-	devnode = udev_device_get_devnode(udev_device);
-	ofono_modem_set_string(modem, "Device", devnode);
-
-	ofono_modem_register(modem);
-}
-
 static void add_modem(struct udev_device *udev_device)
 {
 	struct ofono_modem *modem;
@@ -282,13 +243,13 @@ static void add_modem(struct udev_device *udev_device)
 	else if (g_strcmp0(driver, "n900") == 0)
 		add_isi(modem, udev_device);
 	else if (g_strcmp0(driver, "calypso") == 0)
-		add_calypso(modem, udev_device);
+		__add_common(modem, udev_device);
 	else if (g_strcmp0(driver, "cinterion") == 0)
-		add_cinterion(modem, udev_device);
+		__add_common(modem, udev_device);
 	else if (g_strcmp0(driver, "nokiacdma") == 0)
-		add_nokiacdma(modem, udev_device);
+		__add_common(modem, udev_device);
 	else if (g_strcmp0(driver, "sim900") == 0)
-		add_sim900(modem, udev_device);
+		__add_common(modem, udev_device);
 	else if (g_strcmp0(driver, "wavecom") == 0)
 		add_wavecom(modem, udev_device);
 }
-- 
2.9.3

[PATCH udev 08/15] udev: simplify ifx modem registration

[Jonas Bonn]

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

---
 plugins/udev.c | 48 ++++++++++++++++++------------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index 05945a0..687aa66 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -96,10 +96,9 @@ static const char *get_serial(struct udev_device *udev_device)
 	return serial;
 }
 
-static void add_ifx(struct ofono_modem *modem,
+static void __add_common(struct ofono_modem *modem,
 					struct udev_device *udev_device)
 {
-	struct udev_list_entry *entry;
 	const char *devnode;
 
 	DBG("modem %p", modem);
@@ -107,24 +106,26 @@ static void add_ifx(struct ofono_modem *modem,
 	devnode = udev_device_get_devnode(udev_device);
 	ofono_modem_set_string(modem, "Device", devnode);
 
-	entry = udev_device_get_properties_list_entry(udev_device);
-	while (entry) {
-		const char *name = udev_list_entry_get_name(entry);
-		const char *value = udev_list_entry_get_value(entry);
-
-		if (g_str_equal(name, "OFONO_IFX_LDISC") == TRUE)
-			ofono_modem_set_string(modem, "LineDiscipline", value);
-		else if (g_str_equal(name, "OFONO_IFX_AUDIO") == TRUE)
-			ofono_modem_set_string(modem, "AudioSetting", value);
-		else if (g_str_equal(name, "OFONO_IFX_LOOPBACK") == TRUE)
-			ofono_modem_set_string(modem, "AudioLoopback", value);
-
-		entry = udev_list_entry_get_next(entry);
-	}
-
 	ofono_modem_register(modem);
 }
 
+static void add_ifx(struct ofono_modem *modem, struct udev_device *dev)
+{
+	const char *value;
+
+	value = udev_device_get_property_value(dev, "OFONO_IFX_LDISC");
+	if (value)
+		ofono_modem_set_string(modem, "LineDiscipline", value);
+	value = udev_device_get_property_value(dev, "OFONO_IFX_AUDIO");
+	if (value)
+		ofono_modem_set_string(modem, "AudioSetting", value);
+	value = udev_device_get_property_value(dev, "OFONO_IFX_LOOPBACK");
+	if (value)
+		ofono_modem_set_string(modem, "AudioLoopback", value);
+
+	__add_common(modem, dev);
+}
+
 static void add_isi(struct ofono_modem *modem,
 					struct udev_device *udev_device)
 {
@@ -154,19 +155,6 @@ static void add_isi(struct ofono_modem *modem,
 	ofono_modem_register(modem);
 }
 
-static void __add_common(struct ofono_modem *modem,
-					struct udev_device *udev_device)
-{
-	const char *devnode;
-
-	DBG("modem %p", modem);
-
-	devnode = udev_device_get_devnode(udev_device);
-	ofono_modem_set_string(modem, "Device", devnode);
-
-	ofono_modem_register(modem);
-}
-
 static void add_wavecom(struct ofono_modem *modem,
 					struct udev_device *udev_device)
 {
-- 
2.9.3

[PATCH udev 09/15] udev: simplify wavecom modem registration

[Jonas Bonn]

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

---
 plugins/udev.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index 687aa66..13cef32 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -155,29 +155,15 @@ static void add_isi(struct ofono_modem *modem,
 	ofono_modem_register(modem);
 }
 
-static void add_wavecom(struct ofono_modem *modem,
-					struct udev_device *udev_device)
+static void add_wavecom(struct ofono_modem *modem, struct udev_device *dev)
 {
-	const char *devnode;
-	struct udev_list_entry *entry;
-
-	DBG("modem %p", modem);
-
-	devnode = udev_device_get_devnode(udev_device);
-	ofono_modem_set_string(modem, "Device", devnode);
-
-	entry = udev_device_get_properties_list_entry(udev_device);
-	while (entry) {
-		const char *name = udev_list_entry_get_name(entry);
-		const char *value = udev_list_entry_get_value(entry);
-
-		if (g_str_equal(name, "OFONO_WAVECOM_MODEL") == TRUE)
-			ofono_modem_set_string(modem, "Model", value);
+	const char *value;
 
-		entry = udev_list_entry_get_next(entry);
-	}
+	value = udev_device_get_property_value(dev, "OFONO_WAVECOM_MODEL");
+	if (value)
+		ofono_modem_set_string(modem, "Model", value);
 
-	ofono_modem_register(modem);
+	__add_common(modem, dev);
 }
 
 static void add_modem(struct udev_device *udev_device)
-- 
2.9.3

[PATCH udev 10/15] udev: remove extraneous subsystem check

[Jonas Bonn]

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

The enumeration construct is already set up with matches for these
subsystems.
---
 plugins/udev.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index 13cef32..02cc67a 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -292,14 +292,7 @@ static void enumerate_devices(struct udev *context)
 
 		device = udev_device_new_from_syspath(context, syspath);
 		if (device != NULL) {
-			const char *subsystem;
-
-			subsystem = udev_device_get_subsystem(device);
-
-			if (g_strcmp0(subsystem, "tty") == 0 ||
-					g_strcmp0(subsystem, "net") == 0 ||
-					g_strcmp0(subsystem, "hsi") == 0)
-				add_modem(device);
+			add_modem(device);
 
 			udev_device_unref(device);
 		}
-- 
2.9.3

[PATCH udev 11/15] udevng: add legacy device handling functions

[Jonas Bonn]

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

This adds, but does not hook up, functionality to handle devices
that are currently handled by the 'legacy' udev.c module.

- legacy device is added by calling the add_legacy_device() function.
- the function sets up a modem_info and device_info structure for
  the legacy device
- the device driver is set to "legacy" and the real driver type is
  saved in the device_info structure
- the extra udev information from the add_*() functions of the old
  udev.c module is added to unused fields of the device_info structure
- the "legacy" driver is set-up to use setup_legacy
- setup_legacy extracts the device_info information and applies it
  to the modem in the same way that the add_*() funtions did
- the modem driver is switched to the real driver

...and off we go!
---
 plugins/udevng.c | 255 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 255 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index ce2bc28..4e464c2 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -830,6 +830,52 @@ static gboolean setup_quectel(struct modem_info *modem)
 	return TRUE;
 }
 
+static gboolean setup_legacy(struct modem_info *modem)
+{
+	const char* mdm = NULL;
+	const char* driver = NULL;
+	GSList *list;
+
+	/* These legacy device normally only have one device... */
+	for (list = modem->devices; list; list = list->next) {
+		struct device_info *info = list->data;
+
+		if (!g_strcmp0(modem->driver, "ifx")) {
+			ofono_modem_set_string(modem->modem,
+					"LineDiscipline", info->number);
+			ofono_modem_set_string(modem->modem,
+					"AudioSetting", info->label);
+			ofono_modem_set_string(modem->modem,
+					"AudioLoopback", info->interface);
+		} else if (!g_strcmp0(modem->driver, "8500")) {
+		} else if (!g_strcmp0(modem->driver, "n900")) {
+			if (!info->interface)
+				continue;
+
+			ofono_modem_set_integer(modem->modem,
+				"Address", atoi(info->number));
+			ofono_modem_set_string(modem->modem,
+					"Interface", info->interface);
+
+		} else if (!g_strcmp0(modem->driver, "wavecom")) {
+			ofono_modem_set_string(modem->modem,
+					"Model", info->number);
+		}
+
+		mdm = info->devnode;
+		driver = info->sysattr;
+	}
+
+	if (!mdm)
+		return FALSE;
+
+	ofono_modem_set_driver(modem->modem, driver);
+
+	ofono_modem_set_string(modem->modem, "Device", mdm);	
+
+	return TRUE;
+}
+
 static gboolean setup_ublox(struct modem_info *modem)
 {
 	const char *aux = NULL, *mdm = NULL, *net = NULL;
@@ -959,6 +1005,7 @@ static struct {
 	{ "quectel",	setup_quectel	},
 	{ "ublox",	setup_ublox	},
 	{ "gemalto",	setup_gemalto	},
+	{ "legacy",	setup_legacy	},
 	{ }
 };
 
@@ -1050,6 +1097,214 @@ static gint compare_device(gconstpointer a, gconstpointer b)
 	return g_strcmp0(info1->number, info2->number);
 }
 
+/*
+ * These add_*_info functions are part of the add_legacy_device()
+ * functionality.  Don't re-use them in anything new; ideally they will
+ * be able to go away when the hardware is deemed unavailable.
+ */
+static void add_isi_info(struct device_info* info,
+					struct udev_device *udev_device)
+{
+	const char *value;
+	const char* type;
+	const char* ifname;
+
+	/* Set info->interface if we want this device to be a modem;
+	 * leave it as NULL to ignore it
+	 */
+	if (g_strcmp0(udev_device_get_subsystem(udev_device), "net") != 0)
+		return;
+
+	type = udev_device_get_sysattr_value(udev_device, "type");
+	if (g_strcmp0(type, "820") != 0)
+		return;
+
+	/* OK, we want this device to be a modem */
+	ifname = udev_device_get_sysname(udev_device);
+	if (ifname)
+		info->interface = g_strdup(ifname);
+
+	value = udev_device_get_property_value(udev_device,
+						"OFONO_ISI_ADDRESS");
+	if (value)
+		info->number = g_strdup(value);
+}
+
+
+static void add_wavecom_info(struct device_info *info, struct udev_device *dev)
+{
+	const char *value;
+
+	value = udev_device_get_property_value(dev, "OFONO_WAVECOM_MODEL");
+	if (value)
+		info->number = g_strdup(value);
+}
+
+static void add_ifx_info(struct device_info* info, struct udev_device *dev)
+{
+	const char *value;
+
+	value = udev_device_get_property_value(dev, "OFONO_IFX_LDISC");
+	if (value)
+		info->number = g_strdup(value);
+	value = udev_device_get_property_value(dev, "OFONO_IFX_AUDIO");
+	if (value)
+		info->label = g_strdup(value);
+	value = udev_device_get_property_value(dev, "OFONO_IFX_LOOPBACK");
+	if (value)
+		info->interface = g_strdup(value);
+}
+
+/*
+ * Here we try to find the "modem device".
+ *
+ * In this variant we identify the "modem device" as simply the device
+ * that has the OFONO_DRIVER property.  If the device node doesn't
+ * have this property itself, then we do a brute force search for it
+ * through the device hierarchy. 
+ *
+ */
+static struct udev_device* get_legacy_modem_device(struct udev_device *dev)
+{
+	const char* driver;
+
+	while (dev) {
+		driver = udev_device_get_property_value(dev, "OFONO_DRIVER");
+		if (driver)
+			return dev;
+		dev = udev_device_get_parent(dev);
+	}
+
+	return NULL; 
+}
+
+
+/*
+ * Add 'legacy' device
+ *
+ * The term legacy is a bit misleading, but this adds devices according
+ * to the original ofono model.
+ *
+ * - We cannot assume that these are USB devices
+ * - The modem consists of only a single interface
+ * - The device must have an OFONO_DRIVER property from udev
+ */
+static void add_legacy_device(struct udev_device *dev)
+{
+	const char *syspath, *devpath, *devname, *devnode;
+	struct udev_device *usb_device;
+	struct modem_info *modem;
+	struct device_info *info;
+	const char* vendor = NULL;
+	const char* model = NULL;
+	const char *subsystem;
+	struct udev_device* mdev;
+	const char* driver;
+
+	mdev = get_legacy_modem_device(dev);
+	if (!mdev) {
+		DBG("Device is missing required OFONO_DRIVER property");
+		return;
+	}
+
+	driver = udev_device_get_property_value(mdev, "OFONO_DRIVER");
+	/* Check that this really is a legacy driver */
+	if (!g_strcmp0(driver, "ifx")) goto start;
+	if (!g_strcmp0(driver, "u8500")) goto start;
+	if (!g_strcmp0(driver, "n900")) goto start;
+	if (!g_strcmp0(driver, "calypso")) goto start;
+	if (!g_strcmp0(driver, "cinterion")) goto start;
+	if (!g_strcmp0(driver, "nokiacdma")) goto start;
+	if (!g_strcmp0(driver, "sim900")) goto start;
+	if (!g_strcmp0(driver, "wavecom")) goto start;
+	if (!g_strcmp0(driver, "tc65")) {
+		driver= "cinterion";
+		goto start;
+	}
+	if (!g_strcmp0(driver, "ehs6")) {
+		driver= "cinterion";
+		goto start;
+	}
+
+	/* Driver isn't something we recognize... */
+	return;
+start:
+
+	syspath = udev_device_get_syspath(mdev);
+	devname = udev_device_get_devnode(mdev);
+	devpath = udev_device_get_devpath(mdev);
+
+	devnode = udev_device_get_devnode(dev);
+
+	if (!syspath || !devname || !devpath || !devnode)
+		return;
+
+	/* Maybe this is a USB device... in that case we can grab the
+	 * vendor and model information
+	 */
+	usb_device = udev_device_get_parent_with_subsystem_devtype(mdev,
+							"usb", "usb_device");
+	if (usb_device) {
+		vendor = udev_device_get_property_value(usb_device,
+							"ID_VENDOR_ID");
+		model = udev_device_get_property_value(usb_device,
+							"ID_MODEL_ID");
+	}
+
+	modem = g_hash_table_lookup(modem_list, syspath);
+	if (modem == NULL) {
+		modem = g_try_new0(struct modem_info, 1);
+		if (modem == NULL)
+			return;
+
+		modem->syspath = g_strdup(syspath);
+		modem->devname = g_strdup(devname);
+		modem->driver = g_strdup("legacy");
+		modem->vendor = g_strdup(vendor);
+		modem->model = g_strdup(model);
+
+		g_hash_table_replace(modem_list, modem->syspath, modem);
+	}
+
+	subsystem = udev_device_get_subsystem(dev);
+
+	DBG("%s", syspath);
+	DBG("%s", devpath);
+	DBG("%s (%s)", devnode, driver);
+
+	info = g_try_new0(struct device_info, 1);
+	if (info == NULL)
+		return;
+
+	info->devpath = g_strdup(devpath);
+	info->devnode = g_strdup(devnode);
+	info->subsystem = g_strdup(subsystem);
+
+	/* Here we begin the abuse... we want to switch the modem driver
+	 * from 'legacy' to it's real driver in the setup function, so
+	 * we store the driver name here in the otherwise unused sysattr
+	 * field
+	 */
+	info->sysattr = g_strdup(driver);
+
+	/* Here we abuse some unused device_info attributes to hold
+	 * extra udev properties... this is ugly but this is all
+	 * legacy stuff that hopefully disappears someday anyway.
+	 */
+	if (g_strcmp0(driver, "ifx")) {
+		add_ifx_info(info, dev);
+	} else if (g_strcmp0(driver, "u8500")) {
+		add_isi_info(info, dev);
+	} else if (g_strcmp0(driver, "n900")) {
+		add_isi_info(info, dev);
+	} else if (g_strcmp0(driver, "wavecom")) {
+		add_wavecom_info(info, dev);
+	}
+
+	modem->devices = g_slist_insert_sorted(modem->devices, info,
+							compare_device);
+}
+
 static void add_device(const char *syspath, const char *devname,
 			const char *driver, const char *vendor,
 			const char *model, struct udev_device *device)
-- 
2.9.3

[PATCH udev 12/15] udevng: match on the hsi subsystem for legacy devices

[Jonas Bonn]

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

---
 plugins/udevng.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 4e464c2..640a221 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1595,6 +1595,7 @@ static void enumerate_devices(struct udev *context)
 	udev_enumerate_add_match_subsystem(enumerate, "usb");
 	udev_enumerate_add_match_subsystem(enumerate, "usbmisc");
 	udev_enumerate_add_match_subsystem(enumerate, "net");
+	udev_enumerate_add_match_subsystem(enumerate, "hsi");
 
 	udev_enumerate_scan_devices(enumerate);
 
@@ -1719,6 +1720,7 @@ static int detect_init(void)
 	udev_monitor_filter_add_match_subsystem_devtype(udev_mon,
 							"usbmisc", NULL);
 	udev_monitor_filter_add_match_subsystem_devtype(udev_mon, "net", NULL);
+	udev_monitor_filter_add_match_subsystem_devtype(udev_mon, "hsi", NULL);
 
 	udev_monitor_filter_update(udev_mon);
 
-- 
2.9.3

[PATCH udev 13/15] udevng: hook up legacy devices

[Jonas Bonn]

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

...and disable old udev code by shorting it out in it's init() function.
---
 plugins/udev.c   | 2 ++
 plugins/udevng.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/plugins/udev.c b/plugins/udev.c
index 02cc67a..38ddeea 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -366,6 +366,8 @@ static void udev_start(void)
 
 static int udev_init(void)
 {
+	return 0;
+
 	devpath_list = g_hash_table_new_full(g_str_hash, g_str_equal,
 						g_free, g_free);
 	if (devpath_list == NULL) {
diff --git a/plugins/udevng.c b/plugins/udevng.c
index 640a221..8721447 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1607,6 +1607,7 @@ static void enumerate_devices(struct udev *context)
 		device = udev_device_new_from_syspath(context, syspath);
 		if (device != NULL) {
 			check_device(device);
+			add_legacy_device(device);
 			udev_device_unref(device);
 		}
 
@@ -1659,6 +1660,7 @@ static gboolean udev_event(GIOChannel *channel, GIOCondition cond,
 			g_source_remove(udev_delay);
 
 		check_device(device);
+		add_legacy_device(device);
 
 		udev_delay = g_timeout_add_seconds(1, check_modem_list, NULL);
 	} else if (g_str_equal(action, "remove") == TRUE)
-- 
2.9.3

[PATCH udev 14/15] udevng: get properties from interface

[Jonas Bonn]

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

Device properties are generally on the device, on the USB interface
descriptor, or the on the USB device descriptor.
---
 plugins/udevng.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 8721447..98b4f98 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1365,6 +1365,9 @@ static void add_device(const char *syspath, const char *devname,
 	}
 
 	label = udev_device_get_property_value(device, "OFONO_LABEL");
+	if (!label) {
+		label = udev_device_get_property_value(intf, "OFONO_LABEL");
+	}
 	subsystem = udev_device_get_subsystem(device);
 
 	if (modem->sysattr != NULL)
@@ -1464,6 +1467,7 @@ static struct {
 static void check_usb_device(struct udev_device *device)
 {
 	struct udev_device *usb_device;
+	struct udev_device *intf;
 	const char *syspath, *devname, *driver;
 	const char *vendor = NULL, *model = NULL;
 
@@ -1484,6 +1488,13 @@ static void check_usb_device(struct udev_device *device)
 	model = udev_device_get_property_value(usb_device, "ID_MODEL_ID");
 
 	driver = udev_device_get_property_value(usb_device, "OFONO_DRIVER");
+	if (!driver) {
+		intf = udev_device_get_parent_with_subsystem_devtype(device,
+						"usb", "usb_interface");
+		if (intf)
+			driver = udev_device_get_property_value(
+							intf, "OFONO_DRIVER");
+	}
 	if (driver == NULL) {
 		const char *drv;
 		unsigned int i;
-- 
2.9.3

[PATCH udev 15/15] plugins: remove udev module

[Jonas Bonn]

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

This functionality has been moved into the udevng module and was
already disabled in an earlier patch.
---
 Makefile.am    |   2 -
 plugins/udev.c | 434 ---------------------------------------------------------
 2 files changed, 436 deletions(-)
 delete mode 100644 plugins/udev.c

diff --git a/Makefile.am b/Makefile.am
index d9f9b08..b232f97 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -106,8 +106,6 @@ gril_sources = gril/gril.h gril/gril.c \
 btio_sources = btio/btio.h btio/btio.c
 
 if UDEV
-builtin_modules += udev
-builtin_sources += plugins/udev.c
 builtin_cflags += @UDEV_CFLAGS@
 builtin_libadd += @UDEV_LIBS@
 
diff --git a/plugins/udev.c b/plugins/udev.c
deleted file mode 100644
index 38ddeea..0000000
--- a/plugins/udev.c
+++ /dev/null
@@ -1,434 +0,0 @@
-/*
- *
- *  oFono - Open Source Telephony
- *
- *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License version 2 as
- *  published by the Free Software Foundation.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- *
- */
-
-#ifdef HAVE_CONFIG_H
-#include <config.h>
-#endif
-
-#include <errno.h>
-#include <ctype.h>
-#include <stdlib.h>
-
-#include <libudev.h>
-
-#include <glib.h>
-#include <string.h>
-
-#define OFONO_API_SUBJECT_TO_CHANGE
-#include <ofono/plugin.h>
-#include <ofono/modem.h>
-#include <ofono/log.h>
-
-static GSList *modem_list = NULL;
-static GHashTable *devpath_list = NULL;
-
-static struct ofono_modem *find_modem(const char *devpath)
-{
-	GSList *list;
-
-	for (list = modem_list; list; list = list->next) {
-		struct ofono_modem *modem = list->data;
-		const char *path = ofono_modem_get_string(modem, "Path");
-
-		if (g_strcmp0(devpath, path) == 0)
-			return modem;
-	}
-
-	return NULL;
-}
-
-/*
- * Here we try to find the "modem device".
- *
- * In this variant we identify the "modem device" as simply the device
- * that has the OFONO_DRIVER property.  If the device node doesn't
- * have this property itself, then we do a brute force search for it
- * through the device hierarchy. 
- *
- */
-static struct udev_device* get_modem_device(struct udev_device *dev)
-{
-	const char* driver;
-
-	while (dev) {
-		driver = udev_device_get_property_value(dev, "OFONO_DRIVER");
-		if (driver)
-			return dev;
-		dev = udev_device_get_parent(dev);
-	}
-
-	return NULL; 
-}
-
-static const char *get_serial(struct udev_device *udev_device)
-{
-	const char *serial;
-
-	serial = udev_device_get_property_value(udev_device, "ID_SERIAL_SHORT");
-
-	if (serial != NULL) {
-		unsigned int i, len = strlen(serial);
-
-		for (i = 0; i < len; i++) {
-			if (!g_ascii_isalnum(serial[i]))
-				return NULL;
-		}
-	}
-
-	return serial;
-}
-
-static void __add_common(struct ofono_modem *modem,
-					struct udev_device *udev_device)
-{
-	const char *devnode;
-
-	DBG("modem %p", modem);
-
-	devnode = udev_device_get_devnode(udev_device);
-	ofono_modem_set_string(modem, "Device", devnode);
-
-	ofono_modem_register(modem);
-}
-
-static void add_ifx(struct ofono_modem *modem, struct udev_device *dev)
-{
-	const char *value;
-
-	value = udev_device_get_property_value(dev, "OFONO_IFX_LDISC");
-	if (value)
-		ofono_modem_set_string(modem, "LineDiscipline", value);
-	value = udev_device_get_property_value(dev, "OFONO_IFX_AUDIO");
-	if (value)
-		ofono_modem_set_string(modem, "AudioSetting", value);
-	value = udev_device_get_property_value(dev, "OFONO_IFX_LOOPBACK");
-	if (value)
-		ofono_modem_set_string(modem, "AudioLoopback", value);
-
-	__add_common(modem, dev);
-}
-
-static void add_isi(struct ofono_modem *modem,
-					struct udev_device *udev_device)
-{
-	const char *ifname, *type, *addr;
-
-	DBG("modem %p", modem);
-
-	if (ofono_modem_get_string(modem, "Interface"))
-		return;
-
-	addr = udev_device_get_property_value(udev_device, "OFONO_ISI_ADDRESS");
-	if (addr != NULL)
-		ofono_modem_set_integer(modem, "Address", atoi(addr));
-
-	if (g_strcmp0(udev_device_get_subsystem(udev_device), "net") != 0)
-		return;
-
-	type = udev_device_get_sysattr_value(udev_device, "type");
-	if (g_strcmp0(type, "820") != 0)
-		return;
-
-	ifname = udev_device_get_sysname(udev_device);
-	ofono_modem_set_string(modem, "Interface", ifname);
-
-	DBG("interface %s", ifname);
-
-	ofono_modem_register(modem);
-}
-
-static void add_wavecom(struct ofono_modem *modem, struct udev_device *dev)
-{
-	const char *value;
-
-	value = udev_device_get_property_value(dev, "OFONO_WAVECOM_MODEL");
-	if (value)
-		ofono_modem_set_string(modem, "Model", value);
-
-	__add_common(modem, dev);
-}
-
-static void add_modem(struct udev_device *udev_device)
-{
-	struct ofono_modem *modem;
-	const char *devpath, *curpath, *driver;
-	struct udev_device *mdev;
-
-	mdev = get_modem_device(udev_device);
-	if (!mdev) {
-		DBG("Device has no OFONO_DRIVER property");
-		return;
-	}
-
-	driver = udev_device_get_property_value(mdev, "OFONO_DRIVER");
-
-	if(g_strcmp0(driver, "tc65") == 0)
-		driver = "cinterion";
-	if(g_strcmp0(driver, "ehs6") == 0)
-		driver = "cinterion";
-
-	devpath = udev_device_get_devpath(mdev);
-	if (devpath == NULL)
-		return;
-
-	modem = find_modem(devpath);
-	if (modem == NULL) {
-		const char *serial = get_serial(mdev);
-
-		modem = ofono_modem_create(serial, driver);
-		if (modem == NULL)
-			return;
-
-		ofono_modem_set_string(modem, "Path", devpath);
-
-		modem_list = g_slist_prepend(modem_list, modem);
-	}
-
-	curpath = udev_device_get_devpath(udev_device);
-	if (curpath == NULL)
-		return;
-
-	DBG("%s (%s)", curpath, driver);
-
-	g_hash_table_insert(devpath_list, g_strdup(curpath), g_strdup(devpath));
-
-	if (g_strcmp0(driver, "ifx") == 0)
-		add_ifx(modem, udev_device);
-	else if (g_strcmp0(driver, "u8500") == 0)
-		add_isi(modem, udev_device);
-	else if (g_strcmp0(driver, "n900") == 0)
-		add_isi(modem, udev_device);
-	else if (g_strcmp0(driver, "calypso") == 0)
-		__add_common(modem, udev_device);
-	else if (g_strcmp0(driver, "cinterion") == 0)
-		__add_common(modem, udev_device);
-	else if (g_strcmp0(driver, "nokiacdma") == 0)
-		__add_common(modem, udev_device);
-	else if (g_strcmp0(driver, "sim900") == 0)
-		__add_common(modem, udev_device);
-	else if (g_strcmp0(driver, "wavecom") == 0)
-		add_wavecom(modem, udev_device);
-}
-
-static gboolean devpath_remove(gpointer key, gpointer value, gpointer user_data)
-{
-	const char *path = value;
-	const char *devpath = user_data;
-
-	DBG("%s -> %s", path, devpath);
-
-	return g_str_equal(path, devpath);
-}
-
-static void remove_modem(struct udev_device *udev_device)
-{
-	struct ofono_modem *modem;
-	const char *curpath = udev_device_get_devpath(udev_device);
-	char *devpath, *remove;
-
-	if (curpath == NULL)
-		return;
-
-	DBG("%s", curpath);
-
-	devpath = g_hash_table_lookup(devpath_list, curpath);
-	if (devpath == NULL)
-		return;
-
-	modem = find_modem(devpath);
-	if (modem == NULL)
-		return;
-
-	modem_list = g_slist_remove(modem_list, modem);
-
-	ofono_modem_remove(modem);
-
-	DBG("%s", devpath);
-
-	remove = g_strdup(devpath);
-
-	g_hash_table_foreach_remove(devpath_list, devpath_remove, remove);
-
-	g_free(remove);
-}
-
-static void enumerate_devices(struct udev *context)
-{
-	struct udev_enumerate *enumerate;
-	struct udev_list_entry *entry;
-
-	enumerate = udev_enumerate_new(context);
-	if (enumerate == NULL)
-		return;
-
-	udev_enumerate_add_match_subsystem(enumerate, "tty");
-	udev_enumerate_add_match_subsystem(enumerate, "net");
-	udev_enumerate_add_match_subsystem(enumerate, "hsi");
-
-	udev_enumerate_scan_devices(enumerate);
-
-	entry = udev_enumerate_get_list_entry(enumerate);
-	while (entry) {
-		const char *syspath = udev_list_entry_get_name(entry);
-		struct udev_device *device;
-
-		device = udev_device_new_from_syspath(context, syspath);
-		if (device != NULL) {
-			add_modem(device);
-
-			udev_device_unref(device);
-		}
-
-		entry = udev_list_entry_get_next(entry);
-	}
-
-	udev_enumerate_unref(enumerate);
-}
-
-static struct udev *udev_ctx;
-static struct udev_monitor *udev_mon;
-static guint udev_watch = 0;
-
-static gboolean udev_event(GIOChannel *channel, GIOCondition cond,
-							gpointer user_data)
-{
-	struct udev_device *device;
-	const char *action;
-
-	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
-		ofono_warn("Error with udev monitor channel");
-		udev_watch = 0;
-		return FALSE;
-	}
-
-	device = udev_monitor_receive_device(udev_mon);
-	if (device == NULL)
-		return TRUE;
-
-	action = udev_device_get_action(device);
-	if (action == NULL)
-		goto done;
-
-	if (g_str_equal(action, "add") == TRUE) {
-			add_modem(device);
-	} else if (g_str_equal(action, "remove") == TRUE) {
-			remove_modem(device);
-	}
-
-done:
-	udev_device_unref(device);
-
-	return TRUE;
-}
-
-static void udev_start(void)
-{
-	GIOChannel *channel;
-	int fd;
-
-	if (udev_monitor_enable_receiving(udev_mon) < 0) {
-		ofono_error("Failed to enable udev monitor");
-		return;
-	}
-
-	enumerate_devices(udev_ctx);
-
-	fd = udev_monitor_get_fd(udev_mon);
-
-	channel = g_io_channel_unix_new(fd);
-	if (channel == NULL)
-		return;
-
-	udev_watch = g_io_add_watch(channel,
-				G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
-							udev_event, NULL);
-
-	g_io_channel_unref(channel);
-}
-
-static int udev_init(void)
-{
-	return 0;
-
-	devpath_list = g_hash_table_new_full(g_str_hash, g_str_equal,
-						g_free, g_free);
-	if (devpath_list == NULL) {
-		ofono_error("Failed to create udev path list");
-		return -ENOMEM;
-	}
-
-	udev_ctx = udev_new();
-	if (udev_ctx == NULL) {
-		ofono_error("Failed to create udev context");
-		g_hash_table_destroy(devpath_list);
-		return -EIO;
-	}
-
-	udev_mon = udev_monitor_new_from_netlink(udev_ctx, "udev");
-	if (udev_mon == NULL) {
-		ofono_error("Failed to create udev monitor");
-		g_hash_table_destroy(devpath_list);
-		udev_unref(udev_ctx);
-		udev_ctx = NULL;
-		return -EIO;
-	}
-
-	udev_monitor_filter_add_match_subsystem_devtype(udev_mon, "tty", NULL);
-	udev_monitor_filter_add_match_subsystem_devtype(udev_mon, "net", NULL);
-	udev_monitor_filter_add_match_subsystem_devtype(udev_mon, "hsi", NULL);
-
-	udev_monitor_filter_update(udev_mon);
-
-	udev_start();
-
-	return 0;
-}
-
-static void udev_exit(void)
-{
-	GSList *list;
-
-	if (udev_watch > 0)
-		g_source_remove(udev_watch);
-
-	for (list = modem_list; list; list = list->next) {
-		struct ofono_modem *modem = list->data;
-
-		ofono_modem_remove(modem);
-	}
-
-	g_slist_free(modem_list);
-	modem_list = NULL;
-
-	g_hash_table_destroy(devpath_list);
-	devpath_list = NULL;
-
-	if (udev_ctx == NULL)
-		return;
-
-	udev_monitor_filter_remove(udev_mon);
-
-	udev_monitor_unref(udev_mon);
-	udev_unref(udev_ctx);
-}
-
-OFONO_PLUGIN_DEFINE(udev, "udev hardware detection", VERSION,
-			OFONO_PLUGIN_PRIORITY_DEFAULT, udev_init, udev_exit)
-- 
2.9.3

Re: [PATCH udev 01/15] udevng: simplify logic in check_usb_device

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:57 AM, Jonas Bonn wrote:
> This patch simplifies and cleans up the check_usb_device function a bit
> by doing the two following (slightly intertwined) things:
>
> 1)  The parent "usb_device" is searched for early in this function and this
> device will always have the ID_VENDOR_ID and ID_MODEL_ID properties.
> As such, we can get them from this device and thereby be certain that
> we _always_ have them available.
>
> 2)  The logic of iterating the vendor_list table is cleaned up.  It's
> easier to follow and won't be any less efficient.
> ---
>  plugins/udevng.c | 37 ++++++++++++-------------------------
>  1 file changed, 12 insertions(+), 25 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH udev 02/15] ofono.rules: remove 'change' action

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:57 AM, Jonas Bonn wrote:
> The code doesn't do anything with this action so don't bother setting
> extra device properties for it.
> ---
>  plugins/ofono.rules | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH udev 03/15] udev: remove extraneous subsystem check

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:57 AM, Jonas Bonn wrote:
> The udev-monitor already guarantees that only devices with these
> subsystems will be returned so we don't need to check again.
> ---
>  plugins/udev.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/plugins/udev.c b/plugins/udev.c
> index 3c90e40..196c528 100644
> --- a/plugins/udev.c
> +++ b/plugins/udev.c
> @@ -410,7 +410,7 @@ static gboolean udev_event(GIOChannel *channel, GIOCondition cond,
>  							gpointer user_data)
>  {
>  	struct udev_device *device;
> -	const char *subsystem, *action;
> +	const char *action;
>
>  	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
>  		ofono_warn("Error with udev monitor channel");
> @@ -422,30 +422,16 @@ static gboolean udev_event(GIOChannel *channel, GIOCondition cond,
>  	if (device == NULL)
>  		return TRUE;
>
> -	subsystem = udev_device_get_subsystem(device);
> -	if (subsystem == NULL)
> -		goto done;
> -
>  	action = udev_device_get_action(device);
>  	if (action == NULL)
>  		goto done;
>
> -	DBG("subsystem %s %s", subsystem, action);
> -
>  	if (g_str_equal(action, "add") == TRUE) {
> -		if (g_strcmp0(subsystem, "tty") == 0 ||
> -				g_strcmp0(subsystem, "net") == 0 ||
> -					g_strcmp0(subsystem, "hsi") == 0)
>  			add_modem(device);
>  	} else if (g_str_equal(action, "remove") == TRUE) {
> -		if (g_strcmp0(subsystem, "tty") == 0 ||
> -				g_strcmp0(subsystem, "net") == 0 ||
> -					g_strcmp0(subsystem, "hsi") == 0)
>  			remove_modem(device);
>  	}

This resulted in some weirdness in indentation / style, so I amended 
this patch to remove the now unneeded {} from the if/else if

>
> -	DBG("subsystem %s finished", subsystem);
> -
>  done:
>  	udev_device_unref(device);
>
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH udev 04/15] udev: get udev property via lib function

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:57 AM, Jonas Bonn wrote:
> ---
>  plugins/udev.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH udev 05/15] udev: remove unused modem property

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:57 AM, Jonas Bonn wrote:
> The property 'Registered' is not used anywhere.
> ---
>  plugins/udev.c | 1 -
>  1 file changed, 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH udev 06/15] udev: simplify add_modem

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:57 AM, Jonas Bonn wrote:
> Adding a modem boils down to finding the device in the hierarchy with
> the OFONO_DRIVER property.  The original code special-cased the property
> being on the device itself rather than on a parent device.  This patch
> combines the two cases.
> ---
>  plugins/udev.c | 72 ++++++++++++++++++++++++++--------------------------------
>  1 file changed, 32 insertions(+), 40 deletions(-)
>

I went ahead and applied patches 6-10.

Regards,
-Denis

Re: [PATCH udev 11/15] udevng: add legacy device handling functions

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:58 AM, Jonas Bonn wrote:
> This adds, but does not hook up, functionality to handle devices
> that are currently handled by the 'legacy' udev.c module.
>
> - legacy device is added by calling the add_legacy_device() function.
> - the function sets up a modem_info and device_info structure for
>   the legacy device
> - the device driver is set to "legacy" and the real driver type is
>   saved in the device_info structure
> - the extra udev information from the add_*() functions of the old
>   udev.c module is added to unused fields of the device_info structure
> - the "legacy" driver is set-up to use setup_legacy
> - setup_legacy extracts the device_info information and applies it
>   to the modem in the same way that the add_*() funtions did
> - the modem driver is switched to the real driver
>
> ...and off we go!
> ---
>  plugins/udevng.c | 255 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 255 insertions(+)
>
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index ce2bc28..4e464c2 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -830,6 +830,52 @@ static gboolean setup_quectel(struct modem_info *modem)
>  	return TRUE;
>  }
>
> +static gboolean setup_legacy(struct modem_info *modem)
> +{
> +	const char* mdm = NULL;
> +	const char* driver = NULL;
> +	GSList *list;
> +
> +	/* These legacy device normally only have one device... */
> +	for (list = modem->devices; list; list = list->next) {
> +		struct device_info *info = list->data;
> +
> +		if (!g_strcmp0(modem->driver, "ifx")) {
> +			ofono_modem_set_string(modem->modem,
> +					"LineDiscipline", info->number);
> +			ofono_modem_set_string(modem->modem,
> +					"AudioSetting", info->label);
> +			ofono_modem_set_string(modem->modem,
> +					"AudioLoopback", info->interface);
> +		} else if (!g_strcmp0(modem->driver, "8500")) {
> +		} else if (!g_strcmp0(modem->driver, "n900")) {
> +			if (!info->interface)
> +				continue;
> +
> +			ofono_modem_set_integer(modem->modem,
> +				"Address", atoi(info->number));
> +			ofono_modem_set_string(modem->modem,
> +					"Interface", info->interface);
> +
> +		} else if (!g_strcmp0(modem->driver, "wavecom")) {
> +			ofono_modem_set_string(modem->modem,
> +					"Model", info->number);
> +		}
> +
> +		mdm = info->devnode;
> +		driver = info->sysattr;
> +	}
> +
> +	if (!mdm)
> +		return FALSE;
> +
> +	ofono_modem_set_driver(modem->modem, driver);
> +
> +	ofono_modem_set_string(modem->modem, "Device", mdm);	
> +
> +	return TRUE;
> +}
> +
>  static gboolean setup_ublox(struct modem_info *modem)
>  {
>  	const char *aux = NULL, *mdm = NULL, *net = NULL;
> @@ -959,6 +1005,7 @@ static struct {
>  	{ "quectel",	setup_quectel	},
>  	{ "ublox",	setup_ublox	},
>  	{ "gemalto",	setup_gemalto	},
> +	{ "legacy",	setup_legacy	},
>  	{ }
>  };
>
> @@ -1050,6 +1097,214 @@ static gint compare_device(gconstpointer a, gconstpointer b)
>  	return g_strcmp0(info1->number, info2->number);
>  }
>
> +/*
> + * These add_*_info functions are part of the add_legacy_device()
> + * functionality.  Don't re-use them in anything new; ideally they will
> + * be able to go away when the hardware is deemed unavailable.
> + */
> +static void add_isi_info(struct device_info* info,
> +					struct udev_device *udev_device)
> +{
> +	const char *value;
> +	const char* type;
> +	const char* ifname;
> +
> +	/* Set info->interface if we want this device to be a modem;
> +	 * leave it as NULL to ignore it
> +	 */
> +	if (g_strcmp0(udev_device_get_subsystem(udev_device), "net") != 0)
> +		return;
> +
> +	type = udev_device_get_sysattr_value(udev_device, "type");
> +	if (g_strcmp0(type, "820") != 0)
> +		return;
> +
> +	/* OK, we want this device to be a modem */
> +	ifname = udev_device_get_sysname(udev_device);
> +	if (ifname)
> +		info->interface = g_strdup(ifname);
> +
> +	value = udev_device_get_property_value(udev_device,
> +						"OFONO_ISI_ADDRESS");
> +	if (value)
> +		info->number = g_strdup(value);
> +}
> +
> +
> +static void add_wavecom_info(struct device_info *info, struct udev_device *dev)
> +{
> +	const char *value;
> +
> +	value = udev_device_get_property_value(dev, "OFONO_WAVECOM_MODEL");
> +	if (value)
> +		info->number = g_strdup(value);
> +}
> +
> +static void add_ifx_info(struct device_info* info, struct udev_device *dev)
> +{
> +	const char *value;
> +
> +	value = udev_device_get_property_value(dev, "OFONO_IFX_LDISC");
> +	if (value)
> +		info->number = g_strdup(value);
> +	value = udev_device_get_property_value(dev, "OFONO_IFX_AUDIO");
> +	if (value)
> +		info->label = g_strdup(value);
> +	value = udev_device_get_property_value(dev, "OFONO_IFX_LOOPBACK");
> +	if (value)
> +		info->interface = g_strdup(value);
> +}
> +
> +/*
> + * Here we try to find the "modem device".
> + *
> + * In this variant we identify the "modem device" as simply the device
> + * that has the OFONO_DRIVER property.  If the device node doesn't
> + * have this property itself, then we do a brute force search for it
> + * through the device hierarchy.
> + *
> + */
> +static struct udev_device* get_legacy_modem_device(struct udev_device *dev)
> +{
> +	const char* driver;
> +
> +	while (dev) {
> +		driver = udev_device_get_property_value(dev, "OFONO_DRIVER");
> +		if (driver)
> +			return dev;
> +		dev = udev_device_get_parent(dev);
> +	}
> +
> +	return NULL;
> +}
> +
> +
> +/*
> + * Add 'legacy' device
> + *
> + * The term legacy is a bit misleading, but this adds devices according
> + * to the original ofono model.
> + *
> + * - We cannot assume that these are USB devices
> + * - The modem consists of only a single interface
> + * - The device must have an OFONO_DRIVER property from udev
> + */
> +static void add_legacy_device(struct udev_device *dev)
> +{
> +	const char *syspath, *devpath, *devname, *devnode;
> +	struct udev_device *usb_device;
> +	struct modem_info *modem;
> +	struct device_info *info;
> +	const char* vendor = NULL;
> +	const char* model = NULL;
> +	const char *subsystem;
> +	struct udev_device* mdev;
> +	const char* driver;
> +
> +	mdev = get_legacy_modem_device(dev);
> +	if (!mdev) {
> +		DBG("Device is missing required OFONO_DRIVER property");
> +		return;
> +	}
> +
> +	driver = udev_device_get_property_value(mdev, "OFONO_DRIVER");
> +	/* Check that this really is a legacy driver */
> +	if (!g_strcmp0(driver, "ifx")) goto start;
> +	if (!g_strcmp0(driver, "u8500")) goto start;
> +	if (!g_strcmp0(driver, "n900")) goto start;
> +	if (!g_strcmp0(driver, "calypso")) goto start;
> +	if (!g_strcmp0(driver, "cinterion")) goto start;
> +	if (!g_strcmp0(driver, "nokiacdma")) goto start;
> +	if (!g_strcmp0(driver, "sim900")) goto start;
> +	if (!g_strcmp0(driver, "wavecom")) goto start;

This might look better as a table

> +	if (!g_strcmp0(driver, "tc65")) {
> +		driver= "cinterion";
> +		goto start;
> +	}
> +	if (!g_strcmp0(driver, "ehs6")) {
> +		driver= "cinterion";
> +		goto start;
> +	}
> +
> +	/* Driver isn't something we recognize... */
> +	return;
> +start:
> +
> +	syspath = udev_device_get_syspath(mdev);
> +	devname = udev_device_get_devnode(mdev);
> +	devpath = udev_device_get_devpath(mdev);
> +
> +	devnode = udev_device_get_devnode(dev);
> +
> +	if (!syspath || !devname || !devpath || !devnode)
> +		return;
> +
> +	/* Maybe this is a USB device... in that case we can grab the
> +	 * vendor and model information
> +	 */
> +	usb_device = udev_device_get_parent_with_subsystem_devtype(mdev,
> +							"usb", "usb_device");
> +	if (usb_device) {
> +		vendor = udev_device_get_property_value(usb_device,
> +							"ID_VENDOR_ID");
> +		model = udev_device_get_property_value(usb_device,
> +							"ID_MODEL_ID");

Are you sure about this? I don't think there were any usb devices that 
udev still handled.  Maybe with the exception of some weird CDMA stuff, 
but that doesn't really work and should be handled via normal udevng 
mechanisms.

> +	}
> +
> +	modem = g_hash_table_lookup(modem_list, syspath);
> +	if (modem == NULL) {
> +		modem = g_try_new0(struct modem_info, 1);
> +		if (modem == NULL)
> +			return;
> +
> +		modem->syspath = g_strdup(syspath);
> +		modem->devname = g_strdup(devname);
> +		modem->driver = g_strdup("legacy");
> +		modem->vendor = g_strdup(vendor);
> +		modem->model = g_strdup(model);
> +
> +		g_hash_table_replace(modem_list, modem->syspath, modem);
> +	}
> +
> +	subsystem = udev_device_get_subsystem(dev);
> +
> +	DBG("%s", syspath);
> +	DBG("%s", devpath);
> +	DBG("%s (%s)", devnode, driver);
> +
> +	info = g_try_new0(struct device_info, 1);
> +	if (info == NULL)
> +		return;
> +
> +	info->devpath = g_strdup(devpath);
> +	info->devnode = g_strdup(devnode);
> +	info->subsystem = g_strdup(subsystem);
> +
> +	/* Here we begin the abuse... we want to switch the modem driver
> +	 * from 'legacy' to it's real driver in the setup function, so
> +	 * we store the driver name here in the otherwise unused sysattr
> +	 * field
> +	 */
> +	info->sysattr = g_strdup(driver);

The hi-jacking of device_info attributes is not going to fly.  Why don't 
we simply get rid of sysattr, and invent a brand new structure to hold 
the info for 'legacy' (or really true serial port) devices.  The ideal 
situation would be for us to continue using ofono_modem_set_property for 
all the driver specific stuff.

Is there a reason why we can't create the modem object here?  The 
current logic enumerates all the devices and runs create_modem 
afterwards, but I see no reason why we can't suppress that logic for 
serial-only devices.

> +
> +	/* Here we abuse some unused device_info attributes to hold
> +	 * extra udev properties... this is ugly but this is all
> +	 * legacy stuff that hopefully disappears someday anyway.
> +	 */
> +	if (g_strcmp0(driver, "ifx")) {
> +		add_ifx_info(info, dev);
> +	} else if (g_strcmp0(driver, "u8500")) {
> +		add_isi_info(info, dev);
> +	} else if (g_strcmp0(driver, "n900")) {
> +		add_isi_info(info, dev);
> +	} else if (g_strcmp0(driver, "wavecom")) {
> +		add_wavecom_info(info, dev);
> +	}
> +
> +	modem->devices = g_slist_insert_sorted(modem->devices, info,
> +							compare_device);
> +}
> +
>  static void add_device(const char *syspath, const char *devname,
>  			const char *driver, const char *vendor,
>  			const char *model, struct udev_device *device)
>

Regards,
-Denis

Re: [PATCH udev 14/15] udevng: get properties from interface

[Denis Kenzior]

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

Hi Jonas,

On 03/25/2017 11:58 AM, Jonas Bonn wrote:
> Device properties are generally on the device, on the USB interface
> descriptor, or the on the USB device descriptor.
> ---
>  plugins/udevng.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index 8721447..98b4f98 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1365,6 +1365,9 @@ static void add_device(const char *syspath, const char *devname,
>  	}
>
>  	label = udev_device_get_property_value(device, "OFONO_LABEL");
> +	if (!label) {
> +		label = udev_device_get_property_value(intf, "OFONO_LABEL");
> +	}

Okay, but not really our style.  See doc/coding-style.txt item M1

>  	subsystem = udev_device_get_subsystem(device);
>
>  	if (modem->sysattr != NULL)
> @@ -1464,6 +1467,7 @@ static struct {
>  static void check_usb_device(struct udev_device *device)
>  {
>  	struct udev_device *usb_device;
> +	struct udev_device *intf;

This is better declared inside the if statement below.  Also intf is a 
horrible name.  Just name it usb_device or usb_parent or something.

>  	const char *syspath, *devname, *driver;
>  	const char *vendor = NULL, *model = NULL;
>
> @@ -1484,6 +1488,13 @@ static void check_usb_device(struct udev_device *device)
>  	model = udev_device_get_property_value(usb_device, "ID_MODEL_ID");
>
>  	driver = udev_device_get_property_value(usb_device, "OFONO_DRIVER");
> +	if (!driver) {
> +		intf = udev_device_get_parent_with_subsystem_devtype(device,
> +						"usb", "usb_interface");
> +		if (intf)
> +			driver = udev_device_get_property_value(
> +							intf, "OFONO_DRIVER");
> +	}

doc/coding-style.txt item M1

>  	if (driver == NULL) {
>  		const char *drv;
>  		unsigned int i;
>

Regards,
-Denis

Re: [PATCH udev 01/15] udevng: simplify logic in check_usb_device

[Lukasz Nowak]

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

This works fine for me. I have dropped my parent based patch and rebased the setup_telitqmi changes on top of this.

Lukasz


On 25/03/17 16:57, Jonas Bonn wrote:
> This patch simplifies and cleans up the check_usb_device function a bit
> by doing the two following (slightly intertwined) things:
> 
> 1)  The parent "usb_device" is searched for early in this function and this
> device will always have the ID_VENDOR_ID and ID_MODEL_ID properties.
> As such, we can get them from this device and thereby be certain that
> we _always_ have them available.
> 
> 2)  The logic of iterating the vendor_list table is cleaned up.  It's
> easier to follow and won't be any less efficient.
> ---
>  plugins/udevng.c | 37 ++++++++++++-------------------------
>  1 file changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index 2279bbe..ce2bc28 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1225,9 +1225,12 @@ static void check_usb_device(struct udev_device *device)
>  	if (devname == NULL)
>  		return;
>  
> +	vendor = udev_device_get_property_value(usb_device, "ID_VENDOR_ID");
> +	model = udev_device_get_property_value(usb_device, "ID_MODEL_ID");
> +
>  	driver = udev_device_get_property_value(usb_device, "OFONO_DRIVER");
>  	if (driver == NULL) {
> -		const char *drv, *vid, *pid;
> +		const char *drv;
>  		unsigned int i;
>  
>  		drv = udev_device_get_property_value(device, "ID_USB_DRIVER");
> @@ -1246,40 +1249,24 @@ static void check_usb_device(struct udev_device *device)
>  			}
>  		}
>  
> -		vid = udev_device_get_property_value(device, "ID_VENDOR_ID");
> -		pid = udev_device_get_property_value(device, "ID_MODEL_ID");
>  
> -		DBG("%s [%s:%s]", drv, vid, pid);
> +		DBG("%s [%s:%s]", drv, vendor, model);
>  
>  		for (i = 0; vendor_list[i].driver; i++) {
>  			if (g_str_equal(vendor_list[i].drv, drv) == FALSE)
>  				continue;
>  
> -			if (vendor_list[i].vid == NULL) {
> -				driver = vendor_list[i].driver;
> -				vendor = vid;
> -				model = pid;
> -				continue;
> +			if (vendor_list[i].vid) {
> +				if (!g_str_equal(vendor_list[i].vid, vendor))
> +					continue;
>  			}
>  
> -			if (vid == NULL || pid == NULL)
> -				continue;
> -
> -			if (g_str_equal(vendor_list[i].vid, vid) == TRUE) {
> -				if (vendor_list[i].pid == NULL) {
> -					driver = vendor_list[i].driver;
> -					vendor = vid;
> -					model = pid;
> +			if (vendor_list[i].pid) {
> +				if (!g_str_equal(vendor_list[i].pid, model))
>  					continue;
> -				}
> -
> -				if (g_strcmp0(vendor_list[i].pid, pid) == 0) {
> -					driver = vendor_list[i].driver;
> -					vendor = vid;
> -					model = pid;
> -					break;
> -				}
>  			}
> +
> +			driver = vendor_list[i].driver;
>  		}
>  
>  		if (driver == NULL)
> 

Re: [PATCH udev 11/15] udevng: add legacy device handling functions

[Jonas Bonn]

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

Hi Denis,

On 03/27/2017 05:24 AM, Denis Kenzior wrote:
> Hi Jonas,
>
> On 03/25/2017 11:58 AM, Jonas Bonn wrote:
>> This adds, but does not hook up, functionality to handle devices
>> that are currently handled by the 'legacy' udev.c module.
>>
>> - legacy device is added by calling the add_legacy_device() function.
>> - the function sets up a modem_info and device_info structure for
>>   the legacy device
>> - the device driver is set to "legacy" and the real driver type is
>>   saved in the device_info structure
>> - the extra udev information from the add_*() functions of the old
>>   udev.c module is added to unused fields of the device_info structure
>> - the "legacy" driver is set-up to use setup_legacy
>> - setup_legacy extracts the device_info information and applies it
>>   to the modem in the same way that the add_*() funtions did
>> - the modem driver is switched to the real driver
>>
>> ...and off we go!
>> ---
>> +
>> +    driver = udev_device_get_property_value(mdev, "OFONO_DRIVER");
>> +    /* Check that this really is a legacy driver */
>> +    if (!g_strcmp0(driver, "ifx")) goto start;
>> +    if (!g_strcmp0(driver, "u8500")) goto start;
>> +    if (!g_strcmp0(driver, "n900")) goto start;
>> +    if (!g_strcmp0(driver, "calypso")) goto start;
>> +    if (!g_strcmp0(driver, "cinterion")) goto start;
>> +    if (!g_strcmp0(driver, "nokiacdma")) goto start;
>> +    if (!g_strcmp0(driver, "sim900")) goto start;
>> +    if (!g_strcmp0(driver, "wavecom")) goto start;
>
> This might look better as a table

Agreed, but this will disappear when I rework things again.
>> +    /* Maybe this is a USB device... in that case we can grab the
>> +     * vendor and model information
>> +     */
>> +    usb_device = udev_device_get_parent_with_subsystem_devtype(mdev,
>> +                            "usb", "usb_device");
>> +    if (usb_device) {
>> +        vendor = udev_device_get_property_value(usb_device,
>> +                            "ID_VENDOR_ID");
>> +        model = udev_device_get_property_value(usb_device,
>> +                            "ID_MODEL_ID");
>
> Are you sure about this? I don't think there were any usb devices that 
> udev still handled.  Maybe with the exception of some weird CDMA 
> stuff, but that doesn't really work and should be handled via normal 
> udevng mechanisms.

This was a bit unclear to me.  Nothing in the code gives any clue about 
what bus these 'legacy' devices sit on.  If they are all devices with a 
"serial interface", that makes things a bit simpler. I intend to rework 
this patch based on that assumption.
>> +    /* Here we begin the abuse... we want to switch the modem driver
>> +     * from 'legacy' to it's real driver in the setup function, so
>> +     * we store the driver name here in the otherwise unused sysattr
>> +     * field
>> +     */
>> +    info->sysattr = g_strdup(driver);
>
> The hi-jacking of device_info attributes is not going to fly.  Why 
> don't we simply get rid of sysattr, and invent a brand new structure 
> to hold the info for 'legacy' (or really true serial port) devices.  
> The ideal situation would be for us to continue using 
> ofono_modem_set_property for all the driver specific stuff.
>
> Is there a reason why we can't create the modem object here?  The 
> current logic enumerates all the devices and runs create_modem 
> afterwards, but I see no reason why we can't suppress that logic for 
> serial-only devices.
>

Your suggestion to add a new structure for 'legacy'/serial devices was a 
good one.  Reworking things this wasy makes things much simpler and 
serial devices can be handled pretty much exactly as USB devices... the 
symmetry is nice.

We can certainly run create_modem early for the serial devices.   I will 
look into that, but in order to keep things symmetric for a all device 
types it's easier to just use the same delay everywhere. I'll make a 
note to look into that as well.

Thanks for the review.  I'll post a follow-up shortly.

/Jonas

Re: [PATCH udev 14/15] udevng: get properties from interface

[Jonas Bonn]

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

On 03/27/2017 05:27 AM, Denis Kenzior wrote:
> Hi Jonas,
>
> On 03/25/2017 11:58 AM, Jonas Bonn wrote:
>> Device properties are generally on the device, on the USB interface
>> descriptor, or the on the USB device descriptor.
>> ---
>>  plugins/udevng.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/plugins/udevng.c b/plugins/udevng.c
>> index 8721447..98b4f98 100644
>> --- a/plugins/udevng.c
>> +++ b/plugins/udevng.c
>> @@ -1365,6 +1365,9 @@ static void add_device(const char *syspath, 
>> const char *devname,
>>      }
>>
>>      label = udev_device_get_property_value(device, "OFONO_LABEL");
>> +    if (!label) {
>> +        label = udev_device_get_property_value(intf, "OFONO_LABEL");
>> +    }
>
> Okay, but not really our style.  See doc/coding-style.txt item M1

Hmmm.... I would say that this is essentially the 'variable allocation' 
clause of M1.  Set a value; if fail, then do something about it.  Agree?

>
>>      subsystem = udev_device_get_subsystem(device);
>>
>>      if (modem->sysattr != NULL)
>> @@ -1464,6 +1467,7 @@ static struct {
>>  static void check_usb_device(struct udev_device *device)
>>  {
>>      struct udev_device *usb_device;
>> +    struct udev_device *intf;
>
> This is better declared inside the if statement below.  Also intf is a 
> horrible name.  Just name it usb_device or usb_parent or something.

Heh... I agree, that name stinks.  But it was the name that was already 
used in the code so that's the reason I re-used it.  I'll change it and 
resubmit.

>
>>      const char *syspath, *devname, *driver;
>>      const char *vendor = NULL, *model = NULL;
>>
>> @@ -1484,6 +1488,13 @@ static void check_usb_device(struct 
>> udev_device *device)
>>      model = udev_device_get_property_value(usb_device, "ID_MODEL_ID");
>>
>>      driver = udev_device_get_property_value(usb_device, 
>> "OFONO_DRIVER");
>> +    if (!driver) {
>> +        intf = udev_device_get_parent_with_subsystem_devtype(device,
>> +                        "usb", "usb_interface");
>> +        if (intf)
>> +            driver = udev_device_get_property_value(
>> +                            intf, "OFONO_DRIVER");
>> +    }
>
> doc/coding-style.txt item M1

My comment above applies... allocate and handle failure to allocate.

/Jonas

Re: [PATCH udev 14/15] udevng: get properties from interface

[Denis Kenzior]

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

Hi Jonas,

>>>      label = udev_device_get_property_value(device, "OFONO_LABEL");
>>> +    if (!label) {
>>> +        label = udev_device_get_property_value(intf, "OFONO_LABEL");
>>> +    }
>>
>> Okay, but not really our style.  See doc/coding-style.txt item M1
>
> Hmmm.... I would say that this is essentially the 'variable allocation'
> clause of M1.  Set a value; if fail, then do something about it.  Agree?
>

Yes, but the subsystem assignment on the next line still needs to be 
separated by an empty line.  Also the {} around the if are not needed.


>>>
>>>      driver = udev_device_get_property_value(usb_device,
>>> "OFONO_DRIVER");
>>> +    if (!driver) {
>>> +        intf = udev_device_get_parent_with_subsystem_devtype(device,
>>> +                        "usb", "usb_interface");
>>> +        if (intf)
>>> +            driver = udev_device_get_property_value(
>>> +                            intf, "OFONO_DRIVER");
>>> +    }
>>
>> doc/coding-style.txt item M1
>
> My comment above applies... allocate and handle failure to allocate.

If you declare & assign a variable, we prefer an empty line.  e.g.

if () {
	foo *bar = baz;

	if (bar)
		// do something
}

Also, the original comment was about the two if statements not being 
separated out.  So you have:

if (!driver) {
	...
}
if (driver == NULL)

Regards,
-Denis