[PATCH net 0/2] dpll: zl3073x: fix output pin esync and sibling notifications

[PATCH net 0/2] dpll: zl3073x: fix output pin esync and sibling notifications

[Ivan Vecera]

This series fixes two issues in the zl3073x output pin handling.

Patch 1 makes the embedded sync (eSync) configuration follow the output
carrier frequency. Changing an output pin's frequency did not update its
eSync period/width, so eSync ran at the wrong rate and, for a 1 Hz
carrier, could get stuck on because esync_get() hides it.

Patch 2 notifies the sibling pin of an output pair when a shared HW
setting (divisor, clock type, eSync period/width, phase compensation) is
changed through one pin, so userspace listening on the sibling pin sees
the change. It also serializes access to the per-DPLL pin list, which the
new sibling lookup walks, against pin registration and teardown.

Ivan Vecera (2):
  dpll: zl3073x: make embedded sync follow the output carrier frequency
  dpll: zl3073x: notify sibling pin when shared output config changes

 drivers/dpll/zl3073x/dpll.c | 192 +++++++++++++++++++++++++-----------
 drivers/dpll/zl3073x/out.h  |  36 +++++++
 2 files changed, 173 insertions(+), 55 deletions(-)

-- 
2.55.0

[PATCH net 1/2] dpll: zl3073x: make embedded sync follow the output carrier frequency

[Ivan Vecera]

Changing an output pin's frequency did not update its embedded sync
(eSync) configuration. The eSync period and width are computed relative
to the carrier:

    esync_n_period = out_freq / esync_freq
    esync_n_width  = div / 2

Once zl3073x_dpll_output_pin_frequency_set() changes div, both values no
longer match the new carrier, so the embedded sync runs at the wrong
frequency and duty cycle. Worse, if the new carrier is <= 1 Hz then
esync_get() returns -EOPNOTSUPP (out_freq <= 1), hiding the stale ESYNC
mode so the user can no longer turn it off - it is stuck on.

Factor the eSync handling into zl3073x_out_esync_{is_enabled,enable,
disable}() helpers and reuse them from esync_get()/esync_set(), then
adjust the eSync parameters in the output frequency change path for
non-N-divided outputs after updating div/width:

- If the new frequency is 1 Hz, disable eSync. It cannot work at a 1 Hz
  carrier and esync_get() hides it, so it must be cleared rather than
  left stuck on.
- Otherwise, if eSync is active, recompute its period and width for the
  new carrier keeping the 1 Hz eSync frequency.

N-divided outputs are left untouched: there esync_n_period is the N
divider and eSync is not supported anyway.

Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 67 ++++++++++++++++---------------------
 drivers/dpll/zl3073x/out.h  | 36 ++++++++++++++++++++
 2 files changed, 65 insertions(+), 38 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index f2e137475b40..9eec27331fe3 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -852,7 +852,7 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dpll_pin *pin = pin_priv;
 	const struct zl3073x_synth *synth;
 	const struct zl3073x_out *out;
-	u32 synth_freq, out_freq;
+	u32 synth_freq;
 	u8 out_id;
 
 	guard(mutex)(&zldpll->lock);
@@ -864,38 +864,28 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
 	 * for N-division is also used for the esync divider so both cannot
 	 * be used.
 	 */
-	if (zl3073x_out_is_ndiv(out))
+	if (zl3073x_out_is_ndiv(out) || !pin->esync_control)
 		return -EOPNOTSUPP;
 
 	/* Get attached synth frequency */
 	synth = zl3073x_synth_state_get(zldev, zl3073x_out_synth_get(out));
 	synth_freq = zl3073x_synth_freq_get(synth);
-	out_freq = synth_freq / out->div;
 
-	if (!pin->esync_control || out_freq <= 1)
+	/* The esync is not supported for 1 Hz base frequency */
+	if (synth_freq / out->div <= 1)
 		return -EOPNOTSUPP;
 
 	esync->range = esync_freq_ranges;
 	esync->range_num = ARRAY_SIZE(esync_freq_ranges);
 
