[PATCH] src/modem.c/call_watches null pointer check

[PATCH] src/modem.c/call_watches null pointer check

[Giacinto Cifelli]

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

check the pointer modem->atom_watches, that can be null when the
function is called during the disable/remove of the device,
for example when it is unplugged or switched off.
---
 src/modem.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/modem.c b/src/modem.c
index 9e254482..124a5192 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -255,11 +255,16 @@ static void call_watches(struct ofono_atom *atom,
 				enum ofono_atom_watch_condition cond)
 {
 	struct ofono_modem *modem = atom->modem;
-	GSList *atom_watches = modem->atom_watches->items;
+	GSList *atom_watches;
 	GSList *l;
 	struct atom_watch *watch;
 	ofono_atom_watch_func notify;
 
+	if (!modem->atom_watches)
+		return;
+
+	atom_watches = modem->atom_watches->items;
+
 	for (l = atom_watches; l; l = l->next) {
 		watch = l->data;
 
-- 
2.17.1

[PATCH] src/modem: connection timeout to 60 seconds

[Giacinto Cifelli]

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

Changed the connection timeout to 60 seconds (from 20 seconds),
to accomodate for chipset that take time to boot-up.
---
 src/modem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index 124a5192..e7978bb0 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -1060,7 +1060,7 @@ static DBusMessage *set_property_lockdown(struct ofono_modem *modem,
 		}
 
 		modem->pending = dbus_message_ref(msg);
-		modem->timeout = g_timeout_add_seconds(20,
+		modem->timeout = g_timeout_add_seconds(60,
 						set_powered_timeout, modem);
 		return NULL;
 	}
@@ -1138,7 +1138,7 @@ static DBusMessage *modem_set_property(DBusConnection *conn,
 				return __ofono_error_failed(msg);
 
 			modem->pending = dbus_message_ref(msg);
-			modem->timeout = g_timeout_add_seconds(20,
+			modem->timeout = g_timeout_add_seconds(60,
 						set_powered_timeout, modem);
 			return NULL;
 		}
-- 
2.17.1

Re: [PATCH] src/modem: connection timeout to 60 seconds

[Jonas Bonn]

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

Hi,

Do the USB interfaces show up long before the modem is ready to accept 
AT commands?  Or do the AT commands take a long time to complete?

/Jonas

On 23/10/18 09:50, Giacinto Cifelli wrote:
> Changed the connection timeout to 60 seconds (from 20 seconds),
> to accomodate for chipset that take time to boot-up.
> ---
>   src/modem.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/modem.c b/src/modem.c
> index 124a5192..e7978bb0 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -1060,7 +1060,7 @@ static DBusMessage *set_property_lockdown(struct ofono_modem *modem,
>   		}
>   
>   		modem->pending = dbus_message_ref(msg);
> -		modem->timeout = g_timeout_add_seconds(20,
> +		modem->timeout = g_timeout_add_seconds(60,
>   						set_powered_timeout, modem);
>   		return NULL;
>   	}
> @@ -1138,7 +1138,7 @@ static DBusMessage *modem_set_property(DBusConnection *conn,
>   				return __ofono_error_failed(msg);
>   
>   			modem->pending = dbus_message_ref(msg);
> -			modem->timeout = g_timeout_add_seconds(20,
> +			modem->timeout = g_timeout_add_seconds(60,
>   						set_powered_timeout, modem);
>   			return NULL;
>   		}
> 

Re: [PATCH] src/modem: connection timeout to 60 seconds

[Giacinto Cifelli]

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

Hi,

On Tue, Oct 23, 2018 at 10:34 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Hi,
>
> Do the USB interfaces show up long before the modem is ready to accept
> AT commands?  Or do the AT commands take a long time to complete?

The USB shows up long before the modem is ready to accept AT commands,
and - if supported - the MBIM OPEN takes a long time to complete.

>
> /Jonas

Giacinto

>
> On 23/10/18 09:50, Giacinto Cifelli wrote:
> > Changed the connection timeout to 60 seconds (from 20 seconds),
> > to accomodate for chipset that take time to boot-up.
> > ---
> >   src/modem.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/modem.c b/src/modem.c
> > index 124a5192..e7978bb0 100644
> > --- a/src/modem.c
> > +++ b/src/modem.c
> > @@ -1060,7 +1060,7 @@ static DBusMessage *set_property_lockdown(struct ofono_modem *modem,
> >               }
> >
> >               modem->pending = dbus_message_ref(msg);
> > -             modem->timeout = g_timeout_add_seconds(20,
> > +             modem->timeout = g_timeout_add_seconds(60,
> >                                               set_powered_timeout, modem);
> >               return NULL;
> >       }
> > @@ -1138,7 +1138,7 @@ static DBusMessage *modem_set_property(DBusConnection *conn,
> >                               return __ofono_error_failed(msg);
> >
> >                       modem->pending = dbus_message_ref(msg);
> > -                     modem->timeout = g_timeout_add_seconds(20,
> > +                     modem->timeout = g_timeout_add_seconds(60,
> >                                               set_powered_timeout, modem);
> >                       return NULL;
> >               }
> >

Re: [PATCH] src/modem: connection timeout to 60 seconds

[Jonas Bonn]

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



On 23/10/18 10:40, Giacinto Cifelli wrote:
> Hi,
> 
> On Tue, Oct 23, 2018 at 10:34 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>>
>> Hi,
>>
>> Do the USB interfaces show up long before the modem is ready to accept
>> AT commands?  Or do the AT commands take a long time to complete?
> 
> The USB shows up long before the modem is ready to accept AT commands,
> and - if supported - the MBIM OPEN takes a long time to complete.
> 

This is where I think adding a data item in udevng would be in order. 
...set_int(..., "PoweredTimeout", 60)... or something like that.  It's a 
device quirk, after all.  I've dealt with modems that were slow to boot, 
but usually they don't present the USB interfaces until they are ready 
to go.

/Jonas

>>
>> /Jonas
> 
> Giacinto
> 
>>
>> On 23/10/18 09:50, Giacinto Cifelli wrote:
>>> Changed the connection timeout to 60 seconds (from 20 seconds),
>>> to accomodate for chipset that take time to boot-up.
>>> ---
>>>    src/modem.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/modem.c b/src/modem.c
>>> index 124a5192..e7978bb0 100644
>>> --- a/src/modem.c
>>> +++ b/src/modem.c
>>> @@ -1060,7 +1060,7 @@ static DBusMessage *set_property_lockdown(struct ofono_modem *modem,
>>>                }
>>>
>>>                modem->pending = dbus_message_ref(msg);
>>> -             modem->timeout = g_timeout_add_seconds(20,
>>> +             modem->timeout = g_timeout_add_seconds(60,
>>>                                                set_powered_timeout, modem);
>>>                return NULL;
>>>        }
>>> @@ -1138,7 +1138,7 @@ static DBusMessage *modem_set_property(DBusConnection *conn,
>>>                                return __ofono_error_failed(msg);
>>>
>>>                        modem->pending = dbus_message_ref(msg);
>>> -                     modem->timeout = g_timeout_add_seconds(20,
>>> +                     modem->timeout = g_timeout_add_seconds(60,
>>>                                                set_powered_timeout, modem);
>>>                        return NULL;
>>>                }
>>>

Re: [PATCH] src/modem: connection timeout to 60 seconds

[Giacinto Cifelli]

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

Hi Jonas
>
> This is where I think adding a data item in udevng would be in order.
> ...set_int(..., "PoweredTimeout", 60)... or something like that.  It's a
> device quirk, after all.  I've dealt with modems that were slow to boot,
> but usually they don't present the USB interfaces until they are ready
> to go.

I had in mind a plugin function that would return the timeout it needs
(to be called between .probe and .enable), something like .get_params.

Your parameter should be set in a udev rule, otherwise it would apply
to all modems connected, and so it is marginally better than the
current hardcoding.
Adding udev rules is tedious, USB devices are supposed to work 'out of
the box', without manual tweaks.

So, eventually it is easier to just increase the timeout :)

Regards,
Giacinto

>
> /Jonas
>
> >>
> >> /Jonas
> >
> > Giacinto
> >
> >>
> >> On 23/10/18 09:50, Giacinto Cifelli wrote:
> >>> Changed the connection timeout to 60 seconds (from 20 seconds),
> >>> to accomodate for chipset that take time to boot-up.
> >>> ---
> >>>    src/modem.c | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/src/modem.c b/src/modem.c
> >>> index 124a5192..e7978bb0 100644
> >>> --- a/src/modem.c
> >>> +++ b/src/modem.c
> >>> @@ -1060,7 +1060,7 @@ static DBusMessage *set_property_lockdown(struct ofono_modem *modem,
> >>>                }
> >>>
> >>>                modem->pending = dbus_message_ref(msg);
> >>> -             modem->timeout = g_timeout_add_seconds(20,
> >>> +             modem->timeout = g_timeout_add_seconds(60,
> >>>                                                set_powered_timeout, modem);
> >>>                return NULL;
> >>>        }
> >>> @@ -1138,7 +1138,7 @@ static DBusMessage *modem_set_property(DBusConnection *conn,
> >>>                                return __ofono_error_failed(msg);
> >>>
> >>>                        modem->pending = dbus_message_ref(msg);
> >>> -                     modem->timeout = g_timeout_add_seconds(20,
> >>> +                     modem->timeout = g_timeout_add_seconds(60,
> >>>                                                set_powered_timeout, modem);
> >>>                        return NULL;
> >>>                }
> >>>

Re: [PATCH] src/modem: connection timeout to 60 seconds

[Slava Monich]

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

On 23/10/18 10:50, Giacinto Cifelli wrote:
> Changed the connection timeout to 60 seconds (from 20 seconds),
> to accomodate for chipset that take time to boot-up.

Is it hard to make it configurable?

Cheers,
-Slava

Re: [PATCH] src/modem: connection timeout to 60 seconds

[Jonas Bonn]

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



On 23/10/18 11:26, Giacinto Cifelli wrote:
> Hi Jonas
>>
>> This is where I think adding a data item in udevng would be in order.
>> ...set_int(..., "PoweredTimeout", 60)... or something like that.  It's a
>> device quirk, after all.  I've dealt with modems that were slow to boot,
>> but usually they don't present the USB interfaces until they are ready
>> to go.
> 
> I had in mind a plugin function that would return the timeout it needs
> (to be called between .probe and .enable), something like .get_params.
> 
> Your parameter should be set in a udev rule, otherwise it would apply
> to all modems connected, and so it is marginally better than the
> current hardcoding.
> Adding udev rules is tedious, USB devices are supposed to work 'out of
> the box', without manual tweaks.

No, not in a udev rule.  I was thinking of adding a line like this to 
setup_gemalto() in plugins/udevng.c:

if (g_strcmp0(modem->model, "xxxx"))
	ofono_modem_set_int(modem->modem, "PoweredTimeout", 60);

('model' is hexadecimal, numeric value, so you could parse it and do a 
switch() on it if you don't like the strcmp...)

Then query the setting in src/modem.c.

/Jonas

Re: [PATCH] src/modem.c/call_watches null pointer check

[Denis Kenzior]

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

Hi Giacinto,

On 10/23/2018 02:50 AM, Giacinto Cifelli wrote:
> check the pointer modem->atom_watches, that can be null when the
> function is called during the disable/remove of the device,
> for example when it is unplugged or switched off.

If this caused a crash, a full stack trace would be helpful and should 
be part of this commit description.

Regards,
-Denis

Re: [PATCH] src/modem.c/call_watches null pointer check

[Giacinto Cifelli]

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

Hi Denis,

On Tue, Oct 23, 2018 at 4:30 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> On 10/23/2018 02:50 AM, Giacinto Cifelli wrote:
> > check the pointer modem->atom_watches, that can be null when the
> > function is called during the disable/remove of the device,
> > for example when it is unplugged or switched off.
>
> If this caused a crash, a full stack trace would be helpful and should
> be part of this commit description.

you mean something like the following or more?

[2018-10-23 16:42:57+0200] ofonod[18153]:
plugins/udevng.c:remove_device()
/sys/devices/pci0000:00/0000:00:14.0/usb3/3-6/3-6:1.0/tty/ttyACM0
[2018-10-23 16:42:57+0200] ofonod[18153]:
plugins/udevng.c:destroy_modem()
/sys/devices/pci0000:00/0000:00:14.0/usb3/3-6
[2018-10-23 16:42:57+0200] ofonod[18153]:
src/modem.c:ofono_modem_remove() 0x5642795b0f00
[2018-10-23 16:42:57+0200] ofonod[18153]:
src/modem.c:modem_unregister() 0x5642795b0f00
[2018-10-23 16:42:57+0200] ofonod[18153]:
src/modem.c:modem_change_state() old state: 2, new state: 0
[2018-10-23 16:42:57+0200] ofonod[18153]: src/modem.c:flush_atoms()
[2018-10-23 16:42:57+0200] ofonod[18153]: src/lte.c:lte_atom_remove()
atom: 0x5642795c5010
[2018-10-23 16:42:57+0200] ofonod[18153]:
src/phonebook.c:phonebook_remove() atom: 0x5642795cbac0
[2018-10-23 16:42:57+0200] ofonod[18153]: src/sim.c:sim_remove() atom:
0x5642795b0c40
[2018-10-23 16:42:57+0200] ofonod[18153]:
plugins/gemalto.c:gemalto_disable() 0x5642795b0f00
[2018-10-23 16:42:57+0200] ofonod[18153]: Aborting (signal 11) [src/ofonod]
[2018-10-23 16:42:57+0200] ofonod[18153]: ++++++++ backtrace ++++++++
[2018-10-23 16:42:57+0200] ofonod[18153]: #0  0x7ffad5b36f20 in
/lib/x86_64-linux-gnu/libc.so.6
[2018-10-23 16:42:57+0200] ofonod[18153]: #1  0x5642791cb6ec in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #2  0x5642791cc32e in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #3  0x5642791cd45c in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #4  0x5642791a3f27 in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #5  0x5642791cc71a in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #6  0x5642791ce6a4 in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #7  0x5642791505d3 in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #8  0x7ffad6374091 in
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
[2018-10-23 16:42:57+0200] ofonod[18153]: #9  0x56427915561a in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #10 0x7ffad63860f5 in
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
[2018-10-23 16:42:57+0200] ofonod[18153]: #11 0x7ffad63864c0 in
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
[2018-10-23 16:42:57+0200] ofonod[18153]: #12 0x7ffad63867d2 in
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
[2018-10-23 16:42:57+0200] ofonod[18153]: #13 0x564279150397 in src/ofonod
[2018-10-23 16:42:57+0200] ofonod[18153]: #14 0x7ffad5b19b97 in
/lib/x86_64-linux-gnu/libc.so.6
[2018-10-23 16:42:57+0200] ofonod[18153]: +++++++++++++++++++++++++++


>
> Regards,
> -Denis

Regards,
Giacinto

Re: [PATCH] src/modem.c/call_watches null pointer check

[Denis Kenzior]

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

Hi Giacinto,
> 
> you mean something like the following or more?
> 

Yes, but something that is actually useful and not a bunch of memory 
locations.  e.g.

git show a99c0be535410a92773ffdfbebb766bec66b66fe

Regards,
-Denis

Re: [PATCH] src/modem: connection timeout to 60 seconds

[Denis Kenzior]

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

Hi Giacinto,

On 10/23/2018 03:40 AM, Giacinto Cifelli wrote:
> Hi,
> 
> On Tue, Oct 23, 2018 at 10:34 AM Jonas Bonn <jonas@norrbonn.se> wrote:
>>
>> Hi,
>>
>> Do the USB interfaces show up long before the modem is ready to accept
>> AT commands?  Or do the AT commands take a long time to complete?
> 
> The USB shows up long before the modem is ready to accept AT commands,
> and - if supported - the MBIM OPEN takes a long time to complete.
> 

So you really need to have a serious talk to your firmware guys about 
fixing this.  Why is the modem jumping on the bus before it is ready to 
accept AT commands?

In my 10+ years I've never seen a modem do this.  Some take 5 seconds, 
maybe 10.  I've never seen anything more than that, which is why that 20 
second value was quite conservative when I added it.

Anyhow, go ahead and add ofono_modem_set_powered_timeout_hint(unsigned 
int seconds) and have udevng detection fix this.

Regards,
-Denis

Re: [PATCH] src/modem.c/call_watches null pointer check

[Giacinto Cifelli]

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

Hi Denis,

On Tue, Oct 23, 2018 at 4:47 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
> >
> > you mean something like the following or more?
> >
>
> Yes, but something that is actually useful and not a bunch of memory
> locations.  e.g.
>
> git show a99c0be535410a92773ffdfbebb766bec66b66fe
>
> Regards,
> -Denis

   0  0x0000563b1842b735 in call_watches
(atom=atom(a)entry=0x563b1a3f07d0,
cond=cond(a)entry=OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) at
src/modem.c:265
   1  0x0000563b1842c32e in __ofono_atom_unregister
(atom=atom(a)entry=0x563b1a3f07d0) at src/modem.c:299
   2  0x0000563b1842c3ff in __ofono_atom_unregister
(atom=0x563b1a3f07d0) at src/modem.c:296
   3  flush_atoms (modem=0x563b1a3f1f50,
new_state=MODEM_STATE_POWER_OFF) at src/modem.c:448
   4  modem_change_state (modem=0x563b1a3f1f50,
new_state=MODEM_STATE_POWER_OFF) at src/modem.c:529
   5  0x0000563b1842c577 in set_powered
(modem=modem(a)entry=0x563b1a3f1f50, powered=powered(a)entry=0) at
src/modem.c:915
   6  0x0000563b1842c863 in modem_unregister
(modem=modem(a)entry=0x563b1a3f1f50) at src/modem.c:2111
   7  0x0000563b1842e6a4 in ofono_modem_remove (modem=0x563b1a3f1f50)
at src/modem.c:2177
   8  0x0000563b183b05d3 in destroy_modem (data=0x563b1a3e50a0) at
plugins/udevng.c:1408
   9  0x00007f42b4bdd091 in ?? () from
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
   10 0x0000563b183b561a in remove_device (device=0x563b1a401630) at
plugins/udevng.c:1468
   11 udev_event (channel=<optimized out>, cond=<optimized out>,
user_data=<optimized out>) at plugins/udevng.c:1991
   12 0x00007f42b4bef0f5 in g_main_context_dispatch () from
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
   13 0x00007f42b4bef4c0 in ?? () from
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
   14 0x00007f42b4bef7d2 in g_main_loop_run () from
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
   15 0x0000563b183b0397 in main (argc=<optimized out>,
argv=<optimized out>) at src/main.c:294

?
Regards,
Giacinto

Re: [PATCH] src/modem.c/call_watches null pointer check

[Denis Kenzior]

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

Hi Giacinto,
>     0  0x0000563b1842b735 in call_watches
> (atom=atom(a)entry=0x563b1a3f07d0,
> cond=cond(a)entry=OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) at
> src/modem.c:265
>     1  0x0000563b1842c32e in __ofono_atom_unregister
> (atom=atom(a)entry=0x563b1a3f07d0) at src/modem.c:299
>     2  0x0000563b1842c3ff in __ofono_atom_unregister
> (atom=0x563b1a3f07d0) at src/modem.c:296
>     3  flush_atoms (modem=0x563b1a3f1f50,
> new_state=MODEM_STATE_POWER_OFF) at src/modem.c:448
>     4  modem_change_state (modem=0x563b1a3f1f50,
> new_state=MODEM_STATE_POWER_OFF) at src/modem.c:529
>     5  0x0000563b1842c577 in set_powered
> (modem=modem(a)entry=0x563b1a3f1f50, powered=powered(a)entry=0) at
> src/modem.c:915
>     6  0x0000563b1842c863 in modem_unregister
> (modem=modem(a)entry=0x563b1a3f1f50) at src/modem.c:2111
>     7  0x0000563b1842e6a4 in ofono_modem_remove (modem=0x563b1a3f1f50)
> at src/modem.c:2177
>     8  0x0000563b183b05d3 in destroy_modem (data=0x563b1a3e50a0) at
> plugins/udevng.c:1408
>     9  0x00007f42b4bdd091 in ?? () from
> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>     10 0x0000563b183b561a in remove_device (device=0x563b1a401630) at
> plugins/udevng.c:1468
>     11 udev_event (channel=<optimized out>, cond=<optimized out>,
> user_data=<optimized out>) at plugins/udevng.c:1991
>     12 0x00007f42b4bef0f5 in g_main_context_dispatch () from
> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>     13 0x00007f42b4bef4c0 in ?? () from
> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>     14 0x00007f42b4bef7d2 in g_main_loop_run () from
> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>     15 0x0000563b183b0397 in main (argc=<optimized out>,
> argv=<optimized out>) at src/main.c:294
> 
> ?

Yes, much better.  So now you can explain to me how you're triggering this.

The only way modem->atom_watches is NULL is if ofono_modem_register 
failed.  So are you trying to use a modem object without registering it 
properly?

Regards,
-Denis

Re: [PATCH] src/modem.c/call_watches null pointer check

[Giacinto Cifelli]

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

Hi Denis,

I didn't know this was a surprise interrogation!

On Tue, Oct 23, 2018 at 6:50 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
> >     0  0x0000563b1842b735 in call_watches
> > (atom=atom(a)entry=0x563b1a3f07d0,
> > cond=cond(a)entry=OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) at
> > src/modem.c:265
> >     1  0x0000563b1842c32e in __ofono_atom_unregister
> > (atom=atom(a)entry=0x563b1a3f07d0) at src/modem.c:299
> >     2  0x0000563b1842c3ff in __ofono_atom_unregister
> > (atom=0x563b1a3f07d0) at src/modem.c:296
> >     3  flush_atoms (modem=0x563b1a3f1f50,
> > new_state=MODEM_STATE_POWER_OFF) at src/modem.c:448
> >     4  modem_change_state (modem=0x563b1a3f1f50,
> > new_state=MODEM_STATE_POWER_OFF) at src/modem.c:529
> >     5  0x0000563b1842c577 in set_powered
> > (modem=modem(a)entry=0x563b1a3f1f50, powered=powered(a)entry=0) at
> > src/modem.c:915
> >     6  0x0000563b1842c863 in modem_unregister
> > (modem=modem(a)entry=0x563b1a3f1f50) at src/modem.c:2111
> >     7  0x0000563b1842e6a4 in ofono_modem_remove (modem=0x563b1a3f1f50)
> > at src/modem.c:2177
> >     8  0x0000563b183b05d3 in destroy_modem (data=0x563b1a3e50a0) at
> > plugins/udevng.c:1408
> >     9  0x00007f42b4bdd091 in ?? () from
> > /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >     10 0x0000563b183b561a in remove_device (device=0x563b1a401630) at
> > plugins/udevng.c:1468
> >     11 udev_event (channel=<optimized out>, cond=<optimized out>,
> > user_data=<optimized out>) at plugins/udevng.c:1991
> >     12 0x00007f42b4bef0f5 in g_main_context_dispatch () from
> > /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >     13 0x00007f42b4bef4c0 in ?? () from
> > /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >     14 0x00007f42b4bef7d2 in g_main_loop_run () from
> > /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >     15 0x0000563b183b0397 in main (argc=<optimized out>,
> > argv=<optimized out>) at src/main.c:294
> >
> > ?
>
> Yes, much better.  So now you can explain to me how you're triggering this.
>
> The only way modem->atom_watches is NULL is if ofono_modem_register
> failed.  So are you trying to use a modem object without registering it
> properly?

the problem is that I have moved location_information_create, along
with devinfo_create at the gemalto_enable() phase, because these two
are available also offline, and it is a normal use case to monitor the
GNSS signal while offline.
But when the modem is removed (unplugged or switched off), the
location_reporting atom is not removed, and the modem does not
disappear. devinfo goes away without any issue.

So in the gemalto_remove I have added:
   lratom = __ofono_modem_find_atom(modem,OFONO_ATOM_TYPE_LOCATION_REPORTING);
   if (lratom)
       __ofono_atom_free(lratom);

this works fine, but maybe the watch is already destroyed?

And the whole point is, I wouldn't have to do it manually. What is
missing in this atom?

>
> Regards,
> -Denis
>

Regards,
Giacinto

Re: [PATCH] src/modem.c/call_watches null pointer check

[Denis Kenzior]

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

Hi Giacinto,

On 10/23/2018 12:30 PM, Giacinto Cifelli wrote:
> Hi Denis,
> 
> I didn't know this was a surprise interrogation!

That's how we roll around here.  Keeps you on your toes.

> 
> On Tue, Oct 23, 2018 at 6:50 PM Denis Kenzior <denkenz@gmail.com> wrote:
>>
>> Hi Giacinto,
>>>      0  0x0000563b1842b735 in call_watches
>>> (atom=atom(a)entry=0x563b1a3f07d0,
>>> cond=cond(a)entry=OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) at
>>> src/modem.c:265
>>>      1  0x0000563b1842c32e in __ofono_atom_unregister
>>> (atom=atom(a)entry=0x563b1a3f07d0) at src/modem.c:299
>>>      2  0x0000563b1842c3ff in __ofono_atom_unregister
>>> (atom=0x563b1a3f07d0) at src/modem.c:296
>>>      3  flush_atoms (modem=0x563b1a3f1f50,
>>> new_state=MODEM_STATE_POWER_OFF) at src/modem.c:448
>>>      4  modem_change_state (modem=0x563b1a3f1f50,
>>> new_state=MODEM_STATE_POWER_OFF) at src/modem.c:529
>>>      5  0x0000563b1842c577 in set_powered
>>> (modem=modem(a)entry=0x563b1a3f1f50, powered=powered(a)entry=0) at
>>> src/modem.c:915
>>>      6  0x0000563b1842c863 in modem_unregister
>>> (modem=modem(a)entry=0x563b1a3f1f50) at src/modem.c:2111
>>>      7  0x0000563b1842e6a4 in ofono_modem_remove (modem=0x563b1a3f1f50)
>>> at src/modem.c:2177
>>>      8  0x0000563b183b05d3 in destroy_modem (data=0x563b1a3e50a0) at
>>> plugins/udevng.c:1408
>>>      9  0x00007f42b4bdd091 in ?? () from
>>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>>>      10 0x0000563b183b561a in remove_device (device=0x563b1a401630) at
>>> plugins/udevng.c:1468
>>>      11 udev_event (channel=<optimized out>, cond=<optimized out>,
>>> user_data=<optimized out>) at plugins/udevng.c:1991
>>>      12 0x00007f42b4bef0f5 in g_main_context_dispatch () from
>>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>>>      13 0x00007f42b4bef4c0 in ?? () from
>>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>>>      14 0x00007f42b4bef7d2 in g_main_loop_run () from
>>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>>>      15 0x0000563b183b0397 in main (argc=<optimized out>,
>>> argv=<optimized out>) at src/main.c:294
>>>
>>> ?
>>
>> Yes, much better.  So now you can explain to me how you're triggering this.
>>
>> The only way modem->atom_watches is NULL is if ofono_modem_register
>> failed.  So are you trying to use a modem object without registering it
>> properly?
> 
> the problem is that I have moved location_information_create, along
> with devinfo_create at the gemalto_enable() phase, because these two
> are available also offline, and it is a normal use case to monitor the
> GNSS signal while offline.

Well you can't do that.  .enable() callback is to power the modem up or 
open up the AT command port and brings the modem to RX/TX circuits off 
stage.  So you cannot be adding any atoms until 
ofono_modem_set_powered(modem, TRUE) has been called.

In fact you should not be calling any atom creation functions outside 
the pre_sim, post_sim or post_online callbacks.  There may be one or two 
exceptions due to modems taking a while to initialize the SMS/phonebook 
store, and we still need to fix that.

However, it still doesn't explain to me why modem->atom_watches are NULL 
in the first place.  Are you calling ofono_modem_registered first?

> But when the modem is removed (unplugged or switched off), the
> location_reporting atom is not removed, and the modem does not
> disappear. devinfo goes away without any issue.
> 
> So in the gemalto_remove I have added:
>     lratom = __ofono_modem_find_atom(modem,OFONO_ATOM_TYPE_LOCATION_REPORTING);
>     if (lratom)
>         __ofono_atom_free(lratom);

This tells me you're doing something very wrong...

> 
> this works fine, but maybe the watch is already destroyed?
> 
> And the whole point is, I wouldn't have to do it manually. What is
> missing in this atom?

Nothing is missing, all atoms are managed automagically.

Regards,
-Denis

Re: [PATCH] src/modem.c/call_watches null pointer check

[Giacinto Cifelli]

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

Hi Denis,

On Tue, Oct 23, 2018 at 7:40 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Giacinto,
>
> On 10/23/2018 12:30 PM, Giacinto Cifelli wrote:
> > Hi Denis,
> >
> > I didn't know this was a surprise interrogation!
>
> That's how we roll around here.  Keeps you on your toes.
>
> >
> > On Tue, Oct 23, 2018 at 6:50 PM Denis Kenzior <denkenz@gmail.com> wrote:
> >>
> >> Hi Giacinto,
> >>>      0  0x0000563b1842b735 in call_watches
> >>> (atom=atom(a)entry=0x563b1a3f07d0,
> >>> cond=cond(a)entry=OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) at
> >>> src/modem.c:265
> >>>      1  0x0000563b1842c32e in __ofono_atom_unregister
> >>> (atom=atom(a)entry=0x563b1a3f07d0) at src/modem.c:299
> >>>      2  0x0000563b1842c3ff in __ofono_atom_unregister
> >>> (atom=0x563b1a3f07d0) at src/modem.c:296
> >>>      3  flush_atoms (modem=0x563b1a3f1f50,
> >>> new_state=MODEM_STATE_POWER_OFF) at src/modem.c:448
> >>>      4  modem_change_state (modem=0x563b1a3f1f50,
> >>> new_state=MODEM_STATE_POWER_OFF) at src/modem.c:529
> >>>      5  0x0000563b1842c577 in set_powered
> >>> (modem=modem(a)entry=0x563b1a3f1f50, powered=powered(a)entry=0) at
> >>> src/modem.c:915
> >>>      6  0x0000563b1842c863 in modem_unregister
> >>> (modem=modem(a)entry=0x563b1a3f1f50) at src/modem.c:2111
> >>>      7  0x0000563b1842e6a4 in ofono_modem_remove (modem=0x563b1a3f1f50)
> >>> at src/modem.c:2177
> >>>      8  0x0000563b183b05d3 in destroy_modem (data=0x563b1a3e50a0) at
> >>> plugins/udevng.c:1408
> >>>      9  0x00007f42b4bdd091 in ?? () from
> >>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >>>      10 0x0000563b183b561a in remove_device (device=0x563b1a401630) at
> >>> plugins/udevng.c:1468
> >>>      11 udev_event (channel=<optimized out>, cond=<optimized out>,
> >>> user_data=<optimized out>) at plugins/udevng.c:1991
> >>>      12 0x00007f42b4bef0f5 in g_main_context_dispatch () from
> >>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >>>      13 0x00007f42b4bef4c0 in ?? () from
> >>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >>>      14 0x00007f42b4bef7d2 in g_main_loop_run () from
> >>> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
> >>>      15 0x0000563b183b0397 in main (argc=<optimized out>,
> >>> argv=<optimized out>) at src/main.c:294
> >>>
> >>> ?
> >>
> >> Yes, much better.  So now you can explain to me how you're triggering this.
> >>
> >> The only way modem->atom_watches is NULL is if ofono_modem_register
> >> failed.  So are you trying to use a modem object without registering it
> >> properly?
> >
> > the problem is that I have moved location_information_create, along
> > with devinfo_create at the gemalto_enable() phase, because these two
> > are available also offline, and it is a normal use case to monitor the
> > GNSS signal while offline.
>
> Well you can't do that.  .enable() callback is to power the modem up or
> open up the AT command port and brings the modem to RX/TX circuits off
> stage.  So you cannot be adding any atoms until
> ofono_modem_set_powered(modem, TRUE) has been called.
>
> In fact you should not be calling any atom creation functions outside
> the pre_sim, post_sim or post_online callbacks.  There may be one or two
> exceptions due to modems taking a while to initialize the SMS/phonebook
> store, and we still need to fix that.
>
> However, it still doesn't explain to me why modem->atom_watches are NULL
> in the first place.  Are you calling ofono_modem_registered first?
>
> > But when the modem is removed (unplugged or switched off), the
> > location_reporting atom is not removed, and the modem does not
> > disappear. devinfo goes away without any issue.
> >
> > So in the gemalto_remove I have added:
> >     lratom = __ofono_modem_find_atom(modem,OFONO_ATOM_TYPE_LOCATION_REPORTING);
> >     if (lratom)
> >         __ofono_atom_free(lratom);
>
> This tells me you're doing something very wrong...
>
> >
> > this works fine, but maybe the watch is already destroyed?
> >
> > And the whole point is, I wouldn't have to do it manually. What is
> > missing in this atom?
>
> Nothing is missing, all atoms are managed automagically.
>
> Regards,
> -Denis

ok, I keep it in pre-sim.

Regards,
Giacinto

Re: [PATCH] src/modem: connection timeout to 60 seconds

[Giacinto Cifelli]

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

Hi Denis,

>
> In my 10+ years I've never seen a modem do this.  Some take 5 seconds,
> maybe 10.  I've never seen anything more than that, which is why that 20
> second value was quite conservative when I added it.

Yes, up to 2 years ago 15 seconds were enough. But with the new LTEA
and LTEApro chipset, it takes much longer.

>
> Anyhow, go ahead and add ofono_modem_set_powered_timeout_hint(unsigned
> int seconds) and have udevng detection fix this.

ok. I'll do it so.

>
> Regards,
> -Denis

Regards,
Giacinto