phablet-extras

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

  1. ofono-lp1204644
  2. Merge into ofono
Proposed by Tony Espy
Status: Merged
Approved by: Ricardo Salveti
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 (community) Approve
PS Jenkins bot continuous-integration Approve
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.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:66
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/63/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/38
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/38/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/63/rebuild

review: Approve (continuous-integration)
Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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'.

Revision history for this message
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.

lp:~awe/phablet-extras/ofono-lp1204644 updated
67. By Tony Espy

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:67
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/64/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/39
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/39/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/64/rebuild

review: Approve (continuous-integration)
lp:~awe/phablet-extras/ofono-lp1204644 updated
68. By Tony Espy

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:68
http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-ci/66/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/41
        deb: http://jenkins.qa.ubuntu.com/job/phablet-extras-ofono-saucy-armhf-ci/41/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/phablet-extras-ofono-ci/66/rebuild

review: Approve (continuous-integration)
Revision history for this message
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
=== modified file 'debian/changelog'
--- debian/changelog 2013-07-13 00:40:18 +0000
+++ debian/changelog 2013-07-28 19:23:26 +0000
@@ -1,3 +1,11 @@
1ofono (1.12phablet10) saucy; urgency=low
2
3 * [rilmodem] Fixed disable GPRS bug (LP:1204644).
4 * [gril/rilmodem] Re-factored RIL message code
5 and implemented related unit tests.
6
7 -- Tony Espy <espy@canonical.com> Fri, 26 Jul 2013 15:32:52 -0400
8
1ofono (1.12phablet9) saucy; urgency=low9ofono (1.12phablet9) saucy; urgency=low
210
3 * drivers/rilmodem/gprs.c: Fix roaming bug (LP: 1188404).11 * drivers/rilmodem/gprs.c: Fix roaming bug (LP: 1188404).
412
=== modified file 'drivers/rilmodem/gprs.c'
--- drivers/rilmodem/gprs.c 2013-07-10 23:08:34 +0000
+++ drivers/rilmodem/gprs.c 2013-07-28 19:23:26 +0000
@@ -62,9 +62,9 @@
6262
63struct gprs_data {63struct gprs_data {
64 GRil *ril;64 GRil *ril;
65 gboolean ofono_attached;
65 int max_cids;66 int max_cids;
66 int tech;67 int rild_status;
67 int status;
68};68};
6969
70static void ril_gprs_set_pref_network_cb(struct ril_msg *message,70static void ril_gprs_set_pref_network_cb(struct ril_msg *message,
@@ -110,19 +110,25 @@
110 ofono_gprs_cb_t cb, void *data)110 ofono_gprs_cb_t cb, void *data)
111{111{
112 struct cb_data *cbd = cb_data_new(cb, data);112 struct cb_data *cbd = cb_data_new(cb, data);
113 struct gprs_data *gd = ofono_gprs_get_data(gprs);
113 struct ofono_error error;114 struct ofono_error error;
114115
115 DBG("");116 DBG("attached: %d", attached);
116117
117 decode_ril_error(&error, "OK");118 decode_ril_error(&error, "OK");
118119
119 /* This code should just call the callback with OK, and be done120 /*
120 * there's no explicit RIL command to cause an attach.121 * As RIL offers no actual control over the GPRS 'attached'
122 * state, we save the desired state, and use it to override
123 * the actual modem's state in the 'attached_status' function.
124 * This is similar to the way the core ofono gprs code handles
125 * data roaming ( see src/gprs.c gprs_netreg_update().
121 *126 *
122 * The core gprs code calls driver->set_attached() when a netreg127 * The core gprs code calls driver->set_attached() when a netreg
123 * notificaiton is received and any configured roaming conditions128 * notificaiton is received and any configured roaming conditions
124 * are met.129 * are met.
125 */130 */
131 gd->ofono_attached = attached;
126132
127 cb(&error, cbd->data);133 cb(&error, cbd->data);
128 g_free(cbd);134 g_free(cbd);
@@ -135,6 +141,7 @@
135 struct ofono_gprs *gprs = cbd->user;141 struct ofono_gprs *gprs = cbd->user;
136 struct gprs_data *gd = ofono_gprs_get_data(gprs);142 struct gprs_data *gd = ofono_gprs_get_data(gprs);
137 struct ofono_error error;143 struct ofono_error error;
144 gboolean attached;
138 int status, lac, ci, tech;145 int status, lac, ci, tech;
139 int max_cids = 1;146 int max_cids = 1;
140147
@@ -157,7 +164,7 @@
157 goto error;164 goto error;
158 }165 }
159166
160 if (gd->status == -1)167 if (gd->rild_status == -1)
161 ofono_gprs_register(gprs);168 ofono_gprs_register(gprs);
162169
163 if (max_cids > gd->max_cids) {170 if (max_cids > gd->max_cids) {
@@ -166,8 +173,20 @@
166 ofono_gprs_set_cid_range(gprs, 1, max_cids);173 ofono_gprs_set_cid_range(gprs, 1, max_cids);
167 }174 }
168175
169 gd->status = status;176 gd->rild_status = status;
170 gd->tech = tech;177
178 /*
179 * Override the actual status based upon the desired
180 * attached status set by the core GPRS code ( controlled
181 * by the ConnnectionManager's 'Powered' property ).
182 */
183 attached = (status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
184 status == NETWORK_REGISTRATION_STATUS_ROAMING);
185
186 if (attached && gd->ofono_attached == FALSE) {
187 DBG("attached=true; ofono_attached=false; return !REGISTERED");
188 status = NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
189 }
171190
172error:191error:
173 if (cb)192 if (cb)
@@ -208,8 +227,9 @@
208 return -ENOMEM;227 return -ENOMEM;
209228
210 gd->ril = g_ril_clone(ril);229 gd->ril = g_ril_clone(ril);
230 gd->ofono_attached = FALSE;
211 gd->max_cids = 0;231 gd->max_cids = 0;
212 gd->status = -1;232 gd->rild_status = -1;
213233
214 ofono_gprs_set_data(gprs, gd);234 ofono_gprs_set_data(gprs, gd);
215235

Subscribers

People subscribed via source and target branches