On Mon, 2010-01-18 at 23:49 -0500, Mike Frysinger wrote:
> On Mon, Jan 18, 2010 at 23:35, <[email protected]> wrote:
> > Modified: trunk/arch/blackfin/include/asm/context.S (8178 => 8179)
> >
> > @@ -73,6 +75,7 @@
> > #else
> > cli r0;
> > #endif
> > + TRACE_IRQS_OFF
>
> this call site doesnt need to save any registers before calling a C
> function as all registers have been saved onto the stack
>
Yes. I agree.
> > @@ -289,6 +292,7 @@
> > p0.l = _bfin_irq_flags;
> > r0 = [p0];
> > #endif
> > + TRACE_IRQS_ON
> > sti r0;
> >
> > sp += 4; /* Skip Reserved */
>
> no need to save any registers here as all of them get restored from the stack
>
I think 'r0' should be saved here. However, I can move TRACE_IRQS_ON
before setting 'r0':
RETS = [sp++];
TRACE_IRQS_ON
#ifdef CONFIG_SMP
GET_PDA(p0, r0);
r0 = [p0 + PDA_IRQFLAGS];
#else
p0.h = _bfin_irq_flags;
p0.l = _bfin_irq_flags;
r0 = [p0];
#endif
sti r0;
> > Modified: trunk/arch/blackfin/include/asm/irqflags.h (8178 => 8179)
> >
> > +#ifdef CONFIG_TRACE_IRQFLAGS
> > +
> > +.macro bfin_trace_hardirqs_on
> > + [--sp] = rets;
> > + [--sp] = (r7:0, p5:0);
> > + sp += -12;
> > + call _trace_hardirqs_on;
> > + sp += 12;
> > + (r7:0, p5:0) = [sp++];
> > + rets = [sp++];
> > +.endm
> > +
> > +.macro bfin_trace_hardirqs_off
> > + [--sp] = rets;
> > + [--sp] = (r7:0, p5:0);
> > + sp += -12;
> > + call _trace_hardirqs_off;
> > + sp += 12;
> > + (r7:0, p5:0) = [sp++];
> > + rets = [sp++];
> > +.endm
>
> this seems pretty heavy handed. are you sure you need to save rets ?
> any called C function should be saving/restoring it itself as part of
> the ABI. it might be better to have the call site worry about
> registers that need saving since none of the ones here need any regs
> saved ...
>
Yes. rets need to be saved here, because the C function
"trace_hardirqs_on(off)" is called in assembly code, and sometimes the
assembly code may not follow the ABI. e.g:
ENTRY(_evt_evt14)
#ifdef CONFIG_DEBUG_HWERR
r0 = (EVT_IVHW | EVT_IRPTEN | EVT_EVX | EVT_NMI | EVT_RST |
EVT_EMU);
sti r0;
#else
cli r0;
#endif
TRACE_IRQS_OFF /* rets is changed here */
[--sp] = RETI;
SP += 4;
rts; /* "rts" will return to wrong position */
ENDPROC(_evt_evt14)
Another way is to rewrite "evt_evt14"...
> > +# define TRACE_IRQS_ON bfin_trace_hardirqs_on
> > +# define TRACE_IRQS_OFF bfin_trace_hardirqs_off
> > +#else
> > +# define TRACE_IRQS_ON
> > +# define TRACE_IRQS_OFF
> > #endif
>
> this indirection really isnt needed. have the assembly macro expand
> into nothing:
> .macro bfin_trace_hardirqs_off
> #ifdef TRACE_IRQS_ON
> ....
> #endif
> .endm
>
> then always call bfin_trace_hardirqs_{on,off}
>
Agree.
Given the fact that the places using TRACE_IRQS_ON(OFF) is not so many,
and registers may need to save according to the context, I am thinking
to call the C function "trace_hardirqs_on(off)" directly, without using
macros.
> > Modified: trunk/arch/blackfin/mach-common/entry.S (8178 => 8179)
> >
> > @@ -966,6 +966,7 @@
> > #else
> > cli r0;
> > #endif
> > + TRACE_IRQS_OFF
> > [--sp] = RETI;
> > SP += 4;
> > rts;
>
> i dont think any regs need saving here
>
At least RETS need to be saved here.
> > @@ -997,6 +998,7 @@
> > p0.h = _bfin_irq_flags;
> > r0 = [p0];
> > #endif
> > + TRACE_IRQS_ON
> > sti r0;
> >
> > /* finish the userspace "atomic" functions for it */
>
> nor here
>
"r0" need to be saved here, or move TRACE_IRQS_ON backward.
> > Modified: trunk/arch/blackfin/mach-common/interrupt.S (8178 => 8179)
> >
> > @@ -88,6 +88,7 @@
> > #else
> > cli r1;
> > #endif
> > + TRACE_IRQS_OFF
> > [--sp] = RETI; /* orig_pc */
> > /* Clear all L registers. */
> > r1 = 0 (x);
>
> all state was just saved, so no need to save any regs here
Agree.
> -mike
-Yi
_______________________________________________
Linux-kernel-commits mailing list
[email protected]
https://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
