Merge lp:~rsalveti/ofono/gprs-attach-fix into lp:~phablet-team/ofono/ubuntu

[Ricardo Salveti]

Fix GPRS attach/detach logic (offline/online)

Fix GPRS attach/detach logic

Previously the modem would always show itself as attached, even after disabling gprs.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:6838
http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-ci/6/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-saucy-amd64-ci/6
    FAILURE: http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-saucy-armhf-ci/6/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/phablet-team-ofono-ubuntu-saucy-i386-ci/6

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/phablet-team-ofono-ubuntu-ci/6/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/phablet-team-ofono-ubuntu-ci/7/rebuild

review: Approve (continuous-integration)

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Tested the branch in the following way:

* Cover the phone with tin foil
* Put it in an oven

The phone loses coverage, but after a few seconds it recovers the data connection.

review: Approve

[Dave Morley (davmor2)]

Test the fix on maguro. This fix meant I had consistent 3g ofono conncetion.

review: Approve (tested it in real life on maguro)

=== modified file 'drivers/rilmodem/gprs-context.c'
--- drivers/rilmodem/gprs-context.c	2013-09-09 19:21:32 +0000
+++ drivers/rilmodem/gprs-context.c	2013-10-16 03:46:52 +0000
@@ -53,7 +53,7 @@
 
 struct gprs_context_data {
 	GRil *ril;
-	guint active_ctx_cid;
+	gint active_ctx_cid;
 	gint active_rild_cid;
 	enum state state;
 };
@@ -66,8 +66,8 @@
 {
 	DBG("");
 
-	gcd->active_ctx_cid = -1;
-	gcd->active_rild_cid = -1;
+	gcd->active_ctx_cid = 0;
+	gcd->active_rild_cid = 0;
 	gcd->state = STATE_IDLE;
 }
 
