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

[Ingo Molnar]

* Yinghai Lu <yinghai@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/