| Message ID | 1492636631-28254-1-git-send-email-ulf.hansson@linaro.org |
|---|---|
| Headers | show |
| Series | mmc: Improve/fix support for SDIO IRQs | expand |
Ulf, On Wed, Apr 19, 2017 at 2:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > Changes in v2: > - Folded in a new change, patch 1/5 to fix a race condition while > processing SDIO IRQs. > - Fixed review comments provided by Dough. > - Updated change logs. > - Small changes to how ->ack_sdio_irq() is being invoked, to simplify > code. > - Added a revert patch of the dw_mmc change for runtime PM issues, which > was a quick fix for stable/fixes. > > Some general description of the series: > > Regressions for SDIO IRQs have been reported for dw_mmc, caused by enabling > runtime PM support for it. > > This series extends and improves the SDIO IRQs support in the core, such that > it better suites the need for dw_mmc, particulary around runtime PM. > > Do note, so far this is only compile tested. I would thus appreciate help in > testing and of course also in reviewing. > > > Ulf Hansson (5): > mmc: core: Prevent processing SDIO IRQs when none is claimed > mmc: sdio: Add API to manage SDIO IRQs from a workqueue > mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs > mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled > Revert "mmc: dw_mmc: Don't allow Runtime PM for SDIO cards" > > drivers/mmc/core/host.c | 2 ++ > drivers/mmc/core/sdio_irq.c | 27 +++++++++++++++++++++++++-- > drivers/mmc/core/sdio_ops.h | 2 ++ > drivers/mmc/host/dw_mmc.c | 44 ++++++++++++++++++++++++++++++++------------ > include/linux/mmc/host.h | 3 +++ > 5 files changed, 64 insertions(+), 14 deletions(-) On an rk3288-veyron-jerry system (with Marvell WiFi), I applied your series atop v4.11-rc8. I did basic tests and WiFi seemed to be working OK. I most certainly didn't stress things out, but doing basic transfers / pings worked. Oh, but maybe suspend / resume is a bit unhappy? It's a little hard to tell because it doesn't work 100% reliably (with respect to WiFi) even without your changes, but with your changes it seems to be broken worse. AKA: i can often get suspend/resume to work without your changes, but I've never seen it work with your changes. I get errors like: [ 36.498454] call ff0f0000.dwmmc+ returned 0 after 5 usecs [ 40.210761] Bluetooth: Host sleep enable command failed [ 40.216594] Bluetooth: HS not activated, suspend failed! [ 40.222537] dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns -16 [ 40.229923] call mmc1:0001:2+ returned -16 after 4919039 usecs [ 40.236442] PM: Device mmc1:0001:2 failed to suspend async: error -16 [ 45.330750] mwifiex_sdio mmc1:0001:1: mwifiex_cmd_timeout_func: Timeout cmd id = 0x107, act = 0x0 It appears that ("mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs") is the one causing it. It's plausible that dw_mmc is interacting with things in a bad way, though. I probably can't spend another few days debugging this this right now, though. :( Anyone else on this thread want to dig? If not, I might be able to come back to this in a bit... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 April 2017 at 23:26, Doug Anderson <dianders@google.com> wrote: > Ulf, > > On Wed, Apr 19, 2017 at 2:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> Changes in v2: >> - Folded in a new change, patch 1/5 to fix a race condition while >> processing SDIO IRQs. >> - Fixed review comments provided by Dough. >> - Updated change logs. >> - Small changes to how ->ack_sdio_irq() is being invoked, to simplify >> code. >> - Added a revert patch of the dw_mmc change for runtime PM issues, which >> was a quick fix for stable/fixes. >> >> Some general description of the series: >> >> Regressions for SDIO IRQs have been reported for dw_mmc, caused by enabling >> runtime PM support for it. >> >> This series extends and improves the SDIO IRQs support in the core, such that >> it better suites the need for dw_mmc, particulary around runtime PM. >> >> Do note, so far this is only compile tested. I would thus appreciate help in >> testing and of course also in reviewing. >> >> >> Ulf Hansson (5): >> mmc: core: Prevent processing SDIO IRQs when none is claimed >> mmc: sdio: Add API to manage SDIO IRQs from a workqueue >> mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs >> mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled >> Revert "mmc: dw_mmc: Don't allow Runtime PM for SDIO cards" >> >> drivers/mmc/core/host.c | 2 ++ >> drivers/mmc/core/sdio_irq.c | 27 +++++++++++++++++++++++++-- >> drivers/mmc/core/sdio_ops.h | 2 ++ >> drivers/mmc/host/dw_mmc.c | 44 ++++++++++++++++++++++++++++++++------------ >> include/linux/mmc/host.h | 3 +++ >> 5 files changed, 64 insertions(+), 14 deletions(-) > > On an rk3288-veyron-jerry system (with Marvell WiFi), I applied your > series atop v4.11-rc8. I did basic tests and WiFi seemed to be > working OK. I most certainly didn't stress things out, but doing > basic transfers / pings worked. Great, thanks for testing! > > Oh, but maybe suspend / resume is a bit unhappy? It's a little hard > to tell because it doesn't work 100% reliably (with respect to WiFi) > even without your changes, but with your changes it seems to be broken > worse. AKA: i can often get suspend/resume to work without your > changes, but I've never seen it work with your changes. I get errors > like: > > [ 36.498454] call ff0f0000.dwmmc+ returned 0 after 5 usecs > [ 40.210761] Bluetooth: Host sleep enable command failed > [ 40.216594] Bluetooth: HS not activated, suspend failed! > [ 40.222537] dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns -16 > [ 40.229923] call mmc1:0001:2+ returned -16 after 4919039 usecs > [ 40.236442] PM: Device mmc1:0001:2 failed to suspend async: error -16 > [ 45.330750] mwifiex_sdio mmc1:0001:1: mwifiex_cmd_timeout_func: > Timeout cmd id = 0x107, act = 0x0 > > > It appears that ("mmc: dw_mmc: Convert to use > MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs") is the one causing it. > It's plausible that dw_mmc is interacting with things in a bad way, > though. > > I probably can't spend another few days debugging this this right now, > though. :( Anyone else on this thread want to dig? If not, I might > be able to come back to this in a bit... I understand that this is hard to test, simply because the PM support for SDIO in general is fragile/broken. I am work on fixing this as well, however let's first fix the behavior for SDIO IRQs. :-) That said, your report provides me with valuable information. I believe the problem is that I naively thought the *system_freezable_wq*, would be suitable to manage SDIO IRQ. Of course, during suspend the SDIO func driver may decide to send commands to quiescence its device and those commands may trigger SDIO IRQs. Using the system_freezable_wq, makes those works that becomes scheduled to be put on hold. As simple test can you re-place "system_freezable_wq" with "system_wq" in sdio_signal_irq() (drivers/mmc/core/sdio_irq.c)? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, May 2, 2017 at 12:06 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 28 April 2017 at 23:26, Doug Anderson <dianders@google.com> wrote: >> Ulf, >> >> On Wed, Apr 19, 2017 at 2:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> Changes in v2: >>> - Folded in a new change, patch 1/5 to fix a race condition while >>> processing SDIO IRQs. >>> - Fixed review comments provided by Dough. >>> - Updated change logs. >>> - Small changes to how ->ack_sdio_irq() is being invoked, to simplify >>> code. >>> - Added a revert patch of the dw_mmc change for runtime PM issues, which >>> was a quick fix for stable/fixes. >>> >>> Some general description of the series: >>> >>> Regressions for SDIO IRQs have been reported for dw_mmc, caused by enabling >>> runtime PM support for it. >>> >>> This series extends and improves the SDIO IRQs support in the core, such that >>> it better suites the need for dw_mmc, particulary around runtime PM. >>> >>> Do note, so far this is only compile tested. I would thus appreciate help in >>> testing and of course also in reviewing. >>> >>> >>> Ulf Hansson (5): >>> mmc: core: Prevent processing SDIO IRQs when none is claimed >>> mmc: sdio: Add API to manage SDIO IRQs from a workqueue >>> mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs >>> mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled >>> Revert "mmc: dw_mmc: Don't allow Runtime PM for SDIO cards" >>> >>> drivers/mmc/core/host.c | 2 ++ >>> drivers/mmc/core/sdio_irq.c | 27 +++++++++++++++++++++++++-- >>> drivers/mmc/core/sdio_ops.h | 2 ++ >>> drivers/mmc/host/dw_mmc.c | 44 ++++++++++++++++++++++++++++++++------------ >>> include/linux/mmc/host.h | 3 +++ >>> 5 files changed, 64 insertions(+), 14 deletions(-) >> >> On an rk3288-veyron-jerry system (with Marvell WiFi), I applied your >> series atop v4.11-rc8. I did basic tests and WiFi seemed to be >> working OK. I most certainly didn't stress things out, but doing >> basic transfers / pings worked. > > Great, thanks for testing! > >> >> Oh, but maybe suspend / resume is a bit unhappy? It's a little hard >> to tell because it doesn't work 100% reliably (with respect to WiFi) >> even without your changes, but with your changes it seems to be broken >> worse. AKA: i can often get suspend/resume to work without your >> changes, but I've never seen it work with your changes. I get errors >> like: >> >> [ 36.498454] call ff0f0000.dwmmc+ returned 0 after 5 usecs >> [ 40.210761] Bluetooth: Host sleep enable command failed >> [ 40.216594] Bluetooth: HS not activated, suspend failed! >> [ 40.222537] dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns -16 >> [ 40.229923] call mmc1:0001:2+ returned -16 after 4919039 usecs >> [ 40.236442] PM: Device mmc1:0001:2 failed to suspend async: error -16 >> [ 45.330750] mwifiex_sdio mmc1:0001:1: mwifiex_cmd_timeout_func: >> Timeout cmd id = 0x107, act = 0x0 >> >> >> It appears that ("mmc: dw_mmc: Convert to use >> MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs") is the one causing it. >> It's plausible that dw_mmc is interacting with things in a bad way, >> though. >> >> I probably can't spend another few days debugging this this right now, >> though. :( Anyone else on this thread want to dig? If not, I might >> be able to come back to this in a bit... > > I understand that this is hard to test, simply because the PM support > for SDIO in general is fragile/broken. I am work on fixing this as > well, however let's first fix the behavior for SDIO IRQs. :-) > > That said, your report provides me with valuable information. I > believe the problem is that I naively thought the > *system_freezable_wq*, would be suitable to manage SDIO IRQ. Of > course, during suspend the SDIO func driver may decide to send > commands to quiescence its device and those commands may trigger SDIO > IRQs. Using the system_freezable_wq, makes those works that becomes > scheduled to be put on hold. > > As simple test can you re-place "system_freezable_wq" with "system_wq" > in sdio_signal_irq() (drivers/mmc/core/sdio_irq.c)? Yup, that worked. Nice! With that fix, you can add my Tested-by to this series. As I said, it wasn't a super deep test but at least the bits I tested work the same or better than they used to. ;) I was seeing some problems (with s2r and with ifconfig up/down) even without your patches and I see roughly the same level of problems now after your patches. Very possibly these are just issues with mwifiex upstream? I'll also look over the series one more time when you send it next, but I think I've looked at it enough by now for a Reviewed-by as well. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html