Re: [PATCH net-next v3 04/14] selftests: dpll: add pin enumeration and property tests

[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] In dpll_basic.sh, a failed pin dump is reported as SKIP
  instead of FAIL on hardware or user-selected DPLL devices.
- [Low] The comment in check_pin_properties (dpll_basic.sh line 106) says
  'The parent-device nest is not present for all pin types'.

--

selftests: dpll: add pin enumeration and property tests

This adds pin query helpers and dpllsim_add_pin() to the dpll selftest
library and gives the standard dpllsim device three pins. dpll_basic.sh
gains pin enumeration, pin property and pin state tests. The stress
script now creates its devices with a random set of pins.

> diff --git a/tools/testing/selftests/drivers/dpll/dpll_basic.sh b/tools/testing/selftests/drivers/dpll/dpll_basic.sh
> index 827c389fe1aa3..eb5de2e22081e 100755
> --- a/tools/testing/selftests/drivers/dpll/dpll_basic.sh
> +++ b/tools/testing/selftests/drivers/dpll/dpll_basic.sh

[ ... ]

> @@ -43,6 +65,114 @@ test_device_properties()
>  	log_test "DPLL device properties"
>  }
>  
> +test_pin_enumeration()
> +{
> +	local pin_ids
> +
> +	RET=0
> +
> +	mapfile -t pin_ids < <(dpll_pin_get_ids "$DPLL_DEV_ID")
> +	if [ "${#pin_ids[@]}" -eq 0 ]; then
> +		dpll_log_test_skip_or_fail "DPLL pin enumeration" \
> +		    "no pins found"
> +		return
> +	fi

[Severity: Medium]
Can a failed pin dump get reported as a skip here?

dpll_pin_get_ids() returns 1 when the tool fails:

	out=$(dpll pin show -j 2>/dev/null) || return 1

The exit status of a process substitution is lost, though, and mapfile
itself succeeds. So a failed query leaves pin_ids empty, which looks the
same as a device with no pins.

test_pin_properties() uses the same pattern:

	mapfile -t pin_ids < <(dpll_pin_get_ids "$DPLL_DEV_ID")

test_pin_state() has the same problem. pipefail is not enabled there, so
the pipeline returns the status of head:

	pin_id=$(dpll_pin_find_by_direction "$DPLL_DEV_ID" input | head -1)

All three then call dpll_log_test_skip_or_fail(), which calls
log_test_skip whenever DPLL_CREATED_DEVICE is empty. That happens for the
hardware device dpll_lib_init() picks by default, for a device chosen
through DPLL_DRIVER and for one set with DPLL_DEV_ID.

On such a device, would a pin dump that fails because of a driver or
netlink regression be logged as a skip with "no pins found" or "no input
pin", and not as a failure? On the created dpllsim device the test still
fails, but it gives "no pins found" as the reason and does not say that
the query failed.

The commit message says:

    every pin query runs the tool first and jq on what it printed, so
    that a query which fails returns the status of the tool instead of
    the empty output a missing attribute produces too

None of these callers seem to check that status.

> +	check_pin_ids "${pin_ids[@]}"
> +
> +	log_test "DPLL pin enumeration (${#pin_ids[@]} pins)"
> +}
> +
> +check_pin_properties()
> +{

[ ... ]

> +	# The parent-device nest is not present for all pin types, but when
> +	# the device under test is in it, that entry must carry the
> +	# direction. A pin can be registered on more than one device, so
> +	# match on parent-id instead of taking the first entry.

[Severity: Low]
This isn't a bug, but is the first sentence of this comment accurate? In
the DPLL core, whether the parent-device nest is present depends on how
the pin was registered, not on its type:

drivers/dpll/dpll_netlink.c:dpll_cmd_pin_get_one() {
    ...
	if (xa_empty(&pin->parent_refs))
		ret = dpll_msg_add_pin_dplls(msg, pin, extack);
	else
		ret = dpll_msg_add_pin_parents(msg, pin, ref, extack);
    ...
}

A pin registered directly on a device always gets parent-device nests,
MUX pins included. A pin registered on a parent pin gets parent-pin nests
instead, whatever its type.

The comment above dpll_pin_get_ids() in dpll_lib.sh gets this right
("pins registered on those pins (MUX children)"). Could this comment say
the same thing, that the nest is missing for pins registered on a parent
pin? The check itself is not affected.

[ ... ]

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