| Message ID | 1485282073-16662-1-git-send-email-horms+renesas@verge.net.au |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 01/24/2017 09:21 PM, Simon Horman wrote: > From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > > "swiotlb buffer is full" errors occur after repeated initialisation of a > device - f.e. suspend/resume or ip link set up/down. This is because memory > mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit() > is not released. Resolve this problem by unmapping descriptors when > freeing rings. > > Note, ravb_tx_free() is moved but not otherwise modified by this patch. > > Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > [simon: reworked] > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > -- > v3 [Simon Horman] > * As suggested by Sergei Shtylyov > - consistently use le32_to_cpu(desc->dptr) > - Do not clear desc->ds_cc as it is not used > * Paramatise ravb_tx_free() to allow it to free non-transmitted buffers > > v2 [Simon Horman] > * As suggested by Sergei Shtylyov > - Use dma_mapping_error() and rx_desc->ds_cc when unmapping RX descriptors; > this is consistent with the way that they are mapped > - Use ravb_tx_free() to clear TX descriptors > * Reduce scope of new local variable > > v1 [Kazuya Mizuguchi] > --- > drivers/net/ethernet/renesas/ravb_main.c | 113 ++++++++++++++++++------------- > 1 file changed, 65 insertions(+), 48 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 89ac1e3f6175..57fe1411bb9d 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -179,6 +179,51 @@ static struct mdiobb_ops bb_ops = { > .get_mdio_data = ravb_get_mdio_data, > }; > > +enum ravb_tx_free_mode { > + ravb_tx_free_all, > + ravb_tx_free_txed_only, > +}; > + > +/* Free TX skb function for AVB-IP */ > +static int ravb_tx_free(struct net_device *ndev, int q, > + enum ravb_tx_free_mode free_mode) Hmm... Sorry but this looks over-engineered. A *bool* parameter (named e.g 'all) would suffice IMHO. > +{ > + struct ravb_private *priv = netdev_priv(ndev); > + struct net_device_stats *stats = &priv->stats[q]; > + struct ravb_tx_desc *desc; > + int free_num = 0; > + int entry; > + u32 size; > + > + for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { > + entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * > + NUM_TX_DESC); > + desc = &priv->tx_ring[q][entry]; > + if (free_mode == ravb_tx_free_txed_only && > + desc->die_dt != DT_FEMPTY) > + break; > + /* Descriptor type must be checked before all other reads */ > + dma_rmb(); > + size = le16_to_cpu(desc->ds_tagl) & TX_DS; > + /* Free the original skb. */ > + if (priv->tx_skb[q][entry / NUM_TX_DESC]) { > + dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr), > + size, DMA_TO_DEVICE); > + /* Last packet descriptor? */ > + if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) { > + entry /= NUM_TX_DESC; > + dev_kfree_skb_any(priv->tx_skb[q][entry]); > + priv->tx_skb[q][entry] = NULL; > + stats->tx_packets++; > + } > + free_num++; > + } > + stats->tx_bytes += size; Hmmm... we shouldn't count the discarded unsent packets/bytes as sent, right? [...] > @@ -215,12 +262,19 @@ static void ravb_ring_free(struct net_device *ndev, int q) > } > > if (priv->tx_ring[q]) { > + ravb_tx_free(ndev, q, ravb_tx_free_all); > + > ring_size = sizeof(struct ravb_tx_desc) * > (priv->num_tx_ring[q] * NUM_TX_DESC + 1); > dma_free_coherent(ndev->dev.parent, ring_size, priv->tx_ring[q], > priv->tx_desc_dma[q]); > priv->tx_ring[q] = NULL; > } > + > + /* Free TX skb ringbuffer. > + * SKBs are freed by ravb_tx_free() call above. */ This is not a recommended comment format: /* bla * bla */ [...] MBR, Sergei
On 01/24/2017 09:21 PM, Simon Horman wrote: > From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > > "swiotlb buffer is full" errors occur after repeated initialisation of a > device - f.e. suspend/resume or ip link set up/down. This is because memory > mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit() > is not released. Resolve this problem by unmapping descriptors when > freeing rings. Could you look into the sh_eth driver which seems to have the same issue? > Note, ravb_tx_free() is moved but not otherwise modified by this patch. This is not true anymore BTW. > Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > [simon: reworked] > Signed-off-by: Simon Horman <horms+renesas@verge.net.au> MBR, Sergei
On Wed, Jan 25, 2017 at 07:05:08PM +0300, Sergei Shtylyov wrote: > Hello. > > On 01/24/2017 09:21 PM, Simon Horman wrote: > > >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > > > >"swiotlb buffer is full" errors occur after repeated initialisation of a > >device - f.e. suspend/resume or ip link set up/down. This is because memory > >mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit() > >is not released. Resolve this problem by unmapping descriptors when > >freeing rings. > > > >Note, ravb_tx_free() is moved but not otherwise modified by this patch. > > > >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > >[simon: reworked] > >Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > >-- > >v3 [Simon Horman] > >* As suggested by Sergei Shtylyov > > - consistently use le32_to_cpu(desc->dptr) > > - Do not clear desc->ds_cc as it is not used > >* Paramatise ravb_tx_free() to allow it to free non-transmitted buffers > > > >v2 [Simon Horman] > >* As suggested by Sergei Shtylyov > > - Use dma_mapping_error() and rx_desc->ds_cc when unmapping RX descriptors; > > this is consistent with the way that they are mapped > > - Use ravb_tx_free() to clear TX descriptors > >* Reduce scope of new local variable > > > >v1 [Kazuya Mizuguchi] > >--- > > drivers/net/ethernet/renesas/ravb_main.c | 113 ++++++++++++++++++------------- > > 1 file changed, 65 insertions(+), 48 deletions(-) > > > >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > >index 89ac1e3f6175..57fe1411bb9d 100644 > >--- a/drivers/net/ethernet/renesas/ravb_main.c > >+++ b/drivers/net/ethernet/renesas/ravb_main.c > >@@ -179,6 +179,51 @@ static struct mdiobb_ops bb_ops = { > > .get_mdio_data = ravb_get_mdio_data, > > }; > > > >+enum ravb_tx_free_mode { > >+ ravb_tx_free_all, > >+ ravb_tx_free_txed_only, > >+}; > >+ > >+/* Free TX skb function for AVB-IP */ > >+static int ravb_tx_free(struct net_device *ndev, int q, > >+ enum ravb_tx_free_mode free_mode) > > Hmm... Sorry but this looks over-engineered. A *bool* parameter (named > e.g 'all) would suffice IMHO. Ha! The last time I used a bool for something like this I was encouraged to use an enum, admittedly that was not kernel code but I was unsure which way to go this time. I'll change things to bool as you sugget. > >+{ > >+ struct ravb_private *priv = netdev_priv(ndev); > >+ struct net_device_stats *stats = &priv->stats[q]; > >+ struct ravb_tx_desc *desc; > >+ int free_num = 0; > >+ int entry; > >+ u32 size; > >+ > >+ for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { > >+ entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * > >+ NUM_TX_DESC); > >+ desc = &priv->tx_ring[q][entry]; > >+ if (free_mode == ravb_tx_free_txed_only && > >+ desc->die_dt != DT_FEMPTY) > >+ break; > >+ /* Descriptor type must be checked before all other reads */ > >+ dma_rmb(); > >+ size = le16_to_cpu(desc->ds_tagl) & TX_DS; > >+ /* Free the original skb. */ > >+ if (priv->tx_skb[q][entry / NUM_TX_DESC]) { > >+ dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr), > >+ size, DMA_TO_DEVICE); > >+ /* Last packet descriptor? */ > >+ if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) { > >+ entry /= NUM_TX_DESC; > >+ dev_kfree_skb_any(priv->tx_skb[q][entry]); > >+ priv->tx_skb[q][entry] = NULL; > >+ stats->tx_packets++; > >+ } > >+ free_num++; > >+ } > >+ stats->tx_bytes += size; > > Hmmm... we shouldn't count the discarded unsent packets/bytes as sent, right? Yes, I think so. Sorry for missing that. > [...] > >@@ -215,12 +262,19 @@ static void ravb_ring_free(struct net_device *ndev, int q) > > } > > > > if (priv->tx_ring[q]) { > >+ ravb_tx_free(ndev, q, ravb_tx_free_all); > >+ > > ring_size = sizeof(struct ravb_tx_desc) * > > (priv->num_tx_ring[q] * NUM_TX_DESC + 1); > > dma_free_coherent(ndev->dev.parent, ring_size, priv->tx_ring[q], > > priv->tx_desc_dma[q]); > > priv->tx_ring[q] = NULL; > > } > >+ > >+ /* Free TX skb ringbuffer. > >+ * SKBs are freed by ravb_tx_free() call above. */ > > This is not a recommended comment format: > > /* bla > * bla > */ Thanks, I will fix that.
On Wed, Jan 25, 2017 at 07:18:15PM +0300, Sergei Shtylyov wrote: > On 01/24/2017 09:21 PM, Simon Horman wrote: > > >From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > > > >"swiotlb buffer is full" errors occur after repeated initialisation of a > >device - f.e. suspend/resume or ip link set up/down. This is because memory > >mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit() > >is not released. Resolve this problem by unmapping descriptors when > >freeing rings. > > Could you look into the sh_eth driver which seems to have the same issue? Sure, I will check. > >Note, ravb_tx_free() is moved but not otherwise modified by this patch. > > This is not true anymore BTW. Thanks for noticing, I'll fix that. > >Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > >[simon: reworked] > >Signed-off-by: Simon Horman <horms+renesas@verge.net.au> > > MBR, Sergei >
On Wed, Jan 25, 2017 at 5:18 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > On 01/24/2017 09:21 PM, Simon Horman wrote: > >> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> >> >> "swiotlb buffer is full" errors occur after repeated initialisation of a >> device - f.e. suspend/resume or ip link set up/down. This is because >> memory >> mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit() >> is not released. Resolve this problem by unmapping descriptors when >> freeing rings. > > > Could you look into the sh_eth driver which seems to have the same issue? Indeed, after a few suspend/resume cycles on r8a7791/koelsch: WARNING: CPU: 1 PID: 1699 at lib/dma-debug.c:517 add_dma_entry+0xfc/0x148 DMA-API: exceeded 7 overlapping mappings of cacheline 0x0000000001a827e3 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, Feb 10, 2017 at 02:52:50PM +0100, Geert Uytterhoeven wrote: > On Wed, Jan 25, 2017 at 5:18 PM, Sergei Shtylyov > <sergei.shtylyov@cogentembedded.com> wrote: > > On 01/24/2017 09:21 PM, Simon Horman wrote: > > > >> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com> > >> > >> "swiotlb buffer is full" errors occur after repeated initialisation of a > >> device - f.e. suspend/resume or ip link set up/down. This is because > >> memory > >> mapped using dma_map_single() in ravb_ring_format() and ravb_start_xmit() > >> is not released. Resolve this problem by unmapping descriptors when > >> freeing rings. > > > > > > Could you look into the sh_eth driver which seems to have the same issue? > > Indeed, after a few suspend/resume cycles on r8a7791/koelsch: > > WARNING: CPU: 1 PID: 1699 at lib/dma-debug.c:517 add_dma_entry+0xfc/0x148 > DMA-API: exceeded 7 overlapping mappings of cacheline 0x0000000001a827e3 Thanks for confirming that. It matches my expectation after reading of the sh_eth code.
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 89ac1e3f6175..57fe1411bb9d 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -179,6 +179,51 @@ static struct mdiobb_ops bb_ops = { .get_mdio_data = ravb_get_mdio_data, }; +enum ravb_tx_free_mode { + ravb_tx_free_all, + ravb_tx_free_txed_only, +}; + +/* Free TX skb function for AVB-IP */ +static int ravb_tx_free(struct net_device *ndev, int q, + enum ravb_tx_free_mode free_mode) +{ + struct ravb_private *priv = netdev_priv(ndev); + struct net_device_stats *stats = &priv->stats[q]; + struct ravb_tx_desc *desc; + int free_num = 0; + int entry; + u32 size; + + for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { + entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * + NUM_TX_DESC); + desc = &priv->tx_ring[q][entry]; + if (free_mode == ravb_tx_free_txed_only && + desc->die_dt != DT_FEMPTY) + break; + /* Descriptor type must be checked before all other reads */ + dma_rmb(); + size = le16_to_cpu(desc->ds_tagl) & TX_DS; + /* Free the original skb. */ + if (priv->tx_skb[q][entry / NUM_TX_DESC]) { + dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr), + size, DMA_TO_DEVICE); + /* Last packet descriptor? */ + if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) { + entry /= NUM_TX_DESC; + dev_kfree_skb_any(priv->tx_skb[q][entry]); + priv->tx_skb[q][entry] = NULL; + stats->tx_packets++; + } + free_num++; + } + stats->tx_bytes += size; + desc->die_dt = DT_EEMPTY; + } + return free_num; +} + /* Free skb's and DMA buffers for Ethernet AVB */ static void ravb_ring_free(struct net_device *ndev, int q) { @@ -194,19 +239,21 @@ static void ravb_ring_free(struct net_device *ndev, int q) kfree(priv->rx_skb[q]); priv->rx_skb[q] = NULL; - /* Free TX skb ringbuffer */ - if (priv->tx_skb[q]) { - for (i = 0; i < priv->num_tx_ring[q]; i++) - dev_kfree_skb(priv->tx_skb[q][i]); - } - kfree(priv->tx_skb[q]); - priv->tx_skb[q] = NULL; - /* Free aligned TX buffers */ kfree(priv->tx_align[q]); priv->tx_align[q] = NULL; if (priv->rx_ring[q]) { + for (i = 0; i < priv->num_rx_ring[q]; i++) { + struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i]; + + if (!dma_mapping_error(ndev->dev.parent, + le32_to_cpu(desc->dptr))) + dma_unmap_single(ndev->dev.parent, + le32_to_cpu(desc->dptr), + PKT_BUF_SZ, + DMA_FROM_DEVICE); + } ring_size = sizeof(struct ravb_ex_rx_desc) * (priv->num_rx_ring[q] + 1); dma_free_coherent(ndev->dev.parent, ring_size, priv->rx_ring[q], @@ -215,12 +262,19 @@ static void ravb_ring_free(struct net_device *ndev, int q) } if (priv->tx_ring[q]) { + ravb_tx_free(ndev, q, ravb_tx_free_all); + ring_size = sizeof(struct ravb_tx_desc) * (priv->num_tx_ring[q] * NUM_TX_DESC + 1); dma_free_coherent(ndev->dev.parent, ring_size, priv->tx_ring[q], priv->tx_desc_dma[q]); priv->tx_ring[q] = NULL; } + + /* Free TX skb ringbuffer. + * SKBs are freed by ravb_tx_free() call above. */ + kfree(priv->tx_skb[q]); + priv->tx_skb[q] = NULL; } /* Format skb and descriptor buffer for Ethernet AVB */ @@ -431,44 +485,6 @@ static int ravb_dmac_init(struct net_device *ndev) return 0; } -/* Free TX skb function for AVB-IP */ -static int ravb_tx_free(struct net_device *ndev, int q) -{ - struct ravb_private *priv = netdev_priv(ndev); - struct net_device_stats *stats = &priv->stats[q]; - struct ravb_tx_desc *desc; - int free_num = 0; - int entry; - u32 size; - - for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { - entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * - NUM_TX_DESC); - desc = &priv->tx_ring[q][entry]; - if (desc->die_dt != DT_FEMPTY) - break; - /* Descriptor type must be checked before all other reads */ - dma_rmb(); - size = le16_to_cpu(desc->ds_tagl) & TX_DS; - /* Free the original skb. */ - if (priv->tx_skb[q][entry / NUM_TX_DESC]) { - dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr), - size, DMA_TO_DEVICE); - /* Last packet descriptor? */ - if (entry % NUM_TX_DESC == NUM_TX_DESC - 1) { - entry /= NUM_TX_DESC; - dev_kfree_skb_any(priv->tx_skb[q][entry]); - priv->tx_skb[q][entry] = NULL; - stats->tx_packets++; - } - free_num++; - } - stats->tx_bytes += size; - desc->die_dt = DT_EEMPTY; - } - return free_num; -} - static void ravb_get_tx_tstamp(struct net_device *ndev) { struct ravb_private *priv = netdev_priv(ndev); @@ -902,7 +918,7 @@ static int ravb_poll(struct napi_struct *napi, int budget) spin_lock_irqsave(&priv->lock, flags); /* Clear TX interrupt */ ravb_write(ndev, ~mask, TIS); - ravb_tx_free(ndev, q); + ravb_tx_free(ndev, q, ravb_tx_free_txed_only); netif_wake_subqueue(ndev, q); mmiowb(); spin_unlock_irqrestore(&priv->lock, flags); @@ -1567,7 +1583,8 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev) priv->cur_tx[q] += NUM_TX_DESC; if (priv->cur_tx[q] - priv->dirty_tx[q] > - (priv->num_tx_ring[q] - 1) * NUM_TX_DESC && !ravb_tx_free(ndev, q)) + (priv->num_tx_ring[q] - 1) * NUM_TX_DESC && + !ravb_tx_free(ndev, q, ravb_tx_free_txed_only)) netif_stop_subqueue(ndev, q); exit: