[PATCH net-next v2 0/2] dpll: use pin owner's dpll ref for pin-level set callbacks

[PATCH net-next v2 0/2] dpll: use pin owner's dpll ref for pin-level set callbacks

[Ivan Vecera]

Pin-level attributes (frequency, phase adjust, embedded sync, reference
sync) are properties of the pin itself. The get callbacks already use
only the pin owner's DPLL reference, but the set callbacks iterate over
all registered DPLL devices — resulting in redundant HW writes for
drivers that share a pin across multiple DPLLs.

This series simplifies the set side to match the get side: call the set
callback only through the owner's reference.

Patch 1 prepares the zl3073x driver whose ref_sync_set callback had
per-channel behavior (setting priority on a single DPLL channel). It now
iterates all channels internally so it remains correct when invoked only
once.

Patch 2 drops the xa_for_each loops from dpll_pin_freq_set(),
dpll_pin_esync_set(), dpll_pin_ref_sync_state_set() and
dpll_pin_phase_adj_set(), along with the rollback logic and the per-ref
-EOPNOTSUPP validation scan. The dpll.rst documentation is updated to
reflect the new behavior.

v2:
  - Split zl3073x ref_sync_set fix into a separate preparation patch
  - Update dpll.rst documentation to match the new behavior
  - Expand commit message to describe the -EOPNOTSUPP check change

Ivan Vecera (2):
  dpll: zl3073x: update all DPLL channels on ref_sync_set
  dpll: use pin owner's dpll ref for pin-level attribute setting

 Documentation/driver-api/dpll.rst |  10 +-
 drivers/dpll/dpll_netlink.c       | 213 +++++++-----------------------
 drivers/dpll/zl3073x/dpll.c       |  46 +++++--
 3 files changed, 89 insertions(+), 180 deletions(-)


base-commit: 69963a0678a347d57c4ac8b16939dba216eb95ce
-- 
2.54.0

[PATCH net-next v2 1/2] dpll: zl3073x: update all DPLL channels on ref_sync_set

[Ivan Vecera]

zl3073x_dpll_input_pin_ref_sync_set() excludes the sync source from
automatic reference selection by setting its priority to NONE, but
currently only does this on the single DPLL channel whose pin_priv
was passed to the callback.

Since input pins are registered with every DPLL channel, the priority
update must cover all channels to prevent the sync source from
remaining a selectable candidate on the other channels. This is
a preparation for the following patch which changes the DPLL core to
invoke pin-level set callbacks only through the pin owner's reference
instead of iterating over all registered DPLL devices.

Replace the single-channel priority write with a list_for_each_entry()
loop over all DPLL channels. Each channel's lock is acquired
individually for its read-modify-write sequence. The guard(mutex) is
replaced with explicit mutex_lock/mutex_unlock to allow releasing the
owner's lock before iterating, avoiding nested locking of the same
mutex class.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 46 +++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 0488ae6ac486c8..1ea924a9b986b2 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -265,7 +265,7 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
 	struct zl3073x_ref ref;
 	int rc;
 
-	guard(mutex)(&zldpll->lock);
+	mutex_lock(&zldpll->lock);
 
 	ref_id = zl3073x_input_pin_ref_get(pin->id);
 	sync_ref_id = zl3073x_input_pin_ref_get(sync_pin->id);
@@ -285,17 +285,20 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
 		if (sync_freq > 8000) {
 			NL_SET_ERR_MSG(extack,
 				       "sync frequency must be 8 kHz or less");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto unlock;
 		}
 		if (ref_freq < 1000) {
 			NL_SET_ERR_MSG(extack,
 				       "clock frequency must be 1 kHz or more");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto unlock;
 		}
 		if (ref_freq <= sync_freq) {
 			NL_SET_ERR_MSG(extack,
 				       "clock frequency must be higher than sync frequency");
-			return -EINVAL;
+			rc = -EINVAL;
+			goto unlock;
 		}
 
 		zl3073x_ref_sync_pair_set(&ref, sync_ref_id);
@@ -308,20 +311,39 @@ zl3073x_dpll_input_pin_ref_sync_set(const struct dpll_pin *dpll_pin,
 
 	rc = zl3073x_ref_state_set(zldev, ref_id, &ref);
 	if (rc)
-		return rc;
+		goto unlock;
 
-	/* Exclude sync source from automatic reference selection by setting
-	 * its priority to NONE. On disconnect the priority is left as NONE
-	 * and the user must explicitly make the pin selectable again.
+	/* All code paths accessing per-channel reference priorities are
+	 * serialized by the subsystem dpll_lock, so it is safe to release
+	 * our lock here before iterating over the other channels.
+	 */
+	mutex_unlock(&zldpll->lock);
+
+	/* The datasheet recommends excluding the sync source from automatic
+	 * reference selection by setting its priority to NONE on all DPLL
+	 * channels. This is advisory - the ref sync pair is already
+	 * configured, so a failure here is not fatal. On disconnect the
+	 * priority is left as NONE and the user must explicitly make the
+	 * pin selectable again.
 	 */
 	if (state == DPLL_PIN_STATE_CONNECTED) {
-		chan = *zl3073x_chan_state_get(zldev, zldpll->id);
-		zl3073x_chan_ref_prio_set(&chan, sync_ref_id,
-					  ZL_DPLL_REF_PRIO_NONE);
-		return zl3073x_chan_state_set(zldev, zldpll->id, &chan);
+		list_for_each_entry(zldpll, &zldev->dplls, list) {
+			mutex_lock(&zldpll->lock);
+			chan = *zl3073x_chan_state_get(zldev, zldpll->id);
+			zl3073x_chan_ref_prio_set(&chan, sync_ref_id,
+						  ZL_DPLL_REF_PRIO_NONE);
+			if (zl3073x_chan_state_set(zldev, zldpll->id, &chan))
+				dev_warn(zldev->dev,
+					 "Failed to set ref prio on DPLL%u\n",
+					 zldpll->id);
+			mutex_unlock(&zldpll->lock);
+		}
 	}
 
 	return 0;
+unlock:
+	mutex_unlock(&zldpll->lock);
+	return rc;
 }
 
 static int
-- 
2.54.0

Re: [PATCH net-next v2 1/2] dpll: zl3073x: update all DPLL channels on ref_sync_set

[Ivan Vecera]

Replies to Sashiko findings:

 > Is this only a preparation change? ... Should this carry a Fixes: tag
 > so it gets picked up for stable?

It is preparation only. The old code is correct — the DPLL core's
xa_for_each loop calls ref_sync_set once per registered DPLL, and
since zl3073x registers input pins with every channel, each channel
gets its own callback invocation with the correct pin_priv. All
channels receive the priority update. The bug would only appear
after patch 2 removes the xa_for_each, which is why this patch moves
the iteration into the driver first.

 > Is the invariant in this comment accurate? ... 
zl3073x_dpll_pin_register()
 > reads them with no lock at all ... can a ref-sync DPLL_CMD_PIN_SET on
 > channel 0's pin enter the new loop and write zldev->chan[1].ref_prio
 > while channel 1's zl3073x_dpll_pin_register() is reading 
chan[1].ref_prio?

No race. dpll_lock is held across the entire dpll_pin_register()
call (acquired in dpll_core.c) and across the entire netlink
DPLL_CMD_PIN_SET handler (acquired in dpll_netlink.c). The device
startup loop calls zl3073x_dpll_register() -> dpll_pin_register()
for each channel under dpll_lock, and ref_sync_set also runs under
dpll_lock, so they cannot overlap.

 > Should the sync pin get a change notification for the other channels?

Good catch. Will add a sync_notify flag — set it when a channel's
priority is actually changed from non-NONE to NONE, and call
__dpll_pin_change_ntf(sync_pin->dpll_pin) after the loop if set.

 > Is the downgrade of this failure to a warning intentional? ...
 > The new comment calls the priority step advisory and "not fatal",
 > which reads as the opposite of the changelog statement that "the
 > priority update must cover all channels".

