diff mbox series

[v2,3/3] kselftest: Add new test for detecting unprobed Devicetree devices

Message ID 20230817233635.2306377-4-nfraprado@collabora.com
State New
Headers show
Series Add a test to catch unprobed Devicetree devices | expand

Commit Message

Nícolas F. R. A. Prado Aug. 17, 2023, 11:35 p.m. UTC
Introduce a new kselftest to detect devices that were declared in the
Devicetree, and are expected to be probed by a driver, but weren't.

The test uses two lists: a list of compatibles that can match a
Devicetree device to a driver, and a list of compatibles that should be
ignored. The first is automatically generated by the
dt-extract-compatibles script, and is run as part of building this test.
The list of compatibles to ignore is a hand-crafted list to capture the
few exceptions of compatibles that are expected to match a driver but
not be bound to it.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

Changes in v2:
- Switched output to be in KTAP format
- Changed Makefile to make use of the dt-extract-compatibles instead of
  the Coccinelle script
- Dropped compatibles from compatible_ignore_list that are now already
  filtered out by extraction script
- Added early exit if /proc/device-tree is not present

 tools/testing/selftests/Makefile              |  1 +
 tools/testing/selftests/dt/.gitignore         |  1 +
 tools/testing/selftests/dt/Makefile           | 21 +++++
 .../selftests/dt/compatible_ignore_list       |  1 +
 tools/testing/selftests/dt/ktap_helpers.sh    | 57 +++++++++++++
 .../selftests/dt/test_unprobed_devices.sh     | 79 +++++++++++++++++++
 6 files changed, 160 insertions(+)
 create mode 100644 tools/testing/selftests/dt/.gitignore
 create mode 100644 tools/testing/selftests/dt/Makefile
 create mode 100644 tools/testing/selftests/dt/compatible_ignore_list
 create mode 100644 tools/testing/selftests/dt/ktap_helpers.sh
 create mode 100755 tools/testing/selftests/dt/test_unprobed_devices.sh

Comments

Mark Brown Aug. 18, 2023, 12:54 p.m. UTC | #1
On Thu, Aug 17, 2023 at 07:35:27PM -0400, Nícolas F. R. A. Prado wrote:

> --- /dev/null
> +++ b/tools/testing/selftests/dt/ktap_helpers.sh
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (c) 2023 Collabora Ltd
> +#
> +# Helpers for outputting in KTAP format
> +#

These look generic so could be at the top level kselftest directory in
case any other tests want to use them?

The test itself looks good in so far as I can read shell.
Rob Herring Aug. 18, 2023, 3:05 p.m. UTC | #2
On Thu, Aug 17, 2023 at 6:36 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Introduce a new kselftest to detect devices that were declared in the
> Devicetree, and are expected to be probed by a driver, but weren't.
>
> The test uses two lists: a list of compatibles that can match a
> Devicetree device to a driver, and a list of compatibles that should be
> ignored. The first is automatically generated by the
> dt-extract-compatibles script, and is run as part of building this test.
> The list of compatibles to ignore is a hand-crafted list to capture the
> few exceptions of compatibles that are expected to match a driver but
> not be bound to it.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>
> ---
>
> Changes in v2:
> - Switched output to be in KTAP format
> - Changed Makefile to make use of the dt-extract-compatibles instead of
>   the Coccinelle script
> - Dropped compatibles from compatible_ignore_list that are now already
>   filtered out by extraction script
> - Added early exit if /proc/device-tree is not present
>
>  tools/testing/selftests/Makefile              |  1 +
>  tools/testing/selftests/dt/.gitignore         |  1 +
>  tools/testing/selftests/dt/Makefile           | 21 +++++

Please add this path to DT maintainers entry.

>  .../selftests/dt/compatible_ignore_list       |  1 +
>  tools/testing/selftests/dt/ktap_helpers.sh    | 57 +++++++++++++

As Mark said, looks common.

