diff mbox

mmc: mmci: Fixup and cleanup code for DMA handling

Message ID 1350056012-18625-1-git-send-email-ulf.hansson@stericsson.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ulf Hansson Oct. 12, 2012, 3:33 p.m. UTC
From: Ulf Hansson <ulf.hansson@linaro.org>

The cookie is now used to indicate if dma_unmap_sg shall be
done in post_request. At DMA errors, the DMA job is immediately
not only terminated but also unmapped. To indicate that this
has been done the cookie is reset to zero. post_request will
thus only do dma_umap_sg for requests which has a cookie not set
to zero.

Some corresponding duplicated code could then be removed and
moreover some corrections at DMA errors for terminating the same
DMA job twice has also been fixed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Per Forlin <per.forlin@stericsson.com>
---

I realize that this patch is quite extensive to review, but I could not
manage to simplify it further than this. Sorry for that.

Moreover you require the below patch, recently sent to the mmc-list as well,
be able to apply it.

mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant

---
 drivers/mmc/host/mmci.c |  167 ++++++++++++++++++++++++-----------------------
 1 file changed, 85 insertions(+), 82 deletions(-)

Comments

Linus Walleij Oct. 12, 2012, 9:32 p.m. UTC | #1
On Fri, Oct 12, 2012 at 5:33 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:

> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> The cookie is now used to indicate if dma_unmap_sg shall be
> done in post_request. At DMA errors, the DMA job is immediately
> not only terminated but also unmapped. To indicate that this
> has been done the cookie is reset to zero. post_request will
> thus only do dma_umap_sg for requests which has a cookie not set
> to zero.
>
> Some corresponding duplicated code could then be removed and
> moreover some corrections at DMA errors for terminating the same
> DMA job twice has also been fixed.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Per Forlin <per.forlin@stericsson.com>

It looks like it's both factoring out code and also adding some unmapping
in hithereto unhandled cases, correct? It looks OK to me now atleast so
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Ulf Hansson Oct. 16, 2012, 7:16 a.m. UTC | #2
Hi Linus,

On 12 October 2012 23:32, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Oct 12, 2012 at 5:33 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:
>
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> The cookie is now used to indicate if dma_unmap_sg shall be
>> done in post_request. At DMA errors, the DMA job is immediately
>> not only terminated but also unmapped. To indicate that this
>> has been done the cookie is reset to zero. post_request will
>> thus only do dma_umap_sg for requests which has a cookie not set
>> to zero.
>>
>> Some corresponding duplicated code could then be removed and
>> moreover some corrections at DMA errors for terminating the same
>> DMA job twice has also been fixed.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Per Forlin <per.forlin@stericsson.com>
>
> It looks like it's both factoring out code and also adding some unmapping
> in hithereto unhandled cases, correct? It looks OK to me now atleast so
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

This code has not been tested on a "legacy" ARM PL180 but only for
ux500 boards. Even if it should affect DMA handling we should test
this properly. Would be great if you were able to help out, I guess
you still have available hardware for these tests?

Kind regards
Ulf Hansson
Linus Walleij Oct. 16, 2012, 8:17 a.m. UTC | #3
On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> This code has not been tested on a "legacy" ARM PL180 but only for
> ux500 boards. Even if it should affect DMA handling we should test
> this properly. Would be great if you were able to help out, I guess
> you still have available hardware for these tests?

I can test the Integrator/CP and Realview in IRQ/PIO mode and
the U300 in DMA mode.

I need some help to provoke errorpath in DMA mode though,
so any hints are welcome...

Yours,
Linus Walleij
Ulf Hansson Oct. 16, 2012, 11:44 a.m. UTC | #4
On 16 October 2012 10:17, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
>> This code has not been tested on a "legacy" ARM PL180 but only for
>> ux500 boards. Even if it should affect DMA handling we should test
>> this properly. Would be great if you were able to help out, I guess
>> you still have available hardware for these tests?
>
> I can test the Integrator/CP and Realview in IRQ/PIO mode and
> the U300 in DMA mode.

Great!

>
> I need some help to provoke errorpath in DMA mode though,
> so any hints are welcome...

One good option could be to stress test card insert/removal sequences,
while at the same time doing read/write requests.

>
> Yours,
> Linus Walleij

