diff mbox series

Revert "scan: Don't callback on SCAN_ABORTED"

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

Checks

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

Commit Message

Alvin Šipraga Dec. 12, 2022, 4:42 p.m. UTC
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(-)

Comments

Denis Kenzior Dec. 12, 2022, 7:41 p.m. UTC | #1
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
Alvin Šipraga Dec. 12, 2022, 8:18 p.m. UTC | #2
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
Denis Kenzior Dec. 12, 2022, 8:49 p.m. UTC | #3
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 mbox series

Patch

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)) {
 			/*