gobi: support SMD devices (smartphone SoC)
cover gobi: support SMD devices (smartphone SoC)
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(-)
Signature · 1 line
2.7.4
PATCH 1/2 gobi: support rpmsg device node (SMD channel)
--- plugins/udevng.c | 61 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 33 insertions(+), 28 deletions(-)
plugins/udevng.c +33 -28
@@ -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" );
Signature · 1 line
2.7.4
PATCH 2/2 gobi/qmi: support smdpkt device (MSM kernel forks)
In reply to Joey HewittSearch 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(-)
drivers/qmimodem/qmi.c +29 -4
@@ -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 );
drivers/qmimodem/qmi.h +5 -1
@@ -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);
plugins/gobi.c +5 -2
@@ -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 ;
plugins/udevng.c +5 -0
@@ -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" );
Signature · 1 line
2.7.4
Hi Joey,
Quoted 3 lines
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.
Quoted 1 line
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
Hi Marcel,
Quoted 1 line
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.
Quoted 1 line
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.
Hi Joey,
Quoted 5 lines
Quoted 1 line
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 supportspoll()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.
Quoted 6 lines
Quoted 1 line
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