Re: [PATCH v10 09/14] dpll: sit9531x: implement output pin state on a DPLL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dpll: lockless data race in sit9531x_dpll_output_pin_state_on_dpll_get()
--

commit 52fe1edf88f74a955c8a6a22cda5f2c143e2d6c3
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: implement output pin state on a DPLL

This commit implements the output pin state logic for the sit9531x DPLL driver.
It allows reading and writing the connected or disconnected state (muting)
of the pins by controlling the per-output force and state bits.

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 9f0678ddfe0ee..c1ab202bf2978 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -783,10 +783,97 @@ sit9531x_dpll_output_pin_frequency_set(const struct dpll_pin *pin,

[ ... ]

> +static int
> +sit9531x_dpll_output_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_out *out;
> +	int rc;
> +
> +	/*
> +	 * A mute whose read-back failed left the cache unconfirmed; there is
> +	 * no poll of output state to correct it, so read it here rather than
> +	 * report a value that may predate the request.
> +	 */
> +	if (sitdev->out[dpin->id].state_stale) {
> +		mutex_lock(&sitdev->multiop_lock);
> +		rc = sit9531x_output_state_refresh(sitdev, dpin->id);
> +		mutex_unlock(&sitdev->multiop_lock);
> +		if (rc) {
> +			NL_SET_ERR_MSG(extack,
> +				       "Output mute state could not be read back");
> +			return rc;
> +		}
> +	}
> +
> +	out = sit9531x_out_state_get(sitdev, dpin->id);
> +	*state = out->enabled ? DPLL_PIN_STATE_CONNECTED
> +			      : DPLL_PIN_STATE_DISCONNECTED;

[Severity: Medium]
Is there a data race here against concurrent state changes?

`state_stale` and `out->enabled` are checked and read without holding
`multiop_lock`. If `sit9531x_dpll_output_pin_state_on_dpll_set()` is
running concurrently via another netlink command, could this getter read
an intermediate or stale software state for `enabled` and miss concurrent
`state_stale` updates?

This might lead to an incorrect pin state being reported to userspace.

> +	return 0;
> +}

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