[PATCH 00/17] MMC/SDHCI fixes

[PATCH 00/17] MMC/SDHCI fixes

[Russell King - ARM Linux]

This all started with a report from an iMX6 Hummingboard user that
they were seeing regular "mmc0: Got data interrupt 0x00000002 even
though no data operation was in progress." messages with 4.4-rc and
additional patches to enable UHS support in DT for the platform.

When I tested 4.4-rc on an iMX6 Hummingboard with UHS support enabled,
I found several other issues.  This patch series addresses some of
the issues.

However, while testing on Armada 388, an issue was found there, and
a fix is also included in this patch set, however it does not depend
on the rest of the patch series.

We all know that the SDHCI driver is in poor state, and Ulf's desire
to see it turned into a library (which actually comes from myself.)
However, with the driver in such a poor state, it makes sense to fix
a few of the issues first, especially if they result in the code
shrinking - which they do.

However, given what I've found below, I have to wonder how many of the
quirks that we have in this driver are down to the poor code rather than
real hardware problems: how many of them are due to poorly investigated
bugs.

This series starts off by shutting up some kernel messages that should
not be verbose:
- the voltage-ranges DT property is optional, so we should not print
  a message saying that it's missing from DT.  That suggests it isn't
  optional.  As no ill effect comes from not specifying it, I've
  dropped this message to debug level.

- "retrying because a re-tune was needed" - a re-tune is something that
  is part of the higher speed SD specs, and it's required to happen
  periodically.  So it's expected and normal behaviour.  There's no need
  to print this to the kernel log.

- When tuning fails, rather than a bland "mmc0: tuning execution failed"
  message, print the error code so that we can see what the reason for
  the failure is.  MMC fails on this point in many places.

- Move the initialisation of the command ->error member.  It looks to
  me as if this is not required; mmc_start_request() does this already,
  as it seems the rest of the MMC core does too.  Maybe this should be
  removed altogether?

- Testing error bits, then setting the error, then testing the error
  value, and completing the command is a very lengthy way to deal with
  errors here.  Test the error bits, and then decide what error occurred
  before completing.  (This code is actually buggy, see the next change).

- If a command _response_ CRC error occurs, we can't be certain whether
  the card received the command successfully, and has started processing
  it.  If it has, and it results in data being read from the card, the
  card will enter data transfer state, and start sending data to the
  host.  Therefore, it is completely wrong to blindly terminate the
  command, especially when such termination fails to clean _anything_
  up - eg, it leaves DMA in place so the host can continue DMAing to the
  memory, and it leaves buffers DMA mapped - which eventually annoys the
  DMA API debugging.  Fix this by assuming that a CRC error is a receive
  side error.  If the card did not correctly receive the command, it
  will not initiate a data transfer, and we should time out after the
  card specified timeout.

  With the unmodified mainline code, even if we reset the state machines
  when finishing a request in error, the _card_ itself may well still be
  in data transfer mode when this happens.  How can we be sure what state
  the data side is actually in?  Could this be why (eg) sdhci-esdhc-imx.c
  has this:

        /* FIXME: delay a bit for card to be ready for next tuning due to errors */
        mdelay(1);

  And how many more such _hacks_ are working around similar problems?

- The unaligned buffer handling is a total waste of resources in its
  current form.  We DMA map, sync, and unmap it on every transfer,
  whether the buffer is used or not.  For MMC/SD transfers, these will
  be coming from the VFS/MM layers, which operate through the page
  cache - hence the vast majority of buffers will be page aligned.
  Performance measurements on iMX6 show that this additional DMA
  maintanence is responsible for a 10% reduction in read transfer
  performance.  Switch this to use a DMA coherent mapping instead, which
  needs no DMA maintanence.

- sdhci_adma_table_post() is badly coded: we walk the scatterlist
  looking for any buffers which might not be aligned (which can be a
  rare event, see above) and then we only do something if we find an
  unaligned buffer _and_ it's a read transfer.  So for a write transfer,
  this scatterlist walking is a total waste of CPU cycles.  Testing the
  transfer direction is much cheaper than walking the scatterlist.
  Re-organise the code to do the cheap test first.

- There are two paths to identical DMA unmapping code (the code around
  dma_unmap_sg()) in sdhci_finish_data() and its child function
  sdhci_adma_table_post().  Trivially simplify this duplication.

- Move sdhci_pre_dma_transfer() so we avoid needing to declare it.

- Do the same thing with sdhci_prepare_data() and sdhci_pre_dma_transfer()
  as with sdhci_finish_data() and sdhci_adma_table_post().

- Fix the mess that is the data segment host_cookie handling.  The
  existing code seems like a candidate for the obfuscated coding prize!
  Four patches address this, turning it into something much easier to
  understand, which then allows (finally)...

- The DMA API leak which occurs when we have mapped the DMA buffers in
  sdhci_prepare_data() and an error occurs early on in the processing
  of a request.

- A final change (mentioned above) which fixes a prior commit for
  Armada 38x using sdhci-pxav3.c, found while trying to get the higher
  speed modes working on these SoCs.  Clearly the existing code is
  broken, this at least resolves some of the breakage by allowing the
  correct high-order SDHCI capabilities through.

All these patches are resulting from _only_ today; I'm sure more will
be coming in the forthcoming weeks, as all I seem to have to do is
give code a little shake and _lots_ of bugs appear.  Despite this being
a series of 17 patches, it does not yet solve _all_ the problems I've
found so far on iMX6 with SDHCI.

I've tested these changes on iMX6 at UHS speeds, and Armada 388 SoCs
at HS speeds so far.  Others should test them.

 drivers/mmc/card/block.c       |   4 +-
 drivers/mmc/core/core.c        |  10 +-
 drivers/mmc/host/sdhci-pxav3.c |   6 +-
 drivers/mmc/host/sdhci.c       | 318 ++++++++++++++++++-----------------------
 drivers/mmc/host/sdhci.h       |   4 +-
 5 files changed, 157 insertions(+), 185 deletions(-)

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 01/17] mmc: core: shut up "voltage-ranges unspecified" pr_info()

[Russell King]

Each time a driver such as sdhci-esdhc-imx is probed, we get a info
printk complaining that the DT voltage-ranges property has not been
specified.

However, the DT binding specifically says that the voltage-ranges
property is optional.  That means we should not be complaining that
DT hasn't specified this property: by indicating that it's optional,
it is valid not to have the property in DT.

Silence the warning if the property is missing.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5ae89e48fd85..b5e663b67cb5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1220,8 +1220,12 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 
 	voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
 	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
