linux-kernel - Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq

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: <alpine.LFD.2.00.1003220950570.3147@localhost.localdomain>
Date:	Mon, 22 Mar 2010 11:19:29 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Yinghai Lu <yinghai@...nel.org>
cc:	Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	linux-kernel@...r.kernel.org,
	Ian Campbell <ian.campbell@...rix.com>,
	Ingo Molnar <mingo@...hat.com>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>, x86@...nel.org,
	linuxppc-dev@...abs.org, Rusty Russell <rusty@...tcorp.com.au>,
	lguest@...abs.org, Paul Mundt <lethal@...ux-sh.org>,
	linux-sh@...r.kernel.org
Subject: Re: [PATCH 01/10] irq: move some interrupt arch_* functions into
 struct irq_chip.

On Sun, 21 Mar 2010, Yinghai Lu wrote:

> From: Ian Campbell <ian.campbell@...rix.com>
> 
> Move arch_init_copy_chip_data and arch_free_chip_data into function
> pointers in struct irq_chip since they operate on irq_desc->chip_data.

Not sure about that. These functions are solely used by x86 and there
is really no need to generalize them. The problem you try to solve is
x86/xen specific and can be solved by x86_platform_ops as well w/o
adding extra function pointers to irq_chip.

> arch_init_chip_data cannot be moved into struct irq_chip because
> irq_desc->chip is not known at the time the irq_desc is setup. Instead
> rename arch_init_chip_data to arch_init_irq_desc for PowerPC, the
> only other user, whose usage better matches the new name.
> 
> To replace the x86 arch_init_chip_data functionality
> irq_to_desc_alloc_node now takes a pointer to a function to allocate
> the chip data. This is necessary to ensure the allocation happens
> under the correct locking at the core level. On PowerPC and SH

Err, that argument is totally bogus. The calling convention of
irq_to_desc_alloc_node and arch_init_chip_data/arch_init_irq_desc is
still the same. It does not explain why the heck we need that function
pointer at all.

AFAICT the function pointer to irq_to_desc_alloc_node is completely
pointless. It just solves a Xen/x86 specific problem which can be
solved by using x86_platform_ops and keeps the churn x86 internal.

> architectures (the other users of irq_to_desc_alloc_node) pass in NULL
> which retains existing chip_data behaviour.
 
> I've retained the chip_data behaviour for uv_irq although it isn't
> clear to me if these interrupt types support migration or how closely
> related to the APIC modes they really are. If it weren't for this the
> x86_{init,copy,free}_chip_data functions could be static to
> io_apic.c.
> 
> I've tested by booting on an 64 bit x86 system with sparse IRQ enabled
> and 32 bit without, but it's not clear to me what actions I need to
> take to actually exercise some of these code paths.
> 
> -v4: yinghai add irq_to_desc_alloc_node_x...

Aside of the general objection against this, please use descriptive
function names and do not distinguish functions by adding random
characters which tell us absolutely nothing about the purpose.

Thanks,

	tglx
--
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/