(no subject)

[Ivan G.]

Urban,

About the suggestion to make via_rhine_error handle more interrupts,

enum intr_status_bits {
        IntrRxDone=0x0001, IntrRxErr=0x0004, IntrRxEmpty=0x0020,
        IntrTxDone=0x0002, IntrTxAbort=0x0008, IntrTxUnderrun=0x0010,
        IntrPCIErr=0x0040,
        IntrStatsMax=0x0080, IntrRxEarly=0x0100, IntrMIIChange=0x0200,
        IntrRxOverflow=0x0400, IntrRxDropped=0x0800, IntrRxNoBuf=0x1000,
        IntrTxAborted=0x2000, IntrLinkChange=0x4000,
        IntrRxWakeUp=0x8000,
        IntrNormalSummary=0x0003, IntrAbnormalSummary=0xC260,
};

RxEarly, RxOverflow, RxNoBuf are not handled
(which brings up another question - how should they be handled 
and where?? It doesn't seem to me that those should end up in error,
sending CmdTxDemand. )

RxErr, RxWakeUp, RxDropped, RxEmpty call via_rhine_rx
TxAbort, TxUnderrun,PCIErr, StatsMax, MIIChange call via_rhine_error
TxAborted calls via_rgine_tx
The others don't look like errors.


Martin Eriksson,
The reason my message said PCI Error and not unhandled
is because it specifies a specific interrupt - IntrPCIErr.
(basically the onle one that's left unhandled that can call via_rhine_error)

Re: your mail

[Jeff Garzik]

On Sun, Apr 21, 2002 at 03:16:40PM -0600, Ivan G. wrote:
> Urban,
> 
> About the suggestion to make via_rhine_error handle more interrupts,
> 
> enum intr_status_bits {
>         IntrRxDone=0x0001, IntrRxErr=0x0004, IntrRxEmpty=0x0020,
>         IntrTxDone=0x0002, IntrTxAbort=0x0008, IntrTxUnderrun=0x0010,
>         IntrPCIErr=0x0040,
>         IntrStatsMax=0x0080, IntrRxEarly=0x0100, IntrMIIChange=0x0200,
>         IntrRxOverflow=0x0400, IntrRxDropped=0x0800, IntrRxNoBuf=0x1000,
>         IntrTxAborted=0x2000, IntrLinkChange=0x4000,
>         IntrRxWakeUp=0x8000,
>         IntrNormalSummary=0x0003, IntrAbnormalSummary=0xC260,
> };
> 
> RxEarly, RxOverflow, RxNoBuf are not handled
> (which brings up another question - how should they be handled 
> and where?? It doesn't seem to me that those should end up in error,
> sending CmdTxDemand. )

*blink*  I had not noticed that.

All drivers actually need to handle RxNoBufs and RxOverflow, assuming
they have similar meaning to what I'm familiar with on other chips.
The chip may recover transparently, but one should be at least aware of
them.

RxEarly you very likely do -not- want to handle...

	Jeff

Re: [PATCH] Via-rhine minor issues

[Ivan G.]

Sorry, I forgot the subject line for last mail
Message has no subject. Garzik's reply is Re: Your Mail.

As for the Interrupts:
Actually, RxNoBuf is handled by calling via_rhine_rx 
but not enabled when setting interrupt mask. My patch will fix that.

However, RxOverflow is never handled at all and neither is RxEarly.
Nor are they enabled when setting interrupt mask (patch enables).

How should RxOverflow be handled?
	Should I call via_rhine_rx, like other errors do? - add IntrRxOverflow (and 
possibly RxEarly)

       if (intr_status & (IntrRxDone | IntrRxErr | IntrRxDropped |
                IntrRxWakeUp | IntrRxEmpty | IntrRxNoBuf))
			via_rhine_rx(dev);


How should PCIErr be handled?
Other drivers say:

        /* Hmmmmm, it's not clear how to recover from PCI faults. */
	

> > RxEarly, RxOverflow, RxNoBuf are not handled
> > (which brings up another question - how should they be handled
> > and where?? It doesn't seem to me that those should end up in error,
> > sending CmdTxDemand. )
>
> *blink*  I had not noticed that.
>
> All drivers actually need to handle RxNoBufs and RxOverflow, assuming
> they have similar meaning to what I'm familiar with on other chips.
> The chip may recover transparently, but one should be at least aware of
> them.
>
> RxEarly you very likely do -not- want to handle...
>
> 	Jeff

Re: [PATCH] Via-rhine minor issues

[Urban Widmark]

On Sun, 21 Apr 2002, Ivan G. wrote:

> As for the Interrupts:
> Actually, RxNoBuf is handled by calling via_rhine_rx 
> but not enabled when setting interrupt mask. My patch will fix that.
> 
> However, RxOverflow is never handled at all and neither is RxEarly.
> Nor are they enabled when setting interrupt mask (patch enables).
> 
> How should RxOverflow be handled?

The public docs don't say.

You should probably add them to the list of reasons to call
via_rhine_error, and let them be caught by the "wicked" error rule. A
reason to use a term that doesn't try to be specific is that we don't
really know what is causing the event.


Before your patch IntrTxUnderrun would raise the tx threshold AND issue a
CmdTxDemand. After it will only do the first. Do you know that this is an
improvement?

Does merging this error handling change into the main kernel really help
you test other things for your timeout problem? Can't you just include
this bit in your other work?

/Urban

Re: [PATCH] Via-rhine minor issues

[Ivan G.]

> The public docs don't say.
>

That is precisely the problem.
4 missing interrupts in the interrupt mask, 1 additional interrupt 
which Jeff Garzik says all drivers should handle and the via-rhine doesn't.
2 interrupts handled quite differently in the linuxfet driver.
1 wicked error message of which I've seen 3 different versions and none of 
them makes much sense to me. 

It is lack of documentation.
The tech sheets do not explain what causes the interrupts in detail, how to 
handle the interrupts in detail. They rather provide the bit address of the 
interrupt which is good to know, but not enough. What resources did Donald 
Becker use when he was writing this?

Perhaps I should ask him.
Or do you know a good document on such things?
 

> You should probably add them to the list of reasons to call
> via_rhine_error, and let them be caught by the "wicked" error rule. A
> reason to use a term that doesn't try to be specific is that we don't
> really know what is causing the event.

Well, most Rx errors are handled in via_rhine_rx.
The "Wicked" error rule doesn't do anything besides a CmdTxDemand.
Is this correct handling for, let's say RxOverflow?  ..Again, facing the
overwhelming lack of documentation :) (see above paragraph)

