pci: ftpci100: fix of_irq_get() error check

PATCH In review Sergei Shtylyov 3 participants 8 messages Jul 15, 2017 – Aug 3, 2017 PCI

Share & tools

Sergei Shtylyov

author

2017-07-15 21:44 UTC #0 lore raw

PATCH 1/1 pci: ftpci100: fix of_irq_get() error check

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 <[email protected]>

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

drivers/pci/host/pci-ftpci100.c +2 -2

Index: 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,

Sergei Shtylyov

author

2017-07-15 21:56 UTC #1 lore raw

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

Linus Walleij

Reviewed-by on v1 1/1

2017-07-16 21:17 UTC #2 lore raw

In reply to Sergei Shtylyov

Quoted 7 lines · 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...

Quoted 5 lines

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 <[email protected]>

Looks correct!

Reviewed-by Linus Walleij <[email protected]>

drivers/pci/host/pci-ftpci100.c static int faraday_pci_setup_cascaded_ir · line 355

Quoted 2 lines

drivers/pci/host/pci-ftpci100.c +1 -1

-               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

Sergei Shtylyov

author

2017-07-17 09:55 UTC #3 lore raw

Hello!

Quoted 7 lines · Linus Walleij

Quoted 7 lines
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...

Quoted 8 lines

Quoted 5 lines
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 <[email protected]>
Looks correct!
Reviewed-by Linus Walleij <[email protected]>

Thanks!

Quoted 5 lines

drivers/pci/host/pci-ftpci100.c static int faraday_pci_setup_cascaded_ir · line 355
Quoted 2 lines
drivers/pci/host/pci-ftpci100.c +1 -1
-               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. :-)

Quoted 1 line

Linus Walleij

MBR, Sergei

Bjorn Helgaas

2017-08-02 21:36 UTC #4 lore raw

In reply to Sergei Shtylyov

Quoted 7 lines · 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...

Quoted 5 lines

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 <[email protected]>

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

Quoted 21 lines

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

Index: pci/drivers/pci/host/pci-ftpci100.c
===================================================================

drivers/pci/host/pci-ftpci100.c +2 -2

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

Sergei Shtylyov

author

2017-08-03 09:32 UTC #5 lore raw

Hello!

Quoted 7 lines · Bjorn Helgaas

Quoted 7 lines
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...

Quoted 7 lines

Quoted 5 lines
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 <[email protected]>
Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks!

Thanks! But why only to 4.14?

[...]

MBR, Sergei

Bjorn Helgaas

2017-08-03 16:15 UTC #6 lore raw

Quoted 14 lines · Sergei Shtylyov

Hello!
Quoted 10 lines · Bjorn Helgaas
Quoted 10 lines
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...

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Fixes d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver")
Signed-off-by Sergei Shtylyov <[email protected]>
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

Sergei Shtylyov

author

2017-08-03 16:23 UTC #7 lore raw

Quoted 13 lines · Bjorn Helgaas

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
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 <[email protected]>

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
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...

Quoted 4 lines

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...

Quoted 1 line

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...

Quoted 1 line

Bjorn

MBR, Sergei