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