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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



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_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.


Thanks,
Per
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

Add to Google