Yes, we do know what's causing the event.
We know exactly which interrupt.
We just don't know how to handle it.

> Does merging this error handling change into the main kernel really help
> you test other things for your timeout problem? Can't you just include
> this bit in your other work?
>
> /Urban

Ok not really.
Apparently this change is controversial and I'll leave it out of my patch 
until decided what to do. I would just like the kernel driver to be less 
buggy and I wanted to contribute.

Plus, I've been making too many changes to my copy. It would be easier to 
work over a clean kernel driver.  I would have less things to keep track of 
that have been changed.


---------------------------
While we're talking about bugs, here's a (possibly ignorant) question:
Do Rhine-III's have a place in the following? : 

#ifdef USE_MEM
static void __devinit enable_mmio(long ioaddr, int chip_id)
{
        int n;
        if (chip_id == VT86C100A) {
                /* More recent docs say that this bit is reserved ... */
                n = inb(ioaddr + ConfigA) | 0x20;
                outb(n, ioaddr + ConfigA);
        } else if (chip_id == VT6102) {
                n = inb(ioaddr + ConfigD) | 0x80;
                outb(n, ioaddr + ConfigD);
        }
}
#endif


-----------------------------

Well, apart from things which are not precisely clear how to fix, 
would you like to me to submit any portions of my patch for inclusion at all?
A little warning: as far as testing goes - my card stalls without the fixes,
and it stalls with the fixes :) Yet, they seem simple enough to offer for 
inclusion. 

Re: [PATCH] Via-rhine minor issues

[Urban Widmark]

On Mon, 22 Apr 2002, Ivan G. wrote:

> The tech sheets do not explain what causes the interrupts in detail, how to 
> handle the interrupts in detail. They rather provide the bit address of the 
> interrupt which is good to know, but not enough. What resources did Donald 
> Becker use when he was writing this?
> 
> Perhaps I should ask him.

He's the one who wrote:
"Recovery for other fault sources not known."

I have a feeling he didn't have any other sources, except that he had
written a whole bunch of other (similar) drivers before.

Some things you can try to guess:
RxEarly - An interrupt is sent when the chip begins to receive data. if
          the driver wants to do something when that happens then it
          should enable it. The current driver doesn't so it shouldn't be
          enabled.
Or something completely different.


> Well, most Rx errors are handled in via_rhine_rx.
> The "Wicked" error rule doesn't do anything besides a CmdTxDemand.
> Is this correct handling for, let's say RxOverflow?  ..Again, facing the
> overwhelming lack of documentation :) (see above paragraph)

One option is not to do anything.

Another to catch the event and call via_rhine_error on it. That will make
it print the "wicked" message and do a CmdTxDemand. Does it hurt to make a
CmdTxDemand when you don't really need it?

If you have a reason to think it hurts, then separate the events that you
do CmdTxDemand on:
	if (intr_status & ~IntrRxOverflow)
		writew(CmdTxDemand ...);
(which will still do a CmdTxDemand in all cases it used to do one, but
 not if the interrupt was just a IntrRxOverflow. I think.)

And the same for the other interrupt bits that weren't enabled.


> Plus, I've been making too many changes to my copy. It would be easier to 
> work over a clean kernel driver.  I would have less things to keep track of 
> that have been changed.

Nothing prevents you from having more than one copy of the driver in your
local tree(s). Since the driver is just one file that works ok, make a
copy of it when you have a version you "like". Then you can look back
later and compare.


> While we're talking about bugs, here's a (possibly ignorant) question:
> Do Rhine-III's have a place in the following? : 

Probably. You need to check if their docs also enable MMIO by flipping
that bit, then you could change the "chip_id == VT6102" part to be just an
else.


> Well, apart from things which are not precisely clear how to fix, 
> would you like to me to submit any portions of my patch for inclusion at all?

The wait_for_reset thing is clearly a bug, and the cur_tx-1 is probably
right too because you want the number it was queued as. So yes, send them
to Jeff.

/Urban

Re: [PATCH] Via-rhine minor issues

[Jeff Garzik]

On Sun, Apr 21, 2002 at 12:19:40PM +0200, Urban Widmark wrote:
> On Sun, 21 Apr 2002, Ivan G. wrote:
> 
> > DIFF-ED AGAINST:
> > 2.4.19-pre3 ( I don't think there were changes to via-rhine 
> > b-ween pre3 and pre7)
> 
> You should probably send this to Jeff Garzik instead of Marcelo. Jeff
> collects net driver patches.
> 
> If you don't, Shing Chuang's changes are in -pre7-ac2, so a re-diff vs
> that and seding it to Alan Cox may cause less merge work.

Does Ivan's patch look ok to you, Urban?  I'd like to get your initial
sign-off on it.  From a quick look, it looks ok to me.

And yes, you are correct:  my preference would be to receive two
submissions in my inbox:  Shing Chuang's patch, and then Ivan's patch
(less Shing's work).

If there are multiple issues, multiple patches are encouraged.  More
split-up is always better than less.  :)

Regards,

	Jeff

Re: [PATCH] Via-rhine minor issues

[Urban Widmark]

On Sun, 21 Apr 2002, Ivan G. wrote:

> DIFF-ED AGAINST:
> 2.4.19-pre3 ( I don't think there were changes to via-rhine 
> b-ween pre3 and pre7)

You should probably send this to Jeff Garzik instead of Marcelo. Jeff
collects net driver patches.

If you don't, Shing Chuang's changes are in -pre7-ac2, so a re-diff vs
that and seding it to Alan Cox may cause less merge work.


> - changed chip_id in wait_for_reset as parameter since np is not initialized
> the first time this function is called (should this be fixed differently?)

I think that is an ok change. np could be initialised sooner instead but
I don't know why that would be better.

> - change "Something Wicked" message to "PCI Error" (I still don't see the 
> purpose of the trap)

To trap things ... :)

Maybe there should be a trap for all the unhandled interrupt status error
events. via_rhine_interrupt could list more Intr flags as reasons to call
via_rhine_error, if you know which ones are errors.

/Urban

Re: [PATCH] Via-rhine minor issues

[Martin Eriksson]

----- Original Message -----
From: "Ivan G." <ivangurdiev@yahoo.com>
To: "Urban Widmark" <urban@teststation.com>; "Shing Chuang"
<ShingChuang@via.com.tw>
Cc: "LKML" <linux-kernel@vger.kernel.org>
Sent: Sunday, April 21, 2002 9:02 AM
Subject: [PATCH] Via-rhine minor issues


> Please comment on this before I send to Marcello.
> I am a newbie and would like some help.
> Patch fixes minor issues in the driver.
>
> Note: Shing Chuang, this includes your own patch for Rhine-III support
> since it's not yet in the kernel and I diff-ed against the original
source,
> while I added your patch to my driver version.
>
> BTW your own patch wouldn't patch cleanly for some reason.
>
> DIFF-ED AGAINST:
> 2.4.19-pre3 ( I don't think there were changes to via-rhine
> b-ween pre3 and pre7)
>
> CONTENTS:
> - Shing Chuang's Rhine III support and 6100 cleanup patch.
> - edited comments on supported cards
> - removed unused W_MAX_TIMEOUT
> - added flag HasDavicomPhy for VT86C100A card
> - changed chip_id in wait_for_reset as parameter since np is not
initialized
> the first time this function is called (should this be fixed differently?)
> - enable all interrupts (add 4 additional ones)
> - fix debug message - frame is off by one
> - change "Something Wicked" message to "PCI Error" (I still don't see the
> purpose of the trap)

