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

[Rene Herman]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/