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