[PATCH] Fix sd/sdio/mmc initialization frequency retries

[PATCH] Fix sd/sdio/mmc initialization frequency retries

[Andy Ross]

I'm working with a eMMC device that broke after this commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=88ae8b86648

The older hack I'd been given hard-coded the initialisation frequency
at 200kHz (instead of 400kHz in the original upstream) and works fine.

But the new code tries to dynamically detect the highest frequency
that will work, but as far as I can tell the logic is wrong.  It will
continue trying after failed mmc_send_*_cond() calls, but *not* after
failures later on in mmc_attach_{sd,sdio,mmc}.  My boards (at 400kHz)
are getting successful returns from mmc_send_op_cond(), but failing in
mmc_attach_mmc().  If I retry at 300, it works.

Tested on the same controller using a sdhc card.  No idea how the
extra retries will affect other hardware (though as the original code
was fixed at 400, presumably very few devices will care?).

The attached patch fixes that error, and does some rewriting that
should hopefully make review easier:

+ The freqs array should be static
+ No need for locking around atomic rescan_disable test
+ Remove adjacent bus_put/get calls.
+ Move repeated claim/release host calls out of mmc_rescan loop
+ Hopefully clearer iteration logic for f_init
+ Remove duplicate mmc_attach_sd() logic after sdio failures
+ Move mmc_send_*_cond() calls into mmc_attach_*() functions, as they
  are only called from one place and are the only consumers of the ocr
  return value.
+ Make mmc_attach_*() functions symmetric with respect to
  mmc_claim/release_host().  The existing code is called with the lock
  and releases it before returning, which was really confusing to me
  when debugging this issue.  They now hold the lock throughout,
  releasing across some interior calls to preserve existing behavior.

>From 0357d0d07d4985de43c814a9fea02d13753c121c Mon Sep 17 00:00:00 2001
From: Andy Ross <andy.ross@windriver.com>
Date: Tue, 28 Dec 2010 09:04:10 -0800
Subject: [PATCH] Fix sd/sdio/mmc initialization frequency retries

Rewrite and clean up mmc_rescan() to properly retry frequencies lower
than 400kHz.  Failures can happen both in sd_send_* calls and
mmc_attach_*.  Symmetrize claim/release logic in mmc_attach_* API, and
move the sd_send_* calls there to make mmc_rescan easier to read.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
 drivers/mmc/core/core.c |   86 ++++++++--------------------------------------
 drivers/mmc/core/core.h |    6 ++--
 drivers/mmc/core/mmc.c  |   12 ++++--
 drivers/mmc/core/sd.c   |   10 ++++--
 drivers/mmc/core/sdio.c |   17 ++++++---
 5 files changed, 44 insertions(+), 87 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 31ae07a..c92de04 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1426,23 +1426,13 @@ EXPORT_SYMBOL(mmc_set_blocklen);

 void mmc_rescan(struct work_struct *work)
 {
+	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 	struct mmc_host *host =
 		container_of(work, struct mmc_host, detect.work);
-	u32 ocr;
-	int err;
-	unsigned long flags;
 	int i;
-	const unsigned freqs[] = { 400000, 300000, 200000, 100000 };

-	spin_lock_irqsave(&host->lock, flags);
-
-	if (host->rescan_disable) {
-		spin_unlock_irqrestore(&host->lock, flags);
+	if (host->rescan_disable)
 		return;
-	}
-
-	spin_unlock_irqrestore(&host->lock, flags);
-

 	mmc_bus_get(host);

@@ -1450,19 +1440,12 @@ void mmc_rescan(struct work_struct *work)
 	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
 		host->bus_ops->detect(host);

-	mmc_bus_put(host);
-
-
-	mmc_bus_get(host);
-
 	/* if there still is a card present, stop here */
 	if (host->bus_ops != NULL) {
 		mmc_bus_put(host);
 		goto out;
 	}

-	/* detect a newly inserted card */
-
 	/*
 	 * Only we can add a new handler, so it's safe to
 	 * release the lock here.
@@ -1472,17 +1455,11 @@ void mmc_rescan(struct work_struct *work)
 	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
 		goto out;

-	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
-		mmc_claim_host(host);
+	mmc_claim_host(host);
+	host->f_init = freqs[0];
+	for (i=0; i<ARRAY_SIZE(freqs) && host->f_init > host->f_min; i++) {
+		host->f_init = max(freqs[i], host->f_min);

-		if (freqs[i] >= host->f_min)
-			host->f_init = freqs[i];
-		else if (!i || freqs[i-1] > host->f_min)
-			host->f_init = host->f_min;
-		else {
-			mmc_release_host(host);
-			goto out;
-		}
 #ifdef CONFIG_MMC_DEBUG
 		pr_info("%s: %s: trying to init card at %u Hz\n",
 			mmc_hostname(host), __func__, host->f_init);
@@ -1493,50 +1470,17 @@ void mmc_rescan(struct work_struct *work)

 		mmc_send_if_cond(host, host->ocr_avail);

-		/*
-		 * First we search for SDIO...
-		 */
-		err = mmc_send_io_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sdio(host, ocr)) {
-				mmc_claim_host(host);
-				/*
-				 * Try SDMEM (but not MMC) even if SDIO
-				 * is broken.
-				 */
-				if (mmc_send_app_op_cond(host, 0, &ocr))
-					goto out_fail;
-
-				if (mmc_attach_sd(host, ocr))
-					mmc_power_off(host);
-			}
-			goto out;
-		}
-
-		/*
-		 * ...then normal SD...
-		 */
-		err = mmc_send_app_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sd(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
-
-		/*
-		 * ...and finally MMC.
-		 */
-		err = mmc_send_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_mmc(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
+		/* Try SDIO, then SD, then MMC */
+		if(mmc_attach_sdio(host) == 0)
+			break;
+		else if(mmc_attach_sd(host) == 0)
+			break;
+		else if(mmc_attach_mmc(host) == 0)
+			break;

-out_fail:
-		mmc_release_host(host);
-		mmc_power_off(host);
+                mmc_power_off(host);
 	}
+	mmc_release_host(host);
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 77240cd..54de886 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -54,9 +54,9 @@ void mmc_rescan(struct work_struct *work);
 void mmc_start_host(struct mmc_host *host);
 void mmc_stop_host(struct mmc_host *host);

-int mmc_attach_mmc(struct mmc_host *host, u32 ocr);
-int mmc_attach_sd(struct mmc_host *host, u32 ocr);
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr);
+int mmc_attach_mmc(struct mmc_host *host);
+int mmc_attach_sd(struct mmc_host *host);
+int mmc_attach_sdio(struct mmc_host *host);

 /* Module parameters */
 extern int use_spi_crc;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 77f93c3..cad2eaf 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -737,13 +737,17 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for MMC card init.
  */
