[PATCH 0/8] sdhci: Move real work out of an atomic context

[PATCH 0/8] sdhci: Move real work out of an atomic context

[Anton Vorontsov]

Hi all,

Currently the sdhci driver does everything in the atomic context.
And what is worse, PIO transfers are made from the IRQ handler.

This causes huge latencies (up to 120 ms). On some P2020 SOCs,
DMA and card detection is broken, which means that kernel polls
for the card via PIO transfers every second. Needless to say
that this is quite bad.

So, this patch set reworks sdhci code to avoid atomic context,
almost completely. We only do two device memory operations
in the atomic context, and all the rest is threaded.

I noticed no throughput drop neither with PIO transfers nor
with DMA (tested on MPC8569E CPU), while latencies should be
greatly improved.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 1/8] sdhci: Turn timeout timer into delayed work

[Anton Vorontsov]

There is no need for the timeout handler to run in the atomic
context, so this patch turns timeout timeout into the delayed
work.

Note that the timeout handler still grabs an irqsave spinlock,
we'll deal with it in a separate patch.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/mmc/host/sdhci.c |   14 ++++++++------
 drivers/mmc/host/sdhci.h |    3 ++-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c6d1bd8..dc6328c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -13,6 +13,8 @@
  *     - JMicron (hardware and technical support)
  */
 
+#include <linux/kernel.h>
+#include <linux/workqueue.h>
 #include <linux/delay.h>
 #include <linux/highmem.h>
 #include <linux/io.h>
@@ -906,7 +908,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
-	mod_timer(&host->timer, jiffies + 10 * HZ);
+	schedule_delayed_work(&host->timeout_work, 10 * HZ);
 
 	host->cmd = cmd;
 
@@ -1280,7 +1282,7 @@ static void sdhci_tasklet_finish(unsigned long param)
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	del_timer(&host->timer);
+	__cancel_delayed_work(&host->timeout_work);
 
 	mrq = host->mrq;
 
@@ -1324,12 +1326,12 @@ static void sdhci_tasklet_finish(unsigned long param)
 	mmc_request_done(host->mmc, mrq);
 }
 
-static void sdhci_timeout_timer(unsigned long data)
+static void sdhci_timeout_work(struct work_struct *wk)
 {
 	struct sdhci_host *host;
 	unsigned long flags;
 
-	host = (struct sdhci_host*)data;
+	host = container_of(wk, struct sdhci_host, timeout_work.work);
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1877,7 +1879,7 @@ int sdhci_add_host(struct sdhci_host *host)
 	tasklet_init(&host->finish_tasklet,
 		sdhci_tasklet_finish, (unsigned long)host);
 
-	setup_timer(&host->timer, sdhci_timeout_timer, (unsigned long)host);
+	INIT_DELAYED_WORK(&host->timeout_work, sdhci_timeout_work);
 
 	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
 		mmc_hostname(mmc), host);
@@ -1963,7 +1965,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	free_irq(host->irq, host);
 
-	del_timer_sync(&host->timer);
+	flush_delayed_work(&host->timeout_work);
 
 	tasklet_kill(&host->card_tasklet);
 	tasklet_kill(&host->finish_tasklet);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index c846813..55c114d 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -12,6 +12,7 @@
 #define __SDHCI_H
 
 #include <linux/scatterlist.h>
+#include <linux/workqueue.h>
 #include <linux/compiler.h>
 #include <linux/types.h>
 #include <linux/io.h>
@@ -290,7 +291,7 @@ struct sdhci_host {
 	struct tasklet_struct	card_tasklet;	/* Tasklet structures */
 	struct tasklet_struct	finish_tasklet;
 
-	struct timer_list	timer;		/* Timer for timeouts */
+	struct delayed_work	timeout_work;	/* Work for timeouts */
 
 	unsigned long		private[0] ____cacheline_aligned;
 };
-- 
1.7.0.5

[PATCH 2/8] sdhci: Use work structs instead of tasklets

[Anton Vorontsov]

The driver can happily live without an atomic context and tasklets,
so turn the tasklets into the work structs.

Tasklets handlers still grab irqsave spinlocks, but we'll deal
with it in a separate patch.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/mmc/host/sdhci.c |   48 ++++++++++++++++++++-------------------------
 drivers/mmc/host/sdhci.h |    4 +-
 2 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index dc6328c..748a2e3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -872,7 +872,7 @@ static void sdhci_finish_data(struct sdhci_host *host)
 
 		sdhci_send_command(host, data->stop);
 	} else
-		tasklet_schedule(&host->finish_tasklet);
+		schedule_work(&host->finish_work);
 }
 
 static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
@@ -901,7 +901,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 				"inhibit bit(s).\n", mmc_hostname(host->mmc));
 			sdhci_dumpregs(host);
 			cmd->error = -EIO;
-			tasklet_schedule(&host->finish_tasklet);
+			schedule_work(&host->finish_work);
 			return;
 		}
 		timeout--;
@@ -922,7 +922,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		printk(KERN_ERR "%s: Unsupported response type!\n",
 			mmc_hostname(host->mmc));
 		cmd->error = -EINVAL;
-		tasklet_schedule(&host->finish_tasklet);
+		schedule_work(&host->finish_work);
 		return;
 	}
 
@@ -973,7 +973,7 @@ static void sdhci_finish_command(struct sdhci_host *host)
 		sdhci_finish_data(host);
 
 	if (!host->cmd->data)
-		tasklet_schedule(&host->finish_tasklet);
+		schedule_work(&host->finish_work);
 
 	host->cmd = NULL;
 }
@@ -1122,7 +1122,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	if (!present || host->flags & SDHCI_DEVICE_DEAD) {
 		host->mrq->cmd->error = -ENOMEDIUM;
-		tasklet_schedule(&host->finish_tasklet);
+		schedule_work(&host->finish_work);
 	} else
 		sdhci_send_command(host, mrq->cmd);
 
@@ -1239,16 +1239,16 @@ static const struct mmc_host_ops sdhci_ops = {
 
 /*****************************************************************************\
  *                                                                           *
- * Tasklets                                                                  *
+ * Work handlers                                                             *
  *                                                                           *
 \*****************************************************************************/
 
-static void sdhci_tasklet_card(unsigned long param)
+static void sdhci_card_detect_work(struct work_struct *wk)
 {
 	struct sdhci_host *host;
 	unsigned long flags;
 
-	host = (struct sdhci_host*)param;
+	host = container_of(wk, struct sdhci_host, card_detect_work);
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1263,7 +1263,7 @@ static void sdhci_tasklet_card(unsigned long param)
 			sdhci_reset(host, SDHCI_RESET_DATA);
 
 			host->mrq->cmd->error = -ENOMEDIUM;
-			tasklet_schedule(&host->finish_tasklet);
+			schedule_work(&host->finish_work);
 		}
 	}
 
@@ -1272,13 +1272,13 @@ static void sdhci_tasklet_card(unsigned long param)
 	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
 }
 
