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-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
