> -----Original Message----- > From: Jason Gunthorpe > Sent: Tuesday, December 18, 2018 10:56 AM > To: Parav Pandit <parav@xxxxxxxxxxxx> > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Greg Kroah-Hartman > <gregkh@xxxxxxx> > Subject: Re: [PATCH rdma-next] IB/cm: Properly create sysfs layout of RDMA > devices with InfiniBand link layer > > On Tue, Dec 18, 2018 at 09:50:44AM -0700, Parav Pandit wrote: > > > > > > > From: Jason Gunthorpe > > > Sent: Tuesday, December 18, 2018 10:35 AM > > > To: Parav Pandit <parav@xxxxxxxxxxxx> > > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford > > > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>; > RDMA > > > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Greg Kroah-Hartman > > > <gregkh@xxxxxxx> > > > Subject: Re: [PATCH rdma-next] IB/cm: Properly create sysfs layout > > > of RDMA devices with InfiniBand link layer > > > > > > On Tue, Dec 18, 2018 at 09:32:16AM -0700, Parav Pandit wrote: > > > > > > > > > @@ -4368,13 +4371,12 @@ static void cm_add_one(struct > > > > > > ib_device > > > > > *ib_device) > > > > > > cm_dev->ib_device = ib_device; > > > > > > cm_dev->ack_delay = ib_device->attrs.local_ca_ack_delay; > > > > > > cm_dev->going_down = 0; > > > > > > - cm_dev->device = device_create(&cm_class, &ib_device- > >dev, > > > > > > + cm_dev->devnum = ida_alloc(&cm_ida, GFP_KERNEL); > > > > > > + cm_dev->device = device_create(&cm_class, > > > > > > +ib_device->dev.parent, > > > > > > MKDEV(0, 0), NULL, > > > > > > > > > > Yikes, this is horrifying, why is it creating a struct device at all like > this?? > > > > > > > > > > Probably the CM counter group debug sysfs should just be added > > > > > directly to the actual ib_dev's sysfs?? > > > > > > > > Yes. anchoring to ibdev's sysfs looks right way but that breaks > > > > the ABI/scripts who are accustomed to use it from infiniband_cm. > > > > > > But doesn't this patch rename those paths already? > > > > Yes, it breaks. I did raise this to Leon yesterday. But this patch > > solves some other problem... > > Well, if we decide to break it then lets break it and end up in a sane spot.. > Yes, I also think so. If we anchor at ibdev's sysfs, it handles rename also cleanly. > > > What paths do you think people are using? > > > > > cat /sys/class/infiniband_cm/mlx5_0/1/cm_tx_retries/req > > Could we put a symlink here perhaps - one that tracks rename maybe? If we put under, cat /sys/class/infiniband/mlx5_0/1/cm_tx_retries/req compare to previously, cat /sys/class/infiniband_cm/mlx5_0/1/cm_tx_retries/req It is not that bad. In current patch as end user, I need to figure out, which pci dev cm0 maps to. Then, I need to go through all ibdevices and find out matching pci dev, and map cm0 to that device. Of course, a script can be written... But /sys/class/infiniband/mlx5_0/1/cm_tx_retries/req seems a cleaner approach that doesn't require special scripts, and works with renaming nicely.
