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

[PATCH net v2 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. This was hit
during development and confirmed on an oscilloscope.

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 is told the change happened.

Changes since v1:
- Patch 1: pass the actual output carrier (synth_freq / out.div) to
  zl3073x_out_esync_enable() in the frequency change path, matching
  esync_set(), instead of the raw requested frequency (no functional
  change, output frequencies always divide the synth frequency); drop
  the redundant "? true : false" in zl3073x_out_esync_is_enabled()
  reported by coccinelle (boolconv); fix a comment typo.
- No other functional changes; both commit messages now describe how the
  issues were found and whether they were triggered, as requested on the
  v1 thread.

v1: https://lore.kernel.org/netdev/20261001080648.1424172-1-ivecera@redhat.com/

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 | 196 ++++++++++++++++++++++++++----------
 drivers/dpll/zl3073x/out.h  |  34 +++++++
 2 files changed, 176 insertions(+), 54 deletions(-)

-- 
2.55.0

[PATCH net v2 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 = (synth_freq / div) / 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.

This was hit during development: after changing an output pin's
frequency the embedded sync output stopped working correctly, as
confirmed on an oscilloscope - it ran at the wrong frequency and duty
cycle because the eSync period and width still matched the old carrier.
For a new 1 Hz carrier esync_get() returned -EOPNOTSUPP, leaving eSync
enabled with no way to disable it.

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  | 34 +++++++++++++++++++
 2 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index f2e137475b40..19456fbeeea8 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 to
+				 * the new frequency.
+				 */
+				zl3073x_out_esync_enable(&out, synth_freq / out.div);
+			}
+		}
+
 		/* 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..a27f2dbb5738 100644
--- a/drivers/dpll/zl3073x/out.h
+++ b/drivers/dpll/zl3073x/out.h
@@ -134,4 +134,38 @@ 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)
+{
+	return zl3073x_out_clock_type_get(out) == ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC;
+}
+
+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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] zl3073x_dpll_output_pin_esync_get()
  (drivers/dpll/zl3073x/dpll.c, lines 881-887) now reports esync->freq =…
- [Low] When the requested output frequency equals the synthesizer
  frequency, new_div is 1.

--

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

This patch moves the output pin eSync handling into new
zl3073x_out_esync_{is_enabled,enable,disable}() helpers in out.h and uses
them from esync_get() and esync_set(). It also makes
zl3073x_dpll_output_pin_frequency_set() recompute the eSync period and
width after the divider changes, and disables eSync when the new frequency
is 1 Hz.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index f2e137475b40f..19456fbeeea84 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;
> -
>  	return 0;
>  }

[Severity: Medium]
The commit message calls this factoring the eSync handling into helpers.
However, zl3073x_dpll_output_pin_esync_get() now reports freq = 1 and
pulse = 25 based only on the clock type. It no longer computes them from
esync_n_period, esync_n_width and div. Is this change in the reported
values intended?

zl3073x_out_state_fetch() reads the period and width from the device
as-is, and only rejects a zero period:

drivers/dpll/zl3073x/out.c:zl3073x_out_state_fetch() {
    ...
	rc = zl3073x_read_u32(zldev, ZL_REG_OUTPUT_ESYNC_PERIOD,
			      &out->esync_n_period);
    ...
	rc = zl3073x_read_u32(zldev, ZL_REG_OUTPUT_ESYNC_WIDTH,
			      &out->esync_n_width);
    ...
}

The device could be running a non-1 Hz eSync, either from its flash
configuration or from a stale period left by an older kernel (probe does
not reset the chip). Would that now be reported as 1 Hz?

The pulse value looks affected too. zl3073x_out_esync_enable() sets
esync_n_width = out->div / 2, so an odd divider such as div = 5 gives 20%.
The old getter reported 20 here, and this one reports 25. With div = 1 the
width is 0, which is also reported as 25.

The frequency case is harder to recover from because the core skips the
driver when the requested value equals what esync_get() returns:

drivers/dpll/dpll_netlink.c:dpll_pin_esync_set() {
    ...
	if (freq == esync.freq)
		return 0;
    ...
}

Suppose the hardware eSync on an output is not 1 Hz and a user requests
1 Hz. Would the request succeed without reprogramming anything?

For comparison, zl3073x_dpll_input_pin_esync_get() still checks the
hardware divider before it reports 1 Hz:

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

Should the output pin getter keep deriving the values from the cached
register state in the same way?

[ ... ]

> @@ -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 to
> +				 * the new frequency.
> +				 */
> +				zl3073x_out_esync_enable(&out, synth_freq / out.div);

