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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
