On Thu, Feb 21, 2008 at 06:39:45AM -0600, Glenn Streiff wrote:
Looking at the patches what you did seems OK.
But regarding "review" I have a different criticism directed at Roland:
This driver should really have gotten some review before being included
in the kernel.
Even a simple checkpatch run finds more than > 250 stylistic errors
(not code bugs but cases where the driver violates the standard code
formatting rules of kernel code).
And I'm not talking about the > 2000 checkpatch warnings that are mostly
about too long lines (which should arguably also be fixed).
And many more issues that could have been foung during a review.
E.g. when you look at 3/8 from this series the code
if (!cm_node)
return -EINVAL;
new_send = kzalloc(sizeof(*new_send), GFP_ATOMIC);
if (!new_send)
return -1;
doesn't look good since the -1 should most likely better be something
like -ENOMEM (I haven't checked whether you can immediately change it
at this specific place).
And these are just comments from someone with zero knowledge about
InfiniBand, but I'd expect InfiniBand-specifig bugs might be found
before they hit users if an InfiniBand maintainer would review the
complete driver.
Note that this is not meant as a criticism against Glenn - it's
normal that submitted code contains bugs, but a code review can help to
cope with this.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
