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

[Ivan Vecera <ivecera@redhat.com>]

On 9/30/26 8:57 AM, netdev-bot+sashiko@kernel.org wrote:
> 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.

Good catch. I will reorder zl3073x_out_state_set() to commit the mailbox
fields first and write the direct output_ctrl_x register last.

That fixes the recovery path in zl3073x_dev_output_pin_state_set():

   - stop phase: only ctrl changes (STOP/STOP_HZ), there is no cfg diff, so
     the mailbox commit is skipped and only ctrl is written - unchanged.
   - restart phase: state_set() will write the GPO change through the
     mailbox *while the output is still stopped* (ctrl not written yet),
     and only then clears STOP via ctrl - a clean restart, no glitch on a
     running output.
   - on a mailbox failure ctrl is left untouched, so there is no premature
     restart and dout->ctrl stays consistent with the hardware.

The other callers (frequency_set / esync_set / phase_adjust_set) only
change cfg fields with ctrl unchanged, so the ordering does not affect
them.

> 
>>   
>> -	/* 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?

Yes, I will fix them in v2.

Thanks,
Ivan

pw-bot: cr