Message ID | 55b3c89b3549c61d62b8440636516fd572870842.1430817941.git.michal.simek@xilinx.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, May 5, 2015 at 2:55 PM, Michal Simek <michal.simek@xilinx.com> wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > The AXI-DMA rx-delay interrupt can sometimes be triggered > when there are 0 outstanding packets received. This is due > to the fact that the receive function will greedily consume > as many packets as possible on interrupt. So if two packets > (with a very particular timing) arrive in succession they > will each cause the rx-delay interrupt, but the first interrupt > will consume both packets. > This means the second interrupt is a 0 packet receive. > > This is mostly OK, except that the tail pointer register is > updated unconditionally on receive. Currently the tail pointer > is always set to the current bd-ring descriptor under > the assumption that the hardware has moved onto the next > descriptor. What this means for length 0 recv is the current > descriptor that the hardware is potentially yet to use will > be marked as the tail. This causes the hardware to think > its run out of descriptors deadlocking the whole rx path. > Should it marked to stable ? > Fixed by updating the tail pointer to the most recent > successfully consumed descriptor. > > Reported-by: Wendy Liang <wendy.liang@xilinx.com> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Tested-by: Jason Wu <huanyu@xilinx.com> > Acked-by: Michal Simek <michal.simek@xilinx.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > -- 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, 2015-05-05 at 11:25 +0200, Michal Simek wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > The AXI-DMA rx-delay interrupt can sometimes be triggered > when there are 0 outstanding packets received. This is due > to the fact that the receive function will greedily consume > as many packets as possible on interrupt. So if two packets > (with a very particular timing) arrive in succession they > will each cause the rx-delay interrupt, but the first interrupt > will consume both packets. > This means the second interrupt is a 0 packet receive. > > This is mostly OK, except that the tail pointer register is > updated unconditionally on receive. Currently the tail pointer > is always set to the current bd-ring descriptor under > the assumption that the hardware has moved onto the next > descriptor. What this means for length 0 recv is the current > descriptor that the hardware is potentially yet to use will > be marked as the tail. This causes the hardware to think > its run out of descriptors deadlocking the whole rx path. > > Fixed by updating the tail pointer to the most recent > successfully consumed descriptor. I think some of this would be good to have as comments in the code instead of just in the changelog. -- 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 05/05/2015 03:57 PM, Joe Perches wrote: > On Tue, 2015-05-05 at 11:25 +0200, Michal Simek wrote: >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> The AXI-DMA rx-delay interrupt can sometimes be triggered >> when there are 0 outstanding packets received. This is due >> to the fact that the receive function will greedily consume >> as many packets as possible on interrupt. So if two packets >> (with a very particular timing) arrive in succession they >> will each cause the rx-delay interrupt, but the first interrupt >> will consume both packets. >> This means the second interrupt is a 0 packet receive. >> >> This is mostly OK, except that the tail pointer register is >> updated unconditionally on receive. Currently the tail pointer >> is always set to the current bd-ring descriptor under >> the assumption that the hardware has moved onto the next >> descriptor. What this means for length 0 recv is the current >> descriptor that the hardware is potentially yet to use will >> be marked as the tail. This causes the hardware to think >> its run out of descriptors deadlocking the whole rx path. >> >> Fixed by updating the tail pointer to the most recent >> successfully consumed descriptor. > > I think some of this would be good to have as comments > in the code instead of just in the changelog. Is it really needed? If yes, no problem to add it but git blame can point you to that. Thanks, Michal -- 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, 2015-05-05 at 20:49 +0200, Michal Simek wrote: > On 05/05/2015 03:57 PM, Joe Perches wrote: > > On Tue, 2015-05-05 at 11:25 +0200, Michal Simek wrote: > >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > >> > >> The AXI-DMA rx-delay interrupt can sometimes be triggered > >> when there are 0 outstanding packets received. This is due > >> to the fact that the receive function will greedily consume > >> as many packets as possible on interrupt. So if two packets > >> (with a very particular timing) arrive in succession they > >> will each cause the rx-delay interrupt, but the first interrupt > >> will consume both packets. > >> This means the second interrupt is a 0 packet receive. > >> > >> This is mostly OK, except that the tail pointer register is > >> updated unconditionally on receive. Currently the tail pointer > >> is always set to the current bd-ring descriptor under > >> the assumption that the hardware has moved onto the next > >> descriptor. What this means for length 0 recv is the current > >> descriptor that the hardware is potentially yet to use will > >> be marked as the tail. This causes the hardware to think > >> its run out of descriptors deadlocking the whole rx path. > >> > >> Fixed by updating the tail pointer to the most recent > >> successfully consumed descriptor. > > > > I think some of this would be good to have as comments > > in the code instead of just in the changelog. > Is it really needed? If yes, no problem to add it but git blame can > point you to that. That's up to you. I think that useful but concealed information is always hard to follow or find. -- 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/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index 0ab607732bb4..a4840952d372 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -726,15 +726,15 @@ static void axienet_recv(struct net_device *ndev) u32 csumstatus; u32 size = 0; u32 packets = 0; - dma_addr_t tail_p; + dma_addr_t tail_p = 0; struct axienet_local *lp = netdev_priv(ndev); struct sk_buff *skb, *new_skb; struct axidma_bd *cur_p; - tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci; cur_p = &lp->rx_bd_v[lp->rx_bd_ci]; while ((cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK)) { + tail_p = lp->rx_bd_p + sizeof(*lp->rx_bd_v) * lp->rx_bd_ci; skb = (struct sk_buff *) (cur_p->sw_id_offset); length = cur_p->app4 & 0x0000FFFF; @@ -786,7 +786,8 @@ static void axienet_recv(struct net_device *ndev) ndev->stats.rx_packets += packets; ndev->stats.rx_bytes += size; - axienet_dma_out32(lp, XAXIDMA_RX_TDESC_OFFSET, tail_p); + if (tail_p) + axienet_dma_out32(lp, XAXIDMA_RX_TDESC_OFFSET, tail_p); } /**