Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
Hello! On 01/05/2017 01:43 PM, Simon Horman wrote: From: Kazuya Mizuguchi "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 [simon: reworked] Signed-off-by: Simon Horman -- v1 [Kazuya Mizuguchi] 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 Not sure that was good idea (sorry)... ravb_tx_ring() only unmaps the transmitted buffers, while we need to unmap everything... * Reduce scope of new local variable --- drivers/net/ethernet/renesas/ravb_main.c | 89 ++-- 1 file changed, 51 insertions(+), 38 deletions(-) diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 92d7692c840d..1797c48e3176 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -179,6 +179,44 @@ static struct mdiobb_ops bb_ops = { .get_mdio_data = ravb_get_mdio_data, }; +/* 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) Here, it stop once an untransmitted buffer is encountered... + 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) { @@ -207,6 +245,18 @@ static void ravb_ring_free(struct net_device *ndev, int 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 *rx_desc = &priv->rx_ring[q][i]; + + if (!dma_mapping_error(ndev->dev.parent, + rx_desc->dptr)) { You forgot le32_to_cpu() here, we can't use the raw descriptor fields. + dma_unmap_single(ndev->dev.parent, +le32_to_cpu(rx_desc->dptr), +PKT_BUF_SZ, +DMA_FROM_DEVICE); + rx_desc->ds_cc = cpu_to_le16(0); You don't check it anyway, not sure what that buys... [...] MBR, Sergei
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
On Fri, Jan 06, 2017 at 10:02:36PM +0300, Sergei Shtylyov wrote: > Hello! > > On 01/05/2017 01:43 PM, Simon Horman wrote: > > >From: Kazuya Mizuguchi > > > >"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 > >[simon: reworked] > >Signed-off-by: Simon Horman > >-- > >v1 [Kazuya Mizuguchi] > > > >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 > >Not sure that was good idea (sorry)... ravb_tx_ring() only unmaps the > transmitted buffers, while we need to unmap everything... > > >* Reduce scope of new local variable > >--- > > drivers/net/ethernet/renesas/ravb_main.c | 89 > > ++-- > > 1 file changed, 51 insertions(+), 38 deletions(-) > > > >diff --git a/drivers/net/ethernet/renesas/ravb_main.c > >b/drivers/net/ethernet/renesas/ravb_main.c > >index 92d7692c840d..1797c48e3176 100644 > >--- a/drivers/net/ethernet/renesas/ravb_main.c > >+++ b/drivers/net/ethernet/renesas/ravb_main.c > >@@ -179,6 +179,44 @@ static struct mdiobb_ops bb_ops = { > > .get_mdio_data = ravb_get_mdio_data, > > }; > > > >+/* 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) > >Here, it stop once an untransmitted buffer is encountered... Yes, I see that now. I wonder if we should: a) paramatise ravb_tx_free() so it may either clear all transmitted buffers (current behaviour) or all buffers (new behaviour). b) provide a different version of this loop in ravb_ring_free() What are your thoughts? > >+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) > > { > >@@ -207,6 +245,18 @@ static void ravb_ring_free(struct net_device *ndev, int > >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 *rx_desc = &priv->rx_ring[q][i]; > >+ > >+if (!dma_mapping_error(ndev->dev.parent, > >+ rx_desc->dptr)) { > > You forgot le32_to_cpu() here, we can't use the raw descriptor fields. Thanks, I will fix that. > >+dma_unmap_single(ndev->dev.parent, > >+ le32_to_cpu(rx_desc->dptr), > >+ PKT_BUF_SZ, > >+ DMA_FROM_DEVICE); > >+rx_desc->ds_cc = cpu_to_le16(0); > >You don't check it anyway, not sure what that buys... Thanks, I will see about dropping that.
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
On 01/12/2017 12:11 PM, Simon Horman wrote: From: Kazuya Mizuguchi "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 [simon: reworked] Signed-off-by: Simon Horman -- v1 [Kazuya Mizuguchi] 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 Not sure that was good idea (sorry)... ravb_tx_ring() only unmaps the transmitted buffers, while we need to unmap everything... * Reduce scope of new local variable --- drivers/net/ethernet/renesas/ravb_main.c | 89 ++-- 1 file changed, 51 insertions(+), 38 deletions(-) diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 92d7692c840d..1797c48e3176 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -179,6 +179,44 @@ static struct mdiobb_ops bb_ops = { .get_mdio_data = ravb_get_mdio_data, }; +/* 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) Here, it stop once an untransmitted buffer is encountered... Yes, I see that now. I wonder if we should: a) paramatise ravb_tx_free() so it may either clear all transmitted buffers (current behaviour) or all buffers (new behaviour). b) provide a different version of this loop in ravb_ring_free() What are your thoughts? I'm voting for (b). [...] MBR, Sergei
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
On Thu, Jan 12, 2017 at 03:03:05PM +0300, Sergei Shtylyov wrote: > On 01/12/2017 12:11 PM, Simon Horman wrote: ... > >> Here, it stop once an untransmitted buffer is encountered... > > > >Yes, I see that now. > > > >I wonder if we should: > > > >a) paramatise ravb_tx_free() so it may either clear all transmitted buffers > > (current behaviour) or all buffers (new behaviour). > >b) provide a different version of this loop in ravb_ring_free() > > > >What are your thoughts? > >I'm voting for (b). Ok, something like this? @@ -215,6 +225,30 @@ static void ravb_ring_free(struct net_device *ndev, int q) } if (priv->tx_ring[q]) { + for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { + struct ravb_tx_desc *desc; + int entry; + + entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * +NUM_TX_DESC); + desc = &priv->tx_ring[q][entry]; + + /* Free the original skb. */ + if (priv->tx_skb[q][entry / NUM_TX_DESC]) { + u32 size = le16_to_cpu(desc->ds_tagl) & TX_DS; + + 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; + } + } + } + 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],
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
Hi, On 12.01.2017 10:11, Simon Horman wrote: + + for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { BTW: How can this work correctly when cur_tx wraps and dirty_tx is greater? Regards, Lino
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
On 01/12/2017 04:18 PM, Simon Horman wrote: ... Here, it stop once an untransmitted buffer is encountered... Yes, I see that now. I wonder if we should: a) paramatise ravb_tx_free() so it may either clear all transmitted buffers (current behaviour) or all buffers (new behaviour). b) provide a different version of this loop in ravb_ring_free() What are your thoughts? I'm voting for (b). Ok, something like this? @@ -215,6 +225,30 @@ static void ravb_ring_free(struct net_device *ndev, int q) } if (priv->tx_ring[q]) { + for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { You're still copying the loop logic from ravb_tx_free() while we (I think) need a simple loop over all the descriptor ring. [...] MBR, Sergei
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
On 01/12/2017 04:23 PM, Lino Sanfilippo wrote: + +for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { BTW: How can this work correctly when cur_tx wraps and dirty_tx is greater? {cur|dirty}_tx never wrap. Regards, Lino MBR, Sergei
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
On Thu, Jan 12, 2017 at 07:33:51PM +0300, Sergei Shtylyov wrote: > On 01/12/2017 04:18 PM, Simon Horman wrote: > > >... > > > Here, it stop once an untransmitted buffer is encountered... > >>> > >>>Yes, I see that now. > >>> > >>>I wonder if we should: > >>> > >>>a) paramatise ravb_tx_free() so it may either clear all transmitted buffers > >>> (current behaviour) or all buffers (new behaviour). > >>>b) provide a different version of this loop in ravb_ring_free() > >>> > >>>What are your thoughts? > >> > >> I'm voting for (b). > > > >Ok, something like this? > > > >@@ -215,6 +225,30 @@ static void ravb_ring_free(struct net_device *ndev, int > >q) > > } > > > > if (priv->tx_ring[q]) { > >+for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; > >priv->dirty_tx[q]++) { > >You're still copying the loop logic from ravb_tx_free() while we (I > think) need a simple loop over all the descriptor ring. I thought the above may be better as it seems to me that only descriptors in the range described above could need to be freed. But simpler may be better. And I also noticed several other problems with the code I posted yesterday afternoon. How about something like this? @@ -215,6 +225,17 @@ static void ravb_ring_free(struct net_device *ndev, int q) } if (priv->tx_ring[q]) { + for (i = 0; i < priv->num_tx_ring[q] * NUM_TX_DESC; i++) { + struct ravb_tx_desc *desc = &priv->tx_ring[q][i]; + + if (desc->die_dt != DT_EEMPTY) { + u32 size = le16_to_cpu(desc->ds_tagl) & TX_DS; + + dma_unmap_single(ndev->dev.parent, +le32_to_cpu(desc->dptr), +size, DMA_TO_DEVICE); + } + } 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],
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
Hello! On 01/12/2017 04:18 PM, Simon Horman wrote: ... Here, it stop once an untransmitted buffer is encountered... Yes, I see that now. I wonder if we should: a) paramatise ravb_tx_free() so it may either clear all transmitted buffers (current behaviour) or all buffers (new behaviour). b) provide a different version of this loop in ravb_ring_free() What are your thoughts? I'm voting for (b). After looking at this issue for another time, I'll vote for (a). Sorry for back-and-forth on this matter -- I somehow thought we could do a better job by scanning all the TX ring... Ok, something like this? @@ -215,6 +225,30 @@ static void ravb_ring_free(struct net_device *ndev, int q) } if (priv->tx_ring[q]) { + for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { + struct ravb_tx_desc *desc; + int entry; + + entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * +NUM_TX_DESC); + desc = &priv->tx_ring[q][entry]; + + /* Free the original skb. */ + if (priv->tx_skb[q][entry / NUM_TX_DESC]) { + u32 size = le16_to_cpu(desc->ds_tagl) & TX_DS; + + 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; + } + } + } + This is only different from ravb_tx_free() by not stopping on unfinished descriptor, so we must be good with adding an extra param to it... [...] MBR, Sergei
Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
On Mon, Jan 16, 2017 at 11:41:51PM +0300, Sergei Shtylyov wrote: > Hello! > > On 01/12/2017 04:18 PM, Simon Horman wrote: > > >... > > Here, it stop once an untransmitted buffer is encountered... > >>> > >>>Yes, I see that now. > >>> > >>>I wonder if we should: > >>> > >>>a) paramatise ravb_tx_free() so it may either clear all transmitted buffers > >>> (current behaviour) or all buffers (new behaviour). > >>>b) provide a different version of this loop in ravb_ring_free() > >>> > >>>What are your thoughts? > >> > >> I'm voting for (b). > >After looking at this issue for another time, I'll vote for (a). Sorry > for back-and-forth on this matter -- I somehow thought we could do a better > job by scanning all the TX ring... No problem. I also have had several opinions on this over time. I'll see about respinning the patch. > > >Ok, something like this? > > > >@@ -215,6 +225,30 @@ static void ravb_ring_free(struct net_device *ndev, int > >q) > > } > > > > if (priv->tx_ring[q]) { > >+for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; > >priv->dirty_tx[q]++) { > >+struct ravb_tx_desc *desc; > >+int entry; > >+ > >+entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] * > >+ NUM_TX_DESC); > >+desc = &priv->tx_ring[q][entry]; > >+ > >+/* Free the original skb. */ > >+if (priv->tx_skb[q][entry / NUM_TX_DESC]) { > >+u32 size = le16_to_cpu(desc->ds_tagl) & TX_DS; > >+ > >+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; > >+} > >+} > >+} > >+ > >This is only different from ravb_tx_free() by not stopping on unfinished > descriptor, so we must be good with adding an extra param to it... > > [...] > > MBR, Sergei >
Re: [EXT] Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
Hi, On 12.01.2017 17:37, Sergei Shtylyov wrote: External Email -- On 01/12/2017 04:23 PM, Lino Sanfilippo wrote: + +for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { BTW: How can this work correctly when cur_tx wraps and dirty_tx is greater? {cur|dirty}_tx never wrap. Both values are 32 bit and AFAICS they are only incremented (and never reset or decremented). So what prevents them from wrapping every 2^32 processed tx descriptors? Am I missing something? Regards, Lino
Re: [EXT] Re: [PATCH/RFC v2 net-next] ravb: unmap descriptors when freeing rings
On 01/12/2017 07:55 PM, Lino Sanfilippo wrote: + +for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) { BTW: How can this work correctly when cur_tx wraps and dirty_tx is greater? {cur|dirty}_tx never wrap. Both values are 32 bit and AFAICS they are only incremented (and never reset or decremented). Yeah, sorry, O thought you mean the TX ring size wrapping. So what prevents them from wrapping every 2^32 processed tx descriptors? Am I missing something? The math should still work all right IIUC. Regards, Lino MBR, Sergei