Regressions that prevent a driver from probing a device can significantly affect the functionality of a platform.
A kselftest to verify if devices on a DT-based platform are probed correctly was recently introduced [1], but no such generic test is available for ACPI platforms yet. bootrr [2] provides device probe testing, but relies on a pre-defined list of the peripherals present on each DUT.
On ACPI based hardware, a complete description of the platform is provided to the OS by the system firmware. ACPI namespace objects are mapped by the Linux ACPI subsystem into a device tree in /sys/devices/LNXSYSTEM:00; the information in this subtree can be parsed to build a list of the hw peripherals present on the DUT dynamically.
This series adds a test to verify if the devices declared in the ACPI namespace and supported by the kernel are probed correctly.
This work follows a similar approach to [1], adapted for the ACPI use case.
The first patch introduces a script that builds a list of all ACPI device IDs supported by the kernel, by inspecting the acpi_device_id structs in the sources. This list can be used to avoid testing ACPI-enumerated devices that don't have a matching driver in the kernel. This script was highly inspired by the dt-extract-compatibles script [3].
In the second patch, a new kselftest is added. It parses the /sys/devices/LNXSYSTEM:00 tree to obtain a list of all platform peripherals and verifies which of those, if supported, are correctly bound to a driver.
Feedback is much appreciated,
Thank you,
Laura
[1] https://lore.kernel.org/all/20230828211424.2964562-1-nfraprado@collabora.com... [2] https://github.com/kernelci/bootr [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scri...
Laura Nao (2): acpi: Add script to extract ACPI device ids in the kernel kselftest: Add test to detect unprobed devices on ACPI platforms
MAINTAINERS | 2 + scripts/acpi/acpi-extract-ids | 60 +++++++++++++++ tools/testing/selftests/Makefile | 1 + tools/testing/selftests/acpi/.gitignore | 2 + tools/testing/selftests/acpi/Makefile | 23 ++++++ .../selftests/acpi/test_unprobed_devices.sh | 75 +++++++++++++++++++ 6 files changed, 163 insertions(+) create mode 100755 scripts/acpi/acpi-extract-ids create mode 100644 tools/testing/selftests/acpi/.gitignore create mode 100644 tools/testing/selftests/acpi/Makefile create mode 100755 tools/testing/selftests/acpi/test_unprobed_devices.sh
Add a script to extract all the supported acpi device ids from kernel sources.
The list of IDs returned by the script can be used as a reference to determine if a device declared in the ACPI namespace with certain _HID/_CID is supported by the kernel or not.
Signed-off-by: Laura Nao laura.nao@collabora.com --- MAINTAINERS | 1 + scripts/acpi/acpi-extract-ids | 60 +++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100755 scripts/acpi/acpi-extract-ids
diff --git a/MAINTAINERS b/MAINTAINERS index 27751573e314..7540316d82f5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -294,6 +294,7 @@ F: drivers/pnp/pnpacpi/ F: include/acpi/ F: include/linux/acpi.h F: include/linux/fwnode.h +F: scripts/acpi/acpi-extract-ids F: tools/power/acpi/
ACPI APEI diff --git a/scripts/acpi/acpi-extract-ids b/scripts/acpi/acpi-extract-ids new file mode 100755 index 000000000000..12c8e09281dd --- /dev/null +++ b/scripts/acpi/acpi-extract-ids @@ -0,0 +1,60 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: GPL-2.0-only +# +# Heavily inspired by the scripts/dtc/dt-extract-compatibles script, +# adapted for the ACPI use case. +# + +import os +import glob +import re +import argparse + + +def parse_acpi_device_ids(file): + """ Find all device ID strings in acpi_device_id struct """ + id_list = [] + + with open(file, 'r', encoding='utf-8') as f: + data = f.read().replace('\n', '') + + for m in re.finditer(r'acpi_device_id(\s+\S+)?\s+(\S+)[](\s+\S+)?\s*=\s*({.*?);', data): + id_list += re.findall(r'"(\S+)"', m[4]) + + return id_list + + +def print_acpi_device_ids(filename, id_list): + if not id_list: + return + if show_filename: + compat_str = ' '.join(id_list) + print(filename + ": ID(s): " + compat_str) + else: + print(*id_list, sep='\n') + + +def files_to_parse(path_args): + for f in path_args: + if os.path.isdir(f): + for filename in glob.iglob(f + "/**/*.c", recursive=True): + yield filename + else: + yield f + + +show_filename = False + +if __name__ == "__main__": + ap = argparse.ArgumentParser() + ap.add_argument("cfile", type=str, nargs='*', + help="C source files or directories to parse") + ap.add_argument('-H', '--with-filename', + help="Print filename with device ids", action="store_true") + args = ap.parse_args() + + show_filename = args.with_filename + + for f in files_to_parse(args.cfile): + id_list = parse_acpi_device_ids(f) + print_acpi_device_ids(f, id_list)
Add new kselftest that tests whether devices declared in the ACPI namespace and supported by the kernel are correctly bound to a driver.
The test runs the acpi-extract-ids script to generate a list of all the ACPI device IDs present in the kernel sources. The list is then used as a reference to determine which of the devices declared in the ACPI namespace are supported by the kernel and therefore expected to bind to a driver.
Signed-off-by: Laura Nao laura.nao@collabora.com --- MAINTAINERS | 1 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/acpi/.gitignore | 2 + tools/testing/selftests/acpi/Makefile | 23 ++++++ .../selftests/acpi/test_unprobed_devices.sh | 75 +++++++++++++++++++ 5 files changed, 102 insertions(+) create mode 100644 tools/testing/selftests/acpi/.gitignore create mode 100644 tools/testing/selftests/acpi/Makefile create mode 100755 tools/testing/selftests/acpi/test_unprobed_devices.sh
diff --git a/MAINTAINERS b/MAINTAINERS index 7540316d82f5..5c83b36f26ed 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -295,6 +295,7 @@ F: include/acpi/ F: include/linux/acpi.h F: include/linux/fwnode.h F: scripts/acpi/acpi-extract-ids +F: tools/testing/selftests/acpi/ F: tools/power/acpi/
ACPI APEI diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 11aa8a834794..bb95daf9ae91 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +TARGETS += acpi TARGETS += alsa TARGETS += amd-pstate TARGETS += arm64 diff --git a/tools/testing/selftests/acpi/.gitignore b/tools/testing/selftests/acpi/.gitignore new file mode 100644 index 000000000000..2526540468f4 --- /dev/null +++ b/tools/testing/selftests/acpi/.gitignore @@ -0,0 +1,2 @@ +supported_id_list +ktap_helpers.sh \ No newline at end of file diff --git a/tools/testing/selftests/acpi/Makefile b/tools/testing/selftests/acpi/Makefile new file mode 100644 index 000000000000..806e75f15824 --- /dev/null +++ b/tools/testing/selftests/acpi/Makefile @@ -0,0 +1,23 @@ +PY3 = $(shell which python3 2>/dev/null) + +ifneq ($(PY3),) + +TEST_PROGS := test_unprobed_devices.sh +TEST_GEN_FILES := supported_id_list ktap_helpers.sh + +include ../lib.mk + +$(OUTPUT)/supported_id_list: + $(top_srcdir)/scripts/acpi/acpi-extract-ids $(top_srcdir) > $@ + +$(OUTPUT)/ktap_helpers.sh: + cp $(top_srcdir)/tools/testing/selftests/dt/ktap_helpers.sh $@ + +else + +all: no_py3_warning + +no_py3_warning: + @echo "Missing python3. This test will be skipped." + +endif \ No newline at end of file diff --git a/tools/testing/selftests/acpi/test_unprobed_devices.sh b/tools/testing/selftests/acpi/test_unprobed_devices.sh new file mode 100755 index 000000000000..aa8c62166b4d --- /dev/null +++ b/tools/testing/selftests/acpi/test_unprobed_devices.sh @@ -0,0 +1,75 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Copyright (c) 2023 Collabora Ltd +# +# Inspired by the tools/testing/selftests/dt/test_unprobed_devices.sh +# script, adapted for the ACPI use case. +# +# This script checks whether devices declared in the ACPI namespace +# and supported by the kernel are correctly bound to a driver. +# +# A list of all the ACPI device IDs present in the kernel sources +# is used as reference to determine which of the devices declared +# in the ACPI tables are supported. +# + +DIR="$(dirname "$(readlink -f "$0")")" + +source "${DIR}"/ktap_helpers.sh + +ACPI_SYSTEM_DIR="/sys/devices/LNXSYSTM:00" +SUPPORTED_ID_LIST="${DIR}"/supported_id_list + +KSFT_PASS=0 +KSFT_FAIL=1 +KSFT_SKIP=4 + +ktap_print_header + +if [[ ! -d "${ACPI_SYSTEM_DIR}" ]]; then + ktap_skip_all "${ACPI_SYSTEM_DIR} doesn't exist." + exit "${KSFT_SKIP}" +fi + +# The ACPI specification mandates that ACPI objects representing devices on +# non-enumerable and enumerable busses contain a _HID or an _ADR identification +# object respectively. +# Get a list of devices of both types, by searching the ACPI sysfs subtree for +# directories containing a hid or adr attribute. +supp_dev_paths=$(while IFS=$'\n' read -r dev_path; do + if [ ! -f "${dev_path}"/hid ] && [ ! -f "${dev_path}"/adr ]; then + continue + fi + + if [ -f "${dev_path}"/hid ]; then + if ! grep -x -q -i "$(cat "${dev_path}"/hid)" "${SUPPORTED_ID_LIST}"; then + continue + fi + fi + + echo "${dev_path}" +done < <(find ${ACPI_SYSTEM_DIR} -name uevent -exec dirname {} ;)) + +supp_dev_paths_num=$(echo "${supp_dev_paths}" | wc -w) +ktap_set_plan "${supp_dev_paths_num}" + +ret="${KSFT_PASS}" +for dev_path in ${supp_dev_paths}; do + desc="$(cat "${dev_path}"/path)" + [ -f "${dev_path}"/hid ] && desc+=" $(cat "${dev_path}"/hid)" + + # ACPI device objects might be linked to other objects in the device + # hierarchy (e.g. devices on the PCI bus). + # In these cases, the driver folder will be in the companion object's sysfs + # directory, linked by physical_node. + if [ -d "${dev_path}"/physical_node/driver ] || [ -d "${dev_path}"/driver ]; then + ktap_test_pass "${desc}" + else + ret="${KSFT_FAIL}" + ktap_test_fail "${desc}" + fi +done + +ktap_print_totals +exit "${ret}"
Gentle ping to check if there are any feedback or comments on this series.
Thanks, Laura
Your talk was interesting at Linux Plumbers.
https://www.youtube.com/watch?v=oE73eVSyFXQ [time +2:35]
This is probably a stupid question, but why not just add something to call_driver_probe() which creates a sysfs directory tree with all the driver information?
regards, dan carpenter
Your talk was interesting at Linux Plumbers.
https://www.youtube.com/watch?v=oE73eVSyFXQ [time +2:35]
This is probably a stupid question, but why not just add something to call_driver_probe() which creates a sysfs directory tree with all the driver information?
Thanks for the feedback!
Improving the device driver model to publish driver and devices info was indeed another option we considered. We could have a debugfs entry storing this kind of information, similar to what devices_deferred does and in a standardized format. This would provide an interface that is easier to query at runtime for getting a list of devices that were probed correctly. This would cover devices with a driver that's built into the kernel or as a module; in view of catching also those cases where a device is not probed because the relevant config is not enabled, I think we'd still need another way of building a list of devices present on the platform to be used as reference.
The solution proposed in this RFC follows the same approach used for dt based platforms for simplicity. But if adding a new sysfs entry storing devices and driver info proves to be a viable option for upstream, we can surely explore it and improve the probe test to leverage that.
Best,
Laura
On Thu, Nov 23, 2023 at 01:09:42PM +0100, Laura Nao wrote:
Your talk was interesting at Linux Plumbers.
https://www.youtube.com/watch?v=oE73eVSyFXQ [time +2:35]
This is probably a stupid question, but why not just add something to call_driver_probe() which creates a sysfs directory tree with all the driver information?
Thanks for the feedback!
Improving the device driver model to publish driver and devices info was indeed another option we considered. We could have a debugfs entry storing this kind of information, similar to what devices_deferred does and in a standardized format. This would provide an interface that is easier to query at runtime for getting a list of devices that were probed correctly. This would cover devices with a driver that's built into the kernel or as a module; in view of catching also those cases where a device is not probed because the relevant config is not enabled, I think we'd still need another way of building a list of devices present on the platform to be used as reference.
Yeah. So we'd still need patch #1 as-is and but patch #2 would probably be simpler if we had this information in sysfs. Or a different solution would be to do what someone said in the LPC talk and just save the output of the previous boot and complain if there was a regression where something didn't probe.
The solution proposed in this RFC follows the same approach used for dt based platforms for simplicity. But if adding a new sysfs entry storing devices and driver info proves to be a viable option for upstream, we can surely explore it and improve the probe test to leverage that.
You're saying "simplicity" but I think you mean easiest from a political point of view. It's not the most simple format at all. It's like massive detective work to find the information and then you'll have to redo it for DT and for USB. Are there other kinds of devices which can be probed?
I feel like you're not valuing your stuff at the right level. This shouldn't be in debugfs. It should be a first class citizen in sysfs.
The exact format for this information is slightly tricky and people will probably debate that. But I think most people will agree that it's super useful.
regards, dan carpenter
On 11/23/23 16:14, Dan Carpenter wrote:
On Thu, Nov 23, 2023 at 01:09:42PM +0100, Laura Nao wrote:
Your talk was interesting at Linux Plumbers.
https://www.youtube.com/watch?v=oE73eVSyFXQ [time +2:35]
This is probably a stupid question, but why not just add something to call_driver_probe() which creates a sysfs directory tree with all the driver information?
Thanks for the feedback!
Improving the device driver model to publish driver and devices info was indeed another option we considered. We could have a debugfs entry storing this kind of information, similar to what devices_deferred does and in a standardized format. This would provide an interface that is easier to query at runtime for getting a list of devices that were probed correctly. This would cover devices with a driver that's built into the kernel or as a module; in view of catching also those cases where a device is not probed because the relevant config is not enabled, I think we'd still need another way of building a list of devices present on the platform to be used as reference.
Yeah. So we'd still need patch #1 as-is and but patch #2 would probably be simpler if we had this information in sysfs. Or a different solution would be to do what someone said in the LPC talk and just save the output of the previous boot and complain if there was a regression where something didn't probe.
Right. The main drawback of using the status of a known good boot as reference is to keep it up to date over time. If support for a peripheral gets added at a later stage, the reference needs to be updated as well.
The solution proposed in this RFC follows the same approach used for dt based platforms for simplicity. But if adding a new sysfs entry storing devices and driver info proves to be a viable option for upstream, we can surely explore it and improve the probe test to leverage that.
You're saying "simplicity" but I think you mean easiest from a political point of view. It's not the most simple format at all. It's like massive detective work to find the information and then you'll have to redo it for DT and for USB. Are there other kinds of devices which can be probed?
Yeah, that's what I meant. The ACPI use case is in a way simpler to handle than the dt one, as we can get information on non removable devices on enumerable buses such as PCI from the ACPI tables (leveraging the _ADR objects). But it still requires quite a lot digging in sysfs to get info on what was actually probed. So having a list of probed devices would help both use cases.
I feel like you're not valuing your stuff at the right level. This shouldn't be in debugfs. It should be a first class citizen in sysfs.
The exact format for this information is slightly tricky and people will probably debate that. But I think most people will agree that it's super useful.
Right, agreeing on a format will be tricky. Judging by the response here and in LPC it's still worth a shot though. I'll put some thought into this and experiment a bit to come up with a proposal to submit in another RFC.
Again, thanks for the helpful feedback!
Best, Laura
linux-kselftest-mirror@lists.linaro.org
-
Dan Carpenter -
Laura Nao