-static void sdhci_tasklet_finish(unsigned long param)
+static void sdhci_finish_work(struct work_struct *wk)
 {
 	struct sdhci_host *host;
 	unsigned long flags;
 	struct mmc_request *mrq;
 
-	host = (struct sdhci_host*)param;
+	host = container_of(wk, struct sdhci_host, finish_work);
 
 	spin_lock_irqsave(&host->lock, flags);
 
@@ -1349,7 +1349,7 @@ static void sdhci_timeout_work(struct work_struct *wk)
 			else
 				host->mrq->cmd->error = -ETIMEDOUT;
 
-			tasklet_schedule(&host->finish_tasklet);
+			schedule_work(&host->finish_work);
 		}
 	}
 
@@ -1382,7 +1382,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask)
 		host->cmd->error = -EILSEQ;
 
 	if (host->cmd->error) {
-		tasklet_schedule(&host->finish_tasklet);
+		schedule_work(&host->finish_work);
 		return;
 	}
 
@@ -1528,7 +1528,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
 		sdhci_writel(host, intmask & (SDHCI_INT_CARD_INSERT |
 			SDHCI_INT_CARD_REMOVE), SDHCI_INT_STATUS);
-		tasklet_schedule(&host->card_tasklet);
+		schedule_work(&host->card_detect_work);
 	}
 
 	intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
@@ -1872,19 +1872,17 @@ int sdhci_add_host(struct sdhci_host *host)
 	mmc->max_blk_count = (host->quirks & SDHCI_QUIRK_NO_MULTIBLOCK) ? 1 : 65535;
 
 	/*
-	 * Init tasklets.
+	 * Init work structs.
 	 */
-	tasklet_init(&host->card_tasklet,
-		sdhci_tasklet_card, (unsigned long)host);
-	tasklet_init(&host->finish_tasklet,
-		sdhci_tasklet_finish, (unsigned long)host);
+	INIT_WORK(&host->card_detect_work, sdhci_card_detect_work);
+	INIT_WORK(&host->finish_work, sdhci_finish_work);
 
 	INIT_DELAYED_WORK(&host->timeout_work, sdhci_timeout_work);
 
 	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
 		mmc_hostname(mmc), host);
 	if (ret)
-		goto untasklet;
+		return ret;
 
 	sdhci_init(host, 0);
 
@@ -1923,10 +1921,6 @@ reset:
 	sdhci_reset(host, SDHCI_RESET_ALL);
 	free_irq(host->irq, host);
 #endif
-untasklet:
-	tasklet_kill(&host->card_tasklet);
-	tasklet_kill(&host->finish_tasklet);
-
 	return ret;
 }
 
@@ -1946,7 +1940,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 				" transfer!\n", mmc_hostname(host->mmc));
 
 			host->mrq->cmd->error = -ENOMEDIUM;
-			tasklet_schedule(&host->finish_tasklet);
+			schedule_work(&host->finish_work);
 		}
 
 		spin_unlock_irqrestore(&host->lock, flags);
@@ -1967,8 +1961,8 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	flush_delayed_work(&host->timeout_work);
 
-	tasklet_kill(&host->card_tasklet);
-	tasklet_kill(&host->finish_tasklet);
+	flush_work(&host->card_detect_work);
+	flush_work(&host->finish_work);
 
 	kfree(host->adma_desc);
 	kfree(host->align_buffer);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 55c114d..d96e4dd 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -288,8 +288,8 @@ struct sdhci_host {
 	dma_addr_t		adma_addr;	/* Mapped ADMA descr. table */
 	dma_addr_t		align_addr;	/* Mapped bounce buffer */
 
-	struct tasklet_struct	card_tasklet;	/* Tasklet structures */
-	struct tasklet_struct	finish_tasklet;
+	struct work_struct	card_detect_work;
+	struct work_struct	finish_work;
 
 	struct delayed_work	timeout_work;	/* Work for timeouts */
 
-- 
1.7.0.5

[PATCH 3/8] sdhci: Clear interrupt status register just once

[Anton Vorontsov]

There's no need to clear the interrupt status register bit-by-bit,
we can just clear it once. This simplifies irq handler.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/mmc/host/sdhci.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 748a2e3..9ec245c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1522,38 +1522,29 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 		goto out;
 	}
 
+	sdhci_writel(host, intmask, SDHCI_INT_STATUS);
+
 	DBG("*** %s got interrupt: 0x%08x\n",
 		mmc_hostname(host->mmc), intmask);
 
-	if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE)) {
-		sdhci_writel(host, intmask & (SDHCI_INT_CARD_INSERT |
-			SDHCI_INT_CARD_REMOVE), SDHCI_INT_STATUS);
+	if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE))
 		schedule_work(&host->card_detect_work);
-	}
 
 	intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
 
-	if (intmask & SDHCI_INT_CMD_MASK) {
-		sdhci_writel(host, intmask & SDHCI_INT_CMD_MASK,
-			SDHCI_INT_STATUS);
+	if (intmask & SDHCI_INT_CMD_MASK)
 		sdhci_cmd_irq(host, intmask & SDHCI_INT_CMD_MASK);
-	}
 
-	if (intmask & SDHCI_INT_DATA_MASK) {
-		sdhci_writel(host, intmask & SDHCI_INT_DATA_MASK,
-			SDHCI_INT_STATUS);
+	if (intmask & SDHCI_INT_DATA_MASK)
 		sdhci_data_irq(host, intmask & SDHCI_INT_DATA_MASK);
-	}
 
 	intmask &= ~(SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK);
 
 	intmask &= ~SDHCI_INT_ERROR;
 
-	if (intmask & SDHCI_INT_BUS_POWER) {
+	if (intmask & SDHCI_INT_BUS_POWER)
 		printk(KERN_ERR "%s: Card is consuming too much power!\n",
 			mmc_hostname(host->mmc));
-		sdhci_writel(host, SDHCI_INT_BUS_POWER, SDHCI_INT_STATUS);
-	}
 
 	intmask &= ~SDHCI_INT_BUS_POWER;
 
@@ -1566,8 +1557,6 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 		printk(KERN_ERR "%s: Unexpected interrupt 0x%08x.\n",
 			mmc_hostname(host->mmc), intmask);
 		sdhci_dumpregs(host);
-
-		sdhci_writel(host, intmask, SDHCI_INT_STATUS);
 	}
 
 	result = IRQ_HANDLED;
-- 
1.7.0.5

[PATCH 4/8] sdhci: Use threaded IRQ handler

[Anton Vorontsov]

We only need atomic context to disable SDHCI interrupts, after that
we can run in a kernel thread.

Note that irq handler still grabs an irqsave spinlock, we'll deal
with it in a subsequent patch.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/mmc/host/sdhci.c |   47 +++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9ec245c..0358b35 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1506,9 +1506,8 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask)
 	}
 }
 
