Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancel set_online request if modem is powered off. JB#61333 #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cancel set_online request if modem is powered off. JB#61333 #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf214d8
Simplify spec
pvuorela Mar 22, 2024
32d5319
[connman] Cancel set_online request if modem is powered off. JB#61333
pvuorela Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions connman/plugins/sailfish_ofono.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ struct modem_data {
gulong connctx_handler_id[CONNCTX_HANDLER_COUNT];
guint activate_timeout_id;
guint online_check_id;
GCancellable *pending_set_powered;
struct connman_device *device;
struct connman_network *network;
const char *country;
Expand Down Expand Up @@ -1060,6 +1061,25 @@ static void modem_online_check(struct modem_data *md)
modem_set_online(md, !__connman_technology_get_offlinemode());
}

static void set_powered_cb(OfonoModem* sender, const GError* error, void* arg)
{
struct modem_data *md = arg;
DBG("set powered callback");

md->pending_set_powered = NULL;

if (error) {
connman_warn("Error setting modem powered %s: %s",
ofono_modem_path(md->modem), error->message);

// these are doomed to fail until the modem is powered
if (md->online_check_id) {
g_source_remove(md->online_check_id);
md->online_check_id = 0;
}
}
}

static void modem_changed(OfonoModem *modem, void *arg)
{
struct modem_data *md = arg;
Expand All @@ -1068,7 +1088,12 @@ static void modem_changed(OfonoModem *modem, void *arg)
modem->powered, modem->online);
if (!modem->powered) {
DBG("%s powering up", ofono_modem_path(modem));
ofono_modem_set_powered(modem, TRUE);
if (md->pending_set_powered) {
g_cancellable_cancel(md->pending_set_powered);
}
md->pending_set_powered
= ofono_modem_set_powered_full(modem, TRUE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just wondering that the process kind of gets twisted here. Now we're waiting for a callback to finish but that callback would set the online check off. And regardless of the state we start the online check. For me it would be kind of making sense that we do nothing until we get the verification that the powering was successful. I.e., do the online checking later, not sure if that will break anything but this kind of breaks how it used to work - despite the fact that the success of the action was ignored and was kind of hoping it will succeed.

If the powering does not succeed the online check should not be started. I'd like to try this one out a bit with the online check initiated only after the callback gets a-ok from ofono. What do you think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we're waiting for a callback to finish

AFAICT there is no change in waiting. There is and was async power on, async online set with timer based online checking. The difference being that the latter part gets stopped after the fact if initial power on failed.

If the powering does not succeed the online check should not be started. I'd like to try this one out a bit with the online check initiated only after the callback gets a-ok from ofono. What do you think?

Sounds like it should be this way. But then the same probably would apply for the online set too i.e. wait for result of set instead of timer based poll/retry?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no, I guess.

On one hand this could be waiting for the result before proceeding with the online state. On the other hand the logic split into numerous callbacks for async operations, each one proceeding a bit forward, tends to be tedious to follow. If it's just set_powered + set_online, then on the happy path it shouldn't make a difference, and on the error case the failed set_online shouldn't really matter much either. So if the logic doesn't require splitting into steps, I'd rather not go there.

Also to note I didn't really make logic changes here on the common setup, merely just stopping the repeating failed calls when it's known to keep failing.

Whether the create_modem() case should postpone some setup parts to be done after calling both set_powered and set_online is another topic, getting deeper in the details of what's really going on. Frankly I'm not too excited on going there right now, but I'm happy to hand over that part if you feel like it :) I'm myself already content that connman doesn't keep doing silly things every 2 seconds.

Copy link
Contributor

@LaakkonenJussi LaakkonenJussi Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT there is no change in waiting. There is and was async power on, async online set with timer based online checking. The difference being that the latter part gets stopped after the fact if initial power on failed.

I think I picked the wrong word here. But we are waiting the async to finish to then later decide the fate of the online check.

Sounds like it should be this way. But then the same probably would apply for the online set too i.e. wait for result of set instead of timer based poll/retry?

Yeah, it would be the optimal situation to rely always on what ofono has done and reported back. We'd need timers only for doing this repeatedly as the modems may crash. EDIT: since I think it was the original idea of how this should work in error cases.

&set_powered_cb, md);
}

/* Keep modem online state in sync with the offline mode */
Expand Down Expand Up @@ -1177,7 +1202,9 @@ static void modem_create(struct plugin_data *plugin, OfonoModem *modem)
g_hash_table_replace(plugin->modems, g_strdup(path), md);

if (ofono_modem_valid(modem)) {
ofono_modem_set_powered(modem, TRUE);
md->pending_set_powered
= ofono_modem_set_powered_full(modem, TRUE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here we're doing a lot of unnecessary extra if the modem cannot be powered on. Online check, network create/disable, context updates, network registration. Not sure what the effect is if we have an half setup modem here with online check disabled as it cannot be powered on.

There is a comment in modem_can_create_network():

        /*
	 * Don't create the network if cellular technology is disabled,
	 * otherwise connman will keep on trying to connect it.
	 * /

This is kind of weird situation as connman should not even create a device for this case and that should block from ever trying the false modem in anyway to be powered up. Granted, this callback use is good addition to error cases but I think some blacklisting might be looked at as well...

&set_powered_cb, md);
modem_online_check(md);
}
if (ofono_connmgr_valid(md->connmgr)) {
Expand All @@ -1197,6 +1224,10 @@ static void modem_delete(gpointer value)
g_source_remove(md->online_check_id);
}

if (md->pending_set_powered) {
g_cancellable_cancel(md->pending_set_powered);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cancellation happens asynchronously, via separate mainloop dispatch. I suspect that that can lead to call to set_powered_cb(), which will be poking 'md' object already freed below. So, the callback might need to check that it is not dealing with G_IO_ERROR_CANCELLED error or stte before dereferencing md arg.

But it might just be me being confused with how all this cancellation stuff works in general / in context of libgofono, and this matches how other places in connman deal with similar cancellations -> I guess it is okay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in the core the g_cancellable_cancel is not used. Only in the plugins this is used so I guess it is ok to use here. But yeah, I was wondering that does this call the cb still for the cleanup? Apparently it can.

}

if (md->delayed_set_connected_id) {
g_source_remove(md->delayed_set_connected_id);
}
Expand Down
7 changes: 2 additions & 5 deletions rpm/connman.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ URL: http://connman.net/
Source0: %{name}-%{version}.tar.bz2
Source1: main.conf
Source2: 01-require-home-mount-to-be-present.conf
Requires: dbus >= 1.4
Requires: wpa_supplicant >= 0.7.1
Requires: dbus
Requires: wpa_supplicant
Requires: ofono
Requires: pacrunner
Requires: connman-configs
Expand All @@ -24,8 +24,6 @@ Requires: tayga >= 0.9.2
Requires(preun): systemd
Requires(post): systemd
Requires(postun): systemd
# license macro requires reasonably fresh rpm
BuildRequires: rpm >= 4.11
BuildRequires: pkgconfig(xtables) >= 1.6.1
BuildRequires: pkgconfig(libiptc)
BuildRequires: pkgconfig(glib-2.0) >= 2.62
Expand Down Expand Up @@ -95,7 +93,6 @@ FallbackTimeservers.
%package doc
Summary: Documentation for %{name}
Requires: %{name} = %{version}-%{release}
Obsoletes: %{name}-docs

%description doc
Man pages for %{name}.
Expand Down