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