[PATCH 0/5] Handled return value of platform_get_irq correctly

[PATCH 0/5] Handled return value of platform_get_irq correctly

[Aman Sharma]

As mentioned by Bjorn Helgaas in a private mail, the return value of
platform_get_irq must be checked against the conditon of strictly
smaller than 0 and if check must return the value recieved from
platform_get_irq rather than any other macro like -ENODEV.

Aman Sharma (5):
  pci: handled return value of platform_get_irq correctly
  pci: added check for return value of platform_get_irq
  pci: handled return value of platform_get_irq correctly
  pci: handled return value of platform_get_irq correctly
  pci: added check for return value of platform_get_irq

 drivers/pci/controller/pci-aardvark.c  | 3 +++
 drivers/pci/controller/pci-v3-semi.c   | 4 ++--
 drivers/pci/controller/pcie-mediatek.c | 3 +++
 drivers/pci/controller/pcie-mobiveil.c | 4 ++--
 drivers/pci/controller/pcie-tango.c    | 4 ++--
 5 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 1/5] pci: handled return value of platform_get_irq correctly

[Aman Sharma]

Signed-off-by: Aman Sharma <amanharitsh123@gmail.com>
---
 drivers/pci/controller/pci-v3-semi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-v3-semi.c b/drivers/pci/controller/pci-v3-semi.c
index bd05221f5a22..a5bf945d2eda 100644
--- a/drivers/pci/controller/pci-v3-semi.c
+++ b/drivers/pci/controller/pci-v3-semi.c
@@ -777,9 +777,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);
-- 
2.20.1

[PATCH 2/5] pci: added check for return value of platform_get_irq

[Aman Sharma]

Signed-off-by: Aman Sharma <amanharitsh123@gmail.com>
---
 drivers/pci/controller/pci-aardvark.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 2a20b649f40c..40a4257f0df1 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -973,6 +973,9 @@ 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);
-- 
2.20.1

[PATCH 3/5] pci: handled return value of platform_get_irq correctly

[Aman Sharma]

Signed-off-by: Aman Sharma <amanharitsh123@gmail.com>
---
 drivers/pci/controller/pcie-mobiveil.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mobiveil.c b/drivers/pci/controller/pcie-mobiveil.c
index 3a696ca45bfa..fe816a284dd4 100644
--- a/drivers/pci/controller/pcie-mobiveil.c
+++ b/drivers/pci/controller/pcie-mobiveil.c
@@ -456,9 +456,9 @@ static int mobiveil_pcie_parse_dt(struct mobiveil_pcie *pcie)
 		pcie->ppio_wins = MAX_PIO_WINDOWS;
 
 	pcie->irq = platform_get_irq(pdev, 0);
-	if (pcie->irq <= 0) {
+	if (pcie->irq < 0) {
 		dev_err(dev, "failed to map IRQ: %d\n", pcie->irq);
-		return -ENODEV;
+		return pcie->irq;
 	}
 
 	return 0;
-- 
2.20.1

[PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Aman Sharma]

Signed-off-by: Aman Sharma <amanharitsh123@gmail.com>
---
 drivers/pci/controller/pcie-tango.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-tango.c b/drivers/pci/controller/pcie-tango.c
index 21a208da3f59..18c2c4313eb5 100644
--- a/drivers/pci/controller/pcie-tango.c
+++ b/drivers/pci/controller/pcie-tango.c
@@ -273,9 +273,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);
-- 
2.20.1

[PATCH 5/5] pci: added check for return value of platform_get_irq

[Aman Sharma]

Signed-off-by: Aman Sharma <amanharitsh123@gmail.com>
---
 drivers/pci/controller/pcie-mediatek.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index cb982891b22b..ebfa7d5a4e2d 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -651,6 +651,9 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
 	}
 
 	port->irq = platform_get_irq(pdev, port->slot);
+	if (port->irq < 0)
+		return port->irq;
+
 	irq_set_chained_handler_and_data(port->irq,
 					 mtk_pcie_intr_handler, port);
 
-- 
2.20.1

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Marc Gonzalez]

On 11/03/2020 20:19, Aman Sharma wrote:

> diff --git a/drivers/pci/controller/pcie-tango.c b/drivers/pci/controller/pcie-tango.c
> index 21a208da3f59..18c2c4313eb5 100644
> --- a/drivers/pci/controller/pcie-tango.c
> +++ b/drivers/pci/controller/pcie-tango.c
> @@ -273,9 +273,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);


Weee, here we go again :-)

https://patchwork.kernel.org/patch/11066455/
https://patchwork.kernel.org/patch/10006651/

Last time around, my understanding was that, going forward,
the best solution was:

	virq = platform_get_irq(...)
	if (virq <= 0)
		return virq ? : -ENODEV;

i.e. map 0 to -ENODEV, pass other errors as-is, remove the dev_err

@Bjorn/Lorenzo did you have a change of heart?

Regards.

Re: [PATCH 1/5] pci: handled return value of platform_get_irq correctly

[Linus Walleij]

On Wed, Mar 11, 2020 at 8:19 PM Aman Sharma <amanharitsh123@gmail.com> wrote:

> Signed-off-by: Aman Sharma <amanharitsh123@gmail.com>
> ---
>  drivers/pci/controller/pci-v3-semi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-v3-semi.c b/drivers/pci/controller/pci-v3-semi.c
> index bd05221f5a22..a5bf945d2eda 100644
> --- a/drivers/pci/controller/pci-v3-semi.c
> +++ b/drivers/pci/controller/pci-v3-semi.c
> @@ -777,9 +777,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) {

Have you considered:
https://lwn.net/Articles/470820/

TL;DR Linus (both of them) are not with you on this.

And that is why the code is written like this.

Do you really have a platform that could return 0 as IRQ
here? In that case, can we fix it?

>                 dev_err(dev, "unable to obtain PCIv3 error IRQ\n");
> -               return -ENODEV;
> +               return irq;

That's OK with me.

Yours,
Linus Walleij

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Bjorn Helgaas]

[+cc another Marc]

On Thu, Mar 12, 2020 at 10:53:06AM +0100, Marc Gonzalez wrote:
> On 11/03/2020 20:19, Aman Sharma wrote:
> 
> > diff --git a/drivers/pci/controller/pcie-tango.c b/drivers/pci/controller/pcie-tango.c
> > index 21a208da3f59..18c2c4313eb5 100644
> > --- a/drivers/pci/controller/pcie-tango.c
> > +++ b/drivers/pci/controller/pcie-tango.c
> > @@ -273,9 +273,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);
> 
> Weee, here we go again :-)
> 
> https://patchwork.kernel.org/patch/11066455/
> https://patchwork.kernel.org/patch/10006651/
> 
> Last time around, my understanding was that, going forward,
> the best solution was:
> 
> 	virq = platform_get_irq(...)
> 	if (virq <= 0)
> 		return virq ? : -ENODEV;
> 
> i.e. map 0 to -ENODEV, pass other errors as-is, remove the dev_err
> 
> @Bjorn/Lorenzo did you have a change of heart?

Yes.  In 10006651 (Oct 20, 2017), I thought:

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

was fine.  In 11066455 (Aug 7, 2019), I said I thought I was wrong and
that:

  platform_get_irq() is a generic interface and we have to be able to
  interpret return values consistently.  The overwhelming consensus
  among platform_get_irq() callers is to treat "irq < 0" as an error,
  and I think we should follow suit.
  ...
  I think the best pattern is:

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

I still think what I said in 2019 is the right approach.  I do see
your comment in 10006651 about this pattern:

  if (virq <= 0)
    return virq ? : -ENODEV;

but IMHO it's too complicated for general use.  Admittedly, it's not
*very* complicated, but it's a relatively unusual C idiom and I
stumble over it every time I see it.  If 0 is a special case I think
it should be mapped to a negative error in arch-specific code, which I
think is what Linus T suggested in [1].

I think there's still a large consensus that "irq < 0" is the error
case.  In the tree today we have about 1400 callers of
platform_get_irq() and platform_get_irq_byname() [2].  Of those,
almost 900 check for "irq < 0" [3], while only about 150 check for
"irq <= 0" [4] and about 15 use some variant of a "irq ? : -ENODEV"
pattern.

The bottom line is that in drivers/pci, I'd like to see either a
single style or a compelling argument for why some checks should be
"irq < 0" and others should be "irq <= 0".

[1] https://yarchive.net/comp/linux/zero.html
[2] $ git grep "=.*platform_get_irq" | wc -l
    1422
[3] $ git grep -A4 "=.*platform_get_irq" | grep "<\s*0" | wc -l
    894
[4] $ git grep -A4 "=.*platform_get_irq" | grep "<=\s*0" | wc -l
    151
[5] $ git grep -A4 "=.*platform_get_irq" | grep "return.*?.*:.*;" | wc -l
    15

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Marc Gonzalez]

On 12/03/2020 15:11, Bjorn Helgaas wrote:

> [+cc another Marc]

Doh! I should indeed have CCed maz and tglx.

> On Thu, Mar 12, 2020 at 10:53:06AM +0100, Marc Gonzalez wrote:
>
>> On 11/03/2020 20:19, Aman Sharma wrote:
>>
>>> diff --git a/drivers/pci/controller/pcie-tango.c b/drivers/pci/controller/pcie-tango.c
>>> index 21a208da3f59..18c2c4313eb5 100644
>>> --- a/drivers/pci/controller/pcie-tango.c
>>> +++ b/drivers/pci/controller/pcie-tango.c
>>> @@ -273,9 +273,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);
>>
>> Weee, here we go again :-)
>>
>> https://patchwork.kernel.org/patch/11066455/
>> https://patchwork.kernel.org/patch/10006651/
>>
>> Last time around, my understanding was that, going forward,
>> the best solution was:
>>
>> 	virq = platform_get_irq(...)
>> 	if (virq <= 0)
>> 		return virq ? : -ENODEV;
>>
>> i.e. map 0 to -ENODEV, pass other errors as-is, remove the dev_err
>>
>> @Bjorn/Lorenzo did you have a change of heart?
> 
> Yes.  In 10006651 (Oct 20, 2017), I thought:
> 
>   irq = platform_get_irq(pdev, 0);
>   if (irq <= 0)
>     return -ENODEV;
> 
> was fine.  In 11066455 (Aug 7, 2019), I said I thought I was wrong and
> that:
> 
>   platform_get_irq() is a generic interface and we have to be able to
>   interpret return values consistently.  The overwhelming consensus
>   among platform_get_irq() callers is to treat "irq < 0" as an error,
>   and I think we should follow suit.
>   ...
>   I think the best pattern is:
> 
>     irq = platform_get_irq(pdev, i);
>     if (irq < 0)
>       return irq;
> 
> I still think what I said in 2019 is the right approach.  I do see
> your comment in 10006651 about this pattern:
> 
>   if (virq <= 0)
>     return virq ? : -ENODEV;
> 
> but IMHO it's too complicated for general use.  Admittedly, it's not
> *very* complicated, but it's a relatively unusual C idiom and I
> stumble over it every time I see it.

FTR, omitting the middle operand is a GNU extension.
https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html
The valid C idiom would be virq ? virq : -ENODEV

> If 0 is a special case I think
> it should be mapped to a negative error in arch-specific code, which I
> think is what Linus T suggested in [1].

Lorenzo, being both PCI maintainer and ARM employee should be in a
good position to change the arch-specific code for arm and arm64?

> I think there's still a large consensus that "irq < 0" is the error
> case.  In the tree today we have about 1400 callers of
> platform_get_irq() and platform_get_irq_byname() [2].  Of those,
> almost 900 check for "irq < 0" [3], while only about 150 check for
> "irq <= 0" [4] and about 15 use some variant of a "irq ? : -ENODEV"
> pattern.
> 
> The bottom line is that in drivers/pci, I'd like to see either a
> single style or a compelling argument for why some checks should be
> "irq < 0" and others should be "irq <= 0".
> 
> [1] https://yarchive.net/comp/linux/zero.html
> [2] $ git grep "=.*platform_get_irq" | wc -l
>     1422
> [3] $ git grep -A4 "=.*platform_get_irq" | grep "<\s*0" | wc -l
>     894
> [4] $ git grep -A4 "=.*platform_get_irq" | grep "<=\s*0" | wc -l
>     151
> [5] $ git grep -A4 "=.*platform_get_irq" | grep "return.*?.*:.*;" | wc -l
>     15

Interesting stats, thanks.

Regards.

Re: [PATCH 1/5] pci: handled return value of platform_get_irq correctly

[Bjorn Helgaas]

[+cc Marc, Thomas]

Hi Linus,

