All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC] net/mlx4: Remove improper usage of dma_alloc_coherent().
@ 2015-04-06 23:00 David Daney
  2015-04-07 19:43 ` Ido Shamay
  0 siblings, 1 reply; 5+ messages in thread
From: David Daney @ 2015-04-06 23:00 UTC (permalink / raw)
  To: linux-kernel, netdev, linux-rdma, David S. Miller
  Cc: Roland Dreier, Sean Hefty, Hal Rosenstock, Amir Vadai,
	Or Gerlitz, Yishai Hadas, Matan Barak, Majd Dibbiny,
	Jack Morgenstein, Moni Shoua, Eugenia Emantayev, Saeed Mahameed,
	Yuval Atias, Maor Gottlieb, David Daney

From: David Daney <david.daney@cavium.com>

The dma_alloc_coherent() function returns a virtual address which can
be used for coherent access to the underlying memory.  On some
architectures, like arm64, undefined behavior results if this memory is
also accessed via virtual mappings that are not coherent.  Because of
their undefined nature, operations like virt_to_page() return garbage
when passed virtual addresses obtained from dma_alloc_coherent().  Any
subsequent mappings via vmap() of the garbage page values are unusable
and result in bad things like bus errors (synchronous aborts in ARM64
speak).

The MLX4 driver contains code that does the equivalent of:

  vmap(virt_to_page(dma_alloc_coherent))

This results in an OOPs when the device is opened.

To fix this...

Always use result of dma_alloc_coherent() directly.

Remove 'max_direct' parameter to mlx4_buf_alloc(), as it is unused,
and adjust all callers.

Remove mlx4_en_map_buffer() and mlx4_en_unmap_buffer() as they now do
nothing, and adjust all callers.

Remove 'page_list' element from struct mlx4_buf as it is unused.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/infiniband/hw/mlx4/cq.c                   |   2 +-
 drivers/infiniband/hw/mlx4/qp.c                   |   2 +-
 drivers/infiniband/hw/mlx4/srq.c                  |   3 +-
 drivers/net/ethernet/mellanox/mlx4/alloc.c        | 104 +++++-----------------
 drivers/net/ethernet/mellanox/mlx4/en_cq.c        |   9 +-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c    |   2 +-
 drivers/net/ethernet/mellanox/mlx4/en_resources.c |  32 -------
 drivers/net/ethernet/mellanox/mlx4/en_rx.c        |  11 +--
 drivers/net/ethernet/mellanox/mlx4/en_tx.c        |  14 +--
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h      |   2 -
 drivers/net/ethernet/mellanox/mlx4/mr.c           |   5 +-
 include/linux/mlx4/device.h                       |  11 +--
 12 files changed, 33 insertions(+), 164 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/cq.c b/drivers/infiniband/hw/mlx4/cq.c
index 0176caa..ab8d6ac 100644
--- a/drivers/infiniband/hw/mlx4/cq.c
+++ b/drivers/infiniband/hw/mlx4/cq.c
@@ -102,7 +102,7 @@ static int mlx4_ib_alloc_cq_buf(struct mlx4_ib_dev *dev, struct mlx4_ib_cq_buf *
 	int err;
 
 	err = mlx4_buf_alloc(dev->dev, nent * dev->dev->caps.cqe_size,
-			     PAGE_SIZE * 2, &buf->buf, GFP_KERNEL);
+			     &buf->buf, GFP_KERNEL);
 
 	if (err)
 		goto out;
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index ed2bd67..e095082 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -768,7 +768,7 @@ static int create_qp_common(struct mlx4_ib_dev *dev, struct ib_pd *pd,
 			*qp->db.db = 0;
 		}
 
-		if (mlx4_buf_alloc(dev->dev, qp->buf_size, PAGE_SIZE * 2, &qp->buf, gfp)) {
+		if (mlx4_buf_alloc(dev->dev, qp->buf_size, &qp->buf, gfp)) {
 			err = -ENOMEM;
 			goto err_db;
 		}
diff --git a/drivers/infiniband/hw/mlx4/srq.c b/drivers/infiniband/hw/mlx4/srq.c
index dce5dfe..121730b 100644
--- a/drivers/infiniband/hw/mlx4/srq.c
+++ b/drivers/infiniband/hw/mlx4/srq.c
@@ -140,8 +140,7 @@ struct ib_srq *mlx4_ib_create_srq(struct ib_pd *pd,
 
 		*srq->db.db = 0;
 
-		if (mlx4_buf_alloc(dev->dev, buf_size, PAGE_SIZE * 2, &srq->buf,
-				   GFP_KERNEL)) {
+		if (mlx4_buf_alloc(dev->dev, buf_size, &srq->buf, GFP_KERNEL)) {
 			err = -ENOMEM;
 			goto err_db;
 		}
diff --git a/drivers/net/ethernet/mellanox/mlx4/alloc.c b/drivers/net/ethernet/mellanox/mlx4/alloc.c
index 0c51c69..db6ba3e 100644
--- a/drivers/net/ethernet/mellanox/mlx4/alloc.c
+++ b/drivers/net/ethernet/mellanox/mlx4/alloc.c
@@ -576,103 +576,41 @@ out:
 
 	return res;
 }
-/*
- * Handling for queue buffers -- we allocate a bunch of memory and
- * register it in a memory region at HCA virtual address 0.  If the
- * requested size is > max_direct, we split the allocation into
- * multiple pages, so we don't require too much contiguous memory.
+
+/* Handling for queue buffers -- we allocate a bunch of memory and
+ * register it in a memory region at HCA virtual address 0.
  */
 
-int mlx4_buf_alloc(struct mlx4_dev *dev, int size, int max_direct,
+int mlx4_buf_alloc(struct mlx4_dev *dev, int size,
 		   struct mlx4_buf *buf, gfp_t gfp)
 {
 	dma_addr_t t;
 
-	if (size <= max_direct) {
-		buf->nbufs        = 1;
-		buf->npages       = 1;
-		buf->page_shift   = get_order(size) + PAGE_SHIFT;
-		buf->direct.buf   = dma_alloc_coherent(&dev->persist->pdev->dev,
-						       size, &t, gfp);
-		if (!buf->direct.buf)
-			return -ENOMEM;
-
-		buf->direct.map = t;
-
-		while (t & ((1 << buf->page_shift) - 1)) {
-			--buf->page_shift;
-			buf->npages *= 2;
-		}
+	buf->nbufs        = 1;
+	buf->npages       = 1;
+	buf->page_shift   = get_order(size) + PAGE_SHIFT;
+	buf->direct.buf   = dma_alloc_coherent(&dev->persist->pdev->dev,
+					       size, &t, gfp);
+	if (!buf->direct.buf)
+		return -ENOMEM;
 
-		memset(buf->direct.buf, 0, size);
-	} else {
-		int i;
-
-		buf->direct.buf  = NULL;
-		buf->nbufs       = (size + PAGE_SIZE - 1) / PAGE_SIZE;
-		buf->npages      = buf->nbufs;
-		buf->page_shift  = PAGE_SHIFT;
-		buf->page_list   = kcalloc(buf->nbufs, sizeof(*buf->page_list),
-					   gfp);
-		if (!buf->page_list)
-			return -ENOMEM;
-
-		for (i = 0; i < buf->nbufs; ++i) {
-			buf->page_list[i].buf =
-				dma_alloc_coherent(&dev->persist->pdev->dev,
-						   PAGE_SIZE,
-						   &t, gfp);
-			if (!buf->page_list[i].buf)
-				goto err_free;
-
-			buf->page_list[i].map = t;
-
-			memset(buf->page_list[i].buf, 0, PAGE_SIZE);
-		}
+	buf->direct.map = t;
 
-		if (BITS_PER_LONG == 64) {
-			struct page **pages;
-			pages = kmalloc(sizeof *pages * buf->nbufs, gfp);
-			if (!pages)
-				goto err_free;
-			for (i = 0; i < buf->nbufs; ++i)
-				pages[i] = virt_to_page(buf->page_list[i].buf);
-			buf->direct.buf = vmap(pages, buf->nbufs, VM_MAP, PAGE_KERNEL);
-			kfree(pages);
-			if (!buf->direct.buf)
-				goto err_free;
-		}
+	while (t & ((1 << buf->page_shift) - 1)) {
+		--buf->page_shift;
+		buf->npages *= 2;
 	}
 
-	return 0;
-
-err_free:
-	mlx4_buf_free(dev, size, buf);
+	memset(buf->direct.buf, 0, size);
 
-	return -ENOMEM;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(mlx4_buf_alloc);
 
 void mlx4_buf_free(struct mlx4_dev *dev, int size, struct mlx4_buf *buf)
 {
-	int i;
-
-	if (buf->nbufs == 1)
-		dma_free_coherent(&dev->persist->pdev->dev, size,
-				  buf->direct.buf,
-				  buf->direct.map);
-	else {
-		if (BITS_PER_LONG == 64)
-			vunmap(buf->direct.buf);
-
-		for (i = 0; i < buf->nbufs; ++i)
-			if (buf->page_list[i].buf)
-				dma_free_coherent(&dev->persist->pdev->dev,
-						  PAGE_SIZE,
-						  buf->page_list[i].buf,
-						  buf->page_list[i].map);
-		kfree(buf->page_list);
-	}
+	dma_free_coherent(&dev->persist->pdev->dev, size,
+			  buf->direct.buf, buf->direct.map);
 }
 EXPORT_SYMBOL_GPL(mlx4_buf_free);
 
@@ -789,7 +727,7 @@ void mlx4_db_free(struct mlx4_dev *dev, struct mlx4_db *db)
 EXPORT_SYMBOL_GPL(mlx4_db_free);
 
 int mlx4_alloc_hwq_res(struct mlx4_dev *dev, struct mlx4_hwq_resources *wqres,
-		       int size, int max_direct)
+		       int size)
 {
 	int err;
 
@@ -799,7 +737,7 @@ int mlx4_alloc_hwq_res(struct mlx4_dev *dev, struct mlx4_hwq_resources *wqres,
 
 	*wqres->db.db = 0;
 
-	err = mlx4_buf_alloc(dev, size, max_direct, &wqres->buf, GFP_KERNEL);
+	err = mlx4_buf_alloc(dev, size, &wqres->buf, GFP_KERNEL);
 	if (err)
 		goto err_db;
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_cq.c b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
index 22da4d0..c541fe4 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
@@ -72,22 +72,16 @@ int mlx4_en_create_cq(struct mlx4_en_priv *priv,
 	 */
 	set_dev_node(&mdev->dev->persist->pdev->dev, node);
 	err = mlx4_alloc_hwq_res(mdev->dev, &cq->wqres,
-				cq->buf_size, 2 * PAGE_SIZE);
+				cq->buf_size);
 	set_dev_node(&mdev->dev->persist->pdev->dev, mdev->dev->numa_node);
 	if (err)
 		goto err_cq;
 
-	err = mlx4_en_map_buffer(&cq->wqres.buf);
-	if (err)
-		goto err_res;
-
 	cq->buf = (struct mlx4_cqe *)cq->wqres.buf.direct.buf;
 	*pcq = cq;
 
 	return 0;
 
-err_res:
-	mlx4_free_hwq_res(mdev->dev, &cq->wqres, cq->buf_size);
 err_cq:
 	kfree(cq);
 	*pcq = NULL;
@@ -189,7 +183,6 @@ void mlx4_en_destroy_cq(struct mlx4_en_priv *priv, struct mlx4_en_cq **pcq)
 	struct mlx4_en_dev *mdev = priv->mdev;
 	struct mlx4_en_cq *cq = *pcq;
 
-	mlx4_en_unmap_buffer(&cq->wqres.buf);
 	mlx4_free_hwq_res(mdev->dev, &cq->wqres, cq->buf_size);
 	if (priv->mdev->dev->caps.comp_pool && cq->vector) {
 		mlx4_release_eq(priv->mdev->dev, cq->vector);
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 3485acf..715265e 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2744,7 +2744,7 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 
 	/* Allocate page for receive rings */
 	err = mlx4_alloc_hwq_res(mdev->dev, &priv->res,
-				MLX4_EN_PAGE_SIZE, MLX4_EN_PAGE_SIZE);
+				MLX4_EN_PAGE_SIZE);
 	if (err) {
 		en_err(priv, "Failed to allocate page for rx qps\n");
 		goto out;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_resources.c b/drivers/net/ethernet/mellanox/mlx4/en_resources.c
index 34f2fdf..c632d51 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_resources.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_resources.c
@@ -80,38 +80,6 @@ void mlx4_en_fill_qp_context(struct mlx4_en_priv *priv, int size, int stride,
 	}
 }
 
-
-int mlx4_en_map_buffer(struct mlx4_buf *buf)
-{
-	struct page **pages;
-	int i;
-
-	if (BITS_PER_LONG == 64 || buf->nbufs == 1)
-		return 0;
-
-	pages = kmalloc(sizeof *pages * buf->nbufs, GFP_KERNEL);
-	if (!pages)
-		return -ENOMEM;
-
-	for (i = 0; i < buf->nbufs; ++i)
-		pages[i] = virt_to_page(buf->page_list[i].buf);
-
-	buf->direct.buf = vmap(pages, buf->nbufs, VM_MAP, PAGE_KERNEL);
-	kfree(pages);
-	if (!buf->direct.buf)
-		return -ENOMEM;
-
-	return 0;
-}
-
-void mlx4_en_unmap_buffer(struct mlx4_buf *buf)
-{
-	if (BITS_PER_LONG == 64 || buf->nbufs == 1)
-		return;
-
-	vunmap(buf->direct.buf);
-}
-
 void mlx4_en_sqp_event(struct mlx4_qp *qp, enum mlx4_event event)
 {
     return;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 698d60d..ac70393 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -392,17 +392,11 @@ int mlx4_en_create_rx_ring(struct mlx4_en_priv *priv,
 
 	/* Allocate HW buffers on provided NUMA node */
 	set_dev_node(&mdev->dev->persist->pdev->dev, node);
-	err = mlx4_alloc_hwq_res(mdev->dev, &ring->wqres,
-				 ring->buf_size, 2 * PAGE_SIZE);
+	err = mlx4_alloc_hwq_res(mdev->dev, &ring->wqres, ring->buf_size);
 	set_dev_node(&mdev->dev->persist->pdev->dev, mdev->dev->numa_node);
 	if (err)
 		goto err_info;
 
-	err = mlx4_en_map_buffer(&ring->wqres.buf);
-	if (err) {
-		en_err(priv, "Failed to map RX buffer\n");
-		goto err_hwq;
-	}
 	ring->buf = ring->wqres.buf.direct.buf;
 
 	ring->hwtstamp_rx_filter = priv->hwtstamp_config.rx_filter;
@@ -410,8 +404,6 @@ int mlx4_en_create_rx_ring(struct mlx4_en_priv *priv,
 	*pring = ring;
 	return 0;
 
-err_hwq:
-	mlx4_free_hwq_res(mdev->dev, &ring->wqres, ring->buf_size);
 err_info:
 	vfree(ring->rx_info);
 	ring->rx_info = NULL;
@@ -498,7 +490,6 @@ void mlx4_en_destroy_rx_ring(struct mlx4_en_priv *priv,
 	struct mlx4_en_dev *mdev = priv->mdev;
 	struct mlx4_en_rx_ring *ring = *pring;
 
-	mlx4_en_unmap_buffer(&ring->wqres.buf);
 	mlx4_free_hwq_res(mdev->dev, &ring->wqres, size * stride + TXBB_SIZE);
 	vfree(ring->rx_info);
 	ring->rx_info = NULL;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 55f9f5c..de61140 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -92,20 +92,13 @@ int mlx4_en_create_tx_ring(struct mlx4_en_priv *priv,
 
 	/* Allocate HW buffers on provided NUMA node */
 	set_dev_node(&mdev->dev->persist->pdev->dev, node);
-	err = mlx4_alloc_hwq_res(mdev->dev, &ring->wqres, ring->buf_size,
-				 2 * PAGE_SIZE);
+	err = mlx4_alloc_hwq_res(mdev->dev, &ring->wqres, ring->buf_size);
 	set_dev_node(&mdev->dev->persist->pdev->dev, mdev->dev->numa_node);
 	if (err) {
 		en_err(priv, "Failed allocating hwq resources\n");
 		goto err_bounce;
 	}
 
-	err = mlx4_en_map_buffer(&ring->wqres.buf);
-	if (err) {
-		en_err(priv, "Failed to map TX buffer\n");
-		goto err_hwq_res;
-	}
-
 	ring->buf = ring->wqres.buf.direct.buf;
 
 	en_dbg(DRV, priv, "Allocated TX ring (addr:%p) - buf:%p size:%d buf_size:%d dma:%llx\n",
@@ -116,7 +109,7 @@ int mlx4_en_create_tx_ring(struct mlx4_en_priv *priv,
 				    MLX4_RESERVE_ETH_BF_QP);
 	if (err) {
 		en_err(priv, "failed reserving qp for TX ring\n");
-		goto err_map;
+		goto err_hwq_res;
 	}
 
 	err = mlx4_qp_alloc(mdev->dev, ring->qpn, &ring->qp, GFP_KERNEL);
@@ -151,8 +144,6 @@ int mlx4_en_create_tx_ring(struct mlx4_en_priv *priv,
 
 err_reserve:
 	mlx4_qp_release_range(mdev->dev, ring->qpn, 1);
-err_map:
-	mlx4_en_unmap_buffer(&ring->wqres.buf);
 err_hwq_res:
 	mlx4_free_hwq_res(mdev->dev, &ring->wqres, ring->buf_size);
 err_bounce:
@@ -178,7 +169,6 @@ void mlx4_en_destroy_tx_ring(struct mlx4_en_priv *priv,
 		mlx4_bf_free(mdev->dev, &ring->bf);
 	mlx4_qp_remove(mdev->dev, &ring->qp);
 	mlx4_qp_free(mdev->dev, &ring->qp);
-	mlx4_en_unmap_buffer(&ring->wqres.buf);
 	mlx4_free_hwq_res(mdev->dev, &ring->wqres, ring->buf_size);
 	kfree(ring->bounce_buf);
 	ring->bounce_buf = NULL;
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index ebbe244..45e2f19 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -808,8 +808,6 @@ void mlx4_en_fill_qp_context(struct mlx4_en_priv *priv, int size, int stride,
 		int is_tx, int rss, int qpn, int cqn, int user_prio,
 		struct mlx4_qp_context *context);
 void mlx4_en_sqp_event(struct mlx4_qp *qp, enum mlx4_event event);
-int mlx4_en_map_buffer(struct mlx4_buf *buf);
-void mlx4_en_unmap_buffer(struct mlx4_buf *buf);
 
 void mlx4_en_calc_rx_buf(struct net_device *dev);
 int mlx4_en_config_rss_steer(struct mlx4_en_priv *priv);
diff --git a/drivers/net/ethernet/mellanox/mlx4/mr.c b/drivers/net/ethernet/mellanox/mlx4/mr.c
index 78f51e1..095f3ca 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mr.c
+++ b/drivers/net/ethernet/mellanox/mlx4/mr.c
@@ -802,10 +802,7 @@ int mlx4_buf_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
 		return -ENOMEM;
 
 	for (i = 0; i < buf->npages; ++i)
-		if (buf->nbufs == 1)
-			page_list[i] = buf->direct.map + (i << buf->page_shift);
-		else
-			page_list[i] = buf->page_list[i].map;
+		page_list[i] = buf->direct.map + (i << buf->page_shift);
 
 	err = mlx4_write_mtt(dev, mtt, 0, buf->npages, page_list);
 
diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index e4ebff7..476bfc2 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h
@@ -573,7 +573,6 @@ struct mlx4_buf_list {
 
 struct mlx4_buf {
 	struct mlx4_buf_list	direct;
-	struct mlx4_buf_list   *page_list;
 	int			nbufs;
 	int			npages;
 	int			page_shift;
@@ -982,16 +981,12 @@ static inline int mlx4_is_slave(struct mlx4_dev *dev)
 	return dev->flags & MLX4_FLAG_SLAVE;
 }
 
-int mlx4_buf_alloc(struct mlx4_dev *dev, int size, int max_direct,
+int mlx4_buf_alloc(struct mlx4_dev *dev, int size,
 		   struct mlx4_buf *buf, gfp_t gfp);
 void mlx4_buf_free(struct mlx4_dev *dev, int size, struct mlx4_buf *buf);
 static inline void *mlx4_buf_offset(struct mlx4_buf *buf, int offset)
 {
-	if (BITS_PER_LONG == 64 || buf->nbufs == 1)
-		return buf->direct.buf + offset;
-	else
-		return buf->page_list[offset >> PAGE_SHIFT].buf +
-			(offset & (PAGE_SIZE - 1));
+	return buf->direct.buf + offset;
 }
 
 int mlx4_pd_alloc(struct mlx4_dev *dev, u32 *pdn);
@@ -1027,7 +1022,7 @@ int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order,
 void mlx4_db_free(struct mlx4_dev *dev, struct mlx4_db *db);
 
 int mlx4_alloc_hwq_res(struct mlx4_dev *dev, struct mlx4_hwq_resources *wqres,
-		       int size, int max_direct);
+		       int size);
 void mlx4_free_hwq_res(struct mlx4_dev *mdev, struct mlx4_hwq_resources *wqres,
 		       int size);
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] net/mlx4: Remove improper usage of dma_alloc_coherent().
  2015-04-06 23:00 [PATCH RFC] net/mlx4: Remove improper usage of dma_alloc_coherent() David Daney
@ 2015-04-07 19:43 ` Ido Shamay
  2015-04-12 21:33   ` Ido Shamay
  0 siblings, 1 reply; 5+ messages in thread
From: Ido Shamay @ 2015-04-07 19:43 UTC (permalink / raw)
  To: David Daney, linux-kernel, netdev, linux-rdma, David S. Miller
  Cc: Roland Dreier, Sean Hefty, Hal Rosenstock, Amir Vadai,
	Or Gerlitz, Yishai Hadas, Matan Barak, Majd Dibbiny,
	Jack Morgenstein, Moni Shoua, Eugenia Emantayev, Saeed Mahameed,
	Yuval Atias, Maor Gottlieb, David Daney, liranl, gdror

On 4/7/2015 2:00 AM, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> The dma_alloc_coherent() function returns a virtual address which can
> be used for coherent access to the underlying memory.  On some
> architectures, like arm64, undefined behavior results if this memory is
> also accessed via virtual mappings that are not coherent.  Because of
> their undefined nature, operations like virt_to_page() return garbage
> when passed virtual addresses obtained from dma_alloc_coherent().  Any
> subsequent mappings via vmap() of the garbage page values are unusable
> and result in bad things like bus errors (synchronous aborts in ARM64
> speak).
>
> The MLX4 driver contains code that does the equivalent of:
>
>    vmap(virt_to_page(dma_alloc_coherent))
>
> This results in an OOPs when the device is opened.
>
> To fix this...
>
> Always use result of dma_alloc_coherent() directly.
Hi David,

I'm not sure this solution is good enough for the common case(s).
Typical allocation size will be around 64KB (with default 1K ring size).
We can't rely on the system to always provide us with that amount of 
contiguous memory.

Current code allocation scheme is more robust, max_direct is typically 2 
* PAGE_SIZE,
so pages from order 1 are far more available then higher order.

I need to check why the code is written as it is today,  and not as in 
this RFC (which is much more trivial).
I'll continue to investigate tomorrow, will get back with some answers.

Ido
> Remove 'max_direct' parameter to mlx4_buf_alloc(), as it is unused,
> and adjust all callers.
>
> Remove mlx4_en_map_buffer() and mlx4_en_unmap_buffer() as they now do
> nothing, and adjust all callers.
>
> Remove 'page_list' element from struct mlx4_buf as it is unused.
>
> Signed-off-by: David Daney <david.daney@cavium.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] net/mlx4: Remove improper usage of dma_alloc_coherent().
  2015-04-07 19:43 ` Ido Shamay
