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/[email protected]/
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
--
2.42.0
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 <[email protected]>
---
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}"
--
2.42.0
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 <[email protected]>
---
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
--
2.42.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 <[email protected]>
> ---
>
> 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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
> > > ---
> > >
> > > 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 <[email protected]>
> > > > ---
> > > >
> > > > 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