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

[Ulf Hansson <ulf.hansson@stericsson.com>]

Adrian Hunter wrote:
> 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.
> 

Fixed a v4 patch then. Thanks.

>> ----------
>>
>>>>> 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
>>>
>> ---------------------
>>
> 
>