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

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

[Ulf Hansson]

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.

I am not sure this is material for stable, so perhaps we anyway should pick up
Dough's earlier quick fix for dw_mmc. Thoughts?


Ulf Hansson (3):
  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

 drivers/mmc/core/host.c     |  2 ++
 drivers/mmc/core/sdio_irq.c | 32 ++++++++++++++++++++++++++++++--
 drivers/mmc/core/sdio_ops.h |  2 ++
 drivers/mmc/host/dw_mmc.c   | 37 ++++++++++++++++++++++++++++++++++---
 include/linux/mmc/host.h    |  3 +++
 5 files changed, 71 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH 1/3] 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 this new feature,
however the feature is optional for already existing hosts suppporting
MMC_CAP2_SDIO_IRQ_NOTHREAD.

It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and
benefit from this feature. Further changes will have to tell. Until then
the old path using the kernel thread remains possible.

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

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 6d4b720..1b6006d 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -92,15 +92,43 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 	return ret;
 }
 
-void sdio_run_irqs(struct mmc_host *host)
+static void __sdio_run_irqs(struct mmc_host *host)
 {
-	mmc_claim_host(host);
 	host->sdio_irq_pending = true;
 	process_sdio_pending_irqs(host);
+}
+
+void sdio_run_irqs(struct mmc_host *host)
+{
+	mmc_claim_host(host);
+	__sdio_run_irqs(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);
+
+	mmc_claim_host(host);
+	__sdio_run_irqs(host);
+	if (host->ops->ack_sdio_irq)
+		host->ops->ack_sdio_irq(host);
+	mmc_release_host(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 2/3] 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 | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 249ded6..f086791 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1635,9 +1635,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;
@@ -1655,6 +1654,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);
@@ -1756,6 +1769,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,
@@ -2645,9 +2659,14 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
 				continue;
 
 			if (pending & SDMMC_INT_SDIO(slot->sdio_id)) {
+				u32 int_mask;
+
 				mci_writel(host, RINTSTS,
 					   SDMMC_INT_SDIO(slot->sdio_id));
-				mmc_signal_sdio_irq(slot->mmc);
+				int_mask = mci_readl(host, INTMASK);
+				int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id);
+				mci_writel(host, INTMASK, int_mask);
+				sdio_signal_irq(slot->mmc);
 			}
 		}
 
@@ -2748,6 +2767,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 3/3] 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 f086791..4256957 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -38,6 +38,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"
 
@@ -1657,8 +1658,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

Re: [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs

[Doug Anderson]

Hi,

On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 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 | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 249ded6..f086791 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1635,9 +1635,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;
> @@ -1655,6 +1654,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);

I have some slight paranoia that some code out there might decide to
call enable_sdio_irq(0) while an interrupt is being processed.  In
that case we'll be turning interrupts back on here.  It seems like it
would be "better safe than sorry" to keep track of the "enabled /
disabled" state somewhere.  ...and when we "unmask" we treat it as a
no-op if the interrupt is currently disabled.

> +}
> +
>  static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  {
>         struct dw_mci_slot *slot = mmc_priv(mmc);
> @@ -1756,6 +1769,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,
> @@ -2645,9 +2659,14 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>                                 continue;
>
>                         if (pending & SDMMC_INT_SDIO(slot->sdio_id)) {
> +                               u32 int_mask;
> +
>                                 mci_writel(host, RINTSTS,
>                                            SDMMC_INT_SDIO(slot->sdio_id));
> -                               mmc_signal_sdio_irq(slot->mmc);
> +                               int_mask = mci_readl(host, INTMASK);
> +                               int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id);
> +                               mci_writel(host, INTMASK, int_mask);

Seems like you should be calling "__dw_mci_enable_sdio_irq(slot, 0)"
here.  Specifically the interrupt handler won't have the spinlock so
you're doing an unsafe read/modify/write here.  Yeah, the spinlock is
kinda silly in dw_mmc.  Really the whole interrupt handling needs to
be re-done to use a threaded IRQ instead of the current solution...

> +                               sdio_signal_irq(slot->mmc);
>                         }
>                 }
>
> @@ -2748,6 +2767,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
>

Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue

[Doug Anderson]

Hi,

On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 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 this new feature,
> however the feature is optional for already existing hosts suppporting
> MMC_CAP2_SDIO_IRQ_NOTHREAD.
>
> It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and
> benefit from this feature. Further changes will have to tell. Until then
> the old path using the kernel thread remains possible.

So one other subtle problem with the new approach is that you totally
lose all of the polling logic in sdio_irq_thread().

