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.
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
