* Re: [PATCH v1 1/2] powerpc/bitops: Use immediate operand when possible
From: Segher Boessenkool @ 2021-04-14 17:20 UTC (permalink / raw)
To: David Laight
Cc: linuxppc-dev@lists.ozlabs.org, Paul Mackerras,
linux-kernel@vger.kernel.org, Nicholas Piggin
In-Reply-To: <efcabc9410cf4d03b203749a02e5a935@AcuMS.aculab.com>
On Wed, Apr 14, 2021 at 03:32:04PM +0000, David Laight wrote:
> From: Segher Boessenkool
> > Sent: 14 April 2021 16:19
> ...
> > > Could the kernel use GCC builtin atomic functions instead ?
> > >
> > > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> >
> > Certainly that should work fine for the simpler cases that the atomic
> > operations are meant to provide. But esp. for not-so-simple cases the
> > kernel may require some behaviour provided by the existing assembler
> > implementation, and not by the atomic builtins.
> >
> > I'm not saying this cannot work, just that some serious testing will be
> > needed. If it works it should be the best of all worlds, so then it is
> > a really good idea yes :-)
>
> I suspect they just add an extra layer of abstraction that makes it
> even more difficult to verify and could easily get broken by a compiler
> update (etc).
I would say it uses an existing facility, instead of creating a kernel-
specific one.
> The other issue is that the code needs to be correct with compiled
> with (for example) -O0.
> That could very easily break anything except the asm implementation
> if additional memory accesses and/or increased code size cause grief.
The compiler generates correct code. New versions of the compiler or
old, -O0 or not, under any phase of the moon.
Of course sometimes the compiler is broken, but there are pre-existing
ways of dealing with that, and there is no reason at all to think this
would break more often than random other code.
Segher
^ permalink raw reply
* Re: [PATCH v2 0/5] powerpc/rtas: miscellaneous cleanups
From: Christophe Leroy @ 2021-04-14 17:02 UTC (permalink / raw)
To: Nathan Lynch, linuxppc-dev
Cc: tyreld, ajd, aik, aneesh.kumar, npiggin, brking
In-Reply-To: <20210408140630.205502-1-nathanl@linux.ibm.com>
Le 08/04/2021 à 16:06, Nathan Lynch a écrit :
> This is a reroll of the series posted here:
> https://lore.kernel.org/linuxppc-dev/20210114220004.1138993-1-nathanl@linux.ibm.com/
>
> Originally this work was prompted by failures on radix MMU PowerVM
> guests when passing buffers to RTAS that lay outside of its idea of
> the RMA. In v1 I approached this as a problem to be solved in Linux,
> but RTAS development has since decided to change their code so that
> the RMA restriction does not apply with radix.
>
> So in v2 I retain the cleanups and discard the more significant change
> which accommodated the misbehaving RTAS versions.
Is there a link with https://github.com/linuxppc/issues/issues/252 ?
>
> Changes since v1:
> - Correct missing conversion of RTAS_RMOBUF_MAX ->
> RTAS_USER_REGION_SIZE in in_rmo_buf().
> - Remove unnecessary braces in rtas_syscall_filter_init().
> - Leave expression of RTAS_WORK_AREA_SIZE as-is instead of changing
> the factors in a confusing way, per discussion with Alexey.
> - Drop "powerpc/rtas: constrain user region allocation to RMA"
>
> Nathan Lynch (5):
> powerpc/rtas: improve ppc_rtas_rmo_buf_show documentation
> powerpc/rtas-proc: remove unused RMO_READ_BUF_MAX
> powerpc/rtas: remove ibm_suspend_me_token
> powerpc/rtas: move syscall filter setup into separate function
> powerpc/rtas: rename RTAS_RMOBUF_MAX to RTAS_USER_REGION_SIZE
>
> arch/powerpc/include/asm/rtas.h | 6 +++---
> arch/powerpc/kernel/rtas-proc.c | 15 +++++++++++----
> arch/powerpc/kernel/rtas.c | 34 +++++++++++++++++----------------
> 3 files changed, 32 insertions(+), 23 deletions(-)
>
^ permalink raw reply
* [PATCH v3 3/3] powerpc/atomics: Remove atomic_inc()/atomic_dec() and friends
From: Christophe Leroy @ 2021-04-14 16:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <7aaa03114d8bc4ba1805b51993fb92ec520899e0.1618419539.git.christophe.leroy@csgroup.eu>
Now that atomic_add() and atomic_sub() handle immediate operands,
atomic_inc() and atomic_dec() have no added value compared to the
generic fallback which calls atomic_add(1) and atomic_sub(1).
Also remove atomic_inc_not_zero() which fallsback to
atomic_add_unless() which itself fallsback to
atomic_fetch_add_unless() which now handles immediate operands.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: New
---
arch/powerpc/include/asm/atomic.h | 95 -------------------------------
1 file changed, 95 deletions(-)
diff --git a/arch/powerpc/include/asm/atomic.h b/arch/powerpc/include/asm/atomic.h
index eb1bdf14f67c..00ba5d9e837b 100644
--- a/arch/powerpc/include/asm/atomic.h
+++ b/arch/powerpc/include/asm/atomic.h
@@ -118,71 +118,6 @@ ATOMIC_OPS(xor, xor, "", K)
#undef ATOMIC_OP_RETURN_RELAXED
#undef ATOMIC_OP
-static __inline__ void atomic_inc(atomic_t *v)
-{
- int t;
-
- __asm__ __volatile__(
-"1: lwarx %0,0,%2 # atomic_inc\n\
- addic %0,%0,1\n"
-" stwcx. %0,0,%2 \n\
- bne- 1b"
- : "=&r" (t), "+m" (v->counter)
- : "r" (&v->counter)
- : "cc", "xer");
-}
-#define atomic_inc atomic_inc
-
-static __inline__ int atomic_inc_return_relaxed(atomic_t *v)
-{
- int t;
-
- __asm__ __volatile__(
-"1: lwarx %0,0,%2 # atomic_inc_return_relaxed\n"
-" addic %0,%0,1\n"
-" stwcx. %0,0,%2\n"
-" bne- 1b"
- : "=&r" (t), "+m" (v->counter)
- : "r" (&v->counter)
- : "cc", "xer");
-
- return t;
-}
-
-static __inline__ void atomic_dec(atomic_t *v)
-{
- int t;
-
- __asm__ __volatile__(
-"1: lwarx %0,0,%2 # atomic_dec\n\
- addic %0,%0,-1\n"
-" stwcx. %0,0,%2\n\
- bne- 1b"
- : "=&r" (t), "+m" (v->counter)
- : "r" (&v->counter)
- : "cc", "xer");
-}
-#define atomic_dec atomic_dec
-
-static __inline__ int atomic_dec_return_relaxed(atomic_t *v)
-{
- int t;
-
- __asm__ __volatile__(
-"1: lwarx %0,0,%2 # atomic_dec_return_relaxed\n"
-" addic %0,%0,-1\n"
-" stwcx. %0,0,%2\n"
-" bne- 1b"
- : "=&r" (t), "+m" (v->counter)
- : "r" (&v->counter)
- : "cc", "xer");
-
- return t;
-}
-
-#define atomic_inc_return_relaxed atomic_inc_return_relaxed
-#define atomic_dec_return_relaxed atomic_dec_return_relaxed
-
#define atomic_cmpxchg(v, o, n) (cmpxchg(&((v)->counter), (o), (n)))
#define atomic_cmpxchg_relaxed(v, o, n) \
cmpxchg_relaxed(&((v)->counter), (o), (n))
@@ -252,36 +187,6 @@ static __inline__ int atomic_fetch_add_unless(atomic_t *v, int a, int u)
}
#define atomic_fetch_add_unless atomic_fetch_add_unless
-/**
- * atomic_inc_not_zero - increment unless the number is zero
- * @v: pointer of type atomic_t
- *
- * Atomically increments @v by 1, so long as @v is non-zero.
- * Returns non-zero if @v was non-zero, and zero otherwise.
- */
-static __inline__ int atomic_inc_not_zero(atomic_t *v)
-{
- int t1, t2;
-
- __asm__ __volatile__ (
- PPC_ATOMIC_ENTRY_BARRIER
-"1: lwarx %0,0,%2 # atomic_inc_not_zero\n\
- cmpwi 0,%0,0\n\
- beq- 2f\n\
- addic %1,%0,1\n"
-" stwcx. %1,0,%2\n\
- bne- 1b\n"
- PPC_ATOMIC_EXIT_BARRIER
- "\n\
-2:"
- : "=&r" (t1), "=&r" (t2)
- : "r" (&v->counter)
- : "cc", "xer", "memory");
-
- return t1;
-}
-#define atomic_inc_not_zero(v) atomic_inc_not_zero((v))
-
/*
* Atomically test *v and decrement if it is greater than 0.
* The function returns the old value of *v minus 1, even if
--
2.25.0
^ permalink raw reply related
* [PATCH v3 2/3] powerpc/atomics: Use immediate operand when possible
From: Christophe Leroy @ 2021-04-14 16:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <7aaa03114d8bc4ba1805b51993fb92ec520899e0.1618419539.git.christophe.leroy@csgroup.eu>
Today we get the following code generation for atomic operations:
c001bb2c: 39 20 00 01 li r9,1
c001bb30: 7d 40 18 28 lwarx r10,0,r3
c001bb34: 7d 09 50 50 subf r8,r9,r10
c001bb38: 7d 00 19 2d stwcx. r8,0,r3
c001c7a8: 39 40 00 01 li r10,1
c001c7ac: 7d 00 18 28 lwarx r8,0,r3
c001c7b0: 7c ea 42 14 add r7,r10,r8
c001c7b4: 7c e0 19 2d stwcx. r7,0,r3
By allowing GCC to choose between immediate or regular operation,
we get:
c001bb2c: 7d 20 18 28 lwarx r9,0,r3
c001bb30: 39 49 ff ff addi r10,r9,-1
c001bb34: 7d 40 19 2d stwcx. r10,0,r3
--
c001c7a4: 7d 40 18 28 lwarx r10,0,r3
c001c7a8: 39 0a 00 01 addi r8,r10,1
c001c7ac: 7d 00 19 2d stwcx. r8,0,r3
For "and", the dot form has to be used because "andi" doesn't exist.
For logical operations we use unsigned 16 bits immediate.
For arithmetic operations we use signed 16 bits immediate.
On pmac32_defconfig, it reduces the text by approx another 8 kbytes.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Segher Boessenkool <segher@kernel.crashing.org>
---
v2: Use "addc/addic"
---
arch/powerpc/include/asm/atomic.h | 56 +++++++++++++++----------------
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/arch/powerpc/include/asm/atomic.h b/arch/powerpc/include/asm/atomic.h
index 61c6e8b200e8..eb1bdf14f67c 100644
--- a/arch/powerpc/include/asm/atomic.h
+++ b/arch/powerpc/include/asm/atomic.h
@@ -37,62 +37,62 @@ static __inline__ void atomic_set(atomic_t *v, int i)
__asm__ __volatile__("stw%U0%X0 %1,%0" : "=m"UPD_CONSTR(v->counter) : "r"(i));
}
-#define ATOMIC_OP(op, asm_op) \
+#define ATOMIC_OP(op, asm_op, suffix, sign, ...) \
static __inline__ void atomic_##op(int a, atomic_t *v) \
{ \
int t; \
\
__asm__ __volatile__( \
"1: lwarx %0,0,%3 # atomic_" #op "\n" \
- #asm_op " %0,%2,%0\n" \
+ #asm_op "%I2" suffix " %0,%0,%2\n" \
" stwcx. %0,0,%3 \n" \
" bne- 1b\n" \
: "=&r" (t), "+m" (v->counter) \
- : "r" (a), "r" (&v->counter) \
- : "cc"); \
+ : "r"#sign (a), "r" (&v->counter) \
+ : "cc", ##__VA_ARGS__); \
} \
-#define ATOMIC_OP_RETURN_RELAXED(op, asm_op) \
+#define ATOMIC_OP_RETURN_RELAXED(op, asm_op, suffix, sign, ...) \
static inline int atomic_##op##_return_relaxed(int a, atomic_t *v) \
{ \
int t; \
\
__asm__ __volatile__( \
"1: lwarx %0,0,%3 # atomic_" #op "_return_relaxed\n" \
- #asm_op " %0,%2,%0\n" \
+ #asm_op "%I2" suffix " %0,%0,%2\n" \
" stwcx. %0,0,%3\n" \
" bne- 1b\n" \
: "=&r" (t), "+m" (v->counter) \
- : "r" (a), "r" (&v->counter) \
- : "cc"); \
+ : "r"#sign (a), "r" (&v->counter) \
+ : "cc", ##__VA_ARGS__); \
\
return t; \
}
-#define ATOMIC_FETCH_OP_RELAXED(op, asm_op) \
+#define ATOMIC_FETCH_OP_RELAXED(op, asm_op, suffix, sign, ...) \
static inline int atomic_fetch_##op##_relaxed(int a, atomic_t *v) \
{ \
int res, t; \
\
__asm__ __volatile__( \
"1: lwarx %0,0,%4 # atomic_fetch_" #op "_relaxed\n" \
- #asm_op " %1,%3,%0\n" \
+ #asm_op "%I3" suffix " %1,%0,%3\n" \
" stwcx. %1,0,%4\n" \
" bne- 1b\n" \
: "=&r" (res), "=&r" (t), "+m" (v->counter) \
- : "r" (a), "r" (&v->counter) \
- : "cc"); \
+ : "r"#sign (a), "r" (&v->counter) \
+ : "cc", ##__VA_ARGS__); \
\
return res; \
}
-#define ATOMIC_OPS(op, asm_op) \
- ATOMIC_OP(op, asm_op) \
- ATOMIC_OP_RETURN_RELAXED(op, asm_op) \
- ATOMIC_FETCH_OP_RELAXED(op, asm_op)
+#define ATOMIC_OPS(op, asm_op, suffix, sign, ...) \
+ ATOMIC_OP(op, asm_op, suffix, sign, ##__VA_ARGS__) \
+ ATOMIC_OP_RETURN_RELAXED(op, asm_op, suffix, sign, ##__VA_ARGS__)\
+ ATOMIC_FETCH_OP_RELAXED(op, asm_op, suffix, sign, ##__VA_ARGS__)
-ATOMIC_OPS(add, add)
-ATOMIC_OPS(sub, subf)
+ATOMIC_OPS(add, add, "c", I, "xer")
+ATOMIC_OPS(sub, sub, "c", I, "xer")
#define atomic_add_return_relaxed atomic_add_return_relaxed
#define atomic_sub_return_relaxed atomic_sub_return_relaxed
@@ -101,13 +101,13 @@ ATOMIC_OPS(sub, subf)
#define atomic_fetch_sub_relaxed atomic_fetch_sub_relaxed
#undef ATOMIC_OPS
-#define ATOMIC_OPS(op, asm_op) \
- ATOMIC_OP(op, asm_op) \
- ATOMIC_FETCH_OP_RELAXED(op, asm_op)
+#define ATOMIC_OPS(op, asm_op, suffix, sign) \
+ ATOMIC_OP(op, asm_op, suffix, sign) \
+ ATOMIC_FETCH_OP_RELAXED(op, asm_op, suffix, sign)
-ATOMIC_OPS(and, and)
-ATOMIC_OPS(or, or)
-ATOMIC_OPS(xor, xor)
+ATOMIC_OPS(and, and, ".", K)
+ATOMIC_OPS(or, or, "", K)
+ATOMIC_OPS(xor, xor, "", K)
#define atomic_fetch_and_relaxed atomic_fetch_and_relaxed
#define atomic_fetch_or_relaxed atomic_fetch_or_relaxed
@@ -238,15 +238,15 @@ static __inline__ int atomic_fetch_add_unless(atomic_t *v, int a, int u)
"1: lwarx %0,0,%1 # atomic_fetch_add_unless\n\
cmpw 0,%0,%3 \n\
beq 2f \n\
- add %0,%2,%0 \n"
+ add%I2c %0,%0,%2 \n"
" stwcx. %0,0,%1 \n\
bne- 1b \n"
PPC_ATOMIC_EXIT_BARRIER
-" subf %0,%2,%0 \n\
+" sub%I2c %0,%0,%2 \n\
2:"
: "=&r" (t)
- : "r" (&v->counter), "r" (a), "r" (u)
- : "cc", "memory");
+ : "r" (&v->counter), "rI" (a), "r" (u)
+ : "cc", "memory", "xer");
return t;
}
--
2.25.0
^ permalink raw reply related
* [PATCH v3 1/3] powerpc/bitops: Use immediate operand when possible
From: Christophe Leroy @ 2021-04-14 16:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Today we get the following code generation for bitops like
set or clear bit:
c0009fe0: 39 40 08 00 li r10,2048
c0009fe4: 7c e0 40 28 lwarx r7,0,r8
c0009fe8: 7c e7 53 78 or r7,r7,r10
c0009fec: 7c e0 41 2d stwcx. r7,0,r8
c000d568: 39 00 18 00 li r8,6144
c000d56c: 7c c0 38 28 lwarx r6,0,r7
c000d570: 7c c6 40 78 andc r6,r6,r8
c000d574: 7c c0 39 2d stwcx. r6,0,r7
Most set bits are constant on lower 16 bits, so it can easily
be replaced by the "immediate" version of the operation. Allow
GCC to choose between the normal or immediate form.
For clear bits, on 32 bits 'rlwinm' can be used instead of 'andc' for
when all bits to be cleared are consecutive.
On 64 bits we don't have any equivalent single operation for clearing,
single bits or a few bits, we'd need two 'rldicl' so it is not
worth it, the li/andc sequence is doing the same.
With this patch we get:
c0009fe0: 7d 00 50 28 lwarx r8,0,r10
c0009fe4: 61 08 08 00 ori r8,r8,2048
c0009fe8: 7d 00 51 2d stwcx. r8,0,r10
c000d558: 7c e0 40 28 lwarx r7,0,r8
c000d55c: 54 e7 05 64 rlwinm r7,r7,0,21,18
c000d560: 7c e0 41 2d stwcx. r7,0,r8
On pmac32_defconfig, it reduces the text by approx 10 kbytes.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3:
- Using the mask validation proposed by Segher
v2:
- Use "n" instead of "i" as constraint for the rlwinm mask
- Improve mask verification to handle more than single bit masks
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/bitops.h | 89 ++++++++++++++++++++++++++++---
1 file changed, 81 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
index 299ab33505a6..09500c789972 100644
--- a/arch/powerpc/include/asm/bitops.h
+++ b/arch/powerpc/include/asm/bitops.h
@@ -71,19 +71,61 @@ static inline void fn(unsigned long mask, \
__asm__ __volatile__ ( \
prefix \
"1:" PPC_LLARX(%0,0,%3,0) "\n" \
- stringify_in_c(op) "%0,%0,%2\n" \
+ #op "%I2 %0,%0,%2\n" \
PPC_STLCX "%0,0,%3\n" \
"bne- 1b\n" \
: "=&r" (old), "+m" (*p) \
- : "r" (mask), "r" (p) \
+ : "rK" (mask), "r" (p) \
: "cc", "memory"); \
}
DEFINE_BITOP(set_bits, or, "")
-DEFINE_BITOP(clear_bits, andc, "")
-DEFINE_BITOP(clear_bits_unlock, andc, PPC_RELEASE_BARRIER)
DEFINE_BITOP(change_bits, xor, "")
+static __always_inline bool is_rlwinm_mask_valid(unsigned long x)
+{
+ if (!x)
+ return false;
+ if (x & 1)
+ x = ~x; // make the mask non-wrapping
+ x += x & -x; // adding the low set bit results in at most one bit set
+
+ return !(x & (x - 1));
+}
+
+#define DEFINE_CLROP(fn, prefix) \
+static inline void fn(unsigned long mask, volatile unsigned long *_p) \
+{ \
+ unsigned long old; \
+ unsigned long *p = (unsigned long *)_p; \
+ \
+ if (IS_ENABLED(CONFIG_PPC32) && \
+ __builtin_constant_p(mask) && is_rlwinm_mask_valid(~mask)) {\
+ asm volatile ( \
+ prefix \
+ "1:" "lwarx %0,0,%3\n" \
+ "rlwinm %0,%0,0,%2\n" \
+ "stwcx. %0,0,%3\n" \
+ "bne- 1b\n" \
+ : "=&r" (old), "+m" (*p) \
+ : "n" (~mask), "r" (p) \
+ : "cc", "memory"); \
+ } else { \
+ asm volatile ( \
+ prefix \
+ "1:" PPC_LLARX(%0,0,%3,0) "\n" \
+ "andc %0,%0,%2\n" \
+ PPC_STLCX "%0,0,%3\n" \
+ "bne- 1b\n" \
+ : "=&r" (old), "+m" (*p) \
+ : "r" (mask), "r" (p) \
+ : "cc", "memory"); \
+ } \
+}
+
+DEFINE_CLROP(clear_bits, "")
+DEFINE_CLROP(clear_bits_unlock, PPC_RELEASE_BARRIER)
+
static inline void arch_set_bit(int nr, volatile unsigned long *addr)
{
set_bits(BIT_MASK(nr), addr + BIT_WORD(nr));
@@ -116,12 +158,12 @@ static inline unsigned long fn( \
__asm__ __volatile__ ( \
prefix \
"1:" PPC_LLARX(%0,0,%3,eh) "\n" \
- stringify_in_c(op) "%1,%0,%2\n" \
+ #op "%I2 %1,%0,%2\n" \
PPC_STLCX "%1,0,%3\n" \
"bne- 1b\n" \
postfix \
: "=&r" (old), "=&r" (t) \
- : "r" (mask), "r" (p) \
+ : "rK" (mask), "r" (p) \
: "cc", "memory"); \
return (old & mask); \
}
@@ -130,11 +172,42 @@ DEFINE_TESTOP(test_and_set_bits, or, PPC_ATOMIC_ENTRY_BARRIER,
PPC_ATOMIC_EXIT_BARRIER, 0)
DEFINE_TESTOP(test_and_set_bits_lock, or, "",
PPC_ACQUIRE_BARRIER, 1)
-DEFINE_TESTOP(test_and_clear_bits, andc, PPC_ATOMIC_ENTRY_BARRIER,
- PPC_ATOMIC_EXIT_BARRIER, 0)
DEFINE_TESTOP(test_and_change_bits, xor, PPC_ATOMIC_ENTRY_BARRIER,
PPC_ATOMIC_EXIT_BARRIER, 0)
+static inline unsigned long test_and_clear_bits(unsigned long mask, volatile unsigned long *_p)
+{
+ unsigned long old, t;
+ unsigned long *p = (unsigned long *)_p;
+
+ if (IS_ENABLED(CONFIG_PPC32) &&
+ __builtin_constant_p(mask) && is_rlwinm_mask_valid(mask)) {
+ asm volatile (
+ PPC_ATOMIC_ENTRY_BARRIER
+ "1:" "lwarx %0,0,%3\n"
+ "rlwinm %1,%0,0,%2\n"
+ "stwcx. %1,0,%3\n"
+ "bne- 1b\n"
+ PPC_ATOMIC_EXIT_BARRIER
+ : "=&r" (old), "=&r" (t)
+ : "n" (~mask), "r" (p)
+ : "cc", "memory");
+ } else {
+ asm volatile (
+ PPC_ATOMIC_ENTRY_BARRIER
+ "1:" PPC_LLARX(%0,0,%3,0) "\n"
+ "andc %1,%0,%2\n"
+ PPC_STLCX "%1,0,%3\n"
+ "bne- 1b\n"
+ PPC_ATOMIC_EXIT_BARRIER
+ : "=&r" (old), "=&r" (t)
+ : "r" (mask), "r" (p)
+ : "cc", "memory");
+ }
+
+ return (old & mask);
+}
+
static inline int arch_test_and_set_bit(unsigned long nr,
volatile unsigned long *addr)
{
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
From: Vaibhav Jain @ 2021-04-14 16:21 UTC (permalink / raw)
To: Ira Weiny
Cc: Santosh Sivaraj, linux-nvdimm, Aneesh Kumar K . V, Dan Williams,
linuxppc-dev
In-Reply-To: <20210414153625.GB1904484@iweiny-DESK2.sc.intel.com>
Thanks for looking into this patch Ira,
Ira Weiny <ira.weiny@intel.com> writes:
> On Wed, Apr 14, 2021 at 06:10:26PM +0530, Vaibhav Jain wrote:
>> Currently drc_pmem_qeury_stats() generates a dev_err in case
>> "Enable Performance Information Collection" feature is disabled from
>> HMC. The error is of the form below:
>>
>> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
>> performance stats, Err:-10
>>
>> This error message confuses users as it implies a possible problem
>> with the nvdimm even though its due to a disabled feature.
>>
>> So we fix this by explicitly handling the H_AUTHORITY error from the
>> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
>> error, saying that "Performance stats in-accessible".
>>
>> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 835163f54244..9216424f8be3 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
>> dev_err(&p->pdev->dev,
>> "Unknown performance stats, Err:0x%016lX\n", ret[0]);
>> return -ENOENT;
>> + } else if (rc == H_AUTHORITY) {
>> + dev_warn(&p->pdev->dev, "Performance stats in-accessible");
>> + return -EPERM;
>
> Is this because of a disabled feature or because of permissions?
Its because of a disabled feature that revokes permission for a guest to
retrieve performance statistics.
The feature is called "Enable Performance Information Collection" and
once disabled the hcall H_SCM_PERFORMANCE_STATS returns an error
H_AUTHORITY indicating that the guest doesn't have permission to retrieve
performance statistics.
>
> Ira
>
>> } else if (rc != H_SUCCESS) {
>> dev_err(&p->pdev->dev,
>> "Failed to query performance stats, Err:%lld\n", rc);
>> --
>> 2.30.2
>>
--
Cheers
~ Vaibhav
^ permalink raw reply
* RE: [PATCH 1/1] mm: Fix struct page layout on 32-bit systems
From: David Laight @ 2021-04-14 15:52 UTC (permalink / raw)
To: 'Matthew Wilcox', Jesper Dangaard Brouer
Cc: Arnd Bergmann, Grygorii Strashko, netdev@vger.kernel.org,
Ilias Apalodimas, linux-mips@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mm@kvack.org, Matteo Croce,
linuxppc-dev@lists.ozlabs.org, Christoph Hellwig,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20210414115052.GS2531743@casper.infradead.org>
> Doing this fixes it:
>
> +++ b/include/linux/types.h
> @@ -140,7 +140,7 @@ typedef u64 blkcnt_t;
> * so they don't care about the size of the actual bus addresses.
> */
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -typedef u64 dma_addr_t;
> +typedef u64 __attribute__((aligned(sizeof(void *)))) dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
I hate __packed so much I've been checking what it does!
If you add __packed to the dma_addr_t field inside the union
then gcc (at least) removes the pad from before it, but also
'remembers' the alignment that is enforced by other members
of the structure.
So you don't need the extra aligned(sizeof (void *)) since
that is implicit.
So in this case __packed probably has no side effects.
(Unless a 32bit arch has instructions for a 64bit read
that must not be on an 8n+4 boundary and the address is taken).
It also doesn't affect 64bit - since the previous field
forces 64bit alignment.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
From: Lucas Stach @ 2021-04-14 15:33 UTC (permalink / raw)
To: Robin Gong, Shengjiu Wang
Cc: Nicolin Chen, Linux-ALSA, Liam Girdwood, s.hauer@pengutronix.de,
Timur Tabi, Xiubo Li, shawnguo@kernel.org, S.j. Wang,
linux-kernel, dri-devel@lists.freedesktop.org, Takashi Iwai,
linaro-mm-sig@lists.linaro.org, Mark Brown, dl-linux-imx,
kernel@pengutronix.de, Fabio Estevam, perex@perex.cz,
linuxppc-dev@lists.ozlabs.org, sumit.semwal@linaro.org,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <VE1PR04MB66387217EDE5133FD2D8F793894E9@VE1PR04MB6638.eurprd04.prod.outlook.com>
Hi Robin,
Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong:
> On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang:
> > > Hi
> > >
> > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach <l.stach@pengutronix.de>
> > wrote:
> > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang:
> > > > > There are two requirements that we need to move the request of dma
> > > > > channel from probe to open.
> > > >
> > > > How do you handle -EPROBE_DEFER return code from the channel request
> > > > if you don't do it in probe?
> > >
> > > I use the dma_request_slave_channel or dma_request_channel instead of
> > > dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER
> > > return code.
> >
> > This is a pretty weak argument. The dmaengine device might probe after you
> > try to get the channel. Using a function to request the channel that doesn't
> > allow you to handle probe deferral is IMHO a bug and should be fixed, instead
> > of building even more assumptions on top of it.
> >
> > > > > - When dma device binds with power-domains, the power will be
> > > > > enabled when we request dma channel. If the request of dma channel
> > > > > happen on probe, then the power-domains will be always enabled
> > > > > after kernel boot up, which is not good for power saving, so we
> > > > > need to move the request of dma channel to .open();
> > > >
> > > > This is certainly something which could be fixed in the dmaengine
> > > > driver.
> > >
> > > Dma driver always call the pm_runtime_get_sync in
> > > device_alloc_chan_resources, the device_alloc_chan_resources is called
> > > when channel is requested. so power is enabled on channel request.
> >
> > So why can't you fix the dmaengine driver to do that RPM call at a later time
> > when the channel is actually going to be used? This will allow further power
> > savings with other slave devices than the audio PCM.
> Hi Lucas,
> Thanks for your suggestion. I have tried to implement runtime autosuspend in
> fsl-edma driver on i.mx8qm/qxp with delay time (2 sec) for this feature as below
> (or you can refer to drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/
> pm_runtime_put_autosuspend in all dmaengine driver interface like
> device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_cyclic/
> device_tx_status...
>
>
> pm_runtime_use_autosuspend(fsl_chan->dev);
> pm_runtime_set_autosuspend_delay(fsl_chan->dev, 2000);
>
> That could resolve this audio case since the autosuspend could suspend runtime after
> 2 seconds if there is no further dma transfer but only channel request(device_alloc_chan_resources).
> But unfortunately, it cause another issue. As you know, on our i.mx8qm/qxp,
> power domain done by scfw (drivers/firmware/imx/scu-pd.c) over mailbox:
> imx_sc_pd_power()->imx_scu_call_rpc()-> imx_scu_ipc_write()->mbox_send_message()
> which means have to 'waits for completion', meanwhile, some driver like tty will call dmaengine
> interfaces in non-atomic case as below,
>
> static int uart_write(struct tty_struct *tty, const unsigned char *buf, int count)
> {
> .......
> port = uart_port_lock(state, flags);
> ......
> __uart_start(tty); //call start_tx()->dmaengine_prep_slave_sg...
> uart_port_unlock(port, flags);
> return ret;
> }
>
> Thus dma runtime resume may happen in that timing window and cause kernel alarm.
> I'm not sure whether there are similar limitations on other driver subsystem. But for me,
> It looks like the only way to resolve the contradiction between tty and scu-pd (hardware
> limitation on i.mx8qm/qxp) is to give up autosuspend and keep pm_runtime_get_sync
> only in device_alloc_chan_resources because request channel is a safe non-atomic phase.
> Do you have any idea? Thanks in advance.
If you look closely at the driver you used as an example (hidma.c) it
looks like there is already something in there, which looks very much
like what you need here:
In hidma_issue_pending() the driver tries to get the device to runtime
resume. If this doesn't work, maybe due to the power domain code not
being able to be called in atomic context, the actual work of waking up
the dma hardware and issuing the descriptor is shunted to a tasklet.
If I'm reading this right, this is exactly what you need here to be
able to call the dmaengine code from atomic context: try the rpm get
and issue immediately when possible, otherwise shunt the work to a non-
atomic context where you can deal with the requirements of scu-pd.
Also you don't need the runtime resume in all of the functions you
mentioned. From a quick look into the edma driver it looks like for
example the prep_slave_dma() function only touches data structures in
memory, so you don't actually need the device to be awake at that
point. Only later in the flow when you write registers in the dma
hardware and actually issue the transfer you need to wake the device
from sleep.
Regards,
Lucas
>
>
>
>
>
> >
> > > > > - With FE-BE case, if the dma channel is requested in probe, then
> > > > > there will be below issue, which is caused by that the dma channel
> > > > > will be requested duplicately
> > > >
> > > > Why is this requested a second time? Is this just some missing
> > > > cleanup on a deferred probe path?
> > >
> > > Not relate with deferred probe. With DMA1->ASRC->DMA2->ESAI case, the
> > > DMA1->ASRC->DMA2 is in FE, ESAI is in BE. When ESAI drvier probe,
> > > DMA3 channel is created with ESAI's "dma:tx" (DMA3 channel
> > > is not used in this FE-BE case). When FE-BE startup, DMA2
> > > channel is created, it needs the ESAI's "dma:tx", so below warning
> > > comes out.
> > >
> > > > Regards,
> > > > Lucas
> > > >
> > > > > [ 638.906268] sysfs: cannot create duplicate filename
> > '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx'
> > > > > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted
> > > > > 5.7.0-rc1-12956-gfc64b2585593 #287 [ 638.927113] Hardware name:
> > > > > Freescale i.MX6 Quad/DualLite (Device Tree) [ 638.933690]
> > > > > [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>]
> > > > > (show_stack+0x10/0x14) [ 638.941464] [<c010b8ec>] (show_stack)
> > > > > from [<c0557fc0>] (dump_stack+0xe4/0x118) [ 638.948808]
> > > > > [<c0557fc0>] (dump_stack) from [<c032aeb4>]
> > > > > (sysfs_warn_dup+0x50/0x64) [ 638.956406] [<c032aeb4>]
> > > > > (sysfs_warn_dup) from [<c032b1a8>]
> > > > > (sysfs_do_create_link_sd+0xc8/0xd4)
> > > > > [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from
> > > > > [<c05dc668>] (dma_request_chan+0xb0/0x210) [ 638.974120]
> > > > > [<c05dc668>] (dma_request_chan) from [<c05dc7d0>]
> > > > > (dma_request_slave_channel+0x8/0x14)
> > > > > [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from
> > > > > [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438)
> > > > > [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from
> > > > > [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) [ 639.001952]
> > > > > [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>]
> > > > > (dpcm_fe_dai_hw_params+0x70/0xe4) [ 639.010765] [<c09c39d4>]
> > > > > (dpcm_fe_dai_hw_params) from [<c099b274>]
> > > > > (snd_pcm_hw_params+0x158/0x418) [ 639.019750] [<c099b274>]
> > > > > (snd_pcm_hw_params) from [<c099c5a0>]
> > (snd_pcm_ioctl+0x734/0x183c) [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl)
> > from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) [ 639.035812] [<c029ff94>]
> > (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) [ 639.043490]
> > Exception stack(0xec529fa8 to 0xec529ff0)
> > > > > [ 639.048565] 9fa0: bee84650 01321870
> > 00000004 c25c4111 bee84650 0002000f
> > > > > [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036
> > 00001f40
> > > > > 00000000 0002c2f8 00000003 [ 639.064964] 9fe0: b6f483fc bee8451c
> > > > > b6ee2655 b6e1dcf8 [ 639.070339] fsl-esai-dai 2024000.esai: Cannot
> > > > > create DMA dma:tx symlink
> > > > >
> > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > > ---
> > > > > sound/soc/fsl/imx-pcm-dma.c | 173
> > > > > +++++++++++++++++++++++++++++++++---
> > > > > 1 file changed, 159 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/sound/soc/fsl/imx-pcm-dma.c
> > > > > b/sound/soc/fsl/imx-pcm-dma.c index 04a9bc749016..dae53b384df4
> > > > > 100644
> > > > > --- a/sound/soc/fsl/imx-pcm-dma.c
> > > > > +++ b/sound/soc/fsl/imx-pcm-dma.c
> > > > > @@ -11,6 +11,7 @@
> > > > > #include <linux/dmaengine.h>
> > > > > #include <linux/types.h>
> > > > > #include <linux/module.h>
> > > > > +#include <linux/dma-mapping.h>
> > > > >
> > > > > #include <sound/core.h>
> > > > > #include <sound/pcm.h>
> > > > > @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void
> > *param)
> > > > > return true;
> > > > > }
> > > > >
> > > > > -static const struct snd_dmaengine_pcm_config
> > imx_dmaengine_pcm_config = {
> > > > > - .prepare_slave_config =
> > snd_dmaengine_pcm_prepare_slave_config,
> > > > > - .compat_filter_fn = filter,
> > > > > -};
> > > > > +static int imx_pcm_hw_params(struct snd_soc_component
> > *component,
> > > > > + struct snd_pcm_substream *substream,
> > > > > + struct snd_pcm_hw_params *params) {
> > > > > + struct snd_pcm_runtime *runtime = substream->runtime;
> > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > > > > + struct snd_dmaengine_dai_dma_data *dma_data;
> > > > > + struct dma_slave_config config;
> > > > > + struct dma_chan *chan;
> > > > > + int ret = 0;
> > > > >
> > > > > -int imx_pcm_dma_init(struct platform_device *pdev, size_t size)
> > > > > + snd_pcm_set_runtime_buffer(substream,
> > &substream->dma_buffer);
> > > > > + runtime->dma_bytes = params_buffer_bytes(params);
> > > > > +
> > > > > + chan = snd_dmaengine_pcm_get_chan(substream);
> > > > > + if (!chan)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + ret = snd_hwparams_to_dma_slave_config(substream, params,
> > &config);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream);
> > > > > + if (!dma_data)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + snd_dmaengine_pcm_set_config_from_dai_data(substream,
> > > > > + dma_data,
> > > > > + &config);
> > > > > + return dmaengine_slave_config(chan, &config); }
> > > > > +
> > > > > +static int imx_pcm_hw_free(struct snd_soc_component *component,
> > > > > + struct snd_pcm_substream *substream)
> > > > > {
> > > > > - struct snd_dmaengine_pcm_config *config;
> > > > > + snd_pcm_set_runtime_buffer(substream, NULL);
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component
> > *component,
> > > > > + struct snd_pcm_substream
> > > > > +*substream) {
> > > > > + return snd_dmaengine_pcm_pointer(substream);
> > > > > +}
> > > > > +
> > > > > +static int imx_pcm_trigger(struct snd_soc_component *component,
> > > > > + struct snd_pcm_substream *substream, int
> > > > > +cmd) {
> > > > > + return snd_dmaengine_pcm_trigger(substream, cmd); }
> > > > > +
> > > > > +static int imx_pcm_open(struct snd_soc_component *component,
> > > > > + struct snd_pcm_substream *substream) {
> > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > > > > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> > > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > > > > + struct snd_dmaengine_dai_dma_data *dma_data;
> > > > > + struct device *dev = component->dev;
> > > > > + struct snd_pcm_hardware hw;
> > > > > + struct dma_chan *chan;
> > > > > + int ret;
> > > > > +
> > > > > + ret = snd_pcm_hw_constraint_integer(substream->runtime,
> > > > > +
> > SNDRV_PCM_HW_PARAM_PERIODS);
> > > > > + if (ret < 0) {
> > > > > + dev_err(dev, "failed to set pcm hw params periods\n");
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream);
> > > > > + if (!dma_data)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx");
> > > > > + if (!chan) {
> > > > > + /* Try to request channel using compat_filter_fn */
> > > > > + chan = snd_dmaengine_pcm_request_channel(filter,
> > > > > +
> > dma_data->filter_data);
> > > > > + if (!chan)
> > > > > + return -ENXIO;
> > > > > + }
> > > > >
> > > > > - config = devm_kzalloc(&pdev->dev,
> > > > > - sizeof(struct snd_dmaengine_pcm_config),
> > GFP_KERNEL);
> > > > > - if (!config)
> > > > > - return -ENOMEM;
> > > > > - *config = imx_dmaengine_pcm_config;
> > > > > + ret = snd_dmaengine_pcm_open(substream, chan);
> > > > > + if (ret)
> > > > > + goto pcm_open_fail;
> > > > >
> > > > > - return devm_snd_dmaengine_pcm_register(&pdev->dev,
> > > > > - config,
> > > > > - SND_DMAENGINE_PCM_FLAG_COMPAT);
> > > > > + memset(&hw, 0, sizeof(hw));
> > > > > + hw.info = SNDRV_PCM_INFO_MMAP |
> > SNDRV_PCM_INFO_MMAP_VALID |
> > > > > + SNDRV_PCM_INFO_INTERLEAVED;
> > > > > + hw.periods_min = 2;
> > > > > + hw.periods_max = UINT_MAX;
> > > > > + hw.period_bytes_min = 256;
> > > > > + hw.period_bytes_max =
> > dma_get_max_seg_size(chan->device->dev);
> > > > > + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE;
> > > > > + hw.fifo_size = dma_data->fifo_size;
> > > > > +
> > > > > + /* Refine the hw according to caps of DMA. */
> > > > > + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream,
> > > > > +
> > dma_data,
> > > > > + &hw,
> > > > > + chan);
> > > > > + if (ret < 0)
> > > > > + goto refine_runtime_hwparams_fail;
> > > > > +
> > > > > + snd_soc_set_runtime_hwparams(substream, &hw);
> > > > > +
> > > > > + /* Support allocate memory from IRAM */
> > > > > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM,
> > > > > + chan->device->dev,
> > > > > + hw.buffer_bytes_max,
> > > > > + &substream->dma_buffer);
> > > > > + if (ret < 0)
> > > > > + goto alloc_pagas_fail;
> > > > > +
> > > > > + return 0;
> > > > > +
> > > > > +alloc_pagas_fail:
> > > > > +refine_runtime_hwparams_fail:
> > > > > + snd_dmaengine_pcm_close(substream);
> > > > > +pcm_open_fail:
> > > > > + dma_release_channel(chan);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int imx_pcm_close(struct snd_soc_component *component,
> > > > > + struct snd_pcm_substream *substream) {
> > > > > + if (substream) {
> > > > > + snd_dma_free_pages(&substream->dma_buffer);
> > > > > + substream->dma_buffer.area = NULL;
> > > > > + substream->dma_buffer.addr = 0;
> > > > > + }
> > > > > +
> > > > > + return snd_dmaengine_pcm_close_release_chan(substream);
> > > > > +}
> > > > > +
> > > > > +static int imx_pcm_new(struct snd_soc_component *component,
> > > > > + struct snd_soc_pcm_runtime *rtd) {
> > > > > + struct snd_card *card = rtd->card->snd_card;
> > > > > +
> > > > > + return dma_coerce_mask_and_coherent(card->dev,
> > > > > +DMA_BIT_MASK(32)); }
> > > > > +
> > > > > +static const struct snd_soc_component_driver imx_pcm_component = {
> > > > > + .name = "imx-pcm-dma",
> > > > > + .pcm_construct = imx_pcm_new,
> > > > > + .open = imx_pcm_open,
> > > > > + .close = imx_pcm_close,
> > > > > + .hw_params = imx_pcm_hw_params,
> > > > > + .hw_free = imx_pcm_hw_free,
> > > > > + .trigger = imx_pcm_trigger,
> > > > > + .pointer = imx_pcm_pointer,
> > > > > +};
> > > > > +
> > > > > +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) {
> > > > > + return devm_snd_soc_register_component(&pdev->dev,
> > > > > +
> > &imx_pcm_component,
> > > > > +NULL, 0);
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(imx_pcm_dma_init);
> > > > >
>
^ permalink raw reply
* Re: [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
From: Ira Weiny @ 2021-04-14 15:36 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Santosh Sivaraj, linux-nvdimm, Aneesh Kumar K . V, Dan Williams,
linuxppc-dev
In-Reply-To: <20210414124026.332472-1-vaibhav@linux.ibm.com>
On Wed, Apr 14, 2021 at 06:10:26PM +0530, Vaibhav Jain wrote:
> Currently drc_pmem_qeury_stats() generates a dev_err in case
> "Enable Performance Information Collection" feature is disabled from
> HMC. The error is of the form below:
>
> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
> performance stats, Err:-10
>
> This error message confuses users as it implies a possible problem
> with the nvdimm even though its due to a disabled feature.
>
> So we fix this by explicitly handling the H_AUTHORITY error from the
> H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
> error, saying that "Performance stats in-accessible".
>
> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 835163f54244..9216424f8be3 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
> dev_err(&p->pdev->dev,
> "Unknown performance stats, Err:0x%016lX\n", ret[0]);
> return -ENOENT;
> + } else if (rc == H_AUTHORITY) {
> + dev_warn(&p->pdev->dev, "Performance stats in-accessible");
> + return -EPERM;
Is this because of a disabled feature or because of permissions?
Ira
> } else if (rc != H_SUCCESS) {
> dev_err(&p->pdev->dev,
> "Failed to query performance stats, Err:%lld\n", rc);
> --
> 2.30.2
>
^ permalink raw reply
* RE: [PATCH v1 1/2] powerpc/bitops: Use immediate operand when possible
From: David Laight @ 2021-04-14 15:32 UTC (permalink / raw)
To: 'Segher Boessenkool', Christophe Leroy
Cc: linuxppc-dev@lists.ozlabs.org, Paul Mackerras,
linux-kernel@vger.kernel.org, Nicholas Piggin
In-Reply-To: <20210414151921.GW26583@gate.crashing.org>
From: Segher Boessenkool
> Sent: 14 April 2021 16:19
...
> > Could the kernel use GCC builtin atomic functions instead ?
> >
> > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
>
> Certainly that should work fine for the simpler cases that the atomic
> operations are meant to provide. But esp. for not-so-simple cases the
> kernel may require some behaviour provided by the existing assembler
> implementation, and not by the atomic builtins.
>
> I'm not saying this cannot work, just that some serious testing will be
> needed. If it works it should be the best of all worlds, so then it is
> a really good idea yes :-)
I suspect they just add an extra layer of abstraction that makes it
even more difficult to verify and could easily get broken by a compiler
update (etc).
The other issue is that the code needs to be correct with compiled
with (for example) -O0.
That could very easily break anything except the asm implementation
if additional memory accesses and/or increased code size cause grief.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v1 1/2] powerpc/bitops: Use immediate operand when possible
From: Segher Boessenkool @ 2021-04-14 15:19 UTC (permalink / raw)
To: Christophe Leroy
Cc: Paul Mackerras, linuxppc-dev, linux-kernel, Nicholas Piggin
In-Reply-To: <daacce9f-1900-1034-980b-be5a58d6be09@csgroup.eu>
On Wed, Apr 14, 2021 at 02:42:51PM +0200, Christophe Leroy wrote:
> Le 14/04/2021 à 14:24, Segher Boessenkool a écrit :
> >On Wed, Apr 14, 2021 at 12:01:21PM +1000, Nicholas Piggin wrote:
> >>Would be nice if we could let the compiler deal with it all...
> >>
> >>static inline unsigned long lr(unsigned long *mem)
> >>{
> >> unsigned long val;
> >>
> >> /*
> >> * This doesn't clobber memory but want to avoid memory
> >> operations
> >> * moving ahead of it
> >> */
> >> asm volatile("ldarx %0, %y1" : "=r"(val) : "Z"(*mem) :
> >> "memory");
> >>
> >> return val;
> >>}
> >
> >(etc.)
> >
> >That can not work reliably: the compiler can put random instructions
> >between the larx and stcx. this way, and you then do not have guaranteed
> >forward progress anymore. It can put the two in different routines
> >(after inlining and other interprocedural optimisations), duplicate
> >them, make a different number of copies of them, etc.
> >
> >Nothing of that is okay if you want to guarantee forward progress on all
> >implementations, and also not if you want to have good performance
> >everywhere (or anywhere even). Unfortunately you have to write all
> >larx/stcx. loops as one block of assembler, so that you know exactly
> >what instructions will end up in your binary.
> >
> >If you don't, it will fail mysteriously after random recompilations, or
> >have performance degradations, etc. You don't want to go there :-)
> >
>
> Could the kernel use GCC builtin atomic functions instead ?
>
> https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
Certainly that should work fine for the simpler cases that the atomic
operations are meant to provide. But esp. for not-so-simple cases the
kernel may require some behaviour provided by the existing assembler
implementation, and not by the atomic builtins.
I'm not saying this cannot work, just that some serious testing will be
needed. If it works it should be the best of all worlds, so then it is
a really good idea yes :-)
Segher
^ permalink raw reply
* [PATCH v3 4/4] powerpc: Move copy_from_kernel_nofault_inst()
From: Christophe Leroy @ 2021-04-14 13:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
jniethe5
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <5f6f82572242a59bfee1e19a71194d8f7ef5fca4.1618405715.git.christophe.leroy@csgroup.eu>
When probe_kernel_read_inst() was created, there was no good place to
put it, so a file called lib/inst.c was dedicated for it.
Since then, probe_kernel_read_inst() has been renamed
copy_from_kernel_nofault_inst(). And mm/maccess.h didn't exist at that
time. Today, mm/maccess.h is related to copy_from_kernel_nofault().
Move copy_from_kernel_nofault_inst() into mm/maccess.c
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Remove inst.o from Makefile
---
arch/powerpc/lib/Makefile | 2 +-
arch/powerpc/lib/inst.c | 26 --------------------------
arch/powerpc/mm/maccess.c | 21 +++++++++++++++++++++
3 files changed, 22 insertions(+), 27 deletions(-)
delete mode 100644 arch/powerpc/lib/inst.c
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index d4efc182662a..f2c690ee75d1 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
endif
-obj-y += alloc.o code-patching.o feature-fixups.o pmem.o inst.o test_code-patching.o
+obj-y += alloc.o code-patching.o feature-fixups.o pmem.o test_code-patching.o
ifndef CONFIG_KASAN
obj-y += string.o memcmp_$(BITS).o
diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
deleted file mode 100644
index e554d1357f2f..000000000000
--- a/arch/powerpc/lib/inst.c
+++ /dev/null
@@ -1,26 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright 2020, IBM Corporation.
- */
-
-#include <linux/uaccess.h>
-#include <asm/disassemble.h>
-#include <asm/inst.h>
-#include <asm/ppc-opcode.h>
-
-int copy_inst_from_kernel_nofault(struct ppc_inst *inst, struct ppc_inst *src)
-{
- unsigned int val, suffix;
- int err;
-
- err = copy_from_kernel_nofault(&val, src, sizeof(val));
- if (err)
- return err;
- if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
- err = copy_from_kernel_nofault(&suffix, (void *)src + 4, 4);
- *inst = ppc_inst_prefix(val, suffix);
- } else {
- *inst = ppc_inst(val);
- }
- return err;
-}
diff --git a/arch/powerpc/mm/maccess.c b/arch/powerpc/mm/maccess.c
index fa9a7a718fc6..a3c30a884076 100644
--- a/arch/powerpc/mm/maccess.c
+++ b/arch/powerpc/mm/maccess.c
@@ -3,7 +3,28 @@
#include <linux/uaccess.h>
#include <linux/kernel.h>
+#include <asm/disassemble.h>
+#include <asm/inst.h>
+#include <asm/ppc-opcode.h>
+
bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
{
return is_kernel_addr((unsigned long)unsafe_src);
}
+
+int copy_inst_from_kernel_nofault(struct ppc_inst *inst, struct ppc_inst *src)
+{
+ unsigned int val, suffix;
+ int err;
+
+ err = copy_from_kernel_nofault(&val, src, sizeof(val));
+ if (err)
+ return err;
+ if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
+ err = copy_from_kernel_nofault(&suffix, (void *)src + 4, 4);
+ *inst = ppc_inst_prefix(val, suffix);
+ } else {
+ *inst = ppc_inst(val);
+ }
+ return err;
+}
--
2.25.0
^ permalink raw reply related
* [PATCH v3 1/4] powerpc: Remove probe_user_read_inst()
From: Christophe Leroy @ 2021-04-14 13:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
jniethe5
Cc: linuxppc-dev, linux-kernel
Its name comes from former probe_user_read() function.
That function is now called copy_from_user_nofault().
probe_user_read_inst() uses copy_from_user_nofault() to read only
a few bytes. It is suboptimal.
It does the same as get_user_inst() but in addition disables
page faults.
But on the other hand, it is not used for the time being. So remove it
for now. If one day it is really needed, we can give it a new name
more in line with today's naming, and implement it using get_user_inst()
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/inst.h | 3 ---
arch/powerpc/lib/inst.c | 31 -------------------------------
2 files changed, 34 deletions(-)
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 19e18af2fac9..2902d4e6a363 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -175,9 +175,6 @@ static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], struct ppc_ins
__str; \
})
-int probe_user_read_inst(struct ppc_inst *inst,
- struct ppc_inst __user *nip);
-
int probe_kernel_read_inst(struct ppc_inst *inst,
struct ppc_inst *src);
diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
index 9cc17eb62462..c57b3548de37 100644
--- a/arch/powerpc/lib/inst.c
+++ b/arch/powerpc/lib/inst.c
@@ -9,24 +9,6 @@
#include <asm/ppc-opcode.h>
#ifdef CONFIG_PPC64
-int probe_user_read_inst(struct ppc_inst *inst,
- struct ppc_inst __user *nip)
-{
- unsigned int val, suffix;
- int err;
-
- err = copy_from_user_nofault(&val, nip, sizeof(val));
- if (err)
- return err;
- if (get_op(val) == OP_PREFIX) {
- err = copy_from_user_nofault(&suffix, (void __user *)nip + 4, 4);
- *inst = ppc_inst_prefix(val, suffix);
- } else {
- *inst = ppc_inst(val);
- }
- return err;
-}
-
int probe_kernel_read_inst(struct ppc_inst *inst,
struct ppc_inst *src)
{
@@ -45,19 +27,6 @@ int probe_kernel_read_inst(struct ppc_inst *inst,
return err;
}
#else /* !CONFIG_PPC64 */
-int probe_user_read_inst(struct ppc_inst *inst,
- struct ppc_inst __user *nip)
-{
- unsigned int val;
- int err;
-
- err = copy_from_user_nofault(&val, nip, sizeof(val));
- if (!err)
- *inst = ppc_inst(val);
-
- return err;
-}
-
int probe_kernel_read_inst(struct ppc_inst *inst,
struct ppc_inst *src)
{
--
2.25.0
^ permalink raw reply related
* [PATCH v3 3/4] powerpc: Rename probe_kernel_read_inst()
From: Christophe Leroy @ 2021-04-14 13:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
jniethe5
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <5f6f82572242a59bfee1e19a71194d8f7ef5fca4.1618405715.git.christophe.leroy@csgroup.eu>
When probe_kernel_read_inst() was created, it was to mimic
probe_kernel_read() function.
Since then, probe_kernel_read() has been renamed
copy_from_kernel_nofault().
Rename probe_kernel_read_inst() into copy_inst_from_kernel_nofault().
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: copy_inst_from_kernel_nofault() instead of copy_from_kernel_nofault_inst()
---
arch/powerpc/include/asm/inst.h | 3 +--
arch/powerpc/kernel/align.c | 2 +-
arch/powerpc/kernel/trace/ftrace.c | 22 +++++++++++-----------
arch/powerpc/lib/inst.c | 3 +--
4 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index a40c3913a4a3..eaf5a6299034 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -177,7 +177,6 @@ static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], struct ppc_ins
__str; \
})
-int probe_kernel_read_inst(struct ppc_inst *inst,
- struct ppc_inst *src);
+int copy_inst_from_kernel_nofault(struct ppc_inst *inst, struct ppc_inst *src);
#endif /* _ASM_POWERPC_INST_H */
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index a97d5f1a3905..8f350d0478e6 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -311,7 +311,7 @@ int fix_alignment(struct pt_regs *regs)
CHECK_FULL_REGS(regs);
if (is_kernel_addr(regs->nip))
- r = probe_kernel_read_inst(&instr, (void *)regs->nip);
+ r = copy_inst_from_kernel_nofault(&instr, (void *)regs->nip);
else
r = __get_user_instr(instr, (void __user *)regs->nip);
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 42761ebec9f7..ffe9537195aa 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -68,7 +68,7 @@ ftrace_modify_code(unsigned long ip, struct ppc_inst old, struct ppc_inst new)
*/
/* read the text we want to modify */
- if (probe_kernel_read_inst(&replaced, (void *)ip))
+ if (copy_inst_from_kernel_nofault(&replaced, (void *)ip))
return -EFAULT;
/* Make sure it is what we expect it to be */
@@ -130,7 +130,7 @@ __ftrace_make_nop(struct module *mod,
struct ppc_inst op, pop;
/* read where this goes */
- if (probe_kernel_read_inst(&op, (void *)ip)) {
+ if (copy_inst_from_kernel_nofault(&op, (void *)ip)) {
pr_err("Fetching opcode failed.\n");
return -EFAULT;
}
@@ -164,7 +164,7 @@ __ftrace_make_nop(struct module *mod,
/* When using -mkernel_profile there is no load to jump over */
pop = ppc_inst(PPC_INST_NOP);
- if (probe_kernel_read_inst(&op, (void *)(ip - 4))) {
+ if (copy_inst_from_kernel_nofault(&op, (void *)(ip - 4))) {
pr_err("Fetching instruction at %lx failed.\n", ip - 4);
return -EFAULT;
}
@@ -197,7 +197,7 @@ __ftrace_make_nop(struct module *mod,
* Check what is in the next instruction. We can see ld r2,40(r1), but
* on first pass after boot we will see mflr r0.
*/
- if (probe_kernel_read_inst(&op, (void *)(ip + 4))) {
+ if (copy_inst_from_kernel_nofault(&op, (void *)(ip + 4))) {
pr_err("Fetching op failed.\n");
return -EFAULT;
}
@@ -349,7 +349,7 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
return -1;
/* New trampoline -- read where this goes */
- if (probe_kernel_read_inst(&op, (void *)tramp)) {
+ if (copy_inst_from_kernel_nofault(&op, (void *)tramp)) {
pr_debug("Fetching opcode failed.\n");
return -1;
}
@@ -399,7 +399,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
struct ppc_inst op;
/* Read where this goes */
- if (probe_kernel_read_inst(&op, (void *)ip)) {
+ if (copy_inst_from_kernel_nofault(&op, (void *)ip)) {
pr_err("Fetching opcode failed.\n");
return -EFAULT;
}
@@ -526,10 +526,10 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
struct module *mod = rec->arch.mod;
/* read where this goes */
- if (probe_kernel_read_inst(op, ip))
+ if (copy_inst_from_kernel_nofault(op, ip))
return -EFAULT;
- if (probe_kernel_read_inst(op + 1, ip + 4))
+ if (copy_inst_from_kernel_nofault(op + 1, ip + 4))
return -EFAULT;
if (!expected_nop_sequence(ip, op[0], op[1])) {
@@ -592,7 +592,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
unsigned long ip = rec->ip;
/* read where this goes */
- if (probe_kernel_read_inst(&op, (void *)ip))
+ if (copy_inst_from_kernel_nofault(&op, (void *)ip))
return -EFAULT;
/* It should be pointing to a nop */
@@ -648,7 +648,7 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
}
/* Make sure we have a nop */
- if (probe_kernel_read_inst(&op, ip)) {
+ if (copy_inst_from_kernel_nofault(&op, ip)) {
pr_err("Unable to read ftrace location %p\n", ip);
return -EFAULT;
}
@@ -726,7 +726,7 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
}
/* read where this goes */
- if (probe_kernel_read_inst(&op, (void *)ip)) {
+ if (copy_inst_from_kernel_nofault(&op, (void *)ip)) {
pr_err("Fetching opcode failed.\n");
return -EFAULT;
}
diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
index 0dff3ac2d45f..e554d1357f2f 100644
--- a/arch/powerpc/lib/inst.c
+++ b/arch/powerpc/lib/inst.c
@@ -8,8 +8,7 @@
#include <asm/inst.h>
#include <asm/ppc-opcode.h>
-int probe_kernel_read_inst(struct ppc_inst *inst,
- struct ppc_inst *src)
+int copy_inst_from_kernel_nofault(struct ppc_inst *inst, struct ppc_inst *src)
{
unsigned int val, suffix;
int err;
--
2.25.0
^ permalink raw reply related
* [PATCH v3 2/4] powerpc: Make probe_kernel_read_inst() common to PPC32 and PPC64
From: Christophe Leroy @ 2021-04-14 13:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
jniethe5
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <5f6f82572242a59bfee1e19a71194d8f7ef5fca4.1618405715.git.christophe.leroy@csgroup.eu>
We have two independant versions of probe_kernel_read_inst(), one for
PPC32 and one for PPC64.
The PPC32 is identical to the first part of the PPC64 version.
The remaining part of PPC64 version is not relevant for PPC32, but
not contradictory, so we can easily have a common function with
the PPC64 part opted out via a IS_ENABLED(CONFIG_PPC64).
The only need is to add a version of ppc_inst_prefix() for PPC32.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/inst.h | 2 ++
arch/powerpc/lib/inst.c | 17 +----------------
2 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 2902d4e6a363..a40c3913a4a3 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -102,6 +102,8 @@ static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
#define ppc_inst(x) ((struct ppc_inst){ .val = x })
+#define ppc_inst_prefix(x, y) ppc_inst(x)
+
static inline bool ppc_inst_prefixed(struct ppc_inst x)
{
return false;
diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
index c57b3548de37..0dff3ac2d45f 100644
--- a/arch/powerpc/lib/inst.c
+++ b/arch/powerpc/lib/inst.c
@@ -8,7 +8,6 @@
#include <asm/inst.h>
#include <asm/ppc-opcode.h>
-#ifdef CONFIG_PPC64
int probe_kernel_read_inst(struct ppc_inst *inst,
struct ppc_inst *src)
{
@@ -18,7 +17,7 @@ int probe_kernel_read_inst(struct ppc_inst *inst,
err = copy_from_kernel_nofault(&val, src, sizeof(val));
if (err)
return err;
- if (get_op(val) == OP_PREFIX) {
+ if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
err = copy_from_kernel_nofault(&suffix, (void *)src + 4, 4);
*inst = ppc_inst_prefix(val, suffix);
} else {
@@ -26,17 +25,3 @@ int probe_kernel_read_inst(struct ppc_inst *inst,
}
return err;
}
-#else /* !CONFIG_PPC64 */
-int probe_kernel_read_inst(struct ppc_inst *inst,
- struct ppc_inst *src)
-{
- unsigned int val;
- int err;
-
- err = copy_from_kernel_nofault(&val, src, sizeof(val));
- if (!err)
- *inst = ppc_inst(val);
-
- return err;
-}
-#endif /* CONFIG_PPC64 */
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v2 3/4] powerpc: Rename probe_kernel_read_inst()
From: Christophe Leroy @ 2021-04-14 12:57 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, jniethe5
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <874kg930di.fsf@linux.ibm.com>
Le 14/04/2021 à 07:23, Aneesh Kumar K.V a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>
>> When probe_kernel_read_inst() was created, it was to mimic
>> probe_kernel_read() function.
>>
>> Since then, probe_kernel_read() has been renamed
>> copy_from_kernel_nofault().
>>
>> Rename probe_kernel_read_inst() into copy_from_kernel_nofault_inst().
>
> At first glance I read it as copy from kernel nofault instruction.
> How about copy_inst_from_kernel_nofault()?
Yes good idea.
Christophe
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] of: net: pass the dst buffer to of_get_mac_address()
From: Michael Walle @ 2021-04-14 12:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: kbuild-all, lkp, netdev, kbuild, linux-kernel, ath9k-devel,
linux-renesas-soc, linux-mediatek, linux-amlogic, UNGLinuxDriver,
linuxppc-dev, linux-stm32, linux-arm-kernel
In-Reply-To: <20210414053336.GQ6021@kadam>
Hi Dan,
Am 2021-04-14 07:33, schrieb Dan Carpenter:
> url:
> https://github.com/0day-ci/linux/commits/Michael-Walle/of-net-support-non-platform-devices-in-of_get_mac_address/20210406-234030
> base:
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
> cc0626c2aaed8e475efdd85fa374b497a7192e35
> config: x86_64-randconfig-m001-20210406 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:2069 axienet_probe()
> warn: passing a valid pointer to 'PTR_ERR'
>
> vim +/PTR_ERR +2069 drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>
> 522856cefaf09d Robert Hancock 2019-06-06 2060 /* Check for
> Ethernet core IRQ (optional) */
> 522856cefaf09d Robert Hancock 2019-06-06 2061 if (lp->eth_irq
> <= 0)
> 522856cefaf09d Robert Hancock 2019-06-06 2062
> dev_info(&pdev->dev, "Ethernet core IRQ not defined\n");
> 522856cefaf09d Robert Hancock 2019-06-06 2063
> 8a3b7a252dca9f Daniel Borkmann 2012-01-19 2064 /* Retrieve the
> MAC address */
> 411b125c6ace1f Michael Walle 2021-04-06 2065 ret =
> of_get_mac_address(pdev->dev.of_node, mac_addr);
> 411b125c6ace1f Michael Walle 2021-04-06 2066 if (!ret) {
> 411b125c6ace1f Michael Walle 2021-04-06 2067
> axienet_set_mac_address(ndev, mac_addr);
> 411b125c6ace1f Michael Walle 2021-04-06 2068 } else {
> d05a9ed5c3a773 Robert Hancock 2019-06-06 @2069
> dev_warn(&pdev->dev, "could not find MAC address property: %ld\n",
> d05a9ed5c3a773 Robert Hancock 2019-06-06 2070
> PTR_ERR(mac_addr));
>
> ^^^^^^^^^^^^^^^^^
> This should print "ret".
Thanks, this was fixed (in the now merged) v4. I forgot
to add you to that huge CC list. Sorry for that.
-michael
^ permalink raw reply
* Re: [PATCH v1 1/2] powerpc/bitops: Use immediate operand when possible
From: Christophe Leroy @ 2021-04-14 12:42 UTC (permalink / raw)
To: Segher Boessenkool, Nicholas Piggin
Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <20210414122409.GV26583@gate.crashing.org>
Le 14/04/2021 à 14:24, Segher Boessenkool a écrit :
> On Wed, Apr 14, 2021 at 12:01:21PM +1000, Nicholas Piggin wrote:
>> Would be nice if we could let the compiler deal with it all...
>>
>> static inline unsigned long lr(unsigned long *mem)
>> {
>> unsigned long val;
>>
>> /*
>> * This doesn't clobber memory but want to avoid memory operations
>> * moving ahead of it
>> */
>> asm volatile("ldarx %0, %y1" : "=r"(val) : "Z"(*mem) : "memory");
>>
>> return val;
>> }
>
> (etc.)
>
> That can not work reliably: the compiler can put random instructions
> between the larx and stcx. this way, and you then do not have guaranteed
> forward progress anymore. It can put the two in different routines
> (after inlining and other interprocedural optimisations), duplicate
> them, make a different number of copies of them, etc.
>
> Nothing of that is okay if you want to guarantee forward progress on all
> implementations, and also not if you want to have good performance
> everywhere (or anywhere even). Unfortunately you have to write all
> larx/stcx. loops as one block of assembler, so that you know exactly
> what instructions will end up in your binary.
>
> If you don't, it will fail mysteriously after random recompilations, or
> have performance degradations, etc. You don't want to go there :-)
>
Could the kernel use GCC builtin atomic functions instead ?
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
^ permalink raw reply
* [PATCH] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
From: Vaibhav Jain @ 2021-04-14 12:40 UTC (permalink / raw)
To: linuxppc-dev, linux-nvdimm
Cc: Santosh Sivaraj, Aneesh Kumar K . V, Vaibhav Jain, Dan Williams,
Ira Weiny
Currently drc_pmem_qeury_stats() generates a dev_err in case
"Enable Performance Information Collection" feature is disabled from
HMC. The error is of the form below:
papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
performance stats, Err:-10
This error message confuses users as it implies a possible problem
with the nvdimm even though its due to a disabled feature.
So we fix this by explicitly handling the H_AUTHORITY error from the
H_SCM_PERFORMANCE_STATS hcall and generating a warning instead of an
error, saying that "Performance stats in-accessible".
Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
arch/powerpc/platforms/pseries/papr_scm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 835163f54244..9216424f8be3 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -277,6 +277,9 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
dev_err(&p->pdev->dev,
"Unknown performance stats, Err:0x%016lX\n", ret[0]);
return -ENOENT;
+ } else if (rc == H_AUTHORITY) {
+ dev_warn(&p->pdev->dev, "Performance stats in-accessible");
+ return -EPERM;
} else if (rc != H_SUCCESS) {
dev_err(&p->pdev->dev,
"Failed to query performance stats, Err:%lld\n", rc);
--
2.30.2
^ permalink raw reply related
* Re: [PATCH v1 1/2] powerpc/bitops: Use immediate operand when possible
From: Segher Boessenkool @ 2021-04-14 12:24 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <1618365589.67fxh7cot9.astroid@bobo.none>
On Wed, Apr 14, 2021 at 12:01:21PM +1000, Nicholas Piggin wrote:
> Would be nice if we could let the compiler deal with it all...
>
> static inline unsigned long lr(unsigned long *mem)
> {
> unsigned long val;
>
> /*
> * This doesn't clobber memory but want to avoid memory operations
> * moving ahead of it
> */
> asm volatile("ldarx %0, %y1" : "=r"(val) : "Z"(*mem) : "memory");
>
> return val;
> }
(etc.)
That can not work reliably: the compiler can put random instructions
between the larx and stcx. this way, and you then do not have guaranteed
forward progress anymore. It can put the two in different routines
(after inlining and other interprocedural optimisations), duplicate
them, make a different number of copies of them, etc.
Nothing of that is okay if you want to guarantee forward progress on all
implementations, and also not if you want to have good performance
everywhere (or anywhere even). Unfortunately you have to write all
larx/stcx. loops as one block of assembler, so that you know exactly
what instructions will end up in your binary.
If you don't, it will fail mysteriously after random recompilations, or
have performance degradations, etc. You don't want to go there :-)
Segher
^ permalink raw reply
* Re: [PATCH 1/1] mm: Fix struct page layout on 32-bit systems
From: Ilias Apalodimas @ 2021-04-14 11:56 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Arnd Bergmann, Grygorii Strashko, netdev, linux-kernel,
linux-mips, linux-mm, Jesper Dangaard Brouer, Matteo Croce,
linuxppc-dev, Christoph Hellwig, linux-arm-kernel
In-Reply-To: <20210414115052.GS2531743@casper.infradead.org>
On Wed, Apr 14, 2021 at 12:50:52PM +0100, Matthew Wilcox wrote:
> On Wed, Apr 14, 2021 at 10:10:44AM +0200, Jesper Dangaard Brouer wrote:
> > Yes, indeed! - And very frustrating. It's keeping me up at night.
> > I'm dreaming about 32 vs 64 bit data structures. My fitbit stats tell
> > me that I don't sleep well with these kind of dreams ;-)
>
> Then you're going to love this ... even with the latest patch, there's
> still a problem. Because dma_addr_t is still 64-bit aligned _as a type_,
> that forces the union to be 64-bit aligned (as we already knew and worked
> around), but what I'd forgotten is that forces the entirety of struct
> page to be 64-bit aligned. Which means ...
>
> /* size: 40, cachelines: 1, members: 4 */
> /* padding: 4 */
> /* forced alignments: 1 */
> /* last cacheline: 40 bytes */
> } __attribute__((__aligned__(8)));
>
> .. that we still have a hole! It's just moved from being at offset 4
> to being at offset 36.
>
> > That said, I think we need to have a quicker fix for the immediate
> > issue with 64-bit bit dma_addr on 32-bit arch and the misalignment hole
> > it leaves[3] in struct page. In[3] you mention ppc32, does it only
> > happens on certain 32-bit archs?
>
> AFAICT it happens on mips32, ppc32, arm32 and arc. It doesn't happen
> on x86-32 because dma_addr_t is 32-bit aligned.
>
> Doing this fixes it:
>
> +++ b/include/linux/types.h
> @@ -140,7 +140,7 @@ typedef u64 blkcnt_t;
> * so they don't care about the size of the actual bus addresses.
> */
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -typedef u64 dma_addr_t;
> +typedef u64 __attribute__((aligned(sizeof(void *)))) dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
>
> > I'm seriously considering removing page_pool's support for doing/keeping
> > DMA-mappings on 32-bit arch's. AFAIK only a single driver use this.
>
> ... if you're going to do that, then we don't need to do this.
FWIW I already proposed that to Matthew in private a few days ago...
II am not even sure the AM572x has that support. I'd much prefer getting rid
of it as well, instead of overcomplicating the struct for a device noone is
going to need.
Cheers
/Ilias
^ permalink raw reply
* Re: [PATCH 1/1] mm: Fix struct page layout on 32-bit systems
From: Matthew Wilcox @ 2021-04-14 11:50 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Arnd Bergmann, Grygorii Strashko, netdev, Ilias Apalodimas,
linux-mips, linux-kernel, linux-mm, Matteo Croce, linuxppc-dev,
Christoph Hellwig, linux-arm-kernel
In-Reply-To: <20210414101044.19da09df@carbon>
On Wed, Apr 14, 2021 at 10:10:44AM +0200, Jesper Dangaard Brouer wrote:
> Yes, indeed! - And very frustrating. It's keeping me up at night.
> I'm dreaming about 32 vs 64 bit data structures. My fitbit stats tell
> me that I don't sleep well with these kind of dreams ;-)
Then you're going to love this ... even with the latest patch, there's
still a problem. Because dma_addr_t is still 64-bit aligned _as a type_,
that forces the union to be 64-bit aligned (as we already knew and worked
around), but what I'd forgotten is that forces the entirety of struct
page to be 64-bit aligned. Which means ...
/* size: 40, cachelines: 1, members: 4 */
/* padding: 4 */
/* forced alignments: 1 */
/* last cacheline: 40 bytes */
} __attribute__((__aligned__(8)));
.. that we still have a hole! It's just moved from being at offset 4
to being at offset 36.
> That said, I think we need to have a quicker fix for the immediate
> issue with 64-bit bit dma_addr on 32-bit arch and the misalignment hole
> it leaves[3] in struct page. In[3] you mention ppc32, does it only
> happens on certain 32-bit archs?
AFAICT it happens on mips32, ppc32, arm32 and arc. It doesn't happen
on x86-32 because dma_addr_t is 32-bit aligned.
Doing this fixes it:
+++ b/include/linux/types.h
@@ -140,7 +140,7 @@ typedef u64 blkcnt_t;
* so they don't care about the size of the actual bus addresses.
*/
#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-typedef u64 dma_addr_t;
+typedef u64 __attribute__((aligned(sizeof(void *)))) dma_addr_t;
#else
typedef u32 dma_addr_t;
#endif
> I'm seriously considering removing page_pool's support for doing/keeping
> DMA-mappings on 32-bit arch's. AFAIK only a single driver use this.
... if you're going to do that, then we don't need to do this.
^ permalink raw reply
* [PATCH v5] powerpc/traps: Enhance readability for trap types
From: Xiongwei Song @ 2021-04-14 11:00 UTC (permalink / raw)
To: mpe, benh, paulus, npiggin, christophe.leroy, hbathini, rppt,
akpm, mahesh, nathan, sourabhjain, aneesh.kumar, ravi.bangoria,
mikey, haren, alistair, jniethe5, peterz, leobras.c, walken,
atrajeev, maddy, kjain, kan.liang, aik
Cc: linuxppc-dev, linux-kernel
From: Xiongwei Song <sxwjean@gmail.com>
Define macros to list ppc interrupt types in interttupt.h, replace the
reference of the trap hex values with these macros.
Referred the hex numbers in arch/powerpc/kernel/exceptions-64e.S,
arch/powerpc/kernel/exceptions-64s.S, arch/powerpc/kernel/head_*.S,
arch/powerpc/kernel/head_booke.h and arch/powerpc/include/asm/kvm_asm.h.
v4-v5:
* Delete unnecessary #ifdef.
* Move INTERRUPT_* macros to interttupt.h, classify for different cpu
types. Drop traps.h.
* Directly define INTERRUPT_MACHINE_CHECK with 0x200.
v3-v4:
Fix compile issue:
arch/powerpc/kernel/process.c:1473:14: error: 'INTERRUPT_MACHINE_CHECK' undeclared (first use in this function); did you mean 'TAINT_MACHINE_CHECK'?
Reported-by: kernel test robot <lkp@intel.com>
v2-v3:
Correct the prefix of trap macros with INTERRUPT_, the previous prefix
is TRAP_, which is not precise. This is suggested by Segher Boessenkool
and Nicholas Piggin.
v1-v2:
Define more trap macros to replace more trap hexs in code, not just for
the __show_regs function. This is suggested by Christophe Leroy.
Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
---
arch/powerpc/include/asm/interrupt.h | 48 +++++++++++++++++++++++++--
arch/powerpc/kernel/fadump.c | 2 +-
arch/powerpc/kernel/interrupt.c | 2 +-
arch/powerpc/kernel/process.c | 4 ++-
arch/powerpc/kernel/traps.c | 6 ++--
arch/powerpc/kexec/crash.c | 3 +-
arch/powerpc/mm/book3s64/hash_utils.c | 4 +--
arch/powerpc/mm/fault.c | 16 ++++-----
arch/powerpc/perf/core-book3s.c | 5 +--
arch/powerpc/xmon/xmon.c | 20 +++++++----
10 files changed, 81 insertions(+), 29 deletions(-)
diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
index 05e7fc4ffb50..86daf1242088 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -9,6 +9,46 @@
#include <asm/kprobes.h>
#include <asm/runlatch.h>
+/* BookE/4xx */
+#define INTERRUPT_CRITICAL_INPUT 0x100
+
+/* BookE */
+#define INTERRUPT_DEBUG 0xd00
+#ifdef CONFIG_BOOKE
+#define INTERRUPT_PERFMON 0x260
+#define INTERRUPT_DOORBELL 0x280
+#endif
+
+/* BookS/4xx/8xx */
+#define INTERRUPT_MACHINE_CHECK 0x200
+
+/* BookS/8xx */
+#define INTERRUPT_SYSTEM_RESET 0x100
+
+/* BookS */
+#define INTERRUPT_DATA_SEGMENT 0x380
+#define INTERRUPT_INST_SEGMENT 0x480
+#define INTERRUPT_TRACE 0xd00
+#define INTERRUPT_H_DATA_STORAGE 0xe00
+#define INTERRUPT_H_FAC_UNAVAIL 0xf80
+#ifdef CONFIG_PPC_BOOK3S
+#define INTERRUPT_DOORBELL 0xa00
+#define INTERRUPT_PERFMON 0xf00
+#endif
+
+/* BookE/BookS/4xx/8xx */
+#define INTERRUPT_DATA_STORAGE 0x300
+#define INTERRUPT_INST_STORAGE 0x400
+#define INTERRUPT_ALIGNMENT 0x600
+#define INTERRUPT_PROGRAM 0x700
+#define INTERRUPT_SYSCALL 0xc00
+
+/* BookE/BookS/44x */
+#define INTERRUPT_FP_UNAVAIL 0x800
+
+/* BookE/BookS/44x/8xx */
+#define INTERRUPT_DECREMENTER 0x900
+
static inline void nap_adjust_return(struct pt_regs *regs)
{
#ifdef CONFIG_PPC_970_NAP
@@ -70,7 +110,7 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
* CT_WARN_ON comes here via program_check_exception,
* so avoid recursion.
*/
- if (TRAP(regs) != 0x700)
+ if (TRAP(regs) != INTERRUPT_PROGRAM)
CT_WARN_ON(ct_state() != CONTEXT_KERNEL);
}
#endif
@@ -175,7 +215,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs *regs, struct inte
/* Don't do any per-CPU operations until interrupt state is fixed */
#endif
/* Allow DEC and PMI to be traced when they are soft-NMI */
- if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260) {
+ if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+ TRAP(regs) != INTERRUPT_PERFMON) {
state->ftrace_enabled = this_cpu_get_ftrace_enabled();
this_cpu_set_ftrace_enabled(0);
}
@@ -204,7 +245,8 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
*/
#ifdef CONFIG_PPC64
- if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
+ if (TRAP(regs) != INTERRUPT_DECREMENTER &&
+ TRAP(regs) != INTERRUPT_PERFMON)
this_cpu_set_ftrace_enabled(state->ftrace_enabled);
#ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index eddf362caedc..b55b4c23f3b6 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -728,7 +728,7 @@ void crash_fadump(struct pt_regs *regs, const char *str)
* If we came in via system reset, wait a while for the secondary
* CPUs to enter.
*/
- if (TRAP(&(fdh->regs)) == 0x100) {
+ if (TRAP(&(fdh->regs)) == INTERRUPT_SYSTEM_RESET) {
msecs = CRASH_TIMEOUT;
while ((atomic_read(&cpus_in_fadump) < ncpus) && (--msecs > 0))
mdelay(1);
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4dd4b8f9cfa..9e0f02980494 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -456,7 +456,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
* CT_WARN_ON comes here via program_check_exception,
* so avoid recursion.
*/
- if (TRAP(regs) != 0x700)
+ if (TRAP(regs) != INTERRUPT_PROGRAM)
CT_WARN_ON(ct_state() == CONTEXT_USER);
kuap = kuap_get_and_assert_locked();
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b966c8e0cead..53792a999cbb 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1469,7 +1469,9 @@ static void __show_regs(struct pt_regs *regs)
trap = TRAP(regs);
if (!trap_is_syscall(regs) && cpu_has_feature(CPU_FTR_CFAR))
pr_cont("CFAR: "REG" ", regs->orig_gpr3);
- if (trap == 0x200 || trap == 0x300 || trap == 0x600) {
+ if (trap == INTERRUPT_MACHINE_CHECK ||
+ trap == INTERRUPT_DATA_STORAGE ||
+ trap == INTERRUPT_ALIGNMENT) {
if (IS_ENABLED(CONFIG_4xx) || IS_ENABLED(CONFIG_BOOKE))
pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
else
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index efba99870691..a9b11ba77c4d 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -221,7 +221,7 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
/*
* system_reset_excption handles debugger, crash dump, panic, for 0x100
*/
- if (TRAP(regs) == 0x100)
+ if (TRAP(regs) == INTERRUPT_SYSTEM_RESET)
return;
crash_fadump(regs, "die oops");
@@ -289,7 +289,7 @@ void die(const char *str, struct pt_regs *regs, long err)
/*
* system_reset_excption handles debugger, crash dump, panic, for 0x100
*/
- if (TRAP(regs) != 0x100) {
+ if (TRAP(regs) != INTERRUPT_SYSTEM_RESET) {
if (debugger(regs))
return;
}
@@ -1682,7 +1682,7 @@ DEFINE_INTERRUPT_HANDLER(facility_unavailable_exception)
u8 status;
bool hv;
- hv = (TRAP(regs) == 0xf80);
+ hv = (TRAP(regs) == INTERRUPT_H_FAC_UNAVAIL);
if (hv)
value = mfspr(SPRN_HFSCR);
else
diff --git a/arch/powerpc/kexec/crash.c b/arch/powerpc/kexec/crash.c
index c9a889880214..0196d0c211ac 100644
--- a/arch/powerpc/kexec/crash.c
+++ b/arch/powerpc/kexec/crash.c
@@ -24,6 +24,7 @@
#include <asm/smp.h>
#include <asm/setjmp.h>
#include <asm/debug.h>
+#include <asm/interrupt.h>
/*
* The primary CPU waits a while for all secondary CPUs to enter. This is to
@@ -336,7 +337,7 @@ void default_machine_crash_shutdown(struct pt_regs *regs)
* If we came in via system reset, wait a while for the secondary
* CPUs to enter.
*/
- if (TRAP(regs) == 0x100)
+ if (TRAP(regs) == INTERRUPT_SYSTEM_RESET)
mdelay(PRIMARY_TIMEOUT);
crash_kexec_prepare_cpus(crashing_cpu);
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 7719995323c3..ab49216c0ef3 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -1145,7 +1145,7 @@ unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
/* page is dirty */
if (!test_bit(PG_dcache_clean, &page->flags) && !PageReserved(page)) {
- if (trap == 0x400) {
+ if (trap == INTERRUPT_INST_STORAGE) {
flush_dcache_icache_page(page);
set_bit(PG_dcache_clean, &page->flags);
} else
@@ -1545,7 +1545,7 @@ DEFINE_INTERRUPT_HANDLER_RET(__do_hash_fault)
if (user_mode(regs) || (region_id == USER_REGION_ID))
access &= ~_PAGE_PRIVILEGED;
- if (TRAP(regs) == 0x400)
+ if (TRAP(regs) == INTERRUPT_INST_STORAGE)
access |= _PAGE_EXEC;
err = hash_page_mm(mm, ea, access, TRAP(regs), flags);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 0c0b1c2cfb49..25523f371c79 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -197,7 +197,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
unsigned long address, bool is_write)
{
- int is_exec = TRAP(regs) == 0x400;
+ int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
@@ -391,7 +391,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
struct vm_area_struct * vma;
struct mm_struct *mm = current->mm;
unsigned int flags = FAULT_FLAG_DEFAULT;
- int is_exec = TRAP(regs) == 0x400;
+ int is_exec = TRAP(regs) == INTERRUPT_INST_STORAGE;
int is_user = user_mode(regs);
int is_write = page_fault_is_write(error_code);
vm_fault_t fault, major = 0;
@@ -588,20 +588,20 @@ void __bad_page_fault(struct pt_regs *regs, int sig)
/* kernel has accessed a bad area */
switch (TRAP(regs)) {
- case 0x300:
- case 0x380:
- case 0xe00:
+ case INTERRUPT_DATA_STORAGE:
+ case INTERRUPT_DATA_SEGMENT:
+ case INTERRUPT_H_DATA_STORAGE:
pr_alert("BUG: %s on %s at 0x%08lx\n",
regs->dar < PAGE_SIZE ? "Kernel NULL pointer dereference" :
"Unable to handle kernel data access",
is_write ? "write" : "read", regs->dar);
break;
- case 0x400:
- case 0x480:
+ case INTERRUPT_INST_STORAGE:
+ case INTERRUPT_INST_SEGMENT:
pr_alert("BUG: Unable to handle kernel instruction fetch%s",
regs->nip < PAGE_SIZE ? " (NULL pointer?)\n" : "\n");
break;
- case 0x600:
+ case INTERRUPT_ALIGNMENT:
pr_alert("BUG: Unable to handle kernel unaligned access at 0x%08lx\n",
regs->dar);
break;
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 766f064f00fb..2ae6126ac8f2 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -17,6 +17,7 @@
#include <asm/firmware.h>
#include <asm/ptrace.h>
#include <asm/code-patching.h>
+#include <asm/interrupt.h>
#ifdef CONFIG_PPC64
#include "internal.h"
@@ -168,7 +169,7 @@ static bool regs_use_siar(struct pt_regs *regs)
* they have not been setup using perf_read_regs() and so regs->result
* is something random.
*/
- return ((TRAP(regs) == 0xf00) && regs->result);
+ return ((TRAP(regs) == INTERRUPT_PERFMON) && regs->result);
}
/*
@@ -347,7 +348,7 @@ static inline void perf_read_regs(struct pt_regs *regs)
* hypervisor samples as well as samples in the kernel with
* interrupts off hence the userspace check.
*/
- if (TRAP(regs) != 0xf00)
+ if (TRAP(regs) != INTERRUPT_PERFMON)
use_siar = 0;
else if ((ppmu->flags & PPMU_NO_SIAR))
use_siar = 0;
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 3fe37495f63d..62544a7ab437 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -54,6 +54,7 @@
#include <asm/code-patching.h>
#include <asm/sections.h>
#include <asm/inst.h>
+#include <asm/interrupt.h>
#ifdef CONFIG_PPC64
#include <asm/hvcall.h>
@@ -605,7 +606,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
* debugger break (IPI). This is similar to
* crash_kexec_secondary().
*/
- if (TRAP(regs) != 0x100 || !wait_for_other_cpus(ncpus))
+ if (TRAP(regs) != INTERRUPT_SYSTEM_RESET || !wait_for_other_cpus(ncpus))
smp_send_debugger_break();
wait_for_other_cpus(ncpus);
@@ -615,7 +616,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
if (!locked_down) {
/* for breakpoint or single step, print curr insn */
- if (bp || TRAP(regs) == 0xd00)
+ if (bp || TRAP(regs) == INTERRUPT_TRACE)
ppc_inst_dump(regs->nip, 1, 0);
printf("enter ? for help\n");
}
@@ -684,7 +685,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
disable_surveillance();
if (!locked_down) {
/* for breakpoint or single step, print current insn */
- if (bp || TRAP(regs) == 0xd00)
+ if (bp || TRAP(regs) == INTERRUPT_TRACE)
ppc_inst_dump(regs->nip, 1, 0);
printf("enter ? for help\n");
}
@@ -1769,9 +1770,12 @@ static void excprint(struct pt_regs *fp)
printf(" sp: %lx\n", fp->gpr[1]);
printf(" msr: %lx\n", fp->msr);
- if (trap == 0x300 || trap == 0x380 || trap == 0x600 || trap == 0x200) {
+ if (trap == INTERRUPT_DATA_STORAGE ||
+ trap == INTERRUPT_DATA_SEGMENT ||
+ trap == INTERRUPT_ALIGNMENT ||
+ trap == INTERRUPT_MACHINE_CHECK) {
printf(" dar: %lx\n", fp->dar);
- if (trap != 0x380)
+ if (trap != INTERRUPT_DATA_SEGMENT)
printf(" dsisr: %lx\n", fp->dsisr);
}
@@ -1785,7 +1789,7 @@ static void excprint(struct pt_regs *fp)
current->pid, current->comm);
}
- if (trap == 0x700)
+ if (trap == INTERRUPT_PROGRAM)
print_bug_trap(fp);
printf(linux_banner);
@@ -1846,7 +1850,9 @@ static void prregs(struct pt_regs *fp)
printf("ctr = "REG" xer = "REG" trap = %4lx\n",
fp->ctr, fp->xer, fp->trap);
trap = TRAP(fp);
- if (trap == 0x300 || trap == 0x380 || trap == 0x600)
+ if (trap == INTERRUPT_DATA_STORAGE ||
+ trap == INTERRUPT_DATA_SEGMENT ||
+ trap == INTERRUPT_ALIGNMENT)
printf("dar = "REG" dsisr = %.8lx\n", fp->dar, fp->dsisr);
}
--
2.26.1
^ permalink raw reply related
* [PATCH v2] init: consolidate trap_init()
From: Jisheng Zhang @ 2021-04-14 10:00 UTC (permalink / raw)
To: Vineet Gupta, Russell King, Yoshinori Sato, Brian Cain, Nick Hu,
Greentime Hu, Vincent Chen, Ley Foon Tan, Jonas Bonn,
Stefan Kristiansson, Stafford Horne, James E.J. Bottomley,
Helge Deller, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Jeff Dike, Richard Weinberger, Anton Ivanov, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Michal Simek, David S. Miller,
Christophe Leroy
Cc: uclinux-h8-devel, linux-parisc, linux-hexagon, linux-alpha,
linux-um, linux-kernel, openrisc, Anup Patel, sparclinux,
linux-riscv, linux-snps-arc, linuxppc-dev, linux-arm-kernel
In-Reply-To: <20210414165808.458a3d11@xhacker.debian>
Many architectures implement the trap_init() as NOP, since there is
no such default for trap_init(), this empty stub is duplicated among
these architectures. Provide a generic but weak NOP implementation
to drop the empty stubs of trap_init() in these architectures.
The alpha, microblaze and sparc32 have real trap_init() implementation
but the __init marker is missing, so add it to these three platforms.
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
Since v1:
- add __init marker to trap_init() for alpha, microblaze and sparc32
- adjust the generic weak NOP trap_init() location to make it sits with
other NOP implementations together
arch/alpha/kernel/traps.c | 2 +-
arch/arc/kernel/traps.c | 5 -----
arch/arm/kernel/traps.c | 5 -----
arch/h8300/kernel/traps.c | 13 -------------
arch/hexagon/kernel/traps.c | 4 ----
arch/microblaze/kernel/traps.c | 2 +-
arch/nds32/kernel/traps.c | 5 -----
arch/nios2/kernel/traps.c | 5 -----
arch/openrisc/kernel/traps.c | 5 -----
arch/parisc/kernel/traps.c | 4 ----
arch/powerpc/kernel/traps.c | 5 -----
arch/riscv/kernel/traps.c | 5 -----
arch/sparc/kernel/traps_32.c | 2 +-
arch/um/kernel/trap.c | 4 ----
init/main.c | 2 ++
15 files changed, 5 insertions(+), 63 deletions(-)
diff --git a/arch/alpha/kernel/traps.c b/arch/alpha/kernel/traps.c
index 921d4b6e4d95..96b203199c6c 100644
--- a/arch/alpha/kernel/traps.c
+++ b/arch/alpha/kernel/traps.c
@@ -973,7 +973,7 @@ do_entUnaUser(void __user * va, unsigned long opcode,
return;
}
-void
+void __init
trap_init(void)
{
/* Tell PAL-code what global pointer we want in the kernel. */
diff --git a/arch/arc/kernel/traps.c b/arch/arc/kernel/traps.c
index 57235e5c0cea..6b83e3f2b41c 100644
--- a/arch/arc/kernel/traps.c
+++ b/arch/arc/kernel/traps.c
@@ -20,11 +20,6 @@
#include <asm/unaligned.h>
#include <asm/kprobes.h>
-void __init trap_init(void)
-{
- return;
-}
-
void die(const char *str, struct pt_regs *regs, unsigned long address)
{
show_kernel_fault_diag(str, regs, address);
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 17d5a785df28..9baccef20392 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -780,11 +780,6 @@ void abort(void)
panic("Oops failed to kill thread");
}
-void __init trap_init(void)
-{
- return;
-}
-
#ifdef CONFIG_KUSER_HELPERS
static void __init kuser_init(void *vectors)
{
diff --git a/arch/h8300/kernel/traps.c b/arch/h8300/kernel/traps.c
index 5d8b969cd8f3..c3a3ebf77fbb 100644
--- a/arch/h8300/kernel/traps.c
+++ b/arch/h8300/kernel/traps.c
@@ -30,19 +30,6 @@
static DEFINE_SPINLOCK(die_lock);
-/*
- * this must be called very early as the kernel might
- * use some instruction that are emulated on the 060
- */
-
-void __init base_trap_init(void)
-{
-}
-
-void __init trap_init(void)
-{
-}
-
asmlinkage void set_esp0(unsigned long ssp)
{
current->thread.esp0 = ssp;
diff --git a/arch/hexagon/kernel/traps.c b/arch/hexagon/kernel/traps.c
index 904134b37232..edfc35dafeb1 100644
--- a/arch/hexagon/kernel/traps.c
+++ b/arch/hexagon/kernel/traps.c
@@ -28,10 +28,6 @@
#define TRAP_SYSCALL 1
#define TRAP_DEBUG 0xdb
-void __init trap_init(void)
-{
-}
-
#ifdef CONFIG_GENERIC_BUG
/* Maybe should resemble arch/sh/kernel/traps.c ?? */
int is_valid_bugaddr(unsigned long addr)
diff --git a/arch/microblaze/kernel/traps.c b/arch/microblaze/kernel/traps.c
index 94b6fe93147d..7c15704fe56e 100644
--- a/arch/microblaze/kernel/traps.c
+++ b/arch/microblaze/kernel/traps.c
@@ -18,7 +18,7 @@
#include <asm/exceptions.h>
#include <asm/unwind.h>
-void trap_init(void)
+void __init trap_init(void)
{
__enable_hw_exceptions();
}
diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
index ee0d9ae192a5..f06421c645af 100644
--- a/arch/nds32/kernel/traps.c
+++ b/arch/nds32/kernel/traps.c
@@ -183,11 +183,6 @@ void __pgd_error(const char *file, int line, unsigned long val)
}
extern char *exception_vector, *exception_vector_end;
-void __init trap_init(void)
-{
- return;
-}
-
void __init early_trap_init(void)
{
unsigned long ivb = 0;
diff --git a/arch/nios2/kernel/traps.c b/arch/nios2/kernel/traps.c
index b172da4eb1a9..596986a74a26 100644
--- a/arch/nios2/kernel/traps.c
+++ b/arch/nios2/kernel/traps.c
@@ -105,11 +105,6 @@ void show_stack(struct task_struct *task, unsigned long *stack,
printk("%s\n", loglvl);
}
-void __init trap_init(void)
-{
- /* Nothing to do here */
-}
-
/* Breakpoint handler */
asmlinkage void breakpoint_c(struct pt_regs *fp)
{
diff --git a/arch/openrisc/kernel/traps.c b/arch/openrisc/kernel/traps.c
index 4d61333c2623..aa1e709405ac 100644
--- a/arch/openrisc/kernel/traps.c
+++ b/arch/openrisc/kernel/traps.c
@@ -231,11 +231,6 @@ void unhandled_exception(struct pt_regs *regs, int ea, int vector)
die("Oops", regs, 9);
}
-void __init trap_init(void)
-{
- /* Nothing needs to be done */
-}
-
asmlinkage void do_trap(struct pt_regs *regs, unsigned long address)
{
force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->pc);
diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c
index 8d8441d4562a..747c328fb886 100644
--- a/arch/parisc/kernel/traps.c
+++ b/arch/parisc/kernel/traps.c
@@ -859,7 +859,3 @@ void __init early_trap_init(void)
initialize_ivt(&fault_vector_20);
}
-
-void __init trap_init(void)
-{
-}
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index a44a30b0688c..e952bee89684 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -2207,11 +2207,6 @@ DEFINE_INTERRUPT_HANDLER(kernel_bad_stack)
die("Bad kernel stack pointer", regs, SIGABRT);
}
-void __init trap_init(void)
-{
-}
-
-
#ifdef CONFIG_PPC_EMULATED_STATS
#define WARN_EMULATED_SETUP(type) .type = { .name = #type }
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 0879b5df11b9..b3f3d84de779 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -194,8 +194,3 @@ int is_valid_bugaddr(unsigned long pc)
return ((insn & __COMPRESSED_INSN_MASK) == __BUG_INSN_16);
}
#endif /* CONFIG_GENERIC_BUG */
-
-/* stvec & scratch is already set from head.S */
-void trap_init(void)
-{
-}
diff --git a/arch/sparc/kernel/traps_32.c b/arch/sparc/kernel/traps_32.c
index 247a0d9683b2..c4df20ec01e2 100644
--- a/arch/sparc/kernel/traps_32.c
+++ b/arch/sparc/kernel/traps_32.c
@@ -365,7 +365,7 @@ EXPORT_SYMBOL(do_BUG);
* up here so that timer interrupts work during initialization.
*/
-void trap_init(void)
+void __init trap_init(void)
{
extern void thread_info_offsets_are_bolixed_pete(void);
diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
index ad12f78bda7e..3198c4767387 100644
--- a/arch/um/kernel/trap.c
+++ b/arch/um/kernel/trap.c
@@ -311,7 +311,3 @@ void winch(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
{
do_IRQ(WINCH_IRQ, regs);
}
-
-void trap_init(void)
-{
-}
diff --git a/init/main.c b/init/main.c
index 53b278845b88..8d14674c4e00 100644
--- a/init/main.c
+++ b/init/main.c
@@ -779,6 +779,8 @@ void __init __weak poking_init(void) { }
void __init __weak pgtable_cache_init(void) { }
+void __init __weak trap_init(void) { }
+
bool initcall_debug;
core_param(initcall_debug, initcall_debug, bool, 0644);
--
2.31.0
^ permalink raw reply related
* Re: [PATCH] init: consolidate trap_init()
From: Jisheng Zhang @ 2021-04-14 9:38 UTC (permalink / raw)
To: Christophe Leroy
Cc: Anup Patel, James E.J. Bottomley, Paul Mackerras, linux-hexagon,
linux-riscv, Vincent Chen, Anton Ivanov, Jonas Bonn, Brian Cain,
Richard Weinberger, Helge Deller, Russell King, Ley Foon Tan,
linux-parisc, linux-snps-arc, uclinux-h8-devel, Albert Ou,
Jeff Dike, linux-um, Stefan Kristiansson, openrisc, Greentime Hu,
Paul Walmsley, Stafford Horne, linux-arm-kernel, Yoshinori Sato,
Nick Hu, Vineet Gupta, linux-kernel, Palmer Dabbelt, linuxppc-dev
In-Reply-To: <20210414172757.3ebfaa4c@xhacker.debian>
On Wed, 14 Apr 2021 17:27:57 +0800
Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> On Wed, 14 Apr 2021 11:10:42 +0200
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>
> >
> > Le 14/04/2021 à 10:58, Jisheng Zhang a écrit :
> > > Many architectures implement the trap_init() as NOP, since there is
> > > no such default for trap_init(), this empty stub is duplicated among
> > > these architectures. Provide a generic but weak NOP implementation
> > > to drop the empty stubs of trap_init() in these architectures.
> >
> > You define the weak function in the __init section.
> >
> > Most but not all architectures had it in __init section.
> >
> > And the remaining ones may not be defined in __init section. For instance look at the one in alpha
> > architecture.
> >
> > Have you checked that it is not a problem ? It would be good to say something about it in the commit
> > description.
>
> For those non-nop platforms, I can only test x86/arm64/, but both has
> __init mark. I'm not sure whether this is a problem for alpha etc. Maybe
> I can check which section the trap_init() sits. Or to avoid any possible
> regression, I can add __init mark to those remaining ones without it in
> preparation patches.
>
Hi,
I found only three platforms don't have the __init marker for trap_init(), I
will add the __init marker in three preparation patches in new version.
thanks
^ 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