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

[Sujit Reddy Thumma <sthumma@codeaurora.org>]

Hi Adrian,

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

Agreed. This is much cleaner implementation. Thanks.

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

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

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

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

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

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

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

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


+}
+EXPORT_SYMBOL(mmc_detect_card_removed);
+

-- 
Thanks,
Sujit