Re: [PATCH V2] mmc: Kill block requests if card is removed

["Sujit Reddy Thumma" <sthumma@codeaurora.org>]

> On Thu, Nov 24, 2011 at 12:27 PM, Sujit Reddy Thumma
> <sthumma@codeaurora.org> 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@codeaurora.org>  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@codeaurora.org>
>>>>
>>>> ---
>>>> 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.