Merge lp:~phablet-team/network-manager/lp1418077 into lp:~network-manager/network-manager/ubuntu

[Tony Espy]

This merge a fix for lp: #1418077 - NM takes ~5m to reconnect after GPRS detaches.

[Mathieu Trudel-Lapierre (cyphermox)]

I'd much rather we semantically split any cleanup from actual bugfix, both in the interest of readability of the diff as well as to reduce the scope of changes and risk of regression.

review: Needs Fixing

[Mathieu Trudel-Lapierre (cyphermox)]

These changes might also need to be applied to the RTM branches.

[Tony Espy (awe)]

AFAIK, we're not doing any more updates to the RTM branch. Also, pretty sure these diffs wouldn't apply as is...

[Ricardo Salveti (rsalveti)]

Change looks good, need to test it still.

[Mathieu Trudel-Lapierre (cyphermox)]

Very wise handling of autoconnect there. Has it been discussed with upstream NM developers?

Changes look fine to me, modulo inline comments.

review: Needs Information

[Tony Espy (awe)]

Yes, Dan and I discussed, and we iterated over this approach till it worked.

That said, my only concern is whether or not this approach has any negative impact on the modem-manager behavior.

I've responded to the comments inline.

[Mathieu Trudel-Lapierre (cyphermox)]

I don't know about the impact to MM. It will need to be tested as part of the silo testing.

review: Approve

[lp:~phablet-team/network-manager/lp1418077]

955. By Tony Espy on 2015-04-10

