[PATCH] pci: ftpci100: fix of_irq_get() error check

[PATCH] pci: ftpci100: fix of_irq_get() error check

[Sergei Shtylyov]

of_irq_get() may return a negative error number as well as 0 on failure,
while the driver only checks for 0, blithely continuing with the call to
irq_set_chained_handler_and_data() -- that function expects *unsigned int*
so should probably do nothing when a large IRQ number resulting from a
conversion of a negative error number is passed to it. The driver then
probes successfully while being only partly functional...

Check for 'irq <= 0' instead and propagate the negative error number to
the probe method --  that will allow the deferred probing as well...

Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/pci/host/pci-ftpci100.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: pci/drivers/pci/host/pci-ftpci100.c
===================================================================
--- pci.orig/drivers/pci/host/pci-ftpci100.c
+++ pci/drivers/pci/host/pci-ftpci100.c
@@ -350,9 +350,9 @@ static int faraday_pci_setup_cascaded_ir
 
 	/* All PCI IRQs cascade off this one */
 	irq = of_irq_get(intc, 0);
-	if (!irq) {
+	if (irq <= 0) {
 		dev_err(p->dev, "failed to get parent IRQ\n");
-		return -EINVAL;
+		return irq ?: -EINVAL;
 	}
 
 	p->irqdomain = irq_domain_add_linear(intc, 4,

Re: [PATCH] pci: ftpci100: fix of_irq_get() error check

[Sergei Shtylyov]

Sorry, forgot to mention that the patch is against Bjorn Helgaas's pci.git 
repo's master branch.

Re: [PATCH] pci: ftpci100: fix of_irq_get() error check

[Linus Walleij]

On Sat, Jul 15, 2017 at 11:43 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:

> of_irq_get() may return a negative error number as well as 0 on failure,
> while the driver only checks for 0, blithely continuing with the call to
> irq_set_chained_handler_and_data() -- that function expects *unsigned int*
> so should probably do nothing when a large IRQ number resulting from a
> conversion of a negative error number is passed to it. The driver then
> probes successfully while being only partly functional...
>
> Check for 'irq <= 0' instead and propagate the negative error number to
> the probe method --  that will allow the deferred probing as well...
>
> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Looks correct!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> -               return -EINVAL;
> +               return irq ?: -EINVAL;

I thought I knew C but I learn something new all the time.
I had no clue one can use the ternary operator like this.

Linus Walleij

Re: [PATCH] pci: ftpci100: fix of_irq_get() error check

[Sergei Shtylyov]

Hello!

On 7/17/2017 12:17 AM, Linus Walleij wrote:

>> of_irq_get() may return a negative error number as well as 0 on failure,
>> while the driver only checks for 0, blithely continuing with the call to
>> irq_set_chained_handler_and_data() -- that function expects *unsigned int*
>> so should probably do nothing when a large IRQ number resulting from a
>> conversion of a negative error number is passed to it. The driver then
>> probes successfully while being only partly functional...
>>
>> Check for 'irq <= 0' instead and propagate the negative error number to
>> the probe method --  that will allow the deferred probing as well...
>>
>> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> Looks correct!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

    Thanks!

>> -               return -EINVAL;
>> +               return irq ?: -EINVAL;
> 
> I thought I knew C but I learn something new all the time.
> I had no clue one can use the ternary operator like this.

    It's not a standard C, it's gcc's trickery. :-)

> Linus Walleij

MBR, Sergei

Re: [PATCH] pci: ftpci100: fix of_irq_get() error check

[Bjorn Helgaas]

On Sun, Jul 16, 2017 at 12:43:10AM +0300, Sergei Shtylyov wrote:
> of_irq_get() may return a negative error number as well as 0 on failure,
> while the driver only checks for 0, blithely continuing with the call to
> irq_set_chained_handler_and_data() -- that function expects *unsigned int*
> so should probably do nothing when a large IRQ number resulting from a
> conversion of a negative error number is passed to it. The driver then
> probes successfully while being only partly functional...
> 
> Check for 'irq <= 0' instead and propagate the negative error number to
> the probe method --  that will allow the deferred probing as well...
> 
> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks!

> ---
>  drivers/pci/host/pci-ftpci100.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: pci/drivers/pci/host/pci-ftpci100.c
> ===================================================================
> --- pci.orig/drivers/pci/host/pci-ftpci100.c
> +++ pci/drivers/pci/host/pci-ftpci100.c
> @@ -350,9 +350,9 @@ static int faraday_pci_setup_cascaded_ir
>  
>  	/* All PCI IRQs cascade off this one */
>  	irq = of_irq_get(intc, 0);
> -	if (!irq) {
> +	if (irq <= 0) {
>  		dev_err(p->dev, "failed to get parent IRQ\n");
> -		return -EINVAL;
> +		return irq ?: -EINVAL;
>  	}
>  
>  	p->irqdomain = irq_domain_add_linear(intc, 4,
> 

Re: [PATCH] pci: ftpci100: fix of_irq_get() error check

[Sergei Shtylyov]

Hello!

On 8/3/2017 12:36 AM, Bjorn Helgaas wrote:

>> of_irq_get() may return a negative error number as well as 0 on failure,
>> while the driver only checks for 0, blithely continuing with the call to
>> irq_set_chained_handler_and_data() -- that function expects *unsigned int*
>> so should probably do nothing when a large IRQ number resulting from a
>> conversion of a negative error number is passed to it. The driver then
>> probes successfully while being only partly functional...
>>
>> Check for 'irq <= 0' instead and propagate the negative error number to
>> the probe method --  that will allow the deferred probing as well...
>>
>> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks!

    Thanks! But why only to 4.14?

[...]

MBR, Sergei

Re: [PATCH] pci: ftpci100: fix of_irq_get() error check

[Bjorn Helgaas]

On Thu, Aug 03, 2017 at 12:32:29PM +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 8/3/2017 12:36 AM, Bjorn Helgaas wrote:
> 
> >>of_irq_get() may return a negative error number as well as 0 on failure,
> >>while the driver only checks for 0, blithely continuing with the call to
> >>irq_set_chained_handler_and_data() -- that function expects *unsigned int*
> >>so should probably do nothing when a large IRQ number resulting from a
> >>conversion of a negative error number is passed to it. The driver then
> >>probes successfully while being only partly functional...
> >>
> >>Check for 'irq <= 0' instead and propagate the negative error number to
> >>the probe method --  that will allow the deferred probing as well...
> >>
> >>Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
> >>Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >
> >Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks!
> 
>    Thanks! But why only to 4.14?

Standard practice.  We're currently in the v4.13 cycle, and the merge
window is closed, so the only changes we add for v4.13 are (1) fixes
for something we merged during the v4.13 merge window, or (2) critical
fixes that can't wait for v4.14.

If we want something backported to stable kernels, we can add a tag
for that.  That might apply in this case?  Your patch is a fix for
d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host
Bridge driver"), which appeared in v4.12, so we might want to tag it
for any v4.12 or v4.13 stable kernels.

Bjorn

Re: [PATCH] pci: ftpci100: fix of_irq_get() error check

[Sergei Shtylyov]

On 08/03/2017 07:15 PM, Bjorn Helgaas wrote:

>>>> of_irq_get() may return a negative error number as well as 0 on failure,
>>>> while the driver only checks for 0, blithely continuing with the call to
>>>> irq_set_chained_handler_and_data() -- that function expects *unsigned int*
>>>> so should probably do nothing when a large IRQ number resulting from a
>>>> conversion of a negative error number is passed to it. The driver then
>>>> probes successfully while being only partly functional...
>>>>
>>>> Check for 'irq <= 0' instead and propagate the negative error number to
>>>> the probe method --  that will allow the deferred probing as well...
>>>>
>>>> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks!
>>
>>     Thanks! But why only to 4.14?
> 
> Standard practice.  We're currently in the v4.13 cycle, and the merge
> window is closed, so the only changes we add for v4.13 are (1) fixes
> for something we merged during the v4.13 merge window, or (2) critical
> fixes that can't wait for v4.14.

    OK, I thought the original Linus' commit was a bit more recent, like 4.13...

> If we want something backported to stable kernels, we can add a tag
> for that.  That might apply in this case?  Your patch is a fix for
> d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host
> Bridge driver"), which appeared in v4.12,

    If not in 4.11...

> so we might want to tag it for any v4.12 or v4.13 stable kernels.

    I think the -stable people look at the Fixes: tag now...

> Bjorn

MBR, Sergei