[PATCH 0/2] qmimodem: set data format when needed (and possible)

[PATCH 0/2] qmimodem: set data format when needed (and possible)

[Christophe Ronco]

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

MC7430 is a raw-ip only modem. If accessed through 802.3 network interface,
no packets can be received (IP layer headers missing).

To be able to start a data connection with this modem, we must tell driver
(qmi_wwan.c) to mount a raw-ip network interface instead of a 802.3 interface.

This has been implemented in libqmi package. The idea is:
 - ask modem what data format it wants (raw-ip or 802.3)
 - ask driver what data format it expects (raw-ip or 802.3)
 - change driver data format if values do not match

Patch has been split in two.
In first part, we just add the code needed to get/set driver expected data
format but nobody ever call these functions.
In second part, logic explained above is implemented.
If anything fail (can't get expected data format, can't get modem data format,
...), we just don't set anything and continue.

To be able to use this, a kernel 4.5 or more recent is needed.

Christophe Ronco (2):
  qmimodem: get/set kernel device driver data format
  qmimodem: change kernel driver data format if needed

 drivers/qmimodem/gprs-context.c |  79 +++++++++++++++-
 drivers/qmimodem/qmi.c          | 200 ++++++++++++++++++++++++++++++++++++++++
 drivers/qmimodem/qmi.h          |  11 +++
 drivers/qmimodem/wda.h          |  25 +++++
 4 files changed, 312 insertions(+), 3 deletions(-)
 create mode 100644 drivers/qmimodem/wda.h

-- 
2.11.0

[PATCH 1/2] qmimodem: get/set kernel device driver data format

[Christophe Ronco]

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

Add a way to get and set data format expected by kernel device driver.
This is inspired by what is done in qmicli (package libqmi).
It does not use QMI protocol but a sysfs exported by kernel driver.
To use this feature, kernel version must be equal or more than 4.5.
---
 drivers/qmimodem/qmi.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/qmimodem/qmi.h |  10 +++
 2 files changed, 210 insertions(+)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 0080f250..bf3c7407 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -26,6 +26,8 @@
 #define _GNU_SOURCE
 #include <stdio.h>
 #include <ctype.h>
+#include <dirent.h>
+#include <errno.h>
 #include <fcntl.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -33,6 +35,8 @@
 
 #include <glib.h>
 
+#include <ofono/log.h>
+
 #include "qmi.h"
 #include "ctl.h"
 
@@ -1234,6 +1238,202 @@ bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
 	return true;
 }
 