-	if (!voltage_ranges || !num_ranges) {
-		pr_info("%s: voltage-ranges unspecified\n", np->full_name);
+	if (!voltage_ranges) {
+		pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
+		return -EINVAL;
+	}
+	if (!num_ranges) {
+		pr_err("%s: voltage-ranges empty\n", np->full_name);
 		return -EINVAL;
 	}
 
-- 
2.1.0

Re: [PATCH 01/17] mmc: core: shut up "voltage-ranges unspecified" pr_info()

[Ulf Hansson]

On 19 December 2015 at 21:29, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> Each time a driver such as sdhci-esdhc-imx is probed, we get a info
> printk complaining that the DT voltage-ranges property has not been
> specified.
>
> However, the DT binding specifically says that the voltage-ranges
> property is optional.  That means we should not be complaining that
> DT hasn't specified this property: by indicating that it's optional,
> it is valid not to have the property in DT.

Agree!

>
> Silence the warning if the property is missing.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/mmc/core/core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5ae89e48fd85..b5e663b67cb5 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1220,8 +1220,12 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
>
>         voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
>         num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
> -       if (!voltage_ranges || !num_ranges) {
> -               pr_info("%s: voltage-ranges unspecified\n", np->full_name);
> +       if (!voltage_ranges) {
> +               pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
> +               return -EINVAL;

Because it's optional, I don't think we should return an error code here.

> +       }
> +       if (!num_ranges) {
> +               pr_err("%s: voltage-ranges empty\n", np->full_name);
>                 return -EINVAL;

Ditto.

>         }
>
> --
> 2.1.0
>

Kind regards
Uffe

Re: [PATCH 01/17] mmc: core: shut up "voltage-ranges unspecified" pr_info()

[Russell King - ARM Linux]

On Mon, Dec 21, 2015 at 11:18:15AM +0100, Ulf Hansson wrote:
> On 19 December 2015 at 21:29, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> > Each time a driver such as sdhci-esdhc-imx is probed, we get a info
> > printk complaining that the DT voltage-ranges property has not been
> > specified.
> >
> > However, the DT binding specifically says that the voltage-ranges
> > property is optional.  That means we should not be complaining that
> > DT hasn't specified this property: by indicating that it's optional,
> > it is valid not to have the property in DT.
> 
> Agree!
> 
> >
> > Silence the warning if the property is missing.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/mmc/core/core.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 5ae89e48fd85..b5e663b67cb5 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1220,8 +1220,12 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
> >
> >         voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
> >         num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
> > -       if (!voltage_ranges || !num_ranges) {
> > -               pr_info("%s: voltage-ranges unspecified\n", np->full_name);
> > +       if (!voltage_ranges) {
> > +               pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
> > +               return -EINVAL;
> 
> Because it's optional, I don't think we should return an error code here.

Maybe, but that's changing the behaviour beyond what I said in the
commit message, hence should be a separate patch so that if it causes
problems, then it can be reverted independently of this fix.

> > +       }
> > +       if (!num_ranges) {
> > +               pr_err("%s: voltage-ranges empty\n", np->full_name);
> >                 return -EINVAL;
> 
> Ditto.

For this one, it's entirely reasonable - it's a mal-formed voltage-ranges
property.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 01/17] mmc: core: shut up "voltage-ranges unspecified" pr_info()

[Russell King]

Each time a driver such as sdhci-esdhc-imx is probed, we get a info
printk complaining that the DT voltage-ranges property has not been
specified.

However, the DT binding specifically says that the voltage-ranges
property is optional.  That means we should not be complaining that
DT hasn't specified this property: by indicating that it's optional,
it is valid not to have the property in DT.

Silence the warning if the property is missing.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5ae89e48fd85..b5e663b67cb5 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1220,8 +1220,12 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 
 	voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
 	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
-	if (!voltage_ranges || !num_ranges) {
-		pr_info("%s: voltage-ranges unspecified\n", np->full_name);
+	if (!voltage_ranges) {
+		pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
+		return -EINVAL;
+	}
+	if (!num_ranges) {
+		pr_err("%s: voltage-ranges empty\n", np->full_name);
 		return -EINVAL;
 	}
 
-- 
2.1.0

[PATCH 02/17] mmc: block: shut up "retrying because a re-tune was needed" message

[Russell King]

Re-tuning is part of standard requirements for the higher speed SD
card protocols, and is not an error when this occurs.  When we retry
a command due to a retune, we should not print a message to the
kernel log.

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

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index d8486168415a..16c8d977228e 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1367,8 +1367,8 @@ static int mmc_blk_err_check(struct mmc_card *card,
 
 	if (brq->data.error) {
 		if (need_retune && !brq->retune_retry_done) {
-			pr_info("%s: retrying because a re-tune was needed\n",
-				req->rq_disk->disk_name);
+			pr_debug("%s: retrying because a re-tune was needed\n",
+				 req->rq_disk->disk_name);
 			brq->retune_retry_done = 1;
 			return MMC_BLK_RETRY;
 		}
-- 
2.1.0

[PATCH 03/17] mmc: core: report tuning command execution failure reason

[Russell King]

Print the error code when the tuning command fails.  This allows the
reason for the failure to be reported, which aids debugging.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/core/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b5e663b67cb5..b74c1644c960 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1085,7 +1085,7 @@ int mmc_execute_tuning(struct mmc_card *card)
 	err = host->ops->execute_tuning(host, opcode);
 
 	if (err)
-		pr_err("%s: tuning execution failed\n", mmc_hostname(host));
+		pr_err("%s: tuning execution failed: %d\n", mmc_hostname(host), err);
 	else
 		mmc_retune_enable(host);
 
-- 
2.1.0

[PATCH 04/17] mmc: sdhci: move initialisation of command error member

[Russell King]

When a command is started, logically it has no error.  Initialise the
command's error member to zero whenever we start a command.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b48565ed5616..86e0e847fcc8 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1002,6 +1002,9 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 
 	WARN_ON(host->cmd);
 
+	/* Initially, a command has no error */
+	cmd->error = 0;
+
 	/* Wait max 10 ms */
 	timeout = 10;
 
@@ -1096,8 +1099,6 @@ static void sdhci_finish_command(struct sdhci_host *host)
 		}
 	}
 
-	host->cmd->error = 0;
-
 	/* Finished CMD23, now send actual command. */
 	if (host->cmd == host->mrq->sbc) {
 		host->cmd = NULL;
-- 
2.1.0

[PATCH 05/17] mmc: sdhci: clean up command error handling

[Russell King]

Avoid multiple tests while handling a command error; simplify the code.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 86e0e847fcc8..86310b162304 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2333,13 +2333,13 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)
 		return;
 	}
 
-	if (intmask & SDHCI_INT_TIMEOUT)
-		host->cmd->error = -ETIMEDOUT;
-	else if (intmask & (SDHCI_INT_CRC | SDHCI_INT_END_BIT |
-			SDHCI_INT_INDEX))
-		host->cmd->error = -EILSEQ;
+	if (intmask & (SDHCI_INT_TIMEOUT | SDHCI_INT_CRC |
+		       SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
+		if (intmask & SDHCI_INT_TIMEOUT)
+			host->cmd->error = -ETIMEDOUT;
+		else
+			host->cmd->error = -EILSEQ;
 
-	if (host->cmd->error) {
 		tasklet_schedule(&host->finish_tasklet);
 		return;
 	}
-- 
2.1.0

[PATCH 06/17] mmc: sdhci: command response CRC error handling

[Russell King]

When we get a response CRC error on a command, it means that the
response we received back from the card was not correct.  It does not
mean that the card did not receive the command correctly.  If the
command is one which initiates a data transfer, the card can enter the
data transfer state, and start sending data.

Moreover, if the request contained a data phase, we do not clean this
up, and this results in the driver triggering DMA API debug warnings,
and also creates a race condition in the driver, between running the
finish_tasklet and the data transfer interrupts, which can trigger a
"Got data interrupt" state dump.

Fix this by handing a response CRC error slightly differently: record
the failure of the data initiating command, but allow the remainder of
the request to be processed normally.  This is safe as core MMC checks
the status of all commands and data transfer phases of the request.

If the card does not initiate a data transfer, then we should time out
according to the data transfer parameters.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 86310b162304..3e718e465a1b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2340,6 +2340,23 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)
 		else
 			host->cmd->error = -EILSEQ;
 
+		/*
+		 * If this command initiates a data phase and a response
+		 * CRC error is signalled, the card can start transferring
+		 * data - the card may have received the command without
+		 * error.  We must not terminate the request early.
+		 *
+		 * If the card did not receive the command, the data phase
+		 * will time out.
+		 *
+		 * FIXME: we also need to clean up the data phase if any
+		 * command fails, not just the data initiating command.
+		 */
+		if (host->cmd->data && intmask & SDHCI_INT_CRC) {
+			host->cmd = NULL;
+			return;
+		}
+
 		tasklet_schedule(&host->finish_tasklet);
 		return;
 	}
-- 
2.1.0

[PATCH 07/17] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Russell King]

Unnecessarily mapping and unmapping the align buffer for SD cards is
expensive: performance measurements on iMX6 show that this gives a hit
of 10% on hdparm buffered disk reads.

MMC/SD card IO comes from the mm/vfs which gives us page based IO, so
for this case, the align buffer is not going to be used.  However, we
still map and unmap this buffer.

Eliminate this by switching the align buffer to be a DMA coherent
buffer, which needs no DMA maintanence to access the buffer.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 53 ++++++++++++++++--------------------------------
 1 file changed, 18 insertions(+), 35 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 3e718e465a1b..8a4eb1c41f3e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -465,8 +465,6 @@ static void sdhci_adma_mark_end(void *desc)
 static int sdhci_adma_table_pre(struct sdhci_host *host,
 	struct mmc_data *data)
 {
-	int direction;
-
 	void *desc;
 	void *align;
 	dma_addr_t addr;
@@ -483,20 +481,9 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 	 * We currently guess that it is LE.
 	 */
 
-	if (data->flags & MMC_DATA_READ)
-		direction = DMA_FROM_DEVICE;
-	else
-		direction = DMA_TO_DEVICE;
-
-	host->align_addr = dma_map_single(mmc_dev(host->mmc),
-		host->align_buffer, host->align_buffer_sz, direction);
-	if (dma_mapping_error(mmc_dev(host->mmc), host->align_addr))
-		goto fail;
-	BUG_ON(host->align_addr & host->align_mask);
-
 	host->sg_count = sdhci_pre_dma_transfer(host, data);
 	if (host->sg_count < 0)
-		goto unmap_align;
+		return -EINVAL;
 
 	desc = host->adma_table;
 	align = host->align_buffer;
@@ -567,22 +554,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		/* nop, end, valid */
 		sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
 	}
-
-	/*
-	 * Resync align buffer as we might have changed it.
-	 */
-	if (data->flags & MMC_DATA_WRITE) {
-		dma_sync_single_for_device(mmc_dev(host->mmc),
-			host->align_addr, host->align_buffer_sz, direction);
-	}
-
 	return 0;
-
-unmap_align:
-	dma_unmap_single(mmc_dev(host->mmc), host->align_addr,
-		host->align_buffer_sz, direction);
-fail:
-	return -EINVAL;
 }
 
 static void sdhci_adma_table_post(struct sdhci_host *host,
@@ -602,9 +574,6 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 	else
 		direction = DMA_TO_DEVICE;
 
-	dma_unmap_single(mmc_dev(host->mmc), host->align_addr,
-		host->align_buffer_sz, direction);
-
 	/* Do a quick scan of the SG list for any unaligned mappings */
 	has_unaligned = false;
 	for_each_sg(data->sg, sg, host->sg_count, i)
@@ -3003,6 +2972,10 @@ int sdhci_add_host(struct sdhci_host *host)
 						      host->adma_table_sz,
 						      &host->adma_addr,
 						      GFP_KERNEL);
+		host->align_buffer = dma_alloc_coherent(mmc_dev(mmc),
+							host->align_buffer_sz,
+							&host->align_addr,
+							GFP_KERNEL);
 		host->align_buffer = kmalloc(host->align_buffer_sz, GFP_KERNEL);
 		if (!host->adma_table || !host->align_buffer) {
 			if (host->adma_table)
@@ -3010,7 +2983,11 @@ int sdhci_add_host(struct sdhci_host *host)
 						  host->adma_table_sz,
 						  host->adma_table,
 						  host->adma_addr);
-			kfree(host->align_buffer);
+			if (host->align_buffer)
+				dma_free_coherent(mmc_dev(mmc),
+						  host->align_buffer_sz,
+						  host->align_buffer,
+						  host->align_addr);
 			pr_warn("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n",
 				mmc_hostname(mmc));
 			host->flags &= ~SDHCI_USE_ADMA;
@@ -3022,10 +2999,14 @@ int sdhci_add_host(struct sdhci_host *host)
 			host->flags &= ~SDHCI_USE_ADMA;
 			dma_free_coherent(mmc_dev(mmc), host->adma_table_sz,
 					  host->adma_table, host->adma_addr);
-			kfree(host->align_buffer);
+			dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz,
+					  host->align_buffer, host->align_addr);
 			host->adma_table = NULL;
 			host->align_buffer = NULL;
 		}
