| Message ID | 1542853562-19018-1-git-send-email-haibo.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mmc: sdhci-of-esdhc: fix unchecked return value issue | expand |
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@nxp.com> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > --- > 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? > + 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; >
On Thu, 22 Nov 2018 at 09:29, Adrian Hunter <adrian.hunter@intel.com> 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@nxp.com> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > --- > > 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. > > > + 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
Hi, > -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Friday, December 14, 2018 6:54 PM > To: BOUGH CHEN <haibo.chen@nxp.com> > Cc: linux-mmc@vger.kernel.org; Adrian Hunter <adrian.hunter@intel.com>; > Y.B. LU <yangbo.lu@nxp.com>; dl-linux-imx <linux-imx@nxp.com> > 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@intel.com> > 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@nxp.com> > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > > --- > > > 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
On 19/12/18 9:05 AM, Y.B. LU wrote: > Hi, > >> -----Original Message----- >> From: Ulf Hansson <ulf.hansson@linaro.org> >> Sent: Friday, December 14, 2018 6:54 PM >> To: BOUGH CHEN <haibo.chen@nxp.com> >> Cc: linux-mmc@vger.kernel.org; Adrian Hunter <adrian.hunter@intel.com>; >> Y.B. LU <yangbo.lu@nxp.com>; dl-linux-imx <linux-imx@nxp.com> >> 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@intel.com> >> 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@nxp.com> >>>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com> >>>> --- >>>> 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
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)); + 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;