[PATCH 1/5] mwifiex: remove redundant condition in main process

[PATCH 1/5] mwifiex: remove redundant condition in main process

[Amitkumar Karwar]

This condition while calling mwifiex_check_ps_cond() is redundant.
The function internally already takes care of it.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 2478ccd..3b31ea2 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -355,10 +355,8 @@ process_start:
 
 		/* Check if we need to confirm Sleep Request
 		   received previously */
-		if (adapter->ps_state == PS_STATE_PRE_SLEEP) {
-			if (!adapter->cmd_sent && !adapter->curr_cmd)
-				mwifiex_check_ps_cond(adapter);
-		}
+		if (adapter->ps_state == PS_STATE_PRE_SLEEP)
+			mwifiex_check_ps_cond(adapter);
 
 		/* * The ps_state may have been changed during processing of
 		 * Sleep Request event.
-- 
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 <akarwar@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) {
+		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);
 
 	/* cancel current command */
 	if (adapter->curr_cmd) {
-- 
1.9.1

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 <akarwar@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:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> +		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);
>  
>  	/* cancel current command */
>  	if (adapter->curr_cmd) {
> -- 
> 1.9.1
> 

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 <akarwar@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.

Thanks.

> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 
> > +		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);
> >  
> >  	/* cancel current command */
> >  	if (adapter->curr_cmd) {
> > -- 
> > 1.9.1
> > 

-- 
Dmitry

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

[Amitkumar Karwar]

Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torokhov@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 <akarwar@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)

Regards,
Amitkumar

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

[Dmitry Torokhov]

On Tue, Oct 25, 2016 at 04:11:14PM +0000, Amitkumar Karwar wrote:
> Hi Dmitry,
> 
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@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 <akarwar@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

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

[Amitkumar Karwar]

Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torokhov@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 +0000, Amitkumar Karwar wrote:
> > Hi Dmitry,
> >
> > > From: Dmitry Torokhov [mailto:dmitry.torokhov@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 <akarwar@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.

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.

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

I think, this is already taken care of in current implementation.

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

As I explained above, there won't be a race between mwifiex_main_process() and mwifiex_shutdown_drv().
Let me know if you think otherwise and have any suggestions.

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 +0000, 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.torokhov@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 +0000, 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]

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@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) {
> +		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?

>  
>  	/* 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 <akarwar@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) {
> > +		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

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

[Amitkumar Karwar]

Hi Brian/Dmitry,

> From: Brian Norris [mailto:briannorris@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; briannorris@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 <akarwar@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) {
> > > +		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?"

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

[PATCH 3/5] mwifiex: do not free firmware dump memory in shutdown_drv

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

mwifiex_upload_device_dump() already takes care of freeing firmware dump
memory. Doing the same thing in mwifiex_shutdown_drv() is redundant.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/init.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 8e5e424..365efb8 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -407,8 +407,6 @@ static void mwifiex_free_lock_list(struct mwifiex_adapter *adapter)
 static void
 mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
 {
-	int idx;
-
 	if (!adapter) {
 		pr_err("%s: adapter is NULL\n", __func__);
 		return;
@@ -426,23 +424,6 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
 	mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
 	mwifiex_free_cmd_buffer(adapter);
 
-	for (idx = 0; idx < adapter->num_mem_types; idx++) {
-		struct memory_type_mapping *entry =
-				&adapter->mem_type_mapping_tbl[idx];
-
-		if (entry->mem_ptr) {
-			vfree(entry->mem_ptr);
-			entry->mem_ptr = NULL;
-		}
-		entry->mem_size = 0;
-	}
-
-	if (adapter->drv_info_dump) {
-		vfree(adapter->drv_info_dump);
-		adapter->drv_info_dump = NULL;
-		adapter->drv_info_size = 0;
-	}
-
 	if (adapter->sleep_cfm)
 		dev_kfree_skb_any(adapter->sleep_cfm);
 }
-- 
1.9.1

Re: [PATCH 3/5] mwifiex: do not free firmware dump memory in shutdown_drv

[Brian Norris]

On Mon, Oct 24, 2016 at 07:51:30PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> mwifiex_upload_device_dump() already takes care of freeing firmware dump
> memory. Doing the same thing in mwifiex_shutdown_drv() is redundant.
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>

Looks good:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> ---
>  drivers/net/wireless/marvell/mwifiex/init.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
> index 8e5e424..365efb8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -407,8 +407,6 @@ static void mwifiex_free_lock_list(struct mwifiex_adapter *adapter)
>  static void
>  mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
>  {
> -	int idx;
> -
>  	if (!adapter) {
>  		pr_err("%s: adapter is NULL\n", __func__);
>  		return;
> @@ -426,23 +424,6 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
>  	mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
>  	mwifiex_free_cmd_buffer(adapter);
>  
> -	for (idx = 0; idx < adapter->num_mem_types; idx++) {
> -		struct memory_type_mapping *entry =
> -				&adapter->mem_type_mapping_tbl[idx];
> -
> -		if (entry->mem_ptr) {
> -			vfree(entry->mem_ptr);
> -			entry->mem_ptr = NULL;
> -		}
> -		entry->mem_size = 0;
> -	}
> -
> -	if (adapter->drv_info_dump) {
> -		vfree(adapter->drv_info_dump);
> -		adapter->drv_info_dump = NULL;
> -		adapter->drv_info_size = 0;
> -	}
> -
>  	if (adapter->sleep_cfm)
>  		dev_kfree_skb_any(adapter->sleep_cfm);
>  }
> -- 
> 1.9.1
> 

Re: [PATCH 3/5] mwifiex: do not free firmware dump memory in shutdown_drv

[Dmitry Torokhov]

On Mon, Oct 24, 2016 at 07:51:30PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> mwifiex_upload_device_dump() already takes care of freeing firmware dump
> memory. Doing the same thing in mwifiex_shutdown_drv() is redundant.
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/init.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
> index 8e5e424..365efb8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -407,8 +407,6 @@ static void mwifiex_free_lock_list(struct mwifiex_adapter *adapter)
>  static void
>  mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
>  {
> -	int idx;
> -
>  	if (!adapter) {
>  		pr_err("%s: adapter is NULL\n", __func__);
>  		return;
> @@ -426,23 +424,6 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
>  	mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
>  	mwifiex_free_cmd_buffer(adapter);
>  
> -	for (idx = 0; idx < adapter->num_mem_types; idx++) {
> -		struct memory_type_mapping *entry =
> -				&adapter->mem_type_mapping_tbl[idx];
> -
> -		if (entry->mem_ptr) {
> -			vfree(entry->mem_ptr);
> -			entry->mem_ptr = NULL;
> -		}
> -		entry->mem_size = 0;
> -	}
> -
> -	if (adapter->drv_info_dump) {
> -		vfree(adapter->drv_info_dump);
> -		adapter->drv_info_dump = NULL;
> -		adapter->drv_info_size = 0;
> -	}

Why do you even keep the pointer to dump memory in the adapter
structure? You allocate it in mwifiex_drv_info_dump() and immediately
use it in mwifiex_upload_device_dump(). Why not simply pass the pointer
between the functions?

Thanks.

-- 
Dmitry

RE: [PATCH 3/5] mwifiex: do not free firmware dump memory in shutdown_drv

[Amitkumar Karwar]

Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Tuesday, October 25, 2016 5:47 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> briannorris@google.com; Xinming Hu
> Subject: Re: [PATCH 3/5] mwifiex: do not free firmware dump memory in
> shutdown_drv
> 
> On Mon, Oct 24, 2016 at 07:51:30PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu <huxm@marvell.com>
> >
> > mwifiex_upload_device_dump() already takes care of freeing firmware
> > dump memory. Doing the same thing in mwifiex_shutdown_drv() is
> redundant.
> >
> > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/init.c | 19 -------------------
> >  1 file changed, 19 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> > b/drivers/net/wireless/marvell/mwifiex/init.c
> > index 8e5e424..365efb8 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/init.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> > @@ -407,8 +407,6 @@ static void mwifiex_free_lock_list(struct
> > mwifiex_adapter *adapter)  static void  mwifiex_adapter_cleanup(struct
> > mwifiex_adapter *adapter)  {
> > -	int idx;
> > -
> >  	if (!adapter) {
> >  		pr_err("%s: adapter is NULL\n", __func__);
> >  		return;
> > @@ -426,23 +424,6 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter
> *adapter)
> >  	mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
> >  	mwifiex_free_cmd_buffer(adapter);
> >
> > -	for (idx = 0; idx < adapter->num_mem_types; idx++) {
> > -		struct memory_type_mapping *entry =
> > -				&adapter->mem_type_mapping_tbl[idx];
> > -
> > -		if (entry->mem_ptr) {
> > -			vfree(entry->mem_ptr);
> > -			entry->mem_ptr = NULL;
> > -		}
> > -		entry->mem_size = 0;
> > -	}
> > -
> > -	if (adapter->drv_info_dump) {
> > -		vfree(adapter->drv_info_dump);
> > -		adapter->drv_info_dump = NULL;
> > -		adapter->drv_info_size = 0;
> > -	}
> 
> Why do you even keep the pointer to dump memory in the adapter
> structure? You allocate it in mwifiex_drv_info_dump() and immediately
> use it in mwifiex_upload_device_dump(). Why not simply pass the pointer
> between the functions?
> 

Thanks. This makes sense. I will pass the pointer and get rid of adapter variable in updated version.

Regards,
Amitkumar

[PATCH 4/5] mwifiex: firmware dump code rearrangement in pcie.c

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

The next patch will refer to pcie_work in mwifiex_pcie_remove function.
This patch puts pcie_work related functions ahead to mwifiex_pcie_remove
to avoid static forward declaration.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 590 ++++++++++++++--------------
 1 file changed, 294 insertions(+), 296 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 9147e6a..986bf07 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -65,6 +65,42 @@ static void mwifiex_unmap_pci_memory(struct mwifiex_adapter *adapter,
 }
 
 /*
+ * This function writes data into PCIE card register.
+ */
+static int mwifiex_write_reg(struct mwifiex_adapter *adapter, int reg, u32 data)
+{
+	struct pcie_service_card *card = adapter->card;
+
+	iowrite32(data, card->pci_mmap1 + reg);
+
+	return 0;
+}
+
+/*  This function reads data from PCIE card register.
+ */
+static int mwifiex_read_reg(struct mwifiex_adapter *adapter, int reg, u32 *data)
+{
+	struct pcie_service_card *card = adapter->card;
+
+	*data = ioread32(card->pci_mmap1 + reg);
+	if (*data == 0xffffffff)
+		return 0xffffffff;
+
+	return 0;
+}
+
+/* This function reads u8 data from PCIE card register. */
+static int mwifiex_read_reg_byte(struct mwifiex_adapter *adapter,
+				 int reg, u8 *data)
+{
+	struct pcie_service_card *card = adapter->card;
+
+	*data = ioread8(card->pci_mmap1 + reg);
+
+	return 0;
+}
+
+/*
  * This function reads sleep cookie and checks if FW is ready
  */
 static bool mwifiex_pcie_ok_to_access_hw(struct mwifiex_adapter *adapter)
@@ -88,6 +124,249 @@ static bool mwifiex_pcie_ok_to_access_hw(struct mwifiex_adapter *adapter)
 	return false;
 }
 