+
+		/* dma_alloc_coherent returns page aligned and sized buffers */
+		BUG_ON(host->align_addr & host->align_mask);
 	}
 
 	/*
@@ -3489,7 +3470,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 	if (host->adma_table)
 		dma_free_coherent(mmc_dev(mmc), host->adma_table_sz,
 				  host->adma_table, host->adma_addr);
-	kfree(host->align_buffer);
+	if (host->align_buffer)
+		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz,
+				  host->align_buffer, host->align_addr);
 
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
-- 
2.1.0

Re: [PATCH 07/17] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Ulf Hansson]

On 19 December 2015 at 21:30, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> Unnecessarily mapping and unmapping the align buffer for SD cards is
> expensive: performance measurements on iMX6 show that this gives a hit
> of 10% on hdparm buffered disk reads.
>
> MMC/SD card IO comes from the mm/vfs which gives us page based IO, so
> for this case, the align buffer is not going to be used.  However, we
> still map and unmap this buffer.

What about the SDIO case? How will this change affect SDIO data transfers?

>
> Eliminate this by switching the align buffer to be a DMA coherent
> buffer, which needs no DMA maintanence to access the buffer.

/s/maintanence/maintenance

[...]

Kind regards
Uffe

Re: [PATCH 07/17] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Russell King - ARM Linux]

On Mon, Dec 21, 2015 at 11:28:15AM +0100, Ulf Hansson wrote:
> On 19 December 2015 at 21:30, Russell King <rmk+kernel@arm.linux.org.uk> wrote:
> > Unnecessarily mapping and unmapping the align buffer for SD cards is
> > expensive: performance measurements on iMX6 show that this gives a hit
> > of 10% on hdparm buffered disk reads.
> >
> > MMC/SD card IO comes from the mm/vfs which gives us page based IO, so
> > for this case, the align buffer is not going to be used.  However, we
> > still map and unmap this buffer.
> 
> What about the SDIO case? How will this change affect SDIO data transfers?

I can't quantify that as I have no SDIO cards to do any measurements.

It depends entirely on the expense of accessing to DMA coherent memory
compared to accessing normal memory and then syncing it.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 08/17] mmc: sdhci: clean up coding style in sdhci_adma_table_pre()

[Russell King]

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 8a4eb1c41f3e..a1853671a591 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -465,16 +465,12 @@ static void sdhci_adma_mark_end(void *desc)
 static int sdhci_adma_table_pre(struct sdhci_host *host,
 	struct mmc_data *data)
 {
-	void *desc;
-	void *align;
-	dma_addr_t addr;
-	dma_addr_t align_addr;
-	int len, offset;
-
 	struct scatterlist *sg;
-	int i;
-	char *buffer;
 	unsigned long flags;
+	dma_addr_t addr, align_addr;
+	void *desc, *align;
+	char *buffer;
+	int len, offset, i;
 
 	/*
 	 * The spec does not specify endianness of descriptor table.
@@ -495,10 +491,9 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		len = sg_dma_len(sg);
 
 		/*
-		 * The SDHCI specification states that ADMA
-		 * addresses must be 32-bit aligned. If they
-		 * aren't, then we use a bounce buffer for
-		 * the (up to three) bytes that screw up the
+		 * The SDHCI specification states that ADMA addresses must
+		 * be 32-bit aligned. If they aren't, then we use a bounce
+		 * buffer for the (up to three) bytes that screw up the
 		 * alignment.
 		 */
 		offset = (host->align_sz - (addr & host->align_mask)) &
