On 11/10/2011 7:50 PM, Per Forlin wrote:
On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter<adrian.hunter@xxxxxxxxx> wrote:
On 10/11/11 06:02, Sujit Reddy Thumma wrote:
Hi,
Hi Adrian,
On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunter<adrian.hunter@xxxxxxxxx>
wrote:
On 09/11/11 06:31, Sujit Reddy Thumma wrote:
Kill block requests when the host knows that the card is
removed from the slot and is sure that subsequent requests
are bound to fail. Do this silently so that the block
layer doesn't output unnecessary error messages.
This patch implements suggestion from Adrian Hunter,
http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
Signed-off-by: Sujit Reddy Thumma<sthumma@xxxxxxxxxxxxxx>
---
It is safer to have zero initialisations so I suggest inverting
the meaning of the state bit i.e.
Makes sense. Will take care in V2.
#define MMC_STATE_CARD_GONE (1<<7) /* card removed from the
slot */
Also it would be nice is a request did not start if the card is
gone. For the non-async case, that is easy:
As far as I understand Sujit's patch it will stop new requests from
being issued, blk_fetch_request(q) returns NULL.
Yes, Per you are right. The ongoing requests will fail at the controller
driver layer and only the new requests coming to MMC queue layer will be
blocked.
Adrian's suggestion is to stop all the requests reaching host controller
layer by mmc core. This seems to be a good approach but the problem here is
the host driver should inform mmc core that card is gone.
Adrian, do you agree if we need to change all the host drivers to set the
MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects the
card as removed?
Typically a card detect interrupt queues a rescan which in turn will have
to wait to claim the host. In the meantime, in the async case, the block
driver will not release the host until the queue is empty.
Here is a patch that let async-mmc release host and reclaim it in case of error.
When the host is released mmc_rescan will claim the host and run.
--------------------------------
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index cf73877..8952e63 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
*md, struct mmc_card *card,
return ret;
}
+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
+ struct mmc_queue *mq,
+ struct mmc_queue_req *mqrq,
+ int disable_multi,
+ struct mmc_async_req *areq)
+{
+ /*
+ * Release host after failure in case the host is needed
+ * by someone else. For instance, if the card is removed the
+ * worker thread needs to claim the host in order to do mmc_rescan.
+ */
+ mmc_release_host(card->host);
+ mmc_claim_host(card->host);
+
+ mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+ return mmc_start_req(card->host, areq, NULL);
+}
+
static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
{
struct mmc_blk_data *md = mq->data;
@@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct
mmc_queue *mq, struct request *rqc)
break;
}
- if (ret) {
+ if (ret)
/*
* In case of a incomplete request
* prepare it again and resend.
*/
- mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, mq);
- mmc_start_req(card->host,&mq_rq->mmc_active, NULL);
- }
+ mmc_blk_prep_start(card, mq, mq_rq, disable_multi,
+ &mq_rq->mmc_active);
+
:%s/mmc_blk_prep_start/mmc_blk_resend
} while (ret);
return 1;
@@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
*mq, struct request *rqc)
spin_unlock_irq(&md->lock);
start_new_req:
- if (rqc) {
- mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
- mmc_start_req(card->host,&mq->mqrq_cur->mmc_active, NULL);
- }
+ if (rqc)
+ mmc_blk_prep_start(card, mq, mq->mqrq_cur, 0,
+ &mq->mqrq_cur->mmc_active);
return 0;
}
Thanks Per. This looks good. Can we split this into a different patch?
-------------------------------------
The block
driver will see errors and will use a send status command which will fail
so the request will be aborted, but the next request will be started
anyway.
There are two problems:
1. When do we reliably know that the card has really gone?
At present, the detect method for SD and MMC is just the send status
command, which is what the block driver is doing i.e. if the block
driver sees send status fail, after an errored request, and the
card is removable, then it is very likely the card has gone.
At present, it is not considered that the host controller driver
knows whether the card is really gone - just that it might be.
Setting a MMC_STATE_CARD_GONE flag early may be a little complicated.
e.g. mmc_detect_change() flags that the card may be gone. After an
error, if the "card may be gone" flag is set a new bus method could
be called that just does send status. If that fails, then the
MMC_STATE_CARD_GONE flag is set. Any calls to the detect method
must first check the MMC_STATE_CARD_GONE flag so that, once gone,
a card can not come back.
Maybe you can think of something simpler.
Can we do something like this:
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card
*card, struct request *req,
}
/* We couldn't get a response from the card. Give up. */
- if (err)
+ if (err) {
+ /*
+ * If the card didn't respond to status command,
+ * it is likely that card is gone. Flag it as removed,
+ * mmc_detect_change() cleans the rest.
+ */
+ mmc_card_set_card_gone(card);
return ERR_ABORT;
+ }
/* Flag ECC errors */
if ((status & R1_CARD_ECC_FAILED) ||
and some additions to Per's patch, changes denoted in (++):
+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
+ struct mmc_queue *mq,
+ struct mmc_queue_req *mqrq,
+ int disable_multi,
+ struct mmc_async_req *areq)
+{
+ /*
+ * Release host after failure in case the host is needed
+ * by someone else. For instance, if the card is removed the
+ * worker thread needs to claim the host in order to do mmc_rescan.
+ */
+ mmc_release_host(card->host);
+ mmc_claim_host(card->host);
++ if (mmc_card_gone(card)) {
++ /*
++ * End the pending async request without starting it when card
++ * is removed.
++ */
++ req->cmd_flags |= REQ_QUIET;
++ __blk_end_request(req, -EIO, blk_rq_cur_bytes(req));
+ } else {
+ mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+ return mmc_start_req(card->host, areq, NULL);
+ }
+}
+
2. How can new requests be prevented from starting once the card
is gone?
Using BLKPREP_KILL will do that for all but the request that has been
prepared already (i.e. async case) if the MMC_STATE_CARD_GONE flag is set
early enough.
Maybe blocking requests at a lower level is not needed.
--
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
