Re: [RFC PATCH v3 1/6] dpll: Add DPLL framework base functions

[Jiri Pirko <jiri@resnulli.us>]

Tue, Oct 11, 2022 at 11:23:38PM CEST, vfedorenko@novek.ru wrote:
>On 11.10.2022 09:32, Jiri Pirko wrote:
>> Mon, Oct 10, 2022 at 09:54:26PM CEST, vfedorenko@novek.ru wrote:
>> > On 10.10.2022 10:18, Jiri Pirko wrote:
>> > > Mon, Oct 10, 2022 at 03:17:59AM CEST, vfedorenko@novek.ru wrote:
>> > > > From: Vadim Fedorenko <vadfed@fb.com>
>> > > > 
>> > > > DPLL framework is used to represent and configure DPLL devices
>> > > > in systems. Each device that has DPLL and can configure sources
>> > > > and outputs can use this framework.
>> > > > 
>> > > > Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
>> > > > Co-developed-by: Jakub Kicinski <kuba@kernel.org>
>> > > > Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>> > > > ---
>

[...]


>> > > > +static int dpll_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>> > > > +						 struct genl_info *info)
>> > > > +{
>> > > > +	struct dpll_device *dpll_id = NULL, *dpll_name = NULL;
>> > > > +
>> > > > +	if (!info->attrs[DPLLA_DEVICE_ID] &&
>> > > > +	    !info->attrs[DPLLA_DEVICE_NAME])
>> > > > +		return -EINVAL;
>> > > > +
>> > > > +	if (info->attrs[DPLLA_DEVICE_ID]) {
>> > > > +		u32 id = nla_get_u32(info->attrs[DPLLA_DEVICE_ID]);
>> > > > +
>> > > > +		dpll_id = dpll_device_get_by_id(id);
>> > > > +		if (!dpll_id)
>> > > > +			return -ENODEV;
>> > > > +		info->user_ptr[0] = dpll_id;
>> > > 
>> > > struct dpll_device *dpll should be stored here.
>> > > 
>> > > 
>> > > > +	}
>> > > > +	if (info->attrs[DPLLA_DEVICE_NAME]) {
>> > > 
>> > > You define new API, have one clear handle for devices. Either name or
>> > > ID. Having both is messy.
>> > > 
>> > That was added after the discussion with Jakub and Arkadiusz where we agreed
>> > that the device could be referenced either by index or by name. The example
>> > is that userspace app can easily find specific DPLL device if it knows the
>> > name provided by a driver of that specific device. Without searching through
>> > sysfs to find index value. Later commands could be executed using index once
>> > it's known through CMD_GET_DEVICE/ATTR_DEVICE_NAME.
>> 
>> What exacly is the name? What is the semantics? How the name is
>> generated in case of multiple instances of the same driver. What happens
>> if two drivers use the same name? Is the name predictable (in sense of
>> "stable over reboots")?
>> 
>
>The way we were thinking about name is that driver can provide it's own name
>based on the hardware values, like MAC address or any other unique
>identifier, or the subsystem will use 'dpll%d' template to create the device.
>In the first case names can be predictable and stable over reboots at the
>same time.

Well, I don't think it is in general good idea to allow the drivers such
flexibility in strings directly passed to userspace. From past
experience, it usually end up with mess which is very hard to control.
Therefore, I believe that the driver should pass info in struct of well
defined fields. Like for example THIS_MODULE, and dpll.c can get the
name by module_name()


[...]