[Bug 209181] kernel BUG at arch/powerpc/mm/pgtable.c:304!

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=209181

Christophe Leroy (christophe.le...@csgroup.eu) changed:

   What|Removed |Added

 CC||christophe.le...@csgroup.eu

--- Comment #1 from Christophe Leroy (christophe.le...@csgroup.eu) ---
See https://bugzilla.kernel.org/show_bug.cgi?id=209029

Patch at
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200902040122.136414-1-aneesh.ku...@linux.ibm.com/
to deactivate CONFIG_DEBUG_VM_PGTABLE on powerpc until the issue is fixes.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 209181] New: kernel BUG at arch/powerpc/mm/pgtable.c:304!

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=209181

Bug ID: 209181
   Summary: kernel BUG at arch/powerpc/mm/pgtable.c:304!
   Product: Memory Management
   Version: 2.5
Kernel Version: Linux 5.9-rc4
  Hardware: All
OS: Linux
  Tree: Mainline
Status: NEW
  Severity: normal
  Priority: P1
 Component: Page Allocator
  Assignee: a...@linux-foundation.org
  Reporter: zl...@redhat.com
CC: linuxppc-dev@lists.ozlabs.org
Regression: No

Description of problem:
The latest upstream mainline kernel always panic on ppc64le machine (P9) as
below:

[1.406462] Loading compiled-in X.509 certificates 
[1.436966] Loaded X.509 cert 'Build time autogenerated kernel key:
834a47793f474746e698c2f3a32aa53ffded35db' 
[1.437154] zswap: loaded using pool lzo/zbud 
[1.437509] debug_vm_pgtable: [debug_vm_pgtable ]: Validating
architecture page table helpers 
[1.437571] [ cut here ] 
[1.437584] WARNING: CPU: 0 PID: 1 at arch/powerpc/mm/pgtable.c:185
set_pte_at+0xd8/0x1c0 
[1.437589] Modules linked in: 
[1.437596] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc4 #1 
[1.437602] NIP:  c009bb28 LR: c152e1c0 CTR:
 
[1.437608] REGS: c001fb6eb7b0 TRAP: 0700   Not tainted  (5.9.0-rc4) 
[1.437613] MSR:  82029033   CR: 24002824 
XER: 000a 
[1.437624] CFAR: c009ba74 IRQMASK: 0  
[1.437624] GPR00: c152e1c0 c001fb6eba40 c2138900
c000120f4100  
[1.437624] GPR04: 000b70171815 c000122400a8 05014e010080
  
