Re: [PATCH] (for review and testing first) Implement dynamic allocated array for pnp port/io resources
- From: Bjorn Helgaas <bjorn.helgaas@xxxxxx>
- Date: Wed, 15 Aug 2007 16:01:15 -0600
On Wednesday 15 August 2007 08:03:24 am Thomas Renninger wrote:
This is not a real feature, more a fix.
Without, PNP IO ports might not get considered. This mainly affects ACPI
system board devices with HID PNP0C02 (at least I saw this on my and
some other machines, but it may affect more...).
I think this is definitely the way we should go and a great start.
Thanks a lot for working on this.
...
I expect this is a bit late for 2.6.23?
I think it is too late for 2.6.23, since the problem is not a
regression from 2.6.22.
We have several options here for 2.6.23:
- leave it as it is
I certainly agree that your patch is something we need to do, but
I vote for leaving it as-is for 2.6.23, because I can't think of a
specific problem it would fix.
...
- Add Bjorn's "Increase statically used IRQ ports from 2 to 4" should
be added anyway IMO. This one is really safe, it's here;
http://lkml.org/lkml/2007/7/17/335
Increasing IRQs is safe, but of less value than the I/O ports. AFAIK,
nobody needs more than 2 IRQs at the moment. I want to increase it so
I can convert hpet from an ACPI driver to a PNP driver, but there's no
hurry for that.
Some parts where a reviewer should have a closer eye on:
- the border limits (got a '>' mixed up with a '>=' or similar)
- I removed or better let pnp_init_resource_table invoke
pnp_clean_resource_table as the only difference between those was
the additional NULL assignment to the name. Can this really be
removed or was this in any way useful :)
pnp_init_resource_table() used to set resource.name = NULL. I don't
see where that will happen now, and since you use kmalloc/krealloc,
I think we should completely initialize the allocated space.
- locking: Andi made me a bit nervous about locking. As I didn't
modify much in the design/structure of how it currently is done,
I don't expect simultaneous access to the port resource data
can happen and additional locking should not be needed,
but I am not sure about it.
It's a good thing to think about, but I agree that I don't think you
introduced any additional problems here.
- Only field tested with pnpacpi
You removed PNP_MAX_PORT, but ISAPNP still needs it. Unlike PNPBIOS and
ACPI, the ISAPNP spec does limit the number of resources. So you might
want to add an ISAPNP_MAX_PORT or similar when you fix up ISAPNP.
I saw recent Lindentation patches for pnp. I expect they came in after
-rc2?
You patch applies fine against current upstream, which includes the
Lindent patches you saw (I posted a couple more today, which cause
one conflict with your patch). But your patch adds a few more Lindent
issues :-) Extra blank lines, spaces between function name and open
paren, lack of space between function arguments, etc.
+static void pnp_init_port (struct pnp_resource_table *res, int idx)
+{
+ if (idx < res->allocated_ports) {
+ (res->port_resource + idx)->start = 0;
+ (res->port_resource + idx)->end = 0;
+ (res->port_resource + idx)->flags =
+ IORESOURCE_IO | IORESOURCE_AUTO | IORESOURCE_UNSET;
+ }
How about:
static void pnp_init_port(struct resource *res)
{
res->start = 0;
...
}
and adjusting the caller? Then you don't have to check against
allocated_ports both here and in the caller. Or maybe just fold
into the only call site.
+ pnp_err ("%s: Cannot allocate port", __FUNCTION__);
+ /* pretend we were successful so at least the manager won't try again */
+ return 1;
When this fails, it would be nice if the error message included the
associated device.
/* check if this resource has been manually set, if so skip */
- if (!(dev->res.port_resource[idx].flags & IORESOURCE_AUTO))
+ if (!(pnp_port_flags(dev,idx) & IORESOURCE_AUTO))
return 1;
- start = &dev->res.port_resource[idx].start;
- end = &dev->res.port_resource[idx].end;
- flags = &dev->res.port_resource[idx].flags;
+ start = &pnp_port_start(dev,idx);
+ end = &pnp_port_end(dev,idx);
+ flags = &pnp_port_flags(dev,idx);
I like these changes, but they're really independent of the dynamic
allocation, aren't they? Maybe a separate patch.
+#define pnp_port_res_pointer(dev,bar) ((dev->res.allocated_ports > bar) \
+ ? (dev->res.port_resource + bar) : NULL)
Maybe just "pnp_port_resource(dev,bar)"?
+#define pnp_port_start(dev,bar) ((dev->res.port_resource + bar)->start)
Is it safe to remove the parentheses around "dev" and "bar"? My
personal preference is for array syntax, e.g.,
dev->res.port_resource[bar]->start
+static inline pnp_port_alloc(struct pnp_resource_table *res_table) { return -ENODEV }
Missing return type.
--- linux-2.6.23-rc2.orig/drivers/pnp/pnpbios/rsparser.c
+++ linux-2.6.23-rc2/drivers/pnp/pnpbios/rsparser.c
@@ -97,18 +97,31 @@ static void pnpbios_parse_allocated_iore
{
int i = 0;
...
These functions (pnpbios_parse_allocated_ioresource() and
pnpacpi_parse_allocated_ioresource()) are almost identical between
PNPBIOS and PNPACPI. What would it take to factor out this
duplicated code into a pnp_add_port() sort of function? Maybe
could be a separate patch.
-
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/
- References:
- Prev by Date: Re: [PATCH 3/25][V3] irq_flags / halt routines
- Next by Date: [2.6.23-rc3] NFSv4 client oops
- Previous by thread: Re: [PATCH] (for review and testing first) Implement dynamic allocated array for pnp port/io resources
- Next by thread: [PATCH] ia64: default the NUMA node distance when there is no ACPI SLIT
- Index(es):
Relevant Pages
|
|
