[PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[Leon Romanovsky]

From: Parav Pandit <parav@nvidia.com>

DMA operation of the IB device is done using ib_device->dma_device.
This is well abstracted using ib_dma APIs.

Hence, instead of doing open access to parent device, use IB core
provided dma mapping APIs.

Signed-off-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/mr.c | 40 +++++++++++++--------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index 090e204ef1e1..d24ac339c053 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -42,7 +42,7 @@
 #include "mlx5_ib.h"
 
 /*
- * We can't use an array for xlt_emergency_page because dma_map_single doesn't
+ * We can't use an array for xlt_emergency_page because ib_dma_map_single doesn't
  * work on kernel modules memory
  */
 void *xlt_emergency_page;
@@ -1081,7 +1081,6 @@ static void *mlx5_ib_create_xlt_wr(struct mlx5_ib_mr *mr,
 				   unsigned int flags)
 {
 	struct mlx5_ib_dev *dev = mr->dev;
-	struct device *ddev = dev->ib_dev.dev.parent;
 	dma_addr_t dma;
 	void *xlt;
 
@@ -1089,8 +1088,8 @@ static void *mlx5_ib_create_xlt_wr(struct mlx5_ib_mr *mr,
 				flags & MLX5_IB_UPD_XLT_ATOMIC ? GFP_ATOMIC :
 								 GFP_KERNEL);
 	sg->length = nents * ent_size;
-	dma = dma_map_single(ddev, xlt, sg->length, DMA_TO_DEVICE);
-	if (dma_mapping_error(ddev, dma)) {
+	dma = ib_dma_map_single(&dev->ib_dev, xlt, sg->length, DMA_TO_DEVICE);
+	if (ib_dma_mapping_error(&dev->ib_dev, dma)) {
 		mlx5_ib_err(dev, "unable to map DMA during XLT update.\n");
 		mlx5_ib_free_xlt(xlt, sg->length);
 		return NULL;
@@ -1118,9 +1117,7 @@ static void *mlx5_ib_create_xlt_wr(struct mlx5_ib_mr *mr,
 static void mlx5_ib_unmap_free_xlt(struct mlx5_ib_dev *dev, void *xlt,
 				   struct ib_sge *sg)
 {
-	struct device *ddev = dev->ib_dev.dev.parent;
-
-	dma_unmap_single(ddev, sg->addr, sg->length, DMA_TO_DEVICE);
+	ib_dma_unmap_single(&dev->ib_dev, sg->addr, sg->length, DMA_TO_DEVICE);
 	mlx5_ib_free_xlt(xlt, sg->length);
 }
 
@@ -1143,7 +1140,6 @@ int mlx5_ib_update_xlt(struct mlx5_ib_mr *mr, u64 idx, int npages,
 		       int page_shift, int flags)
 {
 	struct mlx5_ib_dev *dev = mr->dev;
-	struct device *ddev = dev->ib_dev.dev.parent;
 	void *xlt;
 	struct mlx5_umr_wr wr;
 	struct ib_sge sg;
@@ -1195,11 +1191,9 @@ int mlx5_ib_update_xlt(struct mlx5_ib_mr *mr, u64 idx, int npages,
 	     pages_mapped += pages_iter, idx += pages_iter) {
 		npages = min_t(int, pages_iter, pages_to_map - pages_mapped);
 		size_to_map = npages * desc_size;
-		dma_sync_single_for_cpu(ddev, sg.addr, sg.length,
-					DMA_TO_DEVICE);
+		ib_dma_sync_single_for_cpu(&dev->ib_dev, sg.addr, sg.length, DMA_TO_DEVICE);
 		mlx5_odp_populate_xlt(xlt, idx, npages, mr, flags);
-		dma_sync_single_for_device(ddev, sg.addr, sg.length,
-					   DMA_TO_DEVICE);
+		ib_dma_sync_single_for_device(&dev->ib_dev, sg.addr, sg.length, DMA_TO_DEVICE);
 
 		sg.length = ALIGN(size_to_map, MLX5_UMR_MTT_ALIGNMENT);
 
@@ -1222,7 +1216,6 @@ int mlx5_ib_update_xlt(struct mlx5_ib_mr *mr, u64 idx, int npages,
 static int mlx5_ib_update_mr_pas(struct mlx5_ib_mr *mr, unsigned int flags)
 {
 	struct mlx5_ib_dev *dev = mr->dev;
-	struct device *ddev = dev->ib_dev.dev.parent;
 	struct ib_block_iter biter;
 	struct mlx5_mtt *cur_mtt;
 	struct mlx5_umr_wr wr;
@@ -1247,13 +1240,13 @@ static int mlx5_ib_update_mr_pas(struct mlx5_ib_mr *mr, unsigned int flags)
 	rdma_for_each_block (mr->umem->sg_head.sgl, &biter, mr->umem->nmap,
 			     BIT(mr->page_shift)) {
 		if (cur_mtt == (void *)mtt + sg.length) {
-			dma_sync_single_for_device(ddev, sg.addr, sg.length,
-						   DMA_TO_DEVICE);
+			ib_dma_sync_single_for_device(&dev->ib_dev, sg.addr, sg.length,
+						      DMA_TO_DEVICE);
 			err = mlx5_ib_post_send_wait(dev, &wr);
 			if (err)
 				goto err;
-			dma_sync_single_for_cpu(ddev, sg.addr, sg.length,
-						DMA_TO_DEVICE);
+			ib_dma_sync_single_for_cpu(&dev->ib_dev, sg.addr, sg.length,
+						   DMA_TO_DEVICE);
 			wr.offset += sg.length;
 			cur_mtt = mtt;
 		}
@@ -1270,7 +1263,7 @@ static int mlx5_ib_update_mr_pas(struct mlx5_ib_mr *mr, unsigned int flags)
 	wr.wr.send_flags |= xlt_wr_final_send_flags(flags);
 	wr.xlt_size = sg.length;
 
-	dma_sync_single_for_device(ddev, sg.addr, sg.length, DMA_TO_DEVICE);
+	ib_dma_sync_single_for_device(&dev->ib_dev, sg.addr, sg.length, DMA_TO_DEVICE);
 	err = mlx5_ib_post_send_wait(dev, &wr);
 
 err:
@@ -1763,12 +1756,10 @@ mlx5_alloc_priv_descs(struct ib_device *device,
 
 	mr->descs = PTR_ALIGN(mr->descs_alloc, MLX5_UMR_ALIGN);
 
-	mr->desc_map = dma_map_single(device->dev.parent, mr->descs,
-				      size, DMA_TO_DEVICE);
-	if (dma_mapping_error(device->dev.parent, mr->desc_map)) {
-		ret = -ENOMEM;
+	mr->desc_map = ib_dma_map_single(device, mr->descs, size, DMA_TO_DEVICE);
+	ret = ib_dma_mapping_error(device, mr->desc_map);
+	if (ret)
 		goto err;
-	}
 
 	return 0;
 err:
@@ -1784,8 +1775,7 @@ mlx5_free_priv_descs(struct mlx5_ib_mr *mr)
 		struct ib_device *device = mr->ibmr.device;
 		int size = mr->max_descs * mr->desc_size;
 
-		dma_unmap_single(device->dev.parent, mr->desc_map,
-				 size, DMA_TO_DEVICE);
+		ib_dma_unmap_single(device, mr->desc_map, size, DMA_TO_DEVICE);
 		kfree(mr->descs_alloc);
 		mr->descs = NULL;
 	}
-- 
2.28.0

Re: [PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[Jason Gunthorpe]

On Mon, Nov 23, 2020 at 10:24:00AM +0200, Leon Romanovsky wrote:
> From: Parav Pandit <parav@nvidia.com>
> 
> DMA operation of the IB device is done using ib_device->dma_device.
> This is well abstracted using ib_dma APIs.
> 
> Hence, instead of doing open access to parent device, use IB core
> provided dma mapping APIs.

Why?

The ib DMA APIs are for people using verbs, they are only needed to
pack things into the ib_sge

If you are inside a driver, not using the verbs API, or not using
ib_sge, then you should not be using the ib_dma API

It is an abberation, we should minimize its use.

>  /*
> - * We can't use an array for xlt_emergency_page because dma_map_single doesn't
> + * We can't use an array for xlt_emergency_page because ib_dma_map_single doesn't
>   * work on kernel modules memory
>   */
>  void *xlt_emergency_page;
> @@ -1081,7 +1081,6 @@ static void *mlx5_ib_create_xlt_wr(struct mlx5_ib_mr *mr,
>  				   unsigned int flags)
>  {
>  	struct mlx5_ib_dev *dev = mr->dev;
> -	struct device *ddev = dev->ib_dev.dev.parent;

Though this looks wrong, it should be dev->mdev->pdev.dev

ie it is always OK to use a PCI device with the normal DMA API

Jason

RE: [PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[Parav Pandit]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Monday, November 23, 2020 7:30 PM
> 
> On Mon, Nov 23, 2020 at 10:24:00AM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav@nvidia.com>
> >
> > DMA operation of the IB device is done using ib_device->dma_device.
> > This is well abstracted using ib_dma APIs.
> >
> > Hence, instead of doing open access to parent device, use IB core
> > provided dma mapping APIs.
> 
> Why?
> 
> The ib DMA APIs are for people using verbs, they are only needed to pack things
> into the ib_sge
> 
> If you are inside a driver, not using the verbs API, or not using ib_sge, then you
> should not be using the ib_dma API
>
Thanks for clarifying this. Using ib_dma apis make the code clear for dma device access clear and explicit.
 
> It is an abberation, we should minimize its use.
Alright. In that case will use the pci_dev as mlx5 driver internally has the knowledge of it and avoid using ib_dma APIs.

> 
> >  /*
> > - * We can't use an array for xlt_emergency_page because
> > dma_map_single doesn't
> > + * We can't use an array for xlt_emergency_page because
> > + ib_dma_map_single doesn't
> >   * work on kernel modules memory
> >   */
> >  void *xlt_emergency_page;
> > @@ -1081,7 +1081,6 @@ static void *mlx5_ib_create_xlt_wr(struct
> mlx5_ib_mr *mr,
> >  				   unsigned int flags)
> >  {
> >  	struct mlx5_ib_dev *dev = mr->dev;
> > -	struct device *ddev = dev->ib_dev.dev.parent;
> 
> Though this looks wrong, it should be dev->mdev->pdev.dev
> 
> ie it is always OK to use a PCI device with the normal DMA API
>
Ok. will send v2 that uses pci device with existing DMA APIs.

Re: [PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[Leon Romanovsky]

On Tue, Nov 24, 2020 at 03:34:56AM +0000, Parav Pandit wrote:
>
>
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Monday, November 23, 2020 7:30 PM
> >
> > On Mon, Nov 23, 2020 at 10:24:00AM +0200, Leon Romanovsky wrote:
> > > From: Parav Pandit <parav@nvidia.com>
> > >
> > > DMA operation of the IB device is done using ib_device->dma_device.
> > > This is well abstracted using ib_dma APIs.
> > >
> > > Hence, instead of doing open access to parent device, use IB core
> > > provided dma mapping APIs.
> >
> > Why?
> >
> > The ib DMA APIs are for people using verbs, they are only needed to pack things
> > into the ib_sge
> >
> > If you are inside a driver, not using the verbs API, or not using ib_sge, then you
> > should not be using the ib_dma API
> >
> Thanks for clarifying this. Using ib_dma apis make the code clear for dma device access clear and explicit.
>
> > It is an abberation, we should minimize its use.
> Alright. In that case will use the pci_dev as mlx5 driver internally has the knowledge of it and avoid using ib_dma APIs.

Yeah, let's do v2, although I don't understand the purpose of ib_dma
helpers if ib drivers can't use it.

Thanks

Re: [PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[Christoph Hellwig]

On Mon, Nov 23, 2020 at 10:24:00AM +0200, Leon Romanovsky wrote:
> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> index 090e204ef1e1..d24ac339c053 100644
> --- a/drivers/infiniband/hw/mlx5/mr.c
> +++ b/drivers/infiniband/hw/mlx5/mr.c
> @@ -42,7 +42,7 @@
>  #include "mlx5_ib.h"
>  
>  /*
> - * We can't use an array for xlt_emergency_page because dma_map_single doesn't
> + * We can't use an array for xlt_emergency_page because ib_dma_map_single doesn't

Please avoid the pointlessly overly long line.

> +		ib_dma_sync_single_for_cpu(&dev->ib_dev, sg.addr, sg.length, DMA_TO_DEVICE);

And here and much more.

Re: [PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[Leon Romanovsky]

On Tue, Nov 24, 2020 at 09:31:54AM +0000, Christoph Hellwig wrote:
> On Mon, Nov 23, 2020 at 10:24:00AM +0200, Leon Romanovsky wrote:
> > diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> > index 090e204ef1e1..d24ac339c053 100644
> > --- a/drivers/infiniband/hw/mlx5/mr.c
> > +++ b/drivers/infiniband/hw/mlx5/mr.c
> > @@ -42,7 +42,7 @@
> >  #include "mlx5_ib.h"
> >
> >  /*
> > - * We can't use an array for xlt_emergency_page because dma_map_single doesn't
> > + * We can't use an array for xlt_emergency_page because ib_dma_map_single doesn't
>
> Please avoid the pointlessly overly long line.
>
> > +		ib_dma_sync_single_for_cpu(&dev->ib_dev, sg.addr, sg.length, DMA_TO_DEVICE);
>
> And here and much more.

No problem, I will reduce checkpatch limit from its default.

Thanks

Re: [PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[Christoph Hellwig]

On Tue, Nov 24, 2020 at 11:46:28AM +0200, Leon Romanovsky wrote:
> No problem, I will reduce checkpatch limit from its default.

checkpatch unfortunately does not match what is documented in the
codingstyle document and leads to these kinds of problems :(

Re: [PATCH rdma-next] IB/mlx5: Use ib_dma APIs instead of open access to parent device

[Jason Gunthorpe]

On Tue, Nov 24, 2020 at 07:20:32AM +0200, Leon Romanovsky wrote:
> On Tue, Nov 24, 2020 at 03:34:56AM +0000, Parav Pandit wrote:
> >
> >
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Monday, November 23, 2020 7:30 PM
> > >
> > > On Mon, Nov 23, 2020 at 10:24:00AM +0200, Leon Romanovsky wrote:
> > > > From: Parav Pandit <parav@nvidia.com>
> > > >
> > > > DMA operation of the IB device is done using ib_device->dma_device.
> > > > This is well abstracted using ib_dma APIs.
> > > >
> > > > Hence, instead of doing open access to parent device, use IB core
> > > > provided dma mapping APIs.
> > >
> > > Why?
> > >
> > > The ib DMA APIs are for people using verbs, they are only needed to pack things
> > > into the ib_sge
> > >
> > > If you are inside a driver, not using the verbs API, or not using ib_sge, then you
> > > should not be using the ib_dma API
> > >
> > Thanks for clarifying this. Using ib_dma apis make the code clear for dma device access clear and explicit.
> >
> > > It is an abberation, we should minimize its use.
> > Alright. In that case will use the pci_dev as mlx5 driver internally has the knowledge of it and avoid using ib_dma APIs.
> 
> Yeah, let's do v2, although I don't understand the purpose of ib_dma
> helpers if ib drivers can't use it.

They are for ULPs, not drivers. Drivers are supposed to know how they
are doing dma mapping already

Jason