> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed Apr 23 2003 - 22:00:21 EST