[PATCH V2] mmc: Kill block requests if card is removed

[PATCH V2] mmc: Kill block requests if card is removed

[Sujit Reddy Thumma]

Kill block requests when the host knows that the card is
removed from the slot and is sure that subsequent requests
are bound to fail. Do this silently so that the block
layer doesn't output unnecessary error messages.

This patch implements suggestion from Adrian Hunter,
http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474

Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>

---
Changes in v2:
	- Changed the implementation with further comments from Adrian
	- Set the card removed flag in bus notifier callbacks
	- This patch is now dependent on patch from Per Forlin:
	http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
---
 drivers/mmc/card/block.c |   33 ++++++++++++++++++++++++++++-----
 drivers/mmc/card/queue.c |    5 +++++
 drivers/mmc/core/bus.c   |   32 +++++++++++++++++++++++++++++++-
 drivers/mmc/core/core.c  |    8 +++++++-
 include/linux/mmc/card.h |    3 +++
 5 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index edc379e..83956fa 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
 	}
 
 	/* We couldn't get a response from the card.  Give up. */
-	if (err)
+	if (err) {
+		/*
+		 * If the card didn't respond to status command,
+		 * it is likely that card is gone. Flag it as removed,
+		 * mmc_detect_change() cleans the rest.
+		 */
+		mmc_card_set_removed(card);
 		return ERR_ABORT;
+	}
 
 	/* Flag ECC errors */
 	if ((status & R1_CARD_ECC_FAILED) ||
@@ -1168,6 +1175,9 @@ static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
                                                   int disable_multi,
                                                   struct mmc_async_req *areq)
 {
+	struct mmc_blk_data *md = mmc_get_drvdata(card);
+	struct request *req = mqrq->req;
+	int ret;
        /*
         * Release host after failure in case the host is needed
         * by someone else. For instance, if the card is removed the
@@ -1175,11 +1185,24 @@ static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
         */
        mmc_release_host(card->host);
        mmc_claim_host(card->host);
-
-       mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
-       return mmc_start_req(card->host, areq, NULL);
+	if (mmc_card_removed(card)) {
+		/*
+		 * End the pending async request without starting
+		 * it when card is removed.
+		 */
+		spin_lock_irq(&md->lock);
+		req->cmd_flags |= REQ_QUIET;
+		do {
+			ret = __blk_end_request(req,
+					-EIO, blk_rq_cur_bytes(req));
+		} while (ret);
+		spin_unlock_irq(&md->lock);
+		return NULL;
+	} else {
+		mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+		return mmc_start_req(card->host, areq, NULL);
+	}
 }
-
 static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 {
 	struct mmc_blk_data *md = mq->data;
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index dcad59c..2517547 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -29,6 +29,8 @@
  */
 static int mmc_prep_request(struct request_queue *q, struct request *req)
 {
+	struct mmc_queue *mq = q->queuedata;
+
 	/*
 	 * We only like normal block requests and discards.
 	 */
@@ -37,6 +39,9 @@ static int mmc_prep_request(struct request_queue *q, struct request *req)
 		return BLKPREP_KILL;
 	}
 
+	if (mq && mmc_card_removed(mq->card))
+		return BLKPREP_KILL;
+
 	req->cmd_flags |= REQ_DONTPREP;
 
 	return BLKPREP_OK;
diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 5639fdf..ca6e07a 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -102,6 +102,25 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return retval;
 }
 
+static int mmc_bus_notify(struct notifier_block *nb, unsigned long action,
+		void *data)
+{
+	struct mmc_card *card = mmc_dev_to_card((struct device *) data);
+
+	switch (action) {
+	case BUS_NOTIFY_DEL_DEVICE:
+		mmc_card_set_removed(card);
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static struct notifier_block mmc_bus_nb = {
+	.notifier_call = mmc_bus_notify,
+};
+
 static int mmc_bus_probe(struct device *dev)
 {
 	struct mmc_driver *drv = to_mmc_driver(dev->driver);
@@ -191,11 +210,22 @@ static struct bus_type mmc_bus_type = {
 
 int mmc_register_bus(void)
 {
-	return bus_register(&mmc_bus_type);
+	int ret;
+
+	ret = bus_register(&mmc_bus_type);
+	if (ret)
+		goto out;
+
+	ret = bus_register_notifier(&mmc_bus_type, &mmc_bus_nb);
+	if (ret)
+		bus_unregister(&mmc_bus_type);
+out:
+	return ret;
 }
 
 void mmc_unregister_bus(void)
 {
+	bus_unregister_notifier(&mmc_bus_type, &mmc_bus_nb);
 	bus_unregister(&mmc_bus_type);
 }
 
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 271efea..a2ce729 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -259,7 +259,8 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 		wait_for_completion(&mrq->completion);
 
 		cmd = mrq->cmd;
-		if (!cmd->error || !cmd->retries)
+		if (!cmd->error || !cmd->retries ||
+				mmc_card_removed(host->card))
 			break;
 
 		pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
@@ -374,6 +375,11 @@ EXPORT_SYMBOL(mmc_start_req);
  */
 void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
 {
+	if (mmc_card_removed(host->card)) {
+		mrq->cmd->error = -ENOMEDIUM;
+		return;
+	}
+
 	__mmc_start_req(host, mrq);
 	mmc_wait_for_req_done(host, mrq);
 }
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 534974c..9319329 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -209,6 +209,7 @@ struct mmc_card {
 #define MMC_STATE_HIGHSPEED_DDR (1<<4)		/* card is in high speed mode */
 #define MMC_STATE_ULTRAHIGHSPEED (1<<5)		/* card is in ultra high speed mode */
 #define MMC_CARD_SDXC		(1<<6)		/* card is SDXC */
+#define MMC_STATE_CARD_REMOVED	(1<<7)		/* card removed from the slot */
 	unsigned int		quirks; 	/* card quirks */
 #define MMC_QUIRK_LENIENT_FN0	(1<<0)		/* allow SDIO FN0 writes outside of the VS CCCR range */
 #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1)	/* use func->cur_blksize */
@@ -363,6 +364,7 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
 #define mmc_card_sdio(c)	((c)->type == MMC_TYPE_SDIO)
 
 #define mmc_card_present(c)	((c)->state & MMC_STATE_PRESENT)
+#define mmc_card_removed(c)	(c && ((c)->state & MMC_STATE_CARD_REMOVED))
 #define mmc_card_readonly(c)	((c)->state & MMC_STATE_READONLY)
 #define mmc_card_highspeed(c)	((c)->state & MMC_STATE_HIGHSPEED)
 #define mmc_card_blockaddr(c)	((c)->state & MMC_STATE_BLOCKADDR)
@@ -372,6 +374,7 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
 #define mmc_card_ext_capacity(c) ((c)->state & MMC_CARD_SDXC)
 
 #define mmc_card_set_present(c)	((c)->state |= MMC_STATE_PRESENT)
+#define mmc_card_set_removed(c) (c ? (c)->state |= MMC_STATE_CARD_REMOVED : 0)
 #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
 #define mmc_card_set_highspeed(c) ((c)->state |= MMC_STATE_HIGHSPEED)
 #define mmc_card_set_blockaddr(c) ((c)->state |= MMC_STATE_BLOCKADDR)
-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Per Forlin]

Hi Sujit,

On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> Kill block requests when the host knows that the card is
> removed from the slot and is sure that subsequent requests
> are bound to fail. Do this silently so that the block
> layer doesn't output unnecessary error messages.
>
> This patch implements suggestion from Adrian Hunter,
> http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>
> ---
> Changes in v2:
>        - Changed the implementation with further comments from Adrian
>        - Set the card removed flag in bus notifier callbacks
>        - This patch is now dependent on patch from Per Forlin:
>        http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
> ---
>  drivers/mmc/card/block.c |   33 ++++++++++++++++++++++++++++-----
>  drivers/mmc/card/queue.c |    5 +++++
>  drivers/mmc/core/bus.c   |   32 +++++++++++++++++++++++++++++++-
>  drivers/mmc/core/core.c  |    8 +++++++-
>  include/linux/mmc/card.h |    3 +++
>  5 files changed, 74 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index edc379e..83956fa 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
>        }
>
>        /* We couldn't get a response from the card.  Give up. */
> -       if (err)
> +       if (err) {
> +               /*
> +                * If the card didn't respond to status command,
> +                * it is likely that card is gone. Flag it as removed,
> +                * mmc_detect_change() cleans the rest.
> +                */
> +               mmc_card_set_removed(card);
>                return ERR_ABORT;
> +       }
>
>        /* Flag ECC errors */
>        if ((status & R1_CARD_ECC_FAILED) ||
> @@ -1168,6 +1175,9 @@ static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>                                                   int disable_multi,
>                                                   struct mmc_async_req *areq)
>  {
> +       struct mmc_blk_data *md = mmc_get_drvdata(card);
> +       struct request *req = mqrq->req;
> +       int ret;
>        /*
>         * Release host after failure in case the host is needed
>         * by someone else. For instance, if the card is removed the
> @@ -1175,11 +1185,24 @@ static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>         */
>        mmc_release_host(card->host);
>        mmc_claim_host(card->host);
> -
> -       mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
> -       return mmc_start_req(card->host, areq, NULL);
> +       if (mmc_card_removed(card)) {
> +               /*
> +                * End the pending async request without starting
> +                * it when card is removed.
> +                */
> +               spin_lock_irq(&md->lock);
> +               req->cmd_flags |= REQ_QUIET;
> +               do {
> +                       ret = __blk_end_request(req,
> +                                       -EIO, blk_rq_cur_bytes(req));
> +               } while (ret);
> +               spin_unlock_irq(&md->lock);
> +               return NULL;
> +       } else {
> +               mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
> +               return mmc_start_req(card->host, areq, NULL);
> +       }
mmc_blk_resend is only called to resend a request that has failed. If
the card is removed the request will still be issued, but when it
retries it will give up here.
You have added a check in mmc_wait_for_req(). What about this:
--------------------------
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b526036..dcdcb9a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -287,11 +287,17 @@ static void mmc_wait_done(struct mmc_request *mrq)
        complete(&mrq->completion);
 }

-static void __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
+static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
 {
-       init_completion(&mrq->completion);
-       mrq->done = mmc_wait_done;
-       mmc_start_request(host, mrq);
+       if (mmc_card_removed(host->card)) {
+              mrq->cmd->error = -ENOMEDIUM;
+              return -ENOMEDIUM;
+       }
+
+       init_completion(&mrq->completion);
+       mrq->done = mmc_wait_done;
+       mmc_start_request(host, mrq);
+       return 0;
 }

 static void mmc_wait_for_req_done(struct mmc_host *host,
@@ -418,7 +424,8 @@ EXPORT_SYMBOL(mmc_start_req);
  */
 void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
 {
-       __mmc_start_req(host, mrq);
+       if (__mmc_start_req(host, mrq))
+               return
        mmc_wait_for_req_done(host, mrq);
 }
 EXPORT_SYMBOL(mmc_wait_for_req);
----------------------------------
This patch will set error to -ENOMEDIUM for both mmc_start_req() and
mmc_wait_for_req()

mmc_blk_err_check() can check for -ENOMEDIUM and return something like
MMC_BLK_ENOMEDIUM
And eventually do
+               /*
+                * End the pending async request without starting
+                * it when card is removed.
+                */
+               spin_lock_irq(&md->lock);
+               req->cmd_flags |= REQ_QUIET;
+               do {
+                       ret = __blk_end_request(req,
+                                       -EIO, blk_rq_cur_bytes(req));
+               } while (ret);
+               spin_unlock_irq(&md->lock);

Regards,
Per

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Sujit Reddy Thumma]

Hi Per,

On 11/22/2011 6:40 PM, Per Forlin wrote:
> Hi Sujit,
>
> On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma
> <sthumma@codeaurora.org>  wrote:
>> Kill block requests when the host knows that the card is
>> removed from the slot and is sure that subsequent requests
>> are bound to fail. Do this silently so that the block
>> layer doesn't output unnecessary error messages.
>>
>> This patch implements suggestion from Adrian Hunter,
>> http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
>>
>> Signed-off-by: Sujit Reddy Thumma<sthumma@codeaurora.org>
>>
>> ---
>> Changes in v2:
>>         - Changed the implementation with further comments from Adrian
>>         - Set the card removed flag in bus notifier callbacks
>>         - This patch is now dependent on patch from Per Forlin:
>>         http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
>> ---
>>   drivers/mmc/card/block.c |   33 ++++++++++++++++++++++++++++-----
>>   drivers/mmc/card/queue.c |    5 +++++
>>   drivers/mmc/core/bus.c   |   32 +++++++++++++++++++++++++++++++-
>>   drivers/mmc/core/core.c  |    8 +++++++-
>>   include/linux/mmc/card.h |    3 +++
>>   5 files changed, 74 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index edc379e..83956fa 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card, struct request *req,
>>         }
>>
>>         /* We couldn't get a response from the card.  Give up. */
>> -       if (err)
>> +       if (err) {
>> +               /*
>> +                * If the card didn't respond to status command,
>> +                * it is likely that card is gone. Flag it as removed,
>> +                * mmc_detect_change() cleans the rest.
>> +                */
>> +               mmc_card_set_removed(card);
>>                 return ERR_ABORT;
>> +       }
>>
>>         /* Flag ECC errors */
>>         if ((status&  R1_CARD_ECC_FAILED) ||
>> @@ -1168,6 +1175,9 @@ static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>>                                                    int disable_multi,
>>                                                    struct mmc_async_req *areq)
>>   {
>> +       struct mmc_blk_data *md = mmc_get_drvdata(card);
>> +       struct request *req = mqrq->req;
>> +       int ret;
>>         /*
>>          * Release host after failure in case the host is needed
>>          * by someone else. For instance, if the card is removed the
>> @@ -1175,11 +1185,24 @@ static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>>          */
>>         mmc_release_host(card->host);
>>         mmc_claim_host(card->host);
>> -
>> -       mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>> -       return mmc_start_req(card->host, areq, NULL);
>> +       if (mmc_card_removed(card)) {
>> +               /*
>> +                * End the pending async request without starting
>> +                * it when card is removed.
>> +                */
>> +               spin_lock_irq(&md->lock);
>> +               req->cmd_flags |= REQ_QUIET;
>> +               do {
>> +                       ret = __blk_end_request(req,
>> +                                       -EIO, blk_rq_cur_bytes(req));
>> +               } while (ret);
>> +               spin_unlock_irq(&md->lock);
>> +               return NULL;
>> +       } else {
>> +               mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>> +               return mmc_start_req(card->host, areq, NULL);
>> +       }
> mmc_blk_resend is only called to resend a request that has failed. If
> the card is removed the request will still be issued, but when it
> retries it will give up here.

Currently, we set the card_removed flag in two places:

1) If the host supports interrupt detection, mmc_detect_change() calls 
the bus detect method and we set card removed flag in bus notifier call 
back.

2) When there is a transfer going on (host is claimed by mmcqd) and the 
card is removed ungracefully, the driver sends an error code to the 
block layer. mmc_blk_cmd_recovery() tries to send CMD13 to the card 
which in this case fails and we set the card_removed flag.

So when we retry or send next async request we end it in 
mmc_blk_resend() and there will be no subsequent request to the driver 
since we are suppressing the requests in the queue layer.

So I guess there is no need to add further checks in the 
__mmc_start_req(), which could be redundant as there are no calls to the 
core layer from block layer once the card is found to be removed.

In summary the sequence I thought would be like this:
Case 1:
1) Transfer is going on (host claimed by mmcqd) and card is removed.
2) Driver is in middle of transaction, hence some kind of timeout error 
is returned.
3) mmc_blk_cmd_recovery(): Block layer does error checking and sets the 
card as removed.
4) Block layer then retries the request calling mmc_blk_resend().
5) Since the mmc_card_removed is true we end the request here and do not 
send any request to the core.

