On Mon, Nov 14, 2011 at 5:19 AM, Sujit Reddy Thumma
<[email protected]> wrote:
> On 11/10/2011 7:50 PM, Per Forlin wrote:
>>
>> On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter<[email protected]>
>> 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<[email protected]>
>>>>> 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<[email protected]>
>>>>>>> ---
>>>>>>
>>>>>>
>>>>>> 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-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
