Merge lp:~phablet-team/ofono/lp1430700 into lp:~phablet-team/ofono/ubuntu

[Tony Espy]

 [ Alfonso Sanchez-Beato ]
  * mtk.c: Fix Set3g FlightMode regression on krillin (LP: #1430700 )

This fixes a regression introduced by the Set3G slot feature, which causes krillin's modem to become wedged if Set3G is called for the second slot when no SIM is inserted in the first slot.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:6890
http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-ci/67/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-vivid-amd64-ci/5
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-vivid-armhf-ci/5
        deb: http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-vivid-armhf-ci/5/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-vivid-i386-ci/5

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/phablet-team-ofono-ubuntu-ci/67/rebuild

review: Approve (continuous-integration)

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

review: Approve

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Tested on krillin. Bug #1430700 is fixed, urfkill test plan passes:

https://wiki.ubuntu.com/Process/Merges/TestPlans/urfkill

(with the exception of test 9, that will be solved when a recent urfkill fix lands). Basic telephony sanity test performed too (mobile data, phone calls).

$ system-image-cli -i
current build number: 152
device name: krillin
channel: ubuntu-touch/vivid-proposed
last update: 2015-03-18 15:06:18
version version: 152
version ubuntu: 20150318.1
version device: 20150310-ae1ddec
version custom: 20150318.1

[Tony Espy (awe)]

As this change is specific to MTK devices only, my testing was performed on krillin/vivid/#152.

I had two T-Mobile SIMs installed; /ril_0 was a valid PIN-locked SIM, /ril_1 was an expired, unlocked SIM.

The code that was changed involves the MTK device plugin ( mtk.c ) and it's handling of power and SIM mode events/request/reply logic. As such I performed basic sanity testing of voice and data, and then focused on FlightMode.

Before I could even get to the bug scenario ( Set3g + FM ), I hammered on the indicator-network menu's FM switch a bit to see if I could get it to fail.

I managed to recreate bug #1336715 ( switch items in unity sometimes get out of sync with modem system settings ), an airplane was shown on the panel, however the switch showed FM disabled, but then after about 30s to 1m, the switch flipped back to enabled.

When I tried to reproduce this again, I ran into a situation where the modem wouldn't exit FlightMode. When I checked the state of the state of the modems, /ril_0 was offline, and /ril_1 was online.

Here's the output of list-modems:

[ /ril_1 ]
    Online = 1
    Lockdown = 0
    Emergency = 0
    Powered = 1
    Revision = MOLY.WR8.W1315.MD.WG.MP.V37.P5, 2014/05/15 11:49
    Manufacturer = Fake Manufacturer
    Features = rat sim
    Model = Fake Modem Model
    Serial = 354142060413640
    Type = hardware
    Interfaces = org.ofono.RadioSettings org.ofono.SimManager org.ofono.MtkSettings org.ofono.CallVolume org.ofono.VoiceCallManager org.ofono.NetworkTime
    [ org.ofono.RadioSettings ]
    [ org.ofono.SimManager ]
        PinRequired = none
        FixedDialing = 0
        CardIdentifier = 8901260981853207741
        LockedPins =
        Present = 1
        SubscriberNumbers =
        Retries =
        PreferredLanguages = en
        BarredDialing = 0
    [ org.ofono.MtkSettings ]
        Has3G = 0
    [ org.ofono.CallVolume ]
        SpeakerVolume = 0
        MicrophoneVolume = 0
        Muted = 0
    [ org.ofono.VoiceCallManager ]
        EmergencyNumbers = 112 911
    [ org.ofono.NetworkTime ]

[ /ril_0 ]
    Online = 0
    Lockdown = 0
    Emergency = 0
    Powered = 1
    Revision = MOLY.WR8.W1315.MD.WG.MP.V37.P5, 2014/05/15 11:49
    Manufacturer = Fake Manufacturer
    Features =
    Model = Fake Modem Model
    Serial = 354142060133644
    Type = hardware
    Interfaces = org.ofono.CallVolume org.ofono.VoiceCallManager org.ofono.NetworkTime
    [ org.ofono.CallVolume ]
        SpeakerVolume = 0
        MicrophoneVolume = 0
        Muted = 0
    [ org.ofono.VoiceCallManager ]
        EmergencyNumbers = 112 911
    [ org.ofono.NetworkTime ]

urfkill's flight-mode script reports enabled, and the saved-state file shows all devices ( including ALL ) with soft=true, so I don't think this is bug #1430700.

I saved the log file.

review: Needs Fixing

[Tony Espy (awe)]

Also, as this change affects all MTK devices, we need to make sure arale is tested too.

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Tony, thanks for the testing. I have taken a look at syslog and I do not see any obvious errors, all looks fine. All FM operations finish with:

urf_arbitrator_flight_mode_cb: flight-mode operation succeeded

I am wondering if you executed list-modems in the middle of a FM unset, because /ril_1 does not show interfaces that are created when the ofono SIM atom is ready (netreg, connection manager...), so it is not completely initialized yet. So maybe it had just been onlined and /ril_0 had not been onlined yet. It might be the case that indicator-network had pending FM operations and was still processing those. I am trying to reproduce anyway.

[Tony Espy (awe)]

Re-tested on krillin ( same image as above ) and couldn't reproduce the modem hang.

Also tested that the modem comes up on boot, and that FlightMode works correctly on arale:

current build number: 143
device name: m75
channel: ubuntu-touch/vivid-proposed
last update: 2015-03-20 17:36:17
version version: 143

=== modified file 'debian/changelog'
--- debian/changelog	2015-02-24 18:27:43 +0000
+++ debian/changelog	2015-03-17 00:10:59 +0000
@@ -1,3 +1,10 @@
+ofono (1.12.bzr6890+15.04.20150224-0ubuntu1) UNRELEASED; urgency=medium
+
+  [ Alfonso Sanchez-Beato ]
+  * mtk.c: Fix Set3g FlightMode regression on krillin (LP: #1430700 )
+
+ -- CI Train Bot <[e-mail]>  Mon, 16 Mar 2015 19:59:36 -0400
+
 ofono (1.12.bzr6888+15.04.20150224-0ubuntu1) vivid; urgency=medium
 
   [ Alfonso Sanchez-Beato ]

=== modified file 'plugins/mtk.c'
--- plugins/mtk.c	2015-02-09 10:57:46 +0000
+++ plugins/mtk.c	2015-03-17 00:10:59 +0000
@@ -89,6 +89,7 @@
 #define RILD_CONNECT_RETRY_TIME_S 5
 
 #define T_WAIT_DISCONN_MS 1000
+#define T_SIM_SWITCH_FAILSAFE_MS 1000
 
 enum mtk_sim_type {
 	MTK_SIM_TYPE_1 = 1,
@@ -167,6 +168,7 @@
 static void socket_disconnected(gpointer user_data);
 static void start_slot(struct mtk_data *md, struct socket_data *sock,
 			const char *hex_prefix);
+static void exec_pending_online(struct mtk_data *md);
 
 static void mtk_debug(const char *str, void *user_data)
 {
@@ -272,6 +274,30 @@
 	}
 }
 
+static void exec_online_callback(struct mtk_data *md)
+{
+	if (md->online_cb != NULL) {
+		/*
+		 * We assume success, as the sim switch request was successful
+		 * too. Also, the SIM_* states can be returned independently of
+		 * the radio state, so we cannot reliably use it to know if the
+		 * sim switch command really did what was requested.
+		 */
+		CALLBACK_WITH_SUCCESS(md->online_cb, md->online_data);
+		md->online_cb = NULL;
+		md->online_data = NULL;
+
+		if (md->ofono_online)
+			md->mtk_settings =
+				mtk_settings_create(md->modem, md->ril,
+							md->has_3g);
+		else
+			mtk_settings_remove(md->mtk_settings);
+
+		exec_pending_online(md);
+	}
+}
+
 static void mtk_radio_state_changed(struct ril_msg *message, gpointer user_data)
 {
 	struct ofono_modem *modem = user_data;
@@ -279,48 +305,28 @@
 	int radio_state = g_ril_unsol_parse_radio_state_changed(md->ril,
 								message);
 
-	if (radio_state != md->radio_state) {
-		gboolean success = FALSE;
-
-		ofono_info("%s, slot %d: state: %s md->ofono_online: %d",
-				__func__, md->slot,
-				ril_radio_state_to_string(radio_state),
-				md->ofono_online);
-
-		md->radio_state = radio_state;
-
-		switch (radio_state) {
-		case RADIO_STATE_ON:
-		/* Deprecated in AOSP, but still used by MTK */
-		case RADIO_STATE_SIM_NOT_READY:
-		case RADIO_STATE_SIM_LOCKED_OR_ABSENT:
-		case RADIO_STATE_SIM_READY:
-			if (md->ofono_online)
-				success = TRUE;
-			break;
-
-		case RADIO_STATE_UNAVAILABLE:
-		case RADIO_STATE_OFF:
-			if (!md->ofono_online)
-				success = TRUE;
-			break;
-		default:
-			/* Malformed parcel; no radio state == broken rild */
-			g_assert(FALSE);
-		}
-
-		if (md->online_cb != NULL) {
-			if (success)
-				CALLBACK_WITH_SUCCESS(md->online_cb,
-							md->online_data);
-			else
-				CALLBACK_WITH_FAILURE(md->online_cb,
-							md->online_data);
-
-			md->online_cb = NULL;
-			md->online_data = NULL;
-		}
+	ofono_info("%s, slot %d: state: %s md->ofono_online: %d",
+			__func__, md->slot,
+			ril_radio_state_to_string(radio_state),
+			md->ofono_online);
+
+	md->radio_state = radio_state;
+
+	switch (radio_state) {
+	case RADIO_STATE_ON:
+	/* RADIO_STATE_SIM_* are deprecated in AOSP, but still used by MTK */
+	case RADIO_STATE_SIM_NOT_READY:
+	case RADIO_STATE_SIM_LOCKED_OR_ABSENT:
+	case RADIO_STATE_SIM_READY:
+	case RADIO_STATE_UNAVAILABLE:
+	case RADIO_STATE_OFF:
+		break;
+	default:
+		/* Malformed parcel; no radio state == broken rild */
+		g_assert(FALSE);
 	}
+
+	exec_online_callback(md);
 }
 
 static void reg_suspended_cb(struct ril_msg *message, gpointer user_data)
@@ -764,46 +770,14 @@
 			sim_inserted, modem);
 }
 
-static void mtk_sim_mode_cb(struct ril_msg *message, gpointer user_data)
+static void exec_pending_online(struct mtk_data *md)
 {
-	struct cb_data *cbd = user_data;
-	ofono_modem_online_cb_t cb = cbd->cb;
-	struct ofono_modem *modem = cbd->user;
-	struct mtk_data *md = ofono_modem_get_data(modem);
 	struct mtk_data *md_c;
 
+	DBG("");
+
 	mtk_data_0->pending_cb = NULL;
 
-	if (message->error == RIL_E_SUCCESS) {
-		g_ril_print_response_no_args(md->ril, message);
-
-		/*
-		 * Although the request was successful, radio state might not
-		 * have changed yet. In that case we wait for the radio event,
-		 * otherwise RIL requests that depend on radio state will fail.
-		 */
-		if ((md->ofono_online && md->radio_state != RADIO_STATE_OFF) ||
-				(!md->ofono_online &&
-					md->radio_state == RADIO_STATE_OFF)) {
-			CALLBACK_WITH_SUCCESS(cb, cbd->data);
-		} else {
-			/* Wait for the radio state event */
-			md->online_cb = cb;
-			md->online_data = cbd->data;
-		}
-	} else {
-		ofono_error("%s: RIL error %s", __func__,
-				ril_error_to_string(message->error));
-		CALLBACK_WITH_FAILURE(cb, cbd->data);
-	}
-
-	if (md->ofono_online)
-		md->mtk_settings =
-			mtk_settings_create(md->modem, md->ril,
-						md->has_3g);
-	else
-		mtk_settings_remove(md->mtk_settings);
-
 	/* Execute possible pending operation on the other modem */
 
 	md_c = mtk_data_complement(md);
@@ -820,6 +794,44 @@
 	}
 }
 