-static irqreturn_t sdhci_irq(int irq, void *dev_id)
+static irqreturn_t sdhci_irq_thread(int irq, void *dev_id)
 {
-	irqreturn_t result;
 	struct sdhci_host* host = dev_id;
 	u32 intmask;
 	int cardint = 0;
@@ -1516,17 +1515,8 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	spin_lock(&host->lock);
 
 	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
-
-	if (!intmask || intmask == 0xffffffff) {
-		result = IRQ_NONE;
-		goto out;
-	}
-
 	sdhci_writel(host, intmask, SDHCI_INT_STATUS);
 
-	DBG("*** %s got interrupt: 0x%08x\n",
-		mmc_hostname(host->mmc), intmask);
-
 	if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE))
 		schedule_work(&host->card_detect_work);
 
@@ -1559,10 +1549,8 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 		sdhci_dumpregs(host);
 	}
 
-	result = IRQ_HANDLED;
-
 	mmiowb();
-out:
+
 	spin_unlock(&host->lock);
 
 	/*
@@ -1571,7 +1559,28 @@ out:
 	if (cardint)
 		mmc_signal_sdio_irq(host->mmc);
 
-	return result;
+	/* Restore interrupts */
+	intmask = sdhci_readl(host, SDHCI_INT_ENABLE);
+	sdhci_writel(host, intmask, SDHCI_SIGNAL_ENABLE);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t sdhci_irq(int irq, void *dev_id)
+{
+	struct sdhci_host *host = dev_id;
+	u32 intmask = sdhci_readl(host, SDHCI_INT_STATUS);
+
+	if (!intmask || intmask == 0xffffffff)
+		return IRQ_NONE;
+
+	/* Disable interrupts */
+	sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
+
+	DBG("*** %s got interrupt: 0x%08x\n",
+		mmc_hostname(host->mmc), intmask);
+
+	return IRQ_WAKE_THREAD;
 }
 
 /*****************************************************************************\
@@ -1608,8 +1617,8 @@ int sdhci_resume_host(struct sdhci_host *host)
 			host->ops->enable_dma(host);
 	}
 
-	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
-			  mmc_hostname(host->mmc), host);
+	ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_irq_thread,
+			  IRQF_SHARED, mmc_hostname(host->mmc), host);
 	if (ret)
 		return ret;
 
@@ -1868,8 +1877,8 @@ int sdhci_add_host(struct sdhci_host *host)
 
 	INIT_DELAYED_WORK(&host->timeout_work, sdhci_timeout_work);
 
-	ret = request_irq(host->irq, sdhci_irq, IRQF_SHARED,
-		mmc_hostname(mmc), host);
+	ret = request_threaded_irq(host->irq, sdhci_irq, sdhci_irq_thread,
+			  IRQF_SHARED, mmc_hostname(host->mmc), host);
 	if (ret)
 		return ret;
 
-- 
1.7.0.5

[PATCH 5/8] sdhci: Turn host->lock into a mutex

[Anton Vorontsov]

Finally, we can get rid of the host->lock spinlock, and turn it
into a mutex.

This patch does just this.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/mmc/host/sdhci.c |   53 +++++++++++++++++++--------------------------
 drivers/mmc/host/sdhci.h |    3 +-
 2 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0358b35..5eddbdb 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -20,6 +20,7 @@
 #include <linux/io.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/mutex.h>
 #include <linux/scatterlist.h>
 
 #include <linux/leds.h>
@@ -228,16 +229,15 @@ static void sdhci_led_control(struct led_classdev *led,
 	enum led_brightness brightness)
 {
 	struct sdhci_host *host = container_of(led, struct sdhci_host, led);
-	unsigned long flags;
 
-	spin_lock_irqsave(&host->lock, flags);
+	mutex_lock(&host->lock);
 
 	if (brightness == LED_OFF)
 		sdhci_deactivate_led(host);
 	else
 		sdhci_activate_led(host);
 
-	spin_unlock_irqrestore(&host->lock, flags);
+	mutex_unlock(&host->lock);
 }
 #endif
 
@@ -1099,11 +1099,10 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 {
 	struct sdhci_host *host;
 	bool present;
-	unsigned long flags;
 
 	host = mmc_priv(mmc);
 
-	spin_lock_irqsave(&host->lock, flags);
+	mutex_lock(&host->lock);
 
 	WARN_ON(host->mrq != NULL);
 
@@ -1127,18 +1126,17 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		sdhci_send_command(host, mrq->cmd);
 
 	mmiowb();
-	spin_unlock_irqrestore(&host->lock, flags);
+	mutex_unlock(&host->lock);
 }
 
 static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sdhci_host *host;
-	unsigned long flags;
 	u8 ctrl;
 
 	host = mmc_priv(mmc);
 
-	spin_lock_irqsave(&host->lock, flags);
+	mutex_lock(&host->lock);
 
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		goto out;
@@ -1183,25 +1181,24 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 out:
 	mmiowb();
-	spin_unlock_irqrestore(&host->lock, flags);
+	mutex_unlock(&host->lock);
 }
 
 static int sdhci_get_ro(struct mmc_host *mmc)
 {
 	struct sdhci_host *host;
-	unsigned long flags;
 	int present;
 
 	host = mmc_priv(mmc);
 
-	spin_lock_irqsave(&host->lock, flags);
+	mutex_lock(&host->lock);
 
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		present = 0;
 	else
 		present = sdhci_readl(host, SDHCI_PRESENT_STATE);
 
-	spin_unlock_irqrestore(&host->lock, flags);
+	mutex_unlock(&host->lock);
 
 	if (host->quirks & SDHCI_QUIRK_INVERTED_WRITE_PROTECT)
 		return !!(present & SDHCI_WRITE_PROTECT);
@@ -1211,11 +1208,10 @@ static int sdhci_get_ro(struct mmc_host *mmc)
 static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 {
 	struct sdhci_host *host;
-	unsigned long flags;
 
 	host = mmc_priv(mmc);
 
-	spin_lock_irqsave(&host->lock, flags);
+	mutex_lock(&host->lock);
 
 	if (host->flags & SDHCI_DEVICE_DEAD)
 		goto out;
@@ -1227,7 +1223,7 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 out:
 	mmiowb();
 
-	spin_unlock_irqrestore(&host->lock, flags);
+	mutex_unlock(&host->lock);
 }
 
 static const struct mmc_host_ops sdhci_ops = {
@@ -1246,11 +1242,10 @@ static const struct mmc_host_ops sdhci_ops = {
 static void sdhci_card_detect_work(struct work_struct *wk)
 {
 	struct sdhci_host *host;
-	unsigned long flags;
 
 	host = container_of(wk, struct sdhci_host, card_detect_work);
 
-	spin_lock_irqsave(&host->lock, flags);
+	mutex_lock(&host->lock);
 
 	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
 		if (host->mrq) {
@@ -1267,7 +1262,7 @@ static void sdhci_card_detect_work(struct work_struct *wk)
 		}
 	}
 
-	spin_unlock_irqrestore(&host->lock, flags);
+	mutex_unlock(&host->lock);
 
 	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
 }
@@ -1275,12 +1270,11 @@ static void sdhci_card_detect_work(struct work_struct *wk)
 static void sdhci_finish_work(struct work_struct *wk)
 {
 	struct sdhci_host *host;
-	unsigned long flags;
 	struct mmc_request *mrq;
 
 	host = container_of(wk, struct sdhci_host, finish_work);
 
-	spin_lock_irqsave(&host->lock, flags);
+	mutex_lock(&host->lock);
 
 	__cancel_delayed_work(&host->timeout_work);
 
@@ -1321,7 +1315,7 @@ static void sdhci_finish_work(struct work_struct *wk)
 #endif
 
 	mmiowb();
-	spin_unlock_irqrestore(&host->lock, flags);
+	mutex_unlock(&host->lock);
 
 	mmc_request_done(host->mmc, mrq);
 }
@@ -1329,11 +1323,10 @@ static void sdhci_finish_work(struct work_struct *wk)
 static void sdhci_timeout_work(struct work_struct *wk)
 {
 	struct sdhci_host *host;
-	unsigned long flags;
 
 	host = container_of(wk, struct sdhci_host, timeout_work.work);
 
-	spin_lock_irqsave(&host->lock, flags);
+	mutex_lock(&host->lock);
 
 	if (host->mrq) {
 		printk(KERN_ERR "%s: Timeout waiting for hardware "
@@ -1354,7 +1347,7 @@ static void sdhci_timeout_work(struct work_struct *wk)
 	}
 
 	mmiowb();
-	spin_unlock_irqrestore(&host->lock, flags);
+	mutex_unlock(&host->lock);
 }
 
 /*****************************************************************************\
@@ -1512,7 +1505,7 @@ static irqreturn_t sdhci_irq_thread(int irq, void *dev_id)
 	u32 intmask;
 	int cardint = 0;
 
-	spin_lock(&host->lock);
+	mutex_lock(&host->lock);
 
 	intmask = sdhci_readl(host, SDHCI_INT_STATUS);
 	sdhci_writel(host, intmask, SDHCI_INT_STATUS);
@@ -1551,7 +1544,7 @@ static irqreturn_t sdhci_irq_thread(int irq, void *dev_id)
 
 	mmiowb();
 
-	spin_unlock(&host->lock);
+	mutex_unlock(&host->lock);
 
 	/*
 	 * We have to delay this as it calls back into the driver.
@@ -1816,7 +1809,7 @@ int sdhci_add_host(struct sdhci_host *host)
 		return -ENODEV;
 	}
 
-	spin_lock_init(&host->lock);
+	mutex_init(&host->lock);
 
 	/*
 	 * Maximum number of segments. Depends on if the hardware
@@ -1926,10 +1919,8 @@ EXPORT_SYMBOL_GPL(sdhci_add_host);
 
 void sdhci_remove_host(struct sdhci_host *host, int dead)
 {
-	unsigned long flags;
-
 	if (dead) {
-		spin_lock_irqsave(&host->lock, flags);
+		mutex_lock(&host->lock);
 
 		host->flags |= SDHCI_DEVICE_DEAD;
 
@@ -1941,7 +1932,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 			schedule_work(&host->finish_work);
 		}
 
-		spin_unlock_irqrestore(&host->lock, flags);
+		mutex_unlock(&host->lock);
 	}
 
 	sdhci_disable_card_detection(host);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index d96e4dd..364d4e8 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -15,6 +15,7 @@
 #include <linux/workqueue.h>
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include <linux/mutex.h>
 #include <linux/io.h>
 
 /*
@@ -256,7 +257,7 @@ struct sdhci_host {
 	char   led_name[32];
 #endif
 
-	spinlock_t		lock;		/* Mutex */
+	struct mutex		lock;		/* Mutex */
 
 	int			flags;		/* Host attributes */
 #define SDHCI_USE_SDMA		(1<<0)		/* Host is SDMA capable */
-- 
1.7.0.5

[PATCH 6/8] sdhci: Get rid of card detect work

[Anton Vorontsov]

Nowadays we can just call the card detection handler directly,
no need for the separate work struct.

We still need host->finish_work as sdhci_finish_work() calls
mmc_request_done(), which tries to re-enter the driver.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/mmc/host/sdhci.c |   14 ++------------
 drivers/mmc/host/sdhci.h |    1 -
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 5eddbdb..56d5c56 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1239,14 +1239,8 @@ static const struct mmc_host_ops sdhci_ops = {
  *                                                                           *
 \*****************************************************************************/
 
-static void sdhci_card_detect_work(struct work_struct *wk)
+static void sdhci_card_detect(struct sdhci_host *host)
 {
-	struct sdhci_host *host;
-
-	host = container_of(wk, struct sdhci_host, card_detect_work);
-
-	mutex_lock(&host->lock);
-
 	if (!(sdhci_readl(host, SDHCI_PRESENT_STATE) & SDHCI_CARD_PRESENT)) {
 		if (host->mrq) {
 			printk(KERN_ERR "%s: Card removed during transfer!\n",
@@ -1262,8 +1256,6 @@ static void sdhci_card_detect_work(struct work_struct *wk)
 		}
 	}
 
-	mutex_unlock(&host->lock);
-
 	mmc_detect_change(host->mmc, msecs_to_jiffies(200));
 }
 
@@ -1511,7 +1503,7 @@ static irqreturn_t sdhci_irq_thread(int irq, void *dev_id)
 	sdhci_writel(host, intmask, SDHCI_INT_STATUS);
 
 	if (intmask & (SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE))
-		schedule_work(&host->card_detect_work);
+		sdhci_card_detect(host);
 
 	intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE);
 
@@ -1865,7 +1857,6 @@ int sdhci_add_host(struct sdhci_host *host)
 	/*
 	 * Init work structs.
 	 */
-	INIT_WORK(&host->card_detect_work, sdhci_card_detect_work);
 	INIT_WORK(&host->finish_work, sdhci_finish_work);
 
 	INIT_DELAYED_WORK(&host->timeout_work, sdhci_timeout_work);
@@ -1950,7 +1941,6 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 
 	flush_delayed_work(&host->timeout_work);
 
-	flush_work(&host->card_detect_work);
 	flush_work(&host->finish_work);
 
 	kfree(host->adma_desc);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 364d4e8..5f7d649 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -289,7 +289,6 @@ struct sdhci_host {
 	dma_addr_t		adma_addr;	/* Mapped ADMA descr. table */
 	dma_addr_t		align_addr;	/* Mapped bounce buffer */
 
-	struct work_struct	card_detect_work;
 	struct work_struct	finish_work;
 
 	struct delayed_work	timeout_work;	/* Work for timeouts */
-- 
1.7.0.5

[PATCH 7/8] sdhci: Get rid of mdelay()s where it is safe and makes sense

[Anton Vorontsov]

Since we don't run in the atomic context any longer, we can
turn mdelay()s into msleep()s.

The only place where the driver is still using mdelay() is
sdhci_send_command(). There it is possible to use sleepable
delays too, but we don't actually want to force rescheduling
in a hot path.

Sure, we might end up calling msleep() there too, just not
via this safe patch.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/mmc/host/sdhci-of-esdhc.c |    2 +-
 drivers/mmc/host/sdhci.c          |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index c8623de..e9f99fe 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -94,7 +94,7 @@ static void esdhc_set_clock(struct sdhci_host *host, unsigned int clock)
 	setbits32(host->ioaddr + ESDHC_SYSTEM_CONTROL, ESDHC_CLOCK_IPGEN |
 		  ESDHC_CLOCK_HCKEN | ESDHC_CLOCK_PEREN |
 		  div << ESDHC_DIVIDER_SHIFT | pre_div << ESDHC_PREDIV_SHIFT);
-	mdelay(100);
+	msleep(100);
 out:
 	host->clock = clock;
 }
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 56d5c56..e6adda8 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -171,7 +171,7 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
 			return;
 		}
 		timeout--;
