Re: [PATCH 1/2] xmm7modem: Change in xmm7mode plugin for multi PDP handling

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Antara,

On 02/06/2019 02:37 AM, Antara Borwankar wrote:
> Made changes in xmm7modem plugin to allow mutiple PDP context
> activation and to assign correct network interface to the
> activated PDP context.
> ---
>   plugins/udevng.c  | 12 +++++++++++-
>   plugins/xmm7xxx.c | 21 +++++++++++++++++++++
>   2 files changed, 32 insertions(+), 1 deletion(-)

Can you please separate the udevng changes and xmm7xxx changes into 
separate commits?

> 
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index ff6e1fc..e00b6ac 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1179,7 +1179,7 @@ static gboolean setup_gemalto(struct modem_info* modem)
>   
>   static gboolean setup_xmm7xxx(struct modem_info *modem)
>   {
> -	const char *mdm = NULL, *net = NULL;
> +	const char *mdm = NULL, *net = NULL, *net2 = NULL, *net3 = NULL;
>   	GSList *list;
>   
>   	DBG("%s %s\n", __DATE__, __TIME__);
> @@ -1200,6 +1200,10 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
>   			} else if (g_strcmp0(info->subsystem, "net") == 0) {
>   				if (g_strcmp0(info->number, "06") == 0)
>   					net = info->devnode;
> +				if (g_strcmp0(info->number, "08") == 0)
> +					net2 = info->devnode;
> +				if (g_strcmp0(info->number, "0a") == 0)
> +					net3 = info->devnode;
>   			}
>   		} else {
>   			if (g_strcmp0(info->subsystem, "tty") == 0) {
> @@ -1219,6 +1223,12 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
>   
>   	ofono_modem_set_string(modem->modem, "Modem", mdm);
>   	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
> +	if (net2)
> +		ofono_modem_set_string(modem->modem, "NetworkInterface2", net2);
> +	if (net3)
> +		ofono_modem_set_string(modem->modem, "NetworkInterface3", net3);

doc/coding-style.txt item M1

> +	ofono_modem_set_string(modem->modem, "CtrlPath", "/USBCDC/0");
> +	ofono_modem_set_string(modem->modem, "DataPath", "/USBHS/NCM/");
>   
>   	return TRUE;
>   }
> diff --git a/plugins/xmm7xxx.c b/plugins/xmm7xxx.c
> index 237c62c..eaae4ad 100644
> --- a/plugins/xmm7xxx.c
> +++ b/plugins/xmm7xxx.c
> @@ -1269,6 +1269,7 @@ static void xmm7xxx_post_online(struct ofono_modem *modem)
>   	struct xmm7xxx_data *data = ofono_modem_get_data(modem);
>   	struct ofono_gprs *gprs;
>   	struct ofono_gprs_context *gc;
> +	const char *interface = NULL;
>   
>   	DBG("%p", modem);
>   
> @@ -1282,6 +1283,26 @@ static void xmm7xxx_post_online(struct ofono_modem *modem)
>   	if (gprs && gc)
>   		ofono_gprs_add_context(gprs, gc);
>   
> +	interface = ofono_modem_get_string(modem, "NetworkInterface2");
> +
> +	if (interface) {
> +		gc = ofono_gprs_context_create(modem, OFONO_VENDOR_XMM, "ifxmodem",
> +					data->chat);
> +
> +		if (gprs && gc)
> +			ofono_gprs_add_context(gprs, gc);
> +	}
> +
> +	interface = ofono_modem_get_string(modem, "NetworkInterface3");
> +
> +	if (interface) {
> +		gc = ofono_gprs_context_create(modem, OFONO_VENDOR_XMM, "ifxmodem",
> +					data->chat);
> +
> +		if (gprs && gc)
> +			ofono_gprs_add_context(gprs, gc);
> +	}
> +

So this looks fine, but there's one problem, how do you map the 
NetworkInterface to a given gprs_context?  Trying to use any sort of cid 
mapping isn't a great idea since the modem or network can assign these 
as well (e.g. for network initiated contexts).

I think we need to tweak the core to fix this particular issue.  I 
attached a proposed patch for this.  I haven't had a chance to test this 
fully yet, can you give it a quick review/spin?

The idea would be to simply invoke ofono_gprs_context_set_interface in 
the modem driver (e.g. setup code above) and skip this step in the 
actual gprs_context driver code.

>   	ofono_ims_create(modem, "xmm7modem", data->chat);
>   	ofono_netmon_create(modem, 0, "xmm7modem", data->chat);
>   }
> 

Regards,
-Denis

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gprs-Let-gprs_context-interface-be-settable-once.patch --]
[-- Type: text/x-patch, Size: 8639 bytes --]

