On Mon, 2018-09-10 at 13:02 +0200, Toke Høiland-Jørgensen wrote: > > > 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. > > 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. > > > + 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
