RE: pulling failover out of qla2xxx

RE: pulling failover out of qla2xxx

[Andrew Vasquez]

On Thursday, January 15, 2004 2:16 PM, James Bottomley wrote:

> I think the best way to maintain this is to add #ifdef
> CONFIG_SCSI_QLA2XXX_FAILOVER around all this code that was taken out
> of the main routines (so it really is compiled out when failover
> isn't defined) in your vendor tree. 
> 
> Then, to prepare kernel patches, you can run the vendor tree through
> unifdef to remove the failover code...and we can keep the
> non-failover code bases in sync. 
> 

Ok, I've uploaded a rather large patch to the SourceForge site:

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

The file qla2xxx-resync_bk-v8.00.00b9.diff.bz2 available from the
download page will resync the qla2xxx driver in scsi-misc-2.7 with the
latest 8.00.00b9 release driver done yesterday -- of course, without
any FO/IOCTL pieces.

The patch weights in at a hefty 360K bzipped, where the bulk of the
diff is due to the updated ql2300_fw.c file and the three firmware
files for the newly supported ISP types (ISP2322, ISP6312, and
ISP6322).  The residuals weigh-in at ~70K of uncompressed diffs with
the core of the b8 -> b9 changes at around ~40K (the down_timeout
update mentioned below is around 30K).

Other changes include:

	o Remerge Christoph's down_timeout() update for mailbox
	  commands -- use a common and simpler variant of the down
	  with timeout scheme.  (this was appearantly dropped during
	  the bk update).

And again, the changes from 8.00.00b8 to 8.00.00b9:

	o Minimize device_id checks in register access functions
	  (IS_QLA23XX(), ISP_*_Q_*(), ...), emphasis on
	  non-ISP2[12]00 chips.
	o Add initial ISP6312/ISP6322 definitions and checks during
	  board configuration.
	o Only retrieve firmware version and resource counts if
	  we've successfully initialized the firmware.
	o Ensure the driver is logged into the SNS port before
	  performing any registrations.
	o Always use MMIO for chip register accesses.
	  - Remove MEMORY_MAPPED_IO define.
	o Address FLASH deficiencies with ISP2312 v2 chips.
	o Fix more PCI posting issues.
	o Wait for at most MAX_LOOP_TIMEOUT in
	  qla2x00_wait_for_hba_online().
	o Generalize descriptive name used for the driver
	  (i.e. QLogic ISP2xxx FC Driver --> QLogic FC Driver).
	o Remove unused debug routines (ENTER/LEAVE(), etc.).
	o Add new 2300/2322 IPX and 6312/6322 FLX firmwares (3.02.20):
	  - Remove 2300 TPX firmware from distribution.

In the future, now that we (I) have the process down and the driver is
in fact in the tree in some form, reasonbly sized patches will follow
after 8.00.00b9.

James:  I'll also pass this email onto linux-scsi.

Thanks again,
Andrew Vasquez

Re: pulling failover out of qla2xxx

[Christoph Hellwig]

> 	o Add initial ISP6312/ISP6322 definitions and checks during
> 	  board configuration.

> 	o Add new 2300/2322 IPX and 6312/6322 FLX firmwares (3.02.20):
> 	  - Remove 2300 TPX firmware from distribution.

Please add a frontend module per firmware variant instead of adding
them to qla2300.ko - the whole point of these frontend modules is to
avoid having to load unessecary firmware.

Else the update looks okay.

RE: pulling failover out of qla2xxx

[Andrew Vasquez]

On Friday, January 16, 2004 4:07 AM, Christoph Hellwig wrote:
> > 	o Add initial ISP6312/ISP6322 definitions and checks during
> > 	  board configuration.
> 
> > 	o Add new 2300/2322 IPX and 6312/6322 FLX firmwares (3.02.20):
> > 	  - Remove 2300 TPX firmware from distribution.
> 
> Please add a frontend module per firmware variant instead of adding
> them to qla2300.ko - the whole point of these frontend modules is to
> avoid having to load unessecary firmware.
> 

One of the common complaints we've received from users of our HBA
products is that there are currently three _different_ drivers to
support the current line of ISP chips, ISP2100 - qla2100.o, ISP22XX -
qla2200.o, and ISP2300/2310/2312 - qla2300.o.

I originally mentioned this problem during early discussions with you
before you began the qla2xxx lib/isp split.  QLogic's directive then
and now is that from point-forward (ISP23XX), all future ISP chip
support would be added and exported in a single driver module rather
than the previous qlaABXX.o fashion.  Currently that module, while
perhaps incorrectly named, is qla2300.o.

I'll open up a request-for-response from the rest of the listers in
asking if parceling out qlaAB00.ko modules based on firmware types is
both practical and sensible from a user standpoint.  Especially
considering that many users aren't really concerned with the ISP chip
type on their QLAXXXX or OEM-BRANDED ABC host bus adapter.

Now from a development and maintenance standpoint, I can understand
the reluctance of having to accept a 100K file for each ISP type.
But, let me just say QLogic is looking into other ways of distributing
firmware images.  Finally, given the addition of some new binary-load
user->kernel space facilities (namely request_firmware()) the space
issue becomes moot.

> Else the update looks okay.

Thanks again for all your work at shaping the qla2xxx driver for
inclusion into 2.6.  I'd like to extend a personal thanks to you for
all your efforts, and, when I'm in the area of EU again, perhaps offer
to buy you a beer.

Thanks,
Andrew Vasquez

Re: pulling failover out of qla2xxx

[Jeff Garzik]

Andrew Vasquez wrote:
> I'll open up a request-for-response from the rest of the listers in
> asking if parceling out qlaAB00.ko modules based on firmware types is
> both practical and sensible from a user standpoint.  Especially
> considering that many users aren't really concerned with the ISP chip
> type on their QLAXXXX or OEM-BRANDED ABC host bus adapter.


Speaking from a generic driver / distro standpoint, module dependencies 
and PCI tables (i.e. pci_device_id and MODULE_DEVICE_TABLE) all things 
to magically Just Work :)

