[PATCH v2 00/24] MMC/SDHCI fixes

[PATCH v2 00/24] 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.

- reworking the voltage-ranges code so that callers know whether the
  optional voltage-ranges property was specified.

- "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 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.

- Fixing another DMA API leak with the pre/post request handling, where
  a current request can change the state of the SDHCI_REQ_USE_DMA, and
  thus stop a mapped request being unmapped.

- Fixing the SDHCI timeout calculation code to correctly round timeouts
  up rather than down, and correctly calculate the time (in microseconds)
  for a certain number of card clocks.

- Consolidating the SDHCI address/size alignment handling so we only walk
  the scatterlist once instead of twice.

Further patches are necessary as there are still a number of observable
problems when using this driver on iMX6 with UHS cards.  Patches up to
and including "mmc: sdhci: further fix for DMA unmapping in
sdhci_post_req()" should be considered for inclusion in the stable
kernel trees.

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        |  17 +-
 drivers/mmc/host/sdhci-pxav3.c |   6 +-
 drivers/mmc/host/sdhci.c       | 417 +++++++++++++++++++----------------------
 drivers/mmc/host/sdhci.h       |   4 +-
 5 files changed, 208 insertions(+), 240 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.


-- 
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 v2 01/24] 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 v2 02/24] mmc: core: improve mmc_of_parse_voltage() to return better status

[Russell King]

Improve mmc_of_parse_voltage()'s return values so that drivers can tell
whether a voltage-range specification was present, and whether it has
been successfully parsed, or there was an error while parsing.

We return a negative errno when parsing fails, zero if no voltage-range
specification is present, or one if a voltage-range specification is
successfully parsed.

No users need modifying as no users check the return value.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b5e663b67cb5..e698c08c7a67 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1210,8 +1210,9 @@ EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
  * @np: The device node need to be parsed.
  * @mask: mask of voltages available for MMC/SD/SDIO
  *
- * 1. Return zero on success.
- * 2. Return negative errno: voltage-range is invalid.
+ * Parse the "voltage-ranges" DT property, returning zero if it is not
+ * found, negative errno if the voltage-range specification is invalid,
+ * or one if the voltage-range is specified and successfully parsed.
  */
 int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 {
@@ -1222,7 +1223,7 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 	num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
 	if (!voltage_ranges) {
 		pr_debug("%s: voltage-ranges unspecified\n", np->full_name);
-		return -EINVAL;
+		return 0;
 	}
 	if (!num_ranges) {
 		pr_err("%s: voltage-ranges empty\n", np->full_name);
@@ -1244,7 +1245,7 @@ int mmc_of_parse_voltage(struct device_node *np, u32 *mask)
 		*mask |= ocr_mask;
 	}
 
-	return 0;
+	return 1;
 }
 EXPORT_SYMBOL(mmc_of_parse_voltage);
 
-- 
2.1.0

[PATCH v2 03/24] 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 v2 04/24] 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 e698c08c7a67..a88fccb6ffe9 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 v2 05/24] 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 v2 06/24] 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 v2 07/24] 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

Re: [PATCH v2 07/24] mmc: sdhci: command response CRC error handling

[Adrian Hunter]

On 21/12/15 13:40, Russell King wrote:
> 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

Pedantically, if the timeout bit is set as well (CMD line conflict),
it does mean the card did not receive the command, so it should be coded
that way.

> 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

"processed normally" confused me at first because it sounded like you are
ignoring the error.  Not sure why you have a much better explanation in the
cover email than here.

> the status of all commands and data transfer phases of the request.

MMC core is not the only initiator of requests, but it is safe because the
command error takes precedence by design.

Also you don't explain why it is better to continue rather than attempt to
send a stop command and clean up the request properly.  It looks simpler and
less racy, but if that is the reason then it seems worth saying so.

> 
> 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.

This is misleading.  We could terminate the request early if we cleaned it
up.  You should say here why it is better to continue.

> +		 *
> +		 * 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.

This FIXME is too vague.  Please give at least one example of what needs fixing.

> +		 */
> +		if (host->cmd->data && intmask & SDHCI_INT_CRC) {
> +			host->cmd = NULL;
> +			return;
> +		}
> +
>  		tasklet_schedule(&host->finish_tasklet);
>  		return;
>  	}
> 

Re: [PATCH v2 07/24] mmc: sdhci: command response CRC error handling

[Russell King - ARM Linux]

On Tue, Dec 29, 2015 at 03:08:20PM +0200, Adrian Hunter wrote:
> On 21/12/15 13:40, Russell King wrote:
> > 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
> 
> Pedantically, if the timeout bit is set as well (CMD line conflict),
> it does mean the card did not receive the command, so it should be coded
> that way.

Good catch, the SDHCI spec contains a table which describes the CRC and
timeout bit states, though it's not quite as you describe above...
CRC and timeout indicates a command line conflict at some point.

