Re: [PATCH net-next v3 12/14] selftests: dpll: add pin phase adjust test

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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] In tools/testing/selftests/drivers/dpll/dpll_features.sh,
  test_pin_phase_adjust_set_get() refuses to schedule a restore it…

--

selftests: dpll: add pin phase adjust test

This adds test_pin_phase_adjust_set_get() to the dpll selftests. The test
picks a granularity multiple inside the pin's advertised phase adjust
range, sets it, reads it back and then restores the original value. It
bails out before scheduling the restore when the current value is not a
granularity multiple.

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

[ ... ]

> +	json=$(dpll_pin_json "$PIN_TUNE_ID")
> +	min=$(jq -r '.["phase-adjust-min"] // empty' <<< "$json")
> +	max=$(jq -r '.["phase-adjust-max"] // empty' <<< "$json")
> +	gran=$(jq -r '.["phase-adjust-gran"] // empty' <<< "$json")
> +	if [ -z "$min" ] || [ -z "$max" ]; then
> +		dpll_log_test_skip_or_fail "DPLL pin phase-adjust set/get" \
> +		    "no adjust range"
> +		return
> +	fi
> +	# A current value that is not a granularity multiple could not be
> +	# written back, so bail out before the restore is scheduled.
> +	if [ -n "$gran" ] && [ $((before % gran)) -ne 0 ]; then
> +		dpll_log_test_skip_or_fail "DPLL pin phase-adjust set/get" \
> +		    "current phase adjust not a granularity multiple"
> +		return
> +	fi
> +	[ -n "$gran" ] || gran=1
> +	defer dpll_pin_set_phase_adjust "$PIN_TUNE_ID" "$before"

[Severity: Medium]
Can the restore be refused when before lies outside [min, max]?

The granularity guard is the only check before the defer. The core also
checks the range, and it does that first, in dpll_pin_phase_adj_set():

drivers/dpll/dpll_netlink.c:dpll_pin_phase_adj_set() {
    ...
	phase_adj = nla_get_s32(phase_adj_attr);
	if (phase_adj > pin->prop.phase_range.max ||
	    phase_adj < pin->prop.phase_range.min) {
		NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
				    "phase adjust value of out range");
		return -EINVAL;
	}
	if (pin->prop.phase_gran && phase_adj % (s32)pin->prop.phase_gran) {
    ...
}

So an out-of-range current value cannot be written back either, the same
as a value that is not a granularity multiple.

On such a pin, target is built inside [min, max] and the set succeeds.
After that, the explicit restore and the deferred one both fail with
-EINVAL. The pin on a user-supplied device then stays at the top (or
bottom) of the range, and the test reports a FAIL instead of a skip.

ice may hit this. ice_dpll_phase_range_set() derives the range from the
CGU abilities. The current value is read separately from firmware via
ice_aq_get_input_pin_cfg() in ice_dpll_pin_state_update(), and nothing
checks that the two agree. If the abilities report 0, the range is
[0, 0], and any nonzero firmware value is out of range.

Should the guard also bail out when before is less than min or greater
than max, before the restore is scheduled?

> +
> +	# Shell division truncates towards zero, so round the bounds towards
> +	# the inside of the range explicitly, which matters for negative ones.

[ ... ]

> +	dpll_pin_set_phase_adjust "$PIN_TUNE_ID" "$before"
> +	check_err $? "cannot restore phase-adjust"

[ ... ]

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