+static bool get_device_file_name(struct qmi_device *device,
+					char *file_name, int size)
+{
+	pid_t pid;
+	char temp[100];
+	ssize_t result;
+
+	if (size <= 0)
+		return false;
+
+	pid = getpid();
+
+	snprintf(temp, 100, "/proc/%d/fd/%d", (int) pid, device->fd);
+	temp[99] = 0;
+
+	result = readlink(temp, file_name, size - 1);
+
+	if ((result == -1) || (result >= size - 1)) {
+		DBG("Error %d in readlink", errno);
+		return false;
+	}
+
+	file_name[result] = 0;
+
+	return true;
+}
+
+static char *get_first_dir_in_directory(char *dir_path)
+{
+	DIR *dir;
+	struct dirent *dir_entry;
+	char *dir_name = NULL;
+
+	dir = opendir(dir_path);
+
+	if (!dir)
+		return NULL;
+
+	dir_entry = readdir(dir);
+
+	while ((dir_entry != NULL)) {
+		if ((dir_entry->d_type == DT_DIR) &&
+				(strcmp(dir_entry->d_name, ".") != 0) &&
+				(strcmp(dir_entry->d_name, "..") != 0)) {
+			dir_name = strdup(dir_entry->d_name);
+			break;
+		}
+
+		dir_entry = readdir(dir);
+	}
+
+	closedir(dir);
+	return dir_name;
+}
+
+static char *get_device_iface_name(struct qmi_device *device)
+{
+	gchar * driver_names[] = { "usbmisc", "usb" };
+	guint i;
+	char file_path[100];
+	char *file_name;
+	char *int_name = NULL;
+
+	if (!get_device_file_name(device, file_path, 100))
+		return NULL;
+
+	file_name = basename(file_path);
+
+	for (i = 0; i < G_N_ELEMENTS(driver_names) && !int_name; i++) {
+		gchar *sysfs_path;
+
+		sysfs_path = g_strdup_printf("/sys/class/%s/%s/device/net/",
+						driver_names[i], file_name);
+		int_name = get_first_dir_in_directory(sysfs_path);
+		g_free(sysfs_path);
+	}
+
+	return int_name;
+}
+
+enum qmi_device_expected_data_format qmi_device_get_expected_data_format(
+						struct qmi_device *device)
+{
+	gchar *sysfs_path = NULL;
+	char *int_name = NULL;
+	FILE *f = NULL;
+	gchar value;
+	enum qmi_device_expected_data_format expected =
+					QMI_DEVICE_EXPECTED_DATA_FORMAT_UNKNOWN;
+
+	if (!device)
+		goto done;
+
+	int_name = get_device_iface_name(device);
+
+	if (!int_name) {
+		DBG("Error while getting interface name");
+		goto done;
+	}
+
+	/* Build sysfs file path and open it */
+	sysfs_path = g_strdup_printf("/sys/class/net/%s/qmi/raw_ip", int_name);
+
+	f = fopen(sysfs_path, "r");
+	if (f == NULL) {
+		/* maybe not supported by kernel */
+		DBG("Error %d in fopen(%s)", errno, sysfs_path);
+		goto done;
+	}
+
+	if (fread(&value, 1, 1, f) != 1) {
+		DBG("Error %d in fread(%s)", errno, sysfs_path);
+		goto done;
+	}
+
+	if (value == 'Y')
+		expected = QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP;
+	else if (value == 'N')
+		expected = QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3;
+	else
+		DBG("Unexpected sysfs file contents");
+
+done:
+	if (f)
+		fclose(f);
+
+	if (sysfs_path)
+		g_free(sysfs_path);
+
+	if (int_name)
+		free(int_name);
+
+	return expected;
+}
+
+bool qmi_device_set_expected_data_format(struct qmi_device *device,
+			enum qmi_device_expected_data_format format)
+{
+	bool res = false;
+	gchar *sysfs_path = NULL;
+	char *int_name = NULL;
+	FILE *f = NULL;
+	gchar value;
+
+	if (!device)
+		goto done;
+
+	switch (format) {
+	case QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3:
+		value = 'N';
+		break;
+	case QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP:
+		value = 'Y';
+		break;
+	default:
+		DBG("Unhandled firmat: %d", (int) format);
+		goto done;
+	}
+
+	int_name = get_device_iface_name(device);
+
+	if (!int_name) {
+		DBG("Error while getting interface name");
+		goto done;
+	}
+
+	/* Build sysfs file path and open it */
+	sysfs_path = g_strdup_printf("/sys/class/net/%s/qmi/raw_ip", int_name);
+
+	f = fopen(sysfs_path, "w");
+	if (f == NULL) {
+		/* maybe not supported by kernel */
+		DBG("Error %d in fopen(%s)", errno, sysfs_path);
+		goto done;
+	}
+
+	if (fwrite(&value, 1, 1, f) != 1) {
+		DBG("Error %d in fwrite(%s)", errno, sysfs_path);
+		goto done;
+	}
+
+	res = true;
+
+done:
+	if (f)
+		fclose(f);
+
+	if (sysfs_path)
+		g_free(sysfs_path);
+
+	if (int_name)
+		free(int_name);
+
+	return res;
+}
+
 struct qmi_param *qmi_param_new(void)
 {
 	struct qmi_param *param;
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index dca115c4..ac19fe01 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -47,6 +47,12 @@
 #define QMI_SERVICE_RMS		225	/* Remote management service */
 #define QMI_SERVICE_OMA		226	/* OMA device management service */
 
+enum qmi_device_expected_data_format {
+	QMI_DEVICE_EXPECTED_DATA_FORMAT_UNKNOWN,
+	QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3,
+	QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP,
+};
+
 struct qmi_version {
 	uint8_t type;
 	uint16_t major;
@@ -82,6 +88,10 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
 				void *user_data, qmi_destroy_func_t destroy);
 
+enum qmi_device_expected_data_format qmi_device_get_expected_data_format(
+						struct qmi_device *device);
+bool qmi_device_set_expected_data_format(struct qmi_device *device,
+			enum qmi_device_expected_data_format format);
 
 struct qmi_param;
 
-- 
2.11.0

[PATCH 2/2] qmimodem: change kernel driver data format if needed

[Christophe Ronco]

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

During gprs-context driver probe function, check kernel device driver
data format and modem low level data format.
If they are different, align kernel device driver data format on
modem low level data format.
If an error occurs during this process, continue probing without error.
This is inspired by what is done in qmicli and qmi-network (package
libqmi).
---
 drivers/qmimodem/gprs-context.c | 79 +++++++++++++++++++++++++++++++++++++++--
 drivers/qmimodem/qmi.h          |  1 +
 drivers/qmimodem/wda.h          | 25 +++++++++++++
 3 files changed, 102 insertions(+), 3 deletions(-)
 create mode 100644 drivers/qmimodem/wda.h

diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index a39db5e8..83c6b21a 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -30,12 +30,15 @@
 #include <ofono/gprs-context.h>
 
 #include "qmi.h"
+#include "wda.h"
 #include "wds.h"
 
 #include "qmimodem.h"
 
 struct gprs_context_data {
 	struct qmi_service *wds;
+	struct qmi_service *wda;
+	struct qmi_device *dev;
 	unsigned int active_context;
 	uint32_t pkt_handle;
 };
@@ -263,6 +266,69 @@ static void create_wds_cb(struct qmi_service *service, void *user_data)
 					pkt_status_notify, gc, NULL);
 }
 