> > 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
> 
> "processed normally" confused me at first because it sounded like you are
> ignoring the error.  Not sure why you have a much better explanation in the
> cover email than here.

They're written at different times?  I don't accept your comment though -
"record the failure" _clearly_ does not mean that we're ignoring the error.

> > the status of all commands and data transfer phases of the request.
> 
> MMC core is not the only initiator of requests, but it is safe because the
> command error takes precedence by design.
> 
> Also you don't explain why it is better to continue rather than attempt to
> send a stop command and clean up the request properly.  It looks simpler and
> less racy, but if that is the reason then it seems worth saying so.

This patch results from the analysis of failures seen on iMX6 hardware,
where the card has entered data mode, and started to send its data.
Right now, this screws up the next command.

> > 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.
> 
> This is misleading.  We could terminate the request early if we cleaned it
> up.  You should say here why it is better to continue.

That is _not_ misleading, it is entirely accurate.  What the code
currently does when it encounters a CRC error is it terminates the
_request_ early.  The _request_ being "struct mmc_request" - and
it terminates it _without_ sending a STOP command.

Resetting the host controller does not influence what state the card
is in.

So what happens at the moment is that we send a command which initiates
a data phase from the card.  The card responds with a valid response,
and starts sending data to the host.  The host incorrectly receives
the card response with a CRC error.

At this point, the code decides that it had a failure, queues the
finish tasklet, which resets the SDHCI controller, leaving the card
transmitting data to the host, potentially endlessly.  The driver
reports to the MMC layer that the mmc_request is complete, and we
get the next request to process.

We try sending the next request to the card, but the card is still
sending data to the host...  That's the problem here.

Yes, sending a STOP command is one solution, but that's a far bigger
change, one which is likely to be far more buggy based on the fact
that the driver can send the STOP automatically.

> 
> > +		 *
> > +		 * 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.
> 
> This FIXME is too vague.  Please give at least one example of what
> needs fixing.

I don't remember anymore, sorry.  I'll delete the fixme. :)

-- 
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 v2 07/24] mmc: sdhci: command response CRC error handling

[Adrian Hunter]

On 02/01/16 14:25, Russell King - ARM Linux wrote:
> On Tue, Dec 29, 2015 at 03:08:20PM +0200, Adrian Hunter wrote:
>> On 21/12/15 13:40, Russell King wrote:
>>> 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
>>
>> Pedantically, if the timeout bit is set as well (CMD line conflict),
>> it does mean the card did not receive the command, so it should be coded
>> that way.
> 
> Good catch, the SDHCI spec contains a table which describes the CRC and
> timeout bit states, though it's not quite as you describe above...
> CRC and timeout indicates a command line conflict at some point.

In the case of CMD line conflict, the host controller aborts the command, so
presumably there will not be any data timeout.  Will you change it?

> 
>>> 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
>>
>> "processed normally" confused me at first because it sounded like you are
>> ignoring the error.  Not sure why you have a much better explanation in the
>> cover email than here.
> 
> They're written at different times?  I don't accept your comment though -
> "record the failure" _clearly_ does not mean that we're ignoring the error.
> 
>>> the status of all commands and data transfer phases of the request.
>>
>> MMC core is not the only initiator of requests, but it is safe because the
>> command error takes precedence by design.
>>
>> Also you don't explain why it is better to continue rather than attempt to
>> send a stop command and clean up the request properly.  It looks simpler and
>> less racy, but if that is the reason then it seems worth saying so.
> 
> This patch results from the analysis of failures seen on iMX6 hardware,
> where the card has entered data mode, and started to send its data.
> Right now, this screws up the next command.
> 
>>> 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.
>>
>> This is misleading.  We could terminate the request early if we cleaned it
>> up.  You should say here why it is better to continue.
> 
> That is _not_ misleading, it is entirely accurate.  What the code
> currently does when it encounters a CRC error is it terminates the
> _request_ early.  The _request_ being "struct mmc_request" - and
> it terminates it _without_ sending a STOP command.

Sure, but the person reading the comment not should have to know the history
of the code to interpret it.  But it is not a big thing - the comment could
just be:

	We must not terminate early because we don't bother to clean up.

> 
> Resetting the host controller does not influence what state the card
> is in.
> 
> So what happens at the moment is that we send a command which initiates
> a data phase from the card.  The card responds with a valid response,
> and starts sending data to the host.  The host incorrectly receives
> the card response with a CRC error.
> 
> At this point, the code decides that it had a failure, queues the
> finish tasklet, which resets the SDHCI controller, leaving the card
> transmitting data to the host, potentially endlessly.  The driver
> reports to the MMC layer that the mmc_request is complete, and we
> get the next request to process.
> 
> We try sending the next request to the card, but the card is still
> sending data to the host...  That's the problem here.
> 
> Yes, sending a STOP command is one solution, but that's a far bigger
> change, one which is likely to be far more buggy based on the fact
> that the driver can send the STOP automatically.
> 
>>
>>> +		 *
>>> +		 * 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.
>>
>> This FIXME is too vague.  Please give at least one example of what
>> needs fixing.
> 
> I don't remember anymore, sorry.  I'll delete the fixme. :)
> 

