Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1614760331-43499-1-git-send-email-pragalla@qti.qualcomm.com>
In-Reply-To: <1614760331-43499-1-git-send-email-pragalla@qti.qualcomm.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 4 Mar 2021 14:49:55 +0100
Message-ID: <CAPDyKFqFNr7AiOdVP07XS=CKpMbDKC7n0gMPu0516fgH3=S18Q@mail.gmail.com>
Subject: Re: [PATCH V2] mmc: sdhci: Check for reset prior to DMA address unmap
To:     Pradeep P V K <pragalla@qti.qualcomm.com>
Cc:     Adrian Hunter <adrian.hunter@intel.com>,
        Asutosh Das <asutoshd@codeaurora.org>,
        Sahitya Tummala <stummala@codeaurora.org>,
        Ram Prakash Gupta <rampraka@codeaurora.org>,
        Veerabhadrarao Badiganti <vbadigan@codeaurora.org>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Pradeep P V K <pragalla@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 3 Mar 2021 at 09:32, Pradeep P V K <pragalla@qti.qualcomm.com> wrote:
>
> From: Pradeep P V K <pragalla@codeaurora.org>
>
> 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@codeaurora.org>
> Signed-off-by: Pradeep P V K <pragalla@codeaurora.org>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

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
>