On 2020/4/29 16:37, Leon Romanovsky wrote: > On Tue, Apr 28, 2020 at 12:39:49PM +0000, liweihang wrote: >> On 2020/4/28 19:19, Leon Romanovsky wrote: >>> On Tue, Apr 28, 2020 at 08:00:29AM +0000, liweihang wrote: >>>> On 2020/4/27 20:03, Leon Romanovsky wrote: >>>>>>>> /** >>>>>>>> * _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 >>>>>>> It looks like all drivers are setting "%" in their name and "name" can >>>>>>> be changed to be "prefix". >>>>>> Does hfi? I thought the name was forced there for some port swapped >>>>>> reason? >>>>> This patch doesn't touch HFI, nothing prohibits from us to make this >>>>> conversion work for all drivers except HFI and for the HFI add some >>>>> different callback. There is no need to make API harder just because >>>>> one driver needs it. >>>>> >>>>> Thanks >>>>> >>>>>> Jason >>>> >>>> Hi Jason and Leon, >>>> >>>> I missed some codes related to assign_name() in this series including >>>> hfi/qib as Shiraz pointed. And I found a "name" without a "%" in following >>>> funtions in core/nldev.c, and ibdev_name will be used for rxe/siw later. >>>> >>>> static int nldev_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, >>>> struct netlink_ext_ack *extack) >>>> { >>>> ... >>>> >>>> nla_strlcpy(ibdev_name, tb[RDMA_NLDEV_ATTR_DEV_NAME], >>>> sizeof(ibdev_name)); >>>> if (strchr(ibdev_name, '%') || strlen(ibdev_name) == 0) >>>> return -EINVAL; >>>> >>>> ... >>>> } >>>> >>>> I'm not familiar with these codes, but I think the judgment in assign_name() >>>> is for the situaion like above. >>>> >>>> if (strchr(name, '%')) >>>> ret = alloc_name(device, name); >>>> else >>>> ret = dev_set_name(&device->dev, name); >>>> >>>> So is it a better idea to keep using "name" instead of "prefix"? >>> >>> nldev_newlink() doesn't call to ib_alloc_device() and alloc_name(). The >>> check pointed by you is for the user input. >>> >> >> Hi Leon, >> >> nldev_newlink() will call "ops->newlink(ibdev_name, ndev)", and it point to >> siw_newlink() in siw_main.c. And then it will call ib_alloc_device() and >> ib_register_device(). >> >> According to the code I pointed before, it seems that nldev_newlink() >> expects users to input a name without '%', and then passes this name >> to assign_name(). I think siw/rxe have to call ib_alloc_device() with >> a name without '%', so we can't treat it as a prefix and add "_%d" to >> it like for other drivers. > > The opposite is actually true. > > The reason why newlink checks for % is due to the expectation in > alloc_name() to have a name with % for numbered devices, which is > nice, but the better API will be to provide "prefix" and a flag > if to append an index or not. > > Thanks > I see, thank you. >> >>>> >>>> Thanks >>>> Weihang >>> >> >