I heartily encourage the creation of "library modules", which are loaded 
automatically as dependencies when one issues a 'modprobe qla2345' 
Distributions have tools that will notice PCI id tables in the 
small/firmware modules, and will load qla2345.ko.  But the tools will 
also notice the dependent module qla2xxx.ko (or whatever the generic one 
is).  Heck, if there is HBA-chip-specific code in qla2xxx, move that out 
to the specific driver's module too.

	Jeff

Re: pulling failover out of qla2xxx

[Arjan van de Ven]

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

On Fri, Jan 16, 2004 at 12:02:50PM -0800, Andrew Vasquez wrote:
> I'll open up a request-for-response from the rest of the listers in
> asking if parceling out qlaAB00.ko modules based on firmware types is
> both practical and sensible from a user standpoint.  Especially
> considering that many users aren't really concerned with the ISP chip
> type on their QLAXXXX or OEM-BRANDED ABC host bus adapter.

that is what operating system installers are for (and udev/hotplug), modules
load automatically based on the PCI table nowadays ;)

Another approach could be separate modules which ONLY have firmware, one
module per firmware file (but that gets you really close to
request_firmare() api)



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

Re: pulling failover out of qla2xxx

[Jeff Garzik]

On Sat, Jan 17, 2004 at 12:33:28AM +0100, Arjan van de Ven wrote:
> Another approach could be separate modules which ONLY have firmware, one
> module per firmware file (but that gets you really close to
> request_firmare() api)

That's what the upstream tree has right now...  here's the complete
contents of ql2100.c...


/*
 * QLogic ISP2100 device driver for Linux 2.6.x
 * Copyright (C) 2003 Christoph Hellwig.
 * Copyright (C) 2003 QLogic Corporation (www.qlogic.com)
 *
 * Released under GPL v2.
 */

#include <linux/init.h>
#include <linux/module.h>
#include <linux/pci.h>

#include "qla_os.h"
#include "qla_def.h"

static char qla_driver_name[] = "qla2100";

extern unsigned char  fw2100tp_version[];
extern unsigned char  fw2100tp_version_str[];
extern unsigned short fw2100tp_addr01;
extern unsigned short fw2100tp_code01[];
extern unsigned short fw2100tp_length01;

static struct qla_fw_info qla_fw_tbl[] = {
	{
		.addressing	= FW_INFO_ADDR_NORMAL,
		.fwcode		= &fw2100tp_code01[0],
		.fwlen		= &fw2100tp_length01,
		.fwstart	= &fw2100tp_addr01,
	},

	{ FW_INFO_ADDR_NOMORE, },
};

static struct qla_board_info qla_board_tbl = {
	.drv_name	= qla_driver_name,

	.isp_name	= "ISP2100",
	.fw_info	= qla_fw_tbl,
};

static struct pci_device_id qla2100_pci_tbl[] = {
	{
		.vendor		= PCI_VENDOR_ID_QLOGIC,
		.device		= PCI_DEVICE_ID_QLOGIC_ISP2100,
		.subvendor	= PCI_ANY_ID,
		.subdevice	= PCI_ANY_ID,
		.driver_data	= (unsigned long)&qla_board_tbl,
	},

	{0, 0},
};
MODULE_DEVICE_TABLE(pci, qla2100_pci_tbl);

static int __devinit
qla2100_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
{
	return qla2x00_probe_one(pdev,
	    (struct qla_board_info *)id->driver_data);
}

static void __devexit
qla2100_remove_one(struct pci_dev *pdev)
{
	qla2x00_remove_one(pdev);
}

static struct pci_driver qla2100_pci_driver = {
	.name		= "qla2100",
	.id_table	= qla2100_pci_tbl,
	.probe		= qla2100_probe_one,
	.remove		= __devexit_p(qla2100_remove_one),
};

static int __init
qla2100_init(void)
{
	return pci_module_init(&qla2100_pci_driver);
}

static void __exit
qla2100_exit(void)
{
	pci_unregister_driver(&qla2100_pci_driver);
}

module_init(qla2100_init);
module_exit(qla2100_exit);

MODULE_AUTHOR("QLogic Corporation");
MODULE_DESCRIPTION("QLogic ISP21xx FC-SCSI Host Bus Adapter driver");
MODULE_LICENSE("GPL");

Re: pulling failover out of qla2xxx

[Christoph Hellwig]

On Fri, Jan 16, 2004 at 12:02:50PM -0800, Andrew Vasquez wrote:
> Now from a development and maintenance standpoint, I can understand
> the reluctance of having to accept a 100K file for each ISP type.
> But, let me just say QLogic is looking into other ways of distributing
> firmware images.  Finally, given the addition of some new binary-load
> user->kernel space facilities (namely request_firmware()) the space
> issue becomes moot.

If we go to request_firmware the frontend modules are mood and we should
just kill all of them.

Re: pulling failover out of qla2xxx

[Christoph Hellwig]

On Fri, Jan 16, 2004 at 03:24:00PM -0500, Jeff Garzik wrote:
> is).  Heck, if there is HBA-chip-specific code in qla2xxx, move that out 
> to the specific driver's module too.

Well, there is quite a bit of code for just one HBA type, but it's
always a few lines of code in large common codepathes (and a slightly
different register layout).  We though about this but it's not easily
feasible.

Re: pulling failover out of qla2xxx

[Arjan van de Ven]

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

On Sat, Jan 17, 2004 at 05:52:26PM +0100, Christoph Hellwig wrote:
> On Fri, Jan 16, 2004 at 12:02:50PM -0800, Andrew Vasquez wrote:
> > Now from a development and maintenance standpoint, I can understand
> > the reluctance of having to accept a 100K file for each ISP type.
> > But, let me just say QLogic is looking into other ways of distributing
> > firmware images.  Finally, given the addition of some new binary-load
> > user->kernel space facilities (namely request_firmware()) the space
> > issue becomes moot.
> 
> If we go to request_firmware the frontend modules are mood and we should
> just kill all of them.

