Re: [PATCH net-next v3 10/14] selftests: dpll: add pin frequency test

[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: 2 · Low: 0

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] test_pin_frequency_set_get() in
  tools/testing/selftests/drivers/dpll/dpll_features.sh only checks that…
- [Medium] test_pin_frequency_set_get() reprograms the frequency of
  PIN_TUNE_ID.

--

selftests: dpll: add pin frequency test

This adds test_pin_frequency_set_get() to dpll_features.sh. The test
takes a supported frequency other than the current one from an input
pin's frequency-supported list, sets it, reads it back and restores the
original value. It also adds a dpll_pin_set_frequency() helper to
dpll_lib.sh, which keeps the tool's error output in DPLL_LAST_ERR.

> diff --git a/tools/testing/selftests/drivers/dpll/dpll_features.sh b/tools/testing/selftests/drivers/dpll/dpll_features.sh
> index b4b1eff322a21..6649936c710e4 100755
> --- a/tools/testing/selftests/drivers/dpll/dpll_features.sh
> +++ b/tools/testing/selftests/drivers/dpll/dpll_features.sh

[ ... ]

> @@ -237,6 +238,69 @@ test_pin_state_transitions()
>  	log_test "DPLL pin state transitions"
>  }
>  
> +test_pin_frequency_set_get()
> +{

[ ... ]

> +	before=$(dpll_pin_get_frequency "$PIN_TUNE_ID")
> +	if [ -z "$before" ]; then
> +		dpll_log_test_skip_or_fail "DPLL pin frequency set/get" \
> +		    "frequency not reported"
> +		return
> +	fi
> +	defer dpll_pin_set_frequency "$PIN_TUNE_ID" "$before"

[Severity: Medium]
Can the original frequency always be written back here? The only check
on before is that it is not empty. The DPLL core refuses any value
outside the pin's frequency-supported ranges, and it does this before
it compares the value with the current frequency:

drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
	...
	if (!dpll_pin_is_freq_supported(pin, freq)) {
		NL_SET_ERR_MSG_ATTR(extack, a, "frequency is not supported by the device");
		return -EINVAL;
	}
	...
}

zl3073x hits this case on purpose. It reads a sub-Hz reference back as
0 Hz and leaves 0 out of freq_supported:

drivers/dpll/zl3073x/prop.c:zl3073x_pin_props_get() {
	...
	/* Start with current frequency at index 0. A sub-Hz frequency is
	 * read back as 0 Hz and cannot be set, so it is not offered.
	 */
	j = 0;
	if (curr_freq) {
	...
}

Here is what happens on such a pin on a device the user provided:

- before=0 passes the -z check.
- The jq filter picks the first advertised frequency as target.
- Setting target succeeds and reprograms the hardware reference.
- The explicit restore and the deferred restore are both refused with
  EINVAL.

Wouldn't this leave the user's device on a frequency the test picked?
The commit message says the test "restores the original value".

The phase adjust test later in the series bails out before it schedules
the restore when the current value could not be written back. Would a
similar check fit here, one that confirms before lies inside an
advertised frequency-supported range before the pin is touched?

> +
> +	# Pick a supported frequency different from the current one. Both
> +	# ends of every supported range are candidates, so a pin with a
> +	# single range still has an alternative unless it is one value wide.
> +	target=$(dpll_pin_json "$PIN_TUNE_ID" | \
> +		 jq -r --argjson cur "$before" \
> +		    '[.["frequency-supported"][]? |
> +		      .["frequency-min"], .["frequency-max"] |
> +		      numbers | select(. != $cur)][0] // empty')

[ ... ]

> +	# A driver without a frequency setter or with constraints of its own
> +	# may refuse the candidate, a skip on a device the user provided, the
> +	# simulator accepts every advertised value.
> +	if ! dpll_pin_set_frequency "$PIN_TUNE_ID" "$target"; then

[Severity: Medium]
Is it safe to reprogram this pin's frequency based only on its state on
DPLL_DEV_ID? pick_tune_pin() counts a pin as spare when it is
disconnected or selectable on that one device:

tools/testing/selftests/drivers/dpll/dpll_features.sh:pick_tune_pin() {
	...
		state=$(dpll_pin_get_state "$DPLL_DEV_ID" "$pin_id")
		case "$state" in
		disconnected|selectable)
	...
}

State and priority are per parent device, but frequency belongs to the
pin itself. It is shared by every DPLL device the pin is registered
with.

On zl3073x, every channel gets the same dpll_pin for an input index
through dpll_pin_get(clock_id, index). The setter then writes the
chip-wide reference state:

drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_input_pin_frequency_set() {
	...
	ref_id = zl3073x_input_pin_ref_get(pin->id);
	ref = *zl3073x_ref_state_get(zldev, ref_id);

	/* Update frequency */
	zl3073x_ref_freq_set(&ref, frequency);

	/* Commit reference state */
	return zl3073x_ref_state_set(zldev, ref_id, &ref);
}

Say the chosen input is selectable on the EEC channel but is the locked
reference of the PPS channel. Could the PPS channel then lose lock, go
into holdover or switch references until the restore runs? ice also
registers its inputs with both its EEC and PPS DPLLs. The test never
checks that the other channel locks again.

Should the pin be checked against all of its parent devices before this
test changes its frequency?

> +		reason=${DPLL_LAST_ERR%%$'\n'*}
> +		dpll_log_test_skip_or_fail "DPLL pin frequency set/get" \
> +		    "cannot set frequency=$target${reason:+, $reason}"
> +		return
> +	fi

[ ... ]

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