RE: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-31 Thread Amitkumar Karwar 
Hi Kalle,

> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: Tuesday, October 25, 2016 10:25 PM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry Torokhov
> Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device
> unregister
> 

Please ignore this patch series (and also v5)
As Brian pointed out, it doesn't address race issues correctly. 
Xinming Hu has submitted a patch series(having 12 patches) from Brian which 
resolves the issues and performs some cleanup work.

Regards,
Amitkumar Karwar.

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-25 Thread Dmitry Torokhov 
On Tue, Oct 25, 2016 at 03:12:44PM +, Amitkumar Karwar wrote:
> Hi Brian,
> 
> Thanks for review.
> 
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > Sent: Tuesday, October 25, 2016 6:22 AM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry Torokhov
> > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device
> > unregister
> > 
> > Hi Amit,
> > 
> > On Thu, Oct 20, 2016 at 01:11:31PM +, Amitkumar Karwar wrote:
> > > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > > Sent: Tuesday, October 11, 2016 5:53 AM
> > > > To: Amitkumar Karwar
> > > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry
> > > > Torokhov
> > > > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during
> > > > device unregister
> > > >
> > > > On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> > > > > On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > > > > > From: Xinming Hu <h...@marvell.com>
> > > > > >
> > > > > > card->adapter gets initialized during device registration.
> > > > > > As it's not cleared, we may end up accessing invalid memory in
> > > > > > some corner cases. This patch fixes the problem.
> > > > > >
> > > > > > Signed-off-by: Xinming Hu <h...@marvell.com>
> > > > > > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > > > > > ---
> > > > > > v4: Same as v1, v2, v3
> > > > > > ---
> > > > > >  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> > > > > > drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> > > > > >  2 files changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > index f1eeb73..ba9e068 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > > mwifiex_adapter *adapter)
> > > > > > pci_disable_msi(pdev);
> > > > > >}
> > > > > > }
> > > > > > +   card->adapter = NULL;
> > > > > >  }
> > > > > >
> > > > > >  /* This function initializes the PCI-E host memory space, WCB
> > > > rings, etc.
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > index 8718950..4cad1c2 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct
> > > > > > mwifiex_adapter
> > > > *adapter)
> > > > > > struct sdio_mmc_card *card = adapter->card;
> > > > > >
> > > > > > if (adapter->card) {
> > > > > > +   card->adapter = NULL;
> > > > > > sdio_claim_host(card->func);
> > > > > > sdio_disable_func(card->func);
> > > > > > sdio_release_host(card->func);
> > > > >
> > > > > As discussed on v1, I had qualms about the raciness between
> > > > > reads/writes of card->adapter, but I believe we:
> > > > > (a) can't have any command activity while writing the ->adapter
> > > > > field (either we're just init'ing the device, or we've disabled
> > > > > interrupts and are tearing it down) and
> > > > > (b) can't have a race between suspend()/resume() and
> > > > > unregister_dev(), since unregister_dev() is called from device
> > > > > remove() (which should not be concurrent with suspend()).
> > > > >
> > > > > Also, I thought you had the same problem in usb.c, but

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-25 Thread Brian Norris 
Hi Amit,

On Tue, Oct 25, 2016 at 03:12:44PM +, Amitkumar Karwar wrote:
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > Sent: Tuesday, October 25, 2016 6:22 AM
> > To: Amitkumar Karwar
> > 
> > On Thu, Oct 20, 2016 at 01:11:31PM +, Amitkumar Karwar wrote:
> > > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > > Sent: Tuesday, October 11, 2016 5:53 AM
> > > > To: Amitkumar Karwar
> > > >
> > > > On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> > > > > On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > index f1eeb73..ba9e068 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > > mwifiex_adapter *adapter)
> > > > > > pci_disable_msi(pdev);
> > > > > >}
> > > > > > }
> > > > > > +   card->adapter = NULL;
> > > > > >  }
> > > > > >
> > > > > >  /* This function initializes the PCI-E host memory space, WCB
> > > > rings, etc.
> > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > index 8718950..4cad1c2 100644
> > > > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct
> > > > > > mwifiex_adapter
> > > > *adapter)
> > > > > > struct sdio_mmc_card *card = adapter->card;
> > > > > >
> > > > > > if (adapter->card) {
> > > > > > +   card->adapter = NULL;
> > > > > > sdio_claim_host(card->func);
> > > > > > sdio_disable_func(card->func);
> > > > > > sdio_release_host(card->func);
> > > > >

[...]

> > > Thanks for your comments.
> > >
> > > Actually description for this patch was ambiguous and incorrect. Sorry
> > > for that. This patch doesn't fix any race. In fact, we don't have a
> > > race between init and remove threads due to semaphore usage as per
> > > design. This patch just adds missing "card->adapter=NULL" so that when
> > > teardown/remove thread starts after init failure, it won't try freeing
> > > already freed things.
> > 
> > So to be clear, you'r talking about mwifiex_fw_dpc(), which in the error
> > path
> > has:
> > 
> >if (adapter->if_ops.unregister_dev)
> > adapter->if_ops.unregister_dev(adapter); <--- POINT A:
> > This is where you want to set ->adapter = NULL ...
> > if (init_failed)
> > mwifiex_free_adapter(adapter);
> > up(sem); <--- POINT B: This is where you release the semaphore,
> > which is supposed to guarantee that remove() isn't happening
> > return;
> > }
> > 
> > But you *do* have a race between the above code and the remove code in
> > some cases. Particularly, see this:
> > 
> > static void mwifiex_pcie_remove(struct pci_dev *pdev) {
> > struct pcie_service_card *card;
> > struct mwifiex_adapter *adapter;
> > struct mwifiex_private *priv;
> > 
> > card = pci_get_drvdata(pdev);
> > if (!card)
> > return;
> > 
> > adapter = card->adapter; <--- POINT C: This can execute at the
> > same time as unregister_dev()
> > if (!adapter || !adapter->priv_num)
> > return;
> > 
> > if (user_rmmod && !adapter->mfg_mode) { #ifdef CONFIG_PM_SLEEP
> > if (adapter->is_suspended)
> > mwifiex_pcie_resume(>dev); #endif
> > 
> > mwifiex_deauthenticate_all(adapter);
> > 
> > priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> > 
> > mwifiex_disable_auto_ds(priv);
> > 
> > mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
> > }
> > 
> > mwifiex_remove_card(card->adapter, _remove_card_sem); <---
> > POINT D: You only grab the semaphore here }
> > 
> > So IIUC, you have a race **even here, where you claim the semaphore
> > should protect you** such that the adapter might be freed, but you still
> > can access it anywhere between C and D. i.e., you can see this:
> > 
> > Thread 1  Thread 2
> >   (1) POINT C (retrieve adapter !=
> > NULL)
> > (2) POINT A (set adapter NULL)
> > (3) POINT B (adapter has been freed)
> >   (3) Keep accessing freed
> > adapter structure
> >   (4) POINT D - acquire semaphore,
> > but we're too late
> > 
> > Step 3 is an error, and AFAICT, that's exactly what you're trying to
> > solve in this patch. It essentially comes down to the same fact: you're
> > getting a reference to the adapter structure

RE: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-25 Thread Amitkumar Karwar 
Hi Brian,

Thanks for review.

> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: Tuesday, October 25, 2016 6:22 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry Torokhov
> Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device
> unregister
> 
> Hi Amit,
> 
> On Thu, Oct 20, 2016 at 01:11:31PM +, Amitkumar Karwar wrote:
> > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > Sent: Tuesday, October 11, 2016 5:53 AM
> > > To: Amitkumar Karwar
> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry
> > > Torokhov
> > > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during
> > > device unregister
> > >
> > > On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> > > > On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > > > > From: Xinming Hu <h...@marvell.com>
> > > > >
> > > > > card->adapter gets initialized during device registration.
> > > > > As it's not cleared, we may end up accessing invalid memory in
> > > > > some corner cases. This patch fixes the problem.
> > > > >
> > > > > Signed-off-by: Xinming Hu <h...@marvell.com>
> > > > > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > > > > ---
> > > > > v4: Same as v1, v2, v3
> > > > > ---
> > > > >  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> > > > > drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> > > > >  2 files changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > index f1eeb73..ba9e068 100644
> > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > > mwifiex_adapter *adapter)
> > > > >   pci_disable_msi(pdev);
> > > > >  }
> > > > >   }
> > > > > + card->adapter = NULL;
> > > > >  }
> > > > >
> > > > >  /* This function initializes the PCI-E host memory space, WCB
> > > rings, etc.
> > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > index 8718950..4cad1c2 100644
> > > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct
> > > > > mwifiex_adapter
> > > *adapter)
> > > > >   struct sdio_mmc_card *card = adapter->card;
> > > > >
> > > > >   if (adapter->card) {
> > > > > + card->adapter = NULL;
> > > > >   sdio_claim_host(card->func);
> > > > >   sdio_disable_func(card->func);
> > > > >   sdio_release_host(card->func);
> > > >
> > > > As discussed on v1, I had qualms about the raciness between
> > > > reads/writes of card->adapter, but I believe we:
> > > > (a) can't have any command activity while writing the ->adapter
> > > > field (either we're just init'ing the device, or we've disabled
> > > > interrupts and are tearing it down) and
> > > > (b) can't have a race between suspend()/resume() and
> > > > unregister_dev(), since unregister_dev() is called from device
> > > > remove() (which should not be concurrent with suspend()).
> > > >
> > > > Also, I thought you had the same problem in usb.c, but in fact,
> > > > you fixed that ages ago here:
> > > >
> > > > 353d2a69ea26 mwifiex: fix issues in driver unload path for USB
> > > > chipsets
> > > >
> > > > Would be nice if fixes were bettery synchronized across the three
> > > > interface drivers you support. We seem to be discovering
> > > > unnecessary divergence on a few points recently.
> > >

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-24 Thread Brian Norris 
Hi Amit,

On Thu, Oct 20, 2016 at 01:11:31PM +, Amitkumar Karwar wrote:
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > Sent: Tuesday, October 11, 2016 5:53 AM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > raja...@google.com; Xinming Hu; abhishe...@google.com; Dmitry Torokhov
> > Subject: Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device
> > unregister
> > 
> > On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> > > On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > > > From: Xinming Hu <h...@marvell.com>
> > > >
> > > > card->adapter gets initialized during device registration.
> > > > As it's not cleared, we may end up accessing invalid memory in some
> > > > corner cases. This patch fixes the problem.
> > > >
> > > > Signed-off-by: Xinming Hu <h...@marvell.com>
> > > > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > > > ---
> > > > v4: Same as v1, v2, v3
> > > > ---
> > > >  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> > > > drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> > > >  2 files changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > index f1eeb73..ba9e068 100644
> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct
> > mwifiex_adapter *adapter)
> > > > pci_disable_msi(pdev);
> > > >}
> > > > }
> > > > +   card->adapter = NULL;
> > > >  }
> > > >
> > > >  /* This function initializes the PCI-E host memory space, WCB
> > rings, etc.
> > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > index 8718950..4cad1c2 100644
> > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter
> > *adapter)
> > > > struct sdio_mmc_card *card = adapter->card;
> > > >
> > > > if (adapter->card) {
> > > > +   card->adapter = NULL;
> > > > sdio_claim_host(card->func);
> > > > sdio_disable_func(card->func);
> > > > sdio_release_host(card->func);
> > >
> > > As discussed on v1, I had qualms about the raciness between
> > > reads/writes of card->adapter, but I believe we:
> > > (a) can't have any command activity while writing the ->adapter field
> > > (either we're just init'ing the device, or we've disabled interrupts
> > > and are tearing it down) and
> > > (b) can't have a race between suspend()/resume() and unregister_dev(),
> > > since unregister_dev() is called from device remove() (which should
> > > not be concurrent with suspend()).
> > >
> > > Also, I thought you had the same problem in usb.c, but in fact, you
> > > fixed that ages ago here:
> > >
> > > 353d2a69ea26 mwifiex: fix issues in driver unload path for USB
> > > chipsets
> > >
> > > Would be nice if fixes were bettery synchronized across the three
> > > interface drivers you support. We seem to be discovering unnecessary
> > > divergence on a few points recently.
> > >
> > > At any rate:
> > >
> > > Reviewed-by: Brian Norris <briannor...@chromium.org>
> > > Tested-by: Brian Norris <briannor...@chromium.org>
> > 
> > Dmitry helped me re-realize my original qualms:
> > 
> > mwifiex_unregister_dev() is called in the failure path for your async FW
> > request, and so it may race with suspend(). So I retract my Reviewed-by.
> > Sorry.
> 
> Thanks for your comments.
> 
> Actually description for this patch was ambiguous and incorrect. Sorry
> for that. This patch doesn't fix any race. In fact, we don't have a
> race between init and remove threads due to semaphore usage as per
> design. This patch just adds missing "card->adapter=NULL" so that whe

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-10 Thread Brian Norris 
+ Dmitry

Hi Amit,

On Mon, Oct 10, 2016 at 01:53:32PM -0700, Brian Norris wrote:
> On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu 
> > 
> > card->adapter gets initialized during device registration.
> > As it's not cleared, we may end up accessing invalid memory
> > in some corner cases. This patch fixes the problem.
> > 
> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Amitkumar Karwar 
> > ---
> > v4: Same as v1, v2, v3
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
> >  drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index f1eeb73..ba9e068 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct 
> > mwifiex_adapter *adapter)
> > pci_disable_msi(pdev);
> >}
> > }
> > +   card->adapter = NULL;
> >  }
> >  
> >  /* This function initializes the PCI-E host memory space, WCB rings, etc.
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 8718950..4cad1c2 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter 
> > *adapter)
> > struct sdio_mmc_card *card = adapter->card;
> >  
> > if (adapter->card) {
> > +   card->adapter = NULL;
> > sdio_claim_host(card->func);
> > sdio_disable_func(card->func);
> > sdio_release_host(card->func);
> 
> As discussed on v1, I had qualms about the raciness between reads/writes
> of card->adapter, but I believe we:
> (a) can't have any command activity while writing the ->adapter field
> (either we're just init'ing the device, or we've disabled interrupts and
> are tearing it down) and
> (b) can't have a race between suspend()/resume() and unregister_dev(),
> since unregister_dev() is called from device remove() (which should not
> be concurrent with suspend()).
> 
> Also, I thought you had the same problem in usb.c, but in fact, you
> fixed that ages ago here:
> 
> 353d2a69ea26 mwifiex: fix issues in driver unload path for USB chipsets
> 
> Would be nice if fixes were bettery synchronized across the three
> interface drivers you support. We seem to be discovering unnecessary
> divergence on a few points recently.
> 
> At any rate:
> 
> Reviewed-by: Brian Norris 
> Tested-by: Brian Norris 

Dmitry helped me re-realize my original qualms:

mwifiex_unregister_dev() is called in the failure path for your async FW
request, and so it may race with suspend(). So I retract my Reviewed-by.
Sorry.

I'm going to look into converting to asynchronous device probing, which
might remove the need for async FW request, and would therefore resolve
both patch 1 and 3's races without any additional complicated hacks. But
I'm not sure if that will satisfy all mwifiex users well enough. I'll
have to give it a little more thought. Any thoughts from your side,
Amit?

Brian

Re: [PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-10 Thread Brian Norris 
Hi Amit,

On Thu, Oct 06, 2016 at 11:36:24PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu 
> 
> card->adapter gets initialized during device registration.
> As it's not cleared, we may end up accessing invalid memory
> in some corner cases. This patch fixes the problem.
> 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---
> v4: Same as v1, v2, v3
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index f1eeb73..ba9e068 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct 
> mwifiex_adapter *adapter)
>   pci_disable_msi(pdev);
>  }
>   }
> + card->adapter = NULL;
>  }
>  
>  /* This function initializes the PCI-E host memory space, WCB rings, etc.
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 8718950..4cad1c2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
>   struct sdio_mmc_card *card = adapter->card;
>  
>   if (adapter->card) {
> + card->adapter = NULL;
>   sdio_claim_host(card->func);
>   sdio_disable_func(card->func);
>   sdio_release_host(card->func);

As discussed on v1, I had qualms about the raciness between reads/writes
of card->adapter, but I believe we:
(a) can't have any command activity while writing the ->adapter field
(either we're just init'ing the device, or we've disabled interrupts and
are tearing it down) and
(b) can't have a race between suspend()/resume() and unregister_dev(),
since unregister_dev() is called from device remove() (which should not
be concurrent with suspend()).

Also, I thought you had the same problem in usb.c, but in fact, you
fixed that ages ago here:

353d2a69ea26 mwifiex: fix issues in driver unload path for USB chipsets

Would be nice if fixes were bettery synchronized across the three
interface drivers you support. We seem to be discovering unnecessary
divergence on a few points recently.

At any rate:

Reviewed-by: Brian Norris 
Tested-by: Brian Norris 

Thanks.

[PATCH v4 1/3] mwifiex: reset card->adapter during device unregister

2016-10-06 Thread Amitkumar Karwar 
From: Xinming Hu 

card->adapter gets initialized during device registration.
As it's not cleared, we may end up accessing invalid memory
in some corner cases. This patch fixes the problem.

Signed-off-by: Xinming Hu 
Signed-off-by: Amitkumar Karwar 
---
v4: Same as v1, v2, v3
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
 drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index f1eeb73..ba9e068 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter 
*adapter)
pci_disable_msi(pdev);
   }
}
+   card->adapter = NULL;
 }
 
 /* This function initializes the PCI-E host memory space, WCB rings, etc.
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 8718950..4cad1c2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
struct sdio_mmc_card *card = adapter->card;
 
if (adapter->card) {
+   card->adapter = NULL;
sdio_claim_host(card->func);
sdio_disable_func(card->func);
sdio_release_host(card->func);
-- 
1.9.1