@@ -539,19 +534,13 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 	}
 
 	if (host->quirks & SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC) {
-		/*
-		* Mark the last descriptor as the terminating descriptor
-		*/
+		/* Mark the last descriptor as the terminating descriptor */
 		if (desc != host->adma_table) {
 			desc -= host->desc_sz;
 			sdhci_adma_mark_end(desc);
 		}
 	} else {
-		/*
-		* Add a terminating entry.
-		*/
-
-		/* nop, end, valid */
+		/* Add a terminating entry - nop, end, valid */
 		sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
 	}
 	return 0;
-- 
2.1.0

[PATCH 09/17] mmc: sdhci: avoid walking SG list for writes

[Russell King]

If we are writing data to the card, there is no point in walking the
scatterlist to find out if there are any unaligned entries; this is a
needless waste of CPU cycles.  Avoid this by checking for a non-read
tranfer first.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a1853671a591..90ad6fd8bb65 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -556,37 +556,39 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 	void *align;
 	char *buffer;
 	unsigned long flags;
-	bool has_unaligned;
 
 	if (data->flags & MMC_DATA_READ)
 		direction = DMA_FROM_DEVICE;
 	else
 		direction = DMA_TO_DEVICE;
 
-	/* Do a quick scan of the SG list for any unaligned mappings */
-	has_unaligned = false;
-	for_each_sg(data->sg, sg, host->sg_count, i)
-		if (sg_dma_address(sg) & host->align_mask) {
-			has_unaligned = true;
-			break;
-		}
+	if (data->flags & MMC_DATA_READ) {
+		bool has_unaligned = false;
 
-	if (has_unaligned && data->flags & MMC_DATA_READ) {
-		dma_sync_sg_for_cpu(mmc_dev(host->mmc), data->sg,
-			data->sg_len, direction);
+		/* Do a quick scan of the SG list for any unaligned mappings */
+		for_each_sg(data->sg, sg, host->sg_count, i)
+			if (sg_dma_address(sg) & host->align_mask) {
+				has_unaligned = true;
+				break;
+			}
 
-		align = host->align_buffer;
+		if (has_unaligned) {
+			dma_sync_sg_for_cpu(mmc_dev(host->mmc), data->sg,
+				data->sg_len, direction);
 
-		for_each_sg(data->sg, sg, host->sg_count, i) {
-			if (sg_dma_address(sg) & host->align_mask) {
-				size = host->align_sz -
-				       (sg_dma_address(sg) & host->align_mask);
+			align = host->align_buffer;
 
-				buffer = sdhci_kmap_atomic(sg, &flags);
-				memcpy(buffer, align, size);
-				sdhci_kunmap_atomic(buffer, &flags);
+			for_each_sg(data->sg, sg, host->sg_count, i) {
+				if (sg_dma_address(sg) & host->align_mask) {
+					size = host->align_sz -
+					       (sg_dma_address(sg) & host->align_mask);
 
-				align += host->align_sz;
+					buffer = sdhci_kmap_atomic(sg, &flags);
+					memcpy(buffer, align, size);
+					sdhci_kunmap_atomic(buffer, &flags);
+
+					align += host->align_sz;
+				}
 			}
 		}
 	}
-- 
2.1.0

[PATCH 10/17] mmc: sdhci: factor out common DMA cleanup in sdhci_finish_data()

[Russell King]

sdhci_finish_data() has two paths which result in identical DMA cleanup.
One is when SDHCI_USE_ADMA is clear, and the other is just before when
SDHCI_USE_ADMA is set, and is performed within sdhci_adma_table_post().

Simplify the code by removing the 'else' and eliminating the duplicate
inside sdhci_adma_table_post().

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 90ad6fd8bb65..d315273d1184 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -549,19 +549,12 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 static void sdhci_adma_table_post(struct sdhci_host *host,
 	struct mmc_data *data)
 {
-	int direction;
-
 	struct scatterlist *sg;
 	int i, size;
 	void *align;
 	char *buffer;
 	unsigned long flags;
 
-	if (data->flags & MMC_DATA_READ)
-		direction = DMA_FROM_DEVICE;
-	else
-		direction = DMA_TO_DEVICE;
-
 	if (data->flags & MMC_DATA_READ) {
 		bool has_unaligned = false;
 
@@ -574,7 +567,7 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 
 		if (has_unaligned) {
 			dma_sync_sg_for_cpu(mmc_dev(host->mmc), data->sg,
-				data->sg_len, direction);
+					    data->sg_len, DMA_FROM_DEVICE);
 
 			align = host->align_buffer;
 
@@ -592,12 +585,6 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
 			}
 		}
 	}
-
-	if (data->host_cookie == COOKIE_MAPPED) {
-		dma_unmap_sg(mmc_dev(host->mmc), data->sg,
-			data->sg_len, direction);
-		data->host_cookie = COOKIE_UNMAPPED;
-	}
 }
 
 static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
@@ -908,14 +895,12 @@ static void sdhci_finish_data(struct sdhci_host *host)
 	if (host->flags & SDHCI_REQ_USE_DMA) {
 		if (host->flags & SDHCI_USE_ADMA)
 			sdhci_adma_table_post(host, data);
-		else {
-			if (data->host_cookie == COOKIE_MAPPED) {
-				dma_unmap_sg(mmc_dev(host->mmc),
-					data->sg, data->sg_len,
-					(data->flags & MMC_DATA_READ) ?
-					DMA_FROM_DEVICE : DMA_TO_DEVICE);
-				data->host_cookie = COOKIE_UNMAPPED;
-			}
+
+		if (data->host_cookie == COOKIE_MAPPED) {
+			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+				     (data->flags & MMC_DATA_READ) ?
+				     DMA_FROM_DEVICE : DMA_TO_DEVICE);
+			data->host_cookie = COOKIE_UNMAPPED;
 		}
 	}
 
-- 
2.1.0

[PATCH 11/17] mmc: sdhci: move sdhci_pre_dma_transfer()

[Russell King]

Move sdhci_pre_dma_transfer() to avoid needing to declare this function
before use.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 52 +++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d315273d1184..239581ea6856 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -53,8 +53,6 @@ static void sdhci_finish_data(struct sdhci_host *);
 static void sdhci_finish_command(struct sdhci_host *);
 static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode);
 static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable);
