* Re: [ANNOUNCE][RELEASE]: megaraid unified driver version 2.20.0.B1
2004-04-07 23:18 [ANNOUNCE][RELEASE]: megaraid unified driver version 2.20.0.B1 Bagalkote, Sreenivas
@ 2004-04-08 23:56 ` Paul Wagland
2004-04-16 12:18 ` Christoph Hellwig
1 sibling, 0 replies; 3+ messages in thread
From: Paul Wagland @ 2004-04-08 23:56 UTC (permalink / raw)
To: Bagalkote, Sreenivas
Cc: 'jgarzik@pobox.com', 'Matt_Domsch@dell.com',
'James.Bottomley@SteelEye.com',
'arjanv@redhat.com', 'linux-scsi@vger.kernel.org',
'linux-kernel@vger.kernel.org'
[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]
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
[-- Attachment #2: megaraid.b1.1.patch --]
[-- Type: text/x-patch, Size: 302 bytes --]
--- 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, \
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [ANNOUNCE][RELEASE]: megaraid unified driver version 2.20.0.B1
2004-04-07 23:18 [ANNOUNCE][RELEASE]: megaraid unified driver version 2.20.0.B1 Bagalkote, Sreenivas
2004-04-08 23:56 ` Paul Wagland
@ 2004-04-16 12:18 ` Christoph Hellwig
1 sibling, 0 replies; 3+ messages in thread
From: Christoph Hellwig @ 2004-04-16 12:18 UTC (permalink / raw)
To: Bagalkote, Sreenivas
Cc: 'jgarzik@pobox.com', 'Matt_Domsch@dell.com',
'paul@kungfoocoder.org',
'James.Bottomley@SteelEye.com',
'arjanv@redhat.com', 'linux-scsi@vger.kernel.org',
'linux-kernel@vger.kernel.org'
kdep.h:
- mraid_scsi_host_alloc/mraid_scsi_host_dealloc should go away, just
define scsi_host_alloc/scsi_host_put wrappers for 2.4.
- mraid_scsi_set_pdev should go away, it's not needed in 2.6 at all
because scsi_add_host does all the work and for 2.4 just use
scsi_set_pci_device directly.
- all the SCP2FOO defines should go away, the 2.6 variants work for 2.4
aswell
- mraid_set_host_lock should go, just use scsi_assign_lock and define
it for 2.4.
megaraid_clib.c:
- why do you need the scb pool managment code at all? You can dynamically
allocate scbs in ->queuecommand
- can you explain the need for all the mraid_pci_blk_pool? I.e. why the
generic dma pool routines don't work for megaraid
all files:
- please avoid using scsi.h and hosts.h from drivers/scsi in favour of
the include/scsi/ headers, especially get rid of all the Scsi_Foo
typedefs
^ permalink raw reply [flat|nested] 3+ messages in thread