[PATCH 0/8] qla2xxx: updates/fixes for 2.6.27-rc3 [8.02.01-k7].

[PATCH 0/8] qla2xxx: updates/fixes for 2.6.27-rc3 [8.02.01-k7].

[Andrew Vasquez]

Another round of updates/fixes for 2.6.27-rc3.

 drivers/scsi/qla2xxx/qla_attr.c    |   11 +++++++++++
 drivers/scsi/qla2xxx/qla_def.h     |    2 +-
 drivers/scsi/qla2xxx/qla_init.c    |    8 ++++++--
 drivers/scsi/qla2xxx/qla_isr.c     |   14 +++++++-------
 drivers/scsi/qla2xxx/qla_mbx.c     |    2 +-
 drivers/scsi/qla2xxx/qla_os.c      |   18 ++++++++++++------
 drivers/scsi/qla2xxx/qla_version.h |    2 +-
 7 files changed, 39 insertions(+), 18 deletions(-)

here's the commits:

* Remove semaphore.h
* Correct synchronization of software/firmware fcport states.
* Correct vport-state management issues during ISP-ABORT.
* Don't leak SG-DMA mappings while aborting commands.
* Set npiv_supported flag for FCoE HBAs.
* Reference proper ha during SBR handling.
* Explicitly tear-down vports during PCI remove_one().
* Update version number to 8.02.01-k7.

Thanks, AV

[PATCH 1/8] qla2xxx: Remove semaphore.h

[Andrew Vasquez]

From: Matthew Wilcox <matthew@wil.cx>

Now that qla2xxx has been converted to mutexes, it no longer needs the
semaphore include.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_def.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index 6da31ba..8c5b25c 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -25,7 +25,6 @@
 #include <linux/firmware.h>
 #include <linux/aer.h>
 #include <linux/mutex.h>
-#include <linux/semaphore.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_host.h>
-- 
1.6.0.rc2.22.g71b9

[PATCH 2/8] qla2xxx: Correct synchronization of software/firmware fcport states.

[Andrew Vasquez]

Greg Wettstein (greg@enjellic.com) noted:

	http://article.gmane.org/gmane.linux.scsi/43409

on a reboot of a previously recognized SCST target, the initiator
driver would be unable to re-recognize the device as a target.
It turns out that prior to the SCST software reloading and
returning it's "target-capable" abilities in the PRLI payload,
the HBA would be re-initialized as an initiator-only type port.
Since initiators typically classify themselves as an FCP-2
capable device, both software and firmware do not perform an
explicit logout during port-loss.  Unfortunately, as can be seen
by the failure case, when the port (now target-capable) returns,
firmware performs an ADISC without a follow-on PRLI, leaving
stale 'initiator-only' data in the firmware's port database.

Correct the discrepancy by performing the explicit logout during
the transport's request to terminate-rport-io, thus synchronizing
port states and ensuring a follow-on PRLI is performed.

Reported-by: Greg Wettstein <greg@enjellic.com>
Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_attr.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index a319a20..45e7dcb 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -993,6 +993,17 @@ qla2x00_terminate_rport_io(struct fc_rport *rport)
 {
 	fc_port_t *fcport = *(fc_port_t **)rport->dd_data;
 
+	/*
+	 * At this point all fcport's software-states are cleared.  Perform any
+	 * final cleanup of firmware resources (PCBs and XCBs).
+	 */
+	if (fcport->loop_id != FC_NO_LOOP_ID) {
+		fcport->ha->isp_ops->fabric_logout(fcport->ha, fcport->loop_id,
+		    fcport->d_id.b.domain, fcport->d_id.b.area,
+		    fcport->d_id.b.al_pa);
+		fcport->loop_id = FC_NO_LOOP_ID;
+	}
+
 	qla2x00_abort_fcport_cmds(fcport);
 	scsi_target_unblock(&rport->dev);
 }
-- 
1.6.0.rc2.22.g71b9

[PATCH 3/8] qla2xxx: Correct vport-state management issues during ISP-ABORT.

[Andrew Vasquez]

* Use correct 'ha' to mark a device lost from ISR.
  I/Os will always be returned on the physical-HA.
  qla2x00_mark_device_lost() should be called with the HA bound
  to the fcport.
* Mark *all* devices lost during ISP-ABORT (bighammer).

