| Message ID | 20221212164250.3075444-1-alvin@pqrs.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "scan: Don't callback on SCAN_ABORTED" | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-alpine-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-gitlint | success | GitLint |
| prestwoj/iwd-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-incremental_build | success | Incremental build not run PASS |
| prestwoj/iwd-alpine-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-alpine-ci-incremental_build | success | Incremental build not run PASS |
| prestwoj/iwd-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-alpine-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-clang | success | clang PASS |
| prestwoj/iwd-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-ci-testrunner | success | test-runner PASS |
Hi Alvin, On 12/12/22 10:42, Alvin Šipraga wrote: > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > This reverts commit 6051a1495227bbe7ba357f6995d4dbe4a4862331. > > The blamed commit argues that the periodic scan callback doesn't do > anything useful in the event of an aborted scan, but this is not > entirely true. In particular, the callback is responsible for resetting > the station scanning state to false. Failure to do so means that the Hmm... The callback simply flips the station->scanning variable on and off. I'd probably prefer just calling scan_finished instead of a full-on revert. But... > station scanning state remains true indefinitely if a periodic scan is > aborted, and no further periodic scans can be performed by iwd. I don't see how the station->scanning being always 'true' would prevent us starting a new scan? station->scanning isn't used anywhere besides reporting over D-Bus? Maybe I'm missing something? > --- > src/scan.c | 18 ++---------------- > 1 file changed, 2 insertions(+), 16 deletions(-) > Regards, -Denis
Hi Denis, On Mon, Dec 12, 2022 at 01:41:19PM -0600, Denis Kenzior wrote: > Hi Alvin, > > On 12/12/22 10:42, Alvin Šipraga wrote: > > From: Alvin Šipraga <alsi@bang-olufsen.dk> > > > > This reverts commit 6051a1495227bbe7ba357f6995d4dbe4a4862331. > > > > The blamed commit argues that the periodic scan callback doesn't do > > anything useful in the event of an aborted scan, but this is not > > entirely true. In particular, the callback is responsible for resetting > > the station scanning state to false. Failure to do so means that the > > Hmm... The callback simply flips the station->scanning variable on and off. > I'd probably prefer just calling scan_finished instead of a full-on revert. I can send v2 with a more explicit title than just revert? See below and tell me your preference. > But... > > > station scanning state remains true indefinitely if a periodic scan is > > aborted, and no further periodic scans can be performed by iwd. > > I don't see how the station->scanning being always 'true' would prevent us > starting a new scan? station->scanning isn't used anywhere besides > reporting over D-Bus? Maybe I'm missing something? No, you are quite right. It was a misunderstanding on my part due to nested callbacks (scan callback, and periodic scan callback). The reason periodic scans get stuck is because the scan callback, scan_periodic_notify(), doesn't get called. So it doesn't rearm the periodic scan timer. But outwardly - that is, with 'iwctl station wlan0 show' - one can see that something is wrong, and that's because the periodic scan callback, new_scan_results(), doesn't reset the station scanning state to false. As you point out though, this is just cosmetic. Kind regards, Alvin
Hi Alvin, > I can send v2 with a more explicit title than just revert? See below and > tell me your preference. > Reverting is fine, or just a new commit on top that calls scan_finished is also fine. Probably the latter is (slightly) more preferable. >> But... >> >>> station scanning state remains true indefinitely if a periodic scan is >>> aborted, and no further periodic scans can be performed by iwd. >> >> I don't see how the station->scanning being always 'true' would prevent us >> starting a new scan? station->scanning isn't used anywhere besides >> reporting over D-Bus? Maybe I'm missing something? > > No, you are quite right. It was a misunderstanding on my part due to > nested callbacks (scan callback, and periodic scan callback). > > The reason periodic scans get stuck is because the scan callback, > scan_periodic_notify(), doesn't get called. So it doesn't rearm the > periodic scan timer. > Gotcha. That makes sense now. > But outwardly - that is, with 'iwctl station wlan0 show' - one can see > that something is wrong, and that's because the periodic scan callback, > new_scan_results(), doesn't reset the station scanning state to > false. As you point out though, this is just cosmetic. > Yep, we should fix that for sure. Regards, -Denis
diff --git a/src/scan.c b/src/scan.c index 5d2f2957748a..dbc46a754d16 100644 --- a/src/scan.c +++ b/src/scan.c @@ -83,7 +83,6 @@ struct scan_request { bool canceled : 1; /* Is scan_cancel being called on this request? */ bool passive:1; /* Active or Passive scan? */ bool started : 1; /* Has TRIGGER_SCAN succeeded at least once? */ - bool periodic : 1; /* Started as a periodic scan? */ /* * Set to true if the TRIGGER_SCAN command at the head of the 'cmds' * queue was acked by the kernel indicating that the scan request was @@ -997,7 +996,6 @@ static void scan_periodic_destroy(void *user_data) static bool scan_periodic_queue(struct scan_context *sc) { struct scan_parameters params = {}; - struct scan_request *sr; if (sc->sp.needs_active_scan && known_networks_has_hidden()) { params.randomize_mac_addr_hint = true; @@ -1015,13 +1013,7 @@ static bool scan_periodic_queue(struct scan_context *sc) scan_periodic_notify, sc, scan_periodic_destroy); - if (!sc->sp.id) - return false; - - sr = l_queue_peek_tail(sc->requests); - sr->periodic = true; - - return true; + return sc->sp.id != 0; } static bool scan_periodic_is_disabled(void) @@ -2242,13 +2234,7 @@ static void scan_notify(struct l_genl_msg *msg, void *user_data) if (sr->triggered) { sr->triggered = false; - - /* If periodic scan, don't report the abort */ - if (sr->periodic) { - l_queue_remove(sc->requests, sr); - wiphy_radio_work_done(sc->wiphy, sr->work.id); - } else - scan_finished(sc, -ECANCELED, NULL, NULL, sr); + scan_finished(sc, -ECANCELED, NULL, NULL, sr); } else if (wiphy_radio_work_is_running(sc->wiphy, sr->work.id)) { /*
From: Alvin Šipraga <alsi@bang-olufsen.dk> This reverts commit 6051a1495227bbe7ba357f6995d4dbe4a4862331. The blamed commit argues that the periodic scan callback doesn't do anything useful in the event of an aborted scan, but this is not entirely true. In particular, the callback is responsible for resetting the station scanning state to false. Failure to do so means that the station scanning state remains true indefinitely if a periodic scan is aborted, and no further periodic scans can be performed by iwd. --- src/scan.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)