Messages in this thread | | | Date | Tue, 03 Jun 2008 23:13:37 +0200 | From | Rene Herman <> | Subject | Re: [patch 15/15] PNP: convert resource options to single linked list |
| |
On 31-05-08 00:49, Bjorn Helgaas wrote:
Forgot this comment:
> --- 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
> +static struct pnp_option *pnp_clone_dependent_set(struct pnp_dev *dev, > + unsigned int set) > { > - struct pnp_option *head = NULL; > - struct pnp_option *prev = NULL; > - struct pnp_option *res; > - > - /* > - * Build a functional IRQ-optional variant of each MPU option. > - */ > - > - for (res = dev->dependent; res; res = res->next) { > - struct pnp_option *curr; > - struct pnp_port *port; > - struct pnp_port *copy_port; > - struct pnp_irq *irq; > - struct pnp_irq *copy_irq; > - > - port = res->port; > - irq = res->irq; > - if (!port || !irq) > - continue; > + struct pnp_option *tail = NULL, *first_new_option = NULL; > + struct pnp_option *option, *new_option; > + unsigned int flags; > + > + list_for_each_entry(option, &dev->options, list) { > + if (pnp_option_is_dependent(option)) > + tail = option; > + } > + if (!tail) { > + dev_err(&dev->dev, "no dependent option sets\n"); > + return NULL; > + } > > - copy_port = pnp_alloc(sizeof *copy_port); > - if (!copy_port) > - break; > - > - copy_irq = pnp_alloc(sizeof *copy_irq); > - if (!copy_irq) { > - kfree(copy_port); > - break; > - } > + flags = pnp_dependent_option(dev, PNP_RES_PRIORITY_FUNCTIONAL); > + list_for_each_entry(option, &dev->options, list) { > + if (pnp_option_is_dependent(option) && > + pnp_option_set(option) == set) { > + new_option = kmalloc(sizeof(struct pnp_option), > + GFP_KERNEL); > + if (!new_option) { > + dev_err(&dev->dev, "couldn't clone dependent " > + "set %d\n", set); > + return NULL; > + } > > - *copy_port = *port; > - copy_port->next = NULL; > + *new_option = *option; > + new_option->flags = flags; > + if (!first_new_option) > + first_new_option = new_option; > > - *copy_irq = *irq; > - copy_irq->flags |= IORESOURCE_IRQ_OPTIONAL; > - copy_irq->next = NULL; > - > - curr = pnp_build_option(PNP_RES_PRIORITY_FUNCTIONAL); > - if (!curr) { > - kfree(copy_port); > - kfree(copy_irq); > - break; > + list_add(&new_option->list, &tail->list); > + tail = new_option; > } > - curr->port = copy_port; > - curr->irq = copy_irq; > - > - if (prev) > - prev->next = curr; > - else > - head = curr; > - prev = curr; > } > - if (head) > - dev_info(&dev->dev, "adding IRQ-optional MPU options\n"); > > - return head; > + return first_new_option; > }
This works, but I did the "disconnected chain build" that I did due to finding it particularly clumsy to add to the dependent chain while walking it.
This avoids actual problems due to num_sets = dev->num_dependents_sets in caller and the pnp_option_set(option) == set in the loop (not unlike the first version of the original checked for a present irq) but it's again checking the options it just added itself.
If you feel that's perfectly fine then <shrug> but thought I'd still remark on it in case it wasn't intentional.
Rene.
| |