Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1433551 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible Sinan Kaya <okaya@codeaurora.org> - 2016-06-29 10:30 +0200
    Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible "Rafael J. Wysocki" <rafael@kernel.org> - 2016-06-29 15:20 +0200
      Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible Sinan Kaya <okaya@codeaurora.org> - 2016-06-29 20:50 +0200
        Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible "Rafael J. Wysocki" <rafael@kernel.org> - 2016-06-29 23:20 +0200
          Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible "Rafael J. Wysocki" <rafael@kernel.org> - 2016-06-29 23:30 +0200
          Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible "Rafael J. Wysocki" <rafael@kernel.org> - 2016-06-29 23:50 +0200
          Re: [PATCH V2 1/4] ACPI,PCI,IRQ: factor in PCI possible Sinan Kaya <okaya@codeaurora.org> - 2016-06-30 00:20 +0200

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

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

[toc] | [next] | [standalone]

#1433724

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

[toc] | [prev] | [next] | [standalone]

#1433974

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

[toc] | [prev] | [next] | [standalone]

#1434061

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

[toc] | [prev] | [next] | [standalone]

#1434063

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

[toc] | [prev] | [next] | [standalone]

#1434071

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

[toc] | [prev] | [next] | [standalone]

#1434083

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web