Re: [PATCH net-next 03/10] dpll: add domain object

[Petr Oros <poros@redhat.com>]

On 11/6/26 1:09 PM, Ivan Vecera wrote:
> All dpll device and pin callbacks are serialized by the global dpll_lock,
> so a slow device stalls all others, whichever driver they belong to.
> 
> Add struct dpll_domain as a preparation for serializing callbacks per
> driver instance. A domain groups all devices and pins created with the
> same (module, clock_id) pair, which is in practice one chip or card. The
> same pair already identifies the owner of a pin in
> dpll_pin_own_dpll_ref_first().
> 
> Domains are kept in an xarray, like devices and pins, and they live only as
> long as their objects.
> 
> No functional change.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/dpll/dpll_core.c | 61 ++++++++++++++++++++++++++++++++++++++++
>   drivers/dpll/dpll_core.h | 27 ++++++++++++++++++
>   2 files changed, 88 insertions(+)
> 
> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
> index 8389983f425695..d39c4905172f2d 100644
> --- a/drivers/dpll/dpll_core.c
> +++ b/drivers/dpll/dpll_core.c
> @@ -24,6 +24,7 @@ DEFINE_MUTEX(dpll_lock);
>   
>   DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC);
>   DEFINE_XARRAY_FLAGS(dpll_pin_xa, XA_FLAGS_ALLOC);
> +static DEFINE_XARRAY_FLAGS(dpll_domain_xa, XA_FLAGS_ALLOC);
>   
>   static RAW_NOTIFIER_HEAD(dpll_notifier_chain);
>   static DEFINE_IDA(dpll_pin_idx_ida);
> @@ -88,6 +89,50 @@ void dpll_pin_notify(struct dpll_pin *pin, u64 src_clock_id,
>   	call_dpll_notifiers(action, &info);
>   }
>   
> +static struct dpll_domain *dpll_domain_get(u64 clock_id, struct module *module)
> +{
> +	struct dpll_domain *domain;
> +	unsigned long i;
> +	int ret;
> +
> +	lockdep_assert_held(&dpll_lock);
> +
> +	xa_for_each(&dpll_domain_xa, i, domain) {
> +		if (domain->clock_id == clock_id && domain->module == module) {
> +			refcount_inc(&domain->refcount);
> +			return domain;
> +		}
> +	}
> +
> +	domain = kzalloc_obj(*domain);
> +	if (!domain)
> +		return ERR_PTR(-ENOMEM);
> +	domain->clock_id = clock_id;
> +	domain->module = module;
> +	refcount_set(&domain->refcount, 1);
> +	mutex_init(&domain->lock);
> +	ret = xa_alloc(&dpll_domain_xa, &domain->id, domain, xa_limit_32b,
> +		       GFP_KERNEL);
> +	if (ret) {
> +		mutex_destroy(&domain->lock);
> +		kfree(domain);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return domain;
> +}
> +
> +static void dpll_domain_put(struct dpll_domain *domain)
> +{
> +	lockdep_assert_held(&dpll_lock);
> +
> +	if (refcount_dec_and_test(&domain->refcount)) {
> +		xa_erase(&dpll_domain_xa, domain->id);
> +		mutex_destroy(&domain->lock);
> +		kfree(domain);
> +	}
> +}
> +
>   static void dpll_device_tracker_alloc(struct dpll_device *dpll,
>   				      dpll_tracker *tracker)
>   {
> @@ -120,6 +165,7 @@ static void __dpll_device_put(struct dpll_device *dpll, dpll_tracker *tracker)
>   		xa_erase(&dpll_device_xa, dpll->id);
>   		WARN_ON(!list_empty(&dpll->registration_list));
>   		ref_tracker_dir_exit(&dpll->refcnt_tracker);
> +		dpll_domain_put(dpll->domain);
>   		kfree(dpll);
>   	}
>   }
> @@ -159,6 +205,7 @@ static void __dpll_pin_put(struct dpll_pin *pin, dpll_tracker *tracker)
>   		fwnode_handle_put(pin->fwnode);
>   		dpll_pin_idx_free(pin->pin_idx);
>   		ref_tracker_dir_exit(&pin->refcnt_tracker);
> +		dpll_domain_put(pin->domain);
>   		kfree_rcu(pin, rcu);
>   	}
>   }
> @@ -366,6 +413,12 @@ dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module)
>   	dpll = kzalloc_obj(*dpll);
>   	if (!dpll)
>   		return ERR_PTR(-ENOMEM);
> +	dpll->domain = dpll_domain_get(clock_id, module);
> +	if (IS_ERR(dpll->domain)) {
> +		ret = PTR_ERR(dpll->domain);
> +		kfree(dpll);
> +		return ERR_PTR(ret);
> +	}
>   	refcount_set(&dpll->refcount, 1);
>   	INIT_LIST_HEAD(&dpll->registration_list);
>   	dpll->device_idx = device_idx;
> @@ -374,6 +427,7 @@ dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module)
>   	ret = xa_alloc_cyclic(&dpll_device_xa, &dpll->id, dpll, xa_limit_32b,
>   			      &dpll_device_xa_id, GFP_KERNEL);
>   	if (ret < 0) {
> +		dpll_domain_put(dpll->domain);
>   		kfree(dpll);
>   		return ERR_PTR(ret);
>   	}
> @@ -664,6 +718,11 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
>   	ret = dpll_pin_prop_dup(prop, &pin->prop);
>   	if (ret)
>   		goto err_pin_prop;
> +	pin->domain = dpll_domain_get(clock_id, module);
> +	if (IS_ERR(pin->domain)) {
> +		ret = PTR_ERR(pin->domain);
> +		goto err_domain;
> +	}
>   	refcount_set(&pin->refcount, 1);
>   	xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
>   	xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
> @@ -678,6 +737,8 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module,
>   	xa_destroy(&pin->dpll_refs);
>   	xa_destroy(&pin->parent_refs);
>   	xa_destroy(&pin->ref_sync_pins);
> +	dpll_domain_put(pin->domain);
> +err_domain:
>   	dpll_pin_prop_free(&pin->prop);
>   err_pin_prop:
>   	kfree(pin);
> diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
> index da8a369556ed11..6705e81bc11de9 100644
> --- a/drivers/dpll/dpll_core.h
> +++ b/drivers/dpll/dpll_core.h
> @@ -9,18 +9,42 @@
>   
>   #include <linux/dpll.h>
>   #include <linux/list.h>
> +#include <linux/mutex.h>
>   #include <linux/refcount.h>
>   #include <linux/ref_tracker.h>
>   #include "dpll_nl.h"
>   
>   #define DPLL_REGISTERED		XA_MARK_1
>   
> +/**
> + * struct dpll_domain - group of dpll objects sharing a lock
> + * @id:			index of the domain in dpll_domain_xa
> + * @clock_id:		clock_id of the objects in the domain
> + * @module:		module of the objects in the domain
> + * @refcount:		number of dpll devices and pins in the domain
> + * @lock:		lock of the domain
> + *
> + * All dpll devices and pins created with the same (module, clock_id) pair
> + * belong to one domain, in practice to one chip or card.
> + **/
> +struct dpll_domain {
> +	u32 id;
> +	u64 clock_id;
> +	struct module *module;
> +	refcount_t refcount;
> +	/* Serializes the driver callbacks of all DPLL devices and pins
> +	 * that share one (module, clock_id) pair.
> +	 */
> +	struct mutex lock;
> +};
> +
>   /**
>    * struct dpll_device - stores DPLL device internal data
>    * @id:			unique id number for device given by dpll subsystem
>    * @device_idx:		id given by dev driver
>    * @clock_id:		unique identifier (clock_id) of a dpll
>    * @module:		module of creator
> + * @domain:		domain the device belongs to
>    * @type:		type of a dpll
>    * @pin_refs:		stores pins registered within a dpll
>    * @refcount:		refcount
> @@ -32,6 +56,7 @@ struct dpll_device {
>   	u32 device_idx;
>   	u64 clock_id;
>   	struct module *module;
> +	struct dpll_domain *domain;
>   	enum dpll_type type;
>   	struct xarray pin_refs;
>   	refcount_t refcount;
> @@ -46,6 +71,7 @@ struct dpll_device {
>    * @clock_id:		clock_id of creator
>    * @module:		module of creator
>    * @module_name:	module name of creator
> + * @domain:		domain the pin belongs to
>    * @fwnode:		optional reference to firmware node
>    * @dpll_refs:		hold referencees to dplls pin was registered with
>    * @parent_refs:	hold references to parent pins pin was registered with
> @@ -61,6 +87,7 @@ struct dpll_pin {
>   	u64 clock_id;
>   	struct module *module;
>   	char module_name[MODULE_NAME_LEN];
> +	struct dpll_domain *domain;
>   	struct fwnode_handle *fwnode;
>   	struct xarray dpll_refs;
>   	struct xarray parent_refs;

Reviewed-by: Petr Oros <poros@redhat.com>