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

Discussion:

Jonas Bonn

2018-10-23 08:34:30 UTC

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

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;
}

Jonas Bonn

2018-10-23 08:53:50 UTC

Permalink

Hi,

Post by Jonas Bonn
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

Post by Jonas Bonn
/Jonas

Giacinto

Post by Jonas Bonn

Jonas Bonn

2018-10-23 10:45:01 UTC

Permalink

Hi Jonas

Post by Jonas Bonn
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

Slava Monich

2018-10-23 10:35:41 UTC

Permalink

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

Denis Kenzior

2018-10-23 14:30:22 UTC

Permalink

Hi Giacinto,

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

Denis Kenzior

2018-10-23 14:47:19 UTC

Permalink

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

Denis Kenzior

2018-10-23 16:50:35 UTC

Permalink

Hi Giacinto,

0 0x0000563b1842b735 in call_watches
src/modem.c:265
1 0x0000563b1842c32e in __ofono_atom_unregister
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
src/modem.c:915
6 0x0000563b1842c863 in modem_unregister
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

Denis Kenzior

2018-10-23 17:40:05 UTC

Permalink

Hi Giacinto,

Hi Denis,
I didn't know this was a surprise interrogation!

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

Post by Denis Kenzior
Hi Giacinto,

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

Denis Kenzior

2018-10-23 15:08:29 UTC

Permalink

Hi Giacinto,

Hi,

Post by Jonas Bonn
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