Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1394518 > unrolled thread

[PATCH V3 00/17] Add support for Tegra210 AGIC

Back to article view | Back to linux.kernel

Contents

  [PATCH V3 00/17] Add support for Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
    [PATCH V3 11/17] irqchip/gic: Return an error if GIC initialisation fails Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
    [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
      Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2016-05-09 14:30 +0200
        Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-05-09 15:20 +0200
          Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2016-05-09 17:20 +0200
            Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-05-09 17:50 +0200
              Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ Marc Zyngier <marc.zyngier@arm.com> - 2016-05-10 14:30 +0200
    [PATCH V3 15/17] irqchip/gic: Split GIC init in preparation for platform driver Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
    [PATCH V3 14/17] irqchip/gic: Add helper function for configuring a GIC via device-tree Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
    [PATCH V3 10/17] irqchip/gic: Remove static irq_chip definition for eoimode1 Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:30 +0200
    [PATCH V3 07/17] genirq: Ensure IRQ descriptor is valid when setting-up the IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:40 +0200
    [PATCH V3 04/17] irqdomain: Fix handling of type settings for existing mappings Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:40 +0200
    [PATCH V3 09/17] irqchip/gic: Don't initialise chip if mapping IO space fails Jon Hunter <jonathanh@nvidia.com> - 2016-05-04 18:40 +0200

#1394518 — [PATCH V3 00/17] Add support for Tegra210 AGIC

The Tegra210 has a 2nd level interrupt controller located in a separate
power domain to the main GIC interrupt controller and hence requires
runtime-pm support.

Add a platform driver for the GICs that require runtime-pm and make the
necessary changes to the genirq and irqdomain core to support IRQ chips
that require runtime-pm.

Please note that although as the subject states this adds support for
the Tegra210 AGIC, in this current version there is really nothing
specific in this series for Tegra. However, rather than changing the
subject I opted to keep it the same so people recognise this is a
continuation of that work.

Changes since V2:
- Corrected RPM support for irqchips in genirq core as pointed out by
  Kevin Hilman.
- Corrected patch to save the irq type when mapping the interrupt.
- Dropped changes to DT binding documentation and added patch to prevent
  early init of GICs if the 'clocks' and/or 'power-domains' DT
  properties are present (as we have discussed).
- Moved platform driver code into it's own source file.
- Separate changes for preparing for the platform driver into 3 patches
  in an attempt to make it more readable!
- Added fix for checking an interrupt descriptor is valid during IRQ
  setup.

Changes since V1:
- Updated GIC to only WARN and not return an error if configuring a PPI
  fails but will still return an error if an SPI fails (per discussion
  with Marc).
- Dropped change to mask sense bits for GIC-v3 (as this is not
  necessary)
- Split patch to avoid setting interrupt type when mapping the IRQ into
  two patches per TGLX's feedback.
- Changed name of irqchip device structure to "parent_device"
- Moved call to irq_chip_pm_get() outside of chip_bus_lock().
- Dropped patch to remove clock names from GIC DT documentation and
  added AGIC clock names.
- Update GIC platform driver to look-up clocks names from static list.

Jon Hunter (17):
  irqchip/gic: Don't unnecessarily write the IRQ configuration
  irqchip/gic: WARN if setting the interrupt type for a PPI fails
  irqchip: Mask the non-type/sense bits when translating an IRQ
  irqdomain: Fix handling of type settings for existing mappings
  genirq: Look-up trigger type if not specified by caller
  irqdomain: Don't set type when mapping an IRQ
  genirq: Ensure IRQ descriptor is valid when setting-up the IRQ
  genirq: Add runtime power management support for IRQ chips
  irqchip/gic: Don't initialise chip if mapping IO space fails
  irqchip/gic: Remove static irq_chip definition for eoimode1
  irqchip/gic: Return an error if GIC initialisation fails
  irqchip/gic: Pass GIC pointer to save/restore functions
  irqchip/gic: Don't allow early initialisation if GIC requires RPM
  irqchip/gic: Add helper function for configuring a GIC via device-tree
  irqchip/gic: Split GIC init in preparation for platform driver
  irqchip/gic: Prepare for adding platform driver
  irqchip/gic: Add platform driver for non-root GICs that require RPM

 drivers/irqchip/Kconfig          |   6 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-crossbar.c   |   2 +-
 drivers/irqchip/irq-gic-common.c |  19 ++-
 drivers/irqchip/irq-gic-pm.c     | 241 ++++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-gic.c        | 277 +++++++++++++++++++++++----------------
 drivers/irqchip/irq-gic.h        |  63 +++++++++
 drivers/irqchip/irq-tegra.c      |   2 +-
 include/linux/irq.h              |   4 +
 include/linux/irqdomain.h        |   3 +
 kernel/irq/chip.c                |  35 +++++
 kernel/irq/internals.h           |   1 +
 kernel/irq/irqdomain.c           |  58 ++++++--
 kernel/irq/manage.c              |  40 +++++-
 14 files changed, 617 insertions(+), 135 deletions(-)
 create mode 100644 drivers/irqchip/irq-gic-pm.c
 create mode 100644 drivers/irqchip/irq-gic.h

-- 
2.1.4

[toc] | [next] | [standalone]

#1394519 — [PATCH V3 11/17] irqchip/gic: Return an error if GIC initialisation fails

If the GIC initialisation fails, then currently we do not return an error
or clean-up afterwards. Although for root controllers, this failure may be
fatal anyway, for secondary controllers, it may not be fatal and so return
an error on failure and clean-up.

For non-banked GIC controllers, make sure that we free any memory
allocated if we fail to initialise the IRQ domain. Please note that
free_percpu() only frees memory if the pointer passed to it is not NULL
and so it is unnecessary to check if both pointers are valid or not.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 59 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 13a0d77e8ae9..82931aa68868 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -998,13 +998,13 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.unmap = gic_irq_domain_unmap,
 };
 
