| Message ID | 1476915664-27231-3-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Oct 19, 2016 at 06:21:03PM -0400, Sinan Kaya wrote: > The SCI penalize function was removed in two steps (first refactor > and then remove) and these changes are reverted here in one go. > > The commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > refactored the original code so that SCI penalty is calculated dynamically > by the time get_penalty function is called. That change is partially > reverted here, specifically for the SCI IRQ alone. > > The SCI penalize function was finally dropped by commit 9e5ed6d1fb87 > ("ACPI,PCI,IRQ: remove SCI penalize function") that replaced the old SCI > penalty API with penalty calculation carried out dynamically and based > on the acpi_gbl_FADT.sci_interrupt value. > > However, that new algorithm relies on the accurate setting of IRQ > types and that doesn't happen early enough on some platforms which > leads to incorrect penalty assignments for PCI IRQs. In those cases, > irq_get_trigger_type() returns incorrect values for the IRQs in > question, because they have not been registered yet by the time the > penalties are calculated. > > To fix this problem, we only need to fix the penalty for the SCI interrupt. > It seems better to add a single "sci_penalty" variable, set it to > PIRQ_PENALTY_PCI_USING if it's level/low or PIRQ_PENALTY_ISA_ALWAYS > otherwise, and add "sci_penalty" in when appropriate. That should fix it > for *any* SCI IRQ, not just those less than 256, and we don't have to add > these extra penalty table entries that are all unused (except possibly for > one entry if we have an SCI in the 16-255 range). > > For this reason, revert commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI > penalize function") completely to restore the correct behavior. I like this patch fine, except for the changelog. I don't think it's useful to describe this as a revert and give all the historical details. I think the important part is something like this: We previously used irq_get_trigger_type(irq) to help compute the penalty for the SCI, but that depends on the SCI having been registered already. Add acpi_penalize_sci_irq() so platforms can tell us the SCI IRQ, trigger, and polarity so we can compute the penalty even before the SCI has been registered. Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Link: https://lkml.org/lkml/2016/10/4/283 > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > Fixes: commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > Fixes: commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") "commit" is redundant; it's sufficient to say: Fixes: 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") In fact, I don't think you really need to include "commit" in the reference to 9e5ed6d1fb87 above either. > --- > arch/x86/kernel/acpi/boot.c | 1 + > drivers/acpi/pci_link.c | 30 +++++++++++++++--------------- > include/linux/acpi.h | 1 + > 3 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index 90d84c3..0ffd26e 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -453,6 +453,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger, > polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK; > > mp_override_legacy_irq(bus_irq, polarity, trigger, gsi); > + acpi_penalize_sci_irq(bus_irq, trigger, polarity); > > /* > * stash over-ride to indicate we've been here > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > index 4f37938..294b190 100644 > --- a/drivers/acpi/pci_link.c > +++ b/drivers/acpi/pci_link.c > @@ -87,6 +87,7 @@ struct acpi_pci_link { > > static LIST_HEAD(acpi_link_list); > static DEFINE_MUTEX(acpi_link_lock); > +static int sci_irq = -1, sci_penalty; > > /* -------------------------------------------------------------------------- > PCI Link Device Management > @@ -496,25 +497,13 @@ static int acpi_irq_get_penalty(int irq) > { > int penalty = 0; > > - /* > - * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict > - * with PCI IRQ attributes, mark ACPI SCI as ISA_ALWAYS so it won't be > - * use for PCI IRQs. > - */ > - if (irq == acpi_gbl_FADT.sci_interrupt) { > - u32 type = irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK; > - > - if (type != IRQ_TYPE_LEVEL_LOW) > - penalty += PIRQ_PENALTY_ISA_ALWAYS; > - else > - penalty += PIRQ_PENALTY_PCI_USING; > - } > + if (irq == sci_irq) > + penalty += sci_penalty; > > if (irq < ACPI_MAX_ISA_IRQS) > return penalty + acpi_isa_irq_penalty[irq]; > > - penalty += acpi_irq_pci_sharing_penalty(irq); > - return penalty; > + return penalty + acpi_irq_pci_sharing_penalty(irq); > } > > int __init acpi_irq_penalty_init(void) > @@ -881,6 +870,17 @@ bool acpi_isa_irq_available(int irq) > acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS); > } > > +void acpi_penalize_sci_irq(int irq, int trigger, int polarity) > +{ > + sci_irq = irq; > + > + if (trigger == ACPI_MADT_TRIGGER_LEVEL && > + polarity == ACPI_MADT_POLARITY_ACTIVE_LOW) > + sci_penalty = PIRQ_PENALTY_PCI_USING; > + else > + sci_penalty = PIRQ_PENALTY_ISA_ALWAYS; > +} > + > /* > * Over-ride default table to reserve additional IRQs for use by ISA > * e.g. acpi_irq_isa=5 > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index c5eaf2f..67d1d3e 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -318,6 +318,7 @@ struct pci_dev; > int acpi_pci_irq_enable (struct pci_dev *dev); > void acpi_penalize_isa_irq(int irq, int active); > bool acpi_isa_irq_available(int irq); > +void acpi_penalize_sci_irq(int irq, int trigger, int polarity); > void acpi_pci_irq_disable (struct pci_dev *dev); > > extern int ec_read(u8 addr, u8 *val); > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 20, 2016 at 08:58:14PM -0500, Bjorn Helgaas wrote: > On Wed, Oct 19, 2016 at 06:21:03PM -0400, Sinan Kaya wrote: > > The SCI penalize function was removed in two steps (first refactor > > and then remove) and these changes are reverted here in one go. > > > > The commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > > refactored the original code so that SCI penalty is calculated dynamically > > by the time get_penalty function is called. That change is partially > > reverted here, specifically for the SCI IRQ alone. > > > > The SCI penalize function was finally dropped by commit 9e5ed6d1fb87 > > ("ACPI,PCI,IRQ: remove SCI penalize function") that replaced the old SCI > > penalty API with penalty calculation carried out dynamically and based > > on the acpi_gbl_FADT.sci_interrupt value. > > > > However, that new algorithm relies on the accurate setting of IRQ > > types and that doesn't happen early enough on some platforms which > > leads to incorrect penalty assignments for PCI IRQs. In those cases, > > irq_get_trigger_type() returns incorrect values for the IRQs in > > question, because they have not been registered yet by the time the > > penalties are calculated. > > > > To fix this problem, we only need to fix the penalty for the SCI interrupt. > > It seems better to add a single "sci_penalty" variable, set it to > > PIRQ_PENALTY_PCI_USING if it's level/low or PIRQ_PENALTY_ISA_ALWAYS > > otherwise, and add "sci_penalty" in when appropriate. That should fix it > > for *any* SCI IRQ, not just those less than 256, and we don't have to add > > these extra penalty table entries that are all unused (except possibly for > > one entry if we have an SCI in the 16-255 range). > > > > For this reason, revert commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI > > penalize function") completely to restore the correct behavior. > > I like this patch fine, except for the changelog. I don't think it's > useful to describe this as a revert and give all the historical > details. I think the important part is something like this: > > We previously used irq_get_trigger_type(irq) to help compute the > penalty for the SCI, but that depends on the SCI having been > registered already. Add acpi_penalize_sci_irq() so platforms can > tell us the SCI IRQ, trigger, and polarity so we can compute the > penalty even before the SCI has been registered. Thanks for the pointer [1] to the issues around acpi_penalize_sci_irq() being called before kmalloc() is available. That begs the question of exactly *why* we need acpi_penalize_sci_irq() so early -- I doubt we actually need to handle SCIs that early, and obviously we don't need to look up penalties that early (because we only look them up when enabling interrupt links, which happens much later). [1] http://marc.info/?l=linux-acpi&m=145580159209240&w=2) > Acked-by: Bjorn Helgaas <bhelgaas@google.com> Wait a minute, I still have a question here: what about other ACPI arches (ia64, arm64)? Don't they need to call acpi_penalize_sci_irq() somewhere? > > --- > > arch/x86/kernel/acpi/boot.c | 1 + > > drivers/acpi/pci_link.c | 30 +++++++++++++++--------------- > > include/linux/acpi.h | 1 + > > 3 files changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > > index 90d84c3..0ffd26e 100644 > > --- a/arch/x86/kernel/acpi/boot.c > > +++ b/arch/x86/kernel/acpi/boot.c > > @@ -453,6 +453,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger, > > polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK; > > > > mp_override_legacy_irq(bus_irq, polarity, trigger, gsi); > > + acpi_penalize_sci_irq(bus_irq, trigger, polarity); > > > > /* > > * stash over-ride to indicate we've been here > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > > index 4f37938..294b190 100644 > > --- a/drivers/acpi/pci_link.c > > +++ b/drivers/acpi/pci_link.c > > @@ -87,6 +87,7 @@ struct acpi_pci_link { > > > > static LIST_HEAD(acpi_link_list); > > static DEFINE_MUTEX(acpi_link_lock); > > +static int sci_irq = -1, sci_penalty; > > > > /* -------------------------------------------------------------------------- > > PCI Link Device Management > > @@ -496,25 +497,13 @@ static int acpi_irq_get_penalty(int irq) > > { > > int penalty = 0; > > > > - /* > > - * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict > > - * with PCI IRQ attributes, mark ACPI SCI as ISA_ALWAYS so it won't be > > - * use for PCI IRQs. > > - */ > > - if (irq == acpi_gbl_FADT.sci_interrupt) { > > - u32 type = irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK; > > - > > - if (type != IRQ_TYPE_LEVEL_LOW) > > - penalty += PIRQ_PENALTY_ISA_ALWAYS; > > - else > > - penalty += PIRQ_PENALTY_PCI_USING; > > - } > > + if (irq == sci_irq) > > + penalty += sci_penalty; > > > > if (irq < ACPI_MAX_ISA_IRQS) > > return penalty + acpi_isa_irq_penalty[irq]; > > > > - penalty += acpi_irq_pci_sharing_penalty(irq); > > - return penalty; > > + return penalty + acpi_irq_pci_sharing_penalty(irq); > > } > > > > int __init acpi_irq_penalty_init(void) > > @@ -881,6 +870,17 @@ bool acpi_isa_irq_available(int irq) > > acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS); > > } > > > > +void acpi_penalize_sci_irq(int irq, int trigger, int polarity) > > +{ > > + sci_irq = irq; > > + > > + if (trigger == ACPI_MADT_TRIGGER_LEVEL && > > + polarity == ACPI_MADT_POLARITY_ACTIVE_LOW) > > + sci_penalty = PIRQ_PENALTY_PCI_USING; > > + else > > + sci_penalty = PIRQ_PENALTY_ISA_ALWAYS; > > +} > > + > > /* > > * Over-ride default table to reserve additional IRQs for use by ISA > > * e.g. acpi_irq_isa=5 > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index c5eaf2f..67d1d3e 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -318,6 +318,7 @@ struct pci_dev; > > int acpi_pci_irq_enable (struct pci_dev *dev); > > void acpi_penalize_isa_irq(int irq, int active); > > bool acpi_isa_irq_available(int irq); > > +void acpi_penalize_sci_irq(int irq, int trigger, int polarity); > > void acpi_pci_irq_disable (struct pci_dev *dev); > > > > extern int ec_read(u8 addr, u8 *val); > > -- > > 1.9.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2016 7:45 AM, Bjorn Helgaas wrote: > [1] http://marc.info/?l=linux-acpi&m=145580159209240&w=2) > >> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Wait a minute, I still have a question here: what about other ACPI > arches (ia64, arm64)? Don't they need to call acpi_penalize_sci_irq() > somewhere? > ACPI ARM64 architecture implements reduced ACPI profile which doesn't have GED object. Instead, ARM64 architecture uses onchip peripherals for similar functionality. If there is a need to signal interrupts, this is done by ACPI Notify in ASL or if absolutely needed using ACPI Generic Event Device (GED).
On Fri, Oct 21, 2016 at 09:13:06AM -0700, Sinan Kaya wrote: > On 10/21/2016 7:45 AM, Bjorn Helgaas wrote: > > [1] http://marc.info/?l=linux-acpi&m=145580159209240&w=2) > > > >> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > Wait a minute, I still have a question here: what about other ACPI > > arches (ia64, arm64)? Don't they need to call acpi_penalize_sci_irq() > > somewhere? > > > > ACPI ARM64 architecture implements reduced ACPI profile which doesn't > have GED object. Instead, ARM64 architecture uses onchip peripherals > for similar functionality. If there is a need to signal interrupts, > this is done by ACPI Notify in ASL or if absolutely needed using > ACPI Generic Event Device (GED). OK. I guess ia64 never did call acpi_penalize_sci_irq(), so while it could be added someday to unify things, we don't need to add it now. Same for arm64. So I'd like it if you updated the changelog, but I'm OK with the patch: Acked-by: Bjorn Helgaas <bhelgaas@google.com> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 20, 2016 at 08:58:14PM -0500, Bjorn Helgaas wrote: > On Wed, Oct 19, 2016 at 06:21:03PM -0400, Sinan Kaya wrote: > > The SCI penalize function was removed in two steps (first refactor > > and then remove) and these changes are reverted here in one go. > > > > The commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > > refactored the original code so that SCI penalty is calculated dynamically > > by the time get_penalty function is called. That change is partially > > reverted here, specifically for the SCI IRQ alone. > > > > The SCI penalize function was finally dropped by commit 9e5ed6d1fb87 > > ("ACPI,PCI,IRQ: remove SCI penalize function") that replaced the old SCI > > penalty API with penalty calculation carried out dynamically and based > > on the acpi_gbl_FADT.sci_interrupt value. > > > > However, that new algorithm relies on the accurate setting of IRQ > > types and that doesn't happen early enough on some platforms which > > leads to incorrect penalty assignments for PCI IRQs. In those cases, > > irq_get_trigger_type() returns incorrect values for the IRQs in > > question, because they have not been registered yet by the time the > > penalties are calculated. > > > > To fix this problem, we only need to fix the penalty for the SCI interrupt. > > It seems better to add a single "sci_penalty" variable, set it to > > PIRQ_PENALTY_PCI_USING if it's level/low or PIRQ_PENALTY_ISA_ALWAYS > > otherwise, and add "sci_penalty" in when appropriate. That should fix it > > for *any* SCI IRQ, not just those less than 256, and we don't have to add > > these extra penalty table entries that are all unused (except possibly for > > one entry if we have an SCI in the 16-255 range). > > > > For this reason, revert commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI > > penalize function") completely to restore the correct behavior. > > I like this patch fine, except for the changelog. I don't think it's > useful to describe this as a revert and give all the historical > details. I think the important part is something like this: > > We previously used irq_get_trigger_type(irq) to help compute the > penalty for the SCI, but that depends on the SCI having been > registered already. Add acpi_penalize_sci_irq() so platforms can > tell us the SCI IRQ, trigger, and polarity so we can compute the > penalty even before the SCI has been registered. I think this is actually the critical patch that fixes the regression reported by Ondrej, so the changelog really should include details about the regression, e.g., something like this: ACPI: pci_link: Penalize SCI correctly Ondrej reported that IRQs stopped working in v4.7 on several platforms. A typical scenario, from Ondrej's VT82C694X/694X, is: ACPI: Using PIC for interrupt routing ACPI: PCI Interrupt Link [LNKA] (IRQs 1 3 4 5 6 7 10 *11 12 14 15) ACPI: No IRQ available for PCI Interrupt Link [LNKA] 8139too 0000:00:0f.0: PCI INT A: no GSI We're using PIC routing, so acpi_irq_balance == 0, and LNKA is already active at IRQ 11. In that case, acpi_pci_link_allocate() only tries to use the active IRQ (IRQ 11) which also happens to be the SCI. We should penalize the SCI by PIRQ_PENALTY_PCI_USING, but irq_get_trigger_type(11) returns something other than IRQ_TYPE_LEVEL_LOW, so we penalize it by PIRQ_PENALTY_ISA_ALWAYS instead, which makes acpi_pci_link_allocate() assume the IRQ isn't available and give up. Add acpi_penalize_sci_irq() so platforms can tell us the SCI IRQ, trigger, and polarity directly and we don't have to depend on irq_get_trigger_type(). Link: http://lkml.kernel.org/r/201609251512.05657.linux@rainbow-software.org Reported-by: Ondrej Zary <linux@rainbow-software.org> I don't understand what's broken about using irq_get_trigger_type(). The SCI is registered in the following path, which is certainly before we try to enable LNKA for the 8139too device: acpi_init # subsys_initcall acpi_bus_init acpi_enable_subsystem acpi_ev_install_xrupt_handlers acpi_ev_install_sci_handler acpi_os_install_interrupt_handler(sci_interrupt, ...) acpi_gsi_to_irq acpi_register_gsi irq_create_fwspec_mapping irqd_set_trigger_type In any event, I think this patch to add acpi_penalize_sci_irq() is fine. > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > Link: https://lkml.org/lkml/2016/10/4/283 > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > > Fixes: commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > > Fixes: commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") > > "commit" is redundant; it's sufficient to say: > > Fixes: 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > > In fact, I don't think you really need to include "commit" in the > reference to 9e5ed6d1fb87 above either. > > > --- > > arch/x86/kernel/acpi/boot.c | 1 + > > drivers/acpi/pci_link.c | 30 +++++++++++++++--------------- > > include/linux/acpi.h | 1 + > > 3 files changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > > index 90d84c3..0ffd26e 100644 > > --- a/arch/x86/kernel/acpi/boot.c > > +++ b/arch/x86/kernel/acpi/boot.c > > @@ -453,6 +453,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger, > > polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK; > > > > mp_override_legacy_irq(bus_irq, polarity, trigger, gsi); > > + acpi_penalize_sci_irq(bus_irq, trigger, polarity); > > > > /* > > * stash over-ride to indicate we've been here > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > > index 4f37938..294b190 100644 > > --- a/drivers/acpi/pci_link.c > > +++ b/drivers/acpi/pci_link.c > > @@ -87,6 +87,7 @@ struct acpi_pci_link { > > > > static LIST_HEAD(acpi_link_list); > > static DEFINE_MUTEX(acpi_link_lock); > > +static int sci_irq = -1, sci_penalty; > > > > /* -------------------------------------------------------------------------- > > PCI Link Device Management > > @@ -496,25 +497,13 @@ static int acpi_irq_get_penalty(int irq) > > { > > int penalty = 0; > > > > - /* > > - * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict > > - * with PCI IRQ attributes, mark ACPI SCI as ISA_ALWAYS so it won't be > > - * use for PCI IRQs. > > - */ > > - if (irq == acpi_gbl_FADT.sci_interrupt) { > > - u32 type = irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK; > > - > > - if (type != IRQ_TYPE_LEVEL_LOW) > > - penalty += PIRQ_PENALTY_ISA_ALWAYS; > > - else > > - penalty += PIRQ_PENALTY_PCI_USING; > > - } > > + if (irq == sci_irq) > > + penalty += sci_penalty; > > > > if (irq < ACPI_MAX_ISA_IRQS) > > return penalty + acpi_isa_irq_penalty[irq]; > > > > - penalty += acpi_irq_pci_sharing_penalty(irq); > > - return penalty; > > + return penalty + acpi_irq_pci_sharing_penalty(irq); > > } > > > > int __init acpi_irq_penalty_init(void) > > @@ -881,6 +870,17 @@ bool acpi_isa_irq_available(int irq) > > acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS); > > } > > > > +void acpi_penalize_sci_irq(int irq, int trigger, int polarity) > > +{ > > + sci_irq = irq; > > + > > + if (trigger == ACPI_MADT_TRIGGER_LEVEL && > > + polarity == ACPI_MADT_POLARITY_ACTIVE_LOW) > > + sci_penalty = PIRQ_PENALTY_PCI_USING; > > + else > > + sci_penalty = PIRQ_PENALTY_ISA_ALWAYS; > > +} > > + > > /* > > * Over-ride default table to reserve additional IRQs for use by ISA > > * e.g. acpi_irq_isa=5 > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index c5eaf2f..67d1d3e 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -318,6 +318,7 @@ struct pci_dev; > > int acpi_pci_irq_enable (struct pci_dev *dev); > > void acpi_penalize_isa_irq(int irq, int active); > > bool acpi_isa_irq_available(int irq); > > +void acpi_penalize_sci_irq(int irq, int trigger, int polarity); > > void acpi_pci_irq_disable (struct pci_dev *dev); > > > > extern int ec_read(u8 addr, u8 *val); > > -- > > 1.9.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 October 2016 at 09:21, Sinan Kaya <okaya@codeaurora.org> wrote: > The SCI penalize function was removed in two steps (first refactor > and then remove) and these changes are reverted here in one go. > > The commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > refactored the original code so that SCI penalty is calculated dynamically > by the time get_penalty function is called. That change is partially > reverted here, specifically for the SCI IRQ alone. > > The SCI penalize function was finally dropped by commit 9e5ed6d1fb87 > ("ACPI,PCI,IRQ: remove SCI penalize function") that replaced the old SCI > penalty API with penalty calculation carried out dynamically and based > on the acpi_gbl_FADT.sci_interrupt value. > > However, that new algorithm relies on the accurate setting of IRQ > types and that doesn't happen early enough on some platforms which > leads to incorrect penalty assignments for PCI IRQs. In those cases, > irq_get_trigger_type() returns incorrect values for the IRQs in > question, because they have not been registered yet by the time the > penalties are calculated. > > To fix this problem, we only need to fix the penalty for the SCI interrupt. > It seems better to add a single "sci_penalty" variable, set it to > PIRQ_PENALTY_PCI_USING if it's level/low or PIRQ_PENALTY_ISA_ALWAYS > otherwise, and add "sci_penalty" in when appropriate. That should fix it > for *any* SCI IRQ, not just those less than 256, and we don't have to add > these extra penalty table entries that are all unused (except possibly for > one entry if we have an SCI in the 16-255 range). > > For this reason, revert commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI > penalize function") completely to restore the correct behavior. > > Link: https://lkml.org/lkml/2016/10/4/283 > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > Fixes: commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > Fixes: commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") > --- > arch/x86/kernel/acpi/boot.c | 1 + > drivers/acpi/pci_link.c | 30 +++++++++++++++--------------- > include/linux/acpi.h | 1 + > 3 files changed, 17 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index 90d84c3..0ffd26e 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -453,6 +453,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger, > polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK; > > mp_override_legacy_irq(bus_irq, polarity, trigger, gsi); > + acpi_penalize_sci_irq(bus_irq, trigger, polarity); > > /* > * stash over-ride to indicate we've been here > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > index 4f37938..294b190 100644 > --- a/drivers/acpi/pci_link.c > +++ b/drivers/acpi/pci_link.c > @@ -87,6 +87,7 @@ struct acpi_pci_link { > > static LIST_HEAD(acpi_link_list); > static DEFINE_MUTEX(acpi_link_lock); > +static int sci_irq = -1, sci_penalty; > > /* -------------------------------------------------------------------------- > PCI Link Device Management > @@ -496,25 +497,13 @@ static int acpi_irq_get_penalty(int irq) > { > int penalty = 0; > > - /* > - * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict > - * with PCI IRQ attributes, mark ACPI SCI as ISA_ALWAYS so it won't be > - * use for PCI IRQs. > - */ > - if (irq == acpi_gbl_FADT.sci_interrupt) { > - u32 type = irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK; > - > - if (type != IRQ_TYPE_LEVEL_LOW) > - penalty += PIRQ_PENALTY_ISA_ALWAYS; > - else > - penalty += PIRQ_PENALTY_PCI_USING; > - } > + if (irq == sci_irq) > + penalty += sci_penalty; > > if (irq < ACPI_MAX_ISA_IRQS) > return penalty + acpi_isa_irq_penalty[irq]; > > - penalty += acpi_irq_pci_sharing_penalty(irq); > - return penalty; > + return penalty + acpi_irq_pci_sharing_penalty(irq); > } > > int __init acpi_irq_penalty_init(void) > @@ -881,6 +870,17 @@ bool acpi_isa_irq_available(int irq) > acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS); > } > > +void acpi_penalize_sci_irq(int irq, int trigger, int polarity) > +{ > + sci_irq = irq; > + > + if (trigger == ACPI_MADT_TRIGGER_LEVEL && > + polarity == ACPI_MADT_POLARITY_ACTIVE_LOW) > + sci_penalty = PIRQ_PENALTY_PCI_USING; > + else > + sci_penalty = PIRQ_PENALTY_ISA_ALWAYS; > +} > + > /* > * Over-ride default table to reserve additional IRQs for use by ISA > * e.g. acpi_irq_isa=5 > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index c5eaf2f..67d1d3e 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -318,6 +318,7 @@ struct pci_dev; > int acpi_pci_irq_enable (struct pci_dev *dev); > void acpi_penalize_isa_irq(int irq, int active); > bool acpi_isa_irq_available(int irq); > +void acpi_penalize_sci_irq(int irq, int trigger, int polarity); > void acpi_pci_irq_disable (struct pci_dev *dev); > > extern int ec_read(u8 addr, u8 *val); This series fixes one or more network adapters not working in Linux 32-bit x86 guest running inside VirtualBox if I have 4 network adapters enabled. The following message no longer appears in the kernel log: ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or acpi=off Tested-by: Jonathan Liu <net147@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2016 12:13 PM, Sinan Kaya wrote: >> Wait a minute, I still have a question here: what about other ACPI >> > arches (ia64, arm64)? Don't they need to call acpi_penalize_sci_irq() >> > somewhere? >> > > ACPI ARM64 architecture implements reduced ACPI profile which doesn't > have GED object. I actually wanted to mean that ARM64 architecture doesn't have *GPE* object implemented on ACPI reduced profile.
On 10/20/2016 9:58 PM, Bjorn Helgaas wrote: > I like this patch fine, except for the changelog. I don't think it's > useful to describe this as a revert and give all the historical > details. I think the important part is something like this: > > We previously used irq_get_trigger_type(irq) to help compute the > penalty for the SCI, but that depends on the SCI having been > registered already. Add acpi_penalize_sci_irq() so platforms can > tell us the SCI IRQ, trigger, and polarity so we can compute the > penalty even before the SCI has been registered. OK, will replace with this and also change the commit summary as "ACPI,PCI,IRQ: save SCI IRQ details for runtime penalty calculation" > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > >> > Link: https://lkml.org/lkml/2016/10/4/283 >> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> >> > Fixes: commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") >> > Fixes: commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") > "commit" is redundant; it's sufficient to say: OK. I have been fighting with checkpatch. Checkpatch doesn't like a commit summary without "commit 12 char SHA" > > Fixes: 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") > > In fact, I don't think you really need to include "commit" in the > reference to 9e5ed6d1fb87 above either. >
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index 90d84c3..0ffd26e 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -453,6 +453,7 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger, polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK; mp_override_legacy_irq(bus_irq, polarity, trigger, gsi); + acpi_penalize_sci_irq(bus_irq, trigger, polarity); /* * stash over-ride to indicate we've been here diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c index 4f37938..294b190 100644 --- a/drivers/acpi/pci_link.c +++ b/drivers/acpi/pci_link.c @@ -87,6 +87,7 @@ struct acpi_pci_link { static LIST_HEAD(acpi_link_list); static DEFINE_MUTEX(acpi_link_lock); +static int sci_irq = -1, sci_penalty; /* -------------------------------------------------------------------------- PCI Link Device Management @@ -496,25 +497,13 @@ static int acpi_irq_get_penalty(int irq) { int penalty = 0; - /* - * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict - * with PCI IRQ attributes, mark ACPI SCI as ISA_ALWAYS so it won't be - * use for PCI IRQs. - */ - if (irq == acpi_gbl_FADT.sci_interrupt) { - u32 type = irq_get_trigger_type(irq) & IRQ_TYPE_SENSE_MASK; - - if (type != IRQ_TYPE_LEVEL_LOW) - penalty += PIRQ_PENALTY_ISA_ALWAYS; - else - penalty += PIRQ_PENALTY_PCI_USING; - } + if (irq == sci_irq) + penalty += sci_penalty; if (irq < ACPI_MAX_ISA_IRQS) return penalty + acpi_isa_irq_penalty[irq]; - penalty += acpi_irq_pci_sharing_penalty(irq); - return penalty; + return penalty + acpi_irq_pci_sharing_penalty(irq); } int __init acpi_irq_penalty_init(void) @@ -881,6 +870,17 @@ bool acpi_isa_irq_available(int irq) acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS); } +void acpi_penalize_sci_irq(int irq, int trigger, int polarity) +{ + sci_irq = irq; + + if (trigger == ACPI_MADT_TRIGGER_LEVEL && + polarity == ACPI_MADT_POLARITY_ACTIVE_LOW) + sci_penalty = PIRQ_PENALTY_PCI_USING; + else + sci_penalty = PIRQ_PENALTY_ISA_ALWAYS; +} + /* * Over-ride default table to reserve additional IRQs for use by ISA * e.g. acpi_irq_isa=5 diff --git a/include/linux/acpi.h b/include/linux/acpi.h index c5eaf2f..67d1d3e 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -318,6 +318,7 @@ struct pci_dev; int acpi_pci_irq_enable (struct pci_dev *dev); void acpi_penalize_isa_irq(int irq, int active); bool acpi_isa_irq_available(int irq); +void acpi_penalize_sci_irq(int irq, int trigger, int polarity); void acpi_pci_irq_disable (struct pci_dev *dev); extern int ec_read(u8 addr, u8 *val);
The SCI penalize function was removed in two steps (first refactor and then remove) and these changes are reverted here in one go. The commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") refactored the original code so that SCI penalty is calculated dynamically by the time get_penalty function is called. That change is partially reverted here, specifically for the SCI IRQ alone. The SCI penalize function was finally dropped by commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") that replaced the old SCI penalty API with penalty calculation carried out dynamically and based on the acpi_gbl_FADT.sci_interrupt value. However, that new algorithm relies on the accurate setting of IRQ types and that doesn't happen early enough on some platforms which leads to incorrect penalty assignments for PCI IRQs. In those cases, irq_get_trigger_type() returns incorrect values for the IRQs in question, because they have not been registered yet by the time the penalties are calculated. To fix this problem, we only need to fix the penalty for the SCI interrupt. It seems better to add a single "sci_penalty" variable, set it to PIRQ_PENALTY_PCI_USING if it's level/low or PIRQ_PENALTY_ISA_ALWAYS otherwise, and add "sci_penalty" in when appropriate. That should fix it for *any* SCI IRQ, not just those less than 256, and we don't have to add these extra penalty table entries that are all unused (except possibly for one entry if we have an SCI in the 16-255 range). For this reason, revert commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") completely to restore the correct behavior. Link: https://lkml.org/lkml/2016/10/4/283 Signed-off-by: Sinan Kaya <okaya@codeaurora.org> Fixes: commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") Fixes: commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") --- arch/x86/kernel/acpi/boot.c | 1 + drivers/acpi/pci_link.c | 30 +++++++++++++++--------------- include/linux/acpi.h | 1 + 3 files changed, 17 insertions(+), 15 deletions(-)