Messages in this thread Patch in this message | | | From | Bjorn Helgaas <> | Subject | Re: [PATCH] (for review and testing first) Implement dynamic allocated array for pnp port/io resources | 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |