[PATCH 2.6.18-rc1-mm2 0/6] ieee1394: sbp2: misc updates

[PATCH 2.6.18-rc1-mm2 0/6] ieee1394: sbp2: misc updates

[Stefan Richter]

The following patches include minor fixes and cleanups:

1/6 ieee1394: sbp2: safer last_orb and next_ORB handling
2/6 ieee1394: sbp2: discard return value of sbp2_link_orb_command
3/6 ieee1394: sbp2: optimize DMA direction of command ORBs
4/6 ieee1394: sbp2: safer initialization of status fifo
5/6 ieee1394: sbp2: more checks of status block
6/6 ieee1394: sbp2: convert sbp2util_down_timeout to waitqueue

 drivers/ieee1394/sbp2.c |  306 +++++++++++++++++-----------------------
 drivers/ieee1394/sbp2.h |   27 +--
 2 files changed, 143 insertions(+), 190 deletions(-)

I hope to follow up with further fixes that deal with command ORB
queueing during the next few weeks.
-- 
Stefan Richter
-=====-=-==- -=== =-===
http://arcgraph.de/sr/

[PATCH 2.6.18-rc1-mm2 1/6] ieee1394: sbp2: safer last_orb and next_ORB handling

[Stefan Richter]

The sbp2 initiator has two ways to tell a target's fetch agent about new
command ORBs:
 - Write the ORB's address to the ORB_POINTER register.  This must not
   be done while the fetch agent is active.
 - Put the ORB's address into the previously submitted ORB's next_ORB
   field and write to the DOORBELL register.  This may be done while the
   fetch agent is active or suspended.  It must not be done while the
   fetch agent is in reset state.
Sbp2 has a last_orb pointer which indicates in what way a new command
should be announced.  That pointer is concurrently accessed at various
occasions.  Furthermore, initiator and target are accessing the next_ORB
field of ORBs concurrently and asynchronously.

This patch does:
 - Protect all initiator accesses to last_orb by sbp2_command_orb_lock.
 - Add pci_dma_sync_single_for_device before a previously submitted
   ORB's next_ORB field is overwritten.
 - Insert a memory barrier between when next_ORB_lo and next_ORB_hi are
   overwritten.  Next_ORB_hi must not be updated before next_ORB_lo.
 - Remove the rather unspecific and now superfluous qualifier "volatile"
   from the next_ORB fields.
 - Add comments on how last_orb is connected with what is known about
   the target's fetch agent's state.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |  101 +++++++++++++++++++---------------------
 drivers/ieee1394/sbp2.h |    4 -
 2 files changed, 50 insertions(+), 55 deletions(-)

TODO:
There are further issues with how the ORB lists are maintained which I
hope to address RSN.

