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

[Xinming Hu]

> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: 2016年11月4日 2:49
> To: Brian Norris
> Cc: Xinming Hu; Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy Luo;
> Nishant Sarmukadam; raja...@google.com
> Subject: Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in
> shutdown_drv
> 
> On Thu, Nov 3, 2016 at 12:27 PM, Brian Norris <briannor...@chromium.org>
> wrote:
> > On Thu, Nov 03, 2016 at 09:15:04AM -0700, Dmitry Torokhov wrote:
> >> On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote:
> >> > > -Original Message-
> >> > > From: linux-wireless-ow...@vger.kernel.org
> >> > > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry
> >> > > Torokhov
> >> > >
> >> > > Instead please remove call to mwifiex_shutdown_drv() in the main
> >> > > routine and "if (adapter->mwifiex_processing)" check here.
> >> > >
> >> >
> >> > mwifiex_main_process will be used from interrupt or workqueue.
> >> > Now we have disabled interrupt and flush workqueue, so
> >> > mwifiex_main_process won't be scheduled in the future.
> >> > But mwifiex_main_process might just running in context of last
> >> > interrupt, so we need wait current main_process complete in
> >> > mwifiex_shutdown_drv.
> >>
> >> synchronize_irq() is your friend then.
> >
> > Hmm, that sounds right, but IIUC, the "interrupt context" is actually
> > only used for SDIO, and for SDIO, the driver doesn't actually have
> > access to the IRQ number. The MMC/SDIO layer has some extra
> > abstraction around the IRQ. So this may be more difficult than it appears.
> >
> > Do we need a sdio_synchronize_irq() API?
> 
> Actually the ->disable_irq() method should be responsible for making sure it
> does not complete while interrupt handler is running. As far as I can see, for
> SDIO case, we end up calling sdio_card_irq_put() which stops kernel thread
> and won't return while the thread is running. For other interfaces we need to
> check. IIRC USB lacks
> ->disable_irq() altogether and this is something that shoudl be fixed
> (by doing usb_kill_urb() at the minimum).
> 

Thanks for the explain, will keep this cleanup work.

> Thanks.
> 
> --
> Dmitry

Regards,
Xinming

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

[Dmitry Torokhov]

On Thu, Nov 3, 2016 at 12:27 PM, Brian Norris  wrote:
> On Thu, Nov 03, 2016 at 09:15:04AM -0700, Dmitry Torokhov wrote:
>> On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote:
>> > > -Original Message-
>> > > From: linux-wireless-ow...@vger.kernel.org
>> > > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry 
>> > > Torokhov
>> > >
>> > > Instead please remove call to mwifiex_shutdown_drv() in the main routine
>> > > and "if (adapter->mwifiex_processing)" check here.
>> > >
>> >
>> > mwifiex_main_process will be used from interrupt or workqueue.
>> > Now we have disabled interrupt and flush workqueue, so
>> > mwifiex_main_process won't be scheduled in the future.
>> > But mwifiex_main_process might just running in context of last
>> > interrupt, so we need wait current main_process complete in
>> > mwifiex_shutdown_drv.
>>
>> synchronize_irq() is your friend then.
>
> Hmm, that sounds right, but IIUC, the "interrupt context" is actually
> only used for SDIO, and for SDIO, the driver doesn't actually have
> access to the IRQ number. The MMC/SDIO layer has some extra abstraction
> around the IRQ. So this may be more difficult than it appears.
>
> Do we need a sdio_synchronize_irq() API?

Actually the ->disable_irq() method should be responsible for making
sure it does not complete while interrupt handler is running. As far
as I can see, for SDIO case, we end up calling sdio_card_irq_put()
which stops kernel thread and won't return while the thread is
running. For other interfaces we need to check. IIRC USB lacks
->disable_irq() altogether and this is something that shoudl be fixed
(by doing usb_kill_urb() at the minimum).

Thanks.

-- 
Dmitry

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

[Brian Norris]

