Re: [PATCH v2 04/10] irqdomain: Return errors from irq_create_of_mapping()

On Mon, 23 Sep 2013 10:13:38 +0200, Thierry Reding [off-list ref] wrote:

On Sun, Sep 22, 2013 at 04:14:43PM -0500, Rob Herring wrote:

quoted

On Wed, Sep 18, 2013 at 8:24 AM, Thierry Reding
[off-list ref] wrote:

quoted

Instead of returning 0 for all errors, allow the precise error code to
be propagated. This will be used in subsequent patches to allow further
propagation of error codes.

The interrupt number corresponding to the new mapping is returned in an
output parameter so that the return value is reserved to signal success
(== 0) or failure (< 0).

Signed-off-by: Thierry Reding <redacted>

One comment below, otherwise:

Acked-by: Rob Herring <redacted>

quoted

diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 905a24b..ae71b14 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c

@@ -230,6 +230,7 @@ static int pci_read_irq_line(struct pci_dev *pci_dev)
 {
        struct of_irq oirq;
        unsigned int virq;
+       int ret;

        pr_debug("PCI: Try to map irq for %s...\n", pci_name(pci_dev));

@@ -266,8 +267,10 @@ static int pci_read_irq_line(struct pci_dev *pci_dev)
                         oirq.size, oirq.specifier[0], oirq.specifier[1],
                         of_node_full_name(oirq.controller));

-               virq = irq_create_of_mapping(oirq.controller, oirq.specifier,
-                                            oirq.size);
+               ret = irq_create_of_mapping(oirq.controller, oirq.specifier,
+                                           oirq.size, &virq);
+               if (ret)
+                       virq = NO_IRQ;
        }
        if(virq == NO_IRQ) {
                pr_debug(" Failed to map !\n");

Can you get rid of NO_IRQ usage here instead of adding to it.

I was trying to stay consistent with the remainder of the code. PowerPC
is a pretty heavy user of NO_IRQ. Of all 348 references, more than half
(182) are in arch/powerpc, so I'd rather like to get a go-ahead from
Benjamin on this.

That said, perhaps we should just go all the way and get rid of NO_IRQ
for good. Things could get somewhat messy, though. There are a couple of
these spread through the code:

	#ifndef NO_IRQ
	#define NO_IRQ (-1)
	#endif

And all of them are wrong and need to be removed.  :-)  We're /slowly/
getting rid of the -1 and the usage of NO_IRQ. A global search and
replace of s/NO_IRQ/0/g can be very safely done on arch/powerpc since
powerpc has had NO_IRQ set correctly to '0' for a very long time now.

So, yes, if you're keen to do it I'd love to see a series getting rid of
more NO_IRQ users.

g.