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

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

On 13/01/12 15:14, Ulf Hansson wrote:
>>>>> In principles this means the following sequence:
>>>>>
>>>>> We will rely on that the get_cd function will return 0 (indicating card is
>>>>> removed) when the card is "slowly" removed at the point when the rescan
>>>>> function is calling it through the bus_ops->detect -->
>>>>> _mmc_detect_card_removed function.
>>>>>
>>>>> This then becomes a race, meaning that the rescan function must be
>>>>> executing
>>>>> at the same time the get_cd function will returns 0. Otherwise the rescan
>>>>> function will not remove the card.
>>>>>
>>>>> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback
>>>>> function to
>>>>> detect card" will likely improve behavior but is not the safe solution to
>>>>> handle "slowly" removed cards.
>>>>>
>>>>> Again, to be sure, we must let the mmc_detect_card_remove function
>>>>> trigger a
>>>>> rescan when _mmc_detect_card_removed has detected that the card is
>>>>> removed.
>>>>> This should be safe in all circumstances.
>>>> sdhci has no problem because it does this:
>>>>
>>>>     - the host controller debounces the card detect line
>>>>     - the host controller records whether or not the card is present
>>>>     - the sdhci driver prevents (errors out) requests when the card is
>>>>     not present
>>> Debouncing will just be a way of triggering the problem more seldom. Or in
>>> worst case, state the card has been removed even if it has not.
>>
>> If a delay is used with mmc_detect_change, debouncing is not necessary.
>>
>>> Just because you get a GPIO irq on the detect line does not mean the card is
>>> removed, debouncing or not. I consider this as pure mechanical switch which
>>> likely has glitches and I don't see that we should trust it fully. We only
>>> want to trigger a detect work, which is exactly what is done in the patch
>>> from Guennadi Liakhovetski "mmc: add a generic GPIO card-detect helper".
>>
>> The original problem was "slow card removal".  "Unreliable card detect"
>> is a separate problem.  Currently there is polling (MMC_CAP_NEEDS_POLL)
>> for that.  Alternatively there is MMC_CAP2_RESCAN_ON_ERROR as we have
>> discussed.
> 
> I do not understand why you mention "Unreliable card detect"? That has
> nothing to do with this patch.
> 
> So to conclude the discussion, do you believe that this patch is acceptable
> as long as we add a CAPS2 option "MMC_CAP2_RESCAN_ON_ERROR", which if not
> set will prevent the detect work from being scheduled from
> mmc_detect_card_removed?

Yes

> 
>>
>>> If each host driver that supports GPIO card detect makes use of the
>>> card-detect helper and if we accept a version of this patch, I think the
>>> situation should be safe in all cases. Moreover GPIO debouncing will never
>>> be needed for GPIO card detect for your sdhci driver either.
>>
>> Safe in all cases, except at least the 3 already given:
>>     - card is buggy and sometimes fails several commands in a row
> I doubt this will become a real problem. If a card fails several times in a
> row, upper FS layers wont be happy either. So likely we are screwed anyway.
> Don't you think?
>>     - upper layers want to attempt to recover an unresponsive card
> This is not implemented as of right now. I see no problem that my patch will
> prevent this from being implemented in the future.
>>     - even in the case of slow removal, the vendor wants the card
>>     to show as removed immediately whether or not there is any I/O
> That is correct and can not be fully solved unless you use
> MMC_CAP_NEEDS_POLL. I doubt that one would like to use polling in favor of
> card detect only to take care of this issue though. So I believe it is more
> likely you want to trigger a card removal when receiving I/O. Kind of
> "Better late than never". :-)
> 
> 
> By the way, thanks for keeping up the frequency in this quite long discussion.
> 
> BR
> Ulf Hansson
>