Re: [PATCH net-next v14 09/12] dpll: sit9531x: implement output pin state on a DPLL

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 3 (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] In sit9531x_output_hiz_write() (drivers/dpll/sit9531x/core.c),
  un-mute does not undo mute.
- [Medium] In sit9531x_dpll_output_pin_state_on_dpll_set(), a CONNECTED
  request is answered with success without running the un-mute whenever…
- [Medium] The new no-op short-circuit can report success while the
  output system is still in PRG_CMD with its loops open.
- [Low] The commit message describes the register writes inaccurately in
  three ways.
- [Low] Two kernel-docs in drivers/dpll/sit9531x/dpll.c do not match the
  implementation.
- [Low] The commit message promises that "A request that fails after some
  of its writes landed still announces the change, so a subscriber is…

Pre-existing issues:
- [Low] If the first write in sit9531x_prg_enter()
  (SIT9531X_DEBUG_UNLOCK_VAL to SIT9531X_REG_OUTSYS_DEBUG) reports an…

--

dpll: sit9531x: implement output pin state on a DPLL

This adds state_on_dpll_get/set callbacks for sit9531x output pins.
DISCONNECTED mutes an output by forcing its pads to Hi-Z, and CONNECTED
releases the override. The patch also starts registering output pins with
the DPLL core.

> The force bit and the state bit are separate, and a pad follows the loaded
> configuration while the force bit is clear, so both are read to decide
> whether an output is muted and both are written to change it.  Which of
> the four register banks applies depends on the slot and on whether the pad
> is single-ended or differential.

[Severity: Low]
Is this description accurate? A few parts of it don't match the code.

The un-mute branch of sit9531x_output_hiz_write() only clears the MASK
(force) bit. It never writes STATE.

Only the read side, sit9531x_output_forced_hiz(), looks at
sitdev->out[].cmos. Mute and un-mute both write all three pairs (DIFF,
CMOS OutP, CMOS OutN), whatever the output's wiring.

sit9531x_output_get_hiz_regs() uses five mask/state register pairs
(DIFF_07, SE_07, SEN_07, DIFF_811, SE_811), not four banks.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 875a5fe783d5c..9aa0d327b6bf6 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c

[ ... ]

> @@ -622,6 +601,251 @@ static int sit9531x_prg_commit(struct sit9531x_dev *sitdev)
>  	return 0;
>  }
>  
> +/*
> + * Enter the output-system programming state: unlock the debug
> + * registers on Page 3 and issue the PRG_CMD state command.  Register
> + * writes that reconfigure the output system only take effect when
> + * they are made inside this state.
> + */
> +static int sit9531x_prg_enter(struct sit9531x_dev *sitdev)
> +{
> +	int rc;
> +
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_OUTSYS_DEBUG,
> +			       SIT9531X_DEBUG_UNLOCK_VAL);
> +	if (rc)
> +		return rc;

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The
previous patch's sit9531x_prg_enter() had the same early return, and this
patch only moves it.

If this first write reports an error but the transfer still reached the
part, does the output-system debug key stay unlocked? Nothing on this path
writes SIT9531X_DEBUG_LOCK_VAL back. The key would stay open until the next
sequence that runs sit9531x_prg_commit().

> +
> +	rc = sit9531x_write_u8(sitdev, SIT9531X_REG_PRG_DIR_GEN,
> +			       SIT9531X_PRG_CMD_STATE);

[ ... ]

