[PATCH v2 0/3] genirq: Saving/restoring the irqchip state of an irq line

[PATCH v2 0/3] genirq: Saving/restoring the irqchip state of an irq line

[Marc Zyngier]

Despite Linux offering a rather fine grained control over the life
cycle of an interrupt, there is a few cases where it would be very
useful to snapshot (or even set) the internal state of the interrupt
controller for a given interrupt line:

- With KVM, a device shared between VMs must have its whole context
  switched, and that includes the interrupt line state. KVM/arm is
  moving to using this.
- Some GPIO controllers seem to require peeking into the interrupt
  controller they are connected to to report their internal state.

Instead of letting people facing this situation doing horrible
(controller specific) hacks in their code, let's offer a couple of new
entry points that allow a few attributes to be read and set.

Of course, this is a very dangerous thing to do if you don't know what
you doing, and I wouldn't expect most drivers to use this. But this
can also be a life saver at times.

This patch series implement said API, and adds support for this to the
two main ARM interrupt controllers (GIC and GICv3). Available at:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/irqchip_state

>From v1 [1]:
- Rebased on top of 3.18-rc7 + tip/irq/irqdomain-arm
- Now deals with stacked domains
- Changed the API to be less ambiguous (after review by tglx)
- Added IRQCHIP_STATE_LINE_LEVEL (as requested by Bjorn Anderson)

[1] https://lkml.org/lkml/2014/10/25/134

Marc Zyngier (3):
  genirq: Allow the irqchip state of an IRQ to be save/restored
  irqchip: GIC: Add support for irq_{get,set}_irqchip_state
  irqchip: GICv3: Add support for irq_{get,set}_irqchip_state

 drivers/irqchip/irq-gic-v3.c | 83 +++++++++++++++++++++++++++++++++-------
 drivers/irqchip/irq-gic.c    | 69 ++++++++++++++++++++++++++++++---
 include/linux/interrupt.h    | 14 +++++++
 include/linux/irq.h          |  6 +++
 kernel/irq/manage.c          | 91 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 245 insertions(+), 18 deletions(-)

-- 
2.1.3

[PATCH v2 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Marc Zyngier]

There is a number of cases where a kernel subsystem may want to
introspect the state of an interrupt at the irqchip level:

- When a peripheral is shared between virtual machines,
  its interrupt state becomes part of the guest's state,
  and must be switched accordingly. KVM on arm/arm64 requires
  this for its guest-visible timer
- Some GPIO controllers seem to require peeking into the
  interrupt controller they are connected to to report
  their internal state

This seem to be a pattern that is common enough for the core code
to try and support this without too many horrible hacks. Introduce
a pair of accessors (irq_get_irqchip_state/irq_set_irqchip_state)
to retrieve the bits that can be of interest to another subsystem:
pending, active, and masked.

- irq_get_irqchip_state returns the state of the interrupt according
  to a parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
  IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
- irq_set_irqchip_state similarly sets the state of the interrupt.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/interrupt.h | 14 ++++++++
 include/linux/irq.h       |  6 ++++
 kernel/irq/manage.c       | 91 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 69517a2..cbbe6a2 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -356,6 +356,20 @@ static inline int disable_irq_wake(unsigned int irq)
 	return irq_set_irq_wake(irq, 0);
 }
 
+/*
+ * irq_get_irqchip_state/irq_set_irqchip_state specific flags
+ */
+enum irqchip_irq_state {
+	IRQCHIP_STATE_PENDING,		/* Is interrupt pending? */
+	IRQCHIP_STATE_ACTIVE,		/* Is interrupt in progress? */
+	IRQCHIP_STATE_MASKED,		/* Is interrupt masked? */
+	IRQCHIP_STATE_LINE_LEVEL,	/* Is IRQ line high? */
+};
+
+extern int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+				 bool *state);
+extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+				 bool state);
 
 #ifdef CONFIG_IRQ_FORCED_THREADING
 extern bool force_irqthreads;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 8badf34..1922539 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -29,6 +29,7 @@
 struct seq_file;
 struct module;
 struct msi_msg;
