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

[Rene Herman <rene.herman@keyaccess.nl>]

On 22-04-08 01:10, Bjorn Helgaas wrote:

> Yes, right.  I reproduced this on an ACPI system where the BIOS leaves
> a couple devices disabled.

Ah, good, not just  isapnp then. I was starting to feel lonely here, sitting 
atop my pile of ISA crap...

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

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 
mixing up position and index in a bad way?

> 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().

Yes...

> 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
> a BIOS bug).  But I'm not comfortable with this yet.

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 
manual settings.

>> (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).
> 
> Yes, I'd like to do that.  But I think I'd better wait or I'll never
> get anything finished :-)

Well, the idea here was that getting rid of one "idx" here so that things 
communicate directly removes at least one possible ordering artifact...

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

Hmm, yes, that sounds true...

Rene.