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

Proposed by Tony Espy on 2013-07-13
Status: Merged
Approved by: Ricardo Salveti on 2013-07-18
Approved revision: 48
Merged at revision: 47
Proposed branch: lp:~awe/phablet-extras/ofono-lp1188404
Merge into: lp:phablet-extras/ofono
Diff against target: 67 lines (+7/-28) 2 files modified
To merge this branch: bzr merge lp:~awe/phablet-extras/ofono-lp1188404
Reviewer Review Type Date Requested Status
Martin Pitt Approve on 2013-07-18
Ricardo Salveti 2013-07-13 Approve on 2013-07-18
PS Jenkins bot continuous-integration Approve on 2013-07-13
Tiago Salem Herrmann 2013-07-18 Pending
Review via email: mp+174516@code.launchpad.net

Commit Message

[rilmodem] Fix GPRS tight loop when roaming (LP: 1188404).

Description of the Change

This changes fixes a bug when roaming which created a tight loop.

The callback for a RIL_REQ_DATA_REGISTRATION_STATE called ofono_grps_status_notify() in addition to configured callback.
The status_notify call in turn triggered another DATA_REGISTRATION_STATE request, ...

I also discovered that there's no need to listen for an UNSOL_VOICE_NETWORK_STATE_CHANGE in order to fire REQ_DATA_REGISTRATION_STATE requests either, as this is already if necessary by the netreg watch logic in the core src/gprs.c code.

I tested the code on a mako with an image from earlier this week, by modifying the code to always return a status of ROAMING if REGISTERED. I will attach a diff in a comment to the MR.

My test cases, were non-roaming, roaming without 'AllowRoaming' set, and roaming with 'AllowRoaming' set. All work as expected now.

To post a comment you must log in.
Tony Espy (awe) wrote :

The following patch will force the status to ROAMING, instead of REGISTERED:

=== modified file 'drivers/rilmodem/gprs.c'
--- drivers/rilmodem/gprs.c 2013-07-10 23:08:34 +0000
+++ drivers/rilmodem/gprs.c 2013-07-13 00:50:38 +0000
@@ -166,6 +166,12 @@
   ofono_gprs_set_cid_range(gprs, 1, max_cids);
  }

+ if (status == NETWORK_REGISTRATION_STATUS_REGISTERED) {
+ DBG("converting status to ROAMING!");
+ status = NETWORK_REGISTRATION_STATUS_ROAMING;
+ }
+
+ /* Do we need to save either???*/
  gd->status = status;
  gd->tech = tech;

=== modified file 'drivers/rilmodem/network-registration.c'
--- drivers/rilmodem/network-registration.c 2013-06-14 16:11:01 +0000
+++ drivers/rilmodem/network-registration.c 2013-07-13 00:50:20 +0000
@@ -104,6 +104,11 @@
   return;
  }

+ if (status == NETWORK_REGISTRATION_STATUS_REGISTERED) {
+ DBG("converting status to ROAMING!");
+ status = NETWORK_REGISTRATION_STATUS_ROAMING;
+ }
+
  nd->tech = tech;
  cb(&error, status, lac, ci, tech, cbd->data);
 }
@@ -118,6 +123,11 @@
                 return;
         }

+ if (status == NETWORK_REGISTRATION_STATUS_REGISTERED) {
+ DBG("converting status to ROAMING!");
+ status = NETWORK_REGISTRATION_STATUS_ROAMING;
+ }
+
  ofono_netreg_status_notify(netreg, status, lac, ci, tech);
 }

Ricardo Salveti (rsalveti) wrote :

Looks good, would just like this to be tested by one of the bug reporters so we know it really fixed the issue.

Adding Tiago as reviewer, and will top approve once he's able to test the MR.

review: Approve
Martin Pitt (pitti) wrote :

I have a nexus 4 with a SIM card, and for me the effect is even worse: right after boot rild takes > 80% CPU, and ofonod > 10%. In -d I also see this neverending stream of RIL_REQUEST_DATA_REGISTRATION_STATE, which suggests that my problem might be the same as this one.

I built lp:phablet-extras/ofono with this branch merged, and confirm that rild and ofonod are now tame, and making calls still works fine. Thanks!

review: Approve

Preview Diff

1=== modified file 'debian/changelog'
2--- debian/changelog 2013-07-08 17:19:11 +0000
3+++ debian/changelog 2013-07-13 00:50:37 +0000
4@@ -1,3 +1,9 @@
5+ofono (1.12phablet9) saucy; urgency=low
6+
7+ * drivers/rilmodem/gprs.c: Fix roaming bug (LP: 1188404).
8+
9+ -- Tony Espy <espy@canonical.com> Fri, 12 Jul 2013 20:38:32 -0400
10+
11 ofono (1.12phablet8) saucy; urgency=low
12
13 * [rilmodem] Added SIM PIN/PUK support. This is
14
15=== modified file 'drivers/rilmodem/gprs.c'
16--- drivers/rilmodem/gprs.c 2013-06-13 13:31:46 +0000
17+++ drivers/rilmodem/gprs.c 2013-07-13 00:50:37 +0000
18@@ -67,25 +67,6 @@
19 int status;
20 };
21
22-static void ril_gprs_registration_status(struct ofono_gprs *gprs,
23- ofono_gprs_status_cb_t cb,
24- void *data);
25-
26-static void ril_gprs_state_change(struct ril_msg *message, gpointer user_data)
27-{
28- struct ofono_gprs *gprs = user_data;
29-
30- if (message->req != RIL_UNSOL_RESPONSE_VOICE_NETWORK_STATE_CHANGED) {
31- ofono_error("ril_gprs_state_change: invalid message received %d",
32- message->req);
33- return;
34- }
35-
36- /* receipt of tihs event is already logged in network-registration */
37-
38- ril_gprs_registration_status(gprs, NULL, NULL);
39-}
40-
41 static void ril_gprs_set_pref_network_cb(struct ril_msg *message,
42 gpointer user_data)
43 {
44@@ -176,23 +157,15 @@
45 goto error;
46 }
47
48- if (gd->status == -1) {
49+ if (gd->status == -1)
50 ofono_gprs_register(gprs);
51
52- g_ril_register(gd->ril, RIL_UNSOL_RESPONSE_VOICE_NETWORK_STATE_CHANGED,
53- ril_gprs_state_change, gprs);
54- }
55-
56 if (max_cids > gd->max_cids) {
57 DBG("Setting max cids to %d", max_cids);
58 gd->max_cids = max_cids;
59 ofono_gprs_set_cid_range(gprs, 1, max_cids);
60 }
61
62- if (gd->status != status) {
63- ofono_gprs_status_notify(gprs, status);
64- }
65-
66 gd->status = status;
67 gd->tech = tech;
68

Subscribers

People subscribed via source and target branches