aha152x.c - Cleanup, need help in testing and auditing

In motivation for the !use_sg cleanup and use of accessors I needed to do some restructuring of the aha152x.c driver.

I have tried to be as careful as I could, but with out the hardware for testing, it is hard. Christoph and James You are signed on a couple of the last patches to this driver So you are probably somewhat familiar with the code. Could you please review the patchset to see that nothing is broken.

If any one has an Hardware that uses aha152x.c, please help me with testing this patchset. If it does not work could you please do a short bisect of which patch breaks out of the 4.

The problematic parts were that the driver writes all over the scsi_cmnd members in couple of places. To untangle that I divided the work to stages for easier review and testing. I was very verbose at each patch explanation, mainly for myself to try and prove that the changes are sound.

[patch 1/4] aha152x.c - Preliminary fixes and comments
Some weird typos and some hard coded numbers made the code very hard to understand.

[patch 2/4] aha152x.c - Clean Reset path
On the Reset code path the driver would save, write, than restore scsi_cmnd members. Here I propose a solution that does not need to do that.

[patch 3/4] aha152x.c - Fix check_condition code-path
In case of a Status return from a target. The driver would re-queue the command with a REQUEST_SENSE read. Again save, write, restore. I cleaned that up to a stage I can be happy with. Also while at it I fixed a BUG with "resid".

[patch 4/4] aha152x.c - use data accessors and !use_sg cleanup
And now everything is ready for this one.

Thanks for any help
Boaz

hunk by hunk:
- CHECK_CONDITION is what happens to cmnd->status >> 1 or after status_byte() macro. But here it is used directly on status which means 0x1 which is an undefined bit in the standard. And is a status that will never return from a target.

- in busfree_run at the DONE_SC phase we have 3 distinct operation:
1-if(DONE_SC->SCp.phase & check_condition)
The REQUEST_SENSE command return.
- Restore original command
- Than continue to operation 3.
2-if(DONE_SC->SCp.Status==SAM_STAT_CHECK_CONDITION)
A regular command returned with a status.
- Internally re-Q a REQUEST_SENSE.
- Do not do operation 3.
3-
- Complete the command and return it to scsi-ml
So the 0x2 in both these operations (1,2) means the scsi check-condition status, hence SAM_STAT_CHECK_CONDITION

- Here the code asks about !(DONE_SC->SCp.Status & not_issued) but "not_issued" is an enum belonging to the "phase" member and not to the Status returned from target. The reason this works is because not_issued==1 and Also CHECK_CONDITION==1 (remember from hunk 1). So actually the code was asking !(DONE_SC->SCp.Status & CHECK_CONDITION). Which means "Has the status been read from target yet?"
Staus is read at status_run(). "not_issued" is cleared in seldo_run() which is usually earlier than status_run().

So this patch does nothing as far as assembly is concerned but it does let the reader understand what is going on.

---
 drivers/scsi/aha152x.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

index 85f2394..29253f9 100644
@@ -986,7 +986,7 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 	SCpnt->scsi_done	= done;
 	SCpnt->resid 		= SCpnt->request_bufflen;
 	SCpnt->SCp.phase	= not_issued | phase;
-	SCpnt->SCp.Status	= CHECK_CONDITION;
+	SCpnt->SCp.Status	= 0x1; /* Ilegal status by SCSI standard */
 	SCpnt->SCp.Message	= 0;
 	SCpnt->SCp.have_data_in	= 0;
 	SCpnt->SCp.sent_command	= 0;

@@ -1574,12 +1574,12 @@ static void busfree_run(struct Scsi_Host *shpnt)
 			cmd->use_sg          = sc->use_sg;
 			cmd->cmd_len         = sc->cmd_len;
 
-			cmd->SCp.Status = 0x02;
+			cmd->SCp.Status = SAM_STAT_CHECK_CONDITION;
 
 			HOSTDATA(shpnt)->commands--;
 			if (!HOSTDATA(shpnt)->commands)
 				SETPORT(PORTA, 0);	/* turn led off */
-		} else if(DONE_SC->SCp.Status==0x02) {
+		} else if(DONE_SC->SCp.Status==SAM_STAT_CHECK_CONDITION) {
 #if defined(AHA152X_STAT)
 			HOSTDATA(shpnt)->busfree_with_check_condition++;
 #endif

@@ -1587,7 +1587,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
 			DPRINTK(debug_eh, ERR_LEAD "CHECK CONDITION found\n", CMDINFO(DONE_SC));
 #endif
 
-			if(!(DONE_SC->SCp.Status & not_issued)) {
+			if(!(DONE_SC->SCp.phase & not_issued)) {
 				Scsi_Cmnd *ptr = DONE_SC;
 				DONE_SC=NULL;
 #if 0

1.5.2.2.249.g45fd

What Reset code was doing: Save command's important/dangerous Info on stack. NULL those members from scsi_cmnd. Issue a Reset. wait for it to finish than restore members and return.
What I do is save or NULL nothing. But use the "resetting" hint in aha152x_internal_queue() to NULL out working members and leave struct scsi_cmnd alone.
(Actually the Reset path never touches any of these working
members but it looks safer this way, just in case.)

The indentation in aha152x_internal_queue() is wrong at this stage but it will be eliminated in last patch, and for now it is easier for review.

---
 drivers/scsi/aha152x.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

index 29253f9..ea57ecd 100644
@@ -1022,6 +1022,14 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 	   SCp.buffer           : next buffer
 	   SCp.buffers_residual : left buffers in list
 	   SCp.phase            : current state of the command */
+
+	if(phase & resetting) {
+		SCpnt->SCp.ptr           = NULL;
+		SCpnt->SCp.this_residual = 0;
+		SCpnt->resid             = 0;
+		SCpnt->SCp.buffer           = NULL;
+		SCpnt->SCp.buffers_residual = 0;
+	} else {
 	if (SCpnt->use_sg) {
 		SCpnt->SCp.buffer           = (struct scatterlist *) SCpnt->request_buffer;
 		SCpnt->SCp.ptr              = SG_ADDRESS(SCpnt->SCp.buffer);

@@ -1033,6 +1041,7 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 		SCpnt->SCp.buffer           = NULL;
 		SCpnt->SCp.buffers_residual = 0;
 	}
+	}
 
 	DO_LOCK(flags);
 

@@ -1149,10 +1158,6 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 	struct Scsi_Host *shpnt = SCpnt->device->host;
 	DECLARE_COMPLETION(done);
 	int ret, issued, disconnected;
-	unsigned char old_cmd_len = SCpnt->cmd_len;
-	unsigned short old_use_sg = SCpnt->use_sg;
-	void *old_buffer = SCpnt->request_buffer;
-	unsigned old_bufflen = SCpnt->request_bufflen;
 	unsigned long flags;
 	unsigned long timeleft;
 

@@ -1173,11 +1178,6 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 	disconnected = issued && remove_SC(&DISCONNECTED_SC, SCpnt);
 	DO_UNLOCK(flags);
 
-	SCpnt->cmd_len         = 0;
-	SCpnt->use_sg          = 0;
-	SCpnt->request_buffer  = NULL;
-	SCpnt->request_bufflen = 0;
-
 	aha152x_internal_queue(SCpnt, &done, resetting, reset_done);
 
 	timeleft = wait_for_completion_timeout(&done, 100*HZ);

@@ -1188,11 +1188,6 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 		DO_UNLOCK(flags);
 	}
 
-	SCpnt->cmd_len         = old_cmd_len;
-	SCpnt->use_sg          = old_use_sg;
-  	SCpnt->request_buffer  = old_buffer;
-       	SCpnt->request_bufflen = old_bufflen;
-
 	DO_LOCK(flags);
 
 	if(SCpnt->SCp.phase & resetted) {

1.5.2.2.249.g45fd

check_condition code-path was similar but more
complicated to Reset. It went like this:
1. extra space was allocated at aha152x_scdata for mirroring scsi_cmnd members.
2. At aha152x_internal_queue() every command was saved into above members for optional use later.
3. At busfree_run() in the DONE_CS phase if a Status of SAM_STAT_CHECK_CONDITION was detected. The command was re-queued internally using aha152x_internal_queue(,,check_condition,) The command members are over written with a REQUEST_SENSE command info.
4. At busfree_run() in the DONE_CS phase again. If it is a check_condition command, info was restored from mirror made at first call to aha152x_internal_queue() (see 2) and the command is completed.

What I did is:
1. Allocate less space in aha152x_scdata only for the 16-byte original command. (which is actually not needed by scsi-ml anymore at this stage. But this is to much knowledge of scsi-ml)
2. If Status == SAM_STAT_CHECK_CONDITION, then like before re-queue a REQUEST_SENSE command. But only now save original command members. (Less of them)
3. In aha152x_internal_queue(), just like for Reset, use the check_condition hint to set differently the working members.
4. At busfree_run() in the DONE_CS phase again. restore needed members.

While At it this patch fixes a BUG. Old code when sending a REQUEST_SENSE for a failed command. Would than return with cmd->resid == 0 which was the result of the REQUEST_SENSE command. The failing command's resid was lost. Now when would resid be interesting if not on a failing command?

One question. The BUG_ON at before last hunk. Is that guaranteed here? or I sould be safe with an if()?

---
 drivers/scsi/aha152x.c |   55 +++++++++++++++++++++++------------------------
 1 files changed, 27 insertions(+), 28 deletions(-)

index ea57ecd..c2f4823 100644
@@ -552,14 +552,11 @@ struct aha152x_hostdata {
 struct aha152x_scdata {
 	Scsi_Cmnd *next;	/* next sc in queue */
 	struct completion *done;/* semaphore to block on */
-	unsigned char cmd_len;
-	unsigned char cmnd[MAX_COMMAND_SIZE];
-	unsigned short use_sg;
-	unsigned request_bufflen;
-	void *request_buffer;
+	unsigned char aha_orig_cmd_len;
+	unsigned char aha_orig_cmnd[MAX_COMMAND_SIZE];
+	int aha_orig_resid;
 };
 
-
 /* access macros for hostdata */
 
 #define HOSTDATA(shpnt)		((struct aha152x_hostdata *) &shpnt->hostdata)

@@ -997,20 +994,11 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 			return FAILED;
 		}
 	} else {
-		struct aha152x_scdata *sc;
-
 		SCpnt->host_scribble = kmalloc(sizeof(struct aha152x_scdata), GFP_ATOMIC);
 		if(SCpnt->host_scribble==0) {
 			printk(ERR_LEAD "allocation failed\n", CMDINFO(SCpnt));
 			return FAILED;
 		}
-
-		sc = SCDATA(SCpnt);
-		memcpy(sc->cmnd, SCpnt->cmnd, sizeof(sc->cmnd));
-		sc->request_buffer  = SCpnt->request_buffer;
-		sc->request_bufflen = SCpnt->request_bufflen;
-		sc->use_sg          = SCpnt->use_sg;
-		sc->cmd_len         = SCpnt->cmd_len;
 	}
 
 	SCNEXT(SCpnt)		= NULL;

@@ -1023,10 +1011,16 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 	   SCp.buffers_residual : left buffers in list
 	   SCp.phase            : current state of the command */
 
-	if(phase & resetting) {
-		SCpnt->SCp.ptr           = NULL;
-		SCpnt->SCp.this_residual = 0;
-		SCpnt->resid             = 0;
+	if (phase & (check_condition|resetting)) {
+		if (phase & check_condition) {
+			SCpnt->SCp.ptr           = SCpnt->sense_buffer;
+			SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
+			SCpnt->resid             = sizeof(SCpnt->sense_buffer);
+		} else {
+			SCpnt->SCp.ptr           = NULL;
+			SCpnt->SCp.this_residual = 0;
+			SCpnt->resid             = 0;
+		}
 		SCpnt->SCp.buffer           = NULL;
 		SCpnt->SCp.buffers_residual = 0;
 	} else {

@@ -1563,11 +1557,9 @@ static void busfree_run(struct Scsi_Host *shpnt)
 #endif
 
 			/* restore old command */
-			memcpy(cmd->cmnd, sc->cmnd, sizeof(sc->cmnd));
-			cmd->request_buffer  = sc->request_buffer;
-			cmd->request_bufflen = sc->request_bufflen;
-			cmd->use_sg          = sc->use_sg;
-			cmd->cmd_len         = sc->cmd_len;
+			memcpy(cmd->cmnd, sc->aha_orig_cmnd, sizeof(cmd->cmnd));
+			cmd->cmd_len = sc->aha_orig_cmd_len;
+			cmd->resid = sc->aha_orig_resid;
 
 			cmd->SCp.Status = SAM_STAT_CHECK_CONDITION;
 

@@ -1583,12 +1575,22 @@ static void busfree_run(struct Scsi_Host *shpnt)
 #endif
 
 			if(!(DONE_SC->SCp.phase & not_issued)) {
+				struct aha152x_scdata *sc;
 				Scsi_Cmnd *ptr = DONE_SC;
 				DONE_SC=NULL;
 #if 0
 				DPRINTK(debug_eh, ERR_LEAD "requesting sense\n", CMDINFO(ptr));
 #endif
 
+				/* save old command */
+				sc = SCDATA(ptr);
+				/* It was allocated in aha152x_internal_queue? */
+				BUG_ON(!sc);
+				memcpy(sc->aha_orig_cmnd, ptr->cmnd,
+				                            sizeof(ptr->cmnd));
+				sc->aha_orig_cmd_len = ptr->cmd_len;
+				sc->aha_orig_resid = ptr->resid;
+
 				ptr->cmnd[0]         = REQUEST_SENSE;
 				ptr->cmnd[1]         = 0;
 				ptr->cmnd[2]         = 0;

@@ -1596,10 +1598,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
 				ptr->cmnd[4]         = sizeof(ptr->sense_buffer);
 				ptr->cmnd[5]         = 0;
 				ptr->cmd_len         = 6;
-				ptr->use_sg          = 0; 
-				ptr->request_buffer  = ptr->sense_buffer;
-				ptr->request_bufflen = sizeof(ptr->sense_buffer);
-			
+
 				DO_UNLOCK(flags);
 				aha152x_internal_queue(ptr, NULL, check_condition, ptr->scsi_done);
 				DO_LOCK(flags);

1.5.2.2.249.g45fd

And finally this is the regular !use_sg cleanup
and use of accessors.

---
 drivers/scsi/aha152x.c |   91 +++++++++++++++++++++++++-----------------------
 1 files changed, 47 insertions(+), 44 deletions(-)

index c2f4823..e1330a4 100644
@@ -322,6 +322,12 @@ static LIST_HEAD(aha152x_host_list);
                         (cmd) ? ((cmd)->device->id & 0x0f) : -1, \
 			(cmd) ? ((cmd)->device->lun & 0x07) : -1
 
+static inline void
+CMD_INC_RESID(struct scsi_cmnd *cmd, int inc)
+{
+	scsi_set_resid(cmd, scsi_get_resid(cmd) + inc);
+}
+
 #define DELAY_DEFAULT 1000
 
 #if defined(PCMCIA)

@@ -975,13 +981,13 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 #if defined(AHA152X_DEBUG)
 	if (HOSTDATA(shpnt)->debug & debug_queue) {
 		printk(INFO_LEAD "queue: %p; cmd_len=%d pieces=%d size=%u cmnd=",
-		       CMDINFO(SCpnt), SCpnt, SCpnt->cmd_len, SCpnt->use_sg, SCpnt->request_bufflen);
+		       CMDINFO(SCpnt), SCpnt, SCpnt->cmd_len,
+		       scsi_sg_count(SCpnt), scsi_bufflen(SCpnt));
 		__scsi_print_command(SCpnt->cmnd);
 	}
 #endif
 
 	SCpnt->scsi_done	= done;
-	SCpnt->resid 		= SCpnt->request_bufflen;
 	SCpnt->SCp.phase	= not_issued | phase;
 	SCpnt->SCp.Status	= 0x1; /* Ilegal status by SCSI standard */
 	SCpnt->SCp.Message	= 0;

@@ -1011,30 +1017,24 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 	   SCp.buffers_residual : left buffers in list
 	   SCp.phase            : current state of the command */
 
-	if (phase & (check_condition|resetting)) {
+	if ((phase & (check_condition|resetting)) || !scsi_sglist(SCpnt)) {
 		if (phase & check_condition) {
 			SCpnt->SCp.ptr           = SCpnt->sense_buffer;
 			SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
-			SCpnt->resid             = sizeof(SCpnt->sense_buffer);
+			scsi_set_resid(SCpnt, sizeof(SCpnt->sense_buffer));
 		} else {
 			SCpnt->SCp.ptr           = NULL;
 			SCpnt->SCp.this_residual = 0;
-			SCpnt->resid             = 0;
+			scsi_set_resid(SCpnt, 0);
 		}
 		SCpnt->SCp.buffer           = NULL;
 		SCpnt->SCp.buffers_residual = 0;
 	} else {
-	if (SCpnt->use_sg) {
-		SCpnt->SCp.buffer           = (struct scatterlist *) SCpnt->request_buffer;
+		scsi_set_resid(SCpnt, scsi_bufflen(SCpnt));
+		SCpnt->SCp.buffer           = scsi_sglist(SCpnt);
 		SCpnt->SCp.ptr              = SG_ADDRESS(SCpnt->SCp.buffer);
 		SCpnt->SCp.this_residual    = SCpnt->SCp.buffer->length;
-		SCpnt->SCp.buffers_residual = SCpnt->use_sg - 1;
-	} else {
-		SCpnt->SCp.ptr              = (char *) SCpnt->request_buffer;
-		SCpnt->SCp.this_residual    = SCpnt->request_bufflen;
-		SCpnt->SCp.buffer           = NULL;
-		SCpnt->SCp.buffers_residual = 0;
-	}
+		SCpnt->SCp.buffers_residual = scsi_sg_count(SCpnt) - 1;
 	}
 
 	DO_LOCK(flags);

@@ -1520,8 +1520,8 @@ static void busfree_run(struct Scsi_Host *shpnt)
 			/* target sent DISCONNECT */
 			DPRINTK(debug_selection, DEBUG_LEAD "target disconnected at %d/%d\n",
 				CMDINFO(CURRENT_SC),
-				CURRENT_SC->resid,
-				CURRENT_SC->request_bufflen);
+				scsi_get_resid(CURRENT_SC),
+				scsi_bufflen(CURRENT_SC));
 #if defined(AHA152X_STAT)
 			HOSTDATA(shpnt)->disconnections++;
 #endif

@@ -1559,7 +1559,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
 			/* restore old command */
 			memcpy(cmd->cmnd, sc->aha_orig_cmnd, sizeof(cmd->cmnd));
 			cmd->cmd_len = sc->aha_orig_cmd_len;
-			cmd->resid = sc->aha_orig_resid;
+			scsi_set_resid(cmd, sc->aha_orig_resid);
 
 			cmd->SCp.Status = SAM_STAT_CHECK_CONDITION;
 

@@ -1589,7 +1589,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
 				memcpy(sc->aha_orig_cmnd, ptr->cmnd,
 				                            sizeof(ptr->cmnd));
 				sc->aha_orig_cmd_len = ptr->cmd_len;
-				sc->aha_orig_resid = ptr->resid;
+				sc->aha_orig_resid = scsi_get_resid(ptr);
 
 				ptr->cmnd[0]         = REQUEST_SENSE;
 				ptr->cmnd[1]         = 0;

@@ -2174,7 +2174,8 @@ static void datai_init(struct Scsi_Host *shpnt)
 	DATA_LEN=0;
 	DPRINTK(debug_datai,
 		DEBUG_LEAD "datai_init: request_bufflen=%d resid=%d\n",
-		CMDINFO(CURRENT_SC), CURRENT_SC->request_bufflen, CURRENT_SC->resid);
+		CMDINFO(CURRENT_SC), scsi_bufflen(CURRENT_SC),
+		scsi_get_resid(CURRENT_SC));
 }
 
 static void datai_run(struct Scsi_Host *shpnt)

@@ -2287,11 +2288,12 @@ static void datai_run(struct Scsi_Host *shpnt)
 
 static void datai_end(struct Scsi_Host *shpnt)
 {
-	CURRENT_SC->resid -= GETSTCNT();
+	CMD_INC_RESID(CURRENT_SC, -GETSTCNT());
 
 	DPRINTK(debug_datai,
 		DEBUG_LEAD "datai_end: request_bufflen=%d resid=%d stcnt=%d\n",
-		CMDINFO(CURRENT_SC), CURRENT_SC->request_bufflen, CURRENT_SC->resid, GETSTCNT());
+		CMDINFO(CURRENT_SC), scsi_bufflen(CURRENT_SC),
+		scsi_get_resid(CURRENT_SC), GETSTCNT());
 
 	SETPORT(SXFRCTL0, CH1|CLRSTCNT);
 	SETPORT(DMACNTRL0, 0);

@@ -2312,11 +2314,12 @@ static void datao_init(struct Scsi_Host *shpnt)
 	SETPORT(SIMODE0, 0);
 	SETPORT(SIMODE1, ENSCSIPERR | ENSCSIRST | ENPHASEMIS | ENBUSFREE );
 
-	DATA_LEN = CURRENT_SC->resid;
+	DATA_LEN = scsi_get_resid(CURRENT_SC);
 
 	DPRINTK(debug_datao,
 		DEBUG_LEAD "datao_init: request_bufflen=%d; resid=%d\n",
-		CMDINFO(CURRENT_SC), CURRENT_SC->request_bufflen, CURRENT_SC->resid);
+		CMDINFO(CURRENT_SC), scsi_bufflen(CURRENT_SC),
+		scsi_get_resid(CURRENT_SC));
 }
 
 static void datao_run(struct Scsi_Host *shpnt)

