[PATCH for-next 0/3] Clean up and improvements for 5.7

[PATCH for-next 0/3] Clean up and improvements for 5.7

[Dennis Dalessandro]

Here are some clean up/improvement patches. Mike got rid of dead code and Kaike
took a stab at fixing the kobj and cdev complaints.  This serious should
go before the AIP posting. I'll be posting a v2 of the AIP code soon, and I
think I still owe a response on one issue. Coming up, but for now it's just
these 3.

---

Kaike Wan (2):
      IB/hfi1: Remove kobj from hfi1_devdata
      IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

Mike Marciniszyn (1):
      IB/rdmavt: Delete unused routine


 drivers/infiniband/hw/hfi1/device.c   |   23 ++++++++++++-----
 drivers/infiniband/hw/hfi1/file_ops.c |    9 ++-----
 drivers/infiniband/hw/hfi1/hfi.h      |    3 --
 drivers/infiniband/hw/hfi1/init.c     |   44 ++++++++++-----------------------
 drivers/infiniband/sw/rdmavt/vt.c     |    6 -----
 5 files changed, 32 insertions(+), 53 deletions(-)

--
-Denny

[PATCH for-next 1/3] IB/rdmavt: Delete unused routine

[Dennis Dalessandro]

From: Mike Marciniszyn <mike.marciniszyn@intel.com>

This routine was obsoleted by the patch below.

Delete it.

Fixes: a2a074ef396f ("RDMA: Handle ucontext allocations by IB/core")
Reviewed-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/sw/rdmavt/vt.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/infiniband/sw/rdmavt/vt.c b/drivers/infiniband/sw/rdmavt/vt.c
index 986265a..72b031a 100644
--- a/drivers/infiniband/sw/rdmavt/vt.c
+++ b/drivers/infiniband/sw/rdmavt/vt.c
@@ -284,12 +284,6 @@ static int rvt_query_gid(struct ib_device *ibdev, u8 port_num,
 					 &gid->global.interface_id);
 }
 
-static inline struct rvt_ucontext *to_iucontext(struct ib_ucontext
-						*ibucontext)
-{
-	return container_of(ibucontext, struct rvt_ucontext, ibucontext);
-}
-
 /**
  * rvt_alloc_ucontext - Allocate a user context
  * @uctx: Verbs context

[PATCH for-next 2/3] IB/hfi1: Remove kobj from hfi1_devdata

[Dennis Dalessandro]

From: Kaike Wan <kaike.wan@intel.com>

The field kobj was added to hfi1_devdata structure to manage the life
time of the hfi1_devdata structure for PSM accesses:

commit e11ffbd57520 ("IB/hfi1: Do not free hfi1 cdev parent structure early")

Later another mechanism user_refcount/user_comp was introduced to provide the same
functionality:

commit acd7c8fe1493 ("IB/hfi1: Fix an Oops on pci device force remove")

This patch will remove this kobj field, as it is no longer needed.

Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/hfi1/file_ops.c |    4 +---
 drivers/infiniband/hw/hfi1/hfi.h      |    2 --
 drivers/infiniband/hw/hfi1/init.c     |   26 ++++----------------------
 3 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index 2591158..e7fdd70 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -209,7 +209,6 @@ static int hfi1_file_open(struct inode *inode, struct file *fp)
 	fd->mm = current->mm;
 	mmgrab(fd->mm);
 	fd->dd = dd;
-	kobject_get(&fd->dd->kobj);
 	fp->private_data = fd;
 	return 0;
 nomem:
@@ -713,7 +712,6 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
 	deallocate_ctxt(uctxt);
 done:
 	mmdrop(fdata->mm);
-	kobject_put(&dd->kobj);
 
 	if (atomic_dec_and_test(&dd->user_refcount))
 		complete(&dd->user_comp);
@@ -1696,7 +1694,7 @@ static int user_add(struct hfi1_devdata *dd)
 	snprintf(name, sizeof(name), "%s_%d", class_name(), dd->unit);
 	ret = hfi1_cdev_init(dd->unit, name, &hfi1_file_ops,
 			     &dd->user_cdev, &dd->user_device,
-			     true, &dd->kobj);
+			     true, &dd->verbs_dev.rdi.ibdev.dev.kobj);
 	if (ret)
 		user_remove(dd);
 
diff --git a/drivers/infiniband/hw/hfi1/hfi.h b/drivers/infiniband/hw/hfi1/hfi.h
index cae12f4..b06c259 100644
--- a/drivers/infiniband/hw/hfi1/hfi.h
+++ b/drivers/infiniband/hw/hfi1/hfi.h
@@ -1413,8 +1413,6 @@ struct hfi1_devdata {
 	bool aspm_enabled;	/* ASPM state: enabled/disabled */
 	struct rhashtable *sdma_rht;
 
-	struct kobject kobj;
-
 	/* vnic data */
 	struct hfi1_vnic_data vnic;
 	/* Lock to protect IRQ SRC register access */
diff --git a/drivers/infiniband/hw/hfi1/init.c b/drivers/infiniband/hw/hfi1/init.c
index e3acda7..3759d92 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -1198,13 +1198,13 @@ static void finalize_asic_data(struct hfi1_devdata *dd,
 }
 
 /**
- * hfi1_clean_devdata - cleans up per-unit data structure
+ * hfi1_free_devdata - cleans up and frees per-unit data structure
  * @dd: pointer to a valid devdata structure
  *
- * It cleans up all data structures set up by
+ * It cleans up and frees all data structures set up by
  * by hfi1_alloc_devdata().
  */
-static void hfi1_clean_devdata(struct hfi1_devdata *dd)
+void hfi1_free_devdata(struct hfi1_devdata *dd)
 {
 	struct hfi1_asic_data *ad;
 	unsigned long flags;
@@ -1231,23 +1231,6 @@ static void hfi1_clean_devdata(struct hfi1_devdata *dd)
 	rvt_dealloc_device(&dd->verbs_dev.rdi);
 }
 
-static void __hfi1_free_devdata(struct kobject *kobj)
-{
-	struct hfi1_devdata *dd =
-		container_of(kobj, struct hfi1_devdata, kobj);
-
-	hfi1_clean_devdata(dd);
-}
-
-static struct kobj_type hfi1_devdata_type = {
-	.release = __hfi1_free_devdata,
-};
-
-void hfi1_free_devdata(struct hfi1_devdata *dd)
-{
-	kobject_put(&dd->kobj);
-}
-
 /**
  * hfi1_alloc_devdata - Allocate our primary per-unit data structure.
  * @pdev: Valid PCI device
@@ -1333,11 +1316,10 @@ static struct hfi1_devdata *hfi1_alloc_devdata(struct pci_dev *pdev,
 		goto bail;
 	}
 
-	kobject_init(&dd->kobj, &hfi1_devdata_type);
 	return dd;
 
 bail:
-	hfi1_clean_devdata(dd);
+	hfi1_free_devdata(dd);
 	return ERR_PTR(ret);
 }
 

[PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Dennis Dalessandro]

From: Kaike Wan <kaike.wan@intel.com>

This patch is implemented to address the concerns raised in:
  https://marc.info/?l=linux-rdma&m=158101337614772&w=2

The hfi1 driver dynammically allocates a struct device to represent the
cdev in sysfs and devtmpfs (/dev/hfi1_x). On the other hand, the
hfi1_devdata already contains a struct device in its ibdev field
(hfi1_devdata.verbs_dev.rdi.ibdev.dev), and it is therefore possible to
eliminate the dynamical allocation when creating the cdev. Since each
device could be added to the sysfs only once and the function
device_add() is already called for the ibdev in ib_register_device(),
the function cdev_device_add() could not be used to create the cdev,
even though the hfi1_devdata contains both cdev and ibdev in the same
structure.

This patch eliminates the dynamic allocation by creating the cdev
first, setting up the ibdev, and then calling the ib_register_device()
to add the device to sysfs and devtmpfs.

Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/hfi1/device.c   |   23 ++++++++++++++++-------
 drivers/infiniband/hw/hfi1/file_ops.c |    5 ++---
 drivers/infiniband/hw/hfi1/hfi.h      |    1 -
 drivers/infiniband/hw/hfi1/init.c     |   18 +++++++++---------
 4 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/device.c b/drivers/infiniband/hw/hfi1/device.c
index bbb6069..4e1ad5f 100644
--- a/drivers/infiniband/hw/hfi1/device.c
+++ b/drivers/infiniband/hw/hfi1/device.c
@@ -79,10 +79,12 @@ int hfi1_cdev_init(int minor, const char *name,
 		goto done;
 	}
 
-	if (user_accessible)
-		device = device_create(user_class, NULL, dev, NULL, "%s", name);
-	else
+	if (user_accessible) {
+		device = kobj_to_dev(parent);
+		device->devt = dev;
+	} else {
 		device = device_create(class, NULL, dev, NULL, "%s", name);
+	}
 
 	if (IS_ERR(device)) {
 		ret = PTR_ERR(device);
@@ -92,20 +94,27 @@ int hfi1_cdev_init(int minor, const char *name,
 		cdev_del(cdev);
 	}
 done:
-	*devp = device;
+	if (devp)
+		*devp = device;
 	return ret;
 }
 
+/*
+ * The pointer devp will be provided only if *devp is allocated
+ * dynamically, as shown in device_create().
+ */
 void hfi1_cdev_cleanup(struct cdev *cdev, struct device **devp)
 {
-	struct device *device = *devp;
+	struct device *device = NULL;
 
+	if (devp)
+		device = *devp;
 	if (device) {
 		device_unregister(device);
 		*devp = NULL;
-
-		cdev_del(cdev);
 	}
+	/* This will also decrement its parent's refcount */
+	cdev_del(cdev);
 }
 
 static const char *hfi1_class_name = "hfi1";
diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index e7fdd70..38827e4 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -1682,8 +1682,7 @@ static int ctxt_reset(struct hfi1_ctxtdata *uctxt)
 
 static void user_remove(struct hfi1_devdata *dd)
 {
-
-	hfi1_cdev_cleanup(&dd->user_cdev, &dd->user_device);
+	hfi1_cdev_cleanup(&dd->user_cdev, NULL);
 }
 
 static int user_add(struct hfi1_devdata *dd)
@@ -1693,7 +1692,7 @@ static int user_add(struct hfi1_devdata *dd)
 
 	snprintf(name, sizeof(name), "%s_%d", class_name(), dd->unit);
 	ret = hfi1_cdev_init(dd->unit, name, &hfi1_file_ops,
-			     &dd->user_cdev, &dd->user_device,
+			     &dd->user_cdev, NULL,
 			     true, &dd->verbs_dev.rdi.ibdev.dev.kobj);
 	if (ret)
 		user_remove(dd);
diff --git a/drivers/infiniband/hw/hfi1/hfi.h b/drivers/infiniband/hw/hfi1/hfi.h
index b06c259..8e63b11 100644
--- a/drivers/infiniband/hw/hfi1/hfi.h
+++ b/drivers/infiniband/hw/hfi1/hfi.h
@@ -1084,7 +1084,6 @@ struct hfi1_devdata {
 	struct cdev user_cdev;
 	struct cdev diag_cdev;
 	struct cdev ui_cdev;
-	struct device *user_device;
 	struct device *diag_device;
 	struct device *ui_device;
 
diff --git a/drivers/infiniband/hw/hfi1/init.c b/drivers/infiniband/hw/hfi1/init.c
index 3759d92..3c605dd 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -1665,6 +1665,10 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	/* setup vnic */
 	hfi1_vnic_setup(dd);
+	
+	j = hfi1_device_create(dd);
+	if (j)
+		dd_dev_err(dd, "Failed to create /dev devices: %d\n", -j);
 
 	ret = hfi1_register_ib_device(dd);
 
@@ -1680,10 +1684,6 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		hfi1_dbg_ibdev_init(&dd->verbs_dev);
 	}
 
-	j = hfi1_device_create(dd);
-	if (j)
-		dd_dev_err(dd, "Failed to create /dev devices: %d\n", -j);
-
 	if (initfail || ret) {
 		msix_clean_up_interrupts(dd);
 		stop_timers(dd);
@@ -1700,11 +1700,11 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 				ppd->link_wq = NULL;
 			}
 		}
-		if (!j)
-			hfi1_device_remove(dd);
 		if (!ret)
 			hfi1_unregister_ib_device(dd);
 		hfi1_vnic_cleanup(dd);
+		if (!j)
+			hfi1_device_remove(dd);
 		postinit_cleanup(dd);
 		if (initfail)
 			ret = initfail;
@@ -1740,9 +1740,6 @@ static void remove_one(struct pci_dev *pdev)
 	/* close debugfs files before ib unregister */
 	hfi1_dbg_ibdev_exit(&dd->verbs_dev);
 
-	/* remove the /dev hfi1 interface */
-	hfi1_device_remove(dd);
-
 	/* wait for existing user space clients to finish */
 	wait_for_clients(dd);
 
@@ -1751,6 +1748,9 @@ static void remove_one(struct pci_dev *pdev)
 
 	/* cleanup vnic */
 	hfi1_vnic_cleanup(dd);
+	
+	/* remove the /dev hfi1 interface */
+	hfi1_device_remove(dd);
 
 	/*
 	 * Disable the IB link, disable interrupts on the device,

Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Jason Gunthorpe]

On Mon, Mar 16, 2020 at 05:05:07PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <kaike.wan@intel.com>
> 
> This patch is implemented to address the concerns raised in:
>   https://marc.info/?l=linux-rdma&m=158101337614772&w=2
> 
> The hfi1 driver dynammically allocates a struct device to represent the
> cdev in sysfs and devtmpfs (/dev/hfi1_x). On the other hand, the
> hfi1_devdata already contains a struct device in its ibdev field
> (hfi1_devdata.verbs_dev.rdi.ibdev.dev), and it is therefore possible to
> eliminate the dynamical allocation when creating the cdev. Since each
> device could be added to the sysfs only once and the function
> device_add() is already called for the ibdev in ib_register_device(),
> the function cdev_device_add() could not be used to create the cdev,
> even though the hfi1_devdata contains both cdev and ibdev in the same
> structure.
> 
> This patch eliminates the dynamic allocation by creating the cdev
> first, setting up the ibdev, and then calling the ib_register_device()
> to add the device to sysfs and devtmpfs.

What do the sysfs paths for the cdev look like now?

Jason

RE: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Wan, Kaike]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, March 18, 2020 9:32 AM
> To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; Marciniszyn, Mike
> <mike.marciniszyn@intel.com>; Wan, Kaike <kaike.wan@intel.com>
> Subject: Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as
> the parent of cdev
> 
> On Mon, Mar 16, 2020 at 05:05:07PM -0400, Dennis Dalessandro wrote:
> > From: Kaike Wan <kaike.wan@intel.com>
> >
> > This patch is implemented to address the concerns raised in:
> >   https://marc.info/?l=linux-rdma&m=158101337614772&w=2
> >
> > The hfi1 driver dynammically allocates a struct device to represent
> > the cdev in sysfs and devtmpfs (/dev/hfi1_x). On the other hand, the
> > hfi1_devdata already contains a struct device in its ibdev field
> > (hfi1_devdata.verbs_dev.rdi.ibdev.dev), and it is therefore possible
> > to eliminate the dynamical allocation when creating the cdev. Since
> > each device could be added to the sysfs only once and the function
> > device_add() is already called for the ibdev in ib_register_device(),
> > the function cdev_device_add() could not be used to create the cdev,
> > even though the hfi1_devdata contains both cdev and ibdev in the same
> > structure.
> >
> > This patch eliminates the dynamic allocation by creating the cdev
> > first, setting up the ibdev, and then calling the ib_register_device()
> > to add the device to sysfs and devtmpfs.
> 
> What do the sysfs paths for the cdev look like now?

ls -l /sys/dev/char/243:0
lrwxrwxrwx 1 root root 0 Mar 15 14:30 /sys/dev/char/243:0 -> ../../devices/pci0000:00/0000:00:02.0/0000:02:00.0/infiniband/hfi1_0

It points back to the IB device (hfi1_0 ).

Before this change, it pointed back to a virtual device:

ls /sys/dev/char/243:0 -l
lrwxrwxrwx 1 root root 0 Mar 18 11:52 /sys/dev/char/243:0 -> ../../devices/virtual/hfi1_user/hfi1_0


Kaike
> 
> Jason

Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Jason Gunthorpe]

On Wed, Mar 18, 2020 at 04:02:42PM +0000, Wan, Kaike wrote:
> 
> 
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Wednesday, March 18, 2020 9:32 AM
> > To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> > Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; Marciniszyn, Mike
> > <mike.marciniszyn@intel.com>; Wan, Kaike <kaike.wan@intel.com>
> > Subject: Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as
> > the parent of cdev
> > 
> > On Mon, Mar 16, 2020 at 05:05:07PM -0400, Dennis Dalessandro wrote:
> > > From: Kaike Wan <kaike.wan@intel.com>
> > >
> > > This patch is implemented to address the concerns raised in:
> > >   https://marc.info/?l=linux-rdma&m=158101337614772&w=2
> > >
> > > The hfi1 driver dynammically allocates a struct device to represent
> > > the cdev in sysfs and devtmpfs (/dev/hfi1_x). On the other hand, the
> > > hfi1_devdata already contains a struct device in its ibdev field
> > > (hfi1_devdata.verbs_dev.rdi.ibdev.dev), and it is therefore possible
> > > to eliminate the dynamical allocation when creating the cdev. Since
> > > each device could be added to the sysfs only once and the function
> > > device_add() is already called for the ibdev in ib_register_device(),
> > > the function cdev_device_add() could not be used to create the cdev,
> > > even though the hfi1_devdata contains both cdev and ibdev in the same
> > > structure.
> > >
> > > This patch eliminates the dynamic allocation by creating the cdev
> > > first, setting up the ibdev, and then calling the ib_register_device()
> > > to add the device to sysfs and devtmpfs.
> > 
> > What do the sysfs paths for the cdev look like now?
> 
> ls -l /sys/dev/char/243:0
> lrwxrwxrwx 1 root root 0 Mar 15 14:30 /sys/dev/char/243:0 -> ../../devices/pci0000:00/0000:00:02.0/0000:02:00.0/infiniband/hfi1_0
> 
> It points back to the IB device (hfi1_0 ).
> 
> Before this change, it pointed back to a virtual device:
> 
> ls /sys/dev/char/243:0 -l
> lrwxrwxrwx 1 root root 0 Mar 18 11:52 /sys/dev/char/243:0 -> ../../devices/virtual/hfi1_user/hfi1_0

Great, yes this looks right to me

So this came up due to PSM having problems.. The right way for PSM
to work is now to find the hfi1_0 devices under /sys/class/hfi1_user/*
and then map then back to RDMA devices and the physical card by doing
realpath and learning the
'/sys/pci0000:00/0000:00:02.0/0000:02:00.0/infiniband/XXX/' path

Yes?

Jason

RE: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Wan, Kaike]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, March 18, 2020 12:35 PM
> To: Wan, Kaike <kaike.wan@intel.com>
> Cc: Dalessandro, Dennis <dennis.dalessandro@intel.com>;
> dledford@redhat.com; linux-rdma@vger.kernel.org; Marciniszyn, Mike
> <mike.marciniszyn@intel.com>
> Subject: Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as
> the parent of cdev
> 

> > > >
> > > > The hfi1 driver dynammically allocates a struct device to
> > > > represent the cdev in sysfs and devtmpfs (/dev/hfi1_x). On the
> > > > other hand, the hfi1_devdata already contains a struct device in
> > > > its ibdev field (hfi1_devdata.verbs_dev.rdi.ibdev.dev), and it is
> > > > therefore possible to eliminate the dynamical allocation when
> > > > creating the cdev. Since each device could be added to the sysfs
> > > > only once and the function
> > > > device_add() is already called for the ibdev in
> > > > ib_register_device(), the function cdev_device_add() could not be
> > > > used to create the cdev, even though the hfi1_devdata contains
> > > > both cdev and ibdev in the same structure.
> > > >
> > > > This patch eliminates the dynamic allocation by creating the cdev
> > > > first, setting up the ibdev, and then calling the
> > > > ib_register_device() to add the device to sysfs and devtmpfs.
> > >
> > > What do the sysfs paths for the cdev look like now?
> >
> > ls -l /sys/dev/char/243:0
> > lrwxrwxrwx 1 root root 0 Mar 15 14:30 /sys/dev/char/243:0 ->
> > ../../devices/pci0000:00/0000:00:02.0/0000:02:00.0/infiniband/hfi1_0
> >
> > It points back to the IB device (hfi1_0 ).
> >
> > Before this change, it pointed back to a virtual device:
> >
> > ls /sys/dev/char/243:0 -l
> > lrwxrwxrwx 1 root root 0 Mar 18 11:52 /sys/dev/char/243:0 ->
> > ../../devices/virtual/hfi1_user/hfi1_0
> 
> Great, yes this looks right to me
> 
> So this came up due to PSM having problems.. The right way for PSM to work
> is now to find the hfi1_0 devices under /sys/class/hfi1_user/* and then map
> then back to RDMA devices and the physical card by doing realpath and
> learning the '/sys/pci0000:00/0000:00:02.0/0000:02:00.0/infiniband/XXX/'
> path
> 
> Yes?
That is certainly one way to get the device info.

Kaike

Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Jason Gunthorpe]

On Mon, Mar 16, 2020 at 05:05:07PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <kaike.wan@intel.com>
> 
> This patch is implemented to address the concerns raised in:
>   https://marc.info/?l=linux-rdma&m=158101337614772&w=2
> 
> The hfi1 driver dynammically allocates a struct device to represent the
> cdev in sysfs and devtmpfs (/dev/hfi1_x). On the other hand, the
> hfi1_devdata already contains a struct device in its ibdev field
> (hfi1_devdata.verbs_dev.rdi.ibdev.dev), and it is therefore possible to
> eliminate the dynamical allocation when creating the cdev. Since each
> device could be added to the sysfs only once and the function
> device_add() is already called for the ibdev in ib_register_device(),
> the function cdev_device_add() could not be used to create the cdev,
> even though the hfi1_devdata contains both cdev and ibdev in the same
> structure.
> 
> This patch eliminates the dynamic allocation by creating the cdev
> first, setting up the ibdev, and then calling the ib_register_device()
> to add the device to sysfs and devtmpfs.
> 
> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> Signed-off-by: Kaike Wan <kaike.wan@intel.com>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
>  drivers/infiniband/hw/hfi1/device.c   |   23 ++++++++++++++++-------
>  drivers/infiniband/hw/hfi1/file_ops.c |    5 ++---
>  drivers/infiniband/hw/hfi1/hfi.h      |    1 -
>  drivers/infiniband/hw/hfi1/init.c     |   18 +++++++++---------
>  4 files changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/device.c b/drivers/infiniband/hw/hfi1/device.c
> index bbb6069..4e1ad5f 100644
> +++ b/drivers/infiniband/hw/hfi1/device.c
> @@ -79,10 +79,12 @@ int hfi1_cdev_init(int minor, const char *name,
>  		goto done;
>  	}
>  
> -	if (user_accessible)
> -		device = device_create(user_class, NULL, dev, NULL, "%s", name);
> -	else
> +	if (user_accessible) {
> +		device = kobj_to_dev(parent);
> +		device->devt = dev;

What is this doing?

The only caller passes:

parent == &dd->verbs_dev.rdi.ibdev.dev.kobj

So,

 1) the kobj_to_dev is obfuscation
 2) WTF? Why is it changing the devt inside a struct ib_device??

> +	} else {
>  		device = device_create(class, NULL, dev, NULL, "%s", name);
> +	}

And since there is only one caller and user_accessible == true, this
confusing code is dead, please clean this up.

Actually this whole thing would be a lot less confusing to read if this
function was just lifted into user_add().

>  
>  	if (IS_ERR(device)) {
>  		ret = PTR_ERR(device);
> @@ -92,20 +94,27 @@ int hfi1_cdev_init(int minor, const char *name,
>  		cdev_del(cdev);
>  	}
>  done:
> -	*devp = device;
> +	if (devp)
> +		*devp = device;
>  	return ret;
>  }
>  
> +/*
> + * The pointer devp will be provided only if *devp is allocated
> + * dynamically, as shown in device_create().
> + */

