Date:	Thu, 1 May 2008 14:47:37 -0600
From:	Bjorn Helgaas <bjorn.helgaas@...com>
To:	Rene Herman <rene.herman@...access.nl>
Cc:	Rene Herman <rene.herman@...il.com>, 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 00/37] PNP resource_table cleanups, v2

On Thursday 03 April 2008 09:54:51 am Rene Herman wrote:
> However, now that you made me look closer and in context -- there's actually 
> a possibly somewhat serious problem here.
> 
> isapnp_read_resources() stores the resources as read from the hardware at 
> the index in the table that matches the actual index in the hardware and 
> isapnp_set_resources() stores them back into those same hardware indices.
> 
> Now by using pnp_add_foo_resource() which just scans for the first _UNSET 
> resource, the resources might not end up in the same linear position in 
> table/list if intermediate resources were unset in hardware (!ret). A 
> subsequent isapnp_set_resources() would them restore the value to the wrong 
> hardware index.
> 
> The IORESOURCE_ flags currently reserve too few bits (IORESOURCE_BITS,  8) 
> to be able to store the hardware index: IORESOURCE_MEM and IORESOURCE_DMA 
> need 2 and 1 respectively and there are 1 and 0 available respectively. It's 
> ofcourse possible to hijack a few more bits in IORESOURCE_ flags but you're 
> turning this into a list. I suppose the idea is to make it a simple list of 
> struct resource, but perhaps a resource-private "driver_data" sort of field 
> comes in handy for more than this already? Swiping more of IORESOURCE_ is a 
> bit ugly...
> 
> In any case, I missed this, but ISAPnP is still (at least in principle) 
> broken with the current set therefore.

I want to understand this better.  I think the case we're concerned
about is this:

  Memory descriptor 0 is not assigned, i.e., its base and limit/range
  registers starting at 0x40 contain zeroes, but Descriptor 1, starting
  at 0x48, *is* assigned.

The 2.6.25 "get_resources" code doesn't touch the resource table for
Descriptor 0, so its entry remains "unset".  The "set_resources" code
skips Descriptor 0 because its resource table entry is "unset" and
writes Descriptor 1.

When I convert the table to a list, I have to make sure that we write
the Descriptor 1 resources to the correct place starting at 0x48, not
to the Descriptor 0 registers.  To do this, I made "get_resources" set
the pnp_resource.index field to the current descriptor index, and
"set_resources" uses pnp_resource.index to compute the register address.

However, PNPBIOS, PNPACPI, and even ISAPNP Resource Data is all based
on the ordinal position in list (see the fourth paragraph of section
4.6.1 of the ISA spec).  Having pnp_resource.index in addition to a
list position adds a lot of confusion.

I think a better solution would be to get rid of pnp_resource.index
and have "get_resources" add a "disabled" resource for Descriptor 0,
so the Nth MEM resource in the list would always correspond to the
Nth Memory Descriptor register.

Does this make sense?

Bjorn
--
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/

Powered by blists - more mailing lists