[Severity: Low]
What happens here when the requested frequency equals the synth frequency?
In that case new_div is 1, and zl3073x_out_esync_enable() sets:

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

That gives a width of 0 while the ESYNC clock type stays set.
zl3073x_out_state_set() then writes 0 to ZL_REG_OUTPUT_ESYNC_WIDTH, and
esync_get() still reports freq = 1 and pulse = 25.

The 1 Hz case disables eSync. Should a divider that cannot hold a non-zero
25% width also be rejected, or have eSync disabled?

zl3073x_dpll_output_pin_esync_set() already had the same div / 2
arithmetic. This patch adds a new path that reaches it automatically on a
frequency change.

> +			}
> +		}
> +
>  		/* Commit output configuration */
>  		return zl3073x_out_state_set(zldev, out_id, &out);
>  	}

[ ... ]

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

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

[Ivan Vecera]

On 10/6/26 10:08, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · 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).
> 
> - [Medium] zl3073x_dpll_output_pin_esync_get()
>    (drivers/dpll/zl3073x/dpll.c, lines 881-887) now reports esync->freq =…
> - [Low] When the requested output frequency equals the synthesizer
>    frequency, new_div is 1.
> 
> --
> 
> dpll: zl3073x: make embedded sync follow the output carrier frequency
> 
> This patch moves the output pin eSync handling into new
> zl3073x_out_esync_{is_enabled,enable,disable}() helpers in out.h and uses
> them from esync_get() and esync_set(). It also makes
> zl3073x_dpll_output_pin_frequency_set() recompute the eSync period and
> width after the divider changes, and disables eSync when the new frequency
> is 1 Hz.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index f2e137475b40f..19456fbeeea84 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;
>> -
>>   	return 0;
>>   }
> 
> [Severity: Medium]
> The commit message calls this factoring the eSync handling into helpers.
> However, zl3073x_dpll_output_pin_esync_get() now reports freq = 1 and
> pulse = 25 based only on the clock type. It no longer computes them from
> esync_n_period, esync_n_width and div. Is this change in the reported
> values intended?
> 
> zl3073x_out_state_fetch() reads the period and width from the device
> as-is, and only rejects a zero period:
> 
> drivers/dpll/zl3073x/out.c:zl3073x_out_state_fetch() {
>      ...
> 	rc = zl3073x_read_u32(zldev, ZL_REG_OUTPUT_ESYNC_PERIOD,
> 			      &out->esync_n_period);
>      ...
> 	rc = zl3073x_read_u32(zldev, ZL_REG_OUTPUT_ESYNC_WIDTH,
> 			      &out->esync_n_width);
>      ...
> }
> 
> The device could be running a non-1 Hz eSync, either from its flash
> configuration or from a stale period left by an older kernel (probe does
> not reset the chip). Would that now be reported as 1 Hz?
> 
> The pulse value looks affected too. zl3073x_out_esync_enable() sets
> esync_n_width = out->div / 2, so an odd divider such as div = 5 gives 20%.
> The old getter reported 20 here, and this one reports 25. With div = 1 the
> width is 0, which is also reported as 25.
> 
> The frequency case is harder to recover from because the core skips the
> driver when the requested value equals what esync_get() returns:
> 
> drivers/dpll/dpll_netlink.c:dpll_pin_esync_set() {
>      ...
> 	if (freq == esync.freq)
> 		return 0;
>      ...
> }
> 
> Suppose the hardware eSync on an output is not 1 Hz and a user requests
> 1 Hz. Would the request succeed without reprogramming anything?
> 
> For comparison, zl3073x_dpll_input_pin_esync_get() still checks the
> hardware divider before it reports 1 Hz:
> 
> 	esync->freq = ref->esync_n_div == ZL_REF_ESYNC_DIV_1HZ ? 1 : 0;
> 
> Should the output pin getter keep deriving the values from the cached
> register state in the same way?

Yes, reporting the fixed values is intended. The driver only ever
programs a 1 Hz / 25% eSync (zl3073x_out_esync_enable() always sets a
1 Hz period and width = div / 2), so esync_get() reports that nominal
model from the clock type and stays symmetric with esync_set(). I would
like to keep this behaviour.

