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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120615223403.GB7357@linux-sh.org>
Date:	Sat, 16 Jun 2012 07:34:03 +0900
From:	Paul Mundt <lethal@...ux-sh.org>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	linux-sh@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] irqdomain: Support one-shot tear down of domain
 mappings.

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@...ux-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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/