[PATCH 1/2] genirq: Generic chip: add irq_unmap_generic_chip

[PATCH 1/2] genirq: Generic chip: add irq_unmap_generic_chip

[Sebastian Frias]

Without this patch irq_domain_disassociate() cannot properly release the
interrupt.
Indeed, irq_map_generic_chip() checks a bit on 'gc->installed' but said bit
is never cleared, only set.

Commit 088f40b7b027 ("genirq: Generic chip: Add linear irq domain support")
added irq_map_generic_chip() function and also stated "This lacks a removal
function for now".

This commit provides with an implementation of an unmap function that can
be called by irq_domain_disassociate().

Fixes: 088f40b7b027 ("genirq: Generic chip: Add linear irq domain support")

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---

This is required by loadable modules requesting IRQs.
In our case rmmod will perform free_irq() + irq_dispose_mapping().
Without the unmap call the module cannot request the IRQ after "rmmod"
because it is marked as "installed" by the first successful "insmod".

NOTE: While the proposed unmap() function attempts to undo as much things
as done by the map() function, I did not find a way to undo the following:

a) irq_gc_init_mask_cache(gc, dgc->gc_flags)
b) irq_set_lockdep_class(virq, &irq_nested_lock_class)
c) irq_modify_status(virq, dgc->irq_flags_to_clear, dgc->irq_flags_to_set)

Feel free to comment on that matter.

---
 kernel/irq/generic-chip.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index abd286a..7b464cd 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -411,8 +411,34 @@ int irq_map_generic_chip(struct irq_domain *d, unsigned int virq,
 }
 EXPORT_SYMBOL_GPL(irq_map_generic_chip);
 
+void irq_unmap_generic_chip(struct irq_domain *d, unsigned int virq)
+{
+	struct irq_data *data = irq_domain_get_irq_data(d, virq);
+	struct irq_domain_chip_generic *dgc = d->gc;
+	struct irq_chip_generic *gc;
+	unsigned int hw_irq = data->hwirq;
+	int chip_idx, irq_idx;
+
+	if (!d->gc)
+		return;
+
+	chip_idx = hw_irq / dgc->irqs_per_chip;
+	if (chip_idx >= dgc->num_chips)
+		return;
+	gc = dgc->gc[chip_idx];
+
+	irq_idx = hw_irq % dgc->irqs_per_chip;
+
+	clear_bit(irq_idx, &gc->installed);
+	irq_domain_set_info(d, virq, hw_irq,
+			    &no_irq_chip, NULL, NULL, NULL, NULL);
+
+}
+EXPORT_SYMBOL_GPL(irq_unmap_generic_chip);
+
 struct irq_domain_ops irq_generic_chip_ops = {
 	.map	= irq_map_generic_chip,
+	.unmap  = irq_unmap_generic_chip,
 	.xlate	= irq_domain_xlate_onetwocell,
 };
 EXPORT_SYMBOL_GPL(irq_generic_chip_ops);
-- 
1.7.11.2

Re: [PATCH 1/2] genirq: Generic chip: add irq_unmap_generic_chip

[Sebastian Frias]

Hi,

I don't know if somebody has had the time to look at these patches, but if you
have comments (even if related to removing the "fixes" tag if you don't
consider this a fix) please let me know.

Best regards,

Sebastian

On 08/01/2016 04:27 PM, Sebastian Frias wrote:
> Without this patch irq_domain_disassociate() cannot properly release the
> interrupt.
> Indeed, irq_map_generic_chip() checks a bit on 'gc->installed' but said bit
> is never cleared, only set.
> 
> Commit 088f40b7b027 ("genirq: Generic chip: Add linear irq domain support")
> added irq_map_generic_chip() function and also stated "This lacks a removal
> function for now".
> 
> This commit provides with an implementation of an unmap function that can
> be called by irq_domain_disassociate().
> 
> Fixes: 088f40b7b027 ("genirq: Generic chip: Add linear irq domain support")
> 
> Signed-off-by: Sebastian Frias <sf84@laposte.net>
> ---
> 
> This is required by loadable modules requesting IRQs.
> In our case rmmod will perform free_irq() + irq_dispose_mapping().
> Without the unmap call the module cannot request the IRQ after "rmmod"
> because it is marked as "installed" by the first successful "insmod".
> 
> NOTE: While the proposed unmap() function attempts to undo as much things
> as done by the map() function, I did not find a way to undo the following:
> 
> a) irq_gc_init_mask_cache(gc, dgc->gc_flags)
> b) irq_set_lockdep_class(virq, &irq_nested_lock_class)
> c) irq_modify_status(virq, dgc->irq_flags_to_clear, dgc->irq_flags_to_set)
> 
> Feel free to comment on that matter.
> 
> ---
>  kernel/irq/generic-chip.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
> index abd286a..7b464cd 100644
> --- a/kernel/irq/generic-chip.c
> +++ b/kernel/irq/generic-chip.c
> @@ -411,8 +411,34 @@ int irq_map_generic_chip(struct irq_domain *d, unsigned int virq,
>  }
>  EXPORT_SYMBOL_GPL(irq_map_generic_chip);
>  
> +void irq_unmap_generic_chip(struct irq_domain *d, unsigned int virq)
> +{
> +	struct irq_data *data = irq_domain_get_irq_data(d, virq);
> +	struct irq_domain_chip_generic *dgc = d->gc;
> +	struct irq_chip_generic *gc;
> +	unsigned int hw_irq = data->hwirq;
> +	int chip_idx, irq_idx;
> +
> +	if (!d->gc)
> +		return;
> +
> +	chip_idx = hw_irq / dgc->irqs_per_chip;
> +	if (chip_idx >= dgc->num_chips)
> +		return;
> +	gc = dgc->gc[chip_idx];
> +
> +	irq_idx = hw_irq % dgc->irqs_per_chip;
> +
> +	clear_bit(irq_idx, &gc->installed);
> +	irq_domain_set_info(d, virq, hw_irq,
> +			    &no_irq_chip, NULL, NULL, NULL, NULL);
> +
> +}
> +EXPORT_SYMBOL_GPL(irq_unmap_generic_chip);
> +
>  struct irq_domain_ops irq_generic_chip_ops = {
>  	.map	= irq_map_generic_chip,
> +	.unmap  = irq_unmap_generic_chip,
>  	.xlate	= irq_domain_xlate_onetwocell,
>  };
>  EXPORT_SYMBOL_GPL(irq_generic_chip_ops);
> 

