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.062 / U:0.076 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site