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

Rene Herman
2008-04-22T21:02:10Z

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.