random.c changes for sparse irq_desc are crap

random.c changes for sparse irq_desc are crap

[Matt Mackall]

I just noticed you merged a change that pointlessly converts two
random.c functions into ugly random.h inlines without going through the
maintainer.

I also don't like the look of the newly-introduced sparse variants of
these functions. Failure to find an irq descriptor in
get_timer_rand_state is a BUG_ON should-never-happen sort of condition,
not something to silently ignore. Letting the code try to dereference
NULL is preferred here: we'll actually be able to find and fix the
broken driver that's throwing around meaningless irq vectors.

Throwing away the timer_state pointer in the set_timer_rand_state
function is similarly bogus in addition to being a memory leak.

Please fix this up.

-- 
Mathematics is the supreme nostalgia of our time.

Re: random.c changes for sparse irq_desc are crap

[Yinghai Lu]

Matt Mackall wrote:
> I just noticed you merged a change that pointlessly converts two
> random.c functions into ugly random.h inlines without going through the
> maintainer.
> 
> I also don't like the look of the newly-introduced sparse variants of
> these functions. Failure to find an irq descriptor in
> get_timer_rand_state is a BUG_ON should-never-happen sort of condition,
> not something to silently ignore. Letting the code try to dereference
> NULL is preferred here: we'll actually be able to find and fix the
> broken driver that's throwing around meaningless irq vectors.
> 
> Throwing away the timer_state pointer in the set_timer_rand_state
> function is similarly bogus in addition to being a memory leak.
> 
> Please fix this up.
> 

want something like this?

---
 include/linux/random.h |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Index: linux-2.6/include/linux/random.h
===================================================================
--- linux-2.6.orig/include/linux/random.h
+++ linux-2.6/include/linux/random.h
@@ -75,8 +75,7 @@ static inline struct timer_rand_state *g
 
 	desc = irq_to_desc(irq);
 
-	if (!desc)
-		return NULL;
+	BUG_ON(!desc);
 
 	return desc->timer_rand_state;
 }
@@ -87,8 +86,7 @@ static inline void set_timer_rand_state(
 
 	desc = irq_to_desc(irq);
 
-	if (!desc)
-		return;
+	BUG_ON(!desc);
 
 	desc->timer_rand_state = state;
 }

Re: random.c changes for sparse irq_desc are crap

[Matt Mackall]

On Wed, 2008-12-31 at 15:07 -0800, Yinghai Lu wrote:
> Matt Mackall wrote:
> > I just noticed you merged a change that pointlessly converts two
> > random.c functions into ugly random.h inlines without going through the
> > maintainer.
> > 
> > I also don't like the look of the newly-introduced sparse variants of
> > these functions. Failure to find an irq descriptor in
> > get_timer_rand_state is a BUG_ON should-never-happen sort of condition,
> > not something to silently ignore. Letting the code try to dereference
> > NULL is preferred here: we'll actually be able to find and fix the
> > broken driver that's throwing around meaningless irq vectors.
> > 
> > Throwing away the timer_state pointer in the set_timer_rand_state
> > function is similarly bogus in addition to being a memory leak.
> > 
> > Please fix this up.
> > 
> 
> want something like this?

Not quite.

First, please turn these back into normal functions in random.c.
Inlining functions is generally discouraged these days unless you have a
good reason and numbers to back it up.

Second, as I tried to explain above, BUG_ON(!desc) is doing very little
that the subsequent desc->timer_rand_state wouldn't already do
(generating a traceback). In cases where dereferencing NULL should never
happen, it's preferable to not add the extra check and just let the oops
happen. We'll still get a backtrace if anything ever goes wrong, but we
won't have wasted any code space or CPU cycles when it doesn't.

-- 
Mathematics is the supreme nostalgia of our time.

Re: random.c changes for sparse irq_desc are crap

[Yinghai Lu]

On Wed, Dec 31, 2008 at 3:40 PM, Matt Mackall <mpm@selenic.com> wrote:
> On Wed, 2008-12-31 at 15:07 -0800, Yinghai Lu wrote:
>> Matt Mackall wrote:
>> > I just noticed you merged a change that pointlessly converts two
>> > random.c functions into ugly random.h inlines without going through the
>> > maintainer.
>> >
>> > I also don't like the look of the newly-introduced sparse variants of
>> > these functions. Failure to find an irq descriptor in
>> > get_timer_rand_state is a BUG_ON should-never-happen sort of condition,
>> > not something to silently ignore. Letting the code try to dereference
>> > NULL is preferred here: we'll actually be able to find and fix the
>> > broken driver that's throwing around meaningless irq vectors.
>> >
>> > Throwing away the timer_state pointer in the set_timer_rand_state
>> > function is similarly bogus in addition to being a memory leak.
>> >
>> > Please fix this up.
>> >
>>
>> want something like this?
>
> Not quite.
>
> First, please turn these back into normal functions in random.c.
> Inlining functions is generally discouraged these days unless you have a
> good reason and numbers to back it up.

Ingo wanted to hide that #ifdef to .h

>
> Second, as I tried to explain above, BUG_ON(!desc) is doing very little
> that the subsequent desc->timer_rand_state wouldn't already do
> (generating a traceback). In cases where dereferencing NULL should never
> happen, it's preferable to not add the extra check and just let the oops
> happen. We'll still get a backtrace if anything ever goes wrong, but we
> won't have wasted any code space or CPU cycles when it doesn't.

ok

YH

Re: random.c changes for sparse irq_desc are crap

[Matt Mackall]

On Wed, 2008-12-31 at 16:14 -0800, Yinghai Lu wrote:
> On Wed, Dec 31, 2008 at 3:40 PM, Matt Mackall <mpm@selenic.com> wrote:
> > On Wed, 2008-12-31 at 15:07 -0800, Yinghai Lu wrote:
> >> Matt Mackall wrote:
> >> > I just noticed you merged a change that pointlessly converts two
> >> > random.c functions into ugly random.h inlines without going through the
> >> > maintainer.
> >> >
> >> > I also don't like the look of the newly-introduced sparse variants of
> >> > these functions. Failure to find an irq descriptor in
> >> > get_timer_rand_state is a BUG_ON should-never-happen sort of condition,
> >> > not something to silently ignore. Letting the code try to dereference
> >> > NULL is preferred here: we'll actually be able to find and fix the
> >> > broken driver that's throwing around meaningless irq vectors.
> >> >
> >> > Throwing away the timer_state pointer in the set_timer_rand_state
> >> > function is similarly bogus in addition to being a memory leak.
> >> >
> >> > Please fix this up.
> >> >
> >>
> >> want something like this?
> >
> > Not quite.
> >
> > First, please turn these back into normal functions in random.c.
> > Inlining functions is generally discouraged these days unless you have a
> > good reason and numbers to back it up.
> 
> Ingo wanted to hide that #ifdef to .h

Ahh. I think that makes sense in some situations, but I'd prefer not to
do that here. Some headers are actually meant to be more readable than
the corresponding .c file. Also, we still end up with an ifdef in the .c
file so we're not actually winning.

-- 
Mathematics is the supreme nostalgia of our time.

Re: random.c changes for sparse irq_desc are crap

[Ingo Molnar]

* Matt Mackall <mpm@selenic.com> wrote:

> On Wed, 2008-12-31 at 16:14 -0800, Yinghai Lu wrote:
> > On Wed, Dec 31, 2008 at 3:40 PM, Matt Mackall <mpm@selenic.com> wrote:
> > > On Wed, 2008-12-31 at 15:07 -0800, Yinghai Lu wrote:
> > >> Matt Mackall wrote:
> > >> > I just noticed you merged a change that pointlessly converts two
> > >> > random.c functions into ugly random.h inlines without going through the
> > >> > maintainer.
> > >> >
> > >> > I also don't like the look of the newly-introduced sparse variants of
> > >> > these functions. Failure to find an irq descriptor in
> > >> > get_timer_rand_state is a BUG_ON should-never-happen sort of condition,
> > >> > not something to silently ignore. Letting the code try to dereference
> > >> > NULL is preferred here: we'll actually be able to find and fix the
> > >> > broken driver that's throwing around meaningless irq vectors.
> > >> >
> > >> > Throwing away the timer_state pointer in the set_timer_rand_state
> > >> > function is similarly bogus in addition to being a memory leak.
> > >> >
> > >> > Please fix this up.
> > >> >
> > >>
> > >> want something like this?
> > >
> > > Not quite.
> > >
> > > First, please turn these back into normal functions in random.c.
> > > Inlining functions is generally discouraged these days unless you have a
> > > good reason and numbers to back it up.
> > 
> > Ingo wanted to hide that #ifdef to .h
> 
> Ahh. I think that makes sense in some situations, but I'd prefer not to 
> do that here. Some headers are actually meant to be more readable than 
> the corresponding .c file. Also, we still end up with an ifdef in the .c 
> file so we're not actually winning.

