[ANNOUNCE]: megaraid driver version 2.20.0.rc2

[ANNOUNCE]: megaraid driver version 2.20.0.rc2

[Mukker, Atul]

All,

We are pleased to announce the megaraid release candidate (since it is still
in test labs at LSI) for lk 2.6

This driver incorporates the inputs from Paul Wagland, James Bottomley, Matt
Domsch, Christoph Hellwig, Arjan van de Ven, Matthew Wilcox, Marcelo
Tosatti, and many others on the scsi and kernel lists. As always, the
feedback is greatly appreciated.

Highlight of this release

1.	Fully qualified PCI identifiers to identify MegaRAID controllers
2.	PCI shutdown notification routine with hba and devices sync
3.	Support for random drive deletion
4.	Fully re-entrant hot-path w/ data structures protected by their
respective locks. No longer rely on "host_lock". Should boost performance by
5-10% and hopefully better CPU utilization
5.	Better abort and reset handling.

The patch for lk 2.6.6 and the driver is available at

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

Thanks

-Atul Mukker
LSI Logic Corporation

Re: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

[Andre Tomt]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

Mukker, Atul wrote:
> The patch for lk 2.6.6 and the driver is available at
> 
> ftp://ftp.lsil.com/pub/linux-megaraid/drivers/version-2.20.0.rc2/

2.6.6 patch is missing vital drivers/scsi/Makefile part.

-- 
Cheers,
André Tomt