@@ -2340,7 +2343,7 @@ static void datao_run(struct Scsi_Host *shpnt)
 			SETPORT(DMACNTRL0,WRITE_READ|ENDMA|_8BIT);
 			SETPORT(DATAPORT, *CURRENT_SC->SCp.ptr++);
 			CURRENT_SC->SCp.this_residual--;
-			CURRENT_SC->resid--;
+			CMD_INC_RESID(CURRENT_SC, -1);
 			SETPORT(DMACNTRL0,WRITE_READ|ENDMA);
 		}
 

@@ -2349,7 +2352,7 @@ static void datao_run(struct Scsi_Host *shpnt)
 			outsw(DATAPORT, CURRENT_SC->SCp.ptr, data_count);
 			CURRENT_SC->SCp.ptr           += 2 * data_count;
 			CURRENT_SC->SCp.this_residual -= 2 * data_count;
-			CURRENT_SC->resid             -= 2 * data_count;
+			CMD_INC_RESID(CURRENT_SC, -2 * data_count);
 	  	}
 
 		if(CURRENT_SC->SCp.this_residual==0 && CURRENT_SC->SCp.buffers_residual>0) {

@@ -2375,35 +2378,34 @@ static void datao_run(struct Scsi_Host *shpnt)
 static void datao_end(struct Scsi_Host *shpnt)
 {
 	if(TESTLO(DMASTAT, DFIFOEMP)) {
-		int data_count = (DATA_LEN - CURRENT_SC->resid) - GETSTCNT();
+		int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
+		                                                    GETSTCNT();
 
 		DPRINTK(debug_datao, DEBUG_LEAD "datao: %d bytes to resend (%d written, %d transferred)\n",
 			CMDINFO(CURRENT_SC),
 			data_count,
-			DATA_LEN-CURRENT_SC->resid,
+			DATA_LEN - scsi_get_resid(CURRENT_SC),
 			GETSTCNT());
 
-		CURRENT_SC->resid += data_count;
+		CMD_INC_RESID(CURRENT_SC, data_count);
 
-		if(CURRENT_SC->use_sg) {
-			data_count -= CURRENT_SC->SCp.ptr - SG_ADDRESS(CURRENT_SC->SCp.buffer);
-			while(data_count>0) {
-				CURRENT_SC->SCp.buffer--;
-				CURRENT_SC->SCp.buffers_residual++;
-				data_count -= CURRENT_SC->SCp.buffer->length;
-			}
-			CURRENT_SC->SCp.ptr           = SG_ADDRESS(CURRENT_SC->SCp.buffer) - data_count;
-			CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length + data_count;
-		} else {
-			CURRENT_SC->SCp.ptr           -= data_count;
-			CURRENT_SC->SCp.this_residual += data_count;
+		data_count -= CURRENT_SC->SCp.ptr -
+		                             SG_ADDRESS(CURRENT_SC->SCp.buffer);
+		while(data_count>0) {
+			CURRENT_SC->SCp.buffer--;
+			CURRENT_SC->SCp.buffers_residual++;
+			data_count -= CURRENT_SC->SCp.buffer->length;
 		}
+		CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) -
+		                                                     data_count;
+		CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length +
+		                                                     data_count;
 	}
 
 	DPRINTK(debug_datao, DEBUG_LEAD "datao_end: request_bufflen=%d; resid=%d; stcnt=%d\n",
 		CMDINFO(CURRENT_SC),
-		CURRENT_SC->request_bufflen,
-		CURRENT_SC->resid,
+		scsi_bufflen(CURRENT_SC),
+		scsi_get_resid(CURRENT_SC),
 		GETSTCNT());
 
 	SETPORT(SXFRCTL0, CH1|CLRCH1|CLRSTCNT);

@@ -2930,7 +2932,7 @@ static void show_command(Scsi_Cmnd *ptr)
 	__scsi_print_command(ptr->cmnd);
 
 	printk(KERN_DEBUG "); request_bufflen=%d; resid=%d; phase |",
-	       ptr->request_bufflen, ptr->resid);
+	       scsi_bufflen(ptr), scsi_get_resid(ptr));
 
 	if (ptr->SCp.phase & not_issued)
 		printk("not issued|");

@@ -3000,7 +3002,8 @@ static int get_command(char *pos, Scsi_Cmnd * ptr)
 		SPRINTF("0x%02x ", ptr->cmnd[i]);
 
 	SPRINTF("); resid=%d; residual=%d; buffers=%d; phase |",
-		ptr->resid, ptr->SCp.this_residual, ptr->SCp.buffers_residual);
+		scsi_get_resid(ptr), ptr->SCp.this_residual,
+		ptr->SCp.buffers_residual);
 
 	if (ptr->SCp.phase & not_issued)
 		SPRINTF("not issued|");

1.5.2.2.249.g45fd

Ooff I forgot to:

all of these patches.

I could test it on PCMCIA. However, the patches are against the scsi-misc git tree, correct? So I would have to clone that git tree first... that's not good.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

If you could do some testing it is grate. The first 3 patches do not need scsi-misc specifically. Any post 2.6.20 tree will do. The last patch could be done together with attached patch on any 2.6.22 tree. (Apply attached patch anywhere before the last patch (4/4) of the patchset) Though I do recommend scsi-misc tree.

Testing:
Doing a regular Mount, FS testing is good for checking the last (4/4) patch, and it would be good news if it works, but ...
I'm afraid that it will not exercise the code change all that much, since my patches touch the Error-handling and Reset code paths.

Maybe someone can help:
How can we cause an ->eh_device_reset_handler() call from scsi-ml?

As for the patch 3/4 we need a SAM_STAT_CHECK_CONDITION return from a device. If you have a device with known bad sectors than a dd over the bad block should exercise that. Again anyone can think of an easy way to exercise a Status==SAM_STAT_CHECK_CONDITION?

Thanks for any testing you can do. Even if it's only the first 3 it could be nice

Boaz

[SCSI] scsi_cmnd: add data buffer accessors

This adds a set of accessors for the scsi data buffer. This is in preparation for chaining sg lists and bidirectional requests

