Re: [PATCH net-next 2/6] dpll: zl3073x: add GPO support for output pins

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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] Error-recovery path applies the output restart before the
  pending GPO change.
- [Low] The struct zl3073x_dev kernel-doc in drivers/dpll/zl3073x/core.h
  (line 65, '@out: array of outs' invariants') and the inline '/*…

--

dpll: zl3073x: add GPO support for output pins

This commit adds the GPO override mailbox fields and the direct gpo_out_x
registers for CMOS output pins, along with helpers to read and set the pin
function. It also adds the output_ctrl_x stop bits and stop/start helpers.
ctrl moves into the cfg struct_group, and zl3073x_out_state_set() now
writes it directly.

> diff --git a/drivers/dpll/zl3073x/out.c b/drivers/dpll/zl3073x/out.c
> index 410d15b96d0b..82564045890d 100644
> --- a/drivers/dpll/zl3073x/out.c
> +++ b/drivers/dpll/zl3073x/out.c

[ ... ]

> @@ -120,11 +135,17 @@ int zl3073x_out_state_set(struct zl3073x_dev *zldev, u8 index,
>  	struct zl3073x_out *dout = &zldev->out[index];
>  	int rc;
>  
> -	/* Reject attempts to change invariant fields (set at fetch only) */
> -	if (WARN_ON(memcmp(&dout->inv, &out->inv, sizeof(out->inv))))
> -		return -EINVAL;
> +	/* ctrl is a direct register, independent of the output mailbox */
> +	if (dout->ctrl != out->ctrl) {
> +		rc = zl3073x_write_u8(zldev, ZL_REG_OUTPUT_CTRL(index),
> +				      out->ctrl);
> +		if (rc)
> +			return rc;
> +
> +		dout->ctrl = out->ctrl;
> +	}

[Severity: Medium]

zl3073x_out_state_set() now always writes output_ctrl_x before any mailbox
field. It does not roll dout->ctrl back if the later zl3073x_mb_op() or a
mailbox field write fails.

Can this order apply a restart before a pending GPO change on an error
recovery path?

The follow-up patch "dpll: zl3073x: allow enabling/disabling output pins"
adds zl3073x_dev_output_pin_state_set(). It puts a stop before the GPO
change and a start after it. Its recovery path looks like this:

	rc = zl3073x_out_state_set(zldev, out_id, &out);

restart_output:
	zl3073x_out_start(&out);
	rc = zl3073x_out_state_set(zldev, out_id, &out) ? : rc;

The second zl3073x_out_state_set() call only changes gpo_en/gpo_config.
Suppose it fails in zl3073x_mb_op() or in one of the mailbox field writes.
Then dout->cfg keeps the old GPO values, while out still holds the new
ones.

The recovery call then carries two changes: the cleared STOP/STOP_HZ bits
and the uncommitted GPO change. Because ctrl is written first, would the
clock restart, with the GPO override then changed on a running output?

That looks like the toggle the stop/start pair is meant to avoid. The
driver's own comment says the GPO toggle is not glitch-free. For
enable=true, the override is released onto a clock that is already
running, at an arbitrary phase. For enable=false, the override is engaged
in the middle of a clock cycle.

Also, if the retried commit succeeds, the "? : rc" still returns the
earlier error. zl3073x_dpll_output_pin_state_on_dpll_set() would then
report a failure even though the pin state changed.

>  
> -	/* Skip HW write if configuration hasn't changed */
> +	/* Skip the mailbox commit if nothing else has changed */
>  	if (!memcmp(&dout->cfg, &out->cfg, sizeof(out->cfg)))
>  		return 0;

[ ... ]

> diff --git a/drivers/dpll/zl3073x/out.h b/drivers/dpll/zl3073x/out.h
> index 660889c57bff..66a8432c00df 100644
> --- a/drivers/dpll/zl3073x/out.h
> +++ b/drivers/dpll/zl3073x/out.h

[ ... ]

> @@ -29,8 +32,9 @@ struct zl3073x_out {
>  		u32	esync_n_width;
>  		s32	phase_comp;
>  		u8	mode;
> -	);
> -	struct_group(inv, /* Invariants */
> +		u8	gpo_en;
> +		u8	gpo_config_p;
> +		u8	gpo_config_n;
>  		u8	ctrl;
>  	);
>  };

[Severity: Low]

This isn't a bug, but with the inv group removed, struct zl3073x_out has
no invariant fields left. The struct zl3073x_dev comments in core.h still
describe out[] as invariants:

 * @out: array of outs' invariants
...
	/* Invariants */
	struct zl3073x_ref	ref[ZL3073X_NUM_REFS];
	struct zl3073x_out	out[ZL3073X_NUM_OUTS];

These comments are still unchanged at the end of the series. Should they
be updated along with this change?

[ ... ]

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