+/* Function to dump PCIE scratch registers in case of FW crash
+ */
+static int
+mwifiex_pcie_reg_dump(struct mwifiex_adapter *adapter, char *drv_buf)
+{
+	char *p = drv_buf;
+	char buf[256], *ptr;
+	int i;
+	u32 value;
+	struct pcie_service_card *card = adapter->card;
+	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
+	int pcie_scratch_reg[] = {PCIE_SCRATCH_12_REG,
+				  PCIE_SCRATCH_13_REG,
+				  PCIE_SCRATCH_14_REG};
+
+	if (!p)
+		return 0;
+
+	mwifiex_dbg(adapter, MSG, "PCIE register dump start\n");
+
+	if (mwifiex_read_reg(adapter, reg->fw_status, &value)) {
+		mwifiex_dbg(adapter, ERROR, "failed to read firmware status");
+		return 0;
+	}
+
+	ptr = buf;
+	mwifiex_dbg(adapter, MSG, "pcie scratch register:");
+	for (i = 0; i < ARRAY_SIZE(pcie_scratch_reg); i++) {
+		mwifiex_read_reg(adapter, pcie_scratch_reg[i], &value);
+		ptr += sprintf(ptr, "reg:0x%x, value=0x%x\n",
+			       pcie_scratch_reg[i], value);
+	}
+
+	mwifiex_dbg(adapter, MSG, "%s\n", buf);
+	p += sprintf(p, "%s\n", buf);
+
+	mwifiex_dbg(adapter, MSG, "PCIE register dump end\n");
+
+	return p - drv_buf;
+}
+
+/* This function read/write firmware */
+static enum rdwr_status
+mwifiex_pcie_rdwr_firmware(struct mwifiex_adapter *adapter, u8 doneflag)
+{
+	int ret, tries;
+	u8 ctrl_data;
+	u32 fw_status;
+	struct pcie_service_card *card = adapter->card;
+	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
+
+	if (mwifiex_read_reg(adapter, reg->fw_status, &fw_status))
+		return RDWR_STATUS_FAILURE;
+
+	ret = mwifiex_write_reg(adapter, reg->fw_dump_ctrl,
+				reg->fw_dump_host_ready);
+	if (ret) {
+		mwifiex_dbg(adapter, ERROR,
+			    "PCIE write err\n");
+		return RDWR_STATUS_FAILURE;
+	}
+
+	for (tries = 0; tries < MAX_POLL_TRIES; tries++) {
+		mwifiex_read_reg_byte(adapter, reg->fw_dump_ctrl, &ctrl_data);
+		if (ctrl_data == FW_DUMP_DONE)
+			return RDWR_STATUS_SUCCESS;
+		if (doneflag && ctrl_data == doneflag)
+			return RDWR_STATUS_DONE;
+		if (ctrl_data != reg->fw_dump_host_ready) {
+			mwifiex_dbg(adapter, WARN,
+				    "The ctrl reg was changed, re-try again!\n");
+			ret = mwifiex_write_reg(adapter, reg->fw_dump_ctrl,
+						reg->fw_dump_host_ready);
+			if (ret) {
+				mwifiex_dbg(adapter, ERROR,
+					    "PCIE write err\n");
+				return RDWR_STATUS_FAILURE;
+			}
+		}
+		usleep_range(100, 200);
+	}
+
+	mwifiex_dbg(adapter, ERROR, "Fail to pull ctrl_data\n");
+	return RDWR_STATUS_FAILURE;
+}
+
+/* This function dump firmware memory to file */
+static void mwifiex_pcie_fw_dump(struct mwifiex_adapter *adapter)
+{
+	struct pcie_service_card *card = adapter->card;
+	const struct mwifiex_pcie_card_reg *creg = card->pcie.reg;
+	unsigned int reg, reg_start, reg_end;
+	u8 *dbg_ptr, *end_ptr, *tmp_ptr, fw_dump_num, dump_num;
+	u8 idx, i, read_reg, doneflag = 0;
+	enum rdwr_status stat;
+	u32 memory_size;
+	int ret;
+
+	if (!card->pcie.can_dump_fw)
+		return;
+
+	for (idx = 0; idx < adapter->num_mem_types; idx++) {
+		struct memory_type_mapping *entry =
+				&adapter->mem_type_mapping_tbl[idx];
+
+		if (entry->mem_ptr) {
+			vfree(entry->mem_ptr);
+			entry->mem_ptr = NULL;
+		}
+		entry->mem_size = 0;
+	}
+
+	mwifiex_dbg(adapter, MSG, "== mwifiex firmware dump start ==\n");
+
+	/* Read the number of the memories which will dump */
+	stat = mwifiex_pcie_rdwr_firmware(adapter, doneflag);
+	if (stat == RDWR_STATUS_FAILURE)
+		return;
+
+	reg = creg->fw_dump_start;
+	mwifiex_read_reg_byte(adapter, reg, &fw_dump_num);
+
+	/* W8997 chipset firmware dump will be restore in single region*/
+	if (fw_dump_num == 0)
+		dump_num = 1;
+	else
+		dump_num = fw_dump_num;
+
+	/* Read the length of every memory which will dump */
+	for (idx = 0; idx < dump_num; idx++) {
+		struct memory_type_mapping *entry =
+				&adapter->mem_type_mapping_tbl[idx];
+		memory_size = 0;
+		if (fw_dump_num != 0) {
+			stat = mwifiex_pcie_rdwr_firmware(adapter, doneflag);
+			if (stat == RDWR_STATUS_FAILURE)
+				return;
+
+			reg = creg->fw_dump_start;
+			for (i = 0; i < 4; i++) {
+				mwifiex_read_reg_byte(adapter, reg, &read_reg);
+				memory_size |= (read_reg << (i * 8));
+				reg++;
+			}
+		} else {
+			memory_size = MWIFIEX_FW_DUMP_MAX_MEMSIZE;
+		}
+
+		if (memory_size == 0) {
+			mwifiex_dbg(adapter, MSG, "Firmware dump Finished!\n");
+			ret = mwifiex_write_reg(adapter, creg->fw_dump_ctrl,
+						creg->fw_dump_read_done);
+			if (ret) {
+				mwifiex_dbg(adapter, ERROR, "PCIE write err\n");
+				return;
+			}
+			break;
+		}
+
+		mwifiex_dbg(adapter, DUMP,
+			    "%s_SIZE=0x%x\n", entry->mem_name, memory_size);
+		entry->mem_ptr = vmalloc(memory_size + 1);
+		entry->mem_size = memory_size;
+		if (!entry->mem_ptr)
+			return;
+
+		dbg_ptr = entry->mem_ptr;
+		end_ptr = dbg_ptr + memory_size;
+
+		doneflag = entry->done_flag;
+		mwifiex_dbg(adapter, DUMP, "Start %s output, please wait...\n",
+			    entry->mem_name);
+
+		do {
+			stat = mwifiex_pcie_rdwr_firmware(adapter, doneflag);
+			if (stat == RDWR_STATUS_FAILURE)
+				return;
+
+			reg_start = creg->fw_dump_start;
+			reg_end = creg->fw_dump_end;
+			for (reg = reg_start; reg <= reg_end; reg++) {
+				mwifiex_read_reg_byte(adapter, reg, dbg_ptr);
+				if (dbg_ptr < end_ptr) {
+					dbg_ptr++;
+					continue;
+				}
+				mwifiex_dbg(adapter, ERROR,
+					    "pre-allocated buf not enough\n");
+				tmp_ptr =
+					vzalloc(memory_size + MWIFIEX_SIZE_4K);
+				if (!tmp_ptr)
+					return;
+				memcpy(tmp_ptr, entry->mem_ptr, memory_size);
+				vfree(entry->mem_ptr);
+				entry->mem_ptr = tmp_ptr;
+				tmp_ptr = NULL;
+				dbg_ptr = entry->mem_ptr + memory_size;
+				memory_size += MWIFIEX_SIZE_4K;
+				end_ptr = entry->mem_ptr + memory_size;
+			}
+
+			if (stat != RDWR_STATUS_DONE)
+				continue;
+
+			mwifiex_dbg(adapter, DUMP,
+				    "%s done: size=0x%tx\n",
+				    entry->mem_name, dbg_ptr - entry->mem_ptr);
+			break;
+		} while (true);
+	}
+	mwifiex_dbg(adapter, MSG, "== mwifiex firmware dump end ==\n");
+}
+
+static void mwifiex_pcie_device_dump_work(struct mwifiex_adapter *adapter)
+{
+	mwifiex_drv_info_dump(adapter);
+	mwifiex_pcie_fw_dump(adapter);
+	mwifiex_upload_device_dump(adapter);
+}
+
+static unsigned long iface_work_flags;
+static struct mwifiex_adapter *save_adapter;
+static void mwifiex_pcie_work(struct work_struct *work)
+{
+	if (test_and_clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP,
+			       &iface_work_flags))
+		mwifiex_pcie_device_dump_work(save_adapter);
+}
+
+static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
+
+/* This function dumps FW information */
+static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
+{
+	save_adapter = adapter;
+	if (test_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags))
+		return;
+
+	set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags);
+
+	schedule_work(&pcie_work);
+}
+
 #ifdef CONFIG_PM_SLEEP
 /*
  * Kernel needs to suspend all functions separately. Therefore all
@@ -353,58 +632,21 @@ static SIMPLE_DEV_PM_OPS(mwifiex_pcie_pm_ops, mwifiex_pcie_suspend,
 				mwifiex_pcie_resume);
 #endif
 
-/* PCI Device Driver */
-static struct pci_driver __refdata mwifiex_pcie = {
-	.name     = "mwifiex_pcie",
-	.id_table = mwifiex_ids,
-	.probe    = mwifiex_pcie_probe,
-	.remove   = mwifiex_pcie_remove,
-#ifdef CONFIG_PM_SLEEP
-	.driver   = {
-		.pm = &mwifiex_pcie_pm_ops,
-	},
-#endif
-	.shutdown = mwifiex_pcie_shutdown,
-	.err_handler = mwifiex_pcie_err_handler,
-};
-
-/*
- * This function writes data into PCIE card register.
- */
-static int mwifiex_write_reg(struct mwifiex_adapter *adapter, int reg, u32 data)
-{
-	struct pcie_service_card *card = adapter->card;
-
-	iowrite32(data, card->pci_mmap1 + reg);
-
-	return 0;
-}
-
-/*
- * This function reads data from PCIE card register.
- */
-static int mwifiex_read_reg(struct mwifiex_adapter *adapter, int reg, u32 *data)
-{
-	struct pcie_service_card *card = adapter->card;
-
-	*data = ioread32(card->pci_mmap1 + reg);
-	if (*data == 0xffffffff)
-		return 0xffffffff;
-
-	return 0;
-}
-
-/* This function reads u8 data from PCIE card register. */
-static int mwifiex_read_reg_byte(struct mwifiex_adapter *adapter,
-				 int reg, u8 *data)
-{
-	struct pcie_service_card *card = adapter->card;
-
-	*data = ioread8(card->pci_mmap1 + reg);
-
-	return 0;
-}
-
+/* PCI Device Driver */
+static struct pci_driver __refdata mwifiex_pcie = {
+	.name     = "mwifiex_pcie",
+	.id_table = mwifiex_ids,
+	.probe    = mwifiex_pcie_probe,
+	.remove   = mwifiex_pcie_remove,
+#ifdef CONFIG_PM_SLEEP
+	.driver   = {
+		.pm = &mwifiex_pcie_pm_ops,
+	},
+#endif
+	.shutdown = mwifiex_pcie_shutdown,
+	.err_handler = mwifiex_pcie_err_handler,
+};
+
 /*
  * This function adds delay loop to ensure FW is awake before proceeding.
  */
