netdev - Re: [PATCH] MACB: clear transmit buffers properly on TX Underrun

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20071218155232.57027995@dhcp-252-066.norway.atmel.com>
Date:	Tue, 18 Dec 2007 15:52:32 +0100
From:	Haavard Skinnemoen <hskinnemoen@...el.com>
To:	Gregory CLEMENT <gclement00@...il.com>
Cc:	netdev@...r.kernel.org, linux-arm-kernel@...ts.arm.linux.org.uk
Subject: Re: [PATCH] MACB: clear transmit buffers properly on TX Underrun

On Sun, 16 Dec 2007 12:37:25 +0100
Gregory CLEMENT <gclement00@...il.com> wrote:

> Haavard Skinnemoen a écrit :
> > But the patch is a bit mangled, so I don't think it will apply. Please
> > have a look in Documentation/email-clients.txt for information on how
> > to set up Thunderbird to avoid this.
> 
> I read it and apply it. Hope it will be OK as I didn't see any problem in m first mail...

Looks good, thanks.

> > 
> > I also think this should be done as part of the previous loop, before
> > they are freed. Having free buffers in the ring sounds dangerous, even
> > if it's only for a short time.
> 
> OK, I moved this loop before the previous loop, as I wanted to be sure that
> all buffers are marked.
> I did it, but I think we don't really need to do it, as when underrun happen, 
> controller stop transmitting, so it won't transmit a free buffer.

Yes, you're probably right. But it looks better now, I think.

> > 
> >> +
> >>          bp->tx_head = bp->tx_tail = 0;
> > 
> > Now, I wonder if it would be better to just scan the ring descriptors,
> > find the one that failed and just move the DMA pointer to the next
> > entry. The hardware resets the DMA pointer when an underrun happens, so
> > I think your code will work, but we're losing more packets than
> > strictly necessary. In any case, it's better than the existing code.
> 
> As hardware reset the DMA pointer, we have to rewrite all the buffer descriptor.
> 
> For example if descriptor n°78 failed, and there is descriptor 79 and 80 which
> are pending. When underrun happen on 78 the hardware reset DMA pointer which
> will point on descriptor 0. So we have to copy descriptor 79 on 0 and 80 on 1.
> The other option is to change Transmit Buffer Queue Pointer Register (TBQP), to
> make it point on descriptor 79, but on the next descriptor having wrapped bit set,
> the DMA pointer will wrapped on descriptor 79 and not on 0. So I don't think it
> is a good solution.

The second option sounds feasible to me. We know how large the ring is,
so taking care of the wrapping isn't very expensive.

> > Perhaps we need a check in macb_start_xmit() as well to avoid starting
> > a transmission when the ring has just been reset.
> > 
> 
> I joined the patch change along your recommendation, if there is no more error, you
> can maybe submit it for 2.6.24 release, and other improvement can be done later.

Ok, if it works for you, I'll send it upstream; your patch certainly
improves things. I think there's still an unresolved race with
macb_start_xmit()...but closing it involves more complex modifications
to the main tx patch, so it's probably safest to leave it for 2.6.25.

> From: Gregory CLEMENT <gclement00@...il.com>
> Date: Sun, 16 Dec 2007 11:45:03 +0100
> Subject: [PATCH]  MACB: clear transmit buffers properly on transmit underrun
> 
> Initially transmit buffer pointers were only reset. But buffer descriptors
> were possibly still set as ready, and buffer in upper layer was not
> freed. This caused driver hang under big load.
> Now reset clean properly the buffer descriptor and freed upper layer.
> 
> Signed-off-by: Gregory CLEMENT <gclement00@...il.com>
> ---
>  drivers/net/macb.c |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index 047ea7b..e898713 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -307,9 +307,34 @@ static void macb_tx(struct macb *bp)
>  		(unsigned long)status);
>  
>  	if (status & MACB_BIT(UND)) {
> +		int i;
>  		printk(KERN_ERR "%s: TX underrun, resetting buffers\n",
> -		       bp->dev->name);
> +			   bp->dev->name);

I'll just kill these extra spaces...


>  		bp->tx_head = bp->tx_tail = 0;
> +

...and this extra line.

Haavard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html