Hi, > -----Original Message----- > From: Kalle Valo [mailto:kvalo@xxxxxxxxxxxxxx] > Sent: 2018年1月9日 15:40 > To: Brian Norris <briannorris@xxxxxxxxxxxx> > Cc: Xinming Hu <huxm@xxxxxxxxxxx>; Linux Wireless > <linux-wireless@xxxxxxxxxxxxxxx>; Dmitry Torokhov <dtor@xxxxxxxxxx>; > rajatja@xxxxxxxxxx; Zhiyuan Yang <yangzy@xxxxxxxxxxx>; Tim Song > <songtao@xxxxxxxxxxx>; Cathy Luo <cluo@xxxxxxxxxxx>; James Cao > <jcao@xxxxxxxxxxx>; Ganapathi Bhat <gbhat@xxxxxxxxxxx>; Ellie Reeves > <ellierevves@xxxxxxxxx> > Subject: [EXT] Re: [PATCH] mwifiex: cancel pcie/sdio work in remove/shutdown > handler > > External Email > > ---------------------------------------------------------------------- > Brian Norris <briannorris@xxxxxxxxxxxx> writes: > > >> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > >> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > >> @@ -310,6 +310,8 @@ static void mwifiex_pcie_remove(struct pci_dev > *pdev) > >> mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN); > >> } > >> > >> + cancel_work_sync(&card->work); > >> + > > > > Just FYI, this "fix" is not a real fix. It will likely paper over some > > of your bugs (where, e.g., the FW shutdown command times out in the > > previous couple of lines), but this highlights the fact that there are > > other races that could trigger the same behavior. You're not fixing > > those. > > > > For example, what if somebody initiates a scan or other nl80211 > > command between the above line and mwifiex_remove_card()? That > command > > could potentially time out too. > > The hardware status have been reset before downloading the last command(FUNC SHUTDOWN), in this way, follow commands download will be ignored and warned. > > The proper fix would be to institute some kind of mutual exclusion > > (locking) between mwifiex_shutdown_sw() and mwifiex_remove_card(), so > > that they can't occur at the same time. > > I am not sure whether there is any mutual exclusion protect between pcie_reset and pcie_remove in pcie core. But it looks a different race. We still need this fix, right? Regards, Simon > > Unfortunately, I only paid attention to this after Kalle already > > applied this patch. Personally, I'd prefer this patch not get applied, > > since it's a bad solution to an obvious problem, which instead leaves > > a subtle problem that perhaps no one will bother fixing. > > I can revert it, that's not a problem. Can I use the text below as explanation for > the revert? > > ---------------------------------------------------------------------- > Brian Norris <briannorris@xxxxxxxxxxxx> says: > > Just FYI, this "fix" is not a real fix. It will likely paper over some of your bugs > (where, e.g., the FW shutdown command times out in the previous couple of > lines), but this highlights the fact that there are other races that could trigger > the same behavior. You're not fixing those. > > For example, what if somebody initiates a scan or other nl80211 command > between the above line and mwifiex_remove_card()? That command could > potentially time out too. > > The proper fix would be to institute some kind of mutual exclusion > (locking) between mwifiex_shutdown_sw() and mwifiex_remove_card(), so that > they can't occur at the same time. > > ---------------------------------------------------------------------- > > -- > Kalle Valo
