[PATCH v2 0/5] mmc: Improve/fix support for SDIO IRQs

[PATCH v2 0/5] mmc: Improve/fix support for SDIO IRQs

[Ulf Hansson]

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(-)

-- 
2.7.4

[PATCH v2 1/5] mmc: core: Prevent processing SDIO IRQs when none is claimed

[Ulf Hansson]

In cases when MMC_CAP2_SDIO_IRQ_NOTHREAD is set, there is a minor window
for when the mmc host could call sdio_run_irqs(), while in fact an SDIO
func driver could have decided to released the SDIO IRQ via a call to
sdio_release_irq(). In this scenario, processing of the SDIO IRQs are done
even if there is none IRQ claimed, which is not what we want.

To prevent this from happen, close the window by validating that at least
one SDIO IRQs is claimed, before deciding to process them.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_irq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 6d4b720..44d9c86 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -95,8 +95,10 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 void sdio_run_irqs(struct mmc_host *host)
 {
 	mmc_claim_host(host);
-	host->sdio_irq_pending = true;
-	process_sdio_pending_irqs(host);
+	if (host->sdio_irqs) {
+		host->sdio_irq_pending = true;
+		process_sdio_pending_irqs(host);
+	}
 	mmc_release_host(host);
 }
 EXPORT_SYMBOL_GPL(sdio_run_irqs);
-- 
2.7.4

[PATCH v2 2/5] mmc: sdio: Add API to manage SDIO IRQs from a workqueue

[Ulf Hansson]

For hosts not supporting MMC_CAP2_SDIO_IRQ_NOTHREAD but MMC_CAP_SDIO_IRQ,
the SDIO IRQs are processed from a dedicated kernel thread. For these
cases, the host calls mmc_signal_sdio_irq() from its ISR to signal a new
SDIO IRQ.

Signaling an SDIO IRQ makes the host's ->enable_sdio_irq() callback to be
invoked to temporary disable the IRQs, before the kernel thread is woken up
to process it. When processing of the IRQs are completed, they are
re-enabled by the kernel thread, again via invoking the host's
->enable_sdio_irq().

The observation from this, is that the execution path is being unnecessary
complex, as the host driver already knows that it needs to temporary
disable the IRQs before signaling a new one. Moreover, replacing the kernel
thread with a work/workqueue would greatly simplify the code.

To address the above problems, let's continue to build upon the support for
MMC_CAP2_SDIO_IRQ_NOTHREAD, as it already implements SDIO IRQs to be
processed without using the clumsy kernel thread, but it also avoids the
ping-ponging calls of the host's ->enable_sdio_irq() callback for each
processed IRQ.

Therefore, let's add new API sdio_signal_irq(), which enables hosts to
signal/process SDIO IRQs by using a work/workqueue, rather than using the
kernel thread.

Add also a new host callback ->ack_sdio_irq(), which the work invokes when
the SDIO IRQs are processed. This informs the host about when it can
re-enable the SDIO IRQs. Potentially, we could re-use the existing
->enable_sdio_irq() callback for this matter, however it has turned out
that it's more convenient for hosts to get this information via a separate
callback.

Hosts needs to enable MMC_CAP2_SDIO_IRQ_NOTHREAD to use the work/workqueue
to process SDIO IRQs, however implementing the ->ack_sdio_irq() is optional.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/host.c     |  2 ++
 drivers/mmc/core/sdio_irq.c | 21 +++++++++++++++++++++
 drivers/mmc/core/sdio_ops.h |  2 ++
 include/linux/mmc/host.h    |  3 +++
 4 files changed, 28 insertions(+)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 3f8c85d..77058cb 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -30,6 +30,7 @@
 #include "host.h"
 #include "slot-gpio.h"
 #include "pwrseq.h"
+#include "sdio_ops.h"
 
 #define cls_dev_to_mmc_host(d)	container_of(d, struct mmc_host, class_dev)
 
@@ -379,6 +380,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 	spin_lock_init(&host->lock);
 	init_waitqueue_head(&host->wq);
 	INIT_DELAYED_WORK(&host->detect, mmc_rescan);
+	INIT_WORK(&host->sdio_irq_work, sdio_irq_work);
 	setup_timer(&host->retune_timer, mmc_retune_timer, (unsigned long)host);
 
 	/*
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 44d9c86..8e28759 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -98,11 +98,32 @@ void sdio_run_irqs(struct mmc_host *host)
 	if (host->sdio_irqs) {
 		host->sdio_irq_pending = true;
 		process_sdio_pending_irqs(host);
+		if (host->ops->ack_sdio_irq)
+			host->ops->ack_sdio_irq(host);
 	}
 	mmc_release_host(host);
 }
 EXPORT_SYMBOL_GPL(sdio_run_irqs);
 
+void sdio_irq_work(struct work_struct *work)
+{
+	struct mmc_host *host =
+		container_of(work, struct mmc_host, sdio_irq_work);
+
+	sdio_run_irqs(host);
+}
+
+void sdio_signal_irq(struct mmc_host *host)
+{
+	/*
+	 * The system_freezable_wq helps us to avoid processing IRQs while being
+	 * system PM suspended. Instead these IRQs becomes deferred and managed
+	 * when userspace is unfrozen.
+	 */
+	queue_work(system_freezable_wq, &host->sdio_irq_work);
+}
+EXPORT_SYMBOL_GPL(sdio_signal_irq);
+
 static int sdio_irq_thread(void *_host)
 {
 	struct mmc_host *host = _host;
diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
index bed8a83..836e405 100644
--- a/drivers/mmc/core/sdio_ops.h
+++ b/drivers/mmc/core/sdio_ops.h
@@ -17,6 +17,7 @@
 
 struct mmc_host;
 struct mmc_card;
+struct work_struct;
 
 int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);
 int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn,
@@ -25,6 +26,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 	unsigned addr, int incr_addr, u8 *buf, unsigned blocks, unsigned blksz);
 int sdio_reset(struct mmc_host *host);
 unsigned int mmc_align_data_size(struct mmc_card *card, unsigned int sz);
+void sdio_irq_work(struct work_struct *work);
 
 static inline bool mmc_is_io_op(u32 opcode)
 {
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 21385ac..f03df539 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -130,6 +130,7 @@ struct mmc_host_ops {
 	int	(*get_cd)(struct mmc_host *host);
 
 	void	(*enable_sdio_irq)(struct mmc_host *host, int enable);
+	void	(*ack_sdio_irq)(struct mmc_host *host);
 
 	/* optional callback for HC quirks */
 	void	(*init_card)(struct mmc_host *host, struct mmc_card *card);
@@ -358,6 +359,7 @@ struct mmc_host {
 
 	unsigned int		sdio_irqs;
 	struct task_struct	*sdio_irq_thread;
+	struct work_struct	sdio_irq_work;
 	bool			sdio_irq_pending;
 	atomic_t		sdio_irq_thread_abort;
 
@@ -428,6 +430,7 @@ static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 }
 
 void sdio_run_irqs(struct mmc_host *host);
+void sdio_signal_irq(struct mmc_host *host);
 
 #ifdef CONFIG_REGULATOR
 int mmc_regulator_get_ocrmask(struct regulator *supply);
-- 
2.7.4

[PATCH v2 3/5] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs

[Ulf Hansson]

Convert to use the more lightweight method for processing SDIO IRQs, which
involves the following changes:

- Enable MMC_CAP2_SDIO_IRQ_NOTHREAD when SDIO IRQ is supported.
- Mask SDIO IRQ when signaling it for processing.
- Re-enable (unmask) the SDIO IRQ from the ->ack_sdio_irq() callback.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/dw_mmc.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index e45129f..635d76c 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1642,9 +1642,8 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
 	}
 }
 
-static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
+static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, int enb)
 {
-	struct dw_mci_slot *slot = mmc_priv(mmc);
 	struct dw_mci *host = slot->host;
 	unsigned long irqflags;
 	u32 int_mask;
@@ -1662,6 +1661,20 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 	spin_unlock_irqrestore(&host->irq_lock, irqflags);
 }
 
+static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
+{
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+
+	__dw_mci_enable_sdio_irq(slot, enb);
+}
+
+static void dw_mci_ack_sdio_irq(struct mmc_host *mmc)
+{
+	struct dw_mci_slot *slot = mmc_priv(mmc);
+
+	__dw_mci_enable_sdio_irq(slot, 1);
+}
+
 static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct dw_mci_slot *slot = mmc_priv(mmc);
