Re: [PATCH] damon: access_memory_even: remove unused variables
On 9/2/24 21:43, Ba Jing wrote:
These variables are never referenced in the code, just remove it.
remove them?
Add details on how you found this problem in the commit log.
Send v2 with these changes.
Signed-off-by: Ba Jing
---
tools/testing/selftests/damon/access_memory_even.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/damon/access_memory_even.c
b/tools/testing/selftests/damon/access_memory_even.c
index 3be121487432..a9f4e9aaf3a9 100644
--- a/tools/testing/selftests/damon/access_memory_even.c
+++ b/tools/testing/selftests/damon/access_memory_even.c
@@ -14,10 +14,8 @@
int main(int argc, char *argv[])
{
char **regions;
- clock_t start_clock;
int nr_regions;
int sz_region;
- int access_time_ms;
int i;
if (argc != 3) {
With these changes:
Reviewed-by: Shuah Khan
thanks,
-- Shuah
Re: [PATCH 0/2] unicode: kunit: refactor selftest to kunit tests
On 9/23/24 09:42, Pedro Orlando wrote:
+CC linux-kselftest
---
On 22/09/2024 17:16, Gabriela Bittencourt wrote:
Hey all,
We are making these changes as part of a KUnit Hackathon at LKCamp [1].
This patch sets out to refactor fs/unicode/utf8-selftest.c to KUnit tests.
The first commit is the refactoring itself from self test into KUnit, while
the second one applies the naming style conventions.
We appreciate any feedback and suggestions. :)
[1] https://lkcamp.dev/about/
Co-developed-by: Pedro Orlando
Co-developed-by: Danilo Pereira
Signed-off-by: Pedro Orlando
Signed-off-by: Danilo Pereira
Signed-off-by: Gabriela Bittencourt
Gabriela Bittencourt (2):
unicode: kunit: refactor selftest to kunit tests
unicode: kunit: change tests filename and path
fs/unicode/Kconfig | 5 +-
fs/unicode/Makefile | 2 +-
fs/unicode/tests/.kunitconfig | 3 +
.../{utf8-selftest.c => tests/utf8_kunit.c} | 152 --
4 files changed, 76 insertions(+), 86 deletions(-)
create mode 100644 fs/unicode/tests/.kunitconfig
rename fs/unicode/{utf8-selftest.c => tests/utf8_kunit.c} (63%)
Please resend the series with linux-kselftest on the cc.
thanks,
-- Shuah
Re: [PATCH v3 0/3] selftests: livepatch: test livepatching a kprobed function
On 9/23/24 08:45, Marcos Paulo de Souza wrote: On Fri, 2024-09-20 at 13:56 +0200, Michael Vetter wrote: This patchset adds a test for livepatching a kprobed function. Thanks to Petr and Marcos for the reviews! V3: Save and restore kprobe state also when test fails, by integrating it into setup_config() and cleanup(). Rename SYSFS variables in a more logical way. Sort test modules in alphabetical order. Rename module description. V2: Save and restore kprobe state. Michael Vetter (3): selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR selftests: livepatch: save and restore kprobe state selftests: livepatch: test livepatching a kprobed function Thanks for the new version! LGTM, so the series is Reviewed-by: Marcos Paulo de Souza tools/testing/selftests/livepatch/Makefile | 3 +- .../testing/selftests/livepatch/functions.sh | 13 +++- .../selftests/livepatch/test-kprobe.sh | 62 +++ .../selftests/livepatch/test_modules/Makefile | 3 +- .../livepatch/test_modules/test_klp_kprobe.c | 38 5 files changed, 114 insertions(+), 5 deletions(-) create mode 100755 tools/testing/selftests/livepatch/test-kprobe.sh create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c Assuming this is going through livepatch tree: Acked-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 1/2] kselftests: mm: Fix wrong __NR_userfaultfd value
On 9/22/24 23:35, Muhammad Usama Anjum wrote: ... grep -rnIF "#define __NR_userfaultfd" tools/include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282 arch/x86/include/generated/uapi/asm/unistd_32.h:374:#define __NR_userfaultfd 374 arch/x86/include/generated/uapi/asm/unistd_64.h:327:#define __NR_userfaultfd 323 arch/x86/include/generated/uapi/asm/unistd_x32.h:282:#define __NR_userfaultfd (__X32_SYSCALL_BIT + 323) arch/arm/include/generated/uapi/asm/unistd-eabi.h:347:#define __NR_userfaultfd (__NR_SYSCALL_BASE + 388) arch/arm/include/generated/uapi/asm/unistd-oabi.h:359:#define __NR_userfaultfd (__NR_SYSCALL_BASE + 388) include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282 The number is dependent on the architecture. The above data shows that: x86 374 x86_64 323 Correct and the generated header files do the right thing and it is good to include them as this patch does. This is a good find and fix. I wish you explained this in your changelog. Please add more details when you send v2. I'm sending v2 There could be other issues lurking based on what I found. The other two files are the problem where they hard code it to 282 without taking the __NR_SYSCALL_BASE for the arch into consideration: tools/include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282 include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282 I'm unable to find the history of why it is set to 282 in unistd.h and when this problem happened. According to git history it is added in the following commit to include/uapi/asm-generic/unistd.h: 09f7298100ea9767324298ab0c7979f6d7463183 Subject: [PATCH] userfaultfd: register uapi generic syscall (aarch64) and it is added in the following commit to tools/include/uapi/asm-generic/unistd.h 34b009cfde2b8ce20a69c7bfd6bad4ce0e7cd970 Subject: [PATCH] tools include: Grab copies of arm64 dependent unistd.h files I think, the above defines from include/uapi/asm-generic/unistd.h and tools/include/uapi/asm-generic/unistd.h should be removed. Maybe others familiar with userfaultfd can determine the best course of action. We might have other NR_ defines in these two files that are causing problems for tests and tools that we haven't uncovered yet. Added authors of these patches. Thank you. Would you be able top follow up on this and send patches to remove these defines if it deemed to be the correct solution? thanks, -- Shuah
Re: [PATCH v2] kselftests: mm: Fix wrong __NR_userfaultfd value
On 9/22/24 23:38, Muhammad Usama Anjum wrote:
grep -rnIF "#define __NR_userfaultfd"
tools/include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282
arch/x86/include/generated/uapi/asm/unistd_32.h:374:#define
__NR_userfaultfd 374
arch/x86/include/generated/uapi/asm/unistd_64.h:327:#define
__NR_userfaultfd 323
arch/x86/include/generated/uapi/asm/unistd_x32.h:282:#define
__NR_userfaultfd (__X32_SYSCALL_BIT + 323)
arch/arm/include/generated/uapi/asm/unistd-eabi.h:347:#define
__NR_userfaultfd (__NR_SYSCALL_BASE + 388)
arch/arm/include/generated/uapi/asm/unistd-oabi.h:359:#define
__NR_userfaultfd (__NR_SYSCALL_BASE + 388)
include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282
The number is dependent on the architecture. The above data shows that:
x86 374
x86_64 323
The value of __NR_userfaultfd was changed to 282 when
asm-generic/unistd.h was included. It makes the test to fail every time
as the correct number of this syscall on x86_64 is 323. Fix the header
to asm/unistd.h.
Fixes: a5c6bc590094 ("selftests/mm: remove local __NR_* definitions")
Signed-off-by: Muhammad Usama Anjum
---
tools/testing/selftests/mm/pagemap_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c
b/tools/testing/selftests/mm/pagemap_ioctl.c
index fc90af2a97b80..bcc73b4e805c6 100644
--- a/tools/testing/selftests/mm/pagemap_ioctl.c
+++ b/tools/testing/selftests/mm/pagemap_ioctl.c
@@ -15,7 +15,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
Thank you.
Reviewed-by: Shuah Khan
thanks,
-- Shuah
Re: [PATCH v3 0/1] Add KUnit tests for llist
On 9/20/24 20:49, Artur Alves Cavalcante de Barros wrote: On 9/20/24 4:10 AM, David Gow wrote: On Fri, 20 Sept 2024 at 00:01, Shuah Khan wrote: On 9/16/24 18:51, Artur Alves wrote: Hi all, This is part of a hackathon organized by LKCAMP[1], focused on writing tests using KUnit. We reached out a while ago asking for advice on what would be a useful contribution[2] and ended up choosing data structures that did not yet have tests. This patch adds tests for the llist data structure, defined in include/linux/llist.h, and is inspired by the KUnit tests for the doubly linked list in lib/list-test.c[3]. It is important to note that this patch depends on the patch referenced in [4], as it utilizes the newly created lib/tests/ subdirectory. [1] https://lkcamp.dev/about/ [2] https://lore.kernel.org/all/Zktnt7rjKryTh9-N@arch/ [3] https://elixir.bootlin.com/linux/latest/source/lib/list-test.c [4] https://lore.kernel.org/all/20240720181025.work.002-k...@kernel.org/ --- Changes in v3: - Resolved checkpatch warnings: - Renamed tests for macros starting with 'for_each' Shouldn't this a separate patch to make it easy to review? I think that, if this were renaming these in an already existing test (like the confusingly similar list test), then yes. But since it's only a change from v2, I think we're okay. Yes, the renaming refers to some test cases from the test suite that I'm adding, with the purpose of resolving some checkpatch warnings, as suggested by Rae Moar's review[1]. - Removed link from commit message - Replaced hardcoded constants with ENTRIES_SIZE Shouldn't this a separate patch to make it easy to review? Again, if we want to change this in other tests (list, hlist) we should split it into a separate patch, but I think it's okay for llist to go in with these already cleaned up. - Updated initialization of llist_node array - Fixed typos - Update Kconfig.debug message for llist_kunit Are these changes to existing code or warnings on your added code? I think these are all changes to the added code since v2. Artur, is that right? This is the case! All changes are in the added code, so it doesn't introduce any checkpatch warnings that were present in v2. Changes in v2: - Add MODULE_DESCRIPTION() - Move the tests from lib/llist_kunit.c to lib/tests/llist_kunit.c - Change the license from "GPL v2" to "GPL" Artur Alves (1): lib/llist_kunit.c: add KUnit tests for llist lib/Kconfig.debug | 11 ++ lib/tests/Makefile | 1 + lib/tests/llist_kunit.c | 358 3 files changed, 370 insertions(+) create mode 100644 lib/tests/llist_kunit.c You are combining lot of changes in one single patch. Each change as a separate patch will help reviewers. Adding new test should be a separate patch. - renaming as a separate patch I think given that these are just changes between patch versions, not renaming/modifying already committed code, that this is okay to go in as one patch? The actual patch is only doing one thing: adding a test suite for the llist structure. I don't see the point in committing a version of it only to immediately rename things and clean bits up separately in this case. Cheers, -- David Thanks for replying! I'd like to reaffirm that the patch is, in fact, doing one thing: adding tests for the llist data structure. All the changes in V2 and V3 refer to the code that I'm adding. I'm not modifying any existing list tests, only adding new ones. [1] https://lore.kernel.org/all/20240903214027.77533-1-artur...@gmail.com/T/#mc29a53b120d2f8589f8bd882ab972d15c8a3d202 Best regards, - Artur Sounds good to me. It was a bit confusing because the v2 and v3 change new and code which wasn't clear to me. thanks, -- Shuah
Re: [PATCH v3 0/1] Add KUnit tests for llist
On 9/20/24 01:10, David Gow wrote: On Fri, 20 Sept 2024 at 00:01, Shuah Khan wrote: On 9/16/24 18:51, Artur Alves wrote: Hi all, This is part of a hackathon organized by LKCAMP[1], focused on writing tests using KUnit. We reached out a while ago asking for advice on what would be a useful contribution[2] and ended up choosing data structures that did not yet have tests. This patch adds tests for the llist data structure, defined in include/linux/llist.h, and is inspired by the KUnit tests for the doubly linked list in lib/list-test.c[3]. It is important to note that this patch depends on the patch referenced in [4], as it utilizes the newly created lib/tests/ subdirectory. [1] https://lkcamp.dev/about/ [2] https://lore.kernel.org/all/Zktnt7rjKryTh9-N@arch/ [3] https://elixir.bootlin.com/linux/latest/source/lib/list-test.c [4] https://lore.kernel.org/all/20240720181025.work.002-k...@kernel.org/ --- Changes in v3: - Resolved checkpatch warnings: - Renamed tests for macros starting with 'for_each' Shouldn't this a separate patch to make it easy to review? I think that, if this were renaming these in an already existing test (like the confusingly similar list test), then yes. But since it's only a change from v2, I think we're okay. - Removed link from commit message - Replaced hardcoded constants with ENTRIES_SIZE Shouldn't this a separate patch to make it easy to review? Again, if we want to change this in other tests (list, hlist) we should split it into a separate patch, but I think it's okay for llist to go in with these already cleaned up. - Updated initialization of llist_node array - Fixed typos - Update Kconfig.debug message for llist_kunit Are these changes to existing code or warnings on your added code? I think these are all changes to the added code since v2. Artur, is that right? Changes in v2: - Add MODULE_DESCRIPTION() - Move the tests from lib/llist_kunit.c to lib/tests/llist_kunit.c - Change the license from "GPL v2" to "GPL" Artur Alves (1): lib/llist_kunit.c: add KUnit tests for llist lib/Kconfig.debug | 11 ++ lib/tests/Makefile | 1 + lib/tests/llist_kunit.c | 358 3 files changed, 370 insertions(+) create mode 100644 lib/tests/llist_kunit.c You are combining lot of changes in one single patch. Each change as a separate patch will help reviewers. Adding new test should be a separate patch. - renaming as a separate patch I think given that these are just changes between patch versions, not renaming/modifying already committed code, that this is okay to go in as one patch? The actual patch is only doing one thing: adding a test suite for the llist structure. I don't see the point in committing a version of it only to immediately rename things and clean bits up separately in this case. I do think it will help to separate the renaming and adding a new test. It makes it easier to follow. thanks, -- Shuah
Re: [PATCH 1/2] kselftests: mm: Fix wrong __NR_userfaultfd value
On 9/17/24 23:46, Muhammad Usama Anjum wrote: On 9/17/24 6:56 AM, Shuah Khan wrote: On 9/16/24 00:32, Muhammad Usama Anjum wrote: On 9/12/24 8:44 PM, Shuah Khan wrote: On 9/12/24 04:31, Muhammad Usama Anjum wrote: The value of __NR_userfaultfd was changed to 282 when asm-generic/unistd.h was included. It makes the test to fail every time as the correct number of this syscall on x86_64 is 323. Fix the header to asm/unistd.h. "please elaborate every time" - I just built on my x86_64 and built just fine. The build isn't broken. I am not saying this isn't a problem, it is good to understand why and how it is failing before making the change. I mean to say that the test is failing at run time because the correct userfaultfd syscall isn't being found with __NR_userfaultfd = 282. _NR_userfaultfd's value depends on the header. When asm-generic/unistd.h is included, its value (282) is wrong. I've tested on x86_64. Okay - how do you know this is wrong? can you provide more details. git grep _NR_userfaultfd include/uapi/asm-generic/unistd.h:#define __NR_userfaultfd 282 include/uapi/asm-generic/unistd.h:__SYSCALL(__NR_userfaultfd, sys_userfaultfd) tools/include/uapi/asm-generic/unistd.h:#define __NR_userfaultfd 282 The fix is simple. Add the correct header which has _NR_userfaultfd = 323. grep -rnIF "#define __NR_userfaultfd" tools/include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282 arch/x86/include/generated/uapi/asm/unistd_32.h:374:#define __NR_userfaultfd 374 arch/x86/include/generated/uapi/asm/unistd_64.h:327:#define __NR_userfaultfd 323 arch/x86/include/generated/uapi/asm/unistd_x32.h:282:#define __NR_userfaultfd (__X32_SYSCALL_BIT + 323) arch/arm/include/generated/uapi/asm/unistd-eabi.h:347:#define __NR_userfaultfd (__NR_SYSCALL_BASE + 388) arch/arm/include/generated/uapi/asm/unistd-oabi.h:359:#define __NR_userfaultfd (__NR_SYSCALL_BASE + 388) include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282 The number is dependent on the architecture. The above data shows that: x86 374 x86_64 323 Correct and the generated header files do the right thing and it is good to include them as this patch does. This is a good find and fix. I wish you explained this in your changelog. Please add more details when you send v2. There could be other issues lurking based on what I found. The other two files are the problem where they hard code it to 282 without taking the __NR_SYSCALL_BASE for the arch into consideration: tools/include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282 include/uapi/asm-generic/unistd.h:681:#define __NR_userfaultfd 282 I'm unable to find the history of why it is set to 282 in unistd.h and when this problem happened. According to git history it is added in the following commit to include/uapi/asm-generic/unistd.h: 09f7298100ea9767324298ab0c7979f6d7463183 Subject: [PATCH] userfaultfd: register uapi generic syscall (aarch64) and it is added in the following commit to tools/include/uapi/asm-generic/unistd.h 34b009cfde2b8ce20a69c7bfd6bad4ce0e7cd970 Subject: [PATCH] tools include: Grab copies of arm64 dependent unistd.h files I think, the above defines from include/uapi/asm-generic/unistd.h and tools/include/uapi/asm-generic/unistd.h should be removed. Maybe others familiar with userfaultfd can determine the best course of action. We might have other NR_ defines in these two files that are causing problems for tests and tools that we haven't uncovered yet. thanks, -- Shuah
Re: [PATCH v3 0/1] Add KUnit tests for llist
On 9/16/24 18:51, Artur Alves wrote: Hi all, This is part of a hackathon organized by LKCAMP[1], focused on writing tests using KUnit. We reached out a while ago asking for advice on what would be a useful contribution[2] and ended up choosing data structures that did not yet have tests. This patch adds tests for the llist data structure, defined in include/linux/llist.h, and is inspired by the KUnit tests for the doubly linked list in lib/list-test.c[3]. It is important to note that this patch depends on the patch referenced in [4], as it utilizes the newly created lib/tests/ subdirectory. [1] https://lkcamp.dev/about/ [2] https://lore.kernel.org/all/Zktnt7rjKryTh9-N@arch/ [3] https://elixir.bootlin.com/linux/latest/source/lib/list-test.c [4] https://lore.kernel.org/all/20240720181025.work.002-k...@kernel.org/ --- Changes in v3: - Resolved checkpatch warnings: - Renamed tests for macros starting with 'for_each' Shouldn't this a separate patch to make it easy to review? - Removed link from commit message - Replaced hardcoded constants with ENTRIES_SIZE Shouldn't this a separate patch to make it easy to review? - Updated initialization of llist_node array - Fixed typos - Update Kconfig.debug message for llist_kunit Are these changes to existing code or warnings on your added code? Changes in v2: - Add MODULE_DESCRIPTION() - Move the tests from lib/llist_kunit.c to lib/tests/llist_kunit.c - Change the license from "GPL v2" to "GPL" Artur Alves (1): lib/llist_kunit.c: add KUnit tests for llist lib/Kconfig.debug | 11 ++ lib/tests/Makefile | 1 + lib/tests/llist_kunit.c | 358 3 files changed, 370 insertions(+) create mode 100644 lib/tests/llist_kunit.c You are combining lot of changes in one single patch. Each change as a separate patch will help reviewers. Adding new test should be a separate patch. - renaming as a separate patch thanks, -- Shuah
Re: [PATCH] selftests: Makefile: create OUTPUT dir
On 9/16/24 01:56, Anders Roxell wrote: When cross building kselftest out-of-tree the following issue can be seen: [...] make[4]: Entering directory '/src/kernel/linux/tools/testing/selftests/net/lib' CC csum /usr/lib/gcc-cross/aarch64-linux-gnu/13/../../../../aarch64-linux-gnu/bin/ld: cannot open output file /tmp/build/kselftest/net/lib/csum: No such file or directory collect2: error: ld returned 1 exit status [...] Create the output build directory before building the targets, solves this issue with building 'net/lib/csum'. Suggested-by: Jakub Kicinski Signed-off-by: Anders Roxell --- tools/testing/selftests/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index b38199965f99..05c143bcff6a 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -261,6 +261,7 @@ ifdef INSTALL_PATH @ret=1; \ for TARGET in $(TARGETS) $(INSTALL_DEP_TARGETS); do \ BUILD_TARGET=$$BUILD/$$TARGET; \ + mkdir -p $$BUILD_TARGET;\ $(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET install \ INSTALL_PATH=$(INSTALL_PATH)/$$TARGET \ SRC_PATH=$(shell readlink -e $$(pwd)) \ Doesn't the "all" target mkdir work for this case? Why do we need another mkdir here? thanks, -- Shuah
Re: [PATCH v2] selftests/vDSO: support DT_GNU_HASH
On 9/15/24 00:49, Fangrui Song wrote: glibc added support for DT_GNU_HASH in 2006 and DT_HASH has been obsoleted for more than one decade in many Linux distributions. Many vDSOs support DT_GNU_HASH. This patch adds selftests support. Signed-off-by: Fangrui Song Tested-by: Xi Ruoyao -- Changes from v1: * fix style of a multi-line comment. ignore false positive suggestions from checkpath.pl: `ELF(Word) *` --- tools/testing/selftests/vDSO/parse_vdso.c | 105 -- 1 file changed, 79 insertions(+), 26 deletions(-) Quick note that this will be picked up after the merge window closes. thanks, -- Shuah
Re: [PATCH 1/2] kselftests: mm: Fix wrong __NR_userfaultfd value
On 9/16/24 00:32, Muhammad Usama Anjum wrote: On 9/12/24 8:44 PM, Shuah Khan wrote: On 9/12/24 04:31, Muhammad Usama Anjum wrote: The value of __NR_userfaultfd was changed to 282 when asm-generic/unistd.h was included. It makes the test to fail every time as the correct number of this syscall on x86_64 is 323. Fix the header to asm/unistd.h. "please elaborate every time" - I just built on my x86_64 and built just fine. The build isn't broken. I am not saying this isn't a problem, it is good to understand why and how it is failing before making the change. I mean to say that the test is failing at run time because the correct userfaultfd syscall isn't being found with __NR_userfaultfd = 282. _NR_userfaultfd's value depends on the header. When asm-generic/unistd.h is included, its value (282) is wrong. I've tested on x86_64. Okay - how do you know this is wrong? can you provide more details. git grep _NR_userfaultfd include/uapi/asm-generic/unistd.h:#define __NR_userfaultfd 282 include/uapi/asm-generic/unistd.h:__SYSCALL(__NR_userfaultfd, sys_userfaultfd) tools/include/uapi/asm-generic/unistd.h:#define __NR_userfaultfd 282 The fix is simple. Add the correct header which has _NR_userfaultfd = 323. I need more details on this number. thanks, -- Shuah
[GIT PULL] KUnit update for Linux 6.12-rc1
Hi Linus, Please pull the following kunit update for Linux 6.12-rc1. This kunit update for Linux 6.12-rc1 consists of: -- a new int_pow test suite -- documentation update to clarify filename best practices -- kernel-doc fix for EXPORT_SYMBOL_IF_KUNIT -- change to build compile_commands.json automatically instead of requiring a manual build. diff is attached. thanks, -- Shuah The following changes since commit 8400291e289ee6b2bf9779ff1c83a291501f017b: Linux 6.11-rc1 (2024-07-28 14:19:55 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest tags/linux_kselftest-kunit-6.12-rc1 for you to fetch changes up to 7fcc9b53216cd87f73cc6dbb404220350ddc93b8: lib/math: Add int_pow test suite (2024-09-12 10:03:00 -0600) linux_kselftest-kunit-6.12-rc1 This kunit update for Linux 6.12-rc1 consists of: -- a new int_pow test suite -- documentation update to clarify filename best practices -- kernel-doc fix for EXPORT_SYMBOL_IF_KUNIT -- change to build compile_commands.json automatically instead of requiring a manual build. Brendan Jackman (1): kunit: tool: Build compile_commands.json Kees Cook (1): Documentation: KUnit: Update filename best practices Luis Felipe Hernandez (1): lib/math: Add int_pow test suite Michal Wajdeczko (1): kunit: Fix kernel-doc for EXPORT_SYMBOL_IF_KUNIT Documentation/dev-tools/kunit/style.rst | 29 -- include/kunit/visibility.h | 1 + lib/Kconfig.debug | 16 ++ lib/math/Makefile | 1 + lib/math/tests/Makefile | 3 ++ lib/math/tests/int_pow_kunit.c | 52 + tools/testing/kunit/kunit_kernel.py | 3 +- 7 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 lib/math/tests/Makefile create mode 100644 lib/math/tests/int_pow_kunit.c diff --git a/Documentation/dev-tools/kunit/style.rst b/Documentation/dev-tools/kunit/style.rst index b6d0d7359f00..eac81a714a29 100644 --- a/Documentation/dev-tools/kunit/style.rst +++ b/Documentation/dev-tools/kunit/style.rst @@ -188,15 +188,26 @@ For example, a Kconfig entry might look like: Test File and Module Names == -KUnit tests can often be compiled as a module. These modules should be named -after the test suite, followed by ``_test``. If this is likely to conflict with -non-KUnit tests, the suffix ``_kunit`` can also be used. +KUnit tests are often compiled as a separate module. To avoid conflicting +with regular modules, KUnit modules should be named after the test suite, +followed by ``_kunit`` (e.g. if "foobar" is the core module, then +"foobar_kunit" is the KUnit test module). -The easiest way of achieving this is to name the file containing the test suite -``_test.c`` (or, as above, ``_kunit.c``). This file should be -placed next to the code under test. +Test source files, whether compiled as a separate module or an +``#include`` in another source file, are best kept in a ``tests/`` +subdirectory to not conflict with other source files (e.g. for +tab-completion). + +Note that the ``_test`` suffix has also been used in some existing +tests. The ``_kunit`` suffix is preferred, as it makes the distinction +between KUnit and non-KUnit tests clearer. + +So for the common case, name the file containing the test suite +``tests/_kunit.c``. The ``tests`` directory should be placed at +the same level as the code under test. For example, tests for +``lib/string.c`` live in ``lib/tests/string_kunit.c``. If the suite name contains some or all of the name of the test's parent -directory, it may make sense to modify the source filename to reduce redundancy. -For example, a ``foo_firmware`` suite could be in the ``foo/firmware_test.c`` -file. +directory, it may make sense to modify the source filename to reduce +redundancy. For example, a ``foo_firmware`` suite could be in the +``foo/tests/firmware_kunit.c`` file. diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h index 0dfe35feeec6..efff77b58dd6 100644 --- a/include/kunit/visibility.h +++ b/include/kunit/visibility.h @@ -22,6 +22,7 @@ * EXPORTED_FOR_KUNIT_TESTING namespace only if CONFIG_KUNIT is * enabled. Must use MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING) * in test file in order to use symbols. + * @symbol: the symbol identifier to export */ #define EXPORT_SYMBOL_IF_KUNIT(symbol) EXPORT_SYMBOL_NS(symbol, \ EXPORTED_FOR_KUNIT_TESTING) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a30c03a66172..b5696659f027 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -
[GIT PULL] Kselftest update for Linux 6.12-rc1
Hi Linus, Please pull the following kselftest next update for Linux 6.12-rc1. This kselftest update for Linux 6.12-rc1 consists of: -- test coverage for dup_fd() failure handling in unshare_fd() -- new selftest for the acct() syscall -- basic uprobe testcase -- several small fixes and cleanups to existing tests -- user and strscpy removal as they became kunit tests -- fixes to build failures and warnings diff is attached. thanks, -- Shuah The following changes since commit 8400291e289ee6b2bf9779ff1c83a291501f017b: Linux 6.11-rc1 (2024-07-28 14:19:55 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest tags/linux_kselftest-next-6.12-rc1 for you to fetch changes up to a0474b8d5974e142461ac7584c996feea167bcc1: selftests: kselftest: Use strerror() on nolibc (2024-09-11 09:52:33 -0600) linux_kselftest-next-6.12-rc1 This kselftest update for Linux 6.12-rc1 consists of: -- test coverage for dup_fd() failure handling in unshare_fd() -- new selftest for the acct() syscall -- basic uprobe testcase -- several small fixes and cleanups to existing tests -- user and strscpy removal as they became kunit tests -- fixes to build failures and warnings Abdulrasaq Lawani (1): selftest: acct: Add selftest for the acct() syscall Abhinav Jain (1): selftests: filesystems: fix warn_unused_result build warnings Al Viro (1): selftests:core: test coverage for dup_fd() failure handling in unshare_fd() Anders Roxell (2): selftests: rust: config: add trailing newline selftests: rust: config: disable GCC_PLUGINS Chang Yu (1): selftests/exec: Fix grammar in an error message. Masahiro Yamada (2): selftests: harness: remove unneeded __constructor_order_last() selftests: harness: rename __constructor_order for clarification Masami Hiramatsu (Google) (3): selftests/uprobes: Add a basic uprobe testcase selftests/ftrace: Add required dependency for kprobe tests selftests/ftrace: Fix eventfs ownership testcase to find mount point Muhammad Usama Anjum (3): selftests: tpm2: redirect python unittest logs to stdout selftests: user: remove user suite selftests: lib: remove strscpy test Piotr Zalewski (1): kselftest: timers: Fix const correctness Shreeya Patel (1): kselftest: cpufreq: Add RTC wakeup alarm Shuah Khan (1): selftests:resctrl: Fix build failure on archs without __cpuid_count() Steven Rostedt (Google) (2): tracing/selftests: Run the ownership test twice selftests/ftrace: Fix test to handle both old and new kernels zhang jiao (2): selftests/timers: Remove unused NSEC_PER_SEC macro selftests: kselftest: Use strerror() on nolibc tools/testing/selftests/Makefile | 2 +- tools/testing/selftests/acct/.gitignore| 3 + tools/testing/selftests/acct/Makefile | 5 ++ tools/testing/selftests/acct/acct_syscall.c| 78 ++ tools/testing/selftests/core/Makefile | 2 +- tools/testing/selftests/core/unshare_test.c| 94 ++ tools/testing/selftests/cpufreq/cpufreq.sh | 15 tools/testing/selftests/cpufreq/main.sh| 13 ++- .../drivers/s390x/uvdevice/test_uvdevice.c | 6 -- tools/testing/selftests/exec/execveat.c| 2 +- .../filesystems/statmount/statmount_test_ns.c | 7 +- .../ftrace/test.d/00basic/test_ownership.tc| 46 +++ .../ftrace/test.d/dynevent/add_remove_uprobe.tc| 26 ++ .../ftrace/test.d/ftrace/func_set_ftrace_file.tc | 9 ++- .../ftrace/test.d/kprobe/kprobe_args_char.tc | 2 +- .../ftrace/test.d/kprobe/kprobe_args_string.tc | 2 +- tools/testing/selftests/hid/hid_bpf.c | 6 -- tools/testing/selftests/kselftest.h| 10 +-- tools/testing/selftests/kselftest_harness.h| 18 + tools/testing/selftests/lib/Makefile | 3 +- tools/testing/selftests/lib/config | 1 - tools/testing/selftests/lib/strscpy.sh | 3 - tools/testing/selftests/resctrl/cat_test.c | 7 +- tools/testing/selftests/rtc/rtctest.c | 7 -- tools/testing/selftests/rust/config| 3 +- tools/testing/selftests/timers/change_skew.c | 3 - tools/testing/selftests/timers/skew_consistency.c | 2 - tools/testing/selftests/timers/threadtest.c| 4 +- tools/testing/selftests/tpm2/test_async.sh | 2 +- tools/testing/selftests/tpm2/test_smoke.sh | 2 +- tools/testing/selftests/tpm2/test_space.sh | 2 +- tools/testing/selftests/user/Makefile | 9 --- tools/testing/selftests/user/config| 1 - tools
[GIT PULL] nolibc for 6.12-rc1
Hi Linus,
Please pull the following nolibc update for Linux 6.12-rc1.
This nolibc update for Linux 6.12-rc1 consists of:
Highlights
--
* Clang support (including LTO)
Other Changes
-
* stdbool.h support
* argc/argv/envp arguments for constructors
* Small #include ordering fix
Test Results:
Passed:
tools/testing/selftests/nolibc/run-tests.sh
tools/testing/selftests/nolibc/run-tests.sh -m user
diff is attached.
thanks,
-- Shuah
The following changes since commit 8400291e289ee6b2bf9779ff1c83a291501f017b:
Linux 6.11-rc1 (2024-07-28 14:19:55 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest
tags/linux_kselftest-nolibc-6.12-rc1
for you to fetch changes up to 248f6b935bbd8f7bc211cce2b6fd76be4c449848:
Merge tag 'nolibc-20240824-for-6.12-1' of
https://git.kernel.org/pub/scm/linux/kernel/git/nolibc/linux-nolibc into nolibc
(2024-08-27 06:43:34 -0600)
linux_kselftest-nolibc-6.12-rc1
This nolibc update for Linux 6.12-rc1 consists of:
Highlights
--
* Clang support (including LTO)
Other Changes
-
* stdbool.h support
* argc/argv/envp arguments for constructors
* Small #include ordering fix
----
Shuah Khan (1):
Merge tag 'nolibc-20240824-for-6.12-1' of
https://git.kernel.org/pub/scm/linux/kernel/git/nolibc/linux-nolibc into nolibc
Thomas Weißschuh (21):
tools/nolibc: include arch.h from string.h
tools/nolibc: add stdbool.h header
tools/nolibc: pass argc, argv and envp to constructors
tools/nolibc: arm: use clang-compatible asm syntax
tools/nolibc: mips: load current function to $t9
tools/nolibc: powerpc: limit stack-protector workaround to GCC
tools/nolibc: compiler: introduce __nolibc_has_attribute()
tools/nolibc: move entrypoint specifics to compiler.h
tools/nolibc: compiler: use attribute((naked)) if available
selftests/nolibc: report failure if no testcase passed
selftests/nolibc: avoid passing NULL to printf("%s")
selftests/nolibc: determine $(srctree) first
selftests/nolibc: add support for LLVM= parameter
selftests/nolibc: add cc-option compatible with clang cross builds
selftests/nolibc: run-tests.sh: avoid overwriting CFLAGS_EXTRA
selftests/nolibc: don't use libgcc when building with clang
selftests/nolibc: use correct clang target for s390/systemz
selftests/nolibc: run-tests.sh: allow building through LLVM
tools/nolibc: crt: mark _start_c() as used
tools/nolibc: stackprotector: mark implicitly used symbols as used
tools/nolibc: x86_64: use local label in memcpy/memmove
tools/include/nolibc/Makefile| 1 +
tools/include/nolibc/arch-aarch64.h | 4 +--
tools/include/nolibc/arch-arm.h | 8 +++---
tools/include/nolibc/arch-i386.h | 4 +--
tools/include/nolibc/arch-loongarch.h| 4 +--
tools/include/nolibc/arch-mips.h | 8 --
tools/include/nolibc/arch-powerpc.h | 6 ++--
tools/include/nolibc/arch-riscv.h| 4 +--
tools/include/nolibc/arch-s390.h | 4 +--
tools/include/nolibc/arch-x86_64.h | 8 +++---
tools/include/nolibc/compiler.h | 24 +++-
tools/include/nolibc/crt.h | 25 +
tools/include/nolibc/nolibc.h| 3 +-
tools/include/nolibc/stackprotector.h| 4 +--
tools/include/nolibc/stdbool.h | 16 +++
tools/include/nolibc/string.h| 1 +
tools/testing/selftests/nolibc/Makefile | 41 +++-
tools/testing/selftests/nolibc/nolibc-test.c | 9 +++---
tools/testing/selftests/nolibc/run-tests.sh | 16 ---
19 files changed, 123 insertions(+), 67 deletions(-)
create mode 100644 tools/include/nolibc/stdbool.h
diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile
index e69c26abe1ea..a1f55fb24bb3 100644
--- a/tools/include/nolibc/Makefile
+++ b/tools/include/nolibc/Makefile
@@ -35,6 +35,7 @@ all_files := \
stackprotector.h \
std.h \
stdarg.h \
+ stdbool.h \
stdint.h \
stdlib.h \
string.h \
diff --git a/tools/include/nolibc/arch-aarch64.h b/tools/include/nolibc/arch-aarch64.h
index b23ac1f04035..06fdef7b291a 100644
--- a/tools/include/nolibc/arch-aarch64.h
+++ b/tools/include/nolibc/arch-aarch64.h
@@ -142,13 +142,13 @@
})
/* startup code */
-void __attribute__((weak, noreturn, optimize("Os", "omit-frame-pointer"))) __no_stack_protector _start(void)
+void __attribute__((weak, noreturn)) __nolibc_entrypoint __no_stack_protecto
Re: [PATCH 2/2] kselftests: mm: Fail the test if userfaultfd syscall isn't found
On 9/12/24 10:10, Shuah Khan wrote:
On 9/12/24 04:31, Muhammad Usama Anjum wrote:
The userfaultfd is enabled in the config fragment of mm selftest suite.
It must always be present. If it isn't present, we should throw error
and not just skip. This would have helped us catch the test breakage.
Please elaborate on this to help understand the what breakage was
missed.
Also this commit log doesn't look right to me. syscall() could
fail for any reason. Do you mean to see skip is incorrect in this
error leg? Please see comments below.
Adding this now to catch the future breakages.
Signed-off-by: Muhammad Usama Anjum
---
tools/testing/selftests/mm/pagemap_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c
b/tools/testing/selftests/mm/pagemap_ioctl.c
index bcc73b4e805c6..d83dda8edf62c 100644
--- a/tools/testing/selftests/mm/pagemap_ioctl.c
+++ b/tools/testing/selftests/mm/pagemap_ioctl.c
@@ -95,7 +95,7 @@ int init_uffd(void)
uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK |
UFFD_USER_MODE_ONLY);
if (uffd == -1)
- return uffd;
+ ksft_exit_fail_perror("Userfaultfd syscall failed");
This looks wrong to me - Is missing config the only reason this syscall
would fail?
It should still skip if __NR_userfaultfd isn't supported on a release
or an architecture.
The real problem seems to be in main():
if (init_uffd())
ksft_exit_pass();
Why is this ksft_exit_pass()? Looks like further investigation is
necessary to understand the problem and fix.
thanks,
-- Shuah
Re: [PATCH] selftests: Makefile: add missing 'net/lib' to targets
On 9/12/24 09:23, Jakub Kicinski wrote:
On Thu, 12 Sep 2024 08:31:18 +0200 Anders Roxell wrote:
Fixes: 1d0dc857b5d8 ("selftests: drv-net: add checksum tests")
Signed-off-by: Anders Roxell
---
tools/testing/selftests/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 3b7df5477317..fc3681270afe 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -64,6 +64,7 @@ TARGETS += net
TARGETS += net/af_unix
TARGETS += net/forwarding
TARGETS += net/hsr
+TARGETS += net/lib
TARGETS += net/mptcp
TARGETS += net/netfilter
TARGETS += net/openvswitch
Please make sure you always include a commit message. Among other
things writing one would force you to understand the code, and
in this case understand that this target is intentionally left out.
Look around the Makefile for references to net/lib, you'll figure
it out.
+1 - thank you for outlining the benefits of writing a change log
which includes the details.
This patch is missing the changelog completely - change log is
an important part of sending a patch.
The patch is incorrect.
thanks,
-- Shuah
Re: [PATCH 2/2] kselftests: mm: Fail the test if userfaultfd syscall isn't found
On 9/12/24 04:31, Muhammad Usama Anjum wrote:
The userfaultfd is enabled in the config fragment of mm selftest suite.
It must always be present. If it isn't present, we should throw error
and not just skip. This would have helped us catch the test breakage.
Please elaborate on this to help understand the what breakage was
missed.
Also this commit log doesn't look right to me. syscall() could
fail for any reason. Do you mean to see skip is incorrect in this
error leg? Please see comments below.
Adding this now to catch the future breakages.
Signed-off-by: Muhammad Usama Anjum
---
tools/testing/selftests/mm/pagemap_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c
b/tools/testing/selftests/mm/pagemap_ioctl.c
index bcc73b4e805c6..d83dda8edf62c 100644
--- a/tools/testing/selftests/mm/pagemap_ioctl.c
+++ b/tools/testing/selftests/mm/pagemap_ioctl.c
@@ -95,7 +95,7 @@ int init_uffd(void)
uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK | UFFD_USER_MODE_ONLY);
if (uffd == -1)
- return uffd;
+ ksft_exit_fail_perror("Userfaultfd syscall failed");
This looks wrong to me - Is missing config the only reason this syscall
would fail?
uffdio_api.api = UFFD_API;
uffdio_api.features = UFFD_FEATURE_WP_UNPOPULATED |
UFFD_FEATURE_WP_ASYNC |
thanks,
-- Shuah
Re: [PATCH 1/2] kselftests: mm: Fix wrong __NR_userfaultfd value
On 9/12/24 04:31, Muhammad Usama Anjum wrote:
The value of __NR_userfaultfd was changed to 282 when
asm-generic/unistd.h was included. It makes the test to fail every time
as the correct number of this syscall on x86_64 is 323. Fix the header
to asm/unistd.h.
"please elaborate every time" - I just built on my x86_64 and built
just fine. I am not saying this isn't a problem, it is good to
understand why and how it is failing before making the change.
Fixes: a5c6bc590094 ("selftests/mm: remove local __NR_* definitions")
Signed-off-by: Muhammad Usama Anjum
---
tools/testing/selftests/mm/pagemap_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c
b/tools/testing/selftests/mm/pagemap_ioctl.c
index fc90af2a97b80..bcc73b4e805c6 100644
--- a/tools/testing/selftests/mm/pagemap_ioctl.c
+++ b/tools/testing/selftests/mm/pagemap_ioctl.c
@@ -15,7 +15,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
Also please generate a series with these two patches with cover-letter.
thanks,
-- Shuah
Re: [PATCH] selftests: kselftest: Use strerror() on nolibc
On 9/11/24 09:44, Thomas Weißschuh wrote:
Hi Shuah,
On 2024-09-11 09:36:50+, Shuah Khan wrote:
On 9/10/24 22:42, zhangjiao2 wrote:
From: zhang jiao
Nolibc gained an implementation of strerror() recently.
Use it and drop the ifndef.
Signed-off-by: zhang jiao
---
tools/testing/selftests/kselftest.h | 8
1 file changed, 8 deletions(-)
diff --git a/tools/testing/selftests/kselftest.h
b/tools/testing/selftests/kselftest.h
index e195ec156859..29fedf609611 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -373,15 +373,7 @@ static inline __noreturn __printf(1, 2) void
ksft_exit_fail_msg(const char *msg,
static inline __noreturn void ksft_exit_fail_perror(const char *msg)
{
-#ifndef NOLIBC
ksft_exit_fail_msg("%s: %s (%d)\n", msg, strerror(errno), errno);
-#else
- /*
-* nolibc doesn't provide strerror() and it seems
-* inappropriate to add one, just print the errno.
-*/
- ksft_exit_fail_msg("%s: %d)\n", msg, errno);
-#endif
}
static inline __noreturn void ksft_exit_xfail(void)
Adding nolibc maintainers for review.
Willy and Thomas, please review.
Acked-by: Thomas Weißschuh
I did the same for another kselftests function when introducing
strerror(). This one was apparently missed or didn't exist yet.
Thank you. Applied to linux-kselftest next for Linux 6.12-rc1.
thanks,
-- Shuah
Re: [PATCH] selftests: kselftest: Use strerror() on nolibc
On 9/10/24 22:42, zhangjiao2 wrote:
From: zhang jiao
Nolibc gained an implementation of strerror() recently.
Use it and drop the ifndef.
Signed-off-by: zhang jiao
---
tools/testing/selftests/kselftest.h | 8
1 file changed, 8 deletions(-)
diff --git a/tools/testing/selftests/kselftest.h
b/tools/testing/selftests/kselftest.h
index e195ec156859..29fedf609611 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -373,15 +373,7 @@ static inline __noreturn __printf(1, 2) void
ksft_exit_fail_msg(const char *msg,
static inline __noreturn void ksft_exit_fail_perror(const char *msg)
{
-#ifndef NOLIBC
ksft_exit_fail_msg("%s: %s (%d)\n", msg, strerror(errno), errno);
-#else
- /*
-* nolibc doesn't provide strerror() and it seems
-* inappropriate to add one, just print the errno.
-*/
- ksft_exit_fail_msg("%s: %d)\n", msg, errno);
-#endif
}
static inline __noreturn void ksft_exit_xfail(void)
Adding nolibc maintainers for review.
Willy and Thomas, please review.
thanks,
-- Shuah
Re: [PATCH v6 1/2] selftests: Rename sigaltstack to generic signal
On 9/8/24 23:16, Dev Jain wrote: On 9/7/24 01:29, Shuah Khan wrote: On 9/4/24 23:56, Dev Jain wrote: On 9/4/24 22:35, Shuah Khan wrote: On 9/3/24 22:52, Dev Jain wrote: On 9/4/24 03:14, Shuah Khan wrote: On 8/30/24 10:29, Dev Jain wrote: On 8/27/24 17:16, Dev Jain wrote: On 8/27/24 17:14, Shuah Khan wrote: On 8/22/24 06:14, Dev Jain wrote: Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future. Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series? I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c. Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named. Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases. I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem? So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name. Other than the desire to rename the directory to generic, what other value does this change bring? Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack. If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory? Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"? Adding a new directory is much better than going down a path that is more confusing and adding backport overhead. Okay - they are related except that you view signalstack as a subset of signals. I saw Mark's response as well saying sigaction isn't a good name for this. Rename usually wipe out git history as well based on what have seen in the past. My main concern is backports. Considering sigstack hasn't changed 2021 (as Mark's email), let's rename it. I am reluctantly agreeing to the rename as it seems to make sense in this case. Thanks! I guess there is no update required from my side, and you can pull this series? I can pull this with x86v maintainer ack. Or to go through x86 tree: Acked-by: Shuah Khan thanks, -- Shuah
[GIT PULL] KUnit fixes second update for Linux 6.11-rc7
Hi Linus, Please pull the following KUnit fixes second update for Linux 6.11-rc7. This KUnit fixes update for Linux 6.11-rc7 consists of a fix to missing function parameter warning found during documentation build in linux-next. diff is attached. thanks, -- Shuah The following changes since commit f2c6dbd220170c2396fb019ead67fbada1e23ebd: kunit: Device wrappers should also manage driver name (2024-08-26 07:03:46 -0600) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest tags/linux_kselftest-kunit-fixes-6.11-rc7-2 for you to fetch changes up to 12cb32a52eb607dc4d0e45fe6f4cf946d08da0fd: kunit: Fix missing kerneldoc comment (2024-09-05 14:29:10 -0600) linux_kselftest-kunit-fixes-6.11-rc7-2 This KUnit fixes update for Linux 6.11-rc7 consists of a fix to missing function parameter warning found during documentation build in linux-next. David Gow (1): kunit: Fix missing kerneldoc comment include/kunit/test.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/kunit/test.h b/include/kunit/test.h index 5ac237c949a0..34b71e42fb10 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -484,6 +484,7 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp /** * kunit_kfree_const() - conditionally free test managed memory + * @test: The test context object. * @x: pointer to the memory * * Calls kunit_kfree() only if @x is not in .rodata section.
Re: [PATCH v6 1/2] selftests: Rename sigaltstack to generic signal
On 9/4/24 23:56, Dev Jain wrote: On 9/4/24 22:35, Shuah Khan wrote: On 9/3/24 22:52, Dev Jain wrote: On 9/4/24 03:14, Shuah Khan wrote: On 8/30/24 10:29, Dev Jain wrote: On 8/27/24 17:16, Dev Jain wrote: On 8/27/24 17:14, Shuah Khan wrote: On 8/22/24 06:14, Dev Jain wrote: Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future. Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series? I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c. Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named. Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases. I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem? So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name. Other than the desire to rename the directory to generic, what other value does this change bring? Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack. If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory? Because the functionality I am testing is of signals, and signals are a superset of sigaltstack. Still, I can think of a compromise, if semantically you want to consider the new test as not testing signals, but a specific syscall "sigaction" and its interaction with blocking of signals, how about naming the new directory "sigaction"? Adding a new directory is much better than going down a path that is more confusing and adding backport overhead. Okay - they are related except that you view signalstack as a subset of signals. I saw Mark's response as well saying sigaction isn't a good name for this. Rename usually wipe out git history as well based on what have seen in the past. My main concern is backports. Considering sigstack hasn't changed 2021 (as Mark's email), let's rename it. I am reluctantly agreeing to the rename as it seems to make sense in this case. thanks, -- Shuah
Re: [PATCH v2] selftests/timers: Remove unused NSEC_PER_SEC macro
On 9/6/24 11:02, John Stultz wrote:
On Fri, Sep 6, 2024 at 7:29 AM Shuah Khan wrote:
On 9/5/24 20:52, zhangjiao2 wrote:
diff --git a/tools/testing/selftests/timers/skew_consistency.c
b/tools/testing/selftests/timers/skew_consistency.c
index c8e6bffe4e0a..83450145fe65 100644
--- a/tools/testing/selftests/timers/skew_consistency.c
+++ b/tools/testing/selftests/timers/skew_consistency.c
@@ -36,8 +36,6 @@
#include
#include "../kselftest.h"
-#define NSEC_PER_SEC 10LL
-
int main(int argc, char **argv)
{
struct timex tx;
This looks good to me.
Reviewed-by: Shuah Khan
John, I can pick this up with if you are okay with this change.
No objection from me, if you're ok with the commit.
Acked-by: John Stultz
thanks
-john
Thank you. Applied linux-kselftest next for Linux 6.12-rc1.
thanks,
-- Shuah
Re: [PATCH v2] selftests/timers: Remove unused NSEC_PER_SEC macro
On 9/5/24 20:52, zhangjiao2 wrote:
From: zhang jiao
By readind the code, I found the macro NSEC_PER_SEC
reading
is never referenced in the code. Just remove it.
Signed-off-by: zhang jiao
Running checkpatch can catch spelling errors.
---
v1->v2:
Put together files with similar problems
tools/testing/selftests/timers/change_skew.c | 3 ---
tools/testing/selftests/timers/skew_consistency.c | 2 --
2 files changed, 5 deletions(-)
diff --git a/tools/testing/selftests/timers/change_skew.c
b/tools/testing/selftests/timers/change_skew.c
index 4421cd562c24..18e794a46c23 100644
--- a/tools/testing/selftests/timers/change_skew.c
+++ b/tools/testing/selftests/timers/change_skew.c
@@ -30,9 +30,6 @@
#include
#include "../kselftest.h"
-#define NSEC_PER_SEC 10LL
-
-
int change_skew_test(int ppm)
{
struct timex tx;
diff --git a/tools/testing/selftests/timers/skew_consistency.c
b/tools/testing/selftests/timers/skew_consistency.c
index c8e6bffe4e0a..83450145fe65 100644
--- a/tools/testing/selftests/timers/skew_consistency.c
+++ b/tools/testing/selftests/timers/skew_consistency.c
@@ -36,8 +36,6 @@
#include
#include "../kselftest.h"
-#define NSEC_PER_SEC 10LL
-
int main(int argc, char **argv)
{
struct timex tx;
This looks good to me.
Reviewed-by: Shuah Khan
John, I can pick this up with if you are okay with this change.
thanks,
-- Shuah
Re: [PATCH] selftests:resctrl: Fix build failure on archs without __cpuid_count()
On 9/6/24 01:35, Muhammad Usama Anjum wrote:
Hi Shuah,
Thank you for fixing it.
On 9/5/24 11:02 PM, Shuah Khan wrote:
When resctrl is built on architectures without __cpuid_count()
support, build fails. resctrl uses __cpuid_count() defined in
kselftest.h.
Even though the problem is seen while building resctrl on aarch64,
this error can be seen on any platform that doesn't support CPUID.
CPUID is a x86/x86-64 feature and code paths with CPUID asm commands
will fail to build on all other architectures.
All others tests call __cpuid_count() do so from x86/x86_64 code paths
when _i386__ or __x86_64__ are defined. resctrl is an exception.
Fix the problem by defining __cpuid_count() only when __i386__ or
__x86_64__ are defined in kselftest.h and changing resctrl to call
__cpuid_count() only when __i386__ or __x86_64__ are defined.
In file included from resctrl.h:24,
from cat_test.c:11:
In function ‘arch_supports_noncont_cat’,
inlined from ‘noncont_cat_run_test’ at cat_test.c:326:6:
../kselftest.h:74:9: error: impossible constraint in ‘asm’
74 | __asm__ __volatile__ ("cpuid\n\t"
\
| ^~~
cat_test.c:304:17: note: in expansion of macro ‘__cpuid_count’
304 | __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
| ^
../kselftest.h:74:9: error: impossible constraint in ‘asm’
74 | __asm__ __volatile__ ("cpuid\n\t"
\
| ^~~
cat_test.c:306:17: note: in expansion of macro ‘__cpuid_count’
306 | __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
Reported-by: Muhammad Usama Anjum
Reported-by: Ilpo Järvinen
Signed-off-by: Shuah Khan
LGTM
Reviewed-by: Muhammad Usama Anjum
Thank you for the review and finding the problem to begin with.
Much appreciated.
...
diff --git a/tools/testing/selftests/resctrl/cat_test.c
b/tools/testing/selftests/resctrl/cat_test.c
index 742782438ca3..ae3f0fa5390b 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -290,12 +290,12 @@ static int cat_run_test(const struct resctrl_test *test,
const struct user_param
static bool arch_supports_noncont_cat(const struct resctrl_test *test)
{
- unsigned int eax, ebx, ecx, edx;
-
/* AMD always supports non-contiguous CBM. */
if (get_vendor() == ARCH_AMD)
return true;
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+ unsigned int eax, ebx, ecx, edx;
/* Intel support for non-contiguous CBM needs to be discovered. */
if (!strcmp(test->resource, "L3"))
__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
@@ -305,6 +305,8 @@ static bool arch_supports_noncont_cat(const struct
resctrl_test *test)
return false;
return ((ecx >> 3) & 1);
+#endif /* end arch */
+ return false;
nit: empty line before return
Will do.
}
static int noncont_cat_run_test(const struct resctrl_test *test,
thanks,
-- Shuah
Re: [PATCH] selftests:resctrl: Fix build failure on archs without __cpuid_count()
On 9/6/24 04:12, Ilpo Järvinen wrote:
On Thu, 5 Sep 2024, Shuah Khan wrote:
When resctrl is built on architectures without __cpuid_count()
support, build fails. resctrl uses __cpuid_count() defined in
kselftest.h.
Even though the problem is seen while building resctrl on aarch64,
this error can be seen on any platform that doesn't support CPUID.
CPUID is a x86/x86-64 feature and code paths with CPUID asm commands
will fail to build on all other architectures.
All others tests call __cpuid_count() do so from x86/x86_64 code paths
when _i386__ or __x86_64__ are defined. resctrl is an exception.
Fix the problem by defining __cpuid_count() only when __i386__ or
__x86_64__ are defined in kselftest.h and changing resctrl to call
__cpuid_count() only when __i386__ or __x86_64__ are defined.
In file included from resctrl.h:24,
from cat_test.c:11:
In function ‘arch_supports_noncont_cat’,
inlined from ‘noncont_cat_run_test’ at cat_test.c:326:6:
../kselftest.h:74:9: error: impossible constraint in ‘asm’
74 | __asm__ __volatile__ ("cpuid\n\t"
\
| ^~~
cat_test.c:304:17: note: in expansion of macro ‘__cpuid_count’
304 | __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
| ^
../kselftest.h:74:9: error: impossible constraint in ‘asm’
74 | __asm__ __volatile__ ("cpuid\n\t"
\
| ^~~
cat_test.c:306:17: note: in expansion of macro ‘__cpuid_count’
306 | __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
Reported-by: Muhammad Usama Anjum
Reported-by: Ilpo Järvinen
Signed-off-by: Shuah Khan
When the small things from Muhammad and Reinette addressed, this seems
okay.
Reviewed-by: Ilpo Järvinen
Will do. Thank you for the review.
Thanks for the solution.
I'm still left to wonder if the x86 selftest is supposed to clobber
CFLAGS? It seems that problem is orthogonal to this cpuid/resctrl problem.
I mean this question from the perspective of coherency in the entire
kselftest framework, lib.mk seems to want to adjust CFLAGS but those
changes will get clobbered in the case of x86 selftest.
This isn't the case x86 clobbering the CFLAGS. This falls into the case
of x86 customizing the flags for the test and the ones set by the common
lib.mk might interfere with the flags it needs.
There isn't anything here to fix based on the history of this test and
lib.mk.
thanks,
-- Shuah
Re: [PATCH v4 0/4] selftests: Fix cpuid / vendor checking build issues
On 9/5/24 14:43, Reinette Chatre wrote: Hi Shuah, On 9/5/24 11:06 AM, Shuah Khan wrote: On 9/4/24 06:54, Ilpo Järvinen wrote: On Wed, 4 Sep 2024, Shuah Khan wrote: On 9/4/24 06:18, Ilpo Järvinen wrote: On Tue, 3 Sep 2024, Shuah Khan wrote: On 9/3/24 08:45, Ilpo Järvinen wrote: This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM). While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support. I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version. Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build. v4: - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise) I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary. I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do. Some of it by desig. lib.mk offers framework for common things. There are provisions to override like in the case of x86, powerpc. lib.mk tries to be flexible as well. I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework. Thoughts on cpuid: - It is x86 specific. Moving this to kselftest.h was done to avoid duplicate. However now we are running into arm64/arm compile errors due to this which need addressing one way or the other. I have some ideas on how to solve this - but I need answers to the following questions. This is a question for you and Usama. - Does resctrl run on arm64/arm and what's the output? - Can all other tests in resctrl other tests except noncont_cat_run_test? - If so send me the output. Hi Shuah, As mentioned in my coverletter above, resctrl does not currently support arm but there's an ongoing work to add arm support. On kernel side it requires major refactoring to move non-arch specific stuff out from arch/x86 so has (predictably) taken long time. The resctrl selftests are mostly written in arch independent way (*) but there's also a way to limit a test only to CPUs from a particular vendor. And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs (to read the supported flag), it's not needed even on AMD CPUs as they always support non-contiguous CAT bitmask. So to summarize, it would be possible to disable resctrl test for non-x86 but it does not address the underlying problem with cpuid which will just come back later I think. Alternatively, if there's some a good way in C code to do ifdeffery around that cpuid call, I could make that too, but I need to know which symbol to use for that ifdef. (*) The cache topology may make some selftest unusable on new archs but not the selftest code itself. I agree that suppressing resctrl build is not a solution. The real problem is is in defining __cpuid_count() in common code path. I fixed it and send patch in. As I was testing I noticed the following on AMD platform: - it ran the L3_NONCONT_CAT test which is expected. # # Starting L3_NONCONT_CAT test ... # # Mounting resctrl to "/sys/fs/resctrl" # ARCH_AMD - supports non-contiguous CBM # # Write schema "L3:0=ff" to resctrl FS # # Write schema "L3:0=fc3f" to resctrl FS # ok 5 L3_NONCONT_CAT: test - It went on to run L2_NONCONT_CAT - failed It is not intended to appear as a failure but instead just skipping of a test since the platform does not support the feature being tested. # ok 6 # SKIP Hardware does not support L2_NONCONT_CAT or L2_NONCONT_CAT is disabled The output looks as intended. When I run the test on an Intel system without L2 CAT the output looks the same. Does it make sense to run both L3_NONCONT_CAT and L2_NONCONT_CAT on AMD? Maybe it is? resctrl checks L3 or L2 support on Intel. The selftests test the features as exposed by the generic resctrl kernel subsystem instead of relying on its own inventory of what features need to be checked for which vendor. selftests will
Re: [PATCH] selftests:resctrl: Fix build failure on archs without __cpuid_count()
On 9/5/24 14:45, Reinette Chatre wrote:
Hi Shuah,
Thank you very much for looking into this.
On 9/5/24 11:02 AM, Shuah Khan wrote:
When resctrl is built on architectures without __cpuid_count()
support, build fails. resctrl uses __cpuid_count() defined in
kselftest.h.
Even though the problem is seen while building resctrl on aarch64,
this error can be seen on any platform that doesn't support CPUID.
CPUID is a x86/x86-64 feature and code paths with CPUID asm commands
will fail to build on all other architectures.
All others tests call __cpuid_count() do so from x86/x86_64 code paths
when _i386__ or __x86_64__ are defined. resctrl is an exception.
Fix the problem by defining __cpuid_count() only when __i386__ or
__x86_64__ are defined in kselftest.h and changing resctrl to call
__cpuid_count() only when __i386__ or __x86_64__ are defined.
In file included from resctrl.h:24,
from cat_test.c:11:
In function ‘arch_supports_noncont_cat’,
inlined from ‘noncont_cat_run_test’ at cat_test.c:326:6:
../kselftest.h:74:9: error: impossible constraint in ‘asm’
74 | __asm__ __volatile__ ("cpuid\n\t"
\
| ^~~
cat_test.c:304:17: note: in expansion of macro ‘__cpuid_count’
304 | __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
| ^
../kselftest.h:74:9: error: impossible constraint in ‘asm’
74 | __asm__ __volatile__ ("cpuid\n\t"
\
| ^~~
cat_test.c:306:17: note: in expansion of macro ‘__cpuid_count’
306 | __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
If needing to know where this fix is needed, there can be a:
Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test")
Thanks - will add it when I apply the patch.
Reported-by: Muhammad Usama Anjum
Perhaps:
Closes:
https://lore.kernel.org/lkml/20240809071059.265914-1-usama.an...@collabora.com/
Thanks. I will add it when I apply the patch.
Reported-by: Ilpo Järvinen
Signed-off-by: Shuah Khan
---
tools/testing/selftests/kselftest.h | 2 ++
tools/testing/selftests/resctrl/cat_test.c | 6 --
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kselftest.h
b/tools/testing/selftests/kselftest.h
index b8967b6e29d5..e195ec156859 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -61,6 +61,7 @@
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
#endif
+#if defined(__i386__) || defined(__x86_64__) /* arch */
/*
* gcc cpuid.h provides __cpuid_count() since v4.4.
* Clang/LLVM cpuid.h provides __cpuid_count() since v3.4.0.
@@ -75,6 +76,7 @@
: "=a" (a), "=b" (b), "=c" (c), "=d" (d) \
: "0" (level), "2" (count))
#endif
+#endif /* end arch */
/* define kselftest exit codes */
#define KSFT_PASS 0
diff --git a/tools/testing/selftests/resctrl/cat_test.c
b/tools/testing/selftests/resctrl/cat_test.c
index 742782438ca3..ae3f0fa5390b 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -290,12 +290,12 @@ static int cat_run_test(const struct resctrl_test *test,
const struct user_param
static bool arch_supports_noncont_cat(const struct resctrl_test *test)
{
- unsigned int eax, ebx, ecx, edx;
-
/* AMD always supports non-contiguous CBM. */
if (get_vendor() == ARCH_AMD)
return true;
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+ unsigned int eax, ebx, ecx, edx;
/* Intel support for non-contiguous CBM needs to be discovered. */
if (!strcmp(test->resource, "L3"))
__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
@@ -305,6 +305,8 @@ static bool arch_supports_noncont_cat(const struct
resctrl_test *test)
return false;
return ((ecx >> 3) & 1);
+#endif /* end arch */
+ return false;
}
static int noncont_cat_run_test(const struct resctrl_test *test,
Thank you very much.
Acked-by: Reinette Chatre
thanks,
-- Shuah
Re: [PATCH v2] selftests: futex: Fix missing free in main
On 9/4/24 20:01, zhangjiao2 wrote:
From: zhang jiao
By readind the code, I found there is no free() after asprintf().
Just free it.
Signed-off-by: zhang jiao
---
v1->v2:
Set a flag to determine if test_name needs free.
tools/testing/selftests/futex/functional/futex_requeue_pi.c | 4
1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi.c
b/tools/testing/selftests/futex/functional/futex_requeue_pi.c
index 215c6cb539b4..d78bb5112fce 100644
--- a/tools/testing/selftests/futex/functional/futex_requeue_pi.c
+++ b/tools/testing/selftests/futex/functional/futex_requeue_pi.c
@@ -362,6 +362,7 @@ int main(int argc, char *argv[])
{
char *test_name;
int c, ret;
+ int need_f = 1;
This can be a bool which - better to set it to false as a initial
value.
while ((c = getopt(argc, argv, "bchlot:v:")) != -1) {
switch (c) {
@@ -404,6 +405,7 @@ int main(int argc, char *argv[])
"%s broadcast=%d locked=%d owner=%d timeout=%ldns",
TEST_NAME, broadcast, locked, owner, timeout_ns);
if (ret < 0) {
+ need_f = 0;
You would set it to TRUE here.
ksft_print_msg("Failed to generate test name\n");
test_name = TEST_NAME;
}
@@ -416,5 +418,7 @@ int main(int argc, char *argv[])
ret = unit_test(broadcast, locked, owner, timeout_ns);
print_result(test_name, ret);
+ if (need_f)
Check against TRUE
+ free(test_name);
return ret;
}
thanks,
-- Shuah
Re: [PATCH v4 0/4] selftests: Fix cpuid / vendor checking build issues
On 9/4/24 06:54, Ilpo Järvinen wrote: On Wed, 4 Sep 2024, Shuah Khan wrote: On 9/4/24 06:18, Ilpo Järvinen wrote: On Tue, 3 Sep 2024, Shuah Khan wrote: On 9/3/24 08:45, Ilpo Järvinen wrote: This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM). While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support. I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version. Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build. v4: - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise) I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary. I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do. Some of it by desig. lib.mk offers framework for common things. There are provisions to override like in the case of x86, powerpc. lib.mk tries to be flexible as well. I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework. Thoughts on cpuid: - It is x86 specific. Moving this to kselftest.h was done to avoid duplicate. However now we are running into arm64/arm compile errors due to this which need addressing one way or the other. I have some ideas on how to solve this - but I need answers to the following questions. This is a question for you and Usama. - Does resctrl run on arm64/arm and what's the output? - Can all other tests in resctrl other tests except noncont_cat_run_test? - If so send me the output. Hi Shuah, As mentioned in my coverletter above, resctrl does not currently support arm but there's an ongoing work to add arm support. On kernel side it requires major refactoring to move non-arch specific stuff out from arch/x86 so has (predictably) taken long time. The resctrl selftests are mostly written in arch independent way (*) but there's also a way to limit a test only to CPUs from a particular vendor. And now this noncont_cat_run_test needs to use cpuid only on Intel CPUs (to read the supported flag), it's not needed even on AMD CPUs as they always support non-contiguous CAT bitmask. So to summarize, it would be possible to disable resctrl test for non-x86 but it does not address the underlying problem with cpuid which will just come back later I think. Alternatively, if there's some a good way in C code to do ifdeffery around that cpuid call, I could make that too, but I need to know which symbol to use for that ifdef. (*) The cache topology may make some selftest unusable on new archs but not the selftest code itself. I agree that suppressing resctrl build is not a solution. The real problem is is in defining __cpuid_count() in common code path. I fixed it and send patch in. As I was testing I noticed the following on AMD platform: - it ran the L3_NONCONT_CAT test which is expected. # # Starting L3_NONCONT_CAT test ... # # Mounting resctrl to "/sys/fs/resctrl" # ARCH_AMD - supports non-contiguous CBM # # Write schema "L3:0=ff" to resctrl FS # # Write schema "L3:0=fc3f" to resctrl FS # ok 5 L3_NONCONT_CAT: test - It went on to run L2_NONCONT_CAT - failed # ok 6 # SKIP Hardware does not support L2_NONCONT_CAT or L2_NONCONT_CAT is disabled Does it make sense to run both L3_NONCONT_CAT and L2_NONCONT_CAT on AMD? Maybe it is? resctrl checks L3 or L2 support on Intel. Anyway - the problem is fixed now. Please review and test. thanks, -- Shuah
[PATCH] selftests:resctrl: Fix build failure on archs without __cpuid_count()
When resctrl is built on architectures without __cpuid_count()
support, build fails. resctrl uses __cpuid_count() defined in
kselftest.h.
Even though the problem is seen while building resctrl on aarch64,
this error can be seen on any platform that doesn't support CPUID.
CPUID is a x86/x86-64 feature and code paths with CPUID asm commands
will fail to build on all other architectures.
All others tests call __cpuid_count() do so from x86/x86_64 code paths
when _i386__ or __x86_64__ are defined. resctrl is an exception.
Fix the problem by defining __cpuid_count() only when __i386__ or
__x86_64__ are defined in kselftest.h and changing resctrl to call
__cpuid_count() only when __i386__ or __x86_64__ are defined.
In file included from resctrl.h:24,
from cat_test.c:11:
In function ‘arch_supports_noncont_cat’,
inlined from ‘noncont_cat_run_test’ at cat_test.c:326:6:
../kselftest.h:74:9: error: impossible constraint in ‘asm’
74 | __asm__ __volatile__ ("cpuid\n\t"
\
| ^~~
cat_test.c:304:17: note: in expansion of macro ‘__cpuid_count’
304 | __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
| ^
../kselftest.h:74:9: error: impossible constraint in ‘asm’
74 | __asm__ __volatile__ ("cpuid\n\t"
\
| ^~~
cat_test.c:306:17: note: in expansion of macro ‘__cpuid_count’
306 | __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
Reported-by: Muhammad Usama Anjum
Reported-by: Ilpo Järvinen
Signed-off-by: Shuah Khan
---
tools/testing/selftests/kselftest.h| 2 ++
tools/testing/selftests/resctrl/cat_test.c | 6 --
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kselftest.h
b/tools/testing/selftests/kselftest.h
index b8967b6e29d5..e195ec156859 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -61,6 +61,7 @@
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
#endif
+#if defined(__i386__) || defined(__x86_64__) /* arch */
/*
* gcc cpuid.h provides __cpuid_count() since v4.4.
* Clang/LLVM cpuid.h provides __cpuid_count() since v3.4.0.
@@ -75,6 +76,7 @@
: "=a" (a), "=b" (b), "=c" (c), "=d" (d) \
: "0" (level), "2" (count))
#endif
+#endif /* end arch */
/* define kselftest exit codes */
#define KSFT_PASS 0
diff --git a/tools/testing/selftests/resctrl/cat_test.c
b/tools/testing/selftests/resctrl/cat_test.c
index 742782438ca3..ae3f0fa5390b 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -290,12 +290,12 @@ static int cat_run_test(const struct resctrl_test *test,
const struct user_param
static bool arch_supports_noncont_cat(const struct resctrl_test *test)
{
- unsigned int eax, ebx, ecx, edx;
-
/* AMD always supports non-contiguous CBM. */
if (get_vendor() == ARCH_AMD)
return true;
+#if defined(__i386__) || defined(__x86_64__) /* arch */
+ unsigned int eax, ebx, ecx, edx;
/* Intel support for non-contiguous CBM needs to be discovered. */
if (!strcmp(test->resource, "L3"))
__cpuid_count(0x10, 1, eax, ebx, ecx, edx);
@@ -305,6 +305,8 @@ static bool arch_supports_noncont_cat(const struct
resctrl_test *test)
return false;
return ((ecx >> 3) & 1);
+#endif /* end arch */
+ return false;
}
static int noncont_cat_run_test(const struct resctrl_test *test,
--
2.40.1
Re: [PATCH] Date: Remove unused macro
On 9/5/24 02:52, zhangjiao2 wrote:
From: zhang jiao
This macro NSEC_PER_SEC is never referenced in the code.
Just remove it.
Is this duplicate patch?
I think I commented on your patch on futex - include how
you found the problem in change logs. Also you have to
include subsystem prefix in the subject line:
selftests/timers: Remove unused NSEC_PER_SEC macro
I see another patch with similar problems from you.
Refer to submitting patches document in the kernel
repo.
Signed-off-by: zhang jiao
---
tools/testing/selftests/timers/change_skew.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/tools/testing/selftests/timers/change_skew.c
b/tools/testing/selftests/timers/change_skew.c
index 4421cd562c24..18e794a46c23 100644
--- a/tools/testing/selftests/timers/change_skew.c
+++ b/tools/testing/selftests/timers/change_skew.c
@@ -30,9 +30,6 @@
#include
#include "../kselftest.h"
-#define NSEC_PER_SEC 10LL
-
-
int change_skew_test(int ppm)
{
struct timex tx;
thanks,
-- Shuah
Re: [PATCH] Date: Remove unused macro
On 9/5/24 02:13, zhangjiao2 wrote:
From: zhang jiao
This macro NSEC_PER_SEC is never referenced in the code.
Just remove it.
I think I commented on your patch on futex - include how
you found the problem in change logs. Also you have to
include subsystem prefix in the subject line:
selftests/timers: Remove unused NSEC_PER_SEC macro
I see another patch with similar problems from you.
Refer to submitting patches document in the kernel
repo.
Signed-off-by: zhang jiao
---
tools/testing/selftests/timers/skew_consistency.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/timers/skew_consistency.c
b/tools/testing/selftests/timers/skew_consistency.c
index c8e6bffe4e0a..83450145fe65 100644
--- a/tools/testing/selftests/timers/skew_consistency.c
+++ b/tools/testing/selftests/timers/skew_consistency.c
@@ -36,8 +36,6 @@
#include
#include "../kselftest.h"
-#define NSEC_PER_SEC 10LL
-
int main(int argc, char **argv)
{
struct timex tx;
thanks,
-- Shuah
Re: [PATCH] kunit: Fix missing kerneldoc comment
On 9/4/24 20:47, David Gow wrote:
Add a missing kerneldoc comment for the 'test' test context parameter,
fixing the following warning:
include/kunit/test.h:492: warning: Function parameter or struct member 'test'
not described in 'kunit_kfree_const'
Reported-by: Stephen Rothwell
Closes: https://lore.kernel.org/lkml/20240827160631.67e12...@canb.auug.org.au/
Fixes: f2c6dbd22017 ("kunit: Device wrappers should also manage driver name")
Signed-off-by: David Gow
---
include/kunit/test.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 5ac237c949a0..34b71e42fb10 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -484,6 +484,7 @@ static inline void *kunit_kcalloc(struct kunit *test,
size_t n, size_t size, gfp
/**
* kunit_kfree_const() - conditionally free test managed memory
+ * @test: The test context object.
* @x: pointer to the memory
*
* Calls kunit_kfree() only if @x is not in .rodata section.
David,
I sent PR for 6.11-rc7 for the following since it is a critical
fix.
kunit: Device wrappers should also manage driver name
I will take this in once the pr clears applying for 6.12-rc1
thanks,
-- Shuah
Re: [PATCH v6 1/2] selftests: Rename sigaltstack to generic signal
On 9/3/24 22:52, Dev Jain wrote: On 9/4/24 03:14, Shuah Khan wrote: On 8/30/24 10:29, Dev Jain wrote: On 8/27/24 17:16, Dev Jain wrote: On 8/27/24 17:14, Shuah Khan wrote: On 8/22/24 06:14, Dev Jain wrote: Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future. Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series? I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c. Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named. Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases. I understand your point, but commit baa489fabd01 (selftests/vm: rename selftests/vm to selftests/mm) is also present. That was a lot bigger change; sigaltstack contains just one test currently, whose fixes possibly would have to be backported, so I guess it should not be that much of a big problem? So who does the backports whenevenr something changes? You are adding work where as the automated process would just work without this change. It doesn't matter if there is another test that changed the name. Other than the desire to rename the directory to generic, what other value does this change bring? Do you have an alternative suggestion as to where I should put my new test then; I do not see what is the value of creating another directory to just include my test. This will unnecessarily clutter the selftests/ directory with directories containing single tests. And, putting this in "sigaltstack" is just wrong since this test has no relation with sigaltstack. If this new test has no relation to sigaltstack, then why are you changing and renaming the sigaltstack directory? Adding a new directory is much better than going down a path that is more confusing and adding backport overhead. Two options: -- Add a new directory or add a note and keep it under sigaltstack -- Do you foresee this new growing? thanks, -- Shuah
Re: [PATCH] selftests: futex: Fix missing free in main
On 9/3/24 20:55, zhangjiao2 wrote: From: zhang jiao Free string allocated by asprintf(). How did you find this problem? Include the details in the change log - The tool and output from the tool. Signed-off-by: zhang jiao --- tools/testing/selftests/futex/functional/futex_requeue_pi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/testing/selftests/futex/functional/futex_requeue_pi.c b/tools/testing/selftests/futex/functional/futex_requeue_pi.c index 215c6cb539b4..fb2dab46087f 100644 --- a/tools/testing/selftests/futex/functional/futex_requeue_pi.c +++ b/tools/testing/selftests/futex/functional/futex_requeue_pi.c @@ -416,5 +416,8 @@ int main(int argc, char *argv[]) ret = unit_test(broadcast, locked, owner, timeout_ns); print_result(test_name, ret); + if (strlen(test_name) > strlen(TEST_NAME)) + free(test_name); Why not set a flag to determine if test_name needs freeing instead of calling strlen() twice? + return ret; } thanks, -- Shuah
Re: [PATCH v2] selftests/futex: Create test for robust list
On 9/3/24 07:40, André Almeida wrote: Create a test for the robust list mechanism. What does this test - can you elaborate on the testing details? It will help reviewers catch if any tests are missed or not - be able to review the patch. Include output from the test in the chane log. Signed-off-by: André Almeida --- Changes from v1: - Change futex type from int to _Atomic(unsigned int) - Use old futex(FUTEX_WAIT) instead of the new sys_futex_wait() --- .../selftests/futex/functional/.gitignore | 1 + .../selftests/futex/functional/Makefile | 3 +- .../selftests/futex/functional/robust_list.c | 448 ++ 3 files changed, 451 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/futex/functional/robust_list.c diff --git a/tools/testing/selftests/futex/functional/.gitignore b/tools/testing/selftests/futex/functional/.gitignore index fbcbdb6963b3..4726e1be7497 100644 --- a/tools/testing/selftests/futex/functional/.gitignore +++ b/tools/testing/selftests/futex/functional/.gitignore @@ -9,3 +9,4 @@ futex_wait_wouldblock futex_wait futex_requeue futex_waitv +robust_list diff --git a/tools/testing/selftests/futex/functional/Makefile b/tools/testing/selftests/futex/functional/Makefile index f79f9bac7918..b8635a1ac7f6 100644 --- a/tools/testing/selftests/futex/functional/Makefile +++ b/tools/testing/selftests/futex/functional/Makefile @@ -17,7 +17,8 @@ TEST_GEN_PROGS := \ futex_wait_private_mapped_file \ futex_wait \ futex_requeue \ - futex_waitv + futex_waitv \ + robust_list TEST_PROGS := run.sh diff --git a/tools/testing/selftests/futex/functional/robust_list.c b/tools/testing/selftests/futex/functional/robust_list.c new file mode 100644 index ..9308eb189d48 --- /dev/null +++ b/tools/testing/selftests/futex/functional/robust_list.c @@ -0,0 +1,448 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2024 Igalia S.L. + * + * Robust list test by André Almeida + * + * The robust list uAPI allows userspace to create "robust" locks, in the sense + * that if the lock holder thread dies, the remaining threads that are waiting + * for the lock won't block forever, waiting for a lock that will never be + * released. + * + * This is achieve by userspace setting a list where a thread can enter all the + * locks (futexes) that it is holding. The robust list is a linked list, and + * userspace register the start of the list with the syscall set_robust_list(). + * If such thread eventually dies, the kernel will walk this list, waking up one + * thread waiting for each futex and marking the futex word with the flag + * FUTEX_OWNER_DIED. + * + * See also + * man set_robust_list + * Documententation/locking/robust-futex-ABI.rst + * Documententation/locking/robust-futexes.rst + */ + +#define _GNU_SOURCE + +#include "../../kselftest_harness.h" futex test suite doesn't kselftest_harness at the moment. Let's not mix and match the framework in the same test suite. Keep it consistent. thanks, -- Shuah
Re: [PATCH v4 0/4] selftests: Fix cpuid / vendor checking build issues
On 9/4/24 06:18, Ilpo Järvinen wrote: On Tue, 3 Sep 2024, Shuah Khan wrote: On 9/3/24 08:45, Ilpo Järvinen wrote: This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM). While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support. I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version. Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build. v4: - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise) I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary. I didn't like it either what I found from the various makefiles. I think there are many things done which conflict with what lib.mk seems to try to do. Some of it by desig. lib.mk offers framework for common things. There are provisions to override like in the case of x86, powerpc. lib.mk tries to be flexible as well. I tried to ask in the first submission what test I should use in the header file as I'm not very familiar with how arch specific is done in userspace in the first place nor how it should be done within kselftest framework. Thoughts on cpuid: - It is x86 specific. Moving this to kselftest.h was done to avoid duplicate. However now we are running into arm64/arm compile errors due to this which need addressing one way or the other. I have some ideas on how to solve this - but I need answers to the following questions. This is a question for you and Usama. - Does resctrl run on arm64/arm and what's the output? - Can all other tests in resctrl other tests except noncont_cat_run_test? - If so send me the output. thanks, -- Shuah
Re: [PATCH v4 0/4] selftests: Fix cpuid / vendor checking build issues
On 9/3/24 08:45, Ilpo Järvinen wrote: This series first generalizes resctrl selftest non-contiguous CAT check to not assume non-AMD vendor implies Intel. Second, it improves selftests such that the use of __cpuid_count() does not lead into a build failure (happens at least on ARM). While ARM does not currently support resctrl features, there's an ongoing work to enable resctrl support also for it on the kernel side. In any case, a common header such as kselftest.h should have a proper fallback in place for what it provides, thus it seems justified to fix this common level problem on the common level rather than e.g. disabling build for resctrl selftest for archs lacking resctrl support. I've dropped reviewed and tested by tags from the last patch in v3 due to major changes into the makefile logic. So it would be helpful if Muhammad could retest with this version. Acquiring ARCH in lib.mk will likely allow some cleanup into some subdirectory makefiles but that is left as future work because this series focuses in fixing cpuid/build. v4: - New patch to reorder x86 selftest makefile to avoid clobbering CFLAGS (would cause __cpuid_count() related build fail otherwise) I don't like the way this patch series is mushrooming. I am not convinced that changes to lib.mk and x86 Makefile are necessary. I will take a look at this to see if this can be simplified. thanks, -- Shuah
Re: [PATCH v2] selftests: splice: Add usage() to splice_read.c
On 8/30/24 23:14, Rong Tao wrote:
From: Rong Tao
Give the programmer more help information to inform the program on how to
use it.
Signed-off-by: Rong Tao
---
tools/testing/selftests/splice/splice_read.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/splice/splice_read.c
b/tools/testing/selftests/splice/splice_read.c
index 46dae6a25cfb..73a8bc146f97 100644
--- a/tools/testing/selftests/splice/splice_read.c
+++ b/tools/testing/selftests/splice/splice_read.c
@@ -9,6 +9,12 @@
#include
#include
+void usage(const char *prog)
+{
+ fprintf(stderr, "Usage: %s INPUT [BYTES]\n", prog);
+ fprintf(stderr, " %s /etc/os-release | cat\n", prog);
How does replacing %s INPUT [BYTES]\n", prog with /etc/os-release | cat\n" do?
Also what happens on distros that don't have os-release file?
+}
+
int main(int argc, char *argv[])
{
int fd;
@@ -16,7 +22,7 @@ int main(int argc, char *argv[])
ssize_t spliced;
if (argc < 2) {
- fprintf(stderr, "Usage: %s INPUT [BYTES]\n", argv[0]);
+ usage(argv[0]);
return EXIT_FAILURE;
}
@@ -49,6 +55,7 @@ int main(int argc, char *argv[])
size, SPLICE_F_MOVE);
if (spliced < 0) {
perror("splice");
+ usage(argv[0]);
return EXIT_FAILURE;
}
I don't understand how this change helps user.
thanks,
-- Shuah
Re: [PATCH v6 1/2] selftests: Rename sigaltstack to generic signal
On 8/30/24 10:29, Dev Jain wrote: On 8/27/24 17:16, Dev Jain wrote: On 8/27/24 17:14, Shuah Khan wrote: On 8/22/24 06:14, Dev Jain wrote: Rename sigaltstack to generic signal directory, to allow adding more signal tests in the future. Sorry - I think I mentioned I don't like this test renamed. Why are you sending this rename still included in the patch series? I am not renaming the test, just the directory. The directory name is changed to signal, and I have retained the name of the test - sas.c. Gentle ping: I guess there was a misunderstanding; in v5, I was also changing the name of the test, to which you objected, and I agreed. But, we need to change the name of the directory since the new test has no relation to the current directory name, "sigaltstack". The patch description explains that the directory should be generically named. Right. You are no longer changing the test name. You are still changing the directory name. The problem I mentioned stays the same. Any fixes to the existing tests in this directory can no longer auto applied to stables releases. Other than the desire to rename the directory to generic, what other value does this change bring? thanks, -- Shuah
Re: [PATCH] selftests/vDSO: support DT_GNU_HASH
On 8/27/24 07:37, Fangrui Song wrote:
On Tue, Aug 27, 2024 at 10:12 PM Shuah Khan wrote:
On 8/26/24 00:07, Xi Ruoyao wrote:
On Wed, 2024-08-14 at 20:26 -0700, Fangrui Song wrote:
glibc added support for DT_GNU_HASH in 2006 and DT_HASH has been
obsoleted for more than one decade in many Linux distributions.
Many vDSOs support DT_GNU_HASH. This patch adds selftests support.
Signed-off-by: Fangrui Song
---
Ping.
Some context: I'd change LoongArch vDSO to use the toolchain default
instead of forcing DT_HASH (note that LoongArch is launched decades
after all major distros switched to DT_GNU_HASH), but without the
selftest support we'll lose test coverage.
And now ARM64 has already lost test coverage after commit 48f6430505c0.
I am seeing several checkpatch errors - please fix them and send me v2.
thanks,
-- Shuah
The applicable change is:
--- i/tools/testing/selftests/vDSO/parse_vdso.c
+++ w/tools/testing/selftests/vDSO/parse_vdso.c
@@ -177,7 +177,7 @@ void vdso_init_from_sysinfo_ehdr(uintptr_t base)
if (vdso_info.gnu_hash) {
vdso_info.nbucket = vdso_info.gnu_hash[0];
/* The bucket array is located after the header (4
uint32) and the bloom
- filter (size_t array of gnu_hash[2] elements). */
+* filter (size_t array of gnu_hash[2] elements). */
vdso_info.bucket = vdso_info.gnu_hash + 4 +
sizeof(size_t) / 4 * vdso_info.gnu_hash[2];
} else {
Other checkpatch.pl output is not actionable. `ELF(Sym) *sym` instead
of `ELF(Sym) * sym` has the correct spacing (used in this file and
elsewhere ElfW in the code base).
Okay. Send v2 with the actionable change.
thanks,
-- Shuah
Re: [PATCH] selftests: filesystems: fix warn_unused_result build warnings
On 8/16/24 07:11, Shuah Khan wrote: On 8/10/24 07:53, Abhinav Jain wrote: Add return value checks for read & write calls in test_listmount_ns function. This patch resolves below compilation warnings: ``` statmount_test_ns.c: In function ‘test_listmount_ns’: statmount_test_ns.c:322:17: warning: ignoring return value of ‘write’ declared with attribute ‘warn_unused_result’ [-Wunused-result] statmount_test_ns.c:323:17: warning: ignoring return value of ‘read’ declared with attribute ‘warn_unused_result’ [-Wunused-result] ``` Signed-off-by: Abhinav Jain --- Hi Christian, Let me know if it is okay to take this patch through kselftest tree. The change looks good to me. thanks, -- Shuah Thank you for the patch. Applied to linux-kselftest next for Linux 6.12-rc1 thanks, -- Shuah
Re: [PATCH] tools/latency-collector: fix -Wformat-security compile warns
On 4/3/24 19:10, Shuah Khan wrote:
Fix the following -Wformat-security compile warnings adding missing
format arguments:
latency-collector.c: In function ‘show_available’:
latency-collector.c:938:17: warning: format not a string literal and
no format arguments [-Wformat-security]
938 | warnx(no_tracer_msg);
| ^
latency-collector.c:943:17: warning: format not a string literal and
no format arguments [-Wformat-security]
943 | warnx(no_latency_tr_msg);
| ^
latency-collector.c: In function ‘find_default_tracer’:
latency-collector.c:986:25: warning: format not a string literal and
no format arguments [-Wformat-security]
986 | errx(EXIT_FAILURE, no_tracer_msg);
|
^~~~
latency-collector.c: In function ‘scan_arguments’:
latency-collector.c:1881:33: warning: format not a string literal and
no format arguments [-Wformat-security]
1881 | errx(EXIT_FAILURE, no_tracer_msg);
| ^~~~
Signed-off-by: Shuah Khan
---
tools/tracing/latency/latency-collector.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/tracing/latency/latency-collector.c
b/tools/tracing/latency/latency-collector.c
index 0fd9c747d396..cf263fe9deaf 100644
--- a/tools/tracing/latency/latency-collector.c
+++ b/tools/tracing/latency/latency-collector.c
@@ -935,12 +935,12 @@ static void show_available(void)
}
if (!tracers) {
- warnx(no_tracer_msg);
+ warnx("%s", no_tracer_msg);
return;
}
if (!found) {
- warnx(no_latency_tr_msg);
+ warnx("%s", no_latency_tr_msg);
tracefs_list_free(tracers);
return;
}
@@ -983,7 +983,7 @@ static const char *find_default_tracer(void)
for (i = 0; relevant_tracers[i]; i++) {
valid = tracer_valid(relevant_tracers[i], ¬racer);
if (notracer)
- errx(EXIT_FAILURE, no_tracer_msg);
+ errx(EXIT_FAILURE, "%s", no_tracer_msg);
if (valid)
return relevant_tracers[i];
}
@@ -1878,7 +1878,7 @@ static void scan_arguments(int argc, char *argv[])
}
valid = tracer_valid(current_tracer, ¬racer);
if (notracer)
- errx(EXIT_FAILURE, no_tracer_msg);
+ errx(EXIT_FAILURE, "%s", no_tracer_msg);
if (!valid)
errx(EXIT_FAILURE,
"The tracer %s is not supported by your kernel!\n", current_tracer);
Any thoughts on this patch?
thanks,
-- Shuah
[PATCH] tools/latency-collector: fix -Wformat-security compile warns
Fix the following -Wformat-security compile warnings adding missing
format arguments:
latency-collector.c: In function ‘show_available’:
latency-collector.c:938:17: warning: format not a string literal and
no format arguments [-Wformat-security]
938 | warnx(no_tracer_msg);
| ^
latency-collector.c:943:17: warning: format not a string literal and
no format arguments [-Wformat-security]
943 | warnx(no_latency_tr_msg);
| ^
latency-collector.c: In function ‘find_default_tracer’:
latency-collector.c:986:25: warning: format not a string literal and
no format arguments [-Wformat-security]
986 | errx(EXIT_FAILURE, no_tracer_msg);
|
^~~~
latency-collector.c: In function ‘scan_arguments’:
latency-collector.c:1881:33: warning: format not a string literal and
no format arguments [-Wformat-security]
1881 | errx(EXIT_FAILURE, no_tracer_msg);
| ^~~~
Signed-off-by: Shuah Khan
---
tools/tracing/latency/latency-collector.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/tracing/latency/latency-collector.c
b/tools/tracing/latency/latency-collector.c
index 0fd9c747d396..cf263fe9deaf 100644
--- a/tools/tracing/latency/latency-collector.c
+++ b/tools/tracing/latency/latency-collector.c
@@ -935,12 +935,12 @@ static void show_available(void)
}
if (!tracers) {
- warnx(no_tracer_msg);
+ warnx("%s", no_tracer_msg);
return;
}
if (!found) {
- warnx(no_latency_tr_msg);
+ warnx("%s", no_latency_tr_msg);
tracefs_list_free(tracers);
return;
}
@@ -983,7 +983,7 @@ static const char *find_default_tracer(void)
for (i = 0; relevant_tracers[i]; i++) {
valid = tracer_valid(relevant_tracers[i], ¬racer);
if (notracer)
- errx(EXIT_FAILURE, no_tracer_msg);
+ errx(EXIT_FAILURE, "%s", no_tracer_msg);
if (valid)
return relevant_tracers[i];
}
@@ -1878,7 +1878,7 @@ static void scan_arguments(int argc, char *argv[])
}
valid = tracer_valid(current_tracer, ¬racer);
if (notracer)
- errx(EXIT_FAILURE, no_tracer_msg);
+ errx(EXIT_FAILURE, "%s", no_tracer_msg);
if (!valid)
errx(EXIT_FAILURE,
"The tracer %s is not supported by your kernel!\n", current_tracer);
--
2.40.1
Re: [PATCH v3] Documentation: dev-tools: Add Testing Overview
tested with kselftest. To +work around this, some tests include a companion kernel module which exposes +more information or functionality. If a test runs mostly or entirely within the +kernel, however, KUnit may be the more appropriate tool. + +kselftest is therefore suited well to tests of whole features, as these will +expose an interface to userspace, which can be tested, but not implementation +details. This aligns well with 'system' or 'end-to-end' testing. + +For example, all new system calls should be accompanied by kselftest tests. + +Code Coverage Tools +=== + +The Linux Kernel supports two different code coverage measurement tools. These +can be used to verify that a test is executing particular functions or lines +of code. This is useful for determining how much of the kernel is being tested, +and for finding corner-cases which are not covered by the appropriate test. + +:doc:`gcov` is GCC's coverage testing tool, which can be used with the kernel +to get global or per-module coverage. Unlike KCOV, it does not record per-task +coverage. Coverage data can be read from debugfs, and interpreted using the +usual gcov tooling. + +:doc:`kcov` is a feature which can be built in to the kernel to allow +capturing coverage on a per-task level. It's therefore useful for fuzzing and +other situations where information about code executed during, for example, a +single syscall is useful. + + +Dynamic Analysis Tools +== + +The kernel also supports a number of dynamic analysis tools, which attempt to +detect classes of issues when they occur in a running kernel. These typically +each look for a different class of bugs, such as invalid memory accesses, +concurrency issues such as data races, or other undefined behaviour like +integer overflows. + +Some of these tools are listed below: + +* kmemleak detects possible memory leaks. See + Documentation/dev-tools/kmemleak.rst +* KASAN detects invalid memory accesses such as out-of-bounds and + use-after-free errors. See Documentation/dev-tools/kasan.rst +* UBSAN detects behaviour that is undefined by the C standard, like integer + overflows. See Documentation/dev-tools/ubsan.rst +* KCSAN detects data races. See Documentation/dev-tools/kcsan.rst +* KFENCE is a low-overhead detector of memory issues, which is much faster than + KASAN and can be used in production. See Documentation/dev-tools/kfence.rst +* lockdep is a locking correctness validator. See + Documentation/locking/lockdep-design.rst +* There are several other pieces of debug instrumentation in the kernel, many + of which can be found in lib/Kconfig.debug + +These tools tend to test the kernel as a whole, and do not "pass" like +kselftest or KUnit tests. They can be combined with KUnit or kselftest by +running tests on a kernel with these tools enabled: you can then be sure +that none of these errors are occurring during the test. + +Some of these tools integrate with KUnit or kselftest and will +automatically fail tests if an issue is detected. + Thank for you writing this much needed document. Looks great. How about adding a section for Static analysis tools? A mention coccicheck scripts and mention of smatch? thanks, -- Shuah
Re: [PATCH 5.4 00/73] 5.4.114-rc1 review
On 4/19/21 7:05 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.114 release. There are 73 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 21 Apr 2021 13:05:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.114-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 000/103] 5.10.32-rc1 review
On 4/19/21 7:05 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.32 release. There are 103 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 21 Apr 2021 13:05:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.32-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 000/122] 5.11.16-rc1 review
On 4/19/21 7:04 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.16 release. There are 122 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 21 Apr 2021 13:05:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.16-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH RFC v3] media: em28xx: Fix race condition between open and init function
On 4/16/21 1:33 PM, Igor Torrente wrote: On 4/15/21 2:25 PM, Shuah Khan wrote: On 4/15/21 8:07 AM, Igor Matheus Andrade Torrente wrote: Fixes a race condition - for lack of a more precise term - between em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev, media_pad and vdev structs from the em28xx_v4l2, and managing the lifetime of those objects more dynamicaly. The race happens when a thread[1] - containing the em28xx_v4l2_init() code - calls the v4l2_mc_create_media_graph(), and it return a error, if a thread[2] - running v4l2_open() - pass the verification point and reaches the em28xx_v4l2_open() before the thread[1] finishes the deregistration of v4l2 subsystem, the thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free. Reported-by: kernel test robot Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com Signed-off-by: Igor Matheus Andrade Torrente --- V2: Add v4l2_i2c_new_subdev null check Deal with v4l2 subdevs dependencies V3: Fix link error when compiled as a module --- drivers/media/usb/em28xx/em28xx-camera.c | 4 +- drivers/media/usb/em28xx/em28xx-video.c | 300 +++ drivers/media/usb/em28xx/em28xx.h | 6 +- 3 files changed, 209 insertions(+), 101 deletions(-) The changes looks good to me. Have you tried building as a modules and running modprobes and rmmods? You can do that without a device. I tried and everything worked fine. Thank you. Reviewed-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.4 00/38] 4.4.267-rc1 review
On 4/15/21 8:46 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.4.267 release. There are 38 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.4.267-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.14 00/68] 4.14.231-rc1 review
On 4/15/21 8:46 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.14.231 release. There are 68 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.14.231-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.14.y and the diffstat can be found below. thanks, greg k-h - Pseudo-Shortlog of commits: Greg Kroah-Hartman Linux 4.14.231-rc1 Juergen Gross xen/events: fix setting irq affinity Arnaldo Carvalho de Melo perf map: Tighten snprintf() string precision to pass gcc check on some 32-bit arches Florian Westphal netfilter: x_tables: fix compat match/target pad out-of-bound write Florian Fainelli net: phy: broadcom: Only advertise EEE for supported modes Yufen Yu block: only update parent bi_status when bio fail Bob Peterson gfs2: report "already frozen/thawed" errors Arnd Bergmann drm/imx: imx-ldb: fix out of bounds array access warning Suzuki K Poulose KVM: arm64: Disable guest access to trace filter controls Suzuki K Poulose KVM: arm64: Hide system instruction access to Trace registers Greg Kroah-Hartman Revert "cifs: Set CIFS_MOUNT_USE_PREFIX_PATH flag on setting cifs_sb->prepath." Alexander Aring net: ieee802154: stop dump llsec params for monitors Alexander Aring net: ieee802154: forbid monitor for del llsec seclevel Alexander Aring net: ieee802154: forbid monitor for set llsec params Alexander Aring net: ieee802154: fix nl802154 del llsec devkey Alexander Aring net: ieee802154: fix nl802154 add llsec key Alexander Aring net: ieee802154: fix nl802154 del llsec dev Alexander Aring net: ieee802154: fix nl802154 del llsec key Alexander Aring net: ieee802154: nl-mac: fix check on panid Pavel Skripkin net: mac802154: Fix general protection fault Pavel Skripkin drivers: net: fix memory leak in peak_usb_create_dev Pavel Skripkin drivers: net: fix memory leak in atusb_probe Phillip Potter net: tun: set tun->dev->addr_len during TUNSETLINK processing Du Cheng cfg80211: remove WARN_ON() in cfg80211_sme_connect Shuah Khan usbip: fix vudc usbip_sockfd_store races leading to gpf Samuel Mendoza-Jonas net/ncsi: Avoid GFP_KERNEL in response handler Samuel Mendoza-Jonas net/ncsi: Refactor MAC, VLAN filters Samuel Mendoza-Jonas net/ncsi: Add generic netlink family Samuel Mendoza-Jonas net/ncsi: Don't return error on normal response Samuel Mendoza-Jonas net/ncsi: Improve general state logging Wei Yongjun net/ncsi: Make local function ncsi_get_filter() static Krzysztof Kozlowski clk: socfpga: fix iomem pointer cast on 64-bit Potnuri Bharat Teja RDMA/cxgb4: check for ipv6 address properly while destroying listener Raed Salem net/mlx5: Fix placement of log_max_flow_counter Alexander Gordeev s390/cpcmd: fix inline assembly register clobbering Zqiang workqueue: Move the position of debug_work_activate() in __queue_work() Lukasz Bartosik clk: fix invalid usage of list cursor in unregister Lukasz Bartosik clk: fix invalid usage of list cursor in register Arnd Bergmann soc/fsl: qbman: fix conflicting alignment attributes Bastian Germann ASoC: sunxi: sun4i-codec: fill ASoC card owner Milton Miller net/ncsi: Avoid channel_monitor hrtimer deadlock Stefan Riedmueller ARM: dts: imx6: pbab01: Set vmmc supply for both SD interfaces Lv Yunlong net:tipc: Fix a double free in tipc_sk_mcast_rcv Claudiu Manoil gianfar: Handle error code at MAC address change Eric Dumazet sch_red: fix off-by-one checks in red_check_params() Shyam Sundar S K amd-xgbe: Update DMA coherency values Shengjiu Wang ASoC: wm8960: Fix wrong bclk and lrclk with pll enabled for some chips Geert Uytterhoeven regulator: bd9571mwv: Fix AVS and DVFS voltage range Wolfram Sang i2c: turn recovery error on init to debug Shuah Khan usbip: synchronize event handler with sysfs code paths Shuah Khan usbip: stub-dev synchronize sysfs code paths Shuah Khan usbip: add sysfs_lock to synchronize sysfs code paths Pavel Tikhomirov net: sched: sch_teql: fix null-pointer dereference Eric Dumazet net: ensure mac header is set in virtio_net_hdr_to_skb() Tetsuo Handa batman-adv: initialize "struct batadv_tvlv_tt_vlan_data"->reserved field Marek Behún ARM: dts: turris-omnia: configure LED[2]/INTn pin as
Re: [PATCH 4.9 00/47] 4.9.267-rc1 review
On 4/15/21 8:46 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.9.267 release. There are 47 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.267-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.14 00/68] 4.14.231-rc1 review
On 4/15/21 8:46 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.14.231 release. There are 68 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.14.231-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.14.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.4 00/18] 5.4.113-rc1 review
On 4/15/21 8:47 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.113 release. There are 18 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.113-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.19 00/13] 4.19.188-rc1 review
On 4/15/21 8:47 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.188 release. There are 13 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.188-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 00/25] 5.10.31-rc1 review
On 4/15/21 8:47 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.31 release. There are 25 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.31-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 00/23] 5.11.15-rc1 review
On 4/15/21 8:48 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.15 release. There are 23 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sat, 17 Apr 2021 14:44:01 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.15-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH RFC v3] media: em28xx: Fix race condition between open and init function
On 4/15/21 8:07 AM, Igor Matheus Andrade Torrente wrote: Fixes a race condition - for lack of a more precise term - between em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev, media_pad and vdev structs from the em28xx_v4l2, and managing the lifetime of those objects more dynamicaly. The race happens when a thread[1] - containing the em28xx_v4l2_init() code - calls the v4l2_mc_create_media_graph(), and it return a error, if a thread[2] - running v4l2_open() - pass the verification point and reaches the em28xx_v4l2_open() before the thread[1] finishes the deregistration of v4l2 subsystem, the thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free. Reported-by: kernel test robot Reported-and-tested-by: syzbot+b2391895514ed9ef4...@syzkaller.appspotmail.com Signed-off-by: Igor Matheus Andrade Torrente --- V2: Add v4l2_i2c_new_subdev null check Deal with v4l2 subdevs dependencies V3: Fix link error when compiled as a module --- drivers/media/usb/em28xx/em28xx-camera.c | 4 +- drivers/media/usb/em28xx/em28xx-video.c | 300 +++ drivers/media/usb/em28xx/em28xx.h| 6 +- 3 files changed, 209 insertions(+), 101 deletions(-) The changes looks good to me. Have you tried building as a modules and running modprobes and rmmods? You can do that without a device. thanks, -- Shuah
Re: [PATCH v3] firmware_loader: fix use-after-free in firmware_fallback_sysfs
On 4/14/21 9:26 AM, Shuah Khan wrote:
On 4/14/21 6:55 AM, Luis Chamberlain wrote:
Shuah, a question for you toward the end here.
On Wed, Apr 14, 2021 at 02:24:05PM +0530, Anirudh Rayabharam wrote:
This use-after-free happens when a fw_priv object has been freed but
hasn't been removed from the pending list (pending_fw_head). The next
time fw_load_sysfs_fallback tries to insert into the list, it ends up
accessing the pending_list member of the previoiusly freed fw_priv.
The root cause here is that all code paths that abort the fw load
don't delete it from the pending list. For example:
_request_firmware()
-> fw_abort_batch_reqs()
-> fw_state_aborted()
To fix this, delete the fw_priv from the list in __fw_set_state() if
the new state is DONE or ABORTED. This way, all aborts will remove
the fw_priv from the list. Accordingly, remove calls to list_del_init
that were being made before calling fw_state_(aborted|done).
Also, in fw_load_sysfs_fallback, don't add the fw_priv to the pending
list if it is already aborted. Instead, just jump out and return early.
Fixes: bcfbd3523f3c ("firmware: fix a double abort case with
fw_load_sysfs_fallback")
Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam
---
Changes in v3:
Modified the patch to incorporate suggestions by Luis Chamberlain in
order to fix the root cause instead of applying a "band-aid" kind of
fix.
https://lore.kernel.org/lkml/20210403013143.gv4...@42.do-not-panic.com/
Changes in v2:
1. Fixed 1 error and 1 warning (in the commit message) reported by
checkpatch.pl. The error was regarding the format for referring to
another commit "commit ("oneline")". The warning was for line
longer than 75 chars.
---
drivers/base/firmware_loader/fallback.c | 8 ++--
drivers/base/firmware_loader/firmware.h | 6 +-
drivers/base/firmware_loader/main.c | 2 ++
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..73581b6998b4 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -94,7 +94,6 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
if (fw_sysfs_done(fw_priv))
return;
- list_del_init(&fw_priv->pending_list);
fw_state_aborted(fw_priv);
}
@@ -280,7 +279,6 @@ static ssize_t firmware_loading_store(struct
device *dev,
* Same logic as fw_load_abort, only the DONE bit
* is ignored and we set ABORT only on failure.
*/
- list_del_init(&fw_priv->pending_list);
if (rc) {
fw_state_aborted(fw_priv);
written = rc;
@@ -513,6 +511,11 @@ static int fw_load_sysfs_fallback(struct
fw_sysfs *fw_sysfs, long timeout)
}
mutex_lock(&fw_lock);
+ if (fw_state_is_aborted(fw_priv)) {
+ mutex_unlock(&fw_lock);
+ retval = -EAGAIN;
+ goto out;
+ }
Thanks for the quick follow up!
This would regress commit 76098b36b5db1 ("firmware: send -EINTR on
signal abort on fallback mechanism") which I had mentioned in my follow
up email you posted a link to. It would regress it since the condition
is just being met earlier and you nullify the effort. So essentially
on Android you would make not being able to detect signal handlers
like the SIGCHLD signal sent to init, if init was the same process
dealing with the sysfs fallback firmware upload.
The way I dealt with this in my patch was I decided to return -EINTR
in the earlier case in the hunk you added, instead of -EAGAIN. In
addition to this, later on fw_load_sysfs_fallback() when
fw_sysfs_wait_timeout() is used that would also deal with checking
for error codes on wait, and only then check if it was a signal
that cancelled things (the check for -ERESTARTSYS). We therefore
only send to userspace -EAGAIN when the wait really did hit the
timeout.
But also note that my change added a check for
fw_state_is_aborted(fw_priv) inside fw_sysfs_wait_timeout(),
as that was a recently intended goal.
In either case I documented well *why* we do these error checks
before sending a code to userspace on fw_sysfs_wait_timeout() since
otherwise it would be easy to regress that code, so please also
document that as I did.
I'll re-iterate again also:
Shuah's commit 0542ad88fbdd81bb ("firmware loader: Fix
_request_firmware_load() return val for fw load abort") also
wanted to
distinguish the timeout vs -ENOMEM, but for some reason in the
timeout
case -EAGAIN was being sent back to userspace. I am no longer sure if
that is a good idea, but since we started doing that at some point I
guess we want to keep that behaviour.
Shuah, can you t
Re: [PATCH v3] firmware_loader: fix use-after-free in firmware_fallback_sysfs
On 4/14/21 6:55 AM, Luis Chamberlain wrote:
Shuah, a question for you toward the end here.
On Wed, Apr 14, 2021 at 02:24:05PM +0530, Anirudh Rayabharam wrote:
This use-after-free happens when a fw_priv object has been freed but
hasn't been removed from the pending list (pending_fw_head). The next
time fw_load_sysfs_fallback tries to insert into the list, it ends up
accessing the pending_list member of the previoiusly freed fw_priv.
The root cause here is that all code paths that abort the fw load
don't delete it from the pending list. For example:
_request_firmware()
-> fw_abort_batch_reqs()
-> fw_state_aborted()
To fix this, delete the fw_priv from the list in __fw_set_state() if
the new state is DONE or ABORTED. This way, all aborts will remove
the fw_priv from the list. Accordingly, remove calls to list_del_init
that were being made before calling fw_state_(aborted|done).
Also, in fw_load_sysfs_fallback, don't add the fw_priv to the pending
list if it is already aborted. Instead, just jump out and return early.
Fixes: bcfbd3523f3c ("firmware: fix a double abort case with
fw_load_sysfs_fallback")
Reported-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Tested-by: syzbot+de271708674e20930...@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam
---
Changes in v3:
Modified the patch to incorporate suggestions by Luis Chamberlain in
order to fix the root cause instead of applying a "band-aid" kind of
fix.
https://lore.kernel.org/lkml/20210403013143.gv4...@42.do-not-panic.com/
Changes in v2:
1. Fixed 1 error and 1 warning (in the commit message) reported by
checkpatch.pl. The error was regarding the format for referring to
another commit "commit ("oneline")". The warning was for line
longer than 75 chars.
---
drivers/base/firmware_loader/fallback.c | 8 ++--
drivers/base/firmware_loader/firmware.h | 6 +-
drivers/base/firmware_loader/main.c | 2 ++
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 91899d185e31..73581b6998b4 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -94,7 +94,6 @@ static void __fw_load_abort(struct fw_priv *fw_priv)
if (fw_sysfs_done(fw_priv))
return;
- list_del_init(&fw_priv->pending_list);
fw_state_aborted(fw_priv);
}
@@ -280,7 +279,6 @@ static ssize_t firmware_loading_store(struct device *dev,
* Same logic as fw_load_abort, only the DONE bit
* is ignored and we set ABORT only on failure.
*/
- list_del_init(&fw_priv->pending_list);
if (rc) {
fw_state_aborted(fw_priv);
written = rc;
@@ -513,6 +511,11 @@ static int fw_load_sysfs_fallback(struct fw_sysfs
*fw_sysfs, long timeout)
}
mutex_lock(&fw_lock);
+ if (fw_state_is_aborted(fw_priv)) {
+ mutex_unlock(&fw_lock);
+ retval = -EAGAIN;
+ goto out;
+ }
Thanks for the quick follow up!
This would regress commit 76098b36b5db1 ("firmware: send -EINTR on
signal abort on fallback mechanism") which I had mentioned in my follow
up email you posted a link to. It would regress it since the condition
is just being met earlier and you nullify the effort. So essentially
on Android you would make not being able to detect signal handlers
like the SIGCHLD signal sent to init, if init was the same process
dealing with the sysfs fallback firmware upload.
The way I dealt with this in my patch was I decided to return -EINTR
in the earlier case in the hunk you added, instead of -EAGAIN. In
addition to this, later on fw_load_sysfs_fallback() when
fw_sysfs_wait_timeout() is used that would also deal with checking
for error codes on wait, and only then check if it was a signal
that cancelled things (the check for -ERESTARTSYS). We therefore
only send to userspace -EAGAIN when the wait really did hit the
timeout.
But also note that my change added a check for
fw_state_is_aborted(fw_priv) inside fw_sysfs_wait_timeout(),
as that was a recently intended goal.
In either case I documented well *why* we do these error checks
before sending a code to userspace on fw_sysfs_wait_timeout() since
otherwise it would be easy to regress that code, so please also
document that as I did.
I'll re-iterate again also:
Shuah's commit 0542ad88fbdd81bb ("firmware loader: Fix
_request_firmware_load() return val for fw load abort") also wanted to
distinguish the timeout vs -ENOMEM, but for some reason in the timeout
case -EAGAIN was being sent back to userspace. I am no longer sure if
that is a good id
Re: [PATCH 4.19 00/66] 4.19.187-rc1 review
On 4/12/21 2:40 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.187 release. There are 66 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 14 Apr 2021 08:39:44 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.187-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. No problems with wifi this time. I will be on the lookout for this in the future. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.4 000/111] 5.4.112-rc1 review
On 4/12/21 2:39 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.112 release. There are 111 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 14 Apr 2021 08:39:44 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.112-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 000/188] 5.10.30-rc1 review
On 4/12/21 2:38 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.30 release. There are 188 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 14 Apr 2021 08:39:44 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.30-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 000/210] 5.11.14-rc1 review
On 4/12/21 2:38 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.14 release. There are 210 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 14 Apr 2021 08:39:44 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.14-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.9 00/13] 4.9.266-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.9.266 release. There are 13 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.266-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.19 00/18] 4.19.186-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.186 release. There are 18 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.186-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h I am seeing a new warn - will debug later on today and let you know what I find: WARNING: CPU: 9 PID: 0 at drivers/net/wireless/ath/ath10k/htt_rx.c:46 ath10k_htt_rx_pop_paddr+0xde/0x100 [ath10k_core] Modules linked in: cmac algif_hash algif_skcipher af_alg bnep arc4 nls_iso8859_1 wmi_bmof snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi edac_mce_amd snd_hda_intel snd_hda_codec kvm_amd snd_hda_core ccp snd_hwdep kvm snd_pcm snd_seq_midi crct10dif_pclmul snd_seq_midi_event ghash_clmulni_intel pcbc snd_rawmidi ath10k_pci snd_seq ath10k_core aesni_intel ath snd_seq_device rtsx_usb_ms btusb aes_x86_64 snd_timer crypto_simd btrtl cryptd joydev btbcm glue_helper memstick mac80211 snd btintel input_leds bluetooth soundcore cfg80211 ecdh_generic video wmi mac_hid sch_fq_codel parport_pc ppdev lp parport drm ip_tables x_tables autofs4 hid_generic rtsx_usb_sdmmc usbhid rtsx_usb hid crc32_pclmul uas i2c_piix4 r8169 ahci realtek usb_storage libahci gpio_amdpt gpio_generic CPU: 9 PID: 0 Comm: swapper/9 Not tainted 4.19.186-rc1+ #24 Hardware name: LENOVO 90Q30008US/3728, BIOS O4ZKT1CA 09/16/2020 RIP: 0010:ath10k_htt_rx_pop_paddr+0xde/0x100 [ath10k_core] Code: 02 00 00 48 85 c9 74 30 4c 8b 49 28 4d 85 c9 74 1e 48 8b 30 45 31 c0 b9 02 00 00 00 e8 9b 27 ca cc 4c 89 e0 4c 8b 65 f8 c9 c3 <0f> 0b 45 31 e4 4c 89 e0 4c 8b 65 f8 c9 c3 48 8b 0d 1d df 4c cd eb RSP: 0018:8d81bf043da0 EFLAGS: 00010246 RAX: RBX: 8d81927b2150 RCX: 8d81b8c01580 RDX: 0008 RSI: ff708a80 RDI: 8d81b8c01e78 RBP: 8d81bf043da8 R08: 0020 R09: R10: dd1f0f40f300 R11: 000ff000 R12: 8d81b8c02068 R13: 8d81b8c01580 R14: 8d81927b2148 R15: 8d81b8c01580 FS: () GS:8d81bf04() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 55a2c99a2000 CR3: 0003de8d4000 CR4: 00340ee0 Call Trace: ath10k_htt_txrx_compl_task+0x58d/0xe70 [ath10k_core] ath10k_pci_napi_poll+0x52/0x110 [ath10k_pci] net_rx_action+0x13c/0x350 __do_softirq+0xd4/0x2ae irq_exit+0x9c/0xe0 do_IRQ+0x86/0xe0 common_interrupt+0xf/0xf RIP: 0010:cpuidle_enter_state+0x10b/0x2c0 Code: ff e8 f9 68 85 ff 80 7d c7 00 74 17 9c 58 0f 1f 44 00 00 f6 c4 02 0f 85 97 01 00 00 31 ff e8 6c 5d 8b ff fb 66 0f 1f 44 00 00 <48> b8 ff ff ff ff f3 01 00 00 4c 2b 7d c8 ba ff ff ff 7f 49 39 c7 RSP: 0018:b11e01a77e50 EFLAGS: 0246 ORIG_RAX: ffda RAX: 8d81bf0626c0 RBX: 8d81b2690400 RCX: 0006e8cb49d2 RDX: 0689 RSI: 0006e8cb49d2 RDI: RBP: b11e01a77e90 R08: 0006e8cb505b R09: 0e29 R10: 0f04 R11: 8d81bf061528 R12: 0003 R13: 8df9e860 R14: 8df9e980 R15: 0006e8cb505b cpuidle_enter+0x17/0x20 thanks, -- Shuah
Re: [PATCH 5.4 00/23] 5.4.111-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.111 release. There are 23 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.111-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 00/41] 5.10.29-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.29 release. There are 41 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.29-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 00/45] 5.11.13-rc1 review
On 4/9/21 3:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.13 release. There are 45 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Sun, 11 Apr 2021 09:52:52 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.13-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On 4/9/21 2:00 PM, Shuah Khan wrote:
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
In early AMD desktop/mobile platforms (during 2013), when the IOMMU
Performance Counter (PMC) support was first introduced in
commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
resource management"), there was a HW bug where the counters could not
be accessed. The result was reading of the counter always return zero.
At the time, the suggested workaround was to add a test logic prior
to initializing the PMC feature to check if the counters can be
programmed
and read back the same value. This has been working fine until the more
recent desktop/mobile platforms start enabling power gating for the PMC,
which prevents access to the counters. This results in the PMC support
being disabled unnecesarily.
Unfortunatly, there is no documentation of since which generation
of hardware the original PMC HW bug was fixed. Although, it was fixed
soon after the first introduction of the PMC. Base on this, we assume
that the buggy platforms are less likely to be in used, and it should
be relatively safe to remove this legacy logic.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Cc: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
Tested-by: Shuah Khan
Revert + this patch - same as my test on Ryzen 5
On AMD Ryzen 7 4700G with Radeon Graphics
These look real odd to me. Let me know if I should look further.
sudo ./perf stat -e 'amd_iommu_0/cmd_processed/,
amd_iommu_0/cmd_processed_inv/, amd_iommu_0/ign_rd_wr_mmio_1ff8h/,
amd_iommu_0/int_dte_hit/, amd_iommu_0/int_dte_mis/,
amd_iommu_0/mem_dte_hit/, amd_iommu_0/mem_dte_mis/,
amd_iommu_0/mem_iommu_tlb_pde_hit/, amd_iommu_0/mem_iommu_tlb_pde_mis/,
amd_iommu_0/mem_iommu_tlb_pte_hit/, amd_iommu_0/mem_iommu_tlb_pte_mis/,
amd_iommu_0/mem_pass_excl/, amd_iommu_0/mem_pass_pretrans/,
amd_iommu_0/mem_pass_untrans/, amd_iommu_0/mem_target_abort/,
amd_iommu_0/mem_trans_total/, amd_iommu_0/page_tbl_read_gst/,
amd_iommu_0/page_tbl_read_nst/, amd_iommu_0/page_tbl_read_tot/,
amd_iommu_0/smi_blk/, amd_iommu_0/smi_recv/, amd_iommu_0/tlb_inv/,
amd_iommu_0/vapic_int_guest/, amd_iommu_0/vapic_int_non_guest/' sleep 10
Performance counter stats for 'system wide':
17,761,952,514,865,374 amd_iommu_0/cmd_processed/
(33.28%)
18,582,155,570,607,472 amd_iommu_0/cmd_processed_inv/
(33.32%)
0 amd_iommu_0/ign_rd_wr_mmio_1ff8h/
(33.36%)
5,056,087,645,262,255 amd_iommu_0/int_dte_hit/
(33.40%)
32,831,106,446,308,888 amd_iommu_0/int_dte_mis/
(33.44%)
13,461,819,655,591,296 amd_iommu_0/mem_dte_hit/
(33.45%)
208,555,436,221,050,464 amd_iommu_0/mem_dte_mis/
(33.47%)
196,824,154,635,609,888 amd_iommu_0/mem_iommu_tlb_pde_hit/
(33.46%)
193,552,630,440,410,144 amd_iommu_0/mem_iommu_tlb_pde_mis/
(33.45%)
176,936,647,809,098,368 amd_iommu_0/mem_iommu_tlb_pte_hit/
(33.41%)
184,737,401,623,626,464 amd_iommu_0/mem_iommu_tlb_pte_mis/
(33.37%)
0 amd_iommu_0/mem_pass_excl/
(33.33%)
0 amd_iommu_0/mem_pass_pretrans/
(33.30%)
0 amd_iommu_0/mem_pass_untrans/
(33.28%)
0 amd_iommu_0/mem_target_abort/
(33.27%)
245,383,212,924,004,288 amd_iommu_0/mem_trans_total/
(33.27%)
0 amd_iommu_0/page_tbl_read_gst/
(33.28%)
262,267,045,917,967,264 amd_iommu_0/page_tbl_read_nst/
(33.27%)
256,308,216,913,137,600 amd_iommu_0/page_tbl_read_tot/
(33.28%)
0 amd_iommu_0/smi_blk/
(33.27%)
0 amd_iommu_0/smi_recv/
(33.27%)
0 amd_iommu_0/tlb_inv/
(33.27%)
0 amd_iommu_0/vapic_int_guest/
(33.26%)
38,913,544,420,579,888 amd_iommu_0/vapic_int_non_guest/
(33.27%)
10.003967760 seconds time elapsed
thanks,
-- Shuah
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
In early AMD desktop/mobile platforms (during 2013), when the IOMMU
Performance Counter (PMC) support was first introduced in
commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
resource management"), there was a HW bug where the counters could not
be accessed. The result was reading of the counter always return zero.
At the time, the suggested workaround was to add a test logic prior
to initializing the PMC feature to check if the counters can be programmed
and read back the same value. This has been working fine until the more
recent desktop/mobile platforms start enabling power gating for the PMC,
which prevents access to the counters. This results in the PMC support
being disabled unnecesarily.
Unfortunatly, there is no documentation of since which generation
of hardware the original PMC HW bug was fixed. Although, it was fixed
soon after the first introduction of the PMC. Base on this, we assume
that the buggy platforms are less likely to be in used, and it should
be relatively safe to remove this legacy logic.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Cc: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
Tested-by: Shuah Khan
thanks,
-- Shuah
Results with this patch on AMD Ryzen 5 PRO 2400GE w/ Radeon Vega
Graphics
sudo ./perf stat -e 'amd_iommu_0/cmd_processed/,
amd_iommu_0/cmd_processed_inv/, amd_iommu_0/ign_rd_wr_mmio_1ff8h/,
amd_iommu_0/int_dte_hit/, amd_iommu_0/int_dte_mis/,
amd_iommu_0/mem_dte_hit/, amd_iommu_0/mem_dte_mis/,
amd_iommu_0/mem_iommu_tlb_pde_hit/, amd_iommu_0/mem_iommu_tlb_pde_mis/,
amd_iommu_0/mem_iommu_tlb_pte_hit/, amd_iommu_0/mem_iommu_tlb_pte_mis/,
amd_iommu_0/mem_pass_excl/, amd_iommu_0/mem_pass_pretrans/,
amd_iommu_0/mem_pass_untrans/, amd_iommu_0/mem_target_abort/,
amd_iommu_0/mem_trans_total/, amd_iommu_0/page_tbl_read_gst/,
amd_iommu_0/page_tbl_read_nst/, amd_iommu_0/page_tbl_read_tot/,
amd_iommu_0/smi_blk/, amd_iommu_0/smi_recv/, amd_iommu_0/tlb_inv/,
amd_iommu_0/vapic_int_guest/, amd_iommu_0/vapic_int_non_guest/' sleep 10
Performance counter stats for 'system wide':
156 amd_iommu_0/cmd_processed/
(33.30%)
80 amd_iommu_0/cmd_processed_inv/
(33.38%)
0 amd_iommu_0/ign_rd_wr_mmio_1ff8h/
(33.40%)
0 amd_iommu_0/int_dte_hit/
(33.43%)
325 amd_iommu_0/int_dte_mis/
(33.44%)
1,951 amd_iommu_0/mem_dte_hit/
(33.45%)
7,589 amd_iommu_0/mem_dte_mis/
(33.49%)
325 amd_iommu_0/mem_iommu_tlb_pde_hit/
(33.45%)
2,460 amd_iommu_0/mem_iommu_tlb_pde_mis/
(33.41%)
2,510 amd_iommu_0/mem_iommu_tlb_pte_hit/
(33.38%)
5,526 amd_iommu_0/mem_iommu_tlb_pte_mis/
(33.33%)
0 amd_iommu_0/mem_pass_excl/
(33.29%)
0 amd_iommu_0/mem_pass_pretrans/
(33.28%)
1,556 amd_iommu_0/mem_pass_untrans/
(33.27%)
0 amd_iommu_0/mem_target_abort/
(33.26%)
3,112 amd_iommu_0/mem_trans_total/
(33.29%)
0 amd_iommu_0/page_tbl_read_gst/
(33.29%)
1,813 amd_iommu_0/page_tbl_read_nst/
(33.25%)
2,242 amd_iommu_0/page_tbl_read_tot/
(33.27%)
0 amd_iommu_0/smi_blk/
(33.29%)
0 amd_iommu_0/smi_recv/
(33.28%)
0 amd_iommu_0/tlb_inv/
(33.28%)
0 amd_iommu_0/vapic_int_guest/
(33.25%)
0 amd_iommu_0/vapic_int_non_guest/
(33.26%)
10.003200316 seconds time elapsed
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On 4/9/21 10:37 AM, Shuah Khan wrote:
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
In early AMD desktop/mobile platforms (during 2013), when the IOMMU
Performance Counter (PMC) support was first introduced in
commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
resource management"), there was a HW bug where the counters could not
be accessed. The result was reading of the counter always return zero.
At the time, the suggested workaround was to add a test logic prior
to initializing the PMC feature to check if the counters can be
programmed
and read back the same value. This has been working fine until the more
recent desktop/mobile platforms start enabling power gating for the PMC,
which prevents access to the counters. This results in the PMC support
being disabled unnecesarily.
unnecessarily
Unfortunatly, there is no documentation of since which generation
Unfortunately,
Rephrase suggestion:
Unfortunately, it is unclear when the PMC HW bug fixed.
of hardware the original PMC HW bug was fixed. Although, it was fixed
soon after the first introduction of the PMC. Base on this, we assume
Based
that the buggy platforms are less likely to be in used, and it should
in use
be relatively safe to remove this legacy logic.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Cc: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
drivers/iommu/amd/init.c | 24 +---
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 648cdfd03074..247cdda5d683 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1714,33 +1714,16 @@ static int __init init_iommu_all(struct
acpi_table_header *table)
return 0;
}
-static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8
cntr,
- u8 fxn, u64 *value, bool is_write);
-
static void init_iommu_perf_ctr(struct amd_iommu *iommu)
{
+ u64 val;
struct pci_dev *pdev = iommu->dev;
- u64 val = 0xabcd, val2 = 0, save_reg = 0;
Why not leave this u64 val here? Having the pdev assignment as the
first line makes it easier to read/follow.
if (!iommu_feature(iommu, FEATURE_PC))
return;
amd_iommu_pc_present = true;
- /* save the value to restore, if writable */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false))
- goto pc_false;
-
- /* Check if the performance counters can be written to */
- if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) ||
- (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) ||
- (val != val2))
Aha - this is going away anyway. Please ignore my comment on 1/2
about parenthesis around (val != val2) being unnecessary.
- goto pc_false;
-
- /* restore */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true))
- goto pc_false;
-
pci_info(pdev, "IOMMU performance counters supported\n");
val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET);
@@ -1748,11 +1731,6 @@ static void init_iommu_perf_ctr(struct
amd_iommu *iommu)
iommu->max_counters = (u8) ((val >> 7) & 0xf);
return;
-
-pc_false:
- pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n");
- amd_iommu_pc_present = false;
- return;
}
static ssize_t amd_iommu_show_cap(struct device *dev,
thanks,
-- Shuah
Re: [PATCH 1/2] Revert "iommu/amd: Fix performance counter initialization"
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
From: Paul Menzel
This reverts commit 6778ff5b21bd8e78c8bd547fd66437cf2657fd9b.
The original commit tries to address an issue, where PMC power-gating
causing the IOMMU PMC pre-init test to fail on certain desktop/mobile
platforms where the power-gating is normally enabled.
There have been several reports that the workaround still does not
guarantee to work, and can add up to 100 ms (on the worst case)
to the boot process on certain platforms such as the MSI B350M MORTAR
with AMD Ryzen 3 2200G.
Therefore, revert this commit as a prelude to removing the pre-init
test.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Signed-off-by: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
Note: I have revised the commit message to add more detail
and remove uncessary information.
drivers/iommu/amd/init.c | 45 ++--
1 file changed, 11 insertions(+), 34 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 321f5906e6ed..648cdfd03074 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -12,7 +12,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -257,8 +256,6 @@ static enum iommu_init_state init_state = IOMMU_START_STATE;
static int amd_iommu_enable_interrupts(void);
static int __init iommu_go_to_state(enum iommu_init_state state);
static void init_device_table_dma(void);
-static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
- u8 fxn, u64 *value, bool is_write);
static bool amd_iommu_pre_enabled = true;
@@ -1717,11 +1714,13 @@ static int __init init_iommu_all(struct acpi_table_header *table)
return 0;
}
-static void __init init_iommu_perf_ctr(struct amd_iommu *iommu)
+static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
+ u8 fxn, u64 *value, bool is_write);
+
+static void init_iommu_perf_ctr(struct amd_iommu *iommu)
{
- int retry;
struct pci_dev *pdev = iommu->dev;
- u64 val = 0xabcd, val2 = 0, save_reg, save_src;
+ u64 val = 0xabcd, val2 = 0, save_reg = 0;
if (!iommu_feature(iommu, FEATURE_PC))
return;
@@ -1729,39 +1728,17 @@ static void __init init_iommu_perf_ctr(struct amd_iommu
*iommu)
amd_iommu_pc_present = true;
/* save the value to restore, if writable */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false) ||
- iommu_pc_get_set_reg(iommu, 0, 0, 8, &save_src, false))
- goto pc_false;
-
- /*
-* Disable power gating by programing the performance counter
-* source to 20 (i.e. counts the reads and writes from/to IOMMU
-* Reserved Register [MMIO Offset 1FF8h] that are ignored.),
-* which never get incremented during this init phase.
-* (Note: The event is also deprecated.)
-*/
- val = 20;
- if (iommu_pc_get_set_reg(iommu, 0, 0, 8, &val, true))
+ if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false))
goto pc_false;
/* Check if the performance counters can be written to */
- val = 0xabcd;
- for (retry = 5; retry; retry--) {
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true) ||
- iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false) ||
- val2)
- break;
-
- /* Wait about 20 msec for power gating to disable and retry. */
- msleep(20);
- }
-
- /* restore */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true) ||
- iommu_pc_get_set_reg(iommu, 0, 0, 8, &save_src, true))
+ if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) ||
+ (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) ||
+ (val != val2))
Probably don't need parentheses around 'val != val2'
goto pc_false;
- if (val != val2)
+ /* restore */
+ if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true))
goto pc_false;
pci_info(pdev, "IOMMU performance counters supported\n");
thanks,
-- Shuah
Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test
On 4/9/21 2:58 AM, Suravee Suthikulpanit wrote:
In early AMD desktop/mobile platforms (during 2013), when the IOMMU
Performance Counter (PMC) support was first introduced in
commit 30861ddc9cca ("perf/x86/amd: Add IOMMU Performance Counter
resource management"), there was a HW bug where the counters could not
be accessed. The result was reading of the counter always return zero.
At the time, the suggested workaround was to add a test logic prior
to initializing the PMC feature to check if the counters can be programmed
and read back the same value. This has been working fine until the more
recent desktop/mobile platforms start enabling power gating for the PMC,
which prevents access to the counters. This results in the PMC support
being disabled unnecesarily.
unnecessarily
Unfortunatly, there is no documentation of since which generation
Unfortunately,
Rephrase suggestion:
Unfortunately, it is unclear when the PMC HW bug fixed.
of hardware the original PMC HW bug was fixed. Although, it was fixed
soon after the first introduction of the PMC. Base on this, we assume
Based
that the buggy platforms are less likely to be in used, and it should
in use
be relatively safe to remove this legacy logic.
Link:
https://lore.kernel.org/linux-iommu/alpine.lnx.3.20.13.2006030935570.3...@monopod.intra.ispras.ru/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201753
Cc: Tj (Elloe Linux)
Cc: Shuah Khan
Cc: Alexander Monakov
Cc: David Coe
Cc: Paul Menzel
Signed-off-by: Suravee Suthikulpanit
---
drivers/iommu/amd/init.c | 24 +---
1 file changed, 1 insertion(+), 23 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 648cdfd03074..247cdda5d683 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1714,33 +1714,16 @@ static int __init init_iommu_all(struct
acpi_table_header *table)
return 0;
}
-static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
- u8 fxn, u64 *value, bool is_write);
-
static void init_iommu_perf_ctr(struct amd_iommu *iommu)
{
+ u64 val;
struct pci_dev *pdev = iommu->dev;
- u64 val = 0xabcd, val2 = 0, save_reg = 0;
if (!iommu_feature(iommu, FEATURE_PC))
return;
amd_iommu_pc_present = true;
- /* save the value to restore, if writable */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, false))
- goto pc_false;
-
- /* Check if the performance counters can be written to */
- if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) ||
- (iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) ||
- (val != val2))
- goto pc_false;
-
- /* restore */
- if (iommu_pc_get_set_reg(iommu, 0, 0, 0, &save_reg, true))
- goto pc_false;
-
pci_info(pdev, "IOMMU performance counters supported\n");
val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET);
@@ -1748,11 +1731,6 @@ static void init_iommu_perf_ctr(struct amd_iommu *iommu)
iommu->max_counters = (u8) ((val >> 7) & 0xf);
return;
-
-pc_false:
- pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n");
- amd_iommu_pc_present = false;
- return;
}
static ssize_t amd_iommu_show_cap(struct device *dev,
I will test this patch and the revert on my two AMD systems and update
you in a day or two.
thanks,
-- Shuah
Re: [PATCH] media: em28xx: Fix race condition between open and init function
dev_info(&dev->intf->dev,
@@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
/* Save some power by putting tuner to sleep */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
/* initialize videobuf2 stuff */
em28xx_vb2_setup(dev);
@@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
video_unregister_device(&v4l2->vbi_dev);
}
- if (video_is_registered(&v4l2->vdev)) {
+ if (video_is_registered(v4l2->vdev)) {
dev_info(&dev->intf->dev,
"V4L2 device %s deregistered\n",
-video_device_node_name(&v4l2->vdev));
- video_unregister_device(&v4l2->vdev);
+video_device_node_name(v4l2->vdev));
+ video_unregister_device(v4l2->vdev);
}
v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
- v4l2_device_unregister(&v4l2->v4l2_dev);
-err:
+ v4l2_device_unregister(v4l2->v4l2_dev);
+
+v4l2_device_register_error:
+ v4l2_device_put(v4l2->v4l2_dev);
+v4l2_dev_error:
dev->v4l2 = NULL;
kref_put(&v4l2->ref, em28xx_free_v4l2);
+v4l2_error:
mutex_unlock(&dev->lock);
return ret;
}
diff --git a/drivers/media/usb/em28xx/em28xx.h
b/drivers/media/usb/em28xx/em28xx.h
index 6648e11f1271..dbcc297b5a0d 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -552,10 +552,10 @@ struct em28xx_v4l2 {
struct kref ref;
struct em28xx *dev;
- struct v4l2_device v4l2_dev;
+ struct v4l2_device *v4l2_dev;
struct v4l2_ctrl_handler ctrl_handler;
- struct video_device vdev;
+ struct video_device *vdev;
struct video_device vbi_dev;
struct video_device radio_dev;
@@ -601,7 +601,7 @@ struct em28xx_v4l2 {
unsigned int field_count;
#ifdef CONFIG_MEDIA_CONTROLLER
- struct media_pad video_pad, vbi_pad;
+ struct media_pad *video_pad, vbi_pad;
struct media_entity *decoder;
#endif
};
thanks,
-- Shuah
Re: [PATCH] usbip: vhci_hcd: do proper error handling
On 3/31/21 5:23 AM, Muhammad Usama Anjum wrote: On Fri, 2021-03-26 at 14:24 -0600, Shuah Khan wrote: On 3/25/21 5:46 AM, Muhammad Usama Anjum wrote: The driver was assuming that all the parameters would be valid. But it is possible that parameters are sent from userspace. For those cases, appropriate error checks have been added. Are you sure this change is necessary for vhci_hcd? Is there a scenario where vhci_hcd will receive these values from userspace? I'm not sure if these changes are necessary for vhci_hcd. I'd sent this patch following the motivation of a patch (c318840fb2) from dummy_hcd to handle some cases. Yeah, there is scenario where vhci_hcd will receive these values from userspace. For example, typReq = SetPortFeature and wValue = USB_PORT_FEAT_C_CONNECTION can be received from userspace. As USB_PORT_FEAT_C_CONNECTION case isn't being handled, default case will is executed for it. So I'm assuming USB_PORT_FEAT_C_CONNECTION isn't supported and default case shouldn't be executed. The way dummy_hcd handles USB_PORT_FEAT_C_CONNECTION isn't applicable to vhci_hcd. rh_port_connect() and rh_port_disconnect() set the USB_PORT_FEAT_C_CONNECTION this flag to initiate port status polling. This flag is set by the driver as a result of attach/deatch request from the user-space. Current handling of this flag is correct. Is there a way to reproduce the problem? I'm not able to reproduce any problem. But typReq = SetPortFeature and wValue = USB_PORT_FEAT_C_CONNECTION may trigger some behavior which isn't intended as USB_PORT_FEAT_C_CONNECTION may not be supported for vhci_hcd. If you reproduce the problem and it impacts attach/detach and device remote device access, we can to look into this further. thanks, -- Shuah
Re: [PATCH v2] selftests/resctrl: Change a few printed messages
On 4/7/21 1:57 PM, Fenghua Yu wrote:
Change a few printed messages to report test progress more clearly.
Add a missing "\n" at the end of one printed message.
Suggested-by: Shuah Khan
Signed-off-by: Fenghua Yu
---
Change log:
v2:
- Add "Pass:" and "Fail:" sub-strings back (Shuah).
This is a follow-up patch of recent resctrl selftest patches and can be
applied cleanly to:
git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
branch next.
tools/testing/selftests/resctrl/cache.c | 2 +-
tools/testing/selftests/resctrl/mba_test.c | 6 +++---
tools/testing/selftests/resctrl/mbm_test.c | 2 +-
tools/testing/selftests/resctrl/resctrlfs.c | 4 ++--
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c
b/tools/testing/selftests/resctrl/cache.c
index 362e3a418caa..68ff856d36f0 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -301,7 +301,7 @@ int show_cache_info(unsigned long sum_llc_val, int
no_of_bits,
ret = platform && abs((int)diff_percent) > max_diff_percent &&
(cmt ? (abs(avg_diff) > max_diff) : true);
- ksft_print_msg("%s cache miss rate within %d%%\n",
+ ksft_print_msg("%s Check cache miss rate within %d%%\n",
ret ? "Fail:" : "Pass:", max_diff_percent);
ksft_print_msg("Percent diff=%d\n", abs((int)diff_percent));
diff --git a/tools/testing/selftests/resctrl/mba_test.c
b/tools/testing/selftests/resctrl/mba_test.c
index 26f12ad4c663..1a1bdb6180cf 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -80,7 +80,7 @@ static void show_mba_info(unsigned long *bw_imc, unsigned
long *bw_resc)
avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
avg_diff_per = (int)(avg_diff * 100);
- ksft_print_msg("%s MBA: diff within %d%% for schemata %u\n",
+ ksft_print_msg("%s Check MBA diff within %d%% for schemata
%u\n",
avg_diff_per > MAX_DIFF_PERCENT ?
"Fail:" : "Pass:",
MAX_DIFF_PERCENT,
@@ -93,10 +93,10 @@ static void show_mba_info(unsigned long *bw_imc, unsigned
long *bw_resc)
failed = true;
}
- ksft_print_msg("%s schemata change using MBA\n",
+ ksft_print_msg("%s Check schemata change using MBA\n",
failed ? "Fail:" : "Pass:");
if (failed)
- ksft_print_msg("At least one test failed");
+ ksft_print_msg("At least one test failed\n");
}
static int check_results(void)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c
b/tools/testing/selftests/resctrl/mbm_test.c
index 02b1ed03f1e5..8392e5c55ed0 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -37,7 +37,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc,
int span)
avg_diff_per = (int)(avg_diff * 100);
ret = avg_diff_per > MAX_DIFF_PERCENT;
- ksft_print_msg("%s MBM: diff within %d%%\n",
+ ksft_print_msg("%s Check MBM diff within %d%%\n",
ret ? "Fail:" : "Pass:", MAX_DIFF_PERCENT);
ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
ksft_print_msg("Span (MB): %d\n", span);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
b/tools/testing/selftests/resctrl/resctrlfs.c
index ade5f2b8b843..5f5a166ade60 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -570,14 +570,14 @@ bool check_resctrlfs_support(void)
fclose(inf);
- ksft_print_msg("%s kernel supports resctrl filesystem\n",
+ ksft_print_msg("%s Check kernel supports resctrl filesystem\n",
ret ? "Pass:" : "Fail:");
if (!ret)
return ret;
dp = opendir(RESCTRL_PATH);
- ksft_print_msg("%s resctrl mountpoint \"%s\" exists\n",
+ ksft_print_msg("%s Check resctrl mountpoint \"%s\" exists\n",
dp ? "Pass:" : "Fail:", RESCTRL_PATH);
if (dp)
closedir(dp);
Thank you. Applied to linux-kseftest next branch for 5.13-rc1
thanks,
-- Shuah
Re: [PATCH] Documentation: kunit: add tips for using current->kunit_test
On 4/7/21 2:07 PM, Brendan Higgins wrote:
On Tue, Apr 6, 2021 at 3:51 PM Daniel Latypov wrote:
As of commit 359a376081d4 ("kunit: support failure from dynamic analysis
tools"), we can use current->kunit_test to find the current kunit test.
Mention this in tips.rst and give an example of how this can be used in
conjunction with `test->priv` to pass around state and specifically
implement something like mocking.
There's a lot more we could go into on that topic, but given that
example is already longer than every other "tip" on this page, we just
point to the API docs and leave filling in the blanks as an exercise to
the reader.
Also give an example of kunit_fail_current_test().
Signed-off-by: Daniel Latypov
Reviewed-by: Brendan Higgins
Thank you. Applied to linux-kseftest kunit branch for 5.13-rc1
thanks,
-- Shuah
Re: [PATCH] selftests/resctrl: Change a few printed messages
On 4/7/21 11:12 AM, Fenghua Yu wrote:
Hi, Shuah,
On Wed, Apr 07, 2021 at 08:33:23AM -0600, Shuah Khan wrote:
On 4/5/21 6:52 PM, Fenghua Yu wrote:
- ksft_print_msg("%s cache miss rate within %d%%\n",
- ret ? "Fail:" : "Pass:", max_diff_percent);
+ ksft_print_msg("Check cache miss rate within %d%%\n", max_diff_percent);
You need %s and pass in the ret ? "Fail:" : "Pass:" result for the
message to read correctly.
Should I keep the ":" after "Pass"/"Fail"?
Yes please.
I am seeing:
# Check kernel support for resctrl filesystem
It should say the following:
# Fail Check kernel support for resctrl filesystem
i.e. should the printed messages be like the following?
# Fail: Check kernel support for resctrl filesystem
or
# Pass: Check kernel support for resctrl filesystem
This looks good.
thanks,
-- Shuah
Re: [PATCH] selftests/resctrl: Change a few printed messages
On 4/5/21 6:52 PM, Fenghua Yu wrote:
A few printed messages contain pass/fail strings which should be shown
in test results. Remove the pass/fail strings in the messages to avoid
confusion.
Add "\n" at the end of one printed message.
Suggested-by: Shuah Khan
Signed-off-by: Fenghua Yu
---
This is a follow-up patch of recent resctrl selftest patches and can be
applied cleanly to:
git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
branch next.
tools/testing/selftests/resctrl/cache.c | 3 +--
tools/testing/selftests/resctrl/mba_test.c | 9 +++--
tools/testing/selftests/resctrl/mbm_test.c | 3 +--
tools/testing/selftests/resctrl/resctrlfs.c | 7 ++-
4 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c
b/tools/testing/selftests/resctrl/cache.c
index 362e3a418caa..310bbc997c60 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -301,8 +301,7 @@ int show_cache_info(unsigned long sum_llc_val, int
no_of_bits,
ret = platform && abs((int)diff_percent) > max_diff_percent &&
(cmt ? (abs(avg_diff) > max_diff) : true);
- ksft_print_msg("%s cache miss rate within %d%%\n",
- ret ? "Fail:" : "Pass:", max_diff_percent);
+ ksft_print_msg("Check cache miss rate within %d%%\n", max_diff_percent);
You need %s and pass in the ret ? "Fail:" : "Pass:" result for the
message to read correctly.
I am seeing:
# Check kernel support for resctrl filesystem
It should say the following:
# Fail Check kernel support for resctrl filesystem
Same for other such messages.
ksft_print_msg("Percent diff=%d\n", abs((int)diff_percent));
ksft_print_msg("Number of bits: %d\n", no_of_bits);
diff --git a/tools/testing/selftests/resctrl/mba_test.c
b/tools/testing/selftests/resctrl/mba_test.c
index 26f12ad4c663..a909a745754f 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -80,9 +80,7 @@ static void show_mba_info(unsigned long *bw_imc, unsigned
long *bw_resc)
avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
avg_diff_per = (int)(avg_diff * 100);
- ksft_print_msg("%s MBA: diff within %d%% for schemata %u\n",
- avg_diff_per > MAX_DIFF_PERCENT ?
- "Fail:" : "Pass:",
+ ksft_print_msg("Check MBA diff within %d%% for schemata %u\n",
MAX_DIFF_PERCENT,
ALLOCATION_MAX - ALLOCATION_STEP * allocation);
@@ -93,10 +91,9 @@ static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
failed = true;
}
- ksft_print_msg("%s schemata change using MBA\n",
- failed ? "Fail:" : "Pass:");
+ ksft_print_msg("Check schemata change using MBA\n");
if (failed)
- ksft_print_msg("At least one test failed");
+ ksft_print_msg("At least one test failed\n");
}
static int check_results(void)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c
b/tools/testing/selftests/resctrl/mbm_test.c
index 02b1ed03f1e5..e2e7ee4ec630 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -37,8 +37,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc,
int span)
avg_diff_per = (int)(avg_diff * 100);
ret = avg_diff_per > MAX_DIFF_PERCENT;
- ksft_print_msg("%s MBM: diff within %d%%\n",
- ret ? "Fail:" : "Pass:", MAX_DIFF_PERCENT);
+ ksft_print_msg("Check MBM diff within %d%%\n", MAX_DIFF_PERCENT);
Here
ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
ksft_print_msg("Span (MB): %d\n", span);
ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc);
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
b/tools/testing/selftests/resctrl/resctrlfs.c
index ade5f2b8b843..91cb3c48a7da 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -570,15 +570,12 @@ bool check_resctrlfs_support(void)
fclose(inf);
- ksft_print_msg("%s kernel supports resctrl filesystem\n",
- ret ? "Pass:" : "Fail:");
-
+ ksft_print_msg("Check kernel support for resctrl filesystem\n");
Here
if (!ret)
return ret;
dp = opendir(RESCTRL_PATH);
- ksft_print_msg("%s resctrl mountpoint \"%s\" exists\n",
- dp ? "Pass:" : "Fail:", RESCTRL_PATH);
+ ksft_print_msg("Check resctrl mountpoint \"%s\"\n", RESCTRL_PATH);
Here
if (dp)
closedir(dp);
thanks,
-- Shuah
[PATCH] ath10k: Fix ath10k_wmi_tlv_op_pull_peer_stats_info() unlock without lock
ath10k_wmi_tlv_op_pull_peer_stats_info() could try to unlock RCU lock
winthout locking it first when peer reason doesn't match the valid
cases for this function.
Add a default case to return without unlocking.
Fixes: 09078368d516 ("ath10k: hold RCU lock when calling
ieee80211_find_sta_by_ifaddr()")
Reported-by: Pavel Machek
Signed-off-by: Shuah Khan
---
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index d97b33f789e4..7efbe03fbca8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -592,6 +592,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
GFP_ATOMIC
);
break;
+ default:
+ kfree(tb);
+ return;
}
exit:
--
2.27.0
Re: [PATCH] kunit: fix -Wunused-function warning for __kunit_fail_current_test
On 4/6/21 2:50 PM, Brendan Higgins wrote:
On Tue, Apr 6, 2021 at 10:29 AM Daniel Latypov wrote:
When CONFIG_KUNIT is not enabled, __kunit_fail_current_test() an empty
static function.
But GCC complains about unused static functions, *unless* they're static inline.
So add inline to make GCC happy.
Signed-off-by: Daniel Latypov
Fixes: 359a376081d4 ("kunit: support failure from dynamic analysis tools")
Signed-off-by comes after Fixes. Also good to add Reported-by for
Stephen acknowledging the reporter.
I will fix this up when I apply - for future reference.
Reviewed-by: Brendan Higgins
thanks,
-- Shuah
Re: [PATCH 4.4 00/28] 4.4.265-rc1 review
On 4/5/21 2:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.4.265 release. There are 28 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.4.265-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.9 00/35] 4.9.265-rc1 review
On 4/5/21 2:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.9.265 release. There are 35 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.9.265-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.9.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.4 00/74] 5.4.110-rc1 review
On 4/5/21 2:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.4.110 release. There are 74 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.4.110-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.4.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 4.19 00/56] 4.19.185-rc1 review
On 4/5/21 2:53 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 4.19.185 release. There are 56 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v4.x/stable-review/patch-4.19.185-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-4.19.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.10 047/126] ath10k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()
On 4/5/21 9:34 AM, Pavel Machek wrote:
Hi!
Fix ath10k_wmi_tlv_op_pull_peer_stats_info() to hold RCU lock before it
calls ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.
It does that. But is also does the unlock even if it did not take the
lock:
There is only one path after it takes the lock.
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -576,13 +576,13 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
case WMI_TDLS_TEARDOWN_REASON_TX:
case WMI_TDLS_TEARDOWN_REASON_RSSI:
case WMI_TDLS_TEARDOWN_REASON_PTR_TIMEOUT:
+ rcu_read_lock();
Takes the lock here:
station = ieee80211_find_sta_by_ifaddr(ar->hw,
ev->peer_macaddr.addr,
NULL);
if (!station) {
ath10k_warn(ar, "did not find station from tdls peer
event");
- kfree(tb);
- return;
+ goto exit;
Releases it if something fails
}
arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
ieee80211_tdls_oper_request(
@@ -593,6 +593,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
);
break;
}
+
falls through here.
+exit:
+ rcu_read_unlock();
kfree(tb);
}
The switch only takes the lock in 3 branches, but it is released
unconditionally at the end.
I don't see any other switch cases. However, default is missing:
It could be done this way perhaps: (simpler than what you proposed)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index d97b33f789e4..7efbe03fbca8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -592,6 +592,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k
*ar, struct sk_buff *skb)
GFP_ATOMIC
);
break;
+ default:
+ kfree(tb);
+ return;
}
exit:
thanks,
-- Shuah
Re: [PATCH 5.10 000/126] 5.10.28-rc1 review
On 4/5/21 2:52 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.10.28 release. There are 126 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.10.28-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.10.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH 5.11 000/152] 5.11.12-rc1 review
On 4/5/21 2:52 AM, Greg Kroah-Hartman wrote: This is the start of the stable review cycle for the 5.11.12 release. There are 152 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let me know. Responses should be made by Wed, 07 Apr 2021 08:50:09 +. Anything received after that time might be too late. The whole patch series can be found in one patch at: https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.11.12-rc1.gz or in the git tree and branch at: git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git linux-5.11.y and the diffstat can be found below. thanks, greg k-h Compiled and booted on my test system. No dmesg regressions. Tested-by: Shuah Khan thanks, -- Shuah
Re: [PATCH] firmware_loader: Remove unnecessary conversion to bool
On 2/18/21 2:12 AM, Jiapeng Chong wrote:
Fix the following coccicheck warnings:
./tools/testing/selftests/firmware/fw_namespace.c:98:54-59: WARNING:
conversion to bool not needed here.
Reported-by: Abaci Robot
Signed-off-by: Jiapeng Chong
---
tools/testing/selftests/firmware/fw_namespace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/firmware/fw_namespace.c
b/tools/testing/selftests/firmware/fw_namespace.c
index 5ebc1ae..0e393cb 100644
--- a/tools/testing/selftests/firmware/fw_namespace.c
+++ b/tools/testing/selftests/firmware/fw_namespace.c
@@ -95,7 +95,7 @@ static bool test_fw_in_ns(const char *fw_name, const char
*sys_path, bool block_
}
if (block_fw_in_parent_ns)
umount("/lib/firmware");
- return WEXITSTATUS(status) == EXIT_SUCCESS ? true : false;
This looks right to me. test_fw_in_ns() returns true or false and
test_fw_in_ns() callers print appropriate message.
I don't think this patch is necessary.
thanks,
-- Shuah
Re: [PATCH v4 1/2] kunit: support failure from dynamic analysis tools
On 4/2/21 3:44 PM, Shuah Khan wrote:
On 4/2/21 3:25 PM, Daniel Latypov wrote:
On Fri, Apr 2, 2021 at 10:53 AM Shuah Khan
wrote:
On 4/2/21 2:55 AM, Brendan Higgins wrote:
On Thu, Mar 11, 2021 at 7:23 AM Daniel Latypov
wrote:
From: Uriel Guajardo
Add a kunit_fail_current_test() function to fail the currently running
test, if any, with an error message.
This is largely intended for dynamic analysis tools like UBSAN and for
fakes.
E.g. say I had a fake ops struct for testing and I wanted my `free`
function to complain if it was called with an invalid argument, or
caught a double-free. Most return void and have no normal means of
signalling failure (e.g. super_operations, iommu_ops, etc.).
Key points:
* Always update current->kunit_test so anyone can use it.
* commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated
it for
CONFIG_KASAN=y
* Create a new header so non-test code doesn't have
to include all of (e.g. lib/ubsan.c)
* Forward the file and line number to make it easier to track down
failures
* Declare the helper function for nice __printf() warnings about
mismatched
format strings even when KUnit is not enabled.
Example output from kunit_fail_current_test("message"):
[15:19:34] [FAILED] example_simple_test
[15:19:34] # example_simple_test: initializing
[15:19:34] # example_simple_test:
lib/kunit/kunit-example-test.c:24: message
[15:19:34] not ok 1 - example_simple_test
Co-developed-by: Daniel Latypov
Signed-off-by: Daniel Latypov
Signed-off-by: Uriel Guajardo
Reviewed-by: Alan Maguire
Reviewed-by: Brendan Higgins
Please run checkpatch on your patches in the future. I am seeing
a few checkpatch readability type improvements that can be made.
Please make changes and send v2 with Brendan's Reviewed-by.
Thanks for the catch.
checkpatch.pl --strict should now be happy (aside from complaining
about line wrapping)
v5 here:
https://lore.kernel.org/linux-kselftest/20210402212131.835276-1-dlaty...@google.com
Note: Brendan didn't give an explicit Reviewed-by on the second patch,
not sure if that was intentional.
No worries. I applied this one as well. I was able to fix it with just
checkpatch --fix option.
Clarification. Applied 1/2 - I will wait for Brendan's ack on 2/2
thanks,
-- Shuah
Re: [PATCH v4 1/2] kunit: support failure from dynamic analysis tools
On 4/2/21 3:25 PM, Daniel Latypov wrote:
On Fri, Apr 2, 2021 at 10:53 AM Shuah Khan wrote:
On 4/2/21 2:55 AM, Brendan Higgins wrote:
On Thu, Mar 11, 2021 at 7:23 AM Daniel Latypov wrote:
From: Uriel Guajardo
Add a kunit_fail_current_test() function to fail the currently running
test, if any, with an error message.
This is largely intended for dynamic analysis tools like UBSAN and for
fakes.
E.g. say I had a fake ops struct for testing and I wanted my `free`
function to complain if it was called with an invalid argument, or
caught a double-free. Most return void and have no normal means of
signalling failure (e.g. super_operations, iommu_ops, etc.).
Key points:
* Always update current->kunit_test so anyone can use it.
* commit 83c4e7a0363b ("KUnit: KASAN Integration") only updated it for
CONFIG_KASAN=y
* Create a new header so non-test code doesn't have
to include all of (e.g. lib/ubsan.c)
* Forward the file and line number to make it easier to track down
failures
* Declare the helper function for nice __printf() warnings about mismatched
format strings even when KUnit is not enabled.
Example output from kunit_fail_current_test("message"):
[15:19:34] [FAILED] example_simple_test
[15:19:34] # example_simple_test: initializing
[15:19:34] # example_simple_test: lib/kunit/kunit-example-test.c:24: message
[15:19:34] not ok 1 - example_simple_test
Co-developed-by: Daniel Latypov
Signed-off-by: Daniel Latypov
Signed-off-by: Uriel Guajardo
Reviewed-by: Alan Maguire
Reviewed-by: Brendan Higgins
Please run checkpatch on your patches in the future. I am seeing
a few checkpatch readability type improvements that can be made.
Please make changes and send v2 with Brendan's Reviewed-by.
Thanks for the catch.
checkpatch.pl --strict should now be happy (aside from complaining
about line wrapping)
v5 here:
https://lore.kernel.org/linux-kselftest/20210402212131.835276-1-dlaty...@google.com
Note: Brendan didn't give an explicit Reviewed-by on the second patch,
not sure if that was intentional.
No worries. I applied this one as well. I was able to fix it with just
checkpatch --fix option.
All set now.
thanks,
-- Shuah
Re: [PATCH v6 00/21] Miscellaneous fixes for resctrl selftests
On 4/2/21 12:18 PM, Fenghua Yu wrote: Hi, Shuah, On Fri, Apr 02, 2021 at 12:17:17PM -0600, Shuah Khan wrote: On 3/26/21 1:45 PM, Fenghua Yu wrote: Hi, Shuah, On Wed, Mar 17, 2021 at 02:22:34AM +, Fenghua Yu wrote: This patch set has several miscellaneous fixes to resctrl selftest tool that are easily visible to user. V1 had fixes to CAT test and CMT test but they were dropped in V2 because having them here made the patchset humongous. So, changes to CAT test and CMT test will be posted in another patchset. Change Log: v6: - Add Tested-by: Babu Moger . - Replace "cat" by CAT_STR etc (Babu). - Capitalize the first letter of printed message (Babu). Any comment on this series? Will you push it into linux-kselftest.git? Yes. Will apply for 5.13-rc1 Great! Thank you very much for your help! Done. Now applied to linux-selftest next. https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/ next Ran sanity test and suggested a change in message for 12/21. Please take a look other such messages and improve them as well and send follow-on patches. thanks, -- Shuah
Re: [PATCH v6 12/21] selftests/resctrl: Check for resctrl mount point only if resctrl FS is supported
On 3/16/21 8:22 PM, Fenghua Yu wrote:
check_resctrlfs_support() does the following
1. Checks if the platform supports resctrl file system or not by looking
for resctrl in /proc/filesystems
2. Calls opendir() on default resctrl file system path
(i.e. /sys/fs/resctrl)
3. Checks if resctrl file system is mounted or not by looking at
/proc/mounts
Steps 2 and 3 will fail if the platform does not support resctrl file
system. So, there is no need to check for them if step 1 fails.
Fix this by returning immediately if the platform does not support
resctrl file system.
Tested-by: Babu Moger
Signed-off-by: Fenghua Yu
---
tools/testing/selftests/resctrl/resctrlfs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
b/tools/testing/selftests/resctrl/resctrlfs.c
index 6b22a186790a..87195eb78356 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -570,6 +570,9 @@ bool check_resctrlfs_support(void)
ksft_print_msg("%s kernel supports resctrl filesystem\n",
ret ? "Pass:" : "Fail:");
This message is a bit confusing. Please change this to read
and send a follow-on patch on top of linux-kselftest next
"Check kernel support for resctrl filesystem"
thanks,
-- Shuah
Re: [PATCH] kunit: tool: make --kunitconfig accept dirs, add lib/kunit fragment
On 4/2/21 1:27 PM, Daniel Latypov wrote: On Fri, Apr 2, 2021 at 11:00 AM Shuah Khan wrote: On 4/2/21 3:32 AM, Brendan Higgins wrote: TL;DR $ ./tools/testing/kunit/kunit.py run --kunitconfig=lib/kunit Per suggestion from Ted [1], we can reduce the amount of typing by assuming a convention that these files are named '.kunitconfig'. In the case of [1], we now have $ ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4 Also add in such a fragment for kunit itself so we can give that as an example more close to home (and thus less likely to be accidentally broken). [1] https://lore.kernel.org/linux-ext4/ycnf4yp1db97z...@mit.edu/ Signed-off-by: Daniel Latypov Reviewed-by: Brendan Higgins Should this be captured in documentation. Especially since this is file is .* file. Do you want to include doc in this patch? Might be better that way. It definitely should be documented, yes. The only real example hadn't landed yet when I sent this patch (fs/ext4/.kunitconfig was going in through the ext4 tree), but now it's in linus/master. There's still some uncertainties about what best practices for this feature should be, i.e. * how granular should these be? * how should configs in parent dirs be handled? Should they be supersets of all the subdirs? * E.g. should fs/.kunitconfig be a superset of fs/ext4/.kunitconfig and any other hypothetical subdir configs? * Should we wait on saying "you should do this" until we have "import" statements/other mechanisms to make this less manual? * how should we handle non-UML tests, like the KASAN tests? * ideally, kunit.py run will eventually support running tests on x86 (using qemu) If it's fine with you, I was hoping to come back and add a section to kunit/start.rst when we've had some of those questions more figured out. Sound good. I will apply this patch and you can document later. thanks, -- Shuah
Re: [PATCH] kunit: make KUNIT_EXPECT_STREQ() quote values, don't print literals
On 4/2/21 1:09 PM, Daniel Latypov wrote:
On Fri, Apr 2, 2021 at 10:47 AM Shuah Khan wrote:
On 4/2/21 3:35 AM, Brendan Higgins wrote:
On Fri, Feb 5, 2021 at 2:18 PM Daniel Latypov wrote:
Before:
Expected str == "world", but
str == hello
"world" == world
After:
Expected str == "world", but
str == "hello"
Note: like the literal ellision for integers, this doesn't handle the
case of
KUNIT_EXPECT_STREQ(test, "hello", "world")
since we don't expect it to realistically happen in checked in tests.
(If you really wanted a test to fail, KUNIT_FAIL("msg") exists)
In that case, you'd get:
Expected "hello" == "world", but
Signed-off-by: Daniel Latypov
Reviewed-by: Brendan Higgins
Hi Daniel,
Please run checkpatch on your patches in the future. I am seeing
a few checkpatch readability type improvements that can be made.
Please make changes and send v2 with Brendan's Reviewed-by.
Are there some flags you'd like me to pass to checkpatch?
$ ./scripts/checkpatch.pl --git HEAD
total: 0 errors, 0 warnings, 42 lines checked
My commit script uses --strict which shows readability errors.
Commit f66884e8b831 ("kunit: make KUNIT_EXPECT_STREQ() quote values,
don't print literals") has no obvious style problems and is ready for
submission.
I just rebased onto linus/master again since I know checkpatch.pl's
default behavior had changed recently, but I didn't see any errors
there.
I know this commit made some lines go just over 80 characters, so
$ ./scripts/checkpatch.pl --max-line-length=80 --git HEAD
...
total: 0 errors, 4 warnings, 42 lines checked
Don't worry about line wrap warns. I just ignore them. :)
thanks,
-- Shuah
Re: [PATCH v6 00/21] Miscellaneous fixes for resctrl selftests
On 3/26/21 1:45 PM, Fenghua Yu wrote: Hi, Shuah, On Wed, Mar 17, 2021 at 02:22:34AM +, Fenghua Yu wrote: This patch set has several miscellaneous fixes to resctrl selftest tool that are easily visible to user. V1 had fixes to CAT test and CMT test but they were dropped in V2 because having them here made the patchset humongous. So, changes to CAT test and CMT test will be posted in another patchset. Change Log: v6: - Add Tested-by: Babu Moger . - Replace "cat" by CAT_STR etc (Babu). - Capitalize the first letter of printed message (Babu). Any comment on this series? Will you push it into linux-kselftest.git? Yes. Will apply for 5.13-rc1 thanks, -- Shuah
