>From: Jiri Pirko <[email protected]>
>Sent: Friday, May 23, 2025 2:14 PM
>
>Thu, May 22, 2025 at 06:29:37PM +0200, [email protected]
>wrote:
>>Define function for reference sync pin registration and callback ops to
>>set/get current feature state.
>>
>>Implement netlink handler to fill netlink messages with reference sync
>>pin configuration of capable pins (pin-get).
>>
>>Implement netlink handler to call proper ops and configure reference
>>sync pin state (pin-set).
>>
>>Reviewed-by: Przemek Kitszel <[email protected]>
>>Reviewed-by: Milena Olech <[email protected]>
>>Signed-off-by: Arkadiusz Kubalewski <[email protected]>
>>---
>> v3:
>>- fix kdoc missing ':' after argument name ref_sync_pins,
>>- propagate ret in dpll_pin_ref_sync_state_set().
>>---
>> drivers/dpll/dpll_core.c | 27 ++++++
>> drivers/dpll/dpll_core.h | 2 +
>> drivers/dpll/dpll_netlink.c | 188 ++++++++++++++++++++++++++++++++----
>> include/linux/dpll.h | 10 ++
>> 4 files changed, 209 insertions(+), 18 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>index 20bdc52f63a5..805c7aca58c5 100644
>>--- a/drivers/dpll/dpll_core.c
>>+++ b/drivers/dpll/dpll_core.c
>>@@ -506,6 +506,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>module *module,
>> refcount_set(&pin->refcount, 1);
>> xa_init_flags(&pin->dpll_refs, XA_FLAGS_ALLOC);
>> xa_init_flags(&pin->parent_refs, XA_FLAGS_ALLOC);
>>+ xa_init_flags(&pin->ref_sync_pins, XA_FLAGS_ALLOC);
>> ret = xa_alloc_cyclic(&dpll_pin_xa, &pin->id, pin, xa_limit_32b,
>> &dpll_pin_xa_id, GFP_KERNEL);
>> if (ret < 0)
>>@@ -514,6 +515,7 @@ dpll_pin_alloc(u64 clock_id, u32 pin_idx, struct
>>module *module,
>> err_xa_alloc:
>> xa_destroy(&pin->dpll_refs);
>> xa_destroy(&pin->parent_refs);
>>+ xa_destroy(&pin->ref_sync_pins);
>> dpll_pin_prop_free(&pin->prop);
>> err_pin_prop:
>> kfree(pin);
>>@@ -595,6 +597,7 @@ void dpll_pin_put(struct dpll_pin *pin)
>> xa_erase(&dpll_pin_xa, pin->id);
>> xa_destroy(&pin->dpll_refs);
>> xa_destroy(&pin->parent_refs);
>>+ xa_destroy(&pin->ref_sync_pins);
>> dpll_pin_prop_free(&pin->prop);
>> kfree_rcu(pin, rcu);
>> }
>>@@ -783,6 +786,30 @@ void dpll_pin_on_pin_unregister(struct dpll_pin
>>*parent, struct dpll_pin *pin,
>> }
>> EXPORT_SYMBOL_GPL(dpll_pin_on_pin_unregister);
>>
>>+/**
>>+ * dpll_pin_ref_sync_pair_add - create a reference sync signal pin pair
>>+ * @base: pin which produces the base frequency
>>+ * @sync: pin which produces the sync signal
>>+ *
>>+ * Once pins are paired, the user-space configuration of reference sync
>>pair
>>+ * is possible.
>>+ * Context: Acquires a lock (dpll_lock)
>>+ * Return:
>>+ * * 0 on success
>>+ * * negative - error value
>>+ */
>>+int dpll_pin_ref_sync_pair_add(struct dpll_pin *base, struct dpll_pin
>>*sync)
>
>Perhaps call it "pin" and "ref_sync_pin"?
>
Sure, makes sense, will fix.
>
>>+{
>>+ int ret;
>>+
>>+ mutex_lock(&dpll_lock);
>>+ ret = xa_insert(&base->ref_sync_pins, sync->pin_idx, sync,
>>GFP_KERNEL);
>>+ mutex_unlock(&dpll_lock);
>>+
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(dpll_pin_ref_sync_pair_add);
>
>How do you handle remove?
>
I did not, this is hidden issue when you unregister sequentially.
Will fix that in next version.
>
>>+
>> static struct dpll_device_registration *
>> dpll_device_registration_first(struct dpll_device *dpll)
>> {
>>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>index 2b6d8ef1cdf3..93c68e78b351 100644
>>--- a/drivers/dpll/dpll_core.h
>>+++ b/drivers/dpll/dpll_core.h
>>@@ -44,6 +44,7 @@ struct dpll_device {
>> * @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
>>+ * @ref_sync_pins: hold references to pins for Reference SYNC feature
>> * @prop: pin properties copied from the registerer
>> * @rclk_dev_name: holds name of device when pin can recover clock
>>from it
>> * @refcount: refcount
>>@@ -56,6 +57,7 @@ struct dpll_pin {
>> struct module *module;
>> struct xarray dpll_refs;
>> struct xarray parent_refs;
>>+ struct xarray ref_sync_pins;
>> struct dpll_pin_properties prop;
>> refcount_t refcount;
>> struct rcu_head rcu;
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index c130f87147fa..854bd46a7d27 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -48,6 +48,24 @@ dpll_msg_add_dev_parent_handle(struct sk_buff *msg, u32
>>id)
>> return 0;
>> }
>>
>>+static bool dpll_pin_available(struct dpll_pin *pin)
>>+{
>>+ struct dpll_pin_ref *par_ref;
>>+ unsigned long i;
>>+
>>+ if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
>>+ return false;
>>+ xa_for_each(&pin->parent_refs, i, par_ref)
>>+ if (xa_get_mark(&dpll_pin_xa, par_ref->pin->id,
>>+ DPLL_REGISTERED))
>>+ return true;
>>+ xa_for_each(&pin->dpll_refs, i, par_ref)
>>+ if (xa_get_mark(&dpll_device_xa, par_ref->dpll->id,
>>+ DPLL_REGISTERED))
>>+ return true;
>>+ return false;
>>+}
>>+
>> /**
>> * dpll_msg_add_pin_handle - attach pin handle attribute to a given
>>message
>> * @msg: pointer to sk_buff message to attach a pin handle
>>@@ -408,6 +426,47 @@ dpll_msg_add_pin_esync(struct sk_buff *msg, struct
>>dpll_pin *pin,
>> return -EMSGSIZE;
>> }
>>
>>+static int
>>+dpll_msg_add_pin_ref_sync(struct sk_buff *msg, struct dpll_pin *pin,
>>+ struct dpll_pin_ref *ref,
>>+ struct netlink_ext_ack *extack)
>>+{
>>+ const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>>+ struct dpll_device *dpll = ref->dpll;
>>+ enum dpll_pin_state state;
>>+ void *pin_priv, *sp_priv;
>>+ struct dpll_pin *sp;
>
>Make sure you are consistent in variables naming. You call this "sync"
>whan you add it.
>
Sure, will do.
>
>>+ struct nlattr *nest;
>>+ unsigned long index;
>>+ int ret;
>>+
>>+ pin_priv = dpll_pin_on_dpll_priv(dpll, pin);
>>+ xa_for_each(&pin->ref_sync_pins, index, sp) {
>>+ if (!dpll_pin_available(sp))
>>+ continue;
>>+ sp_priv = dpll_pin_on_dpll_priv(dpll, sp);
>>+ if (WARN_ON(!ops->ref_sync_get))
>>+ return -EOPNOTSUPP;
>>+ ret = ops->ref_sync_get(pin, pin_priv, sp, sp_priv,
>>+ &state, extack);
>>+ if (ret)
>>+ return ret;
>>+ nest = nla_nest_start(msg, DPLL_A_PIN_REFERENCE_SYNC);
>>+ if (!nest)
>>+ return -EMSGSIZE;
>>+ if (nla_put_s32(msg, DPLL_A_PIN_ID, sp->id))
>>+ goto nest_cancel;
>>+ if (nla_put_s32(msg, DPLL_A_PIN_STATE, state))
>>+ goto nest_cancel;
>>+ nla_nest_end(msg, nest);
>>+ }
>>+ return 0;
>>+
>>+nest_cancel:
>>+ nla_nest_cancel(msg, nest);
>>+ return -EMSGSIZE;
>>+}
>>+
>> static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq)
>> {
>> int fs;
>>@@ -550,6 +609,10 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct
>>dpll_pin *pin,
>> if (ret)
>> return ret;
>> ret = dpll_msg_add_pin_esync(msg, pin, ref, extack);
>>+ if (ret)
>>+ return ret;
>>+ if (!xa_empty(&pin->ref_sync_pins))
>>+ ret = dpll_msg_add_pin_ref_sync(msg, pin, ref, extack);
>> if (ret)
>> return ret;
>> if (xa_empty(&pin->parent_refs))
>>@@ -642,24 +705,6 @@ __dpll_device_change_ntf(struct dpll_device *dpll)
>> return dpll_device_event_send(DPLL_CMD_DEVICE_CHANGE_NTF, dpll);
>> }
>>
>>-static bool dpll_pin_available(struct dpll_pin *pin)
>>-{
>>- struct dpll_pin_ref *par_ref;
>>- unsigned long i;
>>-
>>- if (!xa_get_mark(&dpll_pin_xa, pin->id, DPLL_REGISTERED))
>>- return false;
>>- xa_for_each(&pin->parent_refs, i, par_ref)
>>- if (xa_get_mark(&dpll_pin_xa, par_ref->pin->id,
>>- DPLL_REGISTERED))
>>- return true;
>>- xa_for_each(&pin->dpll_refs, i, par_ref)
>>- if (xa_get_mark(&dpll_device_xa, par_ref->dpll->id,
>>- DPLL_REGISTERED))
>>- return true;
>>- return false;
>>-}
>>-
>> /**
>> * dpll_device_change_ntf - notify that the dpll device has been changed
>> * @dpll: registered dpll pointer
>>@@ -887,6 +932,108 @@ dpll_pin_esync_set(struct dpll_pin *pin, struct
>>nlattr *a,
>> return ret;
>> }
>>
>>+static int
>>+dpll_pin_ref_sync_state_set(struct dpll_pin *pin, unsigned long
>>sync_pin_idx,
>>+ const enum dpll_pin_state state,
>>+ struct netlink_ext_ack *extack)
>>+
>>+{
>>+ struct dpll_pin_ref *ref, *failed;
>>+ const struct dpll_pin_ops *ops;
>>+ enum dpll_pin_state old_state;
>>+ struct dpll_pin *sync_pin;
>
>Again, please name this consistently...
>
Sure, will fix.
>
>>+ struct dpll_device *dpll;
>>+ unsigned long i;
>>+ int ret;
>>+
>>+ if (state != DPLL_PIN_STATE_CONNECTED &&
>>+ state != DPLL_PIN_STATE_DISCONNECTED)
>
>Extack message? But, isn't this sanitized already by policy? Then,
>please remove.
>
True, will remove.
>
>>+ return -EINVAL;
>>+ sync_pin = xa_find(&pin->ref_sync_pins, &sync_pin_idx, ULONG_MAX,
>>+ XA_PRESENT);
>>+ if (!sync_pin) {
>>+ NL_SET_ERR_MSG(extack, "reference sync pin not found");
>>+ return -EINVAL;
>>+ }
>>+ if (!dpll_pin_available(sync_pin)) {
>>+ NL_SET_ERR_MSG(extack, "reference sync pin not available");
>>+ return -EINVAL;
>>+ }
>>+ ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>>+ ASSERT_NOT_NULL(ref);
>>+ ops = dpll_pin_ops(ref);
>>+ if (!ops->ref_sync_set || !ops->ref_sync_get) {
>>+ NL_SET_ERR_MSG(extack, "reference sync not supported by this
>>pin");
>>+ return -EOPNOTSUPP;
>>+ }
>>+ dpll = ref->dpll;
>>+ ret = ops->ref_sync_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>>sync_pin,
>>+ dpll_pin_on_dpll_priv(dpll, sync_pin),
>>+ &old_state, extack);
>>+ if (ret) {
>>+ NL_SET_ERR_MSG(extack, "unable to get old reference sync
>>state");
>>+ return ret;
>>+ }
>>+ if (state == old_state)
>>+ return 0;
>>+ xa_for_each(&pin->dpll_refs, i, ref) {
>>+ ops = dpll_pin_ops(ref);
>>+ dpll = ref->dpll;
>>+ ret = ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
>>+ sync_pin,
>>+ dpll_pin_on_dpll_priv(dpll, sync_pin),
>>+ state, extack);
>>+ if (ret) {
>>+ failed = ref;
>>+ NL_SET_ERR_MSG_FMT(extack, "reference sync set failed
>>for
>>dpll_id:%u",
>>+ dpll->id);
>>+ goto rollback;
>>+ }
>>+ }
>>+ __dpll_pin_change_ntf(pin);
>>+
>>+ return 0;
>>+
>>+rollback:
>>+ xa_for_each(&pin->dpll_refs, i, ref) {
>>+ if (ref == failed)
>>+ break;
>>+ ops = dpll_pin_ops(ref);
>>+ dpll = ref->dpll;
>>+ if (ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
>>+ sync_pin,
>>+ dpll_pin_on_dpll_priv(dpll, sync_pin),
>>+ old_state, extack))
>>+ NL_SET_ERR_MSG(extack, "set reference sync rollback
>>failed");
>>+ }
>>+ return ret;
>>+}
>>+
>>+static int
>>+dpll_pin_ref_sync_set(struct dpll_pin *pin, struct nlattr *nest,
>>+ struct netlink_ext_ack *extack)
>>+{
>>+ struct nlattr *tb[DPLL_A_PIN_MAX + 1];
>>+ enum dpll_pin_state state;
>>+ u32 sync_pin_id;
>>+
>>+ nla_parse_nested(tb, DPLL_A_PIN_MAX, nest,
>>+ dpll_reference_sync_nl_policy, extack);
>>+ if (!tb[DPLL_A_PIN_ID]) {
>>+ NL_SET_ERR_MSG(extack, "sync pin id expected");
>>+ return -EINVAL;
>>+ }
>>+ sync_pin_id = nla_get_u32(tb[DPLL_A_PIN_ID]);
>>+
>>+ if (!tb[DPLL_A_PIN_STATE]) {
>>+ NL_SET_ERR_MSG(extack, "sync pin state expected");
>>+ return -EINVAL;
>>+ }
>>+ state = nla_get_u32(tb[DPLL_A_PIN_STATE]);
>>+
>>+ return dpll_pin_ref_sync_state_set(pin, sync_pin_id, state, extack);
>>+}
>>+
>> static int
>> dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx,
>> enum dpll_pin_state state,
>>@@ -1193,6 +1340,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin,
>>struct genl_info *info)
>> if (ret)
>> return ret;
>> break;
>>+ case DPLL_A_PIN_REFERENCE_SYNC:
>>+ ret = dpll_pin_ref_sync_set(pin, a, info->extack);
>>+ if (ret)
>>+ return ret;
>>+ break;
>> }
>> }
>>
>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>index 5e4f9ab1cf75..f1f1fdda67fe 100644
>>--- a/include/linux/dpll.h
>>+++ b/include/linux/dpll.h
>>@@ -95,6 +95,14 @@ struct dpll_pin_ops {
>> const struct dpll_device *dpll, void *dpll_priv,
>> struct dpll_pin_esync *esync,
>> struct netlink_ext_ack *extack);
>>+ int (*ref_sync_set)(const struct dpll_pin *pin, void *pin_priv,
>>+ const struct dpll_pin *ref_pin, void *ref_pin_priv,
>>+ const enum dpll_pin_state state,
>>+ struct netlink_ext_ack *extack);
>>+ int (*ref_sync_get)(const struct dpll_pin *pin, void *pin_priv,
>>+ const struct dpll_pin *ref_pin, void *ref_pin_priv,
>
>"ref_pin". This is 4th name of the same variable. Weird...
>
Yes, going to fix them all.
Thank you!
Arkadiusz
>
>>+ enum dpll_pin_state *state,
>>+ struct netlink_ext_ack *extack);
>> };
>>
>> struct dpll_pin_frequency {
>>@@ -194,6 +202,8 @@ int dpll_pin_on_pin_register(struct dpll_pin *parent,
>>struct dpll_pin *pin,
>> void dpll_pin_on_pin_unregister(struct dpll_pin *parent, struct dpll_pin
>>*pin,
>> const struct dpll_pin_ops *ops, void *priv);
>>
>>+int dpll_pin_ref_sync_pair_add(struct dpll_pin *base, struct dpll_pin
>>*sync);
>>+
>> int dpll_device_change_ntf(struct dpll_device *dpll);
>>
>> int dpll_pin_change_ntf(struct dpll_pin *pin);
>>--
>>2.38.1
>>
