* [PATCH v2] mmc: dw_mmc: Make sure we don't get stuck when we get an error
@ 2014-05-20 22:07 Doug Anderson
2014-05-21 9:08 ` Seungwon Jeon
0 siblings, 1 reply; 6+ messages in thread
From: Doug Anderson @ 2014-05-20 22:07 UTC (permalink / raw)
To: Seungwon Jeon, Jaehoon Chung
Cc: Chris Ball, Sonny Rao, Grant Grundler, linux-samsung-soc,
Kukjin Kim, sunil joshi, Tomasz Figa, Doug Anderson,
Yuvaraj Kumar C D, chris, ulf.hansson, linux-mmc, linux-kernel
If we happened to get a data error at just the wrong time the dw_mmc
driver could get into a state where it would never complete its
request. That would leave the caller just hanging there.
We fix this two ways and both of the two fixes on their own appear to
fix the problems we've seen:
1. Fix a race in the tasklet where the interrupt setting the data
error happens _just after_ we check for it, then we get a
EVENT_XFER_COMPLETE. We fix this by repeating a bit of code.
2. Fix it so that if we detect that we've got an error in the "data
busy" state and we're not going to do anything else we end the
request and unblock anyone waiting.
Signed-off-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@gmail.com>
---
Changes in v2:
- Removed TODO
- Set cmd to NULL before calling dw_mci_request_end()
drivers/mmc/host/dw_mmc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index cced599..54ec8b0 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1318,6 +1318,14 @@ static void dw_mci_tasklet_func(unsigned long priv)
/* fall through */
case STATE_SENDING_DATA:
+ /*
+ * We could get a data error and never a transfer
+ * complete so we'd better check for it here.
+ *
+ * Note that we don't really care if we also got a
+ * transfer complete; stopping the DMA and sending an
+ * abort won't hurt.
+ */
if (test_and_clear_bit(EVENT_DATA_ERROR,
&host->pending_events)) {
dw_mci_stop_dma(host);
@@ -1331,7 +1339,29 @@ static void dw_mci_tasklet_func(unsigned long priv)
break;
set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
+
+ /*
+ * Handle an EVENT_DATA_ERROR that might have shown up
+ * before the transfer completed. This might not have
+ * been caught by the check above because the interrupt
+ * could have gone off between the previous check and
+ * the check for transfer complete.
+ *
+ * Technically this ought not be needed assuming we
+ * get a DATA_COMPLETE eventually (we'll notice the
+ * error and end the request), but it shouldn't hurt.
+ *
+ * This has the advantage of sending the stop command.
+ */
+ if (test_and_clear_bit(EVENT_DATA_ERROR,
+ &host->pending_events)) {
+ dw_mci_stop_dma(host);
+ send_stop_abort(host, data);
+ state = STATE_DATA_ERROR;
+ break;
+ }
prev_state = state = STATE_DATA_BUSY;
+
/* fall through */
case STATE_DATA_BUSY:
@@ -1354,6 +1384,22 @@ static void dw_mci_tasklet_func(unsigned long priv)
/* stop command for open-ended transfer*/
if (data->stop)
send_stop_abort(host, data);
+ } else {
+ /*
+ * If we don't have a command complete now we'll
+ * never get one since we just reset everything;
+ * better end the request.
+ *
+ * If we do have a command complete we'll fall
+ * through to the SENDING_STOP command and
+ * everything will be peachy keen.
+ */
+ if (!test_bit(EVENT_CMD_COMPLETE,
+ &host->pending_events)) {
+ host->cmd = NULL;
+ dw_mci_request_end(host, mrq);
+ goto unlock;
+ }
}
/*
--
1.9.1.423.g4596e3a
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH v2] mmc: dw_mmc: Make sure we don't get stuck when we get an error
2014-05-20 22:07 [PATCH v2] mmc: dw_mmc: Make sure we don't get stuck when we get an error Doug Anderson
@ 2014-05-21 9:08 ` Seungwon Jeon
2014-07-27 14:15 ` Seungwon Jeon
2014-08-13 13:38 ` Doug Anderson
0 siblings, 2 replies; 6+ messages in thread
From: Seungwon Jeon @ 2014-05-21 9:08 UTC (permalink / raw)
To: 'Doug Anderson', 'Jaehoon Chung'
Cc: 'Chris Ball', 'Sonny Rao',
'Grant Grundler', 'linux-samsung-soc',
'Kukjin Kim', 'sunil joshi',
'Tomasz Figa', 'Yuvaraj Kumar C D',
chris, ulf.hansson, linux-mmc, linux-kernel
On Wed, May 21, 2014, Doug Anderson wrote:
> If we happened to get a data error at just the wrong time the dw_mmc
> driver could get into a state where it would never complete its
> request. That would leave the caller just hanging there.
>
> We fix this two ways and both of the two fixes on their own appear to
> fix the problems we've seen:
>
> 1. Fix a race in the tasklet where the interrupt setting the data
> error happens _just after_ we check for it, then we get a
> EVENT_XFER_COMPLETE. We fix this by repeating a bit of code.
> 2. Fix it so that if we detect that we've got an error in the "data
> busy" state and we're not going to do anything else we end the
> request and unblock anyone waiting.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@gmail.com>
It will be applied after "mmc: dw_mmc: change to use recommended reset procedure"
Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
Thanks,
Seungwon Jeon
> ---
> Changes in v2:
> - Removed TODO
> - Set cmd to NULL before calling dw_mci_request_end()
>
> drivers/mmc/host/dw_mmc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index cced599..54ec8b0 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1318,6 +1318,14 @@ static void dw_mci_tasklet_func(unsigned long priv)
> /* fall through */
>
> case STATE_SENDING_DATA:
> + /*
> + * We could get a data error and never a transfer
> + * complete so we'd better check for it here.
> + *
> + * Note that we don't really care if we also got a
> + * transfer complete; stopping the DMA and sending an
> + * abort won't hurt.
> + */
> if (test_and_clear_bit(EVENT_DATA_ERROR,
> &host->pending_events)) {
> dw_mci_stop_dma(host);
> @@ -1331,7 +1339,29 @@ static void dw_mci_tasklet_func(unsigned long priv)
> break;
>
> set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
> +
> + /*
> + * Handle an EVENT_DATA_ERROR that might have shown up
> + * before the transfer completed. This might not have
> + * been caught by the check above because the interrupt
> + * could have gone off between the previous check and
> + * the check for transfer complete.
> + *
> + * Technically this ought not be needed assuming we
> + * get a DATA_COMPLETE eventually (we'll notice the
> + * error and end the request), but it shouldn't hurt.
> + *
> + * This has the advantage of sending the stop command.
> + */
> + if (test_and_clear_bit(EVENT_DATA_ERROR,
> + &host->pending_events)) {
> + dw_mci_stop_dma(host);
> + send_stop_abort(host, data);
> + state = STATE_DATA_ERROR;
> + break;
> + }
> prev_state = state = STATE_DATA_BUSY;
> +
> /* fall through */
>
> case STATE_DATA_BUSY:
> @@ -1354,6 +1384,22 @@ static void dw_mci_tasklet_func(unsigned long priv)
> /* stop command for open-ended transfer*/
> if (data->stop)
> send_stop_abort(host, data);
> + } else {
> + /*
> + * If we don't have a command complete now we'll
> + * never get one since we just reset everything;
> + * better end the request.
> + *
> + * If we do have a command complete we'll fall
> + * through to the SENDING_STOP command and
> + * everything will be peachy keen.
> + */
> + if (!test_bit(EVENT_CMD_COMPLETE,
> + &host->pending_events)) {
> + host->cmd = NULL;
> + dw_mci_request_end(host, mrq);
> + goto unlock;
> + }
> }
>
> /*
> --
> 1.9.1.423.g4596e3a
>
> --
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH v2] mmc: dw_mmc: Make sure we don't get stuck when we get an error
2014-05-21 9:08 ` Seungwon Jeon
@ 2014-07-27 14:15 ` Seungwon Jeon
2014-08-13 13:38 ` Doug Anderson
1 sibling, 0 replies; 6+ messages in thread
From: Seungwon Jeon @ 2014-07-27 14:15 UTC (permalink / raw)
To: 'Seungwon Jeon', 'Doug Anderson',
'Jaehoon Chung'
Cc: 'Chris Ball', 'Sonny Rao',
'Grant Grundler', 'linux-samsung-soc',
'Kukjin Kim', 'sunil joshi',
'Tomasz Figa', 'Yuvaraj Kumar C D',
chris, ulf.hansson, linux-mmc, linux-kernel
Hi Chris & Ulf,
I hope you find this patch for next.
Thanks,
Seungwon Jeon
On Wed, May 21, 2014, Seungwon Jeon wrote:
> On Wed, May 21, 2014, Doug Anderson wrote:
> > If we happened to get a data error at just the wrong time the dw_mmc
> > driver could get into a state where it would never complete its
> > request. That would leave the caller just hanging there.
> >
> > We fix this two ways and both of the two fixes on their own appear to
> > fix the problems we've seen:
> >
> > 1. Fix a race in the tasklet where the interrupt setting the data
> > error happens _just after_ we check for it, then we get a
> > EVENT_XFER_COMPLETE. We fix this by repeating a bit of code.
> > 2. Fix it so that if we detect that we've got an error in the "data
> > busy" state and we're not going to do anything else we end the
> > request and unblock anyone waiting.
> >
> > Signed-off-by: Doug Anderson <dianders@chromium.org>
> > Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@gmail.com>
>
> It will be applied after "mmc: dw_mmc: change to use recommended reset procedure"
>
> Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
>
> Thanks,
> Seungwon Jeon
>
> > ---
> > Changes in v2:
> > - Removed TODO
> > - Set cmd to NULL before calling dw_mci_request_end()
> >
> > drivers/mmc/host/dw_mmc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 46 insertions(+)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index cced599..54ec8b0 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -1318,6 +1318,14 @@ static void dw_mci_tasklet_func(unsigned long priv)
> > /* fall through */
> >
> > case STATE_SENDING_DATA:
> > + /*
> > + * We could get a data error and never a transfer
> > + * complete so we'd better check for it here.
> > + *
> > + * Note that we don't really care if we also got a
> > + * transfer complete; stopping the DMA and sending an
> > + * abort won't hurt.
> > + */
> > if (test_and_clear_bit(EVENT_DATA_ERROR,
> > &host->pending_events)) {
> > dw_mci_stop_dma(host);
> > @@ -1331,7 +1339,29 @@ static void dw_mci_tasklet_func(unsigned long priv)
> > break;
> >
> > set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
> > +
> > + /*
> > + * Handle an EVENT_DATA_ERROR that might have shown up
> > + * before the transfer completed. This might not have
> > + * been caught by the check above because the interrupt
> > + * could have gone off between the previous check and
> > + * the check for transfer complete.
> > + *
> > + * Technically this ought not be needed assuming we
> > + * get a DATA_COMPLETE eventually (we'll notice the
> > + * error and end the request), but it shouldn't hurt.
> > + *
> > + * This has the advantage of sending the stop command.
> > + */
> > + if (test_and_clear_bit(EVENT_DATA_ERROR,
> > + &host->pending_events)) {
> > + dw_mci_stop_dma(host);
> > + send_stop_abort(host, data);
> > + state = STATE_DATA_ERROR;
> > + break;
> > + }
> > prev_state = state = STATE_DATA_BUSY;
> > +
> > /* fall through */
> >
> > case STATE_DATA_BUSY:
> > @@ -1354,6 +1384,22 @@ static void dw_mci_tasklet_func(unsigned long priv)
> > /* stop command for open-ended transfer*/
> > if (data->stop)
> > send_stop_abort(host, data);
> > + } else {
> > + /*
> > + * If we don't have a command complete now we'll
> > + * never get one since we just reset everything;
> > + * better end the request.
> > + *
> > + * If we do have a command complete we'll fall
> > + * through to the SENDING_STOP command and
> > + * everything will be peachy keen.
> > + */
> > + if (!test_bit(EVENT_CMD_COMPLETE,
> > + &host->pending_events)) {
> > + host->cmd = NULL;
> > + dw_mci_request_end(host, mrq);
> > + goto unlock;
> > + }
> > }
> >
> > /*
> > --
> > 1.9.1.423.g4596e3a
> >
> > --
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] mmc: dw_mmc: Make sure we don't get stuck when we get an error
2014-05-21 9:08 ` Seungwon Jeon
2014-07-27 14:15 ` Seungwon Jeon
@ 2014-08-13 13:38 ` Doug Anderson
2014-08-13 13:52 ` Jaehoon Chung
2014-08-13 15:04 ` Ulf Hansson
1 sibling, 2 replies; 6+ messages in thread
From: Doug Anderson @ 2014-08-13 13:38 UTC (permalink / raw)
To: Seungwon Jeon, Ulf Hansson
Cc: Jaehoon Chung, Chris Ball, Sonny Rao, Grant Grundler,
linux-samsung-soc, Kukjin Kim, sunil joshi, Tomasz Figa,
Yuvaraj Kumar C D, Chris Ball, linux-mmc, linux-kernel
Hi,
On Wed, May 21, 2014 at 2:08 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> On Wed, May 21, 2014, Doug Anderson wrote:
>> If we happened to get a data error at just the wrong time the dw_mmc
>> driver could get into a state where it would never complete its
>> request. That would leave the caller just hanging there.
>>
>> We fix this two ways and both of the two fixes on their own appear to
>> fix the problems we've seen:
>>
>> 1. Fix a race in the tasklet where the interrupt setting the data
>> error happens _just after_ we check for it, then we get a
>> EVENT_XFER_COMPLETE. We fix this by repeating a bit of code.
>> 2. Fix it so that if we detect that we've got an error in the "data
>> busy" state and we're not going to do anything else we end the
>> request and unblock anyone waiting.
>>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@gmail.com>
>
> It will be applied after "mmc: dw_mmc: change to use recommended reset procedure"
>
> Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
>
> Thanks,
> Seungwon Jeon
I saw that Ulf applied "mmc: dw_mmc: change to use recommended reset
procedure". Could we apply this one now, too? Do you want me to
repost?
-Doug
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmc: dw_mmc: Make sure we don't get stuck when we get an error
2014-08-13 13:38 ` Doug Anderson
@ 2014-08-13 13:52 ` Jaehoon Chung
2014-08-13 15:04 ` Ulf Hansson
1 sibling, 0 replies; 6+ messages in thread
From: Jaehoon Chung @ 2014-08-13 13:52 UTC (permalink / raw)
To: Doug Anderson, Seungwon Jeon, Ulf Hansson
Cc: Jaehoon Chung, Chris Ball, Sonny Rao, Grant Grundler,
linux-samsung-soc, Kukjin Kim, sunil joshi, Tomasz Figa,
Yuvaraj Kumar C D, Chris Ball, linux-mmc, linux-kernel
On 08/13/2014 10:38 PM, Doug Anderson wrote:
> Hi,
>
> On Wed, May 21, 2014 at 2:08 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> On Wed, May 21, 2014, Doug Anderson wrote:
>>> If we happened to get a data error at just the wrong time the dw_mmc
>>> driver could get into a state where it would never complete its
>>> request. That would leave the caller just hanging there.
>>>
>>> We fix this two ways and both of the two fixes on their own appear to
>>> fix the problems we've seen:
>>>
>>> 1. Fix a race in the tasklet where the interrupt setting the data
>>> error happens _just after_ we check for it, then we get a
>>> EVENT_XFER_COMPLETE. We fix this by repeating a bit of code.
>>> 2. Fix it so that if we detect that we've got an error in the "data
>>> busy" state and we're not going to do anything else we end the
>>> request and unblock anyone waiting.
>>>
>>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@gmail.com>
>>
>> It will be applied after "mmc: dw_mmc: change to use recommended reset procedure"
>>
>> Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
>>
>> Thanks,
>> Seungwon Jeon
>
> I saw that Ulf applied "mmc: dw_mmc: change to use recommended reset
> procedure". Could we apply this one now, too? Do you want me to
> repost?
It's good that it will be merged with it.
Best Regards,
Jaehoon Chung
>
> -Doug
> --
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mmc: dw_mmc: Make sure we don't get stuck when we get an error
2014-08-13 13:38 ` Doug Anderson
2014-08-13 13:52 ` Jaehoon Chung
@ 2014-08-13 15:04 ` Ulf Hansson
1 sibling, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2014-08-13 15:04 UTC (permalink / raw)
To: Doug Anderson
Cc: Seungwon Jeon, Jaehoon Chung, Chris Ball, Sonny Rao,
Grant Grundler, linux-samsung-soc, Kukjin Kim, sunil joshi,
Tomasz Figa, Yuvaraj Kumar C D, Chris Ball, linux-mmc,
linux-kernel
On 13 August 2014 15:38, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Wed, May 21, 2014 at 2:08 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> On Wed, May 21, 2014, Doug Anderson wrote:
>>> If we happened to get a data error at just the wrong time the dw_mmc
>>> driver could get into a state where it would never complete its
>>> request. That would leave the caller just hanging there.
>>>
>>> We fix this two ways and both of the two fixes on their own appear to
>>> fix the problems we've seen:
>>>
>>> 1. Fix a race in the tasklet where the interrupt setting the data
>>> error happens _just after_ we check for it, then we get a
>>> EVENT_XFER_COMPLETE. We fix this by repeating a bit of code.
>>> 2. Fix it so that if we detect that we've got an error in the "data
>>> busy" state and we're not going to do anything else we end the
>>> request and unblock anyone waiting.
>>>
>>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>>> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@gmail.com>
>>
>> It will be applied after "mmc: dw_mmc: change to use recommended reset procedure"
>>
>> Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
>>
>> Thanks,
>> Seungwon Jeon
>
> I saw that Ulf applied "mmc: dw_mmc: change to use recommended reset
> procedure". Could we apply this one now, too? Do you want me to
> repost?
Please repost and rebase if needed.
Kind regards
Uffe
>
> -Doug
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-13 15:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-20 22:07 [PATCH v2] mmc: dw_mmc: Make sure we don't get stuck when we get an error Doug Anderson
2014-05-21 9:08 ` Seungwon Jeon
2014-07-27 14:15 ` Seungwon Jeon
2014-08-13 13:38 ` Doug Anderson
2014-08-13 13:52 ` Jaehoon Chung
2014-08-13 15:04 ` Ulf Hansson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.