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 <[email protected]>
Signed-off-by: Pradeep P V K <[email protected]>
---
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;
}
+ /*
+ * 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);
--
2.7.4
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 <[email protected]>
> Signed-off-by: Pradeep P V K <[email protected]>
Seems like a good change to make. A couple of cosmetic tweaks below,
but:
Acked-by: Adrian Hunter <[email protected]>
> ---
> 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);
>