[PATCH 0/6] Trivial MMC patches from Android

[PATCH 0/6] Trivial MMC patches from Android

[John Stultz]

I've recently spent some time looking through the Android tree,
and while there are some controversial interface changes, there
are also a number of patches which look to be simply fixes that
have not yet made it upstream. 

In order to make sure these fixes don't get overlooked, I'm trying
to filter out these small changes into narrowly scoped topic branches
then sending them out for review and comment.

Now, I'm not proposing that these changes be merged as-is. It
may very well be that, unknown to me, android developers have
already tried to submit these patches and they have been rejected
for good reason. Or some patches may very well be necessary hacks
to get thing shipping while deeper fixes are being worked on. If
that is the case, let me know and forgive me for the noise.

Authors: In many cases, the patches are lacking good commit
messages, so I cannot advocate for them very well. Your assistance
in answering questions that come up would be appreciated.

Maintainers: If you do find any of these patches distasteful,
that's fine, I'll be happy to drop them from my tree for now.
I really don't want to stir up another huge mail thread over these 
small patches, but I'd appreciate if you'd consider them as a
bug report illustrating an issue or a desired feature, and suggest 
what you see as a reasonable way to accomplish the desired
functionality presented in the patch.

The following patches are general MMC changes:

The first three look like fairly straightforward fixes.

The second three introduce a paranoid init option which retries
a few times on failure. The #ifdefs clearly need to be cleaned
up, but I wanted to send it out for comment on the basic
functionality.

You can also find the other trivial tree branches I'm working
on here (fair warning, I re-base this tree frequently): 
http://git.linaro.org/gitweb?p=people/jstultz/android-trivial.git;a=summary

thanks
-john

CC: Chris Ball <cjb@laptop.org>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dima Zavin <dima@android.com>
CC: Bentao Zou <bzou1@motorola.com>
CC: David Ding <david.j.ding@motorola.com>
CC: San Mehat <san@google.com>
CC: Colin Cross <ccross@android.com>
CC: Todd Poynor <toddpoynor@google.com>


Colin Cross (1):
  mmc_block: Allow more than 8 partitions per card

David Ding (1):
  mmc: block: Resume multi-block reads after transient read errors.

San Mehat (3):
  mmc: sd: Add new CONFIG_MMC_PARANOID_SD_INIT for enabling retries
    during SD detection
  mmc: sd: When resuming, try a little harder to init the card
  mmc: sd: Add retries in re-detection

Todd Poynor (1):
  sdhci: Always pass clock request value zero to set_clock host op

 drivers/mmc/card/block.c |    9 ++---
 drivers/mmc/core/Kconfig |    9 +++++
 drivers/mmc/core/sd.c    |   84 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.c |    2 +-
 4 files changed, 96 insertions(+), 8 deletions(-)

-- 
1.7.3.2.146.gca209

[PATCH 1/6] mmc: block: Resume multi-block reads after transient read errors.

[John Stultz]

From: David Ding <david.j.ding@motorola.com>

CC: Chris Ball <cjb@laptop.org>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dima Zavin <dima@android.com>
Signed-off-by: Bentao Zou <bzou1@motorola.com>
Signed-off-by: David Ding <david.j.ding@motorola.com>
Signed-off-by: San Mehat <san@google.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/mmc/card/block.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 61d233a..edac9ac 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -440,6 +440,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
 				continue;
 			}
 			status = get_card_status(card, req);