+static void get_data_format_cb(struct qmi_result *result, void *user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
+	uint32_t llproto;
+	enum qmi_device_expected_data_format expected_llproto;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, NULL))
+		goto done;
+
+	if (!qmi_result_get_uint32(result, QMI_WDA_LL_PROTOCOL, &llproto))
+		goto done;
+
+	expected_llproto = qmi_device_get_expected_data_format(data->dev);
+
+	if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_802_3) &&
+			(expected_llproto ==
+				QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP)) {
+		if (!qmi_device_set_expected_data_format(data->dev,
+					QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3))
+			DBG("Fail to set expected data to 802.3");
+		else
+			DBG("expected data set to 802.3");
+	} else if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP) &&
+			(expected_llproto ==
+				QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3)) {
+		if (!qmi_device_set_expected_data_format(data->dev,
+					QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP))
+			DBG("Fail to set expected data to raw-ip");
+		else
+			DBG("expected data set to raw-ip");
+	}
+
+done:
+	qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
+									NULL);
+}
+
+static void create_wda_cb(struct qmi_service *service, void *user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
+
+	DBG("");
+
+	if (!service) {
+		DBG("Failed to request WDA service, continue initialization");
+		goto error;
+	}
+
+	data->wda = qmi_service_ref(service);
+
+	if (qmi_service_send(data->wda, QMI_WDA_GET_DATA_FORMAT, NULL,
+					get_data_format_cb, gc, NULL) > 0)
+		return;
+
+error:
+	qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
+									NULL);
+}
+
 static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
 					unsigned int vendor, void *user_data)
 {
@@ -274,8 +340,9 @@ static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
 	data = g_new0(struct gprs_context_data, 1);
 
 	ofono_gprs_context_set_data(gc, data);
+	data->dev = device;
 
-	qmi_service_create(device, QMI_SERVICE_WDS, create_wds_cb, gc, NULL);
+	qmi_service_create(device, QMI_SERVICE_WDA, create_wda_cb, gc, NULL);
 
 	return 0;
 }
@@ -288,9 +355,15 @@ static void qmi_gprs_context_remove(struct ofono_gprs_context *gc)
 
 	ofono_gprs_context_set_data(gc, NULL);
 
-	qmi_service_unregister_all(data->wds);
+	if (data->wds) {
+		qmi_service_unregister_all(data->wds);
+		qmi_service_unref(data->wds);
+	}
 
-	qmi_service_unref(data->wds);
+	if (data->wda) {
+		qmi_service_unregister_all(data->wda);
+		qmi_service_unref(data->wda);
+	}
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index ac19fe01..2233cdb0 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -42,6 +42,7 @@
 #define QMI_SERVICE_EFS		21	/* Embedded file system service */
 #define QMI_SERVICE_TS		23	/* Thermal sensors service */
 #define QMI_SERVICE_TMD		24	/* Thermal mitigation device service */
+#define QMI_SERVICE_WDA		26	/* Wireless data administrative service */
 #define QMI_SERVICE_PDC		36	/* Persistent device configuration service */
 #define QMI_SERVICE_CAT_OLD	224	/* Card application toolkit service */
 #define QMI_SERVICE_RMS		225	/* Remote management service */
diff --git a/drivers/qmimodem/wda.h b/drivers/qmimodem/wda.h
new file mode 100644
index 00000000..dadd9a29
--- /dev/null
+++ b/drivers/qmimodem/wda.h
@@ -0,0 +1,25 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2011-2012  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.
+ *
+ */
+
+#define QMI_WDA_SET_DATA_FORMAT	32	/* Set data format */
+#define QMI_WDA_GET_DATA_FORMAT	33	/* Get data format */
+
+/* Get and set data format interface */
+#define QMI_WDA_LL_PROTOCOL	0x11	/* uint32_t */
+#define QMI_WDA_DATA_LINK_PROTOCOL_UNKNOWN	0
+#define QMI_WDA_DATA_LINK_PROTOCOL_802_3	1
+#define QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP	2
-- 
2.11.0

Re: [PATCH 1/2] qmimodem: get/set kernel device driver data format

[Denis Kenzior]

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

Hi Christophe,

