[RFC PATCH v2 1/3] dpll: Add DPLL framework base functions

[Kubalewski, Arkadiusz]

-----Original Message-----
From: Vadim Fedorenko <vfedorenko at novek.ru> 
Sent: Friday, July 15, 2022 1:24 AM
>
>On 11.07.2022 10:01, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Vadim Fedorenko <vfedorenko at novek.ru>
>> Sent: Sunday, June 26, 2022 9:25 PM
>>>
>>> From: Vadim Fedorenko <vadfed at 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 at fb.com>
>> 
>> Hi Vadim,
>> I've been trying to implement usage of this code in our driver.
>> Any chance for some testing/example app?
>> 
>
>Hi Arkadiusz,
>Sorry, but I don't have any working app yet, this subsystem is
>treated as experimental and as we now have different interface
>for configuring features we need, app implementation is postponed
>a bit. After some conversation with Jakub I'm thinking about library
>to provide easy interface to this subsystem, but stil no code yet.
>
>>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>>> +					 int outputs_count, void *priv)
>> 
>> Aren't there some alignment issues around function definitions?
>>
>
>Yeah, I know about some style issues, trying to fix them for the next round.
>
>>> +{
>>> +	struct dpll_device *dpll;
>>> +	int ret;
>>> +
>>> +	dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
>>> +	if (!dpll)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	mutex_init(&dpll->lock);
>>> +	dpll->ops = ops;
>>> +	dpll->dev.class = &dpll_class;
>>> +	dpll->sources_count = sources_count;
>>> +	dpll->outputs_count = outputs_count;
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, GFP_KERNEL);
>>> +	if (ret)
>>> +		goto error;
>>> +	dev_set_name(&dpll->dev, "dpll%d", dpll->id);
>> 
>> Not sure if I mentioned it before, the user must be able to identify the
>> purpose and origin of dpll. Right now, if 2 dplls register in the system, it is
>> not possible to determine where they belong or what do they do. I would say,
>> easiest to let caller of dpll_device_alloc assign some name or description.
>>
>
>Maybe driver can export information about dpll device name after registering it?
>But at the same time looks like it's easy enough to implement custom naming. The
>only problem is to control that names are unique.

Right now the name is unique, but its not persistent, so this also might be
hard for the user. Reloading drivers which implemented the dpll interface will
result in a userspace app config mess, as order of loading will impact their id
in dpll<id> format.

Maybe some additional string id in format like <pci_id>.<idx>?
As I understand whatever is registering a dpll, it should be some kind of
device, thus we could pass pointer to dev on alloc and ask it for device name
(dev_name(dev))?
The <idx> could be given also on alloc, the driver developer which would be
using dpll subsystem will take care of getting uniqe values for it, as it won't
be fully usable without it being unique.

>
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +	dpll->priv = priv;
>>> +
>>> +	return dpll;
>>> +
>>> +error:
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +	kfree(dpll);
>>> +	return ERR_PTR(ret);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_device_alloc);
>>> +
>>> +void dpll_device_free(struct dpll_device *dpll)
>>> +{
>>> +	if (!dpll)
>>> +		return;
>>> +
>>> +	mutex_destroy(&dpll->lock);
>>> +	kfree(dpll);
>>> +}
>> 
>> dpll_device_free() is defined in header, shouldn't it be exported?
>> 
>
>yeah, definitely. Already changed in new draft.
>
>>> +
>>> +void dpll_device_register(struct dpll_device *dpll)
>>> +{
>>> +	ASSERT_DPLL_NOT_REGISTERED(dpll);
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
>>> +	dpll_notify_device_create(dpll->id, dev_name(&dpll->dev));
>> 
>> dpll_notify_device_create is not yet defined, this is part of patch 2/3?
>> Also in patch 2/3 similar call was added in dpll_device_alloc().
>>
>
>Ah. Yes, there was some mess with patches, looks like I missed this thing, thank
>you for pointing it.
>
>
>>> +static const struct genl_ops dpll_genl_ops[] = {
>>> +	{
>>> +		.cmd	= DPLL_CMD_DEVICE_GET,
>>> +		.start	= dpll_genl_cmd_start,
>>> +		.dumpit	= dpll_genl_cmd_dumpit,
>>> +		.doit	= dpll_genl_cmd_doit,
>>> +		.policy	= dpll_genl_get_policy,
>>> +		.maxattr = ARRAY_SIZE(dpll_genl_get_policy) - 1,
>>> +	},
>> 
>> I wouldn't leave non-privileged user with possibility to call any HW requests.
>>
>
>Yep, definitely. Didn't thought about security restrictions yet.
>
>
>>> +/* Commands supported by the dpll_genl_family */
>>> +enum dpll_genl_cmd {
>>> +	DPLL_CMD_UNSPEC,
>>> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
>>> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
>>> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Set the DPLL device output type */
>> 
>> This week, I am going to prepare the patch for DPLL mode and input priority list
>> we have discussed on the previous patch series.
>>
>
>Great! Do you have this work somewhere in public git? If not I will try to 
>publish this branch somewhere to make collaboration easier.

Not yet, but sure, please start the branch for collaboration.

Thanks,
Arkadiusz
>
>> Thank you!
>> Arkadiusz
>> 
>>> +
>>> +	__DPLL_CMD_MAX,
>>> +};
>>> +#define DPLL_CMD_MAX (__DPLL_CMD_MAX - 1)
>>> +
>>> +#endif /* _UAPI_LINUX_DPLL_H */
>>> -- 
>>> 2.27.0
>>>
>