Re: [PATCH 06/12] genericirq: make irq_chip related function to take desc

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, 4 Mar 2010, Yinghai Lu wrote:

>  /*
>   * Fixup enable/disable function pointers
>   */
>  void irq_chip_set_defaults(struct irq_chip *chip)
>  {
> +	if (!chip->desc_enable)
> +		chip->desc_enable = default_enable_desc;
> +	if (!chip->desc_disable)
> +		chip->desc_disable = default_disable_desc;
> +	if (!chip->desc_startup)
> +		chip->desc_startup = default_startup_desc;

  This will break all irq_chips which implement enable, disable or
  startup. so the check needs to be:

  	   if (!chip->enable && !chip->desc_enable) 

> +	/*
> +	 * We use chip->disable, when the user provided its own. When
> +	 * we have default_disable set for chip->disable, then we need
> +	 * to use default_shutdown, otherwise the irq line is not
> +	 * disabled on free_irq():
> +	 */
> +	if (!chip->desc_shutdown)
> +		chip->desc_shutdown = chip->desc_disable != default_disable_desc ?
> +			chip->desc_disable : default_shutdown_desc;
> +	if (!chip->desc_end)
> +		chip->desc_end = dummy_irq_chip.desc_end;

  Same  here.

> @@ -295,10 +295,14 @@ void clear_kstat_irqs(struct irq_desc *desc)
>  static void ack_bad(unsigned int irq)

  Why do we keep that function around ?

>  {
>  	struct irq_desc *desc = irq_to_desc(irq);
> -
> -	print_irq_desc(irq, desc);
> +	print_irq_desc(desc);
>  	ack_bad_irq(irq);
>  }

  
>  /*
>   * Generic no controller implementation
> @@ -323,6 +334,13 @@ struct irq_chip no_irq_chip = {
>  	.disable	= noop,
>  	.ack		= ack_bad,
>  	.end		= noop,

  We can remove the old ones right away, can't we ?

> diff --git a/kernel/irq/resend.c b/kernel/irq/resend.c

Yinghai, thanks for doing that. It's halfways reviewable now, but if
we go that way it needs to be split further, really.

But to be honest I have to say that the code churn of that patch
scares me. I expected it to be less horrible.

So I started twisting my brain around a fully automated method of
solving this conversion problem with almost zero risk. It just occured
to me that Julia might be able to help and whip up semantic patch
rules for doing the conversion automagically on a flag day. That would
be one rule and one resulting patch for each of the chip->functions.

So the task would be:

In include/linux/irq.h

  struct irq_chip {
        const char      *name;
- 	unsigned int    startup(unsigned int irq);
+ 	unsigned int    startup(struct irq_desc *desc);

Then finding all irq_chip implementations which set that function
pointer and fix up the function e.g.:

arch/x86/kernel/apic/io_apic.c: 

static struct irq_chip ioapic_chip __read_mostly = {
        .name           = "IO-APIC",
        .startup        = startup_ioapic_irq,

-->

-static unsigned int startup_ioapic_irq(unsigned int irq)
+static unsigned int startup_ioapic_irq(struct irq_desc *desc)
 {
+	unsigned int irq = desc->irq;
        int was_pending = 0;
        unsigned long flags;
        struct irq_cfg *cfg;

        raw_spin_lock_irqsave(&ioapic_lock, flags);
...

To make it simple, we just can put that right after the opening
bracket of the function.

The code which uses the chip functions needs to be changed:

-	desc->chip->fun(irq);
+	desc->chip->fun(desc);

That's mostly in kernel/irq/* but there are some users in arch/*
lowlevel irq handling code as well.

I have not yet checked whether some place set's the function pointers
in the code rather than doing it in the struct definition statically,
but the vast majority should be static initializers.

Julia, can this be done with the semantic patcher magic or are you
already running away screaming ?

Thanks,

	tglx