Re: [PATCH v2 07/24] mmc: sdhci: command response CRC error handling

[Russell King - ARM Linux]

On Mon, Jan 04, 2016 at 01:24:34PM +0200, Adrian Hunter wrote:
> On 02/01/16 14:25, Russell King - ARM Linux wrote:
> > On Tue, Dec 29, 2015 at 03:08:20PM +0200, Adrian Hunter wrote:
> >> On 21/12/15 13:40, Russell King wrote:
> >>> 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
> >>
> >> Pedantically, if the timeout bit is set as well (CMD line conflict),
> >> it does mean the card did not receive the command, so it should be coded
> >> that way.
> > 
> > Good catch, the SDHCI spec contains a table which describes the CRC and
> > timeout bit states, though it's not quite as you describe above...
> > CRC and timeout indicates a command line conflict at some point.
> 
> In the case of CMD line conflict, the host controller aborts the command, so
> presumably there will not be any data timeout.  Will you change it?

Of course, I think that's what I implied above, because CRC + timeout
does *not* mean that we had a CRC error.

> >>> +		/*
> >>> +		 * 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.
> >>
> >> This is misleading.  We could terminate the request early if we cleaned it
> >> up.  You should say here why it is better to continue.
> > 
> > That is _not_ misleading, it is entirely accurate.  What the code
> > currently does when it encounters a CRC error is it terminates the
> > _request_ early.  The _request_ being "struct mmc_request" - and
> > it terminates it _without_ sending a STOP command.
> 
> Sure, but the person reading the comment not should have to know the history
> of the code to interpret it.  But it is not a big thing - the comment could
> just be:
> 
> 	We must not terminate early because we don't bother to clean up.

I think that "we don't bother to clean up" is ambiguous.  We do clean
up the request - and that's part of the problem.  We clear out the
drivers state, reset the host controller command and data paths, and
tear down DMA mappings and the like leaving the card in data transfer
mode.

In this particular case (of a tuning command) I don't believe a stop
command would be expected (it's not a multi-block read.)

In any case, I think the original comment describes what we're doing,
the only change I'll make is to replace "request" with "mmc_request"
which should solve your confusion.

-- 
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 v2 08/24] 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 maintenance 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 v2 08/24] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Adrian Hunter]

On 21/12/15 13:41, Russell King 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.
> 
> Eliminate this by switching the align buffer to be a DMA coherent
> buffer, which needs no DMA maintenance to access the buffer.

Did you consider putting the align buffer in the same allocation
as the adma_table?

> 
> 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);

kmalloc line is still there

>  		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);

It would be nicer not to have any BUG_ON()

>  	}
>  
>  	/*
> @@ -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;
> 

Re: [PATCH v2 08/24] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Russell King - ARM Linux]

On Tue, Dec 29, 2015 at 03:44:38PM +0200, Adrian Hunter wrote:
> On 21/12/15 13:41, Russell King 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.
> > 
> > Eliminate this by switching the align buffer to be a DMA coherent
> > buffer, which needs no DMA maintenance to access the buffer.
> 
> Did you consider putting the align buffer in the same allocation
> as the adma_table?

It's not clear whether host->adma_table_sz would be appropriately
aligned.

> > @@ -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);
> 
> kmalloc line is still there

Good catch, thanks.

> > +
> > +		/* dma_alloc_coherent returns page aligned and sized buffers */
> > +		BUG_ON(host->align_addr & host->align_mask);
> 
> It would be nicer not to have any BUG_ON()

This is a situation that should _never_ occur (if it does, then the
dma_alloc_coherent() implementation is violating the API requirements,
which are to return a page-sized page-aligned buffer.)  I guess it
could be a WARN_ON(), but if it fails we're likely to cause data
corruption.  So, a WARN_ON() and failing the probe seems more
appropriate - but then that means yet more messy cleanup in an already
messy part of the driver.

-- 
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 v2 08/24] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Russell King - ARM Linux]

On Sat, Jan 02, 2016 at 12:29:19PM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 29, 2015 at 03:44:38PM +0200, Adrian Hunter wrote:
> > On 21/12/15 13:41, Russell King 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.
> > > 
> > > Eliminate this by switching the align buffer to be a DMA coherent
> > > buffer, which needs no DMA maintenance to access the buffer.
> > 
> > Did you consider putting the align buffer in the same allocation
> > as the adma_table?
> 
> It's not clear whether host->adma_table_sz would be appropriately
> aligned.

Looking at this closer, it would not be appropriately aligned to place
the alignment buffer after the adma table, but placing the alignment
buffer in the allocation first would give appropriate alignment.

The buffer sizes are:

Table		32-bit	64-bit
alignment	512	1024		(entry sz * 128)
adma		2056	3084		(desc sz * (128 * 2 + 1))

Allocating the two together gives advantages and disadvantages:

* for 32-bit address sizes, instead of allocating two order-0 pages,
  we end up allocating one order-0 page, so halve the coherent DMA
  allocation of the driver.

* for 64-bit address sizes, we allocate one order-1 page instead,
  which may be more prone to failure with a 4k page size, and it
  also means the ADMA table will overlap a page boundary.  I've no
  idea whether that is an issue for any SDHCI controllers or not.

I could add additional complexity to do different allocations in the
32-bit and 64-bit paths, but that results in a _far_ more complicated
cleanup path, and much more prone to errors.

In any case, such a patch should be entirely separate from _this_
patch given that such a change may cause breakage and could need
to be reworked as a result.  Indeed, it's a _separate_ change in
itself, for a different purpose from _this_ patch.

-- 
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 v2 08/24] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Adrian Hunter]

On 02/01/16 16:31, Russell King - ARM Linux wrote:
> On Sat, Jan 02, 2016 at 12:29:19PM +0000, Russell King - ARM Linux wrote:
>> On Tue, Dec 29, 2015 at 03:44:38PM +0200, Adrian Hunter wrote:
>>> On 21/12/15 13:41, Russell King 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.
>>>>
>>>> Eliminate this by switching the align buffer to be a DMA coherent
>>>> buffer, which needs no DMA maintenance to access the buffer.
>>>
>>> Did you consider putting the align buffer in the same allocation
>>> as the adma_table?
>>
>> It's not clear whether host->adma_table_sz would be appropriately
>> aligned.
> 
> Looking at this closer, it would not be appropriately aligned to place
> the alignment buffer after the adma table, but placing the alignment
> buffer in the allocation first would give appropriate alignment.
> 
> The buffer sizes are:
> 
> Table		32-bit	64-bit
> alignment	512	1024		(entry sz * 128)
> adma		2056	3084		(desc sz * (128 * 2 + 1))
> 
> Allocating the two together gives advantages and disadvantages:
> 
> * for 32-bit address sizes, instead of allocating two order-0 pages,
>   we end up allocating one order-0 page, so halve the coherent DMA
>   allocation of the driver.
> 
> * for 64-bit address sizes, we allocate one order-1 page instead,
>   which may be more prone to failure with a 4k page size, and it
>   also means the ADMA table will overlap a page boundary.  I've no
>   idea whether that is an issue for any SDHCI controllers or not.
> 
> I could add additional complexity to do different allocations in the
> 32-bit and 64-bit paths, but that results in a _far_ more complicated
> cleanup path, and much more prone to errors.
> 
> In any case, such a patch should be entirely separate from _this_
> patch given that such a change may cause breakage and could need
> to be reworked as a result.  Indeed, it's a _separate_ change in
> itself, for a different purpose from _this_ patch.

Agreed.

Re: [PATCH v2 08/24] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Adrian Hunter]

On 02/01/16 14:29, Russell King - ARM Linux wrote:
> On Tue, Dec 29, 2015 at 03:44:38PM +0200, Adrian Hunter wrote:
>> On 21/12/15 13:41, Russell King 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.
>>>
>>> Eliminate this by switching the align buffer to be a DMA coherent
>>> buffer, which needs no DMA maintenance to access the buffer.
>>
>> Did you consider putting the align buffer in the same allocation
>> as the adma_table?
> 
> It's not clear whether host->adma_table_sz would be appropriately
> aligned.
> 
>>> @@ -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);
>>
>> kmalloc line is still there
> 
> Good catch, thanks.
> 
>>> +
>>> +		/* dma_alloc_coherent returns page aligned and sized buffers */
>>> +		BUG_ON(host->align_addr & host->align_mask);
>>
>> It would be nicer not to have any BUG_ON()
> 
> This is a situation that should _never_ occur (if it does, then the
> dma_alloc_coherent() implementation is violating the API requirements,
> which are to return a page-sized page-aligned buffer.)  I guess it
> could be a WARN_ON(), but if it fails we're likely to cause data
> corruption.  So, a WARN_ON() and failing the probe seems more
> appropriate - but then that means yet more messy cleanup in an already
> messy part of the driver.

Isn't there already a cleanup path? i.e.

		} else if (host->adma_addr & host->align_mask) {
			pr_warn("%s: unable to allocate aligned ADMA descriptor\n",
				mmc_hostname(mmc));
			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);
			host->adma_table = NULL;
			host->align_buffer = NULL;
		}

Re: [PATCH v2 08/24] mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

[Russell King - ARM Linux]

On Mon, Jan 04, 2016 at 01:50:32PM +0200, Adrian Hunter wrote:
> On 02/01/16 14:29, Russell King - ARM Linux wrote:
> > On Tue, Dec 29, 2015 at 03:44:38PM +0200, Adrian Hunter wrote:
> >> On 21/12/15 13:41, Russell King 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.
> >>>
> >>> Eliminate this by switching the align buffer to be a DMA coherent
> >>> buffer, which needs no DMA maintenance to access the buffer.
> >>
> >> Did you consider putting the align buffer in the same allocation
> >> as the adma_table?
> > 
> > It's not clear whether host->adma_table_sz would be appropriately
> > aligned.
> > 
> >>> @@ -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);
> >>
> >> kmalloc line is still there
> > 
> > Good catch, thanks.
> > 
> >>> +
> >>> +		/* dma_alloc_coherent returns page aligned and sized buffers */
> >>> +		BUG_ON(host->align_addr & host->align_mask);
> >>
> >> It would be nicer not to have any BUG_ON()
> > 
> > This is a situation that should _never_ occur (if it does, then the
> > dma_alloc_coherent() implementation is violating the API requirements,
> > which are to return a page-sized page-aligned buffer.)  I guess it
> > could be a WARN_ON(), but if it fails we're likely to cause data
> > corruption.  So, a WARN_ON() and failing the probe seems more
> > appropriate - but then that means yet more messy cleanup in an already
> > messy part of the driver.
> 
> Isn't there already a cleanup path? i.e.
> 
> 		} else if (host->adma_addr & host->align_mask) {
> 			pr_warn("%s: unable to allocate aligned ADMA descriptor\n",
> 				mmc_hostname(mmc));
> 			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);
> 			host->adma_table = NULL;
> 			host->align_buffer = NULL;
> 		}

It means duplicating this, because the resulting warning would not be
suitable for the alignment buffer.  I'm no fan of misleading printk()s
or duplicating cleanup, especially when the situation should never
occur.

In any case, with the two allocations combined, the BUG_ON() gets
removed again.

-- 
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 v2 09/24] 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 v2 10/24] 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 v2 11/24] 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 v2 12/24] 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 v2 13/24] 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 v2 14/24] 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 v2 15/24] 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 v2 16/24] 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 v2 17/24] 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 v2 18/24] 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

Re: [PATCH v2 18/24] mmc: sdhci-pxav3: fix higher speed mode capabilities

[Marcin Wojtas]

Hi Russel

2015-12-21 12:41 GMT+01:00 Russell King <rmk+kernel@arm.linux.org.uk>:
> 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
>

Thanks for the fix.

Reviewed-by: Marcin Wojtas <mw@semihalf.com>

Best regards,
Marcin

[PATCH v2 19/24] 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>
---
 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 4ead8b2f8f8f..9ecac99ae7e2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2068,13 +2068,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 v2 20/24] 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 v2 21/24] 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

[PATCH v2 22/24] mmc: sdhci: prepare DMA address/size quirk handling consolidation

[Russell King]

Prepare to consolidate the DMA address/size quirk handling into one
single loop.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 65b230ea179e..474d96943560 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -730,21 +730,22 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	 * scatterlist.
 	 */
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		int broken, i;
 		struct scatterlist *sg;
+		unsigned int length_mask;
+		int i;
 
-		broken = 0;
+		length_mask = 0;
 		if (host->flags & SDHCI_USE_ADMA) {
 			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE)
-				broken = 1;
+				length_mask = 3;
 		} else {
 			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_SIZE)
-				broken = 1;
+				length_mask = 3;
 		}
 
