Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1219262 > unrolled thread

[PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.

Started byDavid Daney <ddaney.cavm@gmail.com>
First post2015-09-04 21:20 +0200
Last post2015-09-08 01:50 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages. David Daney <ddaney.cavm@gmail.com> - 2015-09-04 21:20 +0200
    Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed  ..." messages. Frank Rowand <frowand.list@gmail.com> - 2015-09-05 03:20 +0200
      Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed  ..." messages. David Daney <ddaney.cavm@gmail.com> - 2015-09-05 03:50 +0200
        Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed  ..." messages. Frank Rowand <frowand.list@gmail.com> - 2015-09-05 04:40 +0200
      Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages. Rob Herring <robherring2@gmail.com> - 2015-09-06 22:50 +0200
        Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed  ..." messages. Frank Rowand <frowand.list@gmail.com> - 2015-09-07 04:20 +0200
          Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed  ..." messages. Frank Rowand <frowand.list@gmail.com> - 2015-09-07 06:00 +0200
            Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed  ..." messages. Frank Rowand <frowand.list@gmail.com> - 2015-09-08 01:50 +0200

#1219262 — [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.

FromDavid Daney <ddaney.cavm@gmail.com>
Date2015-09-04 21:20 +0200
Subject[PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
Message-ID<q54GK-2JP-1@gated-at.bofh.it>
From: David Daney <david.daney@cavium.com>

It is perfectly legitimate for a PCI device to have an
PCI_INTERRUPT_PIN value of zero.  This happens if the device doesn't
use interrupts, or on PCIe devices, where only MSI/MSI-X are
supported.

Silence the annoying "of_irq_parse_pci() failed with rc=-19" error
messages by making them conditional on !-ENODEV (which can only be
produced in the PCI_INTERRUPT_PIN == 0 case).

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/of/of_pci_irq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index 1710d9d..33d242a 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -106,7 +106,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
 
 	ret = of_irq_parse_pci(dev, &oirq);
 	if (ret) {
-		dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
+		if (ret != -ENODEV)
+			dev_err(&dev->dev,
+				"of_irq_parse_pci() failed with rc=%d\n", ret);
 		return 0; /* Proper return code 0 == NO_IRQ */
 	}
 
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1219408 — Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.

FromFrank Rowand <frowand.list@gmail.com>
Date2015-09-05 03:20 +0200
SubjectRe: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
Message-ID<q5aj8-2hC-7@gated-at.bofh.it>
In reply to#1219262
On 9/4/2015 12:12 PM, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> It is perfectly legitimate for a PCI device to have an
> PCI_INTERRUPT_PIN value of zero.  This happens if the device doesn't
> use interrupts, or on PCIe devices, where only MSI/MSI-X are
> supported.
> 
> Silence the annoying "of_irq_parse_pci() failed with rc=-19" error
> messages by making them conditional on !-ENODEV (which can only be
> produced in the PCI_INTERRUPT_PIN == 0 case).
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/of/of_pci_irq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> index 1710d9d..33d242a 100644
> --- a/drivers/of/of_pci_irq.c
> +++ b/drivers/of/of_pci_irq.c
> @@ -106,7 +106,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
>  
>  	ret = of_irq_parse_pci(dev, &oirq);
>  	if (ret) {
> -		dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
> +		if (ret != -ENODEV)
> +			dev_err(&dev->dev,
> +				"of_irq_parse_pci() failed with rc=%d\n", ret);
>  		return 0; /* Proper return code 0 == NO_IRQ */
>  	}
>  
> 

It is not safe to assume that the functions that of_irq_parse_pci() calls
will never be modified to return -ENODEV, thus resulting in of_irq_parse_pci()
returning -ENODEV for a reason other than PCI_INTERRUPT_PIN == 0.

A more robust solution would be something like:


(1) Change of_irq_parse_pci() to _of_irq_parse_pci(), adding an argument and
use it to report the case of PCI_INTERRUPT_PIN == 0.

static int _of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq, int *no_pin)
{

...
	*no_pin = 0;
...
	/* No pin, exit */
	if (pin == 0) {
		*no_pin = 1;
		return -ENODEV;
	}
...


int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
{
	int no_pin;
	return _of_irq_parse_pci(pdev, out_irq, &no_pin)
}


(2) Then the fix to of_irq_parse_and_map_pci() would be:

  +     int no_pin;
>  	ret = of_irq_parse_pci(dev, &oirq, &no_pin);
>  	if (ret) {
> -		dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
> +		if (!no_pin)
> +			dev_err(&dev->dev,
> +				"of_irq_parse_pci() failed with rc=%d\n", ret);
>  		return 0; /* Proper return code 0 == NO_IRQ */
>  	}


I'm not sure I like my solution, there might be a better way.

I also noticed another bug while looking at of_irq_parse_pci().  It returns
the non-zero return value from pci_read_config_byte().  But that value is
one of the PCI function error values from include/linux/pci.h, such as:

   #define PCIBIOS_BAD_REGISTER_NUMBER     0x87

instead of a negative errno.


-Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219412 — Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.

FromDavid Daney <ddaney.cavm@gmail.com>
Date2015-09-05 03:50 +0200
SubjectRe: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
Message-ID<q5aMa-2OO-7@gated-at.bofh.it>
In reply to#1219408
On 09/04/2015 06:14 PM, Frank Rowand wrote:
> On 9/4/2015 12:12 PM, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> It is perfectly legitimate for a PCI device to have an
>> PCI_INTERRUPT_PIN value of zero.  This happens if the device doesn't
>> use interrupts, or on PCIe devices, where only MSI/MSI-X are
>> supported.
>>
>> Silence the annoying "of_irq_parse_pci() failed with rc=-19" error
>> messages by making them conditional on !-ENODEV (which can only be
>> produced in the PCI_INTERRUPT_PIN == 0 case).
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   drivers/of/of_pci_irq.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
>> index 1710d9d..33d242a 100644
>> --- a/drivers/of/of_pci_irq.c
>> +++ b/drivers/of/of_pci_irq.c
>> @@ -106,7 +106,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
>>
>>   	ret = of_irq_parse_pci(dev, &oirq);
>>   	if (ret) {
>> -		dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
>> +		if (ret != -ENODEV)
>> +			dev_err(&dev->dev,
>> +				"of_irq_parse_pci() failed with rc=%d\n", ret);
>>   		return 0; /* Proper return code 0 == NO_IRQ */
>>   	}
>>
>>
>
> It is not safe to assume that the functions that of_irq_parse_pci() calls
> will never be modified to return -ENODEV, thus resulting in of_irq_parse_pci()
> returning -ENODEV for a reason other than PCI_INTERRUPT_PIN == 0.
>

Since the current implementation *only ever* returns -ENODEV for 
PCI_INTERRUPT_PIN == 0, we could just document that behavior, and not 
hack up the APIs by adding a second return channel from the function.

The additional change on top of my patch would be to add a comment 
describing this behavior.

David Daney.



> A more robust solution would be something like:
>
>
> (1) Change of_irq_parse_pci() to _of_irq_parse_pci(), adding an argument and
> use it to report the case of PCI_INTERRUPT_PIN == 0.
>
> static int _of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq, int *no_pin)
> {
>
> ...
> 	*no_pin = 0;
> ...
> 	/* No pin, exit */
> 	if (pin == 0) {
> 		*no_pin = 1;
> 		return -ENODEV;
> 	}
> ...
>
>
> int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
> {
> 	int no_pin;
> 	return _of_irq_parse_pci(pdev, out_irq, &no_pin)
> }
>
>
> (2) Then the fix to of_irq_parse_and_map_pci() would be:
>
>    +     int no_pin;
>>   	ret = of_irq_parse_pci(dev, &oirq, &no_pin);
>>   	if (ret) {
>> -		dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
>> +		if (!no_pin)
>> +			dev_err(&dev->dev,
>> +				"of_irq_parse_pci() failed with rc=%d\n", ret);
>>   		return 0; /* Proper return code 0 == NO_IRQ */
>>   	}
>
>
> I'm not sure I like my solution, there might be a better way.
>
> I also noticed another bug while looking at of_irq_parse_pci().  It returns
> the non-zero return value from pci_read_config_byte().  But that value is
> one of the PCI function error values from include/linux/pci.h, such as:
>
>     #define PCIBIOS_BAD_REGISTER_NUMBER     0x87
>
> instead of a negative errno.
>
>
> -Frank
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219416 — Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.

FromFrank Rowand <frowand.list@gmail.com>
Date2015-09-05 04:40 +0200
SubjectRe: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
Message-ID<q5byx-42X-1@gated-at.bofh.it>
In reply to#1219412
On 9/4/2015 6:40 PM, David Daney wrote:
> On 09/04/2015 06:14 PM, Frank Rowand wrote:
>> On 9/4/2015 12:12 PM, David Daney wrote:
>>> From: David Daney <david.daney@cavium.com>
>>>
>>> It is perfectly legitimate for a PCI device to have an
>>> PCI_INTERRUPT_PIN value of zero.  This happens if the device doesn't
>>> use interrupts, or on PCIe devices, where only MSI/MSI-X are
>>> supported.
>>>
>>> Silence the annoying "of_irq_parse_pci() failed with rc=-19" error
>>> messages by making them conditional on !-ENODEV (which can only be
>>> produced in the PCI_INTERRUPT_PIN == 0 case).
>>>
>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>> ---
>>>   drivers/of/of_pci_irq.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
>>> index 1710d9d..33d242a 100644
>>> --- a/drivers/of/of_pci_irq.c
>>> +++ b/drivers/of/of_pci_irq.c
>>> @@ -106,7 +106,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
>>>
>>>       ret = of_irq_parse_pci(dev, &oirq);
>>>       if (ret) {
>>> -        dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
>>> +        if (ret != -ENODEV)
>>> +            dev_err(&dev->dev,
>>> +                "of_irq_parse_pci() failed with rc=%d\n", ret);
>>>           return 0; /* Proper return code 0 == NO_IRQ */
>>>       }
>>>
>>>
>>
>> It is not safe to assume that the functions that of_irq_parse_pci() calls
>> will never be modified to return -ENODEV, thus resulting in of_irq_parse_pci()
>> returning -ENODEV for a reason other than PCI_INTERRUPT_PIN == 0.
>>
> 
> Since the current implementation *only ever* returns -ENODEV for PCI_INTERRUPT_PIN == 0, we could just document that behavior, and not hack up the APIs by adding a second return channel from the function.

And for each function that of_irq_parse_pci() calls and returns status from the
function call you would need to document that behavior, and so on recursively.

For example, you would need to document of_irq_parse_one().  And that returns status
from more function calls, so you would need to document of_irq_parse_oldworld(),
of_parse_phandle_with_args(), of_irq_parse_raw(), and then anything they call.
> 
> The additional change on top of my patch would be to add a comment describing this behavior.
> 
> David Daney.

< snip >

-Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219859

FromRob Herring <robherring2@gmail.com>
Date2015-09-06 22:50 +0200
Message-ID<q5P2W-1gv-15@gated-at.bofh.it>
In reply to#1219408
On Fri, Sep 4, 2015 at 8:14 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 9/4/2015 12:12 PM, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> It is perfectly legitimate for a PCI device to have an
>> PCI_INTERRUPT_PIN value of zero.  This happens if the device doesn't
>> use interrupts, or on PCIe devices, where only MSI/MSI-X are
>> supported.
>>
>> Silence the annoying "of_irq_parse_pci() failed with rc=-19" error
>> messages by making them conditional on !-ENODEV (which can only be
>> produced in the PCI_INTERRUPT_PIN == 0 case).
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>  drivers/of/of_pci_irq.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
>> index 1710d9d..33d242a 100644
>> --- a/drivers/of/of_pci_irq.c
>> +++ b/drivers/of/of_pci_irq.c
>> @@ -106,7 +106,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
>>
>>       ret = of_irq_parse_pci(dev, &oirq);
>>       if (ret) {
>> -             dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
>> +             if (ret != -ENODEV)
>> +                     dev_err(&dev->dev,
>> +                             "of_irq_parse_pci() failed with rc=%d\n", ret);
>>               return 0; /* Proper return code 0 == NO_IRQ */
>>       }
>>
>>
>
> It is not safe to assume that the functions that of_irq_parse_pci() calls
> will never be modified to return -ENODEV, thus resulting in of_irq_parse_pci()
> returning -ENODEV for a reason other than PCI_INTERRUPT_PIN == 0.

Yes, but we're talking about a print statement.

>
> A more robust solution would be something like:
>
>
> (1) Change of_irq_parse_pci() to _of_irq_parse_pci(), adding an argument and
> use it to report the case of PCI_INTERRUPT_PIN == 0.
>
> static int _of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq, int *no_pin)
> {
>
> ...
>         *no_pin = 0;
> ...
>         /* No pin, exit */
>         if (pin == 0) {
>                 *no_pin = 1;
>                 return -ENODEV;
>         }
> ...
>
>
> int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
> {
>         int no_pin;
>         return _of_irq_parse_pci(pdev, out_irq, &no_pin)
> }
>
>
> (2) Then the fix to of_irq_parse_and_map_pci() would be:
>
>   +     int no_pin;
>>       ret = of_irq_parse_pci(dev, &oirq, &no_pin);
>>       if (ret) {
>> -             dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
>> +             if (!no_pin)
>> +                     dev_err(&dev->dev,
>> +                             "of_irq_parse_pci() failed with rc=%d\n", ret);
>>               return 0; /* Proper return code 0 == NO_IRQ */
>>       }
>
>
> I'm not sure I like my solution, there might be a better way.

I don't like it. That's way too complex for just silencing an
erroneous error message.

Perhaps just move the error message into of_irq_parse_pci and then you
can control the print more easily. Or just change to dev_dbg would be
okay by me.


> I also noticed another bug while looking at of_irq_parse_pci().  It returns
> the non-zero return value from pci_read_config_byte().  But that value is
> one of the PCI function error values from include/linux/pci.h, such as:
>
>    #define PCIBIOS_BAD_REGISTER_NUMBER     0x87
>
> instead of a negative errno.

I was puzzled by why this is not standard error codes a while back. My
best guess is that that there is some legacy here. Changing error
values on widely used functions is impossible to audit. NO_IRQ being 0
or -1 is one such case.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219894 — Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.

FromFrank Rowand <frowand.list@gmail.com>
Date2015-09-07 04:20 +0200
SubjectRe: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
Message-ID<q5Uch-le-1@gated-at.bofh.it>
In reply to#1219859
On 9/6/2015 1:46 PM, Rob Herring wrote:
> On Fri, Sep 4, 2015 at 8:14 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 9/4/2015 12:12 PM, David Daney wrote:
>>> From: David Daney <david.daney@cavium.com>
>>>
>>> It is perfectly legitimate for a PCI device to have an
>>> PCI_INTERRUPT_PIN value of zero.  This happens if the device doesn't
>>> use interrupts, or on PCIe devices, where only MSI/MSI-X are
>>> supported.
>>>
>>> Silence the annoying "of_irq_parse_pci() failed with rc=-19" error
>>> messages by making them conditional on !-ENODEV (which can only be
>>> produced in the PCI_INTERRUPT_PIN == 0 case).
>>>
>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>> ---
>>>  drivers/of/of_pci_irq.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
>>> index 1710d9d..33d242a 100644
>>> --- a/drivers/of/of_pci_irq.c
>>> +++ b/drivers/of/of_pci_irq.c
>>> @@ -106,7 +106,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
>>>
>>>       ret = of_irq_parse_pci(dev, &oirq);
>>>       if (ret) {
>>> -             dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
>>> +             if (ret != -ENODEV)
>>> +                     dev_err(&dev->dev,
>>> +                             "of_irq_parse_pci() failed with rc=%d\n", ret);
>>>               return 0; /* Proper return code 0 == NO_IRQ */
>>>       }
>>>
>>>
>>
>> It is not safe to assume that the functions that of_irq_parse_pci() calls
>> will never be modified to return -ENODEV, thus resulting in of_irq_parse_pci()
>> returning -ENODEV for a reason other than PCI_INTERRUPT_PIN == 0.
> 
> Yes, but we're talking about a print statement.
> 
>>
>> A more robust solution would be something like:

< snip my bad solution >

>> I'm not sure I like my solution, there might be a better way.
> 
> I don't like it. That's way too complex for just silencing an
> erroneous error message.
> 
> Perhaps just move the error message into of_irq_parse_pci and then you
> can control the print more easily. Or just change to dev_dbg would be
> okay by me.

I knew I was making it way too hard.  Yes, just move the error message
to of_irq_parse_pci(), where the "/* No pin, exit */" test occurs.

 
>> I also noticed another bug while looking at of_irq_parse_pci().  It returns
>> the non-zero return value from pci_read_config_byte().  But that value is
>> one of the PCI function error values from include/linux/pci.h, such as:
>>
>>    #define PCIBIOS_BAD_REGISTER_NUMBER     0x87
>>
>> instead of a negative errno.
> 
> I was puzzled by why this is not standard error codes a while back. My
> best guess is that that there is some legacy here. Changing error
> values on widely used functions is impossible to audit. NO_IRQ being 0
> or -1 is one such case.
> 
> Rob
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219914 — Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.

FromFrank Rowand <frowand.list@gmail.com>
Date2015-09-07 06:00 +0200
SubjectRe: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
Message-ID<q5VL3-2oy-7@gated-at.bofh.it>
In reply to#1219894
On 9/6/2015 7:16 PM, Frank Rowand wrote:
> On 9/6/2015 1:46 PM, Rob Herring wrote:
>> On Fri, Sep 4, 2015 at 8:14 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>>> On 9/4/2015 12:12 PM, David Daney wrote:
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>>>> It is perfectly legitimate for a PCI device to have an
>>>> PCI_INTERRUPT_PIN value of zero.  This happens if the device doesn't
>>>> use interrupts, or on PCIe devices, where only MSI/MSI-X are
>>>> supported.
>>>>
>>>> Silence the annoying "of_irq_parse_pci() failed with rc=-19" error
>>>> messages by making them conditional on !-ENODEV (which can only be
>>>> produced in the PCI_INTERRUPT_PIN == 0 case).
>>>>
>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>> ---
>>>>  drivers/of/of_pci_irq.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
>>>> index 1710d9d..33d242a 100644
>>>> --- a/drivers/of/of_pci_irq.c
>>>> +++ b/drivers/of/of_pci_irq.c
>>>> @@ -106,7 +106,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
>>>>
>>>>       ret = of_irq_parse_pci(dev, &oirq);
>>>>       if (ret) {
>>>> -             dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
>>>> +             if (ret != -ENODEV)
>>>> +                     dev_err(&dev->dev,
>>>> +                             "of_irq_parse_pci() failed with rc=%d\n", ret);
>>>>               return 0; /* Proper return code 0 == NO_IRQ */
>>>>       }
>>>>
>>>>
>>>
>>> It is not safe to assume that the functions that of_irq_parse_pci() calls
>>> will never be modified to return -ENODEV, thus resulting in of_irq_parse_pci()
>>> returning -ENODEV for a reason other than PCI_INTERRUPT_PIN == 0.
>>
>> Yes, but we're talking about a print statement.
>>
>>>
>>> A more robust solution would be something like:
> 
> < snip my bad solution >
> 
>>> I'm not sure I like my solution, there might be a better way.
>>
>> I don't like it. That's way too complex for just silencing an
>> erroneous error message.
>>
>> Perhaps just move the error message into of_irq_parse_pci and then you
>> can control the print more easily. Or just change to dev_dbg would be
>> okay by me.
> 
> I knew I was making it way too hard.  Yes, just move the error message
> to of_irq_parse_pci(), where the "/* No pin, exit */" test occurs.

And this time I replied too quickly, not really thinking through my comment.
There are several error return points in of_irq_parse_pci(), so moving the
error message into of_irq_parse_pci() is not the answer.


>>> I also noticed another bug while looking at of_irq_parse_pci().  It returns
>>> the non-zero return value from pci_read_config_byte().  But that value is
>>> one of the PCI function error values from include/linux/pci.h, such as:
>>>
>>>    #define PCIBIOS_BAD_REGISTER_NUMBER     0x87
>>>
>>> instead of a negative errno.
>>
>> I was puzzled by why this is not standard error codes a while back. My
>> best guess is that that there is some legacy here. Changing error
>> values on widely used functions is impossible to audit. NO_IRQ being 0
>> or -1 is one such case.
>>
>> Rob
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220422 — Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.

FromFrank Rowand <frowand.list@gmail.com>
Date2015-09-08 01:50 +0200
SubjectRe: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
Message-ID<q6ekF-3RF-3@gated-at.bofh.it>
In reply to#1219914
On 9/6/2015 8:50 PM, Frank Rowand wrote:
> On 9/6/2015 7:16 PM, Frank Rowand wrote:
>> On 9/6/2015 1:46 PM, Rob Herring wrote:
>>> On Fri, Sep 4, 2015 at 8:14 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>>>> On 9/4/2015 12:12 PM, David Daney wrote:
>>>>> From: David Daney <david.daney@cavium.com>
>>>>>
>>>>> It is perfectly legitimate for a PCI device to have an
>>>>> PCI_INTERRUPT_PIN value of zero.  This happens if the device doesn't
>>>>> use interrupts, or on PCIe devices, where only MSI/MSI-X are
>>>>> supported.
>>>>>
>>>>> Silence the annoying "of_irq_parse_pci() failed with rc=-19" error
>>>>> messages by making them conditional on !-ENODEV (which can only be
>>>>> produced in the PCI_INTERRUPT_PIN == 0 case).
>>>>>
>>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>>> ---
>>>>>  drivers/of/of_pci_irq.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
>>>>> index 1710d9d..33d242a 100644
>>>>> --- a/drivers/of/of_pci_irq.c
>>>>> +++ b/drivers/of/of_pci_irq.c
>>>>> @@ -106,7 +106,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin)
>>>>>
>>>>>       ret = of_irq_parse_pci(dev, &oirq);
>>>>>       if (ret) {
>>>>> -             dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
>>>>> +             if (ret != -ENODEV)
>>>>> +                     dev_err(&dev->dev,
>>>>> +                             "of_irq_parse_pci() failed with rc=%d\n", ret);
>>>>>               return 0; /* Proper return code 0 == NO_IRQ */
>>>>>       }
>>>>>
>>>>>
>>>>
>>>> It is not safe to assume that the functions that of_irq_parse_pci() calls
>>>> will never be modified to return -ENODEV, thus resulting in of_irq_parse_pci()
>>>> returning -ENODEV for a reason other than PCI_INTERRUPT_PIN == 0.
>>>
>>> Yes, but we're talking about a print statement.
>>>
>>>>
>>>> A more robust solution would be something like:
>>
>> < snip my bad solution >
>>
>>>> I'm not sure I like my solution, there might be a better way.
>>>
>>> I don't like it. That's way too complex for just silencing an
>>> erroneous error message.
>>>
>>> Perhaps just move the error message into of_irq_parse_pci and then you
>>> can control the print more easily. Or just change to dev_dbg would be
>>> okay by me.
>>
>> I knew I was making it way too hard.  Yes, just move the error message
>> to of_irq_parse_pci(), where the "/* No pin, exit */" test occurs.
> 
> And this time I replied too quickly, not really thinking through my comment.
> There are several error return points in of_irq_parse_pci(), so moving the
> error message into of_irq_parse_pci() is not the answer.

                                        is not the answer unless of_irq_parse_pci()
  is changed over to the single point of return style.

I realized I should have typed the whole thought...

-Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web