Re: [ANNOUNCE][RELEASE]: megaraid unified driver version 2.20.0.B1

[Paul Wagland]

Hi all,

On Thu, 2004-04-08 at 01:18, Bagalkote, Sreenivas wrote:
> Hello All,
> 
> We are releasing the megaraid unified driver version 2.20.0.0.
> The source is available at our public ftp site:

OK, I have started to look at this driver, I have come across one
problem, which the attached patch fixes. This patch has been sent
through the list several times and has been accepted into 2.6.5.

If you don't set the module owner, then the module can be removed while
it is in use, with quite disastrous results.

A couple of other comments:

1. The Kconfig.megaraid and Makefile.2.6 files from the alpha release
are not in the beta.

2. In mraid_pci_blk_pool_destroy(), the caller "guarantees that no more
memory from the pool is in use", however, they don't have to guarantee
that pool is not null. Why? In fact, in the code it appears that this
guarantee is also made...

3. I am not sure what the local conventions on this are, but in
megaraid_alloc_cmd_packets() if the first allocation fails then we
return straight away, in all other cases we do a goto fail_alloc_cmds;
This is correct (of course) but personally I would prefer that they all
behave in a consistent fashion, and that the first failed alloc test
also did the goto. Otherwise, there is no need for the
alloc_common_mbox_f local, since whenever you go to fail_alloc_cmds
common_mbox will have been allocated (otherwise we have returned).

4. Also a consistency issue: In megaraid_mbox_mm_cmd() we handle the
deletion of a logical drive specially, by calling
megaraid_mbox_del_logdrv(), which just ensures that the drives are
"quiescent" and then calls megaraid_mbox_internal_command(), otherwise,
we just call megaraid_mbox_internal_command(). My preference, would be
for megaraid_mbox_del_logdrv() to just return when the drives are quiet
and then to call megaraid_mbox_internal_command() as per normal, makes
the flow easier to follow in my opinion. However, when we look at
megaraid_mbox_internal_done() it has the deleting of logical drive logic
inlined. I would prefer either that both functions have the special
logic inlined, or that they both have helper functions to deal with the
special case.

Anyway, hope that this helps,
Paul
--- megaraid.c.orig	2004-04-09 01:07:11.000000000 +0200
+++ megaraid.c	2004-04-09 01:09:38.000000000 +0200
@@ -342,6 +342,7 @@
  */
 #define MRAID_TEMPLATE							\
 {									\
+	.module				= THIS_MODULE,			\
 	.name				= "MegaRAID",			\
 	.proc_name			= "megaraid",			\
 	.info				= megaraid_info,		\