+		} else if (disable_multi == 1) {
+			disable_multi = 0;
 		}
 
 		if (brq.cmd.error) {
-- 
1.7.3.2.146.gca209

[PATCH 2/6] mmc_block: Allow more than 8 partitions per card

[John Stultz]

From: Colin Cross <ccross@android.com>

Set the GENHD_FL_EXT_DEVT flag, which will allocate minor numbers
in major 259 for partitions past disk->minors.

Also remove the use of disk_devt to determine devidx from md->disk.
md->disk->first_minor is always initialized from devidx and can
always be used to recover it.

CC: Chris Ball <cjb@laptop.org>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dima Zavin <dima@android.com>
Signed-off-by: Colin Cross <ccross@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/mmc/card/block.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index edac9ac..f69f948 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -102,11 +102,7 @@ static void mmc_blk_put(struct mmc_blk_data *md)
 	mutex_lock(&open_lock);
 	md->usage--;
 	if (md->usage == 0) {
-		int devmaj = MAJOR(disk_devt(md->disk));
-		int devidx = MINOR(disk_devt(md->disk)) / perdev_minors;
-
-		if (!devmaj)
-			devidx = md->disk->first_minor / perdev_minors;
+		int devidx = md->disk->first_minor / perdev_minors;
 
 		blk_cleanup_queue(md->queue.queue);
 
@@ -623,6 +619,7 @@ static struct mmc_blk_data *mmc_blk_alloc(struct mmc_card *card)
 	md->disk->private_data = md;
 	md->disk->queue = md->queue.queue;
 	md->disk->driverfs_dev = &card->dev;
+	md->disk->flags = GENHD_FL_EXT_DEVT;
 	set_disk_ro(md->disk, md->read_only);
 
 	/*
-- 
1.7.3.2.146.gca209

[PATCH 3/6] sdhci: Always pass clock request value zero to set_clock host op

[John Stultz]

From: Todd Poynor <toddpoynor@google.com>

To allow the set_clock host op to disable the SDCLK source when not
needed, always call the host op when the requested clock speed is
zero.  Do this even if host->clock already equals zero, because
the SDHCI driver may set that value (without calling the host op)
to force an update at the next (non-zero-speed) call.

CC: Chris Ball <cjb@laptop.org>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dima Zavin <dima@android.com>
Change-Id: If99230d76138679b5767f77cb925f15408ae518e
Signed-off-by: Todd Poynor <toddpoynor@google.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/mmc/host/sdhci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9e15f41..3820adf 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -992,7 +992,7 @@ static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	u16 clk;
 	unsigned long timeout;
 
-	if (clock == host->clock)
+	if (clock && clock == host->clock)
 		return;
 
 	if (host->ops->set_clock) {
-- 
1.7.3.2.146.gca209

[PATCH 4/6] mmc: sd: Add new CONFIG_MMC_PARANOID_SD_INIT for enabling retries during SD detection

[John Stultz]

From: San Mehat <san@google.com>

CC: Chris Ball <cjb@laptop.org>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dima Zavin <dima@android.com>
Signed-off-by: San Mehat <san@google.com>
[Add depends on EXPERIMENTAL -jstultz]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/mmc/core/Kconfig |    9 +++++++++
 drivers/mmc/core/sd.c    |   22 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/Kconfig b/drivers/mmc/core/Kconfig
index ef10387..ffc671e 100644
--- a/drivers/mmc/core/Kconfig
+++ b/drivers/mmc/core/Kconfig
@@ -27,3 +27,12 @@ config MMC_CLKGATE
 	  support handling this in order for it to be of any use.
 
 	  If unsure, say N.
+
+config MMC_PARANOID_SD_INIT
+	bool "Enable paranoid SD card initialization (EXPERIMENTAL)"
+	depends on EXPERIMENTAL
+	help
+	  If you say Y here, the MMC layer will be extra paranoid
+	  about re-trying SD init requests. This can be a useful
+	  work-around for buggy controllers and hardware. Enable
+	  if you are experiencing issues with SD detection.
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 6dac89f..c0f14cb 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -403,6 +403,9 @@ struct device_type sd_type = {
 int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid)
 {
 	int err;
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+	int retries;
+#endif
 
 	/*
 	 * Since we're changing the OCR value, we seem to
@@ -482,7 +485,26 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
 		/*
 		 * Fetch switch information from card.
 		 */
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+		for (retries = 1; retries <= 3; retries++) {
+			err = mmc_read_switch(card);
+			if (!err) {
+				if (retries > 1) {
+					printk(KERN_WARNING
+					       "%s: recovered\n",
+					       mmc_hostname(host));
+				}
+				break;
+			} else {
+				printk(KERN_WARNING
+				       "%s: read switch failed (attempt %d)\n",
+				       mmc_hostname(host), retries);
+			}
+		}
+#else
 		err = mmc_read_switch(card);
+#endif
+
 		if (err)
 			return err;
 	}
-- 
1.7.3.2.146.gca209

[PATCH 5/6] mmc: sd: When resuming, try a little harder to init the card

[John Stultz]

From: San Mehat <san@android.com>

CC: Chris Ball <cjb@laptop.org>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Dima Zavin <dima@android.com>
Signed-off-by: San Mehat <san@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/mmc/core/sd.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index c0f14cb..7b6cab2 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -734,12 +734,31 @@ static int mmc_sd_suspend(struct mmc_host *host)
 static int mmc_sd_resume(struct mmc_host *host)
 {
 	int err;
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+	int retries;
+#endif
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
 
 	mmc_claim_host(host);
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+	retries = 5;
+	while (retries) {
+		err = mmc_sd_init_card(host, host->ocr, host->card);
+
+		if (err) {
+			printk(KERN_ERR "%s: Re-init card rc = %d (retries = %d)\n",
+			       mmc_hostname(host), err, retries);
+			mdelay(5);
+			retries--;
+			continue;
+		}
+		break;
+	}
+#else
 	err = mmc_sd_init_card(host, host->ocr, host->card);
+#endif
 	mmc_release_host(host);
 
 	return err;
-- 
1.7.3.2.146.gca209

[PATCH 6/6] mmc: sd: Add retries in re-detection

[John Stultz]

From: San Mehat <san@android.com>

Signed-off-by: San Mehat <san@android.com>

mmc: sd: Remove debugging printk

CC: Chris Ball <cjb@laptop.org>
CC: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Dima Zavin <dima@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/mmc/core/sd.c |   49 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 7b6cab2..24e1a85 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -403,9 +403,6 @@ struct device_type sd_type = {
 int mmc_sd_get_cid(struct mmc_host *host, u32 ocr, u32 *cid)
 {
 	int err;
-#ifdef CONFIG_MMC_PARANOID_SD_INIT
-	int retries;
-#endif
 
 	/*
 	 * Since we're changing the OCR value, we seem to
@@ -459,6 +456,9 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
 	bool reinit)
 {
 	int err;
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+	int retries;
+#endif
 
 	if (!reinit) {
 		/*
@@ -685,7 +685,10 @@ static void mmc_sd_remove(struct mmc_host *host)
  */
 static void mmc_sd_detect(struct mmc_host *host)
 {
-	int err;
+	int err = 0;
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+        int retries = 5;
+#endif
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
@@ -695,8 +698,23 @@ static void mmc_sd_detect(struct mmc_host *host)
 	/*
 	 * Just check if our card has been removed.
 	 */
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+	while(retries) {
+		err = mmc_send_status(host->card, NULL);
+		if (err) {
+			retries--;
+			udelay(5);
+			continue;
+		}
+		break;
+	}
+	if (!retries) {
+		printk(KERN_ERR "%s(%s): Unable to re-detect card (%d)\n",
+		       __func__, mmc_hostname(host), err);
+	}
+#else
 	err = mmc_send_status(host->card, NULL);
-
+#endif
 	mmc_release_host(host);
 
 	if (err) {
@@ -810,6 +828,9 @@ int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
 	u32 ocr;
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+	int retries;
+#endif
 
 	BUG_ON(!host);
 	WARN_ON(!host->claimed);
@@ -865,9 +886,27 @@ int mmc_attach_sd(struct mmc_host *host)
 	/*
 	 * Detect and init the card.
 	 */
+#ifdef CONFIG_MMC_PARANOID_SD_INIT
+	retries = 5;
+	while (retries) {
+		err = mmc_sd_init_card(host, host->ocr, NULL);
+		if (err) {
+			retries--;
+			continue;
+		}
+		break;
+	}
+
+	if (!retries) {
+		printk(KERN_ERR "%s: mmc_sd_init_card() failure (err = %d)\n",
+		       mmc_hostname(host), err);
+		goto err;
+	}
+#else
 	err = mmc_sd_init_card(host, host->ocr, NULL);
 	if (err)
 		goto err;
+#endif
 
 	mmc_release_host(host);
 	err = mmc_add_card(host->card);
-- 
1.7.3.2.146.gca209

Re: [PATCH 1/6] mmc: block: Resume multi-block reads after transient read errors.

[Arnd Bergmann]

On Saturday 23 April 2011, John Stultz wrote:
> From: David Ding <david.j.ding@motorola.com>
> 
> CC: Chris Ball <cjb@laptop.org>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Dima Zavin <dima@android.com>
> Signed-off-by: Bentao Zou <bzou1@motorola.com>
> Signed-off-by: David Ding <david.j.ding@motorola.com>
> Signed-off-by: San Mehat <san@google.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>

The disable_multi logic was introduced in 6a79e391 "mmc_block: ensure
all sectors that do not have errors are read" by Adrian Hunter. Maybe
he can comment on this.

My impression is that the code makes more sense without this
add-on patch, because the flag is set for exactly one request
and makes mmc_blk_issue_rw_rq issue all blocks in that request
one by one up to the first error, while after the patch,
we would read one sector, then read the remaining request at
once, fail, and read the next sector, and so on.

If the problem is transient read errors, a better solution might
be to retry the entire request before doing it one sector at a time.

>  drivers/mmc/card/block.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 61d233a..edac9ac 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -440,6 +440,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
>  				continue;
>  			}
>  			status = get_card_status(card, req);
> +		} else if (disable_multi == 1) {
> +			disable_multi = 0;
>  		}
>  
>  		if (brq.cmd.error) {
> -- 
> 1.7.3.2.146.gca209
> 
> 

Re: [PATCH 2/6] mmc_block: Allow more than 8 partitions per card

[Arnd Bergmann]

On Saturday 23 April 2011, John Stultz wrote:
> From: Colin Cross <ccross@android.com>
> 
> Set the GENHD_FL_EXT_DEVT flag, which will allocate minor numbers
> in major 259 for partitions past disk->minors.
> 
> Also remove the use of disk_devt to determine devidx from md->disk.
> md->disk->first_minor is always initialized from devidx and can
> always be used to recover it.
> 
> CC: Chris Ball <cjb@laptop.org>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Dima Zavin <dima@android.com>
> Signed-off-by: Colin Cross <ccross@android.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>

The new code looks reasonable, but wouldn't changing this be incompatible
with existing root file systems that contain static device nodes?

	Arnd

Re: [PATCH 4/6] mmc: sd: Add new CONFIG_MMC_PARANOID_SD_INIT for enabling retries during SD detection

[Arnd Bergmann]

On Saturday 23 April 2011, John Stultz wrote:
> @@ -482,7 +485,26 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>  		/*
>  		 * Fetch switch information from card.
>  		 */
> +#ifdef CONFIG_MMC_PARANOID_SD_INIT
> +		for (retries = 1; retries <= 3; retries++) {
> +			err = mmc_read_switch(card);
> +			if (!err) {
> +				if (retries > 1) {
> +					printk(KERN_WARNING
> +					       "%s: recovered\n",
> +					       mmc_hostname(host));
> +				}
> +				break;
> +			} else {
> +				printk(KERN_WARNING
> +				       "%s: read switch failed (attempt %d)\n",
> +				       mmc_hostname(host), retries);
> +			}
> +		}
> +#else
>  		err = mmc_read_switch(card);
> +#endif
> +

I see no reason to have this as a compile time option. Having retries in here
might be useful, but a better place would be inside of mmc_read_switch()
or mmc_sd_switch() so it is only done if the error comes from the controller,
not for cases where we know it will fail (e.g. !(card->csd.cmdclass & CCC_SWITCH)).

I would also recommend being a bit less noisy, e.g. print a warning only
after either giving up or succeeding a retry.

	Arnd

Re: [PATCH 5/6] mmc: sd: When resuming, try a little harder to init the card

[Arnd Bergmann]

On Saturday 23 April 2011, John Stultz wrote:
> From: San Mehat <san@android.com>
> 
> CC: Chris Ball <cjb@laptop.org>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Dima Zavin <dima@android.com>
> Signed-off-by: San Mehat <san@android.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/mmc/core/sd.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index c0f14cb..7b6cab2 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -734,12 +734,31 @@ static int mmc_sd_suspend(struct mmc_host *host)
>  static int mmc_sd_resume(struct mmc_host *host)
>  {
>  	int err;
> +#ifdef CONFIG_MMC_PARANOID_SD_INIT
> +	int retries;
> +#endif
>  
>  	BUG_ON(!host);
>  	BUG_ON(!host->card);
>  
>  	mmc_claim_host(host);
> +#ifdef CONFIG_MMC_PARANOID_SD_INIT
> +	retries = 5;
> +	while (retries) {
> +		err = mmc_sd_init_card(host, host->ocr, host->card);
> +
> +		if (err) {
> +			printk(KERN_ERR "%s: Re-init card rc = %d (retries = %d)\n",
> +			       mmc_hostname(host), err, retries);
> +			mdelay(5);
> +			retries--;
> +			continue;
> +		}
> +		break;
> +	}
> +#else
>  	err = mmc_sd_init_card(host, host->ocr, host->card);
> +#endif
>  	mmc_release_host(host);
>  

Again, the configuration option should not be there. Either it is needed for
some hardware or not.

The mdelay(5) is not ok, we must never spin in a kernel thread for that long.
Substituting it for msleep(5) solves that, but could still be considered
inappropriate during the boot process by some people -- a 25 ms delay for
each mmc host may be noticeable when people wait for the boot to complete.

It needs at least an explanation about why the delay is used here.

	Arnd

Re: [PATCH 6/6] mmc: sd: Add retries in re-detection

[Arnd Bergmann]

On Saturday 23 April 2011, John Stultz wrote:
> From: San Mehat <san@android.com>
> 
> Signed-off-by: San Mehat <san@android.com>

Same comments apply as in the previous patch. Maybe San Mehat can
comment on the intentions of this patch. Is there a specific hardware
that is causing this trouble.

> mmc: sd: Remove debugging printk

Stray output in changelog?

	Arnd

Re: [PATCH 2/6] mmc_block: Allow more than 8 partitions per card

[Colin Cross]

On Tue, Apr 26, 2011 at 6:22 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 23 April 2011, John Stultz wrote:
>> From: Colin Cross <ccross@android.com>
>>
>> Set the GENHD_FL_EXT_DEVT flag, which will allocate minor numbers
>> in major 259 for partitions past disk->minors.
>>
>> Also remove the use of disk_devt to determine devidx from md->disk.
>> md->disk->first_minor is always initialized from devidx and can
>> always be used to recover it.
>>
>> CC: Chris Ball <cjb@laptop.org>
>> CC: Arnd Bergmann <arnd@arndb.de>
>> CC: Dima Zavin <dima@android.com>
>> Signed-off-by: Colin Cross <ccross@android.com>
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>
> The new code looks reasonable, but wouldn't changing this be incompatible
> with existing root file systems that contain static device nodes?

I don't think so.  Without this change, /dev/mmcblk0p1 will be (179,
1), /dev/mmcblk0p7 will be (179, 7), and /dev/mmcblk0p8 will be
dropped.  After this change, /dev/mmcblk0p1-7 will be the same, but
/dev/mmcblk0p8 will be (259, <random number>).  A root file system
with static inodes will still have access to partitions 1-7, and will
still not have access to dynamically-assigned partition 8.

Re: [PATCH 2/6] mmc_block: Allow more than 8 partitions per card

[Arnd Bergmann]

On Tuesday 26 April 2011, Colin Cross wrote:
> On Tue, Apr 26, 2011 at 6:22 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Saturday 23 April 2011, John Stultz wrote:
> >> From: Colin Cross <ccross@android.com>
> >>
> >> Set the GENHD_FL_EXT_DEVT flag, which will allocate minor numbers
> >> in major 259 for partitions past disk->minors.
> >>
> >> Also remove the use of disk_devt to determine devidx from md->disk.
> >> md->disk->first_minor is always initialized from devidx and can
> >> always be used to recover it.
> >>
> >> CC: Chris Ball <cjb@laptop.org>
> >> CC: Arnd Bergmann <arnd@arndb.de>
> >> CC: Dima Zavin <dima@android.com>
> >> Signed-off-by: Colin Cross <ccross@android.com>
> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
> >
> > The new code looks reasonable, but wouldn't changing this be incompatible
> > with existing root file systems that contain static device nodes?
> 
> I don't think so.  Without this change, /dev/mmcblk0p1 will be (179,
> 1), /dev/mmcblk0p7 will be (179, 7), and /dev/mmcblk0p8 will be
> dropped.  After this change, /dev/mmcblk0p1-7 will be the same, but
> /dev/mmcblk0p8 will be (259, <random number>).  A root file system
> with static inodes will still have access to partitions 1-7, and will
> still not have access to dynamically-assigned partition 8.

Ah, I see.

The patch looks good to me then.

	Arnd

Re: [PATCH 1/6] mmc: block: Resume multi-block reads after transient read errors.

[Adrian Hunter]

On 26/04/11 16:19, Arnd Bergmann wrote:
> On Saturday 23 April 2011, John Stultz wrote:
>> From: David Ding<david.j.ding@motorola.com>
>>
>> CC: Chris Ball<cjb@laptop.org>
>> CC: Arnd Bergmann<arnd@arndb.de>
>> CC: Dima Zavin<dima@android.com>
>> Signed-off-by: Bentao Zou<bzou1@motorola.com>
>> Signed-off-by: David Ding<david.j.ding@motorola.com>
>> Signed-off-by: San Mehat<san@google.com>
>> Signed-off-by: John Stultz<john.stultz@linaro.org>
>
> The disable_multi logic was introduced in 6a79e391 "mmc_block: ensure
> all sectors that do not have errors are read" by Adrian Hunter. Maybe
> he can comment on this.
>
> My impression is that the code makes more sense without this
> add-on patch, because the flag is set for exactly one request
> and makes mmc_blk_issue_rw_rq issue all blocks in that request
> one by one up to the first error, while after the patch,
> we would read one sector, then read the remaining request at
> once, fail, and read the next sector, and so on.
>
> If the problem is transient read errors, a better solution might
> be to retry the entire request before doing it one sector at a time.

Having to guess what the patch is for is pretty sad.  Is writing
commit messages so hard.

The word "transient" suggests the errors might disappear, in which case
the entire read should be retried some number of times.

The big problem with reading 1 sector at a time is that it
can be quite slow (orders of magnitude slower).  Possibly
this patch is aimed at that.  Somewhere there is a patch to
change the retries from 1 sector at a time to 1 segment
at a time, which helps a bit.  Possibly it would help to bisect
the number sectors/segments. i.e.

	read error
		read half of the number of sectors/segments
		that were read last time (but always read at
		least 1)
	no read error
		read all the remaining sectors/segments

This patch does the second half of that approach.

>
>>   drivers/mmc/card/block.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index 61d233a..edac9ac 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -440,6 +440,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *req)
>>   				continue;
>>   			}
>>   			status = get_card_status(card, req);
>> +		} else if (disable_multi == 1) {
>> +			disable_multi = 0;
>>   		}
>>
>>   		if (brq.cmd.error) {
>> --
>> 1.7.3.2.146.gca209
>>
>>
>
>

Re: [PATCH 1/6] mmc: block: Resume multi-block reads after transient read errors.

[Andrei Warkentin]

On Wed, Apr 27, 2011 at 3:16 AM, Adrian Hunter <adrian.hunter@nokia.com> wrote:
> On 26/04/11 16:19, Arnd Bergmann wrote:
>>
>> On Saturday 23 April 2011, John Stultz wrote:
>>>
>>> From: David Ding<david.j.ding@motorola.com>
>>>
>>> CC: Chris Ball<cjb@laptop.org>
>>> CC: Arnd Bergmann<arnd@arndb.de>
>>> CC: Dima Zavin<dima@android.com>
>>> Signed-off-by: Bentao Zou<bzou1@motorola.com>
>>> Signed-off-by: David Ding<david.j.ding@motorola.com>
>>> Signed-off-by: San Mehat<san@google.com>
>>> Signed-off-by: John Stultz<john.stultz@linaro.org>
>>
>> The disable_multi logic was introduced in 6a79e391 "mmc_block: ensure
>> all sectors that do not have errors are read" by Adrian Hunter. Maybe
>> he can comment on this.
>>
>> My impression is that the code makes more sense without this
>> add-on patch, because the flag is set for exactly one request
>> and makes mmc_blk_issue_rw_rq issue all blocks in that request
>> one by one up to the first error, while after the patch,
>> we would read one sector, then read the remaining request at
>> once, fail, and read the next sector, and so on.
>>

You could definitely interpret the retry path as being:
- do a probe sector-sized read/write
- if it succeeds, pretend everything is fine and retry with disable_multi=0
- else the probed sector must the point of failure

...it would mean more retrying in the case of an actual card failure
mode (at which point you're not doing much anyway), and drastic
improvement in the case of some one-time (or not so one-time) host
glitches...

The other thought I have is that it's not ideal to overcomplicate the
logic inside issue_rw_rq (i.e. bisection stuff). Until Per's
refactoring as part of async issue it was already too big :-).

What do you think?

Anyway I hope the patch owner weighs in.

A