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

[Sinan Kaya]

On 10/22/2016 7:58 PM, Bjorn Helgaas wrote:
> On Sat, Oct 15, 2016 at 12:31:05AM -0400, Sinan Kaya wrote:
>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") removed PCI_USING penalty from
>> acpi_pci_link_allocate function as there is no longer a fixed size penalty
>> array for both PCI interrupts greater than 16.
>>
>> The array size has been reduced to 16 and array name got prefixed as ISA
>> since it only is accountable for the ISA interrupts.
>>
>> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") removed penalty assignment in the code for PCI
>> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
>> function.
>>
>> However, this function only gets called if the IRQ number is greater than
>> 16 and acpi_irq_get_penalty function gets called before ACPI start in
>> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
>> on iterating the link list.
>>
>> We need to add the PCI_USING penalty for ISA interrupts too if the link is
>> in use and matches our ISA IRQ number.
> 
> I think the history about the array size is more than is necessary for this
> changelog.  I think the useful part is something like this:
> 
>   ACPI: pci_link: Include PIRQ_PENALTY_PCI_USING for ISA IRQs
> 
>   103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") replaced
>   the addition of PIRQ_PENALTY_PCI_USING in acpi_pci_link_allocate()
>   with an addition in acpi_irq_pci_sharing_penalty(), but f7eca374f000
>   ("ACPI,PCI,IRQ: separate ISA penalty calculation") removed the use
>   of acpi_irq_pci_sharing_penalty() for ISA IRQs.
> 
>   Therefore, PIRQ_PENALTY_PCI_USING is missing from ISA IRQs used by
>   interrupt links.  Include that penalty by adding it in the
>   acpi_pci_link_allocate() path.
> 
>   Fixes: f7eca374f000 ("ACPI,PCI,IRQ: separate ISA penalty calculation")
>   
>> Signed-off-by: Sinan Kaya 
> 
> Acked-by: Bjorn Helgaas 

OK. Updated as suggested.

> 
>> ---
>>  drivers/acpi/pci_link.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index c983bf7..a212709 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
>> *link)
>>  acpi_device_bid(link->device));
>>  return -ENODEV;
>>  } else {
>> +if (link->irq.active < ACPI_MAX_ISA_IRQS)
>> +acpi_isa_irq_penalty[link->irq.active] +=
>> +PIRQ_PENALTY_PCI_USING;
>> +
>>  printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>> acpi_device_name(link->device),
>> acpi_device_bid(link->device), link->irq.active);
>> -- 
>> 1.9.1
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

On 10/22/2016 7:58 PM, Bjorn Helgaas wrote:
> On Sat, Oct 15, 2016 at 12:31:05AM -0400, Sinan Kaya wrote:
>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") removed PCI_USING penalty from
>> acpi_pci_link_allocate function as there is no longer a fixed size penalty
>> array for both PCI interrupts greater than 16.
>>
>> The array size has been reduced to 16 and array name got prefixed as ISA
>> since it only is accountable for the ISA interrupts.
>>
>> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") removed penalty assignment in the code for PCI
>> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
>> function.
>>
>> However, this function only gets called if the IRQ number is greater than
>> 16 and acpi_irq_get_penalty function gets called before ACPI start in
>> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
>> on iterating the link list.
>>
>> We need to add the PCI_USING penalty for ISA interrupts too if the link is
>> in use and matches our ISA IRQ number.
> 
> I think the history about the array size is more than is necessary for this
> changelog.  I think the useful part is something like this:
> 
>   ACPI: pci_link: Include PIRQ_PENALTY_PCI_USING for ISA IRQs
> 
>   103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") replaced
>   the addition of PIRQ_PENALTY_PCI_USING in acpi_pci_link_allocate()
>   with an addition in acpi_irq_pci_sharing_penalty(), but f7eca374f000
>   ("ACPI,PCI,IRQ: separate ISA penalty calculation") removed the use
>   of acpi_irq_pci_sharing_penalty() for ISA IRQs.
> 
>   Therefore, PIRQ_PENALTY_PCI_USING is missing from ISA IRQs used by
>   interrupt links.  Include that penalty by adding it in the
>   acpi_pci_link_allocate() path.
> 
>   Fixes: f7eca374f000 ("ACPI,PCI,IRQ: separate ISA penalty calculation")
>   
>> Signed-off-by: Sinan Kaya 
> 
> Acked-by: Bjorn Helgaas 

OK. Updated as suggested.

> 
>> ---
>>  drivers/acpi/pci_link.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index c983bf7..a212709 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
>> *link)
>>  acpi_device_bid(link->device));
>>  return -ENODEV;
>>  } else {
>> +if (link->irq.active < ACPI_MAX_ISA_IRQS)
>> +acpi_isa_irq_penalty[link->irq.active] +=
>> +PIRQ_PENALTY_PCI_USING;
>> +
>>  printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>> acpi_device_name(link->device),
>> acpi_device_bid(link->device), link->irq.active);
>> -- 
>> 1.9.1
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Bjorn Helgaas]

On Sat, Oct 15, 2016 at 12:31:05AM -0400, Sinan Kaya wrote:
> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed PCI_USING penalty from
> acpi_pci_link_allocate function as there is no longer a fixed size penalty
> array for both PCI interrupts greater than 16.
> 
> The array size has been reduced to 16 and array name got prefixed as ISA
> since it only is accountable for the ISA interrupts.
> 
> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed penalty assignment in the code for PCI
> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
> function.
> 
> However, this function only gets called if the IRQ number is greater than
> 16 and acpi_irq_get_penalty function gets called before ACPI start in
> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
> on iterating the link list.
> 
> We need to add the PCI_USING penalty for ISA interrupts too if the link is
> in use and matches our ISA IRQ number.

I think the history about the array size is more than is necessary for this
changelog.  I think the useful part is something like this:

  ACPI: pci_link: Include PIRQ_PENALTY_PCI_USING for ISA IRQs

  103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") replaced
  the addition of PIRQ_PENALTY_PCI_USING in acpi_pci_link_allocate()
  with an addition in acpi_irq_pci_sharing_penalty(), but f7eca374f000
  ("ACPI,PCI,IRQ: separate ISA penalty calculation") removed the use
  of acpi_irq_pci_sharing_penalty() for ISA IRQs.

  Therefore, PIRQ_PENALTY_PCI_USING is missing from ISA IRQs used by
  interrupt links.  Include that penalty by adding it in the
  acpi_pci_link_allocate() path.

  Fixes: f7eca374f000 ("ACPI,PCI,IRQ: separate ISA penalty calculation")
  
> Signed-off-by: Sinan Kaya 

Acked-by: Bjorn Helgaas 

> ---
>  drivers/acpi/pci_link.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index c983bf7..a212709 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
> *link)
>   acpi_device_bid(link->device));
>   return -ENODEV;
>   } else {
> + if (link->irq.active < ACPI_MAX_ISA_IRQS)
> + acpi_isa_irq_penalty[link->irq.active] +=
> + PIRQ_PENALTY_PCI_USING;
> +
>   printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>  acpi_device_name(link->device),
>  acpi_device_bid(link->device), link->irq.active);
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

On Sat, Oct 15, 2016 at 12:31:05AM -0400, Sinan Kaya wrote:
> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed PCI_USING penalty from
> acpi_pci_link_allocate function as there is no longer a fixed size penalty
> array for both PCI interrupts greater than 16.
> 
> The array size has been reduced to 16 and array name got prefixed as ISA
> since it only is accountable for the ISA interrupts.
> 
> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed penalty assignment in the code for PCI
> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
> function.
> 
> However, this function only gets called if the IRQ number is greater than
> 16 and acpi_irq_get_penalty function gets called before ACPI start in
> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
> on iterating the link list.
> 
> We need to add the PCI_USING penalty for ISA interrupts too if the link is
> in use and matches our ISA IRQ number.

I think the history about the array size is more than is necessary for this
changelog.  I think the useful part is something like this:

  ACPI: pci_link: Include PIRQ_PENALTY_PCI_USING for ISA IRQs

  103544d86976 ("ACPI,PCI,IRQ: reduce resource requirements") replaced
  the addition of PIRQ_PENALTY_PCI_USING in acpi_pci_link_allocate()
  with an addition in acpi_irq_pci_sharing_penalty(), but f7eca374f000
  ("ACPI,PCI,IRQ: separate ISA penalty calculation") removed the use
  of acpi_irq_pci_sharing_penalty() for ISA IRQs.

  Therefore, PIRQ_PENALTY_PCI_USING is missing from ISA IRQs used by
  interrupt links.  Include that penalty by adding it in the
  acpi_pci_link_allocate() path.

  Fixes: f7eca374f000 ("ACPI,PCI,IRQ: separate ISA penalty calculation")
  
> Signed-off-by: Sinan Kaya 

