[PATCH -v3 1/3] Add ofono_modem_reset()

[PATCH -v3 1/3] Add ofono_modem_reset()

[Gustavo F. Padovan]

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

Some modems can screw up everything and then we will need to do a silent
reset of the modem. This patch take the modem back to the OFFLINE state.
---
 include/modem.h |    2 ++
 src/modem.c     |   44 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/include/modem.h b/include/modem.h
index 7b13ee0..a92eb88 100644
--- a/include/modem.h
+++ b/include/modem.h
@@ -46,6 +46,8 @@ int ofono_modem_register(struct ofono_modem *modem);
 ofono_bool_t ofono_modem_is_registered(struct ofono_modem *modem);
 void ofono_modem_remove(struct ofono_modem *modem);
 
+void ofono_modem_reset(struct ofono_modem *modem);
+
 void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered);
 ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem);
 
diff --git a/src/modem.c b/src/modem.c
index 6d346c3..e57f8fc 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -70,6 +70,7 @@ struct ofono_modem {
 	guint			interface_update;
 	ofono_bool_t		powered;
 	ofono_bool_t		powered_pending;
+	ofono_bool_t		reset;
 	guint			timeout;
 	ofono_bool_t		online;
 	GHashTable		*properties;
@@ -433,6 +434,8 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
 		if (modem->driver->set_online == NULL)
 			modem_change_state(modem, MODEM_STATE_ONLINE);
 
+		modem->reset = FALSE;
+
 		break;
 	}
 }
@@ -784,7 +787,8 @@ void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
 		return;
 	}
 
-	ofono_dbus_signal_property_changed(conn, modem->path,
+	if (!modem->reset)
+		ofono_dbus_signal_property_changed(conn, modem->path,
 					OFONO_MODEM_INTERFACE,
 					"Powered", DBUS_TYPE_BOOLEAN,
 					&dbus_powered);
@@ -799,6 +803,17 @@ void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
 	} else
 		modem_change_state(modem, MODEM_STATE_POWER_OFF);
 
+	if (modem->reset && !powering_down) {
+		if (!modem->powered) {
+			int err = set_powered(modem, TRUE);
+
+			if (err == -EINPROGRESS)
+				return;
+
+			modem_change_state(modem, MODEM_STATE_PRE_SIM);
+		}
+	}
+
 out:
 	if (powering_down && powered == FALSE) {
 		modems_remaining -= 1;
@@ -806,6 +821,7 @@ out:
 		if (modems_remaining == 0)
 			__ofono_exit();
 	}
+
 }
 
 ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem)
@@ -1565,6 +1581,32 @@ void ofono_modem_remove(struct ofono_modem *modem)
 	g_free(modem);
 }
 
+static gboolean __reset_modem(void *data)
+{
+	struct ofono_modem *modem = data;
+	int err;
+
+	modem->reset = TRUE;
+
+	err = set_powered(modem, FALSE);
+	if (err == -EINPROGRESS)
+		return FALSE;
+
+	err = set_powered(modem, TRUE);
+	if (err == -EINPROGRESS)
+		return FALSE;
+
+	modem_change_state(modem, MODEM_STATE_PRE_SIM);
+	return FALSE;
+}
+
+void ofono_modem_reset(struct ofono_modem *modem)
+{
+	DBG("%p", modem);
+
+	g_idle_add(__reset_modem, modem);
+}
+
 int ofono_modem_driver_register(const struct ofono_modem_driver *d)
 {
 	DBG("driver: %p, name: %s", d, d->name);
-- 
1.7.3.1

[PATCH -v3 2/3] phonesim: Add modem reset trigger

[Gustavo F. Padovan]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

---
 plugins/phonesim.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/plugins/phonesim.c b/plugins/phonesim.c
index d2faf42..7426da6 100644
--- a/plugins/phonesim.c
+++ b/plugins/phonesim.c
@@ -237,6 +237,13 @@ static void cfun_set_on_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	ofono_modem_set_powered(modem, ok);
 }
 
