Re: [PATCH net-next v3 2/2] dpll: zl3073x: add NCO virtual input pin

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "I think the report / question about the SEM wasn't asked
on v2?"
---
dpll: zl3073x: add NCO virtual input pin

This change adds a virtual NCO input pin that lets userspace switch a
DPLL channel into NCO mode, and registers it with the new
DPLL_PIN_TYPE_INT_NCO type. It also rearranges chan->df_offset
handling so it is only read when the DPLL is locked, and adds a
ZL_REG_DPLL_CTRL register cache plus state_set support.

> diff --git a/drivers/dpll/zl3073x/chan.c b/drivers/dpll/zl3073x/chan.c
> index 2fe3c3da84bb..f5d6c7917e57 100644
> --- a/drivers/dpll/zl3073x/chan.c
> +++ b/drivers/dpll/zl3073x/chan.c
> @@ -21,6 +21,11 @@ int zl3073x_chan_state_update(struct zl3073x_dev *zldev, u8 index)
>  	u64 val;
>  	int rc;
>  
> +	/* Serialize with zl3073x_chan_nco_mode_set() which also
> +	 * modifies chan->mode_refsel and chan->df_offset.
> +	 */
> +	guard(mutex)(&zldev->multiop_lock);
> +
>  	rc = zl3073x_read_u8(zldev, ZL_REG_DPLL_MON_STATUS(index),
>  			     &chan->mon_status);
>  	if (rc)
> @@ -31,7 +36,10 @@ int zl3073x_chan_state_update(struct zl3073x_dev *zldev, u8 index)
>  	if (rc)
>  		return rc;
>  
> -	/* Read df_offset vs tracked reference */
> +	/* Read df_offset only when locked to a reference */
> +	if (zl3073x_chan_lock_state_get(chan) != ZL_DPLL_MON_STATUS_STATE_LOCK)
> +		return 0;
> +

[Medium]
With this early return, when the DPLL boots in NCO mode and
zl3073x_chan_state_fetch() calls zl3073x_chan_state_update(),
chan->df_offset is left at the kzalloc-zeroed 0 (NCO mode is not
ZL_DPLL_MON_STATUS_STATE_LOCK).

zl3073x_dpll_nco_pin_register() then calls zl3073x_chan_state_set() to
update the ctrl bits, but state_set() never reads
ZL_REG_DPLL_DF_OFFSET. The only place that populates df_offset in NCO
mode is zl3073x_chan_nco_mode_set(), which only runs on a userspace
CONNECTED transition for the NCO pin.

Does this leave zl3073x_dpll_nco_pin_ffo_get() reporting ffo=0 forever
on a system that boots already in NCO mode, until userspace explicitly
disconnects and reconnects the NCO pin?

The commit message says:

    The value is static for the duration of NCO mode and serves as the
    FFO snapshot reported to userspace.

Does that statement still hold for the boot-in-NCO-mode case?

>  	rc = zl3073x_poll_zero_u8(zldev, ZL_REG_DPLL_DF_READ(index),
>  				  ZL_DPLL_DF_READ_SEM);
>  	if (rc)
> @@ -56,6 +64,50 @@ int zl3073x_chan_state_update(struct zl3073x_dev *zldev, u8 index)
>  	return 0;
>  }
>  
> +/**
> + * zl3073x_chan_nco_mode_set - switch DPLL channel to NCO mode
> + * @zldev: pointer to zl3073x_dev structure
> + * @index: DPLL channel index
> + *
> + * Switches the channel to NCO mode, waits for the hardware to
> + * auto-capture the tracking offset via nco_auto_read, then reads
> + * the captured df_offset directly from the register.
> + *
> + * Return: 0 on success, <0 on error
> + */
> +int zl3073x_chan_nco_mode_set(struct zl3073x_dev *zldev, u8 index)
> +{
> +	struct zl3073x_chan *chan = &zldev->chan[index];
> +	u8 mode_refsel;
> +	u64 val;
> +	int rc;
> +
> +	/* Serialize with zl3073x_chan_state_update() which also
> +	 * reads chan->df_offset from the same register.
> +	 */
> +	guard(mutex)(&zldev->multiop_lock);
> +
> +	mode_refsel = chan->mode_refsel;
> +	FIELD_MODIFY(ZL_DPLL_MODE_REFSEL_MODE, &mode_refsel,
> +		     ZL_DPLL_MODE_REFSEL_MODE_NCO);
> +
> +	rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MODE_REFSEL(index),
> +			      mode_refsel);
> +	if (rc)
> +		return rc;
> +
> +	chan->mode_refsel = mode_refsel;
> +
> +	/* Best-effort read of df_offset captured by nco_auto_read.
> +	 * Mode switch already succeeded, so don't propagate a
> +	 * df_offset read failure back to userspace.
> +	 */
> +	rc = zl3073x_read_u48(zldev, ZL_REG_DPLL_DF_OFFSET(index), &val);
> +	chan->df_offset = !rc ? sign_extend64(val, 47) : 0;
> +
> +	return 0;
> +}

[Medium]
Should this read use the same DF_READ semaphore handshake that
zl3073x_chan_state_update() uses for the same DF_OFFSET register?

The existing path does:

    poll ZL_DPLL_DF_READ_SEM == 0
    write ZL_DPLL_DF_READ_SEM | ZL_DPLL_DF_READ_REF_OFST
    poll ZL_DPLL_DF_READ_SEM == 0
    read ZL_REG_DPLL_DF_OFFSET

The new path writes MODE_REFSEL=NCO and then immediately reads the
48-bit DF_OFFSET register without any handshake.

The commit message says:

    The nco_auto_read populates df_offset before the mode switch
    completes so no delay is needed.

Is there a datasheet reference for this guarantee that could be cited
in the code comment?

If the auto-capture takes any non-zero amount of time after the
MODE_REFSEL bus write completes, can this read return stale REFLOCK-era
data or a torn value across the multi-byte 48-bit register? Combined
with the early return added to zl3073x_chan_state_update() for non-LOCK
states, would such a value persist for the entire NCO session?

[Medium]
There is also a question about reader synchronization for chan->df_offset.

zl3073x_chan_nco_mode_set() writes chan->df_offset under
zldev->multiop_lock. The periodic worker zl3073x_dpll_changes_check()
calls zl3073x_dpll_pin_ffo_check(), which reads chan->df_offset via
zl3073x_chan_df_offset_get() without holding multiop_lock:

static inline s64
zl3073x_chan_df_offset_get(const struct zl3073x_chan *chan)
{
    return chan->df_offset;
}

Consider: DPLL is in REFLOCK with a pin locked, the worker has finished
zl3073x_chan_state_update() (released multiop_lock), is iterating pins,
and enters ffo_check just as a netlink connect of the NCO pin calls
zl3073x_chan_nco_mode_set() and overwrites chan->df_offset.

drivers/dpll/zl3073x/Kconfig has no 64BIT dependency, so on 32-bit
builds the s64 store is two 32-bit stores. Can the worker observe a
torn value, push it through atomic64_xchg into pin->freq_offset, and
emit a dpll_pin_change_ntf with that value?

Pre-patch, df_offset was only written from state_update() in the worker
context, so writer and reader were sequential. Should the ffo_check
reader take multiop_lock, or chan->df_offset be promoted to an
atomic64_t/READ_ONCE pair?

> +
>  /**
>   * zl3073x_chan_state_fetch - fetch DPLL channel state from hardware
>   * @zldev: pointer to zl3073x_dev structure

[ ... ]