[PATCH] mmci: calculate remaining bytes at error correctly

[PATCH] mmci: calculate remaining bytes at error correctly

[Linus Walleij]

The MMCIDATACNT register contain the number of byte left at error
not the number of words, so loose the << 2 thing. Further if CRC
fails on the first block, we may end up with a negative number
of transferred bytes which is not good, and the formula was in
wrong order.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/mmc/host/mmci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 2de12fe..1870e74 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -283,13 +283,13 @@ mmci_data_irq(struct mmci_host *host, struct
mmc_data *data,
                u32 remain, success;

                /* Calculate how far we are into the transfer */
-               remain =3D readl(host->base + MMCIDATACNT) << 2;
+               remain =3D readl(host->base + MMCIDATACNT);
                success =3D data->blksz * data->blocks - remain;

                dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ (status
%08x)\n", status);
                if (status & MCI_DATACRCFAIL) {
                        /* Last block was not successful */
-                       host->data_xfered =3D ((success / data->blksz) -
1 * data->blksz);
+                       host->data_xfered =3D ((success - 1) /
data->blksz) * data->blksz;
                        data->error =3D -EILSEQ;
                } else if (status & MCI_DATATIMEOUT) {
                        host->data_xfered =3D success;
--=20
1.7.3.2

I'll put it in the patch tracker for consideration...

Thanks
Linus Walleij

[PATCH] mmci: calculate remaining bytes at error correctly

[Linus Walleij]

The MMCIDATACNT register contain the number of byte left at error
not the number of words, so loose the << 2 thing. Further if CRC
fails on the first block, we may end up with a negative number
of transferred bytes which is not good, and the formula was in
wrong order.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 drivers/mmc/host/mmci.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 2de12fe..ec2db8c 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -283,13 +283,16 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		u32 remain, success;
 
 		/* Calculate how far we are into the transfer */
-		remain = readl(host->base + MMCIDATACNT) << 2;
+		remain = readl(host->base + MMCIDATACNT);
 		success = data->blksz * data->blocks - remain;
 
 		dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ (status %08x)\n", status);
 		if (status & MCI_DATACRCFAIL) {
 			/* Last block was not successful */
-			host->data_xfered = ((success / data->blksz) - 1 * data->blksz);
+			if (success <= data->blksz)
+				host->data_xfered = 0;
+			else
+				host->data_xfered = ((success / data->blksz) - 1) * data->blksz;
 			data->error = -EILSEQ;
 		} else if (status & MCI_DATATIMEOUT) {
 			host->data_xfered = success;
-- 
1.7.3.2

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Thu, Jan 27, 2011 at 03:14:26PM +0100, Linus Walleij wrote:
> The MMCIDATACNT register contain the number of byte left at error
> not the number of words, so loose the << 2 thing. Further if CRC
> fails on the first block, we may end up with a negative number
> of transferred bytes which is not good, and the formula was in
> wrong order.

Hmm.

In order to have a CRC error, you must have transferred one block through
the FIFO.  We might read the data counter before one block has been fully
transferred.

So, maybe we want:

	host->data_xfered = ((success - 1) / data->blksz) * data->blksz;

instead?

[PATCH] mmci: calculate remaining bytes at error correctly

[Linus Walleij]

2011/1/27 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> So, maybe we want:
>
> ? ? ? ?host->data_xfered = ((success - 1) / data->blksz) * data->blksz;
>
> instead?

So like this:

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Thu, Jan 27, 2011 at 05:43:35PM +0100, Linus Walleij wrote:
> 2011/1/27 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> 
> > So, maybe we want:
> >
> > ? ? ? ?host->data_xfered = ((success - 1) / data->blksz) * data->blksz;
> >
> > instead?
> 
> So like this:

Yes, that looks right, and shouldn't ever underflow.

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

Linus,

Here's another couple of fixes...

8<----
Subject: [PATCH 1/2] ARM: mmci: complete the transaction on error

When we encounter an error, make sure we complete the transaction
otherwise we'll leave the request dangling.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/mmci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index b6fd6dc..175a623 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -319,7 +319,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 	if (status & MCI_DATABLOCKEND)
 		dev_err(mmc_dev(host->mmc), "stray MCI_DATABLOCKEND interrupt\n");
 
-	if (status & MCI_DATAEND) {
+	if (status & MCI_DATAEND || data->error) {
 		mmci_stop_data(host);
 
 		if (!data->error)
-- 
1.6.2.5

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Sun, Jan 30, 2011 at 09:28:56PM +0000, Russell King - ARM Linux wrote:
> Linus,
> 
> Here's another couple of fixes...
8<-----
Subject: [PATCH 2/2] ARM: mmci: round down the bytes transferred on error

We should not report incomplete blocks on error.  Return the number of
bytes successfully transferred, rounded down to the nearest block.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/mmci.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 175a623..0de1602 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -289,13 +289,13 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ (status %08x)\n", status);
 		if (status & MCI_DATACRCFAIL) {
 			/* Last block was not successful */
-			host->data_xfered = ((success - 1) / data->blksz) * data->blksz;
+			host->data_xfered = (success - 1) & ~(data->blksz - 1);
 			data->error = -EILSEQ;
 		} else if (status & MCI_DATATIMEOUT) {
-			host->data_xfered = success;
+			host->data_xfered = success & ~(data->blksz - 1);
 			data->error = -ETIMEDOUT;
 		} else if (status & (MCI_TXUNDERRUN|MCI_RXOVERRUN)) {
-			host->data_xfered = success;
+			host->data_xfered = success & ~(data->blksz - 1);
 			data->error = -EIO;
 		}
 
-- 
1.6.2.5

[PATCH] mmci: calculate remaining bytes at error correctly

[Linus Walleij]

2011/1/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> 8<-----
> Subject: [PATCH 2/2] ARM: mmci: round down the bytes transferred on error
>
> We should not report incomplete blocks on error. ?Return the number of
> bytes successfully transferred, rounded down to the nearest block.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> ?drivers/mmc/host/mmci.c | ? ?6 +++---
> ?1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 175a623..0de1602 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -289,13 +289,13 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
> ? ? ? ? ? ? ? ?dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ (status %08x)\n", status);
> ? ? ? ? ? ? ? ?if (status & MCI_DATACRCFAIL) {
> ? ? ? ? ? ? ? ? ? ? ? ?/* Last block was not successful */
> - ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = ((success - 1) / data->blksz) * data->blksz;
> + ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = (success - 1) & ~(data->blksz - 1);

Looks equivalent to using the macro round_down() from
<linux/kernel.h> like this:

host->data_xfered = round_down((success - 1), data->blksz);

Maybe that adds a bit to readability...

> ? ? ? ? ? ? ? ? ? ? ? ?data->error = -EILSEQ;
> ? ? ? ? ? ? ? ?} else if (status & MCI_DATATIMEOUT) {
> - ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = success;
> + ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = success & ~(data->blksz - 1);
> ? ? ? ? ? ? ? ? ? ? ? ?data->error = -ETIMEDOUT;
> ? ? ? ? ? ? ? ?} else if (status & (MCI_TXUNDERRUN|MCI_RXOVERRUN)) {
> - ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = success;
> + ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = success & ~(data->blksz - 1);
> ? ? ? ? ? ? ? ? ? ? ? ?data->error = -EIO;
> ? ? ? ? ? ? ? ?}

Dito on both but just
round_down(success, data->blksz);

Either way it is correct so:
Acked-by: Linus Walleij <linus.walleij@stericsson.com>

Yours,
Linus Walleij

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Mon, Jan 31, 2011 at 11:24:09AM +0100, Linus Walleij wrote:
> 2011/1/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > ? ? ? ? ? ? ? ? ? ? ? ?data->error = -EILSEQ;
> > ? ? ? ? ? ? ? ?} else if (status & MCI_DATATIMEOUT) {
> > - ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = success;
> > + ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = success & ~(data->blksz - 1);
> > ? ? ? ? ? ? ? ? ? ? ? ?data->error = -ETIMEDOUT;
> > ? ? ? ? ? ? ? ?} else if (status & (MCI_TXUNDERRUN|MCI_RXOVERRUN)) {
> > - ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = success;
> > + ? ? ? ? ? ? ? ? ? ? ? host->data_xfered = success & ~(data->blksz - 1);
> > ? ? ? ? ? ? ? ? ? ? ? ?data->error = -EIO;
> > ? ? ? ? ? ? ? ?}

Grr, we still haven't got this quite right.  If I make the MMC block
layer believe data_xfered bytes were transfered successfully, then
I see corruption with FIFO errors:

-00430bf0  71 28 10 81 5b 75 ea b7  4c 3c 35 de 8f 9a ee f9  |q(..[u..L<5.....|
+00430bf0  71 28 10 81 5b 75 ea b7  4c 3c 35 de c4 61 e5 69  |q(..[u..L<5..a.i|

Block #1072 -> Part. #302150 -> Phys Sect #2417263..241771 -> 2417268 in error
mmcblk0: error -5 transferring data, sector 2417269, nr 106, cmd response 0x900, card status 0xb00

-005b39f0  2b d5 e1 e1 f6 c6 6d a8  9e 9e a8 47 a6 cb 80 ea  |+.....m....G....|
+005b39f0  0e 05 01 cd e8 4d ca 84  ce 45 9f ae 8b 4d 91 84  |.....M...E...M..|

Block #1459 -> Part. #302537 -> Phys Sect #2420359..2420367 -> 2420363 in error
mmcblk0: error -5 transferring data, sector 2420364, nr 83, cmd response 0x900, card status 0xb00

-009ddbd0  bb 24 ea e7 97 cb c7 40  74 83 c0 da 07 7f ef aa  |.$..... at t.......|
-009ddbe0  04 22 ff 01 0f 81 72 e0  62 47 4d 3e 2b fa bf 2a  |."....r.bGM>+..*|
-009ddbf0  bf f4 97 29 d9 47 fd 1e  2a 03 7c ad 5e 42 5d ec  |...).G..*.|.^B].|
+009ddbd0  45 45 46 47 49 4b 4b 4c  4b 4b 4c 4d 4e 4f 4f 50  |EEFGIKKLKKLMNOOP|
+009ddbe0  54 57 5b 5e 61 64 6a 6e  74 77 7d 85 86 8c 92 97  |TW[^adjntw}.....| 
+009ddbf0  99 9e a2 a5 aa ad af b1  b1 b3 b4 b4 b3 b3 b2 b1  |................| 

Block #2525 -> Part. #303604 -> Phys Sect #2428895..2428903 -> 2428900 in error
mmcblk0: error -5 transferring data, sector 2428901, nr 2, cmd response 0x900, card status 0xb00

-00e4d5f0  49 f8 f8 bd 54 fa 8d 9c  d6 b7 0d 7a c1 d1 70 1f  |I...T......z..p.|
+00e4d5f0  49 f8 f8 bd 54 fa 8d 9c  73 75 70 6a 65 61 5d 4d  |I...T...supjea]M|

Block #3661 -> Part. #304741 -> Phys Sect #2437991..2437999 -> 2437993 in error
mmcblk0: error -5 transferring data, sector 2437994, nr 13, cmd response 0x900, card status 0xb00 

Notice how the sector number in the kernel reports are for the following
sector, and also notice that the corruption is within the last FIFO-worth
of data.

Most of the time it seems to blame the correct sector, but occasionally,
when the overflow happens towards the end of a block, it doesn't.

It looks to me like the data counter decrements according to bytes
transferred on the MMC bus.  When a FIFO overrun error occurs, we
immediately handle this, disable the data path, and that prevents the
remaining good FIFO data being read.  So, the corruption represented
above is because we haven't read the pending data from the FIFO.

In the vast majority of cases, it seems there's little point as it won't
get us up to a block boundary, so maybe the correct answer is on overruns
to subtract the FIFO size, but not for underruns.  So something like the
below.  I'll give it a spin tomorrow.

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index fe7facc..ada362c 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -293,7 +293,14 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		} else if (status & MCI_DATATIMEOUT) {
 			data->bytes_xfered = round_down(success, data->blksz);
 			data->error = -ETIMEDOUT;
-		} else if (status & (MCI_TXUNDERRUN|MCI_RXOVERRUN)) {
+		} else if (status & MCI_TXUNDERRUN) {
+			data->bytes_xfered = round_down(success, data->blksz);
+			data->error = -EIO;
+		} else if (status & MCI_RXOVERRUN) {
+			if (success > host->variant->fifosize)
+				success -= host->variant->fifosize;
+			else
+				success = 0;
 			data->bytes_xfered = round_down(success, data->blksz);
 			data->error = -EIO;
 		}

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Thu, Feb 03, 2011 at 12:30:54AM +0000, Russell King - ARM Linux wrote:
> It looks to me like the data counter decrements according to bytes
> transferred on the MMC bus.  When a FIFO overrun error occurs, we
> immediately handle this, disable the data path, and that prevents the
> remaining good FIFO data being read.  So, the corruption represented
> above is because we haven't read the pending data from the FIFO.
> 
> In the vast majority of cases, it seems there's little point as it won't
> get us up to a block boundary, so maybe the correct answer is on overruns
> to subtract the FIFO size, but not for underruns.  So something like the
> below.  I'll give it a spin tomorrow.

Right, this is what I've presently got, which appears to resolve
the issue for me.  Ack?

 drivers/mmc/host/mmci.c |   26 +++++++++++++++++++-------
 1 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 577b03f..dcbe9c4 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -540,22 +540,34 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
 		if (dma_inprogress(host))
 			mmci_dma_data_error(host);
 
-		/* Calculate how far we are into the transfer */
+		/*
+		 * Calculate how far we are into the transfer.  Note that
+		 * the data counter gives the number of bytes transferred
+		 * on the MMC bus, not on the host side.  On reads, this
+		 * can be as much as a FIFO-worth of data ahead.  This
+		 * matters for FIFO overruns only.
+		 */
 		remain = readl(host->base + MMCIDATACNT);
 		success = data->blksz * data->blocks - remain;
 
-		dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ (status %08x)\n", status);
+		dev_dbg(mmc_dev(host->mmc), "MCI ERROR IRQ, status 0x%08x at 0x%08x\n",
+			status, success);
 		if (status & MCI_DATACRCFAIL) {
 			/* Last block was not successful */
-			data->bytes_xfered = round_down(success - 1, data->blksz);
+			success -= 1;
 			data->error = -EILSEQ;
 		} else if (status & MCI_DATATIMEOUT) {
-			data->bytes_xfered = round_down(success, data->blksz);
 			data->error = -ETIMEDOUT;
-		} else if (status & (MCI_TXUNDERRUN|MCI_RXOVERRUN)) {
-			data->bytes_xfered = round_down(success, data->blksz);
+		} else if (status & MCI_TXUNDERRUN) {
+			data->error = -EIO;
+		} else if (status & MCI_RXOVERRUN) {
+			if (success > host->variant->fifosize)
+				success -= host->variant->fifosize;
+			else
+				success = 0;
 			data->error = -EIO;
 		}
+		data->bytes_xfered = round_down(success, data->blksz);
 	}
 
 	if (status & MCI_DATABLOCKEND)

[PATCH] mmci: calculate remaining bytes at error correctly

[Linus Walleij]

2011/2/3 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Thu, Feb 03, 2011 at 12:30:54AM +0000, Russell King - ARM Linux wrote:
>> It looks to me like the data counter decrements according to bytes
>> transferred on the MMC bus.

Looks like that to me as well.

> Right, this is what I've presently got, which appears to resolve
> the issue for me. ?Ack?

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH] mmci: calculate remaining bytes at error correctly

[Linus Walleij]

2011/1/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> 8<----
> Subject: [PATCH 1/2] ARM: mmci: complete the transaction on error
>
> When we encounter an error, make sure we complete the transaction
> otherwise we'll leave the request dangling.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> ?drivers/mmc/host/mmci.c | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index b6fd6dc..175a623 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -319,7 +319,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
> ? ? ? ?if (status & MCI_DATABLOCKEND)
> ? ? ? ? ? ? ? ?dev_err(mmc_dev(host->mmc), "stray MCI_DATABLOCKEND interrupt\n");
>
> - ? ? ? if (status & MCI_DATAEND) {
> + ? ? ? if (status & MCI_DATAEND || data->error) {
> ? ? ? ? ? ? ? ?mmci_stop_data(host);

The hardware always sets the MCI_DATAEND bit if there is
some error, so these flags always appear simultaneously, but
it doesn't hurt to take some extra precaution, so
Acked-by: Linus Walleij <linus.walleij@stericsson.com>

Linus Walleij

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Mon, Jan 31, 2011 at 11:17:42AM +0100, Linus Walleij wrote:
> 2011/1/30 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> 
> > 8<----
> > Subject: [PATCH 1/2] ARM: mmci: complete the transaction on error
> >
> > When we encounter an error, make sure we complete the transaction
> > otherwise we'll leave the request dangling.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> > ?drivers/mmc/host/mmci.c | ? ?2 +-
> > ?1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> > index b6fd6dc..175a623 100644
> > --- a/drivers/mmc/host/mmci.c
> > +++ b/drivers/mmc/host/mmci.c
> > @@ -319,7 +319,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
> > ? ? ? ?if (status & MCI_DATABLOCKEND)
> > ? ? ? ? ? ? ? ?dev_err(mmc_dev(host->mmc), "stray MCI_DATABLOCKEND interrupt\n");
> >
> > - ? ? ? if (status & MCI_DATAEND) {
> > + ? ? ? if (status & MCI_DATAEND || data->error) {
> > ? ? ? ? ? ? ? ?mmci_stop_data(host);
> 
> The hardware always sets the MCI_DATAEND bit if there is
> some error, so these flags always appear simultaneously, but
> it doesn't hurt to take some extra precaution, so

The hardware may do, but you won't see that here.  When we setup a
transfer, we do this:

        writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);

When we receive an interrupt:

                status = readl(host->base + MMCISTATUS);
                status &= readl(host->base + MMCIMASK0);

                if (status & (MCI_DATACRCFAIL|MCI_DATATIMEOUT|MCI_TXUNDERRUN|
                              MCI_RXOVERRUN|MCI_DATAEND|MCI_DATABLOCKEND) && data)
                        mmci_data_irq(host, data, status);

When we get to the end of a transfer:

        /*
         * If we run out of data, disable the data IRQs; this
         * prevents a race where the FIFO becomes empty before
         * the chip itself has disabled the data path, and
         * stops us racing with our data end IRQ.
         */
        if (host->size == 0) {
                mmci_set_mask1(host, 0);
                writel(readl(base + MMCIMASK0) | MCI_DATAENDMASK, base + MMCIMASK0);
        }

So, we'll only see DATAEND when we actually reach the end of a transfer.
If we error out before hand, we won't see it.

[PATCH] mmci: calculate remaining bytes at error correctly

[Linus Walleij]

2011/1/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> > - ? ? ? if (status & MCI_DATAEND) {
>> > + ? ? ? if (status & MCI_DATAEND || data->error) {
>> > ? ? ? ? ? ? ? ?mmci_stop_data(host);
>>
>> The hardware always sets the MCI_DATAEND bit if there is
>> some error, so these flags always appear simultaneously, but
>> it doesn't hurt to take some extra precaution, so
>
> The hardware may do, but you won't see that here. ?When we setup a
> transfer, we do this:
>
> ? ? ? ?writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);

Ah! I get it.

We've actually played around with that, having MCI_DATAEND on all the
time, it's not necessary any more since we're not using the blockend IRQ
and they cannot race, so we can take a little bit of quirking out of it.
But I'll get back on that issue.

Thanks,
Linus Walleij

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Mon, Jan 31, 2011 at 11:31:11AM +0100, Linus Walleij wrote:
> 2011/1/31 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> >> > - ? ? ? if (status & MCI_DATAEND) {
> >> > + ? ? ? if (status & MCI_DATAEND || data->error) {
> >> > ? ? ? ? ? ? ? ?mmci_stop_data(host);
> >>
> >> The hardware always sets the MCI_DATAEND bit if there is
> >> some error, so these flags always appear simultaneously, but
> >> it doesn't hurt to take some extra precaution, so
> >
> > The hardware may do, but you won't see that here. ?When we setup a
> > transfer, we do this:
> >
> > ? ? ? ?writel(readl(base + MMCIMASK0) & ~MCI_DATAENDMASK, base + MMCIMASK0);
> 
> Ah! I get it.
> 
> We've actually played around with that, having MCI_DATAEND on all the
> time, it's not necessary any more since we're not using the blockend IRQ
> and they cannot race, so we can take a little bit of quirking out of it.
> But I'll get back on that issue.

Wrong issue.  What can happen is you receive the DATAEND interrupt,
which tears down the data side of the request, and starts the stop
command.  Meanwhile there's still data left in the FIFO for the CPU
to read.

I suspect there's a similar race between DMA and the host CPU too as
you leave the DATAEND interrupt on.  I suspect for reliability, we
need to have the DMA controller callback function in place to enable
the DATAEND interrupt.  Or something like that.

[PATCH] mmci: calculate remaining bytes at error correctly

[Linus Walleij]

On Mon, Jan 31, 2011 at 11:40 AM, Russell King - ARM Linux <
linux@arm.linux.org.uk> wrote:

What can happen is you receive the DATAEND interrupt,
> which tears down the data side of the request, and starts the stop
> command.  Meanwhile there's still data left in the FIFO for the CPU
> to read.
>
> I suspect there's a similar race between DMA and the host CPU too as
> you leave the DATAEND interrupt on.  I suspect for reliability, we
> need to have the DMA controller callback function in place to enable
> the DATAEND interrupt.  Or something like that.
>

Yes I get it now, well, what we do is essentially create three state
variables that are all checked at end of DMA (by adding a
DMA termination callback), end of PIO or DATAEND IRQ arrival.
So a little bit like:

if (dataend && (pio_end || dma_end))
   complete;

I'll cook up some patch ASAP, just need to backport some
stuff on top of your latest MMCI patches.

Linus Walleij
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110131/8faa2c07/attachment.html>

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Mon, Jan 31, 2011 at 02:53:32PM +0100, Linus Walleij wrote:
> On Mon, Jan 31, 2011 at 11:40 AM, Russell King - ARM Linux <
> linux at arm.linux.org.uk> wrote:
> 
> What can happen is you receive the DATAEND interrupt,
> > which tears down the data side of the request, and starts the stop
> > command.  Meanwhile there's still data left in the FIFO for the CPU
> > to read.
> >
> > I suspect there's a similar race between DMA and the host CPU too as
> > you leave the DATAEND interrupt on.  I suspect for reliability, we
> > need to have the DMA controller callback function in place to enable
> > the DATAEND interrupt.  Or something like that.
> >
> 
> Yes I get it now, well, what we do is essentially create three state
> variables that are all checked at end of DMA (by adding a
> DMA termination callback), end of PIO or DATAEND IRQ arrival.

There's no need, and you don't want to do it like that.  If you receive
and ack the data end interrupt before you've completed the transfer,
then you need additional complexity in the PIO data handler.

Keep it simple.  Keep DATAEND masked until you've finished transfering
the data and then enable it - you'll get the interrupt at the right point
that way.

> I'll cook up some patch ASAP, just need to backport some
> stuff on top of your latest MMCI patches.

I have a number of patches on top of just these two.  There's eight
in total.  See the mmci-dma branch.  I want to discard the top patch
as I still don't think the mmci imposes that requirement.

PS, don't send me html encoded mail...

[PATCH] mmci: calculate remaining bytes at error correctly

[Sergei Shtylyov]

Hello.

On 31-01-2011 0:28, Russell King - ARM Linux wrote:

> Linus,

> Here's another couple of fixes...

> 8<----
> Subject: [PATCH 1/2] ARM: mmci: complete the transaction on error

> When we encounter an error, make sure we complete the transaction
> otherwise we'll leave the request dangling.

> Signed-off-by: Russell King<rmk+kernel@arm.linux.org.uk>
[...]

> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index b6fd6dc..175a623 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -319,7 +319,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
>   	if (status&  MCI_DATABLOCKEND)
>   		dev_err(mmc_dev(host->mmc), "stray MCI_DATABLOCKEND interrupt\n");
>
> -	if (status & MCI_DATAEND) {
> +	if (status & MCI_DATAEND

    Shouldn't 'status & MCI_DATAEND' be enclosed in parens?

> || data->error) {

WBR, Sergei

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Mon, Jan 31, 2011 at 03:00:12PM +0300, Sergei Shtylyov wrote:
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index b6fd6dc..175a623 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -319,7 +319,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
>>   	if (status&  MCI_DATABLOCKEND)
>>   		dev_err(mmc_dev(host->mmc), "stray MCI_DATABLOCKEND interrupt\n");
>>
>> -	if (status & MCI_DATAEND) {
>> +	if (status & MCI_DATAEND
>
>    Shouldn't 'status & MCI_DATAEND' be enclosed in parens?

No.  Only people who don't want to know operator precedence enclose
everything in parens.  Here's the grammar:

logical-OR-expression:
               logical-AND-expression
               logical-OR-expression || logical-AND-expression
logical-AND-expression:
               inclusive-OR-expression
               logical-AND-expression && inclusive-OR-expression
inclusive-OR-expression:
               exclusive-OR-expression
               inclusive-OR-expression | exclusive-OR-expression
exclusive-OR-expression:
               AND-expression
               exclusive-OR-expression ^ AND-expression
AND-expression:
               equality-expression
               AND-expression & equality-expression

So, a & b || c gives:

logical-OR-expression := a & b || c
+-logical-OR-expression := a & b
| `-logical-AND-expression := a & b
|   `-inclusive-OR-expression := a & b
|     `-exclusive-OR-expression := a & b
|       `-AND-expression := a & b
|         +-AND-expression := a
|         | `-equality-expression := a
|         `-equality-expression := b
+-logical-AND-expression := c
  `-inclusive-OR-expression := c
    `-exclusive-OR-expression := c
      `-AND-expression := c
       `-equality-expression := c

So, a & b || c is evaluated as ((a) & (b)) || (c), which is exactly what
we want it to be.

If it were && and ||, then there is reason to use parens as people often
get the relative precedence of && and || mixed up.

If you insist on putting parens around a & b there, then for the sake of
consistency, you should be putting parens around every sub-expression
involving || and && - as if you think that a & b || c could be incorrectly
evaluated, a < b || c falls into exactly the same category, as does
a < b || c < d.

[PATCH] mmci: calculate remaining bytes at error correctly

[Sergei Shtylyov]

Hello.

On 31-01-2011 15:25, Russell King - ARM Linux wrote:

>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>> index b6fd6dc..175a623 100644
>>> --- a/drivers/mmc/host/mmci.c
>>> +++ b/drivers/mmc/host/mmci.c
>>> @@ -319,7 +319,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data,
>>>    	if (status&   MCI_DATABLOCKEND)
>>>    		dev_err(mmc_dev(host->mmc), "stray MCI_DATABLOCKEND interrupt\n");

>>> -	if (status & MCI_DATAEND) {
>>> +	if (status & MCI_DATAEND

>>     Shouldn't 'status & MCI_DATAEND' be enclosed in parens?

> No.  Only people who don't want to know operator precedence enclose
> everything in parens.  Here's the grammar:

> logical-OR-expression:
>                 logical-AND-expression
>                 logical-OR-expression || logical-AND-expression
> logical-AND-expression:
>                 inclusive-OR-expression
>                 logical-AND-expression&&  inclusive-OR-expression
> inclusive-OR-expression:
>                 exclusive-OR-expression
>                 inclusive-OR-expression | exclusive-OR-expression
> exclusive-OR-expression:
>                 AND-expression
>                 exclusive-OR-expression ^ AND-expression
> AND-expression:
>                 equality-expression
>                 AND-expression&  equality-expression

> So, a & b || c gives:

> logical-OR-expression := a & b || c
> +-logical-OR-expression := a & b
> | `-logical-AND-expression := a & b
> |   `-inclusive-OR-expression := a & b
> |     `-exclusive-OR-expression := a & b
> |       `-AND-expression := a & b
> |         +-AND-expression := a
> |         | `-equality-expression := a
> |         `-equality-expression := b
> +-logical-AND-expression := c
>    `-inclusive-OR-expression := c
>      `-exclusive-OR-expression := c
>        `-AND-expression := c
>         `-equality-expression := c

> So, a&  b || c is evaluated as ((a)&  (b)) || (c), which is exactly what
> we want it to be.

> If it were && and ||, then there is reason to use parens as people often
> get the relative precedence of && and || mixed up.

    Well, that's certainly not my case. :-)

> If you insist on putting parens around a&  b there, then for the sake of
> consistency, you should be putting parens around every sub-expression
> involving || and &&  - as if you think that a & b || c could be incorrectly
> evaluated, a < b || c falls into exactly the same category, as does
> a < b || c < d.

    I mainly noted about those parens because gcc tends to give warnings in 
such cases, if I remember right.
    Thanks for your elaborate reply. :-)

WBR, Sergei

[PATCH] mmci: calculate remaining bytes at error correctly

[Russell King - ARM Linux]

On Mon, Jan 31, 2011 at 03:31:28PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 31-01-2011 15:25, Russell King - ARM Linux wrote:
>> If you insist on putting parens around a&  b there, then for the sake of
>> consistency, you should be putting parens around every sub-expression
>> involving || and &&  - as if you think that a & b || c could be incorrectly
>> evaluated, a < b || c falls into exactly the same category, as does
>> a < b || c < d.
>
>    I mainly noted about those parens because gcc tends to give warnings 
> in such cases, if I remember right.

Not quite.  GCC warns for these:

	a || b && c
	a | b & c

as it seems much harder to remember the relative precedence of the OR
operators vs the AND operators in each class of logical vs bitwise
operators.