Merge lp:~awe/phablet-extras/ofono-lp1204644 into lp:phablet-extras/ofono

Proposed by Tony Espy on 2013-07-26
Status: Merged
Approved by: Ricardo Salveti on 2013-07-29
Approved revision: 68
Merged at revision: 49
Proposed branch: lp:~awe/phablet-extras/ofono-lp1204644
Merge into: lp:phablet-extras/ofono
Prerequisite: lp:~awe/phablet-extras/ofono-unittest-gprs-context
Diff against target: 110 lines (+37/-9)
2 files modified
debian/changelog (+8/-0)
drivers/rilmodem/gprs.c (+29/-9)
To merge this branch: bzr merge lp:~awe/phablet-extras/ofono-lp1204644
Reviewer Review Type Date Requested Status
Ricardo Salveti 2013-07-26 Approve on 2013-07-29
PS Jenkins bot continuous-integration Approve on 2013-07-28
Review via email: mp+177211@code.launchpad.net

Commit message

[rilmodem] Fixes disable GPRS bug (LP: 1204644).

Description of the change

Fix a problem with disabling GPRS ( ie. setting the ConnectionManager's 'Powered' property to 0, which happens when the networking indicator's "mobile data" toggle is set to "off". This caused a tight loop between the core ofono code and RILD which caused the CPU to max out.

I tested by disabling network manager via an upstart override file.

Next I booted the phone and used the 'enable-gprs', 'activate-context', and 'disable-gprs' scripts to reproduce the same scenario. 'list-modems' and 'list-contexts' can be used to validate that GPRS is indeed detached, and that the data context gets cleaned up properly.

To post a comment you must log in.
Ricardo Salveti (rsalveti) wrote :

73 - gd->status = status;
74 - gd->tech = tech;
75 + gd->rild_status = status;
76 +
77 + attached = (status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
78 + status == NETWORK_REGISTRATION_STATUS_ROAMING);
79 +
80 + if (attached && gd->ofono_attached == FALSE) {
81 + DBG("attached=true; ofono_attached=false; return !REGISTERED");
82 + status = NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
83 + }
84

Not sure if I understand the logic properly here. Seems we're only tracking the ofono attached state, to make that work better from the software level, but isn't you "lying" when you're returning NOT_REGISTERED here?

Shouldn't we also set the modem state to reflect the returned value here? Guess this goes back to the airplane mode support.

Is ofono calling set_attached with 0 when you disable the connection from the network indicator? If so, we might probably want to change the modem state from there instead.

review: Needs Information
Tony Espy (awe) wrote :

I'm usually accused of "over-commenting" code, and in this case I failed... Let me try and explain, and I will add more comments to the code as well.

RIL offers no control whatsoever of the GPRS 'attached' state. It's automatic, unless of course you you switch the modem into manual network selection mode, or power the modem off.

Prior to this change, it was impossible to cause GPRS to go offline by setting the ConnectionManager's 'Powered' property to false. The core gprs codde ( /src/gprs.c ) would call the driver's 'set_attached' function which in our case always immediately invoked the callback with a success return code. This in turn caused the core code to invoke the driver's 'attached_status' function to see if the attach/detach operation succeeded. In the disable GPRS case, the core code has called 'set_attached' with 'attached=0', however the status function would then return the actual state which is still attached. This causes a tight loop, as ofono would again tries to disable GPRS, ad infinitum...

So, what I've done is borrowed a trick from ofono itself, and handled this problem similar to the way ofono handles roaming. 'gprs_data.ofono_attached' is onfono's desired attached state. If true, and the modem is actually attached, then our code returns true to the core. If it's false, then it essentially overrides the actual modem's state. Look at the function gprs_netreg_update() in /src/gprs.c to see how the roaming case is handled.

Think of the attached state as "use-able-for-data". When the upper layers see that the modem isn't attached, it prevents any data calls from being activated. If any calls ( note, we only support a single active call currently ) are active when 'Powered' is toggled, they'll be disconnected due to the core calling the gprs_context_driver's 'detach_shutdown' function.

Airplane mode will be handled slightly different in that ModemManager's 'Powered' property will be toggled as opposed to the ConnectionManager's property. That in turn will result in an actual call to RIL requesting the radio to be powered off. In the GPRS/ConnectionManager case, we're merely preventing any data usage; voice calls and non-MMS text messages still work.

Tony Espy (awe) wrote :

Note, to see the original bug in vivid detail, start ofonod in the foreground and set the debug flag to dump messages from gprs.c:

# ofonod -d */gprs.c -n -P atmodem

Now toggle the GPRS attached state either via the indicator toggle or via the ofono-script 'disable-gprs'.

Ricardo Salveti (rsalveti) wrote :

Right, thanks for the detailed explanation, makes sense now.

Will wait for the commit with more comments in there before approving the MR then.

67. By Tony Espy on 2013-07-27

[rilmodem] Added comments to explain rilmodem's GPRS attached logic.

68. By Tony Espy on 2013-07-28

Re-merge from lp:~awe/phablet-extras/ofono-unittest-gprs-context.

Ricardo Salveti (rsalveti) wrote :

Good, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2013-07-13 00:40:18 +0000
3+++ debian/changelog 2013-07-28 19:23:26 +0000
4@@ -1,3 +1,11 @@
5+ofono (1.12phablet10) saucy; urgency=low
6+
7+ * [rilmodem] Fixed disable GPRS bug (LP:1204644).
8+ * [gril/rilmodem] Re-factored RIL message code
9+ and implemented related unit tests.
10+
11+ -- Tony Espy <espy@canonical.com> Fri, 26 Jul 2013 15:32:52 -0400
12+
13 ofono (1.12phablet9) saucy; urgency=low
14
15 * drivers/rilmodem/gprs.c: Fix roaming bug (LP: 1188404).
16
17=== modified file 'drivers/rilmodem/gprs.c'
18--- drivers/rilmodem/gprs.c 2013-07-10 23:08:34 +0000
19+++ drivers/rilmodem/gprs.c 2013-07-28 19:23:26 +0000
20@@ -62,9 +62,9 @@
21
22 struct gprs_data {
23 GRil *ril;
24+ gboolean ofono_attached;
25 int max_cids;
26- int tech;
27- int status;
28+ int rild_status;
29 };
30
31 static void ril_gprs_set_pref_network_cb(struct ril_msg *message,
32@@ -110,19 +110,25 @@
33 ofono_gprs_cb_t cb, void *data)
34 {
35 struct cb_data *cbd = cb_data_new(cb, data);
36+ struct gprs_data *gd = ofono_gprs_get_data(gprs);
37 struct ofono_error error;
38
39- DBG("");
40+ DBG("attached: %d", attached);
41
42 decode_ril_error(&error, "OK");
43
44- /* This code should just call the callback with OK, and be done
45- * there's no explicit RIL command to cause an attach.
46+ /*
47+ * As RIL offers no actual control over the GPRS 'attached'
48+ * state, we save the desired state, and use it to override
49+ * the actual modem's state in the 'attached_status' function.
50+ * This is similar to the way the core ofono gprs code handles
51+ * data roaming ( see src/gprs.c gprs_netreg_update().
52 *
53 * The core gprs code calls driver->set_attached() when a netreg
54 * notificaiton is received and any configured roaming conditions
55 * are met.
56 */
57+ gd->ofono_attached = attached;
58
59 cb(&error, cbd->data);
60 g_free(cbd);
61@@ -135,6 +141,7 @@
62 struct ofono_gprs *gprs = cbd->user;
63 struct gprs_data *gd = ofono_gprs_get_data(gprs);
64 struct ofono_error error;
65+ gboolean attached;
66 int status, lac, ci, tech;
67 int max_cids = 1;
68
69@@ -157,7 +164,7 @@
70 goto error;
71 }
72
73- if (gd->status == -1)
74+ if (gd->rild_status == -1)
75 ofono_gprs_register(gprs);
76
77 if (max_cids > gd->max_cids) {
78@@ -166,8 +173,20 @@
79 ofono_gprs_set_cid_range(gprs, 1, max_cids);
80 }
81
82- gd->status = status;
83- gd->tech = tech;
84+ gd->rild_status = status;
85+
86+ /*
87+ * Override the actual status based upon the desired
88+ * attached status set by the core GPRS code ( controlled
89+ * by the ConnnectionManager's 'Powered' property ).
90+ */
91+ attached = (status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
92+ status == NETWORK_REGISTRATION_STATUS_ROAMING);
93+
94+ if (attached && gd->ofono_attached == FALSE) {
95+ DBG("attached=true; ofono_attached=false; return !REGISTERED");
96+ status = NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
97+ }
98
99 error:
100 if (cb)
101@@ -208,8 +227,9 @@
102 return -ENOMEM;
103
104 gd->ril = g_ril_clone(ril);
105+ gd->ofono_attached = FALSE;
106 gd->max_cids = 0;
107- gd->status = -1;
108+ gd->rild_status = -1;
109
110 ofono_gprs_set_data(gprs, gd);
111

Subscribers

People subscribed via source and target branches