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

[PATCH] mmc: core: 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>
---
 drivers/mmc/card/queue.c |    5 +++++
 drivers/mmc/core/bus.c   |    2 ++
 include/linux/mmc/card.h |    3 +++
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index dcad59c..f8a3298 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 && mq->card && !mmc_card_inserted(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 46b6e84..ea3be5d 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card)
 			mmc_card_ddr_mode(card) ? "DDR " : "",
 			type, card->rca);
 	}
+	mmc_card_set_inserted(card);
 
 #ifdef CONFIG_DEBUG_FS
 	mmc_add_card_debugfs(card);
@@ -340,6 +341,7 @@ void mmc_remove_card(struct mmc_card *card)
 			pr_info("%s: card %04x removed\n",
 				mmc_hostname(card->host), card->rca);
 		}
+		card->state &= ~MMC_STATE_INSERTED;
 		device_del(&card->dev);
 	}
 
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index b7a8cb1..be4125a 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_INSERTED	(1<<7)		/* card present in 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 */
@@ -362,6 +363,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_inserted(c)	((c)->state & MMC_STATE_INSERTED)
 #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)
@@ -370,6 +372,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_inserted(c) ((c)->state |= MMC_STATE_INSERTED)
 #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] mmc: core: Kill block requests if card is removed

[Adrian Hunter]

On 09/11/11 06:31, Sujit Reddy Thumma 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>
> ---


It is safer to have zero initialisations so I suggest inverting
the meaning of the state bit i.e.

#define MMC_STATE_CARD_GONE	(1<<7)		/* card removed from the slot */


Also it would be nice is a request did not start if the card is
gone.  For the non-async case, that is easy:

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5278ffb..91d7721 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -259,7 +259,7 @@ 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_gone(host->card))
                        break;

                pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
@@ -374,6 +374,10 @@ EXPORT_SYMBOL(mmc_start_req);
  */
 void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
 {
+       if (mmc_card_gone(host->card)) {
+               mrq->cmd->error = -ENODEV;
+               return;
+       }
        __mmc_start_req(host, mrq);
        mmc_wait_for_req_done(host, mrq);
 }



The async case is harder...


>  drivers/mmc/card/queue.c |    5 +++++
>  drivers/mmc/core/bus.c   |    2 ++
>  include/linux/mmc/card.h |    3 +++
>  3 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index dcad59c..f8a3298 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 && mq->card && !mmc_card_inserted(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 46b6e84..ea3be5d 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card)
>  			mmc_card_ddr_mode(card) ? "DDR " : "",
>  			type, card->rca);
>  	}
> +	mmc_card_set_inserted(card);
>  
>  #ifdef CONFIG_DEBUG_FS
>  	mmc_add_card_debugfs(card);
> @@ -340,6 +341,7 @@ void mmc_remove_card(struct mmc_card *card)
>  			pr_info("%s: card %04x removed\n",
>  				mmc_hostname(card->host), card->rca);
>  		}
> +		card->state &= ~MMC_STATE_INSERTED;
>  		device_del(&card->dev);
>  	}
>  
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index b7a8cb1..be4125a 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_INSERTED	(1<<7)		/* card present in 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 */
> @@ -362,6 +363,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_inserted(c)	((c)->state & MMC_STATE_INSERTED)
>  #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)
> @@ -370,6 +372,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_inserted(c) ((c)->state |= MMC_STATE_INSERTED)
>  #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)

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

[Per Forlin]

Hi Adrian,

On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 09/11/11 06:31, Sujit Reddy Thumma 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>
>> ---
>
>
> It is safer to have zero initialisations so I suggest inverting
> the meaning of the state bit i.e.
>
> #define MMC_STATE_CARD_GONE     (1<<7)          /* card removed from the slot */
>
>
> Also it would be nice is a request did not start if the card is
> gone.  For the non-async case, that is easy:
>
As far as I understand Sujit's patch it will stop new requests from
being issued, blk_fetch_request(q) returns NULL.

> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5278ffb..91d7721 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -259,7 +259,7 @@ 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_gone(host->card))
host->card will be NULL
static void mmc_remove(struct mmc_host *host)
{
	BUG_ON(!host);
	BUG_ON(!host->card);

	mmc_remove_card(host->card);
	host->card = NULL;
}
card is not freed until later.

>                        break;
>
>                pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
> @@ -374,6 +374,10 @@ EXPORT_SYMBOL(mmc_start_req);
>  */
>  void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>  {
> +       if (mmc_card_gone(host->card)) {
> +               mrq->cmd->error = -ENODEV;
> +               return;
> +       }
>        __mmc_start_req(host, mrq);
>        mmc_wait_for_req_done(host, mrq);
>  }
>
>
>
> The async case is harder...
>
I can help out with the non-async code if we go for your proposal.

>
>>  drivers/mmc/card/queue.c |    5 +++++
>>  drivers/mmc/core/bus.c   |    2 ++
>>  include/linux/mmc/card.h |    3 +++
>>  3 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>> index dcad59c..f8a3298 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 && mq->card && !mmc_card_inserted(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 46b6e84..ea3be5d 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card)
>>                       mmc_card_ddr_mode(card) ? "DDR " : "",
>>                       type, card->rca);
>>       }
>> +     mmc_card_set_inserted(card);
>>
>>  #ifdef CONFIG_DEBUG_FS
>>       mmc_add_card_debugfs(card);
>> @@ -340,6 +341,7 @@ void mmc_remove_card(struct mmc_card *card)
>>                       pr_info("%s: card %04x removed\n",
>>                               mmc_hostname(card->host), card->rca);
>>               }
>> +             card->state &= ~MMC_STATE_INSERTED;
>>               device_del(&card->dev);
>>       }
>>
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index b7a8cb1..be4125a 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_INSERTED   (1<<7)          /* card present in 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 */
>> @@ -362,6 +363,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_inserted(c) ((c)->state & MMC_STATE_INSERTED)
>>  #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)
>> @@ -370,6 +372,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_inserted(c) ((c)->state |= MMC_STATE_INSERTED)
>>  #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)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Per Forlin]

Hi Adrian,

>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 5278ffb..91d7721 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -259,7 +259,7 @@ 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_gone(host->card))
> host->card will be NULL
> static void mmc_remove(struct mmc_host *host)
> {
>        BUG_ON(!host);
>        BUG_ON(!host->card);
>
>        mmc_remove_card(host->card);
>        host->card = NULL;
> }
> card is not freed until later.
Please ignore this part. I jumped to conclusions. I had another look
and there can't be any incoming requests when host->card is NULL.
I need to study device_del() further, in order to understand the details.

Regards,
Per

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

[Sujit Reddy Thumma]

On 11/10/2011 3:35 AM, Per Forlin wrote:
> Hi Adrian,
>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 5278ffb..91d7721 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -259,7 +259,7 @@ 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_gone(host->card))
>> host->card will be NULL
>> static void mmc_remove(struct mmc_host *host)
>> {
>>         BUG_ON(!host);
>>         BUG_ON(!host->card);
>>
>>         mmc_remove_card(host->card);
>>         host->card = NULL;
>> }
>> card is not freed until later.
> Please ignore this part. I jumped to conclusions. I had another look
> and there can't be any incoming requests when host->card is NULL.
> I need to study device_del() further, in order to understand the details.

There can be incoming requests when the host->card is NULL. This happens 
when we are detecting the card for the first time. That is, in 
mmc_rescan() we send all the initialization commands with host->card 
being NULL.

We can do something like this:
#define mmc_card_gone(c) (c && ((c)->state & MMC_STATE_CARD_GONE))