Case 2:
1) When there is no transfer going on (host->claimed = 0)
2) mmc_detect_change() would set card_removed flag.
3) All the subsequent requests would be killed in queue layer itself.

> You have added a check in mmc_wait_for_req(). What about this:

mmc_wait_for_req() will be called to send regular CMDs as well, hence we 
need to add a check.

> --------------------------
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index b526036..dcdcb9a 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -287,11 +287,17 @@ static void mmc_wait_done(struct mmc_request *mrq)
>          complete(&mrq->completion);
>   }
>
> -static void __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
> +static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
>   {
> -       init_completion(&mrq->completion);
> -       mrq->done = mmc_wait_done;
> -       mmc_start_request(host, mrq);
> +       if (mmc_card_removed(host->card)) {
> +              mrq->cmd->error = -ENOMEDIUM;
> +              return -ENOMEDIUM;
> +       }

This is not yet done here. If an async request comes then it will do a 
wait_for_req_done() for the previous request which returned even without 
doing a init_completion. So we need to handle this case in mmc_start_req().

> +
> +       init_completion(&mrq->completion);
> +       mrq->done = mmc_wait_done;
> +       mmc_start_request(host, mrq);
> +       return 0;
>   }
>
>   static void mmc_wait_for_req_done(struct mmc_host *host,
> @@ -418,7 +424,8 @@ EXPORT_SYMBOL(mmc_start_req);
>    */
>   void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>   {
> -       __mmc_start_req(host, mrq);
> +       if (__mmc_start_req(host, mrq))
> +               return
>          mmc_wait_for_req_done(host, mrq);
>   }
>   EXPORT_SYMBOL(mmc_wait_for_req);
> ----------------------------------
> This patch will set error to -ENOMEDIUM for both mmc_start_req() and
> mmc_wait_for_req()
>
> mmc_blk_err_check() can check for -ENOMEDIUM and return something like
> MMC_BLK_ENOMEDIUM

If I understand correctly, the above patch handles a third case:
1) issue_fn is called by the queue layer (for the first request)
2) just before the mmcqd claims host card is removed and the 
mmc_detect_change has flagged the card as removed.
3) we send the request and before it was sent to host driver 
__mmc_start_req() sent -ENOMEDIUM and the block layer handles new error 
ENOMEDIUM.

This is a valid case but increases the complexity as we need to take 
care of the async request which does not know we have returned 
prematurely without doing init_completion().

I guess we can live with this rare case with just outputting few errors, 
as the whole idea of patch is to reduce (if not completely eliminate) 
unnecessary prints in the log output which can bog down cpu if some 
wired peripheral like UART console is enabled.

Please let me know if I am missing something.

> And eventually do
> +               /*
> +                * End the pending async request without starting
> +                * it when card is removed.
> +                */
> +               spin_lock_irq(&md->lock);
> +               req->cmd_flags |= REQ_QUIET;
> +               do {
> +                       ret = __blk_end_request(req,
> +                                       -EIO, blk_rq_cur_bytes(req));
> +               } while (ret);
> +               spin_unlock_irq(&md->lock);
>
> Regards,
> Per


-- 
Thanks,
Sujit

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Per Forlin]

On Thu, Nov 24, 2011 at 12:27 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> Hi Per,
>
> On 11/22/2011 6:40 PM, Per Forlin wrote:
>>
>> Hi Sujit,
>>
>> On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma
>> <sthumma@codeaurora.org>  wrote:
>>>
>>> Kill block requests when the host knows that the card is
>>> removed from the slot and is sure that subsequent requests
>>> are bound to fail. Do this silently so that the block
>>> layer doesn't output unnecessary error messages.
>>>
>>> This patch implements suggestion from Adrian Hunter,
>>> http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
>>>
>>> Signed-off-by: Sujit Reddy Thumma<sthumma@codeaurora.org>
>>>
>>> ---
>>> Changes in v2:
>>>        - Changed the implementation with further comments from Adrian
>>>        - Set the card removed flag in bus notifier callbacks
>>>        - This patch is now dependent on patch from Per Forlin:
>>>        http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
>>> ---
>>>  drivers/mmc/card/block.c |   33 ++++++++++++++++++++++++++++-----
>>>  drivers/mmc/card/queue.c |    5 +++++
>>>  drivers/mmc/core/bus.c   |   32 +++++++++++++++++++++++++++++++-
>>>  drivers/mmc/core/core.c  |    8 +++++++-
>>>  include/linux/mmc/card.h |    3 +++
>>>  5 files changed, 74 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>> index edc379e..83956fa 100644
>>> --- a/drivers/mmc/card/block.c
>>> +++ b/drivers/mmc/card/block.c
>>> @@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card
>>> *card, struct request *req,
>>>        }
>>>
>>>        /* We couldn't get a response from the card.  Give up. */
>>> -       if (err)
>>> +       if (err) {
>>> +               /*
>>> +                * If the card didn't respond to status command,
>>> +                * it is likely that card is gone. Flag it as removed,
>>> +                * mmc_detect_change() cleans the rest.
>>> +                */
>>> +               mmc_card_set_removed(card);
>>>                return ERR_ABORT;
>>> +       }
>>>
>>>        /* Flag ECC errors */
>>>        if ((status&  R1_CARD_ECC_FAILED) ||
>>> @@ -1168,6 +1175,9 @@ static inline struct mmc_async_req
>>> *mmc_blk_resend(struct mmc_card *card,
>>>                                                   int disable_multi,
>>>                                                   struct mmc_async_req
>>> *areq)
>>>  {
>>> +       struct mmc_blk_data *md = mmc_get_drvdata(card);
>>> +       struct request *req = mqrq->req;
>>> +       int ret;
>>>        /*
>>>         * Release host after failure in case the host is needed
>>>         * by someone else. For instance, if the card is removed the
>>> @@ -1175,11 +1185,24 @@ static inline struct mmc_async_req
>>> *mmc_blk_resend(struct mmc_card *card,
>>>         */
>>>        mmc_release_host(card->host);
>>>        mmc_claim_host(card->host);
>>> -
>>> -       mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>>> -       return mmc_start_req(card->host, areq, NULL);
>>> +       if (mmc_card_removed(card)) {
>>> +               /*
>>> +                * End the pending async request without starting
>>> +                * it when card is removed.
>>> +                */
>>> +               spin_lock_irq(&md->lock);
>>> +               req->cmd_flags |= REQ_QUIET;
>>> +               do {
>>> +                       ret = __blk_end_request(req,
>>> +                                       -EIO, blk_rq_cur_bytes(req));
>>> +               } while (ret);
>>> +               spin_unlock_irq(&md->lock);
>>> +               return NULL;
>>> +       } else {
>>> +               mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>>> +               return mmc_start_req(card->host, areq, NULL);
>>> +       }
>>
>> mmc_blk_resend is only called to resend a request that has failed. If
>> the card is removed the request will still be issued, but when it
>> retries it will give up here.
>
> Currently, we set the card_removed flag in two places:
>
> 1) If the host supports interrupt detection, mmc_detect_change() calls the
> bus detect method and we set card removed flag in bus notifier call back.
>
> 2) When there is a transfer going on (host is claimed by mmcqd) and the card
> is removed ungracefully, the driver sends an error code to the block layer.
> mmc_blk_cmd_recovery() tries to send CMD13 to the card which in this case
> fails and we set the card_removed flag.
>
> So when we retry or send next async request we end it in mmc_blk_resend()
> and there will be no subsequent request to the driver since we are
> suppressing the requests in the queue layer.
>
> So I guess there is no need to add further checks in the __mmc_start_req(),
> which could be redundant as there are no calls to the core layer from block
> layer once the card is found to be removed.
>
> In summary the sequence I thought would be like this:
> Case 1:
> 1) Transfer is going on (host claimed by mmcqd) and card is removed.
> 2) Driver is in middle of transaction, hence some kind of timeout error is
> returned.
> 3) mmc_blk_cmd_recovery(): Block layer does error checking and sets the card
> as removed.
> 4) Block layer then retries the request calling mmc_blk_resend().
> 5) Since the mmc_card_removed is true we end the request here and do not
> send any request to the core.
>
> Case 2:
> 1) When there is no transfer going on (host->claimed = 0)
> 2) mmc_detect_change() would set card_removed flag.
> 3) All the subsequent requests would be killed in queue layer itself.
>
>> You have added a check in mmc_wait_for_req(). What about this:
>
> mmc_wait_for_req() will be called to send regular CMDs as well, hence we
> need to add a check.
>
>> --------------------------
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index b526036..dcdcb9a 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -287,11 +287,17 @@ static void mmc_wait_done(struct mmc_request *mrq)
>>         complete(&mrq->completion);
>>  }
>>
>> -static void __mmc_start_req(struct mmc_host *host, struct mmc_request
>> *mrq)
>> +static int __mmc_start_req(struct mmc_host *host, struct mmc_request
>> *mrq)
>>  {
>> -       init_completion(&mrq->completion);
>> -       mrq->done = mmc_wait_done;
>> -       mmc_start_request(host, mrq);
>> +       if (mmc_card_removed(host->card)) {
>> +              mrq->cmd->error = -ENOMEDIUM;
>> +              return -ENOMEDIUM;
>> +       }
>
> This is not yet done here. If an async request comes then it will do a
> wait_for_req_done() for the previous request which returned even without
> doing a init_completion. So we need to handle this case in mmc_start_req().
>
You're right. Thanks for spotting this. init_completion and done must
be assigned.
The patch should look like this:
-------------------------------
init_completion(&mrq->completion);
mrq->done = mmc_wait_done;
if (mmc_card_removed(host->card)) {
             complete(&mrq->completion);
             mrq->cmd->error = -ENOMEDIUM;
             return -ENOMEDIUM;
}
mmc_start_request(host, mrq);
-----------------------
What about this?

>> +
>> +       init_completion(&mrq->completion);
>> +       mrq->done = mmc_wait_done;
>> +       mmc_start_request(host, mrq);
>> +       return 0;
>>  }
>>
>>  static void mmc_wait_for_req_done(struct mmc_host *host,
>> @@ -418,7 +424,8 @@ EXPORT_SYMBOL(mmc_start_req);
>>   */
>>  void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>>  {
>> -       __mmc_start_req(host, mrq);
>> +       if (__mmc_start_req(host, mrq))
>> +               return
>>         mmc_wait_for_req_done(host, mrq);
>>  }
>>  EXPORT_SYMBOL(mmc_wait_for_req);
>> ----------------------------------
>> This patch will set error to -ENOMEDIUM for both mmc_start_req() and
>> mmc_wait_for_req()
>>
>> mmc_blk_err_check() can check for -ENOMEDIUM and return something like
>> MMC_BLK_ENOMEDIUM
>
> If I understand correctly, the above patch handles a third case:
> 1) issue_fn is called by the queue layer (for the first request)
> 2) just before the mmcqd claims host card is removed and the
> mmc_detect_change has flagged the card as removed.
> 3) we send the request and before it was sent to host driver
> __mmc_start_req() sent -ENOMEDIUM and the block layer handles new error
> ENOMEDIUM.
>
> This is a valid case but increases the complexity as we need to take care of
> the async request which does not know we have returned prematurely without
> doing init_completion().
My intention is to simplify the error handling by checking
is-card-removed in __mmc_start_req() rather than both
mmc_wait_for_req() and mmc_start_req().

Flow for async:
0. *card is removed
1. mmc_start_req()
2. -> mmc_wait_for_req_done()
3. ->->err_check()
4. ->->->mmc_blk_cmd_recovery()
5. ->->->->mmc_card_set_removed(card)
6. skip start next request and returns
7. new request from block layer
8. mmc_start_req()
9. ->__mmc_start_req()
10.new request from block layer
11. mmc_start_req()
12. -> mmc_wait_for_req_done() /* completion is already completed in
__mmc_start_req(), checks card-is-removed and returns */
13. ->-> err_check() /* prints no warning since error is -ENOMEDIUM */

>
> I guess we can live with this rare case with just outputting few errors, as
> the whole idea of patch is to reduce (if not completely eliminate)
> unnecessary prints in the log output which can bog down cpu if some wired
> peripheral like UART console is enabled.
>
> Please let me know if I am missing something.
>
Here's my case. in mmci.c: mmc_detect_change(host->mmc, msecs_to_jiffies(500));
It takes 500 ms until mmc_rescan is called. Of course, I can change
the timing but let's skip that part for now.
This means the block layer will continue to issue requests for 500 ms
after the card is ejected. There will be no warnings for the retries
because the card-is-removed is checked in resend(), but for new
incoming requests there will be error prints.

Regards,
Per

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Adrian Hunter]

Hi

Here is a way to allow mmc block to determine immediately
if a card has been removed while preserving the present rules
and keeping card detection in the bus_ops.

Untested I'm afraid.

Regards
Adrian



>From 2c6c9535b94c07fa3d12af26e9b6de500cb29970 Mon Sep 17 00:00:00 2001
From: Adrian Hunter <adrian.hunter@intel.com>
Date: Thu, 24 Nov 2011 16:32:34 +0200
Subject: [PATCH] mmc: allow upper layers to determine immediately if a card has been removed

Add a function mmc_card_removed() which upper layers can use
to determine immediately if a card has been removed.  This
function should be called after an I/O request fails so that
all queued I/O requests can be errored out immediately
instead of waiting for the card device to be removed.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c  |   32 ++++++++++++++++++++++++++++++++
 drivers/mmc/core/core.h  |    3 +++
 drivers/mmc/core/mmc.c   |   12 +++++++++++-
 drivers/mmc/core/sd.c    |   12 +++++++++++-
 drivers/mmc/core/sdio.c  |   11 ++++++++++-
 include/linux/mmc/card.h |    1 +
 include/linux/mmc/core.h |    2 ++
 7 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 271efea..c317564 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2049,6 +2049,38 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
 	return -EIO;
 }

+int _mmc_card_removed(struct mmc_host *host, int detect_change)
+{
+	int ret;
+
+	if (!(host->caps & MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
+		return 0;
+
+	if (!host->card || (host->card->state & MMC_CARD_REMOVED))
+		return 1;
+
+	/*
+	 * The card will be considered alive unless we have been asked to detect
+	 * a change or host requires polling to provide card detection.
+	 */
+	if (!detect_change && !(host->caps & MMC_CAP_NEEDS_POLL))
+		return 0;
+
+	ret = host->bus_ops->alive(host);
+	if (ret)
+		host->card->state |= MMC_CARD_REMOVED;
+
+	return ret;
+}
+
+int mmc_card_removed(struct mmc_host *host)
+{
+	WARN_ON(!host->claimed);
+
+	return _mmc_card_removed(host, work_pending(&host->detect.work));
+}
+EXPORT_SYMBOL(mmc_card_removed);
+
 void mmc_rescan(struct work_struct *work)
 {
 	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 14664f1..4b6b10e 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -24,6 +24,7 @@ struct mmc_bus_ops {
 	int (*resume)(struct mmc_host *);
 	int (*power_save)(struct mmc_host *);
 	int (*power_restore)(struct mmc_host *);
+	int (*alive)(struct mmc_host *);
 };

 void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
@@ -59,6 +60,8 @@ 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_card_removed(struct mmc_host *host, int detect_change);
+
 int mmc_attach_mmc(struct mmc_host *host);
 int mmc_attach_sd(struct mmc_host *host);
 int mmc_attach_sdio(struct mmc_host *host);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index a1223bd..5841d08 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1104,6 +1104,14 @@ static void mmc_remove(struct mmc_host *host)
 }

 /*
+ * Card detection - card is alive.
+ */
+static int mmc_alive(struct mmc_host *host)
+{
+	return mmc_send_status(host->card, NULL);
+}
+
+/*
  * Card detection callback from host.
  */
 static void mmc_detect(struct mmc_host *host)
@@ -1118,7 +1126,7 @@ static void mmc_detect(struct mmc_host *host)
 	/*
 	 * Just check if our card has been removed.
 	 */
-	err = mmc_send_status(host->card, NULL);
+	err = _mmc_card_removed(host, 1);

 	mmc_release_host(host);

@@ -1223,6 +1231,7 @@ static const struct mmc_bus_ops mmc_ops = {
 	.suspend = NULL,
 	.resume = NULL,
 	.power_restore = mmc_power_restore,
+	.alive = mmc_alive,
 };

 static const struct mmc_bus_ops mmc_ops_unsafe = {
@@ -1233,6 +1242,7 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
 	.suspend = mmc_suspend,
 	.resume = mmc_resume,
 	.power_restore = mmc_power_restore,
+	.alive = mmc_alive,
 };

 static void mmc_attach_bus_ops(struct mmc_host *host)
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 1d5a3bd..82855ed 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1018,6 +1018,14 @@ static void mmc_sd_remove(struct mmc_host *host)
 }

 /*
+ * Card detection - card is alive.
+ */
+static int mmc_sd_alive(struct mmc_host *host)
+{
+	return mmc_send_status(host->card, NULL);
+}
+
+/*
  * Card detection callback from host.
  */
 static void mmc_sd_detect(struct mmc_host *host)
@@ -1032,7 +1040,7 @@ static void mmc_sd_detect(struct mmc_host *host)
 	/*
 	 * Just check if our card has been removed.
 	 */
-	err = mmc_send_status(host->card, NULL);
+	err = _mmc_card_removed(host, 1);

 	mmc_release_host(host);

@@ -1101,6 +1109,7 @@ static const struct mmc_bus_ops mmc_sd_ops = {
 	.suspend = NULL,
 	.resume = NULL,
 	.power_restore = mmc_sd_power_restore,
+	.alive = mmc_sd_alive,
 };

 static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
@@ -1109,6 +1118,7 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
 	.suspend = mmc_sd_suspend,
 	.resume = mmc_sd_resume,
 	.power_restore = mmc_sd_power_restore,
+	.alive = mmc_sd_alive,
 };

 static void mmc_sd_attach_bus_ops(struct mmc_host *host)
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 8c04f7f..ca5c4f9 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -820,6 +820,14 @@ static void mmc_sdio_remove(struct mmc_host *host)
 }

 /*
+ * Card detection - card is alive.
+ */
+static int mmc_sdio_alive(struct mmc_host *host)
+{
+	return mmc_select_card(host->card);
+}
+
+/*
  * Card detection callback from host.
  */
 static void mmc_sdio_detect(struct mmc_host *host)
@@ -841,7 +849,7 @@ static void mmc_sdio_detect(struct mmc_host *host)
 	/*
 	 * Just check if our card has been removed.
 	 */
-	err = mmc_select_card(host->card);
+	err = _mmc_card_removed(host, 1);

 	mmc_release_host(host);

@@ -1019,6 +1027,7 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 	.suspend = mmc_sdio_suspend,
 	.resume = mmc_sdio_resume,
 	.power_restore = mmc_sdio_power_restore,
+	.alive = mmc_sdio_alive,
 };


diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 534974c..c81b09f 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -209,6 +209,7 @@ struct mmc_card {
 #define MMC_STATE_HIGHSPEED_DDR (1<<4)		/* card is in high speed mode */
 #define MMC_STATE_ULTRAHIGHSPEED (1<<5)		/* card is in ultra high speed mode */
 #define MMC_CARD_SDXC		(1<<6)		/* card is SDXC */
+#define MMC_CARD_REMOVED	(1<<7)		/* card has been removed */
 	unsigned int		quirks; 	/* card quirks */
 #define MMC_QUIRK_LENIENT_FN0	(1<<0)		/* allow SDIO FN0 writes outside of the VS CCCR range */
 #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1)	/* use func->cur_blksize */
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 174a844..8604472 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -180,6 +180,8 @@ extern int mmc_try_claim_host(struct mmc_host *host);

 extern int mmc_flush_cache(struct mmc_card *);

+extern int mmc_card_removed(struct mmc_host *host);
+
 /**
  *	mmc_claim_host - exclusively claim a host
  *	@host: mmc host to claim
-- 
1.7.6.4

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Sujit Reddy Thumma]

> On Thu, Nov 24, 2011 at 12:27 PM, Sujit Reddy Thumma
> <sthumma@codeaurora.org> wrote:
>> Hi Per,
>>
>> On 11/22/2011 6:40 PM, Per Forlin wrote:
>>>
>>> Hi Sujit,
>>>
>>> On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma
>>> <sthumma@codeaurora.org>  wrote:
>>>>
>>>> Kill block requests when the host knows that the card is
>>>> removed from the slot and is sure that subsequent requests
>>>> are bound to fail. Do this silently so that the block
>>>> layer doesn't output unnecessary error messages.
>>>>
>>>> This patch implements suggestion from Adrian Hunter,
>>>> http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
>>>>
>>>> Signed-off-by: Sujit Reddy Thumma<sthumma@codeaurora.org>
>>>>
>>>> ---
>>>> Changes in v2:
>>>>        - Changed the implementation with further comments from Adrian
>>>>        - Set the card removed flag in bus notifier callbacks
>>>>        - This patch is now dependent on patch from Per Forlin:
>>>>      
>>>>  http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
>>>> ---
>>>>  drivers/mmc/card/block.c |   33 ++++++++++++++++++++++++++++-----
>>>>  drivers/mmc/card/queue.c |    5 +++++
>>>>  drivers/mmc/core/bus.c   |   32 +++++++++++++++++++++++++++++++-
>>>>  drivers/mmc/core/core.c  |    8 +++++++-
>>>>  include/linux/mmc/card.h |    3 +++
>>>>  5 files changed, 74 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>> index edc379e..83956fa 100644
>>>> --- a/drivers/mmc/card/block.c
>>>> +++ b/drivers/mmc/card/block.c
>>>> @@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card
>>>> *card, struct request *req,
>>>>        }
>>>>
>>>>        /* We couldn't get a response from the card.  Give up. */
>>>> -       if (err)
>>>> +       if (err) {
>>>> +               /*
>>>> +                * If the card didn't respond to status command,
>>>> +                * it is likely that card is gone. Flag it as removed,
>>>> +                * mmc_detect_change() cleans the rest.
>>>> +                */
>>>> +               mmc_card_set_removed(card);
>>>>                return ERR_ABORT;
>>>> +       }
>>>>
>>>>        /* Flag ECC errors */
>>>>        if ((status&  R1_CARD_ECC_FAILED) ||
>>>> @@ -1168,6 +1175,9 @@ static inline struct mmc_async_req
>>>> *mmc_blk_resend(struct mmc_card *card,
>>>>                                                   int disable_multi,
>>>>                                                   struct mmc_async_req
>>>> *areq)
>>>>  {
>>>> +       struct mmc_blk_data *md = mmc_get_drvdata(card);
>>>> +       struct request *req = mqrq->req;
>>>> +       int ret;
>>>>        /*
>>>>         * Release host after failure in case the host is needed
>>>>         * by someone else. For instance, if the card is removed the
>>>> @@ -1175,11 +1185,24 @@ static inline struct mmc_async_req
>>>> *mmc_blk_resend(struct mmc_card *card,
>>>>         */
>>>>        mmc_release_host(card->host);
>>>>        mmc_claim_host(card->host);
>>>> -
>>>> -       mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>>>> -       return mmc_start_req(card->host, areq, NULL);
>>>> +       if (mmc_card_removed(card)) {
>>>> +               /*
>>>> +                * End the pending async request without starting
>>>> +                * it when card is removed.
>>>> +                */
>>>> +               spin_lock_irq(&md->lock);
>>>> +               req->cmd_flags |= REQ_QUIET;
>>>> +               do {
>>>> +                       ret = __blk_end_request(req,
>>>> +                                       -EIO, blk_rq_cur_bytes(req));
>>>> +               } while (ret);
>>>> +               spin_unlock_irq(&md->lock);
>>>> +               return NULL;
>>>> +       } else {
>>>> +               mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>>>> +               return mmc_start_req(card->host, areq, NULL);
>>>> +       }
>>>
>>> mmc_blk_resend is only called to resend a request that has failed. If
>>> the card is removed the request will still be issued, but when it
>>> retries it will give up here.
>>
>> Currently, we set the card_removed flag in two places:
>>
>> 1) If the host supports interrupt detection, mmc_detect_change() calls
>> the
>> bus detect method and we set card removed flag in bus notifier call
>> back.
>>
>> 2) When there is a transfer going on (host is claimed by mmcqd) and the
>> card
>> is removed ungracefully, the driver sends an error code to the block
>> layer.
>> mmc_blk_cmd_recovery() tries to send CMD13 to the card which in this
>> case
>> fails and we set the card_removed flag.
>>
>> So when we retry or send next async request we end it in
>> mmc_blk_resend()
>> and there will be no subsequent request to the driver since we are
>> suppressing the requests in the queue layer.
>>
>> So I guess there is no need to add further checks in the
>> __mmc_start_req(),
>> which could be redundant as there are no calls to the core layer from
>> block
>> layer once the card is found to be removed.
>>
>> In summary the sequence I thought would be like this:
>> Case 1:
>> 1) Transfer is going on (host claimed by mmcqd) and card is removed.
>> 2) Driver is in middle of transaction, hence some kind of timeout error
>> is
>> returned.
>> 3) mmc_blk_cmd_recovery(): Block layer does error checking and sets the
>> card
>> as removed.
>> 4) Block layer then retries the request calling mmc_blk_resend().
>> 5) Since the mmc_card_removed is true we end the request here and do not
>> send any request to the core.
>>
>> Case 2:
>> 1) When there is no transfer going on (host->claimed = 0)
>> 2) mmc_detect_change() would set card_removed flag.
>> 3) All the subsequent requests would be killed in queue layer itself.
>>
>>> You have added a check in mmc_wait_for_req(). What about this:
>>
>> mmc_wait_for_req() will be called to send regular CMDs as well, hence we
>> need to add a check.
>>
>>> --------------------------
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index b526036..dcdcb9a 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -287,11 +287,17 @@ static void mmc_wait_done(struct mmc_request
>>> *mrq)
>>>         complete(&mrq->completion);
>>>  }
>>>
>>> -static void __mmc_start_req(struct mmc_host *host, struct mmc_request
>>> *mrq)
>>> +static int __mmc_start_req(struct mmc_host *host, struct mmc_request
>>> *mrq)
>>>  {
>>> -       init_completion(&mrq->completion);
>>> -       mrq->done = mmc_wait_done;
>>> -       mmc_start_request(host, mrq);
>>> +       if (mmc_card_removed(host->card)) {
>>> +              mrq->cmd->error = -ENOMEDIUM;
>>> +              return -ENOMEDIUM;
>>> +       }
>>
>> This is not yet done here. If an async request comes then it will do a
>> wait_for_req_done() for the previous request which returned even without
>> doing a init_completion. So we need to handle this case in
>> mmc_start_req().
>>
> You're right. Thanks for spotting this. init_completion and done must
> be assigned.
> The patch should look like this:
> -------------------------------
> init_completion(&mrq->completion);
> mrq->done = mmc_wait_done;
> if (mmc_card_removed(host->card)) {
>              complete(&mrq->completion);
>              mrq->cmd->error = -ENOMEDIUM;
>              return -ENOMEDIUM;
> }
> mmc_start_request(host, mrq);
> -----------------------
> What about this?

Looks feasible.

>
>>> +
>>> +       init_completion(&mrq->completion);
>>> +       mrq->done = mmc_wait_done;
>>> +       mmc_start_request(host, mrq);
>>> +       return 0;
>>>  }
>>>
>>>  static void mmc_wait_for_req_done(struct mmc_host *host,
>>> @@ -418,7 +424,8 @@ EXPORT_SYMBOL(mmc_start_req);
>>>   */
>>>  void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>>>  {
>>> -       __mmc_start_req(host, mrq);
>>> +       if (__mmc_start_req(host, mrq))
>>> +               return
>>>         mmc_wait_for_req_done(host, mrq);
>>>  }
>>>  EXPORT_SYMBOL(mmc_wait_for_req);
>>> ----------------------------------
>>> This patch will set error to -ENOMEDIUM for both mmc_start_req() and
>>> mmc_wait_for_req()
>>>
>>> mmc_blk_err_check() can check for -ENOMEDIUM and return something like
>>> MMC_BLK_ENOMEDIUM
>>
>> If I understand correctly, the above patch handles a third case:
>> 1) issue_fn is called by the queue layer (for the first request)
>> 2) just before the mmcqd claims host card is removed and the
>> mmc_detect_change has flagged the card as removed.
>> 3) we send the request and before it was sent to host driver
>> __mmc_start_req() sent -ENOMEDIUM and the block layer handles new error
>> ENOMEDIUM.
>>
>> This is a valid case but increases the complexity as we need to take
>> care of
>> the async request which does not know we have returned prematurely
>> without
>> doing init_completion().
> My intention is to simplify the error handling by checking
> is-card-removed in __mmc_start_req() rather than both
> mmc_wait_for_req() and mmc_start_req().
>
> Flow for async:
> 0. *card is removed
> 1. mmc_start_req()
> 2. -> mmc_wait_for_req_done()
> 3. ->->err_check()
> 4. ->->->mmc_blk_cmd_recovery()
> 5. ->->->->mmc_card_set_removed(card)
> 6. skip start next request and returns
> 7. new request from block layer

I guess this new request wouldn't be arrived. Since,
by now card removed flag is set and the blk_fetch_request()
would always give req = NULL.

We have already made sure to kill new requests in prepare stage itself:

@@ -37,6 +39,9 @@ static int mmc_prep_request(struct request_queue *q,
struct request *req)
               return BLKPREP_KILL;
       }

+       if (mq && mmc_card_removed(mq->card))
+               return BLKPREP_KILL;
+
       req->cmd_flags |= REQ_DONTPREP;

Nevertheless, your patch looks feasible. Please let me know if I can add
it or ignore.

> 8. mmc_start_req()
> 9. ->__mmc_start_req()
> 10.new request from block layer
> 11. mmc_start_req()
> 12. -> mmc_wait_for_req_done() /* completion is already completed in
> __mmc_start_req(), checks card-is-removed and returns */
> 13. ->-> err_check() /* prints no warning since error is -ENOMEDIUM */
>
>>
>> I guess we can live with this rare case with just outputting few errors,
>> as
>> the whole idea of patch is to reduce (if not completely eliminate)
>> unnecessary prints in the log output which can bog down cpu if some
>> wired
>> peripheral like UART console is enabled.
>>
>> Please let me know if I am missing something.
>>
> Here's my case. in mmci.c: mmc_detect_change(host->mmc,
> msecs_to_jiffies(500));
> It takes 500 ms until mmc_rescan is called. Of course, I can change
> the timing but let's skip that part for now.
> This means the block layer will continue to issue requests for 500 ms
> after the card is ejected. There will be no warnings for the retries
> because the card-is-removed is checked in resend(), but for new
> incoming requests there will be error prints.

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Per Forlin]