[-- Attachment #2: 31_scsidrv_megaraid-2.20.0.rc2-missing-makefile-bit.patch --]
[-- Type: text/x-diff, Size: 651 bytes --]

diff -Naur linux-2.6.7-rc1-bk4-s1/drivers/scsi/Makefile linux-2.6.7-rc1-bk4-s1-1/drivers/scsi/Makefile
--- linux-2.6.7-rc1-bk4-s1/drivers/scsi/Makefile	2004-05-28 04:07:43.000000000 +0200
+++ linux-2.6.7-rc1-bk4-s1-1/drivers/scsi/Makefile	2004-05-28 07:57:52.000000000 +0200
@@ -96,7 +96,8 @@
 obj-$(CONFIG_SCSI_EATA)		+= eata.o
 obj-$(CONFIG_SCSI_DC395x)	+= dc395x.o
 obj-$(CONFIG_SCSI_DC390T)	+= tmscsim.o
-obj-$(CONFIG_SCSI_MEGARAID)	+= megaraid.o
+obj-$(CONFIG_MEGARAID_LEGACY)	+= megaraid.o
+obj-$(CONFIG_MEGARAID_NEWGEN)	+= megaraid/
 obj-$(CONFIG_SCSI_ACARD)	+= atp870u.o
 obj-$(CONFIG_SCSI_SUNESP)	+= esp.o
 obj-$(CONFIG_SCSI_GDTH)		+= gdth.o

Re: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

['Christoph Hellwig']

[Cc-list cut down, everyone who cares enough should be on linux-scsi anyway]


 - the Kconfig needs to be reworked, the old and new driver aren exclusive,
   allow both of them (especially important for vendor kernels)
 - I don't think megaraid is widely enough used for the default y/m lines
 - please rename the new driver for mailbox-based HBAs to e.g. megraid_mbox
 - #include "../scsi.h" must go, please use <scsi/*.h>
 - the macros in kdep.h should go away, and the include in there should
   got into the individual source files aswell
 - kill PCI_DIR, just use scp->sc_data_direction directly
 - lockscope_t is unused, kill it
 - any chance to use less typedefs, e.g. struct srb instead of srb_t?
   struct adapter instead of adapter_t and maybe mraid_ prefixes to avoid
   namespace clashes?
 - MRAID_GET_DEVICE_MAP is far too large for a macro, please make it a
   function call or opencode it in the callers
 - please kill mraid_driver_t in favour of individual global variables
 - you are _still_ using your own pci-pool reimplementation!
 - instead of mraid_sleep please use msleep()
 - any chance to rework megaraid_mbox.c to need less forward-declarations
   by reordering the function in their natural order?
 - pci_id_table_g would be much more readable when not using C99 initializers
 - please fix the MRAID_TEMPLATE/megaraid_template_g/megaraid_template_gp
   mess.  You need one struct scsi_host_template and can initialize it
   at calltime, no need for macros or additional pointers.
 - your probe for existing device in megaraid_probe_one is completely
   bogus as it doesn't take pci domains into account.  Just kill the
   device_list and that check, it's the PCI layers business to assure
   you don't get multiple probe calls.
 - alloc_adapter_f/init_mbox_f in megaraid_probe_one and use proper gotos
   as I did in the old megaraid driver
 - mraid_driver_g.attach_count--; isn't used ever, just kill it
 - the device list walk in megaraid_mbox_shutdown is bogus, again please
   trust the upper layers
 - the detach path is bogus.  You need to do scshi_remove_host first before
   tearing down anything and scsi_host_put last
 - megaraid_info is not needed, for static info just stuff it into
   scsi_host_template.name
 - the _f variable vs goto comment applies to megaraid_init_mbox aswell
 - WTF is status_t?
 - your locking looks far to fine-grained to me.  Did you benchmark it?
   Where was the bottleneck with a single per-host lock?
 - please always use list_for_each_entry instead of list_for_each,
   dito for _safe
 - please don't reintroduce megaraid_mm_open/close.  I killed the superflous
   open/close routines in megaraid a few times already and they still come
   back silently.

Re: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

['Christoph Hellwig']

Going through my old list what's done and what still needs to be done,

On Fri, May 28, 2004 at 03:02:38PM +0100, 'Christoph Hellwig' wrote:
> [Cc-list cut down, everyone who cares enough should be on linux-scsi anyway]
> 
> 
>  - the Kconfig needs to be reworked, the old and new driver aren exclusive,
>    allow both of them (especially important for vendor kernels)

OK.

>  - I don't think megaraid is widely enough used for the default y/m lines

still needs fixing

>  - please rename the new driver for mailbox-based HBAs to e.g. megraid_mbox

Ok.

>  - #include "../scsi.h" must go, please use <scsi/*.h>

Ok.

>  - the macros in kdep.h should go away, and the include in there should
>    got into the individual source files aswell

first is ok, the latter only moved to mega_common.h instead of the actual
source files

>  - kill PCI_DIR, just use scp->sc_data_direction directly

Ok.

>  - lockscope_t is unused, kill it

Ok.

>  - any chance to use less typedefs, e.g. struct srb instead of srb_t?
>    struct adapter instead of adapter_t and maybe mraid_ prefixes to avoid
>    namespace clashes?

Some of this seems to be done, thanks.  Please also remove the _t postfixes
of the struct names.

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

>  - please kill mraid_driver_t in favour of individual global variables

still there.

>  - you are _still_ using your own pci-pool reimplementation!

Ok.

>  - instead of mraid_sleep please use msleep()

Ok.

>  - any chance to rework megaraid_mbox.c to need less forward-declarations
>    by reordering the function in their natural order?

still quite a lot of those.

>  - pci_id_table_g would be much more readable when not using C99 initializers

still there.

>  - please fix the MRAID_TEMPLATE/megaraid_template_g/megaraid_template_gp
>    mess.  You need one struct scsi_host_template and can initialize it
>    at calltime, no need for macros or additional pointers.

done, thanks.  But you double-indented the megaraid_template_g members now.

>  - your probe for existing device in megaraid_probe_one is completely
>    bogus as it doesn't take pci domains into account.  Just kill the
>    device_list and that check, it's the PCI layers business to assure
>    you don't get multiple probe calls.

Done, thanks.

>  - 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.

>  - mraid_driver_g.attach_count--; isn't used ever, just kill it

done, thanks.

>  - the device list walk in megaraid_mbox_shutdown is bogus, again please
>    trust the upper layers

done, thanks.

>  - the detach path is bogus.  You need to do scshi_remove_host first before
>    tearing down anything and scsi_host_put last

still needs to be fixed.

>  - megaraid_info is not needed, for static info just stuff it into
>    scsi_host_template.name

done, thanks.

>  - the _f variable vs goto comment applies to megaraid_init_mbox aswell

dito

>  - your locking looks far to fine-grained to me.  Did you benchmark it?
>    Where was the bottleneck with a single per-host lock?

sill applies.

>  - please always use list_for_each_entry instead of list_for_each,
>    dito for _safe

done, thanks.

>  - please don't reintroduce megaraid_mm_open/close.  I killed the superflous
>    open/close routines in megaraid a few times already and they still come
>    back silently.

done, thanks.


Also a new quick observation:  mraid_driver_g.device_list is completely
unused, you can just remove it

Re: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

[Jeff Garzik]

'Christoph Hellwig' wrote:
>> - any chance to rework megaraid_mbox.c to need less forward-declarations
>>   by reordering the function in their natural order?
> 
> 
> still quite a lot of those.

This is maintainer's choice, and is _not_ a requirement for merging.

Changing this causes nothing but code noise, and often increases icache 
misses since gcc doesn't have profile data to indicate which functions 
should be grouped together automatically.

	Jeff

Re: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

['Christoph Hellwig']

On Wed, Jun 23, 2004 at 04:42:30PM -0400, Jeff Garzik wrote:
> 'Christoph Hellwig' wrote:
> >>- any chance to rework megaraid_mbox.c to need less forward-declarations
> >>  by reordering the function in their natural order?
> >
> >
> >still quite a lot of those.
> 
> This is maintainer's choice, and is _not_ a requirement for merging.

Read the request, it's certainly not written as a hard requirement, but
I ask every maintainer nicely for it anyway.

RE: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

[Mukker, Atul]

Thanks for the exhaustive feedback, please see inline comments

>  - the Kconfig needs to be reworked, the old and new driver 
> aren exclusive,
Actually they are. You cannot, for example, have megaraid (legacy) and
megaraid mgmt module since both would try to own the device for management
apps.

>    allow both of them (especially important for vendor kernels)

>  - I don't think megaraid is widely enough used for the 
> default y/m lines
That was a missing patch for scsi/Makefile. Will be fixed in next rev

>  - please rename the new driver for mailbox-based HBAs to 
> e.g. megraid_mbox
>  - #include "../scsi.h" must go, please use <scsi/*.h>
>  - the macros in kdep.h should go away, and the include in 
> there should
>    got into the individual source files aswell
>  - kill PCI_DIR, just use scp->sc_data_direction directly
>  - lockscope_t is unused, kill it
All ok

>  - any chance to use less typedefs, e.g. struct srb instead of srb_t?
>    struct adapter instead of adapter_t and maybe mraid_ 
> prefixes to avoid
>    namespace clashes?
Too many changes, but will be done, but probably not in next rev.

>  - MRAID_GET_DEVICE_MAP is far too large for a macro, please make it a
>    function call or opencode it in the callers
ok

>  - please kill mraid_driver_t in favour of individual global variables
It is in fact like that already! mraid_driver_t is just a template for LLDs

>  - you are _still_ using your own pci-pool reimplementation!
It is being changed right now

>  - instead of mraid_sleep please use msleep()
msleep, is it a standart API? carmel.c and libata-core.c both seem to have
their own definitions for it.

>  - any chance to rework megaraid_mbox.c to need less 
> forward-declarations
>    by reordering the function in their natural order?
It can be done ofcourse. It is a big impact change though, so would be
deferred for now.

>  - pci_id_table_g would be much more readable when not using 
> C99 initializers
But then we would couple pci_id_table_g with current definition of pci
table. Isn't this one of the reasons to use C99 initializations?

>  - please fix the 
> MRAID_TEMPLATE/megaraid_template_g/megaraid_template_gp
>    mess.  You need one struct scsi_host_template and can initialize it
>    at calltime, no need for macros or additional pointers.
It is a residue from lk 2.4 support. Would be removed now

>  - your probe for existing device in megaraid_probe_one is completely
>    bogus as it doesn't take pci domains into account.  Just kill the
>    device_list and that check, it's the PCI layers business to assure
>    you don't get multiple probe calls.
ok

>  - alloc_adapter_f/init_mbox_f in megaraid_probe_one and use 
> proper gotos
>    as I did in the old megaraid driver
Again, what here?

>  - mraid_driver_g.attach_count--; isn't used ever, just kill it
ok

>  - the device list walk in megaraid_mbox_shutdown is bogus, 
> again please
>    trust the upper layers
ok

>  - the detach path is bogus.  You need to do 
> scshi_remove_host first before
>    tearing down anything and scsi_host_put last
megaraid_io_detach does it exactly like that

>  - megaraid_info is not needed, for static info just stuff it into
>    scsi_host_template.name
ok

>  - the _f variable vs goto comment applies to 
> megaraid_init_mbox aswell
This is generic scheme use throughout the driver. Also see,
megaraid_alloc_cmd_packets

>  - WTF is status_t?
nothing, being removed :-)