-	if (zl3073x_out_clock_type_get(out) != ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC) {
-		/* No need to read esync data if it is not enabled */
+	if (zl3073x_out_esync_is_enabled(out)) {
+		esync->freq = 1;
+		esync->pulse = 25;
+	} else {
 		esync->freq = 0;
 		esync->pulse = 0;
-
-		return 0;
 	}
 
-	/* Compute esync frequency */
-	esync->freq = out_freq / out->esync_n_period;
-
-	/* By comparing the esync_pulse_width to the half of the pulse width
-	 * the esync pulse percentage can be determined.
-	 * Note that half pulse width is in units of half synth cycles, which
-	 * is why it reduces down to be output_div.
-	 */
-	esync->pulse = (50 * out->esync_n_width) / out->div;
-
 	return 0;
 }
 
@@ -926,32 +916,17 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 	if (zl3073x_out_is_ndiv(&out))
 		return -EOPNOTSUPP;
 
-	/* Update clock type in output mode */
-	if (freq)
-		zl3073x_out_clock_type_set(&out,
-					   ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC);
-	else
-		zl3073x_out_clock_type_set(&out,
-					   ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
-
-	/* If esync is being disabled just write mailbox and finish */
-	if (!freq)
+	if (!freq) {
+		zl3073x_out_esync_disable(&out);
 		return zl3073x_out_state_set(zldev, out_id, &out);
+	}
 
 	/* Get attached synth frequency */
 	synth = zl3073x_synth_state_get(zldev, zl3073x_out_synth_get(&out));
 	synth_freq = zl3073x_synth_freq_get(synth);
 
-	/* Compute and update esync period */
-	out.esync_n_period = synth_freq / (u32)freq / out.div;
-
-	/* Half of the period in units of 1/2 synth cycle can be represented by
-	 * the output_div. To get the supported esync pulse width of 25% of the
-	 * period the output_div can just be divided by 2. Note that this
-	 * assumes that output_div is even, otherwise some resolution will be
-	 * lost.
-	 */
-	out.esync_n_width = out.div / 2;
+	/* Enable 1PPS eSync for this pin frequency */
+	zl3073x_out_esync_enable(&out, synth_freq / out.div);
 
 	/* Commit output configuration */
 	return zl3073x_out_state_set(zldev, out_id, &out);
@@ -1009,6 +984,22 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		/* For 50/50 duty cycle the divisor is equal to width */
 		out.width = new_div;
 
+		/* The embedded sync period and width are computed relative to
+		 * the output carrier so they have to be adjusted to follow the
+		 * new frequency.
+		 */
+		if (zl3073x_out_esync_is_enabled(&out)) {
+			if (frequency == 1) {
+				/* Disable eSync if the new frequency is 1 Hz */
+				zl3073x_out_esync_disable(&out);
+			} else {
+				/* Update esync period and width according
+				 * the new frequency.
+				 */
+				zl3073x_out_esync_enable(&out, frequency);
+			}
+		}
+
 		/* Commit output configuration */
 		return zl3073x_out_state_set(zldev, out_id, &out);
 	}
diff --git a/drivers/dpll/zl3073x/out.h b/drivers/dpll/zl3073x/out.h
index 660889c57bff..f3e3ef27749a 100644
--- a/drivers/dpll/zl3073x/out.h
+++ b/drivers/dpll/zl3073x/out.h
@@ -134,4 +134,40 @@ static inline u8 zl3073x_out_synth_get(const struct zl3073x_out *out)
 	return FIELD_GET(ZL_OUTPUT_CTRL_SYNTH_SEL, out->ctrl);
 }
 
+static inline bool zl3073x_out_esync_is_enabled(const struct zl3073x_out *out)
+{
+	u8 clock_type = zl3073x_out_clock_type_get(out);
+
+	return clock_type == ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC ? true : false;
+}
+
+static inline
+void zl3073x_out_esync_enable(struct zl3073x_out *out, u32 base_freq)
+{
+	/* Period is expressed as the number of output divider clock cycles.
+	 * So for 1 Hz the period is equal to base frequency.
+	 */
+	out->esync_n_period = base_freq;
+
+	/* Half of the period in units of 1/2 synth cycle can be represented by
+	 * the output_div. To get the supported esync pulse width of 25% of the
+	 * period the output_div can just be divided by 2. Note that this
+	 * assumes that output_div is even, otherwise some resolution will be
+	 * lost.
+	 */
+	out->esync_n_width = out->div / 2;
+
+	/* Update the clock type */
+	zl3073x_out_clock_type_set(out, ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC);
+}
+
+static inline
+void zl3073x_out_esync_disable(struct zl3073x_out *out)
+{
+	/* Just update the clock type. The eSync period and width are
+	 * ignored for this clock type.
+	 */
+	zl3073x_out_clock_type_set(out, ZL_OUTPUT_MODE_CLOCK_TYPE_NORMAL);
+}
+
 #endif /* _ZL3073X_OUT_H */
