lkml.org 
[lkml]   [2008]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] irq: sparseirq enabling v2

* Yinghai Lu <yinghai@kernel.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


\
 
 \ /
  Last update: 2008-11-26 08:51    [W:0.203 / U:0.352 seconds]
©2003-2014 Jasper Spaans. hosted at Digital Ocean