@ 2015-04-12 21:33   ` Ido Shamay
  2015-04-29 14:13     ` Ido Shamay
  0 siblings, 1 reply; 5+ messages in thread
From: Ido Shamay @ 2015-04-12 21:33 UTC (permalink / raw)
  To: David Daney, linux-kernel, netdev, linux-rdma, David S. Miller
  Cc: Roland Dreier, Sean Hefty, Hal Rosenstock, Amir Vadai,
	Or Gerlitz, Yishai Hadas, Matan Barak, Majd Dibbiny,
	Jack Morgenstein, Moni Shoua, Eugenia Emantayev, Saeed Mahameed,
	Yuval Atias, Maor Gottlieb, David Daney, liranl, gdror

On 4/7/2015 10:43 PM, Ido Shamay wrote:
> On 4/7/2015 2:00 AM, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> The dma_alloc_coherent() function returns a virtual address which can
>> be used for coherent access to the underlying memory.  On some
>> architectures, like arm64, undefined behavior results if this memory is
>> also accessed via virtual mappings that are not coherent. Because of
>> their undefined nature, operations like virt_to_page() return garbage
>> when passed virtual addresses obtained from dma_alloc_coherent().  Any
>> subsequent mappings via vmap() of the garbage page values are unusable
>> and result in bad things like bus errors (synchronous aborts in ARM64
>> speak).
>>
>> The MLX4 driver contains code that does the equivalent of:
>>
>>    vmap(virt_to_page(dma_alloc_coherent))
>>
>> This results in an OOPs when the device is opened.
>>
>> To fix this...
>>
>> Always use result of dma_alloc_coherent() directly.
> Hi David,
>
> I'm not sure this solution is good enough for the common case(s).
> Typical allocation size will be around 64KB (with default 1K ring size).
> We can't rely on the system to always provide us with that amount of 
> contiguous memory.
>
> Current code allocation scheme is more robust, max_direct is typically 
> 2 * PAGE_SIZE,
> so pages from order 1 are far more available then higher order.
>
> I need to check why the code is written as it is today,  and not as in 
> this RFC (which is much more trivial).
> I'll continue to investigate tomorrow, will get back with some answers.
>
> Ido

