[PATCH 1/2] gobi: support rpmsg device node (SMD channel)

[PATCH 1/2] gobi: support rpmsg device node (SMD channel)

[Joey Hewitt]

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

---
 plugins/udevng.c | 61 ++++++++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff5d41a..c85128a 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -202,39 +202,43 @@ static gboolean setup_gobi(struct modem_info *modem)
 
 	DBG("%s", modem->syspath);
 
-	for (list = modem->devices; list; list = list->next) {
-		struct device_info *info = list->data;
+	if (modem->type == MODEM_TYPE_USB) {
+		for (list = modem->devices; list; list = list->next) {
+			struct device_info *info = list->data;
 
-		DBG("%s %s %s %s %s %s", info->devnode, info->interface,
-						info->number, info->label,
-						info->sysattr, info->subsystem);
+			DBG("%s %s %s %s %s %s", info->devnode, info->interface,
+							info->number, info->label,
+							info->sysattr, info->subsystem);
 
-		if (g_strcmp0(info->subsystem, "usbmisc") == 0) /* cdc-wdm */
-			qmi = info->devnode;
-		else if (g_strcmp0(info->subsystem, "net") == 0) /* wwan */
-			net = info->devnode;
-		else if (g_strcmp0(info->subsystem, "tty") == 0) {
-			if (g_strcmp0(info->interface, "255/255/255") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)
-					diag = info->devnode; /* ec20 */
-				else if (g_strcmp0(info->number, "01") == 0)
-					diag = info->devnode; /* gobi */
-				else if (g_strcmp0(info->number, "02") == 0)
-					mdm = info->devnode; /* gobi */
-				else if (g_strcmp0(info->number, "03") == 0)
-					gps = info->devnode; /* gobi */
-			} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
-				if (g_strcmp0(info->number, "01") == 0)
-					gps = info->devnode; /* ec20 */
-				if (g_strcmp0(info->number, "02") == 0)
-					mdm = info->devnode; /* ec20 */
-				/* ignore the 3rd device second AT/mdm iface */
+			if (g_strcmp0(info->subsystem, "usbmisc") == 0) /* cdc-wdm */
+				qmi = info->devnode;
+			else if (g_strcmp0(info->subsystem, "net") == 0) /* wwan */
+				net = info->devnode;
+			else if (g_strcmp0(info->subsystem, "tty") == 0) {
+				if (g_strcmp0(info->interface, "255/255/255") == 0) {
+					if (g_strcmp0(info->number, "00") == 0)
+						diag = info->devnode; /* ec20 */
+					else if (g_strcmp0(info->number, "01") == 0)
+						diag = info->devnode; /* gobi */
+					else if (g_strcmp0(info->number, "02") == 0)
+						mdm = info->devnode; /* gobi */
+					else if (g_strcmp0(info->number, "03") == 0)
+						gps = info->devnode; /* gobi */
+				} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
+					if (g_strcmp0(info->number, "01") == 0)
+						gps = info->devnode; /* ec20 */
+					if (g_strcmp0(info->number, "02") == 0)
+						mdm = info->devnode; /* ec20 */
+					/* ignore the 3rd device second AT/mdm iface */
+				}
 			}
 		}
-	}
 
-	if (qmi == NULL || mdm == NULL || net == NULL)
-		return FALSE;
+		if (qmi == NULL || mdm == NULL || net == NULL)
+			return FALSE;
+	} else if (modem->type == MODEM_TYPE_SERIAL) {
+		qmi = modem->serial->devnode;
+	}
 
 	DBG("qmi=%s net=%s mdm=%s gps=%s diag=%s", qmi, net, mdm, gps, diag);
 
@@ -1772,6 +1776,7 @@ static void enumerate_devices(struct udev *context)
 		return;
 
 	udev_enumerate_add_match_subsystem(enumerate, "tty");
+	udev_enumerate_add_match_subsystem(enumerate, "rpmsg");
 	udev_enumerate_add_match_subsystem(enumerate, "usb");
 	udev_enumerate_add_match_subsystem(enumerate, "usbmisc");
 	udev_enumerate_add_match_subsystem(enumerate, "net");
-- 
2.7.4

[PATCH 2/2] gobi/qmi: support smdpkt device (MSM kernel forks)

[Joey Hewitt]

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

