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

[Paul Mundt <lethal@linux-sh.org>]

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@linux-sh.org> 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.