linux-kernel - Re: [PATCH 1/2] irq: sparseirq enabling v2

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, 26 Nov 2008 08:48:26 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] irq: sparseirq enabling v2


* Yinghai Lu <yinghai@...nel.org> wrote:

> impact: new feature sparseirq

> v2: use pointer array instead of hash

ok, this looks pretty good!

A few details:

> +#ifdef CONFIG_SPARSE_IRQ
> +#define set_ioapic_affinity_irq set_ioapic_affinity_irq_desc
> +#else
> +static void set_ioapic_affinity_irq(unsigned int irq, cpumask_t mask)
> +{
> +	struct irq_desc *desc;
> +
> +	desc = irq_to_desc(irq);
> +
> +	set_ioapic_affinity_irq_desc(desc, mask);
> +}
> +#endif

i think this distinction can now go away?.

>  #ifdef CONFIG_INTR_REMAP
> +#ifdef CONFIG_SPARSE_IRQ
> +static void ack_x2apic_level_desc(struct irq_desc **descp)
> +{
> +	ack_x2APIC_irq();
> +}
> +
> +static void ack_x2apic_edge_desc(struct irq_desc **descp)
> +{
> +	ack_x2APIC_irq();
> +}
> +
> +#define ack_x2apic_level ack_x2apic_level_desc
> +#define ack_x2apic_edge ack_x2apic_edge_desc
> +#else

is this needed now?

> +#ifdef CONFIG_SMP
> +	move_native_irq_desc(descp);
> +#endif

please hide this #ifdef in a header.

> +# ifdef CONFIG_SMP
> +			move_masked_irq_desc(descp);
> +# endif

ditto.

> +#ifdef CONFIG_SPARSE_IRQ
> +#define ack_apic_edge ack_apic_edge_desc
> +#define ack_apic_level ack_apic_level_desc
> +#else
> +static void ack_apic_edge(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
> +
> +	ack_apic_edge_desc(&desc);
> +}
> +static void ack_apic_level(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
> +
> +	ack_apic_level_desc(&desc);
> +}
> +#endif

distinction needed?

>  
> +#ifdef CONFIG_SPARSE_IRQ
> +#define mask_lapic_irq mask_lapic_irq_desc
> +#define unmask_lapic_irq unmask_lapic_irq_desc
> +#define ack_lapic_irq ack_lapic_irq_desc
> +#else
> +static void mask_lapic_irq(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
> +
> +	mask_lapic_irq_desc(&desc);
> +}
> +static void unmask_lapic_irq(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
> +
> +	unmask_lapic_irq_desc(&desc);
> +}
> +static void ack_lapic_irq(unsigned int irq)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
> +
> +	ack_lapic_irq_desc(&desc);
> +}
> +#endif

same.

> +#ifdef CONFIG_SPARSE_IRQ
> +	for (new = irq_want; new < NR_IRQS; new++)
> +#else
> +	for (new = irq_want; new > 0; new--)
> +#endif

this assymetry seems unnecessary too now i think.

> +#ifdef CONFIG_SPARSE_IRQ
> +	irq_want = nr_irqs;
> +#else
> +	irq_want = NR_IRQS - 1;
> +#endif

ditto. I think we dont want 'nr_irqs' anymore - just remain with 
NR_IRQS, right?

> +#ifdef CONFIG_SPARSE_IRQ
> +#define set_msi_irq_affinity set_msi_irq_affinity_desc
> +#else
> +static void set_msi_irq_affinity(unsigned int irq, cpumask_t mask)
> +{
> +	struct irq_desc *desc = irq_to_desc(irq);
>  
> +	set_msi_irq_affinity_desc(desc, mask);
> +}
> +#endif

This ifdef seems unnecessary too.

Plus in a few more places.

Please look at _every_ #ifdef or #if in your patch in a .c file and 
ask the question: can we somehow in some way eliminate it and convert 
it to some nice inline somewhere or eliminate it via some other trick?

	Ingo
--
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/