On Wed, Jul 8, 2020 at 4:14 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > On Wed, Jul 8, 2020 at 4:03 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote: > > If I'm reading correctly, you're removing the only remaining use of > > 'per_ce_irq'. Should we kill the field entirely? > > Ah, you are indeed correct! I hadn't noticed that. Unless I hear > otherwise, I'll send a v2 tomorrow that removes the field entirely. A healthy middle ground might put that in a patch 2, so it's easily dropped if desired. *shrug* > > Or perhaps we should > > leave some kind of WARN_ON() (BUG_ON()?) if this function is called > > erroneously with per_ce_irq==true? But I suppose this driver is full > > of landmines if the CE API is used incorrectly. > > Yeah, I originally had a WARN_ON() here and then took it out because > it seemed like extra overhead and, as you said, someone writing the > code has to know how the API works already I think. ...but I'll add > it back in if people want. I believe WARN_ON() and friends have a built-in unlikely(), so it shouldn't have much overhead. But I don't really mind either way. > > Do you need to clear this map if the interface goes down or if there's > > a firmware crash? Right now, I don't think there's a guarantee that > > we'll run through a NAPI poll in those cases, which is the only place > > you clear the map, and if the hardware/firmware has been reset, the > > state map is probably not valid. > > Seems like a good idea. Is the right place at the start of > ath10k_snoc_hif_start()? Either there or in .power_down()/.power_up(). I think either would be equally correct, but I'm not entirely sure if the semantic difference is meaningful for this. Brian
