> -----Original Message-----
> From: Jason Gunthorpe
> Sent: Tuesday, December 18, 2018 12:03 PM
> To: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
> <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>;
> Ira Weiny <ira.weiny@xxxxxxxxx>; Jack Morgenstein
> <jackm@xxxxxxxxxxxxxxxxxx>; Parav Pandit <parav@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next 4/6] IB/umad: Refactor code to use
> cdev_device_add()
>
> On Tue, Dec 18, 2018 at 02:15:46PM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav@xxxxxxxxxxxx>
> >
> > Refactor code to use cdev_device_add() and do other minor refactors
> > while modifying these functions as below.
> >
> > 1. Instead of returning generic -1, return an actual error for
> ib_umad_init_port().
> > 2. Introduce and use ib_umad_init_port_dev() for sm and umad char
> devices.
> > 3. Instead of kobj, use more light weight kref to refcount ib_umad_device.
> > 4. Use modern cdev_device_add() single code cut down three steps of
> > cdev_add(), device_create(). This further helps to move device sysfs
> > files to class attributes in subsequent patch.
> > 5. Remove few empty lines while refactoring these functions.
> > 6. Use sizeof() instead of sizeof to avoid checkpatch warning.
> > 7. Align ib_umad_port fields by tab while changing *dev to dev.
> >
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Reviewed-by: Jack Morgenstein <jackm@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/user_mad.c | 184 +++++++++++++++--------------
> > 1 file changed, 93 insertions(+), 91 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/user_mad.c
> > b/drivers/infiniband/core/user_mad.c
> > index 4558bf8b5ed5..732dd0dbb0dd 100644
> > +++ b/drivers/infiniband/core/user_mad.c
> > @@ -89,25 +89,24 @@ enum {
> > */
> >
> > struct ib_umad_port {
> > - struct cdev cdev;
> > - struct device *dev;
> > -
> > - struct cdev sm_cdev;
> > - struct device *sm_dev;
> > - struct semaphore sm_sem;
> > -
> > - struct mutex file_mutex;
> > - struct list_head file_list;
> > -
> > - struct ib_device *ib_dev;
> > - struct ib_umad_device *umad_dev;
> > - int dev_num;
> > - u8 port_num;
> > + struct cdev cdev;
> > + struct device dev;
> > + struct cdev sm_cdev;
> > + struct device sm_dev;
> > + struct semaphore sm_sem;
> > +
> > + struct mutex file_mutex;
> > + struct list_head file_list;
> > +
> > + struct ib_device *ib_dev;
> > + struct ib_umad_device *umad_dev;
> > + int dev_num;
> > + u8 port_num;
> > };
> >
> > struct ib_umad_device {
> > - struct kobject kobj;
> > - struct ib_umad_port port[0];
> > + struct kref kref;
> > + struct ib_umad_port ports[0];
> > };
>
> Yikes that is a lot of krefs in this one allocation..
>
> Drop the horizontal white space
>
Ok.
> Use c99 flex array syntax (ports[])
Ok.
>
> > +static void ib_umad_release_port(struct device *device) {
> > + /* device_release() requires minimum one release callback
> > + * per class or device. So have an empty one.
> > + */
> > +}
>
> Not empty.. the release function for the struct device should be a
> ib_umad_dev_put() on the containing ib_umad_device struct.
>
Ok.
> > +
> > +static void ib_umad_init_port_dev(struct device *dev,
> > + struct ib_umad_port *port,
> > + const struct ib_device *device) {
> > + device_initialize(dev);
> > + dev->class = &umad_class;
> > + dev->parent = device->dev.parent;
> > + dev_set_drvdata(dev, port);
>
> The drvdata should also be converted to use container_of and removed.
>
I looked at this during v0 version. Couldn't do it because both issm and umad devices have their ibdev and port files and they are part of single structure. So container_of() from the device cannot teach back to umad_device as offsets for two devices are different.
Creating another structure and storing umad_device structure pointer is not worth overhead.
So I couldn't find a simple way to do so.
> > @@ -1284,22 +1289,19 @@ static void ib_umad_add_one(struct ib_device
> *device)
> > s = rdma_start_port(device);
> > e = rdma_end_port(device);
> >
> > - umad_dev = kzalloc(sizeof *umad_dev +
> > + umad_dev = kzalloc(sizeof(*umad_dev) +
> > (e - s + 1) * sizeof (struct ib_umad_port),
> > GFP_KERNEL);
>
> This should be updated to be struct_size(umad_dev, ports, e - s + 1)
Ok.