On Thu, Nov 24, 2011 at 7:48 PM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
>
>> On Thu, Nov 24, 2011 at 12:27 PM, Sujit Reddy Thumma
>> <sthumma@codeaurora.org> wrote:
>>> Hi Per,
>>>
>>> On 11/22/2011 6:40 PM, Per Forlin wrote:
>>>>
>>>> Hi Sujit,
>>>>
>>>> On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma
>>>> <sthumma@codeaurora.org>  wrote:
>>>>>
>>>>> Kill block requests when the host knows that the card is
>>>>> removed from the slot and is sure that subsequent requests
>>>>> are bound to fail. Do this silently so that the block
>>>>> layer doesn't output unnecessary error messages.
>>>>>
>>>>> This patch implements suggestion from Adrian Hunter,
>>>>> http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
>>>>>
>>>>> Signed-off-by: Sujit Reddy Thumma<sthumma@codeaurora.org>
>>>>>
>>>>> ---
>>>>> Changes in v2:
>>>>>        - Changed the implementation with further comments from Adrian
>>>>>        - Set the card removed flag in bus notifier callbacks
>>>>>        - This patch is now dependent on patch from Per Forlin:
>>>>>
>>>>>  http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
>>>>> ---
>>>>>  drivers/mmc/card/block.c |   33 ++++++++++++++++++++++++++++-----
>>>>>  drivers/mmc/card/queue.c |    5 +++++
>>>>>  drivers/mmc/core/bus.c   |   32 +++++++++++++++++++++++++++++++-
>>>>>  drivers/mmc/core/core.c  |    8 +++++++-
>>>>>  include/linux/mmc/card.h |    3 +++
>>>>>  5 files changed, 74 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>>> index edc379e..83956fa 100644
>>>>> --- a/drivers/mmc/card/block.c
>>>>> +++ b/drivers/mmc/card/block.c
>>>>> @@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card
>>>>> *card, struct request *req,
>>>>>        }
>>>>>
>>>>>        /* We couldn't get a response from the card.  Give up. */
>>>>> -       if (err)
>>>>> +       if (err) {
>>>>> +               /*
>>>>> +                * If the card didn't respond to status command,
>>>>> +                * it is likely that card is gone. Flag it as removed,
>>>>> +                * mmc_detect_change() cleans the rest.
>>>>> +                */
>>>>> +               mmc_card_set_removed(card);
>>>>>                return ERR_ABORT;
>>>>> +       }
>>>>>
>>>>>        /* Flag ECC errors */
>>>>>        if ((status&  R1_CARD_ECC_FAILED) ||
>>>>> @@ -1168,6 +1175,9 @@ static inline struct mmc_async_req
>>>>> *mmc_blk_resend(struct mmc_card *card,
>>>>>                                                   int disable_multi,
>>>>>                                                   struct mmc_async_req
>>>>> *areq)
>>>>>  {
>>>>> +       struct mmc_blk_data *md = mmc_get_drvdata(card);
>>>>> +       struct request *req = mqrq->req;
>>>>> +       int ret;
>>>>>        /*
>>>>>         * Release host after failure in case the host is needed
>>>>>         * by someone else. For instance, if the card is removed the
>>>>> @@ -1175,11 +1185,24 @@ static inline struct mmc_async_req
>>>>> *mmc_blk_resend(struct mmc_card *card,
>>>>>         */
>>>>>        mmc_release_host(card->host);
>>>>>        mmc_claim_host(card->host);
>>>>> -
>>>>> -       mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>>>>> -       return mmc_start_req(card->host, areq, NULL);
>>>>> +       if (mmc_card_removed(card)) {
>>>>> +               /*
>>>>> +                * End the pending async request without starting
>>>>> +                * it when card is removed.
>>>>> +                */
>>>>> +               spin_lock_irq(&md->lock);
>>>>> +               req->cmd_flags |= REQ_QUIET;
>>>>> +               do {
>>>>> +                       ret = __blk_end_request(req,
>>>>> +                                       -EIO, blk_rq_cur_bytes(req));
>>>>> +               } while (ret);
>>>>> +               spin_unlock_irq(&md->lock);
>>>>> +               return NULL;
>>>>> +       } else {
>>>>> +               mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
>>>>> +               return mmc_start_req(card->host, areq, NULL);
>>>>> +       }
>>>>
>>>> mmc_blk_resend is only called to resend a request that has failed. If
>>>> the card is removed the request will still be issued, but when it
>>>> retries it will give up here.
>>>
>>> Currently, we set the card_removed flag in two places:
>>>
>>> 1) If the host supports interrupt detection, mmc_detect_change() calls
>>> the
>>> bus detect method and we set card removed flag in bus notifier call
>>> back.
>>>
>>> 2) When there is a transfer going on (host is claimed by mmcqd) and the
>>> card
>>> is removed ungracefully, the driver sends an error code to the block
>>> layer.
>>> mmc_blk_cmd_recovery() tries to send CMD13 to the card which in this
>>> case
>>> fails and we set the card_removed flag.
>>>
>>> So when we retry or send next async request we end it in
>>> mmc_blk_resend()
>>> and there will be no subsequent request to the driver since we are
>>> suppressing the requests in the queue layer.
>>>
>>> So I guess there is no need to add further checks in the
>>> __mmc_start_req(),
>>> which could be redundant as there are no calls to the core layer from
>>> block
>>> layer once the card is found to be removed.
>>>
>>> In summary the sequence I thought would be like this:
>>> Case 1:
>>> 1) Transfer is going on (host claimed by mmcqd) and card is removed.
>>> 2) Driver is in middle of transaction, hence some kind of timeout error
>>> is
>>> returned.
>>> 3) mmc_blk_cmd_recovery(): Block layer does error checking and sets the
>>> card
>>> as removed.
>>> 4) Block layer then retries the request calling mmc_blk_resend().
>>> 5) Since the mmc_card_removed is true we end the request here and do not
>>> send any request to the core.
>>>
>>> Case 2:
>>> 1) When there is no transfer going on (host->claimed = 0)
>>> 2) mmc_detect_change() would set card_removed flag.
>>> 3) All the subsequent requests would be killed in queue layer itself.
>>>
>>>> You have added a check in mmc_wait_for_req(). What about this:
>>>
>>> mmc_wait_for_req() will be called to send regular CMDs as well, hence we
>>> need to add a check.
>>>
>>>> --------------------------
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index b526036..dcdcb9a 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -287,11 +287,17 @@ static void mmc_wait_done(struct mmc_request
>>>> *mrq)
>>>>         complete(&mrq->completion);
>>>>  }
>>>>
>>>> -static void __mmc_start_req(struct mmc_host *host, struct mmc_request
>>>> *mrq)
>>>> +static int __mmc_start_req(struct mmc_host *host, struct mmc_request
>>>> *mrq)
>>>>  {
>>>> -       init_completion(&mrq->completion);
>>>> -       mrq->done = mmc_wait_done;
>>>> -       mmc_start_request(host, mrq);
>>>> +       if (mmc_card_removed(host->card)) {
>>>> +              mrq->cmd->error = -ENOMEDIUM;
>>>> +              return -ENOMEDIUM;
>>>> +       }
>>>
>>> This is not yet done here. If an async request comes then it will do a
>>> wait_for_req_done() for the previous request which returned even without
>>> doing a init_completion. So we need to handle this case in
>>> mmc_start_req().
>>>
>> You're right. Thanks for spotting this. init_completion and done must
>> be assigned.
>> The patch should look like this:
>> -------------------------------
>> init_completion(&mrq->completion);
>> mrq->done = mmc_wait_done;
>> if (mmc_card_removed(host->card)) {
>>              complete(&mrq->completion);
>>              mrq->cmd->error = -ENOMEDIUM;
>>              return -ENOMEDIUM;
>> }
>> mmc_start_request(host, mrq);
>> -----------------------
>> What about this?
>
> Looks feasible.
>
>>
>>>> +
>>>> +       init_completion(&mrq->completion);
>>>> +       mrq->done = mmc_wait_done;
>>>> +       mmc_start_request(host, mrq);
>>>> +       return 0;
>>>>  }
>>>>
>>>>  static void mmc_wait_for_req_done(struct mmc_host *host,
>>>> @@ -418,7 +424,8 @@ EXPORT_SYMBOL(mmc_start_req);
>>>>   */
>>>>  void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>>>>  {
>>>> -       __mmc_start_req(host, mrq);
>>>> +       if (__mmc_start_req(host, mrq))
>>>> +               return
>>>>         mmc_wait_for_req_done(host, mrq);
>>>>  }
>>>>  EXPORT_SYMBOL(mmc_wait_for_req);
>>>> ----------------------------------
>>>> This patch will set error to -ENOMEDIUM for both mmc_start_req() and
>>>> mmc_wait_for_req()
>>>>
>>>> mmc_blk_err_check() can check for -ENOMEDIUM and return something like
>>>> MMC_BLK_ENOMEDIUM
>>>
>>> If I understand correctly, the above patch handles a third case:
>>> 1) issue_fn is called by the queue layer (for the first request)
>>> 2) just before the mmcqd claims host card is removed and the
>>> mmc_detect_change has flagged the card as removed.
>>> 3) we send the request and before it was sent to host driver
>>> __mmc_start_req() sent -ENOMEDIUM and the block layer handles new error
>>> ENOMEDIUM.
>>>
>>> This is a valid case but increases the complexity as we need to take
>>> care of
>>> the async request which does not know we have returned prematurely
>>> without
>>> doing init_completion().
>> My intention is to simplify the error handling by checking
>> is-card-removed in __mmc_start_req() rather than both
>> mmc_wait_for_req() and mmc_start_req().
>>
>> Flow for async:
>> 0. *card is removed
>> 1. mmc_start_req()
>> 2. -> mmc_wait_for_req_done()
>> 3. ->->err_check()
>> 4. ->->->mmc_blk_cmd_recovery()
>> 5. ->->->->mmc_card_set_removed(card)
>> 6. skip start next request and returns
>> 7. new request from block layer
>
> I guess this new request wouldn't be arrived. Since,
> by now card removed flag is set and the blk_fetch_request()
> would always give req = NULL.
>
> We have already made sure to kill new requests in prepare stage itself:
>
Yes you are right. I forgot about that.

> @@ -37,6 +39,9 @@ static int mmc_prep_request(struct request_queue *q,
> struct request *req)
>               return BLKPREP_KILL;
>       }
>
> +       if (mq && mmc_card_removed(mq->card))
> +               return BLKPREP_KILL;
> +
>       req->cmd_flags |= REQ_DONTPREP;
>
> Nevertheless, your patch looks feasible. Please let me know if I can add
> it or ignore.
>
I see, the change I proposed doesn't really solve anything. The only
reason for adding it would be to make the code cleaner.
Personally I don't mind a few extra error prints for both commands and
data transfers after the card has been removed as long as new requests
are stopped from coming in.
Two options
* add my change and add check for -ENOMEDIUM in the mmc_blk_err_check function.
* only check for card-is-removed in block_prep(), let ongoing
transfers try and fail.

Thanks,
Per

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Sujit Reddy Thumma]

Hi Adrian,