@@ -100,14 +100,17 @@
 	for (iterator = unsol->call_list; iterator; iterator = iterator->next) {
 		call = (struct data_call *) iterator->data;
 
-		if (call->cid == gcd->active_rild_cid) {
+		if (call->cid == gcd->active_rild_cid && 
+		    gcd->state != STATE_IDLE) {
 			DBG("Found current call in call list: %d", call->cid);
 			active_cid_found = TRUE;
 
 			if (call->active == 0) {
-				DBG("call->status is DISCONNECTED for cid: %d", call->cid);
+				DBG("call->status is DISCONNECTED for cid: %d", 
+					call->cid);
 				disconnect = TRUE;
-				ofono_gprs_context_deactivated(gc, gcd->active_ctx_cid);
+				ofono_gprs_context_deactivated(gc, 
+						       gcd->active_ctx_cid);
 			}
 
 			break;
@@ -149,9 +152,8 @@
 	gcd->active_rild_cid = reply->cid;
 
 	if (error.type != OFONO_ERROR_TYPE_NO_ERROR) {
-		if (gcd->active_rild_cid != -1)
-			disconnect_context(gc);
-
+		/* parsing failed, need to actually disconnect */	  
+		disconnect_context(gc);
 		goto error;
 	}
 
@@ -185,7 +187,7 @@
 		error.type = OFONO_ERROR_TYPE_FAILURE;
 		error.error = EINVAL;
 
-		set_context_disconnected(gcd);
+		disconnect_context(gc);
 		goto error;
 	}
 
@@ -289,6 +291,7 @@
 	ofono_gprs_context_cb_t cb = cbd->cb;
 	struct ofono_gprs_context *gc = cbd->user;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	gint active_ctx_cid;
 
 	DBG("");
 
@@ -297,6 +300,7 @@
 
 		g_ril_print_response_no_args(gcd->ril, message);
 
+		active_ctx_cid = gcd->active_ctx_cid;
 		set_context_disconnected(gcd);
 
 		/* If the deactivate was a result of a shutdown,
@@ -306,10 +310,10 @@
 		if (cb)
 			CALLBACK_WITH_SUCCESS(cb, cbd->data);
 		else
-			ofono_gprs_context_deactivated(gc, gcd->active_ctx_cid);
+			ofono_gprs_context_deactivated(gc, active_ctx_cid);
 
 	} else {
-		ofono_error("%s: replay failure: %s",
+		ofono_error("%s: reply failure: %s",
 				__func__,
 				ril_error_to_string(message->error));
 
@@ -330,10 +334,10 @@
 	int reqid = RIL_REQUEST_DEACTIVATE_DATA_CALL;
 	int ret = 0;
 
-	DBG("");
+	DBG("cid: %d active_rild_cid: %d", id, gcd->active_rild_cid);
 
-	if (gcd->active_rild_cid == -1) {
-		set_context_disconnected(gcd);
+	if (gcd->state == STATE_IDLE || gcd->state == STATE_DISABLING) {
+		/* nothing to do */ 	  
 
 		if (cb) {
 			CALLBACK_WITH_SUCCESS(cb, data);
@@ -343,7 +347,6 @@
 		return;
 	}
 
-
 	cbd = cb_data_new(cb, data);
 	cbd->user = gc;
 
@@ -371,6 +374,8 @@
 
 error:
 	if (ret <= 0) {
+		/* TODO: should we force state to disconnected here? */	  
+
 		ofono_error("Send RIL_REQUEST_DEACTIVATE_DATA_CALL failed.");
 		g_free(cbd);
 		if (cb)

=== modified file 'drivers/rilmodem/gprs.c'
--- drivers/rilmodem/gprs.c	2013-09-09 19:21:32 +0000
+++ drivers/rilmodem/gprs.c	2013-10-16 03:46:52 +0000
@@ -121,10 +121,14 @@
 	struct ofono_gprs *gprs = cbd->user;
 	struct gprs_data *gd = ofono_gprs_get_data(gprs);
 	struct ofono_error error;
-	gboolean attached;
-	int status, lac, ci, tech;
+	gboolean attached = FALSE;
+	gboolean notify = FALSE;
+	int lac, ci, tech;
+	int old_status, status;
 	int max_cids = 1;
 
+	DBG("");
+
 	if (message->error == RIL_E_SUCCESS) {
 		decode_ril_error(&error, "OK");
 	} else {
@@ -136,6 +140,8 @@
 		goto error;
 	}
 
+	old_status = gd->rild_status;
+
 	if (ril_util_parse_reg(gd->ril, message, &status,
 				&lac, &ci, &tech, &max_cids) == FALSE) {
 		ofono_error("Failure parsing data registration response.");
@@ -144,26 +150,27 @@
 		goto error;
 	}
 
-	if (gd->rild_status == -1) {
-		ofono_gprs_register(gprs);
-
-		/* RILD tracks data network state together with voice */
-		g_ril_register(gd->ril, RIL_UNSOL_RESPONSE_VOICE_NETWORK_STATE_CHANGED,
-				ril_gprs_state_change, gprs);
-	}
-
-	if (max_cids > gd->max_cids) {
-		DBG("Setting max cids to %d", max_cids);
-		gd->max_cids = max_cids;
-		ofono_gprs_set_cid_range(gprs, 1, max_cids);
-	}
-
-	/* Just need to notify ofono if it's already attached */
-	if (gd->ofono_attached && (gd->rild_status != status)) {
-		ofono_gprs_status_notify(gprs, status);
-	}
-
-	gd->rild_status = status;
+	/*
+	 * There are three cases that can result in this callback
+	 * running:
+	 *
+	 * 1) The driver's probe() method was called, and thus an
+	 *    internal call to ril_gprs_registration_status() is
+	 *    generated.  No ofono cb exists.
+	 *
+	 * 2) ril_gprs_state_change() is called due to an unsolicited
+	 *    event from RILD.  No ofono cb exists.
+	 *
+	 * 3) The ofono code code calls the driver's attached_status()
+	 *    function.  A valid ofono cb exists.
+	 */
+
+	if (gd->rild_status != status) {
+		gd->rild_status = status;
+
+		if (cb == NULL)
+			notify = TRUE;
+	}
 
 	/*
 	 * Override the actual status based upon the desired
@@ -176,7 +183,41 @@
 	if (attached && gd->ofono_attached == FALSE) {
 		DBG("attached=true; ofono_attached=false; return !REGISTERED");
 		status = NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
-	}
+
+		/* Further optimization so that if ril_status == 
+		 * NOT_REGISTERED, ofono_attached == false, and status ==
+		 * ROAMING | REGISTERED, then notify gets cleared...
+		 *
+		 * As is, this results in unecessary status notify calls
+		 * when nothing has changed.
+		 */
+		if (status == old_status && notify)
+			notify = FALSE;
+	}
+
+	if (old_status == -1) {
+		ofono_gprs_register(gprs);
+
+		/* RILD tracks data network state together with voice */
+		g_ril_register(gd->ril, RIL_UNSOL_RESPONSE_VOICE_NETWORK_STATE_CHANGED,
+				ril_gprs_state_change, gprs);
+
+		if (max_cids > gd->max_cids) {
+			DBG("Setting max cids to %d", max_cids);
+			gd->max_cids = max_cids;
+			ofono_gprs_set_cid_range(gprs, 1, max_cids);
+		}
+
+		/* 
+		 * This callback is a result of the inital call
+		 * to probe(), so should return after registration.
+		 */
+		return;
+	}
+
+	/* Just need to notify ofono if it's already attached */
+	if (notify)
+		ofono_gprs_status_notify(gprs, status);
 
 error:
 	if (cb)
@@ -194,6 +235,14 @@
 
 	cbd->user = gprs;
 
+	DBG("");
+
+	/* 
+	 * TODO (optimization): if gd->ofono_attached == FALSE, don't 
+	 * send REQ_DATA_REG; just invoke with the callback with
+	 * status == !REGISTERED!!! 
+	 */
+
 	ret = g_ril_send(gd->ril, request,
 				NULL, 0, ril_data_reg_cb, cbd, g_free);