Hi Brian,

From: Brian Norris [mailto:briannorris@chromium.org]
Sent: Thursday, December 01, 2016 12:04 AM
To: Amitkumar Karwar
Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
rajatja@google.com; dmitry.torokhov@gmail.com; Xinming Hu
Subject: Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during
card remove process

On Wed, Nov 30, 2016 at 12:39:11PM +0000, Amitkumar Karwar wrote:

quoted

quoted

Ugh, yet another band-aid? You might do better to still cancel any
pending work, just don't do it synchronously. i.e., do

cancel_work()

quoted

quoted

after you've removed the card. It doesn't make sense to do a FW

dump

quoted

quoted

on the "new" adapter when it was requested for the old one.

I could not find async version of cancel_work().

cancel_work() *is* asynchronous. It does not synchronize with the last
event, so you won't have the deadlock. (Remember: the synchronous
version is cancel_work_sync().)

My bad! What I meant is "I could not find async version of cancel_work_sync()"
cancel_work() isn't available in http://lxr.free-electrons.com/source/kernel/workqueue.c
Anyways, clear_bit() after remove() during card reset would address the problem.

quoted

We can fix this problem with below change at the end of
mwifiex_sdio_work(). All pending work requests would be ignored.

--------
@ -2571,6 +2571,8 @@ static void mwifiex_sdio_work(struct work_struct

*work)

quoted

        if (test_and_clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET,
                               &iface_work_flags))
                mwifiex_sdio_card_reset_work(save_adapter);
+       clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags);
+       clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &iface_work_flags);
 }
----------

I don't think that's exactly what you want. That might lose events,
won't it? I'd rather this sort of hack go into
mwifiex_recreate_adapter(), in between the remove() and probe() calls,
where you don't expect any new events to trigger. And maybe include a
comment as to why.

Right. I have just posted a patch for this.

quoted

quoted

I think I've asked elsewhere but didn't receive an answer: why is
SDIO's mwifiex_recreate_adapter() so much different from PCIe's
mwifiex_do_flr()? It seems like the latter should be refactored to
remove some of the PCIe-specific cruft from main.c and then reused
for SDIO.

Our initial SDIO card reset implementation was based on MMC APIs

where

quoted

remove() and probe() would automatically get called by MMC subsystem
after power cycle.
https://www.spinics.net/lists/linux-wireless/msg98435.html
Later it was improved by Andreas Fenkart by replacing those power
cycle APIs with mmc_hw_reset().

Right.

quoted

For PCIe, function level reset is standard feature. We implemented
".reset_notify" handler which gets called after and before FLR.

OK.

quoted

You are right. We can have SDIO's handling similar to PCIe and avoid
destroying+recreating adapter/card.

So all in all, you're saying it's just an artifact of history, and
there's no good reason they are so different? If so, then this looks
like another instance where you would have done well to refactor and
improve the existing mechanisms at the same time as you added new
features (i.e., PCIe FLR). I've seen this problem already several
times, where it seems development for your SDIO/PCIe/USB interface
drivers occur almost in isolation. IMO, it'd do you well to notice
these patterns while implementing features in the first place. The more
code you can share, the fewer bugs you (or I) will have to chase down.

Thanks for your guidance. I'll follow this for future development.

Regards,
Amitkumar