| Message ID | 1510848825-21965-2-git-send-email-vikas.cha.sajjan@hpe.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, 16 Nov 2017, Vikas C Sajjan wrote: > The platforms which support only IOAPIC mode, pass the SCI information > above the legacy space (0-15) via the FADT mechanism and not via MADT. > In such cases the mp_override_legacy_irq() used by acpi_sci_ioapic_setup() > to register SCI interrupts fails for interrupts >= 16, since it is meant to > handle only legacy space and throws error "Invalid bus_irq %u for legacy > override". Hence add a new function to handle SCI interrupts >= 16 and > invoke it conditionally in acpi_sci_ioapic_setup().The code duplication > due to this new function will be cleaned up in a separate patch. This reads way better, but I have a small nit pick. In the example I gave you there were multiple paragraphs on purpose to separate the different parts. So if I just split the above lump into separate paragraphs: [1] The platforms which support only IOAPIC mode, pass the SCI information above the legacy space (0-15) via the FADT mechanism and not via MADT. [2] In such cases the mp_override_legacy_irq() used by acpi_sci_ioapic_setup() to register SCI interrupts fails for interrupts >= 16, since it is meant to handle only legacy space and throws error "Invalid bus_irq %u for legacy override". [3] Hence add a new function to handle SCI interrupts >= 16 and invoke it conditionally in acpi_sci_ioapic_setup(). [4] The code duplication due to this new function will be cleaned up in a separate patch. then this is clearly structured: [1] describes the context. [2] describes the failure [3] describes the solution [4] is an extra note to tell the reviewer/reader that you are aware of the code duplication and this is addressed later. No need to resend. I can do that when picking it up. > Co-developed-by: Sunil V L <sunil.vl@hpe.com> I had a discussion with Greg about this tag which resulted in a patch so it should be soon part of the official documentation: https://lkml.kernel.org/r/20171116132309.GA8449@kroah.com We agreed that both authors should add their Signed-off-by to document that the work conforms with the Developer Certificate of Origin. I'll add that if that's ok for you. Thanks for following up! tglx -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 16 Nov 2017, Vikas C Sajjan wrote: > The platforms which support only IOAPIC mode, pass the SCI information > above the legacy space (0-15) via the FADT mechanism and not via MADT. > In such cases the mp_override_legacy_irq() used by > acpi_sci_ioapic_setup() to register SCI interrupts fails for > interrupts >= 16, since it is meant to handle only legacy space and > throws error "Invalid bus_irq %u for legacy override". Hence add a new > function to handle SCI interrupts >= 16 and invoke it conditionally in > acpi_sci_ioapic_setup().The code duplication due to this new function will be cleaned up in a separate patch. This reads way better, but I have a small nit pick. In the example I gave you there were multiple paragraphs on purpose to separate the different parts. So if I just split the above lump into separate paragraphs: [1] The platforms which support only IOAPIC mode, pass the SCI information above the legacy space (0-15) via the FADT mechanism and not via MADT. [2] In such cases the mp_override_legacy_irq() used by acpi_sci_ioapic_setup() to register SCI interrupts fails for interrupts >= 16, since it is meant to handle only legacy space and throws error "Invalid bus_irq %u for legacy override". [3] Hence add a new function to handle SCI interrupts >= 16 and invoke it conditionally in acpi_sci_ioapic_setup(). [4] The code duplication due to this new function will be cleaned up in a separate patch. then this is clearly structured: [1] describes the context. [2] describes the failure [3] describes the solution [4] is an extra note to tell the reviewer/reader that you are aware of the code duplication and this is addressed later. No need to resend. I can do that when picking it up. Thanks. > Co-developed-by: Sunil V L <sunil.vl@hpe.com> I had a discussion with Greg about this tag which resulted in a patch so it should be soon part of the official documentation: https://lkml.kernel.org/r/20171116132309.GA8449@kroah.com Great. Good to know that. We agreed that both authors should add their Signed-off-by to document that the work conforms with the Developer Certificate of Origin. I'll add that if that's ok for you. I am OK with that. Please go ahead. Thanks for following up! Thank you for the review. tglx -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" 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/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index ef9e02e..7615379 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -429,6 +429,34 @@ static int mp_config_acpi_gsi(struct device *dev, u32 gsi, int trigger, return 0; } +static int __init mp_register_ioapic_irq(u8 bus_irq, u8 polarity, + u8 trigger, u32 gsi) +{ + struct mpc_intsrc mp_irq; + int ioapic, pin; + + /* Convert 'gsi' to 'ioapic.pin'(INTIN#) */ + ioapic = mp_find_ioapic(gsi); + if (ioapic < 0) { + pr_warn("Failed to find ioapic for gsi : %u\n", gsi); + return ioapic; + } + + pin = mp_find_ioapic_pin(ioapic, gsi); + + mp_irq.type = MP_INTSRC; + mp_irq.irqtype = mp_INT; + mp_irq.irqflag = (trigger << 2) | polarity; + mp_irq.srcbus = MP_ISA_BUS; + mp_irq.srcbusirq = bus_irq; + mp_irq.dstapic = mpc_ioapic_id(ioapic); + mp_irq.dstirq = pin; + + mp_save_irq(&mp_irq); + + return 0; +} + static int __init acpi_parse_ioapic(struct acpi_subtable_header * header, const unsigned long end) { @@ -473,7 +501,11 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger, if (acpi_sci_flags & ACPI_MADT_POLARITY_MASK) polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK; - mp_override_legacy_irq(bus_irq, polarity, trigger, gsi); + if (bus_irq < NR_IRQS_LEGACY) + mp_override_legacy_irq(bus_irq, polarity, trigger, gsi); + else + mp_register_ioapic_irq(bus_irq, polarity, trigger, gsi); + acpi_penalize_sci_irq(bus_irq, trigger, polarity); /*