@@ -1763,6 +1776,7 @@ static const struct mmc_host_ops dw_mci_ops = {
 	.get_cd			= dw_mci_get_cd,
 	.hw_reset               = dw_mci_hw_reset,
 	.enable_sdio_irq	= dw_mci_enable_sdio_irq,
+	.ack_sdio_irq		= dw_mci_ack_sdio_irq,
 	.execute_tuning		= dw_mci_execute_tuning,
 	.card_busy		= dw_mci_card_busy,
 	.start_signal_voltage_switch = dw_mci_switch_voltage,
@@ -2654,7 +2668,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
 			if (pending & SDMMC_INT_SDIO(slot->sdio_id)) {
 				mci_writel(host, RINTSTS,
 					   SDMMC_INT_SDIO(slot->sdio_id));
-				mmc_signal_sdio_irq(slot->mmc);
+				__dw_mci_enable_sdio_irq(slot, 0);
+				sdio_signal_irq(slot->mmc);
 			}
 		}
 
@@ -2755,6 +2770,10 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 	if (ret)
 		goto err_host_allocated;
 
+	/* Process SDIO IRQs through the sdio_irq_work. */
+	if (mmc->caps & MMC_CAP_SDIO_IRQ)
+		mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
+
 	/* Useful defaults if platform data is unset. */
 	if (host->use_dma == TRANS_MODE_IDMAC) {
 		mmc->max_segs = host->ring_size;
-- 
2.7.4

[PATCH v2 4/5] mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs are enabled

[Ulf Hansson]

To be able to handle SDIO IRQs the dw_mmc device needs to be powered and
providing clock to the SDIO card. Therefore, we must not allow the device
to be runtime PM suspended while SDIO IRQs are enabled.

To fix this, let's increase the runtime PM usage count while the mmc core
enables SDIO IRQs. Later when the mmc core tells dw_mmc to disable SDIO
IRQs, we drop the usage count to again allow runtime PM suspend.

This now becomes the default behaviour for dw_mmc. In cases where SDIO IRQs
can be re-routed as GPIO wake-ups during runtime PM suspend, one could
potentially allow runtime PM suspend. However, that will have to be
addressed as a separate change on top of this one.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/dw_mmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 635d76c..c14a36b 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -39,6 +39,7 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/mmc/slot-gpio.h>
+#include <linux/pm_runtime.h>
 
 #include "dw_mmc.h"
 
@@ -1664,8 +1665,15 @@ static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, int enb)
 static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, int enb)
 {
 	struct dw_mci_slot *slot = mmc_priv(mmc);
+	struct dw_mci *host = slot->host;
 
 	__dw_mci_enable_sdio_irq(slot, enb);
+
+	/* Avoid runtime suspending the device when SDIO IRQ is enabled */
+	if (enb)
+		pm_runtime_get_noresume(host->dev);
+	else
+		pm_runtime_put_noidle(host->dev);
 }
 
 static void dw_mci_ack_sdio_irq(struct mmc_host *mmc)
-- 
2.7.4

[PATCH v2 5/5] Revert "mmc: dw_mmc: Don't allow Runtime PM for SDIO cards"

[Ulf Hansson]

This reverts commit a6db2c86033bc41329770e90c20d4f1fec3824e4.

As dw_mmc now is capable of preventing runtime PM suspend while SDIO IRQs
are enabled, let's drop the less fine-grained approach of preventing
runtime PM suspend for all SDIO cards.

In this way we wont keep the host runtime PM resumed, unless really needed
and thus avoiding to waste power.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/dw_mmc.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index c14a36b..c95dc9b 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -23,7 +23,6 @@
 #include <linux/ioport.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/stat.h>
@@ -1622,16 +1621,10 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct mmc_card *card)
 
 		if (card->type == MMC_TYPE_SDIO ||
 		    card->type == MMC_TYPE_SD_COMBO) {
-			if (!test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
-				pm_runtime_get_noresume(mmc->parent);
-				set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-			}
+			set_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
 			clk_en_a = clk_en_a_old & ~clken_low_pwr;
 		} else {
-			if (test_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags)) {
-				pm_runtime_put_noidle(mmc->parent);
-				clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
-			}
+			clear_bit(DW_MMC_CARD_NO_LOW_PWR, &slot->flags);
 			clk_en_a = clk_en_a_old | clken_low_pwr;
 		}
 
-- 
2.7.4

Re: [PATCH v2 0/5] mmc: Improve/fix support for SDIO IRQs

[Doug Anderson]

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

Re: [PATCH v2 0/5] mmc: Improve/fix support for SDIO IRQs

[Ulf Hansson]

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

Re: [PATCH v2 0/5] mmc: Improve/fix support for SDIO IRQs

[Doug Anderson]

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