On Thu, Mar 12, 2020 at 03:07:58PM +0100, Linus Walleij wrote:
> On Wed, Mar 11, 2020 at 8:19 PM Aman Sharma <amanharitsh123@gmail.com> wrote:
> > Signed-off-by: Aman Sharma <amanharitsh123@gmail.com>
> > ---
> >  drivers/pci/controller/pci-v3-semi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-v3-semi.c b/drivers/pci/controller/pci-v3-semi.c
> > index bd05221f5a22..a5bf945d2eda 100644
> > --- a/drivers/pci/controller/pci-v3-semi.c
> > +++ b/drivers/pci/controller/pci-v3-semi.c
> > @@ -777,9 +777,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) {
> 
> Have you considered:
> https://lwn.net/Articles/470820/
> 
> TL;DR Linus (both of them) are not with you on this.
> 
> And that is why the code is written like this.

I'm not sure I understand you here, so please correct me when I go in
the weeds.  I guess you're saying that platform_get_irq() can return
0 here and we should treat that as an error?

This particular driver seems to be ARM-specific -- does that mean we
need to check for 0 on some arches but not others?  That would
definitely be suboptimal, and that's what I'd like to fix here.

IIUC, in the link you mentioned, Linus T says that "dev->irq == 0"
means we don't have a valid IRQ.  I think that makes sense, but I'm
not sure it follows that 0 must be a sensical return value for
platform_get_irq().  It seems to me that platform_get_irq() ought to
return either a valid IRQ or an error, and the convention for errors
is a negative errno.

In fact, the platform_get_irq() function comment says it returns "IRQ
number on success, negative error number on failure."  If we need to
interpret 0 as an error on some arches, it sounds like something is
wrong in the arch-specific bowels of platform_get_irq().

If platform_get_irq() returns an error, a driver might want to
continue in polled mode without IRQs, in which case it could set its
"dev->irq = 0" to indicate that it has no valid IRQ.  But I think we
might be able to separate that "stored IRQ" situation from the
platform_get_irq() interface.

Bjorn

Re: [PATCH 1/5] pci: handled return value of platform_get_irq correctly

[Linus Walleij]

On Thu, Mar 12, 2020 at 8:02 PM Bjorn Helgaas <helgaas@kernel.org> wrote:

> IIUC, in the link you mentioned, Linus T says that "dev->irq == 0"
> means we don't have a valid IRQ.  I think that makes sense, but I'm
> not sure it follows that 0 must be a sensical return value for
> platform_get_irq().  It seems to me that platform_get_irq() ought to
> return either a valid IRQ or an error, and the convention for errors
> is a negative errno.

OK I see your point.

I would be fine of the code is changed from:

if (irq <= 0)
  error;

To:

if (irq < 0)
   error retrieving IRQ

if (!irq)
   error driver requires a valid IRQ

To the driver (this one in specific) the IRQ is expected and
necessary and I think it holds for most PCI hosts.

Yours,
Linus Walleij

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Thomas Gleixner]

Bjorn,

Bjorn Helgaas <helgaas@kernel.org> writes:
> On Thu, Mar 12, 2020 at 10:53:06AM +0100, Marc Gonzalez wrote:
>> Last time around, my understanding was that, going forward,
>> the best solution was:
>> 
>> 	virq = platform_get_irq(...)
>> 	if (virq <= 0)
>> 		return virq ? : -ENODEV;
>> 
>> i.e. map 0 to -ENODEV, pass other errors as-is, remove the dev_err
>> 
>> @Bjorn/Lorenzo did you have a change of heart?
>
> Yes.  In 10006651 (Oct 20, 2017), I thought:
>
>   irq = platform_get_irq(pdev, 0);
>   if (irq <= 0)
>     return -ENODEV;
>
> was fine.  In 11066455 (Aug 7, 2019), I said I thought I was wrong and
> that:
>
>   platform_get_irq() is a generic interface and we have to be able to
>   interpret return values consistently.  The overwhelming consensus
>   among platform_get_irq() callers is to treat "irq < 0" as an error,
>   and I think we should follow suit.
>   ...
>   I think the best pattern is:
>
>     irq = platform_get_irq(pdev, i);
>     if (irq < 0)
>       return irq;

Careful. 0 is not a valid interrupt.

Thanks,

        tglx

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Bjorn Helgaas]