What I do not want to do is claim that it faithfully reports a
configuration the driver did not establish. You are right that:

   - a non-1 Hz eSync left in flash or by an older kernel would now be
     reported as 1 Hz, and
   - an odd divider gives width = div / 2, i.e. a duty cycle below 25%
     (e.g. 20% for div = 5), which is still reported as 25%.

These are both configurations outside the driver's supported eSync model.
The getter deliberately presents the model the driver implements rather
than the raw register state. In practice mostly 1PPS and 1PP2S (0.5 Hz
that is not supported by DPLL core) are used.

The real problem is the commit message, which overstated this. It said
the computed form "could only differ for an inconsistent register state,
which is exactly the stale configuration this change prevents". That is
not accurate: a plain esync_set(1 Hz) over a stale non-1 Hz configuration
is short-circuited by the core (freq == esync.freq in
dpll_pin_esync_set()), so it would not reprogram anything. For the next
version I will reword that paragraph so it only claims what the change
actually does - report the nominal 1 Hz / 25% model - and explicitly
notes the two cases above as configurations the driver does not try to
reproduce. No code change is needed for this point.

> 
> [ ... ]
> 
>> @@ -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 to
>> +				 * the new frequency.
>> +				 */
>> +				zl3073x_out_esync_enable(&out, synth_freq / out.div);
> 
> [Severity: Low]
> What happens here when the requested frequency equals the synth frequency?
> In that case new_div is 1, and zl3073x_out_esync_enable() sets:
> 
> 	out->esync_n_width = out->div / 2;
> 
> That gives a width of 0 while the ESYNC clock type stays set.
> zl3073x_out_state_set() then writes 0 to ZL_REG_OUTPUT_ESYNC_WIDTH, and
> esync_get() still reports freq = 1 and pulse = 25.
> 
> The 1 Hz case disables eSync. Should a divider that cannot hold a non-zero
> 25% width also be rejected, or have eSync disabled?
> 
> zl3073x_dpll_output_pin_esync_set() already had the same div / 2
> arithmetic. This patch adds a new path that reaches it automatically on a
> frequency change.

Good catch. I will make this new path unreachable instead of handling
the width = 0 case. For the next version I plan to add a preparatory
patch ("dpll: zl3073x: reject output frequencies with too small divisor")
that requires an output divisor of at least 2, so zl3073x_pin_check_freq()
will no longer offer the synth frequency (div = 1) in freq_supported. The
only way frequency == synth_freq could still reach frequency_set() is if
the output is already at div = 1, and then the core skips the call
(freq == old_freq in dpll_pin_frequency_set()). With that in place,
frequency_set() can never compute new_div == 1, so the eSync path this
patch adds cannot produce width = 0.

Ivan

[PATCH net v2 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.

This was found by code inspection rather than triggered at runtime: 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 its configuration changed asynchronously through the other
pin.

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 the list_add()
in the pin registration paths and around the list_splice_init() that
detaches the list on teardown, 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 19456fbeeea8..b60d3280574a 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 v2 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: 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).

- [High] In zl3073x_dpll_output_pin_frequency_set()
  (drivers/dpll/zl3073x/dpll.c), the N-div case is excluded from the new…
- [Medium] Pins are on zldpll->pins only for part of the time they are
  registered with the DPLL core, so the new sibling lookup can miss a…

--

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

This commit adds zl3073x_dpll_output_pin_sibling_get(). The output pin
frequency, esync and phase adjust setters use it to send a change
notification for the other pin of a P/N output pair. The setters move from
guard(mutex) to explicit lock/unlock so the notification is sent after
zldpll->lock is dropped, and all accesses to zldpll->pins are now
serialized with zldpll->lock.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 19456fbeeea84..b60d3280574ad 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]
Can the N-pin's frequency still change on this path without a
notification?

In N-div mode, zl3073x_dev_output_pin_freq_get() computes the N-pin
frequency from both the shared divisor and esync_n_period:

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

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

When the P-pin frequency is set, the esync_n_period rescale above
truncates whenever old_period * old_div is not a multiple of new_div.

For example, take a 1 GHz synth with div = 10 (P = 100 MHz) and period = 4
(N = 25 MHz). Setting P to 40 MHz gives new_div = 25 and period = 40 / 25 =
1, so N becomes 40 MHz. Both frequencies pass zl3073x_pin_check_freq()
because they divide the synth frequency.

zl3073x_out_state_set() then commits the new DIV and ESYNC_PERIOD values.
However, sibling is only assigned in the non-N-div branch, so it is still
NULL at the tail of the function:

	if (!rc && sibling)
		__dpll_pin_change_ntf(sibling->dpll_pin);