And the only caller passes NULL:

drivers/infiniband/hw/hfi1/file_ops.c:  hfi1_cdev_cleanup(&dd->user_cdev, NULL);

So why add this comment and obfuscation? Delete this function and call
cdev_del from the only call side

And even user_add /user_remove are only called from one place, so spaghetti

> diff --git a/drivers/infiniband/hw/hfi1/hfi.h b/drivers/infiniband/hw/hfi1/hfi.h
> index b06c259..8e63b11 100644
> +++ b/drivers/infiniband/hw/hfi1/hfi.h
> @@ -1084,7 +1084,6 @@ struct hfi1_devdata {
>  	struct cdev user_cdev;
>  	struct cdev diag_cdev;
>  	struct cdev ui_cdev;
> -	struct device *user_device;
>  	struct device *diag_device;
>  	struct device *ui_device;

And I wondered about these other cdevs.. but this is all some kind of
dead code too, please fix it as well.

..

And I'm looking at some of the existing code around the cdev and
wondering how on earth does it even work?

Why is it calling kobject_set_name()? Look in
Documentation/kobject.txt. This function isn't supposed to be used.

Shouldn't there be a struct device to anchor this in sysfs? I'm very
confused how this is working, where did the hif1_xx sysfs directory come
from?

Jason

Re: [PATCH for-next 0/3] Clean up and improvements for 5.7

[Jason Gunthorpe]

On Mon, Mar 16, 2020 at 05:04:47PM -0400, Dennis Dalessandro wrote:
> Here are some clean up/improvement patches. Mike got rid of dead code and Kaike
> took a stab at fixing the kobj and cdev complaints.  This serious should
> go before the AIP posting. I'll be posting a v2 of the AIP code soon, and I
> think I still owe a response on one issue. Coming up, but for now it's just
> these 3.
> 
> 
> Kaike Wan (2):
>       IB/hfi1: Remove kobj from hfi1_devdata
> 
> Mike Marciniszyn (1):
>       IB/rdmavt: Delete unused routine

I'm going to take these two to for-next

>       IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

This one is getting closer to the right idea but needs a lot more work

Thanks,
Jason

RE: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Wan, Kaike]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, March 18, 2020 7:19 PM
> To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; Marciniszyn, Mike
> <mike.marciniszyn@intel.com>; Wan, Kaike <kaike.wan@intel.com>
> Subject: Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as
> the parent of cdev
> 
> > diff --git a/drivers/infiniband/hw/hfi1/device.c
> > b/drivers/infiniband/hw/hfi1/device.c
> > index bbb6069..4e1ad5f 100644
> > +++ b/drivers/infiniband/hw/hfi1/device.c
> > @@ -79,10 +79,12 @@ int hfi1_cdev_init(int minor, const char *name,
> >  		goto done;
> >  	}
> >
> > -	if (user_accessible)
> > -		device = device_create(user_class, NULL, dev, NULL, "%s",
> name);
> > -	else
> > +	if (user_accessible) {
> > +		device = kobj_to_dev(parent);
> > +		device->devt = dev;
> 
> What is this doing?
> 
> The only caller passes:
> 
> parent == &dd->verbs_dev.rdi.ibdev.dev.kobj
> 
> So,
> 
>  1) the kobj_to_dev is obfuscation
>  2) WTF? Why is it changing the devt inside a struct ib_device??
> 
> > +	} else {
> >  		device = device_create(class, NULL, dev, NULL, "%s", name);
> > +	}
> 
> And since there is only one caller and user_accessible == true, this confusing
> code is dead, please clean this up.
> 
> Actually this whole thing would be a lot less confusing to read if this function
> was just lifted into user_add().
Will clean it up.

