phablet-extras

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

ofono-lp1204644
Merge into ofono

Proposed by Tony Espy on 2013-07-26

Status:

Merged

Approved by:

Ricardo Salveti on 2013-07-29

Approved revision:

Merged at revision:

Proposed branch:

lp://staging/~awe/phablet-extras/ofono-lp1204644

Merge into:

lp://staging/phablet-extras/ofono

Prerequisite:

lp://staging/~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://staging/~awe/phablet-extras/ofono-lp1204644

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Ricardo Salveti (community)		2013-07-26	Approve on 2013-07-29
PS Jenkins bot	continuous-integration		Approve on 2013-07-28
Review via email: mp+177211@code.staging.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.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-07-26:

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 on 2013-07-27:

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 on 2013-07-27:

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.

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 on 2013-07-27:

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 on 2013-07-27:

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://staging/~awe/phablet-extras/ofono-lp1204644 updated on 2013-07-28