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

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

[Ulf Hansson]

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;
+	} else {
+		host->areq = areq;
+	}
+
 	if (error)
 		*error = err;
 	return data;
-- 
1.7.9

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

[Linus Walleij]

On Thu, Mar 1, 2012 at 4:44 PM, Ulf Hansson <ulf.hansson@stericsson.com> 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>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Per Förlin]

On 03/01/2012 04:44 PM, 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;
> +	} else {
> +		host->areq = areq;
> +	}
> +
>  	if (error)
>  		*error = err;
>  	return data;
I have reviewed this patch offline. Patch looks good to me.

Reviewed-by: Per Forlin <per.forlin@stericsson.com>

Br
Per

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

[Jaehoon Chung]

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;
> +	} else {
> +		host->areq = areq;
> +	}
> +
>  	if (error)
>  		*error = err;
>  	return data;

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

[Ulf Hansson]

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;
>> +	} else {
>> +		host->areq = areq;
>> +	}
>> +
>>   	if (error)
>>   		*error = err;
>>   	return data;
>
>
>

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

[Per Förlin]

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

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

[Jaehoon Chung]

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. 

It seems to work fine. But i didn't test yet.

Best Regards,
Jaehoon Chung

> 
> Feel free to comment,
> Per
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Jaehoon Chung]

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;
}
...

Best Regards,
Jaehoon Chung

> 
> It seems to work fine. But i didn't test yet.
> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> Feel free to comment,
>> Per
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Per Förlin]

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