On Thu, Mar 16, 2017 at 12:38:57PM -0700, Brian Norris wrote:
> On Thu, Mar 16, 2017 at 11:41:15AM -0700, Brian Norris wrote:
> > On Thu, Mar 16, 2017 at 11:33:17AM -0700, Dmitry Torokhov wrote:
> > > You need to check this flag before queueing firmware dump work, and
> > > make sure it is not racy with setting this flag in mwifiex_pcie_remove()
> > > (and sdio).
> >
> > That's another approach that could work, but it's a little more
> > invasive.
>
> Never mind, that isn't too invasive. There's only one schedule_work() in
> pcie.c and two in sdio.c. We could even factor out a helper, that knows
> how to check the appropriate MWIFIEX_IFACE_* flags, if we really wanted
> to...
OK, so I took a crack at implementing this, and after thinking about it,
the "make sure it is not racy with setting this flag" part is tougher
than it seems. In the end, I think the key is that to eliminate the
race between setting and checking the flag, we just want to halt all
sources of more work -- e.g., commands (which could time out), or
debugfs entries (which could trigger a FW dump manually) -- without
fiddling with extra flags. We do this already in the first half of
mwifiex_remove_card(), when we terminate the main workqueue(s) and
unregister the net and wiphy devices.
IOW, we can move the cancel_work_sync() into the .cleanup_if() callback,
which occurs after the above described teardown, but before the PCIe
driver has actually called things like pci_disable_device() [1]. Then we
don't need any DONT_RUN flag either.
I'll test the above a bit more here, then send a v3 myself, with the
above reasoning captured. I *think* that should eliminate all the races
we've discussed here.
Brian
[1] BTW, I think I previously blamed mwifiex_init_shutdown_fw() for
racing with the FW dumper; I think that is not actually the smoking
gun (it was an educated guess). Based on testing, I see aborts if
we're still accessing the PCIe device (e.g., in the FW dumper) after
mwifiex_cleanup_pcie() -> pci_disable_device().
