Re: [PATCH net-next v13 12/12] 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: stale INTSYNC state cache on resume error due to missing read-through
--

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

    dpll: sit9531x: model the inter-PLL sync net as a pair of pins
    
This commit models the internal inter-PLL synchronization network in the
sit9531x device as a pair of pins (output on source, input on destination).
It adds logic to detect, configure, and restore the INTSYNC state across
various operations and suspend/resume cycles.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index a744de2c75858..a7b0b2d35cb7c 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -4882,8 +5266,34 @@ static int sit9531x_suspend(struct device *dev)
>  static int sit9531x_resume(struct device *dev)
>  {
>  	struct sit9531x_dev *sitdev = dev_get_drvdata(dev);
> +	int rc;
> +	u8 i;
>  
>  	sit9531x_page_cache_drop(sitdev);
> +
> +	/*
> +	 * A part that lost power across suspend comes back with its
> +	 * profile's INTSYNC routing and output mutes, not with what the
> +	 * caches say, and the poll watches neither.  Read them back here;
> +	 * a cache that cannot be read is left for the getter's read-through.
> +	 * Nothing races this: the poll is parked and the IRQ disabled.
> +	 */
> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_intsync_src_detect(sitdev);
> +	if (rc)
> +		dev_warn(sitdev->dev,
> +			 "INTSYNC source not re-detected on resume: %d\n", rc);

[Severity: Medium]
If sit9531x_intsync_src_detect() fails to read from the device, the code
logs a warning but leaves sitdev->intsync_src holding its pre-suspend value.
Unlike the output mutes right below which set a state_stale flag, is there
anything triggering a subsequent read if this initial resume read fails?

> +	for (i = 0; i < sitdev->info->num_outputs; i++) {
> +		rc = sit9531x_output_state_refresh(sitdev, i);
> +		if (rc) {
> +			dev_warn(sitdev->dev,
> +				 "Output %u mute state not read on resume: %d\n",
> +				 i, rc);
> +			sitdev->out[i].state_stale = true;
> +		}
> +	}
> +	mutex_unlock(&sitdev->multiop_lock);
> +
>  	if (sitdev->irq > 0)
>  		enable_irq(sitdev->irq);

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index dc8d84133b72b..5f530c4a8271d 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c

[ ... ]

> @@ -869,8 +940,325 @@ sit9531x_dpll_output_pin_direction_get(const struct dpll_pin *pin,
> +static int
> +sit9531x_dpll_intsync_src_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;
> +
> +	mutex_lock(&sitdev->multiop_lock);
> +	if (sit9531x_dpll_intsync_src_connected(sitdev, sitdpll))
> +		*state = DPLL_PIN_STATE_CONNECTED;
> +	else
> +		*state = DPLL_PIN_STATE_DISCONNECTED;
> +	mutex_unlock(&sitdev->multiop_lock);

[Severity: Medium]
The comment in sit9531x_resume() states that "a cache that cannot be read
is left for the getter's read-through." However, this getter directly
returns the cached intsync_src value without attempting to read from the
hardware if the cache is stale.

Because the resume path does not invalidate the cache on failure, could this
result in reporting an incorrect, out-of-sync INTSYNC state to user space
and erroneously accepting or rejecting configuration requests?

> +
> +	return 0;
> +}

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