Acked-by: Ido Shamay <idos@mellanox.com>
Thanks David, this is good for us

>> Remove 'max_direct' parameter to mlx4_buf_alloc(), as it is unused,
>> and adjust all callers.
>>
>> Remove mlx4_en_map_buffer() and mlx4_en_unmap_buffer() as they now do
>> nothing, and adjust all callers.
>>
>> Remove 'page_list' element from struct mlx4_buf as it is unused.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] net/mlx4: Remove improper usage of dma_alloc_coherent().
  2015-04-12 21:33   ` Ido Shamay
@ 2015-04-29 14:13     ` Ido Shamay
  2015-04-30  0:07       ` Daney, David
  0 siblings, 1 reply; 5+ messages in thread
From: Ido Shamay @ 2015-04-29 14:13 UTC (permalink / raw)
  To: David Daney, linux-kernel, netdev, linux-rdma, David S. Miller
  Cc: Roland Dreier, Sean Hefty, Hal Rosenstock, Amir Vadai,
	Or Gerlitz, Yishai Hadas, Matan Barak, Majd Dibbiny,
	Jack Morgenstein, Moni Shoua, Eugenia Emantayev, Saeed Mahameed,
	Yuval Atias, Maor Gottlieb, David Daney


On 4/13/2015 12:33 AM, Ido Shamay wrote:
> On 4/7/2015 10:43 PM, Ido Shamay wrote:
>> On 4/7/2015 2:00 AM, David Daney wrote:
>>> From: David Daney <david.daney@cavium.com>
>>>
>>> The dma_alloc_coherent() function returns a virtual address which can
>>> be used for coherent access to the underlying memory.  On some
>>> architectures, like arm64, undefined behavior results if this memory is
>>> also accessed via virtual mappings that are not coherent. Because of
>>> their undefined nature, operations like virt_to_page() return garbage
>>> when passed virtual addresses obtained from dma_alloc_coherent().  Any
>>> subsequent mappings via vmap() of the garbage page values are unusable
>>> and result in bad things like bus errors (synchronous aborts in ARM64
>>> speak).
>>>
>>> The MLX4 driver contains code that does the equivalent of:
>>>
>>>    vmap(virt_to_page(dma_alloc_coherent))
>>>
>>> This results in an OOPs when the device is opened.
>>>
>>> To fix this...
>>>
>>> Always use result of dma_alloc_coherent() directly.
> Acked-by: Ido Shamay <idos@mellanox.com>
> Thanks David, this is good for us

Hi David,

Are you resending this patch or you want us to do that?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH RFC] net/mlx4: Remove improper usage of dma_alloc_coherent().
  2015-04-29 14:13     ` Ido Shamay
