[RFC] results of endianness review of qla2xxx

[RFC] results of endianness review of qla2xxx

[Al Viro]

Assorted endianness problems and uncertain places:

1) In qla24xx_nvram_config():
	    nv->nvram_version < __constant_cpu_to_le16(ICB_VERSION)) {
comparisons work better in host-endian...

2) Same function:
	icb->login_timeout = cpu_to_le16(nv->login_timeout);
both are little-endian

3) qla24xx_login_fabric():
	lg->vp_index = cpu_to_le16(ha->vp_idx);
->vp_index is 8bit, ha->vp_idx is host-endian (and small)

4) qla24xx_fabric_logout():
same story.

5) qla24xx_report_id_acquisition():
	if (rptid_entry->entry_status != 0)
		return;
	if (rptid_entry->entry_status != __constant_cpu_to_le16(CS_COMPLETE))
		return;
For one thing, ->entry_status is 8bit.  For another, CS_COMPLETE is 0, so this
pair of checks looks bogus anyway.

6) Same function:
		vp_idx = LSB(rptid_entry->vp_idx);
->vp_idx is little-endian 16bit; LSB expects host-endian
		if (MSB(rptid_entry->vp_idx) == 1)
... and so does MSB.

7) qla2x00_async_event():
		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
		...
		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
		handles[1] = le32_to_cpu(
		    ((uint32_t)(RD_MAILBOX_REG(ha, reg, 7) << 16)) |
and then those values are passed to qla2x00_process_completed_request() which
expects a host-endian (and uses it as index in array, among other things).

8) qla2x00_fdmi_rpa():
	max_frame_size = IS_FWI2_CAPABLE(ha) ?
		(uint32_t) icb24->frame_payload_size:
		(uint32_t) ha->init_cb->frame_payload_size;
	eiter->a.max_frame_size = cpu_to_be32(max_frame_size);
and AFAICS ->frame_payload_size is little-endian 16bit.

9) qla2x00_clear_nvram_protection():
        wprot_old = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
	stat = qla2x00_write_nvram_word_tmo(ha, ha->nvram_base,
		    __constant_cpu_to_le16(0x1234), 100000);
	wprot = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
	if (stat != QLA_SUCCESS || wprot != 0x1234) {
Odd, since qla2x00_get_nvram_word() returns a host-endian and 
qla2x00_write_nvram_word_tmo() expects one (this stuff is bit-banging
16bit values through).

10)
typedef struct vf_id {
        uint16_t id : 12;
	uint16_t priority : 4;
} vf_id_t;

and

typedef struct vf_hopct {
	uint16_t reserved : 8;
	uint16_t hopct : 8;
} vf_hopct_t;

Buggered, since (a) vf_id is severely endian-dependent and (b) both will happily
get padding on e.g. arm; since they are embedded into hardware-shared structures,
extra 16 bits tacked onto each is Not Good(tm).

11) The same changeset that had introduced vf_id has a related oddity:

-       uint8_t reserved_4[32];
+       uint16_t flags;
+       struct vf_id    id;
+       uint16_t reserved_4;
+       struct vf_hopct  hopct;
+       uint8_t reserved_5[8];

had been bogus - 2 + 2 + 2 + 2 + 8 != 32; 16 bytes got cut.

Re: [RFC] results of endianness review of qla2xxx

[Andrew Vasquez]

On Wed, 16 Apr 2008, Al Viro wrote:

> Assorted endianness problems and uncertain places:
> 
> 1) In qla24xx_nvram_config():
> 	    nv->nvram_version < __constant_cpu_to_le16(ICB_VERSION)) {
> comparisons work better in host-endian...
> 
> 2) Same function:
> 	icb->login_timeout = cpu_to_le16(nv->login_timeout);
> both are little-endian
> 
> 3) qla24xx_login_fabric():
> 	lg->vp_index = cpu_to_le16(ha->vp_idx);
> ->vp_index is 8bit, ha->vp_idx is host-endian (and small)
> 
> 4) qla24xx_fabric_logout():
> same story.
> 
> 5) qla24xx_report_id_acquisition():
> 	if (rptid_entry->entry_status != 0)
> 		return;
> 	if (rptid_entry->entry_status != __constant_cpu_to_le16(CS_COMPLETE))
> 		return;
> For one thing, ->entry_status is 8bit.  For another, CS_COMPLETE is 0, so this
> pair of checks looks bogus anyway.
> 
> 6) Same function:
> 		vp_idx = LSB(rptid_entry->vp_idx);
> ->vp_idx is little-endian 16bit; LSB expects host-endian
> 		if (MSB(rptid_entry->vp_idx) == 1)
> ... and so does MSB.

Thanks, we'll take a look at these and provide relevant fixes.

> 7) qla2x00_async_event():
> 		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
> 		...
> 		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
> 		handles[1] = le32_to_cpu(
> 		    ((uint32_t)(RD_MAILBOX_REG(ha, reg, 7) << 16)) |
> and then those values are passed to qla2x00_process_completed_request() which
> expects a host-endian (and uses it as index in array, among other things).

THis is correct (though a bit confuscated), each of the two 16bit
mailbox-registers makes-up a 32bit LE value, which is converted to
cpu-endian and passed to qla2x00_process_completed_request().

> 8) qla2x00_fdmi_rpa():
> 	max_frame_size = IS_FWI2_CAPABLE(ha) ?
> 		(uint32_t) icb24->frame_payload_size:
> 		(uint32_t) ha->init_cb->frame_payload_size;
> 	eiter->a.max_frame_size = cpu_to_be32(max_frame_size);
> and AFAICS ->frame_payload_size is little-endian 16bit.

Yes indeed, le16_to_cpu() need to surround the
init_cb/icb24->frame_payload_size references.

> 9) qla2x00_clear_nvram_protection():
>         wprot_old = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
> 	stat = qla2x00_write_nvram_word_tmo(ha, ha->nvram_base,
> 		    __constant_cpu_to_le16(0x1234), 100000);
> 	wprot = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
> 	if (stat != QLA_SUCCESS || wprot != 0x1234) {
> Odd, since qla2x00_get_nvram_word() returns a host-endian and 
> qla2x00_write_nvram_word_tmo() expects one (this stuff is bit-banging
> 16bit values through).

Odd, yes.  I was never happy with this endianess intermixing, but
after numerous cycles within our qual-testing, this code works in both
big and little-endian systems...


> 10)
> typedef struct vf_id {
>         uint16_t id : 12;
> 	uint16_t priority : 4;
> } vf_id_t;
> 
> and
> 
> typedef struct vf_hopct {
> 	uint16_t reserved : 8;
> 	uint16_t hopct : 8;
> } vf_hopct_t;
> 
> Buggered, since (a) vf_id is severely endian-dependent and (b) both will happily
> get padding on e.g. arm; since they are embedded into hardware-shared structures,
> extra 16 bits tacked onto each is Not Good(tm).
> 
> 11) The same changeset that had introduced vf_id has a related oddity:
> 
> -       uint8_t reserved_4[32];
> +       uint16_t flags;
> +       struct vf_id    id;
> +       uint16_t reserved_4;
> +       struct vf_hopct  hopct;
> +       uint8_t reserved_5[8];
> 
> had been bogus - 2 + 2 + 2 + 2 + 8 != 32; 16 bytes got cut.

We'll look into these as well...

Thanks,
AV

Re: [RFC] results of endianness review of qla2xxx

[Al Viro]

