Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

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

On 09/01/12 13:02, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 3/01/2012 12:33 p.m., Ulf Hansson wrote:
>>> Removing a card "slowly" can trigger a GPIO irq to be raised far
>>> before the card is actually removed. This means the scheduled
>>> detect work will not find out that the card were removed and thus
>>> the card and the block device will not be unregistered.
>>
>> One way around that problem is to error out requests after the GPIO
>> indicates the card is "removed".  sdhci effectively does that but with
>> a "present" bit.  Perhaps even simpler - set the card-removed flag.
> 
> I get the idea, but that will only partly solve this issue.
> 
> My concern is more about what we actually can trust; either the GPIO irq
> which likely is giving more than one irq when inserting/removing a card
> since the slot is probably not glitch free, or that a "rescan" runs to make
> sure a CMD13 is accepted from the previously inserted card.

Yes, I guess you would need to debounce the GPIO if you wanted to rely on it.

> 
> Moreover, the issue this patch tries to solve can not be solved without
> doing a "rescan" which must be triggered from the the block layer some how.
> I thought this new function that you previously added
> "mmc_detect_card_remove" was the proper place to do this.
> 
>>
>>> Let the mmc_detect_card_removed function trigger a new detect
>>> work immediately when it discovers that a card has been removed.
>>
>> This is changing some long-standing functionality i.e. the card is not
>> removed
>> without a card detect event.  It is difficult to know whether that will be
>> very
>> bad for poor quality cards,
> 
> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
> to make sure it is still present, that is already done from the block layer
> after each read/write request. So I can not see that "poor quality cards"
> will have any further problem with this patch, but I might miss something!?

The block driver has never caused a card to be removed before.  That is new
and it is designed to preserve existing behaviour i.e. do not remove a card
without a card detect event.

You are assuming:
	1. that a poor quality card will not return errors for a few
	commands and then resume operation
	2. that removing a card on error is desirable

Both those assumptions may be true, but there is no evidence that they are.


> 
>>
>>> This will solve the described issue above. Moreover we make sure
>>> the detect work is executed as soon as possible, since there is
>>> no reason for waiting for a "delayed" detect to happen.
>>>
>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>> ---
>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>   include/linux/mmc/host.h |    1 -
>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 4770807..7bc02f4 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>> unsigned long delay)
>>>       WARN_ON(host->removed);
>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>   #endif
>>> -    host->detect_change = 1;
>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>   }
>>>
>>> @@ -2077,18 +2076,23 @@ 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 = 1;
>>>
>>>       WARN_ON(!host->claimed);
>>> -    /*
>>> -     * 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);
>>>
>>> -    host->detect_change = 0;
>>> +    if (card&&  !mmc_card_removed(card)) {
>>> +        if (_mmc_detect_card_removed(host)) {
>>> +            /*
>>> +             * Make sure a detect work is always executed and also
>>> +             * do it as soon as possible.
>>> +             */
>>> +            cancel_delayed_work(&host->detect);
>>> +            mmc_detect_change(host, 0);
>>> +        }
>>> +        ret = mmc_card_removed(card);
>>> +    }
>>>
>>> -    return _mmc_detect_card_removed(host);
>>> +    return ret;
>>>   }
>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>
>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>           host->bus_ops->detect(host);
>>>
>>> -    host->detect_change = 0;
>>> -
>>>       /*
>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>        * the card is no longer present.
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index 031d865..09fa5e6 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>       int            claim_cnt;    /* "claim" nesting count */
>>>
>>>       struct delayed_work    detect;
>>> -    int            detect_change;    /* card detect flag */
>>>       struct mmc_hotplug    hotplug;
>>>
>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>
>>
> 
> Br
> Ulf Hansson
>