PNP: convert resource options to unified dynamic list, v1

Given a completely unknown PNP device, if it happens to have a modem-like string in its name and it matches a COM port address, we assume it's a modem.

We used to check the address against all the possible resource options for the device. But the device is already configured and enabled, so we only need to check the resources it is actually using. If we matched an address that wasn't currently enabled, we would fail anyway as soon as we attempted to touch the device at that address.

This removes a reference to the struct pnp_dev.{independent,dependent} fields, which I will soon make private to the PNP subsystem.

Signed-off-by Bjorn Helgaas <[email protected]>

Index: work10/drivers/serial/8250_pnp.c
===================================================================

drivers/serial/8250_pnp.c +10 -13

@@ -383,22 +383,19 @@ static int __devinit check_name(char *na
 	return 0;
 }
 
-static int __devinit check_resources(struct pnp_option *option)
+static int __devinit check_resources(struct pnp_dev *dev)
 {
-	struct pnp_option *tmp;
-	if (!option)
+	resource_size_t port, size;
+
+	if (!pnp_port_valid(dev, 0))
 		return 0;
 
-	for (tmp = option; tmp; tmp = tmp->next) {
-		struct pnp_port *port;
-		for (port = tmp->port; port; port = port->next)
-			if ((port->size == 8) &&
-			    ((port->min == 0x2f8) ||
-			     (port->min == 0x3f8) ||
-			     (port->min == 0x2e8) ||
-			     (port->min == 0x3e8)))
-				return 1;
-	}
+	port = pnp_port_start(dev, 0);
+	size = pnp_port_len(dev, 0);
+
+	if (size == 8 &&
+	    (port == 0x2f8 || port == 0x3f8 || port == 0x2e8 || port == 0x3e8))
+		return 1;
 
 	return 0;
 }

@@ -420,10 +417,7 @@ static int __devinit serial_pnp_guess_bo
 		(dev->card && check_name(dev->card->name))))

We used to cry "modem" if the device _could_ be configured using a COM address while after this change we'd only do so if it _was_ configured using one.

My old analog modem for example had 5 dependent sets -- the first with the regular COM port addresses (0x3f8, 0x2f8, 0x3e8, 0x2e8) and a 5th one listing a port range of (IIRC, something like that at least) 0x100 to 0xff8.

So say I'd have a PC with the regular two onboard COM ports at COM1 and COM2 (0x3f8 and 0x2f8) and an additional serial controller providing COM3 and COM4 (0x3e8 and 0x2e8). My modem would then be configured to use 0x100 and this code would no longer trigger while it did use to.

No functional change; just make a couple declarations consistent with the rest of the file.

Signed-off-by Bjorn Helgaas <[email protected]>

PNP previously defined PNP_PORT_FLAG_16BITADDR and PNP_PORT_FLAG_FIXED in a private header file, but put those flags in struct resource.flags fields. Better to make them IORESOURCE_IO_* flags like the existing IRQ, DMA, and MEM flags.

Signed-off-by Bjorn Helgaas <[email protected]>

Nothing outside the PNP subsystem should need access to a device's resource options, so this patch moves the option structure declarations to a private header file.

Signed-off-by Bjorn Helgaas <[email protected]>

This adds a typedef for the IRQ bitmap, which should cause no functional change, but will make it easier to pass a pointer to a bitmap to pnp_register_irq_resource().

Signed-off-by Bjorn Helgaas <[email protected]>

ACPI Address Space Descriptors can be up to 64 bits wide. We should keep track of the whole thing when parsing resource options, so this patch changes PNP port and mem option fields from "unsigned short" and "unsigned int" to "resource_size_t".

Signed-off-by Bjorn Helgaas <[email protected]>

When we fail to assign an I/O or MEM resource, include the min/max in the debug output to help match it with the options.

Signed-off-by Bjorn Helgaas <[email protected]>

If the resource list is empty, say that explicitly. Previously, it was confusing because often the heading was followed by zero resource lines, then some "add resource" lines from auto-assignment, so the "add" lines looked like current resources.

Signed-off-by Bjorn Helgaas <[email protected]>

This patch doesn't change any behavior; it just makes the return values more conventional.

This changes pnp_assign_dma() from a void function to one that returns an int, just like the other assignment functions. For now, at least, pnp_assign_dma() always returns 0 (success), so it appears to never fail, just like before.

Signed-off-by Bjorn Helgaas <[email protected]>

pnp_assign_resources() is static and the only caller checks pnp_can_configure() before calling it, so no need to do it again.

Signed-off-by Bjorn Helgaas <[email protected]>

This patch moves all the option allocations (pnp_mem, pnp_port, etc) into the pnp_register_{mem,port,irq,dma}_resource() functions. This will make it easier to rework the option data structures.

The non-trivial part of this patch is the IRQ handling. The backends have to allocate a local pnp_irq_mask_t bitmap, populate it, and pass a pointer to pnp_register_irq_resource().

Signed-off-by Bjorn Helgaas <[email protected]>

Index: work10/drivers/pnp/isapnp/core.c
===================================================================

drivers/pnp/isapnp/core.c +3 -2