-- 
2.55.0

[PATCH net 2/2] dpll: zl3073x: notify sibling pin when shared output config changes

[Ivan Vecera]

Each zl3073x output has a P-pin and an N-pin that share a single HW
output and, outside N-pin divide mode, share the output's divisor,
clock type, esync period/width and phase compensation registers.
Changing one of these settings through one pin's dpll_pin therefore
also changes the other (sibling) pin's effective configuration, but
only the pin the change was requested on gets a dpll_pin_change_ntf()
notification - userspace listening on the sibling pin is never told
its frequency, esync configuration or phase adjustment changed.

Add zl3073x_dpll_output_pin_sibling_get() to look up the other pin of
an output pair, and use it in frequency_set() (for the non-N-divided
signal formats, where the output divisor is shared), esync_set() and
phase_adjust_set() to notify the sibling pin, if it is registered,
whenever the shared HW state actually changes.

The three callbacks are switched from guard(mutex) to explicit
mutex_lock()/mutex_unlock() with goto-based unwinding, because the
notification must run after zldpll->lock is released: the notification
re-enters the pin get callbacks, which take zldpll->lock again, so
calling it under the lock would deadlock. The DPLL subsystem's own
dpll_lock is still held across the callback, so the __ (lock-held)
notification variant is used.

The sibling lookup walks the zldpll->pins list, so serialize all
accesses to that list with zldpll->lock: take it around list_add() and
list_del() in the pin (un)registration paths and assert it is held in
the list walkers. Without this the walk could race with concurrent pin
registration or teardown, and the sibling pin could be freed between
the lookup and the notification.

Fixes: 634ca2cb06d2 ("dpll: zl3073x: Add support to get/set esync on pins")
Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
Fixes: 6287262f761e ("dpll: zl3073x: Add support to adjust phase")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 133 +++++++++++++++++++++++++++++++-----
 1 file changed, 115 insertions(+), 18 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 9eec27331fe3..d71471d64aec 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -123,6 +123,8 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
 {
 	struct zl3073x_dpll_pin *pin;
 
+	lockdep_assert_held(&zldpll->lock);
+
 	list_for_each_entry(pin, &zldpll->pins, list) {
 		if (zl3073x_dpll_is_input_pin(pin) &&
 		    zl3073x_input_pin_ref_get(pin->id) == ref_id)
@@ -132,11 +134,39 @@ zl3073x_dpll_pin_get_by_ref(struct zl3073x_dpll *zldpll, u8 ref_id)
 	return NULL;
 }
 
+/**
+ * zl3073x_dpll_output_pin_sibling_get - get the other pin of an output pair
+ * @pin: output pin whose sibling is sought
+ *
+ * Output pin ids are allocated in P/N pairs (P even, N odd) that share a
+ * single HW output. Looks up the other pin of the pair, if it is
+ * registered as a dpll_pin on this DPLL.
+ *
+ * Return: pointer to sibling pin, or NULL if it is not registered
+ */
+static struct zl3073x_dpll_pin *
+zl3073x_dpll_output_pin_sibling_get(struct zl3073x_dpll_pin *pin)
+{
+	struct zl3073x_dpll_pin *sibling;
+
+	lockdep_assert_held(&pin->dpll->lock);
+
+	list_for_each_entry(sibling, &pin->dpll->pins, list) {
+		if (!zl3073x_dpll_is_input_pin(sibling) &&
+		    sibling->id == (pin->id ^ 1))
+			return sibling;
+	}
+
+	return NULL;
+}
+
 static struct zl3073x_dpll_pin *
 zl3073x_dpll_nco_pin_get(struct zl3073x_dpll *zldpll)
 {
 	struct zl3073x_dpll_pin *pin;
 
+	lockdep_assert_held(&zldpll->lock);
+
 	list_for_each_entry(pin, &zldpll->pins, list) {
 		if (zl3073x_dpll_is_nco_pin(pin))
 			return pin;
@@ -899,12 +929,14 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dpll *zldpll = dpll_priv;
 	struct zl3073x_dev *zldev = zldpll->dev;
 	struct zl3073x_dpll_pin *pin = pin_priv;
+	struct zl3073x_dpll_pin *sibling = NULL;
 	const struct zl3073x_synth *synth;
 	struct zl3073x_out out;
 	u32 synth_freq;
 	u8 out_id;
+	int rc;
 
-	guard(mutex)(&zldpll->lock);
+	mutex_lock(&zldpll->lock);
 
 	out_id = zl3073x_output_pin_out_get(pin->id);
 	out = *zl3073x_out_state_get(zldev, out_id);
@@ -913,12 +945,14 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 	 * for N-division is also used for the esync divider so both cannot
 	 * be used.
 	 */
-	if (zl3073x_out_is_ndiv(&out))
-		return -EOPNOTSUPP;
+	if (zl3073x_out_is_ndiv(&out)) {
+		rc = -EOPNOTSUPP;
+		goto unlock;
+	}
 
 	if (!freq) {
 		zl3073x_out_esync_disable(&out);
-		return zl3073x_out_state_set(zldev, out_id, &out);
+		goto commit;
 	}
 
 	/* Get attached synth frequency */
@@ -927,9 +961,24 @@ zl3073x_dpll_output_pin_esync_set(const struct dpll_pin *dpll_pin,
 
 	/* Enable 1PPS eSync for this pin frequency */
 	zl3073x_out_esync_enable(&out, synth_freq / out.div);
-
+commit:
 	/* Commit output configuration */
-	return zl3073x_out_state_set(zldev, out_id, &out);
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+	if (rc)
+		goto unlock;
+
+	/* The clock type, esync period and esync width are all shared by
+	 * both pins of the output pair, so the sibling pin's esync
+	 * configuration changes too and userspace has to be notified.
+	 */
+	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+unlock:
+	mutex_unlock(&zldpll->lock);
+
+	if (!rc && sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return rc;
 }
 
 static int
@@ -959,12 +1008,14 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dpll *zldpll = dpll_priv;
 	struct zl3073x_dev *zldev = zldpll->dev;
 	struct zl3073x_dpll_pin *pin = pin_priv;
+	struct zl3073x_dpll_pin *sibling = NULL;
 	const struct zl3073x_synth *synth;
 	u32 new_div, synth_freq;
 	struct zl3073x_out out;
 	u8 out_id;
+	int rc;
 
-	guard(mutex)(&zldpll->lock);
+	mutex_lock(&zldpll->lock);
 
 	out_id = zl3073x_output_pin_out_get(pin->id);
 	out = *zl3073x_out_state_get(zldev, out_id);
@@ -977,7 +1028,8 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 	/* Check signal format */
 	if (!zl3073x_out_is_ndiv(&out)) {
 		/* For non N-divided signal formats the frequency is computed
-		 * as division of synth frequency and output divisor.
+		 * as division of synth frequency and output divisor, which
+		 * is shared by both pins of the output pair.
 		 */
 		out.div = new_div;
 
@@ -1001,7 +1053,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		}
 
 		/* Commit output configuration */
-		return zl3073x_out_state_set(zldev, out_id, &out);
+		rc = zl3073x_out_state_set(zldev, out_id, &out);
+		if (rc)
+			goto unlock;
+
+		/* The other pin's frequency changed too - it has to be
+		 * notified about the change.
+		 */
+		sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+
+		goto unlock;
 	}
 
 	if (zl3073x_dpll_is_p_pin(pin)) {
@@ -1013,8 +1074,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		 * Update divisor for N-pin to keep N-pin frequency.
 		 */
 		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
-		if (!out.esync_n_period)
-			return -EINVAL;
+		if (!out.esync_n_period) {
+			rc = -EINVAL;
+			goto unlock;
+		}
 
 		/* Update the output divisor */
 		out.div = new_div;
@@ -1030,15 +1093,24 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		 * Update divisor for N-pin
 		 */
 		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
-		if (!out.esync_n_period)
-			return -EINVAL;
+		if (!out.esync_n_period) {
+			rc = -EINVAL;
+			goto unlock;
+		}
 	}
 
 	/* For 50/50 duty cycle the divisor is equal to width */
 	out.esync_n_width = out.esync_n_period;
 
 	/* Commit output configuration */
-	return zl3073x_out_state_set(zldev, out_id, &out);
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+unlock:
+	mutex_unlock(&zldpll->lock);
+
+	if (!rc && sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return rc;
 }
 
 static int
@@ -1077,10 +1149,12 @@ zl3073x_dpll_output_pin_phase_adjust_set(const struct dpll_pin *dpll_pin,
 	struct zl3073x_dpll *zldpll = dpll_priv;
 	struct zl3073x_dev *zldev = zldpll->dev;
 	struct zl3073x_dpll_pin *pin = pin_priv;
+	struct zl3073x_dpll_pin *sibling = NULL;
 	struct zl3073x_out out;
 	u8 out_id;
+	int rc;
 
-	guard(mutex)(&zldpll->lock);
+	mutex_lock(&zldpll->lock);
 
 	out_id = zl3073x_output_pin_out_get(pin->id);
 	out = *zl3073x_out_state_get(zldev, out_id);
@@ -1089,7 +1163,21 @@ zl3073x_dpll_output_pin_phase_adjust_set(const struct dpll_pin *dpll_pin,
 	out.phase_comp = phase_adjust / pin->phase_gran;
 
 	/* Update output configuration from mailbox */
-	return zl3073x_out_state_set(zldev, out_id, &out);
+	rc = zl3073x_out_state_set(zldev, out_id, &out);
+	if (rc)
+		goto unlock;
+
+	/* The phase compensation register is shared by both pins of the
+	 * output pair, so the sibling pin's phase adjustment changes too.
+	 */
+	sibling = zl3073x_dpll_output_pin_sibling_get(pin);
+unlock:
+	mutex_unlock(&zldpll->lock);
+
+	if (!rc && sibling)
+		__dpll_pin_change_ntf(sibling->dpll_pin);
+
+	return rc;
 }
 
 static int
@@ -1776,10 +1864,15 @@ static void
 zl3073x_dpll_pins_unregister(struct zl3073x_dpll *zldpll)
 {
 	struct zl3073x_dpll_pin *pin, *next;
+	LIST_HEAD(pin_list);
 
-	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
-		zl3073x_dpll_pin_unregister(pin);
+	mutex_lock(&zldpll->lock);
+	list_splice_init(&zldpll->pins, &pin_list);
+	mutex_unlock(&zldpll->lock);
+
+	list_for_each_entry_safe(pin, next, &pin_list, list) {
 		list_del(&pin->list);
+		zl3073x_dpll_pin_unregister(pin);
 		zl3073x_dpll_pin_free(pin);
 	}
 }
@@ -1897,7 +1990,9 @@ zl3073x_dpll_nco_pin_register(struct zl3073x_dpll *zldpll)
 	if (rc)
 		goto err_register;
 
+	mutex_lock(&zldpll->lock);
 	list_add(&pin->list, &zldpll->pins);
+	mutex_unlock(&zldpll->lock);
 
 	return 0;
 
@@ -1954,7 +2049,9 @@ zl3073x_dpll_pins_register(struct zl3073x_dpll *zldpll)
 			goto error;
 		}
 
+		mutex_lock(&zldpll->lock);
 		list_add(&pin->list, &zldpll->pins);
+		mutex_unlock(&zldpll->lock);
 	}
 
 	/* Register NCO virtual input pin */
-- 
2.55.0

Re: [PATCH net 0/2] dpll: zl3073x: fix output pin esync and sibling notifications

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 0/2] dpll: zl3073x: fix output pin esync and sibling notifications

[Ivan Vecera]

On 10/1/26 10:09 AM, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>   - How the issue was discovered, e.g. hit in production, hit during
>     development, syzbot report, manual code inspection, LLM or static
>     analysis tool scan.
> 
>   - Whether the issue was actually triggered, or is only theoretical
>     (e.g. found by code inspection). If it was triggered please include
>     the symptoms, like the stack trace or error messages.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Hi,

thanks, here is the missing information for both patches.

Both issues were found while developing and testing.

Patch 1 fixes a bug that was actually triggered. After changing an
output pin's frequency the embedded sync output stopped working
correctly, which I confirmed on an oscilloscope - the embedded sync ran
at the wrong frequency and duty cycle because its period and width still
matched the previous carrier. In addition, when the new carrier was
1 Hz, esync_get() returned -EOPNOTSUPP, so the stale eSync mode became
invisible and could no longer be disabled.

Patch 2 was found by code inspection rather than triggered at runtime.
Both pins of an output pair drive the same HW output, so changing e.g.
the esync configuration or phase adjustment through one pin also changes
the sibling pin's effective configuration. A dpll pin-get on the sibling
pin returns the correct current values, but because no change
notification is emitted for it, userspace is never told that the
configuration changed asynchronously through the other pin.

Regards,
Ivan

Re: [PATCH net 0/2] dpll: zl3073x: fix output pin esync and sibling notifications

[Ivan Vecera]

Need to send v2 due to coccicheck failure:

+/srv/nipa-builds-contest/testing/wt-cocci/drivers/dpll/zl3073x/out.h:141:63-68: 
WARNING: conversion to bool not needed here

due to this line:
     return clock_type == ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC ? true : false;

Thanks,
Ivan

Re: [PATCH net 1/2] dpll: zl3073x: make embedded sync follow the output carrier frequency

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/dpll/zl3073x/dpll.c,
  zl3073x_dpll_output_pin_esync_get() no longer works out the reported…

--

dpll: zl3073x: make embedded sync follow the output carrier frequency

This commit moves the output pin eSync handling into new
zl3073x_out_esync_{is_enabled,enable,disable}() helpers. When the output
divider changes in zl3073x_dpll_output_pin_frequency_set(), it recomputes
the eSync period and width, and it disables eSync when the new carrier is
1 Hz.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index f2e137475b40f..9eec27331fe3d 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -864,38 +864,28 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,

[ ... ]

>  	esync->range = esync_freq_ranges;
>  	esync->range_num = ARRAY_SIZE(esync_freq_ranges);
>  
> -	if (zl3073x_out_clock_type_get(out) != ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC) {
> -		/* No need to read esync data if it is not enabled */
> +	if (zl3073x_out_esync_is_enabled(out)) {
> +		esync->freq = 1;
> +		esync->pulse = 25;
> +	} else {
>  		esync->freq = 0;
>  		esync->pulse = 0;
> -
> -		return 0;
>  	}
>  
> -	/* Compute esync frequency */
> -	esync->freq = out_freq / out->esync_n_period;
> -
> -	/* By comparing the esync_pulse_width to the half of the pulse width
> -	 * the esync pulse percentage can be determined.
> -	 * Note that half pulse width is in units of half synth cycles, which
> -	 * is why it reduces down to be output_div.
> -	 */
> -	esync->pulse = (50 * out->esync_n_width) / out->div;
> -

[Severity: Medium]
Is it intended that zl3073x_dpll_output_pin_esync_get() now reports fixed
values instead of what is actually programmed in esync_n_period and
esync_n_width?

When the clock type is ESYNC, the getter now always reports freq=1 and
pulse=25, whatever the cached period and width registers contain. The
values can differ from the real signal in at least two cases.

First, zl3073x_out_state_fetch() in out.c loads ESYNC_PERIOD and
ESYNC_WIDTH from the chip at probe and only rejects a zero period:

	rc = zl3073x_read_u32(zldev, ZL_REG_OUTPUT_ESYNC_PERIOD,
			      &out->esync_n_period);
	...
	if (!out->esync_n_period) {

Stale values can carry over. They can come from NVM, or from an older
kernel that changed div without updating esync_n_period, which is the bug
this patch fixes. Take a 1 MHz carrier with esync_n_period = 10000000.
That is really a 0.1 Hz eSync. The old getter reported 0, and the new one
reports 1.

Second, zl3073x_out_esync_enable() in out.h sets:

	out->esync_n_width = out->div / 2;

Its comment notes that resolution is lost when div is odd. With div = 5,
the programmed pulse is 50 * 2 / 5 = 20%. The old getter reported 20, but
the new one reports 25.

The fixed readback also seems to block reprogramming through netlink.
dpll_pin_esync_set() in dpll_netlink.c does:

	if (freq == esync.freq)
		return 0;

So a request for 1 Hz meant to fix a stale period returns success without
calling esync_set(). The registers would only be rewritten by setting 0
and then 1.

In dpll.yaml, esync-frequency and esync-pulse describe the actual embedded
SYNC signal. The input pin getter in the same file only reports 1 after
checking the divider:

	esync->freq = ref->esync_n_div == ZL_REF_ESYNC_DIV_1HZ ? 1 : 0;

Should the output getter keep computing freq and pulse from the cached
esync_n_period, esync_n_width and div? If the change is intended, could
the commit message mention it? Right now it only describes moving code
into helpers, and the values userspace reads back have changed.

>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001080648.1424172-1-ivecera%40redhat.com

Re: [PATCH net 2/2] dpll: zl3073x: notify sibling pin when shared output config changes

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In N-div signal formats, setting the P-pin frequency can change
  the N-pin frequency without notifying the N-pin.
- [Low] The commit message overstates the locking change.

--

dpll: zl3073x: notify sibling pin when shared output config changes

This patch adds zl3073x_dpll_output_pin_sibling_get(). Frequency, esync
and phase adjust set callbacks use it to notify the other pin of an
output pair when shared output state changes. It also takes zldpll->lock
around the registration and unregistration changes to zldpll->pins.

> The sibling lookup walks the zldpll->pins list, so serialize all
> accesses to that list with zldpll->lock: take it around list_add() and
> list_del() in the pin (un)registration paths and assert it is held in
> the list walkers.

[Severity: Low]
Is this description accurate? After this patch, several walkers of
zldpll->pins still don't follow it.

zl3073x_dpll_changes_check() walks the list again after dropping the lock:

unlock:
	mutex_unlock(&zldpll->lock);
	...
	list_for_each_entry(pin, &zldpll->pins, list) {
		...
			dpll_pin_change_ntf(pin->dpll_pin);
	}

zl3073x_dpll_ref_sync_pairs_register() walks the list without the lock
and without an assertion. zl3073x_dpll_ptp_adjtime_phase_step() has no
lockdep_assert_held(), although its caller zl3073x_dpll_ptp_adjtime()
does hold the lock.

Also, zl3073x_dpll_pins_unregister() takes the lock around
list_splice_init(), not list_del(). list_del() runs outside the lock on
the spliced private list.

I don't see a live race here. The monitor work is only queued after
zl3073x_dpll_register() in zl3073x_dev_start(). It is cancelled
synchronously in zl3073x_dev_stop() before unregistration.
zl3073x_dpll_ref_sync_pairs_register() runs in the registration thread.

Could the message be narrowed to the lookup helpers? Alternatively, the
remaining walkers could take the lock, with assertions added where the
lock is already held.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 9eec27331fe3d..d71471d64aec4 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -1001,7 +1053,16 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>  		}
>  
>  		/* Commit output configuration */
> -		return zl3073x_out_state_set(zldev, out_id, &out);
> +		rc = zl3073x_out_state_set(zldev, out_id, &out);
> +		if (rc)
> +			goto unlock;
> +
> +		/* The other pin's frequency changed too - it has to be
> +		 * notified about the change.
> +		 */
> +		sibling = zl3073x_dpll_output_pin_sibling_get(pin);
> +
> +		goto unlock;
>  	}
>  
>  	if (zl3073x_dpll_is_p_pin(pin)) {
> @@ -1013,8 +1074,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>  		 * Update divisor for N-pin to keep N-pin frequency.
>  		 */
>  		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> -		if (!out.esync_n_period)
> -			return -EINVAL;
> +		if (!out.esync_n_period) {
> +			rc = -EINVAL;
> +			goto unlock;
> +		}
>  
>  		/* Update the output divisor */
>  		out.div = new_div;

[Severity: High]
In N-div mode, can setting the P-pin frequency change the N-pin
frequency without any notification for the N-pin?

This branch rewrites out.div, which the N-pin shares. It also recomputes
esync_n_period with truncating division, and only a zero result is
rejected. zl3073x_dev_output_pin_freq_get() reports the N-pin frequency
as:

	freq = zl3073x_synth_freq_get(synth) / out->div;

	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
		freq /= out->esync_n_period;

So if old_div * old_period is not a multiple of new_div, the N-pin
output changes.

For example, take a 600 MHz synth with div=60 and period=10, so P is
10 MHz and N is 1 MHz. Setting P to 2.5 MHz gives new_div=240 and
period = 600 / 240 = 2, so N becomes 1.25 MHz.

zl3073x_pin_check_freq() only checks that the requested frequency
divides the synth frequency, so this request is accepted.
zl3073x_out_state_set() then commits the new values to hardware.

The sibling is only looked up in the !zl3073x_out_is_ndiv() branch.
This path therefore reaches the unlock label with sibling == NULL, and
dpll_pin_freq_set() in the core only notifies the requested pin.

The commit message excludes N-div mode on the assumption that the N-pin
frequency is kept, but the arithmetic doesn't guarantee that.

Would it work to compare the sibling's effective frequency before and
after the commit and notify on change? Another option is to reject P
frequencies that can't keep the N frequency exact.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001080648.1424172-1-ivecera%40redhat.com