linux-kernel - Re: [patch 22/53] PNP: factor pnp_init_resource_table() and pnp_clean_resource

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <480E5246.5000804@keyaccess.nl>
Date:	Tue, 22 Apr 2008 23:01:58 +0200
From:	Rene Herman <rene.herman@...access.nl>
To:	Bjorn Helgaas <bjorn.helgaas@...com>
CC:	Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, Adam Belay <ambx1@....rr.com>,
	Li Shaohua <shaohua.li@...el.com>,
	Matthieu Castet <castet.matthieu@...e.fr>,
	Thomas Renninger <trenn@...e.de>,
	Jaroslav Kysela <perex@...ex.cz>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [patch 22/53] PNP: factor pnp_init_resource_table() and pnp_clean_resource_table()

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