On 01/19/2017 02:15 AM, Christophe Ronco wrote:
> Add a way to get and set data format expected by kernel device driver.
> This is inspired by what is done in qmicli (package libqmi).
> It does not use QMI protocol but a sysfs exported by kernel driver.
> To use this feature, kernel version must be equal or more than 4.5.
> ---
>  drivers/qmimodem/qmi.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/qmimodem/qmi.h |  10 +++
>  2 files changed, 210 insertions(+)
>
> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
> index 0080f250..bf3c7407 100644
> --- a/drivers/qmimodem/qmi.c
> +++ b/drivers/qmimodem/qmi.c
> @@ -26,6 +26,8 @@
>  #define _GNU_SOURCE
>  #include <stdio.h>
>  #include <ctype.h>
> +#include <dirent.h>
> +#include <errno.h>
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> @@ -33,6 +35,8 @@
>
>  #include <glib.h>
>
> +#include <ofono/log.h>
> +
>  #include "qmi.h"
>  #include "ctl.h"
>
> @@ -1234,6 +1238,202 @@ bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
>  	return true;
>  }
>
> +static bool get_device_file_name(struct qmi_device *device,
> +					char *file_name, int size)
> +{
> +	pid_t pid;
> +	char temp[100];
> +	ssize_t result;
> +
> +	if (size <= 0)
> +		return false;
> +
> +	pid = getpid();
> +
> +	snprintf(temp, 100, "/proc/%d/fd/%d", (int) pid, device->fd);
> +	temp[99] = 0;
> +
> +	result = readlink(temp, file_name, size - 1);
> +
> +	if ((result == -1) || (result >= size - 1)) {

Due to operator precedence, both sets of inner parens are not really 
necessary.


> +		DBG("Error %d in readlink", errno);
> +		return false;
> +	}
> +
> +	file_name[result] = 0;
> +
> +	return true;
> +}
> +
> +static char *get_first_dir_in_directory(char *dir_path)
> +{
> +	DIR *dir;
> +	struct dirent *dir_entry;
> +	char *dir_name = NULL;
> +
> +	dir = opendir(dir_path);
> +
> +	if (!dir)
> +		return NULL;
> +
> +	dir_entry = readdir(dir);
> +
> +	while ((dir_entry != NULL)) {
> +		if ((dir_entry->d_type == DT_DIR) &&
> +				(strcmp(dir_entry->d_name, ".") != 0) &&
> +				(strcmp(dir_entry->d_name, "..") != 0)) {

inner parens should not be necessary here either.

> +			dir_name = strdup(dir_entry->d_name);

Might as well use g_strdup here for consistency

> +			break;
> +		}
> +
> +		dir_entry = readdir(dir);
> +	}
> +
> +	closedir(dir);
> +	return dir_name;
> +}
> +
> +static char *get_device_iface_name(struct qmi_device *device)

Can we name this get_device_interface?