index a2e0c10..53e1705 100644
@@ -135,4 +135,21 @@ extern void scsi_kunmap_atomic_sg(void *virt);
 extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
 extern void scsi_free_sgtable(struct scatterlist *, int);
 
+#define scsi_sg_count(cmd) ((cmd)->use_sg)
+#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
+#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+
+static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
+{
+	cmd->resid = resid;
+}
+
+static inline int scsi_get_resid(struct scsi_cmnd *cmd)
+{
+	return cmd->resid;
+}
+
+#define scsi_for_each_sg(cmd, sg, nseg, __i)			\
+	for (__i = 0, sg = scsi_sglist(cmd); __i < (nseg); __i++, (sg)++)
+
 #endif /* _SCSI_SCSI_CMND_H */

The easiest way is a SG_SCSI_RESET ioctl on the sg device.

I took 2.6.22, backed out Christoph's aha152x.c patch (using completion for timeouts), added your 5 patches, and then tested. Does that sound OK?

I booted/tested 4 times. 2 Oopsen and 2 of could-never-mount-the-device-due-to-reset-problems. Log is attached.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Yes what you did is perfect
I was not aware of "Christoph's aha152x.c patch" is that in scsi-misc? I'm almost positive I did a git-pull before I sent the patches. Sorry about that.

Thank you for testing and helping me with this.

I found one thing that should definitely trash the Reset and do funny things. But I'm not sure it is it at all. If below does not work, could you pleas do a small bisect of my patches.

first patch first, than 2nd, 3rd ,accessors thing, and 4th

git-diff

index f2f0619..6762cc8 100644
@@ -1151,6 +1151,7 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 	struct Scsi_Host *shpnt = SCpnt->device->host;
 	DECLARE_COMPLETION(done);
 	int ret, issued, disconnected;
+	unsigned short old_cmd_len = SCpnt->cmd_len;
 	unsigned long flags;
 	unsigned long timeleft;
 

@@ -1171,6 +1172,8 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 	disconnected = issued && remove_SC(&DISCONNECTED_SC, SCpnt);
 	DO_UNLOCK(flags);
 
+	SCpnt->cmd_len         = 0;
+
 	aha152x_internal_queue(SCpnt, &done, resetting, reset_done);
 
 	timeleft = wait_for_completion_timeout(&done, 100*HZ);

@@ -1181,6 +1184,8 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 		DO_UNLOCK(flags);
 	}
 
+	SCpnt->cmd_len         = old_cmd_len;
+
 	DO_LOCK(flags);
 
 	if(SCpnt->SCp.phase & resetted) {

Hi,

I don't see the reset problem after applying this patch, but I'm still seeing an Oops in datao_run. Log is attached.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Jul 18 09:11:24 dragon kernel: klogd 1.4.1, log source = /proc/kmsg started.
Jul 18 09:11:24 dragon kernel: [   22.360000] Adding 2104472k swap on /dev/sda6.  Priority:-1 extents:1 across:2104472k
Jul 18 09:18:22 dragon kernel: [  472.804000] SysRq : Changing Loglevel
Jul 18 09:18:22 dragon kernel: [  472.804000] Loglevel set to 9
Jul 18 09:18:26 dragon kernel: [  476.380000] pccard: PCMCIA card inserted into slot 0
Jul 18 09:18:26 dragon kernel: [  476.380000] cs: memory probe 0xdfc00000-0xdfcfffff: excluding 0xdfc00000-0xdfc0ffff 0xdfcf0000-0xdfcfffff
Jul 18 09:18:26 dragon kernel: [  476.384000] pcmcia: registering new device pcmcia0.0
Jul 18 09:18:27 dragon kernel: [  476.536000] aha152x: resetting bus...
Jul 18 09:18:27 dragon kernel: [  476.892000] aha152x2: vital data: rev=1, io=0x2340 (0x2340/0x2340), irq=3, scsiid=7, reconnect=enabled, parity=enabled, synchronous=enabled, delay=100, extended translation=disabled
Jul 18 09:18:28 dragon kernel: [  476.892000] aha152x2: trying software interrupt, ok.
Jul 18 09:18:28 dragon kernel: [  477.900000] scsi2 : Adaptec 152x SCSI driver; $Revision: 2.7 $
Jul 18 09:18:29 dragon kernel: [  478.956000] (scsi2:4:0) Synchronous Data Transfer Request period = 200 ns offset = 8 
Jul 18 09:18:29 dragon kernel: [  478.956000] scsi 2:0:4:0: Direct-Access     iomega   jaz 2GB          E.17 PQ: 0 ANSI: 2
Jul 18 09:18:32 dragon udevd-event[6207]: wait_for_sysfs: waiting for '/sys/devices/platform/host2/target2:0:4/2:0:4:0/ioerr_cnt' failed
Jul 18 09:18:38 dragon kernel: [  478.960000] sd 2:0:4:0: [sdb] Spinning up disk............ready
Jul 18 09:18:38 dragon kernel: [  488.000000] sd 2:0:4:0: [sdb] 2091050 512-byte hardware sectors (1071 MB)
Jul 18 09:18:38 dragon kernel: [  488.004000] sd 2:0:4:0: [sdb] Write Protect is off
Jul 18 09:18:38 dragon kernel: [  488.004000] sd 2:0:4:0: [sdb] Mode Sense: 39 00 10 08
Jul 18 09:18:38 dragon kernel: [  488.008000] sd 2:0:4:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
Jul 18 09:18:38 dragon kernel: [  488.012000] sd 2:0:4:0: [sdb] 2091050 512-byte hardware sectors (1071 MB)
Jul 18 09:18:38 dragon kernel: [  488.012000] sd 2:0:4:0: [sdb] Write Protect is off
Jul 18 09:18:38 dragon kernel: [  488.012000] sd 2:0:4:0: [sdb] Mode Sense: 39 00 10 08
Jul 18 09:18:38 dragon kernel: [  488.016000] sd 2:0:4:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
Jul 18 09:18:38 dragon kernel: [  488.016000]  sdb: sdb4
Jul 18 09:18:38 dragon kernel: [  488.048000] sd 2:0:4:0: [sdb] Attached SCSI removable disk
Jul 18 09:18:38 dragon kernel: [  488.052000] sd 2:0:4:0: Attached scsi generic sg2 type 0
Jul 18 09:18:38 dragon kernel: [  488.624000] pcmcia: Detected deprecated PCMCIA ioctl usage from process: hald.
Jul 18 09:18:38 dragon kernel: [  488.624000] pcmcia: This interface will soon be removed from the kernel; please expect breakage unless you upgrade to new tools.
Jul 18 09:18:38 dragon kernel: [  488.624000] pcmcia: see http://www.kernel.org/pub/linux/utils/kernel/pcmcia/pcmcia.html for details.
Jul 18 09:19:13 dragon kernel: [  523.408000] kjournald starting.  Commit interval 5 seconds
Jul 18 09:19:13 dragon kernel: [  523.408000] EXT3-fs warning: checktime reached, running e2fsck is recommended
Jul 18 09:19:13 dragon kernel: [  523.492000] EXT3 FS on sdb4, internal journal
Jul 18 09:19:13 dragon kernel: [  523.492000] EXT3-fs: recovery complete.
Jul 18 09:19:13 dragon kernel: [  523.556000] EXT3-fs: mounted filesystem with ordered data mode.
Jul 18 09:20:18 dragon kernel: [  588.000000] BUG: unable to handle kernel NULL pointer dereference at virtual address 00000000
Jul 18 09:20:18 dragon kernel: [  588.000000]  printing eip:
Jul 18 09:20:18 dragon kernel: [  588.000000] f8879284
Jul 18 09:20:18 dragon kernel: [  588.000000] *pde = 00000000
Jul 18 09:20:18 dragon kernel: [  588.000000] Oops: 0000 [#1]
Jul 18 09:20:18 dragon kernel: [  588.000000] SMP 
Jul 18 09:20:18 dragon kernel: [  588.000000] Modules linked in: aha152x_cs loop sr_mod yenta_socket rsrc_nonstatic pcmcia pcmcia_core ipw2200
Jul 18 09:20:18 dragon kernel: [  588.000000] CPU:    0
Jul 18 09:20:18 dragon kernel: [  588.000000] EIP:    0060:[<f8879284>]    Not tainted VLI
Jul 18 09:20:18 dragon kernel: [  588.000000] EFLAGS: 00010206   (2.6.22 #4)
Jul 18 09:20:18 dragon kernel: [  588.000000] EIP is at datao_run+0xd1/0x1f3 [aha152x_cs]
Jul 18 09:20:18 dragon kernel: [  588.000000] eax: c271abe0   ebx: c27078f4   ecx: 00000040   edx: 00002356
Jul 18 09:20:18 dragon kernel: [  588.000000] esi: 00000000   edi: 00000040   ebp: c252bf28   esp: c252bef4
Jul 18 09:20:18 dragon kernel: [  588.000000] ds: 007b   es: 007b   fs: 00d8  gs: 0000  ss: 0068
Jul 18 09:20:18 dragon kernel: [  588.000000] Process events/0 (pid: 6, ti=c252a000 task=c2506030 task.ti=c252a000)
Jul 18 09:20:18 dragon kernel: [  588.000000] Stack: 00000282 c27078f4 c252bf28 f8878ce8 c24d8558 00000000 c2707560 00002340 
Jul 18 09:20:18 dragon kernel: [  588.000000]        c24ded90 00000286 c27078f4 00000001 00000000 c252bf64 f8877332 000001f4 
Jul 18 09:20:18 dragon kernel: [  588.000000]        c2405b20 c252bf48 c0126a7d c24db680 0000234c c2707560 00011940 00000001 
Jul 18 09:20:18 dragon kernel: [  588.000000] Call Trace:
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c0103553>] show_trace_log_lvl+0x1a/0x2f
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c0103605>] show_stack_log_lvl+0x9d/0xa5
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c01037d0>] show_registers+0x1c3/0x295
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c01039b3>] die+0x111/0x213
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c033adc8>] do_page_fault+0x43c/0x50f
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c033975a>] error_code+0x72/0x78
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<f8877332>] run+0x378/0x450 [aha152x_cs]
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c0126322>] run_workqueue+0x80/0x105
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c0126bfd>] worker_thread+0xcb/0xd6
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c01291a5>] kthread+0x3b/0x62
Jul 18 09:20:18 dragon kernel: [  588.000000]  [<c01031ef>] kernel_thread_helper+0x7/0x10
Jul 18 09:20:18 dragon kernel: [  588.000000]  =======================
Jul 18 09:20:18 dragon kernel: [  588.000000] Code: 40 03 00 00 83 c2 12 ee 83 fe 01 7e 44 8b 43 04 89 f7 d1 ff 89 f9 8b 90 f8 00 00 00 89 55 e0 8b 93 40 03 00 00 8b 75 e0 83 c2 16 <f3> 66 6f 8b 75 e0 8d 14 3f 6b cf fe 01 d6 89 b0 f8 00 00 00 8b 
Jul 18 09:20:18 dragon kernel: [  588.000000] EIP: [<f8879284>] datao_run+0xd1/0x1f3 [aha152x_cs] SS:ESP 0068:c252bef4
Jul 18 09:20:48 dragon kernel: [  618.000000] (scsi2:4:0) cannot abort running or disconnected command
Jul 18 09:20:48 dragon kernel: [  618.000000] (scsi2:4:0) cannot reset current device
Jul 18 09:21:28 dragon kernel: [  658.016000] sd 2:0:4:0: scsi: Device offlined - not ready after error recovery
Jul 18 09:21:28 dragon kernel: [  658.016000] sd 2:0:4:0: [sdb] Result: hostbyte=DID_OK driverbyte=DRIVER_TIMEOUT,SUGGEST_OK
Jul 18 09:21:28 dragon kernel: [  658.016000] end_request: I/O error, dev sdb, sector 507944
Jul 18 09:21:28 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63489
Jul 18 09:21:28 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:28 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63490
Jul 18 09:21:28 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:28 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63491
Jul 18 09:21:28 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:28 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63492
Jul 18 09:21:28 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:28 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63493
Jul 18 09:21:28 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:28 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63494
Jul 18 09:21:28 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:28 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63495
Jul 18 09:21:28 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:29 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63496
Jul 18 09:21:29 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:29 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63497
Jul 18 09:21:29 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:29 dragon kernel: [  658.016000] Buffer I/O error on device sdb4, logical block 63498
Jul 18 09:21:29 dragon kernel: [  658.016000] lost page write due to I/O error on sdb4
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device
Jul 18 09:21:29 dragon kernel: [  658.016000] Aborting journal on device sdb4.
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: [sdb] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK,SUGGEST_OK
Jul 18 09:21:29 dragon kernel: [  658.016000] end_request: I/O error, dev sdb, sector 524320
Jul 18 09:21:29 dragon kernel: [  658.016000] EXT3-fs error (device sdb4) in ext3_ordered_writepage: IO failure
Jul 18 09:21:29 dragon kernel: [  658.016000] ext3_abort called.
Jul 18 09:21:29 dragon kernel: [  658.016000] EXT3-fs error (device sdb4): ext3_journal_start_sb: Detected aborted journal
Jul 18 09:21:29 dragon kernel: [  658.016000] Remounting filesystem read-only
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device
Jul 18 09:21:29 dragon kernel: [  658.016000] sd 2:0:4:0: rejecting I/O to offline device