Index: linux/drivers/ieee1394/sbp2.h
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.h	2006-07-22 22:20:16.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.h	2006-07-23 10:08:51.000000000 +0200
@@ -46,8 +46,8 @@
 #define ORB_SET_DIRECTION(value)		((value & 0x1) << 27)
 
 struct sbp2_command_orb {
-	volatile u32 next_ORB_hi;
-	volatile u32 next_ORB_lo;
+	u32 next_ORB_hi;
+	u32 next_ORB_lo;
 	u32 data_descriptor_hi;
 	u32 data_descriptor_lo;
 	u32 misc;
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2006-07-22 22:20:16.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.c	2006-07-23 10:11:10.000000000 +0200
@@ -1705,6 +1705,7 @@ static int sbp2_agent_reset(struct scsi_
 	quadlet_t data;
 	u64 addr;
 	int retval;
+	unsigned long flags;
 
 	SBP2_DEBUG_ENTER();
 
@@ -1724,7 +1725,9 @@ static int sbp2_agent_reset(struct scsi_
 	/*
 	 * Need to make sure orb pointer is written on next command
 	 */
+	spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
 	scsi_id->last_orb = NULL;
+	spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 
 	return 0;
 }
@@ -1966,8 +1969,12 @@ static int sbp2_link_orb_command(struct 
 {
 	struct sbp2scsi_host_info *hi = scsi_id->hi;
 	struct sbp2_command_orb *command_orb = &command->command_orb;
-	struct node_entry *ne = scsi_id->ne;
-	u64 addr;
+	struct sbp2_command_orb *last_orb;
+	dma_addr_t last_orb_dma;
+	u64 addr = scsi_id->sbp2_command_block_agent_addr;
+	quadlet_t data[2];
+	size_t length;
+	unsigned long flags;
 
 	outstanding_orb_incr;
 	SBP2_ORB_DEBUG("sending command orb %p, total orbs = %x",
@@ -1982,64 +1989,50 @@ static int sbp2_link_orb_command(struct 
 	/*
 	 * Check to see if there are any previous orbs to use
 	 */
-	if (scsi_id->last_orb == NULL) {
-		quadlet_t data[2];
-
+	spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
+	last_orb = scsi_id->last_orb;
+	last_orb_dma = scsi_id->last_orb_dma;
+	if (!last_orb) {
 		/*
-		 * Ok, let's write to the target's management agent register
+		 * last_orb == NULL means: We know that the target's fetch agent
+		 * is not active right now.
 		 */
-		addr = scsi_id->sbp2_command_block_agent_addr + SBP2_ORB_POINTER_OFFSET;
+		addr += SBP2_ORB_POINTER_OFFSET;
 		data[0] = ORB_SET_NODE_ID(hi->host->node_id);
 		data[1] = command->command_orb_dma;
 		sbp2util_cpu_to_be32_buffer(data, 8);
-
-		SBP2_ORB_DEBUG("write command agent, command orb %p", command_orb);
-
-		if (sbp2util_node_write_no_wait(ne, addr, data, 8) < 0) {
-			SBP2_ERR("sbp2util_node_write_no_wait failed.\n");
-			return -EIO;
-		}
-
-		SBP2_ORB_DEBUG("write command agent complete");
-
-		scsi_id->last_orb = command_orb;
-		scsi_id->last_orb_dma = command->command_orb_dma;
-
+		length = 8;
 	} else {
-		quadlet_t data;
-
 		/*
-		 * We have an orb already sent (maybe or maybe not
-		 * processed) that we can append this orb to. So do so,
-		 * and ring the doorbell. Have to be very careful
-		 * modifying these next orb pointers, as they are accessed
-		 * both by the sbp2 device and us.
+		 * last_orb != NULL means: We know that the target's fetch agent
+		 * is (very probably) not dead or in reset state right now.
+		 * We have an ORB already sent that we can append a new one to.
+		 * The target's fetch agent may or may not have read this
+		 * previous ORB yet.
 		 */
-		scsi_id->last_orb->next_ORB_lo =
-		    cpu_to_be32(command->command_orb_dma);
+		pci_dma_sync_single_for_cpu(hi->host->pdev, last_orb_dma,
+					    sizeof(struct sbp2_command_orb),
+					    PCI_DMA_BIDIRECTIONAL);
+		last_orb->next_ORB_lo = cpu_to_be32(command->command_orb_dma);
+		wmb();
 		/* Tells hardware that this pointer is valid */
-		scsi_id->last_orb->next_ORB_hi = 0x0;
-		pci_dma_sync_single_for_device(hi->host->pdev,
-					       scsi_id->last_orb_dma,
+		last_orb->next_ORB_hi = 0;
+		pci_dma_sync_single_for_device(hi->host->pdev, last_orb_dma,
 					       sizeof(struct sbp2_command_orb),
 					       PCI_DMA_BIDIRECTIONAL);
+		addr += SBP2_DOORBELL_OFFSET;
+		data[0] = 0;
+		length = 4;
+	}
+	scsi_id->last_orb = command_orb;
+	scsi_id->last_orb_dma = command->command_orb_dma;
+	spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 
-		/*
-		 * Ring the doorbell
-		 */
-		data = cpu_to_be32(command->command_orb_dma);
-		addr = scsi_id->sbp2_command_block_agent_addr + SBP2_DOORBELL_OFFSET;
-
-		SBP2_ORB_DEBUG("ring doorbell, command orb %p", command_orb);
-
-		if (sbp2util_node_write_no_wait(ne, addr, &data, 4) < 0) {
-			SBP2_ERR("sbp2util_node_write_no_wait failed");
-			return -EIO;
-		}
-
-		scsi_id->last_orb = command_orb;
-		scsi_id->last_orb_dma = command->command_orb_dma;
-
+	SBP2_ORB_DEBUG("write to %s register, command orb %p",
+			last_orb ? "DOORBELL" : "ORB_POINTER", command_orb);
+	if (sbp2util_node_write_no_wait(scsi_id->ne, addr, data, length) < 0) {
+		SBP2_ERR("sbp2util_node_write_no_wait failed.\n");
+		return -EIO;
 	}
 	return 0;
 }
@@ -2231,14 +2224,16 @@ static int sbp2_handle_status_write(stru
 		}
 
 		/*
-		 * Check here to see if there are no commands in-use. If there are none, we can
-		 * null out last orb so that next time around we write directly to the orb pointer...
-		 * Quick start saves one 1394 bus transaction.
+		 * Check here to see if there are no commands in-use. If there
+		 * are none, we know that the fetch agent left the active state
+		 * _and_ that we did not reactivate it yet. Therefore clear
+		 * last_orb so that next time we write directly to the
+		 * ORB_POINTER register. That way the fetch agent does not need
+		 * to refetch the next_ORB.
 		 */
 		spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
-		if (list_empty(&scsi_id->sbp2_command_orb_inuse)) {
+		if (list_empty(&scsi_id->sbp2_command_orb_inuse))
 			scsi_id->last_orb = NULL;
-		}
 		spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 
 	} else {

[PATCH 2.6.18-rc1-mm2 2/6] ieee1394: sbp2: discard return value of sbp2_link_orb_command

[Stefan Richter]

Since sbp2 is at the moment unable to do anything with the return value
of sbp2_link_orb_command, just discard it.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |    8 +++-----
 drivers/ieee1394/sbp2.h |    5 -----
 2 files changed, 3 insertions(+), 10 deletions(-)

TODO:
Alas sbp2 initiates 1394 write requests to the target's ORB_POINTER and
DOORBELL registers from atomic context.  These write transactions should
be moved into non-atomic context, i.e. a kthread or workqueue.  That way
typical failures of sbp2util_node_write_no_wait can be avoided in the
first place, and there are better possibilities to recover from
remaining failures.

Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2006-07-23 10:11:10.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.c	2006-07-23 10:26:32.000000000 +0200
@@ -1964,7 +1964,7 @@ static void sbp2_create_command_orb(stru
 /*
  * This function is called in order to begin a regular SBP-2 command.
  */
-static int sbp2_link_orb_command(struct scsi_id_instance_data *scsi_id,
+static void sbp2_link_orb_command(struct scsi_id_instance_data *scsi_id,
 				 struct sbp2_command_info *command)
 {
 	struct sbp2scsi_host_info *hi = scsi_id->hi;
@@ -2030,11 +2030,9 @@ static int sbp2_link_orb_command(struct 
 
 	SBP2_ORB_DEBUG("write to %s register, command orb %p",
 			last_orb ? "DOORBELL" : "ORB_POINTER", command_orb);
-	if (sbp2util_node_write_no_wait(scsi_id->ne, addr, data, length) < 0) {
+	if (sbp2util_node_write_no_wait(scsi_id->ne, addr, data, length))
 		SBP2_ERR("sbp2util_node_write_no_wait failed.\n");
-		return -EIO;
-	}
-	return 0;
+	/* We rely on SCSI EH to deal with _node_write_ failures. */
 }
 
 /*
Index: linux/drivers/ieee1394/sbp2.h
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.h	2006-07-23 10:08:51.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.h	2006-07-23 10:23:26.000000000 +0200
@@ -390,11 +390,6 @@ static int sbp2_logout_device(struct scs
 static int sbp2_handle_status_write(struct hpsb_host *host, int nodeid, int destid,
 				    quadlet_t *data, u64 addr, size_t length, u16 flags);
 static int sbp2_agent_reset(struct scsi_id_instance_data *scsi_id, int wait);
-static int sbp2_link_orb_command(struct scsi_id_instance_data *scsi_id,
-				 struct sbp2_command_info *command);
-static int sbp2_send_command(struct scsi_id_instance_data *scsi_id,
-			     struct scsi_cmnd *SCpnt,
-			     void (*done)(struct scsi_cmnd *));
 static unsigned int sbp2_status_to_sense_data(unchar *sbp2_status,
 					      unchar *sense_data);
 static void sbp2_parse_unit_directory(struct scsi_id_instance_data *scsi_id,

[PATCH 2.6.18-rc1-mm2 3/6] ieee1394: sbp2: optimize DMA direction of command ORBs

[Stefan Richter]

Only the driver writes ORBs, the device just reads them.  Therefore
PCI_DMA_BIDIRECTIONAL can be replaced by PCI_DMA_TODEVICE which may be
cheaper on some architectures.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

TODO:
Convert sbp2 from PCI DMA to generic DMA mapping.

Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2006-07-23 10:16:57.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.c	2006-07-23 10:19:29.000000000 +0200
@@ -492,7 +492,7 @@ static int sbp2util_create_command_orb_p
 		command->command_orb_dma =
 		    pci_map_single(hi->host->pdev, &command->command_orb,
 				   sizeof(struct sbp2_command_orb),
-				   PCI_DMA_BIDIRECTIONAL);
+				   PCI_DMA_TODEVICE);
 		SBP2_DMA_ALLOC("single command orb DMA");
 		command->sge_dma =
 		    pci_map_single(hi->host->pdev,
@@ -525,7 +525,7 @@ static void sbp2util_remove_command_orb_
 			/* Release our generic DMA's */
 			pci_unmap_single(host->pdev, command->command_orb_dma,
 					 sizeof(struct sbp2_command_orb),
-					 PCI_DMA_BIDIRECTIONAL);
+					 PCI_DMA_TODEVICE);
 			SBP2_DMA_FREE("single command orb DMA");
 			pci_unmap_single(host->pdev, command->sge_dma,
 					 sizeof(command->scatter_gather_element),
@@ -1982,7 +1982,7 @@ static void sbp2_link_orb_command(struct
 
 	pci_dma_sync_single_for_device(hi->host->pdev, command->command_orb_dma,
 				       sizeof(struct sbp2_command_orb),
-				       PCI_DMA_BIDIRECTIONAL);
+				       PCI_DMA_TODEVICE);
 	pci_dma_sync_single_for_device(hi->host->pdev, command->sge_dma,
 				       sizeof(command->scatter_gather_element),
 				       PCI_DMA_BIDIRECTIONAL);
@@ -2012,14 +2012,14 @@ static void sbp2_link_orb_command(struct
 		 */
 		pci_dma_sync_single_for_cpu(hi->host->pdev, last_orb_dma,
 					    sizeof(struct sbp2_command_orb),
-					    PCI_DMA_BIDIRECTIONAL);
+					    PCI_DMA_TODEVICE);
 		last_orb->next_ORB_lo = cpu_to_be32(command->command_orb_dma);
 		wmb();
 		/* Tells hardware that this pointer is valid */
 		last_orb->next_ORB_hi = 0;
 		pci_dma_sync_single_for_device(hi->host->pdev, last_orb_dma,
 					       sizeof(struct sbp2_command_orb),
-					       PCI_DMA_BIDIRECTIONAL);
+					       PCI_DMA_TODEVICE);
 		addr += SBP2_DOORBELL_OFFSET;
 		data[0] = 0;
 		length = 4;
@@ -2176,7 +2176,7 @@ static int sbp2_handle_status_write(stru
 		SBP2_DEBUG("Found status for command ORB");
 		pci_dma_sync_single_for_cpu(hi->host->pdev, command->command_orb_dma,
 					    sizeof(struct sbp2_command_orb),
-					    PCI_DMA_BIDIRECTIONAL);
+					    PCI_DMA_TODEVICE);
 		pci_dma_sync_single_for_cpu(hi->host->pdev, command->sge_dma,
 					    sizeof(command->scatter_gather_element),
 					    PCI_DMA_BIDIRECTIONAL);
@@ -2365,7 +2365,7 @@ static void sbp2scsi_complete_all_comman
 		command = list_entry(lh, struct sbp2_command_info, list);
 		pci_dma_sync_single_for_cpu(hi->host->pdev, command->command_orb_dma,
 					    sizeof(struct sbp2_command_orb),
-					    PCI_DMA_BIDIRECTIONAL);
+					    PCI_DMA_TODEVICE);
 		pci_dma_sync_single_for_cpu(hi->host->pdev, command->sge_dma,
 					    sizeof(command->scatter_gather_element),
 					    PCI_DMA_BIDIRECTIONAL);
@@ -2548,7 +2548,7 @@ static int sbp2scsi_abort(struct scsi_cm
 			pci_dma_sync_single_for_cpu(hi->host->pdev,
 						    command->command_orb_dma,
 						    sizeof(struct sbp2_command_orb),
-						    PCI_DMA_BIDIRECTIONAL);
+						    PCI_DMA_TODEVICE);
 			pci_dma_sync_single_for_cpu(hi->host->pdev,
 						    command->sge_dma,
 						    sizeof(command->scatter_gather_element),

[PATCH 2.6.18-rc1-mm2 4/6] ieee1394: sbp2: safer initialization of status fifo

[Stefan Richter]

Sbp2's copy of the status fifo was cleared when management ORBs or new
command ORBs were prepared.  The latter had potential for a race
condition if the block layer's soft IRQ and the 1394 LLD's interrupt
handler ran on different CPUs.  It would also yield wrong status if a
command was completed with non-zero completion status before other
commands that had zero completion status, and no new command was
enqueued in the meantime.
   
Now, the status buffer is cleared right before it is written.  Thus it
ends up in the following simpler and safer access pattern:
 - sbp2_alloc_device: allocates and implicitly clears once,
 - sbp2_handle_status_write: clears, writes, and reads,
 - sbp2_query_logins, sbp2_login_device, sbp2_reconnect_device: read.
The latter three do not race with sbp2_handle_status_write because of
how the protocol works.

As a tiny optimization, the first two quadlets of the status never need
to be cleared.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |   80 +++++++++++++++-------------------------
 1 files changed, 30 insertions(+), 50 deletions(-)

Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2006-07-23 10:19:29.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.c	2006-07-23 10:21:07.000000000 +0200
@@ -1182,7 +1182,6 @@ static int sbp2_query_logins(struct scsi
 			     "sbp2 query logins orb", scsi_id->query_logins_orb_dma);
 
 	memset(scsi_id->query_logins_response, 0, sizeof(struct sbp2_query_logins_response));
-	memset(&scsi_id->status_block, 0, sizeof(struct sbp2_status_block));
 
 	data[0] = ORB_SET_NODE_ID(hi->host->node_id);
 	data[1] = scsi_id->query_logins_orb_dma;
@@ -1278,7 +1277,6 @@ static int sbp2_login_device(struct scsi
 			     "sbp2 login orb", scsi_id->login_orb_dma);
 
 	memset(scsi_id->login_response, 0, sizeof(struct sbp2_login_response));
-	memset(&scsi_id->status_block, 0, sizeof(struct sbp2_status_block));
 
 	data[0] = ORB_SET_NODE_ID(hi->host->node_id);
 	data[1] = scsi_id->login_orb_dma;
@@ -1445,14 +1443,6 @@ static int sbp2_reconnect_device(struct 
 	sbp2util_packet_dump(scsi_id->reconnect_orb, sizeof(struct sbp2_reconnect_orb),
 			     "sbp2 reconnect orb", scsi_id->reconnect_orb_dma);
 
-	/*
-	 * Initialize status fifo
-	 */
-	memset(&scsi_id->status_block, 0, sizeof(struct sbp2_status_block));
-
-	/*
-	 * Ok, let's write to the target's management agent register
-	 */
 	data[0] = ORB_SET_NODE_ID(hi->host->node_id);
 	data[1] = scsi_id->reconnect_orb_dma;
 	sbp2util_cpu_to_be32_buffer(data, 8);
@@ -2069,11 +2059,6 @@ static int sbp2_send_command(struct scsi
 			     "sbp2 command orb", command->command_orb_dma);
 
 	/*
-	 * Initialize status fifo
-	 */
-	memset(&scsi_id->status_block, 0, sizeof(struct sbp2_status_block));
-
-	/*
 	 * Link up the orb, and ring the doorbell if needed
 	 */
 	sbp2_link_orb_command(scsi_id, command);
@@ -2114,12 +2099,14 @@ static unsigned int sbp2_status_to_sense
 /*
  * This function deals with status writes from the SBP-2 device
  */
-static int sbp2_handle_status_write(struct hpsb_host *host, int nodeid, int destid,
-				    quadlet_t *data, u64 addr, size_t length, u16 fl)
+static int sbp2_handle_status_write(struct hpsb_host *host, int nodeid,
+				    int destid, quadlet_t *data, u64 addr,
+				    size_t length, u16 fl)
 {
 	struct sbp2scsi_host_info *hi;
 	struct scsi_id_instance_data *scsi_id = NULL, *scsi_id_tmp;
 	struct scsi_cmnd *SCpnt = NULL;
+	struct sbp2_status_block *sb;
 	u32 scsi_status = SBP2_SCSI_STATUS_GOOD;
 	struct sbp2_command_info *command;
 	unsigned long flags;
@@ -2158,19 +2145,21 @@ static int sbp2_handle_status_write(stru
 	}
 
 	/*
-	 * Put response into scsi_id status fifo...
+	 * Put response into scsi_id status fifo buffer. The first two bytes
+	 * come in big endian bit order. Often the target writes only a
+	 * truncated status block, minimally the first two quadlets. The rest
+	 * is implied to be zeros.
 	 */
-	memcpy(&scsi_id->status_block, data, length);
+	sb = &scsi_id->status_block;
+	memset(sb->command_set_dependent, 0, sizeof(sb->command_set_dependent));
+	memcpy(sb, data, length);
+	sbp2util_be32_to_cpu_buffer(sb, 8);
 
 	/*
-	 * Byte swap first two quadlets (8 bytes) of status for processing
+	 * Handle command ORB status here if necessary. First, need to match
+	 * status with command.
 	 */
-	sbp2util_be32_to_cpu_buffer(&scsi_id->status_block, 8);
-
-	/*
-	 * Handle command ORB status here if necessary. First, need to match status with command.
-	 */
-	command = sbp2util_find_command_for_orb(scsi_id, scsi_id->status_block.ORB_offset_lo);
+	command = sbp2util_find_command_for_orb(scsi_id, sb->ORB_offset_lo);
 	if (command) {
 
 		SBP2_DEBUG("Found status for command ORB");
@@ -2185,7 +2174,8 @@ static int sbp2_handle_status_write(stru
 		outstanding_orb_decr;
 
 		/*
-		 * Matched status with command, now grab scsi command pointers and check status
+		 * Matched status with command, now grab scsi command pointers
+		 * and check status.
 		 */
 		SCpnt = command->Current_SCpnt;
 		spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
@@ -2193,28 +2183,22 @@ static int sbp2_handle_status_write(stru
 		spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 
 		if (SCpnt) {
-
 			/*
-			 * See if the target stored any scsi status information
+			 * See if the target stored any scsi status information.
 			 */
-			if (STATUS_GET_LENGTH(scsi_id->status_block.ORB_offset_hi_misc) > 1) {
-				/*
-				 * Translate SBP-2 status to SCSI sense data
-				 */
+			if (STATUS_GET_LENGTH(sb->ORB_offset_hi_misc) > 1) {
 				SBP2_DEBUG("CHECK CONDITION");
-				scsi_status = sbp2_status_to_sense_data((unchar *)&scsi_id->status_block, SCpnt->sense_buffer);
+				scsi_status = sbp2_status_to_sense_data(
+					(unchar *)sb, SCpnt->sense_buffer);
 			}
 
 			/*
-			 * Check to see if the dead bit is set. If so, we'll have to initiate
-			 * a fetch agent reset.
+			 * Check to see if the dead bit is set. If so, we'll
+			 * have to initiate a fetch agent reset.
 			 */
-			if (STATUS_GET_DEAD_BIT(scsi_id->status_block.ORB_offset_hi_misc)) {
-
-				/*
-				 * Initiate a fetch agent reset.
-				 */
-				SBP2_DEBUG("Dead bit set - initiating fetch agent reset");
+			if (STATUS_GET_DEAD_BIT(sb->ORB_offset_hi_misc)) {
+				SBP2_DEBUG("Dead bit set - "
+					   "initiating fetch agent reset");
                                 sbp2_agent_reset(scsi_id, 0);
 			}
 
@@ -2235,21 +2219,17 @@ static int sbp2_handle_status_write(stru
 		spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 
 	} else {
-
 		/*
 		 * It's probably a login/logout/reconnect status.
 		 */
-		if ((scsi_id->login_orb_dma == scsi_id->status_block.ORB_offset_lo) ||
-		    (scsi_id->query_logins_orb_dma == scsi_id->status_block.ORB_offset_lo) ||
-		    (scsi_id->reconnect_orb_dma == scsi_id->status_block.ORB_offset_lo) ||
-		    (scsi_id->logout_orb_dma == scsi_id->status_block.ORB_offset_lo)) {
+		if ((sb->ORB_offset_lo == scsi_id->reconnect_orb_dma) ||
+		    (sb->ORB_offset_lo == scsi_id->login_orb_dma) ||
+		    (sb->ORB_offset_lo == scsi_id->query_logins_orb_dma) ||
+		    (sb->ORB_offset_lo == scsi_id->logout_orb_dma))
 			atomic_set(&scsi_id->sbp2_login_complete, 1);
-		}
 	}
 
 	if (SCpnt) {
-
-		/* Complete the SCSI command. */
 		SBP2_DEBUG("Completing SCSI command");
 		sbp2scsi_complete_command(scsi_id, scsi_status, SCpnt,
 					  command->Current_done);

[PATCH 2.6.18-rc1-mm2 5/6] ieee1394: sbp2: more checks of status block

[Stefan Richter]

 - Add checks for the (very unlikely) cases that the target writes too
   little or too much status data or writes unsolicited status.
 - Indicate that these and similar conditions are unlikely().
 - Check the 'resp' and 'sbp_status' fields for possible failure status.
 - Slightly optimize access macros for the status block bitfields.
 - Unify a few related log messages.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |   70 +++++++++++++++++-----------------------
 drivers/ieee1394/sbp2.h |   14 ++++----
 2 files changed, 38 insertions(+), 46 deletions(-)

TODO:
Check if status' 'src'==1, then withhold the respective ORB from reuse
until status for any subsequent ORB was received.

Index: linux/drivers/ieee1394/sbp2.h
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.h	2006-07-23 10:11:34.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.h	2006-07-23 10:21:31.000000000 +0200
@@ -180,12 +180,14 @@ struct sbp2_unrestricted_page_table {
 
 #define SBP2_SCSI_STATUS_SELECTION_TIMEOUT	0xff
 
-#define STATUS_GET_ORB_OFFSET_HI(value)         (value & 0xffff)
-#define STATUS_GET_SBP_STATUS(value)            ((value >> 16) & 0xff)
-#define STATUS_GET_LENGTH(value)                ((value >> 24) & 0x7)
-#define STATUS_GET_DEAD_BIT(value)              ((value >> 27) & 0x1)
-#define STATUS_GET_RESP(value)                  ((value >> 28) & 0x3)
-#define STATUS_GET_SRC(value)                   ((value >> 30) & 0x3)
+#define STATUS_GET_SRC(value)			(((value) >> 30) & 0x3)
+#define STATUS_GET_LEN(value)			(((value) >> 24) & 0x7)
+#define STATUS_GET_ORB_OFFSET_HI(value)		((value) & 0x0000ffff)
+#define STATUS_TEST_D(value)			((value) & 0x08000000)
+/* test 'resp' | 'sbp2_status' */
+#define STATUS_TEST_RS(value)			((value) & 0x30ff0000)
+/* test 'resp' | 'dead' | 'sbp2_status' */
+#define STATUS_TEST_RDS(value)			((value) & 0x38ff0000)
 
 struct sbp2_status_block {
 	u32 ORB_offset_hi_misc;
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2006-07-23 10:21:07.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.c	2006-07-23 10:21:31.000000000 +0200
@@ -1201,11 +1201,8 @@ static int sbp2_query_logins(struct scsi
 		return -EIO;
 	}
 
-	if (STATUS_GET_RESP(scsi_id->status_block.ORB_offset_hi_misc) ||
-	    STATUS_GET_DEAD_BIT(scsi_id->status_block.ORB_offset_hi_misc) ||
-	    STATUS_GET_SBP_STATUS(scsi_id->status_block.ORB_offset_hi_misc)) {
-
-		SBP2_INFO("Error querying logins to SBP-2 device - timed out");
+	if (STATUS_TEST_RDS(scsi_id->status_block.ORB_offset_hi_misc)) {
+		SBP2_INFO("Error querying logins to SBP-2 device - failed");
 		return -EIO;
 	}
 
@@ -1298,18 +1295,12 @@ static int sbp2_login_device(struct scsi
 	 * Sanity. Make sure status returned matches login orb.
 	 */
 	if (scsi_id->status_block.ORB_offset_lo != scsi_id->login_orb_dma) {
-		SBP2_ERR("Error logging into SBP-2 device - login timed-out");
+		SBP2_ERR("Error logging into SBP-2 device - timed out");
 		return -EIO;
 	}
 
-	/*
-	 * Check status
-	 */
-	if (STATUS_GET_RESP(scsi_id->status_block.ORB_offset_hi_misc) ||
-	    STATUS_GET_DEAD_BIT(scsi_id->status_block.ORB_offset_hi_misc) ||
-	    STATUS_GET_SBP_STATUS(scsi_id->status_block.ORB_offset_hi_misc)) {
-
-		SBP2_ERR("Error logging into SBP-2 device - login failed");
+	if (STATUS_TEST_RDS(scsi_id->status_block.ORB_offset_hi_misc)) {
+		SBP2_ERR("Error logging into SBP-2 device - failed");
 		return -EIO;
 	}
 
@@ -1333,9 +1324,7 @@ static int sbp2_login_device(struct scsi
 	scsi_id->sbp2_command_block_agent_addr &= 0x0000ffffffffffffULL;
 
 	SBP2_INFO("Logged into SBP-2 device");
-
 	return 0;
-
 }
 
 /*
@@ -1466,25 +1455,17 @@ static int sbp2_reconnect_device(struct 
 	 * Sanity. Make sure status returned matches reconnect orb.
 	 */
 	if (scsi_id->status_block.ORB_offset_lo != scsi_id->reconnect_orb_dma) {
-		SBP2_ERR("Error reconnecting to SBP-2 device - reconnect timed-out");
+		SBP2_ERR("Error reconnecting to SBP-2 device - timed out");
 		return -EIO;
 	}
 
-	/*
-	 * Check status
-	 */
-	if (STATUS_GET_RESP(scsi_id->status_block.ORB_offset_hi_misc) ||
-	    STATUS_GET_DEAD_BIT(scsi_id->status_block.ORB_offset_hi_misc) ||
-	    STATUS_GET_SBP_STATUS(scsi_id->status_block.ORB_offset_hi_misc)) {
-
-		SBP2_ERR("Error reconnecting to SBP-2 device - reconnect failed");
+	if (STATUS_TEST_RDS(scsi_id->status_block.ORB_offset_hi_misc)) {
+		SBP2_ERR("Error reconnecting to SBP-2 device - failed");
 		return -EIO;
 	}
 
 	HPSB_DEBUG("Reconnected to SBP-2 device");
-
 	return 0;
-
 }
 
 /*
@@ -2115,18 +2096,19 @@ static int sbp2_handle_status_write(stru
 
 	sbp2util_packet_dump(data, length, "sbp2 status write by device", (u32)addr);
 
-	if (!host) {
+	if (unlikely(length < 8 || length > sizeof(struct sbp2_status_block))) {
+		SBP2_ERR("Wrong size of status block");
+		return RCODE_ADDRESS_ERROR;
+	}
+	if (unlikely(!host)) {
 		SBP2_ERR("host is NULL - this is bad!");
 		return RCODE_ADDRESS_ERROR;
 	}
-
 	hi = hpsb_get_hostinfo(&sbp2_highlevel, host);
-
-	if (!hi) {
+	if (unlikely(!hi)) {
 		SBP2_ERR("host info is NULL - this is bad!");
 		return RCODE_ADDRESS_ERROR;
 	}
-
 	/*
 	 * Find our scsi_id structure by looking at the status fifo address
 	 * written to by the sbp2 device.
@@ -2138,8 +2120,7 @@ static int sbp2_handle_status_write(stru
 			break;
 		}
 	}
-
-	if (!scsi_id) {
+	if (unlikely(!scsi_id)) {
 		SBP2_ERR("scsi_id is NULL - device is gone?");
 		return RCODE_ADDRESS_ERROR;
 	}
@@ -2156,12 +2137,14 @@ static int sbp2_handle_status_write(stru
 	sbp2util_be32_to_cpu_buffer(sb, 8);
 
 	/*
-	 * Handle command ORB status here if necessary. First, need to match
-	 * status with command.
+	 * Ignore unsolicited status. Handle command ORB status.
 	 */
-	command = sbp2util_find_command_for_orb(scsi_id, sb->ORB_offset_lo);
+	if (unlikely(STATUS_GET_SRC(sb->ORB_offset_hi_misc) == 2))
+		command = NULL;
+	else
+		command = sbp2util_find_command_for_orb(scsi_id,
+							sb->ORB_offset_lo);
 	if (command) {
-
 		SBP2_DEBUG("Found status for command ORB");
 		pci_dma_sync_single_for_cpu(hi->host->pdev, command->command_orb_dma,
 					    sizeof(struct sbp2_command_orb),
@@ -2177,16 +2160,23 @@ static int sbp2_handle_status_write(stru
 		 * Matched status with command, now grab scsi command pointers
 		 * and check status.
 		 */
+		/*
+		 * FIXME: If the src field in the status is 1, the ORB DMA must
+		 * not be reused until status for a subsequent ORB is received.
+		 */
 		SCpnt = command->Current_SCpnt;
 		spin_lock_irqsave(&scsi_id->sbp2_command_orb_lock, flags);
 		sbp2util_mark_command_completed(scsi_id, command);
 		spin_unlock_irqrestore(&scsi_id->sbp2_command_orb_lock, flags);
 
 		if (SCpnt) {
+			if (STATUS_TEST_RS(sb->ORB_offset_hi_misc))
+				scsi_status =
+					SBP2_SCSI_STATUS_COMMAND_TERMINATED;
 			/*
 			 * See if the target stored any scsi status information.
 			 */
-			if (STATUS_GET_LENGTH(sb->ORB_offset_hi_misc) > 1) {
+			if (STATUS_GET_LEN(sb->ORB_offset_hi_misc) > 1) {
 				SBP2_DEBUG("CHECK CONDITION");
 				scsi_status = sbp2_status_to_sense_data(
 					(unchar *)sb, SCpnt->sense_buffer);
@@ -2196,7 +2186,7 @@ static int sbp2_handle_status_write(stru
 			 * Check to see if the dead bit is set. If so, we'll
 			 * have to initiate a fetch agent reset.
 			 */
-			if (STATUS_GET_DEAD_BIT(sb->ORB_offset_hi_misc)) {
+			if (STATUS_TEST_D(sb->ORB_offset_hi_misc)) {
 				SBP2_DEBUG("Dead bit set - "
 					   "initiating fetch agent reset");
                                 sbp2_agent_reset(scsi_id, 0);

[PATCH 2.6.18-rc1-mm2 6/6] ieee1394: sbp2: convert sbp2util_down_timeout to waitqueue

[Stefan Richter]

The waitqueue API is used to replace a custom wait mechanism.  Only one
global waitqueue (instead of per-device waitqueues or completions) is
added because there is usually just one waiter.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |   51 +++++++++++++++++-----------------------
 drivers/ieee1394/sbp2.h |    4 +--
 2 files changed, 24 insertions(+), 31 deletions(-)

Index: linux/drivers/ieee1394/sbp2.h
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.h	2006-07-23 10:26:48.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.h	2006-07-23 13:19:35.000000000 +0200
@@ -320,9 +320,9 @@ struct scsi_id_instance_data {
 	u64 status_fifo_addr;
 
 	/*
-	 * Variable used for logins, reconnects, logouts, query logins
+	 * Waitqueue flag for logins, reconnects, logouts, query logins
 	 */
-	atomic_t sbp2_login_complete;
+	int access_complete:1;
 
 	/*
 	 * Pool of command orbs, so we can have more than overlapped command per id
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c	2006-07-23 12:49:15.000000000 +0200
+++ linux/drivers/ieee1394/sbp2.c	2006-07-23 18:57:47.000000000 +0200
@@ -55,12 +55,12 @@
 #include <linux/smp_lock.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/wait.h>
 
 #include <asm/current.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 #include <asm/byteorder.h>
-#include <asm/atomic.h>
 #include <asm/system.h>
 #include <asm/scatterlist.h>
 
@@ -420,21 +420,23 @@ static void sbp2util_packet_dump(void *b
 #define sbp2util_packet_dump(w,x,y,z)
 #endif
 
+static DECLARE_WAIT_QUEUE_HEAD(access_wq);
+
 /*
- * Goofy routine that basically does a down_timeout function.
+ * Waits for completion of an SBP-2 access request.
+ * Returns nonzero if timed out or prematurely interrupted.
  */
-static int sbp2util_down_timeout(atomic_t *done, int timeout)
+static int sbp2util_access_timeout(struct scsi_id_instance_data *scsi_id,
+				   int timeout)
 {
-	int i;
+	long leftover = wait_event_interruptible_timeout(
+				access_wq, scsi_id->access_complete, timeout);
 
-	for (i = timeout; (i > 0 && atomic_read(done) == 0); i-= HZ/10) {
-		if (msleep_interruptible(100))	/* 100ms */
-			return 1;
-	}
-	return (i > 0) ? 0 : 1;
+	scsi_id->access_complete = 0;
+	return leftover <= 0;
 }
 
-/* Free's an allocated packet */
+/* Frees an allocated packet */
 static void sbp2_free_packet(struct hpsb_packet *packet)
 {
 	hpsb_free_tlabel(packet);
@@ -794,7 +796,6 @@ static struct scsi_id_instance_data *sbp
 	scsi_id->speed_code = IEEE1394_SPEED_100;
 	scsi_id->max_payload_size = sbp2_speedto_max_payload[IEEE1394_SPEED_100];
 	scsi_id->status_fifo_addr = CSR1212_INVALID_ADDR_SPACE;
-	atomic_set(&scsi_id->sbp2_login_complete, 0);
 	INIT_LIST_HEAD(&scsi_id->sbp2_command_orb_inuse);
 	INIT_LIST_HEAD(&scsi_id->sbp2_command_orb_completed);
 	INIT_LIST_HEAD(&scsi_id->scsi_list);
@@ -1187,11 +1188,9 @@ static int sbp2_query_logins(struct scsi
 	data[1] = scsi_id->query_logins_orb_dma;
 	sbp2util_cpu_to_be32_buffer(data, 8);
 
-	atomic_set(&scsi_id->sbp2_login_complete, 0);
-
 	hpsb_node_write(scsi_id->ne, scsi_id->sbp2_management_agent_addr, data, 8);
 
-	if (sbp2util_down_timeout(&scsi_id->sbp2_login_complete, 2*HZ)) {
+	if (sbp2util_access_timeout(scsi_id, 2*HZ)) {
 		SBP2_INFO("Error querying logins to SBP-2 device - timed out");
 		return -EIO;
 	}
@@ -1279,15 +1278,13 @@ static int sbp2_login_device(struct scsi
 	data[1] = scsi_id->login_orb_dma;
 	sbp2util_cpu_to_be32_buffer(data, 8);
 
-	atomic_set(&scsi_id->sbp2_login_complete, 0);
-
 	hpsb_node_write(scsi_id->ne, scsi_id->sbp2_management_agent_addr, data, 8);
 
 	/*
 	 * Wait for login status (up to 20 seconds)...
 	 */
-	if (sbp2util_down_timeout(&scsi_id->sbp2_login_complete, 20*HZ)) {
-		SBP2_ERR("Error logging into SBP-2 device - login timed-out");
+	if (sbp2util_access_timeout(scsi_id, 20*HZ)) {
+		SBP2_ERR("Error logging into SBP-2 device - timed out");
 		return -EIO;
 	}
 
@@ -1374,21 +1371,17 @@ static int sbp2_logout_device(struct scs
 	data[1] = scsi_id->logout_orb_dma;
 	sbp2util_cpu_to_be32_buffer(data, 8);
 
-	atomic_set(&scsi_id->sbp2_login_complete, 0);
-
 	error = hpsb_node_write(scsi_id->ne,
 				scsi_id->sbp2_management_agent_addr, data, 8);
 	if (error)
 		return error;
 
 	/* Wait for device to logout...1 second. */
-	if (sbp2util_down_timeout(&scsi_id->sbp2_login_complete, HZ))
+	if (sbp2util_access_timeout(scsi_id, HZ))
 		return -EIO;
 
 	SBP2_INFO("Logged out of SBP-2 device");
-
 	return 0;
-
 }
 
 /*
@@ -1436,8 +1429,6 @@ static int sbp2_reconnect_device(struct 
 	data[1] = scsi_id->reconnect_orb_dma;
 	sbp2util_cpu_to_be32_buffer(data, 8);
 
-	atomic_set(&scsi_id->sbp2_login_complete, 0);
-
 	error = hpsb_node_write(scsi_id->ne,
 				scsi_id->sbp2_management_agent_addr, data, 8);
 	if (error)
@@ -1446,8 +1437,8 @@ static int sbp2_reconnect_device(struct 
 	/*
 	 * Wait for reconnect status (up to 1 second)...
 	 */
-	if (sbp2util_down_timeout(&scsi_id->sbp2_login_complete, HZ)) {
-		SBP2_ERR("Error reconnecting to SBP-2 device - reconnect timed-out");
+	if (sbp2util_access_timeout(scsi_id, HZ)) {
+		SBP2_ERR("Error reconnecting to SBP-2 device - timed out");
 		return -EIO;
 	}
 
@@ -2215,8 +2206,10 @@ static int sbp2_handle_status_write(stru
 		if ((sb->ORB_offset_lo == scsi_id->reconnect_orb_dma) ||
 		    (sb->ORB_offset_lo == scsi_id->login_orb_dma) ||
 		    (sb->ORB_offset_lo == scsi_id->query_logins_orb_dma) ||
-		    (sb->ORB_offset_lo == scsi_id->logout_orb_dma))
-			atomic_set(&scsi_id->sbp2_login_complete, 1);
+		    (sb->ORB_offset_lo == scsi_id->logout_orb_dma)) {
+			scsi_id->access_complete = 1;
+			wake_up_interruptible(&access_wq);
+		}
 	}
 
 	if (SCpnt) {