Acked-by: Bjorn Helgaas 

> ---
>  drivers/acpi/pci_link.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index c983bf7..a212709 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
> *link)
>   acpi_device_bid(link->device));
>   return -ENODEV;
>   } else {
> + if (link->irq.active < ACPI_MAX_ISA_IRQS)
> + acpi_isa_irq_penalty[link->irq.active] +=
> + PIRQ_PENALTY_PCI_USING;
> +
>   printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>  acpi_device_name(link->device),
>  acpi_device_bid(link->device), link->irq.active);
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sinan Kaya]

On 10/20/2016 7:41 PM, Bjorn Helgaas wrote:
> On Thu, Oct 20, 2016 at 01:01:04PM -0700, Sinan Kaya wrote:
>> On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:
> 
>>>   - Maintain a mapping of (IRQ, penalty).  Initially all penalties are
>>> zero.  This is for *all* IRQs, not just ISA ones.  This could be a
>>> linked list, but the structure is not important as long as we can
>>> add things dynamically.
>>
>> Dynamic allocation doesn't work due to early calls from x86 architecture.
>> This is the reason why we iterate the link objects.
> 
> Where exactly is this early penalization?  That seems to be the
> biggest problem.  Well, maybe the question of ACPI core parsing of
> _CRS/_PRS is a bigger structural problem, but the dynamic allocation
> thing at least seems solvable.
> 

http://marc.info/?l=linux-acpi=145580159209240=2


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

On 10/20/2016 7:41 PM, Bjorn Helgaas wrote:
> On Thu, Oct 20, 2016 at 01:01:04PM -0700, Sinan Kaya wrote:
>> On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:
> 
>>>   - Maintain a mapping of (IRQ, penalty).  Initially all penalties are
>>> zero.  This is for *all* IRQs, not just ISA ones.  This could be a
>>> linked list, but the structure is not important as long as we can
>>> add things dynamically.
>>
>> Dynamic allocation doesn't work due to early calls from x86 architecture.
>> This is the reason why we iterate the link objects.
> 
> Where exactly is this early penalization?  That seems to be the
> biggest problem.  Well, maybe the question of ACPI core parsing of
> _CRS/_PRS is a bigger structural problem, but the dynamic allocation
> thing at least seems solvable.
> 

http://marc.info/?l=linux-acpi=145580159209240=2


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Bjorn Helgaas]

On Thu, Oct 20, 2016 at 01:01:04PM -0700, Sinan Kaya wrote:
> On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:

> >   - Maintain a mapping of (IRQ, penalty).  Initially all penalties are
> > zero.  This is for *all* IRQs, not just ISA ones.  This could be a
> > linked list, but the structure is not important as long as we can
> > add things dynamically.
> 
> Dynamic allocation doesn't work due to early calls from x86 architecture.
> This is the reason why we iterate the link objects.

Where exactly is this early penalization?  That seems to be the
biggest problem.  Well, maybe the question of ACPI core parsing of
_CRS/_PRS is a bigger structural problem, but the dynamic allocation
thing at least seems solvable.

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

[Bjorn Helgaas]

On Thu, Oct 20, 2016 at 01:01:04PM -0700, Sinan Kaya wrote:
> On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:

> >   - Maintain a mapping of (IRQ, penalty).  Initially all penalties are
> > zero.  This is for *all* IRQs, not just ISA ones.  This could be a
> > linked list, but the structure is not important as long as we can
> > add things dynamically.
> 
> Dynamic allocation doesn't work due to early calls from x86 architecture.
> This is the reason why we iterate the link objects.

Where exactly is this early penalization?  That seems to be the
biggest problem.  Well, maybe the question of ACPI core parsing of
_CRS/_PRS is a bigger structural problem, but the dynamic allocation
thing at least seems solvable.

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

[Bjorn Helgaas]

On Thu, Oct 20, 2016 at 01:17:23AM +0200, Rafael J. Wysocki wrote:
> On Thu, Oct 20, 2016 at 12:44 AM, Bjorn Helgaas  wrote:
> > On Tue, Oct 18, 2016 at 08:32:44AM -0700, Sinan Kaya wrote:
> >> Sorry, I think I didn't have enough morning coffee.
> >>
> >> Looking at these again and trying to be specific.
> >>
> >> On 10/18/2016 8:20 AM, Sinan Kaya wrote:
> >> > It seems wrong to me that we call acpi_irq_get_penalty() from
> >> >> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like 
> >> >> they
> >> >> should just manipulate acpi_isa_irq_penalty[irq] directly.
> >> >>
> >> >> acpi_irq_penalty_update() is for command-line parameters, so it 
> >> >> certainly
> >> >> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> >> >> acpi_link_list should be empty at the time we process the command-line
> >> >> parameters).
> >>
> >> Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
> >> any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = 
> >> acpi_irq_get_penalty.
> >>
> >> If this is broken, then we need special care so that we don't assign
> >> dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
> >> results in returning incorrect penalty as
> >>
> >> acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * 
> >> sci_penalty.
> >>
> >> Now that we added sci_penalty into the acpi_irq_get_penalty function,
> >> calling acpi_irq_get_penalty is not correct anymore. This line here needs 
> >> to
> >> be replaced with acpi_isa_irq_penalty[irq] as you suggested.
> >>
> >> if (used)
> >> new_penalty = acpi_irq_get_penalty(irq) +
> >> PIRQ_PENALTY_ISA_USED;
> >> else
> >> new_penalty = 0;
> >>
> >> acpi_isa_irq_penalty[irq] = new_penalty;
> >>
> >>
> >> >>
> >> >> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> >> >> the IRQ -- this should modify the IRQ's penalty, but it shouldn't 
> >> >> depend on
> >> >> the acpi_irq_pci_sharing_penalty() value at all.
> >> >>
> >>
> >> Same problem here. This line will be broken after the sci_penalty change.
> >>
> >> acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> >>   (active ? PIRQ_PENALTY_ISA_USED : 
> >> PIRQ_PENALTY_PCI_USING);
> >
> > I think the fragility of this code is an indication that we have a
> > design problem, so I want to step back from the nitty gritty details
> > for a bit and look at the overall design.
> >
> > Let me restate the overall problem: We have a PCI device connected to
> > an interrupt link.  The interrupt link can be connected to one of
> > several IRQs, and we want to choose one of those IRQs to minimize IRQ
> > sharing.
> >
> > That means we need information about which IRQs are used.
> > Historically we've started with a compiled-in table of common ISA IRQ
> > usage, and we also collect information about which IRQs are used and
> > which *might* be used.  So we have the following inputs:
> >
> >   - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
> > values.  ACPI is *supposed* to tell us about all these usages, so
> > I don't know why we have the table.  But it's been there as long
> > as I can remember.  The table is probably x86-specific, but we
> > keep it in the supposedly generic pci_link.c.
> >
> >   - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
> > acpi_irq_pci().  I suppose these are for cases where we can't
> > figure things out automatically.  I would resist adding parameters
> > like this today (I would treat the need for them as a bug and look
> > for a fix or a quirk), but we might be stuck with these.
> >
> >   - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).
> >
> >   - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
> > acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
> > NOT include interrupt link (PNP0C0F) devices because we don't
> > handle them as PNPACPI devices.  I think this is related to the
> > fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].
> >
> >   - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
> > information from _CRS and _PRS.  This seems sub-optimal and
> > possibly buggy.
> >
> >   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
> > acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
> > call this on x86.  If _PRS exists, we penalize each possible IRQ.
> > If there's no _PRS but _CRS contains an active IRQ, we penalize
> > it.
> >
> >   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
> > enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
> > penalize an IRQ if it appears in _CRS or _PRS of any link device
> > in the system.
> >
> > For IRQs 0-15, this 

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

[Bjorn Helgaas]

