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

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

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. 

Feel free to comment,
Per