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 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