Merge lp://staging/~phablet-team/network-manager/lp1445080 into lp://staging/~phablet-team/network-manager/vivid-phone-overlay

Proposed by Tony Espy
Status: Merged
Approved by: Alfonso Sanchez-Beato
Approved revision: 965
Merged at revision: 964
Proposed branch: lp://staging/~phablet-team/network-manager/lp1445080
Merge into: lp://staging/~phablet-team/network-manager/vivid-phone-overlay
Diff against target: 708 lines (+688/-0)
3 files modified
debian/changelog (+9/-0)
debian/patches/lp1445080-fix-modem-flight-mode.patch (+678/-0)
debian/patches/series (+1/-0)
To merge this branch: bzr merge lp://staging/~phablet-team/network-manager/lp1445080
Reviewer Review Type Date Requested Status
Alfonso Sanchez-Beato Approve
Mathieu Trudel-Lapierre Approve
Review via email: mp+262782@code.staging.launchpad.net

Commit message

  * debian/patches/lp1445080-fix-modem-flight-mode.patch: re-work
    NMModemOfono's modem state logic to get rid of a race condition
    that can cause flight-mode to fail and leave the modem stuck
    with no active connections (LP: #1445080).

Description of the change

This change re-factors the modem_state handling code in NMModemOfono to solve underlying race conditions in the handling of flight-mode.

To post a comment you must log in.
Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

Looks really good, although I have some comments.

review: Needs Fixing
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

Looks fine to me; please fix the issues Alfonso mentioned, but I didn't find anything else that jumped out.

review: Approve
Revision history for this message
Tony Espy (awe) wrote :

Thanks for the review. Please take a look at my replies. I'll address the formatting issues and re-push shortly.

965. By Tony Espy

Fix formatting: /lp1445080-fix-modem-flight-mode.patch

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

LGTM

review: Approve
966. By Tony Espy

Add check for existence of MODEM_CLASS set_mm_enabled function.

Revision history for this message
Tony Espy (awe) wrote :

Tested mako (rc/ubuntu #1) and started seeing off behavior while running the test cases:

https://wiki.ubuntu.com/Process/Merges/TestPlans/network-manager

Discovered a new NM crash file, and quickly found a bug. NMModem doesn't check for a value set_mm_enabled function before calling it, and this function was just removed from NMModemOfono as part of this change.

I just modified nm-modem-ofono.c to add a check for this function before invoking. Note, I opted to do this vs. re-add the function, as the function is essentially a no-op, as it's called when a Killswitch signal is received from URfkill for the modem. At the point the signal is received, there's really nothing else remaining to be done, other than wait for the modem state to settle ( which is handled by NMModemOfono ).

Revision history for this message
Tony Espy (awe) wrote :

Re-tested mako (rc/ubuntu #1) with the new version of network-manager (15.1.4) uploaded to silo 20.

Re-ran all of the tests from:

https://wiki.ubuntu.com/Process/Merges/TestPlans/network-manager

...and made sure to repeat the boot, flight-mode and set-radio tech cases a minimum of five times each.

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

Tested on krillin, as per https://wiki.ubuntu.com/Process/Merges/TestPlans/network-manager . Using latest ofono in github to avoid deactivation bug. All was fine.

$ system-image-cli -i
current build number: 51
device name: krillin
channel: ubuntu-touch/rc-proposed/bq-aquaris.en
last update: 2015-06-29 08:31:01
version version: 51
version ubuntu: 20150629
version device: 20150529-8e13c5f
version custom: 20150528-722-29-15-vivid

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

I have discovered one failure when switching data from SIM2 to SIM1. NM was activating the connection, but the network was not reachable. The interface was up and the context active, but "ip route" showed no routes.

Revision history for this message
Tony Espy (awe) wrote :

Re-tested arale (rc-proposed/ubuntu #37) with the new version of network-manager (15.1.4) uploaded to silo 20.

Again, ran all of the scenarios from the NM test plan. Ran 10 iterations of FlightMode with no problems.

That said I ran into two problems, neither of which I believe are regressions:

1. WiFi DNS Lag: When WiFi is activated when mobile data is currently active, DNS lookups can fail for the first 30-90s.

2. When radio technology is set and/or a forced disconnect happens, mobile data may not be restored.
[ This is due to a race condition that still exists between the NMModemOfono logic, and the NMOfono settings plugin. I will be working on solving this as part of the 'SetPreferred' change for the APN Editor. ]

I also ran into one indicator issue, if the system is connected to a WiFi access point, and the AP is "forgotten" via System Settings::WiFi, NM does the right thing and switches over to mobile data, however the indicator still shows WiFi as 'connected'.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: