> -----Original Message-----
> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
> owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe
> Sent: Tuesday, December 18, 2018 9:49 AM
> To: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: 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 02:15:52PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > The sysfs layout is created by CM incorrectly presented RDMA devices
> > with InfiniBand link layer. Layout of such devices represents device
> > tree of connections. It means that the following issues are fixed by
> > the this change:
> > * Symlink name - It used device name instead of specific identifier.
> > * Target location - It was supposed to point to PCI-ID/infiniband_cm/
> > instead of PCI-ID/infiniband/
> > * Target name - It created extra device file under already existing
> > device folder, e.g. mlx5_0/mlx5_0
> >
> > Fix it by changing sysfs device CM creation logic.
> >
> > Before the change:
> > [leonro@server ~]$ ls -l /sys/class/infiniband_cm/ lrwxrwxrwx 1 root
> > root 0 Dec 14 10:31 mlx5_0 ->
> > ../../devices/pci0000:00/0000:00:0c.0/infiniband/mlx5_0/mlx5_0
> >
> > After the change:
> > [leonro@server ~]$ ls -l /sys/class/infiniband_cm/ lrwxrwxrwx 1 root
> > root 0 Dec 14 10:47 cm0 ->
> > ../../devices/pci0000:00/0000:00:0c.0/infiniband_cm/cm0
> >
> > At the end, we will have same logic and representation as for uverbs, mad
> and issm sysfs classes.
>
> These other ones actually tie to chardevs, this does not.
>
> > Fixes: 110cf374a809 ("infiniband: make cm_device use a struct device
> > and not a kobject.")
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> I think that it
> > should be forwarded to stable@ too.
> > drivers/infiniband/core/cm.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/cm.c
> > b/drivers/infiniband/core/cm.c index edb2cb758be7..e3bee04fb641 100644
> > +++ b/drivers/infiniband/core/cm.c
> > @@ -93,6 +93,8 @@ static const char * const ibcm_rej_reason_strs[] = {
> > [IB_CM_REJ_INVALID_ALT_FLOW_LABEL] = "invalid alt flow
> label",
> > };
> >
> > +static DEFINE_IDA(cm_ida);
> > +
> > const char *__attribute_const__ ibcm_reject_msg(int reason) {
> > size_t index = reason;
> > @@ -220,6 +222,7 @@ struct cm_device {
> > struct list_head list;
> > struct ib_device *ib_device;
> > struct device *device;
> > + int devnum;
> > u8 ack_delay;
> > int going_down;
> > struct cm_port *port[0];
> > @@ -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.
I planned to do it differently to avoid breaking such users by keeping it in infiniband_cm, but since such change doesn't break any functionality, its better we make it right now to anchor at ib_dev' port's kobject.
If there is agreement, I can probably do post these 3 series, as this will likely have code conflicts with those 3 series in works.
> Jason
