From: "Michael Büsch" <m@bues•ch>
To: Neil Horman <nhorman@tuxdriver•com>
Cc: Eric Dumazet <eric.dumazet@gmail•com>,
Alexey Zaytsev <alexey.zaytsev@gmail•com>,
Andrew Morton <akpm@linux-foundation•org>,
netdev@vger•kernel.org, Gary Zambrano <zambrano@broadcom•com>,
bugme-daemon@bugzilla•kernel.org,
"David S. Miller" <davem@davemloft•net>,
Pekka Pietikainen <pp@ee•oulu.fi>,
Florian Schirmer <jolt@tuxbox•org>,
Felix Fietkau <nbd@openwrt•org>, Michael Buesch <mb@bu3sch•de>
Subject: Re: [Bugme-new] [Bug 38102] New: BUG kmalloc-2048: Poison overwritten
Date: Wed, 6 Jul 2011 17:32:43 +0200 [thread overview]
Message-ID: <20110706173243.404d8599@maggie> (raw)
In-Reply-To: <20110705220644.GB12118@hmsreliant.think-freely.org>
On Tue, 5 Jul 2011 18:06:44 -0400
Neil Horman <nhorman@tuxdriver•com> wrote:
> On Tue, Jul 05, 2011 at 10:15:40PM +0200, Eric Dumazet wrote:
> > Le mardi 05 juillet 2011 à 22:02 +0200, Eric Dumazet a écrit :
> > > Le mardi 05 juillet 2011 à 15:53 -0400, Neil Horman a écrit :
> > > > I think this is a goo idea, at least for testing. It seems odd to me that we
> > > > have the B44_DMARX_PTR value which indicates (ostensibly) the pointer to the
> > > > descriptor to be processed next (the documentation isnt' very verbose on the
> > > > subject), along with the EOT bit on a descriptor. It seems like both the
> > > > register and the bit are capable of conveying the same (or at least overlapping)
> > > > information.
> > > >
> > > > I think what I'm having the most trouble with is understanding when the hw looks
> > > > at the EOT bit in the descriptor. If it completes a DMA and sees the EOT bit
> > > > set, does the next DMA occur to the descriptor pointed to by the DMARX_ADDR
> > > > register? Of does it stall until such time as the DMARX_PTR register is rotated
> > > > around? What if it doesn't see the EOT bit set? Does it just keep going with
> > > > the next descriptor?
> >
> > Since there is no OWN bit (at least not on the online doc I got : it
> > says the rx_ring is read only by the NIC), I would say we really need to
> > advance DMARX_PTR to signal NIC a new entry is available for following
> > incoming frames.
> >
> > This is the reason rx_pending max value is B44_RX_RING_SIZE - 1, or else
> > chip could loop on a circular rx_ring.
> >
> Agree, although that still leaves open the question of what exactly should get
> written into the DMARX_PTR. Is it an index of the descriptor number, or a byte
> offset.
>
> Regardless, I think we ned to fix up the looping so as to prevent an EOT reset
> jumping outside of our valid ring window. Alexey, theres better ways to do
> this, but if in the interim, you could please try this patch, it makes the valid
> receive window for b44 cover the entire ring, so as to avoid this problem. It
> will at least help support or refute this theory. Note its not exactly the same
> as my previous patch. If you set the default ring pending to 512, the math in
> the b44_alloc_rx_skb path is wrong, we skip the EOT bit as well as the first
> entry in the ring. At 511 it should work out properly.
>
> Thanks
> Neil
>
>
> diff --git a/drivers/net/b44.c b/drivers/net/b44.c
> index 3d247f3..b7f5ed1 100644
> --- a/drivers/net/b44.c
> +++ b/drivers/net/b44.c
> @@ -57,7 +57,7 @@
> #define B44_MAX_MTU 1500
>
> #define B44_RX_RING_SIZE 512
> -#define B44_DEF_RX_RING_PENDING 200
> +#define B44_DEF_RX_RING_PENDING 511
> #define B44_RX_RING_BYTES (sizeof(struct dma_desc) * \
> B44_RX_RING_SIZE)
> #define B44_TX_RING_SIZE 512
You guys are mixing up quite a bit of stuff here...
The EOT bit has _nothing_ to do with the descriptor pointers.
It simply marks the last descriptor in the (linear) descriptor
page, so that it becomes an actual ring:
DDDDDDDDDDDDDDDDDDDDDDDDDDDE
| O
| T
^--------------------------|
It doesn't say anything about the read and write pointers
to the ring.
The B44_DMARX_PTR is the write-end pointer. It points one entry
beyond the end of the write area. Then there's the software pointer
where we keep track of the read position.
-rx_cons DMARX_PTR-
v v
DDDDDDDDDDDDDDDDDDDDDDDDDDE
^ ^ O
| | T
Device might write from
here to here.
On an RX interrupt (or poll), we read the _actual_ device write
pointer. (B44_DMARX_STAT & DMARX_STAT_CDMASK). If that is equal
to our stored rx_cons, the device didn't write anything.
So we read buffers until we hit the _actual_ device write pointer.
So rx_cons is equal to (B44_DMARX_STAT & DMARX_STAT_CDMASK), except
that it lags behind by one IRQ/poll.
If we read are done, we set the DMARX_PTR write pointer to one desc
beyond the buffer that we just ate. So the device is free to continue
writing the ring _up to_ the position we left.
I don't know why b44 sets the DMARX_PTR to 200 initially (which is 40
descriptors, as this is a byte pointer). This seems kind of arbitrary.
In b43 we set it to (NR_OF_DESCRIPTORS - 1) * sizeof(descriptor).
But I don't think it really matters. It only limits the usable DMA
area before the first interrupt (or poll) occurs. After the final
B44_DMARX_PTR write in b44_rx(), the full descriptor range (well, minus one)
will be usable.
Summary: I don't see where the DMA engine code is broken (except for
the minor missing wmb(), which doesn't trigger this memory corruption, though)
I hope that helps to clear up stuff...
next prev parent reply other threads:[~2011-07-06 15:32 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <bug-38102-10286@https.bugzilla.kernel.org/>
2011-06-29 21:51 ` [Bugme-new] [Bug 38102] New: BUG kmalloc-2048: Poison overwritten Andrew Morton
2011-07-01 6:01 ` Alexey Zaytsev
2011-07-02 21:25 ` Alexey Zaytsev
2011-07-03 15:46 ` Eric Dumazet
2011-07-04 11:48 ` Alexey Zaytsev
2011-07-04 13:05 ` Michael Büsch
2011-07-04 13:57 ` Eric Dumazet
2011-07-04 14:27 ` Michael Büsch
2011-07-04 14:43 ` Michael Büsch
2011-07-04 14:53 ` Eric Dumazet
2011-07-04 15:12 ` Eric Dumazet
2011-07-04 20:25 ` Alexey Zaytsev
2011-07-04 22:29 ` Alexey Zaytsev
2011-07-05 3:44 ` Eric Dumazet
2011-07-05 3:56 ` Alexey Zaytsev
2011-07-05 4:11 ` Eric Dumazet
2011-07-05 4:14 ` Eric Dumazet
2011-07-05 4:17 ` Alexey Zaytsev
2011-07-05 4:18 ` Alexey Zaytsev
2011-07-05 4:25 ` Eric Dumazet
2011-07-05 4:29 ` Alexey Zaytsev
2011-07-05 4:38 ` Eric Dumazet
2011-07-05 4:57 ` Alexey Zaytsev
2011-07-05 5:10 ` Eric Dumazet
2011-07-05 5:18 ` Alexey Zaytsev
2011-07-05 5:33 ` Eric Dumazet
2011-07-05 5:59 ` Eric Dumazet
2011-07-05 16:05 ` Neil Horman
2011-07-05 16:12 ` Eric Dumazet
2011-07-05 16:27 ` Michael Büsch
2011-07-05 16:42 ` Neil Horman
2011-07-05 16:47 ` Eric Dumazet
2011-07-05 16:57 ` Eric Dumazet
2011-07-05 17:01 ` Joe Perches
2011-07-05 17:21 ` Neil Horman
2011-07-05 18:06 ` Neil Horman
2011-07-05 18:13 ` Eric Dumazet
2011-07-05 18:32 ` Eric Dumazet
2011-07-05 18:45 ` Eric Dumazet
2011-07-05 19:53 ` Neil Horman
2011-07-05 20:02 ` Eric Dumazet
2011-07-05 20:15 ` Eric Dumazet
2011-07-05 22:06 ` Neil Horman
2011-07-06 15:32 ` Michael Büsch [this message]
2011-07-06 16:00 ` Eric Dumazet
2011-07-06 16:12 ` Michael Büsch
2011-07-06 16:35 ` Eric Dumazet
2011-07-06 16:56 ` Eric Dumazet
2011-07-07 6:32 ` Alexey Zaytsev
2011-07-07 6:48 ` Eric Dumazet
2011-07-07 7:45 ` Alexey Zaytsev
2011-07-07 9:20 ` Eric Dumazet
2011-07-07 9:34 ` Alexey Zaytsev
2011-07-07 9:37 ` Alexey Zaytsev
2011-07-07 9:43 ` Alexey Zaytsev
2011-07-07 9:52 ` Eric Dumazet
2011-07-05 4:21 ` Eric Dumazet
2011-07-04 14:00 ` Eric Dumazet
2011-07-04 14:31 ` Michael Büsch
2011-07-04 14:45 ` Eric Dumazet
2011-07-04 14:51 ` Michael Büsch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110706173243.404d8599@maggie \
--to=m@bues$(echo .)ch \
--cc=akpm@linux-foundation$(echo .)org \
--cc=alexey.zaytsev@gmail$(echo .)com \
--cc=bugme-daemon@bugzilla$(echo .)kernel.org \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=jolt@tuxbox$(echo .)org \
--cc=mb@bu3sch$(echo .)de \
--cc=nbd@openwrt$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nhorman@tuxdriver$(echo .)com \
--cc=pp@ee$(echo .)oulu.fi \
--cc=zambrano@broadcom$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox