On 19/12/18 9:05 AM, Y.B. LU wrote:
> Hi,
>
>> -----Original Message-----
>> From: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>> Sent: Friday, December 14, 2018 6:54 PM
>> To: BOUGH CHEN <haibo.chen@xxxxxxx>
>> Cc: linux-mmc@xxxxxxxxxxxxxxx; Adrian Hunter <adrian.hunter@xxxxxxxxx>;
>> Y.B. LU <yangbo.lu@xxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>
>> Subject: Re: [PATCH v2] mmc: sdhci-of-esdhc: fix unchecked return value issue
>>
>> On Thu, 22 Nov 2018 at 09:29, Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> wrote:
>>>
>>> On 22/11/18 4:20 AM, BOUGH CHEN wrote:
>>>> Calling dma_set_mask_and_coherent without checking return value.
>>>> This was caught by coverity scan.
>>>>
>>>> Fix this by check the return value, and give a warning if get a
>>>> false.
>>>>
>>>> Acked-by: Yangbo Lu <Yangbo.lu@xxxxxxx>
>>>> Signed-off-by: Haibo Chen <haibo.chen@xxxxxxx>
>>>> ---
>>>> drivers/mmc/host/sdhci-of-esdhc.c | 10 ++++++++--
>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
>>>> b/drivers/mmc/host/sdhci-of-esdhc.c
>>>> index 86fc9f0..51513fd 100644
>>>> --- a/drivers/mmc/host/sdhci-of-esdhc.c
>>>> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
>>>> @@ -475,11 +475,17 @@ static void esdhc_of_adma_workaround(struct
>>>> sdhci_host *host, u32 intmask) static int
>>>> esdhc_of_enable_dma(struct sdhci_host *host) {
>>>> u32 value;
>>>> + int ret;
>>>> 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));
>>>
>>> Why isn't the dma mask set up during initialization?
>>
>> I agree with Adrian, that this is probably what you should do, at least long
>> term.
>>
>> However, my understanding of this is that you want a way to fallback to PIO
>> mode, in case failing to set the dma mask, no? Anyway, then you need to
>> return the error code, otherwise that won't happen.
>>
>
> [Y.b. Lu] sdhci_set_dma_mask() is for dma mask setting.
> Although it may break common sdhci_set_dma_mask() to handle such case(I don’t think it's very good), I have to ask below suggestion.
> Could we accept to make sdhci_set_dma_mask() as a callback of mmc_host_ops to allow vendor driver to define it? Or add a quirk for 40bit dma mask?
What about changing to use sdhci_setup_host() and __sdhci_add_host(), and
then doing dma_set_mask_and_coherent() between them?
>
> BTW, I will confirm with Laurentiu privately who set 40bit dma mask whether there was doc for this problem, since I didn’t notice it.
> Thanks.
>
>>>
>>>> + if (ret) {
>>>> + pr_warn("%s: Failed to set 40-bit DMA mask.\n",
>>>> + mmc_hostname(host->mmc));
>>>> + }
>>>> + }
>>>>
>>>> value = sdhci_readl(host, ESDHC_DMA_SYSCTL);
>>>> value |= ESDHC_DMA_SNOOP;
>>>>
>>>
>>
>> Kind regards
>> Uffe
