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

Commit Message

Dennis Dalessandro March 16, 2020, 9:05 p.m. UTC

From: Kaike Wan <[e-mail]>

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 <[e-mail]>
Reviewed-by: Dennis Dalessandro <[e-mail]>
Signed-off-by: Kaike Wan <[e-mail]>
Signed-off-by: Dennis Dalessandro <[e-mail]>
---
 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(-)

Comments

Jason Gunthorpe March 18, 2020, 1:31 p.m. UTC | #1

On Mon, Mar 16, 2020 at 05:05:07PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <[e-mail]>
> 
> 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

Wan, Kaike March 18, 2020, 4:02 p.m. UTC | #2

> -----Original Message-----
> From: Jason Gunthorpe <[e-mail]>
> Sent: Wednesday, March 18, 2020 9:32 AM
> To: Dalessandro, Dennis <[e-mail]>
> Cc: [e-mail]; [e-mail]; Marciniszyn, Mike
> <[e-mail]>; Wan, Kaike <[e-mail]>
> 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 <[e-mail]>
> >
> > 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

Jason Gunthorpe March 18, 2020, 4:34 p.m. UTC | #3

On Wed, Mar 18, 2020 at 04:02:42PM +0000, Wan, Kaike wrote:
> 
> 
> > From: Jason Gunthorpe <[e-mail]>
> > Sent: Wednesday, March 18, 2020 9:32 AM
> > To: Dalessandro, Dennis <[e-mail]>
> > Cc: [e-mail]; [e-mail]; Marciniszyn, Mike
> > <[e-mail]>; Wan, Kaike <[e-mail]>
> > 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 <[e-mail]>
> > >
> > > 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

Wan, Kaike March 18, 2020, 5:13 p.m. UTC | #4

> -----Original Message-----
> From: Jason Gunthorpe <[e-mail]>
> Sent: Wednesday, March 18, 2020 12:35 PM
> To: Wan, Kaike <[e-mail]>
> Cc: Dalessandro, Dennis <[e-mail]>;
> [e-mail]; [e-mail]; Marciniszyn, Mike
> <[e-mail]>
> 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

Jason Gunthorpe March 18, 2020, 11:18 p.m. UTC | #5

On Mon, Mar 16, 2020 at 05:05:07PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <[e-mail]>
> 
> 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 <[e-mail]>
> Reviewed-by: Dennis Dalessandro <[e-mail]>
> Signed-off-by: Kaike Wan <[e-mail]>
> Signed-off-by: Dennis Dalessandro <[e-mail]>
>  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

Wan, Kaike March 20, 2020, 12:19 p.m. UTC | #6

> -----Original Message-----
> From: Jason Gunthorpe <[e-mail]>
> Sent: Wednesday, March 18, 2020 7:19 PM
> To: Dalessandro, Dennis <[e-mail]>
> Cc: [e-mail]; [e-mail]; Marciniszyn, Mike
> <[e-mail]>; Wan, Kaike <[e-mail]>
> 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.

Wan, Kaike March 20, 2020, 4:09 p.m. UTC | #7

> -----Original Message-----
> From: Jason Gunthorpe <[e-mail]>
> Sent: Wednesday, March 18, 2020 7:19 PM
> To: Dalessandro, Dennis <[e-mail]>
> Cc: [e-mail]; [e-mail]; Marciniszyn, Mike
> <[e-mail]>; Wan, Kaike <[e-mail]>
> 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 <[e-mail]>
> >
> > 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 <[e-mail]>
> > Reviewed-by: Dennis Dalessandro <[e-mail]>
> > Signed-off-by: Kaike Wan <[e-mail]>
> > Signed-off-by: Dennis Dalessandro <[e-mail]>
> >  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

Jason Gunthorpe March 20, 2020, 4:32 p.m. UTC | #8

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

Wan, Kaike March 20, 2020, 5:30 p.m. UTC | #9

> -----Original Message-----
> From: Jason Gunthorpe <[e-mail]>
> Sent: Friday, March 20, 2020 12:32 PM
> To: Wan, Kaike <[e-mail]>
> Cc: Dalessandro, Dennis <[e-mail]>;
> [e-mail]; [e-mail]; Marciniszyn, Mike
> <[e-mail]>
> 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

Jason Gunthorpe March 20, 2020, 5:33 p.m. UTC | #10

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

Patch

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,