Re: [PATCH net-next v15 13/13] dpll: sit9531x: model the inter-PLL sync line as a pair of pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [Medium] dpll: sit9531x: broken rollback in
  __sit9531x_intsync_disable() on write error
- [Medium] dpll: sit9531x: sticky intsync_partial in
  sit9531x_intsync_readback()
- [Medium] dpll: sit9531x: wrong INTSYNC reference check and state on
  manual/partial PLLs
- [Medium] dpll: sit9531x: sit9531x_resume() treats -ENODATA phase read
  as error
- [Medium] dpll: sit9531x: sit9531x_intsync_src_detect() clobbers
  partial state on disconnect failure
- [Low] dpll: sit9531x: lost pin notification when post-disconnect
  detect fails under conflict
--

commit dee9e16728fe17201c2262f079ff7610b8f1bd64
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: model the inter-PLL sync line as a pair of pins

This commit models the internal inter-PLL sync line (INTSYNC) as an output
source pin and an input destination pin on each DPLL, adding EXT-page
source detection, enable/disable sequences, and resume state readback.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 8f305cc27737e..e3ce7b6cad6b9 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> @@ -2585,6 +2604,556 @@ static int sit9531x_clear_notifications(struct sit9531x_dev *sitdev)

[ ... ]

> +int sit9531x_intsync_readback(struct sit9531x_dev *sitdev)
> +{
> +	bool partial = sitdev->intsync_partial;
> +	s8 src = sitdev->intsync_src;
> +	int rc;
> +
> +	rc = sit9531x_intsync_src_detect(sitdev);
> +	if (rc)
> +		return rc;
> +
> +	if (partial && (sitdev->intsync_src < 0 ||
> +			(sitdev->intsync_src == src &&
> +			 !sitdev->intsync_partial))) {
> +		sitdev->intsync_src = src;
> +		sitdev->intsync_partial = true;
> +		sitdev->intsync_foreign = false;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Does sit9531x_intsync_readback() overwrite the freshly detected hardware
state whenever sitdev->intsync_partial was set before the scan?

When sit9531x_resume() calls sit9531x_intsync_readback() after power loss
and a profile reload, a pre-suspend partial state forces intsync_partial
back to true even if the reloaded profile cleanly enabled that PLL (causing
SYNCOUT to be reported as DISCONNECTED and INTSYNC destination pins as
NO_SIGNAL) or cleared all PLLs to the disable pattern (leaving intsync_src
set and blocking other PLLs from enabling SYNCOUT with -EBUSY).

In addition, in sit9531x_dpll_intsync_src_state_on_dpll_set(), when
connecting SYNCOUT fails with detect_rc != 0 or owner_kept set, the error
path sets intsync_src = sitdpll->id and intsync_partial = true without
setting intsync_stale = true, which permanently wedges intsync_src on that
PLL.

[ ... ]

> +static int __sit9531x_intsync_disable(struct sit9531x_dev *sitdev,
> +				      u8 src_pll_idx, bool restore,
> +				      bool *owner_kept, bool *touched)
> +{
> +	bool in_window = false, was_enabled;
> +	u8 ext_page, val;
> +	int rc, lock_rc;
> +	unsigned int i;
> +
> +	lockdep_assert_held(&sitdev->multiop_lock);
> +
> +	if (owner_kept)
> +		*owner_kept = false;
> +	if (touched)
> +		*touched = false;
> +	if (src_pll_idx >= SIT9531X_NUM_PLLS)
> +		return -EINVAL;
> +
> +	ext_page = SIT9531X_PLL_EXT_PAGE(src_pll_idx);
> +
> +	rc = sit9531x_read_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL, &val);
> +	if (rc)
> +		return rc;
> +	was_enabled = val & BIT(SIT9531X_INTSYNC_EN_BIT);
> +	if (touched)
> +		*touched = true;
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL,
> +			       val & ~BIT(SIT9531X_INTSYNC_EN_BIT));
> +	if (rc)
> +		goto restore_global;
> +	/* A sample taken before this write can still show a lock. */
> +	for (i = 0; i < SIT9531X_NUM_PLLS; i++)
> +		if (sitdev->chan[i].routed_ref == SIT9531X_INTSYNC_PIN_ID)
> +			sitdev->chan[i].intsync_dropped = true;
> +
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_GLOBAL_UPDATE,
> +			       SIT9531X_SMALL_UPDATE_CMD);
> +	usleep_range(1000, 2000);
> +	if (rc)
> +		goto restore_global;
> +
> +	rc = sit9531x_write_u8(sitdev,
> +			       SIT9531X_REG(ext_page,
> +					    SIT9531X_PLL_REG_DEBUG),
> +			       SIT9531X_PLL_DEBUG_UNLOCK);
> +	if (rc)
> +		goto restore_global;
> +	in_window = true;
> +
> +	for (i = 0; i < ARRAY_SIZE(intsync_config); i++) {
> +		rc = sit9531x_write_u8(sitdev,
> +				       SIT9531X_REG(ext_page,
> +						    intsync_config[i].offset),
> +				       intsync_config[i].dis_val);
> +		if (rc)
> +			goto restore_global;
> +	}
> +
> +	rc = sit9531x_pll_small_update(sitdev, src_pll_idx);
> +	if (rc && owner_kept)
> +		*owner_kept = true;
> +
> +	goto relock;
> +
> +restore_global:
> +	/*
> +	 * Restore the global enable, or finish the disable if the page holds
> +	 * the disable pattern.
> +	 */
> +	if (restore && in_window) {
> +		bool off;
> +
> +		if (sit9531x_intsync_page_off(sitdev, ext_page, &off)) {
> +			if (owner_kept)
> +				*owner_kept = true;
> +		} else if (off) {
> +			rc = sit9531x_pll_small_update(sitdev, src_pll_idx);
> +			if (rc && owner_kept)
> +				*owner_kept = true;
> +			goto relock;
> +		}
> +	}
> +
> +	if (restore && was_enabled) {
> +		int ret;
> +
> +		ret = sit9531x_read_u8(sitdev, SIT9531X_REG_INTSYNC_GLOBAL,
> +				       &val);
> +		if (!ret)
> +			ret = sit9531x_write_u8(sitdev,
> +						SIT9531X_REG_INTSYNC_GLOBAL,
> +						val |
> +						BIT(SIT9531X_INTSYNC_EN_BIT));
> +		if (!ret) {
> +			/* The page-0 small update applies the global enable */
> +			ret = sit9531x_write_close(sitdev,
> +						   SIT9531X_REG_GLOBAL_UPDATE,
> +						   SIT9531X_SMALL_UPDATE_CMD);
> +			usleep_range(1000, 2000);
> +		}
> +		if (ret) {
> +			dev_warn(sitdev->dev,
> +				 "PLL%c: INTSYNC enable not put back after a failed disable: %d\n",
> +				 'A' + src_pll_idx, ret);
> +			if (owner_kept)
> +				*owner_kept = true;
> +		}
> +	}