On Thu, Oct 20, 2016 at 01:17:23AM +0200, Rafael J. Wysocki wrote:
> On Thu, Oct 20, 2016 at 12:44 AM, Bjorn Helgaas  wrote:
> > On Tue, Oct 18, 2016 at 08:32:44AM -0700, Sinan Kaya wrote:
> >> Sorry, I think I didn't have enough morning coffee.
> >>
> >> Looking at these again and trying to be specific.
> >>
> >> On 10/18/2016 8:20 AM, Sinan Kaya wrote:
> >> > It seems wrong to me that we call acpi_irq_get_penalty() from
> >> >> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like 
> >> >> they
> >> >> should just manipulate acpi_isa_irq_penalty[irq] directly.
> >> >>
> >> >> acpi_irq_penalty_update() is for command-line parameters, so it 
> >> >> certainly
> >> >> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> >> >> acpi_link_list should be empty at the time we process the command-line
> >> >> parameters).
> >>
> >> Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
> >> any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = 
> >> acpi_irq_get_penalty.
> >>
> >> If this is broken, then we need special care so that we don't assign
> >> dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
> >> results in returning incorrect penalty as
> >>
> >> acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * 
> >> sci_penalty.
> >>
> >> Now that we added sci_penalty into the acpi_irq_get_penalty function,
> >> calling acpi_irq_get_penalty is not correct anymore. This line here needs 
> >> to
> >> be replaced with acpi_isa_irq_penalty[irq] as you suggested.
> >>
> >> if (used)
> >> new_penalty = acpi_irq_get_penalty(irq) +
> >> PIRQ_PENALTY_ISA_USED;
> >> else
> >> new_penalty = 0;
> >>
> >> acpi_isa_irq_penalty[irq] = new_penalty;
> >>
> >>
> >> >>
> >> >> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> >> >> the IRQ -- this should modify the IRQ's penalty, but it shouldn't 
> >> >> depend on
> >> >> the acpi_irq_pci_sharing_penalty() value at all.
> >> >>
> >>
> >> Same problem here. This line will be broken after the sci_penalty change.
> >>
> >> acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
> >>   (active ? PIRQ_PENALTY_ISA_USED : 
> >> PIRQ_PENALTY_PCI_USING);
> >
> > I think the fragility of this code is an indication that we have a
> > design problem, so I want to step back from the nitty gritty details
> > for a bit and look at the overall design.
> >
> > Let me restate the overall problem: We have a PCI device connected to
> > an interrupt link.  The interrupt link can be connected to one of
> > several IRQs, and we want to choose one of those IRQs to minimize IRQ
> > sharing.
> >
> > That means we need information about which IRQs are used.
> > Historically we've started with a compiled-in table of common ISA IRQ
> > usage, and we also collect information about which IRQs are used and
> > which *might* be used.  So we have the following inputs:
> >
> >   - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
> > values.  ACPI is *supposed* to tell us about all these usages, so
> > I don't know why we have the table.  But it's been there as long
> > as I can remember.  The table is probably x86-specific, but we
> > keep it in the supposedly generic pci_link.c.
> >
> >   - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
> > acpi_irq_pci().  I suppose these are for cases where we can't
> > figure things out automatically.  I would resist adding parameters
> > like this today (I would treat the need for them as a bug and look
> > for a fix or a quirk), but we might be stuck with these.
> >
> >   - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).
> >
> >   - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
> > acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
> > NOT include interrupt link (PNP0C0F) devices because we don't
> > handle them as PNPACPI devices.  I think this is related to the
> > fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].
> >
> >   - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
> > information from _CRS and _PRS.  This seems sub-optimal and
> > possibly buggy.
> >
> >   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
> > acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
> > call this on x86.  If _PRS exists, we penalize each possible IRQ.
> > If there's no _PRS but _CRS contains an active IRQ, we penalize
> > it.
> >
> >   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
> > enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
> > penalize an IRQ if it appears in _CRS or _PRS of any link device
> > in the system.
> >
> > For IRQs 0-15, this overlaps with the 

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

[Sinan Kaya]

On 10/20/2016 2:08 PM, Rafael J. Wysocki wrote:
> On Thu, Oct 20, 2016 at 10:01 PM, Sinan Kaya  wrote:
>> On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:
> 
> [cut]
> 
>> If we want to move the ISA pieces out of this file, that can be done too.
>> We can also add support for PNPACPI. I'm not a very big fan of scratch
>> everything and start from beginning approach. This refactoring effort already
>> failed 3 times. I'd like to close the issue and move on.
> 
> Understood, but we have broken this for too many times already.
> 
> Either we have a minimum fix that is known working or we are going back.

Agreed. I think my V4 patch satisfies our short term goals and fixes the issue
you are seeing. IMO, it is good enough for the moment.

> 
> Thanks,
> Rafael
> 


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

On 10/20/2016 2:08 PM, Rafael J. Wysocki wrote:
> On Thu, Oct 20, 2016 at 10:01 PM, Sinan Kaya  wrote:
>> On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:
> 
> [cut]
> 
>> If we want to move the ISA pieces out of this file, that can be done too.
>> We can also add support for PNPACPI. I'm not a very big fan of scratch
>> everything and start from beginning approach. This refactoring effort already
>> failed 3 times. I'd like to close the issue and move on.
> 
> Understood, but we have broken this for too many times already.
> 
> Either we have a minimum fix that is known working or we are going back.

Agreed. I think my V4 patch satisfies our short term goals and fixes the issue
you are seeing. IMO, it is good enough for the moment.

> 
> Thanks,
> Rafael
> 


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Rafael J. Wysocki]

On Thu, Oct 20, 2016 at 10:01 PM, Sinan Kaya  wrote:
> On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:

[cut]

> If we want to move the ISA pieces out of this file, that can be done too.
> We can also add support for PNPACPI. I'm not a very big fan of scratch
> everything and start from beginning approach. This refactoring effort already
> failed 3 times. I'd like to close the issue and move on.

Understood, but we have broken this for too many times already.

Either we have a minimum fix that is known working or we are going back.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thu, Oct 20, 2016 at 10:01 PM, Sinan Kaya  wrote:
> On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:

[cut]

> If we want to move the ISA pieces out of this file, that can be done too.
> We can also add support for PNPACPI. I'm not a very big fan of scratch
> everything and start from beginning approach. This refactoring effort already
> failed 3 times. I'd like to close the issue and move on.

Understood, but we have broken this for too many times already.

Either we have a minimum fix that is known working or we are going back.

Thanks,
Rafael

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

[Sinan Kaya]

On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:
[cut]

>>
>> Same problem here. This line will be broken after the sci_penalty change.
>>
>> acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
>>   (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
> 
> I think the fragility of this code is an indication that we have a
> design problem, so I want to step back from the nitty gritty details
> for a bit and look at the overall design.

This is all because we started with a very simple design where we replaced
the array with a get_penalty function. Set penalty function would dynamically
reallocate the link list so that we have a design that works no matter
what the number of IRQs are. 

As we hit the first problem on existing platform, we realized that we can't
actually dynamically reallocate an interrupt because get_penalty function
gets called from early boot stages where heap is not initialized yet.

Then, we started restructuring the code so that we can discover the IRQs
by iterating the link list rather than using an array. While transitioning
into this new design, we forgot the fact that irq_get_penalty function
became a fixed array penalty + dynamically calculated penalty. 

This line was trying to increment the static array but ended up gathering
the dynamic pieces together.

This obviously is a bug and was forgotten in the code.

> 
> Let me restate the overall problem: We have a PCI device connected to
> an interrupt link.  The interrupt link can be connected to one of
> several IRQs, and we want to choose one of those IRQs to minimize IRQ
> sharing.
> 
> That means we need information about which IRQs are used.
> Historically we've started with a compiled-in table of common ISA IRQ
> usage, and we also collect information about which IRQs are used and
> which *might* be used.  So we have the following inputs:
> 
>   - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
> values.  ACPI is *supposed* to tell us about all these usages, so
> I don't know why we have the table.  But it's been there as long
> as I can remember.  The table is probably x86-specific, but we
> keep it in the supposedly generic pci_link.c.
> 

I think it is because the ISA IRQ functions get called in early boot stages
and there is no heap allocator at that point of execution.

>   - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
> acpi_irq_pci().  I suppose these are for cases where we can't
> figure things out automatically.  I would resist adding parameters
> like this today (I would treat the need for them as a bug and look
> for a fix or a quirk), but we might be stuck with these.
> 
>   - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).
> 
>   - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
> acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
> NOT include interrupt link (PNP0C0F) devices because we don't
> handle them as PNPACPI devices.  I think this is related to the
> fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].

The original code supports the entire IRQ range (0..255). We limited it
to 16 during redesign.

> 
>   - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
> information from _CRS and _PRS.  This seems sub-optimal and
> possibly buggy.
> 
>   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
> acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
> call this on x86.  If _PRS exists, we penalize each possible IRQ.
> If there's no _PRS but _CRS contains an active IRQ, we penalize
> it.

Correct.

> 
>   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
> enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
> penalize an IRQ if it appears in _CRS or _PRS of any link device
> in the system.
> 
> For IRQs 0-15, this overlaps with the penalization done at
> boot-time by acpi_irq_penalty_init(): if a device has _PRS, we'll
> add the "possible" penalty twice (once in acpi_irq_penalty_init()
> and again in acpi_irq_pci_sharing_penalty()), and the "using"
> penalty once (in acpi_irq_pci_sharing_penalty()).
> 
> If a device has no _PRS but has _CRS, the "using" penalty is
> applied twice (once in once in acpi_irq_penalty_init() and again
> in acpi_irq_pci_sharing_penalty())
> 
> I think this whole thing is baroque and grotesque.

Regardless of these, we hit four different bugs because we didn't understand
the usage model.

1. You can't use kmalloc in IRQ get penalty
2. SCI IRQ type cannot be gathered from IRQ API
3. acpi_irq_penalty cannot be called from early stages since it is iterating
the link list. If there is a penalty assignment requirement from early stages,
this needs to be done on a statically allocated array.
4. acpi_irq_penalty_init is redundant.

> 
> Here's a strawman idea:
> 
>   - Maintain a mapping of (IRQ, 

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

[Sinan Kaya]

On 10/19/2016 3:44 PM, Bjorn Helgaas wrote:
[cut]

>>
>> Same problem here. This line will be broken after the sci_penalty change.
>>
>> acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
>>   (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
> 
> I think the fragility of this code is an indication that we have a
> design problem, so I want to step back from the nitty gritty details
> for a bit and look at the overall design.

This is all because we started with a very simple design where we replaced
the array with a get_penalty function. Set penalty function would dynamically
reallocate the link list so that we have a design that works no matter
what the number of IRQs are. 

As we hit the first problem on existing platform, we realized that we can't
actually dynamically reallocate an interrupt because get_penalty function
gets called from early boot stages where heap is not initialized yet.

Then, we started restructuring the code so that we can discover the IRQs
by iterating the link list rather than using an array. While transitioning
into this new design, we forgot the fact that irq_get_penalty function
became a fixed array penalty + dynamically calculated penalty. 

This line was trying to increment the static array but ended up gathering
the dynamic pieces together.

This obviously is a bug and was forgotten in the code.

> 
> Let me restate the overall problem: We have a PCI device connected to
> an interrupt link.  The interrupt link can be connected to one of
> several IRQs, and we want to choose one of those IRQs to minimize IRQ
> sharing.
> 
> That means we need information about which IRQs are used.
> Historically we've started with a compiled-in table of common ISA IRQ
> usage, and we also collect information about which IRQs are used and
> which *might* be used.  So we have the following inputs:
> 
>   - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
> values.  ACPI is *supposed* to tell us about all these usages, so
> I don't know why we have the table.  But it's been there as long
> as I can remember.  The table is probably x86-specific, but we
> keep it in the supposedly generic pci_link.c.
> 

I think it is because the ISA IRQ functions get called in early boot stages
and there is no heap allocator at that point of execution.

>   - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
> acpi_irq_pci().  I suppose these are for cases where we can't
> figure things out automatically.  I would resist adding parameters
> like this today (I would treat the need for them as a bug and look
> for a fix or a quirk), but we might be stuck with these.
> 
>   - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).
> 
>   - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
> acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
> NOT include interrupt link (PNP0C0F) devices because we don't
> handle them as PNPACPI devices.  I think this is related to the
> fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].

The original code supports the entire IRQ range (0..255). We limited it
to 16 during redesign.

> 
>   - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
> information from _CRS and _PRS.  This seems sub-optimal and
> possibly buggy.
> 
>   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
> acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
> call this on x86.  If _PRS exists, we penalize each possible IRQ.
> If there's no _PRS but _CRS contains an active IRQ, we penalize
> it.

Correct.

> 
>   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
> enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
> penalize an IRQ if it appears in _CRS or _PRS of any link device
> in the system.
> 
> For IRQs 0-15, this overlaps with the penalization done at
> boot-time by acpi_irq_penalty_init(): if a device has _PRS, we'll
> add the "possible" penalty twice (once in acpi_irq_penalty_init()
> and again in acpi_irq_pci_sharing_penalty()), and the "using"
> penalty once (in acpi_irq_pci_sharing_penalty()).
> 
> If a device has no _PRS but has _CRS, the "using" penalty is
> applied twice (once in once in acpi_irq_penalty_init() and again
> in acpi_irq_pci_sharing_penalty())
> 
> I think this whole thing is baroque and grotesque.

Regardless of these, we hit four different bugs because we didn't understand
the usage model.

1. You can't use kmalloc in IRQ get penalty
2. SCI IRQ type cannot be gathered from IRQ API
3. acpi_irq_penalty cannot be called from early stages since it is iterating
the link list. If there is a penalty assignment requirement from early stages,
this needs to be done on a statically allocated array.
4. acpi_irq_penalty_init is redundant.

> 
> Here's a strawman idea:
> 
>   - Maintain a mapping of (IRQ, 

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

[Rafael J. Wysocki]

On Thu, Oct 20, 2016 at 12:44 AM, Bjorn Helgaas  wrote:
> On Tue, Oct 18, 2016 at 08:32:44AM -0700, Sinan Kaya wrote:
>> Sorry, I think I didn't have enough morning coffee.
>>
>> Looking at these again and trying to be specific.
>>
>> On 10/18/2016 8:20 AM, Sinan Kaya wrote:
>> > It seems wrong to me that we call acpi_irq_get_penalty() from
>> >> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
>> >> should just manipulate acpi_isa_irq_penalty[irq] directly.
>> >>
>> >> acpi_irq_penalty_update() is for command-line parameters, so it certainly
>> >> doesn't need the acpi_irq_pci_sharing_penalty() information (the
>> >> acpi_link_list should be empty at the time we process the command-line
>> >> parameters).
>>
>> Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
>> any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = 
>> acpi_irq_get_penalty.
>>
>> If this is broken, then we need special care so that we don't assign
>> dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
>> results in returning incorrect penalty as
>>
>> acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * sci_penalty.
>>
>> Now that we added sci_penalty into the acpi_irq_get_penalty function,
>> calling acpi_irq_get_penalty is not correct anymore. This line here needs to
>> be replaced with acpi_isa_irq_penalty[irq] as you suggested.
>>
>> if (used)
>> new_penalty = acpi_irq_get_penalty(irq) +
>> PIRQ_PENALTY_ISA_USED;
>> else
>> new_penalty = 0;
>>
>> acpi_isa_irq_penalty[irq] = new_penalty;
>>
>>
>> >>
>> >> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
>> >> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend 
>> >> on
>> >> the acpi_irq_pci_sharing_penalty() value at all.
>> >>
>>
>> Same problem here. This line will be broken after the sci_penalty change.
>>
>> acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
>>   (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
>
> I think the fragility of this code is an indication that we have a
> design problem, so I want to step back from the nitty gritty details
> for a bit and look at the overall design.
>
> Let me restate the overall problem: We have a PCI device connected to
> an interrupt link.  The interrupt link can be connected to one of
> several IRQs, and we want to choose one of those IRQs to minimize IRQ
> sharing.
>
> That means we need information about which IRQs are used.
> Historically we've started with a compiled-in table of common ISA IRQ
> usage, and we also collect information about which IRQs are used and
> which *might* be used.  So we have the following inputs:
>
>   - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
> values.  ACPI is *supposed* to tell us about all these usages, so
> I don't know why we have the table.  But it's been there as long
> as I can remember.  The table is probably x86-specific, but we
> keep it in the supposedly generic pci_link.c.
>
>   - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
> acpi_irq_pci().  I suppose these are for cases where we can't
> figure things out automatically.  I would resist adding parameters
> like this today (I would treat the need for them as a bug and look
> for a fix or a quirk), but we might be stuck with these.
>
>   - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).
>
>   - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
> acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
> NOT include interrupt link (PNP0C0F) devices because we don't
> handle them as PNPACPI devices.  I think this is related to the
> fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].
>
>   - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
> information from _CRS and _PRS.  This seems sub-optimal and
> possibly buggy.
>
>   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
> acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
> call this on x86.  If _PRS exists, we penalize each possible IRQ.
> If there's no _PRS but _CRS contains an active IRQ, we penalize
> it.
>
>   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
> enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
> penalize an IRQ if it appears in _CRS or _PRS of any link device
> in the system.
>
> For IRQs 0-15, this overlaps with the penalization done at
> boot-time by acpi_irq_penalty_init(): if a device has _PRS, we'll
> add the "possible" penalty twice (once in acpi_irq_penalty_init()
> and again in acpi_irq_pci_sharing_penalty()), and the "using"
> penalty once (in acpi_irq_pci_sharing_penalty()).
>

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

[Rafael J. Wysocki]

On Thu, Oct 20, 2016 at 12:44 AM, Bjorn Helgaas  wrote:
> On Tue, Oct 18, 2016 at 08:32:44AM -0700, Sinan Kaya wrote:
>> Sorry, I think I didn't have enough morning coffee.
>>
>> Looking at these again and trying to be specific.
>>
>> On 10/18/2016 8:20 AM, Sinan Kaya wrote:
>> > It seems wrong to me that we call acpi_irq_get_penalty() from
>> >> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
>> >> should just manipulate acpi_isa_irq_penalty[irq] directly.
>> >>
>> >> acpi_irq_penalty_update() is for command-line parameters, so it certainly
>> >> doesn't need the acpi_irq_pci_sharing_penalty() information (the
>> >> acpi_link_list should be empty at the time we process the command-line
>> >> parameters).
>>
>> Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
>> any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = 
>> acpi_irq_get_penalty.
>>
>> If this is broken, then we need special care so that we don't assign
>> dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
>> results in returning incorrect penalty as
>>
>> acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * sci_penalty.
>>
>> Now that we added sci_penalty into the acpi_irq_get_penalty function,
>> calling acpi_irq_get_penalty is not correct anymore. This line here needs to
>> be replaced with acpi_isa_irq_penalty[irq] as you suggested.
>>
>> if (used)
>> new_penalty = acpi_irq_get_penalty(irq) +
>> PIRQ_PENALTY_ISA_USED;
>> else
>> new_penalty = 0;
>>
>> acpi_isa_irq_penalty[irq] = new_penalty;
>>
>>
>> >>
>> >> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
>> >> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend 
>> >> on
>> >> the acpi_irq_pci_sharing_penalty() value at all.
>> >>
>>
>> Same problem here. This line will be broken after the sci_penalty change.
>>
>> acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
>>   (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
>
> I think the fragility of this code is an indication that we have a
> design problem, so I want to step back from the nitty gritty details
> for a bit and look at the overall design.
>
> Let me restate the overall problem: We have a PCI device connected to
> an interrupt link.  The interrupt link can be connected to one of
> several IRQs, and we want to choose one of those IRQs to minimize IRQ
> sharing.
>
> That means we need information about which IRQs are used.
> Historically we've started with a compiled-in table of common ISA IRQ
> usage, and we also collect information about which IRQs are used and
> which *might* be used.  So we have the following inputs:
>
>   - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
> values.  ACPI is *supposed* to tell us about all these usages, so
> I don't know why we have the table.  But it's been there as long
> as I can remember.  The table is probably x86-specific, but we
> keep it in the supposedly generic pci_link.c.
>
>   - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
> acpi_irq_pci().  I suppose these are for cases where we can't
> figure things out automatically.  I would resist adding parameters
> like this today (I would treat the need for them as a bug and look
> for a fix or a quirk), but we might be stuck with these.
>
>   - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).
>
>   - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
> acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
> NOT include interrupt link (PNP0C0F) devices because we don't
> handle them as PNPACPI devices.  I think this is related to the
> fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].
>
>   - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
> information from _CRS and _PRS.  This seems sub-optimal and
> possibly buggy.
>
>   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
> acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
> call this on x86.  If _PRS exists, we penalize each possible IRQ.
> If there's no _PRS but _CRS contains an active IRQ, we penalize
> it.
>
>   - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
> enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
> penalize an IRQ if it appears in _CRS or _PRS of any link device
> in the system.
>
> For IRQs 0-15, this overlaps with the penalization done at
> boot-time by acpi_irq_penalty_init(): if a device has _PRS, we'll
> add the "possible" penalty twice (once in acpi_irq_penalty_init()
> and again in acpi_irq_pci_sharing_penalty()), and the "using"
> penalty once (in acpi_irq_pci_sharing_penalty()).
>
> If a device 

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

[Bjorn Helgaas]

On Tue, Oct 18, 2016 at 08:32:44AM -0700, Sinan Kaya wrote:
> Sorry, I think I didn't have enough morning coffee.
> 
> Looking at these again and trying to be specific.
> 
> On 10/18/2016 8:20 AM, Sinan Kaya wrote:
> > It seems wrong to me that we call acpi_irq_get_penalty() from
> >> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
> >> should just manipulate acpi_isa_irq_penalty[irq] directly.
> >> 
> >> acpi_irq_penalty_update() is for command-line parameters, so it certainly
> >> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> >> acpi_link_list should be empty at the time we process the command-line
> >> parameters).
> 
> Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
> any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = 
> acpi_irq_get_penalty.
> 
> If this is broken, then we need special care so that we don't assign
> dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
> results in returning incorrect penalty as
> 
> acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * sci_penalty.
> 
> Now that we added sci_penalty into the acpi_irq_get_penalty function,
> calling acpi_irq_get_penalty is not correct anymore. This line here needs to
> be replaced with acpi_isa_irq_penalty[irq] as you suggested.
> 
> if (used)
> new_penalty = acpi_irq_get_penalty(irq) +
> PIRQ_PENALTY_ISA_USED;
> else
> new_penalty = 0;
> 
> acpi_isa_irq_penalty[irq] = new_penalty;
> 
> 
> >> 
> >> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> >> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
> >> the acpi_irq_pci_sharing_penalty() value at all.
> >> 
> 
> Same problem here. This line will be broken after the sci_penalty change.
> 
> acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
>   (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);

I think the fragility of this code is an indication that we have a
design problem, so I want to step back from the nitty gritty details
for a bit and look at the overall design.

Let me restate the overall problem: We have a PCI device connected to
an interrupt link.  The interrupt link can be connected to one of
several IRQs, and we want to choose one of those IRQs to minimize IRQ
sharing.

That means we need information about which IRQs are used.
Historically we've started with a compiled-in table of common ISA IRQ
usage, and we also collect information about which IRQs are used and
which *might* be used.  So we have the following inputs:

  - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
values.  ACPI is *supposed* to tell us about all these usages, so
I don't know why we have the table.  But it's been there as long
as I can remember.  The table is probably x86-specific, but we
keep it in the supposedly generic pci_link.c.

  - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
acpi_irq_pci().  I suppose these are for cases where we can't
figure things out automatically.  I would resist adding parameters
like this today (I would treat the need for them as a bug and look
for a fix or a quirk), but we might be stuck with these.

  - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).

  - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
NOT include interrupt link (PNP0C0F) devices because we don't
handle them as PNPACPI devices.  I think this is related to the
fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].

  - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
information from _CRS and _PRS.  This seems sub-optimal and
possibly buggy.

  - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
call this on x86.  If _PRS exists, we penalize each possible IRQ.
If there's no _PRS but _CRS contains an active IRQ, we penalize
it.

  - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
penalize an IRQ if it appears in _CRS or _PRS of any link device
in the system.

For IRQs 0-15, this overlaps with the penalization done at
boot-time by acpi_irq_penalty_init(): if a device has _PRS, we'll
add the "possible" penalty twice (once in acpi_irq_penalty_init()
and again in acpi_irq_pci_sharing_penalty()), and the "using"
penalty once (in acpi_irq_pci_sharing_penalty()).

If a device has no _PRS but has _CRS, the "using" penalty is
applied twice (once in once in acpi_irq_penalty_init() and again
in acpi_irq_pci_sharing_penalty())

I think this whole thing is baroque 

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

[Bjorn Helgaas]

On Tue, Oct 18, 2016 at 08:32:44AM -0700, Sinan Kaya wrote:
> Sorry, I think I didn't have enough morning coffee.
> 
> Looking at these again and trying to be specific.
> 
> On 10/18/2016 8:20 AM, Sinan Kaya wrote:
> > It seems wrong to me that we call acpi_irq_get_penalty() from
> >> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
> >> should just manipulate acpi_isa_irq_penalty[irq] directly.
> >> 
> >> acpi_irq_penalty_update() is for command-line parameters, so it certainly
> >> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> >> acpi_link_list should be empty at the time we process the command-line
> >> parameters).
> 
> Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
> any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = 
> acpi_irq_get_penalty.
> 
> If this is broken, then we need special care so that we don't assign
> dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
> results in returning incorrect penalty as
> 
> acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * sci_penalty.
> 
> Now that we added sci_penalty into the acpi_irq_get_penalty function,
> calling acpi_irq_get_penalty is not correct anymore. This line here needs to
> be replaced with acpi_isa_irq_penalty[irq] as you suggested.
> 
> if (used)
> new_penalty = acpi_irq_get_penalty(irq) +
> PIRQ_PENALTY_ISA_USED;
> else
> new_penalty = 0;
> 
> acpi_isa_irq_penalty[irq] = new_penalty;
> 
> 
> >> 
> >> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> >> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
> >> the acpi_irq_pci_sharing_penalty() value at all.
> >> 
> 
> Same problem here. This line will be broken after the sci_penalty change.
> 
> acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
>   (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);

I think the fragility of this code is an indication that we have a
design problem, so I want to step back from the nitty gritty details
for a bit and look at the overall design.

Let me restate the overall problem: We have a PCI device connected to
an interrupt link.  The interrupt link can be connected to one of
several IRQs, and we want to choose one of those IRQs to minimize IRQ
sharing.

That means we need information about which IRQs are used.
Historically we've started with a compiled-in table of common ISA IRQ
usage, and we also collect information about which IRQs are used and
which *might* be used.  So we have the following inputs:

  - Compiled-in ISA IRQ usage: the static acpi_isa_irq_penalty[]
values.  ACPI is *supposed* to tell us about all these usages, so
I don't know why we have the table.  But it's been there as long
as I can remember.  The table is probably x86-specific, but we
keep it in the supposedly generic pci_link.c.

  - The "acpi_irq_isa=" and "acpi_irq_pci=" command-line overrides via
acpi_irq_pci().  I suppose these are for cases where we can't
figure things out automatically.  I would resist adding parameters
like this today (I would treat the need for them as a bug and look
for a fix or a quirk), but we might be stuck with these.

  - SCI information from the ACPI FADT (acpi_penalize_sci_irq()).

  - PNPBIOS and PNPACPI device IRQ usage from _CRS and _PRS via
acpi_penalize_isa_irq().  This is only for IRQs 0-15, and it does
NOT include interrupt link (PNP0C0F) devices because we don't
handle them as PNPACPI devices.  I think this is related to the
fact that PNP0C0F doesn't appear in acpi_pnp_device_ids[].

  - For non-PNP0C0F, non-PNPACPI devices, we completely ignore IRQ
information from _CRS and _PRS.  This seems sub-optimal and
possibly buggy.

  - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS via
acpi_irq_penalty_init().  This is only for IRQs 0-15, and we only
call this on x86.  If _PRS exists, we penalize each possible IRQ.
If there's no _PRS but _CRS contains an active IRQ, we penalize
it.

  - Interrupt link (PNP0C0F) IRQ usage from _CRS and _PRS when
enabling a new link.  In acpi_irq_pci_sharing_penalty(), we
penalize an IRQ if it appears in _CRS or _PRS of any link device
in the system.

For IRQs 0-15, this overlaps with the penalization done at
boot-time by acpi_irq_penalty_init(): if a device has _PRS, we'll
add the "possible" penalty twice (once in acpi_irq_penalty_init()
and again in acpi_irq_pci_sharing_penalty()), and the "using"
penalty once (in acpi_irq_pci_sharing_penalty()).

If a device has no _PRS but has _CRS, the "using" penalty is
applied twice (once in once in acpi_irq_penalty_init() and again
in acpi_irq_pci_sharing_penalty())

I think this whole thing is baroque 

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

[Sinan Kaya]

Bjorn,

On 10/18/2016 6:59 AM, Bjorn Helgaas wrote:
> It seems wrong to me that we call acpi_irq_get_penalty() from
> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
> should just manipulate acpi_isa_irq_penalty[irq] directly.
> 
> acpi_irq_penalty_update() is for command-line parameters, so it certainly
> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> acpi_link_list should be empty at the time we process the command-line
> parameters).
> 
> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
> the acpi_irq_pci_sharing_penalty() value at all.

I posted v4 with this change and also went back to the original implementation
for sharing penalty calculation whether the IRQ is ISA or PCI.

Let us know what you think. 

I also realized that calculating sharing penalty while the link object is not 
initialized is not right.

Sinan

-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

Bjorn,

On 10/18/2016 6:59 AM, Bjorn Helgaas wrote:
> It seems wrong to me that we call acpi_irq_get_penalty() from
> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
> should just manipulate acpi_isa_irq_penalty[irq] directly.
> 
> acpi_irq_penalty_update() is for command-line parameters, so it certainly
> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> acpi_link_list should be empty at the time we process the command-line
> parameters).
> 
> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
> the acpi_irq_pci_sharing_penalty() value at all.

I posted v4 with this change and also went back to the original implementation
for sharing penalty calculation whether the IRQ is ISA or PCI.

Let us know what you think. 

I also realized that calculating sharing penalty while the link object is not 
initialized is not right.

Sinan

-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

Sorry, I think I didn't have enough morning coffee.

Looking at these again and trying to be specific.

On 10/18/2016 8:20 AM, Sinan Kaya wrote:
> It seems wrong to me that we call acpi_irq_get_penalty() from
>> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
>> should just manipulate acpi_isa_irq_penalty[irq] directly.
>> 
>> acpi_irq_penalty_update() is for command-line parameters, so it certainly
>> doesn't need the acpi_irq_pci_sharing_penalty() information (the
>> acpi_link_list should be empty at the time we process the command-line
>> parameters).

Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = 
acpi_irq_get_penalty.

If this is broken, then we need special care so that we don't assign
dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
results in returning incorrect penalty as

acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * sci_penalty.

Now that we added sci_penalty into the acpi_irq_get_penalty function,
calling acpi_irq_get_penalty is not correct anymore. This line here needs to
be replaced with acpi_isa_irq_penalty[irq] as you suggested.

if (used)
new_penalty = acpi_irq_get_penalty(irq) +
PIRQ_PENALTY_ISA_USED;
else
new_penalty = 0;

acpi_isa_irq_penalty[irq] = new_penalty;


>> 
>> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
>> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
>> the acpi_irq_pci_sharing_penalty() value at all.
>> 

Same problem here. This line will be broken after the sci_penalty change.

acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
  (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);




-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

Sorry, I think I didn't have enough morning coffee.

Looking at these again and trying to be specific.

On 10/18/2016 8:20 AM, Sinan Kaya wrote:
> It seems wrong to me that we call acpi_irq_get_penalty() from
>> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
>> should just manipulate acpi_isa_irq_penalty[irq] directly.
>> 
>> acpi_irq_penalty_update() is for command-line parameters, so it certainly
>> doesn't need the acpi_irq_pci_sharing_penalty() information (the
>> acpi_link_list should be empty at the time we process the command-line
>> parameters).

Calling acpi_irq_get_penalty for ISA IRQ is OK as long as it doesn't have
any dynamic IRQ calculation such that acpi_isa_irq_penalty[irq] = 
acpi_irq_get_penalty.

If this is broken, then we need special care so that we don't assign
dynamically calcualted sci_penalty back to acpi_isa_irq_penalty[irq]. This
results in returning incorrect penalty as

acpi_irq_get_penalty = acpi_isa_irq_original_penalty[irq] + 2 * sci_penalty.

Now that we added sci_penalty into the acpi_irq_get_penalty function,
calling acpi_irq_get_penalty is not correct anymore. This line here needs to
be replaced with acpi_isa_irq_penalty[irq] as you suggested.

if (used)
new_penalty = acpi_irq_get_penalty(irq) +
PIRQ_PENALTY_ISA_USED;
else
new_penalty = 0;

acpi_isa_irq_penalty[irq] = new_penalty;


>> 
>> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
>> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
>> the acpi_irq_pci_sharing_penalty() value at all.
>> 

Same problem here. This line will be broken after the sci_penalty change.

acpi_isa_irq_penalty[irq] = acpi_irq_get_penalty(irq) +
  (active ? PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);




-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

On 10/18/2016 6:59 AM, Bjorn Helgaas wrote:
>> However, this function only gets called if the IRQ number is greater than
>> > 16 and acpi_irq_get_penalty function gets called before ACPI start in
>> > acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
>> > on iterating the link list.

Maybe, I am missing context here. I can add this paragraph to the commit. 

When we started cleaning the code we got rid of the acpi_irq_penalty_init 
function
in favor of acpi_irq_pci_sharing_penalty function as it does have some fair
amount of code duplication.

I tried putting back the acpi_irq_pci_sharing_penalty function into the ISA
IRQ path again during the debug and the machine died way too early. We couldn't
collect any debug message.

This is telling me that we can't even iterate the link list when these two API
is called. ISA IRQ need to be handled with special care due to calling order.


> It seems wrong to me that we call acpi_irq_get_penalty() from
> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
> should just manipulate acpi_isa_irq_penalty[irq] directly.
> 
> acpi_irq_penalty_update() is for command-line parameters, so it certainly
> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> acpi_link_list should be empty at the time we process the command-line
> parameters).
> 
> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
> the acpi_irq_pci_sharing_penalty() value at all.
> 

acpi_irq_get_penalty function knows how to deal with ISA IRQ. So, it is harmless
to call it. Also, reading the acpi_isa_irq_penalty array directly isn't also 
right.
It doesn't contain the SCI penalty. So, it returns incorrect penalty value.

The rule of thumb is:
- all PCI/SCI penalty reads need to go through acpi_isa_irq_penalty function
- all ISA penalty writes need to go through acpi_isa_irq_penalty array directly.
- we do not support modifying the PCI IRQ penalties greater than the ISA IRQ 
numbers.
The original code supported this.


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

On 10/18/2016 6:59 AM, Bjorn Helgaas wrote:
>> However, this function only gets called if the IRQ number is greater than
>> > 16 and acpi_irq_get_penalty function gets called before ACPI start in
>> > acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
>> > on iterating the link list.

Maybe, I am missing context here. I can add this paragraph to the commit. 

When we started cleaning the code we got rid of the acpi_irq_penalty_init 
function
in favor of acpi_irq_pci_sharing_penalty function as it does have some fair
amount of code duplication.

I tried putting back the acpi_irq_pci_sharing_penalty function into the ISA
IRQ path again during the debug and the machine died way too early. We couldn't
collect any debug message.

This is telling me that we can't even iterate the link list when these two API
is called. ISA IRQ need to be handled with special care due to calling order.


> It seems wrong to me that we call acpi_irq_get_penalty() from
> acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
> should just manipulate acpi_isa_irq_penalty[irq] directly.
> 
> acpi_irq_penalty_update() is for command-line parameters, so it certainly
> doesn't need the acpi_irq_pci_sharing_penalty() information (the
> acpi_link_list should be empty at the time we process the command-line
> parameters).
> 
> acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
> the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
> the acpi_irq_pci_sharing_penalty() value at all.
> 

acpi_irq_get_penalty function knows how to deal with ISA IRQ. So, it is harmless
to call it. Also, reading the acpi_isa_irq_penalty array directly isn't also 
right.
It doesn't contain the SCI penalty. So, it returns incorrect penalty value.

The rule of thumb is:
- all PCI/SCI penalty reads need to go through acpi_isa_irq_penalty function
- all ISA penalty writes need to go through acpi_isa_irq_penalty array directly.
- we do not support modifying the PCI IRQ penalties greater than the ISA IRQ 
numbers.
The original code supported this.


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Bjorn Helgaas]

On Sat, Oct 15, 2016 at 12:31:05AM -0400, Sinan Kaya wrote:
> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed PCI_USING penalty from
> acpi_pci_link_allocate function as there is no longer a fixed size penalty
> array for both PCI interrupts greater than 16.
> 
> The array size has been reduced to 16 and array name got prefixed as ISA
> since it only is accountable for the ISA interrupts.
> 
> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed penalty assignment in the code for PCI
> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
> function.
> 
> However, this function only gets called if the IRQ number is greater than
> 16 and acpi_irq_get_penalty function gets called before ACPI start in
> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
> on iterating the link list.

It seems wrong to me that we call acpi_irq_get_penalty() from
acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
should just manipulate acpi_isa_irq_penalty[irq] directly.

acpi_irq_penalty_update() is for command-line parameters, so it certainly
doesn't need the acpi_irq_pci_sharing_penalty() information (the
acpi_link_list should be empty at the time we process the command-line
parameters).

acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
the acpi_irq_pci_sharing_penalty() value at all.

> We need to add the PCI_USING penalty for ISA interrupts too if the link is
> in use and matches our ISA IRQ number.
> 
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/acpi/pci_link.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index c983bf7..a212709 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
> *link)
>   acpi_device_bid(link->device));
>   return -ENODEV;
>   } else {
> + if (link->irq.active < ACPI_MAX_ISA_IRQS)
> + acpi_isa_irq_penalty[link->irq.active] +=
> + PIRQ_PENALTY_PCI_USING;
> +
>   printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>  acpi_device_name(link->device),
>  acpi_device_bid(link->device), link->irq.active);
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Bjorn Helgaas]

On Sat, Oct 15, 2016 at 12:31:05AM -0400, Sinan Kaya wrote:
> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed PCI_USING penalty from
> acpi_pci_link_allocate function as there is no longer a fixed size penalty
> array for both PCI interrupts greater than 16.
> 
> The array size has been reduced to 16 and array name got prefixed as ISA
> since it only is accountable for the ISA interrupts.
> 
> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed penalty assignment in the code for PCI
> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
> function.
> 
> However, this function only gets called if the IRQ number is greater than
> 16 and acpi_irq_get_penalty function gets called before ACPI start in
> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
> on iterating the link list.

It seems wrong to me that we call acpi_irq_get_penalty() from
acpi_irq_penalty_update() and acpi_penalize_isa_irq().  It seems like they
should just manipulate acpi_isa_irq_penalty[irq] directly.

acpi_irq_penalty_update() is for command-line parameters, so it certainly
doesn't need the acpi_irq_pci_sharing_penalty() information (the
acpi_link_list should be empty at the time we process the command-line
parameters).

acpi_penalize_isa_irq() is telling us that a PNP or ACPI device is using
the IRQ -- this should modify the IRQ's penalty, but it shouldn't depend on
the acpi_irq_pci_sharing_penalty() value at all.

> We need to add the PCI_USING penalty for ISA interrupts too if the link is
> in use and matches our ISA IRQ number.
> 
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/acpi/pci_link.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index c983bf7..a212709 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
> *link)
>   acpi_device_bid(link->device));
>   return -ENODEV;
>   } else {
> + if (link->irq.active < ACPI_MAX_ISA_IRQS)
> + acpi_isa_irq_penalty[link->irq.active] +=
> + PIRQ_PENALTY_PCI_USING;
> +
>   printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>  acpi_device_name(link->device),
>  acpi_device_bid(link->device), link->irq.active);
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sinan Kaya]

Hi Rafael,

On 10/15/2016 8:39 AM, Rafael J. Wysocki wrote:
> On Sat, Oct 15, 2016 at 6:31 AM, Sinan Kaya  wrote:
>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") removed PCI_USING penalty from
>> acpi_pci_link_allocate function as there is no longer a fixed size penalty
>> array for both PCI interrupts greater than 16.
>>
>> The array size has been reduced to 16 and array name got prefixed as ISA
>> since it only is accountable for the ISA interrupts.
>>
>> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") removed penalty assignment in the code for PCI
>> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
>> function.
> 
> I'd write the above this way:
> 
> "Commit 103544d86976 (ACPI,PCI,IRQ: reduce resource requirements)
> dropped the PCI_USING penalty from acpi_pci_link_allocate() with the
> assumption that the penalty will be added later in
> acpi_irq_pci_sharing_penalty()."
> 
> This conveys essentially the same information (up to some irrelevant
> bits), but in a clearer way IMO.
> 
>>
>> However, this function only gets called if the IRQ number is greater than
>> 16 and acpi_irq_get_penalty function gets called before ACPI start in
>> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
>> on iterating the link list.
> 
> "However, acpi_irq_pci_sharing_penalty() is only called for IRQ
> numbers above 15.  Moreover, acpi_irq_get_penalty() is invoked by
> acpi_isa_irq_available() and acpi_penalize_isa_irq() before the ACPI
> initialization and the PCI interrupt links list is not ready at that
> point, so it cannot be relied on when computing the penalty."
> 
>>
>> We need to add the PCI_USING penalty for ISA interrupts too if the link is
>> in use and matches our ISA IRQ number.
> 
> "For this reason, the PCI_USING penalty has to be added in
> acpi_pci_link_allocate() directly if the link has been enabled
> successfully and the IRQ number is within the ISA range."
> 
> IIUC
> 
>>
>> Signed-off-by: Sinan Kaya 
>> ---
>>  drivers/acpi/pci_link.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index c983bf7..a212709 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
>> *link)
>> acpi_device_bid(link->device));
>> return -ENODEV;
>> } else {
>> +   if (link->irq.active < ACPI_MAX_ISA_IRQS)
>> +   acpi_isa_irq_penalty[link->irq.active] +=
>> +   PIRQ_PENALTY_PCI_USING;
>> +
> 
> There's no need to break the line here and I would put the above after
> the printk().
> 
> Or even after the whole "else" branch (which is unnecessary, but let's
> limit changes in this patch).
> 
>> printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>>acpi_device_name(link->device),
>>acpi_device_bid(link->device), link->irq.active);
>> --
> 

Thanks for the feedback. I can resubmit with the comments corrected. I'll wait
until I hear from Bjorn first.

> Thanks,
> Rafael
> 


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Sinan Kaya]

Hi Rafael,