Fix ofono mobile-data 5m reconnect bug (LP: #1418077)

This change fixes a bug that occurs when the modem detaches
from GPRS due to signal loss, or a settings change (eg. radio
tech). Now when this happens, the connection's autoconnect
flag is set to FALSE. When re-attachment occurs, the flag
is set to TRUE, and autoconnect is re-triggered. A bug in
the ofono disconnect_done function was also fixed to ensure
the state is updated correctly.

956. By Tony Espy on 2015-04-10

Increase ofono activation timeout to 40s

Increase the ofono activation timeout to 40
to match the value used by connman.

[Tony Espy (awe)]

1. Tested on mako ( vivid-proposed #166 ):

 - device boots & mobile data connection established

 - tested bug scenario ( switch radio tech, no 5m delay )

 - tested flight-mode

 - tested switching from wifi to mobile via wifi toggle

2. Tested on krillin ( vivid-proposed #183 ):

 - device boots with SIM in slot1, mobile data connection (2g) established (2)

 - can't test device scenario ( no 3g )

 - tested flight mode (2)

=== modified file 'debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch'
--- debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch	2015-03-19 20:47:38 +0000
+++ debian/patches/0001-wwan-add-support-for-using-oFono-as-a-modem-manager.patch	2015-04-10 20:41:03 +0000
@@ -15,10 +15,10 @@
  create mode 100644 src/devices/wwan/nm-modem-ofono.c
  create mode 100644 src/devices/wwan/nm-modem-ofono.h
 
-Index: b/configure.ac
+Index: network-manager-0.9.10.0/configure.ac
 ===================================================================
---- a/configure.ac
-+++ b/configure.ac
+--- network-manager-0.9.10.0.orig/configure.ac
++++ network-manager-0.9.10.0/configure.ac
 @@ -593,6 +593,15 @@ else
  fi
  AM_CONDITIONAL(WITH_MODEM_MANAGER_1, test "${with_modem_manager_1}" = "yes")
@@ -35,10 +35,10 @@
  # DHCP client support
  AC_ARG_WITH([dhclient], AS_HELP_STRING([--with-dhclient=yes|no|path], [Enable dhclient 4.x support]))
  AC_ARG_WITH([dhcpcd], AS_HELP_STRING([--with-dhcpcd=yes|no|path], [Enable dhcpcd 4.x support]))
-Index: b/src/devices/wwan/Makefile.am
+Index: network-manager-0.9.10.0/src/devices/wwan/Makefile.am
 ===================================================================
---- a/src/devices/wwan/Makefile.am
-+++ b/src/devices/wwan/Makefile.am
+--- network-manager-0.9.10.0.orig/src/devices/wwan/Makefile.am
++++ network-manager-0.9.10.0/src/devices/wwan/Makefile.am
 @@ -49,6 +49,13 @@ libnm_wwan_la_SOURCES += \
  	nm-modem-broadband.h
  endif
@@ -53,10 +53,10 @@
  WWAN_SYMBOL_VIS_FILE=$(srcdir)/wwan-exports.ver
  
  libnm_wwan_la_LDFLAGS = \
-Index: b/src/devices/wwan/nm-modem-manager.c
+Index: network-manager-0.9.10.0/src/devices/wwan/nm-modem-manager.c
 ===================================================================
---- a/src/devices/wwan/nm-modem-manager.c
-+++ b/src/devices/wwan/nm-modem-manager.c
+--- network-manager-0.9.10.0.orig/src/devices/wwan/nm-modem-manager.c
++++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-manager.c
 @@ -17,7 +17,7 @@
   *
   * Copyright (C) 2009 - 2014 Red Hat, Inc.
@@ -300,11 +300,11 @@
  #if WITH_MODEM_MANAGER_1
  	/* ModemManager >= 0.7 */
  	clear_modem_manager_1_support (self);
-Index: b/src/devices/wwan/nm-modem-ofono.c
+Index: network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c
 ===================================================================
 --- /dev/null
-+++ b/src/devices/wwan/nm-modem-ofono.c
-@@ -0,0 +1,1378 @@
++++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c
+@@ -0,0 +1,1395 @@
 +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
 +/* NetworkManager -- Network link manager
 + *
@@ -414,7 +414,7 @@
 +
 +	NMIP4Config *ip4_config;
 +
-+	gboolean state;
++	gboolean enabled;
 +} NMModemOfonoPrivate;
 +
 +#define NM_OFONO_ERROR (nm_ofono_error_quark ())
@@ -460,6 +460,7 @@
 +{
 +	SimpleDisconnectContext *ctx = (SimpleDisconnectContext*) user_data;
 +	NMModemOfono *self = ctx->self;
++	NMModemState state = nm_modem_get_state (NM_MODEM (self));
 +	GError *error = NULL;
 +
 +	nm_log_dbg (LOGD_MB, "in %s", __func__);
@@ -474,9 +475,10 @@
 +
 +        simple_disconnect_context_free (ctx);
 +
-+	nm_modem_set_state (NM_MODEM (self),
-+	                    NM_MODEM_STATE_CONNECTED,
-+	                    nm_modem_state_to_string (NM_MODEM_STATE_CONNECTED));
++	if (state != NM_MODEM_STATE_SEARCHING)
++		nm_modem_set_state (NM_MODEM (self),
++		                    NM_MODEM_STATE_REGISTERED,
++		                    nm_modem_state_to_string (NM_MODEM_STATE_REGISTERED));
 +}
 +
 +static void
@@ -539,7 +541,7 @@
 +	NMModemState new_state;
 +	NMDeviceStateReason reason;
 +
-+	if (new_enabled == priv->state)
++	if (new_enabled == priv->enabled)
 +		return;
 +
 +	if (new_enabled) {
@@ -559,7 +561,7 @@
 +	                    new_state,
 +	                    nm_modem_state_to_string (new_state));
 +
-+	priv->state = new_enabled;
++	priv->enabled = new_enabled;
 +
 +	nm_log_info (LOGD_MB, "(%s) now in state: %s",
 +	             nm_modem_get_path (NM_MODEM (self)),
@@ -633,7 +635,9 @@
 +	}
 +	else if (g_strcmp0 (key, "Attached") == 0 && G_VALUE_HOLDS_BOOLEAN (value)) {
 +		priv->gprs_attached = g_value_get_boolean (value);
-+	}
++	} else
++		/* No need to update enabled for other property changes */
++		return;
 +
 +	update_ofono_enabled (self, priv->modem_online
 +	                            && priv->gprs_powered
@@ -875,6 +879,13 @@
 +		g_signal_emit_by_name (self, NM_MODEM_PREPARE_RESULT, FALSE,
 +		                       NM_DEVICE_STATE_REASON_MODEM_BUSY);
 +
++		/*
++		 * FIXME: add code to check for InProgress so that the
++		 * connection doesn't continue to try and activate,
++		 * leading to the connection being disabled, and a 5m
++		 * timeout...
++		 */
++
 +		g_error_free (error);
 +	}
 +}
@@ -1080,7 +1091,7 @@
 +
 +	dbus_g_proxy_begin_call_with_timeout (priv->context_proxy,
 +	                                      "SetProperty", stage1_prepare_done,
-+	                                      self, NULL, 20000,
++	                                      self, NULL, 40000,
 +	                                      G_TYPE_STRING, "Active",
 +	                                      G_TYPE_VALUE, &value,
 +	                                      G_TYPE_INVALID);
@@ -1550,6 +1561,12 @@
 +	             nm_modem_get_path (self),
 +                     enabled ? "enabled" : "disabled");
 +
++	/*
++	 * FIXME: this is code is a no-op; we should either make
++	 * work, or get rid of this function, or at least remove
++	 * the call to update_ofono_enabled.
++	 */
++
 +	update_ofono_enabled (NM_MODEM_OFONO (self),
 +	                      priv->modem_online
 +	                      && priv->gprs_powered
@@ -1683,10 +1700,10 @@
 +	//dbus_g_error_domain_register (NM_OFONO_ERROR, NULL, NM_TYPE_OFONO_ERROR);
 +}
 +
-Index: b/src/devices/wwan/nm-modem-ofono.h
+Index: network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.h
 ===================================================================
 --- /dev/null
-+++ b/src/devices/wwan/nm-modem-ofono.h
++++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.h
 @@ -0,0 +1,64 @@
 +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
 +/* NetworkManager -- Network link manager
@@ -1752,10 +1769,10 @@
 +G_END_DECLS
 +
 +#endif /* NM_MODEM_OFONO_H */
-Index: b/src/NetworkManagerUtils.c
+Index: network-manager-0.9.10.0/src/NetworkManagerUtils.c
 ===================================================================
---- a/src/NetworkManagerUtils.c
-+++ b/src/NetworkManagerUtils.c
+--- network-manager-0.9.10.0.orig/src/NetworkManagerUtils.c
++++ network-manager-0.9.10.0/src/NetworkManagerUtils.c
 @@ -1221,7 +1221,13 @@ nm_utils_ip6_property_path (const char *
  
  	len = g_snprintf (path, sizeof (path), IPV6_PROPERTY_DIR "%s/%s",
@@ -1787,3 +1804,39 @@
 +	return name;
  }
  
+Index: network-manager-0.9.10.0/src/devices/wwan/nm-device-modem.c
+===================================================================
+--- network-manager-0.9.10.0.orig/src/devices/wwan/nm-device-modem.c
++++ network-manager-0.9.10.0/src/devices/wwan/nm-device-modem.c
+@@ -201,6 +201,16 @@ modem_state_cb (NMModem *modem,
+ 		}
+ 	}
+ 
++	if (dev_state >= NM_DEVICE_STATE_DISCONNECTED &&
++	    new_state == NM_MODEM_STATE_REGISTERED && old_state < NM_MODEM_STATE_REGISTERED) {
++
++		nm_log_info (LOGD_MB, "(%s): modem re-registered; re-checking autoconnect",
++		             nm_device_get_iface (device));
++
++		g_object_set (G_OBJECT (device), NM_DEVICE_AUTOCONNECT, TRUE, NULL);
++		nm_device_emit_recheck_auto_activate (device);
++	}
++
+ 	if (new_state < NM_MODEM_STATE_CONNECTING &&
+ 	    old_state >= NM_MODEM_STATE_CONNECTING &&
+ 	    dev_state >= NM_DEVICE_STATE_NEED_AUTH &&
+@@ -271,6 +281,14 @@ device_state_changed (NMDevice *device,
+ 		             nm_modem_state_to_string (nm_modem_get_state (priv->modem)));
+ 	}
+ 
++	/* Block autoconnect until the modem is registered again */
++	if (new_state == NM_DEVICE_STATE_FAILED && nm_modem_get_state (priv->modem) == NM_MODEM_STATE_SEARCHING) {
++		nm_log_info (LOGD_MB, "(%s): modem searching; disabling autoconnect",
++		             nm_device_get_iface (device));
++
++		g_object_set (G_OBJECT (device), NM_DEVICE_AUTOCONNECT, FALSE, NULL);
++	}
++
+ 	nm_modem_device_state_changed (priv->modem, new_state, old_state, reason);
+ 
+ 	switch (reason) {