>
> Regards,
> Per


-- 
Thanks & Regards,
Sujit Reddy Thumma

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

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

[Sujit Reddy Thumma]

Hi,
> Hi Adrian,
>
> On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>> On 09/11/11 06:31, Sujit Reddy Thumma 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>
>>> ---
>>
>>
>> It is safer to have zero initialisations so I suggest inverting
>> the meaning of the state bit i.e.

Makes sense. Will take care in V2.

>>
>> #define MMC_STATE_CARD_GONE     (1<<7)          /* card removed from the slot */
>>
>>
>> Also it would be nice is a request did not start if the card is
>> gone.  For the non-async case, that is easy:
>>
> As far as I understand Sujit's patch it will stop new requests from
> being issued, blk_fetch_request(q) returns NULL.

Yes, Per you are right. The ongoing requests will fail at the controller 
driver layer and only the new requests coming to MMC queue layer will be 
blocked.

Adrian's suggestion is to stop all the requests reaching host controller 
layer by mmc core. This seems to be a good approach but the problem here 
is the host driver should inform mmc core that card is gone.

Adrian, do you agree if we need to change all the host drivers to set 
the MMC_STATE_CARD_GONE flag as soon as the card detect irq handler 
detects the card as removed?

>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 5278ffb..91d7721 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -259,7 +259,7 @@ 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_gone(host->card))
> host->card will be NULL

Yes, the host->card will be NULL for some requests. Will take care of it.

> static void mmc_remove(struct mmc_host *host)
> {
> 	BUG_ON(!host);
> 	BUG_ON(!host->card);
>
> 	mmc_remove_card(host->card);
> 	host->card = NULL;
> }
> card is not freed until later.
>
>>                         break;
>>
>>                 pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
>> @@ -374,6 +374,10 @@ EXPORT_SYMBOL(mmc_start_req);
>>   */
>>   void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>>   {
>> +       if (mmc_card_gone(host->card)) {
>> +               mrq->cmd->error = -ENODEV;
>> +               return;
>> +       }

Looks good. Just for clarification, some host controller drivers return 
-ENOMEDIUM in host->ops->request() when they see the card is gone. Do 
you think we can remove this now from host controller drivers?


>>         __mmc_start_req(host, mrq);
>>         mmc_wait_for_req_done(host, mrq);
>>   }
>>
>>
>>
>> The async case is harder...
>>
> I can help out with the non-async code if we go for your proposal.

I will check the possibilities.

>
>>
>>>   drivers/mmc/card/queue.c |    5 +++++
>>>   drivers/mmc/core/bus.c   |    2 ++
>>>   include/linux/mmc/card.h |    3 +++
>>>   3 files changed, 10 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>>> index dcad59c..f8a3298 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&&  mq->card&&  !mmc_card_inserted(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 46b6e84..ea3be5d 100644
>>> --- a/drivers/mmc/core/bus.c
>>> +++ b/drivers/mmc/core/bus.c
>>> @@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card)
>>>                        mmc_card_ddr_mode(card) ? "DDR " : "",
>>>                        type, card->rca);
>>>        }
>>> +     mmc_card_set_inserted(card);
>>>
>>>   #ifdef CONFIG_DEBUG_FS
>>>        mmc_add_card_debugfs(card);
>>> @@ -340,6 +341,7 @@ void mmc_remove_card(struct mmc_card *card)
>>>                        pr_info("%s: card %04x removed\n",
>>>                                mmc_hostname(card->host), card->rca);
>>>                }
>>> +             card->state&= ~MMC_STATE_INSERTED;
>>>                device_del(&card->dev);
>>>        }
>>>
>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>>> index b7a8cb1..be4125a 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_INSERTED   (1<<7)          /* card present in 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 */
>>> @@ -362,6 +363,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_inserted(c) ((c)->state&  MMC_STATE_INSERTED)
>>>   #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)
>>> @@ -370,6 +372,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_inserted(c) ((c)->state |= MMC_STATE_INSERTED)
>>>   #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)
>>


-- 
Thanks, Sujit

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

[Adrian Hunter]

On 10/11/11 06:02, Sujit Reddy Thumma wrote:
> Hi,
>> Hi Adrian,
>>
>> On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@intel.com> 
>> wrote:
>>> On 09/11/11 06:31, Sujit Reddy Thumma 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>
>>>> ---
>>>
>>>
>>> It is safer to have zero initialisations so I suggest inverting
>>> the meaning of the state bit i.e.
> 
> Makes sense. Will take care in V2.
> 
>>>
>>> #define MMC_STATE_CARD_GONE     (1<<7)          /* card removed from the
>>> slot */
>>>
>>>
>>> Also it would be nice is a request did not start if the card is
>>> gone.  For the non-async case, that is easy:
>>>
>> As far as I understand Sujit's patch it will stop new requests from
>> being issued, blk_fetch_request(q) returns NULL.
> 
> Yes, Per you are right. The ongoing requests will fail at the controller
> driver layer and only the new requests coming to MMC queue layer will be
> blocked.
> 
> Adrian's suggestion is to stop all the requests reaching host controller
> layer by mmc core. This seems to be a good approach but the problem here is
> the host driver should inform mmc core that card is gone.
> 
> Adrian, do you agree if we need to change all the host drivers to set the
> MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects the
> card as removed?

Typically a card detect interrupt queues a rescan which in turn will have
to wait to claim the host.  In the meantime, in the async case, the block
driver will not release the host until the queue is empty.  The block
driver will see errors and will use a send status command which will fail
so the request will be aborted, but the next request will be started
anyway.

There are two problems:

1. When do we reliably know that the card has really gone?

At present, the detect method for SD and MMC is just the send status
command, which is what the block driver is doing i.e. if the block
driver sees send status fail, after an errored request, and the
card is removable, then it is very likely the card has gone.

At present, it is not considered that the host controller driver
knows whether the card is really gone - just that it might be.

Setting a MMC_STATE_CARD_GONE flag early may be a little complicated.
e.g. mmc_detect_change() flags that the card may be gone.  After an
error, if the "card may be gone" flag is set a new bus method could
be called that just does send status.  If that fails, then the
MMC_STATE_CARD_GONE flag is set.  Any calls to the detect method
must first check the MMC_STATE_CARD_GONE flag so that, once gone,
a card can not come back.

Maybe you can think of something simpler.

2. How can new requests be prevented from starting once the card
is gone?

Using BLKPREP_KILL will do that for all but the request that has been
prepared already (i.e. async case) if the MMC_STATE_CARD_GONE flag is set
early enough.

Maybe blocking requests at a lower level is not needed.