I don't think "PCI Error" is such a great message.. sounds too serious.

What about "Unhandled <something>"?

 _____________________________________________________
|  Martin Eriksson <nitrax@giron.wox.org>
|  MSc CSE student, department of Computing Science
|  Umeå University, Sweden

[PATCH] Via-rhine minor issues

[Ivan G.]

Please comment on this before I send to Marcello.
I am a newbie and would like some help.
Patch fixes minor issues in the driver.

Note: Shing Chuang, this includes your own patch for Rhine-III support 
since it's not yet in the kernel and I diff-ed against the original source, 
while I added your patch to my driver version.

BTW your own patch wouldn't patch cleanly for some reason.

DIFF-ED AGAINST:
2.4.19-pre3 ( I don't think there were changes to via-rhine 
b-ween pre3 and pre7)

CONTENTS:
- Shing Chuang's Rhine III support and 6100 cleanup patch.
- edited comments on supported cards
- removed unused W_MAX_TIMEOUT
- added flag HasDavicomPhy for VT86C100A card
- changed chip_id in wait_for_reset as parameter since np is not initialized
the first time this function is called (should this be fixed differently?)
- enable all interrupts (add 4 additional ones)
- fix debug message - frame is off by one
- change "Something Wicked" message to "PCI Error" (I still don't see the 
purpose of the trap)

NOTE:
Urban, this should clear up the annoying minor issues so I can do more tests
on my own delay problem (Via-Rhine stalls on transmit thread)

PATCH:  (watch out for kmail newlines on several places)

--- via-rhine.c.origkernel      Sat Mar 30 01:10:02 2002
+++ via-rhine.c Sun Apr 21 00:30:34 2002
@@ -9,8 +9,8 @@
        a complete program and may only be used when the entire operating
        system is licensed under the GPL.

-       This driver is designed for the VIA VT86c100A Rhine-II PCI Fast 
Ethernet
-       controller.  It also works with the older 3043 Rhine-I chip.
+       This driver is designed for the VIA VT86C100A Rhine-I.
+       It also works with the 6102 Rhine-II, and 6105/6105M Rhine-III.

        The author may be reached as becker@scyld.com, or C/O
        Scyld Computing Corporation
@@ -136,9 +136,6 @@

 #define PKT_BUF_SZ             1536                    /* Size of each 
temporary Rx buffer.*/

-/* max time out delay time */
-#define W_MAX_TIMEOUT  0x0FFFU
-
 #if !defined(__OPTIMIZE__)  ||  !defined(__KERNEL__)
 #warning  You must compile this file with the correct options!
 #warning  See the last lines of the source file.
@@ -317,7 +314,8 @@
 enum via_rhine_chips {
        VT86C100A = 0,
        VT6102,
-       VT3043,
+       VT6105,
+       VT6105M
 };

 struct via_rhine_chip_info {
@@ -342,18 +340,21 @@
 static struct via_rhine_chip_info via_rhine_chip_info[] __devinitdata =
 {
        { "VIA VT86C100A Rhine", RHINE_IOTYPE, 128,
-         CanHaveMII | ReqTxAlign },
+         CanHaveMII | ReqTxAlign | HasDavicomPhy },
        { "VIA VT6102 Rhine-II", RHINE_IOTYPE, 256,
          CanHaveMII | HasWOL },
-       { "VIA VT3043 Rhine",    RHINE_IOTYPE, 128,
-         CanHaveMII | ReqTxAlign }
+       { "VIA VT6105 Rhine-III", RHINE_IOTYPE, 256,
+         CanHaveMII | HasWOL },
+       { "VIA VT6105M Rhine-III", RHINE_IOTYPE, 256,
+         CanHaveMII | HasWOL },
 };

 static struct pci_device_id via_rhine_pci_tbl[] __devinitdata =
 {
-       {0x1106, 0x6100, PCI_ANY_ID, PCI_ANY_ID, 0, 0, VT86C100A},
+       {0x1106, 0x3043, PCI_ANY_ID, PCI_ANY_ID, 0, 0, VT86C100A},
        {0x1106, 0x3065, PCI_ANY_ID, PCI_ANY_ID, 0, 0, VT6102},
-       {0x1106, 0x3043, PCI_ANY_ID, PCI_ANY_ID, 0, 0, VT3043},
+       {0x1106, 0x3106, PCI_ANY_ID, PCI_ANY_ID, 0, 0, VT6105},
+       {0x1106, 0x3053, PCI_ANY_ID, PCI_ANY_ID, 0, 0, VT6105M},
        {0,}                    /* terminate list */
 };
 MODULE_DEVICE_TABLE(pci, via_rhine_pci_tbl);
