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

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

[Sinan Kaya]

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

acpi_gbl_FADT.sci_interrupt 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.

Link: http://www.spinics.net/lists/linux-pci/msg54599.html
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: [3/3] Revert "ACPI,PCI,IRQ: remove SCI penalize function"

[Jonathan Liu]

On 2 October 2016 at 04:46, Sinan Kaya <okaya@codeaurora.org> wrote:
> This reverts commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize
> function"). SCI penalty API was replaced by the runtime penalty calculation
> based on the value of acpi_gbl_FADT.sci_interrupt.
>
> acpi_gbl_FADT.sci_interrupt 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.
>
> Link: http://www.spinics.net/lists/linux-pci/msg54599.html
> 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);

This series fixes one or more network adapters in VirtualBox not
working with Linux 32-bit x86 guest 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>

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

[Thomas Gleixner]

On Sat, 1 Oct 2016, Sinan Kaya wrote:

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

This does more than only reverting said commit ....
 
> acpi_gbl_FADT.sci_interrupt 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.

And the obvious question is: Why does irq_get_trigger_type() return the
wrong type?

What's the root cause of this problem? Your changelog does not tell
anything.

Thanks,

	tglx

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

[Sinan Kaya]

On 10/4/2016 3:23 AM, Thomas Gleixner wrote:
> On Sat, 1 Oct 2016, Sinan Kaya wrote:
> 
>> This reverts commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize
>> function"). SCI penalty API was replaced by the runtime penalty calculation
>> based on the value of acpi_gbl_FADT.sci_interrupt.
> 
> This does more than only reverting said commit ....

The SCI function was removed in two steps (first refactor and then remove). 
I was trying to do the revert at one step. I can divide into two if it makes
it better.

>  
>> acpi_gbl_FADT.sci_interrupt 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.
> 
> And the obvious question is: Why does irq_get_trigger_type() return the
> wrong type?

Here is some history:

I now remember that Bjorn indicated the race condition possibility in this thread
here.

https://lkml.org/lkml/2016/3/8/640

My understanding is that 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 it returns the wrong type.

> 
> What's the root cause of this problem? Your changelog does not tell
> anything.

If you are OK with the above description, I can add this to the commit message.


> 
> 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
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Thomas Gleixner]

On Tue, 4 Oct 2016, Sinan Kaya wrote:

> On 10/4/2016 3:23 AM, Thomas Gleixner wrote:
> > On Sat, 1 Oct 2016, Sinan Kaya wrote:
> > 
> >> This reverts commit 9e5ed6d1fb87 ("ACPI,PCI,IRQ: remove SCI penalize
> >> function"). SCI penalty API was replaced by the runtime penalty calculation
> >> based on the value of acpi_gbl_FADT.sci_interrupt.
> > 
> > This does more than only reverting said commit ....
> 
> The SCI function was removed in two steps (first refactor and then remove). 
> I was trying to do the revert at one step. I can divide into two if it makes
> it better

No one step is fine. But this wants to be documented in the changelog.

> >> acpi_gbl_FADT.sci_interrupt 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.
> > 
> > And the obvious question is: Why does irq_get_trigger_type() return the
> > wrong type?
> 
> Here is some history:
> 
> I now remember that Bjorn indicated the race condition possibility in this thread
> here.
> 
> https://lkml.org/lkml/2016/3/8/640

> My understanding is that 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 it returns the wrong type.

Ok.

> > 
> > What's the root cause of this problem? Your changelog does not tell
> > anything.
> 
> If you are OK with the above description, I can add this to the commit message.

Yes please.
 
Thanks,

	tglx