Re: [PATCH 1/2] genirq: Generic chip: add irq_unmap_generic_chip

[Thomas Gleixner]

On Mon, 1 Aug 2016, Sebastian Frias wrote:
> NOTE: While the proposed unmap() function attempts to undo as much things
> as done by the map() function, I did not find a way to undo the following:
> 
> a) irq_gc_init_mask_cache(gc, dgc->gc_flags)

You can't undo that. Because that represents the mask cache of the irq chip
and that is required to be consistent over the life time of the irq
chip.

Unmapping does not make the generic chip and the underlying irqchip go
away.

> b) irq_set_lockdep_class(virq, &irq_nested_lock_class)

No point in undoing that. The irq descriptor is released on unmap.

> c) irq_modify_status(virq, dgc->irq_flags_to_clear, dgc->irq_flags_to_set)

See b)

Thanks,

	tglx

Re: [PATCH 1/2] genirq: Generic chip: add irq_unmap_generic_chip

[Sebastian Frias]

Hi Thomas,

On 09/02/2016 05:12 PM, Thomas Gleixner wrote:
> On Mon, 1 Aug 2016, Sebastian Frias wrote:
>> NOTE: While the proposed unmap() function attempts to undo as much things
>> as done by the map() function, I did not find a way to undo the following:
>>
>> a) irq_gc_init_mask_cache(gc, dgc->gc_flags)
> 
> You can't undo that. Because that represents the mask cache of the irq chip
> and that is required to be consistent over the life time of the irq
> chip.
> 
> Unmapping does not make the generic chip and the underlying irqchip go
> away.
> 
>> b) irq_set_lockdep_class(virq, &irq_nested_lock_class)
> 
> No point in undoing that. The irq descriptor is released on unmap.
> 
>> c) irq_modify_status(virq, dgc->irq_flags_to_clear, dgc->irq_flags_to_set)
> 
> See b)
> 

Thanks for your time, I gather from this that the patch is ok then?

I submitted a few more patches, one of them a follow up of this one, and some
more, here are pointers to them just in case:
   https://lkml.org/lkml/2016/8/1/302
   https://lkml.org/lkml/2016/8/2/118
   https://lkml.org/lkml/2016/8/19/598

Best regards,

Sebastian

[tip:irq/core] genirq/generic_chip: Add irq_unmap callback

[tip-bot for Sebastian Frias]

Commit-ID:  ee26c013cdee0b947e29d6cadfb9ff3341c69ff9
Gitweb:     http://git.kernel.org/tip/ee26c013cdee0b947e29d6cadfb9ff3341c69ff9
Author:     Sebastian Frias <sf84@laposte.net>
AuthorDate: Mon, 1 Aug 2016 16:27:38 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 2 Sep 2016 18:06:49 +0200

genirq/generic_chip: Add irq_unmap callback

Without this patch irq_domain_disassociate() cannot properly release the
interrupt. In fact, irq_map_generic_chip() checks a bit on 'gc->installed'
but said bit is never cleared, only set.

Commit 088f40b7b027 ("genirq: Generic chip: Add linear irq domain support")
added irq_map_generic_chip() function and also stated "This lacks a removal
function for now".

This commit provides an implementation of an unmap function that can be
called by irq_domain_disassociate().

[ tglx: Made the function static and removed the export as we have neither
  	a prototype nor a modular user. ]

Fixes: 088f40b7b027 ("genirq: Generic chip: Add linear irq domain support")
Signed-off-by: Sebastian Frias <sf84@laposte.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Mason <slash.tmp@free.fr>
Cc: Jason Cooper <jason@lakedaemon.net>
Link: http://lkml.kernel.org/r/579F5C5A.2070507@laposte.net
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/irq/generic-chip.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 11ad73b..a3a3920 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -414,8 +414,29 @@ int irq_map_generic_chip(struct irq_domain *d, unsigned int virq,
 	return 0;
 }
 
+static void irq_unmap_generic_chip(struct irq_domain *d, unsigned int virq)
+{
+	struct irq_data *data = irq_domain_get_irq_data(d, virq);
+	struct irq_domain_chip_generic *dgc = d->gc;
+	unsigned int hw_irq = data->hwirq;
+	struct irq_chip_generic *gc;
+	int irq_idx;
+
+	gc = irq_get_domain_generic_chip(d, hw_irq);
+	if (!gc)
+		return;
+
+	irq_idx = hw_irq % dgc->irqs_per_chip;
+
+	clear_bit(irq_idx, &gc->installed);
+	irq_domain_set_info(d, virq, hw_irq, &no_irq_chip, NULL, NULL, NULL,
+			    NULL);
+
+}
+
 struct irq_domain_ops irq_generic_chip_ops = {
 	.map	= irq_map_generic_chip,
+	.unmap  = irq_unmap_generic_chip,
 	.xlate	= irq_domain_xlate_onetwocell,
 };
 EXPORT_SYMBOL_GPL(irq_generic_chip_ops);