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

[Jason Gunthorpe <jgg@ziepe.ca>]

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