@@ -502,15 +503,13 @@
 static int  via_rhine_close(struct net_device *dev);
 static inline void clear_tally_counters(long ioaddr);

-static void wait_for_reset(struct net_device *dev, char *name)
+static void wait_for_reset(struct net_device *dev, int chip_id, char *name)
 {
-       struct netdev_private *np = dev->priv;
        long ioaddr = dev->base_addr;
-       int chip_id = np->chip_id;
        int i;

-       /* 3043 may need long delay after reset (dlink) */
-       if (chip_id == VT3043 || chip_id == VT86C100A)
+       /* VT86C100A may need long delay after reset (dlink) */
+       if (chip_id == VT86C100A)
                udelay(100);

        i = 0;
@@ -531,7 +530,7 @@
 static void __devinit enable_mmio(long ioaddr, int chip_id)
 {
        int n;
-       if (chip_id == VT3043 || chip_id == VT86C100A) {
+       if (chip_id == VT86C100A) {
                /* More recent docs say that this bit is reserved ... */
                n = inb(ioaddr + ConfigA) | 0x20;
                outb(n, ioaddr + ConfigA);
@@ -659,7 +658,7 @@
        writew(CmdReset, ioaddr + ChipCmd);

        dev->base_addr = ioaddr;
-       wait_for_reset(dev, shortname);
+       wait_for_reset(dev, chip_id, shortname);

        /* Reload the station address from the EEPROM. */
 #ifdef USE_IO
@@ -980,7 +979,8 @@

        /* Enable interrupts by setting the interrupt mask. */
        writew(IntrRxDone | IntrRxErr | IntrRxEmpty| IntrRxOverflow| 
IntrRxDropped|
-                  IntrTxDone | IntrTxAbort | IntrTxUnderrun |
+                  IntrRxEarly | IntrRxNoBuf | IntrRxWakeUp | IntrTxAborted|
+                  IntrTxDone | IntrTxAbort | IntrTxUnderrun |
                   IntrPCIErr | IntrStatsMax | IntrLinkChange | IntrMIIChange,
                   ioaddr + IntrEnable);

@@ -1070,7 +1070,7 @@
                return i;
        alloc_rbufs(dev);
        alloc_tbufs(dev);
-       wait_for_reset(dev, dev->name);
+       wait_for_reset(dev, np->chip_id, dev->name);
        init_registers(dev);
        if (debug > 2)
                printk(KERN_DEBUG "%s: Done via_rhine_open(), status %4.4x "
@@ -1177,7 +1177,7 @@
        alloc_rbufs(dev);

        /* Reinitialize the hardware. */
-       wait_for_reset(dev, dev->name);
+       wait_for_reset(dev, np->chip_id, dev->name);
        init_registers(dev);

        spin_unlock(&np->lock);
@@ -1247,7 +1247,7 @@

        if (debug > 4) {
                printk(KERN_DEBUG "%s: Transmit frame #%d queued in slot 
%d.\n",
-                          dev->name, np->cur_tx, entry);
+                          dev->name, np->cur_tx-1, entry);
        }
        return 0;
 }
@@ -1498,12 +1498,11 @@
                        printk(KERN_INFO "%s: Transmitter underrun, 
increasing Tx "
                                   "threshold setting to %2.2x.\n", 
dev->name, np->tx_thresh);
        }
-       if ((intr_status & ~( IntrLinkChange | IntrStatsMax |
-                                                 IntrTxAbort | 
IntrTxAborted))) {
+       if (intr_status & IntrPCIErr) {
                if (debug > 1)
-                       printk(KERN_ERR "%s: Something Wicked happened! 
%4.4x.\n",
+                       printk(KERN_ERR "%s: PCI Error! %4.4x.\n",
                           dev->name, intr_status);
-               /* Recovery for other fault sources not known. */
+               /* Recovery for PCI Error. */
                writew(CmdTxDemand | np->chip_cmd, dev->base_addr + ChipCmd);
        }