* [PATCH 01/10] selftests/powerpc: add process creation benchmark
From: Nicholas Piggin @ 2018-03-06 13:24 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Aneesh Kumar K . V, Christophe Leroy
In-Reply-To: <20180306132507.10649-1-npiggin@gmail.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
.../selftests/powerpc/benchmarks/.gitignore | 2 +
.../testing/selftests/powerpc/benchmarks/Makefile | 8 +-
.../selftests/powerpc/benchmarks/exec_target.c | 5 +
tools/testing/selftests/powerpc/benchmarks/fork.c | 339 +++++++++++++++++++++
4 files changed, 353 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/powerpc/benchmarks/exec_target.c
create mode 100644 tools/testing/selftests/powerpc/benchmarks/fork.c
diff --git a/tools/testing/selftests/powerpc/benchmarks/.gitignore b/tools/testing/selftests/powerpc/benchmarks/.gitignore
index 04dc1e6ef2ce..9161679b1e1a 100644
--- a/tools/testing/selftests/powerpc/benchmarks/.gitignore
+++ b/tools/testing/selftests/powerpc/benchmarks/.gitignore
@@ -1,5 +1,7 @@
gettimeofday
context_switch
+fork
+exec_target
mmap_bench
futex_bench
null_syscall
diff --git a/tools/testing/selftests/powerpc/benchmarks/Makefile b/tools/testing/selftests/powerpc/benchmarks/Makefile
index a35058e3766c..61189a0b8285 100644
--- a/tools/testing/selftests/powerpc/benchmarks/Makefile
+++ b/tools/testing/selftests/powerpc/benchmarks/Makefile
@@ -1,5 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
-TEST_GEN_PROGS := gettimeofday context_switch mmap_bench futex_bench null_syscall
+TEST_GEN_PROGS := gettimeofday context_switch fork mmap_bench futex_bench null_syscall
+TEST_GEN_FILES := exec_target
+
+$(OUTPUT)/exec_target: exec_target.c
+ $(CC) -O2 -static -nostartfiles -oexec_target exec_target.c
CFLAGS += -O2
@@ -10,3 +14,5 @@ $(TEST_GEN_PROGS): ../harness.c
$(OUTPUT)/context_switch: ../utils.c
$(OUTPUT)/context_switch: CFLAGS += -maltivec -mvsx -mabi=altivec
$(OUTPUT)/context_switch: LDLIBS += -lpthread
+
+$(OUTPUT)/fork: LDLIBS += -lpthread
diff --git a/tools/testing/selftests/powerpc/benchmarks/exec_target.c b/tools/testing/selftests/powerpc/benchmarks/exec_target.c
new file mode 100644
index 000000000000..5e2a6e917c1a
--- /dev/null
+++ b/tools/testing/selftests/powerpc/benchmarks/exec_target.c
@@ -0,0 +1,5 @@
+void _exit(int);
+void _start(void)
+{
+ _exit(0);
+}
diff --git a/tools/testing/selftests/powerpc/benchmarks/fork.c b/tools/testing/selftests/powerpc/benchmarks/fork.c
new file mode 100644
index 000000000000..c68a7c360fd2
--- /dev/null
+++ b/tools/testing/selftests/powerpc/benchmarks/fork.c
@@ -0,0 +1,339 @@
+/*
+ * Context switch microbenchmark.
+ *
+ * Copyright (C) 2018 Anton Blanchard <anton@au.ibm.com>, IBM
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <sched.h>
+#include <string.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <getopt.h>
+#include <signal.h>
+#include <assert.h>
+#include <pthread.h>
+#include <limits.h>
+#include <sys/time.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/shm.h>
+#include <sys/wait.h>
+#include <linux/futex.h>
+
+static unsigned int timeout = 30;
+
+static void set_cpu(int cpu)
+{
+ cpu_set_t cpuset;
+
+ if (cpu == -1)
+ return;
+
+ CPU_ZERO(&cpuset);
+ CPU_SET(cpu, &cpuset);
+
+ if (sched_setaffinity(0, sizeof(cpuset), &cpuset)) {
+ perror("sched_setaffinity");
+ exit(1);
+ }
+}
+
+static void start_process_on(void *(*fn)(void *), void *arg, int cpu)
+{
+ int pid;
+
+ pid = fork();
+ if (pid == -1) {
+ perror("fork");
+ exit(1);
+ }
+
+ if (pid)
+ return;
+
+ set_cpu(cpu);
+
+ fn(arg);
+
+ exit(0);
+}
+
+static int cpu;
+static int do_fork = 0;
+static int do_vfork = 0;
+static int do_exec = 0;
+static char *exec_file;
+static int exec_target = 0;
+static unsigned long iterations;
+static unsigned long iterations_prev;
+
+static void run_exec(void)
+{
+#if 0
+ char *const argv[] = { exec_file, "--exec-target", NULL };
+
+ if (execve(exec_file, argv, NULL) == -1) {
+ perror("execve");
+ exit(1);
+ }
+#else
+ char *const argv[] = { "./exec_target", NULL };
+
+ if (execve("./exec_target", argv, NULL) == -1) {
+ perror("execve");
+ exit(1);
+ }
+#endif
+}
+
+static void bench_fork(void)
+{
+ while (1) {
+ pid_t pid = fork();
+ if (pid == -1) {
+ perror("fork");
+ exit(1);
+ }
+ if (pid == 0) {
+ if (do_exec)
+ run_exec();
+ _exit(0);
+ }
+ pid = waitpid(pid, NULL, 0);
+ if (pid == -1) {
+ perror("waitpid");
+ exit(1);
+ }
+ iterations++;
+ }
+}
+
+static void bench_vfork(void)
+{
+ while (1) {
+ pid_t pid = vfork();
+ if (pid == -1) {
+ perror("fork");
+ exit(1);
+ }
+ if (pid == 0) {
+ if (do_exec)
+ run_exec();
+ _exit(0);
+ }
+ pid = waitpid(pid, NULL, 0);
+ if (pid == -1) {
+ perror("waitpid");
+ exit(1);
+ }
+ iterations++;
+ }
+}
+
+
+static void *null_fn(void *arg)
+{
+ pthread_exit(NULL);
+}
+
+static void bench_thread(void)
+{
+ pthread_t tid;
+ cpu_set_t cpuset;
+ pthread_attr_t attr;
+ int rc;
+
+ rc = pthread_attr_init(&attr);
+ if (rc) {
+ errno = rc;
+ perror("pthread_attr_init");
+ exit(1);
+ }
+
+ if (cpu != -1) {
+ CPU_ZERO(&cpuset);
+ CPU_SET(cpu, &cpuset);
+
+ rc = pthread_attr_setaffinity_np(&attr, sizeof(cpu_set_t), &cpuset);
+ if (rc) {
+ errno = rc;
+ perror("pthread_attr_setaffinity_np");
+ exit(1);
+ }
+ }
+
+ while (1) {
+ rc = pthread_create(&tid, &attr, null_fn, NULL);
+ if (rc) {
+ errno = rc;
+ perror("pthread_create");
+ exit(1);
+ }
+ rc = pthread_join(tid, NULL);
+ if (rc) {
+ errno = rc;
+ perror("pthread_join");
+ exit(1);
+ }
+ iterations++;
+ }
+}
+
+
+static void sigalrm_handler(int junk)
+{
+ unsigned long i = iterations;
+
+ printf("%ld\n", i - iterations_prev);
+ iterations_prev = i;
+
+ if (--timeout == 0)
+ kill(0, SIGUSR1);
+
+ alarm(1);
+}
+
+static void sigusr1_handler(int junk)
+{
+ exit(0);
+}
+
+static void *bench_proc(void *arg)
+{
+ signal(SIGALRM, sigalrm_handler);
+ alarm(1);
+
+ if (do_fork)
+ bench_fork();
+ else if (do_vfork)
+ bench_vfork();
+ else
+ bench_thread();
+
+ return NULL;
+}
+
+static struct option options[] = {
+ { "fork", no_argument, &do_fork, 1 },
+ { "vfork", no_argument, &do_vfork, 1 },
+ { "exec", no_argument, &do_exec, 1 },
+ { "timeout", required_argument, 0, 's' },
+ { "exec-target", no_argument, &exec_target, 1 },
+ { 0, },
+};
+
+static void usage(void)
+{
+ fprintf(stderr, "Usage: fork <options> CPU\n\n");
+ fprintf(stderr, "\t\t--fork\tUse fork() (default threads)\n");
+ fprintf(stderr, "\t\t--vfork\tUse vfork() (default threads)\n");
+ fprintf(stderr, "\t\t--exec\tAlso exec() (default no exec)\n");
+ fprintf(stderr, "\t\t--timeout=X\tDuration in seconds to run (default 30)\n");
+ fprintf(stderr, "\t\t--exec-target\tInternal option for exec workload\n");
+}
+
+int main(int argc, char *argv[])
+{
+ signed char c;
+
+ while (1) {
+ int option_index = 0;
+
+ c = getopt_long(argc, argv, "", options, &option_index);
+
+ if (c == -1)
+ break;
+
+ switch (c) {
+ case 0:
+ if (options[option_index].flag != 0)
+ break;
+
+ usage();
+ exit(1);
+ break;
+
+ case 's':
+ timeout = atoi(optarg);
+ break;
+
+ default:
+ usage();
+ exit(1);
+ }
+ }
+
+ if (do_fork && do_vfork) {
+ usage();
+ exit(1);
+ }
+ if (do_exec && !do_fork && !do_vfork) {
+ usage();
+ exit(1);
+ }
+
+ if (do_exec) {
+ char *dirname = strdup(argv[0]);
+ int i;
+ i = strlen(dirname) - 1;
+ while (i) {
+ if (dirname[i] == '/') {
+ dirname[i] = '\0';
+ if (chdir(dirname) == -1) {
+ perror("chdir");
+ exit(1);
+ }
+ break;
+ }
+ i--;
+ }
+ }
+
+ if (exec_target) {
+ exit(0);
+ }
+
+ if (((argc - optind) != 1)) {
+ cpu = -1;
+ } else {
+ cpu = atoi(argv[optind++]);
+ }
+
+ if (do_exec)
+ exec_file = argv[0];
+
+ set_cpu(cpu);
+
+ printf("Using ");
+ if (do_fork)
+ printf("fork");
+ else if (do_vfork)
+ printf("vfork");
+ else
+ printf("clone");
+
+ if (do_exec)
+ printf(" + exec");
+
+ printf(" on cpu %d\n", cpu);
+
+ /* Create a new process group so we can signal everyone for exit */
+ setpgid(getpid(), getpid());
+
+ signal(SIGUSR1, sigusr1_handler);
+
+ start_process_on(bench_proc, NULL, cpu);
+
+ while (1)
+ sleep(3600);
+
+ return 0;
+}
--
2.16.1
^ permalink raw reply related
* [PATCH 00/10] powerpc/mm/slice: improve slice speed and stack use
From: Nicholas Piggin @ 2018-03-06 13:24 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Aneesh Kumar K . V, Christophe Leroy
Since this was last posted, it's been ported on top of Christophe's
8xx slice implementation that is merged in powerpc next, also taken
into account some feedback and bugs from Aneesh and Christophe --
thanks.
A few significant changes, first is refactoring slice_set_user_psize,
which makes it more obvious how the slice state is initialized, which
makes it easier to reason about using dynamic high slice size limits I
think.
Second is a significant change to how the slice masks are kept. No
longer are they bolted on the side and hit with a big recalculation
call that redoes everything whenever something changes. Now they are
just maintained as part of slice conversion.
This now passes vm selftests including the 128TB boundary case tests.
I also added a process microbenchmark and redid benchmarks and stack
measurements.
Overall on POWER8, this series increases vfork+exec+exit
microbenchmark rate by 15.6%, and mmap+munmap rate by 81%. Slice
code/data size is reduced by 1kB, and max stack overhead through
slice_get_unmapped_area call goes rom 992 to 448 bytes. The cost is
288 bytes added to the mm_context_t per mm for the slice masks on
Book3S.
Thanks,
Nick
Nicholas Piggin (10):
selftests/powerpc: add process creation benchmark
powerpc/mm/slice: Simplify and optimise slice context initialisation
powerpc/mm/slice: tidy lpsizes and hpsizes update loops
powerpc/mm/slice: pass pointers to struct slice_mask where possible
powerpc/mm/slice: implement a slice mask cache
powerpc/mm/slice: implement slice_check_range_fits
powerpc/mm/slice: Switch to 3-operand slice bitops helpers
powerpc/mm/slice: Use const pointers to cached slice masks where
possible
powerpc/mm/slice: use the dynamic high slice size to limit bitmap
operations
powerpc/mm/slice: remove radix calls to the slice code
arch/powerpc/include/asm/book3s/64/mmu.h | 18 +
arch/powerpc/include/asm/hugetlb.h | 9 +-
arch/powerpc/include/asm/mmu-8xx.h | 14 +
arch/powerpc/include/asm/slice.h | 8 +-
arch/powerpc/mm/hugetlbpage.c | 5 +-
arch/powerpc/mm/mmu_context_book3s64.c | 9 +-
arch/powerpc/mm/mmu_context_nohash.c | 5 +-
arch/powerpc/mm/slice.c | 458 +++++++++++----------
.../selftests/powerpc/benchmarks/.gitignore | 2 +
.../testing/selftests/powerpc/benchmarks/Makefile | 8 +-
.../selftests/powerpc/benchmarks/exec_target.c | 5 +
tools/testing/selftests/powerpc/benchmarks/fork.c | 339 +++++++++++++++
12 files changed, 632 insertions(+), 248 deletions(-)
create mode 100644 tools/testing/selftests/powerpc/benchmarks/exec_target.c
create mode 100644 tools/testing/selftests/powerpc/benchmarks/fork.c
--
2.16.1
^ permalink raw reply
* Re: [PATCH 2/3] rfi-flush: Make it possible to call setup_rfi_flush() again
From: Michal Suchánek @ 2018-03-06 12:55 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Mauricio Faria de Oliveira, linuxppc-dev
In-Reply-To: <876069flhq.fsf@concordia.ellerman.id.au>
On Tue, 06 Mar 2018 23:15:45 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:
> Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> writes:
>=20
> > Hi Michael, Michal,
> >
> > I got back from vacation. Checking this one. =20
>=20
> Yeah it got stuck in a rut.
>=20
> > On 02/20/2018 02:06 PM, Michal Such=C3=A1nek wrote: =20
> >>> I did it the way I did because otherwise we waste memory on every
> >>> system on earth just to support a use case that we don't actually
> >>> intend for anyone to ever use - ie. migrating from a patched
> >>> machine to an unpatched machine. =20
> >
> > If this thread eventually closes in 'ok, so that memory has to be
> > reserved/wasted anyway', that can be done only in pseries, right?
> >
> > It seems not so much memory for this particular platform/hardware. =20
>=20
> Yes and no. We certainly have pseries machines with 10s of TBs of
> memory, but pseries is also used when running on KVM/Openstack where
> some people are interested in running small VMs - so as usual it's not
> so simple :)
>=20
> But it's not *that* much memory so we should probably stop worrying
> about it and just always allocate the fallback on pseries.
>=20
> I *think* the patch below is all we need, as well as some tweaking of
> patch 2, are you able to test and repost?
>=20
> cheers
>=20
> diff --git a/arch/powerpc/platforms/pseries/setup.c
> b/arch/powerpc/platforms/pseries/setup.c index
> 1a527625acf7..9116824bd7c5 100644 ---
> a/arch/powerpc/platforms/pseries/setup.c +++
> b/arch/powerpc/platforms/pseries/setup.c @@ -468,26 +468,18 @@ static
> void pseries_setup_rfi_flush(void)=20
> /* Enable by default */
> enable =3D true;
> + types =3D L1D_FLUSH_FALLBACK;
> =20
> rc =3D plpar_get_cpu_characteristics(&result);
> if (rc =3D=3D H_SUCCESS) {
> - types =3D L1D_FLUSH_NONE;
> -
> if (result.character & H_CPU_CHAR_L1D_FLUSH_TRIG2)
> types |=3D L1D_FLUSH_MTTRIG;
> if (result.character & H_CPU_CHAR_L1D_FLUSH_ORI30)
> types |=3D L1D_FLUSH_ORI;
> =20
> - /* Use fallback if nothing set in hcall */
> - if (types =3D=3D L1D_FLUSH_NONE)
> - types =3D L1D_FLUSH_FALLBACK;
> -
> if ((!(result.behaviour & H_CPU_BEHAV_L1D_FLUSH_PR))
> || (!(result.behaviour & H_CPU_BEHAV_FAVOUR_SECURITY)))
> enable =3D false;
> - } else {
> - /* Default to fallback if case hcall is not
> available */
> - types =3D L1D_FLUSH_FALLBACK;
> }
> =20
> setup_rfi_flush(types, enable);
>=20
Enabling the fallback flush always looks a bit dodgy but
do_rfi_flush_fixups will overwrite the jump so long any other fixup is
enabled.
Reviewed-by: Michal Such=C3=A1nek <msuchanek@suse.de>
Thanks
Michal
^ permalink raw reply
* Re: [PATCH v3 02/10] include: Move compat_timespec/ timeval to compat_time.h
From: Christian Borntraeger @ 2018-03-06 12:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Deepa Dinamani, Thomas Gleixner, John Stultz, Mark Rutland,
open list:RALINK MIPS ARCHITECTURE, Peter Zijlstra,
Heiko Carstens, Paul Mackerras, H. Peter Anvin, sparclinux, devel,
linux-s390, y2038 Mailman List, Helge Deller,
the arch/x86 maintainers, sebott, James E.J. Bottomley,
Will Deacon, Ingo Molnar, oprofile-list, Catalin Marinas,
Robert Richter, Chris Metcalf, Peter Oberparleiter,
Arnaldo Carvalho de Melo, Julian Wiedmann, Steven Rostedt,
Ursula Braun, gerald.schaefer, Parisc List, gregkh, cohuck,
Linux Kernel Mailing List, Ralf Baechle, Jan Hoeppner,
Stefan Haberland, Martin Schwidefsky, linuxppc-dev, David Miller
In-Reply-To: <CAK8P3a0Gm1L70EaFzJBk0drRNKtX0FE22BHOSrXBgH1wNfKZ5A@mail.gmail.com>
On 03/06/2018 01:46 PM, Arnd Bergmann wrote:
> On Mon, Mar 5, 2018 at 10:30 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>> On 01/16/2018 03:18 AM, Deepa Dinamani wrote:
>>> All the current architecture specific defines for these
>>> are the same. Refactor these common defines to a common
>>> header file.
>>>
>>> The new common linux/compat_time.h is also useful as it
>>> will eventually be used to hold all the defines that
>>> are needed for compat time types that support non y2038
>>> safe types. New architectures need not have to define these
>>> new types as they will only use new y2038 safe syscalls.
>>> This file can be deleted after y2038 when we stop supporting
>>> non y2038 safe syscalls.
>>
>> You are now include a <linux/*.h> from several asm files
>> (
>> arch/arm64/include/asm/stat.h
>> arch/s390/include/asm/elf.h
>> arch/x86/include/asm/ftrace.h
>> arch/x86/include/asm/sys_ia32.h
>> )
>> It works, and it is done in many places, but it looks somewhat weird.
>> Would it make sense to have an asm-generic/compate-time.h instead? Asking for
>> opinions here.
>
> I don't think we have such a rule. If a header file is common to all
> architectures (i.e. no architecture uses a different implementation),
> it should be in include/linux rather than include/asm-generic, regardless
> of whether it can be used by assembler files or not.
>
>>> --- a/drivers/s390/net/qeth_core_main.c
>>> +++ b/drivers/s390/net/qeth_core_main.c
>>> @@ -32,7 +32,7 @@
>>> #include <asm/chpid.h>
>>> #include <asm/io.h>
>>> #include <asm/sysinfo.h>
>>> -#include <asm/compat.h>
>>> +#include <linux/compat.h>
>>> #include <asm/diag.h>
>>> #include <asm/cio.h>
>>> #include <asm/ccwdev.h>
>>
>> Can you move that into the other includes (where all the other <linux/*> includes are.
>
> Good catch, this is definitely a rule we have ;-)
FWIW, this was also broken for
arch/x86/include/asm/sys_ia32.h
^ permalink raw reply
* Re: [PATCH v3 02/10] include: Move compat_timespec/ timeval to compat_time.h
From: Arnd Bergmann @ 2018-03-06 12:46 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Deepa Dinamani, Thomas Gleixner, John Stultz, Mark Rutland,
open list:RALINK MIPS ARCHITECTURE, Peter Zijlstra,
Heiko Carstens, Paul Mackerras, H. Peter Anvin, sparclinux, devel,
linux-s390, y2038 Mailman List, Helge Deller,
the arch/x86 maintainers, sebott, James E.J. Bottomley,
Will Deacon, Ingo Molnar, oprofile-list, Catalin Marinas,
Robert Richter, Chris Metcalf, Peter Oberparleiter,
Arnaldo Carvalho de Melo, Julian Wiedmann, Steven Rostedt,
Ursula Braun, gerald.schaefer, Parisc List, gregkh, cohuck,
Linux Kernel Mailing List, Ralf Baechle, Jan Hoeppner,
Stefan Haberland, Martin Schwidefsky, linuxppc-dev, David Miller
In-Reply-To: <c6fb6676-a8d3-8893-660c-2b9899c5d5ab@de.ibm.com>
On Mon, Mar 5, 2018 at 10:30 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> On 01/16/2018 03:18 AM, Deepa Dinamani wrote:
>> All the current architecture specific defines for these
>> are the same. Refactor these common defines to a common
>> header file.
>>
>> The new common linux/compat_time.h is also useful as it
>> will eventually be used to hold all the defines that
>> are needed for compat time types that support non y2038
>> safe types. New architectures need not have to define these
>> new types as they will only use new y2038 safe syscalls.
>> This file can be deleted after y2038 when we stop supporting
>> non y2038 safe syscalls.
>
> You are now include a <linux/*.h> from several asm files
> (
> arch/arm64/include/asm/stat.h
> arch/s390/include/asm/elf.h
> arch/x86/include/asm/ftrace.h
> arch/x86/include/asm/sys_ia32.h
> )
> It works, and it is done in many places, but it looks somewhat weird.
> Would it make sense to have an asm-generic/compate-time.h instead? Asking for
> opinions here.
I don't think we have such a rule. If a header file is common to all
architectures (i.e. no architecture uses a different implementation),
it should be in include/linux rather than include/asm-generic, regardless
of whether it can be used by assembler files or not.
>> --- a/drivers/s390/net/qeth_core_main.c
>> +++ b/drivers/s390/net/qeth_core_main.c
>> @@ -32,7 +32,7 @@
>> #include <asm/chpid.h>
>> #include <asm/io.h>
>> #include <asm/sysinfo.h>
>> -#include <asm/compat.h>
>> +#include <linux/compat.h>
>> #include <asm/diag.h>
>> #include <asm/cio.h>
>> #include <asm/ccwdev.h>
>
> Can you move that into the other includes (where all the other <linux/*> includes are.
Good catch, this is definitely a rule we have ;-)
Arnd
^ permalink raw reply
* Re: [PATCH 2/3] rfi-flush: Make it possible to call setup_rfi_flush() again
From: Michael Ellerman @ 2018-03-06 12:15 UTC (permalink / raw)
To: Mauricio Faria de Oliveira, Michal Suchánek; +Cc: linuxppc-dev
In-Reply-To: <2bb5a110-3ea4-af5c-81fb-cf5350587dff@linux.vnet.ibm.com>
Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> writes:
> Hi Michael, Michal,
>
> I got back from vacation. Checking this one.
Yeah it got stuck in a rut.
> On 02/20/2018 02:06 PM, Michal Such=C3=A1nek wrote:
>>> I did it the way I did because otherwise we waste memory on every
>>> system on earth just to support a use case that we don't actually
>>> intend for anyone to ever use - ie. migrating from a patched machine
>>> to an unpatched machine.
>
> If this thread eventually closes in 'ok, so that memory has to be
> reserved/wasted anyway', that can be done only in pseries, right?
>
> It seems not so much memory for this particular platform/hardware.
Yes and no. We certainly have pseries machines with 10s of TBs of
memory, but pseries is also used when running on KVM/Openstack where
some people are interested in running small VMs - so as usual it's not
so simple :)
But it's not *that* much memory so we should probably stop worrying
about it and just always allocate the fallback on pseries.
I *think* the patch below is all we need, as well as some tweaking of
patch 2, are you able to test and repost?
cheers
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platform=
s/pseries/setup.c
index 1a527625acf7..9116824bd7c5 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -468,26 +468,18 @@ static void pseries_setup_rfi_flush(void)
=20
/* Enable by default */
enable =3D true;
+ types =3D L1D_FLUSH_FALLBACK;
=20
rc =3D plpar_get_cpu_characteristics(&result);
if (rc =3D=3D H_SUCCESS) {
- types =3D L1D_FLUSH_NONE;
-
if (result.character & H_CPU_CHAR_L1D_FLUSH_TRIG2)
types |=3D L1D_FLUSH_MTTRIG;
if (result.character & H_CPU_CHAR_L1D_FLUSH_ORI30)
types |=3D L1D_FLUSH_ORI;
=20
- /* Use fallback if nothing set in hcall */
- if (types =3D=3D L1D_FLUSH_NONE)
- types =3D L1D_FLUSH_FALLBACK;
-
if ((!(result.behaviour & H_CPU_BEHAV_L1D_FLUSH_PR)) ||
(!(result.behaviour & H_CPU_BEHAV_FAVOUR_SECURITY)))
enable =3D false;
- } else {
- /* Default to fallback if case hcall is not available */
- types =3D L1D_FLUSH_FALLBACK;
}
=20
setup_rfi_flush(types, enable);
^ permalink raw reply related
* Re: [PATCH v3] powerpc/kernel/sysfs: Export ldbar spr to sysfs
From: Michael Ellerman @ 2018-03-06 11:05 UTC (permalink / raw)
To: Anju T Sudhakar; +Cc: linuxppc-dev, linux-kernel, maddy, anju
In-Reply-To: <1510232445-10869-1-git-send-email-anju@linux.vnet.ibm.com>
Anju T Sudhakar <anju@linux.vnet.ibm.com> writes:
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index 4437c70..caefb64 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -757,6 +759,9 @@ static int register_cpu_online(unsigned int cpu)
> device_create_file(s, &pmc_attrs[i]);
>
> #ifdef CONFIG_PPC64
> + if (cpu_has_feature(CPU_FTR_ARCH_300))
> + device_create_file(s, &dev_attr_ldbar);
Is this register readable in supervisor state?
cheers
^ permalink raw reply
* Re: [PATCH 2/3] rfi-flush: Make it possible to call setup_rfi_flush() again
From: Michal Suchánek @ 2018-03-06 10:21 UTC (permalink / raw)
To: Mauricio Faria de Oliveira; +Cc: Michael Ellerman, linuxppc-dev
In-Reply-To: <2bb5a110-3ea4-af5c-81fb-cf5350587dff@linux.vnet.ibm.com>
On Mon, 5 Mar 2018 19:46:12 -0300
Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> wrote:
> Hi Michael, Michal,
>=20
> I got back from vacation. Checking this one.
>=20
> On 02/20/2018 02:06 PM, Michal Such=C3=A1nek wrote:
> >> I did it the way I did because otherwise we waste memory on every
> >> system on earth just to support a use case that we don't actually
> >> intend for anyone to ever use - ie. migrating from a patched
> >> machine to an unpatched machine. =20
>=20
> If this thread eventually closes in 'ok, so that memory has to be
> reserved/wasted anyway', that can be done only in pseries, right?
>=20
> It seems not so much memory for this particular platform/hardware.
Yes, that is what I think too.
>=20
> > If you have multiple hosts running some LPMs and want to update them
> > without shutting down the whole thing I suppose it might easily
> > happen that a machine (re)started on a patched host is migrated to
> > unpatched host. =20
>=20
> Right, but that should be temporary, I think -- after updating some of
> the hosts, the LPAR(s) can be migrated back to one of them, where the
> fallback flush is not required anymore.
Temporary, yes. But for how long?
Also is it possible to migrate between P8 and P9 in P8 compat mode?
AFAIK on P9 fallback mode is used exclusively so far.
>=20
> >> I think I'm inclined to leave it the way it is, unless you feel
> >> strongly about it Michal? =20
>=20
> > I think it would be more user friendly to either support the
> > fallback method 100% or remove it and require patched firmware. =20
>=20
> I beg to disagree. Since the matter is a security issue, the option
> of still have some sort of fix that works on unpatched firmware does
> look good and friendly to users (rather than require 'you _must_ get
> the firmware update') IMHO.
Specifically because this is a security issue user friendly is that
user can tell clearly if the fix is applied or not. If the kernel tells
the user the fix is applied and later it tells "sorry, could not
allocate more memory. disabling fix" this is really unfriendly.=20
Thanks
Michal
^ permalink raw reply
* [FIX PATCH] powerpc/pseries: Fix vector5 in ibm architecture vector table
From: Bharata B Rao @ 2018-03-06 8:14 UTC (permalink / raw)
To: linuxppc-dev; +Cc: nfont, mwb, mpe, Bharata B Rao
With ibm,dynamic-memory-v2 and ibm,drc-info coming around the
same time, byte22 in vector5 of ibm architecture vector table
got set twice separately. The end result is that guest kernel
isn't advertising support for ibm,dynamic-memory-v2.
Fix this by removing the duplicate assignment of byte22.
Fixes: 02ef6dd8109b5 (powerpc: Enable support for ibm,drc-info devtree property)
Fixes: c7a3275e0f9e4 (powerpc/pseries: Revert support for ibm,drc-info devtree property)
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
arch/powerpc/kernel/prom_init.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index d22c41c26bb3..acf4b2e0530c 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -874,7 +874,6 @@ struct ibm_arch_vec __cacheline_aligned ibm_architecture_vec = {
.mmu = 0,
.hash_ext = 0,
.radix_ext = 0,
- .byte22 = 0,
},
/* option vector 6: IBM PAPR hints */
--
2.14.3
^ permalink raw reply related
* Re: [PATCH 9/9] powerpc/eeh: Add eeh_state_active() helper
From: Russell Currey @ 2018-03-06 5:49 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <2c6264797f9efd44679fa8c8ac4f8beb11a38c18.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 11:00 +1100, Sam Bobroff wrote:
> Checking for a "fully active" device state requires testing two flag
> bits, which is open coded in several places, so add a function to do
> it.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
Thanks for the patches.
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 8/9] powerpc/eeh: Factor out common code eeh_reset_device()
From: Russell Currey @ 2018-03-06 5:48 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <fe5673642328c2e11fee269bfc7987095311f25c.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 10:59 +1100, Sam Bobroff wrote:
> The caller will always pass NULL for 'rmv_data' when
> 'eeh_aware_driver' is true, so the first two calls to
> eeh_pe_dev_traverse() can be combined without changing behaviour as
> can the two arms of the final 'if' block.
>
> This should not change behaviour.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
Reviewed-by: Russell Currey <ruscur@russell.cc>
minor nitpick below
> arch/powerpc/kernel/eeh_driver.c | 36 ++++++++++++----------------
> --------
> 1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c
> b/arch/powerpc/kernel/eeh_driver.c
> index 1272f2c8cbd2..39d560e9f071 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -647,16 +647,12 @@ static int eeh_reset_device(bool
> eeh_aware_driver,
> * into pci_hp_add_devices().
> */
> eeh_pe_state_mark(pe, EEH_PE_KEEP);
> - if (!eeh_aware_driver) {
> - if (pe->type & EEH_PE_VF) {
> - eeh_pe_dev_traverse(pe, eeh_rmv_device,
> NULL);
> - } else {
> - pci_lock_rescan_remove();
> - pci_hp_remove_devices(bus);
> - pci_unlock_rescan_remove();
> - }
> - } else {
> + if (eeh_aware_driver || (pe->type & EEH_PE_VF)) {
> eeh_pe_dev_traverse(pe, eeh_rmv_device, rmv_data);
> + } else {
> + pci_lock_rescan_remove();
> + pci_hp_remove_devices(bus);
> + pci_unlock_rescan_remove();
> }
>
> /*
> @@ -691,8 +687,9 @@ static int eeh_reset_device(bool
> eeh_aware_driver,
> * the device up before the scripts have taken it down,
> * potentially weird things happen.
> */
> - if (!eeh_aware_driver) {
> - pr_info("EEH: Sleep 5s ahead of complete
> hotplug\n");
> + if (!eeh_aware_driver || rmv_data->removed) {
> + pr_info("EEH: Sleep 5s ahead of %s hotplug\n",
> + (eeh_aware_driver ? "partial" :
> "complete"));
> ssleep(5);
>
> /*
> @@ -700,24 +697,15 @@ static int eeh_reset_device(bool
> eeh_aware_driver,
> * PE. We should disconnect it so the binding can be
> * rebuilt when adding PCI devices.
> */
> - edev = list_first_entry(&pe->edevs, struct eeh_dev,
> list);
> - eeh_pe_traverse(pe, eeh_pe_detach_dev, NULL);
> - if (pe->type & EEH_PE_VF) {
> - eeh_add_virt_device(edev, NULL);
> - } else {
> - eeh_pe_state_clear(pe, EEH_PE_PRI_BUS);
> - pci_hp_add_devices(bus);
> - }
> - } else if (rmv_data->removed) {
> - pr_info("EEH: Sleep 5s ahead of partial hotplug\n");
> - ssleep(5);
> -
> edev = list_first_entry(&pe->edevs, struct eeh_dev,
> list);
> eeh_pe_traverse(pe, eeh_pe_detach_dev, NULL);
> if (pe->type & EEH_PE_VF)
> eeh_add_virt_device(edev, NULL);
> - else
> + else {
Put brackets around the if block now that the else block isn't a one
liner
> + if (!eeh_aware_driver)
> + eeh_pe_state_clear(pe,
> EEH_PE_PRI_BUS);
> pci_hp_add_devices(bus);
> + }
> }
> eeh_pe_state_clear(pe, EEH_PE_KEEP);
>
^ permalink raw reply
* RE: [PATCH 5/6] dma-mapping: support fsl-mc bus
From: Nipun Gupta @ 2018-03-06 4:41 UTC (permalink / raw)
To: Robin Murphy, Christoph Hellwig
Cc: will.deacon@arm.com, mark.rutland@arm.com,
catalin.marinas@arm.com, iommu@lists.linux-foundation.org,
robh+dt@kernel.org, m.szyprowski@samsung.com,
gregkh@linuxfoundation.org, joro@8bytes.org, Leo Li,
shawnguo@kernel.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linuxppc-dev@lists.ozlabs.org, Bharat Bhushan, stuyoder@gmail.com,
Laurentiu Tudor
In-Reply-To: <1729ae21-d08c-b413-51a3-f22c394b388d@arm.com>
DQoNCj4gRnJvbTogUm9iaW4gTXVycGh5IFttYWlsdG86cm9iaW4ubXVycGh5QGFybS5jb21dDQo+
IFNlbnQ6IFR1ZXNkYXksIE1hcmNoIDA2LCAyMDE4IDA6MjINCj4gDQo+IE9uIDA1LzAzLzE4IDE4
OjM5LCBDaHJpc3RvcGggSGVsbHdpZyB3cm90ZToNCj4gPiBPbiBNb24sIE1hciAwNSwgMjAxOCBh
dCAwMzo0ODozMlBNICswMDAwLCBSb2JpbiBNdXJwaHkgd3JvdGU6DQo+ID4+IFVuZm9ydHVuYXRl
bHkgZm9yIHVzLCBmc2wtbWMgaXMgY29uY2VwdHVhbGx5IHJhdGhlciBsaWtlIFBDSSBpbiB0aGF0
IGl0J3MNCj4gPj4gc29mdHdhcmUtZGlzY292ZXJhYmxlIGFuZCB0aGUgb25seSB0aGluZyBkZXNj
cmliZWQgaW4gRFQgaXMgdGhlIGJ1cyAiaG9zdCIsDQo+ID4+IHRodXMgd2UgbmVlZCB0aGUgc2Ft
ZSBzb3J0IG9mIHRoaW5nIGFzIGZvciBQQ0kgdG8gbWFwIGZyb20gdGhlIGNoaWxkDQo+ID4+IGRl
dmljZXMgYmFjayB0byB0aGUgYnVzIHJvb3QgaW4gb3JkZXIgdG8gZmluZCB0aGUgYXBwcm9wcmlh
dGUgZmlybXdhcmUNCj4gPj4gbm9kZS4gV29yc2UgdGhhbiBQQ0ksIHRob3VnaCwgd2Ugd291bGRu
J3QgZXZlbiBoYXZlIHRoZSBvcHRpb24gb2YNCj4gPj4gZGVzY3JpYmluZyBjaGlsZCBkZXZpY2Vz
IHN0YXRpY2FsbHkgaW4gZmlybXdhcmUgYXQgYWxsLCBzaW5jZSBpdCdzIGFjdHVhbGx5DQo+ID4+
IG9uZSBvZiB0aGVzZSBydW50aW1lLWNvbmZpZ3VyYWJsZSAiYnVpbGQgeW91ciBvd24gbmV0d29y
ayBhY2NlbGVyYXRvciINCj4gPj4gaGFyZHdhcmUgcG9vbHMgd2hlcmUgdXNlcnNwYWNlIGdldHMg
dG8gY3JlYXRlIGFuZCBkZXN0cm95ICJkZXZpY2VzIiBhcyBpdA0KPiA+PiBsaWtlcy4NCj4gPg0K
PiA+IEkgcmVhbGx5IGhhdGUgdGhlIFBDSSBzcGVjaWFsIGNhc2UganVzdCBhcyBtdWNoLiAgTWF5
YmUgd2UganVzdA0KPiA+IG5lZWQgYSBkbWFfY29uZmlndXJlIG1ldGhvZCBvbiB0aGUgYnVzLCBh
bmQgbW92ZSBQQ0kgYXMgd2VsbCBhcyBmc2wtbWMNCj4gPiB0byBpdC4NCj4gDQo+IEhtbSwgb24g
cmVmbGVjdGlvbiwgMTAwJSBhY2sgdG8gdGhhdCBpZGVhLiBJdCB3b3VsZCBuZWF0bHkgc3VwZXJz
ZWRlDQo+IGJ1cy0+Zm9yY2VfZG1hICphbmQqIG1lYW4gdGhhdCB3ZSBkb24ndCBoYXZlIHRvIGVm
ZmVjdGl2ZWx5IHB1bGwgcGNpLmgNCj4gaW50byBldmVyeXRoaW5nLCB3aGljaCBJJ3ZlIG5ldmVy
IGxpa2VkLiBJbiBoaW5kc2lnaHQgZG1hX2NvbmZpZ3VyZSgpDQo+IGRvZXMgZmVlbCBsaWtlIGl0
J3MgZ3Jvd24gaW50byB0aGlzIG9kZCBjaG9rZSBwb2ludCB3aGVyZSB3ZSBtdW5nZQ0KPiBldmVy
eXRoaW5nIGluIGp1c3QgZm9yIGl0IHRvIGF3a3dhcmRseSB1bnBpY2sgdGhpbmdzIGFnYWluLg0K
PiANCj4gUm9iaW4uDQoNCisxIHRvIHRoZSBpZGVhLg0KDQpTb3JyeSBmb3IgYXNraW5nIGEgdHJp
dmlhbCBxdWVzdGlvbiAtIGxvb2tpbmcgaW50byBkbWFfY29uZmlndXJlKCkgSSBzZWUgdGhhdA0K
UENJIGlzIHVzZWQgaW4gdGhlIHN0YXJ0IGFuZCB0aGUgZW5kIG9mIHRoZSBBUEkuDQpJbiB0aGUg
ZW5kIHBhcnQgcGNpX3B1dF9ob3N0X2JyaWRnZV9kZXZpY2UoKSBpcyBjYWxsZWQuDQpTbyBhcmUg
dHdvIGJ1cyBjYWxsYmFja3Mgc29tZXRoaW5nIGxpa2UgJ2RtYV9jb25maWdfc3RhcnQnICYgJ2Rt
YV9jb25maWdfZW5kJw0Kd2lsbCBiZSByZXF1aXJlZCB3aGVyZSB0aGUgZm9ybWVyIG9uZSB3aWxs
IHJldHVybiAiZG1hX2RldiI/DQoNClJlZ2FyZHMsDQpOaXB1bg0K
^ permalink raw reply
* Re: [PATCH 7/9] powerpc/eeh: Remove always-true tests in eeh_reset_device()
From: Russell Currey @ 2018-03-06 4:38 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <62fba6f13022a6d3cfb1e6468560abce10b0abfc.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 10:59 +1100, Sam Bobroff wrote:
> eeh_reset_device() tests the value of 'bus' more than once but the
> only caller, eeh_handle_normal_device() does this test itself and
> will
> never pass NULL.
>
> So, remove the dead tests.
>
> This should not change behaviour.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 6/9] powerpc/eeh: Clarify arguments to eeh_reset_device()
From: Russell Currey @ 2018-03-06 4:37 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <c9191611543d221d5f003fad4a68dd5f86839404.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 10:59 +1100, Sam Bobroff wrote:
> It is currently difficult to understand the behaviour of
> eeh_reset_device() due to the way it's parameters are used. In
> particular, when 'bus' is NULL, it's value is still necessary so the
> same value is looked up again locally under a different name
> ('frozen_bus') but behaviour is changed.
>
> To clarify this, add a new parameter 'eeh_aware_driver', and have the
> caller set it when it would have passed NULL for 'bus' and always
> pass
> a value for 'bus'. Then change any test that was on 'bus' to one on
> '!eeh_aware_driver' and replace uses of 'frozen_bus' with 'bus'.
>
> Also update the function's comment.
>
> This should not change behaviour.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
> arch/powerpc/kernel/eeh_driver.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c
> b/arch/powerpc/kernel/eeh_driver.c
> index cb584d72b0a5..6c3577133223 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -619,17 +619,19 @@ int eeh_pe_reset_and_recover(struct eeh_pe *pe)
>
> /**
> * eeh_reset_device - Perform actual reset of a pci slot
> + * @eeh_aware_driver: Does the device's driver provide EEH support?
> * @pe: EEH PE
> * @bus: PCI bus corresponding to the isolcated slot
> + * @rmv_data: Optional, list to record removed devices
> *
> * This routine must be called to do reset on the indicated PE.
> * During the reset, udev might be invoked because those affected
> * PCI devices will be removed and then added.
> */
Just a nitpick here, I would prefer that the PE remain the first
argument of the function since that's one thing that's pretty standard
across the EEH implementation. I'd also say that "driver_aware" is
clearer than "aware_driver".
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 4/9] powerpc/eeh: Remove misleading test in eeh_handle_normal_event()
From: Daniel Axtens @ 2018-03-06 2:14 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <52c15c0414d248981e251386384ed2a716315003.1520294174.git.sam.bobroff@au1.ibm.com>
Sam Bobroff <sam.bobroff@au1.ibm.com> writes:
> Remove a test that checks if "frozen_bus" is NULL, because it cannot
> have changed since it was tested at the start of the function and so
> must be true here.
As far as I can tell this was added back in 2012 in 9b3c76f08122f. As
far as I can tell it couldn't be NULL back then either. Weird.
LGTM, but I'm not super comfortable chnging something I don't understand
so I will leave the formal review for now.
Regards,
Daniel
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
> arch/powerpc/kernel/eeh_driver.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index 5b7a5ed4db4d..04a5d9db5499 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -930,20 +930,18 @@ void eeh_handle_normal_event(struct eeh_pe *pe)
> * all removed devices correctly to avoid access
> * the their PCI config any more.
> */
> - if (frozen_bus) {
> - if (pe->type & EEH_PE_VF) {
> - eeh_pe_dev_traverse(pe, eeh_rmv_device, NULL);
> - eeh_pe_dev_mode_mark(pe, EEH_DEV_REMOVED);
> - } else {
> - eeh_pe_state_clear(pe, EEH_PE_PRI_BUS);
> - eeh_pe_dev_mode_mark(pe, EEH_DEV_REMOVED);
> + if (pe->type & EEH_PE_VF) {
> + eeh_pe_dev_traverse(pe, eeh_rmv_device, NULL);
> + eeh_pe_dev_mode_mark(pe, EEH_DEV_REMOVED);
> + } else {
> + eeh_pe_state_clear(pe, EEH_PE_PRI_BUS);
> + eeh_pe_dev_mode_mark(pe, EEH_DEV_REMOVED);
>
> - pci_lock_rescan_remove();
> - pci_hp_remove_devices(frozen_bus);
> - pci_unlock_rescan_remove();
> - /* The passed PE should no longer be used */
> - return;
> - }
> + pci_lock_rescan_remove();
> + pci_hp_remove_devices(frozen_bus);
> + pci_unlock_rescan_remove();
> + /* The passed PE should no longer be used */
> + return;
> }
> final:
> eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
> --
> 2.16.1.74.g9b0b1f47b
^ permalink raw reply
* Re: [PATCH 2/9] powerpc/eeh: Manage EEH_PE_RECOVERING inside eeh_handle_normal_event()
From: Daniel Axtens @ 2018-03-06 1:47 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <163b24ef504d753f4abf1510d9193962bbac32b7.1520294174.git.sam.bobroff@au1.ibm.com>
Hi Sam,
> Currently the EEH_PE_RECOVERING flag for a PE is managed by both the
> caller and callee of eeh_handle_normal_event() (among other places not
> considered here). This is complicated by the fact that the PE may
> or may not have been invalidated by the call.
So, I applied this patch and looked at those places. They're now
restricted to eeh_driver.c and eeh.c.
The only other place it's marked is
eeh_driver.c::eeh_pe_reset_and_recover(), which is itself only called
from eeh.c::eeh_pe_change_owner(). That is only called from 2 places,
both in eeh.c - eeh_dev_open() and eeh_dev_release(). I have not yet
wrapped my head around the logic of that.
I suspect the entire logic can have some more simplifications, but this
is definitely a good step.
I also read through your patch and have no other comments, so:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Regards,
Daniel
>
> So move the callee's handling into eeh_handle_normal_event(), which
> clarifies it and allows the return type to be changed to void (because
> it no longer needs to indicate at the PE has been invalidated).
>
> This should not change behaviour except in eeh_event_handler() where
> it was previously possible to cause eeh_pe_state_clear() to be called
> on an invalid PE, which is now avoided.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> ---
> arch/powerpc/include/asm/eeh_event.h | 2 +-
> arch/powerpc/kernel/eeh_driver.c | 29 +++++++++++------------------
> arch/powerpc/kernel/eeh_event.c | 2 --
> 3 files changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/eeh_event.h b/arch/powerpc/include/asm/eeh_event.h
> index 0a168038882d..9884e872686f 100644
> --- a/arch/powerpc/include/asm/eeh_event.h
> +++ b/arch/powerpc/include/asm/eeh_event.h
> @@ -34,7 +34,7 @@ struct eeh_event {
> int eeh_event_init(void);
> int eeh_send_failure_event(struct eeh_pe *pe);
> void eeh_remove_event(struct eeh_pe *pe, bool force);
> -bool eeh_handle_normal_event(struct eeh_pe *pe);
> +void eeh_handle_normal_event(struct eeh_pe *pe);
> void eeh_handle_special_event(void);
>
> #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index 51b21c97910f..5b7a5ed4db4d 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -733,7 +733,8 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
>
> /**
> * eeh_handle_normal_event - Handle EEH events on a specific PE
> - * @pe: EEH PE
> + * @pe: EEH PE - which should not be used after we return, as it may
> + * have been invalidated.
> *
> * Attempts to recover the given PE. If recovery fails or the PE has failed
> * too many times, remove the PE.
> @@ -750,10 +751,8 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
> * & devices under this slot, and then finally restarting the device
> * drivers (which cause a second set of hotplug events to go out to
> * userspace).
> - *
> - * Returns true if @pe should no longer be used, else false.
> */
> -bool eeh_handle_normal_event(struct eeh_pe *pe)
> +void eeh_handle_normal_event(struct eeh_pe *pe)
> {
> struct pci_bus *frozen_bus;
> struct eeh_dev *edev, *tmp;
> @@ -765,9 +764,11 @@ bool eeh_handle_normal_event(struct eeh_pe *pe)
> if (!frozen_bus) {
> pr_err("%s: Cannot find PCI bus for PHB#%x-PE#%x\n",
> __func__, pe->phb->global_number, pe->addr);
> - return false;
> + return;
> }
>
> + eeh_pe_state_mark(pe, EEH_PE_RECOVERING);
> +
> eeh_pe_update_time_stamp(pe);
> pe->freeze_count++;
> if (pe->freeze_count > eeh_max_freezes) {
> @@ -904,7 +905,7 @@ bool eeh_handle_normal_event(struct eeh_pe *pe)
> pr_info("EEH: Notify device driver to resume\n");
> eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
>
> - return false;
> + goto final;
>
> hard_fail:
> /*
> @@ -940,12 +941,12 @@ bool eeh_handle_normal_event(struct eeh_pe *pe)
> pci_lock_rescan_remove();
> pci_hp_remove_devices(frozen_bus);
> pci_unlock_rescan_remove();
> -
> /* The passed PE should no longer be used */
> - return true;
> + return;
> }
> }
> - return false;
> +final:
> + eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
> }
>
> /**
> @@ -1018,15 +1019,7 @@ void eeh_handle_special_event(void)
> */
> if (rc == EEH_NEXT_ERR_FROZEN_PE ||
> rc == EEH_NEXT_ERR_FENCED_PHB) {
> - /*
> - * eeh_handle_normal_event() can make the PE stale if it
> - * determines that the PE cannot possibly be recovered.
> - * Don't modify the PE state if that's the case.
> - */
> - if (eeh_handle_normal_event(pe))
> - continue;
> -
> - eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
> + eeh_handle_normal_event(pe);
> } else {
> pci_lock_rescan_remove();
> list_for_each_entry(hose, &hose_list, list_node) {
> diff --git a/arch/powerpc/kernel/eeh_event.c b/arch/powerpc/kernel/eeh_event.c
> index 872bcfe8f90e..61c9356bf9c9 100644
> --- a/arch/powerpc/kernel/eeh_event.c
> +++ b/arch/powerpc/kernel/eeh_event.c
> @@ -73,7 +73,6 @@ static int eeh_event_handler(void * dummy)
> /* We might have event without binding PE */
> pe = event->pe;
> if (pe) {
> - eeh_pe_state_mark(pe, EEH_PE_RECOVERING);
> if (pe->type & EEH_PE_PHB)
> pr_info("EEH: Detected error on PHB#%x\n",
> pe->phb->global_number);
> @@ -82,7 +81,6 @@ static int eeh_event_handler(void * dummy)
> "PHB#%x-PE#%x\n",
> pe->phb->global_number, pe->addr);
> eeh_handle_normal_event(pe);
> - eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
> } else {
> eeh_handle_special_event();
> }
> --
> 2.16.1.74.g9b0b1f47b
^ permalink raw reply
* Re: [PATCH 1/9] powerpc/eeh: Remove eeh_handle_event()
From: Daniel Axtens @ 2018-03-06 1:08 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <d3d01073da02a5eb844cc8998fdac602b8121b62.1520294174.git.sam.bobroff@au1.ibm.com>
Hi Sam,
> The function eeh_handle_event(pe) does nothing other than switching
> between calling eeh_handle_normal_event(pe) and
> eeh_handle_special_event(). However it is only called in two places,
> one where pe can't be NULL and the other where it must be NULL (see
> eeh_event_handler()) so it does nothing but obscure the flow of
> control.
I've verified this.
>
> So, remove it.
Sounds good.
> + * While PHB detects address or data parity errors on particular PCI
> + * slot, the associated PE will be frozen. Besides, DMA's occurring
> + * to wild addresses (which usually happen due to bugs in device
> + * drivers or in PCI adapter firmware) can cause EEH error. #SERR,
> + * #PERR or other misc PCI-related errors also can trigger EEH errors.
> + *
> + * Recovery process consists of unplugging the device driver (which
> + * generated hotplug events to userspace), then issuing a PCI #RST to
> + * the device, then reconfiguring the PCI config space for all bridges
> + * & devices under this slot, and then finally restarting the device
> + * drivers (which cause a second set of hotplug events to go out to
> + * userspace).
> + *
So this is the comment from eeh_handle_event. This seems as good a place
as any to put it. (At some point someone should check if it lines up
well with Documentation/powerpc/eeh-pci-error-recovery.txt but it can wait.)
In conclusion:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Thanks Sam!
Regards,
Daniel
> * Returns true if @pe should no longer be used, else false.
> */
> -static bool eeh_handle_normal_event(struct eeh_pe *pe)
> +bool eeh_handle_normal_event(struct eeh_pe *pe)
> {
> struct pci_bus *frozen_bus;
> struct eeh_dev *edev, *tmp;
> @@ -942,7 +955,7 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
> * specific PE. Iterates through possible failures and handles them as
> * necessary.
> */
> -static void eeh_handle_special_event(void)
> +void eeh_handle_special_event(void)
> {
> struct eeh_pe *pe, *phb_pe;
> struct pci_bus *bus;
> @@ -1049,28 +1062,3 @@ static void eeh_handle_special_event(void)
> break;
> } while (rc != EEH_NEXT_ERR_NONE);
> }
> -
> -/**
> - * eeh_handle_event - Reset a PCI device after hard lockup.
> - * @pe: EEH PE
> - *
> - * While PHB detects address or data parity errors on particular PCI
> - * slot, the associated PE will be frozen. Besides, DMA's occurring
> - * to wild addresses (which usually happen due to bugs in device
> - * drivers or in PCI adapter firmware) can cause EEH error. #SERR,
> - * #PERR or other misc PCI-related errors also can trigger EEH errors.
> - *
> - * Recovery process consists of unplugging the device driver (which
> - * generated hotplug events to userspace), then issuing a PCI #RST to
> - * the device, then reconfiguring the PCI config space for all bridges
> - * & devices under this slot, and then finally restarting the device
> - * drivers (which cause a second set of hotplug events to go out to
> - * userspace).
> - */
> -void eeh_handle_event(struct eeh_pe *pe)
> -{
> - if (pe)
> - eeh_handle_normal_event(pe);
> - else
> - eeh_handle_special_event();
> -}
> diff --git a/arch/powerpc/kernel/eeh_event.c b/arch/powerpc/kernel/eeh_event.c
> index accbf8b5fd46..872bcfe8f90e 100644
> --- a/arch/powerpc/kernel/eeh_event.c
> +++ b/arch/powerpc/kernel/eeh_event.c
> @@ -81,10 +81,10 @@ static int eeh_event_handler(void * dummy)
> pr_info("EEH: Detected PCI bus error on "
> "PHB#%x-PE#%x\n",
> pe->phb->global_number, pe->addr);
> - eeh_handle_event(pe);
> + eeh_handle_normal_event(pe);
> eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
> } else {
> - eeh_handle_event(NULL);
> + eeh_handle_special_event();
> }
>
> kfree(event);
> --
> 2.16.1.74.g9b0b1f47b
^ permalink raw reply
* Re: [PATCH 5/9] powerpc/eeh: Rename frozen_bus to bus in eeh_handle_normal_event()
From: Russell Currey @ 2018-03-06 0:57 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <a8d5f7792b761721852b68363c68519c6c8b62ff.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 10:59 +1100, Sam Bobroff wrote:
> The name "frozen_bus" is misleading: it's not necessarily frozen,
> it's
> just the PE's PCI bus.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 4/9] powerpc/eeh: Remove misleading test in eeh_handle_normal_event()
From: Russell Currey @ 2018-03-06 0:56 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <52c15c0414d248981e251386384ed2a716315003.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 10:59 +1100, Sam Bobroff wrote:
> Remove a test that checks if "frozen_bus" is NULL, because it cannot
> have changed since it was tested at the start of the function and so
> must be true here.
It could potentially be modified since it gets passed into
eeh_reset_device() but it doesn't look like it does anywhere.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 3/9] powerpc/eeh: Fix misleading comment in __eeh_addr_cache_get_device()
From: Russell Currey @ 2018-03-06 0:49 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <a08f33ca4af93b1a6ec066b8091953c855d52f45.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 10:59 +1100, Sam Bobroff wrote:
> Commit "0ba178888b05 powerpc/eeh: Remove reference to PCI device"
> removed a call to pci_dev_get() from __eeh_addr_cache_get_device()
> but
> did not update the comment to match.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 2/9] powerpc/eeh: Manage EEH_PE_RECOVERING inside eeh_handle_normal_event()
From: Russell Currey @ 2018-03-06 0:48 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <163b24ef504d753f4abf1510d9193962bbac32b7.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 10:58 +1100, Sam Bobroff wrote:
> Currently the EEH_PE_RECOVERING flag for a PE is managed by both the
> caller and callee of eeh_handle_normal_event() (among other places
> not
> considered here). This is complicated by the fact that the PE may
> or may not have been invalidated by the call.
>
> So move the callee's handling into eeh_handle_normal_event(), which
> clarifies it and allows the return type to be changed to void
> (because
> it no longer needs to indicate at the PE has been invalidated).
>
> This should not change behaviour except in eeh_event_handler() where
> it was previously possible to cause eeh_pe_state_clear() to be called
> on an invalid PE, which is now avoided.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH 1/9] powerpc/eeh: Remove eeh_handle_event()
From: Russell Currey @ 2018-03-06 0:44 UTC (permalink / raw)
To: Sam Bobroff, linuxppc-dev
In-Reply-To: <d3d01073da02a5eb844cc8998fdac602b8121b62.1520294174.git.sam.bobroff@au1.ibm.com>
On Tue, 2018-03-06 at 10:58 +1100, Sam Bobroff wrote:
> The function eeh_handle_event(pe) does nothing other than switching
> between calling eeh_handle_normal_event(pe) and
> eeh_handle_special_event(). However it is only called in two places,
> one where pe can't be NULL and the other where it must be NULL (see
> eeh_event_handler()) so it does nothing but obscure the flow of
> control.
>
> So, remove it.
>
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
Reviewed-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* [PATCH 9/9] powerpc/eeh: Add eeh_state_active() helper
From: Sam Bobroff @ 2018-03-06 0:00 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <cover.1520294174.git.sam.bobroff@au1.ibm.com>
Checking for a "fully active" device state requires testing two flag
bits, which is open coded in several places, so add a function to do
it.
Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
arch/powerpc/include/asm/eeh.h | 6 ++++++
arch/powerpc/kernel/eeh.c | 19 ++++++-------------
arch/powerpc/platforms/powernv/eeh-powernv.c | 9 ++-------
3 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index fd37cc101f4f..c2266ca61853 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -256,6 +256,12 @@ static inline void eeh_serialize_unlock(unsigned long flags)
raw_spin_unlock_irqrestore(&confirm_error_lock, flags);
}
+static inline bool eeh_state_active(int state)
+{
+ return (state & (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE))
+ == (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE);
+}
+
typedef void *(*eeh_traverse_func)(void *data, void *flag);
void eeh_set_pe_aux_size(int size);
int eeh_phb_pe_create(struct pci_controller *phb);
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 2b9df0040d6b..bc640e4c5ca5 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -394,9 +394,7 @@ static int eeh_phb_check_failure(struct eeh_pe *pe)
/* Check PHB state */
ret = eeh_ops->get_state(phb_pe, NULL);
if ((ret < 0) ||
- (ret == EEH_STATE_NOT_SUPPORT) ||
- (ret & (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE)) ==
- (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE)) {
+ (ret == EEH_STATE_NOT_SUPPORT) || eeh_state_active(ret)) {
ret = 0;
goto out;
}
@@ -433,7 +431,6 @@ static int eeh_phb_check_failure(struct eeh_pe *pe)
int eeh_dev_check_failure(struct eeh_dev *edev)
{
int ret;
- int active_flags = (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE);
unsigned long flags;
struct device_node *dn;
struct pci_dev *dev;
@@ -525,8 +522,7 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
* state, PE is in good state.
*/
if ((ret < 0) ||
- (ret == EEH_STATE_NOT_SUPPORT) ||
- ((ret & active_flags) == active_flags)) {
+ (ret == EEH_STATE_NOT_SUPPORT) || eeh_state_active(ret)) {
eeh_stats.false_positives++;
pe->false_positives++;
rc = 0;
@@ -546,8 +542,7 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
/* Frozen parent PE ? */
ret = eeh_ops->get_state(parent_pe, NULL);
- if (ret > 0 &&
- (ret & active_flags) != active_flags)
+ if (ret > 0 && !eeh_state_active(ret))
pe = parent_pe;
/* Next parent level */
@@ -888,7 +883,6 @@ static void *eeh_set_dev_freset(void *data, void *flag)
*/
int eeh_pe_reset_full(struct eeh_pe *pe)
{
- int active_flags = (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE);
int reset_state = (EEH_PE_RESET | EEH_PE_CFG_BLOCKED);
int type = EEH_RESET_HOT;
unsigned int freset = 0;
@@ -919,7 +913,7 @@ int eeh_pe_reset_full(struct eeh_pe *pe)
/* Wait until the PE is in a functioning state */
state = eeh_ops->wait_state(pe, PCI_BUS_RESET_WAIT_MSEC);
- if ((state & active_flags) == active_flags)
+ if (eeh_state_active(state))
break;
if (state < 0) {
@@ -1352,16 +1346,15 @@ static int eeh_pe_change_owner(struct eeh_pe *pe)
struct eeh_dev *edev, *tmp;
struct pci_dev *pdev;
struct pci_device_id *id;
- int flags, ret;
+ int ret;
/* Check PE state */
- flags = (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE);
ret = eeh_ops->get_state(pe, NULL);
if (ret < 0 || ret == EEH_STATE_NOT_SUPPORT)
return 0;
/* Unfrozen PE, nothing to do */
- if ((ret & flags) == flags)
+ if (eeh_state_active(ret))
return 0;
/* Frozen PE, check if it needs PE level reset */
diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
index 33c86c1a1720..ddfc3544d285 100644
--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
@@ -1425,11 +1425,8 @@ static int pnv_eeh_get_pe(struct pci_controller *hose,
dev_pe = dev_pe->parent;
while (dev_pe && !(dev_pe->type & EEH_PE_PHB)) {
int ret;
- int active_flags = (EEH_STATE_MMIO_ACTIVE |
- EEH_STATE_DMA_ACTIVE);
-
ret = eeh_ops->get_state(dev_pe, NULL);
- if (ret <= 0 || (ret & active_flags) == active_flags) {
+ if (ret <= 0 || eeh_state_active(ret)) {
dev_pe = dev_pe->parent;
continue;
}
@@ -1463,7 +1460,6 @@ static int pnv_eeh_next_error(struct eeh_pe **pe)
struct eeh_pe *phb_pe, *parent_pe;
__be64 frozen_pe_no;
__be16 err_type, severity;
- int active_flags = (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE);
long rc;
int state, ret = EEH_NEXT_ERR_NONE;
@@ -1626,8 +1622,7 @@ static int pnv_eeh_next_error(struct eeh_pe **pe)
/* Frozen parent PE ? */
state = eeh_ops->get_state(parent_pe, NULL);
- if (state > 0 &&
- (state & active_flags) != active_flags)
+ if (state > 0 && !eeh_state_active(state))
*pe = parent_pe;
/* Next parent level */
--
2.16.1.74.g9b0b1f47b
^ permalink raw reply related
* [PATCH 8/9] powerpc/eeh: Factor out common code eeh_reset_device()
From: Sam Bobroff @ 2018-03-05 23:59 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <cover.1520294174.git.sam.bobroff@au1.ibm.com>
The caller will always pass NULL for 'rmv_data' when
'eeh_aware_driver' is true, so the first two calls to
eeh_pe_dev_traverse() can be combined without changing behaviour as
can the two arms of the final 'if' block.
This should not change behaviour.
Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
arch/powerpc/kernel/eeh_driver.c | 36 ++++++++++++------------------------
1 file changed, 12 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 1272f2c8cbd2..39d560e9f071 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -647,16 +647,12 @@ static int eeh_reset_device(bool eeh_aware_driver,
* into pci_hp_add_devices().
*/
eeh_pe_state_mark(pe, EEH_PE_KEEP);
- if (!eeh_aware_driver) {
- if (pe->type & EEH_PE_VF) {
- eeh_pe_dev_traverse(pe, eeh_rmv_device, NULL);
- } else {
- pci_lock_rescan_remove();
- pci_hp_remove_devices(bus);
- pci_unlock_rescan_remove();
- }
- } else {
+ if (eeh_aware_driver || (pe->type & EEH_PE_VF)) {
eeh_pe_dev_traverse(pe, eeh_rmv_device, rmv_data);
+ } else {
+ pci_lock_rescan_remove();
+ pci_hp_remove_devices(bus);
+ pci_unlock_rescan_remove();
}
/*
@@ -691,8 +687,9 @@ static int eeh_reset_device(bool eeh_aware_driver,
* the device up before the scripts have taken it down,
* potentially weird things happen.
*/
- if (!eeh_aware_driver) {
- pr_info("EEH: Sleep 5s ahead of complete hotplug\n");
+ if (!eeh_aware_driver || rmv_data->removed) {
+ pr_info("EEH: Sleep 5s ahead of %s hotplug\n",
+ (eeh_aware_driver ? "partial" : "complete"));
ssleep(5);
/*
@@ -700,24 +697,15 @@ static int eeh_reset_device(bool eeh_aware_driver,
* PE. We should disconnect it so the binding can be
* rebuilt when adding PCI devices.
*/
- edev = list_first_entry(&pe->edevs, struct eeh_dev, list);
- eeh_pe_traverse(pe, eeh_pe_detach_dev, NULL);
- if (pe->type & EEH_PE_VF) {
- eeh_add_virt_device(edev, NULL);
- } else {
- eeh_pe_state_clear(pe, EEH_PE_PRI_BUS);
- pci_hp_add_devices(bus);
- }
- } else if (rmv_data->removed) {
- pr_info("EEH: Sleep 5s ahead of partial hotplug\n");
- ssleep(5);
-
edev = list_first_entry(&pe->edevs, struct eeh_dev, list);
eeh_pe_traverse(pe, eeh_pe_detach_dev, NULL);
if (pe->type & EEH_PE_VF)
eeh_add_virt_device(edev, NULL);
- else
+ else {
+ if (!eeh_aware_driver)
+ eeh_pe_state_clear(pe, EEH_PE_PRI_BUS);
pci_hp_add_devices(bus);
+ }
}
eeh_pe_state_clear(pe, EEH_PE_KEEP);
--
2.16.1.74.g9b0b1f47b
^ permalink raw reply related
* [PATCH 7/9] powerpc/eeh: Remove always-true tests in eeh_reset_device()
From: Sam Bobroff @ 2018-03-05 23:59 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <cover.1520294174.git.sam.bobroff@au1.ibm.com>
eeh_reset_device() tests the value of 'bus' more than once but the
only caller, eeh_handle_normal_device() does this test itself and will
never pass NULL.
So, remove the dead tests.
This should not change behaviour.
Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
arch/powerpc/kernel/eeh_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 6c3577133223..1272f2c8cbd2 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -655,7 +655,7 @@ static int eeh_reset_device(bool eeh_aware_driver,
pci_hp_remove_devices(bus);
pci_unlock_rescan_remove();
}
- } else if (bus) {
+ } else {
eeh_pe_dev_traverse(pe, eeh_rmv_device, rmv_data);
}
@@ -708,7 +708,7 @@ static int eeh_reset_device(bool eeh_aware_driver,
eeh_pe_state_clear(pe, EEH_PE_PRI_BUS);
pci_hp_add_devices(bus);
}
- } else if (bus && rmv_data->removed) {
+ } else if (rmv_data->removed) {
pr_info("EEH: Sleep 5s ahead of partial hotplug\n");
ssleep(5);
--
2.16.1.74.g9b0b1f47b
^ permalink raw reply related
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