Move the common part of pnp_init_resource_table() and
pnp_clean_resource_table() into a new pnp_init_resource().
This reduces a little code duplication and will be
useful later to initialize an individual resource.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

---
 drivers/pnp/base.h    |    2 +
 drivers/pnp/manager.c |   86 +++++++++++++++++++-------------------------------
 2 files changed, 36 insertions(+), 52 deletions(-)

Index: work8/drivers/pnp/base.h
===================================================================
--- work8.orig/drivers/pnp/base.h	2008-04-10 16:47:29.000000000 -0600
+++ work8/drivers/pnp/base.h	2008-04-10 16:48:43.000000000 -0600
@@ -16,3 +16,5 @@
 int pnp_check_mem(struct pnp_dev * dev, int idx);
 int pnp_check_irq(struct pnp_dev * dev, int idx);
 int pnp_check_dma(struct pnp_dev * dev, int idx);
+
+void pnp_init_resource(struct resource *res);
Index: work8/drivers/pnp/manager.c
===================================================================
--- work8.orig/drivers/pnp/manager.c	2008-04-10 16:48:34.000000000 -0600
+++ work8/drivers/pnp/manager.c	2008-04-10 16:48:43.000000000 -0600
@@ -200,6 +200,24 @@
 	*flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
 }
 
