On Mon, 2018-09-10 at 13:02 +0200, Toke Høiland-Jørgensen wrote:

quoted

Is there a case where it's allowed to call this while *not* empty? At
least for the drivers it seems not, so perhaps when called from the
driver it should WARN() here or so?

Yeah, mac80211 calls this on wake_txq, where it will often be scheduled
already. And since that can happen while drivers schedule stuff, it
could also be the case that a driver re-schedules a queue that is
already scheduled.

Right, I kinda gathered that but was thinking more from a driver POV as
I was reviewing, makes sense.

quoted

I'm just a bit worried that drivers will get this a bit wrong, and
we'll never know, but things won't work properly in some weird way.

What, subtle bugs in the data path that causes hangs in hard-to-debug
cases? Nah, that never happens ;)

Good :-P

Actually though, we can't put a warn on there, because the driver might
take a txq, then mac80211 puts it on the list due to a new packet, and
then the driver also returns it.

quoted

quoted

+	txqi = list_first_entry(&local->active_txqs[ac],
+				struct txq_info,
+				schedule_order);
+
+	if (txqi->schedule_round == local->schedule_round[ac])
+		txqi = NULL;

that assigment seems unnecessary? txqi defaults to NULL.

No, this is an 'undo' of the previous line. I.e., we get the first txqi,
check if we've already seen it this round, and if we have, unset txqi so
the function returns NULL (causing the driver to stop looping).

Err, yeah, obviously.

johannes