RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[Mukker, Atul]

Christoph,

I have put the revised driver on the ftp server

ftp://ftp.lsil.com/pub/linux-megaraid/drivers/version-2.20.0.1/

This driver incorporates almost all your suggestions. I think it's high
time, the driver makes into the mainstream kernel.

Please see my other responses below


> Umm, of course.  But the default statements are usually only 
> for things
> which are nessecary to get a working ssystem.  E.g. the much 
> more widely
> used Intel PIIX or VIA IDE controller drivers found on just 
> about every
> motherboard don't have a default statement either.
> 
Fixed


> I didn't actually complain about macros, but that the include 
> files aren't
> in the individual files.
The low level drivers (megaraid_mbox.c) includes their main header file
(megaraid_mbox.h). The main header file includes all the relevant header
files

> 
> > >  - MRAID_GET_DEVICE_MAP is far too large for a macro, 
> please make it a
> > >    function call or opencode it in the callers
> > 
> > Still needs to be done.  There's only one caller anyway, so 
> no need for
> > the macro
> > AM> This code base uses at only one place, but there is 
> another low level
> > driver if offing which uses the same definitions. So this 
> should stay.
> 
> then please convert it to an inline function.
Then we would need to have a inline function in the header file itself (no
common C library anymore), which is a kludge. If the comments are ignored,
it isn't a very big macro after all.

> Sorry, my apologies.  I still looked huge beause you have 
> each ID spread
> over multiple lines.  
I am hesitant to club all on one line, since many entries would violate our
80-column rule

> 
> > >  - alloc_adapter_f/init_mbox_f in megaraid_probe_one and 
> use proper gotos
> > >    as I did in the old megaraid driver
> > 
> > still needs to be done.
FIXED

> 
> > >  - the detach path is bogus.  You need to do 
> scshi_remove_host first
FIXED

> 
> > >  - your locking looks far to fine-grained to me.  Did you 
> benchmark it?
Megaraid is not the only driver, which has fine-grained locks in 2.6.7. I
will anyway provide the driver comparisons

Re: [ANNOUNCE]: megaraid driver version 2.20.0.1

['Christoph Hellwig']

On Thu, Jun 24, 2004 at 09:07:48PM -0400, Mukker, Atul wrote:
> Christoph,
> 
> I have put the revised driver on the ftp server
> 
> ftp://ftp.lsil.com/pub/linux-megaraid/drivers/version-2.20.0.1/
> 
> This driver incorporates almost all your suggestions. I think it's high
> time, the driver makes into the mainstream kernel.

