[PATCH RFT] MMC: core/core.c: mmc_rescan detects card change in one run

[PATCH RFT] MMC: core/core.c: mmc_rescan detects card change in one run

[Jorg Schummer]

With this patch, mmc_rescan can detect the removal of an mmc card and
the insertion of (possibly another) card in the same run. This means
that a card change can be detected without having to call
mmc_detect_change multiple times.

This change should generalise the core such that it can be easily used
by hosts which provide a mechanism to detect only the presence of a card
reader cover, which has to be taken off in order to insert a card. Other
hosts ("card detect" or "MMC_CAP_NEEDS_POLL") each receive an event when
a card is removed and when a card is inserted, so it is sufficient for
them if mmc_rescan handles only one event at a time. "Cover detect"
hosts, however, only receive events about the cover status. This means
that between 2 subsequent events, both a card removal and a card
insertion can occur. In this case, the current version of mmc_rescan
would only detect the removal of the previous card but not the insertion
of the new card.

This patch has been tested with the "cover detect" mechanism. If you
can, please test it with "card detect" and/or "polling" card readers.

Signed-off-by: Jorg Schummer <ext-jorg.2.schummer@nokia.com>
---
 drivers/mmc/core/core.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index df6ce4a..cd2e29f 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -740,6 +740,22 @@ void mmc_rescan(struct work_struct *work)
 
 	mmc_bus_get(host);
 
+	/* if there is a card registered */
+	if (host->bus_ops != NULL) {
+
+		if (host->bus_ops->detect && !host->bus_dead) {
+
+			/* check whether the card is still present */
+			host->bus_ops->detect(host);
+
+			/* release the bus and update bus status in case
+			   the card was removed */
+			mmc_bus_put(host);
+			mmc_bus_get(host);
+		}
+	}
+
+	/* if there is no card registered */
 	if (host->bus_ops == NULL) {
 		/*
 		 * Only we can add a new handler, so it's safe to
@@ -789,12 +805,8 @@ void mmc_rescan(struct work_struct *work)
 
 		mmc_release_host(host);
 		mmc_power_off(host);
-	} else {
-		if (host->bus_ops->detect && !host->bus_dead)
-			host->bus_ops->detect(host);
-
+	} else
 		mmc_bus_put(host);
-	}
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
-- 
1.5.4.3

Re: [PATCH RFT] MMC: core/core.c: mmc_rescan detects card change in one run

[Pierre Ossman]

On Thu, 19 Feb 2009 17:26:26 +0200
Jorg Schummer <ext-jorg.2.schummer@nokia.com> wrote:

> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index df6ce4a..cd2e29f 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -740,6 +740,22 @@ void mmc_rescan(struct work_struct *work)
>  
>  	mmc_bus_get(host);
>  
> +	/* if there is a card registered */
> +	if (host->bus_ops != NULL) {
> +
> +		if (host->bus_ops->detect && !host->bus_dead) {
> +
> +			/* check whether the card is still present */
> +			host->bus_ops->detect(host);
> +
> +			/* release the bus and update bus status in case
> +			   the card was removed */
> +			mmc_bus_put(host);
> +			mmc_bus_get(host);
> +		}
> +	}
> +
> +	/* if there is no card registered */
>  	if (host->bus_ops == NULL) {
>  		/*
>  		 * Only we can add a new handler, so it's safe to

Perhaps it's more clear if you grab the lock for the first section,
release it after and then regrab it for the second section. A bit less
efficient, but I don't think that will be a problem in practice.

> @@ -789,12 +805,8 @@ void mmc_rescan(struct work_struct *work)
>  
>  		mmc_release_host(host);
>  		mmc_power_off(host);
> -	} else {
> -		if (host->bus_ops->detect && !host->bus_dead)
> -			host->bus_ops->detect(host);
> -
> +	} else
>  		mmc_bus_put(host);
> -	}
>  out:
>  	if (host->caps & MMC_CAP_NEEDS_POLL)
>  		mmc_schedule_delayed_work(&host->detect, HZ);

The else section got a bit small here with that code removed. Perhaps
we should instead have:

	if (host->bus_ops != NULL) {
		mmc_bus_put(host);
		goto out;
	}

With some adjusted comments to make the flow clear.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH RFT] MMC: core/core.c: mmc_rescan detects card change in one run

[Jörg Schummer]

Hello,

thanks for your reply.

On Mon, 2009-03-02 at 21:09 +0100, ext Pierre Ossman wrote:
> On Thu, 19 Feb 2009 17:26:26 +0200
> Jorg Schummer <ext-jorg.2.schummer@nokia.com> wrote:
> 
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index df6ce4a..cd2e29f 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -740,6 +740,22 @@ void mmc_rescan(struct work_struct *work)
> >  
> >  	mmc_bus_get(host);
> >  
> > +	/* if there is a card registered */
> > +	if (host->bus_ops != NULL) {
> > +
> > +		if (host->bus_ops->detect && !host->bus_dead) {
> > +
> > +			/* check whether the card is still present */
> > +			host->bus_ops->detect(host);
> > +
> > +			/* release the bus and update bus status in case
> > +			   the card was removed */
> > +			mmc_bus_put(host);
> > +			mmc_bus_get(host);
> > +		}
> > +	}
> > +
> > +	/* if there is no card registered */
> >  	if (host->bus_ops == NULL) {
> >  		/*
> >  		 * Only we can add a new handler, so it's safe to
> 
> Perhaps it's more clear if you grab the lock for the first section,
> release it after and then regrab it for the second section. A bit less
> efficient, but I don't think that will be a problem in practice.

Yes, you're probably right, I should take read- and maintainability into
account there. Will change that.

> 
> > @@ -789,12 +805,8 @@ void mmc_rescan(struct work_struct *work)
> >  
> >  		mmc_release_host(host);
> >  		mmc_power_off(host);
> > -	} else {
> > -		if (host->bus_ops->detect && !host->bus_dead)
> > -			host->bus_ops->detect(host);
> > -
> > +	} else
> >  		mmc_bus_put(host);
> > -	}
> >  out:
> >  	if (host->caps & MMC_CAP_NEEDS_POLL)
> >  		mmc_schedule_delayed_work(&host->detect, HZ);
> 
> The else section got a bit small here with that code removed. Perhaps
> we should instead have:
> 
> 	if (host->bus_ops != NULL) {
> 		mmc_bus_put(host);
> 		goto out;
> 	}

I guess you meant

	else {
		mmc_bus_put(host);
		goto out;
	}

since it would be the else clause of

	if (host->bus_ops == NULL)


Also: Are you sure "goto out" should be added in the else-branch? (Since
it's not present in the end of the corresponding if-branch either, and
is technically not needed.)

Regards,
Jörg

Re: [PATCH RFT] MMC: core/core.c: mmc_rescan detects card change in one run

[Pierre Ossman]

On Tue, 03 Mar 2009 09:24:19 +0200
Jörg Schummer <ext-jorg.2.schummer@nokia.com> wrote:

> > 
> > The else section got a bit small here with that code removed. Perhaps
> > we should instead have:
> > 
> > 	if (host->bus_ops != NULL) {
> > 		mmc_bus_put(host);
> > 		goto out;
> > 	}
> 
> I guess you meant
> 
> 	else {
> 		mmc_bus_put(host);
> 		goto out;
> 	}
> 
> since it would be the else clause of
> 
> 	if (host->bus_ops == NULL)
> 

No, I meant to put it further up and unindent the detection code.
Basically you'll be reversing the if-else blocks, but using a goto
instead of an else.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[PATCH 1/1] MMC: core/core.c: mmc_rescan detects card change in one run

[Jorg Schummer]

With this patch, mmc_rescan can detect the removal of an mmc card and
the insertion of (possibly another) card in the same run. This means
that a card change can be detected without having to call
mmc_detect_change multiple times.

This change generalises the core such that it can be easily used by
hosts which provide a mechanism to detect only the presence of a card
reader cover, which has to be taken off in order to insert a card. Other
hosts ("card detect" or "MMC_CAP_NEEDS_POLL") each receive an event when
a card is removed and when a card is inserted, so it is sufficient for
them if mmc_rescan handles only one event at a time. "Cover detect"
hosts, however, only receive events about the cover status. This means
that between 2 subsequent events, both a card removal and a card
insertion can occur. In this case, the pre-patch version of mmc_rescan
would only detect the removal of the previous card but not the insertion
of the new card.

Signed-off-by: Jorg Schummer <ext-jorg.2.schummer@nokia.com>
---
 drivers/mmc/core/core.c |   99 ++++++++++++++++++++++++++---------------------
 1 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index df6ce4a..5970719 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -740,61 +740,72 @@ void mmc_rescan(struct work_struct *work)
 
 	mmc_bus_get(host);
 
-	if (host->bus_ops == NULL) {
-		/*
-		 * Only we can add a new handler, so it's safe to
-		 * release the lock here.
-		 */
+	/* if there is a card registered, check whether it is still present */
+	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;
+	}
 
-		if (host->ops->get_cd && host->ops->get_cd(host) == 0)
-			goto out;
+	/* detect a newly inserted card */
 
-		mmc_claim_host(host);
+	/*
+	 * Only we can add a new handler, so it's safe to
+	 * release the lock here.
+	 */
+	mmc_bus_put(host);
 
-		mmc_power_up(host);
-		mmc_go_idle(host);
+	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
+		goto out;
 
-		mmc_send_if_cond(host, host->ocr_avail);
+	mmc_claim_host(host);
 
-		/*
-		 * First we search for SDIO...
-		 */
-		err = mmc_send_io_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sdio(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
+	mmc_power_up(host);
+	mmc_go_idle(host);
 
-		/*
-		 * ...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;
-		}
+	mmc_send_if_cond(host, host->ocr_avail);
 
-		/*
-		 * ...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;
-		}
+	/*
+	 * First we search for SDIO...
+	 */
+	err = mmc_send_io_op_cond(host, 0, &ocr);
+	if (!err) {
+		if (mmc_attach_sdio(host, ocr))
+			mmc_power_off(host);
+		goto out;
+	}
 
-		mmc_release_host(host);
-		mmc_power_off(host);
-	} else {
-		if (host->bus_ops->detect && !host->bus_dead)
-			host->bus_ops->detect(host);
+	/*
+	 * ...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;
+	}
 
-		mmc_bus_put(host);
+	/*
+	 * ...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;
 	}
+
+	mmc_release_host(host);
+	mmc_power_off(host);
+
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
-- 
1.5.4.3

Re: [PATCH 1/1] MMC: core/core.c: mmc_rescan detects card change in one run

[Pierre Ossman]

On Tue, 31 Mar 2009 17:51:21 +0300
Jorg Schummer <ext-jorg.2.schummer@nokia.com> wrote:

> With this patch, mmc_rescan can detect the removal of an mmc card and
> the insertion of (possibly another) card in the same run. This means
> that a card change can be detected without having to call
> mmc_detect_change multiple times.
> 
> This change generalises the core such that it can be easily used by
> hosts which provide a mechanism to detect only the presence of a card
> reader cover, which has to be taken off in order to insert a card. Other
> hosts ("card detect" or "MMC_CAP_NEEDS_POLL") each receive an event when
> a card is removed and when a card is inserted, so it is sufficient for
> them if mmc_rescan handles only one event at a time. "Cover detect"
> hosts, however, only receive events about the cover status. This means
> that between 2 subsequent events, both a card removal and a card
> insertion can occur. In this case, the pre-patch version of mmc_rescan
> would only detect the removal of the previous card but not the insertion
> of the new card.
> 
> Signed-off-by: Jorg Schummer <ext-jorg.2.schummer@nokia.com>
> ---

Queued, thanks.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org
  TigerVNC, core developer          http://www.tigervnc.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH 1/1] MMC: core/core.c: mmc_rescan detects card change in one run

[Jörg Schummer]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

On Fri, 2009-04-10 at 18:39 +0200, ext Pierre Ossman wrote:
> On Tue, 31 Mar 2009 17:51:21 +0300
> Jorg Schummer <ext-jorg.2.schummer@nokia.com> wrote:
> 
> > With this patch, mmc_rescan can detect the removal of an mmc card and
> > the insertion of (possibly another) card in the same run. This means
> > that a card change can be detected without having to call
> > mmc_detect_change multiple times.
> > 
> > This change generalises the core such that it can be easily used by
> > hosts which provide a mechanism to detect only the presence of a card
> > reader cover, which has to be taken off in order to insert a card. Other
> > hosts ("card detect" or "MMC_CAP_NEEDS_POLL") each receive an event when
> > a card is removed and when a card is inserted, so it is sufficient for
> > them if mmc_rescan handles only one event at a time. "Cover detect"
> > hosts, however, only receive events about the cover status. This means
> > that between 2 subsequent events, both a card removal and a card
> > insertion can occur. In this case, the pre-patch version of mmc_rescan
> > would only detect the removal of the previous card but not the insertion
> > of the new card.
> > 
> > Signed-off-by: Jorg Schummer <ext-jorg.2.schummer@nokia.com>
> > ---
> 
> Queued, thanks.

Hi Pierre,

is there any news about this patch?

Regards,
Jörg


[-- Attachment #2: mmc_rescan.patch --]
[-- Type: message/rfc822, Size: 4050 bytes --]

From: ext-jorg.2.schummer@nokia.com
Subject: [PATCH 1/1] MMC: core/core.c: mmc_rescan detects card change in one run
Date: Wed, 03 Jun 2009 12:47:48 +0300
Message-ID: <1244022468.20907.6.camel@jorg-desktop>

With this patch, mmc_rescan can detect the removal of an mmc card and
the insertion of (possibly another) card in the same run. This means
that a card change can be detected without having to call
mmc_detect_change multiple times.

This change generalises the core such that it can be easily used by
hosts which provide a mechanism to detect only the presence of a card
reader cover, which has to be taken off in order to insert a card. Other
hosts ("card detect" or "MMC_CAP_NEEDS_POLL") each receive an event when
a card is removed and when a card is inserted, so it is sufficient for
them if mmc_rescan handles only one event at a time. "Cover detect"
hosts, however, only receive events about the cover status. This means
that between 2 subsequent events, both a card removal and a card
insertion can occur. In this case, the pre-patch version of mmc_rescan
would only detect the removal of the previous card but not the insertion
of the new card.

Signed-off-by: Jorg Schummer <ext-jorg.2.schummer@nokia.com>
---
 drivers/mmc/core/core.c |   99 ++++++++++++++++++++++++++---------------------
 1 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index df6ce4a..5970719 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -740,61 +740,72 @@ void mmc_rescan(struct work_struct *work)
 
 	mmc_bus_get(host);
 
-	if (host->bus_ops == NULL) {
-		/*
-		 * Only we can add a new handler, so it's safe to
-		 * release the lock here.
-		 */
+	/* if there is a card registered, check whether it is still present */
+	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;
+	}
 
-		if (host->ops->get_cd && host->ops->get_cd(host) == 0)
-			goto out;
+	/* detect a newly inserted card */
 
-		mmc_claim_host(host);
+	/*
+	 * Only we can add a new handler, so it's safe to
+	 * release the lock here.
+	 */
+	mmc_bus_put(host);
 
-		mmc_power_up(host);
-		mmc_go_idle(host);
+	if (host->ops->get_cd && host->ops->get_cd(host) == 0)
+		goto out;
 
-		mmc_send_if_cond(host, host->ocr_avail);
+	mmc_claim_host(host);
 
-		/*
-		 * First we search for SDIO...
-		 */
-		err = mmc_send_io_op_cond(host, 0, &ocr);
-		if (!err) {
-			if (mmc_attach_sdio(host, ocr))
-				mmc_power_off(host);
-			goto out;
-		}
+	mmc_power_up(host);
+	mmc_go_idle(host);
 
-		/*
-		 * ...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;
-		}
+	mmc_send_if_cond(host, host->ocr_avail);
 
-		/*
-		 * ...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;
-		}
+	/*
+	 * First we search for SDIO...
+	 */
+	err = mmc_send_io_op_cond(host, 0, &ocr);
+	if (!err) {
+		if (mmc_attach_sdio(host, ocr))
+			mmc_power_off(host);
+		goto out;
+	}
 
-		mmc_release_host(host);
-		mmc_power_off(host);
-	} else {
-		if (host->bus_ops->detect && !host->bus_dead)
-			host->bus_ops->detect(host);
+	/*
+	 * ...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;
+	}
 
-		mmc_bus_put(host);
+	/*
+	 * ...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;
 	}
+
+	mmc_release_host(host);
+	mmc_power_off(host);
+
 out:
 	if (host->caps & MMC_CAP_NEEDS_POLL)
 		mmc_schedule_delayed_work(&host->detect, HZ);
-- 
1.5.4.3

Re: [PATCH 1/1] MMC: core/core.c: mmc_rescan detects card change in one run

[Pierre Ossman]

On Wed, 03 Jun 2009 12:47:48 +0300
Jörg Schummer <ext-jorg.2.schummer@nokia.com> wrote:

> 
> Hi Pierre,
> 
> is there any news about this patch?
> 

It's in my -next branch and will be submitted upstream in the next
merge window.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org
  TigerVNC, core developer          http://www.tigervnc.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.