The ref sync pair configuration (zl3073x_ref_state_set) is the
essential operation whose failure is propagated. The per-channel
priority exclusion is a datasheet recommendation — the ref sync pair
functions regardless. Will align the changelog to say "recommended"
instead of "must". A mailbox failure here would indicate a serious
HW/bus problem that would surface through other error paths.

Thanks,
Ivan

[PATCH net-next v2 2/2] dpll: use pin owner's dpll ref for pin-level attribute setting

[Ivan Vecera]

Pin-level attributes (frequency, phase adjust, embedded sync, reference
sync) are properties of the pin itself, not of a particular DPLL device.
The get callbacks already use only the pin owner's DPLL reference
(via dpll_pin_own_dpll_ref_first()), but the set callbacks iterate over
all registered DPLL references and invoke the set operation on each one.

This is redundant because a pin is a single physical entity — setting
its frequency or phase adjust once through the owner's ops is sufficient.
Calling set on every registered DPLL just results in duplicate HW writes
for drivers that share a pin across multiple DPLL devices (e.g. ice
registers each input pin with both the EEC and PPS DPLL, zl3073x
registers input pins with every DPLL channel).

Simplify dpll_pin_freq_set(), dpll_pin_esync_set(),
dpll_pin_ref_sync_state_set() and dpll_pin_phase_adj_set() to call the
set callback only through the owner's DPLL reference, matching the
existing get-side behavior. This removes the xa_for_each iteration
loops, the now-unnecessary rollback logic, and several local variables.

The -EOPNOTSUPP validation loop, which checked ops support across all
owner-matching references, is replaced with a direct check on the
single owner reference returned by dpll_pin_own_dpll_ref_first().

The documentation in dpll.rst is updated to reflect that pin-level
attributes are set through the pin owner's dpll reference only.

No existing driver is affected:
  - ptp_ocp and mlx5 register each pin with a single DPLL.
  - ice registers input pins with two DPLLs (EEC and PPS) using
    identical ops and pin_priv; the set callbacks address the HW by
    pin index, not by DPLL, so the second call was a no-op.
  - zl3073x registers input pins with every DPLL channel; the set
    callbacks address HW by pin/ref ID regardless of DPLL. The
    ref_sync_set callback was the only one with per-channel behavior,
    addressed by the preceding patch.

Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 Documentation/driver-api/dpll.rst |  10 +-
 drivers/dpll/dpll_netlink.c       | 213 +++++++-----------------------
 2 files changed, 55 insertions(+), 168 deletions(-)

diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
index f83150917814e2..6fb50e53475c09 100644
--- a/Documentation/driver-api/dpll.rst
+++ b/Documentation/driver-api/dpll.rst
@@ -116,8 +116,8 @@ Shared pins
 A single pin object can be attached to multiple dpll devices.
 Then there are two groups of configuration knobs:
 
-1) Set on a pin - the configuration affects all dpll devices pin is
-   registered to (i.e., ``DPLL_A_PIN_FREQUENCY``),
+1) Set on a pin - the configuration is performed through the pin owner's
+   dpll reference only (i.e., ``DPLL_A_PIN_FREQUENCY``),
 2) Set on a pin-dpll tuple - the configuration affects only selected
    dpll device (i.e., ``DPLL_A_PIN_PRIO``, ``DPLL_A_PIN_STATE``,
    ``DPLL_A_PIN_DIRECTION``).
@@ -507,9 +507,9 @@ as well as parameter being configured (``DPLL_A_MODE``).
 ``DPLL_CMD_PIN_SET`` - to target a pin user must provide a
 ``DPLL_A_PIN_ID``, which is unique identifier of a pin in the system.
 Also configured pin parameters must be added.
-If ``DPLL_A_PIN_FREQUENCY`` is configured, this affects all the dpll
-devices that are connected with the pin, that is why frequency attribute
-shall not be enclosed in ``DPLL_A_PIN_PARENT_DEVICE``.
+If ``DPLL_A_PIN_FREQUENCY`` is configured, it is set through the pin
+owner's dpll reference only.  The frequency attribute shall not be
+enclosed in ``DPLL_A_PIN_PARENT_DEVICE``.
 Other attributes: ``DPLL_A_PIN_PRIO``, ``DPLL_A_PIN_STATE`` or
 ``DPLL_A_PIN_DIRECTION`` must be enclosed in
 ``DPLL_A_PIN_PARENT_DEVICE`` as their configuration relates to only one
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index afb31c0040382c..a909cd4451b008 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -1079,10 +1079,9 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 		  struct netlink_ext_ack *extack)
 {
 	u64 freq = nla_get_u64(a), old_freq;
-	struct dpll_pin_ref *ref, *failed;
 	const struct dpll_pin_ops *ops;
+	struct dpll_pin_ref *ref;
 	struct dpll_device *dpll;
-	unsigned long i;
 	int ret;
 
 	if (!dpll_pin_is_freq_supported(pin, freq)) {
@@ -1090,22 +1089,17 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 		return -EINVAL;
 	}
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if ((!ops->frequency_set || !ops->frequency_get) &&
-		    ref->dpll->module == pin->module &&
-		    ref->dpll->clock_id == pin->clock_id) {
-			NL_SET_ERR_MSG(extack,
-				       "frequency set not supported by the device");
-			return -EOPNOTSUPP;
-		}
-	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
 	if (!ref) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
 	ops = dpll_pin_ops(ref);
+	if (!ops->frequency_set || !ops->frequency_get) {
+		NL_SET_ERR_MSG(extack,
+			       "frequency set not supported by the device");
+		return -EOPNOTSUPP;
+	}
 	dpll = ref->dpll;
 	ret = ops->frequency_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
 				 dpll_priv(dpll), &old_freq, extack);
@@ -1116,68 +1110,42 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 	if (freq == old_freq)
 		return 0;
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if (!ops->frequency_set)
-			continue;
-		dpll = ref->dpll;
-		ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-					 dpll, dpll_priv(dpll), freq, extack);
-		if (ret) {
-			failed = ref;
-			NL_SET_ERR_MSG_FMT(extack, "frequency set failed for dpll_id:%u",
-					   dpll->id);
-			goto rollback;
-		}
+	ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				 dpll, dpll_priv(dpll), freq, extack);
+	if (ret) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "frequency set failed for dpll_id:%u",
+				   dpll->id);
+		return ret;
 	}
 	__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);
-		if (!ops->frequency_set)
-			continue;
-		dpll = ref->dpll;
-		if (ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-				       dpll, dpll_priv(dpll), old_freq, extack))
-			NL_SET_ERR_MSG(extack, "set frequency rollback failed");
-	}
-	return ret;
 }
 
 static int
 dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a,
 		   struct netlink_ext_ack *extack)
 {
-	struct dpll_pin_ref *ref, *failed;
 	const struct dpll_pin_ops *ops;
 	struct dpll_pin_esync esync;
 	u64 freq = nla_get_u64(a);
+	struct dpll_pin_ref *ref;
 	struct dpll_device *dpll;
 	bool supported = false;
-	unsigned long i;
-	int ret;
+	int ret, i;
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if ((!ops->esync_set || !ops->esync_get) &&
-		    ref->dpll->module == pin->module &&
-		    ref->dpll->clock_id == pin->clock_id) {
-			NL_SET_ERR_MSG(extack,
-				       "embedded sync feature is not supported by this device");
-			return -EOPNOTSUPP;
-		}
-	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
 	if (!ref) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
 	ops = dpll_pin_ops(ref);
+	if (!ops->esync_set || !ops->esync_get) {
+		NL_SET_ERR_MSG(extack,
+			       "embedded sync feature is not supported by this device");
+		return -EOPNOTSUPP;
+	}
 	dpll = ref->dpll;
 	ret = ops->esync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
 			     dpll_priv(dpll), &esync, extack);
