Re: [PATCH] irqchip: Add support for ARMv7-M's NVIC

[Russell King - ARM Linux]

On Tue, Mar 12, 2013 at 08:27:02PM +0100, Uwe Kleine-König wrote:
> On Tue, Mar 12, 2013 at 04:01:01PM +0000, Russell King - ARM Linux wrote:
> > On Tue, Mar 12, 2013 at 04:54:33PM +0100, Uwe Kleine-König wrote:
> > > +#include <asm/irq.h>
> > > +#include <asm/io.h>
> > 
> > linux/io.h
> > 
> > > +	unsigned int irqs, i, irq_base;
> > > +
> > > +	irq_base = irq_alloc_descs(-1, 16, irqs - 16, numa_node_id());
> > > +	if (IS_ERR_VALUE(irq_base)) {
> > 
> > Erm... irq_alloc_descs() returns a negative number on error.
> > 
> > 	if ((int)irq_base < 0)
> > 
> > or make irq_base an int, and use:
> > 
> > 	if (irq_base < 0)
> Just for me: So the check using IS_ERR_VALUE is as wrong as the other
> occurences in arch/arm that you just kicked out or is it just ugly?

See my recent patch removing all but one.

What we're suffering from here is a mentality problem - one which seems
to be basically this:

	If a macro exists which looks like it does the job I need,
	I must use it.  I won't look at the function and check its
	range of values that it returns, I'll just use it and hope
	it's the right thing.

The IS_ERR_VALUE() patch and my IS_ERR_OR_NULL() patches, I've spent on
each one less than a minute, greping, reading the function, checking its
range of return values, sometimes longer if I need to look at other
functions, and worked out what the valid range of return values are.

However, the general pattern in the kernel is this:

	For any function that returns an int, values of success will
	be positive.  Values indicating errors will be negative.

There are very few int-returning functions which violate that.  There
is one big, well known exception, and that's in the mmap() stuff,
where there's a need to return valid values in the range (0..TASK_SIZE)
but differentiate them from -ve errnos.  This is where IS_ERR_VALUE()
came from, and why it was created.  See 07ab67c8d0d7c (Fix
get_unmapped_area sanity tests).


Today, it seems that IS_ERR_VALUE() is now being used just as a subsitute
for testing for < 0... and it needs to stop.  See above - unless there's
a *good* reason, treat +ve values as success, -ve values as failure from
functions returning int.  Always design functions in the kernel like that.
Again - unless there's a *good* reason like needing to return 0..TASK_SIZE.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/