Re: [PATCH net-next v8 10/15] dpll: sit9531x: add support to adjust output phase

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Positive phase adjustments bypass modulo folding and fail to fold back into a single output period.
- [Medium] Phase adjust readback returns the cached requested value rather than computing the actual hardware register state.
--

commit 5bf72d790f8a4746d1529ab82b4bac47ba928371
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to adjust output phase

Shift an output in time against the others driven by the same PLL.  The
device has a coarse delay counted in VCO cycles and a three-bit fine field
in fixed thirty-picosecond steps, so a requested offset is split between
the two and what the core reads back is what the registers hold rather
than what was asked for.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 0f886cfd14013..be033d7cfe7c3 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -1342,6 +1342,149 @@ int sit9531x_output_freq_get(struct sit9531x_dev *sitdev, u8 out_idx,
[ ... ]
> +	/*
> +	 * Convert to unsigned absolute delay.  Negative phase (advance)
> +	 * is rendered as T_out - |phase|, modulo the output period.
> +	 */
> +	if (phase_ps == 0) {
> +		abs_ps = 0;
> +	} else if (phase_ps > 0) {
> +		abs_ps = (u64)phase_ps;

[Severity: Medium]
Will this assignment cause positive phase adjustments to bypass modulo folding
and fail to fold back into a single output period?

The commit message states that "an offset larger than one output period is
folded back into a single period". However, it seems a requested large positive
phase adjust directly sets abs_ps without applying the modulo by the output
period.

Could this result in asymmetrical and potentially very large hardware delays
(up to 34 bits of VCO cycles, pausing the output), whereas a negative phase
adjust is properly modulo-folded?

> +	} else {
> +		u64 t_out_ps = div64_u64(1000000000000ULL, freq);
> +		u64 advance = (u64)(-(s64)phase_ps);
[ ... ]
> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 6365a83e4c52b..c07fb22565107 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -736,12 +736,63 @@ sit9531x_dpll_output_pin_state_on_dpll_set(const struct dpll_pin *pin,
[ ... ]
> +static int
> +sit9531x_dpll_output_pin_phase_adjust_get(const struct dpll_pin *pin,
> +					  void *pin_priv,
> +					  const struct dpll_device *dpll,
> +					  void *dpll_priv, s32 *phase_adjust,
> +					  struct netlink_ext_ack *extack)
> +{
> +	struct sit9531x_dpll_pin *dpin = pin_priv;
> +
> +	*phase_adjust = dpin->phase_adjust;

[Severity: Medium]
Does this readback intentionally return the cached requested value rather than
computing the actual hardware register state?

The commit message documents that "what the core reads back is what the
registers hold rather than what was asked for." However, userspace reading this
will receive exactly what was requested, hiding any quantization errors or
register limits.

> +	return 0;
> +}
> +
> +/*
> + * sit9531x_dpll_output_pin_phase_adjust_set - set output phase adjustment
[ ... ]
> +static int
> +sit9531x_dpll_output_pin_phase_adjust_set(const struct dpll_pin *pin,
> +					  void *pin_priv,
> +					  const struct dpll_device *dpll,
> +					  void *dpll_priv, s32 phase_adjust,
> +					  struct netlink_ext_ack *extack)
> +{
> +	struct sit9531x_dpll_pin *dpin = pin_priv;
> +	struct sit9531x_dpll *sitdpll = dpll_priv;
> +	struct sit9531x_dev *sitdev = sitdpll->dev;
> +	int rc;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_output_phase_adjust_set(sitdev, dpin->id, phase_adjust);
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	if (rc) {
> +		NL_SET_ERR_MSG(extack, "Phase adjust failed");
> +		return rc;
> +	}
> +
> +	dpin->phase_adjust = phase_adjust;

[Severity: Medium]
Should this cache the requested value unconditionally, or should it determine
and store the actual hardware-configured phase achieved by the math?

This reinforces the issue above where readback returns the requested value
rather than the true quantized phase offset.

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902214030.20955-1-arouhi@sitime.com?part=10