+static gboolean sim_switch_failsafe(gpointer user_data)
+{
+	struct mtk_data *md = user_data;
+
+	exec_online_callback(md);
+
+	return FALSE;
+}
+
+static void mtk_sim_mode_cb(struct ril_msg *message, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_modem_online_cb_t cb = cbd->cb;
+	struct ofono_modem *modem = cbd->user;
+	struct mtk_data *md = ofono_modem_get_data(modem);
+
+	if (message->error == RIL_E_SUCCESS) {
+		g_ril_print_response_no_args(md->ril, message);
+		/*
+		 * Although the request was successful, radio state might not
+		 * have changed yet. So we wait for the upcoming radio event,
+		 * otherwise RIL requests that depend on radio state will fail.
+		 * If we are switching the 3G slot, we cannot really trust this
+		 * behaviour, so we add a failsafe timer.
+		 */
+		md->online_cb = cb;
+		md->online_data = cbd->data;
+
+		g_timeout_add(T_SIM_SWITCH_FAILSAFE_MS,
+						sim_switch_failsafe, md);
+	} else {
+		ofono_error("%s: RIL error %s", __func__,
+				ril_error_to_string(message->error));
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		exec_pending_online(md);
+	}
+}
+
 static int sim_state()
 {
 	int state = mtk_data_0->ofono_online ? SIM_1_ACTIVE : NO_SIM_ACTIVE;