[PATCH] gpio: pl061: Warn when IRQ line has not been configured

[PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Alexander A Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Existing (irq < 0) condition is always false because adev->irq has unsigned
type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
the mapping errors were silently ignored.

Seems that repairing this check would be backwards-incompatible and might
break the probe() for the implementations without IRQ support. Therefore
warn the user instead.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/gpio/gpio-pl061.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 5df7782..3439120 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 
 	writeb(0, pl061->base + GPIOIE); /* disable irqs */
 	irq = adev->irq[0];
-	if (irq < 0) {
-		dev_err(&adev->dev, "invalid IRQ\n");
-		return -ENODEV;
-	}
+	if (!irq)
+		dev_warn(&adev->dev, "IRQ support disabled\n");
 	pl061->parent_irq = irq;
 
 	girq = &pl061->gc.irq;
-- 
2.4.6

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Bartosz Golaszewski]

wt., 3 mar 2020 o 10:29 Alexander A Sverdlin
<alexander.sverdlin@nokia.com> napisał(a):
>
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>
> Existing (irq < 0) condition is always false because adev->irq has unsigned
> type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
> the mapping errors were silently ignored.
>
> Seems that repairing this check would be backwards-incompatible and might
> break the probe() for the implementations without IRQ support. Therefore
> warn the user instead.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> ---
>  drivers/gpio/gpio-pl061.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 5df7782..3439120 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
> @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>
>         writeb(0, pl061->base + GPIOIE); /* disable irqs */
>         irq = adev->irq[0];
> -       if (irq < 0) {
> -               dev_err(&adev->dev, "invalid IRQ\n");
> -               return -ENODEV;
> -       }
> +       if (!irq)
> +               dev_warn(&adev->dev, "IRQ support disabled\n");
>         pl061->parent_irq = irq;
>
>         girq = &pl061->gc.irq;
> --
> 2.4.6
>

What happens later on if irq == 0? Does irq_set_irq_wake() fail?

Bart

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Alexander Sverdlin]

Hi!

On 04/03/2020 15:21, Bartosz Golaszewski wrote:
> wt., 3 mar 2020 o 10:29 Alexander A Sverdlin
> <alexander.sverdlin@nokia.com> napisał(a):
>> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>>
>> Existing (irq < 0) condition is always false because adev->irq has unsigned
>> type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
>> the mapping errors were silently ignored.
>>
>> Seems that repairing this check would be backwards-incompatible and might
>> break the probe() for the implementations without IRQ support. Therefore
>> warn the user instead.
>>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>> ---
>>  drivers/gpio/gpio-pl061.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>> index 5df7782..3439120 100644
>> --- a/drivers/gpio/gpio-pl061.c
>> +++ b/drivers/gpio/gpio-pl061.c
>> @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>>
>>         writeb(0, pl061->base + GPIOIE); /* disable irqs */
>>         irq = adev->irq[0];
>> -       if (irq < 0) {
>> -               dev_err(&adev->dev, "invalid IRQ\n");
>> -               return -ENODEV;
>> -       }
>> +       if (!irq)
>> +               dev_warn(&adev->dev, "IRQ support disabled\n");
>>         pl061->parent_irq = irq;
>>
>>         girq = &pl061->gc.irq;
>> --
>> 2.4.6
>>
> What happens later on if irq == 0? Does irq_set_irq_wake() fail?

Yes, would fail if IRQs would be requested from PL061:

int irq_set_irq_wake(unsigned int irq, unsigned int on)                                                                                                                                                            
{                                                                                                                                                                                                                  
        unsigned long flags;                                                                                                                                                                                       
        struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);                                                                                                                      
        int ret = 0;                                                                                                                                                                                               
                                                                                                                                                                                                                   
        if (!desc)                                                                                                                                                                                                 
                return -EINVAL;                                                                                                                                                                                    


-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Bartosz Golaszewski]

śr., 4 mar 2020 o 15:58 Alexander Sverdlin
<alexander.sverdlin@nokia.com> napisał(a):
>
> Hi!
>
> On 04/03/2020 15:21, Bartosz Golaszewski wrote:
> > wt., 3 mar 2020 o 10:29 Alexander A Sverdlin
> > <alexander.sverdlin@nokia.com> napisał(a):
> >> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> >>
> >> Existing (irq < 0) condition is always false because adev->irq has unsigned
> >> type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
> >> the mapping errors were silently ignored.
> >>
> >> Seems that repairing this check would be backwards-incompatible and might
> >> break the probe() for the implementations without IRQ support. Therefore
> >> warn the user instead.
> >>
> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> >> ---
> >>  drivers/gpio/gpio-pl061.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> >> index 5df7782..3439120 100644
> >> --- a/drivers/gpio/gpio-pl061.c
> >> +++ b/drivers/gpio/gpio-pl061.c
> >> @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> >>
> >>         writeb(0, pl061->base + GPIOIE); /* disable irqs */
> >>         irq = adev->irq[0];
> >> -       if (irq < 0) {
> >> -               dev_err(&adev->dev, "invalid IRQ\n");
> >> -               return -ENODEV;
> >> -       }
> >> +       if (!irq)
> >> +               dev_warn(&adev->dev, "IRQ support disabled\n");
> >>         pl061->parent_irq = irq;
> >>
> >>         girq = &pl061->gc.irq;
> >> --
> >> 2.4.6
> >>
> > What happens later on if irq == 0? Does irq_set_irq_wake() fail?
>
> Yes, would fail if IRQs would be requested from PL061:
>
> int irq_set_irq_wake(unsigned int irq, unsigned int on)
> {
>         unsigned long flags;
>         struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
>         int ret = 0;
>
>         if (!desc)
>                 return -EINVAL;
>
>
> --
> Best regards,
> Alexander Sverdlin.

Ok, I'll go ahead and queue this then.

Thanks!
Bartosz

[PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Alexander A Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Existing (irq < 0) condition is always false because adev->irq has unsigned
type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
the mapping errors were silently ignored.

Seems that repairing this check would be backwards-incompatible and might
break the probe() for the implementations without IRQ support. Therefore
warn the user instead.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/gpio/gpio-pl061.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 5df7782..3439120 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 
 	writeb(0, pl061->base + GPIOIE); /* disable irqs */
 	irq = adev->irq[0];
-	if (irq < 0) {
-		dev_err(&adev->dev, "invalid IRQ\n");
-		return -ENODEV;
-	}
+	if (!irq)
+		dev_warn(&adev->dev, "IRQ support disabled\n");
 	pl061->parent_irq = irq;
 
 	girq = &pl061->gc.irq;
-- 
2.4.6

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Alexander Sverdlin]

Hello Andy,

On 18/03/2021 11:51, Andy Shevchenko wrote:
> 
> 
> On Wednesday, March 17, 2021, Alexander A Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>> wrote:
> 
>     From: Alexander Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>>
> 
>     Existing (irq < 0) condition is always false because adev->irq has unsigned
>     type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
>     the mapping errors were silently ignored.
> 
>     Seems that repairing this check would be backwards-incompatible and might
>     break the probe() for the implementations without IRQ support. Therefore
>     warn the user instead.
> 
>     Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>>
>     ---
>      drivers/gpio/gpio-pl061.c | 6 ++----
>      1 file changed, 2 insertions(+), 4 deletions(-)
> 
>     diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>     index 5df7782..3439120 100644
>     --- a/drivers/gpio/gpio-pl061.c
>     +++ b/drivers/gpio/gpio-pl061.c
>     @@ -326,10 +326,8 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> 
>             writeb(0, pl061->base + GPIOIE); /* disable irqs */
>             irq = adev->irq[0];
>     -       if (irq < 0) {
>     -               dev_err(&adev->dev, "invalid IRQ\n");
>     -               return -ENODEV;
>     -       }
>     +       if (!irq)
>     +               dev_warn(&adev->dev, "IRQ support disabled\n");
> 
> 
> 
> I guess you need to preserve bailing out. Seems nobody hit this error path.

Do you mean preserve "return -ENODEV;"?
This never ever happened, because the "if" is "always false", irqs coming from irq[] cannot be
negative.
And there is another use-case actually: there are legal PL061 configurations without IRQs at all,
which simply work even trying to instantiate irq chip, but as devm_gpiochip_add_data() doesn't
fail with irq==0, this goes completely unnoticed and such a gpio bank works fine.

The proper way would be not even try to instantiate any irq chip in such case.
Let me know if I shall rework the patch this way.

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] gpio: pl061: Warn when IRQ line has not been configured

[Andy Shevchenko]

On Thu, Mar 18, 2021 at 1:12 PM Alexander Sverdlin
<alexander.sverdlin@nokia.com> wrote:
> On 18/03/2021 11:51, Andy Shevchenko wrote:
> > On Wednesday, March 17, 2021, Alexander A Sverdlin <alexander.sverdlin@nokia.com <mailto:alexander.sverdlin@nokia.com>> wrote:

...

> >     Existing (irq < 0) condition is always false because adev->irq has unsigned
> >     type and contains 0 in case of failed irq_of_parse_and_map(). Up to now all
> >     the mapping errors were silently ignored.
> >
> >     Seems that repairing this check would be backwards-incompatible and might
> >     break the probe() for the implementations without IRQ support. Therefore
> >     warn the user instead.

...

> >     -       if (irq < 0) {
> >     -               dev_err(&adev->dev, "invalid IRQ\n");
> >     -               return -ENODEV;
> >     -       }
> >     +       if (!irq)
> >     +               dev_warn(&adev->dev, "IRQ support disabled\n");
> >
> >
> >
> > I guess you need to preserve bailing out. Seems nobody hit this error path.
>
> Do you mean preserve "return -ENODEV;"?
> This never ever happened, because the "if" is "always false", irqs coming from irq[] cannot be
> negative.
> And there is another use-case actually: there are legal PL061 configurations without IRQs at all,
> which simply work even trying to instantiate irq chip, but as devm_gpiochip_add_data() doesn't
> fail with irq==0, this goes completely unnoticed and such a gpio bank works fine.
>
> The proper way would be not even try to instantiate any irq chip in such case.
> Let me know if I shall rework the patch this way.

Yes, please, rewrite it like
if (irq > 0) {
 ... instantiate an IRQ chip ...
} else {
 // nothing. No warning is needed (as it wasn't ever before), perhaps
just a debug message
}
--
With Best Regards,
Andy Shevchenko