LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: "Smart, James" <James.Smart@Emulex.com>
Cc: "'linux-scsi@vger.kernel.org'" <linux-scsi@vger.kernel.org>,
	"'linux-kernel@vger.kernel.org'" <linux-kernel@vger.kernel.org>
Subject: Re: [(re)Announce] Emulex LightPulse Device Driver
Date: Sun, 9 May 2004 09:20:45 +0100	[thread overview]
Message-ID: <20040509092045.A22643@infradead.org> (raw)
In-Reply-To: <3356669BBE90C448AD4645C843E2BF28034F92F0@xbl.ma.emulex.com>; from James.Smart@Emulex.com on Sun, May 09, 2004 at 12:33:35AM -0400

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

  reply	other threads:[~2004-05-09  8:20 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2004-05-09  4:33 Smart, James
2004-05-09  8:20 ` Christoph Hellwig [this message]
2004-05-09  8:51   ` Christoph Hellwig
2004-05-10 16:34 ` Matthew Wilcox
2004-05-10 16:47 Smart, James
2004-05-13 21:21 David.Egolf
2004-05-13 21:26 ` Christoph Hellwig
2004-05-14 19:51 Smart, James
2004-05-14 20:03 ` 'Christoph Hellwig'
2004-05-17 21:25 Smart, James

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20040509092045.A22643@infradead.org \
    --to=hch@infradead.org \
    --cc=James.Smart@Emulex.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --subject='Re: [(re)Announce] Emulex LightPulse Device Driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).