Kind regards
Ulf Hansson
Ulf Hansson Nov. 21, 2012, 3:22 p.m. UTC | #5
On 16 October 2012 13:44, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 16 October 2012 10:17, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>>> This code has not been tested on a "legacy" ARM PL180 but only for
>>> ux500 boards. Even if it should affect DMA handling we should test
>>> this properly. Would be great if you were able to help out, I guess
>>> you still have available hardware for these tests?
>>
>> I can test the Integrator/CP and Realview in IRQ/PIO mode and
>> the U300 in DMA mode.
>
> Great!
>
>>
>> I need some help to provoke errorpath in DMA mode though,
>> so any hints are welcome...
>
> One good option could be to stress test card insert/removal sequences,
> while at the same time doing read/write requests.
>
>>
>> Yours,
>> Linus Walleij
>
> Kind regards
> Ulf Hansson

Just a kind reminder for Linus; did you manage to do some tests for
these patches?

Kind regards
Ulf Hansson
Ulf Hansson Nov. 28, 2012, 2 p.m. UTC | #6
On 21 November 2012 16:22, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 16 October 2012 13:44, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 16 October 2012 10:17, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>
>>>> This code has not been tested on a "legacy" ARM PL180 but only for
>>>> ux500 boards. Even if it should affect DMA handling we should test
>>>> this properly. Would be great if you were able to help out, I guess
>>>> you still have available hardware for these tests?
>>>
>>> I can test the Integrator/CP and Realview in IRQ/PIO mode and
>>> the U300 in DMA mode.
>>
>> Great!
>>
>>>
>>> I need some help to provoke errorpath in DMA mode though,
>>> so any hints are welcome...
>>
>> One good option could be to stress test card insert/removal sequences,
>> while at the same time doing read/write requests.
>>
>>>
>>> Yours,
>>> Linus Walleij
>>
>> Kind regards
>> Ulf Hansson
>
> Just a kind reminder for Linus; did you manage to do some tests for
> these patches?
>
> Kind regards
> Ulf Hansson

I have now tested these patches for ARM-RealView PB1176. Looks good! I
stress tested card insert/remove at the same time you write/read to
the card. Error handling is stable.

Kind regards
Ulf Hansson
Ulf Hansson Dec. 13, 2012, 10:59 a.m. UTC | #7
On 28 November 2012 15:00, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 21 November 2012 16:22, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 16 October 2012 13:44, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 16 October 2012 10:17, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> On Tue, Oct 16, 2012 at 9:16 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>
>>>>> This code has not been tested on a "legacy" ARM PL180 but only for
>>>>> ux500 boards. Even if it should affect DMA handling we should test
>>>>> this properly. Would be great if you were able to help out, I guess
>>>>> you still have available hardware for these tests?
>>>>
>>>> I can test the Integrator/CP and Realview in IRQ/PIO mode and
>>>> the U300 in DMA mode.
>>>
>>> Great!
>>>
>>>>
>>>> I need some help to provoke errorpath in DMA mode though,
>>>> so any hints are welcome...
>>>
>>> One good option could be to stress test card insert/removal sequences,
>>> while at the same time doing read/write requests.
>>>
>>>>
>>>> Yours,
>>>> Linus Walleij
>>>
>>> Kind regards
>>> Ulf Hansson
>>
>> Just a kind reminder for Linus; did you manage to do some tests for
>> these patches?
>>
>> Kind regards
>> Ulf Hansson
>
> I have now tested these patches for ARM-RealView PB1176. Looks good! I
> stress tested card insert/remove at the same time you write/read to
> the card. Error handling is stable.
>
> Kind regards
> Ulf Hansson

For clarification. Skip this patch.

The next version is rebased on top of 3.7 and is thus not based on
"mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant",
which needs some rework.

Kind regards
Ulf Hansson
diff mbox

Patch

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index ca6d128..3165808 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -383,10 +383,33 @@  static inline void mmci_dma_release(struct mmci_host *host)
 	host->dma_rx_channel = host->dma_tx_channel = NULL;
 }
 
