[PATCH 0/3] atom watch cleanup

[PATCH 0/3] atom watch cleanup

[Mika Liljeberg]

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

Hi,

This patch set simplifies atom watch registration and removes
redundant code.

This approach also correctly handles the case when there are
multiple atoms of the same type (e.g. parallel probing of
alternate drivers or GPRS context drivers).

Br,

	MikaL

[PATCH 1/3] core: notify watches of already registered atoms
[PATCH 2/3] core: remove redundant code
[PATCH 3/3] atmodem: remove redundant code

 drivers/atmodem/sim-poll.c |   13 +------------
 src/call-barring.c         |    7 -------
 src/call-forwarding.c      |    7 -------
 src/call-settings.c        |    7 -------
 src/cbs.c                  |    7 -------
 src/gprs.c                 |    7 -------
 src/modem.c                |   16 +++++++++++++++-
 src/sms.c                  |   11 -----------
 src/voicecall.c            |    6 ------
 9 files changed, 16 insertions(+), 65 deletions(-)

[PATCH 1/3] core: notify watches of already registered atoms

[Mika Liljeberg]

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

---
 src/modem.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index 655994b..d22c718 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -271,6 +271,9 @@ unsigned int __ofono_modem_add_atom_watch(struct ofono_modem *modem,
 					void *data, ofono_destroy_func destroy)
 {
 	struct atom_watch *watch;
+	unsigned int id;
+	GSList *l;
+	struct ofono_atom *atom;
 
 	if (notify == NULL)
 		return 0;
@@ -282,8 +285,19 @@ unsigned int __ofono_modem_add_atom_watch(struct ofono_modem *modem,
 	watch->item.destroy = destroy;
 	watch->item.notify_data = data;
 
-	return __ofono_watchlist_add_item(modem->atom_watches,
+	id = __ofono_watchlist_add_item(modem->atom_watches,
 					(struct ofono_watchlist_item *)watch);
+
+	for (l = modem->atoms; l; l = l->next) {
+		atom = l->data;
+
+		if (atom->type != type || atom->unregister == NULL)
+			continue;
+
+		notify(atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED, data);
+	}
+
+	return id;
 }
 
 gboolean __ofono_modem_remove_atom_watch(struct ofono_modem *modem,
-- 
1.7.1

Re: [PATCH 1/3] core: notify watches of already registered atoms

[Denis Kenzior]

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

Hi Mika,

On 03/29/2011 02:48 AM, Mika Liljeberg wrote:
> ---
>  src/modem.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/src/modem.c b/src/modem.c
> index 655994b..d22c718 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -271,6 +271,9 @@ unsigned int __ofono_modem_add_atom_watch(struct ofono_modem *modem,
>  					void *data, ofono_destroy_func destroy)
>  {
>  	struct atom_watch *watch;
> +	unsigned int id;
> +	GSList *l;
> +	struct ofono_atom *atom;
>  
>  	if (notify == NULL)
>  		return 0;
> @@ -282,8 +285,19 @@ unsigned int __ofono_modem_add_atom_watch(struct ofono_modem *modem,
>  	watch->item.destroy = destroy;
>  	watch->item.notify_data = data;
>  
> -	return __ofono_watchlist_add_item(modem->atom_watches,
> +	id = __ofono_watchlist_add_item(modem->atom_watches,
>  					(struct ofono_watchlist_item *)watch);
> +
> +	for (l = modem->atoms; l; l = l->next) {
> +		atom = l->data;
> +
> +		if (atom->type != type || atom->unregister == NULL)
> +			continue;
> +
> +		notify(atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED, data);
> +	}
> +
> +	return id;
>  }
>  
>  gboolean __ofono_modem_remove_atom_watch(struct ofono_modem *modem,

This patch is behavior changing.  Whether this is a good idea or not I'm
still undecided, though I see its merits.

However, I don't see how it is fixing anything.  The watches are called
only upon registration (which means the driver is probed and ready).  So
even if you create two netreg atoms in hopes of probing the modem type,
only one should ever register and this code should not be needed.

Regards,
-Denis

RE: [PATCH 1/3] core: notify watches of already registered atoms

[Mika.Liljeberg]

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

Hi Denis,

> On 03/29/2011 02:48 AM, Mika Liljeberg wrote:
> > ---
> >  src/modem.c |   16 +++++++++++++++-
> >  1 files changed, 15 insertions(+), 1 deletions(-)
> > 
> > diff --git a/src/modem.c b/src/modem.c
> > index 655994b..d22c718 100644
> > --- a/src/modem.c
> > +++ b/src/modem.c
> > @@ -271,6 +271,9 @@ unsigned int 
> __ofono_modem_add_atom_watch(struct ofono_modem *modem,
> >  					void *data, 
> ofono_destroy_func destroy)
> >  {
> >  	struct atom_watch *watch;
> > +	unsigned int id;
> > +	GSList *l;
> > +	struct ofono_atom *atom;
> >  
> >  	if (notify == NULL)
> >  		return 0;
> > @@ -282,8 +285,19 @@ unsigned int 
> __ofono_modem_add_atom_watch(struct ofono_modem *modem,
> >  	watch->item.destroy = destroy;
> >  	watch->item.notify_data = data;
> >  
> > -	return __ofono_watchlist_add_item(modem->atom_watches,
> > +	id = __ofono_watchlist_add_item(modem->atom_watches,
> >  					(struct 
> ofono_watchlist_item *)watch);
> > +
> > +	for (l = modem->atoms; l; l = l->next) {
> > +		atom = l->data;
> > +
> > +		if (atom->type != type || atom->unregister == NULL)
> > +			continue;
> > +
> > +		notify(atom, 
> OFONO_ATOM_WATCH_CONDITION_REGISTERED, data);
> > +	}
> > +
> > +	return id;
> >  }
> >  
> >  gboolean __ofono_modem_remove_atom_watch(struct ofono_modem *modem,
> 
> This patch is behavior changing.  Whether this is a good idea 
> or not I'm
> still undecided, though I see its merits.
> 
> However, I don't see how it is fixing anything.  The watches 
> are called
> only upon registration (which means the driver is probed and 
> ready).  So
> even if you create two netreg atoms in hopes of probing the 
> modem type,
> only one should ever register and this code should not be needed.

I do have an actual bug, which this patch fixes. I've been investigating why, with isiusb, the gprs atom sometimes fails to receive the network registration status and therefore fails to attach to the GPRS service. It turns out that the problem comes from the client side code pattern for registering atom watches. The code assumes that there will only be a single atom of the same type (registered or not).

Basically, gprs atom and the alternative netreg atoms are probing in parallel and may register themselves in a random order. If gprs finishes first, it will correctly get a call to its watch handler when one of the netreg atoms completes its registration. However, if the order is reversed and gprs finishes after the netreg atom, the following snippet of code tries to find the already registered netreg atom:

        netreg_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_NETREG);

        if (netreg_atom && __ofono_atom_get_registered(netreg_atom))
                netreg_watch(netreg_atom,
                                OFONO_ATOM_WATCH_CONDITION_REGISTERED, gprs);

The trouble here is that __ofono_modem_find_atom() just returns the first neterg atom it finds, regardless of whether it is registered or not. In this case it happens to be the wrong netreg (wgmodem2.5 version, which fails to probe), even though the other netreg atom is already present and registered. Because of this, the watch funtion is never called.

I realize that there are other ways to fix this. The client side pattern could be changed to use __ofono_modem_foreach_atom() to look for already registered atoms, instead of doing the same within the __ofono_modem_add_atom_watch() as in my patch. This would introduce quite a bit of more code. Alternatively, an __ofono_modem_find_registered_atom() function could be introduced to look for a registerd atom of a certain type (might be a good idea to do this in any case). Or isiusb (and any other similar cases) could be revectored to not probe multiple atoms of the same type in parallel. However, we already create multiple GPRS context atoms as a matter of course, so the assumption that there will only be a single instance is no longer valid for all atom types anyway. IMO, it would be good to get rid of the assumption altogether.

So, I like my patch because it removes a lot of copy-paste code and does not assume anything about how many atoms of a certain type there can be.

Regards,

	MikaL

Re: [PATCH 1/3] core: notify watches of already registered atoms

[Denis Kenzior]

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

Hi Mika,

> I do have an actual bug, which this patch fixes. I've been investigating why, with isiusb, the gprs atom sometimes fails to receive the network registration status and therefore fails to attach to the GPRS service. It turns out that the problem comes from the client side code pattern for registering atom watches. The code assumes that there will only be a single atom of the same type (registered or not).
> 
> Basically, gprs atom and the alternative netreg atoms are probing in parallel and may register themselves in a random order. If gprs finishes first, it will correctly get a call to its watch handler when one of the netreg atoms completes its registration. However, if the order is reversed and gprs finishes after the netreg atom, the following snippet of code tries to find the already registered netreg atom:
> 
>         netreg_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_NETREG);
> 
>         if (netreg_atom && __ofono_atom_get_registered(netreg_atom))
>                 netreg_watch(netreg_atom,
>                                 OFONO_ATOM_WATCH_CONDITION_REGISTERED, gprs);
> 
> The trouble here is that __ofono_modem_find_atom() just returns the first neterg atom it finds, regardless of whether it is registered or not. In this case it happens to be the wrong netreg (wgmodem2.5 version, which fails to probe), even though the other netreg atom is already present and registered. Because of this, the watch funtion is never called.
> 

I changed the behavior of __ofono_modem_find_atom to only return ones
that are registered.  This is really what we want anyway, the only
exception was the devinfo atom which was never being registered.
However, I fixed that as well.

> I realize that there are other ways to fix this. The client side pattern could be changed to use __ofono_modem_foreach_atom() to look for already registered atoms, instead of doing the same within the __ofono_modem_add_atom_watch() as in my patch. This would introduce quite a bit of more code. Alternatively, an __ofono_modem_find_registered_atom() function could be introduced to look for a registerd atom of a certain type (might be a good idea to do this in any case). Or isiusb (and any other similar cases) could be revectored to not probe multiple atoms of the same type in parallel. However, we already create multiple GPRS context atoms as a matter of course, so the assumption that there will only be a single instance is no longer valid for all atom types anyway. IMO, it would be good to get rid of the assumption altogether.
> 

The current code was really not setup to have multiple netreg atoms.
That is just not something we wanted to allow.  This might be OK in this
particular case where a single driver is involved.  However, if some
modem driver ends up creating every single atom twice to figure out what
modem is on the other side, then it is a serious performance issue.
Ideally I'd like the modem driver to be fixed.

> So, I like my patch because it removes a lot of copy-paste code and does not assume anything about how many atoms of a certain type there can be.
> 

In the end I agree and I applied all three of your patches and caught
some other occurrences you missed.

Regards,
-Denis

[PATCH 2/3] core: remove redundant code

[Mika Liljeberg]

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

---
 src/call-barring.c    |    7 -------
 src/call-forwarding.c |    7 -------
 src/call-settings.c   |    7 -------
 src/cbs.c             |    7 -------
 src/gprs.c            |    7 -------
 src/sms.c             |   11 -----------
 src/voicecall.c       |    6 ------
 7 files changed, 0 insertions(+), 52 deletions(-)

diff --git a/src/call-barring.c b/src/call-barring.c
index e6570e0..d2f88c6 100644
--- a/src/call-barring.c
+++ b/src/call-barring.c
@@ -1092,7 +1092,6 @@ void ofono_call_barring_register(struct ofono_call_barring *cb)
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path = __ofono_atom_get_path(cb->atom);
 	struct ofono_modem *modem = __ofono_atom_get_modem(cb->atom);
-	struct ofono_atom *ussd_atom;
 
 	if (!g_dbus_register_interface(conn, path,
 					OFONO_CALL_BARRING_INTERFACE,
@@ -1110,12 +1109,6 @@ void ofono_call_barring_register(struct ofono_call_barring *cb)
 					OFONO_ATOM_TYPE_USSD,
 					ussd_watch, cb, NULL);
 
-	ussd_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_USSD);
-
-	if (ussd_atom && __ofono_atom_get_registered(ussd_atom))
-		ussd_watch(ussd_atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED,
-				cb);
-
 	__ofono_atom_register(cb->atom, call_barring_unregister);
 }
 
diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 1ba588a..84d3067 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -1539,7 +1539,6 @@ void ofono_call_forwarding_register(struct ofono_call_forwarding *cf)
 	const char *path = __ofono_atom_get_path(cf->atom);
 	struct ofono_modem *modem = __ofono_atom_get_modem(cf->atom);
 	struct ofono_atom *sim_atom;
-	struct ofono_atom *ussd_atom;
 
 	if (!g_dbus_register_interface(conn, path,
 					OFONO_CALL_FORWARDING_INTERFACE,
@@ -1566,12 +1565,6 @@ void ofono_call_forwarding_register(struct ofono_call_forwarding *cf)
 					OFONO_ATOM_TYPE_USSD,
 					ussd_watch, cf, NULL);
 
-	ussd_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_USSD);
-
-	if (ussd_atom && __ofono_atom_get_registered(ussd_atom))
-		ussd_watch(ussd_atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED,
-				cf);
-
 	__ofono_atom_register(cf->atom, call_forwarding_unregister);
 }
 
diff --git a/src/call-settings.c b/src/call-settings.c
index ede1a88..2d68adc 100644
--- a/src/call-settings.c
+++ b/src/call-settings.c
@@ -1454,7 +1454,6 @@ void ofono_call_settings_register(struct ofono_call_settings *cs)
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path = __ofono_atom_get_path(cs->atom);
 	struct ofono_modem *modem = __ofono_atom_get_modem(cs->atom);
-	struct ofono_atom *ussd_atom;
 
 	if (!g_dbus_register_interface(conn, path,
 					OFONO_CALL_SETTINGS_INTERFACE,
@@ -1472,12 +1471,6 @@ void ofono_call_settings_register(struct ofono_call_settings *cs)
 					OFONO_ATOM_TYPE_USSD,
 					ussd_watch, cs, NULL);
 
-	ussd_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_USSD);
-
-	if (ussd_atom && __ofono_atom_get_registered(ussd_atom))
-		ussd_watch(ussd_atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED,
-				cs);
-
 	__ofono_atom_register(cs->atom, call_settings_unregister);
 }
 
diff --git a/src/cbs.c b/src/cbs.c
index d99f250..d81104e 100644
--- a/src/cbs.c
+++ b/src/cbs.c
@@ -1087,7 +1087,6 @@ void ofono_cbs_register(struct ofono_cbs *cbs)
 	const char *path = __ofono_atom_get_path(cbs->atom);
 	struct ofono_atom *sim_atom;
 	struct ofono_atom *stk_atom;
-	struct ofono_atom *netreg_atom;
 
 	if (!g_dbus_register_interface(conn, path,
 					OFONO_CELL_BROADCAST_INTERFACE,
@@ -1119,12 +1118,6 @@ void ofono_cbs_register(struct ofono_cbs *cbs)
 					OFONO_ATOM_TYPE_NETREG,
 					netreg_watch, cbs, NULL);
 
-	netreg_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_NETREG);
-
-	if (netreg_atom && __ofono_atom_get_registered(netreg_atom))
-		netreg_watch(netreg_atom,
-				OFONO_ATOM_WATCH_CONDITION_REGISTERED, cbs);
-
 	__ofono_atom_register(cbs->atom, cbs_unregister);
 }
 
diff --git a/src/gprs.c b/src/gprs.c
index f9e327a..deffeb8 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -2869,7 +2869,6 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
 	DBusConnection *conn = ofono_dbus_get_connection();
 	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
 	const char *path = __ofono_atom_get_path(gprs->atom);
-	struct ofono_atom *netreg_atom;
 
 	if (gprs->contexts == NULL) /* Automatic provisioning failed */
 		add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
@@ -2892,12 +2891,6 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
 					OFONO_ATOM_TYPE_NETREG,
 					netreg_watch, gprs, NULL);
 
-	netreg_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_NETREG);
-
-	if (netreg_atom && __ofono_atom_get_registered(netreg_atom))
-		netreg_watch(netreg_atom,
-				OFONO_ATOM_WATCH_CONDITION_REGISTERED, gprs);
-
 	__ofono_atom_register(gprs->atom, gprs_unregister);
 }
 
