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. >> >> > -- 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
