* Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Brendan Higgins @ 2019-06-26 6:41 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Stephen Boyd, Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook,
Kieran Bingham, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190625230253.GQ19023@42.do-not-panic.com>
On Tue, Jun 25, 2019 at 4:02 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Tue, Jun 25, 2019 at 03:14:45PM -0700, Brendan Higgins wrote:
> > On Tue, Jun 25, 2019 at 2:44 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > Since its a new architecture and since you seem to imply most tests
> > > don't require locking or even IRQs disabled, I think its worth to
> > > consider the impact of adding such extreme locking requirements for
> > > an initial ramp up.
> >
> > Fair enough, I can see the point of not wanting to use irq disabled
> > until we get someone complaining about it, but I think making it
> > thread safe is reasonable. It means there is one less thing to confuse
> > a KUnit user and the only penalty paid is some very minor performance.
>
> One reason I'm really excited about kunit is speed... so by all means I
> think we're at a good point to analyze performance optimizationsm if
> they do make sense.
Yeah, but I think there are much lower hanging fruit than this (as you
point out below). I am all for making/keeping KUnit super fast, but I
also don't want to waste time with premature optimizations and I think
having thread safe expectations and non-thread safe expectations hurts
usability.
Still, I am on board with making this a mutex instead of a spinlock for now.
> While on the topic of parallization, what about support for running
> different test cases in parallel? Or at the very least different kunit
> modules in parallel. Few questions come up based on this prospect:
>
> * Why not support parallelism from the start?
Just because it is more work and there isn't much to gain from it right now.
Some numbers:
I currently have a collection of 86 test cases in the branch that this
patchset is from. I turned on PRINTK_TIME and looked at the first
KUnit output and the last. On UML, start time was 0.090000, and end
time was 0.090000. Looks like sched_clock is not very good on UML.
Still it seems quite likely that all of these tests run around 0.01
seconds or less on UML: I ran KUnit with only 2 test cases enabled
three times and got an average runtime of 1.55867 seconds with a
standard deviation of 0.0346747. I then ran it another three times
with all test cases enabled and got an average runtime of 1.535
seconds with a standard deviation of 0.0150997. The second average is
less, but that doesn't really mean anything because it is well within
one standard deviation with a very small sample size. Nevertheless, we
can conclude that the actual runtime of those 84 test cases is most
likely within one standard deviation, so on the order of 0.01 seconds.
On x86 running on QEMU, first message from KUnit was printed at
0.194251 and the last KUnit message was printed at 0.340915, meaning
that all 86 test cases ran in about 0.146664 seconds.
In any case, running KUnit tests in parallel is definitely something I
plan on adding it eventually, but it just doesn't really seem worth it
right now. I find the incremental build time of the kernel to
typically be between 3 and 30 seconds, and a clean build to be between
30 seconds to several minutes, depending on the number of available
cores, so I don't think most users would even notice the amount of
runtime contributed by the actual unit tests until we start getting
into the 1000s of test cases. I don't suspect it will become an issue
until we get into the 10,000s of test cases. I think we are a pretty
long way off from that.
> * Are you opposed to eventually having this added? For instance, there is
> enough code on lib/test_kmod.c for batching tons of kthreads each
> one running its own thing for testing purposes which could be used
> as template.
I am not opposed to adding it eventually at all. I actually plan on
doing so, just not in this patchset. There are a lot of additional
features, improvements, and sugar that I really want to add, so much
so that most of it doesn't belong in this patchset; I just think this
is one of those things that belongs in a follow up. I tried to boil
down this patchset to as small as I could while still being useful;
this is basically an MVP. Maybe after this patchset gets merged I
should post a list of things I have ready for review, or would like to
work on, and people can comment on what things they want to see next.
> * If we eventually *did* support it:
> - Would logs be skewed?
Probably, before I went with the TAP approach, I was tagging each
message with the test case it came from and I could have parsed it and
assembled a coherent view of the logs using that; now that I am using
TAP conforming output, that won't work. I haven't really thought too
hard about how to address it, but there are ways. For the UML users, I
am planning on adding a feature to guarantee hermeticity between tests
running in different modules by adding a feature that allows a single
kernel to be built with all tests included, and then determine which
tests get run by passing in command line arguments or something. This
way you can get the isolation from running tests in separate
environments without increasing the build cost. We could also use this
method to achieve parallelism by dispatching multiple kernels at once.
That only works for UML, but I imagine you could do something similar
for users running tests under qemu.
> - Could we have a way to query: give me log for only kunit module
> named "foo"?
Yeah, I think that would make sense as part of the hermeticity thing I
mentioned above.
Hope that seems reasonable!
^ permalink raw reply
* Re: [PATCH] docs: zh_CN: submitting-drivers.rst: Remove a duplicated Documentation/
From: Alex Shi @ 2019-06-26 3:45 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, linux-kernel, Jonathan Corbet, Harry Wei
In-Reply-To: <47f81418930438d1deab8da1307bcd89ba9afd91.1561225663.git.mchehab+samsung@kernel.org>
Thanks for catching.
Reviewed-by: Alex Shi
在 2019/6/23 上午1:47, Mauro Carvalho Chehab 写道:
> Somehow, this file ended with Documentation/ twice.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> Documentation/translations/zh_CN/process/submitting-drivers.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/translations/zh_CN/process/submitting-drivers.rst b/Documentation/translations/zh_CN/process/submitting-drivers.rst
> index 72c6cd935821..72f4f45c98de 100644
> --- a/Documentation/translations/zh_CN/process/submitting-drivers.rst
> +++ b/Documentation/translations/zh_CN/process/submitting-drivers.rst
> @@ -22,7 +22,7 @@
> 兴趣的是显卡驱动程序，你也许应该访问 XFree86 项目(http://www.xfree86.org/)
> 和／或 X.org 项目 (http://x.org)。
>
> -另请参阅 Documentation/Documentation/translations/zh_CN/process/submitting-patches.rst 文档。
> +另请参阅 Documentation/translations/zh_CN/process/submitting-patches.rst 文档。
>
>
> 分配设备号
^ permalink raw reply
* Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Stephen Boyd @ 2019-06-26 3:40 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g46Jhxsz6_VXHEVYvTeDRwwzgKpr=aUWLL5b3S4kUukb8g@mail.gmail.com>
Quoting Brendan Higgins (2019-06-25 13:28:25)
> On Wed, Jun 19, 2019 at 5:15 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-06-17 01:25:56)
> > > diff --git a/kunit/test.c b/kunit/test.c
> > > new file mode 100644
> > > index 0000000000000..d05d254f1521f
> > > --- /dev/null
> > > +++ b/kunit/test.c
> > > @@ -0,0 +1,210 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Base unit test (KUnit) API.
> > > + *
> > > + * Copyright (C) 2019, Google LLC.
> > > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > > + */
> > > +
> > > +#include <linux/sched/debug.h>
> > > +#include <kunit/test.h>
> > > +
> > > +static bool kunit_get_success(struct kunit *test)
> > > +{
> > > + unsigned long flags;
> > > + bool success;
> > > +
> > > + spin_lock_irqsave(&test->lock, flags);
> > > + success = test->success;
> > > + spin_unlock_irqrestore(&test->lock, flags);
> >
> > I still don't understand the locking scheme in this code. Is the
> > intention to make getter and setter APIs that are "safe" by adding in a
> > spinlock that is held around getting and setting various members in the
> > kunit structure?
>
> Yes, your understanding is correct. It is possible for a user to write
> a test such that certain elements may be updated in different threads;
> this would most likely happen in the case where someone wants to make
> an assertion or an expectation in a thread created by a piece of code
> under test. Although this should generally be avoided, it is possible,
> and there are occasionally good reasons to do so, so it is
> functionality that we should support.
>
> Do you think I should add a comment to this effect?
No, I think the locking should be removed.
>
> > In what situation is there more than one thread reading or writing the
> > kunit struct? Isn't it only a single process that is going to be
>
> As I said above, it is possible that the code under test may spawn a
> new thread that may make an expectation or an assertion. It is not a
> super common use case, but it is possible.
Sure, sounds super possible and OK.
>
> > operating on this structure? And why do we need to disable irqs? Are we
> > expecting to be modifying the unit tests from irq contexts?
>
> There are instances where someone may want to test a driver which has
> an interrupt handler in it. I actually have (not the greatest) example
> here. Now in these cases, I expect someone to use a mock irqchip or
> some other fake mechanism to trigger the interrupt handler and not
> actual hardware; technically speaking in this case, it is not going to
> be accessed from a "real" irq context; however, the code under test
> should think that it is in an irq context; given that, I figured it is
> best to just treat it as a real irq context. Does that make sense?
Can you please describe the scenario in which grabbing the lock here,
updating a single variable, and then releasing the lock right after
does anything useful vs. not having the lock? I'm looking for a two CPU
scenario like below, but where it is a problem. There could be three
CPUs, or even one CPU and three threads if you want to describe the
extra thread scenario.
Here's my scenario where it isn't needed:
CPU0 CPU1
---- ----
kunit_run_test(&test)
test_case_func()
....
[mock hardirq]
kunit_set_success(&test)
[hardirq ends]
...
complete(&test_done)
wait_for_completion(&test_done)
kunit_get_success(&test)
We don't need to care about having locking here because success or
failure only happens in one place and it's synchronized with the
completion.
>
> > > +
> > > + return success;
> > > +}
> > > +
> > > +static void kunit_set_success(struct kunit *test, bool success)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(&test->lock, flags);
> > > + test->success = success;
> > > + spin_unlock_irqrestore(&test->lock, flags);
> > > +}
^ permalink raw reply
* Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Luis Chamberlain @ 2019-06-26 3:36 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Peter Zijlstra, Rob Herring, Stephen Boyd, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g46TLAONgXiZkFM98BPd-sariMTwhmYG9hSJ+M9=r-ixeg@mail.gmail.com>
On Tue, Jun 25, 2019 at 05:07:32PM -0700, Brendan Higgins wrote:
> On Tue, Jun 25, 2019 at 3:33 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Mon, Jun 17, 2019 at 01:25:56AM -0700, Brendan Higgins wrote:
> > > +/**
> > > + * module_test() - used to register a &struct kunit_module with KUnit.
> > > + * @module: a statically allocated &struct kunit_module.
> > > + *
> > > + * Registers @module with the test framework. See &struct kunit_module for more
> > > + * information.
> > > + */
> > > +#define module_test(module) \
> > > + static int module_kunit_init##module(void) \
> > > + { \
> > > + return kunit_run_tests(&module); \
> > > + } \
> > > + late_initcall(module_kunit_init##module)
> >
> > Becuase late_initcall() is used, if these modules are built-in, this
> > would preclude the ability to test things prior to this part of the
> > kernel under UML or whatever architecture runs the tests. So, this
> > limits the scope of testing. Small detail but the scope whould be
> > documented.
>
> You aren't the first person to complain about this (and I am not sure
> it is the first time you have complained about it). Anyway, I have
> some follow on patches that will improve the late_initcall thing, and
> people seemed okay with discussing the follow on patches as part of a
> subsequent patchset after this gets merged.
>
> I will nevertheless document the restriction until then.
To be clear, I am not complaining about it. I just find it simply
critical to document its limitations, so folks don't try to invest
time and energy on kunit right away for an early init test, if it
cannot support it.
If support for that requires some work, it may be worth mentioning
as well.
> > > +static void kunit_print_tap_version(void)
> > > +{
> > > + if (!kunit_has_printed_tap_version) {
> > > + kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
> >
> > What is this TAP thing? Why should we care what version it is on?
> > Why are we printing this?
>
> It's part of the TAP specification[1]. Greg and Frank asked me to make
> the intermediate format conform to TAP. Seems like something else I
> should probable document...
Yes thanks!
> > > + kunit_has_printed_tap_version = true;
> > > + }
> > > +}
> > > +
> > > +static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> > > +{
> > > + struct kunit_case *test_case;
> > > + size_t len = 0;
> > > +
> > > + for (test_case = test_cases; test_case->run_case; test_case++)
> >
> > If we make the last test case NULL, we'd just check for test_case here,
> > and save ourselves an extra few bytes per test module. Any reason why
> > the last test case cannot be NULL?
>
> Is there anyway to make that work with a statically defined array?
No you're right.
> Basically, I want to be able to do something like:
>
> static struct kunit_case example_test_cases[] = {
> KUNIT_CASE(example_simple_test),
> KUNIT_CASE(example_mock_test),
> {}
> };
>
> FYI,
> #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
>
> In order to do what you are proposing, I think I need an array of
> pointers to test cases, which is not ideal.
Yeah, you're right. The only other alternative is to have a:
struct kunit_module {
const char name[256];
int (*init)(struct kunit *test);
void (*exit)(struct kunit *test);
struct kunit_case *test_cases;
+ unsigned int num_cases;
};
And then something like:
#define KUNIT_MODULE(name, init, exit, cases) { \
.name = name, \
.init = init, \
.exit = exit, \
.test_cases = cases,
num_cases = ARRAY_SIZE(cases), \
}
Let's evaluate which is better: one extra test case per all test cases, or
an extra unsigned int for each kunit module.
Luis
^ permalink raw reply
* Re: [PATCH] bcache: make stripe_size configurable and persistent for hardware raid5/6
From: Martin K. Petersen @ 2019-06-26 2:50 UTC (permalink / raw)
To: Eric Wheeler
Cc: Martin K. Petersen, Coly Li, linux-block, Jonathan Corbet,
Kent Overstreet, open list:DOCUMENTATION, open list,
open list:BCACHE (BLOCK LAYER CACHE)
In-Reply-To: <alpine.LRH.2.11.1906260005570.1114@mx.ewheeler.net>
Eric,
> * LSI 2108 (Supermicro)
> * LSI 3108 (Dell)
> * Areca 1882
> * Areca 1883
> * Fibrechannel 8gbe connected to a Storwize 3700
I have a 3108 that provides the BL VPD. Surprised the 1883 doesn't.
As a rule of thumb, you need 12 Gbps SAS or 16 Gbps FC devices for the
VPD page to be present. The protocol feature is not tied to the
transport signaling speed in any way. But general support for the BL VPD
page roughly coincided with vendors introducing 12 Gbps SAS and 16 Gbps
FC products to the market.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH v5 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework
From: Luis Chamberlain @ 2019-06-26 2:38 UTC (permalink / raw)
To: Frank Rowand
Cc: Brendan Higgins, gregkh, jpoimboe, keescook, kieran.bingham,
peterz, robh, sboyd, shuah, tytso, yamada.masahiro, devicetree,
dri-devel, kunit-dev, linux-doc, linux-fsdevel, linux-kbuild,
linux-kernel, linux-kselftest, linux-nvdimm, linux-um,
Alexander.Levin, Tim.Bird, amir73il, dan.carpenter, daniel, jdike,
joel, julia.lawall, khilman, knut.omang, logang, mpe, pmladek,
rdunlap, richard, rientjes, rostedt, wfg
In-Reply-To: <10feac3e-7621-65e5-fbf0-9c63fcbe09c9@gmail.com>
On Wed, Jun 19, 2019 at 06:17:51PM -0700, Frank Rowand wrote:
> It does not matter whether KUnit provides additional things, relative
> to kselftest. The point I was making is that there appears to be
> _some_ overlap between kselftest and KUnit, and if there is overlap
> then it is worth considering whether the overlap can be unified instead
> of duplicated.
From my experience as an author of several kselftests drivers, one
faily complex, and after reviewing the sysctl kunit test module, I
disagree with this.
Even if there were an overlap, I'd say let the best test harness win.
Just as we have different LSMs that do something similar.
But this is not about that though. Although both are testing code,
they do so in *very* different ways.
The design philosophy and architecture are fundamentally different. The
*only* thing I can think of where there is overlap is that both can test
similar code paths. Beyond that, the layout of how one itemizes tests
could be borrowed, but that would be up to each kselftest author to
decide, in fact some ksefltests do exist which follow similar pattern of
itemizing test cases and running them. Kunit just provides a proper
framework to do this easily but also with a focus on UML. This last
aspect makes kselftests fundamentally orthogonal from an architecture /
design perspective.
After careful review, I cannot personally identify what could be shared
at this point. Can you? If you did identify one part, how do you
recommend to share it?
Luis
^ permalink raw reply
* Re: [PATCH v5 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC SYSCTL section
From: Luis Chamberlain @ 2019-06-26 2:19 UTC (permalink / raw)
To: Brendan Higgins
Cc: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, peterz,
robh, sboyd, shuah, tytso, yamada.masahiro, devicetree, dri-devel,
kunit-dev, linux-doc, linux-fsdevel, linux-kbuild, linux-kernel,
linux-kselftest, linux-nvdimm, linux-um, Alexander.Levin,
Tim.Bird, amir73il, dan.carpenter, daniel, jdike, joel,
julia.lawall, khilman, knut.omang, logang, mpe, pmladek, rdunlap,
richard, rientjes, rostedt, wfg
In-Reply-To: <20190617082613.109131-19-brendanhiggins@google.com>
On Mon, Jun 17, 2019 at 01:26:13AM -0700, Brendan Higgins wrote:
> Add entry for the new proc sysctl KUnit test to the PROC SYSCTL section.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Luis
^ permalink raw reply
* Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
From: Luis Chamberlain @ 2019-06-26 2:17 UTC (permalink / raw)
To: Brendan Higgins
Cc: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, peterz,
robh, sboyd, shuah, tytso, yamada.masahiro, devicetree, dri-devel,
kunit-dev, linux-doc, linux-fsdevel, linux-kbuild, linux-kernel,
linux-kselftest, linux-nvdimm, linux-um, Alexander.Levin,
Tim.Bird, amir73il, dan.carpenter, daniel, jdike, joel,
julia.lawall, khilman, knut.omang, logang, mpe, pmladek, rdunlap,
richard, rientjes, rostedt, wfg, Iurii Zaikin
In-Reply-To: <20190617082613.109131-18-brendanhiggins@google.com>
On Mon, Jun 17, 2019 at 01:26:12AM -0700, Brendan Higgins wrote:
> From: Iurii Zaikin <yzaikin@google.com>
>
> KUnit tests for initialized data behavior of proc_dointvec that is
> explicitly checked in the code. Includes basic parsing tests including
> int min/max overflow.
First, thanks for this work! My review below.
>
> Signed-off-by: Iurii Zaikin <yzaikin@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
> Changes Since Last Revision:
> - Iurii did some clean up (thanks Iurii!) as suggested by Stephen Boyd.
> ---
> kernel/Makefile | 2 +
> kernel/sysctl-test.c | 242 +++++++++++++++++++++++++++++++++++++++++++
> lib/Kconfig.debug | 10 ++
> 3 files changed, 254 insertions(+)
> create mode 100644 kernel/sysctl-test.c
>
> diff --git a/kernel/Makefile b/kernel/Makefile
> index a8d923b5481ba..50fd511cd0ee0 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -114,6 +114,8 @@ obj-$(CONFIG_HAS_IOMEM) += iomem.o
> obj-$(CONFIG_ZONE_DEVICE) += memremap.o
> obj-$(CONFIG_RSEQ) += rseq.o
>
> +obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o
And we have lib/test_sysctl.c of selftests.
I'm fine with this going in as-is to its current place, but if we have
to learn from selftests I'd say we try to stick to a convention so
folks know what framework a test is for, and to ensure folks can
easily tell if its test code or not.
Perhaps simply a directory for kunit tests would suffice alone.
> +
> obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += stackleak.o
> KASAN_SANITIZE_stackleak.o := n
> KCOV_INSTRUMENT_stackleak.o := n
> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> new file mode 100644
> index 0000000000000..cb61ad3c7db63
> --- /dev/null
> +++ b/kernel/sysctl-test.c
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test of proc sysctl.
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/sysctl.h>
> +
> +static int i_zero;
> +static int i_one_hundred = 100;
> +
> +struct test_sysctl_data {
> + int int_0001;
> + int int_0002;
> + int int_0003[4];
> +
> + unsigned int uint_0001;
> +
> + char string_0001[65];
> +};
> +
> +static struct test_sysctl_data test_data = {
> + .int_0001 = 60,
> + .int_0002 = 1,
> +
> + .int_0003[0] = 0,
> + .int_0003[1] = 1,
> + .int_0003[2] = 2,
> + .int_0003[3] = 3,
> +
> + .uint_0001 = 314,
> +
> + .string_0001 = "(none)",
> +};
> +
> +static void sysctl_test_dointvec_null_tbl_data(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = NULL,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + void *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + size_t len;
> + loff_t pos;
> +
> + len = 1234;
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 0, buffer, &len, &pos));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
It is a bit odd, but it does happen, for a developer to be calling
proc_dointvec() directly, instead typically folks just register a table
and let it do its thing. That said, someone not too familiar with proc
code would see this and really have no clue exactly what is being
tested.
Even as a maintainer, I had to read the code for proc_dointvec() a bit
to understand that the above is a *read* attempt to the .data field
being allocated. Because its a write, the len set to a bogus does not
matter as we are expecting the proc_dointvec() to update len for us.
If a test fails, it would be good to for anyone to easily grasp what is
being tested. So... a few words documenting each test case would be nice.
> + len = 1234;
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, buffer, &len, &pos));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
And this is a write...
A nice tests given the data on the table allocated is not assigned.
I don't see any other areas in the kernel where we open code a
proc_dointvec() call where the second argument is a digit, it
always is with a variable. As such there would be no need for
us to expose helpers to make it clear if one is a read or write.
But for *this* case, I think it would be useful to add two wrappers
inside this kunit test module which sprinkles the 0 or 1, this way
a reader can easily know what mode is being tested.
kunit_proc_dointvec_read()
kunit_proc_dointvec_write()
Or just use #define KUNIT_PROC_READ 0, #define KUNIT_PROC_WRITE 1.
Whatever makes this code more legible.
> +}
> +
> +static void sysctl_test_dointvec_table_maxlen_unset(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = 0,
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + void *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + size_t len;
> + loff_t pos;
> +
> + len = 1234;
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 0, buffer, &len, &pos));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> + len = 1234;
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, buffer, &len, &pos));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> +}
In a way this is also testing for general kernel API changes. This is and the
last one were good examples. So this is not just testing functionality
here. There is no wrong or write answer if 0 or -EINVAL was returned
other than the fact that we have been doing this for years.
Its a perhaps small but important difference for some of these tests. I
*do* think its worth clarifying through documentation which ones are
testing for API consistency Vs proper correctness.
> +
> +static void sysctl_test_dointvec_table_len_is_zero(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + void *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + size_t len;
> + loff_t pos;
> +
> + len = 0;
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 0, buffer, &len, &pos));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, buffer, &len, &pos));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> +}
Likewise an API change test.
> +
> +static void sysctl_test_dointvec_table_read_but_position_set(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + void *buffer = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + size_t len;
> + loff_t pos;
> +
> + len = 1234;
> + pos = 1;
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 0, buffer, &len, &pos));
> + KUNIT_EXPECT_EQ(test, (size_t)0, len);
> +}
Likewise an API test.
All the above kunit test cases are currently testing this call on
__do_proc_dointvec():
if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write))
{
*lenp = 0;
return 0;
}
Just an API test.
Perhaps use an api prefix or postfix for these to help distinguish
which are api tests Vs correctness. We want someone who runs into
a failure to *easily* determine *what* went wrong.
Right now this kunit test leaves no leashes around to help the reader.
> +
> +static void sysctl_test_dointvec_happy_single_positive(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + char input[] = "9";
> + size_t len = sizeof(input) - 1;
> + loff_t pos = 0;
> +
> + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, input, &len, &pos));
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, (size_t)pos);
> + KUNIT_EXPECT_EQ(test, 9, ((int *)table.data)[0]);
> +}
Yeap, running these kunit test cases will surely be faster than stupid
shell :) nice!
> +static void sysctl_test_dointvec_happy_single_negative(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + char input[] = "-9";
> + size_t len = sizeof(input) - 1;
> + loff_t pos = 0;
> +
> + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + KUNIT_EXPECT_EQ(test, 0, proc_dointvec(&table, 1, input, &len, &pos));
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, (size_t)pos);
> + KUNIT_EXPECT_EQ(test, -9, ((int *)table.data)[0]);
> +}
> +
> +static void sysctl_test_dointvec_single_less_int_min(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + char input[32];
> + size_t len = sizeof(input) - 1;
> + loff_t pos = 0;
> + unsigned long abs_of_less_than_min = (unsigned long)INT_MAX
> + - (INT_MAX + INT_MIN) + 1;
> +
> + KUNIT_EXPECT_LT(test,
> + (size_t)snprintf(input, sizeof(input), "-%lu",
> + abs_of_less_than_min),
> + sizeof(input));
> +
> + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + KUNIT_EXPECT_EQ(test, -EINVAL,
> + proc_dointvec(&table, 1, input, &len, &pos));
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> + KUNIT_EXPECT_EQ(test, 0, ((int *)table.data)[0]);
> +}
API test.
> +static void sysctl_test_dointvec_single_greater_int_max(struct kunit *test)
> +{
> + struct ctl_table table = {
> + .procname = "foo",
> + .data = &test_data.int_0001,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + .extra1 = &i_zero,
> + .extra2 = &i_one_hundred,
> + };
> + char input[32];
> + size_t len = sizeof(input) - 1;
> + loff_t pos = 0;
> + unsigned long greater_than_max = (unsigned long)INT_MAX + 1;
> +
> + KUNIT_EXPECT_GT(test, greater_than_max, (unsigned long)INT_MAX);
> + KUNIT_EXPECT_LT(test, (size_t)snprintf(input, sizeof(input), "%lu",
> + greater_than_max),
> + sizeof(input));
> + table.data = kunit_kzalloc(test, sizeof(int), GFP_USER);
> + KUNIT_EXPECT_EQ(test, -EINVAL,
> + proc_dointvec(&table, 1, input, &len, &pos));
> + KUNIT_EXPECT_EQ(test, sizeof(input) - 1, len);
> + KUNIT_EXPECT_EQ(test, 0, ((int *)table.data)[0]);
> +}
> +
API test.
> +static struct kunit_case sysctl_test_cases[] = {
> + KUNIT_CASE(sysctl_test_dointvec_null_tbl_data),
> + KUNIT_CASE(sysctl_test_dointvec_table_maxlen_unset),
> + KUNIT_CASE(sysctl_test_dointvec_table_len_is_zero),
> + KUNIT_CASE(sysctl_test_dointvec_table_read_but_position_set),
> + KUNIT_CASE(sysctl_test_dointvec_happy_single_positive),
> + KUNIT_CASE(sysctl_test_dointvec_happy_single_negative),
> + KUNIT_CASE(sysctl_test_dointvec_single_less_int_min),
> + KUNIT_CASE(sysctl_test_dointvec_single_greater_int_max),
> + {}
> +};
Oh all are API tests.. perhaps then just rename then
sysctl_test_cases to sysctl_api_test_cases.
Would be good to add at least *two* other tests cases for this
example, one which does a valid read and one which does a valid write.
If that is done either we add another kunit test module for correctness
or just extend the above and use prefix / postfixes on the functions
to distinguish between API / correctness somehow.
> +
> +static struct kunit_module sysctl_test_module = {
> + .name = "sysctl_test",
> + .test_cases = sysctl_test_cases,
> +};
> +
> +module_test(sysctl_test_module);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index cbdfae3798965..389b8986f5b77 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1939,6 +1939,16 @@ config TEST_SYSCTL
>
> If unsure, say N.
>
> +config SYSCTL_KUNIT_TEST
> + bool "KUnit test for sysctl"
> + depends on KUNIT
> + help
> + This builds the proc sysctl unit test, which runs on boot. For more
> + information on KUnit and unit tests in general please refer to the
> + KUnit documentation in Documentation/dev-tools/kunit/.
A little more description here would help. It is testing for API and
hopefully also correctness (if extended with those two examples I
mentioned).
> +
> + If unsure, say N.
> +
> config TEST_UDELAY
> tristate "udelay test driver"
> help
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
Thanks for the work, it is very much appreciated and gives a clearer
appreciation of value of kunit and what can be done and not. Another
random test idea that comes up, would be to use different memory types
for the table data. In case the kernel API users does something odd,
we should be ensuring we do something proper.
Luis
^ permalink raw reply
* Re: [PATCH] bcache: make stripe_size configurable and persistent for hardware raid5/6
From: Eric Wheeler @ 2019-06-26 0:23 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Coly Li, linux-block, Jonathan Corbet, Kent Overstreet,
open list\:DOCUMENTATION, open list,
open list\:BCACHE \(BLOCK LAYER CACHE\)
In-Reply-To: <yq17e9ao9c3.fsf@oracle.com>
On Mon, 24 Jun 2019, Martin K. Petersen wrote:
> > Perhaps they do not set stripe_width using io_opt? I did a grep to see
> > if any of them did, but I didn't see them. How is stripe_width
> > indicated by RAID controllers?
>
> The values are reported in the Block Limits VPD page for each SCSI block
> device and are thus set by the SCSI disk driver. IOW, the RAID
> controller device drivers have nothing to do with this.
>
> For RAID controllers specifically, the controller firmware will fill out
> the VPD fields for each virtual SCSI disk when you configure a RAID
> set. For pretty much everything else, the Block Limits come straight
> from the device itself.
>
> Also note that these values aren't specific to RAID controllers at
> all. Most new SCSI devices, including disk drives and SSDs, will fill
> out the Block Limits VPD page one way or the other. Even some USB
> storage devices are providing this page.
Thanks, that makes sense. Interesting about USB.
> > If they do set io_opt, then at least my Areca 1883 does not set io_opt
> > as of 4.19.x. I also have a LSI MegaRAID 3108 which does not report
> > io_opt as of 4.1.x, but that is an older kernel so maybe support has
> > been added since then.
>
> I have several MegaRAIDs that all report it. But it depends on the
> controller firmware.
>
> > Is it visible through sysfs or debugfs so I can check my hardware
> > support without hacking debugging the kernel?
>
> To print the block device topology:
>
> # lsblk -t
>
> or look up io_opt in sysfs:
>
> # grep . /sys/block/sdX/queue/optimal_io_size
>
> You can also query a SCSI device's Block Limits directly:
>
> # sg_vpd -p bl /dev/sdX
Perfect, thank you for that. I've tried the following controllers that I
have access to. One worked (hspa/HP Gen8 Smart Array Controller), but the
others I tried are not providing VPDs:
* LSI 2108 (Supermicro)
* LSI 3108 (Dell)
* Areca 1882
* Areca 1883
* Fibrechannel 8gbe connected to a Storwize 3700
~]# sg_vpd -p bl /dev/sdb
VPD page=0xb0
fetching VPD page failed
> If you want to tinker, you can simulate a SCSI disk with your choice of
> io_opt:
>
> # modprobe scsi_debug opt_blks=N
>
> where N is the number of logical blocks to report as being the optimal
> I/O size.
Neat, thanks for the hint!
-Eric
>
> --
> Martin K. Petersen Oracle Linux Engineering
>
^ permalink raw reply
* Re: [PATCH v2 2/2] crypto: doc - Fix formatting of new crypto engine content
From: Joe Perches @ 2019-06-26 0:13 UTC (permalink / raw)
To: Hook, Gary, herbert@gondor.apana.org.au, corbet@lwn.net,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-crypto@vger.kernel.org, davem@davemloft.net
In-Reply-To: <156150622886.22527.934327975584441429.stgit@taos>
On Tue, 2019-06-25 at 23:43 +0000, Hook, Gary wrote:
> Tidy up the formatting/grammar in crypto_engine.rst. Use bulleted lists
> where appropriate.
Hi again Gary.
> diff --git a/Documentation/crypto/crypto_engine.rst b/Documentation/crypto/crypto_engine.rst
[]
> +Before transferring any request, you have to fill the context enginectx by
> +providing functions for the following:
> +
> +* ``prepare_crypt_hardware``: Called once before any prepare functions are
> + called.
> +
> +* ``unprepare_crypt_hardware``: Called once after all unprepare functions have
> + been called.
> +
> +* ``prepare_cipher_request``/``prepare_hash_request``: Called before each
> + corresponding request is performed. If some processing or other preparatory
> + work is required, do it here.
> +
> +* ``unprepare_cipher_request``/``unprepare_hash_request``: Called after each
> + request is handled. Clean up / undo what was done in the prepare function.
> +
> +* ``cipher_one_request``/``hash_one_request``: Handle the current request by
> + performing the operation.
I again suggest not using ``<func>`` but instead use <func>()
and remove unnecessary blank lines.
i.e.:
* prepare_crypt_hardware(): Called once before any prepare functions are
called.
* unprepare_crypt_hardware(): Called once after all unprepare functions
have been called.
* prepare_cipher_request()/prepare_hash_request(): Called before each
corresponding request is performed. If some processing or other preparatory
work is required, do it here.
* unprepare_cipher_request()/unprepare_hash_request(): Called after each
request is handled. Clean up / undo what was done in the prepare function.
* cipher_one_request()/hash_one_request(): Handle the current request by
performing the operation.
[]
> +When your driver receives a crypto_request, you must to transfer it to
> +the crypto engine via one of:
> +
> +* crypto_transfer_ablkcipher_request_to_engine()
And removing the unnecessary blank lines below
> +
> +* crypto_transfer_aead_request_to_engine()
> +
> +* crypto_transfer_akcipher_request_to_engine()
> +
> +* crypto_transfer_hash_request_to_engine()
> +
> +* crypto_transfer_skcipher_request_to_engine()
> +
> +At the end of the request process, a call to one of the following functions is needed:
> +
> +* crypto_finalize_ablkcipher_request()
> +
> +* crypto_finalize_aead_request()
> +
> +* crypto_finalize_akcipher_request()
> +
> +* crypto_finalize_hash_request()
> +
> +* crypto_finalize_skcipher_request()
^ permalink raw reply
* Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Brendan Higgins @ 2019-06-26 0:07 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Peter Zijlstra, Rob Herring, Stephen Boyd, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190625223312.GP19023@42.do-not-panic.com>
On Tue, Jun 25, 2019 at 3:33 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Mon, Jun 17, 2019 at 01:25:56AM -0700, Brendan Higgins wrote:
> > +/**
> > + * module_test() - used to register a &struct kunit_module with KUnit.
> > + * @module: a statically allocated &struct kunit_module.
> > + *
> > + * Registers @module with the test framework. See &struct kunit_module for more
> > + * information.
> > + */
> > +#define module_test(module) \
> > + static int module_kunit_init##module(void) \
> > + { \
> > + return kunit_run_tests(&module); \
> > + } \
> > + late_initcall(module_kunit_init##module)
>
> Becuase late_initcall() is used, if these modules are built-in, this
> would preclude the ability to test things prior to this part of the
> kernel under UML or whatever architecture runs the tests. So, this
> limits the scope of testing. Small detail but the scope whould be
> documented.
You aren't the first person to complain about this (and I am not sure
it is the first time you have complained about it). Anyway, I have
some follow on patches that will improve the late_initcall thing, and
people seemed okay with discussing the follow on patches as part of a
subsequent patchset after this gets merged.
I will nevertheless document the restriction until then.
> > +static void kunit_print_tap_version(void)
> > +{
> > + if (!kunit_has_printed_tap_version) {
> > + kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
>
> What is this TAP thing? Why should we care what version it is on?
> Why are we printing this?
It's part of the TAP specification[1]. Greg and Frank asked me to make
the intermediate format conform to TAP. Seems like something else I
should probable document...
> > + kunit_has_printed_tap_version = true;
> > + }
> > +}
> > +
> > +static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> > +{
> > + struct kunit_case *test_case;
> > + size_t len = 0;
> > +
> > + for (test_case = test_cases; test_case->run_case; test_case++)
>
> If we make the last test case NULL, we'd just check for test_case here,
> and save ourselves an extra few bytes per test module. Any reason why
> the last test case cannot be NULL?
Is there anyway to make that work with a statically defined array?
Basically, I want to be able to do something like:
static struct kunit_case example_test_cases[] = {
KUNIT_CASE(example_simple_test),
KUNIT_CASE(example_mock_test),
{}
};
FYI,
#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
In order to do what you are proposing, I think I need an array of
pointers to test cases, which is not ideal.
> > +void kunit_init_test(struct kunit *test, const char *name)
> > +{
> > + spin_lock_init(&test->lock);
> > + test->name = name;
> > + test->success = true;
> > +}
> > +
> > +/*
> > + * Performs all logic to run a test case.
> > + */
> > +static void kunit_run_case(struct kunit_module *module,
> > + struct kunit_case *test_case)
> > +{
> > + struct kunit test;
> > + int ret = 0;
> > +
> > + kunit_init_test(&test, test_case->name);
> > +
> > + if (module->init) {
> > + ret = module->init(&test);
>
> I believe if we used struct kunit_module *kmodule it would be much
> clearer who's init this is.
That's reasonable. I will fix in next revision.
Cheers!
[1] https://github.com/TestAnything/Specification/blob/tap-14-specification/specification.md
^ permalink raw reply
* Re: [PATCH v5 13/18] kunit: tool: add Python wrappers for running KUnit tests
From: Luis Chamberlain @ 2019-06-26 0:01 UTC (permalink / raw)
To: Brendan Higgins
Cc: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, peterz,
robh, sboyd, shuah, tytso, yamada.masahiro, devicetree, dri-devel,
kunit-dev, linux-doc, linux-fsdevel, linux-kbuild, linux-kernel,
linux-kselftest, linux-nvdimm, linux-um, Alexander.Levin,
Tim.Bird, amir73il, dan.carpenter, daniel, jdike, joel,
julia.lawall, khilman, knut.omang, logang, mpe, pmladek, rdunlap,
richard, rientjes, rostedt, wfg, Felix Guo
In-Reply-To: <20190617082613.109131-14-brendanhiggins@google.com>
On Mon, Jun 17, 2019 at 01:26:08AM -0700, Brendan Higgins wrote:
> create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
> create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-crash.log
> create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-failure.log
> create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-no_tests_run.log
> create mode 100644 tools/testing/kunit/test_data/test_output_isolated_correctly.log
> create mode 100644 tools/testing/kunit/test_data/test_read_from_file.kconfig
Why are these being added upstream? The commit log does not explain
this.
Luis
^ permalink raw reply
* [PATCH v2 2/2] crypto: doc - Fix formatting of new crypto engine content
From: Hook, Gary @ 2019-06-25 23:43 UTC (permalink / raw)
To: herbert@gondor.apana.org.au, corbet@lwn.net,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-crypto@vger.kernel.org, davem@davemloft.net
In-Reply-To: <156150616764.22527.16524544899486041609.stgit@taos>
Tidy up the formatting/grammar in crypto_engine.rst. Use bulleted lists
where appropriate.
Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
Documentation/crypto/crypto_engine.rst | 111 +++++++++++++++++++++-----------
1 file changed, 73 insertions(+), 38 deletions(-)
diff --git a/Documentation/crypto/crypto_engine.rst b/Documentation/crypto/crypto_engine.rst
index 1d56221dfe35..236c674d6897 100644
--- a/Documentation/crypto/crypto_engine.rst
+++ b/Documentation/crypto/crypto_engine.rst
@@ -1,50 +1,85 @@
-=============
-CRYPTO ENGINE
+.. SPDX-License-Identifier: GPL-2.0
+Crypto Engine
=============
Overview
--------
-The crypto engine API (CE), is a crypto queue manager.
+The crypto engine (CE) API is a crypto queue manager.
Requirement
-----------
-You have to put at start of your tfm_ctx the struct crypto_engine_ctx::
+You must put, at the start of your transform context your_tfm_ctx, the structure
+crypto_engine:
+
+::
- struct your_tfm_ctx {
- struct crypto_engine_ctx enginectx;
- ...
- };
+ struct your_tfm_ctx {
+ struct crypto_engine engine;
+ ...
+ };
-Why: Since CE manage only crypto_async_request, it cannot know the underlying
-request_type and so have access only on the TFM.
-So using container_of for accessing __ctx is impossible.
-Furthermore, the crypto engine cannot know the "struct your_tfm_ctx",
-so it must assume that crypto_engine_ctx is at start of it.
+The crypto engine only manages asynchronous requests in the form of
+crypto_async_request. It cannot know the underlying request type and thus only
+has access to the transform structure. It is not possible to access the context
+using container_of. In addition, the engine knows nothing about your
+structure "``struct your_tfm_ctx``". The engine assumes (requires) the placement
+of the known member ``struct crypto_engine`` at the beginning.
Order of operations
-------------------
-You have to obtain a struct crypto_engine via crypto_engine_alloc_init().
-And start it via crypto_engine_start().
-
-Before transferring any request, you have to fill the enginectx.
-- prepare_request: (taking a function pointer) If you need to do some processing before doing the request
-- unprepare_request: (taking a function pointer) Undoing what's done in prepare_request
-- do_one_request: (taking a function pointer) Do encryption for current request
-
-Note: that those three functions get the crypto_async_request associated with the received request.
-So your need to get the original request via container_of(areq, struct yourrequesttype_request, base);
-
-When your driver receive a crypto_request, you have to transfer it to
-the cryptoengine via one of:
-- crypto_transfer_ablkcipher_request_to_engine()
-- crypto_transfer_aead_request_to_engine()
-- crypto_transfer_akcipher_request_to_engine()
-- crypto_transfer_hash_request_to_engine()
-- crypto_transfer_skcipher_request_to_engine()
-
-At the end of the request process, a call to one of the following function is needed:
-- crypto_finalize_ablkcipher_request
-- crypto_finalize_aead_request
-- crypto_finalize_akcipher_request
-- crypto_finalize_hash_request
-- crypto_finalize_skcipher_request
+You are required to obtain a struct crypto_engine via ``crypto_engine_alloc_init()``.
+Start it via ``crypto_engine_start()``. When finished with your work, shut down the
+engine using ``crypto_engine_stop()`` and destroy the engine with
+``crypto_engine_exit()``.
+
+Before transferring any request, you have to fill the context enginectx by
+providing functions for the following:
+
+* ``prepare_crypt_hardware``: Called once before any prepare functions are
+ called.
+
+* ``unprepare_crypt_hardware``: Called once after all unprepare functions have
+ been called.
+
+* ``prepare_cipher_request``/``prepare_hash_request``: Called before each
+ corresponding request is performed. If some processing or other preparatory
+ work is required, do it here.
+
+* ``unprepare_cipher_request``/``unprepare_hash_request``: Called after each
+ request is handled. Clean up / undo what was done in the prepare function.
+
+* ``cipher_one_request``/``hash_one_request``: Handle the current request by
+ performing the operation.
+
+Note that these functions access the crypto_async_request structure
+associated with the received request. You are able to retrieve the original
+request by using:
+
+::
+
+ container_of(areq, struct yourrequesttype_request, base);
+
+When your driver receives a crypto_request, you must to transfer it to
+the crypto engine via one of:
+
+* crypto_transfer_ablkcipher_request_to_engine()
+
+* crypto_transfer_aead_request_to_engine()
+
+* crypto_transfer_akcipher_request_to_engine()
+
+* crypto_transfer_hash_request_to_engine()
+
+* crypto_transfer_skcipher_request_to_engine()
+
+At the end of the request process, a call to one of the following functions is needed:
+
+* crypto_finalize_ablkcipher_request()
+
+* crypto_finalize_aead_request()
+
+* crypto_finalize_akcipher_request()
+
+* crypto_finalize_hash_request()
+
+* crypto_finalize_skcipher_request()
^ permalink raw reply related
* [PATCH v2 1/2] crypto: doc - Add parameter documentation
From: Hook, Gary @ 2019-06-25 23:43 UTC (permalink / raw)
To: herbert@gondor.apana.org.au, corbet@lwn.net,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-crypto@vger.kernel.org, davem@davemloft.net
In-Reply-To: <156150616764.22527.16524544899486041609.stgit@taos>
Fill in missing parameter descriptions for the compression algorithm,
then pick them up to document for the compression_alg structure.
Signed-off-by: Gary R Hook <gary.hook@amd.com>
---
Documentation/crypto/api-skcipher.rst | 2 +-
include/linux/crypto.h | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/Documentation/crypto/api-skcipher.rst b/Documentation/crypto/api-skcipher.rst
index 4eec4a93f7e3..20ba08dddf2e 100644
--- a/Documentation/crypto/api-skcipher.rst
+++ b/Documentation/crypto/api-skcipher.rst
@@ -5,7 +5,7 @@ Block Cipher Algorithm Definitions
:doc: Block Cipher Algorithm Definitions
.. kernel-doc:: include/linux/crypto.h
- :functions: crypto_alg ablkcipher_alg blkcipher_alg cipher_alg
+ :functions: crypto_alg ablkcipher_alg blkcipher_alg cipher_alg compress_alg
Symmetric Key Cipher API
------------------------
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 311237b1dab0..4b4e2ffbee74 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -327,6 +327,17 @@ struct cipher_alg {
void (*cia_decrypt)(struct crypto_tfm *tfm, u8 *dst, const u8 *src);
};
+/**
+ * struct compress_alg - compression/decompression algorithm
+ * @coa_compress: Compress a buffer of specified length, storing the resulting
+ * data in the specified buffer. Return the length of the
+ * compressed data in dlen.
+ * @coa_decompress: Decompress the source buffer, storing the uncompressed
+ * data in the specified buffer. The length of the data is
+ * returned in dlen.
+ *
+ * All fields are mandatory.
+ */
struct compress_alg {
int (*coa_compress)(struct crypto_tfm *tfm, const u8 *src,
unsigned int slen, u8 *dst, unsigned int *dlen);
^ permalink raw reply related
* [PATCH v2 0/2] Clean up crypto documentation
From: Hook, Gary @ 2019-06-25 23:43 UTC (permalink / raw)
To: herbert@gondor.apana.org.au, corbet@lwn.net,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-crypto@vger.kernel.org, davem@davemloft.net
Tidy up the crypto documentation by filling in some variable
descriptions, make some grammatical corrections, and enhance
formatting.
Changes since v1:
- Remove patch with superfluous change to index (patch 2)
- Remove unnecessary markup on function names in patch 3
- Un-add extraneous white space (patch 3)
---
Gary R Hook (2):
crypto: doc - Add parameter documentation
crypto: doc - Fix formatting of new crypto engine content
Documentation/crypto/api-skcipher.rst | 2 -
Documentation/crypto/crypto_engine.rst | 111 +++++++++++++++++++++-----------
include/linux/crypto.h | 11 +++
3 files changed, 85 insertions(+), 39 deletions(-)
--
^ permalink raw reply
* Re: [PATCH] scsi: convert to rst for documenation
From: Douglas Gilbert @ 2019-06-25 23:36 UTC (permalink / raw)
To: Jonathan Corbet, Phong Tran
Cc: skhan, martin.petersen, axboe, avri.altman, beanhuo, evgreen,
henrik, jpittman, linux-doc, linux-kernel, linux-kernel-mentees,
linux-scsi
In-Reply-To: <20190625153658.53ad0e18@lwn.net>
Please see below.
On 2019-06-25 5:36 p.m., Jonathan Corbet wrote:
> On Sat, 22 Jun 2019 22:19:47 +0700
> Phong Tran <tranmanphong@gmail.com> wrote:
>
>> - Update to the link in documenation
>> - Remove trailing white space
>> - Adaptation the sphinx doc syntax
>>
>> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
>
> Thanks for working to improve the documentation! That said, I think this
> patch needs a fair amount of work before we are ready to accept it. I'll
> only get partway in, but it should be enough to start with.
>
> The first overall thing I would like to point out (and hopefully the SCSI
> folks won't fight me too much on this) is that Documentation/scsi is the
> wrong place for much of this stuff. We are doing our best to organize the
> documentation with the audience in mind. So, for example, documents that
> are of interest to system administrators should go into
> Documentation/admin-guide. Information for driver developers should go in
> Documentation/driver-api. And so on.
>
> [...]
>
>> diff --git a/Documentation/scsi/link_power_management_policy.rst b/Documentation/scsi/link_power_management_policy.rst
>> new file mode 100644
>> index 000000000000..170f58c94cac
>> --- /dev/null
>> +++ b/Documentation/scsi/link_power_management_policy.rst
>> @@ -0,0 +1,22 @@
>> +SCSI Power Management Policy
>> +============================
>> +Its
>> +This parameter allows the user to set the link (interface) power management.
>> +There are 3 possible options:
>
> This isn't your fault, but...*which* parameter allows this? The document
> describes the values, but not where they can be set. That makes it less
> than fully useful.
>
>> ++-------------------+------------------------------------------------------+
>> +| Value | Effect |
>> ++===================+======================================================+
>> +| min_power | Tell the controller to try to make the link use the |
>> +| | least possible power when possible. This may |
>> +| | sacrifice some performance due to increased latency |
>> +| | when coming out of lower power states. |
>> ++-------------------+------------------------------------------------------+
>> +| max_performance | Generally, this means no power management. Tell |
>> +| | the controller to have performance be a priority |
>> +| | over power management. |
>> ++-------------------+------------------------------------------------------+
>> +| medium_power | Tell the controller to enter a lower power state |
>> +| | when possible, but do not enter the lowest power |
>> +| | state, thus improving latency over min_power setting.|
>> ++-------------------+------------------------------------------------------+
>
> [...]
>
>> diff --git a/Documentation/scsi/scsi-changer.txt b/Documentation/scsi/scsi-changer.rst
>> similarity index 71%
>> rename from Documentation/scsi/scsi-changer.txt
>> rename to Documentation/scsi/scsi-changer.rst
>> index ade046ea7c17..a4923873c77b 100644
>> --- a/Documentation/scsi/scsi-changer.txt
>> +++ b/Documentation/scsi/scsi-changer.rst
>> @@ -1,4 +1,3 @@
>> -
>> README for the SCSI media changer driver
>> ========================================
>>
>> @@ -10,7 +9,7 @@ common small CD-ROM changers, neither one-lun-per-slot SCSI changers
>> nor IDE drives.
>>
>> Userland tools available from here:
>> - http://linux.bytesex.org/misc/changer.html
>> + http://linux.bytesex.org/misc/changer.html
>>
>>
>> General Information
>> @@ -28,15 +27,17 @@ The SCSI changer model is complex, compared to - for example - IDE-CD
>> changers. But it allows to handle nearly all possible cases. It knows
>> 4 different types of changer elements:
>>
>> +::
>
> Two notes:
>
> - You can put the double colon on the line above ("...elements::") and
> don't need to make a separate line for it.
>
> - But, more to the point, please avoid the temptation to use a literal
> block for something that doesn't actually require that treatment. This
> should be reworked as an RST definition list.
>
>> media transport - this one shuffles around the media, i.e. the
>> transport arm. Also known as "picker".
>> storage - a slot which can hold a media.
>> import/export - the same as above, but is accessible from outside,
>> i.e. there the operator (you !) can use this to
>> fill in and remove media from the changer.
>> - Sometimes named "mailslot".
>> + Sometimes named "mailslot".
>> data transfer - this is the device which reads/writes, i.e. the
>> - CD-ROM / Tape / whatever drive.
>> + CD-ROM / Tape / whatever drive.
>
> [...]
>
>> diff --git a/Documentation/scsi/scsi-generic.txt b/Documentation/scsi/scsi-generic.rst
>> similarity index 70%
>> rename from Documentation/scsi/scsi-generic.txt
>> rename to Documentation/scsi/scsi-generic.rst
>> index 51be20a6a14d..8356810160f0 100644
>> --- a/Documentation/scsi/scsi-generic.txt
>> +++ b/Documentation/scsi/scsi-generic.rst
>> @@ -1,8 +1,10 @@
>> - Notes on Linux SCSI Generic (sg) driver
>> - ---------------------------------------
>> - 20020126
>> +=======================================
>> +Notes on Linux SCSI Generic (sg) driver
>> +=======================================
>> +20020126
>> +
>> Introduction
>> -============
>> +------------
>> The SCSI Generic driver (sg) is one of the four "high level" SCSI device
>> drivers along with sd, st and sr (disk, tape and CDROM respectively). Sg
>> is more generalized (but lower level) than its siblings and tends to be
>> @@ -16,20 +18,20 @@ and examples.
>>
>>
>> Major versions of the sg driver
>> -===============================
>> +-------------------------------
>> There are three major versions of sg found in the linux kernel (lk):
>> - - sg version 1 (original) from 1992 to early 1999 (lk 2.2.5) .
>> - It is based in the sg_header interface structure.
>> + - sg version 1 (original) from 1992 to early 1999 (lk 2.2.5) .
>> + It is based in the sg_header interface structure.
>> - sg version 2 from lk 2.2.6 in the 2.2 series. It is based on
>> - an extended version of the sg_header interface structure.
>> + an extended version of the sg_header interface structure.
>> - sg version 3 found in the lk 2.4 series (and the lk 2.5 series).
>> - It adds the sg_io_hdr interface structure.
>> + It adds the sg_io_hdr interface structure.
>
> Perhaps we don't *really* need to preserve information about what versions
> were around in the 1990's?
>
>> Sg driver documentation
>> -=======================
>> +-----------------------
>> The most recent documentation of the sg driver is kept at the Linux
>> -Documentation Project's (LDP) site:
>> +Documentation Project's (LDP) site:
>> http://www.tldp.org/HOWTO/SCSI-Generic-HOWTO
>
> That document claims to have been last updated in 2002. Is there really
> nothing more recent than that?
http://sg.danny.cz/sg/sg_v40.html
Last updated: yesterday.
>> This describes the sg version 3 driver found in the lk 2.4 series.
>
> ...and it's unclear to me that users of the 5.x kernel are much concerned
> with what was found in 2.4.
>
> That is the problem with this document in general. I suspect that about
> the only useful information left in it is the location of the sg3_utils
> source. I honestly don't think that it helps the documentation that much
> to carry forward ancient information to the RST format.
That old document is pretty damn accurate. At least one feature was
quietly removed over the years. And some ioctls no longer do anything
(see the ioctl table towards the end of the above document). The sg
driver's public interface is a lot more stable than the kernel API behind
it :-) . There is driver in FreeBSD that implements that interface, it's
called scsi_sg .
> Of course, doing this right by deleting obsolete information and updating
> the documents to reflect current reality is a *lot* more work. Probably
> far more than you were thinking of signing up for. If you were willing to
> work on this, there may be somebody from the SCSI community who would be in
> a position to help you with it.
Hmmm.
> Unfortunately, the SCSI community probably did not see this patch because
> you didn't copy the linux-scsi list. I'll fix that now, but they will not
> have seen your original patch. You should be sure to include them on
> future postings.
>
> I would like to make a suggestion, in addition to all of the above: rather
> than trying to do a mass conversion in a single 4000-line patch, start with
> a single file and post a patch doing just that one, being sure to include
> the linux-scsi list. That will give everybody something more workable to
> start with.
Editorial comments welcome.
Martin Petersen wants to see that document reworked in order to market the
new features that I am proposing. My approach is to document the code that
I have in front of me ***, plus add a bit of historical context.
I chose that format so I could add diagrams (using Libreoffice writer).
Do those diagrams help?
Doug Gilbert
*** it helps me find bugs in my code and my documentation. I'm also testing
the code at the same time.
^ permalink raw reply
* Re: [PATCH v13 05/13] clk: ingenic: Add driver for the TCU clocks
From: Stephen Boyd @ 2019-06-25 23:28 UTC (permalink / raw)
To: Paul Cercueil
Cc: Daniel Lezcano, James Hogan, Jason Cooper, Jonathan Corbet,
Lee Jones, Marc Zyngier, Michael Turquette, Paul Burton,
Ralf Baechle, Thomas Gleixner, Mathieu Malaterre, od, devicetree,
linux-kernel, linux-mips, linux-doc, linux-clk, Artur Rojek
In-Reply-To: <1561502227.10069.1@crapouillou.net>
Quoting Paul Cercueil (2019-06-25 15:37:07)
> >
> > Do you need to get the clk by name? Or can that clk be "known" to the
> > TCU somehow so we can already have a direct clk pointer?
>
> This clock is provided by a separate driver, so I have to obtain the
> clock pointer from devicetree.
Ok.
> >> +}
> >> +
> >> +static int __maybe_unused tcu_pm_suspend(void)
> >> +{
> >> + struct ingenic_tcu *tcu = ingenic_tcu;
> >> +
> >> + if (tcu->clk)
> >> + clk_disable(tcu->clk);
> >
> > Do you need to unprepare? Or it just isn't possible because this is
> > called from syscore and thus we can't sleep?
>
> I thought that clk_disable() was enough. We don't actually need to
> unprepare, do we? And yes, as you pointed out, this call cannot sleep.
Yeah unprepare isn't necessary, but it will be different on different
platforms. This is a highly platform specific driver though so I suspect
this is all fine.
^ permalink raw reply
* Re: [PATCH v5 07/18] kunit: test: add initial tests
From: Luis Chamberlain @ 2019-06-25 23:22 UTC (permalink / raw)
To: Brendan Higgins
Cc: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, peterz,
robh, sboyd, shuah, tytso, yamada.masahiro, devicetree, dri-devel,
kunit-dev, linux-doc, linux-fsdevel, linux-kbuild, linux-kernel,
linux-kselftest, linux-nvdimm, linux-um, Alexander.Levin,
Tim.Bird, amir73il, dan.carpenter, daniel, jdike, joel,
julia.lawall, khilman, knut.omang, logang, mpe, pmladek, rdunlap,
richard, rientjes, rostedt, wfg
In-Reply-To: <20190617082613.109131-8-brendanhiggins@google.com>
On Mon, Jun 17, 2019 at 01:26:02AM -0700, Brendan Higgins wrote:
> diff --git a/kunit/example-test.c b/kunit/example-test.c
> new file mode 100644
> index 0000000000000..f44b8ece488bb
> --- /dev/null
> +++ b/kunit/example-test.c
<-- snip -->
> +/*
> + * This defines a suite or grouping of tests.
> + *
> + * Test cases are defined as belonging to the suite by adding them to
> + * `kunit_cases`.
> + *
> + * Often it is desirable to run some function which will set up things which
> + * will be used by every test; this is accomplished with an `init` function
> + * which runs before each test case is invoked. Similarly, an `exit` function
> + * may be specified which runs after every test case and can be used to for
> + * cleanup. For clarity, running tests in a test module would behave as follows:
> + *
To be clear this is not the kernel module init, but rather the kunit
module init. I think using kmodule would make this clearer to a reader.
> + * module.init(test);
> + * module.test_case[0](test);
> + * module.exit(test);
> + * module.init(test);
> + * module.test_case[1](test);
> + * module.exit(test);
> + * ...;
> + */
Luis
^ permalink raw reply
* Re: [PATCH v7 2/2] fTPM: add documentation for ftpm driver
From: Randy Dunlap @ 2019-06-25 23:13 UTC (permalink / raw)
To: Sasha Levin, peterhuewe, jarkko.sakkinen, jgg
Cc: corbet, linux-kernel, linux-doc, linux-integrity, linux-kernel,
thiruan, bryankel, tee-dev, ilias.apalodimas, sumit.garg
In-Reply-To: <20190625201341.15865-3-sashal@kernel.org>
On 6/25/19 1:13 PM, Sasha Levin wrote:
> This patch adds basic documentation to describe the new fTPM driver.
>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
Thanks.
> ---
> Documentation/security/tpm/index.rst | 1 +
> Documentation/security/tpm/tpm_ftpm_tee.rst | 31 +++++++++++++++++++++
> 2 files changed, 32 insertions(+)
> create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst
>
> diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst
> index af77a7bbb070..15783668644f 100644
> --- a/Documentation/security/tpm/index.rst
> +++ b/Documentation/security/tpm/index.rst
> @@ -4,4 +4,5 @@ Trusted Platform Module documentation
>
> .. toctree::
>
> + tpm_ftpm_tee
> tpm_vtpm_proxy
> diff --git a/Documentation/security/tpm/tpm_ftpm_tee.rst b/Documentation/security/tpm/tpm_ftpm_tee.rst
> new file mode 100644
> index 000000000000..48de0dcec0f6
> --- /dev/null
> +++ b/Documentation/security/tpm/tpm_ftpm_tee.rst
> @@ -0,0 +1,31 @@
> +=============================================
> +Firmware TPM Driver
> +=============================================
> +
> +| Authors:
> +| Thirupathaiah Annapureddy <thiruan@microsoft.com>
> +| Sasha Levin <sashal@kernel.org>
> +
> +This document describes the firmware Trusted Platform Module (fTPM)
> +device driver.
> +
> +Introduction
> +============
> +
> +This driver is a shim for firmware implemented in ARM's TrustZone
> +environment. The driver allows programs to interact with the TPM in the same
> +way they would interact with a hardware TPM.
> +
> +Design
> +======
> +
> +The driver acts as a thin layer that passes commands to and from a TPM
> +implemented in firmware. The driver itself doesn't contain much logic and is
> +used more like a dumb pipe between firmware and kernel/userspace.
> +
> +The firmware itself is based on the following paper:
> +https://www.microsoft.com/en-us/research/wp-content/uploads/2017/06/ftpm1.pdf
> +
> +When the driver is loaded it will expose ``/dev/tpmX`` character devices to
> +userspace which will enable userspace to communicate with the firmware TPM
> +through this device.
>
--
~Randy
^ permalink raw reply
* Re: [PATCH v5 06/18] kbuild: enable building KUnit
From: Luis Chamberlain @ 2019-06-25 23:03 UTC (permalink / raw)
To: Brendan Higgins
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Peter Zijlstra, Rob Herring, Stephen Boyd, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g448rYqr3PHg0cfoddr70nktkWXcRfJoZHmuPJjTW53YYg@mail.gmail.com>
On Tue, Jun 25, 2019 at 03:41:29PM -0700, Brendan Higgins wrote:
> On Tue, Jun 25, 2019 at 3:13 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Mon, Jun 17, 2019 at 01:26:01AM -0700, Brendan Higgins wrote:
> > > diff --git a/Kconfig b/Kconfig
> > > index 48a80beab6853..10428501edb78 100644
> > > --- a/Kconfig
> > > +++ b/Kconfig
> > > @@ -30,3 +30,5 @@ source "crypto/Kconfig"
> > > source "lib/Kconfig"
> > >
> > > source "lib/Kconfig.debug"
> > > +
> > > +source "kunit/Kconfig"
> >
> > This patch would break compilation as kunit/Kconfig is not introduced. This
> > would would also break bisectability on this commit. This change should
> > either be folded in to the next patch, or just be a separate patch after
> > the next one.
>
> Maybe my brain isn't working right now, but I am pretty darn sure that
> I introduce kunit/Kconfig in the very first patch of this series.
> Quoting from the change summary from the first commit:
Indeed, my mistake, thanks!
Luis
^ permalink raw reply
* Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Luis Chamberlain @ 2019-06-25 23:02 UTC (permalink / raw)
To: Brendan Higgins
Cc: Stephen Boyd, Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook,
Kieran Bingham, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <CAFd5g47OABqN127cPKqoCOA_Wr9w=LFh_0XkF7LXu2iY9sFkSw@mail.gmail.com>
On Tue, Jun 25, 2019 at 03:14:45PM -0700, Brendan Higgins wrote:
> On Tue, Jun 25, 2019 at 2:44 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > Since its a new architecture and since you seem to imply most tests
> > don't require locking or even IRQs disabled, I think its worth to
> > consider the impact of adding such extreme locking requirements for
> > an initial ramp up.
>
> Fair enough, I can see the point of not wanting to use irq disabled
> until we get someone complaining about it, but I think making it
> thread safe is reasonable. It means there is one less thing to confuse
> a KUnit user and the only penalty paid is some very minor performance.
One reason I'm really excited about kunit is speed... so by all means I
think we're at a good point to analyze performance optimizationsm if
they do make sense.
While on the topic of parallization, what about support for running
different test cases in parallel? Or at the very least different kunit
modules in parallel. Few questions come up based on this prospect:
* Why not support parallelism from the start?
* Are you opposed to eventually having this added? For instance, there is
enough code on lib/test_kmod.c for batching tons of kthreads each
one running its own thing for testing purposes which could be used
as template.
* If we eventually *did* support it:
- Would logs be skewed?
- Could we have a way to query: give me log for only kunit module
named "foo"?
Luis
^ permalink raw reply
* Re: [PATCH v5 06/18] kbuild: enable building KUnit
From: Brendan Higgins @ 2019-06-25 22:41 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Peter Zijlstra, Rob Herring, Stephen Boyd, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190625221318.GO19023@42.do-not-panic.com>
On Tue, Jun 25, 2019 at 3:13 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Mon, Jun 17, 2019 at 01:26:01AM -0700, Brendan Higgins wrote:
> > diff --git a/Kconfig b/Kconfig
> > index 48a80beab6853..10428501edb78 100644
> > --- a/Kconfig
> > +++ b/Kconfig
> > @@ -30,3 +30,5 @@ source "crypto/Kconfig"
> > source "lib/Kconfig"
> >
> > source "lib/Kconfig.debug"
> > +
> > +source "kunit/Kconfig"
>
> This patch would break compilation as kunit/Kconfig is not introduced. This
> would would also break bisectability on this commit. This change should
> either be folded in to the next patch, or just be a separate patch after
> the next one.
Maybe my brain isn't working right now, but I am pretty darn sure that
I introduce kunit/Kconfig in the very first patch of this series.
Quoting from the change summary from the first commit:
> include/kunit/test.h | 161 +++++++++++++++++++++++++++++++++
> kunit/Kconfig | 17 ++++
> kunit/Makefile | 1 +
> kunit/test.c | 210 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 389 insertions(+)
> create mode 100644 include/kunit/test.h
> create mode 100644 kunit/Kconfig
I am not crazy, right?
> create mode 100644 kunit/Makefile
> create mode 100644 kunit/test.c
^ permalink raw reply
* Re: [PATCH v13 05/13] clk: ingenic: Add driver for the TCU clocks
From: Paul Cercueil @ 2019-06-25 22:37 UTC (permalink / raw)
To: Stephen Boyd
Cc: Daniel Lezcano, James Hogan, Jason Cooper, Jonathan Corbet,
Lee Jones, Marc Zyngier, Michael Turquette, Paul Burton,
Ralf Baechle, Thomas Gleixner, Mathieu Malaterre, od, devicetree,
linux-kernel, linux-mips, linux-doc, linux-clk, Artur Rojek
In-Reply-To: <20190625220931.2F69B2086D@mail.kernel.org>
Le mer. 26 juin 2019 à 0:09, Stephen Boyd <sboyd@kernel.org> a écrit :
> Quoting Paul Cercueil (2019-06-24 15:57:51)
>> diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
>> new file mode 100644
>> index 000000000000..6d667c4a2bd5
>> --- /dev/null
>> +++ b/drivers/clk/ingenic/tcu.c
>> @@ -0,0 +1,473 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * JZ47xx SoCs TCU clocks driver
>> + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/mfd/ingenic-tcu.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/syscore_ops.h>
>> +
>> +#include <dt-bindings/clock/ingenic,tcu.h>
>> +
> [...]
>> +
>> +static const struct ingenic_soc_info jz4740_soc_info = {
>> + .num_channels = 8,
>> + .has_ost = false,
>> + .has_tcu_clk = true,
>> +};
>> +
>> +static const struct ingenic_soc_info jz4725b_soc_info = {
>> + .num_channels = 6,
>> + .has_ost = true,
>> + .has_tcu_clk = true,
>> +};
>> +
>> +static const struct ingenic_soc_info jz4770_soc_info = {
>> + .num_channels = 8,
>> + .has_ost = true,
>> + .has_tcu_clk = false,
>> +};
>> +
>> +static const struct of_device_id ingenic_tcu_of_match[]
>> __initconst = {
>> + { .compatible = "ingenic,jz4740-tcu", .data =
>> &jz4740_soc_info, },
>> + { .compatible = "ingenic,jz4725b-tcu", .data =
>> &jz4725b_soc_info, },
>> + { .compatible = "ingenic,jz4770-tcu", .data =
>> &jz4770_soc_info, },
>> + { }
>> +};
>> +
>> +static int __init ingenic_tcu_probe(struct device_node *np)
>> +{
>> + const struct of_device_id *id =
>> of_match_node(ingenic_tcu_of_match, np);
>> + struct ingenic_tcu *tcu;
>> + struct regmap *map;
>> + unsigned int i;
>> + int ret;
>> +
>> + map = ingenic_tcu_get_regmap(np);
>> + if (IS_ERR(map))
>> + return PTR_ERR(map);
>> +
>> + tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
>> + if (!tcu)
>> + return -ENOMEM;
>> +
>> + tcu->map = map;
>> + tcu->soc_info = id->data;
>> +
>> + if (tcu->soc_info->has_tcu_clk) {
>> + tcu->clk = of_clk_get_by_name(np, "tcu");
>
> Do you need to get the clk by name? Or can that clk be "known" to the
> TCU somehow so we can already have a direct clk pointer?
This clock is provided by a separate driver, so I have to obtain the
clock pointer from devicetree.
>
>> + if (IS_ERR(tcu->clk)) {
>> + ret = PTR_ERR(tcu->clk);
>> + pr_crit("Cannot get TCU clock\n");
>> + goto err_free_tcu;
>> + }
>> +
>> + ret = clk_prepare_enable(tcu->clk);
>> + if (ret) {
>> + pr_crit("Unable to enable TCU clock\n");
>> + goto err_put_clk;
>> + }
>> + }
>> +
>> + tcu->clocks = kzalloc(sizeof(*tcu->clocks) +
>> + sizeof(*tcu->clocks->hws) *
>> TCU_CLK_COUNT,
>> + GFP_KERNEL);
>> + if (!tcu->clocks) {
>> + ret = -ENOMEM;
>> + goto err_clk_disable;
>> + }
>> +
>> + tcu->clocks->num = TCU_CLK_COUNT;
>> +
>> + for (i = 0; i < tcu->soc_info->num_channels; i++) {
>> + ret = ingenic_tcu_register_clock(tcu, i,
>> TCU_PARENT_EXT,
>> +
>> &ingenic_tcu_clk_info[i],
>> + tcu->clocks);
>> + if (ret) {
>> + pr_crit("cannot register clock %i\n", i);
>> + goto err_unregister_timer_clocks;
>> + }
>> + }
>> +
>> + /*
>> + * We set EXT as the default parent clock for all the TCU
>> clocks
>> + * except for the watchdog one, where we set the RTC clock
>> as the
>> + * parent. Since the EXT and PCLK are much faster than the
>> RTC clock,
>> + * the watchdog would kick after a maximum time of 5s, and
>> we might
>> + * want a slower kicking time.
>> + */
>> + ret = ingenic_tcu_register_clock(tcu, TCU_CLK_WDT,
>> TCU_PARENT_RTC,
>> +
>> &ingenic_tcu_watchdog_clk_info,
>> + tcu->clocks);
>> + if (ret) {
>> + pr_crit("cannot register watchdog clock\n");
>> + goto err_unregister_timer_clocks;
>> + }
>> +
>> + if (tcu->soc_info->has_ost) {
>> + ret = ingenic_tcu_register_clock(tcu, TCU_CLK_OST,
>> + TCU_PARENT_EXT,
>> +
>> &ingenic_tcu_ost_clk_info,
>> + tcu->clocks);
>> + if (ret) {
>> + pr_crit("cannot register ost clock\n");
>> + goto err_unregister_watchdog_clock;
>> + }
>> + }
>> +
>> + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
>> tcu->clocks);
>> + if (ret) {
>> + pr_crit("cannot add OF clock provider\n");
>> + goto err_unregister_ost_clock;
>> + }
>> +
>> + ingenic_tcu = tcu;
>> +
>> + return 0;
>> +
>> +err_unregister_ost_clock:
>> + if (tcu->soc_info->has_ost)
>> + clk_hw_unregister(tcu->clocks->hws[i + 1]);
>> +err_unregister_watchdog_clock:
>> + clk_hw_unregister(tcu->clocks->hws[i]);
>> +err_unregister_timer_clocks:
>> + for (i = 0; i < tcu->clocks->num; i++)
>> + if (tcu->clocks->hws[i])
>> + clk_hw_unregister(tcu->clocks->hws[i]);
>> + kfree(tcu->clocks);
>> +err_clk_disable:
>> + if (tcu->soc_info->has_tcu_clk)
>> + clk_disable_unprepare(tcu->clk);
>> +err_put_clk:
>> + if (tcu->soc_info->has_tcu_clk)
>> + clk_put(tcu->clk);
>> +err_free_tcu:
>> + kfree(tcu);
>> + return ret;
>
> Too bad this isn't a device with devm!
>
>> +}
>> +
>> +static int __maybe_unused tcu_pm_suspend(void)
>> +{
>> + struct ingenic_tcu *tcu = ingenic_tcu;
>> +
>> + if (tcu->clk)
>> + clk_disable(tcu->clk);
>
> Do you need to unprepare? Or it just isn't possible because this is
> called from syscore and thus we can't sleep?
I thought that clk_disable() was enough. We don't actually need to
unprepare, do we? And yes, as you pointed out, this call cannot sleep.
>> +
>> + return 0;
>> +}
>> +
>> +static void __maybe_unused tcu_pm_resume(void)
>> +{
>> + struct ingenic_tcu *tcu = ingenic_tcu;
>> +
>> + if (tcu->clk)
>> + clk_enable(tcu->clk);
>> +}
>> +
>> +static struct syscore_ops __maybe_unused tcu_pm_ops = {
>> + .suspend = tcu_pm_suspend,
>> + .resume = tcu_pm_resume,
>> +};
>> +
>> +static void __init ingenic_tcu_init(struct device_node *np)
>> +{
>> + int ret = ingenic_tcu_probe(np);
>> +
>> + if (ret)
>> + pr_crit("Failed to initialize TCU clocks: %i\n",
>> ret);
>
> Should be %d instead of %i? Applies to all this file.
OK.
Thanks,
-Paul
>> +
>> + if (IS_ENABLED(CONFIG_PM_SLEEP))
>> + register_syscore_ops(&tcu_pm_ops);
>> +}
^ permalink raw reply
* Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Luis Chamberlain @ 2019-06-25 22:33 UTC (permalink / raw)
To: Brendan Higgins
Cc: frowand.list, gregkh, jpoimboe, keescook, kieran.bingham, peterz,
robh, sboyd, shuah, tytso, yamada.masahiro, devicetree, dri-devel,
kunit-dev, linux-doc, linux-fsdevel, linux-kbuild, linux-kernel,
linux-kselftest, linux-nvdimm, linux-um, Alexander.Levin,
Tim.Bird, amir73il, dan.carpenter, daniel, jdike, joel,
julia.lawall, khilman, knut.omang, logang, mpe, pmladek, rdunlap,
richard, rientjes, rostedt, wfg
In-Reply-To: <20190617082613.109131-2-brendanhiggins@google.com>
On Mon, Jun 17, 2019 at 01:25:56AM -0700, Brendan Higgins wrote:
> +/**
> + * module_test() - used to register a &struct kunit_module with KUnit.
> + * @module: a statically allocated &struct kunit_module.
> + *
> + * Registers @module with the test framework. See &struct kunit_module for more
> + * information.
> + */
> +#define module_test(module) \
> + static int module_kunit_init##module(void) \
> + { \
> + return kunit_run_tests(&module); \
> + } \
> + late_initcall(module_kunit_init##module)
Becuase late_initcall() is used, if these modules are built-in, this
would preclude the ability to test things prior to this part of the
kernel under UML or whatever architecture runs the tests. So, this
limits the scope of testing. Small detail but the scope whould be
documented.
> +static void kunit_print_tap_version(void)
> +{
> + if (!kunit_has_printed_tap_version) {
> + kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n");
What is this TAP thing? Why should we care what version it is on?
Why are we printing this?
> + kunit_has_printed_tap_version = true;
> + }
> +}
> +
> +static size_t kunit_test_cases_len(struct kunit_case *test_cases)
> +{
> + struct kunit_case *test_case;
> + size_t len = 0;
> +
> + for (test_case = test_cases; test_case->run_case; test_case++)
If we make the last test case NULL, we'd just check for test_case here,
and save ourselves an extra few bytes per test module. Any reason why
the last test case cannot be NULL?
> +void kunit_init_test(struct kunit *test, const char *name)
> +{
> + spin_lock_init(&test->lock);
> + test->name = name;
> + test->success = true;
> +}
> +
> +/*
> + * Performs all logic to run a test case.
> + */
> +static void kunit_run_case(struct kunit_module *module,
> + struct kunit_case *test_case)
> +{
> + struct kunit test;
> + int ret = 0;
> +
> + kunit_init_test(&test, test_case->name);
> +
> + if (module->init) {
> + ret = module->init(&test);
I believe if we used struct kunit_module *kmodule it would be much
clearer who's init this is.
Luis
^ permalink raw reply
* Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Brendan Higgins @ 2019-06-25 22:14 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Stephen Boyd, Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook,
Kieran Bingham, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190625214427.GN19023@42.do-not-panic.com>
On Tue, Jun 25, 2019 at 2:44 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Tue, Jun 25, 2019 at 01:28:25PM -0700, Brendan Higgins wrote:
> > On Wed, Jun 19, 2019 at 5:15 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-06-17 01:25:56)
> > > > diff --git a/kunit/test.c b/kunit/test.c
> > > > new file mode 100644
> > > > index 0000000000000..d05d254f1521f
> > > > --- /dev/null
> > > > +++ b/kunit/test.c
> > > > @@ -0,0 +1,210 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Base unit test (KUnit) API.
> > > > + *
> > > > + * Copyright (C) 2019, Google LLC.
> > > > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > > > + */
> > > > +
> > > > +#include <linux/sched/debug.h>
> > > > +#include <kunit/test.h>
> > > > +
> > > > +static bool kunit_get_success(struct kunit *test)
> > > > +{
> > > > + unsigned long flags;
> > > > + bool success;
> > > > +
> > > > + spin_lock_irqsave(&test->lock, flags);
> > > > + success = test->success;
> > > > + spin_unlock_irqrestore(&test->lock, flags);
> > >
> > > I still don't understand the locking scheme in this code. Is the
> > > intention to make getter and setter APIs that are "safe" by adding in a
> > > spinlock that is held around getting and setting various members in the
> > > kunit structure?
> >
> > Yes, your understanding is correct. It is possible for a user to write
> > a test such that certain elements may be updated in different threads;
> > this would most likely happen in the case where someone wants to make
> > an assertion or an expectation in a thread created by a piece of code
> > under test. Although this should generally be avoided, it is possible,
> > and there are occasionally good reasons to do so, so it is
> > functionality that we should support.
> >
> > Do you think I should add a comment to this effect?
> >
> > > In what situation is there more than one thread reading or writing the
> > > kunit struct? Isn't it only a single process that is going to be
> >
> > As I said above, it is possible that the code under test may spawn a
> > new thread that may make an expectation or an assertion. It is not a
> > super common use case, but it is possible.
>
> I wonder if it is worth to have then different types of tests based on
> locking requirements. One with no locking, since it seems you imply
> most tests would fall under this category, then locking, and another
> with IRQ context.
>
> If no locking is done at all for all tests which do not require locking,
> is there any gains at run time? I'm sure it might be minimum but
> curious.
Yeah, I don't think it is worth it.
I don't think we need to be squeezing every ounce of performance out
of unit tests, since they are inherently a cost and are not intended
to be run in a production deployed kernel as part of normal production
usage.
> > > operating on this structure? And why do we need to disable irqs? Are we
> > > expecting to be modifying the unit tests from irq contexts?
> >
> > There are instances where someone may want to test a driver which has
> > an interrupt handler in it. I actually have (not the greatest) example
> > here. Now in these cases, I expect someone to use a mock irqchip or
> > some other fake mechanism to trigger the interrupt handler and not
> > actual hardware; technically speaking in this case, it is not going to
> > be accessed from a "real" irq context; however, the code under test
> > should think that it is in an irq context; given that, I figured it is
> > best to just treat it as a real irq context. Does that make sense?
>
> Since its a new architecture and since you seem to imply most tests
> don't require locking or even IRQs disabled, I think its worth to
> consider the impact of adding such extreme locking requirements for
> an initial ramp up.
Fair enough, I can see the point of not wanting to use irq disabled
until we get someone complaining about it, but I think making it
thread safe is reasonable. It means there is one less thing to confuse
a KUnit user and the only penalty paid is some very minor performance.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox