| Message ID | 20170730181051.733168837@cogentembedded.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Forgot to mention that the patch is against the 'fixes' branch of Vinod's 'slave-dma.git' repo. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jul 30, 2017 at 09:10:44PM +0300, Sergei Shtylyov wrote: > of_irq_get() may return 0 as well as negative error number on failure, > while the driver only checks for the negative values. The driver would then > call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get > valid channel interrupt. > > Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe > iff of_irq_get() returned 0. > > Fixes: f46b195799b5 ("dmaengine: tegra-adma: Add support for Tegra210 ADMA") > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > drivers/dma/tegra210-adma.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Yeah, that interface isn't very optimal. The problem with it, and we've had this kind before, is that every driver ends up having to implement a fallback for == 0 with the result of everyone returning a different error code. Perhaps a good long-term solution would be to fix of_irq_get() to only return positive for valid interrupts and negative error codes, so that nobody has to deal with == 0 anymore. That's obviously going to be a fairly big undertaking, so I'm fine with fixing up the individual callsites first. > Index: slave-dma/drivers/dma/tegra210-adma.c > =================================================================== > --- slave-dma.orig/drivers/dma/tegra210-adma.c > +++ slave-dma/drivers/dma/tegra210-adma.c > @@ -717,8 +717,8 @@ static int tegra_adma_probe(struct platf > tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i); > > tdc->irq = of_irq_get(pdev->dev.of_node, i); > - if (tdc->irq < 0) { > - ret = tdc->irq; > + if (tdc->irq <= 0) { > + ret = tdc->irq ?: -ENXIO; > goto irq_dispose; > } > I think in this particular case it would be better to just call platform_get_irq(), which already implements the equivalent. Thierry
Hello! On 7/31/2017 12:30 PM, Thierry Reding wrote: >> of_irq_get() may return 0 as well as negative error number on failure, >> while the driver only checks for the negative values. The driver would then >> call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get >> valid channel interrupt. >> >> Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe >> iff of_irq_get() returned 0. >> >> Fixes: f46b195799b5 ("dmaengine: tegra-adma: Add support for Tegra210 ADMA") >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> drivers/dma/tegra210-adma.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > Yeah, that interface isn't very optimal. The problem with it, and we've > had this kind before, is that every driver ends up having to implement a > fallback for == 0 with the result of everyone returning a different > error code. Agreed. > Perhaps a good long-term solution would be to fix of_irq_get() to only > return positive for valid interrupts and negative error codes, so that > nobody has to deal with == 0 anymore. See http://marc.info/?t=146447243300002&r=1 for my attempt to do this back in 2016. > That's obviously going to be a fairly big undertaking, so I'm fine with > fixing up the individual callsites first. > >> Index: slave-dma/drivers/dma/tegra210-adma.c >> =================================================================== >> --- slave-dma.orig/drivers/dma/tegra210-adma.c >> +++ slave-dma/drivers/dma/tegra210-adma.c >> @@ -717,8 +717,8 @@ static int tegra_adma_probe(struct platf >> tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i); >> >> tdc->irq = of_irq_get(pdev->dev.of_node, i); >> - if (tdc->irq < 0) { >> - ret = tdc->irq; >> + if (tdc->irq <= 0) { >> + ret = tdc->irq ?: -ENXIO; >> goto irq_dispose; >> } >> > > I think in this particular case it would be better to just call > platform_get_irq(), which already implements the equivalent. OK, I'll try looking into that... > Thierry MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2017 12:57 PM, Sergei Shtylyov wrote: >>> of_irq_get() may return 0 as well as negative error number on failure, >>> while the driver only checks for the negative values. The driver would then >>> call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get >>> valid channel interrupt. >>> >>> Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe >>> iff of_irq_get() returned 0. >>> >>> Fixes: f46b195799b5 ("dmaengine: tegra-adma: Add support for Tegra210 ADMA") >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [...] >>> Index: slave-dma/drivers/dma/tegra210-adma.c >>> =================================================================== >>> --- slave-dma.orig/drivers/dma/tegra210-adma.c >>> +++ slave-dma/drivers/dma/tegra210-adma.c >>> @@ -717,8 +717,8 @@ static int tegra_adma_probe(struct platf >>> tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i); >>> tdc->irq = of_irq_get(pdev->dev.of_node, i); >>> - if (tdc->irq < 0) { >>> - ret = tdc->irq; >>> + if (tdc->irq <= 0) { >>> + ret = tdc->irq ?: -ENXIO; >>> goto irq_dispose; >>> } >>> >> >> I think in this particular case it would be better to just call >> platform_get_irq(), which already implements the equivalent. Not that platform_get_irq() was no better (if not worse indeed, as it could return 0 both on failure ad success), until I fixed it back in 2016: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e330b9a6bb35dc7097a4f02cb1ae7b6f96df92af > > OK, I'll try looking into that... I've looked at the commit history and no, I don't agree to use platform_get_irq(): my platform_get_irq() fix landed in mainline later than this driver, so fixing this bug in stable kernels would ensue an extra dependency... >> Thierry MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 31, 2017 at 03:59:35PM +0300, Sergei Shtylyov wrote: > On 07/31/2017 12:57 PM, Sergei Shtylyov wrote: > > > > > of_irq_get() may return 0 as well as negative error number on failure, > > > > while the driver only checks for the negative values. The driver would then > > > > call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get > > > > valid channel interrupt. > > > > > > > > Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe > > > > iff of_irq_get() returned 0. > > > > > > > > Fixes: f46b195799b5 ("dmaengine: tegra-adma: Add support for Tegra210 ADMA") > > > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > [...] > > > > > Index: slave-dma/drivers/dma/tegra210-adma.c > > > > =================================================================== > > > > --- slave-dma.orig/drivers/dma/tegra210-adma.c > > > > +++ slave-dma/drivers/dma/tegra210-adma.c > > > > @@ -717,8 +717,8 @@ static int tegra_adma_probe(struct platf > > > > tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i); > > > > tdc->irq = of_irq_get(pdev->dev.of_node, i); > > > > - if (tdc->irq < 0) { > > > > - ret = tdc->irq; > > > > + if (tdc->irq <= 0) { > > > > + ret = tdc->irq ?: -ENXIO; > > > > goto irq_dispose; > > > > } > > > > > > > > > > I think in this particular case it would be better to just call > > > platform_get_irq(), which already implements the equivalent. > > Not that platform_get_irq() was no better (if not worse indeed, as it > could return 0 both on failure ad success), until I fixed it back in 2016: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e330b9a6bb35dc7097a4f02cb1ae7b6f96df92af > > > > > OK, I'll try looking into that... > > I've looked at the commit history and no, I don't agree to use > platform_get_irq(): my platform_get_irq() fix landed in mainline later than > this driver, so fixing this bug in stable kernels would ensue an extra > dependency... Well, surely the platform_get_irq() function should also be fixed in stable kernels, so that all callers can be fixed at the same time. That said, this does fix a bug, and it fixes it in the least invasive way, and the damage I'm complaining about is preexisting, so: Acked-by: Thierry Reding <treding@nvidia.com>
On 07/31/2017 07:56 PM, Thierry Reding wrote: >>>>> of_irq_get() may return 0 as well as negative error number on failure, >>>>> while the driver only checks for the negative values. The driver would then >>>>> call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get >>>>> valid channel interrupt. >>>>> >>>>> Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe >>>>> iff of_irq_get() returned 0. >>>>> >>>>> Fixes: f46b195799b5 ("dmaengine: tegra-adma: Add support for Tegra210 ADMA") >>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> [...] >> >>>>> Index: slave-dma/drivers/dma/tegra210-adma.c >>>>> =================================================================== >>>>> --- slave-dma.orig/drivers/dma/tegra210-adma.c >>>>> +++ slave-dma/drivers/dma/tegra210-adma.c >>>>> @@ -717,8 +717,8 @@ static int tegra_adma_probe(struct platf >>>>> tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i); >>>>> tdc->irq = of_irq_get(pdev->dev.of_node, i); >>>>> - if (tdc->irq < 0) { >>>>> - ret = tdc->irq; >>>>> + if (tdc->irq <= 0) { >>>>> + ret = tdc->irq ?: -ENXIO; >>>>> goto irq_dispose; >>>>> } >>>>> >>>> >>>> I think in this particular case it would be better to just call >>>> platform_get_irq(), which already implements the equivalent. >> Not that platform_get_irq() was no better (if not worse indeed, as it s/Not/Note/. >> could return 0 both on failure ad success), until I fixed it back in 2016: s/ad/and/. >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e330b9a6bb35dc7097a4f02cb1ae7b6f96df92af >> >>> OK, I'll try looking into that... >> >> I've looked at the commit history and no, I don't agree to use >> platform_get_irq(): my platform_get_irq() fix landed in mainline later than >> this driver, so fixing this bug in stable kernels would ensue an extra >> dependency... > > Well, surely the platform_get_irq() function should also be fixed in > stable kernels, so that all callers can be fixed at the same time. It has been fixed in 3.16.y, 4.4.y and 4.8.y so far. I haven't received other stable mails yet. > That said, this does fix a bug, and it fixes it in the least invasive > way, and the damage I'm complaining about is preexisting, so: > > Acked-by: Thierry Reding <treding@nvidia.com> Thank you! MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31/07/17 17:56, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, Jul 31, 2017 at 03:59:35PM +0300, Sergei Shtylyov wrote: >> On 07/31/2017 12:57 PM, Sergei Shtylyov wrote: >> >>>>> of_irq_get() may return 0 as well as negative error number on failure, >>>>> while the driver only checks for the negative values. The driver would then >>>>> call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get >>>>> valid channel interrupt. >>>>> >>>>> Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe >>>>> iff of_irq_get() returned 0. >>>>> >>>>> Fixes: f46b195799b5 ("dmaengine: tegra-adma: Add support for Tegra210 ADMA") >>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> [...] >> >>>>> Index: slave-dma/drivers/dma/tegra210-adma.c >>>>> =================================================================== >>>>> --- slave-dma.orig/drivers/dma/tegra210-adma.c >>>>> +++ slave-dma/drivers/dma/tegra210-adma.c >>>>> @@ -717,8 +717,8 @@ static int tegra_adma_probe(struct platf >>>>> tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i); >>>>> tdc->irq = of_irq_get(pdev->dev.of_node, i); >>>>> - if (tdc->irq < 0) { >>>>> - ret = tdc->irq; >>>>> + if (tdc->irq <= 0) { >>>>> + ret = tdc->irq ?: -ENXIO; >>>>> goto irq_dispose; >>>>> } >>>>> >>>> >>>> I think in this particular case it would be better to just call >>>> platform_get_irq(), which already implements the equivalent. >> >> Not that platform_get_irq() was no better (if not worse indeed, as it >> could return 0 both on failure ad success), until I fixed it back in 2016: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e330b9a6bb35dc7097a4f02cb1ae7b6f96df92af >> >>> >>> OK, I'll try looking into that... >> >> I've looked at the commit history and no, I don't agree to use >> platform_get_irq(): my platform_get_irq() fix landed in mainline later than >> this driver, so fixing this bug in stable kernels would ensue an extra >> dependency... > > Well, surely the platform_get_irq() function should also be fixed in > stable kernels, so that all callers can be fixed at the same time. > > That said, this does fix a bug, and it fixes it in the least invasive > way, and the damage I'm complaining about is preexisting, so: > > Acked-by: Thierry Reding <treding@nvidia.com> FWIW ... Acked-by: Jon Hunter <jonathanh@nvidia.com> Thanks! Jon
On Sun, Jul 30, 2017 at 09:10:44PM +0300, Sergei Shtylyov wrote: > of_irq_get() may return 0 as well as negative error number on failure, > while the driver only checks for the negative values. The driver would then > call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get > valid channel interrupt. > > Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe > iff of_irq_get() returned 0. Applied, thanks
Index: slave-dma/drivers/dma/tegra210-adma.c =================================================================== --- slave-dma.orig/drivers/dma/tegra210-adma.c +++ slave-dma/drivers/dma/tegra210-adma.c @@ -717,8 +717,8 @@ static int tegra_adma_probe(struct platf tdc->chan_addr = tdma->base_addr + ADMA_CH_REG_OFFSET(i); tdc->irq = of_irq_get(pdev->dev.of_node, i); - if (tdc->irq < 0) { - ret = tdc->irq; + if (tdc->irq <= 0) { + ret = tdc->irq ?: -ENXIO; goto irq_dispose; }
of_irq_get() may return 0 as well as negative error number on failure, while the driver only checks for the negative values. The driver would then call request_irq(0, ...) in tegra_adma_alloc_chan_resources() and never get valid channel interrupt. Check for 'tdc->irq <= 0' instead and return -ENXIO from the driver's probe iff of_irq_get() returned 0. Fixes: f46b195799b5 ("dmaengine: tegra-adma: Add support for Tegra210 ADMA") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- drivers/dma/tegra210-adma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html