the "problem" for now is booting from qlogic; initrd's in current distros
don't work with request_firmware yet... while I'm sure this is a temporary
inconvenience, it is a real one. And it's not like the frontend modules are
THAT huge to keep around for a while until enough distros have this working
well.

Greetings,
    Arjan van de Ven

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

Re: pulling failover out of qla2xxx

[Christoph Hellwig]

On Sat, Jan 17, 2004 at 07:47:37PM +0100, Arjan van de Ven wrote:
> the "problem" for now is booting from qlogic; initrd's in current distros
> don't work with request_firmware yet... while I'm sure this is a temporary
> inconvenience, it is a real one. And it's not like the frontend modules are
> THAT huge to keep around for a while until enough distros have this working
> well.

Well, we'll have to keep them around until everything works of course.
As one module per firmware as currently in mainline.

Re: pulling failover out of qla2xxx

[Andrew Vasquez]

On Fri, 16 Jan 2004, Jeff Garzik wrote:

> >I'll open up a request-for-response from the rest of the listers in
> >asking if parceling out qlaAB00.ko modules based on firmware types is
> >both practical and sensible from a user standpoint.  Especially
> >considering that many users aren't really concerned with the ISP chip
> >type on their QLAXXXX or OEM-BRANDED ABC host bus adapter.
> 
> 
> Speaking from a generic driver / distro standpoint, module dependencies 
> and PCI tables (i.e. pci_device_id and MODULE_DEVICE_TABLE) all things 
> to magically Just Work :)
> 

I certainly hope this to be the case.  Perhaps I'm still viewing this
with an eye as an external module maintainer.  When a user compiles
the qla2xxx driver she's given the following options:

	...
        [ ] QLogic ISP2100 host adapter family support
        [ ] QLogic ISP2200 host adapter family support
        [ ] QLogic ISP2300 host adapter family support
        [ ] QLogic ISP2322 host adapter family support
        [ ] QLogic ISP6312 host adapter family support
        [ ] QLogic ISP6322 host adapter family support
	...

how will she know which option to select for her QLA2362 HBA?  Will
she know it's based on an ISP2322 chip and properly select the correct
CONFIG option?  How about for a QLA2342 card?  BTW: it's based on the
ISP2312 chip so one must select the ISP2300 CONFIG option.

The CONFIG list will just get longer as new ISP chips come out.

> I heartily encourage the creation of "library modules", which are loaded 

Library modules seems fine, but, as you noted in a later email, the
qla2100/qla2200.ko modules are mostly wrappers for firmware loads.

Regards,
Andrew Vasquez

Re: pulling failover out of qla2xxx

[Arjan van de Ven]

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

On Sat, 2004-01-17 at 21:52, Andrew Vasquez wrote:
> On Fri, 16 Jan 2004, Jeff Garzik wrote:
> 
> > >I'll open up a request-for-response from the rest of the listers in
> > >asking if parceling out qlaAB00.ko modules based on firmware types is
> > >both practical and sensible from a user standpoint.  Especially
> > >considering that many users aren't really concerned with the ISP chip
> > >type on their QLAXXXX or OEM-BRANDED ABC host bus adapter.
> > 
> > 
> > Speaking from a generic driver / distro standpoint, module dependencies 
> > and PCI tables (i.e. pci_device_id and MODULE_DEVICE_TABLE) all things 
> > to magically Just Work :)
> > 
> 
> I certainly hope this to be the case.  Perhaps I'm still viewing this
> with an eye as an external module maintainer.  When a user compiles
> the qla2xxx driver she's given the following options:
> 
> 	...
>         [ ] QLogic ISP2100 host adapter family support
>         [ ] QLogic ISP2200 host adapter family support
>         [ ] QLogic ISP2300 host adapter family support
>         [ ] QLogic ISP2322 host adapter family support
>         [ ] QLogic ISP6312 host adapter family support
>         [ ] QLogic ISP6322 host adapter family support
> 	...
> 
> how will she know which option to select for her QLA2362 HBA?  