-		mdelay(1);
+		msleep(1);
 	}
 
 	if (host->quirks & SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET)
@@ -1019,7 +1019,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 			return;
 		}
 		timeout--;
-		mdelay(1);
+		msleep(1);
 	}
 
 	clk |= SDHCI_CLOCK_CARD_EN;
@@ -1086,7 +1086,7 @@ static void sdhci_set_power(struct sdhci_host *host, unsigned short power)
 	 * can apply clock after applying power
 	 */
 	if (host->quirks & SDHCI_QUIRK_DELAY_AFTER_POWER)
-		mdelay(10);
+		msleep(10);
 }
 
 /*****************************************************************************\
-- 
1.7.0.5

[PATCH 8/8] sdhci: Use jiffies instead of a timeout counter

[Anton Vorontsov]

Just a cosmetic change, should not affect functionality.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/mmc/host/sdhci.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e6adda8..c754df1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/scatterlist.h>
+#include <linux/jiffies.h>
 
 #include <linux/leds.h>
 
@@ -160,17 +161,16 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask)
 		host->clock = 0;
 
 	/* Wait max 100 ms */
-	timeout = 100;
+	timeout = jiffies + msecs_to_jiffies(100);
 
 	/* hw clears the bit when it's done */
 	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
-		if (timeout == 0) {
+		if (time_after(jiffies, timeout)) {
 			printk(KERN_ERR "%s: Reset 0x%x never completed.\n",
 				mmc_hostname(host->mmc), (int)mask);
 			sdhci_dumpregs(host);
 			return;
 		}
-		timeout--;
 		msleep(1);
 	}
 
@@ -884,7 +884,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	WARN_ON(host->cmd);
 
 	/* Wait max 10 ms */
-	timeout = 10;
+	timeout = jiffies + msecs_to_jiffies(10);
 
 	mask = SDHCI_CMD_INHIBIT;
 	if ((cmd->data != NULL) || (cmd->flags & MMC_RSP_BUSY))
@@ -896,7 +896,7 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mask &= ~SDHCI_DATA_INHIBIT;
 
 	while (sdhci_readl(host, SDHCI_PRESENT_STATE) & mask) {
-		if (timeout == 0) {
+		if (time_after(jiffies, timeout)) {
 			printk(KERN_ERR "%s: Controller never released "
 				"inhibit bit(s).\n", mmc_hostname(host->mmc));
 			sdhci_dumpregs(host);
@@ -904,7 +904,6 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 			schedule_work(&host->finish_work);
 			return;
 		}
-		timeout--;
 		mdelay(1);
 	}
 
-- 
1.7.0.5

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Matt Fleming]

On Wed, 14 Jul 2010 17:07:28 +0400, Anton Vorontsov <avorontsov@mvista.com> wrote:
> Hi all,
> 
> Currently the sdhci driver does everything in the atomic context.
> And what is worse, PIO transfers are made from the IRQ handler.
> 
> This causes huge latencies (up to 120 ms). On some P2020 SOCs,
> DMA and card detection is broken, which means that kernel polls
> for the card via PIO transfers every second. Needless to say
> that this is quite bad.
> 
> So, this patch set reworks sdhci code to avoid atomic context,
> almost completely. We only do two device memory operations
> in the atomic context, and all the rest is threaded.
> 
> I noticed no throughput drop neither with PIO transfers nor
> with DMA (tested on MPC8569E CPU), while latencies should be
> greatly improved.

I haven't had time to read these patches in detail yet but they all seem
to be sensible changes. A very nice series!

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Andrew Morton]

On Wed, 14 Jul 2010 17:07:28 +0400
Anton Vorontsov <avorontsov@mvista.com> wrote:

> Hi all,
> 
> Currently the sdhci driver does everything in the atomic context.
> And what is worse, PIO transfers are made from the IRQ handler.
> 
> This causes huge latencies (up to 120 ms). On some P2020 SOCs,
> DMA and card detection is broken, which means that kernel polls
> for the card via PIO transfers every second. Needless to say
> that this is quite bad.
> 
> So, this patch set reworks sdhci code to avoid atomic context,
> almost completely. We only do two device memory operations
> in the atomic context, and all the rest is threaded.
> 
> I noticed no throughput drop neither with PIO transfers nor
> with DMA (tested on MPC8569E CPU), while latencies should be
> greatly improved.
> 

The patchset looks good to me, but it'd be nice to hear from the other
people who work on this code, please?

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Andrew Morton]

On Wed, 14 Jul 2010 17:07:28 +0400
Anton Vorontsov <avorontsov@mvista.com> wrote:

> Hi all,
> 
> Currently the sdhci driver does everything in the atomic context.
> And what is worse, PIO transfers are made from the IRQ handler.
> 
> This causes huge latencies (up to 120 ms). On some P2020 SOCs,
> DMA and card detection is broken, which means that kernel polls
> for the card via PIO transfers every second. Needless to say
> that this is quite bad.
> 
> So, this patch set reworks sdhci code to avoid atomic context,
> almost completely. We only do two device memory operations
> in the atomic context, and all the rest is threaded.
> 
> I noticed no throughput drop neither with PIO transfers nor
> with DMA (tested on MPC8569E CPU), while latencies should be
> greatly improved.
> 

This patchset isn't causing any problems yet, but may do so in the
future and will impact the validity of any testing.  It seems to be
kind of stuck.  Should I drop it all?

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Chris Ball]

Hi Andrew,

On Tue, Sep 07, 2010 at 03:38:13PM -0700, Andrew Morton wrote:
> > I noticed no throughput drop neither with PIO transfers nor
> > with DMA (tested on MPC8569E CPU), while latencies should be
> > greatly improved.
> 
> This patchset isn't causing any problems yet, but may do so in the
> future and will impact the validity of any testing.  It seems to be
> kind of stuck.  Should I drop it all?

I suggest keeping it -- I'll find time to test it out here soon, and
will keep it in mind as a possible regression cause.

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Anton Vorontsov]

