Message-ID: <53ECCF72.5050102@linaro.org>
Date:	Thu, 14 Aug 2014 16:02:10 +0100
From:	Daniel Thompson <daniel.thompson@...aro.org>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
CC:	Anton Vorontsov <anton.vorontsov@...aro.org>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	kgdb-bugreport@...ts.sourceforge.net, patches@...aro.org,
	linaro-kernel@...ts.linaro.org,
	John Stultz <john.stultz@...aro.org>,
	Colin Cross <ccross@...roid.com>, kernel-team@...roid.com,
	Rob Herring <robherring2@...il.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Ben Dooks <ben.dooks@...ethink.co.uk>,
	Catalin Marinas <catalin.marinas@....com>,
	Dave Martin <Dave.Martin@....com>,
	Fabio Estevam <festevam@...il.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Nicolas Pitre <nico@...aro.org>
Subject: Re: [PATCH v8 4/4] ARM: Add KGDB/KDB FIQ debugger generic code

On 14/08/14 13:36, Russell King - ARM Linux wrote:
> On Thu, Aug 14, 2014 at 11:48:36AM +0100, Daniel Thompson wrote:
>> Don't worry, you certainly did make it clear!
>>
>> I actually went as far as removing the trace_* calls before having a
>> change of heart. As a result I spent some time looking at the
>> trace_hardirqs_on() code and, as far as I remember, I couldn't find a
>> lock in the code that wasn't bypassed when in_nmi() returned true.
>>
>> However the code is pretty complex and its certainly possible that I
>> missed something. Are you in a position to point any particular lock to
>> show I messed this up or are you working from memory?
> 
> The complexity alone is an argument against calling it from FIQ context,
> because there's no guarantee that what's there will stay that way.
> 
> trace_hardirqs_on() is made more complex because there's not one function
> with that name, but two.  One is in the trace code, the other is in the
> lockdep code.  Here's the trace code, which has at least two problems:
> 
> trace_hardirqs_on
>  -> stop_critical_timing
>   -> check_critical_timing
>    -> raw_spin_lock_irqsave(&max_trace_lock, flags);
>    -> __trace_stack
>     -> __ftrace_trace_stack
>      -> save_stack_trace
>       -> __save_stack_trace
>        -> walk_stackframe
>         -> unwind_frame
>          -> unwind_find_idx
>           -> spin_lock_irqsave(&unwind_lock, flags);

Looks like I had an out-by-one error when reviewing this... sadly the
value was boolean.

Thanks this is very clear.


>>> So, how about moving the FIQ assembly code to entry-armv.S and making
>>> it less kgdb specific?  (Though... we do want to keep a /very/ close
>>> eye on users to ensure that they don't do silly stuff with locking.)
>>
>> I think I can do that.
>>
>> Something like this?
>>
>> 1. Install the current trap handler code by default (either with or
>>    without trace_* calls).
> 
> Without trace_* calls.  The FIQ should be transparent to tracing -
> tracing and lockdep is too much of an unknown (due to size and
> complexity) to ensure that it always follows the rules.

Will do.


>> 2. Have default trap handler call an RCU notifier chain to allow it to
>>    hook up with "normal" code without any hard coding (kgdb, IPI
>>    handling, etc)
> 
> Maybe... that sounds like it opens up FIQ for general purpose use which
> is something I want to avoid - I've little motivation to ensure that
> everyone plays by the rules.  Given the choice, I'd rather maintain our
> present stance that using FIQs is hard and requires a lot of thought.

I'll see what feels natural. Not exporting the symbol to register for
notification would go some way to preventing abuse whilst still allowing
decoupling from kgdb (which cannot be a module).


>> 3. Retain existing install_fiq() behaviour for people who want FIQ
>>    to be a fast-interrupt rather than an NMI (for example, the
>>    Raspberry pi USB optimizations).
> 
> Yes, arch/arm/kernel/fiq.c should be relatively untouched by the core
> mods I suggested - we're just replacing the default "subs" instruction
> (which just ignores the FIQ) with something which can do something
> with it.
> 
> It should be noted that using arch/arm/kernel/fiq.c would override this
> default FIQ handler - and that's a feature of it - fiq.c is based on
> the premise that there is only one single owner of the FIQ at any one
> time and there is no sharing of it, and that's something we want to
> retain.

However, particularly for IPIs, if we've build useful debug features
using them it would be nice to provide hooks that the owner can use to
keep them working if they wish.

However I agree with your response to #4; interfacing at a level lower
than C ABI isn't right. To use any hooks the owner must first make it
safe to call C.


>> 4. Ensure default handler is an exported symbol to allow the meths
>>    drinkers from #3 to chain to logic for NMI/debug features
>>    if they want to.
> 
> That's not something I particularly want to permit - especially as the
> requirements for each "handler" are different - this new default code
> relies upon r13 pointing at some storage to save some registers, which
> may not be true of other FIQ handlers.  Chaining it means that we
> force that use of r13 on others, and we then have to also come up with
> some way to export the correct r13 value.
> 
> Given that fiq.c doesn't work with SMP, this isn't something I want to
> encourage.

Makes sense.


> Further more, I can't get excited about Raspberry Pi "optimisations"
> using FIQ until I see the code, and I see no reason to think about
> catering for such stuff until the patches become visible here.

IIRC they don't add any use-cases beyond the existing in-kernel FIQ
users (such as R-PC floppy disc). It's just that they do it on more
recent hardware (and as you say, out-of-tree).

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists