| Message ID | 56D8745D.70509@codeaurora.org |
|---|---|
| State | Changes Requested |
| Headers | show |
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@codeaurora.org> > 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/ > + */ > + 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. > +} > > 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 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. 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; > } > > 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. The acpi_irq_add_penalty() call in acpi_pci_link_allocate() should go away for the same reason. 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). 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. > > @@ -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). > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@codeaurora.org> >> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
[+cc Thomas, irq_get_trigger_type() question below] On Mon, Mar 07, 2016 at 11:55:43AM -0500, Sinan Kaya wrote: > 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: > >> From 6cc33747feb469fe4da2088f34e2c875a36f58f4 Mon Sep 17 00:00:00 2001 > >> From: Sinan Kaya <okaya@codeaurora.org> > >> 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 > >> 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. I'm confused. I don't think SCI is Intel-specific. Per PCI Spec r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low. Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable, level interrupt". Are you saying SCI is active-high on ARM? If so, I don't think that's necessarily a huge problem, although we'd have to audit the ACPI code to make sure we handle it correctly. The point here is that a PCI Interrupt Link can only use an IRQ that is level-triggered, active low. If an IRQ is already set to any other state, whether for an ISA device or for an active-high SCI, we can't use it for a PCI Interrupt Link. It'd be nice if there were a generic way we could figure out what the trigger mode of an IRQ is. I was hoping can_request_irq() was that way, but I don't think it is, because it only looks at IRQF_SHARED, not at IRQF_TRIGGER_LOW. Maybe irq_get_trigger_type() is what we want? > > 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)) I'm saying: - If the IRQ trigger type is anything other than level/low, reject this IRQ, and - If this IRQ is the SCI IRQ, penalize the IRQ as though we have a PCI device already using it. > > 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. You might be able to do this incrementally, in several patches, and I'd prefer that if it's possible. It's much easier to review patches if each one is as small as possible and changes only one thing at a time. > >> @@ -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? I don't think the SCI IRQ needs to be exclusive. The ACPI spec says it should be sharable, as long as the other devices use a compatible trigger mode (level/low per spec). If we know the SCI IRQ, we don't need sci_irq_penalty or acpi_penalize_sci_irq() because we can penalize an IRQ with the PIRQ_PENALTY_PCI_USING, something like this: static int pci_compatible_trigger(int irq) { int type = irq_get_trigger_type(irq); return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE); } static unsigned int acpi_irq_get_penalty(int irq) { unsigned int penalty = 0; if (irq == acpi_gbl_FADT.sci_interrupt) penalty += PIRQ_PENALTY_PCI_USING; penalty += acpi_irq_pci_sharing_penalty(irq); return penalty; } static int acpi_pci_link_allocate(struct acpi_pci_link *link) { unsigned int best = ~0; ... for (i = (link->irq.possible_count - 1); i >= 0; i--) { candidate = link->irq.possible[i]; if (!pci_compatible_trigger(candidate)) continue; penalty = acpi_irq_get_penalty(candidate); if (penalty < best) { irq = candidate; best = penalty; } } ... } This looks racy, because we test irq_get_trigger_type() without any kind of locking, and later acpi_register_gsi() calls irq_create_fwspec_mapping(), which looks like it sets the new trigger type. But I don't know how to fix that. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+cc Thomas for real, sorry] On Mon, Mar 07, 2016 at 06:25:58PM -0600, Bjorn Helgaas wrote: > [+cc Thomas, irq_get_trigger_type() question below] > > On Mon, Mar 07, 2016 at 11:55:43AM -0500, Sinan Kaya wrote: > > 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: > > > >> From 6cc33747feb469fe4da2088f34e2c875a36f58f4 Mon Sep 17 00:00:00 2001 > > >> From: Sinan Kaya <okaya@codeaurora.org> > > >> 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 > > > >> 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. > > I'm confused. I don't think SCI is Intel-specific. Per PCI Spec > r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low. > Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable, > level interrupt". > > Are you saying SCI is active-high on ARM? If so, I don't think that's > necessarily a huge problem, although we'd have to audit the ACPI code > to make sure we handle it correctly. > > The point here is that a PCI Interrupt Link can only use an IRQ that > is level-triggered, active low. If an IRQ is already set to any other > state, whether for an ISA device or for an active-high SCI, we can't > use it for a PCI Interrupt Link. > > It'd be nice if there were a generic way we could figure out what the > trigger mode of an IRQ is. I was hoping can_request_irq() was that > way, but I don't think it is, because it only looks at IRQF_SHARED, > not at IRQF_TRIGGER_LOW. > > Maybe irq_get_trigger_type() is what we want? > > > > 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)) > > I'm saying: > > - If the IRQ trigger type is anything other than level/low, reject > this IRQ, and > > - If this IRQ is the SCI IRQ, penalize the IRQ as though we have a > PCI device already using it. > > > > 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. > > You might be able to do this incrementally, in several patches, and > I'd prefer that if it's possible. It's much easier to review patches > if each one is as small as possible and changes only one thing at a > time. > > > >> @@ -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? > > I don't think the SCI IRQ needs to be exclusive. The ACPI spec says > it should be sharable, as long as the other devices use a compatible > trigger mode (level/low per spec). > > If we know the SCI IRQ, we don't need sci_irq_penalty or > acpi_penalize_sci_irq() because we can penalize an IRQ with the > PIRQ_PENALTY_PCI_USING, something like this: > > static int pci_compatible_trigger(int irq) > { > int type = irq_get_trigger_type(irq); > > return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE); > } > > static unsigned int acpi_irq_get_penalty(int irq) > { > unsigned int penalty = 0; > > if (irq == acpi_gbl_FADT.sci_interrupt) > penalty += PIRQ_PENALTY_PCI_USING; > > penalty += acpi_irq_pci_sharing_penalty(irq); > return penalty; > } > > static int acpi_pci_link_allocate(struct acpi_pci_link *link) > { > unsigned int best = ~0; > ... > > for (i = (link->irq.possible_count - 1); i >= 0; i--) { > candidate = link->irq.possible[i]; > if (!pci_compatible_trigger(candidate)) > continue; > > penalty = acpi_irq_get_penalty(candidate); > if (penalty < best) { > irq = candidate; > best = penalty; > } > } > ... > } > > This looks racy, because we test irq_get_trigger_type() without any > kind of locking, and later acpi_register_gsi() calls > irq_create_fwspec_mapping(), which looks like it sets the new trigger > type. But I don't know how to fix that. > > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 7 Mar 2016, Bjorn Helgaas wrote: > On Mon, Mar 07, 2016 at 11:55:43AM -0500, Sinan Kaya wrote: > > 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. > > I'm confused. I don't think SCI is Intel-specific. Per PCI Spec > r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low. > Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable, > level interrupt". > > Are you saying SCI is active-high on ARM? If so, I don't think that's > necessarily a huge problem, although we'd have to audit the ACPI code > to make sure we handle it correctly. > > The point here is that a PCI Interrupt Link can only use an IRQ that > is level-triggered, active low. If an IRQ is already set to any other > state, whether for an ISA device or for an active-high SCI, we can't > use it for a PCI Interrupt Link. > > It'd be nice if there were a generic way we could figure out what the > trigger mode of an IRQ is. I was hoping can_request_irq() was that > way, but I don't think it is, because it only looks at IRQF_SHARED, > not at IRQF_TRIGGER_LOW. > > Maybe irq_get_trigger_type() is what we want? Yes, that gives you the trigger typ, if the interrupt is already set up. > static int pci_compatible_trigger(int irq) > { > int type = irq_get_trigger_type(irq); > > return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE); > } > > static unsigned int acpi_irq_get_penalty(int irq) > { > unsigned int penalty = 0; > > if (irq == acpi_gbl_FADT.sci_interrupt) > penalty += PIRQ_PENALTY_PCI_USING; > > penalty += acpi_irq_pci_sharing_penalty(irq); > return penalty; > } > > static int acpi_pci_link_allocate(struct acpi_pci_link *link) > { > unsigned int best = ~0; > ... > > for (i = (link->irq.possible_count - 1); i >= 0; i--) { > candidate = link->irq.possible[i]; > if (!pci_compatible_trigger(candidate)) > continue; > > penalty = acpi_irq_get_penalty(candidate); > if (penalty < best) { > irq = candidate; > best = penalty; > } > } > ... > } > > This looks racy, because we test irq_get_trigger_type() without any > kind of locking, and later acpi_register_gsi() calls > irq_create_fwspec_mapping(), which looks like it sets the new trigger > type. But I don't know how to fix that. Right, if that pci link allocation code can be executed concurrent, then you might end up with problem, but isn't that a problem even without irq_get_trigger_type()? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 08, 2016 at 09:22:13AM +0100, Thomas Gleixner wrote: > On Mon, 7 Mar 2016, Bjorn Helgaas wrote: > > On Mon, Mar 07, 2016 at 11:55:43AM -0500, Sinan Kaya wrote: > > > 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. > > > > I'm confused. I don't think SCI is Intel-specific. Per PCI Spec > > r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low. > > Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable, > > level interrupt". > > > > Are you saying SCI is active-high on ARM? If so, I don't think that's > > necessarily a huge problem, although we'd have to audit the ACPI code > > to make sure we handle it correctly. > > > > The point here is that a PCI Interrupt Link can only use an IRQ that > > is level-triggered, active low. If an IRQ is already set to any other > > state, whether for an ISA device or for an active-high SCI, we can't > > use it for a PCI Interrupt Link. > > > > It'd be nice if there were a generic way we could figure out what the > > trigger mode of an IRQ is. I was hoping can_request_irq() was that > > way, but I don't think it is, because it only looks at IRQF_SHARED, > > not at IRQF_TRIGGER_LOW. > > > > Maybe irq_get_trigger_type() is what we want? > > Yes, that gives you the trigger typ, if the interrupt is already set up. > > > static int pci_compatible_trigger(int irq) > > { > > int type = irq_get_trigger_type(irq); > > > > return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE); > > } > > > > static unsigned int acpi_irq_get_penalty(int irq) > > { > > unsigned int penalty = 0; > > > > if (irq == acpi_gbl_FADT.sci_interrupt) > > penalty += PIRQ_PENALTY_PCI_USING; > > > > penalty += acpi_irq_pci_sharing_penalty(irq); > > return penalty; > > } > > > > static int acpi_pci_link_allocate(struct acpi_pci_link *link) > > { > > unsigned int best = ~0; > > ... > > > > for (i = (link->irq.possible_count - 1); i >= 0; i--) { > > candidate = link->irq.possible[i]; > > if (!pci_compatible_trigger(candidate)) > > continue; > > > > penalty = acpi_irq_get_penalty(candidate); > > if (penalty < best) { > > irq = candidate; > > best = penalty; > > } > > } > > ... > > } > > > > This looks racy, because we test irq_get_trigger_type() without any > > kind of locking, and later acpi_register_gsi() calls > > irq_create_fwspec_mapping(), which looks like it sets the new trigger > > type. But I don't know how to fix that. > > Right, if that pci link allocation code can be executed concurrent, then you > might end up with problem, but isn't that a problem even without > irq_get_trigger_type()? Yes. It's not a new problem, I just noticed it since we're thinking more about the details of what's happening here. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> 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. >> >> I'm confused. I don't think SCI is Intel-specific. Per PCI Spec >> r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low. >> Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable, >> level interrupt". >> >> Are you saying SCI is active-high on ARM? If so, I don't think that's >> necessarily a huge problem, although we'd have to audit the ACPI code >> to make sure we handle it correctly. We don't have an SCI interrupt on ARM. That's why, I assumed it is an Intel specific interrupt. However, all legacy interrupts are active-high level sensitive. This is a limitation of the ARM GIC Interrupt Controller. Here is what a PCI Link object looks like. Device(LN0D){ Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link Name(_UID, 4) Name(_PRS, ResourceTemplate(){ Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {123} }) Method(_DIS) {} Method(_CRS) { Return (_PRS) } Method(_SRS, 1) {} } >> >> The point here is that a PCI Interrupt Link can only use an IRQ that >> is level-triggered, active low. If an IRQ is already set to any other >> state, whether for an ISA device or for an active-high SCI, we can't >> use it for a PCI Interrupt Link. Unfortunately, this still doesn't hold. A patch is long overdue for this series. I'll post v3. We can go from there.
On Tue, Mar 8, 2016 at 8:04 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > >>>>> 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. >>> >>> I'm confused. I don't think SCI is Intel-specific. Per PCI Spec >>> r3.0, sec 2.2.6, PCI interrupts are level-sensitive, asserted low. >>> Per ACPI Spec v3.0, sec 2.1, the SCI is an "active, low, shareable, >>> level interrupt". That's correct. The SCI isn't Intel-specific or even x86-specific for that matter. However, it is not available on hardware-reduced ACPI platforms which are all ARM using ACPI. >>> Are you saying SCI is active-high on ARM? If so, I don't think that's >>> necessarily a huge problem, although we'd have to audit the ACPI code >>> to make sure we handle it correctly. > > We don't have an SCI interrupt on ARM. That's why, I assumed it is an Intel specific > interrupt. However, all legacy interrupts are active-high level sensitive. This is a > limitation of the ARM GIC Interrupt Controller. > > Here is what a PCI Link object looks like. > > Device(LN0D){ > Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link > Name(_UID, 4) > Name(_PRS, ResourceTemplate(){ > Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {123} > }) > Method(_DIS) {} > Method(_CRS) { Return (_PRS) } > Method(_SRS, 1) {} > } > >>> >>> The point here is that a PCI Interrupt Link can only use an IRQ that >>> is level-triggered, active low. If an IRQ is already set to any other >>> state, whether for an ISA device or for an active-high SCI, we can't >>> use it for a PCI Interrupt Link. > > Unfortunately, this still doesn't hold. I think that the original active-low requirement is related to the fact that those IRQ can be shared. If they aren't shared, I guess the point is slightly moot. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On 3/8/2016 2:04 PM, Sinan Kaya wrote: >>> The point here is that a PCI Interrupt Link can only use an IRQ that >>> >> is level-triggered, active low. If an IRQ is already set to any other >>> >> state, whether for an ISA device or for an active-high SCI, we can't >>> >> use it for a PCI Interrupt Link. > Unfortunately, this still doesn't hold. > > A patch is long overdue for this series. I'll post v3. We can go from there. I just posted a new series and changed the title. Let's continue the discussion there. [PATCH 1/4] acpi,pci,irq: reduce resource requirements ...
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. + */ + 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; +} 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); } 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; } @@ -900,6 +916,7 @@ void acpi_penalize_sci_irq(int irq, int trigger, int polarity) if (irq < 0) return; + sci_irq = irq; if (trigger != ACPI_MADT_TRIGGER_LEVEL || polarity != ACPI_MADT_POLARITY_ACTIVE_LOW) penalty = PIRQ_PENALTY_ISA_ALWAYS;