On Wed, Sep 08, 2010 at 10:37:41PM +0100, Chris Ball wrote:
> Hi Andrew,
> 
> On Tue, Sep 07, 2010 at 03:38:13PM -0700, Andrew Morton wrote:
> > > I noticed no throughput drop neither with PIO transfers nor
> > > with DMA (tested on MPC8569E CPU), while latencies should be
> > > greatly improved.
> > 
> > This patchset isn't causing any problems yet, but may do so in the
> > future and will impact the validity of any testing.  It seems to be
> > kind of stuck.  Should I drop it all?
> 
> I suggest keeping it -- I'll find time to test it out here soon, and
> will keep it in mind as a possible regression cause.

Thanks!

Would be also great if you could point out which patch causes
most of the performance drop (if any)?

Albert, if you could find time, can you also "bisect" the
patchset? I wouldn't want to buy Nintendo WII just to debug the
perf regression. ;-) FWIW, I tried to disable multiblock
read/writes and test with SD cards, and still didn't notice
any performance drops.

Maybe it's SDIO IRQs that cause the performance drop for the
WII case, as we delay them a little bit? Or it could be the
patch that introduces threaded IRQ handler in whole causes
it. If so, I guess we'd need to move some of the processing to
the real IRQ context, keeping the handler lockless (if
possible) or introducing a very fine grained locking.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Chris Ball]

Hi Anton,

On Thu, Sep 09, 2010 at 01:57:50AM +0400, Anton Vorontsov wrote:
> Thanks!
> 
> Would be also great if you could point out which patch causes
> most of the performance drop (if any)?
> 
> Albert, if you could find time, can you also "bisect" the
> patchset? I wouldn't want to buy Nintendo WII just to debug the
> perf regression. ;-) FWIW, I tried to disable multiblock
> read/writes and test with SD cards, and still didn't notice
> any performance drops.
> 
> Maybe it's SDIO IRQs that cause the performance drop for the
> WII case, as we delay them a little bit? Or it could be the
> patch that introduces threaded IRQ handler in whole causes
> it. If so, I guess we'd need to move some of the processing to
> the real IRQ context, keeping the handler lockless (if
> possible) or introducing a very fine grained locking.

I didn't know anything about a reported performance drop, and I don't
think Andrew did either -- Albert's test results don't seem to have
made it to this list, or anywhere else that I can see.  Could you 
link to/repost his comments?

(I'll be testing with libertas, so that will stress-test SDIO IRQs.)

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Anton Vorontsov]

On Wed, Sep 08, 2010 at 11:05:48PM +0100, Chris Ball wrote:
> Hi Anton,
> 
> On Thu, Sep 09, 2010 at 01:57:50AM +0400, Anton Vorontsov wrote:
> > Thanks!
> > 
> > Would be also great if you could point out which patch causes
> > most of the performance drop (if any)?
> > 
> > Albert, if you could find time, can you also "bisect" the
> > patchset? I wouldn't want to buy Nintendo WII just to debug the
> > perf regression. ;-) FWIW, I tried to disable multiblock
> > read/writes and test with SD cards, and still didn't notice
> > any performance drops.
> > 
> > Maybe it's SDIO IRQs that cause the performance drop for the
> > WII case, as we delay them a little bit? Or it could be the
> > patch that introduces threaded IRQ handler in whole causes
> > it. If so, I guess we'd need to move some of the processing to
> > the real IRQ context, keeping the handler lockless (if
> > possible) or introducing a very fine grained locking.
> 
> I didn't know anything about a reported performance drop, and I don't
> think Andrew did either -- Albert's test results don't seem to have
> made it to this list, or anywhere else that I can see.  Could you 
> link to/repost his comments?
> 
> (I'll be testing with libertas, so that will stress-test SDIO IRQs.)

Sure thing, here are Albert's results.

----- Forwarded message from Albert Herranz <albert_herranz@yahoo.es> -----

Date: Mon, 02 Aug 2010 21:23:51 +0200
From: Albert Herranz <albert_herranz@yahoo.es>
To: Anton Vorontsov <cbouatmailru@gmail.com>
CC: akpm@linux-foundation.org, mm-commits@vger.kernel.org,
	ben-linux@fluff.org, matt@console-pimps.org, pierre@ossman.eu,
	w.sang@pengutronix.de, mb@bu3sch.de
Subject: Re: + sdhci-use-work-structs-instead-of-tasklets.patch added to -mm
	tree

Hi,

Some initial numbers regarding performance. The patchset seems to cause a noticeable performance drop.
I've run two iperf client tests (see the two invocations of iperf -c) and two iperf server tests (see iperf -s invocation).

== 2.6.33 ==

$ iperf -c 192.168.1.130 
------------------------------------------------------------
Client connecting to 192.168.1.130, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  3] local 192.168.1.127 port 40119 connected with 192.168.1.130 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.1 sec  1.05 MBytes    872 Kbits/sec

$ iperf -c 192.168.1.130 
------------------------------------------------------------
Client connecting to 192.168.1.130, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  3] local 192.168.1.127 port 40120 connected with 192.168.1.130 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec  1.04 MBytes    870 Kbits/sec

$ iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[  4] local 192.168.1.127 port 5001 connected with 192.168.1.130 port 36691
[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.2 sec  3.61 MBytes  2.98 Mbits/sec
[  5] local 192.168.1.127 port 5001 connected with 192.168.1.130 port 36692
[  5]  0.0-10.1 sec  4.94 MBytes  4.09 Mbits/sec


== 2.6.33 + "sdhci: Move real work out of an atomic context" patchset ==

$ iperf -c 192.168.1.130 
------------------------------------------------------------
Client connecting to 192.168.1.130, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  3] local 192.168.1.127 port 39210 connected with 192.168.1.130 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec    368 KBytes    301 Kbits/sec

$ iperf -c 192.168.1.130 
------------------------------------------------------------
Client connecting to 192.168.1.130, TCP port 5001
TCP window size: 16.0 KByte (default)
------------------------------------------------------------
[  3] local 192.168.1.127 port 39211 connected with 192.168.1.130 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.2 sec    440 KBytes    354 Kbits/sec

$ iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[  4] local 192.168.1.127 port 5001 connected with 192.168.1.130 port 57833
[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.2 sec  2.37 MBytes  1.95 Mbits/sec
[  5] local 192.168.1.127 port 5001 connected with 192.168.1.130 port 57834
[  5]  0.0-10.2 sec  2.30 MBytes  1.90 Mbits/sec

The subjective feeling is too that the system is slower.

Cheers,
Albert

----- End forwarded message -----

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Chris Ball]

Hi,

On Wed, Sep 08, 2010 at 10:37:41PM +0100, Chris Ball wrote:
> Hi Andrew,
> 
> On Tue, Sep 07, 2010 at 03:38:13PM -0700, Andrew Morton wrote:
> > > I noticed no throughput drop neither with PIO transfers nor
> > > with DMA (tested on MPC8569E CPU), while latencies should be
> > > greatly improved.
> > 
> > This patchset isn't causing any problems yet, but may do so in the
> > future and will impact the validity of any testing.  It seems to be
> > kind of stuck.  Should I drop it all?
> 
> I suggest keeping it -- I'll find time to test it out here soon, and
> will keep it in mind as a possible regression cause.

Am running this now.  The first thing I'm noticing is a repeated BUG():

