Re: [PATCH net-next v5 3/4] dpll: zl3073x: add per-DPLL serialization lock

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "While we wait for more acks - claude says.."
---
dpll: zl3073x: add per-DPLL serialization lock

This patch adds a per-DPLL mutex serializing netlink callbacks and the
periodic worker, moves zl3073x_chan_state_update() under the new lock,
defers change notifications outside the lock to avoid ABBA with
dpll_lock, and converts pin->freq_offset from atomic64_t to plain s64.

> diff --git a/drivers/dpll/zl3073x/core.c b/drivers/dpll/zl3073x/core.c
> index 0b2050aa2ed9..27c71807e4ef 100644
> --- a/drivers/dpll/zl3073x/core.c
> +++ b/drivers/dpll/zl3073x/core.c
> @@ -567,19 +567,7 @@ zl3073x_dev_ref_states_update(struct zl3073x_dev *zldev)
>  	}
>  }
>  
> -static void
> -zl3073x_dev_chan_states_update(struct zl3073x_dev *zldev)
> -{
> -	int i, rc;
>  
> -	for (i = 0; i < zldev->info->num_channels; i++) {
> -		rc = zl3073x_chan_state_update(zldev, i);
> -		if (rc)
> -			dev_warn(zldev->dev,
> -				 "Failed to get DPLL%u state: %pe\n", i,
> -				 ERR_PTR(rc));
> -	}
> -}

[ ... ]

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 5e58ded5734d..4bee3d0c4659 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -1817,27 +1906,37 @@ zl3073x_dpll_pin_measured_freq_check(struct zl3073x_dpll_pin *pin)
>  void
>  zl3073x_dpll_changes_check(struct zl3073x_dpll *zldpll)
>  {
> +	DECLARE_BITMAP(changed_pins, ZL3073X_NUM_INPUT_PINS);
>  	struct zl3073x_dev *zldev = zldpll->dev;
>  	enum dpll_lock_status lock_status;
>  	struct device *dev = zldev->dev;
>  	struct zl3073x_dpll_pin *pin;
> +	bool dev_changed = false;
>  	int rc;
>  
> +	bitmap_zero(changed_pins, ZL3073X_NUM_INPUT_PINS);
> +
> +	mutex_lock(&zldpll->lock);
> +
>  	zldpll->check_count++;
>  
> -	/* Get current lock status for the DPLL */
> -	rc = zl3073x_dpll_lock_status_get(zldpll->dpll_dev, zldpll,
> -					  &lock_status, NULL, NULL);
> +	rc = zl3073x_chan_state_update(zldev, zldpll->id);
> +	if (rc) {
> +		dev_err(dev, "Failed to get DPLL%u state: %pe\n",
> +			zldpll->id, ERR_PTR(rc));
> +		goto unlock;
> +	}

[Low]
The previous code path, zl3073x_dev_chan_states_update() in core.c, used
dev_warn() and continued the periodic update for the device, allowing the
subsequent lock-status, phase-offset and per-pin checks to run for every
DPLL even when an individual chan_state_update() failed.

After this change, a single failure of zl3073x_chan_state_update() inside
zl3073x_dpll_changes_check() is logged at dev_err and triggers
goto unlock, skipping all remaining work for that DPLL in that cycle:

	rc = zl3073x_chan_state_update(zldev, zldpll->id);
	if (rc) {
		dev_err(dev, "Failed to get DPLL%u state: %pe\n",
			zldpll->id, ERR_PTR(rc));
		goto unlock;
	}

The commit message describes the move only as a serialization change so
that it "runs under zldpll->lock". Should the log-level promotion from warn
to err and the new short-circuit of the rest of the cycle on transient
register-read failures be mentioned in the commit message, or alternatively
preserved as warn-and-continue?

> +
> +	rc = __zl3073x_dpll_lock_status_get(zldpll, &lock_status);
>  	if (rc) {
>  		dev_err(dev, "Failed to get DPLL%u lock status: %pe\n",
>  			zldpll->id, ERR_PTR(rc));
> -		return;
> +		goto unlock;
>  	}

[ ... ]

>  }

The commit message states:

    All DPLL pin and device callbacks that access mutable state take the
    lock as the first operation.

[Low]
Does this hold for zl3073x_dpll_freq_monitor_get() and
zl3073x_dpll_freq_monitor_set()? Both are device callbacks that read or
write zldpll->dev->freq_monitor, but neither takes zldpll->lock:

	static int
	zl3073x_dpll_freq_monitor_get(const struct dpll_device *dpll, ...)
	{
		struct zl3073x_dpll *zldpll = dpll_priv;

		if (zldpll->dev->freq_monitor)
			*state = DPLL_FEATURE_STATE_ENABLE;
		else
			*state = DPLL_FEATURE_STATE_DISABLE;

		return 0;
	}

	static int
	zl3073x_dpll_freq_monitor_set(const struct dpll_device *dpll, ...)
	{
		struct zl3073x_dpll *item, *zldpll = dpll_priv;

		zldpll->dev->freq_monitor = (state == DPLL_FEATURE_STATE_ENABLE);
		...
	}

The same field is read in zl3073x_dpll_pin_measured_freq_check() under
lockdep_assert_held(&zldpll->lock):

	static bool
	zl3073x_dpll_pin_measured_freq_check(struct zl3073x_dpll_pin *pin)
	{
		...
		lockdep_assert_held(&zldpll->lock);

		if (!zldpll->dev->freq_monitor)
			return false;
		...
	}

freq_monitor is a per-device bool shared by all channels, so the per-DPLL
lock would not provide cross-DPLL serialization for it anyway. Should the
commit message carve out an exception for this per-device field, or should
some appropriate lock be taken in the freq_monitor get/set callbacks to
match the stated invariant?