More log after I added a printk() in datao_run:
See <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< line.

[   95.424000] aha152x datao_run: data_count=128, this_residual=1536, cur.ptr=0xf644ba00
[   95.428000] aha152x datao_run: data_count=128, this_residual=1408, cur.ptr=0xf644ba80
[   95.432000] aha152x datao_run: data_count=128, this_residual=1280, cur.ptr=0xf644bb00
[   95.436000] aha152x datao_run: data_count=128, this_residual=1152, cur.ptr=0xf644bb80
[   95.440000] aha152x datao_run: data_count=128, this_residual=1024, cur.ptr=0xf644bc00
[   95.444000] aha152x datao_run: data_count=128, this_residual=896, cur.ptr=0xf644bc80
[   95.452000] aha152x datao_run: data_count=128, this_residual=768, cur.ptr=0xf644bd00
[   95.456000] aha152x datao_run: data_count=128, this_residual=640, cur.ptr=0xf644bd80
[   95.460000] aha152x datao_run: data_count=128, this_residual=512, cur.ptr=0xf644be00
[   95.464000] aha152x datao_run: data_count=128, this_residual=384, cur.ptr=0xf644be80
[   95.468000] aha152x datao_run: data_count=128, this_residual=256, cur.ptr=0xf644bf00
[   95.472000] aha152x datao_run: data_count=128, this_residual=128, cur.ptr=0xf644bf80
[  112.816000] aha152x datao_run: data_count=128, this_residual=4096, cur.ptr=0x00000000 <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
[  112.820000] BUG: unable to handle kernel NULL pointer dereference at virtual address 00000000
[  112.824000]  printing eip:
[  112.828000] f88792a0
[  112.828000] *pde = 00000000
[  112.832000] Oops: 0000 [#1]
[  112.832000] SMP 
[  112.832000] Modules linked in: aha152x_cs loop sr_mod yenta_socket rsrc_nonstatic pcmcia pcmcia_core ipw2200
[  112.832000] CPU:    0
[  112.832000] EIP:    0060:[<f88792a0>]    Not tainted VLI
[  112.832000] EFLAGS: 00010206   (2.6.22 #6)
[  112.832000] EIP is at datao_run+0xed/0x20f [aha152x_cs]
[  112.832000] eax: c26f6808   ebx: f7a15a34   ecx: 00000040   edx: 00002356
[  112.832000] esi: 00000000   edi: 00000040   ebp: c252bf28   esp: c252bef4
[  112.832000] ds: 007b   es: 007b   fs: 00d8  gs: 0000  ss: 0068
[  112.832000] Process events/0 (pid: 6, ti=c252a000 task=c2506030 task.ti=c252a000)
[  112.832000] Stack: f887ad00 00000080 00001000 00000000 ffff49ff 00000000 f7a156a0 00002340 
[  112.832000]        c24ded90 00000286 f7a15a34 00000001 00000000 c252bf64 f8877332 c24d8558 
[  112.832000]        c252bf4c c0126a55 000000fa c2405ae0 0000234c f7a156a0 ffff4934 00000001 
[  112.832000] Call Trace:
[  112.832000]  [<c0103553>] show_trace_log_lvl+0x1a/0x2f
[  112.832000]  [<c0103605>] show_stack_log_lvl+0x9d/0xa5
[  112.832000]  [<c01037d0>] show_registers+0x1c3/0x295
[  112.832000]  [<c01039b3>] die+0x111/0x213
[  112.832000]  [<c033afb0>] do_page_fault+0x43c/0x50f
[  112.832000]  [<c0339942>] error_code+0x72/0x78
[  112.832000]  [<f8877332>] run+0x378/0x450 [aha152x_cs]
[  112.832000]  [<c0126322>] run_workqueue+0x80/0x105
[  112.832000]  [<c0126bfd>] worker_thread+0xcb/0xd6
[  112.832000]  [<c01291a5>] kthread+0x3b/0x62
[  112.832000]  [<c01031ef>] kernel_thread_helper+0x7/0x10
[  112.832000]  =======================
[  112.832000] Code: 8b 93 40 03 00 00 83 c2 12 ee 83 ff 01 7e 42 8b 43 04 d1 ff 89 f9 8b 90 f8 00 00 00 89 55 e0 8b 93 40 03 00 00 8b 75 e0 83 c2 16 <f3> 66 6f 8b 75 e0 8d 14 3f 6b cf fe 01 d6 89 b0 f8 00 00 00 8b 
[  112.832000] EIP: [<f88792a0>] datao_run+0xed/0x20f [aha152x_cs] SS:ESP 0068:c252bef4
[  172.816000] (scsi2:4:0) cannot abort running or disconnected command
[  172.820000] (scsi2:4:0) cannot reset current device

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Looks to me like this driver doesn't handle highmem virtual addresses very well. I expect that it would work OK if CONFIG_HIGHMEM=n or it had a slave_alloc function to limit the memory addresses that it sees (like ppa & imm do). As it is on a machine with HIGHMEM=y, it gets sglist virtual addresses of 0, e.g.:

[  527.416000] (scsi2:4:0) queue: c27331a0; cmd_len=10 pieces=13 size=53248 cmnd=Write(10) 2a 00 00 0c c0 20 00 00 68 00
[  527.416000] (scsi2:4:0) queue: buffer=sglist=c25ffc68, ptr=ADDR=00000000, this_resid=4096, bufs_resid=12
[  527.416000] queue segs[0], seg=c25ffc68, page=c2259e18, offset=0x0, virt=00000000
[  527.416000] queue segs[1], seg=c25ffc78, page=c23c94d8, offset=0x0, virt=00000000
[  527.416000] queue segs[2], seg=c25ffc88, page=c223c840, offset=0x0, virt=00000000
[  527.416000] queue segs[3], seg=c25ffc98, page=c2230248, offset=0x0, virt=00000000
[  527.416000] queue segs[4], seg=c25ffca8, page=c23c65a8, offset=0x0, virt=ff8af000
[  527.416000] queue segs[5], seg=c25ffcb8, page=c226d210, offset=0x0, virt=ff8fb000
[  527.416000] queue segs[6], seg=c25ffcc8, page=c2246908, offset=0x0, virt=00000000
[  527.416000] queue segs[7], seg=c25ffcd8, page=c22467a0, offset=0x0, virt=00000000
[  527.416000] queue segs[8], seg=c25ffce8, page=c2264de0, offset=0x0, virt=00000000
[  527.416000] queue segs[9], seg=c25ffcf8, page=c223c8e0, offset=0x0, virt=00000000
[  527.416000] queue segs[10], seg=c25ffd08, page=c23c8bf0, offset=0x0, virt=00000000
[  527.416000] queue segs[11], seg=c25ffd18, page=c2231e40, offset=0x0, virt=00000000
[  527.416000] queue segs[12], seg=c25ffd28, page=c22467c8, offset=0x0, virt=00000000

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Confirmed that limiting the blk queue bounce limit to BLK_BOUNCE_HIGH runs as expected (copy files, compare them successfully).

include/asm-i386/scatterlist.h talks about using pci_map_sg() to get bus addresses from the scatterlist table. However, this driver knows nothing about PCI. Should it use dma sg mapping instead? or what is the desired fix here?

Thanks.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

OK 1 bug down.

This is grate! Thanks for checking and debugging this. Am I right in assuming that the remaining bug has been there before my patchset? Has my patchset made it any worse?

I'm afraid this is out of my league. James, Christoph, anybody? What would be the right thing to do here?

Thank you. Please forgive me with the debug thing. One thing I learned in my 17 years of programing is never send code you have not compiled & tested. I mean what can go wrong with turning debugging on, right?;)

I will wait out the solution to above remaining problem and send another round of patches.

One more thing if I may? Do you have at hand an old Zip cartridge that has bad sectors or other errors on it. My motivation is to exercise the Sense return code-path. I would like to make sure that its doing the expected. It is the part that changed the most.
I will try to look if there is a way to send a scsi_read using sg3_utils that will read passed the media capacity, and will trigger a short read. I guess on an unmounted device. Douglas?

Thanks again
Boaz

Yes, this problem has been around forever AFAIK. You didn't add to it.

No, I don't have any bad media that I know of.
I suggest that you look into the "Fault-injection framework" (CONFIG_FAULT_INJECTION) to see if it can inject some errors for you.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Do we need to file a bug report in Bugzilla or something, so people have a documentation of the work-around, until it is fixed?

I think that for now I will wrap it up as it is. Could you send me the right BUG_ON() in place, with a printk pointing users to a solution? and I'll redo all the patches.

Oh, I didn't realize there is one. OK nice I will look into it, thanks.

Boaz

Wouldn't hurt, I guess, but the SCSI people know about it.

I didn't add a BUG_ON(), so I don't have that patch.
(I just used a .slave_alloc function to force no high memory data addresses, just like ppa.c does.)

You could just do a BUG_ON() just before there is an outsw() or insw() to a NULL pointer, e.g., in datao_run:

		BUG_ON(CURRENT_SC->SCp.ptr);

or we could make building the driver depend on !HIGHMEM.

I prefer either of the !HIGHMEM or slave_alloc changes to adding a BUG_ON(). However, the SCSI people likely won't want to use the slave_alloc() change because then the driver may never get fixed. (Of course, it hasn't got fixed with the BUG happening either.)