> 
> >
> >  	if (IS_ERR(device)) {
> >  		ret = PTR_ERR(device);
> > @@ -92,20 +94,27 @@ int hfi1_cdev_init(int minor, const char *name,
> >  		cdev_del(cdev);
> >  	}
> >  done:
> > -	*devp = device;
> > +	if (devp)
> > +		*devp = device;
> >  	return ret;
> >  }
> >
> > +/*
> > + * The pointer devp will be provided only if *devp is allocated
> > + * dynamically, as shown in device_create().
> > + */
> 
> And the only caller passes NULL:
> 
> drivers/infiniband/hw/hfi1/file_ops.c:  hfi1_cdev_cleanup(&dd->user_cdev,
> NULL);
> 
> So why add this comment and obfuscation? Delete this function and call
> cdev_del from the only call side
> 
> And even user_add /user_remove are only called from one place, so
> spaghetti
> 
> > diff --git a/drivers/infiniband/hw/hfi1/hfi.h
> > b/drivers/infiniband/hw/hfi1/hfi.h
> > index b06c259..8e63b11 100644
> > +++ b/drivers/infiniband/hw/hfi1/hfi.h
> > @@ -1084,7 +1084,6 @@ struct hfi1_devdata {
> >  	struct cdev user_cdev;
> >  	struct cdev diag_cdev;
> >  	struct cdev ui_cdev;
> > -	struct device *user_device;
> >  	struct device *diag_device;
> >  	struct device *ui_device;
> 
> And I wondered about these other cdevs.. but this is all some kind of dead
> code too, please fix it as well.
Will do.

RE: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Wan, Kaike]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, March 18, 2020 7:19 PM
> To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; Marciniszyn, Mike
> <mike.marciniszyn@intel.com>; Wan, Kaike <kaike.wan@intel.com>
> Subject: Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as
> the parent of cdev
> 
> On Mon, Mar 16, 2020 at 05:05:07PM -0400, Dennis Dalessandro wrote:
> > From: Kaike Wan <kaike.wan@intel.com>
> >
> > This patch is implemented to address the concerns raised in:
> >   https://marc.info/?l=linux-rdma&m=158101337614772&w=2
> >
> > The hfi1 driver dynammically allocates a struct device to represent
> > the cdev in sysfs and devtmpfs (/dev/hfi1_x). On the other hand, the
> > hfi1_devdata already contains a struct device in its ibdev field
> > (hfi1_devdata.verbs_dev.rdi.ibdev.dev), and it is therefore possible
> > to eliminate the dynamical allocation when creating the cdev. Since
> > each device could be added to the sysfs only once and the function
> > device_add() is already called for the ibdev in ib_register_device(),
> > the function cdev_device_add() could not be used to create the cdev,
> > even though the hfi1_devdata contains both cdev and ibdev in the same
> > structure.
> >
> > This patch eliminates the dynamic allocation by creating the cdev
> > first, setting up the ibdev, and then calling the ib_register_device()
> > to add the device to sysfs and devtmpfs.
> >
> > Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
> > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> > Signed-off-by: Kaike Wan <kaike.wan@intel.com>
> > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> >  drivers/infiniband/hw/hfi1/device.c   |   23 ++++++++++++++++-------
> >  drivers/infiniband/hw/hfi1/file_ops.c |    5 ++---
> >  drivers/infiniband/hw/hfi1/hfi.h      |    1 -
> >  drivers/infiniband/hw/hfi1/init.c     |   18 +++++++++---------
> >  4 files changed, 27 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/hfi1/device.c
> > b/drivers/infiniband/hw/hfi1/device.c
> > index bbb6069..4e1ad5f 100644
> > +++ b/drivers/infiniband/hw/hfi1/device.c
> > @@ -79,10 +79,12 @@ int hfi1_cdev_init(int minor, const char *name,
> >  		goto done;
> >  	}
> >
> > -	if (user_accessible)
> > -		device = device_create(user_class, NULL, dev, NULL, "%s",
> name);
> > -	else
> > +	if (user_accessible) {
> > +		device = kobj_to_dev(parent);
> > +		device->devt = dev;
> 
> What is this doing?
> 
> The only caller passes:
> 
> parent == &dd->verbs_dev.rdi.ibdev.dev.kobj
> 
> So,
> 
>  1) the kobj_to_dev is obfuscation
Will be fixed.