On Fri, Mar 13, 2020 at 10:05:58PM +0100, Thomas Gleixner wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> > On Thu, Mar 12, 2020 at 10:53:06AM +0100, Marc Gonzalez wrote:
> >> Last time around, my understanding was that, going forward,
> >> the best solution was:
> >> 
> >> 	virq = platform_get_irq(...)
> >> 	if (virq <= 0)
> >> 		return virq ? : -ENODEV;
> >> 
> >> i.e. map 0 to -ENODEV, pass other errors as-is, remove the dev_err
> >> 
> >> @Bjorn/Lorenzo did you have a change of heart?
> >
> > Yes.  In 10006651 (Oct 20, 2017), I thought:
> >
> >   irq = platform_get_irq(pdev, 0);
> >   if (irq <= 0)
> >     return -ENODEV;
> >
> > was fine.  In 11066455 (Aug 7, 2019), I said I thought I was wrong and
> > that:
> >
> >   platform_get_irq() is a generic interface and we have to be able to
> >   interpret return values consistently.  The overwhelming consensus
> >   among platform_get_irq() callers is to treat "irq < 0" as an error,
> >   and I think we should follow suit.
> >   ...
> >   I think the best pattern is:
> >
> >     irq = platform_get_irq(pdev, i);
> >     if (irq < 0)
> >       return irq;
> 
> Careful. 0 is not a valid interrupt.

Should callers of platform_get_irq() check for a 0 return value?
About 900 of them do not.

Or should platform_get_irq() return a negative error instead of 0?
If 0 is not a valid interrupt, I think it would be easier to use the
interface if we made it so platform_get_irq() could never return 0,
which I think would also fit the interface documentation better:

 * Return: IRQ number on success, negative error number on failure.

Bjorn

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Bjorn Helgaas]

On Fri, Mar 13, 2020 at 04:56:42PM -0500, Bjorn Helgaas wrote:
> On Fri, Mar 13, 2020 at 10:05:58PM +0100, Thomas Gleixner wrote:
> > Bjorn Helgaas <helgaas@kernel.org> writes:
> > > On Thu, Mar 12, 2020 at 10:53:06AM +0100, Marc Gonzalez wrote:
> > >> Last time around, my understanding was that, going forward,
> > >> the best solution was:
> > >> 
> > >> 	virq = platform_get_irq(...)
> > >> 	if (virq <= 0)
> > >> 		return virq ? : -ENODEV;
> > >> 
> > >> i.e. map 0 to -ENODEV, pass other errors as-is, remove the dev_err
> > >> 
> > >> @Bjorn/Lorenzo did you have a change of heart?
> > >
> > > Yes.  In 10006651 (Oct 20, 2017), I thought:
> > >
> > >   irq = platform_get_irq(pdev, 0);
> > >   if (irq <= 0)
> > >     return -ENODEV;
> > >
> > > was fine.  In 11066455 (Aug 7, 2019), I said I thought I was wrong and
> > > that:
> > >
> > >   platform_get_irq() is a generic interface and we have to be able to
> > >   interpret return values consistently.  The overwhelming consensus
> > >   among platform_get_irq() callers is to treat "irq < 0" as an error,
> > >   and I think we should follow suit.
> > >   ...
> > >   I think the best pattern is:
> > >
> > >     irq = platform_get_irq(pdev, i);
> > >     if (irq < 0)
> > >       return irq;
> > 
> > Careful. 0 is not a valid interrupt.
> 
> Should callers of platform_get_irq() check for a 0 return value?
> About 900 of them do not.
> 
> Or should platform_get_irq() return a negative error instead of 0?
> If 0 is not a valid interrupt, I think it would be easier to use the
> interface if we made it so platform_get_irq() could never return 0,
> which I think would also fit the interface documentation better:
> 
>  * Return: IRQ number on success, negative error number on failure.

Trying again -- I'm not quite catching your meaning, Thomas.

If platform_get_irq*() can return 0, but 0 is not a valid IRQ, I think
it's sort of complicated to parse that return value.  Drivers that
require an IRQ would do this:

  irq = platform_get_irq(pdev, i);
  if (irq < 0)
    return irq;
  if (irq == 0)
    return -EINVAL;         # error since driver requires IRQ
  return devm_request_irq(dev, irq, ...);

Drivers that can either use an IRQ or do polling would do this:

  irq = platform_get_irq(pdev, i);
  if (irq <= 0)
    return setup_polling();
  return devm_request_irq(dev, irq, ...);

I think those are sort of ungainly, especially the first.  If we made
it so those functions never returned 0, drivers that need an IRQ could
do this:

  irq = platform_get_irq(pdev, i);
  if (irq < 0)
    return irq;
  return devm_request_irq(dev, irq, ...);

and drivers that support polling could do this:

  irq = platform_get_irq(pdev, i);
  if (irq < 0)
    return setup_polling();
  return devm_request_irq(dev, irq, ...);

