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