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

[Ingo Molnar <mingo@elte.hu>]

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

> +void __init arch_early_irq_init(void)
>  {
> -	return irq < nr_irqs ? irq_cfgx + irq : NULL;
> +	struct irq_cfg *cfg;
> +	struct irq_desc *desc;
> +	int count;
> +	int i;
> +#ifdef CONFIG_SPARSE_IRQ
> +	int count_desc = NR_IRQS_LEGACY;
> +#else
> +	int count_desc = NR_IRQS;
> +#endif

could this be hidden in a .h file by creating some sort of nr_boot_irqs() 
inline, or a NR_BOOT_IRQS define?

> Index: linux-2.6/drivers/char/random.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/random.c
> +++ linux-2.6/drivers/char/random.c
> @@ -558,6 +558,8 @@ struct timer_rand_state {
>  	unsigned dont_count_entropy:1;
>  };
>  
> +#ifndef CONFIG_SPARSE_IRQ
> +
>  static struct timer_rand_state *irq_timer_state[NR_IRQS];
>  
>  static struct timer_rand_state *get_timer_rand_state(unsigned int irq)
> @@ -576,6 +578,33 @@ static void set_timer_rand_state(unsigne
>  	irq_timer_state[irq] = state;
>  }
>  
> +#else
> +
> +static struct timer_rand_state *get_timer_rand_state(unsigned int irq)
> +{
> +	struct irq_desc *desc;
> +
> +	desc = irq_to_desc(irq);
> +
> +	if (!desc)
> +		return NULL;
> +
> +	return desc->timer_rand_state;
> +}
> +
> +static void set_timer_rand_state(unsigned int irq, struct timer_rand_state *state)
> +{
> +	struct irq_desc *desc;
> +
> +	desc = irq_to_desc(irq);
> +
> +	if (!desc)
> +		return;
> +
> +	desc->timer_rand_state = state;
> +}
> +#endif

i'd suggest to move this into a .h file.

> +++ linux-2.6/fs/proc/stat.c
> @@ -27,6 +27,9 @@ static int show_stat(struct seq_file *p,
>  	u64 sum = 0;
>  	struct timespec boottime;
>  	unsigned int per_irq_sum;
> +#ifdef CONFIG_GENERIC_HARDIRQS
> +	struct irq_desc *desc;
> +#endif

Why is this define needed? If it's about a build warning, you can add 
something like this to defines:

 (void)(param)

to make unused parameters used as well.

> +#ifdef CONFIG_SPARSE_IRQ
> +		seq_printf(p, " %d:%u", j, per_irq_sum);
> +#else
>  		seq_printf(p, " %u", per_irq_sum);
> -	}
> +#endif

doesnt this change the /proc ABI ? A couple of tools would break. I think 
the right approach is to go from 0 to NR_IRQS-1 and print zeroes for NULL 
descs. I.e. a natural extension of the current scheme.

> +++ linux-2.6/fs/proc/interrupts.c
> @@ -8,6 +8,23 @@
>  /*
>   * /proc/interrupts
>   */
> +#ifdef CONFIG_SPARSE_IRQ
> +static void *int_seq_start(struct seq_file *f, loff_t *pos)
> +{
> +	rcu_read_lock();
> +	return seq_list_start(&sparse_irqs_head, *pos);

is this rcu-safe approach still needed?

> ===================================================================
> --- linux-2.6.orig/include/linux/interrupt.h
> +++ linux-2.6/include/linux/interrupt.h
> @@ -18,6 +18,8 @@
>  #include <asm/ptrace.h>
>  #include <asm/system.h>
>  
> +extern int nr_irqs;

isnt this obsolete now?

> Index: linux-2.6/arch/x86/kernel/irq.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/kernel/irq.c
> +++ linux-2.6/arch/x86/kernel/irq.c
> @@ -99,25 +99,37 @@ static int show_other_interrupts(struct
>  int show_interrupts(struct seq_file *p, void *v)
>  {
>  	unsigned long flags, any_count = 0;
> -	int i = *(loff_t *) v, j;
> +	int i, j;
>  	struct irqaction *action;
>  	struct irq_desc *desc;
> +	int head = 0;
>  
> +#ifdef CONFIG_SPARSE_IRQ
> +	desc = list_entry(v, struct irq_desc, list);
> +	i = desc->irq;
> +	if (&desc->list == sparse_irqs_head.next)
> +		head = 1;
> +#else
> +	i = *(loff_t *) v;
>  	if (i > nr_irqs)
>  		return 0;
>  
>  	if (i == nr_irqs)
>  		return show_other_interrupts(p);
> +	if (i == 0)
> +		head = 1;
> +
> +	desc = irq_to_desc(i);
> +#endif

i dont think this has to change? We have an array of pointers, and we 
should extend the current loops by skipping over NULL entries.

	Ingo