-static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
+static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 			   void __iomem *dist_base, void __iomem *cpu_base,
 			   u32 percpu_offset, struct fwnode_handle *handle)
 {
 	irq_hw_number_t hwirq_base;
 	struct gic_chip_data *gic;
-	int gic_irqs, irq_base, i;
+	int gic_irqs, irq_base, i, ret;
 
 	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
 
@@ -1017,7 +1017,7 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic->chip.irq_mask = gic_eoimode1_mask_irq;
 		gic->chip.irq_eoi = gic_eoimode1_eoi_irq;
 		gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
-		gic->chip.name = "GICv2";
+		gic->chip.name = kasprintf(GFP_KERNEL, "GICv2");
 	} else {
 		gic->chip.name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
 	}
@@ -1027,17 +1027,16 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		gic->chip.irq_set_affinity = gic_set_affinity;
 #endif
 
-#ifdef CONFIG_GIC_NON_BANKED
-	if (percpu_offset) { /* Frankein-GIC without banked registers... */
+	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+		/* Frankein-GIC without banked registers... */
 		unsigned int cpu;
 
 		gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
 		gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
 		if (WARN_ON(!gic->dist_base.percpu_base ||
 			    !gic->cpu_base.percpu_base)) {
-			free_percpu(gic->dist_base.percpu_base);
-			free_percpu(gic->cpu_base.percpu_base);
-			return;
+			ret = -ENOMEM;
+			goto error;
 		}
 
 		for_each_possible_cpu(cpu) {
@@ -1049,9 +1048,8 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		}
 
 		gic_set_base_accessor(gic, gic_get_percpu_base);
-	} else
-#endif
-	{			/* Normal, sane GIC... */
+	} else {
+		/* Normal, sane GIC... */
 		WARN(percpu_offset,
 		     "GIC_NON_BANKED not enabled, ignoring %08x offset!",
 		     percpu_offset);
@@ -1101,8 +1099,10 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 					hwirq_base, &gic_irq_domain_ops, gic);
 	}
 
-	if (WARN_ON(!gic->domain))
-		return;
+	if (WARN_ON(!gic->domain)) {
+		ret = -ENODEV;
+		goto error;
+	}
 
 	if (gic_nr == 0) {
 		/*
@@ -1124,6 +1124,18 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	gic_dist_init(gic);
 	gic_cpu_init(gic);
 	gic_pm_init(gic);
+
+	return 0;
+
+error:
+	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+		free_percpu(gic->dist_base.percpu_base);
+		free_percpu(gic->cpu_base.percpu_base);
+	}
+
+	kfree(gic->chip.name);
+
+	return ret;
 }
 
 void __init gic_init(unsigned int gic_nr, int irq_start,
@@ -1184,7 +1196,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	void __iomem *cpu_base;
 	void __iomem *dist_base;
 	u32 percpu_offset;
-	int irq;
+	int irq, ret;
 
 	if (WARN_ON(!node))
 		return -ENODEV;
@@ -1209,8 +1221,14 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;
 
-	__gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
+	ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
 			 &node->fwnode);
+	if (ret) {
+		iounmap(dist_base);
+		iounmap(cpu_base);
+		return ret;
+	}
+
 	if (!gic_cnt)
 		gic_init_physaddr(node);
 
@@ -1299,7 +1317,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 	struct acpi_madt_generic_distributor *dist;
 	void __iomem *cpu_base, *dist_base;
 	struct fwnode_handle *domain_handle;
-	int count;
+	int count, ret;
 
 	/* Collect CPU base addresses */
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
@@ -1342,7 +1360,14 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 		return -ENOMEM;
 	}
 
-	__gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+	ret = __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+	if (ret) {
+		pr_err("Failed to initialise GIC\n");
+		irq_domain_free_fwnode(domain_handle);
+		iounmap(cpu_base);
+		iounmap(dist_base);
+		return ret;
+	}
 
 	acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
 
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1394520 — [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ

Some IRQ chips, such as GPIO controllers or secondary level interrupt
controllers, may require require additional runtime power management
control to ensure they are accessible. For such IRQ chips, it makes sense
to enable the IRQ chip when interrupts are requested and disabled them
again once all interrupts have been freed.

When mapping an IRQ, the IRQ type settings are read and then programmed.
The mapping of the IRQ happens before the IRQ is requested and so the
programming of the type settings occurs before the IRQ is requested. This
is a problem for IRQ chips that require additional power management
control because they may not be accessible yet. Therefore, when mapping
the IRQ, don't program the type settings, just save them and then program
these saved settings when the IRQ is requested (so long as if they are not
overridden via the call to request the IRQ).

Add a stub function for irq_domain_free_irqs() to avoid any compilation
errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 include/linux/irqdomain.h |  3 +++
 kernel/irq/irqdomain.c    | 23 ++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 2aed04396210..fc66876d1965 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
 	return -1;
 }
 
+static inline void irq_domain_free_irqs(unsigned int virq,
+					unsigned int nr_irqs) { }
+
 static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
 {
 	return false;
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index d68371213fc9..bbf5b9b8ac3d 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -564,6 +564,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
 unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 {
 	struct irq_domain *domain;
+	struct irq_data *irq_data;
 	irq_hw_number_t hwirq;
 	unsigned int type = IRQ_TYPE_NONE;
 	int virq;
@@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		 * it now and return the interrupt number.
 		 */
 		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
-			irq_set_irq_type(virq, type);
+			irq_data = irq_get_irq_data(virq);
+			if (!irq_data)
+				return 0;
+
+			irqd_set_trigger_type(irq_data, type);
 			return virq;
 		}
 
@@ -633,10 +638,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 			return virq;
 	}
 
-	/* Set type if specified and different than the current one */
-	if (type != IRQ_TYPE_NONE &&
-	    type != irq_get_trigger_type(virq))
-		irq_set_irq_type(virq, type);
+	irq_data = irq_get_irq_data(virq);
+	if (!irq_data) {
+		if (irq_domain_is_hierarchy(domain))
+			irq_domain_free_irqs(virq, 1);
+		else
+			irq_dispose_mapping(virq);
+		return 0;
+	}
+
+	/* Store trigger type */
+	irqd_set_trigger_type(irq_data, type);
+
 	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1397003 — Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ

On 04/05/16 17:25, Jon Hunter wrote:
> Some IRQ chips, such as GPIO controllers or secondary level interrupt
> controllers, may require require additional runtime power management
> control to ensure they are accessible. For such IRQ chips, it makes sense
> to enable the IRQ chip when interrupts are requested and disabled them
> again once all interrupts have been freed.
> 
> When mapping an IRQ, the IRQ type settings are read and then programmed.
> The mapping of the IRQ happens before the IRQ is requested and so the
> programming of the type settings occurs before the IRQ is requested. This
> is a problem for IRQ chips that require additional power management
> control because they may not be accessible yet. Therefore, when mapping
> the IRQ, don't program the type settings, just save them and then program
> these saved settings when the IRQ is requested (so long as if they are not
> overridden via the call to request the IRQ).
> 
> Add a stub function for irq_domain_free_irqs() to avoid any compilation
> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  include/linux/irqdomain.h |  3 +++
>  kernel/irq/irqdomain.c    | 23 ++++++++++++++++++-----
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 2aed04396210..fc66876d1965 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
>  	return -1;
>  }
>  
> +static inline void irq_domain_free_irqs(unsigned int virq,
> +					unsigned int nr_irqs) { }
> +
>  static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>  {
>  	return false;
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index d68371213fc9..bbf5b9b8ac3d 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -564,6 +564,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  {
>  	struct irq_domain *domain;
> +	struct irq_data *irq_data;
>  	irq_hw_number_t hwirq;
>  	unsigned int type = IRQ_TYPE_NONE;
>  	int virq;
> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  		 * it now and return the interrupt number.
>  		 */
>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
> -			irq_set_irq_type(virq, type);
> +			irq_data = irq_get_irq_data(virq);
> +			if (!irq_data)
> +				return 0;
> +
> +			irqd_set_trigger_type(irq_data, type);
>  			return virq;
>  		}
>  
> @@ -633,10 +638,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  			return virq;
>  	}
>  
> -	/* Set type if specified and different than the current one */
> -	if (type != IRQ_TYPE_NONE &&
> -	    type != irq_get_trigger_type(virq))
> -		irq_set_irq_type(virq, type);
> +	irq_data = irq_get_irq_data(virq);
> +	if (!irq_data) {
> +		if (irq_domain_is_hierarchy(domain))
> +			irq_domain_free_irqs(virq, 1);
> +		else
> +			irq_dispose_mapping(virq);
> +		return 0;
> +	}
> +
> +	/* Store trigger type */
> +	irqd_set_trigger_type(irq_data, type);
> +
>  	return virq;
>  }
>  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> 

This patch have the effect of making misconfigured PPIs absolutely
obvious. I still need to wrap my head around the root cause, but here's
the findings I have so far:

- kvmtool generates a DT with the wrong trigger information (edge
instead of level) for the timer.
- with this patch applied, "cyclictest -S" reliably locks up when run in
a guest (missing a timer interrupt, goodbye CPU).
- Either fixing kvmtool or reverting that patch makes it work reliably
again.

My gut feeling is that until that patch, the failing irq_set_irq_type()
wasn't affecting the kernel's view of the trigger (it was still treated
as level). With this patch, the kernel now trusts whatever is coming
from the firmware, and the misconfiguration becomes obvious. And just
grepping through the DT files for arm and arm64 sends makes me thing
"Holly effin' crap!".

I'm not saying that we shouldn't perform this change though. But it is
quite obvious that it is going to break an awful lot of existing code
and platforms. I'm also cooking a small patch for the arch timer (which
seems to be described in DT with a fairly high level of brokenness), so
that we can mop-up most of the brain damage.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1397045 — Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ

On 09/05/16 13:23, Marc Zyngier wrote:
> On 04/05/16 17:25, Jon Hunter wrote:
>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>> controllers, may require require additional runtime power management
>> control to ensure they are accessible. For such IRQ chips, it makes sense
>> to enable the IRQ chip when interrupts are requested and disabled them
>> again once all interrupts have been freed.
>>
>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>> The mapping of the IRQ happens before the IRQ is requested and so the
>> programming of the type settings occurs before the IRQ is requested. This
>> is a problem for IRQ chips that require additional power management
>> control because they may not be accessible yet. Therefore, when mapping
>> the IRQ, don't program the type settings, just save them and then program
>> these saved settings when the IRQ is requested (so long as if they are not
>> overridden via the call to request the IRQ).
>>
>> Add a stub function for irq_domain_free_irqs() to avoid any compilation
>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  include/linux/irqdomain.h |  3 +++
>>  kernel/irq/irqdomain.c    | 23 ++++++++++++++++++-----
>>  2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 2aed04396210..fc66876d1965 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
>>  	return -1;
>>  }
>>  
>> +static inline void irq_domain_free_irqs(unsigned int virq,
>> +					unsigned int nr_irqs) { }
>> +
>>  static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>>  {
>>  	return false;
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index d68371213fc9..bbf5b9b8ac3d 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -564,6 +564,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  {
>>  	struct irq_domain *domain;
>> +	struct irq_data *irq_data;
>>  	irq_hw_number_t hwirq;
>>  	unsigned int type = IRQ_TYPE_NONE;
>>  	int virq;
>> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  		 * it now and return the interrupt number.
>>  		 */
>>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>> -			irq_set_irq_type(virq, type);
>> +			irq_data = irq_get_irq_data(virq);
>> +			if (!irq_data)
>> +				return 0;
>> +
>> +			irqd_set_trigger_type(irq_data, type);
>>  			return virq;
>>  		}
>>  
>> @@ -633,10 +638,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  			return virq;
>>  	}
>>  
>> -	/* Set type if specified and different than the current one */
>> -	if (type != IRQ_TYPE_NONE &&
>> -	    type != irq_get_trigger_type(virq))
>> -		irq_set_irq_type(virq, type);
>> +	irq_data = irq_get_irq_data(virq);
>> +	if (!irq_data) {
>> +		if (irq_domain_is_hierarchy(domain))
>> +			irq_domain_free_irqs(virq, 1);
>> +		else
>> +			irq_dispose_mapping(virq);
>> +		return 0;
>> +	}
>> +
>> +	/* Store trigger type */
>> +	irqd_set_trigger_type(irq_data, type);
>> +
>>  	return virq;
>>  }
>>  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
>>
> 
> This patch have the effect of making misconfigured PPIs absolutely
> obvious. I still need to wrap my head around the root cause, but here's
> the findings I have so far:
>
> - kvmtool generates a DT with the wrong trigger information (edge
> instead of level) for the timer.
> - with this patch applied, "cyclictest -S" reliably locks up when run in
> a guest (missing a timer interrupt, goodbye CPU).
> - Either fixing kvmtool or reverting that patch makes it work reliably
> again.
> 
> My gut feeling is that until that patch, the failing irq_set_irq_type()
> wasn't affecting the kernel's view of the trigger (it was still treated
> as level). With this patch, the kernel now trusts whatever is coming
> from the firmware, and the misconfiguration becomes obvious. And just
> grepping through the DT files for arm and arm64 sends makes me thing
> "Holly effin' crap!".
> 
> I'm not saying that we shouldn't perform this change though. But it is
> quite obvious that it is going to break an awful lot of existing code
> and platforms. I'm also cooking a small patch for the arch timer (which
> seems to be described in DT with a fairly high level of brokenness), so
> that we can mop-up most of the brain damage.

Hmmm ... yes I see. I wonder if we should make the setting of the type
here dependent upon PM being enabled for an irqchip? We could check to
see if the .parent_device is populated and if so only then save the type
and otherwise just set it as we do today.

We could add a WARN to the existing irq_set_irq_type() or may be just a
pr_warn() if a WARN is too verbose so people can fix up any issues.

I am also wondering if patch 4/17 "iqdomain: Fix handling of type
settings for existing mappings" could generate a lot of reports
interrupts failing due to bad firmware? I wonder if I should tone this
patch down to a warning message as well as opposed to a complete failure.

Cheers
Jon

[toc] | [prev] | [next] | [standalone]

#1397135 — Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ

On 09/05/16 14:13, Jon Hunter wrote:
> 
> On 09/05/16 13:23, Marc Zyngier wrote:
>> On 04/05/16 17:25, Jon Hunter wrote:
>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>>> controllers, may require require additional runtime power management
>>> control to ensure they are accessible. For such IRQ chips, it makes sense
>>> to enable the IRQ chip when interrupts are requested and disabled them
>>> again once all interrupts have been freed.
>>>
>>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>>> The mapping of the IRQ happens before the IRQ is requested and so the
>>> programming of the type settings occurs before the IRQ is requested. This
>>> is a problem for IRQ chips that require additional power management
>>> control because they may not be accessible yet. Therefore, when mapping
>>> the IRQ, don't program the type settings, just save them and then program
>>> these saved settings when the IRQ is requested (so long as if they are not
>>> overridden via the call to request the IRQ).
>>>
>>> Add a stub function for irq_domain_free_irqs() to avoid any compilation
>>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  include/linux/irqdomain.h |  3 +++
>>>  kernel/irq/irqdomain.c    | 23 ++++++++++++++++++-----
>>>  2 files changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>> index 2aed04396210..fc66876d1965 100644
>>> --- a/include/linux/irqdomain.h
>>> +++ b/include/linux/irqdomain.h
>>> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain,
>>>  	return -1;
>>>  }
>>>  
>>> +static inline void irq_domain_free_irqs(unsigned int virq,
>>> +					unsigned int nr_irqs) { }
>>> +
>>>  static inline bool irq_domain_is_hierarchy(struct irq_domain *domain)
>>>  {
>>>  	return false;
>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>>> index d68371213fc9..bbf5b9b8ac3d 100644
>>> --- a/kernel/irq/irqdomain.c
>>> +++ b/kernel/irq/irqdomain.c
>>> @@ -564,6 +564,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
>>>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>>  {
>>>  	struct irq_domain *domain;
>>> +	struct irq_data *irq_data;
>>>  	irq_hw_number_t hwirq;
>>>  	unsigned int type = IRQ_TYPE_NONE;
>>>  	int virq;
>>> @@ -613,7 +614,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>>  		 * it now and return the interrupt number.
>>>  		 */
>>>  		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
>>> -			irq_set_irq_type(virq, type);
>>> +			irq_data = irq_get_irq_data(virq);
>>> +			if (!irq_data)
>>> +				return 0;
>>> +
>>> +			irqd_set_trigger_type(irq_data, type);
>>>  			return virq;
>>>  		}
>>>  
>>> @@ -633,10 +638,18 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>>  			return virq;
>>>  	}
>>>  
>>> -	/* Set type if specified and different than the current one */
>>> -	if (type != IRQ_TYPE_NONE &&
>>> -	    type != irq_get_trigger_type(virq))
>>> -		irq_set_irq_type(virq, type);
>>> +	irq_data = irq_get_irq_data(virq);
>>> +	if (!irq_data) {
>>> +		if (irq_domain_is_hierarchy(domain))
>>> +			irq_domain_free_irqs(virq, 1);
>>> +		else
>>> +			irq_dispose_mapping(virq);
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* Store trigger type */
>>> +	irqd_set_trigger_type(irq_data, type);
>>> +
>>>  	return virq;
>>>  }
>>>  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
>>>
>>
>> This patch have the effect of making misconfigured PPIs absolutely
>> obvious. I still need to wrap my head around the root cause, but here's
>> the findings I have so far:
>>
>> - kvmtool generates a DT with the wrong trigger information (edge
>> instead of level) for the timer.
>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>> a guest (missing a timer interrupt, goodbye CPU).
>> - Either fixing kvmtool or reverting that patch makes it work reliably
>> again.
>>
>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>> wasn't affecting the kernel's view of the trigger (it was still treated
>> as level). With this patch, the kernel now trusts whatever is coming
>> from the firmware, and the misconfiguration becomes obvious. And just
>> grepping through the DT files for arm and arm64 sends makes me thing
>> "Holly effin' crap!".
>>
>> I'm not saying that we shouldn't perform this change though. But it is
>> quite obvious that it is going to break an awful lot of existing code
>> and platforms. I'm also cooking a small patch for the arch timer (which
>> seems to be described in DT with a fairly high level of brokenness), so
>> that we can mop-up most of the brain damage.
> 
> Hmmm ... yes I see. I wonder if we should make the setting of the type
> here dependent upon PM being enabled for an irqchip? We could check to
> see if the .parent_device is populated and if so only then save the type
> and otherwise just set it as we do today.

I don't really like the idea of having multiple code paths for the same thing.
This is very error prone, and likely to bitrot pretty quickly.

> We could add a WARN to the existing irq_set_irq_type() or may be just a
> pr_warn() if a WARN is too verbose so people can fix up any issues.
> 
> I am also wondering if patch 4/17 "iqdomain: Fix handling of type
> settings for existing mappings" could generate a lot of reports
> interrupts failing due to bad firmware? I wonder if I should tone this
> patch down to a warning message as well as opposed to a complete failure.

We'll see. We can always tone it down a notch, should it prove to be too noisy...
So far, I haven't seen it firing. On the other hand, I get the following stuff
on my APM board:

[    0.000000] GIC: PPI0 is either secure or misconfigured
[    0.000000] GIC: PPI13 is either secure or misconfigured
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ1, assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware

Pretty awesome...

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

[toc] | [prev] | [next] | [standalone]

#1397154 — Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ

On 09/05/16 16:10, Marc Zyngier wrote:
> On 09/05/16 14:13, Jon Hunter wrote:
>> On 09/05/16 13:23, Marc Zyngier wrote:

[snip]

>>> This patch have the effect of making misconfigured PPIs absolutely
>>> obvious. I still need to wrap my head around the root cause, but here's
>>> the findings I have so far:
>>>
>>> - kvmtool generates a DT with the wrong trigger information (edge
>>> instead of level) for the timer.
>>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>>> a guest (missing a timer interrupt, goodbye CPU).
>>> - Either fixing kvmtool or reverting that patch makes it work reliably
>>> again.
>>>
>>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>>> wasn't affecting the kernel's view of the trigger (it was still treated
>>> as level). With this patch, the kernel now trusts whatever is coming
>>> from the firmware, and the misconfiguration becomes obvious. And just
>>> grepping through the DT files for arm and arm64 sends makes me thing
>>> "Holly effin' crap!".
>>>
>>> I'm not saying that we shouldn't perform this change though. But it is
>>> quite obvious that it is going to break an awful lot of existing code
>>> and platforms. I'm also cooking a small patch for the arch timer (which
>>> seems to be described in DT with a fairly high level of brokenness), so
>>> that we can mop-up most of the brain damage.
>>
>> Hmmm ... yes I see. I wonder if we should make the setting of the type
>> here dependent upon PM being enabled for an irqchip? We could check to
>> see if the .parent_device is populated and if so only then save the type
>> and otherwise just set it as we do today.
> 
> I don't really like the idea of having multiple code paths for the same thing.
> This is very error prone, and likely to bitrot pretty quickly.

True. However, we really need this change for irqchips and runtime-pm.
So to confirm what are you suggesting we do? We could add a WARN around
irq_set_irq_type() in irq_create_fwspec_mapping() for v4.7 and see how
many complaints we get :-)

>> We could add a WARN to the existing irq_set_irq_type() or may be just a
>> pr_warn() if a WARN is too verbose so people can fix up any issues.
>>
>> I am also wondering if patch 4/17 "iqdomain: Fix handling of type
>> settings for existing mappings" could generate a lot of reports
>> interrupts failing due to bad firmware? I wonder if I should tone this
>> patch down to a warning message as well as opposed to a complete failure.
> 
> We'll see. We can always tone it down a notch, should it prove to be too noisy...
> So far, I haven't seen it firing. On the other hand, I get the following stuff
> on my APM board:
> 
> [    0.000000] GIC: PPI0 is either secure or misconfigured
> [    0.000000] GIC: PPI13 is either secure or misconfigured
> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ1, assuming level low
> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, assuming level low
> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> 
> Pretty awesome...

Indeed.

Jon

[toc] | [prev] | [next] | [standalone]

#1398045 — Re: [PATCH V3 06/17] irqdomain: Don't set type when mapping an IRQ

On 09/05/16 16:44, Jon Hunter wrote:
> 
> On 09/05/16 16:10, Marc Zyngier wrote:
>> On 09/05/16 14:13, Jon Hunter wrote:
>>> On 09/05/16 13:23, Marc Zyngier wrote:
> 
> [snip]
> 
>>>> This patch have the effect of making misconfigured PPIs absolutely
>>>> obvious. I still need to wrap my head around the root cause, but here's
>>>> the findings I have so far:
>>>>
>>>> - kvmtool generates a DT with the wrong trigger information (edge
>>>> instead of level) for the timer.
>>>> - with this patch applied, "cyclictest -S" reliably locks up when run in
>>>> a guest (missing a timer interrupt, goodbye CPU).
>>>> - Either fixing kvmtool or reverting that patch makes it work reliably
>>>> again.
>>>>
>>>> My gut feeling is that until that patch, the failing irq_set_irq_type()
>>>> wasn't affecting the kernel's view of the trigger (it was still treated
>>>> as level). With this patch, the kernel now trusts whatever is coming
>>>> from the firmware, and the misconfiguration becomes obvious. And just
>>>> grepping through the DT files for arm and arm64 sends makes me thing
>>>> "Holly effin' crap!".
>>>>
>>>> I'm not saying that we shouldn't perform this change though. But it is
>>>> quite obvious that it is going to break an awful lot of existing code
>>>> and platforms. I'm also cooking a small patch for the arch timer (which
>>>> seems to be described in DT with a fairly high level of brokenness), so
>>>> that we can mop-up most of the brain damage.
>>>
>>> Hmmm ... yes I see. I wonder if we should make the setting of the type
>>> here dependent upon PM being enabled for an irqchip? We could check to
>>> see if the .parent_device is populated and if so only then save the type
>>> and otherwise just set it as we do today.
>>
>> I don't really like the idea of having multiple code paths for the same thing.
>> This is very error prone, and likely to bitrot pretty quickly.
> 
> True. However, we really need this change for irqchips and runtime-pm.
> So to confirm what are you suggesting we do? We could add a WARN around
> irq_set_irq_type() in irq_create_fwspec_mapping() for v4.7 and see how
> many complaints we get :-)

Let's add a pr_warn(), and see how noisy this becomes. We may have to
remove it if it becomes too loud though.

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1394521 — [PATCH V3 15/17] irqchip/gic: Split GIC init in preparation for platform driver

To re-use the code that initialises the GIC (found in
__gic_init_bases()), from within a platform driver, it is necessary to
move the code from the __init section so that it is always present and
not removed. Unfortunately, it is not possible to simply drop the __init
from the function declaration for __gic_init_bases() because it contains
calls to set_smp_cross_call() and set_handle_irq() which are both
located in the __init section. Fortunately, these calls are only
required for the root controller and because the initial platform driver
will only support non-root controllers that can be initialised later in
the boot process, we can move these calls to another function.

Move the bulk of the code from __gic_init_bases() to a new function
called gic_init_bases() which is not located in the __init section and
can be used by the platform driver. Update __gic_init_bases() to call
gic_init_bases() and if necessary, set_smp_cross_call() and
set_handle_irq().

To allow the platform driver to dynamically allocate the GIC chip data
structure, rather than passing an index to reference the GIC to
gic_init_bases() pass a pointer to the chip data instead. This means
that the name must be passed to gic_init_bases() as well, because the
name will not be passed upon an index for a platform device.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 71 +++++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 24 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 2fc5018d6eb9..15e8a12813cc 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1002,32 +1002,29 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.unmap = gic_irq_domain_unmap,
 };
 
-static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
-			   void __iomem *dist_base, void __iomem *cpu_base,
-			   u32 percpu_offset, struct fwnode_handle *handle)
+static int gic_init_bases(struct gic_chip_data *gic, int irq_start,
+			  void __iomem *dist_base, void __iomem *cpu_base,
+			  u32 percpu_offset, struct fwnode_handle *handle,
+			  const char *name)
 {
 	irq_hw_number_t hwirq_base;
-	struct gic_chip_data *gic;
-	int gic_irqs, irq_base, i, ret;
-
-	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
+	int gic_irqs, irq_base, ret;
 
-	gic = &gic_data[gic_nr];
+	if (WARN_ON(!gic || gic->domain))
+		return -EINVAL;
 
 	/* Initialize irq_chip */
 	gic->chip = gic_chip;
+	gic->chip.name = name;
 
-	if (static_key_true(&supports_deactivate) && gic_nr == 0) {
+	if (static_key_true(&supports_deactivate) && gic == &gic_data[0]) {
 		gic->chip.irq_mask = gic_eoimode1_mask_irq;
 		gic->chip.irq_eoi = gic_eoimode1_eoi_irq;
 		gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
-		gic->chip.name = kasprintf(GFP_KERNEL, "GICv2");
-	} else {
-		gic->chip.name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
 	}
 
 #ifdef CONFIG_SMP
-	if (gic_nr == 0)
+	if (gic == &gic_data[0])
 		gic->chip.irq_set_affinity = gic_set_affinity;
 #endif
 
@@ -1081,7 +1078,7 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		 * For primary GICs, skip over SGIs.
 		 * For secondary GICs, skip over PPIs, too.
 		 */
-		if (gic_nr == 0 && (irq_start & 31) > 0) {
+		if (gic == &gic_data[0] && (irq_start & 31) > 0) {
 			hwirq_base = 16;
 			if (irq_start != -1)
 				irq_start = (irq_start & ~31) + 16;
@@ -1108,6 +1105,42 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		goto error;
 	}
 
+	return 0;
+
+error:
+	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+		free_percpu(gic->dist_base.percpu_base);
+		free_percpu(gic->cpu_base.percpu_base);
+	}
+
+	return ret;
+}
+
+static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
+			   void __iomem *dist_base, void __iomem *cpu_base,
+			   u32 percpu_offset, struct fwnode_handle *handle)
+{
+	struct gic_chip_data *gic;
+	char *name;
+	int i, ret;
+
+	if (WARN_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR))
+		return -EINVAL;
+
+	gic = &gic_data[gic_nr];
+
+	if (static_key_true(&supports_deactivate) && gic_nr == 0)
+		name = kasprintf(GFP_KERNEL, "GICv2");
+	else
+		name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
+
+	ret = gic_init_bases(gic, irq_start, dist_base, cpu_base, percpu_offset,
+			     handle, name);
+	if (ret) {
+		kfree(name);
+		return ret;
+	}
+
 	if (gic_nr == 0) {
 		/*
 		 * Initialize the CPU interface map to all CPUs.
@@ -1130,16 +1163,6 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	gic_pm_init(gic);
 
 	return 0;
-
-error:
-	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
-		free_percpu(gic->dist_base.percpu_base);
-		free_percpu(gic->cpu_base.percpu_base);
-	}
-
-	kfree(gic->chip.name);
-
-	return ret;
 }
 
 void __init gic_init(unsigned int gic_nr, int irq_start,
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1394522 — [PATCH V3 14/17] irqchip/gic: Add helper function for configuring a GIC via device-tree

Move the code that configures a GIC via device-tree into it's own
function. This will allow us to re-use this function when adding a
platform driver to support GICs that require power-management.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a10274926690..2fc5018d6eb9 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1194,6 +1194,28 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
+static int gic_of_setup(struct device_node *node, void __iomem **dist_base,
+			void __iomem **cpu_base, u32 *percpu_offset)
+{
+	if (!node || !dist_base || !cpu_base || !percpu_offset)
+		return -EINVAL;
+
+	*dist_base = of_iomap(node, 0);
+	if (WARN(!*dist_base, "unable to map gic dist registers\n"))
+		return -ENOMEM;
+
+	*cpu_base = of_iomap(node, 1);
+	if (WARN(!*cpu_base, "unable to map gic cpu registers\n")) {
+		iounmap(*dist_base);
+		return -ENOMEM;
+	}
+
+	if (of_property_read_u32(node, "cpu-offset", percpu_offset))
+		*percpu_offset = 0;
+
+	return 0;
+}
+
 int __init
 gic_of_init(struct device_node *node, struct device_node *parent)
 {
@@ -1214,15 +1236,9 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	    of_property_read_bool(node, "power-domains"))
 		return 0;
 
-	dist_base = of_iomap(node, 0);
-	if (WARN(!dist_base, "unable to map gic dist registers\n"))
-		return -ENOMEM;
-
-	cpu_base = of_iomap(node, 1);
-	if (WARN(!cpu_base, "unable to map gic cpu registers\n")) {
-		iounmap(dist_base);
-		return -ENOMEM;
-	}
+	ret = gic_of_setup(node, &dist_base, &cpu_base, &percpu_offset);
+	if (ret)
+		return ret;
 
 	/*
 	 * Disable split EOI/Deactivate if either HYP is not available
@@ -1231,9 +1247,6 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	if (gic_cnt == 0 && !gic_check_eoimode(node, &cpu_base))
 		static_key_slow_dec(&supports_deactivate);
 
-	if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
-		percpu_offset = 0;
-
 	ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
 			 &node->fwnode);
 	if (ret) {
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1394525 — [PATCH V3 10/17] irqchip/gic: Remove static irq_chip definition for eoimode1

There are only 3 differences (not including the name) in the definitions
of the gic_chip and gic_eoimode1_chip structures. Instead of statically
defining the gic_eoimode1_chip structure, remove it and populate the
eoimode1 functions dynamically for the appropriate GIC irqchips.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 16f632dcc2ad..13a0d77e8ae9 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -391,20 +391,6 @@ static struct irq_chip gic_chip = {
 				  IRQCHIP_MASK_ON_SUSPEND,
 };
 
-static struct irq_chip gic_eoimode1_chip = {
-	.name			= "GICv2",
-	.irq_mask		= gic_eoimode1_mask_irq,
-	.irq_unmask		= gic_unmask_irq,
-	.irq_eoi		= gic_eoimode1_eoi_irq,
-	.irq_set_type		= gic_set_type,
-	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
-	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
-	.irq_set_vcpu_affinity	= gic_irq_set_vcpu_affinity,
-	.flags			= IRQCHIP_SET_TYPE_MASKED |
-				  IRQCHIP_SKIP_SET_WAKE |
-				  IRQCHIP_MASK_ON_SUSPEND,
-};
-
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 {
 	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
@@ -1025,10 +1011,14 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	gic = &gic_data[gic_nr];
 
 	/* Initialize irq_chip */
+	gic->chip = gic_chip;
+
 	if (static_key_true(&supports_deactivate) && gic_nr == 0) {
-		gic->chip = gic_eoimode1_chip;
+		gic->chip.irq_mask = gic_eoimode1_mask_irq;
+		gic->chip.irq_eoi = gic_eoimode1_eoi_irq;
+		gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
+		gic->chip.name = "GICv2";
 	} else {
-		gic->chip = gic_chip;
 		gic->chip.name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
 	}
 
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1394527 — [PATCH V3 07/17] genirq: Ensure IRQ descriptor is valid when setting-up the IRQ

In the function, setup_irq(), we don't check that the descriptor
returned from irq_to_desc() is valid before we start using it. For
example chip_bus_lock() called from setup_irq(), assumes that the
descriptor pointer is valid and doesn't check before dereferencing it.

In many other functions including setup/free_percpu_irq() we do check
that the descriptor returned is not NULL and therefore add the same test
to setup_irq() to ensure the descriptor returned is valid.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 kernel/irq/manage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index b2a93a37f772..eaedeb74b49d 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1414,7 +1414,7 @@ int setup_irq(unsigned int irq, struct irqaction *act)
 	int retval;
 	struct irq_desc *desc = irq_to_desc(irq);
 
-	if (WARN_ON(irq_settings_is_per_cpu_devid(desc)))
+	if (!desc || WARN_ON(irq_settings_is_per_cpu_devid(desc)))
 		return -EINVAL;
 	chip_bus_lock(desc);
 	retval = __setup_irq(irq, desc, act);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1394530 — [PATCH V3 04/17] irqdomain: Fix handling of type settings for existing mappings

When mapping an IRQ, it is possible that a mapping for the IRQ already
exists. If mapping does exist then there are the following issues with
regard to the handling of the IRQ type settings ...
1. If the domain is part of a hierarchy, then:
   a. We do not check that the type settings for the existing mapping
      match those of the new mapping.
   b. We do not check to see if the type settings have been programmed
      yet (and they might not have been) and so we may never set the
      type.
2. If the domain is NOT part of a hierarchy, we will overwrite the
   current type settings programmed if they are different from the
   previous mapping. Please note that irq_create_mapping()
   calls irq_find_mapping() to check if a mapping already exists.

Although, it may be unlikely that the type settings for a shared
interrupt would not match, nonetheless we should check for this.
Therefore, to fix this check if a mapping exists (regardless of whether
the domain is part of a hierarchy or not) and if it does then:
1. Return the IRQ number if the type settings match or are not
   specified.
2. Program the type settings and return the IRQ number if the type
   settings have not been programmed yet.
3. Otherwise if the type setting do not match, then print a warning
   and don't return the IRQ number.

Furthermore, add a warning if the type return by irq_domain_translate()
has bits outside the sense mask set and then clear these bits. If these
bits are not cleared then this will cause the comparision of the type
settings for an existing mapping to fail with that of the new mapping
even if the sense bit themselves match. The reason being is that the
existing type settings are read by calling irq_get_trigger_type() which
will clear any bits outside the sense mask. This will allow us to detect
irqchips that are not correctly clearing these bits and fix them.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
---
 kernel/irq/irqdomain.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index e564e16f2530..d68371213fc9 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -587,15 +587,42 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	if (irq_domain_translate(domain, fwspec, &hwirq, &type))
 		return 0;
 
-	if (irq_domain_is_hierarchy(domain)) {
+	/*
+	 * WARN if the irqchip returns a type with bits
+	 * outside the sense mask set and clear these bits.
+	 */
+	if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
+		type &= IRQ_TYPE_SENSE_MASK;
+
+	/*
+	 * If we've already configured this interrupt,
+	 * don't do it again, or hell will break loose.
+	 */
+	virq = irq_find_mapping(domain, hwirq);
+	if (virq) {
 		/*
-		 * If we've already configured this interrupt,
-		 * don't do it again, or hell will break loose.
+		 * If the trigger type is not specified or matches the
+		 * current trigger type then we are done so return the
+		 * interrupt number.
 		 */
-		virq = irq_find_mapping(domain, hwirq);
-		if (virq)
+		if (type == IRQ_TYPE_NONE || type == irq_get_trigger_type(virq))
 			return virq;
 
+		/*
+		 * If the trigger type has not been set yet, then set
+		 * it now and return the interrupt number.
+		 */
+		if (irq_get_trigger_type(virq) == IRQ_TYPE_NONE) {
+			irq_set_irq_type(virq, type);
+			return virq;
+		}
+
+		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
+			hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));
+		return 0;
+	}
+
+	if (irq_domain_is_hierarchy(domain)) {
 		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
 		if (virq <= 0)
 			return 0;
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1394531 — [PATCH V3 09/17] irqchip/gic: Don't initialise chip if mapping IO space fails

If we fail to map the address space for the GIC distributor or CPU
interface, then don't attempt to initialise the chip, just WARN and
return.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-gic.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 095bb5b5c3f2..16f632dcc2ad 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1200,10 +1200,14 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 		return -ENODEV;
 
 	dist_base = of_iomap(node, 0);
-	WARN(!dist_base, "unable to map gic dist registers\n");
+	if (WARN(!dist_base, "unable to map gic dist registers\n"))
+		return -ENOMEM;
 
 	cpu_base = of_iomap(node, 1);
-	WARN(!cpu_base, "unable to map gic cpu registers\n");
+	if (WARN(!cpu_base, "unable to map gic cpu registers\n")) {
+		iounmap(dist_base);
+		return -ENOMEM;
+	}
 
 	/*
 	 * Disable split EOI/Deactivate if either HYP is not available
-- 
2.1.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web