Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Manfred Spraul]

Andrew wrote:

>>  - qla2x00_intr_handler should use spin_lock, not spin_lock_irqsave
>
>Are you sure about that?  I'll need to refresh my interrupt handling
>know-how...
>  
>
It's an optimization that is used by many drivers:
Interrupt handlers are never reentered - if you are within 
qla2x00_intr_handler handling irq x, then it's guaranteed that the 
function won't be reentered by another occurance of the same interrupt.
If your driver registers only one interrupt handler, then you can skip 
disabling the local interrupts - a deadlock is not possible.

You need _irqsave if the spinlock is shared between multiple instances 
of the hba, with different interrupts (i.e. it's possible that 
qla2x00_intr_handler is called for irq y while handling irq x).

--
    Manfred

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Christoph Hellwig]

On Fri, Jul 18, 2003 at 03:46:42PM +0200, Arjan van de Ven wrote:
> On Fri, 2003-07-18 at 13:23, Christoph Hellwig wrote:
> >  
> >  - qla2x00_intr_handler should use spin_lock, not spin_lock_irqsave
> 
> possibly correct; on x86 irq handlers run with interrupts enabled for
> example; just too dangerous to do this esp if error recovery and similar
> code calls this from process context as well (iirc a few places do)

Well, that's only true for SA_INTERRUPT.  But I missed that qla2xxx
sets this.  So drop þhat comment.

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Christoph Hellwig]

On Fri, Jul 18, 2003 at 12:23:04PM +0100, Christoph Hellwig wrote:
>  - drivers/scsi/qla2xxx/ has duplicates of the documentation

Sorry, this isn't actually true, they were just left over from an
older version.

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4 ).

[Jamie Wellnitz]

On Fri, Jul 18, 2003 at 09:46:42AM -0400, Arjan van de Ven wrote:
> On Fri, 2003-07-18 at 13:23, Christoph Hellwig wrote:
> >  
> >  - qla2x00_intr_handler should use spin_lock, not spin_lock_irqsave
> 
> possibly correct; on x86 irq handlers run with interrupts enabled for
> example; just too dangerous to do this esp if error recovery and similar
> code calls this from process context as well (iirc a few places do)

Is this true?  Do irq handlers _always_ run with interrupts enabled?
I thought the driver could control this behavior with the SA_INTERRUPT
flag.

This code in handle_IRQ_event seems to turn interrupts back on
(apparently someone has turned them off), but only if SA_INTERRUPT is
not set.

   if (!(action->flags & SA_INTERRUPT))
                 __sti();
 
Thanks,
Jamie Wellnitz
Jamie.Wellnitz@emulex.com

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

On Fri, 2003-07-18 at 13:23, Christoph Hellwig wrote:
>  
>  - qla2x00_intr_handler should use spin_lock, not spin_lock_irqsave

possibly correct; on x86 irq handlers run with interrupts enabled for
example; just too dangerous to do this esp if error recovery and similar
code calls this from process context as well (iirc a few places do)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Christoph Hellwig]

On Fri, Jul 18, 2003 at 02:41:27PM +0200, Lars Marowsky-Bree wrote:
> However, while I agree about this being cruft in qla2xxx, it is _used_.

And?  LiS is used, too as are tons of hacks that have no chance of
getting in mainline ever.

> It's a driver / HBA feature which is actively deployed. I'd like to see
> it go sooner than later, but by blocking this feature you preclude
> those users of the driver from using the mainline one again, which is
> the entire point of this exercise.
> 
> Dropping such a feature needs some preparation to protect the users, and
> isn't justified by the personal dislikes of myself or you I'm afraid
> ;-)

It's not about personal dislikes but about policies.  If we allow qlogic
to get their mpio code into the kernel there'll soon be more and more
HBA vendors who do the same.

And it's not like the code will vanish from earth if it's gone from
the mainline driver, if SuSE wishes to patch it back in to protect
the investments of their custorers (or insert random other CEO-speak
here) they're free to patch in the mess again.

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Lars Marowsky-Bree]

On 2003-07-18T13:34:04,
   Christoph Hellwig <hch@infradead.org> said:

> So what?  Vendor ship all kinds of strange stuff and we can't really
> keep the compat cruft for that around.  And the mpio stuff in qla2xxx
> is _lots_ of cruft.

The other points you mentioned preclude merging it into mainline right
now anyway and needs fixing. Your comments there are spot on.

However, while I agree about this being cruft in qla2xxx, it is _used_.
It's a driver / HBA feature which is actively deployed. I'd like to see
it go sooner than later, but by blocking this feature you preclude
those users of the driver from using the mainline one again, which is
the entire point of this exercise.

Dropping such a feature needs some preparation to protect the users, and
isn't justified by the personal dislikes of myself or you I'm afraid
;-)


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
SuSE Labs - Research & Development, SuSE Linux AG
  
"If anything can go wrong, it will." "Chance favors the prepared (mind)."
  -- Capt. Edward A. Murphy            -- Louis Pasteur

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Christoph Hellwig]

On Fri, Jul 18, 2003 at 02:26:22PM +0200, Lars Marowsky-Bree wrote:
> > Given that this module never was in 2.4 I don't see clear backwards
> > compatiblity problems..
> 
> You are being very funny. No, nobody ever shipped a qlogic driver.
> Right.

So what?  Vendor ship all kinds of strange stuff and we can't really
keep the compat cruft for that around.  And the mpio stuff in qla2xxx
is _lots_ of cruft.

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Lars Marowsky-Bree]

On 2003-07-18T13:13:52,
   Christoph Hellwig <hch@infradead.org> said:

> > Please come and attend the multipath session at the Kernel Summit and/or
> > the Ottawa Linux Symposium next week to learn why this is a bad idea and
> > how it can be done better -> http://www.linuxsymposium.org/
> -ENOTIME :)

Well, I was inviting Andrew, mostly, I know that you are around for
KS/OLS and are also probably already convinced ;-)

> > QLogic is reasonably good already in that it can be disabled via a
> > module parameter (ql2xfailover=0), allowing the higher level solutions
> > to operate.
> Given that this module never was in 2.4 I don't see clear backwards
> compatiblity problems..

You are being very funny. No, nobody ever shipped a qlogic driver.
Right.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
SuSE Labs - Research & Development, SuSE Linux AG
  
"If anything can go wrong, it will." "Chance favors the prepared (mind)."
  -- Capt. Edward A. Murphy            -- Louis Pasteur

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Christoph Hellwig]

On Fri, Jul 18, 2003 at 02:12:02PM +0200, Lars Marowsky-Bree wrote:
> Please come and attend the multipath session at the Kernel Summit and/or
> the Ottawa Linux Symposium next week to learn why this is a bad idea and
> how it can be done better -> http://www.linuxsymposium.org/

-ENOTIME :)

> However, for backwards compatibility with 2.4, I assume it might still
> be needed in 2.5/2.6, but _only_ with the clear intend of phasing it out
> within 2.6 and dropping it in 2.7.
> 
> QLogic is reasonably good already in that it can be disabled via a
> module parameter (ql2xfailover=0), allowing the higher level solutions
> to operate.

Given that this module never was in 2.4 I don't see clear backwards
compatiblity problems..

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Lars Marowsky-Bree]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On 2003-07-18T12:23:04,
   Christoph Hellwig <hch@infradead.org> said:

>  - the driver still has multipath support in the lowlevel driver which
>    we don't want in Linux

Fully agreed.

Please come and attend the multipath session at the Kernel Summit and/or
the Ottawa Linux Symposium next week to learn why this is a bad idea and
how it can be done better -> http://www.linuxsymposium.org/

However, for backwards compatibility with 2.4, I assume it might still
be needed in 2.5/2.6, but _only_ with the clear intend of phasing it out
within 2.6 and dropping it in 2.7.

QLogic is reasonably good already in that it can be disabled via a
module parameter (ql2xfailover=0), allowing the higher level solutions
to operate.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
SuSE Labs - Research & Development, SuSE Linux AG
  
"If anything can go wrong, it will." "Chance favors the prepared (mind)."
  -- Capt. Edward A. Murphy            -- Louis Pasteur

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Christoph Hellwig]

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

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Christoph Hellwig]

On Thu, Jul 17, 2003 at 04:40:00PM -0700, Andrew Vasquez wrote:
> Finally, regarding some of the more interesting (if not the) question(s)
> pertaining to the development of qla2xxx:
> 
>   o Creation of a single driver module rather than three distinct
> 	drivers for each ISP type (21xx, 22xx, and 23xx).
> 
>   From the technical side, there aren't many compelling reasons for
>   the change to not occur.  Support for 2k logins on 2300s did
>   introduce a rather large, but manageable (through the compile-time
>   preprocessor), interface change between the host driver and
>   firmware.  The driver could of course manage this during run-time
>   with some creative structure overlays, etc.  Secondly, bundling
>   firmware for all ISP types can lead to a rather large binary
>   module (21xx - ~64kbytes, 22xx - ~90kbytes, 23xx - ~110kbytes).

Well, support for each of the subtypes can be conditional or even
in their own submodules that share a common "library" module.

>   Unfortunately, it is support that ultimately becomes the
>   overriding factor in maintaining the three-module build process.

Well, the current build process is horrible :)

>   By building distinct modules (i.e. qla2300.ko to support ISP2300,
>   ISP2312, and ISP2322 chips) our DVT group would focus their time and
>   efforts on testing 23xx HBAs and not on regressing support with
>   EOL'd products.

Why can't you declare the others unsupported even if they are in
the same module? You'd have three config option for including support
for each specific chip type and two of them would be marked unsupported.

> Until a policy change, the 8.x driver in its current form will have
> the limitation of only one driver, qla2100, qla2200, or qla2300, can
> be built as part of the kernel at any given time.

This is not acceptable for a driver in mainline, just FYI.

[ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b4).

[Andrew Vasquez]

All,

A new version of the 8.x series driver for Linux 2.6.x kernels has
been uploaded to SourceForge:

	http://sourceforge.net/projects/linux-qla2xxx/

This beta contains a large rewrite of some fundamental structures used
throughout the driver.  This change sets the stage for some of the
more interesting and useful updates such as asynchronous mailbox
commands and a less intrusive discovery processes.

Additionally, the beta includes several performance related changes:

	o An initial merge of performance patches from Arjan van de
	  Ven.  Read the revision notes for details:
	  - Lock contention fixes.
	  - RIO support.
	o SCSI command --> IOCB formation speedup.

BTW: future driver drops *will* come out at a more spirited pace.

Changes include:

 *	- IOCB staging fixups.
 *	- Full support for 2k port logins (~2000 fabric devices).
 *	- Resync with 6.06.00b12.
 *	- Resync with Linux Kernel 2.6.0-test1.

Finally, regarding some of the more interesting (if not the) question(s)
pertaining to the development of qla2xxx:

  o Creation of a single driver module rather than three distinct
	drivers for each ISP type (21xx, 22xx, and 23xx).

  From the technical side, there aren't many compelling reasons for
  the change to not occur.  Support for 2k logins on 2300s did
  introduce a rather large, but manageable (through the compile-time
  preprocessor), interface change between the host driver and
  firmware.  The driver could of course manage this during run-time
  with some creative structure overlays, etc.  Secondly, bundling
  firmware for all ISP types can lead to a rather large binary
  module (21xx - ~64kbytes, 22xx - ~90kbytes, 23xx - ~110kbytes).
  Also, when formal support for the ISP2322 chip is added, an
  additional firmware image (again ~110kbytes) will need to be
  compiled with the driver.  The new user-space firmware-load
  interface could address the size concerns, but at the same time it
  also calls into question the larger issue of support.

  Unfortunately, it is support that ultimately becomes the
  overriding factor in maintaining the three-module build process.
  By building distinct modules (i.e. qla2300.ko to support ISP2300,
  ISP2312, and ISP2322 chips) our DVT group would focus their time and
  efforts on testing 23xx HBAs and not on regressing support with
  EOL'd products.

Until a policy change, the 8.x driver in its current form will have
the limitation of only one driver, qla2100, qla2200, or qla2300, can
be built as part of the kernel at any given time.  Please note, all
three binaries built as modules *can* be loaded without incident.  For
example:

  The following combinations will NOT work:

	- 2100 and 2200 support built as part of the kernel.
	- 2200 and 2300 support built as part of the kernel.
	- 2100 and 2300 support built as part of the kernel.

  The following combinations will work:

	- 2100, 2200 and 2300 support built as modules.
	- 2100 support build as part of the kernel, 2200 and 2300
	  support built as modules.
	- 2300 support build as part of the kernel, 2100 and 2200
	  support built as modules.
	- etc...

Regards,
Andrew Vasquez