On Fri, Apr 18, 2008 at 11:17:58AM -0700, Andrew Vasquez wrote:
> > 7) qla2x00_async_event():
> > 		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
> > 		...
> > 		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
> > 		handles[1] = le32_to_cpu(
> > 		    ((uint32_t)(RD_MAILBOX_REG(ha, reg, 7) << 16)) |
> > and then those values are passed to qla2x00_process_completed_request() which
> > expects a host-endian (and uses it as index in array, among other things).
> 
> THis is correct (though a bit confuscated), each of the two 16bit
> mailbox-registers makes-up a 32bit LE value, which is converted to
> cpu-endian and passed to qla2x00_process_completed_request().

Um, no.  First of all, other branches in that switch clearly assume
that mb[] is already host-endian.  But leaving that aside, what you
are doing here will _not_ work even if mb[1] and mb[2] are l-e in
this case.

Look what happens:
* l-e host:
	handles[0] = (mb[2] << 16) | mb[1], which is equal to
(le16_to_cpu(mb[2]) << 16) | le16_to_cpu(mb[1])
* on b-e host:
	handles[0] = swab32((mb[2] << 16) | mb[1]), which is equal to
(swab16(mb[1]) << 16) | swab16(mb[2]), which is equal to
(le16_to_cpu(mb[1]) << 16) | le16_to_cpu(mb[2])

IOW, resulting host-endian value will be different with the same contents
in mb[].  Put it another way, suppose you have mb[1] == 0, mb[2] == 0x101.
Then on l-e you'll get handles[0] == 0x10100 and on b-e - 0x101.  The latter
will pass the check for index being too high, the former will fail...

BTW, RD_MAILBOX_REG() boils down to readw(), which is definitely host-endian.
And we have
                        mb[0] = MBA_SCSI_COMPLETION;
                        mb[1] = MSW(stat);
                        mb[2] = RD_MAILBOX_REG(ha, reg, 2);
                        qla2x00_async_event(ha, mb);
so there's at least one path reaching qla2x00_async_event() with mb[0]
equal to MBA_SCSI_COMPLETION and mb[2] containing host-endian...  Looking
at the contents of stat and the things we do to it (switch (stat & 0xff) right
outside that code), mb[1] is host-endian as well...  AFAICS, the same holds
for all codepaths - contents of mb[] is host-endian.

> > 9) qla2x00_clear_nvram_protection():
> >         wprot_old = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
> > 	stat = qla2x00_write_nvram_word_tmo(ha, ha->nvram_base,
> > 		    __constant_cpu_to_le16(0x1234), 100000);
> > 	wprot = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
> > 	if (stat != QLA_SUCCESS || wprot != 0x1234) {
> > Odd, since qla2x00_get_nvram_word() returns a host-endian and 
> > qla2x00_write_nvram_word_tmo() expects one (this stuff is bit-banging
> > 16bit values through).
> 
> Odd, yes.  I was never happy with this endianess intermixing, but
> after numerous cycles within our qual-testing, this code works in both
> big and little-endian systems...

It looks like check that location is writable; what happens if you replace
the first instance with 0x1234 and the second (in wprot == ...) with
cpu_to_le16(0x1234)?  One "we won't find it there by accident, must've been
successful write" value seems to be as good as other, unless something else
is going on...

Re: [RFC] results of endianness review of qla2xxx

[Al Viro]

	More of that stuff (added by [SCSI] qla2xxx: Add hardware
trace-logging support):
                        qla24xx_write_flash_dword(ha, faddr++,
                            cpu_to_le32(jiffies));
                        qla24xx_write_flash_dword(ha, faddr++, 0);
                        qla24xx_write_flash_dword(ha, faddr++, *fdata++);
                        qla24xx_write_flash_dword(ha, faddr++, *fdata);
in qla2xxx_hw_event_store().  All other callers pass host-endian last
argument and function sure as hell looks like it's expecting one...

Re: [RFC] results of endianness review of qla2xxx

[Al Viro]

... and while we are at it,
                        for (miter = 0, s = optrom, d = dwptr;
                            miter < OPTROM_BURST_DWORDS; miter++, s++, d++)
                                *s = cpu_to_le32(*d);

                        ret = qla2x00_load_ram(ha, optrom_dma,
                            flash_data_to_access_addr(faddr),
                            OPTROM_BURST_DWORDS);

in qla24xx_write_flash_data() looks bloody odd.  We have dwptr pointing
to fixed-endian data, so conversion gets us host-endian and then we pass
the area filled with results of conversion for DMA?

And yes, dwptr points to fixed-endian - it comes from
qla24xx_write_optrom_data(struct scsi_qla_host *ha, uint8_t *buf,
    uint32_t offset, uint32_t length)
{
...
        rval = qla24xx_write_flash_data(ha, (uint32_t *)buf, offset >> 2,
            length >> 2);

Comments?

Re: [RFC] results of endianness review of qla2xxx

[Andrew Vasquez]

On Fri, 18 Apr 2008, Al Viro wrote:

> On Fri, Apr 18, 2008 at 11:17:58AM -0700, Andrew Vasquez wrote:
> > > 7) qla2x00_async_event():
> > > 		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
> > > 		...
> > > 		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
> > > 		handles[1] = le32_to_cpu(
> > > 		    ((uint32_t)(RD_MAILBOX_REG(ha, reg, 7) << 16)) |
> > > and then those values are passed to qla2x00_process_completed_request() which
> > > expects a host-endian (and uses it as index in array, among other things).
> > 
> > THis is correct (though a bit confuscated), each of the two 16bit
> > mailbox-registers makes-up a 32bit LE value, which is converted to
> > cpu-endian and passed to qla2x00_process_completed_request().
> 
> Um, no.  First of all, other branches in that switch clearly assume
> that mb[] is already host-endian.  But leaving that aside, what you
> are doing here will _not_ work even if mb[1] and mb[2] are l-e in
> this case.
> 
> Look what happens:
> * l-e host:
> 	handles[0] = (mb[2] << 16) | mb[1], which is equal to
> (le16_to_cpu(mb[2]) << 16) | le16_to_cpu(mb[1])
> * on b-e host:
> 	handles[0] = swab32((mb[2] << 16) | mb[1]), which is equal to
> (swab16(mb[1]) << 16) | swab16(mb[2]), which is equal to
> (le16_to_cpu(mb[1]) << 16) | le16_to_cpu(mb[2])
> 
> IOW, resulting host-endian value will be different with the same contents
> in mb[].  Put it another way, suppose you have mb[1] == 0, mb[2] == 0x101.
> Then on l-e you'll get handles[0] == 0x10100 and on b-e - 0x101.  The latter
> will pass the check for index being too high, the former will fail...
> 
> BTW, RD_MAILBOX_REG() boils down to readw(), which is definitely host-endian.
> And we have
>                         mb[0] = MBA_SCSI_COMPLETION;
>                         mb[1] = MSW(stat);
>                         mb[2] = RD_MAILBOX_REG(ha, reg, 2);
>                         qla2x00_async_event(ha, mb);
> so there's at least one path reaching qla2x00_async_event() with mb[0]
> equal to MBA_SCSI_COMPLETION and mb[2] containing host-endian...  Looking
> at the contents of stat and the things we do to it (switch (stat & 0xff) right
> outside that code), mb[1] is host-endian as well...  AFAICS, the same holds
> for all codepaths - contents of mb[] is host-endian.

Yes, all mb[] "values" are going to be in host-endian format.

But, what's missing here is an understanding of what mb[2] and mb[1]
are representing during MBA_SCSI_COMPLETION and MBA_CMPLT_2_32BIT
handling.  Upon command submission, a 32bit handle (cookie) is
associated with each IOCB:

	...
	cmd_pkt = (cmd_entry_t *)ha->request_ring_ptr;
	cmd_pkt->handle = handle;
	/* Zero out remaining portion of packet. */
	clr_ptr = (uint32_t *)cmd_pkt + 2;
	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
	cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);

	/* Set target ID and LUN number*/
	SET_TARGET_ID(ha, cmd_pkt->target, sp->fcport->loop_id);
	cmd_pkt->lun = cpu_to_le16(sp->cmd->device->lun);
	...

Notice that the 'handle'-value is assigned to the command-IOCB
structure in host-endian format and *not* the typical little-endian
form used to fill-in the other non-byte-sized members of the
structure.  So, for example, start with a hypothetical 'handle' value
of 0x12345678.  On an BE machine, the layout in memory of the value
written to the structure will be:

	mem[0]	mem[1]	mem[2]	mem[3]
	------	------	-----	------
	12	34	56	78

On an LE machine:

	mem[0]	mem[1]	mem[2]	mem[3]
	------	------	-----	------
	78	56	34	12

'handle' sent as a host-endian value is done for a reason, as
typically, firmware reports IOCB completion via a structure called the
status-IOCB [2]:

	#define STATUS_TYPE     0x03            /* Status entry. */
	typedef struct {
		uint8_t entry_type;             /* Entry type. */
		uint8_t entry_count;            /* Entry count. */
		uint8_t sys_define;             /* System defined. */
		uint8_t entry_status;           /* Entry Status. */
		uint32_t handle;                /* System handle. */
		uint16_t scsi_status;           /* SCSI status. */
		...

where, during creation of the status-IOCB by firmware, the
'handle'-value (at offset 0x4) is copied from the 'handle' specified
from the command-IOCB.  Since this 'handle' is a a simple index (in
host-endian format) into the outstanding_cmds array of the HBA,
there's really no reason to convert the value to little-endian format
at submission time, then convert from little-endian to host-endian
form during status-IOCB handling within the driver [3]:

	...
	/* Fast path completion. */
	if (comp_status == CS_COMPLETE && scsi_status == 0) {
		qla2x00_process_completed_request(ha, sts->handle);

		return;
	}

So, getting back to MBA_SCSI_COMPLETION and MBA_CMPLT_2_32BIT
completions, the "value" of the mailbox registers returned by firmware
are:

	mb[1]: mem[1] << 8 | mem[0]
	mb[2]: mem[3] << 8 | mem[2]

which on an LE machine result in:

	mb[1]: 0x5678
	mb[2]: 0x1234

	handle[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
	handle[0] = (0x1234 << 16) | 0x5678)
	handle[0] = 0x12345678

and on an BE machine the values are:

	mb[1]: 0x3412
	mb[2]: 0x7856

	handle[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
	handle[0] = swab32((0x7856 << 16) | 0x3412)
	handle[0] = swab32(0x78563412)
	handle[0] = 0x12345678

> > > 9) qla2x00_clear_nvram_protection():
> > >         wprot_old = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
> > > 	stat = qla2x00_write_nvram_word_tmo(ha, ha->nvram_base,
> > > 		    __constant_cpu_to_le16(0x1234), 100000);
> > > 	wprot = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
> > > 	if (stat != QLA_SUCCESS || wprot != 0x1234) {
> > > Odd, since qla2x00_get_nvram_word() returns a host-endian and 
> > > qla2x00_write_nvram_word_tmo() expects one (this stuff is bit-banging
> > > 16bit values through).
> > 
> > Odd, yes.  I was never happy with this endianess intermixing, but
> > after numerous cycles within our qual-testing, this code works in both
> > big and little-endian systems...
> 
> It looks like check that location is writable; what happens if you replace
> the first instance with 0x1234 and the second (in wprot == ...) with
> cpu_to_le16(0x1234)?  One "we won't find it there by accident, must've been
> successful write" value seems to be as good as other, unless something else
> is going on...

One would hope so, yes...

Anyway, hope that helped...

--
av

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/scsi/qla2xxx/qla_iocb.c;h=5489d5024673a326b5345f5545151a7dab37a533;hb=HEAD#l346

[2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/scsi/qla2xxx/qla_def.h;h=094d95f0764c6027cb421e4d3e5da9bf17012dfd;hb=HEAD#l1259

[3] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/scsi/qla2xxx/qla_isr.c;h=285479b62d8f02084ad0daebc12ab249f16348c0;hb=HEAD#l944

Re: [RFC] results of endianness review of qla2xxx

[Andrew Vasquez]

On Sat, 19 Apr 2008, Al Viro wrote:

> 	More of that stuff (added by [SCSI] qla2xxx: Add hardware
> trace-logging support):
>                         qla24xx_write_flash_dword(ha, faddr++,
>                             cpu_to_le32(jiffies));
>                         qla24xx_write_flash_dword(ha, faddr++, 0);
>                         qla24xx_write_flash_dword(ha, faddr++, *fdata++);
>                         qla24xx_write_flash_dword(ha, faddr++, *fdata);
> in qla2xxx_hw_event_store().

The data portions of 'fdata[]' are composites of:

	#define QMARK(a, b, c, d) \
	    cpu_to_le32(LSB(a) << 24 | LSB(b) << 16 | LSB(c) << 8 | LSB(d))
	...
	/* Create marker. */
	marker[0] = QMARK('L', ha->fw_major_version,
	    ha->fw_minor_version, ha->fw_subminor_version);
	marker[1] = QMARK(QLA_DRIVER_MAJOR_VER, QLA_DRIVER_MINOR_VER,
	    QLA_DRIVER_PATCH_VER, QLA_DRIVER_BETA_VER);

and:

        /* Store error.  */
        fdata[0] = cpu_to_le32(code << 16 | d1);
        fdata[1] = cpu_to_le32(d2 << 16 | d3);
        rval = qla2xxx_hw_event_store(ha, fdata);

> All other callers pass host-endian last
> argument and function sure as hell looks like it's expecting one...

I'm not entirely clear on what you are driving at here...  The data in
question is written to flash in little-endian format...

Re: [RFC] results of endianness review of qla2xxx

[Andrew Vasquez]

On Sat, 19 Apr 2008, Al Viro wrote:

> ... and while we are at it,
>                         for (miter = 0, s = optrom, d = dwptr;
>                             miter < OPTROM_BURST_DWORDS; miter++, s++, d++)
>                                 *s = cpu_to_le32(*d);
> 
>                         ret = qla2x00_load_ram(ha, optrom_dma,
>                             flash_data_to_access_addr(faddr),
>                             OPTROM_BURST_DWORDS);
> 
> in qla24xx_write_flash_data() looks bloody odd.  We have dwptr pointing
> to fixed-endian data, so conversion gets us host-endian and then we pass
> the area filled with results of conversion for DMA?
> 
> And yes, dwptr points to fixed-endian - it comes from
> qla24xx_write_optrom_data(struct scsi_qla_host *ha, uint8_t *buf,
>     uint32_t offset, uint32_t length)
> {
> ...
>         rval = qla24xx_write_flash_data(ha, (uint32_t *)buf, offset >> 2,
>             length >> 2);
> 
> Comments?

Sure, you're not understanding the format of the various pieces of
data as they flow through the driver (often times opaquely)...

Look at this code from qla_init.c:qla24xx_load_risc_flash():

	...
	qla24xx_read_flash_data(ha, dcode, faddr, dlen);
	for (i = 0; i < dlen; i++)
		dcode[i] = swab32(dcode[i]);

	rval = qla2x00_load_ram(ha, ha->request_dma, risc_addr, dlen);
	...

why the explicit swab32()?  Because at production time the data was
written in big-endian format.  Yet, when the driver used to carry the
firmware within the driver, the FW folks built the corresponding .C
file of the array of 32-bit words in little-endian format...  There's
a similar case for the code in qla24xx_load_risc() where the .bin file
read via request_firmware() was generated in big-endian format...
Why?  Well...sometimes "stuff" happens and we accept things...

Look, rather than pick apart (the rat hole) each access-region within
flash and determine which endian-format is used during read and
write, let me just say that we'll continue to be extra-cautious when
adding features which interact with the ISPs little-endian RISC and
various BE/LE regions of flash...

Thanks again for your endianess-scrubbing of qla2xxx, I do appreciate
your time and efforts and should have patches submitted shortly for
much of the 'badness' reported.

Thanks,
Andrew Vasquez