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

[Vadim Fedorenko <vfedorenko@novek.ru>]

On 11.07.2022 10:01, Kubalewski, Arkadiusz wrote:
> -----Original Message-----
> From: Vadim Fedorenko <vfedorenko@novek.ru>
> Sent: Sunday, June 26, 2022 9:25 PM
>>
>> 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>
> 
> 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.

>> +	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.

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