Re: [PATCH net-next v7 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:
- [High] Unconditional insertion of DISCONNECTED pins into the hardware priority table.
- [High] Disconnecting INTSYNC as the last source fails to stop it from being tracked.
- [Medium] INTSYNC destination state reporting ignores the hardware priority table.
- [Medium] The INTSYNC destination pin incorrectly accepts DPLL_PIN_STATE_CONNECTED without enforcing it.
- [Medium] Input and INTSYNC destination pin states report CONNECTED and hide SELECTABLE pins during free-run mode.
- [Low] Input pins falsely advertise phase adjustment capabilities by providing phase_adjust_get and configuring a phase_range.
- [Low] Undefined macro `kzalloc_obj` used for memory allocation.
--

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

dpll: add SiTime SiT9531x DPLL clock driver

This commit adds a DPLL subsystem driver for the SiTime SiT95316 and
SiT95317 clock generators. It provides support for lock monitoring,
input priority management, frequency offset measurement, and output
phase adjustment using the device's I2C interface and NVM.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> new file mode 100644
> index 0000000000000..cd283922e7ffa
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> @@ -782,31 +782,31 @@ int sit9531x_input_prio_set(struct sit9531x_dev *sitdev, u8 pll_idx,
>  int sit9531x_input_prio_remove(struct sit9531x_dev *sitdev, u8 pll_idx,
>  			       u8 input_idx)
>  {
>  	u8 srcs[SIT9531X_PRIO_MAX_SLOTS];
>  	u8 kept[SIT9531X_PRIO_MAX_SLOTS];
>  	u8 slot, count = 0;
>  	bool found = false;
>  	int rc;
>  
>  	lockdep_assert_held(&sitdev->multiop_lock);
>  
>  	if (pll_idx >= SIT9531X_NUM_PLLS)
>  		return -EINVAL;
>  
>  	rc = sit9531x_prio_table_read(sitdev, pll_idx, srcs);
>  	if (rc)
>  		return rc;
>  
>  	for (slot = 0; slot < SIT9531X_PRIO_MAX_SLOTS; slot++) {
>  		if (srcs[slot] == input_idx)
>  			found = true;
>  		else
>  			kept[count++] = srcs[slot];
>  	}
>  
>  	if (!found || count == 0)
>  		return 0;

[Severity: High]
If userspace requests to disconnect the INTSYNC pin when it is the only
source in the priority table, the count becomes 0 and this function
aborts without modifying the table.

Since the INTSYNC net lacks a physical buffer that can be disabled, does
this leave the hardware actively tracking the INTSYNC reference when it was
commanded to disconnect?

[ ... ]
> @@ -2526,17 +2526,17 @@ void sit9531x_dev_stop(struct sit9531x_dev *sitdev)
>  }
>  
>  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);

[Severity: Low]
Does this macro exist in the kernel headers?

The use of kzalloc_obj appears to be an undefined identifier that will cause
a build failure here, and in a few other allocation sites throughout the
driver (such as sit9531x_dpll_alloc and sit9531x_pin_props_get).

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> new file mode 100644
> index 0000000000000..e258beceffdd4
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> @@ -258,35 +258,35 @@ 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)
>  {
>  	struct sit9531x_dpll_pin *dpin = pin_priv;
>  	struct sit9531x_dpll *sitdpll = dpll_priv;
>  	struct sit9531x_dev *sitdev = sitdpll->dev;
>  	const struct sit9531x_chan *chan;
>  	const struct sit9531x_ref *ref;
>  
>  	chan = sit9531x_chan_state_get(sitdev, sitdpll->id);
>  	ref = sit9531x_ref_state_get(sitdev, dpin->id);
>  
>  	mutex_lock(&sitdev->multiop_lock);
>  
>  	/*
>  	 * Report CONNECTED only when the PLL is genuinely locked to this
>  	 * input.  chan->locked already excludes a PLL the configuration
>  	 * leaves unused, which would otherwise claim its default source as
>  	 * connected right after probe; the inner loop has to be locked as
>  	 * well before the reference can be called connected.
>  	 */
>  	if (chan->locked && !chan->inner_lol &&
>  	    chan->selected_ref == dpin->id)
>  		*state = DPLL_PIN_STATE_CONNECTED;
>  	/*
>  	 * Selectable means this DPLL may choose the input, so it has to be
>  	 * in this DPLL's priority table -- ref->enabled only says the
>  	 * receiver is powered, which is shared by every DPLL fed from the
>  	 * same lane.  Signal quality is reported through the pin's own
>  	 * attributes rather than by demoting the state, so a lane that is
>  	 * routed here but momentarily in LOS stays selectable.
>  	 */
>  	else if (!chan->mode && (ref->pll_mask & BIT(sitdpll->id)))
>  		*state = DPLL_PIN_STATE_SELECTABLE;

[Severity: Medium]
When the PLL is in free-run mode (outer loop disabled), the state getters
can still see chan->locked as true if the outer loss-of-lock bit is clear.

Without verifying !chan->mode here, won't this incorrectly report the pin
as connected while the PLL is actually freely running and ignoring the
reference?

Additionally, because the subsequent branch for the selectable state requires
!chan->mode, are other eligible pins erroneously reported as disconnected
instead of selectable during free-run mode?

[ ... ]
> @@ -433,26 +433,26 @@ static int
>  sit9531x_dpll_input_pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
>  				 const struct dpll_device *dpll, void *dpll_priv,
>  				 u32 prio, 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;
>  
>  	if (dpin->dir != DPLL_PIN_DIRECTION_INPUT) {
>  		NL_SET_ERR_MSG(extack, "Priority applies only to input pins");
>  		return -EINVAL;
>  	}
>  
>  	if (prio >= SIT9531X_PRIO_MAX_SLOTS) {
>  		NL_SET_ERR_MSG(extack, "Priority out of range (0-10)");
>  		return -EINVAL;
>  	}
>  
>  	mutex_lock(&sitdev->multiop_lock);
>  	rc = sit9531x_input_prio_set(sitdev, sitdpll->id,
>  				     sit9531x_input_hw_src(dpin->id),
>  				     (u8)prio);
>  	mutex_unlock(&sitdev->multiop_lock);

[Severity: High]
This logic updates the priority table unconditionally.

If a pin is currently in the disconnected state, will inserting it into the
hardware priority table cause the PLL to start tracking it automatically if
higher-priority sources fail?

[ ... ]
> @@ -585,18 +585,18 @@ static int
>  sit9531x_dpll_input_pin_phase_offset_get(const struct dpll_pin *pin,
>  					 void *pin_priv,
>  					 const struct dpll_device *dpll,
>  					 void *dpll_priv, s64 *phase_offset,
>  					 struct netlink_ext_ack *extack)
>  {
>  [ ... ]
>  static const struct dpll_pin_ops sit9531x_dpll_input_pin_ops = {
>  	.direction_get		= sit9531x_dpll_input_pin_direction_get,
>  	.frequency_get		= sit9531x_dpll_input_pin_frequency_get,
>  	.state_on_dpll_get	= sit9531x_dpll_input_pin_state_on_dpll_get,
>  	.state_on_dpll_set	= sit9531x_dpll_input_pin_state_on_dpll_set,
>  	.prio_get		= sit9531x_dpll_input_pin_prio_get,
>  	.prio_set		= sit9531x_dpll_input_pin_prio_set,
>  	.phase_adjust_get	= sit9531x_dpll_input_pin_phase_adjust_get,
>  	.phase_offset_get	= sit9531x_dpll_input_pin_phase_offset_get,

[Severity: Low]
Is it intentional to advertise phase adjustment on input pins?

Phase adjustment appears to be an output-only feature in this driver, and
the lack of a phase_adjust_set callback means setting the adjustment
is not supported for inputs.

[ ... ]
> @@ -744,22 +744,22 @@ sit9531x_dpll_intsync_src_state_on_dpll_set(const struct dpll_pin *pin,
>  static int
>  sit9531x_dpll_intsync_dst_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)
>  {
>  	struct sit9531x_dpll *sitdpll = dpll_priv;
>  	struct sit9531x_dev *sitdev = sitdpll->dev;
>  	const struct sit9531x_chan *chan;
>  
>  	chan = sit9531x_chan_state_get(sitdev, sitdpll->id);
>  
>  	mutex_lock(&sitdev->multiop_lock);
>  	if (sitdev->intsync_src == sitdpll->id)
>  		*state = DPLL_PIN_STATE_DISCONNECTED;
>  	else if (chan->locked && !chan->inner_lol &&
>  		 chan->selected_ref == SIT9531X_INTSYNC_PIN_ID)
>  		*state = DPLL_PIN_STATE_CONNECTED;
>  	else if (!chan->mode && sitdev->intsync_src >= 0)
>  		*state = DPLL_PIN_STATE_SELECTABLE;

[Severity: Medium]
This unconditionally reports the INTSYNC pin as selectable as long as
another PLL is driving INTSYNC and this PLL is in automatic mode.

If userspace previously set the INTSYNC pin to a disconnected state (removing
it from the priority table), shouldn't this reflect the disconnected state
rather than bypassing the priority table check?

>  	else
>  		*state = DPLL_PIN_STATE_DISCONNECTED;
>  	mutex_unlock(&sitdev->multiop_lock);
>  
>  	return 0;
>  }
>  
>  /*
>   * sit9531x_dpll_intsync_dst_state_on_dpll_set - lock a PLL to INTSYNC
>   *
>   *   CONNECTED/SELECTABLE -> add INTSYNC to this PLL's priority table
>   *   DISCONNECTED         -> drop INTSYNC from this PLL's priority table
>   *
>   * INTSYNC is an internal net with no physical receiver, so only the
>   * per-PLL priority table is touched; the source pin controls generation.
>   */
>  static int
>  sit9531x_dpll_intsync_dst_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)
>  {
>  	struct sit9531x_dpll *sitdpll = dpll_priv;
>  	struct sit9531x_dev *sitdev = sitdpll->dev;
>  	u8 hw_src = sit9531x_input_hw_src(SIT9531X_INTSYNC_PIN_ID);
>  	int rc;
>  
>  	mutex_lock(&sitdev->multiop_lock);
>  
>  	switch (state) {
>  	case DPLL_PIN_STATE_DISCONNECTED:
>  		rc = sit9531x_input_prio_remove(sitdev, sitdpll->id, hw_src);
>  		break;
>  	case DPLL_PIN_STATE_SELECTABLE:
>  	case DPLL_PIN_STATE_CONNECTED:
>  		if (sitdev->intsync_src == sitdpll->id) {
>  			NL_SET_ERR_MSG(extack,
>  				       "PLL cannot lock to the INTSYNC it drives");
>  			rc = -EINVAL;
>  			break;
>  		}
>  		rc = sit9531x_input_prio_add(sitdev, sitdpll->id, hw_src);
>  		break;

[Severity: Medium]
Is it correct to handle DPLL_PIN_STATE_CONNECTED identically to
DPLL_PIN_STATE_SELECTABLE here?

For physical inputs, the driver explicitly rejects connected states because
the device selects references by priority. Accepting it here silently leaves
the hardware in automatic selection mode, mismatched with the userspace
request for an exclusive connection.

> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> new file mode 100644
> index 0000000000000..8a0105c8c6472
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/prop.c
[ ... ]
> @@ -245,15 +245,15 @@ sit9531x_pin_props_get(struct sit9531x_dev *sitdev,
>  		props->dpll_props.type = DPLL_PIN_TYPE_GNSS;
>  		props->dpll_props.capabilities =
>  			DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
>  		curr_freq = sitdev->out[index].freq;
>  	}
>  
>  	/* Allow phase-adjust over +/-1 ms window.  The subsystem rejects
>  	 * pin_set(phase-adjust, X) when X falls outside [min, max], so
>  	 * leaving these at 0 silently blocks every netlink call.  1 ms is
>  	 * well beyond the DCO dynamic range but costs nothing.
>  	 */
>  	props->dpll_props.phase_range.min = -1000000000; /* -1 ms in ps */
>  	props->dpll_props.phase_range.max =  1000000000; /* +1 ms in ps */

[Severity: Low]
This unconditionally configures phase range limits for all pins, including
inputs. Since inputs do not support phase adjustment, should these limits
be restricted to output pins?

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