>  .../selftests/dt/test_unprobed_devices.sh     | 79 +++++++++++++++++++
>  6 files changed, 160 insertions(+)
>  create mode 100644 tools/testing/selftests/dt/.gitignore
>  create mode 100644 tools/testing/selftests/dt/Makefile
>  create mode 100644 tools/testing/selftests/dt/compatible_ignore_list
>  create mode 100644 tools/testing/selftests/dt/ktap_helpers.sh
>  create mode 100755 tools/testing/selftests/dt/test_unprobed_devices.sh
>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 42806add0114..e8823097698c 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -18,6 +18,7 @@ TARGETS += drivers/dma-buf
>  TARGETS += drivers/s390x/uvdevice
>  TARGETS += drivers/net/bonding
>  TARGETS += drivers/net/team
> +TARGETS += dt
>  TARGETS += efivarfs
>  TARGETS += exec
>  TARGETS += fchmodat2
> diff --git a/tools/testing/selftests/dt/.gitignore b/tools/testing/selftests/dt/.gitignore
> new file mode 100644
> index 000000000000..f6476c9f2884
> --- /dev/null
> +++ b/tools/testing/selftests/dt/.gitignore
> @@ -0,0 +1 @@
> +compatible_list

Not sure on the selftests, but is this enough that it gets cleaned?

> diff --git a/tools/testing/selftests/dt/Makefile b/tools/testing/selftests/dt/Makefile
> new file mode 100644
> index 000000000000..62dc00ee4978
> --- /dev/null
> +++ b/tools/testing/selftests/dt/Makefile
> @@ -0,0 +1,21 @@
> +PY3 = $(shell which python3 2>/dev/null)
> +
> +ifneq ($(PY3),)
> +
> +TEST_PROGS := test_unprobed_devices.sh
> +TEST_GEN_FILES := compatible_list
> +TEST_FILES := compatible_ignore_list ktap_helpers.sh
> +
> +include ../lib.mk
> +
> +$(OUTPUT)/compatible_list:
> +       $(top_srcdir)/scripts/dtc/dt-extract-compatibles -d $(top_srcdir) > $@
> +
> +else
> +
> +all: no_py3_warning
> +
> +no_py3_warning:
> +       @echo "Missing python3. This test will be skipped."
> +
> +endif
> diff --git a/tools/testing/selftests/dt/compatible_ignore_list b/tools/testing/selftests/dt/compatible_ignore_list
> new file mode 100644
> index 000000000000..1323903feca9
> --- /dev/null
> +++ b/tools/testing/selftests/dt/compatible_ignore_list
> @@ -0,0 +1 @@
> +simple-mfd
> diff --git a/tools/testing/selftests/dt/ktap_helpers.sh b/tools/testing/selftests/dt/ktap_helpers.sh
> new file mode 100644
> index 000000000000..27e89a31e602
> --- /dev/null
> +++ b/tools/testing/selftests/dt/ktap_helpers.sh
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (c) 2023 Collabora Ltd
> +#
> +# Helpers for outputting in KTAP format
> +#
> +KTAP_TESTNO=1
> +
> +ktap_print_header() {
> +       echo "TAP version 13"
> +}
> +
> +ktap_set_plan() {
> +       num_tests="$1"
> +
> +       echo "1..$num_tests"
> +}
> +
> +ktap_skip_all() {
> +       echo -n "1..0 # SKIP "
> +       echo $@
> +}
> +
> +__ktap_test() {
> +       result="$1"
> +       description="$2"
> +       directive="$3" # optional
> +
> +       local directive_str=
> +       [[ ! -z "$directive" ]] && directive_str="# $directive"
> +
> +       echo $result $KTAP_TESTNO $description $directive_str
> +
> +       KTAP_TESTNO=$((KTAP_TESTNO+1))
> +}
> +
> +ktap_test_pass() {
> +       description="$1"
> +
> +       result="ok"
> +       __ktap_test "$result" "$description"
> +}
> +
> +ktap_test_skip() {
> +       description="$1"
> +
> +       result="ok"
> +       directive="SKIP"
> +       __ktap_test "$result" "$description" "$directive"
> +}
> +
> +ktap_test_fail() {
> +       description="$1"
> +
> +       result="not ok"
> +       __ktap_test "$result" "$description"
> +}
> diff --git a/tools/testing/selftests/dt/test_unprobed_devices.sh b/tools/testing/selftests/dt/test_unprobed_devices.sh
> new file mode 100755
> index 000000000000..b523767cdbfb
> --- /dev/null
> +++ b/tools/testing/selftests/dt/test_unprobed_devices.sh
> @@ -0,0 +1,79 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (c) 2023 Collabora Ltd
> +#
> +# Based on Frank Rowand's dt_stat script.
> +#
> +# This script tests for devices that were declared on the Devicetree and are
> +# expected to bind to a driver, but didn't.
> +#
> +# To achieve this, two lists are used:
> +# * a list of the compatibles that can be matched by a Devicetree node
> +# * a list of compatibles that should be ignored
> +#
> +
> +DIR="$(dirname $(readlink -f "$0"))"
> +
> +source "${DIR}"/ktap_helpers.sh
> +
> +PDT=/proc/device-tree/

