On 31-05-08 00:49, Bjorn Helgaas wrote:
> ISAPNP, PNPBIOS, and ACPI describe the "possible resource settings" of
> a device, i.e., the possibilities an OS bus driver has when it assigns
> I/O port, MMIO, and other resources to the device.
>
> PNP used to maintain this "possible resource setting" information in
> one independent option structure and a list of dependent option
> structures for each device. Each of these option structures had lists
> of I/O, memory, IRQ, and DMA resources, for example:
>
> dev
> independent options
> ind-io0 -> ind-io1 ...
> ind-mem0 -> ind-mem1 ...
> ...
> dependent option set 0
> dep0-io0 -> dep0-io1 ...
> dep0-mem0 -> dep0-mem1 ...
> ...
> dependent option set 1
> dep1-io0 -> dep1-io1 ...
> dep1-mem0 -> dep1-mem1 ...
> ...
> ...
>
> This data structure was designed for ISAPNP, where the OS configures
> device resource settings by writing directly to configuration
> registers. The OS can write the registers in arbitrary order much
> like it writes PCI BARs.
>
> However, for PNPBIOS and ACPI devices, the OS uses firmware interfaces
> that perform device configuration, and it is important to pass the
> desired settings to those interfaces in the correct order. The OS
> learns the correct order by using firmware interfaces that return the
> "current resource settings" and "possible resource settings," but the
> option structures above doesn't store the ordering information.
>
> This patch replaces the independent and dependent lists with a single
> list of options. For example, a device might have possible resource
> settings like this:
>
> dev
> options
> ind-io0 -> dep0-io0 -> dep1->io0 -> ind-io1 ...
Just for the record -- due to an ISAPnP bug, ISAPnP doesn't actually at
the moment deal with independents following dependents at all (it does
not reset the option to independent upon seeing an _STAG_ENDDEP tag) and
the rework actually fixes this bug. The ISAPnP spec does recommend that
independents precede dependents, but does not guarantee.
Might be good to mention this in the log though? In the unlikely event
that some piece of ISAPnP hardware out there actually changes behaviour
(to "fixed" ...) that might be a hint in looking at it.
<snip>
> +#define PNP_OPTION_DEPENDENT 0x80000000
> +#define PNP_OPTION_SET_MASK 0xff
> +#define PNP_OPTION_SET_SHIFT 16
> +#define PNP_OPTION_PRIORITY_MASK 0xffff
> +#define PNP_OPTION_PRIORITY_SHIFT 0
I checked, and ISAPnP and PnPBIOS definitely limit the priority to 8-bit
and ACPI seems to as well meaning the (only) reason for using 16 would
be the RES_PRIORITY_INVALID value.
I have on the other hand not encountered text limiting the number of
dependents sets to 255 nor do I believe the old code enforced this?
Yes, I suppose this is exceedingly unlikely to be a practical issue.
<snip>
> static void pnp_print_option(pnp_info_buffer_t * buffer, char *space,
> - struct pnp_option *option, int dep)
> + struct pnp_option *option)
> + switch (option->type) {
> + case IORESOURCE_IO:
> + pnp_print_port(buffer, space, &option->u.port);
> + break;
> + case IORESOURCE_MEM:
> + pnp_print_mem(buffer, space, &option->u.mem);
> + break;
> + case IORESOURCE_IRQ:
> + pnp_print_irq(buffer, space, &option->u.irq);
> + break;
I believe it would be good if pnp_print_irq() now also displayed
"(optional)" if the flag is set.
> @@ -659,26 +654,24 @@ static int __init isapnp_create_device(s
> priority = 0x100 | tmp[0];
> size = 0;
> }
> - option = pnp_register_dependent_option(dev, priority);
> - if (!option)
> - return 1;
> + option_flags = pnp_dependent_option(dev, priority);
> break;
> case _STAG_ENDDEP:
> if (size != 0)
> goto __skip;
> - priority = 0;
> - dev_dbg(&dev->dev, "end dependent options\n");
> + option_flags = pnp_independent_option();
(here is the bugfix)
> -static int pnp_assign_resources(struct pnp_dev *dev, int depnum)
> +static int pnp_assign_resources(struct pnp_dev *dev, int set)
> {
> - struct pnp_port *port;
> - struct pnp_mem *mem;
> - struct pnp_irq *irq;
> - struct pnp_dma *dma;
> + struct pnp_option *option;
> int nport = 0, nmem = 0, nirq = 0, ndma = 0;
> + int ret = 0;
>
> - dbg_pnp_show_resources(dev, "before pnp_assign_resources");
> + dev_dbg(&dev->dev, "pnp_assign_resources, try dependent set %d\n", set);
> mutex_lock(&pnp_res_mutex);
> pnp_clean_resource_table(dev);
> - if (dev->independent) {
> - dev_dbg(&dev->dev, "assigning independent options\n");
> - port = dev->independent->port;
> - mem = dev->independent->mem;
> - irq = dev->independent->irq;
> - dma = dev->independent->dma;
> - while (port) {
> - if (pnp_assign_port(dev, port, nport) < 0)
> - goto fail;
> - nport++;
> - port = port->next;
> - }
> - while (mem) {
> - if (pnp_assign_mem(dev, mem, nmem) < 0)
> - goto fail;
> - nmem++;
> - mem = mem->next;
> - }
> - while (irq) {
> - if (pnp_assign_irq(dev, irq, nirq) < 0)
> - goto fail;
> - nirq++;
> - irq = irq->next;
> - }
> - while (dma) {
> - if (pnp_assign_dma(dev, dma, ndma) < 0)
> - goto fail;
> - ndma++;
> - dma = dma->next;
> - }
> - }
>
> - if (depnum) {
> - struct pnp_option *dep;
> - int i;
> -
> - dev_dbg(&dev->dev, "assigning dependent option %d\n", depnum);
> - for (i = 1, dep = dev->dependent; i < depnum;
> - i++, dep = dep->next)
> - if (!dep)
> - goto fail;
> - port = dep->port;
> - mem = dep->mem;
> - irq = dep->irq;
> - dma = dep->dma;
> - while (port) {
> - if (pnp_assign_port(dev, port, nport) < 0)
> - goto fail;
> - nport++;
> - port = port->next;
> - }
> - while (mem) {
> - if (pnp_assign_mem(dev, mem, nmem) < 0)
> - goto fail;
> - nmem++;
> - mem = mem->next;
> - }
> - while (irq) {
> - if (pnp_assign_irq(dev, irq, nirq) < 0)
> - goto fail;
> - nirq++;
> - irq = irq->next;
> + list_for_each_entry(option, &dev->options, list) {
> + if (pnp_option_is_dependent(option) &&
> + pnp_option_set(option) != set)
> + continue;
> +
> + switch (option->type) {
> + case IORESOURCE_IO:
> + ret = pnp_assign_port(dev, &option->u.port, nport++);
> + break;
> + case IORESOURCE_MEM:
> + ret = pnp_assign_mem(dev, &option->u.mem, nmem++);
> + break;
> + case IORESOURCE_IRQ:
> + ret = pnp_assign_irq(dev, &option->u.irq, nirq++);
> + break;
> + case IORESOURCE_DMA:
> + ret = pnp_assign_dma(dev, &option->u.dma, ndma++);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> }
> - while (dma) {
> - if (pnp_assign_dma(dev, dma, ndma) < 0)
> - goto fail;
> - ndma++;
> - dma = dma->next;
> - }
> - } else if (dev->dependent)
> - goto fail;
> + if (ret < 0)
> + break;
> + }
Mmm. Previously when it failed halfway through it re-cleared the
resources (at label fail). Does it want to do so now also?
> mutex_unlock(&pnp_res_mutex);
> - dbg_pnp_show_resources(dev, "after pnp_assign_resources");
> - return 1;
> -
> -fail:
> - pnp_clean_resource_table(dev);
> - mutex_unlock(&pnp_res_mutex);
> - dbg_pnp_show_resources(dev, "after pnp_assign_resources (failed)");
> - return 0;
> + if (ret == 0)
> + dbg_pnp_show_resources(dev, "pnp_assign_resources succeeded");
> + else
> + dev_dbg(&dev->dev, "pnp_assign_resources failed (%d)\n", ret);
> + return ret;
> }
>
> /**
> @@ -332,29 +282,21 @@ fail:
> */
> int pnp_auto_config_dev(struct pnp_dev *dev)
> {
> - struct pnp_option *dep;
> - int i = 1;
> + int i, ret = 0;
>
> if (!pnp_can_configure(dev)) {
> dev_dbg(&dev->dev, "configuration not supported\n");
> return -ENODEV;
> }
>
> - if (!dev->dependent) {
> - if (pnp_assign_resources(dev, 0))
> + for (i = 0; i == 0 || i < dev->num_dependent_sets; i++) {
> + ret = pnp_assign_resources(dev, i);
> + if (ret == 0)
> return 0;
Eeeew. Perhaps:
i = 0;
do {
ret = pnp_assign_resources(dev, i);
if (ret == 0)
return 0;
} while (++i < dev->num_dependent_sets);
<snip>
> Index: work10/drivers/pnp/quirks.c
> ===================================================================
> --- work10.orig/drivers/pnp/quirks.c 2008-05-30 15:58:14.000000000 -0600
> +++ work10/drivers/pnp/quirks.c 2008-05-30 15:58:15.000000000 -0600
> @@ -5,6 +5,8 @@
> * when building up the resource structure for the first time.
> *
> * Copyright (c) 2000 Peter Denison <[email protected]>
> + * (c) Copyright 2008 Hewlett-Packard Development Company, L.P.
> + * Bjorn Helgaas <[email protected]>
> *
> * Heavily based on PCI quirks handling which is
> *
> @@ -20,188 +22,208 @@
> #include <linux/kallsyms.h>
> #include "base.h"
>
> +static void quirk_awe32_add_ports(struct pnp_dev *dev,
> + struct pnp_option *option,
> + unsigned int offset)
> +{
> + struct pnp_option *new_option;
> +
> + new_option = kmalloc(sizeof(struct pnp_option), GFP_KERNEL);
> + if (!new_option) {
> + dev_err(&dev->dev, "couldn't add ioport region to option set "
> + "%d\n", pnp_option_set(option));
> + return;
> + }
> +
> + *new_option = *option;
> + new_option->u.port.min += offset;
> + new_option->u.port.max += offset;
> + list_add(&new_option->list, &option->list);
> +
> + dev_info(&dev->dev, "added ioport region %#llx-%#llx to set %d\n",
> + (unsigned long long) new_option->u.port.min,
> + (unsigned long long) new_option->u.port.max,
> + pnp_option_set(option));
> +}
> +
> static void quirk_awe32_resources(struct pnp_dev *dev)
> {
> - struct pnp_port *port, *port2, *port3;
> - struct pnp_option *res = dev->dependent;
> + struct pnp_option *option;
> + unsigned int set = ~0;
>
> /*
> - * Unfortunately the isapnp_add_port_resource is too tightly bound
> - * into the PnP discovery sequence, and cannot be used. Link in the
> - * two extra ports (at offset 0x400 and 0x800 from the one given) by
> - * hand.
> + * Add two extra ioport regions (at offset 0x400 and 0x800 from the
> + * one given) to every dependent option set.
> */
> - for (; res; res = res->next) {
> - port2 = pnp_alloc(sizeof(struct pnp_port));
> - if (!port2)
> - return;
> - port3 = pnp_alloc(sizeof(struct pnp_port));
> - if (!port3) {
> - kfree(port2);
> - return;
> + list_for_each_entry(option, &dev->options, list) {
> + if (pnp_option_is_dependent(option) &&
> + pnp_option_set(option) != set) {
> + set = pnp_option_set(option);
> + quirk_awe32_add_ports(dev, option, 0x800);
> + quirk_awe32_add_ports(dev, option, 0x400);
> }
> - port = res->port;
> - memcpy(port2, port, sizeof(struct pnp_port));
> - memcpy(port3, port, sizeof(struct pnp_port));
> - port->next = port2;
> - port2->next = port3;
> - port2->min += 0x400;
> - port2->max += 0x400;
> - port3->min += 0x800;
> - port3->max += 0x800;
Why do you do 0x800, 0x400 in that order? Shouldn't it just be 0x400,
0x800 to mimick the old order?
> - dev_info(&dev->dev,
> - "AWE32 quirk - added ioports 0x%lx and 0x%lx\n",
> - (unsigned long)port2->min,
> - (unsigned long)port3->min);
> }
> }
>
> static void quirk_cmi8330_resources(struct pnp_dev *dev)
> {
> - struct pnp_option *res = dev->dependent;
> - unsigned long tmp;
> -
> - for (; res; res = res->next) {
> -
> - struct pnp_irq *irq;
> - struct pnp_dma *dma;
> + struct pnp_option *option;
> + struct pnp_irq *irq;
> + struct pnp_dma *dma;
>
> - for (irq = res->irq; irq; irq = irq->next) { // Valid irqs are 5, 7, 10
> - tmp = 0x04A0;
> - bitmap_copy(irq->map.bits, &tmp, 16); // 0000 0100 1010 0000
> - }
> + list_for_each_entry(option, &dev->options, list) {
> + if (!pnp_option_is_dependent(option))
> + continue;
>
> - for (dma = res->dma; dma; dma = dma->next) // Valid 8bit dma channels are 1,3
> + if (option->type == IORESOURCE_IRQ) {
> + irq = &option->u.irq;
> + bitmap_zero(irq->map.bits, PNP_IRQ_NR);
> + __set_bit(5, irq->map.bits);
> + __set_bit(7, irq->map.bits);
> + __set_bit(10, irq->map.bits);
> + dev_info(&dev->dev, "set possible IRQs in "
> + "option set %d to 5, 7, 10\n",
> + pnp_option_set(option));
> + } else if (option->type == IORESOURCE_DMA) {
> + dma = &option->u.dma;
> if ((dma->flags & IORESOURCE_DMA_TYPE_MASK) ==
> - IORESOURCE_DMA_8BIT)
> + IORESOURCE_DMA_8BIT &&
> + dma->map != 0x000A) {
It's a char, so 0x0A? 0x000A makes it looks like it's 16-bit.
> + dev_info(&dev->dev, "changing possible "
> + "DMA channel mask in option set %d "
> + "from %#x to 0xA (1, 3)\n",
> + pnp_option_set(option), dma->map);
> dma->map = 0x000A;
And here...
> + }
> + }
> }
> - dev_info(&dev->dev, "CMI8330 quirk - forced possible IRQs to 5, 7, 10 "
> - "and DMA channels to 1, 3\n");
> }
>
> static void quirk_sb16audio_resources(struct pnp_dev *dev)
> {
> + struct pnp_option *option;
> + unsigned int prev_option_flags = ~0, n = 0;
> struct pnp_port *port;
> - struct pnp_option *res = dev->dependent;
> - int changed = 0;
>
> /*
> * The default range on the mpu port for these devices is 0x388-0x388.
> * Here we increase that range so that two such cards can be
> * auto-configured.
> */
While you're there -- s/mpu/OPL/.
> +static void quirk_ad1815_mpu_resources(struct pnp_dev *dev)
> {
> - struct pnp_option *res;
> + struct pnp_option *option, *irq_option = NULL;
> + unsigned int independent_irqs = 0;
> +
> + list_for_each_entry(option, &dev->options, list) {
> + if (option->type == IORESOURCE_IRQ &&
> + !pnp_option_is_dependent(option)) {
> + independent_irqs++;
> + irq_option = option;
> + }
> + }
>
> - res = dev->dependent;
> - if (!res)
> + if (independent_irqs != 1)
> return;
>
> - while (res->next)
> - res = res->next;
> + irq_option->flags |= IORESOURCE_IRQ_OPTIONAL;
You're in maze of unsigned variables, all named flags...
irq_option->u.irq.flags |= IORESOURCE_IRQ_OPTIONAL;
This was the thing that made things not work when I tested -- on an
AD1815...
> + dev_info(&dev->dev, "made independent IRQ optional\n");
>
> - res->next = quirk_isapnp_mpu_options(dev);
> + quirk_add_irq_optional_dependent_sets(dev);
And as before, this one be gone.
The other quirks look good -- but I have the hardware and will test them
specifically.
> Index: work10/drivers/pnp/resource.c
> ===================================================================
> --- work10.orig/drivers/pnp/resource.c 2008-05-30 15:58:13.000000000 -0600
> +++ work10/drivers/pnp/resource.c 2008-05-30 15:58:15.000000000 -0600
> @@ -3,6 +3,8 @@
> *
> * based on isapnp.c resource management (c) Jaroslav Kysela <[email protected]>
> * Copyright 2003 Adam Belay <[email protected]>
> + * (c) Copyright 2008 Hewlett-Packard Development Company, L.P.
> + * Bjorn Helgaas <[email protected]>
> */
>
> #include <linux/module.h>
> @@ -28,78 +30,37 @@ static int pnp_reserve_mem[16] = {[0 ...
> * option registration
> */
>
> -struct pnp_option *pnp_build_option(int priority)
> +struct pnp_option *pnp_build_option(struct pnp_dev *dev,
> + unsigned long type,
> + unsigned int option_flags)
> {
> - struct pnp_option *option = pnp_alloc(sizeof(struct pnp_option));
> + struct pnp_option *option;
>
> + option = kzalloc(sizeof(struct pnp_option), GFP_KERNEL);
> if (!option)
> return NULL;
>
> - option->priority = priority & 0xff;
> - /* make sure the priority is valid */
> - if (option->priority > PNP_RES_PRIORITY_FUNCTIONAL)
> - option->priority = PNP_RES_PRIORITY_INVALID;
Dropping this made the 0x100 | prio ISAPnP/PnPBIOS bug appear. That
0x100 should go anyway but perhaps this limiting is still a good idea?
> + option->flags = option_flags;
> + option->type = type;
>
> + list_add_tail(&option->list, &dev->options);
> return option;
> }
Rene.
On Tuesday 03 June 2008 01:57:11 pm Rene Herman wrote:
> On 31-05-08 00:49, Bjorn Helgaas wrote:
> Just for the record -- due to an ISAPnP bug, ISAPnP doesn't actually at
> the moment deal with independents following dependents at all (it does
> not reset the option to independent upon seeing an _STAG_ENDDEP tag) and
> the rework actually fixes this bug. The ISAPnP spec does recommend that
> independents precede dependents, but does not guarantee.
>
> Might be good to mention this in the log though? In the unlikely event
> that some piece of ISAPnP hardware out there actually changes behaviour
> (to "fixed" ...) that might be a hint in looking at it.
I split that bugfix into a separate patch; thanks for
pointing this out.
> > +#define PNP_OPTION_DEPENDENT 0x80000000
> > +#define PNP_OPTION_SET_MASK 0xff
> > +#define PNP_OPTION_SET_SHIFT 16
> > +#define PNP_OPTION_PRIORITY_MASK 0xffff
> > +#define PNP_OPTION_PRIORITY_SHIFT 0
>
> I checked, and ISAPnP and PnPBIOS definitely limit the priority to 8-bit
> and ACPI seems to as well meaning the (only) reason for using 16 would
> be the RES_PRIORITY_INVALID value.
>
> I have on the other hand not encountered text limiting the number of
> dependents sets to 255 nor do I believe the old code enforced this?
>
> Yes, I suppose this is exceedingly unlikely to be a practical issue.
That's definitely backwards. I reversed the sizes, so we'll have
8 bits for the priority byte (including compatibility/performance/
robustness) and 16 bits for the dependent set number. Actually,
I made the priority field 12 bits so we'd have space to keep
PNP_RES_PRIORITY_INVALID as a truly out-of-band value.
> > static void pnp_print_option(pnp_info_buffer_t * buffer, char *space,
> > - struct pnp_option *option, int dep)
> > + struct pnp_option *option)
>
> > + switch (option->type) {
> > + case IORESOURCE_IO:
> > + pnp_print_port(buffer, space, &option->u.port);
> > + break;
> > + case IORESOURCE_MEM:
> > + pnp_print_mem(buffer, space, &option->u.mem);
> > + break;
> > + case IORESOURCE_IRQ:
> > + pnp_print_irq(buffer, space, &option->u.irq);
> > + break;
>
> I believe it would be good if pnp_print_irq() now also displayed
> "(optional)" if the flag is set.
I agree; I added this to the patch that added IORESOURCE_IRQ_OPTIONAL.
> > @@ -659,26 +654,24 @@ static int __init isapnp_create_device(s
> > priority = 0x100 | tmp[0];
> > size = 0;
> > }
> > - option = pnp_register_dependent_option(dev, priority);
> > - if (!option)
> > - return 1;
> > + option_flags = pnp_dependent_option(dev, priority);
> > break;
> > case _STAG_ENDDEP:
> > if (size != 0)
> > goto __skip;
> > - priority = 0;
> > - dev_dbg(&dev->dev, "end dependent options\n");
> > + option_flags = pnp_independent_option();
>
> (here is the bugfix)
Yep, I split into a separate patch as mentioned above.
> > -static int pnp_assign_resources(struct pnp_dev *dev, int depnum)
> > +static int pnp_assign_resources(struct pnp_dev *dev, int set)
> > {
> > ...
> Mmm. Previously when it failed halfway through it re-cleared the
> resources (at label fail). Does it want to do so now also?
Yes, I suppose so. I can't remember why I removed that, but it
shouldn't hurt, so I added it back.
> > mutex_unlock(&pnp_res_mutex);
> > - dbg_pnp_show_resources(dev, "after pnp_assign_resources");
> > - return 1;
> > -
> > -fail:
> > - pnp_clean_resource_table(dev);
> > - mutex_unlock(&pnp_res_mutex);
> > - dbg_pnp_show_resources(dev, "after pnp_assign_resources (failed)");
> > - return 0;
> > + if (ret == 0)
> > + dbg_pnp_show_resources(dev, "pnp_assign_resources succeeded");
> > + else
> > + dev_dbg(&dev->dev, "pnp_assign_resources failed (%d)\n", ret);
> > + return ret;
> > }
> >
> > /**
> > @@ -332,29 +282,21 @@ fail:
> > */
> > int pnp_auto_config_dev(struct pnp_dev *dev)
> > {
> > - struct pnp_option *dep;
> > - int i = 1;
> > + int i, ret = 0;
> >
> > if (!pnp_can_configure(dev)) {
> > dev_dbg(&dev->dev, "configuration not supported\n");
> > return -ENODEV;
> > }
> >
> > - if (!dev->dependent) {
> > - if (pnp_assign_resources(dev, 0))
> > + for (i = 0; i == 0 || i < dev->num_dependent_sets; i++) {
> > + ret = pnp_assign_resources(dev, i);
> > + if (ret == 0)
> > return 0;
>
> Eeeew. Perhaps:
>
> i = 0;
> do {
> ret = pnp_assign_resources(dev, i);
> if (ret == 0)
> return 0;
> } while (++i < dev->num_dependent_sets);
Heh :-) I vacillated on that one because I have a personal aversion
to "do { ... } while ()", especially with a pre-increment. How would
you feel about this alternative?
ret = pnp_assign_resources(dev, 0);
if (ret == 0)
return 0;
for (i = 1; i < dev->num_dependent_sets; i++) {
ret = pnp_assign_resources(dev, i);
if (ret == 0)
return 0;
}
> > - port2->min += 0x400;
> > - port2->max += 0x400;
> > - port3->min += 0x800;
> > - port3->max += 0x800;
>
> Why do you do 0x800, 0x400 in that order? Shouldn't it just be 0x400,
> 0x800 to mimick the old order?
I think they do end up in the correct order because I'm passing the
same list_head to both list_add() calls, e.g., we'll have something
like this:
io -> ...
io -> (io + 0x800) -> ...
io -> (io + 0x400) -> (io + 0x800) -> ...
> > + } else if (option->type == IORESOURCE_DMA) {
> > + dma = &option->u.dma;
> > if ((dma->flags & IORESOURCE_DMA_TYPE_MASK) ==
> > - IORESOURCE_DMA_8BIT)
> > + IORESOURCE_DMA_8BIT &&
> > + dma->map != 0x000A) {
>
> It's a char, so 0x0A? 0x000A makes it looks like it's 16-bit.
>
> > + dev_info(&dev->dev, "changing possible "
> > + "DMA channel mask in option set %d "
> > + "from %#x to 0xA (1, 3)\n",
> > + pnp_option_set(option), dma->map);
> > dma->map = 0x000A;
>
> And here...
Makes sense; I changed this as well as the dev_info() formats.
> > /*
> > * The default range on the mpu port for these devices is 0x388-0x388.
> > * Here we increase that range so that two such cards can be
> > * auto-configured.
> > */
>
> While you're there -- s/mpu/OPL/.
Done.
> > +static void quirk_ad1815_mpu_resources(struct pnp_dev *dev)
> > {
> > - struct pnp_option *res;
> > + struct pnp_option *option, *irq_option = NULL;
> > + unsigned int independent_irqs = 0;
> > +
> > + list_for_each_entry(option, &dev->options, list) {
> > + if (option->type == IORESOURCE_IRQ &&
> > + !pnp_option_is_dependent(option)) {
> > + independent_irqs++;
> > + irq_option = option;
> > + }
> > + }
> >
> > - res = dev->dependent;
> > - if (!res)
> > + if (independent_irqs != 1)
> > return;
> >
> > - while (res->next)
> > - res = res->next;
> > + irq_option->flags |= IORESOURCE_IRQ_OPTIONAL;
>
> You're in maze of unsigned variables, all named flags...
>
> irq_option->u.irq.flags |= IORESOURCE_IRQ_OPTIONAL;
>
> This was the thing that made things not work when I tested -- on an
> AD1815...
Ah, of course, thanks. I changed "irq_option" to a "struct pnp_irq *irq"
like I should have done to begin with.
> > + dev_info(&dev->dev, "made independent IRQ optional\n");
> >
> > - res->next = quirk_isapnp_mpu_options(dev);
> > + quirk_add_irq_optional_dependent_sets(dev);
>
> And as before, this one be gone.
OK, great. I wasn't sure I had understood those quirks correctly,
so thanks for pointing this out.
> > -struct pnp_option *pnp_build_option(int priority)
> > +struct pnp_option *pnp_build_option(struct pnp_dev *dev,
> > + unsigned long type,
> > + unsigned int option_flags)
> > {
> > - struct pnp_option *option = pnp_alloc(sizeof(struct pnp_option));
> > + struct pnp_option *option;
> >
> > + option = kzalloc(sizeof(struct pnp_option), GFP_KERNEL);
> > if (!option)
> > return NULL;
> >
> > - option->priority = priority & 0xff;
> > - /* make sure the priority is valid */
> > - if (option->priority > PNP_RES_PRIORITY_FUNCTIONAL)
> > - option->priority = PNP_RES_PRIORITY_INVALID;
>
> Dropping this made the 0x100 | prio ISAPnP/PnPBIOS bug appear. That
> 0x100 should go anyway but perhaps this limiting is still a good idea?
Yes; I added the check back to pnp_dependent_option().
PNP_RES_PRIORITY_INVALID was previously 65536, which requires a 16-bit
field. I changed it to 0xfff to fit in the 12-bit field I mentioned
earlier.
I need to go back over all your comments and make sure I've addressed
them all, then I'll post the revised patches, hopefully tomorrow.
Thanks again for all your work reviewing and testing these. It's
been incredibly useful.
Bjorn
On 03-06-08 21:57, Rene Herman wrote:
> The other quirks look good -- but I have the hardware and will test them
> specifically.
The other quirks are good (In the sense that they work as before. The
comments below doubting them have nothing to do with Bjorn reworking
them). The options display is with the 0x100 | prio thing fixed locally
already.
== 1 == quirk_awe32_resources, CTL0022:
* pre-quirk:
Dependent: 00 - Priority preferred
port 0x620-0x620, align 0x0, size 0x4, 16-bit address decoding
Dependent: 01 - Priority acceptable
port 0x620-0x680, align 0x1f, size 0x4, 16-bit address decoding
* post-quirk:
Dependent: 00 - Priority preferred
port 0x620-0x620, align 0x0, size 0x4, 16-bit address decoding
port 0xa20-0xa20, align 0x0, size 0x4, 16-bit address decoding
port 0xe20-0xe20, align 0x0, size 0x4, 16-bit address decoding
Dependent: 01 - Priority acceptable
port 0x620-0x680, align 0x1f, size 0x4, 16-bit address decoding
port 0xa20-0xa80, align 0x1f, size 0x4, 16-bit address decoding
port 0xe20-0xe80, align 0x1f, size 0x4, 16-bit address decoding
I actually expect this quirk to be totally bogus. This adds ports which
isapnp_set_resources is then going to try to program into hardware which
wouldn't be expecting them. The ALSA driver only references port0 (the
0x620 one) and hardcodes the two other ports at +0x400 and +0x800
itself. I expect the issue is simply that the offsetted ones alias and
somebody once thought this could be a way to just reserve those or
something.
It's been there since the dawning of time though. Will look at some
other time. Things work fine.
== 2 == quirk_cmi8330_resources, @X@0001 (Invalid ID!)
* pre-quirk:
Dependent: 00 - Priority preferred
port 0x220-0x220, align 0x0, size 0x10, 10-bit address decoding
irq 5 High-Edge
dma 1 8-bit byte-count compatible
dma 5 16-bit word-count compatible
Dependent: 01 - Priority acceptable
port 0x220-0x240, align 0x1f, size 0x10, 10-bit address decoding
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,7 16-bit word-count compatible
Dependent: 02 - Priority acceptable
port 0x220-0x240, align 0x1f, size 0x10, 10-bit address decoding
irq 5,7,2/9,10,11,12 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,7 16-bit word-count compatible
* post-quirk:
Dependent: 00 - Priority preferred
port 0x220-0x220, align 0x0, size 0x10, 10-bit address decoding
irq 5,7,10 High-Edge
dma 1,3 8-bit byte-count compatible
dma 5 16-bit word-count compatible
Dependent: 01 - Priority acceptable
port 0x220-0x240, align 0x1f, size 0x10, 10-bit address decoding
irq 5,7,10 High-Edge
dma 1,3 8-bit byte-count compatible
dma 5,7 16-bit word-count compatible
Dependent: 02 - Priority acceptable
port 0x220-0x240, align 0x1f, size 0x10, 10-bit address decoding
irq 5,7,10 High-Edge
dma 1,3 8-bit byte-count compatible
dma 5,7 16-bit word-count compatible
I'll need to verify if this wasn't just someone with a busted DMA
channel 0 and too few free interrupts. Unlikely quirk...
== 3 == quirk_sb16audio_resources, CTL0042:
* pre-quirk:
Dependent: 00 - Priority preferred
irq 5 High-Edge
dma 1 8-bit byte-count compatible
dma 5 16-bit word-count compatible
port 0x220-0x220, align 0x0, size 0x10, 16-bit address decoding
port 0x330-0x330, align 0x0, size 0x2, 16-bit address decoding
port 0x388-0x388, align 0x0, size 0x4, 16-bit address decoding
Dependent: 01 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,6,7 16-bit word-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0x2f, size 0x2, 16-bit address decoding
port 0x388-0x388, align 0x0, size 0x4, 16-bit address decoding
Dependent: 02 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,6,7 16-bit word-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0x2f, size 0x2, 16-bit address decoding
Dependent: 03 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,6,7 16-bit word-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
Dependent: 04 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0x2f, size 0x2, 16-bit address decoding
port 0x388-0x388, align 0x0, size 0x4, 16-bit address decoding
Dependent: 05 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0x2f, size 0x2, 16-bit address decoding
Dependent: 06 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
Dependent: 07 - Priority functional
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,6,7 16-bit word-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
port 0x388-0x394, align 0x3, size 0x4, 16-bit address decoding
* post-quirk:
Dependent: 00 - Priority preferred
irq 5 High-Edge
dma 1 8-bit byte-count compatible
dma 5 16-bit word-count compatible
port 0x220-0x220, align 0x0, size 0x10, 16-bit address decoding
port 0x330-0x330, align 0x0, size 0x2, 16-bit address decoding
port 0x388-0x3f8, align 0x0, size 0x4, 16-bit address decoding
Dependent: 01 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,6,7 16-bit word-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0x2f, size 0x2, 16-bit address decoding
port 0x388-0x3f8, align 0x0, size 0x4, 16-bit address decoding
Dependent: 02 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,6,7 16-bit word-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0x2f, size 0x2, 16-bit address decoding
Dependent: 03 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,6,7 16-bit word-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
Dependent: 04 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0x2f, size 0x2, 16-bit address decoding
port 0x388-0x3f8, align 0x0, size 0x4, 16-bit address decoding
Dependent: 05 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0x2f, size 0x2, 16-bit address decoding
Dependent: 06 - Priority acceptable
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
Dependent: 07 - Priority functional
irq 5,7,2/9,10 High-Edge
dma 0,1,3 8-bit byte-count compatible
dma 5,6,7 16-bit word-count compatible
port 0x220-0x280, align 0x1f, size 0x10, 16-bit address decoding
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
port 0x388-0x394, align 0x3, size 0x4, 16-bit address decoding
I'll need to verify if the OPL chip in fact works when not on 0x388 (to
0x394 as set 7 allows for) or if this was just someone believing it
would. I haven't tested all supported IDs (I might have them all) but if
there are none that do _not_ allow the OPL to be completely absent as
this CTL0042 (an early Soundblaster AWE64) does, this quirk should just
go anyway.
=== 4 === quirk_ad1815_mpu_resources, ADS7151
These results are with the dependent cloning already removed locally.
* pre-quirk:
irq 5,7,2/9,11,12 High-Edge
Dependent: 00 - Priority preferred
port 0x330-0x330, align 0x0, size 0x2, 10-bit address decoding
Dependent: 01 - Priority acceptable
port 0x300-0x300, align 0x0, size 0x2, 10-bit address decoding
Dependent: 02 - Priority functional
port 0x100-0x3fe, align 0x1, size 0x2, 10-bit address decoding
* post-quirk:
irq 5,7,2/9,11,12 High-Edge
Dependent: 00 - Priority preferred
port 0x330-0x330, align 0x0, size 0x2, 10-bit address decoding
Dependent: 01 - Priority acceptable
port 0x300-0x300, align 0x0, size 0x2, 10-bit address decoding
Dependent: 02 - Priority functional
port 0x100-0x3fe, align 0x1, size 0x2, 10-bit address decoding
See why that "(optional)" flag display is good? :-) I ofcourse tested
things and the flag works great...
== 5 == quirk_add_irq_optional_dependent_sets, ADS7181
* pre-quirk:
Dependent: 00 - Priority preferred
irq 2/9 High-Edge
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 01 - Priority acceptable
irq 2/9 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 02 - Priority functional
irq 2/9,10,11 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
* post-quirk:
Dependent: 00 - Priority preferred
irq 2/9 High-Edge
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 01 - Priority acceptable
irq 2/9 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 02 - Priority functional
irq 2/9,10,11 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 03 - Priority functional
irq 2/9 High-Edge
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 04 - Priority functional
irq 2/9 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 05 - Priority functional
irq 2/9,10,11 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Again needs the flag display to make sense but as intended and works fine.
The PCI one didn't change here...
Rene.
On 04-06-08 01:03, Bjorn Helgaas wrote:
> That's definitely backwards. I reversed the sizes, so we'll have
> 8 bits for the priority byte (including compatibility/performance/
> robustness) and 16 bits for the dependent set number. Actually,
> I made the priority field 12 bits so we'd have space to keep
> PNP_RES_PRIORITY_INVALID as a truly out-of-band value.
Sounds perfect.
>>> + for (i = 0; i == 0 || i < dev->num_dependent_sets; i++) {
>>> + ret = pnp_assign_resources(dev, i);
>>> + if (ret == 0)
>>> return 0;
>> Eeeew. Perhaps:
>>
>> i = 0;
>> do {
>> ret = pnp_assign_resources(dev, i);
>> if (ret == 0)
>> return 0;
>> } while (++i < dev->num_dependent_sets);
>
> Heh :-) I vacillated on that one because I have a personal aversion
> to "do { ... } while ()", especially with a pre-increment. How would
> you feel about this alternative?
>
> ret = pnp_assign_resources(dev, 0);
> if (ret == 0)
> return 0;
>
> for (i = 1; i < dev->num_dependent_sets; i++) {
> ret = pnp_assign_resources(dev, i);
> if (ret == 0)
> return 0;
> }
You could fix the pre-increment by sticking a i++ inside the loop body
but there's no arguing with personal aversions...
Yes, I think the latter is better. Straight-forward and clear.
>> Why do you do 0x800, 0x400 in that order? Shouldn't it just be 0x400,
>> 0x800 to mimick the old order?
>
> I think they do end up in the correct order because I'm passing the
> same list_head to both list_add() calls, e.g., we'll have something
> like this:
>
> io -> ...
> io -> (io + 0x800) -> ...
> io -> (io + 0x400) -> (io + 0x800) -> ...
Yep. Just needed to see it happen once in the quirk testing I just now did.
> I need to go back over all your comments and make sure I've addressed
> them all, then I'll post the revised patches, hopefully tomorrow.
>
> Thanks again for all your work reviewing and testing these. It's
> been incredibly useful.
I've been impressed by this work. This is a good redesign of PnP with a
fully bisectable way to get there. And PnP was in need of some work...
Rene.
On 04-06-08 01:52, Rene Herman wrote:
Mmm. If you care for it:
> === 4 === quirk_ad1815_mpu_resources, ADS7151
>
> These results are with the dependent cloning already removed locally.
>
> * pre-quirk:
>
> irq 5,7,2/9,11,12 High-Edge
> Dependent: 00 - Priority preferred
> port 0x330-0x330, align 0x0, size 0x2, 10-bit address decoding
> Dependent: 01 - Priority acceptable
> port 0x300-0x300, align 0x0, size 0x2, 10-bit address decoding
> Dependent: 02 - Priority functional
> port 0x100-0x3fe, align 0x1, size 0x2, 10-bit address decoding
>
> * post-quirk:
>
> irq 5,7,2/9,11,12 High-Edge
> Dependent: 00 - Priority preferred
> port 0x330-0x330, align 0x0, size 0x2, 10-bit address decoding
> Dependent: 01 - Priority acceptable
> port 0x300-0x300, align 0x0, size 0x2, 10-bit address decoding
> Dependent: 02 - Priority functional
> port 0x100-0x3fe, align 0x1, size 0x2, 10-bit address decoding
>
>
> See why that "(optional)" flag display is good? :-) I ofcourse tested
> things and the flag works great...
>
>
> == 5 == quirk_add_irq_optional_dependent_sets, ADS7181
>
> * pre-quirk:
>
> Dependent: 00 - Priority preferred
> irq 2/9 High-Edge
> port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 01 - Priority acceptable
> irq 2/9 High-Edge
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 02 - Priority functional
> irq 2/9,10,11 High-Edge
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
>
> * post-quirk:
>
> Dependent: 00 - Priority preferred
> irq 2/9 High-Edge
> port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 01 - Priority acceptable
> irq 2/9 High-Edge
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 02 - Priority functional
> irq 2/9,10,11 High-Edge
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 03 - Priority functional
> irq 2/9 High-Edge
> port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 04 - Priority functional
> irq 2/9 High-Edge
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 05 - Priority functional
> irq 2/9,10,11 High-Edge
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
ADS7181 in fact might as well delete the IRQ from the dependents and add
"irq 2/9,10,11 High-Edge Optinal" in front as an independent same as
ADS7151. That way, all the cloning can go.
I'll probably place that on top if you don't, but you might already want
to since it loses all that code. Only difference would be that if IRQ 9
is taken and 10 free, the new situation would grab 330/10 while the old
would've taken 300/10, but that's in fact better...
Rene.
On Wednesday 04 June 2008 05:48:27 am Rene Herman wrote:
> On 04-06-08 01:52, Rene Herman wrote:
> ADS7181 in fact might as well delete the IRQ from the dependents and add
> "irq 2/9,10,11 High-Edge Optinal" in front as an independent same as
> ADS7151. That way, all the cloning can go.
We currently clone for AZT0002 as well as ADS7181. Can we do the
same for both? It would be nice to get rid of the cloning code
if we can.
Bjorn
On 04-06-08 22:50, Bjorn Helgaas wrote:
> On Wednesday 04 June 2008 05:48:27 am Rene Herman wrote:
>> On 04-06-08 01:52, Rene Herman wrote:
>
>> ADS7181 in fact might as well delete the IRQ from the dependents and add
>> "irq 2/9,10,11 High-Edge Optinal" in front as an independent same as
>> ADS7151. That way, all the cloning can go.
>
> We currently clone for AZT0002 as well as ADS7181. Can we do the
> same for both? It would be nice to get rid of the cloning code
> if we can.
Yes. AZT0002 (the MPU401 on an AZT2320 chip) is the exact same as
ADS7181 (the MPU401 on an AD1816 chip).
IORESOURCE_IRQ_OPTIONAL clears the path for doing things better. I see
its dependent 2 can then just go entirely in fact.
Hardware says:
Dependent: 00 - Priority preferred
irq 2/9 High-Edge
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 01 - Priority acceptable
irq 2/9 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 02 - Priority functional
irq 2/9,10,11 High-Edge
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
We want it to end up as:
irq 2/9,10,11 High-Edge (Optional)
Dependent: 00 - Priority preferred
port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
Dependent: 01 - Priority acceptable
port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
So walk dependents deleting IRQs, except last dependent IRQ which is
cloned into a new independent (inserted in front would be best for
ISAPnP since the spec does recommend this) while making it optional and
then just delete the last dependent completely (it would be same as the
previous dependent after all).
Rene.
On Wednesday 04 June 2008 04:31:02 pm Rene Herman wrote:
> On 04-06-08 22:50, Bjorn Helgaas wrote:
>
> > On Wednesday 04 June 2008 05:48:27 am Rene Herman wrote:
> >> On 04-06-08 01:52, Rene Herman wrote:
> >
> >> ADS7181 in fact might as well delete the IRQ from the dependents and add
> >> "irq 2/9,10,11 High-Edge Optinal" in front as an independent same as
> >> ADS7151. That way, all the cloning can go.
> >
> > We currently clone for AZT0002 as well as ADS7181. Can we do the
> > same for both? It would be nice to get rid of the cloning code
> > if we can.
>
> Yes. AZT0002 (the MPU401 on an AZT2320 chip) is the exact same as
> ADS7181 (the MPU401 on an AD1816 chip).
>
> IORESOURCE_IRQ_OPTIONAL clears the path for doing things better. I see
> its dependent 2 can then just go entirely in fact.
>
> Hardware says:
>
> Dependent: 00 - Priority preferred
> irq 2/9 High-Edge
> port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 01 - Priority acceptable
> irq 2/9 High-Edge
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 02 - Priority functional
> irq 2/9,10,11 High-Edge
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
>
> We want it to end up as:
>
> irq 2/9,10,11 High-Edge (Optional)
> Dependent: 00 - Priority preferred
> port 0x330-0x330, align 0xf, size 0x2, 16-bit address decoding
> Dependent: 01 - Priority acceptable
> port 0x300-0x330, align 0xf, size 0x2, 16-bit address decoding
>
> So walk dependents deleting IRQs, except last dependent IRQ which is
> cloned into a new independent (inserted in front would be best for
> ISAPnP since the spec does recommend this) while making it optional and
> then just delete the last dependent completely (it would be same as the
> previous dependent after all).
That would probably simplify things a bit. Although in the general
case, I suppose we'd want to make sure the options we delete are
a subset of the independent one we add. Will look at this more
tomorrow.
Bjorn