> 
>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 5278ffb..91d7721 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -259,7 +259,7 @@ 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_gone(host->card))
>> host->card will be NULL
> 
> Yes, the host->card will be NULL for some requests. Will take care of it.
> 
>> static void mmc_remove(struct mmc_host *host)
>> {
>>     BUG_ON(!host);
>>     BUG_ON(!host->card);
>>
>>     mmc_remove_card(host->card);
>>     host->card = NULL;
>> }
>> card is not freed until later.
>>
>>>                         break;
>>>
>>>                 pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
>>> @@ -374,6 +374,10 @@ EXPORT_SYMBOL(mmc_start_req);
>>>   */
>>>   void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>>>   {
>>> +       if (mmc_card_gone(host->card)) {
>>> +               mrq->cmd->error = -ENODEV;
>>> +               return;
>>> +       }
> 
> Looks good. Just for clarification, some host controller drivers return
> -ENOMEDIUM in host->ops->request() when they see the card is gone. Do you
> think we can remove this now from host controller drivers?

I see include/linux/mmc/core.h has specified the use of -ENOMEDIUM so that
should be used.

> 
> 
>>>         __mmc_start_req(host, mrq);
>>>         mmc_wait_for_req_done(host, mrq);
>>>   }
>>>
>>>
>>>
>>> The async case is harder...
>>>
>> I can help out with the non-async code if we go for your proposal.
> 
> I will check the possibilities.
> 
>>
>>>
>>>>   drivers/mmc/card/queue.c |    5 +++++
>>>>   drivers/mmc/core/bus.c   |    2 ++
>>>>   include/linux/mmc/card.h |    3 +++
>>>>   3 files changed, 10 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>>>> index dcad59c..f8a3298 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&&  mq->card&&  !mmc_card_inserted(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 46b6e84..ea3be5d 100644
>>>> --- a/drivers/mmc/core/bus.c
>>>> +++ b/drivers/mmc/core/bus.c
>>>> @@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card)
>>>>                        mmc_card_ddr_mode(card) ? "DDR " : "",
>>>>                        type, card->rca);
>>>>        }
>>>> +     mmc_card_set_inserted(card);
>>>>
>>>>   #ifdef CONFIG_DEBUG_FS
>>>>        mmc_add_card_debugfs(card);
>>>> @@ -340,6 +341,7 @@ void mmc_remove_card(struct mmc_card *card)
>>>>                        pr_info("%s: card %04x removed\n",
>>>>                                mmc_hostname(card->host), card->rca);
>>>>                }
>>>> +             card->state&= ~MMC_STATE_INSERTED;
>>>>                device_del(&card->dev);
>>>>        }
>>>>
>>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>>>> index b7a8cb1..be4125a 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_INSERTED   (1<<7)          /* card present in 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 */
>>>> @@ -362,6 +363,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_inserted(c) ((c)->state&  MMC_STATE_INSERTED)
>>>>   #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)
>>>> @@ -370,6 +372,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_inserted(c) ((c)->state |= MMC_STATE_INSERTED)
>>>>   #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)
>>>
> 
> 

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

[Per Forlin]

On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 10/11/11 06:02, Sujit Reddy Thumma wrote:
>> Hi,
>>> Hi Adrian,
>>>
>>> On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@intel.com>
>>> wrote:
>>>> On 09/11/11 06:31, Sujit Reddy Thumma 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>
>>>>> ---
>>>>
>>>>
>>>> It is safer to have zero initialisations so I suggest inverting
>>>> the meaning of the state bit i.e.
>>
>> Makes sense. Will take care in V2.
>>
>>>>
>>>> #define MMC_STATE_CARD_GONE     (1<<7)          /* card removed from the
>>>> slot */
>>>>
>>>>
>>>> Also it would be nice is a request did not start if the card is
>>>> gone.  For the non-async case, that is easy:
>>>>
>>> As far as I understand Sujit's patch it will stop new requests from
>>> being issued, blk_fetch_request(q) returns NULL.
>>
>> Yes, Per you are right. The ongoing requests will fail at the controller
>> driver layer and only the new requests coming to MMC queue layer will be
>> blocked.
>>
>> Adrian's suggestion is to stop all the requests reaching host controller
>> layer by mmc core. This seems to be a good approach but the problem here is
>> the host driver should inform mmc core that card is gone.
>>
>> Adrian, do you agree if we need to change all the host drivers to set the
>> MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects the
>> card as removed?
>
> Typically a card detect interrupt queues a rescan which in turn will have
> to wait to claim the host.  In the meantime, in the async case, the block
> driver will not release the host until the queue is empty.
Here is a patch that let async-mmc release host and reclaim it in case of error.
When the host is released mmc_rescan will claim the host and run.
--------------------------------
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index cf73877..8952e63 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
*md, struct mmc_card *card,
 	return ret;
 }

+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
+						   struct mmc_queue *mq,
+						   struct mmc_queue_req *mqrq,
+						   int disable_multi,
+						   struct mmc_async_req *areq)
+{
+	/*
+	 * Release host after failure in case the host is needed
+	 * by someone else. For instance, if the card is removed the
+	 * worker thread needs to claim the host in order to do mmc_rescan.
+	 */
+	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);
+}
+
 static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
 {
 	struct mmc_blk_data *md = mq->data;
@@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct
mmc_queue *mq, struct request *rqc)
 			break;
 		}

-		if (ret) {
+		if (ret)
 			/*
 			 * In case of a incomplete request
 			 * prepare it again and resend.
 			 */
-			mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, mq);
-			mmc_start_req(card->host, &mq_rq->mmc_active, NULL);
-		}
+			mmc_blk_prep_start(card, mq, mq_rq, disable_multi,
+					   &mq_rq->mmc_active);
+
 	} while (ret);

 	return 1;
@@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
*mq, struct request *rqc)
 	spin_unlock_irq(&md->lock);

  start_new_req:
-	if (rqc) {
-		mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
-		mmc_start_req(card->host, &mq->mqrq_cur->mmc_active, NULL);
-	}
+	if (rqc)
+		mmc_blk_prep_start(card, mq, mq->mqrq_cur, 0,
+				   &mq->mqrq_cur->mmc_active);

 	return 0;
 }
-------------------------------------


> The block
> driver will see errors and will use a send status command which will fail
> so the request will be aborted, but the next request will be started
> anyway.
>
> There are two problems:
>
> 1. When do we reliably know that the card has really gone?
>
> At present, the detect method for SD and MMC is just the send status
> command, which is what the block driver is doing i.e. if the block
> driver sees send status fail, after an errored request, and the
> card is removable, then it is very likely the card has gone.
>
> At present, it is not considered that the host controller driver
> knows whether the card is really gone - just that it might be.
>
> Setting a MMC_STATE_CARD_GONE flag early may be a little complicated.
> e.g. mmc_detect_change() flags that the card may be gone.  After an
> error, if the "card may be gone" flag is set a new bus method could
> be called that just does send status.  If that fails, then the
> MMC_STATE_CARD_GONE flag is set.  Any calls to the detect method
> must first check the MMC_STATE_CARD_GONE flag so that, once gone,
> a card can not come back.
>
> Maybe you can think of something simpler.
>
> 2. How can new requests be prevented from starting once the card
> is gone?
>
> Using BLKPREP_KILL will do that for all but the request that has been
> prepared already (i.e. async case) if the MMC_STATE_CARD_GONE flag is set
> early enough.
>
> Maybe blocking requests at a lower level is not needed.
>
>
>>
>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index 5278ffb..91d7721 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -259,7 +259,7 @@ 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_gone(host->card))
>>> host->card will be NULL
>>
>> Yes, the host->card will be NULL for some requests. Will take care of it.
>>
>>> static void mmc_remove(struct mmc_host *host)
>>> {
>>>     BUG_ON(!host);
>>>     BUG_ON(!host->card);
>>>
>>>     mmc_remove_card(host->card);
>>>     host->card = NULL;
>>> }
>>> card is not freed until later.
>>>
>>>>                         break;
>>>>
>>>>                 pr_debug("%s: req failed (CMD%u): %d, retrying...\n",
>>>> @@ -374,6 +374,10 @@ EXPORT_SYMBOL(mmc_start_req);
>>>>   */
>>>>   void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>>>>   {
>>>> +       if (mmc_card_gone(host->card)) {
>>>> +               mrq->cmd->error = -ENODEV;
>>>> +               return;
>>>> +       }
>>
>> Looks good. Just for clarification, some host controller drivers return
>> -ENOMEDIUM in host->ops->request() when they see the card is gone. Do you
>> think we can remove this now from host controller drivers?
>
> I see include/linux/mmc/core.h has specified the use of -ENOMEDIUM so that
> should be used.
>
>>
>>
>>>>         __mmc_start_req(host, mrq);
>>>>         mmc_wait_for_req_done(host, mrq);
>>>>   }
>>>>
>>>>
>>>>
>>>> The async case is harder...
>>>>
>>> I can help out with the non-async code if we go for your proposal.
>>
>> I will check the possibilities.
>>
>>>
>>>>
>>>>>   drivers/mmc/card/queue.c |    5 +++++
>>>>>   drivers/mmc/core/bus.c   |    2 ++
>>>>>   include/linux/mmc/card.h |    3 +++
>>>>>   3 files changed, 10 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>>>>> index dcad59c..f8a3298 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&&  mq->card&&  !mmc_card_inserted(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 46b6e84..ea3be5d 100644
>>>>> --- a/drivers/mmc/core/bus.c
>>>>> +++ b/drivers/mmc/core/bus.c
>>>>> @@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card)
>>>>>                        mmc_card_ddr_mode(card) ? "DDR " : "",
>>>>>                        type, card->rca);
>>>>>        }
>>>>> +     mmc_card_set_inserted(card);
>>>>>
>>>>>   #ifdef CONFIG_DEBUG_FS
>>>>>        mmc_add_card_debugfs(card);
>>>>> @@ -340,6 +341,7 @@ void mmc_remove_card(struct mmc_card *card)
>>>>>                        pr_info("%s: card %04x removed\n",
>>>>>                                mmc_hostname(card->host), card->rca);
>>>>>                }
>>>>> +             card->state&= ~MMC_STATE_INSERTED;
>>>>>                device_del(&card->dev);
>>>>>        }
>>>>>
>>>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>>>>> index b7a8cb1..be4125a 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_INSERTED   (1<<7)          /* card present in 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 */
>>>>> @@ -362,6 +363,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_inserted(c) ((c)->state&  MMC_STATE_INSERTED)
>>>>>   #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)
>>>>> @@ -370,6 +372,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_inserted(c) ((c)->state |= MMC_STATE_INSERTED)
>>>>>   #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)
>>>>
>>
>>
>
>

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

[Sujit Reddy Thumma]

On 11/10/2011 7:50 PM, Per Forlin wrote:
> On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter<adrian.hunter@intel.com>  wrote:
>> On 10/11/11 06:02, Sujit Reddy Thumma wrote:
>>> Hi,
>>>> Hi Adrian,
>>>>
>>>> On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@intel.com>
>>>> wrote:
>>>>> On 09/11/11 06:31, Sujit Reddy Thumma 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>
>>>>>> ---
>>>>>
>>>>>
>>>>> It is safer to have zero initialisations so I suggest inverting
>>>>> the meaning of the state bit i.e.
>>>
>>> Makes sense. Will take care in V2.
>>>
>>>>>
>>>>> #define MMC_STATE_CARD_GONE     (1<<7)          /* card removed from the
>>>>> slot */
>>>>>
>>>>>
>>>>> Also it would be nice is a request did not start if the card is
>>>>> gone.  For the non-async case, that is easy:
>>>>>
>>>> As far as I understand Sujit's patch it will stop new requests from
>>>> being issued, blk_fetch_request(q) returns NULL.
>>>
>>> Yes, Per you are right. The ongoing requests will fail at the controller
>>> driver layer and only the new requests coming to MMC queue layer will be
>>> blocked.
>>>
>>> Adrian's suggestion is to stop all the requests reaching host controller
>>> layer by mmc core. This seems to be a good approach but the problem here is
>>> the host driver should inform mmc core that card is gone.
>>>
>>> Adrian, do you agree if we need to change all the host drivers to set the
>>> MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects the
>>> card as removed?
>>
>> Typically a card detect interrupt queues a rescan which in turn will have
>> to wait to claim the host.  In the meantime, in the async case, the block
>> driver will not release the host until the queue is empty.
> Here is a patch that let async-mmc release host and reclaim it in case of error.
> When the host is released mmc_rescan will claim the host and run.
> --------------------------------
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index cf73877..8952e63 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
> *md, struct mmc_card *card,
>   	return ret;
>   }
>
> +/*
> + * This function should be called to resend a request after failure.
> + * Prepares and starts the request.
> + */
> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
> +						   struct mmc_queue *mq,
> +						   struct mmc_queue_req *mqrq,
> +						   int disable_multi,
> +						   struct mmc_async_req *areq)
> +{
> +	/*
> +	 * Release host after failure in case the host is needed
> +	 * by someone else. For instance, if the card is removed the
> +	 * worker thread needs to claim the host in order to do mmc_rescan.
> +	 */
> +	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);
> +}
> +
>   static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>   {
>   	struct mmc_blk_data *md = mq->data;
> @@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct
> mmc_queue *mq, struct request *rqc)
>   			break;
>   		}
>
> -		if (ret) {
> +		if (ret)
>   			/*
>   			 * In case of a incomplete request
>   			 * prepare it again and resend.
>   			 */
> -			mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, mq);
> -			mmc_start_req(card->host,&mq_rq->mmc_active, NULL);
> -		}
> +			mmc_blk_prep_start(card, mq, mq_rq, disable_multi,
> +					&mq_rq->mmc_active);
> +

:%s/mmc_blk_prep_start/mmc_blk_resend

>   	} while (ret);
>
>   	return 1;
> @@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
> *mq, struct request *rqc)
>   	spin_unlock_irq(&md->lock);
>
>    start_new_req:
> -	if (rqc) {
> -		mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> -		mmc_start_req(card->host,&mq->mqrq_cur->mmc_active, NULL);
> -	}
> +	if (rqc)
> +		mmc_blk_prep_start(card, mq, mq->mqrq_cur, 0,
> +				&mq->mqrq_cur->mmc_active);
>
>   	return 0;
>   }

Thanks Per. This looks good. Can we split this into a different patch?

> -------------------------------------
>
>
>>   The block
>> driver will see errors and will use a send status command which will fail
>> so the request will be aborted, but the next request will be started
>> anyway.
>>
>> There are two problems:
>>
>> 1. When do we reliably know that the card has really gone?
>>
>> At present, the detect method for SD and MMC is just the send status
>> command, which is what the block driver is doing i.e. if the block
>> driver sees send status fail, after an errored request, and the
>> card is removable, then it is very likely the card has gone.
>>
>> At present, it is not considered that the host controller driver
>> knows whether the card is really gone - just that it might be.
>>
>> Setting a MMC_STATE_CARD_GONE flag early may be a little complicated.
>> e.g. mmc_detect_change() flags that the card may be gone.  After an
>> error, if the "card may be gone" flag is set a new bus method could
>> be called that just does send status.  If that fails, then the
>> MMC_STATE_CARD_GONE flag is set.  Any calls to the detect method
>> must first check the MMC_STATE_CARD_GONE flag so that, once gone,
>> a card can not come back.
>>
>> Maybe you can think of something simpler.

Can we do something like this:

--- 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_card_gone(card);
                 return ERR_ABORT;
+       }

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

and some additions to Per's patch, changes denoted in (++):

+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
+						   struct mmc_queue *mq,
+						   struct mmc_queue_req *mqrq,
+						   int disable_multi,
+						   struct mmc_async_req *areq)
+{
+	/*
+	 * Release host after failure in case the host is needed
+	 * by someone else. For instance, if the card is removed the
+	 * worker thread needs to claim the host in order to do mmc_rescan.
+	 */
+	mmc_release_host(card->host);
+	mmc_claim_host(card->host);
++	if (mmc_card_gone(card)) {
++	/*
++	 * End the pending async request without starting it when card
++	 * is removed.
++	 */
++		req->cmd_flags |= REQ_QUIET;
++		__blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
+	} else {
+		mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+		return mmc_start_req(card->host, areq, NULL);
+	}
+}
+


>>
>> 2. How can new requests be prevented from starting once the card
>> is gone?
>>
>> Using BLKPREP_KILL will do that for all but the request that has been
>> prepared already (i.e. async case) if the MMC_STATE_CARD_GONE flag is set
>> early enough.
>>
>> Maybe blocking requests at a lower level is not needed.
>>
>>
>>>
>>>>

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

[Per Forlin]

On Mon, Nov 14, 2011 at 5:19 AM, Sujit Reddy Thumma
<sthumma@codeaurora.org> wrote:
> On 11/10/2011 7:50 PM, Per Forlin wrote:
>>
>> On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter<adrian.hunter@intel.com>
>>  wrote:
>>>
>>> On 10/11/11 06:02, Sujit Reddy Thumma wrote:
>>>>
>>>> Hi,
>>>>>
>>>>> Hi Adrian,
>>>>>
>>>>> On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@intel.com>
>>>>> wrote:
>>>>>>
>>>>>> On 09/11/11 06:31, Sujit Reddy Thumma 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>
>>>>>>> ---
>>>>>>
>>>>>>
>>>>>> It is safer to have zero initialisations so I suggest inverting
>>>>>> the meaning of the state bit i.e.
>>>>
>>>> Makes sense. Will take care in V2.
>>>>
>>>>>>
>>>>>> #define MMC_STATE_CARD_GONE     (1<<7)          /* card removed from
>>>>>> the
>>>>>> slot */
>>>>>>
>>>>>>
>>>>>> Also it would be nice is a request did not start if the card is
>>>>>> gone.  For the non-async case, that is easy:
>>>>>>
>>>>> As far as I understand Sujit's patch it will stop new requests from
>>>>> being issued, blk_fetch_request(q) returns NULL.
>>>>
>>>> Yes, Per you are right. The ongoing requests will fail at the controller
>>>> driver layer and only the new requests coming to MMC queue layer will be
>>>> blocked.
>>>>
>>>> Adrian's suggestion is to stop all the requests reaching host controller
>>>> layer by mmc core. This seems to be a good approach but the problem here
>>>> is
>>>> the host driver should inform mmc core that card is gone.
>>>>
>>>> Adrian, do you agree if we need to change all the host drivers to set
>>>> the
>>>> MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects
>>>> the
>>>> card as removed?
>>>
>>> Typically a card detect interrupt queues a rescan which in turn will have
>>> to wait to claim the host.  In the meantime, in the async case, the block
>>> driver will not release the host until the queue is empty.
>>
>> Here is a patch that let async-mmc release host and reclaim it in case of
>> error.
>> When the host is released mmc_rescan will claim the host and run.
>> --------------------------------
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index cf73877..8952e63 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
>> *md, struct mmc_card *card,
>>        return ret;
>>  }
>>
>> +/*
>> + * This function should be called to resend a request after failure.
>> + * Prepares and starts the request.
>> + */
>> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>> +                                                  struct mmc_queue *mq,
>> +                                                  struct mmc_queue_req
>> *mqrq,
>> +                                                  int disable_multi,
>> +                                                  struct mmc_async_req
>> *areq)
>> +{
>> +       /*
>> +        * Release host after failure in case the host is needed
>> +        * by someone else. For instance, if the card is removed the
>> +        * worker thread needs to claim the host in order to do
>> mmc_rescan.
>> +        */
>> +       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);
>> +}
>> +
>>  static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>>  {
>>        struct mmc_blk_data *md = mq->data;
>> @@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct
>> mmc_queue *mq, struct request *rqc)
>>                        break;
>>                }
>>
>> -               if (ret) {
>> +               if (ret)
>>                        /*
>>                         * In case of a incomplete request
>>                         * prepare it again and resend.
>>                         */
>> -                       mmc_blk_rw_rq_prep(mq_rq, card, disable_multi,
>> mq);
>> -                       mmc_start_req(card->host,&mq_rq->mmc_active,
>> NULL);
>> -               }
>> +                       mmc_blk_prep_start(card, mq, mq_rq, disable_multi,
>> +                                       &mq_rq->mmc_active);
>> +
>
> :%s/mmc_blk_prep_start/mmc_blk_resend
>
I'll update.

>>        } while (ret);
>>
>>        return 1;
>> @@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
>> *mq, struct request *rqc)
>>        spin_unlock_irq(&md->lock);
>>
>>   start_new_req:
>> -       if (rqc) {
>> -               mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>> -               mmc_start_req(card->host,&mq->mqrq_cur->mmc_active, NULL);
>> -       }
>> +       if (rqc)
>> +               mmc_blk_prep_start(card, mq, mq->mqrq_cur, 0,
>> +                               &mq->mqrq_cur->mmc_active);
>>
>>        return 0;
>>  }
>
> Thanks Per. This looks good. Can we split this into a different patch?
>
Yes I agree. My intention was to treat this as a separate patch.
I'll post it.

>> -------------------------------------
>>
>>
>>>  The block
>>> driver will see errors and will use a send status command which will fail
>>> so the request will be aborted, but the next request will be started
>>> anyway.
>>>
>>> There are two problems:
>>>
>>> 1. When do we reliably know that the card has really gone?
>>>
>>> At present, the detect method for SD and MMC is just the send status
>>> command, which is what the block driver is doing i.e. if the block
>>> driver sees send status fail, after an errored request, and the
>>> card is removable, then it is very likely the card has gone.
>>>
>>> At present, it is not considered that the host controller driver
>>> knows whether the card is really gone - just that it might be.
>>>
>>> Setting a MMC_STATE_CARD_GONE flag early may be a little complicated.
>>> e.g. mmc_detect_change() flags that the card may be gone.  After an
>>> error, if the "card may be gone" flag is set a new bus method could
>>> be called that just does send status.  If that fails, then the
>>> MMC_STATE_CARD_GONE flag is set.  Any calls to the detect method
>>> must first check the MMC_STATE_CARD_GONE flag so that, once gone,
>>> a card can not come back.
>>>
>>> Maybe you can think of something simpler.
>
> Can we do something like this:
>
> --- 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_card_gone(card);
>                return ERR_ABORT;
> +       }
>
>        /* Flag ECC errors */
>        if ((status & R1_CARD_ECC_FAILED) ||
>
> and some additions to Per's patch, changes denoted in (++):
>
> +/*
> + * This function should be called to resend a request after failure.
> + * Prepares and starts the request.
> + */
> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
> +                                                  struct mmc_queue *mq,
> +                                                  struct mmc_queue_req
> *mqrq,
> +                                                  int disable_multi,
> +                                                  struct mmc_async_req
> *areq)
> +{
> +       /*
> +        * Release host after failure in case the host is needed
> +        * by someone else. For instance, if the card is removed the
> +        * worker thread needs to claim the host in order to do mmc_rescan.
> +        */
> +       mmc_release_host(card->host);
> +       mmc_claim_host(card->host);
> ++      if (mmc_card_gone(card)) {
> ++      /*
> ++       * End the pending async request without starting it when card
> ++       * is removed.
> ++       */
> ++              req->cmd_flags |= REQ_QUIET;
> ++              __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
I'll add this to the patch and send it out.


Thanks,
Per

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

[Per Forlin]

On Mon, Nov 14, 2011 at 8:52 AM, Per Forlin <per.lkml@gmail.com> wrote:
> On Mon, Nov 14, 2011 at 5:19 AM, Sujit Reddy Thumma
> <sthumma@codeaurora.org> wrote:
>> On 11/10/2011 7:50 PM, Per Forlin wrote:
>>>
>>> On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter<adrian.hunter@intel.com>
>>>  wrote:
>>>>
>>>> On 10/11/11 06:02, Sujit Reddy Thumma wrote:
>>>>>
>>>>> Hi,
>>>>>>
>>>>>> Hi Adrian,
>>>>>>
>>>>>> On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@intel.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 09/11/11 06:31, Sujit Reddy Thumma 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>
>>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>> It is safer to have zero initialisations so I suggest inverting
>>>>>>> the meaning of the state bit i.e.
>>>>>
>>>>> Makes sense. Will take care in V2.
>>>>>
>>>>>>>
>>>>>>> #define MMC_STATE_CARD_GONE     (1<<7)          /* card removed from
>>>>>>> the
>>>>>>> slot */
>>>>>>>
>>>>>>>
>>>>>>> Also it would be nice is a request did not start if the card is
>>>>>>> gone.  For the non-async case, that is easy:
>>>>>>>
>>>>>> As far as I understand Sujit's patch it will stop new requests from
>>>>>> being issued, blk_fetch_request(q) returns NULL.
>>>>>
>>>>> Yes, Per you are right. The ongoing requests will fail at the controller
>>>>> driver layer and only the new requests coming to MMC queue layer will be
>>>>> blocked.
>>>>>
>>>>> Adrian's suggestion is to stop all the requests reaching host controller
>>>>> layer by mmc core. This seems to be a good approach but the problem here
>>>>> is
>>>>> the host driver should inform mmc core that card is gone.
>>>>>
>>>>> Adrian, do you agree if we need to change all the host drivers to set
>>>>> the
>>>>> MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects
>>>>> the
>>>>> card as removed?
>>>>
>>>> Typically a card detect interrupt queues a rescan which in turn will have
>>>> to wait to claim the host.  In the meantime, in the async case, the block
>>>> driver will not release the host until the queue is empty.
>>>
>>> Here is a patch that let async-mmc release host and reclaim it in case of
>>> error.
>>> When the host is released mmc_rescan will claim the host and run.
>>> --------------------------------
>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>> index cf73877..8952e63 100644
>>> --- a/drivers/mmc/card/block.c
>>> +++ b/drivers/mmc/card/block.c
>>> @@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
>>> *md, struct mmc_card *card,
>>>        return ret;
>>>  }
>>>
>>> +/*
>>> + * This function should be called to resend a request after failure.
>>> + * Prepares and starts the request.
>>> + */
>>> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>>> +                                                  struct mmc_queue *mq,
>>> +                                                  struct mmc_queue_req
>>> *mqrq,
>>> +                                                  int disable_multi,
>>> +                                                  struct mmc_async_req
>>> *areq)
>>> +{
>>> +       /*
>>> +        * Release host after failure in case the host is needed
>>> +        * by someone else. For instance, if the card is removed the
>>> +        * worker thread needs to claim the host in order to do
>>> mmc_rescan.
>>> +        */
>>> +       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);
>>> +}
>>> +
>>>  static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>>>  {
>>>        struct mmc_blk_data *md = mq->data;
>>> @@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct
>>> mmc_queue *mq, struct request *rqc)
>>>                        break;
>>>                }
>>>
>>> -               if (ret) {
>>> +               if (ret)
>>>                        /*
>>>                         * In case of a incomplete request
>>>                         * prepare it again and resend.
>>>                         */
>>> -                       mmc_blk_rw_rq_prep(mq_rq, card, disable_multi,
>>> mq);
>>> -                       mmc_start_req(card->host,&mq_rq->mmc_active,
>>> NULL);
>>> -               }
>>> +                       mmc_blk_prep_start(card, mq, mq_rq, disable_multi,
>>> +                                       &mq_rq->mmc_active);
>>> +
>>
>> :%s/mmc_blk_prep_start/mmc_blk_resend
>>
> I'll update.
>
>>>        } while (ret);
>>>
>>>        return 1;
>>> @@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
>>> *mq, struct request *rqc)
>>>        spin_unlock_irq(&md->lock);
>>>
>>>   start_new_req:
>>> -       if (rqc) {
>>> -               mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>>> -               mmc_start_req(card->host,&mq->mqrq_cur->mmc_active, NULL);
>>> -       }
>>> +       if (rqc)
>>> +               mmc_blk_prep_start(card, mq, mq->mqrq_cur, 0,
>>> +                               &mq->mqrq_cur->mmc_active);
>>>
>>>        return 0;
>>>  }
>>
>> Thanks Per. This looks good. Can we split this into a different patch?
>>
> Yes I agree. My intention was to treat this as a separate patch.
> I'll post it.
>
>>> -------------------------------------
>>>
>>>
>>>>  The block
>>>> driver will see errors and will use a send status command which will fail
>>>> so the request will be aborted, but the next request will be started
>>>> anyway.
>>>>
>>>> There are two problems:
>>>>
>>>> 1. When do we reliably know that the card has really gone?
>>>>
>>>> At present, the detect method for SD and MMC is just the send status
>>>> command, which is what the block driver is doing i.e. if the block
>>>> driver sees send status fail, after an errored request, and the
>>>> card is removable, then it is very likely the card has gone.
>>>>
>>>> At present, it is not considered that the host controller driver
>>>> knows whether the card is really gone - just that it might be.
>>>>
>>>> Setting a MMC_STATE_CARD_GONE flag early may be a little complicated.
>>>> e.g. mmc_detect_change() flags that the card may be gone.  After an
>>>> error, if the "card may be gone" flag is set a new bus method could
>>>> be called that just does send status.  If that fails, then the
>>>> MMC_STATE_CARD_GONE flag is set.  Any calls to the detect method
>>>> must first check the MMC_STATE_CARD_GONE flag so that, once gone,
>>>> a card can not come back.
>>>>
>>>> Maybe you can think of something simpler.
>>
>> Can we do something like this:
>>
>> --- 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_card_gone(card);
>>                return ERR_ABORT;
>> +       }
>>
>>        /* Flag ECC errors */
>>        if ((status & R1_CARD_ECC_FAILED) ||
>>
>> and some additions to Per's patch, changes denoted in (++):
>>
>> +/*
>> + * This function should be called to resend a request after failure.
>> + * Prepares and starts the request.
>> + */
>> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>> +                                                  struct mmc_queue *mq,
>> +                                                  struct mmc_queue_req
>> *mqrq,
>> +                                                  int disable_multi,
>> +                                                  struct mmc_async_req
>> *areq)
>> +{
>> +       /*
>> +        * Release host after failure in case the host is needed
>> +        * by someone else. For instance, if the card is removed the
>> +        * worker thread needs to claim the host in order to do mmc_rescan.
>> +        */
>> +       mmc_release_host(card->host);
>> +       mmc_claim_host(card->host);
>> ++      if (mmc_card_gone(card)) {
>> ++      /*
>> ++       * End the pending async request without starting it when card
>> ++       * is removed.
>> ++       */
>> ++              req->cmd_flags |= REQ_QUIET;
>> ++              __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
> I'll add this to the patch and send it out.
I'll wait adding this since mmc_card_gone() is not available yet.
Maybe we should send these two patches (aync error release and kill
block request) in the same patch-set?

Thanks,
Per

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

[Sujit Reddy Thumma]

On 11/14/2011 1:54 PM, Per Forlin wrote:
> On Mon, Nov 14, 2011 at 8:52 AM, Per Forlin<per.lkml@gmail.com>  wrote:
>> On Mon, Nov 14, 2011 at 5:19 AM, Sujit Reddy Thumma
>> <sthumma@codeaurora.org>  wrote:
>>> On 11/10/2011 7:50 PM, Per Forlin wrote:
>>>>
>>>> On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter<adrian.hunter@intel.com>
>>>>   wrote:
>>>>>
>>>>> On 10/11/11 06:02, Sujit Reddy Thumma wrote:
>>>>>>
>>>>>> Hi,
>>>>>>>
>>>>>>> Hi Adrian,
>>>>>>>
>>>>>>> On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@intel.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On 09/11/11 06:31, Sujit Reddy Thumma 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>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>>
>>>>>>>> It is safer to have zero initialisations so I suggest inverting
>>>>>>>> the meaning of the state bit i.e.
>>>>>>
>>>>>> Makes sense. Will take care in V2.
>>>>>>
>>>>>>>>
>>>>>>>> #define MMC_STATE_CARD_GONE     (1<<7)          /* card removed from
>>>>>>>> the
>>>>>>>> slot */
>>>>>>>>
>>>>>>>>
>>>>>>>> Also it would be nice is a request did not start if the card is
>>>>>>>> gone.  For the non-async case, that is easy:
>>>>>>>>
>>>>>>> As far as I understand Sujit's patch it will stop new requests from
>>>>>>> being issued, blk_fetch_request(q) returns NULL.
>>>>>>
>>>>>> Yes, Per you are right. The ongoing requests will fail at the controller
>>>>>> driver layer and only the new requests coming to MMC queue layer will be
>>>>>> blocked.
>>>>>>
>>>>>> Adrian's suggestion is to stop all the requests reaching host controller
>>>>>> layer by mmc core. This seems to be a good approach but the problem here
>>>>>> is
>>>>>> the host driver should inform mmc core that card is gone.
>>>>>>
>>>>>> Adrian, do you agree if we need to change all the host drivers to set
>>>>>> the
>>>>>> MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects
>>>>>> the
>>>>>> card as removed?
>>>>>
>>>>> Typically a card detect interrupt queues a rescan which in turn will have
>>>>> to wait to claim the host.  In the meantime, in the async case, the block
>>>>> driver will not release the host until the queue is empty.
>>>>
>>>> Here is a patch that let async-mmc release host and reclaim it in case of
>>>> error.
>>>> When the host is released mmc_rescan will claim the host and run.
>>>> --------------------------------
>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>>>> index cf73877..8952e63 100644
>>>> --- a/drivers/mmc/card/block.c
>>>> +++ b/drivers/mmc/card/block.c
>>>> @@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
>>>> *md, struct mmc_card *card,
>>>>         return ret;
>>>>   }
>>>>
>>>> +/*
>>>> + * This function should be called to resend a request after failure.
>>>> + * Prepares and starts the request.
>>>> + */
>>>> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>>>> +                                                  struct mmc_queue *mq,
>>>> +                                                  struct mmc_queue_req
>>>> *mqrq,
>>>> +                                                  int disable_multi,
>>>> +                                                  struct mmc_async_req
>>>> *areq)
>>>> +{
>>>> +       /*
>>>> +        * Release host after failure in case the host is needed
>>>> +        * by someone else. For instance, if the card is removed the
>>>> +        * worker thread needs to claim the host in order to do
>>>> mmc_rescan.
>>>> +        */
>>>> +       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);
>>>> +}
>>>> +
>>>>   static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
>>>>   {
>>>>         struct mmc_blk_data *md = mq->data;
>>>> @@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct
>>>> mmc_queue *mq, struct request *rqc)
>>>>                         break;
>>>>                 }
>>>>
>>>> -               if (ret) {
>>>> +               if (ret)
>>>>                         /*
>>>>                          * In case of a incomplete request
>>>>                          * prepare it again and resend.
>>>>                          */
>>>> -                       mmc_blk_rw_rq_prep(mq_rq, card, disable_multi,
>>>> mq);
>>>> -                       mmc_start_req(card->host,&mq_rq->mmc_active,
>>>> NULL);
>>>> -               }
>>>> +                       mmc_blk_prep_start(card, mq, mq_rq, disable_multi,
>>>> +&mq_rq->mmc_active);
>>>> +
>>>
>>> :%s/mmc_blk_prep_start/mmc_blk_resend
>>>
>> I'll update.
>>
>>>>         } while (ret);
>>>>
>>>>         return 1;
>>>> @@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
>>>> *mq, struct request *rqc)
>>>>         spin_unlock_irq(&md->lock);
>>>>
>>>>    start_new_req:
>>>> -       if (rqc) {
>>>> -               mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
>>>> -               mmc_start_req(card->host,&mq->mqrq_cur->mmc_active, NULL);
>>>> -       }
>>>> +       if (rqc)
>>>> +               mmc_blk_prep_start(card, mq, mq->mqrq_cur, 0,
>>>> +&mq->mqrq_cur->mmc_active);
>>>>
>>>>         return 0;
>>>>   }
>>>
>>> Thanks Per. This looks good. Can we split this into a different patch?
>>>
>> Yes I agree. My intention was to treat this as a separate patch.
>> I'll post it.
>>
>>>> -------------------------------------
>>>>
>>>>
>>>>>   The block
>>>>> driver will see errors and will use a send status command which will fail
>>>>> so the request will be aborted, but the next request will be started
>>>>> anyway.
>>>>>
>>>>> There are two problems:
>>>>>
>>>>> 1. When do we reliably know that the card has really gone?
>>>>>
>>>>> At present, the detect method for SD and MMC is just the send status
>>>>> command, which is what the block driver is doing i.e. if the block
>>>>> driver sees send status fail, after an errored request, and the
>>>>> card is removable, then it is very likely the card has gone.
>>>>>
>>>>> At present, it is not considered that the host controller driver
>>>>> knows whether the card is really gone - just that it might be.
>>>>>
>>>>> Setting a MMC_STATE_CARD_GONE flag early may be a little complicated.
>>>>> e.g. mmc_detect_change() flags that the card may be gone.  After an
>>>>> error, if the "card may be gone" flag is set a new bus method could
>>>>> be called that just does send status.  If that fails, then the
>>>>> MMC_STATE_CARD_GONE flag is set.  Any calls to the detect method
>>>>> must first check the MMC_STATE_CARD_GONE flag so that, once gone,
>>>>> a card can not come back.
>>>>>
>>>>> Maybe you can think of something simpler.
>>>
>>> Can we do something like this:
>>>
>>> --- 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_card_gone(card);
>>>                 return ERR_ABORT;
>>> +       }
>>>
>>>         /* Flag ECC errors */
>>>         if ((status&  R1_CARD_ECC_FAILED) ||
>>>
>>> and some additions to Per's patch, changes denoted in (++):
>>>
>>> +/*
>>> + * This function should be called to resend a request after failure.
>>> + * Prepares and starts the request.
>>> + */
>>> +static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
>>> +                                                  struct mmc_queue *mq,
>>> +                                                  struct mmc_queue_req
>>> *mqrq,
>>> +                                                  int disable_multi,
>>> +                                                  struct mmc_async_req
>>> *areq)
>>> +{
>>> +       /*
>>> +        * Release host after failure in case the host is needed
>>> +        * by someone else. For instance, if the card is removed the
>>> +        * worker thread needs to claim the host in order to do mmc_rescan.
>>> +        */
>>> +       mmc_release_host(card->host);
>>> +       mmc_claim_host(card->host);
>>> ++      if (mmc_card_gone(card)) {
>>> ++      /*
>>> ++       * End the pending async request without starting it when card
>>> ++       * is removed.
>>> ++       */
>>> ++              req->cmd_flags |= REQ_QUIET;
>>> ++              __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
>> I'll add this to the patch and send it out.
> I'll wait adding this since mmc_card_gone() is not available yet.
> Maybe we should send these two patches (aync error release and kill
> block request) in the same patch-set?

I think we can split this up without having this change in your patch. I 
can add this as part of kill block request change itself which makes 
your patch independent.

>
> Thanks,
> Per


-- 
Thanks,
Sujit

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

[Per Forlin]

Hi Sujit,

On Wed, Nov 9, 2011 at 5:31 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>
> ---
>  drivers/mmc/card/queue.c |    5 +++++
>  drivers/mmc/core/bus.c   |    2 ++
>  include/linux/mmc/card.h |    3 +++
>  3 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index dcad59c..f8a3298 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 && mq->card && !mmc_card_inserted(mq->card))
I guess the card is not freed until all pending requests have been
flushed? mq->card will be valid as long as the queue is active.

Another way to detect card removal is to subscribe for
"BUS_NOTIFY_DEL_DEVICE" mmc card device.

> +               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 46b6e84..ea3be5d 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card)
>                        mmc_card_ddr_mode(card) ? "DDR " : "",
>                        type, card->rca);
>        }
> +       mmc_card_set_inserted(card);
If the card-alloction is freed when the card is removed. Is it
necessary to set this bit for the new allocated card? Or could this be
the same card allocation?

