sdhci: Move real work out of an atomic context

PATCH In review Anton Vorontsov 5 participants 18 messages Jul 14, 2010 – Sep 9, 2010

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Anton Vorontsov

author

2010-07-14 13:07 UTC #0 lore raw

cover sdhci: Move real work out of an atomic context

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,

Signature · 3 lines

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Anton Vorontsov

author

2010-07-14 13:07 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/mmc/host/sdhci.c |   14 ++++++++------
 drivers/mmc/host/sdhci.h |    3 ++-
 2 files changed, 10 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +8 -6

index c6d1bd8..dc6328c 100644
@@ -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);

drivers/mmc/host/sdhci.h +2 -1

index c846813..55c114d 100644
@@ -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;
 };

Signature · 1 line

1.7.0.5

Anton Vorontsov

author

2010-07-14 13:07 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/mmc/host/sdhci.c |   48 ++++++++++++++++++++-------------------------
 drivers/mmc/host/sdhci.h |    4 +-
 2 files changed, 23 insertions(+), 29 deletions(-)

drivers/mmc/host/sdhci.c +21 ~~-27~~

index dc6328c..748a2e3 100644
@@ -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);

drivers/mmc/host/sdhci.h +2 -2

index 55c114d..d96e4dd 100644
@@ -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 */

Signature · 1 line

1.7.0.5

Anton Vorontsov

author

2010-07-14 13:08 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/mmc/host/sdhci.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

drivers/mmc/host/sdhci.c +6 ~~-17~~

index 748a2e3..9ec245c 100644
@@ -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;

Signature · 1 line

1.7.0.5

Anton Vorontsov

author

2010-07-14 13:08 UTC #4 lore raw

PATCH 4/8 sdhci: Use threaded IRQ handler

In reply to 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 <[email protected]>

---
 drivers/mmc/host/sdhci.c |   47 +++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 19 deletions(-)

drivers/mmc/host/sdhci.c +28 ~~-19~~

index 9ec245c..0358b35 100644
@@ -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;

Signature · 1 line

1.7.0.5

Anton Vorontsov

author

2010-07-14 13:08 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/mmc/host/sdhci.c |   53 +++++++++++++++++++--------------------------
 drivers/mmc/host/sdhci.h |    3 +-
 2 files changed, 24 insertions(+), 32 deletions(-)

drivers/mmc/host/sdhci.c +22 ~~-31~~

index 0358b35..5eddbdb 100644
@@ -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);

drivers/mmc/host/sdhci.h +2 -1

index d96e4dd..364d4e8 100644
@@ -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 */

Signature · 1 line

1.7.0.5

Anton Vorontsov

author

2010-07-14 13:08 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/mmc/host/sdhci.c |   14 ++------------
 drivers/mmc/host/sdhci.h |    1 -
 2 files changed, 2 insertions(+), 13 deletions(-)

drivers/mmc/host/sdhci.c +2 ~~-12~~

index 5eddbdb..56d5c56 100644
@@ -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);

drivers/mmc/host/sdhci.h +0 -1

index 364d4e8..5f7d649 100644
@@ -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 */

Signature · 1 line

1.7.0.5

Anton Vorontsov

author

2010-07-14 13:08 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/mmc/host/sdhci-of-esdhc.c |    2 +-
 drivers/mmc/host/sdhci.c          |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

drivers/mmc/host/sdhci-of-esdhc.c +1 -1

index c8623de..e9f99fe 100644
@@ -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;
 }

drivers/mmc/host/sdhci.c +3 -3

index 56d5c56..e6adda8 100644
@@ -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);
 }
 
 /*****************************************************************************\

Signature · 1 line

1.7.0.5

Anton Vorontsov

author

2010-07-14 13:08 UTC #8 lore raw

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

In reply to Anton Vorontsov

Just a cosmetic change, should not affect functionality.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/mmc/host/sdhci.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

drivers/mmc/host/sdhci.c +5 -6

index e6adda8..c754df1 100644
@@ -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);
 	}

Signature · 1 line

1.7.0.5

Matt Fleming

2010-07-15 06:02 UTC #9 lore raw

In reply to Anton Vorontsov

Quoted 10 lines · 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.

Quoted 7 lines

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!

Andrew Morton

2010-07-21 21:14 UTC #10 lore raw

In reply to Anton Vorontsov

Quoted 10 lines · On Wed, 14 Jul 2010 17:07:28 +0400 Anton Vorontsov <[email protected]>…

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.

Quoted 8 lines

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?

Andrew Morton

2010-09-07 22:38 UTC #11 lore raw

In reply to Anton Vorontsov

Quoted 10 lines · On Wed, 14 Jul 2010 17:07:28 +0400 Anton Vorontsov <[email protected]>…

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.

Quoted 8 lines

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?

Chris Ball

2010-09-08 21:38 UTC #12 lore raw

Hi Andrew,

Quoted 7 lines · Andrew Morton

Quoted 3 lines
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,

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Anton Vorontsov

2010-09-08 21:57 UTC #13 lore raw

Quoted 7 lines · Chris Ball

Hi Andrew,
Quoted 4 lines · Andrew Morton
Quoted 3 lines
I noticed no throughput drop neither with PIO transfers nor with DMA (tested on MPC8569E CPU), while latencies should be greatly improved.

Quoted 6 lines

Quoted 3 lines
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,

Signature · 3 lines

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

Chris Ball

2010-09-08 22:06 UTC #14 lore raw

Hi Anton,

Quoted 11 lines · Anton Vorontsov

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.

Quoted 6 lines

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,

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Anton Vorontsov

2010-09-08 22:27 UTC #15 lore raw

Quoted 21 lines · Chris Ball

Hi Anton,
Quoted 11 lines · Anton Vorontsov
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.
Quoted 6 lines
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.

Quoted 6 lines

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 <[email protected]> -----

Date: Mon, 02 Aug 2010 21:23:51 +0200
From: Albert Herranz <[email protected]>
To: Anton Vorontsov <[email protected]>
CC: [email protected], [email protected],
[email protected], [email protected], [email protected], [email protected], [email protected]
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 -----

Chris Ball

2010-09-09 02:28 UTC #16 lore raw

In reply to Chris Ball

Hi,

Quoted 7 lines · Chris Ball

Hi Andrew,
Quoted 4 lines · Andrew Morton
Quoted 3 lines
I noticed no throughput drop neither with PIO transfers nor with DMA (tested on MPC8569E CPU), while latencies should be greatly improved.

Quoted 6 lines

Quoted 3 lines
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,

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Anton Vorontsov

2010-09-09 07:15 UTC #17 lore raw

On Thu, Sep 09, 2010 at 03:28:34AM +0100, Chris Ball wrote:
[...]

Quoted 4 lines

[    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. :-(

Quoted 15 lines

[    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

Quoted 4 lines

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.

Quoted 3 lines

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.

Signature · 3 lines

Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2