On 11/24/2011 8:22 PM, Adrian Hunter wrote:
> Hi
>
> Here is a way to allow mmc block to determine immediately
> if a card has been removed while preserving the present rules
> and keeping card detection in the bus_ops.
>
> Untested I'm afraid.
>
> Regards
> Adrian
>
>
>
>  From 2c6c9535b94c07fa3d12af26e9b6de500cb29970 Mon Sep 17 00:00:00 2001
> From: Adrian Hunter<adrian.hunter@intel.com>
> Date: Thu, 24 Nov 2011 16:32:34 +0200
> Subject: [PATCH] mmc: allow upper layers to determine immediately if a card has been removed
>
> Add a function mmc_card_removed() which upper layers can use
> to determine immediately if a card has been removed.  This
> function should be called after an I/O request fails so that
> all queued I/O requests can be errored out immediately
> instead of waiting for the card device to be removed.
>
> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
> ---
>   drivers/mmc/core/core.c  |   32 ++++++++++++++++++++++++++++++++
>   drivers/mmc/core/core.h  |    3 +++
>   drivers/mmc/core/mmc.c   |   12 +++++++++++-
>   drivers/mmc/core/sd.c    |   12 +++++++++++-
>   drivers/mmc/core/sdio.c  |   11 ++++++++++-
>   include/linux/mmc/card.h |    1 +
>   include/linux/mmc/core.h |    2 ++
>   7 files changed, 70 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 271efea..c317564 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2049,6 +2049,38 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>   	return -EIO;
>   }
>
> +int _mmc_card_removed(struct mmc_host *host, int detect_change)
> +{
> +	int ret;
> +
> +	if (!(host->caps&  MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
> +		return 0;
> +
> +	if (!host->card || (host->card->state&  MMC_CARD_REMOVED))
> +		return 1;
> +
> +	/*
> +	 * The card will be considered alive unless we have been asked to detect
> +	 * a change or host requires polling to provide card detection.
> +	 */
> +	if (!detect_change&&  !(host->caps&  MMC_CAP_NEEDS_POLL))
> +		return 0;
> +
> +	ret = host->bus_ops->alive(host);
> +	if (ret)
> +		host->card->state |= MMC_CARD_REMOVED;
> +
> +	return ret;
> +}
> +

The patch itself looks good to me, but can't we decide this in the block 
layer itself when we issue get_card_status() when the ongoing request fails?

----------------------------------------------
         for (retry = 2; retry >= 0; retry--) {
                 err = get_card_status(card, &status, 0);
                 if (!err)
                         break;

                 prev_cmd_status_valid = false;
                 pr_err("%s: error %d sending status command, %sing\n",
                        req->rq_disk->disk_name, err, retry ? "retry" : 
"abort");
         }


  	/* We couldn't get a response from the card.  Give up. */
-	if (err)
+	if (err) {
+		/*
+		 * If the card didn't respond to status command,
+		 * it is likely that card is gone. Flag it as removed,
+		 * mmc_detect_change() cleans the rest.
+		 */
+		mmc_card_set_removed(card);
  		return ERR_ABORT;
+	}
------------------------------------------------

The V2 patch I have posted takes care of this. Please let me know if it 
not good to decide in the block layer itself. Essentially, both the 
implementations are sending CMD13 to know the status of the card.

> +int mmc_card_removed(struct mmc_host *host)
> +{
> +	WARN_ON(!host->claimed);
> +
> +	return _mmc_card_removed(host, work_pending(&host->detect.work));
> +}
> +EXPORT_SYMBOL(mmc_card_removed);
> +
>   void mmc_rescan(struct work_struct *work)
>   {
>   	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 14664f1..4b6b10e 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -24,6 +24,7 @@ struct mmc_bus_ops {
>   	int (*resume)(struct mmc_host *);
>   	int (*power_save)(struct mmc_host *);
>   	int (*power_restore)(struct mmc_host *);
> +	int (*alive)(struct mmc_host *);
>   };
>
>   void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
> @@ -59,6 +60,8 @@ 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_card_removed(struct mmc_host *host, int detect_change);
> +
>   int mmc_attach_mmc(struct mmc_host *host);
>   int mmc_attach_sd(struct mmc_host *host);
>   int mmc_attach_sdio(struct mmc_host *host);
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index a1223bd..5841d08 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1104,6 +1104,14 @@ static void mmc_remove(struct mmc_host *host)
>   }
>
>   /*
> + * Card detection - card is alive.
> + */
> +static int mmc_alive(struct mmc_host *host)
> +{
> +	return mmc_send_status(host->card, NULL);
> +}
> +
> +/*
>    * Card detection callback from host.
>    */
>   static void mmc_detect(struct mmc_host *host)
> @@ -1118,7 +1126,7 @@ static void mmc_detect(struct mmc_host *host)
>   	/*
>   	 * Just check if our card has been removed.
>   	 */
> -	err = mmc_send_status(host->card, NULL);
> +	err = _mmc_card_removed(host, 1);
>
>   	mmc_release_host(host);
>
> @@ -1223,6 +1231,7 @@ static const struct mmc_bus_ops mmc_ops = {
>   	.suspend = NULL,
>   	.resume = NULL,
>   	.power_restore = mmc_power_restore,
> +	.alive = mmc_alive,
>   };
>
>   static const struct mmc_bus_ops mmc_ops_unsafe = {
> @@ -1233,6 +1242,7 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
>   	.suspend = mmc_suspend,
>   	.resume = mmc_resume,
>   	.power_restore = mmc_power_restore,
> +	.alive = mmc_alive,
>   };
>
>   static void mmc_attach_bus_ops(struct mmc_host *host)
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 1d5a3bd..82855ed 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -1018,6 +1018,14 @@ static void mmc_sd_remove(struct mmc_host *host)
>   }
>
>   /*
> + * Card detection - card is alive.
> + */
> +static int mmc_sd_alive(struct mmc_host *host)
> +{
> +	return mmc_send_status(host->card, NULL);
> +}
> +
> +/*
>    * Card detection callback from host.
>    */
>   static void mmc_sd_detect(struct mmc_host *host)
> @@ -1032,7 +1040,7 @@ static void mmc_sd_detect(struct mmc_host *host)
>   	/*
>   	 * Just check if our card has been removed.
>   	 */
> -	err = mmc_send_status(host->card, NULL);
> +	err = _mmc_card_removed(host, 1);
>
>   	mmc_release_host(host);
>
> @@ -1101,6 +1109,7 @@ static const struct mmc_bus_ops mmc_sd_ops = {
>   	.suspend = NULL,
>   	.resume = NULL,
>   	.power_restore = mmc_sd_power_restore,
> +	.alive = mmc_sd_alive,
>   };
>
>   static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
> @@ -1109,6 +1118,7 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
>   	.suspend = mmc_sd_suspend,
>   	.resume = mmc_sd_resume,
>   	.power_restore = mmc_sd_power_restore,
> +	.alive = mmc_sd_alive,
>   };
>
>   static void mmc_sd_attach_bus_ops(struct mmc_host *host)
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 8c04f7f..ca5c4f9 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -820,6 +820,14 @@ static void mmc_sdio_remove(struct mmc_host *host)
>   }
>
>   /*
> + * Card detection - card is alive.
> + */
> +static int mmc_sdio_alive(struct mmc_host *host)
> +{
> +	return mmc_select_card(host->card);
> +}
> +
> +/*
>    * Card detection callback from host.
>    */
>   static void mmc_sdio_detect(struct mmc_host *host)
> @@ -841,7 +849,7 @@ static void mmc_sdio_detect(struct mmc_host *host)
>   	/*
>   	 * Just check if our card has been removed.
>   	 */
> -	err = mmc_select_card(host->card);
> +	err = _mmc_card_removed(host, 1);
>
>   	mmc_release_host(host);
>
> @@ -1019,6 +1027,7 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
>   	.suspend = mmc_sdio_suspend,
>   	.resume = mmc_sdio_resume,
>   	.power_restore = mmc_sdio_power_restore,
> +	.alive = mmc_sdio_alive,
>   };
>
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 534974c..c81b09f 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -209,6 +209,7 @@ struct mmc_card {
>   #define MMC_STATE_HIGHSPEED_DDR (1<<4)		/* card is in high speed mode */
>   #define MMC_STATE_ULTRAHIGHSPEED (1<<5)		/* card is in ultra high speed mode */
>   #define MMC_CARD_SDXC		(1<<6)		/* card is SDXC */
> +#define MMC_CARD_REMOVED	(1<<7)		/* card has been removed */
>   	unsigned int		quirks; 	/* card quirks */
>   #define MMC_QUIRK_LENIENT_FN0	(1<<0)		/* allow SDIO FN0 writes outside of the VS CCCR range */
>   #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1)	/* use func->cur_blksize */
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 174a844..8604472 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -180,6 +180,8 @@ extern int mmc_try_claim_host(struct mmc_host *host);
>
>   extern int mmc_flush_cache(struct mmc_card *);
>
> +extern int mmc_card_removed(struct mmc_host *host);
> +
>   /**
>    *	mmc_claim_host - exclusively claim a host
>    *	@host: mmc host to claim


-- 
Thanks,
Sujit

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Adrian Hunter]

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

On 25/11/11 12:26, Sujit Reddy Thumma wrote:
> Hi Adrian,
> 
> On 11/24/2011 8:22 PM, Adrian Hunter wrote:
>> Hi
>>
>> Here is a way to allow mmc block to determine immediately
>> if a card has been removed while preserving the present rules
>> and keeping card detection in the bus_ops.
>>
>> Untested I'm afraid.
>>
>> Regards
>> Adrian
>>
>>
>>
>>  From 2c6c9535b94c07fa3d12af26e9b6de500cb29970 Mon Sep 17 00:00:00 2001
>> From: Adrian Hunter<adrian.hunter@intel.com>
>> Date: Thu, 24 Nov 2011 16:32:34 +0200
>> Subject: [PATCH] mmc: allow upper layers to determine immediately if a
>> card has been removed
>>
>> Add a function mmc_card_removed() which upper layers can use
>> to determine immediately if a card has been removed.  This
>> function should be called after an I/O request fails so that
>> all queued I/O requests can be errored out immediately
>> instead of waiting for the card device to be removed.
>>
>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>> ---
>>   drivers/mmc/core/core.c  |   32 ++++++++++++++++++++++++++++++++
>>   drivers/mmc/core/core.h  |    3 +++
>>   drivers/mmc/core/mmc.c   |   12 +++++++++++-
>>   drivers/mmc/core/sd.c    |   12 +++++++++++-
>>   drivers/mmc/core/sdio.c  |   11 ++++++++++-
>>   include/linux/mmc/card.h |    1 +
>>   include/linux/mmc/core.h |    2 ++
>>   7 files changed, 70 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 271efea..c317564 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2049,6 +2049,38 @@ static int mmc_rescan_try_freq(struct mmc_host
>> *host, unsigned freq)
>>       return -EIO;
>>   }
>>
>> +int _mmc_card_removed(struct mmc_host *host, int detect_change)
>> +{
>> +    int ret;
>> +
>> +    if (!(host->caps&  MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
>> +        return 0;
>> +
>> +    if (!host->card || (host->card->state&  MMC_CARD_REMOVED))
>> +        return 1;
>> +
>> +    /*
>> +     * The card will be considered alive unless we have been asked to detect
>> +     * a change or host requires polling to provide card detection.
>> +     */
>> +    if (!detect_change&&  !(host->caps&  MMC_CAP_NEEDS_POLL))
>> +        return 0;
>> +
>> +    ret = host->bus_ops->alive(host);
>> +    if (ret)
>> +        host->card->state |= MMC_CARD_REMOVED;
>> +
>> +    return ret;
>> +}
>> +
> 
> The patch itself looks good to me, but can't we decide this in the block
> layer itself when we issue get_card_status() when the ongoing request fails?
> 
> ----------------------------------------------
>         for (retry = 2; retry >= 0; retry--) {
>                 err = get_card_status(card, &status, 0);
>                 if (!err)
>                         break;
> 
>                 prev_cmd_status_valid = false;
>                 pr_err("%s: error %d sending status command, %sing\n",
>                        req->rq_disk->disk_name, err, retry ? "retry" :
> "abort");
>         }
> 
> 
>      /* We couldn't get a response from the card.  Give up. */
> -    if (err)
> +    if (err) {
> +        /*
> +         * If the card didn't respond to status command,
> +         * it is likely that card is gone. Flag it as removed,
> +         * mmc_detect_change() cleans the rest.
> +         */
> +        mmc_card_set_removed(card);
>          return ERR_ABORT;
> +    }
> ------------------------------------------------
> 
> The V2 patch I have posted takes care of this. Please let me know if it not
> good to decide in the block layer itself. Essentially, both the
> implementations are sending CMD13 to know the status of the card.

I think it is better to have the decision over whether or not the card
has been removed in only one place.  Also, never flagging
MMC_CAP_NONREMOVABLE cards as removed nor if the HC driver has not called
mmc_detect_change.

But the block change is essentially the same:

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c80bb6d..32590c3 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -632,6 +632,8 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
struct request *req,
 	u32 status, stop_status = 0;
 	int err, retry;

+	if (mmc_card_removed(card))
+		return ERR_ABORT;
 	/*
 	 * Try to get card status which indicates both the card state
 	 * and why there was no response.  If the first attempt fails,
@@ -648,8 +650,10 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
struct request *req,
 	}

 	/* We couldn't get a response from the card.  Give up. */
