Re: [RESEND PATCH V3] mmc: core: Detect card removal on I/O error

[Adrian Hunter <adrian.hunter@intel.com>]

On 03/02/12 15:23, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 03/02/12 14:16, Ulf Hansson wrote:
>>> Adrian Hunter wrote:
>>>> On 03/02/12 11:33, Ulf Hansson wrote:
>>>>> To prevent I/O as soon as possible at card removal, a new detect work
>>>>> is re-scheduled without a delay to let a rescan remove the card device
>>>>> as soon a possible.
>>>>>
>>>>> Additionally, MMC_CAP2_DETECT_ON_ERR can now be used to handle "slowly"
>>>>> removed cards that a scheduled detect work did not detect as removed.
>>>>> To prevent further I/O requests for these lingering removed cards,
>>>>> check if card has been removed and
>>>>> then schedule a detect work to properly remove it.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com> ---
>>>>>
>>>>> Changes in v3: - Check for card is NULL and minor code
>>>>> simplifications.
>>>>>
>>>>> Changes in v2: - Updated according to review comments. -
>>>>> Merging two patches for this feature into one.
>>>>>
>>>>> --- drivers/mmc/core/core.c  |   24 +++++++++++++++++++++---
>>>>> include/linux/mmc/host.h |    1 + 2 files changed, 22
>>>>> insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
>>>>> bec0bf2..9645e8c 100644 --- a/drivers/mmc/core/core.c +++
>>>>> b/drivers/mmc/core/core.c @@ -2077,18 +2077,36 @@ int
>>>>> _mmc_detect_card_removed(struct mmc_host *host) int
>>>>> mmc_detect_card_removed(struct mmc_host *host) { struct
>>>>> mmc_card *card = host->card; +    int ret;
>>>>>
>>>>> WARN_ON(!host->claimed); + +    if (!card) +        return 1; +
>>>>>  +    ret = mmc_card_removed(card); /* * The card will be
>>>>> considered unchanged unless we have been asked to * detect a
>>>>> change or host requires polling to provide card detection. */ -
>>>>> if (card && !host->detect_change && !(host->caps &
>>>>> MMC_CAP_NEEDS_POLL)) -        return mmc_card_removed(card); +
>>>>> if (!host->detect_change && !(host->caps & MMC_CAP_NEEDS_POLL)
>>>>> && +        !(host->caps2 & MMC_CAP2_DETECT_ON_ERR)) +
>>>>> return ret;
>>>>>
>>>>> host->detect_change = 0; +    if (!ret) { +        ret =
>>>>> _mmc_detect_card_removed(host); +        if (ret) {
>>>> Please make this
>>>>
>>>> if (ret && (host->caps2 & MMC_CAP2_DETECT_ON_ERR))
>>>>
>>>> because if rescan is already running this will needlessly queue another
>>>> one.
>>>>
>>> It wont queue another one. It will cancel a work which likely has
>>> been scheduled to be executed within several ms later. Then it will
>>> re-schedule a new one without any delay since there is no need to
>>> wait, when we already know that the card has been removed.
>>
>> It won't cancel a rescan that is already running (waiting to claim
>> the host for example) but it will queue another one.  Hence the
>> comment.
>>
> 
> Do you think this is case that we need to bother about? It should be a
> rare case and in worst case we only schedule a second not needed rescan,
> which I believe should not be a problem.
> 
> I discussed this with Jaehoon Chung, previously regarding the return value
> of cancel_delayed_work(&host->detect). See below copied comments:
> 
> I change accordingly if you like, please let me know. Again. :-)

Yes please.

> 
> ----------
> 
>>>> if (cancel_delayed_work(&host->detect))
>>>>>> mmc_detect_change(host, 0); isn't?
>>>>
>>>> Good comment. That will mean that patch 2 will have to be updated
>>>> as well to something like below.
>>>>
>>>> if (cancel_delayed_work(&host->detect) || (host->caps2 &
>>>> MMC_CAP2_DETECT_ON_ERR)) mmc_detect_change(host, 0);
>>>>
>>>> What do you think?
>>>>
>>>> Could we skip this entirely and leave it as is without checking
>>>> the return value of cancel_delayed_work? That will only mean that
>>>> in some very rare cases (since rescan is clearing the
>>>> detect_change flag) one additional detect work will be triggered
>>>> which shall not cause any problems I believe. But I happily
>>>> change to what you propose if you prefer!
>>
>> Right...maybe..don't cause any problems..i also think. It's rare
>> case.  :) Best Regards, Jaehoon Chung
>>
> 
> ---------------------
>