[PATCH 2/2] net: at91_ether.c - Allow transmitter interrupt to be handled first in ISR

[PATCH 2/2] net: at91_ether.c - Allow transmitter interrupt to be handled first in ISR

[James Kosin]

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

Ok,

This next patch is optional....

The idea is TUND should only happen on occasions when the PHY is unable
to receive the transmitter data in a timely fashion to successfully send
the data in a single burst.  This is not a hard error; so, why do we
treat it as such.

This patch allows the transmitter to resend the failed skb inside the
ISR without having to deal with the overhead of freeing the skb then
having the main task re-allocate a new skb for the failed packet.

James Kosin


[-- Attachment #2: ether_patch_2.patch --]
[-- Type: application/octet-stream, Size: 1348 bytes --]

--- C:/Documents and Settings/jkosin/My Documents/junk/kernel/Copy of linux-2.6.31.5/drivers/net/arm/at91_ether.c	Wed Jan 13 12:26:13 2010
+++ C:/Documents and Settings/jkosin/My Documents/junk/kernel/linux-2.6.31.5/drivers/net/arm/at91_ether.c	Wed Jan 13 12:38:55 2010
@@ -927,15 +927,24 @@
 
 	if (intstatus & AT91_EMAC_TCOM) {	/* Transmit complete */
 		/* The TCOM bit is set even if the transmission failed. */
-		if (intstatus & (AT91_EMAC_TUND | AT91_EMAC_RTRY))
-			dev->stats.tx_errors += 1;
+		if (intstatus & AT91_EMAC_TUND) {
+			if (lp->skb) {
+				/* Set address of the data in the Transmit Address register */
+				at91_emac_write(AT91_EMAC_TAR, lp->skb_physaddr);
+				/* Set length of the packet in the Transmit Control register */
+				at91_emac_write(AT91_EMAC_TCR, lp->skb->len);
+			}
+		} else {
+			if (intstatus & AT91_EMAC_RTRY)
+				dev->stats.tx_errors += 1;
 
-		if (lp->skb) {
-			dev_kfree_skb_irq(lp->skb);
-			lp->skb = NULL;
-			dma_unmap_single(NULL, lp->skb_physaddr, lp->skb_length, DMA_TO_DEVICE);
+			if (lp->skb) {
+				dev_kfree_skb_irq(lp->skb);
+				lp->skb = NULL;
+				dma_unmap_single(NULL, lp->skb_physaddr, lp->skb_length, DMA_TO_DEVICE);
+			}
+			netif_wake_queue(dev);
 		}
-		netif_wake_queue(dev);
 	}
 
 	if (intstatus & AT91_EMAC_RCOM)		/* Receive complete */

Re: [PATCH 2/2] net: at91_ether.c - Allow transmitter interrupt to be handled first in ISR

[Eric Dumazet]

Le 13/01/2010 18:46, James Kosin a écrit :
> Ok,
> 
> This next patch is optional....
> 
> The idea is TUND should only happen on occasions when the PHY is unable
> to receive the transmitter data in a timely fashion to successfully send
> the data in a single burst.  This is not a hard error; so, why do we
> treat it as such.
> 
> This patch allows the transmitter to resend the failed skb inside the
> ISR without having to deal with the overhead of freeing the skb then
> having the main task re-allocate a new skb for the failed packet.

Are we sure chip doesnt report TUND forever in some situations ?
Should'nt we have a retry limit for each skb ?

> 
> James Kosin
> 
--- C:/Documents and Settings/jkosin/My Documents/junk/kernel/Copy of linux-2.6.31.5/drivers/net/arm/at91_ether.c	Wed Jan 13 12:26:13 2010
+++ C:/Documents and Settings/jkosin/My Documents/junk/kernel/linux-2.6.31.5/drivers/net/arm/at91_ether.c	Wed Jan 13 12:38:55 2010

Oh well :)

Please read Documentation/SubmittingPatches


1) Should be in "diff -u" form
9) Your patch should be based on latest kernel (preferrably on David net-next-2.6 git tree)

12) Should be Signed-off-by


You said in a previous mail the chip was capable of queueing two frames,
it would be nice to exploit this in driver, since at91 has only one frame
in transmit queue.

RE: [PATCH 2/2] net: at91_ether.c - Allow transmitter interrupt to be handled first in ISR

[James Kosin]

-----Original Message-----
>From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On >Behalf Of Eric Dumazet
>Sent: Wednesday, January 13, 2010 1:09 PM
>To: James Kosin
>Cc: linux-kernel@vger.kernel.org; Linux Netdev List
>Subject: Re: [PATCH 2/2] net: at91_ether.c - Allow transmitter interrupt to be handled >first in ISR
>
>Le 13/01/2010 18:46, James Kosin a écrit :
>> Ok,
>> 
>> This next patch is optional....
>> 
>
>Are we sure chip doesnt report TUND forever in some situations ?
>Should'nt we have a retry limit for each skb ?
>

Directly from the documentation for the AT91RM9200 MAC registers:

For the transmitter status register:
* UND: Transmit Underrun
Set when transmit DMA was not able to read data from memory in time. If this happens, the transmitter forces bad CRC.
Cleared by writing a one to this bit.

For the interrupt status register:
* TUND: Transmit Buffer Underrun
Ethernet transmit buffer underrun. The transmit DMA did not complete fetch frame data in time for it to be transmitted.
Cleared on read.

In section 36.5.4.1

The transmit address register must be written before the transmit control register. While a frame is being transmitted, it is possible to set up one other frame for transmission by writing new values to the transmit address and control registers. Reading the transmit address register returns the address of the buffer currently being accessed by the transmit FIFO.

This is where we may be able to exploit multiple transmit frames.
I'm not sure how to handle this in the confines of the kernel for this ARM part.  Currently, the architecture strictly limits the number of frames to (1) one only!  I'm not sure why; since there is no comment on this in the source for the driver.  My only guess, is that the authors where worried that if an error occurred on a frame they would not be able to determine which frame was effected by the error.

---


>From the AT91SAM9G20 MAC registers:

* UND: Transmit Underrun
Set when transmit DMA was not able to read data from memory, either because the bus was not granted in time, because
a not OK hresp(bus error) was returned or because a used bit was read midway through frame transmission. If this
occurs, the transmitter forces bad CRC. Cleared by writing a one to this bit.

* TUND: Ethernet Transmit Buffer Underrun
The transmit DMA did not fetch frame data in time for it to be transmitted or hresp returned not OK. Also set if a used bit
is read mid-frame or when a new transmit queue pointer is written. Cleared on read.


The 91SAM9G20 MAC supports transmitter DMA queues; and I don't believe this driver should be used with that part; but, the descriptions on the events causing the error are consistent.
The error should only happen if the MAC is unable to transfer the frame from shared memory due to some situations where the processor or other device is accessing the same memory area.  I'm also guessing if the shared memory area is in SDRAM instead of internal SRAM this could also be caused by mandated SDRAM refresh cycles.

====

Sorry about the patch submitting... it is really my first time and I'm in the process of installing Cygwin to be able to submit a more formal patch and to be able to verify the performance and particulars.  I'll get the git tree ASAP; so, I can diff and compile against that.  Thanks for the tips on this.

I can attach the source file and copy you to look at, if you wish?

Thanks,
James