This is considered the legacy path though we will probably never get
rid of it. Use the sysfs path instead.

> +COMPAT_LIST="${DIR}"/compatible_list
> +IGNORE_LIST="${DIR}"/compatible_ignore_list
> +
> +KSFT_PASS=0
> +KSFT_FAIL=1
> +KSFT_SKIP=4
> +
> +ktap_print_header
> +
> +if [[ ! -d "${PDT}" ]]; then
> +       ktap_skip_all "${PDT} doesn't exist."
> +       exit "${KSFT_SKIP}"
> +fi
> +
> +nodes_compatible=$(
> +       for node_compat in $(find ${PDT} -name compatible); do
> +               node=$(dirname "${node_compat}")
> +               # Check if node is available
> +               [[ -e "${node}"/status && $(tr -d '\000' < "${node}"/status) != "okay" ]] && continue

Note that "ok" is accepted by the kernel and does show up some. But
for your use, probably okay as is.

> +               echo "${node}" | sed -e 's|\/proc\/device-tree||'
> +       done | sort
> +       )
> +
> +nodes_dev_bound=$(
> +       IFS=$'\n'
> +       for uevent in $(find /sys/devices -name uevent); do
> +               if [[ -d "$(dirname "${uevent}")"/driver ]]; then
> +                       grep '^OF_FULLNAME=' "${uevent}" | sed -e 's|OF_FULLNAME=||'
> +               fi
> +       done
> +       )
> +
> +num_tests=$(echo ${nodes_compatible} | wc -w)
> +ktap_set_plan "${num_tests}"
> +
> +retval="${KSFT_PASS}"
> +for node in ${nodes_compatible}; do
> +       if ! echo "${nodes_dev_bound}" | grep -E -q "(^| )${node}( |\$)"; then
> +               compatibles=$(tr '\000' '\n' < "${PDT}"/"${node}"/compatible)
> +
> +               for compatible in ${compatibles}; do
> +                       if grep -x -q "${compatible}" "${IGNORE_LIST}"; then
> +                               continue
> +                       fi
> +
> +                       if grep -x -q "${compatible}" "${COMPAT_LIST}"; then
> +                               ktap_test_fail "${node}"
> +                               retval="${KSFT_FAIL}"
> +                               continue 2
> +                       fi
> +               done
> +               ktap_test_skip "${node}"
> +       else
> +               ktap_test_pass "${node}"
> +       fi
> +
> +done
> +
> +exit "${retval}"
> --
> 2.41.0
>
Shuah Khan Aug. 18, 2023, 4:11 p.m. UTC | #3
On 8/18/23 09:08, Nícolas F. R. A. Prado wrote:
> On Fri, Aug 18, 2023 at 01:54:21PM +0100, Mark Brown wrote:
>> On Thu, Aug 17, 2023 at 07:35:27PM -0400, Nícolas F. R. A. Prado wrote:
>>
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/dt/ktap_helpers.sh
>>> @@ -0,0 +1,57 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +#
>>> +# Copyright (c) 2023 Collabora Ltd
>>> +#
>>> +# Helpers for outputting in KTAP format
>>> +#
>>
>> These look generic so could be at the top level kselftest directory in
>> case any other tests want to use them?
> 
> Yes, they're generic. And sure, we can move it up. The tests using it will need
> to source it at run-time, so we can either update the kselftest Makefile to
> always copy this helper when installing, or each test's Makefile can
> make its own copy during build.
> 

Moving this up would require the above changes. I prefer
making these later after this test goes in to avoid conflicts
with linux-kselftest next and Rob's dt as this one depends
on  patches 1&2 which aren't in my Inbox.

I would like also  to see a common solution that works for C
and shell tests. Sourcing works just for shell tests.

