diff mbox

[V3] mmc: mmci: Fixup and cleanup code for DMA handling

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

Commit Message

Ulf Hansson Jan. 7, 2013, 2:58 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>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---

This patch has been tested both on a u8500 board and an ARM-RealView
PB1176 board.

Changes in v3:
	Add ack from Linus Walleij.
	Rebased on top of Linux 3.8-rc2

Changes in v2:
	Rebased patch on top of Linux 3.7

---
 drivers/mmc/host/mmci.c |  190 ++++++++++++++++++++++++++---------------------
 1 file changed, 106 insertions(+), 84 deletions(-)

Comments

Russell King - ARM Linux Jan. 13, 2013, 7:24 p.m. UTC | #1 
On Mon, Jan 07, 2013 at 03:58:27PM +0100, Ulf Hansson wrote:
> @@ -374,19 +415,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);

Please explain the change of behaviour here.  Before your change, we _only_
set data->error if the error is not set.  Here, we overwrite the error code
no matter what.  What is the reasoning for that change?

The reason the code is like it _was_ is so that any bytes remaining in the
FIFO are _only_ reported as an error if there wasn't a preceding error.
That is the behaviour I desired when I wrote this code.
Ulf Hansson Jan. 16, 2013, 10:23 a.m. UTC | #2 
On 13 January 2013 20:24, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 07, 2013 at 03:58:27PM +0100, Ulf Hansson wrote:
>> @@ -374,19 +415,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);
>
> Please explain the change of behaviour here.  Before your change, we _only_
> set data->error if the error is not set.  Here, we overwrite the error code
> no matter what.  What is the reasoning for that change?
>
> The reason the code is like it _was_ is so that any bytes remaining in the
> FIFO are _only_ reported as an error if there wasn't a preceding error.
> That is the behaviour I desired when I wrote this code.

Since we need to do dmaengine_terminate_all(chan), that will mean
another request could potentially already be prepared and thus it
could also be terminated.

Then by always reporting an error the async request handling in the
mmc protocol layer, can do proper error handling and clean up the
previously prepared request.

Kind regards
Ulf Hansson
Russell King - ARM Linux Jan. 16, 2013, 10:49 a.m. UTC | #3 
On Wed, Jan 16, 2013 at 11:23:57AM +0100, Ulf Hansson wrote:
> On 13 January 2013 20:24, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jan 07, 2013 at 03:58:27PM +0100, Ulf Hansson wrote:
> >> @@ -374,19 +415,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);
> >
> > Please explain the change of behaviour here.  Before your change, we _only_
> > set data->error if the error is not set.  Here, we overwrite the error code
> > no matter what.  What is the reasoning for that change?
> >
> > The reason the code is like it _was_ is so that any bytes remaining in the
> > FIFO are _only_ reported as an error if there wasn't a preceding error.
> > That is the behaviour I desired when I wrote this code.
> 
> Since we need to do dmaengine_terminate_all(chan), that will mean
> another request could potentially already be prepared and thus it
> could also be terminated.
> 
> Then by always reporting an error the async request handling in the
> mmc protocol layer, can do proper error handling and clean up the
> previously prepared request.

Sigh, I don't think you understand what this code is doing at all then.

We will _always_ report _an_ error as the code originally stands if we
encounter MCI_RXDATAAVLBLMASK being set.  The error that we report will
be the _correct_ one - either the one which preceeds this condition
occuring _or_ an IO error because not all data was read from the FIFO
by the DMA engine.

If the DMA engine fails to read all the data from the FIFO, _and_ there
was a preceeding error, we should _not_ overwrite the preceeding error.
This is exactly what the original code does.

