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

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

On 13/01/12 13:31, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 13/01/12 12:04, Ulf Hansson wrote:
>>> Adrian Hunter wrote:
>>>> On 10/01/12 12:59, Ulf Hansson wrote:
>>>>> Adrian Hunter wrote:
>>>>>> 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.
>>>>> That is not correct. The rescan uses the get_cd function to find out if
>>>>> it really make sense to try to initialize a new card. It is not used for
>>>>> removing existing cards.
>>>> mmc_rescan() first calls host->bus_ops->detect() to see if the card is
>>>> still
>>>> there.  If the card does not respond then it is removed.  Then mmc_rescan
>>>> attempts to initialize a new card. host->bus_ops->detect() is not used for
>>>> that.
>>>>
>>>>> You were referring to "[PATCH 2/4 v4] MMC/SD: Add callback function to
>>>>> detect card". This patch will prevent the bus_ops->alive function to be
>>>>> called if the get_cd function indicates that the card is still there. I
>>>>> can not see how this on it's own will help out to solve the issue my
>>>>> patch is trying to solve.
>>>> Yes it will because it is called by mmc_rescan() and used to remove the
>>>> card
>>>> via host->bus_ops->detect()
>>>>
>>> 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.

> 
> 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
	- upper layers want to attempt to recover an unresponsive card
	- 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

> 
>>
>> So it should work if you:
>>
>>     - debounce the gpio line
>>     - record whether or not the card is present based on the debounced
>>     gpio line
>>     - either error out requests when the card is not present
>>     or
>>     - use the get_cd patch (still ought to be driver selected)
>>     and implement get_cd based on whether you have recorded the card
>>     present or not

In fact the get_cd approach is flawed.  If a new card has been inserted
then get_cd will say the old card is present whereas CMD13 would fail for a
new card because it has not been initialized.

>>
>>
>