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

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

Adrian Hunter wrote:
> On 13/01/12 16:35, Ulf Hansson wrote:
>> Adrian Hunter wrote:
>>> 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
>>>
>> OK, but.. :-)
>>
>> I were just about to update the patch according to your recommendation when
>> I realized the following:
>>
>> Once _mmc_detect_card_removed has set the card state as removed
>> ("mmc_card_set_removed"), the card will never be accessible for I/O requests
>> any more, all I/O will "silently" be thrown away in the block layer. This
>> leads to that there should definitely be no reason for _not_ letting a
>> scheduled rescan remove the card as soon as possible. In other words the
>> CAP2 should not be needed.
>>
>> Did I miss something?
>>
>> Agree?
> 
> No.  mmc_detect_card_removed() will not check/set the card removed
> unless there has been a call to mmc_detect_change() to set the
> host->detect_change flag.

That were before this patch. This patch removes the detect_change flag 
since it is used as you say to prevent "mmc_detect_card_removed" from 
calling _mmc_detect_card_removed and thus possibly setting the card 
state to removed.

The use of the detect_flag is a bit strange I think. It means simply 
that after getting one GPIO cd irq and then an I/O error we will only 
try at _most_ _one_ time from mmc_detect_card_removed to see if the card 
really has been removed. If the mmc_detect_card_removed the first time 
does not detect that the card is removed it will have to wait for the 
rescan the cover it, which is likely not what we want!?

I will see if I can figure out a way of keeping the old scenario in 
parallel with having MMC_CAP2_RESCAN_ON_ERROR... I will post a new patch.

> 
> MMC_CAP2_RESCAN_ON_ERROR is definitely needed.
> 
> Do not confuse mmc_detect_card_removed() with _mmc_detect_card_removed().
> The former is called by block.c.  The latter is only called by mmc_rescan()
> via the ->detect method.
>