On 09/01/12 16:27, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>> 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.
>>> True, but is this a problem!?
>>
>> Better not to find out.
>
> :-)
>
> Then there is lot of other things around mmc we also should not change.
Can you give an example of a change in existing functionality? Isn't
everything either a bug fix or new functionality?
>
>>
>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>> remove a card "slowly", a "rescan" work, which the GPIO irq has triggered to
>>> run will run the CMD13 to verify that the card is still there. Since it has
>>> not completely been removed the CMD13 will succeed and the card will not be
>>> removed.
>>>
>>> Moreover every other new block request will soon start to fail and always
>>> do; until a new rescan is triggered (which is when you insert a new card or
>>> do a suspend-resume cycle). In practice I think it is more preferred that
>>> the card gets removed and it's corresponding block device.
>>
>> There are other ways to solve that problem. Apart from my previous
>> suggestion, there is also the possibility to make use of ->get_cd
>> instead of CMD13, someone already posted a patch for that
>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>> but it should probably be selected on a per driver basis (i.e. add a
>> MMC_CAP2 for it). I guess you would still need to debounce the GPIO
>> though.
>>
>
> Unfortunately that wont help to solve this issue either. That patch will
> only prevent you from executing a CMD13 if the get_cd function says the card
> is still there. I kind of micro optimization I think, unless you very often
> encounters errors in the block layer.
No, the rescan calls that code, so if get_cd() returns 0 the card will be
removed irrespective of whether it has been pulled out slowly or not.
>
> The key in this patch is that a rescan work is triggered to fully verify
> that the card is still there and if not, it can remove it. I don't think
> this is such a big matter, but of course this is my own opinion. :-)
Another issue with your patch is that the card will not be removed unless
there is subsequent I/O to cause an I/O error and subsequent rescan.
>
>>>> You are assuming:
>>>> 1. that a poor quality card will not return errors for a few
>>>> commands and then resume operation
>>> I see your point. I did some tests with a bunch of old crappy cards, both SD
>>> and MMC which I had in my collection. I have found none of these to trigger
>>> a undesirable removal of the card.
>>>
>>> Of course I have only a subset of all cards, so this can not be fully tested
>>> for all existing cards.
>>>
>>>> 2. that removing a card on error is desirable
>>> Well, we will just fire of a rescan work to check if the card has been
>>> removed. If it is still there it will of course not be removed.
>>
>> Not if it has stopped responding. Again, this is a change in behaviour.
>> Previously, a card that stopped responding was not removed.
>>
>> Perhaps in the future someone will want to try to recover cards that
>> stop responding, for example by power-cycling. That would be in
>> conflict with your approach because it would power cycle on every single
>> card removal.
>
> This is pure hypothetical and the simple solution to such an idea would
> just be to do a "power-cycle attempt" before considering scheduling the
> rescan work in the mmc_detect_card_removed function.
Nevertheless, in your case a power-cycle would be done for every card removal.
>
>>
>>>> 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@xxxxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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
>>>>>
>>>>
>>>
>>
>>
>
>
--
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
