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

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

Hi Aman,

1) Check your mailer config.  These messages had no "To:" header, so
replying didn't work correctly.  I added "Cc: linux-pci" manually, but
on the mailing lists, the convention is to "reply-all" so everybody
can participate.

2) The cc list is a little bit overboard.
"$ ./scripts/get_maintainer.pl -f drivers/pci/controller/pci-v3-semi.c"
shows Linus W, Lorenzo, Andrew, myself, linux-pci, linux-kernel.
That's plenty.

3) Study "git log drivers/pci" and make your commit subjects and
logs match the convention in capitalization, sentence structure, verb
tense, etc.

4) Every commit must have non-empty log, even if the commit seems
trivial.  The log message should be independent of the subject.  The
subject is like an essay title; the log message is like the essay
body.  The body is separate from the title, not a continuation of it.

5) Function names in subjects and logs have "()" after them.

6) Cite previous similar work, e.g., mention ef75369a5b9a ("PCI:
altera: Fix platform_get_irq() error handling"), which is one of many
similar patches.

7) You mentioned similar issues with platform_get_irq_byname().
Please add patches in this series to fix them as well.

8) You asked about dev_err() usage.  See 6c9050a73469 ("irqchip:
Remove dev_err() usage after platform_get_irq()") and feel free to do
the same here.  If you do, cite that commit in your commit log.

I think the patches themselves look OK, and the series is correctly
structured with a cover letter and patches as responses to the cover.

When you post a revised series, make sure it's labeled "PATCH v2 0/5".

Thanks,
  Bjorn

On Thu, Mar 12, 2020 at 12:49:02AM +0530, Aman Sharma 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) {
>  		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
>

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

[+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

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

On Wed, Mar 11, 2020 at 03:57:23PM -0500, Bjorn Helgaas wrote:
> ...
> When you post a revised series, make sure it's labeled "PATCH v2 0/5".

Hi Aman,

What's your thought on this series?  I'd really like to see it go
forward.  I think we have general agreement that checking "irq < 0" is
the right way to do this.

Bjorn