linux-kernel - Re: [PATCH 05/13 v3] irqchip/gic: assign irqchip dynamically

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

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

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

Message-ID: <5638E877.70508@arm.com>
Date:	Tue, 03 Nov 2015 17:01:43 +0000
From:	Marc Zyngier <marc.zyngier@....com>
To:	Linus Walleij <linus.walleij@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>
CC:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/13 v3] irqchip/gic: assign irqchip dynamically

On 02/11/15 14:34, Linus Walleij wrote:
> Instead of having the irqchip being a static struct, make it part
> of the per-instance data so we can assign it a dynamic name. This
> has the usable side effect of displaying the GIC with an instance
> number as GIC0, GIC1 ... GICn in /proc/interrupts, which is helpful
> when debugging cascaded GICs, such as on the ARM PB11MPCore.
> 
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Jason Cooper <jason@...edaemon.net>
> Cc: Marc Zyngier <marc.zyngier@....com>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> ChangeLog v2->v3:
> - Handle the EOIMODE1 chips properly: select a proper &chip pointer
>   at chip registration and store in the per-GIC .chip member.
>   Saves hard-to-grasp code in the .map function too. We select the
>   EOIMODE1 chip if and only if it's GIC0 and the machine suppors
>   deactivation.
> - Name ordinary GIC chips GIC0, ... GICN and the EOIMODE1 chip
>   GICEOI, as there can be only one such chip.
> ChangeLog v1->v2:
> - Keep the static structs around, just delete the .name
>   field assign them to the chips at registration time, updating
>   the name field with the instance number.
> - Also enumerate the EOIMODE1 sub-chips.
> - Broke out this irqchip stuff from the rest of the series so as
>   not to stress the irqchip maintainers. It has no dependencies
>   on the other patches anyways, and can be merged stand-alone.
> 
> Marc: can't test the EOIMODE1 thing, it's far above me, but it
> "should work". Is it correct that there is one unique and coupled
> EOIMODE1 instance per GIC instance like this?
> ---
>  drivers/irqchip/irq-gic.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 9ec8cf5137d9..9c61ca182433 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -58,6 +58,7 @@ union gic_base {
>  };
>  
>  struct gic_chip_data {
> +	struct irq_chip chip;
>  	union gic_base dist_base;
>  	union gic_base cpu_base;
>  #ifdef CONFIG_CPU_PM
> @@ -370,7 +371,6 @@ static void gic_handle_cascade_irq(struct irq_desc *desc)
>  }
>  
>  static struct irq_chip gic_chip = {
> -	.name			= "GIC",
>  	.irq_mask		= gic_mask_irq,
>  	.irq_unmask		= gic_unmask_irq,
>  	.irq_eoi		= gic_eoi_irq,
> @@ -386,7 +386,7 @@ static struct irq_chip gic_chip = {
>  };
>  
>  static struct irq_chip gic_eoimode1_chip = {
> -	.name			= "GICv2",
> +	.name			= "GICEOI",

Hmmm. I'd rather leave this to GICv2, because EOImode==1 is a feature of
the architecture, rather than a piece of HW. Only nitpicking, really.

>  	.irq_mask		= gic_eoimode1_mask_irq,
>  	.irq_unmask		= gic_unmask_irq,
>  	.irq_eoi		= gic_eoimode1_eoi_irq,
> @@ -880,12 +880,8 @@ void __init gic_init_physaddr(struct device_node *node)
>  static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
>  				irq_hw_number_t hw)
>  {
> -	struct irq_chip *chip = &gic_chip;
> -
> -	if (static_key_true(&supports_deactivate)) {
> -		if (d->host_data == (void *)&gic_data[0])
> -			chip = &gic_eoimode1_chip;
> -	}
> +	struct gic_chip_data *gic = d->host_data;
> +	struct irq_chip *chip = &gic->chip;
>  
>  	if (hw < 32) {
>  		irq_set_percpu_devid(irq);
> @@ -989,6 +985,9 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
>  	BUG_ON(gic_nr >= MAX_GIC_NR);
>  
>  	gic = &gic_data[gic_nr];
> +	gic->chip = gic_chip;
> +	gic->chip.name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr);
> +
>  #ifdef CONFIG_GIC_NON_BANKED
>  	if (percpu_offset) { /* Frankein-GIC without banked registers... */
>  		unsigned int cpu;
> @@ -1079,8 +1078,10 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
>  		register_cpu_notifier(&gic_cpu_notifier);
>  #endif
>  		set_handle_irq(gic_handle_irq);
> -		if (static_key_true(&supports_deactivate))
> +		if (static_key_true(&supports_deactivate)) {
>  			pr_info("GIC: Using split EOI/Deactivate mode\n");
> +			gic->chip = gic_eoimode1_chip;
> +		}
>  	}
>  
>  	gic_dist_init(gic);
> 

Other than the above:

Acked-by: Marc Zyngier <marc.zyngier@....com>

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/