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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Use-After-Free of Pin Properties and Frequency Arrays
- [High] State Mismatch / Logic Flaw in Pin Getters
- [High] Destructive Priority Overwrite in Hardware Table
- [High] Use-After-Free on Probe Error Path Due to Missing DPLL Unregistration
- [Medium] Silent Failure on Manual Pin Connection Request
- [Medium] Missing SYSREF/SYNCB/Pulser Features
- [Medium] Hardware Routing Loop via INTSYNC
--

--- Patch [3]: [PATCH net-next v6 3/3] dpll: add SiTime SiT9531x DPLL clock driver ---
commit 841a91aff3f30b8a2e2bee8952633134d7bbdee2
Author: Ali Rouhi <arouhi@sitime.com>
Subject: dpll: add SiTime SiT9531x DPLL clock driver
Summary: This commit adds a DPLL subsystem driver for the SiTime SiT95316
and SiT95317 clock generators. It supports hardware state monitoring, priority
management, and frequency/phase configuration.

>  - SYSREF/SYNCB/Pulser output mode control

[Severity: Medium]
Is this functionality fully exposed to userspace? The commit message mentions
support for SYSREF/SYNCB/Pulser modes, but looking at the driver,
sit9531x_output_pulse_ctrl_set() only appears to be called with a hardcoded
default during embedded sync setup. It looks like the other modes might not
be reachable.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 0000000000000..711ab16b0897a
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> @@ -596,32 +596,25 @@ int sit9531x_input_prio_set(struct sit9531x_dev *sitdev, u8 pll_idx,
>  
>  	usleep_range(10000, 12000);
>  
>  	/*
>  	 * Write the requested slot on Page 1, then fill the remaining
>  	 * slots with the same source, matching the procedure script's
>  	 * backfill behaviour.
>  	 */
>  	for (slot = prio; slot < SIT9531X_PRIO_MAX_SLOTS; slot++) {
>  		reg = sit9531x_prio_reg(pll_idx, slot);
>  
>  		rc = sit9531x_read_u8(sitdev, reg, &val);
>  		if (rc)
>  			goto commit;
>  
>  		val = sit9531x_prio_slot_set(val, slot, input_idx);

[Severity: High]
Does this loop unintentionally wipe out fallback configurations? When userspace
sets a priority, this loop fills not only the requested slot but also all
lower-priority slots with the exact same input source. This appears to destroy
any other fallback inputs the user might have configured in the hardware table.

[ ... ]

> @@ -2496,25 +2496,15 @@ int sit9531x_dev_start(struct sit9531x_dev *sitdev)
>  	if (rc)
>  		return rc;
>  
>  	list_for_each_entry(sitdpll, &sitdev->dplls, list) {
>  		rc = sit9531x_dpll_register(sitdpll);
>  		if (rc) {
>  			dev_err_probe(sitdev->dev, rc,
>  				      "Failed to register DPLL%u\n",
>  				      sitdpll->id);
>  			return rc;
>  		}
>  	}

[ ... ]

> @@ -2825,18 +2815,10 @@ static void sit9531x_dpll_list_free(struct sit9531x_dev *sitdev)
>  {
>  	struct sit9531x_dpll *sitdpll, *next;
>  
>  	list_for_each_entry_safe(sitdpll, next, &sitdev->dplls, list) {
>  		list_del(&sitdpll->list);
>  		sit9531x_dpll_free(sitdpll);
>  	}
>  }

[Severity: High]
Could this error path leave the DPLL subsystem with dangling pointers? If
sit9531x_dpll_register() succeeds for the first DPLL but fails for the next,
sit9531x_dev_start() returns an error without unwinding the already
registered DPLLs. The error handler in sit9531x_devm_dpll_init() then calls
sit9531x_dpll_list_free() to free the backing memory directly, bypassing
dpll_device_unregister() and dpll_pin_unregister().

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 0000000000000..364f870ac3041
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> @@ -259,32 +259,25 @@ static int
>  sit9531x_dpll_input_pin_state_on_dpll_get(const struct dpll_pin *pin,
>  					  void *pin_priv,
>  					  const struct dpll_device *dpll,
>  					  void *dpll_priv,
>  					  enum dpll_pin_state *state,
>  					  struct netlink_ext_ack *extack)
>  {

[ ... ]

>  	if (chan->locked && !chan->inner_lol &&
>  	    chan->selected_ref == dpin->id)
>  		*state = DPLL_PIN_STATE_CONNECTED;
>  	/* In auto mode, enabled inputs with good signal are selectable */
>  	else if (!chan->mode && ref->enabled && !ref->los && !ref->oof)
>  		*state = DPLL_PIN_STATE_SELECTABLE;
>  	else
>  		*state = DPLL_PIN_STATE_DISCONNECTED;

[Severity: High]
Might this logic report an incorrect routing state? The ref->enabled flag
represents a global physical state for the pin. If DPLL A requests an input
and enables the receiver, DPLL B will also see ref->enabled as true and
report the pin as DPLL_PIN_STATE_SELECTABLE, even if that pin isn't in DPLL
B's priority table.

Additionally, if a signal is lost (ref->los becomes true), this drops the
state to DPLL_PIN_STATE_DISCONNECTED. Does the DPLL API expect degraded pins
to still show as logically connected or selectable rather than entirely
disconnected?

[ ... ]

> @@ -326,32 +319,25 @@ static int
>  sit9531x_dpll_input_pin_state_on_dpll_set(const struct dpll_pin *pin,
>  					  void *pin_priv,
>  					  const struct dpll_device *dpll,
>  					  void *dpll_priv,
>  					  enum dpll_pin_state state,
>  					  struct netlink_ext_ack *extack)
>  {

[ ... ]

>  	switch (state) {
>  	case DPLL_PIN_STATE_DISCONNECTED:
>  		rc = sit9531x_input_prio_remove(sitdev, sitdpll->id, hw_src);
>  		if (rc)
>  			break;
>  		ref->pll_mask &= ~pll_bit;
>  		if (ref->pll_mask)
>  			rc = 0;	/* another DPLL still uses this input */
>  		else
>  			rc = sit9531x_input_disable(sitdev, dpin->id);
>  		break;
>  	case DPLL_PIN_STATE_SELECTABLE:
>  	case DPLL_PIN_STATE_CONNECTED:
>  		rc = sit9531x_input_enable(sitdev, dpin->id);
>  		if (rc)
>  			break;
>  		rc = sit9531x_input_prio_add(sitdev, sitdpll->id, hw_src);

[Severity: Medium]
Should DPLL_PIN_STATE_CONNECTED enforce a manual lock here? It appears to
fall through to the exact same logic as DPLL_PIN_STATE_SELECTABLE, adding
the input to the priority table but leaving the hardware in automatic
selection mode instead of forcing a connection.

[ ... ]

> @@ -651,32 +637,25 @@ static int
>  sit9531x_dpll_intsync_src_state_on_dpll_set(const struct dpll_pin *pin,
>  					    void *pin_priv,
>  					    const struct dpll_device *dpll,
>  					    void *dpll_priv,
>  					    enum dpll_pin_state state,
>  					    struct netlink_ext_ack *extack)
>  {

[ ... ]

>  	switch (state) {
>  	case DPLL_PIN_STATE_CONNECTED:
>  		if (sitdev->intsync_src == sitdpll->id)
>  			break;
>  		if (sitdev->intsync_src >= 0) {
>  			NL_SET_ERR_MSG(extack,
>  				       "INTSYNC is already sourced by another PLL");
>  			rc = -EBUSY;
>  			break;
>  		}
>  		rc = sit9531x_intsync_enable(sitdev, sitdpll->id);

[Severity: Medium]
Is there a protection missing here against hardware routing loops? While the
destination getter correctly prevents adding INTSYNC to a PLL's priority
table if that PLL is already the source, this function doesn't seem to check
if the PLL already has INTSYNC as a destination before making it the source.
Could userspace configure a PLL to source from INTSYNC and then drive INTSYNC
with the same PLL?

> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> index 0000000000000..8a0105c8c6472
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/prop.c

[ ... ]

> @@ -357,10 +357,8 @@ void sit9531x_pin_props_put(struct sit9531x_pin_props *props)
>  {
>  	kfree(props->dpll_props.freq_supported);
>  
>  	if (props->fwnode)
>  		fwnode_handle_put(props->fwnode);
>  
>  	kfree(props);
>  }

[Severity: Critical]
Will this lead to a use-after-free when userspace queries the pin properties?
The sit9531x_pin_props_get() function dynamically allocates both the props
structure and the freq_supported array. When passed into dpll_pin_get()
in sit9531x_dpll_pin_register(), the DPLL core retains those pointers.
Since sit9531x_pin_props_put() immediately frees these allocations on the
success path, it looks like the DPLL core might be left holding dangling
pointers.

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