Message ID | 20110705160531.GC2959@hmsreliant.think-freely.org |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
Le mardi 05 juillet 2011 à 12:05 -0400, Neil Horman a écrit : > On Tue, Jul 05, 2011 at 07:59:33AM +0200, Eric Dumazet wrote: > > Le mardi 05 juillet 2011 à 07:33 +0200, Eric Dumazet a écrit : > > > Le mardi 05 juillet 2011 à 09:18 +0400, Alexey Zaytsev a écrit : > > > > > > > Actually, I've added a trace to show b44_init_rings and b44_free_rings > > > > calls, and they are only called once, right after the driver is > > > > loaded. So it can't be related to START_RFO. Will attach the diff and > > > > dmesg. > > > > > > Thanks > > > > > > I was wondering if DMA could be faster if providing word aligned > > > addresses, could you try : > > > > > > -#define RX_PKT_OFFSET (RX_HEADER_LEN + 2) > > > +#define RX_PKT_OFFSET (RX_HEADER_LEN + NET_IP_ALIGN) > > > > > > (On x86, we now have NET_IP_ALIGN = 0 since commit ea812ca1) > > > > > > > I suspect a hardware bug. > > > I'm not sure if this helps, but I've been reading over this bug, and it seems > that the rx path never checks the status of a buffers rx header prior to > unmapping it or otherwise modifying it in hardware. If we were to start munging > pointers in the rx channel while a dma was active in it still, it sems like the > observed corruption might be the result. The docs aren't super clear on this, > but I think a descriptor needs to be in the idle wait or stopped state prior to > being acessed. This patch might help out there (although I don't have hardware > to test) > Neil > > diff --git a/drivers/net/b44.c b/drivers/net/b44.c > index 3d247f3..48540ad 100644 > --- a/drivers/net/b44.c > +++ b/drivers/net/b44.c > @@ -769,7 +769,19 @@ static int b44_rx(struct b44 *bp, int budget) > dma_addr_t map = rp->mapping; > struct rx_header *rh; > u16 len; > - > + u32 state = br32(bp, B44_DMARX_STAT) & DMARX_STAT_SMASK; > + state >>= 12; > + > + /* > + * I _think_ descriptors need to be in the idle or stopped state > + * before its safe to access them. If the current buffer > + * pointed to by the dma channel is in state 1 or lower (active > + * or disabled), then we should just stop receving until the > + * next interrupt kicks us again (I think) > + */ > + if (state < 2) > + return; > + > dma_sync_single_for_cpu(bp->sdev->dev, map, > RX_PKT_BUF_SZ, > DMA_FROM_DEVICE); Hmm... We are in a NAPI handler... There wont be a new interrupt. Plus, we do at start of b44_rx() : prod = br32(bp, B44_DMARX_STAT) & DMARX_STAT_CDMASK; So all descriptors before prod are guaranteed to be ready for host consume... Fact that a dma access is running on 'next descriptor' should be irrelevant. IMHO Peeking B44_DMARX_STAT for each packet would be a waste of time. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 05 Jul 2011 18:12:32 +0200 Eric Dumazet <eric.dumazet@gmail.com> wrote: > Hmm... We are in a NAPI handler... There wont be a new interrupt. > > Plus, we do at start of b44_rx() : > > prod = br32(bp, B44_DMARX_STAT) & DMARX_STAT_CDMASK; > > So all descriptors before prod are guaranteed to be ready for host > consume... Fact that a dma access is running on 'next descriptor' should > be irrelevant. > > IMHO Peeking B44_DMARX_STAT for each packet would be a waste of time. Yeah I think so, too. We don't need to wait for the _whole_ DMA engine to go idle, before we can access a buffer in the ring. We just need to make sure that the device is finished with that buffer. And we do this by reading the current descriptor pointer. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 05, 2011 at 06:12:32PM +0200, Eric Dumazet wrote: > Le mardi 05 juillet 2011 à 12:05 -0400, Neil Horman a écrit : > > On Tue, Jul 05, 2011 at 07:59:33AM +0200, Eric Dumazet wrote: > > > Le mardi 05 juillet 2011 à 07:33 +0200, Eric Dumazet a écrit : > > > > Le mardi 05 juillet 2011 à 09:18 +0400, Alexey Zaytsev a écrit : > > > > > > > > > Actually, I've added a trace to show b44_init_rings and b44_free_rings > > > > > calls, and they are only called once, right after the driver is > > > > > loaded. So it can't be related to START_RFO. Will attach the diff and > > > > > dmesg. > > > > > > > > Thanks > > > > > > > > I was wondering if DMA could be faster if providing word aligned > > > > addresses, could you try : > > > > > > > > -#define RX_PKT_OFFSET (RX_HEADER_LEN + 2) > > > > +#define RX_PKT_OFFSET (RX_HEADER_LEN + NET_IP_ALIGN) > > > > > > > > (On x86, we now have NET_IP_ALIGN = 0 since commit ea812ca1) > > > > > > > > > > I suspect a hardware bug. > > > > > I'm not sure if this helps, but I've been reading over this bug, and it seems > > that the rx path never checks the status of a buffers rx header prior to > > unmapping it or otherwise modifying it in hardware. If we were to start munging > > pointers in the rx channel while a dma was active in it still, it sems like the > > observed corruption might be the result. The docs aren't super clear on this, > > but I think a descriptor needs to be in the idle wait or stopped state prior to > > being acessed. This patch might help out there (although I don't have hardware > > to test) > > Neil > > > > diff --git a/drivers/net/b44.c b/drivers/net/b44.c > > index 3d247f3..48540ad 100644 > > --- a/drivers/net/b44.c > > +++ b/drivers/net/b44.c > > @@ -769,7 +769,19 @@ static int b44_rx(struct b44 *bp, int budget) > > dma_addr_t map = rp->mapping; > > struct rx_header *rh; > > u16 len; > > - > > + u32 state = br32(bp, B44_DMARX_STAT) & DMARX_STAT_SMASK; > > + state >>= 12; > > + > > + /* > > + * I _think_ descriptors need to be in the idle or stopped state > > + * before its safe to access them. If the current buffer > > + * pointed to by the dma channel is in state 1 or lower (active > > + * or disabled), then we should just stop receving until the > > + * next interrupt kicks us again (I think) > > + */ > > + if (state < 2) > > + return; > > + > > dma_sync_single_for_cpu(bp->sdev->dev, map, > > RX_PKT_BUF_SZ, > > DMA_FROM_DEVICE); > > Hmm... We are in a NAPI handler... There wont be a new interrupt. > Not until we're done with the napi handler, no. But if we encounter a dma descriptor that isn't idle, then we know that either we're clearing out the ring (ie. for a shutdown), or we'll get another interrupt when the descriptor we failed on completes. > Plus, we do at start of b44_rx() : > > prod = br32(bp, B44_DMARX_STAT) & DMARX_STAT_CDMASK; > Yes, that just tells us which is the current dma index. After that we loop through subsequent dma descriptor incrementing the index each time. > So all descriptors before prod are guaranteed to be ready for host > consume... Fact that a dma access is running on 'next descriptor' should > be irrelevant. > But we handle more than one descriptor per b44_rx call - theres a while loop in there where we do advance to the next descriptor. > IMHO Peeking B44_DMARX_STAT for each packet would be a waste of time. > > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le mardi 05 juillet 2011 à 12:42 -0400, Neil Horman a écrit : > On Tue, Jul 05, 2011 at 06:12:32PM +0200, Eric Dumazet wrote: > > So all descriptors before prod are guaranteed to be ready for host > > consume... Fact that a dma access is running on 'next descriptor' should > > be irrelevant. > > > But we handle more than one descriptor per b44_rx call - theres a while loop in > there where we do advance to the next descriptor. Yes, but we advance up to 'prod', which is the very last safe descriptor. If hardware advertises descriptor X being ready to be handled by host, while DMA on this X descriptor is not yet finished, this would be a really useless hardware ;) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 05, 2011 at 06:47:21PM +0200, Eric Dumazet wrote: > Le mardi 05 juillet 2011 à 12:42 -0400, Neil Horman a écrit : > > On Tue, Jul 05, 2011 at 06:12:32PM +0200, Eric Dumazet wrote: > > > > So all descriptors before prod are guaranteed to be ready for host > > > consume... Fact that a dma access is running on 'next descriptor' should > > > be irrelevant. > > > > > But we handle more than one descriptor per b44_rx call - theres a while loop in > > there where we do advance to the next descriptor. > > Yes, but we advance up to 'prod', which is the very last safe > descriptor. > > If hardware advertises descriptor X being ready to be handled by host, > while DMA on this X descriptor is not yet finished, this would be a > really useless hardware ;) > Doh, sorry, I completely missed the fact that the status register index value might be more than 1 entry ahead of cons, and that we advance cons up to prod, but not past. I assume then, that the dma state refers to the state of the channel, rather than the state of the packet that the status register currently indexes? Please disregard everything I said :) Neil > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/b44.c b/drivers/net/b44.c index 3d247f3..48540ad 100644 --- a/drivers/net/b44.c +++ b/drivers/net/b44.c @@ -769,7 +769,19 @@ static int b44_rx(struct b44 *bp, int budget) dma_addr_t map = rp->mapping; struct rx_header *rh; u16 len; - + u32 state = br32(bp, B44_DMARX_STAT) & DMARX_STAT_SMASK; + state >>= 12; + + /* + * I _think_ descriptors need to be in the idle or stopped state + * before its safe to access them. If the current buffer + * pointed to by the dma channel is in state 1 or lower (active + * or disabled), then we should just stop receving until the + * next interrupt kicks us again (I think) + */ + if (state < 2) + return; + dma_sync_single_for_cpu(bp->sdev->dev, map, RX_PKT_BUF_SZ, DMA_FROM_DEVICE);