Re: [PATCH 2/2] module: fix bne2 "gave up waiting for init of modulelibcrc32c"

[Linus Torvalds]

On Wed, 2 Jun 2010, Rusty Russell wrote:
> 
> OK, this might be worthwhile.  Take the very first mail you sent:
> 
>    Hmm. That does seem to be buggy.  We can't just drop and re-take the lock: 
>    that may make sense internally as far as resolve_symbol() itself is 
>    concerned, but the caller will its own local variables, and some of those 
>    will no longer be valid if the lock was dropped. 
> 
> The implication here is that that I don't know locking, and that this was
> done without thought or care.  In fact, I did worry about it and decided it
> was less risky than a locking reduction given my limited cycles (indeed,
> it has been slightly).

Well, part of the context here is that the commit had been bisected as 
being buggy. It turns out the bug was a different issue, but at the same 
time, it very much looked like the locking was simply known a-priori to be 
buggy. No?

And I do agree with the notion that it might have been a "simpler hack for 
a quick fix", and potentially less risky (due to being more targeted to 
the particular problem), when it then causes oopses, the default 
explanation is that the dubious locking trick really was broken. No?

>    That commit also changes the return value semantics of "use_module()", 
>    which is an exported interface where the only users seem to be 
>    out-of-kernel (the only in-kernel use is in kernel/module.c itself). That 
>    seems like a really really bad idea too.
> 
> Again with the implication that this was done without consideration.

No. The implication was that it's wrong to do and a bad idea. Anything 
else is just you reading things into it.

It's an exported interface, and you changed it with no real reason. 
Whether you then talked to users or not is immaterial.

You could easily have just changed the _internal_ thing, and exported the 
unchanged interface.

Even your second version is just very confused. It renames it, but then 
exports the renamed version. What does that help? It still means that the 
external module - for no good reason - needs to have basically a 
source-level version number check.

Why?

It's still unclear to me. No reason for that exported interface change 
seems to exist.

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