-	if (err)
+	if (err) {
+		mmc_detect_card_removed(card->host);
 		return ERR_ABORT;
+	}

 	/* Flag ECC errors */
 	if ((status & R1_CARD_ECC_FAILED) ||

I attached a revised version of the patch adding -ENOMEDIUM
errors from __mmc_start_request as you and Per discussed.

[-- Attachment #2: 0001-mmc-allow-upper-layers-to-determine-immediately-if-a.patch --]
[-- Type: text/x-patch, Size: 9183 bytes --]

From: Adrian Hunter <adrian.hunter@intel.com>
Date: Thu, 24 Nov 2011 16:32:34 +0200
Subject: [PATCH V2] mmc: allow upper layers to determine immediately if a card has been removed

Add a function mmc_detect_card_removed() which upper layers
can use to determine immediately if a card has been removed.
This function should be called after an I/O request fails so
that all queued I/O requests can be errored out immediately
instead of waiting for the card device to be removed.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c  |   44 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/mmc/core/core.h  |    3 +++
 drivers/mmc/core/mmc.c   |   12 +++++++++++-
 drivers/mmc/core/sd.c    |   12 +++++++++++-
 drivers/mmc/core/sdio.c  |   11 ++++++++++-
 include/linux/mmc/card.h |    3 +++
 include/linux/mmc/core.h |    2 ++
 7 files changed, 82 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 271efea..74463ef 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -140,7 +140,7 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
 			cmd->retries = 0;
 	}
 
-	if (err && cmd->retries) {
+	if (err && cmd->retries && !mmc_card_removed(host->card)) {
 		/*
 		 * Request starter must handle retries - see
 		 * mmc_wait_for_req_done().
@@ -247,6 +247,11 @@ static void __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
 {
 	init_completion(&mrq->completion);
 	mrq->done = mmc_wait_done;
+	if (mmc_card_removed(host->card)) {
+		mrq->cmd->error = -ENOMEDIUM;
+		complete(&mrq->completion);
+		return;
+	}
 	mmc_start_request(host, mrq);
 }
 
@@ -259,7 +264,8 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
 		wait_for_completion(&mrq->completion);
 
 		cmd = mrq->cmd;
-		if (!cmd->error || !cmd->retries)
+		if (!cmd->error || !cmd->retries ||
+		    mmc_card_removed(host->card))
 			break;
 
 		pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
@@ -2049,6 +2055,40 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
 	return -EIO;
 }
 
+int _mmc_detect_card_removed(struct mmc_host *host, int detect_change)
+{
+	int ret;
+
+	if (!(host->caps & MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
+		return 0;
+
+	if (!host->card || mmc_card_removed(host->card))
+		return 1;
+
+	/*
+	 * The card will be considered alive unless we have been asked to detect
+	 * a change or host requires polling to provide card detection.
+	 */
+	if (!detect_change && !(host->caps & MMC_CAP_NEEDS_POLL))
+		return 0;
+
+	ret = host->bus_ops->alive(host);
+	if (ret) {
+		mmc_card_set_removed(host->card);
+		pr_info("%s: card removed\n", mmc_hostname(host));
+	}
+
+	return ret;
+}
+
+int mmc_detect_card_removed(struct mmc_host *host)
+{
+	WARN_ON(!host->claimed);
+
+	return _mmc_detect_card_removed(host, work_pending(&host->detect.work));
+}
+EXPORT_SYMBOL(mmc_detect_card_removed);
+
 void mmc_rescan(struct work_struct *work)
 {
 	static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 14664f1..1d3fdfd 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -24,6 +24,7 @@ struct mmc_bus_ops {
 	int (*resume)(struct mmc_host *);
 	int (*power_save)(struct mmc_host *);
 	int (*power_restore)(struct mmc_host *);
+	int (*alive)(struct mmc_host *);
 };
 
 void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops);
@@ -59,6 +60,8 @@ 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_detect_card_removed(struct mmc_host *host, int detect_change);
+
 int mmc_attach_mmc(struct mmc_host *host);
 int mmc_attach_sd(struct mmc_host *host);
 int mmc_attach_sdio(struct mmc_host *host);
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index a1223bd..4c2c6d5 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1104,6 +1104,14 @@ static void mmc_remove(struct mmc_host *host)
 }
 
 /*
+ * Card detection - card is alive.
+ */
+static int mmc_alive(struct mmc_host *host)
+{
+	return mmc_send_status(host->card, NULL);
+}
+
+/*
  * Card detection callback from host.
  */
 static void mmc_detect(struct mmc_host *host)
@@ -1118,7 +1126,7 @@ static void mmc_detect(struct mmc_host *host)
 	/*
 	 * Just check if our card has been removed.
 	 */
-	err = mmc_send_status(host->card, NULL);
+	err = _mmc_detect_card_removed(host, 1);
 
 	mmc_release_host(host);
 
@@ -1223,6 +1231,7 @@ static const struct mmc_bus_ops mmc_ops = {
 	.suspend = NULL,
 	.resume = NULL,
 	.power_restore = mmc_power_restore,
+	.alive = mmc_alive,
 };
 
 static const struct mmc_bus_ops mmc_ops_unsafe = {
@@ -1233,6 +1242,7 @@ static const struct mmc_bus_ops mmc_ops_unsafe = {
 	.suspend = mmc_suspend,
 	.resume = mmc_resume,
 	.power_restore = mmc_power_restore,
+	.alive = mmc_alive,
 };
 
 static void mmc_attach_bus_ops(struct mmc_host *host)
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 1d5a3bd..9ed598b 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1018,6 +1018,14 @@ static void mmc_sd_remove(struct mmc_host *host)
 }
 
 /*
+ * Card detection - card is alive.
+ */
+static int mmc_sd_alive(struct mmc_host *host)
+{
+	return mmc_send_status(host->card, NULL);
+}
+
+/*
  * Card detection callback from host.
  */
 static void mmc_sd_detect(struct mmc_host *host)
@@ -1032,7 +1040,7 @@ static void mmc_sd_detect(struct mmc_host *host)
 	/*
 	 * Just check if our card has been removed.
 	 */
-	err = mmc_send_status(host->card, NULL);
+	err = _mmc_detect_card_removed(host, 1);
 
 	mmc_release_host(host);
 
@@ -1101,6 +1109,7 @@ static const struct mmc_bus_ops mmc_sd_ops = {
 	.suspend = NULL,
 	.resume = NULL,
 	.power_restore = mmc_sd_power_restore,
+	.alive = mmc_sd_alive,
 };
 
 static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
@@ -1109,6 +1118,7 @@ static const struct mmc_bus_ops mmc_sd_ops_unsafe = {
 	.suspend = mmc_sd_suspend,
 	.resume = mmc_sd_resume,
 	.power_restore = mmc_sd_power_restore,
+	.alive = mmc_sd_alive,
 };
 
 static void mmc_sd_attach_bus_ops(struct mmc_host *host)
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 8c04f7f..107c382 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -820,6 +820,14 @@ static void mmc_sdio_remove(struct mmc_host *host)
 }
 
 /*
+ * Card detection - card is alive.
+ */
+static int mmc_sdio_alive(struct mmc_host *host)
+{
+	return mmc_select_card(host->card);
+}
+
+/*
  * Card detection callback from host.
  */
 static void mmc_sdio_detect(struct mmc_host *host)
@@ -841,7 +849,7 @@ static void mmc_sdio_detect(struct mmc_host *host)
 	/*
 	 * Just check if our card has been removed.
 	 */
-	err = mmc_select_card(host->card);
+	err = _mmc_detect_card_removed(host, 1);
 
 	mmc_release_host(host);
 
@@ -1019,6 +1027,7 @@ static const struct mmc_bus_ops mmc_sdio_ops = {
 	.suspend = mmc_sdio_suspend,
 	.resume = mmc_sdio_resume,
 	.power_restore = mmc_sdio_power_restore,
+	.alive = mmc_sdio_alive,
 };
 
 
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 534974c..6402d92 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -209,6 +209,7 @@ struct mmc_card {
 #define MMC_STATE_HIGHSPEED_DDR (1<<4)		/* card is in high speed mode */
 #define MMC_STATE_ULTRAHIGHSPEED (1<<5)		/* card is in ultra high speed mode */
 #define MMC_CARD_SDXC		(1<<6)		/* card is SDXC */
+#define MMC_CARD_REMOVED	(1<<7)		/* card has been removed */
 	unsigned int		quirks; 	/* card quirks */
 #define MMC_QUIRK_LENIENT_FN0	(1<<0)		/* allow SDIO FN0 writes outside of the VS CCCR range */
 #define MMC_QUIRK_BLKSZ_FOR_BYTE_MODE (1<<1)	/* use func->cur_blksize */
@@ -370,6 +371,7 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
 #define mmc_card_uhs(c)		((c)->state & MMC_STATE_ULTRAHIGHSPEED)
 #define mmc_sd_card_uhs(c)	((c)->state & MMC_STATE_ULTRAHIGHSPEED)
 #define mmc_card_ext_capacity(c) ((c)->state & MMC_CARD_SDXC)
+#define mmc_card_removed(c)	((c) && ((c)->state & MMC_CARD_REMOVED))
 
 #define mmc_card_set_present(c)	((c)->state |= MMC_STATE_PRESENT)
 #define mmc_card_set_readonly(c) ((c)->state |= MMC_STATE_READONLY)
@@ -379,6 +381,7 @@ static inline void __maybe_unused remove_quirk(struct mmc_card *card, int data)
 #define mmc_card_set_uhs(c) ((c)->state |= MMC_STATE_ULTRAHIGHSPEED)
 #define mmc_sd_card_set_uhs(c) ((c)->state |= MMC_STATE_ULTRAHIGHSPEED)
 #define mmc_card_set_ext_capacity(c) ((c)->state |= MMC_CARD_SDXC)
+#define mmc_card_set_removed(c) ((c)->state |= MMC_CARD_REMOVED)
 
 /*
  * Quirk add/remove for MMC products.
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 174a844..87a976c 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -180,6 +180,8 @@ extern int mmc_try_claim_host(struct mmc_host *host);
 
 extern int mmc_flush_cache(struct mmc_card *);
 
+extern int mmc_detect_card_removed(struct mmc_host *host);
+
 /**
  *	mmc_claim_host - exclusively claim a host
  *	@host: mmc host to claim
-- 
1.7.6.4

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Sujit Reddy Thumma]

Hi Adrian,

On 11/25/2011 4:58 PM, Adrian Hunter wrote:
> On 25/11/11 12:26, Sujit Reddy Thumma wrote:
>> Hi Adrian,
>>
>> On 11/24/2011 8:22 PM, Adrian Hunter wrote:
>>> Hi
>>>
>>> Here is a way to allow mmc block to determine immediately
>>> if a card has been removed while preserving the present rules
>>> and keeping card detection in the bus_ops.
>>>
>>> Untested I'm afraid.
>>>
>>> Regards
>>> Adrian
>>>
>>>
>>>
>>>   From 2c6c9535b94c07fa3d12af26e9b6de500cb29970 Mon Sep 17 00:00:00 2001
>>> From: Adrian Hunter<adrian.hunter@intel.com>
>>> Date: Thu, 24 Nov 2011 16:32:34 +0200
>>> Subject: [PATCH] mmc: allow upper layers to determine immediately if a
>>> card has been removed
>>>
>>> Add a function mmc_card_removed() which upper layers can use
>>> to determine immediately if a card has been removed.  This
>>> function should be called after an I/O request fails so that
>>> all queued I/O requests can be errored out immediately
>>> instead of waiting for the card device to be removed.
>>>
>>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>>> ---
>>>    drivers/mmc/core/core.c  |   32 ++++++++++++++++++++++++++++++++
>>>    drivers/mmc/core/core.h  |    3 +++
>>>    drivers/mmc/core/mmc.c   |   12 +++++++++++-
>>>    drivers/mmc/core/sd.c    |   12 +++++++++++-
>>>    drivers/mmc/core/sdio.c  |   11 ++++++++++-
>>>    include/linux/mmc/card.h |    1 +
>>>    include/linux/mmc/core.h |    2 ++
>>>    7 files changed, 70 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 271efea..c317564 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -2049,6 +2049,38 @@ static int mmc_rescan_try_freq(struct mmc_host
>>> *host, unsigned freq)
>>>        return -EIO;
>>>    }
>>>
>>> +int _mmc_card_removed(struct mmc_host *host, int detect_change)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (!(host->caps&   MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
>>> +        return 0;
>>> +
>>> +    if (!host->card || (host->card->state&   MMC_CARD_REMOVED))
>>> +        return 1;
>>> +
>>> +    /*
>>> +     * The card will be considered alive unless we have been asked to detect
>>> +     * a change or host requires polling to provide card detection.
>>> +     */
>>> +    if (!detect_change&&   !(host->caps&   MMC_CAP_NEEDS_POLL))
>>> +        return 0;
>>> +
>>> +    ret = host->bus_ops->alive(host);
>>> +    if (ret)
>>> +        host->card->state |= MMC_CARD_REMOVED;
>>> +
>>> +    return ret;
>>> +}
>>> +
>>
>> The patch itself looks good to me, but can't we decide this in the block
>> layer itself when we issue get_card_status() when the ongoing request fails?
>>
>> ----------------------------------------------
>>          for (retry = 2; retry>= 0; retry--) {
>>                  err = get_card_status(card,&status, 0);
>>                  if (!err)
>>                          break;
>>
>>                  prev_cmd_status_valid = false;
>>                  pr_err("%s: error %d sending status command, %sing\n",
>>                         req->rq_disk->disk_name, err, retry ? "retry" :
>> "abort");
>>          }
>>
>>
>>       /* We couldn't get a response from the card.  Give up. */
>> -    if (err)
>> +    if (err) {
>> +        /*
>> +         * If the card didn't respond to status command,
>> +         * it is likely that card is gone. Flag it as removed,
>> +         * mmc_detect_change() cleans the rest.
>> +         */
>> +        mmc_card_set_removed(card);
>>           return ERR_ABORT;
>> +    }
>> ------------------------------------------------
>>
>> The V2 patch I have posted takes care of this. Please let me know if it not
>> good to decide in the block layer itself. Essentially, both the
>> implementations are sending CMD13 to know the status of the card.
>
> I think it is better to have the decision over whether or not the card
> has been removed in only one place.  Also, never flagging
> MMC_CAP_NONREMOVABLE cards as removed nor if the HC driver has not called
> mmc_detect_change.

