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

[Petr Oros <poros@redhat.com>]

On 10/6/26 5:31 PM, Ivan Vecera wrote:
> 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 the list membership
> has to match the DPLL registration state. dpll_pin_register() publishes
> a pin (and P is registered before N) before it used to be added to the
> list, and teardown detached the whole list before unregistering any
> pin, so a sibling that is registered - and thus reachable by a PIN_SET
> on the other pin - could be missing from the list and never notified.
> Add the pin to the list before dpll_pin_register() and remove it only
> after dpll_pin_unregister(), both under zldpll->lock, and assert the
> lock in the lookup helper. A pin that is transiently on the list while
> not registered is harmless: __dpll_pin_change_ntf() is a no-op for a
> pin that is not available, and the sibling cannot be freed under the
> lookup because the freeing goes through dpll_pin_unregister(), which
> takes dpll_lock that the notification path holds.
> 
> 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 | 187 ++++++++++++++++++++++++++++--------
>   1 file changed, 146 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 65107b4cc4f8..9c678acc3e77 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;
>   
> @@ -1002,7 +1054,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)) {
> @@ -1017,18 +1078,10 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   		u32 rem;
>   
>   		out.esync_n_period = div_u64_rem(prod, new_div, &rem);
> -		if (rem != 0) {
> -			NL_SET_ERR_MSG_FMT(extack,
> -					   "OUT%uN freq must divide OUT%uP freq",
> -					   out_id, out_id);
> -			return -EINVAL;
> -		}
> -		if (out.esync_n_period < 2) {
> -			NL_SET_ERR_MSG_FMT(extack,
> -					   "OUT%uN freq must be less than OUT%uP freq",
> -					   out_id, out_id);
> -			return -EINVAL;
> -		}
> +		if (rem != 0)
> +			goto err_n_nondiv;
> +		if (out.esync_n_period < 2)
> +			goto err_n_toohigh;
>   
>   		/* Update the output divisor */
>   		out.div = new_div;
> @@ -1046,25 +1099,36 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>   		u64 rem, prod = frequency * out.div;
>   
>   		out.esync_n_period = div64_u64_rem(synth_freq, prod, &rem);
> -		if (rem != 0) {
> -			NL_SET_ERR_MSG_FMT(extack,
> -					   "OUT%uN freq must divide OUT%uP freq",
> -					   out_id, out_id);
> -			return -EINVAL;
> -		}
> -		if (out.esync_n_period < 2) {
> -			NL_SET_ERR_MSG_FMT(extack,
> -					   "OUT%uN freq must be less than OUT%uP freq",
> -					   out_id, out_id);
> -			return -EINVAL;
> -		}
> +		if (rem != 0)
> +			goto err_n_nondiv;
> +		if (out.esync_n_period < 2)
> +			goto err_n_toohigh;
>   	}
>   
>   	/* 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;
> +err_n_nondiv:
> +	NL_SET_ERR_MSG_FMT(extack,
> +			   "OUT%uN freq must divide OUT%uP freq",
> +			   out_id, out_id);
> +	rc = -EINVAL;
> +	goto unlock;
> +err_n_toohigh:
> +	NL_SET_ERR_MSG_FMT(extack,
> +			   "OUT%uN freq must be less than OUT%uP freq",
> +			   out_id, out_id);
> +	rc = -EINVAL;
> +	goto unlock;
>   }
>   
>   static int
> @@ -1103,10 +1167,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);
> @@ -1115,7 +1181,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
> @@ -1739,6 +1819,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)
> @@ -1750,6 +1837,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;
> @@ -1784,6 +1874,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;
>   
> @@ -1803,9 +1900,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);
>   	}
>   }
> @@ -1918,16 +2017,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 the list reflects the DPLL registration state.
> +	 */
> +	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;
> @@ -1979,8 +2086,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 */

Reviewed-by: Petr Oros <poros@redhat.com>