On 10/15/2016 8:39 AM, Rafael J. Wysocki wrote:
> On Sat, Oct 15, 2016 at 6:31 AM, Sinan Kaya  wrote:
>> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") removed PCI_USING penalty from
>> acpi_pci_link_allocate function as there is no longer a fixed size penalty
>> array for both PCI interrupts greater than 16.
>>
>> The array size has been reduced to 16 and array name got prefixed as ISA
>> since it only is accountable for the ISA interrupts.
>>
>> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
>> resource requirements") removed penalty assignment in the code for PCI
>> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
>> function.
> 
> I'd write the above this way:
> 
> "Commit 103544d86976 (ACPI,PCI,IRQ: reduce resource requirements)
> dropped the PCI_USING penalty from acpi_pci_link_allocate() with the
> assumption that the penalty will be added later in
> acpi_irq_pci_sharing_penalty()."
> 
> This conveys essentially the same information (up to some irrelevant
> bits), but in a clearer way IMO.
> 
>>
>> However, this function only gets called if the IRQ number is greater than
>> 16 and acpi_irq_get_penalty function gets called before ACPI start in
>> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
>> on iterating the link list.
> 
> "However, acpi_irq_pci_sharing_penalty() is only called for IRQ
> numbers above 15.  Moreover, acpi_irq_get_penalty() is invoked by
> acpi_isa_irq_available() and acpi_penalize_isa_irq() before the ACPI
> initialization and the PCI interrupt links list is not ready at that
> point, so it cannot be relied on when computing the penalty."
> 
>>
>> We need to add the PCI_USING penalty for ISA interrupts too if the link is
>> in use and matches our ISA IRQ number.
> 
> "For this reason, the PCI_USING penalty has to be added in
> acpi_pci_link_allocate() directly if the link has been enabled
> successfully and the IRQ number is within the ISA range."
> 
> IIUC
> 
>>
>> Signed-off-by: Sinan Kaya 
>> ---
>>  drivers/acpi/pci_link.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index c983bf7..a212709 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
>> *link)
>> acpi_device_bid(link->device));
>> return -ENODEV;
>> } else {
>> +   if (link->irq.active < ACPI_MAX_ISA_IRQS)
>> +   acpi_isa_irq_penalty[link->irq.active] +=
>> +   PIRQ_PENALTY_PCI_USING;
>> +
> 
> There's no need to break the line here and I would put the above after
> the printk().
> 
> Or even after the whole "else" branch (which is unnecessary, but let's
> limit changes in this patch).
> 
>> printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>>acpi_device_name(link->device),
>>acpi_device_bid(link->device), link->irq.active);
>> --
> 

Thanks for the feedback. I can resubmit with the comments corrected. I'll wait
until I hear from Bjorn first.

> Thanks,
> Rafael
> 


-- 
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 V3 1/3] ACPI, PCI IRQ: add PCI_USING penalty for ISA interrupts

[Rafael J. Wysocki]

On Sat, Oct 15, 2016 at 6:31 AM, Sinan Kaya  wrote:
> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed PCI_USING penalty from
> acpi_pci_link_allocate function as there is no longer a fixed size penalty
> array for both PCI interrupts greater than 16.
>
> The array size has been reduced to 16 and array name got prefixed as ISA
> since it only is accountable for the ISA interrupts.
>
> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed penalty assignment in the code for PCI
> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
> function.

I'd write the above this way:

"Commit 103544d86976 (ACPI,PCI,IRQ: reduce resource requirements)
dropped the PCI_USING penalty from acpi_pci_link_allocate() with the
assumption that the penalty will be added later in
acpi_irq_pci_sharing_penalty()."

This conveys essentially the same information (up to some irrelevant
bits), but in a clearer way IMO.

>
> However, this function only gets called if the IRQ number is greater than
> 16 and acpi_irq_get_penalty function gets called before ACPI start in
> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
> on iterating the link list.

"However, acpi_irq_pci_sharing_penalty() is only called for IRQ
numbers above 15.  Moreover, acpi_irq_get_penalty() is invoked by
acpi_isa_irq_available() and acpi_penalize_isa_irq() before the ACPI
initialization and the PCI interrupt links list is not ready at that
point, so it cannot be relied on when computing the penalty."

>
> We need to add the PCI_USING penalty for ISA interrupts too if the link is
> in use and matches our ISA IRQ number.

"For this reason, the PCI_USING penalty has to be added in
acpi_pci_link_allocate() directly if the link has been enabled
successfully and the IRQ number is within the ISA range."

IIUC

>
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/acpi/pci_link.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index c983bf7..a212709 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
> *link)
> acpi_device_bid(link->device));
> return -ENODEV;
> } else {
> +   if (link->irq.active < ACPI_MAX_ISA_IRQS)
> +   acpi_isa_irq_penalty[link->irq.active] +=
> +   PIRQ_PENALTY_PCI_USING;
> +

There's no need to break the line here and I would put the above after
the printk().

Or even after the whole "else" branch (which is unnecessary, but let's
limit changes in this patch).

> printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>acpi_device_name(link->device),
>acpi_device_bid(link->device), link->irq.active);
> --

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Sat, Oct 15, 2016 at 6:31 AM, Sinan Kaya  wrote:
> The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed PCI_USING penalty from
> acpi_pci_link_allocate function as there is no longer a fixed size penalty
> array for both PCI interrupts greater than 16.
>
> The array size has been reduced to 16 and array name got prefixed as ISA
> since it only is accountable for the ISA interrupts.
>
> The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
> resource requirements") removed penalty assignment in the code for PCI
> thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
> function.

I'd write the above this way:

"Commit 103544d86976 (ACPI,PCI,IRQ: reduce resource requirements)
dropped the PCI_USING penalty from acpi_pci_link_allocate() with the
assumption that the penalty will be added later in
acpi_irq_pci_sharing_penalty()."

This conveys essentially the same information (up to some irrelevant
bits), but in a clearer way IMO.

>
> However, this function only gets called if the IRQ number is greater than
> 16 and acpi_irq_get_penalty function gets called before ACPI start in
> acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
> on iterating the link list.

"However, acpi_irq_pci_sharing_penalty() is only called for IRQ
numbers above 15.  Moreover, acpi_irq_get_penalty() is invoked by
acpi_isa_irq_available() and acpi_penalize_isa_irq() before the ACPI
initialization and the PCI interrupt links list is not ready at that
point, so it cannot be relied on when computing the penalty."

>
> We need to add the PCI_USING penalty for ISA interrupts too if the link is
> in use and matches our ISA IRQ number.

"For this reason, the PCI_USING penalty has to be added in
acpi_pci_link_allocate() directly if the link has been enabled
successfully and the IRQ number is within the ISA range."

IIUC

>
> Signed-off-by: Sinan Kaya 
> ---
>  drivers/acpi/pci_link.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index c983bf7..a212709 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
> *link)
> acpi_device_bid(link->device));
> return -ENODEV;
> } else {
> +   if (link->irq.active < ACPI_MAX_ISA_IRQS)
> +   acpi_isa_irq_penalty[link->irq.active] +=
> +   PIRQ_PENALTY_PCI_USING;
> +

There's no need to break the line here and I would put the above after
the printk().

Or even after the whole "else" branch (which is unnecessary, but let's
limit changes in this patch).

> printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>acpi_device_name(link->device),
>acpi_device_bid(link->device), link->irq.active);
> --

Thanks,
Rafael

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

[Sinan Kaya]

The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
resource requirements") removed PCI_USING penalty from
acpi_pci_link_allocate function as there is no longer a fixed size penalty
array for both PCI interrupts greater than 16.

The array size has been reduced to 16 and array name got prefixed as ISA
since it only is accountable for the ISA interrupts.

The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
resource requirements") removed penalty assignment in the code for PCI
thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
function.

However, this function only gets called if the IRQ number is greater than
16 and acpi_irq_get_penalty function gets called before ACPI start in
acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
on iterating the link list.

We need to add the PCI_USING penalty for ISA interrupts too if the link is
in use and matches our ISA IRQ number.

Signed-off-by: Sinan Kaya 
---
 drivers/acpi/pci_link.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index c983bf7..a212709 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
*link)
acpi_device_bid(link->device));
return -ENODEV;
} else {
+   if (link->irq.active < ACPI_MAX_ISA_IRQS)
+   acpi_isa_irq_penalty[link->irq.active] +=
+   PIRQ_PENALTY_PCI_USING;
+
printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
   acpi_device_name(link->device),
   acpi_device_bid(link->device), link->irq.active);
-- 
1.9.1

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

[Sinan Kaya]

The change introduced in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
resource requirements") removed PCI_USING penalty from
acpi_pci_link_allocate function as there is no longer a fixed size penalty
array for both PCI interrupts greater than 16.

The array size has been reduced to 16 and array name got prefixed as ISA
since it only is accountable for the ISA interrupts.

The original change in commit 103544d86976 ("ACPI,PCI,IRQ: reduce
resource requirements") removed penalty assignment in the code for PCI
thinking that we will add the penalty later in acpi_irq_pci_sharing_penalty
function.

However, this function only gets called if the IRQ number is greater than
16 and acpi_irq_get_penalty function gets called before ACPI start in
acpi_isa_irq_available and acpi_penalize_isa_irq functions. We can't rely
on iterating the link list.

We need to add the PCI_USING penalty for ISA interrupts too if the link is
in use and matches our ISA IRQ number.

Signed-off-by: Sinan Kaya 
---
 drivers/acpi/pci_link.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index c983bf7..a212709 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -619,6 +619,10 @@ static int acpi_pci_link_allocate(struct acpi_pci_link 
*link)
acpi_device_bid(link->device));
return -ENODEV;
} else {
+   if (link->irq.active < ACPI_MAX_ISA_IRQS)
+   acpi_isa_irq_penalty[link->irq.active] +=
+   PIRQ_PENALTY_PCI_USING;
+
printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
   acpi_device_name(link->device),
   acpi_device_bid(link->device), link->irq.active);
-- 
1.9.1