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 <vbadi...@codeaurora.org>
> Signed-off-by: Pradeep P V K <praga...@codeaurora.org>
Seems like a good change to make. A couple of cosmetic tweaks below,
but:
Acked-by: Adrian Hunter <adrian.hun...@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);
>