@@ -1196,44 +1164,17 @@ dpll_pin_esync_set(struct dpll_pin *pin, struct nlattr *a,
 		return -EINVAL;
 	}
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		void *pin_dpll_priv;
-
-		ops = dpll_pin_ops(ref);
-		if (!ops->esync_set)
-			continue;
-		dpll = ref->dpll;
-		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
-		ret = ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
-				      freq, extack);
-		if (ret) {
-			failed = ref;
-			NL_SET_ERR_MSG_FMT(extack,
-					   "embedded sync frequency set failed for dpll_id: %u",
-					   dpll->id);
-			goto rollback;
-		}
+	ret = ops->esync_set(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
+			     dpll_priv(dpll), freq, extack);
+	if (ret) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "embedded sync frequency set failed for dpll_id: %u",
+				   dpll->id);
+		return ret;
 	}
 	__dpll_pin_change_ntf(pin);
 
 	return 0;
-
-rollback:
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		void *pin_dpll_priv;
-
-		if (ref == failed)
-			break;
-		ops = dpll_pin_ops(ref);
-		if (!ops->esync_set)
-			continue;
-		dpll = ref->dpll;
-		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
-		if (ops->esync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
-				   esync.freq, extack))
-			NL_SET_ERR_MSG(extack, "set embedded sync frequency rollback failed");
-	}
-	return ret;
 }
 
 static int
@@ -1241,14 +1182,12 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin,
 			    unsigned long ref_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 *ref_sync_pin;
+	struct dpll_pin_ref *ref;
 	struct dpll_device *dpll;
-	unsigned long i;
 	int ret;
 
 	ref_sync_pin = xa_find(&pin->ref_sync_pins, &ref_sync_pin_idx,
@@ -1282,42 +1221,20 @@ dpll_pin_ref_sync_state_set(struct dpll_pin *pin,
 	}
 	if (state == old_state)
 		return 0;
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if (!ops->ref_sync_set)
-			continue;
-		dpll = ref->dpll;
-		ret = ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-					ref_sync_pin,
-					dpll_pin_on_dpll_priv(dpll,
-							      ref_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;
-		}
+
+	ret = ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				ref_sync_pin,
+				dpll_pin_on_dpll_priv(dpll, ref_sync_pin),
+				state, extack);
+	if (ret) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "reference sync set failed for dpll_id:%u",
+				   dpll->id);
+		return ret;
 	}
 	__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);
-		if (!ops->ref_sync_set)
-			continue;
-		dpll = ref->dpll;
-		if (ops->ref_sync_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-				      ref_sync_pin,
-				      dpll_pin_on_dpll_priv(dpll, ref_sync_pin),
-				      old_state, extack))
-			NL_SET_ERR_MSG(extack, "set reference sync rollback failed");
-	}
-	return ret;
 }
 
 static int
@@ -1478,11 +1395,10 @@ static int
 dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 		       struct netlink_ext_ack *extack)
 {
-	struct dpll_pin_ref *ref, *failed;
 	const struct dpll_pin_ops *ops;
 	s32 phase_adj, old_phase_adj;
+	struct dpll_pin_ref *ref;
 	struct dpll_device *dpll;
-	unsigned long i;
 	int ret;
 
 	phase_adj = nla_get_s32(phase_adj_attr);
@@ -1499,21 +1415,16 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 		return -EINVAL;
 	}
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if ((!ops->phase_adjust_set || !ops->phase_adjust_get) &&
-		    ref->dpll->module == pin->module &&
-		    ref->dpll->clock_id == pin->clock_id) {
-			NL_SET_ERR_MSG(extack, "phase adjust not supported");
-			return -EOPNOTSUPP;
-		}
-	}
 	ref = dpll_pin_own_dpll_ref_first(pin);
 	if (!ref) {
 		NL_SET_ERR_MSG(extack, "pin owner dpll not found");
 		return -ENODEV;
 	}
 	ops = dpll_pin_ops(ref);