+static void crst_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+
+	ofono_modem_reset(modem);
+}
+
 static void phonesim_disconnected(gpointer user_data)
 {
 	struct ofono_modem *modem = user_data;
@@ -389,6 +396,9 @@ static int phonesim_enable(struct ofono_modem *modem)
 	g_at_chat_send(data->chat, "AT+CSCS=\"GSM\"", none_prefix,
 			NULL, NULL, NULL);
 
+	g_at_chat_register(data->chat, "+CRST:",
+				crst_notify, FALSE, modem, NULL);
+
 	return 0;
 }
 
-- 
1.7.3.1

[PATCH -v3 3/3] modem: add support to restore state when resetting the modem

[Gustavo F. Padovan]

[-- Attachment #1: Type: text/plain, Size: 3671 bytes --]

---
 src/modem.c |   74 ++++++++++++++++++++++++++++++++--------------------------
 1 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index e57f8fc..704de29 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -61,6 +61,7 @@ enum modem_state {
 struct ofono_modem {
 	char			*path;
 	enum modem_state	modem_state;
+	enum modem_state	old_state;
 	GSList			*atoms;
 	struct ofono_watchlist	*atom_watches;
 	GSList			*interface_list;
@@ -375,7 +376,7 @@ static void modem_change_state(struct ofono_modem *modem,
 	if (old_state == new_state)
 		return;
 
-	if (new_online != modem->online) {
+	if (new_online != modem->online && !modem->reset) {
 		DBusConnection *conn = ofono_dbus_get_connection();
 		modem->online = new_online;
 		ofono_dbus_signal_property_changed(conn, modem->path,
@@ -414,48 +415,26 @@ static void modem_change_state(struct ofono_modem *modem,
 	}
 }
 
-static void sim_state_watch(enum ofono_sim_state new_state, void *user)
-{
-	struct ofono_modem *modem = user;
-
-	switch (new_state) {
-	case OFONO_SIM_STATE_NOT_PRESENT:
-		modem_change_state(modem, MODEM_STATE_PRE_SIM);
-		break;
-	case OFONO_SIM_STATE_INSERTED:
-		break;
-	case OFONO_SIM_STATE_READY:
-		modem_change_state(modem, MODEM_STATE_OFFLINE);
-
-		/*
-		 * If we don't have the set_online method, also proceed
-		 * straight to the online state
-		 */
-		if (modem->driver->set_online == NULL)
-			modem_change_state(modem, MODEM_STATE_ONLINE);
-
-		modem->reset = FALSE;
-
-		break;
-	}
-}
-
 static void online_cb(const struct ofono_error *error, void *data)
 {
 	struct ofono_modem *modem = data;
 	DBusMessage *reply;
 
 	if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
-			modem->modem_state == MODEM_STATE_OFFLINE)
+			modem->modem_state == MODEM_STATE_OFFLINE) {
+		modem_change_state(modem, MODEM_STATE_ONLINE);
+
+		if (modem->reset) {
+			modem->reset = FALSE;
+			return;
+		}
+
 		reply = dbus_message_new_method_return(modem->pending);
-	else
+	} else {
 		reply = __ofono_error_failed(modem->pending);
+	}
 
 	__ofono_dbus_pending_reply(&modem->pending, reply);
-
-	if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
-			modem->modem_state == MODEM_STATE_OFFLINE)
-		modem_change_state(modem, MODEM_STATE_ONLINE);
 }
 
 static void offline_cb(const struct ofono_error *error, void *data)
@@ -475,6 +454,34 @@ static void offline_cb(const struct ofono_error *error, void *data)
 		modem_change_state(modem, MODEM_STATE_OFFLINE);
 }
 
+static void sim_state_watch(enum ofono_sim_state new_state, void *user)
+{
+	struct ofono_modem *modem = user;
+
+	switch (new_state) {
+	case OFONO_SIM_STATE_NOT_PRESENT:
+		modem_change_state(modem, MODEM_STATE_PRE_SIM);
+		break;
+	case OFONO_SIM_STATE_INSERTED:
+		break;
+	case OFONO_SIM_STATE_READY:
+		modem_change_state(modem, MODEM_STATE_OFFLINE);
+
+		/*
+		 * If we don't have the set_online method, also proceed
+		 * straight to the online state
+		 */
+		if (modem->driver->set_online == NULL)
+			modem_change_state(modem, MODEM_STATE_ONLINE);
+		else if (modem->old_state > MODEM_STATE_OFFLINE)
+			modem->driver->set_online(modem, 1, online_cb, modem);
+		else
+			modem->reset = FALSE;
+
+		break;
+	}
+}
+
 static DBusMessage *set_property_online(struct ofono_modem *modem,
 					DBusMessage *msg,
 					DBusMessageIter *var)
@@ -1586,6 +1593,7 @@ static gboolean __reset_modem(void *data)
 	struct ofono_modem *modem = data;
 	int err;
 
+	modem->old_state = modem->modem_state;
 	modem->reset = TRUE;
 
 	err = set_powered(modem, FALSE);
-- 
1.7.3.1

Re: [PATCH -v3 3/3] modem: add support to restore state when resetting the modem

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

Hi Gustavo,

On 11/19/2010 03:36 PM, Gustavo F. Padovan wrote:
> ---
>  src/modem.c |   74 ++++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 41 insertions(+), 33 deletions(-)
> 

Ah ok, you implement the getting back to online process in this patch.
Ignore my previous comment.

> diff --git a/src/modem.c b/src/modem.c
> index e57f8fc..704de29 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -61,6 +61,7 @@ enum modem_state {
>  struct ofono_modem {
>  	char			*path;
>  	enum modem_state	modem_state;
> +	enum modem_state	old_state;

Can we simply use a boolean like 'get_online' that is FALSE by default,
but gets set to TRUE (assuming we're online) during a silent reset?
This can then get re-used for implementing of the Lockdown property as well.

It also seems that the 'reset' value is not really needed.

Regards,
-Denis

Re: [PATCH -v3 3/3] modem: add support to restore state when resetting the modem

[Gustavo F. Padovan]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Hi Denis,

* Denis Kenzior <denkenz@gmail.com> [2010-11-23 05:04:40 -0600]:

> Hi Gustavo,
> 
> On 11/19/2010 03:36 PM, Gustavo F. Padovan wrote:
> > ---
> >  src/modem.c |   74 ++++++++++++++++++++++++++++++++--------------------------
> >  1 files changed, 41 insertions(+), 33 deletions(-)
> > 
> 
> Ah ok, you implement the getting back to online process in this patch.
> Ignore my previous comment.
> 
> > diff --git a/src/modem.c b/src/modem.c
> > index e57f8fc..704de29 100644
> > --- a/src/modem.c
> > +++ b/src/modem.c
> > @@ -61,6 +61,7 @@ enum modem_state {
> >  struct ofono_modem {
> >  	char			*path;
> >  	enum modem_state	modem_state;
> > +	enum modem_state	old_state;
> 
> Can we simply use a boolean like 'get_online' that is FALSE by default,
> but gets set to TRUE (assuming we're online) during a silent reset?
> This can then get re-used for implementing of the Lockdown property as well.

Yeah, I'm reusing it there.

> 
> It also seems that the 'reset' value is not really needed.

I'm using that to avoid emit DBus signal during the silent reset, Not
seeing how to do that without modem->reset, maybe we can rely on
modem->pending, not sure yet. 

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH -v3 3/3] modem: add support to restore state when resetting the modem

[Gustavo F. Padovan]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

* Gustavo F. Padovan <padovan@profusion.mobi> [2010-11-23 15:50:09 -0200]:

> Hi Denis,
> 
> * Denis Kenzior <denkenz@gmail.com> [2010-11-23 05:04:40 -0600]:
> 
> > Hi Gustavo,
> > 
> > On 11/19/2010 03:36 PM, Gustavo F. Padovan wrote:
> > > ---
> > >  src/modem.c |   74 ++++++++++++++++++++++++++++++++--------------------------
> > >  1 files changed, 41 insertions(+), 33 deletions(-)
> > > 
> > 
> > Ah ok, you implement the getting back to online process in this patch.
> > Ignore my previous comment.
> > 
> > > diff --git a/src/modem.c b/src/modem.c
> > > index e57f8fc..704de29 100644
> > > --- a/src/modem.c
> > > +++ b/src/modem.c
> > > @@ -61,6 +61,7 @@ enum modem_state {
> > >  struct ofono_modem {
> > >  	char			*path;
> > >  	enum modem_state	modem_state;
> > > +	enum modem_state	old_state;
> > 
> > Can we simply use a boolean like 'get_online' that is FALSE by default,
> > but gets set to TRUE (assuming we're online) during a silent reset?
> > This can then get re-used for implementing of the Lockdown property as well.
> 
> Yeah, I'm reusing it there.
> 
> > 
> > It also seems that the 'reset' value is not really needed.
> 
> I'm using that to avoid emit DBus signal during the silent reset, Not
> seeing how to do that without modem->reset, maybe we can rely on
> modem->pending, not sure yet. 

Or do you mean that have DBus signaling about Powered and Online during
silent reset is just fine? That is starting to make sense to me. We may
want to notify users that the modem is not there.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH -v3 3/3] modem: add support to restore state when resetting the modem

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 917 bytes --]

Hi Gustavo,

>>>
>>> It also seems that the 'reset' value is not really needed.
>>
>> I'm using that to avoid emit DBus signal during the silent reset, Not
>> seeing how to do that without modem->reset, maybe we can rely on
>> modem->pending, not sure yet. 
> 
> Or do you mean that have DBus signaling about Powered and Online during
> silent reset is just fine? That is starting to make sense to me. We may
> want to notify users that the modem is not there.
> 

I actually think that emitting powered / online going on and off is
okay, there's no real way to hide this since all the atoms have to be
re-initialized.  So all the interfaces go away and magically re-appear,
all properties are re-emitted, etc.  Not emitting Powered or Online is
the least of our worries.

Silent resets are really just about avoiding having the user re-enter
the PIN when the modem crashes.

Regards,
-Denis

Re: [PATCH -v3 2/3] phonesim: Add modem reset trigger

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

Hi Gustavo,

On 11/19/2010 03:36 PM, Gustavo F. Padovan wrote:
> ---
>  plugins/phonesim.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/plugins/phonesim.c b/plugins/phonesim.c
> index d2faf42..7426da6 100644
> --- a/plugins/phonesim.c
> +++ b/plugins/phonesim.c
> @@ -237,6 +237,13 @@ static void cfun_set_on_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  	ofono_modem_set_powered(modem, ok);
>  }
>  
> +static void crst_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +
> +	ofono_modem_reset(modem);

I suggest you simply clean up the phonesim connection (e.g. closing the
tcp socket) when you receive this notification, and then call
modem_reset at the end.

> +}
> +
>  static void phonesim_disconnected(gpointer user_data)
>  {
>  	struct ofono_modem *modem = user_data;
> @@ -389,6 +396,9 @@ static int phonesim_enable(struct ofono_modem *modem)
>  	g_at_chat_send(data->chat, "AT+CSCS=\"GSM\"", none_prefix,
>  			NULL, NULL, NULL);
>  
> +	g_at_chat_register(data->chat, "+CRST:",
> +				crst_notify, FALSE, modem, NULL);
> +
>  	return 0;
>  }
>  

Regards,
-Denis

Re: [PATCH -v3 2/3] phonesim: Add modem reset trigger

[Gustavo F. Padovan]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

* Denis Kenzior <denkenz@gmail.com> [2010-11-23 04:59:33 -0600]:

> Hi Gustavo,
> 
> On 11/19/2010 03:36 PM, Gustavo F. Padovan wrote:
> > ---
> >  plugins/phonesim.c |   10 ++++++++++
> >  1 files changed, 10 insertions(+), 0 deletions(-)
> > 
> > diff --git a/plugins/phonesim.c b/plugins/phonesim.c
> > index d2faf42..7426da6 100644
> > --- a/plugins/phonesim.c
> > +++ b/plugins/phonesim.c
> > @@ -237,6 +237,13 @@ static void cfun_set_on_cb(gboolean ok, GAtResult *result, gpointer user_data)
> >  	ofono_modem_set_powered(modem, ok);
> >  }
> >  
> > +static void crst_notify(GAtResult *result, gpointer user_data)
> > +{
> > +	struct ofono_modem *modem = user_data;
> > +
> > +	ofono_modem_reset(modem);
> 
> I suggest you simply clean up the phonesim connection (e.g. closing the
> tcp socket) when you receive this notification, and then call
> modem_reset at the end.

So here do you mean unref phonesim data->chat and also call
ofono_modem_set_powered(modem, FALSE) before call ofono_modem_reset()?

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH -v3 2/3] phonesim: Add modem reset trigger

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

Hi Gustavo,

>>> +static void crst_notify(GAtResult *result, gpointer user_data)
>>> +{
>>> +	struct ofono_modem *modem = user_data;
>>> +
>>> +	ofono_modem_reset(modem);
>>
>> I suggest you simply clean up the phonesim connection (e.g. closing the
>> tcp socket) when you receive this notification, and then call
>> modem_reset at the end.
> 
> So here do you mean unref phonesim data->chat and also call
> ofono_modem_set_powered(modem, FALSE) before call ofono_modem_reset()?
> 

So I really mean performing the same steps as disable.  Calling
ofono_modem_set_powered is pretty much optional.  The core should simply
assume that once ofono_modem_reset has been called, the modem is off and
needs to be re-initialized from the beginning.

Regards,
-Denis

Re: [PATCH -v3 1/3] Add ofono_modem_reset()

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4160 bytes --]

Hi Gustavo,

On 11/19/2010 03:36 PM, Gustavo F. Padovan wrote:
> Some modems can screw up everything and then we will need to do a silent
> reset of the modem. This patch take the modem back to the OFFLINE state.
> ---
>  include/modem.h |    2 ++
>  src/modem.c     |   44 +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 1 deletions(-)
> 
> diff --git a/include/modem.h b/include/modem.h
> index 7b13ee0..a92eb88 100644
> --- a/include/modem.h
> +++ b/include/modem.h
> @@ -46,6 +46,8 @@ int ofono_modem_register(struct ofono_modem *modem);
>  ofono_bool_t ofono_modem_is_registered(struct ofono_modem *modem);
>  void ofono_modem_remove(struct ofono_modem *modem);
>  
> +void ofono_modem_reset(struct ofono_modem *modem);
> +
>  void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered);
>  ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem);
>  
> diff --git a/src/modem.c b/src/modem.c
> index 6d346c3..e57f8fc 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -70,6 +70,7 @@ struct ofono_modem {
>  	guint			interface_update;
>  	ofono_bool_t		powered;
>  	ofono_bool_t		powered_pending;
> +	ofono_bool_t		reset;
>  	guint			timeout;
>  	ofono_bool_t		online;
>  	GHashTable		*properties;
> @@ -433,6 +434,8 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
>  		if (modem->driver->set_online == NULL)
>  			modem_change_state(modem, MODEM_STATE_ONLINE);
>  
> +		modem->reset = FALSE;
> +
>  		break;
>  	}
>  }
> @@ -784,7 +787,8 @@ void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
>  		return;
>  	}
>  
> -	ofono_dbus_signal_property_changed(conn, modem->path,
> +	if (!modem->reset)
> +		ofono_dbus_signal_property_changed(conn, modem->path,
>  					OFONO_MODEM_INTERFACE,
>  					"Powered", DBUS_TYPE_BOOLEAN,
>  					&dbus_powered);
> @@ -799,6 +803,17 @@ void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
>  	} else
>  		modem_change_state(modem, MODEM_STATE_POWER_OFF);
>  
> +	if (modem->reset && !powering_down) {
> +		if (!modem->powered) {
> +			int err = set_powered(modem, TRUE);
> +
> +			if (err == -EINPROGRESS)
> +				return;
> +
> +			modem_change_state(modem, MODEM_STATE_PRE_SIM);
> +		}
> +	}
> +
>  out:
>  	if (powering_down && powered == FALSE) {
>  		modems_remaining -= 1;
> @@ -806,6 +821,7 @@ out:
>  		if (modems_remaining == 0)
>  			__ofono_exit();
>  	}
> +
>  }
>  
>  ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem)
> @@ -1565,6 +1581,32 @@ void ofono_modem_remove(struct ofono_modem *modem)
>  	g_free(modem);
>  }
>  
> +static gboolean __reset_modem(void *data)