>  2) WTF? Why is it changing the devt inside a struct ib_device??
This is needed to create /dev/hfi1_xxx. See below.

> 
> And I'm looking at some of the existing code around the cdev and wondering
> how on earth does it even work?
> 
> Why is it calling kobject_set_name()? Look in Documentation/kobject.txt.
> This function isn't supposed to be used.
There is no need to set the kobject name in cdev. Will be removed.
> 
> Shouldn't there be a struct device to anchor this in sysfs? I'm very confused
> how this is working, where did the hif1_xx sysfs directory come from?
Yes, ib_device is the struct device the cdev is anchored to. All we do here is to imitate what is done in cdev_device_add(), as suggested by you previously. The cdev and ib_device are in the same hfi1_devdata structure and normally we should use cdev_device_add() to add the cdev to the system. However, due to the fact that ib_register_device() calls device_add() to add the ib_device to the system, we can't call
cdev_device_add() (which also calls device_add()) directly. Instead, we have to set devt inside ib_device first, call cdev_set_parent() and cdev_add(), and eventually call ib_register_device().

If this is not desirable, we could keep the current approach to create the struct device dynamically through device_create(). In that case, all we need to do is to clean up the code. Which one do you prefer?

Kaike

Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Jason Gunthorpe]

On Fri, Mar 20, 2020 at 04:09:36PM +0000, Wan, Kaike wrote:

> >  2) WTF? Why is it changing the devt inside a struct ib_device??
> This is needed to create /dev/hfi1_xxx. See below.

Well, you can't do this, that belongs to the ib_device, not the
driver.
  
> > Why is it calling kobject_set_name()? Look in Documentation/kobject.txt.
> > This function isn't supposed to be used.
> There is no need to set the kobject name in cdev. Will be removed.

So the hfi1_0 name is actually the name of the ib device? But that
makes no sense, now the name of the char dev is not going to be stable
in sysfs after the ib_device is renamed.

> > Shouldn't there be a struct device to anchor this in sysfs? I'm very confused
> > how this is working, where did the hif1_xx sysfs directory come
> > from?

> Yes, ib_device is the struct device the cdev is anchored to. All we
> do here is to imitate what is done in cdev_device_add(), as
> suggested by you previously. 

I said to use cdev_device_add because that is the right thing to do.

> If this is not desirable, we could keep the current approach to
> create the struct device dynamically through device_create(). In
> that case, all we need to do is to clean up the code. Which one do
> you prefer?

The issue here was parentage. There should not be a virtual device
involved.

The hfi1 user_class device should be parented to the ib_device, look
at how things like umad work to do this properly.

And of course refcounting is tricky, so the cdev and this other device
must be in the same struct, again refer to umad.

Jason

RE: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Wan, Kaike]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, March 20, 2020 12:32 PM
> To: Wan, Kaike <kaike.wan@intel.com>
> Cc: Dalessandro, Dennis <dennis.dalessandro@intel.com>;
> dledford@redhat.com; linux-rdma@vger.kernel.org; Marciniszyn, Mike
> <mike.marciniszyn@intel.com>
> Subject: Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as
> the parent of cdev
> 
> On Fri, Mar 20, 2020 at 04:09:36PM +0000, Wan, Kaike wrote:
> 
> > >  2) WTF? Why is it changing the devt inside a struct ib_device??
> > This is needed to create /dev/hfi1_xxx. See below.
> 
> Well, you can't do this, that belongs to the ib_device, not the driver.
> 
> > > Why is it calling kobject_set_name()? Look in Documentation/kobject.txt.
> > > This function isn't supposed to be used.
> > There is no need to set the kobject name in cdev. Will be removed.
> 
> So the hfi1_0 name is actually the name of the ib device? But that makes no
> sense, now the name of the char dev is not going to be stable in sysfs after
> the ib_device is renamed.
> 
> > > Shouldn't there be a struct device to anchor this in sysfs? I'm very
> > > confused how this is working, where did the hif1_xx sysfs directory
> > > come from?
> 
> > Yes, ib_device is the struct device the cdev is anchored to. All we do
> > here is to imitate what is done in cdev_device_add(), as suggested by
> > you previously.
> 
> I said to use cdev_device_add because that is the right thing to do.
> 
> > If this is not desirable, we could keep the current approach to create
> > the struct device dynamically through device_create(). In that case,
> > all we need to do is to clean up the code. Which one do you prefer?
> 
> The issue here was parentage. There should not be a virtual device involved.
> 
> The hfi1 user_class device should be parented to the ib_device, look at how
> things like umad work to do this properly.
So all we need to do is:
-- Change user_device from struct device * to struct device in hfi1_devdata;
-- Set up dd->user_device properly including setting its parent to ib_device;
-- call cdev_device_all().

Correct?

Kaike

Re: [PATCH for-next 3/3] IB/hfi1: Use the ibdev in hfi1_devdata as the parent of cdev

[Jason Gunthorpe]

On Fri, Mar 20, 2020 at 05:30:40PM +0000, Wan, Kaike wrote:

> > > If this is not desirable, we could keep the current approach to create
> > > the struct device dynamically through device_create(). In that case,
> > > all we need to do is to clean up the code. Which one do you prefer?
> > 
> > The issue here was parentage. There should not be a virtual device involved.
> > 
> > The hfi1 user_class device should be parented to the ib_device, look at how
> > things like umad work to do this properly.
> So all we need to do is:
> -- Change user_device from struct device * to struct device in hfi1_devdata;
> -- Set up dd->user_device properly including setting its parent to ib_device;
> -- call cdev_device_all().

Yes, but keep in mind that putting multiple krefs inside the same
structure is very tricky - be sure to do it right.

Jason