Skip to contentSkip to conversation

ACPI, PCI, irq: support IRQ numbers greater than 256

PATCH In review Sinan Kaya 2 participants 3 messages Dec 3, 2015 – Dec 9, 2015

Subsystems (2)
ACPI PCI
lore
Share & tools
lore

Sinan Kaya

author
#0 lore raw

PATCH v8 1/1 ACPI, PCI, irq: support IRQ numbers greater than 256

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.

Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/pci_link.c | 134 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 30 deletions(-)
drivers/acpi/pci_link.c +104 -30
index 7c8408b..e10661f 100644
@@ -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
 
 #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);
+
+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)
+{
+	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;
+}
+
 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);
+				}
 			}
 
 		} else if (link->irq.active) {
-			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) {
+		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);
 	}
 }
Signature · 2 lines
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
Acked-by on v8 1/1
#1 lore raw
Quoted 6 lines · Sinan Kaya

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.

Quoted 7 lines

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]>
Quoted 33 lines
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/pci_link.c | 134 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 30 deletions(-)
drivers/acpi/pci_link.c +3 -2
index 7c8408b..e10661f 100644
@@ -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;
 };

drivers/acpi/pci_link.c static int acpi_pci_link_set(struct acpi_pci_link *link, int irq) · line 441

Quoted 6 lines
drivers/acpi/pci_link.c +1 -2
@@ -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.

Quoted 15 lines
drivers/acpi/pci_link.c +1 -1
 #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 */

drivers/acpi/pci_link.c static int acpi_irq_penalty[ACPI_MAX_IRQS] = { · line 475

Quoted 10 lines
drivers/acpi/pci_link.c +7 -1
-	/* >IRQ15 */
 };
 
+struct irq_penalty_info {
+	unsigned int irq;
+	int penalty;
+	struct list_head node;
+};
+
+LIST_HEAD(acpi_irq_penalty_list);

Should be static.

Quoted 6 lines
drivers/acpi/pci_link.c +6 -0
+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];

drivers/acpi/pci_link.c static int acpi_irq_penalty[ACPI_MAX_IRQS] = { · line 492

Quoted 10 lines
drivers/acpi/pci_link.c +10 -0
+
+	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.

Quoted 18 lines
drivers/acpi/pci_link.c +18 -0
+{
+	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 */

drivers/acpi/pci_link.c static int acpi_irq_penalty[ACPI_MAX_IRQS] = { · line 520

Quoted 10 lines
drivers/acpi/pci_link.c +10 -0
+	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);

}

Quoted 9 lines
drivers/acpi/pci_link.c +1 -1
+
 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)

drivers/acpi/pci_link.c int __init acpi_irq_penalty_init(void) · line 548

Quoted 10 lines
drivers/acpi/pci_link.c +7 -3
-					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);

drivers/acpi/pci_link.c int __init acpi_irq_penalty_init(void) · line 552

Quoted 4 lines
drivers/acpi/pci_link.c +1 -0
+				}
 			}
 
 		} 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.

Quoted 71 lines
drivers/acpi/pci_link.c +22 -12
-			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;

drivers/acpi/pci_link.c static int __init acpi_irq_penalty_update(char *str, int used) · line 888

Quoted 5 lines
drivers/acpi/pci_link.c +1 -1
@@ -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.

Quoted 51 lines
drivers/acpi/pci_link.c +15 -7
+		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);
 	}
 }
Signature · 8 lines
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/
author
#2 lore raw
Quoted 15 lines · Bjorn Helgaas
Quoted 6 lines · Sinan Kaya

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.

Quoted 7 lines

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.

Quoted 3 lines

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?

I'll post a patch soon

Quoted 7 lines

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

thanks

Quoted 37 lines
Quoted 37 lines
Signed-off-by Sinan Kaya <[email protected]>
---
 drivers/acpi/pci_link.c | 134 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 30 deletions(-)
drivers/acpi/pci_link.c +3 -2
index 7c8408b..e10661f 100644
@@ -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.
  */
Quoted 5 lines

drivers/acpi/pci_link.c static int acpi_pci_link_set(struct acpi_pci_link *link, int irq) · line 441

Quoted 3 lines
drivers/acpi/pci_link.c +1 -2
-#define ACPI_MAX_IRQS		256
-#define ACPI_MAX_ISA_IRQ	16
+ #define ACPI_MAX_ISA_IRQ	16

Extra leading space here.

Done.

Quoted 19 lines
Quoted 19 lines
drivers/acpi/pci_link.c +1 -2
 #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 */
 };
Quoted 9 lines

drivers/acpi/pci_link.c static int acpi_irq_penalty[ACPI_MAX_IRQS] = { · line 475

Quoted 7 lines
drivers/acpi/pci_link.c +7 -0
+struct irq_penalty_info {
+	unsigned int irq;
+	int penalty;
+	struct list_head node;
+};
+
+LIST_HEAD(acpi_irq_penalty_list);

Should be static.

OK

Quoted 12 lines
Quoted 12 lines
drivers/acpi/pci_link.c +11 -0
+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;
+	}
Quoted 8 lines

drivers/acpi/pci_link.c static int acpi_irq_penalty[ACPI_MAX_IRQS] = { · line 492

Quoted 5 lines
drivers/acpi/pci_link.c +5 -0
+
+	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.

Done

Quoted 33 lines
Quoted 19 lines
drivers/acpi/pci_link.c +18 -0
+{
+	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 */

drivers/acpi/pci_link.c static int acpi_irq_penalty[ACPI_MAX_IRQS] = { · line 520

Quoted 10 lines
drivers/acpi/pci_link.c +10 -0
+	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:

Quoted 7 lines
  static void acpi_irq_add_penalty(int irq, int penalty)
  {
    int current = acpi_irq_get_penalty(irq);
    acpi_irq_set_penalty(irq, current + penalty);

}

Good idea.

Quoted 31 lines
Quoted 9 lines
drivers/acpi/pci_link.c +1 -1
+
 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)

drivers/acpi/pci_link.c int __init acpi_irq_penalty_init(void) · line 548

Quoted 10 lines
drivers/acpi/pci_link.c +7 -3
-					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);

drivers/acpi/pci_link.c int __init acpi_irq_penalty_init(void) · line 552

Quoted 4 lines
drivers/acpi/pci_link.c +1 -0
+				}
 			}
 
 		} 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.

Quoted 7 lines

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.

I'll leave them alone for now.

Quoted 83 lines
Quoted 72 lines
drivers/acpi/pci_link.c +22 -12
-			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;

drivers/acpi/pci_link.c static int __init acpi_irq_penalty_update(char *str, int used) · line 888

Quoted 5 lines
drivers/acpi/pci_link.c +1 -1
@@ -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;
Quoted 7 lines
  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.

OK, cleaner.

Quoted 52 lines
Quoted 52 lines
drivers/acpi/pci_link.c +15 -7
+		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);
 	}
 }
Signature · 8 lines
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/
Signature · 4 lines
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