Re: [tip:irq/irqdomain] irqdomain: Introduce helper function irq_domain_add_hierarchy()

[Thomas Gleixner <tglx@linutronix.de>]

On Sat, 29 Nov 2014, Borislav Petkov wrote:
> So I'm seeing the lockdep splat below really early on an IVB laptop.
> 
> Basically we're not supposed to do __GFP_FS allocations with IRQs off:
> 
>   2737		/* We're only interested __GFP_FS allocations for now */
>   2738		if (!(gfp_mask & __GFP_FS))
>   2739			return;
>   2740	
>   2741		/*
>   2742		 * Oi! Can't be having __GFP_FS allocations with IRQs disabled.
>   2743		 */
>   2744		if (DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))) 			<--- HERE!
>   2745			return;
>   2746	
>   2747		mark_held_locks(curr, RECLAIM_FS);
>   2748	}
> 
> Now, AFAICT, enable_IR_x2apic() disables interrupts and the whole init
> is done with IRQs off but down that path intel_setup_irq_remapping()
> calls irq_domain_add_hierarchy() and it does by default GFP_KERNEL
> allocations.
> 
> The obvious fix is this and the machine boots fine with it. I'm not sure
> it is kosher though so I rather run it by people first:
> 
> ---
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 7fac311057b8..c21a003b996a 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -46,14 +46,18 @@ struct irq_domain *__irq_domain_add(struct device_node *of_node, int size,
>  				    void *host_data)
>  {
>  	struct irq_domain *domain;
> +	gfp_t gfp_flags = GFP_KERNEL;
> +
> +	if (irqs_disabled())
> +		gfp_flags = GFP_NOFS;

We want to use GFP_ATOMIC for that, but I really hate to do so. There
is no reason except for the early boot stage to call into this code
with interrupts disabled. And there we are covered by gfp_allowed_mask,
so that a GFP_KERNEL allocation can succeed.

I have no idea, why enable_IR_x2apic() has been bolted into
smp_prepare_cpus(). Probably just because.

There is no reason WHY this cannot be done in the early irq setup path
(at least nowadays with the allocators being available early), but
that is another area which needs some care and cleanup, but definitely
too late before the 3.19 merge window opens.

So we have to bite the bullet and apply something like this along with
a big fat comment WHY we are doing so and I'm tempted to wrap this
into a #ifdef CONFIG_X86 so that noone else thinks that calling this
code with interrupts disabled - except for the early boot stage - is a
brilliant idea.

Thanks,

	tglx