Re: [Intel-wired-lan] [PATCH net-next v2 2/3] dpll: add Reference SYNC get/set

[Kubalewski, Arkadiusz]

>From: Jiri Pirko 
>Sent: Saturday, May 10, 2025 6:48 AM
>

[..]

>>
>>+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;
>>+ struct dpll_device *dpll;
>>+ unsigned long i;
>>+ int ret;
>>+
>>+ if (state != DPLL_PIN_STATE_CONNECTED &&
>>+ state != DPLL_PIN_STATE_DISCONNECTED)
>>+ 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 -EINVAL;
>
>Propagate ret. Not sure why you ignored my comment about this.
>

Not ignored, this was on the other series, just missed it, fixed in v3.

>
>
>>+ }
>>+ 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);
>
>Why you print id? User knows what he works on, don't he?
>

This is for easier debugging in case multiple parent dpll devices, that
pin can be connected with. If that fails on one it is better to know which
one caused failure.

Thank you!
Arkadiusz

[...]

Re: [Intel-wired-lan] [PATCH net-next v2 2/3] dpll: add Reference SYNC get/set

[Jiri Pirko]

Fri, May 09, 2025 at 02:46:50PM +0200, arkadiusz.kubalew...@intel.com 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 
>Reviewed-by: Milena Olech 
>Signed-off-by: Arkadiusz Kubalewski 
>---
>v2:
>- rename sync_pins -> ref_sync_pins within `struct dpll_pin` and add doxygen
>  description of ref_sync_pins,
>- improve commit message.
>---
> 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)
>+{
>+  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);
>+
> 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..d0b2b995fd62 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..7a3db0984b1e 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))
>+