Re: [PATCH] dmaengine: tegra210-adma: fix of_irq_get() error check

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxx>
> 
> ---
>  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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux