Skip to contentSkip to conversation

PNP: convert resource table to dynamic list, v1

PATCH In review Bjorn Helgaas 3 participants 18 messages May 5, 2008 – May 19, 2008

Subsystems (3)
ACPI Pnp support Isapnp
lore
Share & tools
lore

Bjorn Helgaas

author
#0 lore raw

cover PNP: convert resource table to dynamic list, v1

This series replaces the fixed-size pnp_resource_table with a linked list of resources.

The first patch is in -mm already and should appear in Linus' tree soon, so just skip that patch if necessary.

As a reminder, if anybody tests this and sees problems, the dmesg log with CONFIG_PNP_DEBUG enabled would be a great help to me.

Bjorn

author
#1 lore raw

PATCH 1/8 PNP: set IRQ index in sysfs "set irq" interface

We have to set the ISAPNP register index when setting an IRQ via the sysfs interface. We already do it for IO, MEM, and DMA resources; I just missed the IRQ one.

This should be in 2.6.26.

Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pnp/interface.c +1 -1
Index: work10/drivers/pnp/interface.c
===================================================================
@@ -424,7 +424,7 @@
 				start = simple_strtoul(buf, &buf, 0);
 				pnp_res = pnp_add_irq_resource(dev, start, 0);
 				if (pnp_res)
-					nirq++;
+					pnp_res->index = nirq++;
 				continue;
 			}
 			if (!strnicmp(buf, "dma", 3)) {
author
#2 lore raw

PATCH 2/8 PNP: add detail to debug resource dump

In reply to Bjorn Helgaas

In the debug resource dump, decode the flags and indicate when a resource is disabled or has been automatically assigned.

Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pnp/support.c +26 -8
Index: work10/drivers/pnp/support.c
===================================================================
@@ -63,28 +63,46 @@ void dbg_pnp_show_resources(struct pnp_d
 	for (i = 0; i < PNP_MAX_IRQ; i++) {
 		res = pnp_get_resource(dev, IORESOURCE_IRQ, i);
 		if (res && !(res->flags & IORESOURCE_UNSET))
-			dev_dbg(&dev->dev, "  irq %lld flags %#lx\n",
-				(unsigned long long) res->start, res->flags);
+			dev_dbg(&dev->dev, "  irq %lld flags %#lx%s%s\n",
+				(unsigned long long) res->start, res->flags,
+				res->flags & IORESOURCE_DISABLED ?
+					" DISABLED" : "",
+				res->flags & IORESOURCE_AUTO ?
+					" AUTO" : "");
 	}
 	for (i = 0; i < PNP_MAX_DMA; i++) {
 		res = pnp_get_resource(dev, IORESOURCE_DMA, i);
 		if (res && !(res->flags & IORESOURCE_UNSET))
-			dev_dbg(&dev->dev, "  dma %lld flags %#lx\n",
-				(unsigned long long) res->start, res->flags);
+			dev_dbg(&dev->dev, "  dma %lld flags %#lx%s%s\n",
+				(unsigned long long) res->start, res->flags,
+				res->flags & IORESOURCE_DISABLED ?
+					" DISABLED" : "",
+				res->flags & IORESOURCE_AUTO ?
+					" AUTO" : "");
 	}
 	for (i = 0; i < PNP_MAX_PORT; i++) {
 		res = pnp_get_resource(dev, IORESOURCE_IO, i);
 		if (res && !(res->flags & IORESOURCE_UNSET))
-			dev_dbg(&dev->dev, "  io  %#llx-%#llx flags %#lx\n",
+			dev_dbg(&dev->dev, "  io  %#llx-%#llx flags %#lx"
+				"%s%s\n",
 				(unsigned long long) res->start,
-				(unsigned long long) res->end, res->flags);
+				(unsigned long long) res->end, res->flags,
+				res->flags & IORESOURCE_DISABLED ?
+					" DISABLED" : "",
+				res->flags & IORESOURCE_AUTO ?
+					" AUTO" : "");
 	}
 	for (i = 0; i < PNP_MAX_MEM; i++) {
 		res = pnp_get_resource(dev, IORESOURCE_MEM, i);
 		if (res && !(res->flags & IORESOURCE_UNSET))
-			dev_dbg(&dev->dev, "  mem %#llx-%#llx flags %#lx\n",
+			dev_dbg(&dev->dev, "  mem %#llx-%#llx flags %#lx"
+				"%s%s\n",
 				(unsigned long long) res->start,
-				(unsigned long long) res->end, res->flags);
+				(unsigned long long) res->end, res->flags,
+				res->flags & IORESOURCE_DISABLED ?
+					" DISABLED" : "",
+				res->flags & IORESOURCE_AUTO ?
+					" AUTO" : "");
 	}
 #endif
 }
author
#4 lore raw

PATCH 3/8 PNP: remove pnp_resource.index

In reply to Bjorn Helgaas

We used pnp_resource.index to keep track of which ISAPNP configuration register a resource should be written to. We needed this only to handle the case where a register is disabled but a subsequent register in the same set is enabled.

Rather than explicitly maintaining the pnp_resource.index, this patch adds a resource every time we read an ISAPNP configuration register and marks the resource as IORESOURCE_DISABLED when appropriate. This makes the position in the pnp_resource_table always correspond to the config register index.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/pnp/base.h        |    1 
 drivers/pnp/interface.c   |   20 ++---------
 drivers/pnp/isapnp/core.c |   80 ++++++++++++++--------------------------------
 drivers/pnp/manager.c     |    4 --
 4 files changed, 29 insertions(+), 76 deletions(-)
drivers/pnp/base.h +0 -1
Index: work10/drivers/pnp/base.h
===================================================================
@@ -56,7 +56,6 @@ struct pnp_resource *pnp_get_pnp_resourc
 
 struct pnp_resource {
 	struct resource res;
-	unsigned int index;		/* ISAPNP config register index */
 };
 
 struct pnp_resource_table {
drivers/pnp/interface.c +4 -16
Index: work10/drivers/pnp/interface.c
===================================================================
@@ -320,7 +320,6 @@ pnp_set_current_resources(struct device 
 			  const char *ubuf, size_t count)
 {
 	struct pnp_dev *dev = to_pnp_dev(dmdev);
-	struct pnp_resource *pnp_res;
 	char *buf = (void *)ubuf;
 	int retval = 0;
 	resource_size_t start, end;

@@ -368,7 +367,6 @@ pnp_set_current_resources(struct device 
 		goto done;
 	}
 	if (!strnicmp(buf, "set", 3)) {
-		int nport = 0, nmem = 0, nirq = 0, ndma = 0;
 		if (dev->active)
 			goto done;
 		buf += 3;

@@ -391,10 +389,7 @@ pnp_set_current_resources(struct device 
 					end = simple_strtoul(buf, &buf, 0);
 				} else
 					end = start;
-				pnp_res = pnp_add_io_resource(dev, start, end,
-							      0);
-				if (pnp_res)
-					pnp_res->index = nport++;
+				pnp_add_io_resource(dev, start, end, 0);
 				continue;
 			}
 			if (!strnicmp(buf, "mem", 3)) {

@@ -411,10 +406,7 @@ pnp_set_current_resources(struct device 
 					end = simple_strtoul(buf, &buf, 0);
 				} else
 					end = start;
-				pnp_res = pnp_add_mem_resource(dev, start, end,
-							       0);
-				if (pnp_res)
-					pnp_res->index = nmem++;
+				pnp_add_mem_resource(dev, start, end, 0);
 				continue;
 			}
 			if (!strnicmp(buf, "irq", 3)) {

@@ -422,9 +414,7 @@ pnp_set_current_resources(struct device 
 				while (isspace(*buf))
 					++buf;
 				start = simple_strtoul(buf, &buf, 0);
-				pnp_res = pnp_add_irq_resource(dev, start, 0);
-				if (pnp_res)
-					pnp_res->index = nirq++;
+				pnp_add_irq_resource(dev, start, 0);
 				continue;
 			}
 			if (!strnicmp(buf, "dma", 3)) {

@@ -432,9 +422,7 @@ pnp_set_current_resources(struct device 
 				while (isspace(*buf))
 					++buf;
 				start = simple_strtoul(buf, &buf, 0);
-				pnp_res = pnp_add_dma_resource(dev, start, 0);
-				if (pnp_res)
-					pnp_res->index = ndma++;
+				pnp_add_dma_resource(dev, start, 0);
 				continue;
 			}
 			break;
drivers/pnp/isapnp/core.c +29 -55

133 lines not on this page · raw message

drivers/pnp/manager.c +0 -4
Index: work10/drivers/pnp/manager.c
===================================================================
@@ -40,7 +40,6 @@ static int pnp_assign_port(struct pnp_de
 	}
 
 	/* set the initial values */
-	pnp_res->index = idx;
 	res->flags |= rule->flags | IORESOURCE_IO;
 	res->flags &= ~IORESOURCE_UNSET;
 

@@ -90,7 +89,6 @@ static int pnp_assign_mem(struct pnp_dev
 	}
 
 	/* set the initial values */
-	pnp_res->index = idx;
 	res->flags |= rule->flags | IORESOURCE_MEM;
 	res->flags &= ~IORESOURCE_UNSET;
 

@@ -155,7 +153,6 @@ static int pnp_assign_irq(struct pnp_dev
 	}
 
 	/* set the initial values */
-	pnp_res->index = idx;
 	res->flags |= rule->flags | IORESOURCE_IRQ;
 	res->flags &= ~IORESOURCE_UNSET;
 

@@ -214,7 +211,6 @@ static void pnp_assign_dma(struct pnp_de
 	}
 
 	/* set the initial values */
-	pnp_res->index = idx;
 	res->flags |= rule->flags | IORESOURCE_DMA;
 	res->flags &= ~IORESOURCE_UNSET;
Acked-by on v1 3/8
#5 lore raw

Re: [patch 3/8] PNP: remove pnp_resource.index

Quoted 13 lines · Bjorn Helgaas

We used pnp_resource.index to keep track of which ISAPNP configuration register a resource should be written to. We needed this only to handle the case where a register is disabled but a subsequent register in the same set is enabled.

Rather than explicitly maintaining the pnp_resource.index, this patch adds a resource every time we read an ISAPNP configuration register and marks the resource as IORESOURCE_DISABLED when appropriate. This makes the position in the pnp_resource_table always correspond to the config register index.

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

drivers/pnp/interface.c pnp_set_current_resources(struct device · line 392

Quoted 5 lines
drivers/pnp/interface.c +1 -4
-			pnp_res = pnp_add_io_resource(dev, start, end,
-						      0);
-			if (pnp_res)
-				pnp_res->index = nport++;
+			pnp_add_io_resource(dev, start, end, 0);

In the tree after your v2 series, pnp_add_foo_resource() are called as void functions yet still return a struct pnp_resource *. You might have other plans but if not, I guess they can _be_ void functions?

Otherwise:

Acked-by Rene Herman <[email protected]>

Rene.

author
#6 lore raw

Re: [patch 3/8] PNP: remove pnp_resource.index

Quoted 15 lines · Rene Herman
Quoted 12 lines · Bjorn Helgaas

We used pnp_resource.index to keep track of which ISAPNP configuration register a resource should be written to. We needed this only to handle the case where a register is disabled but a subsequent register in the same set is enabled.

Rather than explicitly maintaining the pnp_resource.index, this patch adds a resource every time we read an ISAPNP configuration register and marks the resource as IORESOURCE_DISABLED when appropriate. This makes the position in the pnp_resource_table always correspond to the config register index.

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

drivers/pnp/interface.c pnp_set_current_resources(struct device · line 392

Quoted 5 lines
drivers/pnp/interface.c +1 -4
-			pnp_res = pnp_add_io_resource(dev, start, end,
-						      0);
-			if (pnp_res)
-				pnp_res->index = nport++;
+			pnp_add_io_resource(dev, start, end, 0);

In the tree after your v2 series, pnp_add_foo_resource() are called as void functions yet still return a struct pnp_resource *. You might have other plans but if not, I guess they can _be_ void functions?

You're right, I still don't do anything with the return value. It could be used to check for success/failure, but we currently don't do that. Possibly a future cleanup since there's no functional problem here.

Bjorn

author
#7 lore raw

PATCH 4/8 PNP: add pnp_resource_type() internal interface

In reply to Bjorn Helgaas

Given a struct resource, this returns the type (IO, MEM, IRQ, DMA).

Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pnp/base.h +1 -0
Index: work10/drivers/pnp/base.h
===================================================================
@@ -45,6 +45,7 @@ int pnp_check_dma(struct pnp_dev *dev, s
 void dbg_pnp_show_resources(struct pnp_dev *dev, char *desc);
 
 void pnp_init_resource(struct resource *res);
+int pnp_resource_type(struct resource *res);
 
 struct pnp_resource *pnp_get_pnp_resource(struct pnp_dev *dev,
 					  unsigned int type, unsigned int num);
drivers/pnp/resource.c +6 -0
Index: work10/drivers/pnp/resource.c
===================================================================
@@ -499,6 +499,12 @@ int pnp_check_dma(struct pnp_dev *dev, s
 #endif
 }
 
+int pnp_resource_type(struct resource *res)
+{
+	return res->flags & (IORESOURCE_IO  | IORESOURCE_MEM |
+			     IORESOURCE_IRQ | IORESOURCE_DMA);
+}
+
 struct pnp_resource *pnp_get_pnp_resource(struct pnp_dev *dev,
 					  unsigned int type, unsigned int num)
 {
author
#9 lore raw

PATCH 5/8 PNP: add pnp_resource_type_name() helper function

In reply to Bjorn Helgaas

This patch adds a "pnp_resource_type_name(struct resource *)" that returns the string resource type. This will be used by the sysfs "show resources" function and the debug resource dump function.

Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pnp/base.h +1 -0
Index: work10/drivers/pnp/base.h
===================================================================
@@ -42,6 +42,7 @@ int pnp_check_mem(struct pnp_dev *dev, s
 int pnp_check_irq(struct pnp_dev *dev, struct resource *res);
 int pnp_check_dma(struct pnp_dev *dev, struct resource *res);
 
+char *pnp_resource_type_name(struct resource *res);
 void dbg_pnp_show_resources(struct pnp_dev *dev, char *desc);
 
 void pnp_init_resource(struct resource *res);
drivers/pnp/support.c +15 -0
Index: work10/drivers/pnp/support.c
===================================================================
@@ -52,6 +52,21 @@ void pnp_eisa_id_to_string(u32 id, char 
 	str[7] = '\0';
 }
 
+char *pnp_resource_type_name(struct resource *res)
+{
+	switch (pnp_resource_type(res)) {
+	case IORESOURCE_IO:
+		return "io";
+	case IORESOURCE_MEM:
+		return "mem";
+	case IORESOURCE_IRQ:
+		return "irq";
+	case IORESOURCE_DMA:
+		return "dma";
+	}
+	return NULL;
+}
+
 void dbg_pnp_show_resources(struct pnp_dev *dev, char *desc)
 {
 #ifdef DEBUG
Acked-by on v1 5/8
#10 lore raw

Re: [patch 5/8] PNP: add pnp_resource_type_name() helper function

Quoted 5 lines · Bjorn Helgaas

This patch adds a "pnp_resource_type_name(struct resource *)" that returns the string resource type. This will be used by the sysfs "show resources" function and the debug resource dump function.

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

Rene.

author
#11 lore raw

PATCH 6/8 PNP: replace pnp_resource_table with dynamically allocated resources

In reply to Bjorn Helgaas

PNP used to have a fixed-size pnp_resource_table for tracking the resources used by a device. This table often overflowed, so we've had to increase the table size, which wastes memory because most devices have very few resources.

This patch replaces the table with a linked list of resources where the entries are allocated on demand.

This removes messages like these:

pnpacpi: exceeded the max number of IO resources 00:01: too many I/O port resources

References:

    http://bugzilla.kernel.org/show_bug.cgi?id=9535
    http://bugzilla.kernel.org/show_bug.cgi?id=9740
    http://lkml.org/lkml/2007/11/30/110

This patch also changes the way PNP uses the IORESOURCE_UNSET, IORESOURCE_AUTO, and IORESOURCE_DISABLED flags.

Prior to this patch, the pnp_resource_table entries used the flags like this:

IORESOURCE_UNSET
This table entry is unused and available for use. When this flag is set, we shouldn't look at anything else in the resource structure. This flag is set when a resource table entry is initialized.

IORESOURCE_AUTO
This resource was assigned automatically by pnp_assign_{io,mem,etc}().

This flag is set when a resource table entry is initialized and cleared whenever we discover a resource setting by reading an ISAPNP config register, parsing a PNPBIOS resource data stream, parsing an ACPI _CRS list, or interpreting a sysfs "set" command.

Resources marked IORESOURCE_AUTO are reinitialized and marked as IORESOURCE_UNSET by pnp_clean_resource_table() in these cases:

- before we attempt to assign resources automatically,
- if we fail to assign resources automatically,
- after disabling a device

IORESOURCE_DISABLED
Set by pnp_assign_{io,mem,etc}() when automatic assignment fails. Also set by PNPBIOS and PNPACPI for:

- invalid IRQs or GSI registration failures
- invalid DMA channels
- I/O ports above 0x10000
- mem ranges with negative length

After this patch, there is no pnp_resource_table, and the resource list entries use the flags like this:

IORESOURCE_UNSET
This flag is no longer used in PNP. Instead of keeping IORESOURCE_UNSET entries in the resource list, we remove entries from the list and free them.

IORESOURCE_AUTO
No change in meaning: it still means the resource was assigned automatically by pnp_assign_{port,mem,etc}(), but these functions now set the bit explicitly.

We still "clean" a device's resource list in the same places, but rather than reinitializing IORESOURCE_AUTO entries, we just remove them from the list.

Note that IORESOURCE_AUTO entries are always at the end of the list, so removing them doesn't reorder other list entries. This is because non-IORESOURCE_AUTO entries are added by the ISAPNP, PNPBIOS, or PNPACPI "get resources" methods and by the sysfs "set" command. In each of these cases, we completely free the resource list first.

IORESOURCE_DISABLED
In addition to the cases where we used to set this flag, ISAPNP now adds an IORESOURCE_DISABLED resource when it reads a configuration register with a "disabled" value.

[Rene, I made isapnp_set_resources() skip IORESOURCE_DISABLED resources, because that's effectively what we used to do. I think it *ought* to work to write zero (or 4 for DMA) back to a configuration register to disable it, and that might even be useful as a way to manually disable a single resource, but for now it seems safer to preserve the previous behavior.]

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/pnp/base.h        |   18 ---
 drivers/pnp/core.c        |   25 +++--
 drivers/pnp/interface.c   |   60 +++++-------
 drivers/pnp/isapnp/core.c |    8 -
 drivers/pnp/manager.c     |  218 +++++++++++-----------------------------------
 drivers/pnp/quirks.c      |    3 
 drivers/pnp/resource.c    |   86 +++---------------
 drivers/pnp/support.c     |   68 +++++---------
 drivers/pnp/system.c      |    8 -
 include/linux/pnp.h       |    5 -
 10 files changed, 150 insertions(+), 349 deletions(-)
drivers/pnp/base.h +3 -15
Index: work10/drivers/pnp/base.h
===================================================================
@@ -45,27 +45,15 @@ int pnp_check_dma(struct pnp_dev *dev, s
 char *pnp_resource_type_name(struct resource *res);
 void dbg_pnp_show_resources(struct pnp_dev *dev, char *desc);
 
-void pnp_init_resource(struct resource *res);
+void pnp_free_resources(struct pnp_dev *dev);
 int pnp_resource_type(struct resource *res);
 
-struct pnp_resource *pnp_get_pnp_resource(struct pnp_dev *dev,
-					  unsigned int type, unsigned int num);
-
-#define PNP_MAX_PORT		40
-#define PNP_MAX_MEM		24
-#define PNP_MAX_IRQ		 2
-#define PNP_MAX_DMA		 2
-
 struct pnp_resource {
+	struct list_head list;
 	struct resource res;
 };
 
-struct pnp_resource_table {
-	struct pnp_resource port[PNP_MAX_PORT];
-	struct pnp_resource mem[PNP_MAX_MEM];
-	struct pnp_resource dma[PNP_MAX_DMA];
-	struct pnp_resource irq[PNP_MAX_IRQ];
-};
+void pnp_free_resource(struct pnp_resource *pnp_res);
 
 struct pnp_resource *pnp_add_irq_resource(struct pnp_dev *dev, int irq,
 					  int flags);
drivers/pnp/core.c +17 -8
Index: work10/drivers/pnp/core.c
===================================================================
@@ -99,6 +99,21 @@ static void pnp_free_ids(struct pnp_dev 
 	}
 }
 
+void pnp_free_resource(struct pnp_resource *pnp_res)
+{
+	list_del(&pnp_res->list);
+	kfree(pnp_res);
+}
+
+void pnp_free_resources(struct pnp_dev *dev)
+{
+	struct pnp_resource *pnp_res, *tmp;
+
+	list_for_each_entry_safe(pnp_res, tmp, &dev->resources, list) {
+		pnp_free_resource(pnp_res);
+	}
+}
+
 static void pnp_release_device(struct device *dmdev)
 {
 	struct pnp_dev *dev = to_pnp_dev(dmdev);

@@ -106,7 +121,7 @@ static void pnp_release_device(struct de
 	pnp_free_option(dev->independent);
 	pnp_free_option(dev->dependent);
 	pnp_free_ids(dev);
-	kfree(dev->res);
+	pnp_free_resources(dev);
 	kfree(dev);
 }
 

@@ -119,12 +134,7 @@ struct pnp_dev *pnp_alloc_dev(struct pnp
 	if (!dev)
 		return NULL;
 
-	dev->res = kzalloc(sizeof(struct pnp_resource_table), GFP_KERNEL);
-	if (!dev->res) {
-		kfree(dev);
-		return NULL;
-	}
-
+	INIT_LIST_HEAD(&dev->resources);
 	dev->protocol = protocol;
 	dev->number = id;
 	dev->dma_mask = DMA_24BIT_MASK;

@@ -140,7 +150,6 @@ struct pnp_dev *pnp_alloc_dev(struct pnp
 
 	dev_id = pnp_add_id(dev, pnpid);
 	if (!dev_id) {
-		kfree(dev->res);
 		kfree(dev);
 		return NULL;
 	}
drivers/pnp/interface.c +26 -34

72 lines not on this page · raw message

drivers/pnp/isapnp/core.c +4 -8
Index: work10/drivers/pnp/isapnp/core.c
===================================================================
@@ -973,8 +973,7 @@ static int isapnp_set_resources(struct p
 	dev->active = 1;
 	for (tmp = 0; tmp < ISAPNP_MAX_PORT; tmp++) {
 		res = pnp_get_resource(dev, IORESOURCE_IO, tmp);
-		if (res && pnp_resource_valid(res) &&
-		    !(res->flags & IORESOURCE_DISABLED)) {
+		if (res && !(res->flags & IORESOURCE_DISABLED)) {
 			dev_dbg(&dev->dev, "  set io  %d to %#llx\n",
 				tmp, (unsigned long long) res->start);
 			isapnp_write_word(ISAPNP_CFG_PORT + (tmp << 1),

@@ -983,8 +982,7 @@ static int isapnp_set_resources(struct p
 	}
 	for (tmp = 0; tmp < ISAPNP_MAX_IRQ; tmp++) {
 		res = pnp_get_resource(dev, IORESOURCE_IRQ, tmp);
-		if (res && pnp_resource_valid(res) &&
-		    !(res->flags & IORESOURCE_DISABLED)) {
+		if (res && !(res->flags & IORESOURCE_DISABLED)) {
 			int irq = res->start;
 			if (irq == 2)
 				irq = 9;

@@ -994,8 +992,7 @@ static int isapnp_set_resources(struct p
 	}
 	for (tmp = 0; tmp < ISAPNP_MAX_DMA; tmp++) {
 		res = pnp_get_resource(dev, IORESOURCE_DMA, tmp);
-		if (res && pnp_resource_valid(res) &&
-		    !(res->flags & IORESOURCE_DISABLED)) {
+		if (res && !(res->flags & IORESOURCE_DISABLED)) {
 			dev_dbg(&dev->dev, "  set dma %d to %lld\n",
 				tmp, (unsigned long long) res->start);
 			isapnp_write_byte(ISAPNP_CFG_DMA + tmp, res->start);

@@ -1003,8 +1000,7 @@ static int isapnp_set_resources(struct p
 	}
 	for (tmp = 0; tmp < ISAPNP_MAX_MEM; tmp++) {
 		res = pnp_get_resource(dev, IORESOURCE_MEM, tmp);
-		if (res && pnp_resource_valid(res) &&
-		    !(res->flags & IORESOURCE_DISABLED)) {
+		if (res && !(res->flags & IORESOURCE_DISABLED)) {
 			dev_dbg(&dev->dev, "  set mem %d to %#llx\n",
 				tmp, (unsigned long long) res->start);
 			isapnp_write_word(ISAPNP_CFG_MEM + (tmp << 3),
drivers/pnp/manager.c +54 -164

345 lines not on this page · raw message

drivers/pnp/quirks.c +1 -2
Index: work10/drivers/pnp/quirks.c
===================================================================
@@ -141,8 +141,7 @@ static void quirk_system_pci_resources(s
 			for (j = 0;
 			     (res = pnp_get_resource(dev, IORESOURCE_MEM, j));
 			     j++) {
-				if (res->flags & IORESOURCE_UNSET ||
-				    (res->start == 0 && res->end == 0))
+				if (res->start == 0 && res->end == 0)
 					continue;
 
 				pnp_start = res->start;
drivers/pnp/resource.c +18 -68

134 lines not on this page · raw message

drivers/pnp/support.c +24 -44

78 lines not on this page · raw message

drivers/pnp/system.c +1 -7
Index: work10/drivers/pnp/system.c
===================================================================
@@ -60,8 +60,6 @@ static void reserve_resources_of_dev(str
 	int i;
 
 	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) {
-		if (res->flags & IORESOURCE_UNSET)
-			continue;
 		if (res->start == 0)
 			continue;	/* disabled */
 		if (res->start < 0x100)

@@ -80,12 +78,8 @@ static void reserve_resources_of_dev(str
 		reserve_range(dev, res->start, res->end, 1);
 	}
 
-	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_MEM, i)); i++) {
-		if (res->flags & IORESOURCE_UNSET)
-			continue;
-
+	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_MEM, i)); i++)
 		reserve_range(dev, res->start, res->end, 0);
-	}
 }
 
 static int system_pnp_probe(struct pnp_dev *dev,
include/linux/pnp.h +2 -3
Index: work10/include/linux/pnp.h
===================================================================
@@ -15,7 +15,6 @@
 
 struct pnp_protocol;
 struct pnp_dev;
-struct pnp_resource_table;
 
 /*
  * Resource Management

@@ -24,7 +23,7 @@ struct resource *pnp_get_resource(struct
 
 static inline int pnp_resource_valid(struct resource *res)
 {
-	if (res && !(res->flags & IORESOURCE_UNSET))
+	if (res)
 		return 1;
 	return 0;
 }

@@ -248,7 +247,7 @@ struct pnp_dev {
 	int capabilities;
 	struct pnp_option *independent;
 	struct pnp_option *dependent;
-	struct pnp_resource_table *res;
+	struct list_head resources;
 
 	char name[PNP_NAME_LEN];	/* contains a human-readable name */
 	int flags;			/* used by protocols */
#12 lore raw

Re: [patch 6/8] PNP: replace pnp_resource_table with dynamically allocated resources

Quoted 13 lines · On Mon, 05 May 2008 16:36:36 -0600 Bjorn Helgaas <[email protected]> wrote:

PNP used to have a fixed-size pnp_resource_table for tracking the resources used by a device. This table often overflowed, so we've had to increase the table size, which wastes memory because most devices have very few resources.

This patch replaces the table with a linked list of resources where the entries are allocated on demand.

This removes messages like these:

pnpacpi: exceeded the max number of IO resources 00:01: too many I/O port resources

Quoted 8 lines

References:

    http://bugzilla.kernel.org/show_bug.cgi?id=9535
    http://bugzilla.kernel.org/show_bug.cgi?id=9740
    http://lkml.org/lkml/2007/11/30/110

This patch also changes the way PNP uses the IORESOURCE_UNSET, IORESOURCE_AUTO, and IORESOURCE_DISABLED flags.

This patch kills my prehistoric dual PIII.

http://userweb.kernel.org/~akpm/p5135912.jpg
http://userweb.kernel.org/~akpm/config-vmm.txt
http://userweb.kernel.org/~akpm/dmesg-vmm.txt

author
#13 lore raw

Re: [patch 6/8] PNP: replace pnp_resource_table with dynamically allocated resources

Quoted 21 lines · Andrew Morton
Quoted 19 lines · On Mon, 05 May 2008 16:36:36 -0600 Bjorn Helgaas <[email protected]> wrote:

PNP used to have a fixed-size pnp_resource_table for tracking the resources used by a device. This table often overflowed, so we've had to increase the table size, which wastes memory because most devices have very few resources.

This patch replaces the table with a linked list of resources where the entries are allocated on demand.

This removes messages like these:

pnpacpi: exceeded the max number of IO resources 00:01: too many I/O port resources

References:

    http://bugzilla.kernel.org/show_bug.cgi?id=9535
    http://bugzilla.kernel.org/show_bug.cgi?id=9740
    http://lkml.org/lkml/2007/11/30/110
Quoted 8 lines
Quoted 2 lines

This patch also changes the way PNP uses the IORESOURCE_UNSET, IORESOURCE_AUTO, and IORESOURCE_DISABLED flags.

This patch kills my prehistoric dual PIII.

http://userweb.kernel.org/~akpm/p5135912.jpg
http://userweb.kernel.org/~akpm/config-vmm.txt
http://userweb.kernel.org/~akpm/dmesg-vmm.txt

Thanks for the report. I think I've fixed the problem. You had to remove the following three patches:

pnp-replace-pnp_resource_table-with-dynamically-allocated-resources.patch pnp-remove-ratelimit-on-add-resource-failures.patch pnp-dont-sort-by-type-in-sys-resources.patch

I'll post a new set of four patches (I added one patch to fix this problem) to replace those. They should go at the same point in the series as the three you removed.

Bjorn

#14 lore raw

Re: [patch 6/8] PNP: replace pnp_resource_table with dynamically allocated resources

Quoted 1 line · Bjorn Helgaas

Thanks for the report. I think I've fixed the problem.

I saw it was PnPBIOS by the way. PnPBIOS appears to be obsolete these days; do you have machines to test it on yourself? I do (plenty), so although I'm not being fast with testing anyway, would you want me to switch those from PnPACPI to PnPBIOS?

Rene.

author
#15 lore raw

Re: [patch 6/8] PNP: replace pnp_resource_table with dynamically allocated resources

Quoted 8 lines · Rene Herman
Quoted 1 line · Bjorn Helgaas

Thanks for the report. I think I've fixed the problem.

I saw it was PnPBIOS by the way. PnPBIOS appears to be obsolete these days; do you have machines to test it on yourself? I do (plenty), so although I'm not being fast with testing anyway, would you want me to switch those from PnPACPI to PnPBIOS?

All of my machines have ACPI, but in this case, I was able to reproduce the null pointer dereference Andrew by booting with "pnpacpi=off".

PNPBIOS testing is pretty scarce, though, so anything you do along those lines would be much appreciated.

Bjorn

author
#16 lore raw

PATCH 7/8 PNP: remove ratelimit on add resource failures

In reply to Bjorn Helgaas

We used to have a fixed-size resource table. If a device had twenty resources when the table only had space for ten, we didn't need ten warnings, so we added the ratelimit.

Now that we can dynamically allocate new resources, we should only get failures if the allocation fails. That should be rare enough that we don't need to ratelimit the messages.

Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pnp/resource.c +8 -26
Index: work10/drivers/pnp/resource.c
===================================================================
@@ -537,15 +537,10 @@ struct pnp_resource *pnp_add_irq_resourc
 {
 	struct pnp_resource *pnp_res;
 	struct resource *res;
-	static unsigned char warned;
 
 	pnp_res = pnp_new_resource(dev);
 	if (!pnp_res) {
-		if (!warned) {
-			dev_err(&dev->dev, "can't add resource for IRQ %d\n",
-				irq);
-			warned = 1;
-		}
+		dev_err(&dev->dev, "can't add resource for IRQ %d\n", irq);
 		return NULL;
 	}
 

@@ -563,15 +558,10 @@ struct pnp_resource *pnp_add_dma_resourc
 {
 	struct pnp_resource *pnp_res;
 	struct resource *res;
-	static unsigned char warned;
 
 	pnp_res = pnp_new_resource(dev);
 	if (!pnp_res) {
-		if (!warned) {
-			dev_err(&dev->dev, "can't add resource for DMA %d\n",
-				dma);
-			warned = 1;
-		}
+		dev_err(&dev->dev, "can't add resource for DMA %d\n", dma);
 		return NULL;
 	}
 

@@ -590,16 +580,12 @@ struct pnp_resource *pnp_add_io_resource
 {
 	struct pnp_resource *pnp_res;
 	struct resource *res;
-	static unsigned char warned;
 
 	pnp_res = pnp_new_resource(dev);
 	if (!pnp_res) {
-		if (!warned) {
-			dev_err(&dev->dev, "can't add resource for IO "
-				"%#llx-%#llx\n",(unsigned long long) start,
-				(unsigned long long) end);
-			warned = 1;
-		}
+		dev_err(&dev->dev, "can't add resource for IO %#llx-%#llx\n",
+			(unsigned long long) start,
+			(unsigned long long) end);
 		return NULL;
 	}
 

@@ -619,16 +605,12 @@ struct pnp_resource *pnp_add_mem_resourc
 {
 	struct pnp_resource *pnp_res;
 	struct resource *res;
-	static unsigned char warned;
 
 	pnp_res = pnp_new_resource(dev);
 	if (!pnp_res) {
-		if (!warned) {
-			dev_err(&dev->dev, "can't add resource for MEM "
-				"%#llx-%#llx\n",(unsigned long long) start,
-				(unsigned long long) end);
-			warned = 1;
-		}
+		dev_err(&dev->dev, "can't add resource for MEM %#llx-%#llx\n",
+			(unsigned long long) start,
+			(unsigned long long) end);
 		return NULL;
 	}
author
#17 lore raw

PATCH 8/8 PNP: dont sort by type in /sys/.../resources

In reply to Bjorn Helgaas

Rather than stepping through all IO resources, then stepping through all MMIO resources, etc., we can just iterate over the resource list once directly.

This can change the order in /sys, e.g.,

    # cat /sys/devices/pnp0/00:07/resources     # OLD
    state = active
    io 0x3f8-0x3ff
    irq 4
    # cat /sys/devices/pnp0/00:07/resources     # NEW
    state = active
    irq 4
    io 0x3f8-0x3ff

The old code artificially sorted resources by type; the new code just lists them in the order we read them from the ISAPNP hardware or the BIOS.

Signed-off-by Bjorn Helgaas <[email protected]>
drivers/pnp/interface.c +23 -35
Index: work10/drivers/pnp/interface.c
===================================================================
@@ -248,8 +248,9 @@ static ssize_t pnp_show_current_resource
 					  char *buf)
 {
 	struct pnp_dev *dev = to_pnp_dev(dmdev);
+	struct pnp_resource *pnp_res;
 	struct resource *res;
-	int i, ret;
+	int ret;
 	pnp_info_buffer_t *buffer;
 
 	if (!dev)

@@ -262,46 +263,33 @@ static ssize_t pnp_show_current_resource
 	buffer->buffer = buf;
 	buffer->curr = buffer->buffer;
 
-	pnp_printf(buffer, "state = ");
-	if (dev->active)
-		pnp_printf(buffer, "active\n");
-	else
-		pnp_printf(buffer, "disabled\n");
-
-	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IO, i)); i++) {
-		pnp_printf(buffer, "io");
-		if (res->flags & IORESOURCE_DISABLED)
-			pnp_printf(buffer, " disabled\n");
-		else
-			pnp_printf(buffer, " 0x%llx-0x%llx\n",
-				   (unsigned long long) res->start,
-				   (unsigned long long) res->end);
-	}
-	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_MEM, i)); i++) {
-		pnp_printf(buffer, "mem");
-		if (res->flags & IORESOURCE_DISABLED)
+	pnp_printf(buffer, "state = %s\n", dev->active ? "active" : "disabled");
+
+	list_for_each_entry(pnp_res, &dev->resources, list) {
+		res = &pnp_res->res;
+
+		pnp_printf(buffer, pnp_resource_type_name(res));
+
+		if (res->flags & IORESOURCE_DISABLED) {
 			pnp_printf(buffer, " disabled\n");
-		else
-			pnp_printf(buffer, " 0x%llx-0x%llx\n",
+			continue;
+		}
+
+		switch (pnp_resource_type(res)) {
+		case IORESOURCE_IO:
+		case IORESOURCE_MEM:
+			pnp_printf(buffer, " %#llx-%#llx\n",
 				   (unsigned long long) res->start,
 				   (unsigned long long) res->end);
-	}
-	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_IRQ, i)); i++) {
-		pnp_printf(buffer, "irq");
-		if (res->flags & IORESOURCE_DISABLED)
-			pnp_printf(buffer, " disabled\n");
-		else
-			pnp_printf(buffer, " %lld\n",
-				   (unsigned long long) res->start);
-	}
-	for (i = 0; (res = pnp_get_resource(dev, IORESOURCE_DMA, i)); i++) {
-		pnp_printf(buffer, "dma");
-		if (res->flags & IORESOURCE_DISABLED)
-			pnp_printf(buffer, " disabled\n");
-		else
+			break;
+		case IORESOURCE_IRQ:
+		case IORESOURCE_DMA:
 			pnp_printf(buffer, " %lld\n",
 				   (unsigned long long) res->start);
+			break;
+		}
 	}
+
 	ret = (buffer->curr - buf);
 	kfree(buffer);
 	return ret;