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

[Ivan Vecera]

Hi Kuba,

On 6/4/26 3:51 AM, Jakub Kicinski wrote:
> 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.

Well, any error zl3073x_chan_state_update() are very very unusual and
if they happen (communication error, timeout) then it means that
the device hangs or there is an bus (I2C controller) error.

> 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?

I think the error is more appropriate in this case.

> > +
> > +	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?

The assert is not for serializing accesses to zldpll->dev->*_monitor and
could be moved below this 'if'... Do you want to move it?

Regarding cross-DPLL serialization, IMHO this is an overkill just for
toggles like zldpll->dev->freq_monitor.

Thoughts?

Thanks,
Ivan