There's no need to use '__' prefix for static functions.  That prefix is
reserved for private API functions declared in ofono.h

> +{
> +	struct ofono_modem *modem = data;
> +	int err;
> +
> +	modem->reset = TRUE;
> +
> +	err = set_powered(modem, FALSE);
> +	if (err == -EINPROGRESS)
> +		return FALSE;

So we really don't need to power down the modem for the silent reset.
Simply assume that the modem driver has performed the necessary steps of
cleaning up the modem to a 'powered off state'.  We can also assume that
silent resets only happen when the modem is at least powered.

One thing I'm not seeing in this patch is the functionality for getting
the modem back into online state after a silent reset.

> +
> +	err = set_powered(modem, TRUE);
> +	if (err == -EINPROGRESS)
> +		return FALSE;
> +
> +	modem_change_state(modem, MODEM_STATE_PRE_SIM);
> +	return FALSE;
> +}
> +
> +void ofono_modem_reset(struct ofono_modem *modem)
> +{
> +	DBG("%p", modem);
> +
> +	g_idle_add(__reset_modem, modem);

In general I don't like using g_idle_add in the core.  What is the
purpose of this one?  If you really have to do it, please track the
gsource and make sure to remove it when the modem is removed.

> +}
> +
>  int ofono_modem_driver_register(const struct ofono_modem_driver *d)
>  {
>  	DBG("driver: %p, name: %s", d, d->name);

Regards,
-Denis

Re: [PATCH -v3 1/3] Add ofono_modem_reset()

[Gustavo F. Padovan]

[-- Attachment #1: Type: text/plain, Size: 4644 bytes --]

Hi Denis,

* Denis Kenzior <denkenz@gmail.com> [2010-11-23 04:57:35 -0600]:

> Hi Gustavo,
> 
> On 11/19/2010 03:36 PM, Gustavo F. Padovan wrote:
> > Some modems can screw up everything and then we will need to do a silent
> > reset of the modem. This patch take the modem back to the OFFLINE state.
> > ---
> >  include/modem.h |    2 ++
> >  src/modem.c     |   44 +++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 45 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/modem.h b/include/modem.h
> > index 7b13ee0..a92eb88 100644
> > --- a/include/modem.h
> > +++ b/include/modem.h
> > @@ -46,6 +46,8 @@ int ofono_modem_register(struct ofono_modem *modem);
> >  ofono_bool_t ofono_modem_is_registered(struct ofono_modem *modem);
> >  void ofono_modem_remove(struct ofono_modem *modem);
> >  
> > +void ofono_modem_reset(struct ofono_modem *modem);
> > +
> >  void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered);
> >  ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem);
> >  
> > diff --git a/src/modem.c b/src/modem.c
> > index 6d346c3..e57f8fc 100644
> > --- a/src/modem.c
> > +++ b/src/modem.c
> > @@ -70,6 +70,7 @@ struct ofono_modem {
> >  	guint			interface_update;
> >  	ofono_bool_t		powered;
> >  	ofono_bool_t		powered_pending;
> > +	ofono_bool_t		reset;
> >  	guint			timeout;
> >  	ofono_bool_t		online;
> >  	GHashTable		*properties;
> > @@ -433,6 +434,8 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
> >  		if (modem->driver->set_online == NULL)
> >  			modem_change_state(modem, MODEM_STATE_ONLINE);
> >  
> > +		modem->reset = FALSE;
> > +
> >  		break;
> >  	}
> >  }
> > @@ -784,7 +787,8 @@ void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
> >  		return;
> >  	}
> >  
> > -	ofono_dbus_signal_property_changed(conn, modem->path,
> > +	if (!modem->reset)
> > +		ofono_dbus_signal_property_changed(conn, modem->path,
> >  					OFONO_MODEM_INTERFACE,
> >  					"Powered", DBUS_TYPE_BOOLEAN,
> >  					&dbus_powered);
> > @@ -799,6 +803,17 @@ void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
> >  	} else
> >  		modem_change_state(modem, MODEM_STATE_POWER_OFF);
> >  
> > +	if (modem->reset && !powering_down) {
> > +		if (!modem->powered) {
> > +			int err = set_powered(modem, TRUE);
> > +
> > +			if (err == -EINPROGRESS)
> > +				return;
> > +
> > +			modem_change_state(modem, MODEM_STATE_PRE_SIM);
> > +		}
> > +	}
> > +
> >  out:
> >  	if (powering_down && powered == FALSE) {
> >  		modems_remaining -= 1;
> > @@ -806,6 +821,7 @@ out:
> >  		if (modems_remaining == 0)
> >  			__ofono_exit();
> >  	}
> > +
> >  }
> >  
> >  ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem)
> > @@ -1565,6 +1581,32 @@ void ofono_modem_remove(struct ofono_modem *modem)
> >  	g_free(modem);
> >  }
> >  
> > +static gboolean __reset_modem(void *data)
> 
> There's no need to use '__' prefix for static functions.  That prefix is
> reserved for private API functions declared in ofono.h

