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