> +{
> +	gchar * driver_names[] = { "usbmisc", "usb" };
> +	guint i;

use char & unsigned int here.  We try to avoid GLib types whenever possible.

> +	char file_path[100];

Any reason this isn't PATH_MAX?  What's the file_path expected to be?

> +	char *file_name;
> +	char *int_name = NULL;
> +
> +	if (!get_device_file_name(device, file_path, 100))

100 -> sizeof(file_path)?

> +		return NULL;
> +
> +	file_name = basename(file_path);
> +
> +	for (i = 0; i < G_N_ELEMENTS(driver_names) && !int_name; i++) {
> +		gchar *sysfs_path;
> +
> +		sysfs_path = g_strdup_printf("/sys/class/%s/%s/device/net/",
> +						driver_names[i], file_name);
> +		int_name = get_first_dir_in_directory(sysfs_path);
> +		g_free(sysfs_path);
> +	}
> +
> +	return int_name;
> +}
> +
> +enum qmi_device_expected_data_format qmi_device_get_expected_data_format(
> +						struct qmi_device *device)
> +{
> +	gchar *sysfs_path = NULL;

char *

> +	char *int_name = NULL;
> +	FILE *f = NULL;

We don't use FILE operations and prefer to use the low-level versions 
(open/close/read/write)

> +	gchar value;

char

> +	enum qmi_device_expected_data_format expected =
> +					QMI_DEVICE_EXPECTED_DATA_FORMAT_UNKNOWN;
> +
> +	if (!device)
> +		goto done;
> +
> +	int_name = get_device_iface_name(device);

How about a better name here.  E.g. interface = get_device_interface(device)

> +
> +	if (!int_name) {
> +		DBG("Error while getting interface name");
> +		goto done;
> +	}
> +
> +	/* Build sysfs file path and open it */
> +	sysfs_path = g_strdup_printf("/sys/class/net/%s/qmi/raw_ip", int_name);
> +
> +	f = fopen(sysfs_path, "r");
> +	if (f == NULL) {
> +		/* maybe not supported by kernel */
> +		DBG("Error %d in fopen(%s)", errno, sysfs_path);
> +		goto done;
> +	}
> +
> +	if (fread(&value, 1, 1, f) != 1) {
> +		DBG("Error %d in fread(%s)", errno, sysfs_path);
> +		goto done;
> +	}

Can we replace fopen/fread by open/read?

> +
> +	if (value == 'Y')
> +		expected = QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP;
> +	else if (value == 'N')
> +		expected = QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3;
> +	else
> +		DBG("Unexpected sysfs file contents");
> +
> +done:
> +	if (f)
> +		fclose(f);
> +
> +	if (sysfs_path)
> +		g_free(sysfs_path);
> +
> +	if (int_name)
> +		free(int_name);

g_free if you use g_strdup.

> +
> +	return expected;
> +}
> +
> +bool qmi_device_set_expected_data_format(struct qmi_device *device,
> +			enum qmi_device_expected_data_format format)
> +{
> +	bool res = false;
> +	gchar *sysfs_path = NULL;

char

> +	char *int_name = NULL;

char *interface

> +	FILE *f = NULL;
> +	gchar value;

char

> +
> +	if (!device)
> +		goto done;
> +
> +	switch (format) {
> +	case QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3:
> +		value = 'N';
> +		break;
> +	case QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP:
> +		value = 'Y';
> +		break;
> +	default:
> +		DBG("Unhandled firmat: %d", (int) format);
> +		goto done;
> +	}
> +
> +	int_name = get_device_iface_name(device);

interface = ?

> +
> +	if (!int_name) {
> +		DBG("Error while getting interface name");
> +		goto done;
> +	}
> +
> +	/* Build sysfs file path and open it */
> +	sysfs_path = g_strdup_printf("/sys/class/net/%s/qmi/raw_ip", int_name);
> +
> +	f = fopen(sysfs_path, "w");
> +	if (f == NULL) {
> +		/* maybe not supported by kernel */
> +		DBG("Error %d in fopen(%s)", errno, sysfs_path);
> +		goto done;
> +	}
> +
> +	if (fwrite(&value, 1, 1, f) != 1) {
> +		DBG("Error %d in fwrite(%s)", errno, sysfs_path);
> +		goto done;
> +	}

fopen, fwrite, fclose -> open/write/close

> +
> +	res = true;
> +
> +done:
> +	if (f)
> +		fclose(f);
> +
> +	if (sysfs_path)
> +		g_free(sysfs_path);
> +
> +	if (int_name)
> +		free(int_name);
> +
> +	return res;
> +}
> +
>  struct qmi_param *qmi_param_new(void)
>  {
>  	struct qmi_param *param;
> diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
> index dca115c4..ac19fe01 100644
> --- a/drivers/qmimodem/qmi.h
> +++ b/drivers/qmimodem/qmi.h
> @@ -47,6 +47,12 @@
>  #define QMI_SERVICE_RMS		225	/* Remote management service */
>  #define QMI_SERVICE_OMA		226	/* OMA device management service */
>
> +enum qmi_device_expected_data_format {
> +	QMI_DEVICE_EXPECTED_DATA_FORMAT_UNKNOWN,
> +	QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3,
> +	QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP,
> +};
> +
>  struct qmi_version {
>  	uint8_t type;
>  	uint16_t major;
> @@ -82,6 +88,10 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
>  bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
>  				void *user_data, qmi_destroy_func_t destroy);
>
> +enum qmi_device_expected_data_format qmi_device_get_expected_data_format(
> +						struct qmi_device *device);
> +bool qmi_device_set_expected_data_format(struct qmi_device *device,
> +			enum qmi_device_expected_data_format format);
>
>  struct qmi_param;
>
>

Regards,
-Denis

Re: [PATCH 2/2] qmimodem: change kernel driver data format if needed

[Denis Kenzior]

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

Hi Christophe,

On 01/19/2017 02:15 AM, Christophe Ronco wrote:
> During gprs-context driver probe function, check kernel device driver
> data format and modem low level data format.
> If they are different, align kernel device driver data format on
> modem low level data format.
> If an error occurs during this process, continue probing without error.
> This is inspired by what is done in qmicli and qmi-network (package
> libqmi).
> ---
>  drivers/qmimodem/gprs-context.c | 79 +++++++++++++++++++++++++++++++++++++++--
>  drivers/qmimodem/qmi.h          |  1 +
>  drivers/qmimodem/wda.h          | 25 +++++++++++++
>  3 files changed, 102 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/qmimodem/wda.h
>

This looks fine to me.  You do need to add wda.h to the build in 
Makefile.am and also update the Copyright in that file accordingly.

Regards,
-Denis

[PATCH 0/2] V2 qmimodem: set data format when needed (and possible)

[Christophe Ronco]

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

Remarks from Denis integrated.

MC7430 is a raw-ip only modem. If accessed through 802.3 network interface,
no packets can be received (IP layer headers missing).

To be able to start a data connection with this modem, we must tell driver
(qmi_wwan.c) to mount a raw-ip network interface instead of a 802.3 interface.

This has been implemented in libqmi package. The idea is:
 - ask modem what data format it wants (raw-ip or 802.3)
 - ask driver what data format it expects (raw-ip or 802.3)
 - change driver data format if values do not match

Patch has been split in two.
In first part, we just add the code needed to get/set driver expected data
format but nobody ever call these functions.
In second part, logic explained above is implemented.
If anything fail (can't get expected data format, can't get modem data format,
...), we just don't set anything and continue.

To be able to use this, a kernel 4.5 or more recent is needed.

Christophe Ronco (2):
  qmimodem: get/set kernel device driver data format
  qmimodem: change kernel driver data format if needed

 Makefile.am                     |   3 +-
 drivers/qmimodem/gprs-context.c |  79 +++++++++++++++-
 drivers/qmimodem/qmi.c          | 200 ++++++++++++++++++++++++++++++++++++++++
 drivers/qmimodem/qmi.h          |  11 +++
 drivers/qmimodem/wda.h          |  25 +++++
 5 files changed, 314 insertions(+), 4 deletions(-)
 create mode 100644 drivers/qmimodem/wda.h

-- 
2.11.0

[PATCH 1/2] qmimodem: get/set kernel device driver data format

[Christophe Ronco]

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

Add a way to get and set data format expected by kernel device driver.
This is inspired by what is done in qmicli (package libqmi).
It does not use QMI protocol but a sysfs exported by kernel driver.
To use this feature, kernel version must be equal or more than 4.5.
---
 drivers/qmimodem/qmi.c | 200 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/qmimodem/qmi.h |  10 +++
 2 files changed, 210 insertions(+)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 0080f250..85b40693 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -26,6 +26,8 @@
 #define _GNU_SOURCE
 #include <stdio.h>
 #include <ctype.h>
+#include <dirent.h>
+#include <errno.h>
 #include <fcntl.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -33,6 +35,8 @@
 
 #include <glib.h>
 
+#include <ofono/log.h>
+
 #include "qmi.h"
 #include "ctl.h"
 
@@ -1234,6 +1238,202 @@ bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
 	return true;
 }
 
+static bool get_device_file_name(struct qmi_device *device,
+					char *file_name, int size)
+{
+	pid_t pid;
+	char temp[100];
+	ssize_t result;
+
+	if (size <= 0)
+		return false;
+
+	pid = getpid();
+
+	snprintf(temp, 100, "/proc/%d/fd/%d", (int) pid, device->fd);
+	temp[99] = 0;
+
+	result = readlink(temp, file_name, size - 1);
+
+	if (result == -1 || result >= size - 1) {
+		DBG("Error %d in readlink", errno);
+		return false;
+	}
+
+	file_name[result] = 0;
+
+	return true;
+}
+
+static char *get_first_dir_in_directory(char *dir_path)
+{
+	DIR *dir;
+	struct dirent *dir_entry;
+	char *dir_name = NULL;
+
+	dir = opendir(dir_path);
+
+	if (!dir)
+		return NULL;
+
+	dir_entry = readdir(dir);
+
+	while ((dir_entry != NULL)) {
+		if (dir_entry->d_type == DT_DIR &&
+				strcmp(dir_entry->d_name, ".") != 0 &&
+				strcmp(dir_entry->d_name, "..") != 0) {
+			dir_name = g_strdup(dir_entry->d_name);
+			break;
+		}
+
+		dir_entry = readdir(dir);
+	}
+
+	closedir(dir);
+	return dir_name;
+}
+
+static char *get_device_interface(struct qmi_device *device)
+{
+	char * const driver_names[] = { "usbmisc", "usb" };
+	unsigned int i;
+	char file_path[PATH_MAX];
+	char *file_name;
+	char *interface = NULL;
+
+	if (!get_device_file_name(device, file_path, sizeof(file_path)))
+		return NULL;
+
+	file_name = basename(file_path);
+
+	for (i = 0; i < G_N_ELEMENTS(driver_names) && !interface; i++) {
+		gchar *sysfs_path;
+
+		sysfs_path = g_strdup_printf("/sys/class/%s/%s/device/net/",
+						driver_names[i], file_name);
+		interface = get_first_dir_in_directory(sysfs_path);
+		g_free(sysfs_path);
+	}
+
+	return interface;
+}
+
+enum qmi_device_expected_data_format qmi_device_get_expected_data_format(
+						struct qmi_device *device)
+{
+	char *sysfs_path = NULL;
+	char *interface = NULL;
+	int fd = -1;
+	char value;
+	enum qmi_device_expected_data_format expected =
+					QMI_DEVICE_EXPECTED_DATA_FORMAT_UNKNOWN;
+
+	if (!device)
+		goto done;
+
+	interface = get_device_interface(device);
+
+	if (!interface) {
+		DBG("Error while getting interface name");
+		goto done;
+	}
+
+	/* Build sysfs file path and open it */
+	sysfs_path = g_strdup_printf("/sys/class/net/%s/qmi/raw_ip", interface);
+
+	fd = open(sysfs_path, O_RDONLY);
+	if (fd < 0) {
+		/* maybe not supported by kernel */
+		DBG("Error %d in open(%s)", errno, sysfs_path);
+		goto done;
+	}
+
+	if (read(fd, &value, 1) != 1) {
+		DBG("Error %d in read(%s)", errno, sysfs_path);
+		goto done;
+	}
+
+	if (value == 'Y')
+		expected = QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP;
+	else if (value == 'N')
+		expected = QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3;
+	else
+		DBG("Unexpected sysfs file contents");
+
+done:
+	if (fd >= 0)
+		close(fd);
+
+	if (sysfs_path)
+		g_free(sysfs_path);
+
+	if (interface)
+		g_free(interface);
+
+	return expected;
+}
+
+bool qmi_device_set_expected_data_format(struct qmi_device *device,
+			enum qmi_device_expected_data_format format)
+{
+	bool res = false;
+	char *sysfs_path = NULL;
+	char *interface = NULL;
+	int fd = -1;
+	char value;
+
+	if (!device)
+		goto done;
+
+	switch (format) {
+	case QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3:
+		value = 'N';
+		break;
+	case QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP:
+		value = 'Y';
+		break;
+	default:
+		DBG("Unhandled firmat: %d", (int) format);
+		goto done;
+	}
+
+	interface = get_device_interface(device);
+
+	if (!interface) {
+		DBG("Error while getting interface name");
+		goto done;
+	}
+
+	/* Build sysfs file path and open it */
+	sysfs_path = g_strdup_printf("/sys/class/net/%s/qmi/raw_ip", interface);
+
+	fd = open(sysfs_path, O_WRONLY);
+	if (fd < 0) {
+		/* maybe not supported by kernel */
+		DBG("Error %d in open(%s)", errno, sysfs_path);
+		goto done;
+	}
+
+	if (write(fd, &value, 1) != 1) {
+		DBG("Error %d in write(%s)", errno, sysfs_path);
+		goto done;
+	}
+
+	res = true;
+
+done:
+	if (fd >= 0)
+		close(fd);
+
+	if (sysfs_path)
+		g_free(sysfs_path);
+
+	if (interface)
+		g_free(interface);
+
+	return res;
+}
+
 struct qmi_param *qmi_param_new(void)
 {
 	struct qmi_param *param;
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index dca115c4..ac19fe01 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -47,6 +47,12 @@
 #define QMI_SERVICE_RMS		225	/* Remote management service */
 #define QMI_SERVICE_OMA		226	/* OMA device management service */
 
+enum qmi_device_expected_data_format {
+	QMI_DEVICE_EXPECTED_DATA_FORMAT_UNKNOWN,
+	QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3,
+	QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP,
+};
+
 struct qmi_version {
 	uint8_t type;
 	uint16_t major;
@@ -82,6 +88,10 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 bool qmi_device_shutdown(struct qmi_device *device, qmi_shutdown_func_t func,
 				void *user_data, qmi_destroy_func_t destroy);
 
+enum qmi_device_expected_data_format qmi_device_get_expected_data_format(
+						struct qmi_device *device);
+bool qmi_device_set_expected_data_format(struct qmi_device *device,
+			enum qmi_device_expected_data_format format);
 
 struct qmi_param;
 
-- 
2.11.0

[PATCH 2/2] qmimodem: change kernel driver data format if needed

[Christophe Ronco]

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

During gprs-context driver probe function, check kernel device driver
data format and modem low level data format.
If they are different, align kernel device driver data format on
modem low level data format.
If an error occurs during this process, continue probing without error.
This is inspired by what is done in qmicli and qmi-network (package
libqmi).
---
 Makefile.am                     |  3 +-
 drivers/qmimodem/gprs-context.c | 79 +++++++++++++++++++++++++++++++++++++++--
 drivers/qmimodem/qmi.h          |  1 +
 drivers/qmimodem/wda.h          | 25 +++++++++++++
 4 files changed, 104 insertions(+), 4 deletions(-)
 create mode 100644 drivers/qmimodem/wda.h

diff --git a/Makefile.am b/Makefile.am
index 560fc1cb..250ec30f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -216,7 +216,8 @@ qmi_sources = drivers/qmimodem/qmi.h drivers/qmimodem/qmi.c \
 					drivers/qmimodem/wms.h \
 					drivers/qmimodem/wds.h \
 					drivers/qmimodem/pds.h \
-					drivers/qmimodem/common.h
+					drivers/qmimodem/common.h \
+					drivers/qmimodem/wda.h
 
 builtin_modules += qmimodem
 builtin_sources += $(qmi_sources) \
diff --git a/drivers/qmimodem/gprs-context.c b/drivers/qmimodem/gprs-context.c
index b7f29b60..da2be241 100644
--- a/drivers/qmimodem/gprs-context.c
+++ b/drivers/qmimodem/gprs-context.c
@@ -30,12 +30,15 @@
 #include <ofono/gprs-context.h>
 
 #include "qmi.h"
+#include "wda.h"
 #include "wds.h"
 
 #include "qmimodem.h"
 
 struct gprs_context_data {
 	struct qmi_service *wds;
+	struct qmi_service *wda;
+	struct qmi_device *dev;
 	unsigned int active_context;
 	uint32_t pkt_handle;
 };
@@ -287,6 +290,69 @@ static void create_wds_cb(struct qmi_service *service, void *user_data)
 					pkt_status_notify, gc, NULL);
 }
 
