On Tue, Oct 25, 2016 at 04:11:14PM +0000, Amitkumar Karwar wrote: > Hi Dmitry, > > > From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com] > > Sent: Tuesday, October 25, 2016 5:28 AM > > To: Brian Norris > > Cc: Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy Luo; Nishant > > Sarmukadam > > Subject: Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' > > in shutdown_drv > > > > On Mon, Oct 24, 2016 at 12:19:15PM -0700, Brian Norris wrote: > > > On Mon, Oct 24, 2016 at 07:51:29PM +0530, Amitkumar Karwar wrote: > > > > This variable is guarded by spinlock at all other places. This patch > > > > takes care of missing spinlock usage in mwifiex_shutdown_drv(). > > > > > > > > Signed-off-by: Amitkumar Karwar <akar...@marvell.com> > > > > --- > > > > drivers/net/wireless/marvell/mwifiex/init.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c > > > > b/drivers/net/wireless/marvell/mwifiex/init.c > > > > index 82839d9..8e5e424 100644 > > > > --- a/drivers/net/wireless/marvell/mwifiex/init.c > > > > +++ b/drivers/net/wireless/marvell/mwifiex/init.c > > > > @@ -670,11 +670,14 @@ mwifiex_shutdown_drv(struct mwifiex_adapter > > > > *adapter) > > > > > > > > adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING; > > > > /* wait for mwifiex_process to complete */ > > > > + spin_lock_irqsave(&adapter->main_proc_lock, flags); > > > > if (adapter->mwifiex_processing) { > > > > > > I'm not quite sure why we have this check in the first place; if the > > > main process is still running, then don't we have bigger problems here > > > anyway? But this is definitely the "right" way to check this flag, so: > > > > No, this is definitely not right way to check it. What exactly does this > > spinlock protect? It seems that the intent is to make sure we do not > > call mwifiex_shutdown_drv() while mwifiex_main_process() is executing. > > It even says above that we are "waiting" for it (but we do not, we may > > bail out or we may not, depends on luck). > > > > To implement this properly you not only need to take a lock and check > > the flag, but keep the lock until mwifiex_shutdown_drv() is done, or use > > some other way to let mwifiex_main_process() know it should not access > > the adapter while mwifiex_shutdown_drv() is running. > > > > NACK. > > > > As I explained in other email, here is the sequence. > 1) We find mwifiex_processing is true in mwifiex_shitdown_drv(). > "-EINPROGRESS" is returned by the function and hw_status state is changed to > MWIFIEX_HW_STATUS_CLOSING. > 2) We wait until main_process is completed. > > if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS) > wait_event_interruptible(adapter->init_wait_q, > adapter->init_wait_q_woken); > > 3) We have following check at the end of main_process() > > exit_main_proc: > if (adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) > mwifiex_shutdown_drv(adapter); > > 4) Here at the end of mwifiex_shutdown_drv(), we are setting > "adapter->init_wait_q_woken" and waking up the thread in point (2)
1. We do not find mwifiex_processing to be true 2. We proceed to try and shut down the driver 3. Interrupt is raised in the meantime, kicking work item 4. mwifiex_main_process() sees that adapter->hw_status is MWIFIEX_HW_STATUS_CLOSING and jumps to exit_main_proc 5. mwifiex_main_process() calls into mwifiex_shutdown_drv() that is now racing with another copy of the same. It seems to me that mwifiex_main_process() is [almost] always used from a workqueue. Can you instead of sprinkling spinlocks ensure that mwifiex_shutdown_drv(): 1. Inhibits scheduling of mwifiex_main_process() 2. Does cancel_work_sync(...) to ensure that mwifiex_main_process() does not run 3. Continues shutting down the driver. Alternatively, do these have to be spinlocks? Can you make them mutexes and take them for the duration of mwifiex_main_process() and mwifiex_shutdown_drv() and others, as needed? Thanks. -- Dmitry