@@ -2482,250 +2724,6 @@ static int mwifiex_pcie_host_to_card(struct mwifiex_adapter *adapter, u8 type,
 	return 0;
 }
 
-/* Function to dump PCIE scratch registers in case of FW crash
- */
-static int
-mwifiex_pcie_reg_dump(struct mwifiex_adapter *adapter, char *drv_buf)
-{
-	char *p = drv_buf;
-	char buf[256], *ptr;
-	int i;
-	u32 value;
-	struct pcie_service_card *card = adapter->card;
-	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
-	int pcie_scratch_reg[] = {PCIE_SCRATCH_12_REG,
-				  PCIE_SCRATCH_13_REG,
-				  PCIE_SCRATCH_14_REG};
-
-	if (!p)
-		return 0;
-
-	mwifiex_dbg(adapter, MSG, "PCIE register dump start\n");
-
-	if (mwifiex_read_reg(adapter, reg->fw_status, &value)) {
-		mwifiex_dbg(adapter, ERROR, "failed to read firmware status");
-		return 0;
-	}
-
-	ptr = buf;
-	mwifiex_dbg(adapter, MSG, "pcie scratch register:");
-	for (i = 0; i < ARRAY_SIZE(pcie_scratch_reg); i++) {
-		mwifiex_read_reg(adapter, pcie_scratch_reg[i], &value);
-		ptr += sprintf(ptr, "reg:0x%x, value=0x%x\n",
-			       pcie_scratch_reg[i], value);
-	}
-
-	mwifiex_dbg(adapter, MSG, "%s\n", buf);
-	p += sprintf(p, "%s\n", buf);
-
-	mwifiex_dbg(adapter, MSG, "PCIE register dump end\n");
-
-	return p - drv_buf;
-}
-
-/* This function read/write firmware */
-static enum rdwr_status
-mwifiex_pcie_rdwr_firmware(struct mwifiex_adapter *adapter, u8 doneflag)
-{
-	int ret, tries;
-	u8 ctrl_data;
-	u32 fw_status;
-	struct pcie_service_card *card = adapter->card;
-	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
-
-	if (mwifiex_read_reg(adapter, reg->fw_status, &fw_status))
-		return RDWR_STATUS_FAILURE;
-
-	ret = mwifiex_write_reg(adapter, reg->fw_dump_ctrl,
-				reg->fw_dump_host_ready);
-	if (ret) {
-		mwifiex_dbg(adapter, ERROR,
-			    "PCIE write err\n");
-		return RDWR_STATUS_FAILURE;
-	}
-
-	for (tries = 0; tries < MAX_POLL_TRIES; tries++) {
-		mwifiex_read_reg_byte(adapter, reg->fw_dump_ctrl, &ctrl_data);
-		if (ctrl_data == FW_DUMP_DONE)
-			return RDWR_STATUS_SUCCESS;
-		if (doneflag && ctrl_data == doneflag)
-			return RDWR_STATUS_DONE;
-		if (ctrl_data != reg->fw_dump_host_ready) {
-			mwifiex_dbg(adapter, WARN,
-				    "The ctrl reg was changed, re-try again!\n");
-			ret = mwifiex_write_reg(adapter, reg->fw_dump_ctrl,
-						reg->fw_dump_host_ready);
-			if (ret) {
-				mwifiex_dbg(adapter, ERROR,
-					    "PCIE write err\n");
-				return RDWR_STATUS_FAILURE;
-			}
-		}
-		usleep_range(100, 200);
-	}
-
-	mwifiex_dbg(adapter, ERROR, "Fail to pull ctrl_data\n");
-	return RDWR_STATUS_FAILURE;
-}
-
-/* This function dump firmware memory to file */
-static void mwifiex_pcie_fw_dump(struct mwifiex_adapter *adapter)
-{
-	struct pcie_service_card *card = adapter->card;
-	const struct mwifiex_pcie_card_reg *creg = card->pcie.reg;
-	unsigned int reg, reg_start, reg_end;
-	u8 *dbg_ptr, *end_ptr, *tmp_ptr, fw_dump_num, dump_num;
-	u8 idx, i, read_reg, doneflag = 0;
-	enum rdwr_status stat;
-	u32 memory_size;
-	int ret;
-
-	if (!card->pcie.can_dump_fw)
-		return;
-
-	for (idx = 0; idx < adapter->num_mem_types; idx++) {
-		struct memory_type_mapping *entry =
-				&adapter->mem_type_mapping_tbl[idx];
-
-		if (entry->mem_ptr) {
-			vfree(entry->mem_ptr);
-			entry->mem_ptr = NULL;
-		}
-		entry->mem_size = 0;
-	}
-
-	mwifiex_dbg(adapter, MSG, "== mwifiex firmware dump start ==\n");
-
-	/* Read the number of the memories which will dump */
-	stat = mwifiex_pcie_rdwr_firmware(adapter, doneflag);
-	if (stat == RDWR_STATUS_FAILURE)
-		return;
-
-	reg = creg->fw_dump_start;
-	mwifiex_read_reg_byte(adapter, reg, &fw_dump_num);
-
-	/* W8997 chipset firmware dump will be restore in single region*/
-	if (fw_dump_num == 0)
-		dump_num = 1;
-	else
-		dump_num = fw_dump_num;
-
-	/* Read the length of every memory which will dump */
-	for (idx = 0; idx < dump_num; idx++) {
-		struct memory_type_mapping *entry =
-				&adapter->mem_type_mapping_tbl[idx];
-		memory_size = 0;
-		if (fw_dump_num != 0) {
-			stat = mwifiex_pcie_rdwr_firmware(adapter, doneflag);
-			if (stat == RDWR_STATUS_FAILURE)
-				return;
-
-			reg = creg->fw_dump_start;
-			for (i = 0; i < 4; i++) {
-				mwifiex_read_reg_byte(adapter, reg, &read_reg);
-				memory_size |= (read_reg << (i * 8));
-				reg++;
-			}
-		} else {
-			memory_size = MWIFIEX_FW_DUMP_MAX_MEMSIZE;
-		}
-
-		if (memory_size == 0) {
-			mwifiex_dbg(adapter, MSG, "Firmware dump Finished!\n");
-			ret = mwifiex_write_reg(adapter, creg->fw_dump_ctrl,
-						creg->fw_dump_read_done);
-			if (ret) {
-				mwifiex_dbg(adapter, ERROR, "PCIE write err\n");
-				return;
-			}
-			break;
-		}
-
-		mwifiex_dbg(adapter, DUMP,
-			    "%s_SIZE=0x%x\n", entry->mem_name, memory_size);
-		entry->mem_ptr = vmalloc(memory_size + 1);
-		entry->mem_size = memory_size;
-		if (!entry->mem_ptr) {
-			mwifiex_dbg(adapter, ERROR,
-				    "Vmalloc %s failed\n", entry->mem_name);
-			return;
-		}
-		dbg_ptr = entry->mem_ptr;
-		end_ptr = dbg_ptr + memory_size;
-
-		doneflag = entry->done_flag;
-		mwifiex_dbg(adapter, DUMP, "Start %s output, please wait...\n",
-			    entry->mem_name);
-
-		do {
-			stat = mwifiex_pcie_rdwr_firmware(adapter, doneflag);
-			if (RDWR_STATUS_FAILURE == stat)
-				return;
-
-			reg_start = creg->fw_dump_start;
-			reg_end = creg->fw_dump_end;
-			for (reg = reg_start; reg <= reg_end; reg++) {
-				mwifiex_read_reg_byte(adapter, reg, dbg_ptr);
-				if (dbg_ptr < end_ptr) {
-					dbg_ptr++;
-					continue;
-				}
-				mwifiex_dbg(adapter, ERROR,
-					    "pre-allocated buf not enough\n");
-				tmp_ptr =
-					vzalloc(memory_size + MWIFIEX_SIZE_4K);
-				if (!tmp_ptr)
-					return;
-				memcpy(tmp_ptr, entry->mem_ptr, memory_size);
-				vfree(entry->mem_ptr);
-				entry->mem_ptr = tmp_ptr;
-				tmp_ptr = NULL;
-				dbg_ptr = entry->mem_ptr + memory_size;
-				memory_size += MWIFIEX_SIZE_4K;
-				end_ptr = entry->mem_ptr + memory_size;
-			}
-
-			if (stat != RDWR_STATUS_DONE)
-				continue;
-
-			mwifiex_dbg(adapter, DUMP,
-				    "%s done: size=0x%tx\n",
-				    entry->mem_name, dbg_ptr - entry->mem_ptr);
-			break;
-		} while (true);
-	}
-	mwifiex_dbg(adapter, MSG, "== mwifiex firmware dump end ==\n");
-}
-
-static void mwifiex_pcie_device_dump_work(struct mwifiex_adapter *adapter)
-{
-	mwifiex_drv_info_dump(adapter);
-	mwifiex_pcie_fw_dump(adapter);
-	mwifiex_upload_device_dump(adapter);
-}
-
-static unsigned long iface_work_flags;
-static struct mwifiex_adapter *save_adapter;
-static void mwifiex_pcie_work(struct work_struct *work)
-{
-	if (test_and_clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP,
-			       &iface_work_flags))
-		mwifiex_pcie_device_dump_work(save_adapter);
-}
-
-static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
-/* This function dumps FW information */
-static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
-{
-	save_adapter = adapter;
-	if (test_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags))
-		return;
-
-	set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags);
-
-	schedule_work(&pcie_work);
-}
-
 /*
  * This function initializes the PCI-E host memory space, WCB rings, etc.
  *
-- 
1.9.1

[PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

This patch ensures to wait for firmware dump completion in
mwifiex_remove_card().

For sdio interface, reset_trigger variable is used to identify
if mwifiex_sdio_remove() is called by sdio_work during reset or
the call is from sdio subsystem.

This patch fixes a kernel crash observed during reboot when firmware
dump operation is in process.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c |  2 ++
 drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 986bf07..4512e86 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 	if (!adapter || !adapter->priv_num)
 		return;
 
+	cancel_work_sync(&pcie_work);
+
 	if (user_rmmod && !adapter->mfg_mode) {
 #ifdef CONFIG_PM_SLEEP
 		if (adapter->is_suspended)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 4cad1c2..f974260 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -46,6 +46,15 @@
  */
 static u8 user_rmmod;
 
