[PATCH net 0/2] net: systemport: TX dma fixes

[PATCH net 0/2] net: systemport: TX dma fixes

[Florian Fainelli]

Hi David,

This patch series contains two fixes for our transmit path, first one
is a pretty nasty one since we were not allocating a large enough
dma coherent pool for our transmit descriptors, which would work most of the
time, since allocations are contiguous and we could have.

Second patch fixes a less frequent, though highly likley crash when using
CMA allocations.

I just missed your pull request to Linus, though I assume there will be
another one?

Thanks!

Florian Fainelli (2):
  net: systemport: fix DMA allocation/freeing sizes
  net: systemport: do not crash freeing an unitialized TX ring

 drivers/net/ethernet/broadcom/bcmsysport.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH net 1/2] net: systemport: fix DMA allocation/freeing sizes

[Florian Fainelli]

We should not be allocating a single byte of DMA coherent memory, but
instead a full-sized struct dma_desc (8 bytes).

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 3a6778a667f4..c81bf74685c0 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1110,7 +1110,8 @@ static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
 	/* We just need one DMA descriptor which is DMA-able, since writing to
 	 * the port will allocate a new descriptor in its internal linked-list
 	 */
-	p = dma_zalloc_coherent(kdev, 1, &ring->desc_dma, GFP_KERNEL);
+	p = dma_zalloc_coherent(kdev, sizeof(struct dma_desc), &ring->desc_dma,
+				GFP_KERNEL);
 	if (!p) {
 		netif_err(priv, hw, priv->netdev, "DMA alloc failed\n");
 		return -ENOMEM;
@@ -1183,7 +1184,8 @@ static void bcm_sysport_fini_tx_ring(struct bcm_sysport_priv *priv,
 	ring->cbs = NULL;
 
 	if (ring->desc_dma) {
-		dma_free_coherent(kdev, 1, ring->desc_cpu, ring->desc_dma);
+		dma_free_coherent(kdev, sizeof(struct dma_desc),
+				  ring->desc_cpu, ring->desc_dma);
 		ring->desc_dma = 0;
 	}
 	ring->size = 0;
-- 
1.9.1

[PATCH net 2/2] net: systemport: do not crash freeing an unitialized TX ring

[Florian Fainelli]

Callers of bcm_sysport_init_tx_ring() can currently fail, and will
always call bcm_sysport_fini_tx_ring() in a loop ending at the number of
TX queues (32) without checking if the TX ring was successfully
initialized or not.

Update bcm_sysport_fini_tx_ring() to return early and avoid a crash
de-referencing ring->cbs if the TX ring was not initialized, since
ring->cbs is the last part of the initialization done by
bcm_sysport_init_tx_ring() that could fail.

Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Reported-by: Maxime Bizon <mbizon@freebox.fr>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index c81bf74685c0..531bb7c57531 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1175,6 +1175,13 @@ static void bcm_sysport_fini_tx_ring(struct bcm_sysport_priv *priv,
 	if (!(reg & TDMA_DISABLED))
 		netdev_warn(priv->netdev, "TDMA not stopped!\n");
 
+	/* ring->cbs is the last part in bcm_sysport_init_tx_ring which could
+	 * fail, so by checking this pointer we know whether the TX ring was
+	 * fully initialized or not.
+	 */
+	if (!ring->cbs)
+		return;
+
 	napi_disable(&ring->napi);
 	netif_napi_del(&ring->napi);
 
-- 
1.9.1

Re: [PATCH net 0/2] net: systemport: TX dma fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 31 Oct 2014 15:51:33 -0700

> This patch series contains two fixes for our transmit path, first one
> is a pretty nasty one since we were not allocating a large enough
> dma coherent pool for our transmit descriptors, which would work most of the
> time, since allocations are contiguous and we could have.
> 
> Second patch fixes a less frequent, though highly likley crash when using
> CMA allocations.

Series applied, thanks.

> I just missed your pull request to Linus, though I assume there will be
> another one?

Yes, there should be another one, or two, or three, or...