Agreed. This is much cleaner implementation. Thanks.

>
> But the block change is essentially the same:
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index c80bb6d..32590c3 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -632,6 +632,8 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
> struct request *req,
>   	u32 status, stop_status = 0;
>   	int err, retry;
>
> +	if (mmc_card_removed(card))
> +		return ERR_ABORT;
>   	/*
>   	 * Try to get card status which indicates both the card state
>   	 * and why there was no response.  If the first attempt fails,
> @@ -648,8 +650,10 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
> struct request *req,
>   	}
>
>   	/* We couldn't get a response from the card.  Give up. */
> -	if (err)
> +	if (err) {
> +		mmc_detect_card_removed(card->host);
>   		return ERR_ABORT;
> +	}
>
>   	/* Flag ECC errors */
>   	if ((status&  R1_CARD_ECC_FAILED) ||
>
> I attached a revised version of the patch adding -ENOMEDIUM
> errors from __mmc_start_request as you and Per discussed.

Thanks. I had a look at it and I have some comments:

+int mmc_detect_card_removed(struct mmc_host *host)
+{
+       WARN_ON(!host->claimed);
+
+       return _mmc_detect_card_removed(host, 
work_pending(&host->detect.work));

The work_pending bit would be set only when the HC driver has detected a 
change in the slot and called mmc_detect_change() to queue the detect 
work after a delay. In my case this delay is zero. So as soon as the 
card is removed following happen:

->mmc_detect_change
-->mmc_schedule_delayed_work
--->work_pending bit is set
---->since the delay is zero, work is queued asap
----->just before work->func is called, work_pending bit is cleared
------>work->func() i.e., mmc_rescan is started
------->mmc_rescan waiting for claim host.

So there can be a case where work_pending bit is set as well as cleared 
even before block layer get a chance to call mmc_detect_card_removed(), 
hence the block layer would always see that card is alive even if the 
card is removed until mmc_rescan is completed.

+       /*
+        * The card will be considered alive unless we have been asked 
to detect
+        * a change or host requires polling to provide card detection.
+        */
+       if (!detect_change && !(host->caps & MMC_CAP_NEEDS_POLL))
+               return 0;
+

Since, the main purpose of _mmc_detect_card_removed() is to detect 
whether the card is removed or not I feel "detect_change" is redundant. 
Let me know if you see any other problem without this.


+}
+EXPORT_SYMBOL(mmc_detect_card_removed);
+

-- 
Thanks,
Sujit

Re: [PATCH V2] mmc: Kill block requests if card is removed

[Adrian Hunter]

On 28/11/11 08:23, Sujit Reddy Thumma wrote:
> Hi Adrian,
> 
> On 11/25/2011 4:58 PM, Adrian Hunter wrote:
>> On 25/11/11 12:26, Sujit Reddy Thumma wrote:
>>> Hi Adrian,
>>>
>>> On 11/24/2011 8:22 PM, Adrian Hunter wrote:
>>>> Hi
>>>>
>>>> Here is a way to allow mmc block to determine immediately
>>>> if a card has been removed while preserving the present rules
>>>> and keeping card detection in the bus_ops.
>>>>
>>>> Untested I'm afraid.
>>>>
>>>> Regards
>>>> Adrian
>>>>
>>>>
>>>>
>>>>   From 2c6c9535b94c07fa3d12af26e9b6de500cb29970 Mon Sep 17 00:00:00 2001
>>>> From: Adrian Hunter<adrian.hunter@intel.com>
>>>> Date: Thu, 24 Nov 2011 16:32:34 +0200
>>>> Subject: [PATCH] mmc: allow upper layers to determine immediately if a
>>>> card has been removed
>>>>
>>>> Add a function mmc_card_removed() which upper layers can use
>>>> to determine immediately if a card has been removed.  This
>>>> function should be called after an I/O request fails so that
>>>> all queued I/O requests can be errored out immediately
>>>> instead of waiting for the card device to be removed.
>>>>
>>>> Signed-off-by: Adrian Hunter<adrian.hunter@intel.com>
>>>> ---
>>>>    drivers/mmc/core/core.c  |   32 ++++++++++++++++++++++++++++++++
>>>>    drivers/mmc/core/core.h  |    3 +++
>>>>    drivers/mmc/core/mmc.c   |   12 +++++++++++-
>>>>    drivers/mmc/core/sd.c    |   12 +++++++++++-
>>>>    drivers/mmc/core/sdio.c  |   11 ++++++++++-
>>>>    include/linux/mmc/card.h |    1 +
>>>>    include/linux/mmc/core.h |    2 ++
>>>>    7 files changed, 70 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index 271efea..c317564 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -2049,6 +2049,38 @@ static int mmc_rescan_try_freq(struct mmc_host
>>>> *host, unsigned freq)
>>>>        return -EIO;
>>>>    }
>>>>
>>>> +int _mmc_card_removed(struct mmc_host *host, int detect_change)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    if (!(host->caps&   MMC_CAP_NONREMOVABLE) || !host->bus_ops->alive)
>>>> +        return 0;
>>>> +
>>>> +    if (!host->card || (host->card->state&   MMC_CARD_REMOVED))
>>>> +        return 1;
>>>> +
>>>> +    /*
>>>> +     * The card will be considered alive unless we have been asked to detect
>>>> +     * a change or host requires polling to provide card detection.
>>>> +     */
>>>> +    if (!detect_change&&   !(host->caps&   MMC_CAP_NEEDS_POLL))
>>>> +        return 0;
>>>> +
>>>> +    ret = host->bus_ops->alive(host);
>>>> +    if (ret)
>>>> +        host->card->state |= MMC_CARD_REMOVED;
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>
>>> The patch itself looks good to me, but can't we decide this in the block
>>> layer itself when we issue get_card_status() when the ongoing request fails?
>>>
>>> ----------------------------------------------
>>>          for (retry = 2; retry>= 0; retry--) {
>>>                  err = get_card_status(card,&status, 0);
>>>                  if (!err)
>>>                          break;
>>>
>>>                  prev_cmd_status_valid = false;
>>>                  pr_err("%s: error %d sending status command, %sing\n",
>>>                         req->rq_disk->disk_name, err, retry ? "retry" :
>>> "abort");
>>>          }
>>>
>>>
>>>       /* We couldn't get a response from the card.  Give up. */
>>> -    if (err)
>>> +    if (err) {
>>> +        /*
>>> +         * If the card didn't respond to status command,
>>> +         * it is likely that card is gone. Flag it as removed,
>>> +         * mmc_detect_change() cleans the rest.
>>> +         */
>>> +        mmc_card_set_removed(card);
>>>           return ERR_ABORT;
>>> +    }
>>> ------------------------------------------------
>>>
>>> The V2 patch I have posted takes care of this. Please let me know if it not
>>> good to decide in the block layer itself. Essentially, both the
>>> implementations are sending CMD13 to know the status of the card.
>>
>> I think it is better to have the decision over whether or not the card
>> has been removed in only one place.  Also, never flagging
>> MMC_CAP_NONREMOVABLE cards as removed nor if the HC driver has not called
>> mmc_detect_change.
> 
> Agreed. This is much cleaner implementation. Thanks.
> 
>>
>> But the block change is essentially the same:
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index c80bb6d..32590c3 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -632,6 +632,8 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
>> struct request *req,
>>       u32 status, stop_status = 0;
>>       int err, retry;
>>
>> +    if (mmc_card_removed(card))
>> +        return ERR_ABORT;
>>       /*
>>        * Try to get card status which indicates both the card state
>>        * and why there was no response.  If the first attempt fails,
>> @@ -648,8 +650,10 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
>> struct request *req,
>>       }
>>
>>       /* We couldn't get a response from the card.  Give up. */
>> -    if (err)
>> +    if (err) {
>> +        mmc_detect_card_removed(card->host);
>>           return ERR_ABORT;
>> +    }
>>
>>       /* Flag ECC errors */
>>       if ((status&  R1_CARD_ECC_FAILED) ||
>>
>> I attached a revised version of the patch adding -ENOMEDIUM
>> errors from __mmc_start_request as you and Per discussed.
> 
> Thanks. I had a look at it and I have some comments:
> 
> +int mmc_detect_card_removed(struct mmc_host *host)
> +{
> +       WARN_ON(!host->claimed);
> +
> +       return _mmc_detect_card_removed(host, work_pending(&host->detect.work));
> 
> The work_pending bit would be set only when the HC driver has detected a change in the slot and called mmc_detect_change() to queue the detect work after a delay. In my case this delay is zero. So as soon as the card is removed following happen:
> 
> ->mmc_detect_change
> -->mmc_schedule_delayed_work
> --->work_pending bit is set
> ---->since the delay is zero, work is queued asap
> ----->just before work->func is called, work_pending bit is cleared
> ------>work->func() i.e., mmc_rescan is started
> ------->mmc_rescan waiting for claim host.
> 
> So there can be a case where work_pending bit is set as well as cleared even before block layer get a chance to call mmc_detect_card_removed(), hence the block layer would always see that card is alive even if the card is removed until mmc_rescan is completed.

Yes, the work_pending bit cannot be used.

> 
> +       /*
> +        * The card will be considered alive unless we have been asked to detect
> +        * a change or host requires polling to provide card detection.
> +        */
> +       if (!detect_change && !(host->caps & MMC_CAP_NEEDS_POLL))
> +               return 0;
> +
> 
> Since, the main purpose of _mmc_detect_card_removed() is to detect whether the card is removed or not I feel "detect_change" is redundant. Let me know if you see any other problem without this.
> 
> 

I would like to preserve the same rules that are used now.
i.e. the card will not get removed unless there has been
a card-detect event.

So I have replaced the work_pending bit with a flag.  See
V3 posted separately.