Re: [PATCH] powerpc/mpic: Disable preemption when calling mpic_processor_id()

[Scott Wood <scottwood@freescale.com>]

On Fri, 2013-09-27 at 12:09 -0500, Kumar Gala wrote:
> On Sep 27, 2013, at 11:15 AM, Scott Wood wrote:
> 
> > On Fri, 2013-09-27 at 10:52 -0500, Kumar Gala wrote:
> >> On Sep 26, 2013, at 7:18 PM, Scott Wood wrote:
> >> 
> >>> Otherwise, we get a debug traceback due to the use of
> >>> smp_processor_id() (or get_paca()) inside hard_smp_processor_id().
> >>> mpic_host_map() is just looking for a default CPU, so it doesn't matter
> >>> if we migrate after getting the CPU ID.
> >>> 
> >>> Signed-off-by: Scott Wood <scottwood@freescale.com>
> >>> ---
> >>> arch/powerpc/sysdev/mpic.c | 8 +++++++-
> >>> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> >>> index 1be54fa..bdcb858 100644
> >>> --- a/arch/powerpc/sysdev/mpic.c
> >>> +++ b/arch/powerpc/sysdev/mpic.c
> >>> @@ -1088,8 +1088,14 @@ static int mpic_host_map(struct irq_domain *h, unsigned int virq,
> >>> 	 * is done here.
> >>> 	 */
> >>> 	if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
> >>> +		int cpu;
> >>> +
> >>> +		preempt_disable();
> >>> +		cpu = mpic_processor_id(mpic);
> >>> +		preempt_enable();
> >>> +
> >> 
> >> Any reason you didn't stick this inside of mpic_processor_id() ?
> > 
> > Because the debug check might be valid for other callers and we don't
> > want to defeat it.  In this caller it's used only as a heuristic and
> > thus it doesn't matter if we re-enable preemption before using the
> > result.
> 
> Gotcha, I think you should reword the commit message.  Reading it makes
> me think that preemption should be disabled for all mpic_processor_id()
> calls.

It should be disabled for all mpic_processor_id() calls -- but some
calls will need preemption to be disabled for longer than just the call
to mpic_processor_id().

I did mention in the commit message why mpic_host_map() is unusual.

BTW, I wonder why we don't use mpic_set_affinity (and thus
irq_choose_cpu), both here and in the non-MPIC_NO_RESET case.  Though I
usually notice IRQs being round robinned -- I guess userspace is writing
to the affinity in proc?  Or is something elsewhere in the kernel
calling set_affinity at some point?

Another thing I just noticed while looking at IRQ_DESTINATION-related
code, in mpic_teardown_this_cpu(), what stops it from removing the only
CPU that an interrupt is targeted at?

-Scott