On Tue, 16 Mar 2021 18:27:10 +0100 Stefan Assmann wrote: > On 16.03.21 18:14, Jakub Kicinski wrote: > > On Tue, 16 Mar 2021 11:01:41 +0100 Stefan Assmann wrote: > >> To avoid races between iavf_init_task(), iavf_reset_task(), > >> iavf_watchdog_task(), iavf_adminq_task() as well as the shutdown and > >> remove functions more locking is required. > >> The current protection by __IAVF_IN_CRITICAL_TASK is needed in > >> additional places. > >> > >> - The reset task performs state transitions, therefore needs locking. > >> - The adminq task acts on replies from the PF in > >> iavf_virtchnl_completion() which may alter the states. > >> - The init task is not only run during probe but also if a VF gets stuck > >> to reinitialize it. > >> - The shutdown function performs a state transition. > >> - The remove function perorms a state transition and also free's > >> resources. > >> > >> iavf_lock_timeout() is introduced to avoid waiting infinitely > >> and cause a deadlock. Rather unlock and print a warning. > >> > >> Signed-off-by: Stefan Assmann <sassm...@kpanic.de> > > > > I personally think that the overuse of flags in Intel drivers brings > > nothing but trouble. At which point does it make sense to just add a > > lock / semaphore here rather than open code all this with no clear > > semantics? No code seems to just test the __IAVF_IN_CRITICAL_TASK flag, > > all the uses look like poor man's locking at a quick grep. What am I > > missing? > > I agree with you that the locking could be done with other locking > mechanisms just as good. I didn't invent the current method so I'll let > Intel comment on that part, but I'd like to point out that what I'm > making use of is fixing what is currently in the driver.
Right, I should have made it clear that I don't blame you for the current state of things. Would you mind sending a patch on top of this one to do a conversion to a semaphore? Intel folks any opinions?