...so if I take your series and then comment out "cap-sdio-irq;" in
the veyron "dtsi" then things stop working.  Right now dw_mmc only
enables SDIO Interrupts if that bit is set and relies on polling
otherwise.  Presumably there's not a _huge_ reason why you would need
to make dw_mmc work without actual SDIO IRQ signaling, but the way the
code is structured right now things will probably break for some users
out there.

One note is that I remember on exynos5250-snow that we needed to
enable a hybrid interrupt/polling mechanism.  The problem we ran into
was terribly rare and it was never root caused if there was just some
subtle bug or if certain versions of dw_mmc sometimes just dropped
interrupts (and the patch was never upstreamed), so possibly we don't
care.  ...but having the polling code there as a fallback seems like
it could have a benefit.

Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue

[Ulf Hansson]

On 18 April 2017 at 23:43, Doug Anderson <dianders@google.com> wrote:
> Hi,
>
> On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> 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 this new feature,
>> however the feature is optional for already existing hosts suppporting
>> MMC_CAP2_SDIO_IRQ_NOTHREAD.
>>
>> It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and
>> benefit from this feature. Further changes will have to tell. Until then
>> the old path using the kernel thread remains possible.
>
> So one other subtle problem with the new approach is that you totally
> lose all of the polling logic in sdio_irq_thread().

The polling is still there, as I haven't removed the kthread in this series.

I was also thinking of the next step, which could move the polling
inside the work, simply by re-schedule itself.

>
> ...so if I take your series and then comment out "cap-sdio-irq;" in
> the veyron "dtsi" then things stop working.  Right now dw_mmc only
> enables SDIO Interrupts if that bit is set and relies on polling
> otherwise.  Presumably there's not a _huge_ reason why you would need
> to make dw_mmc work without actual SDIO IRQ signaling, but the way the
> code is structured right now things will probably break for some users
> out there.

Did you actually test this or the conclusion was theoretical?

I *was* actually thinking of the polling case and I think it should be
addressed, unless I am missing something of course.

More precisely, in patch2, I make sure MMC_CAP2_SDIO_IRQ_NOTHREAD
becomes set for dw_mmc, only *if* MMC_CAP_SDIO_IRQ is set. That means
the polling with the kthread is done for cases when MMC_CAP_SDIO_IRQ
is unset. Right!?

>
> One note is that I remember on exynos5250-snow that we needed to
> enable a hybrid interrupt/polling mechanism.  The problem we ran into
> was terribly rare and it was never root caused if there was just some
> subtle bug or if certain versions of dw_mmc sometimes just dropped
> interrupts (and the patch was never upstreamed), so possibly we don't
> care.  ...but having the polling code there as a fallback seems like
> it could have a benefit.

I see. To be clear, removing the polling is not my intent and isn't
what the series tries to do.

Kind regards
Uffe

Re: [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs

[Ulf Hansson]

On 18 April 2017 at 23:25, Doug Anderson <dianders@google.com> wrote:
> Hi,
>
> On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> 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 | 29 ++++++++++++++++++++++++++---
>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 249ded6..f086791 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -1635,9 +1635,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;
>> @@ -1655,6 +1654,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);
>
> I have some slight paranoia that some code out there might decide to
> call enable_sdio_irq(0) while an interrupt is being processed.  In
> that case we'll be turning interrupts back on here.  It seems like it
> would be "better safe than sorry" to keep track of the "enabled /
> disabled" state somewhere.  ...and when we "unmask" we treat it as a
> no-op if the interrupt is currently disabled.

I understand your concern and your paranoia, which probably relates to
the current tricky code that involves running our own kthread in
sdio_irq_thread(). :-)

For example, the sdio_irq_thread() need to release the host,
mmc_release_host(), before it invokes ->enable_sdio_irq(), which is
after it has processed the SDIO IRQs. This is actually wrong, as host
drivers expects the host to be claimed when any of the host ops
callbacks are being invoked, particularly from runtime PM point of
view.

Anyway, the current code *seems* to work - but for sure it's fragile
and it has been so for too long.

That said, you have a point about keeping track of the enabled/disable
state. However, by digging a bit deeper into this, I realized the
problem is actually even worse. Let me explain a bit more:

->ack_sdio_irq() is *only* called from the work that processes the
SDIO IRQ. The difference compared to kthread is that the host is being
claimed throughout the entire process when using the work, which by
itself is an improvement. This also means, that the only reason to why
->enable_sdio_irq(0) can be called, is because an SDIO func driver
decides to release the SDIO IRQ. However, for it to do that, it must
first claim the host.

