| Message ID | 1401178092-1228-20-git-send-email-jiang.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Jiang, On Tue, 27 May 2014, Jiang Liu wrote: > +static int alloc_irq_from_domain(struct irq_domain *domain, u32 gsi, int pin) > { > + int irq = -1; > + > + if (gsi >= arch_dynirq_lower_bound(0)) { > + irq = irq_create_mapping(domain, pin); > + } else if (gsi < NR_IRQS_LEGACY) { > + if (!ioapic_identity_map) > + irq = irq_create_mapping(domain, pin); > + else if (irq_domain_associate(domain, gsi, pin) == 0) > + irq = gsi; > + } else if (irq_create_strict_mappings(domain, gsi, pin, 1) == 0) { > + irq = gsi; > + } So you have these cases covered here: 1) The ACPI case of secondary ioapics. You only have the strict 1:1 mapping for the first ioapic 2) The gsi < NR_IRQS_LEGACY case where you have two options: a) Let the core create a random virq number if ioapic_identity_map is 0 ioapic_identity_map is only set by SFI and devicetree So in all other cases we fall into that code path for all legacy interrupts. So how is that supposed to work lets say for i8042 which has hardcoded irq 1 and 12? irq_create_mapping(1) hint = 1 % nr_irqs; --> 1 virq = irq_alloc_desc_from(hint, of_node_to_nid(domain->of_node)); This returns something >= 16, because the irq descriptors for 0-15 (LEGACY) are allocated already. The pin association works, but how is the i8042 driver supposed to figure out that it should request the virq >=16 which was created instead of the hardcoded 1 ? b) Associate the gsi and the pin This only works because the virqs are already allocated at boot time unconditionally due to arch_probe_nr_irqs() returning NR_IRQS_LEGACY. So irq_domain_associate() works. Undocumented works by chance behaviour. 3) The case where gsi < arch_dynirq_lower_bound() You create a strict mapping here, fine. This is confusing at best. First of all, we should use legacy_pic->nr_legacy_irqs instead of NR_IRQS_LEGACY all over the place. mshyperv, ce4100 and intel-mid use the null_legacy_pic which has nr_legacy_irqs = 0 and everything else uses the real pic which has nr_legacy_irqs = NR_IRQS_LEGACY. So why do we even bother to allocate and deal with NR_IRQS_LEGACY in the cases where we have no legacy? ce4100 is an oddball though. The ioapic is registered way before the interrupt subsystem is initialized and I have a hard time to understand that comment: /* We can't set this earlier, because we need to calibrate the timer */ legacy_pic = &null_legacy_pic; The timer calibration happens after the interrupts are set up. I assume it's check_timer() which wants that, but we know exactly how the ce4100 works, so we might be able to avoid that whole "testing" stuff. Sebastian, any input on this? If it turns out that ce4100 needs the inital real legacy pic for some magic reason we still can be clever by extending the legacy pic data structure to tell us about that change, i.e. instead of using legacy_pic->nr_legacy_irqs having a field "nr_allocated_irqs", which is set to NR_IRQS_LEGACY for the real pic and to 0 for the null_pic and let ce4100 set that field to NR_IRQS_LEGACY before switching the legacy_pic over to the null implementation. But what's really disgusting is the magic ioapic_identity_map and the extra ACPI specific ioapic_dynirq_base hackery. Why do we need strict mappings in the non ACPI case for all ioapic pins? What's so different about ACPI? Or is this just to avoid breaking the existing SFI/devicetree stuff. If that's the reason I'm fine with it, but ... independent of this question we want to be more clever about the handling of strict, legacy and freely associated linux irq numbers. So you have this weird ioapic_create_domain_fn callback in mp_register_ioapic, which is solely there so the different callers can hand in their domain ops and eventually set the magic ioapic_identity_map flag. +void __init mp_register_ioapic(int id, u32 address, u32 gsi_base, + ioapic_create_domain_fn cb, void *arg) What about having struct ioapic_domain { struct irqdomain *domain; const struct irq_domain_ops *ops; void *arg; enum domain_type type; }; and add this struct to the ioapic struct. type is: enum domain_type { IOAPIC_STRICT, IOAPIC_LEGACY, IOAPIC_DYNAMIC, }; Now the register function changes to: void mp_register_ioapic(int id, u32 address, u32 gsi_base, const struct irq_domain_ops *ops, int type) { ... ioapics[idx].irqdomain.ops = ops; ioapics[idx].irqdomain.arg = arg; ioapics[idx].irqdomain.type = type; ioapics[idx].irqdomain.domain = NULL; ... } and you can use mp_irqdomain_create() unconditionally for creating all domains. And there you do: static int dynirq_lower_bound; mp_irqdomain_create() { ioapic->irqdomain.domain = irq_domain_add_linear(...); switch (ioapic->irqdomain.type) { case IOAPIC_LEGACY: /* * Associate the legacy interrupts which have been * already allocated. */ for (i = 0; i < legacy_pic->nr_allocated_irqs; i++) irq_domain_associate(domain, i, i); case IOAPIC_STRICT: dynirq_lower_bound += ioapic->nr_gsis; case IOAPIC_DYNAMIC: break; } } So arch_dynirq_lower_bound() gets simplified to: { return dynirq_lower_bound; } And alloc_irq_from_domain() becomes: int alloc_irq_from_domain(struct ioapic_domain *domain, int gsi, int pin) { switch (domain->type) { case IOAPIC_DYNAMIC: return irq_create_mapping(domain->domain, pin); case IOAPIC_LEGACY: case IOAPIC_STRICT: return irq_create_strict_mappings(domain, gsi, pin, 1); } } At the call site of alloc_irq_from_domain() you have already: irq = irq_find_mapping(domain, pin); if (irq <=0 ....) alloc_irq_from_domain(domain, gsi, pin); So because we associated the legacy_pic->nr_allocated_irqs in mp_irqdomain_create() already, you'll never call into alloc_irq_from_domain() for those and the remaining ones for that first ioapic are handled by the IOAPIC_STRICT fall through. For simplicity you can let SFI and devicetree register the ioapics with their specific domain ops plus IOAPIC_LEGACY for the first ioapic and IOAPIC_STRICT for all others. That also covers the case where the null_legacy_pic with legacy_pic->nr_allocated_irqs == 0 is used. In the ACPI case you register with the acpi domain ops and IOAPIC_LEGACY for the first and IOAPIC_DYNAMIC for the extra ioapics. Should be way cleaner and understandable, at least to me :) Now there is that last oddity which bugs me in mp_map_pin_to_irq() /* * Don't use irqdomain to manage ISA IRQs because there may be * multiple IOAPIC pins sharing the same ISA IRQ number and * irqdomain only supports 1:1 mapping between IOAPIC pin and * IRQ number. */ if (idx >= 0 && test_bit(mp_irqs[idx].srcbus, mp_bus_not_pci)) { irq = mp_irqs[idx].srcbusirq; if ((flags & IOAPIC_MAP_ALLOC) && info->count == 0 && mp_irqdomain_map(domain, irq, pin) != 0) irq = -1; That really looks like a hack. I'm aware that the current irqdomain code cannot deal with that oddball case. So what you are saying is that there are devices which have a separate physical wire to different ioapic pins, but the ioapic is supposed to bundle them to a shared interrupt. I agree that this is odd enough to handle at the ioapic level, but it'd be nice to have a more elaborative comment on this. Aside of the above I'm pretty happy about the progress of this patch set. One thing, which needs to be looked at are the usage sites of irq_data->irq, whether they are safe. I did not spot any unsafe ones, but a few functions which are called with irq_data->irq and make no use of it. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thomas, Thanks for your comments. Please refer to inline comments below. On 2014/5/28 3:58, Thomas Gleixner wrote: > Jiang, > > On Tue, 27 May 2014, Jiang Liu wrote: > >> +static int alloc_irq_from_domain(struct irq_domain *domain, u32 gsi, int pin) >> { >> + int irq = -1; >> + >> + if (gsi >= arch_dynirq_lower_bound(0)) { >> + irq = irq_create_mapping(domain, pin); >> + } else if (gsi < NR_IRQS_LEGACY) { >> + if (!ioapic_identity_map) >> + irq = irq_create_mapping(domain, pin); >> + else if (irq_domain_associate(domain, gsi, pin) == 0) >> + irq = gsi; >> + } else if (irq_create_strict_mappings(domain, gsi, pin, 1) == 0) { >> + irq = gsi; >> + } > > So you have these cases covered here: > > 1) The ACPI case of secondary ioapics. You only have the strict 1:1 > mapping for the first ioapic > > 2) The gsi < NR_IRQS_LEGACY case where you have two options: > > a) Let the core create a random virq number if ioapic_identity_map > is 0 > > ioapic_identity_map is only set by SFI and devicetree > > So in all other cases we fall into that code path for all > legacy interrupts. So how is that supposed to work lets say for > i8042 which has hardcoded irq 1 and 12? > > irq_create_mapping(1) > > hint = 1 % nr_irqs; --> 1 > virq = irq_alloc_desc_from(hint, of_node_to_nid(domain->of_node)); > > This returns something >= 16, because the irq descriptors > for 0-15 (LEGACY) are allocated already. > > The pin association works, but how is the i8042 driver supposed > to figure out that it should request the virq >=16 which was > created instead of the hardcoded 1 ? This is used to work around special non-ISA interrupts with GSI below NR_IRQS_LEGACY. The original code for the special case is: /* * Provide an identity mapping of gsi == irq except on truly * weird platforms that have non isa irqs in the first 16 gsis. */ return gsi >= NR_IRQS_LEGACY ? gsi : gsi_top + gsi; We have one path to handle ISA IRQs before calling alloc_irq_from_domain() as below: if (idx >= 0 && test_bit(mp_irqs[idx].srcbus, mp_bus_not_pci)) return mp_irqs[idx].srcbusirq; > > b) Associate the gsi and the pin > > This only works because the virqs are already allocated at boot > time unconditionally due to arch_probe_nr_irqs() returning > NR_IRQS_LEGACY. So irq_domain_associate() works. > Undocumented works by chance behaviour. Yes. It's a good suggestion to enhance legacy_pic to make this code more clear. > > 3) The case where gsi < arch_dynirq_lower_bound() > > You create a strict mapping here, fine. > > This is confusing at best. > > First of all, we should use legacy_pic->nr_legacy_irqs instead of > NR_IRQS_LEGACY all over the place. > > mshyperv, ce4100 and intel-mid use the null_legacy_pic which has > nr_legacy_irqs = 0 and everything else uses the real pic which has > nr_legacy_irqs = NR_IRQS_LEGACY. So why do we even bother to allocate > and deal with NR_IRQS_LEGACY in the cases where we have no legacy? I'm not sure whether it works with ce4100, so used NR_IRQS_LEGACY instead of legacy_pic->nr_legacy_irqs for safety. Will try to refine it in next version. > > ce4100 is an oddball though. The ioapic is registered way before the > interrupt subsystem is initialized and I have a hard time to > understand that comment: > > /* We can't set this earlier, because we need to calibrate the timer */ > legacy_pic = &null_legacy_pic; I haven't figured out the story behind the comment yet:( > > The timer calibration happens after the interrupts are set up. I > assume it's check_timer() which wants that, but we know exactly how > the ce4100 works, so we might be able to avoid that whole "testing" > stuff. Sebastian, any input on this? > > If it turns out that ce4100 needs the inital real legacy pic for some > magic reason we still can be clever by extending the legacy pic data > structure to tell us about that change, i.e. instead of using > legacy_pic->nr_legacy_irqs having a field "nr_allocated_irqs", which > is set to NR_IRQS_LEGACY for the real pic and to 0 for the null_pic > and let ce4100 set that field to NR_IRQS_LEGACY before switching the > legacy_pic over to the null implementation. Good suggestion, will try this way. > But what's really disgusting is the magic ioapic_identity_map and the > extra ACPI specific ioapic_dynirq_base hackery. > > Why do we need strict mappings in the non ACPI case for all ioapic > pins? What's so different about ACPI? Or is this just to avoid > breaking the existing SFI/devicetree stuff. If that's the reason I'm > fine with it, but ... It's to avoid breaking SFI/intel_mid stuff. intel_mid assumes IRQ number equals to pin number and use pci_dev->irq to save both IRQ number and pin number. > > independent of this question we want to be more clever about the > handling of strict, legacy and freely associated linux irq numbers. > > So you have this weird ioapic_create_domain_fn callback in > mp_register_ioapic, which is solely there so the different callers can > hand in their domain ops and eventually set the magic > ioapic_identity_map flag. > > +void __init mp_register_ioapic(int id, u32 address, u32 gsi_base, > + ioapic_create_domain_fn cb, void *arg) > > What about having > > struct ioapic_domain { > struct irqdomain *domain; > const struct irq_domain_ops *ops; > void *arg; > enum domain_type type; > }; > > and add this struct to the ioapic struct. type is: > > enum domain_type { > IOAPIC_STRICT, > IOAPIC_LEGACY, > IOAPIC_DYNAMIC, > }; > > > Now the register function changes to: > > void mp_register_ioapic(int id, u32 address, u32 gsi_base, > const struct irq_domain_ops *ops, > int type) > { > ... > > ioapics[idx].irqdomain.ops = ops; > ioapics[idx].irqdomain.arg = arg; > ioapics[idx].irqdomain.type = type; > ioapics[idx].irqdomain.domain = NULL; > > ... > } > > and you can use mp_irqdomain_create() unconditionally for creating all > domains. And there you do: > > static int dynirq_lower_bound; > > mp_irqdomain_create() > { > ioapic->irqdomain.domain = irq_domain_add_linear(...); > > switch (ioapic->irqdomain.type) { > case IOAPIC_LEGACY: > /* > * Associate the legacy interrupts which have been > * already allocated. > */ > for (i = 0; i < legacy_pic->nr_allocated_irqs; i++) > irq_domain_associate(domain, i, i); > > case IOAPIC_STRICT: > dynirq_lower_bound += ioapic->nr_gsis; > > case IOAPIC_DYNAMIC: > break; > } > } > > So arch_dynirq_lower_bound() gets simplified to: > { > return dynirq_lower_bound; > } > > And alloc_irq_from_domain() becomes: > > int alloc_irq_from_domain(struct ioapic_domain *domain, int gsi, int pin) > { > switch (domain->type) { > case IOAPIC_DYNAMIC: > return irq_create_mapping(domain->domain, pin); > case IOAPIC_LEGACY: > case IOAPIC_STRICT: > return irq_create_strict_mappings(domain, gsi, pin, 1); > } > } > > At the call site of alloc_irq_from_domain() you have already: > > irq = irq_find_mapping(domain, pin); > if (irq <=0 ....) > alloc_irq_from_domain(domain, gsi, pin); > > So because we associated the legacy_pic->nr_allocated_irqs in > mp_irqdomain_create() already, you'll never call into > alloc_irq_from_domain() for those and the remaining ones for that > first ioapic are handled by the IOAPIC_STRICT fall through. > > For simplicity you can let SFI and devicetree register the ioapics > with their specific domain ops plus IOAPIC_LEGACY for the first ioapic > and IOAPIC_STRICT for all others. That also covers the case where the > null_legacy_pic with legacy_pic->nr_allocated_irqs == 0 is used. > > In the ACPI case you register with the acpi domain ops and > IOAPIC_LEGACY for the first and IOAPIC_DYNAMIC for the extra ioapics. > > Should be way cleaner and understandable, at least to me :) Really good suggestions! Make thing much more clear. > > Now there is that last oddity which bugs me in mp_map_pin_to_irq() > > /* > * Don't use irqdomain to manage ISA IRQs because there may be > * multiple IOAPIC pins sharing the same ISA IRQ number and > * irqdomain only supports 1:1 mapping between IOAPIC pin and > * IRQ number. > */ > if (idx >= 0 && test_bit(mp_irqs[idx].srcbus, mp_bus_not_pci)) { > irq = mp_irqs[idx].srcbusirq; > if ((flags & IOAPIC_MAP_ALLOC) && info->count == 0 && > mp_irqdomain_map(domain, irq, pin) != 0) > irq = -1; > > That really looks like a hack. I'm aware that the current irqdomain > code cannot deal with that oddball case. > > So what you are saying is that there are devices which have a separate > physical wire to different ioapic pins, but the ioapic is supposed to > bundle them to a shared interrupt. > > I agree that this is odd enough to handle at the ioapic level, but > it'd be nice to have a more elaborative comment on this. Will try to improve the comment. > > Aside of the above I'm pretty happy about the progress of this patch > set. One thing, which needs to be looked at are the usage sites of > irq_data->irq, whether they are safe. I did not spot any unsafe ones, > but a few functions which are called with irq_data->irq and make no > use of it. Sure, will check usages of irq_data->irq. Really appreciate you suggestions, it will improve the code a lot. Thanks! Gerry > > Thanks, > > tglx > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/27/2014 09:58 PM, Thomas Gleixner wrote: > ce4100 is an oddball though. The ioapic is registered way before the > interrupt subsystem is initialized and I have a hard time to > understand that comment: > > /* We can't set this earlier, because we need to calibrate the timer */ > legacy_pic = &null_legacy_pic; > > The timer calibration happens after the interrupts are set up. I > assume it's check_timer() which wants that, but we know exactly how > the ce4100 works, so we might be able to avoid that whole "testing" > stuff. Sebastian, any input on this? According to my memory there was some PIC init we need but I don't recall the details. However, booting the system with "legacy_pic = &null_legacy_pic;" in x86_ce4100_early_setup() gives me this: [ 0.001000] Enabling APIC mode: Flat. Using 2 I/O APICs [ 0.001000] leaving PIC mode, enabling APIC mode. [ 0.001000] enabled ExtINT on CPU#0 [ 0.001000] ENABLING IO-APIC IRQs [ 0.001000] Setting 1 in the phys_id_present_map [ 0.001000] ...changing IO-APIC physical APIC ID to 1 ... ok. [ 0.001000] Setting 2 in the phys_id_present_map [ 0.001000] ...changing IO-APIC physical APIC ID to 2 ... ok. [ 0.001000] smpboot: CPU0: Intel(R) Atom(TM) CPU CE4150 @ 1.20GHz (fam: 06, model: 1c, stepping: 0a) [ 0.001000] Using local APIC timer interrupts. [ 0.001000] calibrating APIC timer ... and we stand still. With the assignment as it is now: [ 0.001000] Enabling APIC mode: Flat. Using 2 I/O APICs [ 0.001000] leaving PIC mode, enabling APIC mode. [ 0.001000] enabled ExtINT on CPU#0 [ 0.002312] ENABLING IO-APIC IRQs [ 0.003009] Setting 1 in the phys_id_present_map [ 0.004007] ...changing IO-APIC physical APIC ID to 1 ... ok. [ 0.005533] Setting 2 in the phys_id_present_map [ 0.006006] ...changing IO-APIC physical APIC ID to 2 ... ok. [ 0.008373] ..TIMER: vector=0x30 apic1=-1 pin1=-1 apic2=-1 pin2=-1 [ 0.009000] ...trying to set up timer as Virtual Wire IRQ... [ 0.019490] ..... works. [ 0.020005] smpboot: CPU0: Intel(R) Atom(TM) CPU CE4150 @ 1.20GHz (fam: 06, model: 1c, stepping: 0a) [ 0.024005] Using local APIC timer interrupts. [ 0.024005] calibrating APIC timer ... [ 0.026000] ... lapic delta = 624992 [ 0.026000] ... PM-Timer delta = 0 [ 0.026000] ..... delta 624992 [ 0.026000] ..... mult: 26843202 [ 0.026000] ..... calibration result: 99998 [ 0.026000] ..... CPU clock speed is 1199.0984 MHz. [ 0.026000] ..... host bus clock speed is 99.0998 MHz. [ 0.026000] ... verify APIC timer [ 0.128565] ... jiffies delta = 100 [ 0.129004] ... jiffies result ok and it goes on. As you see, "..TIMER:" is missing in the upper output. > > Thanks, > > tglx Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 28 May 2014, Sebastian Andrzej Siewior wrote: > On 05/27/2014 09:58 PM, Thomas Gleixner wrote: > > ce4100 is an oddball though. The ioapic is registered way before the > > interrupt subsystem is initialized and I have a hard time to > > understand that comment: > > > > /* We can't set this earlier, because we need to calibrate the timer */ > > legacy_pic = &null_legacy_pic; > > > > The timer calibration happens after the interrupts are set up. I > > assume it's check_timer() which wants that, but we know exactly how > > the ce4100 works, so we might be able to avoid that whole "testing" > > stuff. Sebastian, any input on this? > > According to my memory there was some PIC init we need but I don't > recall the details. However, booting the system with "legacy_pic = > &null_legacy_pic;" in x86_ce4100_early_setup() gives me this: > > [ 0.001000] Enabling APIC mode: Flat. Using 2 I/O APICs > [ 0.001000] leaving PIC mode, enabling APIC mode. > [ 0.001000] enabled ExtINT on CPU#0 > [ 0.001000] ENABLING IO-APIC IRQs > [ 0.001000] Setting 1 in the phys_id_present_map > [ 0.001000] ...changing IO-APIC physical APIC ID to 1 ... ok. > [ 0.001000] Setting 2 in the phys_id_present_map > [ 0.001000] ...changing IO-APIC physical APIC ID to 2 ... ok. > [ 0.001000] smpboot: CPU0: Intel(R) Atom(TM) CPU CE4150 @ 1.20GHz > (fam: 06, model: 1c, stepping: 0a) > [ 0.001000] Using local APIC timer interrupts. > [ 0.001000] calibrating APIC timer ... > > and we stand still. With the assignment as it is now: > > [ 0.001000] Enabling APIC mode: Flat. Using 2 I/O APICs > [ 0.001000] leaving PIC mode, enabling APIC mode. > [ 0.001000] enabled ExtINT on CPU#0 > [ 0.002312] ENABLING IO-APIC IRQs > [ 0.003009] Setting 1 in the phys_id_present_map > [ 0.004007] ...changing IO-APIC physical APIC ID to 1 ... ok. > [ 0.005533] Setting 2 in the phys_id_present_map > [ 0.006006] ...changing IO-APIC physical APIC ID to 2 ... ok. > [ 0.008373] ..TIMER: vector=0x30 apic1=-1 pin1=-1 apic2=-1 pin2=-1 > [ 0.009000] ...trying to set up timer as Virtual Wire IRQ... > [ 0.019490] ..... works. Right, so it needs the setup of irq 0 and that only happens when the legacy_pic->nr_legacy_irqs > 0. Do you remember, why we switch to the null_pic later on? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/28/2014 12:07 PM, Thomas Gleixner wrote: > > Right, so it needs the setup of irq 0 and that only happens when the > legacy_pic->nr_legacy_irqs > 0. > > Do you remember, why we switch to the null_pic later on? According to my memory all interrupts are serviced by the IOAPIC. The first 16 by the first IOAPIC, 17+ (everything behind the "PCI-bus") by the second IOAPIC. For that reason I didn't see a reason for the legacy PIC and set it to null_pic. Due to the timer problem it is delayed. > Thanks, > > tglx Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 28 May 2014, Jiang Liu wrote: > On 2014/5/28 3:58, Thomas Gleixner wrote: > > So you have these cases covered here: > > > > 1) The ACPI case of secondary ioapics. You only have the strict 1:1 > > mapping for the first ioapic > > > > 2) The gsi < NR_IRQS_LEGACY case where you have two options: > > > > a) Let the core create a random virq number if ioapic_identity_map > > is 0 > > > > ioapic_identity_map is only set by SFI and devicetree > > > > So in all other cases we fall into that code path for all > > legacy interrupts. So how is that supposed to work lets say for > > i8042 which has hardcoded irq 1 and 12? > > > > irq_create_mapping(1) > > > > hint = 1 % nr_irqs; --> 1 > > virq = irq_alloc_desc_from(hint, of_node_to_nid(domain->of_node)); > > > > This returns something >= 16, because the irq descriptors > > for 0-15 (LEGACY) are allocated already. > > > > The pin association works, but how is the i8042 driver supposed > > to figure out that it should request the virq >=16 which was > > created instead of the hardcoded 1 ? > This is used to work around special non-ISA interrupts with GSI below > NR_IRQS_LEGACY. The original code for the special case is: > /* > * Provide an identity mapping of gsi == irq except on truly > * weird platforms that have non isa irqs in the first 16 gsis. > */ > return gsi >= NR_IRQS_LEGACY ? gsi : gsi_top + gsi; That looks really, really wrong. What's wrong with assigning that irq irq number on those platforms? The weird stuff is SFI and devicetree, if I understand your code correctly. So if those platforms do not have actual legacy irqs, what's wrong with giving out the legacy numbers? > We have one path to handle ISA IRQs before calling > alloc_irq_from_domain() as below: > if (idx >= 0 && test_bit(mp_irqs[idx].srcbus, mp_bus_not_pci)) > return mp_irqs[idx].srcbusirq; Ok. > > /* We can't set this earlier, because we need to calibrate the timer */ > > legacy_pic = &null_legacy_pic; > I haven't figured out the story behind the comment yet:( Sebastian gave some insight. > > Why do we need strict mappings in the non ACPI case for all ioapic > > pins? What's so different about ACPI? Or is this just to avoid > > breaking the existing SFI/devicetree stuff. If that's the reason I'm > > fine with it, but ... > It's to avoid breaking SFI/intel_mid stuff. intel_mid assumes IRQ > number equals to pin number and use pci_dev->irq to save both IRQ > number and pin number. Fair enough. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 28 May 2014, Thomas Gleixner wrote: > On Wed, 28 May 2014, Jiang Liu wrote: > > This is used to work around special non-ISA interrupts with GSI below > > NR_IRQS_LEGACY. The original code for the special case is: > > /* > > * Provide an identity mapping of gsi == irq except on truly > > * weird platforms that have non isa irqs in the first 16 gsis. > > */ > > return gsi >= NR_IRQS_LEGACY ? gsi : gsi_top + gsi; > > That looks really, really wrong. What's wrong with assigning that irq > irq number on those platforms? > > The weird stuff is SFI and devicetree, if I understand your code > correctly. > > So if those platforms do not have actual legacy irqs, what's wrong > with giving out the legacy numbers? Though if it's a real issue, we can simply mark the ioapic on those devices as DYNAMIC and avoid that whole magic. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 47247708c9eb..ebdb1193821b 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -832,6 +832,7 @@ config X86_IO_APIC def_bool y depends on X86_64 || SMP || X86_32_NON_STANDARD || X86_UP_IOAPIC || PCI_MSI select GENERIC_IRQ_LEGACY_ALLOC_HWIRQ + select IRQ_DOMAIN config X86_REROUTE_FOR_BROKEN_BOOT_IRQS bool "Reroute for broken boot IRQs" diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h index 978e51fdcb59..4778d129f225 100644 --- a/arch/x86/include/asm/io_apic.h +++ b/arch/x86/include/asm/io_apic.h @@ -98,6 +98,8 @@ struct IR_IO_APIC_route_entry { #define IOAPIC_AUTO -1 #define IOAPIC_EDGE 0 #define IOAPIC_LEVEL 1 +#define IOAPIC_MAP_ALLOC 0x1 +#define IOAPIC_MAP_CHECK 0x2 #ifdef CONFIG_X86_IO_APIC @@ -130,6 +132,9 @@ extern int noioapicquirk; /* -1 if "noapic" boot option passed */ extern int noioapicreroute; +/* Build identity mapping for non-ISA IRQs below NR_IRQS_LEGACY */ +extern int ioapic_identity_map; + /* * If we use the IO-APIC for IRQ routing, disable automatic * assignment of PCI IRQ's. @@ -169,10 +174,12 @@ struct mp_ioapic_gsi{ }; extern u32 gsi_top; +typedef struct irq_domain *(*ioapic_create_domain_fn)(int idx, void *arg); + extern int mp_find_ioapic(u32 gsi); extern int mp_find_ioapic_pin(int ioapic, u32 gsi); extern u32 mp_pin_to_gsi(int ioapic, int pin); -extern int mp_map_gsi_to_irq(u32 gsi); +extern int mp_map_gsi_to_irq(u32 gsi, unsigned int flags); extern void __init mp_register_ioapic(int id, u32 address, u32 gsi_base); extern void __init pre_init_apic_IRQ0(void); @@ -215,7 +222,7 @@ static inline void ioapic_insert_resources(void) { } #define gsi_top (NR_IRQS_LEGACY) static inline int mp_find_ioapic(u32 gsi) { return 0; } static inline u32 mp_pin_to_gsi(int ioapic, int pin) { return UINT_MAX; } -static inline int mp_map_gsi_to_irq(u32 gsi) { return gsi; } +static inline int mp_map_gsi_to_irq(u32 gsi, unsigned int flags) { return gsi; } struct io_apic_irq_attr; static inline int io_apic_set_pci_routing(struct device *dev, int irq, diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index f740d2766c42..9bbdd685df64 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -99,7 +99,7 @@ static u32 isa_irq_to_gsi[NR_IRQS_LEGACY] __read_mostly = { #define ACPI_INVALID_GSI INT_MIN -static int map_gsi_to_irq(unsigned int gsi) +static int map_gsi_to_irq(unsigned int gsi, unsigned int flags) { int i; @@ -107,7 +107,7 @@ static int map_gsi_to_irq(unsigned int gsi) if (isa_irq_to_gsi[i] == gsi) return i; - return mp_map_gsi_to_irq(gsi); + return mp_map_gsi_to_irq(gsi, flags); } /* @@ -483,7 +483,7 @@ void __init acpi_pic_sci_set_trigger(unsigned int irq, u16 trigger) int acpi_gsi_to_irq(u32 gsi, unsigned int *irqp) { - int irq = map_gsi_to_irq(gsi); + int irq = map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC | IOAPIC_MAP_CHECK); if (irq >= 0) { #ifdef CONFIG_X86_IO_APIC @@ -1030,7 +1030,7 @@ static int mp_register_gsi(struct device *dev, u32 gsi, int trigger, if (acpi_gbl_FADT.sci_interrupt == gsi) return gsi; - irq = map_gsi_to_irq(gsi); + irq = map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC); if (irq < 0) return irq; diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c index cd3a886431b8..b16e74133686 100644 --- a/arch/x86/kernel/apic/io_apic.c +++ b/arch/x86/kernel/apic/io_apic.c @@ -31,6 +31,7 @@ #include <linux/acpi.h> #include <linux/module.h> #include <linux/syscore_ops.h> +#include <linux/irqdomain.h> #include <linux/msi.h> #include <linux/htirq.h> #include <linux/freezer.h> @@ -83,6 +84,7 @@ int sis_apic_bug = -1; static DEFINE_RAW_SPINLOCK(ioapic_lock); static DEFINE_RAW_SPINLOCK(vector_lock); +static DEFINE_MUTEX(ioapic_mutex); static struct ioapic { /* @@ -97,6 +99,9 @@ static struct ioapic { struct mpc_ioapic mp_config; /* IO APIC gsi routing info */ struct mp_ioapic_gsi gsi_config; + struct irq_domain *irqdomain; + ioapic_create_domain_fn irqdomain_cb; + void *irqdomain_arg; DECLARE_BITMAP(pin_programmed, MP_MAX_IOAPIC_PIN + 1); } ioapics[MAX_IO_APICS]; @@ -135,6 +140,11 @@ static inline int mp_init_irq_at_boot(int ioapic, int irq) return ioapic == 0 || (irq >= 0 && irq < NR_IRQS_LEGACY); } +static inline struct irq_domain *mp_ioapic_irqdomain(int ioapic) +{ + return ioapics[ioapic].irqdomain; +} + int nr_ioapics; /* The one past the highest gsi number used */ @@ -153,6 +163,7 @@ int mp_bus_id_to_type[MAX_MP_BUSSES]; DECLARE_BITMAP(mp_bus_not_pci, MAX_MP_BUSSES); int skip_ioapic_setup; +int ioapic_identity_map; /** * disable_ioapic_support() - disables ioapic support at runtime @@ -963,19 +974,75 @@ static int irq_trigger(int idx) return trigger; } -int mp_map_gsi_to_irq(u32 gsi) +static int alloc_irq_from_domain(struct irq_domain *domain, u32 gsi, int pin) { + int irq = -1; + + if (gsi >= arch_dynirq_lower_bound(0)) { + irq = irq_create_mapping(domain, pin); + } else if (gsi < NR_IRQS_LEGACY) { + if (!ioapic_identity_map) + irq = irq_create_mapping(domain, pin); + else if (irq_domain_associate(domain, gsi, pin) == 0) + irq = gsi; + } else if (irq_create_strict_mappings(domain, gsi, pin, 1) == 0) { + irq = gsi; + } + + return irq > 0 ? irq : -1; +} + +static int mp_map_pin_to_irq(u32 gsi, int idx, int ioapic, int pin, + unsigned int flags) +{ + int irq; + struct irq_domain *domain = mp_ioapic_irqdomain(ioapic); + /* - * Provide an identity mapping of gsi == irq except on truly weird - * platforms that have non isa irqs in the first 16 gsis. + * Don't use irqdomain to manage ISA IRQs because there may be + * multiple IOAPIC pins sharing the same ISA IRQ number and + * irqdomain only supports 1:1 mapping between IOAPIC pin and + * IRQ number. */ - return gsi >= NR_IRQS_LEGACY ? gsi : gsi_top + gsi; + if (idx >= 0 && test_bit(mp_irqs[idx].srcbus, mp_bus_not_pci)) + return mp_irqs[idx].srcbusirq; + + if (!domain) { + /* + * Provide an identity mapping of gsi == irq except on truly + * weird platforms that have non isa irqs in the first 16 gsis. + */ + return gsi >= NR_IRQS_LEGACY ? gsi : gsi_top + gsi; + } + + mutex_lock(&ioapic_mutex); + irq = irq_find_mapping(domain, pin); + if (irq <= 0 && (flags & IOAPIC_MAP_ALLOC)) + irq = alloc_irq_from_domain(domain, gsi, pin); + mutex_unlock(&ioapic_mutex); + + return irq > 0 ? irq : -1; } -static int pin_2_irq(int idx, int apic, int pin) +int mp_map_gsi_to_irq(u32 gsi, unsigned int flags) { - int irq; - int bus = mp_irqs[idx].srcbus; + int ioapic, pin, idx; + + ioapic = mp_find_ioapic(gsi); + if (ioapic < 0) + return -1; + + pin = mp_find_ioapic_pin(ioapic, gsi); + idx = find_irq_entry(ioapic, pin, mp_INT); + if ((flags & IOAPIC_MAP_CHECK) && idx < 0) + return -1; + + return mp_map_pin_to_irq(gsi, idx, ioapic, pin, flags); +} + +static int pin_2_irq(int idx, int ioapic, int pin, unsigned int flags) +{ + u32 gsi = mp_pin_to_gsi(ioapic, pin); /* * Debugging check, we are in big trouble if this message pops up! @@ -993,7 +1060,7 @@ static int pin_2_irq(int idx, int apic, int pin) apic_printk(APIC_VERBOSE, KERN_DEBUG "disabling PIRQ%d\n", pin-16); } else { - irq = pirq_entries[pin-16]; + int irq = pirq_entries[pin-16]; apic_printk(APIC_VERBOSE, KERN_DEBUG "using PIRQ%d -> IRQ %d\n", pin-16, irq); @@ -1003,12 +1070,7 @@ static int pin_2_irq(int idx, int apic, int pin) } #endif - if (test_bit(bus, mp_bus_not_pci)) - irq = mp_irqs[idx].srcbusirq; - else - irq = mp_map_gsi_to_irq(mp_pin_to_gsi(apic, pin)); - - return irq; + return mp_map_pin_to_irq(gsi, idx, ioapic, pin, flags); } /* @@ -1018,7 +1080,7 @@ static int pin_2_irq(int idx, int apic, int pin) int IO_APIC_get_PCI_irq_vector(int bus, int slot, int pin, struct io_apic_irq_attr *irq_attr) { - int irq, i, best_guess = -1; + int irq, i, best_ioapic = -1, best_idx = -1; apic_printk(APIC_DEBUG, "querying PCI -> IRQ mapping bus:%d, slot:%d, pin:%d.\n", @@ -1047,30 +1109,37 @@ int IO_APIC_get_PCI_irq_vector(int bus, int slot, int pin, continue; /* Skip ISA IRQs */ - irq = pin_2_irq(i, ioapic_idx, mp_irqs[i].dstirq); - if (ioapic_idx == 0 && !IO_APIC_IRQ(irq)) + irq = pin_2_irq(i, ioapic_idx, mp_irqs[i].dstirq, 0); + if (irq > 0 && !IO_APIC_IRQ(irq)) continue; if (pin == (mp_irqs[i].srcbusirq & 3)) { - set_io_apic_irq_attr(irq_attr, ioapic_idx, - mp_irqs[i].dstirq, - irq_trigger(i), - irq_polarity(i)); - return irq; + best_idx = i; + best_ioapic = ioapic_idx; + goto out; } + /* * Use the first all-but-pin matching entry as a * best-guess fuzzy result for broken mptables. */ - if (best_guess < 0) { - set_io_apic_irq_attr(irq_attr, ioapic_idx, - mp_irqs[i].dstirq, - irq_trigger(i), - irq_polarity(i)); - best_guess = irq; + if (best_idx < 0) { + best_idx = i; + best_ioapic = ioapic_idx; } } - return best_guess; + if (best_idx < 0) + return -1; + +out: + irq = pin_2_irq(best_idx, best_ioapic, mp_irqs[best_idx].dstirq, + IOAPIC_MAP_ALLOC); + if (irq > 0) + set_io_apic_irq_attr(irq_attr, best_ioapic, + mp_irqs[best_idx].dstirq, + irq_trigger(best_idx), + irq_polarity(best_idx)); + return irq; } EXPORT_SYMBOL(IO_APIC_get_PCI_irq_vector); @@ -1261,7 +1330,7 @@ static inline int IO_APIC_irq_trigger(int irq) for_each_ioapic_pin(apic, pin) { idx = find_irq_entry(apic, pin, mp_INT); - if ((idx != -1) && (irq == pin_2_irq(idx, apic, pin))) + if ((idx != -1) && (irq == pin_2_irq(idx, apic, pin, 0))) return irq_trigger(idx); } /* @@ -1387,8 +1456,9 @@ static void __init __io_apic_setup_irqs(unsigned int ioapic_idx) if (io_apic_pin_not_connected(idx, ioapic_idx, pin)) continue; - irq = pin_2_irq(idx, ioapic_idx, pin); - if (!mp_init_irq_at_boot(ioapic_idx, irq)) + irq = pin_2_irq(idx, ioapic_idx, pin, + ioapic_idx ? 0 : IOAPIC_MAP_ALLOC); + if (irq < 0 || !mp_init_irq_at_boot(ioapic_idx, irq)) continue; /* @@ -1438,8 +1508,8 @@ void setup_IO_APIC_irq_extra(u32 gsi) if (idx == -1) return; - irq = pin_2_irq(idx, ioapic_idx, pin); - if (mp_init_irq_at_boot(ioapic_idx, irq)) + irq = pin_2_irq(idx, ioapic_idx, pin, IOAPIC_MAP_ALLOC); + if (irq < 0 || mp_init_irq_at_boot(ioapic_idx, irq)) return; set_io_apic_irq_attr(&attr, ioapic_idx, pin, irq_trigger(idx), @@ -3554,8 +3624,8 @@ void __init setup_ioapic_dest(void) if (irq_entry == -1) continue; - irq = pin_2_irq(irq_entry, ioapic, pin); - if (!mp_init_irq_at_boot(ioapic, irq)) + irq = pin_2_irq(irq_entry, ioapic, pin, 0); + if (irq < 0 || !mp_init_irq_at_boot(ioapic, irq)) continue; idata = irq_get_irq_data(irq);
Currently x86 support identity mapping between GSI(IOAPIC pin) and IRQ number, so continous IRQs at low end are statically allocated to IOAPICs at boot time. This design causes trouble to support IOAPIC hotplug. This patch implements basic mechanism to dynamically allocate IRQ on demand for IOAPIC pins by using irqdomain framework. It first adds several fields into struct ioapic to support irqdomain. Then it implements an algorithm to dynamically allocate IRQ number for IOAPIC pins on demand as below: 1) Legacy(ISA) IRQs is not managed by irqdomain because there may be multiple pins sharing the same IRQ number and current irqdomain only supports 1:1 mapping between pins and IRQ. 2) Build identity mapping for GSIs between NR_IRQS_LEGACY and arch_dynirq_lower_bound(0). This is typically used to support legacy IRQs and simple platforms. 3) Dynamically allocate IRQs for GSIs above arch_dynirq_lower_bound(0). This may be used to support big system and IOAPIC hotplug. 4) Dynamically allocate IRQs for non-ISA IRQs below NR_IRQS_LEGACY if platform doens't request identity mapping. This is to support some really weired platforms. 5) Build identity mapping for GSIs below NR_IRQS_LEGACY if platform requests identity mapping. This is to support MID and CE41000 etc. Function arch_dynirq_lower_bound(0) will be enhanced in coming patch to enable dynamic IRQ allocation for IOAPIC. To ease our life, arch_dynirq_lower_bound(0) must be greater than or equal to NR_IRQS_LEGACY, otherwise it may break backward compatibilities. Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> --- arch/x86/Kconfig | 1 + arch/x86/include/asm/io_apic.h | 11 +++- arch/x86/kernel/acpi/boot.c | 8 +-- arch/x86/kernel/apic/io_apic.c | 142 ++++++++++++++++++++++++++++++---------- 4 files changed, 120 insertions(+), 42 deletions(-)