Re: [PATCH rdma-next] RDMA: Explicitly pass in the dma_device to ib_register_device

["Bernard Metzler" <BMT@zurich.ibm.com>]

-----"Leon Romanovsky" <leon@kernel.org> wrote: -----

>To: "Doug Ledford" <dledford@redhat.com>, "Jason Gunthorpe"
><jgg@nvidia.com>
>From: "Leon Romanovsky" <leon@kernel.org>
>Date: 09/22/2020 10:28AM
>Cc: "Adit Ranadive" <aditr@vmware.com>, "Ariel Elior"
><aelior@marvell.com>, "Bernard Metzler" <bmt@zurich.ibm.com>,
>"Christian Benvenuti" <benve@cisco.com>, "Dennis Dalessandro"
><dennis.dalessandro@intel.com>, "Devesh Sharma"
><devesh.sharma@broadcom.com>, "Faisal Latif"
><faisal.latif@intel.com>, "Gal Pressman" <galpress@amazon.com>,
>"Lijun Ou" <oulijun@huawei.com>, linux-rdma@vger.kernel.org, "Michal
>Kalderon" <mkalderon@marvell.com>, "Mike Marciniszyn"
><mike.marciniszyn@intel.com>, "Naresh Kumar PBS"
><nareshkumar.pbs@broadcom.com>, "Nelson Escobar"
><neescoba@cisco.com>, "Parav Pandit" <parav@nvidia.com>, "Parvi
>Kaustubhi" <pkaustub@cisco.com>, "Potnuri Bharat Teja"
><bharat@chelsio.com>, "Selvin Xavier" <selvin.xavier@broadcom.com>,
>"Shiraz Saleem" <shiraz.saleem@intel.com>, "Somnath Kotur"
><somnath.kotur@broadcom.com>, "Sriharsha Basavapatna"
><sriharsha.basavapatna@broadcom.com>, "VMware PV-Drivers"
><pv-drivers@vmware.com>, "Weihang Li" <liweihang@huawei.com>, "Wei
>Hu(Xavier)" <huwei87@hisilicon.com>, "Yishai Hadas"
><yishaih@nvidia.com>, "Zhu Yanjun" <yanjunz@nvidia.com>
>Subject: [EXTERNAL] [PATCH rdma-next] RDMA: Explicitly pass in the
>dma_device to ib_register_device
>
>From: Jason Gunthorpe <jgg@nvidia.com>
>
>The current design is convoulted where the IB core makes assumptions
>that a real DMA device will usually come from parent unless it looks
>like the ib_device is partially setup for DMA, in which case the
>ib_device itself is used for DMA, but somethings might still come
>from the parent.
>
>Make this clearer by having the caller explicitly specify what the
>DMA device should be. The caller is always responsible to fully
>setup the DMA device it specifies. If NULL is used then the
>ib_device will be used as the DMA device, but the caller must
>still set it up completely.
>
>rvt is the only driver that did not fully setup the DMA device
>before registering. Move the rvt specific code out of
>setup_dma_device() into rvt and set the dma_mask's directly.
>
>Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>Reviewed-by: Parav Pandit <parav@nvidia.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
> drivers/infiniband/core/device.c              | 73
>+++++++------------
> drivers/infiniband/hw/bnxt_re/main.c          |  2 +-
> drivers/infiniband/hw/cxgb4/provider.c        |  3 +-
> drivers/infiniband/hw/efa/efa_main.c          |  2 +-
> drivers/infiniband/hw/hns/hns_roce_main.c     |  2 +-
> drivers/infiniband/hw/i40iw/i40iw_verbs.c     |  2 +-
> drivers/infiniband/hw/mlx4/main.c             |  3 +-
> drivers/infiniband/hw/mlx5/main.c             |  2 +-
> drivers/infiniband/hw/mthca/mthca_provider.c  |  2 +-
> drivers/infiniband/hw/ocrdma/ocrdma_main.c    |  3 +-
> drivers/infiniband/hw/qedr/main.c             |  2 +-
> drivers/infiniband/hw/usnic/usnic_ib_main.c   |  2 +-
> .../infiniband/hw/vmw_pvrdma/pvrdma_main.c    |  2 +-
> drivers/infiniband/sw/rdmavt/vt.c             |  8 +-
> drivers/infiniband/sw/rxe/rxe_verbs.c         |  2 +-
> drivers/infiniband/sw/siw/siw_main.c          |  4 +-
> include/rdma/ib_verbs.h                       |  3 +-
> 17 files changed, 52 insertions(+), 65 deletions(-)
>
>diff --git a/drivers/infiniband/core/device.c
>b/drivers/infiniband/core/device.c
>index ec3becf85cac..417d93bbdaca 100644
>--- a/drivers/infiniband/core/device.c
>+++ b/drivers/infiniband/core/device.c
>@@ -1177,58 +1177,34 @@ static int assign_name(struct ib_device
>*device, const char *name)
> 	return ret;
> }
> 
>-static void setup_dma_device(struct ib_device *device)
>+static void setup_dma_device(struct ib_device *device,
>+			     struct device *dma_device)
> {
>-	struct device *parent = device->dev.parent;
>-
>-	WARN_ON_ONCE(device->dma_device);
>-
>-#ifdef CONFIG_DMA_OPS
>-	if (device->dev.dma_ops) {
>+	if (!dma_device) {
> 		/*
>-		 * The caller provided custom DMA operations. Copy the
>-		 * DMA-related fields that are used by e.g. dma_alloc_coherent()
>-		 * into device->dev.
>+		 * If the caller does not provide a DMA capable device then the
>+		 * IB device will be used. In this case the caller should fully
>+		 * setup the ibdev for DMA. This usually means using
>+		 * dma_virt_ops.
> 		 */
>-		device->dma_device = &device->dev;
>-		if (!device->dev.dma_mask) {
>-			if (parent)
>-				device->dev.dma_mask = parent->dma_mask;
>-			else
>-				WARN_ON_ONCE(true);
>-		}
>-		if (!device->dev.coherent_dma_mask) {
>-			if (parent)
>-				device->dev.coherent_dma_mask =
>-					parent->coherent_dma_mask;
>-			else
>-				WARN_ON_ONCE(true);
>-		}
>-	} else
>-#endif /* CONFIG_DMA_OPS */
>-	{
>+#ifdef CONFIG_DMA_OPS
>+		if (WARN_ON(!device->dev.dma_ops))
>+			return;
>+#endif
>+		if (WARN_ON(!device->dev.dma_parms))
>+			return;
>+
>+		dma_device = &device->dev;
>+	} else {
>+		device->dev.dma_parms = dma_device->dma_parms;
> 		/*
>-		 * The caller did not provide custom DMA operations. Use the
>-		 * DMA mapping operations of the parent device.
>+		 * Auto setup the segment size if a DMA device was passed in.
>+		 * The PCI core sets the maximum segment size to 64 KB. Increase
>+		 * this parameter to 2 GB.
> 		 */
>-		WARN_ON_ONCE(!parent);
>-		device->dma_device = parent;
>-	}
>-
>-	if (!device->dev.dma_parms) {
>-		if (parent) {
>-			/*
>-			 * The caller did not provide DMA parameters, so
>-			 * 'parent' probably represents a PCI device. The PCI
>-			 * core sets the maximum segment size to 64
>-			 * KB. Increase this parameter to 2 GB.
>-			 */
>-			device->dev.dma_parms = parent->dma_parms;
>-			dma_set_max_seg_size(device->dma_device, SZ_2G);
>-		} else {
>-			WARN_ON_ONCE(true);
>-		}
>+		dma_set_max_seg_size(dma_device, SZ_2G);
> 	}
>+	device->dma_device = dma_device;
> }
> 
> /*
>@@ -1241,7 +1217,6 @@ static int setup_device(struct ib_device
>*device)
> 	struct ib_udata uhw = {.outlen = 0, .inlen = 0};
> 	int ret;
> 
>-	setup_dma_device(device);
> 	ib_device_check_mandatory(device);
> 
> 	ret = setup_port_data(device);
>@@ -1361,7 +1336,8 @@ static void prevent_dealloc_device(struct
>ib_device *ib_dev)
>  * asynchronously then the device pointer may become freed as soon
>as this
>  * function returns.
>  */
>-int ib_register_device(struct ib_device *device, const char *name)
>+int ib_register_device(struct ib_device *device, const char *name,
>+		       struct device *dma_device)
> {
> 	int ret;
> 
>@@ -1369,6 +1345,7 @@ int ib_register_device(struct ib_device
>*device, const char *name)
> 	if (ret)
> 		return ret;
> 
>+	setup_dma_device(device, dma_device);
> 	ret = setup_device(device);
> 	if (ret)
> 		return ret;
>diff --git a/drivers/infiniband/hw/bnxt_re/main.c
>b/drivers/infiniband/hw/bnxt_re/main.c
>index 53aee5a42ab8..b3bc62021039 100644
>--- a/drivers/infiniband/hw/bnxt_re/main.c
>+++ b/drivers/infiniband/hw/bnxt_re/main.c
>@@ -736,7 +736,7 @@ static int bnxt_re_register_ib(struct bnxt_re_dev
>*rdev)
> 	if (ret)
> 		return ret;
> 
>-	return ib_register_device(ibdev, "bnxt_re%d");
>+	return ib_register_device(ibdev, "bnxt_re%d",
>&rdev->en_dev->pdev->dev);
> }
> 
> static void bnxt_re_dev_remove(struct bnxt_re_dev *rdev)
>diff --git a/drivers/infiniband/hw/cxgb4/provider.c
>b/drivers/infiniband/hw/cxgb4/provider.c
>index 4b76f2f3f4e4..5f4f3abf41e4 100644
>--- a/drivers/infiniband/hw/cxgb4/provider.c
>+++ b/drivers/infiniband/hw/cxgb4/provider.c
>@@ -570,7 +570,8 @@ void c4iw_register_device(struct work_struct
>*work)
> 	ret = set_netdevs(&dev->ibdev, &dev->rdev);
> 	if (ret)
> 		goto err_dealloc_ctx;
>-	ret = ib_register_device(&dev->ibdev, "cxgb4_%d");
>+	ret = ib_register_device(&dev->ibdev, "cxgb4_%d",
>+				 &dev->rdev.lldi.pdev->dev);
> 	if (ret)
> 		goto err_dealloc_ctx;
> 	return;
>diff --git a/drivers/infiniband/hw/efa/efa_main.c
>b/drivers/infiniband/hw/efa/efa_main.c
>index 92d701146320..4de5be3e1dfe 100644
>--- a/drivers/infiniband/hw/efa/efa_main.c
>+++ b/drivers/infiniband/hw/efa/efa_main.c
>@@ -331,7 +331,7 @@ static int efa_ib_device_add(struct efa_dev *dev)
> 
> 	ib_set_device_ops(&dev->ibdev, &efa_dev_ops);
> 
>-	err = ib_register_device(&dev->ibdev, "efa_%d");
>+	err = ib_register_device(&dev->ibdev, "efa_%d", &pdev->dev);
> 	if (err)
> 		goto err_release_doorbell_bar;
> 
>diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c
>b/drivers/infiniband/hw/hns/hns_roce_main.c
>index 2b4d75733e72..1b5f895d7daf 100644
>--- a/drivers/infiniband/hw/hns/hns_roce_main.c
>+++ b/drivers/infiniband/hw/hns/hns_roce_main.c
>@@ -547,7 +547,7 @@ static int hns_roce_register_device(struct
>hns_roce_dev *hr_dev)
> 		if (ret)
> 			return ret;
> 	}
>-	ret = ib_register_device(ib_dev, "hns_%d");
>+	ret = ib_register_device(ib_dev, "hns_%d", dev);
> 	if (ret) {
> 		dev_err(dev, "ib_register_device failed!\n");
> 		return ret;
>diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
>b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
>index e53f6c0dc12e..945d30a86bbc 100644
>--- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
>+++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
>@@ -2748,7 +2748,7 @@ int i40iw_register_rdma_device(struct
>i40iw_device *iwdev)
> 	if (ret)
> 		goto error;
> 
>-	ret = ib_register_device(&iwibdev->ibdev, "i40iw%d");
>+	ret = ib_register_device(&iwibdev->ibdev, "i40iw%d",
>&iwdev->hw.pcidev->dev);
> 	if (ret)
> 		goto error;
> 
>diff --git a/drivers/infiniband/hw/mlx4/main.c
>b/drivers/infiniband/hw/mlx4/main.c
>index 753c70402498..cd0fba6b0964 100644
>--- a/drivers/infiniband/hw/mlx4/main.c
>+++ b/drivers/infiniband/hw/mlx4/main.c
>@@ -2841,7 +2841,8 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
> 		goto err_steer_free_bitmap;
> 
> 	rdma_set_device_sysfs_group(&ibdev->ib_dev, &mlx4_attr_group);
>-	if (ib_register_device(&ibdev->ib_dev, "mlx4_%d"))
>+	if (ib_register_device(&ibdev->ib_dev, "mlx4_%d",
>+			       &dev->persist->pdev->dev))
> 		goto err_diag_counters;
> 
> 	if (mlx4_ib_mad_init(ibdev))
>diff --git a/drivers/infiniband/hw/mlx5/main.c
>b/drivers/infiniband/hw/mlx5/main.c
>index 3ae681a6ae3b..bca57c7661eb 100644
>--- a/drivers/infiniband/hw/mlx5/main.c
>+++ b/drivers/infiniband/hw/mlx5/main.c
>@@ -4404,7 +4404,7 @@ static int mlx5_ib_stage_ib_reg_init(struct
>mlx5_ib_dev *dev)
> 		name = "mlx5_%d";
> 	else
> 		name = "mlx5_bond_%d";
>-	return ib_register_device(&dev->ib_dev, name);
>+	return ib_register_device(&dev->ib_dev, name,
>&dev->mdev->pdev->dev);
> }
> 
> static void mlx5_ib_stage_pre_ib_reg_umr_cleanup(struct mlx5_ib_dev
>*dev)
>diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c
>b/drivers/infiniband/hw/mthca/mthca_provider.c
>index 31b558ff8218..c4d9cdc4ee97 100644
>--- a/drivers/infiniband/hw/mthca/mthca_provider.c
>+++ b/drivers/infiniband/hw/mthca/mthca_provider.c
>@@ -1206,7 +1206,7 @@ int mthca_register_device(struct mthca_dev
>*dev)
> 	mutex_init(&dev->cap_mask_mutex);
> 
> 	rdma_set_device_sysfs_group(&dev->ib_dev, &mthca_attr_group);
>-	ret = ib_register_device(&dev->ib_dev, "mthca%d");
>+	ret = ib_register_device(&dev->ib_dev, "mthca%d", &dev->pdev->dev);
> 	if (ret)
> 		return ret;
> 
>diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c
>b/drivers/infiniband/hw/ocrdma/ocrdma_main.c
>index d8c47d24d6d6..60416186f1d0 100644
>--- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c
>+++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c
>@@ -255,7 +255,8 @@ static int ocrdma_register_device(struct
>ocrdma_dev *dev)
> 	if (ret)
> 		return ret;
> 
>-	return ib_register_device(&dev->ibdev, "ocrdma%d");
>+	return ib_register_device(&dev->ibdev, "ocrdma%d",
>+				  &dev->nic_info.pdev->dev);
> }
> 
> static int ocrdma_alloc_resources(struct ocrdma_dev *dev)
>diff --git a/drivers/infiniband/hw/qedr/main.c
>b/drivers/infiniband/hw/qedr/main.c
>index 7c0aac3e635b..464becdd41f7 100644
>--- a/drivers/infiniband/hw/qedr/main.c
>+++ b/drivers/infiniband/hw/qedr/main.c
>@@ -293,7 +293,7 @@ static int qedr_register_device(struct qedr_dev
>*dev)
> 	if (rc)
> 		return rc;
> 
>-	return ib_register_device(&dev->ibdev, "qedr%d");
>+	return ib_register_device(&dev->ibdev, "qedr%d", &dev->pdev->dev);
> }
> 
> /* This function allocates fast-path status block memory */
>diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c
>b/drivers/infiniband/hw/usnic/usnic_ib_main.c
>index 462ed71abf53..6c23a5472168 100644
>--- a/drivers/infiniband/hw/usnic/usnic_ib_main.c
>+++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c
>@@ -425,7 +425,7 @@ static void *usnic_ib_device_add(struct pci_dev
>*dev)
> 	if (ret)
> 		goto err_fwd_dealloc;
> 
>-	if (ib_register_device(&us_ibdev->ib_dev, "usnic_%d"))
>+	if (ib_register_device(&us_ibdev->ib_dev, "usnic_%d", &dev->dev))
> 		goto err_fwd_dealloc;
> 
> 	usnic_fwd_set_mtu(us_ibdev->ufdev, us_ibdev->netdev->mtu);
>diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
>b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
>index 780fd2dfc07e..5b2c94441125 100644
>--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
>+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
>@@ -270,7 +270,7 @@ static int pvrdma_register_device(struct
>pvrdma_dev *dev)
> 	spin_lock_init(&dev->srq_tbl_lock);
> 	rdma_set_device_sysfs_group(&dev->ib_dev, &pvrdma_attr_group);
> 
>-	ret = ib_register_device(&dev->ib_dev, "vmw_pvrdma%d");
>+	ret = ib_register_device(&dev->ib_dev, "vmw_pvrdma%d",
>&dev->pdev->dev);
> 	if (ret)
> 		goto err_srq_free;
> 
>diff --git a/drivers/infiniband/sw/rdmavt/vt.c
>b/drivers/infiniband/sw/rdmavt/vt.c
>index f904bb34477a..2f117ac11c8b 100644
>--- a/drivers/infiniband/sw/rdmavt/vt.c
>+++ b/drivers/infiniband/sw/rdmavt/vt.c
>@@ -581,7 +581,11 @@ int rvt_register_device(struct rvt_dev_info
>*rdi)
> 	spin_lock_init(&rdi->n_cqs_lock);
> 
> 	/* DMA Operations */
>-	rdi->ibdev.dev.dma_ops = rdi->ibdev.dev.dma_ops ? : &dma_virt_ops;
>+	rdi->ibdev.dev.dma_ops = &dma_virt_ops;
>+	rdi->ibdev.dev.dma_parms = rdi->ibdev.dev.parent->dma_parms;
>+	rdi->ibdev.dev.dma_mask = rdi->ibdev.dev.parent->dma_mask;
>+	rdi->ibdev.dev.coherent_dma_mask =
>+		rdi->ibdev.dev.parent->coherent_dma_mask;
> 
> 	/* Protection Domain */
> 	spin_lock_init(&rdi->n_pds_lock);
>@@ -629,7 +633,7 @@ int rvt_register_device(struct rvt_dev_info *rdi)
> 		rdi->ibdev.num_comp_vectors = 1;
> 
> 	/* We are now good to announce we exist */
>-	ret = ib_register_device(&rdi->ibdev, dev_name(&rdi->ibdev.dev));
>+	ret = ib_register_device(&rdi->ibdev, dev_name(&rdi->ibdev.dev),
>NULL);
> 	if (ret) {
> 		rvt_pr_err(rdi, "Failed to register driver with ib core.\n");
> 		goto bail_wss;
>diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c
>b/drivers/infiniband/sw/rxe/rxe_verbs.c
>index f368dc16281a..37fee72755be 100644
>--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
>+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
>@@ -1182,7 +1182,7 @@ int rxe_register_device(struct rxe_dev *rxe,
>const char *ibdev_name)
> 	rxe->tfm = tfm;
> 
> 	rdma_set_device_sysfs_group(dev, &rxe_attr_group);
>-	err = ib_register_device(dev, ibdev_name);
>+	err = ib_register_device(dev, ibdev_name, NULL);
> 	if (err)
> 		pr_warn("%s failed with error %d\n", __func__, err);
> 
>diff --git a/drivers/infiniband/sw/siw/siw_main.c
>b/drivers/infiniband/sw/siw/siw_main.c
>index d862bec84376..0362d57b4db8 100644
>--- a/drivers/infiniband/sw/siw/siw_main.c
>+++ b/drivers/infiniband/sw/siw/siw_main.c
>@@ -69,7 +69,7 @@ static int siw_device_register(struct siw_device
>*sdev, const char *name)
> 
> 	sdev->vendor_part_id = dev_id++;
> 
>-	rv = ib_register_device(base_dev, name);
>+	rv = ib_register_device(base_dev, name, NULL);
> 	if (rv) {
> 		pr_warn("siw: device registration error %d\n", rv);
> 		return rv;
>@@ -386,6 +386,8 @@ static struct siw_device
>*siw_device_create(struct net_device *netdev)
> 	base_dev->dev.dma_parms = &sdev->dma_parms;
> 	sdev->dma_parms = (struct device_dma_parameters)
> 		{ .max_segment_size = SZ_2G };
>+	dma_coerce_mask_and_coherent(&base_dev->dev,
>+				     dma_get_required_mask(&base_dev->dev));

Leon, can you please help me to understand this
additional logic? Do we need to setup the DMA device
for (software) RDMA devices which rely on dma_virt_ops
in the end, or better leave it untouched?

Thanks very much!
Bernard.