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
>From 13c0e25eb22111ffa6af44d41c3d8d388538fdfb Mon Sep 17 00:00:00 2001
From: Denis Kenzior <[email protected]>
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

_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono

Reply via email to