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.
Fixes: 5552d7ad596c ("mmc: sdhci-of-esdhc: set proper dma mask for ls104x chips")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
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;
+
sdhci_writel(host, esdhc_proctl, SDHCI_HOST_CONTROL);
}
return ret;
--
2.25.1
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 <[email protected]>
> ---
> 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;
>
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