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

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

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

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
>