From: Doug Anderson <dianders@chromium.org> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: Linux MMC List <linux-mmc@vger.kernel.org>, Adrian Hunter <adrian.hunter@intel.com>, Matthias Kaehlcke <mka@chromium.org>, Kalle Valo <kvalo@codeaurora.org>, Tony Lindgren <tony@atomide.com>, Wen Gong <wgong@codeaurora.org>, Erik Stromdahl <erik.stromdahl@gmail.com>, Eyal Reizer <eyalreizer@gmail.com>, linux-wireless <linux-wireless@vger.kernel.org>, Brian Norris <briannorris@chromium.org> Subject: Re: [PATCH v2 1/3] mwifiex: Re-work support for SDIO HW reset Date: Tue, 12 Nov 2019 10:04:48 -0800 [thread overview] Message-ID: <CAD=FV=VWdzqGY778SXZnC1YDyxc6EHPgRjkJ_2sOHrxHTams-w@mail.gmail.com> (raw) In-Reply-To: <CAPDyKFq-djJFyYu6Wzg9t9hLOQMuqff9KVhbx5Zp5i=Fsynsdw@mail.gmail.com> Hi, On Tue, Nov 12, 2019 at 4:14 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c > > > index 24c041dad9f6..2417c94c29c0 100644 > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c > > > @@ -444,6 +444,9 @@ static int mwifiex_sdio_suspend(struct device *dev) > > > return 0; > > > } > > > > > > + if (!adapter->is_adapter_up) > > > + return -EBUSY; > > > > I'm moderately concerned that there might be cases where firmware > > never got loaded but we could suspend/resume OK. ...and now we never > > will? I'm not familiar enough with the code to know if this is a real > > concern, so I guess we can do this and then see... > > There is a completion variable that is used to make sure the firmware > is loaded, before the mwifiex driver runs ->suspend|remove(). This is > needed, because during ->probe() the FW will be loaded asynchronously, > hence both mwifiex_sdio_remove() and mwifiex_sdio_suspend(), may be > called while waiting for the FW to be loaded. > > If a HW reset has been scheduled but not completed, which would be the > case if mmc_hw_reset() gets called after mmc_pm_notify() with a > PM_SUSPEND_PREPARE. This is because mmc_pm_notify() then disables the > rescan work, but then re-kicks/enables it at PM_POST_SUSPEND (after > the system has resumed). > > Returning -EBUSY, should allow the mmc rescan work to be completed > when the system have resumed. > > Of course, one could also considering using pm_wakeup_event(), in case > mmc_hw_reset() needed to schedule the reset, as to prevent the system > for suspending for a small amount of time. As to make sure the rescan > work, gets to run. But I am not sure that's needed here. I was more worried that we could get into a state where we'd return EBUSY forever, but I think I've convinced myself that this isn't possible. If we fail to load things then the adapter variable will be freed anyway. > Finally, if you want to verify that the above system suspend path > works fine, you could change the call to "_mmc_detect_change(host, 0, > false)" in mmc_sdio_hw_reset(), into "_mmc_detect_change(host, > msecs_to_jiffies(30000), false)", in patch3. > > This should leave you a 30s window of where you can try to system > suspend the platform, while also waiting for the scheduled reset to be > completed. It worked. https://pastebin.com/NdsvAdE8
WARNING: multiple messages have this Message-ID (diff)
From: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> To: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> Cc: Linux MMC List <linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Adrian Hunter <adrian.hunter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, Matthias Kaehlcke <mka-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>, Wen Gong <wgong-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Erik Stromdahl <erik.stromdahl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Eyal Reizer <eyalreizer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, linux-wireless <linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> Subject: Re: [PATCH v2 1/3] mwifiex: Re-work support for SDIO HW reset Date: Tue, 12 Nov 2019 10:04:48 -0800 [thread overview] Message-ID: <CAD=FV=VWdzqGY778SXZnC1YDyxc6EHPgRjkJ_2sOHrxHTams-w@mail.gmail.com> (raw) In-Reply-To: <CAPDyKFq-djJFyYu6Wzg9t9hLOQMuqff9KVhbx5Zp5i=Fsynsdw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> Hi, On Tue, Nov 12, 2019 at 4:14 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote: > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c > > > index 24c041dad9f6..2417c94c29c0 100644 > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c > > > @@ -444,6 +444,9 @@ static int mwifiex_sdio_suspend(struct device *dev) > > > return 0; > > > } > > > > > > + if (!adapter->is_adapter_up) > > > + return -EBUSY; > > > > I'm moderately concerned that there might be cases where firmware > > never got loaded but we could suspend/resume OK. ...and now we never > > will? I'm not familiar enough with the code to know if this is a real > > concern, so I guess we can do this and then see... > > There is a completion variable that is used to make sure the firmware > is loaded, before the mwifiex driver runs ->suspend|remove(). This is > needed, because during ->probe() the FW will be loaded asynchronously, > hence both mwifiex_sdio_remove() and mwifiex_sdio_suspend(), may be > called while waiting for the FW to be loaded. > > If a HW reset has been scheduled but not completed, which would be the > case if mmc_hw_reset() gets called after mmc_pm_notify() with a > PM_SUSPEND_PREPARE. This is because mmc_pm_notify() then disables the > rescan work, but then re-kicks/enables it at PM_POST_SUSPEND (after > the system has resumed). > > Returning -EBUSY, should allow the mmc rescan work to be completed > when the system have resumed. > > Of course, one could also considering using pm_wakeup_event(), in case > mmc_hw_reset() needed to schedule the reset, as to prevent the system > for suspending for a small amount of time. As to make sure the rescan > work, gets to run. But I am not sure that's needed here. I was more worried that we could get into a state where we'd return EBUSY forever, but I think I've convinced myself that this isn't possible. If we fail to load things then the adapter variable will be freed anyway. > Finally, if you want to verify that the above system suspend path > works fine, you could change the call to "_mmc_detect_change(host, 0, > false)" in mmc_sdio_hw_reset(), into "_mmc_detect_change(host, > msecs_to_jiffies(30000), false)", in patch3. > > This should leave you a 30s window of where you can try to system > suspend the platform, while also waiting for the scheduled reset to be > completed. It worked. https://pastebin.com/NdsvAdE8
next prev parent reply other threads:[~2019-11-12 18:05 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-09 10:30 [PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards Ulf Hansson 2019-11-09 10:30 ` Ulf Hansson 2019-11-09 10:30 ` [PATCH v2 1/3] mwifiex: Re-work support for SDIO HW reset Ulf Hansson 2019-11-09 10:30 ` Ulf Hansson 2019-11-12 0:33 ` Doug Anderson 2019-11-12 0:33 ` Doug Anderson 2019-11-12 12:13 ` Ulf Hansson 2019-11-12 12:13 ` Ulf Hansson 2019-11-12 18:04 ` Doug Anderson [this message] 2019-11-12 18:04 ` Doug Anderson 2019-11-13 15:00 ` Ulf Hansson 2019-11-13 15:00 ` Ulf Hansson 2019-11-09 10:30 ` [PATCH v2 2/3] mmc: core: Drop check for mmc_card_is_removable() in mmc_rescan() Ulf Hansson 2019-11-09 10:30 ` Ulf Hansson 2019-11-09 10:30 ` [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards Ulf Hansson 2019-11-09 10:30 ` Ulf Hansson 2019-11-12 0:33 ` Doug Anderson 2019-11-12 0:33 ` Doug Anderson 2019-11-12 12:19 ` Ulf Hansson 2019-11-12 12:19 ` Ulf Hansson 2019-11-20 6:28 ` Kalle Valo 2019-11-20 6:28 ` Kalle Valo 2019-11-20 6:28 ` Kalle Valo 2019-11-20 6:28 ` Kalle Valo [not found] ` <87zhgr5af6.fsf@codeaurora.org> 2019-11-20 7:10 ` wgong 2019-11-20 7:10 ` wgong 2019-11-20 7:10 ` wgong 2019-11-20 7:10 ` wgong-sgV2jX0FEOL9JmXXK+q4OQ [not found] ` <6e6b53b28581a8f1a2944ca0bc65311e@codeaurora.org> 2019-11-20 7:20 ` Kalle Valo 2019-11-20 7:20 ` Kalle Valo 2019-11-20 7:20 ` Kalle Valo 2019-11-20 7:20 ` Kalle Valo [not found] ` <0101016e87aeb8b6-761ad812-5da7-4b0d-8cae-c69633d90de0-000000@us-west-2.amazonses.com> 2019-11-20 12:10 ` Ulf Hansson 2019-11-20 12:10 ` Ulf Hansson 2019-11-20 12:10 ` Ulf Hansson 2019-11-20 16:41 ` Kalle Valo 2019-11-20 16:41 ` Kalle Valo 2019-11-20 16:41 ` Kalle Valo 2019-11-20 16:41 ` Kalle Valo [not found] ` <87zhgqmref.fsf@kamboji.qca.qualcomm.com> 2019-11-21 2:29 ` wgong 2019-11-21 2:29 ` wgong-sgV2jX0FEOL9JmXXK+q4OQ 2019-11-21 2:29 ` wgong 2019-11-21 2:29 ` wgong 2019-11-11 22:08 ` [PATCH v2 0/3] mmc: Fixup " Tony Lindgren 2019-11-11 22:08 ` Tony Lindgren 2019-11-12 12:23 ` Ulf Hansson 2019-11-12 12:23 ` Ulf Hansson 2019-11-12 18:00 ` Tony Lindgren 2019-11-12 18:00 ` Tony Lindgren 2019-11-12 0:51 ` Doug Anderson 2019-11-12 0:51 ` Doug Anderson 2019-11-12 12:27 ` Ulf Hansson 2019-11-12 12:27 ` Ulf Hansson 2019-11-12 17:42 ` Doug Anderson 2019-11-12 17:42 ` Doug Anderson 2019-11-13 15:11 ` Ulf Hansson 2019-11-13 15:11 ` Ulf Hansson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAD=FV=VWdzqGY778SXZnC1YDyxc6EHPgRjkJ_2sOHrxHTams-w@mail.gmail.com' \ --to=dianders@chromium.org \ --cc=adrian.hunter@intel.com \ --cc=briannorris@chromium.org \ --cc=erik.stromdahl@gmail.com \ --cc=eyalreizer@gmail.com \ --cc=kvalo@codeaurora.org \ --cc=linux-mmc@vger.kernel.org \ --cc=linux-wireless@vger.kernel.org \ --cc=mka@chromium.org \ --cc=tony@atomide.com \ --cc=ulf.hansson@linaro.org \ --cc=wgong@codeaurora.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.