via velocity trivial patch

via velocity trivial patch

[Radek Podgorny]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello everyone!

I've sent the following mail to Francois Romieu <romieu@fr.zoreil.com>
but did not get any reply. So I'm sending directly here. It's a trivial
patch for via-velocity driver.

Sincerely
Radek Podgorny


Radek Podgorny wrote:
> Hello,
> 
> attached is my trivial patch for the via-velocity driver. The actual
> change has been suggested years ago by Jeff Garzik
> (http://lkml.indiana.edu/hypermail/linux/kernel/0406.0/0500.html).
> 
> The problem:
> 
> When multiple velocity cards share the same interrupt (I have a 4x combo
> card on a very old motherboard without APIC or anything) the interrupt
> routine gets called for all of them (which is correct) but the chip that
> is not in question tries to react (which is wrong). The patch should
> explain the case better.
> 
> It has been tested on my old (and until now broken) system and on a
> modern one (APIC, no shared interrupts) and works flawlessly.
> 
> Sincerely
> Radek Podgorny
> 
> diff -ru linux-2.6.29.1-orig/drivers/net/via-velocity.c
> linux-2.6.29.1/drivers/net/via-velocity.c
> --- linux-2.6.29.1-orig/drivers/net/via-velocity.c	2009-04-02
> 22:55:27.000000000 +0200
> +++ linux-2.6.29.1/drivers/net/via-velocity.c	2009-04-24
> 13:09:45.945318255 +0200
> @@ -2226,8 +2226,8 @@
>  	spin_lock(&vptr->lock);
>  	isr_status = mac_read_isr(vptr->mac_regs);
> 
> -	/* Not us ? */
> -	if (isr_status == 0) {
> +	/* Not us or garbage ? */
> +	if (isr_status == 0 || isr_status == 0xffffffff) {
>  		spin_unlock(&vptr->lock);
>  		return IRQ_NONE;
>  	}
> @@ -2236,10 +2236,11 @@
> 
>  	/*
>  	 *	Keep processing the ISR until we have completed
> -	 *	processing and the isr_status becomes zero
> +	 *	processing and the isr_status becomes zero or
> +	 *	we read out some garbage
>  	 */
> 
> -	while (isr_status != 0) {
> +	while (isr_status != 0 && isr_status != 0xffffffff) {
>  		mac_write_isr(vptr->mac_regs, isr_status);
>  		if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
>  			velocity_error(vptr, isr_status);
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkoB+/UACgkQ7mej6pjlbYTriwCgvMZ7md0ilITiVqWQphzoyCHq
Ko4AoMqxNS2dSt6QyE29N6DK9eCInbMp
=/ZrZ
-----END PGP SIGNATURE-----

[-- Attachment #2: via-velocity.patch --]
[-- Type: text/plain, Size: 997 bytes --]

diff -ru linux-2.6.29.1-orig/drivers/net/via-velocity.c linux-2.6.29.1/drivers/net/via-velocity.c
--- linux-2.6.29.1-orig/drivers/net/via-velocity.c	2009-04-02 22:55:27.000000000 +0200
+++ linux-2.6.29.1/drivers/net/via-velocity.c	2009-04-24 13:09:45.945318255 +0200
@@ -2226,8 +2226,8 @@
 	spin_lock(&vptr->lock);
 	isr_status = mac_read_isr(vptr->mac_regs);
 
-	/* Not us ? */
-	if (isr_status == 0) {
+	/* Not us or garbage ? */
+	if (isr_status == 0 || isr_status == 0xffffffff) {
 		spin_unlock(&vptr->lock);
 		return IRQ_NONE;
 	}
@@ -2236,10 +2236,11 @@
 
 	/*
 	 *	Keep processing the ISR until we have completed
-	 *	processing and the isr_status becomes zero
+	 *	processing and the isr_status becomes zero or
+	 *	we read out some garbage
 	 */
 
-	while (isr_status != 0) {
+	while (isr_status != 0 && isr_status != 0xffffffff) {
 		mac_write_isr(vptr->mac_regs, isr_status);
 		if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
 			velocity_error(vptr, isr_status);

Re: via velocity trivial patch

[David Miller]

From: Radek Podgorny <radek@podgorny.cz>
Date: Wed, 06 May 2009 23:07:02 +0200

> I've sent the following mail to Francois Romieu <romieu@fr.zoreil.com>
> but did not get any reply. So I'm sending directly here. It's a trivial
> patch for via-velocity driver.

"some garbage"...

Why would the interrupt status register give us all 1's
if it is properly initialized?  I should report a zero,
rather than "some garbage".

You also failed to provide a proper sign-off as described
in Documentation/SubmittingPatches

Re: via velocity trivial patch

[Radek Podgorny]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi David,

thanks for the feedback. I apologize for the inconvenience, this is my
first try on kernel contribution.

The "some garbage" are just my words for what Jeff Garzik called "also
bail if 0xffffffff (hardware fault / hardware unplugged)" in
http://lkml.indiana.edu/hypermail/linux/kernel/0406.0/0500.html in 2004.
Should I change that?

Anyway, I've verified this is really happening on my hardware when
multiple via velocity devices share the same interrupt. The one the
interrupt is for reads the correct ISR value, the others sometimes read
0 and sometimes 0xffffffff. I'm not educated enough to tell why.

Reasons to include the fix:
1) Someone smarter than me suggested it (Jeff Garzik).
2) Fixed my problem.
3) Does not introduce a new problem.

So I'm attaching the patch again. Now with the proper sign-off.

Sincerely
Radek Podgorny

Signed-off-by: Radek Podgorny <radek@podgorny.cz>

diff -ru linux-2.6.29.1-orig/drivers/net/via-velocity.c linux-2.6.29.1/drivers/net/via-velocity.c
- --- linux-2.6.29.1-orig/drivers/net/via-velocity.c	2009-04-02 22:55:27.000000000 +0200
+++ linux-2.6.29.1/drivers/net/via-velocity.c	2009-04-24 13:09:45.945318255 +0200
@@ -2226,8 +2226,8 @@
 	spin_lock(&vptr->lock);
 	isr_status = mac_read_isr(vptr->mac_regs);
 
- -	/* Not us ? */
- -	if (isr_status == 0) {
+	/* Not us or garbage ? */
+	if (isr_status == 0 || isr_status == 0xffffffff) {
 		spin_unlock(&vptr->lock);
 		return IRQ_NONE;
 	}
@@ -2236,10 +2236,11 @@
 
 	/*
 	 *	Keep processing the ISR until we have completed
- -	 *	processing and the isr_status becomes zero
+	 *	processing and the isr_status becomes zero or
+	 *	we read out some garbage
 	 */
 
- -	while (isr_status != 0) {
+	while (isr_status != 0 && isr_status != 0xffffffff) {
 		mac_write_isr(vptr->mac_regs, isr_status);
 		if (isr_status & (~(ISR_PRXI | ISR_PPRXI | ISR_PTXI | ISR_PPTXI)))
 			velocity_error(vptr, isr_status);

David Miller wrote:
> From: Radek Podgorny <radek@podgorny.cz>
> Date: Wed, 06 May 2009 23:07:02 +0200
> 
>> I've sent the following mail to Francois Romieu <romieu@fr.zoreil.com>
>> but did not get any reply. So I'm sending directly here. It's a trivial
>> patch for via-velocity driver.
> 
> "some garbage"...
> 
> Why would the interrupt status register give us all 1's
> if it is properly initialized?  I should report a zero,
> rather than "some garbage".
> 
> You also failed to provide a proper sign-off as described
> in Documentation/SubmittingPatches
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkoEGE0ACgkQ7mej6pjlbYSKVgCcCIr64sZq9N+yQhdjMgui3HfP
6ZcAnjXkaeVuiCA4vXkwK1+IQNfLO/9Q
=XZap
-----END PGP SIGNATURE-----

Re: via velocity trivial patch

[Francois Romieu]

Radek Podgorny <radek@podgorny.cz> :
[...]
> Anyway, I've verified this is really happening on my hardware when
> multiple via velocity devices share the same interrupt. The one the
> interrupt is for reads the correct ISR value, the others sometimes read
> 0 and sometimes 0xffffffff. I'm not educated enough to tell why.

Your quad port network card includes its own pci bus, right ?
If so can you send a 'lspci -vt' of the host ?

> Reasons to include the fix:
> 1) Someone smarter than me suggested it (Jeff Garzik).
> 2) Fixed my problem.
> 3) Does not introduce a new problem.

3. probably stands: there is no reason for the driver to see a
0xffffffff interupt status register (hardware fault or unplug apart).

Well, almost:

$ grep mac.*isr drivers/net/via-velocity.h
#define mac_read_isr(regs)              readl(&((regs)->ISR))
#define mac_write_isr(regs, x)          writel((x),&((regs)->ISR))
#define mac_clear_isr(regs)             writel(0xffffffffL,&((regs)->ISR))

I can figure that there is a window through which the 2nd, 3rd and 4th
devices can read a 0xffffffff when they are brought up but it should
not be noticeable after this stage (and it may happen on the 1st device
as well if it shares its interrupt with some other device).

Does it match your experience ?

> So I'm attaching the patch again. Now with the proper sign-off.

davem's life is probably simpler if you do not include anything
after the patch.

-- 
Ueimor

Re: via velocity trivial patch

[Radek Podgorny]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Francois,

Francois Romieu wrote:
> Radek Podgorny <radek@podgorny.cz> :
> [...]
>> Anyway, I've verified this is really happening on my hardware when
>> multiple via velocity devices share the same interrupt. The one the
>> interrupt is for reads the correct ISR value, the others sometimes read
>> 0 and sometimes 0xffffffff. I'm not educated enough to tell why.
> 
> Your quad port network card includes its own pci bus, right ?
> If so can you send a 'lspci -vt' of the host ?

I don't know much about the card except for it's this model
http://www.roc-noc.com/product.php?productid=55. The output of lspci follows:

radek@europa ~ % sudo lspci -vt 
- -[0000:00]-+-00.0  Intel Corporation 82815 815 Chipset Host Bridge and Memory Controller Hub
           +-01.0-[0000:01]----00.0  NVidia / SGS Thomson (Joint Venture) Riva128
           +-1e.0-[0000:02-03]--+-00.0-[0000:03]--+-08.0  VIA Technologies, Inc. VT6120/VT6121/VT6122 Gigabit Ethernet Adapter
           |                    |                 +-09.0  VIA Technologies, Inc. VT6120/VT6121/VT6122 Gigabit Ethernet Adapter
           |                    |                 +-0a.0  VIA Technologies, Inc. VT6120/VT6121/VT6122 Gigabit Ethernet Adapter
           |                    |                 \-0b.0  VIA Technologies, Inc. VT6120/VT6121/VT6122 Gigabit Ethernet Adapter
           |                    \-04.0  Intersil Corporation Prism 2.5 Wavelan chipset
           +-1f.0  Intel Corporation 82801BA ISA Bridge (LPC)
           +-1f.1  Intel Corporation 82801BA IDE U100 Controller
           +-1f.2  Intel Corporation 82801BA/BAM USB Controller #1
           +-1f.3  Intel Corporation 82801BA/BAM SMBus Controller
           +-1f.4  Intel Corporation 82801BA/BAM USB Controller #1
           \-1f.5  Intel Corporation 82801BA/BAM AC'97 Audio Controller

>> Reasons to include the fix:
>> 1) Someone smarter than me suggested it (Jeff Garzik).
>> 2) Fixed my problem.
>> 3) Does not introduce a new problem.
> 
> 3. probably stands: there is no reason for the driver to see a
> 0xffffffff interupt status register (hardware fault or unplug apart).
> 
> Well, almost:
> 
> $ grep mac.*isr drivers/net/via-velocity.h
> #define mac_read_isr(regs)              readl(&((regs)->ISR))
> #define mac_write_isr(regs, x)          writel((x),&((regs)->ISR))
> #define mac_clear_isr(regs)             writel(0xffffffffL,&((regs)->ISR))
> 
> I can figure that there is a window through which the 2nd, 3rd and 4th
> devices can read a 0xffffffff when they are brought up but it should
> not be noticeable after this stage (and it may happen on the 1st device
> as well if it shares its interrupt with some other device).
> 
> Does it match your experience ?

Well I haven't done any extensive data gathering (how often the 0xffffffff situation
happens and so on) because I've found a way way to fix (workaround?) it and
unfortunately it's a production machine. But, if you have any suggestions on
what I shall do to help you debug it, please let me know. I can do some tests for
short periods of time during a day.
> 
>> So I'm attaching the patch again. Now with the proper sign-off.
> 
> davem's life is probably simpler if you do not include anything
> after the patch.
> 
Thanks for the info. I don't mean to complicate things.

Radek Podgorny
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkoHQdYACgkQ7mej6pjlbYQOYgCgoe19dnfkh74del3hXODGPEM1
RqYAn05j7ysqpJykjxyGdHsMB3x6FkV7
=bUxK
-----END PGP SIGNATURE-----

Re: via velocity trivial patch

[Radek Podgorny]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

so I've tried to study the source a bit more and don't really get some things.
Why is there a mac_write_isr() in the IRQ handling routine? Isn't ISR supposed
to be read-only? Even if not, why does it write the exact same value as it has
read just moments ago?

    ...
    isr_status = mac_read_isr(vptr->mac_regs);

    /* Not us ? */
    if (isr_status == 0) {
        spin_unlock(&vptr->lock);
        return IRQ_NONE;
    }

    mac_disable_int(vptr->mac_regs);

    /*
     *  Keep processing the ISR until we have completed
     *  processing and the isr_status becomes zero
     */

    while (isr_status != 0) {
        mac_write_isr(vptr->mac_regs, isr_status);
        ...
        isr_status = mac_read_isr(vptr->mac_regs);
    ...

Maybe the value is mangled on write. But even if it was, it should not affect
other devices, just the one in question, right?

The other way for ISR to show 0xffffffff is in device init but I experience
repetitive all-ones. :-( Will have to test it again to be really sure, though.

Do you have any links to datasheets or something so I can try to look up the specs?
Or, do you have any ideas what I should do next to debug (or help you debug)
the issue?

Thanks in advance
Radek Podgorny


Francois Romieu wrote:
> Radek Podgorny <radek@podgorny.cz> :
> [...]
>> Anyway, I've verified this is really happening on my hardware when
>> multiple via velocity devices share the same interrupt. The one the
>> interrupt is for reads the correct ISR value, the others sometimes read
>> 0 and sometimes 0xffffffff. I'm not educated enough to tell why.
> 
> Your quad port network card includes its own pci bus, right ?
> If so can you send a 'lspci -vt' of the host ?
> 
>> Reasons to include the fix:
>> 1) Someone smarter than me suggested it (Jeff Garzik).
>> 2) Fixed my problem.
>> 3) Does not introduce a new problem.
> 
> 3. probably stands: there is no reason for the driver to see a
> 0xffffffff interupt status register (hardware fault or unplug apart).
> 
> Well, almost:
> 
> $ grep mac.*isr drivers/net/via-velocity.h
> #define mac_read_isr(regs)              readl(&((regs)->ISR))
> #define mac_write_isr(regs, x)          writel((x),&((regs)->ISR))
> #define mac_clear_isr(regs)             writel(0xffffffffL,&((regs)->ISR))
> 
> I can figure that there is a window through which the 2nd, 3rd and 4th
> devices can read a 0xffffffff when they are brought up but it should
> not be noticeable after this stage (and it may happen on the 1st device
> as well if it shares its interrupt with some other device).
> 
> Does it match your experience ?
>> So I'm attaching the patch again. Now with the proper sign-off.
> 
> davem's life is probably simpler if you do not include anything
> after the patch.
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkoKxTcACgkQ7mej6pjlbYTLtgCaAvmxl3/FLK4mmrznEiq7+XYD
TzAAnA+EewHvGOdKvwIIMZtisH+AQbsr
=3g6c
-----END PGP SIGNATURE-----

