[PATCH V2 3/3] Revert "ACPI,PCI,IRQ: remove SCI penalize function"

[PATCH V2 3/3] Revert "ACPI,PCI,IRQ: remove SCI penalize function"

[Sinan Kaya]

The SCI function was removed in two steps (first refactor and then remove).
This patch does the revert in one step.

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. This patch does a partial
revert for the SCI functionality only.

The commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") is
for the removal of the function. SCI penalty API was replaced by the
runtime penalty calculation based on the value of
acpi_gbl_FADT.sci_interrupt.

The IRQ type does not get updated at the right time for some platforms and
results in incorrect penalty assignment for PCI IRQs as
irq_get_trigger_type returns the wrong type.

The register_gsi function delivers the IRQ found in the ACPI table to
the interrupt controller driver.  Penalties are calculated before a
link object is enabled to find out which interrupt has the least number
of users. By the time penalties are calculated, the IRQ is not registered
yet and the API returns the wrong type.

Tested-by: Jonathan Liu <net147@gmail.com>
Tested-by: Ondrej Zary <linux@rainbow-software.org>
Link: https://lkml.org/lkml/2016/10/4/283
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 arch/x86/kernel/acpi/boot.c |  1 +
 drivers/acpi/pci_link.c     | 34 ++++++++++++++--------------------
 include/linux/acpi.h        |  1 +
 3 files changed, 16 insertions(+), 20 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 06c2a11..6a2af19 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -495,27 +495,10 @@ static int acpi_irq_pci_sharing_penalty(int irq)
 
 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 < ACPI_MAX_ISA_IRQS)
-		return penalty + acpi_irq_penalty[irq];
+	if (irq < ACPI_MAX_IRQS)
+		return acpi_irq_penalty[irq];
 
-	penalty += acpi_irq_pci_sharing_penalty(irq);
-	return penalty;
+	return acpi_irq_pci_sharing_penalty(irq);
 }
 
 int __init acpi_irq_penalty_init(void)
@@ -886,6 +869,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)
+{
+	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
+		if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
+		    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
+			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
+		else
+			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
+	}
+}
+
 /*
  * 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 4d8452c..85ac7d5 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

Re: [PATCH V2 3/3] Revert "ACPI,PCI,IRQ: remove SCI penalize function"

[Thomas Gleixner]

On Tue, 4 Oct 2016, Sinan Kaya wrote:

> The SCI function was removed in two steps (first refactor and then remove).
> This patch does the revert in one step.
> 
> 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. This patch does a partial
> revert for the SCI functionality only.
> 
> The commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") is
> for the removal of the function. SCI penalty API was replaced by the
> runtime penalty calculation based on the value of
> acpi_gbl_FADT.sci_interrupt.
> 
> The IRQ type does not get updated at the right time for some platforms and
> results in incorrect penalty assignment for PCI IRQs as
> irq_get_trigger_type returns the wrong type.
> 
> The register_gsi function delivers the IRQ found in the ACPI table to
> the interrupt controller driver.  Penalties are calculated before a
> link object is enabled to find out which interrupt has the least number
> of users. By the time penalties are calculated, the IRQ is not registered
> yet and the API returns the wrong type.
> 
> Tested-by: Jonathan Liu <net147@gmail.com>
> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> Link: https://lkml.org/lkml/2016/10/4/283
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH V2 3/3] Revert "ACPI,PCI,IRQ: remove SCI penalize function"

[Bjorn Helgaas]

On Tue, Oct 04, 2016 at 05:15:19PM -0400, Sinan Kaya wrote:
> The SCI function was removed in two steps (first refactor and then remove).
> This patch does the revert in one step.
> 
> 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. This patch does a partial
> revert for the SCI functionality only.
> 
> The commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize function") is
> for the removal of the function. SCI penalty API was replaced by the
> runtime penalty calculation based on the value of
> acpi_gbl_FADT.sci_interrupt.
> 
> The IRQ type does not get updated at the right time for some platforms and
> results in incorrect penalty assignment for PCI IRQs as
> irq_get_trigger_type returns the wrong type.
> 
> The register_gsi function delivers the IRQ found in the ACPI table to
> the interrupt controller driver.  Penalties are calculated before a
> link object is enabled to find out which interrupt has the least number
> of users. By the time penalties are calculated, the IRQ is not registered
> yet and the API returns the wrong type.
> 
> Tested-by: Jonathan Liu <net147@gmail.com>
> Tested-by: Ondrej Zary <linux@rainbow-software.org>
> Link: https://lkml.org/lkml/2016/10/4/283
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  arch/x86/kernel/acpi/boot.c |  1 +
>  drivers/acpi/pci_link.c     | 34 ++++++++++++++--------------------
>  include/linux/acpi.h        |  1 +
>  3 files changed, 16 insertions(+), 20 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 06c2a11..6a2af19 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -495,27 +495,10 @@ static int acpi_irq_pci_sharing_penalty(int irq)
>  
>  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 < ACPI_MAX_ISA_IRQS)
> -		return penalty + acpi_irq_penalty[irq];
> +	if (irq < ACPI_MAX_IRQS)
> +		return acpi_irq_penalty[irq];
>  
> -	penalty += acpi_irq_pci_sharing_penalty(irq);
> -	return penalty;
> +	return acpi_irq_pci_sharing_penalty(irq);
>  }
>  
>  int __init acpi_irq_penalty_init(void)
> @@ -886,6 +869,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)
> +{
> +	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
> +		if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
> +		    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> +			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
> +		else
> +			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;

This would be a lot easier to read if the trigger/polarity tests were
positive, e.g.,

  if (trigger == ACPI_MADT_TRIGGER_LEVEL &&
      polarity == ACPI_MADT_POLARITY_ACTIVE_LOW)
    acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
  else
    acpi_irq_penalty[irq] += 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 4d8452c..85ac7d5 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
> 
> --
> 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