[Severity: Medium]
If an I2C write fails partway through the intsync_config[] loop, does
restore_global re-enable SIT9531X_REG_INTSYNC_GLOBAL while leaving the
partially written dis_val entries in the EXT page and chan->intsync_dropped
set?

Because sit9531x_pll_small_update() was not reached, the hardware still
drives the line with the previously latched enable pattern (until a later
PLL small update latches the half-disabled registers under an active global
enable), but sit9531x_intsync_src_detect() sees a mix of en_val and dis_val
in the unlatched EXT page and sets sitdev->intsync_partial = true, reporting
SYNCOUT as DISCONNECTED and INTSYNC destination pins as NO_SIGNAL.

Conversely, when was_enabled is false (such as when disconnecting a partial
source whose EXT page already holds unlatched dis_val entries) and a write
fails before in_window is set, *owner_kept remains false and
sit9531x_intsync_src_detect() clears sitdev->intsync_src to -1 even though
the disable pattern was never latched.

[ ... ]

> @@ -4420,12 +5000,128 @@ static int sit9531x_suspend(struct device *dev)
>  static int sit9531x_resume(struct device *dev)
>  {

[ ... ]

> +	for (i = 0; i < sitdev->info->num_outputs; i++) {
> +		struct sit9531x_out *out = &sitdev->out[i];
> +		bool beyond;
> +		s64 phase_ps;
> +
> +		if (!out->routed)
> +			continue;
> +		rc = sit9531x_output_phase_read(sitdev, i, &phase_ps,
> +						&beyond);
> +		if (rc) {
> +			dev_warn(sitdev->dev,
> +				 "Output %u delay not read on resume: %d\n",
> +				 i, rc);
> +			out->phase_stale = true;
> +			outs_unread |= BIT(i);
> +			continue;
> +		}

[Severity: Medium]
Should sit9531x_resume() ignore -ENODATA from sit9531x_output_phase_read()
the way sit9531x_dev_state_fetch() does at probe?

When an output's PLL has an unprogrammed divider or out-of-range VCO,
sit9531x_output_phase_read() returns -ENODATA.  Setting out->phase_stale =
true on -ENODATA causes sit9531x_dpll_output_pin_phase_adjust_get() to fail
with -ENODATA and sit9531x_dpll_output_ntf_pending() to return false on
every poll tick, permanently blocking change notifications for that output
pin.

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index ba4ac5e7d365d..1bd2b962cdabe 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> @@ -662,6 +689,325 @@ sit9531x_dpll_output_pin_direction_get(const struct dpll_pin *pin,

[ ... ]

> +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)
> +{

[ ... ]

> +		hw_src = sit9531x_input_hw_src(SIT9531X_INTSYNC_PIN_ID);
> +		if (sit9531x_input_prio_present(sitdev, sitdpll->id, hw_src)) {
> +			NL_SET_ERR_MSG(extack,
> +				       "PLL selects INTSYNC as a reference; it cannot drive it");
> +			rc = -EBUSY;
> +			break;
> +		}
> +		rc = sit9531x_intsync_enable(sitdev, sitdpll->id, &owner_kept,
> +					     &changed);
> +		break;
> +	case DPLL_PIN_STATE_DISCONNECTED:
> +		if (sitdev->intsync_src != sitdpll->id)
> +			break;
> +		rc = sit9531x_intsync_disable(sitdev, sitdpll->id,
> +					      &owner_kept, &changed);
> +		break;
> +	default:
> +		rc = -EINVAL;
> +		break;
> +	}
> +
> +	if (changed && !rc) {
> +		sitdev->intsync_src = state == DPLL_PIN_STATE_CONNECTED ?
> +				      sitdpll->id : -1;
> +		sitdev->intsync_partial = false;
> +	}
> +
> +	if (changed)
> +		detect_rc = sit9531x_intsync_src_detect(sitdev);
> +	if (changed && rc) {
> +		if (owner_kept ||
> +		    (state == DPLL_PIN_STATE_CONNECTED &&
> +		     (detect_rc || sitdev->intsync_src == sitdpll->id))) {
> +			sitdev->intsync_src = sitdpll->id;
> +			sitdev->intsync_partial = true;
> +			sitdev->intsync_foreign = false;
> +		} else if (detect_rc) {
> +			sitdev->intsync_stale = true;
> +		}
> +	} else if (changed && detect_rc && sitdev->intsync_conflict) {
> +		sitdev->intsync_stale = true;
> +	}

[Severity: Medium]
Does calling sit9531x_intsync_src_detect() instead of
sit9531x_intsync_readback() here lose the recorded intsync_partial state
when a disconnect fails before modifying the EXT page?

If a PLL has sitdev->intsync_partial == true because an earlier enable left
en_val in the EXT page (with the global enable set) without latching via
sit9531x_pll_small_update(), and a subsequent DPLL_PIN_STATE_DISCONNECTED
request fails before unlocking the EXT page while restore_global restores
SIT9531X_REG_INTSYNC_GLOBAL (so owner_kept is false),
sit9531x_intsync_src_detect() sees all en_val with global enable set and
clears sitdev->intsync_partial to false.  The failed disconnect then flips
the pin state from DISCONNECTED to CONNECTED and sends a change
notification.

[Severity: Low]
When disconnecting a PLL under sitdev->intsync_conflict succeeds (changed &&
!rc) but the post-write sit9531x_intsync_src_detect() fails, can the pin
change notification be lost?

The changed && !rc block clears sitdev->intsync_src to -1 before
sitdev->intsync_stale = true is set.  If the immediate getter refresh inside
__dpll_pin_change_ntf() also fails, the later periodic retry in
sit9531x_intsync_refresh() sees sit9531x_intsync_owner() == -1 both before
and after sit9531x_intsync_readback() and never sets
sitdev->intsync_ntf_pending.

[ ... ]

> +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;
> +	int rc;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_dpll_intsync_refresh(sitdev, extack);
> +	if (rc) {
> +		mutex_unlock(&sitdev->multiop_lock);
> +		return rc;
> +	}
> +	if (sitdev->intsync_src == sitdpll->id)
> +		*state = DPLL_PIN_STATE_DISCONNECTED;
> +	else
> +		sit9531x_dpll_selection_state_get(sitdev, sitdpll,
> +						  SIT9531X_INTSYNC_PIN_ID,
> +						  state);
> +	mutex_unlock(&sitdev->multiop_lock);
> +
> +	return 0;
> +}

[Severity: Medium]
Does checking sit9531x_input_prio_present() in
sit9531x_dpll_intsync_src_state_on_dpll_set() work when the PLL uses manual
input selection, or when sitdev->intsync_partial is set?

On a manual-input PLL (chan->manual_input), reference selection uses
chan->manual_ref / chan->routed_ref rather than chan->prio_mask, and
sit9531x_dpll_intsync_dst_pin_manual_ops has no .state_on_dpll_set callback.
Checking only chan->prio_mask allows a manual PLL routing INTSYNC to drive
the line it locks to, while permanently blocking a manual PLL routing
another input if its unused priority table happens to list
SIT9531X_PRIO_SRC_INTSYNC.

Also, sit9531x_dpll_intsync_dst_state_on_dpll_get() and
sit9531x_dpll_pin_snapshot() report the INTSYNC input pin as DISCONNECTED
whenever sitdev->intsync_src == sitdpll->id, even when
sitdev->intsync_partial is true (so SYNCOUT is also DISCONNECTED).  If the
priority table still contains INTSYNC, enabling SYNCOUT fails with -EBUSY
("PLL selects INTSYNC as a reference; it cannot drive it") while the INTSYNC
input pin is reported as DISCONNECTED.

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