No notification is sent for the N-pin, even though it is registered in
N-div mode.

The truncation itself predates this patch. The missing notification,
though, is the case this patch sets out to fix. The commit message says:

    share a single HW output and, outside N-pin divide mode, share the
    output's divisor, ...

and:

    ... to notify the sibling pin, if it is registered, whenever the
    shared HW state actually changes.

Is that accurate, given that the divisor is also part of the N-pin
frequency in N-div mode?

Should this branch either notify the N-pin when its effective frequency
changes, or reject a rescale that is not exact?

[ ... ]

> @@ -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);
>  	}

[Severity: Medium]
Can zl3073x_dpll_output_pin_sibling_get() miss a sibling that is already
registered with the DPLL core?

zl3073x_dpll_pin_register()->dpll_pin_register() publishes the pin. It
takes and drops dpll_lock and sends PIN_CREATE before the pin is added to
zldpll->pins here. P (even id) is registered before N (odd id), so this
window exists:

CPU1 (probe)
zl3073x_dpll_pins_register()
  zl3073x_dpll_pin_register(N)
    dpll_pin_register()
      N is now visible, PIN_CREATE sent

CPU2 (netlink PIN_SET on P, dpll_lock held)
zl3073x_dpll_output_pin_phase_adjust_set()
  zl3073x_out_state_set()
  zl3073x_dpll_output_pin_sibling_get()
    N is not on zldpll->pins yet, returns NULL

CPU1
  list_add(&pin->list, &zldpll->pins);

At that point userspace still has the CREATE values for N, but the shared
div, esync or phase_comp has already changed, and no change notification
is sent for N.

Teardown has the opposite window. zl3073x_dpll_pins_unregister() empties
the list before any dpll_pin_unregister() call:

	mutex_lock(&zldpll->lock);
	list_splice_init(&zldpll->pins, &pin_list);
	mutex_unlock(&zldpll->lock);

A PIN_SET on a P-pin that is still registered therefore also skips an
N-pin that is still registered. A DELETE notification follows in that
case.

This cannot cause a use-after-free. The sibling is freed only after
dpll_pin_unregister(), which needs dpll_lock, and the PIN_SET path holds
dpll_lock.

However, list membership does not match DPLL registration state, so the
notification this patch adds can still be lost. Should the lookup be tied
to registration state instead of list membership?

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

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

[Ivan Vecera]

On 10/6/26 10:08, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · 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).
> 
> - [High] In zl3073x_dpll_output_pin_frequency_set()
>    (drivers/dpll/zl3073x/dpll.c), the N-div case is excluded from the new…
> - [Medium] Pins are on zldpll->pins only for part of the time they are
>    registered with the DPLL core, so the new sibling lookup can miss a…
> 
> --
> 
> dpll: zl3073x: notify sibling pin when shared output config changes
> 
> This commit adds zl3073x_dpll_output_pin_sibling_get(). The output pin
> frequency, esync and phase adjust setters use it to send a change
> notification for the other pin of a P/N output pair. The setters move from
> guard(mutex) to explicit lock/unlock so the notification is sent after
> zldpll->lock is dropped, and all accesses to zldpll->pins are now
> serialized with zldpll->lock.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 19456fbeeea84..b60d3280574ad 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]
> Can the N-pin's frequency still change on this path without a
> notification?
> 
> In N-div mode, zl3073x_dev_output_pin_freq_get() computes the N-pin
> frequency from both the shared divisor and esync_n_period:
> 
> 	freq = zl3073x_synth_freq_get(synth) / out->div;
> 
> 	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> 		freq /= out->esync_n_period;
> 
> When the P-pin frequency is set, the esync_n_period rescale above
> truncates whenever old_period * old_div is not a multiple of new_div.
> 
> For example, take a 1 GHz synth with div = 10 (P = 100 MHz) and period = 4
> (N = 25 MHz). Setting P to 40 MHz gives new_div = 25 and period = 40 / 25 =
> 1, so N becomes 40 MHz. Both frequencies pass zl3073x_pin_check_freq()
> because they divide the synth frequency.
> 
> zl3073x_out_state_set() then commits the new DIV and ESYNC_PERIOD values.
> However, sibling is only assigned in the non-N-div branch, so it is still
> NULL at the tail of the function:
> 
> 	if (!rc && sibling)
> 		__dpll_pin_change_ntf(sibling->dpll_pin);
> 
> No notification is sent for the N-pin, even though it is registered in
> N-div mode.
> 
> The truncation itself predates this patch. The missing notification,
> though, is the case this patch sets out to fix. The commit message says:
> 
>      share a single HW output and, outside N-pin divide mode, share the
>      output's divisor, ...
> 
> and:
> 
>      ... to notify the sibling pin, if it is registered, whenever the
>      shared HW state actually changes.
> 
> Is that accurate, given that the divisor is also part of the N-pin
> frequency in N-div mode?
> 
> Should this branch either notify the N-pin when its effective frequency
> changes, or reject a rescale that is not exact?

I will take the second option and reject a rescale that is not exact. For
the next version I plan to add a preparatory patch ("dpll: zl3073x:
reject inexact frequencies for N-divided outputs"), ordered before this
notification patch, that changes the P-pin branch to

     prod = esync_n_period * div;
     esync_n_period = div_u64_rem(prod, new_div, &rem);
     if (rem)                   -> reject (-EINVAL)
     if (esync_n_period < 2)    -> reject (-EINVAL)

so the request is refused whenever the rescale is not exact. When the
P-pin set succeeds, the N-pin frequency is

     synth / (new_div * new_period)
       = synth / (new_div * (old_period * old_div / new_div))
       = synth / (old_div * old_period)

i.e. exactly the old N-pin frequency. So on this path the N-pin's
effective frequency never changes and no notification is required. With
that patch in front, the claim in this commit message ("to notify the
sibling pin ... whenever the shared HW state actually changes") holds,
and I will adjust the wording to make the N-div case explicit.

> [ ... ]
> 
>> @@ -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);
>>   	}
> 
> [Severity: Medium]
> Can zl3073x_dpll_output_pin_sibling_get() miss a sibling that is already
> registered with the DPLL core?
> 
> zl3073x_dpll_pin_register()->dpll_pin_register() publishes the pin. It
> takes and drops dpll_lock and sends PIN_CREATE before the pin is added to
> zldpll->pins here. P (even id) is registered before N (odd id), so this
> window exists:
> 
> CPU1 (probe)
> zl3073x_dpll_pins_register()
>    zl3073x_dpll_pin_register(N)
>      dpll_pin_register()
>        N is now visible, PIN_CREATE sent
> 
> CPU2 (netlink PIN_SET on P, dpll_lock held)
> zl3073x_dpll_output_pin_phase_adjust_set()
>    zl3073x_out_state_set()
>    zl3073x_dpll_output_pin_sibling_get()
>      N is not on zldpll->pins yet, returns NULL
> 
> CPU1
>    list_add(&pin->list, &zldpll->pins);
> 
> At that point userspace still has the CREATE values for N, but the shared
> div, esync or phase_comp has already changed, and no change notification
> is sent for N.
> 
> Teardown has the opposite window. zl3073x_dpll_pins_unregister() empties
> the list before any dpll_pin_unregister() call:
> 
> 	mutex_lock(&zldpll->lock);
> 	list_splice_init(&zldpll->pins, &pin_list);
> 	mutex_unlock(&zldpll->lock);
> 
> A PIN_SET on a P-pin that is still registered therefore also skips an
> N-pin that is still registered. A DELETE notification follows in that
> case.
> 
> This cannot cause a use-after-free. The sibling is freed only after
> dpll_pin_unregister(), which needs dpll_lock, and the PIN_SET path holds
> dpll_lock.
> 
> However, list membership does not match DPLL registration state, so the
> notification this patch adds can still be lost. Should the lookup be tied
> to registration state instead of list membership?

You are right, and I will tie the list membership to the registration
state. For the next version I plan to:

   - add the pin to zldpll->pins *before* dpll_pin_register() (removing it
     again on a registration failure), and
   - remove it from zldpll->pins only *after* dpll_pin_unregister(),
     instead of splicing the whole list away before unregistering any pin,

both under zldpll->lock. That closes both windows you described: a pin
that is registered - and thus reachable by a PIN_SET on its sibling - is
always present on the list, so the lookup will find it.

The opposite transient state (a pin on the list that is not yet, or no
longer, registered) is harmless:

   - __dpll_pin_change_ntf() is a no-op for such a pin, because
     dpll_pin_event_send() bails out on !dpll_pin_available(), and
   - there is no use-after-free, as you also noted: the sibling is freed
     only after dpll_pin_unregister(), which takes dpll_lock, and the
     PIN_SET path holds dpll_lock across the lookup and the notification.

Thanks,
Ivan