> -----Original Message----- > From: Jason Gunthorpe <jgg@xxxxxxxx> > Sent: Wednesday, March 18, 2020 7:19 PM > To: Dalessandro, Dennis <dennis.dalessandro@xxxxxxxxx> > Cc: dledford@xxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; Marciniszyn, Mike > <mike.marciniszyn@xxxxxxxxx>; Wan, Kaike <kaike.wan@xxxxxxxxx> > 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@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx> > > Signed-off-by: Kaike Wan <kaike.wan@xxxxxxxxx> > > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx> > > 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