+enum irqchip_irq_state;
 
 /*
  * IRQ line status.
@@ -323,6 +324,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  *				irq_request_resources
  * @irq_compose_msi_msg:	optional to compose message content for MSI
  * @irq_write_msi_msg:	optional to write message content for MSI
+ * @irq_get_irqchip_state:	return the internal state of an interrupt
+ * @irq_set_irqchip_state:	set the internal state of a interrupt
  * @flags:		chip specific flags
  */
 struct irq_chip {
@@ -362,6 +365,9 @@ struct irq_chip {
 	void		(*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg);
 	void		(*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg);
 
+	int		(*irq_get_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool *state);
+	int		(*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state);
+
 	unsigned long	flags;
 };
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 8069237..06840ba 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1758,3 +1758,94 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 
 	return retval;
 }
+
+/**
+ *	irq_get_irqchip_state - returns the irqchip state of a interrupt.
+ *	@irq: Interrupt line that is forwarded to a VM
+ *	@which: One of IRQCHIP_STATE_* the caller wants to know about
+ *	@state: a pointer to a boolean where the state is to be storeed
+ *
+ *	This call snapshots the internal irqchip state of an
+ *	interrupt, returning into @state the bit corresponding to
+ *	stage @which
+ *
+ *	This function should be called with preemption disabled if the
+ *	interrupt controller has per-cpu registers.
+ */
+int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+			  bool *state)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	struct irq_chip *chip;
+	unsigned long flags;
+	int err = -EINVAL;
+
+	desc = irq_get_desc_buslock(irq, &flags, 0);
+	if (!desc)
+		return err;
+
+	data = irq_desc_get_irq_data(desc);
+
+	do {
+		chip = irq_data_get_irq_chip(data);
+		if (chip->irq_get_irqchip_state)
+			break;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		data = data->parent_data;
+#else
+		data = NULL;
+#endif
+	} while (data);
+
+	if (data)
+		err = chip->irq_get_irqchip_state(data, which, state);
+
+	irq_put_desc_busunlock(desc, flags);
+	return err;
+}
+
+/**
+ *	irq_set_irqchip_state - set the state of a forwarded interrupt.
+ *	@irq: Interrupt line that is forwarded to a VM
+ *	@which: State to be restored (one of IRQCHIP_STATE_*)
+ *	@val: Value corresponding to @which
+ *
+ *	This call sets the internal irqchip state of an interrupt,
+ *	depending on the value of @which.
+ *
+ *	This function should be called with preemption disabled if the
+ *	interrupt controller has per-cpu registers.
+ */
+int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
+			  bool val)
+{
+	struct irq_desc *desc;
+	struct irq_data *data;
+	struct irq_chip *chip;
+	unsigned long flags;
+	int err = -EINVAL;
+
+	desc = irq_get_desc_buslock(irq, &flags, 0);
+	if (!desc)
+		return err;
+
+	data = irq_desc_get_irq_data(desc);
+
+	do {
+		chip = irq_data_get_irq_chip(data);
+		if (chip->irq_set_irqchip_state)
+			break;
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+		data = data->parent_data;
+#else
+		data = NULL;
+#endif
+	} while (data);
+
+	if (data)
+		chip->irq_set_irqchip_state(data, which, val);
+
+	irq_put_desc_busunlock(desc, flags);
+	return err;
+}
-- 
2.1.3

[PATCH v2 2/3] irqchip: GIC: Add support for irq_{get,set}_irqchip_state

[Marc Zyngier]

Add the required hooks for the internal state of an interrupt
to be exposed to other subsystems.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 69 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 5a71be7..543cbc9 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -151,12 +151,22 @@ static inline unsigned int gic_irq(struct irq_data *d)
 /*
  * Routines to acknowledge, disable and enable interrupts
  */
-static void gic_mask_irq(struct irq_data *d)
+static void gic_poke_irq(struct irq_data *d, u32 offset)
+{
+	u32 mask = 1 << (gic_irq(d) % 32);
+	writel_relaxed(mask, gic_dist_base(d) + offset + (gic_irq(d) / 32) * 4);
+}
+
+static int gic_peek_irq(struct irq_data *d, u32 offset)
 {
 	u32 mask = 1 << (gic_irq(d) % 32);
+	return !!(readl_relaxed(gic_dist_base(d) + offset + (gic_irq(d) / 32) * 4) & mask);
+}
 
