Re: [PATCH net-next 6/6] dpll: zl3073x: add PTP periodic output support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In zl3073x_dpll_ptp_register()
  (drivers/dpll/zl3073x/dpll.c:3045-3054), any perout-capable output pin…
- [Medium] zl3073x_dpll_perout_enable()
  (drivers/dpll/zl3073x/dpll.c:2887-2904) checks only perout->period ==…
- [Medium] For single-ended outputs that are not N-divided, both pins of
  the pair (OUTxP/OUTxN) share the div and width registers.
- [Medium] zl3073x_dpll_ptp_enable() passes zldpll->ptp_clock to
  ptp_find_pin() (dpll.c:2959).
- [Low] zl3073x_dpll_perout_enable() writes the new 1 Hz divider to
  hardware first (zl3073x_out_state_set() at dpll.c:2897, which also…
- [Low] zl3073x_dpll_ptp_register() reads the zldev->out[] cache through
  zl3073x_dev_output_pin_state_get() (dpll.c:3049) without zldpll->lock,…
- [Low] On a non-N-divided output, zl3073x_dpll_perout_enable() relies on
  zl3073x_dpll_output_pin_freq_set(), which only rewrites div and width…

--

dpll: zl3073x: add PTP periodic output support

This commit registers a PTP periodic output pin for each zl3073x DPLL
output pin that supports step-time and declares 1 Hz support in firmware.
Enabling a perout channel programs the assigned pin for 1 PPS and
connects it. Disabling the channel disconnects the pin.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index 0a36a2acf15b..f406d1e72530 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c

[ ... ]

> @@ -2823,6 +2840,159 @@ zl3073x_dpll_ptp_getmaxphase(struct ptp_clock_info *info __always_unused)

[ ... ]

> +static int
> +zl3073x_dpll_perout_enable(struct zl3073x_dpll_pin *pin,
> +			   struct ptp_perout_request *perout)
> +{
> +	u8 out_id = zl3073x_output_pin_out_get(pin->id);
> +	struct zl3073x_dev *zldev = pin->dpll->dev;
> +	struct zl3073x_out out;
> +	int rc;
> +
> +	/* Only 1 PPS (1 Hz) periodic output is supported */
> +	if (perout->period.sec != 1 || perout->period.nsec)
> +		return -EINVAL;

[Severity: Medium]
Should perout->start be checked here too?

supported_perout_flags is left at 0, so ptp_perout_request() rejects
PTP_PEROUT_PHASE. That means every request reaching this driver uses the
absolute start time semantics from include/uapi/linux/ptp_clock.h:

	/*
	 * Absolute start time.
	 * Valid only if (flags & PTP_PEROUT_PHASE) is unset.
	 */
	struct ptp_clock_time start;

start is never read. The divider is programmed, the pin is connected
right away, and success is returned. A request with a future start time
or a non-zero start.nsec is accepted, but the edge is not aligned to it.
If the pin is already connected, the only hardware change is a divider
rewrite on the running output.

Could requests whose start time cannot be honoured be rejected? At a
minimum, could the commit message mention this limitation?

> +
> +	out = *zl3073x_out_state_get(zldev, out_id);
> +
> +	rc = zl3073x_dpll_output_pin_freq_set(pin, &out, 1);

[Severity: Medium]
On single-ended outputs that are not N-divided, both pins of the pair
share div and width. zl3073x_dpll_output_pin_freq_set() just overwrites
them in that case:

	if (!zl3073x_out_is_ndiv(out)) {
		...
		out->div = new_div;
		...
		out->width = new_div;

		return 0;
	}

OUTxP and OUTxN can both be advertised as independent PTP perout pins.
Enabling a channel on OUT5P would therefore also retune OUT5N to 1 Hz,
even if OUT5N is connected and driving something like 10 MHz. The
sibling dpll_pin_change_ntf() only reports this after the fact.

Should this path refuse when the sibling is connected at a different
frequency?

zl3073x_dpll_perout_disable() also only disconnects the pin. It does not
restore the previous divider, so a sibling that is still connected stays
at 1 Hz after the PTP channel is turned off. Is that intended?

[Severity: Low]
On a non-N-divided output, the same helper leaves the output mode clock
type and esync_n_period/esync_n_width unchanged. If the output was in
ZL_OUTPUT_MODE_CLOCK_TYPE_ESYNC, does it stay in ESYNC mode after the
switch to 1 Hz? esync_n_period and esync_n_width would still be the
values computed for the old carrier.

Once out_freq is 1, zl3073x_dpll_output_pin_esync_get() hides this
state:

	if (!(pin->caps & ZL3073X_DPLL_PIN_CAP_ESYNC) || out_freq <= 1)
		return -EOPNOTSUPP;

Userspace can then no longer see or clear the ESYNC mode that is still
set in hardware.

Should the perout path force CLOCK_TYPE_NORMAL, or reject outputs that
are in ESYNC mode? The existing DPLL frequency_set path appears to have
the same gap.

> +	if (rc)
> +		return rc;
> +
> +	rc = zl3073x_out_state_set(zldev, out_id, &out);
> +	if (rc)
> +		return rc;
> +
> +	if (zl3073x_dev_output_pin_state_get(zldev, pin->id))
> +		return 0;
> +
> +	return zl3073x_dev_output_pin_state_set(zldev, pin->id, true);

[Severity: Low]
What happens if zl3073x_dev_output_pin_state_set() fails here? By then
the new 1 Hz divider has already been committed by the
zl3073x_out_state_set() call above, and the cache has been updated.

The connect sequence can fail early. For example, the first stop write
in zl3073x_dev_output_pin_state_set() returns immediately:

	zl3073x_out_stop(&out);
	rc = zl3073x_out_state_set(zldev, out_id, &out);
	if (rc)
		return rc;

The old div/width and esync_n_period/width are not restored. The output
is left at 1 Hz and disconnected, and so is the sibling for non-N-div
formats.

zl3073x_dpll_ptp_enable() then returns before either notification:

	if (rc)
		return rc;

zl3073x_dpll_changes_check() only sends notifications for input pins.
Would DPLL userspace ever learn that the frequency of this pin or its
sibling changed?

> +}

[ ... ]

> +static int zl3073x_dpll_ptp_enable(struct ptp_clock_info *info,
> +				   struct ptp_clock_request *rq, int on)
> +{

[ ... ]

> +	pin_idx = ptp_find_pin(zldpll->ptp_clock, PTP_PF_PEROUT,
> +			       rq->perout.index);

[Severity: Medium]
Can zldpll->ptp_clock still be NULL here? zl3073x_dpll_ptp_register()
only assigns it after ptp_clock_register() returns:

	ptp_clock = ptp_clock_register(&zldpll->ptp_info, zldev->dev);
	...
	zldpll->ptp_clock = ptp_clock;

Inside ptp_clock_register(), posix_clock_register() has already made
/dev/ptpN and the sysfs pin and period attributes live. Any of these,
issued in that window, would reach this call with a NULL clock:

  - PTP_PEROUT_REQUEST
  - PTP_PIN_SETFUNC, via ptp_set_pinfunc()->ptp_disable_pinfunc()->
    enable(on=0), which is reachable because pins can be seeded as
    PEROUT at probe
  - a write to the sysfs period attribute

ptp_find_pin() dereferences ptp->info->n_pins without a NULL check.

The callback already receives info. Could it scan info->pin_config
directly rather than depend on the pointer that is published later?

> +	if (pin_idx < 0)
> +		return -EINVAL;

[ ... ]

> @@ -2843,16 +3015,53 @@ static const struct ptp_clock_info zl3073x_dpll_ptp_clock_info = {
>  static int zl3073x_dpll_ptp_register(struct zl3073x_dpll *zldpll)
>  {

[ ... ]

> +	i = 0;
> +	for_each_set_bit(id, zldpll->perout_map, ZL3073X_NUM_OUTPUT_PINS) {
> +		pin = zl3073x_dpll_output_pin_get_by_id(zldpll, id);
> +		strscpy(pin_config[i].name, pin->label);
> +		pin_config[i].index = i;
> +		if (zl3073x_dev_output_pin_state_get(zldev, id)) {
> +			pin_config[i].func = PTP_PF_PEROUT;
> +			pin_config[i].chan = i;
> +		}

[Severity: Medium]
Should this also check that the output runs at 1 Hz? Any perout-capable
pin that is connected at probe is reported as holding PEROUT channel i.
That includes a pin that firmware or DPLL netlink set up as, say, a
10 MHz clock.

Normal PTP pin management would then turn that output off. Each of
these triggers it:

  - PTP_PIN_SETFUNC moving channel i to another pin
  - setting the pin to PTP_PF_NONE
  - PTP_PEROUT_REQUEST with period 0 on channel i

They all end up in:

ptp_set_pinfunc() or ptp_perout_request()
  zl3073x_dpll_ptp_enable(on = 0)
    zl3073x_dpll_perout_disable()
      zl3073x_dev_output_pin_state_set(zldev, pin->id, false)

This disconnects an output that was never enabled through PTP. The
commit message does not mention that existing outputs are adopted this
way.

Would it be better to seed only pins that are connected and running at
1 Hz as PTP_PF_PEROUT, and leave the rest as PTP_PF_NONE?

[Severity: Low]
This reads the zldev->out[] cache through
zl3073x_dev_output_pin_state_get() without holding zldpll->lock.

zl3073x_dpll_register() has already registered the DPLL device and pins
before zl3073x_dpll_ptp_register() runs. DPLL netlink ops such as
zl3073x_dpll_output_pin_state_on_dpll_set() can therefore update the
same cache concurrently under zldpll->lock.

zl3073x_dev_output_pin_state_set() also briefly stops the whole output
pair around an fsleep(). An unlocked read in that window could see a
connected pin as stopped and seed it as PTP_PF_NONE.

Would holding zldpll->lock around this seeding loop fix it?

> +		i++;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928185552.1103515-1-ivecera%40redhat.com