+static void get_data_format_cb(struct qmi_result *result, void *user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
+	uint32_t llproto;
+	enum qmi_device_expected_data_format expected_llproto;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, NULL))
+		goto done;
+
+	if (!qmi_result_get_uint32(result, QMI_WDA_LL_PROTOCOL, &llproto))
+		goto done;
+
+	expected_llproto = qmi_device_get_expected_data_format(data->dev);
+
+	if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_802_3) &&
+			(expected_llproto ==
+				QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP)) {
+		if (!qmi_device_set_expected_data_format(data->dev,
+					QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3))
+			DBG("Fail to set expected data to 802.3");
+		else
+			DBG("expected data set to 802.3");
+	} else if ((llproto == QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP) &&
+			(expected_llproto ==
+				QMI_DEVICE_EXPECTED_DATA_FORMAT_802_3)) {
+		if (!qmi_device_set_expected_data_format(data->dev,
+					QMI_DEVICE_EXPECTED_DATA_FORMAT_RAW_IP))
+			DBG("Fail to set expected data to raw-ip");
+		else
+			DBG("expected data set to raw-ip");
+	}
+
+done:
+	qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
+									NULL);
+}
+
+static void create_wda_cb(struct qmi_service *service, void *user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *data = ofono_gprs_context_get_data(gc);
+
+	DBG("");
+
+	if (!service) {
+		DBG("Failed to request WDA service, continue initialization");
+		goto error;
+	}
+
+	data->wda = qmi_service_ref(service);
+
+	if (qmi_service_send(data->wda, QMI_WDA_GET_DATA_FORMAT, NULL,
+					get_data_format_cb, gc, NULL) > 0)
+		return;
+
+error:
+	qmi_service_create(data->dev, QMI_SERVICE_WDS, create_wds_cb, gc,
+									NULL);
+}
+
 static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
 					unsigned int vendor, void *user_data)
 {
@@ -298,8 +364,9 @@ static int qmi_gprs_context_probe(struct ofono_gprs_context *gc,
 	data = g_new0(struct gprs_context_data, 1);
 
 	ofono_gprs_context_set_data(gc, data);
+	data->dev = device;
 
-	qmi_service_create(device, QMI_SERVICE_WDS, create_wds_cb, gc, NULL);
+	qmi_service_create(device, QMI_SERVICE_WDA, create_wda_cb, gc, NULL);
 
 	return 0;
 }
@@ -312,9 +379,15 @@ static void qmi_gprs_context_remove(struct ofono_gprs_context *gc)
 
 	ofono_gprs_context_set_data(gc, NULL);
 
-	qmi_service_unregister_all(data->wds);
+	if (data->wds) {
+		qmi_service_unregister_all(data->wds);
+		qmi_service_unref(data->wds);
+	}
 
-	qmi_service_unref(data->wds);
+	if (data->wda) {
+		qmi_service_unregister_all(data->wda);
+		qmi_service_unref(data->wda);
+	}
 
 	g_free(data);
 }
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index ac19fe01..2233cdb0 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -42,6 +42,7 @@
 #define QMI_SERVICE_EFS		21	/* Embedded file system service */
 #define QMI_SERVICE_TS		23	/* Thermal sensors service */
 #define QMI_SERVICE_TMD		24	/* Thermal mitigation device service */
+#define QMI_SERVICE_WDA		26	/* Wireless data administrative service */
 #define QMI_SERVICE_PDC		36	/* Persistent device configuration service */
 #define QMI_SERVICE_CAT_OLD	224	/* Card application toolkit service */
 #define QMI_SERVICE_RMS		225	/* Remote management service */
diff --git a/drivers/qmimodem/wda.h b/drivers/qmimodem/wda.h
new file mode 100644
index 00000000..359f5b8e
--- /dev/null
+++ b/drivers/qmimodem/wda.h
@@ -0,0 +1,25 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2017  Kerlink SA. 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.
+ *
+ */
+
+#define QMI_WDA_SET_DATA_FORMAT	32	/* Set data format */
+#define QMI_WDA_GET_DATA_FORMAT	33	/* Get data format */
+
+/* Get and set data format interface */
+#define QMI_WDA_LL_PROTOCOL	0x11	/* uint32_t */
+#define QMI_WDA_DATA_LINK_PROTOCOL_UNKNOWN	0
+#define QMI_WDA_DATA_LINK_PROTOCOL_802_3	1
+#define QMI_WDA_DATA_LINK_PROTOCOL_RAW_IP	2
-- 
2.11.0

Re: [PATCH 0/2] V2 qmimodem: set data format when needed (and possible)

[Denis Kenzior]

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

Hi Christophe,

On 02/01/2017 04:32 AM, Christophe Ronco wrote:
> Remarks from Denis integrated.
>
> MC7430 is a raw-ip only modem. If accessed through 802.3 network interface,
> no packets can be received (IP layer headers missing).
>
> To be able to start a data connection with this modem, we must tell driver
> (qmi_wwan.c) to mount a raw-ip network interface instead of a 802.3 interface.
>
> This has been implemented in libqmi package. The idea is:
>  - ask modem what data format it wants (raw-ip or 802.3)
>  - ask driver what data format it expects (raw-ip or 802.3)
>  - change driver data format if values do not match
>
> Patch has been split in two.
> In first part, we just add the code needed to get/set driver expected data
> format but nobody ever call these functions.
> In second part, logic explained above is implemented.
> If anything fail (can't get expected data format, can't get modem data format,
> ...), we just don't set anything and continue.
>
> To be able to use this, a kernel 4.5 or more recent is needed.
>
> Christophe Ronco (2):
>   qmimodem: get/set kernel device driver data format
>   qmimodem: change kernel driver data format if needed
>
>  Makefile.am                     |   3 +-
>  drivers/qmimodem/gprs-context.c |  79 +++++++++++++++-
>  drivers/qmimodem/qmi.c          | 200 ++++++++++++++++++++++++++++++++++++++++
>  drivers/qmimodem/qmi.h          |  11 +++
>  drivers/qmimodem/wda.h          |  25 +++++
>  5 files changed, 314 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/qmimodem/wda.h
>

All applied, thanks.

Regards,
-Denis