@ 2015-04-30  0:07       ` Daney, David
  0 siblings, 0 replies; 5+ messages in thread
From: Daney, David @ 2015-04-30  0:07 UTC (permalink / raw)
  To: Ido Shamay, David Daney, linux-kernel, netdev, linux-rdma,
	David S. Miller
  Cc: Roland Dreier, Sean Hefty, Hal Rosenstock, Amir Vadai,
	Or Gerlitz, Yishai Hadas, Matan Barak, Majd Dibbiny,
	Jack Morgenstein, Moni Shoua, Eugenia Emantayev, Saeed Mahameed,
	Yuval Atias, Maor Gottlieb, David Daney

First of all, let me apologize for top posting, but it is currently my only option.

I have been away from my office for the last couple of weeks, I was going to re-send on Monday, May 4.  If you would like to do it before then, please go ahead and do that.

Thanks,
David Daney

-----Original Message-----
From: Ido Shamay [mailto:idos@dev.mellanox.co.il] 
Sent: Wednesday, April 29, 2015 7:14 AM
To: David Daney; linux-kernel@vger.kernel.org; netdev@vger.kernel.org; linux-rdma@vger.kernel.org; David S. Miller
Cc: Roland Dreier; Sean Hefty; Hal Rosenstock; Amir Vadai; Or Gerlitz; Yishai Hadas; Matan Barak; Majd Dibbiny; Jack Morgenstein; Moni Shoua; Eugenia Emantayev; Saeed Mahameed; Yuval Atias; Maor Gottlieb; David Daney
Subject: Re: [PATCH RFC] net/mlx4: Remove improper usage of dma_alloc_coherent().


On 4/13/2015 12:33 AM, Ido Shamay wrote:
> On 4/7/2015 10:43 PM, Ido Shamay wrote:
>> On 4/7/2015 2:00 AM, David Daney wrote:
>>> From: David Daney <david.daney@cavium.com>
>>>
>>> The dma_alloc_coherent() function returns a virtual address which 
>>> can be used for coherent access to the underlying memory.  On some 
>>> architectures, like arm64, undefined behavior results if this memory 
>>> is also accessed via virtual mappings that are not coherent. Because 
>>> of their undefined nature, operations like virt_to_page() return 
>>> garbage when passed virtual addresses obtained from 
>>> dma_alloc_coherent().  Any subsequent mappings via vmap() of the 
>>> garbage page values are unusable and result in bad things like bus 
>>> errors (synchronous aborts in ARM64 speak).
>>>
>>> The MLX4 driver contains code that does the equivalent of:
>>>
>>>    vmap(virt_to_page(dma_alloc_coherent))
>>>
>>> This results in an OOPs when the device is opened.
>>>
>>> To fix this...
>>>
>>> Always use result of dma_alloc_coherent() directly.
> Acked-by: Ido Shamay <idos@mellanox.com> Thanks David, this is good 
> for us

Hi David,

Are you resending this patch or you want us to do that?

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-04-30  0:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-06 23:00 [PATCH RFC] net/mlx4: Remove improper usage of dma_alloc_coherent() David Daney
2015-04-07 19:43 ` Ido Shamay
2015-04-12 21:33   ` Ido Shamay
2015-04-29 14:13     ` Ido Shamay
2015-04-30  0:07       ` Daney, David

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.