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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] DPLL pin properties (`package_label` and `freq_supported`) are freed immediately after registration, causing a critical use-after-free vulnerability.
- [High] The driver uses a non-existent macro `kzalloc_obj` for memory allocation, causing a build failure.
- [High] The crystal oscillator frequency (`xtal_freq`) is never initialized, permanently breaking core frequency and phase calculations.
- [High] The INTRB threaded IRQ is registered before the kthread worker is initialized and torn down after the worker is destroyed, causing a NULL pointer dereference race.
- [High] The driver fails to clear hardware interrupt notifications, causing an unhandled interrupt storm.
- [High] Phase adjustments are written to the wrong hardware registers because the logical output index is used instead of the physical slot.
- [High] Unsigned 32-bit hardware fractional numerators are read into signed integers, causing a sign-extension bug that corrupts DCO math.
- [Medium] Hardware state cache arrays are updated and read concurrently without locking, leading to data races.
- [Medium] DCO frequency adjustment and SYSREF/SYNCB output modes are fully implemented but entirely unreachable (Dead Code).
--

commit 7809f8a3992ad8b550d7f2507263595d95f3b3d8
Author: Ali Rouhi <rouhi.ali@gmail.com>

dpll: add SiTime SiT9531x DPLL clock driver

Adds a DPLL subsystem driver for the SiTime SiT95316 and SiT95317
clock generators.

The driver exposes all inputs and outputs through the Linux DPLL
subsystem, supporting:
[ ... ]
 - DCO (digitally controlled oscillator) frequency adjustment
[ ... ]
 - SYSREF/SYNCB/Pulser output mode control

