linux-kernel - Re: [PATCH] media: hantro: Fix check for single irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8101406.vZ8PxZ7URt@jernej-laptop>
Date:   Fri, 06 Aug 2021 06:44:35 +0200
From:   Jernej Škrabec <jernej.skrabec@...il.com>
To:     p.zabel@...gutronix.de, Ezequiel Garcia <ezequiel@...labora.com>
Cc:     mchehab@...nel.org, gregkh@...uxfoundation.org,
        hverkuil-cisco@...all.nl, emil.velikov@...labora.com,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: hantro: Fix check for single irq

Dne petek, 06. avgust 2021 ob 00:03:36 CEST je Ezequiel Garcia napisal(a):
> Hi Jernej,
> 
> On Thu, 2021-08-05 at 21:04 +0200, Jernej Skrabec wrote:
> > Some cores use only one interrupt and in such case interrupt name in DT
> > is not needed. Driver supposedly accounted that, but due to the wrong
> > field check it never worked. Fix that.
> > 
> > Fixes: 18d6c8b7b4c9 ("media: hantro: add fallback handling for single
> > irq/clk") Signed-off-by: Jernej Skrabec <jernej.skrabec@...il.com>
> > ---
> >  drivers/staging/media/hantro/hantro_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c
> > b/drivers/staging/media/hantro/hantro_drv.c index
> > 8a2edd67f2c6..20e508158871 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -919,7 +919,7 @@ static int hantro_probe(struct platform_device *pdev)
> >                 if (!vpu->variant->irqs[i].handler)
> >                         continue;
> >  
> > -               if (vpu->variant->num_clocks > 1) {
> > +               if (vpu->variant->num_irqs > 1) {
> 
> Oops, thanks for spotting this.
> 
> How about this instead?

No, original solution is more robust. With solution below, you're assuming 
that irq order in driver array is same as in DT. That doesn't matter if there 
is only one name or if names match. However, if there is a typo, either in DT 
node or in driver, driver will still happily assign clock based on index and 
that might not be correct one. Even if it works out, you can easily miss that 
you have a typo. Driver doesn't tell you which irq is used, if it is 
successfully acquired.

Best regards,
Jernej

> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c
> b/drivers/staging/media/hantro/hantro_drv.c index
> 31d8449ca1d2..af7054b04155 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -918,16 +918,15 @@ static int hantro_probe(struct platform_device *pdev)
>                 if (!vpu->variant->irqs[i].handler)
>                         continue;
> 
> -               if (vpu->variant->num_clocks > 1) {
> -                       irq_name = vpu->variant->irqs[i].name;
> -                       irq = platform_get_irq_byname(vpu->pdev, irq_name);
> -               } else {
> +               irq_name = vpu->variant->irqs[i].name;
> +               irq = platform_get_irq_byname(vpu->pdev, irq_name);
> +               if (irq <= 0) {
>                         /*
> -                        * If the driver has a single IRQ, chances are there
> -                        * will be no actual name in the DT bindings. +    
>                    * Missing interrupt-names property in device tree, +    
>                    * looking up interrupts by index.
>                          */
>                         irq_name = "default";
> -                       irq = platform_get_irq(vpu->pdev, 0);
> +                       irq = platform_get_irq(vpu->pdev, i);
>                 }
>                 if (irq <= 0)
>                         return -ENXIO;