linux-kernel - Re: [PATCH] mmc: sdhci-of-esdhc: Check for error num after setting mask

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ae5373d2-372a-5eb6-1b2a-7b1dae888355@intel.com>
Date:   Wed, 12 Jan 2022 09:06:35 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Jiasheng Jiang <jiasheng@...as.ac.cn>, ulf.hansson@...aro.org
Cc:     linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mmc: sdhci-of-esdhc: Check for error num after setting
 mask

On 06/01/2022 04:16, Jiasheng Jiang wrote:
> Because of the possible failure of the dma_supported(), the
> dma_set_mask_and_coherent() may return error num.
> Therefore, it should be better to check it and return the error if
> fails.
> Also, the caller, esdhc_of_resume(), should deal with the return value.
> Moreover, as the sdhci_esdhc_driver has not been used, it does not need to
> be considered.

Apologies, but that last sentence I don't understand.  Can you clarify it a bit.
What doesn't need to be considered and why?

> 
> Fixes: 5552d7ad596c ("mmc: sdhci-of-esdhc: set proper dma mask for ls104x chips")
> Signed-off-by: Jiasheng Jiang <jiasheng@...as.ac.cn>
> ---
>  drivers/mmc/host/sdhci-of-esdhc.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index a593b1fbd69e..bedfc7bb5174 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -524,12 +524,16 @@ static void esdhc_of_adma_workaround(struct sdhci_host *host, u32 intmask)
>  
>  static int esdhc_of_enable_dma(struct sdhci_host *host)
>  {
> +	int ret;
>  	u32 value;
>  	struct device *dev = mmc_dev(host->mmc);
>  
>  	if (of_device_is_compatible(dev->of_node, "fsl,ls1043a-esdhc") ||
> -	    of_device_is_compatible(dev->of_node, "fsl,ls1046a-esdhc"))
> -		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40));
> +	    of_device_is_compatible(dev->of_node, "fsl,ls1046a-esdhc")) {
> +		ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40));
> +		if (ret)
> +			return ret;
> +	}
>  
>  	value = sdhci_readl(host, ESDHC_DMA_SYSCTL);
>  
> @@ -1245,7 +1249,10 @@ static int esdhc_of_resume(struct device *dev)
>  
>  	if (ret == 0) {
>  		/* Isn't this already done by sdhci_resume_host() ? --rmk */
> -		esdhc_of_enable_dma(host);
> +		ret = esdhc_of_enable_dma(host);
> +		if (ret)
> +			return ret;
> +

This is already done by sdhci_resume_host(), which assumes there can be no
error if DMA has been enabled previously i.e. -> enable_dma() is called
at setup and the return value checked then.  If it is possible that DMA
support can disappear later, then it would be better to address that in
SDHCI so that all SDHCI drivers get the benefit.

>  		sdhci_writel(host, esdhc_proctl, SDHCI_HOST_CONTROL);
>  	}
>  	return ret;
>