linux-kernel - Re: [git patch] free

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: <m1r6cvgpw2.fsf@frodo.ebiederm.org>
Date:	Thu, 24 Apr 2008 09:48:45 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jeff Garzik <jeff@...zik.org>,
	Rene Herman <rene.herman@...access.nl>,
	Adrian Bunk <bunk@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>, rmk@....linux.org.uk,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>
Subject: Re: [git patch] free_irq() fixes

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Thu, 24 Apr 2008, Jeff Garzik wrote:
>> 
>> However, it does not follow that an int is what _must_ be passed around.  We
>> already have design patterns like
>> 
>> 	cookie_pointer = ioremap(raw bus resource)
>> 
>> Not that I am the one pushing for that, just noting.
>
> I do agree that we could use something more type-safe.
>
> So a "pointer" to a structure that doesn't actually exist would be fine 
> and would give us some C type checking. 
>
> But then you'd have to have some way to "printk" the information, which is 
> a very common requirement (and the printk still needs to be a number, 
> because you want to match up 'dmesg' output with the '/proc/interrupts' 
> file etc).
>
> That, in turn, would effectively force a whole new function, and then 
> you'd have things like
>
> 	printk(.. irq %d .., irq_number(irqcookie) ..)
>
> which while not ugly isn't really all that clean either. And I guarantee 
> that people would misuse that "irq_number(cookie)" exactly in the same 
> ways they'd misuse "irq" (ie not very much).
>
> Quite frankly, I'd much prefer a
>
> 	typedef int __bitwise irq_t;

Well that had better be.

	typedef unsigned int __bitwise irq_t;

Or else we have changed the historical type.

The one real advantage of a pointer cookie type is that we can remove
the confusion of dealing with irq 0.  Which is a valid irq but no
driver can use it, or our if (!irq) tests fail.

If irq 0 does not have a NULL cookie all of those problems disappear.

> and then we can use sparse to do this testing, without breaking any 
> existing use at all (because it will still be an "int" to gcc, but sparse 
> will make "irq_t" a type of its own and make sure that people pass it 
> around as such and not do arithmetic ops on it etc).

My only real concern is that it doesn't get too hard for the
implementation to go from the irq cookie to the struct irq_desc when
we kill the arrays.

Right now the irq number is very much an array index and that
property is limiting.

Even if we never export them to drivers we will need to implement
in genirq functions like:

int __must_check irq_request(struct irq_desc *irq, irq_handler_t handler,
	 unsigned long irqflags, const char *devname, void *devid);
        	
int __must_check request_irq(unsigned int irq, irq_handler_t handler,
	unsigned long irqflags, const char *devname, void *devid)
{
        return irq_request(cookie_to_desc(irq), handler, irqflags, devname, devid);
}

Or at the very least do the mapping from cookie to irq_desc at the
start of all of the genirq functions.  One valid implementation of
that cookie to desc will be the current array lookup.  But for x86
we need something less limiting.

>> > EVERYTHING else would be architecture-specific. And that is exactly what we
>> > do not want. EVER. 
>> 
>> Not true -- you have metadata/OOB data like MSI messages, where you are passed
>> a value from the PCI hardware in the PCI message, not just an "interrupt
>> asserted" condition.  Or s/value/values/ if you enable PCI MSI's multiple
>> message support.
>
> The point is, MSI *is* architecture-specific. In fact, it's even 
> motherboard-specific, in that you are going to have (for the forseeable 
> future) drivers that have to work with or witgout MSI even on the same 
> architecture.

And on x86 at least the hardware maps the MSI write into an interrupt.
So there is not an opportunity to get any metdata/OOB data from the
MSI message.  Instead we just potentially get a boatload more irq
sources.  Which is one of the things making a static NR_IRQS painful.

To be safe we have to make NR_IRQS 10x+ or so bigger then people use
today.  Just in case they decide to plug in some really irq hungry
cards.

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