[PATCH net v5 0/5] dpll: zl3073x: fix output pin frequency, esync and sibling notifications

[PATCH net v5 0/5] dpll: zl3073x: fix output pin frequency, esync and sibling notifications

[Ivan Vecera]

This series fixes several issues in the zl3073x output pin handling:

1. Do not offer 0 Hz as a supported frequency, it leads to a division
   by zero.
2. Reject N-divided output frequencies that cannot be set exactly.
3. Reject output frequencies whose divisor is too small for the HW.
4. Update the eSync period and width when the output frequency changes.
5. Notify the sibling pin when a shared output setting changes.

Changes since v4:
- Reorder: the setter checks for N-divided outputs (now patch 2) come
  before the probe-time divisor check (now patch 3), so every commit is
  consistent.
- Patch 1: move the 0 Hz check in zl3073x_pin_check_freq() here from
  the divisor patch, so the division by zero is fixed at this commit.
- Patch 5: fix the comment in zl3073x_dpll_nco_pin_register().
- Say in each commit message how the issue was found.

Changes since v3:
- New patch 1 to fix a division by zero reported by Sashiko.
- Patch 2: keep the P-pin quotient in 64 bits and reject values above
  U32_MAX; check esync_n_period < 2 first; fix comments and extack.
- Patch 3: state that the check is necessary but not sufficient for
  N-divided outputs.
- Patch 4: report the eSync frequency and pulse from the registers again
  instead of a fixed 1 Hz / 25%; compute the pulse in 64 bits.
- Patch 5: fix the sibling lookup kernel-doc and the commit message.

Changes since v2:
- Added patches 2 and 3.
- Patch 4: simplify zl3073x_out_esync_is_enabled().
- Patch 5: add/remove pins from zldpll->pins around dpll_pin_register()/
  dpll_pin_unregister() so a registered sibling is always found.

Changes since v1:
- Patch 4: pass the actual output carrier to zl3073x_out_esync_enable(),
  drop a redundant "? true : false", fix a comment typo.
- Patch 5: commit message wording.

v4: https://lore.kernel.org/netdev/20261009192556.272263-1-ivecera@redhat.com/
v3: https://lore.kernel.org/netdev/20261006153116.347497-1-ivecera@redhat.com/
v2: https://lore.kernel.org/netdev/20261002074520.30048-1-ivecera@redhat.com/
v1: https://lore.kernel.org/netdev/20261001080648.1424172-1-ivecera@redhat.com/

Ivan Vecera (5):
  dpll: zl3073x: do not offer 0 Hz as a supported pin frequency
  dpll: zl3073x: reject inexact frequencies for N-divided outputs
  dpll: zl3073x: reject output frequencies with too small divisor
  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 | 237 +++++++++++++++++++++++++++---------
 drivers/dpll/zl3073x/out.h  |  36 ++++++
 drivers/dpll/zl3073x/prop.c |  56 +++++++--
 3 files changed, 264 insertions(+), 65 deletions(-)

-- 
2.56.0

[PATCH net v5 1/5] dpll: zl3073x: do not offer 0 Hz as a supported pin frequency

[Ivan Vecera]

zl3073x_pin_props_get() always adds the current pin frequency to the
supported list without checking it. A sub-Hz frequency reads back as
0 Hz, so 0 Hz can then be requested and the output frequency set
callback divides by zero. A 0 Hz entry in the firmware node is not
rejected either: it is offered for an input pin, and for an output pin
zl3073x_pin_check_freq() divides by it at probe time.

Do not add a 0 Hz current frequency and reject 0 Hz in
zl3073x_pin_check_freq(). If the list ends up empty, do not publish it
at all, as the DPLL core rejects an empty non-NULL list.

Found by code inspection, not triggered at runtime.

Fixes: 85a9aaac4a38 ("dpll: zl3073x: Include current frequency in supported frequencies list")
Reviewed-by: Petr Oros <poros@redhat.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/prop.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
index ac9d41d0f978..768e5424de2d 100644
--- a/drivers/dpll/zl3073x/prop.c
+++ b/drivers/dpll/zl3073x/prop.c
@@ -30,7 +30,7 @@ static bool
 zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
 		       u8 id, u64 freq)
 {
-	if (freq > U32_MAX)
+	if (!freq || freq > U32_MAX)
 		goto err_inv_freq;
 
 	if (dir == DPLL_PIN_DIRECTION_INPUT) {
@@ -295,13 +295,20 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
 		goto err_alloc_ranges;
 	}
 
-	/* Start with current frequency at index 0 */
-	ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);
+	/* Start with current frequency at index 0. A sub-Hz frequency is
+	 * read back as 0 Hz and cannot be set, so it is not offered.
+	 */
+	j = 0;
+	if (curr_freq) {
+		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(curr_freq);
+
+		ranges[j++] = freq;
+	}
 
 	/* Add frequencies from firmware node, skipping current frequency
 	 * and filtering out frequencies not representable by device
 	 */
-	for (i = 0, j = 1; i < num_freqs; i++) {
+	for (i = 0; i < num_freqs; i++) {
 		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(freqs[i]);
 
 		if (freqs[i] == curr_freq)
@@ -312,6 +319,14 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
 		}
 	}
 
+	/* The DPLL core rejects a non-NULL list without entries, so do
+	 * not publish the array at all when nothing is settable.
+	 */
+	if (!j) {
+		kfree(ranges);
+		ranges = NULL;
+	}
+
 	/* Save number of freq ranges and pointer to them into pin properties */
 	props->dpll_props.freq_supported = ranges;
 	props->dpll_props.freq_supported_num = j;
-- 
2.56.0

Re: [PATCH net v5 1/5] dpll: zl3073x: do not offer 0 Hz as a supported pin frequency

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch fixes two separate 0 Hz bugs, but its only Fixes: tag
  names 85a9aaac4a38 ("dpll: zl3073x: Include current frequency in…

Pre-existing issues:
- [Medium] zl3073x_pin_props_get() still adds the probe-time current
  frequency at index 0 after only a zero check (prop.c line 302).

--

dpll: zl3073x: do not offer 0 Hz as a supported pin frequency

This patch stops zl3073x_pin_props_get() from adding a 0 Hz current
frequency to the supported list. It also makes zl3073x_pin_check_freq()
reject 0 Hz firmware node entries. If the resulting list is empty, it is
not published.

> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> index ac9d41d0f978e..768e5424de2d2 100644
> --- a/drivers/dpll/zl3073x/prop.c
> +++ b/drivers/dpll/zl3073x/prop.c
> @@ -30,7 +30,7 @@ static bool
>  zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
>  		       u8 id, u64 freq)
>  {
> -	if (freq > U32_MAX)
> +	if (!freq || freq > U32_MAX)
>  		goto err_inv_freq;

[Severity: Low]

The commit message also says that a 0 Hz firmware node entry is divided by
in zl3073x_pin_check_freq() at probe time. Does the Fixes: tag cover that
part of the fix?

Before this patch, zl3073x_pin_check_freq() had:

	if (freq > U32_MAX)
		goto err_inv_freq;
	...
		if (synth_freq % (u32)freq)
			goto err_inv_freq;

Both checks came from a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin
properties from system firmware"), not from 85a9aaac4a38. On input pins,
zl3073x_ref_freq_factorize(0) also succeeds, so 0 Hz was accepted there too.

Would it make sense to add this tag as well?

Fixes: a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin properties from system firmware")

Without it, stable trees that have a99a9f0ebdaa but not 85a9aaac4a38 might
not pick this up.

The later patch in this series, "dpll: zl3073x: reject output frequencies
with too small divisor", carries Fixes: a99a9f0ebdaa. It still divides by
freq via div_u64_rem(), so it seems to depend on the !freq guard added here.

>  
>  	if (dir == DPLL_PIN_DIRECTION_INPUT) {
> @@ -295,13 +295,20 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
>  		goto err_alloc_ranges;
>  	}
>  
> -	/* Start with current frequency at index 0 */
> -	ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);
> +	/* Start with current frequency at index 0. A sub-Hz frequency is
> +	 * read back as 0 Hz and cannot be set, so it is not offered.
> +	 */
> +	j = 0;
> +	if (curr_freq) {
> +		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(curr_freq);
> +
> +		ranges[j++] = freq;
> +	}

[Severity: Medium]

This is a pre-existing issue and was not introduced by this patch. The new
comment says a frequency that cannot be set is not offered, but only 0 Hz
is filtered here. Should curr_freq also go through
zl3073x_pin_check_freq(), as the firmware node entries do?

curr_freq comes from truncating divisions:

  - outputs: synth_freq / out->div, then / esync_n_period for N-pins
  - inputs: mul_u64_u32_div(base * mult, M, N)

So a non-zero curr_freq can still be a value that the set callbacks cannot
program.

For output pins, zl3073x_dpll_output_pin_frequency_set() does:

	new_div = synth_freq / (u32)frequency;

The non-N-div path has no remainder or minimum divisor check. Take a 1 GHz
synth with a probe-time divisor of 300000. curr_freq is 3333, and selecting
it later gives new_div = 300030, not 300000.

If the probe-time divisor was 1, a user can switch away and then pick the
still-advertised curr_freq again. That puts the output back into divider
bypass. The later patch in this series says the hardware ignores the width
and eSync registers in that mode.

For input pins, zl3073x_dpll_input_pin_frequency_set() ignores the result
of the factorization:

	/* Update frequency */
	zl3073x_ref_freq_set(&ref, frequency);

	/* Commit reference state */
	return zl3073x_ref_state_set(zldev, ref_id, &ref);

Suppose curr_freq came from an M/N ratio that has no base * mult
factorization, such as a truncated 161132812 Hz. zl3073x_ref_freq_set()
fails and ref stays unchanged. zl3073x_ref_state_set() then finds nothing
to write and returns 0, so the request reports success while the reference
keeps its previous frequency.

The unconditional curr_freq entry comes from 85a9aaac4a38. The ignored
zl3073x_ref_freq_set() return value is older.

[ ... ]

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

Re: [PATCH net v5 1/5] dpll: zl3073x: do not offer 0 Hz as a supported pin frequency

[Ivan Vecera]

On 10/12/26 08:13, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The patch fixes two separate 0 Hz bugs, but its only Fixes: tag
>    names 85a9aaac4a38 ("dpll: zl3073x: Include current frequency in…
> 
> Pre-existing issues:
> - [Medium] zl3073x_pin_props_get() still adds the probe-time current
>    frequency at index 0 after only a zero check (prop.c line 302).
> 
> --
> 
> dpll: zl3073x: do not offer 0 Hz as a supported pin frequency
> 
> This patch stops zl3073x_pin_props_get() from adding a 0 Hz current
> frequency to the supported list. It also makes zl3073x_pin_check_freq()
> reject 0 Hz firmware node entries. If the resulting list is empty, it is
> not published.
> 
>> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
>> index ac9d41d0f978e..768e5424de2d2 100644
>> --- a/drivers/dpll/zl3073x/prop.c
>> +++ b/drivers/dpll/zl3073x/prop.c
>> @@ -30,7 +30,7 @@ static bool
>>   zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
>>   		       u8 id, u64 freq)
>>   {
>> -	if (freq > U32_MAX)
>> +	if (!freq || freq > U32_MAX)
>>   		goto err_inv_freq;
> 
> [Severity: Low]
> 
> The commit message also says that a 0 Hz firmware node entry is divided by
> in zl3073x_pin_check_freq() at probe time. Does the Fixes: tag cover that
> part of the fix?
> 
> Before this patch, zl3073x_pin_check_freq() had:
> 
> 	if (freq > U32_MAX)
> 		goto err_inv_freq;
> 	...
> 		if (synth_freq % (u32)freq)
> 			goto err_inv_freq;
> 
> Both checks came from a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin
> properties from system firmware"), not from 85a9aaac4a38. On input pins,
> zl3073x_ref_freq_factorize(0) also succeeds, so 0 Hz was accepted there too.
> 
> Would it make sense to add this tag as well?
> 
> Fixes: a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin properties from system firmware")

Maybe, does it make sense to make another respin? Paulo,Jakub?
> Without it, stable trees that have a99a9f0ebdaa but not 85a9aaac4a38 might
> not pick this up.
> 
> The later patch in this series, "dpll: zl3073x: reject output frequencies
> with too small divisor", carries Fixes: a99a9f0ebdaa. It still divides by
> freq via div_u64_rem(), so it seems to depend on the !freq guard added here.
> 
>>   
>>   	if (dir == DPLL_PIN_DIRECTION_INPUT) {
>> @@ -295,13 +295,20 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
>>   		goto err_alloc_ranges;
>>   	}
>>   
>> -	/* Start with current frequency at index 0 */
>> -	ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);
>> +	/* Start with current frequency at index 0. A sub-Hz frequency is
>> +	 * read back as 0 Hz and cannot be set, so it is not offered.
>> +	 */
>> +	j = 0;
>> +	if (curr_freq) {
>> +		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(curr_freq);
>> +
>> +		ranges[j++] = freq;
>> +	}
> 
> [Severity: Medium]
> 
> This is a pre-existing issue and was not introduced by this patch. The new
> comment says a frequency that cannot be set is not offered, but only 0 Hz
> is filtered here. Should curr_freq also go through
> zl3073x_pin_check_freq(), as the firmware node entries do?
No. The current frequency is listed so that the pin always reports
a frequency from its supported list, even when the flash configuration
uses a value the driver itself would not set. Going back to such a
value can then be inexact or fail.

Thanks,
Ivan

[PATCH net v5 2/5] dpll: zl3073x: reject inexact frequencies for N-divided outputs

[Ivan Vecera]

For N-divided outputs the P-pin and N-pin share the output divisor and
the N-pin frequency is synth_freq / (div * esync_n_period). The frequency
set helper computed esync_n_period with a truncating division and only
rejected a zero result, so a frequency that does not divide evenly was
silently rounded. Setting the P-pin could then shift the N-pin frequency
too (e.g. 600 MHz synth, div=60, period=10 gives P=10 MHz, N=1 MHz;
setting P to 2.5 MHz moves N to 1.25 MHz). When setting the P-pin, the
product esync_n_period * div used for the rescale was computed in 32
bits and could wrap, so a wrong N divider was stored for low N-pin
frequencies.

Require esync_n_period >= 2 so the N-pin frequency stays below the P-pin
one, reject a P-pin frequency for which the esync_n_period keeping the
N-pin frequency does not fit into 32 bits, and check the division
remainder and reject the request if it is not exact.

Found by code inspection, not triggered at runtime.

Fixes: ce26d7ca50a5 ("dpll: zl3073x: Add support to get/set frequency on pins")
Reviewed-by: Petr Oros <poros@redhat.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 46 ++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index f2e137475b40..7a617d9bd671 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -987,6 +987,7 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 	const struct zl3073x_synth *synth;
 	u32 new_div, synth_freq;
 	struct zl3073x_out out;
+	u64 n_period, rem;
 	u8 out_id;
 
 	guard(mutex)(&zldpll->lock);
@@ -1014,16 +1015,11 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 	}
 
 	if (zl3073x_dpll_is_p_pin(pin)) {
-		/* We are going to change output frequency for P-pin but
-		 * if the requested frequency is less than current N-pin
-		 * frequency then indicate a failure as we are not able
-		 * to compute N-pin divisor to keep its frequency unchanged.
-		 *
-		 * Update divisor for N-pin to keep N-pin frequency.
+		/* Keep the N-pin frequency: the new P-pin frequency has to be
+		 * a multiple of it, at least twice as high.
 		 */
-		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
-		if (!out.esync_n_period)
-			return -EINVAL;
+		n_period = mul_u32_u32(out.esync_n_period, out.div);
+		n_period = div64_u64_rem(n_period, new_div, &rem);
 
 		/* Update the output divisor */
 		out.div = new_div;
@@ -1031,17 +1027,31 @@ 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 = out.div;
 	} else {
-		/* We are going to change frequency of N-pin but if
-		 * the requested freq is greater or equal than freq of P-pin
-		 * in the output pair we cannot compute divisor for the N-pin.
-		 * In this case indicate a failure.
-		 *
-		 * Update divisor for N-pin
+		/* The N-pin frequency has to divide the P-pin frequency and
+		 * be at most half of it.
 		 */
-		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
-		if (!out.esync_n_period)
-			return -EINVAL;
+		n_period = frequency * out.div;
+		n_period = div64_u64_rem(synth_freq, n_period, &rem);
+	}
+	if (n_period < 2) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "OUT%uN freq must be at most half of OUT%uP freq",
+				   out_id, out_id);
+		return -EINVAL;
+	}
+	if (n_period > U32_MAX) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "OUT%uN freq is too low for OUT%uP freq",
+				   out_id, out_id);
+		return -EINVAL;
+	}
+	if (rem != 0) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "OUT%uN freq must divide OUT%uP freq",
+				   out_id, out_id);
+		return -EINVAL;
 	}
+	out.esync_n_period = n_period;
 
 	/* For 50/50 duty cycle the divisor is equal to width */
 	out.esync_n_width = out.esync_n_period;
-- 
2.56.0

Re: [PATCH net v5 2/5] dpll: zl3073x: reject inexact frequencies for N-divided outputs

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: zl3073x_pin_props_get() always lists the current
  (truncated) frequency as supported.

Pre-existing issues:
- [Low] Concern: userspace never sees the three new extack messages added
  in `zl3073x_dpll_output_pin_frequency_set()` ("must be at most half",…

--

dpll: zl3073x: reject inexact frequencies for N-divided outputs

This commit tightens zl3073x_dpll_output_pin_frequency_set() for
N-divided outputs. It computes esync_n_period in 64 bits and requires it
to be at least 2 and to fit into 32 bits. It also rejects any request
whose division leaves a remainder, and reports each failure case through
extack.

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

[ ... ]

> @@ -1031,17 +1027,31 @@ 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 = out.div;
>  	} else {

[ ... ]

> +		n_period = frequency * out.div;
> +		n_period = div64_u64_rem(synth_freq, n_period, &rem);
> +	}
> +	if (n_period < 2) {
> +		NL_SET_ERR_MSG_FMT(extack,
> +				   "OUT%uN freq must be at most half of OUT%uP freq",
> +				   out_id, out_id);
> +		return -EINVAL;
> +	}

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch, but
will userspace ever see this message, or the two added below it?

When ops->frequency_set() fails, the DPLL core overwrites the driver's
extack text:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
    ...
	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;
	}
    ...
}

NL_SET_ERR_MSG_FMT() always writes into extack->_msg_buf and resets
_msg. The "must be at most half", "is too low" and "must divide" messages
are therefore replaced by the generic text, and they can only be seen
through the netlink_extack tracepoint. The -EINVAL return is still
correct.

> +	if (n_period > U32_MAX) {
> +		NL_SET_ERR_MSG_FMT(extack,
> +				   "OUT%uN freq is too low for OUT%uP freq",
> +				   out_id, out_id);
> +		return -EINVAL;
> +	}
> +	if (rem != 0) {
> +		NL_SET_ERR_MSG_FMT(extack,
> +				   "OUT%uN freq must divide OUT%uP freq",
> +				   out_id, out_id);
> +		return -EINVAL;
>  	}

[Severity: Low]
Can this check reject a frequency that zl3073x_pin_props_get() lists as
supported?

zl3073x_pin_props_get() always puts the current frequency into the
supported list. For an N-pin, that value comes from a truncating
division:

drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
    ...
	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
		freq /= out->esync_n_period;
    ...
}

Also, zl3073x_out_state_fetch() only refuses esync_n_period == 0, so a
boot config with esync_n_period == 1 is accepted.

A request for the current value never reaches the driver, because
dpll_pin_freq_set() returns early when freq == old_freq. If the pin is
first moved to another frequency, though, a later request for the
advertised boot value can fail.

Take a 600 MHz synth, div=60 and a boot esync_n_period of 7. The N-pin
is advertised as 1428571 Hz. After N is moved to 1 MHz, a request for
1428571 gives rem=180 and returns -EINVAL.

In the same way, a P-pin frequency that would bring back
esync_n_period == 1 now fails the n_period < 2 check. According to the
series commit "dpll: zl3073x: reject output frequencies with too small
divisor", that state gave no N-pin output.

This looks consistent with the goal of rejecting inexact N frequencies.
A later comment in prop.c in this series also says the supported list
for N-divided outputs is only a necessary condition. The old code
already rejected some advertised values, such as N-pin frequencies at or
above the P-pin frequency.

Is it intended that a user cannot go back to an inexact boot-time N-pin
setting by asking for its advertised value?

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

Re: [PATCH net v5 2/5] dpll: zl3073x: reject inexact frequencies for N-divided outputs

[Ivan Vecera]

On 10/12/26 08:13, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Concern: zl3073x_pin_props_get() always lists the current
>    (truncated) frequency as supported.
> 
> Pre-existing issues:
> - [Low] Concern: userspace never sees the three new extack messages added
>    in `zl3073x_dpll_output_pin_frequency_set()` ("must be at most half",…
> 
> --
> 
> dpll: zl3073x: reject inexact frequencies for N-divided outputs
> 
> This commit tightens zl3073x_dpll_output_pin_frequency_set() for
> N-divided outputs. It computes esync_n_period in 64 bits and requires it
> to be at least 2 and to fit into 32 bits. It also rejects any request
> whose division leaves a remainder, and reports each failure case through
> extack.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index f2e137475b40f..7a617d9bd6712 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
> 
> [ ... ]
> 
>> @@ -1031,17 +1027,31 @@ 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 = out.div;
>>   	} else {
> 
> [ ... ]
> 
>> +		n_period = frequency * out.div;
>> +		n_period = div64_u64_rem(synth_freq, n_period, &rem);
>> +	}
>> +	if (n_period < 2) {
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "OUT%uN freq must be at most half of OUT%uP freq",
>> +				   out_id, out_id);
>> +		return -EINVAL;
>> +	}
> 
> [Severity: Low]
> This is a pre-existing issue and was not introduced by this patch, but
> will userspace ever see this message, or the two added below it?

No, as I mentioned in v4, dpll_pin_freq_set() overwrites them. That is a DPLL
core issue and out of scope for this series.

> 
> When ops->frequency_set() fails, the DPLL core overwrites the driver's
> extack text:
> 
> drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
>      ...
> 	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;
> 	}
>      ...
> }
> 
> NL_SET_ERR_MSG_FMT() always writes into extack->_msg_buf and resets
> _msg. The "must be at most half", "is too low" and "must divide" messages
> are therefore replaced by the generic text, and they can only be seen
> through the netlink_extack tracepoint. The -EINVAL return is still
> correct.
> 
>> +	if (n_period > U32_MAX) {
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "OUT%uN freq is too low for OUT%uP freq",
>> +				   out_id, out_id);
>> +		return -EINVAL;
>> +	}
>> +	if (rem != 0) {
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "OUT%uN freq must divide OUT%uP freq",
>> +				   out_id, out_id);
>> +		return -EINVAL;
>>   	}
> 
> [Severity: Low]
> Can this check reject a frequency that zl3073x_pin_props_get() lists as
> supported?
> 
> zl3073x_pin_props_get() always puts the current frequency into the
> supported list. For an N-pin, that value comes from a truncating
> division:
> 
> drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
>      ...
> 	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> 		freq /= out->esync_n_period;
>      ...
> }
> 
> Also, zl3073x_out_state_fetch() only refuses esync_n_period == 0, so a
> boot config with esync_n_period == 1 is accepted.
> 
> A request for the current value never reaches the driver, because
> dpll_pin_freq_set() returns early when freq == old_freq. If the pin is
> first moved to another frequency, though, a later request for the
> advertised boot value can fail.
> 
> Take a 600 MHz synth, div=60 and a boot esync_n_period of 7. The N-pin
> is advertised as 1428571 Hz. After N is moved to 1 MHz, a request for
> 1428571 gives rem=180 and returns -EINVAL.
> 
> In the same way, a P-pin frequency that would bring back
> esync_n_period == 1 now fails the n_period < 2 check. According to the
> series commit "dpll: zl3073x: reject output frequencies with too small
> divisor", that state gave no N-pin output.
> 
> This looks consistent with the goal of rejecting inexact N frequencies.
> A later comment in prop.c in this series also says the supported list
> for N-divided outputs is only a necessary condition. The old code
> already rejected some advertised values, such as N-pin frequencies at or
> above the P-pin frequency.
> 
> Is it intended that a user cannot go back to an inexact boot-time N-pin
> setting by asking for its advertised value?

Yes. Such a value can only come from the flash configuration and cannot
be requested exactly in whole Hz. Accepting truncated values would
bring back the silent rounding this patch removes.

Thanks,
Ivan

[PATCH net v5 3/5] dpll: zl3073x: reject output frequencies with too small divisor

[Ivan Vecera]

The output divisor has to be at least 2: a divisor of 1 bypasses the
divider and the hardware then ignores the pulse width and eSync
registers. For the N-pin of an N-divided output the N divider (>= 2)
applies on top, so its effective divisor has to be at least 4.

zl3073x_pin_check_freq() only checked that the frequency divides the
synth frequency. Require also the minimum divisor. For N-divided
outputs this is necessary but not sufficient, the rest is checked by
the frequency set callback.

Hit during development: an N-pin frequency that needs an N divider of 1
was accepted, and with esync_n_period = 1 the N-pin produced no output
on an oscilloscope.

Fixes: a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin properties from system firmware")
Reviewed-by: Petr Oros <poros@redhat.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/prop.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
index 768e5424de2d..4e006ed5950a 100644
--- a/drivers/dpll/zl3073x/prop.c
+++ b/drivers/dpll/zl3073x/prop.c
@@ -22,7 +22,8 @@
  * The function checks the given frequency is valid for the device. For input
  * pins it checks that the frequency can be factorized using supported base
  * frequencies. For output pins it checks that the frequency divides connected
- * synth frequency without remainder.
+ * synth frequency without remainder and that the resulting divisor is not
+ * below the minimum the driver supports.
  *
  * Return: true if the frequency is valid, false if not.
  */
@@ -41,18 +42,38 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
 		if (rc)
 			goto err_inv_freq;
 	} else {
-		u32 synth_freq;
-		u8 out, synth;
+		u32 synth_freq, div, min_div, rem;
+		const struct zl3073x_out *out;
+		u8 out_id, synth;
 
 		/* Get output pin synthesizer */
-		out = zl3073x_output_pin_out_get(id);
-		synth = zl3073x_dev_out_synth_get(zldev, out);
+		out_id = zl3073x_output_pin_out_get(id);
+		synth = zl3073x_dev_out_synth_get(zldev, out_id);
 
 		/* Get synth frequency */
 		synth_freq = zl3073x_dev_synth_freq_get(zldev, synth);
 
 		/* Check the frequency divides synth frequency */
-		if (synth_freq % (u32)freq)
+		div = div_u64_rem(synth_freq, freq, &rem);
+		if (rem)
+			goto err_inv_freq;
+
+		/* The driver requires an output divisor of at least 2 -
+		 * below that the hardware bypasses the divider and ignores
+		 * the pulse width and eSync registers the driver programs.
+		 * For the N-pin of an N-divided output the effective divisor
+		 * also includes the N divider (>= 2), so the minimum is 4.
+		 * For N-divided outputs this is only a necessary condition -
+		 * whether the frequency can be set also depends on the output
+		 * divisor shared by both pins, which is checked when the
+		 * frequency is set.
+		 */
+		out = zl3073x_out_state_get(zldev, out_id);
+		if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
+			min_div = 4;
+		else
+			min_div = 2;
+		if (div < min_div)
 			goto err_inv_freq;
 	}
 
-- 
2.56.0

Re: [PATCH net v5 3/5] dpll: zl3073x: reject output frequencies with too small divisor

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (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] The current output frequency skips the new minimum-divisor
  rule.
- [Medium] The commit message says that for N-divided outputs the min-
  divisor check is 'necessary but not sufficient, the rest is checked by…

Pre-existing issues:
- [Medium] eSync can be enabled on an output whose divisor is 1.

--

dpll: zl3073x: reject output frequencies with too small divisor

This adds a minimum output divisor check to zl3073x_pin_check_freq().
Firmware-provided output frequencies must now give a divisor of at least
2, or at least 4 for the N-pin of an N-divided output. Frequencies that
fail the check are dropped from the supported list.

> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> index 768e5424de2d2..4e006ed5950ad 100644
> --- a/drivers/dpll/zl3073x/prop.c
> +++ b/drivers/dpll/zl3073x/prop.c

[ ... ]

> @@ -41,18 +42,38 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,

[ ... ]

>  		/* Check the frequency divides synth frequency */
> -		if (synth_freq % (u32)freq)
> +		div = div_u64_rem(synth_freq, freq, &rem);
> +		if (rem)
> +			goto err_inv_freq;
> +
> +		/* The driver requires an output divisor of at least 2 -
> +		 * below that the hardware bypasses the divider and ignores
> +		 * the pulse width and eSync registers the driver programs.
> +		 * For the N-pin of an N-divided output the effective divisor
> +		 * also includes the N divider (>= 2), so the minimum is 4.
> +		 * For N-divided outputs this is only a necessary condition -
> +		 * whether the frequency can be set also depends on the output
> +		 * divisor shared by both pins, which is checked when the
> +		 * frequency is set.
> +		 */
> +		out = zl3073x_out_state_get(zldev, out_id);
> +		if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> +			min_div = 4;
> +		else
> +			min_div = 2;
> +		if (div < min_div)
>  			goto err_inv_freq;
>  	}

[Severity: Medium]
Can the current output frequency get past this new minimum divisor check?

zl3073x_pin_props_get() puts curr_freq at index 0 of freq_supported
without calling zl3073x_pin_check_freq(). It also skips any firmware
entry equal to curr_freq, so that value is never validated either:

