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

[Denis Kenzior <denkenz@gmail.com>]

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