On 11/14/2011 1:54 PM, Per Forlin wrote:
On Mon, Nov 14, 2011 at 8:52 AM, Per Forlin<per.lkml@xxxxxxxxx> wrote:On Mon, Nov 14, 2011 at 5:19 AM, Sujit Reddy Thumma <sthumma@xxxxxxxxxxxxxx> wrote: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_resendI'll update.} 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?Yes I agree. My intention was to treat this as a separate patch. I'll post it.-------------------------------------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));I'll add this to the patch and send it out.I'll wait adding this since mmc_card_gone() is not available yet. Maybe we should send these two patches (aync error release and kill block request) in the same patch-set?
I think we can split this up without having this change in your patch. I can add this as part of kill block request change itself which makes your patch independent.
Thanks, Per
-- Thanks, Sujit -- 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
