[RFC PATCH 0/2] Add test to verify probe of devices from discoverable busses on DT platforms
This is part of an effort to improve detection of regressions impacting device probe on all platforms. The recently merged DT kselftest [1] detects probe issues for all devices described statically in the DT. That leaves out devices discovered at run-time from discoverable busses.
This is where this test comes in. All of the devices that are connected through discoverable busses (ie USB and PCI), and which are internal and therefore always present, can be described in a per-platform file so they can be checked for. The test will check that the device has been instantiated and bound to a driver.
Patch 1 introduces the test. Patch 2 adds the test definitions for the google,spherion machine (Acer Chromebook 514) as an example.
This is the sample output from the test running on Spherion:
TAP version 13 Using board file: boards/google,spherion 1..10 ok 1 usb.camera.0.device ok 2 usb.camera.0.driver ok 3 usb.camera.1.device ok 4 usb.camera.1.driver ok 5 usb.bluetooth.0.device ok 6 usb.bluetooth.0.driver ok 7 usb.bluetooth.1.device ok 8 usb.bluetooth.1.driver ok 9 pci.wifi.device ok 10 pci.wifi.driver Totals: pass:10 fail:0 xfail:0 xpass:0 skip:0 error:0
[1] https://lore.kernel.org/all/20230828211424.2964562-1-nfraprado@collabora.com...
Nícolas F. R. A. Prado (2): kselftest: Add test to verify probe of devices from discoverable busses kselftest: devices: Add board file for google,spherion
tools/testing/selftests/Makefile | 1 + tools/testing/selftests/devices/.gitignore | 1 + tools/testing/selftests/devices/Makefile | 8 + .../selftests/devices/boards/google,spherion | 3 + .../devices/test_discoverable_devices.sh | 165 ++++++++++++++++++ 5 files changed, 178 insertions(+) create mode 100644 tools/testing/selftests/devices/.gitignore create mode 100644 tools/testing/selftests/devices/Makefile create mode 100644 tools/testing/selftests/devices/boards/google,spherion create mode 100755 tools/testing/selftests/devices/test_discoverable_devices.sh
Add a new test to verify that all expected devices from discoverable busses (ie USB, PCI) on a given Devicetree-based platform have been successfully instantiated and probed by a driver.
The per-platform list of expected devices is selected based on compatible and stored under the boards/ directory.
The tests encode the devices to test for based on the hardware topology. For USB devices, the format is: usb <test_name> <controller_address>[,<additional_match>] <ports_path> <configuration> <interfaces>
The additional match field is optional and used to differentiate between two busses (USB2 and USB3) sharing the same USB host controller.
For PCI devices, the format is: pci <test_name> <controller_address> <device-function_pairs_path>
Signed-off-by: Nícolas F. R. A. Prado nfraprado@collabora.com
---
tools/testing/selftests/Makefile | 1 + tools/testing/selftests/devices/.gitignore | 1 + tools/testing/selftests/devices/Makefile | 8 + .../devices/test_discoverable_devices.sh | 165 ++++++++++++++++++ 4 files changed, 175 insertions(+) create mode 100644 tools/testing/selftests/devices/.gitignore create mode 100644 tools/testing/selftests/devices/Makefile create mode 100755 tools/testing/selftests/devices/test_discoverable_devices.sh
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 3b2061d1c1a5..7f5088006c3c 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -13,6 +13,7 @@ TARGETS += core TARGETS += cpufreq TARGETS += cpu-hotplug TARGETS += damon +TARGETS += devices TARGETS += dmabuf-heaps TARGETS += drivers/dma-buf TARGETS += drivers/s390x/uvdevice diff --git a/tools/testing/selftests/devices/.gitignore b/tools/testing/selftests/devices/.gitignore new file mode 100644 index 000000000000..e3c5c04d1b19 --- /dev/null +++ b/tools/testing/selftests/devices/.gitignore @@ -0,0 +1 @@ +ktap_helpers.sh diff --git a/tools/testing/selftests/devices/Makefile b/tools/testing/selftests/devices/Makefile new file mode 100644 index 000000000000..ff2fdc8fc5e2 --- /dev/null +++ b/tools/testing/selftests/devices/Makefile @@ -0,0 +1,8 @@ +TEST_PROGS := test_discoverable_devices.sh +TEST_GEN_FILES := ktap_helpers.sh +TEST_FILES := boards + +include ../lib.mk + +$(OUTPUT)/ktap_helpers.sh: + cp ../dt/ktap_helpers.sh $@ diff --git a/tools/testing/selftests/devices/test_discoverable_devices.sh b/tools/testing/selftests/devices/test_discoverable_devices.sh new file mode 100755 index 000000000000..91842b0c769f --- /dev/null +++ b/tools/testing/selftests/devices/test_discoverable_devices.sh @@ -0,0 +1,165 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Copyright (c) 2023 Collabora Ltd +# +# This script tests for presence and driver binding of devices from discoverable +# busses (ie USB, PCI) on Devicetree-based platforms. +# +# The per-platform list of devices to be tested is stored inside the boards/ +# directory and chosen based on compatible. +# + +DIR="$(dirname "$(readlink -f "$0")")" + +source "${DIR}"/ktap_helpers.sh + +KSFT_FAIL=1 +KSFT_SKIP=4 + +retval=0 + +usb() +{ + name="$1" + controller="$2" + path="$3" + configuration="$4" + interfaces="$5" + + # Extract additional match if present + if [[ "$controller" =~ , ]]; then + additional_match=${controller#*,} + address=${controller%,*} + else + address="$controller" + fi + + for controller_uevent in /sys/bus/usb/devices/usb*/uevent; do + if grep -q "OF_FULLNAME=.*@$address$" "$controller_uevent"; then + # Look for additional match if present. It is needed to + # disambiguate two USB busses that share the same + # controller. + if [ -n "$additional_match" ]; then + if ! grep -q "$additional_match" "$controller_uevent"; then + continue + fi + fi + dir=$(basename "$(dirname "$controller_uevent")") + busnum=${dir#usb} + fi + done + + usbdevs=/sys/bus/usb/devices/ + + IFS=, + for intf in $interfaces; do + devfile="$busnum"-"$path":"$configuration"."$intf" + + if [ -d "$usbdevs"/"$devfile" ]; then + ktap_test_pass usb."$name"."$intf".device + else + ktap_test_fail usb."$name"."$intf".device + retval=$KSFT_FAIL + fi + + if [ -d "$usbdevs"/"$devfile"/driver ]; then + ktap_test_pass usb."$name"."$intf".driver + else + ktap_test_fail usb."$name"."$intf".driver + retval=$KSFT_FAIL + fi + done +} + +pci() +{ + name="$1" + controller="$2" + path="$3" + + IFS=$'\n' + while read -r uevent; do + grep -q "OF_FULLNAME=.*@$controller$" "$uevent" || continue + + # Ignore PCI bus directory, since it will have the same backing + # OF node, but not the PCI devices as subdirectories. + [[ "$uevent" =~ pci_bus ]] && continue + + host_dir=$(dirname "$uevent") + done < <(find /sys/devices -name uevent) + + # Add * to each level of the PCI hierarchy so we can rely on globbing to + # find the device directory on sysfs. + globbed_path=$(echo "$path" | sed -e 's|^|*|' -e 's|/|/*|') + device_path="$host_dir/pci*/$globbed_path" + + # Intentionally left unquoted to allow the glob to expand + if [ -d $device_path ]; then + ktap_test_pass pci."$name".device + else + ktap_test_fail pci."$name".device + retval=$KSFT_FAIL + fi + + if [ -d $device_path/driver ]; then + ktap_test_pass pci."$name".driver + else + ktap_test_fail pci."$name".driver + retval=$KSFT_FAIL + fi +} + +count_tests() +{ + board_file="$1" + num_tests=0 + + # Each USB interface in a single USB test in the board file is a + # separate test + while read -r line; do + num_intfs=$(echo "$line" | tr -dc , | wc -c) + num_intfs=$((num_intfs + 1)) + num_tests=$((num_tests + num_intfs)) + done < <(grep ^usb "$board_file" | cut -d ' ' -f 6 -) + + num_pci=$(grep -c ^pci "$board_file") + num_tests=$((num_tests + num_pci)) + + # Account for device and driver test for each of the tests listed in the + # board file. + num_tests=$((num_tests * 2)) + echo $num_tests +} + +ktap_print_header + +plat_compatible=/proc/device-tree/compatible + +if [ ! -f "$plat_compatible" ]; then + ktap_skip_all "No board compatible available" + exit "$KSFT_SKIP" +fi + +compatibles=$(tr '\000' '\n' < "$plat_compatible") + +for compatible in $compatibles; do + if [ -f boards/"$compatible" ]; then + board_file=boards/"$compatible" + break + fi +done + +if [ -z "$board_file" ]; then + ktap_skip_all "No matching board file found" + exit "$KSFT_SKIP" +fi + +echo "# Using board file: " "$board_file" + +ktap_set_plan "$(count_tests "$board_file")" + +source "$board_file" + +ktap_print_totals +exit "${retval}"
Add the list of devices expected to be probed from the USB and PCI busses on the google,spherion machine. The USB host controller at 11200000 is shared between two busses, for USB2 and USB3, so an additional match is used to select the USB2 bus.
Signed-off-by: Nícolas F. R. A. Prado nfraprado@collabora.com ---
tools/testing/selftests/devices/boards/google,spherion | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tools/testing/selftests/devices/boards/google,spherion
diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion new file mode 100644 index 000000000000..ba86ffcfe43c --- /dev/null +++ b/tools/testing/selftests/devices/boards/google,spherion @@ -0,0 +1,3 @@ +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 +pci wifi 11230000 0.0/0.0
On Tue, Oct 24, 2023 at 05:18:00PM -0400, Nícolas F. R. A. Prado wrote:
Add the list of devices expected to be probed from the USB and PCI busses on the google,spherion machine. The USB host controller at 11200000 is shared between two busses, for USB2 and USB3, so an additional match is used to select the USB2 bus.
Signed-off-by: Nícolas F. R. A. Prado nfraprado@collabora.com
tools/testing/selftests/devices/boards/google,spherion | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tools/testing/selftests/devices/boards/google,spherion
diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion new file mode 100644 index 000000000000..ba86ffcfe43c --- /dev/null +++ b/tools/testing/selftests/devices/boards/google,spherion @@ -0,0 +1,3 @@ +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 +pci wifi 11230000 0.0/0.0
USB busses (and PCI ids) are not determinisitic and can, and will, change values randomly. So while it is nice to test "did the devices show up properly", you can not do that based on bus ids at all, sorry.
Unless I'm reading these values wrong? What are the fields representing? Perhaps a comment at the top to describe them so that we know how to parse them?
thanks,
greg k-h
On Wed, Oct 25, 2023 at 12:32:15PM +0200, Greg Kroah-Hartman wrote:
On Tue, Oct 24, 2023 at 05:18:00PM -0400, Nícolas F. R. A. Prado wrote:
Add the list of devices expected to be probed from the USB and PCI busses on the google,spherion machine. The USB host controller at 11200000 is shared between two busses, for USB2 and USB3, so an additional match is used to select the USB2 bus.
Signed-off-by: Nícolas F. R. A. Prado nfraprado@collabora.com
tools/testing/selftests/devices/boards/google,spherion | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tools/testing/selftests/devices/boards/google,spherion
diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion new file mode 100644 index 000000000000..ba86ffcfe43c --- /dev/null +++ b/tools/testing/selftests/devices/boards/google,spherion @@ -0,0 +1,3 @@ +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 +pci wifi 11230000 0.0/0.0
USB busses (and PCI ids) are not determinisitic and can, and will, change values randomly. So while it is nice to test "did the devices show up properly", you can not do that based on bus ids at all, sorry.
Unless I'm reading these values wrong? What are the fields representing? Perhaps a comment at the top to describe them so that we know how to parse them?
Hi Greg,
I have described the fields in the commit message of patch 1. Here they are:
usb <test_name> <controller_address>[,<additional_match>] <ports_path> <configuration> <interfaces>
pci <test_name> <controller_address> <device-function_pairs_path>
I'm aware that bus IDs are assigned at runtime, and that's exactly why I've avoided those in the test definitions, instead describing the hardware topology, which won't ever change.
And just to be extra clear, by hardware topology I mean:
For USB, we find the USB bus based on the address of its controller (and optionally its productID if two busses share the same controller for USB2 and USB3), and then find the device by following the ports at each hub. The configuration number and interfaces then describe what interfaces to check for presence and driver binding.
For PCI, we find the controller again based on its address, and follow the device-function pairs at each level in the topology until we arrive at the desired device.
We don't rely on the USB bus number, nor on the PCI domain and bus number, since these are all assigned at runtime.
Thanks, Nícolas
On Wed, Oct 25, 2023 at 08:32:42AM -0400, Nícolas F. R. A. Prado wrote:
On Wed, Oct 25, 2023 at 12:32:15PM +0200, Greg Kroah-Hartman wrote:
On Tue, Oct 24, 2023 at 05:18:00PM -0400, Nícolas F. R. A. Prado wrote:
Add the list of devices expected to be probed from the USB and PCI busses on the google,spherion machine. The USB host controller at 11200000 is shared between two busses, for USB2 and USB3, so an additional match is used to select the USB2 bus.
Signed-off-by: Nícolas F. R. A. Prado nfraprado@collabora.com
tools/testing/selftests/devices/boards/google,spherion | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tools/testing/selftests/devices/boards/google,spherion
diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion new file mode 100644 index 000000000000..ba86ffcfe43c --- /dev/null +++ b/tools/testing/selftests/devices/boards/google,spherion @@ -0,0 +1,3 @@ +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 +pci wifi 11230000 0.0/0.0
USB busses (and PCI ids) are not determinisitic and can, and will, change values randomly. So while it is nice to test "did the devices show up properly", you can not do that based on bus ids at all, sorry.
Unless I'm reading these values wrong? What are the fields representing? Perhaps a comment at the top to describe them so that we know how to parse them?
Hi Greg,
I have described the fields in the commit message of patch 1. Here they are:
usb <test_name> <controller_address>[,<additional_match>] <ports_path> <configuration> <interfaces>
pci <test_name> <controller_address> <device-function_pairs_path>
That's not a good place to document them, we'll never find them, and I obviously missed it as well.
Please put it in a comment at the top of this file _AND_ in a comment in the script that parses these files.
I'm aware that bus IDs are assigned at runtime, and that's exactly why I've avoided those in the test definitions, instead describing the hardware topology, which won't ever change.
And just to be extra clear, by hardware topology I mean:
For USB, we find the USB bus based on the address of its controller (and optionally its productID if two busses share the same controller for USB2 and USB3),
What exactly do you mean by "address" of a controller? That will be unique per bus-type that the controller lives on, right?
and then find the device by following the ports at each hub. The configuration number and interfaces then describe what interfaces to check for presence and driver binding.
Ok, good, hub and port locations _should_ be stable, but note they have changed at times so you will have to deal with that for the next 20+ years, are you ok with that?
For PCI, we find the controller again based on its address, and follow the device-function pairs at each level in the topology until we arrive at the desired device.
"address"? What exactly do you mean by this value? For PCI, that will change.
We don't rely on the USB bus number, nor on the PCI domain and bus number, since these are all assigned at runtime.
You are relying on a specific sysfs tree as well, are you able to handle it when new devices get added to the middle of a device path? That sometimes happens in sysfs too.
thanks,
greg k-h
On Fri, Oct 27, 2023 at 12:48:35PM +0200, Greg Kroah-Hartman wrote:
On Wed, Oct 25, 2023 at 08:32:42AM -0400, Nícolas F. R. A. Prado wrote:
On Wed, Oct 25, 2023 at 12:32:15PM +0200, Greg Kroah-Hartman wrote:
On Tue, Oct 24, 2023 at 05:18:00PM -0400, Nícolas F. R. A. Prado wrote:
Add the list of devices expected to be probed from the USB and PCI busses on the google,spherion machine. The USB host controller at 11200000 is shared between two busses, for USB2 and USB3, so an additional match is used to select the USB2 bus.
Signed-off-by: Nícolas F. R. A. Prado nfraprado@collabora.com
tools/testing/selftests/devices/boards/google,spherion | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 tools/testing/selftests/devices/boards/google,spherion
diff --git a/tools/testing/selftests/devices/boards/google,spherion b/tools/testing/selftests/devices/boards/google,spherion new file mode 100644 index 000000000000..ba86ffcfe43c --- /dev/null +++ b/tools/testing/selftests/devices/boards/google,spherion @@ -0,0 +1,3 @@ +usb camera 11200000,PRODUCT=.*/2/.* 1.4.1 1 0,1 +usb bluetooth 11200000,PRODUCT=.*/2/.* 1.4.2 1 0,1 +pci wifi 11230000 0.0/0.0
USB busses (and PCI ids) are not determinisitic and can, and will, change values randomly. So while it is nice to test "did the devices show up properly", you can not do that based on bus ids at all, sorry.
Unless I'm reading these values wrong? What are the fields representing? Perhaps a comment at the top to describe them so that we know how to parse them?
Hi Greg,
I have described the fields in the commit message of patch 1. Here they are:
usb <test_name> <controller_address>[,<additional_match>] <ports_path> <configuration> <interfaces>
pci <test_name> <controller_address> <device-function_pairs_path>
That's not a good place to document them, we'll never find them, and I obviously missed it as well.
Please put it in a comment at the top of this file _AND_ in a comment in the script that parses these files.
Fair enough, will do so in the next version.
I'm aware that bus IDs are assigned at runtime, and that's exactly why I've avoided those in the test definitions, instead describing the hardware topology, which won't ever change.
And just to be extra clear, by hardware topology I mean:
For USB, we find the USB bus based on the address of its controller (and optionally its productID if two busses share the same controller for USB2 and USB3),
What exactly do you mean by "address" of a controller? That will be unique per bus-type that the controller lives on, right?
Well, in its current form the test is targetted at DT-based platforms, so by address I mean the MMIO address of the host controller, as defined in the controller's node in the DT.
In order to expand the coverage of the test to non-DT platforms as well, yes, we'll need to allow describing controllers that are under some bus, rather than at a fixed memory address. I'm already planning to do that as a next step.
and then find the device by following the ports at each hub. The configuration number and interfaces then describe what interfaces to check for presence and driver binding.
Ok, good, hub and port locations _should_ be stable, but note they have changed at times so you will have to deal with that for the next 20+ years, are you ok with that?
If the locations changed it means we have a new hardware revision. I'd expect most of those occurences to result in new compatible strings or DMI identifiers, which we could then use to run a different set of tests, using the new hub and port locations, since this is effectively a different piece of hardware.
For PCI, we find the controller again based on its address, and follow the device-function pairs at each level in the topology until we arrive at the desired device.
"address"? What exactly do you mean by this value? For PCI, that will change.
Same as for USB.
We don't rely on the USB bus number, nor on the PCI domain and bus number, since these are all assigned at runtime.
You are relying on a specific sysfs tree as well, are you able to handle it when new devices get added to the middle of a device path? That sometimes happens in sysfs too.
Right, I'm relying on the fact that the PCI topology is reflected into the sysfs topology, meaning the sysfs directories for devices on a PCI bus are direct descendants of the directory for the PCI bridge exposing that bus. That's the only place I'm aware of which exposes the PCI topology.
In any case, although simplistic, this approach has worked well so far on the platforms I've tested on.
Thanks, Nícolas
linux-kselftest-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Nícolas F. R. A. Prado