>>
>> The test itself looks good in so far as I can read shell.
> 
> Thanks for the feedback!
>
Rob, Are you planning to take this through your tree. If you
do, here is my Reviewed-by

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah
Rob Herring Aug. 28, 2023, 4:37 p.m. UTC | #4
On Fri, Aug 18, 2023 at 11:38 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> On Fri, Aug 18, 2023 at 10:05:00AM -0500, Rob Herring wrote:
> > On Thu, Aug 17, 2023 at 6:36 PM Nícolas F. R. A. Prado
> > <nfraprado@collabora.com> wrote:
> [..]
> > >  tools/testing/selftests/Makefile              |  1 +
> > >  tools/testing/selftests/dt/.gitignore         |  1 +
> > >  tools/testing/selftests/dt/Makefile           | 21 +++++
> >
> > Please add this path to DT maintainers entry.
>
> OK.
>
> >
> > >  .../selftests/dt/compatible_ignore_list       |  1 +
> > >  tools/testing/selftests/dt/ktap_helpers.sh    | 57 +++++++++++++
> >
> > As Mark said, looks common.
>
> Yes, I'll move it one folder up.
>
> >
> [..]
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/dt/.gitignore
> > > @@ -0,0 +1 @@
> > > +compatible_list
> >
> > Not sure on the selftests, but is this enough that it gets cleaned?
>
> I've just double-checked that compatible_list does get removed with make clean.
> Not because it is in this gitignore, but because it is listed in TEST_GEN_FILES
> in the Makefile.
>
> >
> [..]
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/dt/test_unprobed_devices.sh
> [..]
> > > +PDT=/proc/device-tree/
> >
> > This is considered the legacy path though we will probably never get
> > rid of it. Use the sysfs path instead.
>
> The /sys/firmware/devicetree/* entry in
> Documentation/ABI/testing/sysfs-firmware-ofw reads:
>
> Userspace must not use the /sys/firmware/devicetree/base
> path directly, but instead should follow /proc/device-tree
> symlink. It is possible that the absolute path will change
> in the future, but the symlink is the stable ABI.
>
> So is this information outdated?

No, my memory is just wrong...

Rob
diff mbox series

Patch

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 42806add0114..e8823097698c 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -18,6 +18,7 @@  TARGETS += drivers/dma-buf
 TARGETS += drivers/s390x/uvdevice
 TARGETS += drivers/net/bonding
 TARGETS += drivers/net/team
+TARGETS += dt
 TARGETS += efivarfs
 TARGETS += exec
 TARGETS += fchmodat2
diff --git a/tools/testing/selftests/dt/.gitignore b/tools/testing/selftests/dt/.gitignore
new file mode 100644
index 000000000000..f6476c9f2884
--- /dev/null
+++ b/tools/testing/selftests/dt/.gitignore
@@ -0,0 +1 @@ 
+compatible_list
diff --git a/tools/testing/selftests/dt/Makefile b/tools/testing/selftests/dt/Makefile
new file mode 100644
index 000000000000..62dc00ee4978
--- /dev/null
+++ b/tools/testing/selftests/dt/Makefile
@@ -0,0 +1,21 @@ 
+PY3 = $(shell which python3 2>/dev/null)
+
+ifneq ($(PY3),)
+
+TEST_PROGS := test_unprobed_devices.sh
+TEST_GEN_FILES := compatible_list
+TEST_FILES := compatible_ignore_list ktap_helpers.sh
+
+include ../lib.mk
+
+$(OUTPUT)/compatible_list:
+	$(top_srcdir)/scripts/dtc/dt-extract-compatibles -d $(top_srcdir) > $@
+
+else
+
+all: no_py3_warning
+
+no_py3_warning:
+	@echo "Missing python3. This test will be skipped."
+
+endif
diff --git a/tools/testing/selftests/dt/compatible_ignore_list b/tools/testing/selftests/dt/compatible_ignore_list
new file mode 100644
index 000000000000..1323903feca9
--- /dev/null
+++ b/tools/testing/selftests/dt/compatible_ignore_list
@@ -0,0 +1 @@ 
+simple-mfd
diff --git a/tools/testing/selftests/dt/ktap_helpers.sh b/tools/testing/selftests/dt/ktap_helpers.sh
new file mode 100644
index 000000000000..27e89a31e602
--- /dev/null
+++ b/tools/testing/selftests/dt/ktap_helpers.sh
@@ -0,0 +1,57 @@ 
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2023 Collabora Ltd
+#
+# Helpers for outputting in KTAP format
+#
+KTAP_TESTNO=1
+
+ktap_print_header() {
+	echo "TAP version 13"
+}
+
+ktap_set_plan() {
+	num_tests="$1"
+
+	echo "1..$num_tests"
+}
+
+ktap_skip_all() {
+	echo -n "1..0 # SKIP "
+	echo $@
+}
+
+__ktap_test() {
+	result="$1"
+	description="$2"
+	directive="$3" # optional
+
+	local directive_str=
+	[[ ! -z "$directive" ]] && directive_str="# $directive"
+
+	echo $result $KTAP_TESTNO $description $directive_str
+
+	KTAP_TESTNO=$((KTAP_TESTNO+1))
+}
+
+ktap_test_pass() {
+	description="$1"
+
+	result="ok"
+	__ktap_test "$result" "$description"
+}
+
+ktap_test_skip() {
+	description="$1"
+
+	result="ok"
+	directive="SKIP"
+	__ktap_test "$result" "$description" "$directive"
+}
+
+ktap_test_fail() {
+	description="$1"
+
+	result="not ok"
+	__ktap_test "$result" "$description"
+}
diff --git a/tools/testing/selftests/dt/test_unprobed_devices.sh b/tools/testing/selftests/dt/test_unprobed_devices.sh
new file mode 100755
index 000000000000..b523767cdbfb
--- /dev/null
+++ b/tools/testing/selftests/dt/test_unprobed_devices.sh
@@ -0,0 +1,79 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2023 Collabora Ltd
+#
+# Based on Frank Rowand's dt_stat script.
+#
+# This script tests for devices that were declared on the Devicetree and are
+# expected to bind to a driver, but didn't.
+#
+# To achieve this, two lists are used:
+# * a list of the compatibles that can be matched by a Devicetree node
+# * a list of compatibles that should be ignored
+#
+
+DIR="$(dirname $(readlink -f "$0"))"
+
+source "${DIR}"/ktap_helpers.sh
+
+PDT=/proc/device-tree/
+COMPAT_LIST="${DIR}"/compatible_list
+IGNORE_LIST="${DIR}"/compatible_ignore_list
+
+KSFT_PASS=0
+KSFT_FAIL=1
+KSFT_SKIP=4
+
+ktap_print_header
+
+if [[ ! -d "${PDT}" ]]; then
+	ktap_skip_all "${PDT} doesn't exist."
+	exit "${KSFT_SKIP}"
+fi
+
+nodes_compatible=$(
+	for node_compat in $(find ${PDT} -name compatible); do
+		node=$(dirname "${node_compat}")
+		# Check if node is available
+		[[ -e "${node}"/status && $(tr -d '\000' < "${node}"/status) != "okay" ]] && continue
+		echo "${node}" | sed -e 's|\/proc\/device-tree||'
+	done | sort
+	)
+
+nodes_dev_bound=$(
+	IFS=$'\n'
+	for uevent in $(find /sys/devices -name uevent); do
+		if [[ -d "$(dirname "${uevent}")"/driver ]]; then
+			grep '^OF_FULLNAME=' "${uevent}" | sed -e 's|OF_FULLNAME=||'
+		fi
+	done
+	)
+
+num_tests=$(echo ${nodes_compatible} | wc -w)
+ktap_set_plan "${num_tests}"
+
+retval="${KSFT_PASS}"
+for node in ${nodes_compatible}; do
+	if ! echo "${nodes_dev_bound}" | grep -E -q "(^| )${node}( |\$)"; then
+		compatibles=$(tr '\000' '\n' < "${PDT}"/"${node}"/compatible)
+
+		for compatible in ${compatibles}; do
+			if grep -x -q "${compatible}" "${IGNORE_LIST}"; then
+				continue
+			fi
+
+			if grep -x -q "${compatible}" "${COMPAT_LIST}"; then
+				ktap_test_fail "${node}"
+				retval="${KSFT_FAIL}"
+				continue 2
+			fi
+		done
+		ktap_test_skip "${node}"
+	else
+		ktap_test_pass "${node}"
+	fi
+
+done
+
+exit "${retval}"