| Message ID | 20230609121853.3607724-4-arkadiusz.kubalewski@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Create common DPLL configuration API | expand |
Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com wrote: >From: Vadim Fedorenko <vadim.fedorenko@linux.dev> > >DPLL framework is used to represent and configure DPLL devices >in systems. Each device that has DPLL and can configure inputs >and outputs can use this framework. > >Implement core framework functions for further interactions >with device drivers implementing dpll subsystem, as well as for >interactions of DPLL netlink framework part with the subsystem >itself. > >Co-developed-by: Milena Olech <milena.olech@intel.com> >Signed-off-by: Milena Olech <milena.olech@intel.com> >Co-developed-by: Michal Michalik <michal.michalik@intel.com> >Signed-off-by: Michal Michalik <michal.michalik@intel.com> >Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >--- > drivers/dpll/dpll_core.c | 953 +++++++++++++++++++++++++++++++++++++++ > drivers/dpll/dpll_core.h | 104 +++++ Overall, looks very good! I pinpointed couple of nits below, nothing big. General question: Why do you put documentation comment to every static function? Does not make any sense to me. Even for non-exported functions I think it is overkill. Most of them (if not all) give the reader no additional information and only make the code a bit harder to read. Care to drop them? > 2 files changed, 1057 insertions(+) > create mode 100644 drivers/dpll/dpll_core.c > create mode 100644 drivers/dpll/dpll_core.h > >diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c >new file mode 100644 >index 000000000000..ee515b7c18be >--- /dev/null >+++ b/drivers/dpll/dpll_core.c >@@ -0,0 +1,953 @@ >+// SPDX-License-Identifier: GPL-2.0 >+/* >+ * dpll_core.c - Generic DPLL Management class support. What's "management class support"? Does not sound recognizable. >+ * >+ * Copyright (c) 2023 Meta Platforms, Inc. and affiliates >+ * Copyright (c) 2023 Intel Corporation. >+ */ >+ >+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >+ >+#include <linux/device.h> >+#include <linux/err.h> >+#include <linux/slab.h> >+#include <linux/string.h> >+ >+#include "dpll_core.h" >+ >+DEFINE_MUTEX(dpll_lock); >+ >+DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC); >+DEFINE_XARRAY_FLAGS(dpll_pin_xa, XA_FLAGS_ALLOC); >+ >+#define ASSERT_DPLL_REGISTERED(d) \ >+ WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED)) >+#define ASSERT_DPLL_NOT_REGISTERED(d) \ >+ WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED)) >+ >+/** >+ * dpll_device_get_by_id - find dpll device by it's id >+ * @id: id of searched dpll >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * dpll_device struct if found >+ * * NULL otherwise >+ */ >+struct dpll_device *dpll_device_get_by_id(int id) >+{ >+ if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED)) >+ return xa_load(&dpll_device_xa, id); >+ >+ return NULL; >+} >+ >+/** >+ * dpll_pin_registration_find - find a pin registration record >+ * @ref: reference between dpll and pin >+ * @ops: searched pin ops pointer >+ * @priv: searched pin priv pointer >+ * >+ * Search dpll's registered pins for given ops and priv data. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * NULL - if pin was not found >+ * * pointer to `struct dpll_pin_registration` if found >+ */ >+static struct dpll_pin_registration * >+dpll_pin_registration_find(struct dpll_pin_ref *ref, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ struct dpll_pin_registration *reg; >+ >+ list_for_each_entry(reg, &ref->registration_list, list) { >+ if (reg->ops == ops && reg->priv == priv) >+ return reg; >+ } >+ return NULL; >+} >+ >+/** >+ * dpll_xa_ref_pin_add - add pin reference to a given xarray >+ * @xa_pins: dpll_pin_ref xarray holding pins >+ * @pin: pin being added >+ * @ops: ops for a pin >+ * @priv: pointer to private data of owner >+ * >+ * Allocate and create reference of a pin and enlist a registration >+ * structure storing ops and priv pointers of a caller registant. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * 0 on success >+ * * -ENOMEM on failed allocation >+ */ >+static int >+dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ struct dpll_pin_registration *reg; >+ struct dpll_pin_ref *ref; >+ bool ref_exists = false; >+ unsigned long i; >+ int ret; >+ >+ 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); >+ if (!reg) { >+ if (!ref_exists) >+ kfree(ref); >+ return -ENOMEM; >+ } >+ reg->ops = ops; >+ reg->priv = priv; >+ if (ref_exists) If you set refcount to 0 couple of lines above, you can drop the check here. >+ refcount_inc(&ref->refcount); >+ list_add_tail(®->list, &ref->registration_list); >+ >+ return 0; >+} >+ >+/** >+ * dpll_xa_ref_pin_del - remove reference of a pin from xarray >+ * @xa_pins: dpll_pin_ref xarray holding pins >+ * @pin: pointer to a pin being removed >+ * @ops: pointer to ops of pin being removed >+ * @priv: pointer to private data of registerer who invoked pin removal >+ * >+ * Decrement refcount of existing pin reference on given xarray. >+ * If all registrations are lifted delete the reference and free its memory. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * 0 on success >+ * * -EINVAL if reference to a pin was not found >+ */ >+static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ struct dpll_pin_registration *reg; >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ >+ xa_for_each(xa_pins, i, ref) { >+ if (ref->pin != pin) >+ continue; >+ reg = dpll_pin_registration_find(ref, ops, priv); >+ if (WARN_ON(!reg)) >+ return -EINVAL; >+ if (refcount_dec_and_test(&ref->refcount)) { >+ list_del(®->list); >+ kfree(reg); >+ xa_erase(xa_pins, i); >+ WARN_ON(!list_empty(&ref->registration_list)); >+ kfree(ref); >+ } >+ return 0; >+ } >+ >+ return -EINVAL; >+} >+ >+/** >+ * dpll_xa_ref_dpll_add - add dpll reference to a given xarray >+ * @xa_dplls: dpll_pin_ref xarray holding dplls >+ * @dpll: dpll being added >+ * @ops: pin-reference ops for a dpll >+ * @priv: pointer to private data of owner >+ * >+ * Allocate and create reference of a dpll-pin ops or increase refcount >+ * on existing dpll reference on given xarray. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * 0 on success >+ * * -ENOMEM on failed allocation >+ */ >+static int >+dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ struct dpll_pin_registration *reg; >+ struct dpll_pin_ref *ref; >+ bool ref_exists = false; >+ unsigned long i; >+ int ret; >+ >+ xa_for_each(xa_dplls, i, ref) { >+ if (ref->dpll != dpll) >+ 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->dpll = dpll; >+ INIT_LIST_HEAD(&ref->registration_list); >+ ret = xa_insert(xa_dplls, dpll->device_idx, ref, GFP_KERNEL); >+ if (ret) { >+ kfree(ref); >+ return ret; >+ } >+ refcount_set(&ref->refcount, 1); >+ } >+ >+ reg = kzalloc(sizeof(*reg), GFP_KERNEL); >+ if (!reg) { >+ if (!ref_exists) >+ kfree(ref); >+ return -ENOMEM; >+ } >+ reg->ops = ops; >+ reg->priv = priv; >+ if (ref_exists) If you set refcount to 0 couple of lines above, you can drop the check here. >+ refcount_inc(&ref->refcount); >+ list_add_tail(®->list, &ref->registration_list); >+ >+ return 0; >+} >+ >+/** >+ * dpll_xa_ref_dpll_del - remove reference of a dpll from xarray >+ * @xa_dplls: dpll_pin_ref xarray holding dplls >+ * @dpll: pointer to a dpll to remove >+ * @ops: pointer to ops of dpll being removed >+ * @priv: pointer to private data of registerer who invoked dpll removal >+ * >+ * Decrement refcount of existing dpll reference on given xarray. >+ * If all references are dropped, delete the reference and free its memory. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ */ >+static void >+dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ struct dpll_pin_registration *reg; >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ >+ xa_for_each(xa_dplls, i, ref) { >+ if (ref->dpll != dpll) >+ continue; >+ reg = dpll_pin_registration_find(ref, ops, priv); >+ if (WARN_ON(!reg)) >+ return; >+ if (refcount_dec_and_test(&ref->refcount)) { >+ list_del(®->list); >+ kfree(reg); >+ xa_erase(xa_dplls, i); >+ WARN_ON(!list_empty(&ref->registration_list)); >+ kfree(ref); >+ } >+ return; >+ } >+} >+ >+/** >+ * dpll_xa_ref_dpll_find - find dpll reference on xarray >+ * @xa_refs: dpll_pin_ref xarray holding dpll references >+ * @dpll: pointer to a dpll being searched >+ * >+ * Search for dpll-pin ops reference struct of a given dpll on given xarray. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * pin reference struct pointer on success >+ * * NULL - reference to a pin was not found >+ */ >+struct dpll_pin_ref * >+dpll_xa_ref_dpll_find(struct xarray *xa_refs, const struct dpll_device *dpll) >+{ >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ >+ xa_for_each(xa_refs, i, ref) { >+ if (ref->dpll == dpll) >+ return ref; >+ } >+ >+ return NULL; >+} >+ >+/** >+ * dpll_xa_ref_dpll_first - find first record of given xarray >+ * @xa_refs: xarray >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: first element on given xaaray >+ */ >+struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs) >+{ >+ struct dpll_pin_ref *ref; >+ unsigned long i = 0; >+ >+ ref = xa_find(xa_refs, &i, ULONG_MAX, XA_PRESENT); >+ WARN_ON(!ref); >+ return ref; >+} >+ >+/** >+ * dpll_device_alloc - allocate the memory for dpll device >+ * @clock_id: clock_id of creator >+ * @device_idx: id given by dev driver >+ * @module: reference to registering module >+ * >+ * Allocates memory and initialize dpll device, hold its reference on global >+ * xarray. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * valid dpll_device struct pointer if succeeded >+ * * ERR_PTR(-ENOMEM) - failed memory allocation >+ * * ERR_PTR(X) - failed allocation on dpll's xa >+ */ >+static struct dpll_device * >+dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module) >+{ >+ struct dpll_device *dpll; >+ int ret; >+ >+ dpll = kzalloc(sizeof(*dpll), GFP_KERNEL); >+ if (!dpll) >+ return ERR_PTR(-ENOMEM); >+ refcount_set(&dpll->refcount, 1); >+ INIT_LIST_HEAD(&dpll->registration_list); >+ dpll->device_idx = device_idx; >+ dpll->clock_id = clock_id; >+ dpll->module = module; >+ ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, >+ GFP_KERNEL); >+ if (ret) { >+ kfree(dpll); >+ return ERR_PTR(ret); >+ } >+ xa_init_flags(&dpll->pin_refs, XA_FLAGS_ALLOC); >+ >+ return dpll; >+} >+ >+/** >+ * dpll_device_get - find existing or create new dpll device >+ * @clock_id: clock_id of creator >+ * @device_idx: idx given by device driver >+ * @module: reference to registering module >+ * >+ * Get existing object of a dpll device, unique for given arguments. >+ * Create new if doesn't exist yet. >+ * >+ * Context: Acquires a lock (dpll_lock) >+ * Return: >+ * * valid dpll_device struct pointer if succeeded >+ * * ERR_PTR(-ENOMEM) - failed memory allocation >+ * * ERR_PTR(X) - failed allocation on dpll's xa >+ */ >+struct dpll_device * >+dpll_device_get(u64 clock_id, u32 device_idx, struct module *module) >+{ >+ struct dpll_device *dpll, *ret = NULL; >+ unsigned long index; >+ >+ mutex_lock(&dpll_lock); >+ xa_for_each(&dpll_device_xa, index, dpll) { >+ if (dpll->clock_id == clock_id && >+ dpll->device_idx == device_idx && >+ dpll->module == module) { >+ ret = dpll; >+ refcount_inc(&ret->refcount); >+ break; >+ } >+ } >+ if (!ret) >+ ret = dpll_device_alloc(clock_id, device_idx, module); >+ mutex_unlock(&dpll_lock); >+ >+ return ret; >+} >+EXPORT_SYMBOL_GPL(dpll_device_get); >+ >+/** >+ * dpll_device_put - decrease the refcount and free memory if possible >+ * @dpll: dpll_device struct pointer >+ * >+ * Context: Acquires a lock (dpll_lock) >+ * Drop reference for a dpll device, if all references are gone, delete >+ * dpll device object. >+ */ >+void dpll_device_put(struct dpll_device *dpll) >+{ >+ if (!dpll) Drop this check and rely on a basic driver sanity. I'm pretty sure I asked you to drop checks like this during the last version review. >+ return; >+ mutex_lock(&dpll_lock); >+ if (refcount_dec_and_test(&dpll->refcount)) { >+ ASSERT_DPLL_NOT_REGISTERED(dpll); >+ WARN_ON_ONCE(!xa_empty(&dpll->pin_refs)); >+ xa_destroy(&dpll->pin_refs); >+ xa_erase(&dpll_device_xa, dpll->id); >+ WARN_ON(!list_empty(&dpll->registration_list)); >+ kfree(dpll); >+ } >+ mutex_unlock(&dpll_lock); >+} >+EXPORT_SYMBOL_GPL(dpll_device_put); >+ >+static struct dpll_device_registration * >+dpll_device_registration_find(struct dpll_device *dpll, >+ const struct dpll_device_ops *ops, void *priv) >+{ >+ struct dpll_device_registration *reg; >+ >+ list_for_each_entry(reg, &dpll->registration_list, list) { >+ if (reg->ops == ops && reg->priv == priv) >+ return reg; >+ } >+ return NULL; >+} >+ >+/** >+ * dpll_device_register - register the dpll device in the subsystem >+ * @dpll: pointer to a dpll >+ * @type: type of a dpll >+ * @ops: ops for a dpll device >+ * @priv: pointer to private information of owner >+ * >+ * Make dpll device available for user space. >+ * >+ * Context: Acquires a lock (dpll_lock) >+ * Return: >+ * * 0 on success >+ * * -EINVAL on failure due to wrong arguments provided >+ * * -EEXIST if device was already registered >+ */ >+int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, >+ const struct dpll_device_ops *ops, void *priv) >+{ >+ struct dpll_device_registration *reg; >+ bool first_registration = false; >+ >+ if (WARN_ON(!ops)) >+ return -EINVAL; >+ if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) >+ return -EINVAL; >+ >+ mutex_lock(&dpll_lock); >+ reg = dpll_device_registration_find(dpll, ops, priv); >+ if (reg) { >+ mutex_unlock(&dpll_lock); >+ return -EEXIST; >+ } >+ >+ reg = kzalloc(sizeof(*reg), GFP_KERNEL); >+ if (!reg) { >+ mutex_unlock(&dpll_lock); >+ return -EEXIST; -ENOMEM; >+ } >+ reg->ops = ops; >+ reg->priv = priv; >+ dpll->type = type; >+ first_registration = list_empty(&dpll->registration_list); >+ list_add_tail(®->list, &dpll->registration_list); >+ if (!first_registration) { >+ mutex_unlock(&dpll_lock); >+ return 0; >+ } >+ >+ xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); >+ mutex_unlock(&dpll_lock); >+ dpll_device_create_ntf(dpll); >+ >+ return 0; >+} >+EXPORT_SYMBOL_GPL(dpll_device_register); >+ >+/** >+ * dpll_device_unregister - unregister dpll device >+ * @dpll: registered dpll pointer >+ * @ops: ops for a dpll device >+ * @priv: pointer to private information of owner >+ * >+ * Unregister device, make it unavailable for userspace. >+ * Note: It does not free the memory >+ * Context: Acquires a lock (dpll_lock) >+ */ >+void dpll_device_unregister(struct dpll_device *dpll, >+ const struct dpll_device_ops *ops, void *priv) >+{ >+ struct dpll_device_registration *reg; >+ >+ mutex_lock(&dpll_lock); >+ ASSERT_DPLL_REGISTERED(dpll); >+ dpll_device_delete_ntf(dpll); >+ reg = dpll_device_registration_find(dpll, ops, priv); >+ if (WARN_ON(!reg)) { >+ mutex_unlock(&dpll_lock); >+ return; >+ } >+ list_del(®->list); >+ kfree(reg); >+ >+ if (!list_empty(&dpll->registration_list)) { >+ mutex_unlock(&dpll_lock); >+ return; >+ } >+ xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); >+ mutex_unlock(&dpll_lock); >+} >+EXPORT_SYMBOL_GPL(dpll_device_unregister); >+ >+/** >+ * dpll_pin_alloc - allocate the memory for dpll pin >+ * @clock_id: clock_id of creator >+ * @pin_idx: idx given by dev driver >+ * @module: reference to registering module >+ * @prop: dpll pin properties >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * valid allocated dpll_pin struct pointer if succeeded >+ * * ERR_PTR(-ENOMEM) - failed memory allocation >+ * * ERR_PTR(-EINVAL) - wrong pin type property value >+ */ >+static struct dpll_pin * >+dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module, >+ const struct dpll_pin_properties *prop) >+{ >+ struct dpll_pin *pin; >+ int ret; >+ >+ pin = kzalloc(sizeof(*pin), GFP_KERNEL); >+ if (!pin) >+ return ERR_PTR(-ENOMEM); >+ pin->pin_idx = pin_idx; >+ pin->clock_id = clock_id; >+ pin->module = module; >+ if (WARN_ON(prop->type < DPLL_PIN_TYPE_MUX || >+ prop->type > DPLL_PIN_TYPE_MAX)) { >+ ret = -EINVAL; >+ goto err; >+ } >+ pin->prop = prop; >+ refcount_set(&pin->refcount, 1); >+ xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC); >+ xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC); >+ ret = xa_alloc(&dpll_pin_xa, &pin->id, pin, xa_limit_16b, GFP_KERNEL); >+ if (ret) >+ goto err; >+ return pin; >+err: >+ xa_destroy(&pin->dpll_refs); >+ xa_destroy(&pin->parent_refs); >+ kfree(pin); >+ return ERR_PTR(ret); >+} >+ >+/** >+ * dpll_pin_get - find existing or create new dpll pin >+ * @clock_id: clock_id of creator >+ * @pin_idx: idx given by dev driver >+ * @module: reference to registering module >+ * @prop: dpll pin properties >+ * >+ * Get existing object of a pin (unique for given arguments) or create new >+ * if doesn't exist yet. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: >+ * * valid allocated dpll_pin struct pointer if succeeded >+ * * ERR_PTR of an error >+ */ >+struct dpll_pin * >+dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, >+ const struct dpll_pin_properties *prop) >+{ >+ struct dpll_pin *pos, *ret = NULL; >+ unsigned long i; >+ >+ xa_for_each(&dpll_pin_xa, i, pos) { >+ if (pos->clock_id == clock_id && >+ pos->pin_idx == pin_idx && >+ pos->module == module) { >+ ret = pos; >+ refcount_inc(&ret->refcount); >+ break; >+ } >+ } >+ if (!ret) >+ ret = dpll_pin_alloc(clock_id, pin_idx, module, prop); >+ >+ return ret; >+} >+EXPORT_SYMBOL_GPL(dpll_pin_get); >+ >+/** >+ * dpll_pin_put - decrease the refcount and free memory if possible >+ * @pin: pointer to a pin to be put >+ * >+ * Drop reference for a pin, if all references are gone, delete pin object. >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ */ >+void dpll_pin_put(struct dpll_pin *pin) >+{ >+ if (!pin) Drop this check. >+ return; >+ if (refcount_dec_and_test(&pin->refcount)) { >+ xa_destroy(&pin->dpll_refs); >+ xa_destroy(&pin->parent_refs); >+ xa_erase(&dpll_pin_xa, pin->id); >+ kfree(pin); >+ } >+} >+EXPORT_SYMBOL_GPL(dpll_pin_put); >+ >+static int >+__dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ int ret; >+ >+ if (WARN_ON(!ops)) >+ return -EINVAL; >+ >+ ret = dpll_xa_ref_pin_add(&dpll->pin_refs, pin, ops, priv); >+ if (ret) >+ return ret; >+ ret = dpll_xa_ref_dpll_add(&pin->dpll_refs, dpll, ops, priv); >+ if (ret) >+ goto ref_pin_del; >+ else >+ dpll_pin_create_ntf(pin); >+ >+ return ret; >+ >+ref_pin_del: >+ dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv); >+ return ret; >+} >+ >+/** >+ * dpll_pin_register - register the dpll pin in the subsystem >+ * @dpll: pointer to a dpll >+ * @pin: pointer to a dpll pin >+ * @ops: ops for a dpll pin ops >+ * @priv: pointer to private information of owner >+ * >+ * Context: Acquires a lock (dpll_lock) >+ * Return: >+ * * 0 on success >+ * * -EINVAL - missing pin ops >+ * * -ENOMEM - failed to allocate memory >+ */ >+int >+dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ int ret; >+ >+ mutex_lock(&dpll_lock); >+ if (WARN_ON(!(dpll->module == pin->module && >+ dpll->clock_id == pin->clock_id))) >+ ret = -EFAULT; In other cases like this you return -EINVAL. I think it is better. One way or another, be consistent please. >+ else >+ ret = __dpll_pin_register(dpll, pin, ops, priv); >+ mutex_unlock(&dpll_lock); >+ >+ return ret; >+} >+EXPORT_SYMBOL_GPL(dpll_pin_register); >+ >+static void >+__dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv); >+ dpll_xa_ref_dpll_del(&pin->dpll_refs, dpll, ops, priv); >+} >+ >+/** >+ * dpll_pin_unregister - unregister dpll pin from dpll device >+ * @dpll: registered dpll pointer >+ * @pin: pointer to a pin >+ * @ops: ops for a dpll pin >+ * @priv: pointer to private information of owner >+ * >+ * Note: It does not free the memory >+ * Context: Acquires a lock (dpll_lock) >+ */ >+void dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ if (WARN_ON(xa_empty(&dpll->pin_refs))) >+ return; >+ >+ mutex_lock(&dpll_lock); >+ __dpll_pin_unregister(dpll, pin, ops, priv); >+ mutex_unlock(&dpll_lock); >+} >+EXPORT_SYMBOL_GPL(dpll_pin_unregister); >+ >+/** >+ * dpll_pin_on_pin_register - register a pin with a parent pin >+ * @parent: pointer to a parent pin >+ * @pin: pointer to a pin >+ * @ops: ops for a dpll pin >+ * @priv: pointer to private information of owner >+ * >+ * Register a pin with a parent pin, create references between them and >+ * between newly registered pin and dplls connected with a parent pin. >+ * >+ * Context: Acquires a lock (dpll_lock) >+ * Return: >+ * * 0 on success >+ * * -EINVAL missing pin or parent >+ * * -ENOMEM failed allocation >+ * * -EPERM if parent is not allowed >+ */ >+int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ struct dpll_pin_ref *ref; >+ unsigned long i, stop; >+ int ret; >+ >+ if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX)) >+ return -EINVAL; >+ ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv); >+ if (ret) >+ goto unlock; >+ refcount_inc(&pin->refcount); >+ xa_for_each(&parent->dpll_refs, i, ref) { >+ mutex_lock(&dpll_lock); >+ ret = __dpll_pin_register(ref->dpll, pin, ops, priv); >+ mutex_unlock(&dpll_lock); >+ if (ret) { >+ stop = i; >+ goto dpll_unregister; >+ } >+ dpll_pin_create_ntf(pin); >+ } >+ >+ return ret; >+ >+dpll_unregister: >+ xa_for_each(&parent->dpll_refs, i, ref) { >+ if (i < stop) { >+ mutex_lock(&dpll_lock); >+ __dpll_pin_unregister(ref->dpll, pin, ops, priv); >+ mutex_unlock(&dpll_lock); >+ } >+ } >+ refcount_dec(&pin->refcount); >+ dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv); >+unlock: >+ return ret; >+} >+EXPORT_SYMBOL_GPL(dpll_pin_on_pin_register); >+ >+/** >+ * dpll_pin_on_pin_unregister - unregister dpll pin from a parent pin >+ * @parent: pointer to a parent pin >+ * @pin: pointer to a pin >+ * @ops: ops for a dpll pin >+ * @priv: pointer to private information of owner >+ * >+ * Context: Acquires a lock (dpll_lock) >+ * Note: It does not free the memory >+ */ >+void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ struct dpll_pin_ref *ref; >+ unsigned long i; >+ >+ mutex_lock(&dpll_lock); >+ dpll_pin_delete_ntf(pin); >+ dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv); >+ refcount_dec(&pin->refcount); >+ xa_for_each(&pin->dpll_refs, i, ref) { >+ __dpll_pin_unregister(ref->dpll, pin, ops, priv); >+ } Drop "{}"s for one line statement. Didn't checkpack warn you? >+ mutex_unlock(&dpll_lock); >+} >+EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister); >+ >+ >+/** >+ * dpll_device_registration_first - get first registration of dpll device >+ * @dpll: pointer to a dpll >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: pointer to the first registration of a dpll >+ */ >+static struct dpll_device_registration * >+dpll_device_registration_first(struct dpll_device *dpll) >+{ >+ struct dpll_device_registration *reg; >+ >+ reg = list_first_entry_or_null((struct list_head *) &dpll->registration_list, >+ struct dpll_device_registration, list); >+ WARN_ON(!reg); >+ return reg; >+} >+ >+/** >+ * dpll_priv - get the dpll device private owner data >+ * @dpll: registered dpll pointer >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: pointer to the first registration priv data >+ */ >+void *dpll_priv(struct dpll_device *dpll) >+{ >+ struct dpll_device_registration *reg; >+ >+ reg = dpll_device_registration_first(dpll); >+ return reg->priv; >+} >+ >+/** >+ * dpll_device_ops - get the dpll device ops pointer >+ * @dpll: registered dpll pointer >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: pointer to the first registration ops of the dpll >+ */ >+const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll) >+{ >+ struct dpll_device_registration *reg; >+ >+ reg = dpll_device_registration_first(dpll); >+ return reg->ops; >+} >+ >+/** >+ * dpll_pin_registration_first - get first registration of dpll pin ref >+ * @ref: pointer to a pin ref struct >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: pointer to the first registration of a dpll_pin_ref >+ */ >+static struct dpll_pin_registration * >+dpll_pin_registration_first(struct dpll_pin_ref *ref) >+{ >+ struct dpll_pin_registration *reg; >+ >+ reg = list_first_entry_or_null(&ref->registration_list, >+ struct dpll_pin_registration, list); >+ WARN_ON(!reg); >+ return reg; >+} >+ >+/** >+ * dpll_pin_on_dpll_priv - get the dpll device private owner data >+ * @dpll: registered dpll pointer >+ * @pin: pointer to a pin >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: pointer to the data >+ */ >+void *dpll_pin_on_dpll_priv(struct dpll_device *dpll, >+ struct dpll_pin *pin) >+{ >+ struct dpll_pin_registration *reg; >+ struct dpll_pin_ref *ref; >+ >+ ref = xa_load(&dpll->pin_refs, pin->pin_idx); >+ if (!ref) >+ return NULL; >+ reg = dpll_pin_registration_first(ref); >+ return reg->priv; >+} >+ >+/** >+ * dpll_pin_on_pin_priv - get the dpll pin private owner data >+ * @parent: pointer to a parent pin >+ * @pin: pointer to a pin >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: pointer to the data >+ */ >+void *dpll_pin_on_pin_priv(struct dpll_pin *parent, >+ struct dpll_pin *pin) >+{ >+ struct dpll_pin_registration *reg; >+ struct dpll_pin_ref *ref; >+ >+ ref = xa_load(&pin->parent_refs, parent->pin_idx); >+ if (!ref) >+ return NULL; >+ reg = dpll_pin_registration_first(ref); >+ return reg->priv; >+} >+ >+/** >+ * dpll_pin_ops - get the pin ops pointer >+ * @ref: dpll pin ref >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: pointer to the first ops registered with the pin >+ */ >+const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref) >+{ >+ struct dpll_pin_registration *reg; >+ >+ reg = dpll_pin_registration_first(ref); >+ return reg->ops; >+} >+ >+/** >+ * dpll_init - initialize dpll subsystem >+ * >+ * Return: >+ * 0 - success >+ * negative - netlink init error >+ */ >+static int __init dpll_init(void) >+{ >+ int ret; >+ >+ ret = dpll_netlink_init(); >+ if (ret) >+ goto error; >+ >+ return 0; >+ >+error: >+ mutex_destroy(&dpll_lock); >+ return ret; >+} >+subsys_initcall(dpll_init); >diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h >new file mode 100644 >index 000000000000..ef95e272937c >--- /dev/null >+++ b/drivers/dpll/dpll_core.h >@@ -0,0 +1,104 @@ >+/* SPDX-License-Identifier: GPL-2.0 */ >+/* >+ * Copyright (c) 2023 Meta Platforms, Inc. and affiliates >+ * Copyright (c) 2023 Intel and affiliates >+ */ >+ >+#ifndef __DPLL_CORE_H__ >+#define __DPLL_CORE_H__ >+ >+#include <linux/dpll.h> >+#include <linux/list.h> >+#include <linux/refcount.h> >+#include "dpll_netlink.h" >+ >+#define DPLL_REGISTERED XA_MARK_1 >+ >+struct dpll_device_registration { >+ struct list_head list; >+ const struct dpll_device_ops *ops; >+ void *priv; You don't need this struct in the header. Move to .c >+}; >+ >+/** >+ * struct dpll_device - stores DPLL device internal data >+ * @id: unique id number for each device given by kernel Who's kernel? Say "dpll subsystem" instead. >+ * @device_idx: id given by dev driver >+ * @clock_id: unique identifier (clock_id) of a dpll >+ * @module: module of creator >+ * @type: type of a dpll >+ * @pin_refs: stores pins registered within a dpll >+ * @mode_supported_mask: mask of supported modes >+ * @refcount: refcount >+ * @registration_list: list of registered ops and priv data of dpll owners >+ **/ >+struct dpll_device { >+ u32 id; >+ u32 device_idx; >+ u64 clock_id; >+ struct module *module; >+ enum dpll_type type; >+ struct xarray pin_refs; >+ unsigned long mode_supported_mask; >+ refcount_t refcount; >+ struct list_head registration_list; >+}; >+ >+/** >+ * struct dpll_pin - structure for a dpll pin >+ * @id: unique id number for pin given by kernel Who's kernel? Say "dpll subsystem" instead. >+ * @pin_idx: index of a pin given by dev driver >+ * @clock_id: clock_id of creator >+ * @module: module of creator >+ * @dpll_refs: hold referencees to dplls pin was registered with >+ * @parent_refs: hold references to parent pins pin was registered with >+ * @prop: pointer to pin properties given by registerer >+ * @rclk_dev_name: holds name of device when pin can recover clock from it >+ * @refcount: 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; >+}; >+ >+struct dpll_pin_registration { >+ struct list_head list; >+ const struct dpll_pin_ops *ops; >+ void *priv; >+}; You don't need this struct in the header. Move to .c >+ >+/** >+ * struct dpll_pin_ref - structure for referencing either dpll or pins >+ * @dpll: pointer to a dpll >+ * @pin: pointer to a pin >+ * @registration_list: list of ops and priv data registered with the ref >+ * @refcount: refcount >+ **/ >+struct dpll_pin_ref { >+ union { >+ struct dpll_device *dpll; >+ struct dpll_pin *pin; >+ }; >+ struct list_head registration_list; >+ refcount_t refcount; >+}; >+ >+void *dpll_priv(struct dpll_device *dpll); >+void *dpll_pin_on_dpll_priv(struct dpll_device *dpll, struct dpll_pin *pin); >+void *dpll_pin_on_pin_priv(struct dpll_pin *parent, struct dpll_pin *pin); >+ >+const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll); >+struct dpll_device *dpll_device_get_by_id(int id); >+const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref); >+struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs); >+extern struct xarray dpll_device_xa; >+extern struct xarray dpll_pin_xa; >+extern struct mutex dpll_lock; >+#endif >-- >2.37.3 >
Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com wrote: >From: Vadim Fedorenko <vadim.fedorenko@linux.dev> [...] >+int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, >+ const struct dpll_device_ops *ops, void *priv) >+{ >+ struct dpll_device_registration *reg; >+ bool first_registration = false; >+ >+ if (WARN_ON(!ops)) >+ return -EINVAL; >+ if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) >+ return -EINVAL; >+ >+ mutex_lock(&dpll_lock); >+ reg = dpll_device_registration_find(dpll, ops, priv); >+ if (reg) { >+ mutex_unlock(&dpll_lock); >+ return -EEXIST; >+ } >+ >+ reg = kzalloc(sizeof(*reg), GFP_KERNEL); >+ if (!reg) { >+ mutex_unlock(&dpll_lock); >+ return -EEXIST; >+ } >+ reg->ops = ops; >+ reg->priv = priv; >+ dpll->type = type; >+ first_registration = list_empty(&dpll->registration_list); >+ list_add_tail(®->list, &dpll->registration_list); >+ if (!first_registration) { >+ mutex_unlock(&dpll_lock); >+ return 0; >+ } >+ >+ xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); >+ mutex_unlock(&dpll_lock); >+ dpll_device_create_ntf(dpll); This function is introduced in the next patch. Breaks bissection. Make sure you can compile the code after every patch applied. >+ >+ return 0; >+} [...]
Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com wrote: >From: Vadim Fedorenko <vadim.fedorenko@linux.dev> [...] >+ * dpll_xa_ref_dpll_first - find first record of given xarray >+ * @xa_refs: xarray >+ * >+ * Context: shall be called under a lock (dpll_lock) >+ * Return: first element on given xaaray typo: xarray >+ */ >+struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs) [...] >+/** >+ * dpll_device_get - find existing or create new dpll device >+ * @clock_id: clock_id of creator >+ * @device_idx: idx given by device driver >+ * @module: reference to registering module >+ * >+ * Get existing object of a dpll device, unique for given arguments. >+ * Create new if doesn't exist yet. >+ * >+ * Context: Acquires a lock (dpll_lock) >+ * Return: >+ * * valid dpll_device struct pointer if succeeded >+ * * ERR_PTR(-ENOMEM) - failed memory allocation Yeah, that is kind of obvious, isn't? Really, drop this pointless coments. >+ * * ERR_PTR(X) - failed allocation on dpll's xa >+ */ >+struct dpll_device * >+dpll_device_get(u64 clock_id, u32 device_idx, struct module *module) [...] >+/** >+ * dpll_pin_register - register the dpll pin in the subsystem >+ * @dpll: pointer to a dpll >+ * @pin: pointer to a dpll pin >+ * @ops: ops for a dpll pin ops >+ * @priv: pointer to private information of owner >+ * >+ * Context: Acquires a lock (dpll_lock) >+ * Return: >+ * * 0 on success >+ * * -EINVAL - missing pin ops >+ * * -ENOMEM - failed to allocate memory Does not make sense to assign one errno to one specific error. Avoid tables like this. >+ */ >+int >+dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, >+ const struct dpll_pin_ops *ops, void *priv) >+{ >+ int ret; >+ >+ mutex_lock(&dpll_lock); >+ if (WARN_ON(!(dpll->module == pin->module && >+ dpll->clock_id == pin->clock_id))) >+ ret = -EFAULT; -EINVAL; >+ else >+ ret = __dpll_pin_register(dpll, pin, ops, priv); >+ mutex_unlock(&dpll_lock); >+ >+ return ret; >+} [...]
On Sun, 2023-06-11 at 11:36 +0200, Jiri Pirko wrote: > Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com wrote: > > From: Vadim Fedorenko <vadim.fedorenko@linux.dev> > > [...] > > > +int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, > > + const struct dpll_device_ops *ops, void *priv) > > +{ > > + struct dpll_device_registration *reg; > > + bool first_registration = false; > > + > > + if (WARN_ON(!ops)) > > + return -EINVAL; > > + if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) > > + return -EINVAL; > > + > > + mutex_lock(&dpll_lock); > > + reg = dpll_device_registration_find(dpll, ops, priv); > > + if (reg) { > > + mutex_unlock(&dpll_lock); > > + return -EEXIST; > > + } > > + > > + reg = kzalloc(sizeof(*reg), GFP_KERNEL); > > + if (!reg) { > > + mutex_unlock(&dpll_lock); > > + return -EEXIST; > > + } > > + reg->ops = ops; > > + reg->priv = priv; > > + dpll->type = type; > > + first_registration = list_empty(&dpll->registration_list); > > + list_add_tail(®->list, &dpll->registration_list); > > + if (!first_registration) { > > + mutex_unlock(&dpll_lock); > > + return 0; > > + } > > + > > + xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); > > + mutex_unlock(&dpll_lock); > > + dpll_device_create_ntf(dpll); > > This function is introduced in the next patch. Breaks bissection. Make > sure you can compile the code after every patch applied. WRT, I think the easiest way to solve the above is adding the function call in the next patch. Cheers, Paolo
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? > + if (!reg) { > + if (!ref_exists) > + kfree(ref); ref has already been inserted into xa_pins > + return -ENOMEM;
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Saturday, June 10, 2023 7:38 PM > >Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> >>DPLL framework is used to represent and configure DPLL devices >>in systems. Each device that has DPLL and can configure inputs >>and outputs can use this framework. >> >>Implement core framework functions for further interactions >>with device drivers implementing dpll subsystem, as well as for >>interactions of DPLL netlink framework part with the subsystem >>itself. >> >>Co-developed-by: Milena Olech <milena.olech@intel.com> >>Signed-off-by: Milena Olech <milena.olech@intel.com> >>Co-developed-by: Michal Michalik <michal.michalik@intel.com> >>Signed-off-by: Michal Michalik <michal.michalik@intel.com> >>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>--- >> drivers/dpll/dpll_core.c | 953 +++++++++++++++++++++++++++++++++++++++ >> drivers/dpll/dpll_core.h | 104 +++++ > >Overall, looks very good! I pinpointed couple of nits below, nothing big. >General question: Why do you put documentation comment to every static >function? Does not make any sense to me. Even for non-exported functions >I think it is overkill. Most of them (if not all) give the reader no >additional information and only make the code a bit harder to read. >Care to drop them? > > >> 2 files changed, 1057 insertions(+) >> create mode 100644 drivers/dpll/dpll_core.c >> create mode 100644 drivers/dpll/dpll_core.h >> >>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c >>new file mode 100644 >>index 000000000000..ee515b7c18be >>--- /dev/null >>+++ b/drivers/dpll/dpll_core.c >>@@ -0,0 +1,953 @@ >>+// SPDX-License-Identifier: GPL-2.0 >>+/* >>+ * dpll_core.c - Generic DPLL Management class support. > >What's "management class support"? Does not sound recognizable. > Fixed. > >>+ * >>+ * Copyright (c) 2023 Meta Platforms, Inc. and affiliates >>+ * Copyright (c) 2023 Intel Corporation. >>+ */ >>+ >>+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>+ >>+#include <linux/device.h> >>+#include <linux/err.h> >>+#include <linux/slab.h> >>+#include <linux/string.h> >>+ >>+#include "dpll_core.h" >>+ >>+DEFINE_MUTEX(dpll_lock); >>+ >>+DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC); >>+DEFINE_XARRAY_FLAGS(dpll_pin_xa, XA_FLAGS_ALLOC); >>+ >>+#define ASSERT_DPLL_REGISTERED(d) \ >>+ WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED)) >>+#define ASSERT_DPLL_NOT_REGISTERED(d) \ >>+ WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED)) >>+ >>+/** >>+ * dpll_device_get_by_id - find dpll device by it's id >>+ * @id: id of searched dpll >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * dpll_device struct if found >>+ * * NULL otherwise >>+ */ >>+struct dpll_device *dpll_device_get_by_id(int id) >>+{ >>+ if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED)) >>+ return xa_load(&dpll_device_xa, id); >>+ >>+ return NULL; >>+} >>+ >>+/** >>+ * dpll_pin_registration_find - find a pin registration record >>+ * @ref: reference between dpll and pin >>+ * @ops: searched pin ops pointer >>+ * @priv: searched pin priv pointer >>+ * >>+ * Search dpll's registered pins for given ops and priv data. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * NULL - if pin was not found >>+ * * pointer to `struct dpll_pin_registration` if found >>+ */ >>+static struct dpll_pin_registration * >>+dpll_pin_registration_find(struct dpll_pin_ref *ref, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ struct dpll_pin_registration *reg; >>+ >>+ list_for_each_entry(reg, &ref->registration_list, list) { >>+ if (reg->ops == ops && reg->priv == priv) >>+ return reg; >>+ } >>+ return NULL; >>+} >>+ >>+/** >>+ * dpll_xa_ref_pin_add - add pin reference to a given xarray >>+ * @xa_pins: dpll_pin_ref xarray holding pins >>+ * @pin: pin being added >>+ * @ops: ops for a pin >>+ * @priv: pointer to private data of owner >>+ * >>+ * Allocate and create reference of a pin and enlist a registration >>+ * structure storing ops and priv pointers of a caller registant. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * 0 on success >>+ * * -ENOMEM on failed allocation >>+ */ >>+static int >>+dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ struct dpll_pin_registration *reg; >>+ struct dpll_pin_ref *ref; >>+ bool ref_exists = false; >>+ unsigned long i; >>+ int ret; >>+ >>+ 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); >>+ if (!reg) { >>+ if (!ref_exists) >>+ kfree(ref); >>+ return -ENOMEM; >>+ } >>+ reg->ops = ops; >>+ reg->priv = priv; >>+ if (ref_exists) > >If you set refcount to 0 couple of lines above, you can drop the check >here. > Fixed. > >>+ refcount_inc(&ref->refcount); >>+ list_add_tail(®->list, &ref->registration_list); >>+ >>+ return 0; >>+} >>+ >>+/** >>+ * dpll_xa_ref_pin_del - remove reference of a pin from xarray >>+ * @xa_pins: dpll_pin_ref xarray holding pins >>+ * @pin: pointer to a pin being removed >>+ * @ops: pointer to ops of pin being removed >>+ * @priv: pointer to private data of registerer who invoked pin removal >>+ * >>+ * Decrement refcount of existing pin reference on given xarray. >>+ * If all registrations are lifted delete the reference and free its memory. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * 0 on success >>+ * * -EINVAL if reference to a pin was not found >>+ */ >>+static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin >>*pin, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ struct dpll_pin_registration *reg; >>+ struct dpll_pin_ref *ref; >>+ unsigned long i; >>+ >>+ xa_for_each(xa_pins, i, ref) { >>+ if (ref->pin != pin) >>+ continue; >>+ reg = dpll_pin_registration_find(ref, ops, priv); >>+ if (WARN_ON(!reg)) >>+ return -EINVAL; >>+ if (refcount_dec_and_test(&ref->refcount)) { >>+ list_del(®->list); >>+ kfree(reg); >>+ xa_erase(xa_pins, i); >>+ WARN_ON(!list_empty(&ref->registration_list)); >>+ kfree(ref); >>+ } >>+ return 0; >>+ } >>+ >>+ return -EINVAL; >>+} >>+ >>+/** >>+ * dpll_xa_ref_dpll_add - add dpll reference to a given xarray >>+ * @xa_dplls: dpll_pin_ref xarray holding dplls >>+ * @dpll: dpll being added >>+ * @ops: pin-reference ops for a dpll >>+ * @priv: pointer to private data of owner >>+ * >>+ * Allocate and create reference of a dpll-pin ops or increase refcount >>+ * on existing dpll reference on given xarray. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * 0 on success >>+ * * -ENOMEM on failed allocation >>+ */ >>+static int >>+dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ struct dpll_pin_registration *reg; >>+ struct dpll_pin_ref *ref; >>+ bool ref_exists = false; >>+ unsigned long i; >>+ int ret; >>+ >>+ xa_for_each(xa_dplls, i, ref) { >>+ if (ref->dpll != dpll) >>+ 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->dpll = dpll; >>+ INIT_LIST_HEAD(&ref->registration_list); >>+ ret = xa_insert(xa_dplls, dpll->device_idx, ref, GFP_KERNEL); >>+ if (ret) { >>+ kfree(ref); >>+ return ret; >>+ } >>+ refcount_set(&ref->refcount, 1); >>+ } >>+ >>+ reg = kzalloc(sizeof(*reg), GFP_KERNEL); >>+ if (!reg) { >>+ if (!ref_exists) >>+ kfree(ref); >>+ return -ENOMEM; >>+ } >>+ reg->ops = ops; >>+ reg->priv = priv; >>+ if (ref_exists) > >If you set refcount to 0 couple of lines above, you can drop the check >here. > Fixed. > >>+ refcount_inc(&ref->refcount); >>+ list_add_tail(®->list, &ref->registration_list); >>+ >>+ return 0; >>+} >>+ >>+/** >>+ * dpll_xa_ref_dpll_del - remove reference of a dpll from xarray >>+ * @xa_dplls: dpll_pin_ref xarray holding dplls >>+ * @dpll: pointer to a dpll to remove >>+ * @ops: pointer to ops of dpll being removed >>+ * @priv: pointer to private data of registerer who invoked dpll removal >>+ * >>+ * Decrement refcount of existing dpll reference on given xarray. >>+ * If all references are dropped, delete the reference and free its memory. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ */ >>+static void >>+dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ struct dpll_pin_registration *reg; >>+ struct dpll_pin_ref *ref; >>+ unsigned long i; >>+ >>+ xa_for_each(xa_dplls, i, ref) { >>+ if (ref->dpll != dpll) >>+ continue; >>+ reg = dpll_pin_registration_find(ref, ops, priv); >>+ if (WARN_ON(!reg)) >>+ return; >>+ if (refcount_dec_and_test(&ref->refcount)) { >>+ list_del(®->list); >>+ kfree(reg); >>+ xa_erase(xa_dplls, i); >>+ WARN_ON(!list_empty(&ref->registration_list)); >>+ kfree(ref); >>+ } >>+ return; >>+ } >>+} >>+ >>+/** >>+ * dpll_xa_ref_dpll_find - find dpll reference on xarray >>+ * @xa_refs: dpll_pin_ref xarray holding dpll references >>+ * @dpll: pointer to a dpll being searched >>+ * >>+ * Search for dpll-pin ops reference struct of a given dpll on given xarray. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * pin reference struct pointer on success >>+ * * NULL - reference to a pin was not found >>+ */ >>+struct dpll_pin_ref * >>+dpll_xa_ref_dpll_find(struct xarray *xa_refs, const struct dpll_device *dpll) >>+{ >>+ struct dpll_pin_ref *ref; >>+ unsigned long i; >>+ >>+ xa_for_each(xa_refs, i, ref) { >>+ if (ref->dpll == dpll) >>+ return ref; >>+ } >>+ >>+ return NULL; >>+} >>+ >>+/** >>+ * dpll_xa_ref_dpll_first - find first record of given xarray >>+ * @xa_refs: xarray >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: first element on given xaaray >>+ */ >>+struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs) >>+{ >>+ struct dpll_pin_ref *ref; >>+ unsigned long i = 0; >>+ >>+ ref = xa_find(xa_refs, &i, ULONG_MAX, XA_PRESENT); >>+ WARN_ON(!ref); >>+ return ref; >>+} >>+ >>+/** >>+ * dpll_device_alloc - allocate the memory for dpll device >>+ * @clock_id: clock_id of creator >>+ * @device_idx: id given by dev driver >>+ * @module: reference to registering module >>+ * >>+ * Allocates memory and initialize dpll device, hold its reference on >>global >>+ * xarray. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * valid dpll_device struct pointer if succeeded >>+ * * ERR_PTR(-ENOMEM) - failed memory allocation >>+ * * ERR_PTR(X) - failed allocation on dpll's xa >>+ */ >>+static struct dpll_device * >>+dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module) >>+{ >>+ struct dpll_device *dpll; >>+ int ret; >>+ >>+ dpll = kzalloc(sizeof(*dpll), GFP_KERNEL); >>+ if (!dpll) >>+ return ERR_PTR(-ENOMEM); >>+ refcount_set(&dpll->refcount, 1); >>+ INIT_LIST_HEAD(&dpll->registration_list); >>+ dpll->device_idx = device_idx; >>+ dpll->clock_id = clock_id; >>+ dpll->module = module; >>+ ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, >>+ GFP_KERNEL); >>+ if (ret) { >>+ kfree(dpll); >>+ return ERR_PTR(ret); >>+ } >>+ xa_init_flags(&dpll->pin_refs, XA_FLAGS_ALLOC); >>+ >>+ return dpll; >>+} >>+ >>+/** >>+ * dpll_device_get - find existing or create new dpll device >>+ * @clock_id: clock_id of creator >>+ * @device_idx: idx given by device driver >>+ * @module: reference to registering module >>+ * >>+ * Get existing object of a dpll device, unique for given arguments. >>+ * Create new if doesn't exist yet. >>+ * >>+ * Context: Acquires a lock (dpll_lock) >>+ * Return: >>+ * * valid dpll_device struct pointer if succeeded >>+ * * ERR_PTR(-ENOMEM) - failed memory allocation >>+ * * ERR_PTR(X) - failed allocation on dpll's xa >>+ */ >>+struct dpll_device * >>+dpll_device_get(u64 clock_id, u32 device_idx, struct module *module) >>+{ >>+ struct dpll_device *dpll, *ret = NULL; >>+ unsigned long index; >>+ >>+ mutex_lock(&dpll_lock); >>+ xa_for_each(&dpll_device_xa, index, dpll) { >>+ if (dpll->clock_id == clock_id && >>+ dpll->device_idx == device_idx && >>+ dpll->module == module) { >>+ ret = dpll; >>+ refcount_inc(&ret->refcount); >>+ break; >>+ } >>+ } >>+ if (!ret) >>+ ret = dpll_device_alloc(clock_id, device_idx, module); >>+ mutex_unlock(&dpll_lock); >>+ >>+ return ret; >>+} >>+EXPORT_SYMBOL_GPL(dpll_device_get); >>+ >>+/** >>+ * dpll_device_put - decrease the refcount and free memory if possible >>+ * @dpll: dpll_device struct pointer >>+ * >>+ * Context: Acquires a lock (dpll_lock) >>+ * Drop reference for a dpll device, if all references are gone, delete >>+ * dpll device object. >>+ */ >>+void dpll_device_put(struct dpll_device *dpll) >>+{ >>+ if (!dpll) > >Drop this check and rely on a basic driver sanity. I'm pretty sure I >asked you to drop checks like this during the last version review. > Fixed. > >>+ return; >>+ mutex_lock(&dpll_lock); >>+ if (refcount_dec_and_test(&dpll->refcount)) { >>+ ASSERT_DPLL_NOT_REGISTERED(dpll); >>+ WARN_ON_ONCE(!xa_empty(&dpll->pin_refs)); >>+ xa_destroy(&dpll->pin_refs); >>+ xa_erase(&dpll_device_xa, dpll->id); >>+ WARN_ON(!list_empty(&dpll->registration_list)); >>+ kfree(dpll); >>+ } >>+ mutex_unlock(&dpll_lock); >>+} >>+EXPORT_SYMBOL_GPL(dpll_device_put); >>+ >>+static struct dpll_device_registration * >>+dpll_device_registration_find(struct dpll_device *dpll, >>+ const struct dpll_device_ops *ops, void *priv) >>+{ >>+ struct dpll_device_registration *reg; >>+ >>+ list_for_each_entry(reg, &dpll->registration_list, list) { >>+ if (reg->ops == ops && reg->priv == priv) >>+ return reg; >>+ } >>+ return NULL; >>+} >>+ >>+/** >>+ * dpll_device_register - register the dpll device in the subsystem >>+ * @dpll: pointer to a dpll >>+ * @type: type of a dpll >>+ * @ops: ops for a dpll device >>+ * @priv: pointer to private information of owner >>+ * >>+ * Make dpll device available for user space. >>+ * >>+ * Context: Acquires a lock (dpll_lock) >>+ * Return: >>+ * * 0 on success >>+ * * -EINVAL on failure due to wrong arguments provided >>+ * * -EEXIST if device was already registered >>+ */ >>+int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, >>+ const struct dpll_device_ops *ops, void *priv) >>+{ >>+ struct dpll_device_registration *reg; >>+ bool first_registration = false; >>+ >>+ if (WARN_ON(!ops)) >>+ return -EINVAL; >>+ if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) >>+ return -EINVAL; >>+ >>+ mutex_lock(&dpll_lock); >>+ reg = dpll_device_registration_find(dpll, ops, priv); >>+ if (reg) { >>+ mutex_unlock(&dpll_lock); >>+ return -EEXIST; >>+ } >>+ >>+ reg = kzalloc(sizeof(*reg), GFP_KERNEL); >>+ if (!reg) { >>+ mutex_unlock(&dpll_lock); >>+ return -EEXIST; > >-ENOMEM; > Fixed. > >>+ } >>+ reg->ops = ops; >>+ reg->priv = priv; >>+ dpll->type = type; >>+ first_registration = list_empty(&dpll->registration_list); >>+ list_add_tail(®->list, &dpll->registration_list); >>+ if (!first_registration) { >>+ mutex_unlock(&dpll_lock); >>+ return 0; >>+ } >>+ >>+ xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); >>+ mutex_unlock(&dpll_lock); >>+ dpll_device_create_ntf(dpll); >>+ >>+ return 0; >>+} >>+EXPORT_SYMBOL_GPL(dpll_device_register); >>+ >>+/** >>+ * dpll_device_unregister - unregister dpll device >>+ * @dpll: registered dpll pointer >>+ * @ops: ops for a dpll device >>+ * @priv: pointer to private information of owner >>+ * >>+ * Unregister device, make it unavailable for userspace. >>+ * Note: It does not free the memory >>+ * Context: Acquires a lock (dpll_lock) >>+ */ >>+void dpll_device_unregister(struct dpll_device *dpll, >>+ const struct dpll_device_ops *ops, void *priv) >>+{ >>+ struct dpll_device_registration *reg; >>+ >>+ mutex_lock(&dpll_lock); >>+ ASSERT_DPLL_REGISTERED(dpll); >>+ dpll_device_delete_ntf(dpll); >>+ reg = dpll_device_registration_find(dpll, ops, priv); >>+ if (WARN_ON(!reg)) { >>+ mutex_unlock(&dpll_lock); >>+ return; >>+ } >>+ list_del(®->list); >>+ kfree(reg); >>+ >>+ if (!list_empty(&dpll->registration_list)) { >>+ mutex_unlock(&dpll_lock); >>+ return; >>+ } >>+ xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); >>+ mutex_unlock(&dpll_lock); >>+} >>+EXPORT_SYMBOL_GPL(dpll_device_unregister); >>+ >>+/** >>+ * dpll_pin_alloc - allocate the memory for dpll pin >>+ * @clock_id: clock_id of creator >>+ * @pin_idx: idx given by dev driver >>+ * @module: reference to registering module >>+ * @prop: dpll pin properties >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * valid allocated dpll_pin struct pointer if succeeded >>+ * * ERR_PTR(-ENOMEM) - failed memory allocation >>+ * * ERR_PTR(-EINVAL) - wrong pin type property value >>+ */ >>+static struct dpll_pin * >>+dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module, >>+ const struct dpll_pin_properties *prop) >>+{ >>+ struct dpll_pin *pin; >>+ int ret; >>+ >>+ pin = kzalloc(sizeof(*pin), GFP_KERNEL); >>+ if (!pin) >>+ return ERR_PTR(-ENOMEM); >>+ pin->pin_idx = pin_idx; >>+ pin->clock_id = clock_id; >>+ pin->module = module; >>+ if (WARN_ON(prop->type < DPLL_PIN_TYPE_MUX || >>+ prop->type > DPLL_PIN_TYPE_MAX)) { >>+ ret = -EINVAL; >>+ goto err; >>+ } >>+ pin->prop = prop; >>+ refcount_set(&pin->refcount, 1); >>+ xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC); >>+ xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC); >>+ ret = xa_alloc(&dpll_pin_xa, &pin->id, pin, xa_limit_16b, GFP_KERNEL); >>+ if (ret) >>+ goto err; >>+ return pin; >>+err: >>+ xa_destroy(&pin->dpll_refs); >>+ xa_destroy(&pin->parent_refs); >>+ kfree(pin); >>+ return ERR_PTR(ret); >>+} >>+ >>+/** >>+ * dpll_pin_get - find existing or create new dpll pin >>+ * @clock_id: clock_id of creator >>+ * @pin_idx: idx given by dev driver >>+ * @module: reference to registering module >>+ * @prop: dpll pin properties >>+ * >>+ * Get existing object of a pin (unique for given arguments) or create new >>+ * if doesn't exist yet. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: >>+ * * valid allocated dpll_pin struct pointer if succeeded >>+ * * ERR_PTR of an error >>+ */ >>+struct dpll_pin * >>+dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, >>+ const struct dpll_pin_properties *prop) >>+{ >>+ struct dpll_pin *pos, *ret = NULL; >>+ unsigned long i; >>+ >>+ xa_for_each(&dpll_pin_xa, i, pos) { >>+ if (pos->clock_id == clock_id && >>+ pos->pin_idx == pin_idx && >>+ pos->module == module) { >>+ ret = pos; >>+ refcount_inc(&ret->refcount); >>+ break; >>+ } >>+ } >>+ if (!ret) >>+ ret = dpll_pin_alloc(clock_id, pin_idx, module, prop); >>+ >>+ return ret; >>+} >>+EXPORT_SYMBOL_GPL(dpll_pin_get); >>+ >>+/** >>+ * dpll_pin_put - decrease the refcount and free memory if possible >>+ * @pin: pointer to a pin to be put >>+ * >>+ * Drop reference for a pin, if all references are gone, delete pin object. >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ */ >>+void dpll_pin_put(struct dpll_pin *pin) >>+{ >>+ if (!pin) > >Drop this check. > Fixed. > >>+ return; >>+ if (refcount_dec_and_test(&pin->refcount)) { >>+ xa_destroy(&pin->dpll_refs); >>+ xa_destroy(&pin->parent_refs); >>+ xa_erase(&dpll_pin_xa, pin->id); >>+ kfree(pin); >>+ } >>+} >>+EXPORT_SYMBOL_GPL(dpll_pin_put); >>+ >>+static int >>+__dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ int ret; >>+ >>+ if (WARN_ON(!ops)) >>+ return -EINVAL; >>+ >>+ ret = dpll_xa_ref_pin_add(&dpll->pin_refs, pin, ops, priv); >>+ if (ret) >>+ return ret; >>+ ret = dpll_xa_ref_dpll_add(&pin->dpll_refs, dpll, ops, priv); >>+ if (ret) >>+ goto ref_pin_del; >>+ else >>+ dpll_pin_create_ntf(pin); >>+ >>+ return ret; >>+ >>+ref_pin_del: >>+ dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv); >>+ return ret; >>+} >>+ >>+/** >>+ * dpll_pin_register - register the dpll pin in the subsystem >>+ * @dpll: pointer to a dpll >>+ * @pin: pointer to a dpll pin >>+ * @ops: ops for a dpll pin ops >>+ * @priv: pointer to private information of owner >>+ * >>+ * Context: Acquires a lock (dpll_lock) >>+ * Return: >>+ * * 0 on success >>+ * * -EINVAL - missing pin ops >>+ * * -ENOMEM - failed to allocate memory >>+ */ >>+int >>+dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ int ret; >>+ >>+ mutex_lock(&dpll_lock); >>+ if (WARN_ON(!(dpll->module == pin->module && >>+ dpll->clock_id == pin->clock_id))) >>+ ret = -EFAULT; > >In other cases like this you return -EINVAL. I think it is better. One >way or another, be consistent please. > Fixed. > >>+ else >>+ ret = __dpll_pin_register(dpll, pin, ops, priv); >>+ mutex_unlock(&dpll_lock); >>+ >>+ return ret; >>+} >>+EXPORT_SYMBOL_GPL(dpll_pin_register); >>+ >>+static void >>+__dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv); >>+ dpll_xa_ref_dpll_del(&pin->dpll_refs, dpll, ops, priv); >>+} >>+ >>+/** >>+ * dpll_pin_unregister - unregister dpll pin from dpll device >>+ * @dpll: registered dpll pointer >>+ * @pin: pointer to a pin >>+ * @ops: ops for a dpll pin >>+ * @priv: pointer to private information of owner >>+ * >>+ * Note: It does not free the memory >>+ * Context: Acquires a lock (dpll_lock) >>+ */ >>+void dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ if (WARN_ON(xa_empty(&dpll->pin_refs))) >>+ return; >>+ >>+ mutex_lock(&dpll_lock); >>+ __dpll_pin_unregister(dpll, pin, ops, priv); >>+ mutex_unlock(&dpll_lock); >>+} >>+EXPORT_SYMBOL_GPL(dpll_pin_unregister); >>+ >>+/** >>+ * dpll_pin_on_pin_register - register a pin with a parent pin >>+ * @parent: pointer to a parent pin >>+ * @pin: pointer to a pin >>+ * @ops: ops for a dpll pin >>+ * @priv: pointer to private information of owner >>+ * >>+ * Register a pin with a parent pin, create references between them and >>+ * between newly registered pin and dplls connected with a parent pin. >>+ * >>+ * Context: Acquires a lock (dpll_lock) >>+ * Return: >>+ * * 0 on success >>+ * * -EINVAL missing pin or parent >>+ * * -ENOMEM failed allocation >>+ * * -EPERM if parent is not allowed >>+ */ >>+int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ struct dpll_pin_ref *ref; >>+ unsigned long i, stop; >>+ int ret; >>+ >>+ if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX)) >>+ return -EINVAL; >>+ ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv); >>+ if (ret) >>+ goto unlock; >>+ refcount_inc(&pin->refcount); >>+ xa_for_each(&parent->dpll_refs, i, ref) { >>+ mutex_lock(&dpll_lock); >>+ ret = __dpll_pin_register(ref->dpll, pin, ops, priv); >>+ mutex_unlock(&dpll_lock); >>+ if (ret) { >>+ stop = i; >>+ goto dpll_unregister; >>+ } >>+ dpll_pin_create_ntf(pin); >>+ } >>+ >>+ return ret; >>+ >>+dpll_unregister: >>+ xa_for_each(&parent->dpll_refs, i, ref) { >>+ if (i < stop) { >>+ mutex_lock(&dpll_lock); >>+ __dpll_pin_unregister(ref->dpll, pin, ops, priv); >>+ mutex_unlock(&dpll_lock); >>+ } >>+ } >>+ refcount_dec(&pin->refcount); >>+ dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv); >>+unlock: >>+ return ret; >>+} >>+EXPORT_SYMBOL_GPL(dpll_pin_on_pin_register); >>+ >>+/** >>+ * dpll_pin_on_pin_unregister - unregister dpll pin from a parent pin >>+ * @parent: pointer to a parent pin >>+ * @pin: pointer to a pin >>+ * @ops: ops for a dpll pin >>+ * @priv: pointer to private information of owner >>+ * >>+ * Context: Acquires a lock (dpll_lock) >>+ * Note: It does not free the memory >>+ */ >>+void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin >>*pin, >>+ const struct dpll_pin_ops *ops, void *priv) >>+{ >>+ struct dpll_pin_ref *ref; >>+ unsigned long i; >>+ >>+ mutex_lock(&dpll_lock); >>+ dpll_pin_delete_ntf(pin); >>+ dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv); >>+ refcount_dec(&pin->refcount); >>+ xa_for_each(&pin->dpll_refs, i, ref) { >>+ __dpll_pin_unregister(ref->dpll, pin, ops, priv); >>+ } > >Drop "{}"s for one line statement. Didn't checkpack warn you? > Fixed. > >>+ mutex_unlock(&dpll_lock); >>+} >>+EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister); >>+ >>+ >>+/** >>+ * dpll_device_registration_first - get first registration of dpll device >>+ * @dpll: pointer to a dpll >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: pointer to the first registration of a dpll >>+ */ >>+static struct dpll_device_registration * >>+dpll_device_registration_first(struct dpll_device *dpll) >>+{ >>+ struct dpll_device_registration *reg; >>+ >>+ reg = list_first_entry_or_null((struct list_head *) &dpll- >>registration_list, >>+ struct dpll_device_registration, list); >>+ WARN_ON(!reg); >>+ return reg; >>+} >>+ >>+/** >>+ * dpll_priv - get the dpll device private owner data >>+ * @dpll: registered dpll pointer >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: pointer to the first registration priv data >>+ */ >>+void *dpll_priv(struct dpll_device *dpll) >>+{ >>+ struct dpll_device_registration *reg; >>+ >>+ reg = dpll_device_registration_first(dpll); >>+ return reg->priv; >>+} >>+ >>+/** >>+ * dpll_device_ops - get the dpll device ops pointer >>+ * @dpll: registered dpll pointer >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: pointer to the first registration ops of the dpll >>+ */ >>+const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll) >>+{ >>+ struct dpll_device_registration *reg; >>+ >>+ reg = dpll_device_registration_first(dpll); >>+ return reg->ops; >>+} >>+ >>+/** >>+ * dpll_pin_registration_first - get first registration of dpll pin ref >>+ * @ref: pointer to a pin ref struct >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: pointer to the first registration of a dpll_pin_ref >>+ */ >>+static struct dpll_pin_registration * >>+dpll_pin_registration_first(struct dpll_pin_ref *ref) >>+{ >>+ struct dpll_pin_registration *reg; >>+ >>+ reg = list_first_entry_or_null(&ref->registration_list, >>+ struct dpll_pin_registration, list); >>+ WARN_ON(!reg); >>+ return reg; >>+} >>+ >>+/** >>+ * dpll_pin_on_dpll_priv - get the dpll device private owner data >>+ * @dpll: registered dpll pointer >>+ * @pin: pointer to a pin >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: pointer to the data >>+ */ >>+void *dpll_pin_on_dpll_priv(struct dpll_device *dpll, >>+ struct dpll_pin *pin) >>+{ >>+ struct dpll_pin_registration *reg; >>+ struct dpll_pin_ref *ref; >>+ >>+ ref = xa_load(&dpll->pin_refs, pin->pin_idx); >>+ if (!ref) >>+ return NULL; >>+ reg = dpll_pin_registration_first(ref); >>+ return reg->priv; >>+} >>+ >>+/** >>+ * dpll_pin_on_pin_priv - get the dpll pin private owner data >>+ * @parent: pointer to a parent pin >>+ * @pin: pointer to a pin >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: pointer to the data >>+ */ >>+void *dpll_pin_on_pin_priv(struct dpll_pin *parent, >>+ struct dpll_pin *pin) >>+{ >>+ struct dpll_pin_registration *reg; >>+ struct dpll_pin_ref *ref; >>+ >>+ ref = xa_load(&pin->parent_refs, parent->pin_idx); >>+ if (!ref) >>+ return NULL; >>+ reg = dpll_pin_registration_first(ref); >>+ return reg->priv; >>+} >>+ >>+/** >>+ * dpll_pin_ops - get the pin ops pointer >>+ * @ref: dpll pin ref >>+ * >>+ * Context: shall be called under a lock (dpll_lock) >>+ * Return: pointer to the first ops registered with the pin >>+ */ >>+const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref) >>+{ >>+ struct dpll_pin_registration *reg; >>+ >>+ reg = dpll_pin_registration_first(ref); >>+ return reg->ops; >>+} >>+ >>+/** >>+ * dpll_init - initialize dpll subsystem >>+ * >>+ * Return: >>+ * 0 - success >>+ * negative - netlink init error >>+ */ >>+static int __init dpll_init(void) >>+{ >>+ int ret; >>+ >>+ ret = dpll_netlink_init(); >>+ if (ret) >>+ goto error; >>+ >>+ return 0; >>+ >>+error: >>+ mutex_destroy(&dpll_lock); >>+ return ret; >>+} >>+subsys_initcall(dpll_init); >>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h >>new file mode 100644 >>index 000000000000..ef95e272937c >>--- /dev/null >>+++ b/drivers/dpll/dpll_core.h >>@@ -0,0 +1,104 @@ >>+/* SPDX-License-Identifier: GPL-2.0 */ >>+/* >>+ * Copyright (c) 2023 Meta Platforms, Inc. and affiliates >>+ * Copyright (c) 2023 Intel and affiliates >>+ */ >>+ >>+#ifndef __DPLL_CORE_H__ >>+#define __DPLL_CORE_H__ >>+ >>+#include <linux/dpll.h> >>+#include <linux/list.h> >>+#include <linux/refcount.h> >>+#include "dpll_netlink.h" >>+ >>+#define DPLL_REGISTERED XA_MARK_1 >>+ >>+struct dpll_device_registration { >>+ struct list_head list; >>+ const struct dpll_device_ops *ops; >>+ void *priv; > >You don't need this struct in the header. Move to .c > Fixed. > >>+}; >>+ >>+/** >>+ * struct dpll_device - stores DPLL device internal data >>+ * @id: unique id number for each device given by kernel > >Who's kernel? Say "dpll subsystem" instead. > Fixed. > >>+ * @device_idx: id given by dev driver >>+ * @clock_id: unique identifier (clock_id) of a dpll >>+ * @module: module of creator >>+ * @type: type of a dpll >>+ * @pin_refs: stores pins registered within a dpll >>+ * @mode_supported_mask: mask of supported modes >>+ * @refcount: refcount >>+ * @registration_list: list of registered ops and priv data of dpll >>owners >>+ **/ >>+struct dpll_device { >>+ u32 id; >>+ u32 device_idx; >>+ u64 clock_id; >>+ struct module *module; >>+ enum dpll_type type; >>+ struct xarray pin_refs; >>+ unsigned long mode_supported_mask; >>+ refcount_t refcount; >>+ struct list_head registration_list; >>+}; >>+ >>+/** >>+ * struct dpll_pin - structure for a dpll pin >>+ * @id: unique id number for pin given by kernel > >Who's kernel? Say "dpll subsystem" instead. > Fixed. > >>+ * @pin_idx: index of a pin given by dev driver >>+ * @clock_id: clock_id of creator >>+ * @module: module of creator >>+ * @dpll_refs: hold referencees to dplls pin was registered with >>+ * @parent_refs: hold references to parent pins pin was registered with >>+ * @prop: pointer to pin properties given by registerer >>+ * @rclk_dev_name: holds name of device when pin can recover clock from it >>+ * @refcount: 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; >>+}; >>+ >>+struct dpll_pin_registration { >>+ struct list_head list; >>+ const struct dpll_pin_ops *ops; >>+ void *priv; >>+}; > >You don't need this struct in the header. Move to .c > Fixed. Thank you, Arkadiusz > >>+ >>+/** >>+ * struct dpll_pin_ref - structure for referencing either dpll or pins >>+ * @dpll: pointer to a dpll >>+ * @pin: pointer to a pin >>+ * @registration_list: list of ops and priv data registered with the >>ref >>+ * @refcount: refcount >>+ **/ >>+struct dpll_pin_ref { >>+ union { >>+ struct dpll_device *dpll; >>+ struct dpll_pin *pin; >>+ }; >>+ struct list_head registration_list; >>+ refcount_t refcount; >>+}; >>+ >>+void *dpll_priv(struct dpll_device *dpll); >>+void *dpll_pin_on_dpll_priv(struct dpll_device *dpll, struct dpll_pin *pin); >>+void *dpll_pin_on_pin_priv(struct dpll_pin *parent, struct dpll_pin *pin); >>+ >>+const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll); >>+struct dpll_device *dpll_device_get_by_id(int id); >>+const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref); >>+struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs); >>+extern struct xarray dpll_device_xa; >>+extern struct xarray dpll_pin_xa; >>+extern struct mutex dpll_lock; >>+#endif >>-- >>2.37.3 >>
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Saturday, June 10, 2023 7:38 PM > >Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> >>DPLL framework is used to represent and configure DPLL devices >>in systems. Each device that has DPLL and can configure inputs >>and outputs can use this framework. >> >>Implement core framework functions for further interactions >>with device drivers implementing dpll subsystem, as well as for >>interactions of DPLL netlink framework part with the subsystem >>itself. >> >>Co-developed-by: Milena Olech <milena.olech@intel.com> >>Signed-off-by: Milena Olech <milena.olech@intel.com> >>Co-developed-by: Michal Michalik <michal.michalik@intel.com> >>Signed-off-by: Michal Michalik <michal.michalik@intel.com> >>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>--- >> drivers/dpll/dpll_core.c | 953 +++++++++++++++++++++++++++++++++++++++ >> drivers/dpll/dpll_core.h | 104 +++++ > >Overall, looks very good! I pinpointed couple of nits below, nothing big. >General question: Why do you put documentation comment to every static >function? Does not make any sense to me. Even for non-exported functions >I think it is overkill. Most of them (if not all) give the reader no >additional information and only make the code a bit harder to read. >Care to drop them? > I forgot to respond here.. I would rather leave it, but if the others think the same way, we could remove them. Thank you! Arkadiusz [...]
>From: Paolo Abeni <pabeni@redhat.com> >Sent: Monday, June 12, 2023 9:25 AM > >On Sun, 2023-06-11 at 11:36 +0200, Jiri Pirko wrote: >> Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com >wrote: >> > From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> >> [...] >> >> > +int dpll_device_register(struct dpll_device *dpll, enum dpll_type >type, >> > + const struct dpll_device_ops *ops, void *priv) >> > +{ >> > + struct dpll_device_registration *reg; >> > + bool first_registration = false; >> > + >> > + if (WARN_ON(!ops)) >> > + return -EINVAL; >> > + if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) >> > + return -EINVAL; >> > + >> > + mutex_lock(&dpll_lock); >> > + reg = dpll_device_registration_find(dpll, ops, priv); >> > + if (reg) { >> > + mutex_unlock(&dpll_lock); >> > + return -EEXIST; >> > + } >> > + >> > + reg = kzalloc(sizeof(*reg), GFP_KERNEL); >> > + if (!reg) { >> > + mutex_unlock(&dpll_lock); >> > + return -EEXIST; >> > + } >> > + reg->ops = ops; >> > + reg->priv = priv; >> > + dpll->type = type; >> > + first_registration = list_empty(&dpll->registration_list); >> > + list_add_tail(®->list, &dpll->registration_list); >> > + if (!first_registration) { >> > + mutex_unlock(&dpll_lock); >> > + return 0; >> > + } >> > + >> > + xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); >> > + mutex_unlock(&dpll_lock); >> > + dpll_device_create_ntf(dpll); >> >> This function is introduced in the next patch. Breaks bissection. Make >> sure you can compile the code after every patch applied. > >WRT, I think the easiest way to solve the above is adding the function >call in the next patch. > >Cheers, > >Paolo Sure, will try to fix as suggested, but this will be much easier to do on final version of patches before sending. Thank you, Arkadiusz
>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.. 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, where each driver has own priv and ops. 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. 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;
Wed, Jun 21, 2023 at 08:55:35PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Saturday, June 10, 2023 7:38 PM >> >>Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>> >>>DPLL framework is used to represent and configure DPLL devices >>>in systems. Each device that has DPLL and can configure inputs >>>and outputs can use this framework. >>> >>>Implement core framework functions for further interactions >>>with device drivers implementing dpll subsystem, as well as for >>>interactions of DPLL netlink framework part with the subsystem >>>itself. >>> >>>Co-developed-by: Milena Olech <milena.olech@intel.com> >>>Signed-off-by: Milena Olech <milena.olech@intel.com> >>>Co-developed-by: Michal Michalik <michal.michalik@intel.com> >>>Signed-off-by: Michal Michalik <michal.michalik@intel.com> >>>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>--- >>> drivers/dpll/dpll_core.c | 953 +++++++++++++++++++++++++++++++++++++++ >>> drivers/dpll/dpll_core.h | 104 +++++ >> >>Overall, looks very good! I pinpointed couple of nits below, nothing big. >>General question: Why do you put documentation comment to every static >>function? Does not make any sense to me. Even for non-exported functions >>I think it is overkill. Most of them (if not all) give the reader no >>additional information and only make the code a bit harder to read. >>Care to drop them? >> > >I forgot to respond here.. I would rather leave it, but if the others think >the same way, we could remove them. Could you explain what is the benefit of leaving them? What are they good for. From what I see, they are obvious and only add blank LOC.
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;
On 10.06.2023 18:38, Jiri Pirko wrote: > Fri, Jun 09, 2023 at 02:18:46PM CEST, arkadiusz.kubalewski@intel.com wrote: >> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> >> DPLL framework is used to represent and configure DPLL devices >> in systems. Each device that has DPLL and can configure inputs >> and outputs can use this framework. >> >> Implement core framework functions for further interactions >> with device drivers implementing dpll subsystem, as well as for >> interactions of DPLL netlink framework part with the subsystem >> itself. >> >> Co-developed-by: Milena Olech <milena.olech@intel.com> >> Signed-off-by: Milena Olech <milena.olech@intel.com> >> Co-developed-by: Michal Michalik <michal.michalik@intel.com> >> Signed-off-by: Michal Michalik <michal.michalik@intel.com> >> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> Co-developed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> --- >> drivers/dpll/dpll_core.c | 953 +++++++++++++++++++++++++++++++++++++++ >> drivers/dpll/dpll_core.h | 104 +++++ > > Overall, looks very good! I pinpointed couple of nits below, nothing big. > General question: Why do you put documentation comment to every static > function? Does not make any sense to me. Even for non-exported functions > I think it is overkill. Most of them (if not all) give the reader no > additional information and only make the code a bit harder to read. > Care to drop them? > > >> 2 files changed, 1057 insertions(+) >> create mode 100644 drivers/dpll/dpll_core.c >> create mode 100644 drivers/dpll/dpll_core.h >> >> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c >> new file mode 100644 >> index 000000000000..ee515b7c18be >> --- /dev/null >> +++ b/drivers/dpll/dpll_core.c >> @@ -0,0 +1,953 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * dpll_core.c - Generic DPLL Management class support. > > What's "management class support"? Does not sound recognizable. > > >> + * >> + * Copyright (c) 2023 Meta Platforms, Inc. and affiliates >> + * Copyright (c) 2023 Intel Corporation. >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include <linux/device.h> >> +#include <linux/err.h> >> +#include <linux/slab.h> >> +#include <linux/string.h> >> + >> +#include "dpll_core.h" >> + >> +DEFINE_MUTEX(dpll_lock); >> + >> +DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC); >> +DEFINE_XARRAY_FLAGS(dpll_pin_xa, XA_FLAGS_ALLOC); >> + >> +#define ASSERT_DPLL_REGISTERED(d) \ >> + WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED)) >> +#define ASSERT_DPLL_NOT_REGISTERED(d) \ >> + WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED)) >> + >> +/** >> + * dpll_device_get_by_id - find dpll device by it's id >> + * @id: id of searched dpll >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * dpll_device struct if found >> + * * NULL otherwise >> + */ >> +struct dpll_device *dpll_device_get_by_id(int id) >> +{ >> + if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED)) >> + return xa_load(&dpll_device_xa, id); >> + >> + return NULL; >> +} >> + >> +/** >> + * dpll_pin_registration_find - find a pin registration record >> + * @ref: reference between dpll and pin >> + * @ops: searched pin ops pointer >> + * @priv: searched pin priv pointer >> + * >> + * Search dpll's registered pins for given ops and priv data. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * NULL - if pin was not found >> + * * pointer to `struct dpll_pin_registration` if found >> + */ >> +static struct dpll_pin_registration * >> +dpll_pin_registration_find(struct dpll_pin_ref *ref, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + struct dpll_pin_registration *reg; >> + >> + list_for_each_entry(reg, &ref->registration_list, list) { >> + if (reg->ops == ops && reg->priv == priv) >> + return reg; >> + } >> + return NULL; >> +} >> + >> +/** >> + * dpll_xa_ref_pin_add - add pin reference to a given xarray >> + * @xa_pins: dpll_pin_ref xarray holding pins >> + * @pin: pin being added >> + * @ops: ops for a pin >> + * @priv: pointer to private data of owner >> + * >> + * Allocate and create reference of a pin and enlist a registration >> + * structure storing ops and priv pointers of a caller registant. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * 0 on success >> + * * -ENOMEM on failed allocation >> + */ >> +static int >> +dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + struct dpll_pin_registration *reg; >> + struct dpll_pin_ref *ref; >> + bool ref_exists = false; >> + unsigned long i; >> + int ret; >> + >> + 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); >> + if (!reg) { >> + if (!ref_exists) >> + kfree(ref); >> + return -ENOMEM; >> + } >> + reg->ops = ops; >> + reg->priv = priv; >> + if (ref_exists) > > If you set refcount to 0 couple of lines above, you can drop the check > here. > This is impossible. You cannot increment on zero refcount, it's treated as use-after-free. I'll think about re-arrange the code a bit to avoid such contructions of possible. > >> + refcount_inc(&ref->refcount); >> + list_add_tail(®->list, &ref->registration_list); >> + >> + return 0; >> +} >> + >> +/** >> + * dpll_xa_ref_pin_del - remove reference of a pin from xarray >> + * @xa_pins: dpll_pin_ref xarray holding pins >> + * @pin: pointer to a pin being removed >> + * @ops: pointer to ops of pin being removed >> + * @priv: pointer to private data of registerer who invoked pin removal >> + * >> + * Decrement refcount of existing pin reference on given xarray. >> + * If all registrations are lifted delete the reference and free its memory. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * 0 on success >> + * * -EINVAL if reference to a pin was not found >> + */ >> +static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + struct dpll_pin_registration *reg; >> + struct dpll_pin_ref *ref; >> + unsigned long i; >> + >> + xa_for_each(xa_pins, i, ref) { >> + if (ref->pin != pin) >> + continue; >> + reg = dpll_pin_registration_find(ref, ops, priv); >> + if (WARN_ON(!reg)) >> + return -EINVAL; >> + if (refcount_dec_and_test(&ref->refcount)) { >> + list_del(®->list); >> + kfree(reg); >> + xa_erase(xa_pins, i); >> + WARN_ON(!list_empty(&ref->registration_list)); >> + kfree(ref); >> + } >> + return 0; >> + } >> + >> + return -EINVAL; >> +} >> + >> +/** >> + * dpll_xa_ref_dpll_add - add dpll reference to a given xarray >> + * @xa_dplls: dpll_pin_ref xarray holding dplls >> + * @dpll: dpll being added >> + * @ops: pin-reference ops for a dpll >> + * @priv: pointer to private data of owner >> + * >> + * Allocate and create reference of a dpll-pin ops or increase refcount >> + * on existing dpll reference on given xarray. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * 0 on success >> + * * -ENOMEM on failed allocation >> + */ >> +static int >> +dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + struct dpll_pin_registration *reg; >> + struct dpll_pin_ref *ref; >> + bool ref_exists = false; >> + unsigned long i; >> + int ret; >> + >> + xa_for_each(xa_dplls, i, ref) { >> + if (ref->dpll != dpll) >> + 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->dpll = dpll; >> + INIT_LIST_HEAD(&ref->registration_list); >> + ret = xa_insert(xa_dplls, dpll->device_idx, ref, GFP_KERNEL); >> + if (ret) { >> + kfree(ref); >> + return ret; >> + } >> + refcount_set(&ref->refcount, 1); >> + } >> + >> + reg = kzalloc(sizeof(*reg), GFP_KERNEL); >> + if (!reg) { >> + if (!ref_exists) >> + kfree(ref); >> + return -ENOMEM; >> + } >> + reg->ops = ops; >> + reg->priv = priv; >> + if (ref_exists) > > If you set refcount to 0 couple of lines above, you can drop the check > here. > > >> + refcount_inc(&ref->refcount); >> + list_add_tail(®->list, &ref->registration_list); >> + >> + return 0; >> +} >> + >> +/** >> + * dpll_xa_ref_dpll_del - remove reference of a dpll from xarray >> + * @xa_dplls: dpll_pin_ref xarray holding dplls >> + * @dpll: pointer to a dpll to remove >> + * @ops: pointer to ops of dpll being removed >> + * @priv: pointer to private data of registerer who invoked dpll removal >> + * >> + * Decrement refcount of existing dpll reference on given xarray. >> + * If all references are dropped, delete the reference and free its memory. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + */ >> +static void >> +dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + struct dpll_pin_registration *reg; >> + struct dpll_pin_ref *ref; >> + unsigned long i; >> + >> + xa_for_each(xa_dplls, i, ref) { >> + if (ref->dpll != dpll) >> + continue; >> + reg = dpll_pin_registration_find(ref, ops, priv); >> + if (WARN_ON(!reg)) >> + return; >> + if (refcount_dec_and_test(&ref->refcount)) { >> + list_del(®->list); >> + kfree(reg); >> + xa_erase(xa_dplls, i); >> + WARN_ON(!list_empty(&ref->registration_list)); >> + kfree(ref); >> + } >> + return; >> + } >> +} >> + >> +/** >> + * dpll_xa_ref_dpll_find - find dpll reference on xarray >> + * @xa_refs: dpll_pin_ref xarray holding dpll references >> + * @dpll: pointer to a dpll being searched >> + * >> + * Search for dpll-pin ops reference struct of a given dpll on given xarray. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * pin reference struct pointer on success >> + * * NULL - reference to a pin was not found >> + */ >> +struct dpll_pin_ref * >> +dpll_xa_ref_dpll_find(struct xarray *xa_refs, const struct dpll_device *dpll) >> +{ >> + struct dpll_pin_ref *ref; >> + unsigned long i; >> + >> + xa_for_each(xa_refs, i, ref) { >> + if (ref->dpll == dpll) >> + return ref; >> + } >> + >> + return NULL; >> +} >> + >> +/** >> + * dpll_xa_ref_dpll_first - find first record of given xarray >> + * @xa_refs: xarray >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: first element on given xaaray >> + */ >> +struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs) >> +{ >> + struct dpll_pin_ref *ref; >> + unsigned long i = 0; >> + >> + ref = xa_find(xa_refs, &i, ULONG_MAX, XA_PRESENT); >> + WARN_ON(!ref); >> + return ref; >> +} >> + >> +/** >> + * dpll_device_alloc - allocate the memory for dpll device >> + * @clock_id: clock_id of creator >> + * @device_idx: id given by dev driver >> + * @module: reference to registering module >> + * >> + * Allocates memory and initialize dpll device, hold its reference on global >> + * xarray. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * valid dpll_device struct pointer if succeeded >> + * * ERR_PTR(-ENOMEM) - failed memory allocation >> + * * ERR_PTR(X) - failed allocation on dpll's xa >> + */ >> +static struct dpll_device * >> +dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module) >> +{ >> + struct dpll_device *dpll; >> + int ret; >> + >> + dpll = kzalloc(sizeof(*dpll), GFP_KERNEL); >> + if (!dpll) >> + return ERR_PTR(-ENOMEM); >> + refcount_set(&dpll->refcount, 1); >> + INIT_LIST_HEAD(&dpll->registration_list); >> + dpll->device_idx = device_idx; >> + dpll->clock_id = clock_id; >> + dpll->module = module; >> + ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, >> + GFP_KERNEL); >> + if (ret) { >> + kfree(dpll); >> + return ERR_PTR(ret); >> + } >> + xa_init_flags(&dpll->pin_refs, XA_FLAGS_ALLOC); >> + >> + return dpll; >> +} >> + >> +/** >> + * dpll_device_get - find existing or create new dpll device >> + * @clock_id: clock_id of creator >> + * @device_idx: idx given by device driver >> + * @module: reference to registering module >> + * >> + * Get existing object of a dpll device, unique for given arguments. >> + * Create new if doesn't exist yet. >> + * >> + * Context: Acquires a lock (dpll_lock) >> + * Return: >> + * * valid dpll_device struct pointer if succeeded >> + * * ERR_PTR(-ENOMEM) - failed memory allocation >> + * * ERR_PTR(X) - failed allocation on dpll's xa >> + */ >> +struct dpll_device * >> +dpll_device_get(u64 clock_id, u32 device_idx, struct module *module) >> +{ >> + struct dpll_device *dpll, *ret = NULL; >> + unsigned long index; >> + >> + mutex_lock(&dpll_lock); >> + xa_for_each(&dpll_device_xa, index, dpll) { >> + if (dpll->clock_id == clock_id && >> + dpll->device_idx == device_idx && >> + dpll->module == module) { >> + ret = dpll; >> + refcount_inc(&ret->refcount); >> + break; >> + } >> + } >> + if (!ret) >> + ret = dpll_device_alloc(clock_id, device_idx, module); >> + mutex_unlock(&dpll_lock); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(dpll_device_get); >> + >> +/** >> + * dpll_device_put - decrease the refcount and free memory if possible >> + * @dpll: dpll_device struct pointer >> + * >> + * Context: Acquires a lock (dpll_lock) >> + * Drop reference for a dpll device, if all references are gone, delete >> + * dpll device object. >> + */ >> +void dpll_device_put(struct dpll_device *dpll) >> +{ >> + if (!dpll) > > Drop this check and rely on a basic driver sanity. I'm pretty sure I > asked you to drop checks like this during the last version review. > > >> + return; >> + mutex_lock(&dpll_lock); >> + if (refcount_dec_and_test(&dpll->refcount)) { >> + ASSERT_DPLL_NOT_REGISTERED(dpll); >> + WARN_ON_ONCE(!xa_empty(&dpll->pin_refs)); >> + xa_destroy(&dpll->pin_refs); >> + xa_erase(&dpll_device_xa, dpll->id); >> + WARN_ON(!list_empty(&dpll->registration_list)); >> + kfree(dpll); >> + } >> + mutex_unlock(&dpll_lock); >> +} >> +EXPORT_SYMBOL_GPL(dpll_device_put); >> + >> +static struct dpll_device_registration * >> +dpll_device_registration_find(struct dpll_device *dpll, >> + const struct dpll_device_ops *ops, void *priv) >> +{ >> + struct dpll_device_registration *reg; >> + >> + list_for_each_entry(reg, &dpll->registration_list, list) { >> + if (reg->ops == ops && reg->priv == priv) >> + return reg; >> + } >> + return NULL; >> +} >> + >> +/** >> + * dpll_device_register - register the dpll device in the subsystem >> + * @dpll: pointer to a dpll >> + * @type: type of a dpll >> + * @ops: ops for a dpll device >> + * @priv: pointer to private information of owner >> + * >> + * Make dpll device available for user space. >> + * >> + * Context: Acquires a lock (dpll_lock) >> + * Return: >> + * * 0 on success >> + * * -EINVAL on failure due to wrong arguments provided >> + * * -EEXIST if device was already registered >> + */ >> +int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, >> + const struct dpll_device_ops *ops, void *priv) >> +{ >> + struct dpll_device_registration *reg; >> + bool first_registration = false; >> + >> + if (WARN_ON(!ops)) >> + return -EINVAL; >> + if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) >> + return -EINVAL; >> + >> + mutex_lock(&dpll_lock); >> + reg = dpll_device_registration_find(dpll, ops, priv); >> + if (reg) { >> + mutex_unlock(&dpll_lock); >> + return -EEXIST; >> + } >> + >> + reg = kzalloc(sizeof(*reg), GFP_KERNEL); >> + if (!reg) { >> + mutex_unlock(&dpll_lock); >> + return -EEXIST; > > -ENOMEM; > > >> + } >> + reg->ops = ops; >> + reg->priv = priv; >> + dpll->type = type; >> + first_registration = list_empty(&dpll->registration_list); >> + list_add_tail(®->list, &dpll->registration_list); >> + if (!first_registration) { >> + mutex_unlock(&dpll_lock); >> + return 0; >> + } >> + >> + xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); >> + mutex_unlock(&dpll_lock); >> + dpll_device_create_ntf(dpll); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(dpll_device_register); >> + >> +/** >> + * dpll_device_unregister - unregister dpll device >> + * @dpll: registered dpll pointer >> + * @ops: ops for a dpll device >> + * @priv: pointer to private information of owner >> + * >> + * Unregister device, make it unavailable for userspace. >> + * Note: It does not free the memory >> + * Context: Acquires a lock (dpll_lock) >> + */ >> +void dpll_device_unregister(struct dpll_device *dpll, >> + const struct dpll_device_ops *ops, void *priv) >> +{ >> + struct dpll_device_registration *reg; >> + >> + mutex_lock(&dpll_lock); >> + ASSERT_DPLL_REGISTERED(dpll); >> + dpll_device_delete_ntf(dpll); >> + reg = dpll_device_registration_find(dpll, ops, priv); >> + if (WARN_ON(!reg)) { >> + mutex_unlock(&dpll_lock); >> + return; >> + } >> + list_del(®->list); >> + kfree(reg); >> + >> + if (!list_empty(&dpll->registration_list)) { >> + mutex_unlock(&dpll_lock); >> + return; >> + } >> + xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); >> + mutex_unlock(&dpll_lock); >> +} >> +EXPORT_SYMBOL_GPL(dpll_device_unregister); >> + >> +/** >> + * dpll_pin_alloc - allocate the memory for dpll pin >> + * @clock_id: clock_id of creator >> + * @pin_idx: idx given by dev driver >> + * @module: reference to registering module >> + * @prop: dpll pin properties >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * valid allocated dpll_pin struct pointer if succeeded >> + * * ERR_PTR(-ENOMEM) - failed memory allocation >> + * * ERR_PTR(-EINVAL) - wrong pin type property value >> + */ >> +static struct dpll_pin * >> +dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module, >> + const struct dpll_pin_properties *prop) >> +{ >> + struct dpll_pin *pin; >> + int ret; >> + >> + pin = kzalloc(sizeof(*pin), GFP_KERNEL); >> + if (!pin) >> + return ERR_PTR(-ENOMEM); >> + pin->pin_idx = pin_idx; >> + pin->clock_id = clock_id; >> + pin->module = module; >> + if (WARN_ON(prop->type < DPLL_PIN_TYPE_MUX || >> + prop->type > DPLL_PIN_TYPE_MAX)) { >> + ret = -EINVAL; >> + goto err; >> + } >> + pin->prop = prop; >> + refcount_set(&pin->refcount, 1); >> + xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC); >> + xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC); >> + ret = xa_alloc(&dpll_pin_xa, &pin->id, pin, xa_limit_16b, GFP_KERNEL); >> + if (ret) >> + goto err; >> + return pin; >> +err: >> + xa_destroy(&pin->dpll_refs); >> + xa_destroy(&pin->parent_refs); >> + kfree(pin); >> + return ERR_PTR(ret); >> +} >> + >> +/** >> + * dpll_pin_get - find existing or create new dpll pin >> + * @clock_id: clock_id of creator >> + * @pin_idx: idx given by dev driver >> + * @module: reference to registering module >> + * @prop: dpll pin properties >> + * >> + * Get existing object of a pin (unique for given arguments) or create new >> + * if doesn't exist yet. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: >> + * * valid allocated dpll_pin struct pointer if succeeded >> + * * ERR_PTR of an error >> + */ >> +struct dpll_pin * >> +dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, >> + const struct dpll_pin_properties *prop) >> +{ >> + struct dpll_pin *pos, *ret = NULL; >> + unsigned long i; >> + >> + xa_for_each(&dpll_pin_xa, i, pos) { >> + if (pos->clock_id == clock_id && >> + pos->pin_idx == pin_idx && >> + pos->module == module) { >> + ret = pos; >> + refcount_inc(&ret->refcount); >> + break; >> + } >> + } >> + if (!ret) >> + ret = dpll_pin_alloc(clock_id, pin_idx, module, prop); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(dpll_pin_get); >> + >> +/** >> + * dpll_pin_put - decrease the refcount and free memory if possible >> + * @pin: pointer to a pin to be put >> + * >> + * Drop reference for a pin, if all references are gone, delete pin object. >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + */ >> +void dpll_pin_put(struct dpll_pin *pin) >> +{ >> + if (!pin) > > Drop this check. > > >> + return; >> + if (refcount_dec_and_test(&pin->refcount)) { >> + xa_destroy(&pin->dpll_refs); >> + xa_destroy(&pin->parent_refs); >> + xa_erase(&dpll_pin_xa, pin->id); >> + kfree(pin); >> + } >> +} >> +EXPORT_SYMBOL_GPL(dpll_pin_put); >> + >> +static int >> +__dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + int ret; >> + >> + if (WARN_ON(!ops)) >> + return -EINVAL; >> + >> + ret = dpll_xa_ref_pin_add(&dpll->pin_refs, pin, ops, priv); >> + if (ret) >> + return ret; >> + ret = dpll_xa_ref_dpll_add(&pin->dpll_refs, dpll, ops, priv); >> + if (ret) >> + goto ref_pin_del; >> + else >> + dpll_pin_create_ntf(pin); >> + >> + return ret; >> + >> +ref_pin_del: >> + dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv); >> + return ret; >> +} >> + >> +/** >> + * dpll_pin_register - register the dpll pin in the subsystem >> + * @dpll: pointer to a dpll >> + * @pin: pointer to a dpll pin >> + * @ops: ops for a dpll pin ops >> + * @priv: pointer to private information of owner >> + * >> + * Context: Acquires a lock (dpll_lock) >> + * Return: >> + * * 0 on success >> + * * -EINVAL - missing pin ops >> + * * -ENOMEM - failed to allocate memory >> + */ >> +int >> +dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + int ret; >> + >> + mutex_lock(&dpll_lock); >> + if (WARN_ON(!(dpll->module == pin->module && >> + dpll->clock_id == pin->clock_id))) >> + ret = -EFAULT; > > In other cases like this you return -EINVAL. I think it is better. One > way or another, be consistent please. > > >> + else >> + ret = __dpll_pin_register(dpll, pin, ops, priv); >> + mutex_unlock(&dpll_lock); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(dpll_pin_register); >> + >> +static void >> +__dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv); >> + dpll_xa_ref_dpll_del(&pin->dpll_refs, dpll, ops, priv); >> +} >> + >> +/** >> + * dpll_pin_unregister - unregister dpll pin from dpll device >> + * @dpll: registered dpll pointer >> + * @pin: pointer to a pin >> + * @ops: ops for a dpll pin >> + * @priv: pointer to private information of owner >> + * >> + * Note: It does not free the memory >> + * Context: Acquires a lock (dpll_lock) >> + */ >> +void dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + if (WARN_ON(xa_empty(&dpll->pin_refs))) >> + return; >> + >> + mutex_lock(&dpll_lock); >> + __dpll_pin_unregister(dpll, pin, ops, priv); >> + mutex_unlock(&dpll_lock); >> +} >> +EXPORT_SYMBOL_GPL(dpll_pin_unregister); >> + >> +/** >> + * dpll_pin_on_pin_register - register a pin with a parent pin >> + * @parent: pointer to a parent pin >> + * @pin: pointer to a pin >> + * @ops: ops for a dpll pin >> + * @priv: pointer to private information of owner >> + * >> + * Register a pin with a parent pin, create references between them and >> + * between newly registered pin and dplls connected with a parent pin. >> + * >> + * Context: Acquires a lock (dpll_lock) >> + * Return: >> + * * 0 on success >> + * * -EINVAL missing pin or parent >> + * * -ENOMEM failed allocation >> + * * -EPERM if parent is not allowed >> + */ >> +int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + struct dpll_pin_ref *ref; >> + unsigned long i, stop; >> + int ret; >> + >> + if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX)) >> + return -EINVAL; >> + ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv); >> + if (ret) >> + goto unlock; >> + refcount_inc(&pin->refcount); >> + xa_for_each(&parent->dpll_refs, i, ref) { >> + mutex_lock(&dpll_lock); >> + ret = __dpll_pin_register(ref->dpll, pin, ops, priv); >> + mutex_unlock(&dpll_lock); >> + if (ret) { >> + stop = i; >> + goto dpll_unregister; >> + } >> + dpll_pin_create_ntf(pin); >> + } >> + >> + return ret; >> + >> +dpll_unregister: >> + xa_for_each(&parent->dpll_refs, i, ref) { >> + if (i < stop) { >> + mutex_lock(&dpll_lock); >> + __dpll_pin_unregister(ref->dpll, pin, ops, priv); >> + mutex_unlock(&dpll_lock); >> + } >> + } >> + refcount_dec(&pin->refcount); >> + dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv); >> +unlock: >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(dpll_pin_on_pin_register); >> + >> +/** >> + * dpll_pin_on_pin_unregister - unregister dpll pin from a parent pin >> + * @parent: pointer to a parent pin >> + * @pin: pointer to a pin >> + * @ops: ops for a dpll pin >> + * @priv: pointer to private information of owner >> + * >> + * Context: Acquires a lock (dpll_lock) >> + * Note: It does not free the memory >> + */ >> +void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin *pin, >> + const struct dpll_pin_ops *ops, void *priv) >> +{ >> + struct dpll_pin_ref *ref; >> + unsigned long i; >> + >> + mutex_lock(&dpll_lock); >> + dpll_pin_delete_ntf(pin); >> + dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv); >> + refcount_dec(&pin->refcount); >> + xa_for_each(&pin->dpll_refs, i, ref) { >> + __dpll_pin_unregister(ref->dpll, pin, ops, priv); >> + } > > Drop "{}"s for one line statement. Didn't checkpack warn you? > > >> + mutex_unlock(&dpll_lock); >> +} >> +EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister); >> + >> + >> +/** >> + * dpll_device_registration_first - get first registration of dpll device >> + * @dpll: pointer to a dpll >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: pointer to the first registration of a dpll >> + */ >> +static struct dpll_device_registration * >> +dpll_device_registration_first(struct dpll_device *dpll) >> +{ >> + struct dpll_device_registration *reg; >> + >> + reg = list_first_entry_or_null((struct list_head *) &dpll->registration_list, >> + struct dpll_device_registration, list); >> + WARN_ON(!reg); >> + return reg; >> +} >> + >> +/** >> + * dpll_priv - get the dpll device private owner data >> + * @dpll: registered dpll pointer >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: pointer to the first registration priv data >> + */ >> +void *dpll_priv(struct dpll_device *dpll) >> +{ >> + struct dpll_device_registration *reg; >> + >> + reg = dpll_device_registration_first(dpll); >> + return reg->priv; >> +} >> + >> +/** >> + * dpll_device_ops - get the dpll device ops pointer >> + * @dpll: registered dpll pointer >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: pointer to the first registration ops of the dpll >> + */ >> +const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll) >> +{ >> + struct dpll_device_registration *reg; >> + >> + reg = dpll_device_registration_first(dpll); >> + return reg->ops; >> +} >> + >> +/** >> + * dpll_pin_registration_first - get first registration of dpll pin ref >> + * @ref: pointer to a pin ref struct >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: pointer to the first registration of a dpll_pin_ref >> + */ >> +static struct dpll_pin_registration * >> +dpll_pin_registration_first(struct dpll_pin_ref *ref) >> +{ >> + struct dpll_pin_registration *reg; >> + >> + reg = list_first_entry_or_null(&ref->registration_list, >> + struct dpll_pin_registration, list); >> + WARN_ON(!reg); >> + return reg; >> +} >> + >> +/** >> + * dpll_pin_on_dpll_priv - get the dpll device private owner data >> + * @dpll: registered dpll pointer >> + * @pin: pointer to a pin >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: pointer to the data >> + */ >> +void *dpll_pin_on_dpll_priv(struct dpll_device *dpll, >> + struct dpll_pin *pin) >> +{ >> + struct dpll_pin_registration *reg; >> + struct dpll_pin_ref *ref; >> + >> + ref = xa_load(&dpll->pin_refs, pin->pin_idx); >> + if (!ref) >> + return NULL; >> + reg = dpll_pin_registration_first(ref); >> + return reg->priv; >> +} >> + >> +/** >> + * dpll_pin_on_pin_priv - get the dpll pin private owner data >> + * @parent: pointer to a parent pin >> + * @pin: pointer to a pin >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: pointer to the data >> + */ >> +void *dpll_pin_on_pin_priv(struct dpll_pin *parent, >> + struct dpll_pin *pin) >> +{ >> + struct dpll_pin_registration *reg; >> + struct dpll_pin_ref *ref; >> + >> + ref = xa_load(&pin->parent_refs, parent->pin_idx); >> + if (!ref) >> + return NULL; >> + reg = dpll_pin_registration_first(ref); >> + return reg->priv; >> +} >> + >> +/** >> + * dpll_pin_ops - get the pin ops pointer >> + * @ref: dpll pin ref >> + * >> + * Context: shall be called under a lock (dpll_lock) >> + * Return: pointer to the first ops registered with the pin >> + */ >> +const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref) >> +{ >> + struct dpll_pin_registration *reg; >> + >> + reg = dpll_pin_registration_first(ref); >> + return reg->ops; >> +} >> + >> +/** >> + * dpll_init - initialize dpll subsystem >> + * >> + * Return: >> + * 0 - success >> + * negative - netlink init error >> + */ >> +static int __init dpll_init(void) >> +{ >> + int ret; >> + >> + ret = dpll_netlink_init(); >> + if (ret) >> + goto error; >> + >> + return 0; >> + >> +error: >> + mutex_destroy(&dpll_lock); >> + return ret; >> +} >> +subsys_initcall(dpll_init); >> diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h >> new file mode 100644 >> index 000000000000..ef95e272937c >> --- /dev/null >> +++ b/drivers/dpll/dpll_core.h >> @@ -0,0 +1,104 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright (c) 2023 Meta Platforms, Inc. and affiliates >> + * Copyright (c) 2023 Intel and affiliates >> + */ >> + >> +#ifndef __DPLL_CORE_H__ >> +#define __DPLL_CORE_H__ >> + >> +#include <linux/dpll.h> >> +#include <linux/list.h> >> +#include <linux/refcount.h> >> +#include "dpll_netlink.h" >> + >> +#define DPLL_REGISTERED XA_MARK_1 >> + >> +struct dpll_device_registration { >> + struct list_head list; >> + const struct dpll_device_ops *ops; >> + void *priv; > > You don't need this struct in the header. Move to .c > > >> +}; >> + >> +/** >> + * struct dpll_device - stores DPLL device internal data >> + * @id: unique id number for each device given by kernel > > Who's kernel? Say "dpll subsystem" instead. > > >> + * @device_idx: id given by dev driver >> + * @clock_id: unique identifier (clock_id) of a dpll >> + * @module: module of creator >> + * @type: type of a dpll >> + * @pin_refs: stores pins registered within a dpll >> + * @mode_supported_mask: mask of supported modes >> + * @refcount: refcount >> + * @registration_list: list of registered ops and priv data of dpll owners >> + **/ >> +struct dpll_device { >> + u32 id; >> + u32 device_idx; >> + u64 clock_id; >> + struct module *module; >> + enum dpll_type type; >> + struct xarray pin_refs; >> + unsigned long mode_supported_mask; >> + refcount_t refcount; >> + struct list_head registration_list; >> +}; >> + >> +/** >> + * struct dpll_pin - structure for a dpll pin >> + * @id: unique id number for pin given by kernel > > Who's kernel? Say "dpll subsystem" instead. > > >> + * @pin_idx: index of a pin given by dev driver >> + * @clock_id: clock_id of creator >> + * @module: module of creator >> + * @dpll_refs: hold referencees to dplls pin was registered with >> + * @parent_refs: hold references to parent pins pin was registered with >> + * @prop: pointer to pin properties given by registerer >> + * @rclk_dev_name: holds name of device when pin can recover clock from it >> + * @refcount: 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; >> +}; >> + >> +struct dpll_pin_registration { >> + struct list_head list; >> + const struct dpll_pin_ops *ops; >> + void *priv; >> +}; > > You don't need this struct in the header. Move to .c > > >> + >> +/** >> + * struct dpll_pin_ref - structure for referencing either dpll or pins >> + * @dpll: pointer to a dpll >> + * @pin: pointer to a pin >> + * @registration_list: list of ops and priv data registered with the ref >> + * @refcount: refcount >> + **/ >> +struct dpll_pin_ref { >> + union { >> + struct dpll_device *dpll; >> + struct dpll_pin *pin; >> + }; >> + struct list_head registration_list; >> + refcount_t refcount; >> +}; >> + >> +void *dpll_priv(struct dpll_device *dpll); >> +void *dpll_pin_on_dpll_priv(struct dpll_device *dpll, struct dpll_pin *pin); >> +void *dpll_pin_on_pin_priv(struct dpll_pin *parent, struct dpll_pin *pin); >> + >> +const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll); >> +struct dpll_device *dpll_device_get_by_id(int id); >> +const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref); >> +struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs); >> +extern struct xarray dpll_device_xa; >> +extern struct xarray dpll_pin_xa; >> +extern struct mutex dpll_lock; >> +#endif >> -- >> 2.37.3 >>
diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c new file mode 100644 index 000000000000..ee515b7c18be --- /dev/null +++ b/drivers/dpll/dpll_core.c @@ -0,0 +1,953 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * dpll_core.c - Generic DPLL Management class support. + * + * Copyright (c) 2023 Meta Platforms, Inc. and affiliates + * Copyright (c) 2023 Intel Corporation. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/string.h> + +#include "dpll_core.h" + +DEFINE_MUTEX(dpll_lock); + +DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC); +DEFINE_XARRAY_FLAGS(dpll_pin_xa, XA_FLAGS_ALLOC); + +#define ASSERT_DPLL_REGISTERED(d) \ + WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED)) +#define ASSERT_DPLL_NOT_REGISTERED(d) \ + WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED)) + +/** + * dpll_device_get_by_id - find dpll device by it's id + * @id: id of searched dpll + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * dpll_device struct if found + * * NULL otherwise + */ +struct dpll_device *dpll_device_get_by_id(int id) +{ + if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED)) + return xa_load(&dpll_device_xa, id); + + return NULL; +} + +/** + * dpll_pin_registration_find - find a pin registration record + * @ref: reference between dpll and pin + * @ops: searched pin ops pointer + * @priv: searched pin priv pointer + * + * Search dpll's registered pins for given ops and priv data. + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * NULL - if pin was not found + * * pointer to `struct dpll_pin_registration` if found + */ +static struct dpll_pin_registration * +dpll_pin_registration_find(struct dpll_pin_ref *ref, + const struct dpll_pin_ops *ops, void *priv) +{ + struct dpll_pin_registration *reg; + + list_for_each_entry(reg, &ref->registration_list, list) { + if (reg->ops == ops && reg->priv == priv) + return reg; + } + return NULL; +} + +/** + * dpll_xa_ref_pin_add - add pin reference to a given xarray + * @xa_pins: dpll_pin_ref xarray holding pins + * @pin: pin being added + * @ops: ops for a pin + * @priv: pointer to private data of owner + * + * Allocate and create reference of a pin and enlist a registration + * structure storing ops and priv pointers of a caller registant. + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * 0 on success + * * -ENOMEM on failed allocation + */ +static int +dpll_xa_ref_pin_add(struct xarray *xa_pins, struct dpll_pin *pin, + const struct dpll_pin_ops *ops, void *priv) +{ + struct dpll_pin_registration *reg; + struct dpll_pin_ref *ref; + bool ref_exists = false; + unsigned long i; + int ret; + + 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); + if (!reg) { + if (!ref_exists) + kfree(ref); + return -ENOMEM; + } + reg->ops = ops; + reg->priv = priv; + if (ref_exists) + refcount_inc(&ref->refcount); + list_add_tail(®->list, &ref->registration_list); + + return 0; +} + +/** + * dpll_xa_ref_pin_del - remove reference of a pin from xarray + * @xa_pins: dpll_pin_ref xarray holding pins + * @pin: pointer to a pin being removed + * @ops: pointer to ops of pin being removed + * @priv: pointer to private data of registerer who invoked pin removal + * + * Decrement refcount of existing pin reference on given xarray. + * If all registrations are lifted delete the reference and free its memory. + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * 0 on success + * * -EINVAL if reference to a pin was not found + */ +static int dpll_xa_ref_pin_del(struct xarray *xa_pins, struct dpll_pin *pin, + const struct dpll_pin_ops *ops, void *priv) +{ + struct dpll_pin_registration *reg; + struct dpll_pin_ref *ref; + unsigned long i; + + xa_for_each(xa_pins, i, ref) { + if (ref->pin != pin) + continue; + reg = dpll_pin_registration_find(ref, ops, priv); + if (WARN_ON(!reg)) + return -EINVAL; + if (refcount_dec_and_test(&ref->refcount)) { + list_del(®->list); + kfree(reg); + xa_erase(xa_pins, i); + WARN_ON(!list_empty(&ref->registration_list)); + kfree(ref); + } + return 0; + } + + return -EINVAL; +} + +/** + * dpll_xa_ref_dpll_add - add dpll reference to a given xarray + * @xa_dplls: dpll_pin_ref xarray holding dplls + * @dpll: dpll being added + * @ops: pin-reference ops for a dpll + * @priv: pointer to private data of owner + * + * Allocate and create reference of a dpll-pin ops or increase refcount + * on existing dpll reference on given xarray. + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * 0 on success + * * -ENOMEM on failed allocation + */ +static int +dpll_xa_ref_dpll_add(struct xarray *xa_dplls, struct dpll_device *dpll, + const struct dpll_pin_ops *ops, void *priv) +{ + struct dpll_pin_registration *reg; + struct dpll_pin_ref *ref; + bool ref_exists = false; + unsigned long i; + int ret; + + xa_for_each(xa_dplls, i, ref) { + if (ref->dpll != dpll) + 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->dpll = dpll; + INIT_LIST_HEAD(&ref->registration_list); + ret = xa_insert(xa_dplls, dpll->device_idx, ref, GFP_KERNEL); + if (ret) { + kfree(ref); + return ret; + } + refcount_set(&ref->refcount, 1); + } + + reg = kzalloc(sizeof(*reg), GFP_KERNEL); + if (!reg) { + if (!ref_exists) + kfree(ref); + return -ENOMEM; + } + reg->ops = ops; + reg->priv = priv; + if (ref_exists) + refcount_inc(&ref->refcount); + list_add_tail(®->list, &ref->registration_list); + + return 0; +} + +/** + * dpll_xa_ref_dpll_del - remove reference of a dpll from xarray + * @xa_dplls: dpll_pin_ref xarray holding dplls + * @dpll: pointer to a dpll to remove + * @ops: pointer to ops of dpll being removed + * @priv: pointer to private data of registerer who invoked dpll removal + * + * Decrement refcount of existing dpll reference on given xarray. + * If all references are dropped, delete the reference and free its memory. + * + * Context: shall be called under a lock (dpll_lock) + */ +static void +dpll_xa_ref_dpll_del(struct xarray *xa_dplls, struct dpll_device *dpll, + const struct dpll_pin_ops *ops, void *priv) +{ + struct dpll_pin_registration *reg; + struct dpll_pin_ref *ref; + unsigned long i; + + xa_for_each(xa_dplls, i, ref) { + if (ref->dpll != dpll) + continue; + reg = dpll_pin_registration_find(ref, ops, priv); + if (WARN_ON(!reg)) + return; + if (refcount_dec_and_test(&ref->refcount)) { + list_del(®->list); + kfree(reg); + xa_erase(xa_dplls, i); + WARN_ON(!list_empty(&ref->registration_list)); + kfree(ref); + } + return; + } +} + +/** + * dpll_xa_ref_dpll_find - find dpll reference on xarray + * @xa_refs: dpll_pin_ref xarray holding dpll references + * @dpll: pointer to a dpll being searched + * + * Search for dpll-pin ops reference struct of a given dpll on given xarray. + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * pin reference struct pointer on success + * * NULL - reference to a pin was not found + */ +struct dpll_pin_ref * +dpll_xa_ref_dpll_find(struct xarray *xa_refs, const struct dpll_device *dpll) +{ + struct dpll_pin_ref *ref; + unsigned long i; + + xa_for_each(xa_refs, i, ref) { + if (ref->dpll == dpll) + return ref; + } + + return NULL; +} + +/** + * dpll_xa_ref_dpll_first - find first record of given xarray + * @xa_refs: xarray + * + * Context: shall be called under a lock (dpll_lock) + * Return: first element on given xaaray + */ +struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs) +{ + struct dpll_pin_ref *ref; + unsigned long i = 0; + + ref = xa_find(xa_refs, &i, ULONG_MAX, XA_PRESENT); + WARN_ON(!ref); + return ref; +} + +/** + * dpll_device_alloc - allocate the memory for dpll device + * @clock_id: clock_id of creator + * @device_idx: id given by dev driver + * @module: reference to registering module + * + * Allocates memory and initialize dpll device, hold its reference on global + * xarray. + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * valid dpll_device struct pointer if succeeded + * * ERR_PTR(-ENOMEM) - failed memory allocation + * * ERR_PTR(X) - failed allocation on dpll's xa + */ +static struct dpll_device * +dpll_device_alloc(const u64 clock_id, u32 device_idx, struct module *module) +{ + struct dpll_device *dpll; + int ret; + + dpll = kzalloc(sizeof(*dpll), GFP_KERNEL); + if (!dpll) + return ERR_PTR(-ENOMEM); + refcount_set(&dpll->refcount, 1); + INIT_LIST_HEAD(&dpll->registration_list); + dpll->device_idx = device_idx; + dpll->clock_id = clock_id; + dpll->module = module; + ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, + GFP_KERNEL); + if (ret) { + kfree(dpll); + return ERR_PTR(ret); + } + xa_init_flags(&dpll->pin_refs, XA_FLAGS_ALLOC); + + return dpll; +} + +/** + * dpll_device_get - find existing or create new dpll device + * @clock_id: clock_id of creator + * @device_idx: idx given by device driver + * @module: reference to registering module + * + * Get existing object of a dpll device, unique for given arguments. + * Create new if doesn't exist yet. + * + * Context: Acquires a lock (dpll_lock) + * Return: + * * valid dpll_device struct pointer if succeeded + * * ERR_PTR(-ENOMEM) - failed memory allocation + * * ERR_PTR(X) - failed allocation on dpll's xa + */ +struct dpll_device * +dpll_device_get(u64 clock_id, u32 device_idx, struct module *module) +{ + struct dpll_device *dpll, *ret = NULL; + unsigned long index; + + mutex_lock(&dpll_lock); + xa_for_each(&dpll_device_xa, index, dpll) { + if (dpll->clock_id == clock_id && + dpll->device_idx == device_idx && + dpll->module == module) { + ret = dpll; + refcount_inc(&ret->refcount); + break; + } + } + if (!ret) + ret = dpll_device_alloc(clock_id, device_idx, module); + mutex_unlock(&dpll_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(dpll_device_get); + +/** + * dpll_device_put - decrease the refcount and free memory if possible + * @dpll: dpll_device struct pointer + * + * Context: Acquires a lock (dpll_lock) + * Drop reference for a dpll device, if all references are gone, delete + * dpll device object. + */ +void dpll_device_put(struct dpll_device *dpll) +{ + if (!dpll) + return; + mutex_lock(&dpll_lock); + if (refcount_dec_and_test(&dpll->refcount)) { + ASSERT_DPLL_NOT_REGISTERED(dpll); + WARN_ON_ONCE(!xa_empty(&dpll->pin_refs)); + xa_destroy(&dpll->pin_refs); + xa_erase(&dpll_device_xa, dpll->id); + WARN_ON(!list_empty(&dpll->registration_list)); + kfree(dpll); + } + mutex_unlock(&dpll_lock); +} +EXPORT_SYMBOL_GPL(dpll_device_put); + +static struct dpll_device_registration * +dpll_device_registration_find(struct dpll_device *dpll, + const struct dpll_device_ops *ops, void *priv) +{ + struct dpll_device_registration *reg; + + list_for_each_entry(reg, &dpll->registration_list, list) { + if (reg->ops == ops && reg->priv == priv) + return reg; + } + return NULL; +} + +/** + * dpll_device_register - register the dpll device in the subsystem + * @dpll: pointer to a dpll + * @type: type of a dpll + * @ops: ops for a dpll device + * @priv: pointer to private information of owner + * + * Make dpll device available for user space. + * + * Context: Acquires a lock (dpll_lock) + * Return: + * * 0 on success + * * -EINVAL on failure due to wrong arguments provided + * * -EEXIST if device was already registered + */ +int dpll_device_register(struct dpll_device *dpll, enum dpll_type type, + const struct dpll_device_ops *ops, void *priv) +{ + struct dpll_device_registration *reg; + bool first_registration = false; + + if (WARN_ON(!ops)) + return -EINVAL; + if (WARN_ON(type < DPLL_TYPE_PPS || type > DPLL_TYPE_MAX)) + return -EINVAL; + + mutex_lock(&dpll_lock); + reg = dpll_device_registration_find(dpll, ops, priv); + if (reg) { + mutex_unlock(&dpll_lock); + return -EEXIST; + } + + reg = kzalloc(sizeof(*reg), GFP_KERNEL); + if (!reg) { + mutex_unlock(&dpll_lock); + return -EEXIST; + } + reg->ops = ops; + reg->priv = priv; + dpll->type = type; + first_registration = list_empty(&dpll->registration_list); + list_add_tail(®->list, &dpll->registration_list); + if (!first_registration) { + mutex_unlock(&dpll_lock); + return 0; + } + + xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); + mutex_unlock(&dpll_lock); + dpll_device_create_ntf(dpll); + + return 0; +} +EXPORT_SYMBOL_GPL(dpll_device_register); + +/** + * dpll_device_unregister - unregister dpll device + * @dpll: registered dpll pointer + * @ops: ops for a dpll device + * @priv: pointer to private information of owner + * + * Unregister device, make it unavailable for userspace. + * Note: It does not free the memory + * Context: Acquires a lock (dpll_lock) + */ +void dpll_device_unregister(struct dpll_device *dpll, + const struct dpll_device_ops *ops, void *priv) +{ + struct dpll_device_registration *reg; + + mutex_lock(&dpll_lock); + ASSERT_DPLL_REGISTERED(dpll); + dpll_device_delete_ntf(dpll); + reg = dpll_device_registration_find(dpll, ops, priv); + if (WARN_ON(!reg)) { + mutex_unlock(&dpll_lock); + return; + } + list_del(®->list); + kfree(reg); + + if (!list_empty(&dpll->registration_list)) { + mutex_unlock(&dpll_lock); + return; + } + xa_clear_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED); + mutex_unlock(&dpll_lock); +} +EXPORT_SYMBOL_GPL(dpll_device_unregister); + +/** + * dpll_pin_alloc - allocate the memory for dpll pin + * @clock_id: clock_id of creator + * @pin_idx: idx given by dev driver + * @module: reference to registering module + * @prop: dpll pin properties + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * valid allocated dpll_pin struct pointer if succeeded + * * ERR_PTR(-ENOMEM) - failed memory allocation + * * ERR_PTR(-EINVAL) - wrong pin type property value + */ +static struct dpll_pin * +dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct module *module, + const struct dpll_pin_properties *prop) +{ + struct dpll_pin *pin; + int ret; + + pin = kzalloc(sizeof(*pin), GFP_KERNEL); + if (!pin) + return ERR_PTR(-ENOMEM); + pin->pin_idx = pin_idx; + pin->clock_id = clock_id; + pin->module = module; + if (WARN_ON(prop->type < DPLL_PIN_TYPE_MUX || + prop->type > DPLL_PIN_TYPE_MAX)) { + ret = -EINVAL; + goto err; + } + pin->prop = prop; + refcount_set(&pin->refcount, 1); + xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC); + xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC); + ret = xa_alloc(&dpll_pin_xa, &pin->id, pin, xa_limit_16b, GFP_KERNEL); + if (ret) + goto err; + return pin; +err: + xa_destroy(&pin->dpll_refs); + xa_destroy(&pin->parent_refs); + kfree(pin); + return ERR_PTR(ret); +} + +/** + * dpll_pin_get - find existing or create new dpll pin + * @clock_id: clock_id of creator + * @pin_idx: idx given by dev driver + * @module: reference to registering module + * @prop: dpll pin properties + * + * Get existing object of a pin (unique for given arguments) or create new + * if doesn't exist yet. + * + * Context: shall be called under a lock (dpll_lock) + * Return: + * * valid allocated dpll_pin struct pointer if succeeded + * * ERR_PTR of an error + */ +struct dpll_pin * +dpll_pin_get(u64 clock_id, u32 pin_idx, struct module *module, + const struct dpll_pin_properties *prop) +{ + struct dpll_pin *pos, *ret = NULL; + unsigned long i; + + xa_for_each(&dpll_pin_xa, i, pos) { + if (pos->clock_id == clock_id && + pos->pin_idx == pin_idx && + pos->module == module) { + ret = pos; + refcount_inc(&ret->refcount); + break; + } + } + if (!ret) + ret = dpll_pin_alloc(clock_id, pin_idx, module, prop); + + return ret; +} +EXPORT_SYMBOL_GPL(dpll_pin_get); + +/** + * dpll_pin_put - decrease the refcount and free memory if possible + * @pin: pointer to a pin to be put + * + * Drop reference for a pin, if all references are gone, delete pin object. + * + * Context: shall be called under a lock (dpll_lock) + */ +void dpll_pin_put(struct dpll_pin *pin) +{ + if (!pin) + return; + if (refcount_dec_and_test(&pin->refcount)) { + xa_destroy(&pin->dpll_refs); + xa_destroy(&pin->parent_refs); + xa_erase(&dpll_pin_xa, pin->id); + kfree(pin); + } +} +EXPORT_SYMBOL_GPL(dpll_pin_put); + +static int +__dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, + const struct dpll_pin_ops *ops, void *priv) +{ + int ret; + + if (WARN_ON(!ops)) + return -EINVAL; + + ret = dpll_xa_ref_pin_add(&dpll->pin_refs, pin, ops, priv); + if (ret) + return ret; + ret = dpll_xa_ref_dpll_add(&pin->dpll_refs, dpll, ops, priv); + if (ret) + goto ref_pin_del; + else + dpll_pin_create_ntf(pin); + + return ret; + +ref_pin_del: + dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv); + return ret; +} + +/** + * dpll_pin_register - register the dpll pin in the subsystem + * @dpll: pointer to a dpll + * @pin: pointer to a dpll pin + * @ops: ops for a dpll pin ops + * @priv: pointer to private information of owner + * + * Context: Acquires a lock (dpll_lock) + * Return: + * * 0 on success + * * -EINVAL - missing pin ops + * * -ENOMEM - failed to allocate memory + */ +int +dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, + const struct dpll_pin_ops *ops, void *priv) +{ + int ret; + + mutex_lock(&dpll_lock); + if (WARN_ON(!(dpll->module == pin->module && + dpll->clock_id == pin->clock_id))) + ret = -EFAULT; + else + ret = __dpll_pin_register(dpll, pin, ops, priv); + mutex_unlock(&dpll_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(dpll_pin_register); + +static void +__dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, + const struct dpll_pin_ops *ops, void *priv) +{ + dpll_xa_ref_pin_del(&dpll->pin_refs, pin, ops, priv); + dpll_xa_ref_dpll_del(&pin->dpll_refs, dpll, ops, priv); +} + +/** + * dpll_pin_unregister - unregister dpll pin from dpll device + * @dpll: registered dpll pointer + * @pin: pointer to a pin + * @ops: ops for a dpll pin + * @priv: pointer to private information of owner + * + * Note: It does not free the memory + * Context: Acquires a lock (dpll_lock) + */ +void dpll_pin_unregister(struct dpll_device *dpll, struct dpll_pin *pin, + const struct dpll_pin_ops *ops, void *priv) +{ + if (WARN_ON(xa_empty(&dpll->pin_refs))) + return; + + mutex_lock(&dpll_lock); + __dpll_pin_unregister(dpll, pin, ops, priv); + mutex_unlock(&dpll_lock); +} +EXPORT_SYMBOL_GPL(dpll_pin_unregister); + +/** + * dpll_pin_on_pin_register - register a pin with a parent pin + * @parent: pointer to a parent pin + * @pin: pointer to a pin + * @ops: ops for a dpll pin + * @priv: pointer to private information of owner + * + * Register a pin with a parent pin, create references between them and + * between newly registered pin and dplls connected with a parent pin. + * + * Context: Acquires a lock (dpll_lock) + * Return: + * * 0 on success + * * -EINVAL missing pin or parent + * * -ENOMEM failed allocation + * * -EPERM if parent is not allowed + */ +int dpll_pin_on_pin_register(struct dpll_pin *parent, struct dpll_pin *pin, + const struct dpll_pin_ops *ops, void *priv) +{ + struct dpll_pin_ref *ref; + unsigned long i, stop; + int ret; + + if (WARN_ON(parent->prop->type != DPLL_PIN_TYPE_MUX)) + return -EINVAL; + ret = dpll_xa_ref_pin_add(&pin->parent_refs, parent, ops, priv); + if (ret) + goto unlock; + refcount_inc(&pin->refcount); + xa_for_each(&parent->dpll_refs, i, ref) { + mutex_lock(&dpll_lock); + ret = __dpll_pin_register(ref->dpll, pin, ops, priv); + mutex_unlock(&dpll_lock); + if (ret) { + stop = i; + goto dpll_unregister; + } + dpll_pin_create_ntf(pin); + } + + return ret; + +dpll_unregister: + xa_for_each(&parent->dpll_refs, i, ref) { + if (i < stop) { + mutex_lock(&dpll_lock); + __dpll_pin_unregister(ref->dpll, pin, ops, priv); + mutex_unlock(&dpll_lock); + } + } + refcount_dec(&pin->refcount); + dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv); +unlock: + return ret; +} +EXPORT_SYMBOL_GPL(dpll_pin_on_pin_register); + +/** + * dpll_pin_on_pin_unregister - unregister dpll pin from a parent pin + * @parent: pointer to a parent pin + * @pin: pointer to a pin + * @ops: ops for a dpll pin + * @priv: pointer to private information of owner + * + * Context: Acquires a lock (dpll_lock) + * Note: It does not free the memory + */ +void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin *pin, + const struct dpll_pin_ops *ops, void *priv) +{ + struct dpll_pin_ref *ref; + unsigned long i; + + mutex_lock(&dpll_lock); + dpll_pin_delete_ntf(pin); + dpll_xa_ref_pin_del(&pin->parent_refs, parent, ops, priv); + refcount_dec(&pin->refcount); + xa_for_each(&pin->dpll_refs, i, ref) { + __dpll_pin_unregister(ref->dpll, pin, ops, priv); + } + mutex_unlock(&dpll_lock); +} +EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister); + + +/** + * dpll_device_registration_first - get first registration of dpll device + * @dpll: pointer to a dpll + * + * Context: shall be called under a lock (dpll_lock) + * Return: pointer to the first registration of a dpll + */ +static struct dpll_device_registration * +dpll_device_registration_first(struct dpll_device *dpll) +{ + struct dpll_device_registration *reg; + + reg = list_first_entry_or_null((struct list_head *) &dpll->registration_list, + struct dpll_device_registration, list); + WARN_ON(!reg); + return reg; +} + +/** + * dpll_priv - get the dpll device private owner data + * @dpll: registered dpll pointer + * + * Context: shall be called under a lock (dpll_lock) + * Return: pointer to the first registration priv data + */ +void *dpll_priv(struct dpll_device *dpll) +{ + struct dpll_device_registration *reg; + + reg = dpll_device_registration_first(dpll); + return reg->priv; +} + +/** + * dpll_device_ops - get the dpll device ops pointer + * @dpll: registered dpll pointer + * + * Context: shall be called under a lock (dpll_lock) + * Return: pointer to the first registration ops of the dpll + */ +const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll) +{ + struct dpll_device_registration *reg; + + reg = dpll_device_registration_first(dpll); + return reg->ops; +} + +/** + * dpll_pin_registration_first - get first registration of dpll pin ref + * @ref: pointer to a pin ref struct + * + * Context: shall be called under a lock (dpll_lock) + * Return: pointer to the first registration of a dpll_pin_ref + */ +static struct dpll_pin_registration * +dpll_pin_registration_first(struct dpll_pin_ref *ref) +{ + struct dpll_pin_registration *reg; + + reg = list_first_entry_or_null(&ref->registration_list, + struct dpll_pin_registration, list); + WARN_ON(!reg); + return reg; +} + +/** + * dpll_pin_on_dpll_priv - get the dpll device private owner data + * @dpll: registered dpll pointer + * @pin: pointer to a pin + * + * Context: shall be called under a lock (dpll_lock) + * Return: pointer to the data + */ +void *dpll_pin_on_dpll_priv(struct dpll_device *dpll, + struct dpll_pin *pin) +{ + struct dpll_pin_registration *reg; + struct dpll_pin_ref *ref; + + ref = xa_load(&dpll->pin_refs, pin->pin_idx); + if (!ref) + return NULL; + reg = dpll_pin_registration_first(ref); + return reg->priv; +} + +/** + * dpll_pin_on_pin_priv - get the dpll pin private owner data + * @parent: pointer to a parent pin + * @pin: pointer to a pin + * + * Context: shall be called under a lock (dpll_lock) + * Return: pointer to the data + */ +void *dpll_pin_on_pin_priv(struct dpll_pin *parent, + struct dpll_pin *pin) +{ + struct dpll_pin_registration *reg; + struct dpll_pin_ref *ref; + + ref = xa_load(&pin->parent_refs, parent->pin_idx); + if (!ref) + return NULL; + reg = dpll_pin_registration_first(ref); + return reg->priv; +} + +/** + * dpll_pin_ops - get the pin ops pointer + * @ref: dpll pin ref + * + * Context: shall be called under a lock (dpll_lock) + * Return: pointer to the first ops registered with the pin + */ +const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref) +{ + struct dpll_pin_registration *reg; + + reg = dpll_pin_registration_first(ref); + return reg->ops; +} + +/** + * dpll_init - initialize dpll subsystem + * + * Return: + * 0 - success + * negative - netlink init error + */ +static int __init dpll_init(void) +{ + int ret; + + ret = dpll_netlink_init(); + if (ret) + goto error; + + return 0; + +error: + mutex_destroy(&dpll_lock); + return ret; +} +subsys_initcall(dpll_init); diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h new file mode 100644 index 000000000000..ef95e272937c --- /dev/null +++ b/drivers/dpll/dpll_core.h @@ -0,0 +1,104 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2023 Meta Platforms, Inc. and affiliates + * Copyright (c) 2023 Intel and affiliates + */ + +#ifndef __DPLL_CORE_H__ +#define __DPLL_CORE_H__ + +#include <linux/dpll.h> +#include <linux/list.h> +#include <linux/refcount.h> +#include "dpll_netlink.h" + +#define DPLL_REGISTERED XA_MARK_1 + +struct dpll_device_registration { + struct list_head list; + const struct dpll_device_ops *ops; + void *priv; +}; + +/** + * struct dpll_device - stores DPLL device internal data + * @id: unique id number for each device given by kernel + * @device_idx: id given by dev driver + * @clock_id: unique identifier (clock_id) of a dpll + * @module: module of creator + * @type: type of a dpll + * @pin_refs: stores pins registered within a dpll + * @mode_supported_mask: mask of supported modes + * @refcount: refcount + * @registration_list: list of registered ops and priv data of dpll owners + **/ +struct dpll_device { + u32 id; + u32 device_idx; + u64 clock_id; + struct module *module; + enum dpll_type type; + struct xarray pin_refs; + unsigned long mode_supported_mask; + refcount_t refcount; + struct list_head registration_list; +}; + +/** + * struct dpll_pin - structure for a dpll pin + * @id: unique id number for pin given by kernel + * @pin_idx: index of a pin given by dev driver + * @clock_id: clock_id of creator + * @module: module of creator + * @dpll_refs: hold referencees to dplls pin was registered with + * @parent_refs: hold references to parent pins pin was registered with + * @prop: pointer to pin properties given by registerer + * @rclk_dev_name: holds name of device when pin can recover clock from it + * @refcount: 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; +}; + +struct dpll_pin_registration { + struct list_head list; + const struct dpll_pin_ops *ops; + void *priv; +}; + +/** + * struct dpll_pin_ref - structure for referencing either dpll or pins + * @dpll: pointer to a dpll + * @pin: pointer to a pin + * @registration_list: list of ops and priv data registered with the ref + * @refcount: refcount + **/ +struct dpll_pin_ref { + union { + struct dpll_device *dpll; + struct dpll_pin *pin; + }; + struct list_head registration_list; + refcount_t refcount; +}; + +void *dpll_priv(struct dpll_device *dpll); +void *dpll_pin_on_dpll_priv(struct dpll_device *dpll, struct dpll_pin *pin); +void *dpll_pin_on_pin_priv(struct dpll_pin *parent, struct dpll_pin *pin); + +const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll); +struct dpll_device *dpll_device_get_by_id(int id); +const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref); +struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs); +extern struct xarray dpll_device_xa; +extern struct xarray dpll_pin_xa; +extern struct mutex dpll_lock; +#endif