linux-kernel - Re: [PATCH] x86/dt: use linear irq domain for ioapic(s).

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 8 Aug 2012 12:46:29 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Sebastian Andrzej Siewior <sebastian@...akpoint.cc>
Cc:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Grant Likely <grant.likely@...retlab.ca>, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/dt: use linear irq domain for ioapic(s).

On Mon, Aug 06, 2012 at 09:38:11AM +0200, Sebastian Andrzej Siewior wrote:
> The former conversion to irq_domain_add_legacy() did not fully work
> since we miss the irq decs for NR_IRQS_LEGACY+.
> Ideally we could use irq_domain_add_simple() or the no-map variant (and
> program the virq <-> line mapping directly into ioapic) but this would
> require a different irq lookup in "do_IRQ()" and won't work with ACPI
> without changes. So this is probably easiest for everyone.
> 
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@...akpoint.cc>
> ---
>  arch/x86/kernel/devicetree.c |   52 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index 3ae2ced..df225fc 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -342,6 +342,48 @@ const struct irq_domain_ops ioapic_irq_domain_ops = {
>  	.xlate = ioapic_xlate,
>  };
>  
> +static void dt_add_ioapic_domain(unsigned int ioapic_num,
> +		struct device_node *np)
> +{
> +	struct irq_domain *id;
> +	struct mp_ioapic_gsi *gsi_cfg;
> +	int ret;
> +	int num;
> +
> +	gsi_cfg = mp_ioapic_gsi_routing(ioapic_num);
> +	num = gsi_cfg->gsi_end - gsi_cfg->gsi_base + 1;
> +
> +	id = irq_domain_add_linear(np, num,
> +			&ioapic_irq_domain_ops,
> +			(void *)ioapic_num);

This fits on two lines instead of three.

> +	BUG_ON(!id);
> +	if (gsi_cfg->gsi_base == 0) {
> +		/*
> +		 * The first NR_IRQS_LEGACY irq descs are allocated in
> +		 * early_irq_init() and need just a mapping. The
> +		 * remaining irqs need both. All of them are preallocated
> +		 * and assigned so we can keep the 1:1 mapping which the ioapic
> +		 * is having.
> +		 */
> +		ret = irq_domain_associate_many(id, 0, 0, NR_IRQS_LEGACY);
> +		if (ret)
> +			pr_err("Error mapping legacy irqs: %d\n", ret);
> +
> +		if (num > NR_IRQS_LEGACY) {
> +			ret = irq_create_strict_mappings(id, NR_IRQS_LEGACY,
> +					NR_IRQS_LEGACY, num - NR_IRQS_LEGACY);
> +			if (ret)
> +				pr_err("Error creating mapping for the "
> +						"remaining  irqs: %d\n", ret);

There's an extra space between "remaining" and "irqs". Also other places
use the spelling IRQ and IRQs respectively in strings, so it may be nice
to stay consistent.

> +		}
> +		irq_set_default_host(id);
> +	} else {
> +		ret = irq_create_strict_mappings(id, gsi_cfg->gsi_base, 0, num);
> +		if (ret)
> +			pr_err("Error creating irq mapping: %d\n", ret);
> +	}
> +}
> +
>  static void __init ioapic_add_ofnode(struct device_node *np)
>  {
>  	struct resource r;
> @@ -356,15 +398,7 @@ static void __init ioapic_add_ofnode(struct device_node *np)
>  
>  	for (i = 0; i < nr_ioapics; i++) {
>  		if (r.start == mpc_ioapic_addr(i)) {
> -			struct irq_domain *id;
> -			struct mp_ioapic_gsi *gsi_cfg;
> -
> -			gsi_cfg = mp_ioapic_gsi_routing(i);
> -
> -			id = irq_domain_add_legacy(np, 32, gsi_cfg->gsi_base, 0,
> -						   &ioapic_irq_domain_ops,
> -						   (void*)i);
> -			BUG_ON(!id);
> +			dt_add_ioapic_domain(i, np);
>  			return;
>  		}
>  	}

Besides the above nitpicks:

Reviewed-by: Thierry Reding <thierry.reding@...onic-design.de>
Tested-by: Thierry Reding <thierry.reding@...onic-design.de>

On another note, I saw that you've used the "intel,ce4100" prefix in
various places and I wonder if it would be useful to replace them with
something more generic like "intel,hpet", "intel,lapic" and
"intel,ioapic" respectively. The hardware that I use is based on an Atom
N450 and works with the current code, so it really isn't ce4100-
specific.

Given that this is x86/devicetree only and fixes things that didn't work
before, can it go into 3.6? Backporting to stable is probably not worth
it because it depends on a number of other IRQ domain patches that are
only available in 3.6.

Thierry

Content of type "application/pgp-signature" skipped