Re: [PATCH RFC 1/2] m68k: Add tracirqs

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 23 Oct 2024 10:59:42 +0200
Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org> wrote:

> 
> Sadly, not that straightforward.
> I have this patch right now:
> diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
> index b2a3093af677..fc4a2d124514 100644
> --- a/arch/m68k/Kconfig
> +++ b/arch/m68k/Kconfig
> @@ -41,6 +41,7 @@ config M68K
>          select ZONE_DMA
>          select ARCH_SUPPORTS_RT
>          select IRQ_FORCED_THREADING
> +       select TRACE_IRQFLAGS_SUPPORT
> 
>   config CPU_BIG_ENDIAN
>          def_bool y
> diff --git a/arch/m68k/coldfire/entry.S b/arch/m68k/coldfire/entry.S
> index 4ea08336e2fb..fbdc4404f29e 100644
> --- a/arch/m68k/coldfire/entry.S
> +++ b/arch/m68k/coldfire/entry.S
> @@ -57,6 +57,9 @@ enosys:
>   ENTRY(system_call)
>          SAVE_ALL_SYS
>          move    #0x2000,%sr             /* enable intrs again */
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +       jbsr    trace_hardirqs_on
> +#endif

Note, the trace_hardirqs_on/off() needs to be done when interrupts are
disabled. That is:

	__local_irq_disable();
	trace_hardirqs_off();

	[..]

	trace_hardirqs_on();
	__local_irq_enable();

I don't know the m68k assembly, but from the comments it looks like you
are calling these with interrupts enabled.

-- Steve


