linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Andrew Vasquez <andrew.vasquez@qlogic.com>
Cc: Linux-Kernel <linux-kernel@vger.kernel.org>,
	Linux-SCSI <linux-scsi@vger.kernel.org>
Subject: Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).
Date: Fri, 18 Jul 2003 12:23:04 +0100	[thread overview]
Message-ID: <20030718122304.A23013@infradead.org> (raw)
In-Reply-To: <B179AE41C1147041AA1121F44614F0B0598B10@AVEXCH02.qlogic.org>; from andrew.vasquez@qlogic.com on Thu, Jul 17, 2003 at 04:40:00PM -0700


More comments:

 - Documentation/scsi/qla2xxx.txt seems to document a 2.4ish driver
 - Documentation/scsi/qla2xxx.release.txt looks superflous, can you
   merge it into the (updated) qla2xxx.txt?
 - drivers/scsi/qla2xxx/ has duplicates of the documentation

 - the (v8) comments in drivers/scsi/qla2xxx/Kconfig are superflous
 - drivers/scsi/qla2xxx/Makefile is a mess.  If you want to compile
   a single source file multiple times in different ways create a
   new source file that sets the needed defines and #includes the
   source file.  But in general this should be avoided in favour of
   a common library module..

 - please document which files are shared with other operating systems
   (some look like they do, don't they?)
 - the driver still has multipath support in the lowlevel driver which
   we don't want in Linux
 - UNIQUE_FW_NAME is still there.  shouldn't this be enabled uncdontionally?
 - still tons of typedefs that want cleaning up
 - your include structures is very confusing.  Please include the linux
   headers directly i nthe files using them (except of course for
   source files shares with other OSes)
 
 - qla2x00_intr_handler should use spin_lock, not spin_lock_irqsave
 - you're using down_interruptible without checking the return value
 - please switch to newstyle module/boot parameter handling (module_param)
   (oh, okay, found the TODO comment)
 - what is #if defined (CONFIG_SCSIFCHOTSWAP) || defined(CONFIG_GAMAP)?
   that's never defined in a 2.5 tree and the code looks ugly as hell.
 - .unchecked_isa_dma      = 0 in the host template is superflous,
   it's 0 by default.
 - what is CONFIG_MD_MULTIHOST_FC?
 - you are using a static buffer in qla2x00_info, there's a lock needed
   to protect it
 - in qla2x00_queuecommand you need to call spin_unlock_irq/spin_lock_irq
   on the hostlock, otherwise your whole ->queuecommand runs with (local)
   interrupts disabled
 - qla2x00_biosparam is superflous, if there's no ->biosparam the midlayer
   calls scsicam_bios_param directly
 - you don't need to call scsi_set_device if you pass the proper struct device
   to scsi_add_host
 - please don't use scsi_assign_lock.  In 2.5 we already have a per-HBA
   lock and using a different one than the default one doesn't make sense.
 - you need to check the return value from scsi_add_host
 - please remove the "sanity check" in qla2x00_remove_device.  If you
   can't trust the pci layer you have worse problems..
 - ->proc_info is deprecated, please implement shost and sdev sysfs
   attributes instead.
 
 - all contents of qla_ip.c is surrounded by a big if defined(FC_IP_SUPPORT),
   please compile the file conditionally instead.  (also in linux we prefer
   #ifdef over #if defined)
 - where is qla2200_ip_inquiry/qla2300_ip_inquiry called??  also it makes
   more sense to pass the scsi_qla_host_t directrly to it instead of an
   adapter number that requires list walking


  parent reply	other threads:[~2003-07-18 11:08 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2003-07-17 23:40 [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4) Andrew Vasquez
2003-07-18 10:48 ` Christoph Hellwig
2003-07-18 11:23 ` Christoph Hellwig [this message]
2003-07-18 12:12   ` Lars Marowsky-Bree
2003-07-18 12:13     ` Christoph Hellwig
2003-07-18 12:26       ` Lars Marowsky-Bree
2003-07-18 12:34         ` Christoph Hellwig
2003-07-18 12:41           ` Lars Marowsky-Bree
2003-07-18 12:46             ` Christoph Hellwig
2003-07-18 13:46   ` Arjan van de Ven
2003-07-18 14:03     ` [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4 ) Jamie Wellnitz
2003-07-18 19:12     ` [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4) Christoph Hellwig
2003-07-18 19:10   ` Christoph Hellwig
2003-07-19  1:05 Manfred Spraul

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=20030718122304.A23013@infradead.org \
    --to=hch@infradead.org \
    --cc=andrew.vasquez@qlogic.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).