Re: [PATCH] RFC: mmc: dw_mmc: Always go to STATE_DATA_BUSY from STATE_DATA_ERROR

Doug Anderson
2013-04-08T23:09:41Z

Seungwon,

On Mon, Apr 8, 2013 at 5:17 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> I guess Doug are debugging it with wifi, right?

Yes, we're debugging it on the Samsung ARM Chromebook on a part that
has an SDIO WiFi module by Marvell. Bing Zhao (CCed) has a unit in
hand that generates lots of CRC errors and has been testing patches
I've sent him.

> The problem happens when dw_mci_stop_dma is called in the middle of data transfers.
> If data error occurs in the end of block, EVENT_XFER_COMPLETE might be set. So, it's fine.
> Actually, dw_mci_idmac_stop_dma stops the dma working, there is no further interrupt for dma completion.

That sounds right to me.

> There are two solutions we have applied.

I'm a little confused. Have you already applied one or both of the
solutions you list below, or are you proposing them as alternates to
the patch I submitted?

> #1. deferring the call of dw_mci_stop_dma until EVENT_XFER_COMPLETE flag is set into pending_events.
> In this case, dma transfer will be continued with error.
>
> @@ -1062,7 +1062,6 @@ static void dw_mci_tasklet_func(unsigned long priv)
> case STATE_SENDING_DATA:
> if (test_and_clear_bit(EVENT_DATA_ERROR,
> &host->pending_events)) {
> - dw_mci_stop_dma(host);
> if (data->stop)
> send_stop_cmd(host, data);
> state = STATE_DATA_ERROR;
> @@ -1155,6 +1154,9 @@ static void dw_mci_tasklet_func(unsigned long priv)
> &host->pending_events))
> break;
>
> + dw_mci_stop_dma(host);
> + set_bit(EVENT_XFER_COMPLETE, &host->completed_events);
> +
> state = STATE_DATA_BUSY;
> break;

I can't say that I'm quite familiar enough with the intricate details
of the driver to know whether this is a good idea or guaranteed to
work. Do we really think that we'll still get the end of the transfer
properly if we've seen an error already? I worry that we won't.

> #2. set EVENT_XFER_COMPLETE flag when dw_mci_stop_dma is called regardless using_dma.
>
> @@ -299,10 +299,9 @@ static void dw_mci_stop_dma(struct dw_mci *host)
> if (host->using_dma) {
> host->dma_ops->stop(host);
> host->dma_ops->cleanup(host);
> - } else {
> - /* Data transfer was stopped by the interrupt handler */
> - set_bit(EVENT_XFER_COMPLETE, &host->pending_events);
> }
> +
> + set_bit(EVENT_XFER_COMPLETE, &host->pending_events);
> }

This is fairly similar to my patch but goes further. I believe my
patch has this effect but only for the call to dw_mci_stop_dma() in
STATE_SENDING_DATA in the tasklet. Your affects all 3 calls to
dw_mci_stop_dma().

This seems reasonable but I don't have confidence in my understanding
of this driver's state machine (especially with regards to the error
conditions) that I can say which is better. If you think that this is
a more correct solution than mine then we can give it some testing.

Thanks!

-Doug