* Re: [PATCH 1/2] xmm7modem: Change in xmm7mode plugin for multi PDP handling
2019-02-06 8:37 Antara Borwankar
@ 2019-02-11 23:58 ` Denis Kenzior
0 siblings, 0 replies; 5+ messages in thread
From: Denis Kenzior @ 2019-02-11 23:58 UTC (permalink / raw)
To: ofono
[-- 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
^ permalink raw reply related [flat|nested] 5+ messages in thread