PCI: aardvark: Check for platform_get_irq() error

PATCH In review Fabio Estevam 6 participants 11 messages Oct 14, 2017 – Oct 26, 2017

Subsystems (2)

PCI Arm/Zynq architecture

lore

Share & tools

lore

Fabio Estevam

author

2017-10-14 22:07 UTC #0 lore raw

PATCH 1/5 PCI: aardvark: Check for platform_get_irq() error

platform_get_irq() may fail and in the case of failure the error value should be propagated.

Cc Thomas Petazzoni <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

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

drivers/pci/host/pci-aardvark.c +2 -0

index 26ed0c0..f3b16ff 100644
@@ -901,6 +901,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pcie->base);
 
 	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
 	ret = devm_request_irq(dev, irq, advk_pcie_irq_handler,
 			       IRQF_SHARED | IRQF_NO_THREAD, "advk-pcie",
 			       pcie);

Signature · 1 line

2.7.4

Fabio Estevam

author

2017-10-14 22:07 UTC #1 lore raw

PATCH 2/5 PCI: v3-semi: Fix platform_get_irq() error handling

When platform_get_irq() fails we should propagate the real error value instead of always returning -ENODEV.

Cc Linus Walleij <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

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

drivers/pci/host/pci-v3-semi.c +2 -2

index a544d72..01017af 100644
@@ -806,9 +806,9 @@ static int v3_pci_probe(struct platform_device *pdev)
 
 	/* Get and request error IRQ resource */
 	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
+	if (irq < 0) {
 		dev_err(dev, "unable to obtain PCIv3 error IRQ\n");
-		return -ENODEV;
+		return irq;
 	}
 	ret = devm_request_irq(dev, irq, v3_irq, 0,
 			"PCIv3 error", v3);

Signature · 1 line

2.7.4

Linus Walleij

2017-10-14 22:26 UTC #2 lore raw

Re: [PATCH 2/5] PCI: v3-semi: Fix platform_get_irq() error handling

Quoted 5 lines · Fabio Estevam

When platform_get_irq() fails we should propagate the real error value instead of always returning -ENODEV.
Cc Linus Walleij <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>

drivers/pci/host/pci-v3-semi.c static int v3_pci_probe(struct platform_device *pdev) · line 809

Quoted 3 lines

        irq = platform_get_irq(pdev, 0);

drivers/pci/host/pci-v3-semi.c +1 -1

-       if (irq <= 0) {
+       if (irq < 0) {

So 0 is NO_IRQ and not valid these days. See:
https://lwn.net/Articles/470820/

drivers/pci/host/pci-v3-semi.c static int v3_pci_probe(struct platform_device *pdev) · line 811

Quoted 3 lines

                dev_err(dev, "unable to obtain PCIv3 error IRQ\n");

drivers/pci/host/pci-v3-semi.c +1 -1

-               return -ENODEV;
+               return irq;

But this is right, if irq < 0.

Yours,
Linus Walleij

Fabio Estevam

author

2017-10-14 22:07 UTC #3 lore raw

PATCH 3/5 PCI: mediatek: Check for platform_get_irq() error

In reply to Fabio Estevam

platform_get_irq() may fail and in the case of failure the error value should be propagated.

Cc Ryder Lee <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/pci/host/pcie-mediatek.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/pci/host/pcie-mediatek.c +2 -0

index db93efd..05f8c33 100644
@@ -642,6 +642,8 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	int err, irq;
 
 	irq = platform_get_irq(pdev, port->slot);
+	if (irq < 0)
+		return irq;
 	err = devm_request_irq(dev, irq, mtk_pcie_intr_handler,
 			       IRQF_SHARED, "mtk-pcie", port);
 	if (err) {

Signature · 1 line

2.7.4

Fabio Estevam

author

2017-10-14 22:07 UTC #4 lore raw

PATCH 4/5 PCI: tango: Fix platform_get_irq() error handling

In reply to Fabio Estevam

When platform_get_irq() fails we should propagate the real error value instead of always returning -ENXIO.

Cc Marc Gonzalez <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

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

drivers/pci/host/pcie-tango.c +2 -2

index e23f738..5196583 100644
@@ -272,9 +272,9 @@ static int tango_pcie_probe(struct platform_device *pdev)
 		writel_relaxed(0, pcie->base + SMP8759_ENABLE + offset);
 
 	virq = platform_get_irq(pdev, 1);
-	if (virq <= 0) {
+	if (virq < 0) {
 		dev_err(dev, "Failed to map IRQ\n");
-		return -ENXIO;
+		return virq;
 	}
 
 	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &dom_ops, pcie);

Signature · 1 line

2.7.4

Linus Walleij

2017-10-16 09:19 UTC #6 lore raw

Re: [PATCH 4/5] PCI: tango: Fix platform_get_irq() error handling

Quoted 7 lines · Marc Gonzalez

I don't think this patch is correct.
AFAIU, on all but legacy platforms, when platform_get_irq() returns 0, it is to signal an error condition.
Marc Z pointed out this discussion:
http://yarchive.net/comp/linux/zero.html

Same comment as I had, more or less.

Quoted 5 lines

I suppose it would be slightly nicer to write:

        virq = platform_get_irq(pdev, 1);
        if (virq <= 0)
                return virq ? virq : -ENODEV;

This looks good to me.

Yours,
Linus Walleij

Bjorn Helgaas

2017-10-20 22:31 UTC #7 lore raw

Re: [PATCH 4/5] PCI: tango: Fix platform_get_irq() error handling

In reply to Marc Gonzalez

Quoted 50 lines · Marc Gonzalez

+ gkh, maz, tglx, linus w
Quoted 15 lines · Fabio Estevam
When platform_get_irq() fails we should propagate the real error value instead of always returning -ENXIO.
Cc Marc Gonzalez <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/pci/host/pcie-tango.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/host/pcie-tango.c +0 -0
index e23f738..5196583 100644
@@ -272,9 +272,9 @@ static int tango_pcie_probe(struct platform_device *pdev)
 		writel_relaxed(0, pcie->base + SMP8759_ENABLE + offset);
drivers/pci/host/pcie-tango.c static int tango_pcie_probe(struct platform_device *pdev) · line 280
Quoted 10 lines
drivers/pci/host/pcie-tango.c +2 -2
 
 	virq = platform_get_irq(pdev, 1);
-	if (virq <= 0) {
+	if (virq < 0) {
 		dev_err(dev, "Failed to map IRQ\n");
-		return -ENXIO;
+		return virq;
 	}
 
 	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &dom_ops, pcie);
Hello Fabio,
I don't think this patch is correct.
AFAIU, on all but legacy platforms, when platform_get_irq() returns 0, it is to signal an error condition.
Marc Z pointed out this discussion:
http://yarchive.net/comp/linux/zero.html
Looking more closely at the platform_get_irq implementation, and ignoring the CONFIG_SPARC special-case, the return value is either:
* a valid virq > 0, or
* -EPROBE_DEFER, or
* some error code < 0, in the ACPI case, or
* -ENXIO, or
* res->start (an unsigned type, so >= 0)

Quoted 5 lines

I suppose it would be slightly nicer to write:
	virq = platform_get_irq(pdev, 1);
	if (virq <= 0)
		return virq ? virq : -ENODEV;

All these return values end up getting returned by the driver probe() functions. I don't think there's much value in returning the exact error we got from platform_get_irq() vs a generic error like -ENXIO or -ENODEV, so my $0.02 is that the above is more complicated than it's worth.

I think existing code that looks like:

  irq = platform_get_irq(pdev, 0);
  if (irq <= 0)
    return -ENODEV;

is fine and there's no reason to change it. For code that doesn't check the platform_get_irq() return value, I'd say it's worth adding a check for "irq <= 0". Maybe somebody will point out an arch that can return 0 as a legitimate IRQ, and then we'll have to rethink.

Bjorn

Mason

2017-10-26 06:37 UTC #8 lore raw

Re: [PATCH 4/5] PCI: tango: Fix platform_get_irq() error handling

+ linus t, mark b

Quoted 66 lines · Bjorn Helgaas

Quoted 50 lines · Marc Gonzalez
+ gkh, maz, tglx, linus w
Quoted 15 lines · Fabio Estevam
When platform_get_irq() fails we should propagate the real error value instead of always returning -ENXIO.
Cc Marc Gonzalez <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>
---
 drivers/pci/host/pcie-tango.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/host/pcie-tango.c +0 -0
index e23f738..5196583 100644
@@ -272,9 +272,9 @@ static int tango_pcie_probe(struct platform_device *pdev)
 		writel_relaxed(0, pcie->base + SMP8759_ENABLE + offset);
drivers/pci/host/pcie-tango.c static int tango_pcie_probe(struct platform_device *pdev) · line 280
Quoted 10 lines
drivers/pci/host/pcie-tango.c +2 -2
 
 	virq = platform_get_irq(pdev, 1);
-	if (virq <= 0) {
+	if (virq < 0) {
 		dev_err(dev, "Failed to map IRQ\n");
-		return -ENXIO;
+		return virq;
 	}
 
 	irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &dom_ops, pcie);
Hello Fabio,
I don't think this patch is correct.
AFAIU, on all but legacy platforms, when platform_get_irq() returns 0, it is to signal an error condition.
Marc Z pointed out this discussion:
http://yarchive.net/comp/linux/zero.html
Looking more closely at the platform_get_irq implementation, and ignoring the CONFIG_SPARC special-case, the return value is either:
* a valid virq > 0, or
* -EPROBE_DEFER, or
* some error code < 0, in the ACPI case, or
* -ENXIO, or
* res->start (an unsigned type, so >= 0)
Quoted 5 lines
I suppose it would be slightly nicer to write:
	virq = platform_get_irq(pdev, 1);
	if (virq <= 0)
		return virq ? virq : -ENODEV;
All these return values end up getting returned by the driver probe() functions. I don't think there's much value in returning the exact error we got from platform_get_irq() vs a generic error like -ENXIO or -ENODEV, so my $0.02 is that the above is more complicated than it's worth.
I think existing code that looks like:

Quoted 8 lines

  irq = platform_get_irq(pdev, 0);
  if (irq <= 0)
    return -ENODEV;
is fine and there's no reason to change it. For code that doesn't check the platform_get_irq() return value, I'd say it's worth adding a check for "irq <= 0". Maybe somebody will point out an arch that can return 0 as a legitimate IRQ, and then we'll have to rethink.

I agree with what Bjorn wrote, but there might be one exception to consider: mapping EPROBE_DEFER to something else will likely prevent the driver core from correctly handling deferred probes.

References for myself:
https://lwn.net/Articles/485194/
https://patchwork.kernel.org/patch/9558675/

Regards.

Fabio Estevam

author

2017-10-14 22:07 UTC #9 lore raw

PATCH 5/5 PCI: xilinx-nwl: Fix platform_get_irq() error handling

In reply to Fabio Estevam

When platform_get_irq() fails we should propagate the real error value instead of always returning -EINVAL.

Cc Michal Simek <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/pci/host/pcie-xilinx-nwl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/pci/host/pcie-xilinx-nwl.c +3 -3

index 65dea98..b896167 100644
@@ -591,7 +591,7 @@ static int nwl_pcie_enable_msi(struct nwl_pcie *pcie)
 	msi->irq_msi1 = platform_get_irq_byname(pdev, "msi1");
 	if (msi->irq_msi1 < 0) {
 		dev_err(dev, "failed to get IRQ#%d\n", msi->irq_msi1);
-		ret = -EINVAL;
+		ret = msi->irq_msi1;
 		goto err;
 	}

@@ -602,7 +602,7 @@ static int nwl_pcie_enable_msi(struct nwl_pcie *pcie)
 	msi->irq_msi0 = platform_get_irq_byname(pdev, "msi0");
 	if (msi->irq_msi0 < 0) {
 		dev_err(dev, "failed to get IRQ#%d\n", msi->irq_msi0);
-		ret = -EINVAL;
+		ret = msi->irq_msi0;
 		goto err;
 	}

@@ -735,7 +735,7 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
 	if (pcie->irq_misc < 0) {
 		dev_err(dev, "failed to get misc IRQ %d\n",
 			pcie->irq_misc);
-		return -EINVAL;
+		return pcie->irq_misc;
 	}
 
 	err = devm_request_irq(dev, pcie->irq_misc,

Signature · 1 line

2.7.4

Michal Simek

Acked-by on v1 5/5

2017-10-17 08:55 UTC #10 lore raw

Re: [PATCH 5/5] PCI: xilinx-nwl: Fix platform_get_irq() error handling

Quoted 33 lines · Fabio Estevam

When platform_get_irq() fails we should propagate the real error value instead of always returning -EINVAL.

Cc Michal Simek <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/pci/host/pcie-xilinx-nwl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/pci/host/pcie-xilinx-nwl.c +2 -2

index 65dea98..b896167 100644
@@ -591,7 +591,7 @@ static int nwl_pcie_enable_msi(struct nwl_pcie *pcie)
 	msi->irq_msi1 = platform_get_irq_byname(pdev, "msi1");
 	if (msi->irq_msi1 < 0) {
 		dev_err(dev, "failed to get IRQ#%d\n", msi->irq_msi1);
-		ret = -EINVAL;
+		ret = msi->irq_msi1;
 		goto err;
 	}

@@ -602,7 +602,7 @@ static int nwl_pcie_enable_msi(struct nwl_pcie *pcie)
 	msi->irq_msi0 = platform_get_irq_byname(pdev, "msi0");
 	if (msi->irq_msi0 < 0) {
 		dev_err(dev, "failed to get IRQ#%d\n", msi->irq_msi0);
-		ret = -EINVAL;
+		ret = msi->irq_msi0;
 		goto err;
 	}

drivers/pci/host/pcie-xilinx-nwl.c static int nwl_pcie_bridge_init(struct nwl_pcie *pcie) · line 741

Quoted 8 lines

drivers/pci/host/pcie-xilinx-nwl.c +1 -1

@@ -735,7 +735,7 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
 	if (pcie->irq_misc < 0) {
 		dev_err(dev, "failed to get misc IRQ %d\n",
 			pcie->irq_misc);
-		return -EINVAL;
+		return pcie->irq_misc;
 	}
 
 	err = devm_request_irq(dev, pcie->irq_misc,

Acked-by Michal Simek <[email protected]>

Thanks,
Michal