+void pnp_init_resource(struct resource *res)
+{
+	unsigned long type;
+
+	type = res->flags & (IORESOURCE_IO  | IORESOURCE_MEM |
+			     IORESOURCE_IRQ | IORESOURCE_DMA);
+
+	res->name = NULL;
+	res->flags = type | IORESOURCE_AUTO | IORESOURCE_UNSET;
+	if (type == IORESOURCE_IRQ || type == IORESOURCE_DMA) {
+		res->start = -1;
+		res->end = -1;
+	} else {
+		res->start = 0;
+		res->end = 0;
+	}
+}
+
 /**
  * pnp_init_resources - Resets a resource table to default values.
  * @table: pointer to the desired resource table
@@ -209,34 +227,14 @@
 	struct pnp_resource_table *table = &dev->res;
 	int idx;
 
-	for (idx = 0; idx < PNP_MAX_IRQ; idx++) {
-		table->irq_resource[idx].name = NULL;
-		table->irq_resource[idx].start = -1;
-		table->irq_resource[idx].end = ...
[ message continues ]

... it's not being set.

Rene.
--

[ message continues ]

Yep, you're right.  I fixed it, but it requires changes to a couple
other patches down the line, so I'll wait to repost the series until
next week in case you find more problems.

I think this is only a problem for bisection.  By the time you
apply the whole series, we start with an empty resource list, and
every time we add one, we set the type in pnp_new_resource().

That makes me wonder whether pnp_init_resources() should just
free the whole list, so we just start again with an empty list.

Bjorn
--

[ message continues ]

Probably. Just a quick heads up; if you don't beat me to it I'll look at it 
in more detail over the weekend but with the current series ISAPnP is quite 
decidedly broken. Forget the OOPS for now which seems just a symptom of the 
same thing as the "too many" warnings". Result of "modprobe snd-cs4236".

I believe it's pnp_assign_resources() which has all the pnp_assign_foo() 
helpers fail due to there not being any yet. I guess pnp_assign_mem() and 
friends should allocate upon not finding one available and not fail?

Don't get why isapnp_get_resources() didn't allocate though. As said, unless 
you beat me to it I'll look into it more tomorrow or sunday.

===
pnp: the driver 'cs4236_isapnp' has been registered
cs4236_isapnp 01:01.00: driver attached
cs4236_isapnp 01:01.02: driver attached
cs4236_isapnp 01:01.03: driver attached
cs4236_isapnp 01:01.00: too many I/O port resources
cs4236_isapnp 01:01.00: too many I/O port resources
cs4236_isapnp 01:01.00: too many I/O port resources
cs4236_isapnp 01:01.00: too many IRQ resources
cs4236_isapnp 01:01.00: too many DMA resources
cs4236_isapnp 01:01.00: too many DMA resources
cs4236_isapnp 01:01.00: activated
BUG: unable to handle kernel NULL pointer dereference at 00000000
IP: [<f0c604da>] :snd_cs4236:snd_cs423x_pnpc_detect+0x104/0x390
*pde = 00000000
Oops: 0000 [#1] PREEMPT
Modules linked in: snd_cs4236(+) snd_opl3_lib snd_hwdep snd_cs4236_lib 
snd_mpu401_uart snd_rawmidi snd_seq_device snd_cs4231_lib snd_pcm snd_timer 
snd soundcore snd_page_alloc nfsd lockd nfs_acl sunrpc exportfs

Pid: 1198, comm: modprobe Not tainted (2.6.25 #2)
EIP: 0060:[<f0c604da>] EFLAGS: 00010202 CPU: 0
EIP is at snd_cs423x_pnpc_detect+0x104/0x390 [snd_cs4236]
EAX: 00000000 EBX: ef868c00 ECX: ef868d6c EDX: 00000000
ESI: 00000000 EDI: f0c63004 EBP: ef84d600 ESP: eed4ce88
  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
Process modprobe (pid: 1198, ti=eed4c000 task=eecf0aa0 task.ti=eed4c000)
Stack: ef312d40 ef84d7b8 ef843200 ef312d40 f0c63004 00000003 ...
[ message continues ]

Oh, yes, 01:01.00 has three port resources, 1 IRQ resource and 2 DMA resources.

Rene.
--

[ message continues ]

isapnp_get_resources didn't allocate simply since it hasn't been called yet; 
there's nothing to get from an inactive device. It is pnp_assign_resources 
which constructs the table which isapnp_set_resources then puts to the hardware.

Currently with the list, pnp_assign_resources doesn't construct anything. It 
finds an empty resource list and then fails all pnp_assign_foo() helpers. I 
can get things functional by making pnp_assign_foo do a pnp_add_foo_resource 
upon not finding one yet but that's a hack -- things are just not very right 
at the moment.

Getting things working also needs setting pnp_res->index (to nport, nmem, 
nirq, ndma in pnp_assign_resources) so that the isapnp_set_resources which 
follows sets to the correct hardware index, but at that point position in 
the list and the index are mixing together in unhealthy ways -- in the 
pnp_assign_foo helpers, pnp_get_resource(.., idx) just get the "idx-th" 
resource of the correct type in the list but it seems it really should be 
getting the resource of the correct type with its ->index set to "idx".

Just making it do that then again disagrees with pnpbios/acpi which do not 
set the index field, nor do I know if you'd want them to or want to keep the 
index totally isapnp specific.

Anyways, currently things really don't work though, due to

1) pnp_assign_resources needing to construct the list and
2) it needing to match resources by their index.

(do note that pnp_assign_foo are the only callers of pnp_check_foo and they 
could be either merged together or at least not communicate via "idx" but 
simply by passing the res/pnp_res).

Also note -- manually set resources are skipped in pnp_assign_resources, yet 
they also definitely need their index initialized for use by 
isapnp_set_resources.

No patches due to the huge series where I don't know where you want to take 
a different turn to end up right. Also many, many opportunities for more 
cleanup of drivers/pnp/manager.c in its post-series state but I ...
[ message continues ]

Yes, right.  I reproduced this on an ACPI system where the BIOS leaves

I don't mind setting pnp_res->index in the generic pnp_assign_* code.
We have to do that already in pnp_set_current_resources() (the /sys
interface), and I don't see a good way around it.

In pnp_assign_resources(), we currently assume that all independent
options appear before any dependent ones because we compute nport,
nmem, etc by iterating through the independent options first.  Then
we use those nport, nmem, etc values as the "index" (CSR index for
ISAPNP, nth resource type in the template for PNPBIOS and PNPACPI).
I don't know whether this assumption is in the spec, but at least
we've assumed it for a long time.

I'm trying to figure out the cases where pnp_assign_resources()
has to pay attention to pre-existing configuration.  It looks like
the common case is that we'll start with an empty resource list, and
we can just find non-conflicting values and use pnp_add_foo_resource().

But I'm concerned about all the IORESOURCE_AUTO stuff.  Seems like
we should only get to pnp_assign_foo() with !IORESOURCE_AUTO if
(a) we've used /sys to set some but not all resources, or (b) the
BIOS described fewer things in _CRS than in _PRS (which seems like

Yes, I'd like to do that.  But I think I'd better wait or I'll never

Yes.  "Manually set resources" includes ones from /sys and also the
resources we discover from active devices.  We should already be setting
those in the /sys and ISAPNP "read resources" paths.

Bjorn
--

[ message continues ]

Ah, good, not just  isapnp then. I was starting to feel lonely here, sitting 

Did this just address my position/index worry above?

It seems you designed the list to be basically in any order, judging by 
things such as pnp_new_resource which'll happily reuse resources of the 
correct type at any position in the list. Yet, pnp_assign_foo() and friends 
retrieve resources (through pnp_get_resource) by position in the list and 
not by the index. I'm not overly sure of failure scenarios but isn't this 


Sounds right to me. Note that the /sys stuff is also not a corner case 
situation either, as it's the way to force at least ISAPnP hardware to 

Well, the idea here was that getting rid of one "idx" here so that things 

Hmm, yes, that sounds true...

Rene.
--

[ message continues ]

Yes, I think you're right.

My idea is that the list should end up in the same order as the
resource list from the BIOS, e.g., the _CRS and _PRS lists for
ACPI.  In the case of ISAPNP, it'll be in the order that we read
things from the hardware registers.

I think I should probably make pnp_new_resource() always allocate
a new resource and get rid of the idea of reusing something already

Yup.  I ended up doing this after all.

I updated the patches and my ACPI box with the inactive devices now
boots (it failed with the v3 patches).  I'll work on the
pnp_new_resource() thing tomorrow, but I'll send you the current
patches now in case you have a chance to try them on your ISA box.

Bjorn
--

[ message continues ]

Could do a quick test and its cs4236 soundcard works fine with them. There 
are a few "current resources: after pnp_assign_resources (failed)" in there 
but they might be expected, I didn't yet look. There's a dmesg from booting 
it and "modprobe snd-cs4236" at:

http://members.home.nl/rene.herman/dmesg-2.6.25-pnpv3.9

Matching "options" files:

root@6bap:~# for OPTIONS in /sys/devices/pnp1/01\:01/01\:01.0?/options; do 
echo; echo $OPTIONS; echo; cat $OPTIONS; done

/sys/devices/pnp1/01:01/01:01.00/options

Dependent: 01 - Priority preferred
    port 0x534-0x534, align 0x3, size 0x4, 16-bit address decoding
    port 0x388-0x388, align 0x7, size 0x4, 16-bit address decoding
    port 0x220-0x220, align 0x1f, size 0x10, 16-bit address decoding
    irq 5 High-Edge
    dma 1 8-bit byte-count compatible
    dma 0,1 8-bit byte-count compatible
Dependent: 02 - Priority acceptable
    port 0x534-0xffc, align 0x3, size 0x4, 16-bit address decoding
    port 0x388-0x388, align 0x7, size 0x4, 16-bit address decoding
    port 0x220-0x260, align 0x1f, size 0x10, 16-bit address decoding
    irq 5,7,2/9,11,12,15 High-Edge
    dma 1,3 8-bit byte-count compatible
    dma 0,1,3 8-bit byte-count compatible
Dependent: 03 - Priority functional
    port 0x534-0xffc, align 0x3, size 0x4, 16-bit address decoding
    port 0x388-0x3f8, align 0x7, size 0x4, 16-bit address decoding
    port 0x220-0x300, align 0x1f, size 0x10, 16-bit address decoding
    irq 5,7,2/9,11,12,15 High-Edge
    dma 0,1,3 8-bit byte-count compatible

/sys/devices/pnp1/01:01/01:01.01/options

Dependent: 01 - Priority preferred
    port 0x200-0x200, align 0x7, size 0x8, 16-bit address decoding
Dependent: 02 - Priority acceptable
    port 0x208-0x208, align 0x7, size 0x8, 16-bit address decoding

/sys/devices/pnp1/01:01/01:01.02/options

port 0x120-0xff8, align 0x7, size 0x8, 16-bit address decoding

/sys/devices/pnp1/01:01/01:01.03/options

Dependent: 01 - Priority preferred
    port 0x330-0x330, align 0x7, size ...
[ message continues ]

Oh, this was on v2.6.25 vanilla + 0093cb1199ec551f179562ca9fbd6f64fb750645 
(only post 2.6.25 change to drivers/pnp).

Rene
--

[ message continues ]

Here's something interesting.  I was mistaken about the ordering
of independent vs. dependent options coming from BIOS -- I have
several examples where the dependent ones come first, including
this one:

    pnp 00:0a: parse resource options
    pnp 00:0a: new independent option
    pnp 00:0a: new dependent option (priority 0x1)
    pnp 00:0a:   io  min 0x3f8 max 0x3f8 align 1 size 8 flags 0x1
    pnp 00:0a: new dependent option (priority 0x1)
    pnp 00:0a:   io  min 0x2e8 max 0x2e8 align 1 size 8 flags 0x1
    pnp 00:0a: new dependent option (priority 0x1)
    pnp 00:0a:   io  min 0x2f8 max 0x2f8 align 1 size 8 flags 0x1
    pnp 00:0a: new dependent option (priority 0x1)
    pnp 00:0a:   io  min 0x3e8 max 0x3e8 align 1 size 8 flags 0x1
    pnp 00:0a: end dependent options
    pnp 00:0a:   irq bitmask 00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000cf8 flags 0x1
    pnp 00:0a:   io  min 0x100 max 0x130 align 16 size 8 flags 0x1
    pnp 00:0a:   dma bitmask 0xe flags 0x2
    pnp 00:0a: Plug and Play ACPI device, IDs SMCf010 (disabled)

The interesting thing here is that this IRDA device comes up disabled,
and we use pnp_assign_resources() before enabling it.  But we
assign independent options first, so we end up reversing the order
of these IO regions:

    state = active
    io 0x100-0x107
    io 0x2e8-0x2ef
    irq 5
    dma 1

The driver for this device (smsc-ircc2.c) currently can't claim it
via PNP, but by long painful trial and error, I figured out last
summer that I could get the PNP claim code to work if I manually
swapped the IO regions!

I haven't done anything about this yet, but it sure looks to me like
we need to preserve the order of the independent/dependent options
as we get them from the BIOS.

Bjorn
--

[ message continues ]