Re: [PATCH for-next] RDMA/core: Assign the name of device when allocating ib_device

[Leon Romanovsky <leon@kernel.org>]

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

>
> >>
> >> Thanks
> >> Weihang
> >
>