>
>  #ifdef CONFIG_DEBUG_FS
>        mmc_add_card_debugfs(card);
> @@ -340,6 +341,7 @@ void mmc_remove_card(struct mmc_card *card)
>                        pr_info("%s: card %04x removed\n",
>                                mmc_hostname(card->host), card->rca);
>                }
> +               card->state &= ~MMC_STATE_INSERTED;
>                device_del(&card->dev);
>        }
>
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index b7a8cb1..be4125a 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_INSERTED     (1<<7)          /* card present in 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 */
> @@ -362,6 +363,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_inserted(c)   ((c)->state & MMC_STATE_INSERTED)
>  #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)
> @@ -370,6 +372,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_inserted(c) ((c)->state |= MMC_STATE_INSERTED)
>  #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.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Sujit Reddy Thumma]

Hi Per,

On 11/10/2011 3:17 AM, Per Forlin wrote:
> Hi Sujit,
>
> On Wed, Nov 9, 2011 at 5:31 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>
>> ---
>>   drivers/mmc/card/queue.c |    5 +++++
>>   drivers/mmc/core/bus.c   |    2 ++
>>   include/linux/mmc/card.h |    3 +++
>>   3 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>> index dcad59c..f8a3298 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&&  mq->card&&  !mmc_card_inserted(mq->card))
> I guess the card is not freed until all pending requests have been
> flushed? mq->card will be valid as long as the queue is active.

