Re: [PATCH net-next v8 14/15] 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] The internal software tracking variable `sitdev->intsync_src` is unconditionally initialized to `-1` without reading the hardware's actual INTSYNC configuration during driver probe, which can lead to hardware contention.
--

commit 484ecadc39855015e8fd1f1cdcf1c007689b79ce
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.

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 1a14255e89a8f..993a991d5b259 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> @@ -717,8 +731,214 @@ sit9531x_dpll_output_pin_direction_get(const struct dpll_pin *pin,
>  				       enum dpll_pin_direction *direction,
>  				       struct netlink_ext_ack *extack);
>  
[ ... ]
> +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;
> +	u8 prio;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +
> +	switch (state) {
> +	case DPLL_PIN_STATE_CONNECTED:
> +		if (sitdev->intsync_src == sitdpll->id)
> +			break;
> +		if (sitdev->intsync_src >= 0) {

[Severity: Medium]
Will this properly protect against a regression causing hardware contention
on the internal INTSYNC net if it is already active during boot?

Since the driver preserves pre-boot NVM state (by explicitly keeping the
reset GPIO deasserted), the hardware might already have a PLL driving
INTSYNC. However, during probe sitdev->intsync_src is unconditionally
initialized to -1:

drivers/dpll/sit9531x/core.c:sit9531x_dev_probe() {
    ...
	sitdev->clock_id = sit9531x_derive_clock_id(sitdev);
	sitdev->intsync_src = -1;
    ...
}

And sit9531x_dev_state_fetch() does not read the SIT9531X_REG_INTSYNC_GLOBAL
register to update it with the actual hardware state. 

This means sitdev->intsync_src will remain -1 even if INTSYNC is being
driven. Can a user then successfully command another PLL to connect to
INTSYNC, bypassing this safety check and causing a regression where
multiple PLLs contend on the internal hardware net?

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

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