-static int sdhci_pre_dma_transfer(struct sdhci_host *host,
-					struct mmc_data *data);
 static int sdhci_do_get_cd(struct sdhci_host *host);
 
 #ifdef CONFIG_PM
@@ -428,6 +426,31 @@ static void sdhci_transfer_pio(struct sdhci_host *host)
 	DBG("PIO transfer complete.\n");
 }
 
+static int sdhci_pre_dma_transfer(struct sdhci_host *host,
+				  struct mmc_data *data)
+{
+	int sg_count;
+
+	if (data->host_cookie == COOKIE_MAPPED) {
+		data->host_cookie = COOKIE_GIVEN;
+		return data->sg_count;
+	}
+
+	WARN_ON(data->host_cookie == COOKIE_GIVEN);
+
+	sg_count = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+				data->flags & MMC_DATA_WRITE ?
+				DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+	if (sg_count == 0)
+		return -ENOSPC;
+
+	data->sg_count = sg_count;
+	data->host_cookie = COOKIE_MAPPED;
+
+	return sg_count;
+}
+
 static char *sdhci_kmap_atomic(struct scatterlist *sg, unsigned long *flags)
 {
 	local_irq_save(*flags);
@@ -2079,31 +2102,6 @@ static void sdhci_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	}
 }
 
-static int sdhci_pre_dma_transfer(struct sdhci_host *host,
-				       struct mmc_data *data)
-{
-	int sg_count;
-
-	if (data->host_cookie == COOKIE_MAPPED) {
-		data->host_cookie = COOKIE_GIVEN;
-		return data->sg_count;
-	}
-
-	WARN_ON(data->host_cookie == COOKIE_GIVEN);
-
-	sg_count = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-				data->flags & MMC_DATA_WRITE ?
-				DMA_TO_DEVICE : DMA_FROM_DEVICE);
-
-	if (sg_count == 0)
-		return -ENOSPC;
-
-	data->sg_count = sg_count;
-	data->host_cookie = COOKIE_MAPPED;
-
-	return sg_count;
-}
-
 static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 			       bool is_first_req)
 {
-- 
2.1.0

[PATCH 12/17] mmc: sdhci: factor out sdhci_pre_dma_transfer() from sdhci_adma_table_pre()

[Russell King]

In sdhci_prepare_data(), when SDHCI_REQ_USE_DMA is set, there are two
paths that prepare the data buffers for transfer.  One is when
SDHCI_USE_ADMA is set, and is located inside sdhci_adma_table_pre().
The other is when SDHCI_USE_ADMA is clear, in the else clause of the
above.

Factor out the call to sdhci_pre_dma_transfer() along with its error
checking.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 62 ++++++++++++++++++------------------------------
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 239581ea6856..d7cc61101a1a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -485,8 +485,8 @@ static void sdhci_adma_mark_end(void *desc)
 	dma_desc->cmd |= cpu_to_le16(ADMA2_END);
 }
 
-static int sdhci_adma_table_pre(struct sdhci_host *host,
-	struct mmc_data *data)
+static void sdhci_adma_table_pre(struct sdhci_host *host,
+	struct mmc_data *data, int sg_count)
 {
 	struct scatterlist *sg;
 	unsigned long flags;
@@ -500,9 +500,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 	 * We currently guess that it is LE.
 	 */
 
-	host->sg_count = sdhci_pre_dma_transfer(host, data);
-	if (host->sg_count < 0)
-		return -EINVAL;
+	host->sg_count = sg_count;
 
 	desc = host->adma_table;
 	align = host->align_buffer;
@@ -566,7 +564,6 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,
 		/* Add a terminating entry - nop, end, valid */
 		sdhci_adma_write_desc(host, desc, 0, 0, ADMA2_NOP_END_VALID);
 	}
