Re: [PATCH v4 2/2] selftests/pcie_lmr: Add tests for the Lane Margining debugfs interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/pcie_lmr: reversed logic skips LNKCTL restoration test
--

commit b1633adf22c71efa783c2272b3fd92aab3e39291
Author: Priyank Rathod <rathodpriyank@google.com>

selftests/pcie_lmr: Add tests for the Lane Margining debugfs interface

Adds a KTAP test for the Lane Margining at the Receiver debugfs interface.
It runs default passive checks on every pcie_lmr_* directory, and active
margining sessions if PCIE_LMR_DEV points to a port.

> diff --git a/tools/testing/selftests/pcie_lmr/pcie_lmr.sh b/tools/testing/selftests/pcie_lmr/pcie_lmr.sh
> new file mode 100755
> index 0000000000000..99070d221ae06
> --- /dev/null
> +++ b/tools/testing/selftests/pcie_lmr/pcie_lmr.sh

[ ... ]

> +active_tests()
> +{

[ ... ]

> +	if check_end_session "$d"; then
> +		ktap_test_pass "$dev: end the session"
> +		ACTIVE_DIR=
> +	else
> +		ktap_test_fail "$dev: end the session"
> +	fi
> +
> +	up=$([ "$rx" = 6 ] && echo "$dev" || echo "$partner")
> +	after="$(link_regs "$dev") $(link_regs "$partner")"
> +	if [ -z "${before// }" ]; then
> +		ktap_test_skip "$dev: setpci not found"
> +	elif [ -n "$up" ] && [ ! -d "/sys/bus/pci/devices/$up/link" ]; then
> +		ktap_test_skip "$dev: no OS ASPM control on link"

[Severity: Medium]
Is it possible that the logic for identifying the upstream port is reversed?

In active_tests(), the variable up is assigned using the receiver type.
If rx=6, dev is the Endpoint device, so up becomes the Endpoint.
If rx=1, dev is the Downstream Port, meaning partner is the Endpoint,
so up still evaluates to the Endpoint device.

Because up always evaluates to the Endpoint device instead of the
Downstream Port, the check for the sysfs link directory will always fail,
as that directory is only present on the Downstream Port. Will this cause
the test to unconditionally skip the final LNKCTL restoration checks?

> +	else
> +		check "$dev: LNKCTL and LNKCTL2 restored on both ends" \
> +			test "$before" = "$after"
> +	fi
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-pcie-link-endpoints-v4-0-ad5398c4260c@google.com?part=2