diff --git a/src/sms.c b/src/sms.c
index a413a37..c17e5c8 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -1955,7 +1955,6 @@ void ofono_sms_register(struct ofono_sms *sms)
 	DBusConnection *conn = ofono_dbus_get_connection();
 	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
 	const char *path = __ofono_atom_get_path(sms->atom);
-	struct ofono_atom *atom;
 	struct ofono_atom *sim_atom;
 
 	if (!g_dbus_register_interface(conn, path,
@@ -1974,20 +1973,10 @@ void ofono_sms_register(struct ofono_sms *sms)
 					OFONO_ATOM_TYPE_MESSAGE_WAITING,
 					mw_watch, sms, NULL);
 
-	atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_MESSAGE_WAITING);
-
-	if (atom && __ofono_atom_get_registered(atom))
-		mw_watch(atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED, sms);
-
 	sms->netreg_watch = __ofono_modem_add_atom_watch(modem,
 					OFONO_ATOM_TYPE_NETREG,
 					netreg_watch, sms, NULL);
 
-	atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_NETREG);
-
-	if (atom && __ofono_atom_get_registered(atom))
-		netreg_watch(atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED, sms);
-
 	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
 
 	/*
diff --git a/src/voicecall.c b/src/voicecall.c
index b1d5586..afda96a 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -2368,7 +2368,6 @@ void ofono_voicecall_register(struct ofono_voicecall *vc)
 	DBusConnection *conn = ofono_dbus_get_connection();
 	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
 	const char *path = __ofono_atom_get_path(vc->atom);
-	struct ofono_atom *sim_atom;
 
 	if (!g_dbus_register_interface(conn, path,
 					OFONO_VOICECALL_MANAGER_INTERFACE,
@@ -2393,11 +2392,6 @@ void ofono_voicecall_register(struct ofono_voicecall *vc)
 						OFONO_ATOM_TYPE_SIM,
 						sim_watch, vc, NULL);
 
-	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
-
-	if (sim_atom && __ofono_atom_get_registered(sim_atom))
-		sim_watch(sim_atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED, vc);
-
 	__ofono_atom_register(vc->atom, voicecall_unregister);
 }
 
-- 
1.7.1

[PATCH 3/3] atmodem: remove redundant code

[Mika Liljeberg]

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

---
 drivers/atmodem/sim-poll.c |   13 +------------
 1 files changed, 1 insertions(+), 12 deletions(-)

diff --git a/drivers/atmodem/sim-poll.c b/drivers/atmodem/sim-poll.c
index daef24a..da00ddd 100644
--- a/drivers/atmodem/sim-poll.c
+++ b/drivers/atmodem/sim-poll.c
@@ -239,14 +239,9 @@ static void stk_watch(struct ofono_atom *atom,
 
 void atmodem_poll_enable(struct ofono_modem *modem, GAtChat *chat)
 {
-	struct ofono_atom *sim_atom;
-	struct ofono_atom *stk_atom;
 	struct sim_poll_data *spd;
 
-	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
-	stk_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_STK);
-
-	if (sim_atom == NULL)
+	if (__ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM) == NULL)
 		return;
 
 	spd = g_new0(struct sim_poll_data, 1);
@@ -256,13 +251,7 @@ void atmodem_poll_enable(struct ofono_modem *modem, GAtChat *chat)
 
 	spd->stk_watch = __ofono_modem_add_atom_watch(spd->modem,
 			OFONO_ATOM_TYPE_STK, stk_watch, spd, NULL);
-	if (stk_atom && __ofono_atom_get_registered(stk_atom))
-		stk_watch(stk_atom,
-				OFONO_ATOM_WATCH_CONDITION_REGISTERED, spd);
 
 	spd->sim_watch = __ofono_modem_add_atom_watch(spd->modem,
 			OFONO_ATOM_TYPE_SIM, sim_watch, spd, NULL);
-	if (__ofono_atom_get_registered(sim_atom))
-		sim_watch(sim_atom,
-				OFONO_ATOM_WATCH_CONDITION_REGISTERED, spd);
 }
-- 
1.7.1