Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Thomas Gleixner]

On Sun, 13 Mar 2016, Jianyu Zhan wrote:
> On Sun, Mar 13, 2016 at 2:35 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >> On Sun, Mar 13, 2016 at 4:08 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >> > This is pointless, because it's only called when local apic is enabled as all
> >> > call sites of alloc_intr_gate() depend on CONFIG_X86_LOCAL_APIC ....
> >>
> >> Not exactly,  currently at least  smp_intr_init()  DOES NOT depend on
> >> CONFIG_X86_LOCAL_APIC:
> >>
> >> static void __init smp_intr_init(void)
> >> {
> >> #ifdef CONFIG_SMP
> >
> > It does, because CONFIG_SMP enables CONFIG_X86_LOCAL_APIC
> >
> 
> It is.  Once CONFIG_SMP is on, CONFIG_X86_LOCAL_APIC will be turned on.
> 
> So the situation should be described as:   first_system_vector will be updated
> on system vector init,  on SMP case(which indirectly implies
> CONFIG_X86_LOCAL_APIC)
> and on explicit CONFIG_X86_LOCAL_APIC(X86_UP_APIC).
> 
> So initial code:
> 
> #ifndef CONFIG_X86_LOCAL_APIC
> #define first_system_vector NR_VECTORS
> #endif
> 
> is actually not needed,  becaused it won't ever change on !
> CONFIG_X86_LOCAL_APIC
> case.

It will never ever be updated in that case, simply because nothing uses it.
 
> > Do you actually understand how all that works together?
> >
> 
> So the dependency should be reversed, and it should be like this:
> 
> Currently we have CONFIG_X86_LOCAL_APIC detangle from  CONFIG_SMP
> (we can enable CONFIG_X86_LOCAL_APIC on 32-bit uniprocessor).

And what's the benefit of this?
 
> Among these stands out IRQ_WORK,   which neither depends on SMP nor
> CONFIG_X86_LOCAL_APIC.
> 
> So the new init funciton is look like(from [2/3]):
> 
> static void __init system_intr_init(void)
> {
>        smp_intr_init();
> 
> #ifdef CONFIG_IRQ_WORK
>        alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt);
> #endif

And that is completely wrong. IRQ_WORK can work independent of LOCAL_APIC, but
if LOCAL_APIC is disabled it does not use the interrupt, simply because there
is no way to trigger it. That setup is inside #ifdef CONFIG_X86_LOCAL_APIC for
exactly that reason.

Just because IRQ_WORK has no config dependency on LOCAL APIC that does not
mean it uses the interrupt gate unconditionally.

The code is correct as is and there is no reason to shuffle it in circles for
no value.

Thanks,

	tglx