> +static int sit9531x_output_hiz_write(struct sit9531x_dev *sitdev, u8 slot,
> +				     bool mute)
> +{
> +	u8 mask[SIT9531X_HIZ_PAIRS], state[SIT9531X_HIZ_PAIRS];
> +	struct sit9531x_hiz_pair p[SIT9531X_HIZ_PAIRS];
> +	int i, rc, undo_rc;
> +
> +	sit9531x_output_get_hiz_regs(slot, p);
> +
> +	if (!mute) {
> +		for (i = 0; i < SIT9531X_HIZ_PAIRS; i++) {
> +			rc = sit9531x_hiz_set_bit(sitdev, p[i].mask, p[i].bit,
> +						  false);
> +			if (rc)
> +				return rc;
> +		}
> +		return 0;
> +	}
> +
> +	/*
> +	 * Remember every pair as it was: if the mute fails part-way, that
> +	 * is what the pad goes back to -- which may already be a mute, from
> +	 * an earlier request or from the loaded profile.
> +	 */
> +	for (i = 0; i < SIT9531X_HIZ_PAIRS; i++) {
> +		rc = sit9531x_read_u8(sitdev, p[i].state, &state[i]);
> +		if (rc)
> +			return rc;
> +		rc = sit9531x_read_u8(sitdev, p[i].mask, &mask[i]);
> +		if (rc)
> +			return rc;
> +	}

[Severity: Medium]
Does un-mute actually undo a mute here? The mute path reads each pair's
MASK and STATE first, because a pair "may already be a mute, from an
earlier request or from the loaded profile". Those values are only used
by the rollback.

The !mute branch above clears MASK on all three pairs unconditionally. So
any Hi-Z override the loaded profile set on one of these pads is lost
after a DISCONNECTED -> CONNECTED cycle.

Take a CMOS output where the profile forces only one pad to Hi-Z. It still
decodes as CONNECTED, because sit9531x_output_forced_hiz() needs both pads
forced. After a mute/un-mute round trip, that pad follows its
unoverridden configuration. The getter reports CONNECTED both before and
after.

The kernel-doc of sit9531x_output_enable() says the device drives the
output "again as the loaded profile configures it". That doesn't hold in
this case. Could the pre-mute MASK/STATE bits be kept and restored on
un-mute?

[ ... ]

> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index aa021b374c45c..f6e76318a567a 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
> @@ -831,10 +831,140 @@ sit9531x_dpll_output_pin_frequency_set(const struct dpll_pin *pin,
>  	return rc;
>  }
>  
> +/*
> + * sit9531x_dpll_output_pin_state_on_dpll_get - get output pin state
> + *
> + * reports CONNECTED when the output is driven and
> + * DISCONNECTED when it has been muted via sit9531x_output_disable().
> + */

[ ... ]

> +/*
> + * sit9531x_dpll_output_pin_state_on_dpll_set - mute/un-mute an output
> + *
> + * Forces Hi-Z on the output through the Hi-Z force/state register pairs
> + * of its slot, the differential and the single-ended one.
> + *   CONNECTED    -> enable (release the override, the device drives it)
> + *   DISCONNECTED -> disable (force Hi-Z)
> + */

[Severity: Low]
This isn't a bug, but these two comments don't match the implementation.

sit9531x_output_hiz_write() writes three pairs (DIFF, CMOS OutP and CMOS
OutN), not two. For slots 8-11, CMOS OutN is a second bit inside the
DIFF_811 registers (slot - 8 + SIT9531X_HIZ_SEN_811_SHIFT).

The getter comment says DISCONNECTED means the output was muted via
sit9531x_output_disable(). However, sit9531x_out_state_fetch() also
reports DISCONNECTED for a Hi-Z override the loaded profile left at
probe.

> +static int
> +sit9531x_dpll_output_pin_state_on_dpll_set(const struct dpll_pin *pin,

[ ... ]

> +	mutex_lock(&sitdev->multiop_lock);
> +
> +	/*
> +	 * The core forwards a request for the state the output is already
> +	 * in.  The Hi-Z pairs say what the pad does now, so read them and
> +	 * answer such a request from them rather than run the programming
> +	 * sequence, which holds the output loops unlocked for the 100 ms its
> +	 * commit takes and would change nothing.  A read that fails leaves
> +	 * the request to the sequence and its own read-back.
> +	 */
> +	if (!sit9531x_output_state_refresh(sitdev, dpin->id) &&
> +	    sitdev->out[dpin->id].enabled == enable) {
> +		mutex_unlock(&sitdev->multiop_lock);
> +		return 0;
> +	}

[Severity: Medium]
Can this early return leave a CMOS pad forced to Hi-Z while every
CONNECTED request reports success?

For a CMOS output, sit9531x_output_forced_hiz() reports muted only when
both pads are forced. It stops as soon as CMOS_P isn't forced:

	rc = sit9531x_hiz_pair_forced(sitdev, &p[SIT9531X_HIZ_CMOS_P], muted);
	if (rc || !*muted)
		return rc;

So enabled here means "not every CMOS pad is forced", not "no override is
left".

The un-mute branch of sit9531x_output_hiz_write() clears MASK in the order
DIFF, CMOS_P, CMOS_N. It returns on the first failure without rolling
back. Suppose the CMOS_N read-modify-write fails after CMOS_P was released:

sit9531x_output_enable()
  sit9531x_output_hiz_write(mute=false)   CMOS_P released, CMOS_N fails
  sit9531x_prg_commit()                   commits the partial result
  sit9531x_output_forced_hiz()            decodes enabled = true

The callback returns an error and notifies CONNECTED. A CONNECTED retry
then sees enabled == enable here and returns 0 without calling
sit9531x_output_enable(). CMOS_N stays forced on every later retry.

The same thing happens if a mute's CMOS_N write fails and its CMOS_P
rollback also fails, leaving P forced and N released. Only a DISCONNECTED
followed by a CONNECTED clears it.

[Severity: Medium]
Could the same early return also skip re-locking the output loops?

Suppose the Hi-Z writes in sit9531x_output_enable() or
sit9531x_output_disable() succeed, but all SIT9531X_LOOP_LOCK_TRIES writes
of SIT9531X_LOOP_LOCK in sit9531x_prg_commit() fail. prg_commit() only
logs "output loops left unlocked after programming" and returns rc2.

If the bus recovers before the read-back that follows the msleep(100),
the cache shows the requested state, state_stale is cleared, and the
callback returns an error.

A retry of the same request then takes this early return and returns 0
without running prg_enter()/prg_commit(). The loops stay open until some
unrelated programming sequence runs. Without this short-circuit, a retry
would re-run the sequence and re-lock them. Nothing records that the exit
from the programming state failed.

[ ... ]

> +	if (rc) {
> +		struct dpll_pin *dpll_pin;
> +
> +		NL_SET_ERR_MSG(extack, "Failed to set output pin state");

[ ... ]

> +		dpll_pin = READ_ONCE(dpin->dpll_pin);
> +		if (changed && dpll_pin)
> +			__dpll_pin_change_ntf(dpll_pin);
> +	}

[Severity: Low]
The commit message says:

  A request that fails after some of its writes landed still announces
  the change, so a subscriber is not left with the old state

Does that hold when the read-back keeps failing? If the Hi-Z read-back in
sit9531x_output_enable()/disable() fails, state_stale is set and this
code calls __dpll_pin_change_ntf(). That builds the message synchronously:

__dpll_pin_change_ntf()
  dpll_pin_event_send()
    dpll_cmd_pin_get_one()
      dpll_msg_add_pin_dplls()
        dpll_msg_add_pin_on_dpll_state()
          sit9531x_dpll_output_pin_state_on_dpll_get()
            sit9531x_output_state_refresh()

If that second read also fails, the getter returns an error and
dpll_pin_event_send() cancels the message without multicasting it:

	ret = dpll_cmd_pin_get_one(msg, pin, NULL);
	if (ret)
		goto err_cancel_msg;

The return value of __dpll_pin_change_ntf() is ignored here, and no
pending notification is recorded. The periodic poll doesn't look at
outputs. After the bus recovers, would subscribers keep the old state
until some other operation sends a notification?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016005823.13690-1-arouhi%40sitime.com