Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Alan Cox <alan@redhat.com>]

>     a) please avoid typedefs for structs.  Also adapter_t is a bit
>     too generic.  What about struct mega_adapter instead?
>     b) please don't use such global arrays but always use the
>     ->hostdata field.

a) megaraid always used a set of basic typedefs. Seems to be your personal
view not general comment

b) Probably right

> static struct notifier_block mega_notifier = {
> 	.notifier_call = megaraid_reboot_notify
> };
> 
>     Do you really need this?  Why can you use
>     struct device_driver->shutdown?

Not on 2.2

> 	if( max_mbox_busy_wait > MBOX_BUSY_WAIT )
> 		max_mbox_busy_wait = MBOX_BUSY_WAIT;
> }
> 
>     Please run the whole driver through scripts/Lindent

Looks fine to me anyway

> megaraid_detect(Scsi_Host_Template *host_template)
> 
>     Please get rid of ->detect and use the new-style pci API +
>     scsi_add_host instead.

Doesnt work on 2.4

> static void internal_done (Scsi_Cmnd *cmd)
> {
> 	internal_done_errcode = cmd->result;
> 	internal_done_flag++;
> 	wake_up (&internal_wait);
> }
> 
> /* shouldn't be used, but included for completeness */

>     This looks horribly racy.

Its ok on older kernels 2.2/2.4 wont afaik ever use it.

> 	return FALSE;
> 
>     Please don't use TRUE/FALSE but 1/0 directly.

TRUE/FALSE is IMHO perfectly clear

> 	if (!try_module_get(THIS_MODULE)) {
> 		return -ENXIO;
> 	}
> 
>    This is broken as hell (and I fixed it in the old megraid driver
>    ages ago!)  Just set ->owner in the file_operation.

Definitely wants fixing
 
> 	while( atomic_read(&adapter->pend_cmds) > 0 ||
> 			!list_empty(&adapter->pending_list) ) {
> 
> 		sleep_on_timeout( &wq, 1*HZ );	/* sleep for 1s */
> 	}
> 
>   Again racy.  You need to opencode this (similar to wait_event, maybe
>   just add wait_even_timeout).

Actually the timeout means the worst that occurs is you wait a 
second, and the code is portable to old 2.2/2.4

> static Scsi_Host_Template driver_template = MEGARAID;
> 
>     Get rid of the ugly macro and add the members right here.
> 
> While we're at it:  Please also get rid of all those prototypes in
> megaraid.h

2.2