>          GET_CURRENT(%d2)
> 
>          cmpl    #NR_syscalls,%d0
> @@ -99,6 +102,9 @@ ENTRY(system_call)
>          addql   #4,%sp
> 
>   ret_from_exception:
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +       jbsr    trace_hardirqs_off
> +#endif
>          move    #0x2700,%sr             /* disable intrs */
>          btst    #5,%sp@(PT_OFF_SR)      /* check if returning to kernel */
>          jeq     Luser_return            /* if so, skip resched, signals */
> @@ -140,6 +146,9 @@ Lreturn:
>   Lwork_to_do:
>          movel   %a0@(TINFO_FLAGS),%d1   /* get thread_info->flags */
>          move    #0x2000,%sr             /* enable intrs again */
> +#ifdef CONFIG_TRACE_IRQFLAGS
> +       jbsr    trace_hardirqs_on
> +#endif
>          btst    #TIF_NEED_RESCHED,%d1
>          jne     reschedule
> 
> But it fails when init is called:
> [    5.313000] Run /bin/bash as init process
> [    5.314000]   with arguments:
> [    5.315000]     /bin/bash
> [    5.316000]   with environment:
> [    5.317000]     HOME=/
> [    5.318000]     TERM=linux
> [    5.684000] Kernel panic - not syncing: Attempted to kill init! 
> exitcode=0x0000000b
> [    5.684000] CPU: 0 UID: 0 PID: 1 Comm: bash Not tainted 
> 6.12.0-rc4-00049-g4393ca34ead3 #364
> [    5.684000] Stack from 41a03e18:
> [    5.684000]         41a03e18 41540187 41540187 0000000a ffffffff 
> 415a8fb4 4140dd90 41416588
> [    5.684000]         41540187 4140d5de 4102ba84 4100b19c 4100b1a8 
> 00000000 41a38000 4102ba3a
> [    5.684000]         41a03ea0 4100c34a 4150e87e 0000000b 0000000b 
> 41a03f80 0000000a 41a3c0d4
> [    5.684000]         41a02000 400004d8 41a08000 4102ba3a 4102ba84 
> 00000000 00000000 000000ff
> [    5.684000]         00000000 00000000 41a03ef4 4100cb82 0000000b 
> 0000000b 41a03f80 41a03f6c
> [    5.684000]         41016cd2 0000000b 41a03f6c 0000001d 00000026 
> 0000048c ffffffff 00000006
> [    5.684000] Call Trace: [<4140dd90>] _printk+0x0/0x18
> [    5.684000]  [<41416588>] dump_stack+0xc/0x10
> [    5.684000]  [<4140d5de>] panic+0xf2/0x2d4
> [    5.684000]  [<4102ba84>] preempt_count_sub+0x0/0x2e
> [    5.684000]  [<4100b19c>] arch_local_irq_enable+0x0/0xc
> [    5.684000]  [<4100b1a8>] arch_irqs_disabled+0x0/0x10
> [    5.684000]  [<4102ba3a>] preempt_count_add+0x0/0x1e
> [    5.684000]  [<4100c34a>] do_exit+0x266/0x930
> [    5.684000]  [<4102ba3a>] preempt_count_add+0x0/0x1e
> [    5.684000]  [<4102ba84>] preempt_count_sub+0x0/0x2e
> [    5.684000]  [<4100cb82>] do_group_exit+0x26/0xba
> [    5.684000]  [<41016cd2>] get_signal+0x60e/0x76c
> [    5.684000]  [<410044e8>] test_ti_thread_flag+0x0/0x14
> [    5.684000]  [<4102ba94>] preempt_count_sub+0x10/0x2e
> [    5.684000]  [<41004b68>] do_notify_resume+0x3a/0x4c6
> [    5.684000]  [<41015d52>] force_sig_fault_to_task+0x32/0x3e
> [    5.684000]  [<41015d72>] force_sig_fault+0x14/0x1a
> [    5.684000]  [<41005606>] buserr_c+0x9a/0x188
> [    5.684000]  [<410065fc>] Lsignal_return+0x14/0x24
> [    5.684000]  [<410065de>] Lwork_to_do+0xe/0x18
> [    5.684000]
> [    5.684000] ---[ end Kernel panic - not syncing: Attempted to kill 
> init! exitcode=0x0000000b ]---
> 
> 
> JM
> 
> >>  
> >>>
> >>> Registers %d0-%d5 and %a0-%a2 are saved on the stack at this point and
> >>> can be clobbered if need be.  
> >>
> >> I don't know if they need to be clobbered...  
> > 
> > I meant to say that if you need registers to prepare function arguments 
> > for trace_irqs_on/off() on the stack, these can be used. But that may 
> > not be necessary in this case.
> > 
> > Cheers,
> > 
> >      Michael
> > 
> >   
> >>
> >> Thanks,
> >> JM
> >>  
> >>>
> >>> Cheers,
> >>>
> >>>      Michael
> >>>
> >>>  
> >>>>
> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>
> >>>> ---
> >>>>  arch/m68k/Kconfig      | 1 +
> >>>>  arch/m68k/kernel/irq.c | 2 ++
> >>>>  2 files changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
> >>>> index
> >>>> cc26df907bfe3c8143a931d259eceabb16af7411..ab3375475721fa63418c40d4ba6ac76679ebc77d
> >>>> 100644
> >>>> --- a/arch/m68k/Kconfig
> >>>> +++ b/arch/m68k/Kconfig
> >>>> @@ -39,6 +39,7 @@ config M68K
> >>>>      select OLD_SIGSUSPEND3
> >>>>      select UACCESS_MEMCPY if !MMU
> >>>>      select ZONE_DMA
> >>>> +    select TRACE_IRQFLAGS_SUPPORT
> >>>>
> >>>>  config CPU_BIG_ENDIAN
> >>>>      def_bool y
> >>>> diff --git a/arch/m68k/kernel/irq.c b/arch/m68k/kernel/irq.c
> >>>> index
> >>>> 9ab4f550342e5de11c528f55781432675ffd66bf..74cf60ebbc4bca51f3caa4046dbd2bdb02355711
> >>>> 100644
> >>>> --- a/arch/m68k/kernel/irq.c
> >>>> +++ b/arch/m68k/kernel/irq.c
> >>>> @@ -21,9 +21,11 @@ asmlinkage void do_IRQ(int irq, struct pt_regs 
> >>>> *regs)
> >>>>  {
> >>>>      struct pt_regs *oldregs = set_irq_regs(regs);
> >>>>
> >>>> +    trace_hardirqs_off();
> >>>>      irq_enter();
> >>>>      generic_handle_irq(irq);
> >>>>      irq_exit();
> >>>> +    trace_hardirqs_on();
> >>>>
> >>>>      set_irq_regs(oldregs);
> >>>>  }
> >>>>  
> >>