+/* reset_trigger variable is used to identify if mwifiex_sdio_remove()
+ * is called by sdio_work during reset or the call is from sdio subsystem.
+ * We will cancel sdio_work only if the call is from sdio subsystem.
+ */
+static u8 reset_triggered;
+
+static void mwifiex_sdio_work(struct work_struct *work);
+static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
+
 static struct mwifiex_if_ops sdio_ops;
 static unsigned long iface_work_flags;
 
@@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func)
 	if (!adapter || !adapter->priv_num)
 		return;
 
+	if (!reset_triggered)
+		cancel_work_sync(&sdio_work);
+
 	mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
 
 	if (user_rmmod && !adapter->mfg_mode) {
@@ -2290,7 +2302,9 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
 	 * discovered and initializes them from scratch.
 	 */
 
+	reset_triggered = 1;
 	mwifiex_sdio_remove(func);
+	reset_triggered = 0;
 
 	/* power cycle the adapter */
 	sdio_claim_host(func);
@@ -2621,7 +2635,6 @@ static void mwifiex_sdio_work(struct work_struct *work)
 		mwifiex_sdio_card_reset_work(save_adapter);
 }
 
-static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
 /* This function resets the card */
 static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
 {
-- 
1.9.1

Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Brian Norris]

On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> This patch ensures to wait for firmware dump completion in
> mwifiex_remove_card().
> 
> For sdio interface, reset_trigger variable is used to identify
> if mwifiex_sdio_remove() is called by sdio_work during reset or
> the call is from sdio subsystem.
> 
> This patch fixes a kernel crash observed during reboot when firmware
> dump operation is in process.
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 ++
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 986bf07..4512e86 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
>  	if (!adapter || !adapter->priv_num)
>  		return;
>  
> +	cancel_work_sync(&pcie_work);