>From 13c0e25eb22111ffa6af44d41c3d8d388538fdfb Mon Sep 17 00:00:00 2001
From: Denis Kenzior <denkenz@gmail.com>
Date: Mon, 11 Feb 2019 17:51:16 -0600
Subject: [PATCH] gprs: Let gprs_context interface be settable once

This patch allows a driver to set the interface only once, instead of at
every context activation.  The previous way was originally designed for
PPP and RAW_IP based contexts which would have a (potentially)
differently named interface after each context activation due to use of
TUN/TAP.  This also worked for static high-speed interface setups as
well, since these usually had a single interface only.

For devices that support multiple high-speed interfaces it would be
advantageous to have each gprs_context get an interface assignment right
in the modem driver and skip having to setup the interface on every
activation.
---
 src/gprs.c | 70 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 33 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 58a998ca..3dce001b 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -104,7 +104,6 @@ struct ipv6_settings {
 };
 
 struct context_settings {
-	char *interface;
 	struct ipv4_settings *ipv4;
 	struct ipv6_settings *ipv6;
 };
@@ -115,6 +114,7 @@ struct ofono_gprs_context {
 	ofono_bool_t inuse;
 	const struct ofono_gprs_context_driver *driver;
 	void *driver_data;
+	char *interface;
 	struct context_settings *settings;
 	struct ofono_atom *atom;
 };
@@ -322,12 +322,10 @@ static void context_settings_free(struct context_settings *settings)
 		g_free(settings->ipv6);
 		settings->ipv6 = NULL;
 	}
