7:56 p.m.
From: Lukasz Nowak <lnowak(a)tycoint.com>
Changes from V3:
- dropped patch:
[PATCH v4 2/3] udevng: find vendor and product id for child devices
instead, this patch set now depends on one from Jonas:
[PATCH udev 01/15] udevng: simplify logic in check_usb_device
- removed unused "Modem" string from telitqmi
Changes from V3:
- fixed a major synchronization problem in core qmimodem code
It was not doing anything bad by pure chance. With some of the
other changes, memory layout has moved around, and the code
was reliably segfaulting during modem physical power cycle
- added feature to udevng to discover usb vendor:product id
for child devices, which previously were reporting null:null
- with the vendor:product id available for all devices,
separated setup function from setup_gobi into setup_telitqmi
Changes from V2:
- made AlwaysOnline flow cleaner in gobi.c
- it turns out that AlwaysOnline is already a part of core logic
in src/modem.c which have the same effect as if the driver had
no set_online() function at all. Hence removed changes to
gobi_set_online() as it is never called.
Changes from V1:
- split into multiple smaller commits
- added comments in code, documenting issues with the Telit QMI firmware
- disabled non-working low-power operating modes in additional cases
- use mccmnc string instead of truncating an invalid operator name
Lukasz Nowak (2):
qmimodem: add protection against modem_unregister
udevng: add Telit LE910 V1 support
drivers/qmimodem/qmi.c | 39 +++++++++++++++++++++++++++++++++------
drivers/qmimodem/qmi.h | 2 +-
plugins/udevng.c | 41 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 75 insertions(+), 7 deletions(-)
--
2.7.4
7:56 p.m.
New subject: [PATCH v5 1/2] qmimodem: add protection against modem_unregister
From: Lukasz Nowak <lnowak(a)tycoint.com>
The qmimodem driver sets up numerous timeouts, passing internal
data structures to them as user_data. If a physical modem device
is removed, udev will call modem_unregister, which in turn will
call gobi_remove(). That function calls qmi_device_unref() which
frees all internal data structure memory inside qmidriver.
If a timeout is in progress while the usb device is removed,
the callback function will operate on already freed memory,
causing unpredicatble results. In some cases nothing bad will
happen, in others - segfault.
In order to solve this problem, add calls to qmi_device_ref
before setting up each timeout. Then inside callbacks, check
if this is the last reference (i.e. gobi_remove() has been called).
If that happens, do not operate on memory, just clean-up and exit.
---
drivers/qmimodem/qmi.c | 39 +++++++++++++++++++++++++++++++++------
drivers/qmimodem/qmi.h | 2 +-
2 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 8a9a88d..db36ddf 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -912,13 +912,17 @@ struct qmi_device *qmi_device_ref(struct qmi_device *device)
return device;
}
-void qmi_device_unref(struct qmi_device *device)
+/*
+ * Return TRUE if this was the last reference of the device and all
+ * data structures have been freed.
+ */
+bool qmi_device_unref(struct qmi_device *device)
{
if (!device)
- return;
+ return FALSE;
if (__sync_sub_and_fetch(&device->ref_count, 1))
- return;
+ return FALSE;
__debug_device(device, "device %p free", device);
@@ -946,6 +950,8 @@ void qmi_device_unref(struct qmi_device *device)
g_free(device->version_list);
g_free(device);
+
+ return TRUE;
}
void qmi_device_set_debug(struct qmi_device *device,
@@ -1013,6 +1019,9 @@ static void discover_callback(uint16_t message, uint16_t length,
g_source_remove(data->timeout);
+ if (qmi_device_unref(device))
+ goto error;
+
count = 0;
list = NULL;
@@ -1083,6 +1092,7 @@ done:
if (data->func)
data->func(count, list, data->user_data);
+error:
if (data->destroy)
data->destroy(data->user_data);
@@ -1096,7 +1106,7 @@ static gboolean discover_reply(gpointer user_data)
data->timeout = 0;
- if (data->func)
+ if (!qmi_device_unref(data->device) && data->func)
data->func(device->version_count,
device->version_list, data->user_data);
@@ -1130,6 +1140,7 @@ bool qmi_device_discover(struct qmi_device *device,
qmi_discover_func_t func,
data->destroy = destroy;
if (device->version_list) {
+ qmi_device_ref(device);
g_timeout_add_seconds(0, discover_reply, data);
return true;
}
@@ -1150,6 +1161,7 @@ bool qmi_device_discover(struct qmi_device *device,
qmi_discover_func_t func,
__request_submit(device, req, hdr->transaction);
+ qmi_device_ref(device);
data->timeout = g_timeout_add_seconds(5, discover_reply, data);
return true;
@@ -1192,7 +1204,7 @@ static gboolean shutdown_reply(gpointer user_data)
{
struct shutdown_data *data = user_data;
- if (data->func)
+ if (!qmi_device_unref(data->device) && data->func)
data->func(data->user_data);
g_free(data);
@@ -1205,6 +1217,12 @@ static gboolean shutdown_timeout(gpointer user_data)
struct shutdown_data *data = user_data;
struct qmi_device *device = data->device;
+ if (qmi_device_unref(data->device)) {
+ g_free(data);
+ return FALSE;
+ }
+
+ qmi_device_ref(device);
if (device->release_users > 0)
return TRUE;
@@ -1230,6 +1248,7 @@ bool qmi_device_shutdown(struct qmi_device *device,
qmi_shutdown_func_t func,
data->user_data = user_data;
data->destroy = destroy;
+ qmi_device_ref(device);
if (device->release_users > 0)
g_timeout_add_seconds(0, shutdown_timeout, data);
else
@@ -1716,7 +1735,8 @@ static gboolean service_create_reply(gpointer user_data)
{
struct service_create_data *data = user_data;
- data->func(NULL, data->user_data);
+ if (!qmi_device_unref(data->device))
+ data->func(NULL, data->user_data);
if (data->destroy)
data->destroy(data->user_data);
@@ -1739,6 +1759,9 @@ static void service_create_callback(uint16_t message, uint16_t
length,
g_source_remove(data->timeout);
+ if (qmi_device_unref(data->device))
+ goto error;
+
result_code = tlv_get(buffer, length, 0x02, &len);
if (!result_code)
goto done;
@@ -1781,6 +1804,7 @@ static void service_create_callback(uint16_t message, uint16_t
length,
done:
data->func(service, data->user_data);
+error:
qmi_service_unref(service);
if (data->destroy)
@@ -1816,6 +1840,8 @@ static void service_create_discover(uint8_t count,
if (!req) {
if (data->timeout > 0)
g_source_remove(data->timeout);
+ else
+ qmi_device_ref(device);
g_timeout_add_seconds(0, service_create_reply, data);
return;
@@ -1861,6 +1887,7 @@ static bool service_create(struct qmi_device *device, bool shared,
return false;
done:
+ qmi_device_ref(device);
data->timeout = g_timeout_add_seconds(8, service_create_reply, data);
return true;
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index 2233cdb..22cdcac 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -77,7 +77,7 @@ typedef void (*qmi_discover_func_t)(uint8_t count,
struct qmi_device *qmi_device_new(int fd);
struct qmi_device *qmi_device_ref(struct qmi_device *device);
-void qmi_device_unref(struct qmi_device *device);
+bool qmi_device_unref(struct qmi_device *device);
void qmi_device_set_debug(struct qmi_device *device,
qmi_debug_func_t func, void *user_data);
--
2.7.4
2:39 p.m.
New subject: [PATCH v5 1/2] qmimodem: add protection against modem_unregister
Hi Lukasz,
On 03/27/2017 03:56 AM, Lukasz Nowak wrote:
From: Lukasz Nowak <lnowak(a)tycoint.com>
The qmimodem driver sets up numerous timeouts, passing internal
data structures to them as user_data. If a physical modem device
is removed, udev will call modem_unregister, which in turn will
call gobi_remove(). That function calls qmi_device_unref() which
frees all internal data structure memory inside qmidriver.
If a timeout is in progress while the usb device is removed,
the callback function will operate on already freed memory,
causing unpredicatble results. In some cases nothing bad will
happen, in others - segfault.
In order to solve this problem, add calls to qmi_device_ref
before setting up each timeout. Then inside callbacks, check
if this is the last reference (i.e. gobi_remove() has been called).
If that happens, do not operate on memory, just clean-up and exit.
No, this is not the right way of doing this. The proper way to do this
is to store the glib gsource identifier for every timeout and remove
that timeout in the appropriate place / time.
I only see about 5 g_timeout_add calls inside qmi.c where the return
value isn't saved off. So you probably need to go through these and
save the gsource id in the appropriate data structure.
Regards,
-Denis
7:56 p.m.
New subject: [PATCH v5 2/2] udevng: add Telit LE910 V1 support
From: Lukasz Nowak <lnowak(a)tycoint.com>
Tested with LE910-SVG and Verizon.
---
plugins/udevng.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/plugins/udevng.c b/plugins/udevng.c
index ce2bc28..5eb7da8 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -665,6 +665,44 @@ static gboolean setup_telit(struct modem_info *modem)
return TRUE;
}
+static gboolean setup_telitqmi(struct modem_info *modem)
+{
+ const char *qmi = NULL, *net = NULL;
+ GSList *list;
+ gboolean telit = FALSE;
+
+ DBG("%s", modem->syspath);
+
+ for (list = modem->devices; list; list = list->next) {
+ struct device_info *info = list->data;
+
+ DBG("%s %s %s %s %s", info->devnode, info->interface,
+ info->number, info->label, info->subsystem);
+
+ if (g_strcmp0(info->interface, "255/255/255") == 0 &&
+ g_strcmp0(info->number, "02") == 0) {
+ if (g_strcmp0(info->subsystem, "net") == 0)
+ net = info->devnode;
+ else if (g_strcmp0(info->subsystem, "usbmisc") == 0)
+ qmi = info->devnode;
+ }
+ }
+
+ if (qmi == NULL || net == NULL)
+ return FALSE;
+
+ DBG("qmi=%s net=%s", qmi, net);
+
+ ofono_modem_set_string(modem->modem, "Device", qmi);
+ ofono_modem_set_string(modem->modem, "NetworkInterface", net);
+
+ ofono_modem_set_boolean(modem->modem, "ForceSimLegacy", TRUE);
+ ofono_modem_set_boolean(modem->modem, "AlwaysOnline", TRUE);
+ ofono_modem_set_driver(modem->modem, "gobi");
+
+ return TRUE;
+}
+
static gboolean setup_simcom(struct modem_info *modem)
{
const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
@@ -952,6 +990,7 @@ static struct {
{ "novatel", setup_novatel },
{ "nokia", setup_nokia },
{ "telit", setup_telit, "device/interface" },
+ { "telitqmi", setup_telitqmi },
{ "simcom", setup_simcom },
{ "zte", setup_zte },
{ "icera", setup_icera },
@@ -1190,6 +1229,8 @@ static struct {
{ "telit", "usbserial", "1bc7" },
{ "telit", "option", "1bc7" },
{ "telit", "cdc_acm", "1bc7", "0021" },
+ { "telitqmi", "qmi_wwan", "1bc7", "1201" },
+ { "telitqmi", "option", "1bc7", "1201" },
{ "nokia", "option", "0421", "060e" },
{ "nokia", "option", "0421", "0623" },
{ "samsung", "option", "04e8", "6889" },
--
2.7.4
2:41 p.m.
New subject: [PATCH v5 2/2] udevng: add Telit LE910 V1 support
Hi Lukasz,
On 03/27/2017 03:56 AM, Lukasz Nowak wrote:
From: Lukasz Nowak <lnowak(a)tycoint.com>
Tested with LE910-SVG and Verizon.
---
plugins/udevng.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
plugins/udevng.c: In function ‘setup_telitqmi’:
plugins/udevng.c:672:11: error: unused variable ‘telit’
[-Werror=unused-variable]
gboolean telit = FALSE;
^
cc1: all warnings being treated as errors
I went ahead and squashed that line and pushed out this patch.
Regards,
-Denis
2001
days inactive
2001
days old
4 comments
2 participants
participants (2)
-
Denis Kenzior -
Lukasz Nowak