Is there a good reason you have to cancel the work? What if you were
just to finish it (i.e., flush_work())?

In any case, I think this is fine, so for the PCIe bits:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> +
>  	if (user_rmmod && !adapter->mfg_mode) {
>  #ifdef CONFIG_PM_SLEEP
>  		if (adapter->is_suspended)
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 4cad1c2..f974260 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -46,6 +46,15 @@
>   */
>  static u8 user_rmmod;
>  
> +/* reset_trigger variable is used to identify if mwifiex_sdio_remove()
> + * is called by sdio_work during reset or the call is from sdio subsystem.
> + * We will cancel sdio_work only if the call is from sdio subsystem.
> + */
> +static u8 reset_triggered;
> +
> +static void mwifiex_sdio_work(struct work_struct *work);
> +static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> +
>  static struct mwifiex_if_ops sdio_ops;
>  static unsigned long iface_work_flags;
>  
> @@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func)
>  	if (!adapter || !adapter->priv_num)
>  		return;
>  
> +	if (!reset_triggered)
> +		cancel_work_sync(&sdio_work);
> +
>  	mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
>  
>  	if (user_rmmod && !adapter->mfg_mode) {
> @@ -2290,7 +2302,9 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
>  	 * discovered and initializes them from scratch.
>  	 */
>  
> +	reset_triggered = 1;
>  	mwifiex_sdio_remove(func);
> +	reset_triggered = 0;

Boy that's ugly! Couldn't you just create something like
__mwifiex_sdio_remove(), which does everything except the
cancel_work_sync()? Then you'd do this for the .remove() callback:

	cancel_work_sync(&sdio_work);
	__mwifiex_sdio_remove(func);

and for mwifiex_recreate_adapter() you'd just call
__mwifiex_sdio_remove()? The static variable that simply serves as a
(non-reentrant) function call parameter seems like a really poor
alternative to this simple refactoring.

Or you could just address the TODO in this function, and you wouldn't
have to do this dance at all...

Brian

>  
>  	/* power cycle the adapter */
>  	sdio_claim_host(func);
> @@ -2621,7 +2635,6 @@ static void mwifiex_sdio_work(struct work_struct *work)
>  		mwifiex_sdio_card_reset_work(save_adapter);
>  }
>  
> -static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
>  /* This function resets the card */
>  static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
>  {
> -- 
> 1.9.1
> 

RE: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Amitkumar Karwar]

Hi Brian,

> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Tuesday, October 25, 2016 1:54 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> dmitry.torokhov@gmail.com; Xinming Hu
> Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> remove_card
> 
> On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu <huxm@marvell.com>
> >
> > This patch ensures to wait for firmware dump completion in
> > mwifiex_remove_card().
> >
> > For sdio interface, reset_trigger variable is used to identify if
> > mwifiex_sdio_remove() is called by sdio_work during reset or the call
> > is from sdio subsystem.
> >
> > This patch fixes a kernel crash observed during reboot when firmware
> > dump operation is in process.
> >
> > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 ++
> > drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++++-
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index 986bf07..4512e86 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev
> *pdev)
> >  	if (!adapter || !adapter->priv_num)
> >  		return;
> >
> > +	cancel_work_sync(&pcie_work);
> 
> Is there a good reason you have to cancel the work? What if you were
> just to finish it (i.e., flush_work())?

I assume if work is running, cancel_work_sync() will wait until completion. Otherwise it will cancel queued work. Firmware dump takes 20-30 seconds to complete. I think, it's ok to cancel it if work is just queued and not running yet. Reboot taking significant time due to our wait in remove handler may not be a good experience from end user point of view.

If you prefer flush_work(), I can use the same.

> 
> In any case, I think this is fine, so for the PCIe bits:
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 
> > +
> >  	if (user_rmmod && !adapter->mfg_mode) {  #ifdef CONFIG_PM_SLEEP
> >  		if (adapter->is_suspended)
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 4cad1c2..f974260 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -46,6 +46,15 @@
> >   */
> >  static u8 user_rmmod;
> >
> > +/* reset_trigger variable is used to identify if
> > +mwifiex_sdio_remove()
> > + * is called by sdio_work during reset or the call is from sdio
> subsystem.
> > + * We will cancel sdio_work only if the call is from sdio subsystem.
> > + */
> > +static u8 reset_triggered;
> > +
> > +static void mwifiex_sdio_work(struct work_struct *work); static
> > +DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> > +
> >  static struct mwifiex_if_ops sdio_ops;  static unsigned long
> > iface_work_flags;
> >
> > @@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func)
> >  	if (!adapter || !adapter->priv_num)
> >  		return;
> >
> > +	if (!reset_triggered)
> > +		cancel_work_sync(&sdio_work);
> > +
> >  	mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
> >
> >  	if (user_rmmod && !adapter->mfg_mode) { @@ -2290,7 +2302,9 @@
> static
> > void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
> >  	 * discovered and initializes them from scratch.
> >  	 */
> >
> > +	reset_triggered = 1;
> >  	mwifiex_sdio_remove(func);
> > +	reset_triggered = 0;
> 
> Boy that's ugly! Couldn't you just create something like
> __mwifiex_sdio_remove(), which does everything except the
> cancel_work_sync()? Then you'd do this for the .remove() callback:
> 
> 	cancel_work_sync(&sdio_work);
> 	__mwifiex_sdio_remove(func);
> 
> and for mwifiex_recreate_adapter() you'd just call
> __mwifiex_sdio_remove()? The static variable that simply serves as a
> (non-reentrant) function call parameter seems like a really poor
> alternative to this simple refactoring.

Thanks a lot for the suggestion.
I will use this approach in updated version.

Regards,
Amitkumar

Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Dmitry Torokhov]

On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> This patch ensures to wait for firmware dump completion in
> mwifiex_remove_card().
> 
> For sdio interface, reset_trigger variable is used to identify
> if mwifiex_sdio_remove() is called by sdio_work during reset or
> the call is from sdio subsystem.
> 
> This patch fixes a kernel crash observed during reboot when firmware
> dump operation is in process.
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 ++
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 986bf07..4512e86 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
>  	if (!adapter || !adapter->priv_num)
>  		return;
>  
> +	cancel_work_sync(&pcie_work);
> +
>  	if (user_rmmod && !adapter->mfg_mode) {
>  #ifdef CONFIG_PM_SLEEP
>  		if (adapter->is_suspended)
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 4cad1c2..f974260 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -46,6 +46,15 @@
>   */
>  static u8 user_rmmod;
>  
> +/* reset_trigger variable is used to identify if mwifiex_sdio_remove()
> + * is called by sdio_work during reset or the call is from sdio subsystem.
> + * We will cancel sdio_work only if the call is from sdio subsystem.
> + */
> +static u8 reset_triggered;

It would be really great if the driver supported multiple devices. IOW
please avoid module-globals.

> +
> +static void mwifiex_sdio_work(struct work_struct *work);
> +static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> +
>  static struct mwifiex_if_ops sdio_ops;
>  static unsigned long iface_work_flags;
>  
> @@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func)
>  	if (!adapter || !adapter->priv_num)
>  		return;
>  
> +	if (!reset_triggered)
> +		cancel_work_sync(&sdio_work);
> +
>  	mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
>  
>  	if (user_rmmod && !adapter->mfg_mode) {
> @@ -2290,7 +2302,9 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
>  	 * discovered and initializes them from scratch.
>  	 */
>  
> +	reset_triggered = 1;
>  	mwifiex_sdio_remove(func);
> +	reset_triggered = 0;

What exactly happens if I trigger mwifiex_sdio_remove() from sysfs at
the same time this code is running?

>  
>  	/* power cycle the adapter */
>  	sdio_claim_host(func);
> @@ -2621,7 +2635,6 @@ static void mwifiex_sdio_work(struct work_struct *work)
>  		mwifiex_sdio_card_reset_work(save_adapter);
>  }
>  
> -static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
>  /* This function resets the card */
>  static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
>  {
> -- 
> 1.9.1
> 

-- 
Dmitry

RE: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Amitkumar Karwar]

Hi Dmitry,

> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Tuesday, October 25, 2016 5:44 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> briannorris@google.com; Xinming Hu
> Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> remove_card
> 
> On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu <huxm@marvell.com>
> >
> > This patch ensures to wait for firmware dump completion in
> > mwifiex_remove_card().
> >
> > For sdio interface, reset_trigger variable is used to identify if
> > mwifiex_sdio_remove() is called by sdio_work during reset or the call
> > is from sdio subsystem.
> >
> > This patch fixes a kernel crash observed during reboot when firmware
> > dump operation is in process.
> >
> > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c |  2 ++
> > drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++++-
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index 986bf07..4512e86 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev
> *pdev)
> >  	if (!adapter || !adapter->priv_num)
> >  		return;
> >
> > +	cancel_work_sync(&pcie_work);
> > +
> >  	if (user_rmmod && !adapter->mfg_mode) {  #ifdef CONFIG_PM_SLEEP
> >  		if (adapter->is_suspended)
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 4cad1c2..f974260 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -46,6 +46,15 @@
> >   */
> >  static u8 user_rmmod;
> >
> > +/* reset_trigger variable is used to identify if
> > +mwifiex_sdio_remove()
> > + * is called by sdio_work during reset or the call is from sdio
> subsystem.
> > + * We will cancel sdio_work only if the call is from sdio subsystem.
> > + */
> > +static u8 reset_triggered;
> 
> It would be really great if the driver supported multiple devices. IOW
> please avoid module-globals.

You are right. As Brian pointed out in other email, I will refactor the code and get rid of global variable.

> 
> > +
> > +static void mwifiex_sdio_work(struct work_struct *work); static
> > +DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> > +
> >  static struct mwifiex_if_ops sdio_ops;  static unsigned long
> > iface_work_flags;
> >
> > @@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func)
> >  	if (!adapter || !adapter->priv_num)
> >  		return;
> >
> > +	if (!reset_triggered)
> > +		cancel_work_sync(&sdio_work);
> > +
> >  	mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
> >
> >  	if (user_rmmod && !adapter->mfg_mode) { @@ -2290,7 +2302,9 @@
> static
> > void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
> >  	 * discovered and initializes them from scratch.
> >  	 */
> >
> > +	reset_triggered = 1;
> >  	mwifiex_sdio_remove(func);
> > +	reset_triggered = 0;
> 
> What exactly happens if I trigger mwifiex_sdio_remove() from sysfs at
> the same time this code is running?

Yes. There would be a race. It will be avoided with Brian's code refactoring approach.

Regards,
Amitkumar

Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Kalle Valo]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

>> +/* reset_trigger variable is used to identify if mwifiex_sdio_remove()
>> + * is called by sdio_work during reset or the call is from sdio subsystem.
>> + * We will cancel sdio_work only if the call is from sdio subsystem.
>> + */
>> +static u8 reset_triggered;
>
> It would be really great if the driver supported multiple devices. IOW
> please avoid module-globals.

Good catch, it's a hard requirement to support multiple devices at the
same time.

-- 
Kalle Valo

Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Brian Norris]

On Thu, Oct 27, 2016 at 04:20:25PM +0300, Kalle Valo wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> 
> >> +/* reset_trigger variable is used to identify if mwifiex_sdio_remove()
> >> + * is called by sdio_work during reset or the call is from sdio subsystem.
> >> + * We will cancel sdio_work only if the call is from sdio subsystem.
> >> + */
> >> +static u8 reset_triggered;
> >
> > It would be really great if the driver supported multiple devices. IOW
> > please avoid module-globals.
> 
> Good catch, it's a hard requirement to support multiple devices at the
> same time.

BTW, this problem is repeated in several places throughout this driver.
For instance, look for 'user_rmmod' (why? you shouldn't need to treat
module unload differently...) and the work structs (and corresponding
'saved_adapter' and 'iface_flags') used for PCIe function-level reset
and SDIO reset.

Hopefully either Marvell's or my cleanups can move to get rid of these
anti-patterns soon...

Brian

RE: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Amitkumar Karwar]

Hi Brian,

> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Thursday, November 03, 2016 2:16 AM
> To: Kalle Valo
> Cc: Dmitry Torokhov; Amitkumar Karwar; linux-wireless@vger.kernel.org;
> Cathy Luo; Nishant Sarmukadam; Xinming Hu
> Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> remove_card
> 
> On Thu, Oct 27, 2016 at 04:20:25PM +0300, Kalle Valo wrote:
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> >
> > >> +/* reset_trigger variable is used to identify if
> > >> +mwifiex_sdio_remove()
> > >> + * is called by sdio_work during reset or the call is from sdio
> subsystem.
> > >> + * We will cancel sdio_work only if the call is from sdio
> subsystem.
> > >> + */
> > >> +static u8 reset_triggered;
> > >
> > > It would be really great if the driver supported multiple devices.
> > > IOW please avoid module-globals.
> >
> > Good catch, it's a hard requirement to support multiple devices at the
> > same time.
> 
> BTW, this problem is repeated in several places throughout this driver.
> For instance, look for 'user_rmmod' (why? you shouldn't need to treat
> module unload differently...)

We have 2 kinds of teardown cases.
1) Chip is going to be powered off.
	a) System reboot
	b) Someone manually removed wifi card from system
2) User unloaded the driver.

In case 1. b), we can't have logic to terminate WIFI connection and download SHUTDOWN command to firmware, as hardware itself is not present.

This logic is useful when user just unloads and loads the driver. Firmware download will be skipped in this case, as it's already running. SHUTDOWN command sent during unload has cleared firmware's state. 

'user_rmmod' flag doesn't create problem for supporting multiple devices. The flag is true during module unload OR reboot. It's applicable for all devices.

> and the work structs (and corresponding
> 'saved_adapter' and 'iface_flags') used for PCIe function-level reset
> and SDIO reset.

We are working on the v3 of this patch series. We will try to get rid of these variables along with global "work_struct" as you suggested.

Regards,
Amitkumar

Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Brian Norris]

On Wed, Nov 09, 2016 at 12:35:22PM +0000, Amitkumar Karwar wrote:
> Hi Brian,

Hi,

> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Thursday, November 03, 2016 2:16 AM
> > To: Kalle Valo
> > Cc: Dmitry Torokhov; Amitkumar Karwar; linux-wireless@vger.kernel.org;
> > Cathy Luo; Nishant Sarmukadam; Xinming Hu
> > Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> > remove_card
> > 
> > On Thu, Oct 27, 2016 at 04:20:25PM +0300, Kalle Valo wrote:
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> > >
> > > >> +/* reset_trigger variable is used to identify if
> > > >> +mwifiex_sdio_remove()
> > > >> + * is called by sdio_work during reset or the call is from sdio
> > subsystem.
> > > >> + * We will cancel sdio_work only if the call is from sdio
> > subsystem.
> > > >> + */
> > > >> +static u8 reset_triggered;
> > > >
> > > > It would be really great if the driver supported multiple devices.
> > > > IOW please avoid module-globals.
> > >
> > > Good catch, it's a hard requirement to support multiple devices at the
> > > same time.
> > 
> > BTW, this problem is repeated in several places throughout this driver.
> > For instance, look for 'user_rmmod' (why? you shouldn't need to treat
> > module unload differently...)
> 
> We have 2 kinds of teardown cases.
> 1) Chip is going to be powered off.
> 	a) System reboot
> 	b) Someone manually removed wifi card from system
> 2) User unloaded the driver.
> 
> In case 1. b), we can't have logic to terminate WIFI connection and
> download SHUTDOWN command to firmware, as hardware itself is not
> present.

That seems like a poor way of determining the difference between hot
unplug and clean shutdown. What if unplug happens concurrently with
rmmod? Seems like it'd be better to identify hardware failures as such,
and just skip talking to HW. Also, for interfaces that support unplug
well (like USB), shouldn't you be able to get hotplug info from the
driver core, instead of having to guess the inverse of that ("this was
not a hotplug") from static variables like that?

> This logic is useful when user just unloads and loads the driver.
> Firmware download will be skipped in this case, as it's already
> running. SHUTDOWN command sent during unload has cleared firmware's
> state. 

Why is that useful?

> 'user_rmmod' flag doesn't create problem for supporting multiple
> devices. The flag is true during module unload OR reboot. It's
> applicable for all devices.
> 
> > and the work structs (and corresponding
> > 'saved_adapter' and 'iface_flags') used for PCIe function-level reset
> > and SDIO reset.
> 
> We are working on the v3 of this patch series. We will try to get rid
> of these variables along with global "work_struct" as you suggested.

Good, thanks.

Brian

RE: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Amitkumar Karwar]

Hi Brian,

> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Thursday, November 10, 2016 2:07 AM
> To: Amitkumar Karwar
> Cc: Kalle Valo; Dmitry Torokhov; linux-wireless@vger.kernel.org; Cathy
> Luo; Nishant Sarmukadam; Xinming Hu
> Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> remove_card
> 
> On Wed, Nov 09, 2016 at 12:35:22PM +0000, Amitkumar Karwar wrote:
> > Hi Brian,
> 
> Hi,
> 
> > > From: Brian Norris [mailto:briannorris@chromium.org]
> > > Sent: Thursday, November 03, 2016 2:16 AM
> > > To: Kalle Valo
> > > Cc: Dmitry Torokhov; Amitkumar Karwar;
> > > linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > Xinming Hu
> > > Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion
> > > in remove_card
> > >
> > > On Thu, Oct 27, 2016 at 04:20:25PM +0300, Kalle Valo wrote:
> > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> > > >
> > > > >> +/* reset_trigger variable is used to identify if
> > > > >> +mwifiex_sdio_remove()
> > > > >> + * is called by sdio_work during reset or the call is from
> > > > >> +sdio
> > > subsystem.
> > > > >> + * We will cancel sdio_work only if the call is from sdio
> > > subsystem.
> > > > >> + */
> > > > >> +static u8 reset_triggered;
> > > > >
> > > > > It would be really great if the driver supported multiple
> devices.
> > > > > IOW please avoid module-globals.
> > > >
> > > > Good catch, it's a hard requirement to support multiple devices at
> > > > the same time.
> > >
> > > BTW, this problem is repeated in several places throughout this
> driver.
> > > For instance, look for 'user_rmmod' (why? you shouldn't need to
> > > treat module unload differently...)
> >
> > We have 2 kinds of teardown cases.
> > 1) Chip is going to be powered off.
> > 	a) System reboot
> > 	b) Someone manually removed wifi card from system
> > 2) User unloaded the driver.
> >
> > In case 1. b), we can't have logic to terminate WIFI connection and
> > download SHUTDOWN command to firmware, as hardware itself is not
> > present.
> 
> That seems like a poor way of determining the difference between hot
> unplug and clean shutdown. What if unplug happens concurrently with
> rmmod?

I agree. hotunplug thread may read the flag as "true" in this race and try to interact with hardware.

> Seems like it'd be better to identify hardware failures as such,
> and just skip talking to HW. Also, for interfaces that support unplug
> well (like USB), shouldn't you be able to get hotplug info from the
> driver core, instead of having to guess the inverse of that ("this was
> not a hotplug") from static variables like that?

You are right. We can identify hardware read/write failure and continue performing remaining cleanup.
This way "user_rmmod" flag can be avoided.
I will plan for this cleanup. Tests need to be performed with SDIO, PCIe use USB devices.
I just found hotunplug info is received in case of USB.(udev->state changed to USB_STATE_NOTATTACHED). We will make use of this.

> 
> > This logic is useful when user just unloads and loads the driver.
> > Firmware download will be skipped in this case, as it's already
> > running. SHUTDOWN command sent during unload has cleared firmware's
> > state.
> 
> Why is that useful?

Sending SHUTDOWN command helps firmware reset/clean it's state. As per our protocol, SHUTDOWN is the last command downloaded to firmware. After this, firmware's state would be same as freshly downloaded firmware.
Next time when driver is loaded, firmware download is skipped, as it's already active. Driver just need to send couple of initialization commands.

Regards,
Amitkumar

RE: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Amitkumar Karwar]

Hi Brian,

> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Thursday, November 10, 2016 2:07 AM
> To: Amitkumar Karwar
> Cc: Kalle Valo; Dmitry Torokhov; linux-wireless@vger.kernel.org; Cathy
> Luo; Nishant Sarmukadam; Xinming Hu
> Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> remove_card
> 
> On Wed, Nov 09, 2016 at 12:35:22PM +0000, Amitkumar Karwar wrote:
> > Hi Brian,
> 
> Hi,
> 
> > > From: Brian Norris [mailto:briannorris@chromium.org]
> > > Sent: Thursday, November 03, 2016 2:16 AM
> > > To: Kalle Valo
> > > Cc: Dmitry Torokhov; Amitkumar Karwar;
> > > linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > Xinming Hu
> > > Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion
> > > in remove_card
> > >
> > > On Thu, Oct 27, 2016 at 04:20:25PM +0300, Kalle Valo wrote:
> > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> > > >
> > > > >> +/* reset_trigger variable is used to identify if
> > > > >> +mwifiex_sdio_remove()
> > > > >> + * is called by sdio_work during reset or the call is from
> > > > >> +sdio
> > > subsystem.
> > > > >> + * We will cancel sdio_work only if the call is from sdio
> > > subsystem.
> > > > >> + */
> > > > >> +static u8 reset_triggered;
> > > > >
> > > > > It would be really great if the driver supported multiple
> devices.
> > > > > IOW please avoid module-globals.
> > > >
> > > > Good catch, it's a hard requirement to support multiple devices
> at
> > > > the same time.
> > >
> > > BTW, this problem is repeated in several places throughout this
> driver.
> > > For instance, look for 'user_rmmod' (why? you shouldn't need to
> > > treat module unload differently...)
> >
> > We have 2 kinds of teardown cases.
> > 1) Chip is going to be powered off.
> > 	a) System reboot
> > 	b) Someone manually removed wifi card from system
> > 2) User unloaded the driver.
> >
> > In case 1. b), we can't have logic to terminate WIFI connection and
> > download SHUTDOWN command to firmware, as hardware itself is not
> > present.
> 
> That seems like a poor way of determining the difference between hot
> unplug and clean shutdown. What if unplug happens concurrently with
> rmmod? Seems like it'd be better to identify hardware failures as such,
> and just skip talking to HW. Also, for interfaces that support unplug
> well (like USB), shouldn't you be able to get hotplug info from the
> driver core, instead of having to guess the inverse of that ("this was
> not a hotplug") from static variables like that?
> 
> > This logic is useful when user just unloads and loads the driver.
> > Firmware download will be skipped in this case, as it's already
> > running. SHUTDOWN command sent during unload has cleared firmware's
> > state.
> 
> Why is that useful?
> 
> > 'user_rmmod' flag doesn't create problem for supporting multiple
> > devices. The flag is true during module unload OR reboot. It's
> > applicable for all devices.
> >
> > > and the work structs (and corresponding 'saved_adapter' and
> > > 'iface_flags') used for PCIe function-level reset and SDIO reset.
> >
> > We are working on the v3 of this patch series. We will try to get rid
> > of these variables along with global "work_struct" as you suggested.

I observed some crash issues while testing with PCIe/SDIO chipsets after removing user_rmmod. We are still checking them. I will not include user_rmmod removal patch in v3 series. Other pcie_work related global variables are removed in v3 series.
Card is freed and recreated during mwifiex_sdio_card_reset_work(). It is one of the activities in sdio_work. So moving sdio_work inside card won't be straight forward.

Regards,
Amitkumar 

Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card

[Brian Norris]

On Wed, Nov 16, 2016 at 01:07:31PM +0000, Amitkumar Karwar wrote:
> I observed some crash issues while testing with PCIe/SDIO chipsets
> after removing user_rmmod. We are still checking them. I will not
> include user_rmmod removal patch in v3 series. Other pcie_work related
> global variables are removed in v3 series.

Sounds fine to me.

> Card is freed and recreated during mwifiex_sdio_card_reset_work(). It
> is one of the activities in sdio_work. So moving sdio_work inside card
> won't be straight forward.

That SDIO reset code is really crappy anyway, so this deserves some more
attention eventually. For instance, why does PCIe FLR have completely
different code structure? Also, the 'card' should really not be
reallocated every time. But anyway, that's not something to solve
immediately.

Brian

Re: [PATCH 1/5] mwifiex: remove redundant condition in main process

[Brian Norris]

On Mon, Oct 24, 2016 at 07:51:28PM +0530, Amitkumar Karwar wrote:
> This condition while calling mwifiex_check_ps_cond() is redundant.
> The function internally already takes care of it.
> 
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>

Unless you're intentionally *not* logging the "Delay Sleep Confirm" in
some cases, this looks good:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 2478ccd..3b31ea2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -355,10 +355,8 @@ process_start:
>  
>  		/* Check if we need to confirm Sleep Request
>  		   received previously */
> -		if (adapter->ps_state == PS_STATE_PRE_SLEEP) {
> -			if (!adapter->cmd_sent && !adapter->curr_cmd)
> -				mwifiex_check_ps_cond(adapter);
> -		}
> +		if (adapter->ps_state == PS_STATE_PRE_SLEEP)
> +			mwifiex_check_ps_cond(adapter);
>  
>  		/* * The ps_state may have been changed during processing of
>  		 * Sleep Request event.
> -- 
> 1.9.1
>