On Tue, Jan 19, 2010 at 02:16, Li Yi wrote:
> 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/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:
sorry, you're correct of course. i was confusing non-leaf functions
with leaf functions. the former will save/restore rets because it
calls another function while the latter does not. so we'll need to
save/restore the rets when appropriate as you note.
>> > +# 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.
yeah, and since each location has so much possible register
requirements, that's probably the way to go
-mike
_______________________________________________
Linux-kernel-commits mailing list
[email protected]
https://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
