On 2020/4/28 1:56, Saleem, Shiraz wrote: >> Subject: [PATCH for-next] RDMA/core: Assign the name of device when allocating >> ib_device >> >> If the name of a device is assigned during ib_register_device(), some drivers have >> to use dev_*() for printing before register device. Bring >> assign_name() into ib_alloc_device(), so that drivers can use ibdev_*() anywhere. >> >> Signed-off-by: Weihang Li <liweihang@xxxxxxxxxx> >> --- >> drivers/infiniband/core/device.c | 85 +++++++++++++------------- >> drivers/infiniband/hw/bnxt_re/main.c | 4 +- >> drivers/infiniband/hw/cxgb4/device.c | 2 +- >> drivers/infiniband/hw/cxgb4/provider.c | 2 +- >> drivers/infiniband/hw/efa/efa_main.c | 4 +- >> drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 2 +- >> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 2 +- >> drivers/infiniband/hw/hns/hns_roce_main.c | 2 +- >> drivers/infiniband/hw/i40iw/i40iw_verbs.c | 4 +- >> drivers/infiniband/hw/mlx4/main.c | 4 +- >> drivers/infiniband/hw/mlx5/ib_rep.c | 8 ++- >> drivers/infiniband/hw/mlx5/main.c | 18 +++--- >> drivers/infiniband/hw/mthca/mthca_main.c | 2 +- >> drivers/infiniband/hw/mthca/mthca_provider.c | 2 +- >> drivers/infiniband/hw/ocrdma/ocrdma_main.c | 4 +- >> drivers/infiniband/hw/qedr/main.c | 4 +- >> drivers/infiniband/hw/usnic/usnic_ib_main.c | 4 +- >> drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 4 +- >> drivers/infiniband/sw/rxe/rxe.c | 4 +- >> drivers/infiniband/sw/rxe/rxe.h | 2 +- >> drivers/infiniband/sw/rxe/rxe_net.c | 4 +- >> drivers/infiniband/sw/rxe/rxe_verbs.c | 4 +- >> drivers/infiniband/sw/rxe/rxe_verbs.h | 2 +- >> include/rdma/ib_verbs.h | 8 +-- >> 24 files changed, 95 insertions(+), 86 deletions(-) > > I think you ll need to update siw driver similarly. > > rvt_register_device should be adapted to use the revised device registration API. > hfi1/qib also need some rework. > rvt_alloc_device needs to be adapted for the new one-shot > name + device allocation scheme. > Hoping we can just use move the name setting from rvt_set_ibdev_name > Hi Shiraz, Sorry for missing hfi1/qib, I will try to modify as your comments. > A few more comments below. >> [...] > >> /** >> * _ib_alloc_device - allocate an IB device struct >> * @size:size of structure to allocate >> + * @name: unique string device name. This may include a '%' which will >> + * cause a unique index to be added to the passed device name. >> * >> * Low-level drivers should use ib_alloc_device() to allocate &struct >> * ib_device. @size is the size of the structure to be allocated, @@ -567,7 +603,7 >> @@ static void rdma_init_coredev(struct ib_core_device *coredev, >> * ib_dealloc_device() must be used to free structures allocated with >> * ib_alloc_device(). >> */ >> -struct ib_device *_ib_alloc_device(size_t size) >> +struct ib_device *_ib_alloc_device(size_t size, const char *name) >> { >> struct ib_device *device; >> >> @@ -586,6 +622,11 @@ struct ib_device *_ib_alloc_device(size_t size) >> device->groups[0] = &ib_dev_attr_group; >> rdma_init_coredev(&device->coredev, device, &init_net); >> >> + if (assign_name(device, name)) { >> + kfree(device); > Don't you need to do a rdma_restrack_clean here? > Yes, I think so. Thanks for your reminder. > >> + return NULL; >> + } >> + >> INIT_LIST_HEAD(&device->event_handler_list); >> spin_lock_init(&device->qp_open_list_lock); >> init_rwsem(&device->event_handler_rwsem); >> @@ -1132,40 +1173,6 @@ static __net_init int rdma_dev_init_net(struct net *net) >> return ret; >> } >> > > [...] > >> diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c >> b/drivers/infiniband/hw/i40iw/i40iw_verbs.c >> index 1b6fb13..ccb0d70 100644 >> --- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c >> +++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c >> @@ -2692,7 +2692,7 @@ static struct i40iw_ib_device >> *i40iw_init_rdma_device(struct i40iw_device *iwdev >> struct net_device *netdev = iwdev->netdev; >> struct pci_dev *pcidev = (struct pci_dev *)iwdev->hw.dev_context; >> >> - iwibdev = ib_alloc_device(i40iw_ib_device, ibdev); >> + iwibdev = ib_alloc_device(i40iw_ib_device, ibdev, "i40iw%d"); >> if (!iwibdev) { >> i40iw_pr_err("iwdev == NULL\n"); >> return NULL; >> @@ -2780,7 +2780,7 @@ int i40iw_register_rdma_device(struct i40iw_device >> *iwdev) >> if (ret) >> goto error; >> >> - ret = ib_register_device(&iwibdev->ibdev, "i40iw%d"); >> + ret = ib_register_device(&iwibdev->ibdev); >> if (ret) >> goto error; >> > > i40iw looks ok except for the missing underscore which I think was brought up already in another provider. > > Thanks for this work! > > Shiraz > OK, will add it. Thanks Weihang >