+static void mmci_dma_data_error(struct mmci_host *host)
+{
+	dev_err(mmc_dev(host->mmc), "error during DMA transfer!\n");
+	dmaengine_terminate_all(host->dma_current);
+	host->dma_current = NULL;
+	host->dma_desc_current = NULL;
+	host->data->host_cookie = 0;
+}
+
 static void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data)
 {
-	struct dma_chan *chan = host->dma_current;
+	struct dma_chan *chan;
 	enum dma_data_direction dir;
+
+	if (data->flags & MMC_DATA_READ) {
+		dir = DMA_FROM_DEVICE;
+		chan = host->dma_rx_channel;
+	} else {
+		dir = DMA_TO_DEVICE;
+		chan = host->dma_tx_channel;
+	}
+
+	dma_unmap_sg(chan->device->dev, data->sg, data->sg_len, dir);
+}
+
+static void mmci_dma_finalize(struct mmci_host *host, struct mmc_data *data)
+{
 	u32 status;
 	int i;
 
@@ -405,19 +428,12 @@  static void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data)
 	 * contiguous buffers.  On TX, we'll get a FIFO underrun error.
 	 */
 	if (status & MCI_RXDATAAVLBLMASK) {
-		dmaengine_terminate_all(chan);
-		if (!data->error)
-			data->error = -EIO;
-	}
-
-	if (data->flags & MMC_DATA_WRITE) {
-		dir = DMA_TO_DEVICE;
-	} else {
-		dir = DMA_FROM_DEVICE;
+		data->error = -EIO;
+		mmci_dma_data_error(host);
 	}
 
 	if (!data->host_cookie)
