Re: [PATCH] mmc: core: Clean up after mmc_pre_req if card was removed

["Per Förlin" <per.forlin@stericsson.com>]

On 03/05/2012 07:08 AM, Jaehoon Chung wrote:
> On 03/05/2012 02:08 PM, Jaehoon Chung wrote:
> 
>> On 03/03/2012 12:29 AM, Per Förlin wrote:
>>
>>> On 03/02/2012 09:51 AM, Ulf HANSSON wrote:
>>>> Hi Jaehoon,
>>>>
>>>> I did not know this. Which host driver are you using? I would very much 
>>>> appreciate of you could debug and share some result.
>>>>
>>>> Thanks!
>>>>
>>>> BR
>>>> Ulf Hansson
>>>>
>>>> On 03/02/2012 09:28 AM, Jaehoon Chung wrote:
>>>>> Hi Ulf.
>>>>>
>>>>> I tested with this patch.
>>>>> But in my environment, this patch didn't work fine before.
>>>>> 1) When remove/insert, didn't entered the suspend.
>>>>> 2) When removed during something write,
>>>>> [   50.755067] FAT-fs (mmcblk1p1): Directory bread(block 8254) failed
>>>>> [   50.761235] FAT-fs (mmcblk1p1): Directory bread(block 8255) failed
>>>>> then at next-time, didn't detect sd-card.
>>>>>
>>>>> Did you know this?
>>>>> If you want more information, i will debug, and share the result.
>>>>>
>>>>> Best Regards,
>>>>> Jaehoon Chung
>>>>>
>>>>> On 03/02/2012 12:44 AM, Ulf Hansson wrote:
>>>>>
>>>>>> Make sure mmc_start_req cancel the prepared job, if the request
>>>>>> was prevented to be started due to the card has been removed.
>>>>>>
>>>>>> This bug was introduced in commit:
>>>>>> mmc: allow upper layers to know immediately if card has been removed
>>>>>>
>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>> ---
>>>>>>   drivers/mmc/core/core.c |   35 +++++++++++++++--------------------
>>>>>>   1 files changed, 15 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>> index 0b317f0..9e562ab 100644
>>>>>> --- a/drivers/mmc/core/core.c
>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>> @@ -249,16 +249,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;
>>>>>>   	if (mmc_card_removed(host->card)) {
>>>>>>   		mrq->cmd->error = -ENOMEDIUM;
>>>>>>   		complete(&mrq->completion);
>>>>>> -		return;
>>>>>> +		return -ENOMEDIUM;
>>>>>>   	}
>>>>>>   	mmc_start_request(host, mrq);
>>>>>> +	return 0;
>>>>>>   }
>>>>>>
>>>>>>   static void mmc_wait_for_req_done(struct mmc_host *host,
>>>>>> @@ -342,6 +343,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
>>>>>>   				    struct mmc_async_req *areq, int *error)
>>>>>>   {
>>>>>>   	int err = 0;
>>>>>> +	int start_err = 0;
>>>>>>   	struct mmc_async_req *data = host->areq;
>>>>>>
>>>>>>   	/* Prepare a new request */
>>>>>> @@ -351,30 +353,23 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,
>>>>>>   	if (host->areq) {
>>>>>>   		mmc_wait_for_req_done(host, host->areq->mrq);
>>>>>>   		err = host->areq->err_check(host->card, host->areq);
>>>>>> -		if (err) {
>>>>>> -			/* post process the completed failed request */
>>>>>> -			mmc_post_req(host, host->areq->mrq, 0);
>>>>>> -			if (areq)
>>>>>> -				/*
>>>>>> -				 * Cancel the new prepared request, because
>>>>>> -				 * it can't run until the failed
>>>>>> -				 * request has been properly handled.
>>>>>> -				 */
>>>>>> -				mmc_post_req(host, areq->mrq, -EINVAL);
>>>>>> -
>>>>>> -			host->areq = NULL;
>>>>>> -			goto out;
>>>>>> -		}
>>>>>>   	}
>>>>>>
>>>>>> -	if (areq)
>>>>>> -		__mmc_start_req(host, areq->mrq);
>>>>>> +	if (!err&&  areq)
>>>>>> +		start_err = __mmc_start_req(host, areq->mrq);
>>>>>>
>>>>>>   	if (host->areq)
>>>>>>   		mmc_post_req(host, host->areq->mrq, 0);
>>>>>>
>>>>>> -	host->areq = areq;
>>>>>> - out:
>>>>>> +	if (err || start_err) {
>>>>>> +		if (areq)
>>>>>> +			/* The prepared request was not started, cancel it. */
>>>>>> +			mmc_post_req(host, areq->mrq, -EINVAL);
>>>>>> +		host->areq = NULL;
>>> There seems to be an issue when setting host->areq=NULL when __mmc_start_req fails. host->areq == NULL indicates there are no ongoing transfers.
>>> host->areq is used in block.c to check if there are pending requests. 
>>>
>>> This seem to work:
>>> ...
>>> if (err || start_err) {
>>> 	if (areq)
>>> 		/* The prepared request was not started, cancel it. */
>>> 		mmc_post_req(host, areq->mrq, -EINVAL);
>>> }
>>>
>>> if (err)
>>> 	host->areq = NULL;
>>> else
>>> 	host->areq = areq;
>>> ...
>>>
>>> This issue will be addressed in version 2. How to resolve it is not decided yet. 
> 
> If start_err is set and err didn't set, maybe should be set "host->areq = areq".
> Then in block.c, should be check the "status".
> 
> But start_err didn't be assigned anywhere, how about this?
> ...
> if (error) {
> 	*error = err
> 	if (start_err)
> 		*error = start_err;
> }
> ...
Thanks for your feedback.

The execution flow may look like this.
1. mmc_start_req(req1)
2. (request starts)
3. (card is removed)
4. mmc_start(req2)
5. req1 is returned with error code set, but req2 gets start_err.
In this case start_err should not be returned.

If NULL is returned from mmc_start_req the error code is discarded.
I think you may set *error=start_err and return data, but only if err is 0.

if (!err && start_err)
	*error = start_err;

Br
Per