[PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible

[PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible

[Sinan Kaya]

The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
resource requirements") omitted the initially assigned POSSIBLE penalty
when the IRQ is active.

The original code would assign the POSSIBLE value divided by the number
of possible IRQs during initialization.

Later, if the IRQ is chosen as the active IRQ or if the IRQ is in use
by ISA; additional penalties get added.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/acpi/pci_link.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 8fc7323..f2b69e3 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -470,6 +470,7 @@ static int acpi_irq_pci_sharing_penalty(int irq)
 {
 	struct acpi_pci_link *link;
 	int penalty = 0;
+	int i;
 
 	list_for_each_entry(link, &acpi_link_list, list) {
 		/*
@@ -478,18 +479,14 @@ static int acpi_irq_pci_sharing_penalty(int irq)
 		 */
 		if (link->irq.active && link->irq.active == irq)
 			penalty += PIRQ_PENALTY_PCI_USING;
-		else {
-			int i;
-
-			/*
-			 * If a link is inactive, penalize the IRQs it
-			 * might use, but not as severely.
-			 */
-			for (i = 0; i < link->irq.possible_count; i++)
-				if (link->irq.possible[i] == irq)
-					penalty += PIRQ_PENALTY_PCI_POSSIBLE /
-						link->irq.possible_count;
-		}
+
+		/*
+		 * penalize the IRQs PCI might use, but not as severely.
+		 */
+		for (i = 0; i < link->irq.possible_count; i++)
+			if (link->irq.possible[i] == irq)
+				penalty += PIRQ_PENALTY_PCI_POSSIBLE /
+					link->irq.possible_count;
 	}
 
 	return penalty;
-- 
1.8.2.1

[PATCH V2 2/4] Revert "ACPI,PCI,IRQ: remove redundant code in acpi_irq_penalty_init()"

[Sinan Kaya]

Trying to make the ISA and PCI init functionality common turned out to be
a bad idea. ISA path depends on external functionality. Restoring the
previous behavior and limiting the refactoring to PCI interrupts only.

This reverts commit 1fcb6a813c4f ("ACPI,PCI,IRQ: remove redundant code in
acpi_irq_penalty_init()").

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 arch/x86/pci/acpi.c         |  1 +
 drivers/acpi/pci_link.c     | 36 ++++++++++++++++++++++++++++++++++++
 include/acpi/acpi_drivers.h |  1 +
 3 files changed, 38 insertions(+)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index b2a4e2a..3cd6983 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -396,6 +396,7 @@ int __init pci_acpi_init(void)
 		return -ENODEV;
 
 	printk(KERN_INFO "PCI: Using ACPI for IRQ routing\n");
+	acpi_irq_penalty_init();
 	pcibios_enable_irq = acpi_pci_irq_enable;
 	pcibios_disable_irq = acpi_pci_irq_disable;
 	x86_init.pci.init_irq = x86_init_noop;
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index f2b69e3..714ba4d 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -517,6 +517,42 @@ static int acpi_irq_get_penalty(int irq)
 	return penalty;
 }
 
+int __init acpi_irq_penalty_init(void)
+{
+	struct acpi_pci_link *link;
+	int i;
+
+	/*
+	 * Update penalties to facilitate IRQ balancing.
+	 */
+	list_for_each_entry(link, &acpi_link_list, list) {
+
+		/*
+		 * reflect the possible and active irqs in the penalty table --
+		 * useful for breaking ties.
+		 */
+		if (link->irq.possible_count) {
+			int penalty =
+			    PIRQ_PENALTY_PCI_POSSIBLE /
+			    link->irq.possible_count;
+
+			for (i = 0; i < link->irq.possible_count; i++) {
+				if (link->irq.possible[i] < ACPI_MAX_ISA_IRQS)
+					acpi_isa_irq_penalty[link->irq.
+							 possible[i]] +=
+					    penalty;
+			}
+
+		} else if (link->irq.active &&
+				(link->irq.active < ACPI_MAX_ISA_IRQS)) {
+			acpi_isa_irq_penalty[link->irq.active] +=
+			    PIRQ_PENALTY_PCI_POSSIBLE;
+		}
+	}
+
+	return 0;
+}
+
 static int acpi_irq_balance = -1;	/* 0: static, 1: balance */
 
 static int acpi_pci_link_allocate(struct acpi_pci_link *link)
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index 797ae2e..29c6912 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -78,6 +78,7 @@
 
 /* ACPI PCI Interrupt Link (pci_link.c) */
 
+int acpi_irq_penalty_init(void);
 int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
 			       int *polarity, char **name);
 int acpi_pci_link_free_irq(acpi_handle handle);
-- 
1.8.2.1

[PATCH V2 3/4] ACPI,PCI,IRQ: separate ISA penalty calculation

[Sinan Kaya]

Since commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements")
the penalty values are calculated on the fly rather than boot time.

This works fine for PCI interrupts but not so well for the ISA interrupts.
Whether an ISA interrupt is in use or not information is not available
inside the pci_link.c file. This information gets sent externally via
acpi_penalize_isa_irq function. If active is true, then the IRQ is in use
by ISA. Otherwise, IRQ is in use by PCI.

Since the current code relies on PCI Link object for determination of
penalties, we are factoring in the PCI penalty twice after
acpi_penalize_isa_irq function is called.

This change is limiting the newly added functionality to just PCI
interrupts so that old behavior is still maintained.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/acpi/pci_link.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 714ba4d..8c08971 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -496,9 +496,6 @@ static int acpi_irq_get_penalty(int irq)
 {
 	int penalty = 0;
 
-	if (irq < ACPI_MAX_ISA_IRQS)
-		penalty += acpi_isa_irq_penalty[irq];
-
 	/*
 	* 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
@@ -513,6 +510,9 @@ static int acpi_irq_get_penalty(int irq)
 			penalty += PIRQ_PENALTY_PCI_USING;
 	}
 
+	if (irq < ACPI_MAX_ISA_IRQS)
+		return penalty + acpi_isa_irq_penalty[irq];
+
 	penalty += acpi_irq_pci_sharing_penalty(irq);
 	return penalty;
 }
-- 
1.8.2.1

[PATCH V2 4/4] ACPI,PCI,IRQ: correct operator precedence

[Sinan Kaya]

The omitted parenthesis prevents the addition operation when
acpi_penalize_isa_irq function is called.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/acpi/pci_link.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 8c08971..c983bf7 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -872,7 +872,7 @@ void acpi_penalize_isa_irq(int irq, int active)
 {
 	if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty)))
 		acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
-			active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING;
+		  (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
 }
 
 bool acpi_isa_irq_available(int irq)
-- 
1.8.2.1

Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible

[Rafael J. Wysocki]

On Wed, Jun 29, 2016 at 10:27 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") omitted the initially assigned POSSIBLE penalty
> when the IRQ is active.

It would be good to say what can go wrong with that here.

> The original code would assign the POSSIBLE value divided by the number
> of possible IRQs during initialization.
>
> Later, if the IRQ is chosen as the active IRQ or if the IRQ is in use
> by ISA; additional penalties get added.

Thanks,
Rafael

Re: [PATCH V2 4/4] ACPI,PCI,IRQ: correct operator precedence

[Rafael J. Wysocki]

On Wed, Jun 29, 2016 at 10:27 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
> The omitted parenthesis prevents the addition operation when
> acpi_penalize_isa_irq function is called.
>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>

Well, this is a rather obvious one, so I'm wondering why it is the
last one in the series?

> ---
>  drivers/acpi/pci_link.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 8c08971..c983bf7 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -872,7 +872,7 @@ void acpi_penalize_isa_irq(int irq, int active)
>  {
>         if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_isa_irq_penalty)))
>                 acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> -                       active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING;
> +                 (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
>  }
>
>  bool acpi_isa_irq_available(int irq)
> --

Thanks,
Rafael

Re: [PATCH V2 4/4] ACPI,PCI,IRQ: correct operator precedence

[Sinan Kaya]

On 6/29/2016 9:16 AM, Rafael J. Wysocki wrote:
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Well, this is a rather obvious one, so I'm wondering why it is the
> last one in the series?
> 

The first three are more relevant to each other. It makes easy to
correlate the changes.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible

[Sinan Kaya]

On 6/29/2016 9:13 AM, Rafael J. Wysocki wrote:
> On Wed, Jun 29, 2016 at 10:27 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") omitted the initially assigned POSSIBLE penalty
>> when the IRQ is active.
> 
> It would be good to say what can go wrong with that here.
> 

I can add more description. Here is a first attempt.

Incorrect calculation of penalty leads to ACPI code assigning the wrong
interrupt number to PCI INTx interrupts.

This would not be as bad as it sounds in theory. You would just cause the
interrupts to be shared and observe performance penalty.

However, some drivers like parallel port driver doesn't like interrupt
sharing as in this example and causes all other PCI drivers sharing the interrupt
to malfunction.

The issue has not been caught because the behavior is platform specific
and depends on the peripheral drivers sharing the IRQ. 

I can claim that this could be a BIOS bug. if interrupt 7 is not good for PCI,
it shouldn't have been listed in the possible PCI interrupts to begin with. 
Given this is an existing platform, I don't think we have the luxury to request
all BIOS to be updated. This bugfix is needed to support existing platforms.


Feel free to request more information if the above description is not clear.


>> The original code would assign the POSSIBLE value divided by the number
>> of possible IRQs during initialization.
>>
>> Later, if the IRQ is chosen as the active IRQ or if the IRQ is in use
>> by ISA; additional penalties get added.
> 
> Thanks,
> Rafael
> --
> 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 Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2 4/4] ACPI,PCI,IRQ: correct operator precedence

[Rafael J. Wysocki]

On Wed, Jun 29, 2016 at 8:29 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 6/29/2016 9:16 AM, Rafael J. Wysocki wrote:
>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> Well, this is a rather obvious one, so I'm wondering why it is the
>> last one in the series?
>>
>
> The first three are more relevant to each other. It makes easy to
> correlate the changes.

But this one doesn't seem to depend on them and it could be applied
without them, right?

Thanks,
Rafael

Re: [PATCH V2 4/4] ACPI,PCI,IRQ: correct operator precedence

[Sinan Kaya]

On 6/29/2016 5:14 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 29, 2016 at 8:29 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 6/29/2016 9:16 AM, Rafael J. Wysocki wrote:
>>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>>> Well, this is a rather obvious one, so I'm wondering why it is the
>>> last one in the series?
>>>
>>
>> The first three are more relevant to each other. It makes easy to
>> correlate the changes.
> 
> But this one doesn't seem to depend on them and it could be applied
> without them, right?
> 

Sure. It has no dependency. 


> Thanks,
> Rafael
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible

[Rafael J. Wysocki]

On Wed, Jun 29, 2016 at 8:47 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 6/29/2016 9:13 AM, Rafael J. Wysocki wrote:
>> On Wed, Jun 29, 2016 at 10:27 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>>> resource requirements") omitted the initially assigned POSSIBLE penalty
>>> when the IRQ is active.
>>
>> It would be good to say what can go wrong with that here.
>>
>
> I can add more description. Here is a first attempt.
>
> Incorrect calculation of penalty leads to ACPI code assigning the wrong
> interrupt number to PCI INTx interrupts.
>
> This would not be as bad as it sounds in theory. You would just cause the
> interrupts to be shared and observe performance penalty.
>
> However, some drivers like parallel port driver doesn't like interrupt
> sharing as in this example and causes all other PCI drivers sharing the interrupt
> to malfunction.
>
> The issue has not been caught because the behavior is platform specific
> and depends on the peripheral drivers sharing the IRQ.
>
> I can claim that this could be a BIOS bug. if interrupt 7 is not good for PCI,
> it shouldn't have been listed in the possible PCI interrupts to begin with.
> Given this is an existing platform, I don't think we have the luxury to request
> all BIOS to be updated. This bugfix is needed to support existing platforms.
>
>
> Feel free to request more information if the above description is not clear.

It is clear enough.  I can add it to the changelog when applying the patch.

>
>>> The original code would assign the POSSIBLE value divided by the number
>>> of possible IRQs during initialization.
>>>
>>> Later, if the IRQ is chosen as the active IRQ or if the IRQ is in use
>>> by ISA; additional penalties get added.

Does "later" here mean "later in that code path" or "in a later patch"?

Thanks,
Rafael

Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible

[Sinan Kaya]

On 6/29/2016 5:19 PM, Rafael J. Wysocki wrote:
> On Wed, Jun 29, 2016 at 8:47 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 6/29/2016 9:13 AM, Rafael J. Wysocki wrote:
>>> On Wed, Jun 29, 2016 at 10:27 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>>>> resource requirements") omitted the initially assigned POSSIBLE penalty
>>>> when the IRQ is active.
>>>
>>> It would be good to say what can go wrong with that here.
>>>
>>
>> I can add more description. Here is a first attempt.
>>
>> Incorrect calculation of penalty leads to ACPI code assigning the wrong
>> interrupt number to PCI INTx interrupts.
>>
>> This would not be as bad as it sounds in theory. You would just cause the
>> interrupts to be shared and observe performance penalty.
>>
>> However, some drivers like parallel port driver doesn't like interrupt
>> sharing as in this example and causes all other PCI drivers sharing the interrupt
>> to malfunction.
>>
>> The issue has not been caught because the behavior is platform specific
>> and depends on the peripheral drivers sharing the IRQ.
>>
>> I can claim that this could be a BIOS bug. if interrupt 7 is not good for PCI,
>> it shouldn't have been listed in the possible PCI interrupts to begin with.
>> Given this is an existing platform, I don't think we have the luxury to request
>> all BIOS to be updated. This bugfix is needed to support existing platforms.
>>
>>
>> Feel free to request more information if the above description is not clear.
> 
> It is clear enough.  I can add it to the changelog when applying the patch.

OK

> 
>>
>>>> The original code would assign the POSSIBLE value divided by the number
>>>> of possible IRQs during initialization.
>>>>
>>>> Later, if the IRQ is chosen as the active IRQ or if the IRQ is in use
>>>> by ISA; additional penalties get added.
> 
> Does "later" here mean "later in that code path" or "in a later patch"?

"later in that code path"

> 
> Thanks,
> Rafael
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible

[Rafael J. Wysocki]

On Wed, Jun 29, 2016 at 11:21 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 6/29/2016 5:19 PM, Rafael J. Wysocki wrote:
>> On Wed, Jun 29, 2016 at 8:47 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>> On 6/29/2016 9:13 AM, Rafael J. Wysocki wrote:
>>>> On Wed, Jun 29, 2016 at 10:27 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>>>>> resource requirements") omitted the initially assigned POSSIBLE penalty
>>>>> when the IRQ is active.
>>>>
>>>> It would be good to say what can go wrong with that here.
>>>>
>>>
>>> I can add more description. Here is a first attempt.
>>>
>>> Incorrect calculation of penalty leads to ACPI code assigning the wrong
>>> interrupt number to PCI INTx interrupts.
>>>
>>> This would not be as bad as it sounds in theory. You would just cause the
>>> interrupts to be shared and observe performance penalty.
>>>
>>> However, some drivers like parallel port driver doesn't like interrupt
>>> sharing as in this example and causes all other PCI drivers sharing the interrupt
>>> to malfunction.
>>>
>>> The issue has not been caught because the behavior is platform specific
>>> and depends on the peripheral drivers sharing the IRQ.
>>>
>>> I can claim that this could be a BIOS bug. if interrupt 7 is not good for PCI,
>>> it shouldn't have been listed in the possible PCI interrupts to begin with.
>>> Given this is an existing platform, I don't think we have the luxury to request
>>> all BIOS to be updated. This bugfix is needed to support existing platforms.
>>>
>>>
>>> Feel free to request more information if the above description is not clear.
>>
>> It is clear enough.  I can add it to the changelog when applying the patch.
>
> OK
>
>>
>>>
>>>>> The original code would assign the POSSIBLE value divided by the number
>>>>> of possible IRQs during initialization.
>>>>>
>>>>> Later, if the IRQ is chosen as the active IRQ or if the IRQ is in use
>>>>> by ISA; additional penalties get added.
>>
>> Does "later" here mean "later in that code path" or "in a later patch"?
>
> "later in that code path"

OK

I'm hoping we'll hear from the reporter shortly.

Thanks,
Rafael

Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible

[Rafael J. Wysocki]

On Wed, Jun 29, 2016 at 11:21 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 6/29/2016 5:19 PM, Rafael J. Wysocki wrote:
>> On Wed, Jun 29, 2016 at 8:47 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>> On 6/29/2016 9:13 AM, Rafael J. Wysocki wrote:
>>>> On Wed, Jun 29, 2016 at 10:27 AM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>>>>> resource requirements") omitted the initially assigned POSSIBLE penalty
>>>>> when the IRQ is active.
>>>>
>>>> It would be good to say what can go wrong with that here.
>>>>
>>>
>>> I can add more description. Here is a first attempt.
>>>
>>> Incorrect calculation of penalty leads to ACPI code assigning the wrong
>>> interrupt number to PCI INTx interrupts.
>>>
>>> This would not be as bad as it sounds in theory. You would just cause the
>>> interrupts to be shared and observe performance penalty.
>>>
>>> However, some drivers like parallel port driver doesn't like interrupt
>>> sharing as in this example and causes all other PCI drivers sharing the interrupt
>>> to malfunction.
>>>
>>> The issue has not been caught because the behavior is platform specific
>>> and depends on the peripheral drivers sharing the IRQ.
>>>
>>> I can claim that this could be a BIOS bug. if interrupt 7 is not good for PCI,
>>> it shouldn't have been listed in the possible PCI interrupts to begin with.
>>> Given this is an existing platform, I don't think we have the luxury to request
>>> all BIOS to be updated. This bugfix is needed to support existing platforms.
>>>
>>>
>>> Feel free to request more information if the above description is not clear.
>>
>> It is clear enough.  I can add it to the changelog when applying the patch.
>
> OK

BTW, care to add Fixes: tags to these patches?

Thanks,
Rafael

Re: [PATCH V2 4/4] ACPI,PCI,IRQ: correct operator precedence

[Rafael J. Wysocki]

On Wed, Jun 29, 2016 at 11:19 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 6/29/2016 5:14 PM, Rafael J. Wysocki wrote:
>> On Wed, Jun 29, 2016 at 8:29 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>> On 6/29/2016 9:16 AM, Rafael J. Wysocki wrote:
>>>>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>>>> Well, this is a rather obvious one, so I'm wondering why it is the
>>>> last one in the series?
>>>>
>>>
>>> The first three are more relevant to each other. It makes easy to
>>> correlate the changes.
>>
>> But this one doesn't seem to depend on them and it could be applied
>> without them, right?
>>
>
> Sure. It has no dependency.

OK

I've queued up this one.

Thanks,
Rafael

Re: [PATCH V2 3/4] ACPI,PCI,IRQ: separate ISA penalty calculation

[Bjorn Helgaas]

Hi Sinan,

On Wed, Jun 29, 2016 at 04:27:37AM -0400, Sinan Kaya wrote:
> Since commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements")
> the penalty values are calculated on the fly rather than boot time.
> 
> This works fine for PCI interrupts but not so well for the ISA interrupts.
> Whether an ISA interrupt is in use or not information is not available
> inside the pci_link.c file. This information gets sent externally via
> acpi_penalize_isa_irq function. If active is true, then the IRQ is in use
> by ISA. Otherwise, IRQ is in use by PCI.
> 
> Since the current code relies on PCI Link object for determination of
> penalties, we are factoring in the PCI penalty twice after
> acpi_penalize_isa_irq function is called.

I know this patch has already been merged, but I'm confused.

Can you be a little more specific about how we factor in the PCI
penalty twice?  I think that when we enumerate an enabled link device,
we call acpi_penalize_isa_irq(x) in this path:

  pnpacpi_allocated_resource
    pnpacpi_add_irqresource
      pcibios_penalize_isa_irq
        acpi_penalize_isa_irq
          acpi_isa_irq_penalty[x] = PIRQ_PENALTY_ISA_USED

And I see that acpi_irq_penalty_init() also adds in some penalty
(either "PIRQ_PENALTY_PCI_POSSIBLE / possible_count" or
PIRQ_PENALTY_PCI_POSSIBLE).  And when we call acpi_irq_get_penalty(x),
we add in PIRQ_PENALTY_PCI_USING.

It doesn't seem right to me that we're adding both
PIRQ_PENALTY_ISA_USED and PIRQ_PENALTY_PCI_USING.  Is that the problem
you're referring to?

> This change is limiting the newly added functionality to just PCI
> interrupts so that old behavior is still maintained.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/acpi/pci_link.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 714ba4d..8c08971 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -496,9 +496,6 @@ static int acpi_irq_get_penalty(int irq)
>  {
>  	int penalty = 0;
>  
> -	if (irq < ACPI_MAX_ISA_IRQS)
> -		penalty += acpi_isa_irq_penalty[irq];
> -
>  	/*
>  	* 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
> @@ -513,6 +510,9 @@ static int acpi_irq_get_penalty(int irq)
>  			penalty += PIRQ_PENALTY_PCI_USING;
>  	}
>  
> +	if (irq < ACPI_MAX_ISA_IRQS)
> +		return penalty + acpi_isa_irq_penalty[irq];
> +
>  	penalty += acpi_irq_pci_sharing_penalty(irq);
>  	return penalty;

I don't understand what's going on here.

acpi_irq_pci_sharing_penalty(X) basically tells us how many link
devices are already using IRQ X.  This change makes it so we don't
consider that information if X < ACPI_MAX_ISA_IRQS.

Let's say we have several link devices that are initially disabled,
e.g.,

  LNKA (IRQs 9 10 11)
  LNKB (IRQs 9 10 11)
  LNKC (IRQs 9 10 11)

When we enable these, I think we'll choose the same IRQ for all of
them because we no longer look at the other links to see how they're
configured.

>  }
> -- 
> 1.8.2.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

Re: [PATCH V2 3/4] ACPI,PCI,IRQ: separate ISA penalty calculation

[Sinan Kaya]

On 10/18/2016 3:46 AM, Bjorn Helgaas wrote:
> Hi Sinan,
> 
> On Wed, Jun 29, 2016 at 04:27:37AM -0400, Sinan Kaya wrote:
>> Since commit 103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements")
>> the penalty values are calculated on the fly rather than boot time.
>>
>> This works fine for PCI interrupts but not so well for the ISA interrupts.
>> Whether an ISA interrupt is in use or not information is not available
>> inside the pci_link.c file. This information gets sent externally via
>> acpi_penalize_isa_irq function. If active is true, then the IRQ is in use
>> by ISA. Otherwise, IRQ is in use by PCI.
>>
>> Since the current code relies on PCI Link object for determination of
>> penalties, we are factoring in the PCI penalty twice after
>> acpi_penalize_isa_irq function is called.
> 
> I know this patch has already been merged, but I'm confused.
> 
> Can you be a little more specific about how we factor in the PCI
> penalty twice?  I think that when we enumerate an enabled link device,
> we call acpi_penalize_isa_irq(x) in this path:
> 
>   pnpacpi_allocated_resource
>     pnpacpi_add_irqresource
>       pcibios_penalize_isa_irq
>         acpi_penalize_isa_irq
>           acpi_isa_irq_penalty[x] = PIRQ_PENALTY_ISA_USED
> 

This is not really a problem but more information about how things work. 
I was trying to point out the fact that acpi_penalize_isa_irq is changing
the penalties externally while ISA IRQs get initialized based on the active
parameter. 

The penalty determination of ISA IRQ goes through 2 paths.
1. assign PCI_USING during power up via acpi_irq_penalty_init
2. update the penalty with acpi_irq_pci_sharing_penalty function based on active
parameter.


> And I see that acpi_irq_penalty_init() also adds in some penalty
> (either "PIRQ_PENALTY_PCI_POSSIBLE / possible_count" or
> PIRQ_PENALTY_PCI_POSSIBLE).  And when we call acpi_irq_get_penalty(x),
> we add in PIRQ_PENALTY_PCI_USING.
> 
> It doesn't seem right to me that we're adding both
> PIRQ_PENALTY_ISA_USED and PIRQ_PENALTY_PCI_USING.  Is that the problem
> you're referring to?

Correct, this is the one. What happened in this case is that 
acpi_irq_penalty_init added a PCI_USING penalty during boot. Then, when we
wanted to get the penalty for an ISA IRQ. This added another PCI_USING penalty
in acpi_irq_pci_sharing_penalty function in addition to originally added penalty.

Now, we have 2 * PCI_USING assigned to an ISA IRQ.

> 
>> This change is limiting the newly added functionality to just PCI
>> interrupts so that old behavior is still maintained.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/acpi/pci_link.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index 714ba4d..8c08971 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -496,9 +496,6 @@ static int acpi_irq_get_penalty(int irq)
>>  {
>>  	int penalty = 0;
>>  
>> -	if (irq < ACPI_MAX_ISA_IRQS)
>> -		penalty += acpi_isa_irq_penalty[irq];
>> -
>>  	/*
>>  	* 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
>> @@ -513,6 +510,9 @@ static int acpi_irq_get_penalty(int irq)
>>  			penalty += PIRQ_PENALTY_PCI_USING;
>>  	}
>>  
>> +	if (irq < ACPI_MAX_ISA_IRQS)
>> +		return penalty + acpi_isa_irq_penalty[irq];
>> +
>>  	penalty += acpi_irq_pci_sharing_penalty(irq);
>>  	return penalty;
> 
> I don't understand what's going on here.
> 
> acpi_irq_pci_sharing_penalty(X) basically tells us how many link
> devices are already using IRQ X.  This change makes it so we don't
> consider that information if X < ACPI_MAX_ISA_IRQS.
> 

The ISA IRQ doesn't need the penalties coming from
acpi_irq_pci_sharing_penalty function since acpi_irq_pci_sharing_penalty
is intended do the same thing as acpi_irq_penalty_init. It is just smarter
to cover more IRQ range.

Since acpi_irq_penalty_init is called during boot for the ISA IRQS, calling
acpi_irq_pci_sharing_penalty again is incorrect.


> Let's say we have several link devices that are initially disabled,
> e.g.,
> 
>   LNKA (IRQs 9 10 11)
>   LNKB (IRQs 9 10 11)
>   LNKC (IRQs 9 10 11)
> 
> When we enable these, I think we'll choose the same IRQ for all of
> them because we no longer look at the other links to see how they're
> configured.

You are right. This is the reason why I have this patch.

[PATCH V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

The penalties get assigned by the acpi_irq_penalty_init and acpi_penalize_isa_irq
functions before the PCI Link object is created until this moment. 

By the time link object is getting initialized, the code chooses the correct penalty here:

/
 * Select the best IRQ.  This is done in reverse to promote
 * the use of IRQs 9, 10, 11, and >15.
 */
for (i = (link->irq.possible_count - 1); i >= 0; i--) {
        if (acpi_irq_get_penalty(irq) >
            acpi_irq_get_penalty(link->irq.possible[i]))
                irq = link->irq.possible[i];
}

and the code needs to increment the penalty on this IRQ so that the next PCI Link object
would find another IRQ. This is missing right now.


> 
>>  }
>> -- 
>> 1.8.2.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
> --
> 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.