Re: [PATCH net-next v8 11/15] dpll: sit9531x: add support to get and set esync on pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver's esync and regular output frequency controls are not mutually exclusive or synchronized, allowing subsequent operations to silently break the esync state.
- [High] The driver misinterprets the DPLL embedded sync frequency parameter, requiring a 10 MHz carrier instead of the 1 Hz sync frequency.
- [High] Disabling embedded sync mutes the output entirely and leaks the `pulse_ctrl` state, corrupting the pin if re-enabled as a regular clock.
- [Medium] Missing rollback on error path leaves the hardware in a partially configured state.
--

commit 6a002e713c18df7ee3b9b8cd0d53f2b485acb5fc
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to get and set esync on pins

This commit introduces support for embedded sync marks on pins for the
sit9531x DPLL driver. It adds esync get and set routines that control
the output frequency and pulse configurations.

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index c07fb22565107..76d7d7d21d3d6 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -25,6 +25,17 @@
>  #define SIT9531X_ESYNC_FREQ_10MHZ	10000000ULL
>  #define SIT9531X_ESYNC_PULSE_DEFAULT	50
>  
> +static const struct dpll_pin_frequency sit9531x_esync_ranges[] = {
> +	DPLL_PIN_FREQUENCY(0),
> +	DPLL_PIN_FREQUENCY(SIT9531X_ESYNC_FREQ_10MHZ),
> +};

[Severity: High]
Does this range advertise the carrier frequency instead of the embedded
sync frequency?

The DPLL subsystem ABI expects the frequency parameter in esync_set to
represent the embedded sync pulse frequency itself, which would be 1 Hz
here (matching the commit message's description of a one-pulse-per-second
boundary). Advertising 10,000,000 might confuse userspace expecting a 
sync rate.

[ ... ]
> +static int
> +sit9531x_dpll_output_pin_esync_set(const struct dpll_pin *pin,
> +				   void *pin_priv,
> +				   const struct dpll_device *dpll,
> +				   void *dpll_priv,
> +				   u64 freq,
> +				   struct netlink_ext_ack *extack)
> +{
> +	struct sit9531x_dpll_pin *dpin = pin_priv;
> +	struct sit9531x_dpll *sitdpll = dpll_priv;
> +	struct sit9531x_dev *sitdev = sitdpll->dev;
> +	u8 actual_pll;
> +	int rc;
> +
> +	if (!sit9531x_dpll_esync_pin_supported(dpin)) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Embedded sync not enabled for this pin");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	actual_pll = sitdev->out[dpin->id].pll_idx;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +
> +	/*
> +	 * This output is a dedicated embedded-sync pin.
> +	 * Treat freq=0 as a request to disable the entire output.
> +	 */
> +	if (!freq) {
> +		rc = sit9531x_output_disable(sitdev, dpin->id);
> +		if (!rc)
> +			dpin->esync_freq = 0;
> +		mutex_unlock(&sitdev->multiop_lock);
> +		return rc;
> +	}

[Severity: High]
Does disabling the embedded sync here inadvertently corrupt the pin's state
for future use?

The commit message states that "disabling it restores the requested rate".
However, calling sit9531x_output_disable() forces the pin into Hi-Z
instead of restoring the prior frequency. 

Additionally, does this leak the pulse width configuration? Because
PROG0 PULSE_CTRL is not cleared, if the pin is later re-enabled as a
regular clock via state_on_dpll_set(), it looks like it will still run
with the stale embedded sync pulse control active.

> +
> +	if (freq != SIT9531X_ESYNC_FREQ_10MHZ) {
> +		mutex_unlock(&sitdev->multiop_lock);
> +		NL_SET_ERR_MSG(extack,
> +			       "Only 10 MHz esync frequency is supported");
> +		return -EINVAL;
> +	}

[Severity: High]
Will this check incorrectly reject a valid 1 Hz embedded sync request
from userspace?

If userspace passes 1 Hz to configure the 1 PPS sync pulse, this check
returns -EINVAL. It seems to conflate the carrier frequency with the
sync pulse frequency.

> +
> +	rc = sit9531x_output_freq_set(sitdev, dpin->id, actual_pll,
> +				      SIT9531X_ESYNC_FREQ_10MHZ);
> +	/*
> +	 * Program the pulse generator (PROG0 PULSE_CTRL) so the embedded-sync
> +	 * pulse is actually emitted; without it the output carries the clock
> +	 * but no esync marker.  SIT9531X_ESYNC_PULSE_DEFAULT is the same duty
> +	 * the esync_get callback advertises.
> +	 */
> +	if (!rc)
> +		rc = sit9531x_output_pulse_ctrl_set(sitdev, dpin->id,
> +						    SIT9531X_ESYNC_PULSE_DEFAULT);
> +	if (!rc)
> +		rc = sit9531x_output_enable(sitdev, dpin->id);
> +
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	if (!rc)
> +		dpin->esync_freq = SIT9531X_ESYNC_FREQ_10MHZ;
> +
> +	return rc;
> +}

[Severity: Medium]
If sit9531x_output_pulse_ctrl_set() fails, does this leave the hardware
in a partially configured state?

The hardware carrier frequency is changed to 10 MHz via
sit9531x_output_freq_set() first. If the subsequent pulse control set
fails, the function returns an error without reverting the frequency
change. This seems to irreversibly alter the hardware carrier frequency
to 10 MHz while leaving dpin->esync_freq as 0, desynchronizing the
software and hardware states.

[Severity: High]
Is the embedded sync state vulnerable to being silently broken by a
standard frequency change?

Because the esync logic and regular output frequency controls do not appear
to be mutually exclusive, a subsequent call to:

    sit9531x_dpll_output_pin_frequency_set()
        sit9531x_output_freq_set()

will unconditionally overwrite the hardware output divider without checking
if esync is active or clearing the dpin->esync_freq state. This could
silently corrupt the esync operation while software still reports it as
active.

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