These fixes correct issues discovered locally where during
link-perturbation and heavy vport-I/O fcport/rport states would
stray and an rport's scsi-target lost (timed-out).

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_init.c |    3 +++
 drivers/scsi/qla2xxx/qla_isr.c  |    7 +++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 601a6b2..e1de7a6 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -3251,6 +3251,7 @@ qla2x00_abort_isp(scsi_qla_host_t *ha)
 {
 	int rval;
 	uint8_t        status = 0;
+	scsi_qla_host_t *vha;
 
 	if (ha->flags.online) {
 		ha->flags.online = 0;
@@ -3265,6 +3266,8 @@ qla2x00_abort_isp(scsi_qla_host_t *ha)
 		if (atomic_read(&ha->loop_state) != LOOP_DOWN) {
 			atomic_set(&ha->loop_state, LOOP_DOWN);
 			qla2x00_mark_all_devices_lost(ha, 0);
+			list_for_each_entry(vha, &ha->vp_list, vp_list)
+			       qla2x00_mark_all_devices_lost(vha, 0);
 		} else {
 			if (!atomic_read(&ha->loop_down_timer))
 				atomic_set(&ha->loop_down_timer,
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 874d802..d6a1a66 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -1184,9 +1184,8 @@ qla2x00_status_entry(scsi_qla_host_t *ha, void *pkt)
 		    atomic_read(&fcport->state)));
 
 		cp->result = DID_BUS_BUSY << 16;
-		if (atomic_read(&fcport->state) == FCS_ONLINE) {
-			qla2x00_mark_device_lost(ha, fcport, 1, 1);
-		}
+		if (atomic_read(&fcport->state) == FCS_ONLINE)
+			qla2x00_mark_device_lost(fcport->ha, fcport, 1, 1);
 		break;
 
 	case CS_RESET:
@@ -1229,7 +1228,7 @@ qla2x00_status_entry(scsi_qla_host_t *ha, void *pkt)
 
 		/* Check to see if logout occurred. */
 		if ((le16_to_cpu(sts->status_flags) & SF_LOGOUT_SENT))
-			qla2x00_mark_device_lost(ha, fcport, 1, 1);
+			qla2x00_mark_device_lost(fcport->ha, fcport, 1, 1);
 		break;
 
 	default:
-- 
1.6.0.rc2.22.g71b9

[PATCH 4/8] qla2xxx: Don't leak SG-DMA mappings while aborting commands.

[Andrew Vasquez]

Original code inadvertently cleared an SRB's 'flags' while
aborting; causing a follow-on scsi_dma_unmap() to be potentially
missed.

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_os.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 7c8af7e..c2092ad 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1080,9 +1080,7 @@ qla2x00_abort_all_cmds(scsi_qla_host_t *ha, int res)
 		sp = ha->outstanding_cmds[cnt];
 		if (sp) {
 			ha->outstanding_cmds[cnt] = NULL;
-			sp->flags = 0;
 			sp->cmd->result = res;
-			sp->cmd->host_scribble = (unsigned char *)NULL;
 			qla2x00_sp_compl(ha, sp);
 		}
 	}
-- 
1.6.0.rc2.22.g71b9

[PATCH 5/8] qla2xxx: Set npiv_supported flag for FCoE HBAs.

[Andrew Vasquez]

From: Mike Hernandez <mike.hernandez@qlogic.com>

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_init.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index e1de7a6..ee89ddd 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -976,8 +976,9 @@ qla2x00_setup_chip(scsi_qla_host_t *ha)
 				    &ha->fw_attributes, &ha->fw_memory_size);
 				qla2x00_resize_request_q(ha);
 				ha->flags.npiv_supported = 0;
