Re: [RFC PATCH v8 03/10] dpll: core: Add DPLL framework base functions

[Jiri Pirko <jiri@resnulli.us>]

Wed, Jun 21, 2023 at 11:17:26PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Jakub Kicinski <kuba@kernel.org>
>>Sent: Tuesday, June 13, 2023 1:45 AM
>>
>>On Fri,  9 Jun 2023 14:18:46 +0200 Arkadiusz Kubalewski wrote:
>>> +	xa_for_each(xa_pins, i, ref) {
>>> +		if (ref->pin != pin)
>>> +			continue;
>>> +		reg = dpll_pin_registration_find(ref, ops, priv);
>>> +		if (reg) {
>>> +			refcount_inc(&ref->refcount);
>>> +			return 0;
>>> +		}
>>> +		ref_exists = true;
>>> +		break;
>>> +	}
>>> +
>>> +	if (!ref_exists) {
>>> +		ref = kzalloc(sizeof(*ref), GFP_KERNEL);
>>> +		if (!ref)
>>> +			return -ENOMEM;
>>> +		ref->pin = pin;
>>> +		INIT_LIST_HEAD(&ref->registration_list);
>>> +		ret = xa_insert(xa_pins, pin->pin_idx, ref, GFP_KERNEL);
>>> +		if (ret) {
>>> +			kfree(ref);
>>> +			return ret;
>>> +		}
>>> +		refcount_set(&ref->refcount, 1);
>>> +	}
>>> +
>>> +	reg = kzalloc(sizeof(*reg), GFP_KERNEL);
>>
>>Why do we have two structures - ref and reg?
>>
>
>Thank to Jiri and reg struct we solved a pin/dpll association
>with multiple device drivers..

Multiple instances of the same driver.


>I.e. for pin:
>
>struct dpll_pin_registration {
>	struct list_head list;
>	const struct dpll_pin_ops *ops;
>	void *priv;
>};
>
>struct dpll_pin_ref {
>	union {
>		struct dpll_device *dpll;
>		struct dpll_pin *pin;
>	};
>	struct list_head registration_list;
>	refcount_t refcount;
>};
>
>struct dpll_pin {
>	u32 id;
>	u32 pin_idx;
>	u64 clock_id;
>	struct module *module;
>	struct xarray dpll_refs;
>	struct xarray parent_refs;
>	const struct dpll_pin_properties *prop;
>	char *rclk_dev_name;
>	refcount_t refcount;
>};
>
>Basically, a pin or a device can be registered from multiple drivers,

Again, multiple instances of the same driver.


>where each driver has own priv and ops.

Each instance/device.


>A single dpll_pin has references to dplls or pins (dpll_refs/parent_refs)
>it is connected with, and thanks to registration list single reference can
>have multiple drivers being attached with a particular dpll/pin.

Multiple instances/devices.


In case of mlx5, the same dpll device and same dpll pin could be shared
among two PFs but also among multiple VFs and SFs. They all share the
same clock, same dpll device.


>
>The same scheme is for a dpll_device struct and associated pins.
>
>
>>> +	if (!reg) {
>>> +		if (!ref_exists)
>>> +			kfree(ref);
>>
>>ref has already been inserted into xa_pins
>>
>
>True, seems like a bug, will fix it.
>
>Thank you,
>Arkadiusz
>
>>> +		return -ENOMEM;