Re: [PATCH net-next v4 3/3] dpll: add SiTime SiT9531x DPLL clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The DPLL pin properties are freed immediately after registration, causing a Use-After-Free when the DPLL subsystem accesses them.
- [High] The embedded sync (esync) feature fails to activate because the hardware pulser mode is never enabled.
- [Medium] The DCO (digitally controlled oscillator) adjustment feature is implemented but never hooked into the DPLL subsystem, making it dead code.
- [Low] Comment falsely claims all outputs are bound to DPLL 0, contradicting the implemented logic.
- [Low] Comments erroneously describe byte order as 'big-endian' when the code correctly implements it as Little-Endian.
--

--- Patch [3]: [PATCH net-next v4 3/3] dpll: add SiTime SiT9531x DPLL clock driver ---
commit 254cd6abf86b3bbdd7f42035ec2ed9c191f48a3b
Author: Ali Rouhi <arouhi@sitime.com>
Subject: dpll: add SiTime SiT9531x DPLL clock driver

Add a DPLL subsystem driver for the SiTime SiT95316 and SiT95317
clock generators. The driver reads all configuration from the device's
on-chip NVM at probe time.

diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
--- a/drivers/dpll/sit9531x/core.c
+++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> +static int sit9531x_dco_read_divn2(struct sit9531x_dev *sitdev, u8 pll_idx,
> +				   s64 *n2_int, u32 *n2_fracn, u32 *n2_fracd)
> +{
> +	int rc, i;
> +	u8 v;
> +
> +	/* Integer part (5 bytes, big-endian: 0x42 is MSB) */
> +	*n2_int = 0;
> +	for (i = 4; i >= 0; i--) {
> +		rc = sit9531x_read_pll_u8(sitdev, pll_idx,
> +					  SIT9531X_PLL_REG_DIVN2_INT + i, &v);

[Severity: Low]
Does this comment accurately reflect the byte order? The loop processes byte
0x42 first and stores it as the most significant byte, which means the MSB
is at the highest memory address. This matches little-endian rather than
big-endian byte order.

[ ... ]
> +int sit9531x_dco_adjust(struct sit9531x_dev *sitdev, u8 pll_idx, s64 ppb)
> +{

[Severity: Medium]
Is the digitally controlled oscillator (DCO) adjustment feature accessible
from userspace? The commit message mentions DCO frequency adjustment support,
and this function implements the fractional divider steering. However,
sit9531x_dco_adjust() does not appear to be hooked into the DPLL subsystem
callbacks (such as .ffo_get / .ffo_set).

[ ... ]
diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
--- a/drivers/dpll/sit9531x/dpll.c
+++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> +static int
> +sit9531x_dpll_output_pin_frequency_set(const struct dpll_pin *pin,
> +				       void *pin_priv,
> +				       const struct dpll_device *dpll,
> +				       void *dpll_priv, u64 frequency,
> +				       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;
> +
> +	/* Use the actual PLL that drives this output (populated by
> +	 * out_state_fetch from the chip's OUT_MAP registers), not the
> +	 * DPLL device id -- in our current registration all outputs are
> +	 * bound to DPLL 0 for convenience, but physically they may be
> +	 * driven by PLL A/B/C/D with different Fvco.
> +	 */

[Severity: Low]
Is the comment about all outputs being bound to DPLL 0 still accurate?
In sit9531x_dpll_pin_is_registrable(), output pins seem to be dynamically
bound to the DPLL device that physically drives them rather than being
statically bound to DPLL 0.

[ ... ]
> +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)
> +{
[ ... ]
> +	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);

[Severity: High]
Will the embedded sync feature emit pulses without enabling the pulser
hardware mode? The callback configures the pulse control byte, but does not
call sit9531x_pll_sysref_mode_set() to transition the underlying PLL into
the pulser mode and enable the SYSREF output buffer.

[ ... ]
> +static int sit9531x_dpll_pin_register(struct sit9531x_dpll_pin *pin,
> +				      u32 index)
> +{
> +	struct sit9531x_dpll *sitdpll = pin->dpll;
> +	struct sit9531x_pin_props *props;
> +	const struct dpll_pin_ops *ops;
> +	int rc;
> +
> +	/* Get pin properties from firmware nodes */
> +	props = sit9531x_pin_props_get(sitdpll->dev, pin->dir, pin->id);
> +	if (IS_ERR(props))
> +		return PTR_ERR(props);
> +
> +	/* Save package label and firmware node */
> +	strscpy(pin->label, props->package_label, sizeof(pin->label));
> +	pin->fwnode = fwnode_handle_get(props->fwnode);
> +	pin->esync_control = props->esync_control;
> +
> +	/* Create or get existing DPLL pin */
> +	pin->dpll_pin = dpll_pin_get(sitdpll->dev->clock_id, index,
> +				     THIS_MODULE, &props->dpll_props,
> +				     &pin->tracker);
> +	if (IS_ERR(pin->dpll_pin)) {
> +		rc = PTR_ERR(pin->dpll_pin);
> +		goto err_pin_get;
> +	}
> +	dpll_pin_fwnode_set(pin->dpll_pin, props->fwnode);
> +
> +	ops = sit9531x_dpll_pin_ops_get(pin);
> +
> +	/* Register the pin */
> +	rc = dpll_pin_register(sitdpll->dpll_dev, pin->dpll_pin, ops, pin);
> +	if (rc)
> +		goto err_register;
> +
> +	/* Free pin properties */
> +	sit9531x_pin_props_put(props);

[Severity: Critical]
Could this sequence cause a use-after-free when the DPLL subsystem accesses
the pin properties?

The sit9531x_pin_props_get() function dynamically allocates the
freq_supported array and stores it in props->dpll_props.freq_supported.
The dpll_pin_get() function performs a shallow copy of the struct, keeping
the raw pointers.

Immediately after dpll_pin_register(), sit9531x_pin_props_put(props) is
called, which frees the freq_supported array and the props struct, leaving
the DPLL core with dangling pointers.

[ ... ]

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