Anyway, I'll re-read Documentation/DMA*.txt to see if I can fix it.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Hi Randy.

could you please send me the patch you have with the .slave_alloc solution for now. I would like to revise the patchset and send them for inclusion now. They do fix some problems and let users work. We can put a big fat warring and explanation about highmem in the commit log comments so people can watch out.

Thanks
Boaz

From: Randy Dunlap <[email protected]>

Cause highmem buffers to be bounced to low memory until this driver supports highmem addresses. Otherwise it just oopses on NULL buffer addresses.

---
 drivers/scsi/aha152x.c |    7 +++++++
 1 file changed, 7 insertions(+)

@@ -3476,6 +3476,12 @@ static int aha152x_proc_info(struct Scsi
 	return thislength < length ? thislength : length;
 }
 
+static int aha152x_adjust_queue(struct scsi_device *device)
+{
+	blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_HIGH);
+	return 0;
+}
+
 static struct scsi_host_template aha152x_driver_template = {
 	.module				= THIS_MODULE,
 	.name				= AHA152X_REVID,

@@ -3492,6 +3498,7 @@ static struct scsi_host_template aha152x
 	.sg_tablesize			= SG_ALL,
 	.cmd_per_lun			= 1,
 	.use_clustering			= DISABLE_CLUSTERING,
+	.slave_alloc			= aha152x_adjust_queue,
 };
 
 #if !defined(PCMCIA)

I have asked this once before, but I don't like the answer.

Currently aha152x.c only works with !HIGHMEM due to highmem-pages not being mapped into the driver's memory space. James and hch tell me that using DMA mapping functions is the way to fix this, but those appear to me to be for DMA (!!), and this driver is (only) using PIO.

I did try to use dma_map_sg() & dma_unmap_sg() here, but they end up giving the driver physical buffer addresses, which aren't what is needed for PIO. Should this driver be using
scsi_kmap_atomic_sg() and scsi_kunmap_atomic_sg()
or other API functions, or should the driver just use
kmap_atomic() + kunmap_atomic()
for each highmem page?

Thanks,
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

For DMA transfers ... which is where you give the device a bus physical address and a length and tell it to go off and perform the transfer on its own, you need to use the dma_map_ functions.

For PIO transfers, which are usually ones where you have to feed the data into a special device register, since the kernel needs access to do this, it has to be done via kmap_atomic()/kunmap_atomic(). So, if everything has to go via pio, you use a sequence like

scsi_for_each_sg(cmd, sg, max, i) {
	offset = sg->offset;
	len = sg->len;
	do {
		coffset = offset; clen =len;
		buf = scsi_kmap_atomic_sg(sg, 1, &coffset, &clen)
		offset -= coffset; clen -= len;
		<feed buf+coffset to/from PIO for clen>
		scsi_kunmap_atomic_sg(buf);
	} while (len > 0);
}

James

Thanks for the confirmation.

		->length;

why 1 ?

		       += ?

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

You're mapping a single segment at a time to do the PIO.

James

I'm hitting a WARN_ON(1) in scsi_kmap_atomic_sg():
(I added the sg address to make sure that it's the same one that I am passing in to scsi_kmap_atomic_sg())

scsi_kmap_atomic_sg: sg at: 0xc25fd180, bytes in sg: 36, requested offset 304, elements 1

WARNING: at drivers/scsi/scsi_lib.c:2246 scsi_kmap_atomic_sg()

The sg that I am passing in looks like:

data segs[0], sg=c25fd180, page=c18a9698, offset=0x130, dma_adr: 0x0, len=0x24

so I'm calling like so:

		coffset = offset; // offset = sg->offset; // == 0x130
		clen = len;       // len = sg->length; // == 36

		buf = scsi_kmap_atomic_sg(sg, 1, &coffset, &clen);

Or consider a 1-element sg(list) that wants to read 0x800 bytes into a page at page offset 0x800. How does scsi_kmap_atomic_sg() handle this?

thanks,
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***