Search udev for smdpkt devices, work around smdpkt's poll() not
reporting when writable (we make writes block, and don't poll)
and plumb the subystem type through so we can activate that
workaround.
---
 drivers/qmimodem/qmi.c | 33 +++++++++++++++++++++++++++++----
 drivers/qmimodem/qmi.h |  6 +++++-
 plugins/gobi.c         |  7 +++++--
 plugins/udevng.c       |  5 +++++
 4 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 19ec130..8dd293d 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -29,6 +29,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <sys/ioctl.h>
 #include <unistd.h>
 #include <stdlib.h>
 #include <string.h>
@@ -82,6 +83,7 @@ struct qmi_device {
 	guint shutdown_source;
 	bool shutting_down : 1;
 	bool destroyed : 1;
+	bool is_smdpkt : 1;
 };
 
 struct qmi_service {
@@ -679,6 +681,13 @@ static gboolean can_write_data(GIOChannel *channel, GIOCondition cond,
 	return FALSE;
 }
 
+static gboolean can_write_data_idle(gpointer user_data)
+{
+	struct qmi_device *device = user_data;
+
+	return can_write_data(device->io, G_IO_OUT, user_data);
+}
+
 static void write_watch_destroy(gpointer user_data)
 {
 	struct qmi_device *device = user_data;
@@ -691,9 +700,15 @@ static void wakeup_writer(struct qmi_device *device)
 	if (device->write_watch > 0)
 		return;
 
-	device->write_watch = g_io_add_watch_full(device->io, G_PRIORITY_HIGH,
-				G_IO_OUT | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
-				can_write_data, device, write_watch_destroy);
+	if (!device->is_smdpkt) {
+		device->write_watch = g_io_add_watch_full(device->io, G_PRIORITY_HIGH,
+					G_IO_OUT | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+					can_write_data, device, write_watch_destroy);
+	} else {
+		// Polling G_IO_OUT does not work for smdpkt, so just write on idle.
+		device->write_watch = g_idle_add_full(G_PRIORITY_HIGH,
+					can_write_data_idle, device, write_watch_destroy);
+	}
 }
 
 static uint16_t __request_submit(struct qmi_device *device,
@@ -937,10 +952,11 @@ static void service_destroy(gpointer data)
 	service->device = NULL;
 }
 
-struct qmi_device *qmi_device_new(int fd)
+struct qmi_device *qmi_device_new(int fd, const char *subsystem)
 {
 	struct qmi_device *device;
 	long flags;
+	unsigned int blocking_flag = 1;
 
 	device = g_try_new0(struct qmi_device, 1);
 	if (!device)
@@ -952,6 +968,7 @@ struct qmi_device *qmi_device_new(int fd)
 
 	device->fd = fd;
 	device->close_on_unref = false;
+	device->is_smdpkt = (!g_strcmp0(subsystem, "smdpkt"));
 
 	flags = fcntl(device->fd, F_GETFL, NULL);
 	if (flags < 0) {
@@ -966,6 +983,14 @@ struct qmi_device *qmi_device_new(int fd)
 		}
 	}
 
+	if (device->is_smdpkt) {
+		// Make writes block, since smdpkt doesn't support polling writability
+		if (ioctl(fd, SMD_PKT_IOCTL_BLOCKING_WRITE, &blocking_flag) < 0) {
+			g_free(device);
+			return NULL;
+		}
+	}
+
 	device->io = g_io_channel_unix_new(device->fd);
 
 	g_io_channel_set_encoding(device->io, NULL, NULL);
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index 2665c44..d61b62b 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -55,6 +55,10 @@
 #define QMI_SERVICE_RMS		225	/* Remote management service */
 #define QMI_SERVICE_OMA		226	/* OMA device management service */
 
+#define SMD_PKT_IOCTL_MAGIC (0xC2)
+#define SMD_PKT_IOCTL_BLOCKING_WRITE \
+		_IOR(SMD_PKT_IOCTL_MAGIC, 0, unsigned int)
+
 enum qmi_device_expected_data_format {
 	QMI_DEVICE_EXPECTED_DATA_FORMAT_UNKNOWN,
 	QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3,
@@ -73,7 +77,7 @@ typedef void (*qmi_sync_func_t)(void *user_data);
 typedef void (*qmi_shutdown_func_t)(void *user_data);
 typedef void (*qmi_discover_func_t)(void *user_data);
 
-struct qmi_device *qmi_device_new(int fd);
+struct qmi_device *qmi_device_new(int fd, const char *subsystem);
 
 struct qmi_device *qmi_device_ref(struct qmi_device *device);
 void qmi_device_unref(struct qmi_device *device);
diff --git a/plugins/gobi.c b/plugins/gobi.c
index 8521891..2e83e10 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -317,7 +317,7 @@ static void discover_cb(void *user_data)
 static int gobi_enable(struct ofono_modem *modem)
 {
 	struct gobi_data *data = ofono_modem_get_data(modem);
-	const char *device;
+	const char *device, *subsystem;
 	int fd;
 
 	DBG("%p", modem);
@@ -325,12 +325,15 @@ static int gobi_enable(struct ofono_modem *modem)
 	device = ofono_modem_get_string(modem, "Device");
 	if (!device)
 		return -EINVAL;
+	subsystem = ofono_modem_get_string(modem, "Subsystem");
+	if (!subsystem)
+		return -EINVAL;
 
 	fd = open(device, O_RDWR | O_NONBLOCK | O_CLOEXEC);
 	if (fd < 0)
 		return -EIO;
 
-	data->device = qmi_device_new(fd);
+	data->device = qmi_device_new(fd, subsystem);
 	if (!data->device) {
 		close(fd);
 		return -ENOMEM;
diff --git a/plugins/udevng.c b/plugins/udevng.c
index c85128a..1ae5b11 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -198,11 +198,13 @@ static gboolean setup_gobi(struct modem_info *modem)
 {
 	const char *qmi = NULL, *mdm = NULL, *net = NULL;
 	const char *gps = NULL, *diag = NULL;
+	const char *subsystem = NULL;
 	GSList *list;
 
 	DBG("%s", modem->syspath);
 
 	if (modem->type == MODEM_TYPE_USB) {
+		subsystem = "usb";
 		for (list = modem->devices; list; list = list->next) {
 			struct device_info *info = list->data;
 
@@ -237,6 +239,7 @@ static gboolean setup_gobi(struct modem_info *modem)
 		if (qmi == NULL || mdm == NULL || net == NULL)
 			return FALSE;
 	} else if (modem->type == MODEM_TYPE_SERIAL) {
+		subsystem = modem->serial->subsystem;
 		qmi = modem->serial->devnode;
 	}
 
@@ -246,6 +249,7 @@ static gboolean setup_gobi(struct modem_info *modem)
 	ofono_modem_set_string(modem->modem, "Modem", mdm);
 	ofono_modem_set_string(modem->modem, "Diag", diag);
 	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
+	ofono_modem_set_string(modem->modem, "Subsystem", subsystem);
 
 	return TRUE;
 }
@@ -1777,6 +1781,7 @@ static void enumerate_devices(struct udev *context)
 
 	udev_enumerate_add_match_subsystem(enumerate, "tty");
 	udev_enumerate_add_match_subsystem(enumerate, "rpmsg");
+	udev_enumerate_add_match_subsystem(enumerate, "smdpkt");
 	udev_enumerate_add_match_subsystem(enumerate, "usb");
 	udev_enumerate_add_match_subsystem(enumerate, "usbmisc");
 	udev_enumerate_add_match_subsystem(enumerate, "net");
-- 
2.7.4

[PATCH 0/2] gobi: support SMD devices (smartphone SoC)

[Joey Hewitt]

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

These add support for Qualcomm-based smartphone modems.

The first is fairly straightforward: searching for "rpmsg"-subsystem devices and supporting them in gobi as a serial device. A friend has tested it on the mainline kernel, with the help of something like `rpmsgexport /dev/rpmsg_ctrl0 DATA5_CNTL` (https://github.com/andersson/rpmsgexport) and then giving the created rpmsg node OFONO_DRIVER=gobi property.

The second is uglier, and maybe controversial, since it's for Qualcomm's forked kernel. The smdpkt device doesn't support polling for write, so oFono would wait forever on the poll. This would probably be a 5-line fix in the kernel, but there are thousands of kernel forks, so I thought it might be better to workaround in oFono. I've tested it on Samsung S4 Mini LTE, running a kernel based on the MSM fork.

Joey Hewitt (2):
  gobi: support rpmsg device node (SMD channel)
  gobi/qmi: support smdpkt device (MSM kernel forks)

 drivers/qmimodem/qmi.c | 33 ++++++++++++++++++++++---
 drivers/qmimodem/qmi.h |  6 ++++-
 plugins/gobi.c         |  7 ++++--
 plugins/udevng.c       | 66 +++++++++++++++++++++++++++++---------------------
 4 files changed, 77 insertions(+), 35 deletions(-)

-- 
2.7.4

Re: [PATCH 0/2] gobi: support SMD devices (smartphone SoC)

[Marcel Holtmann]

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

Hi Joey,

> These add support for Qualcomm-based smartphone modems.
> 
> The first is fairly straightforward: searching for "rpmsg"-subsystem devices and supporting them in gobi as a serial device. A friend has tested it on the mainline kernel, with the help of something like `rpmsgexport /dev/rpmsg_ctrl0 DATA5_CNTL` (https://github.com/andersson/rpmsgexport) and then giving the created rpmsg node OFONO_DRIVER=gobi property.

I would actually prefer that the functionality of RPMSG_CREATE_EPT_IOCTL gets done inside oFono’s plugin. Also lifetime rules of that created node are wonky. I would expect that oFono quits, they are returned back to RPMSG subsystem.

> The second is uglier, and maybe controversial, since it's for Qualcomm's forked kernel. The smdpkt device doesn't support polling for write, so oFono would wait forever on the poll. This would probably be a 5-line fix in the kernel, but there are thousands of kernel forks, so I thought it might be better to workaround in oFono. I've tested it on Samsung S4 Mini LTE, running a kernel based on the MSM fork.

I would still fix this upstream and only workaround devices blacklisted with a quirk.

This reminds me also why RPMSG is not actually exported as a socket in the first. Creating a device node first and then using it later is always prone to race conditions. An alternative is to open the control character device and turn it into a proper pipe for QMI. I would like to see some proper fixes in the RPMSG subsystem to allow for proper handling by a telephony daemon.

On a side note, we had all these race conditions and issues with GSM 07.07 multiplexer code before. Having to wait for a device node to be create is crap. One reason why oFono comes with a builtin GSM 07.07 implementation.

Regards

Marcel

Re: [PATCH 0/2] gobi: support SMD devices (smartphone SoC)

[Joey Hewitt]

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

Hi Marcel,

> I would still fix this upstream and only workaround devices blacklisted with a quirk.

I don't see how this would be possible. The issue with poll() lies in 
the kernel, not the device. I guess a patched kernel could somehow 
indicate that it supports poll() for writing, like an attr on the device 
node.

OK, there's probably a lot I don't know about rpmsg, and I don't 
currently have a device running this setup.

> This reminds me also why RPMSG is not actually exported as a socket in the first. Creating a device node first and then using it later is always prone to race conditions. An alternative is to open the control character device and turn it into a proper pipe for QMI. I would like to see some proper fixes in the RPMSG subsystem to allow for proper handling by a telephony daemon.
I don't understand; what is the race condition? oFono plus another 
client trying to use the same device?

How would the control device become a QMI pipe? As it stands today, I 
think all you can do with it is open and do that ioctl 
(https://github.com/torvalds/linux/blob/60cc43fc888428bb2f18f08997432d426a243338/drivers/rpmsg/rpmsg_char.c#L451). 
How is the rpmsg endpoint char device not a proper pipe for QMI, 
compared to what you're envisioning?

It sounds like the userspace rpmsg system is not mature enough to do 
this the way you want. Will we have to wait until then to merge this?

Regards,
Joey

P.S. These patches should probably add the new subsystems to the hotplug 
code, not just the enumeration at startup.

Re: [PATCH 0/2] gobi: support SMD devices (smartphone SoC)

[Marcel Holtmann]

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

Hi Joey,

>> I would still fix this upstream and only workaround devices blacklisted with a quirk.
> 
> I don't see how this would be possible. The issue with poll() lies in the kernel, not the device. I guess a patched kernel could somehow indicate that it supports poll() for writing, like an attr on the device node.
> 
> OK, there's probably a lot I don't know about rpmsg, and I don't currently have a device running this setup.

it is a kernel bug for sure and should be fixed there. Not supporting POLLOUT properly is just bad.

>> This reminds me also why RPMSG is not actually exported as a socket in the first. Creating a device node first and then using it later is always prone to race conditions. An alternative is to open the control character device and turn it into a proper pipe for QMI. I would like to see some proper fixes in the RPMSG subsystem to allow for proper handling by a telephony daemon.
> I don't understand; what is the race condition? oFono plus another client trying to use the same device?
> 
> How would the control device become a QMI pipe? As it stands today, I think all you can do with it is open and do that ioctl (https://github.com/torvalds/linux/blob/60cc43fc888428bb2f18f08997432d426a243338/drivers/rpmsg/rpmsg_char.c#L451). How is the rpmsg endpoint char device not a proper pipe for QMI, compared to what you're envisioning?
> 
> It sounds like the userspace rpmsg system is not mature enough to do this the way you want. Will we have to wait until then to merge this?

Sadly, I have seen this before where it is not fully thought through. It could be really great and race free, but seems right now it is only half done. So I would start improving RPMSG subsystem.

We can look into working around older kernels (with tons of comments on why), but first I would like to see if we can do this properly. So that we don’t have to workaround our own workaround once the kernel behaves better.

Regards

Marcel