diff mbox series

[V1] mmc: sdhci: Check for reset prior to DMA address unmap

Message ID 1614163985-38914-1-git-send-email-pragalla@codeaurora.org
State New
Headers show
Series [V1] mmc: sdhci: Check for reset prior to DMA address unmap | expand

Commit Message

Pradeep P V K Feb. 24, 2021, 10:53 a.m. UTC
For data read commands, SDHC may initiate data transfers even before it
completely process the command response. In case command itself fails,
driver un-maps the memory associated with data transfer but this memory
can still be accessed by SDHC for the already initiated data transfer.
This scenario can lead to un-mapped memory access error.

To avoid this scenario, reset SDHC (when command fails) prior to
un-mapping memory. Resetting SDHC ensures that all in-flight data
transfers are either aborted or completed. So we don't run into this
scenario.

Swap the reset, un-map steps sequence in sdhci_request_done().

Suggested-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
Signed-off-by: Pradeep P V K <pragalla@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 58 ++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

Comments

Adrian Hunter March 3, 2021, 7:01 a.m. UTC | #1
On 24/02/21 12:53 pm, Pradeep P V K wrote:
> For data read commands, SDHC may initiate data transfers even before it

> completely process the command response. In case command itself fails,

> driver un-maps the memory associated with data transfer but this memory

> can still be accessed by SDHC for the already initiated data transfer.

> This scenario can lead to un-mapped memory access error.

> 

> To avoid this scenario, reset SDHC (when command fails) prior to

> un-mapping memory. Resetting SDHC ensures that all in-flight data

> transfers are either aborted or completed. So we don't run into this

> scenario.

> 

> Swap the reset, un-map steps sequence in sdhci_request_done().

> 

> Suggested-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>

> Signed-off-by: Pradeep P V K <pragalla@codeaurora.org>


Seems like a good change to make.  A couple of cosmetic tweaks below,
but:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>


> ---

>  drivers/mmc/host/sdhci.c | 58 ++++++++++++++++++++++++------------------------

>  1 file changed, 29 insertions(+), 29 deletions(-)

> 

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c

> index 646823d..e78d84c 100644

> --- a/drivers/mmc/host/sdhci.c

> +++ b/drivers/mmc/host/sdhci.c

> @@ -2996,6 +2996,35 @@ static bool sdhci_request_done(struct sdhci_host *host)

>  		spin_unlock_irqrestore(&host->lock, flags);

>  		return true;

>  	}


Blank line here please.

> +	/*

> +	 * The controller needs a reset of internal state machines

> +	 * upon error conditions.

> +	 */

> +	if (sdhci_needs_reset(host, mrq)) {

> +		/*

> +		 * Do not finish until command and data lines are available for

> +		 * reset. Note there can only be one other mrq, so it cannot

> +		 * also be in mrqs_done, otherwise host->cmd and host->data_cmd

> +		 * would both be null.

> +		 */

> +		if (host->cmd || host->data_cmd) {

> +			spin_unlock_irqrestore(&host->lock, flags);

> +			return true;

> +		}

> +

> +		/* Some controllers need this kick or reset won't work here */

> +		if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)

> +			/* This is to force an update */

> +			host->ops->set_clock(host, host->clock);

> +

> +		/* Spec says we should do both at the same time, but Ricoh

> +		 * controllers do not like that.

> +		 */


Please change comment style:

		/*
		 * Spec says we should do both at the same time, but Ricoh
		 * controllers do not like that.
		 */

> +		sdhci_do_reset(host, SDHCI_RESET_CMD);

> +		sdhci_do_reset(host, SDHCI_RESET_DATA);

> +

> +		host->pending_reset = false;

> +	}

>  

>  	/*

>  	 * Always unmap the data buffers if they were mapped by

> @@ -3060,35 +3089,6 @@ static bool sdhci_request_done(struct sdhci_host *host)

>  		}

>  	}

>  

> -	/*

> -	 * The controller needs a reset of internal state machines

> -	 * upon error conditions.

> -	 */

> -	if (sdhci_needs_reset(host, mrq)) {

> -		/*

> -		 * Do not finish until command and data lines are available for

> -		 * reset. Note there can only be one other mrq, so it cannot

> -		 * also be in mrqs_done, otherwise host->cmd and host->data_cmd

> -		 * would both be null.

> -		 */

> -		if (host->cmd || host->data_cmd) {

> -			spin_unlock_irqrestore(&host->lock, flags);

> -			return true;

> -		}

> -

> -		/* Some controllers need this kick or reset won't work here */

> -		if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)

> -			/* This is to force an update */

> -			host->ops->set_clock(host, host->clock);

> -

> -		/* Spec says we should do both at the same time, but Ricoh

> -		   controllers do not like that. */

> -		sdhci_do_reset(host, SDHCI_RESET_CMD);

> -		sdhci_do_reset(host, SDHCI_RESET_DATA);

> -

> -		host->pending_reset = false;

> -	}

> -

>  	host->mrqs_done[i] = NULL;

>  

>  	spin_unlock_irqrestore(&host->lock, flags);

>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 646823d..e78d84c 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2996,6 +2996,35 @@  static bool sdhci_request_done(struct sdhci_host *host)
 		spin_unlock_irqrestore(&host->lock, flags);
 		return true;
 	}
+	/*
+	 * The controller needs a reset of internal state machines
+	 * upon error conditions.
+	 */
+	if (sdhci_needs_reset(host, mrq)) {
+		/*
+		 * Do not finish until command and data lines are available for
+		 * reset. Note there can only be one other mrq, so it cannot
+		 * also be in mrqs_done, otherwise host->cmd and host->data_cmd
+		 * would both be null.
+		 */
+		if (host->cmd || host->data_cmd) {
+			spin_unlock_irqrestore(&host->lock, flags);
+			return true;
+		}
+
+		/* Some controllers need this kick or reset won't work here */
+		if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
+			/* This is to force an update */
+			host->ops->set_clock(host, host->clock);
+
+		/* Spec says we should do both at the same time, but Ricoh
+		 * controllers do not like that.
+		 */
+		sdhci_do_reset(host, SDHCI_RESET_CMD);
+		sdhci_do_reset(host, SDHCI_RESET_DATA);
+
+		host->pending_reset = false;
+	}
 
 	/*
 	 * Always unmap the data buffers if they were mapped by
@@ -3060,35 +3089,6 @@  static bool sdhci_request_done(struct sdhci_host *host)
 		}
 	}
 
-	/*
-	 * The controller needs a reset of internal state machines
-	 * upon error conditions.
-	 */
-	if (sdhci_needs_reset(host, mrq)) {
-		/*
-		 * Do not finish until command and data lines are available for
-		 * reset. Note there can only be one other mrq, so it cannot
-		 * also be in mrqs_done, otherwise host->cmd and host->data_cmd
-		 * would both be null.
-		 */
-		if (host->cmd || host->data_cmd) {
-			spin_unlock_irqrestore(&host->lock, flags);
-			return true;
-		}
-
-		/* Some controllers need this kick or reset won't work here */
-		if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
-			/* This is to force an update */
-			host->ops->set_clock(host, host->clock);
-
-		/* Spec says we should do both at the same time, but Ricoh
-		   controllers do not like that. */
-		sdhci_do_reset(host, SDHCI_RESET_CMD);
-		sdhci_do_reset(host, SDHCI_RESET_DATA);
-
-		host->pending_reset = false;
-	}
-
 	host->mrqs_done[i] = NULL;
 
 	spin_unlock_irqrestore(&host->lock, flags);