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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20081128163456.GB10487@elte.hu>
Date:	Fri, 28 Nov 2008 17:34:56 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] irq: sparseirq enabling v3


* Yinghai Lu <yinghai@...nel.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/