Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4b483680-2532-98e9-71b0-4b2ced508313@gmail.com>
Date:   Sat, 25 Mar 2023 19:56:09 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH 1/2] of: unittest: option to allow tests that trigger
 kernel stack dump
Content-Language: en-US
To:     Rob Herring <robh@kernel.org>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Jonathan Corbet <corbet@lwn.net>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
References: <20230301012116.1488132-1-frowand.list@gmail.com>
 <20230301012116.1488132-2-frowand.list@gmail.com>
 <018f54ee-55cc-e30b-4595-ce555fff1708@roeck-us.net>
 <96cfd1ee-768d-cc03-53dd-35ccf2396863@gmail.com>
 <20230324214341.GA20080-robh@kernel.org>
From:   Frank Rowand <frowand.list@gmail.com>
In-Reply-To: <20230324214341.GA20080-robh@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/24/23 16:43, Rob Herring wrote:
> On Wed, Mar 01, 2023 at 10:01:36AM -0600, Frank Rowand wrote:
>> On 2/28/23 22:07, Guenter Roeck wrote:
>>> On 2/28/23 17:21, Frank Rowand wrote:
>>>> Commit 74df14cd301a ("of: unittest: add node lifecycle tests") added
>>>> some tests that trigger a kernel stack dump.  Filtering the boot
>>>> messages with scripts/dtc/of_unittest_expect detects that the stack
>>>> dump is expected instead of being a test error.
>>>>
>>>> Test beds might interpret the stack dumps as errors, resulting in
>>>> needless debugging and error reports.  These test beds are likely
>>>> to remove unittests due to these stack dumps. To avoid these problems,
>>>> have unittest default to skip the tests that trigger a stack dump.
>>>>
>>>> Add a kernel cmdline option to not skip those tests.  This option can
>>>> be used by testers who are able to interpret the stack dumps as not
>>>> an error.
>>>>
>>>> Signed-off-by: Frank Rowand <frowand.list@gmail.com>
>>>> ---
>>>>   drivers/of/unittest.c | 54 ++++++++++++++++++++++++++++++++++++++++---
>>>>   1 file changed, 51 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>>>> index b5a7a31d8bd2..3a9bc2bc4ba1 100644
>>>> --- a/drivers/of/unittest.c
>>>> +++ b/drivers/of/unittest.c
>>>> @@ -70,6 +70,36 @@ static struct unittest_results {
>>>>   #define EXPECT_NOT_END(level, fmt, ...) \
>>>>       printk(level pr_fmt("EXPECT_NOT / : ") fmt, ##__VA_ARGS__)
>>>>   +/*
>>>> + * Some tests will cause the kernel to emit a stack dump, aka back trace,
>>>> + * when the test is successful.  The tests should make it possible for
>>>> + * test beds to detect that the trace is not an error via EXPECT_BEGIN().
>>>> + *
>>>> + * Most test beds do not process the EXPECT_BEGIN() information and may
>>>> + * flag the stack dump as an error, thus reporting a false failure.  It
>>>> + * is hoped that the KTAP version 4 specification will add the EXPECT_BEGIN()
>>>> + * processing to test beds.
>>>> + *
>>>> + * By default, skip tests that cause a stack dump.  Test beds that process
>>>> + * EXPECT_BEGIN() information should enable these tests via a kernel boot
>>>> + * command line option.
>>>> + */
>>>> +static int stackdump_tests_enabled;
>>>> +
>>>> +static int __init enable_unittest_stackdump(char *str)
>>>> +{
>>>> +    stackdump_tests_enabled = 1;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int __init disable_unittest_stackdump(char *str)
>>>> +{
>>>> +    stackdump_tests_enabled = 0;
>>>> +    return 0;
>>>> +}
>>>> +early_param("of_unittest_stackdump", enable_unittest_stackdump);
>>>> +early_param("no_of_unittest_stackdump", disable_unittest_stackdump);
>>>
>>> Does no_of_unittest_stackdump have any benefit or value ?
>>
>> I would say no, but it is a common pattern to provide both
>> foo and no_foo.
> 
> It is? I see one documented example. I see numerous ones that are 
> 'no_foo'.

I reconsidered.  I plan to remove the no_of_unittest_stackdump in v2, updated
to the current kernel version.

-Frank

> 
> This doesn't scale well if lots of tests need to disable it. Perhaps it 
> should be more generic (at least documentation/naming wise even if the 
> implmentation lives in DT unittest for now).
> 
> Rob