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

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

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

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


> 
> 
>>>>> 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.
>>>>
>>> This is exactly the problem this patch is trying to solve. Instead of
>>> "forever" keeping the card inserted and thus returning errors for every
>>> new I/O request, we trigger a rescan to fully remove the card.
>>
>> If the user pulls out the card slowly so that the rescan sees the card still
>> there, then if there is no I/O there will be no I/O error and the kernel
>> will not remove the card - until the user sticks in another card or tries to
>> access files that are not there.
>>
> 
> Br
> Ulf Hansson
>