[PATCH net-next] tg3: inconsistent interrupt value reported

[PATCH net-next] tg3: inconsistent interrupt value reported

[Andy Gospodarek]

Systems that use MSI do not report the correct 'Interrupt' value in
ifconfig output.  This patch sets dev->irq after initializing the
interrupt as the value will change when using MSI.

I suspect this is a problem with a lot of drivers, so I'll snoop around
and post some more patches if needed.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
---

 tg3.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index eb9f8f3..fa95e99 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -7892,6 +7892,7 @@ static int tg3_request_irq(struct tg3 *tp)
 	irq_handler_t fn;
 	unsigned long flags;
 	struct net_device *dev = tp->dev;
+	int ret;
 
 	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
 		fn = tg3_msi;
@@ -7904,7 +7905,9 @@ static int tg3_request_irq(struct tg3 *tp)
 			fn = tg3_interrupt_tagged;
 		flags = IRQF_SHARED | IRQF_SAMPLE_RANDOM;
 	}
-	return (request_irq(tp->pdev->irq, fn, flags, dev->name, dev));
+	ret = request_irq(tp->pdev->irq, fn, flags, dev->name, dev);
+	dev->irq = tp->pdev->irq;
+	return ret;
 }
 
 static int tg3_test_interrupt(struct tg3 *tp)

Re: [PATCH net-next] tg3: inconsistent interrupt value reported

[Jeff Garzik]

Andy Gospodarek wrote:
> Systems that use MSI do not report the correct 'Interrupt' value in
> ifconfig output.  This patch sets dev->irq after initializing the
> interrupt as the value will change when using MSI.
> 
> I suspect this is a problem with a lot of drivers, so I'll snoop around
> and post some more patches if needed.
> 
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
> ---
> 
>  tg3.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index eb9f8f3..fa95e99 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -7892,6 +7892,7 @@ static int tg3_request_irq(struct tg3 *tp)
>  	irq_handler_t fn;
>  	unsigned long flags;
>  	struct net_device *dev = tp->dev;
> +	int ret;
>  
>  	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
>  		fn = tg3_msi;
> @@ -7904,7 +7905,9 @@ static int tg3_request_irq(struct tg3 *tp)
>  			fn = tg3_interrupt_tagged;
>  		flags = IRQF_SHARED | IRQF_SAMPLE_RANDOM;
>  	}
> -	return (request_irq(tp->pdev->irq, fn, flags, dev->name, dev));
> +	ret = request_irq(tp->pdev->irq, fn, flags, dev->name, dev);
> +	dev->irq = tp->pdev->irq;
> +	return ret;

The ifconfig value is useless, and was never designed to carry and 
display that kind of information (MSI vectors, etc.).

In fact, part of the reason why the ethtool bus-id information was added 
was to give better insight into the hardware attached to the device, 
notably including the interrupt information.

In modern drivers (read: newer than [E]ISA), dev->irq use is 
inconsistent and largely for __best effort__ display purposes only.

I'd argue that a policy of leaving dev->irq at zero might be a better 
idea.  That ensures users do not pay attention to what is already an 
inconsistent/truncate/device-dependent piece of information.

	Jeff

Re: [PATCH net-next] tg3: inconsistent interrupt value reported

[Andy Gospodarek]

On Fri, Oct 31, 2008 at 12:09:08PM -0400, Jeff Garzik wrote:
> Andy Gospodarek wrote:
>> Systems that use MSI do not report the correct 'Interrupt' value in
>> ifconfig output.  This patch sets dev->irq after initializing the
>> interrupt as the value will change when using MSI.
>>
>> I suspect this is a problem with a lot of drivers, so I'll snoop around
>> and post some more patches if needed.
>>
>> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>> ---
>>
>>  tg3.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>> index eb9f8f3..fa95e99 100644
>> --- a/drivers/net/tg3.c
>> +++ b/drivers/net/tg3.c
>> @@ -7892,6 +7892,7 @@ static int tg3_request_irq(struct tg3 *tp)
>>  	irq_handler_t fn;
>>  	unsigned long flags;
>>  	struct net_device *dev = tp->dev;
>> +	int ret;
>>   	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
>>  		fn = tg3_msi;
>> @@ -7904,7 +7905,9 @@ static int tg3_request_irq(struct tg3 *tp)
>>  			fn = tg3_interrupt_tagged;
>>  		flags = IRQF_SHARED | IRQF_SAMPLE_RANDOM;
>>  	}
>> -	return (request_irq(tp->pdev->irq, fn, flags, dev->name, dev));
>> +	ret = request_irq(tp->pdev->irq, fn, flags, dev->name, dev);
>> +	dev->irq = tp->pdev->irq;
>> +	return ret;
>
> The ifconfig value is useless, and was never designed to carry and  
> display that kind of information (MSI vectors, etc.).
>
> In fact, part of the reason why the ethtool bus-id information was added  
> was to give better insight into the hardware attached to the device,  
> notably including the interrupt information.
>
> In modern drivers (read: newer than [E]ISA), dev->irq use is  
> inconsistent and largely for __best effort__ display purposes only.
>
> I'd argue that a policy of leaving dev->irq at zero might be a better  
> idea.  That ensures users do not pay attention to what is already an  
> inconsistent/truncate/device-dependent piece of information.
>

This came up because the output is currently wrong.

# ifconfig eth0 | grep Inter && grep eth0 /proc/interrupts
          Interrupt:169 Memory:f6000000-f6012100
122:       2894           0          0         PCI-MSI  eth0

Some drivers take the stand of not setting dev->irq anything (like most
of the Intel drivers), but I didn't take that route with tg3 (or with
the others I'd planned) simply because I didn't want to hear crying
about breaking user-space with a patch that would make that line
disappear.

If this seems like a reasonable change that we can force on user-space
I'll post a patch that drops the setting of dev->irq all together, so
this disappers.

zeroing dev->irq (was Re: [PATCH net-next] tg3: inconsistent interrupt value reported)

[Jeff Garzik]

Andy Gospodarek wrote:
> On Fri, Oct 31, 2008 at 12:09:08PM -0400, Jeff Garzik wrote:
>> Andy Gospodarek wrote:
>>> Systems that use MSI do not report the correct 'Interrupt' value in
>>> ifconfig output.  This patch sets dev->irq after initializing the
>>> interrupt as the value will change when using MSI.
>>>
>>> I suspect this is a problem with a lot of drivers, so I'll snoop around
>>> and post some more patches if needed.
>>>
>>> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>>> ---
>>>
>>>  tg3.c |    5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
>>> index eb9f8f3..fa95e99 100644
>>> --- a/drivers/net/tg3.c
>>> +++ b/drivers/net/tg3.c
>>> @@ -7892,6 +7892,7 @@ static int tg3_request_irq(struct tg3 *tp)
>>>  	irq_handler_t fn;
>>>  	unsigned long flags;
>>>  	struct net_device *dev = tp->dev;
>>> +	int ret;
>>>   	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
>>>  		fn = tg3_msi;
>>> @@ -7904,7 +7905,9 @@ static int tg3_request_irq(struct tg3 *tp)
>>>  			fn = tg3_interrupt_tagged;
>>>  		flags = IRQF_SHARED | IRQF_SAMPLE_RANDOM;
>>>  	}
>>> -	return (request_irq(tp->pdev->irq, fn, flags, dev->name, dev));
>>> +	ret = request_irq(tp->pdev->irq, fn, flags, dev->name, dev);
>>> +	dev->irq = tp->pdev->irq;
>>> +	return ret;
>> The ifconfig value is useless, and was never designed to carry and  
>> display that kind of information (MSI vectors, etc.).
>>
>> In fact, part of the reason why the ethtool bus-id information was added  
>> was to give better insight into the hardware attached to the device,  
>> notably including the interrupt information.
>>
>> In modern drivers (read: newer than [E]ISA), dev->irq use is  
>> inconsistent and largely for __best effort__ display purposes only.
>>
>> I'd argue that a policy of leaving dev->irq at zero might be a better  
>> idea.  That ensures users do not pay attention to what is already an  
>> inconsistent/truncate/device-dependent piece of information.
>>
> 
> This came up because the output is currently wrong.
> 
> # ifconfig eth0 | grep Inter && grep eth0 /proc/interrupts
>           Interrupt:169 Memory:f6000000-f6012100
> 122:       2894           0          0         PCI-MSI  eth0
> 
> Some drivers take the stand of not setting dev->irq anything (like most
> of the Intel drivers), but I didn't take that route with tg3 (or with
> the others I'd planned) simply because I didn't want to hear crying
> about breaking user-space with a patch that would make that line
> disappear.
> 
> If this seems like a reasonable change that we can force on user-space
> I'll post a patch that drops the setting of dev->irq all together, so
> this disappers.

I would vote for zeroing dev->irq in not only tg3, but also other 
modern, ethtool-enabled drivers...  It is a relic of the ISA days, and 
is incompatible with multiple MSI vector scenarios, something also found 
on some non-x86 and embedded ethernet drivers.

For years now, dev->irq has been providing information on an unreliable, 
best-effort basis.  I prefer definitive, reliable, predictable 
behaviors, and think always-zero is therefore an improvement.

Comments welcome...

	Jeff

Re: zeroing dev->irq

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 31 Oct 2008 12:47:50 -0400

> Andy Gospodarek wrote:
> > This came up because the output is currently wrong.
> > # ifconfig eth0 | grep Inter && grep eth0 /proc/interrupts
> >           Interrupt:169 Memory:f6000000-f6012100
> > 122:       2894           0          0         PCI-MSI  eth0
> > Some drivers take the stand of not setting dev->irq anything (like most
> > of the Intel drivers), but I didn't take that route with tg3 (or with
> > the others I'd planned) simply because I didn't want to hear crying
> > about breaking user-space with a patch that would make that line
> > disappear.
> > If this seems like a reasonable change that we can force on user-space
> > I'll post a patch that drops the setting of dev->irq all together, so
> > this disappers.
>
> I would vote for zeroing dev->irq in not only tg3, but also other
> modern, ethtool-enabled drivers...  It is a relic of the ISA days,
> and is incompatible with multiple MSI vector scenarios, something
> also found on some non-x86 and embedded ethernet drivers.
>
> For years now, dev->irq has been providing information on an
> unreliable, best-effort basis.  I prefer definitive, reliable,
> predictable behaviors, and think always-zero is therefore an
> improvement.
>
> Comments welcome...

I completely agree with Jeff and I'll happily take patches
which do this into net-next-2.6