On Thu, Nov 03, 2016 at 09:15:04AM -0700, Dmitry Torokhov wrote:
> On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote:
> > > -Original Message-
> > > From: linux-wireless-ow...@vger.kernel.org
> > > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov
> > > 
> > > Instead please remove call to mwifiex_shutdown_drv() in the main routine
> > > and "if (adapter->mwifiex_processing)" check here.
> > > 
> > 
> > mwifiex_main_process will be used from interrupt or workqueue.
> > Now we have disabled interrupt and flush workqueue, so
> > mwifiex_main_process won't be scheduled in the future.
> > But mwifiex_main_process might just running in context of last
> > interrupt, so we need wait current main_process complete in
> > mwifiex_shutdown_drv.
> 
> synchronize_irq() is your friend then.

Hmm, that sounds right, but IIUC, the "interrupt context" is actually
only used for SDIO, and for SDIO, the driver doesn't actually have
access to the IRQ number. The MMC/SDIO layer has some extra abstraction
around the IRQ. So this may be more difficult than it appears.

Do we need a sdio_synchronize_irq() API?

Brian

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

[Dmitry Torokhov]

On Thu, Nov 03, 2016 at 08:34:06AM +, Xinming Hu wrote:
> Hi Dmitry,
> 
> > -Original Message-
> > From: linux-wireless-ow...@vger.kernel.org
> > [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov
> > Sent: 2016年10月28日 1:44
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > raja...@google.com; briannor...@google.com
> > Subject: Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' 
> > in
> > shutdown_drv
> > 
> > Hi Amit,
> > 
> > On Thu, Oct 27, 2016 at 02:42:40PM +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().
> > 
> > Since in the previous discussion you stated that we inhibit interrupts and 
> > flush
> > the workqueue so that mwifiex_shutdown_drv() can't run simultaneously with
> > the main processing routine, why do we need this?
> > 
> > Instead please remove call to mwifiex_shutdown_drv() in the main routine
> > and "if (adapter->mwifiex_processing)" check here.
> > 
> 
> mwifiex_main_process will be used from interrupt or workqueue.
> Now we have disabled interrupt and flush workqueue, so mwifiex_main_process 
> won't be scheduled in the future.
> But mwifiex_main_process might just running in context of last interrupt, so 
> we need wait current main_process complete in mwifiex_shutdown_drv.

synchronize_irq() is your friend then.

Thanks.

-- 
Dmitry

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

[Xinming Hu]

Hi Dmitry,

> -Original Message-
> From: linux-wireless-ow...@vger.kernel.org
> [mailto:linux-wireless-ow...@vger.kernel.org] On Behalf Of Dmitry Torokhov
> Sent: 2016年10月28日 1:44
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> raja...@google.com; briannor...@google.com
> Subject: Re: [PATCH v2 2/5] mwifiex: use spinlock for 'mwifiex_processing' in
> shutdown_drv
> 
> Hi Amit,
> 
> On Thu, Oct 27, 2016 at 02:42:40PM +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().
> 
> Since in the previous discussion you stated that we inhibit interrupts and 
> flush
> the workqueue so that mwifiex_shutdown_drv() can't run simultaneously with
> the main processing routine, why do we need this?
> 
> Instead please remove call to mwifiex_shutdown_drv() in the main routine
> and "if (adapter->mwifiex_processing)" check here.
> 

mwifiex_main_process will be used from interrupt or workqueue.
Now we have disabled interrupt and flush workqueue, so mwifiex_main_process 
won't be scheduled in the future.
But mwifiex_main_process might just running in context of last interrupt, so we 
need wait current main_process complete in mwifiex_shutdown_drv.


> Thanks.
> 
> >
> > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > ---
> > v2: Same as v1
> > ---
> >  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
> >
> 
> --
> Dmitry

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

[Dmitry Torokhov]

Hi Amit,

On Thu, Oct 27, 2016 at 02:42:40PM +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().

Since in the previous discussion you stated that we inhibit interrupts
and flush the workqueue so that mwifiex_shutdown_drv() can't run
simultaneously with the main processing routine, why do we need this?

Instead please remove call to mwifiex_shutdown_drv() in the main routine
and "if (adapter->mwifiex_processing)" check here.

Thanks.

> 
> Signed-off-by: Amitkumar Karwar 
> ---
> v2: Same as v1
> ---
>  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
> 

-- 
Dmitry

[PATCH v2 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 
---
v2: Same as v1
---
 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