3:46 a.m.
From: Guillaume Lucas <guillaumex.lucas(a)intel.com>
---
plugins/ifx.c | 83 ++++++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 65 insertions(+), 18 deletions(-)
diff --git a/plugins/ifx.c b/plugins/ifx.c
index 527a8c4..9c2a5c1 100644
--- a/plugins/ifx.c
+++ b/plugins/ifx.c
@@ -26,8 +26,12 @@
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
+#include <string.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
+#include <linux/types.h>
+#include <linux/gsmmux.h>
+#include <unistd.h>
#include <glib.h>
#include <gatchat.h>
@@ -75,9 +79,9 @@
static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ",
"GPRS1: ",
"GPRS2: ", "GPRS3: ", "Aux: " };
-static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1",
"/dev/ttyGSM2",
- "/dev/ttyGSM3", "/dev/ttyGSM4",
- "/dev/ttyGSM5", "/dev/ttyGSM6" };
+static const char *dlc_nodes[NUM_DLC] = { "/dev/gsmtty1",
"/dev/gsmtty2",
+ "/dev/gsmtty3", "/dev/gsmtty4",
+ "/dev/gsmtty5", "/dev/gsmtty6" };
static const char *none_prefix[] = { NULL };
static const char *xdrv_prefix[] = { "+XDRV:", NULL };
@@ -392,6 +396,7 @@ static gboolean dlc_ready_check(gpointer user_data)
struct ifx_data *data = ofono_modem_get_data(modem);
struct stat st;
int i;
+ GHashTable *options;
DBG("");
@@ -405,8 +410,14 @@ static gboolean dlc_ready_check(gpointer user_data)
return TRUE;
}
+ options = g_hash_table_new(g_str_hash, g_str_equal);
+ if (options == NULL)
+ goto error;
+
+ g_hash_table_insert(options, "Local", "on");
+
for (i = 0; i < NUM_DLC; i++) {
- GIOChannel *channel = g_at_tty_open(dlc_nodes[i], NULL);
+ GIOChannel *channel = g_at_tty_open(dlc_nodes[i], options);
data->dlcs[i] = create_chat(channel, modem, dlc_prefixes[i]);
if (data->dlcs[i] == NULL) {
@@ -415,6 +426,8 @@ static gboolean dlc_ready_check(gpointer user_data)
}
}
+ g_hash_table_destroy(options);
+
data->dlc_poll_source = 0;
/* iterate through mainloop */
@@ -455,7 +468,7 @@ static void setup_internal_mux(struct ofono_modem *modem)
ofono_error("Failed to create channel");
goto error;
}
- }
+ }
/* wait for DLC creation to settle */
data->dlc_init_source = g_timeout_add(10, dlc_setup, modem);
@@ -471,7 +484,9 @@ static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer
user_data)
{
struct ofono_modem *modem = user_data;
struct ifx_data *data = ofono_modem_get_data(modem);
- int fd;
+ int err;
+ struct gsm_config cfg;
+ int fd = -1;
DBG("");
@@ -504,6 +519,37 @@ static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer
user_data)
goto error;
}
+ memset(&cfg, 0, sizeof(struct gsm_config));
+
+ err = ioctl(fd, GSMIOC_GETCONF, &cfg);
+ if (err < 0) {
+ ofono_error("Failed to get multiplexer configuration: %d", err);
+ goto error;
+ }
+
+ ofono_info("Got default configuration...");
+ ofono_info("adaption = %d", cfg.adaption);
+ ofono_info("encapsulation = %d", cfg.encapsulation);
+ ofono_info("initiator = %d", cfg.initiator);
+ ofono_info("t1 = %d", cfg.t1);
+ ofono_info("t2 = %d", cfg.t2);
+ ofono_info("t3 = %d", cfg.t3);
+ ofono_info("n2 = %d", cfg.n2);
+ ofono_info("mru = %d", cfg.mru);
+ ofono_info("mtu = %d", cfg.mtu);
+ ofono_info("k = %d", cfg.k);
+
+ cfg.encapsulation = 0; /* encoding - set to basic */
+ cfg.initiator = 1; /* we are starting side */
+ cfg.mru = 1500;
+ cfg.mtu = 1500;
+
+ err = ioctl(fd, GSMIOC_SETCONF, &cfg);
+ if (err < 0) {
+ ofono_error("Failed to configure n_gsm multiplexer: %d", err);
+ goto error;
+ }
+
data->dlc_poll_count = 0;
data->dlc_poll_source = g_timeout_add_seconds(1, dlc_ready_check,
modem);
@@ -511,6 +557,9 @@ static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer
user_data)
return;
error:
+ if (ioctl(fd, TIOCSETD, &data->saved_ldisc) < 0)
+ ofono_warn("Failed to restore line discipline");
+
data->saved_ldisc = -1;
g_io_channel_unref(data->device);
@@ -593,9 +642,9 @@ static int ifx_enable(struct ofono_modem *modem)
NULL, NULL, NULL);
/* Enable multiplexer */
- data->frame_size = 1509;
+ data->frame_size = 1500;
- g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
+ g_at_chat_send(chat, "AT+CMUX=0,0,,1500,10,3,30,,", NULL,
mux_setup_cb, modem, NULL);
data->mux_init_timeout = g_timeout_add_seconds(5, mux_timeout_cb,
@@ -732,22 +781,20 @@ static void ifx_post_online(struct ofono_modem *modem)
if (gprs == NULL)
return;
- if (data->mux_ldisc < 0) {
- gc = ofono_gprs_context_create(modem, 0,
+ gc = ofono_gprs_context_create(modem, 0,
"ifxmodem", data->dlcs[GPRS1_DLC]);
- if (gc)
- ofono_gprs_add_context(gprs, gc);
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
- gc = ofono_gprs_context_create(modem, 0,
+ gc = ofono_gprs_context_create(modem, 0,
"ifxmodem", data->dlcs[GPRS2_DLC]);
- if (gc)
+ if (gc)
ofono_gprs_add_context(gprs, gc);
- gc = ofono_gprs_context_create(modem, 0,
+ gc = ofono_gprs_context_create(modem, 0,
"ifxmodem", data->dlcs[GPRS3_DLC]);
- if (gc)
- ofono_gprs_add_context(gprs, gc);
- }
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
}
static struct ofono_modem_driver ifx_driver = {
--
1.6.3.3
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
8:46 a.m.
Hi Lucas,
diff --git a/plugins/ifx.c b/plugins/ifx.c
index 527a8c4..9c2a5c1 100644
--- a/plugins/ifx.c
+++ b/plugins/ifx.c
@@ -26,8 +26,12 @@
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
+#include <string.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
+#include <linux/types.h>
+#include <linux/gsmmux.h>
+#include <unistd.h>
please move unistd.h up since as a rule of thumb, the system headers
should be included before the unix ones.
Also gsmmux.h is not acceptable. We have to carry a local copy for a
while since otherwise compilation would fail on older kernels.
Why do you need linux/types.h?
#include <glib.h>
#include <gatchat.h>
@@ -75,9 +79,9 @@
static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ",
"GPRS1: ",
"GPRS2: ", "GPRS3: ", "Aux: " };
-static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1",
"/dev/ttyGSM2",
- "/dev/ttyGSM3", "/dev/ttyGSM4",
- "/dev/ttyGSM5", "/dev/ttyGSM6" };
+static const char *dlc_nodes[NUM_DLC] = { "/dev/gsmtty1",
"/dev/gsmtty2",
+ "/dev/gsmtty3", "/dev/gsmtty4",
+ "/dev/gsmtty5", "/dev/gsmtty6" };
I am not a big fan of these hardcoded names. I was not a big fan for the
original MUX code and that has not changed.
So what happens if you have to multiplexer running at the same time?
static const char *none_prefix[] = { NULL };
static const char *xdrv_prefix[] = { "+XDRV:", NULL };
@@ -392,6 +396,7 @@ static gboolean dlc_ready_check(gpointer user_data)
struct ifx_data *data = ofono_modem_get_data(modem);
struct stat st;
int i;
+ GHashTable *options;
DBG("");
@@ -405,8 +410,14 @@ static gboolean dlc_ready_check(gpointer user_data)
return TRUE;
}
+ options = g_hash_table_new(g_str_hash, g_str_equal);
+ if (options == NULL)
+ goto error;
+
+ g_hash_table_insert(options, "Local", "on");
+
Why is this needed?
for (i = 0; i < NUM_DLC; i++) {
- GIOChannel *channel = g_at_tty_open(dlc_nodes[i], NULL);
+ GIOChannel *channel = g_at_tty_open(dlc_nodes[i], options);
data->dlcs[i] = create_chat(channel, modem, dlc_prefixes[i]);
if (data->dlcs[i] == NULL) {
@@ -415,6 +426,8 @@ static gboolean dlc_ready_check(gpointer user_data)
}
}
+ g_hash_table_destroy(options);
+
data->dlc_poll_source = 0;
/* iterate through mainloop */
@@ -455,7 +468,7 @@ static void setup_internal_mux(struct ofono_modem *modem)
ofono_error("Failed to create channel");
goto error;
}
- }
+ }
/* wait for DLC creation to settle */
data->dlc_init_source = g_timeout_add(10, dlc_setup, modem);
@@ -471,7 +484,9 @@ static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer
user_data)
{
struct ofono_modem *modem = user_data;
struct ifx_data *data = ofono_modem_get_data(modem);
- int fd;
+ int err;
+ struct gsm_config cfg;
+ int fd = -1;
DBG("");
@@ -504,6 +519,37 @@ static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer
user_data)
goto error;
}
+ memset(&cfg, 0, sizeof(struct gsm_config));
+
+ err = ioctl(fd, GSMIOC_GETCONF, &cfg);
+ if (err < 0) {
+ ofono_error("Failed to get multiplexer configuration: %d", err);
+ goto error;
+ }
+
+ ofono_info("Got default configuration...");
+ ofono_info("adaption = %d", cfg.adaption);
+ ofono_info("encapsulation = %d", cfg.encapsulation);
+ ofono_info("initiator = %d", cfg.initiator);
+ ofono_info("t1 = %d", cfg.t1);
+ ofono_info("t2 = %d", cfg.t2);
+ ofono_info("t3 = %d", cfg.t3);
+ ofono_info("n2 = %d", cfg.n2);
+ ofono_info("mru = %d", cfg.mru);
+ ofono_info("mtu = %d", cfg.mtu);
+ ofono_info("k = %d", cfg.k);
This pretty heavy debug information and not ofono_info material.
+ cfg.encapsulation = 0; /* encoding - set to basic */
+ cfg.initiator = 1; /* we are starting side */
+ cfg.mru = 1500;
+ cfg.mtu = 1500;
+
+ err = ioctl(fd, GSMIOC_SETCONF, &cfg);
+ if (err < 0) {
+ ofono_error("Failed to configure n_gsm multiplexer: %d", err);
+ goto error;
+ }
+
data->dlc_poll_count = 0;
data->dlc_poll_source = g_timeout_add_seconds(1, dlc_ready_check,
modem);
@@ -511,6 +557,9 @@ static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer
user_data)
return;
error:
+ if (ioctl(fd, TIOCSETD, &data->saved_ldisc) < 0)
+ ofono_warn("Failed to restore line discipline");
+
This is not right. Even if you fail GSMIOC_GETCONF you try to restore
the line discipline. That makes no sense.
data->saved_ldisc = -1;
g_io_channel_unref(data->device);
@@ -593,9 +642,9 @@ static int ifx_enable(struct ofono_modem *modem)
NULL, NULL, NULL);
/* Enable multiplexer */
- data->frame_size = 1509;
+ data->frame_size = 1500;
- g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
+ g_at_chat_send(chat, "AT+CMUX=0,0,,1500,10,3,30,,", NULL,
mux_setup_cb, modem, NULL);
So why did the frame_size changed here? Does the modem firmware changed
or is this actual a bug in oFono already today?
data->mux_init_timeout = g_timeout_add_seconds(5,
mux_timeout_cb,
@@ -732,22 +781,20 @@ static void ifx_post_online(struct ofono_modem *modem)
if (gprs == NULL)
return;
- if (data->mux_ldisc < 0) {
- gc = ofono_gprs_context_create(modem, 0,
+ gc = ofono_gprs_context_create(modem, 0,
"ifxmodem", data->dlcs[GPRS1_DLC]);
- if (gc)
- ofono_gprs_add_context(gprs, gc);
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
- gc = ofono_gprs_context_create(modem, 0,
+ gc = ofono_gprs_context_create(modem, 0,
"ifxmodem", data->dlcs[GPRS2_DLC]);
- if (gc)
+ if (gc)
ofono_gprs_add_context(gprs, gc);
- gc = ofono_gprs_context_create(modem, 0,
+ gc = ofono_gprs_context_create(modem, 0,
"ifxmodem", data->dlcs[GPRS3_DLC]);
- if (gc)
- ofono_gprs_add_context(gprs, gc);
- }
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
}
We can not just do this unconditionally. Then using the internal MUX
will not work anymore.
Regards
Marcel
8:48 p.m.
New subject: [PATCH 1/3] ifx: integration of kernel mux (n_gsm) and its line discipline
Hi Marcel,
Hi Lucas,
> diff --git a/plugins/ifx.c b/plugins/ifx.c
> index 527a8c4..9c2a5c1 100644
> --- a/plugins/ifx.c
> +++ b/plugins/ifx.c
> @@ -26,8 +26,12 @@
> #include <stdio.h>
> #include <errno.h>
> #include <stdlib.h>
> +#include <string.h>
> #include <sys/stat.h>
> #include <sys/ioctl.h>
> +#include <linux/types.h>
> +#include <linux/gsmmux.h>
> +#include <unistd.h>
please move unistd.h up since as a rule of thumb, the system headers
should be included before the unix ones.
Also gsmmux.h is not acceptable. We have to carry a local copy for a
while since otherwise compilation would fail on older kernels.
Ok. It's what Robertino did in his first patch for the kernel mux integration.
I've changed that thinking it was better to used the gsmmux.h of the kernel tree.
I'll redo that.
Why do you need linux/types.h?
Do not really remember :(
I think this is for the gsmmux.h file but if it's the case it to this file to include
this one. I'll review that.
> #include <glib.h>
> #include <gatchat.h>
> @@ -75,9 +79,9 @@
> static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ",
"GPRS1:
",
> "GPRS2: ", "GPRS3: ", "Aux: " };
>
> -static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1",
"/dev/ttyGSM2",
> - "/dev/ttyGSM3", "/dev/ttyGSM4",
> - "/dev/ttyGSM5", "/dev/ttyGSM6" };
> +static const char *dlc_nodes[NUM_DLC] = { "/dev/gsmtty1",
"/dev/gsmtty2",
> + "/dev/gsmtty3", "/dev/gsmtty4",
> + "/dev/gsmtty5", "/dev/gsmtty6" };
I am not a big fan of these hardcoded names. I was not a big fan for
the original MUX code and that has not changed.
So what happens if you have to multiplexer running at the same time?
No idea. I'll try to analyze this point.
> static const char *none_prefix[] = { NULL };
> static const char *xdrv_prefix[] = { "+XDRV:", NULL };
> @@ -392,6 +396,7 @@ static gboolean dlc_ready_check(gpointer
user_data)
> struct ifx_data *data = ofono_modem_get_data(modem);
> struct stat st;
> int i;
> + GHashTable *options;
>
> DBG("");
>
> @@ -405,8 +410,14 @@ static gboolean dlc_ready_check(gpointer
user_data)
> return TRUE;
> }
>
> + options = g_hash_table_new(g_str_hash, g_str_equal);
> + if (options == NULL)
> + goto error;
> +
> + g_hash_table_insert(options, "Local", "on");
> +
Why is this needed?
This is needed for PDP context deactivation.
If you don't set the local to on a tty close occurs when you do the PDP context
deactivation.
> +
> + ofono_info("Got default configuration...");
> + ofono_info("adaption = %d", cfg.adaption);
> + ofono_info("encapsulation = %d", cfg.encapsulation);
> + ofono_info("initiator = %d", cfg.initiator);
> + ofono_info("t1 = %d", cfg.t1);
> + ofono_info("t2 = %d", cfg.t2);
> + ofono_info("t3 = %d", cfg.t3);
> + ofono_info("n2 = %d", cfg.n2);
> + ofono_info("mru = %d", cfg.mru);
> + ofono_info("mtu = %d", cfg.mtu);
> + ofono_info("k = %d", cfg.k);
This pretty heavy debug information and not ofono_info material.
Agree. I kept them from the original patch of Robertino but I'll change that.
> + cfg.encapsulation = 0; /* encoding - set to basic */
> + cfg.initiator = 1; /* we are starting side */
> + cfg.mru = 1500;
> + cfg.mtu = 1500;
> +
> + err = ioctl(fd, GSMIOC_SETCONF, &cfg);
> + if (err < 0) {
> + ofono_error("Failed to configure n_gsm multiplexer: %d",
err);
> + goto error;
> + }
> +
> data->dlc_poll_count = 0;
> data->dlc_poll_source = g_timeout_add_seconds(1, dlc_ready_check,
> modem);
> @@ -511,6 +557,9 @@ static void mux_setup_cb(gboolean ok, GAtResult
*result, gpointer user_data)
> return;
>
> error:
> + if (ioctl(fd, TIOCSETD, &data->saved_ldisc) < 0)
> + ofono_warn("Failed to restore line discipline");
> +
This is not right. Even if you fail GSMIOC_GETCONF you try to restore
the line discipline. That makes no sense.
Agree that is not right but not for the GSMIOC_GETCONF failing.
It's not right because I try to restore the line discipline even if the ioctl who
gives it failed. I'll review that.
> data->saved_ldisc = -1;
>
> g_io_channel_unref(data->device);
> @@ -593,9 +642,9 @@ static int ifx_enable(struct ofono_modem *modem)
> NULL, NULL, NULL);
>
> /* Enable multiplexer */
> - data->frame_size = 1509;
> + data->frame_size = 1500;
>
> - g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
> + g_at_chat_send(chat, "AT+CMUX=0,0,,1500,10,3,30,,", NULL,
> mux_setup_cb, modem, NULL);
So why did the frame_size changed here? Does the modem firmware changed
or is this actual a bug in oFono already today?
It's a bug in oFono.
> data->mux_init_timeout = g_timeout_add_seconds(5,
mux_timeout_cb,
> @@ -732,22 +781,20 @@ static void ifx_post_online(struct ofono_modem
*modem)
> if (gprs == NULL)
> return;
>
> - if (data->mux_ldisc < 0) {
> - gc = ofono_gprs_context_create(modem, 0,
> + gc = ofono_gprs_context_create(modem, 0,
> "ifxmodem", data->dlcs[GPRS1_DLC]);
> - if (gc)
> - ofono_gprs_add_context(gprs, gc);
> + if (gc)
> + ofono_gprs_add_context(gprs, gc);
>
> - gc = ofono_gprs_context_create(modem, 0,
> + gc = ofono_gprs_context_create(modem, 0,
> "ifxmodem", data->dlcs[GPRS2_DLC]);
> - if (gc)
> + if (gc)
> ofono_gprs_add_context(gprs, gc);
>
> - gc = ofono_gprs_context_create(modem, 0,
> + gc = ofono_gprs_context_create(modem, 0,
> "ifxmodem", data->dlcs[GPRS3_DLC]);
> - if (gc)
> - ofono_gprs_add_context(gprs, gc);
> - }
> + if (gc)
> + ofono_gprs_add_context(gprs, gc);
> }
We can not just do this unconditionally. Then using the internal MUX
will not work anymore.
Don't understand your point. This part of code is done in both cases: using internal
MUX or not.
The check (data->mux_ldisc < 0) is TRUE when you are using the internal MUX.
Regards,
Guillaume
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
9:04 a.m.
Hi Guillaume,
> > diff --git a/plugins/ifx.c b/plugins/ifx.c
> > index 527a8c4..9c2a5c1 100644
> > --- a/plugins/ifx.c
> > +++ b/plugins/ifx.c
> > @@ -26,8 +26,12 @@
> > #include <stdio.h>
> > #include <errno.h>
> > #include <stdlib.h>
> > +#include <string.h>
> > #include <sys/stat.h>
> > #include <sys/ioctl.h>
> > +#include <linux/types.h>
> > +#include <linux/gsmmux.h>
> > +#include <unistd.h>
>
> please move unistd.h up since as a rule of thumb, the system headers
> should be included before the unix ones.
>
> Also gsmmux.h is not acceptable. We have to carry a local copy for a
> while since otherwise compilation would fail on older kernels.
Ok. It's what Robertino did in his first patch for the kernel mux integration.
I've changed that thinking it was better to used the gsmmux.h of the kernel tree.
I'll redo that.
>
> Why do you need linux/types.h?
>
Do not really remember :(
I think this is for the gsmmux.h file but if it's the case it to this file to include
this one. I'll review that.
in theory if gsmmux.h requires linux/types.h, then it should include it.
> > #include <glib.h>
> > #include <gatchat.h>
> > @@ -75,9 +79,9 @@
> > static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ",
"GPRS1:
> ",
> > "GPRS2: ", "GPRS3: ", "Aux: " };
> >
> > -static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1",
> "/dev/ttyGSM2",
> > - "/dev/ttyGSM3", "/dev/ttyGSM4",
> > - "/dev/ttyGSM5", "/dev/ttyGSM6" };
> > +static const char *dlc_nodes[NUM_DLC] = { "/dev/gsmtty1",
> "/dev/gsmtty2",
> > + "/dev/gsmtty3", "/dev/gsmtty4",
> > + "/dev/gsmtty5", "/dev/gsmtty6" };
>
> I am not a big fan of these hardcoded names. I was not a big fan for
> the original MUX code and that has not changed.
>
> So what happens if you have to multiplexer running at the same time?
>
No idea. I'll try to analyze this point.
This needs a bit more thinking here. Also in reality the names are not
guaranteed. It might have happened that udev renamed them.
The current code has exactly the same problem. I know that is an issue,
it just happens to be that the current code is not used anyway.
> > static const char *none_prefix[] = { NULL };
> > static const char *xdrv_prefix[] = { "+XDRV:", NULL };
> > @@ -392,6 +396,7 @@ static gboolean dlc_ready_check(gpointer
> user_data)
> > struct ifx_data *data = ofono_modem_get_data(modem);
> > struct stat st;
> > int i;
> > + GHashTable *options;
> >
> > DBG("");
> >
> > @@ -405,8 +410,14 @@ static gboolean dlc_ready_check(gpointer
> user_data)
> > return TRUE;
> > }
> >
> > + options = g_hash_table_new(g_str_hash, g_str_equal);
> > + if (options == NULL)
> > + goto error;
> > +
> > + g_hash_table_insert(options, "Local", "on");
> > +
>
> Why is this needed?
This is needed for PDP context deactivation.
If you don't set the local to on a tty close occurs when you do the PDP context
deactivation.
That sounds like a kernel bug to me as well. Why is the kernel closing
the TTY if you switch back to AT command mode. We are still holding an
open file descriptor to it.
> > + ofono_info("Got default configuration...");
> > + ofono_info("adaption = %d", cfg.adaption);
> > + ofono_info("encapsulation = %d", cfg.encapsulation);
> > + ofono_info("initiator = %d", cfg.initiator);
> > + ofono_info("t1 = %d", cfg.t1);
> > + ofono_info("t2 = %d", cfg.t2);
> > + ofono_info("t3 = %d", cfg.t3);
> > + ofono_info("n2 = %d", cfg.n2);
> > + ofono_info("mru = %d", cfg.mru);
> > + ofono_info("mtu = %d", cfg.mtu);
> > + ofono_info("k = %d", cfg.k);
>
> This pretty heavy debug information and not ofono_info material.
>
Agree. I kept them from the original patch of Robertino but I'll change that.
> > + cfg.encapsulation = 0; /* encoding - set to basic */
> > + cfg.initiator = 1; /* we are starting side */
> > + cfg.mru = 1500;
> > + cfg.mtu = 1500;
> > +
> > + err = ioctl(fd, GSMIOC_SETCONF, &cfg);
> > + if (err < 0) {
> > + ofono_error("Failed to configure n_gsm multiplexer: %d",
> err);
> > + goto error;
> > + }
> > +
> > data->dlc_poll_count = 0;
> > data->dlc_poll_source = g_timeout_add_seconds(1, dlc_ready_check,
> > modem);
> > @@ -511,6 +557,9 @@ static void mux_setup_cb(gboolean ok, GAtResult
> *result, gpointer user_data)
> > return;
> >
> > error:
> > + if (ioctl(fd, TIOCSETD, &data->saved_ldisc) < 0)
> > + ofono_warn("Failed to restore line discipline");
> > +
>
> This is not right. Even if you fail GSMIOC_GETCONF you try to restore
> the line discipline. That makes no sense.
>
Agree that is not right but not for the GSMIOC_GETCONF failing.
It's not right because I try to restore the line discipline even if the ioctl who
gives it failed. I'll review that.
> > data->saved_ldisc = -1;
> >
> > g_io_channel_unref(data->device);
> > @@ -593,9 +642,9 @@ static int ifx_enable(struct ofono_modem *modem)
> > NULL, NULL, NULL);
> >
> > /* Enable multiplexer */
> > - data->frame_size = 1509;
> > + data->frame_size = 1500;
> >
> > - g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
> > + g_at_chat_send(chat, "AT+CMUX=0,0,,1500,10,3,30,,", NULL,
> > mux_setup_cb, modem, NULL);
>
> So why did the frame_size changed here? Does the modem firmware changed
> or is this actual a bug in oFono already today?
>
It's a bug in oFono.
Then lets get that fixed separately with a proper commit message
explaining why using frame size of 1500 is correct.
We need a proper track record of changes and don't intermix them.
> > data->mux_init_timeout = g_timeout_add_seconds(5,
mux_timeout_cb,
> > @@ -732,22 +781,20 @@ static void ifx_post_online(struct ofono_modem
> *modem)
> > if (gprs == NULL)
> > return;
> >
> > - if (data->mux_ldisc < 0) {
> > - gc = ofono_gprs_context_create(modem, 0,
> > + gc = ofono_gprs_context_create(modem, 0,
> > "ifxmodem", data->dlcs[GPRS1_DLC]);
> > - if (gc)
> > - ofono_gprs_add_context(gprs, gc);
> > + if (gc)
> > + ofono_gprs_add_context(gprs, gc);
> >
> > - gc = ofono_gprs_context_create(modem, 0,
> > + gc = ofono_gprs_context_create(modem, 0,
> > "ifxmodem", data->dlcs[GPRS2_DLC]);
> > - if (gc)
> > + if (gc)
> > ofono_gprs_add_context(gprs, gc);
> >
> > - gc = ofono_gprs_context_create(modem, 0,
> > + gc = ofono_gprs_context_create(modem, 0,
> > "ifxmodem", data->dlcs[GPRS3_DLC]);
> > - if (gc)
> > - ofono_gprs_add_context(gprs, gc);
> > - }
> > + if (gc)
> > + ofono_gprs_add_context(gprs, gc);
> > }
>
> We can not just do this unconditionally. Then using the internal MUX
> will not work anymore.
Don't understand your point. This part of code is done in both cases: using internal
MUX or not.
The check (data->mux_ldisc < 0) is TRUE when you are using the internal MUX.
I see that you do an extra set of checks in the gprs-context.c. I need
to think about that a little bit.
Regards
Marcel
8:35 p.m.
New subject: [PATCH 1/3] ifx: integration of kernel mux (n_gsm) and its line discipline
Hi Marcel,
Hi Guillaume,
> > > diff --git a/plugins/ifx.c b/plugins/ifx.c
> > > index 527a8c4..9c2a5c1 100644
> > > --- a/plugins/ifx.c
> > > +++ b/plugins/ifx.c
> > > @@ -26,8 +26,12 @@
> > > #include <stdio.h>
> > > #include <errno.h>
> > > #include <stdlib.h>
> > > +#include <string.h>
> > > #include <sys/stat.h>
> > > #include <sys/ioctl.h>
> > > +#include <linux/types.h>
> > > +#include <linux/gsmmux.h>
> > > +#include <unistd.h>
> >
> > please move unistd.h up since as a rule of thumb, the system headers
> > should be included before the unix ones.
> >
> > Also gsmmux.h is not acceptable. We have to carry a local copy for a
> > while since otherwise compilation would fail on older kernels.
>
> Ok. It's what Robertino did in his first patch for the kernel mux integration.
> I've changed that thinking it was better to used the gsmmux.h of the kernel
tree.
> I'll redo that.
>
> >
> > Why do you need linux/types.h?
> >
>
> Do not really remember :(
> I think this is for the gsmmux.h file but if it's the case it to this file to
> include this one. I'll review that.
in theory if gsmmux.h requires linux/types.h, then it should include
it.
I've asked the team who handle this file to add this missing include.
> > >
> > > + options = g_hash_table_new(g_str_hash, g_str_equal);
> > > + if (options == NULL)
> > > + goto error;
> > > +
> > > + g_hash_table_insert(options, "Local", "on");
> > > +
> >
> > Why is this needed?
>
> This is needed for PDP context deactivation.
> If you don't set the local to on a tty close occurs when you do the PDP context
deactivation.
That sounds like a kernel bug to me as well. Why is the kernel closing
the TTY if you switch back to AT command mode. We are still holding an
open file descriptor to it.
It's the kernel mux driver that makes a TTY hang-up when the "Local" option
is set to OFF.
The TTY hang-up is done because the modem line TIOCM_CD moves from 0 to 1 (DV bit of the
modem status command set to 1).
It's what I've seen in the code, I'm not enough expert of 27.010 and TTY to be
able to say if this correct or not.
Regarding the SPEC the mapping done in the kernel mux between 27.010 control signals and
TTY signals seems correct for me.
> > >
> > > /* Enable multiplexer */
> > > - data->frame_size = 1509;
> > > + data->frame_size = 1500;
> > >
> > > - g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
> > > + g_at_chat_send(chat, "AT+CMUX=0,0,,1500,10,3,30,,", NULL,
> > > mux_setup_cb, modem, NULL);
> >
> > So why did the frame_size changed here? Does the modem firmware changed
> > or is this actual a bug in oFono already today?
> >
>
> It's a bug in oFono.
Then lets get that fixed separately with a proper commit message
explaining why using frame size of 1500 is correct.
We need a proper track record of changes and don't intermix them.
I'll do that.
Regards,
Guillaume
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
4206
days inactive
4211
days old
4 comments
2 participants
participants (2)
-
Lucas, GuillaumeX -
Marcel Holtmann