Hi Brian, > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > Sent: Wednesday, October 05, 2016 3:28 AM > To: Amitkumar Karwar > Cc: linux-wireless@xxxxxxxxxxxxxxx; Cathy Luo; Nishant Sarmukadam; > rajatja@xxxxxxxxxx; briannorris@xxxxxxxxxx; Xinming Hu > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device > unregister > > Hi, > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote: > > From: Xinming Hu <huxm@xxxxxxxxxxx> > > > > 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 <huxm@xxxxxxxxxxx> > > Signed-off-by: Amitkumar Karwar <akarwar@xxxxxxxxxxx> > > --- > > v2: Same as v1 > > --- > > 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; > > I think you have a similar problem here as in patch 2; there is no > locking to protect fields in struct pcie_service_card or struct > sdio_mmc_card below. That problem kind of already exists, except that > you only write the value of card->adapter once at registration time, so > it's not actually unsafe. But now that you're introducing a second > write, you have a problem. > > Brian > We have a global "add_remove_card_sem" semaphore in our code for synchronizing initialization and teardown threads. Ideally "init + teardown/reboot" should not have a race issue with this logic Later there was a loophole introduced in this after using async firmware download API. During initialization, firmware downloads asynchronously in a separate thread where might have released the semaphore. I am working on a patch to fix this. So "card->adapter" doesn't need to have locking. Even if we have two write operations, those two threads can't run simultaneously due to above mentioned logic. Regards, Amitkumar