[1.437624] GPR08: 0080 07c0 05c0
0001  
[1.437624] GPR12: 2000 c405 
c1569d38  
[1.437624] GPR16: c0001221 f0ff c001fb52f8a0
c2231cb8  
[1.437624] GPR20: c10d8de0 c100 c0001220b2e8
c000122f8000  
[1.437624] GPR24: 0100 014e c000122f8028
8105  
[1.437624] GPR28: 000b70171815 c000122118c0 c000120f4100
c000122400a8  
[1.437668] NIP [c009bb28] set_pte_at+0xd8/0x1c0 
[1.437674] LR [c152e1c0] debug_vm_pgtable+0x8f4/0x1e14 
[1.437679] Call Trace: 
[1.437685] [c001fb6eba40] [c1082f48] _raw_spin_lock+0x88/0x100
(unreliable) 
[1.437693] [c001fb6eba80] [c152dfd4]
debug_vm_pgtable+0x708/0x1e14 
[1.437700] [c001fb6ebb90] [c001208c] do_one_initcall+0xbc/0x5f0 
[1.437707] [c001fb6ebc80] [c14e4d04]
kernel_init_freeable+0x4bc/0x58c 
[1.437714] [c001fb6ebdb0] [c0012de8] kernel_init+0x2c/0x164 
[1.437721] [c001fb6ebe20] [c000d5d0]
ret_from_kernel_thread+0x5c/0x6c 
[1.437726] Instruction dump: 
[1.437731] 41820068 e8010050 ebc10030 7c0803a6 4b8c 4b88 3d200700
792907c6  
[1.437741] 612900c0 7d4a4838 2faa00c0 419eff54 <0fe0> 4b4c 3fe0bfef
63ff  
[1.437751] irq event stamp: 275292 
[1.437757] hardirqs last  enabled at (275291): []
inc_zone_page_state+0xa0/0xd0 
[1.437764] hardirqs last disabled at (275292): []
program_check_common_virt+0x2bc/0x310 
[1.437771] softirqs last  enabled at (273036): []
inet6_register_protosw+0x154/0x2a0 
[1.437778] softirqs last disabled at (273034): []
inet6_register_protosw+0x44/0x2a0 
[1.437784] ---[ end trace 39aeb34808a575d2 ]--- 
[1.437790] [ cut here ] 
[1.437795] kernel BUG at arch/powerpc/mm/pgtable.c:304! 
[1.437801] Oops: Exception in kernel mode, sig: 5 [#1] 
[1.437805] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries 
[1.437807] Modules linked in: 
[1.437811] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW
5.9.0-rc4 #1 
[1.437815] NIP:  c009c1a8 LR: c05f9de0 CTR:
 
[1.437819] REGS: c001fb6eb720 TRAP: 0700   Tainted: GW 
(5.9.0-rc4) 
[1.437822] MSR:  82029033   CR: 24002828 
XER: 000a 
[1.437829] CFAR: c009c148 IRQMASK: 0  
[1.437829] GPR00: c05f9de0 c001fb6eb9b0 c2138900
c000120f4100  
[1.437829] GPR04: 000b70171815 c000122400a8 122f8000
00802f12  
[1.437829] GPR08:  0001 0028
0001  
[1.437829] GPR12: 2000 c405 
c1569d38  
[1.437829] GPR16: c0001221 f0ff c001fb52f8a0
c2231cb8  
[1.437829] GPR20: c10d8de0 c100 c0001220b2e8
c000122f8000  
[1.437829] GPR24: 0100 0008 c2231ca8
000a  
[1.437829] GPR28: c2231cb8 c2231cb0 000b70171815

Re: [RFC PATCH 12/12] powerpc/64s: power4 nap fixup in C

[Christophe Leroy]

Le 07/09/2020 à 06:02, Nicholas Piggin a écrit :

Excerpts from Christophe Leroy's message of September 6, 2020 5:32 pm:



Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :

There is no need for this to be in asm, use the new intrrupt entry wrapper.

Signed-off-by: Nicholas Piggin 
---
   arch/powerpc/include/asm/interrupt.h   | 14 
   arch/powerpc/include/asm/processor.h   |  1 +
   arch/powerpc/include/asm/thread_info.h |  6 
   arch/powerpc/kernel/exceptions-64s.S   | 45 --
   arch/powerpc/kernel/idle_book3s.S  |  4 +++
   5 files changed, 25 insertions(+), 45 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h 
b/arch/powerpc/include/asm/processor.h
index ed0d633ab5aa..3da1dba91386 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -424,6 +424,7 @@ extern unsigned long isa300_idle_stop_mayloss(unsigned long 
psscr_val);
   extern unsigned long isa206_idle_insn_mayloss(unsigned long type);
   #ifdef CONFIG_PPC_970_NAP
   extern void power4_idle_nap(void);
+extern void power4_idle_nap_return(void);


Please please please, 'extern' keyword is pointless and deprecated for
function prototypes. Don't add new ones.

Also, put it outside the #ifdef, so that you can use IS_ENABLED()
instead of #ifdef when using it.


I just copy paste and forget to remove it. I expect someone will do a
"cleanup" patch to get rid of them in one go, I find a random assortment
of extern and not extern to be even uglier :(


If we don't want to make fixes backporting a huge headache, some 
transition with random assortment is the price to pay.


One day, when 'extern' have become the minority, we can get rid of the 
few last ones.


But if someone believe it is not such a problem with backporting, I can 
provide a cleanup patch now.


Christophe

Re: [RFC PATCH 12/12] powerpc/64s: power4 nap fixup in C

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of September 6, 2020 5:32 pm:
> 
> 
> Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :
>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>> 
>> Signed-off-by: Nicholas Piggin 
>> ---
>>   arch/powerpc/include/asm/interrupt.h   | 14 
>>   arch/powerpc/include/asm/processor.h   |  1 +
>>   arch/powerpc/include/asm/thread_info.h |  6 
>>   arch/powerpc/kernel/exceptions-64s.S   | 45 --
>>   arch/powerpc/kernel/idle_book3s.S  |  4 +++
>>   5 files changed, 25 insertions(+), 45 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/interrupt.h 
>> b/arch/powerpc/include/asm/interrupt.h
>> index 3ae3d2f93b61..acfcc7d5779b 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -8,6 +8,16 @@
>>   #include 
>>   #include 
>>   
>> +static inline void nap_adjust_return(struct pt_regs *regs)
>> +{
>> +#ifdef CONFIG_PPC_970_NAP
> 
> Avoid #ifdef, you can use IS_ENABLED(CONFIG_PPC_970_NAP) in the 'if' below

Yeah I guess.

>> +if (test_thread_local_flags(_TLF_NAPPING)) {
>> +clear_thread_local_flags(_TLF_NAPPING);
>> +regs->nip = (unsigned long)power4_idle_nap_return;
>> +}
>> +#endif
>> +}
>> +
>>   #ifdef CONFIG_PPC_BOOK3S_64
>>   static inline void interrupt_enter_prepare(struct pt_regs *regs)
>>   {
>> @@ -33,6 +43,8 @@ static inline void interrupt_async_enter_prepare(struct 
>> pt_regs *regs)
>>  if (cpu_has_feature(CPU_FTR_CTRL) &&
>>  !test_thread_local_flags(_TLF_RUNLATCH))
>>  __ppc64_runlatch_on();
>> +
>> +nap_adjust_return(regs);
>>   }
>>   
>>   #else /* CONFIG_PPC_BOOK3S_64 */
>> @@ -72,6 +84,8 @@ static inline void interrupt_nmi_enter_prepare(struct 
>> pt_regs *regs, struct inte
>>   
>>  this_cpu_set_ftrace_enabled(0);
>>   
>> +nap_adjust_return(regs);
>> +
>>  nmi_enter();
>>   }
>>   
>> diff --git a/arch/powerpc/include/asm/processor.h 
>> b/arch/powerpc/include/asm/processor.h
>> index ed0d633ab5aa..3da1dba91386 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -424,6 +424,7 @@ extern unsigned long isa300_idle_stop_mayloss(unsigned 
>> long psscr_val);
>>   extern unsigned long isa206_idle_insn_mayloss(unsigned long type);
>>   #ifdef CONFIG_PPC_970_NAP
>>   extern void power4_idle_nap(void);
>> +extern void power4_idle_nap_return(void);
> 
> Please please please, 'extern' keyword is pointless and deprecated for 
> function prototypes. Don't add new ones.
> 
> Also, put it outside the #ifdef, so that you can use IS_ENABLED() 
> instead of #ifdef when using it.

I just copy paste and forget to remove it. I expect someone will do a 
"cleanup" patch to get rid of them in one go, I find a random assortment
of extern and not extern to be even uglier :(

>>   #endif
>>   
>>   extern unsigned long cpuidle_disable;
>> diff --git a/arch/powerpc/include/asm/thread_info.h 
>> b/arch/powerpc/include/asm/thread_info.h
>> index ca6c97025704..9b15f7edb0cb 100644
>> --- a/arch/powerpc/include/asm/thread_info.h
>> +++ b/arch/powerpc/include/asm/thread_info.h
>> @@ -156,6 +156,12 @@ void arch_setup_new_exec(void);
>>   
>>   #ifndef __ASSEMBLY__
>>   
>> +static inline void clear_thread_local_flags(unsigned int flags)
>> +{
>> +struct thread_info *ti = current_thread_info();
>> +ti->local_flags &= ~flags;
>> +}
>> +
>>   static inline bool test_thread_local_flags(unsigned int flags)
>>   {
>>  struct thread_info *ti = current_thread_info();
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S 
>> b/arch/powerpc/kernel/exceptions-64s.S
>> index 227bad3a586d..1db6b3438c88 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -692,25 +692,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
>>  ld  r1,GPR1(r1)
>>   .endm
>>   
>> -/*
>> - * When the idle code in power4_idle puts the CPU into NAP mode,
>> - * it has to do so in a loop, and relies on the external interrupt
>> - * and decrementer interrupt entry code to get it out of the loop.
>> - * It sets the _TLF_NAPPING bit in current_thread_info()->local_flags
>> - * to signal that it is in the loop and needs help to get out.
>> - */
>> -#ifdef CONFIG_PPC_970_NAP
>> -#define FINISH_NAP  \
>> -BEGIN_FTR_SECTION   \
>> -ld  r11, PACA_THREAD_INFO(r13); \
>> -ld  r9,TI_LOCAL_FLAGS(r11); \
>> -andi.   r10,r9,_TLF_NAPPING;\
>> -bnelpower4_fixup_nap;   \
>> -END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
>> -#else
>> -#define FINISH_NAP
>> -#endif
>> -
>>   /*
>>* There are a few constraints to be concerned with.
>>* - Real mode exceptions code/data must be located at their physical 
>> location.
>> @@ -1250,7 +1231,6 @@ EXC_COMMON_BEGIN(machine_check_common)
>>   */
>>  GEN_COMMON machine_check
>>   
>> -

Re: [PATCH -next] powerpc/book3s64: fix link error with CONFIG_PPC_RADIX_MMU=n

[Yang Yingliang]

On 2020/9/6 14:50, Christophe Leroy wrote:



Le 05/09/2020 à 13:25, Yang Yingliang a écrit :

Fix link error when CONFIG_PPC_RADIX_MMU is disabled:
powerpc64-linux-gnu-ld: 
arch/powerpc/platforms/pseries/lpar.o:(.toc+0x0): undefined reference 
to `mmu_pid_bits'


Reported-by: Hulk Robot 
Signed-off-by: Yang Yingliang 
---
  arch/powerpc/mm/book3s64/mmu_context.c | 4 


In your commit log, you are just mentionning 
arch/powerpc/platforms/pseries/lpar.o, which is right.


You shouldn't need to modify arch/powerpc/mm/book3s64/mmu_context.c at 
all, see below.



  arch/powerpc/platforms/pseries/lpar.c  | 2 ++
  2 files changed, 6 insertions(+)

diff --git a/arch/powerpc/mm/book3s64/mmu_context.c 
b/arch/powerpc/mm/book3s64/mmu_context.c

index 0ba30b8b935b..a8e292cd88f0 100644
--- a/arch/powerpc/mm/book3s64/mmu_context.c
+++ b/arch/powerpc/mm/book3s64/mmu_context.c
@@ -152,6 +152,7 @@ void hash__setup_new_exec(void)
    static int radix__init_new_context(struct mm_struct *mm)
  {
+#ifdef CONFIG_PPC_RADIX_MMU


This shouldn't be required. radix__init_new_context() is only called 
when radix_enabled() returns true.
As it is a static function, when it is not called it gets optimised 
away, so you will never get an undefined reference to `mmu_pid_bits` 
there.
powerpc64-linux-gnu-ld: 
arch/powerpc/mm/book3s64/mmu_context.o:(.toc+0x0): undefined reference 
to `mmu_pid_bits'
powerpc64-linux-gnu-ld: 
arch/powerpc/mm/book3s64/mmu_context.o:(.toc+0x8): undefined reference 
to `mmu_base_pid'



mmu_context.c is always compiled, it uses mmu_pid_bits and mmu_base_pid.




  unsigned long rts_field;
  int index, max_id;
  @@ -177,6 +178,9 @@ static int radix__init_new_context(struct 
mm_struct *mm)

  mm->context.hash_context = NULL;
    return index;
+#else
+    return -ENOTSUPP;
+#endif
  }
    int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
diff --git a/arch/powerpc/platforms/pseries/lpar.c 
b/arch/powerpc/platforms/pseries/lpar.c

index baf24eacd268..e454e218dbba 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -1726,10 +1726,12 @@ void __init hpte_init_pseries(void)
    void radix_init_pseries(void)
  {
+#ifdef CONFIG_PPC_RADIX_MMU


This function is only called from 
/arch/powerpc/mm/book3s64/radix_pgtable.c which is only built when 
CONFIG_PPC_RADIX_MMU is selected.


So the entire function should be encloded in the #ifdef.

OK, I will send a v2 later.



  pr_info("Using radix MMU under hypervisor\n");
    pseries_lpar_register_process_table(__pa(process_tb),
  0, PRTB_SIZE_SHIFT - 12);
+#endif
  }
    #ifdef CONFIG_PPC_SMLPAR



Christophe
.

Re: [PATCH v3 4/6] powerpc: Introduce temporary mm

[Christopher M. Riedl]

On Thu Aug 27, 2020 at 11:15 AM CDT, Jann Horn wrote:
> On Thu, Aug 27, 2020 at 7:24 AM Christopher M. Riedl 
> wrote:
> > x86 supports the notion of a temporary mm which restricts access to
> > temporary PTEs to a single CPU. A temporary mm is useful for situations
> > where a CPU needs to perform sensitive operations (such as patching a
> > STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
> > said mappings to other CPUs. A side benefit is that other CPU TLBs do
> > not need to be flushed when the temporary mm is torn down.
> >
> > Mappings in the temporary mm can be set in the userspace portion of the
> > address-space.
> [...]
> > diff --git a/arch/powerpc/lib/code-patching.c 
> > b/arch/powerpc/lib/code-patching.c
> [...]
> > @@ -44,6 +45,70 @@ int raw_patch_instruction(struct ppc_inst *addr, struct 
> > ppc_inst instr)
> >  }
> >
> >  #ifdef CONFIG_STRICT_KERNEL_RWX
> > +
> > +struct temp_mm {
> > +   struct mm_struct *temp;
> > +   struct mm_struct *prev;
> > +   bool is_kernel_thread;
> > +   struct arch_hw_breakpoint brk[HBP_NUM_MAX];
> > +};
> > +
> > +static inline void init_temp_mm(struct temp_mm *temp_mm, struct mm_struct 
> > *mm)
> > +{
> > +   temp_mm->temp = mm;
> > +   temp_mm->prev = NULL;
> > +   temp_mm->is_kernel_thread = false;
> > +   memset(_mm->brk, 0, sizeof(temp_mm->brk));
> > +}
> > +
> > +static inline void use_temporary_mm(struct temp_mm *temp_mm)
> > +{
> > +   lockdep_assert_irqs_disabled();
> > +
> > +   temp_mm->is_kernel_thread = current->mm == NULL;
>
> (That's a somewhat misleading variable name - kernel threads can have
> a non-NULL ->mm, too.)
>

Oh I didn't know that, in that case yes this is not a good name. I am
considering some changes (based on your comments about current->mm
below) which would make this variable superfluous.

> > +   if (temp_mm->is_kernel_thread)
> > +   temp_mm->prev = current->active_mm;
> > +   else
> > +   temp_mm->prev = current->mm;
>
> Why the branch? Shouldn't current->active_mm work in both cases?
>
>

Yes you are correct.

> > +   /*
> > +* Hash requires a non-NULL current->mm to allocate a userspace 
> > address
> > +* when handling a page fault. Does not appear to hurt in Radix 
> > either.
> > +*/
> > +   current->mm = temp_mm->temp;
>
> This looks dangerous to me. There are various places that attempt to
> find all userspace tasks that use a given mm by iterating through all
> tasks on the system and comparing each task's ->mm pointer to
> current's. Things like current_is_single_threaded() as part of various
> security checks, mm_update_next_owner(), zap_threads(), and so on. So
> if this is reachable from userspace task context (which I think it
> is?), I don't think we're allowed to switch out the ->mm pointer here.
>
>

Thanks for pointing this out! I took a step back and looked at this
again in more detail. The only reason for reassigning the ->mm pointer
is that when patching we need to hash the page and allocate an SLB 
entry w/ the hash MMU. That codepath includes a check to ensure that
->mm is not NULL. Overwriting ->mm temporarily and restoring it is
pretty crappy in retrospect. I _think_ a better approach is to just call
the hashing and allocate SLB functions from `map_patch` directly - this
both removes the need to overwrite ->mm (since the functions take an mm
parameter) and it avoids taking two exceptions when doing the actual
patching.

This works fine on Power9 and a Power8 at least but needs some testing
on PPC32 before I can send a v4.

> > +   switch_mm_irqs_off(NULL, temp_mm->temp, current);
>
> switch_mm_irqs_off() calls switch_mmu_context(), which in the nohash
> implementation increments next->context.active and decrements
> prev->context.active if prev is non-NULL, right? So this would
> increase temp_mm->temp->context.active...
>
> > +   if (ppc_breakpoint_available()) {
> > +   struct arch_hw_breakpoint null_brk = {0};
> > +   int i = 0;
> > +
> > +   for (; i < nr_wp_slots(); ++i) {
> > +   __get_breakpoint(i, _mm->brk[i]);
> > +   if (temp_mm->brk[i].type != 0)
> > +   __set_breakpoint(i, _brk);
> > +   }
> > +   }
> > +}
> > +
> > +static inline void unuse_temporary_mm(struct temp_mm *temp_mm)
> > +{
> > +   lockdep_assert_irqs_disabled();
> > +
> > +   if (temp_mm->is_kernel_thread)
> > +   current->mm = NULL;
> > +   else
> > +   current->mm = temp_mm->prev;
> > +   switch_mm_irqs_off(NULL, temp_mm->prev, current);
>
> ... whereas this would increase temp_mm->prev->context.active. As far
> as I can tell, that'll mean that both the original mm and the patching
> mm will have their .active counts permanently too high after
> use_temporary_mm()+unuse_temporary_mm()?
>

Yes you are correct. Hmm, I can't 

Re: fsl_espi errors on v5.7.15

[Chris Packham]

On 5/09/20 5:23 am, Heiner Kallweit wrote:


On Fri 4. Sep 2020 at 01:58, Chris Packham 
mailto:chris.pack...@alliedtelesis.co.nz>> 
wrote:


On 1/09/20 6:14 pm, Nicholas Piggin wrote:

> Excerpts from Chris Packham's message of September 1, 2020 11:25 am:

>> On 1/09/20 12:33 am, Heiner Kallweit wrote:

>>> On 30.08.2020 23:59, Chris Packham wrote:

 On 31/08/20 9:41 am, Heiner Kallweit wrote:

> On 30.08.2020 23:00, Chris Packham wrote:

>> On 31/08/20 12:30 am, Nicholas Piggin wrote:

>>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:

>> 

>>

 I've also now seen the RX FIFO not empty error on the T2080RDB



 fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!

 fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!

 fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!

 fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!

 fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't 
 empty!

 fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32



 With my current workaround of emptying the RX FIFO. It seems

 survivable. Interestingly it only ever seems to be 1 extra byte in 
 the

 RX FIFO and it seems to be after either a READ_SR or a READ_FSR.



 fsl_espi ffe11.spi: tx 70

 fsl_espi ffe11.spi: rx 03

 fsl_espi ffe11.spi: Extra RX 00

 fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!

 fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't 
 empty!

 fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32

 fsl_espi ffe11.spi: tx 05

 fsl_espi ffe11.spi: rx 00

 fsl_espi ffe11.spi: Extra RX 03

 fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!

 fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't 
 empty!

 fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32

 fsl_espi ffe11.spi: tx 05

 fsl_espi ffe11.spi: rx 00

 fsl_espi ffe11.spi: Extra RX 03



  From all the Micron SPI-NOR datasheets I've got access to it 
 is

 possible to continually read the SR/FSR. But I've no idea why it

 happens some times and not others.

>>> So I think I've got a reproduction and I think I've bisected the 
>>> problem

>>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt 
>>> replay in

>>> C"). My day is just finishing now so I haven't applied too much 
>>> scrutiny

>>> to this result. Given the various rabbit holes I've been down on 
>>> this

>>> issue already I'd take this information with a good degree of 
>>> skepticism.

>>>

>> OK, so an easy test should be to re-test with a 5.4 kernel.

>> It doesn't have yet the change you're referring to, and the fsl-espi 
>> driver

>> is basically the same as in 5.7 (just two small changes in 5.7).

> There's 6cc0c16d82f88 and maybe also other interrupt related patches

> around this time that could affect book E, so it's good if that exact

> patch is confirmed.

 My confirmation is basically that I can induce the issue in a 5.4 
 kernel

 by cherry-picking 3282a3da25bd. I'm also able to "fix" the issue in

 5.9-rc2 by reverting that one commit.



 I both cases it's not exactly a clean cherry-pick/revert so I also

 confirmed the bisection result by building at 3282a3da25bd (which sees

 the issue) and the commit just before (which does not).

>>> Thanks for testing, that confirms it well.

>>>

>>> [snip patch]

>>>

 I still saw the issue with this change applied. PPC_IRQ_SOFT_MASK_DEBUG

 didn't report anything (either with or without the change above).

>>> Okay, it was a bit of a shot in the dark. I still can't see what

>>> else has changed.

>>>

>>> What would cause this, a lost interrupt? A spurious interrupt? Or

>>> higher interrupt latency?

>>>

>>> I don't think the patch should cause significantly worse latency,

>>> (it's supposed to be a bit better if anything because it doesn't set

>>> up the full interrupt frame). But it's possible.

>> My working theory is that the SPI_DON indication is all about the TX

>> direction an now that the interrupts are faster we're hitting an error

>> because there is still RX activity going on. Heiner disagrees with my

>> interpretation of the SPI_DON indication and the fact that it doesn't

>> happen 

Re: fsl_espi errors on v5.7.15

[Chris Packham]

(resend as something decided to insert html, some context stripped)

On 5/09/20 5:23 am, Heiner Kallweit wrote:
> On Fri 4. Sep 2020 at 01:58, Chris Packham 
>  > wrote:
>
>

>
>
> I tried ftrace but I really wasn't sure what I was looking for.
>
> Capturing a "bad" case was pretty tricky. But I think I've
> identified a
>
> fix (I'll send it as a proper patch shortly). The gist is
>
>
>
> diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
>
> index 7e7c92cafdbb..cb120b68c0e2 100644
>
> --- a/drivers/spi/spi-fsl-espi.c
>
> +++ b/drivers/spi/spi-fsl-espi.c
>
> @@ -574,13 +574,14 @@ static void fsl_espi_cpu_irq(struct fsl_espi
>
> *espi, u32 events)
>
>   static irqreturn_t fsl_espi_irq(s32 irq, void *context_data)
>
>   {
>
>  struct fsl_espi *espi = context_data;
>
> -   u32 events;
>
> +   u32 events, mask;
>
>
>
>  spin_lock(>lock);
>
>
>
>  /* Get interrupt events(tx/rx) */
>
>  events = fsl_espi_read_reg(espi, ESPI_SPIE);
>
> -   if (!events) {
>
> +   mask = fsl_espi_read_reg(espi, ESPI_SPIM);
>
> +   if (!(events & mask)) {
>
>  spin_unlock(>lock);
>
>  return IRQ_NONE;
>
>  }
>
>
>
> The SPIE register contains the TXCNT so events is pretty much always
>
> going to have something set. By checking events against what we've
>
> actually requested interrupts for we don't see any spurious events.
>
>
> Usually we shouldn’t receive interrupts we’re not interested in, 
> except the interrupt is shared.
My theory is that we get an interrupt to the core for RXT and another 
for DON. With the changes to the powerpc interrupt handling and the fact 
that fsl_espi_cpu_irq() doesn't actually look at the specific event bits 
means that sometimes both events are handled in the processing of the 
first interrupt and the second one trips the SPI_DON not set error.

There's an old comment "SPI bus sometimes got lost interrupts..." which 
makes me wonder if the interrupt handling change has fixed this original 
problem.

I still think the change makes sense regardless because the SPIE 
register has some counter fields in it.

> This leads to the question: is the SPI interrupt shared with another 
> device on your system?
It's not shared on either the custom board or the T2080RDB.
> Do you see spurious interrupts with the patch under 
> /proc/irq/(irq)/spurious?

Yes it looks like it

[root@linuxbox ~]# cat /proc/irq/53/spurious
count 3126
unhandled 0
last_unhandled 0 ms

[root@linuxbox ~]# /flash/dd_test.sh

[root@linuxbox ~]# cat /proc/irq/53/spurious
count 1016
unhandled 0
last_unhandled 4294746100 ms

[root@linuxbox ~]# /flash/dd_test.sh

[root@linuxbox ~]# cat /proc/irq/53/spurious
count 88391
unhandled 0
last_unhandled 4294746100 ms

[root@linuxbox ~]# /flash/dd_test.sh

[root@linuxbox ~]# cat /proc/irq/53/spurious
count 72459
unhandled 2
last_unhandled 4294758632 ms
>
>
>
> I've tested this on the T2080RDB and on our custom hardware and it
> seems
>
> to resolve the problem.
>
>
>

Re: [RFC PATCH 12/12] powerpc/64s: power4 nap fixup in C

[Christophe Leroy]

Le 05/09/2020 à 19:43, Nicholas Piggin a écrit :

There is no need for this to be in asm, use the new intrrupt entry wrapper.

Signed-off-by: Nicholas Piggin 
---
  arch/powerpc/include/asm/interrupt.h   | 14 
  arch/powerpc/include/asm/processor.h   |  1 +
  arch/powerpc/include/asm/thread_info.h |  6 
  arch/powerpc/kernel/exceptions-64s.S   | 45 --
  arch/powerpc/kernel/idle_book3s.S  |  4 +++
  5 files changed, 25 insertions(+), 45 deletions(-)

diff --git a/arch/powerpc/include/asm/interrupt.h 
b/arch/powerpc/include/asm/interrupt.h
index 3ae3d2f93b61..acfcc7d5779b 100644
--- a/arch/powerpc/include/asm/interrupt.h
+++ b/arch/powerpc/include/asm/interrupt.h
@@ -8,6 +8,16 @@
  #include 
  #include 
  
+static inline void nap_adjust_return(struct pt_regs *regs)

+{
+#ifdef CONFIG_PPC_970_NAP


Avoid #ifdef, you can use IS_ENABLED(CONFIG_PPC_970_NAP) in the 'if' below


+   if (test_thread_local_flags(_TLF_NAPPING)) {
+   clear_thread_local_flags(_TLF_NAPPING);
+   regs->nip = (unsigned long)power4_idle_nap_return;
+   }
+#endif
+}
+
  #ifdef CONFIG_PPC_BOOK3S_64
  static inline void interrupt_enter_prepare(struct pt_regs *regs)
  {
@@ -33,6 +43,8 @@ static inline void interrupt_async_enter_prepare(struct 
pt_regs *regs)
if (cpu_has_feature(CPU_FTR_CTRL) &&
!test_thread_local_flags(_TLF_RUNLATCH))
__ppc64_runlatch_on();
+
+   nap_adjust_return(regs);
  }
  
  #else /* CONFIG_PPC_BOOK3S_64 */

@@ -72,6 +84,8 @@ static inline void interrupt_nmi_enter_prepare(struct pt_regs 
*regs, struct inte
  
  	this_cpu_set_ftrace_enabled(0);
  
+	nap_adjust_return(regs);

+
nmi_enter();
  }
  
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h

index ed0d633ab5aa..3da1dba91386 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -424,6 +424,7 @@ extern unsigned long isa300_idle_stop_mayloss(unsigned long 
psscr_val);
  extern unsigned long isa206_idle_insn_mayloss(unsigned long type);
  #ifdef CONFIG_PPC_970_NAP
  extern void power4_idle_nap(void);
+extern void power4_idle_nap_return(void);


Please please please, 'extern' keyword is pointless and deprecated for 
function prototypes. Don't add new ones.


Also, put it outside the #ifdef, so that you can use IS_ENABLED() 
instead of #ifdef when using it.



  #endif
  
  extern unsigned long cpuidle_disable;

diff --git a/arch/powerpc/include/asm/thread_info.h 
b/arch/powerpc/include/asm/thread_info.h
index ca6c97025704..9b15f7edb0cb 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -156,6 +156,12 @@ void arch_setup_new_exec(void);
  
  #ifndef __ASSEMBLY__
  
+static inline void clear_thread_local_flags(unsigned int flags)

+{
+   struct thread_info *ti = current_thread_info();
+   ti->local_flags &= ~flags;
+}
+
  static inline bool test_thread_local_flags(unsigned int flags)
  {
struct thread_info *ti = current_thread_info();
diff --git a/arch/powerpc/kernel/exceptions-64s.S 
b/arch/powerpc/kernel/exceptions-64s.S
index 227bad3a586d..1db6b3438c88 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -692,25 +692,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
ld  r1,GPR1(r1)
  .endm
  
-/*

- * When the idle code in power4_idle puts the CPU into NAP mode,
- * it has to do so in a loop, and relies on the external interrupt
- * and decrementer interrupt entry code to get it out of the loop.
- * It sets the _TLF_NAPPING bit in current_thread_info()->local_flags
- * to signal that it is in the loop and needs help to get out.
- */
-#ifdef CONFIG_PPC_970_NAP
-#define FINISH_NAP \
-BEGIN_FTR_SECTION  \
-   ld  r11, PACA_THREAD_INFO(r13); \
-   ld  r9,TI_LOCAL_FLAGS(r11); \
-   andi.   r10,r9,_TLF_NAPPING;\
-   bnelpower4_fixup_nap;   \
-END_FTR_SECTION_IFSET(CPU_FTR_CAN_NAP)
-#else
-#define FINISH_NAP
-#endif
-
  /*
   * There are a few constraints to be concerned with.
   * - Real mode exceptions code/data must be located at their physical 
location.
@@ -1250,7 +1231,6 @@ EXC_COMMON_BEGIN(machine_check_common)
 */
GEN_COMMON machine_check
  
-	FINISH_NAP

/* Enable MSR_RI when finished with PACA_EXMC */
li  r10,MSR_RI
mtmsrd  r10,1
@@ -1572,7 +1552,6 @@ EXC_VIRT_BEGIN(hardware_interrupt, 0x4500, 0x100)
  EXC_VIRT_END(hardware_interrupt, 0x4500, 0x100)
  EXC_COMMON_BEGIN(hardware_interrupt_common)
GEN_COMMON hardware_interrupt
-   FINISH_NAP
addir3,r1,STACK_FRAME_OVERHEAD
bl  do_IRQ
b   interrupt_return
@@ -1757,7 +1736,6 @@ EXC_VIRT_BEGIN(decrementer, 0x4900, 0x80)
  EXC_VIRT_END(decrementer, 0x4900, 0x80)
  EXC_COMMON_BEGIN(decrementer_common)

Re: [PATCH -next] powerpc/eeh: fix compile warning with CONFIG_PROC_FS=n

[Christophe Leroy]

Le 05/09/2020 à 13:17, Yang Yingliang a écrit :

Fix the compile warning:

arch/powerpc/kernel/eeh.c:1639:12: error: 'proc_eeh_show' defined but not used 
[-Werror=unused-function]
  static int proc_eeh_show(struct seq_file *m, void *v)

Reported-by: Hulk Robot 
Signed-off-by: Yang Yingliang 
---
  arch/powerpc/kernel/eeh.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 94682382fc8c..420c3c25c6e7 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1636,6 +1636,7 @@ int eeh_pe_inject_err(struct eeh_pe *pe, int type, int 
func,
  }
  EXPORT_SYMBOL_GPL(eeh_pe_inject_err);
  
+#ifdef CONFIG_PROC_FS


I don't like this way of fixing the issue, because you'll get an 
unbalanced source code: proc_eeh_show() is apparently referenced all the 
time in eeh_init_proc(), but because proc_create_single() is a NULL 
macro when CONFIG_PROC_FS is not selected, you get the 'unused function' 
error.


I think the right fix should be to rewrite proc_create_single() as a 
static inline function that calls proc_create_single_data() when 
CONFIG_PROC_FS is selected and just returns NULL otherwise.



  static int proc_eeh_show(struct seq_file *m, void *v)
  {
if (!eeh_enabled()) {
@@ -1662,6 +1663,7 @@ static int proc_eeh_show(struct seq_file *m, void *v)
  
  	return 0;

  }
+#endif
  
  #ifdef CONFIG_DEBUG_FS

  static int eeh_enable_dbgfs_set(void *data, u64 val)



Christophe

Re: [PATCH -next] powerpc/book3s64: fix link error with CONFIG_PPC_RADIX_MMU=n

[Christophe Leroy]

Le 05/09/2020 à 13:25, Yang Yingliang a écrit :

Fix link error when CONFIG_PPC_RADIX_MMU is disabled:
powerpc64-linux-gnu-ld: arch/powerpc/platforms/pseries/lpar.o:(.toc+0x0): 
undefined reference to `mmu_pid_bits'

Reported-by: Hulk Robot 
Signed-off-by: Yang Yingliang 
---
  arch/powerpc/mm/book3s64/mmu_context.c | 4 


In your commit log, you are just mentionning 
arch/powerpc/platforms/pseries/lpar.o, which is right.


You shouldn't need to modify arch/powerpc/mm/book3s64/mmu_context.c at 
all, see below.



  arch/powerpc/platforms/pseries/lpar.c  | 2 ++
  2 files changed, 6 insertions(+)

diff --git a/arch/powerpc/mm/book3s64/mmu_context.c 
b/arch/powerpc/mm/book3s64/mmu_context.c
index 0ba30b8b935b..a8e292cd88f0 100644
--- a/arch/powerpc/mm/book3s64/mmu_context.c
+++ b/arch/powerpc/mm/book3s64/mmu_context.c
@@ -152,6 +152,7 @@ void hash__setup_new_exec(void)
  
  static int radix__init_new_context(struct mm_struct *mm)

  {
+#ifdef CONFIG_PPC_RADIX_MMU


This shouldn't be required. radix__init_new_context() is only called 
when radix_enabled() returns true.
As it is a static function, when it is not called it gets optimised 
away, so you will never get an undefined reference to `mmu_pid_bits` there.



unsigned long rts_field;
int index, max_id;
  
@@ -177,6 +178,9 @@ static int radix__init_new_context(struct mm_struct *mm)

mm->context.hash_context = NULL;
  
  	return index;

+#else
+   return -ENOTSUPP;
+#endif
  }
  
  int init_new_context(struct task_struct *tsk, struct mm_struct *mm)

diff --git a/arch/powerpc/platforms/pseries/lpar.c 
b/arch/powerpc/platforms/pseries/lpar.c
index baf24eacd268..e454e218dbba 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -1726,10 +1726,12 @@ void __init hpte_init_pseries(void)
  
  void radix_init_pseries(void)

  {
+#ifdef CONFIG_PPC_RADIX_MMU


This function is only called from 
/arch/powerpc/mm/book3s64/radix_pgtable.c which is only built when 
CONFIG_PPC_RADIX_MMU is selected.


So the entire function should be encloded in the #ifdef.


pr_info("Using radix MMU under hypervisor\n");
  
  	pseries_lpar_register_process_table(__pa(process_tb),

0, PRTB_SIZE_SHIFT - 12);
+#endif
  }
  
  #ifdef CONFIG_PPC_SMLPAR




Christophe