-
-	g_free(settings->interface);
-	settings->interface = NULL;
 }
 
 static void context_settings_append_ipv4(struct context_settings *settings,
+						const char *interface,
 						DBusMessageIter *iter)
 {
 	DBusMessageIter variant;
@@ -352,7 +350,7 @@ static void context_settings_append_ipv4(struct context_settings *settings,
 		goto done;
 
 	ofono_dbus_dict_append(&array, "Interface",
-				DBUS_TYPE_STRING, &settings->interface);
+				DBUS_TYPE_STRING, &interface);
 
 	/* If we have a Proxy, no other settings are relevant */
 	if (settings->ipv4->proxy) {
@@ -392,6 +390,7 @@ done:
 }
 
 static void context_settings_append_ipv4_dict(struct context_settings *settings,
+						const char *interface,
 						DBusMessageIter *dict)
 {
 	DBusMessageIter entry;
@@ -402,12 +401,13 @@ static void context_settings_append_ipv4_dict(struct context_settings *settings,
 
 	dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING, &key);
 
-	context_settings_append_ipv4(settings, &entry);
+	context_settings_append_ipv4(settings, interface, &entry);
 
 	dbus_message_iter_close_container(dict, &entry);
 }
 
 static void context_settings_append_ipv6(struct context_settings *settings,
+						const char *interface,
 						DBusMessageIter *iter)
 {
 	DBusMessageIter variant;
@@ -431,7 +431,7 @@ static void context_settings_append_ipv6(struct context_settings *settings,
 		goto done;
 
 	ofono_dbus_dict_append(&array, "Interface",
-				DBUS_TYPE_STRING, &settings->interface);
+				DBUS_TYPE_STRING, &interface);
 
 	if (settings->ipv6->ip)
 		ofono_dbus_dict_append(&array, "Address", DBUS_TYPE_STRING,
@@ -457,6 +457,7 @@ done:
 }
 
 static void context_settings_append_ipv6_dict(struct context_settings *settings,
+						const char *interface,
 						DBusMessageIter *dict)
 {
 	DBusMessageIter entry;
@@ -467,13 +468,14 @@ static void context_settings_append_ipv6_dict(struct context_settings *settings,
 
 	dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING, &key);
 
-	context_settings_append_ipv6(settings, &entry);
+	context_settings_append_ipv6(settings, interface, &entry);
 
 	dbus_message_iter_close_container(dict, &entry);
 }
 
 static void signal_settings(struct pri_context *ctx, const char *prop,
-		void (*append)(struct context_settings *, DBusMessageIter *))
+		void (*append)(struct context_settings *,
+					const char *, DBusMessageIter *))
 
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -481,6 +483,7 @@ static void signal_settings(struct pri_context *ctx, const char *prop,
 	DBusMessage *signal;
 	DBusMessageIter iter;
 	struct context_settings *settings;
+	const char *interface;
 
 	signal = dbus_message_new_signal(path,
 					OFONO_CONNECTION_CONTEXT_INTERFACE,
@@ -492,12 +495,15 @@ static void signal_settings(struct pri_context *ctx, const char *prop,
 	dbus_message_iter_init_append(signal, &iter);
 	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &prop);
 
-	if (ctx->context_driver)
+	if (ctx->context_driver) {
 		settings = ctx->context_driver->settings;
-	else
+		interface = ctx->context_driver->interface;
+	} else {
 		settings = NULL;
+		interface = NULL;
+	}
 
-	append(settings, &iter);
+	append(settings, interface, &iter);
 	g_dbus_send_message(conn, signal);
 }
 
@@ -680,18 +686,16 @@ static void pri_setproxy(const char *interface, const char *proxy)
 static void pri_reset_context_settings(struct pri_context *ctx)
 {
 	struct context_settings *settings;
-	char *interface;
+	const char *interface;
 	gboolean signal_ipv4;
 	gboolean signal_ipv6;
 
 	if (ctx->context_driver == NULL)
 		return;
 
+	interface = ctx->context_driver->interface;
 	settings = ctx->context_driver->settings;
 
-	interface = settings->interface;
-	settings->interface = NULL;
-
 	signal_ipv4 = settings->ipv4 != NULL;
 	signal_ipv6 = settings->ipv6 != NULL;
 
@@ -708,8 +712,6 @@ static void pri_reset_context_settings(struct pri_context *ctx)
 	}
 
 	pri_ifupdown(interface, FALSE);
-
-	g_free(interface);
 }
 
 static void pri_update_mms_context_settings(struct pri_context *ctx)
@@ -724,10 +726,10 @@ static void pri_update_mms_context_settings(struct pri_context *ctx)
 
 	DBG("proxy %s port %u", ctx->proxy_host, ctx->proxy_port);
 
-	pri_set_ipv4_addr(settings->interface, settings->ipv4->ip);
+	pri_set_ipv4_addr(gc->interface, settings->ipv4->ip);
 
 	if (ctx->proxy_host)
-		pri_setproxy(settings->interface, ctx->proxy_host);
+		pri_setproxy(gc->interface, ctx->proxy_host);
 }
 
 static void append_context_properties(struct pri_context *ctx,
@@ -739,6 +741,7 @@ static void append_context_properties(struct pri_context *ctx,
 	dbus_bool_t value;
 	const char *strvalue;
 	struct context_settings *settings;
+	const char *interface;
 
 	ofono_dbus_dict_append(dict, "Name", DBUS_TYPE_STRING, &name);
 
@@ -775,13 +778,16 @@ static void append_context_properties(struct pri_context *ctx,
 					DBUS_TYPE_STRING, &strvalue);
 	}
 
-	if (ctx->context_driver)
+	if (ctx->context_driver) {
 		settings = ctx->context_driver->settings;
-	else
+		interface = ctx->context_driver->interface;
+	} else {
 		settings = NULL;
+		interface = NULL;
+	}
 
-	context_settings_append_ipv4_dict(settings, dict);
-	context_settings_append_ipv6_dict(settings, dict);
+	context_settings_append_ipv4_dict(settings, interface, dict);
+	context_settings_append_ipv6_dict(settings, interface, dict);
 }
 
 static DBusMessage *pri_get_properties(DBusConnection *conn,
@@ -830,8 +836,8 @@ static void pri_activate_callback(const struct ofono_error *error, void *data)
 	__ofono_dbus_pending_reply(&ctx->pending,
 				dbus_message_new_method_return(ctx->pending));
 
-	if (gc->settings->interface != NULL) {
-		pri_ifupdown(gc->settings->interface, TRUE);
+	if (gc->interface != NULL) {
+		pri_ifupdown(gc->interface, TRUE);
 
 		if (ctx->type == OFONO_GPRS_CONTEXT_TYPE_MMS &&
 				gc->settings->ipv4)
@@ -922,8 +928,8 @@ static void pri_read_settings_callback(const struct ofono_error *error,
 
 	pri_ctx->active = TRUE;
 
-	if (gc->settings->interface != NULL) {
-		pri_ifupdown(gc->settings->interface, TRUE);
+	if (gc->interface != NULL) {
+		pri_ifupdown(gc->interface, TRUE);
 
 		pri_context_signal_settings(pri_ctx, gc->settings->ipv4 != NULL,
 						gc->settings->ipv6 != NULL);
@@ -1433,7 +1439,7 @@ static gboolean context_dbus_unregister(struct pri_context *ctx)
 
 	if (ctx->active == TRUE) {
 		const char *interface =
-			ctx->context_driver->settings->interface;
+			ctx->context_driver->interface;
 
 		if (ctx->type == OFONO_GPRS_CONTEXT_TYPE_MMS)
 			pri_set_ipv4_addr(interface, NULL);
@@ -2808,10 +2814,8 @@ enum ofono_gprs_context_type ofono_gprs_context_get_type(
 void ofono_gprs_context_set_interface(struct ofono_gprs_context *gc,
 					const char *interface)
 {
-	struct context_settings *settings = gc->settings;
-
-	g_free(settings->interface);
-	settings->interface = g_strdup(interface);
+	g_free(gc->interface);
+	gc->interface = g_strdup(interface);
 }
 
 void ofono_gprs_context_set_ipv4_address(struct ofono_gprs_context *gc,
-- 
2.13.5