Re: [PATCH V2] acpi, pci, irq: account for early penalty assignment
From: Sinan Kaya
Date: Mon Mar 07 2016 - 11:56:03 EST
On 3/4/2016 1:09 PM, Bjorn Helgaas wrote:
> On Thu, Mar 03, 2016 at 12:29:01PM -0500, Sinan Kaya wrote:
>> On 3/3/2016 10:12 AM, Sinan Kaya wrote:
>>> On 3/3/2016 10:10 AM, Bjorn Helgaas wrote:
>>>> That was my idea, but your minimal patch from last night looks awfully
>>>> attractive, and maybe it's not worth moving it to arch/x86. I do think we
>>>> could simplify the code significantly by getting rid of the kzalloc and
>>>> acpi_irq_penalty_list from acpi_irq_set_penalty(). How about pushing on
>>>> that a little bit first, and see what it looks like then?
>>>
>>> OK. Let me go that direction.
>>>
>>
>> How about this?
>>
>> --
>> Sinan Kaya
>> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>
>> From 6cc33747feb469fe4da2088f34e2c875a36f58f4 Mon Sep 17 00:00:00 2001
>> From: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
>> Date: Thu, 3 Mar 2016 10:14:22 -0500
>> Subject: [PATCH] acpi,pci,irq: account for early penalty assignment
>>
>> ---
>> drivers/acpi/pci_link.c | 77 ++++++++++++++++++++++++++++++-------------------
>> 1 file changed, 47 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index fa28635..09eea42 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -87,6 +87,7 @@ struct acpi_pci_link {
>>
>> static LIST_HEAD(acpi_link_list);
>> static DEFINE_MUTEX(acpi_link_lock);
>> +static int sci_irq, sci_irq_penalty;
>>
>> /* --------------------------------------------------------------------------
>> PCI Link Device Management
>> @@ -466,56 +467,71 @@ static int acpi_irq_isa_penalty[ACPI_MAX_ISA_IRQ] = {
>> PIRQ_PENALTY_ISA_USED, /* IRQ15 ide1 */
>> };
>>
>> -struct irq_penalty_info {
>> - int irq;
>> - int penalty;
>> - struct list_head node;
>> -};
>> +static int acpi_irq_pci_sharing_penalty(int irq)
>> +{
>> + struct acpi_pci_link *link;
>> + int penalty = 0;
>> + bool found = false;
>> +
>> + list_for_each_entry(link, &acpi_link_list, list) {
>> + /*
>> + * If a link is active, penalize its IRQ heavily
>> + * so we try to choose a different IRQ.
>> + */
>> + if (link->irq.active && link->irq.active == irq) {
>> + penalty += PIRQ_PENALTY_PCI_USING;
>> + found = true;
>> + } else {
>> + int i;
>> +
>> + /*
>> + * If a link is inactive, penalize the IRQs it
>> + * might, but not as severely.
>
> s/might/might use/
OK
>
>> + */
>> + for (i = 0; i < link->irq.possible_count; i++) {
>> + if (link->irq.possible[i] == irq) {
>> + penalty += PIRQ_PENALTY_PCI_POSSIBLE /
>> + link->irq.possible_count;
>> + found = true;
>> + }
>> + }
>> + }
>> + }
>>
>> -static LIST_HEAD(acpi_irq_penalty_list);
>> + if (found)
>> + return penalty;
>> +
>> + return PIRQ_PENALTY_PCI_AVAILABLE;
>
> It's a nit, but I don't think "#define PIRQ_PENALTY_PCI_AVAILABLE 0"
> adds any readability. If we dropped that, you could get rid of
> "found" as well, and simply "return penalty" here.
>
Right, I never looked at what PIRQ_PENALTY_PCI_AVAILABLE value is. I was trying to
match what current code is doing. I'll get rid of PIRQ_PENALTY_PCI_AVAILABLE
altogether.
>> +}
>>
>> static int acpi_irq_get_penalty(int irq)
>> {
>> - struct irq_penalty_info *irq_info;
>> -
>> if (irq < ACPI_MAX_ISA_IRQ)
>> return acpi_irq_isa_penalty[irq];
>>
>> - list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
>> - if (irq_info->irq == irq)
>> - return irq_info->penalty;
>> - }
>> + if (irq == sci_irq)
>> + return sci_irq_penalty;
>>
>> - return 0;
>> + return acpi_irq_pci_sharing_penalty(irq);
>
> I think there are two issues here that should be teased apart a bit
> more:
>
> 1) Trigger settings: If the IRQ is configured as anything other than
> level-triggered, active-low, we can't use it at all for a PCI
> interrupt, and we should return an "infinite" penalty. We currently
> increase the penalty for the SCI IRQ if it's not level/low, but
> doesn't it apply to *all* IRQs, not just the SCI IRQ?
>
It makes sense for SCI as it is Intel specific.
Unfortunately, this cannot be done in an arch independent way. Of course,
ARM had to implement its own thing. While level-triggered, active-low is
good for intel world, it is not for the ARM world. ARM uses active-high
level triggered.
Of course, we could do something like this and check for level.
+ if (IS_ENABLED(CONFIG_ARM) || IS_ENABLED(CONFIG_ARM64))
+ polarity = ACPI_ACTIVE_HIGH;
+
Let me know what you think.
> It looks like we do something similar in the pcibios_lookup_irq()
> loop that uses can_request_irq(). If we used can_request_irq() in
> pci_link.c, would that mean we could get rid of the SCI
> trigger/polarity stuff and just keep track of the SCI IRQ? I don't
> know whether the SCI IRQ is hooked into whatever can_request_irq()
> uses, but it seems like it *should* be.
Sorry, you lost me here. We are only tracking sci_irq and sci_penalty.
Do you want to add an additional check here? something like this?
if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
- polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
+ polarity != ACPI_MADT_POLARITY_ACTIVE_LOW) &&
+ !can_request_irq(irq, IRQF_SHARED | IRQF_TRIGGER_LOW))
>
> 2) Sharing with other devices: It seems useful to me to accumulate
> the penalties because even an IRQ in the 0-15 range might be used by
> PCI devices. Wouldn't we want to count those users in addition to
> whatever ISA users there might be? E.g., something like this:
>
> penalty = 0;
>
> if (irq < ACPI_MAX_ISA_IRQ)
> penalty += acpi_irq_isa_penalty[irq];
>
> if (irq == sci_irq)
> penalty += PIRQ_PENALTY_PCI_USING;
>
> penalty += acpi_irq_pci_sharing_penalty(irq);
> return penalty;
>
>> }
Sure, makes sense. Changed it like above.
>>
>> static int acpi_irq_set_penalty(int irq, int new_penalty)
>> {
>> - struct irq_penalty_info *irq_info;
>> -
>> /* see if this is a ISA IRQ */
>> if (irq < ACPI_MAX_ISA_IRQ) {
>> acpi_irq_isa_penalty[irq] = new_penalty;
>> return 0;
>> }
>>
>> - /* next, try to locate from the dynamic list */
>> - list_for_each_entry(irq_info, &acpi_irq_penalty_list, node) {
>> - if (irq_info->irq == irq) {
>> - irq_info->penalty = new_penalty;
>> - return 0;
>> - }
>> + if (irq == sci_irq) {
>> + sci_irq_penalty = new_penalty;
>> + return 0;
>> }
>>
>> - /* nope, let's allocate a slot for this IRQ */
>> - irq_info = kzalloc(sizeof(*irq_info), GFP_KERNEL);
>> - if (!irq_info)
>> - return -ENOMEM;
>> -
>> - irq_info->irq = irq;
>> - irq_info->penalty = new_penalty;
>> - list_add_tail(&irq_info->node, &acpi_irq_penalty_list);
>> -
>> + /*
>> + * This is the remaining PCI IRQs. They are calculated on the
>> + * flight in acpi_irq_get_penalty function.
>> + */
>> return 0;
>> }
>
> If we adopt the idea that we compute the penalties on the fly in
> acpi_irq_get_penalty(), I don't think we need acpi_irq_penalty_init()
> any more. It does basically the same thing acpi_irq_get_penalty()
> would do, and it's confusing to do it twice.
Agreed, I was going to take it out. I didn't want to get on it yet. Emptied
the function now.
>
> The acpi_irq_add_penalty() call in acpi_pci_link_allocate() should go
> away for the same reason.
got it.
>
> The acpi_irq_add_penalty() call in acpi_penalize_sci_irq() should go
> away (assuming we can use can_request_irq() or something similar to
> validate trigger settings).
@@ -917,13 +880,15 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
return;
sci_irq = irq;
+
if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
- polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
+ polarity != ACPI_MADT_POLARITY_ACTIVE_LOW) &&
+ !can_request_irq(irq, IRQF_SHARED | IRQF_TRIGGER_LOW))
penalty = PIRQ_PENALTY_ISA_ALWAYS;
else
penalty = PIRQ_PENALTY_PCI_USING;
- acpi_irq_add_penalty(irq, penalty);
+ sci_penalty = penalty;
>
> I think acpi_irq_add_penalty() itself could go away, since the only
> remaining user would be the command-line processing, which could just
> set the penalty directly.
OK, this is how I changed acpi_penalize_isa_irq.
@@ -894,8 +857,8 @@ static int __init acpi_irq_penalty_update(char *str, int used)
void acpi_penalize_isa_irq(int irq, int active)
{
if (irq >= 0)
- acpi_irq_add_penalty(irq, active ?
- PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
+ acpi_irq_isa_penalty[irq] = active ?
+ PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING;
}
>
>>
>> @@ -900,6 +916,7 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
>> if (irq < 0)
>> return;
>>
>> + sci_irq = irq;
>
> Possibly acpi_penalize_sci_irq() itself could go away, since all we
> really need is the SCI IRQ, and we might be able to just use
> acpi_gbl_FADT.sci_interrupt (I'm not 100% sure sci_interrupt is
> identical to a Linux IRQ number; we'd have to check that).
Is SCI IRQ exclusive in general? We are now keeping track of SCI IRQ and
adding penalty when irq matches sci_irq in get_penalty function.
How do we make sci_irq_penalty and acpi_penalize_sci_irq disappear?
>
>> if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
>> polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
>> penalty = PIRQ_PENALTY_ISA_ALWAYS;
>> --
>> 1.8.2.1
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project