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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dpll/sit9531x: partial INTSYNC enable state prevents retry from succeeding
--

commit 04b8069f0eb6eee7c1faf966517e2a726ccd2acc
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

    dpll: sit9531x: model the inter-PLL sync net as a pair of pins
    
    The device has an internal net by which one PLL can drive the others: the
    source PLL puts its output on it, and any other PLL can select it as a
    reference instead of an external input.  The two ends are nothing alike --
    one is driven, the other is selected -- so they are two pins rather than
    one: an output pin on the source and an input pin on each destination.
    
    That keeps each pin honest about what its state means.  The source pin
    reports whether this PLL is the one driving the net, and setting it takes
    the net over or gives it up; a destination pin reports whether its PLL has
    selected the net, and behaves like any other selectable input.  A single
    pin would have had to answer both questions at once and could only have
    been right about one of them.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 45baa703aa45b..6b607f3d5fc31 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> +		} else if (n_en && n_dis &&
> +			   n_en + n_dis == ARRAY_SIZE(intsync_config) &&
> +			   partial < 0) {
> +			/*
> +			 * Every register holds one of the two patterns and both
> +			 * occur: the residue of an enable or a disable that
> +			 * stopped part way, and nothing a profile writes.
> +			 */
> +			partial = pll;
> +		}
> +	}
> +
> +	/*
> +	 * A disable that stopped part way leaves a PLL holding a mix of the
> +	 * two patterns.  Treat it as the owner, so that a repeated disconnect
> +	 * on that PLL runs the disable again and another PLL cannot be
> +	 * enabled on top of the leftover configuration.
> +	 */
> +	if (src < 0 && partial >= 0) {
> +		dev_warn(sitdev->dev,
> +			 "PLL%c holds a partial INTSYNC source configuration\n",
> +			 'A' + partial);
> +		src = partial;
> +	}
> +
> +	sitdev->intsync_src = src;

[Severity: Medium]
Could caching the partially-configured PLL as the active owner of the
INTSYNC net cause issues during a retry?

> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 6bf4efd3c6334..52d5b1a6e36f7 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> +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)
> +{
> +	struct sit9531x_dpll *sitdpll = dpll_priv;
> +	struct sit9531x_dev *sitdev = sitdpll->dev;
> +	int rc = 0, detect_rc = 0;
> +	bool changed = false;
> +	u8 hw_src;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +
> +	switch (state) {
> +	case DPLL_PIN_STATE_CONNECTED:
> +		if (sitdev->intsync_src == sitdpll->id)
> +			break;

[Severity: Medium]
Does this short-circuit condition prevent repairing the partially-configured
hardware?

If a transient bus error during sit9531x_intsync_enable() triggers a rollback
via sit9531x_intsync_disable(), and the rollback also fails, the
INTSYNC_GLOBAL bit is restored and the hardware is left partially configured.

The subsequent call to sit9531x_intsync_src_detect() then caches this PLL as
the net owner (sitdev->intsync_src = partial) as noted above.

When userspace retries the failed connect request, it seems this check in
sit9531x_dpll_intsync_src_state_on_dpll_set() might erroneously assume the
PLL is already fully enabled based on the cached partial state, returning
success without actually repairing the hardware.

> +		if (sitdev->intsync_src >= 0) {
> +			NL_SET_ERR_MSG(extack,
> +				       "INTSYNC is already sourced by another PLL");
> +			rc = -EBUSY;
> +			break;
> +		}

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