drivers/dpll/zl3073x/prop.c:zl3073x_pin_props_get() {
    ...
	j = 0;
	if (curr_freq) {
		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(curr_freq);

		ranges[j++] = freq;
	}
    ...
	for (i = 0; i < num_freqs; i++) {
		...
		if (freqs[i] == curr_freq)
			continue;
		if (zl3073x_pin_check_freq(zldev, dir, index, freqs[i])) {
    ...
}

zl3073x_out_state_fetch() only rejects a zero divisor. An output can
therefore come up from the flash configuration with div == 1, and then
curr_freq == synth_freq stays in the supported list.

dpll_pin_freq_set() only checks that the frequency is in
freq_supported. zl3073x_dpll_output_pin_frequency_set() puts no lower
bound on new_div:

	new_div = synth_freq / (u32)frequency;

	/* Check signal format */
	if (!zl3073x_out_is_ndiv(&out)) {
		...
		out.div = new_div;

		/* For 50/50 duty cycle the divisor is equal to width */
		out.width = new_div;

The N-div P-pin branch also does out.div = new_div after checking only
n_period.

A user can move the output away from its boot frequency and then set it
back. The driver then programs divisor 1 again. The commit message says
the hardware then ignores the pulse width and eSync registers. In N-div
mode those registers also hold the N-pin's N divider.

At the end of the series, a frequency_set with eSync enabled calls
zl3073x_out_esync_enable() with div == 1, which writes
esync_n_width = 0.

The previous patch in the series ("dpll: zl3073x: do not offer 0 Hz as
a supported pin frequency") already filters curr_freq for 0 Hz. Should
curr_freq also go through the divisor check? Alternatively, should
zl3073x_dpll_output_pin_frequency_set() reject new_div < 2? Later
patches in the series don't seem to add either check.

[Severity: Medium]
The commit message says:

  For N-divided outputs this is necessary but not sufficient, the rest
  is checked by the frequency set callback.

Does the frequency set callback actually check the out.div >= 2 part?
The N-pin branch of zl3073x_dpll_output_pin_frequency_set() uses
whatever out.div is currently programmed:

	} else {
		/* The N-pin frequency has to divide the P-pin frequency and
		 * be at most half of it.
		 */
		n_period = frequency * out.div;
		n_period = div64_u64_rem(synth_freq, n_period, &rem);
	}

After that it only rejects n_period < 2, n_period > U32_MAX and a
non-zero remainder before committing out.esync_n_period.

The check added here only looks at the total divisor
synth_freq / freq >= 4. That can't tell out.div = 2, N = 2 apart from
out.div = 1, N = 4.

An N-div output can end up with out.div == 1 in two ways:

  - the boot configuration, since zl3073x_out_state_fetch() only
    rejects 0
  - restoring a P-pin frequency equal to synth_freq through the
    unvalidated curr_freq entry

In that case the N-pin set succeeds, and
zl3073x_dev_output_pin_freq_get() reports synth_freq / N.

The hardware ignores the N divider register at divisor 1. Wouldn't the
reported N-pin frequency then differ from what the pin actually
produces? This looks like a variant of the "no output on an
oscilloscope" case in the commit message. It still seems to be present
at the end of the series.

[Severity: Medium]
This isn't a bug introduced by this patch, but the eSync callbacks seem
to reach the same divisor 1 case.

zl3073x_dpll_output_pin_esync_get() advertises eSync ranges whenever
the output is not N-div, esync_control is set and out_freq > 1:

	out_freq = synth_freq / out->div;

	if (!pin->esync_control || out_freq <= 1)
		return -EOPNOTSUPP;

With out->div == 1, out_freq == synth_freq, so this check passes.
zl3073x_dpll_output_pin_esync_set() then sets the clock type to ESYNC
and commits without checking out.div >= 2:

	out.esync_n_period = synth_freq / (u32)freq / out.div;
	...
	out.esync_n_width = out.div / 2;

For div == 1 this writes esync_n_width = 0.

This commit says the hardware ignores the eSync registers at divisor 1.
Would the driver then report eSync as enabled with a given frequency
and pulse, while the device outputs a plain clock?

At the end of the series, zl3073x_out_esync_enable() uses the same
div / 2 width computation. frequency_set re-enables eSync for any
carrier other than 1 Hz, including when div == 1.

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

Re: [PATCH net v5 3/5] dpll: zl3073x: reject output frequencies with too small divisor

[Ivan Vecera]

On 10/12/26 08:13, 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: 2 (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] The current output frequency skips the new minimum-divisor
>    rule.
> - [Medium] The commit message says that for N-divided outputs the min-
>    divisor check is 'necessary but not sufficient, the rest is checked by…
> 
> Pre-existing issues:
> - [Medium] eSync can be enabled on an output whose divisor is 1.
> 
> --
> 
> dpll: zl3073x: reject output frequencies with too small divisor
> 
> This adds a minimum output divisor check to zl3073x_pin_check_freq().
> Firmware-provided output frequencies must now give a divisor of at least
> 2, or at least 4 for the N-pin of an N-divided output. Frequencies that
> fail the check are dropped from the supported list.
> 
>> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
>> index 768e5424de2d2..4e006ed5950ad 100644
>> --- a/drivers/dpll/zl3073x/prop.c
>> +++ b/drivers/dpll/zl3073x/prop.c
> 
> [ ... ]
> 
>> @@ -41,18 +42,38 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
> 
> [ ... ]
> 
>>   		/* Check the frequency divides synth frequency */
>> -		if (synth_freq % (u32)freq)
>> +		div = div_u64_rem(synth_freq, freq, &rem);
>> +		if (rem)
>> +			goto err_inv_freq;
>> +
>> +		/* The driver requires an output divisor of at least 2 -
>> +		 * below that the hardware bypasses the divider and ignores
>> +		 * the pulse width and eSync registers the driver programs.
>> +		 * For the N-pin of an N-divided output the effective divisor
>> +		 * also includes the N divider (>= 2), so the minimum is 4.
>> +		 * For N-divided outputs this is only a necessary condition -
>> +		 * whether the frequency can be set also depends on the output
>> +		 * divisor shared by both pins, which is checked when the
>> +		 * frequency is set.
>> +		 */
>> +		out = zl3073x_out_state_get(zldev, out_id);
>> +		if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
>> +			min_div = 4;
>> +		else
>> +			min_div = 2;
>> +		if (div < min_div)
>>   			goto err_inv_freq;
>>   	}
> 
> [Severity: Medium]
> Can the current output frequency get past this new minimum divisor check?
> 
> zl3073x_pin_props_get() puts curr_freq at index 0 of freq_supported
> without calling zl3073x_pin_check_freq(). It also skips any firmware
> entry equal to curr_freq, so that value is never validated either:
> 
> drivers/dpll/zl3073x/prop.c:zl3073x_pin_props_get() {
>      ...
> 	j = 0;
> 	if (curr_freq) {
> 		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(curr_freq);
> 
> 		ranges[j++] = freq;
> 	}
>      ...
> 	for (i = 0; i < num_freqs; i++) {
> 		...
> 		if (freqs[i] == curr_freq)
> 			continue;
> 		if (zl3073x_pin_check_freq(zldev, dir, index, freqs[i])) {
>      ...
> }
> 
> zl3073x_out_state_fetch() only rejects a zero divisor. An output can
> therefore come up from the flash configuration with div == 1, and then
> curr_freq == synth_freq stays in the supported list.
> 
> dpll_pin_freq_set() only checks that the frequency is in
> freq_supported. zl3073x_dpll_output_pin_frequency_set() puts no lower
> bound on new_div:
> 
> 	new_div = synth_freq / (u32)frequency;
> 
> 	/* Check signal format */
> 	if (!zl3073x_out_is_ndiv(&out)) {
> 		...
> 		out.div = new_div;
> 
> 		/* For 50/50 duty cycle the divisor is equal to width */
> 		out.width = new_div;
> 
> The N-div P-pin branch also does out.div = new_div after checking only
> n_period.
> 
> A user can move the output away from its boot frequency and then set it
> back. The driver then programs divisor 1 again. The commit message says
> the hardware then ignores the pulse width and eSync registers. In N-div
> mode those registers also hold the N-pin's N divider.
> 
> At the end of the series, a frequency_set with eSync enabled calls
> zl3073x_out_esync_enable() with div == 1, which writes
> esync_n_width = 0.
> 
> The previous patch in the series ("dpll: zl3073x: do not offer 0 Hz as
> a supported pin frequency") already filters curr_freq for 0 Hz. Should
> curr_freq also go through the divisor check? Alternatively, should
> zl3073x_dpll_output_pin_frequency_set() reject new_div < 2? Later
> patches in the series don't seem to add either check.

No. A divisor of 1 can only come from the boot configuration and is
rarely used in practice: with the synthesizer running at up to 750 MHz
it is only needed for output frequencies above 375 MHz, which only
differential outputs support. Setting the current frequency back just
restores that boot state, which the hardware supports, and the
hardware ignores the pulse width in that mode anyway.

> [Severity: Medium]
> The commit message says:
> 
>    For N-divided outputs this is necessary but not sufficient, the rest
>    is checked by the frequency set callback.
> 
> Does the frequency set callback actually check the out.div >= 2 part?
> The N-pin branch of zl3073x_dpll_output_pin_frequency_set() uses
> whatever out.div is currently programmed:
> 
> 	} else {
> 		/* The N-pin frequency has to divide the P-pin frequency and
> 		 * be at most half of it.
> 		 */
> 		n_period = frequency * out.div;
> 		n_period = div64_u64_rem(synth_freq, n_period, &rem);
> 	}
> 
> After that it only rejects n_period < 2, n_period > U32_MAX and a
> non-zero remainder before committing out.esync_n_period.
> 
> The check added here only looks at the total divisor
> synth_freq / freq >= 4. That can't tell out.div = 2, N = 2 apart from
> out.div = 1, N = 4.
> 
> An N-div output can end up with out.div == 1 in two ways:
> 
>    - the boot configuration, since zl3073x_out_state_fetch() only
>      rejects 0
>    - restoring a P-pin frequency equal to synth_freq through the
>      unvalidated curr_freq entry
> 
> In that case the N-pin set succeeds, and
> zl3073x_dev_output_pin_freq_get() reports synth_freq / N.
> 
> The hardware ignores the N divider register at divisor 1. Wouldn't the
> reported N-pin frequency then differ from what the pin actually
> produces? This looks like a variant of the "no output on an
> oscilloscope" case in the commit message. It still seems to be present
> at the end of the series.

The datasheet requires an output divisor of 2 or more in N-pin divide
mode, so out.div == 1 there is an invalid boot configuration. N-pin
divide modes are also CMOS only, and CMOS outputs are limited to
250 MHz, so a divisor of 1 is never needed there. The driver does not
try to fix it.

> [Severity: Medium]
> This isn't a bug introduced by this patch, but the eSync callbacks seem
> to reach the same divisor 1 case.
> 
> zl3073x_dpll_output_pin_esync_get() advertises eSync ranges whenever
> the output is not N-div, esync_control is set and out_freq > 1:
> 
> 	out_freq = synth_freq / out->div;
> 
> 	if (!pin->esync_control || out_freq <= 1)
> 		return -EOPNOTSUPP;
> 
> With out->div == 1, out_freq == synth_freq, so this check passes.
> zl3073x_dpll_output_pin_esync_set() then sets the clock type to ESYNC
> and commits without checking out.div >= 2:
> 
> 	out.esync_n_period = synth_freq / (u32)freq / out.div;
> 	...
> 	out.esync_n_width = out.div / 2;
> 
> For div == 1 this writes esync_n_width = 0.
> 
> This commit says the hardware ignores the eSync registers at divisor 1.
> Would the driver then report eSync as enabled with a given frequency
> and pulse, while the device outputs a plain clock?

Yes, but only for an output that the flash configuration left in
divider bypass. Supporting eSync in that mode is out of scope for this
series.

Thanks,
Ivan

[PATCH net v5 4/5] dpll: zl3073x: make embedded sync follow the output carrier frequency

[Ivan Vecera]

The eSync period and width are relative to the output carrier, but
changing the output frequency did not update them, so eSync ran at
a wrong frequency and duty cycle. For a 1 Hz carrier esync_get()
returns -EOPNOTSUPP, so eSync left enabled could not be disabled.

Factor out zl3073x_out_esync_{is_enabled,enable,disable}() helpers
and use them in frequency_set() to recompute the eSync period and
width for the new carrier, or to disable eSync for a 1 Hz carrier.

Also compute the reported pulse width in 64 bits, as
50 * esync_n_width overflows u32 for low carrier frequencies.

Hit during development, confirmed on an oscilloscope.

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

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 7a617d9bd671..6d9a6d21d30b 100644
--- a/drivers/dpll/zl3073x/dpll.c
+++ b/drivers/dpll/zl3073x/dpll.c
@@ -878,7 +878,7 @@ 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) {
+	if (!zl3073x_out_esync_is_enabled(out)) {
 		/* No need to read esync data if it is not enabled */
 		esync->freq = 0;
 		esync->pulse = 0;
@@ -894,7 +894,7 @@ zl3073x_dpll_output_pin_esync_get(const struct dpll_pin *dpll_pin,
 	 * 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;
+	esync->pulse = div_u64(mul_u32_u32(50, out->esync_n_width), out->div);
 
 	return 0;
 }
@@ -926,32 +926,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);
@@ -1010,6 +995,23 @@ 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) {
+				/* eSync cannot work on a 1 Hz carrier */
+				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..d2b8b4eb5a76 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;
+}
+
+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.56.0

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

[Ivan Vecera]

The P-pin and N-pin of an output share the divisor, clock type, eSync
and phase compensation registers (outside N-divided mode). Changing
them through one pin changes the other pin too, but only the requested
pin gets a change notification.

Add zl3073x_dpll_output_pin_sibling_get() and notify the sibling from
frequency_set(), esync_set() and phase_adjust_set(). The notification
re-enters the pin get callbacks, so it has to be sent after
zldpll->lock is released and the callbacks switch from guard(mutex)
to explicit unlocking.

To always find a registered sibling, add a pin to zldpll->pins before
dpll_pin_register() and remove it only after dpll_pin_unregister(),
both under zldpll->lock. A pin that is on the list but not registered
gets no netlink notification, as dpll_pin_event_send() skips such
pins, and the only in-tree DPLL notifier ignores DPLL_PIN_CHANGED.

Found by code inspection, not triggered at runtime.

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")
Reviewed-by: Petr Oros <poros@redhat.com>
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 drivers/dpll/zl3073x/dpll.c | 155 +++++++++++++++++++++++++++++++-----
 1 file changed, 135 insertions(+), 20 deletions(-)

diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
index 6d9a6d21d30b..1cdadb8e46fb 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,42 @@ 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 in the pin list
+ * of this DPLL. A pin is on the list from just before its registration
+ * until just after its unregistration, so a registered sibling is always
+ * found, but the returned pin may also be one that is not (yet or any
+ * longer) registered.
+ *
+ * Return: pointer to sibling pin, or NULL if it is not on the pin list
+ */
+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;
@@ -909,12 +942,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);
@@ -923,12 +958,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 */
@@ -937,9 +974,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
@@ -969,13 +1021,15 @@ 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;
 	u64 n_period, rem;
 	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);
@@ -988,7 +1042,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;
 
@@ -1013,7 +1068,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)) {
@@ -1039,19 +1103,22 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 		NL_SET_ERR_MSG_FMT(extack,
 				   "OUT%uN freq must be at most half of OUT%uP freq",
 				   out_id, out_id);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto unlock;
 	}
 	if (n_period > U32_MAX) {
 		NL_SET_ERR_MSG_FMT(extack,
 				   "OUT%uN freq is too low for OUT%uP freq",
 				   out_id, out_id);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto unlock;
 	}
 	if (rem != 0) {
 		NL_SET_ERR_MSG_FMT(extack,
 				   "OUT%uN freq must divide OUT%uP freq",
 				   out_id, out_id);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto unlock;
 	}
 	out.esync_n_period = n_period;
 
@@ -1059,7 +1126,14 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
 	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
@@ -1098,10 +1172,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);
@@ -1110,7 +1186,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
@@ -1734,6 +1824,13 @@ zl3073x_dpll_pin_register(struct zl3073x_dpll_pin *pin, u32 index)
 	else
 		ops = &zl3073x_dpll_output_pin_ops;
 
+	/* Add the pin to the list before registering it with the DPLL core so
+	 * that it is findable as a sibling as soon as the core publishes it.
+	 */
+	mutex_lock(&zldpll->lock);
+	list_add(&pin->list, &zldpll->pins);
+	mutex_unlock(&zldpll->lock);
+
 	/* Register the pin */
 	rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
 	if (rc)
@@ -1745,6 +1842,9 @@ zl3073x_dpll_pin_register(struct zl3073x_dpll_pin *pin, u32 index)
 	return 0;
 
 err_register:
+	mutex_lock(&zldpll->lock);
+	list_del(&pin->list);
+	mutex_unlock(&zldpll->lock);
 	dpll_pin_put(pin->dpll_pin, &pin->tracker);
 err_pin_get:
 	pin->dpll_pin = NULL;
@@ -1779,6 +1879,13 @@ zl3073x_dpll_pin_unregister(struct zl3073x_dpll_pin *pin)
 	/* Unregister the pin */
 	dpll_pin_unregister(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
 
+	/* Remove the pin from the list only after it has been unregistered so
+	 * that a still-registered pin is always findable as a sibling.
+	 */
+	mutex_lock(&zldpll->lock);
+	list_del(&pin->list);
+	mutex_unlock(&zldpll->lock);
+
 	dpll_pin_put(pin->dpll_pin, &pin->tracker);
 	pin->dpll_pin = NULL;
 
@@ -1798,9 +1905,11 @@ zl3073x_dpll_pins_unregister(struct zl3073x_dpll *zldpll)
 {
 	struct zl3073x_dpll_pin *pin, *next;
 
+	/* Unregister each pin before removing it from the list so that a
+	 * still-registered pin is always findable as a sibling.
+	 */
 	list_for_each_entry_safe(pin, next, &zldpll->pins, list) {
 		zl3073x_dpll_pin_unregister(pin);
-		list_del(&pin->list);
 		zl3073x_dpll_pin_free(pin);
 	}
 }
@@ -1913,16 +2022,24 @@ zl3073x_dpll_nco_pin_register(struct zl3073x_dpll *zldpll)
 		goto err_pin_get;
 	}
 
+	/* Add the pin to the list before registering it with the DPLL core so
+	 * that a registered pin is always on the list.
+	 */
+	mutex_lock(&zldpll->lock);
+	list_add(&pin->list, &zldpll->pins);
+	mutex_unlock(&zldpll->lock);
+
 	rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin,
 			       &zl3073x_dpll_nco_pin_ops, pin);
 	if (rc)
 		goto err_register;
 
-	list_add(&pin->list, &zldpll->pins);
-
 	return 0;
 
 err_register:
+	mutex_lock(&zldpll->lock);
+	list_del(&pin->list);
+	mutex_unlock(&zldpll->lock);
 	dpll_pin_put(pin->dpll_pin, &pin->tracker);
 err_pin_get:
 	pin->dpll_pin = NULL;
@@ -1974,8 +2091,6 @@ zl3073x_dpll_pins_register(struct zl3073x_dpll *zldpll)
 			zl3073x_dpll_pin_free(pin);
 			goto error;
 		}
-
-		list_add(&pin->list, &zldpll->pins);
 	}
 
 	/* Register NCO virtual input pin */
-- 
2.56.0

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch passes pins that may not be registered to
  __dpll_pin_change_ntf(), whose kernel-doc requires a "registered pin…

--

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

This commit adds zl3073x_dpll_output_pin_sibling_get(). The output
frequency, esync and phase adjust set callbacks now use it to send a
change notification for the other pin of the P/N pair. So that the
sibling can always be found, pins are added to zldpll->pins before
dpll_pin_register() and removed only after dpll_pin_unregister().

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 6d9a6d21d30b7..1cdadb8e46fb2 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -132,11 +134,42 @@ 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 in the pin list
> + * of this DPLL. A pin is on the list from just before its registration
> + * until just after its unregistration, so a registered sibling is always
> + * found, but the returned pin may also be one that is not (yet or any
> + * longer) registered.