Oh well, let's include it and I'll fix up some minor cruft later on.
Note that changes to drivers/pci/pci.ids should be sent separately to
pci-ids@ucw.cz (see http://pciids.sourceforge.net/ for defails)

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[Mukker, Atul]

> Note that changes to drivers/pci/pci.ids should be sent separately to
> pci-ids@ucw.cz (see http://pciids.sourceforge.net/ for defails)
Which I already did sometime back.

Thanks
-Atul

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[Mukker, Atul]

A bug was discovered in the driver unload path, causing kernel panic. The
new driver will be available in the FTP site soon. The patch with the fix is
inlined.




diff -u 2.20.0.1/ChangeLog.megaraid 2.20.1.0/ChangeLog.megaraid
--- 2.20.0.1/ChangeLog.megaraid	2004-06-24 20:59:46.000000000 -0400
+++ 2.20.1.0/ChangeLog.megaraid	2004-06-25 20:58:56.000000000 -0400
@@ -1,3 +1,11 @@
+Release Date	: Fri Jun 25 18:58:43 EDT 2004 - Atul Mukker
<atulm@lsil.com>
+Current Version	: 2.20.1.0
+Older Version	: megaraid 2.20.0.1
+
+i.	Stale list pointer in adapter causes kernel panic when module
+	megaraid_mbox is unloaded
+
+
 Release Date	: Thu Jun 24 20:37:11 EDT 2004 - Atul Mukker
<atulm@lsil.com>
 Current Version	: 2.20.0.1
 Older Version	: megaraid 2.20.0.00
@@ -13,6 +21,7 @@
 iv.	scsi_host_put(), do just before completing HBA shutdown.
 
 
+
 Release Date	: Mon Jun 21 19:53:54 EDT 2004 - Atul Mukker
<atulm@lsil.com>
 Current Version	: 2.20.0.0
 Older Version	: megaraid 2.20.0.rc2 and 2.00.3
diff -u 2.20.0.1/mega_common.h 2.20.1.0/mega_common.h
--- 2.20.0.1/mega_common.h	2004-06-24 20:59:46.000000000 -0400
+++ 2.20.1.0/mega_common.h	2004-06-25 20:58:57.000000000 -0400
@@ -113,7 +113,6 @@
  * @param max_channel		: maximum channel number supported -
inclusive
  * @param max_target		: max target supported - inclusive
  * @param max_lun		: max lun supported - inclusive
- * @param list			: list of megaraid host structures
  * @param unique_id		: unique identifier for each adapter
  * @param irq			: IRQ for this adapter
  * @param ito			: internal timeout value, (-1) means no
timeout
@@ -171,7 +170,6 @@
 	uint16_t		max_target;
 	uint8_t			max_lun;
 
-	struct list_head	list;
 	uint32_t		unique_id;
 	uint8_t			irq;
 	uint8_t			ito;
diff -u 2.20.0.1/megaraid_mbox.c 2.20.1.0/megaraid_mbox.c
--- 2.20.0.1/megaraid_mbox.c	2004-06-24 20:59:46.000000000 -0400
+++ 2.20.1.0/megaraid_mbox.c	2004-06-25 20:58:57.000000000 -0400
@@ -10,7 +10,7 @@
  *	   2 of the License, or (at your option) any later version.
  *
  * FILE		: megaraid.c
- * Version	: v2.20.0.1 (June 24 2004)
+ * Version	: v2.20.1.0 (June 25 2004)
  *
  * Authors:
  * 	Atul Mukker		<Atul.Mukker@lsil.com>
@@ -683,9 +683,6 @@
 	// pending with us?
 	atomic_set(&adapter->being_detached, 1);
 
-	// remove the adapter from the global list of our controllers
-	list_del_init(&adapter->list);
-
 	// detach from the IO sub-system
 	megaraid_io_detach(adapter);
 
diff -u 2.20.0.1/megaraid_mbox.h 2.20.1.0/megaraid_mbox.h
--- 2.20.0.1/megaraid_mbox.h	2004-06-24 20:59:46.000000000 -0400
+++ 2.20.1.0/megaraid_mbox.h	2004-06-25 20:58:58.000000000 -0400
@@ -21,8 +21,8 @@
 #include "megaraid_ioctl.h"
 
 
-#define MEGARAID_VERSION	"2.20.0.1"
-#define MEGARAID_EXT_VERSION	"(Release Date: Thu Jun 24 20:37:11 EDT
2004)"
+#define MEGARAID_VERSION	"2.20.1.0"
+#define MEGARAID_EXT_VERSION	"(Release Date: Fri Jun 25 18:58:43 EDT
2004)"
 
 
 /*


> -----Original Message-----
> From: 'Christoph Hellwig' [mailto:hch@infradead.org]
> Sent: Friday, June 25, 2004 4:14 AM
> To: Mukker, Atul
> Cc: 'Christoph Hellwig'; 'linux-scsi@vger.kernel.org'
> Subject: Re: [ANNOUNCE]: megaraid driver version 2.20.0.1
> 
> 
> On Thu, Jun 24, 2004 at 09:07:48PM -0400, Mukker, Atul wrote:
> > Christoph,
> > 
> > I have put the revised driver on the ftp server
> > 
> > ftp://ftp.lsil.com/pub/linux-megaraid/drivers/version-2.20.0.1/
> > 
> > This driver incorporates almost all your suggestions. I 
> think it's high
> > time, the driver makes into the mainstream kernel.
> 
> Oh well, let's include it and I'll fix up some minor cruft later on.
> Note that changes to drivers/pci/pci.ids should be sent separately to
> pci-ids@ucw.cz (see http://pciids.sourceforge.net/ for defails)
> 

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[James Bottomley]

On Fri, 2004-06-25 at 19:55, Mukker, Atul wrote:
> A bug was discovered in the driver unload path, causing kernel panic. The
> new driver will be available in the FTP site soon. The patch with the fix is
> inlined.

OK, I think I'm looking at the latest one now.

The biggest structural issue still seems to be the pending queue.  It
looks to me like the driver could be nicely simplified if you got rid of
it and simply relied on returning SCSI_MLQUEUE_HOST_BUSY from
queeucommand().  You'd still get the queue restart almost from the ISR
like you want.

My other main gripe is things like this:

+	// wait for maximum 1 second for status to post
+	for (i = 0; i < 40000; i++) {
+		if (mbox->numstatus != 0xFF) break;
+		udelay(25); yield();
+	}


which litter the driver.  Use of yield() in drivers is deprecated.   I
suspect what you want is a minimum delay to wait if the relevant bit is
not set 

Something like

if (mbox->numstatus != 0xFF)
	goto out;
udelay(25);
for (i = 0; mbox->numstatus != 0xFF && i < 1000; i++)
	msleep(1);
out:

which will give initially 25us to see if the box becomes ready and then
begin yeilding the CPU.

James

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[Mukker, Atul]

> The biggest structural issue still seems to be the pending queue.  It
> looks to me like the driver could be nicely simplified if you 
> got rid of
> it and simply relied on returning SCSI_MLQUEUE_HOST_BUSY from
> queeucommand().  You'd still get the queue restart almost from the ISR
This is a very good suggestion and I would be more than willing to include
in the driver code. Only one concern though, there are other instances also
when we need to stop sending commands to the controllers, but not
necessarily stop getting commands from mid-layer. E.g., in case of delete
drive operation, driver has to wait till all outstanding commands to the
controller are complete, before issuing drive deletion command. In this
situation, having a pending list seems natural.

> Something like
> 
> if (mbox->numstatus != 0xFF)
> 	goto out;
> udelay(25);
> for (i = 0; mbox->numstatus != 0xFF && i < 1000; i++)
> 	msleep(1);
> out:

Sounds good

-Atul Mukker

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[James Bottomley]

On Tue, 2004-07-06 at 13:32, Mukker, Atul wrote:
> > The biggest structural issue still seems to be the pending queue.  It
> > looks to me like the driver could be nicely simplified if you 
> > got rid of
> > it and simply relied on returning SCSI_MLQUEUE_HOST_BUSY from
> > queeucommand().  You'd still get the queue restart almost from the ISR
> This is a very good suggestion and I would be more than willing to include
> in the driver code. Only one concern though, there are other instances also
> when we need to stop sending commands to the controllers, but not
> necessarily stop getting commands from mid-layer. E.g., in case of delete
> drive operation, driver has to wait till all outstanding commands to the
> controller are complete, before issuing drive deletion command. In this
> situation, having a pending list seems natural.

We have this type of functionality already in the scsi_device_quiesce()
interface.  It will not return until all normal commands have completed
(specials and error handling are still allowed).  It was designed for
Domain Validation, but it sounds like you could use it as well.

James

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[Mukker, Atul]

This is a delayed post because of some offline discussions.



> The biggest structural issue still seems to be the pending queue.  It
> looks to me like the driver could be nicely simplified if you 
> got rid of
> it and simply relied on returning SCSI_MLQUEUE_HOST_BUSY from
> queeucommand().  You'd still get the queue restart almost from the ISR
> like you want.
Is there a existing driver using this interface.

> The biggest structural issue still seems to be the pending queue.  It
> looks to me like the driver could be nicely simplified if you 
> got rid of
> it and simply relied on returning SCSI_MLQUEUE_HOST_BUSY from
> queeucommand().  You'd still get the queue restart almost from the ISR
> like you want.
> 
> My other main gripe is things like this:
> 
> +	// wait for maximum 1 second for status to post
> +	for (i = 0; i < 40000; i++) {
> +		if (mbox->numstatus != 0xFF) break;
> +		udelay(25); yield();
> +	}
> 
Also, are these two changes gating factor for the driver to be included in
the kernel. I do not want to make driver releases too often, unless
of-course there is a critical fix, since it reset the driver validation in
our testing labs. I have marked this and the yield() fix for the next drop
of the driver

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[James Bottomley]

On Tue, 2004-07-20 at 16:21, Mukker, Atul wrote:
> Is there a existing driver using this interface.

All the device drivers that use the transport class DV use it.  At the
moment, that's 53c700 and sym_2.

The philosophy for scsi drivers for a while has been to remove internal
queueing, which is why yours attracted my attention, so the question I
was asking is can you use these APIs instead of having to maintain this
internal queue?

> Also, are these two changes gating factor for the driver to be included in
> the kernel. I do not want to make driver releases too often, unless
> of-course there is a critical fix, since it reset the driver validation in
> our testing labs. I have marked this and the yield() fix for the next drop
> of the driver

Either the changes or a good explanation why they shouldn't be done,
yes.  External release cycles aren't a good reason to press for kernel
acceptance.

James

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[Mukker, Atul]

> The philosophy for scsi drivers for a while has been to 
> remove internal
> queueing, which is why yours attracted my attention, so the question I
> was asking is can you use these APIs instead of having to 
> maintain this
> internal queue?
The internal pending queue is fed not only from the mid-layer packets but
also the packets from the management module and (forthcoming) sysfs module.
Both of these modules rely on the fact that the commands issued by them
would be issued sooner or later.

In addition, in certain situations (logical drive deletion) driver has to
stop sending commands to the FW before these operations can be initiated.
This is also implemented using the pending queue mechanism. Although would
be minor change to move over to scsi_block_requests and scsi_unblock_request
to achieve this.

This is not to say that I am reluctant to implement SCSI_MLQUEUE_HOST_BUSY,
but just a precursor where additional impacts would be so that reviewers can
focus on those areas as well.

Thanks
-Atul Mukker
 

RE: [ANNOUNCE]: megaraid driver version 2.20.0.1

[James Bottomley]

On Wed, 2004-07-21 at 08:48, Mukker, Atul wrote:
> > The philosophy for scsi drivers for a while has been to 
> > remove internal
> > queueing, which is why yours attracted my attention, so the question I
> > was asking is can you use these APIs instead of having to 
> > maintain this
> > internal queue?
> The internal pending queue is fed not only from the mid-layer packets but
> also the packets from the management module and (forthcoming) sysfs module.
> Both of these modules rely on the fact that the commands issued by them
> would be issued sooner or later.

Hmm, well, in order to re-use the block layer queueing, you can wrap
these up into special requests and queue them as well.  The actual SCSI
implementation does do a head of queue add, so any request so queued is
the first to be pulled off again and executed

> In addition, in certain situations (logical drive deletion) driver has to
> stop sending commands to the FW before these operations can be initiated.
> This is also implemented using the pending queue mechanism. Although would
> be minor change to move over to scsi_block_requests and scsi_unblock_request
> to achieve this.
> 
> This is not to say that I am reluctant to implement SCSI_MLQUEUE_HOST_BUSY,
> but just a precursor where additional impacts would be so that reviewers can
> focus on those areas as well.

Well, OK, just do the changes to get rid of yield, and we can look at
fixing the queuing in tree.

James