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

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

On 11/14/2011 1:54 PM, Per Forlin wrote:
> On Mon, Nov 14, 2011 at 8:52 AM, Per Forlin<per.lkml@gmail.com>  wrote:
>> On Mon, Nov 14, 2011 at 5:19 AM, Sujit Reddy Thumma
>> <sthumma@codeaurora.org>  wrote:
>>> On 11/10/2011 7:50 PM, Per Forlin wrote:
>>>>
>>>> On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter<adrian.hunter@intel.com>
>>>>   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@intel.com>
>>>>>>> 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@codeaurora.org>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>>
>>>>>>>> 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
>>>
>> I'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