RE: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Amitkumar Karwar]

Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Wednesday, October 26, 2016 10:06 PM
> To: Amitkumar Karwar
> Cc: Brian Norris; linux-wireless@vger.kernel.org; Cathy Luo; Nishant
> Sarmukadam
> Subject: Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing'
> in shutdown_drv
> 
> Hi Amit,
> 
> On Wed, Oct 26, 2016 at 03:23:08PM +, Amitkumar Karwar wrote:
> >
> > This race won't occur. At this point of time(i.e while calling
> mwifiex_shutdown_drv() in deinit), following things are completed. We
> don't expect mwifiex_main_process() to be scheduled.
> > 1) Connection to peer device is terminated at the beginning of
> teardown thread. So we don't receive any Tx data from kernel.
> > 2) Last command SHUTDOWN is exchanged with firmware. So there won't be
> any activity/interrupt from firmware.
> > 3) Interrupts are disabled.
> > 4) "adapter->surprise_removed" flag is set. It will skip
> mwifiex_main_process() calls.
> >
> > ---
> > static void mwifiex_main_work_queue(struct work_struct *work) {
> > struct mwifiex_adapter *adapter =
> > container_of(work, struct mwifiex_adapter, main_work);
> >
> > if (adapter->surprise_removed)
> > return;
> > mwifiex_main_process(adapter); }
> > --
> > 5) We have "mwifiex_terminate_workqueue(adapter)" call to flush and
> destroy workqueue.
> 
> OK, but if interrupts are disabled and you ensure that work is flushed
> or completed before you call mwifiex_shutdown_drv() then I do not
> understand why you need all of this at all? Why do you need to check
> status in mwifiex_shutdown_drv() and why do you want
> mwifiex_main_process() to call mwifiex_shutdown_drv() in certain cases?
> Can you simply remove all this stuff?
> 

I agree. This code is there for long time. I will prepare a patch for this 
cleanup work.

Regards,
Amitkumar

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

Hi Amit,

On Wed, Oct 26, 2016 at 03:23:08PM +, Amitkumar Karwar wrote:
> 
> This race won't occur. At this point of time(i.e while calling 
> mwifiex_shutdown_drv() in deinit), following things are completed. We don't 
> expect mwifiex_main_process() to be scheduled.
> 1) Connection to peer device is terminated at the beginning of teardown 
> thread. So we don't receive any Tx data from kernel.
> 2) Last command SHUTDOWN is exchanged with firmware. So there won't be any 
> activity/interrupt from firmware.
> 3) Interrupts are disabled.
> 4) "adapter->surprise_removed" flag is set. It will skip 
> mwifiex_main_process() calls.
> 
> ---
> static void mwifiex_main_work_queue(struct work_struct *work)
> {
> struct mwifiex_adapter *adapter =
> container_of(work, struct mwifiex_adapter, main_work);
> 
> if (adapter->surprise_removed)
> return;
> mwifiex_main_process(adapter);
> }
> --
> 5) We have "mwifiex_terminate_workqueue(adapter)" call to flush and destroy 
> workqueue.

OK, but if interrupts are disabled and you ensure that work is flushed
or completed before you call mwifiex_shutdown_drv() then I do not
understand why you need all of this at all? Why do you need to check
status in mwifiex_shutdown_drv() and why do you want
mwifiex_main_process() to call mwifiex_shutdown_drv() in certain cases?
Can you simply remove all this stuff?

Thanks.

-- 
Dmitry

RE: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Amitkumar Karwar]

Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Tuesday, October 25, 2016 10:05 PM
> To: Amitkumar Karwar
> Cc: Brian Norris; linux-wireless@vger.kernel.org; Cathy Luo; Nishant
> Sarmukadam
> Subject: Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing'
> in shutdown_drv
> 
> On Tue, Oct 25, 2016 at 04:11:14PM +, 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(>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.

This race won't occur. At this point of time(i.e while calling 
mwifiex_shutdown_drv() in deinit), following things are completed. We do

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

On Tue, Oct 25, 2016 at 04:11:14PM +, 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(>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

RE: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Amitkumar Karwar]

Hi Brian/Dmitry,

> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: Tuesday, October 25, 2016 5:26 AM
> To: Dmitry Torokhov
> Cc: Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy Luo; Nishant
> Sarmukadam; briannor...@google.com
> Subject: Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing'
> in shutdown_drv
> 
> Hi,
> 
> On Mon, Oct 24, 2016 at 04:47:20PM -0700, Dmitry Torokhov 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(>main_proc_lock, flags);
> > >   if (adapter->mwifiex_processing) {
> > > + spin_unlock_irqrestore(>main_proc_lock, flags);
> > >   mwifiex_dbg(adapter, WARN,
> > >   "main process is still running\n");
> > >   return ret;
> > >   }
> > > + spin_unlock_irqrestore(>main_proc_lock, flags);
> >
> > What happens if adapter->mwifiex_processing will become true here?
> 
> That's why I commented:
> 
> "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?"

If mwifiex_processing is true here, it means main_process is still running. We 
have set "adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING" in 
mwifiex_shutdown_drv().
Now we will wait until main_process() gets completed.

-
if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
 wait_event_interruptible(adapter->init_wait_q,
  adapter->init_wait_q_woken);
--

We have following logic at the end of main_process()
---
exit_main_proc:
if (adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING)
mwifiex_shutdown_drv(adapter);


Regards,
Amitkumar Karwar

RE: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Amitkumar Karwar]

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(>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)

Regards,
Amitkumar

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

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 
> > ---
> >  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(>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.

Thanks.

> 
> Reviewed-by: Brian Norris 
> 
> > +   spin_unlock_irqrestore(>main_proc_lock, flags);
> > mwifiex_dbg(adapter, WARN,
> > "main process is still running\n");
> > return ret;
> > }
> > +   spin_unlock_irqrestore(>main_proc_lock, flags);
> >  
> > /* cancel current command */
> > if (adapter->curr_cmd) {
> > -- 
> > 1.9.1
> > 

-- 
Dmitry

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Brian Norris]

Hi,

On Mon, Oct 24, 2016 at 04:47:20PM -0700, Dmitry Torokhov 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 
> > ---
> >  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(>main_proc_lock, flags);
> > if (adapter->mwifiex_processing) {
> > +   spin_unlock_irqrestore(>main_proc_lock, flags);
> > mwifiex_dbg(adapter, WARN,
> > "main process is still running\n");
> > return ret;
> > }
> > +   spin_unlock_irqrestore(>main_proc_lock, flags);
> 
> What happens if adapter->mwifiex_processing will become true here?

That's why I commented:

"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?"

I think the answer is, we really better NOT see that become true. That
means that we've fired off more interrupts and began processing them.
But all callers should have disabled interrupts and stopped or flushed
the queue at this point, AFAICT.

So I expect the intention here is to be essentially an assert(), without
actually making it fatal. Maybe there's a better way to handle this? I
can't really think of a good one right now.

Brian

> >  
> > /* cancel current command */
> > if (adapter->curr_cmd) {
> > -- 
> > 1.9.1
> > 
> 
> -- 
> Dmitry

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Dmitry Torokhov]

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 
> ---
>  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(>main_proc_lock, flags);
>   if (adapter->mwifiex_processing) {
> + spin_unlock_irqrestore(>main_proc_lock, flags);
>   mwifiex_dbg(adapter, WARN,
>   "main process is still running\n");
>   return ret;
>   }
> + spin_unlock_irqrestore(>main_proc_lock, flags);

What happens if adapter->mwifiex_processing will become true here?

>  
>   /* cancel current command */
>   if (adapter->curr_cmd) {
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Brian Norris]

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 
> ---
>  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(>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:

Reviewed-by: Brian Norris 

> + spin_unlock_irqrestore(>main_proc_lock, flags);
>   mwifiex_dbg(adapter, WARN,
>   "main process is still running\n");
>   return ret;
>   }
> + spin_unlock_irqrestore(>main_proc_lock, flags);
>  
>   /* cancel current command */
>   if (adapter->curr_cmd) {
> -- 
> 1.9.1
> 

[PATCH 2/5] mwifiex: use spinlock for 'mwifiex_processing' in shutdown_drv

[Amitkumar Karwar]

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 
---
 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(>main_proc_lock, flags);
if (adapter->mwifiex_processing) {
+   spin_unlock_irqrestore(>main_proc_lock, flags);
mwifiex_dbg(adapter, WARN,
"main process is still running\n");
return ret;
}
+   spin_unlock_irqrestore(>main_proc_lock, flags);
 
/* cancel current command */
if (adapter->curr_cmd) {
-- 
1.9.1