[PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[Namjae Jeon]

host controller can not rise timeout interrupt in write not read in DMA status. 
because host can just know whether card is finish to program to use busy line. 
If unstable card is keep holding busy line while writing using DMA.
hang problem happen by wait_for_completion. so I think that mmc driver need some exception to avoid this problem.
I suggest to use wait_for_completion_timeout instead of wait_for_completion.

Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
---
 drivers/mmc/core/core.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 557856b..0fe83d3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -253,7 +253,20 @@ static void __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
 static void mmc_wait_for_req_done(struct mmc_host *host,
 				  struct mmc_request *mrq)
 {
-	wait_for_completion(&mrq->completion);
+	unsigned int timeout_us;
+
+	if ((mrq->data != NULL) && (mrq->data->flags & MMC_DATA_WRITE)) {
+		timeout_us = mrq->data->timeout_ns / 1000;
+		if (mmc_host_clk_rate(host))
+			timeout_us += mrq->data->timeout_clks * 1000 /
+				(mmc_host_clk_rate(host) / 1000);
+		if (!timeout_us)
+			timeout_us = 1000000;
+		if (!wait_for_completion_timeout(&mrq->completion,
+				usecs_to_jiffies(timeout_us)))
+			mrq->cmd->error = -ETIMEDOUT;
+	} else
+		wait_for_completion(&mrq->completion);
 }
 
 /**
-- 
1.7.4.4

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[Ulf Hansson]

Namjae Jeon wrote:
> host controller can not rise timeout interrupt in write not read in DMA status. 
> because host can just know whether card is finish to program to use busy line. 
> If unstable card is keep holding busy line while writing using DMA.
> hang problem happen by wait_for_completion. so I think that mmc driver need some exception to avoid this problem.
> I suggest to use wait_for_completion_timeout instead of wait_for_completion.
> 

I see what you are trying to solve, but you can never calculate the 
timeout for this type of operation in such a way. Your timeout involves 
the entire data write operation, how can you ever know how long this 
will take?

I think a much better approach is to make you host driver not using 
"busy signaling" (if that is possible), thus when the DMA job is done 
call mmc_request_done to finalize the data transfer. The mmc framework 
will then send a CMD13 (SEND_STATUS) to make sure the data is written 
before issuing the next request.

BR
Ulf Hansson

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[Murali Krishna Palnati]

On Tue, Sep 20, 2011 at 1:09 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>
> Namjae Jeon wrote:
>>
>> host controller can not rise timeout interrupt in write not read in DMA status. because host can just know whether card is finish to program to use busy line. If unstable card is keep holding busy line while writing using DMA.
>> hang problem happen by wait_for_completion. so I think that mmc driver need some exception to avoid this problem.
>> I suggest to use wait_for_completion_timeout instead of wait_for_completion.
>>
>
> I see what you are trying to solve, but you can never calculate the timeout for this type of operation in such a way. Your timeout involves the entire data write operation, how can you ever know how long this will take?
>
> I think a much better approach is to make you host driver not using "busy signaling" (if that is possible), thus when the DMA job is done call mmc_request_done to finalize the data transfer. The mmc framework will then send a CMD13 (SEND_STATUS) to make sure the data is written before issuing the next request.
>
> BR
> Ulf Hansson


To add to what Hansson mentioned, the timeout mechanism should not be
implemented at the MMC core layer. If the core layer simply times out
like this, the host controller driver still remains in the same state
which is not right. If such a timeout mechanism is needed, it is best
to implement in the host controller driver layer and inform the core
layer by calling mmc_request_done() when the time out happens. This
will also give a chance to the host controller driver to do any
required clean up at the controller layer so that it is in a known
good state to handle further requests from the core layer.

Regards,
Murali Krishna

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[NamJae Jeon]

2011/9/20 Murali Krishna Palnati <palnati.muralikrishna@gmail.com>:
> On Tue, Sep 20, 2011 at 1:09 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>>
>> Namjae Jeon wrote:
>>>
>>> host controller can not rise timeout interrupt in write not read in DMA status. because host can just know whether card is finish to program to use busy line. If unstable card is keep holding busy line while writing using DMA.
>>> hang problem happen by wait_for_completion. so I think that mmc driver need some exception to avoid this problem.
>>> I suggest to use wait_for_completion_timeout instead of wait_for_completion.
>>>
>>
>> I see what you are trying to solve, but you can never calculate the timeout for this type of operation in such a way. Your timeout involves the entire data write operation, how can you ever know how long this will take?
>>
>> I think a much better approach is to make you host driver not using "busy signaling" (if that is possible), thus when the DMA job is done call mmc_request_done to finalize the data transfer. The mmc framework will then send a CMD13 (SEND_STATUS) to make sure the data is written before issuing the next request.
>>
>> BR
>> Ulf Hansson
>
>
> To add to what Hansson mentioned, the timeout mechanism should not be
> implemented at the MMC core layer. If the core layer simply times out
> like this, the host controller driver still remains in the same state
> which is not right. If such a timeout mechanism is needed, it is best
> to implement in the host controller driver layer and inform the core
> layer by calling mmc_request_done() when the time out happens. This
> will also give a chance to the host controller driver to do any
> required clean up at the controller layer so that it is in a known
> good state to handle further requests from the core layer.
>
> Regards,
> Murali Krishna
>

yes, I knew. But if card keep holding busy line(data 0) in the middle
of DMA operation(multi blocks write),  host driver layer can not know
card status in this situation. addtionally, After timeout happen, host
driver should send stop cmd to card for putting out busyline. and we
should reset/cleanup host contoller to intialize DMA controller. and
it should be impremented in host driver. so after returing write fail,
host can access card again.

Write_timeout is calculated by the R2W_FACTOR field * mult in the CSD.
but it is timeout about only one block. so I think that timeout value
is multiplied with number of blocks.
I will modify v2 patch by adding this code.

timeout_us = (mrq->data->timeout_ns * mrq->data->blocks)  / 1000;

I think that this exception should be like this.
If write 128KB request  -> timeout is calculated to 6.4 sec if it
spend 250ms per block -> timeout error happen -> mmc core send stop
cmd ->  mmc core try to call host reset/cleanup -> return write cmd
error.

How do you think ?

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[Murali Krishna Palnati]

On Tue, Sep 20, 2011 at 4:05 PM, NamJae Jeon <linkinjeon@gmail.com> wrote:
> yes, I knew. But if card keep holding busy line(data 0) in the middle
> of DMA operation(multi blocks write),  host driver layer can not know
> card status in this situation.

The host driver shall start a software timer and indpendently needs to
watch out for the expiry of the timeout. If the host doesn't hear back
from the
controller/DMA engine even after waiting for the specified timeout,
then the timer handler that gets invoked can then do the necessary
cleanup and
inform the MMC core layer that the request failed. Does this sound feasible?

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[NamJae Jeon]

2011/9/20 Murali Krishna Palnati <palnati.muralikrishna@gmail.com>:
> On Tue, Sep 20, 2011 at 4:05 PM, NamJae Jeon <linkinjeon@gmail.com> wrote:
>> yes, I knew. But if card keep holding busy line(data 0) in the middle
>> of DMA operation(multi blocks write),  host driver layer can not know
>> card status in this situation.
>
> The host driver shall start a software timer and indpendently needs to
> watch out for the expiry of the timeout. If the host doesn't hear back
> from the
> controller/DMA engine even after waiting for the specified timeout,
> then the timer handler that gets invoked can then do the necessary
> cleanup and
> inform the MMC core layer that the request failed. Does this sound feasible?
>

It may be no good choice that sw timer is on host driver. also I don't
know what is different.
It is the same as the ata driver's dma_map_sg.
dma_map_sg is repectively called in mmc host driver. but it is being
processed in ata core of ata driver.
because it is common routine, ata core have called it instead of host driver.
if it is processed in mmc core, there is no need to modify several
hosts.(except dma controller reset)

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[Murali Krishna Palnati]

On Tue, Sep 20, 2011 at 7:52 PM, NamJae Jeon <linkinjeon@gmail.com> wrote:
> It may be no good choice that sw timer is on host driver. also I don't
> know what is different.

It helps to have this functionality implemented at host controller
layer so that the host layer is informed about this. If we just end
the request from the MMC core layer, host controller driver doesnt
even kow about that and it remains in the same state processing the
request (that already got timed out at core layer). It is good to have
the host layer trigger this timeout, do necessary clean up and then
duly end the request by informing the core layer by calling
mmc_request_done().

Let me put the question in this way. If the core layer times out
(because of wait_for_completion_timeout) then in the patch that you
have submitted, i dont see how the host layer knows about it.
Apologize, if i sound like a broken record saying the same thing again
and again.

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[NamJae Jeon]

2011/9/21 Murali Krishna Palnati <palnati.muralikrishna@gmail.com>:
> On Tue, Sep 20, 2011 at 7:52 PM, NamJae Jeon <linkinjeon@gmail.com> wrote:
>> It may be no good choice that sw timer is on host driver. also I don't
>> know what is different.
>
> It helps to have this functionality implemented at host controller
> layer so that the host layer is informed about this. If we just end
> the request from the MMC core layer, host controller driver doesnt
> even kow about that and it remains in the same state processing the
> request (that already got timed out at core layer). It is good to have
> the host layer trigger this timeout, do necessary clean up and then
> duly end the request by informing the core layer by calling
> mmc_request_done().
>
> Let me put the question in this way. If the core layer times out
> (because of wait_for_completion_timeout) then in the patch that you
> have submitted, i dont see how the host layer knows about it.
> Apologize, if i sound like a broken record saying the same thing again
> and again.
>

I understand what you want. So I will add emergency_cleanup for host
driver like this.
struct mmc_host_ops {
................
.................
void (*emergency_cleanup)(struct mmc_host *host);

}

When timeout error happen, mmc core will call this function as soon as
sending stop cmd.
And when timeout error happen by wait_for_completion_timeout, calling
mmc_request_done is not needed.

Thanks.

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[NamJae Jeon]

2011/9/21 NamJae Jeon <linkinjeon@gmail.com>:
> 2011/9/21 Murali Krishna Palnati <palnati.muralikrishna@gmail.com>:
>> On Tue, Sep 20, 2011 at 7:52 PM, NamJae Jeon <linkinjeon@gmail.com> wrote:
>>> It may be no good choice that sw timer is on host driver. also I don't
>>> know what is different.
>>
>> It helps to have this functionality implemented at host controller
>> layer so that the host layer is informed about this. If we just end
>> the request from the MMC core layer, host controller driver doesnt
>> even kow about that and it remains in the same state processing the
>> request (that already got timed out at core layer). It is good to have
>> the host layer trigger this timeout, do necessary clean up and then
>> duly end the request by informing the core layer by calling
>> mmc_request_done().
>>
>> Let me put the question in this way. If the core layer times out
>> (because of wait_for_completion_timeout) then in the patch that you
>> have submitted, i dont see how the host layer knows about it.
>> Apologize, if i sound like a broken record saying the same thing again
>> and again.
>>
>
> I understand what you want. So I will add emergency_cleanup for host
> driver like this.
> struct mmc_host_ops {
> ................
> .................
> void (*emergency_cleanup)(struct mmc_host *host);
>
> }
>
> When timeout error happen, mmc core will call this function as soon as
> sending stop cmd.
> And when timeout error happen by wait_for_completion_timeout, calling
> mmc_request_done is not needed.
>
> Thanks.
>

I'll follow the opinion of chris.

Thanks.

Re: [PATCH] mmc : Use wait_for_completion_timeout() instead of wait_for_completion in case of write.

[Linus Walleij]

On Sun, Sep 18, 2011 at 5:21 PM, Namjae Jeon <linkinjeon@gmail.com> wrote:

> +                       timeout_us += mrq->data->timeout_clks * 1000 /
> +                               (mmc_host_clk_rate(host) / 1000);

The math teacher says this is equivalent to:

timeout_us += (mrq->data->timeout_clks / mmc_host_clk_rate) * 1000000 ;

Yours,
Linus Walleij