On Mon, Mar 16, 2020 at 11:42:00AM -0700, Keith Busch wrote: > On Mon, Mar 16, 2020 at 07:10:58PM +0100, Lukas Wunner wrote: > > On Mon, Mar 16, 2020 at 09:15:43AM -0700, Keith Busch wrote: > > > I'm not sure why the hard-irq context is even setting the thread running > > > flag while it can still exit without handling anything. Shouldn't it > > > leave the flag cleared until knows it's actually going to do something? > > > > No, ist_running must be set to true before the invocation of > > atomic_xchg(&ctrl->pending_events, 0). > > Okay, I see what you mean. > > Even with David's patch, there's still another condition that could exit > with ist_running set. It may make sense to move the setting just above > the atomic_xchg() so that clearing it doesn't need to be duplicated for > the remaining uncommon exit case. Right, or introduce a goto label before the teardown code at the end of pciehp_ist() and replace the return statements with gotos. I've compared both approaches and the goto solution required the least lines of code and subjectively looked the nicest, so I went with that. > > There's a time window between the atomic_xchg() and actually > > turning off the slot when pending_events is 0. Previously we > > only checked in the sysfs functions that pending_events is 0. > > That was insufficient as we risked returning prematurely from > > the sysfs functions. The point of ist_running is to prevent > > that. > > Oh, right, I'm remembering now. And since we've a polled option for > pciehp, using synchronize_irq() from the sysfs path isn't possible. Precisely. :-) Thanks, Lukas
