Re: [PATCH V1] mmc: sdhci: Check for reset prior to DMA address unmap

[Adrian Hunter <adrian.hunter@intel.com>]

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 <vbadigan@codeaurora.org>
> Signed-off-by: Pradeep P V K <pragalla@codeaurora.org>

Seems like a good change to make.  A couple of cosmetic tweaks below,
but:

Acked-by: Adrian Hunter <adrian.hunter@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);
>