-	return 0;
 }
 
 static void sdhci_adma_table_post(struct sdhci_host *host,
@@ -696,7 +693,6 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 ctrl;
 	struct mmc_data *data = cmd->data;
-	int ret;
 
 	WARN_ON(host->data);
 
@@ -783,39 +779,27 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		if (host->flags & SDHCI_USE_ADMA) {
-			ret = sdhci_adma_table_pre(host, data);
-			if (ret) {
-				/*
-				 * This only happens when someone fed
-				 * us an invalid request.
-				 */
-				WARN_ON(1);
-				host->flags &= ~SDHCI_REQ_USE_DMA;
-			} else {
-				sdhci_writel(host, host->adma_addr,
-					SDHCI_ADMA_ADDRESS);
-				if (host->flags & SDHCI_USE_64_BIT_DMA)
-					sdhci_writel(host,
-						     (u64)host->adma_addr >> 32,
-						     SDHCI_ADMA_ADDRESS_HI);
-			}
-		} else {
-			int sg_cnt;
+		int sg_cnt = sdhci_pre_dma_transfer(host, data);
 
-			sg_cnt = sdhci_pre_dma_transfer(host, data);
-			if (sg_cnt <= 0) {
-				/*
-				 * This only happens when someone fed
-				 * us an invalid request.
-				 */
-				WARN_ON(1);
-				host->flags &= ~SDHCI_REQ_USE_DMA;
-			} else {
-				WARN_ON(sg_cnt != 1);
-				sdhci_writel(host, sg_dma_address(data->sg),
-					SDHCI_DMA_ADDRESS);
-			}
+		if (sg_cnt <= 0) {
+			/*
+			 * This only happens when someone fed
+			 * us an invalid request.
+			 */
+			WARN_ON(1);
+			host->flags &= ~SDHCI_REQ_USE_DMA;
+		} else if (host->flags & SDHCI_USE_ADMA) {
+			sdhci_adma_table_pre(host, data, sg_cnt);
+
+			sdhci_writel(host, host->adma_addr, SDHCI_ADMA_ADDRESS);
+			if (host->flags & SDHCI_USE_64_BIT_DMA)
+				sdhci_writel(host,
+					     (u64)host->adma_addr >> 32,
+					     SDHCI_ADMA_ADDRESS_HI);
+		} else {
+			WARN_ON(sg_cnt != 1);
+			sdhci_writel(host, sg_dma_address(data->sg),
+				SDHCI_DMA_ADDRESS);
 		}
 	}
 
-- 
2.1.0

[PATCH 13/17] mmc: sdhci: pass the cookie into sdhci_pre_dma_transfer()

[Russell King]

Pass the desired cookie for a successful map.  This is in preparation to
clean up the MAPPED/GIVEN states.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d7cc61101a1a..46cffb7f6841 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -427,7 +427,7 @@ static void sdhci_transfer_pio(struct sdhci_host *host)
 }
 
 static int sdhci_pre_dma_transfer(struct sdhci_host *host,
-				  struct mmc_data *data)
+				  struct mmc_data *data, int cookie)
 {
 	int sg_count;
 
@@ -446,7 +446,7 @@ static int sdhci_pre_dma_transfer(struct sdhci_host *host,
 		return -ENOSPC;
 
 	data->sg_count = sg_count;
-	data->host_cookie = COOKIE_MAPPED;
+	data->host_cookie = cookie;
 
 	return sg_count;
 }
@@ -779,7 +779,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		int sg_cnt = sdhci_pre_dma_transfer(host, data);
+		int sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
 
 		if (sg_cnt <= 0) {
 			/*
@@ -2094,7 +2094,7 @@ static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	mrq->data->host_cookie = COOKIE_UNMAPPED;
 
 	if (host->flags & SDHCI_REQ_USE_DMA)
-		sdhci_pre_dma_transfer(host, mrq->data);
+		sdhci_pre_dma_transfer(host, mrq->data, COOKIE_MAPPED);
 }
 
 static void sdhci_card_event(struct mmc_host *mmc)
-- 
2.1.0

[PATCH 14/17] mmc: sdhci: always unmap a mapped data transfer in sdhci_post_req()

[Russell King]

If the host cookie indicates that the data buffers of a request are
mapped at sdhci_post_req() time, always unmap the data buffers.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 46cffb7f6841..264248ac439e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2077,8 +2077,7 @@ static void sdhci_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	struct mmc_data *data = mrq->data;
 
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		if (data->host_cookie == COOKIE_GIVEN ||
-				data->host_cookie == COOKIE_MAPPED)
+		if (data->host_cookie != COOKIE_UNMAPPED)
 			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
 					 data->flags & MMC_DATA_WRITE ?
 					 DMA_TO_DEVICE : DMA_FROM_DEVICE);
-- 
2.1.0

[PATCH 15/17] mmc: sdhci: clean up host cookie handling

[Russell King]

Commit d31911b9374a ("mmc: sdhci: fix dma memory leak in sdhci_pre_req()")
added a complicated method to manage the DMA map state for the data
transfer, but this complexity is not required.

There are three states:
* Unmapped
* Mapped by sdhci_pre_req()
* Mapped by sdhci_prepare_data()

sdhci_prepare_data() needs to know when the data buffers have been
successfully mapped by sdhci_pre_req(), and if so, there is no need to
map them a second time.

When we come to tear down the mapping, we want to know whether
sdhci_post_req() will be called (which is determined by sdhci_pre_req()
having been previously called) so that we can postpone the unmap
operation.

Hence, it makes sense to simply record when the successful DMA map
happened (via COOKIE_PRE_MAPPED vs COOKIE_MAPPED) rather than having
the complex mechanics involving COOKIE_MAPPED vs COOKIE_GIVEN.

If a mapping is created by sdhci_prepare_data(), we must tear it down
ourselves, without waiting for sdhci_post_req() (hence, the new
COOKIE_MAPPED case).  If the mapping is created by sdhci_pre_req()
then sdhci_post_req() is responsible for tearing the mapping down.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 264248ac439e..6f23dba74cbc 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -431,12 +431,12 @@ static int sdhci_pre_dma_transfer(struct sdhci_host *host,
 {
 	int sg_count;
 
-	if (data->host_cookie == COOKIE_MAPPED) {
-		data->host_cookie = COOKIE_GIVEN;
+	/*
+	 * If the data buffers are already mapped, return the previous
+	 * dma_map_sg() result.
+	 */
+	if (data->host_cookie == COOKIE_PRE_MAPPED)
 		return data->sg_count;
-	}
-
-	WARN_ON(data->host_cookie == COOKIE_GIVEN);
 
 	sg_count = dma_map_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
 				data->flags & MMC_DATA_WRITE ?
@@ -2093,7 +2093,7 @@ static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	mrq->data->host_cookie = COOKIE_UNMAPPED;
 
 	if (host->flags & SDHCI_REQ_USE_DMA)
-		sdhci_pre_dma_transfer(host, mrq->data, COOKIE_MAPPED);
+		sdhci_pre_dma_transfer(host, mrq->data, COOKIE_PRE_MAPPED);
 }
 
 static void sdhci_card_event(struct mmc_host *mmc)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 9d4aa31b683a..9dd2e1f688ea 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -311,8 +311,8 @@ struct sdhci_adma2_64_desc {
 
 enum sdhci_cookie {
 	COOKIE_UNMAPPED,
-	COOKIE_MAPPED,
-	COOKIE_GIVEN,
+	COOKIE_PRE_MAPPED,	/* mapped by sdhci_pre_req() */
+	COOKIE_MAPPED,		/* mapped by sdhci_prepare_data() */
 };
 
 struct sdhci_host {
-- 
2.1.0

[PATCH 16/17] mmc: sdhci: plug DMA mapping leak on error

[Russell King]

If we terminate a command early, we fail to properly clean up the DMA
mappings for the data part of the request.  Move this clean up to the
tasklet, which is the common path for finishing a request so we always
clean up after ourselves.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6f23dba74cbc..4ead8b2f8f8f 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -899,17 +899,9 @@ static void sdhci_finish_data(struct sdhci_host *host)
 	data = host->data;
 	host->data = NULL;
 
-	if (host->flags & SDHCI_REQ_USE_DMA) {
-		if (host->flags & SDHCI_USE_ADMA)
-			sdhci_adma_table_post(host, data);
-
-		if (data->host_cookie == COOKIE_MAPPED) {
-			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-				     (data->flags & MMC_DATA_READ) ?
-				     DMA_FROM_DEVICE : DMA_TO_DEVICE);
-			data->host_cookie = COOKIE_UNMAPPED;
-		}
-	}
+	if ((host->flags & (SDHCI_REQ_USE_DMA | SDHCI_USE_ADMA)) ==
+	    (SDHCI_REQ_USE_DMA | SDHCI_USE_ADMA))
+		sdhci_adma_table_post(host, data);
 
 	/*
 	 * The specification states that the block count register must
@@ -2174,6 +2166,22 @@ static void sdhci_tasklet_finish(unsigned long param)
 	mrq = host->mrq;
 
 	/*
+	 * Always unmap the data buffers if they were mapped by
+	 * sdhci_prepare_data() whenever we finish with a request.
+	 * This avoids leaking DMA mappings on error.
+	 */
+	if (host->flags & SDHCI_REQ_USE_DMA) {
+		struct mmc_data *data = mrq->data;
+
+		if (data && data->host_cookie == COOKIE_MAPPED) {
+			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+				     (data->flags & MMC_DATA_READ) ?
+				     DMA_FROM_DEVICE : DMA_TO_DEVICE);
+			data->host_cookie = COOKIE_UNMAPPED;
+		}
+	}
+
+	/*
 	 * The controller needs a reset of internal state machines
 	 * upon error conditions.
 	 */
