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

Rene Herman
2008-04-21T18:41:44Z

On 19-04-08 06:43, Rene Herman wrote:

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

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 suppose you
want the series bisectably functional first. I'll wait for a next version
for now I guess?

Rene.