Hi Amit, On Thu, Oct 06, 2016 at 01:03:02PM +0000, Amitkumar Karwar wrote: > > From: linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless- > > ow...@vger.kernel.org] On Behalf Of Brian Norris > > Sent: Wednesday, October 05, 2016 10:00 PM > > To: Amitkumar Karwar > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; > > raja...@google.com; Xinming Hu > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device > > unregister > > > > On Wed, Oct 05, 2016 at 02:04:53PM +0000, Amitkumar Karwar wrote: > > > > From: Brian Norris [mailto:briannor...@chromium.org] > > > > Sent: Wednesday, October 05, 2016 3:28 AM > > > > To: Amitkumar Karwar > > > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; > > > > raja...@google.com; briannor...@google.com; Xinming Hu > > > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during > > > > device unregister > > > > > > > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote: > > > > > > > --- 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;
... > > What about writes racing with reads? You have lots of unsynchronized > > cases that read this, although most of them should be halted by now > > (e.g., cmd processing). I was looking at suspend() in particular, which > > I thought you were looking at in this patch series. > > Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files > > Writes won't have race with reads. > > 1) write 1 --- "card->adapter = adapter;" in mwifiex_register_dev() > This place is at the beginning of initialization. > mwifiex_pcie_probe() -> mwifiex_add_card() -> > adapter->if_ops.register_dev() > There is no chance that "card->adapter" is read anywhere at this point. > FW is not yet downloaded Sure. > 2) write 2 ---- "card->adapter = NULL;" in mwifiex_unregister_dev() > This place the end of teardown phase. > Interrupts are disabled and all cleanup is done. We have > "card->adapter" NULL checks at entry point of > suspend/remove/resume, if they get called after this. I guess the question boils down to: can driver suspend() race with mwifiex_unregister_dev() here, then? And I guess the answer is "no", because unregistration only happens via PCIe driver remove() -> mwifiex_remove_card() -> adapter->if_ops.unregister_dev() and I think the device driver core guarantees that suspend() and remove() won't race. In that case: Reviewed-by: Brian Norris <briannor...@chromium.org> I'll reply to the latest revision too, since it's identical. Thanks for the explanations. Regards, Brian
