Re: [PATCH 1/2] mmc: sdhci: Properly set DMA mask

[Adrian Hunter]

On 4/01/19 12:47 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
> 
> The implementation of sdhci_set_dma_mask() is conflating two things: on
> one hand it uses the SDHCI_USE_64_BIT_DMA flag to determine whether or
> not to use the 64-bit addressing capability of the controller and on the
> other hand it also uses that flag to set a DMA mask for the controller's
> parent device.
> 
> However, a controller supporting 64-bit addressing doesn't mean that it
> needs to support addressing 64 bits of address range. It's perfectly
> acceptable to use 64-bit addressing for a 32-bit address range or even
> smaller, even if that makes little sense, considering the extra overhead
> of the 64-bit addressing descriptors.
> 
> But it is fairly common for hardware to support somewhere between 32 and
> 64 bits of address range. Tegra124 and Tegra210, for example, support 34
> bits and the newer Tegra186 and Tegra194 support 40 bits. The latter can
> also use an IOMMU for address translation, which has an input address
> range of 48 bits. This causes problems with the current algorithm in the
> SDHCI core for choosing the DMA mask. If the DMA mask is set to 64 bits,
> the DMA memory allocations can (and usually do because the allocator
> starts from the top) end up beyond the 40 bit boundary addressable by
> the SDHCI controller, causing IOMMU faults.
> 
> For Tegra specifically this problem is currently worked around by
> setting the SDHCI_QUIRK2_BROKEN_64_BIT_DMA quirk. This causes the DMA
> mask to always be set to 32 bits and therefore all allocations will fit
> within the range addressable by the controller.
> 
> This commit reworks the code in sdhci_set_dma_mask() to fix the above
> issue. The rationale behind this change is that the SDHCI controller
> driver should be the authoritative source of the DMA mask setting. The
> SDHCI core has no way of knowing what the individual SDHCI controllers
> are capable of. So instead of overriding the DMA mask depending on
> whether or not 64-bit addressing mode is used, the DMA mask is only
> modified if absolutely necessary. On one hand, if the controller can
> only address 32 bits of memory or less, we disable use of 64-bit
> addressing mode because it is not needed. On the other hand, we also
> want to make sure that if we don't support 64-bit addressing mode, such
> as in the case where the BROKEN_64_BIT_DMA quirk is set, we do restrict
> the DMA mask to fit the capabilities. The latter is an inconsistency by
> the driver, so we warn about it to make sure it will be addressed in the
> driver.

sdhci_set_dma_mask() was added because people did want sdhci to set the DMA
mask.  Also using 64-bit DMA even with 32-bit systems has the advantage of
reducing exposure to problems i.e. the same logic is used with the same SoC
irrespective of whether or not it is in 32-bit compatibility mode.  So the
policy for sdhci is always to use 64-bit DMA if it is supported.

I suggest we add a new sdhci op for ->set_dma_mask() and call that instead
of sdhci_set_dma_mask() if it is not NULL.

> 
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
>  drivers/mmc/host/sdhci.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 7c6c93e85b7e..01f81e96be23 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3499,27 +3499,35 @@ static int sdhci_set_dma_mask(struct sdhci_host *host)
>  {
>  	struct mmc_host *mmc = host->mmc;
>  	struct device *dev = mmc_dev(mmc);
> -	int ret = -EINVAL;
> +	u64 dma_mask = dma_get_mask(dev);
> +	u64 dma32 = DMA_BIT_MASK(32);
> +	int ret = 0;
>  
>  	if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA)
>  		host->flags &= ~SDHCI_USE_64_BIT_DMA;
>  
> -	/* Try 64-bit mask if hardware is capable  of it */
> -	if (host->flags & SDHCI_USE_64_BIT_DMA) {
> -		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> -		if (ret) {
> -			pr_warn("%s: Failed to set 64-bit DMA mask.\n",
> -				mmc_hostname(mmc));
> -			host->flags &= ~SDHCI_USE_64_BIT_DMA;
> -		}
> +	/*
> +	 * Hardware that can't address more than the 32-bit address range does
> +	 * not need to use 64-bit addressing mode, even if it supports it.
> +	 */
> +	if ((host->flags & SDHCI_USE_64_BIT_DMA) && (dma_mask <= dma32)) {
> +		pr_debug("%s: controller needs addresses <= 32-bits\n",
> +			mmc_hostname(mmc));
> +		host->flags &= ~SDHCI_USE_64_BIT_DMA;
>  	}
>  
> -	/* 32-bit mask as default & fallback */
> -	if (ret) {
> -		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +	/*
> +	 * If the hardware doesn't support 64-bit addressing, make sure to
> +	 * restrict the DMA mask so we don't get buffers allocated beyond the
> +	 * 32-bit boundary.
> +	 */
> +	if (!(host->flags & SDHCI_USE_64_BIT_DMA) && (dma_mask > dma32)) {
> +		WARN(1, "64-bit DMA not supported, DMA mask %llx\n", dma_mask);
> +
> +		ret = dma_set_mask_and_coherent(dev, dma32);
>  		if (ret)
> -			pr_warn("%s: Failed to set 32-bit DMA mask.\n",
> -				mmc_hostname(mmc));
> +			pr_warn("%s: failed to set 32-bit DMA mask: %d\n",
> +				mmc_hostname(mmc), ret);
>  	}
>  
>  	return ret;
>