fwiw there is no rule to have a Kconfig line for each driver; you could
just have one "QLogic fiber channel drivers" question, maybe with a
subquestion about "enable all chipsets" with a pull down thingy for
selected ones if the user wants to pick and choose. There is also help
text for more info.
(and most people who have qlogic fiber adapters will just say "m" to
all, as will distros)

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

Re: pulling failover out of qla2xxx

[Andrew Vasquez]

On Sat, 17 Jan 2004, Arjan van de Ven wrote:

> On Sat, 2004-01-17 at 21:52, Andrew Vasquez wrote:
> > I certainly hope this to be the case.  Perhaps I'm still viewing this
> > with an eye as an external module maintainer.  When a user compiles
> > the qla2xxx driver she's given the following options:
> > 
> > 	...
> >         [ ] QLogic ISP2100 host adapter family support
> >         [ ] QLogic ISP2200 host adapter family support
> >         [ ] QLogic ISP2300 host adapter family support
> >         [ ] QLogic ISP2322 host adapter family support
> >         [ ] QLogic ISP6312 host adapter family support
> >         [ ] QLogic ISP6322 host adapter family support
> > 	...
> > 
> > how will she know which option to select for her QLA2362 HBA?  
> 
> fwiw there is no rule to have a Kconfig line for each driver; you could
> just have one "QLogic fiber channel drivers" question, maybe with a
> subquestion about "enable all chipsets" with a pull down thingy for
> selected ones if the user wants to pick and choose. There is also help
> text for more info.

Funny, that's how the Kconfig format was laid out in earlier drivers
8.00.00b7):

	config SCSI_QLA2XXX
		bool "QLogic ISP2xxx PCI/PCI-X Fibre Channel HBA Support"
		depends on PCI && SCSI
		---help---
		These drivers support the QLogic 2xxx host adapter family of
		SCSI FCP HBAs.

	config SCSI_QLA21XX
		tristate "ISP2100 host adapter family support"
		depends on SCSI_QLA2XXX
		---help---
		This driver supports the QLogic 21xx (ISP2100) host adapter
		family.

	config SCSI_QLA22XX
		tristate "ISP2200 host adapter family support"
		depends on SCSI_QLA2XXX

	...

There seemed to be some resistence to that and in the end it was
changed to the current form.

> (and most people who have qlogic fiber adapters will just say "m" to
> all, as will distros)

I'll just throw this out, if in fact as you say request_firmware()
won't work with the current initrd's used by the distros then perhaps
we could simply build the firmware images as modules (minus the
per-firmware loader) and have a generic qlafc.ko (or whatever the name
you'd like) loader that the user would modprobe. The qlafc driver
would then register the pci_device_id tables for all ISP chips and
wait for the pci_driver callback to signify which firmwares modules to
load.  Load the firmware, call probe(), then move on.  

Nothing fancy or genuine, but perhaps a smooth transition to when in
fact request_firmware() will do.

--
Andrew

Re: pulling failover out of qla2xxx

[Arjan van de Ven]

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

On Sat, 2004-01-17 at 22:52, Andrew Vasquez wrote:

> I'll just throw this out, if in fact as you say request_firmware()
> won't work with the current initrd's used by the distros then perhaps
> we could simply build the firmware images as modules (minus the
> per-firmware loader) and have a generic qlafc.ko (or whatever the name
> you'd like) loader that the user would modprobe. The qlafc driver
> would then register the pci_device_id tables for all ISP chips and
> wait for the pci_driver callback to signify which firmwares modules to
> load.  Load the firmware, call probe(), then move on.  

this breaks those initrd's just as much.
The current approach actually is pretty clean (module dependencies pull
everything in etc etc) and I *really* don't see the objection to it.
Sure the request_firmware() interface is nicer but it's not like having
the a few frontend modules is actually a problem; all distros just do
the right thing with these, even for custom compiled kernels. 


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