+	if (!ops->phase_adjust_set || !ops->phase_adjust_get) {
+		NL_SET_ERR_MSG(extack, "phase adjust not supported");
+		return -EOPNOTSUPP;
+	}
 	dpll = ref->dpll;
 	ret = ops->phase_adjust_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
 				    dpll, dpll_priv(dpll), &old_phase_adj,
@@ -1525,41 +1436,17 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr *phase_adj_attr,
 	if (phase_adj == old_phase_adj)
 		return 0;
 
-	xa_for_each(&pin->dpll_refs, i, ref) {
-		ops = dpll_pin_ops(ref);
-		if (!ops->phase_adjust_set)
-			continue;
-		dpll = ref->dpll;
-		ret = ops->phase_adjust_set(pin,
-					    dpll_pin_on_dpll_priv(dpll, pin),
-					    dpll, dpll_priv(dpll), phase_adj,
-					    extack);
-		if (ret) {
-			failed = ref;
-			NL_SET_ERR_MSG_FMT(extack,
-					   "phase adjust set failed for dpll_id:%u",
-					   dpll->id);
-			goto rollback;
-		}
+	ret = ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
+				    dpll, dpll_priv(dpll), phase_adj, extack);
+	if (ret) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "phase adjust set failed for dpll_id:%u",
+				   dpll->id);
+		return ret;
 	}
 	__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);
-		if (!ops->phase_adjust_set)
-			continue;
-		dpll = ref->dpll;
-		if (ops->phase_adjust_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
-					  dpll, dpll_priv(dpll), old_phase_adj,
-					  extack))
-			NL_SET_ERR_MSG(extack, "set phase adjust rollback failed");
-	}
-	return ret;
 }
 
 static int
-- 
2.54.0

Re: [PATCH net-next v2 2/2] dpll: use pin owner's dpll ref for pin-level attribute setting

[Paolo Abeni]

On 8/3/26 2:02 PM, Ivan Vecera wrote:
> Pin-level attributes (frequency, phase adjust, embedded sync, reference
> sync) are properties of the pin itself, not of a particular DPLL device.
> The get callbacks already use only the pin owner's DPLL reference
> (via dpll_pin_own_dpll_ref_first()), but the set callbacks iterate over
> all registered DPLL references and invoke the set operation on each one.
> 
> This is redundant because a pin is a single physical entity — setting
> its frequency or phase adjust once through the owner's ops is sufficient.
> Calling set on every registered DPLL just results in duplicate HW writes
> for drivers that share a pin across multiple DPLL devices (e.g. ice
> registers each input pin with both the EEC and PPS DPLL, zl3073x
> registers input pins with every DPLL channel).
> 
> Simplify dpll_pin_freq_set(), dpll_pin_esync_set(),
> dpll_pin_ref_sync_state_set() and dpll_pin_phase_adj_set() to call the
> set callback only through the owner's DPLL reference, matching the
> existing get-side behavior. This removes the xa_for_each iteration
> loops, the now-unnecessary rollback logic, and several local variables.
> 
> The -EOPNOTSUPP validation loop, which checked ops support across all
> owner-matching references, is replaced with a direct check on the
> single owner reference returned by dpll_pin_own_dpll_ref_first().
> 
> The documentation in dpll.rst is updated to reflect that pin-level
> attributes are set through the pin owner's dpll reference only.
> 
> No existing driver is affected:
>   - ptp_ocp and mlx5 register each pin with a single DPLL.
>   - ice registers input pins with two DPLLs (EEC and PPS) using
>     identical ops and pin_priv; the set callbacks address the HW by
>     pin index, not by DPLL, so the second call was a no-op.
>   - zl3073x registers input pins with every DPLL channel; the set
>     callbacks address HW by pin/ref ID regardless of DPLL. The
>     ref_sync_set callback was the only one with per-channel behavior,
>     addressed by the preceding patch.
> 
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>  Documentation/driver-api/dpll.rst |  10 +-
>  drivers/dpll/dpll_netlink.c       | 213 +++++++-----------------------
>  2 files changed, 55 insertions(+), 168 deletions(-)
> 
> diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
> index f83150917814e2..6fb50e53475c09 100644
> --- a/Documentation/driver-api/dpll.rst
> +++ b/Documentation/driver-api/dpll.rst
> @@ -116,8 +116,8 @@ Shared pins
>  A single pin object can be attached to multiple dpll devices.
>  Then there are two groups of configuration knobs:
>  
> -1) Set on a pin - the configuration affects all dpll devices pin is
> -   registered to (i.e., ``DPLL_A_PIN_FREQUENCY``),
> +1) Set on a pin - the configuration is performed through the pin owner's
> +   dpll reference only (i.e., ``DPLL_A_PIN_FREQUENCY``),