-int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
+int mmc_attach_mmc(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;

 	BUG_ON(!host);
 	WARN_ON(!host->claimed);

+ 	if((err = mmc_send_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_attach_bus_ops(host);

 	/*
@@ -784,20 +788,20 @@ int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
 		goto err;

 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;

 	return 0;

 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
-	host->card = NULL;
 	mmc_claim_host(host);
+	host->card = NULL;
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);

 	printk(KERN_ERR "%s: error %d whilst initialising MMC card\n",
 		mmc_hostname(host), err);
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 49da4df..9fad37a 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -764,13 +764,17 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for SD card init.
  */
-int mmc_attach_sd(struct mmc_host *host, u32 ocr)
+int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;

 	BUG_ON(!host);
 	WARN_ON(!host->claimed);

+	if((err = mmc_send_app_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_sd_attach_bus_ops(host);

 	/*
@@ -820,20 +824,20 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
 		goto err;

 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;

 	return 0;

 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
 	host->card = NULL;
 	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);

 	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
 		mmc_hostname(host), err);
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index efef5f9..5d2f33d 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -690,15 +690,18 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 /*
  * Starting point for SDIO card init.
  */
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
+int mmc_attach_sdio(struct mmc_host *host)
 {
-	int err;
-	int i, funcs;
+	int err, i, funcs;
+	u32 ocr;
 	struct mmc_card *card;

 	BUG_ON(!host);
 	WARN_ON(!host->claimed);

+	if((err =mmc_send_io_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_attach_bus(host, &mmc_sdio_ops);

 	/*
@@ -769,12 +772,12 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
 	}

-	mmc_release_host(host);
-
 	/*
 	 * First add the card to the driver model...
 	 */
+	mmc_release_host(host);
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_added;

@@ -792,15 +795,17 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)

 remove_added:
 	/* Remove without lock if the device has been added. */
+	mmc_release_host(host);
 	mmc_sdio_remove(host);
 	mmc_claim_host(host);
 remove:
 	/* And with lock if it hasn't been added. */
+	mmc_release_host(host);
 	if (host->card)
 		mmc_sdio_remove(host);
+	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);

 	printk(KERN_ERR "%s: error %d whilst initialising SDIO card\n",
 		mmc_hostname(host), err);
-- 
1.7.1

Re: [PATCH] Fix sd/sdio/mmc initialization frequency retries

[Chris Ball]

Hi Andy,

This patch doesn't apply against mmc-next, which makes it hard to review
properly; could you rebase/resend?

I like the look of these changes, just a few comments:

On Tue, Dec 28, 2010 at 09:15:05AM -0800, Andy Ross wrote:
> @@ -1450,19 +1440,12 @@ void mmc_rescan(struct work_struct *work)
>  	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
>  		host->bus_ops->detect(host);
> 
> -	mmc_bus_put(host);
> -
> -
> -	mmc_bus_get(host);
> -
>  	/* if there still is a card present, stop here */
>  	if (host->bus_ops != NULL) {
>  		mmc_bus_put(host);
>  		goto out;
>  	}

Let's leave these in -- I think we're depending on the side-effect of 
mmc_bus_put() clearing out host->bus_ops.  I'll check and submit a
separate patch adding a comment there explaining what's going on.

> +		/* Try SDIO, then SD, then MMC */
> +		if(mmc_attach_sdio(host) == 0)
> +			break;
> +		else if(mmc_attach_sd(host) == 0)
> +			break;
> +		else if(mmc_attach_mmc(host) == 0)
> +			break;

Space after if, and perhaps let's use !mmc_attach_*() rather than == 0
for brevity.  I guess we could also consider:

		/* Try SDIO, then SD, then MMC */
		if (mmc_attach_sdio(host) && mmc_attach_sd(host) &&
		    mmc_attach_mmc(host))
			mmc_power_off(host);

	mmc_release_host(host);

which would save quite a few lines, but perhaps it's less clear (or not
equivalent somehow).  What do you think?

> -out_fail:
> -		mmc_release_host(host);
> -		mmc_power_off(host);
> +              mmc_power_off(host);

This looks misaligned.

> + 	if((err = mmc_send_op_cond(host, 0, &ocr)))
> +		return err;

Redundant parens, and space after if.

> +	if((err =mmc_send_io_op_cond(host, 0, &ocr)))

As above, plus space after =.

Thanks!  You might also consider splitting this up into two separate
patches, but I'll leave that up to you.

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

Re: [PATCH] Fix sd/sdio/mmc initialization frequency retries

[Hein_Tibosch]

Hi Andy,

It looks like a good cleanup!

On 29-12-2010 01:15, Andy Ross wrote:
> I'm working with a eMMC device that broke after this commit:
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=88ae8b86648
>
> The older hack I'd been given hard-coded the initialisation frequency
> at 200kHz (instead of 400kHz in the original upstream) and works fine.
>
> But the new code tries to dynamically detect the highest frequency
> that will work, but as far as I can tell the logic is wrong.  It will
> continue trying after failed mmc_send_*_cond() calls, but *not* after
> failures later on in mmc_attach_{sd,sdio,mmc}.  My boards (at 400kHz)
> are getting successful returns from mmc_send_op_cond(), but failing in
> mmc_attach_mmc().  If I retry at 300, it works.
>
> Tested on the same controller using a sdhc card.  No idea how the
> extra retries will affect other hardware (though as the original code
> was fixed at 400, presumably very few devices will care?).

And before the fixed 400 Khz, it went like this:

    host->ios.clock = host->f_min;

So with the older versions, your card(s) would have initialized fine,
and mine as well.

> The attached patch fixes that error, and does some rewriting that
> should hopefully make review easier:
>
> + The freqs array should be static
> + No need for locking around atomic rescan_disable test
> + Remove adjacent bus_put/get calls.
> + Move repeated claim/release host calls out of mmc_rescan loop
> + Hopefully clearer iteration logic for f_init
> + Remove duplicate mmc_attach_sd() logic after sdio failures
> + Move mmc_send_*_cond() calls into mmc_attach_*() functions, as they
>   are only called from one place and are the only consumers of the ocr
>   return value.
> + Make mmc_attach_*() functions symmetric with respect to
>   mmc_claim/release_host().  The existing code is called with the lock
>   and releases it before returning, which was really confusing to me
>   when debugging this issue.  They now hold the lock throughout,
>   releasing across some interior calls to preserve existing behavior.
>
> >From 0357d0d07d4985de43c814a9fea02d13753c121c Mon Sep 17 00:00:00 2001
> From: Andy Ross <andy.ross@windriver.com>
> Date: Tue, 28 Dec 2010 09:04:10 -0800
> Subject: [PATCH] Fix sd/sdio/mmc initialization frequency retries
>
> Rewrite and clean up mmc_rescan() to properly retry frequencies lower
> than 400kHz.  Failures can happen both in sd_send_* calls and
> mmc_attach_*.  Symmetrize claim/release logic in mmc_attach_* API, and
> move the sd_send_* calls there to make mmc_rescan easier to read.
>
> Signed-off-by: Andy Ross <andy.ross@windriver.com>
> ---
>  drivers/mmc/core/core.c |   86 ++++++++--------------------------------------
>  drivers/mmc/core/core.h |    6 ++--
>  drivers/mmc/core/mmc.c  |   12 ++++--
>  drivers/mmc/core/sd.c   |   10 ++++--
>  drivers/mmc/core/sdio.c |   17 ++++++---
>  5 files changed, 44 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 31ae07a..c92de04 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1426,23 +1426,13 @@ EXPORT_SYMBOL(mmc_set_blocklen);
>
>  void mmc_rescan(struct work_struct *work)
>  {
> +	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
>  	struct mmc_host *host =
>  		container_of(work, struct mmc_host, detect.work);
> -	u32 ocr;
> -	int err;
> -	unsigned long flags;
>  	int i;
> -	const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
>
> -	spin_lock_irqsave(&host->lock, flags);
> -
> -	if (host->rescan_disable) {
> -		spin_unlock_irqrestore(&host->lock, flags);
> +	if (host->rescan_disable)
>  		return;
> -	}
> -
> -	spin_unlock_irqrestore(&host->lock, flags);
> -
>
>  	mmc_bus_get(host);
>
> @@ -1450,19 +1440,12 @@ void mmc_rescan(struct work_struct *work)
>  	if ((host->bus_ops != NULL) && host->bus_ops->detect && !host->bus_dead)
>  		host->bus_ops->detect(host);
>
> -	mmc_bus_put(host);
> -
> -
> -	mmc_bus_get(host);
> -
>  	/* if there still is a card present, stop here */
>  	if (host->bus_ops != NULL) {
>  		mmc_bus_put(host);
>  		goto out;
>  	}
>
> -	/* detect a newly inserted card */
> -
>  	/*
>  	 * Only we can add a new handler, so it's safe to
>  	 * release the lock here.
> @@ -1472,17 +1455,11 @@ void mmc_rescan(struct work_struct *work)
>  	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
>  		goto out;
>
> -	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> -		mmc_claim_host(host);
> +	mmc_claim_host(host);
> +	host->f_init = freqs[0];
> +	for (i=0; i<ARRAY_SIZE(freqs) && host->f_init > host->f_min; i++) {

Suppose f_min > 400 Khz, no initialization will ever take place?

Can't you change it to:

+	for (i=0;
+		i<ARRAY_SIZE(freqs) && (host->f_init > host->f_min || !i);
+		i++) {

ans so at least f_min will be tried?

> +		host->f_init = max(freqs[i], host->f_min);
>
> -		if (freqs[i] >= host->f_min)
> -			host->f_init = freqs[i];
> -		else if (!i || freqs[i-1] > host->f_min)
> -			host->f_init = host->f_min;
> -		else {
> -			mmc_release_host(host);
> -			goto out;
> -		}
>  #ifdef CONFIG_MMC_DEBUG
>  		pr_info("%s: %s: trying to init card at %u Hz\n",
>  			mmc_hostname(host), __func__, host->f_init);
> @@ -1493,50 +1470,17 @@ void mmc_rescan(struct work_struct *work)
>
>  		mmc_send_if_cond(host, host->ocr_avail);
>
> -		/*
> -		 * First we search for SDIO...
> -		 */
> -		err = mmc_send_io_op_cond(host, 0, &ocr);
> -		if (!err) {
> -			if (mmc_attach_sdio(host, ocr)) {
> -				mmc_claim_host(host);
> -				/*
> -				 * Try SDMEM (but not MMC) even if SDIO
> -				 * is broken.
> -				 */
> -				if (mmc_send_app_op_cond(host, 0, &ocr))
> -					goto out_fail;
> -
> -				if (mmc_attach_sd(host, ocr))
> -					mmc_power_off(host);
> -			}
> -			goto out;
> -		}
> -
> -		/*
> -		 * ...then normal SD...
> -		 */
> -		err = mmc_send_app_op_cond(host, 0, &ocr);
> -		if (!err) {
> -			if (mmc_attach_sd(host, ocr))
> -				mmc_power_off(host);
> -			goto out;
> -		}
> -
> -		/*
> -		 * ...and finally MMC.
> -		 */
> -		err = mmc_send_op_cond(host, 0, &ocr);
> -		if (!err) {
> -			if (mmc_attach_mmc(host, ocr))
> -				mmc_power_off(host);
> -			goto out;
> -		}
> +		/* Try SDIO, then SD, then MMC */
> +		if(mmc_attach_sdio(host) == 0)
> +			break;
> +		else if(mmc_attach_sd(host) == 0)
> +			break;
> +		else if(mmc_attach_mmc(host) == 0)
> +			break;
>
The "else" after break is syntactically redundant, this would do the same:

+		if (!mmc_attach_sdio(host))
+			break;
+		if (!mmc_attach_sd(host))
+			break;
+		if (!mmc_attach_mmc(host))
+			break;


> -out_fail:
> -		mmc_release_host(host);
> -		mmc_power_off(host);
> +                mmc_power_off(host);
>  	}
> +	mmc_release_host(host);
>  out:
>  	if (host->caps & MMC_CAP_NEEDS_POLL)
>  		mmc_schedule_delayed_work(&host->detect, HZ);
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 77240cd..54de886 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -54,9 +54,9 @@ void mmc_rescan(struct work_struct *work);
>  void mmc_start_host(struct mmc_host *host);
>  void mmc_stop_host(struct mmc_host *host);
>
> -int mmc_attach_mmc(struct mmc_host *host, u32 ocr);
> -int mmc_attach_sd(struct mmc_host *host, u32 ocr);
> -int mmc_attach_sdio(struct mmc_host *host, u32 ocr);
> +int mmc_attach_mmc(struct mmc_host *host);
> +int mmc_attach_sd(struct mmc_host *host);
> +int mmc_attach_sdio(struct mmc_host *host);
>
>  /* Module parameters */
>  extern int use_spi_crc;
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 77f93c3..cad2eaf 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -737,13 +737,17 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
>  /*
>   * Starting point for MMC card init.
>   */
> -int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
> +int mmc_attach_mmc(struct mmc_host *host)
>  {
>  	int err;
> +	u32 ocr;
>
>  	BUG_ON(!host);
>  	WARN_ON(!host->claimed);
>
> + 	if((err = mmc_send_op_cond(host, 0, &ocr)))
> +		return err;
> +
>  	mmc_attach_bus_ops(host);
>
>  	/*
> @@ -784,20 +788,20 @@ int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
>  		goto err;
>
>  	mmc_release_host(host);
> -
>  	err = mmc_add_card(host->card);
> +	mmc_claim_host(host);
>  	if (err)
>  		goto remove_card;
>
>  	return 0;
>
>  remove_card:
> +	mmc_release_host(host);
>  	mmc_remove_card(host->card);
> -	host->card = NULL;
>  	mmc_claim_host(host);
> +	host->card = NULL;
>  err:
>  	mmc_detach_bus(host);
> -	mmc_release_host(host);
>
>  	printk(KERN_ERR "%s: error %d whilst initialising MMC card\n",
>  		mmc_hostname(host), err);
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 49da4df..9fad37a 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -764,13 +764,17 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
>  /*
>   * Starting point for SD card init.
>   */
> -int mmc_attach_sd(struct mmc_host *host, u32 ocr)
> +int mmc_attach_sd(struct mmc_host *host)
>  {
>  	int err;
> +	u32 ocr;
>
>  	BUG_ON(!host);
>  	WARN_ON(!host->claimed);
>
> +	if((err = mmc_send_app_op_cond(host, 0, &ocr)))
> +		return err;
> +
>  	mmc_sd_attach_bus_ops(host);
>
>  	/*
> @@ -820,20 +824,20 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
>  		goto err;
>
>  	mmc_release_host(host);
> -
>  	err = mmc_add_card(host->card);
>  	return 0;
>
>  remove_card:
> +	mmc_release_host(host);
>  	mmc_remove_card(host->card);
>  	host->card = NULL;
>  	mmc_claim_host(host);
>  err:
>  	mmc_detach_bus(host);
> -	mmc_release_host(host);
>
>  	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
>  		mmc_hostname(host), err);
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index efef5f9..5d2f33d 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -690,15 +690,18 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
>  /*
>   * Starting point for SDIO card init.
>   */
> -int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
> +int mmc_attach_sdio(struct mmc_host *host)
>  {
> -	int err;
> -	int i, funcs;
> +	int err, i, funcs;
> +	u32 ocr;
>  	struct mmc_card *card;
>
>  	BUG_ON(!host);
>  	WARN_ON(!host->claimed);
>
> +	if((err =mmc_send_io_op_cond(host, 0, &ocr)))
> +		return err;
> +
>  	mmc_attach_bus(host, &mmc_sdio_ops);
>
>  	/*
> @@ -769,12 +772,12 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>  			pm_runtime_enable(&card->sdio_func[i]->dev);
>  	}
>
> -	mmc_release_host(host);
> -
>  	/*
>  	 * First add the card to the driver model...
>  	 */
> +	mmc_release_host(host);
>  	err = mmc_add_card(host->card);
> +	mmc_claim_host(host);
>  	if (err)
>  		goto remove_added;
>
> @@ -792,15 +795,17 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>
>  remove_added:
>  	/* Remove without lock if the device has been added. */
> +	mmc_release_host(host);
>  	mmc_sdio_remove(host);
>  	mmc_claim_host(host);
>  remove:
>  	/* And with lock if it hasn't been added. */
> +	mmc_release_host(host);
>  	if (host->card)
>  		mmc_sdio_remove(host);
> +	mmc_claim_host(host);
>  err:
>  	mmc_detach_bus(host);
> -	mmc_release_host(host);
>
>  	printk(KERN_ERR "%s: error %d whilst initialising SDIO card\n",
>  		mmc_hostname(host), err);

Don't know why all these additions of claim/release?
In some cases it will lead to unnecessary releases, just because
later in the branch claim will be called.

I think in all 3 cases (sd/sdio/mmc) the change can be much simpler.
For instance for sd.c I would do this:

---
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 49da4df..18331eb
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -764,13 +764,17 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for SD card init.
  */
-int mmc_attach_sd(struct mmc_host *host, u32 ocr)
+int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;

 	BUG_ON(!host);
 	WARN_ON(!host->claimed);

+	if((err = mmc_send_app_op_cond(host, 0, &ocr)))
+		return err;
+
 	mmc_sd_attach_bus_ops(host);

 	/*
@@ -825,6 +829,7 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
 	if (err)
 		goto remove_card;

+	mmc_claim_host(host);
 	return 0;

 remove_card:
@@ -833,7 +838,6 @@ remove_card:
 	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);

 	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
 		mmc_hostname(host), err);
--

Or in other words: add a claim before "return 0" and remove release
after "err".

In the next days I'll test your patch on my system, which also had
problems with the fixed 400 Khz. SD-cards will initialize at less
than 280 Khz.


Hein Tibosch

Re: [PATCH] Fix sd/sdio/mmc initialization frequency retries

[Hein_Tibosch]

On 1-1-2011 04:43, Hein_Tibosch wrote:
> On 29-12-2010 01:15, Andy Ross wrote:
>> I'm working with a eMMC device that broke after this commit:
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=88ae8b86648
>>
>> The older hack I'd been given hard-coded the initialisation frequency
>> at 200kHz (instead of 400kHz in the original upstream) and works fine.
>>
>> But the new code tries to dynamically detect the highest frequency
>> that will work, but as far as I can tell the logic is wrong.  It will
>> continue trying after failed mmc_send_*_cond() calls, but *not* after
>> failures later on in mmc_attach_{sd,sdio,mmc}.  My boards (at 400kHz)
>> are getting successful returns from mmc_send_op_cond(), but failing in
>> mmc_attach_mmc().  If I retry at 300, it works.
>>
>> Tested on the same controller using a sdhc card.  No idea how the
>> extra retries will affect other hardware (though as the original code
>> was fixed at 400, presumably very few devices will care?).
>
> <snip>
>
> Don't know why all these additions of claim/release?
> In some cases it will lead to unnecessary releases, just because
> later in the branch claim will be called.
>
> I think in all 3 cases (sd/sdio/mmc) the change can be much simpler.
> For instance for sd.c I would do this:
>
> ---
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 49da4df..18331eb
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
>
> <snip>
>
> @@ -825,6 +829,7 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
>  	if (err)
>  		goto remove_card;
>
> +	mmc_claim_host(host);
>  	return 0;
>
>  remove_card:
> @@ -833,7 +838,6 @@ remove_card:
>  	mmc_claim_host(host);
>  err:
>  	mmc_detach_bus(host);
> -	mmc_release_host(host);
>
>  	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
>  		mmc_hostname(host), err);
> --
>
I tested the simplified patch as described above on AVR32 with an sd-card.

It worked well and the timing was the same as before: each frequency tried
takes about 22 ms. The third time, at 200Khz, the card got initialized.

But not much time is lost, in a meanwhile it was starting up the USB host

Hein

Re: [PATCH] Fix sd/sdio/mmc initialization frequency retries

[Andy Ross]

[Combined response to Chris and Hein, hope that's not too confusing.
And sorry about the delay, had to wait until this morning to have
access to my board to test.]

Chris Ball wrote:
> This patch doesn't apply against mmc-next, which makes it hard to review
> properly;

Yeah.  And you got the pre-checkpatch version too, thus the syntax
goofs.  Apologies, I do kernel stuff only occasionally.  New patch
below.   Applies and builds vs. mmc-next, tested on my board this
morning with the problematic eMMC device and a sdhc card.

> Let's leave these in -- I think we're depending on the side-effect
> of mmc_bus_put() clearing out host->bus_ops.  I'll check and submit
> a separate patch adding a comment there explaining what's going on.

OK, I put exactly that text in as a placeholder. :)

> Space after if, and perhaps let's use !mmc_attach_*() rather than ==
> 0 for brevity.  I guess we could also consider:
>     if (mmc_attach_sdio(host) && mmc_attach_sd(host) &&

Personally, I use the "== 0" syntax because my brain wants to read "!"
as a test for failure, not success.  But I'm fine with whatever.  I
didn't do the folded test though -- that not only flips my internal
sense of the boolean, it makes me think through the short circuit
behavior of "&&" in the transformed gauge.

Hein Tibosch wrote:
> Suppose f_min > 400 Khz, no initialization will ever take place?

Uh... yeah.  That's a bug.  Fixed in this version; I split out the
"try_freq" step so the loop contains only the frequency selection
logic.  Hopefully this is even clearer.

> The "else" after break is syntactically redundant, this would do the
> same:

Quibble: the else is *semantically* redundant. :)

The syntactic meaning of "if ... if" vs. "if ... else if" is
definitely different, and the only reason those two cases are
equivalent is because of the special behavior of the break statement
*inside* the if expressions.  I like mine for this reason (it's robust
against changes to the code inside the block), but I'm not the
maintainer.  Fixed.

> Don't know why all these additions of claim/release?  In some cases
> it will lead to unnecessary releases, just because later in the
> branch claim will be called.

It's voodoo, basically, because I didn't want to break something.  The
original code for these functions was called with the claim held, and
released it before returning.  This confused me terribly and made the
exit conditions complicated in mmc_rescan(), so I modified the
functions to hold the cookie across the call.

But some of the calls in the original were made after the release, so
I do an explicit release/claim around the calls that did *not* hold
the lock in the original because I wasn't sure what the assumptions
were.  I'm pretty sure it preserves the original semantics, but I'm
not expert enough to know whether it can be optimized.

Andy

>From 411a58a232e4266877d0af4840226661dd687474 Mon Sep 17 00:00:00 2001
From: Andy Ross <andy.ross@windriver.com>
Date: Mon, 3 Jan 2011 10:30:23 -0800
Subject: [PATCH] Fix sd/sdio/mmc initialization frequency retries

Rewrite and clean up mmc_rescan() to properly retry frequencies lower
than 400kHz.  Failures can happen both in sd_send_* calls and
mmc_attach_*.  Break out "mmc_rescan_try_freq" from the frequency
selection loop.  Symmetrize claim/release logic in mmc_attach_* API,
and move the sd_send_* calls there to make mmc_rescan easier to read.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
 drivers/mmc/core/core.c |  124 +++++++++++++++--------------------------------
 drivers/mmc/core/core.h |    6 +-
 drivers/mmc/core/mmc.c  |   13 +++--
 drivers/mmc/core/sd.c   |   11 +++-
 drivers/mmc/core/sdio.c |   18 +++++--
 5 files changed, 71 insertions(+), 101 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index a8e89f3..ef7f8cc 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1485,25 +1485,41 @@ int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen)
 }
 EXPORT_SYMBOL(mmc_set_blocklen);

+static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
+{
+	host->f_init = freq;
+
+#ifdef CONFIG_MMC_DEBUG
+	pr_info("%s: %s: trying to init card at %u Hz\n",
+		mmc_hostname(host), __func__, host->f_init);
+#endif
+	mmc_power_up(host);
+	sdio_reset(host);
+	mmc_go_idle(host);
+
+	mmc_send_if_cond(host, host->ocr_avail);
+
+	/* Order's important: probe SDIO, then SD, then MMC */
+	if (!mmc_attach_sdio(host))
+		return 0;
+	if (!mmc_attach_sd(host))
+		return 0;
+	if (!mmc_attach_mmc(host))
+		return 0;
+
+	mmc_power_off(host);
+	return -EIO;
+}
+
 void mmc_rescan(struct work_struct *work)
 {
+	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
 	struct mmc_host *host =
 		container_of(work, struct mmc_host, detect.work);
-	u32 ocr;
-	int err;
-	unsigned long flags;
 	int i;
-	const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
-
-	spin_lock_irqsave(&host->lock, flags);

-	if (host->rescan_disable) {
-		spin_unlock_irqrestore(&host->lock, flags);
+	if (host->rescan_disable)
 		return;
-	}
-
-	spin_unlock_irqrestore(&host->lock, flags);
-

 	mmc_bus_get(host);

@@ -1515,9 +1531,10 @@ void mmc_rescan(struct work_struct *work)
 	    && mmc_card_is_removable(host))
 		host->bus_ops->detect(host);

+	/* "Let's leave these in -- I think we're depending on the
+	 * side-effect of mmc_bus_put() clearing out host->bus_ops."
+	 * -cjb 2010-12-30 */
 	mmc_bus_put(host);
-
-
 	mmc_bus_get(host);

 	/* if there still is a card present, stop here */
@@ -1526,8 +1543,6 @@ void mmc_rescan(struct work_struct *work)
 		goto out;
 	}

-	/* detect a newly inserted card */
-
 	/*
 	 * Only we can add a new handler, so it's safe to
 	 * release the lock here.
@@ -1537,77 +1552,16 @@ void mmc_rescan(struct work_struct *work)
 	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
 		goto out;

+	mmc_claim_host(host);
 	for (i = 0; i < ARRAY_SIZE(freqs); i++) {
-		mmc_claim_host(host);
-
-		if (freqs[i] >= host->f_min)
-			host->f_init = freqs[i];
-		else if (!i || freqs[i-1] > host->f_min)
-			host->f_init = host->f_min;
-		else {
-			mmc_release_host(host);
-			goto out;
-		}
-#ifdef CONFIG_MMC_DEBUG
-		pr_info("%s: %s: trying to init card at %u Hz\n",
-			mmc_hostname(host), __func__, host->f_init);
-#endif
-		mmc_power_up(host);
-		sdio_reset(host);
-		mmc_go_idle(host);
-
-		mmc_send_if_cond(host, host->ocr_avail);
-
-		/*
-		 * First we search for SDIO...
-		 */
-		err = mmc_send_io_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sdio(host, ocr)) {
-				mmc_claim_host(host);
-				/*
-				 * Try SDMEM (but not MMC) even if SDIO
-				 * is broken.
-				 */
-				mmc_power_up(host);
-				sdio_reset(host);
-				mmc_go_idle(host);
-				mmc_send_if_cond(host, host->ocr_avail);
-
-				if (mmc_send_app_op_cond(host, 0, &ocr))
-					goto out_fail;
-
-				if (mmc_attach_sd(host, ocr))
-					mmc_power_off(host);
-			}
-			goto out;
-		}
-
-		/*
-		 * ...then normal SD...
-		 */
-		err = mmc_send_app_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sd(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
-
-		/*
-		 * ...and finally MMC.
-		 */
-		err = mmc_send_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_mmc(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
-
-out_fail:
-		mmc_release_host(host);
-		mmc_power_off(host);
+		if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
+			break;
+		if (freqs[i] < host->f_min)
+			break;
 	}
-out:
+	mmc_release_host(host);
+
+ out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
 }
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 026c975..ca1fdde 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -57,9 +57,9 @@ void mmc_rescan(struct work_struct *work);
 void mmc_start_host(struct mmc_host *host);
 void mmc_stop_host(struct mmc_host *host);

-int mmc_attach_mmc(struct mmc_host *host, u32 ocr);
-int mmc_attach_sd(struct mmc_host *host, u32 ocr);
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr);
+int mmc_attach_mmc(struct mmc_host *host);
+int mmc_attach_sd(struct mmc_host *host);
+int mmc_attach_sdio(struct mmc_host *host);

 /* Module parameters */
 extern int use_spi_crc;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c86dd73..16006ef 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -755,13 +755,18 @@ static void mmc_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for MMC card init.
  */
-int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
+int mmc_attach_mmc(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;

 	BUG_ON(!host);
 	WARN_ON(!host->claimed);

+	err = mmc_send_op_cond(host, 0, &ocr);
+	if (err)
+		return err;
+
 	mmc_attach_bus_ops(host);
 	if (host->ocr_avail_mmc)
 		host->ocr_avail = host->ocr_avail_mmc;
@@ -804,20 +809,20 @@ int mmc_attach_mmc(struct mmc_host *host, u32 ocr)
 		goto err;

 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;

 	return 0;

 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
-	host->card = NULL;
 	mmc_claim_host(host);
+	host->card = NULL;
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);

 	printk(KERN_ERR "%s: error %d whilst initialising MMC card\n",
 		mmc_hostname(host), err);
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index de062eb..d18c32b 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -764,13 +764,18 @@ static void mmc_sd_attach_bus_ops(struct mmc_host *host)
 /*
  * Starting point for SD card init.
  */
-int mmc_attach_sd(struct mmc_host *host, u32 ocr)
+int mmc_attach_sd(struct mmc_host *host)
 {
 	int err;
+	u32 ocr;

 	BUG_ON(!host);
 	WARN_ON(!host->claimed);

+	err = mmc_send_app_op_cond(host, 0, &ocr);
+	if (err)
+		return err;
+
 	mmc_sd_attach_bus_ops(host);
 	if (host->ocr_avail_sd)
 		host->ocr_avail = host->ocr_avail_sd;
@@ -823,20 +828,20 @@ int mmc_attach_sd(struct mmc_host *host, u32 ocr)
 		goto err;

 	mmc_release_host(host);
-
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_card;

 	return 0;

 remove_card:
+	mmc_release_host(host);
 	mmc_remove_card(host->card);
 	host->card = NULL;
 	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);

 	printk(KERN_ERR "%s: error %d whilst initialising SD card\n",
 		mmc_hostname(host), err);
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 82f4b90..5c4a54d 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -702,15 +702,19 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 /*
  * Starting point for SDIO card init.
  */
-int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
+int mmc_attach_sdio(struct mmc_host *host)
 {
-	int err;
-	int i, funcs;
+	int err, i, funcs;
+	u32 ocr;
 	struct mmc_card *card;

 	BUG_ON(!host);
 	WARN_ON(!host->claimed);

+	err = mmc_send_io_op_cond(host, 0, &ocr);
+	if (err)
+		return err;
+
 	mmc_attach_bus(host, &mmc_sdio_ops);
 	if (host->ocr_avail_sdio)
 		host->ocr_avail = host->ocr_avail_sdio;
@@ -783,12 +787,12 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 			pm_runtime_enable(&card->sdio_func[i]->dev);
 	}

-	mmc_release_host(host);
-
 	/*
 	 * First add the card to the driver model...
 	 */
+	mmc_release_host(host);
 	err = mmc_add_card(host->card);
+	mmc_claim_host(host);
 	if (err)
 		goto remove_added;

@@ -806,15 +810,17 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)

 remove_added:
 	/* Remove without lock if the device has been added. */
+	mmc_release_host(host);
 	mmc_sdio_remove(host);
 	mmc_claim_host(host);
 remove:
 	/* And with lock if it hasn't been added. */
+	mmc_release_host(host);
 	if (host->card)
 		mmc_sdio_remove(host);
+	mmc_claim_host(host);
 err:
 	mmc_detach_bus(host);
-	mmc_release_host(host);

 	printk(KERN_ERR "%s: error %d whilst initialising SDIO card\n",
 		mmc_hostname(host), err);
-- 
1.7.1

Re: [PATCH] Fix sd/sdio/mmc initialization frequency retries

[Chris Ball]

Hi Andy,

On Mon, Jan 03, 2011 at 10:36:56AM -0800, Andy Ross wrote:
> Subject: [PATCH] Fix sd/sdio/mmc initialization frequency retries
> 
> Rewrite and clean up mmc_rescan() to properly retry frequencies lower
> than 400kHz.  Failures can happen both in sd_send_* calls and
> mmc_attach_*.  Break out "mmc_rescan_try_freq" from the frequency
> selection loop.  Symmetrize claim/release logic in mmc_attach_* API,
> and move the sd_send_* calls there to make mmc_rescan easier to read.
> 
> Signed-off-by: Andy Ross <andy.ross@windriver.com>

Thanks!  Pushed to mmc-next for .38, after adding Reviewed-by: tags and
removing the mmc_bus_{put,get}() comment, since I'll take care of that
separately.

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

Re: [PATCH] Fix sd/sdio/mmc initialization frequency retries

[Chris Ball]

Hi,

On Tue, Jan 04, 2011 at 01:29:02AM +0000, Chris Ball wrote:
> Thanks!  Pushed to mmc-next for .38, after adding Reviewed-by: tags and
> removing the mmc_bus_{put,get}() comment, since I'll take care of that
> separately.

And here's the comment patch:

From: Chris Ball <cjb@laptop.org>
Date: Tue, 4 Jan 2011 12:20:22 -0500
Subject: [PATCH] mmc: Explain why we make adjacent mmc_bus_{put,get} calls during rescan.

Signed-off-by: Chris Ball <cjb@laptop.org>
---
 drivers/mmc/core/core.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 286e6ce..b8e3b97 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1529,13 +1529,15 @@ void mmc_rescan(struct work_struct *work)
 	 */
 	if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
 	    && mmc_card_is_removable(host))
 		host->bus_ops->detect(host);
 
+	/*
+	 * Let mmc_bus_put() free the bus/bus_ops if we've found that
+	 * the card is no longer present.
+	 */
 	mmc_bus_put(host);
-
-
 	mmc_bus_get(host);
 
 	/* if there still is a card present, stop here */
 	if (host->bus_ops != NULL) {
 		mmc_bus_put(host);
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child