>  - your locking looks far to fine-grained to me.  Did you 
> benchmark it?
>    Where was the bottleneck with a single per-host lock?
This locks are more natural. We are benchmarking right now

>  - please always use list_for_each_entry instead of list_for_each,
>    dito for _safe
ok

>  - please don't reintroduce megaraid_mm_open/close.  I killed 
> the superflous
>    open/close routines in megaraid a few times already and 
> they still come
>    back silently.
> 
ok

Re: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

['Christoph Hellwig']

On Fri, May 28, 2004 at 10:56:29AM -0400, Mukker, Atul wrote:
> >  - the Kconfig needs to be reworked, the old and new driver 
> > aren exclusive,
> Actually they are. You cannot, for example, have megaraid (legacy) and
> megaraid mgmt module since both would try to own the device for management
> apps.

Even if you can't load both you can still build both and the other will
fail to load (or at least should, if not that's a bug on your side ;-))

> >  - instead of mraid_sleep please use msleep()
> msleep, is it a standart API? carmel.c and libata-core.c both seem to have
> their own definitions for it.

We moved to a common implementation after 2.6.6 was released.

> >  - any chance to rework megaraid_mbox.c to need less 
> > forward-declarations
> >    by reordering the function in their natural order?
> It can be done ofcourse. It is a big impact change though, so would be
> deferred for now.

Sure, just sweep over it when everythign else it done.

> >  - pci_id_table_g would be much more readable when not using 
> > C99 initializers
> But then we would couple pci_id_table_g with current definition of pci
> table. Isn't this one of the reasons to use C99 initializations?

PCI table doesn't change, there's lots of users that prefer the more
readable variant.  And it's really far less and much easier to grok
lines without C99 initializers.


> >  - alloc_adapter_f/init_mbox_f in megaraid_probe_one and use 
> > proper gotos
> >    as I did in the old megaraid driver
> Again, what here?

Use goto's to jump to the appropinquate cleanup step instead of the
boolean foo_f variables and a single cleanup label.

> >  - the detach path is bogus.  You need to do 
> > scshi_remove_host first before
> >    tearing down anything and scsi_host_put last
> megaraid_io_detach does it exactly like that

No. In megaraid_detach_one your first set being_detached, and set
the pci driver data to NULL, then call megaraid_io_detach with
scsi_remove_host, then scsi_host_put (btw, please kill megaraid_io_detach
as a separate function),  then call megaraid_fini_mbox and kfree(adapter).


> >  - the _f variable vs goto comment applies to 
> > megaraid_init_mbox aswell
> This is generic scheme use throughout the driver. Also see,
> megaraid_alloc_cmd_packets

Right, that also needs fixing..

RE: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

[Mukker, Atul]

>  - I don't think megaraid is widely enough used for the default y/m lines

still needs fixing

AM> The statement is wrong. MegaRAID *is* a very widely used controller

>  - the macros in kdep.h should go away, and the include in there should
>    got into the individual source files aswell

first is ok, the latter only moved to mega_common.h instead of the actual
source files
AM> IMO, these subset of original macros make the translation to mid-layer
to LLD very convenient.

Some of this seems to be done, thanks.  Please also remove the _t postfixes
of the struct names.
AM> That's next major rev goal.

>  - 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.

>  - please kill mraid_driver_t in favour of individual global variables

still there.

AM> This will be fixed

>  - any chance to rework megaraid_mbox.c to need less forward-declarations
>    by reordering the function in their natural order?

still quite a lot of those.
AM> Again, this matches with our driver functions layout. If you observe
closely, the driver functions (well mostly) are laid out in a breadth-first
approach, with do-undo functions kept together. If this approach does not
meet general driver guidelines, we will fix it, but next major rev

>  - pci_id_table_g would be much more readable when not using C99
initializers

still there.

AM> NO, foul! It has been corrected

>  - please fix the MRAID_TEMPLATE/megaraid_template_g/megaraid_template_gp
>    mess.  You need one struct scsi_host_template and can initialize it
>    at calltime, no need for macros or additional pointers.

done, thanks.  But you double-indented the megaraid_template_g members now.
AM> Oversight, will fix it


>  - 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.

AM> I feel, this is a cleaner approach to up-wrap in case something goes
bad. With goto 'many labels' the place you are going to jump from actually
wants to undo (possibly unrelated) previous step

>  - the detach path is bogus.  You need to do scshi_remove_host first
before
>    tearing down anything and scsi_host_put last

still needs to be fixed.
AM> I must be missing something here. Please elaborate again.

>  - your locking looks far to fine-grained to me.  Did you benchmark it?
>    Where was the bottleneck with a single per-host lock?

sill applies.
AM> If fact, the performance did improve on the enterprise kernels with
fine-grain locks. So, this locking mechanism should stay

Also a new quick observation:  mraid_driver_g.device_list is completely
unused, you can just remove it
AM> Will do

Re: [ANNOUNCE]: megaraid driver version 2.20.0.rc2

['Christoph Hellwig']

On Wed, Jun 23, 2004 at 11:45:14PM -0400, Mukker, Atul wrote:
> >  - I don't think megaraid is widely enough used for the default y/m lines
> 
> still needs fixing
> 
> AM> The statement is wrong. MegaRAID *is* a very widely used controller

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.

> >  - the macros in kdep.h should go away, and the include in there should
> >    got into the individual source files aswell
> 
> first is ok, the latter only moved to mega_common.h instead of the actual
> source files
> AM> IMO, these subset of original macros make the translation to mid-layer
> to LLD very convenient.

I didn't actually complain about macros, but that the include files aren't
in the individual 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.

> 
> >  - any chance to rework megaraid_mbox.c to need less forward-declarations
> >    by reordering the function in their natural order?
> 
> still quite a lot of those.
> AM> Again, this matches with our driver functions layout. If you observe
> closely, the driver functions (well mostly) are laid out in a breadth-first
> approach, with do-undo functions kept together. If this approach does not
> meet general driver guidelines, we will fix it, but next major rev

Well, it's okay, I just asked because I didn't get any feedback on it.

> >  - pci_id_table_g would be much more readable when not using C99
> initializers
> 
> still there.
> 
> AM> NO, foul! It has been corrected

Sorry, my apologies.  I still looked huge beause you have each ID spread
over multiple lines.  

> >  - 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.
> 
> AM> I feel, this is a cleaner approach to up-wrap in case something goes
> bad. With goto 'many labels' the place you are going to jump from actually
> wants to undo (possibly unrelated) previous step

You usually want to undo all previous steps, exactly because of that the
goto approach is superior.

> >  - the detach path is bogus.  You need to do scshi_remove_host first
> before
> >    tearing down anything and scsi_host_put last
> 
> still needs to be fixed.
> AM> I must be missing something here. Please elaborate again.

Basic rule for a ->remove routine of a scsi driver is:

Call scsi_remove_host() first, then do all teardown of the hardware and
unregistration of ressources (release_region, iounmap, free_irq, etc..) and
then finally call scsi_host_put.

> >  - your locking looks far to fine-grained to me.  Did you benchmark it?
> >    Where was the bottleneck with a single per-host lock?
> 
> sill applies.
> AM> If fact, the performance did improve on the enterprise kernels with
> fine-grain locks. So, this locking mechanism should stay

Ok, that's why I asked for benchmark numbers.