From:   Jiasheng Jiang <jiasheng@iscas.ac.cn>
To:     adrian.hunter@intel.com, ulf.hansson@linaro.org
Cc:     linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jiasheng Jiang <jiasheng@iscas.ac.cn>
Subject: Re: Re: [PATCH] mmc: sdhci-of-esdhc: Check for error num after setting mask
Date:   Wed, 12 Jan 2022 16:17:15 +0800
Message-Id: <20220112081715.692436-1-jiasheng@iscas.ac.cn>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, Jan 12, 2022 at 02:45:13PM +0800, Adrian Hunter 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?

Thanks, because the original esdhc_of_enable_dma() only returns 0, the
caller may not consider to check the return value.
I also notice that the esdhc_of_enable_dma() is assigned to
sdhci_esdhc_le_pdata and sdhci_esdhc_be_pdata, which is only used by
sdhci_esdhc_driver.
And now the sdhci_esdhc_driver only have 'probe' and 'remove', without
other action.
So we should not consider to check whether there is a caller for
esdhc_of_enable_dma() in sdhci_esdhc_driver.

>>  	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.

Fine, since it is already checked in setup, I think it is no need to
check later.

I will send a v2 without the change of esdhc_of_resume().

Sincerely thanks,
Jiang