That seems a lot easier to get correct, and it matches what most of
the callers already do.

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Thomas Gleixner]

Bjorn,

Bjorn Helgaas <helgaas@kernel.org> writes:
> On Fri, Mar 13, 2020 at 04:56:42PM -0500, Bjorn Helgaas wrote:
>> On Fri, Mar 13, 2020 at 10:05:58PM +0100, Thomas Gleixner wrote:
>> > >   I think the best pattern is:
>> > >
>> > >     irq = platform_get_irq(pdev, i);
>> > >     if (irq < 0)
>> > >       return irq;
>> > 
>> > Careful. 0 is not a valid interrupt.
>> 
>> Should callers of platform_get_irq() check for a 0 return value?
>> About 900 of them do not.

I don't know what I was looking at.

platform_get_irq() does the right thing already, so checking for irq < 0
is sufficient.

Sorry for the confusion!

Thanks,

        tglx

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Bjorn Helgaas]

On Wed, Mar 18, 2020 at 02:42:48PM +0100, Thomas Gleixner wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> > On Fri, Mar 13, 2020 at 04:56:42PM -0500, Bjorn Helgaas wrote:
> >> On Fri, Mar 13, 2020 at 10:05:58PM +0100, Thomas Gleixner wrote:
> >> > >   I think the best pattern is:
> >> > >
> >> > >     irq = platform_get_irq(pdev, i);
> >> > >     if (irq < 0)
> >> > >       return irq;
> >> > 
> >> > Careful. 0 is not a valid interrupt.
> >> 
> >> Should callers of platform_get_irq() check for a 0 return value?
> >> About 900 of them do not.
> 
> I don't know what I was looking at.
> 
> platform_get_irq() does the right thing already, so checking for irq < 0
> is sufficient.
> 
> Sorry for the confusion!

Thanks, I was indeed confused!  Maybe we could reduce future confusion
by strengthening the comments slightly, e.g.,

  - * Return: IRQ number on success, negative error number on failure.
  + * Return: non-zero IRQ number on success, negative error number on failure.

I don't want to push my luck, but it's pretty hard to prove that
platform_get_irq() never returns 0.  What would you think of something
like the following?

@@ -133,23 +133,24 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
  *		if (irq < 0)
  *			return irq;
  *
- * Return: IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, negative error number on failure.
  */
 int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 {
+	int ret;
 #ifdef CONFIG_SPARC
 	/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
 	if (!dev || num >= dev->archdata.num_irqs)
 		return -ENXIO;
-	return dev->archdata.irqs[num];
+	ret = dev->archdata.irqs[num];
+	goto out;
 #else
 	struct resource *r;
-	int ret;
 
 	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
 		ret = of_irq_get(dev->dev.of_node, num);
 		if (ret > 0 || ret == -EPROBE_DEFER)
-			return ret;
+			goto out;
 	}
 
 	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
@@ -157,7 +158,7 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		if (r && r->flags & IORESOURCE_DISABLED) {
 			ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
 			if (ret)
-				return ret;
+				goto out;
 		}
 	}
 
@@ -171,13 +172,17 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		struct irq_data *irqd;
 
 		irqd = irq_get_irq_data(r->start);
-		if (!irqd)
-			return -ENXIO;
+		if (!irqd) {
+			ret = -ENXIO;
+			goto out;
+		}
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	if (r)
-		return r->start;
+	if (r) {
+		ret = r->start;
+		goto out;
+	}
 
 	/*
 	 * For the index 0 interrupt, allow falling back to GpioInt
@@ -190,11 +195,14 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
 		/* Our callers expect -ENXIO for missing IRQs. */
 		if (ret >= 0 || ret == -EPROBE_DEFER)
-			return ret;
+			goto out;
 	}
 
-	return -ENXIO;
+	ret = -ENXIO;
 #endif
+out:
+	WARN(ret == 0, "0 is an invalid IRQ number\n");
+	return ret;
 }
 EXPORT_SYMBOL_GPL(platform_get_irq_optional);
 
@@ -212,7 +220,7 @@ EXPORT_SYMBOL_GPL(platform_get_irq_optional);
  *		if (irq < 0)
  *			return irq;
  *
- * Return: IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, negative error number on failure.
  */
 int platform_get_irq(struct platform_device *dev, unsigned int num)
 {
@@ -284,8 +292,10 @@ static int __platform_get_irq_byname(struct platform_device *dev,
 	}
 
 	r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
-	if (r)
+	if (r) {
+		WARN(r->start == 0, "0 is an invalid IRQ number\n");
 		return r->start;
+	}
 
 	return -ENXIO;
 }
