Messages in this thread | | | Date | Sun, 9 May 2004 09:20:45 +0100 | From | Christoph Hellwig <> | Subject | Re: [(re)Announce] Emulex LightPulse Device Driver |
| |
A bunch of comments from looking over the headers and itnerface to upper layers a little: (next I'll try to understand what's going on in the I/O submission path - it's just to freakin complicated..):
- the hba api crap must die - lots of strange typedefs in sSuDly Caps that want to die. - Dito for the strange pfoo pointer naming. - please explain USE_HGP_HOST_SLIM and why only ppc64 sets it, per-arch ifdefs in LLDDs are very suspect - do you really care for pre-2.6.5 compat? - lpfc_fcp.h duplicates the scsi opcode - use those from scsi/scsi.h - lpfc_msg* stuff is not acceptable. simply put the string you want to print into the printks - your module_param usage is b0rked. Instead of duplicating every option 31 times use module_param_array. You also seem to miss the paramter descriptions (MODULE_PARAM_DESC) - lpfc_clock.c should go away, just use add_timer/etc. diretly and embedd the timer into your structures. Yes, I know that's not how SVR4-derivates work, but Linux does. You also have an unchecked kmalloc and a list_head cast in there.. - you're using list_for_each{,_safe} a lot where you'd want to use the _entry versions. - #define EXPORT_SYMTAB in lpfc_fcp.c is wrong, you never need that one in 2.6. - why do you need <linux/if_arp.h> and <linux/rtnetlink.h> in there? - please always include <linux/*> first, then <asm/*>, then <scsi/*>, then private headers. - OpenSource in the module description is useless - MODULE_LICENSE already tels us that. - why do you disable clustering in the 2.6 driver? - lpfcDRVR_t should go away in favour of a bunch of global variables - lpfc_linux_attach should be merged into lpfc_pci_detect, which would better be named lpfc_{,pci_}probe{,one} - same for lpfc_linux_detach into lpfc_pci_release - formatting of lpfc_driver is broken - not need to memset lpfcDRVR, it's 0 if it's in .bss already - what do you need lpfcDRVR.loadtime for? - lpfcDRVR.hba_list_head should be initializes at compile time LIST_HEAD(hba_list); if properly taken out of the struct. - you must attach the driver attributes always unless pci_module_init failed, alese hotpluggin does´t work. - lpfc_sleep_ms is broken, it'll happily sleep under spinlock, please use mdelay directly in contexts where you can't sleep and schedule_timeout only where you can sleep. - lpfc_sleep needs to die, it's just a broken reimplementation of sleep_on{,_interruptible,}{_timeout,} - lpfc_tasklet would probably benefit from a list_splice_init early on so the proceasing can happen without retaking the spinlock all the time - why all the EXPORT_SYMBOLS? - pleae kill all those silly one-line wrappers in lpfc_mem.c. - four mempool per hba seems a little much, care to explain why they're needed? - you don't need a pci_pool per hba, just one per driver - lpfc_sysfs_set_show/lpfc_sysfs_set_store violate the one value per attribute rule - dito for lpfc_sysfs_params_show/lpfc_sysfs_params_store - dito for lpfc_sysfs_info_show, in short all your sysfs work is completely wrong, also you seem to use driver attributes instead of scsi device/host attributes. why? - in lpfc_queuecommand you don't need to spin_lock_irqsave your drvlock as the host_lock is already taken with irqs disabled. you should probably redesign your driver to use the host_lock instead of the drvlock everywhere. - the error return if queuecommand lpfc_get_scsi_buf fails looks bogus, why do you set DID_BUS_BUSY? also instead of 1 please return SCSI_MLQUEUE_HOST_BUSY - you shouldn't need the scsi_{bus,target,lun} members in lpfc_cmd but always use the scsi_cmnd->device linked from there. - the lpfc_find_target usage in queuecommand looks bogus. You have scsi_device->hostdata to put per-lun data, from which you can trivially link per-target data directly. All the checks for inquiry and valid luns are similarly b0rked - scsi probing works by calling ->slave_alloc first so you'll a) have a place where you know the midlayer is probing and b) always private data when quecommand is called. - in lpfc_scsi_cmd_start you use lpfc_find_lun_device which is copletely bogus again, use scsi_cmnd->device->hostdata - don't mess with eh_timeout from a LLDD please - 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/
| |