This leads us to two scenarios:
1) The work manages to claim the host before the SDIO func driver.
Then everything should be fine, simply because the work processes and
acks the IRQ, before the SDIO func driver gets permission to release
it.

2) The SDIO func driver gets to claim the host before the work. That
means it releases the IRQ before the work gets permission to run and
process the IRQ. This means we are into trouble. Not only as you say,
->enable_sdio_irq(0) becomes called before ->ack_sdio_irq(), but the
actual processing of the IRQ, mmc_io_rw_direct() etc, becomes executed
when it shouldn't.

So, to fix the problems, I think a better solution than keeping track
of the enabled/disabled state, is to actually prevent the IRQ from
being processed in scenario 2. Including to prevent invoking
->ack_sdio_irq() from the work.

Allow me to cook a separate patch for this, because I think this is
already an existing problem when using MMC_CAP2_SDIO_IRQ_NOTHREAD.

>
>> +}
>> +
>>  static int dw_mci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>  {
>>         struct dw_mci_slot *slot = mmc_priv(mmc);
>> @@ -1756,6 +1769,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,
>> @@ -2645,9 +2659,14 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
>>                                 continue;
>>
>>                         if (pending & SDMMC_INT_SDIO(slot->sdio_id)) {
>> +                               u32 int_mask;
>> +
>>                                 mci_writel(host, RINTSTS,
>>                                            SDMMC_INT_SDIO(slot->sdio_id));
>> -                               mmc_signal_sdio_irq(slot->mmc);
>> +                               int_mask = mci_readl(host, INTMASK);
>> +                               int_mask &= ~SDMMC_INT_SDIO(slot->sdio_id);
>> +                               mci_writel(host, INTMASK, int_mask);
>
> Seems like you should be calling "__dw_mci_enable_sdio_irq(slot, 0)"
> here.  Specifically the interrupt handler won't have the spinlock so
> you're doing an unsafe read/modify/write here.  Yeah, the spinlock is
> kinda silly in dw_mmc.  Really the whole interrupt handling needs to
> be re-done to use a threaded IRQ instead of the current solution...

Thanks, this make sense! New version on its way.

>
>> +                               sdio_signal_irq(slot->mmc);
>>                         }
>>                 }
>>
>> @@ -2748,6 +2767,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
>>

Kind regards
Uffe

Re: [PATCH 2/3] mmc: dw_mmc: Convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs

[Doug Anderson]

Hi,

On Wed, Apr 19, 2017 at 5:10 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 18 April 2017 at 23:25, Doug Anderson <dianders@google.com> wrote:
>> Hi,
>>
>> On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> 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 | 29 ++++++++++++++++++++++++++---
>>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 249ded6..f086791 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -1635,9 +1635,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;
>>> @@ -1655,6 +1654,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);
>>
>> I have some slight paranoia that some code out there might decide to
>> call enable_sdio_irq(0) while an interrupt is being processed.  In
>> that case we'll be turning interrupts back on here.  It seems like it
>> would be "better safe than sorry" to keep track of the "enabled /
>> disabled" state somewhere.  ...and when we "unmask" we treat it as a
>> no-op if the interrupt is currently disabled.
>
> I understand your concern and your paranoia, which probably relates to
> the current tricky code that involves running our own kthread in
> sdio_irq_thread(). :-)
>
> For example, the sdio_irq_thread() need to release the host,
> mmc_release_host(), before it invokes ->enable_sdio_irq(), which is
> after it has processed the SDIO IRQs. This is actually wrong, as host
> drivers expects the host to be claimed when any of the host ops
> callbacks are being invoked, particularly from runtime PM point of
> view.

Yeah, I remember that causing problems in the past...  ...but in
general we can't assume that the host is claimed in enable_sdio_irq()
because (historically) it's called directly from an IRQ.  We can't
claim the host from the IRQ..


> Anyway, the current code *seems* to work - but for sure it's fragile
> and it has been so for too long.
>
> That said, you have a point about keeping track of the enabled/disable
> state. However, by digging a bit deeper into this, I realized the
> problem is actually even worse. Let me explain a bit more:
>
> ->ack_sdio_irq() is *only* called from the work that processes the
> SDIO IRQ. The difference compared to kthread is that the host is being
> claimed throughout the entire process when using the work, which by
> itself is an improvement. This also means, that the only reason to why
> ->enable_sdio_irq(0) can be called, is because an SDIO func driver
> decides to release the SDIO IRQ. However, for it to do that, it must
> first claim the host.

It took me a little while to understand this, but I think you're
talking about my paranoia case of the func driver tries to call
sdio_release_irq() while an interrupt is pending?  That could
effectively call enable_sdio_irq(0).  ...and if the work hasn't
processed yet then we'll be in trouble.


> This leads us to two scenarios:
> 1) The work manages to claim the host before the SDIO func driver.
> Then everything should be fine, simply because the work processes and
> acks the IRQ, before the SDIO func driver gets permission to release
> it.
>
> 2) The SDIO func driver gets to claim the host before the work. That
> means it releases the IRQ before the work gets permission to run and
> process the IRQ. This means we are into trouble. Not only as you say,
> ->enable_sdio_irq(0) becomes called before ->ack_sdio_irq(), but the
> actual processing of the IRQ, mmc_io_rw_direct() etc, becomes executed
> when it shouldn't.
>
> So, to fix the problems, I think a better solution than keeping track
> of the enabled/disabled state, is to actually prevent the IRQ from
> being processed in scenario 2. Including to prevent invoking
> ->ack_sdio_irq() from the work.
>
> Allow me to cook a separate patch for this, because I think this is
> already an existing problem when using MMC_CAP2_SDIO_IRQ_NOTHREAD.

Yeah, you're right that there could be more serious problems here if a
host releases the IRQ while it's pending.

Even with the fixes it still makes me nervous that we could be mixed
up.  If it were up to me I'd love to see at least some sort of warning
if you "acked" a disabled interrupt, but I won't push for it if nobody
else agrees.


-Doug

Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue

[Doug Anderson]

Hi,

On Wed, Apr 19, 2017 at 3:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 18 April 2017 at 23:43, Doug Anderson <dianders@google.com> wrote:
>> Hi,
>>
>> On Tue, Apr 18, 2017 at 5:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> 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 this new feature,
>>> however the feature is optional for already existing hosts suppporting
>>> MMC_CAP2_SDIO_IRQ_NOTHREAD.
>>>
>>> It's likely that all host can convert to use MMC_CAP2_SDIO_IRQ_NOTHREAD and
>>> benefit from this feature. Further changes will have to tell. Until then
>>> the old path using the kernel thread remains possible.
>>
>> So one other subtle problem with the new approach is that you totally
>> lose all of the polling logic in sdio_irq_thread().
>
> The polling is still there, as I haven't removed the kthread in this series.

The code still exists, but it won't be called, right?

Oh!  Shoot, I see that you only enable the new code in dw_mmc when the
cap is set.  Hrm.


> I was also thinking of the next step, which could move the polling
> inside the work, simply by re-schedule itself.
>
>>
>> ...so if I take your series and then comment out "cap-sdio-irq;" in
>> the veyron "dtsi" then things stop working.  Right now dw_mmc only
>> enables SDIO Interrupts if that bit is set and relies on polling
>> otherwise.  Presumably there's not a _huge_ reason why you would need
>> to make dw_mmc work without actual SDIO IRQ signaling, but the way the
>> code is structured right now things will probably break for some users
>> out there.
>
> Did you actually test this or the conclusion was theoretical?

I did, but I had confirmation bias so upon the first sign of failure I
decided "I must be right--it doesn't work".  :(  Maybe something else
was causing problems.  Trying again now.

OK, let's see:

With "cap-sdio-irq" commented out but without your 3 patches:
=> Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8".  Seems OK.

With "cap-sdio-irq" commented out but _with_ your 3 patches:
=> Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8".  Seems OK.

So I guess the conclusion is that I missed the part about your patch
only enabling the new features if MMC_CAP_SDIO_IRQ.  Sorry.  :(
...and then I must have hit some other unrelated failure that I can't
reproduce now and assumed it was your patch's fault.

So basically I would say that I've lightly tested your code.  It's not
code I've stressed a ton, but it survived some basic tests anyway...
:)  The code also looks pretty sane to me.


> I *was* actually thinking of the polling case and I think it should be
> addressed, unless I am missing something of course.
>
> More precisely, in patch2, I make sure MMC_CAP2_SDIO_IRQ_NOTHREAD
> becomes set for dw_mmc, only *if* MMC_CAP_SDIO_IRQ is set. That means
> the polling with the kthread is done for cases when MMC_CAP_SDIO_IRQ
> is unset. Right!?

Yeah, looks right to me now that I have my glasses on.


>> One note is that I remember on exynos5250-snow that we needed to
>> enable a hybrid interrupt/polling mechanism.  The problem we ran into
>> was terribly rare and it was never root caused if there was just some
>> subtle bug or if certain versions of dw_mmc sometimes just dropped
>> interrupts (and the patch was never upstreamed), so possibly we don't
>> care.  ...but having the polling code there as a fallback seems like
>> it could have a benefit.
>
> I see. To be clear, removing the polling is not my intent and isn't
> what the series tries to do.

OK, makes sense.  Just figured I'd mention this in case you had future
plans around this code.  :)

Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue

[Ulf Hansson]

[...]

>
> With "cap-sdio-irq" commented out but without your 3 patches:
> => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8".  Seems OK.
>
> With "cap-sdio-irq" commented out but _with_ your 3 patches:
> => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8".  Seems OK.
>
> So I guess the conclusion is that I missed the part about your patch
> only enabling the new features if MMC_CAP_SDIO_IRQ.  Sorry.  :(
> ...and then I must have hit some other unrelated failure that I can't
> reproduce now and assumed it was your patch's fault.
>
> So basically I would say that I've lightly tested your code.  It's not
> code I've stressed a ton, but it survived some basic tests anyway...
> :)  The code also looks pretty sane to me.

Thanks a lot for your feedback a for running a new round of tests.
This seems promising then!

When you have the time, it we awesome if you could run yet another new
round of test with the new version of the series. I posted it
yesterday evening my local time.

I would also be very interested to know if converting to the work
queue approach has any impact on throughput. Maybe you have some
simple test suite to also verify that?

[...]

Kind regards
Uffe

Re: [PATCH 1/3] mmc: sdio: Add API to manage SDIO IRQs from a workqueue

[Doug Anderson]

Ulf

On Thu, Apr 20, 2017 at 5:14 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> [...]
>
>>
>> With "cap-sdio-irq" commented out but without your 3 patches:
>> => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8".  Seems OK.
>>
>> With "cap-sdio-irq" commented out but _with_ your 3 patches:
>> => Boot up, "ifconfig eth0 down; ping -c 20 8.8.8.8".  Seems OK.
>>
>> So I guess the conclusion is that I missed the part about your patch
>> only enabling the new features if MMC_CAP_SDIO_IRQ.  Sorry.  :(
>> ...and then I must have hit some other unrelated failure that I can't
>> reproduce now and assumed it was your patch's fault.
>>
>> So basically I would say that I've lightly tested your code.  It's not
>> code I've stressed a ton, but it survived some basic tests anyway...
>> :)  The code also looks pretty sane to me.
>
> Thanks a lot for your feedback a for running a new round of tests.
> This seems promising then!
>
> When you have the time, it we awesome if you could run yet another new
> round of test with the new version of the series. I posted it
> yesterday evening my local time.

Sorry for taking so long to get to this.  I've done this now and
things seem to be working fine with your new series.  As with before,
I'm not doing really stressful testing here, but what I've done seems
OK.


> I would also be very interested to know if converting to the work
> queue approach has any impact on throughput. Maybe you have some
> simple test suite to also verify that?

I don't personally.  To really get something repeatable for WiFi, you
need things in a chamber and need a test server setup.

I did this quick test with the device sitting on my desk (with
whatever the closest WiFi access point was), though:

ifconfig eth0 down
for i in $(seq 5); do
  curl http://SomeBigFile > /dev/null
done


>From this very simple test, the performance looks like a wash.

--

Before your changes:

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 54.0M  100 54.0M    0     0  2753k      0  0:00:20  0:00:20 --:--:-- 2952k
100 54.0M  100 54.0M    0     0  1073k      0  0:00:51  0:00:51 --:--:--  902k
100 54.0M  100 54.0M    0     0  2494k      0  0:00:22  0:00:22 --:--:-- 3061k
100 54.0M  100 54.0M    0     0  2455k      0  0:00:22  0:00:22 --:--:-- 2890k
100 54.0M  100 54.0M    0     0  1934k      0  0:00:28  0:00:28 --:--:-- 2301k

After your changes:

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 54.0M  100 54.0M    0     0  2376k      0  0:00:23  0:00:23 --:--:-- 2739k
100 54.0M  100 54.0M    0     0  2303k      0  0:00:24  0:00:24 --:--:-- 2698k
100 54.0M  100 54.0M    0     0  2512k      0  0:00:22  0:00:22 --:--:-- 2359k
100 54.0M  100 54.0M    0     0  1774k      0  0:00:31  0:00:31 --:--:-- 1742k
100 54.0M  100 54.0M    0     0  2124k      0  0:00:26  0:00:26 --:--:-- 2753k

--

It's possible that Brian Norris (CCed) might have an easy way to do a
better test?

-Doug