@@ -433,20 +433,23 @@ static void __init isapnp_parse_irq_reso
 					     int size)
 {
 	unsigned char tmp[3];
-	struct pnp_irq *irq;
 	unsigned long bits;
+	int i;
+	pnp_irq_mask_t map;
+	unsigned char flags = IORESOURCE_IRQ_HIGHEDGE;
 
 	isapnp_peek(tmp, size);
-	irq = kzalloc(sizeof(struct pnp_irq), GFP_KERNEL);

Quoted 9 lines · Bjorn Helgaas

This patch moves all the option allocations (pnp_mem, pnp_port, etc) into the pnp_register_{mem,port,irq,dma}_resource() functions. This will make it easier to rework the option data structures.

The non-trivial part of this patch is the IRQ handling. The backends have to allocate a local pnp_irq_mask_t bitmap, populate it, and pass a pointer to pnp_register_irq_resource().

Signed-off-by Bjorn Helgaas <[email protected]>

[ ... ]

Quoted 14 lines

Index: work10/drivers/pnp/isapnp/core.c
===================================================================

drivers/pnp/isapnp/core.c +3 -1

@@ -433,20 +433,23 @@ static void __init isapnp_parse_irq_reso
 					     int size)
 {
 	unsigned char tmp[3];
-	struct pnp_irq *irq;
 	unsigned long bits;
+	int i;
+	pnp_irq_mask_t map;
+	unsigned char flags = IORESOURCE_IRQ_HIGHEDGE;
 

drivers/pnp/isapnp/core.c static void __init isapnp_parse_irq_reso · line 448

Quoted 12 lines

drivers/pnp/isapnp/core.c +6 -4

 	isapnp_peek(tmp, size);
-	irq = kzalloc(sizeof(struct pnp_irq), GFP_KERNEL);
-	if (!irq)
-		return;
 	bits = (tmp[1] << 8) | tmp[0];
-	bitmap_copy(irq->map.bits, &bits, 16);
+
+	bitmap_zero(map.bits, PNP_IRQ_NR);
+	for (i = 0; i < 15; i++)
+		if (bits & (1 << i))
+			__set_bit(i, map.bits);
+

for (i = 0; i < 16; i++) (16 instead of 15) it would seem.

And why not simply:

bitmap_zero(map.bits, PNP_IRQ_NR);
bitmap_copy(map.bits, &bits, 16);

That last bit of the comment same for pnpbios_parse_irq_option(). Should
be fine, no?

ACPI Extended Interrupt Descriptors can encode 32-bit interrupt numbers, so an interrupt number may exceed the size of the bitmap we use to track possible IRQ settings.

To avoid corrupting memory, complain and ignore too-large interrupt numbers.

There's similar code in pnpacpi_parse_irq_option(), but I didn't change that because the small IRQ descriptor can only encode IRQs 0-15, which do not exceed bitmap size.

In the future, we could handle IRQ numbers greater than PNP_IRQ_NR by replacing the bitmap with a table or list.

Signed-off-by Bjorn Helgaas <[email protected]>

No functional change; just rename "data" to something more descriptive.

Signed-off-by Bjorn Helgaas <[email protected]>

This patch adds an IORESOURCE_IRQ_OPTIONAL flag for use when assigning resources to a device. If the flag is set and we are unable to assign an IRQ to the device, we can leave the IRQ disabled but allow the overall resource allocation to succeed.

Some devices request an IRQ, but can run without an IRQ (possibly with degraded performance). This flag lets us run the device without the IRQ instead of just leaving the device disabled.

This is a reimplementation of this previous change by Rene Herman <[email protected]>:

    http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3b73a223661ed137c5d3d2635f954382e94f5a43

I reimplemented this for two reasons:
- to prepare for converting all resource options into a single linked list, as opposed to the per-resource-type lists we have now, and
- to preserve the order and number of resource options.

In PNPBIOS and ACPI, we configure a device by giving firmware a list of resource assignments. It is important that this list has exactly the same number of resources, in the same order, as the "template" list we got from the firmware in the first place.

The problem of a sound card being left disabled for want of an IRQ was reported by Uwe Bugla <[email protected]>.

Signed-off-by Bjorn Helgaas <[email protected]>

Index: work10/include/linux/ioport.h
===================================================================

include/linux/ioport.h +0 -20

@@ -164,10 +176,6 @@ static void quirk_ad1815_mpu_resources(s
 	struct pnp_option *res;
 	struct pnp_irq *irq;
 
-	/*
-	 * Distribute the independent IRQ over the dependent options
-	 */
-
 	res = dev->independent;
 	if (!res)
 		return;

@@ -176,33 +184,10 @@ static void quirk_ad1815_mpu_resources(s
 	if (!irq || irq->next)
 		return;
 
-	res = dev->dependent;
-	if (!res)
-		return;
-
-	while (1) {
-		struct pnp_irq *copy;
-
-		copy = pnp_alloc(sizeof *copy);
-		if (!copy)
-			break;
-
-		bitmap_copy(copy->map.bits, irq->map.bits, PNP_IRQ_NR);
-		copy->flags = irq->flags;
-
-		copy->next = res->irq; /* Yes, this is NULL */
-		res->irq = copy;

Quoted 10 lines

Index: work10/include/linux/ioport.h
===================================================================

include/linux/ioport.h +1 -0

@@ -59,6 +59,7 @@ struct resource_list {
 #define IORESOURCE_IRQ_HIGHLEVEL	(1<<2)
 #define IORESOURCE_IRQ_LOWLEVEL		(1<<3)
 #define IORESOURCE_IRQ_SHAREABLE	(1<<4)
+#define IORESOURCE_IRQ_OPTIONAL		(1<<5)
 

include/linux/ioport.h struct resource_list { · line 65

Quoted 2 lines

include/linux/ioport.h +0 -0

 /* ISA PnP DMA specific bits (IORESOURCE_BITS) */
 #define IORESOURCE_DMA_TYPE_MASK	(3<<0)

Andrew scooped up a patch from a previous reaction of mine that deleted the "ISA " from these comments. I tend to use comments as a great way of avoiding actually reading code (I'm silly like that) and so as to be burned a _bit_ less by it, I think it's good if they're accurate. In this case for example, one might be tempted to assume these bits were specific to drivers/pnp/isapnp.

Quoted 10 lines · Rene Herman

Quoted 10 lines

Index: work10/include/linux/ioport.h
===================================================================

include/linux/ioport.h +1 -0

@@ -59,6 +59,7 @@ struct resource_list {
 #define IORESOURCE_IRQ_HIGHLEVEL	(1<<2)
 #define IORESOURCE_IRQ_LOWLEVEL		(1<<3)
 #define IORESOURCE_IRQ_SHAREABLE	(1<<4)
+#define IORESOURCE_IRQ_OPTIONAL		(1<<5)
 

Quoted 3 lines

include/linux/ioport.h struct resource_list { · line 65

Quoted 2 lines

include/linux/ioport.h +0 -0

 /* ISA PnP DMA specific bits (IORESOURCE_BITS) */
 #define IORESOURCE_DMA_TYPE_MASK	(3<<0)

Andrew scooped up a patch from a previous reaction of mine that deleted the "ISA " from these comments. I tend to use comments as a great way of avoiding actually reading code (I'm silly like that) and so as to be burned a _bit_ less by it, I think it's good if they're accurate. In this case for example, one might be tempted to assume these bits were specific to drivers/pnp/isapnp.

Yes, I see that's pnpacpi-fix-irq-flag-decoding-comment-fix.patch in mmotm. How about if I incorporate that in my series (the one Andrew hasn't yet picked up), since your patch will otherwise conflict with it?

ISAPNP, PNPBIOS, and ACPI describe the "possible resource settings" of a device, i.e., the possibilities an OS bus driver has when it assigns I/O port, MMIO, and other resources to the device.

PNP used to maintain this "possible resource setting" information in one independent option structure and a list of dependent option structures for each device. Each of these option structures had lists of I/O, memory, IRQ, and DMA resources, for example:

dev

    independent options
      ind-io0  -> ind-io1  ...
      ind-mem0 -> ind-mem1 ...
      ...
    dependent option set 0
      dep0-io0  -> dep0-io1  ...
      dep0-mem0 -> dep0-mem1 ...
      ...
    dependent option set 1
      dep1-io0  -> dep1-io1  ...
      dep1-mem0 -> dep1-mem1 ...
      ...
    ...

This data structure was designed for ISAPNP, where the OS configures device resource settings by writing directly to configuration registers. The OS can write the registers in arbitrary order much like it writes PCI BARs.

However, for PNPBIOS and ACPI devices, the OS uses firmware interfaces that perform device configuration, and it is important to pass the desired settings to those interfaces in the correct order. The OS learns the correct order by using firmware interfaces that return the "current resource settings" and "possible resource settings," but the option structures above doesn't store the ordering information.

This patch replaces the independent and dependent lists with a single list of options. For example, a device might have possible resource settings like this:

dev

    options
      ind-io0 -> dep0-io0 -> dep1->io0 -> ind-io1 ...

static void pnp_print_option(pnp_info_buffer_t * buffer, char *space,

drivers/pnp/interface.c +1 -1

-			     struct pnp_option *option, int dep)
+			     struct pnp_option *option)
 

drivers/pnp/isapnp/core.c +0 -1

@@ -659,26 +654,24 @@ static int __init isapnp_create_device(s
 				priority = 0x100 | tmp[0];
 				size = 0;
 			}
-			option = pnp_register_dependent_option(dev, priority);

drivers/pnp/manager.c +25 -68

-static int pnp_assign_resources(struct pnp_dev *dev, int depnum)
+static int pnp_assign_resources(struct pnp_dev *dev, int set)
 {
-	struct pnp_port *port;
-	struct pnp_mem *mem;
-	struct pnp_irq *irq;
-	struct pnp_dma *dma;
+	struct pnp_option *option;
 	int nport = 0, nmem = 0, nirq = 0, ndma = 0;
+	int ret = 0;
 
-	dbg_pnp_show_resources(dev, "before pnp_assign_resources");
+	dev_dbg(&dev->dev, "pnp_assign_resources, try dependent set %d\n", set);
 	mutex_lock(&pnp_res_mutex);
 	pnp_clean_resource_table(dev);
-	if (dev->independent) {
-		dev_dbg(&dev->dev, "assigning independent options\n");
-		port = dev->independent->port;
-		mem = dev->independent->mem;
-		irq = dev->independent->irq;
-		dma = dev->independent->dma;
-		while (port) {
-			if (pnp_assign_port(dev, port, nport) < 0)
-				goto fail;
-			nport++;
-			port = port->next;
-		}
-		while (mem) {
-			if (pnp_assign_mem(dev, mem, nmem) < 0)
-				goto fail;
-			nmem++;
-			mem = mem->next;
-		}
-		while (irq) {
-			if (pnp_assign_irq(dev, irq, nirq) < 0)
-				goto fail;
-			nirq++;
-			irq = irq->next;
-		}
-		while (dma) {
-			if (pnp_assign_dma(dev, dma, ndma) < 0)
-				goto fail;
-			ndma++;
-			dma = dma->next;
-		}
-	}
 
-	if (depnum) {
-		struct pnp_option *dep;
-		int i;
-
-		dev_dbg(&dev->dev, "assigning dependent option %d\n", depnum);
-		for (i = 1, dep = dev->dependent; i < depnum;
-		     i++, dep = dep->next)
-			if (!dep)
-				goto fail;
-		port = dep->port;
-		mem = dep->mem;
-		irq = dep->irq;
-		dma = dep->dma;
-		while (port) {
-			if (pnp_assign_port(dev, port, nport) < 0)
-				goto fail;
-			nport++;
-			port = port->next;
-		}
-		while (mem) {
-			if (pnp_assign_mem(dev, mem, nmem) < 0)
-				goto fail;
-			nmem++;
-			mem = mem->next;
-		}
-		while (irq) {
-			if (pnp_assign_irq(dev, irq, nirq) < 0)
-				goto fail;
-			nirq++;
-			irq = irq->next;
+	list_for_each_entry(option, &dev->options, list) {
+		if (pnp_option_is_dependent(option) &&
+		    pnp_option_set(option) != set)
+				continue;
+
+		switch (option->type) {
+		case IORESOURCE_IO:
+			ret = pnp_assign_port(dev, &option->u.port, nport++);
+			break;
+		case IORESOURCE_MEM:
+			ret = pnp_assign_mem(dev, &option->u.mem, nmem++);
+			break;
+		case IORESOURCE_IRQ:
+			ret = pnp_assign_irq(dev, &option->u.irq, nirq++);
+			break;
+		case IORESOURCE_DMA:
+			ret = pnp_assign_dma(dev, &option->u.dma, ndma++);
+			break;
+		default:
+			ret = -EINVAL;
+			break;
 		}
-		while (dma) {

drivers/pnp/manager.c +6 -10

 	mutex_unlock(&pnp_res_mutex);
-	dbg_pnp_show_resources(dev, "after pnp_assign_resources");
-	return 1;
-
-fail:
-	pnp_clean_resource_table(dev);
-	mutex_unlock(&pnp_res_mutex);
-	dbg_pnp_show_resources(dev, "after pnp_assign_resources (failed)");
-	return 0;
+	if (ret == 0)
+		dbg_pnp_show_resources(dev, "pnp_assign_resources succeeded");
+	else
+		dev_dbg(&dev->dev, "pnp_assign_resources failed (%d)\n", ret);
+	return ret;
 }
 
 /**

@@ -332,29 +282,21 @@ fail:
  */
 int pnp_auto_config_dev(struct pnp_dev *dev)
 {
-	struct pnp_option *dep;
-	int i = 1;
+	int i, ret = 0;
 
 	if (!pnp_can_configure(dev)) {

Index: work10/drivers/pnp/quirks.c
===================================================================

drivers/pnp/quirks.c +36 -14

@@ -5,6 +5,8 @@
  *  when building up the resource structure for the first time.
  *
  *  Copyright (c) 2000 Peter Denison <[email protected]>
+ *  (c) Copyright 2008 Hewlett-Packard Development Company, L.P.
+ *	Bjorn Helgaas <[email protected]>
  *
  *  Heavily based on PCI quirks handling which is
  *

@@ -20,188 +22,208 @@
 #include <linux/kallsyms.h>
 #include "base.h"
 
+static void quirk_awe32_add_ports(struct pnp_dev *dev,
+				  struct pnp_option *option,
+				  unsigned int offset)
+{
+	struct pnp_option *new_option;
+
+	new_option = kmalloc(sizeof(struct pnp_option), GFP_KERNEL);
+	if (!new_option) {
+		dev_err(&dev->dev, "couldn't add ioport region to option set "
+			"%d\n", pnp_option_set(option));
+		return;
+	}
+
+	*new_option = *option;
+	new_option->u.port.min += offset;
+	new_option->u.port.max += offset;
+	list_add(&new_option->list, &option->list);
+
+	dev_info(&dev->dev, "added ioport region %#llx-%#llx to set %d\n",
+		(unsigned long long) new_option->u.port.min,
+		(unsigned long long) new_option->u.port.max,
+		pnp_option_set(option));
+}
+
 static void quirk_awe32_resources(struct pnp_dev *dev)
 {
-	struct pnp_port *port, *port2, *port3;
-	struct pnp_option *res = dev->dependent;
+	struct pnp_option *option;
+	unsigned int set = ~0;
 
 	/*
-	 * Unfortunately the isapnp_add_port_resource is too tightly bound
-	 * into the PnP discovery sequence, and cannot be used. Link in the
-	 * two extra ports (at offset 0x400 and 0x800 from the one given) by
-	 * hand.
+	 * Add two extra ioport regions (at offset 0x400 and 0x800 from the
+	 * one given) to every dependent option set.
 	 */
-	for (; res; res = res->next) {
-		port2 = pnp_alloc(sizeof(struct pnp_port));
-		if (!port2)
-			return;
-		port3 = pnp_alloc(sizeof(struct pnp_port));
-		if (!port3) {
-			kfree(port2);
-			return;
+	list_for_each_entry(option, &dev->options, list) {
+		if (pnp_option_is_dependent(option) &&
+		    pnp_option_set(option) != set) {
+			set = pnp_option_set(option);
+			quirk_awe32_add_ports(dev, option, 0x800);
+			quirk_awe32_add_ports(dev, option, 0x400);

drivers/pnp/quirks.c +11 -16

-		dev_info(&dev->dev,
-			"AWE32 quirk - added ioports 0x%lx and 0x%lx\n",
-			(unsigned long)port2->min,
-			(unsigned long)port3->min);
 	}
 }
 
 static void quirk_cmi8330_resources(struct pnp_dev *dev)
 {
-	struct pnp_option *res = dev->dependent;
-	unsigned long tmp;
-
-	for (; res; res = res->next) {
-
-		struct pnp_irq *irq;
-		struct pnp_dma *dma;
+	struct pnp_option *option;
+	struct pnp_irq *irq;
+	struct pnp_dma *dma;
 
-		for (irq = res->irq; irq; irq = irq->next) {	// Valid irqs are 5, 7, 10
-			tmp = 0x04A0;
-			bitmap_copy(irq->map.bits, &tmp, 16);	// 0000 0100 1010 0000
-		}
+	list_for_each_entry(option, &dev->options, list) {
+		if (!pnp_option_is_dependent(option))
+			continue;
 
-		for (dma = res->dma; dma; dma = dma->next)	// Valid 8bit dma channels are 1,3
+		if (option->type == IORESOURCE_IRQ) {
+			irq = &option->u.irq;
+			bitmap_zero(irq->map.bits, PNP_IRQ_NR);
+			__set_bit(5, irq->map.bits);
+			__set_bit(7, irq->map.bits);

drivers/pnp/quirks.c +3 -2

+			}
+		}
 	}
-	dev_info(&dev->dev, "CMI8330 quirk - forced possible IRQs to 5, 7, 10 "
-		"and DMA channels to 1, 3\n");
 }
 
 static void quirk_sb16audio_resources(struct pnp_dev *dev)
 {
+	struct pnp_option *option;

drivers/pnp/quirks.c +10 -1

+static void quirk_ad1815_mpu_resources(struct pnp_dev *dev)
 {
-	struct pnp_option *res;
+	struct pnp_option *option, *irq_option = NULL;
+	unsigned int independent_irqs = 0;
+
+	list_for_each_entry(option, &dev->options, list) {
+		if (option->type == IORESOURCE_IRQ &&
+		    !pnp_option_is_dependent(option)) {
+			independent_irqs++;
+			irq_option = option;
+		}

Index: work10/drivers/pnp/resource.c
===================================================================

drivers/pnp/resource.c +5 -2

@@ -3,6 +3,8 @@
  *
  * based on isapnp.c resource management (c) Jaroslav Kysela <[email protected]>
  * Copyright 2003 Adam Belay <[email protected]>
+ * (c) Copyright 2008 Hewlett-Packard Development Company, L.P.
+ *	Bjorn Helgaas <[email protected]>
  */
 
 #include <linux/module.h>

@@ -28,78 +30,37 @@ static int pnp_reserve_mem[16] = {[0 ...
  * option registration
  */
 
-struct pnp_option *pnp_build_option(int priority)
+struct pnp_option *pnp_build_option(struct pnp_dev *dev,
+					   unsigned long type,
+					   unsigned int option_flags)
 {
-	struct pnp_option *option = pnp_alloc(sizeof(struct pnp_option));

On 31-05-08 00:49, Bjorn Helgaas wrote:

Just for the record -- due to an ISAPnP bug, ISAPnP doesn't actually at the moment deal with independents following dependents at all (it does not reset the option to independent upon seeing an _STAG_ENDDEP tag) and the rework actually fixes this bug. The ISAPnP spec does recommend that independents precede dependents, but does not guarantee.

Might be good to mention this in the log though? In the unlikely event that some piece of ISAPnP hardware out there actually changes behaviour (to "fixed" ...) that might be a hint in looking at it.

drivers/pnp/base.h struct pnp_mem { · line 65

Quoted 5 lines

drivers/pnp/base.h +5 -0

+#define PNP_OPTION_DEPENDENT		0x80000000
+#define PNP_OPTION_SET_MASK		0xff
+#define PNP_OPTION_SET_SHIFT		16
+#define PNP_OPTION_PRIORITY_MASK	0xffff
+#define PNP_OPTION_PRIORITY_SHIFT	0

I checked, and ISAPnP and PnPBIOS definitely limit the priority to 8-bit and ACPI seems to as well meaning the (only) reason for using 16 would be the RES_PRIORITY_INVALID value.

I have on the other hand not encountered text limiting the number of dependents sets to 255 nor do I believe the old code enforced this?

Yes, I suppose this is exceedingly unlikely to be a practical issue.

Quoted 3 lines

static void pnp_print_option(pnp_info_buffer_t * buffer, char *space,

drivers/pnp/interface.c +1 -1

-			     struct pnp_option *option, int dep)
+			     struct pnp_option *option)

drivers/pnp/interface.c static void pnp_print_mem(pnp_info_buffe · line 198

Quoted 10 lines

drivers/pnp/interface.c +10 -0

+	switch (option->type) {
+	case IORESOURCE_IO:
+		pnp_print_port(buffer, space, &option->u.port);
+		break;
+	case IORESOURCE_MEM:
+		pnp_print_mem(buffer, space, &option->u.mem);
+		break;
+	case IORESOURCE_IRQ:
+		pnp_print_irq(buffer, space, &option->u.irq);
+		break;

I believe it would be good if pnp_print_irq() now also displayed "(optional)" if the flag is set.

Quoted 9 lines

drivers/pnp/isapnp/core.c +1 -3

@@ -659,26 +654,24 @@ static int __init isapnp_create_device(s
 				priority = 0x100 | tmp[0];
 				size = 0;
 			}
-			option = pnp_register_dependent_option(dev, priority);
-			if (!option)
-				return 1;
+			option_flags = pnp_dependent_option(dev, priority);
 			break;

drivers/pnp/isapnp/core.c static int __init isapnp_create_device(s · line 662

Quoted 6 lines

drivers/pnp/isapnp/core.c +1 -2

 		case _STAG_ENDDEP:
 			if (size != 0)
 				goto __skip;
-			priority = 0;
-			dev_dbg(&dev->dev, "end dependent options\n");
+			option_flags = pnp_independent_option();

(here is the bugfix)

drivers/pnp/manager.c static void pnp_clean_resource_table(str · line 236

Quoted 4 lines

drivers/pnp/manager.c +1 -1

-static int pnp_assign_resources(struct pnp_dev *dev, int depnum)
+static int pnp_assign_resources(struct pnp_dev *dev, int set)
 {
...

Mmm. Previously when it failed halfway through it re-cleared the resources (at label fail). Does it want to do so now also?

Quoted 26 lines

drivers/pnp/manager.c +6 -10

 	mutex_unlock(&pnp_res_mutex);
-	dbg_pnp_show_resources(dev, "after pnp_assign_resources");
-	return 1;
-
-fail:
-	pnp_clean_resource_table(dev);
-	mutex_unlock(&pnp_res_mutex);
-	dbg_pnp_show_resources(dev, "after pnp_assign_resources (failed)");
-	return 0;
+	if (ret == 0)
+		dbg_pnp_show_resources(dev, "pnp_assign_resources succeeded");
+	else
+		dev_dbg(&dev->dev, "pnp_assign_resources failed (%d)\n", ret);
+	return ret;
 }
 
 /**

@@ -332,29 +282,21 @@ fail:
  */
 int pnp_auto_config_dev(struct pnp_dev *dev)
 {
-	struct pnp_option *dep;
-	int i = 1;
+	int i, ret = 0;
 
 	if (!pnp_can_configure(dev)) {

drivers/pnp/manager.c fail: · line 295

Quoted 10 lines

drivers/pnp/manager.c +3 -2

 		dev_dbg(&dev->dev, "configuration not supported\n");
 		return -ENODEV;
 	}
 
-	if (!dev->dependent) {
-		if (pnp_assign_resources(dev, 0))
+	for (i = 0; i == 0 || i < dev->num_dependent_sets; i++) {
+		ret = pnp_assign_resources(dev, i);
+		if (ret == 0)
 			return 0;

Eeeew. Perhaps:

i = 0;

do {
	ret = pnp_assign_resources(dev, i);
	if (ret == 0)
		return 0;
} while (++i < dev->num_dependent_sets);

drivers/pnp/quirks.c line 51

Quoted 4 lines

drivers/pnp/quirks.c +0 -4

-		port2->min += 0x400;
-		port2->max += 0x400;
-		port3->min += 0x800;
-		port3->max += 0x800;

Why do you do 0x800, 0x400 in that order? Shouldn't it just be 0x400, 0x800 to mimick the old order?

drivers/pnp/quirks.c line 91

Quoted 6 lines

drivers/pnp/quirks.c +4 -1

+		} else if (option->type == IORESOURCE_DMA) {
+			dma = &option->u.dma;
 			if ((dma->flags & IORESOURCE_DMA_TYPE_MASK) ==
-			    IORESOURCE_DMA_8BIT)
+						IORESOURCE_DMA_8BIT &&
+			    dma->map != 0x000A) {

It's a char, so 0x0A? 0x000A makes it looks like it's 16-bit.

drivers/pnp/quirks.c line 96

Quoted 5 lines

drivers/pnp/quirks.c +4 -0

+				dev_info(&dev->dev, "changing possible "
+					 "DMA channel mask in option set %d "
+					 "from %#x to 0xA (1, 3)\n",
+					 pnp_option_set(option), dma->map);
 				dma->map = 0x000A;

And here...

drivers/pnp/quirks.c line 112

Quoted 5 lines

drivers/pnp/quirks.c +0 -0

 	/*
 	 * The default range on the mpu port for these devices is 0x388-0x388.
 	 * Here we increase that range so that two such cards can be
 	 * auto-configured.
 	 */

While you're there -- s/mpu/OPL/.

Quoted 12 lines

drivers/pnp/quirks.c +10 -1

+static void quirk_ad1815_mpu_resources(struct pnp_dev *dev)
 {
-	struct pnp_option *res;
+	struct pnp_option *option, *irq_option = NULL;
+	unsigned int independent_irqs = 0;
+
+	list_for_each_entry(option, &dev->options, list) {
+		if (option->type == IORESOURCE_IRQ &&
+		    !pnp_option_is_dependent(option)) {
+			independent_irqs++;
+			irq_option = option;
+		}

drivers/pnp/quirks.c line 223

Quoted 10 lines

drivers/pnp/quirks.c +3 -4

+	}
 
-	res = dev->dependent;
-	if (!res)
+	if (independent_irqs != 1)
 		return;
 
-	while (res->next)
-		res = res->next;
+	irq_option->flags |= IORESOURCE_IRQ_OPTIONAL;

You're in maze of unsigned variables, all named flags...

irq_option->u.irq.flags |= IORESOURCE_IRQ_OPTIONAL;

This was the thing that made things not work when I tested -- on an AD1815...

drivers/pnp/quirks.c line 226

Quoted 4 lines

drivers/pnp/quirks.c +2 -1

+	dev_info(&dev->dev, "made independent IRQ optional\n");
 
-	res->next = quirk_isapnp_mpu_options(dev);
+	quirk_add_irq_optional_dependent_sets(dev);

And as before, this one be gone.

Quoted 12 lines

drivers/pnp/resource.c +5 -2

-struct pnp_option *pnp_build_option(int priority)
+struct pnp_option *pnp_build_option(struct pnp_dev *dev,
+					   unsigned long type,
+					   unsigned int option_flags)
 {
-	struct pnp_option *option = pnp_alloc(sizeof(struct pnp_option));
+	struct pnp_option *option;
 
+	option = kzalloc(sizeof(struct pnp_option), GFP_KERNEL);
 	if (!option)
 		return NULL;

drivers/pnp/resource.c static int pnp_reserve_mem[16] = {[0 ... · line 41

Quoted 4 lines

drivers/pnp/resource.c +0 -4

-	option->priority = priority & 0xff;
-	/* make sure the priority is valid */
-	if (option->priority > PNP_RES_PRIORITY_FUNCTIONAL)
-		option->priority = PNP_RES_PRIORITY_INVALID;

Dropping this made the 0x100 | prio ISAPnP/PnPBIOS bug appear. That 0x100 should go anyway but perhaps this limiting is still a good idea?

That's definitely backwards. I reversed the sizes, so we'll have 8 bits for the priority byte (including compatibility/performance/ robustness) and 16 bits for the dependent set number. Actually, I made the priority field 12 bits so we'd have space to keep PNP_RES_PRIORITY_INVALID as a truly out-of-band value.

Quoted 6 lines

drivers/pnp/manager.c fail: · line 295

Quoted 4 lines

drivers/pnp/manager.c +3 -0

+	for (i = 0; i == 0 || i < dev->num_dependent_sets; i++) {
+		ret = pnp_assign_resources(dev, i);
+		if (ret == 0)
 			return 0;

Eeeew. Perhaps:

Quoted 6 lines

i = 0;

do {
	ret = pnp_assign_resources(dev, i);
	if (ret == 0)
		return 0;
} while (++i < dev->num_dependent_sets);

Heh :-) I vacillated on that one because I have a personal aversion to "do { ... } while ()", especially with a pre-increment. How would you feel about this alternative?

        ret = pnp_assign_resources(dev, 0);
        if (ret == 0)
                return 0;

        for (i = 1; i < dev->num_dependent_sets; i++) {
                ret = pnp_assign_resources(dev, i);
                if (ret == 0)
                        return 0;
        }

Quoted 1 line

Why do you do 0x800, 0x400 in that order? Shouldn't it just be 0x400, 0x800 to mimick the old order?

I think they do end up in the correct order because I'm passing the same list_head to both list_add() calls, e.g., we'll have something like this:

    io -> ...
    io -> (io + 0x800) -> ...
    io -> (io + 0x400) -> (io + 0x800) -> ...

I need to go back over all your comments and make sure I've addressed them all, then I'll post the revised patches, hopefully tomorrow.

Thanks again for all your work reviewing and testing these. It's been incredibly useful.

The other quirks look good -- but I have the hardware and will test them specifically.

=== 4 === quirk_ad1815_mpu_resources, ADS7151

These results are with the dependent cloning already removed locally.

* pre-quirk:

irq 5,7,2/9,11,12 High-Edge
Dependent: 00 - Priority preferred
port 0x330-0x330, align 0x0, size 0x2, 10-bit address decoding
Dependent: 01 - Priority acceptable
port 0x300-0x300, align 0x0, size 0x2, 10-bit address decoding
Dependent: 02 - Priority functional
port 0x100-0x3fe, align 0x1, size 0x2, 10-bit address decoding

* post-quirk:

irq 5,7,2/9,11,12 High-Edge
Dependent: 00 - Priority preferred
port 0x330-0x330, align 0x0, size 0x2, 10-bit address decoding
Dependent: 01 - Priority acceptable
port 0x300-0x300, align 0x0, size 0x2, 10-bit address decoding
Dependent: 02 - Priority functional
port 0x100-0x3fe, align 0x1, size 0x2, 10-bit address decoding

See why that "(optional)" flag display is good? :-) I ofcourse tested things and the flag works great...

== 5 == quirk_add_irq_optional_dependent_sets, ADS7181

* pre-quirk:

Dependent: 00 - Priority preferred
irq 2/9 High-Edge
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 01 - Priority acceptable
irq 2/9 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 02 - Priority functional
irq 2/9,10,11 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding

* post-quirk:

Dependent: 00 - Priority preferred
irq 2/9 High-Edge
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 01 - Priority acceptable
irq 2/9 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 02 - Priority functional
irq 2/9,10,11 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 03 - Priority functional

irq 2/9 High-Edge
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 04 - Priority functional
irq 2/9 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 05 - Priority functional
irq 2/9,10,11 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding

On 04-06-08 01:52, Rene Herman wrote:

ADS7181 in fact might as well delete the IRQ from the dependents and add "irq 2/9,10,11 High-Edge Optinal" in front as an independent same as ADS7151. That way, all the cloning can go.

Quoted 4 lines · Rene Herman

On 04-06-08 01:52, Rene Herman wrote:

ADS7181 in fact might as well delete the IRQ from the dependents and add
"irq 2/9,10,11 High-Edge Optinal" in front as an independent same as ADS7151. That way, all the cloning can go.

We currently clone for AZT0002 as well as ADS7181. Can we do the same for both? It would be nice to get rid of the cloning code if we can.

Quoted 10 lines · Bjorn Helgaas

Quoted 5 lines · Rene Herman

On 04-06-08 01:52, Rene Herman wrote:

ADS7181 in fact might as well delete the IRQ from the dependents and add "irq 2/9,10,11 High-Edge Optinal" in front as an independent same as ADS7151. That way, all the cloning can go.

We currently clone for AZT0002 as well as ADS7181. Can we do the same for both? It would be nice to get rid of the cloning code if we can.

Yes. AZT0002 (the MPU401 on an AZT2320 chip) is the exact same as ADS7181 (the MPU401 on an AD1816 chip).

IORESOURCE_IRQ_OPTIONAL clears the path for doing things better. I see its dependent 2 can then just go entirely in fact.

Hardware says:

Dependent: 00 - Priority preferred
irq 2/9 High-Edge
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 01 - Priority acceptable
irq 2/9 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 02 - Priority functional
irq 2/9,10,11 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding

We want it to end up as:

irq 2/9,10,11 High-Edge (Optional)
Dependent: 00 - Priority preferred
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 01 - Priority acceptable
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding

So walk dependents deleting IRQs, except last dependent IRQ which is cloned into a new independent (inserted in front would be best for ISAPnP since the spec does recommend this) while making it optional and then just delete the last dependent completely (it would be same as the previous dependent after all).

drivers/pnp/quirks.c +31 -44

 
+static struct pnp_option *pnp_clone_dependent_set(struct pnp_dev *dev,
+						  unsigned int set)
 {
-	struct pnp_option *head = NULL;
-	struct pnp_option *prev = NULL;
-	struct pnp_option *res;
-
-	/*
-	 * Build a functional IRQ-optional variant of each MPU option.
-	 */
-
-	for (res = dev->dependent; res; res = res->next) {
-		struct pnp_option *curr;
-		struct pnp_port *port;
-		struct pnp_port *copy_port;
-		struct pnp_irq *irq;
-		struct pnp_irq *copy_irq;
-
-		port = res->port;
-		irq = res->irq;
-		if (!port || !irq)
-			continue;
+	struct pnp_option *tail = NULL, *first_new_option = NULL;
+	struct pnp_option *option, *new_option;
+	unsigned int flags;
+
+	list_for_each_entry(option, &dev->options, list) {
+		if (pnp_option_is_dependent(option))
+			tail = option;
+	}
+	if (!tail) {
+		dev_err(&dev->dev, "no dependent option sets\n");
+		return NULL;
+	}
 
-		copy_port = pnp_alloc(sizeof *copy_port);
-		if (!copy_port)
-			break;
-
-		copy_irq = pnp_alloc(sizeof *copy_irq);
-		if (!copy_irq) {
-			kfree(copy_port);
-			break;
-		}
+	flags = pnp_dependent_option(dev, PNP_RES_PRIORITY_FUNCTIONAL);
+	list_for_each_entry(option, &dev->options, list) {
+		if (pnp_option_is_dependent(option) &&
+		    pnp_option_set(option) == set) {
+			new_option = kmalloc(sizeof(struct pnp_option),
+					     GFP_KERNEL);
+			if (!new_option) {
+				dev_err(&dev->dev, "couldn't clone dependent "
+					"set %d\n", set);
+				return NULL;
+			}
 
-		*copy_port = *port;
-		copy_port->next = NULL;
+			*new_option = *option;
+			new_option->flags = flags;
+			if (!first_new_option)
+				first_new_option = new_option;
 
-		*copy_irq = *irq;
-		copy_irq->flags |= IORESOURCE_IRQ_OPTIONAL;
-		copy_irq->next = NULL;
-
-		curr = pnp_build_option(PNP_RES_PRIORITY_FUNCTIONAL);
-		if (!curr) {
-			kfree(copy_port);
-			kfree(copy_irq);
-			break;
+			list_add(&new_option->list, &tail->list);
+			tail = new_option;
 		}
-		curr->port = copy_port;
-		curr->irq = copy_irq;
-
-		if (prev)
-			prev->next = curr;

On 31-05-08 00:49, Bjorn Helgaas wrote:

Forgot this comment:

Quoted 83 lines

drivers/pnp/quirks.c +31 -44

 
+static struct pnp_option *pnp_clone_dependent_set(struct pnp_dev *dev,
+						  unsigned int set)
 {
-	struct pnp_option *head = NULL;
-	struct pnp_option *prev = NULL;
-	struct pnp_option *res;
-
-	/*
-	 * Build a functional IRQ-optional variant of each MPU option.
-	 */
-
-	for (res = dev->dependent; res; res = res->next) {
-		struct pnp_option *curr;
-		struct pnp_port *port;
-		struct pnp_port *copy_port;
-		struct pnp_irq *irq;
-		struct pnp_irq *copy_irq;
-
-		port = res->port;
-		irq = res->irq;
-		if (!port || !irq)
-			continue;
+	struct pnp_option *tail = NULL, *first_new_option = NULL;
+	struct pnp_option *option, *new_option;
+	unsigned int flags;
+
+	list_for_each_entry(option, &dev->options, list) {
+		if (pnp_option_is_dependent(option))
+			tail = option;
+	}
+	if (!tail) {
+		dev_err(&dev->dev, "no dependent option sets\n");
+		return NULL;
+	}
 
-		copy_port = pnp_alloc(sizeof *copy_port);
-		if (!copy_port)
-			break;
-
-		copy_irq = pnp_alloc(sizeof *copy_irq);
-		if (!copy_irq) {
-			kfree(copy_port);
-			break;
-		}
+	flags = pnp_dependent_option(dev, PNP_RES_PRIORITY_FUNCTIONAL);
+	list_for_each_entry(option, &dev->options, list) {
+		if (pnp_option_is_dependent(option) &&
+		    pnp_option_set(option) == set) {
+			new_option = kmalloc(sizeof(struct pnp_option),
+					     GFP_KERNEL);
+			if (!new_option) {
+				dev_err(&dev->dev, "couldn't clone dependent "
+					"set %d\n", set);
+				return NULL;
+			}
 
-		*copy_port = *port;
-		copy_port->next = NULL;
+			*new_option = *option;
+			new_option->flags = flags;
+			if (!first_new_option)
+				first_new_option = new_option;
 
-		*copy_irq = *irq;
-		copy_irq->flags |= IORESOURCE_IRQ_OPTIONAL;
-		copy_irq->next = NULL;
-
-		curr = pnp_build_option(PNP_RES_PRIORITY_FUNCTIONAL);
-		if (!curr) {
-			kfree(copy_port);
-			kfree(copy_irq);
-			break;
+			list_add(&new_option->list, &tail->list);
+			tail = new_option;
 		}
-		curr->port = copy_port;
-		curr->irq = copy_irq;
-
-		if (prev)
-			prev->next = curr;

drivers/pnp/quirks.c line 176

Quoted 10 lines

drivers/pnp/quirks.c +1 -6

-		else
-			head = curr;
-		prev = curr;
 	}
-	if (head)
-		dev_info(&dev->dev, "adding IRQ-optional MPU options\n");
 
-	return head;
+	return first_new_option;
 }

This works, but I did the "disconnected chain build" that I did due to finding it particularly clumsy to add to the dependent chain while walking it.

This avoids actual problems due to num_sets = dev->num_dependents_sets in caller and the pnp_option_set(option) == set in the loop (not unlike the first version of the original checked for a present irq) but it's again checking the options it just added itself.

If you feel that's perfectly fine then <shrug> but thought I'd still remark on it in case it wasn't intentional.

This patch series converts the PNP resource option structures to a unified linked list. This preserves resource order, which is important for some devices. There's more detail in the comments for the last patch.

Any comments would be welcome.

This depends on some patches that are in -mm, but not yet upstream. In mmotm, these would probably go after pnp-dont-sort-by-type-in-sys-resources.patch

Quoted 10 lines · Bjorn Helgaas

This patch series converts the PNP resource option structures to a unified linked list. This preserves resource order, which is important for some devices. There's more detail in the comments for the last patch.

Any comments would be welcome.

This depends on some patches that are in -mm, but not yet upstream. In mmotm, these would probably go after pnp-dont-sort-by-type-in-sys-resources.patch

Will look at this in more detail but as first testing feedback -- I need this on top.

Both ISAPnP and PnPBIOS for some or other reason set the priority to 0x100 | prio after which that 0x100 is immediately masked of again in pnp_build_option() leaving just the prio. Your new scheme reserves 16 bits for the priority though meaning the 0x100 survives causing it to be considered "invalid" by at least pnp_option_priority_name() for example.

There cannot be any currently valid reason for the 0x100 it seems given that it's immediately masked of again in pnp_build_option() so let's just get rid of it...