linux-kernel - Re: 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>] [day] [month] [year] [list]

Date:   Wed, 12 Jan 2022 16:17:15 +0800
From:   Jiasheng Jiang <jiasheng@...as.ac.cn>
To:     adrian.hunter@...el.com, ulf.hansson@...aro.org
Cc:     linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jiasheng Jiang <jiasheng@...as.ac.cn>
Subject: Re: Re: [PATCH] mmc: sdhci-of-esdhc: Check for error num after setting mask

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