[ ... ]

> @@ -937,9 +974,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);

[Severity: Low]
Is it ok to pass a sibling that may not be registered to
__dpll_pin_change_ntf()? The kernel-doc for that function asks for a
registered pin:

drivers/dpll/dpll_netlink.c:
/**
 * __dpll_pin_change_ntf - notify that the pin has been changed
 * @pin: registered pin pointer
 ...
int __dpll_pin_change_ntf(struct dpll_pin *pin)
{
	lockdep_assert_held(&dpll_lock);
	dpll_pin_notify(pin, pin->clock_id, DPLL_PIN_CHANGED);
	return dpll_pin_event_send(DPLL_CMD_PIN_CHANGE_NTF, pin);
}

dpll_pin_event_send() checks dpll_pin_available(), so no netlink message
is sent. But dpll_pin_notify() runs before that check, so the in-kernel
notifier chain still gets DPLL_PIN_CHANGED for the unregistered pin.

This window looks reachable during probe. zl3073x_dpll_register()
registers the dpll device before its pins, and pins are registered in
index order. That means P is already registered when N is list_add()ed
in zl3073x_dpll_pin_register():

    mutex_lock(&zldpll->lock);
    list_add(&pin->list, &zldpll->pins);
    mutex_unlock(&zldpll->lock);

    /* Register the pin */
    rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin, ops, pin);

Suppose a DPLL_CMD_PIN_SET on P holds dpll_lock from dpll_pin_pre_doit().
N's dpll_pin_register() then waits on dpll_lock. Meanwhile P's callback
finds N on the list and sends DPLL_PIN_CHANGED for N before N's
DPLL_PIN_CREATED.

Teardown is the mirror case. list_add() inserts at the head, so N is
unregistered before P. N also stays on the list until after
dpll_pin_unregister() returns in zl3073x_dpll_pin_unregister(). A
PIN_SET on P during that window would send CHANGED for N after its
DPLL_PIN_DELETED.

zl3073x_dpll_output_pin_frequency_set() and
zl3073x_dpll_output_pin_phase_adjust_set() have the same issue.

zl3073x_dpll_nco_pin_register() now also adds the NCO pin to the list
before dpll_pin_register(). Because of that, the existing callers that
notify the result of zl3073x_dpll_nco_pin_get() (the input pin
state_on_dpll_set and mode_set paths) hit the same window.

As the commit message notes, nothing breaks in-tree today, because
ice_dpll_pin_notify() ignores every action except DPLL_PIN_CREATED and
DPLL_PIN_DELETED.

Would it be better to skip the notification when the sibling is not
registered? Alternatively, the __dpll_pin_change_ntf() kernel-doc and
dpll_pin_notify() could be updated to accept unregistered pins
explicitly.

> +
> +	return rc;
>  }

[ ... ]

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

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

[Ivan Vecera]

On 10/12/26 08:13, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The patch passes pins that may not be registered to
>    __dpll_pin_change_ntf(), whose kernel-doc requires a "registered pin…
> 
> --
> 
> dpll: zl3073x: notify sibling pin when shared output config changes
> 
> This commit adds zl3073x_dpll_output_pin_sibling_get(). The output
> frequency, esync and phase adjust set callbacks now use it to send a
> change notification for the other pin of the P/N pair. So that the
> sibling can always be found, pins are added to zldpll->pins before
> dpll_pin_register() and removed only after dpll_pin_unregister().
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 6d9a6d21d30b7..1cdadb8e46fb2 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
> 
> [ ... ]
> 
>> @@ -132,11 +134,42 @@ 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 in the pin list
>> + * of this DPLL. A pin is on the list from just before its registration
>> + * until just after its unregistration, so a registered sibling is always
>> + * found, but the returned pin may also be one that is not (yet or any
>> + * longer) registered.
> 
> [ ... ]
> 
>> @@ -937,9 +974,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);
> 
> [Severity: Low]
> Is it ok to pass a sibling that may not be registered to
> __dpll_pin_change_ntf()? The kernel-doc for that function asks for a
> registered pin:
> 
> drivers/dpll/dpll_netlink.c:
> /**
>   * __dpll_pin_change_ntf - notify that the pin has been changed
>   * @pin: registered pin pointer
>   ...
> int __dpll_pin_change_ntf(struct dpll_pin *pin)
> {
> 	lockdep_assert_held(&dpll_lock);
> 	dpll_pin_notify(pin, pin->clock_id, DPLL_PIN_CHANGED);
> 	return dpll_pin_event_send(DPLL_CMD_PIN_CHANGE_NTF, pin);
> }
> 
> dpll_pin_event_send() checks dpll_pin_available(), so no netlink message
> is sent. But dpll_pin_notify() runs before that check, so the in-kernel
> notifier chain still gets DPLL_PIN_CHANGED for the unregistered pin.
> 
> This window looks reachable during probe. zl3073x_dpll_register()
> registers the dpll device before its pins, and pins are registered in
> index order. That means P is already registered when N is list_add()ed
> in zl3073x_dpll_pin_register():
> 
>      mutex_lock(&zldpll->lock);
>      list_add(&pin->list, &zldpll->pins);
>      mutex_unlock(&zldpll->lock);
> 
>      /* Register the pin */
>      rc = dpll_pin_register(zldpll->dpll_dev, pin->dpll_pin, ops, pin);
> 
> Suppose a DPLL_CMD_PIN_SET on P holds dpll_lock from dpll_pin_pre_doit().
> N's dpll_pin_register() then waits on dpll_lock. Meanwhile P's callback
> finds N on the list and sends DPLL_PIN_CHANGED for N before N's
> DPLL_PIN_CREATED.
> 
> Teardown is the mirror case. list_add() inserts at the head, so N is
> unregistered before P. N also stays on the list until after
> dpll_pin_unregister() returns in zl3073x_dpll_pin_unregister(). A
> PIN_SET on P during that window would send CHANGED for N after its
> DPLL_PIN_DELETED.
> 
> zl3073x_dpll_output_pin_frequency_set() and
> zl3073x_dpll_output_pin_phase_adjust_set() have the same issue.
> 
> zl3073x_dpll_nco_pin_register() now also adds the NCO pin to the list
> before dpll_pin_register(). Because of that, the existing callers that
> notify the result of zl3073x_dpll_nco_pin_get() (the input pin
> state_on_dpll_set and mode_set paths) hit the same window.
> 
> As the commit message notes, nothing breaks in-tree today, because
> ice_dpll_pin_notify() ignores every action except DPLL_PIN_CREATED and
> DPLL_PIN_DELETED.
> 
> Would it be better to skip the notification when the sibling is not
> registered? Alternatively, the __dpll_pin_change_ntf() kernel-doc and
> dpll_pin_notify() could be updated to accept unregistered pins
> explicitly.
Such a pin gets no netlink notification and the only in-tree notifier
ignores DPLL_PIN_CHANGED, so I would rather not add a registration
check in a net fix.

Thanks,
Ivan