Yinghai - i think Matt is right and there are a number of things we could 
do to improve cleanliness here. (Matt, sorry about this - we'll quickly 
fix it.)

The most important thing to observe is that the irq_desc->timer_rand_state 
cleanup you did should be made unconditional, and should not depend on 
SPARSE_IRQ. That will eliminate most of the #ifdefs, and it makes the 
random.c and random.h impact rather straightforward.

Another cleanliness detail is that these types of checks:

#ifndef CONFIG_SPARSE_IRQ
        if (irq >= nr_irqs)
                return;
#endif

can go away completely. They were needed from the sparseirq data model 
that had a dynamic array size - but that is not so anymore, so we can 
remove these complications.

Agreed?

	Ingo

[PATCH] sparseirq: move set/get_timer_rand_state back to .c

[Yinghai Lu]

Impact: cleanup

those two functions only used in that .c

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/char/random.c  |   40 +++++++++++++++++++++++++++++++++------
 include/linux/random.h |   50 -------------------------------------------------
 2 files changed, 34 insertions(+), 56 deletions(-)

Index: linux-2.6/drivers/char/random.c
===================================================================
--- linux-2.6.orig/drivers/char/random.c
+++ linux-2.6/drivers/char/random.c
@@ -559,7 +559,40 @@ struct timer_rand_state {
 };
 
 #ifndef CONFIG_SPARSE_IRQ
-struct timer_rand_state *irq_timer_state[NR_IRQS];
+
+static struct timer_rand_state *irq_timer_state[NR_IRQS];
+
+static struct timer_rand_state *get_timer_rand_state(unsigned int irq)
+{
+	return irq_timer_state[irq];
+}
+
+static void set_timer_rand_state(unsigned int irq,
+				 struct timer_rand_state *state)
+{
+	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);
+
+	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);
+
+	desc->timer_rand_state = state;
+}
 #endif
 
 static struct timer_rand_state input_timer_state;
@@ -919,11 +952,6 @@ void rand_initialize_irq(int irq)
 {
 	struct timer_rand_state *state;
 
-#ifndef CONFIG_SPARSE_IRQ
-	if (irq >= nr_irqs)
-		return;
-#endif
-
 	state = get_timer_rand_state(irq);
 
 	if (state)
Index: linux-2.6/include/linux/random.h
===================================================================
--- linux-2.6.orig/include/linux/random.h
+++ linux-2.6/include/linux/random.h
@@ -45,56 +45,6 @@ struct rand_pool_info {
 
 extern void rand_initialize_irq(int irq);
 
-struct timer_rand_state;
-#ifndef CONFIG_SPARSE_IRQ
-
-extern struct timer_rand_state *irq_timer_state[];
-
-static inline struct timer_rand_state *get_timer_rand_state(unsigned int irq)
-{
-	if (irq >= nr_irqs)
-		return NULL;
-
-	return irq_timer_state[irq];
-}
-
-static inline void set_timer_rand_state(unsigned int irq, struct timer_rand_state *state)
-{
-	if (irq >= nr_irqs)
-		return;
-
-	irq_timer_state[irq] = state;
-}
-
-#else
-
-#include <linux/irq.h>
-static inline 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 inline 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
-
-
 extern void add_input_randomness(unsigned int type, unsigned int code,
 				 unsigned int value);
 extern void add_interrupt_randomness(int irq);