This isn't a bug, but the commit message indicates that the driver supports
DCO frequency adjustment and SYSREF/SYNCB output mode control. However,
these features do not appear to be hooked into sit9531x_dpll_device_ops or
otherwise exposed to the DPLL subsystem.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> new file mode 100644
> index 0000000000000..4c15a10aaa0ae
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> +static int sit9531x_dco_read_divn(struct sit9531x_dev *sitdev, u8 pll_idx,
> +				  u32 *int_part, s32 *fracn, u32 *fracd)
> +{
[ ... ]
> +	/* Numerator (4 bytes, little-endian) */
> +	*fracn = 0;
> +	for (i = 3; i >= 0; i--) {
> +		rc = sit9531x_read_pll_u8(sitdev, pll_idx,
> +					  SIT9531X_PLL_REG_DIVN_NUM + i, &v);
> +		if (rc)
> +			return rc;
> +		*fracn = (*fracn << 8) | v;
> +	}

Could this left shift into a signed 32-bit integer result in a negative
value?

If the hardware fractional numerator represents a value greater than or
equal to 0.5, the most significant bit will be set. This might sign-extend
when cast later and alter the calculation in sit9531x_dco_calc_inner.

[ ... ]
> +static u64 sit9531x_get_fvco(struct sit9531x_dev *sitdev, u8 pll_idx)
> +{
[ ... ]
> +	doubler = sit9531x_is_xo_doubler_enabled(sitdev);
> +	if (doubler < 0)
> +		return 0;
> +
> +	fref = (u64)sitdev->xtal_freq << doubler;

Where is sitdev->xtal_freq populated?

If it remains zero-initialized from the initial allocation, fref will
evaluate to zero. This would cause fvco to return zero and affect
subsequent frequency calculations and hardware state updates.

[ ... ]
> +int sit9531x_output_phase_adjust_set(struct sit9531x_dev *sitdev,
> +				     u8 out_idx, s32 phase_ps)
> +{
[ ... ]
> +	page = (out_idx < 6) ? SIT9531X_PAGE_OUTSYS0
> +			     : SIT9531X_PAGE_OUTSYS1;
> +	base = SIT9531X_OUT_PRG_DELAY_BASE +
> +	       SIT9531X_OUT_PRG_SLOT_STRIDE * (out_idx % 6);

Should this use the physical slot mapping instead of the logical index?

Using out_idx directly here might cause the driver to write to the phase
delay registers of a different output port. sit9531x_output_freq_set() uses
info->clkout_map to resolve this correctly.

[ ... ]
> +static irqreturn_t sit9531x_irq_thread_fn(int irq, void *data)
> +{
> +	struct sit9531x_dev *sitdev = data;
> +
> +	kthread_mod_delayed_work(sitdev->kworker, &sitdev->work, 0);
> +	return IRQ_HANDLED;
> +}

Does the hardware require interrupt notifications to be explicitly cleared?

The handler returns IRQ_HANDLED after waking the worker, but
sit9531x_clear_notifications() is never called. If this is a level-triggered
interrupt line, leaving the notification active might cause an interrupt
storm.

[ ... ]
> +int sit9531x_dev_probe(struct sit9531x_dev *sitdev)
> +{
[ ... ]
> +	if (sitdev->irq > 0) {
> +		rc = devm_request_threaded_irq(sitdev->dev, sitdev->irq,
> +					       NULL, sit9531x_irq_thread_fn,
> +					       IRQF_ONESHOT,
> +					       dev_name(sitdev->dev), sitdev);
> +		if (rc)
> +			return dev_err_probe(sitdev->dev, rc,
> +					     "Failed to request IRQ %d\n",
> +					     sitdev->irq);
> +		dev_info(sitdev->dev,
> +			 "INTRB IRQ %d wired (threaded handler kicks periodic poll)\n",
> +			 sitdev->irq);
> +	}
> +
> +	/* Register DPLL channels */
> +	rc = sit9531x_devm_dpll_init(sitdev);

What happens if the interrupt fires right after it is registered?

Since devm_request_threaded_irq is called before the kworker is initialized
inside sit9531x_devm_dpll_init, an early interrupt could cause the thread
handler to dereference a null pointer when interacting with the worker.

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> new file mode 100644
> index 0000000000000..c766c7c2dd366
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> +static int
> +sit9531x_dpll_lock_status_get(const struct dpll_device *dpll, void *dpll_priv,
> +			      enum dpll_lock_status *status,
> +			      enum dpll_lock_status_error *status_error,
> +			      struct netlink_ext_ack *extack)
> +{
> +	struct sit9531x_dpll *sitdpll = dpll_priv;
> +	const struct sit9531x_chan *chan;
> +
> +	if (status_error)
> +		*status_error = DPLL_LOCK_STATUS_ERROR_NONE;
> +
> +	chan = sit9531x_chan_state_get(sitdpll->dev, sitdpll->id);
> +
> +	if (chan->locked) {

Is it safe to read the channel state here without holding a lock?

The periodic worker locklessly updates the hardware state cache arrays.
Concurrent reads in the netlink callbacks could potentially see torn data
or mismatched state vectors.

[ ... ]
> +static struct sit9531x_dpll_pin *
> +sit9531x_dpll_pin_alloc(struct sit9531x_dpll *sitdpll,
> +			enum dpll_pin_direction dir, u8 id)
> +{
> +	struct sit9531x_dpll_pin *pin;
> +
> +	pin = kzalloc_obj(*pin, GFP_KERNEL);
> +	if (!pin)
> +		return ERR_PTR(-ENOMEM);

Does the macro kzalloc_obj exist in the tree?

It looks like this might cause a build failure since kzalloc is the standard
allocation macro. This same pattern seems to be used in sit9531x_dpll_alloc
and sit9531x_pin_props_get.

[ ... ]
> +static int sit9531x_dpll_pin_register(struct sit9531x_dpll_pin *pin,
> +				      u32 index)
> +{
[ ... ]
> +	/* 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);
> +
> +	return 0;

Are the pointers for package_label and freq_supported safely copied?

Looking at the DPLL core, dpll_pin_get() appears to perform a shallow copy.
Freeing the properties right after registration could result in a use after
free when the subsystem later attempts to read them.

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