Hi Brian, > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > Sent: Thursday, November 10, 2016 2:07 AM > To: Amitkumar Karwar > Cc: Kalle Valo; Dmitry Torokhov; linux-wireless@xxxxxxxxxxxxxxx; Cathy > Luo; Nishant Sarmukadam; Xinming Hu > Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in > remove_card > > On Wed, Nov 09, 2016 at 12:35:22PM +0000, Amitkumar Karwar wrote: > > Hi Brian, > > Hi, > > > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > > Sent: Thursday, November 03, 2016 2:16 AM > > > To: Kalle Valo > > > Cc: Dmitry Torokhov; Amitkumar Karwar; > > > linux-wireless@xxxxxxxxxxxxxxx; Cathy Luo; Nishant Sarmukadam; > > > Xinming Hu > > > Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion > > > in remove_card > > > > > > On Thu, Oct 27, 2016 at 04:20:25PM +0300, Kalle Valo wrote: > > > > Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> writes: > > > > > > > > >> +/* reset_trigger variable is used to identify if > > > > >> +mwifiex_sdio_remove() > > > > >> + * is called by sdio_work during reset or the call is from > > > > >> +sdio > > > subsystem. > > > > >> + * We will cancel sdio_work only if the call is from sdio > > > subsystem. > > > > >> + */ > > > > >> +static u8 reset_triggered; > > > > > > > > > > It would be really great if the driver supported multiple > devices. > > > > > IOW please avoid module-globals. > > > > > > > > Good catch, it's a hard requirement to support multiple devices > at > > > > the same time. > > > > > > BTW, this problem is repeated in several places throughout this > driver. > > > For instance, look for 'user_rmmod' (why? you shouldn't need to > > > treat module unload differently...) > > > > We have 2 kinds of teardown cases. > > 1) Chip is going to be powered off. > > a) System reboot > > b) Someone manually removed wifi card from system > > 2) User unloaded the driver. > > > > In case 1. b), we can't have logic to terminate WIFI connection and > > download SHUTDOWN command to firmware, as hardware itself is not > > present. > > That seems like a poor way of determining the difference between hot > unplug and clean shutdown. What if unplug happens concurrently with > rmmod? Seems like it'd be better to identify hardware failures as such, > and just skip talking to HW. Also, for interfaces that support unplug > well (like USB), shouldn't you be able to get hotplug info from the > driver core, instead of having to guess the inverse of that ("this was > not a hotplug") from static variables like that? > > > This logic is useful when user just unloads and loads the driver. > > Firmware download will be skipped in this case, as it's already > > running. SHUTDOWN command sent during unload has cleared firmware's > > state. > > Why is that useful? > > > 'user_rmmod' flag doesn't create problem for supporting multiple > > devices. The flag is true during module unload OR reboot. It's > > applicable for all devices. > > > > > and the work structs (and corresponding 'saved_adapter' and > > > 'iface_flags') used for PCIe function-level reset and SDIO reset. > > > > We are working on the v3 of this patch series. We will try to get rid > > of these variables along with global "work_struct" as you suggested. I observed some crash issues while testing with PCIe/SDIO chipsets after removing user_rmmod. We are still checking them. I will not include user_rmmod removal patch in v3 series. Other pcie_work related global variables are removed in v3 series. Card is freed and recreated during mwifiex_sdio_card_reset_work(). It is one of the activities in sdio_work. So moving sdio_work inside card won't be straight forward. Regards, Amitkumar
