On Thu, Dec 03, 2015 at 01:58:55PM -0500, Sinan Kaya wrote: > The ACPI compiler uses the extended format when used interrupt numbers > are greater than 15. The extended IRQ is 32 bits according to the ACPI > spec. The code supports parsing the extended interrupt numbers. However, > due to used data structure type; the code silently truncates interrupt > numbers greater than 256. > > First, this patch changes the interrupt number type to 32 bits. Next, the > penalty array has been limited to 16 for ISA IRQs. Finally, a new penalty > linklist has been added for all other interrupts greater than 16. If an IRQ > is not found in the link list, an IRQ info structure will be dynamically > allocated on the first access and will be placed on the list for further > reuse. The list will grow by the number of supported interrupts in the > ACPI table rather than having a 256 hard limitation.
Can you split this into two patches? One to replace the penalty storage scheme, and a second to change the interrupt number types from u8 to u32? Generally looks good to me. Tracking all the penalty information still seems clunky, but I don't have any great ideas of better ways. I have a few minor comments below; when you address them, you can add my: Acked-by: Bjorn Helgaas <[email protected]> > Signed-off-by: Sinan Kaya <[email protected]> > --- > drivers/acpi/pci_link.c | 134 > +++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 104 insertions(+), 30 deletions(-) > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c > index 7c8408b..e10661f 100644 > --- a/drivers/acpi/pci_link.c > +++ b/drivers/acpi/pci_link.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2001, 2002 Andy Grover <[email protected]> > * Copyright (C) 2001, 2002 Paul Diefenbaugh <[email protected]> > * Copyright (C) 2002 Dominik Brodowski <[email protected]> > + * Copyright (c) 2015, The Linux Foundation. All rights reserved. > * > * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > * > @@ -67,12 +68,12 @@ static struct acpi_scan_handler pci_link_handler = { > * later even the link is disable. Instead, we just repick the active irq > */ > struct acpi_pci_link_irq { > - u8 active; /* Current IRQ */ > + u32 active; /* Current IRQ */ > u8 triggering; /* All IRQs */ > u8 polarity; /* All IRQs */ > u8 resource_type; > u8 possible_count; > - u8 possible[ACPI_PCI_LINK_MAX_POSSIBLE]; > + u32 possible[ACPI_PCI_LINK_MAX_POSSIBLE]; > u8 initialized:1; > u8 reserved:7; > }; > @@ -437,8 +438,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, > int irq) > * enabled system. > */ > > -#define ACPI_MAX_IRQS 256 > -#define ACPI_MAX_ISA_IRQ 16 > + #define ACPI_MAX_ISA_IRQ 16 Extra leading space here. > #define PIRQ_PENALTY_PCI_AVAILABLE (0) > #define PIRQ_PENALTY_PCI_POSSIBLE (16*16) > @@ -447,7 +447,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, > int irq) > #define PIRQ_PENALTY_ISA_USED (16*16*16*16*16) > #define PIRQ_PENALTY_ISA_ALWAYS (16*16*16*16*16*16) > > -static int acpi_irq_penalty[ACPI_MAX_IRQS] = { > +static int acpi_irq_isa_penalty[ACPI_MAX_ISA_IRQ] = { > PIRQ_PENALTY_ISA_ALWAYS, /* IRQ0 timer */ > PIRQ_PENALTY_ISA_ALWAYS, /* IRQ1 keyboard */ > PIRQ_PENALTY_ISA_ALWAYS, /* IRQ2 cascade */ > @@ -464,9 +464,61 @@ static int acpi_irq_penalty[ACPI_MAX_IRQS] = { > PIRQ_PENALTY_ISA_USED, /* IRQ13 fpe, sometimes */ > PIRQ_PENALTY_ISA_USED, /* IRQ14 ide0 */ > PIRQ_PENALTY_ISA_USED, /* IRQ15 ide1 */ > - /* >IRQ15 */ > }; > > +struct irq_penalty_info { > + unsigned int irq; > + int penalty; > + struct list_head node; > +}; > + > +LIST_HEAD(acpi_irq_penalty_list); Should be static. > +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; > + } > + > + return 0; > +} > + > +static int acpi_irq_set_penalty(int irq, unsigned int new_penalty) "int new_penalty" to match irq_info->penalty and acpi_irq_get_penalty() return type. > +{ > + 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; > + } > + } > + > + /* 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); > + > + return 0; > +} An "acpi_irq_add_penalty(int irq, int penalty)" here would simplify most of the calls below: static void acpi_irq_add_penalty(int irq, int penalty) { int current = acpi_irq_get_penalty(irq); acpi_irq_set_penalty(irq, current + penalty); } > + > int __init acpi_irq_penalty_init(void) > { > struct acpi_pci_link *link; > @@ -487,15 +539,22 @@ int __init acpi_irq_penalty_init(void) > link->irq.possible_count; > > for (i = 0; i < link->irq.possible_count; i++) { > - if (link->irq.possible[i] < ACPI_MAX_ISA_IRQ) > - acpi_irq_penalty[link->irq. > - possible[i]] += > - penalty; > + if (link->irq.possible[i] < ACPI_MAX_ISA_IRQ) { > + int irqpos = link->irq.possible[i]; > + int curpen; > + > + curpen = acpi_irq_get_penalty(irqpos); > + curpen += penalty; > + acpi_irq_set_penalty(irqpos, curpen); acpi_irq_add_penalty(link->irq.possible[i], penalty); > + } > } > > } else if (link->irq.active) { You didn't change this, but the "else" here looks wrong to me: if we got any IRQs from _PRS, we never add PIRQ_PENALTY_PCI_POSSIBLE to the active IRQ. It also seems wrong that we loop through everything on acpi_link_list. It would be better if we could do this for each link as it is enumerated in acpi_pci_link_add(), so any hot-added links would be handled the same way. These are both pre-existing issues/questions, so I don't think you're obligated to address them. > - acpi_irq_penalty[link->irq.active] += > - PIRQ_PENALTY_PCI_POSSIBLE; > + int curpen; > + > + curpen = acpi_irq_get_penalty(link->irq.active); > + curpen += PIRQ_PENALTY_PCI_POSSIBLE; > + acpi_irq_set_penalty(link->irq.active, curpen); > } > } > > @@ -547,12 +606,12 @@ static int acpi_pci_link_allocate(struct acpi_pci_link > *link) > * the use of IRQs 9, 10, 11, and >15. > */ > for (i = (link->irq.possible_count - 1); i >= 0; i--) { > - if (acpi_irq_penalty[irq] > > - acpi_irq_penalty[link->irq.possible[i]]) > + if (acpi_irq_get_penalty(irq) > > + acpi_irq_get_penalty(link->irq.possible[i])) > irq = link->irq.possible[i]; > } > } > - if (acpi_irq_penalty[irq] >= PIRQ_PENALTY_ISA_ALWAYS) { > + if (acpi_irq_get_penalty(irq) >= PIRQ_PENALTY_ISA_ALWAYS) { > printk(KERN_ERR PREFIX "No IRQ available for %s [%s]. " > "Try pci=noacpi or acpi=off\n", > acpi_device_name(link->device), > @@ -568,7 +627,12 @@ static int acpi_pci_link_allocate(struct acpi_pci_link > *link) > acpi_device_bid(link->device)); > return -ENODEV; > } else { > - acpi_irq_penalty[link->irq.active] += PIRQ_PENALTY_PCI_USING; > + int curpen; > + > + curpen = acpi_irq_get_penalty(link->irq.active); > + curpen += PIRQ_PENALTY_PCI_USING; > + acpi_irq_set_penalty(link->irq.active, curpen); > + > printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n", > acpi_device_name(link->device), > acpi_device_bid(link->device), link->irq.active); > @@ -778,7 +842,7 @@ static void acpi_pci_link_remove(struct acpi_device > *device) > } > > /* > - * modify acpi_irq_penalty[] from cmdline > + * modify penalty from cmdline > */ > static int __init acpi_irq_penalty_update(char *str, int used) > { > @@ -796,13 +860,15 @@ static int __init acpi_irq_penalty_update(char *str, > int used) > if (irq < 0) > continue; > > - if (irq >= ARRAY_SIZE(acpi_irq_penalty)) > - continue; > + if (used) { > + int curpen; > > - if (used) > - acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_USED; > + curpen = acpi_irq_get_penalty(irq); > + curpen += PIRQ_PENALTY_ISA_USED; > + acpi_irq_set_penalty(irq, curpen); > + } > else > - acpi_irq_penalty[irq] = PIRQ_PENALTY_PCI_AVAILABLE; > + acpi_irq_set_penalty(irq, PIRQ_PENALTY_PCI_AVAILABLE); > > if (retval != 2) /* no next number */ > break; > @@ -819,18 +885,22 @@ static int __init acpi_irq_penalty_update(char *str, > int used) > */ > void acpi_penalize_isa_irq(int irq, int active) > { > - if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) { > + if (irq >= 0) { I would structure this as: if (irq < 0) return; if (active) acpi_irq_add_penalty(irq, PIRQ_PENALTY_ISA_USED); else acpi_irq_add_penalty(irq, PIRQ_PENALTY_PCI_USING); But that might be just my personal preference. Similarly in acpi_penalize_sci_irq() below. > + int curpen; > + > + curpen = acpi_irq_get_penalty(irq); > if (active) > - acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_USED; > + curpen += PIRQ_PENALTY_ISA_USED; > else > - acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING; > + curpen += PIRQ_PENALTY_PCI_USING; > + acpi_irq_set_penalty(irq, curpen); > } > } > > bool acpi_isa_irq_available(int irq) > { > - return irq >= 0 && (irq >= ARRAY_SIZE(acpi_irq_penalty) || > - acpi_irq_penalty[irq] < PIRQ_PENALTY_ISA_ALWAYS); > + return irq >= 0 && > + (acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS); > } > > /* > @@ -840,12 +910,16 @@ bool acpi_isa_irq_available(int irq) > */ > void acpi_penalize_sci_irq(int irq, int trigger, int polarity) > { > - if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) { > + if (irq >= 0) { > + int curpen; > + > + curpen = acpi_irq_get_penalty(irq); > if (trigger != ACPI_MADT_TRIGGER_LEVEL || > polarity != ACPI_MADT_POLARITY_ACTIVE_LOW) > - acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS; > + curpen += PIRQ_PENALTY_ISA_ALWAYS; > else > - acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING; > + curpen += PIRQ_PENALTY_PCI_USING; > + acpi_irq_set_penalty(irq, curpen); > } > } > > -- > 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 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