@@ -297,7 +307,7 @@ static int __platform_get_irq_byname(struct platform_device *dev,
  *
  * Get an IRQ like platform_get_irq(), but then by name rather then by index.
  *
- * Return: IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, negative error number on failure.
  */
 int platform_get_irq_byname(struct platform_device *dev, const char *name)
 {
@@ -319,7 +329,7 @@ EXPORT_SYMBOL_GPL(platform_get_irq_byname);
  * Get an optional IRQ by name like platform_get_irq_byname(). Except that it
  * does not print an error message if an IRQ can not be obtained.
  *
- * Return: IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, negative error number on failure.
  */
 int platform_get_irq_byname_optional(struct platform_device *dev,
 				     const char *name)

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Thomas Gleixner]

Bjorn Helgaas <helgaas@kernel.org> writes:
> On Wed, Mar 18, 2020 at 02:42:48PM +0100, Thomas Gleixner wrote:
>> Bjorn Helgaas <helgaas@kernel.org> writes:
>> > On Fri, Mar 13, 2020 at 04:56:42PM -0500, Bjorn Helgaas wrote:
>> >> On Fri, Mar 13, 2020 at 10:05:58PM +0100, Thomas Gleixner wrote:
>> >> > >   I think the best pattern is:
>> >> > >
>> >> > >     irq = platform_get_irq(pdev, i);
>> >> > >     if (irq < 0)
>> >> > >       return irq;
>> >> > 
>> >> > Careful. 0 is not a valid interrupt.
>> >> 
>> >> Should callers of platform_get_irq() check for a 0 return value?
>> >> About 900 of them do not.
>> 
>> I don't know what I was looking at.
>> 
>> platform_get_irq() does the right thing already, so checking for irq < 0
>> is sufficient.
>> 
>> Sorry for the confusion!
>
> Thanks, I was indeed confused!  Maybe we could reduce future confusion
> by strengthening the comments slightly, e.g.,
>
>   - * Return: IRQ number on success, negative error number on failure.
>   + * Return: non-zero IRQ number on success, negative error number on failure.
>
> I don't want to push my luck, but it's pretty hard to prove that
> platform_get_irq() never returns 0.  What would you think of something
> like the following?

No objections from my side.

Re: [PATCH 4/5] pci: handled return value of platform_get_irq correctly

[Bjorn Helgaas]

On Thu, Mar 19, 2020 at 09:47:47AM +0100, Thomas Gleixner wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> > On Wed, Mar 18, 2020 at 02:42:48PM +0100, Thomas Gleixner wrote:
> >> Bjorn Helgaas <helgaas@kernel.org> writes:
> >> > On Fri, Mar 13, 2020 at 04:56:42PM -0500, Bjorn Helgaas wrote:
> >> >> On Fri, Mar 13, 2020 at 10:05:58PM +0100, Thomas Gleixner wrote:
> >> >> > >   I think the best pattern is:
> >> >> > >
> >> >> > >     irq = platform_get_irq(pdev, i);
> >> >> > >     if (irq < 0)
> >> >> > >       return irq;
> >> >> > 
> >> >> > Careful. 0 is not a valid interrupt.
> >> >> 
> >> >> Should callers of platform_get_irq() check for a 0 return value?
> >> >> About 900 of them do not.
> >> 
> >> I don't know what I was looking at.
> >> 
> >> platform_get_irq() does the right thing already, so checking for irq < 0
> >> is sufficient.
> >> 
> >> Sorry for the confusion!
> >
> > Thanks, I was indeed confused!  Maybe we could reduce future confusion
> > by strengthening the comments slightly, e.g.,
> >
> >   - * Return: IRQ number on success, negative error number on failure.
> >   + * Return: non-zero IRQ number on success, negative error number on failure.
> >
> > I don't want to push my luck, but it's pretty hard to prove that
> > platform_get_irq() never returns 0.  What would you think of something
> > like the following?
> 
> No objections from my side.

OK, thanks!

Aman, my suggestion for you is to include the patch below as the first
patch in your series.  Then post a v2 of it, making sure to cc:
everybody who commented on the first version.

Bjorn


driver core: platform: Specify that IRQ 0 is invalid

These interfaces return a negative error number or an IRQ:

  platform_get_irq()
  platform_get_irq_optional()
  platform_get_irq_byname()
  platform_get_irq_byname_optional()

The function comments suggest checking for error like this:

  irq = platform_get_irq(...);
  if (irq < 0)
    return irq;

which is what most callers (~900 of 1400) do, so it's implicit that IRQ 0
is invalid.  But some callers check for "irq <= 0", and it's not obvious
from the source that we never return an IRQ 0.

Make this more explicit by updating the comments to say that an IRQ number
is always non-zero and adding a WARN() if we ever do return zero.  If we do
return IRQ 0, it likely indicates a bug in the arch-specific parts of
platform_get_irq().

Relevant prior discussion at [1,2].

[1] https://lore.kernel.org/lkml/Pine.LNX.4.64.0701250940220.25027@woody.linux-foundation.org/
[2] https://lore.kernel.org/lkml/Pine.LNX.4.64.0701252029570.25027@woody.linux-foundation.org/
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 7fa654f1288b..50f3a5da89dc 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -133,23 +133,24 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
  *		if (irq < 0)
  *			return irq;
  *
- * Return: IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, negative error number on failure.
  */
 int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 {
+	int ret;
 #ifdef CONFIG_SPARC
 	/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
 	if (!dev || num >= dev->archdata.num_irqs)
 		return -ENXIO;
-	return dev->archdata.irqs[num];
+	ret = dev->archdata.irqs[num];
+	goto out;
 #else
 	struct resource *r;
-	int ret;
 
 	if (IS_ENABLED(CONFIG_OF_IRQ) && dev->dev.of_node) {
 		ret = of_irq_get(dev->dev.of_node, num);
 		if (ret > 0 || ret == -EPROBE_DEFER)
-			return ret;
+			goto out;
 	}
 
 	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
@@ -157,7 +158,7 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		if (r && r->flags & IORESOURCE_DISABLED) {
 			ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
 			if (ret)
-				return ret;
+				goto out;
 		}
 	}
 
@@ -171,13 +172,17 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		struct irq_data *irqd;
 
 		irqd = irq_get_irq_data(r->start);
-		if (!irqd)
-			return -ENXIO;
+		if (!irqd) {
+			ret = -ENXIO;
+			goto out;
+		}
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	if (r)
-		return r->start;
+	if (r) {
+		ret = r->start;
+		goto out;
+	}
 
 	/*
 	 * For the index 0 interrupt, allow falling back to GpioInt
@@ -190,11 +195,14 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num);
 		/* Our callers expect -ENXIO for missing IRQs. */
 		if (ret >= 0 || ret == -EPROBE_DEFER)
-			return ret;
+			goto out;
 	}
 
