Re: [PATCH 2/2] irqdomain: Support one-shot tear down of domainmappings.

From: Paul Mundt
Date: Fri Jun 15 2012 - 18:34:04 EST


On Fri, Jun 15, 2012 at 12:35:18PM -0600, Grant Likely wrote:
> On Wed, 13 Jun 2012 16:34:01 +0900, Paul Mundt <lethal@xxxxxxxxxxxx> wrote:
> > +/**
> > + * irq_domain_dispose_mappings() - Dispose of all mappings in a domain
> > + * @domain: domain to tear down
> > + */
> > +void irq_domain_dispose_mappings(struct irq_domain *domain)
> > +{
> > + if (domain->revmap_type == IRQ_DOMAIN_MAP_NOMAP)
> > + return;
> > +
> > + if (domain->linear_size) {
> > + int i;
> > +
> > + for (i = 0; i < domain->linear_size; i++)
> > + irq_dispose_mapping(domain->linear_revmap[i]);
> > + } else {
>
> The 'else' should be dropped I think. With the merge of linear and
> tree mappings into the same domain type, it is possible for a domain
> to have both linear and tree mappings in the same instance. So tree
> needs to be cleared even if linear_size is non-zero.
>
Ok, that's fine. Though by that same logic wouldn't it also be possible
for a domain to have multiple linear ranges?

> > + struct irq_data *irq_data_batch[IRQ_DOMAIN_BATCH_IRQS];
> > + unsigned int nr_found;
> > + unsigned long index = 0;
> > +
> > + rcu_read_lock();
> > +
> > + for (;;) {
>
> Nit; while not 'while(1)'?
>
Personal taste, though really it's probably better to just do a
do { ... } while (nr_found == IRQ_DOMAIN_BATCH_IRQS), which will save an
iteration when we've run out of tree.

> > + int i;
>
> Nit; move 'i' up with nr_found above.
>
I prefer to have my variables defined in the scope in which they are
used. Though obviously they'll be the same now with consolidation.

> > +
> > + nr_found = radix_tree_gang_lookup(&domain->radix_tree,
> > + (void **)irq_data_batch, index,
> > + ARRAY_SIZE(irq_data_batch));
> > + if (!nr_found)
> > + break;
>
> Or better:
>
> while (nr_found = radix_tree_gang_lookup(&domain->radix_tree,
> (void **)irq_data_batch, index,
> ARRAY_SIZE(irq_data_batch))) {
>
That'll still loop even if nr_found returns less than the batch size
(which only ocurred to me after I sent the patch), so the do { } while
will at least save a superfluous lookup.

I'll respin with the above rework.
--
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/