-		if (unlikely(broken)) {
+		if (unlikely(length_mask)) {
 			for_each_sg(data->sg, sg, data->sg_len, i) {
-				if (sg->length & 0x3) {
+				if (sg->length & length_mask) {
 					DBG("Reverting to PIO because of "
 						"transfer size (%d)\n",
 						sg->length);
@@ -760,10 +761,11 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	 * translation to device address space.
 	 */
 	if (host->flags & SDHCI_REQ_USE_DMA) {
-		int broken, i;
 		struct scatterlist *sg;
+		unsigned int offset_mask;
+		int i;
 
-		broken = 0;
+		offset_mask = 0;
 		if (host->flags & SDHCI_USE_ADMA) {
 			/*
 			 * As we use 3 byte chunks to work around
@@ -771,15 +773,15 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 			 * quirk.
 			 */
 			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE)
-				broken = 1;
+				offset_mask = 3;
 		} else {
 			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
-				broken = 1;
+				offset_mask = 3;
 		}
 
-		if (unlikely(broken)) {
+		if (unlikely(offset_mask)) {
 			for_each_sg(data->sg, sg, data->sg_len, i) {
-				if (sg->offset & 0x3) {
+				if (sg->offset & offset_mask) {
 					DBG("Reverting to PIO because of "
 						"bad alignment\n");
 					host->flags &= ~SDHCI_REQ_USE_DMA;
-- 
2.1.0

[PATCH v2 23/24] mmc: sdhci: consolidate the DMA/ADMA size/address quicks

[Russell King]

Rather than scanning the scatterlist multiple times for each quirk,
scan it once, checking for each possible quirk.  This should be
cheaper due to the length and offset members commonly sharing the
same cache line than scanning the scatterlist multiple times.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 474d96943560..ce1259f2add3 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -728,22 +728,35 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	/*
 	 * FIXME: This doesn't account for merging when mapping the
 	 * scatterlist.
+	 *
+	 * The assumption here being that alignment and lengths are
+	 * the same after DMA mapping to device address space.
 	 */
 	if (host->flags & SDHCI_REQ_USE_DMA) {
 		struct scatterlist *sg;
-		unsigned int length_mask;
+		unsigned int length_mask, offset_mask;
 		int i;
 
 		length_mask = 0;
+		offset_mask = 0;
 		if (host->flags & SDHCI_USE_ADMA) {
-			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE)
+			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE) {
 				length_mask = 3;
+				/*
+				 * As we use up to 3 byte chunks to work
+				 * around alignment problems, we need to
+				 * check the offset as well.
+				 */
+				offset_mask = 3;
+			}
 		} else {
 			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_SIZE)
 				length_mask = 3;
+			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
+				offset_mask = 3;
 		}
 
-		if (unlikely(length_mask)) {
+		if (unlikely(length_mask | offset_mask)) {
 			for_each_sg(data->sg, sg, data->sg_len, i) {
 				if (sg->length & length_mask) {
 					DBG("Reverting to PIO because of "
@@ -752,35 +765,6 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 					host->flags &= ~SDHCI_REQ_USE_DMA;
 					break;
 				}
-			}
-		}
-	}
-
-	/*
-	 * The assumption here being that alignment is the same after
-	 * translation to device address space.
-	 */
-	if (host->flags & SDHCI_REQ_USE_DMA) {
-		struct scatterlist *sg;
-		unsigned int offset_mask;
-		int i;
-
-		offset_mask = 0;
-		if (host->flags & SDHCI_USE_ADMA) {
-			/*
-			 * As we use 3 byte chunks to work around
-			 * alignment problems, we need to check this
-			 * quirk.
-			 */
-			if (host->quirks & SDHCI_QUIRK_32BIT_ADMA_SIZE)
-				offset_mask = 3;
-		} else {
-			if (host->quirks & SDHCI_QUIRK_32BIT_DMA_ADDR)
-				offset_mask = 3;
-		}
-
-		if (unlikely(offset_mask)) {
-			for_each_sg(data->sg, sg, data->sg_len, i) {
 				if (sg->offset & offset_mask) {
 					DBG("Reverting to PIO because of "
 						"bad alignment\n");
-- 
2.1.0

[PATCH v2 24/24] mmc: sdhci: further code simplication

[Russell King]

Further simplify the code in sdhci_prepare_data() - we don't set
SDHCI_REQ_USE_DMA anywhere else in the driver, so there is no
need to set it, and then immediately test it.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ce1259f2add3..dde388837891 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -722,21 +722,20 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	host->data_early = 0;
 	host->data->bytes_xfered = 0;
 
-	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))
-		host->flags |= SDHCI_REQ_USE_DMA;
-
-	/*
-	 * FIXME: This doesn't account for merging when mapping the
-	 * scatterlist.
-	 *
-	 * The assumption here being that alignment and lengths are
-	 * the same after DMA mapping to device address space.
-	 */
-	if (host->flags & SDHCI_REQ_USE_DMA) {
+	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
 		struct scatterlist *sg;
 		unsigned int length_mask, offset_mask;
 		int i;
 
+		host->flags |= SDHCI_REQ_USE_DMA;
+
+		/*
+		 * FIXME: This doesn't account for merging when mapping the
+		 * scatterlist.
+		 *
+		 * The assumption here being that alignment and lengths are
+		 * the same after DMA mapping to device address space.
+		 */
 		length_mask = 0;
 		offset_mask = 0;
 		if (host->flags & SDHCI_USE_ADMA) {
-- 
2.1.0

Re: [PATCH v2 00/24] MMC/SDHCI fixes

[Ulf Hansson]

On 21 December 2015 at 12:39, 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.
>
> - reworking the voltage-ranges code so that callers know whether the
>   optional voltage-ranges property was specified.
>
> - "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 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.
>
> - Fixing another DMA API leak with the pre/post request handling, where
>   a current request can change the state of the SDHCI_REQ_USE_DMA, and
>   thus stop a mapped request being unmapped.
>
> - Fixing the SDHCI timeout calculation code to correctly round timeouts
>   up rather than down, and correctly calculate the time (in microseconds)
>   for a certain number of card clocks.
>
> - Consolidating the SDHCI address/size alignment handling so we only walk
>   the scatterlist once instead of twice.
>
> Further patches are necessary as there are still a number of observable
> problems when using this driver on iMX6 with UHS cards.  Patches up to
> and including "mmc: sdhci: further fix for DMA unmapping in
> sdhci_post_req()" should be considered for inclusion in the stable
> kernel trees.
>
> 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        |  17 +-
>  drivers/mmc/host/sdhci-pxav3.c |   6 +-
>  drivers/mmc/host/sdhci.c       | 417 +++++++++++++++++++----------------------
>  drivers/mmc/host/sdhci.h       |   4 +-
>  5 files changed, 208 insertions(+), 240 deletions(-)

This looks good to me!

I decided to try to apply it for my next branch, to get some good test
coverage. Although, it failed when reaching patch 8. Would you mind
posting yet another new re-based version, please.

Thanks!

Kind regards
Uffe

Re: [PATCH v2 00/24] MMC/SDHCI fixes

[Russell King - ARM Linux]

On Mon, Dec 21, 2015 at 01:35:36PM +0100, Ulf Hansson wrote:
> I decided to try to apply it for my next branch, to get some good test
> coverage. Although, it failed when reaching patch 8. Would you mind
> posting yet another new re-based version, please.

Given that these are _fixes_ and need to be applied to -rc kernels, they
are based on -rc6.  I don't see the point of rebasing them onto non-rc
kernels to test, because then you're not testing against where they
should be applied.

-- 
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 v2 00/24] MMC/SDHCI fixes

[Ulf Hansson]

On 21 December 2015 at 13:51, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Dec 21, 2015 at 01:35:36PM +0100, Ulf Hansson wrote:
>> I decided to try to apply it for my next branch, to get some good test
>> coverage. Although, it failed when reaching patch 8. Would you mind
>> posting yet another new re-based version, please.
>
> Given that these are _fixes_ and need to be applied to -rc kernels, they
> are based on -rc6.  I don't see the point of rebasing them onto non-rc
> kernels to test, because then you're not testing against where they
> should be applied.

I see your point, but I would rather not aim for rcs with these
changes, unless you insist.

Not because they aren't fixes, but because it's old errors. Instead we
can "cc stable" or use the fixes tag as we are in quite late stage of
the rc. This approach will also allow us to get a bit better test
coverage.

What do you think?

Kind regards
Uffe

Re: [PATCH v2 00/24] MMC/SDHCI fixes

[Russell King - ARM Linux]

On Mon, Dec 21, 2015 at 02:23:17PM +0100, Ulf Hansson wrote:
> On 21 December 2015 at 13:51, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Dec 21, 2015 at 01:35:36PM +0100, Ulf Hansson wrote:
> >> I decided to try to apply it for my next branch, to get some good test
> >> coverage. Although, it failed when reaching patch 8. Would you mind
> >> posting yet another new re-based version, please.
> >
> > Given that these are _fixes_ and need to be applied to -rc kernels, they
> > are based on -rc6.  I don't see the point of rebasing them onto non-rc
> > kernels to test, because then you're not testing against where they
> > should be applied.
> 
> I see your point, but I would rather not aim for rcs with these
> changes, unless you insist.
> 
> Not because they aren't fixes, but because it's old errors. Instead we
> can "cc stable" or use the fixes tag as we are in quite late stage of
> the rc. This approach will also allow us to get a bit better test
> coverage.

Even if we do this, we still need to get _these_ tested because _these_
will be the ones which need to be applied to stable trees such as the
4.4 stable series.

What I suggest is applying them against -rc6, and then merging them
into your -next branch, and fixing the resulting conflicts.  We then
have the patches ready for -rc, but which can still be tested in -next
(for what that's worth.)  If people find problems, they can always
re-test these patches without all the development stuff.

If I were to give you a set of patches suitable just for -next, then
people can only test with all the development stuff included, and we'll
have no idea whether any new problems are the result of development or
these patches.

-- 
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 v2 00/24] MMC/SDHCI fixes

[Ulf Hansson]

On 21 December 2015 at 14:41, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Dec 21, 2015 at 02:23:17PM +0100, Ulf Hansson wrote:
>> On 21 December 2015 at 13:51, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Dec 21, 2015 at 01:35:36PM +0100, Ulf Hansson wrote:
>> >> I decided to try to apply it for my next branch, to get some good test
>> >> coverage. Although, it failed when reaching patch 8. Would you mind
>> >> posting yet another new re-based version, please.
>> >
>> > Given that these are _fixes_ and need to be applied to -rc kernels, they
>> > are based on -rc6.  I don't see the point of rebasing them onto non-rc
>> > kernels to test, because then you're not testing against where they
>> > should be applied.
>>
>> I see your point, but I would rather not aim for rcs with these
>> changes, unless you insist.
>>
>> Not because they aren't fixes, but because it's old errors. Instead we
>> can "cc stable" or use the fixes tag as we are in quite late stage of
>> the rc. This approach will also allow us to get a bit better test
>> coverage.
>
> Even if we do this, we still need to get _these_ tested because _these_
> will be the ones which need to be applied to stable trees such as the
> 4.4 stable series.
>
> What I suggest is applying them against -rc6, and then merging them
> into your -next branch, and fixing the resulting conflicts.  We then
> have the patches ready for -rc, but which can still be tested in -next
> (for what that's worth.)  If people find problems, they can always
> re-test these patches without all the development stuff.
>
> If I were to give you a set of patches suitable just for -next, then
> people can only test with all the development stuff included, and we'll
> have no idea whether any new problems are the result of development or
> these patches.

Okay! I will publish the branch as soon as I can.

Kind regards
Uffe

Re: [PATCH v2 00/24] MMC/SDHCI fixes

[Ulf Hansson]

On 21 December 2015 at 14:59, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 21 December 2015 at 14:41, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Mon, Dec 21, 2015 at 02:23:17PM +0100, Ulf Hansson wrote:
>>> On 21 December 2015 at 13:51, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk> wrote:
>>> > On Mon, Dec 21, 2015 at 01:35:36PM +0100, Ulf Hansson wrote:
>>> >> I decided to try to apply it for my next branch, to get some good test
>>> >> coverage. Although, it failed when reaching patch 8. Would you mind
>>> >> posting yet another new re-based version, please.
>>> >
>>> > Given that these are _fixes_ and need to be applied to -rc kernels, they
>>> > are based on -rc6.  I don't see the point of rebasing them onto non-rc
>>> > kernels to test, because then you're not testing against where they
>>> > should be applied.
>>>
>>> I see your point, but I would rather not aim for rcs with these
>>> changes, unless you insist.
>>>
>>> Not because they aren't fixes, but because it's old errors. Instead we
>>> can "cc stable" or use the fixes tag as we are in quite late stage of
>>> the rc. This approach will also allow us to get a bit better test
>>> coverage.
>>
>> Even if we do this, we still need to get _these_ tested because _these_
>> will be the ones which need to be applied to stable trees such as the
>> 4.4 stable series.
>>
>> What I suggest is applying them against -rc6, and then merging them
>> into your -next branch, and fixing the resulting conflicts.  We then
>> have the patches ready for -rc, but which can still be tested in -next
>> (for what that's worth.)  If people find problems, they can always
>> re-test these patches without all the development stuff.
>>
>> If I were to give you a set of patches suitable just for -next, then
>> people can only test with all the development stuff included, and we'll
>> have no idea whether any new problems are the result of development or
>> these patches.
>
> Okay! I will publish the branch as soon as I can.

Huh. The merge ends up in a quite complicated conflict to resolve for
me. And I am running out of time.

I have queued a quite limited amount of sdhci patches for this cycle,
so indeed I think it's good opportunity to try out your changes right
now.

In particular, it's the following patch which I have queued for next
that conflicts with your changes.
"mmc: sdhci: 64-bit DMA actually has 4-byte alignment"

Could you perhaps reconsider to rebase your patches on top of my next
branch. I have also rebased my next branch on top of rc6.

Kind regards
Uffe

Re: [PATCH v2 00/24] MMC/SDHCI fixes

[Russell King - ARM Linux]

On Tue, Dec 22, 2015 at 12:25:53PM +0100, Ulf Hansson wrote:
> Huh. The merge ends up in a quite complicated conflict to resolve for
> me. And I am running out of time.

Sorry, I'm out of time too - I've still a number of things to do
commercially, I have to get several pull requests out that I've promised
to get out a few weeks back, and I still need to sort out some remaining
ARM patches in the patch system - some of which have been sent by people
too eager, so need cross-referencing with what's been discussed on the
mailing lists.

Today is likely to be the last day I do anything time-consuming on the
kernel before Christmas, so I'm afraid a rebase will have to wait until
the new year.

-- 
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 v2 00/24] MMC/SDHCI fixes

[Russell King - ARM Linux]

On Mon, Dec 21, 2015 at 11:39:56AM +0000, Russell King - ARM Linux wrote:
> - Fixing the SDHCI timeout calculation code to correctly round timeouts
>   up rather than down, and correctly calculate the time (in microseconds)
>   for a certain number of card clocks.

What I didn't point out, but should have, is the obvious issue.

Given that the overall effect of the currently broken code is that the
calculated timeout microsecond value is smaller than it should be, and
in the case of a card which specifies a number of clock cycles, it will
be _substantially_ smaller than it should be, I think that _all_ users
of SDHCI_QUIRK_BROKEN_TIMEOUT_VAL are suspect mis-diagnosis of this
driver bug.

Once this series goes in, I'm intending to completely rip out the
SDHCI_QUIRK_BROKEN_TIMEOUT_VAL quirk, so diagnosis of any problems can
restart from a correct timeout calculation, and we will see how many
SDHCI implementations _actually_ have an issue.

If anyone knows for certain that the timeout hardware is broken, then
they need to reply to this point.  Otherwise, it would be useful if
people could try removing this quirk with these fixes in place, and
submit patches removing it from the drivers which are fixed by this
change.

-- 
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.