Agreed. This is a redundant check, will remove it in v2.

>
> Another way to detect card removal is to subscribe for
> "BUS_NOTIFY_DEL_DEVICE" mmc card device.

Thanks, this sounds good, for the current v1 patch.
I have a concern about this when we take Adrian's suggestion. If we want 
to set the card gone flag as soon as the card is removed, then we can 
stop any new block request. Registering for BUS_NOTIFY_DEL_DEVICE only 
stops sync requests issued in device_del().

>
>> +               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 46b6e84..ea3be5d 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -308,6 +308,7 @@ int mmc_add_card(struct mmc_card *card)
>>                         mmc_card_ddr_mode(card) ? "DDR " : "",
>>                         type, card->rca);
>>         }
>> +       mmc_card_set_inserted(card);
> If the card-alloction is freed when the card is removed. Is it
> necessary to set this bit for the new allocated card? Or could this be
> the same card allocation?
>
Adrian's suggestion, "It is safer to have zero initialisations so I 
suggest inverting the meaning of the state bit," I guess, answer your 
question. We set the card gone flag when the card is removed i.e., 
either in mmc_remove_card() or host driver's card detect irq handler and 
while card allocation is freed it is cleared anyways.

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

[David Taylor]

Sujit Reddy Thumma <sthumma <at> codeaurora.org> writes:

> 
> 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 <at> codeaurora.org>
> ---
>  drivers/mmc/card/queue.c |    5 +++++
>  drivers/mmc/core/bus.c   |    2 ++
>  include/linux/mmc/card.h |    3 +++
>  3 files changed, 10 insertions(+), 0 deletions(-)
> 

Thanks, this patch worked nicely to control a seemingly endless series of 
driver complaints when the SD card was removed during a transfer.

The OMAP4 I'm working on has hardware assisted detection of card removal, and 
this patch looks like it will be sufficient.

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

[Per Forlin]

Hi David,

On Tue, Nov 22, 2011 at 9:18 PM, David Taylor <dmtaylor@skytex.net> wrote:
> Sujit Reddy Thumma <sthumma <at> codeaurora.org> writes:
>
>>
>> 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 <at> codeaurora.org>
>> ---
>>  drivers/mmc/card/queue.c |    5 +++++
>>  drivers/mmc/core/bus.c   |    2 ++
>>  include/linux/mmc/card.h |    3 +++
>>  3 files changed, 10 insertions(+), 0 deletions(-)
>>
>
> Thanks, this patch worked nicely to control a seemingly endless series of
> driver complaints when the SD card was removed during a transfer.
>
> The OMAP4 I'm working on has hardware assisted detection of card removal, and
> this patch looks like it will be sufficient.
>
What happens if you run "dd if=/dev/zero of=/dev/mmcblk0 bs=1M
count=1000" and during dd the card is ejected?
When I test this, the host is claimed until the dd has tried to
transfer all 1000MB.

Regards,
Per

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

[Sujit Reddy Thumma]

On 11/24/2011 3:00 PM, Per Forlin wrote:
> Hi David,
>
> On Tue, Nov 22, 2011 at 9:18 PM, David Taylor<dmtaylor@skytex.net>  wrote:
>> Sujit Reddy Thumma<sthumma<at>  codeaurora.org>  writes:
>>
>>>
>>> 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<at>  codeaurora.org>
>>> ---
>>>   drivers/mmc/card/queue.c |    5 +++++
>>>   drivers/mmc/core/bus.c   |    2 ++
>>>   include/linux/mmc/card.h |    3 +++
>>>   3 files changed, 10 insertions(+), 0 deletions(-)
>>>
>>
>> Thanks, this patch worked nicely to control a seemingly endless series of
>> driver complaints when the SD card was removed during a transfer.
>>
>> The OMAP4 I'm working on has hardware assisted detection of card removal, and
>> this patch looks like it will be sufficient.
>>
> What happens if you run "dd if=/dev/zero of=/dev/mmcblk0 bs=1M
> count=1000" and during dd the card is ejected?
> When I test this, the host is claimed until the dd has tried to
> transfer all 1000MB.

That is true only when we are using >3.0 kernel.

--
Thanks,
Sujit