+static void gic_mask_irq(struct irq_data *d)
+{
 	raw_spin_lock(&irq_controller_lock);
-	writel_relaxed(mask, gic_dist_base(d) + GIC_DIST_ENABLE_CLEAR + (gic_irq(d) / 32) * 4);
+	gic_poke_irq(d, GIC_DIST_ENABLE_CLEAR);
 	if (gic_arch_extn.irq_mask)
 		gic_arch_extn.irq_mask(d);
 	raw_spin_unlock(&irq_controller_lock);
@@ -164,12 +174,10 @@ static void gic_mask_irq(struct irq_data *d)
 
 static void gic_unmask_irq(struct irq_data *d)
 {
-	u32 mask = 1 << (gic_irq(d) % 32);
-
 	raw_spin_lock(&irq_controller_lock);
 	if (gic_arch_extn.irq_unmask)
 		gic_arch_extn.irq_unmask(d);
-	writel_relaxed(mask, gic_dist_base(d) + GIC_DIST_ENABLE_SET + (gic_irq(d) / 32) * 4);
+	gic_poke_irq(d, GIC_DIST_ENABLE_SET);
 	raw_spin_unlock(&irq_controller_lock);
 }
 
@@ -184,6 +192,55 @@ static void gic_eoi_irq(struct irq_data *d)
 	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_EOI);
 }
 