Your version _always_ overwrites the previous error.  This could result
in FIFO/CRC errors always being reported as an IO error rather than
their proper error codes, and therefore _breaking_ error handling.
Ulf Hansson Jan. 16, 2013, 11:01 a.m. UTC | #4 
On 16 January 2013 11:49, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jan 16, 2013 at 11:23:57AM +0100, Ulf Hansson wrote:
>> On 13 January 2013 20:24, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Jan 07, 2013 at 03:58:27PM +0100, Ulf Hansson wrote:
>> >> @@ -374,19 +415,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);
>> >
>> > Please explain the change of behaviour here.  Before your change, we _only_
>> > set data->error if the error is not set.  Here, we overwrite the error code
>> > no matter what.  What is the reasoning for that change?
>> >
>> > The reason the code is like it _was_ is so that any bytes remaining in the
>> > FIFO are _only_ reported as an error if there wasn't a preceding error.
>> > That is the behaviour I desired when I wrote this code.
>>
>> Since we need to do dmaengine_terminate_all(chan), that will mean
>> another request could potentially already be prepared and thus it
>> could also be terminated.
>>
>> Then by always reporting an error the async request handling in the
>> mmc protocol layer, can do proper error handling and clean up the
>> previously prepared request.
>
> Sigh, I don't think you understand what this code is doing at all then.
>
> We will _always_ report _an_ error as the code originally stands if we
> encounter MCI_RXDATAAVLBLMASK being set.  The error that we report will
> be the _correct_ one - either the one which preceeds this condition
> occuring _or_ an IO error because not all data was read from the FIFO
> by the DMA engine.
>
> If the DMA engine fails to read all the data from the FIFO, _and_ there
> was a preceeding error, we should _not_ overwrite the preceeding error.
> This is exactly what the original code does.
>
> Your version _always_ overwrites the previous error.  This could result
> in FIFO/CRC errors always being reported as an IO error rather than
> their proper error codes, and therefore _breaking_ error handling.

Sorry, Russell I was talking junk. :-) Please ignore my last respond.

It shall be safe to keep the original behavior. The important thing is
that an error code get set, so the async request handling can clean up
a prepared request, which was already handled.
I will fixup the patch and send a v4, thanks a lot spotting this.

Kind regards
Ulf Hansson
diff mbox

Patch

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 1507723..0baa37b 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -134,6 +134,24 @@  static struct variant_data variant_ux500v2 = {
 };
 
 /*
+ * Validate mmc prerequisites
+ */
+static int mmci_validate_data(struct mmci_host *host,
+			      struct mmc_data *data)
+{
+	if (!data)
+		return 0;
+
+	if (!is_power_of_2(data->blksz)) {
+		dev_err(mmc_dev(host->mmc),
+			"unsupported block size (%d bytes)\n", data->blksz);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
  * This must be called with host->lock held
  */
 static void mmci_write_clkreg(struct mmci_host *host, u32 clk)
@@ -352,10 +370,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;
 
@@ -374,19 +415,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.
@@ -396,16 +430,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 = {
@@ -423,16 +456,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;
@@ -462,29 +485,41 @@  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;
 
-	ret = mmci_dma_prep_data(host, host->data, NULL);
+	ret = mmci_dma_prep_data(host, host->data);
 	if (ret)
 		return ret;
 
@@ -514,19 +549,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;
 }
@@ -541,19 +568,13 @@  static void mmci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq,
 	if (!data)
 		return;
 
-	if (data->host_cookie) {
-		data->host_cookie = 0;
+	BUG_ON(data->host_cookie);
+
+	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,
@@ -561,29 +582,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;
 	}
 }
 
@@ -604,6 +619,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)
 {
 }
@@ -751,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
@@ -791,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)
@@ -828,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);
@@ -1055,10 +1079,8 @@  static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	WARN_ON(host->mrq != NULL);
 
-	if (mrq->data && !is_power_of_2(mrq->data->blksz)) {
-		dev_err(mmc_dev(mmc), "unsupported block size (%d bytes)\n",
-			mrq->data->blksz);
-		mrq->cmd->error = -EINVAL;
+	mrq->cmd->error = mmci_validate_data(host, mrq->data);
+	if (mrq->cmd->error) {
 		mmc_request_done(mmc, mrq);
 		return;
 	}