I find the new text confusing; it seems to me that the pin configuration
now affects a single DPLL.

Sashiko nipa has several comments, please have a look:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260803120245.56046-1-ivecera%40redhat.com

and also please be aware of net-next commit c82ff94592fb.

/P

/P

Re: [PATCH net-next v2 2/2] dpll: use pin owner's dpll ref for pin-level attribute setting

[Ivan Vecera]

On 8/6/26 11:41 AM, Paolo Abeni wrote:
> On 8/3/26 2:02 PM, Ivan Vecera wrote:
>> ...
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>>   Documentation/driver-api/dpll.rst |  10 +-
>>   drivers/dpll/dpll_netlink.c       | 213 +++++++-----------------------
>>   2 files changed, 55 insertions(+), 168 deletions(-)
>>
>> diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
>> index f83150917814e2..6fb50e53475c09 100644
>> --- a/Documentation/driver-api/dpll.rst
>> +++ b/Documentation/driver-api/dpll.rst
>> @@ -116,8 +116,8 @@ Shared pins
>>   A single pin object can be attached to multiple dpll devices.
>>   Then there are two groups of configuration knobs:
>>   
>> -1) Set on a pin - the configuration affects all dpll devices pin is
>> -   registered to (i.e., ``DPLL_A_PIN_FREQUENCY``),
>> +1) Set on a pin - the configuration is performed through the pin owner's
>> +   dpll reference only (i.e., ``DPLL_A_PIN_FREQUENCY``),
> 
> I find the new text confusing; it seems to me that the pin configuration
> now affects a single DPLL.
> 
> Sashiko nipa has several comments, please have a look:
> 
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260803120245.56046-1-ivecera%40redhat.com
> 
> and also please be aware of net-next commit c82ff94592fb.

Paolo, thanks for pointing out...

Regarding Sashiko's findings in this patch...

 > Should this list item keep describing the observable effect rather than
 > the kernel call path? ... The new text reads as if the frequency change
 > is scoped to a single dpll device, while the commit message argues the
 > opposite.

You are right. The documentation should describe the user-visible
behavior, not the implementation. The pin is one physical entity and
the frequency change is visible on every dpll the pin is registered
to. Will reword to keep the observable-effect perspective.

 > The removed sentence carried the reason why the attribute must not be
 > nested in DPLL_A_PIN_PARENT_DEVICE ... After the rewording the uAPI
 > requirement is stated without any rationale.

Good point. Will restore the rationale — the attribute is not nested
in DPLL_A_PIN_PARENT_DEVICE because it is a property of the pin
itself, not of a specific pin-dpll relationship.

 > Does the zl3073x implementation added by the preceding patch still
 > report errors here? ... the per-channel priority commit failure is
 > only warned about, and the function then falls through to return 0.

See the reply to the preceding patch. The ref sync pair configuration
(zl3073x_ref_state_set) is the essential part and its failure is
propagated. The per-channel priority exclusion is advisory per the
datasheet — a failure there does not invalidate the ref sync pair
and is reported via dev_warn. The old code propagated the error only
because it was a single-channel write that happened to be the last
statement.

Thanks,
Ivan