Re: [PATCH] atmodem: CEREG support for LTE network status reporting in AT modem
Hi Denis 2011/2/22 Denis Kenzior > Hi, > > On 02/22/2011 10:08 AM, Soum, RedouaneX wrote: > > Hi guys, > > > > > >>> I agree with you , both bearers are almost similar.Minor difference i > >>> see are context managment (especially default context creation) and > some > >>> eps related spill over on other existing atoms (For ex SIM would not > >>> contain some ISIM (IMPU/IMPI)related stuff).My idea is seperate atoms > >>> solution would even work for legacy switch back(CSFB) too with a > minimal > >>> impact on exiting architecture.Your comments on these ideas would also > >>> very valuable here as i assume you have real modem unlike me. > >>> > >> My main concern is about LTE only modems, these ones would not register > gprs > >> atom so all stuff from gprs atom needs to be done in eps atom, plus > CEREG > >> and initial PDN. Than if you have a mix modem with 3G and LTE than all > this "stuff" > >> would be done twice without some additional logic. Sounds complicated to > me. > >> About initial PDN, acually I think it can be placed in gprs atom > >> too, it won't influence 3G modems at all and we have +CGEV: handling > there already > >> (maybe not the strongest argument but would make things easier). > > > > I agree, the differences between 2G/3G and LTE are not so big so it'll be > better if we keep the logic in the existing atoms. > > A lot of LTE AT commands were extended from 2G/3G to support LTE. > > > > A good approach would be to extend netreg and gprs atoms to handle lte > including initial default bearer (as part of attach procedure) and dedicated > bearers( secondary context in 2G/2G). > > > > For the concepts that are not present in 2G and 3G, such as IMS related > concepts then we can use a dedicated atom. > > One thing to keep in mind about LTE is that we're not only looking to > support GSM style networks. There are hybrid networks such as Verizon > which have CDMA/LTE mix. From an API point of view it might not make > sense to expose unneeded GSM details in such situations. > > There are also plenty of implementation details inside gprs atom > specific to gprs. So for ease of implementation it might be sensible to > have a separate LTE atom anyway, even if it still implements the exact > same API. We can factor out common context / bearer management into a > separate utility / atom if needed. > > Regards, > -Denis > ___ > ofono mailing list > ofono@ofono.org > http://lists.ofono.org/listinfo/ofono > Common gprs atom would be able to handle all situation: 3G only, 3G / LTE, LTE only, marking technology in Bearer property. Though it might be difficult to handle double registration of 3G and LTE at the same time for mix modems, if this is allowed. 3G only stuff in gprs seems to be only gprs_suspend during callback as in LTE call won't suspend connection, and attached property as LTE is always attached. Most of this atom is context related which is common. Fast look gives me this division of gprs: - gprs atom: suspend, attached, status handling - lte atom: status handling - context atom (common part for 3G and LTE): whole context handling, cid map, ~80% of current src/gprs.c, + some IMS stuff all would probably need to use netreg_watch In my opinion, combined gprs atom would be easier to do and probably enough, separate atoms would be more "looking into the future" like but I am not sure if this division is necessary. Br Tomasz ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] atmodem: CEREG support for LTE network status reporting in AT modem
2011/2/22 > > > > -Original Message- > > From: Tomasz Gregorek [mailto:tomasz.grego...@gmail.com] > > Sent: 22 February 2011 16:09 > > To: ofono@ofono.org > > Cc: Nayani Vijay > > Subject: Re: [PATCH] atmodem: CEREG support for LTE network > > status reporting in AT modem > > > > Hi Vijay > > > > > > 2011/2/22 > > > > > > > > > > > Subject: [PATCH] atmodem: CEREG support for LTE network > > > status reporting in AT modem > > > > > > [PATCH] atmodem: CEREG support for LTE network status > > > reporting in AT modem Tomasz Gregorek tomasz.gregorek at > > > gmail.com Thu Feb 17 19:52:45 PST 2011 > > > > > > * Previous message: [PATCH 2/5] bluetooth: add a > > > bluetoothd connect watch > > > * Next message: Problem with SIM lock states not showing > > > correctly in Ofono API. > > > * Messages sorted by: [ date ] [ thread ] [ > > subject ] [ author ] > > > > > > From: Tomasz Gregorek > > > > > > > > This is a proposal for CEREG support based on the AT modem. > > > Support in driver should work, though I have an issue > > with the core. > > > > > > The core has one gprs status currently. In case of having > > > second status for LTE, there is need of having two satuses, > > > one for each, 3G and LTE, or to combine those two into one. > > > > > > I took second approach as it leaves current oFono API, though > > > it is not perfect. > > > > > > I have been working on solution that comprises of > > separate eps atom and > > corresponding driver. Code has been tested against > > modified phonesim for > > eps.Will provide an RFC patch soon once I bring it to > > certain logical > > end. > > > > Regards, > > Vijay > > > > > > > > This is what I was thinking about too. > > For me, from status point of view, both networks look very > > similar, thats why I was thinking about using gprs atom / > > driver for status handling and create separate atom for QoS / IMS. > > > > I agree with you , both bearers are almost similar.Minor difference i > see are context managment (especially default context creation) and some > eps related spill over on other existing atoms (For ex SIM would not > contain some ISIM (IMPU/IMPI)related stuff).My idea is seperate atoms > solution would even work for legacy switch back(CSFB) too with a minimal > impact on exiting architecture.Your comments on these ideas would also > very valuable here as i assume you have real modem unlike me. > > My main concern is about LTE only modems, these ones would not register gprs atom so all stuff from gprs atom needs to be done in eps atom, plus CEREG and initial PDN. Than if you have a mix modem with 3G and LTE than all this "stuff" would be done twice without some additional logic. Sounds complicated to me. About initial PDN, acually I think it can be placed in gprs atom too, it won't influence 3G modems at all and we have +CGEV: handling there already (maybe not the strongest argument but would make things easier). > > I am at most interested in your solution. I know from Denis > > that this is what was agreed. > > > > Br > > Tomasz Gregorek > > > > Will submit the rfc patch and short design write up once i have code > ready. > Ok. More comments as soon as there will be some code. > > Br, > Vijay > ___ > ofono mailing list > ofono@ofono.org > http://lists.ofono.org/listinfo/ofono > Br Tomasz ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] atmodem: CEREG support for LTE network status reporting in AT modem
Hi Vijay 2011/2/22 > > > > Subject: [PATCH] atmodem: CEREG support for LTE network > > status reporting in AT modem > > > > [PATCH] atmodem: CEREG support for LTE network status > > reporting in AT modem Tomasz Gregorek tomasz.gregorek at > > gmail.com Thu Feb 17 19:52:45 PST 2011 > > > > * Previous message: [PATCH 2/5] bluetooth: add a > > bluetoothd connect watch > > * Next message: Problem with SIM lock states not showing > > correctly in Ofono API. > > * Messages sorted by: [ date ] [ thread ] [ subject ] [ author ] > > > > From: Tomasz Gregorek > > > > This is a proposal for CEREG support based on the AT modem. > > Support in driver should work, though I have an issue with the core. > > > > The core has one gprs status currently. In case of having > > second status for LTE, there is need of having two satuses, > > one for each, 3G and LTE, or to combine those two into one. > > > > I took second approach as it leaves current oFono API, though > > it is not perfect. > > I have been working on solution that comprises of separate eps atom and > corresponding driver. Code has been tested against modified phonesim for > eps.Will provide an RFC patch soon once I bring it to certain logical > end. > > Regards, > Vijay > This is what I was thinking about too. For me, from status point of view, both networks look very similar, thats why I was thinking about using gprs atom / driver for status handling and create separate atom for QoS / IMS. I am at most interested in your solution. I know from Denis that this is what was agreed. Br Tomasz Gregorek ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] atmodem: CEREG support for LTE network status reporting in AT modem
From: Tomasz Gregorek
This is a proposal for CEREG support based on the AT modem.
Support in driver should work, though I have an issue with
the core.
The core has one gprs status currently. In case of having
second status for LTE, there is need of having two satuses,
one for each, 3G and LTE, or to combine those two into one.
I took second approach as it leaves current oFono API, though
it is not perfect.
---
drivers/atmodem/gprs.c | 151 +++
drivers/isimodem/gprs.c |6 +-
include/gprs.h | 11 +++-
src/gprs.c | 43 -
4 files changed, 189 insertions(+), 22 deletions(-)
diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index 6e01994..f6585de 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -43,12 +43,15 @@
#include "vendor.h"
static const char *cgreg_prefix[] = { "+CGREG:", NULL };
+static const char *cereg_prefix[] = { "+CEREG:", NULL };
static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
static const char *none_prefix[] = { NULL };
struct gprs_data {
GAtChat *chat;
unsigned int vendor;
+ ofono_bool_t have_cgreg;
+ ofono_bool_t have_cereg;
};
static void at_cgatt_cb(gboolean ok, GAtResult *result, gpointer user_data)
@@ -80,6 +83,31 @@ static void at_gprs_set_attached(struct ofono_gprs *gprs,
int attached,
CALLBACK_WITH_FAILURE(cb, data);
}
+static void at_cereg_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct cb_data *cbd = user_data;
+ ofono_gprs_status_cb_t cb = cbd->cb;
+ struct ofono_error error;
+ int status;
+ struct gprs_data *gd = cbd->user;
+
+ decode_at_error(&error, g_at_result_final_response(result));
+
+ if (!ok) {
+ cb(&error, -1, GPRS_PS_STATUS_SOURCE_LTE, cbd->data);
+ return;
+ }
+
+ if (at_util_parse_reg(result, "+CEREG:", NULL, &status,
+ NULL, NULL, NULL, gd->vendor) == FALSE) {
+ CALLBACK_WITH_FAILURE(cb, -1,
+ GPRS_PS_STATUS_SOURCE_LTE, cbd->data);
+ return;
+ }
+
+ cb(&error, status, GPRS_PS_STATUS_SOURCE_LTE, cbd->data);
+}
+
static void at_cgreg_cb(gboolean ok, GAtResult *result, gpointer user_data)
{
struct cb_data *cbd = user_data;
@@ -91,17 +119,28 @@ static void at_cgreg_cb(gboolean ok, GAtResult *result,
gpointer user_data)
decode_at_error(&error, g_at_result_final_response(result));
if (!ok) {
- cb(&error, -1, cbd->data);
+ cb(&error, -1, GPRS_PS_STATUS_SOURCE_LTE, cbd->data);
return;
}
if (at_util_parse_reg(result, "+CGREG:", NULL, &status,
NULL, NULL, NULL, gd->vendor) == FALSE) {
- CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+ CALLBACK_WITH_FAILURE(cb, -1,
+ GPRS_PS_STATUS_SOURCE_3G, cbd->data);
+ g_free(cbd);
return;
}
- cb(&error, status, cbd->data);
+ cb(&error, status, GPRS_PS_STATUS_SOURCE_3G, cbd->data);
+
+ if (gd->have_cereg) {
+ if (g_at_chat_send(gd->chat, "AT+CEREG?", cereg_prefix,
+ at_cereg_cb, cbd, g_free) == FALSE) {
+ CALLBACK_WITH_FAILURE(cb, -1,
+ GPRS_PS_STATUS_SOURCE_LTE, cbd->data);
+ g_free(cbd);
+ }
+ }
}
static void at_gprs_registration_status(struct ofono_gprs *gprs,
@@ -132,9 +171,17 @@ static void at_gprs_registration_status(struct ofono_gprs
*gprs,
break;
}
- if (g_at_chat_send(gd->chat, "AT+CGREG?", cgreg_prefix,
- at_cgreg_cb, cbd, g_free) > 0)
+ if (gd->have_cgreg) {
+ if (g_at_chat_send(gd->chat, "AT+CGREG?", cgreg_prefix,
+ at_cgreg_cb, cbd, NULL) > 0)
return;
+ }
+ else {
+ if (g_at_chat_send(gd->chat, "AT+CEREG?", cereg_prefix,
+ at_cereg_cb, cbd, g_free) > 0)
+ return;
+ }
+
g_free(cbd);
@@ -151,7 +198,20 @@ static void cgreg_notify(GAtResult *result, gpointer
user_data)
NULL, NULL, NULL, gd->vendor) == FALSE)
return;
- ofono_gprs_status_notify(gprs, status);
+ ofono_gprs_status_notify(gprs, status, GPRS_PS_STATUS_SOURCE_3G);
+}
+
+static void cereg_notify(GAtResult *result, gpointer user_data)
+{
+ struct ofono_gprs *gprs = user_data;
+ int status;
+ struct gprs_data *gd = ofono_gprs_get_da
Re: [RFC PATCH v3] gprs: add function to handle activated context
Hi Redouane
Find some comments below.
2011/2/9 Soum, RedouaneX
> The purpose of the patch is to handle Network Initiated Context Activation
> and PDN connection request as part of EPS Attach procedure (LTE) in oFono
> core.
>
> In order to avoid issue regarding CID allocation :
> - driver should call the core using ofono_gprs_set_cid_range function to
> specify CID range for UE initiated PDN connection
> - driver could use a new function to check if the CID is already used or
> not : ofono_gprs_is_cid_used, usefull in case the modem is not using a
> specific range for UE initiated PDN connection.
> - driver will use a new function to notify the gprs atom of a new PDN
> connection with all the ip parameters from AT+CGCONTRDP (I can provide an
> update later to handle PCSCF and IMS Signaling Flag as well) commmand :
> ofono_gprs_context_activated
>
> ofono_gprs_context_activated is similar to ofono_gprs_context_deactivated.
> This function must be call by any gprs driver when :
> - a context has been activated without explicit request as part of attach
> procedure in LTE (PDN ACT X and X is a unused CID)
> - a context has been activated by the Network (NW PDN ACT X, NW ACT X,Y)
>
> Behavior of the function :
>
> List all the context and try to find correct APN with empty username and
> empty password.
> If such context is not found then create a new one with "Internet" type
> only if we are in LTE with no Active context.
> Otherwise deactivate the connection.
>
> For the context (found or created):
>Update the settings
>Set to active
>
> The function will also assign the context to a context driver.
> ---
> include/gprs-context.h |7 +++
> include/gprs.h |3 +
> src/gprs.c | 139
>
> src/idmap.c| 12
> src/idmap.h|1 +
> 5 files changed, 162 insertions(+), 0 deletions(-)
>
> diff --git a/include/gprs-context.h b/include/gprs-context.h
> index c29c0dc..e208b66 100644
> --- a/include/gprs-context.h
> +++ b/include/gprs-context.h
> @@ -76,6 +76,13 @@ struct ofono_gprs_context_driver {
>ofono_gprs_context_cb_t cb, void
> *data);
> };
>
> +
> +void ofono_gprs_context_activated(struct ofono_gprs_context *gc,
> + const int cid, const char *apn,
> + const char *interface, ofono_bool_t static_ip,
> + const char *address, const char *netmask,
> + const char *gw, const char **dns);
> +
>
When this function is called we don't know *interface yet, as it is *gc
specific.
You look for a proper *gc_driver later on.
Maybe we should address found *gc_driver for a *interface?
Also looking at incoming IPv6 patches, maybe we should consider sending
connection data with separate functions?
Br
Tomasz Gregorek
ST-Ericsson
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: voicecall: behavior of ReleaseAndAnswere with held calls?
Hi Denis
2011/1/25 Denis Kenzior
> Hi Tomasz,
>
> On 01/25/2011 03:43 PM, Tomasz Gregorek wrote:
> > Hi Denis
> >
> > 2011/1/25 Denis Kenzior mailto:denk...@gmail.com>>
> >
> > Hi Tomasz,
> >
> > > Shouldn’t ReleaseAndAnswere() release the active call and bring
> > back the
> > > held one in such situation?
> > >
> >
> > You shouldn't be using ReleaseAndAnswer in this case, instead you
> should
> > use SwapCalls. SwapCalls has the added benefit of allowing swapping
> of
> > held and active calls even if there is a call waiting (if your modem
> > hardware supports this.)
> >
> >
> > SwapCalls wont release active call. This would be a case when we finished
> > active call and we want to disconnect and get back to the held one.
> > Though this can also be done with Release on active call followed
> > by SwapCalls.
> >
>
> Yep, Hangup or HangupMultiparty then SwapCalls. If you feel that a
> single operation to accomplish hangup + swap is required we can
> certainly consider it. For now it didn't pass our API is Minimal +
> Complete test. Perhaps ReleaseAndSwap()...?
>
I would go with easy solution by just changing src/voicecall.c line 1418
static DBusMessage *manager_release_and_answer(DBusConnection *conn,
DBusMessage *msg, void *data)
{
...
1418:
-if (!voicecalls_have_waiting(vc))
+if (!voicecalls_have_waiting(vc) && !voicecalls_have_with_status(vc,
CALL_STATUS_HELD))
return __ofono_error_failed(msg);
...
vc->driver->release_all_active(vc, generic_callback, vc);
return NULL;
}
This would give ReleaseAndAnswer full functionality of +CHLD=1.
(sorry for pseudo-patch, working temporary on windows machine)
>
> > >
> > > There also could be a little more description of behavior for a
> case
> > > when we have held and waiting calls saying that the waiting call
> > will be answered and
> > > that held won’t be released.
> > >
> >
> > The documentation says: "Releases currently active call and answers
> the
> > currently waiting call".
> >
> > Is this not enough? Can you suggest better wording?
> >
> >
> > If we are not touching held calls with this function than it is enough.
> > I would only add "if any exist" as this function works when there are
> held
> > and waiting calls.
> >
> > "Releases currently active call if any exists, and answers the
> > currently waiting call."
> >
>
> Fair enough, fixed with commit b937d99791abc8c33ef968be40f193f3985bca8d.
>
Thanks.
>
> Regards,
> -Denis
>
Br
Tomasz
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: voicecall: behavior of ReleaseAndAnswere with held calls?
Hi Denis 2011/1/25 Denis Kenzior > Hi Tomasz, > > > Shouldn’t ReleaseAndAnswere() release the active call and bring back the > > held one in such situation? > > > > You shouldn't be using ReleaseAndAnswer in this case, instead you should > use SwapCalls. SwapCalls has the added benefit of allowing swapping of > held and active calls even if there is a call waiting (if your modem > hardware supports this.) > SwapCalls wont release active call. This would be a case when we finished active call and we want to disconnect and get back to the held one. Though this can also be done with Release on active call followed by SwapCalls. > > > There also could be a little more description of behavior for a case > > when we have held and waiting calls saying that the waiting call will be > answered and > > that held won’t be released. > > > > The documentation says: "Releases currently active call and answers the > currently waiting call". > > Is this not enough? Can you suggest better wording? > If we are not touching held calls with this function than it is enough. I would only add "if any exist" as this function works when there are held and waiting calls. "Releases currently active call if any exists, and answers the currently waiting call." > > Regards, > -Denis > ___ > ofono mailing list > ofono@ofono.org > http://lists.ofono.org/listinfo/ofono > Best regards Tomasz ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
voicecall: behavior of ReleaseAndAnswere with held calls?
Hi I have a question about behavior of ReleaseAndAnswere(). If I am right, this function implements +CHLD=1 at command. Spec 22.030 says Entering 1 followed by SEND - Releases all active calls (if any exist) and accepts the other (held or waiting) call. oFono is failing on this command when there are held and active calls as there is only check if waiting call exists in manager_release_and_answere(). Shouldn't ReleaseAndAnswere() release the active call and bring back the held one in such situation? There also could be a little more description of behavior for a case when we have held and waiting calls saying that the waiting call will be answered and that held won't be released. Best regards Tomasz Gregorek ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] gprs: mark context driver as not used when removing active context
From: Tomasz Gregorek
---
src/gprs.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/gprs.c b/src/gprs.c
index 7ef81d5..0661f74 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1679,6 +1679,8 @@ static void gprs_deactivate_for_remove(const struct
ofono_error *error,
gprs_cid_release(gprs, ctx->context.cid);
ctx->context.cid = 0;
+ ctx->context_driver->inuse = FALSE;
+ ctx->context_driver = NULL;
if (gprs->settings) {
g_key_file_remove_group(gprs->settings, ctx->key, NULL);
--
1.7.1
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
[PATCH 2/2] plugins-mbm: Remove data->reopen_source timer before setting up new one
From: Tomasz Gregorek Check if there is a timer running already and remove it before creating a new one. This will prevent calling reopen_callback() more than if mbm_disconnect() is called more than once. --- plugins/mbm.c |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/plugins/mbm.c b/plugins/mbm.c index 38583ac..4048f6a 100644 --- a/plugins/mbm.c +++ b/plugins/mbm.c @@ -351,6 +351,9 @@ static void mbm_disconnect(gpointer user_data) data->data_port = NULL; /* Waiting for the +CGEV: ME DEACT might also work */ + if (data->reopen_source > 0) + g_source_remove(data->reopen_source); + data->reopen_source = g_timeout_add_seconds(1, reopen_callback, modem); } -- 1.7.1 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 1/2] plugins-mbm: Adding timer removal to mbm_remove()
From: Tomasz Gregorek
In case the modem is disconnected during enabling process, mbm_disconnect()
will set up data->reopen_source timer. This timer needs to be
removed in mbm_remove() to stop execution of reopen_callback() after
hardware disconnection.
---
plugins/mbm.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/plugins/mbm.c b/plugins/mbm.c
index dca9bd8..38583ac 100644
--- a/plugins/mbm.c
+++ b/plugins/mbm.c
@@ -91,6 +91,11 @@ static void mbm_remove(struct ofono_modem *modem)
DBG("%p", modem);
+ if (data->reopen_source > 0) {
+ g_source_remove(data->reopen_source);
+ data->reopen_source = 0;
+ }
+
ofono_modem_set_data(modem, NULL);
g_at_chat_unref(data->data_port);
--
1.7.1
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH] plugins-mbm: Combining mbm_disable and reopen_callback
Hi Sjur 2011/1/12 Sjur Brændeland > Hi Tomasz, don't worry the tone on the list is quite rough. If you > like marit or I can review tomorrow, but feel free to send patch as > well if you like. :) Sjur > > 2011/1/12, Tomasz Gregorek : > > Hi Marcel > > I already had my solution in head and misunderstood you. I will correct > and > > retest it. > > Br > > Tomasz > > > > 2011/1/12 Marcel Holtmann > > > >> Hi Tomasz, > >> > >> > Combining mbm_disable and reopen_callback into one and removing 1 > second > >> > delay between them to avoid call to reopen_callback after modem being > >> > disconnected. > >> > >> have you actually tested this? This will not work since you can not > >> reopen the TTY right away. You need that delay in between. > >> > >> From my previous email, I remember saying that the right fix would be to > >> disarm the timer within disable. Why are we not doing that? > >> > >> Regards > >> > >> Marcel > >> > >> > >> ___ > >> ofono mailing list > >> ofono@ofono.org > >> http://lists.ofono.org/listinfo/ofono > >> > > > I don't worry ;) Just assumed things too fast which led me to misunderstanding. Br Tomasz ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] plugins-mbm: Combining mbm_disable and reopen_callback
Hi Marcel I already had my solution in head and misunderstood you. I will correct and retest it. Br Tomasz 2011/1/12 Marcel Holtmann > Hi Tomasz, > > > Combining mbm_disable and reopen_callback into one and removing 1 second > > delay between them to avoid call to reopen_callback after modem being > > disconnected. > > have you actually tested this? This will not work since you can not > reopen the TTY right away. You need that delay in between. > > From my previous email, I remember saying that the right fix would be to > disarm the timer within disable. Why are we not doing that? > > Regards > > Marcel > > > ___ > ofono mailing list > ofono@ofono.org > http://lists.ofono.org/listinfo/ofono > ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] plugins-mbm: Combining mbm_disable and reopen_callback
From: Tomasz Gregorek
Combining mbm_disable and reopen_callback into one and removing 1 second
delay between them to avoid call to reopen_callback after modem being
disconnected.
---
plugins/mbm.c | 41 +++--
1 files changed, 11 insertions(+), 30 deletions(-)
diff --git a/plugins/mbm.c b/plugins/mbm.c
index dca9bd8..d1529ac 100644
--- a/plugins/mbm.c
+++ b/plugins/mbm.c
@@ -66,7 +66,6 @@ struct mbm_data {
gboolean have_sim;
struct ofono_gprs *gprs;
struct ofono_gprs_context *gc;
- guint reopen_source;
enum mbm_variant variant;
};
@@ -297,21 +296,26 @@ static GAtChat *create_port(const char *device)
return chat;
}
-static void mbm_disconnect(gpointer user_data);
-
-static gboolean reopen_callback(gpointer user_data)
+static void mbm_disconnect(gpointer user_data)
{
struct ofono_modem *modem = user_data;
struct mbm_data *data = ofono_modem_get_data(modem);
const char *data_dev;
- data->reopen_source = 0;
+ DBG("");
+
+ if (data->gc)
+ ofono_gprs_context_remove(data->gc);
+
+ g_at_chat_unref(data->data_port);
+ data->data_port = NULL;
data_dev = ofono_modem_get_string(modem, "DataDevice");
data->data_port = create_port(data_dev);
+
if (data->data_port == NULL)
- return FALSE;
+ return;
if (getenv("OFONO_AT_DEBUG"))
g_at_chat_set_debug(data->data_port, mbm_debug, "Data: ");
@@ -323,30 +327,12 @@ static gboolean reopen_callback(gpointer user_data)
data->gc = ofono_gprs_context_create(modem, 0,
"atmodem", data->data_port);
+
if (data->gprs && data->gc) {
ofono_gprs_context_set_type(data->gc,
OFONO_GPRS_CONTEXT_TYPE_MMS);
ofono_gprs_add_context(data->gprs, data->gc);
}
-
- return FALSE;
-}
-
-static void mbm_disconnect(gpointer user_data)
-{
- struct ofono_modem *modem = user_data;
- struct mbm_data *data = ofono_modem_get_data(modem);
-
- DBG("");
-
- if (data->gc)
- ofono_gprs_context_remove(data->gc);
-
- g_at_chat_unref(data->data_port);
- data->data_port = NULL;
-
- /* Waiting for the +CGEV: ME DEACT might also work */
- data->reopen_source = g_timeout_add_seconds(1, reopen_callback, modem);
}
static int mbm_enable(struct ofono_modem *modem)
@@ -425,11 +411,6 @@ static int mbm_disable(struct ofono_modem *modem)
DBG("%p", modem);
- if (data->reopen_source > 0) {
- g_source_remove(data->reopen_source);
- data->reopen_source = 0;
- }
-
if (data->modem_port == NULL)
return 0;
--
1.7.1
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
[BUG] [MBM] mbm_disconnect(..) and reopen_callback(..) gives oFono seg fault
Hi Marcel
I am using MBM modem in oFono and I have run into a problem with
reopen_callback(..) function.
I think you might be interested in looking into this as an author
of the code.
The situation is as follow:
1. connect modem to USB port
2. Power the modem up: Modem.SetProperty( Powered = True )
2a. oFono is in the middle of enabling process
3. disconnect modem from USB port
2b. oFono is still in the middle of enabling process, and modem is not marked
as enabled
4. mbm_disconnect(..) is called, gprs atom removed, reopen_callback(..) is set
to be called in 1 second delay with data->reopen_source timer
5. mbm_remove(..) is called, notice that at this point the mbm_disable(..)
function call is not called as modem is not enabled yet, and the
reopen_callback 1 second timer is still running.
6. timer expiries and as a result reopen_callback(..) is called. At this
point modem is already removed and function accesses an already
freed memory
7. oFono crashes with segmentation fault
Normally reopen_callback timer would be removed in mbm_disable(..) called
if enabling process has finished.
The easy solution is to add removal of reopen_callback timer in mbm_remove(..)
but I can imagine a race condition then. It might happen that during heavy load
time between mbm_disconnect(..) and mbm_disable(..) or mbm_remove(..) can
be longer than 1 second resulting in call to reopen_callback(..) during hardware
disconnection process. Though I am not sure what can happen here, if this will
result in segmentation fault or it would just execute without any side effects.
I have also noticed that other vendors don't go with delayed
reopen_collback(..) at all. They usually try to reopen directly
in mbm_disconnect(..).
Best regards
Tomasz Gregorek
ST-Ericsson
Below an example of log from oFono and some gdb data with source code of
reopen_callback(..) and mbm_disconnect(..) as reference.
[1]
ofonod[1735]: plugins/mbm.c:mbm_probe() 0x812ce58
ofonod[1735]: plugins/smart-messaging.c:modem_watch() modem: 0x812ce58, added: 1
ofonod[1735]: plugins/push-notification.c:modem_watch() modem: 0x812ce58,
added: 1
ofonod[1735]: plugins/mbm.c:mbm_enable() 0x812ce58
ofonod[1735]: src/modem.c:get_modem_property() modem 0x812ce58 property
ModemDevice
ofonod[1735]: src/modem.c:get_modem_property() modem 0x812ce58 property
DataDevice
[2] ofonod[1735]: plugins/mbm.c:mbm_enable() /dev/ttyACM1, /dev/ttyACM0
ofonod[1735]: Modem: > AT\r
ofonod[1735]: Data: > AT\r
ofonod[1735]: Data: < \r\nOK\r\n
ofonod[1735]: Data: > AT&F E0 V1 X4 &C1 +CMEE=1\r
ofonod[1735]: Data: < AT&F E0 V1 X4 &C1 +CMEE=1\r
ofonod[1735]: Data: < \r\nOK\r\n
[3]
[4] ofonod[1735]: plugins/mbm.c:mbm_disconnect() mbm_disconnect
**
ofonod[1735]: plugins/udev.c:remove_modem()
/devices/pci:00/:00:0b.0/usb1/1-1/1-1:1.1/tty/ttyACM0
ofonod[1735]: src/modem.c:get_modem_property() modem 0x812ce58 property Path
ofonod[1735]: src/modem.c:ofono_modem_remove() 0x812ce58
[5] ofonod[1735]: plugins/mbm.c:mbm_remove() 0x812ce58
ofonod[1735]: src/modem.c:unregister_property() property 0x812ded8
ofonod[1735]: src/modem.c:unregister_property() property 0x812d2d0
ofonod[1735]: src/modem.c:unregister_property() property 0x812c988
ofonod[1735]: src/modem.c:unregister_property() property 0x812d080
ofonod[1735]: src/modem.c:unregister_property() property 0x812cee0
ofonod[1735]: plugins/smart-messaging.c:modem_watch() modem: 0x812ce58, added: 0
ofonod[1735]: plugins/push-notification.c:modem_watch() modem: 0x812ce58,
added: 0
ofonod[1735]: plugins/udev.c:remove_modem()
/devices/pci:00/:00:0b.0/usb1/1-1/1-1:1.3/tty/ttyACM1
ofonod[1735]: plugins/udev.c:remove_modem()
/devices/pci:00/:00:0b.0/usb1/1-1/1-1:1.6/net/usb0
[6] ofonod[1735]: plugins/mbm.c:reopen_callback() reopen_callback
**
[7]
Program received signal SIGSEGV, Segmentation fault.
0x080a7927 in reopen_callback (user_data=0x812ce58) at plugins/mbm.c:318
318data->reopen_source = 0;
(gdb) p data
$1 = (struct mbm_data *) 0x2d303336
(gdb) p *data
Cannot access memory at address 0x2d303336
(gdb) p *modem
$2 = {path = 0x812e468 "", modem_state = 135453480, atoms = 0x2f6c6169,
atom_watches = 0x692d7962, interface_list = 0x73752f64,
feature_list = 0x54532d62, call_ids = 1769096493, pending = 0x6f737363,
interface_update = 1414750062, powered = 1769096493,
powered_pending = 1869837155, timeout = 1867341678, online = 1701603682,
online_watches = 0x6f72425f, properties = 0x61626461,
sim = 0x465f646e, sim_watch = 1160788025, sim_ready_watch = 876754743, driver
= 0x36454432, driver_data = 0x2d303336,
driver_type = 0x33306669 , name = 0x0}
(gdb) l 305,361
305 static void mbm_disconnect(gpointer user_data);
306
307 static gboolean reopen_callback(gpointer user_data)
308 {
309struct ofono_mode