Ok.

> 
> > +{
> > +	struct ofono_modem *modem = data;
> > +	int err;
> > +
> > +	modem->reset = TRUE;
> > +
> > +	err = set_powered(modem, FALSE);
> > +	if (err == -EINPROGRESS)
> > +		return FALSE;
> 
> So we really don't need to power down the modem for the silent reset.
> Simply assume that the modem driver has performed the necessary steps of
> cleaning up the modem to a 'powered off state'.  We can also assume that
> silent resets only happen when the modem is at least powered.

Ok.

> 
> One thing I'm not seeing in this patch is the functionality for getting
> the modem back into online state after a silent reset.
> 
> > +
> > +	err = set_powered(modem, TRUE);
> > +	if (err == -EINPROGRESS)
> > +		return FALSE;
> > +
> > +	modem_change_state(modem, MODEM_STATE_PRE_SIM);
> > +	return FALSE;
> > +}
> > +
> > +void ofono_modem_reset(struct ofono_modem *modem)
> > +{
> > +	DBG("%p", modem);
> > +
> > +	g_idle_add(__reset_modem, modem);
> 
> In general I don't like using g_idle_add in the core.  What is the
> purpose of this one?  If you really have to do it, please track the
> gsource and make sure to remove it when the modem is removed.

I was facing a segfault due to caling driver->disable here. It destroys
the data->chat in phonesim, but we need it there because we are in the
middle of callback. I'll ckeck if with the new changes you proposed we
will still need this.

-- 
Gustavo F. Padovan
http://profusion.mobi