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

[Gustavo F. Padovan <padovan@profusion.mobi>]

[-- 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