All of lore.kernel.org
 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: 21+ 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:26         ` Lars Marowsky-Bree
2003-07-18 12:34         ` Christoph Hellwig
2003-07-18 12:41           ` Lars Marowsky-Bree
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:12       ` Christoph Hellwig
2003-07-18 19:10   ` Christoph Hellwig
2003-07-18 21:15 Andrew Vasquez
2003-07-18 21:46 Andrew Vasquez
2003-07-18 21:53 Andrew Vasquez
2003-07-19  1:05 Manfred Spraul
2005-01-14 20:24 Eric Pesciotta

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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.