* [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
@ 2015-09-04 19:12 David Daney
2015-09-05 1:14 ` Frank Rowand
0 siblings, 1 reply; 8+ messages in thread
From: David Daney @ 2015-09-04 19:12 UTC (permalink / raw)
To: devicetree, linux-kernel, Rob Herring, Frank Rowand, Grant Likely
Cc: David Daney
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
2015-09-04 19:12 [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages David Daney
@ 2015-09-05 1:14 ` Frank Rowand
2015-09-05 1:40 ` David Daney
2015-09-06 20:46 ` Rob Herring
0 siblings, 2 replies; 8+ messages in thread
From: Frank Rowand @ 2015-09-05 1:14 UTC (permalink / raw)
To: David Daney
Cc: devicetree, linux-kernel, Rob Herring, Grant Likely, David Daney
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
2015-09-05 1:14 ` Frank Rowand
@ 2015-09-05 1:40 ` David Daney
2015-09-05 2:38 ` Frank Rowand
2015-09-06 20:46 ` Rob Herring
1 sibling, 1 reply; 8+ messages in thread
From: David Daney @ 2015-09-05 1:40 UTC (permalink / raw)
To: frowand.list
Cc: devicetree, linux-kernel, Rob Herring, Grant Likely, David Daney
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
2015-09-05 1:40 ` David Daney
@ 2015-09-05 2:38 ` Frank Rowand
0 siblings, 0 replies; 8+ messages in thread
From: Frank Rowand @ 2015-09-05 2:38 UTC (permalink / raw)
To: David Daney
Cc: devicetree, linux-kernel, Rob Herring, Grant Likely, David Daney
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
2015-09-05 1:14 ` Frank Rowand
2015-09-05 1:40 ` David Daney
@ 2015-09-06 20:46 ` Rob Herring
2015-09-07 2:16 ` Frank Rowand
1 sibling, 1 reply; 8+ messages in thread
From: Rob Herring @ 2015-09-06 20:46 UTC (permalink / raw)
To: Frank Rowand
Cc: David Daney, devicetree, linux-kernel, Rob Herring, Grant Likely,
David Daney
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
2015-09-06 20:46 ` Rob Herring
@ 2015-09-07 2:16 ` Frank Rowand
2015-09-07 3:50 ` Frank Rowand
0 siblings, 1 reply; 8+ messages in thread
From: Frank Rowand @ 2015-09-07 2:16 UTC (permalink / raw)
To: Rob Herring
Cc: David Daney, devicetree, linux-kernel, Rob Herring, Grant Likely,
David Daney
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
2015-09-07 2:16 ` Frank Rowand
@ 2015-09-07 3:50 ` Frank Rowand
2015-09-07 23:44 ` Frank Rowand
0 siblings, 1 reply; 8+ messages in thread
From: Frank Rowand @ 2015-09-07 3:50 UTC (permalink / raw)
To: frowand.list
Cc: Rob Herring, David Daney, devicetree, linux-kernel, Rob Herring,
Grant Likely, David Daney
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
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages.
2015-09-07 3:50 ` Frank Rowand
@ 2015-09-07 23:44 ` Frank Rowand
0 siblings, 0 replies; 8+ messages in thread
From: Frank Rowand @ 2015-09-07 23:44 UTC (permalink / raw)
To: frowand.list
Cc: Rob Herring, David Daney, devicetree, linux-kernel, Rob Herring,
Grant Likely, David Daney
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-09-07 23:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-04 19:12 [PATCH] of_pci_irq: Silence bogus "of_irq_parse_pci() failed ..." messages David Daney
2015-09-05 1:14 ` Frank Rowand
2015-09-05 1:40 ` David Daney
2015-09-05 2:38 ` Frank Rowand
2015-09-06 20:46 ` Rob Herring
2015-09-07 2:16 ` Frank Rowand
2015-09-07 3:50 ` Frank Rowand
2015-09-07 23:44 ` Frank Rowand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).