> On Thu, Nov 24, 2011 at 12:27 PM, Sujit Reddy Thumma > <sthumma@xxxxxxxxxxxxxx> wrote: >> Hi Per, >> >> On 11/22/2011 6:40 PM, Per Forlin wrote: >>> >>> Hi Sujit, >>> >>> On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma >>> <sthumma@xxxxxxxxxxxxxx> 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> >>>> >>>> --- >>>> Changes in v2: >>>> - Changed the implementation with further comments from Adrian >>>> - Set the card removed flag in bus notifier callbacks >>>> - This patch is now dependent on patch from Per Forlin: >>>> >>>> http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211 >>>> --- >>>> drivers/mmc/card/block.c | 33 ++++++++++++++++++++++++++++----- >>>> drivers/mmc/card/queue.c | 5 +++++ >>>> drivers/mmc/core/bus.c | 32 +++++++++++++++++++++++++++++++- >>>> drivers/mmc/core/core.c | 8 +++++++- >>>> include/linux/mmc/card.h | 3 +++ >>>> 5 files changed, 74 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c >>>> index edc379e..83956fa 100644 >>>> --- 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_removed(card); >>>> return ERR_ABORT; >>>> + } >>>> >>>> /* Flag ECC errors */ >>>> if ((status& R1_CARD_ECC_FAILED) || >>>> @@ -1168,6 +1175,9 @@ static inline struct mmc_async_req >>>> *mmc_blk_resend(struct mmc_card *card, >>>> int disable_multi, >>>> struct mmc_async_req >>>> *areq) >>>> { >>>> + struct mmc_blk_data *md = mmc_get_drvdata(card); >>>> + struct request *req = mqrq->req; >>>> + int ret; >>>> /* >>>> * Release host after failure in case the host is needed >>>> * by someone else. For instance, if the card is removed the >>>> @@ -1175,11 +1185,24 @@ static inline struct mmc_async_req >>>> *mmc_blk_resend(struct mmc_card *card, >>>> */ >>>> 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); >>>> + if (mmc_card_removed(card)) { >>>> + /* >>>> + * End the pending async request without starting >>>> + * it when card is removed. >>>> + */ >>>> + spin_lock_irq(&md->lock); >>>> + req->cmd_flags |= REQ_QUIET; >>>> + do { >>>> + ret = __blk_end_request(req, >>>> + -EIO, blk_rq_cur_bytes(req)); >>>> + } while (ret); >>>> + spin_unlock_irq(&md->lock); >>>> + return NULL; >>>> + } else { >>>> + mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq); >>>> + return mmc_start_req(card->host, areq, NULL); >>>> + } >>> >>> mmc_blk_resend is only called to resend a request that has failed. If >>> the card is removed the request will still be issued, but when it >>> retries it will give up here. >> >> Currently, we set the card_removed flag in two places: >> >> 1) If the host supports interrupt detection, mmc_detect_change() calls >> the >> bus detect method and we set card removed flag in bus notifier call >> back. >> >> 2) When there is a transfer going on (host is claimed by mmcqd) and the >> card >> is removed ungracefully, the driver sends an error code to the block >> layer. >> mmc_blk_cmd_recovery() tries to send CMD13 to the card which in this >> case >> fails and we set the card_removed flag. >> >> So when we retry or send next async request we end it in >> mmc_blk_resend() >> and there will be no subsequent request to the driver since we are >> suppressing the requests in the queue layer. >> >> So I guess there is no need to add further checks in the >> __mmc_start_req(), >> which could be redundant as there are no calls to the core layer from >> block >> layer once the card is found to be removed. >> >> In summary the sequence I thought would be like this: >> Case 1: >> 1) Transfer is going on (host claimed by mmcqd) and card is removed. >> 2) Driver is in middle of transaction, hence some kind of timeout error >> is >> returned. >> 3) mmc_blk_cmd_recovery(): Block layer does error checking and sets the >> card >> as removed. >> 4) Block layer then retries the request calling mmc_blk_resend(). >> 5) Since the mmc_card_removed is true we end the request here and do not >> send any request to the core. >> >> Case 2: >> 1) When there is no transfer going on (host->claimed = 0) >> 2) mmc_detect_change() would set card_removed flag. >> 3) All the subsequent requests would be killed in queue layer itself. >> >>> You have added a check in mmc_wait_for_req(). What about this: >> >> mmc_wait_for_req() will be called to send regular CMDs as well, hence we >> need to add a check. >> >>> -------------------------- >>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>> index b526036..dcdcb9a 100644 >>> --- a/drivers/mmc/core/core.c >>> +++ b/drivers/mmc/core/core.c >>> @@ -287,11 +287,17 @@ static void mmc_wait_done(struct mmc_request >>> *mrq) >>> complete(&mrq->completion); >>> } >>> >>> -static void __mmc_start_req(struct mmc_host *host, struct mmc_request >>> *mrq) >>> +static int __mmc_start_req(struct mmc_host *host, struct mmc_request >>> *mrq) >>> { >>> - init_completion(&mrq->completion); >>> - mrq->done = mmc_wait_done; >>> - mmc_start_request(host, mrq); >>> + if (mmc_card_removed(host->card)) { >>> + mrq->cmd->error = -ENOMEDIUM; >>> + return -ENOMEDIUM; >>> + } >> >> This is not yet done here. If an async request comes then it will do a >> wait_for_req_done() for the previous request which returned even without >> doing a init_completion. So we need to handle this case in >> mmc_start_req(). >> > You're right. Thanks for spotting this. init_completion and done must > be assigned. > The patch should look like this: > ------------------------------- > init_completion(&mrq->completion); > mrq->done = mmc_wait_done; > if (mmc_card_removed(host->card)) { > complete(&mrq->completion); > mrq->cmd->error = -ENOMEDIUM; > return -ENOMEDIUM; > } > mmc_start_request(host, mrq); > ----------------------- > What about this? Looks feasible. > >>> + >>> + init_completion(&mrq->completion); >>> + mrq->done = mmc_wait_done; >>> + mmc_start_request(host, mrq); >>> + return 0; >>> } >>> >>> static void mmc_wait_for_req_done(struct mmc_host *host, >>> @@ -418,7 +424,8 @@ EXPORT_SYMBOL(mmc_start_req); >>> */ >>> void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq) >>> { >>> - __mmc_start_req(host, mrq); >>> + if (__mmc_start_req(host, mrq)) >>> + return >>> mmc_wait_for_req_done(host, mrq); >>> } >>> EXPORT_SYMBOL(mmc_wait_for_req); >>> ---------------------------------- >>> This patch will set error to -ENOMEDIUM for both mmc_start_req() and >>> mmc_wait_for_req() >>> >>> mmc_blk_err_check() can check for -ENOMEDIUM and return something like >>> MMC_BLK_ENOMEDIUM >> >> If I understand correctly, the above patch handles a third case: >> 1) issue_fn is called by the queue layer (for the first request) >> 2) just before the mmcqd claims host card is removed and the >> mmc_detect_change has flagged the card as removed. >> 3) we send the request and before it was sent to host driver >> __mmc_start_req() sent -ENOMEDIUM and the block layer handles new error >> ENOMEDIUM. >> >> This is a valid case but increases the complexity as we need to take >> care of >> the async request which does not know we have returned prematurely >> without >> doing init_completion(). > My intention is to simplify the error handling by checking > is-card-removed in __mmc_start_req() rather than both > mmc_wait_for_req() and mmc_start_req(). > > Flow for async: > 0. *card is removed > 1. mmc_start_req() > 2. -> mmc_wait_for_req_done() > 3. ->->err_check() > 4. ->->->mmc_blk_cmd_recovery() > 5. ->->->->mmc_card_set_removed(card) > 6. skip start next request and returns > 7. new request from block layer I guess this new request wouldn't be arrived. Since, by now card removed flag is set and the blk_fetch_request() would always give req = NULL. We have already made sure to kill new requests in prepare stage itself: @@ -37,6 +39,9 @@ static int mmc_prep_request(struct request_queue *q, struct request *req) return BLKPREP_KILL; } + if (mq && mmc_card_removed(mq->card)) + return BLKPREP_KILL; + req->cmd_flags |= REQ_DONTPREP; Nevertheless, your patch looks feasible. Please let me know if I can add it or ignore. > 8. mmc_start_req() > 9. ->__mmc_start_req() > 10.new request from block layer > 11. mmc_start_req() > 12. -> mmc_wait_for_req_done() /* completion is already completed in > __mmc_start_req(), checks card-is-removed and returns */ > 13. ->-> err_check() /* prints no warning since error is -ENOMEDIUM */ > >> >> I guess we can live with this rare case with just outputting few errors, >> as >> the whole idea of patch is to reduce (if not completely eliminate) >> unnecessary prints in the log output which can bog down cpu if some >> wired >> peripheral like UART console is enabled. >> >> Please let me know if I am missing something. >> > Here's my case. in mmci.c: mmc_detect_change(host->mmc, > msecs_to_jiffies(500)); > It takes 500 ms until mmc_rescan is called. Of course, I can change > the timing but let's skip that part for now. > This means the block layer will continue to issue requests for 500 ms > after the card is ejected. There will be no warnings for the retries > because the card-is-removed is checked in resend(), but for new > incoming requests there will be error prints. -- 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
