* Re: [Linux-kernel-mentees][PATCH v5 1/5] Documentation: RCU: Convert RCU basic concepts to reST
From: Steven Rostedt @ 2019-06-27 15:13 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Jiunn Chang, skhan, linux-kernel-mentees, rcu, linux-doc, paulmck,
josh, mathieu.desnoyers, jiangshanlai, joel
In-Reply-To: <20190627083443.4f4918a7@lwn.net>
On Thu, 27 Jun 2019 08:34:43 -0600
Jonathan Corbet <corbet@lwn.net> wrote:
> On Wed, 26 Jun 2019 15:07:01 -0500
> Jiunn Chang <c0d1n61at3@gmail.com> wrote:
>
> > RCU basic concepts reST markup.
> >
> > Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> So this is a little detail but ... your signoff should be the last thing
> in the set of tags on the patch.
Note, I've been seeing this a lot lately, and then noticed, that when I
downloaded a patch directly from patchwork, it placed all the
Reviewed-by and Acked-by tags after the original Signed-off-by. I
checked the original patch on the mailing list, and it had no other
tags but the Signed-off-by. I then pulled one of my own patches, and it
did it to that patch as well.
I too prefer the Signed-off-by be last, but our tooling needs to do
this as well, otherwise it's a failure in our procedures.
-- Steve
^ permalink raw reply
* Re: [PATCH 1/2] mm, memcontrol: Add memcg_iterate_all()
From: Michal Hocko @ 2019-06-27 15:07 UTC (permalink / raw)
To: Waiman Long
Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, Alexander Viro, Jonathan Corbet, Luis Chamberlain,
Kees Cook, Johannes Weiner, Vladimir Davydov, linux-mm, linux-doc,
linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <20190624174219.25513-2-longman@redhat.com>
On Mon 24-06-19 13:42:18, Waiman Long wrote:
> Add a memcg_iterate_all() function for iterating all the available
> memory cgroups and call the given callback function for each of the
> memory cgruops.
Why is a trivial wrapper any better than open coded usage of the
iterator?
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> include/linux/memcontrol.h | 3 +++
> mm/memcontrol.c | 13 +++++++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 1dcb763bb610..0e31418e5a47 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1268,6 +1268,9 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
> struct kmem_cache *memcg_kmem_get_cache(struct kmem_cache *cachep);
> void memcg_kmem_put_cache(struct kmem_cache *cachep);
>
> +extern void memcg_iterate_all(void (*callback)(struct mem_cgroup *memcg,
> + void *arg), void *arg);
> +
> #ifdef CONFIG_MEMCG_KMEM
> int __memcg_kmem_charge(struct page *page, gfp_t gfp, int order);
> void __memcg_kmem_uncharge(struct page *page, int order);
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ba9138a4a1de..c1c4706f7696 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -443,6 +443,19 @@ static int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg)
> static void memcg_free_shrinker_maps(struct mem_cgroup *memcg) { }
> #endif /* CONFIG_MEMCG_KMEM */
>
> +/*
> + * Iterate all the memory cgroups and call the given callback function
> + * for each of the memory cgroups.
> + */
> +void memcg_iterate_all(void (*callback)(struct mem_cgroup *memcg, void *arg),
> + void *arg)
> +{
> + struct mem_cgroup *memcg;
> +
> + for_each_mem_cgroup(memcg)
> + callback(memcg, arg);
> +}
> +
> /**
> * mem_cgroup_css_from_page - css of the memcg associated with a page
> * @page: page of interest
> --
> 2.18.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [Linux-kernel-mentees][PATCH v5 1/5] Documentation: RCU: Convert RCU basic concepts to reST
From: Jonathan Corbet @ 2019-06-27 14:34 UTC (permalink / raw)
To: Jiunn Chang
Cc: skhan, linux-kernel-mentees, rcu, linux-doc, paulmck, josh,
rostedt, mathieu.desnoyers, jiangshanlai, joel
In-Reply-To: <20190626200705.24501-2-c0d1n61at3@gmail.com>
On Wed, 26 Jun 2019 15:07:01 -0500
Jiunn Chang <c0d1n61at3@gmail.com> wrote:
> RCU basic concepts reST markup.
>
> Signed-off-by: Jiunn Chang <c0d1n61at3@gmail.com>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
So this is a little detail but ... your signoff should be the last thing
in the set of tags on the patch.
This isn't worth making you do yet another revision, so I went ahead and
applied the patches and fixed the tag ordering on the way in. I'll also
append a patch adding the new RCU stuff into the core-api manual so people
can actually get to it.
Thanks,
jon
^ permalink raw reply
* [PATCH] docs: format kernel-parameters -- as code
From: Stephen Kitt @ 2019-06-27 13:59 UTC (permalink / raw)
To: corbet, linux-doc; +Cc: linux-kernel, Stephen Kitt
The current ReStructuredText formatting results in "--", used to
indicate the end of the kernel command-line parameters, appearing as
an en-dash instead of two hyphens; this patch formats them as code,
"``--``", as done elsewhere in the documentation.
Signed-off-by: Stephen Kitt <steve@sk2.org>
---
Documentation/admin-guide/kernel-parameters.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index 0124980dca2d..b8d479b76648 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -9,11 +9,11 @@ and sorted into English Dictionary order (defined as ignoring all
punctuation and sorting digits before letters in a case insensitive
manner), and with descriptions where known.
-The kernel parses parameters from the kernel command line up to "--";
+The kernel parses parameters from the kernel command line up to "``--``";
if it doesn't recognize a parameter and it doesn't contain a '.', the
parameter gets passed to init: parameters with '=' go into init's
environment, others are passed as command line arguments to init.
-Everything after "--" is passed as an argument to init.
+Everything after "``--``" is passed as an argument to init.
Module parameters can be specified in two ways: via the kernel command
line with a module name prefix, or via modprobe, e.g.::
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
From: Ilias Apalodimas @ 2019-06-27 13:30 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Sasha Levin, peterhuewe, jgg, corbet, linux-kernel, linux-doc,
linux-integrity, linux-kernel, thiruan, bryankel, tee-dev,
sumit.garg, rdunlap
In-Reply-To: <b688e845ccbe011c54b10043fbc3c0de8f0befc2.camel@linux.intel.com>
Hi Jarkko,
> On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote:
> > > You've used so much on this so shouldn't this have that somewhat new
> > > co-developed-by tag? I'm also wondering can this work at all
> >
> > Honestly, I've just been massaging this patch more than "authoring" it.
> > If you feel strongly about it feel free to add a Co-authored patch with
> > my name, but in my mind this is just Thiru's work.
>
> This is just my subjective view but writing code is easier than making
> it work in the mainline in 99% of cases. If this patch was doing
> something revolutional, lets say a new outstanding scheduling algorithm,
> then I would think otherwise. It is not. You without question deserve
> both credit and also the blame (if this breaks everything) :-)
>
> > > process-wise if the original author of the patch is also the only tester
> > > of the patch?
> >
> > There's not much we can do about this... Linaro folks have tested this
> > without the fTPM firmware, so at the very least it won't explode for
> > everyone. If for some reason non-microsoft folks see issues then we can
> > submit patches on top to fix this, we're not just throwing this at you
> > and running away.
>
> So why any of those Linaro folks can't do it? I can add after tested-by
> tag parentheses something explaining that context of testing. It is
> reasonable given the circumstances.
There's 2 teams from Microsoft trying to do this [1]. We tested the previous
implementation (which problems on probing as built-in). We had to change some
stuff in the OP-TEE fTPM implementation [2] and test it in QEMU.
What i quickly did with this module was to replace the kernel of the previous
build with the new one. Unfortunately i couldn't get it to work, but i don't
know if it's the module or the changes in the fTPM OP-TEE part. Since you have
tested it my guess is that it has something to do with the OP-TEE part. I don't
have any objections in this going in. On the contrary i think the functionality
is really useful. I don't have hardware to test this at the moment, but once i
get it, i'll give it a spin.
The part i tested is that the probing works as expected when no fTPM
implementation is running on secure world.
Since it has been tested and doesn't break anything we can always fix corner,
cases afterwards with more extensive testing
[1]
https://github.com/ms-iot/linux/blob/ms-optee-ocalls-merge/drivers/char/tpm/tpm_ftpm_optee.c
[2] https://github.com/jbech-linaro/manifest/blob/ftpm/README.md
Thanks
/Ilias
>
> I can also give an explanation in my next PR along the lines what you
> are saying. This would definitely work for me.
>
> /Jarkko
>
^ permalink raw reply
* Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
From: Jarkko Sakkinen @ 2019-06-27 13:19 UTC (permalink / raw)
To: Sasha Levin
Cc: peterhuewe, jgg, corbet, linux-kernel, linux-doc, linux-integrity,
linux-kernel, thiruan, bryankel, tee-dev, ilias.apalodimas,
sumit.garg, rdunlap
In-Reply-To: <b688e845ccbe011c54b10043fbc3c0de8f0befc2.camel@linux.intel.com>
On Thu, 2019-06-27 at 16:17 +0300, Jarkko Sakkinen wrote:
> On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote:
> > > You've used so much on this so shouldn't this have that somewhat new
> > > co-developed-by tag? I'm also wondering can this work at all
> >
> > Honestly, I've just been massaging this patch more than "authoring" it.
> > If you feel strongly about it feel free to add a Co-authored patch with
> > my name, but in my mind this is just Thiru's work.
>
> This is just my subjective view but writing code is easier than making
> it work in the mainline in 99% of cases. If this patch was doing
> something revolutional, lets say a new outstanding scheduling algorithm,
> then I would think otherwise. It is not. You without question deserve
> both credit and also the blame (if this breaks everything) :-)
Not like I'm putting the pressure on this. You make the call
with the tag. Put it if you wwant. I'm cool with either.
/Jarkko
^ permalink raw reply
* Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
From: Jarkko Sakkinen @ 2019-06-27 13:17 UTC (permalink / raw)
To: Sasha Levin
Cc: peterhuewe, jgg, corbet, linux-kernel, linux-doc, linux-integrity,
linux-kernel, thiruan, bryankel, tee-dev, ilias.apalodimas,
sumit.garg, rdunlap
In-Reply-To: <20190626235653.GL7898@sasha-vm>
On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote:
> > You've used so much on this so shouldn't this have that somewhat new
> > co-developed-by tag? I'm also wondering can this work at all
>
> Honestly, I've just been massaging this patch more than "authoring" it.
> If you feel strongly about it feel free to add a Co-authored patch with
> my name, but in my mind this is just Thiru's work.
This is just my subjective view but writing code is easier than making
it work in the mainline in 99% of cases. If this patch was doing
something revolutional, lets say a new outstanding scheduling algorithm,
then I would think otherwise. It is not. You without question deserve
both credit and also the blame (if this breaks everything) :-)
> > process-wise if the original author of the patch is also the only tester
> > of the patch?
>
> There's not much we can do about this... Linaro folks have tested this
> without the fTPM firmware, so at the very least it won't explode for
> everyone. If for some reason non-microsoft folks see issues then we can
> submit patches on top to fix this, we're not just throwing this at you
> and running away.
So why any of those Linaro folks can't do it? I can add after tested-by
tag parentheses something explaining that context of testing. It is
reasonable given the circumstances.
I can also give an explanation in my next PR along the lines what you
are saying. This would definitely work for me.
/Jarkko
^ permalink raw reply
* Re: [PATCH v2 0/4] Compile-test UAPI and kernel headers
From: Jani Nikula @ 2019-06-27 11:39 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: Sam Ravnborg, Masahiro Yamada, Tony Luck, linux-doc,
John Fastabend, Jonathan Corbet, Jakub Kicinski, linux-riscv,
Daniel Borkmann, xdp-newbies, Anton Vorontsov, Palmer Dabbelt,
Matthias Brugger, Song Liu, Yonghong Song, Michal Marek,
Jesper Dangaard Brouer, Martin KaFai Lau, linux-mediatek,
linux-arm-kernel, Albert Ou, Colin Cross, David S. Miller,
Kees Cook, Alexei Starovoitov, netdev, linux-kernel, bpf
In-Reply-To: <20190627014617.600-1-yamada.masahiro@socionext.com>
On Thu, 27 Jun 2019, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 1/4: reworked v2.
>
> 2/4: fix a flaw I noticed when I was working on this series
>
> 3/4: maybe useful for 4/4 and in some other places
>
> 4/4: v2. compile as many headers as possible.
>
>
> Changes in v2:
> - Add CONFIG_CPU_{BIG,LITTLE}_ENDIAN guard to avoid build error
> - Use 'header-test-' instead of 'no-header-test'
> - Avoid weird 'find' warning when cleaning
> - New patch
> - New patch
> - Add everything to test coverage, and exclude broken ones
> - Rename 'Makefile' to 'Kbuild'
> - Add CONFIG_KERNEL_HEADER_TEST option
>
> Masahiro Yamada (4):
> kbuild: compile-test UAPI headers to ensure they are self-contained
> kbuild: do not create wrappers for header-test-y
> kbuild: support header-test-pattern-y
> kbuild: compile-test kernel headers to ensure they are self-contained
[responding here because I didn't receive the actual patch]
This looks like it's doing what it's supposed to, but I ran into a bunch
of build fails with CONFIG_OF=n. Sent a fix to one [1], but stopped at
the next. Looks like you'll have to exclude more. And I'm pretty sure
we'll uncover more configurations where this will fail.
But I do applaud the goal, and I'm committed to making all include/drm
headers self-contained. I wouldn't block this based on the issues, it's
pretty much the only way to expose them and get them fixed/excluded, and
it's behind a config knob after all.
With the caveat that I didn't finish the build, but OTOH tested the
rainy day scenario and had the patch find issues it's meant to find:
Tested-by: Jani Nikula <jani.nikula@intel.com>
[1] http://patchwork.freedesktop.org/patch/msgid/20190627110103.7539-1-jani.nikula@intel.com
>
> .gitignore | 1 -
> Documentation/dontdiff | 1 -
> Documentation/kbuild/makefiles.txt | 13 +-
> Makefile | 4 +-
> include/Kbuild | 1134 ++++++++++++++++++++++++++++
> init/Kconfig | 22 +
> scripts/Makefile.build | 10 +-
> scripts/Makefile.lib | 12 +-
> scripts/cc-system-headers.sh | 8 +
> usr/.gitignore | 1 -
> usr/Makefile | 2 +
> usr/include/.gitignore | 3 +
> usr/include/Makefile | 133 ++++
> 13 files changed, 1331 insertions(+), 13 deletions(-)
> create mode 100644 include/Kbuild
> create mode 100755 scripts/cc-system-headers.sh
> create mode 100644 usr/include/.gitignore
> create mode 100644 usr/include/Makefile
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH v2 3/4] kbuild: support header-test-pattern-y
From: Jani Nikula @ 2019-06-27 11:28 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: Sam Ravnborg, Masahiro Yamada, linux-doc, linux-kernel,
Jonathan Corbet, Michal Marek
In-Reply-To: <20190627014617.600-4-yamada.masahiro@socionext.com>
On Thu, 27 Jun 2019, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> In my view, most of headers can be self-contained. So, it would be
> tedious to add every header to header-test-y explicitly. We usually
> end up with "all headers with some exceptions".
>
> There are two types in exceptions:
>
> [1] headers that are never compiled as standalone units
>
> For examples, include/linux/compiler-gcc.h is not intended to be
> included directly. We should always exclude such ones.
>
> [2] headers that are conditionally compiled as standalone units
>
> Some headers can be compiled only for particular architectures.
> For example, include/linux/arm-cci.h can be compiled only for
> arm/arm64 because it requires <asm/arm-cci.h> to exist.
> Clang can compile include/soc/nps/mtm.h only for arc because
> it contains an arch-specific register in inline assembler.
>
> For [2], we can write Makefile like this:
>
> header-test-$(CONFIG_ARM) += linux/arm-cci.h
>
> The new syntax header-test-pattern-y will be useful to specify
> "the rest".
>
> The typical usage is like this:
>
> header-test-pattern-y += */*.h
>
> This adds all the headers in sub-directories to the test coverage,
> but headers added to header-test- are excluded. In this regards,
> header-test-pattern-y behaves like a weaker variant of header-test-y.
>
> Caveat:
> The patterns in header-test-pattern-y are prefixed with $(srctree)/$(src)/
> but not $(objtree)/$(obj)/. Stale generated patterns are often left over.
> For example, you will have ones when you traverse the git history for
> 'git bisect' without cleaning. If a wildcard is used for generated
> headers, it may match to stale headers.
>
> If you really want to compile-test generated headers, I recommend to
> add them to header-test-y explicitly. One pitfall is $(srctree)/$(src)/
> and $(objtree)/$(obj)/ point to the same directory for in-tree building.
> So, header-test-pattern-y should be used with care. It can potentially
> match to generated headers, which may be stale and fail to compile.
>
> Caveat2:
> You could use wildcard for header-test-. For example,
>
> header-test- += asm-generic/%
>
> ... will exclude headers in asm-generic directory. Unfortunately, the
> wildcard character is '%' instead of '*' because this is evaluated by
> $(filter-out ...) whereas header-test-pattern-y is evaluated by
> $(wildcard ...). This is a kludge, but seems useful in some places...
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Awesome! This will let us get rid of our local $(wildcard) hacks.
Tested-by: Jani Nikula <jani.nikula@intel.com>
> ---
>
> Changes in v2:
> - New patch
>
> Documentation/kbuild/makefiles.txt | 10 ++++++++++
> scripts/Makefile.lib | 10 ++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
> index 5080fec34609..b817e6cefb77 100644
> --- a/Documentation/kbuild/makefiles.txt
> +++ b/Documentation/kbuild/makefiles.txt
> @@ -1025,6 +1025,16 @@ When kbuild executes, the following steps are followed (roughly):
> i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled,
> this builds them as part of extra-y.
>
> + header-test-pattern-y
> +
> + This works as a weaker version of header-test-y, and accepts wildcard
> + patterns. The typical usage is:
> +
> + header-test-pattern-y += *.h
> +
> + This specifies all the files that matches to '*.h' in the current
> + directory, but the files in 'header-test-' are excluded.
> +
> --- 6.7 Commands useful for building a boot image
>
> Kbuild provides a few macros that are useful when building a
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 55ae1ec65342..54444933bbab 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -67,6 +67,16 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
> endif
>
> # Test self-contained headers
> +
> +# Wildcard searches in $(srctree)/$(src)/, but not in $(objtree)/$(obj)/.
> +# Stale generated headers are often left over, so wildcard matching should
> +# be avoided. Please notice $(srctree)/$(src)/ and $(objtree)/$(obj) point
> +# to the same location for in-tree building.
> +header-test-y += $(filter-out $(header-test-), \
> + $(patsubst $(srctree)/$(src)/%, %, \
> + $(wildcard $(addprefix $(srctree)/$(src)/, \
> + $(header-test-pattern-y)))))
> +
> extra-$(CONFIG_HEADER_TEST) += $(addsuffix .s, $(header-test-y))
>
> # Add subdir path
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH v2 2/4] kbuild: do not create wrappers for header-test-y
From: Jani Nikula @ 2019-06-27 11:25 UTC (permalink / raw)
To: Masahiro Yamada, linux-kbuild
Cc: Sam Ravnborg, Masahiro Yamada, linux-doc, Jonathan Corbet,
linux-kernel, Michal Marek
In-Reply-To: <20190627014617.600-3-yamada.masahiro@socionext.com>
On Thu, 27 Jun 2019, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> header-test-y does not work with headers in sub-directories.
>
> For example, you can write a Makefile, like this:
>
> include/linux/Kbuild:
>
> header-test-y += mtd/nand.h
>
> This entry creates a wrapper include/linux/mtd/nand.hdrtest.c with
> the following content:
>
> #include "mtd/nand.h"
>
> To make this work, we need to add $(srctree)/include/linux to the
> header search path. It would be tedious to add ccflags-y.
>
> We could change the *.hdrtest.c rule to wrap:
>
> #include "nand.h"
>
> This works for in-tree build since #include "..." searches in the
> relative path from the header with this directive. For O=... build,
> we need to add $(srctree)/include/linux/mtd to the header search path,
> which will be even more tedious.
>
> After all, I thought it would be handier to compile headers directly
> without creating wrappers.
>
> I added a new build rule to compile %.h into %.h.s
>
> I chose %.h.s instead of %.h.o because it was a little bit faster.
> Also, for GCC, an empty assembly is smaller than an empty object.
>
> I wrote the build rule:
>
> $(CC) $(c_flags) -S -o $@ -x c /dev/null -include $<
>
> instead of:
>
> $(CC) $(c_flags) -S -o $@ -x c $<
>
> Both work fine with GCC, but the latter is not good for Clang.
>
> This comes down to the difference in the -Wunused-function policy.
> GCC does not warn about unused 'static inline' functions at all.
> Clang does not warn about the ones in included headers, but does
> about the ones in the source. So, we should handle headers as
> headers, not as source files.
>
> In fact, this has been hidden since commit abb2ea7dfd82 ("compiler,
> clang: suppress warning for unused static inline functions"), but we
> should not rely on that.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v2:
> - New patch
>
> .gitignore | 1 -
> Documentation/dontdiff | 1 -
> Documentation/kbuild/makefiles.txt | 3 +--
> Makefile | 1 -
> scripts/Makefile.build | 10 +++++-----
> scripts/Makefile.lib | 2 +-
> 6 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index 4bb60f0fa23b..7587ef56b92d 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -22,7 +22,6 @@
> *.elf
> *.gcno
> *.gz
> -*.hdrtest.c
> *.i
> *.ko
> *.lex.c
> diff --git a/Documentation/dontdiff b/Documentation/dontdiff
> index 554dfe4883d2..5eba889ea84d 100644
> --- a/Documentation/dontdiff
> +++ b/Documentation/dontdiff
> @@ -19,7 +19,6 @@
> *.grep
> *.grp
> *.gz
> -*.hdrtest.c
> *.html
> *.i
> *.jpeg
> diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
> index ca4b24ec0399..5080fec34609 100644
> --- a/Documentation/kbuild/makefiles.txt
> +++ b/Documentation/kbuild/makefiles.txt
> @@ -1023,8 +1023,7 @@ When kbuild executes, the following steps are followed (roughly):
> header-test-y specifies headers (*.h) in the current directory that
> should be compile tested to ensure they are self-contained,
> i.e. compilable as standalone units. If CONFIG_HEADER_TEST is enabled,
> - this autogenerates dummy sources to include the headers, and builds them
> - as part of extra-y.
> + this builds them as part of extra-y.
>
> --- 6.7 Commands useful for building a boot image
>
> diff --git a/Makefile b/Makefile
> index f23516980796..7f293b0431fe 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1648,7 +1648,6 @@ clean: $(clean-dirs)
> -o -name '*.dwo' -o -name '*.lst' \
> -o -name '*.su' \
> -o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
> - -o -name '*.hdrtest.c' \
> -o -name '*.lex.c' -o -name '*.tab.[ch]' \
> -o -name '*.asn1.[ch]' \
> -o -name '*.symtypes' -o -name 'modules.order' \
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index ee0319560513..776842b7e6a3 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -294,14 +294,14 @@ quiet_cmd_cc_lst_c = MKLST $@
> $(obj)/%.lst: $(src)/%.c FORCE
> $(call if_changed_dep,cc_lst_c)
>
> -# Dummy C sources for header test (header-test-y target)
> +# header test (header-test-y target)
> # ---------------------------------------------------------------------------
>
> -quiet_cmd_header_test = HDRTEST $@
> - cmd_header_test = echo "\#include \"$*.h\"" > $@
> +quiet_cmd_cc_s_h = CC $@
> + cmd_cc_s_h = $(CC) $(c_flags) -S -o $@ -x c /dev/null -include $<
I think I'd prefer HDRTEST or something more informative than just CC in
the quiet cmd to distinguish this from the usual build lines. Especially
now that the file name is also just .h.s.
Other than that, good job getting rid of the intermediate file. I
couldn't figure it out when I did the original.
Acked-by: Jani Nikula <jani.nikula@intel.com>
Tested-by: Jani Nikula <jani.nikula@intel.com>
>
> -$(obj)/%.hdrtest.c:
> - $(call cmd,header_test)
> +$(obj)/%.h.s: $(src)/%.h FORCE
> + $(call if_changed_dep,cc_s_h)
>
> # Compile assembler sources (.S)
> # ---------------------------------------------------------------------------
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 3e630fcaffd1..55ae1ec65342 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -67,7 +67,7 @@ extra-$(CONFIG_OF_ALL_DTBS) += $(patsubst %.dtb,%.dt.yaml, $(dtb-))
> endif
>
> # Test self-contained headers
> -extra-$(CONFIG_HEADER_TEST) += $(patsubst %.h,%.hdrtest.o,$(header-test-y))
> +extra-$(CONFIG_HEADER_TEST) += $(addsuffix .s, $(header-test-y))
>
> # Add subdir path
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH 12/14] doc-rst: add ABI documentation to the admin-guide book
From: Mauro Carvalho Chehab @ 2019-06-27 10:52 UTC (permalink / raw)
To: Jani Nikula
Cc: Greg Kroah-Hartman, Linux Doc Mailing List, linux-kernel,
Jonathan Corbet
In-Reply-To: <87blyjqrz7.fsf@intel.com>
Em Thu, 27 Jun 2019 12:48:12 +0300
Jani Nikula <jani.nikula@linux.intel.com> escreveu:
> On Fri, 21 Jun 2019, Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> > Em Wed, 19 Jun 2019 13:37:39 -0300
> > Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:
> >
> >> Em Tue, 18 Jun 2019 11:47:32 +0300
> >> Jani Nikula <jani.nikula@linux.intel.com> escreveu:
> >>
> >> > On Mon, 17 Jun 2019, Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> >> > > Yeah, I guess it should be possible to do that. How a python script
> >> > > can identify if it was called by Sphinx, or if it was called directly?
> >> >
> >> > if __name__ == '__main__':
> >> > # run on the command-line, not imported
> >>
> >> Ok, when I have some spare time, I may try to convert one script
> >> to python and see how it behaves.
> >
> > Did a quick test here...
> >
> > Probably I'm doing something wrong (as I'm a rookie with Python), but,
> > in order to be able to use the same script as command line and as an Sphinx
> > extension, everything that it is currently there should be "escaped"
> > by an:
> >
> > if __name__ != '__main__':
> >
> > As event the class definition:
> >
> > class KernelCmd(Directive):
> >
> > depends on:
> >
> > from docutils.parsers.rst import directives, Directive
> >
> > With is only required when one needs to parse ReST - e. g. only
> > when the script runs as a Sphinx extension.
> >
> > If this is right, as we want a script that can run by command line
> > to parse and search inside ABI files, at the end of the day, it will
> > be a lot easier to maintain if the parser script is different from the
> > Sphinx extension.
>
> Split it into two files, one has the nuts and bolts of parsing and has
> the "if __name__ == '__main__':" bit to run on the command line, and the
> other interfaces with Sphinx and imports the parser.
It seems we have an agreement here: the best is indeed to have two
files, one with the Documentation/ABI parser, and another one with the
Sphinx extension...
>
> > If so, as the Sphinx extension script will need to call a parsing script
> > anyway, it doesn't matter the language of the script with will be
> > doing the ABI file parsing.
>
> Calling the parser using an API will be easier to use, maintain and
> extend than using pipes, with all the interleaved sideband information
> to adjust line numbers and whatnot.
... and here is where we have two different views.
From debug PoV, the Documentation/ABI parser script should be able to
print the results by a command line call. This is also a feature
that it is useful for the users: to be able to seek for a symbol
and output its ABI description. So, the "stdout" output will be
there anyway.
The only extra data for the extension side is the file name where
the information came and the line number.
From maintainership PoV, adding the sideband API for file+line is
one line at the parser script (a print) and two lines at the Sphinx
extension (a regex expression and a match line). That's simple to
maintain.
It is also simple to verify both sides independently, as what
you see when running the parser script is what you get at the
extension.
If we add a new ABI between the parser script and the extension
script, this would require to also maintain the ABI, and would
make harder to identify problems on ABI problems.
-
Another advantage of having those independent is that the
language of the parsing script can be different. Not being
python is a big advantage for me, as perl is a lot more
intuitive and easier to write parser scripts for my eyes.
I can write a perl parsing script in a matter of minutes.
It takes me a lot more time to do the same with python, and then
ensure that it will work with two similar but different languages
(python2 and python3) [1].
[1] btw, with that regards, I still don't know how to teach a
python script that it should "prefer" to run with python3 but would
fall back to python2. Adding this shebang:
# /usr/bin/env python
just do the opposite - at least on Fedora
Thanks,
Mauro
^ permalink raw reply
* Re: [PATCH 0/2] arm64: Introduce boot parameter to disable TLB flush instruction within the same inner shareable domain
From: Will Deacon @ 2019-06-27 10:27 UTC (permalink / raw)
To: qi.fuli@fujitsu.com
Cc: Will Deacon, indou.takao@fujitsu.com, linux-doc@vger.kernel.org,
peterz@infradead.org, Catalin Marinas, Jonathan Corbet,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <e8fe8faa-72ef-8185-1a9d-dc1bbe0ae15d@jp.fujitsu.com>
On Mon, Jun 24, 2019 at 10:34:02AM +0000, qi.fuli@fujitsu.com wrote:
> On 6/18/19 2:03 AM, Will Deacon wrote:
> > On Mon, Jun 17, 2019 at 11:32:53PM +0900, Takao Indoh wrote:
> >> From: Takao Indoh <indou.takao@fujitsu.com>
> >>
> >> I found a performance issue related on the implementation of Linux's TLB
> >> flush for arm64.
> >>
> >> When I run a single-threaded test program on moderate environment, it
> >> usually takes 39ms to finish its work. However, when I put a small
> >> apprication, which just calls mprotest() continuously, on one of sibling
> >> cores and run it simultaneously, the test program slows down significantly.
> >> It becomes 49ms(125%) on ThunderX2. I also detected the same problem on
> >> ThunderX1 and Fujitsu A64FX.
> > This is a problem for any applications that share hardware resources with
> > each other, so I don't think it's something we should be too concerned about
> > addressing unless there is a practical DoS scenario, which there doesn't
> > appear to be in this case. It may be that the real answer is "don't call
> > mprotect() in a loop".
> I think there has been a misunderstanding, please let me explain.
> This application is just an example using for reproducing the
> performance issue we found.
> Our original purpose is reducing OS jitter by this series.
> The OS jitter on massively parallel processing systems have been known
> and studied for many years.
> The 2.5% OS jitter can result in over a factor of 20 slowdown for the
> same application [1].
I think it's worth pointing out that the system in question was neither
ARM-based nor running Linux, so I'd be cautious in applying the conclusions
of that paper directly to our TLB invalidation code. Furthermore, the noise
being generated in their experiments uses a timer interrupt, which has a
/vastly/ different profile to a DVM message in terms of both system impact
and frequency.
> Though it may be an extreme example, reducing the OS jitter has been an
> issue in HPC environment.
>
> [1] Ferreira, Kurt B., Patrick Bridges, and Ron Brightwell.
> "Characterizing application sensitivity to OS interference using
> kernel-level noise injection." Proceedings of the 2008 ACM/IEEE
> conference on Supercomputing. IEEE Press, 2008.
>
> >> I suppose the root cause of this issue is the implementation of Linux's TLB
> >> flush for arm64, especially use of TLBI-is instruction which is a broadcast
> >> to all processor core on the system. In case of the above situation,
> >> TLBI-is is called by mprotect().
> > On the flip side, Linux is providing the hardware with enough information
> > not to broadcast to cores for which the remote TLBs don't have entries
> > allocated for the ASID being invalidated. I would say that the root cause
> > of the issue is that this filtering is not taking place.
>
> Do you mean that the filter should be implemented in hardware?
Yes. If you're building a large system and you care about "jitter", then
you either need to partition it in such a way that sources of noise are
contained, or you need to introduce filters to limit their scope. Rewriting
the low-level memory-management parts of the operating system is a red
herring and imposes a needless burden on everybody else without solving
the real problem, which is that contended use of shared resources doesn't
scale.
Will
^ permalink raw reply
* Re: [PATCH 1/2] Documentation: perf: Update documentation for ThunderX2 PMU uncore driver
From: Will Deacon @ 2019-06-27 10:01 UTC (permalink / raw)
To: Ganapatrao Kulkarni
Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Will.Deacon@arm.com,
mark.rutland@arm.com, corbet@lwn.net, jnair@caviumnetworks.com,
Robert.Richter@cavium.com, Jan.Glauber@cavium.com,
gklkml16@gmail.com
In-Reply-To: <1560534144-13896-2-git-send-email-gkulkarni@marvell.com>
On Fri, Jun 14, 2019 at 05:42:45PM +0000, Ganapatrao Kulkarni wrote:
> From: Ganapatrao Kulkarni <ganapatrao.kulkarni@marvell.com>
>
> Add documentation for Cavium Coherent Processor Interconnect (CCPI2) PMU.
>
> Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> ---
> Documentation/perf/thunderx2-pmu.txt | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/perf/thunderx2-pmu.txt b/Documentation/perf/thunderx2-pmu.txt
> index dffc57143736..62243230abc3 100644
> --- a/Documentation/perf/thunderx2-pmu.txt
> +++ b/Documentation/perf/thunderx2-pmu.txt
> @@ -2,24 +2,26 @@ Cavium ThunderX2 SoC Performance Monitoring Unit (PMU UNCORE)
> =============================================================
>
> The ThunderX2 SoC PMU consists of independent, system-wide, per-socket
> -PMUs such as the Level 3 Cache (L3C) and DDR4 Memory Controller (DMC).
> +PMUs such as the Level 3 Cache (L3C), DDR4 Memory Controller (DMC) and
> +Cavium Coherent Processor Interconnect (CCPI2).
>
> The DMC has 8 interleaved channels and the L3C has 16 interleaved tiles.
> Events are counted for the default channel (i.e. channel 0) and prorated
> to the total number of channels/tiles.
>
> -The DMC and L3C support up to 4 counters. Counters are independently
> -programmable and can be started and stopped individually. Each counter
> -can be set to a different event. Counters are 32-bit and do not support
> -an overflow interrupt; they are read every 2 seconds.
> +The DMC, L3C support up to 4 counters and CCPI2 support up to 8 counters.
The DMC and L3C support up to 4 counters, while the CCPI2 supports up to 8
counters.
> +Counters are independently programmable and can be started and stopped
> +individually. Each counter can be set to a different event. DMC and L3C
> +Counters are 32-bit and do not support an overflow interrupt; they are read
Counters -> counters
> +every 2 seconds. CCPI2 counters are 64-bit.
Assuming CCPI2 also doesn't support an overflow interrupt, I'd reword these
two sentences as:
None of the counters support an overflow interrupt and therefore sampling
events are unsupported. The DMC and L3C counters are 32-bit and read every
2 seconds. The CCPI2 counters are 64-bit and assumed not to overflow in
normal operation.
> PMU UNCORE (perf) driver:
>
> The thunderx2_pmu driver registers per-socket perf PMUs for the DMC and
> -L3C devices. Each PMU can be used to count up to 4 events
> -simultaneously. The PMUs provide a description of their available events
> -and configuration options under sysfs, see
> -/sys/devices/uncore_<l3c_S/dmc_S/>; S is the socket id.
> +L3C devices. Each PMU can be used to count up to 4(DMC/L3C) or up to 8
Space between 4 and (
Will
^ permalink raw reply
* Re: [PATCH 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
From: Will Deacon @ 2019-06-27 9:57 UTC (permalink / raw)
To: Ganapatrao Kulkarni
Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Will.Deacon@arm.com,
mark.rutland@arm.com, corbet@lwn.net, jnair@caviumnetworks.com,
Robert.Richter@cavium.com, Jan.Glauber@cavium.com,
gklkml16@gmail.com
In-Reply-To: <1560534144-13896-3-git-send-email-gkulkarni@marvell.com>
Hi Ganapat,
On Fri, Jun 14, 2019 at 05:42:46PM +0000, Ganapatrao Kulkarni wrote:
> CCPI2 is a low-latency high-bandwidth serial interface for connecting
> ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
>
> Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> ---
> drivers/perf/thunderx2_pmu.c | 179 ++++++++++++++++++++++++++++++-----
> 1 file changed, 157 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> index 43d76c85da56..3791ac9b897d 100644
> --- a/drivers/perf/thunderx2_pmu.c
> +++ b/drivers/perf/thunderx2_pmu.c
> @@ -16,7 +16,9 @@
> * they need to be sampled before overflow(i.e, at every 2 seconds).
> */
>
> -#define TX2_PMU_MAX_COUNTERS 4
> +#define TX2_PMU_DMC_L3C_MAX_COUNTERS 4
I find it odd that you rename this...
> +#define TX2_PMU_CCPI2_MAX_COUNTERS 8
> +
> #define TX2_PMU_DMC_CHANNELS 8
> #define TX2_PMU_L3_TILES 16
>
> @@ -28,11 +30,22 @@
> */
> #define DMC_EVENT_CFG(idx, val) ((val) << (((idx) * 8) + 1))
>
> +#define GET_EVENTID_CCPI2(ev) ((ev->hw.config) & 0x1ff)
> +/* bits[3:0] to select counters, starts from 8, bit[3] set always. */
> +#define GET_COUNTERID_CCPI2(ev) ((ev->hw.idx) & 0x7)
> +#define CCPI2_COUNTER_OFFSET 8
... but leave GET_EVENTID alone, even though it only applies to DMC/L3C
events. Saying that, if you have the event you can figure out its type,
so could you avoid the need for additional macros entirely and just use
the correct masks based on the corresponding PMU type? That might also
avoid some of the conditional control flow you're introducing elsewhere.
> #define L3C_COUNTER_CTL 0xA8
> #define L3C_COUNTER_DATA 0xAC
> #define DMC_COUNTER_CTL 0x234
> #define DMC_COUNTER_DATA 0x240
>
> +#define CCPI2_PERF_CTL 0x108
> +#define CCPI2_COUNTER_CTL 0x10C
> +#define CCPI2_COUNTER_SEL 0x12c
> +#define CCPI2_COUNTER_DATA_L 0x130
> +#define CCPI2_COUNTER_DATA_H 0x134
> +
> /* L3C event IDs */
> #define L3_EVENT_READ_REQ 0xD
> #define L3_EVENT_WRITEBACK_REQ 0xE
> @@ -51,9 +64,16 @@
> #define DMC_EVENT_READ_TXNS 0xF
> #define DMC_EVENT_MAX 0x10
>
> +#define CCPI2_EVENT_REQ_PKT_SENT 0x3D
> +#define CCPI2_EVENT_SNOOP_PKT_SENT 0x65
> +#define CCPI2_EVENT_DATA_PKT_SENT 0x105
> +#define CCPI2_EVENT_GIC_PKT_SENT 0x12D
> +#define CCPI2_EVENT_MAX 0x200
> +
> enum tx2_uncore_type {
> PMU_TYPE_L3C,
> PMU_TYPE_DMC,
> + PMU_TYPE_CCPI2,
> PMU_TYPE_INVALID,
> };
>
> @@ -73,8 +93,8 @@ struct tx2_uncore_pmu {
> u32 max_events;
> u64 hrtimer_interval;
> void __iomem *base;
> - DECLARE_BITMAP(active_counters, TX2_PMU_MAX_COUNTERS);
> - struct perf_event *events[TX2_PMU_MAX_COUNTERS];
> + DECLARE_BITMAP(active_counters, TX2_PMU_CCPI2_MAX_COUNTERS);
> + struct perf_event *events[TX2_PMU_DMC_L3C_MAX_COUNTERS];
Hmm, that looks very odd. Why are they now different sizes?
> struct device *dev;
> struct hrtimer hrtimer;
> const struct attribute_group **attr_groups;
> @@ -92,7 +112,21 @@ static inline struct tx2_uncore_pmu *pmu_to_tx2_pmu(struct pmu *pmu)
> return container_of(pmu, struct tx2_uncore_pmu, pmu);
> }
>
> -PMU_FORMAT_ATTR(event, "config:0-4");
> +#define TX2_PMU_FORMAT_ATTR(_var, _name, _format) \
> +static ssize_t \
> +__tx2_pmu_##_var##_show(struct device *dev, \
> + struct device_attribute *attr, \
> + char *page) \
> +{ \
> + BUILD_BUG_ON(sizeof(_format) >= PAGE_SIZE); \
> + return sprintf(page, _format "\n"); \
> +} \
> + \
> +static struct device_attribute format_attr_##_var = \
> + __ATTR(_name, 0444, __tx2_pmu_##_var##_show, NULL)
> +
> +TX2_PMU_FORMAT_ATTR(event, event, "config:0-4");
> +TX2_PMU_FORMAT_ATTR(event_ccpi2, event, "config:0-9");
This doesn't look right. Can perf deal with overlapping fields? It seems
wrong that we'd allow the user to specify both, for example.
>
> static struct attribute *l3c_pmu_format_attrs[] = {
> &format_attr_event.attr,
> @@ -104,6 +138,11 @@ static struct attribute *dmc_pmu_format_attrs[] = {
> NULL,
> };
>
> +static struct attribute *ccpi2_pmu_format_attrs[] = {
> + &format_attr_event_ccpi2.attr,
> + NULL,
> +};
> +
> static const struct attribute_group l3c_pmu_format_attr_group = {
> .name = "format",
> .attrs = l3c_pmu_format_attrs,
> @@ -114,6 +153,11 @@ static const struct attribute_group dmc_pmu_format_attr_group = {
> .attrs = dmc_pmu_format_attrs,
> };
>
> +static const struct attribute_group ccpi2_pmu_format_attr_group = {
> + .name = "format",
> + .attrs = ccpi2_pmu_format_attrs,
> +};
> +
> /*
> * sysfs event attributes
> */
> @@ -164,6 +208,19 @@ static struct attribute *dmc_pmu_events_attrs[] = {
> NULL,
> };
>
> +TX2_EVENT_ATTR(req_pktsent, CCPI2_EVENT_REQ_PKT_SENT);
> +TX2_EVENT_ATTR(snoop_pktsent, CCPI2_EVENT_SNOOP_PKT_SENT);
> +TX2_EVENT_ATTR(data_pktsent, CCPI2_EVENT_DATA_PKT_SENT);
> +TX2_EVENT_ATTR(gic_pktsent, CCPI2_EVENT_GIC_PKT_SENT);
> +
> +static struct attribute *ccpi2_pmu_events_attrs[] = {
> + &tx2_pmu_event_attr_req_pktsent.attr.attr,
> + &tx2_pmu_event_attr_snoop_pktsent.attr.attr,
> + &tx2_pmu_event_attr_data_pktsent.attr.attr,
> + &tx2_pmu_event_attr_gic_pktsent.attr.attr,
> + NULL,
> +};
> +
> static const struct attribute_group l3c_pmu_events_attr_group = {
> .name = "events",
> .attrs = l3c_pmu_events_attrs,
> @@ -174,6 +231,11 @@ static const struct attribute_group dmc_pmu_events_attr_group = {
> .attrs = dmc_pmu_events_attrs,
> };
>
> +static const struct attribute_group ccpi2_pmu_events_attr_group = {
> + .name = "events",
> + .attrs = ccpi2_pmu_events_attrs,
> +};
> +
> /*
> * sysfs cpumask attributes
> */
> @@ -213,6 +275,13 @@ static const struct attribute_group *dmc_pmu_attr_groups[] = {
> NULL
> };
>
> +static const struct attribute_group *ccpi2_pmu_attr_groups[] = {
> + &ccpi2_pmu_format_attr_group,
> + &pmu_cpumask_attr_group,
> + &ccpi2_pmu_events_attr_group,
> + NULL
> +};
> +
> static inline u32 reg_readl(unsigned long addr)
> {
> return readl((void __iomem *)addr);
> @@ -265,6 +334,17 @@ static void init_cntr_base_dmc(struct perf_event *event,
> + DMC_COUNTER_DATA + (0xc * GET_COUNTERID(event));
> }
>
> +static void init_cntr_base_ccpi2(struct perf_event *event,
> + struct tx2_uncore_pmu *tx2_pmu)
> +{
> +
> + struct hw_perf_event *hwc = &event->hw;
> +
> + hwc->config_base = (unsigned long)tx2_pmu->base
> + + CCPI2_COUNTER_CTL + (4 * GET_COUNTERID_CCPI2(event));
> + hwc->event_base = (unsigned long)tx2_pmu->base;
> +}
> +
> static void uncore_start_event_l3c(struct perf_event *event, int flags)
> {
> u32 val;
> @@ -312,6 +392,29 @@ static void uncore_stop_event_dmc(struct perf_event *event)
> reg_writel(val, hwc->config_base);
> }
>
> +static void uncore_start_event_ccpi2(struct perf_event *event, int flags)
> +{
> + u32 val;
> + struct hw_perf_event *hwc = &event->hw;
> +
> + /* Bit [09:00] to set event id, set level and type to 1 */
> + val = reg_readl(hwc->config_base);
> + reg_writel((val & ~0xFFF) | (3 << 10) |
> + GET_EVENTID_CCPI2(event), hwc->config_base);
> + /* reset[4], enable[0] and start[1] counters */
> + reg_writel(0x13, hwc->event_base + CCPI2_PERF_CTL);
> + local64_set(&event->hw.prev_count, 0ULL);
> +}
> +
> +static void uncore_stop_event_ccpi2(struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> +
> + /* disable and stop counter */
> + reg_writel(0, hwc->event_base + CCPI2_PERF_CTL);
How come you need to clear the event register here? You don't do that for
the DMC/L3C paths.
> + reg_writel(0, hwc->config_base);
When starting event you're careful to update this using a read-modify-write
sequence. Why is it safe to zero the whole thing when stopping?
> +}
> +
> static void tx2_uncore_event_update(struct perf_event *event)
> {
> s64 prev, delta, new = 0;
> @@ -323,12 +426,20 @@ static void tx2_uncore_event_update(struct perf_event *event)
> tx2_pmu = pmu_to_tx2_pmu(event->pmu);
> type = tx2_pmu->type;
> prorate_factor = tx2_pmu->prorate_factor;
> -
> - new = reg_readl(hwc->event_base);
> - prev = local64_xchg(&hwc->prev_count, new);
> -
> - /* handles rollover of 32 bit counter */
> - delta = (u32)(((1UL << 32) - prev) + new);
> + if (type == PMU_TYPE_CCPI2) {
> + reg_writel(CCPI2_COUNTER_OFFSET + GET_COUNTERID_CCPI2(event),
> + hwc->event_base + CCPI2_COUNTER_SEL);
> + new = reg_readl(hwc->event_base + CCPI2_COUNTER_DATA_L);
> + new |= (u64)reg_readl(hwc->event_base +
> + CCPI2_COUNTER_DATA_H) << 32;
Can you not access the event register using a 64-bit read?
> + prev = local64_xchg(&hwc->prev_count, new);
> + delta = new - prev;
> + } else {
> + new = reg_readl(hwc->event_base);
> + prev = local64_xchg(&hwc->prev_count, new);
> + /* handles rollover of 32 bit counter */
> + delta = (u32)(((1UL << 32) - prev) + new);
> + }
>
> /* DMC event data_transfers granularity is 16 Bytes, convert it to 64 */
> if (type == PMU_TYPE_DMC &&
> @@ -351,6 +462,7 @@ static enum tx2_uncore_type get_tx2_pmu_type(struct acpi_device *adev)
> } devices[] = {
> {"CAV901D", PMU_TYPE_L3C},
> {"CAV901F", PMU_TYPE_DMC},
> + {"CAV901E", PMU_TYPE_CCPI2},
> {"", PMU_TYPE_INVALID}
> };
>
> @@ -380,7 +492,8 @@ static bool tx2_uncore_validate_event(struct pmu *pmu,
> * Make sure the group of events can be scheduled at once
> * on the PMU.
> */
> -static bool tx2_uncore_validate_event_group(struct perf_event *event)
> +static bool tx2_uncore_validate_event_group(struct perf_event *event,
> + int max_counters)
> {
> struct perf_event *sibling, *leader = event->group_leader;
> int counters = 0;
> @@ -403,7 +516,7 @@ static bool tx2_uncore_validate_event_group(struct perf_event *event)
> * If the group requires more counters than the HW has,
> * it cannot ever be scheduled.
> */
> - return counters <= TX2_PMU_MAX_COUNTERS;
> + return counters <= max_counters;
> }
>
>
> @@ -439,7 +552,7 @@ static int tx2_uncore_event_init(struct perf_event *event)
> hwc->config = event->attr.config;
>
> /* Validate the group */
> - if (!tx2_uncore_validate_event_group(event))
> + if (!tx2_uncore_validate_event_group(event, tx2_pmu->max_counters))
> return -EINVAL;
>
> return 0;
> @@ -457,7 +570,8 @@ static void tx2_uncore_event_start(struct perf_event *event, int flags)
> perf_event_update_userpage(event);
>
> /* Start timer for first event */
> - if (bitmap_weight(tx2_pmu->active_counters,
> + if (tx2_pmu->type != PMU_TYPE_CCPI2 &&
> + bitmap_weight(tx2_pmu->active_counters,
> tx2_pmu->max_counters) == 1) {
> hrtimer_start(&tx2_pmu->hrtimer,
> ns_to_ktime(tx2_pmu->hrtimer_interval),
> @@ -495,7 +609,8 @@ static int tx2_uncore_event_add(struct perf_event *event, int flags)
> if (hwc->idx < 0)
> return -EAGAIN;
>
> - tx2_pmu->events[hwc->idx] = event;
> + if (tx2_pmu->type != PMU_TYPE_CCPI2)
> + tx2_pmu->events[hwc->idx] = event;
> /* set counter control and data registers base address */
> tx2_pmu->init_cntr_base(event, tx2_pmu);
>
> @@ -514,10 +629,14 @@ static void tx2_uncore_event_del(struct perf_event *event, int flags)
> tx2_uncore_event_stop(event, PERF_EF_UPDATE);
>
> /* clear the assigned counter */
> - free_counter(tx2_pmu, GET_COUNTERID(event));
> + if (tx2_pmu->type == PMU_TYPE_CCPI2)
> + free_counter(tx2_pmu, GET_COUNTERID_CCPI2(event));
> + else
> + free_counter(tx2_pmu, GET_COUNTERID(event));
>
> perf_event_update_userpage(event);
> - tx2_pmu->events[hwc->idx] = NULL;
> + if (tx2_pmu->type != PMU_TYPE_CCPI2)
> + tx2_pmu->events[hwc->idx] = NULL;
> hwc->idx = -1;
> }
>
> @@ -580,8 +699,12 @@ static int tx2_uncore_pmu_add_dev(struct tx2_uncore_pmu *tx2_pmu)
> cpu_online_mask);
>
> tx2_pmu->cpu = cpu;
> - hrtimer_init(&tx2_pmu->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> - tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> + /* CCPI2 counters are 64 bit counters. */
> + if (tx2_pmu->type != PMU_TYPE_CCPI2) {
> + hrtimer_init(&tx2_pmu->hrtimer,
> + CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> + }
So I take it the CCPI2 also doesn't have an IRQ, and therefore you can't
hook up sampling events?
Will
^ permalink raw reply
* Re: [PATCH 12/14] doc-rst: add ABI documentation to the admin-guide book
From: Jani Nikula @ 2019-06-27 9:48 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Greg Kroah-Hartman, Linux Doc Mailing List, linux-kernel,
Jonathan Corbet
In-Reply-To: <20190621112700.6922d80d@coco.lan>
On Fri, 21 Jun 2019, Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
> Em Wed, 19 Jun 2019 13:37:39 -0300
> Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:
>
>> Em Tue, 18 Jun 2019 11:47:32 +0300
>> Jani Nikula <jani.nikula@linux.intel.com> escreveu:
>>
>> > On Mon, 17 Jun 2019, Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote:
>> > > Yeah, I guess it should be possible to do that. How a python script
>> > > can identify if it was called by Sphinx, or if it was called directly?
>> >
>> > if __name__ == '__main__':
>> > # run on the command-line, not imported
>>
>> Ok, when I have some spare time, I may try to convert one script
>> to python and see how it behaves.
>
> Did a quick test here...
>
> Probably I'm doing something wrong (as I'm a rookie with Python), but,
> in order to be able to use the same script as command line and as an Sphinx
> extension, everything that it is currently there should be "escaped"
> by an:
>
> if __name__ != '__main__':
>
> As event the class definition:
>
> class KernelCmd(Directive):
>
> depends on:
>
> from docutils.parsers.rst import directives, Directive
>
> With is only required when one needs to parse ReST - e. g. only
> when the script runs as a Sphinx extension.
>
> If this is right, as we want a script that can run by command line
> to parse and search inside ABI files, at the end of the day, it will
> be a lot easier to maintain if the parser script is different from the
> Sphinx extension.
Split it into two files, one has the nuts and bolts of parsing and has
the "if __name__ == '__main__':" bit to run on the command line, and the
other interfaces with Sphinx and imports the parser.
> If so, as the Sphinx extension script will need to call a parsing script
> anyway, it doesn't matter the language of the script with will be
> doing the ABI file parsing.
Calling the parser using an API will be easier to use, maintain and
extend than using pipes, with all the interleaved sideband information
to adjust line numbers and whatnot.
BR,
Jani.
>
> See the enclosing file. I didn't add anything from the ABI parsing
> script yet. It was just changed in order to not generate an error
> when trying to run it from command line.
>
>
> Thanks,
> Mauro
> #!/usr/bin/env python3
> # coding=utf-8
> # SPDX-License-Identifier: GPL-2.0
> #
> u"""
> kernel-abi
> ~~~~~~~~~~
>
> Implementation of the ``kernel-abi`` reST-directive.
>
> :copyright: Copyright (C) 2016 Markus Heiser
> :copyright: Copyright (C) 2016-2019 Mauro Carvalho Chehab
> :maintained-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> :license: GPL Version 2, June 1991 see Linux/COPYING for details.
>
> The ``kernel-abi`` (:py:class:`KernelCmd`) directive calls the
> scripts/get_abi.pl script to parse the Kernel ABI files.
>
> Overview of directive's argument and options.
>
> .. code-block:: rst
>
> .. kernel-abi:: <ABI directory location>
> :debug:
>
> The argument ``<ABI directory location>`` is required. It contains the
> location of the ABI files to be parsed.
>
> ``debug``
> Inserts a code-block with the *raw* reST. Sometimes it is helpful to see
> what reST is generated.
>
> """
>
> import codecs
> import os
> import subprocess
> import sys
>
> from os import path
>
> if __name__ != '__main__':
> from docutils import nodes, statemachine
> from docutils.statemachine import ViewList
> from docutils.parsers.rst import directives, Directive
> from docutils.utils.error_reporting import ErrorString
>
> #
> # AutodocReporter is only good up to Sphinx 1.7
> #
> import sphinx
>
> Use_SSI = sphinx.__version__[:3] >= '1.7'
> if Use_SSI:
> from sphinx.util.docutils import switch_source_input
> else:
> from sphinx.ext.autodoc import AutodocReporter
>
> __version__ = '1.0'
>
> if __name__ != '__main__':
> def setup(app):
>
> app.add_directive("kernel-abi", KernelCmd)
> return dict(
> version = __version__
> , parallel_read_safe = True
> , parallel_write_safe = True
> )
>
> class KernelCmd(Directive):
>
> u"""KernelABI (``kernel-abi``) directive"""
>
> required_arguments = 1
> optional_arguments = 2
> has_content = False
> final_argument_whitespace = True
>
> option_spec = {
> "debug" : directives.flag,
> "rst" : directives.unchanged
> }
>
> def warn(self, message, **replace):
> replace["fname"] = self.state.document.current_source
> replace["line_no"] = replace.get("line_no", self.lineno)
> message = ("%(fname)s:%(line_no)s: [kernel-abi WARN] : " + message) % replace
> self.state.document.settings.env.app.warn(message, prefix="")
>
> def run(self):
>
> doc = self.state.document
> if not doc.settings.file_insertion_enabled:
> raise self.warning("docutils: file insertion disabled")
>
> env = doc.settings.env
> cwd = path.dirname(doc.current_source)
> fname = self.arguments[0]
>
> cmd = "get_abi.pl rest --dir "
> cmd += fname
>
> if 'rst' in self.options:
> cmd += " --rst-source"
>
> srctree = path.abspath(os.environ["srctree"])
>
> # extend PATH with $(srctree)/scripts
> path_env = os.pathsep.join([
> srctree + os.sep + "scripts",
> os.environ["PATH"]
> ])
> shell_env = os.environ.copy()
> shell_env["PATH"] = path_env
> shell_env["srctree"] = srctree
>
> lines = self.runCmd(cmd, shell=True, cwd=cwd, env=shell_env)
> nodeList = self.nestedParse(lines, self.arguments[0])
> return nodeList
>
> def runCmd(self, cmd, **kwargs):
> u"""Run command ``cmd`` and return it's stdout as unicode."""
>
> try:
> proc = subprocess.Popen(
> cmd
> , stdout = subprocess.PIPE
> , stderr = subprocess.PIPE
> , **kwargs
> )
> out, err = proc.communicate()
>
> out, err = codecs.decode(out, 'utf-8'), codecs.decode(err, 'utf-8')
>
> if proc.returncode != 0:
> raise self.severe(
> u"command '%s' failed with return code %d"
> % (cmd, proc.returncode)
> )
> except OSError as exc:
> raise self.severe(u"problems with '%s' directive: %s."
> % (self.name, ErrorString(exc)))
> return out
>
> def nestedParse(self, lines, fname):
> content = ViewList()
> node = nodes.section()
>
> if "debug" in self.options:
> code_block = "\n\n.. code-block:: rst\n :linenos:\n"
> for l in lines.split("\n"):
> code_block += "\n " + l
> lines = code_block + "\n\n"
>
> for c, l in enumerate(lines.split("\n")):
> content.append(l, fname, c)
>
> buf = self.state.memo.title_styles, self.state.memo.section_level, self.state.memo.reporter
>
> if Use_SSI:
> with switch_source_input(self.state, content):
> self.state.nested_parse(content, 0, node, match_titles=1)
> else:
> self.state.memo.title_styles = []
> self.state.memo.section_level = 0
> self.state.memo.reporter = AutodocReporter(content, self.state.memo.reporter)
> try:
> self.state.nested_parse(content, 0, node, match_titles=1)
> finally:
> self.state.memo.title_styles, self.state.memo.section_level, self.state.memo.reporter = buf
>
> return node.children
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Florian Weimer @ 2019-06-27 9:38 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Dave Martin, Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, open list:DOCUMENTATION, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Balbir Singh, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Szabolcs Nagy, libc-alpha
In-Reply-To: <CALCETrVZCzh+KFCF6ijuf4QEPn=R2gJ8FHLpyFd=n+pNOMMMjA@mail.gmail.com>
* Andy Lutomirski:
> Also, I don't think there's any actual requirement that the upstream
> kernel recognize existing CET-enabled RHEL 8 binaries as being
> CET-enabled. I tend to think that RHEL 8 jumped the gun here.
The ABI was supposed to be finalized and everyone involved thought it
had been reviewed by the GNU gABI community and other interested
parties. It had been included in binutils for several releases.
From my point of view, the kernel is just a consumer of the ABI. The
kernel would not change an instruction encoding if it doesn't like it
for some reason, either.
> While the upstream kernel should make some reasonble effort to make
> sure that RHEL 8 binaries will continue to run, I don't see why we
> need to go out of our way to keep the full set of mitigations
> available for binaries that were developed against a non-upstream
> kernel.
They were developed against the ABI specification.
I do not have a strong opinion what the kernel should do going forward.
I just want to make clear what happened.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Dave Martin @ 2019-06-27 9:27 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
LKML, open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Szabolcs Nagy, libc-alpha
In-Reply-To: <CALCETrVZCzh+KFCF6ijuf4QEPn=R2gJ8FHLpyFd=n+pNOMMMjA@mail.gmail.com>
On Wed, Jun 26, 2019 at 10:14:07AM -0700, Andy Lutomirski wrote:
> On Thu, May 2, 2019 at 4:10 AM Dave Martin <Dave.Martin@arm.com> wrote:
[...]
> > A couple of questions before I look in more detail:
> >
> > 1) Can we rely on PT_GNU_PROPERTY being present in the phdrs to describe
> > the NT_GNU_PROPERTY_TYPE_0 note? If so, we can avoid trying to parse
> > irrelevant PT_NOTE segments.
> >
> >
> > 2) Are there standard types for things like the program property header?
> > If not, can we add something in elf.h? We should try to coordinate with
> > libc on that. Something like
> >
>
> Where did PT_GNU_PROPERTY come from? Are there actual docs for it?
> Can someone here tell us what the actual semantics of this new ELF
> thingy are? From some searching, it seems like it's kind of an ELF
> note but kind of not. An actual description would be fantastic.
https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf
I don't know _when_ it was added, and the description is minimal, but
it's there.
(I'd say it's fairly obvious how it should be used, but it could do with
some clarification...)
> Also, I don't think there's any actual requirement that the upstream
> kernel recognize existing CET-enabled RHEL 8 binaries as being
> CET-enabled. I tend to think that RHEL 8 jumped the gun here. While
> the upstream kernel should make some reasonble effort to make sure
> that RHEL 8 binaries will continue to run, I don't see why we need to
> go out of our way to keep the full set of mitigations available for
> binaries that were developed against a non-upstream kernel.
If that's an accpetable approach, it should certainly make our life
easier.
> In fact, if we handle the legacy bitmap differently from RHEL 8, we
> may *have* to make sure that we don't recognize existing RHEL 8
> binaries as CET-enabled.
Can't comment on that. If the existing RHEL 8 binaries strictly don't
have the PT_GNU_PROPERTY phdr, then this might serve a dual purpose ...
otherwise, x86 might need some additional annotation for new binaries.
I'll leave it for others to comment.
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver
From: Paul Cercueil @ 2019-06-27 9:19 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Jonathan Corbet, Daniel Lezcano, Thomas Gleixner,
Michael Turquette, Stephen Boyd, Jason Cooper, Marc Zyngier,
Mathieu Malaterre, linux-kernel, devicetree, linux-mips,
linux-doc, linux-clk, od
In-Reply-To: <20190627090102.GA2000@dell>
Le jeu. 27 juin 2019 à 11:01, Lee Jones <lee.jones@linaro.org> a
écrit :
> On Thu, 27 Jun 2019, Paul Cercueil wrote:
>> Le jeu. 27 juin 2019 à 8:58, Lee Jones <lee.jones@linaro.org> a
>> écrit :
>> > On Wed, 26 Jun 2019, Paul Cercueil wrote:
>> > > Le mer. 26 juin 2019 à 15:18, Lee Jones
>> <lee.jones@linaro.org> a
>> > > écrit :
>> > > > On Tue, 21 May 2019, Paul Cercueil wrote:
>> > > >
>> > > > > This driver will provide a regmap that can be retrieved
>> very
>> > > early
>> > > > > in
>> > > > > the boot process through the API function
>> > > ingenic_tcu_get_regmap().
>> > > > >
>> > > > > Additionally, it will call devm_of_platform_populate() so
>> that
>> > > all
>> > > > > the
>> > > > > children devices will be probed.
>> > > > >
>> > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> > > > > ---
>> > > > >
>> > > > > Notes:
>> > > > > v12: New patch
>> > > > >
>> > > > > drivers/mfd/Kconfig | 8 +++
>> > > > > drivers/mfd/Makefile | 1 +
>> > > > > drivers/mfd/ingenic-tcu.c | 113
>> > > > > ++++++++++++++++++++++++++++++++
>> > > > > include/linux/mfd/ingenic-tcu.h | 8 +++
>> > > > > 4 files changed, 130 insertions(+)
>> > > > > create mode 100644 drivers/mfd/ingenic-tcu.c
>> >
>> > [...]
>> >
>> > > > > +static struct regmap * __init
>> ingenic_tcu_create_regmap(struct
>> > > > > device_node *np)
>> > > > > +{
>> > > > > + struct resource res;
>> > > > > + void __iomem *base;
>> > > > > + struct regmap *map;
>> > > > > +
>> > > > > + if (!of_match_node(ingenic_tcu_of_match, np))
>> > > > > + return ERR_PTR(-EINVAL);
>> >
>> > Drop this check.
>> >
>> > > > > + base = of_io_request_and_map(np, 0, "TCU");
>> > > > > + if (IS_ERR(base))
>> > > > > + return ERR_PTR(PTR_ERR(base));
>> > > > > +
>> > > > > + map = regmap_init_mmio(NULL, base,
>> > > &ingenic_tcu_regmap_config);
>> > > > > + if (IS_ERR(map))
>> > > > > + goto err_iounmap;
>> >
>> > Place this inside probe().
>> >
>> > > > > + return map;
>> > > > > +
>> > > > > +err_iounmap:
>> > > > > + iounmap(base);
>> > > > > + of_address_to_resource(np, 0, &res);
>> > > > > + release_mem_region(res.start, resource_size(&res));
>> > > > > +
>> > > > > + return map;
>> > > > > +}
>> > > >
>> > > > Why does this need to be set-up earlier than probe()?
>> > >
>> > > See the explanation below.
>> >
>> > I think the answer is, it doesn't.
>> >
>> > > > > +static int __init ingenic_tcu_probe(struct
>> platform_device
>> > > *pdev)
>> > > > > +{
>> > > > > + struct regmap *map =
>> > > ingenic_tcu_get_regmap(pdev->dev.of_node);
>> > > > > +
>> > > > > + platform_set_drvdata(pdev, map);
>> > > > > +
>> > > > > + regmap_attach_dev(&pdev->dev, map,
>> > > &ingenic_tcu_regmap_config);
>> > > > > +
>> > > > > + return devm_of_platform_populate(&pdev->dev);
>> > > > > +}
>> > > > > +
>> > > > > +static struct platform_driver ingenic_tcu_driver = {
>> > > > > + .driver = {
>> > > > > + .name = "ingenic-tcu",
>> > > > > + .of_match_table = ingenic_tcu_of_match,
>> > > > > + },
>> > > > > +};
>> > > > > +
>> > > > > +static int __init ingenic_tcu_platform_init(void)
>> > > > > +{
>> > > > > + return platform_driver_probe(&ingenic_tcu_driver,
>> > > > > + ingenic_tcu_probe);
>> > > >
>> > > > What? Why?
>> > >
>> > > The device driver probed here will populate the children
>> devices,
>> > > which will be able to retrieve the pointer to the regmap
>> through
>> > > device_get_regmap(dev->parent).
>> >
>> > I've never heard of this call. Where is it?
>>
>> dev_get_regmap, in <linux/regmap.h>.
>>
>> > > The children devices are normal platform drivers that can be
>> probed
>> > > the normal way. These are the PWM driver, the watchdog driver,
>> and
>> > > the
>> > > OST (OS Timer) clocksource driver, all part of the same
>> hardware
>> > > block
>> > > (the Timer/Counter Unit or TCU).
>> >
>> > If they are normal devices, then there is no need to roll your own
>> > regmap-getter implementation like this.
>> >
>> > > > > +}
>> > > > > +subsys_initcall(ingenic_tcu_platform_init);
>> > > > > +
>> > > > > +struct regmap * __init ingenic_tcu_get_regmap(struct
>> > > device_node
>> > > > > *np)
>> > > > > +{
>> > > > > + if (!tcu_regmap)
>> > > > > + tcu_regmap = ingenic_tcu_create_regmap(np);
>> > > > > +
>> > > > > + return tcu_regmap;
>> > > > > +}
>> > > >
>> > > > This makes me pretty uncomfortable.
>> > > >
>> > > > What calls it?
>> > >
>> > > The TCU IRQ driver (patch [06/13]), clocks driver (patch
>> [05/13]),
>> > > and the
>> > > non-OST clocksource driver (patch [07/13]) all probe very
>> early in
>> > > the boot
>> > > process, and share the same devicetree node. They call this
>> > > function to get
>> > > a pointer to the regmap.
>> >
>> > Horrible!
>> >
>> > Instead, you should send it through platform_set_drvdata() and
>> collect
>> > it in the child drivers with platform_get_drvdata(dev->parent).
>>
>> The IRQ, clocks and clocksource driver do NOT have a "struct
>> device" to
>> begin with. They are not platform drivers, and cannot be platform
>> drivers,
>> as they must register so early in the boot process, before "struct
>> device"
>> is even a thing.
>>
>> All they get is a pointer to the same devicetree node. Since all of
>> these
>> have to use the same registers, they need to use a shared regmap,
>> which
>> they obtain by calling ingenic_tcu_get_regmap() below.
>>
>> Then, when this driver's probe gets called, the regmap is retrieved
>> and
>> attached to the struct device, and then the children devices will be
>> probed: the watchdog device, the PWM device, the OST device. These
>> three
>> will retrieve the regmap by calling dev_get_regmap(dev->parent,
>> NULL).
>
> That makes sense.
>
> This explanation certainly belongs in the commit log.
Right.
> Can you send your v14, as you intended. I will re-review it with new
> eyes when you do.
Could you review v13 instead? v14 will be a v13 with tiny teeny
non-code fixes (delete some newlines, replace %i with %d, and
convert the documentation from .txt to .rst).
>> > > > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev,
>> unsigned
>> > > int
>> > > > > channel)
>> > > > > +{
>> > > > > + const struct ingenic_soc_info *soc =
>> > > > > device_get_match_data(dev->parent);
>> > > > > +
>> > > > > + /* Enable all TCU channels for PWM use by default except
>> > > channels
>> > > > > 0/1 */
>> > > > > + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1,
>> 2);
>> > > > > +
>> > > > > + device_property_read_u32(dev->parent,
>> > > "ingenic,pwm-channels-mask",
>> > > > > + &pwm_channels_mask);
>> >
>> > Doesn't this call overwrite the previous assignment above?
>>
>> Yes, that's intended. You have a default value, that can be
>> overriden
>> by a device property.
>
> You should provide a comment here to make your intentions clear.
Ok.
>> > > > > + return !!(pwm_channels_mask & BIT(channel));
>> > > > > +}
>> > > > > +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>> >
>> > Where is this called from?
>>
>> This is called from the PWM driver.
>
> Why can't it live in the PWM driver?
It totally can. I'll move it there.
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver
From: Lee Jones @ 2019-06-27 9:01 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Jonathan Corbet, Daniel Lezcano, Thomas Gleixner,
Michael Turquette, Stephen Boyd, Jason Cooper, Marc Zyngier,
Mathieu Malaterre, linux-kernel, devicetree, linux-mips,
linux-doc, linux-clk, od
In-Reply-To: <1561625387.1745.0@crapouillou.net>
On Thu, 27 Jun 2019, Paul Cercueil wrote:
> Le jeu. 27 juin 2019 à 8:58, Lee Jones <lee.jones@linaro.org> a écrit :
> > On Wed, 26 Jun 2019, Paul Cercueil wrote:
> > > Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a
> > > écrit :
> > > > On Tue, 21 May 2019, Paul Cercueil wrote:
> > > >
> > > > > This driver will provide a regmap that can be retrieved very
> > > early
> > > > > in
> > > > > the boot process through the API function
> > > ingenic_tcu_get_regmap().
> > > > >
> > > > > Additionally, it will call devm_of_platform_populate() so that
> > > all
> > > > > the
> > > > > children devices will be probed.
> > > > >
> > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > > v12: New patch
> > > > >
> > > > > drivers/mfd/Kconfig | 8 +++
> > > > > drivers/mfd/Makefile | 1 +
> > > > > drivers/mfd/ingenic-tcu.c | 113
> > > > > ++++++++++++++++++++++++++++++++
> > > > > include/linux/mfd/ingenic-tcu.h | 8 +++
> > > > > 4 files changed, 130 insertions(+)
> > > > > create mode 100644 drivers/mfd/ingenic-tcu.c
> >
> > [...]
> >
> > > > > +static struct regmap * __init ingenic_tcu_create_regmap(struct
> > > > > device_node *np)
> > > > > +{
> > > > > + struct resource res;
> > > > > + void __iomem *base;
> > > > > + struct regmap *map;
> > > > > +
> > > > > + if (!of_match_node(ingenic_tcu_of_match, np))
> > > > > + return ERR_PTR(-EINVAL);
> >
> > Drop this check.
> >
> > > > > + base = of_io_request_and_map(np, 0, "TCU");
> > > > > + if (IS_ERR(base))
> > > > > + return ERR_PTR(PTR_ERR(base));
> > > > > +
> > > > > + map = regmap_init_mmio(NULL, base,
> > > &ingenic_tcu_regmap_config);
> > > > > + if (IS_ERR(map))
> > > > > + goto err_iounmap;
> >
> > Place this inside probe().
> >
> > > > > + return map;
> > > > > +
> > > > > +err_iounmap:
> > > > > + iounmap(base);
> > > > > + of_address_to_resource(np, 0, &res);
> > > > > + release_mem_region(res.start, resource_size(&res));
> > > > > +
> > > > > + return map;
> > > > > +}
> > > >
> > > > Why does this need to be set-up earlier than probe()?
> > >
> > > See the explanation below.
> >
> > I think the answer is, it doesn't.
> >
> > > > > +static int __init ingenic_tcu_probe(struct platform_device
> > > *pdev)
> > > > > +{
> > > > > + struct regmap *map =
> > > ingenic_tcu_get_regmap(pdev->dev.of_node);
> > > > > +
> > > > > + platform_set_drvdata(pdev, map);
> > > > > +
> > > > > + regmap_attach_dev(&pdev->dev, map,
> > > &ingenic_tcu_regmap_config);
> > > > > +
> > > > > + return devm_of_platform_populate(&pdev->dev);
> > > > > +}
> > > > > +
> > > > > +static struct platform_driver ingenic_tcu_driver = {
> > > > > + .driver = {
> > > > > + .name = "ingenic-tcu",
> > > > > + .of_match_table = ingenic_tcu_of_match,
> > > > > + },
> > > > > +};
> > > > > +
> > > > > +static int __init ingenic_tcu_platform_init(void)
> > > > > +{
> > > > > + return platform_driver_probe(&ingenic_tcu_driver,
> > > > > + ingenic_tcu_probe);
> > > >
> > > > What? Why?
> > >
> > > The device driver probed here will populate the children devices,
> > > which will be able to retrieve the pointer to the regmap through
> > > device_get_regmap(dev->parent).
> >
> > I've never heard of this call. Where is it?
>
> dev_get_regmap, in <linux/regmap.h>.
>
> > > The children devices are normal platform drivers that can be probed
> > > the normal way. These are the PWM driver, the watchdog driver, and
> > > the
> > > OST (OS Timer) clocksource driver, all part of the same hardware
> > > block
> > > (the Timer/Counter Unit or TCU).
> >
> > If they are normal devices, then there is no need to roll your own
> > regmap-getter implementation like this.
> >
> > > > > +}
> > > > > +subsys_initcall(ingenic_tcu_platform_init);
> > > > > +
> > > > > +struct regmap * __init ingenic_tcu_get_regmap(struct
> > > device_node
> > > > > *np)
> > > > > +{
> > > > > + if (!tcu_regmap)
> > > > > + tcu_regmap = ingenic_tcu_create_regmap(np);
> > > > > +
> > > > > + return tcu_regmap;
> > > > > +}
> > > >
> > > > This makes me pretty uncomfortable.
> > > >
> > > > What calls it?
> > >
> > > The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]),
> > > and the
> > > non-OST clocksource driver (patch [07/13]) all probe very early in
> > > the boot
> > > process, and share the same devicetree node. They call this
> > > function to get
> > > a pointer to the regmap.
> >
> > Horrible!
> >
> > Instead, you should send it through platform_set_drvdata() and collect
> > it in the child drivers with platform_get_drvdata(dev->parent).
>
> The IRQ, clocks and clocksource driver do NOT have a "struct device" to
> begin with. They are not platform drivers, and cannot be platform drivers,
> as they must register so early in the boot process, before "struct device"
> is even a thing.
>
> All they get is a pointer to the same devicetree node. Since all of these
> have to use the same registers, they need to use a shared regmap, which
> they obtain by calling ingenic_tcu_get_regmap() below.
>
> Then, when this driver's probe gets called, the regmap is retrieved and
> attached to the struct device, and then the children devices will be
> probed: the watchdog device, the PWM device, the OST device. These three
> will retrieve the regmap by calling dev_get_regmap(dev->parent, NULL).
That makes sense.
This explanation certainly belongs in the commit log.
Can you send your v14, as you intended. I will re-review it with new
eyes when you do.
> > > > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned
> > > int
> > > > > channel)
> > > > > +{
> > > > > + const struct ingenic_soc_info *soc =
> > > > > device_get_match_data(dev->parent);
> > > > > +
> > > > > + /* Enable all TCU channels for PWM use by default except
> > > channels
> > > > > 0/1 */
> > > > > + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> > > > > +
> > > > > + device_property_read_u32(dev->parent,
> > > "ingenic,pwm-channels-mask",
> > > > > + &pwm_channels_mask);
> >
> > Doesn't this call overwrite the previous assignment above?
>
> Yes, that's intended. You have a default value, that can be overriden
> by a device property.
You should provide a comment here to make your intentions clear.
> > > > > + return !!(pwm_channels_mask & BIT(channel));
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> >
> > Where is this called from?
>
> This is called from the PWM driver.
Why can't it live in the PWM driver?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver
From: Paul Cercueil @ 2019-06-27 8:49 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Jonathan Corbet, Daniel Lezcano, Thomas Gleixner,
Michael Turquette, Stephen Boyd, Jason Cooper, Marc Zyngier,
Mathieu Malaterre, linux-kernel, devicetree, linux-mips,
linux-doc, linux-clk, od
In-Reply-To: <20190627065808.GY21119@dell>
Le jeu. 27 juin 2019 à 8:58, Lee Jones <lee.jones@linaro.org> a écrit
:
> On Wed, 26 Jun 2019, Paul Cercueil wrote:
>> Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a
>> écrit :
>> > On Tue, 21 May 2019, Paul Cercueil wrote:
>> >
>> > > This driver will provide a regmap that can be retrieved very
>> early
>> > > in
>> > > the boot process through the API function
>> ingenic_tcu_get_regmap().
>> > >
>> > > Additionally, it will call devm_of_platform_populate() so that
>> all
>> > > the
>> > > children devices will be probed.
>> > >
>> > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> > > ---
>> > >
>> > > Notes:
>> > > v12: New patch
>> > >
>> > > drivers/mfd/Kconfig | 8 +++
>> > > drivers/mfd/Makefile | 1 +
>> > > drivers/mfd/ingenic-tcu.c | 113
>> > > ++++++++++++++++++++++++++++++++
>> > > include/linux/mfd/ingenic-tcu.h | 8 +++
>> > > 4 files changed, 130 insertions(+)
>> > > create mode 100644 drivers/mfd/ingenic-tcu.c
>
> [...]
>
>> > > +static struct regmap * __init ingenic_tcu_create_regmap(struct
>> > > device_node *np)
>> > > +{
>> > > + struct resource res;
>> > > + void __iomem *base;
>> > > + struct regmap *map;
>> > > +
>> > > + if (!of_match_node(ingenic_tcu_of_match, np))
>> > > + return ERR_PTR(-EINVAL);
>
> Drop this check.
>
>> > > + base = of_io_request_and_map(np, 0, "TCU");
>> > > + if (IS_ERR(base))
>> > > + return ERR_PTR(PTR_ERR(base));
>> > > +
>> > > + map = regmap_init_mmio(NULL, base,
>> &ingenic_tcu_regmap_config);
>> > > + if (IS_ERR(map))
>> > > + goto err_iounmap;
>
> Place this inside probe().
>
>> > > + return map;
>> > > +
>> > > +err_iounmap:
>> > > + iounmap(base);
>> > > + of_address_to_resource(np, 0, &res);
>> > > + release_mem_region(res.start, resource_size(&res));
>> > > +
>> > > + return map;
>> > > +}
>> >
>> > Why does this need to be set-up earlier than probe()?
>>
>> See the explanation below.
>
> I think the answer is, it doesn't.
>
>> > > +static int __init ingenic_tcu_probe(struct platform_device
>> *pdev)
>> > > +{
>> > > + struct regmap *map =
>> ingenic_tcu_get_regmap(pdev->dev.of_node);
>> > > +
>> > > + platform_set_drvdata(pdev, map);
>> > > +
>> > > + regmap_attach_dev(&pdev->dev, map,
>> &ingenic_tcu_regmap_config);
>> > > +
>> > > + return devm_of_platform_populate(&pdev->dev);
>> > > +}
>> > > +
>> > > +static struct platform_driver ingenic_tcu_driver = {
>> > > + .driver = {
>> > > + .name = "ingenic-tcu",
>> > > + .of_match_table = ingenic_tcu_of_match,
>> > > + },
>> > > +};
>> > > +
>> > > +static int __init ingenic_tcu_platform_init(void)
>> > > +{
>> > > + return platform_driver_probe(&ingenic_tcu_driver,
>> > > + ingenic_tcu_probe);
>> >
>> > What? Why?
>>
>> The device driver probed here will populate the children devices,
>> which will be able to retrieve the pointer to the regmap through
>> device_get_regmap(dev->parent).
>
> I've never heard of this call. Where is it?
dev_get_regmap, in <linux/regmap.h>.
>> The children devices are normal platform drivers that can be probed
>> the normal way. These are the PWM driver, the watchdog driver, and
>> the
>> OST (OS Timer) clocksource driver, all part of the same hardware
>> block
>> (the Timer/Counter Unit or TCU).
>
> If they are normal devices, then there is no need to roll your own
> regmap-getter implementation like this.
>
>> > > +}
>> > > +subsys_initcall(ingenic_tcu_platform_init);
>> > > +
>> > > +struct regmap * __init ingenic_tcu_get_regmap(struct
>> device_node
>> > > *np)
>> > > +{
>> > > + if (!tcu_regmap)
>> > > + tcu_regmap = ingenic_tcu_create_regmap(np);
>> > > +
>> > > + return tcu_regmap;
>> > > +}
>> >
>> > This makes me pretty uncomfortable.
>> >
>> > What calls it?
>>
>> The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]),
>> and the
>> non-OST clocksource driver (patch [07/13]) all probe very early in
>> the boot
>> process, and share the same devicetree node. They call this
>> function to get
>> a pointer to the regmap.
>
> Horrible!
>
> Instead, you should send it through platform_set_drvdata() and collect
> it in the child drivers with platform_get_drvdata(dev->parent).
The IRQ, clocks and clocksource driver do NOT have a "struct device" to
begin with. They are not platform drivers, and cannot be platform
drivers,
as they must register so early in the boot process, before "struct
device"
is even a thing.
All they get is a pointer to the same devicetree node. Since all of
these
have to use the same registers, they need to use a shared regmap, which
they obtain by calling ingenic_tcu_get_regmap() below.
Then, when this driver's probe gets called, the regmap is retrieved and
attached to the struct device, and then the children devices will be
probed: the watchdog device, the PWM device, the OST device. These three
will retrieve the regmap by calling dev_get_regmap(dev->parent, NULL).
>> > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned
>> int
>> > > channel)
>> > > +{
>> > > + const struct ingenic_soc_info *soc =
>> > > device_get_match_data(dev->parent);
>> > > +
>> > > + /* Enable all TCU channels for PWM use by default except
>> channels
>> > > 0/1 */
>> > > + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
>> > > +
>> > > + device_property_read_u32(dev->parent,
>> "ingenic,pwm-channels-mask",
>> > > + &pwm_channels_mask);
>
> Doesn't this call overwrite the previous assignment above?
Yes, that's intended. You have a default value, that can be overriden
by a device property.
>> > > + return !!(pwm_channels_mask & BIT(channel));
>> > > +}
>> > > +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>
> Where is this called from?
This is called from the PWM driver.
> I think this needs a review by the DT guys.
Rob already acked the bindings, which describe this property.
>> > > diff --git a/include/linux/mfd/ingenic-tcu.h
>> > > b/include/linux/mfd/ingenic-tcu.h
>> > > index 2083fa20821d..21df23916cd2 100644
>> > > --- a/include/linux/mfd/ingenic-tcu.h
>> > > +++ b/include/linux/mfd/ingenic-tcu.h
>> > > @@ -6,6 +6,11 @@
>> > > #define __LINUX_MFD_INGENIC_TCU_H_
>> > >
>> > > #include <linux/bitops.h>
>> > > +#include <linux/init.h>
>> > > +
>> > > +struct device;
>> > > +struct device_node;
>> > > +struct regmap;
>> > >
>> > > #define TCU_REG_WDT_TDR 0x00
>> > > #define TCU_REG_WDT_TCER 0x04
>> > > @@ -53,4 +58,7 @@
>> > > #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) *
>> > > TCU_CHANNEL_STRIDE))
>> > > #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) *
>> > > TCU_CHANNEL_STRIDE))
>> > >
>> > > +struct regmap * __init ingenic_tcu_get_regmap(struct
>> device_node
>> > > *np);
>> > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned
>> int
>> > > channel);
>> > > +
>> > > #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
>> >
>>
>>
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver
From: Lee Jones @ 2019-06-27 6:58 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Jonathan Corbet, Daniel Lezcano, Thomas Gleixner,
Michael Turquette, Stephen Boyd, Jason Cooper, Marc Zyngier,
Mathieu Malaterre, linux-kernel, devicetree, linux-mips,
linux-doc, linux-clk, od
In-Reply-To: <1561557350.1872.0@crapouillou.net>
On Wed, 26 Jun 2019, Paul Cercueil wrote:
> Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a écrit :
> > On Tue, 21 May 2019, Paul Cercueil wrote:
> >
> > > This driver will provide a regmap that can be retrieved very early
> > > in
> > > the boot process through the API function ingenic_tcu_get_regmap().
> > >
> > > Additionally, it will call devm_of_platform_populate() so that all
> > > the
> > > children devices will be probed.
> > >
> > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > > ---
> > >
> > > Notes:
> > > v12: New patch
> > >
> > > drivers/mfd/Kconfig | 8 +++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/ingenic-tcu.c | 113
> > > ++++++++++++++++++++++++++++++++
> > > include/linux/mfd/ingenic-tcu.h | 8 +++
> > > 4 files changed, 130 insertions(+)
> > > create mode 100644 drivers/mfd/ingenic-tcu.c
[...]
> > > +static struct regmap * __init ingenic_tcu_create_regmap(struct
> > > device_node *np)
> > > +{
> > > + struct resource res;
> > > + void __iomem *base;
> > > + struct regmap *map;
> > > +
> > > + if (!of_match_node(ingenic_tcu_of_match, np))
> > > + return ERR_PTR(-EINVAL);
Drop this check.
> > > + base = of_io_request_and_map(np, 0, "TCU");
> > > + if (IS_ERR(base))
> > > + return ERR_PTR(PTR_ERR(base));
> > > +
> > > + map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> > > + if (IS_ERR(map))
> > > + goto err_iounmap;
Place this inside probe().
> > > + return map;
> > > +
> > > +err_iounmap:
> > > + iounmap(base);
> > > + of_address_to_resource(np, 0, &res);
> > > + release_mem_region(res.start, resource_size(&res));
> > > +
> > > + return map;
> > > +}
> >
> > Why does this need to be set-up earlier than probe()?
>
> See the explanation below.
I think the answer is, it doesn't.
> > > +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> > > +{
> > > + struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
> > > +
> > > + platform_set_drvdata(pdev, map);
> > > +
> > > + regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
> > > +
> > > + return devm_of_platform_populate(&pdev->dev);
> > > +}
> > > +
> > > +static struct platform_driver ingenic_tcu_driver = {
> > > + .driver = {
> > > + .name = "ingenic-tcu",
> > > + .of_match_table = ingenic_tcu_of_match,
> > > + },
> > > +};
> > > +
> > > +static int __init ingenic_tcu_platform_init(void)
> > > +{
> > > + return platform_driver_probe(&ingenic_tcu_driver,
> > > + ingenic_tcu_probe);
> >
> > What? Why?
>
> The device driver probed here will populate the children devices,
> which will be able to retrieve the pointer to the regmap through
> device_get_regmap(dev->parent).
I've never heard of this call. Where is it?
> The children devices are normal platform drivers that can be probed
> the normal way. These are the PWM driver, the watchdog driver, and the
> OST (OS Timer) clocksource driver, all part of the same hardware block
> (the Timer/Counter Unit or TCU).
If they are normal devices, then there is no need to roll your own
regmap-getter implementation like this.
> > > +}
> > > +subsys_initcall(ingenic_tcu_platform_init);
> > > +
> > > +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
> > > *np)
> > > +{
> > > + if (!tcu_regmap)
> > > + tcu_regmap = ingenic_tcu_create_regmap(np);
> > > +
> > > + return tcu_regmap;
> > > +}
> >
> > This makes me pretty uncomfortable.
> >
> > What calls it?
>
> The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]), and the
> non-OST clocksource driver (patch [07/13]) all probe very early in the boot
> process, and share the same devicetree node. They call this function to get
> a pointer to the regmap.
Horrible!
Instead, you should send it through platform_set_drvdata() and collect
it in the child drivers with platform_get_drvdata(dev->parent).
> > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
> > > channel)
> > > +{
> > > + const struct ingenic_soc_info *soc =
> > > device_get_match_data(dev->parent);
> > > +
> > > + /* Enable all TCU channels for PWM use by default except channels
> > > 0/1 */
> > > + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> > > +
> > > + device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
> > > + &pwm_channels_mask);
Doesn't this call overwrite the previous assignment above?
> > > + return !!(pwm_channels_mask & BIT(channel));
> > > +}
> > > +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
Where is this called from?
I think this needs a review by the DT guys.
> > > diff --git a/include/linux/mfd/ingenic-tcu.h
> > > b/include/linux/mfd/ingenic-tcu.h
> > > index 2083fa20821d..21df23916cd2 100644
> > > --- a/include/linux/mfd/ingenic-tcu.h
> > > +++ b/include/linux/mfd/ingenic-tcu.h
> > > @@ -6,6 +6,11 @@
> > > #define __LINUX_MFD_INGENIC_TCU_H_
> > >
> > > #include <linux/bitops.h>
> > > +#include <linux/init.h>
> > > +
> > > +struct device;
> > > +struct device_node;
> > > +struct regmap;
> > >
> > > #define TCU_REG_WDT_TDR 0x00
> > > #define TCU_REG_WDT_TCER 0x04
> > > @@ -53,4 +58,7 @@
> > > #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) *
> > > TCU_CHANNEL_STRIDE))
> > > #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) *
> > > TCU_CHANNEL_STRIDE))
> > >
> > > +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
> > > *np);
> > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
> > > channel);
> > > +
> > > #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
> >
>
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH] Documentation:sh:convert register-banks.txt to register-banks.rst
From: Vandana BN @ 2019-06-27 6:33 UTC (permalink / raw)
To: ysato, dalias, corbet, linux-sh, linux-doc, linux-kernel
Cc: skhan, gregkh, linux-kernel-mentees, Vandana BN
This patch converts register-banks.txt to ReST format, No content
change.
Added register-banks.rst to sh/index.rst
Signed-off-by: Vandana BN <bnvandana@gmail.com>
---
Documentation/sh/index.rst | 5 +++++
.../sh/{register-banks.txt => register-banks.rst} | 8 ++++----
2 files changed, 9 insertions(+), 4 deletions(-)
rename Documentation/sh/{register-banks.txt => register-banks.rst} (90%)
diff --git a/Documentation/sh/index.rst b/Documentation/sh/index.rst
index bc8db7ba894a..59b4e0e17aca 100644
--- a/Documentation/sh/index.rst
+++ b/Documentation/sh/index.rst
@@ -57,3 +57,8 @@ Maple
.. kernel-doc:: drivers/sh/maple/maple.c
:export:
+
+.. toctree::
+ :maxdepth: 2
+
+ register-banks
diff --git a/Documentation/sh/register-banks.txt b/Documentation/sh/register-banks.rst
similarity index 90%
rename from Documentation/sh/register-banks.txt
rename to Documentation/sh/register-banks.rst
index a6719f2f6594..acccfaf80355 100644
--- a/Documentation/sh/register-banks.txt
+++ b/Documentation/sh/register-banks.rst
@@ -1,8 +1,9 @@
- Notes on register bank usage in the kernel
- ==========================================
+==========================================
+Notes on register bank usage in the kernel
+==========================================
Introduction
-------------
+============
The SH-3 and SH-4 CPU families traditionally include a single partial register
bank (selected by SR.RB, only r0 ... r7 are banked), whereas other families
@@ -30,4 +31,3 @@ Presently the kernel uses several of these registers.
- The SR.IMASK interrupt handler makes use of this to set the
interrupt priority level (used by local_irq_enable())
- r7_bank (current)
-
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
From: Luis Chamberlain @ 2019-06-27 6:10 UTC (permalink / raw)
To: Iurii Zaikin, linux-api, Michael Kerrisk (man-pages)
Cc: Brendan Higgins, frowand.list, gregkh, jpoimboe, Kees Cook,
kieran.bingham, peterz, robh, Stephen Boyd, 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 Vetter, jdike, joel, julia.lawall, khilman,
knut.omang, logang, mpe, pmladek, rdunlap, richard,
David Rientjes, rostedt, wfg
In-Reply-To: <CAAXuY3p+kVhjQ4LYtzormqVcH2vKu1abc_K9Z0XY=JX=bp8NcQ@mail.gmail.com>
On Wed, Jun 26, 2019 at 09:07:43PM -0700, Iurii Zaikin wrote:
> On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > +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.
>
> You make a good point that the test codifies the existing behavior of
> the function in lieu of formal documentation. However, the test cases
> were derived from examining the source code of the function under test
> and attempting to cover all branches. The assertions were added only
> for the values that appeared to be set deliberately in the
> implementation. And it makes sense to me to test that the code does
> exactly what the implementation author intended.
I'm not arguing against adding them. I'm suggesting that it is different
to test for API than for correctness of intended functionality, and
it would be wise to make it clear which test cases are for API and which
for correctness.
This will come up later for other kunit tests and it would be great
to set precendent so that other kunit tests can follow similar
practices to ensure its clear what is API realted Vs correctness of
intended functionality.
In fact, I'm not yet sure if its possible to test public kernel API to
userspace with kunit, but if it is possible... well, that could make
linux-api folks happy as they could enable us to codify interpreation of
what is expected into kunit test cases, and we'd ensure that the
codified interpretation is not only documented in man pages but also
through formal kunit test cases.
A regression in linux-api then could be formalized through a proper
kunit tests case. And if an API evolves, it would force developers to
update the respective kunit which codifies that contract.
> > > +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.
> >
> Not sure why.
Because you are codifying that we *definitely* return -EINVAL on
overlow. Some parts of the kernel return -ERANGE for overflows for
instance.
It would be a generic test for overflow if it would just test
for any error.
It is a fine and good test to keep. All these tests are good to keep.
> I believe there has been a real bug with int overflow in
> proc_dointvec.
> Covering it with test seems like a good idea.
Oh definitely.
> > > +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.
> Added valid reads. There already are 2 valid writes.
Thanks.
> > 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).
> >
> Added "Tests the API contract and implementation correctness of sysctl."
Yes, much clearer, thanks!
Luis
^ permalink raw reply
* Re: [PATCH v5 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec()
From: Iurii Zaikin @ 2019-06-27 4:07 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Brendan Higgins, frowand.list, gregkh, jpoimboe, Kees Cook,
kieran.bingham, peterz, robh, Stephen Boyd, 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 Vetter, jdike, joel, julia.lawall, khilman,
knut.omang, logang, mpe, pmladek, rdunlap, richard,
David Rientjes, rostedt, wfg
In-Reply-To: <20190626021744.GU19023@42.do-not-panic.com>
On Tue, Jun 25, 2019 at 7:17 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> 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.
Went with the #define * suggestion above to keep it clear what
function is being tested.
> > +}
> > +
> > +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.
>
You make a good point that the test codifies the existing behavior of
the function
in lieu of formal documentation.
However, the test cases were derived from examining the source code
of the function under test and attempting to cover all branches. The
assertions were added
only for the values that appeared to be set deliberately in the
implementation. And it makes
sense to me to test that the code does exactly what the implementation
author intended.
> > +
> > +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.
>
Same as the above, if the implementation author meant the function to
behave deterministically
with the given input, it makes sense to test the behavior. Otherwise,
why not just remove the branch in
the function under test and say that the given input results in
undefined behavior?
> > +
> > +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.
>
Not sure why. I believe there has been a real bug with int overflow in
proc_dointvec.
Covering it with test seems like a good idea.
> > +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.
Added valid reads. There already are 2 valid writes.
> 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).
>
Added "Tests the API contract and implementation correctness of sysctl."
> > +
> > + 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 v2 0/4] Compile-test UAPI and kernel headers
From: Masahiro Yamada @ 2019-06-27 3:12 UTC (permalink / raw)
To: Linux Kbuild mailing list
Cc: Sam Ravnborg, Tony Luck, open list:DOCUMENTATION, John Fastabend,
Jonathan Corbet, Jakub Kicinski, linux-riscv, Daniel Borkmann,
xdp-newbies, Anton Vorontsov, Palmer Dabbelt, Matthias Brugger,
Song Liu, Yonghong Song, Michal Marek, Jesper Dangaard Brouer,
Martin KaFai Lau, moderated list:ARM/Mediatek SoC support,
linux-arm-kernel, Albert Ou, Colin Cross, David S. Miller,
Kees Cook, Alexei Starovoitov, Networking,
Linux Kernel Mailing List, bpf
In-Reply-To: <20190627014617.600-1-yamada.masahiro@socionext.com>
On Thu, Jun 27, 2019 at 10:49 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
>
> 1/4: reworked v2.
>
> 2/4: fix a flaw I noticed when I was working on this series
>
> 3/4: maybe useful for 4/4 and in some other places
>
> 4/4: v2. compile as many headers as possible.
>
If you want to test this series,
please check:
git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
header-test-v2
> Changes in v2:
> - Add CONFIG_CPU_{BIG,LITTLE}_ENDIAN guard to avoid build error
> - Use 'header-test-' instead of 'no-header-test'
> - Avoid weird 'find' warning when cleaning
> - New patch
> - New patch
> - Add everything to test coverage, and exclude broken ones
> - Rename 'Makefile' to 'Kbuild'
> - Add CONFIG_KERNEL_HEADER_TEST option
>
> Masahiro Yamada (4):
> kbuild: compile-test UAPI headers to ensure they are self-contained
> kbuild: do not create wrappers for header-test-y
> kbuild: support header-test-pattern-y
> kbuild: compile-test kernel headers to ensure they are self-contained
>
> .gitignore | 1 -
> Documentation/dontdiff | 1 -
> Documentation/kbuild/makefiles.txt | 13 +-
> Makefile | 4 +-
> include/Kbuild | 1134 ++++++++++++++++++++++++++++
> init/Kconfig | 22 +
> scripts/Makefile.build | 10 +-
> scripts/Makefile.lib | 12 +-
> scripts/cc-system-headers.sh | 8 +
> usr/.gitignore | 1 -
> usr/Makefile | 2 +
> usr/include/.gitignore | 3 +
> usr/include/Makefile | 133 ++++
> 13 files changed, 1331 insertions(+), 13 deletions(-)
> create mode 100644 include/Kbuild
> create mode 100755 scripts/cc-system-headers.sh
> create mode 100644 usr/include/.gitignore
> create mode 100644 usr/include/Makefile
>
> --
> 2.17.1
>
--
Best Regards
Masahiro Yamada
^ 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