[    7.288186] Write protecting the kernel read-only data: 1072k
[    7.306446] BUG: sleeping function called from invalid context at kernel/mutex.c:94
[    7.324375] in_atomic(): 1, irqs_disabled(): 0, pid: 532, name: mmc2/0
[    7.340989] Pid: 532, comm: mmc2/0 Not tainted 2.6.35.4_xo1.5-20100908.2141.olpc.44f3b38_DIRTY #1
[    7.360129] Call Trace:
[    7.372843]  [<b04193ce>] __might_sleep+0xd9/0xe0
[    7.387864]  [<b07260cc>] mutex_lock+0x1c/0x2a
[    7.402576]  [<b06396e8>] sdhci_led_control+0x1a/0x41
[    7.417727]  [<b063bece>] led_trigger_event+0x42/0x5c
[    7.432807]  [<b06326f8>] mmc_request_done+0x56/0x6f
[    7.447597]  [<b063a2d1>] sdhci_finish_work+0xc8/0xcd
[    7.462643]  [<b063a209>] ? sdhci_finish_work+0x0/0xcd
[    7.477941]  [<b0432776>] worker_thread+0x165/0x1ed
[    7.492856]  [<b063a209>] ? sdhci_finish_work+0x0/0xcd
[    7.508204]  [<b0435591>] ? autoremove_wake_function+0x0/0x34
[    7.524178]  [<b0432611>] ? worker_thread+0x0/0x1ed
[    7.538953]  [<b04352a0>] kthread+0x63/0x68
[    7.552659]  [<b043523d>] ? kthread+0x0/0x68
[    7.566349]  [<b0402cf6>] kernel_thread_helper+0x6/0x10
[    7.709931] udev: starting version 141
[    7.940374] mmc2: new high speed SDHC card at address e4da
[    8.058165] mmcblk0: mmc2:e4da SU04G 3.69 GiB 
[    8.135730]  mmcblk0: p1 p2

Full dmesg is at http://chris.printf.net/anton-mutex-dmesg.txt. 
Anton, the kernel is 2.6.35.4-olpc plus your patchset from -mm.
I can think about how to test on an upstream kernel instead, but
perhaps your own tests simply didn't hit sdhci_led_control(). 

Andrew, if you want to drop this while the BUG() and potential
performance regressions are worked out, I'd be happy to keep 
testing patches from Anton until it's without regressions here.

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Anton Vorontsov]

On Thu, Sep 09, 2010 at 03:28:34AM +0100, Chris Ball wrote:
[...]
> [    7.372843]  [<b04193ce>] __might_sleep+0xd9/0xe0
> [    7.387864]  [<b07260cc>] mutex_lock+0x1c/0x2a
> [    7.402576]  [<b06396e8>] sdhci_led_control+0x1a/0x41
> [    7.417727]  [<b063bece>] led_trigger_event+0x42/0x5c

led_trigger_even grabs a readlock. :-(

> [    7.432807]  [<b06326f8>] mmc_request_done+0x56/0x6f
> [    7.447597]  [<b063a2d1>] sdhci_finish_work+0xc8/0xcd
> [    7.462643]  [<b063a209>] ? sdhci_finish_work+0x0/0xcd
> [    7.477941]  [<b0432776>] worker_thread+0x165/0x1ed
> [    7.492856]  [<b063a209>] ? sdhci_finish_work+0x0/0xcd
> [    7.508204]  [<b0435591>] ? autoremove_wake_function+0x0/0x34
> [    7.524178]  [<b0432611>] ? worker_thread+0x0/0x1ed
> [    7.538953]  [<b04352a0>] kthread+0x63/0x68
> [    7.552659]  [<b043523d>] ? kthread+0x0/0x68
> [    7.566349]  [<b0402cf6>] kernel_thread_helper+0x6/0x10
> [    7.709931] udev: starting version 141
> [    7.940374] mmc2: new high speed SDHC card at address e4da
> [    8.058165] mmcblk0: mmc2:e4da SU04G 3.69 GiB 
> [    8.135730]  mmcblk0: p1 p2
> 
> Full dmesg is at http://chris.printf.net/anton-mutex-dmesg.txt. 
> Anton, the kernel is 2.6.35.4-olpc plus your patchset from -mm.
> I can think about how to test on an upstream kernel instead, but
> perhaps your own tests simply didn't hit sdhci_led_control(). 

Yep, LEDS support was turned off.

> Andrew, if you want to drop this while the BUG() and potential
> performance regressions are worked out, I'd be happy to keep 
> testing patches from Anton until it's without regressions here.

Thanks Chris.

I also think that it's better to drop these series now,
and meanwhile I'll try to prepare another patchset.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH 0/8] sdhci: Move real work out of an atomic context

[Jeremie Samuel]

Hi all,

Currently the sdhci driver does everything in the atomic context.
And what is worse, PIO transfers are made from the IRQ handler.

Some patches were already submitted to solve this issue. But there were 
rejected because they involved new issues.

This set of patches is an evolution of an old patch from Anton Vorontsov.
I tried to fix all the problems involved by the patches. I tested it for 
several time now with SD cards and SDIO.

So, this patch set reworks sdhci code to avoid atomic context,
almost completely.

Thanks,

Jeremie Samuel

Jeremie Samuel (8):
  sdhci: Turn timeout timer into delayed work
  sdhci: Turn tuning timeout timer into delayed work
  sdhci: Use work structs instead of tasklets
  sdhci: Use threaded IRQ handler
  sdhci: Delay led blinking
  sdhci: Turn host->lock into a mutex
  sdhci: Get rid of mdelay()s where it is safe and makes sense
  sdhci: Use jiffies instead of a timeout counter

 drivers/mmc/host/sdhci.c  |  327 ++++++++++++++++++++++-----------------------
 include/linux/mmc/sdhci.h |   13 +-
 2 files changed, 168 insertions(+), 172 deletions(-)

-- 
1.7.10.4

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Jeremie Samuel]

Hi,

I posted these patches a few weeks ago. Is it possible to get a feedback for this submission?

Thank you for your help.

-- 
Jeremie Samuel              Parrot S.A.
Software Engineer           14, quai de Jemmapes
R&D/OS Platform             75010 Paris, France
http://www.parrot.com

On 24/05/2013 18:00, Jérémie Samuel wrote:

> Hi all,
>
> Currently the sdhci driver does everything in the atomic context.
> And what is worse, PIO transfers are made from the IRQ handler.
>
> Some patches were already submitted to solve this issue. But there were
> rejected because they involved new issues.
>
> This set of patches is an evolution of an old patch from Anton Vorontsov.
> I tried to fix all the problems involved by the patches. I tested it for
> several time now with SD cards and SDIO.
>
> So, this patch set reworks sdhci code to avoid atomic context,
> almost completely.
>
> Thanks,
>
> Jeremie Samuel
>
> Jeremie Samuel (8):
>    sdhci: Turn timeout timer into delayed work
>    sdhci: Turn tuning timeout timer into delayed work
>    sdhci: Use work structs instead of tasklets
>    sdhci: Use threaded IRQ handler
>    sdhci: Delay led blinking
>    sdhci: Turn host->lock into a mutex
>    sdhci: Get rid of mdelay()s where it is safe and makes sense
>    sdhci: Use jiffies instead of a timeout counter
>
>   drivers/mmc/host/sdhci.c  |  327 ++++++++++++++++++++++-----------------------
>   include/linux/mmc/sdhci.h |   13 +-
>   2 files changed, 168 insertions(+), 172 deletions(-)
>

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Chris Ball]

Hi Jeremie,

On Thu, Jun 13 2013, Jeremie Samuel wrote:
> I posted these patches a few weeks ago. Is it possible to get a
> feedback for this submission?

Please could you resubmit in two weeks or so, so that we can spend a
full cycle with these in linux-next before including them in Linux
3.12 if there are no problems?  Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH 0/8] sdhci: Move real work out of an atomic context

[Jeremie Samuel]

Hi all,

Currently the sdhci driver does everything in the atomic context.
And what is worse, PIO transfers are made from the IRQ handler.

Some patches were already submitted to solve this issue. But there were
rejected because they involved new issues.

This set of patches is an evolution of an old patch from Anton Vorontsov.
I tried to fix all the problems involved by the patches. I tested it for
several time now with SD cards and SDIO.

So, this patch set reworks sdhci code to avoid atomic context,
almost completely.

Thanks,

Jeremie Samuel

Jeremie Samuel (8):
  sdhci: Turn timeout timer into delayed work
  sdhci: Turn tuning timeout timer into delayed work
  sdhci: Use work structs instead of tasklets
  sdhci: Use threaded IRQ handler
  sdhci: Delay led blinking
  sdhci: Turn host->lock into a mutex
  sdhci: Get rid of mdelay()s where it is safe and makes sense
  sdhci: Use jiffies instead of a timeout counter

 drivers/mmc/host/sdhci.c  |  327 ++++++++++++++++++++++-----------------------
 include/linux/mmc/sdhci.h |   13 +-
 2 files changed, 168 insertions(+), 172 deletions(-)

-- 
1.7.10.4

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Philip Rakity]

On Jul 9, 2013, at 4:44 PM, Jeremie Samuel <jeremie.samuel.ext@parrot.com> wrote:

> Hi all,
> 
> Currently the sdhci driver does everything in the atomic context.
> And what is worse, PIO transfers are made from the IRQ handler.
> 
> Some patches were already submitted to solve this issue. But there were
> rejected because they involved new issues.
> 
> This set of patches is an evolution of an old patch from Anton Vorontsov.
> I tried to fix all the problems involved by the patches. I tested it for
> several time now with SD cards and SDIO.
> 
> So, this patch set reworks sdhci code to avoid atomic context,
> almost completely.
> 
> Thanks,
> 

Running DDR50, SDR104 or HS200 what is the performance impact ?


> Jeremie Samuel
> 
> Jeremie Samuel (8):
>  sdhci: Turn timeout timer into delayed work
>  sdhci: Turn tuning timeout timer into delayed work
>  sdhci: Use work structs instead of tasklets
>  sdhci: Use threaded IRQ handler
>  sdhci: Delay led blinking
>  sdhci: Turn host->lock into a mutex
>  sdhci: Get rid of mdelay()s where it is safe and makes sense
>  sdhci: Use jiffies instead of a timeout counter
> 
> drivers/mmc/host/sdhci.c  |  327 ++++++++++++++++++++++-----------------------
> include/linux/mmc/sdhci.h |   13 +-
> 2 files changed, 168 insertions(+), 172 deletions(-)
> 
> -- 
> 1.7.10.4
> 
> --
> 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

Re: [PATCH 0/8] sdhci: Move real work out of an atomic context

[Jeremie Samuel]

On 09/07/2013 17:52, Philip Rakity wrote:

> On Jul 9, 2013, at 4:44 PM, Jeremie Samuel<jeremie.samuel.ext@parrot.com>  wrote:
>
>> Hi all,
>>
>> Currently the sdhci driver does everything in the atomic context.
>> And what is worse, PIO transfers are made from the IRQ handler.
>>
>> Some patches were already submitted to solve this issue. But there were
>> rejected because they involved new issues.
>>
>> This set of patches is an evolution of an old patch from Anton Vorontsov.
>> I tried to fix all the problems involved by the patches. I tested it for
>> several time now with SD cards and SDIO.
>>
>> So, this patch set reworks sdhci code to avoid atomic context,
>> almost completely.
>>
>> Thanks,
>>
> Running DDR50, SDR104 or HS200 what is the performance impact ?
I've realised performance tests with iozone benchmark and the performance impact was approximatively 0.5%.

>
>
>> Jeremie Samuel
>>
>> Jeremie Samuel (8):
>>   sdhci: Turn timeout timer into delayed work
>>   sdhci: Turn tuning timeout timer into delayed work
>>   sdhci: Use work structs instead of tasklets
>>   sdhci: Use threaded IRQ handler
>>   sdhci: Delay led blinking
>>   sdhci: Turn host->lock into a mutex
>>   sdhci: Get rid of mdelay()s where it is safe and makes sense
>>   sdhci: Use jiffies instead of a timeout counter
>>
>> drivers/mmc/host/sdhci.c  |  327 ++++++++++++++++++++++-----------------------
>> include/linux/mmc/sdhci.h |   13 +-
>> 2 files changed, 168 insertions(+), 172 deletions(-)
>>
>> -- 
>> 1.7.10.4
>>
>> --
>> 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

[PATCH 0/8] sdhci: Move real work out of an atomic context

[Jeremie Samuel]

Hi all,

Currently the sdhci driver does everything in the atomic context.
And what is worse, PIO transfers are made from the IRQ handler.

Some patches were already submitted to solve this issue. But there were
rejected because they involved new issues.

This set of patches is an evolution of an old patch from Anton Vorontsov.
I tried to fix all the problems involved by the patches. I tested it for
several time now with SD cards and SDIO.

So, this patch set reworks sdhci code to avoid atomic context,
almost completely.

Thanks,

Jeremie Samuel

Jeremie Samuel (8):
  sdhci: Turn timeout timer into delayed work
  sdhci: Turn tuning timeout timer into delayed work
  sdhci: Use work structs instead of tasklets
  sdhci: Use threaded IRQ handler
  sdhci: Delay led blinking
  sdhci: Turn host->lock into a mutex
  sdhci: Get rid of mdelay()s where it is safe and makes sense
  sdhci: Use jiffies instead of a timeout counter

 drivers/mmc/host/sdhci-esdhc-imx.c |    4 +-
 drivers/mmc/host/sdhci-pxav3.c     |   10 +-
 drivers/mmc/host/sdhci-s3c.c       |    5 +-
 drivers/mmc/host/sdhci.c           |  329 ++++++++++++++++++------------------
 include/linux/mmc/sdhci.h          |   13 +-
 5 files changed, 177 insertions(+), 184 deletions(-)

-- 
1.7.10.4