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.
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.
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.
You are assuming:
1. that a poor quality card will not return errors for a few
commands and then resume operation
I see your point. I did some tests with a bunch of old crappy cards, both SD
and MMC which I had in my collection. I have found none of these to trigger
a undesirable removal of the card.
Of course I have only a subset of all cards, so this can not be fully tested
for all existing cards.
2. that removing a card on error is desirable
Well, we will just fire of a rescan work to check if the card has been
removed. If it is still there it will of course not be removed.
Not if it has stopped responding. Again, this is a change in behaviour.
Previously, a card that stopped responding was not removed.
Perhaps in the future someone will want to try to recover cards that
stop responding, for example by power-cycling. That would be in
conflict with your approach because it would power cycle on every single
card removal.
This is pure hypothetical and the simple solution to such an idea would
just be to do a "power-cycle attempt" before considering scheduling the
rescan work in the mmc_detect_card_removed function.
Nevertheless, in your case a power-cycle would be done for every card removal.
Do quite follow, what do you mean by every card removal? We detect that
the card has been removed (by using _mmc_detect_card_removed) and only
at that time a rescan (power-cycle) is triggered.
Both those assumptions may be true, but there is no evidence that they are.
This will solve the described issue above. Moreover we make sure
the detect work is executed as soon as possible, since there is
no reason for waiting for a "delayed" detect to happen.
Signed-off-by: Ulf Hansson<ulf.hansson@xxxxxxxxxxxxxx>
---
drivers/mmc/core/core.c | 24 +++++++++++++-----------
include/linux/mmc/host.h | 1 -
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 4770807..7bc02f4 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
unsigned long delay)
WARN_ON(host->removed);
spin_unlock_irqrestore(&host->lock, flags);
#endif
- host->detect_change = 1;
mmc_schedule_delayed_work(&host->detect, delay);
}
@@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
*host)
int mmc_detect_card_removed(struct mmc_host *host)
{
struct mmc_card *card = host->card;
+ int ret = 1;
WARN_ON(!host->claimed);
- /*
- * The card will be considered unchanged unless we have been
asked to
- * detect a change or host requires polling to provide card
detection.
- */
- if (card&& !host->detect_change&& !(host->caps&
MMC_CAP_NEEDS_POLL))
- return mmc_card_removed(card);
- host->detect_change = 0;
+ if (card&& !mmc_card_removed(card)) {
+ if (_mmc_detect_card_removed(host)) {
+ /*
+ * Make sure a detect work is always executed and also
+ * do it as soon as possible.
+ */
+ cancel_delayed_work(&host->detect);
+ mmc_detect_change(host, 0);
+ }
+ ret = mmc_card_removed(card);
+ }
- return _mmc_detect_card_removed(host);
+ return ret;
}
EXPORT_SYMBOL(mmc_detect_card_removed);
@@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
&& !(host->caps& MMC_CAP_NONREMOVABLE))
host->bus_ops->detect(host);
- host->detect_change = 0;
-
/*
* Let mmc_bus_put() free the bus/bus_ops if we've found that
* the card is no longer present.
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 031d865..09fa5e6 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -305,7 +305,6 @@ struct mmc_host {
int claim_cnt; /* "claim" nesting count */
struct delayed_work detect;
- int detect_change; /* card detect flag */
struct mmc_hotplug hotplug;
const struct mmc_bus_ops *bus_ops; /* current bus driver */
Br
Ulf Hansson
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