-		dma_unmap_sg(chan->device->dev, data->sg, data->sg_len, dir);
+		mmci_dma_unmap(host, data);
 
 	/*
 	 * Use of DMA with scatter-gather is impossible.
@@ -427,16 +443,15 @@  static void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data)
 		dev_err(mmc_dev(host->mmc), "buggy DMA detected. Taking evasive action.\n");
 		mmci_dma_release(host);
 	}
-}
 
-static void mmci_dma_data_error(struct mmci_host *host)
-{
-	dev_err(mmc_dev(host->mmc), "error during DMA transfer!\n");
-	dmaengine_terminate_all(host->dma_current);
+	host->dma_current = NULL;
+	host->dma_desc_current = NULL;
 }
 
-static int mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data,
-			      struct mmci_host_next *next)
+/* prepares DMA channel and DMA descriptor, returns non-zero on failure */
+static int __mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data,
+				struct dma_chan **dma_chan,
+				struct dma_async_tx_descriptor **dma_desc)
 {
 	struct variant_data *variant = host->variant;
 	struct dma_slave_config conf = {
@@ -454,16 +469,6 @@  static int mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data,
 	enum dma_data_direction buffer_dirn;
 	int nr_sg;
 
-	/* Check if next job is already prepared */
-	if (data->host_cookie && !next &&
-	    host->dma_current && host->dma_desc_current)
-		return 0;
-
-	if (!next) {
-		host->dma_current = NULL;
-		host->dma_desc_current = NULL;
-	}
-
 	if (data->flags & MMC_DATA_READ) {
 		conf.direction = DMA_DEV_TO_MEM;
 		buffer_dirn = DMA_FROM_DEVICE;
@@ -497,30 +502,42 @@  static int mmci_dma_prep_data(struct mmci_host *host, struct mmc_data *data,
 	if (!desc)
 		goto unmap_exit;
 
-	if (next) {
-		next->dma_chan = chan;
-		next->dma_desc = desc;
-	} else {
-		host->dma_current = chan;
-		host->dma_desc_current = desc;
-	}
+	*dma_chan = chan;
+	*dma_desc = desc;
 
 	return 0;
 
  unmap_exit:
-	if (!next)
-		dmaengine_terminate_all(chan);
 	dma_unmap_sg(device->dev, data->sg, data->sg_len, buffer_dirn);
 	return -ENOMEM;
 }
 
+static inline int mmci_dma_prep_data(struct mmci_host *host,
+				     struct mmc_data *data)
+{
+	/* Check if next job is already prepared. */
+	if (host->dma_current && host->dma_desc_current)
+		return 0;
+
+	/* No job were prepared thus do it now. */
+	return __mmci_dma_prep_data(host, data, &host->dma_current,
+				    &host->dma_desc_current);
+}
+
+static inline int mmci_dma_prep_next(struct mmci_host *host,
+				     struct mmc_data *data)
+{
+	struct mmci_host_next *nd = &host->next_data;
+	return __mmci_dma_prep_data(host, data, &nd->dma_chan, &nd->dma_desc);
+}
+
 static int mmci_dma_start_data(struct mmci_host *host, unsigned int datactrl)
 {
 	int ret;
 	struct mmc_data *data = host->data;
 	struct variant_data *variant = host->variant;
 
-	ret = mmci_dma_prep_data(host, host->data, NULL);
+	ret = mmci_dma_prep_data(host, host->data);
 	if (ret)
 		return ret;
 
@@ -555,19 +572,11 @@  static void mmci_get_next_data(struct mmci_host *host, struct mmc_data *data)
 {
 	struct mmci_host_next *next = &host->next_data;
 
-	if (data->host_cookie && data->host_cookie != next->cookie) {
-		pr_warning("[%s] invalid cookie: data->host_cookie %d"
-		       " host->next_data.cookie %d\n",
-		       __func__, data->host_cookie, host->next_data.cookie);
-		data->host_cookie = 0;
-	}
-
-	if (!data->host_cookie)
-		return;
+	WARN_ON(data->host_cookie && data->host_cookie != next->cookie);
+	WARN_ON(!data->host_cookie && (next->dma_desc || next->dma_chan));
 
 	host->dma_desc_current = next->dma_desc;
 	host->dma_current = next->dma_chan;
-
 	next->dma_desc = NULL;
 	next->dma_chan = NULL;
 }
@@ -582,22 +591,13 @@  static void mmci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq,
 	if (!data)
 		return;
 
-	if (mmci_validate_data(host, mrq->data))
-		return;
+	BUG_ON(data->host_cookie);
 
-	if (data->host_cookie) {
-		data->host_cookie = 0;
+	if (mmci_validate_data(host, data))
 		return;
-	}
 
-	/* if config for dma */
-	if (((data->flags & MMC_DATA_WRITE) && host->dma_tx_channel) ||
-	    ((data->flags & MMC_DATA_READ) && host->dma_rx_channel)) {
-		if (mmci_dma_prep_data(host, data, nd))
-			data->host_cookie = 0;
-		else
-			data->host_cookie = ++nd->cookie < 0 ? 1 : nd->cookie;
-	}
+	if (!mmci_dma_prep_next(host, data))
+		data->host_cookie = ++nd->cookie < 0 ? 1 : nd->cookie;
 }
 
 static void mmci_post_request(struct mmc_host *mmc, struct mmc_request *mrq,
@@ -605,29 +605,23 @@  static void mmci_post_request(struct mmc_host *mmc, struct mmc_request *mrq,
 {
 	struct mmci_host *host = mmc_priv(mmc);
 	struct mmc_data *data = mrq->data;
-	struct dma_chan *chan;
-	enum dma_data_direction dir;
 
-	if (!data)
+	if (!data || !data->host_cookie)
 		return;
 
-	if (data->flags & MMC_DATA_READ) {
-		dir = DMA_FROM_DEVICE;
-		chan = host->dma_rx_channel;
-	} else {
-		dir = DMA_TO_DEVICE;
-		chan = host->dma_tx_channel;
-	}
+	mmci_dma_unmap(host, data);
 
+	if (err) {
+		struct mmci_host_next *next = &host->next_data;
+		struct dma_chan *chan;
+		if (data->flags & MMC_DATA_READ)
+			chan = host->dma_rx_channel;
+		else
+			chan = host->dma_tx_channel;
+		dmaengine_terminate_all(chan);
 
-	/* if config for dma */
-	if (chan) {
-		if (err)
-			dmaengine_terminate_all(chan);
-		if (data->host_cookie)
-			dma_unmap_sg(mmc_dev(host->mmc), data->sg,
-				     data->sg_len, dir);
-		mrq->data->host_cookie = 0;
+		next->dma_desc = NULL;
+		next->dma_chan = NULL;
 	}
 }
 
@@ -648,6 +642,11 @@  static inline void mmci_dma_unmap(struct mmci_host *host, struct mmc_data *data)
 {
 }
 
+static inline void mmci_dma_finalize(struct mmci_host *host,
+				     struct mmc_data *data)
+{
+}
+
 static inline void mmci_dma_data_error(struct mmci_host *host)
 {
 }
@@ -772,8 +771,10 @@  mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		u32 remain, success;
 
 		/* Terminate the DMA transfer */
-		if (dma_inprogress(host))
+		if (dma_inprogress(host)) {
 			mmci_dma_data_error(host);
+			mmci_dma_unmap(host, data);
+		}
 
 		/*
 		 * Calculate how far we are into the transfer.  Note that
@@ -812,7 +813,7 @@  mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 
 	if (status & MCI_DATAEND || data->error) {
 		if (dma_inprogress(host))
-			mmci_dma_unmap(host, data);
+			mmci_dma_finalize(host, data);
 		mmci_stop_data(host);
 
 		if (!data->error)
@@ -849,8 +850,10 @@  mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
 	if (!cmd->data || cmd->error) {
 		if (host->data) {
 			/* Terminate the DMA transfer */
-			if (dma_inprogress(host))
+			if (dma_inprogress(host)) {
 				mmci_dma_data_error(host);
+				mmci_dma_unmap(host, host->data);
+			}
 			mmci_stop_data(host);
 		}
 		mmci_request_end(host, cmd->mrq);