| Message ID | 1428833292-15356-4-git-send-email-nbd@openwrt.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, 2015-04-12 at 12:08 +0200, Felix Fietkau wrote: > The driver needs to inform the hardware about the first invalid (not yet > filled) rx slot, by writing its DMA descriptor pointer offset to the > BGMAC_DMA_RX_INDEX register. > > This register was set to a value exceeding the rx ring size, effectively > allowing the hardware constant access to the full ring, regardless of > which slots are initialized. > > Fix this by updating the register in bgmac_dma_rx_setup_desc. > > Signed-off-by: Felix Fietkau <nbd@openwrt.org> > --- > drivers/net/ethernet/broadcom/bgmac.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c > index e332de8..856ceee 100644 > --- a/drivers/net/ethernet/broadcom/bgmac.c > +++ b/drivers/net/ethernet/broadcom/bgmac.c > @@ -380,6 +380,12 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac, > dma_desc->addr_high = cpu_to_le32(upper_32_bits(ring->slots[desc_idx].dma_addr)); > dma_desc->ctl0 = cpu_to_le32(ctl0); > dma_desc->ctl1 = cpu_to_le32(ctl1); > + > + desc_idx = (desc_idx + 1) % BGMAC_RX_RING_SLOTS; > + > + bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX, > + ring->index_base + > + desc_idx * sizeof(struct bgmac_dma_desc)); > } > > static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, > @@ -394,9 +400,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, > end_slot &= BGMAC_DMA_RX_STATDPTR; > end_slot /= sizeof(struct bgmac_dma_desc); > > - ring->end = end_slot; > - > - while (ring->start != ring->end) { > + while (ring->start != end_slot) { > struct device *dma_dev = bgmac->core->dma_dev; > struct bgmac_slot_info *slot = &ring->slots[ring->start]; > struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET; > @@ -693,10 +697,6 @@ static void bgmac_dma_init(struct bgmac *bgmac) > for (j = 0; j < ring->num_slots; j++) > bgmac_dma_rx_setup_desc(bgmac, ring, j); > Missing dma_wmb() here (or legacy wmb() for stable kernels) > - bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX, > - ring->index_base + > - ring->num_slots * sizeof(struct bgmac_dma_desc)); > - This might be better for performance to perform one single bgmac_write() at the end of bgmac_dma_rx_read(), and leave this one in place as well, not for performance since this is slow path, but correctness. Also I am surprised there is no memory barrier to make sure changes to dma_desc are committed to memory ? I would use dma_wmb() before bgmac_write(), like the wmb() done in bgmac_dma_tx_add() Thanks -- 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 Sun, 12 Apr 2015 12:08:12 +0200, Felix Fietkau wrote: > The driver needs to inform the hardware about the first invalid (not yet > filled) rx slot, by writing its DMA descriptor pointer offset to the > BGMAC_DMA_RX_INDEX register. > > This register was set to a value exceeding the rx ring size, effectively > allowing the hardware constant access to the full ring, regardless of > which slots are initialized. > > Fix this by updating the register in bgmac_dma_rx_setup_desc. > > Signed-off-by: Felix Fietkau <nbd@openwrt.org> > --- > drivers/net/ethernet/broadcom/bgmac.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c > index e332de8..856ceee 100644 > --- a/drivers/net/ethernet/broadcom/bgmac.c > +++ b/drivers/net/ethernet/broadcom/bgmac.c > @@ -380,6 +380,12 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac, > dma_desc->addr_high = cpu_to_le32(upper_32_bits(ring->slots[desc_idx].dma_addr)); > dma_desc->ctl0 = cpu_to_le32(ctl0); > dma_desc->ctl1 = cpu_to_le32(ctl1); > + > + desc_idx = (desc_idx + 1) % BGMAC_RX_RING_SLOTS; > + > + bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX, > + ring->index_base + > + desc_idx * sizeof(struct bgmac_dma_desc)); > } Newb question if I may: why is no mem barrier necessary in this case? -- 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 2015-04-12 12:31, Eric Dumazet wrote: > On Sun, 2015-04-12 at 12:08 +0200, Felix Fietkau wrote: >> The driver needs to inform the hardware about the first invalid (not yet >> filled) rx slot, by writing its DMA descriptor pointer offset to the >> BGMAC_DMA_RX_INDEX register. >> >> This register was set to a value exceeding the rx ring size, effectively >> allowing the hardware constant access to the full ring, regardless of >> which slots are initialized. >> >> Fix this by updating the register in bgmac_dma_rx_setup_desc. >> >> Signed-off-by: Felix Fietkau <nbd@openwrt.org> >> --- >> drivers/net/ethernet/broadcom/bgmac.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c >> index e332de8..856ceee 100644 >> --- a/drivers/net/ethernet/broadcom/bgmac.c >> +++ b/drivers/net/ethernet/broadcom/bgmac.c >> @@ -693,10 +697,6 @@ static void bgmac_dma_init(struct bgmac *bgmac) >> for (j = 0; j < ring->num_slots; j++) >> bgmac_dma_rx_setup_desc(bgmac, ring, j); >> > > Missing dma_wmb() here (or legacy wmb() for stable kernels) Right, thanks. >> - bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX, >> - ring->index_base + >> - ring->num_slots * sizeof(struct bgmac_dma_desc)); >> - > > > This might be better for performance to perform one single bgmac_write() > at the end of bgmac_dma_rx_read(), and leave this one in place as well, > not for performance since this is slow path, but correctness. I intentionally made it write this field for every slot update, because it might potentially allow the hardware to push frames faster when under pressure. The CPU isn't fast enough to handle gigabit speeds anyway. And by the way, the value that is written in this removed call is quite wrong, so I'd rather just remove it instead of leaving a duplicate here. > Also I am surprised there is no memory barrier to make sure changes to > dma_desc are committed to memory ? > > I would use dma_wmb() before bgmac_write(), like the wmb() done in > bgmac_dma_tx_add() Will add that. - Felix -- 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
Hello. On 4/12/2015 1:08 PM, Felix Fietkau wrote: > The driver needs to inform the hardware about the first invalid (not yet > filled) rx slot, by writing its DMA descriptor pointer offset to the > BGMAC_DMA_RX_INDEX register. > This register was set to a value exceeding the rx ring size, effectively > allowing the hardware constant access to the full ring, regardless of > which slots are initialized. > Fix this by updating the register in bgmac_dma_rx_setup_desc. > Signed-off-by: Felix Fietkau <nbd@openwrt.org> > --- > drivers/net/ethernet/broadcom/bgmac.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c > index e332de8..856ceee 100644 > --- a/drivers/net/ethernet/broadcom/bgmac.c > +++ b/drivers/net/ethernet/broadcom/bgmac.c > @@ -380,6 +380,12 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac, > dma_desc->addr_high = cpu_to_le32(upper_32_bits(ring->slots[desc_idx].dma_addr)); > dma_desc->ctl0 = cpu_to_le32(ctl0); > dma_desc->ctl1 = cpu_to_le32(ctl1); > + > + desc_idx = (desc_idx + 1) % BGMAC_RX_RING_SLOTS; > + > + bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX, > + ring->index_base + > + desc_idx * sizeof(struct bgmac_dma_desc)); The continuation lines should start on the next column after ( on the first line. [...] WBR, Sergei -- 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 Sun, 2015-04-12 at 12:43 +0200, Felix Fietkau wrote: > On 2015-04-12 12:31, Eric Dumazet wrote: > > This might be better for performance to perform one single bgmac_write() > > at the end of bgmac_dma_rx_read(), and leave this one in place as well, > > not for performance since this is slow path, but correctness. > I intentionally made it write this field for every slot update, because > it might potentially allow the hardware to push frames faster when under > pressure. The CPU isn't fast enough to handle gigabit speeds anyway. If CPU is not fast enough, then it makes sense to optimize this part, and save cpu cycles for actual processing. You know, even a fast cpu is not able to keep up at 40Gbits. You have a clear opportunity to have batching right there, take it. -- 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 2015-04-12 19:24, Eric Dumazet wrote: > On Sun, 2015-04-12 at 12:43 +0200, Felix Fietkau wrote: >> On 2015-04-12 12:31, Eric Dumazet wrote: > >> > This might be better for performance to perform one single bgmac_write() >> > at the end of bgmac_dma_rx_read(), and leave this one in place as well, >> > not for performance since this is slow path, but correctness. >> I intentionally made it write this field for every slot update, because >> it might potentially allow the hardware to push frames faster when under >> pressure. The CPU isn't fast enough to handle gigabit speeds anyway. > > If CPU is not fast enough, then it makes sense to optimize this part, > and save cpu cycles for actual processing. > > You know, even a fast cpu is not able to keep up at 40Gbits. > > You have a clear opportunity to have batching right there, take it. Already did that in v2. :) - Felix -- 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/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c index e332de8..856ceee 100644 --- a/drivers/net/ethernet/broadcom/bgmac.c +++ b/drivers/net/ethernet/broadcom/bgmac.c @@ -380,6 +380,12 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac, dma_desc->addr_high = cpu_to_le32(upper_32_bits(ring->slots[desc_idx].dma_addr)); dma_desc->ctl0 = cpu_to_le32(ctl0); dma_desc->ctl1 = cpu_to_le32(ctl1); + + desc_idx = (desc_idx + 1) % BGMAC_RX_RING_SLOTS; + + bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX, + ring->index_base + + desc_idx * sizeof(struct bgmac_dma_desc)); } static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, @@ -394,9 +400,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, end_slot &= BGMAC_DMA_RX_STATDPTR; end_slot /= sizeof(struct bgmac_dma_desc); - ring->end = end_slot; - - while (ring->start != ring->end) { + while (ring->start != end_slot) { struct device *dma_dev = bgmac->core->dma_dev; struct bgmac_slot_info *slot = &ring->slots[ring->start]; struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET; @@ -693,10 +697,6 @@ static void bgmac_dma_init(struct bgmac *bgmac) for (j = 0; j < ring->num_slots; j++) bgmac_dma_rx_setup_desc(bgmac, ring, j); - bgmac_write(bgmac, ring->mmio_base + BGMAC_DMA_RX_INDEX, - ring->index_base + - ring->num_slots * sizeof(struct bgmac_dma_desc)); - ring->start = 0; ring->end = 0; }
The driver needs to inform the hardware about the first invalid (not yet filled) rx slot, by writing its DMA descriptor pointer offset to the BGMAC_DMA_RX_INDEX register. This register was set to a value exceeding the rx ring size, effectively allowing the hardware constant access to the full ring, regardless of which slots are initialized. Fix this by updating the register in bgmac_dma_rx_setup_desc. Signed-off-by: Felix Fietkau <nbd@openwrt.org> --- drivers/net/ethernet/broadcom/bgmac.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)