-- 
2.1.0

[PATCH 17/17] mmc: sdhci-pxav3: fix higher speed mode capabilities

[Russell King]

Commit 1140011ee9d9 ("mmc: sdhci-pxav3: Modify clock settings for the
SDR50 and DDR50 modes") broke any chance of the SDR50 or DDR50 modes
being used.

The commit claims that SDR50 and DDR50 require clock adjustments in
the SDIO3 Configuration register, which is located via the "conf-sdio3"
resource.  However, when this resource is given, we fail to read the
host capabilities 1 register, resulting in host->caps1 being zero.
Hence, both SDHCI_SUPPORT_SDR50 and SDHCI_SUPPORT_DDR50 bits remain
zero, disabling the SDR50 and DDR50 modes.

The underlying idea in this function appears to be to read the device
capabilities, modify them, and set SDHCI_QUIRK_MISSING_CAPS to cause
our modified capabilities to be used.  Implement exactly that.

Fixes: 1140011ee9d9 ("mmc: sdhci-pxav3: Modify clock settings for the SDR50 and DDR50 modes")
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci-pxav3.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index f5edf9d3a18a..c7f27fe4805a 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -137,6 +137,10 @@ static int armada_38x_quirks(struct platform_device *pdev,
 
 	host->quirks &= ~SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN;
 	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
+
+	host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
+
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 					   "conf-sdio3");
 	if (res) {
@@ -150,7 +154,6 @@ static int armada_38x_quirks(struct platform_device *pdev,
 		 * Configuration register, if the adjustment is not done,
 		 * remove them from the capabilities.
 		 */
-		host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
 		host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50);
 
 		dev_warn(&pdev->dev, "conf-sdio3 register not found: disabling SDR50 and DDR50 modes.\nConsider updating your dtb\n");
@@ -161,7 +164,6 @@ static int armada_38x_quirks(struct platform_device *pdev,
 	 * controller has different capabilities than the ones shown
 	 * in its registers
 	 */
-	host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
 	if (of_property_read_bool(np, "no-1-8-v")) {
 		host->caps &= ~SDHCI_CAN_VDD_180;
 		host->mmc->caps &= ~MMC_CAP_1_8V_DDR;
-- 
2.1.0

[PATCH 18/17] mmc: sdhci: further fix for DMA unmapping in sdhci_post_req()

[Russell King]

sdhci_post_req() exists to unmap a previously mapped but already
finished request, while the next request is in progress.  However, the
state of the SDHCI_REQ_USE_DMA flag depends on the last submitted
request.

This means we can end up clearing the flag due to a quirk, which then
means that sdhci_post_req() fails to unmap the DMA buffer, potentially
leading to data corruption.

We can safely ignore the SDHCI_REQ_USE_DMA here, as testing
data->host_cookie is entirely sufficient.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
This is another fairly important bug fix.

 drivers/mmc/host/sdhci.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index df72d1eb54a4..f877a092c6bc 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2053,13 +2053,12 @@ static void sdhci_post_req(struct mmc_host *mmc, struct mmc_request *mrq,
 	struct sdhci_host *host = mmc_priv(mmc);
 	struct mmc_data *data = mrq->data;
 
-	if (host->flags & SDHCI_REQ_USE_DMA) {
-		if (data->host_cookie != COOKIE_UNMAPPED)
-			dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-					 data->flags & MMC_DATA_WRITE ?
-					 DMA_TO_DEVICE : DMA_FROM_DEVICE);
-		data->host_cookie = COOKIE_UNMAPPED;
-	}
+	if (data->host_cookie != COOKIE_UNMAPPED)
+		dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+			     data->flags & MMC_DATA_WRITE ?
+			       DMA_TO_DEVICE : DMA_FROM_DEVICE);
+
+	data->host_cookie = COOKIE_UNMAPPED;
 }
 
 static void sdhci_pre_req(struct mmc_host *mmc, struct mmc_request *mrq,
-- 
2.1.0

[PATCH 19/17] mmc: sdhci: fix data timeout (part 1)

[Russell King]

The data timeout gives the minimum amount of time that should be
waited before timing out if no data is received from the card.
Simply dividing the nanosecond part by 1000 does not give this
required guarantee, since such a division rounds down.  Use
DIV_ROUND_UP() to give the desired timeout.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/mmc/host/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9ecac99ae7e2..82e799fde113 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -630,7 +630,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 	if (!data)
 		target_timeout = cmd->busy_timeout * 1000;
 	else {
-		target_timeout = data->timeout_ns / 1000;
+		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
 		if (host->clock)
 			target_timeout += data->timeout_clks / host->clock;
 	}
-- 
2.1.0

[PATCH 20/17] mmc: sdhci: fix data timeout (part 2)

[Russell King]

The calculation for the timeout based on the number of card clocks is
incorrect.  The calculation assumed:

	timeout in microseconds = clock cycles / clock in Hz

which is clearly a several orders of magnitude wrong.  Fix this by
multiplying the clock cycles by 1000000 prior to dividing by the Hz
based clock.  Also, as per part 1, ensure that the division rounds
up.

As this needs 64-bit math via do_div(), avoid it if the clock cycles
is zero.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 82e799fde113..65b230ea179e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -631,8 +631,19 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 		target_timeout = cmd->busy_timeout * 1000;
 	else {
 		target_timeout = DIV_ROUND_UP(data->timeout_ns, 1000);
-		if (host->clock)
-			target_timeout += data->timeout_clks / host->clock;
+		if (host->clock && data->timeout_clks) {
+			unsigned long long val;
+
+			/*
+			 * data->timeout_clks is in units of clock cycles.
+			 * host->clock is in Hz.  target_timeout is in us.
+			 * Hence, us = 1000000 * cycles / Hz.  Round up.
+			 */
+			val = 1000000 * data->timeout_clks;
+			if (do_div(val, host->clock))
+				target_timeout++;
+			target_timeout += val;
+		}
 	}
 
 	/*
-- 
2.1.0

Re: [PATCH 00/17] MMC/SDHCI fixes

[Ulf Hansson]

On 19 December 2015 at 21:28, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> This all started with a report from an iMX6 Hummingboard user that
> they were seeing regular "mmc0: Got data interrupt 0x00000002 even
> though no data operation was in progress." messages with 4.4-rc and
> additional patches to enable UHS support in DT for the platform.
>
> When I tested 4.4-rc on an iMX6 Hummingboard with UHS support enabled,
> I found several other issues.  This patch series addresses some of
> the issues.
>
> However, while testing on Armada 388, an issue was found there, and
> a fix is also included in this patch set, however it does not depend
> on the rest of the patch series.
>
> We all know that the SDHCI driver is in poor state, and Ulf's desire
> to see it turned into a library (which actually comes from myself.)
> However, with the driver in such a poor state, it makes sense to fix
> a few of the issues first, especially if they result in the code
> shrinking - which they do.
>
> However, given what I've found below, I have to wonder how many of the
> quirks that we have in this driver are down to the poor code rather than
> real hardware problems: how many of them are due to poorly investigated
> bugs.
>
> This series starts off by shutting up some kernel messages that should
> not be verbose:
> - the voltage-ranges DT property is optional, so we should not print
>   a message saying that it's missing from DT.  That suggests it isn't
>   optional.  As no ill effect comes from not specifying it, I've
>   dropped this message to debug level.
>
> - "retrying because a re-tune was needed" - a re-tune is something that
>   is part of the higher speed SD specs, and it's required to happen
>   periodically.  So it's expected and normal behaviour.  There's no need
>   to print this to the kernel log.
>
> - When tuning fails, rather than a bland "mmc0: tuning execution failed"
>   message, print the error code so that we can see what the reason for
>   the failure is.  MMC fails on this point in many places.
>
> - Move the initialisation of the command ->error member.  It looks to
>   me as if this is not required; mmc_start_request() does this already,
>   as it seems the rest of the MMC core does too.  Maybe this should be
>   removed altogether?
>
> - Testing error bits, then setting the error, then testing the error
>   value, and completing the command is a very lengthy way to deal with
>   errors here.  Test the error bits, and then decide what error occurred
>   before completing.  (This code is actually buggy, see the next change).
>
> - If a command _response_ CRC error occurs, we can't be certain whether
>   the card received the command successfully, and has started processing
>   it.  If it has, and it results in data being read from the card, the
>   card will enter data transfer state, and start sending data to the
>   host.  Therefore, it is completely wrong to blindly terminate the
>   command, especially when such termination fails to clean _anything_
>   up - eg, it leaves DMA in place so the host can continue DMAing to the
>   memory, and it leaves buffers DMA mapped - which eventually annoys the
>   DMA API debugging.  Fix this by assuming that a CRC error is a receive
>   side error.  If the card did not correctly receive the command, it
>   will not initiate a data transfer, and we should time out after the
>   card specified timeout.
>
>   With the unmodified mainline code, even if we reset the state machines
>   when finishing a request in error, the _card_ itself may well still be
>   in data transfer mode when this happens.  How can we be sure what state
>   the data side is actually in?  Could this be why (eg) sdhci-esdhc-imx.c
>   has this:
>
>         /* FIXME: delay a bit for card to be ready for next tuning due to errors */
>         mdelay(1);
>
>   And how many more such _hacks_ are working around similar problems?
>
> - The unaligned buffer handling is a total waste of resources in its
>   current form.  We DMA map, sync, and unmap it on every transfer,
>   whether the buffer is used or not.  For MMC/SD transfers, these will
>   be coming from the VFS/MM layers, which operate through the page
>   cache - hence the vast majority of buffers will be page aligned.
>   Performance measurements on iMX6 show that this additional DMA
>   maintanence is responsible for a 10% reduction in read transfer
>   performance.  Switch this to use a DMA coherent mapping instead, which
>   needs no DMA maintanence.
>
> - sdhci_adma_table_post() is badly coded: we walk the scatterlist
>   looking for any buffers which might not be aligned (which can be a
>   rare event, see above) and then we only do something if we find an
>   unaligned buffer _and_ it's a read transfer.  So for a write transfer,
>   this scatterlist walking is a total waste of CPU cycles.  Testing the
>   transfer direction is much cheaper than walking the scatterlist.
>   Re-organise the code to do the cheap test first.
>
> - There are two paths to identical DMA unmapping code (the code around
>   dma_unmap_sg()) in sdhci_finish_data() and its child function
>   sdhci_adma_table_post().  Trivially simplify this duplication.
>
> - Move sdhci_pre_dma_transfer() so we avoid needing to declare it.
>
> - Do the same thing with sdhci_prepare_data() and sdhci_pre_dma_transfer()
>   as with sdhci_finish_data() and sdhci_adma_table_post().
>
> - Fix the mess that is the data segment host_cookie handling.  The
>   existing code seems like a candidate for the obfuscated coding prize!
>   Four patches address this, turning it into something much easier to
>   understand, which then allows (finally)...
>
> - The DMA API leak which occurs when we have mapped the DMA buffers in
>   sdhci_prepare_data() and an error occurs early on in the processing
>   of a request.
>
> - A final change (mentioned above) which fixes a prior commit for
>   Armada 38x using sdhci-pxav3.c, found while trying to get the higher
>   speed modes working on these SoCs.  Clearly the existing code is
>   broken, this at least resolves some of the breakage by allowing the
>   correct high-order SDHCI capabilities through.
>
> All these patches are resulting from _only_ today; I'm sure more will
> be coming in the forthcoming weeks, as all I seem to have to do is
> give code a little shake and _lots_ of bugs appear.  Despite this being
> a series of 17 patches, it does not yet solve _all_ the problems I've
> found so far on iMX6 with SDHCI.
>
> I've tested these changes on iMX6 at UHS speeds, and Armada 388 SoCs
> at HS speeds so far.  Others should test them.

Thanks for the detailed change-log, really helpful!

I have reviewed the patches, and found only a few minor things.

Related to patch1, I noticed that the callers of
mmc_of_parse_voltage() don't check the return value. Adopting my
suggested change to patch1, would enable them to be able to check the
return code.

Kind regards
Uffe

>
>  drivers/mmc/card/block.c       |   4 +-
>  drivers/mmc/core/core.c        |  10 +-
>  drivers/mmc/host/sdhci-pxav3.c |   6 +-
>  drivers/mmc/host/sdhci.c       | 318 ++++++++++++++++++-----------------------
>  drivers/mmc/host/sdhci.h       |   4 +-
>  5 files changed, 157 insertions(+), 185 deletions(-)
>
> --
> RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

Re: [PATCH 00/17] MMC/SDHCI fixes

[Russell King - ARM Linux]

On Mon, Dec 21, 2015 at 11:46:32AM +0100, Ulf Hansson wrote:
> Related to patch1, I noticed that the callers of
> mmc_of_parse_voltage() don't check the return value. Adopting my
> suggested change to patch1, would enable them to be able to check the
> return code.

I disagree with that, because you make the "no voltage-ranges specified"
case appear to be successful.  What if a driver wants it to be mandatory?
You force them down the complicated path of checking the resulting OCR
which is horrid.

It would be better to return some other error code if it's missing, or
maybe return zero for "no voltage-ranges specified" and one for a
successfully parsed "voltage-range" specifier.  This would allow these
cases to be identified by users of this API.

I've gone with that, new patch series will follow when I get a moment
to prepare it, thanks.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.