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