RE: [PATCH v2 rdma-core] irdma: Add ice and irdma to kernel-boot rules

["Nikolova, Tatyana E" <tatyana.e.nikolova@xxxxxxxxx>]

Hi Jason,

I know it has been a while since we discussed this. Based on your feedback, we are proposing another solution for the irdma kernel-boot rules. Could you please review it?

> > udevadm info --attribute-walk /sys/class/infiniband/rocep47s0f0
> >
> >   looking at device
> '/devices/pci0000:2e/0000:2e:00.0/0000:2f:00.0/infiniband/rocep47s0f0':
> 
> This looks like the problem. For any of this to work the infiniband 
> device needs to be parented to the aux device, not the PCI device.
> 
> mlx5 did not due this for backwards compat reasons, but this is a new 
> driver so it could do it properly.
> 
> Then you can use SUBSYSTEMS and so forth as I first suggested.

Here is a patch for irdma driver to set aux_dev as the ibdev parent:

diff --git a/drivers/infiniband/hw/irdma/i40iw_if.c b/drivers/infiniband/hw/irdma/i40iw_if.c
index 4053ead32416..e08fbfe148ea 100644
--- a/drivers/infiniband/hw/irdma/i40iw_if.c
+++ b/drivers/infiniband/hw/irdma/i40iw_if.c
@@ -90,6 +90,7 @@ static void i40iw_fill_device_info(struct irdma_device *iwdev, struct i40e_info
        iwdev->rcv_wnd = IRDMA_CM_DEFAULT_RCV_WND_SCALED;
        iwdev->rcv_wscale = IRDMA_CM_DEFAULT_RCV_WND_SCALE;
        iwdev->netdev = cdev_info->netdev;
+       iwdev->aux_dev = cdev_info->aux_dev;
        iwdev->vsi_num = 0;
 }
 
diff --git a/drivers/infiniband/hw/irdma/main.c b/drivers/infiniband/hw/irdma/main.c
index 514453777e07..835a087a3329 100644
--- a/drivers/infiniband/hw/irdma/main.c
+++ b/drivers/infiniband/hw/irdma/main.c
@@ -279,6 +279,7 @@ static int irdma_probe(struct auxiliary_device *aux_dev, const struct auxiliary_
        }
 
        irdma_fill_device_info(iwdev, pf, vsi);
+       iwdev->aux_dev = aux_dev;
        rf = iwdev->rf;
 
        err = irdma_ctrl_init_hw(rf);
diff --git a/drivers/infiniband/hw/irdma/main.h b/drivers/infiniband/hw/irdma/main.h
index 65e966ad3453..f2f86b882cef 100644
--- a/drivers/infiniband/hw/irdma/main.h
+++ b/drivers/infiniband/hw/irdma/main.h
@@ -329,6 +329,7 @@ struct irdma_device {
        struct ib_device ibdev;
        struct irdma_pci_f *rf;
        struct net_device *netdev;
+       struct auxiliary_device *aux_dev;
        struct workqueue_struct *cleanup_wq;
        struct irdma_sc_vsi vsi;
        struct irdma_cm_core cm_core;
diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index a22afbb25bc5..f1304b6b58b3 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -4549,7 +4549,6 @@ static int irdma_init_iw_device(struct irdma_device *iwdev)
  */
 static int irdma_init_rdma_device(struct irdma_device *iwdev)  {
-       struct pci_dev *pcidev = iwdev->rf->pcidev;
        int ret;
 
        if (iwdev->roce_mode) {
@@ -4561,7 +4560,7 @@ static int irdma_init_rdma_device(struct irdma_device *iwdev)
        }
        iwdev->ibdev.phys_port_cnt = 1;
        iwdev->ibdev.num_comp_vectors = iwdev->rf->ceqs_count;
-       iwdev->ibdev.dev.parent = &pcidev->dev;
+       iwdev->ibdev.dev.parent = &iwdev->aux_dev->dev;
        ib_set_device_ops(&iwdev->ibdev, &irdma_dev_ops);
 
        return 0;

Here is the udev output after this change:
    
    looking at device
    '/devices/pci0000:2e/0000:2e:00.0/0000:2f:00.0/ice.roce.0/infiniband/rocep47s0f0':
        KERNEL=="rocep47s0f0"
        SUBSYSTEM=="infiniband"
        DRIVER==""
        ATTR{fw_ver}=="1.48"
        ATTR{node_desc}==""
        ATTR{node_guid}=="6a05:caff:fec1:c790"
        ATTR{node_type}=="1: CA"
        ATTR{sys_image_guid}=="6a05:caff:fec1:c790"
    
      looking at parent device
    '/devices/pci0000:2e/0000:2e:00.0/0000:2f:00.0/ice.roce.0':
        KERNELS=="ice.roce.0"
        SUBSYSTEMS=="auxiliary"
        DRIVERS=="irdma"
    
      looking at parent device
    '/devices/pci0000:2e/0000:2e:00.0/0000:2f:00.0':
        KERNELS=="0000:2f:00.0"
        SUBSYSTEMS=="pci"
        DRIVERS=="ice" 

Having the aux_dev as a parent of the irdma ibdev allows for udev rules to set ID_RDMA_IWARP or ID_RDMA_ROCE environment vars based on the auxiliary device name with the following patch:

diff --git a/kernel-boot/rdma-description.rules b/kernel-boot/rdma-description.rules
index 48a7ced..0b844bd 100644
--- a/kernel-boot/rdma-description.rules
+++ b/kernel-boot/rdma-description.rules
@@ -29,6 +29,8 @@ DRIVERS=="i40e", ENV{ID_RDMA_IWARP}="1"
 DRIVERS=="be2net", ENV{ID_RDMA_ROCE}="1"
 DRIVERS=="bnxt_en", ENV{ID_RDMA_ROCE}="1"
 DRIVERS=="hns", ENV{ID_RDMA_ROCE}="1"
+DRIVERS=="irdma", KERNELS=="*iw*", ENV{ID_RDMA_IWARP}="1"
+DRIVERS=="irdma", KERNELS=="*roce*", ENV{ID_RDMA_ROCE}="1"
 DRIVERS=="mlx4_core", ENV{ID_RDMA_ROCE}="1"
 DRIVERS=="mlx5_core", ENV{ID_RDMA_ROCE}="1"
 DRIVERS=="qede", ENV{ID_RDMA_ROCE}="1"

In addition to this patch, changes to rdma-core rdma_rename logic are necessary to handle ib devices with aux_dev parent and rename them by_onboard or by_pci the same way as it did before introducing the aux_dev parent.
Please review rdma-core PR at https://github.com/linux-rdma/rdma-core/pull/1248

Thank you,
Tatyana