+static int gic_irq_set_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool val)
+{
+	u32 reg;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		reg = val ? GIC_DIST_PENDING_SET : GIC_DIST_PENDING_CLEAR;
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		reg = val ? GIC_DIST_ACTIVE_SET : GIC_DIST_ACTIVE_CLEAR;
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		reg = val ? GIC_DIST_ENABLE_CLEAR : GIC_DIST_ENABLE_SET;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	gic_poke_irq(d, reg);
+	return 0;
+}
+
+static int gic_irq_get_irqchip_state(struct irq_data *d,
+				      enum irqchip_irq_state which, bool *val)
+{
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		*val = gic_peek_irq(d, GIC_DIST_PENDING_SET);
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		*val = gic_peek_irq(d, GIC_DIST_ACTIVE_SET);
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		*val = !gic_peek_irq(d, GIC_DIST_ENABLE_SET);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int gic_set_type(struct irq_data *d, unsigned int type)
 {
 	void __iomem *base = gic_dist_base(d);
@@ -322,6 +379,8 @@ static struct irq_chip gic_chip = {
 	.irq_set_affinity	= gic_set_affinity,
 #endif
 	.irq_set_wake		= gic_set_wake,
+	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
+	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 };
 
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
-- 
2.1.3

[PATCH v2 3/3] irqchip: GICv3: Add support for irq_{get,set}_irqchip_state

[Marc Zyngier]

Add the required hooks for the internal state of an interrupt
to be exposed to other subsystems.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic-v3.c | 83 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 70 insertions(+), 13 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 1a146cc..6e73846 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -195,6 +195,19 @@ static void gic_enable_redist(bool enable)
 /*
  * Routines to disable, enable, EOI and route interrupts
  */
+static int gic_peek_irq(struct irq_data *d, u32 offset)
+{
+	u32 mask = 1 << (gic_irq(d) % 32);
+	void __iomem *base;
+
+	if (gic_irq_in_rdist(d))
+		base = gic_data_rdist_sgi_base();
+	else
+		base = gic_data.dist_base;
+
+	return !!(readl_relaxed(base + offset + (gic_irq(d) / 32) * 4) & mask);
+}
+
 static void gic_poke_irq(struct irq_data *d, u32 offset)
 {
 	u32 mask = 1 << (gic_irq(d) % 32);
@@ -223,6 +236,61 @@ static void gic_unmask_irq(struct irq_data *d)
 	gic_poke_irq(d, GICD_ISENABLER);
 }
 
+static int gic_irq_set_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool val)
+{
+	u32 reg;
+
+	if (d->hwirq >= gic_data.irq_nr) /* PPI/SPI only */
+		return -EINVAL;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		reg = val ? GICD_ISPENDR : GICD_ICPENDR;
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		reg = val ? GICD_ISACTIVER : GICD_ICACTIVER;
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		reg = val ? GICD_ICENABLER : GICD_ISENABLER;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	gic_poke_irq(d, reg);
+	return 0;
+}
+
+static int gic_irq_get_irqchip_state(struct irq_data *d,
+				     enum irqchip_irq_state which, bool *val)
+{
+	if (d->hwirq >= gic_data.irq_nr) /* PPI/SPI only */
+		return -EINVAL;
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		*val = gic_peek_irq(d, GICD_ISPENDR);
+		break;
+
+	case IRQCHIP_STATE_ACTIVE:
+		*val = gic_peek_irq(d, GICD_ISACTIVER);
+		break;
+
+	case IRQCHIP_STATE_MASKED:
+		*val = !gic_peek_irq(d, GICD_ISENABLER);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void gic_eoi_irq(struct irq_data *d)
 {
 	gic_write_eoir(gic_irq(d));
@@ -418,19 +486,6 @@ static void gic_cpu_init(void)
 }
 
 #ifdef CONFIG_SMP
-static int gic_peek_irq(struct irq_data *d, u32 offset)
-{
-	u32 mask = 1 << (gic_irq(d) % 32);
-	void __iomem *base;
-
-	if (gic_irq_in_rdist(d))
-		base = gic_data_rdist_sgi_base();
-	else
-		base = gic_data.dist_base;
-
-	return !!(readl_relaxed(base + offset + (gic_irq(d) / 32) * 4) & mask);
-}
-
 static int gic_secondary_init(struct notifier_block *nfb,
 			      unsigned long action, void *hcpu)
 {
@@ -597,6 +652,8 @@ static struct irq_chip gic_chip = {
 	.irq_eoi		= gic_eoi_irq,
 	.irq_set_type		= gic_set_type,
 	.irq_set_affinity	= gic_set_affinity,
+	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
+	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
 };
 
 #define GIC_ID_NR		(1U << gic_data.rdists.id_bits)
-- 
2.1.3

Re: [PATCH v2 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Bjorn Andersson]

On Wed, Dec 3, 2014 at 8:18 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> There is a number of cases where a kernel subsystem may want to
> introspect the state of an interrupt at the irqchip level:
>
> - When a peripheral is shared between virtual machines,
>   its interrupt state becomes part of the guest's state,
>   and must be switched accordingly. KVM on arm/arm64 requires
>   this for its guest-visible timer
> - Some GPIO controllers seem to require peeking into the
>   interrupt controller they are connected to to report
>   their internal state
>
> This seem to be a pattern that is common enough for the core code
> to try and support this without too many horrible hacks. Introduce
> a pair of accessors (irq_get_irqchip_state/irq_set_irqchip_state)
> to retrieve the bits that can be of interest to another subsystem:
> pending, active, and masked.
>
> - irq_get_irqchip_state returns the state of the interrupt according
>   to a parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
>   IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
> - irq_set_irqchip_state similarly sets the state of the interrupt.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

With the addition of actually assigning err to something useful in the
setter below:

Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c

[..]

> +/**
> + *     irq_set_irqchip_state - set the state of a forwarded interrupt.
> + *     @irq: Interrupt line that is forwarded to a VM
> + *     @which: State to be restored (one of IRQCHIP_STATE_*)
> + *     @val: Value corresponding to @which
> + *
> + *     This call sets the internal irqchip state of an interrupt,
> + *     depending on the value of @which.
> + *
> + *     This function should be called with preemption disabled if the
> + *     interrupt controller has per-cpu registers.
> + */
> +int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
> +                         bool val)
> +{
> +       struct irq_desc *desc;
> +       struct irq_data *data;
> +       struct irq_chip *chip;
> +       unsigned long flags;
> +       int err = -EINVAL;
> +
> +       desc = irq_get_desc_buslock(irq, &flags, 0);
> +       if (!desc)
> +               return err;
> +
> +       data = irq_desc_get_irq_data(desc);
> +
> +       do {
> +               chip = irq_data_get_irq_chip(data);
> +               if (chip->irq_set_irqchip_state)
> +                       break;
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +               data = data->parent_data;
> +#else
> +               data = NULL;
> +#endif
> +       } while (data);
> +
> +       if (data)
> +               chip->irq_set_irqchip_state(data, which, val);

err =

> +
> +       irq_put_desc_busunlock(desc, flags);
> +       return err;
> +}

Regards,
Bjorn

Re: [PATCH v2 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Marc Zyngier]

On 08/12/14 22:42, Bjorn Andersson wrote:
> On Wed, Dec 3, 2014 at 8:18 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> There is a number of cases where a kernel subsystem may want to
>> introspect the state of an interrupt at the irqchip level:
>>
>> - When a peripheral is shared between virtual machines,
>>   its interrupt state becomes part of the guest's state,
>>   and must be switched accordingly. KVM on arm/arm64 requires
>>   this for its guest-visible timer
>> - Some GPIO controllers seem to require peeking into the
>>   interrupt controller they are connected to to report
>>   their internal state
>>
>> This seem to be a pattern that is common enough for the core code
>> to try and support this without too many horrible hacks. Introduce
>> a pair of accessors (irq_get_irqchip_state/irq_set_irqchip_state)
>> to retrieve the bits that can be of interest to another subsystem:
>> pending, active, and masked.
>>
>> - irq_get_irqchip_state returns the state of the interrupt according
>>   to a parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
>>   IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
>> - irq_set_irqchip_state similarly sets the state of the interrupt.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> With the addition of actually assigning err to something useful in the
> setter below:
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> 
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> 
> [..]
> 
>> +/**
>> + *     irq_set_irqchip_state - set the state of a forwarded interrupt.
>> + *     @irq: Interrupt line that is forwarded to a VM
>> + *     @which: State to be restored (one of IRQCHIP_STATE_*)
>> + *     @val: Value corresponding to @which
>> + *
>> + *     This call sets the internal irqchip state of an interrupt,
>> + *     depending on the value of @which.
>> + *
>> + *     This function should be called with preemption disabled if the
>> + *     interrupt controller has per-cpu registers.
>> + */
>> +int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
>> +                         bool val)
>> +{
>> +       struct irq_desc *desc;
>> +       struct irq_data *data;
>> +       struct irq_chip *chip;
>> +       unsigned long flags;
>> +       int err = -EINVAL;
>> +
>> +       desc = irq_get_desc_buslock(irq, &flags, 0);
>> +       if (!desc)
>> +               return err;
>> +
>> +       data = irq_desc_get_irq_data(desc);
>> +
>> +       do {
>> +               chip = irq_data_get_irq_chip(data);
>> +               if (chip->irq_set_irqchip_state)
>> +                       break;
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +               data = data->parent_data;
>> +#else
>> +               data = NULL;
>> +#endif
>> +       } while (data);
>> +
>> +       if (data)
>> +               chip->irq_set_irqchip_state(data, which, val);
> 
> err =

Ah, that will teach me to write test code that actually checks the
return value! :-)

Thanks for the fix and the tags.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Jason Cooper]

Marc,

On Tue, Dec 09, 2014 at 09:37:26AM +0000, Marc Zyngier wrote:
> On 08/12/14 22:42, Bjorn Andersson wrote:
> > On Wed, Dec 3, 2014 at 8:18 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> There is a number of cases where a kernel subsystem may want to
> >> introspect the state of an interrupt at the irqchip level:
> >>
> >> - When a peripheral is shared between virtual machines,
> >>   its interrupt state becomes part of the guest's state,
> >>   and must be switched accordingly. KVM on arm/arm64 requires
> >>   this for its guest-visible timer
> >> - Some GPIO controllers seem to require peeking into the
> >>   interrupt controller they are connected to to report
> >>   their internal state
> >>
> >> This seem to be a pattern that is common enough for the core code
> >> to try and support this without too many horrible hacks. Introduce
> >> a pair of accessors (irq_get_irqchip_state/irq_set_irqchip_state)
> >> to retrieve the bits that can be of interest to another subsystem:
> >> pending, active, and masked.
> >>
> >> - irq_get_irqchip_state returns the state of the interrupt according
> >>   to a parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
> >>   IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
> >> - irq_set_irqchip_state similarly sets the state of the interrupt.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > 
> > With the addition of actually assigning err to something useful in the
> > setter below:
> > 
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > 
> >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > 
> > [..]
> > 
> >> +/**
> >> + *     irq_set_irqchip_state - set the state of a forwarded interrupt.
> >> + *     @irq: Interrupt line that is forwarded to a VM
> >> + *     @which: State to be restored (one of IRQCHIP_STATE_*)
> >> + *     @val: Value corresponding to @which
> >> + *
> >> + *     This call sets the internal irqchip state of an interrupt,
> >> + *     depending on the value of @which.
> >> + *
> >> + *     This function should be called with preemption disabled if the
> >> + *     interrupt controller has per-cpu registers.
> >> + */
> >> +int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
> >> +                         bool val)
> >> +{
> >> +       struct irq_desc *desc;
> >> +       struct irq_data *data;
> >> +       struct irq_chip *chip;
> >> +       unsigned long flags;
> >> +       int err = -EINVAL;
> >> +
> >> +       desc = irq_get_desc_buslock(irq, &flags, 0);
> >> +       if (!desc)
> >> +               return err;
> >> +
> >> +       data = irq_desc_get_irq_data(desc);
> >> +
> >> +       do {
> >> +               chip = irq_data_get_irq_chip(data);
> >> +               if (chip->irq_set_irqchip_state)
> >> +                       break;
> >> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> >> +               data = data->parent_data;
> >> +#else
> >> +               data = NULL;
> >> +#endif
> >> +       } while (data);
> >> +
> >> +       if (data)
> >> +               chip->irq_set_irqchip_state(data, which, val);
> > 
> > err =
> 
> Ah, that will teach me to write test code that actually checks the
> return value! :-)
> 
> Thanks for the fix and the tags.

Did I miss the new version of this?

thx,

Jason.

Re: [PATCH v2 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Marc Zyngier]

On 07/01/15 16:05, Jason Cooper wrote:
> Marc,
> 
> On Tue, Dec 09, 2014 at 09:37:26AM +0000, Marc Zyngier wrote:
>> On 08/12/14 22:42, Bjorn Andersson wrote:
>>> On Wed, Dec 3, 2014 at 8:18 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> There is a number of cases where a kernel subsystem may want to
>>>> introspect the state of an interrupt at the irqchip level:
>>>>
>>>> - When a peripheral is shared between virtual machines,
>>>>   its interrupt state becomes part of the guest's state,
>>>>   and must be switched accordingly. KVM on arm/arm64 requires
>>>>   this for its guest-visible timer
>>>> - Some GPIO controllers seem to require peeking into the
>>>>   interrupt controller they are connected to to report
>>>>   their internal state
>>>>
>>>> This seem to be a pattern that is common enough for the core code
>>>> to try and support this without too many horrible hacks. Introduce
>>>> a pair of accessors (irq_get_irqchip_state/irq_set_irqchip_state)
>>>> to retrieve the bits that can be of interest to another subsystem:
>>>> pending, active, and masked.
>>>>
>>>> - irq_get_irqchip_state returns the state of the interrupt according
>>>>   to a parameter set to IRQCHIP_STATE_PENDING, IRQCHIP_STATE_ACTIVE,
>>>>   IRQCHIP_STATE_MASKED or IRQCHIP_STATE_LINE_LEVEL.
>>>> - irq_set_irqchip_state similarly sets the state of the interrupt.
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>
>>> With the addition of actually assigning err to something useful in the
>>> setter below:
>>>
>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>>
>>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>>>
>>> [..]
>>>
>>>> +/**
>>>> + *     irq_set_irqchip_state - set the state of a forwarded interrupt.
>>>> + *     @irq: Interrupt line that is forwarded to a VM
>>>> + *     @which: State to be restored (one of IRQCHIP_STATE_*)
>>>> + *     @val: Value corresponding to @which
>>>> + *
>>>> + *     This call sets the internal irqchip state of an interrupt,
>>>> + *     depending on the value of @which.
>>>> + *
>>>> + *     This function should be called with preemption disabled if the
>>>> + *     interrupt controller has per-cpu registers.
>>>> + */
>>>> +int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
>>>> +                         bool val)
>>>> +{
>>>> +       struct irq_desc *desc;
>>>> +       struct irq_data *data;
>>>> +       struct irq_chip *chip;
>>>> +       unsigned long flags;
>>>> +       int err = -EINVAL;
>>>> +
>>>> +       desc = irq_get_desc_buslock(irq, &flags, 0);
>>>> +       if (!desc)
>>>> +               return err;
>>>> +
>>>> +       data = irq_desc_get_irq_data(desc);
>>>> +
>>>> +       do {
>>>> +               chip = irq_data_get_irq_chip(data);
>>>> +               if (chip->irq_set_irqchip_state)
>>>> +                       break;
>>>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>>>> +               data = data->parent_data;
>>>> +#else
>>>> +               data = NULL;
>>>> +#endif
>>>> +       } while (data);
>>>> +
>>>> +       if (data)
>>>> +               chip->irq_set_irqchip_state(data, which, val);
>>>
>>> err =
>>
>> Ah, that will teach me to write test code that actually checks the
>> return value! :-)
>>
>> Thanks for the fix and the tags.
> 
> Did I miss the new version of this?

I'm rebasing all my stuff at the moment. Expect something later today
(or tomorrow morning worse case).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/3] genirq: Allow the irqchip state of an IRQ to be save/restored

[Jason Cooper]

On Wed, Jan 07, 2015 at 04:09:54PM +0000, Marc Zyngier wrote:
> On 07/01/15 16:05, Jason Cooper wrote:
...
> > Did I miss the new version of this?
> 
> I'm rebasing all my stuff at the moment. Expect something later today
> (or tomorrow morning worse case).

Ok, wasn't trying to rush.  I had a few days of mail anomalies, so I was
just double checking that it didn't go missing.

thx,

Jason.