-	return -ENXIO;
+	ret = -ENXIO;
 #endif
+out:
+	WARN(ret == 0, "0 is an invalid IRQ number\n");
+	return ret;
 }
 EXPORT_SYMBOL_GPL(platform_get_irq_optional);
 
@@ -212,7 +220,7 @@ EXPORT_SYMBOL_GPL(platform_get_irq_optional);
  *		if (irq < 0)
  *			return irq;
  *
- * Return: IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, negative error number on failure.
  */
 int platform_get_irq(struct platform_device *dev, unsigned int num)
 {
@@ -284,8 +292,10 @@ static int __platform_get_irq_byname(struct platform_device *dev,
 	}
 
 	r = platform_get_resource_byname(dev, IORESOURCE_IRQ, name);
-	if (r)
+	if (r) {
+		WARN(r->start == 0, "0 is an invalid IRQ number\n");
 		return r->start;
+	}
 
 	return -ENXIO;
 }
@@ -297,7 +307,7 @@ static int __platform_get_irq_byname(struct platform_device *dev,
  *
  * Get an IRQ like platform_get_irq(), but then by name rather then by index.
  *
- * Return: IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, negative error number on failure.
  */
 int platform_get_irq_byname(struct platform_device *dev, const char *name)
 {
@@ -319,7 +329,7 @@ EXPORT_SYMBOL_GPL(platform_get_irq_byname);
  * Get an optional IRQ by name like platform_get_irq_byname(). Except that it
  * does not print an error message if an IRQ can not be obtained.
  *
- * Return: IRQ number on success, negative error number on failure.
+ * Return: non-zero IRQ number on success, negative error number on failure.
  */
 int platform_get_irq_byname_optional(struct platform_device *dev,
 				     const char *name)