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

[Jakub Kicinski]

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 leaves dchan->mode_refsel cached at the old value while the hardware
has the new value:

zl3073x_chan_state_set()
    dchan->mode_refsel != chan->mode_refsel is true
    zl3073x_write_u8(... mode_refsel) succeeds
        hardware now has new mode_refsel value
    zl3073x_mb_op(...READ...) fails
    return rc without updating dchan->cfg
        dchan->mode_refsel still has old value

Later calls that get state via zl3073x_chan_state_get() will see the stale
cached mode_refsel. If they try to set mode_refsel back to the old value,
the comparison 'dchan->mode_refsel != chan->mode_refsel' evaluates to false
(both match the old value), skipping the write, while hardware retains the
new value.

Before this patch, mode_refsel was written in a single atomic step with no
possibility of partial failure, so this desynchronization scenario couldn't
occur.