Re: via velocity trivial patch

[Francois Romieu]

Radek Podgorny <radek@podgorny.cz> :
> -----BEGIN PGP SIGNED MESSAGE-----
[...]
> so I've tried to study the source a bit more and don't really get some things.
> Why is there a mac_write_isr() in the IRQ handling routine? Isn't ISR supposed
> to be read-only? Even if not, why does it write the exact same value as it has
> read just moments ago?

It clears the bits.

-- 
Ueimor

Re: via velocity trivial patch

[David Miller]

From: Radek Podgorny <radek@podgorny.cz>
Date: Fri, 08 May 2009 13:32:30 +0200

> The "some garbage" are just my words for what Jeff Garzik called "also
> bail if 0xffffffff (hardware fault / hardware unplugged)" in
> http://lkml.indiana.edu/hypermail/linux/kernel/0406.0/0500.html in 2004.
> Should I change that?
> 
> Anyway, I've verified this is really happening on my hardware when
> multiple via velocity devices share the same interrupt. The one the
> interrupt is for reads the correct ISR value, the others sometimes read
> 0 and sometimes 0xffffffff. I'm not educated enough to tell why.
> 
> Reasons to include the fix:
> 1) Someone smarter than me suggested it (Jeff Garzik).
> 2) Fixed my problem.
> 3) Does not introduce a new problem.
> 
> So I'm attaching the patch again. Now with the proper sign-off.

I don't think we should include this until we know why your
card is showing this value in the status register.

Since I assume you aren't unplugging the device showing this
behavior, the only possible remaining reason is "hardware fault"
and we need to understand that better before we start working
around it.

I consider it a good bet that Jeff has no idea why the chip is
doing this either. :-)

Re: via velocity trivial patch

[David Miller]

From: Radek Podgorny <radek@podgorny.cz>
Date: Fri, 08 May 2009 13:32:30 +0200

> @@ -2226,8 +2226,8 @@
>  	spin_lock(&vptr->lock);
>  	isr_status = mac_read_isr(vptr->mac_regs);
>  
> - -	/* Not us ? */
> - -	if (isr_status == 0) {
> +	/* Not us or garbage ? */
> +	if (isr_status == 0 || isr_status == 0xffffffff) {

Your patch is also completely mangled and would not apply
anyways.