On Wed, 3 Mar 2021 at 09:32, Pradeep P V K <pragalla@xxxxxxxxxxxxxxxx> wrote:
>
> From: Pradeep P V K <pragalla@xxxxxxxxxxxxxx>
>
> 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 <vbadigan@xxxxxxxxxxxxxx>
> Signed-off-by: Pradeep P V K <pragalla@xxxxxxxxxxxxxx>
> Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Seems like it might be a good idea to tag this for stable? I did that,
but awaiting for your confirmation.
So, applied for next, thanks!
Kind regards
Uffe
> ---
> 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
>
