Re: [net-next,5/6] dpll: zl3073x: add reference priority to zl3073x_chan

[Ivan Vecera]

On 3/14/26 8:53 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email says "Is this legit? Quite hard to follow the logic of this change :/"
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> dpll: zl3073x: add reference priority to zl3073x_chan
>
> This commit caches the ZL_REG_DPLL_REF_PRIO registers in the zl3073x_chan
> cfg group, adding a ref_prio[] array and inline helpers for accessing
> priority values. It extends state_fetch and state_set with DPLL mailbox
> operations to read and write these priority registers.
>
> > diff --git a/drivers/dpll/zl3073x/chan.c b/drivers/dpll/zl3073x/chan.c
> > index 10189c8a5ded..74d00cee624e 100644
> > --- a/drivers/dpll/zl3073x/chan.c
> > +++ b/drivers/dpll/zl3073x/chan.c
>
> [ ... ]
>
> > @@ -85,7 +102,9 @@ const struct zl3073x_chan *zl3073x_chan_state_get(struct zl3073x_dev *zldev,
> >    * @chan: desired channel state
> >    *
> >    * Skips the HW write if the configuration is unchanged, and otherwise
> > - * writes the mode_refsel register to hardware.
> > + * writes only the changed registers to hardware. The mode_refsel register
> > + * is written directly, while the reference priority registers are written
> > + * via the DPLL mailbox interface.
> >    *
> >    * Return: 0 on success, <0 on HW error
> >    */
> > @@ -93,14 +112,48 @@ int zl3073x_chan_state_set(struct zl3073x_dev *zldev, u8 index,
> >   			   const struct zl3073x_chan *chan)
> >   {
> >   	struct zl3073x_chan *dchan = &zldev->chan[index];
> > -	int rc;
> > +	int rc, i;
> >
> >   	/* Skip HW write if configuration hasn't changed */
> >   	if (!memcmp(&dchan->cfg, &chan->cfg, sizeof(chan->cfg)))
> >   		return 0;
> >
> > -	rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MODE_REFSEL(index),
> > -			      chan->mode_refsel);
> > +	/* Direct register write for mode_refsel */
> > +	if (dchan->mode_refsel != chan->mode_refsel) {
> > +		rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MODE_REFSEL(index),
> > +				      chan->mode_refsel);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> > +	/* Mailbox write for ref_prio if changed */
> > +	if (!memcmp(dchan->ref_prio, chan->ref_prio, sizeof(chan->ref_prio))) {
> > +		dchan->cfg = chan->cfg;
> > +		return 0;
> > +	}
> > +
> > +	guard(mutex)(&zldev->multiop_lock);
> > +
> > +	/* Read DPLL configuration into mailbox */
> > +	rc = zl3073x_mb_op(zldev, ZL_REG_DPLL_MB_SEM, ZL_DPLL_MB_SEM_RD,
> > +			   ZL_REG_DPLL_MB_MASK, BIT(index));
> > +	if (rc)
> > +		return rc;
> > +
> > +	/* Update changed ref_prio registers */
> > +	for (i = 0; i < ARRAY_SIZE(chan->ref_prio); i++) {
> > +		if (dchan->ref_prio[i] != chan->ref_prio[i]) {
> > +			rc = zl3073x_write_u8(zldev,
> > +					      ZL_REG_DPLL_REF_PRIO(i),
> > +					      chan->ref_prio[i]);
> > +			if (rc)
> > +				return rc;
> > +		}
> > +	}
> > +
> > +	/* Commit DPLL configuration */
> > +	rc = zl3073x_mb_op(zldev, ZL_REG_DPLL_MB_SEM, ZL_DPLL_MB_SEM_WR,
> > +			   ZL_REG_DPLL_MB_MASK, BIT(index));
> >   	if (rc)
> >   		return rc;
>
> Could this function leave the cache and hardware out of sync?
>
> When both mode_refsel and ref_prio have changed, the function writes
> mode_refsel to hardware first. If the subsequent mailbox operation fails
> (during the mailbox read, a ref_prio write, or the mailbox commit), the
> function returns an error without executing 'dchan->cfg = chan->cfg'.

This potential situation is very unusual (mode_refsel updated and
mailbox access erroneous) but yes, after successful write of mode_refsel
dchan->mon_refsel should be updated to avoid this issue.

Will fix in v2.

Ivan