Re: [patch 15/15] PNP: convert resource options to single linkedlist

From: Rene Herman
Date: Tue Jun 03 2008 - 17:14:05 EST


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.
--
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/