-				if ((IS_QLA24XX(ha) || IS_QLA25XX(ha)) &&
-				    (ha->fw_attributes & BIT_2)) {
+				if ((IS_QLA24XX(ha) || IS_QLA25XX(ha) ||
+				     IS_QLA84XX(ha)) &&
+					 (ha->fw_attributes & BIT_2)) {
 					ha->flags.npiv_supported = 1;
 					if ((!ha->max_npiv_vports) ||
 					    ((ha->max_npiv_vports + 1) %
-- 
1.6.0.rc2.22.g71b9

[PATCH 6/8] qla2xxx: Reference proper ha during SBR handling.

[Andrew Vasquez]

The executing-HA of an SRB can be referenced from the sp->fcport.
Use this correct value while processing status-continuation data
and abort processing.

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_isr.c |    7 ++++---
 drivers/scsi/qla2xxx/qla_os.c  |    3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index d6a1a66..45a3b93 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -879,11 +879,12 @@ qla2x00_handle_sense(srb_t *sp, uint8_t *sense_data, uint32_t sense_len)
 	sp->request_sense_ptr += sense_len;
 	sp->request_sense_length -= sense_len;
 	if (sp->request_sense_length != 0)
-		sp->ha->status_srb = sp;
+		sp->fcport->ha->status_srb = sp;
 
 	DEBUG5(printk("%s(): Check condition Sense data, scsi(%ld:%d:%d:%d) "
-	    "cmd=%p pid=%ld\n", __func__, sp->ha->host_no, cp->device->channel,
-	    cp->device->id, cp->device->lun, cp, cp->serial_number));
+	    "cmd=%p pid=%ld\n", __func__, sp->fcport->ha->host_no,
+	    cp->device->channel, cp->device->id, cp->device->lun, cp,
+	    cp->serial_number));
 	if (sense_len)
 		DEBUG5(qla2x00_dump_buffer(cp->sense_buffer,
 		    CMD_ACTUAL_SNSLEN(cp)));
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index c2092ad..e0880ad 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -780,7 +780,8 @@ qla2x00_eh_wait_for_pending_commands(scsi_qla_host_t *ha, unsigned int t,
 		sp = pha->outstanding_cmds[cnt];
 		if (!sp)
 			continue;
-		if (ha->vp_idx != sp->ha->vp_idx)
+
+		if (ha->vp_idx != sp->fcport->ha->vp_idx)
 			continue;
 		match = 0;
 		switch (type) {
-- 
1.6.0.rc2.22.g71b9

[PATCH 7/8] qla2xxx: Explicitly tear-down vports during PCI remove_one().

[Andrew Vasquez]

During internal testing, we've seen issues (hangs) with the
'deferred' vport tear-down-processing typically accompanied with
the fc_remove_host() call.  This is due to the current
implementation's back-end vport handling being performed by the
physical-HA's DPC thread where premature shutdown could lead to
latent vport requests without a processor.

This should also address a problem reported by Gal Rosen
(http://marc.info/?l=linux-scsi&m=121731664417358&w=2) where the
driver would attempt to awaken a previously torn-down DPC thread
from interrupt context by implicitly calling wake_up_process()
rather than the driver's qla2xxx_wake_dpc() helper.  Rather, than
reshuffle the remove_one() device-removal code, during unload,
depend on the driver's timer to wake-up the DPC process, by
limiting wake-ups based on an 'unloading' flag.

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_def.h |    1 +
 drivers/scsi/qla2xxx/qla_mbx.c |    2 +-
 drivers/scsi/qla2xxx/qla_os.c  |   13 ++++++++++---
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index 8c5b25c..431c19e 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -2236,6 +2236,7 @@ typedef struct scsi_qla_host {
 #define REGISTER_FDMI_NEEDED	26
 #define FCPORT_UPDATE_NEEDED	27
 #define VP_DPC_NEEDED		28	/* wake up for VP dpc handling */
+#define UNLOADING		29
 
 	uint32_t	device_flags;
 #define DFLG_LOCAL_DEVICES		BIT_0
diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
index bc90d6b..813bc77 100644
--- a/drivers/scsi/qla2xxx/qla_mbx.c
+++ b/drivers/scsi/qla2xxx/qla_mbx.c
@@ -2686,7 +2686,7 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *ha,
 		set_bit(VP_IDX_ACQUIRED, &vha->vp_flags);
 		set_bit(VP_DPC_NEEDED, &ha->dpc_flags);
 
-		wake_up_process(ha->dpc_thread);
+		qla2xxx_wake_dpc(ha);
 	}
 }
 
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index e0880ad..26afe44 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1775,10 +1775,15 @@ probe_out:
 static void
 qla2x00_remove_one(struct pci_dev *pdev)
 {
-	scsi_qla_host_t *ha;
+	scsi_qla_host_t *ha, *vha, *temp;
 
 	ha = pci_get_drvdata(pdev);
 
+	list_for_each_entry_safe(vha, temp, &ha->vp_list, vp_list)
+		fc_vport_terminate(vha->fc_vport);
+
+	set_bit(UNLOADING, &ha->dpc_flags);
+
 	qla2x00_dfs_remove(ha);
 
 	qla84xx_put_chip(ha);
@@ -2450,8 +2455,10 @@ qla2x00_do_dpc(void *data)
 void
 qla2xxx_wake_dpc(scsi_qla_host_t *ha)
 {
-	if (ha->dpc_thread)
-		wake_up_process(ha->dpc_thread);
+	struct task_struct *t = ha->dpc_thread;
+
+	if (!test_bit(UNLOADING, &ha->dpc_flags) && t)
+		wake_up_process(t);
 }
 
 /*
-- 
1.6.0.rc2.22.g71b9

Re: [PATCH 7/8] qla2xxx: Explicitly tear-down vports during PCI remove_one().

[Vladislav Bolkhovitin]

Andrew Vasquez wrote:
> During internal testing, we've seen issues (hangs) with the
> 'deferred' vport tear-down-processing typically accompanied with
> the fc_remove_host() call.  This is due to the current
> implementation's back-end vport handling being performed by the
> physical-HA's DPC thread where premature shutdown could lead to
> latent vport requests without a processor.
> 
> This should also address a problem reported by Gal Rosen
> (http://marc.info/?l=linux-scsi&m=121731664417358&w=2) where the
> driver would attempt to awaken a previously torn-down DPC thread
> from interrupt context by implicitly calling wake_up_process()
> rather than the driver's qla2xxx_wake_dpc() helper.  Rather, than
> reshuffle the remove_one() device-removal code, during unload,
> depend on the driver's timer to wake-up the DPC process, by
> limiting wake-ups based on an 'unloading' flag.
> 
> Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
> ---
>  drivers/scsi/qla2xxx/qla_def.h |    1 +
>  drivers/scsi/qla2xxx/qla_mbx.c |    2 +-
>  drivers/scsi/qla2xxx/qla_os.c  |   13 ++++++++++---
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 8c5b25c..431c19e 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -2236,6 +2236,7 @@ typedef struct scsi_qla_host {
>  #define REGISTER_FDMI_NEEDED	26
>  #define FCPORT_UPDATE_NEEDED	27
>  #define VP_DPC_NEEDED		28	/* wake up for VP dpc handling */
> +#define UNLOADING		29
>  
>  	uint32_t	device_flags;
>  #define DFLG_LOCAL_DEVICES		BIT_0
> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index bc90d6b..813bc77 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -2686,7 +2686,7 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *ha,
>  		set_bit(VP_IDX_ACQUIRED, &vha->vp_flags);
>  		set_bit(VP_DPC_NEEDED, &ha->dpc_flags);
>  
> -		wake_up_process(ha->dpc_thread);
> +		qla2xxx_wake_dpc(ha);
>  	}
>  }
>  
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index e0880ad..26afe44 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -1775,10 +1775,15 @@ probe_out:
>  static void
>  qla2x00_remove_one(struct pci_dev *pdev)
>  {
> -	scsi_qla_host_t *ha;
> +	scsi_qla_host_t *ha, *vha, *temp;
>  
>  	ha = pci_get_drvdata(pdev);
>  
> +	list_for_each_entry_safe(vha, temp, &ha->vp_list, vp_list)
> +		fc_vport_terminate(vha->fc_vport);
> +
> +	set_bit(UNLOADING, &ha->dpc_flags);
> +
>  	qla2x00_dfs_remove(ha);
>  
>  	qla84xx_put_chip(ha);
> @@ -2450,8 +2455,10 @@ qla2x00_do_dpc(void *data)
>  void
>  qla2xxx_wake_dpc(scsi_qla_host_t *ha)
>  {
> -	if (ha->dpc_thread)
> -		wake_up_process(ha->dpc_thread);
> +	struct task_struct *t = ha->dpc_thread;
> +
> +	if (!test_bit(UNLOADING, &ha->dpc_flags) && t)
> +		wake_up_process(t);
>  }

Sorry, but as I already pointed out, such approach doesn't fix anything, 
it only narrows down the race window. Increasing distance between "set" 
and "test" operations changes nothing, because in a kernel with 
preemption enabled preemption might happens in any place in the code, 
where it isn't explicitly or implicitly disabled. For instance, between 
lines

if (!test_bit(UNLOADING, &ha->dpc_flags) && t)

and

wake_up_process(t)

or inside wake_up_process(), so it would operate on already dead data 
pointed by t. I can see a lot of call trees where qla2xxx_wake_dpc() 
called with not disabled preemption, i.e. not from IRQ context and not 
under any lock (the driver doesn't explicitly disable preemption 
anywhere), for example on path 
qla2x00_do_dpc()->qla2x00_fabric_logout()->qla2x00_mailbox_command().

Also, I don't think anybody can predict on which maximum distance 
between code pieces side effects caused by CPU's out of order code 
execution can be seen.

If you want to make it better (but I'm not sure that complete), you 
should disable preemption in qla2xxx_wake_dpc() and insert memory 
barriers after setting UNLOADING bit and before testing it. But I 
wonder, how much advantage that would have over a simple spinlock as I 
proposed? The disadvantage can be seen pretty clearly: it's very 
non-obvious, hence quite hard to analyze and error prone to change.

Vlad

[PATCH 8/8] qla2xxx: Update version number to 8.02.01-k7.

[Andrew Vasquez]

Signed-off-by: Andrew Vasquez <andrew.vasquez@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_version.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_version.h b/drivers/scsi/qla2xxx/qla_version.h
index 676c390..4160e4c 100644
--- a/drivers/scsi/qla2xxx/qla_version.h
+++ b/drivers/scsi/qla2xxx/qla_version.h
@@ -7,7 +7,7 @@
 /*
  * Driver version
  */
-#define QLA2XXX_VERSION      "8.02.01-k6"
+#define QLA2XXX_VERSION      "8.02.01-k7"
 
 #define QLA_DRIVER_MAJOR_VER	8
 #define QLA_DRIVER_MINOR_VER	2
-- 
1.6.0.rc2.22.g71b9