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