On 12.06.2017 14:14, Arkadiusz Hiler wrote:
> On Tue, Jun 06, 2017 at 11:54:14AM +0300, Abdiel Janulgue wrote:
>> v3: Drop redundant test covered by drv_hangman/basic. Descend thru
>> debugfs path when reading sysfs entries (Chris).
>>
>> v2: Use internal igt_debugfs functions instead of cat and document
>> debugfs tests.
>> Convert sysfs_l3_parity properly.
>> Rename redundant names in tests.
>>
>> Converted:
>> - check_drm_clients (ensures no other clients are running.
>> functionality provided by drm_open_driver_master).
>> - debugfs_emon_crash
>> - debugfs_wedged
>> - drv_debugfs_reader
>> - sysfs_l3_parity
>> - test_rte_check (same as check_drm_clients)
>> - tools_test
>> - ZZ_check_dmesg
>
> Hey,
>
> Doing each tool in a separate commit would help with readability.
Will do.
>
>>
>> Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@xxxxxxxxxxxxxxx>
>> ---
>> tests/Makefile.sources | 9 +---
>> tests/ZZ_check_dmesg | 11 -----
>> tests/check_drm_clients | 6 ---
>> tests/debugfs.c | 96 +++++++++++++++++++++++++++++++++++++
>> tests/debugfs_emon_crash | 16 -------
>> tests/debugfs_wedged | 10 ----
>> tests/drv_debugfs_reader | 9 ----
>> tests/sysfs_l3_parity | 22 ---------
>> tests/test_rte_check | 6 ---
>> tests/tools.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++
>> tests/tools_test | 16 -------
>> 11 files changed, 220 insertions(+), 103 deletions(-)
>> delete mode 100755 tests/ZZ_check_dmesg
>> delete mode 100755 tests/check_drm_clients
>> create mode 100644 tests/debugfs.c
>> delete mode 100755 tests/debugfs_emon_crash
>> delete mode 100755 tests/debugfs_wedged
>> delete mode 100755 tests/drv_debugfs_reader
>> delete mode 100755 tests/sysfs_l3_parity
>> delete mode 100755 tests/test_rte_check
>> create mode 100644 tests/tools.c
>> delete mode 100755 tests/tools_test
>>
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index 9553e4d..c4a78a9 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -243,6 +243,8 @@ TESTS_progs = \
>> drv_module_reload \
>> kms_sysfs_edid_timing \
>> perf \
>> + debugfs \
>
> The name clashes on AOSP build:
> build/core/base_rules.mk:238: error: external/igt/tests: MODULE.TARGET.EXECUTABLES.debugfs already defined by external/e2fsprogs/debugfs.
>
> debugfs is a name of a binary that debugs ext{2,3,4} family of FSes.
>
> I have to look into the build system and our Android.mks more deeply as
> IGTs output binaries should be stored elsewhere and the clash is rather
> enigmatic.
>
> Also are we changing names of the test? I thought it was only a rewrite.
It's just a rewrite. Now that we combine those debugfs_* scripts into
one executable, maybe just the prefix would suffice? How about
debugfs_tests.c?
>
>> + tools \
>> $(NULL)
>>
>> # IMPORTANT: The ZZ_ tests need to be run last!
>> @@ -251,11 +253,6 @@ TESTS_scripts_M = \
>> $(NULL)
>>
>> TESTS_scripts = \
>> - debugfs_emon_crash \
>> - drv_debugfs_reader \
>> - sysfs_l3_parity \
>> - test_rte_check \
>> - tools_test \
>> $(NULL)
>
> TESTS_scripts now is only $(NULL), so you may as well just remove it
> from the `kernel_tests` variable and get rid of it completely.
>
>>
>> # This target contains testcases which support automagic subtest enumeration
>> @@ -317,9 +314,7 @@ HANG = \
>> $(NULL)
>>
>> scripts = \
>> - check_drm_clients \
>> ddx_intel_after_fbdev \
>> - debugfs_wedged \
>> drm_lib.sh \
>> drm_getopt.sh \
>> $(NULL)
>
> <SNIP>
>
>> diff --git a/tests/debugfs.c b/tests/debugfs.c
>> new file mode 100644
>> index 0000000..b9ae86c
>> --- /dev/null
>> +++ b/tests/debugfs.c
>> @@ -0,0 +1,96 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +#ifdef HAVE_CONFIG_H
>> +#include "config.h"
>> +#endif
>> +#include "igt.h"
>> +#include "igt_sysfs.h"
>> +#include <fcntl.h>
>> +#include <sys/types.h>
>> +#include <dirent.h>
>> +
>> +static void get_sysfs_entry(int path_fd)
>
> I was a little bit confused by the function - we are not getting
> anything from it. Just reading and discarding.
>
> Maybe read_and_discard_sysfs_entry? A comment would also do.
read_and_discard_sysfs_entry() sounds better.
>
>> +{
>> + struct dirent *dirent;
>> + DIR *dir;
>> +
>> + dir = fdopendir(path_fd);
>> + if (!dir)
>> + return;
>> +
>> + while ((dirent = readdir(dir))) {
>> + if (!strcmp(dirent->d_name, ".") ||
>> + !strcmp(dirent->d_name, ".."))
>> + continue;
>> + if (dirent->d_type == DT_DIR) {
>> + int sub_fd = -1;
>> + igt_assert((sub_fd =
>> + openat(path_fd, dirent->d_name, O_RDONLY |
>> + O_DIRECTORY)) > 0);
>> + get_sysfs_entry(sub_fd);
>> + close(sub_fd);
>> + } else {
>> + char *buf = igt_sysfs_get(path_fd, dirent->d_name);
>
> igt_sysfs_get may get us NULL.
> Shouldn't we assert on that? It's an error-worthy.
Yep.
> <SNIP>
>> + igt_assert(asprintf(&cmd, "dmesg | grep "
>> + "-- '------\\[ cut here \\]----'")
>> + != -1);
>> + igt_assert(igt_system_quiet(cmd) != IGT_EXIT_SUCCESS);
>> + free(cmd);
>
> Are we testing the dmesg here?
>
> If not they why systeming out "dmesg | grep" instead of going through
> /dev/kmsg?
Yes. This could be improved. Thanks for the review!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
