[PATCH 1/2] irqdomain: Fix up linear revmap for non-zero hwirq displacement.

[PATCH 1/2] irqdomain: Fix up linear revmap for non-zero hwirq displacement.

[Paul Mundt]

Presently the linear revmap code assumes that all hwirqs start at 0,
using the hwirq directly as an index value for the lookup. In the case of
legacy revmaps this isn't necessarily the case, as the first_hwirq value
passed in can be non-zero, causing those types of users to silently have
their IRQs placed in the radix tree instead.

With this change, hwirq displacement is factored in at association time
directly. This also makes it possible for non-legacy users to use linear
revmaps regardless of hwirq base position. This could potentially lead to
a bug if there's an attempt to associate multiple times in to the linear
map in a nonsensical and non-linear order, but at that point being
silently punted to the radix tree is likely to be the least of your
concerns (in such a case it's fairly trivial to simply extend
irq_domain_add_linear() to take a hwirq base and move the linear base
assignment there).

Signed-off-by: Paul Mundt <lethal@linux-sh.org>
---
 include/linux/irqdomain.h |    1 +
 kernel/irq/irqdomain.c    |   18 +++++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 775765d..58defd5 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -97,6 +97,7 @@ struct irq_domain {
 	/* Reverse mapping data */
 	unsigned int nomap_max_irq;
 	struct radix_tree_root radix_tree;
+	unsigned int linear_start;
 	unsigned int linear_size;
 	unsigned int linear_revmap[];
 };
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 906307b..8591cb6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -55,6 +55,7 @@ static struct irq_domain *irq_domain_alloc(struct device_node *of_node,
 	domain->ops = ops;
 	domain->host_data = host_data;
 	domain->of_node = of_node_get(of_node);
+	domain->linear_start = 0;
 	domain->linear_size = size;
 
 	return domain;
@@ -268,8 +269,8 @@ static void irq_domain_disassociate_many(struct irq_domain *domain,
 		irq_data->hwirq = 0;
 
 		/* Clear reverse map */
-		if (hwirq < domain->linear_size)
-			domain->linear_revmap[hwirq] = 0;
+		if (hwirq - domain->linear_start < domain->linear_size)
+			domain->linear_revmap[hwirq - domain->linear_start] = 0;
 		else {
 			mutex_lock(&revmap_trees_mutex);
 			radix_tree_delete(&domain->radix_tree, hwirq);
@@ -288,6 +289,13 @@ int irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
 	pr_debug("%s(%s, irqbase=%i, hwbase=%i, count=%i)\n", __func__,
 		of_node_full_name(domain->of_node), irq_base, (int)hwirq_base, count);
 
+	/*
+	 * The linear revmap may not begin at 0, factor in the hwirq
+	 * displacement here.
+	 */
+	if (domain->linear_size)
+		domain->linear_start = hwirq_base;
+
 	for (i = 0; i < count; i++) {
 		struct irq_data *irq_data = irq_get_irq_data(virq + i);
 
@@ -311,8 +319,8 @@ int irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
 			goto err_unmap;
 		}
 
-		if (hwirq < domain->linear_size)
-			domain->linear_revmap[hwirq] = virq;
+		if (hwirq - domain->linear_start < domain->linear_size)
+			domain->linear_revmap[hwirq - domain->linear_start] = virq;
 		else {
 			mutex_lock(&revmap_trees_mutex);
 			radix_tree_insert(&domain->radix_tree, hwirq, irq_data);
@@ -585,7 +593,7 @@ unsigned int irq_linear_revmap(struct irq_domain *domain,
 	BUG_ON(domain->revmap_type != IRQ_DOMAIN_MAP_LINEAR);
 
 	/* Check revmap bounds; complain if exceeded */
-	if (hwirq >= domain->linear_size) {
+	if (hwirq - domain->linear_start >= domain->linear_size) {
 		rcu_read_lock();
 		data = radix_tree_lookup(&domain->radix_tree, hwirq);
 		rcu_read_unlock();
-- 
1.7.9.rc0.28.g0e1cf

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

[Paul Mundt]

This implements a sledgehammer approach for batched domain unmapping
regardless of domain type. In the linear revmap case it's fairly easy to
track state and iterate over the IRQs disposing of them one at a time,
but it's not always so straightforward in the radix tree case.

A new irq_domain_dispose_mappings() is added to ensure all of the
mappings are appropriately discarded, and is to be used prior to domain
removal. Gang lookups are batched, presently 16 at a time, for no reason
other than it seemed like a good number at the time.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>
---
 include/linux/irqdomain.h |    1 +
 kernel/irq/irqdomain.c    |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 58defd5..6f27c55 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -149,6 +149,7 @@ static inline int irq_domain_associate(struct irq_domain *domain, unsigned int i
 extern unsigned int irq_create_mapping(struct irq_domain *host,
 				       irq_hw_number_t hwirq);
 extern void irq_dispose_mapping(unsigned int virq);
+extern void irq_domain_dispose_mappings(struct irq_domain *domain);
 extern unsigned int irq_find_mapping(struct irq_domain *host,
 				     irq_hw_number_t hwirq);
 extern unsigned int irq_create_direct_mapping(struct irq_domain *host);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8591cb6..40c9ba1 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -546,6 +546,49 @@ void irq_dispose_mapping(unsigned int virq)
 }
 EXPORT_SYMBOL_GPL(irq_dispose_mapping);
 
+#define IRQ_DOMAIN_BATCH_IRQS	16
+
+/**
+ * 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 {
+		struct irq_data *irq_data_batch[IRQ_DOMAIN_BATCH_IRQS];
+		unsigned int nr_found;
+		unsigned long index = 0;
+
+		rcu_read_lock();
+
+		for (;;) {
+			int i;
+
+			nr_found = radix_tree_gang_lookup(&domain->radix_tree,
+					(void **)irq_data_batch, index,
+					ARRAY_SIZE(irq_data_batch));
+			if (!nr_found)
+				break;
+
+			for (i = 0; i < nr_found; i++)
+				irq_dispose_mapping(irq_data_batch[i]->irq);
+
+			index += nr_found;
+		}
+
+		rcu_read_unlock();
+	}
+}
+EXPORT_SYMBOL_GPL(irq_domain_dispose_mappings);
+
 /**
  * irq_find_mapping() - Find a linux irq from an hw irq number.
  * @domain: domain owning this hardware interrupt
-- 
1.7.9.rc0.28.g0e1cf

Re: [PATCH 1/2] irqdomain: Fix up linear revmap for non-zero hwirq displacement.

[Grant Likely]

On Wed, 13 Jun 2012 16:34:00 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> Presently the linear revmap code assumes that all hwirqs start at 0,
> using the hwirq directly as an index value for the lookup. In the case of
> legacy revmaps this isn't necessarily the case, as the first_hwirq value
> passed in can be non-zero, causing those types of users to silently have
> their IRQs placed in the radix tree instead.
> 
> With this change, hwirq displacement is factored in at association time
> directly. This also makes it possible for non-legacy users to use linear
> revmaps regardless of hwirq base position. This could potentially lead to
> a bug if there's an attempt to associate multiple times in to the linear
> map in a nonsensical and non-linear order, but at that point being
> silently punted to the radix tree is likely to be the least of your
> concerns (in such a case it's fairly trivial to simply extend
> irq_domain_add_linear() to take a hwirq base and move the linear base
> assignment there).

I actually hoped to be rid of the whole hwirq start offset thing.
Doing without it simplifies the code, is slightly faster.  I suspect
very few controllers actually need it, and for those that do I'm
hoping the wasted space is in the order of 0-32 words.

Instead of this, can we change the affected controllers to use the
maximum hwirq number when setting the size of the linear map?

Do you have hardware where the first hwirq is a >32 number?

g.

> 
> Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> ---
>  include/linux/irqdomain.h |    1 +
>  kernel/irq/irqdomain.c    |   18 +++++++++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 775765d..58defd5 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -97,6 +97,7 @@ struct irq_domain {
>  	/* Reverse mapping data */
>  	unsigned int nomap_max_irq;
>  	struct radix_tree_root radix_tree;
> +	unsigned int linear_start;
>  	unsigned int linear_size;
>  	unsigned int linear_revmap[];
>  };
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 906307b..8591cb6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -55,6 +55,7 @@ static struct irq_domain *irq_domain_alloc(struct device_node *of_node,
>  	domain->ops = ops;
>  	domain->host_data = host_data;
>  	domain->of_node = of_node_get(of_node);
> +	domain->linear_start = 0;
>  	domain->linear_size = size;
>  
>  	return domain;
> @@ -268,8 +269,8 @@ static void irq_domain_disassociate_many(struct irq_domain *domain,
>  		irq_data->hwirq = 0;
>  
>  		/* Clear reverse map */
> -		if (hwirq < domain->linear_size)
> -			domain->linear_revmap[hwirq] = 0;
> +		if (hwirq - domain->linear_start < domain->linear_size)
> +			domain->linear_revmap[hwirq - domain->linear_start] = 0;
>  		else {
>  			mutex_lock(&revmap_trees_mutex);
>  			radix_tree_delete(&domain->radix_tree, hwirq);
> @@ -288,6 +289,13 @@ int irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
>  	pr_debug("%s(%s, irqbase=%i, hwbase=%i, count=%i)\n", __func__,
>  		of_node_full_name(domain->of_node), irq_base, (int)hwirq_base, count);
>  
> +	/*
> +	 * The linear revmap may not begin at 0, factor in the hwirq
> +	 * displacement here.
> +	 */
> +	if (domain->linear_size)
> +		domain->linear_start = hwirq_base;
> +
>  	for (i = 0; i < count; i++) {
>  		struct irq_data *irq_data = irq_get_irq_data(virq + i);
>  
> @@ -311,8 +319,8 @@ int irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
>  			goto err_unmap;
>  		}
>  
> -		if (hwirq < domain->linear_size)
> -			domain->linear_revmap[hwirq] = virq;
> +		if (hwirq - domain->linear_start < domain->linear_size)
> +			domain->linear_revmap[hwirq - domain->linear_start] = virq;
>  		else {
>  			mutex_lock(&revmap_trees_mutex);
>  			radix_tree_insert(&domain->radix_tree, hwirq, irq_data);
> @@ -585,7 +593,7 @@ unsigned int irq_linear_revmap(struct irq_domain *domain,
>  	BUG_ON(domain->revmap_type != IRQ_DOMAIN_MAP_LINEAR);
>  
>  	/* Check revmap bounds; complain if exceeded */
> -	if (hwirq >= domain->linear_size) {
> +	if (hwirq - domain->linear_start >= domain->linear_size) {
>  		rcu_read_lock();
>  		data = radix_tree_lookup(&domain->radix_tree, hwirq);
>  		rcu_read_unlock();
> -- 
> 1.7.9.rc0.28.g0e1cf
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

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

[Grant Likely]

On Wed, 13 Jun 2012 16:34:01 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> This implements a sledgehammer approach for batched domain unmapping
> regardless of domain type. In the linear revmap case it's fairly easy to
> track state and iterate over the IRQs disposing of them one at a time,
> but it's not always so straightforward in the radix tree case.
> 
> A new irq_domain_dispose_mappings() is added to ensure all of the
> mappings are appropriately discarded, and is to be used prior to domain
> removal. Gang lookups are batched, presently 16 at a time, for no reason
> other than it seemed like a good number at the time.
> 
> Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> ---
>  include/linux/irqdomain.h |    1 +
>  kernel/irq/irqdomain.c    |   43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 58defd5..6f27c55 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -149,6 +149,7 @@ static inline int irq_domain_associate(struct irq_domain *domain, unsigned int i
>  extern unsigned int irq_create_mapping(struct irq_domain *host,
>  				       irq_hw_number_t hwirq);
>  extern void irq_dispose_mapping(unsigned int virq);
> +extern void irq_domain_dispose_mappings(struct irq_domain *domain);
>  extern unsigned int irq_find_mapping(struct irq_domain *host,
>  				     irq_hw_number_t hwirq);
>  extern unsigned int irq_create_direct_mapping(struct irq_domain *host);
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 8591cb6..40c9ba1 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -546,6 +546,49 @@ void irq_dispose_mapping(unsigned int virq)
>  }
>  EXPORT_SYMBOL_GPL(irq_dispose_mapping);
>  
> +#define IRQ_DOMAIN_BATCH_IRQS	16
> +
> +/**
> + * 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.

> +		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)'?

> +			int i;

Nit; move 'i' up with nr_found above.

> +
> +			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))) {

Otherwise seems okay... though the gang lookup feels more complex than
it needs to be.

g.

> +
> +			for (i = 0; i < nr_found; i++)
> +				irq_dispose_mapping(irq_data_batch[i]->irq);
> +
> +			index += nr_found;
> +		}
> +
> +		rcu_read_unlock();
> +	}
> +}
> +EXPORT_SYMBOL_GPL(irq_domain_dispose_mappings);
> +
>  /**
>   * irq_find_mapping() - Find a linux irq from an hw irq number.
>   * @domain: domain owning this hardware interrupt
> -- 
> 1.7.9.rc0.28.g0e1cf
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

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

[Paul Mundt]

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.

Re: [PATCH 1/2] irqdomain: Fix up linear revmap for non-zero hwirq displacement.

[Paul Mundt]

On Fri, Jun 15, 2012 at 12:25:40PM -0600, Grant Likely wrote:
> On Wed, 13 Jun 2012 16:34:00 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> > Presently the linear revmap code assumes that all hwirqs start at 0,
> > using the hwirq directly as an index value for the lookup. In the case of
> > legacy revmaps this isn't necessarily the case, as the first_hwirq value
> > passed in can be non-zero, causing those types of users to silently have
> > their IRQs placed in the radix tree instead.
> > 
> > With this change, hwirq displacement is factored in at association time
> > directly. This also makes it possible for non-legacy users to use linear
> > revmaps regardless of hwirq base position. This could potentially lead to
> > a bug if there's an attempt to associate multiple times in to the linear
> > map in a nonsensical and non-linear order, but at that point being
> > silently punted to the radix tree is likely to be the least of your
> > concerns (in such a case it's fairly trivial to simply extend
> > irq_domain_add_linear() to take a hwirq base and move the linear base
> > assignment there).
> 
> I actually hoped to be rid of the whole hwirq start offset thing.
> Doing without it simplifies the code, is slightly faster.  I suspect
> very few controllers actually need it, and for those that do I'm
> hoping the wasted space is in the order of 0-32 words.
> 
> Instead of this, can we change the affected controllers to use the
> maximum hwirq number when setting the size of the linear map?
> 
> Do you have hardware where the first hwirq is a >32 number?
> 
Yes. On the CPU I was just working on I have two linear ranges and a
tree, one of the linear ranges begins at hwirq 56. On other CPUs we have
linear ranges that begin at 64, 72, etc. most of which are fairly low in
the space. On newer parts on the ARM side there are also controllers with
ranges that begin > 400.

I don't particularly care for the linear_start hack myself either, but I
couldn't think of any cleaner approach for it. The simplest might be if
we can just bury these details in a domain-specific canonicalization op
(distinctly different from xlate), and plug it in for the few cases that
need a non-zero hwirq base. I don't mind hacking that up if you're more
agreeable to that approach.

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

[Grant Likely]

On Sat, 16 Jun 2012 07:34:03 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> 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?

except that there is only one linear map.  It would be possible to
have multiple maps, but I'm going to resist adding a whole lot of
options for reverse mapping to keep the lookup overhead low.  A
sufficiently large linear map would also handle multiple linear maps.

g.

Re: [PATCH 1/2] irqdomain: Fix up linear revmap for non-zero hwirq displacement.

[Paul Mundt]

On Sat, Jun 16, 2012 at 08:14:19AM +0900, Paul Mundt wrote:
> On Fri, Jun 15, 2012 at 12:25:40PM -0600, Grant Likely wrote:
> > On Wed, 13 Jun 2012 16:34:00 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> > > Presently the linear revmap code assumes that all hwirqs start at 0,
> > > using the hwirq directly as an index value for the lookup. In the case of
> > > legacy revmaps this isn't necessarily the case, as the first_hwirq value
> > > passed in can be non-zero, causing those types of users to silently have
> > > their IRQs placed in the radix tree instead.
> > > 
> > > With this change, hwirq displacement is factored in at association time
> > > directly. This also makes it possible for non-legacy users to use linear
> > > revmaps regardless of hwirq base position. This could potentially lead to
> > > a bug if there's an attempt to associate multiple times in to the linear
> > > map in a nonsensical and non-linear order, but at that point being
> > > silently punted to the radix tree is likely to be the least of your
> > > concerns (in such a case it's fairly trivial to simply extend
> > > irq_domain_add_linear() to take a hwirq base and move the linear base
> > > assignment there).
> > 
> > I actually hoped to be rid of the whole hwirq start offset thing.
> > Doing without it simplifies the code, is slightly faster.  I suspect
> > very few controllers actually need it, and for those that do I'm
> > hoping the wasted space is in the order of 0-32 words.
> > 
> > Instead of this, can we change the affected controllers to use the
> > maximum hwirq number when setting the size of the linear map?
> > 
> > Do you have hardware where the first hwirq is a >32 number?
> > 
> Yes. On the CPU I was just working on I have two linear ranges and a
> tree, one of the linear ranges begins at hwirq 56. On other CPUs we have
> linear ranges that begin at 64, 72, etc. most of which are fairly low in
> the space. On newer parts on the ARM side there are also controllers with
> ranges that begin > 400.
> 
> I don't particularly care for the linear_start hack myself either, but I
> couldn't think of any cleaner approach for it. The simplest might be if
> we can just bury these details in a domain-specific canonicalization op
> (distinctly different from xlate), and plug it in for the few cases that
> need a non-zero hwirq base. I don't mind hacking that up if you're more
> agreeable to that approach.

Ping?

We can't do away with the first_irq thing in the legacy->linear merge
without at least having a strategy for getting existing users off of it.
Requiring the linear revmap to always begin at 0 seems like a significant
regression in functionality for marginal performance gain, so if you're
not willing to have the linear_start factored in we do need some other
alternative. I've proposed several, if you don't like any of those you
are welcome to propose an alternative. I don't mind doing the work one
way or the other, but I do mind losing the functionality.

Punishing legacy users with leading gaps in their revmap is likewise
undesirable, especially as that most of the in-tree users (regmap-irq
especially) of this functionality are using this legacy behaviour without
incident.

If you want to merge legacy in to linear in the name of simplification
that's fine, but killing useful functionality that people are presently
dependent on and others wish to use going forward in the name of
simplification seems like more of a step back than one forward.

Re: [PATCH 1/2] irqdomain: Fix up linear revmap for non-zero hwirq displacement.

[Grant Likely]

On Thu, 21 Jun 2012 10:19:40 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> On Sat, Jun 16, 2012 at 08:14:19AM +0900, Paul Mundt wrote:
> > On Fri, Jun 15, 2012 at 12:25:40PM -0600, Grant Likely wrote:
> > > On Wed, 13 Jun 2012 16:34:00 +0900, Paul Mundt <lethal@linux-sh.org> wrote:
> > > > Presently the linear revmap code assumes that all hwirqs start at 0,
> > > > using the hwirq directly as an index value for the lookup. In the case of
> > > > legacy revmaps this isn't necessarily the case, as the first_hwirq value
> > > > passed in can be non-zero, causing those types of users to silently have
> > > > their IRQs placed in the radix tree instead.
> > > > 
> > > > With this change, hwirq displacement is factored in at association time
> > > > directly. This also makes it possible for non-legacy users to use linear
> > > > revmaps regardless of hwirq base position. This could potentially lead to
> > > > a bug if there's an attempt to associate multiple times in to the linear
> > > > map in a nonsensical and non-linear order, but at that point being
> > > > silently punted to the radix tree is likely to be the least of your
> > > > concerns (in such a case it's fairly trivial to simply extend
> > > > irq_domain_add_linear() to take a hwirq base and move the linear base
> > > > assignment there).
> > > 
> > > I actually hoped to be rid of the whole hwirq start offset thing.
> > > Doing without it simplifies the code, is slightly faster.  I suspect
> > > very few controllers actually need it, and for those that do I'm
> > > hoping the wasted space is in the order of 0-32 words.
> > > 
> > > Instead of this, can we change the affected controllers to use the
> > > maximum hwirq number when setting the size of the linear map?
> > > 
> > > Do you have hardware where the first hwirq is a >32 number?
> > > 
> > Yes. On the CPU I was just working on I have two linear ranges and a
> > tree, one of the linear ranges begins at hwirq 56. On other CPUs we have
> > linear ranges that begin at 64, 72, etc. most of which are fairly low in
> > the space. On newer parts on the ARM side there are also controllers with
> > ranges that begin > 400.
> > 
> > I don't particularly care for the linear_start hack myself either, but I
> > couldn't think of any cleaner approach for it. The simplest might be if
> > we can just bury these details in a domain-specific canonicalization op
> > (distinctly different from xlate), and plug it in for the few cases that
> > need a non-zero hwirq base. I don't mind hacking that up if you're more
> > agreeable to that approach.
> 
> Ping?
> 
> We can't do away with the first_irq thing in the legacy->linear merge
> without at least having a strategy for getting existing users off of it.
> Requiring the linear revmap to always begin at 0 seems like a significant
> regression in functionality for marginal performance gain, so if you're
> not willing to have the linear_start factored in we do need some other
> alternative. I've proposed several, if you don't like any of those you
> are welcome to propose an alternative. I don't mind doing the work one
> way or the other, but I do mind losing the functionality.

>From another perspective, even if irqs do start at 400, that is wasted
space of 1600 bytes.  Less than half a page.  It still isn't a huge
amount.  The choice to use a linear vs. radix map is a choice between
speed and sparse flexability.  Considering that one of Ben's concerns is
preserving the fastest lookup path possible, I greatly prefer the
simplicity of a single offset between hwirq and irq.  If you want to
avoid it in your driver, I won't object, but it seems to me a case of
premature optimization.

However, I do agree that allocating 400 unused irq_descs would be a
problem, but the patches don't work that way.  irq_domain_add_legacy()
only calls irq_domain_associate_many() on the requested range of hwirqs
by using the value of hwirq_base passed in.

> Punishing legacy users with leading gaps in their revmap is likewise
> undesirable, especially as that most of the in-tree users (regmap-irq
> especially) of this functionality are using this legacy behaviour without
> incident.

Hardly punishment.  It is a different optimization decision.  The vast
majority of _add_legacy calls use 0 for hwirq_base, and the ones that
don't use a very small number.

...ummm are we talking about the same things?  You mention regmap-irq
specifically, but regmap irq also uses 0 for hwirq_start, so there is no
leading gap.

g.


-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.