Re: [patch 22/53] PNP: factor pnp_init_resource_table() and pnp_clean_resource_table()

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

On Tuesday 22 April 2008 03:01:58 pm Rene Herman wrote:
> On 22-04-08 01:10, Bjorn Helgaas wrote:
> >> 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".
> > 
> > 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.
> 
> Did this just address my position/index worry above?

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