On Thu, 4 Mar 2021 at 16:16, [off-list ref] wrote:

On 2021-03-04 19:19, Ulf Hansson wrote:

quoted

On Wed, 3 Mar 2021 at 09:32, Pradeep P V K [off-list ref]
wrote:

quoted

From: Pradeep P V K <redacted>

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

Changes since V1:
- Added an empty line and fixed the comment style.
- Retained the Acked-by signoff.

Suggested-by: Veerabhadrarao Badiganti <redacted>
Signed-off-by: Pradeep P V K <redacted>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Hi Uffe,

quoted

Seems like it might be a good idea to tag this for stable? I did that,
but awaiting for your confirmation.

Yes, this fix is applicable for all stable starting from 4.9 (n/a for
4.4).
Kindly go ahead.

quoted

So, applied for next, thanks!

Kind regards
Uffe

Thanks and Regards,
Pradeep

Thanks for confirming, I have updated the stable tag.

Kind regards
Uffe

quoted

quoted

---
 drivers/mmc/host/sdhci.c | 60
+++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 646823d..130fd2d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c

@@ -2998,6 +2998,37 @@ 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;
+       }
+
+       /*
         * 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.

@@ -3060,35 +3091,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